From c95c22fb8a9b6042d35a1449f734ddf27488dc67 Mon Sep 17 00:00:00 2001 From: David Tivris <163727302+tivris@users.noreply.github.com> Date: Fri, 10 Apr 2026 22:41:58 +0300 Subject: [PATCH 1/2] fix(reasoning): respect user config and Effort::None for models without effort support Two bugs caused reasoning parameters to leak to models that don't support them (e.g. Claude Haiku 4.5), even when the user explicitly disabled reasoning in .forge.toml: 1. The reasoning config merge used agent-as-base with overwrite_none, so the forge agent's hardcoded `enabled: true` silently overrode the user's `enabled: false`. Reversed the merge direction to match the compact config pattern (user config as base, agent fills gaps). 2. Effort::None ("skip reasoning entirely") was mapped to OutputEffort::Low in the Anthropic DTO, actively enabling reasoning. Now returns (None, None) to skip reasoning, matching the OpenAI path. Closes #2940, addresses #2924. --- crates/forge_app/src/agent.rs | 63 ++++++++++++--- crates/forge_app/src/dto/anthropic/request.rs | 77 +++++++++++++++---- 2 files changed, 118 insertions(+), 22 deletions(-) diff --git a/crates/forge_app/src/agent.rs b/crates/forge_app/src/agent.rs index 30562da060..5273008d9c 100644 --- a/crates/forge_app/src/agent.rs +++ b/crates/forge_app/src/agent.rs @@ -144,7 +144,7 @@ impl AgentExt for Agent { } // Apply workflow reasoning configuration to agents. - // Agent-level fields take priority; config fills in any unset fields. + // User config takes priority; agent defaults fill in any unset fields. if let Some(ref config_reasoning) = config.reasoning { use forge_config::Effort as ConfigEffort; let config_as_domain = ReasoningConfig { @@ -161,9 +161,9 @@ impl AgentExt for Agent { exclude: config_reasoning.exclude, enabled: config_reasoning.enabled, }; - // Start from the agent's own settings and fill unset fields from config. - let mut merged = agent.reasoning.clone().unwrap_or_default(); - merged.merge(config_as_domain); + // Start from user config; agent defaults fill unset fields. + let mut merged = config_as_domain; + merged.merge(agent.reasoning.clone().unwrap_or_default()); agent.reasoning = Some(merged); } @@ -208,10 +208,10 @@ mod tests { assert_eq!(actual, expected); } - /// When the agent already has reasoning fields set, those fields take - /// priority; config only fills in fields the agent left unset. + /// When both the agent and config set reasoning fields, user config + /// takes priority; agent values only fill in fields the config left unset. #[test] - fn test_reasoning_agent_fields_take_priority_over_config() { + fn test_reasoning_config_fields_take_priority_over_agent() { let config = ForgeConfig::default().reasoning( ConfigReasoningConfig::default() .enabled(true) @@ -219,18 +219,63 @@ mod tests { .max_tokens(1024_usize), ); - // Agent overrides effort but leaves enabled and max_tokens unset. + // Agent overrides effort but config takes priority. let agent = fixture_agent().reasoning(ReasoningConfig::default().effort(Effort::High)); let actual = agent.apply_config(&config).reasoning; let expected = Some( ReasoningConfig::default() - .effort(Effort::High) // agent's value wins + .effort(Effort::Low) // config's value wins .enabled(true) // filled in from config .max_tokens(1024_usize), // filled in from config ); assert_eq!(actual, expected); } + + /// When the user sets enabled=false in config, it overrides the agent's + /// enabled=true. This is the core scenario from issue #2924. + #[test] + fn test_reasoning_config_enabled_false_overrides_agent() { + let config = ForgeConfig::default().reasoning( + ConfigReasoningConfig::default() + .enabled(false) + .effort(ConfigEffort::None), + ); + + // Agent has enabled=true (like forge.md). + let agent = fixture_agent().reasoning(ReasoningConfig::default().enabled(true)); + + let actual = agent.apply_config(&config).reasoning; + + let expected = Some(ReasoningConfig::default().enabled(false).effort(Effort::None)); + + assert_eq!(actual, expected); + } + + /// When the user config leaves fields unset, the agent's defaults fill them. + #[test] + fn test_reasoning_agent_fills_unset_config_fields() { + let config = ForgeConfig::default() + .reasoning(ConfigReasoningConfig::default().effort(ConfigEffort::Medium)); + + // Agent has enabled=true and max_tokens=2048; config has neither. + let agent = fixture_agent().reasoning( + ReasoningConfig::default() + .enabled(true) + .max_tokens(2048_usize), + ); + + let actual = agent.apply_config(&config).reasoning; + + let expected = Some( + ReasoningConfig::default() + .effort(Effort::Medium) // from config + .enabled(true) // from agent (config had None) + .max_tokens(2048_usize), // from agent (config had None) + ); + + assert_eq!(actual, expected); + } } diff --git a/crates/forge_app/src/dto/anthropic/request.rs b/crates/forge_app/src/dto/anthropic/request.rs index f6c34c6f7c..292fff57ec 100644 --- a/crates/forge_app/src/dto/anthropic/request.rs +++ b/crates/forge_app/src/dto/anthropic/request.rs @@ -144,19 +144,24 @@ impl TryFrom for Request { None, ) } else if let Some(effort) = reasoning.effort { - // Effort without budget → newer output_config API. - let output_effort = match effort { - forge_domain::Effort::Low => OutputEffort::Low, - forge_domain::Effort::High => OutputEffort::High, - forge_domain::Effort::Max => OutputEffort::Max, - // Map unsupported variants to the nearest Anthropic-valid effort. - forge_domain::Effort::None | forge_domain::Effort::Minimal => { - OutputEffort::Low - } - forge_domain::Effort::Medium => OutputEffort::Medium, - forge_domain::Effort::XHigh => OutputEffort::Max, - }; - (None, Some(OutputConfig { effort: output_effort })) + if matches!(effort, forge_domain::Effort::None) { + // "None" means skip reasoning entirely. + (None, None) + } else { + // Effort without budget → newer output_config API. + let output_effort = match effort { + forge_domain::Effort::Low | forge_domain::Effort::Minimal => { + OutputEffort::Low + } + forge_domain::Effort::Medium => OutputEffort::Medium, + forge_domain::Effort::High => OutputEffort::High, + forge_domain::Effort::Max | forge_domain::Effort::XHigh => { + OutputEffort::Max + } + forge_domain::Effort::None => unreachable!(), + }; + (None, Some(OutputConfig { effort: output_effort })) + } } else { // Enabled-only → thinking with default budget. ( @@ -680,6 +685,52 @@ mod tests { assert_eq!(actual.thinking, None); } + #[test] + fn test_effort_none_with_enabled_skips_reasoning() { + // Effort::None means "skip reasoning entirely", even if enabled=true. + let fixture = Context::default().reasoning(ReasoningConfig { + enabled: Some(true), + effort: Some(forge_domain::Effort::None), + max_tokens: None, + exclude: None, + }); + + let actual = Request::try_from(fixture).unwrap(); + + assert_eq!(actual.thinking, None); + assert_eq!(actual.output_config, None); + } + + #[test] + fn test_effort_none_with_disabled_skips_reasoning() { + let fixture = Context::default().reasoning(ReasoningConfig { + enabled: Some(false), + effort: Some(forge_domain::Effort::None), + max_tokens: None, + exclude: None, + }); + + let actual = Request::try_from(fixture).unwrap(); + + assert_eq!(actual.thinking, None); + assert_eq!(actual.output_config, None); + } + + #[test] + fn test_effort_minimal_maps_to_low() { + let fixture = Context::default().reasoning(ReasoningConfig { + enabled: Some(true), + effort: Some(forge_domain::Effort::Minimal), + max_tokens: None, + exclude: None, + }); + + let actual = Request::try_from(fixture).unwrap(); + + assert_eq!(actual.output_config, Some(OutputConfig { effort: OutputEffort::Low })); + assert_eq!(actual.thinking, None); + } + #[test] fn test_context_conversion_stream_defaults_to_true() { let fixture = Context::default(); From 2758b3109eabc574ff826b7440501d72fe081d92 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Fri, 10 Apr 2026 19:44:02 +0000 Subject: [PATCH 2/2] [autofix.ci] apply automated fixes --- crates/forge_app/src/agent.rs | 9 +++++++-- crates/forge_app/src/dto/anthropic/request.rs | 5 ++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/crates/forge_app/src/agent.rs b/crates/forge_app/src/agent.rs index 5273008d9c..54bc0055d2 100644 --- a/crates/forge_app/src/agent.rs +++ b/crates/forge_app/src/agent.rs @@ -249,12 +249,17 @@ mod tests { let actual = agent.apply_config(&config).reasoning; - let expected = Some(ReasoningConfig::default().enabled(false).effort(Effort::None)); + let expected = Some( + ReasoningConfig::default() + .enabled(false) + .effort(Effort::None), + ); assert_eq!(actual, expected); } - /// When the user config leaves fields unset, the agent's defaults fill them. + /// When the user config leaves fields unset, the agent's defaults fill + /// them. #[test] fn test_reasoning_agent_fills_unset_config_fields() { let config = ForgeConfig::default() diff --git a/crates/forge_app/src/dto/anthropic/request.rs b/crates/forge_app/src/dto/anthropic/request.rs index 292fff57ec..8bb96a373e 100644 --- a/crates/forge_app/src/dto/anthropic/request.rs +++ b/crates/forge_app/src/dto/anthropic/request.rs @@ -727,7 +727,10 @@ mod tests { let actual = Request::try_from(fixture).unwrap(); - assert_eq!(actual.output_config, Some(OutputConfig { effort: OutputEffort::Low })); + assert_eq!( + actual.output_config, + Some(OutputConfig { effort: OutputEffort::Low }) + ); assert_eq!(actual.thinking, None); }