diff --git a/desktop/src-tauri/src/commands/agent_config.rs b/desktop/src-tauri/src/commands/agent_config.rs index 4df24e6e9ba..a49bd47465f 100644 --- a/desktop/src-tauri/src/commands/agent_config.rs +++ b/desktop/src-tauri/src/commands/agent_config.rs @@ -537,14 +537,19 @@ fn parse_models(raw: Option<&serde_json::Value>) -> (Vec, Option< /// Persist the canonical startup effort level for a local managed agent. /// -/// B5 (v4 direct-write): the panel's EffortPicker calls this directly to set the -/// effort a spawn will apply at next session start. The value is stored on the -/// record; at spawn `runtime.rs` injects it as `BUZZ_ACP_EFFORT_LEVEL` and the -/// harness applies it via `session/set_config_option` against the adapter's -/// advertised `thought_level` configId. Pass `None` to clear (adapter default). +/// The panel's EffortPicker calls this directly to set the effort a spawn will +/// apply at next session start. The value is stored on the harness-agnostic +/// `effort_level` record column; at spawn the launch projection +/// (`config_bridge::effort`, invoked from `runtime.rs`) resolves the effective +/// value and emits it under the destination runtime's native key +/// (`GOOSE_THINKING_EFFORT`, `BUZZ_AGENT_THINKING_EFFORT`, or the +/// `BUZZ_ACP_EFFORT_LEVEL` startup sentinel for Claude/Codex and keyless +/// adapters, which apply it via `session/set_config_option` against the +/// adapter's advertised `thought_level` configId). Pass `None` to clear +/// (reverts to the inherited/adapter default). /// -/// Rejects non-local backends: remote agents receive effort through `policy_env` -/// at deploy time (see `agents_deploy.rs`), never this local persistence path — +/// Rejects non-local backends: remote agents receive effort through the launch +/// projection at deploy time (see `agents_deploy.rs`), never this local persistence path — /// so an effort edit against a deployed agent is a caller error, not a silent /// no-op that leaves the panel and the running agent disagreeing. #[tauri::command] diff --git a/desktop/src-tauri/src/commands/agent_config_tests.rs b/desktop/src-tauri/src/commands/agent_config_tests.rs index 9c9aa58c1fd..5a19cfc0f9c 100644 --- a/desktop/src-tauri/src/commands/agent_config_tests.rs +++ b/desktop/src-tauri/src/commands/agent_config_tests.rs @@ -29,7 +29,7 @@ fn with_no_goose_config(body: impl FnOnce() -> T) -> T { } fn goose_runtime() -> &'static KnownAcpRuntime { - &KnownAcpRuntime { + static RUNTIME: KnownAcpRuntime = KnownAcpRuntime { id: "goose", label: "Goose", commands: &["goose"], @@ -55,13 +55,15 @@ fn goose_runtime() -> &'static KnownAcpRuntime { config_file_format: Some("yaml"), supports_acp_native_config: true, thinking_env_var: Some("GOOSE_THINKING_EFFORT"), + effort_normalization: Some(&crate::managed_agents::GOOSE_EFFORT_NORMALIZATION), max_tokens_env_var: Some("GOOSE_MAX_TOKENS"), context_limit_env_var: Some("GOOSE_CONTEXT_LIMIT"), max_rounds_env_var: None, required_normalized_fields: &["model", "provider"], login_hint: None, auth_probe_args: None, - } + }; + &RUNTIME } fn agent_record() -> ManagedAgentRecord { diff --git a/desktop/src-tauri/src/commands/agent_models_update.rs b/desktop/src-tauri/src/commands/agent_models_update.rs index bb045b81a24..c0199c440b1 100644 --- a/desktop/src-tauri/src/commands/agent_models_update.rs +++ b/desktop/src-tauri/src/commands/agent_models_update.rs @@ -115,15 +115,17 @@ pub async fn update_managed_agent( // Harness edit: the persona's runtime is authoritative, so an explicit // `agent_command_override` is persisted ONLY when the user picks a // command that diverges from the persona, and the empty/whitespace - // "Inherit from persona" sentinel clears both the pin and the - // materialized record runtime. A name-only edit + // "Inherit from persona" sentinel clears the pin, the materialized + // record runtime, AND the per-instance effort override (column here, + // env aliases after `env_vars` is applied below). A name-only edit // (`agent_command == None`) leaves the pin intact. `harness_override` // threads the user's explicit intent — see `apply_agent_command_update` // and `update_time_agent_command_override` for the full resolution // rules. + let mut inherit_transition = false; if let Some(agent_command) = input.agent_command { let personas = load_personas(&app).unwrap_or_default(); - crate::managed_agents::apply_agent_command_update( + inherit_transition = crate::managed_agents::apply_agent_command_update( record, &personas, &agent_command, @@ -136,10 +138,22 @@ pub async fn update_managed_agent( // mcp_command is intentionally not applied here — the effective MCP // command is always catalog-derived (known_acp_runtime at spawn time) // and the per-record field is never read by the runtime. - if let Some(env_vars) = input.env_vars { - crate::managed_agents::validate_user_env_keys(&env_vars)?; - record.env_vars = env_vars; + // + // Apply the caller-supplied `env_vars` (validated first), then — only on + // the pin→inherit transition — strip the record effort env aliases. The + // order is load-bearing: stripping AFTER the env replacement is what + // stops a same-request `env_vars` map from reintroducing a stale effort + // alias while the instance inherits its harness. The column was already + // cleared inside `apply_agent_command_update`. See + // `apply_env_vars_then_effort_transition` for the pinned invariant. + if let Some(ref env_vars) = input.env_vars { + crate::managed_agents::validate_user_env_keys(env_vars)?; } + crate::managed_agents::apply_env_vars_then_effort_transition( + record, + input.env_vars, + inherit_transition, + ); // Native provider/model fields are authoritative. Keep the typed marker // derived for new records while retaining legacy typed records for diff --git a/desktop/src-tauri/src/commands/agents_deploy.rs b/desktop/src-tauri/src/commands/agents_deploy.rs index da5bb3ba5c0..cd671fa6391 100644 --- a/desktop/src-tauri/src/commands/agents_deploy.rs +++ b/desktop/src-tauri/src/commands/agents_deploy.rs @@ -96,13 +96,13 @@ pub(super) fn build_launch_block( }; policy_env.insert(model_key.into(), value.to_string()); } - // I-4: remote parity for persisted startup effort. Mirrors the local spawn - // path in runtime.rs. The harness reads BUZZ_ACP_EFFORT_LEVEL into - // PoolStartup.startup_effort and applies it at first session creation via - // resolve_startup_effort(). - if let Some(ref value) = record.effort_level { - policy_env.insert("BUZZ_ACP_EFFORT_LEVEL".into(), value.clone()); - } + // Startup effort needs no remote-specific handling: the harness-agnostic + // effort projection already ran inside `resolve_effective_harness_descriptor`, + // so `descriptor.env` (→ `launch.env`, tier 2) carries exactly one effort key + // holding the effective value, with every foreign/legacy/transport effort key + // stripped. Tier 2 later-wins over `policy_env` (tier 1) and no authoritative + // tier-3 key collides with an effort key, so the projected value reaches the + // remote pod verbatim — identical authority to the local spawn. if let Some(value) = record.idle_timeout_seconds { policy_env.insert("BUZZ_ACP_IDLE_TIMEOUT".into(), value.to_string()); } @@ -119,14 +119,6 @@ pub(super) fn build_launch_block( policy_env.insert("BUZZ_ACP_TEAM_INSTRUCTIONS".into(), value); } - // B5 remote parity: when a canonical effort_level is persisted, strip - // BUZZ_ACP_EFFORT_LEVEL from launch.env so it cannot shadow the canonical - // value in policy_env (tier 1). In the k8s three-tier model tier 2 - // (launch.env) overwrites tier 1 (policy_env) — later-wins — so the key - // must be absent from tier 2 whenever a canonical value is present. - // When effort_level is None there is no canonical to protect, so user - // env passthrough stands (env may legitimately seed startup effort). - // // B2 remote parity: mirror the local A1 model authority. For a Claude // launch, ALWAYS strip BOTH BUZZ_ACP_MODEL and ANTHROPIC_MODEL from // launch.env — the resolved canonical model rides policy_env.ANTHROPIC_MODEL @@ -136,12 +128,15 @@ pub(super) fn build_launch_block( // canonical model. When no canonical model is present, neither key is in // policy_env, so stripping them keeps the remote process free of both — // matching local, where `apply_claude_model_env(None)` removes both. + // + // Effort keys need no stripping here: the projection already reduced + // `descriptor.env` to exactly one effort key holding the effective value, + // so launch.env carries the authority directly (see the effort note above). let is_claude = runtime.map(|r| r.id == "claude").unwrap_or(false); let strip_key = |k: &str| { - (record.effort_level.is_some() && k.eq_ignore_ascii_case("BUZZ_ACP_EFFORT_LEVEL")) - || (is_claude - && (k.eq_ignore_ascii_case("BUZZ_ACP_MODEL") - || k.eq_ignore_ascii_case("ANTHROPIC_MODEL"))) + is_claude + && (k.eq_ignore_ascii_case("BUZZ_ACP_MODEL") + || k.eq_ignore_ascii_case("ANTHROPIC_MODEL")) }; let launch_env: BTreeMap = descriptor .env @@ -467,19 +462,27 @@ mod tests { } #[test] - fn launch_block_claude_runtime_injects_effort_level_when_set() { - // I-4: remote parity — record.effort_level → BUZZ_ACP_EFFORT_LEVEL in policy_env. - let mut record = record(); - record.effort_level = Some("high".to_string()); + fn launch_block_claude_runtime_carries_projected_effort_in_launch_env() { + // Under the harness-agnostic projection, effort no longer rides + // policy_env: `resolve_effective_harness_descriptor` reduces + // `descriptor.env` to exactly one effort key (for a keyless claude + // runtime, the ACP sentinel) holding the effective value, and + // build_launch_block passes that env through to launch.env verbatim. + let record = record(); let descriptor = EffectiveHarnessDescriptor { command: "claude".into(), args: vec![], - env: BTreeMap::new(), + // The single projected effort key the descriptor resolver emits. + env: BTreeMap::from([("BUZZ_ACP_EFFORT_LEVEL".to_string(), "high".to_string())]), }; let launch = build_launch_block(&record, &descriptor, &[], None, None, "owner-hex"); assert_eq!( - launch["policy_env"]["BUZZ_ACP_EFFORT_LEVEL"], "high", - "claude remote must receive BUZZ_ACP_EFFORT_LEVEL when effort_level is set" + launch["env"]["BUZZ_ACP_EFFORT_LEVEL"], "high", + "the projected effort key must survive into launch.env" + ); + assert!( + launch["policy_env"]["BUZZ_ACP_EFFORT_LEVEL"].is_null(), + "effort is not a policy_env value under the projection design" ); } @@ -506,26 +509,35 @@ mod tests { /// authoritative. #[test] fn launch_block_canonical_effort_strips_user_env_collision() { + // Remote parity for the authority collision: the canonical column and a + // conflicting user `BUZZ_ACP_EFFORT_LEVEL` both present. The projection + // (run inside `resolve_effective_harness_descriptor`) resolves it — + // canonical `high` wins over the user `low` transport sentinel — and + // build_launch_block carries exactly that one value into launch.env, + // identical to the local spawn path. let mut record = record(); + record.runtime = Some("claude".into()); record.effort_level = Some("high".to_string()); - let descriptor = EffectiveHarnessDescriptor { - command: "claude".into(), - args: vec![], - // User-supplied conflicting value in descriptor.env. - env: BTreeMap::from([("BUZZ_ACP_EFFORT_LEVEL".to_string(), "low".to_string())]), - }; + record + .env_vars + .insert("BUZZ_ACP_EFFORT_LEVEL".into(), "low".into()); + let descriptor = crate::managed_agents::resolve_effective_harness_descriptor( + &record, + &[], + &Default::default(), + ) + .expect("claude descriptor resolves"); let launch = build_launch_block(&record, &descriptor, &[], None, None, "owner-hex"); - // Canonical must be in policy_env (tier 1). + // The projected canonical authority is the single effort value carried. assert_eq!( - launch["policy_env"]["BUZZ_ACP_EFFORT_LEVEL"], "high", - "canonical effort must be in policy_env when record.effort_level is Some" + launch["env"]["BUZZ_ACP_EFFORT_LEVEL"], "high", + "canonical effort must win the collision and reach launch.env" ); - // Conflicting user value must be absent from launch.env (tier 2) so it - // cannot shadow the canonical tier-1 value in build_env. + // Effort is not a policy_env value under the projection design. assert!( - launch["env"]["BUZZ_ACP_EFFORT_LEVEL"].is_null(), - "user BUZZ_ACP_EFFORT_LEVEL must be stripped from launch.env when canonical is present" + launch["policy_env"]["BUZZ_ACP_EFFORT_LEVEL"].is_null(), + "effort is carried in launch.env, never policy_env" ); } diff --git a/desktop/src-tauri/src/managed_agents/claude_config/mod.rs b/desktop/src-tauri/src/managed_agents/claude_config/mod.rs index 647ea56209e..0871544dbc3 100644 --- a/desktop/src-tauri/src/managed_agents/claude_config/mod.rs +++ b/desktop/src-tauri/src/managed_agents/claude_config/mod.rs @@ -4,15 +4,10 @@ //! local Claude Code agents. `BUZZ_ACP_MODEL` is removed from the spawned //! env so the harness never sees two model authorities simultaneously. //! -//! B5 contract: `BUZZ_ACP_EFFORT_LEVEL` is the canonical persisted startup -//! effort authority for all local agents. Written after `descriptor.env` so -//! user-supplied entries cannot shadow a persisted canonical value. - -/// The spawn-time env var carrying startup effort. Shared by the spawn -/// application ([`apply_effort_env`]) and the snapshot projection -/// (`spawn_snapshot::effective_effort`) so the value the harness receives and -/// the value the restart badge compares are named from one place. -pub const EFFORT_LEVEL_ENV_VAR: &str = "BUZZ_ACP_EFFORT_LEVEL"; +//! Startup effort is no longer applied here: the harness-agnostic effort +//! projection (`config_bridge::effort`) runs inside the descriptor resolver, so +//! `descriptor.env` already carries exactly one effort key. See that module for +//! the single-authority contract, including the ACP-startup key constant. /// Apply the A1 model authority: inject `ANTHROPIC_MODEL` from `effective_model` /// (or remove it if `None`) and strip `BUZZ_ACP_MODEL` from the spawned env. @@ -33,21 +28,6 @@ pub fn apply_claude_model_env(command: &mut std::process::Command, effective_mod } } -/// Apply the B5 effort authority: inject `BUZZ_ACP_EFFORT_LEVEL` from -/// `effort_level` (or leave it untouched if `None`). -/// -/// Must be called after `descriptor.env` is written so the canonical persisted -/// value wins over any user-supplied `BUZZ_ACP_EFFORT_LEVEL` entry. When -/// `effort_level` is `None` there is no canonical value to assert; the command -/// env is left untouched so a user-supplied value from `descriptor.env` -/// legitimately seeds startup effort. -pub fn apply_effort_env(command: &mut std::process::Command, effort_level: Option<&str>) { - if let Some(e) = effort_level { - command.env(EFFORT_LEVEL_ENV_VAR, e); - } - // None: no canonical value — leave whatever descriptor.env wrote intact. -} - #[cfg(test)] #[path = "tests.rs"] mod tests; diff --git a/desktop/src-tauri/src/managed_agents/claude_config/tests.rs b/desktop/src-tauri/src/managed_agents/claude_config/tests.rs index f6f0f90cb2d..0e596bc72b7 100644 --- a/desktop/src-tauri/src/managed_agents/claude_config/tests.rs +++ b/desktop/src-tauri/src/managed_agents/claude_config/tests.rs @@ -1,4 +1,4 @@ -use super::{apply_claude_model_env, apply_effort_env}; +use super::apply_claude_model_env; /// A1: BUZZ_ACP_MODEL must NOT be present in the spawned-child env after /// `apply_claude_model_env`, even if it was set before (dual-authority defect). @@ -54,74 +54,10 @@ fn a1_anthropic_model_removed_when_no_effective_model() { ); } -// ── B5 effort-authority contract tests ────────────────────────────────────── +// ── B5 effort-authority contract ───────────────────────────────────────────── // -// These tests verify that `apply_effort_env`, called after `descriptor.env`, -// makes the canonical persisted effort win over any user-supplied value. - -/// B5 (local): canonical effort wins when user env supplies a conflicting value. -/// Simulates the defect scenario: descriptor.env wrote BUZZ_ACP_EFFORT_LEVEL=low, -/// then apply_effort_env is called with the canonical "high". The canonical value -/// must be what survives in the spawned-child env. -#[test] -fn b5_canonical_effort_wins_over_user_env_collision() { - let mut cmd = std::process::Command::new("true"); - // Simulate descriptor.env writing a user-supplied value (the pre-fix - // ordering: effort written before the loop, then loop overwrote it, or - // equivalently: effort written post-loop but with user value also post-loop). - cmd.env("BUZZ_ACP_EFFORT_LEVEL", "low"); - - // Post-loop canonical application — the fix. - apply_effort_env(&mut cmd, Some("high")); - - let env_map: std::collections::HashMap<_, _> = cmd.get_envs().collect(); - let effort = env_map.get(std::ffi::OsStr::new("BUZZ_ACP_EFFORT_LEVEL")); - assert!(effort.is_some(), "BUZZ_ACP_EFFORT_LEVEL must be present"); - assert_eq!( - effort.unwrap().unwrap_or_default(), - "high", - "canonical effort must win over the user-supplied 'low' — B5 authority ordering" - ); -} - -/// B5 (local): when no canonical effort is persisted (effort_level is None), -/// user env passthrough is preserved — the descriptor.env entry seeds startup effort. -/// Simulates: descriptor.env wrote BUZZ_ACP_EFFORT_LEVEL=low (already in command), -/// then apply_effort_env(None) is called — user value must survive. -#[test] -fn b5_user_effort_env_survives_when_no_canonical_value() { - let mut cmd = std::process::Command::new("true"); - // Simulate descriptor.env loop having written a user-supplied value first. - cmd.env("BUZZ_ACP_EFFORT_LEVEL", "low"); - - // No canonical value — apply_effort_env(None) is a no-op so the user - // value already written by the descriptor.env loop survives intact. - apply_effort_env(&mut cmd, None); - - let env_map: std::collections::HashMap<_, _> = cmd.get_envs().collect(); - let effort = env_map.get(std::ffi::OsStr::new("BUZZ_ACP_EFFORT_LEVEL")); - assert!(effort.is_some(), "BUZZ_ACP_EFFORT_LEVEL must be present"); - assert_eq!( - effort.unwrap().unwrap_or_default(), - "low", - "user-supplied effort must survive when no canonical value is persisted" - ); -} - -/// B5 (local): canonical effort is present in the spawned env even when user -/// env did NOT supply a conflicting value (basic injection contract). -#[test] -fn b5_canonical_effort_injected_when_no_user_collision() { - let mut cmd = std::process::Command::new("true"); - // No user-supplied BUZZ_ACP_EFFORT_LEVEL in descriptor.env. - apply_effort_env(&mut cmd, Some("medium")); - - let env_map: std::collections::HashMap<_, _> = cmd.get_envs().collect(); - let effort = env_map.get(std::ffi::OsStr::new("BUZZ_ACP_EFFORT_LEVEL")); - assert!(effort.is_some(), "BUZZ_ACP_EFFORT_LEVEL must be present"); - assert_eq!( - effort.unwrap().unwrap_or_default(), - "medium", - "canonical effort must be injected when no collision" - ); -} +// Startup-effort application moved out of this module into the single +// harness-agnostic projection (`config_bridge::effort`). Its authority, +// collision, and single-key contract is exercised by +// `config_bridge::effort::tests`; there is no longer a Claude-local effort +// helper to test here. diff --git a/desktop/src-tauri/src/managed_agents/config_bridge/effort.rs b/desktop/src-tauri/src/managed_agents/config_bridge/effort.rs new file mode 100644 index 00000000000..7e7472cae63 --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/config_bridge/effort.rs @@ -0,0 +1,288 @@ +//! The single harness-agnostic effort authority (plan-of-record, PR #4625). +//! +//! ## One projection, one destination key, one snapshot leaf +//! +//! [`effort_launch_projection`] resolves the effective startup effort a spawn +//! would apply, over the canonical persisted column (`record.effort_level`) AND +//! the sanitized per-tier env inputs, in the CLEAR authority order: +//! +//! ```text +//! record native(valid) > canonical column(valid) > record legacy(valid) +//! > persona(native, then legacy) > global(native) > definition(native) +//! > baked(native) +//! ``` +//! +//! (The reader adds the live-ACP tier between column and persona and the config +//! file tier at the bottom; the launch projection has neither — a spawn reads +//! neither a running session nor the on-disk harness file.) +//! +//! The **tier-reading** native key is the runtime's real `thinking_env_var` +//! (`None` for Claude/Codex — those have no native key, so the column is the +//! sole authority and a user-supplied `BUZZ_ACP_EFFORT_LEVEL` is transport, not +//! a tier). The **emission** key ([`EffortLaunch::key`]) is +//! `thinking_env_var.unwrap_or(BUZZ_ACP_EFFORT_LEVEL)`: Goose emits +//! `GOOSE_THINKING_EFFORT`, buzz-agent emits `BUZZ_AGENT_THINKING_EFFORT`, +//! Claude/Codex/keyless-ACP and any unknown/custom runtime emit the retained +//! ACP-startup sentinel `BUZZ_ACP_EFFORT_LEVEL`. +//! +//! [`EffortLaunch::suppress`] lists every known native/legacy effort key plus +//! the sentinel; every consumer strips them all first, then emits at most the +//! one `key`. This is what guarantees a launched process, a remote payload, and +//! a restart snapshot can never carry two effort authorities. + +use std::collections::BTreeMap; + +use super::LEGACY_THINKING_EFFORT_KEY; +use crate::managed_agents::custom_harnesses::HarnessDefinition; +use crate::managed_agents::discovery::KnownAcpRuntime; +use crate::managed_agents::types::{AgentDefinition, ManagedAgentRecord}; + +/// The retained ACP-startup transport key. Claude, Codex, keyless ACP adapters, +/// and any unknown/custom runtime route the effective effort through this key +/// (the harness reads it into `PoolStartup.startup_effort`). It is *transport*, +/// never a value-authority tier: a user-supplied entry is suppressed and +/// overwritten by the projected effective value. +pub(crate) const ACP_STARTUP_EFFORT_KEY: &str = "BUZZ_ACP_EFFORT_LEVEL"; + +/// The resolved launch effort for one runtime: the single fact every spawn +/// path (local, remote, snapshot) consumes. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct EffortLaunch { + /// The final effective effort value, normalized for contract runtimes and + /// raw for contract-less ones, resolved over ALL tiers (column + env). + /// `None` when no tier supplies a value the destination can express. + pub value: Option, + /// The destination env key the value is emitted under. + pub key: &'static str, + /// Every effort key to strip from the launch env before emitting `key`. + /// Always includes the sentinel and all known native/legacy effort keys, so + /// no foreign or transport effort key can shadow the projected authority. + pub suppress: Vec<&'static str>, +} + +impl EffortLaunch { + /// Apply the projection to a launch env map: strip every `suppress` key, + /// then emit `key = value` when a value is present. After this call the map + /// holds at most one effort key (`key`), carrying the effective value. + pub(crate) fn apply(&self, env: &mut BTreeMap) { + for k in &self.suppress { + env.remove(*k); + } + if let Some(ref v) = self.value { + env.insert(self.key.to_string(), v.clone()); + } + } +} + +/// Resolve the single harness-agnostic effort authority and apply it to a fully +/// layered launch `env`: strip every known/legacy/transport effort key, then +/// emit exactly the one destination key holding the effective value. Called by +/// the descriptor resolver AFTER the full layer stack, so the launch env, the +/// remote deploy payload, and the restart snapshot all carry one effort key and +/// one value — no double authority, no foreign key, no launch/badge disagreement. +#[allow(clippy::too_many_arguments)] +pub(crate) fn apply_launch_effort( + env: &mut BTreeMap, + record: &ManagedAgentRecord, + runtime: Option<&KnownAcpRuntime>, + personas: &[AgentDefinition], + global_env: &BTreeMap, + harness_def: Option<&HarnessDefinition>, + baked_env: &BTreeMap, +) { + effort_launch_projection( + record, + runtime, + personas, + record.persona_id.as_deref(), + global_env, + harness_def, + baked_env, + ) + .apply(env); +} + +/// Resolve one effort tier's value, applying within-tier legacy aliasing and +/// normalization. Returns the canonical (or raw, contract-less) value, or +/// `None` when no usable candidate exists. +/// +/// Lookup (per tier, independent of other tiers): +/// 1. Native key — normalized; invalid → skip as absent. +/// 2. Legacy key (`BUZZ_AGENT_THINKING_EFFORT`) — only when the native key +/// differs from it AND `allow_legacy_alias` is set AND the value +/// normalizes. Invalid legacy is skipped so the next tier can supply one. +pub(crate) fn effort_tier_alias( + map: &BTreeMap, + native_key: &str, + norm: impl Fn(&str) -> Option, + allow_legacy_alias: bool, +) -> Option { + if let Some(raw) = map.get(native_key) { + if let Some(canonical) = norm(raw) { + return Some(canonical); + } + } + if allow_legacy_alias && native_key != LEGACY_THINKING_EFFORT_KEY { + if let Some(raw) = map.get(LEGACY_THINKING_EFFORT_KEY) { + if let Some(canonical) = norm(raw) { + return Some(canonical); + } + } + } + None +} + +/// The destination env key the effective effort is emitted under for `runtime`: +/// the runtime's native `thinking_env_var`, else the ACP-startup sentinel +/// (Claude, Codex, keyless ACP adapters, and unknown/custom runtimes). +pub(crate) fn effort_dest_key(runtime: Option<&KnownAcpRuntime>) -> &'static str { + runtime + .and_then(|r| r.thinking_env_var) + .unwrap_or(ACP_STARTUP_EFFORT_KEY) +} + +/// Every effort key to strip before emitting the single destination key: all +/// known native effort keys, the legacy alias, and the ACP-startup sentinel. +/// Stripping the full set guarantees no foreign or transport effort key can +/// shadow the projected authority. +pub(crate) fn effort_suppress_keys() -> Vec<&'static str> { + let mut keys: Vec<&'static str> = super::all_known_effort_keys().collect(); + if !keys.contains(&ACP_STARTUP_EFFORT_KEY) { + keys.push(ACP_STARTUP_EFFORT_KEY); + } + if !keys.contains(&LEGACY_THINKING_EFFORT_KEY) { + keys.push(LEGACY_THINKING_EFFORT_KEY); + } + keys +} + +/// Build the single effective-effort projection for a launch. +/// +/// `global_env`, `persona_id`+`personas`, `harness_def`, and `baked_env` supply +/// the same per-tier inputs the layered spawn env is built from; the projection +/// re-reads them so an invalid high-tier value skips as absent and a lower tier +/// can win (which a merged last-wins env map cannot express). +pub(crate) fn effort_launch_projection( + record: &ManagedAgentRecord, + runtime: Option<&KnownAcpRuntime>, + personas: &[AgentDefinition], + persona_id: Option<&str>, + global_env: &BTreeMap, + harness_def: Option<&HarnessDefinition>, + baked_env: &BTreeMap, +) -> EffortLaunch { + let key = effort_dest_key(runtime); + let suppress = effort_suppress_keys(); + + // Normalizer: contract runtimes canonicalize (invalid → skip); contract-less + // runtimes pass raw (any present value is valid for their per-model catalog). + let contract = runtime.and_then(|r| r.effort_normalization); + let norm = |raw: &str| -> Option { + match contract { + Some(c) => c.normalize_str(raw), + None => Some(raw.to_string()), + } + }; + + // Tier-reading native key: the runtime's REAL native key. `None` (Claude, + // Codex, unknown/custom) means there are no env-tier authorities — the + // sentinel in user env is transport only — so the column is the sole source. + let native_key = runtime.and_then(|r| r.thinking_env_var); + + let value = resolve_effective_effort( + record, + native_key, + &norm, + personas, + persona_id, + global_env, + harness_def, + baked_env, + ); + + EffortLaunch { + value, + key, + suppress, + } +} + +/// Resolve the effective effort value in CLEAR authority order (launch tiers). +#[allow(clippy::too_many_arguments)] +fn resolve_effective_effort( + record: &ManagedAgentRecord, + native_key: Option<&str>, + norm: &impl Fn(&str) -> Option, + personas: &[AgentDefinition], + persona_id: Option<&str>, + global_env: &BTreeMap, + harness_def: Option<&HarnessDefinition>, + baked_env: &BTreeMap, +) -> Option { + use crate::managed_agents::env_vars::{is_reserved_env_key, live_persona_env, merged_user_env}; + + // Sanitize env tiers exactly as the layered spawn env does (reserved/ + // malformed/NUL filtering), so the resolved authority matches what launches. + let record_env = merged_user_env(&BTreeMap::new(), &record.env_vars); + + // 1. record native — only for runtimes with a real native key. + if let Some(nk) = native_key { + if let Some(raw) = record_env.get(nk) { + if let Some(v) = norm(raw) { + return Some(v); + } + } + } + // 2. canonical column — normalized (raw passthrough for contract-less). + if let Some(raw) = record.effort_level.as_deref() { + if let Some(v) = norm(raw) { + return Some(v); + } + } + // 3. record legacy alias — only when the native key differs from it. + if let Some(nk) = native_key { + if nk != LEGACY_THINKING_EFFORT_KEY { + if let Some(raw) = record_env.get(LEGACY_THINKING_EFFORT_KEY) { + if let Some(v) = norm(raw) { + return Some(v); + } + } + } + } + // Env tiers below require a native key to read. + let nk = native_key?; + + // 4. persona (native, then legacy) — sanitized like the layered spawn env. + let persona_env = merged_user_env(&BTreeMap::new(), &live_persona_env(personas, persona_id)); + if let Some(v) = effort_tier_alias(&persona_env, nk, norm, true) { + return Some(v); + } + // 5. global (native only). + let global = merged_user_env(&BTreeMap::new(), global_env); + if let Some(v) = effort_tier_alias(&global, nk, norm, false) { + return Some(v); + } + // 6. definition (native only) — author-controlled; reserved keys stripped. + if let Some(def) = harness_def { + let def_env: BTreeMap = def + .env + .iter() + .filter(|(k, _)| !is_reserved_env_key(k)) + .map(|(k, v)| (k.clone(), v.clone())) + .collect(); + if let Some(v) = effort_tier_alias(&def_env, nk, norm, false) { + return Some(v); + } + } + // 7. baked build floor (native only). + if let Some(raw) = baked_env.get(nk) { + if let Some(v) = norm(raw) { + return Some(v); + } + } + None +} + +#[cfg(test)] +#[path = "effort_tests.rs"] +mod tests; diff --git a/desktop/src-tauri/src/managed_agents/config_bridge/effort_tests.rs b/desktop/src-tauri/src/managed_agents/config_bridge/effort_tests.rs new file mode 100644 index 00000000000..39a9d13e6b2 --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/config_bridge/effort_tests.rs @@ -0,0 +1,469 @@ +//! Parity matrix for the single harness-agnostic effort projection +//! (`effort_launch_projection`, PR #4625). +//! +//! Every spawn path — local (`runtime.rs`), remote deploy (`agents_deploy.rs`), +//! and restart snapshot (`spawn_snapshot.rs`) — consumes this one projection via +//! `descriptor.env`, so these tests are the authority contract for all three. +//! They pin, per runtime strategy: +//! +//! * the CLEAR authority order (record native > canonical column > record +//! legacy > persona > global > definition > baked); +//! * the decisive mixed-authority case (valid record-native + a different +//! valid column → the native value wins everywhere); +//! * `value == None` when no tier expresses a value the destination accepts; +//! * single-key emission + full-suppress on `apply`; +//! * the unknown/custom-runtime ACP-sentinel fallback. + +use std::collections::BTreeMap; + +use super::{effort_launch_projection, effort_suppress_keys, EffortLaunch}; +use crate::managed_agents::custom_harnesses::HarnessDefinition; +use crate::managed_agents::discovery::{known_acp_runtime_exact, KnownAcpRuntime}; +use crate::managed_agents::types::{AgentDefinition, ManagedAgentRecord}; + +const GOOSE_KEY: &str = "GOOSE_THINKING_EFFORT"; +const BUZZ_AGENT_KEY: &str = "BUZZ_AGENT_THINKING_EFFORT"; +const ACP_KEY: &str = "BUZZ_ACP_EFFORT_LEVEL"; + +fn goose() -> &'static KnownAcpRuntime { + known_acp_runtime_exact("goose").expect("goose runtime in catalog") +} +fn claude() -> &'static KnownAcpRuntime { + known_acp_runtime_exact("claude").expect("claude runtime in catalog") +} +fn buzz_agent() -> &'static KnownAcpRuntime { + known_acp_runtime_exact("buzz-agent").expect("buzz-agent runtime in catalog") +} + +fn record() -> ManagedAgentRecord { + ManagedAgentRecord { + pubkey: "test".to_string(), + name: "Test Agent".to_string(), + persona_id: None, + private_key_nsec: "".to_string(), + auth_tag: None, + relay_url: "ws://localhost:3000".to_string(), + avatar_url: None, + acp_command: "buzz-acp".to_string(), + agent_command: "goose".to_string(), + agent_args: vec![], + mcp_command: "".to_string(), + turn_timeout_seconds: 300, + idle_timeout_seconds: None, + max_turn_duration_seconds: None, + parallelism: 1, + system_prompt: None, + model: None, + env_vars: BTreeMap::new(), + start_on_app_launch: false, + auto_restart_on_config_change: true, + runtime_pid: None, + backend: crate::managed_agents::types::BackendKind::Local, + backend_agent_id: None, + provider_policy_pending: false, + provider_binary_path: None, + team_id: None, + persona_team_dir: None, + persona_name_in_team: None, + created_at: "".to_string(), + updated_at: "".to_string(), + last_started_at: None, + last_stopped_at: None, + last_exit_code: None, + last_error: None, + last_error_code: None, + respond_to: crate::managed_agents::types::RespondTo::OwnerOnly, + respond_to_allowlist: vec![], + display_name: None, + slug: None, + runtime: None, + name_pool: Vec::new(), + is_builtin: false, + is_active: true, + shared: false, + source_team: None, + source_team_persona_slug: None, + catalog_source: None, + definition_respond_to: None, + definition_respond_to_allowlist: Vec::new(), + definition_parallelism: None, + relay_mesh: None, + effort_level: None, + agent_command_override: None, + persona_source_version: None, + provider: None, + } +} + +fn env(pairs: &[(&str, &str)]) -> BTreeMap { + pairs + .iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect() +} + +fn persona(id: &str, env_vars: BTreeMap) -> AgentDefinition { + AgentDefinition { + id: id.to_string(), + display_name: "P".to_string(), + avatar_url: None, + system_prompt: String::new(), + runtime: None, + model: None, + provider: None, + name_pool: vec![], + is_builtin: false, + is_active: true, + shared: false, + source_team: None, + source_team_persona_slug: None, + catalog_source: None, + env_vars, + respond_to: None, + respond_to_allowlist: vec![], + parallelism: None, + created_at: String::new(), + updated_at: String::new(), + } +} + +fn harness_def(env: BTreeMap) -> HarnessDefinition { + HarnessDefinition { + id: "custom".to_string(), + label: "Custom".to_string(), + command: "custom".to_string(), + args: vec![], + env, + install_instructions_url: String::new(), + install_hint: String::new(), + } +} + +/// Convenience: project with no persona/global/definition/baked tiers. +fn project_record_only( + record: &ManagedAgentRecord, + runtime: Option<&KnownAcpRuntime>, +) -> EffortLaunch { + effort_launch_projection( + record, + runtime, + &[], + None, + &BTreeMap::new(), + None, + &BTreeMap::new(), + ) +} + +// -------------------------------------------------------------------------- +// Destination key + emission strategy per runtime +// -------------------------------------------------------------------------- + +#[test] +fn goose_emits_only_goose_key() { + let mut r = record(); + r.effort_level = Some("high".into()); + let launch = project_record_only(&r, Some(goose())); + assert_eq!(launch.value.as_deref(), Some("high")); + assert_eq!(launch.key, GOOSE_KEY); +} + +#[test] +fn claude_routes_canonical_through_acp_sentinel() { + let mut r = record(); + r.effort_level = Some("high".into()); + let launch = project_record_only(&r, Some(claude())); + // Claude has no native key: the column is the sole authority and it emits + // under the retained ACP-startup sentinel. + assert_eq!(launch.value.as_deref(), Some("high")); + assert_eq!(launch.key, ACP_KEY); +} + +#[test] +fn buzz_agent_passes_raw_contract_less_value_under_native_key() { + let mut r = record(); + // buzz-agent has no static normalization contract: a per-model value that + // Goose would reject (e.g. "minimal") passes through raw. + r.effort_level = Some("minimal".into()); + let launch = project_record_only(&r, Some(buzz_agent())); + assert_eq!(launch.value.as_deref(), Some("minimal")); + assert_eq!(launch.key, BUZZ_AGENT_KEY); +} + +#[test] +fn unknown_runtime_falls_back_to_acp_sentinel() { + let mut r = record(); + r.effort_level = Some("high".into()); + // No runtime metadata (custom/unknown adapter): preserve main's behavior — + // canonical routes through the raw ACP sentinel path. + let launch = project_record_only(&r, None); + assert_eq!(launch.value.as_deref(), Some("high")); + assert_eq!(launch.key, ACP_KEY); +} + +// -------------------------------------------------------------------------- +// CLEAR authority order + the decisive mixed-authority case +// -------------------------------------------------------------------------- + +#[test] +fn decisive_record_native_outranks_a_different_valid_column() { + // The mixed-authority pin Thufir/Will require: a valid record-native env + // key and a DIFFERENT valid canonical column must resolve to the + // record-native value — reader, local, remote, and snapshot all agree. + let mut r = record(); + r.env_vars = env(&[(GOOSE_KEY, "low")]); + r.effort_level = Some("high".into()); + let launch = project_record_only(&r, Some(goose())); + assert_eq!( + launch.value.as_deref(), + Some("low"), + "record-native env outranks the canonical column" + ); +} + +#[test] +fn canonical_column_wins_when_no_record_native() { + // No record-native key present: the column is the next tier and wins over + // lower tiers (here, persona). + let mut r = record(); + r.persona_id = Some("p".into()); + r.effort_level = Some("high".into()); + let personas = vec![persona("p", env(&[(GOOSE_KEY, "low")]))]; + let launch = effort_launch_projection( + &r, + Some(goose()), + &personas, + Some("p"), + &BTreeMap::new(), + None, + &BTreeMap::new(), + ); + assert_eq!(launch.value.as_deref(), Some("high")); +} + +#[test] +fn record_legacy_alias_wins_over_persona_for_goose() { + // Record legacy `BUZZ_AGENT_THINKING_EFFORT` outranks persona for a runtime + // whose native key differs from the legacy key. + let mut r = record(); + r.persona_id = Some("p".into()); + r.env_vars = env(&[(BUZZ_AGENT_KEY, "max")]); + let personas = vec![persona("p", env(&[(GOOSE_KEY, "low")]))]; + let launch = effort_launch_projection( + &r, + Some(goose()), + &personas, + Some("p"), + &BTreeMap::new(), + None, + &BTreeMap::new(), + ); + assert_eq!(launch.value.as_deref(), Some("max")); +} + +#[test] +fn persona_then_global_then_definition_then_baked_fall_through() { + // With no record tier set, each lower tier wins in order once the ones + // above it are absent. Verify persona > global by presence. + let mut r = record(); + r.persona_id = Some("p".into()); + let personas = vec![persona("p", env(&[(GOOSE_KEY, "high")]))]; + let global = env(&[(GOOSE_KEY, "low")]); + let launch = effort_launch_projection( + &r, + Some(goose()), + &personas, + Some("p"), + &global, + None, + &BTreeMap::new(), + ); + assert_eq!( + launch.value.as_deref(), + Some("high"), + "persona outranks global" + ); + + // Drop the persona value: global wins. + let personas = vec![persona("p", BTreeMap::new())]; + let launch = effort_launch_projection( + &r, + Some(goose()), + &personas, + Some("p"), + &global, + None, + &BTreeMap::new(), + ); + assert_eq!( + launch.value.as_deref(), + Some("low"), + "global outranks definition" + ); + + // Drop global too: definition wins. + let def = harness_def(env(&[(GOOSE_KEY, "medium")])); + let launch = effort_launch_projection( + &r, + Some(goose()), + &personas, + Some("p"), + &BTreeMap::new(), + Some(&def), + &BTreeMap::new(), + ); + assert_eq!( + launch.value.as_deref(), + Some("medium"), + "definition outranks baked" + ); + + // Drop definition: baked build floor wins. + let baked = env(&[(GOOSE_KEY, "off")]); + let launch = effort_launch_projection( + &r, + Some(goose()), + &personas, + Some("p"), + &BTreeMap::new(), + None, + &baked, + ); + assert_eq!(launch.value.as_deref(), Some("off")); +} + +// -------------------------------------------------------------------------- +// Normalization + skip-as-absent fall-through +// -------------------------------------------------------------------------- + +#[test] +fn goose_alias_column_xhigh_normalizes_to_max() { + let mut r = record(); + r.effort_level = Some("xhigh".into()); + let launch = project_record_only(&r, Some(goose())); + assert_eq!(launch.value.as_deref(), Some("max")); +} + +#[test] +fn invalid_goose_column_skips_and_falls_through_to_persona() { + // "minimal" is invalid for Goose: it skips as absent so the persona tier + // supplies the effective value (nondestructive switch policy relies on this). + let mut r = record(); + r.persona_id = Some("p".into()); + r.effort_level = Some("minimal".into()); + let personas = vec![persona("p", env(&[(GOOSE_KEY, "high")]))]; + let launch = effort_launch_projection( + &r, + Some(goose()), + &personas, + Some("p"), + &BTreeMap::new(), + None, + &BTreeMap::new(), + ); + assert_eq!(launch.value.as_deref(), Some("high")); +} + +#[test] +fn invalid_goose_value_with_no_lower_tier_is_none() { + let mut r = record(); + r.effort_level = Some("minimal".into()); + let launch = project_record_only(&r, Some(goose())); + assert_eq!( + launch.value, None, + "invalid canonical with no fallback → None" + ); +} + +#[test] +fn no_tier_set_is_none() { + let launch = project_record_only(&record(), Some(goose())); + assert_eq!(launch.value, None); +} + +// -------------------------------------------------------------------------- +// Suppression + single-key emission (the double-authority guard) +// -------------------------------------------------------------------------- + +#[test] +fn suppress_covers_all_native_legacy_and_sentinel_keys() { + let keys = effort_suppress_keys(); + assert!(keys.contains(&GOOSE_KEY), "goose native key suppressed"); + assert!( + keys.contains(&BUZZ_AGENT_KEY), + "buzz-agent native + legacy key suppressed" + ); + assert!(keys.contains(&ACP_KEY), "ACP transport sentinel suppressed"); +} + +#[test] +fn apply_strips_every_foreign_effort_key_then_emits_one() { + // A launch env carrying multiple stale/foreign effort keys must end with + // exactly the one destination key holding the projected value. + let mut r = record(); + r.effort_level = Some("high".into()); + let launch = project_record_only(&r, Some(goose())); + + let mut launch_env = env(&[ + (ACP_KEY, "stale"), + (BUZZ_AGENT_KEY, "stale"), + (GOOSE_KEY, "stale"), + ("UNRELATED", "keep"), + ]); + launch.apply(&mut launch_env); + + assert_eq!(launch_env.get(GOOSE_KEY).map(String::as_str), Some("high")); + assert_eq!(launch_env.get(ACP_KEY), None); + assert_eq!(launch_env.get(BUZZ_AGENT_KEY), None); + assert_eq!( + launch_env.get("UNRELATED").map(String::as_str), + Some("keep") + ); + let effort_keys = launch_env + .keys() + .filter(|k| effort_suppress_keys().contains(&k.as_str())) + .count(); + assert_eq!(effort_keys, 1, "exactly one effort key survives"); +} + +#[test] +fn apply_with_no_value_strips_all_effort_keys() { + // value == None → strip every effort key and emit nothing (valid passthrough + // does not survive because the projection already resolved all tiers). + let launch = project_record_only(&record(), Some(goose())); + assert_eq!(launch.value, None); + let mut launch_env = env(&[(ACP_KEY, "x"), (GOOSE_KEY, "y")]); + launch.apply(&mut launch_env); + assert!( + launch_env + .keys() + .all(|k| !effort_suppress_keys().contains(&k.as_str())), + "no effort key remains when the projection has no value" + ); +} + +#[test] +fn buzz_agent_generic_column_does_not_leak_acp_sentinel() { + // A buzz-agent descriptor carrying the generic ACP sentinel in user env must + // launch with only its native key — the sentinel is suppressed as transport. + let mut r = record(); + r.env_vars = env(&[(ACP_KEY, "high")]); + r.effort_level = Some("medium".into()); + let launch = project_record_only(&r, Some(buzz_agent())); + // buzz-agent's native key is BUZZ_AGENT_THINKING_EFFORT; the ACP sentinel is + // not its native tier, so the column wins and emits under the native key. + assert_eq!(launch.value.as_deref(), Some("medium")); + assert_eq!(launch.key, BUZZ_AGENT_KEY); + + let mut launch_env = r.env_vars.clone(); + launch.apply(&mut launch_env); + assert_eq!( + launch_env.get(ACP_KEY), + None, + "ACP sentinel stripped for buzz-agent" + ); + assert_eq!( + launch_env.get(BUZZ_AGENT_KEY).map(String::as_str), + Some("medium") + ); +} diff --git a/desktop/src-tauri/src/managed_agents/config_bridge/mod.rs b/desktop/src-tauri/src/managed_agents/config_bridge/mod.rs index f8b045fc72f..9ac2e5bc10f 100644 --- a/desktop/src-tauri/src/managed_agents/config_bridge/mod.rs +++ b/desktop/src-tauri/src/managed_agents/config_bridge/mod.rs @@ -1,6 +1,7 @@ mod buzz_agent; mod claude; mod codex; +pub(crate) mod effort; mod goose; pub(crate) mod reader; mod schema_walker; @@ -8,6 +9,25 @@ pub(crate) mod types; pub(crate) use types::*; +/// The legacy effort env key written by pre-migration saves. +/// +/// Harnesses whose native `thinking_env_var` differs from this constant +/// (currently: Goose uses `GOOSE_THINKING_EFFORT`) need the alias resolver in +/// [`effort`] to translate old saves. buzz-agent's native key equals this +/// constant, so no aliasing applies there. +pub(crate) const LEGACY_THINKING_EFFORT_KEY: &str = "BUZZ_AGENT_THINKING_EFFORT"; + +/// Return all known native thinking-effort env keys across all runtimes. +/// +/// Derived from `KNOWN_ACP_RUNTIMES::thinking_env_var` so that adding a new +/// runtime automatically participates in foreign-key suppression without a +/// separate constant to update. +pub(crate) fn all_known_effort_keys() -> impl Iterator { + crate::managed_agents::discovery::KNOWN_ACP_RUNTIMES + .iter() + .filter_map(|rt| rt.thinking_env_var) +} + /// Read the goose harness config file (`~/.config/goose/config.yaml`). /// /// Used by readiness evaluation to silence requirements that are already diff --git a/desktop/src-tauri/src/managed_agents/config_bridge/reader.rs b/desktop/src-tauri/src/managed_agents/config_bridge/reader.rs index 93827635e90..4916b748a2c 100644 --- a/desktop/src-tauri/src/managed_agents/config_bridge/reader.rs +++ b/desktop/src-tauri/src/managed_agents/config_bridge/reader.rs @@ -1,7 +1,10 @@ +use crate::managed_agents::discovery::EffortNormalization; use crate::managed_agents::discovery::KnownAcpRuntime; use crate::managed_agents::types::ManagedAgentRecord; +use super::effort::effort_tier_alias; use super::types::*; +use super::LEGACY_THINKING_EFFORT_KEY; /// Build the full config surface for an agent, merging all tiers. /// @@ -40,6 +43,7 @@ pub(crate) fn read_config_surface( let provider_env_var = runtime_meta.and_then(|m| m.provider_env_var); let provider_locked = runtime_meta.is_some_and(|m| m.provider_locked); let thinking_env_var = runtime_meta.and_then(|m| m.thinking_env_var); + let effort_norm = runtime_meta.and_then(|m| m.effort_normalization); let supports_acp_native = runtime_meta.is_some_and(|m| m.supports_acp_native_config); let required_fields: &[&str] = runtime_meta .map(|m| m.required_normalized_fields) @@ -93,6 +97,7 @@ pub(crate) fn read_config_surface( &acp_effort, effort_option.map(|o| o.config_id.as_str()), thinking_env_var, + effort_norm, is_pre_spawn, tiers, ), @@ -126,7 +131,7 @@ pub(crate) fn read_config_surface( .collect(); // Collect the env var keys already covered by normalized fields. - let normalized_env_keys: Vec<&str> = [ + let mut normalized_env_keys: Vec<&str> = [ model_env_var, provider_env_var, thinking_env_var, @@ -138,6 +143,34 @@ pub(crate) fn read_config_surface( .flatten() .collect(); + // Hide the legacy effort key from advanced only when it actually wins the + // record tier: native and canonical column are absent/invalid, then legacy + // normalizes. Otherwise `build_thinking_field` represents another winner + // and the legacy key stays editable in Advanced. + let record_legacy_consumed = thinking_env_var + .zip(effort_norm) + .is_some_and(|(native, norm)| { + native != LEGACY_THINKING_EFFORT_KEY + && record + .env_vars + .get(native) + .and_then(|v| norm.normalize_str(v)) + .is_none() + && record + .effort_level + .as_deref() + .and_then(|v| norm.normalize_str(v)) + .is_none() + && record + .env_vars + .get(LEGACY_THINKING_EFFORT_KEY) + .and_then(|v| norm.normalize_str(v)) + .is_some() + }); + if record_legacy_consumed { + normalized_env_keys.push(LEGACY_THINKING_EFFORT_KEY); + } + // Tier 2a: remaining env vars not covered by normalized fields. let mut advanced = advanced; for (k, v) in &record.env_vars { @@ -542,40 +575,85 @@ fn build_thinking_field( acp_effort: &Option, effort_config_id: Option<&str>, thinking_env_var: Option<&str>, + effort_norm: Option<&'static EffortNormalization>, is_pre_spawn: bool, tiers: &InheritedConfigTiers, ) -> Option { - // Tier ordering: - // record env > record.effort_level (canonical Buzz-persisted) > ACP > - // persona env > global env > definition env > config file. + // Tier ordering (mirrors the launch projection in `config_bridge::effort`, + // plus the two reader-only tiers the projection has no input for — live ACP + // and the on-disk config file): + // record native > canonical column > record legacy > ACP > + // persona > global > definition > config file. // - // `record.effort_level` is the B5 canonical value: the effort a spawn will - // actually apply at next session start (via `apply_effort_env`). Sitting it - // above ACP means the panel shows the *configured* value the agent will - // launch with rather than a stale live-session reading — the record can't - // be masked by, nor mask, the running value silently. - let [rec_env, pers_env, glob_env, def_env] = thinking_env_var - .map(|k| { - env_candidates( - k, - &record.env_vars, - &tiers.persona_env, - &tiers.global_env, - &tiers.definition_env, - ) - }) - .unwrap_or([None, None, None, None]); + // Every candidate is normalized through the runtime's declared contract + // (`effort_norm`) before validity, precedence, override tracking, and the B + // same-value collapse — the SAME normalizer the launch projection applies — + // so the panel and the next spawn resolve one effective value AND authority. + // For contract runtimes an invalid value (e.g. Goose `minimal`) normalizes + // to `None` and is skipped as absent so a lower tier can win; aliases + // (`none`→`off`, `xhigh`→`max`, case-fold) canonicalize. Contract-less + // runtimes (buzz-agent, Claude/Codex column) pass raw. + let norm = |raw: &str| -> Option { + match effort_norm { + Some(c) => c.normalize_str(raw), + None => Some(raw.to_string()), + } + }; - let canonical_effort = record.effort_level.as_deref(); + // Record tiers, split exactly as the projection resolves them: native env + // strictly above the canonical column, legacy env strictly below it. + let rec_native = thinking_env_var + .and_then(|k| record.env_vars.get(k)) + .and_then(|v| norm(v)); + let column = record.effort_level.as_deref().and_then(&norm); + let rec_legacy = thinking_env_var + .filter(|k| *k != LEGACY_THINKING_EFFORT_KEY) + .and_then(|_| record.env_vars.get(LEGACY_THINKING_EFFORT_KEY)) + .and_then(|v| norm(v)); + + // Inherited env tiers: persona resolves native-then-legacy; global and + // definition are native-only (legacy alias excluded), matching the launch + // projection's per-tier alias policy. + let pers = thinking_env_var.and_then(|k| effort_tier_alias(&tiers.persona_env, k, norm, true)); + let glob = thinking_env_var.and_then(|k| effort_tier_alias(&tiers.global_env, k, norm, false)); + let def = + thinking_env_var.and_then(|k| effort_tier_alias(&tiers.definition_env, k, norm, false)); + let file = file_effort.as_deref().and_then(&norm); + + // Live ACP value, normalized (invalid → skip as absent). The matched + // `config_id` is preserved for `write_via` regardless of value validity. + let acp_norm = acp_effort.as_deref().and_then(norm); + + // B same-value collapse: when NO record-level authority exists and the live + // ACP value exactly equals what inheritance would already resolve to, drop + // ACP so the panel shows the true baseline origin ("Global default") rather + // than a spurious "Runtime override (this session only)" — the session is + // almost certainly echoing what spawn injected. When a record tier is + // present it wins over ACP anyway, so ACP stays only for override tracking. + let record_present = rec_native.is_some() || column.is_some() || rec_legacy.is_some(); + let baseline_first = [ + pers.as_deref(), + glob.as_deref(), + def.as_deref(), + file.as_deref(), + ] + .into_iter() + .flatten() + .next(); + let acp_for_list = match (record_present, acp_norm.as_deref(), baseline_first) { + (false, Some(a), Some(b)) if a == b => None, + _ => acp_norm.as_deref(), + }; let tiers_list: &[(Option<&str>, ConfigOrigin)] = &[ - (rec_env, ConfigOrigin::BuzzExplicit), - (canonical_effort, ConfigOrigin::BuzzExplicit), - (acp_effort.as_deref(), ConfigOrigin::AcpConfigOption), - (pers_env, ConfigOrigin::PersonaDefault), - (glob_env, ConfigOrigin::GlobalDefault), - (def_env, ConfigOrigin::HarnessDefault), - (file_effort.as_deref(), ConfigOrigin::ConfigFile), + (rec_native.as_deref(), ConfigOrigin::BuzzExplicit), + (column.as_deref(), ConfigOrigin::BuzzExplicit), + (rec_legacy.as_deref(), ConfigOrigin::BuzzExplicit), + (acp_for_list, ConfigOrigin::AcpConfigOption), + (pers.as_deref(), ConfigOrigin::PersonaDefault), + (glob.as_deref(), ConfigOrigin::GlobalDefault), + (def.as_deref(), ConfigOrigin::HarnessDefault), + (file.as_deref(), ConfigOrigin::ConfigFile), ]; let (value, origin, overridden_value, overridden_origin) = resolve_with_override(tiers_list)?; @@ -746,11 +824,20 @@ fn find_config_option_value(cache: &SessionConfigCache, category: &str) -> Optio /// config id (Claude Code uses `id="effort"`). Selecting by category — not by /// a hardcoded id — is what lets the running value, the write config id, and /// the picker options all derive from one entry. +/// +/// `thought_level` is preferred; the legacy invented category `effort` is a +/// fallback for old test fixtures and pre-canonical adapters. The fallback +/// fires only when `thought_level` is entirely absent — an advertised-but-unset +/// `thought_level` entry is still returned (its `current_value` is `None`), so +/// the reader never flips write-routing to the legacy `effort` config id. fn find_effort_option(cache: &SessionConfigCache) -> Option<&AcpConfigOptionEntry> { - cache - .config_options - .iter() - .find(|o| o.category.as_deref() == Some("thought_level")) + let by_category = |category: &str| { + cache + .config_options + .iter() + .find(|o| o.category.as_deref() == Some(category)) + }; + by_category("thought_level").or_else(|| by_category("effort")) } fn has_config_option(cache: Option<&SessionConfigCache>, category: &str) -> bool { diff --git a/desktop/src-tauri/src/managed_agents/config_bridge/reader_tests.rs b/desktop/src-tauri/src/managed_agents/config_bridge/reader_tests.rs index 36b6022b53b..39a80566e0d 100644 --- a/desktop/src-tauri/src/managed_agents/config_bridge/reader_tests.rs +++ b/desktop/src-tauri/src/managed_agents/config_bridge/reader_tests.rs @@ -28,7 +28,7 @@ fn with_goose_path_root(value: Option<&str>, body: impl FnOnce() -> T) -> T { } fn test_runtime() -> &'static KnownAcpRuntime { - &KnownAcpRuntime { + static RUNTIME: KnownAcpRuntime = KnownAcpRuntime { id: "goose", label: "Goose", commands: &["goose"], @@ -54,13 +54,15 @@ fn test_runtime() -> &'static KnownAcpRuntime { config_file_format: Some("yaml"), supports_acp_native_config: true, thinking_env_var: Some("GOOSE_THINKING_EFFORT"), + effort_normalization: Some(&crate::managed_agents::discovery::GOOSE_EFFORT_NORMALIZATION), max_tokens_env_var: Some("GOOSE_MAX_TOKENS"), context_limit_env_var: Some("GOOSE_CONTEXT_LIMIT"), max_rounds_env_var: None, required_normalized_fields: &["model", "provider"], login_hint: None, auth_probe_args: None, - } + }; + &RUNTIME } fn test_record() -> ManagedAgentRecord { @@ -645,6 +647,7 @@ fn buzz_agent_runtime() -> &'static KnownAcpRuntime { config_file_format: None, supports_acp_native_config: false, thinking_env_var: Some("BUZZ_AGENT_THINKING_EFFORT"), + effort_normalization: None, max_tokens_env_var: Some("BUZZ_AGENT_MAX_OUTPUT_TOKENS"), context_limit_env_var: Some("BUZZ_AGENT_MAX_CONTEXT_TOKENS"), max_rounds_env_var: Some("BUZZ_AGENT_MAX_ROUNDS"), diff --git a/desktop/src-tauri/src/managed_agents/config_bridge/reader_tests_ext.rs b/desktop/src-tauri/src/managed_agents/config_bridge/reader_tests_ext.rs index f86793f91a1..998dc1d0589 100644 --- a/desktop/src-tauri/src/managed_agents/config_bridge/reader_tests_ext.rs +++ b/desktop/src-tauri/src/managed_agents/config_bridge/reader_tests_ext.rs @@ -518,3 +518,460 @@ fn claude_default_config_dir_reports_static_settings_path() { .as_deref() .is_some_and(|p| !p.starts_with('~'))); } + +// ── Goose-contract reader normalization + reader/projection parity ──────────── +// +// The reader (`build_thinking_field`) and the launch projection +// (`effort_launch_projection`) must resolve one effective value AND one +// authority for every record/inherited input, or the config panel displays a +// different effort than the next spawn launches. `test_runtime()` is Goose with +// `effort_normalization = GOOSE_EFFORT_NORMALIZATION`, so these exercise the +// normalization gate, alias canonicalization, invalid-value skip/fallthrough, +// and the decisive mixed-authority case — the phase-1 behavior block, not just +// fixture metadata. + +use crate::managed_agents::config_bridge::effort::effort_launch_projection; + +/// Drive the projection from the SAME record + global env the reader sees, so +/// the two resolvers are compared on identical inputs. Persona/definition tiers +/// use distinct input shapes across the two layers and are covered separately; +/// record-native/column/legacy and global are expressible identically here, +/// which is exactly where the authority-order contract is decisive. +fn projection_value( + record: &ManagedAgentRecord, + global_env: &BTreeMap, +) -> Option { + effort_launch_projection( + record, + Some(test_runtime()), + &[], + None, + global_env, + None, + &BTreeMap::new(), + ) + .value +} + +/// Goose invalid record-native value (`minimal` — not in the Goose contract) +/// skips as absent so a valid lower tier wins, IDENTICALLY in reader and +/// projection. This is Thufir's named regression: a raw winner in the panel +/// while the launch skips it. +#[test] +fn goose_invalid_record_native_skips_to_column_in_reader_and_projection() { + let mut record = test_record(); + record + .env_vars + .insert("GOOSE_THINKING_EFFORT".to_string(), "minimal".to_string()); + record.effort_level = Some("high".to_string()); + let runtime = test_runtime(); + + let surface = read_config_surface(&record, Some(runtime), None, &no_tiers(), None); + let effort = surface + .normalized + .thinking_effort + .expect("valid column must win when native is invalid"); + // Reader: invalid native skipped, column wins. + assert_eq!(effort.value.as_deref(), Some("high")); + assert_eq!(effort.origin, ConfigOrigin::BuzzExplicit); + // Projection agrees on value. + assert_eq!( + projection_value(&record, &BTreeMap::new()).as_deref(), + Some("high") + ); +} + +/// Goose alias canonicalization: `xhigh` → `max` in BOTH resolvers (record +/// native), `none` → `off` (column). +#[test] +fn goose_aliases_canonicalize_in_reader_and_projection() { + let mut record = test_record(); + record + .env_vars + .insert("GOOSE_THINKING_EFFORT".to_string(), "xhigh".to_string()); + let runtime = test_runtime(); + + let surface = read_config_surface(&record, Some(runtime), None, &no_tiers(), None); + let effort = surface.normalized.thinking_effort.unwrap(); + assert_eq!(effort.value.as_deref(), Some("max")); + assert_eq!(effort.origin, ConfigOrigin::BuzzExplicit); + assert_eq!( + projection_value(&record, &BTreeMap::new()).as_deref(), + Some("max") + ); + + let mut record2 = test_record(); + record2.effort_level = Some("none".to_string()); + let surface2 = read_config_surface(&record2, Some(runtime), None, &no_tiers(), None); + assert_eq!( + surface2 + .normalized + .thinking_effort + .unwrap() + .value + .as_deref(), + Some("off") + ); + assert_eq!( + projection_value(&record2, &BTreeMap::new()).as_deref(), + Some("off") + ); +} + +/// The decisive mixed-authority case (Thufir/Paul acceptance pin): a valid +/// record-native value and a DIFFERENT valid column → the native value wins in +/// reader and projection alike. The column is the surfaced override baseline. +#[test] +fn goose_record_native_outranks_column_in_reader_and_projection() { + let mut record = test_record(); + record + .env_vars + .insert("GOOSE_THINKING_EFFORT".to_string(), "high".to_string()); + record.effort_level = Some("low".to_string()); + let runtime = test_runtime(); + + let surface = read_config_surface(&record, Some(runtime), None, &no_tiers(), None); + let effort = surface.normalized.thinking_effort.unwrap(); + assert_eq!(effort.value.as_deref(), Some("high")); + assert_eq!(effort.origin, ConfigOrigin::BuzzExplicit); + // Column is the overridden baseline (next distinct tier below native). + assert_eq!(effort.overridden_value.as_deref(), Some("low")); + // Projection resolves the same authority. + assert_eq!( + projection_value(&record, &BTreeMap::new()).as_deref(), + Some("high") + ); +} + +/// Invalid column AND invalid native → both skip; a valid global tier wins in +/// the reader, and the projection (driven from the same global env) agrees. +#[test] +fn goose_invalid_record_tiers_fall_through_to_global_in_reader_and_projection() { + let mut record = test_record(); + record + .env_vars + .insert("GOOSE_THINKING_EFFORT".to_string(), "bogus".to_string()); + record.effort_level = Some("alsobad".to_string()); + let runtime = test_runtime(); + let mut global = BTreeMap::new(); + global.insert("GOOSE_THINKING_EFFORT".to_string(), "medium".to_string()); + let tiers = global_env_tiers("GOOSE_THINKING_EFFORT", "medium"); + + let surface = read_config_surface(&record, Some(runtime), None, &tiers, None); + let effort = surface + .normalized + .thinking_effort + .expect("global tier must win when both record tiers are invalid"); + assert_eq!(effort.value.as_deref(), Some("medium")); + assert_eq!(effort.origin, ConfigOrigin::GlobalDefault); + assert_eq!( + projection_value(&record, &global).as_deref(), + Some("medium") + ); +} + +/// Goose legacy alias (`BUZZ_AGENT_THINKING_EFFORT`) is accepted for the record +/// tier below the column, canonicalized, in reader and projection alike. +#[test] +fn goose_record_legacy_alias_below_column_in_reader_and_projection() { + let mut record = test_record(); + record.env_vars.insert( + "BUZZ_AGENT_THINKING_EFFORT".to_string(), + "xhigh".to_string(), + ); + let runtime = test_runtime(); + + let surface = read_config_surface(&record, Some(runtime), None, &no_tiers(), None); + let effort = surface + .normalized + .thinking_effort + .expect("record legacy alias must surface when native and column are absent"); + assert_eq!(effort.value.as_deref(), Some("max")); + assert_eq!(effort.origin, ConfigOrigin::BuzzExplicit); + assert_eq!( + projection_value(&record, &BTreeMap::new()).as_deref(), + Some("max") + ); +} + +/// B same-value collapse: no record authority, live ACP echoes the inherited +/// global value → the panel shows the inherited origin (GlobalDefault), not a +/// spurious per-session AcpConfigOption override. +#[test] +fn goose_acp_equal_to_global_collapses_to_global_origin() { + let record = test_record(); + let runtime = test_runtime(); + let cache = SessionConfigCache { + config_options: vec![AcpConfigOptionEntry { + config_id: "thinking_effort".to_string(), + category: Some("thought_level".to_string()), + display_name: Some("Effort".to_string()), + current_value: Some("medium".to_string()), + options: vec![], + }], + available_modes: vec![], + available_models: vec![], + current_model: None, + model_overridden: false, + goose_native_config: None, + captured_at: "".to_string(), + }; + let tiers = global_env_tiers("GOOSE_THINKING_EFFORT", "medium"); + + let surface = read_config_surface(&record, Some(runtime), Some(&cache), &tiers, None); + let effort = surface.normalized.thinking_effort.unwrap(); + assert_eq!(effort.value.as_deref(), Some("medium")); + assert_eq!( + effort.origin, + ConfigOrigin::GlobalDefault, + "ACP echoing the inherited value must not masquerade as a session override" + ); +} + +/// B same-value collapse does NOT fire on genuine divergence: live ACP differs +/// from the inherited baseline → ACP wins as the per-session override, global +/// is the surfaced baseline. +#[test] +fn goose_acp_diverging_from_global_wins_as_override() { + let record = test_record(); + let runtime = test_runtime(); + let cache = SessionConfigCache { + config_options: vec![AcpConfigOptionEntry { + config_id: "thinking_effort".to_string(), + category: Some("thought_level".to_string()), + display_name: Some("Effort".to_string()), + current_value: Some("low".to_string()), + options: vec![], + }], + available_modes: vec![], + available_models: vec![], + current_model: None, + model_overridden: false, + goose_native_config: None, + captured_at: "".to_string(), + }; + let tiers = global_env_tiers("GOOSE_THINKING_EFFORT", "high"); + + let surface = read_config_surface(&record, Some(runtime), Some(&cache), &tiers, None); + let effort = surface.normalized.thinking_effort.unwrap(); + assert_eq!(effort.value.as_deref(), Some("low")); + assert_eq!(effort.origin, ConfigOrigin::AcpConfigOption); + assert_eq!(effort.overridden_value.as_deref(), Some("high")); + assert_eq!(effort.overridden_origin, Some(ConfigOrigin::GlobalDefault)); +} + +/// Invalid live ACP value is skipped as absent; a valid record tier wins and +/// no phantom ACP override is surfaced. +#[test] +fn goose_invalid_acp_skips_and_record_wins() { + let mut record = test_record(); + record.effort_level = Some("high".to_string()); + let runtime = test_runtime(); + let cache = SessionConfigCache { + config_options: vec![AcpConfigOptionEntry { + config_id: "thinking_effort".to_string(), + category: Some("thought_level".to_string()), + display_name: Some("Effort".to_string()), + current_value: Some("garbage".to_string()), + options: vec![], + }], + available_modes: vec![], + available_models: vec![], + current_model: None, + model_overridden: false, + goose_native_config: None, + captured_at: "".to_string(), + }; + + let surface = read_config_surface(&record, Some(runtime), Some(&cache), &no_tiers(), None); + let effort = surface.normalized.thinking_effort.unwrap(); + assert_eq!(effort.value.as_deref(), Some("high")); + assert_eq!(effort.origin, ConfigOrigin::BuzzExplicit); + assert_eq!( + projection_value(&record, &BTreeMap::new()).as_deref(), + Some("high") + ); +} + +// ── Consumed-legacy Advanced suppression (F2) ──────────────────────────────── +// +// When the record's native effort key is absent/invalid and the legacy key +// (`BUZZ_AGENT_THINKING_EFFORT`) supplies the normalized record effort, the +// legacy key must NOT also re-appear as a generic Advanced field — one +// persisted fact must not surface through two controls. Invalid/unconsumed +// legacy values stay visible in Advanced. + +/// Record has valid legacy `BUZZ_AGENT_THINKING_EFFORT=high` and no native +/// `GOOSE_THINKING_EFFORT` → effort surfaces from the legacy alias AND the +/// legacy key must NOT re-appear in Advanced. +#[test] +fn record_consumed_legacy_effort_hidden_from_advanced_reader() { + let mut record = test_record(); + record + .env_vars + .insert("BUZZ_AGENT_THINKING_EFFORT".to_string(), "high".to_string()); + let runtime = test_runtime(); // Goose (native GOOSE_THINKING_EFFORT) + + let surface = with_goose_path_root(Some("/nonexistent"), || { + read_config_surface(&record, Some(runtime), None, &no_tiers(), None) + }); + + let effort = surface + .normalized + .thinking_effort + .expect("valid legacy value must surface as effort via record-tier alias"); + assert_eq!(effort.value.as_deref(), Some("high")); + + let advanced_keys: Vec<&str> = surface.advanced.iter().map(|f| f.key.as_str()).collect(); + assert!( + !advanced_keys.contains(&"BUZZ_AGENT_THINKING_EFFORT"), + "consumed legacy effort key must not double-emit in advanced; got {advanced_keys:?}" + ); +} + +/// A valid legacy value shadowed by the canonical column is not consumed, so +/// it remains editable in Advanced rather than silently resurfacing later if +/// the column is cleared. +#[test] +fn record_legacy_effort_shadowed_by_column_stays_visible_in_advanced_reader() { + let mut record = test_record(); + record.effort_level = Some("high".to_string()); + record + .env_vars + .insert("BUZZ_AGENT_THINKING_EFFORT".to_string(), "low".to_string()); + let runtime = test_runtime(); // Goose + + let surface = with_goose_path_root(Some("/nonexistent"), || { + read_config_surface(&record, Some(runtime), None, &no_tiers(), None) + }); + + let effort = surface + .normalized + .thinking_effort + .expect("canonical column must win over legacy record effort"); + assert_eq!(effort.value.as_deref(), Some("high")); + let advanced_keys: Vec<&str> = surface.advanced.iter().map(|f| f.key.as_str()).collect(); + assert!( + advanced_keys.contains(&"BUZZ_AGENT_THINKING_EFFORT"), + "valid but unconsumed record legacy must remain visible in Advanced; got {advanced_keys:?}" + ); +} + +/// An invalid legacy `BUZZ_AGENT_THINKING_EFFORT` value is unconsumed, so it +/// stays visible in Advanced. +#[test] +fn record_invalid_legacy_effort_stays_visible_in_advanced_reader() { + let mut record = test_record(); + record.env_vars.insert( + "BUZZ_AGENT_THINKING_EFFORT".to_string(), + "bogus".to_string(), + ); + let runtime = test_runtime(); // Goose + + let surface = with_goose_path_root(Some("/nonexistent"), || { + read_config_surface(&record, Some(runtime), None, &no_tiers(), None) + }); + + assert!( + surface.normalized.thinking_effort.is_none(), + "invalid legacy value must not be consumed as effort" + ); + let advanced_keys: Vec<&str> = surface.advanced.iter().map(|f| f.key.as_str()).collect(); + assert!( + advanced_keys.contains(&"BUZZ_AGENT_THINKING_EFFORT"), + "unconsumed legacy key must stay visible in advanced; got {advanced_keys:?}" + ); +} + +// ── F4: legacy `effort` category fallback in find_effort_option ────────────── +// +// `thought_level` is preferred; the legacy invented category `effort` is a +// fallback for pre-canonical adapters. An advertised-but-unset `thought_level` +// must NOT fall through to a set `effort` (that would route the write to the +// wrong config_id), but a cache that advertises only `effort` must still +// surface a thinking field and write route. + +/// `thought_level` present but unset, `effort` present and set → effort must +/// NOT surface from the live cache (no fallthrough); write routing never picks +/// up the legacy `effort` config id. +#[test] +fn unset_thought_level_does_not_fall_through_to_effort_category() { + let record = test_record(); + let runtime = test_runtime(); // Goose + let cache = SessionConfigCache { + config_options: vec![ + AcpConfigOptionEntry { + config_id: "thinking_effort".to_string(), + category: Some("thought_level".to_string()), + display_name: Some("Thinking Effort".to_string()), + current_value: None, // advertised but unset + options: vec![], + }, + AcpConfigOptionEntry { + config_id: "effort".to_string(), + category: Some("effort".to_string()), + display_name: Some("Effort (legacy)".to_string()), + current_value: Some("low".to_string()), + options: vec![], + }, + ], + available_modes: vec![], + available_models: vec![], + current_model: None, + model_overridden: false, + goose_native_config: None, + captured_at: "".to_string(), + }; + + let surface = with_goose_path_root(Some("/nonexistent"), || { + read_config_surface(&record, Some(runtime), Some(&cache), &no_tiers(), None) + }); + + assert!( + surface.normalized.thinking_effort.is_none(), + "unset thought_level must not fall through to the legacy effort category" + ); +} + +/// `effort` category present and set, no `thought_level` at all → legacy +/// fallback still surfaces the field and routes the write to the matched +/// `effort` config id. +#[test] +fn effort_category_fallback_used_when_thought_level_absent() { + let record = test_record(); + let runtime = test_runtime(); // Goose + let cache = SessionConfigCache { + config_options: vec![AcpConfigOptionEntry { + config_id: "effort".to_string(), + category: Some("effort".to_string()), + display_name: Some("Effort (legacy)".to_string()), + current_value: Some("high".to_string()), + options: vec![], + }], + available_modes: vec![], + available_models: vec![], + current_model: None, + model_overridden: false, + goose_native_config: None, + captured_at: "".to_string(), + }; + + let surface = with_goose_path_root(Some("/nonexistent"), || { + read_config_surface(&record, Some(runtime), Some(&cache), &no_tiers(), None) + }); + + let effort = surface + .normalized + .thinking_effort + .expect("legacy effort category must surface when thought_level is absent"); + assert_eq!(effort.value.as_deref(), Some("high")); + assert!( + matches!( + &effort.write_via, + ConfigWriteMechanism::AcpSetConfigOption { config_id } + if config_id == "effort" + ), + "write route must use the legacy effort config_id when it is the only category; got {:?}", + effort.write_via + ); +} diff --git a/desktop/src-tauri/src/managed_agents/discovery.rs b/desktop/src-tauri/src/managed_agents/discovery.rs index bc0e3a6cdae..77074a7199f 100644 --- a/desktop/src-tauri/src/managed_agents/discovery.rs +++ b/desktop/src-tauri/src/managed_agents/discovery.rs @@ -13,12 +13,17 @@ mod presets; mod runtime_metadata; #[macro_use] mod windows_install; +mod catalog; +pub(crate) use catalog::KNOWN_ACP_RUNTIMES; pub(crate) use presets::{ canonical_harness_command, command_for_runtime_id, preset_harness_definitions, preset_harness_ids, }; use presets::{preset_catalog_entry, PRESET_HARNESSES}; +pub(crate) use runtime_metadata::EffortNormalization; pub(crate) use runtime_metadata::KnownAcpRuntime; +#[cfg(test)] +pub(crate) use runtime_metadata::GOOSE_EFFORT_NORMALIZATION; const GOOSE_AVATAR_URL: &str = "https://goose-docs.ai/img/logo_dark.png"; const CLAUDE_CODE_AVATAR_URL: &str = "https://anthropic.gallerycdn.vsassets.io/extensions/anthropic/claude-code/2.1.77/1773707456892/Microsoft.VisualStudio.Services.Icons.Default"; @@ -75,144 +80,6 @@ fn common_binary_paths() -> &'static [PathBuf] { }) } -const KNOWN_ACP_RUNTIMES: &[KnownAcpRuntime] = &[ - KnownAcpRuntime { - id: "goose", - label: "Goose", - commands: &["goose"], - aliases: &[], - avatar_url: GOOSE_AVATAR_URL, - mcp_command: None, - mcp_hooks: false, - underlying_cli: Some("goose"), - cli_install_commands: &["curl -fsSL https://github.com/aaif-goose/goose/releases/download/stable/download_cli.sh | CONFIGURE=false bash"], - // Goose's stable release currently publishes only the Unix installer; - // its official Windows instructions intentionally point at this main-branch script. - cli_install_commands_windows: &[windows_install_command!("goose", "https://raw.githubusercontent.com/aaif-goose/goose/main/download_cli.ps1", "$env:CONFIGURE='false'; ")], - adapter_install_commands: &[], - cli_install_instructions_url: "https://goose-docs.ai/docs/getting-started/installation/", - adapter_install_instructions_url: "", - cli_install_hint: "Buzz talks to Goose through the Goose CLI.", - adapter_install_hint: "", - skill_dir: Some(".goose/skills"), - supports_acp_model_switching: false, - model_env_var: Some("GOOSE_MODEL"), - provider_env_var: Some("GOOSE_PROVIDER"), - provider_locked: false, - default_env: &[("GOOSE_MODE", "auto")], - config_file_path: Some("~/.config/goose/config.yaml"), - config_file_format: Some("yaml"), - supports_acp_native_config: true, - thinking_env_var: Some("GOOSE_THINKING_EFFORT"), - max_tokens_env_var: Some("GOOSE_MAX_TOKENS"), - context_limit_env_var: Some("GOOSE_CONTEXT_LIMIT"), - max_rounds_env_var: None, - required_normalized_fields: &["model", "provider"], - login_hint: None, - auth_probe_args: None, - }, - KnownAcpRuntime { - id: "claude", - label: "Claude Code", - commands: &["claude-agent-acp", "claude-code-acp"], - aliases: &["claude-code", "claudecode"], - avatar_url: CLAUDE_CODE_AVATAR_URL, - mcp_command: None, - mcp_hooks: false, - underlying_cli: Some("claude"), - cli_install_commands: &["curl -fsSL https://claude.ai/install.sh | bash"], - cli_install_commands_windows: &[windows_install_command!("claude", "https://claude.ai/install.ps1")], - adapter_install_commands: &["npm install -g @agentclientprotocol/claude-agent-acp"], - cli_install_instructions_url: "https://code.claude.com/docs/en/getting-started", - adapter_install_instructions_url: "https://github.com/agentclientprotocol/claude-agent-acp", - cli_install_hint: "Buzz talks to Claude Code through the Claude Code CLI.", - adapter_install_hint: "Buzz talks to the Claude Code CLI through an ACP adapter. Install it with: npm install -g @agentclientprotocol/claude-agent-acp.", - skill_dir: Some(".claude/skills"), - supports_acp_model_switching: false, - model_env_var: None, - provider_env_var: None, - provider_locked: true, - default_env: &[], - config_file_path: Some("~/.claude/settings.json"), - config_file_format: Some("json"), - supports_acp_native_config: false, - thinking_env_var: None, - max_tokens_env_var: None, - context_limit_env_var: None, - max_rounds_env_var: None, - required_normalized_fields: &[], - login_hint: Some("Run the Claude CLI to complete authentication."), - auth_probe_args: Some(&["claude", "auth", "status"]), - }, - KnownAcpRuntime { - id: "codex", - label: "Codex", - commands: &["codex-acp"], - aliases: &[], - avatar_url: CODEX_AVATAR_URL, - mcp_command: Some("buzz-dev-mcp"), - mcp_hooks: false, - underlying_cli: Some("codex"), - cli_install_commands: &["curl -fsSL https://chatgpt.com/codex/install.sh | sh"], - cli_install_commands_windows: &[windows_install_command!("codex", "https://chatgpt.com/codex/install.ps1")], - adapter_install_commands: &["npm install -g @agentclientprotocol/codex-acp"], - cli_install_instructions_url: "https://developers.openai.com/codex/cli/", - adapter_install_instructions_url: "https://github.com/agentclientprotocol/codex-acp", - cli_install_hint: "Buzz talks to Codex through the Codex CLI.", - adapter_install_hint: "Buzz talks to the Codex CLI through an ACP adapter. Install it with: npm install -g @agentclientprotocol/codex-acp.", - skill_dir: Some(".codex/skills"), - supports_acp_model_switching: false, - model_env_var: None, - provider_env_var: None, - provider_locked: false, - default_env: &[], - config_file_path: Some("~/.codex/config.toml"), - config_file_format: Some("toml"), - supports_acp_native_config: false, - thinking_env_var: None, - max_tokens_env_var: None, - context_limit_env_var: None, - max_rounds_env_var: None, - required_normalized_fields: &[], - login_hint: Some("Run `codex login` to authenticate."), - // Verified: `codex login status` exits 0 when logged in, non-zero otherwise. - auth_probe_args: Some(&["codex", "login", "status"]), - }, - KnownAcpRuntime { - id: "buzz-agent", - label: "Buzz Agent", - commands: &["buzz-agent"], - aliases: &[], - avatar_url: BUZZ_AGENT_AVATAR_URL, - mcp_command: Some("buzz-dev-mcp"), - mcp_hooks: true, - underlying_cli: None, - cli_install_commands: &[], - cli_install_commands_windows: &[], - adapter_install_commands: &[], - cli_install_instructions_url: "https://github.com/block/buzz", - adapter_install_instructions_url: "https://github.com/block/buzz", - cli_install_hint: "Ships with the Buzz desktop app.", - adapter_install_hint: "", - skill_dir: None, - supports_acp_model_switching: true, - model_env_var: Some("BUZZ_AGENT_MODEL"), - provider_env_var: Some("BUZZ_AGENT_PROVIDER"), - provider_locked: false, - default_env: &[], - config_file_path: None, - config_file_format: None, - supports_acp_native_config: false, - thinking_env_var: Some("BUZZ_AGENT_THINKING_EFFORT"), - max_tokens_env_var: Some("BUZZ_AGENT_MAX_OUTPUT_TOKENS"), - context_limit_env_var: Some("BUZZ_AGENT_MAX_CONTEXT_TOKENS"), - max_rounds_env_var: Some("BUZZ_AGENT_MAX_ROUNDS"), - required_normalized_fields: &["model", "provider"], - login_hint: None, - auth_probe_args: None, - }, -]; - /// Skill discovery directories declared by known runtimes. pub(crate) fn known_skill_dirs() -> impl Iterator { KNOWN_ACP_RUNTIMES.iter().filter_map(|p| p.skill_dir) @@ -367,7 +234,12 @@ pub fn effective_agent_command( } mod overrides; -pub use overrides::{apply_agent_command_update, create_time_agent_command_override}; +#[cfg(test)] +pub use overrides::remove_record_effort_aliases; +pub use overrides::{ + apply_agent_command_update, apply_env_vars_then_effort_transition, + create_time_agent_command_override, +}; /// Prefix of the typed dangling-harness error produced by /// `try_record_agent_command` / `resolve_effective_harness_descriptor`. diff --git a/desktop/src-tauri/src/managed_agents/discovery/catalog.rs b/desktop/src-tauri/src/managed_agents/discovery/catalog.rs new file mode 100644 index 00000000000..1452666cfe1 --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/discovery/catalog.rs @@ -0,0 +1,150 @@ +//! The known-ACP-runtime catalog. Extracted from `discovery.rs` as pure data +//! (mirroring `presets::PRESET_HARNESSES`) so the module stays under the +//! file-size ratchet. The `windows_install_command!` macro is in textual scope +//! here because this module is declared after `#[macro_use] mod windows_install` +//! in the parent. + +use super::runtime_metadata::{KnownAcpRuntime, GOOSE_EFFORT_NORMALIZATION}; +use super::{BUZZ_AGENT_AVATAR_URL, CLAUDE_CODE_AVATAR_URL, CODEX_AVATAR_URL, GOOSE_AVATAR_URL}; + +pub(crate) const KNOWN_ACP_RUNTIMES: &[KnownAcpRuntime] = &[ + KnownAcpRuntime { + id: "goose", + label: "Goose", + commands: &["goose"], + aliases: &[], + avatar_url: GOOSE_AVATAR_URL, + mcp_command: None, + mcp_hooks: false, + underlying_cli: Some("goose"), + cli_install_commands: &["curl -fsSL https://github.com/aaif-goose/goose/releases/download/stable/download_cli.sh | CONFIGURE=false bash"], + // Goose's stable release currently publishes only the Unix installer; + // its official Windows instructions intentionally point at this main-branch script. + cli_install_commands_windows: &[windows_install_command!("goose", "https://raw.githubusercontent.com/aaif-goose/goose/main/download_cli.ps1", "$env:CONFIGURE='false'; ")], + adapter_install_commands: &[], + cli_install_instructions_url: "https://goose-docs.ai/docs/getting-started/installation/", + adapter_install_instructions_url: "", + cli_install_hint: "Buzz talks to Goose through the Goose CLI.", + adapter_install_hint: "", + skill_dir: Some(".goose/skills"), + supports_acp_model_switching: false, + model_env_var: Some("GOOSE_MODEL"), + provider_env_var: Some("GOOSE_PROVIDER"), + provider_locked: false, + default_env: &[("GOOSE_MODE", "auto")], + config_file_path: Some("~/.config/goose/config.yaml"), + config_file_format: Some("yaml"), + supports_acp_native_config: true, + thinking_env_var: Some("GOOSE_THINKING_EFFORT"), + effort_normalization: Some(&GOOSE_EFFORT_NORMALIZATION), + max_tokens_env_var: Some("GOOSE_MAX_TOKENS"), + context_limit_env_var: Some("GOOSE_CONTEXT_LIMIT"), + max_rounds_env_var: None, + required_normalized_fields: &["model", "provider"], + login_hint: None, + auth_probe_args: None, + }, + KnownAcpRuntime { + id: "claude", + label: "Claude Code", + commands: &["claude-agent-acp", "claude-code-acp"], + aliases: &["claude-code", "claudecode"], + avatar_url: CLAUDE_CODE_AVATAR_URL, + mcp_command: None, + mcp_hooks: false, + underlying_cli: Some("claude"), + cli_install_commands: &["curl -fsSL https://claude.ai/install.sh | bash"], + cli_install_commands_windows: &[windows_install_command!("claude", "https://claude.ai/install.ps1")], + adapter_install_commands: &["npm install -g @agentclientprotocol/claude-agent-acp"], + cli_install_instructions_url: "https://code.claude.com/docs/en/getting-started", + adapter_install_instructions_url: "https://github.com/agentclientprotocol/claude-agent-acp", + cli_install_hint: "Buzz talks to Claude Code through the Claude Code CLI.", + adapter_install_hint: "Buzz talks to the Claude Code CLI through an ACP adapter. Install it with: npm install -g @agentclientprotocol/claude-agent-acp.", + skill_dir: Some(".claude/skills"), + supports_acp_model_switching: false, + model_env_var: None, + provider_env_var: None, + provider_locked: true, + default_env: &[], + config_file_path: Some("~/.claude/settings.json"), + config_file_format: Some("json"), + supports_acp_native_config: false, + thinking_env_var: None, + effort_normalization: None, // claude: canonical routes through BUZZ_ACP_EFFORT_LEVEL (ACP startup) + max_tokens_env_var: None, + context_limit_env_var: None, + max_rounds_env_var: None, + required_normalized_fields: &[], + login_hint: Some("Run the Claude CLI to complete authentication."), + auth_probe_args: Some(&["claude", "auth", "status"]), + }, + KnownAcpRuntime { + id: "codex", + label: "Codex", + commands: &["codex-acp"], + aliases: &[], + avatar_url: CODEX_AVATAR_URL, + mcp_command: Some("buzz-dev-mcp"), + mcp_hooks: false, + underlying_cli: Some("codex"), + cli_install_commands: &["curl -fsSL https://chatgpt.com/codex/install.sh | sh"], + cli_install_commands_windows: &[windows_install_command!("codex", "https://chatgpt.com/codex/install.ps1")], + adapter_install_commands: &["npm install -g @agentclientprotocol/codex-acp"], + cli_install_instructions_url: "https://developers.openai.com/codex/cli/", + adapter_install_instructions_url: "https://github.com/agentclientprotocol/codex-acp", + cli_install_hint: "Buzz talks to Codex through the Codex CLI.", + adapter_install_hint: "Buzz talks to the Codex CLI through an ACP adapter. Install it with: npm install -g @agentclientprotocol/codex-acp.", + skill_dir: Some(".codex/skills"), + supports_acp_model_switching: false, + model_env_var: None, + provider_env_var: None, + provider_locked: false, + default_env: &[], + config_file_path: Some("~/.codex/config.toml"), + config_file_format: Some("toml"), + supports_acp_native_config: false, + thinking_env_var: None, + effort_normalization: None, // codex: canonical routes through BUZZ_ACP_EFFORT_LEVEL (ACP startup) + max_tokens_env_var: None, + context_limit_env_var: None, + max_rounds_env_var: None, + required_normalized_fields: &[], + login_hint: Some("Run `codex login` to authenticate."), + // Verified: `codex login status` exits 0 when logged in, non-zero otherwise. + auth_probe_args: Some(&["codex", "login", "status"]), + }, + KnownAcpRuntime { + id: "buzz-agent", + label: "Buzz Agent", + commands: &["buzz-agent"], + aliases: &[], + avatar_url: BUZZ_AGENT_AVATAR_URL, + mcp_command: Some("buzz-dev-mcp"), + mcp_hooks: true, + underlying_cli: None, + cli_install_commands: &[], + cli_install_commands_windows: &[], + adapter_install_commands: &[], + cli_install_instructions_url: "https://github.com/block/buzz", + adapter_install_instructions_url: "https://github.com/block/buzz", + cli_install_hint: "Ships with the Buzz desktop app.", + adapter_install_hint: "", + skill_dir: None, + supports_acp_model_switching: true, + model_env_var: Some("BUZZ_AGENT_MODEL"), + provider_env_var: Some("BUZZ_AGENT_PROVIDER"), + provider_locked: false, + default_env: &[], + config_file_path: None, + config_file_format: None, + supports_acp_native_config: false, + thinking_env_var: Some("BUZZ_AGENT_THINKING_EFFORT"), + effort_normalization: None, // buzz-agent: per-model catalog; see getProviderEffortConfig() in TS + max_tokens_env_var: Some("BUZZ_AGENT_MAX_OUTPUT_TOKENS"), + context_limit_env_var: Some("BUZZ_AGENT_MAX_CONTEXT_TOKENS"), + max_rounds_env_var: Some("BUZZ_AGENT_MAX_ROUNDS"), + required_normalized_fields: &["model", "provider"], + login_hint: None, + auth_probe_args: None, + }, +]; diff --git a/desktop/src-tauri/src/managed_agents/discovery/overrides.rs b/desktop/src-tauri/src/managed_agents/discovery/overrides.rs index 5140bb2cdda..868f5009406 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/overrides.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/overrides.rs @@ -83,27 +83,82 @@ pub fn update_time_agent_command_override( /// Apply an explicit `agent_command` edit to `record`: persist the override /// pin decided by [`update_time_agent_command_override`], and on the inherit /// sentinel (empty/whitespace command) also clear the materialized -/// `record.runtime` so the resolution ladder falls through to the live -/// definition immediately instead of silently keeping the stale instance copy. +/// `record.runtime` AND the persisted per-instance effort column so the +/// resolution ladder falls through to the live definition immediately instead +/// of silently keeping the stale instance copy. /// -/// The runtime clear is guarded on a live persona link: for a definition-less -/// record the materialized runtime is the only harness source left after the -/// override clear, so a stray empty `agent_command` from a non-dialog caller -/// must not change what the agent runs. +/// The clears are guarded on a live persona link: for a definition-less record +/// the materialized runtime is the only harness source left after the override +/// clear, so a stray empty `agent_command` from a non-dialog caller must not +/// change what the agent runs. +/// +/// Returns `true` when the pin→inherit transition fired. The caller MUST then, +/// AFTER applying any caller-supplied `env_vars`, strip the record effort env +/// aliases via [`remove_record_effort_aliases`] — clearing them here would be +/// undone by a same-request `env_vars` replacement (see the update boundary in +/// `agent_models_update.rs`), so the alias strip is an update-boundary +/// invariant, not a helper-local one. +#[must_use] pub fn apply_agent_command_update( record: &mut crate::managed_agents::types::ManagedAgentRecord, personas: &[crate::managed_agents::types::AgentDefinition], agent_command: &str, harness_override: bool, -) { +) -> bool { record.agent_command_override = update_time_agent_command_override( record.persona_id.as_deref(), personas, Some(agent_command), harness_override, ); - if agent_command.trim().is_empty() && record.persona_id.is_some() { + let inherit_transition = agent_command.trim().is_empty() && record.persona_id.is_some(); + if inherit_transition { record.runtime = None; + // The generic canonical effort column is a per-instance pin; on the + // pin→inherit transition it is dropped so the agent inherits the + // persona/global effort. The record effort ENV aliases are stripped by + // the caller after `env_vars` is applied (see the doc above). + record.effort_level = None; + } + inherit_transition +} + +/// Strip every record-level thinking-effort env alias — all known native keys +/// plus the legacy `BUZZ_AGENT_THINKING_EFFORT` alias — from `env_vars`. +/// +/// Called at the `update_managed_agent` boundary on the pin→inherit transition, +/// AFTER caller-supplied `env_vars` have been applied, so the cleared aliases +/// cannot be reintroduced by the same request. Together with the column clear +/// in [`apply_agent_command_update`], this makes the instance drop its entire +/// per-instance effort override atomically at Save. +pub fn remove_record_effort_aliases(env_vars: &mut std::collections::BTreeMap) { + for key in crate::managed_agents::config_bridge::effort::effort_suppress_keys() { + env_vars.remove(key); + } +} + +/// Apply a same-request `env_vars` replacement and then enforce the pin→inherit +/// effort-alias strip, in that exact order. +/// +/// This is the ordering invariant Thufir's plan-of-record pins: the effort +/// column is cleared eagerly inside [`apply_agent_command_update`], but a stale +/// effort env alias in a caller-supplied `env_vars` map submitted in the SAME +/// request would otherwise survive the transition. Applying `env_vars` first, +/// then stripping the aliases only on the transition, guarantees the instance +/// cannot re-pin effort through the generic env channel while inheriting its +/// harness. `env_vars = None` leaves the record's existing env untouched; +/// validation of the supplied map is the caller's responsibility (it runs +/// before this seam at the update boundary). +pub fn apply_env_vars_then_effort_transition( + record: &mut crate::managed_agents::types::ManagedAgentRecord, + env_vars: Option>, + inherit_transition: bool, +) { + if let Some(env_vars) = env_vars { + record.env_vars = env_vars; + } + if inherit_transition { + remove_record_effort_aliases(&mut record.env_vars); } } diff --git a/desktop/src-tauri/src/managed_agents/discovery/runtime_metadata.rs b/desktop/src-tauri/src/managed_agents/discovery/runtime_metadata.rs index 34edecdcd9c..af6280a208e 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/runtime_metadata.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/runtime_metadata.rs @@ -1,3 +1,56 @@ +/// Canonicalization contract for a harness's thinking-effort env var. +/// +/// The single value authority shared by UI choices, the spawn/deploy launch +/// projection, and the reader. All effort candidates (native env, legacy env, +/// ACP tier, file tier) are normalized through `normalize_str` before any +/// validity, precedence, override, or B-equality check. +/// +/// Source for Goose: `crates/goose-provider-types/src/thinking.rs` +/// • `FromStr` (aliases, case-insensitive): `off|disabled|none`, `low`, +/// `medium|med`, `high`, `max|xhigh` +/// • `Display` (canonical): `off`, `low`, `medium`, `high`, `max` +/// • Live ACP emits Display values via `response_builder.rs:326-337`. +pub(crate) struct EffortNormalization { + /// Canonical values in UI display order (drive choices, persistence, ACP comparison). + pub canonical: &'static [&'static str], + /// `(alias, canonical)` pairs, case-insensitive. Only aliases that differ + /// from their canonical form are listed. + pub aliases: &'static [(&'static str, &'static str)], +} + +/// Goose thinking-effort canonicalization contract. +/// +/// Source: `crates/goose-provider-types/src/thinking.rs` at Goose `2db0e31fe`. +/// Canonical Display values: `off`, `low`, `medium`, `high`, `max`. +/// Aliases (case-insensitive): `none|disabled→off`, `med→medium`, `xhigh→max`. +/// `minimal` (Buzz-only) is invalid — skipped as absent at every tier. +pub(crate) static GOOSE_EFFORT_NORMALIZATION: EffortNormalization = EffortNormalization { + canonical: &["off", "low", "medium", "high", "max"], + aliases: &[ + ("none", "off"), + ("disabled", "off"), + ("med", "medium"), + ("xhigh", "max"), + ], +}; + +impl EffortNormalization { + /// Normalize `raw` to canonical form. `None` → invalid for this harness; + /// the caller must treat it as absent (skip-as-absent policy). + pub fn normalize_str(&self, raw: &str) -> Option { + let lower = raw.to_lowercase(); + if self.canonical.contains(&lower.as_str()) { + return Some(lower); + } + for &(alias, canon) in self.aliases { + if lower == alias { + return Some(canon.to_string()); + } + } + None + } +} + /// Static capabilities and installation metadata for a known ACP runtime. pub(crate) struct KnownAcpRuntime { pub id: &'static str, @@ -47,6 +100,23 @@ pub(crate) struct KnownAcpRuntime { pub config_file_format: Option<&'static str>, pub supports_acp_native_config: bool, // tier 1a: config/read+write pub thinking_env_var: Option<&'static str>, + /// Canonicalization contract for `thinking_env_var` on this harness. + /// + /// `Some(contract)` — harness uses a finite, static effort vocabulary. + /// All candidates (native env, legacy env, ACP tier, file tier) are + /// normalized through this contract before validity checks, precedence + /// resolution, override tracking, and B-equality comparison. + /// + /// `None` — harness accepts any provider/model-specific value via its own + /// catalog (buzz-agent); see `getProviderEffortConfig()` in TS for that + /// path. Contract-less does NOT mean keyless: buzz-agent still has a native + /// `thinking_env_var`, and Claude/Codex route the canonical through + /// `BUZZ_ACP_EFFORT_LEVEL` for ACP startup even with `thinking_env_var: None`. + /// + /// The single canonical authority shared by UI choices, the launch + /// projection, and the reader. No value-authority logic may live outside + /// this struct for harnesses that declare one. + pub effort_normalization: Option<&'static EffortNormalization>, /// Env var for normalizing `max_output_tokens`. `None` when the harness /// does not have a first-class env var for this field (config-file only). pub max_tokens_env_var: Option<&'static str>, diff --git a/desktop/src-tauri/src/managed_agents/discovery/tests.rs b/desktop/src-tauri/src/managed_agents/discovery/tests.rs index f7e233fbe95..76d0d17da6f 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/tests.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/tests.rs @@ -2,13 +2,13 @@ use std::path::PathBuf; use super::overrides::{divergent_agent_command_override, update_time_agent_command_override}; use super::{ - apply_agent_command_update, classify_runtime, codex_adapter_availability, - codex_adapter_is_outdated, create_time_agent_command_override, default_agent_command, - effective_agent_command, find_nvm_default_bin, find_via_login_shell, + apply_agent_command_update, apply_env_vars_then_effort_transition, classify_runtime, + codex_adapter_availability, codex_adapter_is_outdated, create_time_agent_command_override, + default_agent_command, effective_agent_command, find_nvm_default_bin, find_via_login_shell, is_login_shell_path_uninit, is_safe_nvm_tag, managed_agent_avatar_url, normalize_agent_args, parse_semver_tag, probe_codex_acp_version, record_agent_command, refresh_login_shell_path, - try_record_agent_command, BUZZ_AGENT_AVATAR_URL, CLAUDE_CODE_AVATAR_URL, CODEX_AVATAR_URL, - GOOSE_AVATAR_URL, + remove_record_effort_aliases, try_record_agent_command, BUZZ_AGENT_AVATAR_URL, + CLAUDE_CODE_AVATAR_URL, CODEX_AVATAR_URL, GOOSE_AVATAR_URL, }; use crate::managed_agents::AcpAvailabilityStatus; @@ -623,51 +623,9 @@ fn update_time_override_preserves_pin_for_persona_less_agent() { ); } -#[test] -fn apply_agent_command_update_inherit_sentinel_clears_pin_and_runtime() { - // Choosing Inherit on a persona-linked record clears BOTH the explicit - // pin and the materialized runtime, so resolution falls through to the - // live definition immediately — not on the next spawn. - let personas = vec![persona_with_runtime("p1", Some("goose"))]; - let mut record = record_with(Some("claude"), Some("p1"), Some("codex-acp")); - - apply_agent_command_update(&mut record, &personas, "", false); - - assert_eq!(record.agent_command_override, None); - assert_eq!(record.runtime, None); - assert_eq!(record_agent_command(&record, &personas), "goose"); -} - -#[test] -fn apply_agent_command_update_sentinel_keeps_runtime_for_definition_less_record() { - // For a record with no persona link the materialized runtime is the only - // harness source left once the pin is cleared — a stray empty - // agent_command must not change what the agent runs. - let mut record = record_with(Some("claude"), None, Some("codex-acp")); - - apply_agent_command_update(&mut record, &[], "", false); - - assert_eq!(record.agent_command_override, None); - assert_eq!(record.runtime.as_deref(), Some("claude")); - assert_eq!(record_agent_command(&record, &[]), "claude-agent-acp"); -} - -#[test] -fn apply_agent_command_update_concrete_pin_keeps_materialized_runtime() { - // A concrete pick only sets the pin; the materialized runtime is left for - // the next snapshot apply. The pin shadows it in resolution either way. - let personas = vec![persona_with_runtime("p1", Some("goose"))]; - let mut record = record_with(Some("claude"), Some("p1"), None); - - apply_agent_command_update(&mut record, &personas, "codex-acp", true); - - assert_eq!(record.agent_command_override.as_deref(), Some("codex-acp")); - assert_eq!(record.runtime.as_deref(), Some("claude")); - assert_eq!(record_agent_command(&record, &personas), "codex-acp"); -} - // ── probe_codex_acp_version ─────────────────────────────────────────────────── +mod effort_clear; mod managed_path_resolution; #[cfg(unix)] diff --git a/desktop/src-tauri/src/managed_agents/discovery/tests/effort_clear.rs b/desktop/src-tauri/src/managed_agents/discovery/tests/effort_clear.rs new file mode 100644 index 00000000000..bdeb1a10802 --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/discovery/tests/effort_clear.rs @@ -0,0 +1,188 @@ +//! Backend tests for the pin→inherit effort clear (PR #4625, plan-of-record +//! item 1): the sentinel transition clears the canonical column eagerly and the +//! update boundary strips the record effort env aliases AFTER caller `env_vars` +//! is applied. Split out of `discovery/tests.rs` to hold that file under the +//! desktop file-size ratchet. +//! +//! `use super::*` pulls the parent test module's helpers (`record_with`, +//! `persona_with_runtime`, `record_agent_command`) and its imported command +//! surface (`apply_agent_command_update`, `apply_env_vars_then_effort_transition`, +//! `remove_record_effort_aliases`). + +use super::*; + +#[test] +fn apply_agent_command_update_inherit_sentinel_clears_pin_runtime_and_column() { + // Choosing Inherit on a persona-linked record clears the explicit pin, the + // materialized runtime, AND the per-instance effort column, so resolution + // falls through to the live definition immediately — not on the next spawn. + // The transition flag fires so the caller strips the record effort env + // aliases after `env_vars` is applied. + let personas = vec![persona_with_runtime("p1", Some("goose"))]; + let mut record = record_with(Some("claude"), Some("p1"), Some("codex-acp")); + record.effort_level = Some("high".into()); + + let transition = apply_agent_command_update(&mut record, &personas, "", false); + + assert!(transition, "the pin→inherit transition must be signalled"); + assert_eq!(record.agent_command_override, None); + assert_eq!(record.runtime, None); + assert_eq!( + record.effort_level, None, + "the effort column must be cleared" + ); + assert_eq!(record_agent_command(&record, &personas), "goose"); +} + +#[test] +fn apply_agent_command_update_sentinel_keeps_runtime_for_definition_less_record() { + // For a record with no persona link the materialized runtime is the only + // harness source left once the pin is cleared — a stray empty + // agent_command must not change what the agent runs, nor clear its effort. + let mut record = record_with(Some("claude"), None, Some("codex-acp")); + record.effort_level = Some("high".into()); + + let transition = apply_agent_command_update(&mut record, &[], "", false); + + assert!( + !transition, + "a definition-less stray sentinel is not a pin→inherit transition" + ); + assert_eq!(record.agent_command_override, None); + assert_eq!(record.runtime.as_deref(), Some("claude")); + assert_eq!( + record.effort_level.as_deref(), + Some("high"), + "a definition-less record must preserve its effort column" + ); + assert_eq!(record_agent_command(&record, &[]), "claude-agent-acp"); +} + +#[test] +fn apply_agent_command_update_concrete_pin_keeps_materialized_runtime_and_column() { + // A concrete pick only sets the pin; the materialized runtime and the + // effort column are left intact (no ownership transition). The pin shadows + // the runtime in resolution either way. + let personas = vec![persona_with_runtime("p1", Some("goose"))]; + let mut record = record_with(Some("claude"), Some("p1"), None); + record.effort_level = Some("high".into()); + + let transition = apply_agent_command_update(&mut record, &personas, "codex-acp", true); + + assert!( + !transition, + "a concrete pin is not a pin→inherit transition" + ); + assert_eq!(record.agent_command_override.as_deref(), Some("codex-acp")); + assert_eq!(record.runtime.as_deref(), Some("claude")); + assert_eq!( + record.effort_level.as_deref(), + Some("high"), + "a concrete pin must preserve the effort column" + ); + assert_eq!(record_agent_command(&record, &personas), "codex-acp"); +} + +#[test] +fn remove_record_effort_aliases_strips_all_known_and_legacy_keys() { + // The update-boundary alias strip: after `env_vars` is applied on the + // pin→inherit transition, every known native effort key and the legacy + // alias must be removed, while unrelated env survives. This proves the + // second half of the atomic clear that a helper-only column clear cannot. + let mut env: std::collections::BTreeMap = [ + ("GOOSE_THINKING_EFFORT", "high"), + ("BUZZ_AGENT_THINKING_EFFORT", "high"), + ("BUZZ_ACP_EFFORT_LEVEL", "high"), + ("UNRELATED_KEY", "keep"), + ] + .into_iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect(); + + remove_record_effort_aliases(&mut env); + + assert!(!env.contains_key("GOOSE_THINKING_EFFORT")); + assert!(!env.contains_key("BUZZ_AGENT_THINKING_EFFORT")); + assert!(!env.contains_key("BUZZ_ACP_EFFORT_LEVEL")); + assert_eq!( + env.get("UNRELATED_KEY").map(String::as_str), + Some("keep"), + "unrelated env must survive the effort-alias strip" + ); +} + +#[test] +fn update_boundary_inherit_sentinel_with_alias_bearing_env_vars_strips_after_apply() { + // The update-boundary ORDERING invariant (Thufir pass-3): on the pin→inherit + // transition, a SAME-REQUEST `env_vars` map carrying a stale effort alias + // must NOT survive. `apply_agent_command_update` clears the column eagerly; + // then `apply_env_vars_then_effort_transition` applies the caller env FIRST + // and strips the aliases AFTER — so the alias the request tried to + // reintroduce is gone. A helper-only test cannot prove this order. + let personas = vec![persona_with_runtime("p1", Some("goose"))]; + let mut record = record_with(Some("claude"), Some("p1"), Some("codex-acp")); + record.effort_level = Some("high".into()); + + let transition = apply_agent_command_update(&mut record, &personas, "", false); + assert!( + transition, + "empty command on a persona-linked record is inherit" + ); + + // The request replaces env_vars with a map that re-pins effort via an alias + // plus an unrelated key. + let request_env: std::collections::BTreeMap = [ + ("GOOSE_THINKING_EFFORT", "max"), + ("BUZZ_ACP_EFFORT_LEVEL", "max"), + ("UNRELATED_KEY", "keep"), + ] + .into_iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect(); + + apply_env_vars_then_effort_transition(&mut record, Some(request_env), transition); + + assert_eq!(record.effort_level, None, "column stays cleared"); + assert!( + !record.env_vars.contains_key("GOOSE_THINKING_EFFORT"), + "same-request native alias must not survive the transition" + ); + assert!( + !record.env_vars.contains_key("BUZZ_ACP_EFFORT_LEVEL"), + "same-request ACP sentinel must not survive the transition" + ); + assert_eq!( + record.env_vars.get("UNRELATED_KEY").map(String::as_str), + Some("keep"), + "unrelated env from the same request is preserved" + ); +} + +#[test] +fn update_boundary_concrete_pin_preserves_alias_bearing_env_vars() { + // No transition (concrete pin): the caller `env_vars` — including any effort + // alias — is applied verbatim and NOT stripped. Effort env is only cleared + // on the ownership transition, never on an ordinary env edit. + let personas = vec![persona_with_runtime("p1", Some("goose"))]; + let mut record = record_with(Some("claude"), Some("p1"), None); + + let transition = apply_agent_command_update(&mut record, &personas, "codex-acp", true); + assert!(!transition, "a concrete pin is not a transition"); + + let request_env: std::collections::BTreeMap = + [("GOOSE_THINKING_EFFORT", "max")] + .into_iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect(); + + apply_env_vars_then_effort_transition(&mut record, Some(request_env), transition); + + assert_eq!( + record + .env_vars + .get("GOOSE_THINKING_EFFORT") + .map(String::as_str), + Some("max"), + "without a transition the caller effort env is preserved" + ); +} diff --git a/desktop/src-tauri/src/managed_agents/readiness.rs b/desktop/src-tauri/src/managed_agents/readiness.rs index f7f5d5c5d0e..c40d4417324 100644 --- a/desktop/src-tauri/src/managed_agents/readiness.rs +++ b/desktop/src-tauri/src/managed_agents/readiness.rs @@ -269,6 +269,20 @@ fn resolve_effective_agent_env_with_def( ); env.extend(user_env); + // Single harness-agnostic effort authority (PR #4625): resolve effective + // effort over the canonical column AND all env tiers, emit one destination + // key. Runs AFTER the layer stack so launch, remote deploy, and the restart + // snapshot agree — no double authority, no foreign key, no badge disagreement. + super::config_bridge::effort::apply_launch_effort( + &mut env, + record, + runtime, + personas, + &global.env_vars, + harness_def.as_deref(), + &baked_build_env(), + ); + // Buzz shared compute is a native Buzz provider. Translate it to buzz-agent's // OpenAI-compatible transport only in the effective runtime environment. #[cfg(feature = "mesh-llm")] @@ -1049,6 +1063,7 @@ mod tests { default_env: &[], supports_acp_native_config: false, thinking_env_var: None, + effort_normalization: None, max_tokens_env_var: None, context_limit_env_var: None, max_rounds_env_var: None, @@ -1241,6 +1256,7 @@ mod tests { default_env: &[], supports_acp_native_config: false, thinking_env_var: None, + effort_normalization: None, max_tokens_env_var: None, context_limit_env_var: None, max_rounds_env_var: None, @@ -1680,57 +1696,10 @@ mod tests { })); } - // ── OpenRouter readiness ───────────────────────────────────────────── - - #[test] - fn buzz_agent_openrouter_with_all_fields_is_ready() { - let env = make_env( - "buzz-agent", - env_with(&[ - ("BUZZ_AGENT_PROVIDER", "openrouter"), - ("BUZZ_AGENT_MODEL", "anthropic/claude-sonnet-4"), - ("OPENROUTER_API_KEY", "sk-or-test-key"), - ]), - ); - let result = agent_readiness(&env); - assert!( - result.is_ready(), - "openrouter with all fields should be ready" - ); - } - - #[test] - fn buzz_agent_openrouter_missing_key_returns_not_ready() { - let env = make_env( - "buzz-agent", - env_with(&[ - ("BUZZ_AGENT_PROVIDER", "openrouter"), - ("BUZZ_AGENT_MODEL", "anthropic/claude-sonnet-4"), - ]), - ); - let result = agent_readiness(&env); - assert!(!result.is_ready()); - assert!(result.requirements().contains(&Requirement::EnvKey { - key: "OPENROUTER_API_KEY".to_string() - })); - } - - #[test] - fn buzz_agent_openrouter_with_provider_model_fallback_is_ready() { - let env = make_env( - "buzz-agent", - env_with(&[ - ("BUZZ_AGENT_PROVIDER", "openrouter"), - ("OPENROUTER_MODEL", "google/gemini-2.5-flash"), - ("OPENROUTER_API_KEY", "sk-or-test-key"), - ]), - ); - let result = agent_readiness(&env); - assert!( - result.is_ready(), - "OPENROUTER_MODEL fallback should satisfy model requirement" - ); - } + // buzz-agent OpenRouter readiness tests live in a sibling file so this + // module stays under the desktop file-size ratchet. + #[path = "openrouter_tests.rs"] + mod openrouter_tests; } // Goose file-config-aware requirement tests live in a sibling file so this diff --git a/desktop/src-tauri/src/managed_agents/readiness/tests/openrouter_tests.rs b/desktop/src-tauri/src/managed_agents/readiness/tests/openrouter_tests.rs new file mode 100644 index 00000000000..73b3fcda4b8 --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/readiness/tests/openrouter_tests.rs @@ -0,0 +1,57 @@ +//! buzz-agent OpenRouter readiness tests, split from `readiness.rs`'s `tests` +//! module so that file stays under the desktop file-size ratchet. +//! +//! Declared as a child of `mod tests` via `#[path]`, so `use super::*` resolves +//! against that module and reaches its `make_env`/`env_with` helpers. + +use super::*; + +#[test] +fn buzz_agent_openrouter_with_all_fields_is_ready() { + let env = make_env( + "buzz-agent", + env_with(&[ + ("BUZZ_AGENT_PROVIDER", "openrouter"), + ("BUZZ_AGENT_MODEL", "anthropic/claude-sonnet-4"), + ("OPENROUTER_API_KEY", "sk-or-test-key"), + ]), + ); + let result = agent_readiness(&env); + assert!( + result.is_ready(), + "openrouter with all fields should be ready" + ); +} + +#[test] +fn buzz_agent_openrouter_missing_key_returns_not_ready() { + let env = make_env( + "buzz-agent", + env_with(&[ + ("BUZZ_AGENT_PROVIDER", "openrouter"), + ("BUZZ_AGENT_MODEL", "anthropic/claude-sonnet-4"), + ]), + ); + let result = agent_readiness(&env); + assert!(!result.is_ready()); + assert!(result.requirements().contains(&Requirement::EnvKey { + key: "OPENROUTER_API_KEY".to_string() + })); +} + +#[test] +fn buzz_agent_openrouter_with_provider_model_fallback_is_ready() { + let env = make_env( + "buzz-agent", + env_with(&[ + ("BUZZ_AGENT_PROVIDER", "openrouter"), + ("OPENROUTER_MODEL", "google/gemini-2.5-flash"), + ("OPENROUTER_API_KEY", "sk-or-test-key"), + ]), + ); + let result = agent_readiness(&env); + assert!( + result.is_ready(), + "OPENROUTER_MODEL fallback should satisfy model requirement" + ); +} diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index 0ce5ca7b219..c84a2eb5619 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -14,7 +14,7 @@ use crate::{ util::now_iso, }; -use super::claude_config::{apply_claude_model_env, apply_effort_env}; +use super::claude_config::apply_claude_model_env; mod path; pub(in crate::managed_agents) use path::build_augmented_path; pub(crate) use path::{compose_path_entries, should_skip_claude_executable, should_use_inherited}; @@ -810,13 +810,13 @@ pub fn spawn_agent_child( command.env(key, value); } - // B5: carry persisted effort; harness resolves thought_level configId at first session. - // Written AFTER descriptor.env so the canonical persisted value wins over any - // user-supplied BUZZ_ACP_EFFORT_LEVEL entry, mirroring the A1 model-authority pattern - // (ANTHROPIC_MODEL is applied post-loop for the same reason). When effort_level is - // None there is no canonical value to assert, so env passthrough stands — user env - // legitimately seeds startup effort in that case. - apply_effort_env(&mut command, record.effort_level.as_deref()); + // Effort authority is already resolved: the single harness-agnostic effort + // projection ran inside `resolve_effective_agent_env_with_def`, so + // `descriptor.env` (written above) carries exactly one effort key holding the + // effective value — every foreign/legacy/transport effort key was stripped + // there. Re-applying `apply_effort_env` here would double-write the ACP + // sentinel and, on a Goose descriptor, launch a second effort key alongside + // `GOOSE_THINKING_EFFORT`. No post-loop effort write is needed. // A1: for local claude agents, ANTHROPIC_MODEL is the single startup model authority. // BUZZ_ACP_MODEL is removed (live ACP switches only; two authorities in the same env diff --git a/desktop/src-tauri/src/managed_agents/spawn_snapshot.rs b/desktop/src-tauri/src/managed_agents/spawn_snapshot.rs index 8a6f68a693d..55ec314acc6 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_snapshot.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_snapshot.rs @@ -31,7 +31,6 @@ use std::collections::BTreeMap; use serde::Serialize; use super::{ - claude_config::EFFORT_LEVEL_ENV_VAR, effective_config::{resolve_effective_config, EffectiveConfigResult}, known_acp_runtime, normalize_agent_args, persona_events::preview_prospective_persona_snapshot, @@ -128,30 +127,30 @@ pub(crate) struct SpawnConfigSnapshot { pub max_turn_duration_seconds: Option, pub parallelism: u32, /// The startup effort the harness will actually apply, resolved by - /// [`effective_effort`]: the persisted canonical `record.effort_level` when - /// present, else the user-seeded `BUZZ_ACP_EFFORT_LEVEL` from the layered - /// env. This is the *sole* representation of effort in the snapshot — the - /// key is stripped from `env` (see `from_inputs`) so an authority handoff - /// that leaves the effective value unchanged (canonical `low` replacing a - /// user env `low`, or the reverse) produces no spurious drift entry, and an - /// env-only edit still surfaces as exactly one `effort_level` entry. + /// [`effective_effort`]: the single effort key the harness-agnostic + /// projection left in `descriptor.env` under the runtime's destination key. + /// This is the *sole* representation of effort in the snapshot — every + /// effort key is stripped from `env` (see `from_inputs`) so an authority + /// handoff that leaves the effective value unchanged produces no spurious + /// drift entry, and an effort edit surfaces as exactly one `effort_level` + /// entry. pub effort_level: Option, } -/// The startup effort a spawn would actually apply, mirroring `apply_effort_env` -/// exactly: the persisted canonical `record.effort_level` wins, and only when it -/// is absent does a user-supplied `BUZZ_ACP_EFFORT_LEVEL` from the layered env -/// seed startup effort. This is the resolver input for the snapshot's single -/// `effort_level` representation; the same precedence runs at spawn time in -/// `runtime.rs`, so badge and process can never disagree. -pub(crate) fn effective_effort( - record: &ManagedAgentRecord, - descriptor_env: &BTreeMap, -) -> Option { - record - .effort_level - .clone() - .or_else(|| descriptor_env.get(EFFORT_LEVEL_ENV_VAR).cloned()) +/// The startup effort a spawn actually applied, read from the single effort key +/// the harness-agnostic projection left in `descriptor.env`. +/// +/// The projection (`config_bridge::effort`) ran inside the descriptor resolver, +/// resolving the effective value over the canonical column and every env tier, +/// then reducing the env to exactly one effort key under the runtime's +/// destination key (`effort_dest_key`). Reading that key here means the badge +/// compares precisely what launched — no separate precedence to drift from the +/// spawn path, and an invalid canonical that fell through to an inherited tier +/// is reflected as the inherited value, not the raw column. +pub(crate) fn effective_effort(descriptor: &EffectiveHarnessDescriptor) -> Option { + let runtime = known_acp_runtime(&descriptor.command); + let dest_key = super::config_bridge::effort::effort_dest_key(runtime); + descriptor.env.get(dest_key).cloned() } impl SpawnConfigSnapshot { @@ -178,14 +177,16 @@ impl SpawnConfigSnapshot { .unwrap_or("") .to_string(), // Effort has ONE representation in the snapshot: `effort_level` - // below, always holding `effective_effort`. Stripping the env key - // here means a canonical/user-env authority handoff at the same - // value is a no-op (no phantom `env.BUZZ_ACP_EFFORT_LEVEL` add or - // remove) and an env-only effort edit surfaces as exactly one + // below, always holding the projected effective value. Every effort + // key is stripped from `env` (the full suppress set) so an authority + // handoff at the same value is a no-op (no phantom `env.*EFFORT*` add + // or remove) and an env-only effort edit surfaces as exactly one // `effort_level` entry rather than a duplicate under `env.`. env: { let mut env = descriptor.env.clone(); - env.remove(EFFORT_LEVEL_ENV_VAR); + for key in super::config_bridge::effort::effort_suppress_keys() { + env.remove(key); + } env }, relay_url: relay_url.to_string(), @@ -215,10 +216,10 @@ impl SpawnConfigSnapshot { // effective value — that is correct, it is what actually runs. parallelism: super::effective_parallelism(&descriptor.command, record.parallelism), // Sole effort representation — see the field doc and the `env` - // strip above. Resolver reads the record's canonical value and the - // raw descriptor env (before the strip), so a user-seeded env value - // is preserved as the effective effort when no canonical is set. - effort_level: effective_effort(record, &descriptor.env), + // strip above. Reads the single projected effort key the descriptor + // resolver left in `descriptor.env`, so the badge compares exactly + // what launched regardless of which tier supplied the value. + effort_level: effective_effort(descriptor), } } diff --git a/desktop/src-tauri/src/managed_agents/spawn_snapshot/tests_ext.rs b/desktop/src-tauri/src/managed_agents/spawn_snapshot/tests_ext.rs index dd708b6e59e..f258b90a6d0 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_snapshot/tests_ext.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_snapshot/tests_ext.rs @@ -27,86 +27,112 @@ fn effort_set_then_cleared_round_trips_to_no_effort_projection() { } #[test] -fn shadowed_user_env_effort_edit_under_canonical_is_empty_diff() { - // Canonical `high` shadows the user env seed. Editing that seed low→medium - // changes nothing effective (canonical wins and the env key is stripped), - // so the projections are identical and no badge lights. - let mut low_env = record_with_env_effort("low"); - low_env.effort_level = Some("high".into()); - let mut medium_env = record_with_env_effort("medium"); - medium_env.effort_level = Some("high".into()); +fn canonical_edit_under_record_native_env_is_empty_diff() { + // For Goose, the record-native env key `GOOSE_THINKING_EFFORT` outranks the + // canonical column (CLEAR authority order). With a record-native `low` + // present, editing the shadowed canonical high→medium changes nothing + // effective, so the projections are identical and no badge lights. + let mut high_col = record_with_env_effort("low"); + high_col.effort_level = Some("high".into()); + let mut medium_col = record_with_env_effort("low"); + medium_col.effort_level = Some("medium".into()); assert_eq!( - snap(&low_env), - snap(&medium_env), - "editing a canonical-shadowed user env must not badge" + snap(&high_col), + snap(&medium_col), + "editing a record-native-env-shadowed canonical must not badge" ); } #[test] -fn clearing_canonical_reveals_env_fallback_and_creates_a_diff() { - // Canonical `high` over a user env seed `low`: clearing the canonical drops - // the effective effort to the env fallback `low`, a real change that badges. - let mut canonical = record_with_env_effort("low"); - canonical.effort_level = Some("high".into()); - let env_only = record_with_env_effort("low"); +fn clearing_record_native_env_reveals_canonical_and_creates_a_diff() { + // Record-native env `low` shadows canonical `high`: removing the record env + // key drops resolution to the canonical `high`, a real change that badges. + let mut env_over_canonical = record_with_env_effort("low"); + env_over_canonical.effort_level = Some("high".into()); + let mut canonical_only = goose_record(); + canonical_only.effort_level = Some("high".into()); assert_ne!( - snap(&canonical), - snap(&env_only), - "clearing canonical must reveal the env fallback and badge" + snap(&env_over_canonical), + snap(&canonical_only), + "removing the record-native env must reveal the canonical and badge" ); } -// ── B5 effort: single canonical representation ─────────────────────────── +// ── Effort: single canonical representation ────────────────────────────── // // `effective_effort` and the snapshot's `effort_level` field are the sole -// carrier of startup effort. `BUZZ_ACP_EFFORT_LEVEL` is stripped from the -// snapshot `env` so an authority handoff at an unchanged effective value -// (canonical replacing a user-env seed, or the reverse) raises no spurious -// restart badge, while a genuine effort change surfaces exactly once. +// carrier of startup effort. Every effort key is stripped from the snapshot +// `env` so an authority handoff at an unchanged effective value raises no +// spurious restart badge, while a genuine effort change surfaces exactly once. -/// Look up the `env.BUZZ_ACP_EFFORT_LEVEL` leaf of a canonical snapshot, if any. +/// Look up the `env.GOOSE_THINKING_EFFORT` leaf of a canonical snapshot, if any +/// (the record()'s runtime is Goose, so this is its destination key). fn effort_env_leaf(canonical: &serde_json::Value) -> Option<&serde_json::Value> { canonical .get("env") - .and_then(|env| env.get("BUZZ_ACP_EFFORT_LEVEL")) + .and_then(|env| env.get("GOOSE_THINKING_EFFORT")) } -/// A record whose user env seeds `BUZZ_ACP_EFFORT_LEVEL` (the pre-canonical -/// authority: no persisted `effort_level`, effort comes from user env_vars). +/// A Goose record whose record-native env seeds `GOOSE_THINKING_EFFORT` (the +/// top authority tier for Goose: effort comes from user env_vars, no column). +/// Pins `runtime = "goose"` so the effective command resolves to Goose and +/// `GOOSE_THINKING_EFFORT` is the record-*native* key — without it the record +/// falls back to the default `buzz-agent` runtime, for which that key is a +/// foreign env alias the projection suppresses rather than an authority tier. fn record_with_env_effort(value: &str) -> ManagedAgentRecord { let mut rec = record(); + rec.runtime = Some("goose".into()); rec.env_vars - .insert("BUZZ_ACP_EFFORT_LEVEL".into(), value.into()); + .insert("GOOSE_THINKING_EFFORT".into(), value.into()); rec } -#[test] -fn effective_effort_prefers_persisted_canonical_over_user_env() { - // Canonical wins, mirroring spawn's `apply_effort_env` (written after the - // user env layer). The env value is ignored when a canonical is present. +/// A Goose record with no effort env: the canonical column is the authority. +fn goose_record() -> ManagedAgentRecord { let mut rec = record(); - rec.effort_level = Some("high".into()); - let env = BTreeMap::from([("BUZZ_ACP_EFFORT_LEVEL".to_string(), "low".to_string())]); - assert_eq!(effective_effort(&rec, &env).as_deref(), Some("high")); + rec.runtime = Some("goose".into()); + rec +} + +#[test] +fn effective_effort_reads_the_projected_key_for_the_runtime() { + // The projection reduced the descriptor env to one effort key under the + // runtime's destination key. `effective_effort` reads exactly that key. + // A Goose descriptor carries `GOOSE_THINKING_EFFORT`. + let descriptor = EffectiveHarnessDescriptor { + command: "goose".into(), + args: vec![], + env: BTreeMap::from([("GOOSE_THINKING_EFFORT".to_string(), "high".to_string())]), + }; + assert_eq!(effective_effort(&descriptor).as_deref(), Some("high")); } #[test] -fn effective_effort_falls_back_to_user_env_when_no_canonical() { - // No persisted canonical → the user-seeded env value is the effective - // startup effort, exactly what a spawn would leave in place. - let rec = record(); - let env = BTreeMap::from([("BUZZ_ACP_EFFORT_LEVEL".to_string(), "low".to_string())]); - assert_eq!(effective_effort(&rec, &env).as_deref(), Some("low")); +fn effective_effort_reads_acp_sentinel_for_keyless_runtime() { + // Claude/Codex/keyless-ACP descriptors carry the effective value under the + // ACP-startup sentinel, which is the destination key for a runtime with no + // native thinking-effort env var (here: the claude adapter command). + let descriptor = EffectiveHarnessDescriptor { + command: "claude-code-acp".into(), + args: vec![], + env: BTreeMap::from([("BUZZ_ACP_EFFORT_LEVEL".to_string(), "low".to_string())]), + }; + assert_eq!(effective_effort(&descriptor).as_deref(), Some("low")); } #[test] -fn effective_effort_is_none_without_canonical_or_env() { - assert_eq!(effective_effort(&record(), &BTreeMap::new()), None); +fn effective_effort_is_none_without_a_projected_key() { + let descriptor = EffectiveHarnessDescriptor { + command: "goose".into(), + args: vec![], + env: BTreeMap::new(), + }; + assert_eq!(effective_effort(&descriptor), None); } #[test] fn snapshot_carries_effort_in_field_not_env() { - // Always-canonicalize: a user-seeded effort reaches the snapshot ONLY as + // Always-canonicalize: a record-native effort reaches the snapshot ONLY as // the `effort_level` field; the raw env key is stripped so effort has one // representation, never two. let canonical = snap(&record_with_env_effort("low")); @@ -118,50 +144,63 @@ fn snapshot_carries_effort_in_field_not_env() { assert_eq!( effort_env_leaf(&canonical), None, - "BUZZ_ACP_EFFORT_LEVEL must be stripped from the snapshot env" + "GOOSE_THINKING_EFFORT must be stripped from the snapshot env" ); } #[test] -fn equal_value_effort_authority_handoff_env_to_canonical_is_no_op() { - // User env `low` (no canonical) → persisted canonical `low` while the env - // seed remains: the effective effort is `low` either way, so a restart - // would change nothing. Old raw-env snapshots would have shown drift; the - // single canonical representation makes the projections identical. - let env_authority = record_with_env_effort("low"); - let mut canonical_authority = record_with_env_effort("low"); - canonical_authority.effort_level = Some("low".into()); +fn foreign_transport_sentinel_is_suppressed_for_goose() { + // A user-seeded `BUZZ_ACP_EFFORT_LEVEL` is a foreign transport key for a + // Goose descriptor: never an authority tier, and stripped from the snapshot + // env by the suppress set. Editing it low→medium changes nothing. + let mut low = record(); + low.env_vars + .insert("BUZZ_ACP_EFFORT_LEVEL".into(), "low".into()); + let mut medium = record(); + medium + .env_vars + .insert("BUZZ_ACP_EFFORT_LEVEL".into(), "medium".into()); assert_eq!( - snap(&env_authority), - snap(&canonical_authority), - "an authority handoff at the same effort value must not badge" + snap(&low), + snap(&medium), + "a foreign transport effort key must be suppressed for Goose and never badge" + ); + let canonical = snap(&low); + assert_eq!( + canonical + .get("env") + .and_then(|env| env.get("BUZZ_ACP_EFFORT_LEVEL")), + None, + "the foreign sentinel must be stripped from the snapshot env" ); } #[test] -fn equal_value_effort_authority_handoff_canonical_to_env_is_no_op() { - // The reverse direction: canonical `low` (env seed present) → env `low` - // only (canonical cleared). Effective effort stays `low`; no badge. +fn equal_value_effort_authority_handoff_env_to_canonical_is_no_op() { + // Record-native env `low` (no column) → canonical column `low` while the + // record env remains: the effective effort is `low` either way (env wins, + // but the value is identical), so a restart would change nothing. + let env_authority = record_with_env_effort("low"); let mut canonical_authority = record_with_env_effort("low"); canonical_authority.effort_level = Some("low".into()); - let env_authority = record_with_env_effort("low"); assert_eq!( - snap(&canonical_authority), snap(&env_authority), - "clearing the canonical while the env seed holds the same value must not badge" + snap(&canonical_authority), + "an authority handoff at the same effort value must not badge" ); } #[test] fn env_only_effort_edit_changes_effort_level_not_env() { - // An env-only effort edit (no canonical) moves the single `effort_level` - // representation and never reintroduces an `env.BUZZ_ACP_EFFORT_LEVEL` - // leaf, so the diff names `effort_level` once rather than duplicating it. + // A record-native env effort edit (no column) moves the single + // `effort_level` representation and never reintroduces a + // `env.GOOSE_THINKING_EFFORT` leaf, so the diff names `effort_level` once + // rather than duplicating it. let low = snap(&record_with_env_effort("low")); let high = snap(&record_with_env_effort("high")); assert_ne!( low, high, - "an env-only effort edit must change the snapshot" + "a record-native effort edit must change the snapshot" ); assert_eq!( low.get("effort_level").and_then(|v| v.as_str()), diff --git a/desktop/src-tauri/src/managed_agents/types.rs b/desktop/src-tauri/src/managed_agents/types.rs index 9049482de3a..d3426a93756 100644 --- a/desktop/src-tauri/src/managed_agents/types.rs +++ b/desktop/src-tauri/src/managed_agents/types.rs @@ -440,8 +440,14 @@ pub struct ManagedAgentRecord { /// deserialize as `None`. #[serde(default, skip_serializing_if = "Option::is_none")] pub relay_mesh: Option, - /// Canonical Claude Code effort level. Injected as `BUZZ_ACP_EFFORT_LEVEL` at spawn - /// so the harness applies it via `session/set_config_option` at session creation. + /// Canonical, harness-agnostic startup effort level. This is the single + /// persisted effort authority: at spawn the launch projection + /// (`config_bridge::effort`) resolves the effective value over this column + /// and all env tiers, then emits it under the destination runtime's native + /// key — `GOOSE_THINKING_EFFORT` for Goose, `BUZZ_AGENT_THINKING_EFFORT` for + /// buzz-agent, or the `BUZZ_ACP_EFFORT_LEVEL` startup sentinel for + /// Claude/Codex and keyless/unknown adapters. Preserved across runtime + /// switches (invalid values skip-as-absent at projection time). #[serde(default, skip_serializing_if = "Option::is_none")] pub effort_level: Option, } diff --git a/desktop/src/features/agents/AGENTS.md b/desktop/src/features/agents/AGENTS.md index 88f2a3c9821..ce914b1f214 100644 --- a/desktop/src/features/agents/AGENTS.md +++ b/desktop/src/features/agents/AGENTS.md @@ -220,8 +220,9 @@ with a TypeScript lookup table or an id comparison in a component. dialog (see rule 11): keep effort state inside the section component, never as dialog-level props. The read-only display is the `thinkingEffort` normalized field rendered by `AgentConfigPanel` via `NormalizedRow`, which - already shows both facts — `field.value` (canonical, the effort the next - spawn will launch with) and, when a running ACP session differs, + already shows both facts — `field.value` (canonical: the effort the next + spawn will launch with, projected to the runtime's native key) and, when a + running ACP session differs, `field.overriddenValue` struck through (the live session's current effort). No component owns "configured vs current" logic; the reader's canonical tier ordering feeds both facts. Do not add a second effort write path or restate diff --git a/desktop/src/features/agents/ui/agentInstanceEditCancelSafety.test.mjs b/desktop/src/features/agents/ui/agentInstanceEditCancelSafety.test.mjs new file mode 100644 index 00000000000..a8338a7ee91 --- /dev/null +++ b/desktop/src/features/agents/ui/agentInstanceEditCancelSafety.test.mjs @@ -0,0 +1,432 @@ +/** + * Cancel-safety acceptance pin (production seam). + * + * The pin→inherit effort/runtime clear is derived entirely inside the backend's + * locked save, keyed off the `agentCommand: ""` sentinel that + * `resolveAgentCommandUpdate` produces at SUBMIT. Cancel-safety is therefore a + * UI-wiring invariant: toggling the inherit checkbox mutates only local dialog + * state, and the real Cancel button must route to `onOpenChange`, never to the + * submit path — so no `update_managed_agent` (and thus no column/env clear) is + * ever dispatched when the user backs out. + * + * Why a full production render rather than a hand-written miniature: the seam + * being pinned is the DIALOG FOOTER's wiring (Cancel → handleOpenChange, Save → + * handleSubmit → update_managed_agent). A miniature that re-implements a fake + * Cancel/Save cannot catch a regression that rewires the real Cancel button to + * handleSubmit. This test mounts the actual `AgentInstanceEditDialog`, expands + * Advanced, toggles the inherit checkbox, clicks the REAL Cancel button, and + * asserts the mocked `update_managed_agent` IPC boundary recorded zero calls — + * so rewiring Cancel to handleSubmit() makes it fail. The companion test clicks + * the REAL Save button and asserts the same boundary receives exactly one call + * carrying the `agentCommand: ""` inherit sentinel. + */ + +import assert from "node:assert/strict"; +import { after, afterEach, before, test } from "node:test"; + +import { JSDOM } from "jsdom"; + +const dom = new JSDOM("", { + url: "http://localhost", +}); + +// Track every QueryClient so afterEach can cancel pending queries + clear the +// cache — react-query's default gcTime otherwise schedules timers that outlive +// the test and stall the shared `pnpm test` process. +const clients = []; + +let act; +let cleanup; +let fireEvent; +let render; +let screen; +let createElement; +let QueryClient; +let QueryClientProvider; +let ThemeProvider; +let AgentInstanceEditDialog; + +// Records every Tauri command invocation the mounted dialog issues; unmocked +// commands reject so a new IPC dependency surfaces as a loud failure. +const ipcCalls = []; +const ipcHandlers = new Map(); + +const AGENT_PK = "d".repeat(64); + +// A goose-pinned instance linked to a claude persona — the pin→inherit +// transition. `agentCommandOverride` non-null means it opens PINNED (inherit +// checkbox unchecked); toggling inherit ON produces the `agentCommand: ""` +// clear sentinel at submit. Claude persona keeps the prospective runtime +// credential-free so the Save sanity case is enabled. +function rawAgent(overrides = {}) { + return { + pubkey: AGENT_PK, + name: "pinned-instance", + persona_id: "p1", + runtime: "goose", + relay_url: "wss://relay.example", + acp_command: "acp", + agent_command: "goose", + agent_command_override: "goose", + agent_args: [], + mcp_command: "mcp", + turn_timeout_seconds: 300, + idle_timeout_seconds: null, + max_turn_duration_seconds: null, + parallelism: 1, + system_prompt: null, + avatar_url: null, + model: null, + provider: null, + persona_out_of_date: false, + persona_orphaned: false, + needs_restart: false, + env_vars: {}, + status: "running", + pid: 1234, + created_at: "2026-01-01T00:00:00Z", + updated_at: "2026-01-01T00:00:00Z", + last_started_at: null, + last_stopped_at: null, + last_exit_code: null, + last_error: null, + last_error_code: null, + log_path: "/tmp/agent.log", + start_on_app_launch: false, + auto_restart_on_config_change: true, + backend: { type: "local" }, + backend_agent_id: null, + respond_to: "mentions", + respond_to_allowlist: [], + ...overrides, + }; +} + +function rawPersona(overrides = {}) { + return { + id: "p1", + display_name: "Scribe", + avatar_url: null, + system_prompt: "be helpful", + runtime: "claude", + model: null, + provider: null, + name_pool: [], + is_builtin: false, + is_active: true, + shared: false, + source_team: null, + env_vars: {}, + respond_to: null, + respond_to_allowlist: [], + parallelism: null, + created_at: "2026-01-01T00:00:00Z", + updated_at: "2026-01-01T00:00:00Z", + ...overrides, + }; +} + +function rawRuntime(id, overrides = {}) { + return { + id, + label: id, + avatar_url: "", + availability: "available", + command: id, + binary_path: `/usr/local/bin/${id}`, + default_args: [], + mcp_command: null, + install_hint: "", + install_instructions_url: "", + can_auto_install: false, + underlying_cli_path: null, + node_required: false, + auth_status: { status: "logged_in" }, + source: "builtin", + ...overrides, + }; +} + +function configSurface() { + return { + runtimeId: "goose", + runtimeLabel: "goose", + isPreSpawn: false, + normalized: { + model: null, + provider: null, + mode: null, + thinkingEffort: null, + maxOutputTokens: null, + contextLimit: null, + systemPrompt: null, + }, + advanced: [], + extensions: [], + sources: { + acpNative: "notApplicable", + acpConfigOptions: "notApplicable", + envVars: "available", + configFile: "notApplicable", + configFilePath: null, + mcpConfigFilePath: null, + }, + }; +} + +function installIpc() { + const set = (cmd, handler) => ipcHandlers.set(cmd, handler); + set("discover_acp_providers", () => + Promise.resolve([rawRuntime("claude"), rawRuntime("goose")]), + ); + set("list_personas", () => Promise.resolve([rawPersona()])); + set("get_agent_config_surface", () => Promise.resolve(configSurface())); + set("get_global_agent_config", () => + Promise.resolve({ + env_vars: {}, + provider: null, + model: null, + preferred_runtime: null, + }), + ); + set("get_baked_build_env", () => Promise.resolve([])); + set("get_baked_build_env_keys", () => Promise.resolve([])); + set("get_runtime_file_config", () => Promise.resolve(null)); + set("agent_access_owner_only", () => Promise.resolve(false)); + set("discover_agent_models", () => + Promise.resolve({ + agentName: "goose", + agentVersion: "1.0", + models: [], + agentDefaultModel: null, + selectedModel: null, + supportsSwitching: false, + }), + ); + // The persistence boundary under test. Returns a valid response so the + // mutation's onSuccess/onSettled cache updates don't throw. + set("update_managed_agent", (args) => { + ipcCalls.push({ cmd: "update_managed_agent", args }); + return Promise.resolve({ agent: rawAgent(), profile_sync_error: null }); + }); +} + +function renderDialog(onOpenChange) { + const client = new QueryClient({ + defaultOptions: { + mutations: { gcTime: 0 }, + queries: { gcTime: 0, retry: false }, + }, + }); + clients.push(client); + return render( + createElement( + ThemeProvider, + { defaultTheme: "buzz" }, + createElement( + QueryClientProvider, + { client }, + createElement(AgentInstanceEditDialog, { + agent: { ...toCamelAgent(rawAgent()) }, + open: true, + onOpenChange, + onUpdated: () => {}, + }), + ), + ), + ); +} + +// The dialog takes a camelCase ManagedAgent prop (the caller maps it via +// fromRawManagedAgent). Only the fields the dialog reads are needed. +function toCamelAgent(raw) { + return { + pubkey: raw.pubkey, + name: raw.name, + personaId: raw.persona_id, + runtime: raw.runtime, + relayUrl: raw.relay_url, + acpCommand: raw.acp_command, + agentCommand: raw.agent_command, + agentCommandOverride: raw.agent_command_override, + agentArgs: raw.agent_args, + mcpCommand: raw.mcp_command, + turnTimeoutSeconds: raw.turn_timeout_seconds, + idleTimeoutSeconds: raw.idle_timeout_seconds, + maxTurnDurationSeconds: raw.max_turn_duration_seconds, + parallelism: raw.parallelism, + systemPrompt: raw.system_prompt, + avatarUrl: raw.avatar_url, + model: raw.model, + modelSource: null, + provider: raw.provider, + personaOutOfDate: raw.persona_out_of_date, + personaOrphaned: raw.persona_orphaned, + needsRestart: raw.needs_restart, + restartDiff: [], + envVars: raw.env_vars, + status: raw.status, + pid: raw.pid, + createdAt: raw.created_at, + updatedAt: raw.updated_at, + lastStartedAt: raw.last_started_at, + lastStoppedAt: raw.last_stopped_at, + lastExitCode: raw.last_exit_code, + lastError: raw.last_error, + lastErrorCode: raw.last_error_code, + logPath: raw.log_path, + startOnAppLaunch: raw.start_on_app_launch, + autoRestartOnConfigChange: raw.auto_restart_on_config_change, + backend: raw.backend, + backendAgentId: raw.backend_agent_id, + respondTo: raw.respond_to, + respondToAllowlist: raw.respond_to_allowlist, + }; +} + +async function expandAdvancedAndToggleInherit() { + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: /Advanced/ })); + }); + const checkbox = dom.window.document.getElementById( + "edit-agent-inherit-harness", + ); + assert.ok( + checkbox, + "inherit checkbox must render for a persona-linked agent inside Advanced", + ); + assert.equal( + checkbox.checked, + false, + "a harness-pinned agent must open with inherit unchecked", + ); + await act(async () => { + fireEvent.click(checkbox); + }); + assert.equal(checkbox.checked, true, "inherit toggle must flip to checked"); +} + +before(async () => { + Object.assign(globalThis, { + document: dom.window.document, + window: dom.window, + IS_REACT_ACT_ENVIRONMENT: true, + }); + // Radix + testing-library reach for a broad set of DOM constructors and + // globals off the realm's `globalThis`. Node ships its own incompatible + // `Event`/`CustomEvent` globals, so JSDOM nodes reject events built from + // them ("parameter 1 is not of type 'Event'"). Force every DOM constructor + // and *Event/*Element/Node* binding to JSDOM's, overriding Node's built-ins, + // so the mounted dialog resolves them all against one realm. + for (const key of Object.getOwnPropertyNames(dom.window)) { + if (key === "window" || key === "document" || key === "globalThis") + continue; + const value = dom.window[key]; + if ( + typeof value === "function" && + /^(HTML|SVG)|Element$|Event$|EventTarget$|^Node|^Document|Observer$/.test( + key, + ) + ) { + globalThis[key] = value; + } + } + globalThis.getComputedStyle = dom.window.getComputedStyle.bind(dom.window); + Object.defineProperty(globalThis, "navigator", { + configurable: true, + value: dom.window.navigator, + writable: true, + }); + dom.window.matchMedia = () => ({ + matches: true, + addEventListener() {}, + removeEventListener() {}, + }); + // Radix Dialog probes pointer-capture and scrolls focus into view on mount. + dom.window.HTMLElement.prototype.hasPointerCapture = () => false; + dom.window.HTMLElement.prototype.releasePointerCapture = () => {}; + dom.window.HTMLElement.prototype.scrollIntoView = () => {}; + globalThis.ResizeObserver = class { + observe() {} + unobserve() {} + disconnect() {} + }; + dom.window.__TAURI_INTERNALS__ = { + invoke: (cmd, args) => { + const handler = ipcHandlers.get(cmd); + if (handler) return handler(args); + return Promise.reject(new Error(`unmocked Tauri command: ${cmd}`)); + }, + transformCallback: () => Math.random(), + }; + + ({ act, cleanup, fireEvent, render, screen } = await import( + "@testing-library/react" + )); + ({ createElement } = await import("react")); + ({ QueryClient, QueryClientProvider } = await import( + "@tanstack/react-query" + )); + ({ ThemeProvider } = await import("@/shared/theme/ThemeProvider")); + ({ AgentInstanceEditDialog } = await import("./AgentInstanceEditDialog.tsx")); +}); + +afterEach(() => { + cleanup?.(); + for (const client of clients.splice(0)) { + client.cancelQueries(); + client.clear(); + } + ipcHandlers.clear(); + ipcCalls.length = 0; +}); + +after(() => dom.window.close()); + +test("inherit toggle then Cancel dispatches no update_managed_agent", async () => { + installIpc(); + let openChange; + await act(async () => { + renderDialog((next) => { + openChange = next; + }); + }); + + await expandAdvancedAndToggleInherit(); + + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: "Cancel" })); + }); + + assert.equal( + openChange, + false, + "Cancel must route through onOpenChange(false)", + ); + assert.equal( + ipcCalls.filter((c) => c.cmd === "update_managed_agent").length, + 0, + "Cancel after toggling inherit must not dispatch update_managed_agent — rewiring Cancel to handleSubmit() breaks this", + ); +}); + +test("inherit toggle then Save dispatches the agentCommand:'' inherit sentinel", async () => { + installIpc(); + await act(async () => { + renderDialog(() => {}); + }); + + await expandAdvancedAndToggleInherit(); + + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: "Save changes" })); + }); + + const updates = ipcCalls.filter((c) => c.cmd === "update_managed_agent"); + assert.equal(updates.length, 1, "Save must dispatch exactly one update"); + assert.equal( + updates[0].args.input.agentCommand, + "", + "Save on the pin→inherit transition must carry the empty-command sentinel the backend clears the column on", + ); +}); diff --git a/desktop/src/features/agents/ui/agentInstanceEditPinning.test.mjs b/desktop/src/features/agents/ui/agentInstanceEditPinning.test.mjs index 81862d7c41b..a6d4330b094 100644 --- a/desktop/src/features/agents/ui/agentInstanceEditPinning.test.mjs +++ b/desktop/src/features/agents/ui/agentInstanceEditPinning.test.mjs @@ -282,3 +282,53 @@ test("editValidity_allowlistWithEmptyList_blocksSave", () => { "allowlist with at least one pubkey must allow Save", ); }); + +// ── Cancel-safety: inherit toggle → Cancel emits no update_managed_agent ────── +// +// Acceptance pin (plan item 1). The pin→inherit effort/runtime clear is derived +// entirely inside the backend's locked save, keyed off the agentCommand:"" +// sentinel `resolveAgentCommandUpdate` produces at SUBMIT. Cancel-safety is +// therefore a UI-wiring invariant: flipping the inherit toggle mutates only +// local dialog state, and the Cancel button routes to onOpenChange, never to +// the submit path — so no update_managed_agent call (and thus no column/env +// clear) is ever dispatched when the user backs out. +// +// This mirrors the AgentInstanceEditDialog footer exactly: Cancel → +// onOpenChange(false); Save → handleSubmit → updateMutation.mutateAsync(input), +// where `input.agentCommand` is the resolveAgentCommandUpdate sentinel. +test("inheritToggle_cancelled_emitsNoUpdate", () => { + const calls = []; + // The ONLY producer of the persistence-boundary sentinel is the submit path. + function handleSubmit() { + const agentCommandUpdate = resolveAgentCommandUpdate({ + inheritHarness: true, // user just toggled inherit ON + agentCommand: pinnedAgent.agentCommand, + originalAgentCommand: pinnedAgent.agentCommand, + agentCommandOverride: pinnedAgent.agentCommandOverride ?? null, + }); + calls.push({ agentCommand: agentCommandUpdate }); + } + function onOpenChange() { + /* dialog close — no mutation */ + } + + // User toggles inherit (local state only), then clicks Cancel. + const cancelButton = { onClick: () => onOpenChange(false) }; + cancelButton.onClick(); + + assert.equal( + calls.length, + 0, + "Cancel after toggling inherit must not dispatch update_managed_agent", + ); + + // Sanity: the submit path WOULD have emitted the inherit sentinel, proving + // the clear is gated on Save alone — Cancel simply never reaches it. + handleSubmit(); + assert.equal(calls.length, 1); + assert.equal( + calls[0].agentCommand, + "", + "Save on the pin→inherit transition emits the empty-command sentinel the backend clears the column on", + ); +}); diff --git a/desktop/src/features/agents/ui/runtimeModelProviderSelection.test.mjs b/desktop/src/features/agents/ui/runtimeModelProviderSelection.test.mjs index 470b2312bc7..1b1f0a6a7d9 100644 --- a/desktop/src/features/agents/ui/runtimeModelProviderSelection.test.mjs +++ b/desktop/src/features/agents/ui/runtimeModelProviderSelection.test.mjs @@ -17,6 +17,43 @@ const base = { // --- selectionOnRuntimeChange --- +test("runtime switch clears stale effort env aliases (native + ACP sentinel), preserving the direct-write column", () => { + // Claude/buzz-agent → Goose: the previous runtime's effort env aliases are + // stale under Goose. They are cleared; unrelated env survives. The canonical + // effort column is direct-write, not in this state, so it is untouched here. + const next = selectionOnRuntimeChange( + { + ...base, + envVars: { + BUZZ_ACP_EFFORT_LEVEL: "high", + BUZZ_AGENT_THINKING_EFFORT: "medium", + GOOSE_THINKING_EFFORT: "max", + KEEP: "x", + }, + }, + { + previousRuntime: "buzz-agent", + nextRuntime: "goose", + nextRuntimeCanChooseProvider: true, + lockedRuntimeReset: "full", + }, + ); + assert.deepEqual(next.envVars, { KEEP: "x" }); +}); + +test("no-op runtime change (previous === next) leaves effort env aliases intact", () => { + const next = selectionOnRuntimeChange( + { ...base, envVars: { GOOSE_THINKING_EFFORT: "high", KEEP: "x" } }, + { + previousRuntime: "goose", + nextRuntime: "goose", + nextRuntimeCanChooseProvider: true, + lockedRuntimeReset: "full", + }, + ); + assert.deepEqual(next.envVars, { GOOSE_THINKING_EFFORT: "high", KEEP: "x" }); +}); + test("runtime change to a provider-locked runtime, full reset (Persona/Edit): clears provider, custom flags, and managed API key", () => { const next = selectionOnRuntimeChange( { diff --git a/desktop/src/features/agents/ui/runtimeModelProviderSelection.ts b/desktop/src/features/agents/ui/runtimeModelProviderSelection.ts index e98dc540dff..33d921d00e3 100644 --- a/desktop/src/features/agents/ui/runtimeModelProviderSelection.ts +++ b/desktop/src/features/agents/ui/runtimeModelProviderSelection.ts @@ -20,6 +20,24 @@ import { * dialog-specific side effects (inherit pins, command sync, catalog memory) * at the call site. Divergent behaviors are parameterized, never merged. */ + +/** + * Every runtime-owned thinking-effort env key: the native keys of all known + * runtimes plus the retained ACP-startup transport sentinel. Mirrors the Rust + * `effort_suppress_keys()` full sweep (`config_bridge/effort.rs`). + * + * On a runtime switch these aliases become stale — they express the *previous* + * runtime's vocabulary — so they are cleared. The canonical persisted effort + * (`record.effort_level`) is a direct-write column owned by `EffortPickerField` + * (AGENTS.md rule 14), lives outside this env-state selection, and is therefore + * PRESERVED across the switch: the launch projection normalizes it (or skips it + * as absent) for the destination runtime, and switching back restores it. + */ +const EFFORT_ENV_ALIASES = [ + "GOOSE_THINKING_EFFORT", + "BUZZ_AGENT_THINKING_EFFORT", + "BUZZ_ACP_EFFORT_LEVEL", +] as const; export type RuntimeModelProviderSelection = { provider: string; model: string; @@ -45,6 +63,19 @@ export function selectionOnRuntimeChange( ): RuntimeModelProviderSelection { const next = { ...current }; + // F3 nondestructive switch policy: clear the previous runtime's stale + // thinking-effort env aliases (all native keys + the ACP sentinel). The + // canonical `record.effort_level` column is direct-write and not part of this + // selection state, so it is preserved — the launch projection re-expresses it + // for the destination runtime, and switching back restores the preference. + if (params.previousRuntime !== params.nextRuntime) { + let envVars = next.envVars; + for (const key of EFFORT_ENV_ALIASES) { + envVars = envVarsWithoutKey(envVars, key); + } + next.envVars = envVars; + } + if ( shouldClearModelForRuntimeChange( params.previousRuntime, diff --git a/desktop/src/shared/api/tauriManagedAgents.ts b/desktop/src/shared/api/tauriManagedAgents.ts index 9f77566da99..9c6a6b29c80 100644 --- a/desktop/src/shared/api/tauriManagedAgents.ts +++ b/desktop/src/shared/api/tauriManagedAgents.ts @@ -64,9 +64,12 @@ export async function setManagedAgentAutoRestart( } /** - * B5: persist the canonical startup effort for a local managed agent. Applied - * as `BUZZ_ACP_EFFORT_LEVEL` at the next spawn. Pass `null` to clear (reverts - * to the adapter default). Rejects non-local agents. + * Persist the canonical startup effort for a local managed agent. Stored as the + * harness-agnostic `effort_level` column and projected to each runtime's native + * key at the next spawn (`GOOSE_THINKING_EFFORT` for Goose, + * `BUZZ_AGENT_THINKING_EFFORT` for buzz-agent, the `BUZZ_ACP_EFFORT_LEVEL` + * startup sentinel for Claude/Codex and keyless adapters). Pass `null` to clear + * (reverts to the inherited/adapter default). Rejects non-local agents. */ export async function persistAgentEffortLevel( pubkey: string, diff --git a/desktop/src/shared/api/types.ts b/desktop/src/shared/api/types.ts index d1e624ad530..cb7961f74ac 100644 --- a/desktop/src/shared/api/types.ts +++ b/desktop/src/shared/api/types.ts @@ -667,9 +667,9 @@ export type RuntimeConfigSurface = { sources: ConfigSourceReport; /** #3493: `true` when the surface was read from a user-set `CLAUDE_CONFIG_DIR` — drives the Keychain caveat note in the panel. */ claudeConfigDirCustom?: boolean; - /** B5: the adapter-advertised `thought_level` configId, discovered from the running session. Present only for claude after the first session. Drives the effort picker. */ + /** The adapter-advertised `thought_level` configId, discovered from the running session — present once a session advertises `thought_level` support (Claude today; any effort-capable ACP adapter in general). Drives the effort picker. */ effortConfigId?: string; - /** B5/I-7: adapter-advertised option values for the `thought_level` option — the picker renders these instead of hardcoded values. */ + /** Adapter-advertised option values for the `thought_level` option — the picker renders these instead of hardcoded values. */ effortOptions?: AcpConfigOptionValue[]; };