diff --git a/crates/forge_app/src/agent.rs b/crates/forge_app/src/agent.rs index 30562da060..54bc0055d2 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,68 @@ 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..8bb96a373e 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,55 @@ 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();