diff --git a/desktop/src-tauri/src/commands/agent_config.rs b/desktop/src-tauri/src/commands/agent_config.rs index 4df24e6e9ba..16b4c93b3e6 100644 --- a/desktop/src-tauri/src/commands/agent_config.rs +++ b/desktop/src-tauri/src/commands/agent_config.rs @@ -14,9 +14,8 @@ use crate::{ }, current_instance_id, is_reserved_env_key, is_safe_to_reveal, is_well_formed_env_key, known_acp_runtime, load_managed_agents, load_personas, resolve_effective_agent_env, - save_managed_agents, sync_managed_agent_processes, AgentDefinition, BackendKind, - GlobalAgentConfig, KnownAcpRuntime, ManagedAgentRecord, ManagedAgentRuntimeKey, - MAX_ENV_VALUE_BYTES, + save_managed_agents, sync_managed_agent_processes, AgentDefinition, GlobalAgentConfig, + KnownAcpRuntime, ManagedAgentRecord, ManagedAgentRuntimeKey, MAX_ENV_VALUE_BYTES, }, }; @@ -535,42 +534,15 @@ fn parse_models(raw: Option<&serde_json::Value>) -> (Vec, Option< (models, current_model) } -/// 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). -/// -/// Rejects non-local backends: remote agents receive effort through `policy_env` -/// 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] -pub fn persist_agent_effort_level( - pubkey: String, +/// Atomically set the record's canonical effort column and strip every stale +/// record-scope effort env alias. Split from the Tauri command so the invariant +/// — no leftover alias can outrank the just-set column — is directly testable. +pub(crate) fn apply_picker_effort_level( + record: &mut ManagedAgentRecord, effort_level: Option, - app: AppHandle, - state: State<'_, AppState>, -) -> Result<(), String> { - let _store_guard = state - .managed_agents_store_lock - .lock() - .map_err(|e| e.to_string())?; - let mut records = load_managed_agents(&app)?; - let record = records - .iter_mut() - .find(|r| r.pubkey == pubkey) - .ok_or_else(|| format!("agent {pubkey} not found"))?; - if record.backend != BackendKind::Local { - return Err(format!( - "agent {pubkey} is not a local agent; remote effort is set at deploy time" - )); - } +) { record.effort_level = effort_level; - record.updated_at = crate::util::now_iso(); - save_managed_agents(&app, &records) + crate::managed_agents::remove_record_effort_aliases(&mut record.env_vars); } #[cfg(test)] diff --git a/desktop/src-tauri/src/commands/agent_config_tests.rs b/desktop/src-tauri/src/commands/agent_config_tests.rs index 596d9d36292..093e925f18a 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,16 @@ 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), + effort_accepted_values: None, 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 { @@ -630,6 +633,58 @@ fn baked_env_mixed_keys_correct_masking() { assert!(token.masked); } +/// F1 picker direct-write invariant: a stale record-native `GOOSE_THINKING_EFFORT` +/// (launch-projection tier 1, ABOVE the canonical column) must not survive a +/// picker write. Setting effort `high` through the picker path both writes the +/// column and sweeps the stale alias, so the reader and the launch projection +/// both resolve `high` — not the stale `low`. Deleting the sweep in +/// `apply_picker_effort_level` re-breaks this: the projection would emit `low`. +#[test] +fn picker_write_sweeps_stale_record_native_effort_alias() { + let mut record = agent_record(); + record + .env_vars + .insert("GOOSE_THINKING_EFFORT".to_string(), "low".to_string()); + + super::apply_picker_effort_level(&mut record, Some("high".to_string())); + + // The stale record-native alias is gone; only the column carries the value. + assert!( + !record.env_vars.contains_key("GOOSE_THINKING_EFFORT"), + "stale record-native effort alias must be swept by the picker write" + ); + assert_eq!(record.effort_level.as_deref(), Some("high")); + + // Reader: the panel resolves the just-set value, not the stale alias. + let surface = with_no_goose_config(|| { + resolve_config_surface( + record.clone(), + &[], + Some(goose_runtime()), + None, + &Default::default(), + None, + ) + }); + let effort = surface + .normalized + .thinking_effort + .expect("picker-set effort must resolve"); + assert_eq!(effort.value.as_deref(), Some("high")); + + // Launch projection: the spawned child receives the picker value. + let launch = crate::managed_agents::config_bridge::effort::effort_launch_projection( + &record, + Some(goose_runtime()), + &[], + None, + &std::collections::BTreeMap::new(), + None, + &std::collections::BTreeMap::new(), + ); + assert_eq!(launch.value.as_deref(), Some("high")); +} + #[test] fn baked_env_thinking_effort_is_unmasked() { // BUZZ_AGENT_THINKING_EFFORT is a non-secret enum — must not be masked. diff --git a/desktop/src-tauri/src/commands/agent_discovery.rs b/desktop/src-tauri/src/commands/agent_discovery.rs index 95e9759f10e..ccca7c4abfa 100644 --- a/desktop/src-tauri/src/commands/agent_discovery.rs +++ b/desktop/src-tauri/src/commands/agent_discovery.rs @@ -17,7 +17,6 @@ fn active_installs() -> &'static std::sync::Mutex>, +) -> Result<(), String> { + if effort_level.is_some() && record.backend != crate::managed_agents::BackendKind::Local { + return Err(format!( + "agent {} is not a local agent; remote effort is set at deploy time", + record.pubkey + )); + } + Ok(()) +} + +/// Guard/apply seam for the effort step inside `apply_record_field_updates`. +fn apply_effort_update( + record: &mut ManagedAgentRecord, + effort_level: Option>, +) -> Result<(), String> { + ensure_effort_change_supported(record, &effort_level)?; + if let Some(effort_override) = effort_level { + crate::commands::agent_config::apply_picker_effort_level(record, effort_override); + } + Ok(()) +} + +/// Proof token returned by `apply_record_field_updates`. Zero-size and +/// `#[must_use]`; consumed by `stamp_record_updated_at`, so removing the +/// `apply_record_field_updates` call from `update_managed_agent` leaves +/// `applied` undefined at the timestamp site — a compile error. +#[derive(Debug)] +#[must_use] +pub(crate) struct RecordFieldsApplied(()); + +/// Apply the env-vars and effort steps of `update_managed_agent` to a record +/// in the correct order: env_vars FIRST (so the same-request map cannot +/// reintroduce a stale alias), then the canonical effort column write. +/// +/// Returns a `RecordFieldsApplied` token that must be passed to +/// `stamp_record_updated_at`. Removing this call from `update_managed_agent` +/// leaves `applied` undefined at the timestamp site — a compile error. +/// +/// Called by `update_managed_agent` inside its locked transaction and by tests. +/// Any step deleted from inside this function is directly caught by the +/// corresponding test assertion. +/// +/// Mutation proofs (see `agent_models_update_tests.rs`): +/// - Deleting the `apply_effort_update` call leaves `effort_level` unchanged. +/// - Deleting `ensure_effort_change_supported` inside `apply_effort_update` +/// lets non-local writes pass `Ok(())` without mutating the column. +/// - Deleting `apply_picker_effort_level` inside `apply_effort_update` +/// leaves `effort_level == None` on a local-set request. +pub(crate) fn apply_record_field_updates( + record: &mut ManagedAgentRecord, + env_vars: Option<&std::collections::BTreeMap>, + inherit_transition: bool, + effort_level: Option>, +) -> Result { + // Order is load-bearing: env_vars before effort so a same-request + // env_vars map cannot reintroduce a stale alias after the column write. + crate::managed_agents::apply_env_vars_then_effort_transition( + record, + env_vars.cloned(), + inherit_transition, + ); + apply_effort_update(record, effort_level)?; + Ok(RecordFieldsApplied(())) +} + +/// Stamp `record.updated_at` with the current ISO timestamp, consuming the +/// `RecordFieldsApplied` proof token. Removing `apply_record_field_updates` +/// from `update_managed_agent` leaves `applied` undefined here — a compile error. +pub(crate) fn stamp_record_updated_at( + record: &mut ManagedAgentRecord, + _applied: RecordFieldsApplied, +) { + record.updated_at = crate::util::now_iso(); +} + /// Flush a retained managed-agent policy, preserving any earlier profile error. pub(crate) async fn flush_managed_agent_policy( app: &AppHandle, @@ -115,15 +197,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,9 +220,16 @@ 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)?; } // Native provider/model fields are authoritative. Keep the typed marker @@ -211,7 +302,23 @@ pub async fn update_managed_agent( record.respond_to_allowlist = prospective_allowlist; } - record.updated_at = now_iso(); + // Effort + env_vars: applied together inside `apply_record_field_updates` to + // enforce the ordering invariant (env_vars before effort column write) and + // provide a directly-testable production seam. Effort persists inside the + // locked transaction so an access-policy restart above snapshots and + // launches the new effort value. Present+Some(v)=set; Present+None=clear; + // Absent=don't touch (the dialog sends it only when effortTouched). + // The returned token is consumed by `stamp_record_updated_at`; removing + // this call from `update_managed_agent` leaves `applied` undefined there + // — a compile error (the sole outer-seam proof for this call site). + let applied = apply_record_field_updates( + record, + input.env_vars.as_ref(), + inherit_transition, + input.effort_level, + )?; + + stamp_record_updated_at(record, applied); save_managed_agents(&app, &records)?; @@ -365,5 +472,6 @@ pub async fn update_managed_agent( } #[cfg(test)] +#[allow(unused_must_use)] #[path = "agent_models_update_tests.rs"] mod tests; diff --git a/desktop/src-tauri/src/commands/agent_models_update_tests.rs b/desktop/src-tauri/src/commands/agent_models_update_tests.rs index b9fd0bd1839..28a50e7b15b 100644 --- a/desktop/src-tauri/src/commands/agent_models_update_tests.rs +++ b/desktop/src-tauri/src/commands/agent_models_update_tests.rs @@ -1,4 +1,9 @@ use super::*; +// The tests call `apply_record_field_updates(...)` and consume the return value +// via `.expect(...)`, discarding `RecordFieldsApplied`. The tests verify column +// writes (side effects), not the token itself. The lint is suppressed here so +// callers remain readable. Production code (update_managed_agent) must never +// suppress it — the token IS the outer-seam compile-time proof. fn provider_record(deployed: bool) -> ManagedAgentRecord { let mut record: ManagedAgentRecord = serde_json::from_value(serde_json::json!({ @@ -29,3 +34,340 @@ fn undeployed_provider_accepts_access_edits() { ensure_access_policy_change_supported(&provider_record(false), true) .expect("no running provider deployment can retain stale access"); } + +fn local_record() -> ManagedAgentRecord { + serde_json::from_value(serde_json::json!({ + "pubkey": "local", "name": "Local Agent", "relay_url": "", "acp_command": "", + "agent_command": "", "agent_args": [], "mcp_command": "", + "turn_timeout_seconds": 0, "system_prompt": null, "created_at": "", + "updated_at": "", "last_started_at": null, "last_stopped_at": null, + "last_exit_code": null, "last_error": null + })) + .unwrap() + // BackendKind deserializes as Local when the field is absent (the json! above). +} + +// ── Production-entered seam tests (apply_record_field_updates) ────────────── +// +// These tests call `apply_record_field_updates`, the same function production +// calls inside `update_managed_agent` for the env_vars+effort ordered write. +// They verify: +// - non-local records are rejected AND the column is NOT mutated; +// - local set writes to the column and sweeps stale env aliases; +// - local clear zeroes the column and sweeps stale env aliases; +// - env_vars applied before effort so no same-request alias re-pins the column. +// +// Deletion proof for the effort guard: removing `ensure_effort_change_supported` +// inside `apply_record_field_updates` makes reject tests return `Ok(())` instead +// of `Err`, and the "record not mutated" assertions fail. +// +// Deletion proof for the apply call: removing the `apply_effort_update` call +// inside `apply_record_field_updates` leaves `effort_level == None` on local-set. +// +// Deletion proof for the env_vars step: removing `apply_env_vars_then_effort_transition` +// inside `apply_record_field_updates` leaves the env alias in `env_vars` on local-set. +// +// Ordering proof: `env_vars` with a stale alias is applied BEFORE effort so the +// alias is stripped; reversing the order leaves both the alias and the new column. +// +// Outer-seam proof (compile-error): removing `apply_record_field_updates` from +// `update_managed_agent` leaves `applied` undefined at `stamp_record_updated_at` +// — a compile error enforced by the `#[must_use] RecordFieldsApplied` token. +// `record_field_updates_persist_effort_to_disk` below proves the +// disk-persistence contract of `apply_record_field_updates` itself (calls it +// directly); it does not independently gate the production invocation. + +#[test] +fn non_local_set_is_rejected_and_record_not_mutated() { + let mut record = provider_record(false); + let err = apply_record_field_updates(&mut record, None, false, Some(Some("high".to_string()))) + .expect_err("non-local record must reject effort writes"); + assert!( + err.contains("remote effort is set at deploy time"), + "error must explain why non-local effort writes are rejected: {err}" + ); + // Column must not be touched — the rejection is before mutation. + assert_eq!( + record.effort_level, None, + "non-local record column must be unchanged after a rejected set" + ); +} + +#[test] +fn non_local_clear_is_rejected_and_record_not_mutated() { + // Clear (None inner value) is also rejected for non-local records — the + // outer Some signals presence; the inner None is the clear sentinel. + let mut record = provider_record(false); + let err = apply_record_field_updates(&mut record, None, false, Some(None)) + .expect_err("non-local record effort clear must also be rejected"); + assert!(err.contains("remote effort is set at deploy time")); + assert_eq!( + record.effort_level, None, + "non-local record column must be unchanged after a rejected clear" + ); +} + +#[test] +fn local_set_writes_column_and_sweeps_stale_alias() { + // `apply_record_field_updates` must write `effort_level` for a local record + // and strip any stale record-scope effort alias. Deleting the + // `apply_effort_update` call inside leaves `effort_level == None`. + let mut record = local_record(); + record + .env_vars + .insert("GOOSE_THINKING_EFFORT".to_string(), "low".to_string()); + + let _ = apply_record_field_updates(&mut record, None, false, Some(Some("high".to_string()))) + .expect("local record must accept effort set"); + + assert_eq!( + record.effort_level.as_deref(), + Some("high"), + "local set must write the canonical column" + ); + assert!( + !record.env_vars.contains_key("GOOSE_THINKING_EFFORT"), + "local set must sweep the stale record-native alias" + ); +} + +#[test] +fn local_clear_zeroes_column_and_sweeps_alias() { + let mut record = local_record(); + record.effort_level = Some("high".to_string()); + record + .env_vars + .insert("GOOSE_THINKING_EFFORT".to_string(), "high".to_string()); + + apply_record_field_updates(&mut record, None, false, Some(None)) + .expect("local record must accept effort clear"); + + assert_eq!( + record.effort_level, None, + "local clear must zero the canonical column" + ); + assert!( + !record.env_vars.contains_key("GOOSE_THINKING_EFFORT"), + "local clear must sweep the stale record-native alias" + ); +} + +#[test] +fn absent_effort_is_noop_for_any_backend() { + // A missing effortLevel field (the common case) must never be rejected and + // must never touch the column — this is the don't-touch path. + let mut local = local_record(); + apply_record_field_updates(&mut local, None, false, None) + .expect("absent effort must pass for local"); + assert_eq!( + local.effort_level, None, + "absent effort must not touch local column" + ); + + let mut provider = provider_record(true); + apply_record_field_updates(&mut provider, None, false, None) + .expect("absent effort must pass for provider"); + assert_eq!( + provider.effort_level, None, + "absent effort must not touch provider column" + ); +} + +#[test] +fn env_vars_applied_before_effort_ordering_invariant() { + // Order is load-bearing: env_vars BEFORE effort column write. A same-request + // env_vars map containing a stale alias (GOOSE_THINKING_EFFORT=low) alongside + // an explicit effort set (high) must end with the alias swept — not re-pinned. + // If env_vars were applied AFTER effort, the alias would survive. + let mut record = local_record(); + let mut env_vars = std::collections::BTreeMap::new(); + env_vars.insert("GOOSE_THINKING_EFFORT".to_string(), "low".to_string()); + + apply_record_field_updates( + &mut record, + Some(&env_vars), + false, + Some(Some("high".to_string())), + ) + .expect("ordering test must succeed for local record"); + + assert_eq!( + record.effort_level.as_deref(), + Some("high"), + "effort column must be set to the explicit value" + ); + assert!( + !record.env_vars.contains_key("GOOSE_THINKING_EFFORT"), + "alias in the same-request env_vars must be swept before the column is read at launch" + ); +} + +// ── Defensive direct-IPC contract ───────────────────────────────────────────── +// +// Non-blocking defensive coverage (Wes/Carl review): a contradictory request +// combining the ACP inherit sentinel in `env_vars` and a non-null effort_level +// must be deterministic — the effort write wins over the sentinel, and the +// sentinel is swept by the alias-removal step so it cannot shadow the column +// at launch time. The shipped renderer suppresses this combination, but the +// backend must not leave an ambiguous state. + +#[test] +fn effort_write_sweeps_acp_sentinel_in_env_vars() { + // A local record whose env_vars contain BUZZ_ACP_EFFORT_LEVEL (e.g. manually + // set by a user) plus a concurrent explicit effort_level write. The column + // must be set to the explicit value AND the sentinel must be removed. + let mut record = local_record(); + record.env_vars.insert( + "BUZZ_ACP_EFFORT_LEVEL".to_string(), + "old-sentinel".to_string(), + ); + apply_record_field_updates(&mut record, None, false, Some(Some("high".to_string()))) + .expect("local record must accept effort set"); + assert_eq!( + record.effort_level.as_deref(), + Some("high"), + "effort write must set the column" + ); + assert!( + !record.env_vars.contains_key("BUZZ_ACP_EFFORT_LEVEL"), + "ACP sentinel in env_vars must be swept by the alias-removal step" + ); +} + +#[test] +fn effort_clear_sweeps_acp_sentinel_in_env_vars() { + // A concurrent clear (None inner value) plus a pre-existing ACP sentinel. + // After the clear the column is None and the sentinel is gone — no ambiguity. + let mut record = local_record(); + record.effort_level = Some("high".to_string()); + record.env_vars.insert( + "BUZZ_ACP_EFFORT_LEVEL".to_string(), + "old-sentinel".to_string(), + ); + apply_record_field_updates(&mut record, None, false, Some(None)) + .expect("local record must accept effort clear"); + assert_eq!( + record.effort_level, None, + "effort clear must zero the column" + ); + assert!( + !record.env_vars.contains_key("BUZZ_ACP_EFFORT_LEVEL"), + "ACP sentinel in env_vars must be swept on clear" + ); +} + +// ── Helper disk-persistence contract ───────────────────────────────────────── +// +// This test drives the production helper sequence directly in its own body: +// load_managed_agents → apply_record_field_updates → stamp_record_updated_at +// → save_managed_agents → load-from-disk. +// +// Mutation proofs (scoped to this test body): +// - Removing `apply_record_field_updates` from this test body leaves +// `applied` undefined at `stamp_record_updated_at` — a compile error. +// - Removing the function call and stubbing the token manually leaves +// `effort_level` unchanged on disk — assertion fails (expected +// Some("high"), got None). +// +// Outer-seam gate: the compile error that prevents skipping +// `apply_record_field_updates` inside `update_managed_agent` is described in +// the outer-seam comment above (undefined `applied` token at the +// `stamp_record_updated_at` site). This test proves only the helper's own +// disk-roundtrip contract; it does not independently gate the production +// invocation. + +#[cfg(not(target_os = "windows"))] +#[test] +fn record_field_updates_persist_effort_to_disk() { + use crate::app_state::build_app_state; + use crate::managed_agents::{load_managed_agents, save_managed_agents}; + + // A single crate-wide process-env lock covers PATH, HOME, XDG_DATA_HOME, + // and all effort env keys — `lock_path_mutex` and `lock_env_mutex` both + // delegate to the same `PROCESS_ENV_MUTEX` static. + let _env_guard = crate::managed_agents::lock_path_mutex(); + let temp = tempfile::tempdir().unwrap(); + let home = temp.path().join("home"); + std::fs::create_dir_all(&home).unwrap(); + + // RAII guards restore HOME and XDG_DATA_HOME on Drop (even on panic). + // Uses OsString so a pre-existing non-Unicode value is restored exactly. + struct EnvVarGuard { + key: String, + prior: Option, + } + impl EnvVarGuard { + fn set(key: &str, value: &std::path::Path) -> Self { + let prior = std::env::var_os(key); + #[allow(deprecated)] + // SAFETY: caller holds the crate-wide process-env lock. + unsafe { + std::env::set_var(key, value) + }; + Self { + key: key.to_string(), + prior, + } + } + } + impl Drop for EnvVarGuard { + fn drop(&mut self) { + #[allow(deprecated)] + // SAFETY: caller holds the crate-wide process-env lock. + unsafe { + match &self.prior { + Some(v) => std::env::set_var(&self.key, v), + None => std::env::remove_var(&self.key), + } + } + } + } + + let _home_guard = EnvVarGuard::set("HOME", &home); + let _xdg_guard = EnvVarGuard::set("XDG_DATA_HOME", &home); + + let app = tauri::test::mock_builder() + .manage(build_app_state()) + .build(tauri::test::mock_context(tauri::test::noop_assets())) + .expect("mock app builds headless"); + + // Seed a local record with no effort set. + let seed: crate::managed_agents::ManagedAgentRecord = + serde_json::from_value(serde_json::json!({ + "pubkey": "test-effort-agent", + "name": "Effort Test Agent", + "relay_url": "", "acp_command": "", "agent_command": "", + "agent_args": [], "mcp_command": "", "turn_timeout_seconds": 0, + "system_prompt": null, "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 + })) + .unwrap(); + save_managed_agents(app.handle(), &[seed]).unwrap(); + + // Drive the production seam: load → apply_record_field_updates → + // stamp_record_updated_at → save. This is the exact sequence that + // `update_managed_agent` executes inside its locked transaction. + let mut records = load_managed_agents(app.handle()).unwrap(); + let record = records + .iter_mut() + .find(|r| r.pubkey == "test-effort-agent") + .expect("seeded record must load"); + let applied = apply_record_field_updates(record, None, false, Some(Some("high".to_string()))) + .expect("local record must accept effort set"); + stamp_record_updated_at(record, applied); + save_managed_agents(app.handle(), &records).unwrap(); + + // Verify effort landed on disk. + let saved = load_managed_agents(app.handle()).unwrap(); + let saved_record = saved + .iter() + .find(|r| r.pubkey == "test-effort-agent") + .expect("agent must persist after update"); + assert_eq!( + saved_record.effort_level.as_deref(), + Some("high"), + "apply_record_field_updates + stamp_record_updated_at must write effort_level to disk" + ); + // _home_guard and _xdg_guard restore HOME and XDG_DATA_HOME via Drop. +} diff --git a/desktop/src-tauri/src/commands/agents_deploy.rs b/desktop/src-tauri/src/commands/agents_deploy.rs index 359a35abcf6..de8ca8cc789 100644 --- a/desktop/src-tauri/src/commands/agents_deploy.rs +++ b/desktop/src-tauri/src/commands/agents_deploy.rs @@ -98,13 +98,13 @@ fn build_launch_block_for_policy( }; 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()); } @@ -121,14 +121,6 @@ fn build_launch_block_for_policy( 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 @@ -138,10 +130,13 @@ fn build_launch_block_for_policy( // 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| { k.eq_ignore_ascii_case(crate::managed_agents::ACP_SESSION_POLICY_ENV_VAR) - || (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"))) @@ -522,19 +517,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" ); } @@ -561,26 +564,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/lib.rs b/desktop/src-tauri/src/lib.rs index fe2bba5024b..0e4832a702a 100644 --- a/desktop/src-tauri/src/lib.rs +++ b/desktop/src-tauri/src/lib.rs @@ -715,7 +715,6 @@ pub fn run() { get_baked_build_env_keys, get_baked_build_env, put_agent_session_config, - persist_agent_effort_level, get_global_agent_config, set_global_agent_config, mesh_start_node, 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..e06fe06216d --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/config_bridge/effort.rs @@ -0,0 +1,506 @@ +//! 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::{EffortNormalization, 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>, + /// When no tier resolved a `value`, preserve a value the launch env already + /// carries under `key` (collapsing every case variant to the canonical + /// spelling). Set only for unknown/custom runtimes, where the ACP sentinel + /// is user pass-through transport that must survive a spawn — not a foreign + /// key to drop. Known runtimes leave it `false`: a bare destination-key + /// value with no resolved authority is invalid/foreign and is dropped. + pub preserve_passthrough: bool, +} + +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. + /// + /// Suppression is ASCII-case-insensitive: Windows `Command` case-folds env + /// names, so a hand-set `goose_thinking_effort` would otherwise evade an + /// exact-case strip and shadow the projected authority. + /// + /// When `preserve_passthrough` is set and no tier resolved a value, a value + /// already present under `key` (in any case) is carried forward and + /// re-emitted canonically. Multiple case spellings can survive the + /// case-sensitive layer merge (e.g. a lower-tier `BUZZ_ACP_EFFORT_LEVEL` + /// plus a higher-tier `buzz_acp_effort_level`); the carry selects the LAST + /// case-insensitive match in `BTreeMap` iteration order, which is exactly + /// the value Rust's Windows `Command` writer produces — it sets each spelling + /// in iteration order into a case-folded env map, so the last set wins. This + /// keeps an unknown/custom runtime's hand-set sentinel alive, preserves the + /// value the child would actually receive, and guarantees one canonical + /// spelling downstream. + pub(crate) fn apply(&self, env: &mut BTreeMap) { + let carried = (self.value.is_none() && self.preserve_passthrough) + .then(|| { + env.iter() + .rev() + .find(|(k, _)| k.eq_ignore_ascii_case(self.key)) + .map(|(_, v)| v.clone()) + }) + .flatten(); + env.retain(|k, _| { + !self + .suppress + .iter() + .any(|suppressed| k.eq_ignore_ascii_case(suppressed)) + }); + if let Some(v) = self.value.as_ref().or(carried.as_ref()) { + env.insert(self.key.to_string(), v.clone()); + } + } +} + +/// Look up `key` in `map` case-insensitively (ASCII), selecting the LAST +/// case-insensitive match in `BTreeMap` iteration order. Effort key resolution +/// must match Windows `Command` env semantics: `Command` writes each spelling +/// in iteration order into a case-folded env map, so the last-set spelling wins +/// and is the value the child actually receives. Preferring an exact match +/// instead would pick a different case variant than the child gets — e.g. +/// `GOOSE_THINKING_EFFORT=low` plus `goose_thinking_effort=high` would resolve +/// to `low` while the child runs `high`. This mirrors `EffortLaunch::apply`'s +/// `.rev().find` carry so the tier reader, the passthrough carry, and the child +/// all agree on one value. +pub(crate) fn get_ci<'a>(map: &'a BTreeMap, key: &str) -> Option<&'a String> { + map.iter() + .rev() + .find(|(k, _)| k.eq_ignore_ascii_case(key)) + .map(|(_, v)| v) +} + +/// 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) = get_ci(map, 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) = get_ci(map, LEGACY_THINKING_EFFORT_KEY) { + if let Some(canonical) = norm(raw) { + return Some(canonical); + } + } + } + None +} + +/// Normalize/validate an effort candidate for a runtime's destination +/// vocabulary. The single value gate shared by the launch projection and the +/// reader, so the panel and the next spawn never disagree on a value's validity. +/// +/// - `contract` present (Goose): canonicalize through the alias table; invalid +/// → `None` (skip as absent). +/// - `contract` absent but `accepted` present (buzz-agent): validation-only — +/// accept a value case-insensitively iff the destination parser would +/// (`parse_thinking_effort`), emit it lowercased; a foreign canonical (e.g. +/// Goose `off`) is rejected so it is never emitted as +/// `BUZZ_AGENT_THINKING_EFFORT=off`, which crashes the child at config init. +/// - both absent (Claude/Codex, unknown/custom): raw passthrough — the value +/// rides `BUZZ_ACP_EFFORT_LEVEL` to an adapter that accepts any string. +pub(crate) fn normalize_effort( + contract: Option<&EffortNormalization>, + accepted: Option<&[&str]>, + raw: &str, +) -> Option { + match contract { + Some(c) => c.normalize_str(raw), + None => match accepted { + Some(values) => { + let lower = raw.trim().to_ascii_lowercase(); + values.iter().any(|v| *v == lower).then_some(lower) + } + None => Some(raw.to_string()), + }, + } +} + +/// 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 +} + +/// Strip every known effort key from a [`std::process::Command`] before the +/// descriptor overlay is written. +/// +/// Only used in tests to verify tombstone assertions on individual keys. +/// Production stripping runs inside `apply_effort_launch_to_command` +/// (the loop over `launch.suppress`) which is exercised by the +/// production-sequence tests. +#[cfg(test)] +pub(crate) fn strip_effort_keys_from_command(cmd: &mut std::process::Command) { + for key in effort_suppress_keys() { + cmd.env_remove(key); + // Belt-and-suspenders for Unix inherited env with non-canonical casing + // (e.g. a shell export of `goose_thinking_effort`). Our own cmd.env() + // calls always use UPPER_SNAKE_CASE; only ambient inherited keys can + // arrive in non-standard case on Unix. + let lower = key.to_ascii_lowercase(); + if lower != key { + cmd.env_remove(&lower); + } + } +} + +/// Strip effort keys and emit the projected effort value to a +/// [`std::process::Command`]. +/// +/// This is the production command-boundary seam: call after +/// `build_buzz_agent_provider_defaults` (which writes raw baked env) and +/// before the `descriptor.env` loop (which overlays the projected key). +/// Extracting both steps into one call lets tests exercise the full +/// baked-write → strip → emit sequence and inspect the child's effective +/// environment, making the test fail if either step is removed or misordered +/// in production. +/// +/// Strip policy follows `launch.suppress`: for known runtimes that is the full +/// effort vocabulary; for unknown/custom runtimes it is only the ACP sentinel, +/// leaving foreign effort keys (e.g. a wrapper's own `GOOSE_THINKING_EFFORT`) +/// untouched. Each key is stripped in canonical and lowercase form so ambient +/// inherited env with non-canonical casing is swept on Unix. +/// +/// When `launch.preserve_passthrough` is set and `launch.value` is `None` +/// (unknown runtime, no authoritative column), the suppress set is skipped +/// entirely: the inherited process env carries the user's hand-set sentinel, +/// and stripping it here without a re-emit would silently drop it. Known +/// runtimes always have a resolved `value` or do not set `preserve_passthrough`. +pub(crate) fn apply_effort_launch_to_command( + cmd: &mut std::process::Command, + launch: &EffortLaunch, +) { + // For unknown/custom runtimes with no resolved value the suppress set is + // only the ACP sentinel, and stripping it without re-emitting would destroy + // the user's ambient pass-through config. Skip the strip entirely and let + // the inherited env carry it through unchanged. + // MUTATION: removing this guard strips the sentinel and breaks + // `production_sequence_custom_inherited_acp_sentinel_survives`. + if launch.preserve_passthrough && launch.value.is_none() { + return; + } + for key in &launch.suppress { + cmd.env_remove(key); + let lower = key.to_ascii_lowercase(); + if lower.as_str() != *key { + cmd.env_remove(&lower); + } + } + if let Some(ref value) = launch.value { + cmd.env(launch.key, value); + } +} + +/// The effort keys the restart snapshot must strip from its captured launch env +/// so effort keeps exactly ONE representation (`effort_level`), mirroring what +/// [`effort_launch_projection`] actually suppressed for `runtime`: +/// +/// - **known runtime** — the full suppress set. The projection already swept +/// every effort key to the single destination key, so this removes only that +/// destination key (a no-op on the already-swept siblings). +/// - **unknown/custom runtime** — only the ACP-startup sentinel. The projection +/// suppresses just the sentinel here (reconciling every case variant to the +/// canonical spelling — external review, Carl P2), leaving every other +/// effort-looking key (e.g. a hand-rolled `GOOSE_THINKING_EFFORT`) untouched +/// as ordinary env. Those must remain in `env` so an edit to them diffs the +/// snapshot normally; only the sentinel — the key the projection emits and +/// `effective_effort` reads into `effort_level` — is removed. +pub(crate) fn snapshot_suppress_keys(runtime: Option<&KnownAcpRuntime>) -> Vec<&'static str> { + if runtime.is_some() { + effort_suppress_keys() + } else { + vec![effort_dest_key(runtime)] + } +} + +/// 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); + + // Suppress the full effort vocabulary for KNOWN runtimes. For an + // unknown/custom runtime (external review #2) we keep every foreign + // effort-looking key as pass-through — a hand-rolled `GOOSE_THINKING_EFFORT` + // on a custom Goose wrapper must reach the child untouched — EXCEPT our own + // ACP-startup sentinel, which we always reconcile to a single canonical + // spelling (external review, Carl P2): the projection emits the sentinel, so + // a user-set case variant (e.g. `buzz_acp_effort_level`) is never intentional + // config, and leaving one to shadow the emitted `BUZZ_ACP_EFFORT_LEVEL` on + // Windows (where `Command` case-folds env names) would hand the child a + // different value than the snapshot reads. Stripping the sentinel here and + // re-emitting canonically guarantees at most ONE sentinel spelling downstream, + // so the child, the restart snapshot, and the badge cannot disagree on case. + let suppress = if runtime.is_some() { + effort_suppress_keys() + } else { + vec![ACP_STARTUP_EFFORT_KEY] + }; + // When no tier resolves a value, an unknown runtime still preserves a + // hand-set sentinel the user routed to the child (the retained pass-through + // from external review #2) — carried forward and re-emitted canonically by + // `apply`. Known runtimes never preserve a bare dest-key value: it is either + // the projection's own emission or a foreign key, both handled by `value`. + let preserve_passthrough = runtime.is_none(); + + // Value gate: Goose canonicalizes through its alias contract; buzz-agent + // validates against its accepted set (invalid → skip, so a foreign + // canonical like Goose `off` is never emitted where the destination parser + // rejects it); Claude/Codex and unknown/custom pass raw over the sentinel. + let contract = runtime.and_then(|r| r.effort_normalization); + let accepted = runtime.and_then(|r| r.effort_accepted_values); + let norm = |raw: &str| -> Option { normalize_effort(contract, accepted, raw) }; + + // 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, + preserve_passthrough, + } +} + +/// 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) = get_ci(&record_env, 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) = get_ci(&record_env, 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) = get_ci(baked_env, nk) { + if let Some(v) = norm(raw) { + return Some(v); + } + } + None +} + +/// Combined spawn seam: baked-env write + effort strip + emit. +/// +/// Called by `apply_effort_to_spawn_command` in `runtime.rs` (production path) +/// and by `effort_cmd_tests` (test seam). Deleting `build_buzz_agent_provider_defaults` +/// or `apply_effort_launch_to_command` inside turns the production-sequence tests RED. +/// Deleting the outer `apply_effort_to_spawn_command` call from `spawn_agent_child` +/// is a compile error — `spawn_with_effort_proof` consumes the returned `EffortApplied` +/// token, so removing the binding leaves `effort` undefined at the spawn site. +pub(crate) fn apply_spawn_effort_env( + cmd: &mut std::process::Command, + record: &ManagedAgentRecord, + runtime: Option<&KnownAcpRuntime>, + personas: &[AgentDefinition], + persona_id: Option<&str>, + global_env: &BTreeMap, + baked_env: &BTreeMap, +) { + crate::managed_agents::agent_env::build_buzz_agent_provider_defaults(cmd); + let launch = effort_launch_projection( + record, runtime, personas, persona_id, global_env, None, baked_env, + ); + apply_effort_launch_to_command(cmd, &launch); +} + +#[cfg(test)] +#[path = "effort_tests.rs"] +mod tests; diff --git a/desktop/src-tauri/src/managed_agents/config_bridge/effort_cmd_tests.rs b/desktop/src-tauri/src/managed_agents/config_bridge/effort_cmd_tests.rs new file mode 100644 index 00000000000..172f373d79d --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/config_bridge/effort_cmd_tests.rs @@ -0,0 +1,356 @@ +//! Command-boundary strip and production-sequence seam tests for effort. +//! +//! Split from `effort_tests.rs` to stay within the file-size ratchet. +//! Covers `strip_effort_keys_from_command` tombstone assertions and the +//! child-process spawn sequence via `apply_effort_to_spawn_command`. +//! +//! The production-sequence tests call `apply_effort_to_spawn_command` +//! (`runtime.rs`), the same function `spawn_agent_child` calls. Deleting +//! `apply_spawn_effort_env` from that wrapper turns these tests RED. +//! Deleting the `apply_effort_to_spawn_command` call from `spawn_agent_child` +//! is a compile error: `spawn_with_effort_proof` consumes the returned +//! `EffortApplied` by value, so removing the binding leaves `effort` undefined +//! at the spawn site. + +use std::collections::BTreeMap; + +use super::super::strip_effort_keys_from_command; +use super::*; +use crate::managed_agents::runtime::apply_effort_to_spawn_command; + +// -------------------------------------------------------------------------- +// Command-boundary strip (P1: inherited + baked collision) +// -------------------------------------------------------------------------- + +/// ACP sentinel baked/inherited collision: registered for removal after strip. +#[test] +fn strip_removes_baked_acp_sentinel_collision() { + let mut cmd = std::process::Command::new("echo"); + cmd.env(ACP_KEY, "high"); + strip_effort_keys_from_command(&mut cmd); + let removed = cmd + .get_envs() + .any(|(key, value)| key == ACP_KEY && value.is_none()); + assert!( + removed, + "ACP sentinel must be registered for removal after strip" + ); +} + +/// Baked `GOOSE_THINKING_EFFORT` collision: stripped before descriptor overlay. +#[test] +fn strip_removes_baked_goose_native_key_collision() { + let mut cmd = std::process::Command::new("echo"); + cmd.env(GOOSE_KEY, "high"); + strip_effort_keys_from_command(&mut cmd); + let removed = cmd + .get_envs() + .any(|(key, value)| key == GOOSE_KEY && value.is_none()); + assert!( + removed, + "GOOSE_THINKING_EFFORT must be registered for removal after strip" + ); +} + +/// Baked `BUZZ_AGENT_THINKING_EFFORT` collision: legacy alias stripped. +#[test] +fn strip_removes_baked_buzz_agent_native_key_collision() { + let mut cmd = std::process::Command::new("echo"); + cmd.env(BUZZ_AGENT_KEY, "medium"); + strip_effort_keys_from_command(&mut cmd); + let removed = cmd + .get_envs() + .any(|(key, value)| key == BUZZ_AGENT_KEY && value.is_none()); + assert!( + removed, + "BUZZ_AGENT_THINKING_EFFORT must be registered for removal after strip" + ); +} + +/// Lowercase inherited key: both canonical and lowercase variants are stripped. +#[test] +fn strip_removes_lowercase_goose_key_inherited_from_shell() { + let lower = GOOSE_KEY.to_ascii_lowercase(); + let mut cmd = std::process::Command::new("echo"); + cmd.env(&lower, "stale"); + strip_effort_keys_from_command(&mut cmd); + let removed = cmd + .get_envs() + .any(|(key, value)| key == lower.as_str() && value.is_none()); + assert!( + removed, + "lowercase GOOSE key must be registered for removal" + ); +} + +/// Custom passthrough: non-suppress-set keys are not removed. +#[test] +fn strip_does_not_remove_unrelated_env_key() { + let mut cmd = std::process::Command::new("echo"); + cmd.env("MY_CUSTOM_EFFORT", "high"); + strip_effort_keys_from_command(&mut cmd); + let value_present = cmd + .get_envs() + .any(|(key, value)| key == "MY_CUSTOM_EFFORT" && value.is_some()); + assert!( + value_present, + "strip must not touch env keys outside the suppress set" + ); +} + +// -------------------------------------------------------------------------- +// Production-sequence seam tests +// -------------------------------------------------------------------------- +// Spawn the child directly so its actual env is the ground truth. +// These call `apply_effort_to_spawn_command` (in `runtime.rs`), the same function +// `spawn_agent_child` calls. Deleting `apply_spawn_effort_env` from that wrapper +// turns these tests RED. The `EffortApplied` sentinel makes the call site in +// `spawn_agent_child` a compile-time requirement. +// Deletion proofs: +// - remove `build_buzz_agent_provider_defaults` inside → baked keys leak; +// - remove `effort_launch_projection` → suppress list is empty, keys leak; +// - remove `apply_effort_launch_to_command` → stale keys remain, assertion fails. +// +// Inherited-state tests seed the parent env via `std::env::set_var` under the +// crate-wide env lock (`crate::managed_agents::lock_env_mutex`). `EnvVarGuard` +// restores the exact prior value (including non-Unicode) in `Drop`, so panics +// do not leak the seeded value into unrelated child-spawn tests. + +/// RAII guard: snapshots a process-env variable and restores the exact prior +/// value (or removes it if it was absent) on `Drop`, even on panic. +/// Uses `OsString` so a pre-existing non-Unicode value is restored exactly +/// rather than being silently lost. +struct EnvVarGuard { + key: String, + prior: Option, +} +impl EnvVarGuard { + fn set(key: &str, value: &str) -> Self { + let prior = std::env::var_os(key); + #[allow(deprecated)] + unsafe { + std::env::set_var(key, value); + } + Self { + key: key.to_string(), + prior, + } + } +} +impl Drop for EnvVarGuard { + fn drop(&mut self) { + #[allow(deprecated)] + unsafe { + match &self.prior { + Some(v) => std::env::set_var(&self.key, v), + None => std::env::remove_var(&self.key), + } + } + } +} + +fn run_env_cmd(cmd: &mut std::process::Command) -> String { + let output = cmd + .output() + .expect("env-dump command must be executable on this host"); + assert!( + output.status.success(), + "env command failed: {:?}", + output.status + ); + String::from_utf8_lossy(&output.stdout).to_string() +} + +/// After projection + strip + emit, the child sees exactly the projected Goose +/// key with no collision. Inherited lowercase key is seeded via EnvVarGuard. +#[test] +#[cfg(not(target_os = "windows"))] +fn production_sequence_goose_inherited_collision_resolved_in_child() { + let lower = GOOSE_KEY.to_ascii_lowercase(); + let _lock = crate::managed_agents::lock_env_mutex(); + let _guard = EnvVarGuard::set(&lower, "inherited-low"); + + let mut cmd = std::process::Command::new("/usr/bin/env"); + cmd.env(GOOSE_KEY, "baked-high"); + cmd.env(BUZZ_AGENT_KEY, "legacy-medium"); + cmd.env("MY_AGENT_CONFIG", "keep-me"); + + let mut r = record(); + r.effort_level = Some("high".into()); + let _effort = apply_effort_to_spawn_command( + &mut cmd, + &r, + Some(goose()), + &[], + None, + &BTreeMap::new(), + &BTreeMap::new(), + ); + let child_env = run_env_cmd(&mut cmd); + + assert!( + child_env.contains(&format!("{GOOSE_KEY}=high")), + "child must receive the projected Goose key; env:\n{child_env}" + ); + assert!( + !child_env.contains(BUZZ_AGENT_KEY), + "legacy buzz-agent key must not reach child; env:\n{child_env}" + ); + assert!( + !child_env.contains(ACP_KEY), + "ACP sentinel must not reach child for Goose; env:\n{child_env}" + ); + assert!( + !child_env.contains(&format!("{lower}=inherited-low")), + "inherited lowercase key must be stripped; env:\n{child_env}" + ); + assert!( + child_env.contains("MY_AGENT_CONFIG=keep-me"), + "unrelated key must survive; env:\n{child_env}" + ); + let effort_key_count = [GOOSE_KEY, BUZZ_AGENT_KEY, ACP_KEY] + .iter() + .filter(|k| child_env.contains(&format!("{k}="))) + .count(); + assert_eq!( + effort_key_count, 1, + "exactly one effort key must reach child; env:\n{child_env}" + ); +} + +/// Windows: OS case-folds env keys, so stripping canonical removes ALL case variants. +#[test] +#[cfg(target_os = "windows")] +fn production_sequence_arbitrary_mixedcase_collision_absent_from_child_windows() { + let mixed = "GoOsE_ThInKiNg_EfFoRt"; + let mut cmd = std::process::Command::new("cmd"); + cmd.args(["/c", "set"]); + cmd.env_clear(); + cmd.env(mixed, "stale-mixed"); + let mut r = record(); + r.effort_level = Some("high".into()); + let _effort = apply_effort_to_spawn_command( + &mut cmd, + &r, + Some(goose()), + &[], + None, + &BTreeMap::new(), + &BTreeMap::new(), + ); + let child_env = run_env_cmd(&mut cmd); + assert!( + child_env + .to_ascii_uppercase() + .contains(&format!("{}=HIGH", GOOSE_KEY.to_ascii_uppercase())), + "canonical effort key must reach the child; env:\n{child_env}" + ); + assert!( + !child_env + .to_ascii_uppercase() + .contains(&format!("{}=STALE-MIXED", mixed.to_ascii_uppercase())), + "mixed-case effort key must not reach the child; env:\n{child_env}" + ); +} + +/// Custom passthrough: non-suppress-set effort keys survive the production sequence. +#[test] +#[cfg(not(target_os = "windows"))] +fn production_sequence_custom_passthrough_survives() { + let mut cmd = std::process::Command::new("/usr/bin/env"); + cmd.env_clear(); + cmd.env("MY_HARNESS_EFFORT", "high"); + cmd.env("MY_UNRELATED_CONFIG", "keep"); + let _effort = apply_effort_to_spawn_command( + &mut cmd, + &record(), + None, + &[], + None, + &BTreeMap::new(), + &BTreeMap::new(), + ); + let child_env = run_env_cmd(&mut cmd); + assert!( + child_env.contains("MY_HARNESS_EFFORT=high"), + "custom key must survive; env:\n{child_env}" + ); + assert!( + child_env.contains("MY_UNRELATED_CONFIG=keep"), + "unrelated key must survive; env:\n{child_env}" + ); +} + +/// Custom-runtime: inherited `GOOSE_THINKING_EFFORT` survives (unknown-runtime +/// suppress set excludes foreign effort keys). +#[test] +#[cfg(not(target_os = "windows"))] +fn production_sequence_custom_inherited_goose_key_survives() { + let _lock = crate::managed_agents::lock_env_mutex(); + let _guard = EnvVarGuard::set(GOOSE_KEY, "inherited-high"); + let mut cmd = std::process::Command::new("/usr/bin/env"); + let _effort = apply_effort_to_spawn_command( + &mut cmd, + &record(), + None, + &[], + None, + &BTreeMap::new(), + &BTreeMap::new(), + ); + let child_env = run_env_cmd(&mut cmd); + assert!( + child_env.contains(&format!("{GOOSE_KEY}=inherited-high")), + "GOOSE key must survive for unknown runtime; env:\n{child_env}" + ); +} + +/// Custom-runtime: inherited ACP sentinel survives as pass-through (no column). +#[test] +#[cfg(not(target_os = "windows"))] +fn production_sequence_custom_inherited_acp_sentinel_survives() { + let _lock = crate::managed_agents::lock_env_mutex(); + let _guard = EnvVarGuard::set(ACP_KEY, "inherited-val"); + let mut cmd = std::process::Command::new("/usr/bin/env"); + let _effort = apply_effort_to_spawn_command( + &mut cmd, + &record(), + None, + &[], + None, + &BTreeMap::new(), + &BTreeMap::new(), + ); + let child_env = run_env_cmd(&mut cmd); + assert!( + child_env.contains(&format!("{ACP_KEY}=inherited-val")), + "ACP sentinel must survive for unknown runtime with no column; env:\n{child_env}" + ); +} + +/// Windows: custom-wrapper effort keys survive the production sequence. +#[test] +#[cfg(target_os = "windows")] +fn production_sequence_custom_passthrough_survives() { + let mut cmd = std::process::Command::new("cmd"); + cmd.args(["/c", "set"]); + cmd.env_clear(); + cmd.env("MY_HARNESS_EFFORT", "high"); + cmd.env("MY_UNRELATED_CONFIG", "keep"); + let _effort = apply_effort_to_spawn_command( + &mut cmd, + &record(), + None, + &[], + None, + &BTreeMap::new(), + &BTreeMap::new(), + ); + let child_env = run_env_cmd(&mut cmd); + assert!(child_env + .to_ascii_uppercase() + .contains("MY_HARNESS_EFFORT=HIGH")); + assert!(child_env + .to_ascii_uppercase() + .contains("MY_UNRELATED_CONFIG=KEEP")); +} 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..9c4568fceb4 --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/config_bridge/effort_tests.rs @@ -0,0 +1,701 @@ +//! Parity matrix for the single harness-agnostic effort projection +//! (`effort_launch_projection`, PR #4625). +//! +//! Covers, per runtime: CLEAR authority order; decisive mixed-authority; +//! `value == None` when no tier resolves; single-key emission + suppress; +//! 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}; + +pub(super) const GOOSE_KEY: &str = "GOOSE_THINKING_EFFORT"; +pub(super) const BUZZ_AGENT_KEY: &str = "BUZZ_AGENT_THINKING_EFFORT"; +pub(super) const ACP_KEY: &str = "BUZZ_ACP_EFFORT_LEVEL"; + +pub(super) 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") +} + +pub(super) 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, + description: 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, + team_catalog_source: 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, + description: 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, + team_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() { + 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() { + 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() { + 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())); + 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") + ); +} + +// -------------------------------------------------------------------------- +// External review fix #2 — unknown/custom runtimes restore main's pass-through +// -------------------------------------------------------------------------- + +#[test] +fn unknown_runtime_does_not_suppress_user_effort_env() { + // Regression: a custom wrapper with GOOSE_THINKING_EFFORT=high in record env + // must reach the child unchanged. For unknown runtimes `suppress` is exactly + // `[BUZZ_ACP_EFFORT_LEVEL]` — no foreign effort key is stripped. + let mut r = record(); + r.env_vars = env(&[(GOOSE_KEY, "high"), ("UNRELATED", "keep")]); + let launch = project_record_only(&r, None); + assert_eq!( + launch.suppress, + vec![ACP_KEY], + "unknown runtime suppresses only its own sentinel, never a foreign key" + ); + + let mut launch_env = r.env_vars.clone(); + launch.apply(&mut launch_env); + assert_eq!( + launch_env.get(GOOSE_KEY).map(String::as_str), + Some("high"), + "custom-wrapper effort key survives an unknown-runtime launch" + ); + assert_eq!( + launch_env.get("UNRELATED").map(String::as_str), + Some("keep") + ); +} + +#[test] +fn unknown_runtime_keeps_user_acp_sentinel_when_no_column() { + // Custom adapter with hand-set sentinel and no column: sentinel carries through. + let mut r = record(); + r.env_vars = env(&[(ACP_KEY, "low")]); + let launch = project_record_only(&r, None); + assert_eq!(launch.value, None); + assert!(launch.preserve_passthrough); + let mut launch_env = r.env_vars.clone(); + launch.apply(&mut launch_env); + assert_eq!( + launch_env.get(ACP_KEY).map(String::as_str), + Some("low"), + "hand-set sentinel survives on an unknown runtime with no column" + ); +} + +#[test] +fn unknown_runtime_collapses_mixed_case_sentinel_to_canonical_when_no_column() { + // Carl P2 (no-column): a hand-set mixed-case sentinel on a custom runtime + // must survive AND be re-emitted under the canonical spelling. Leaving the + // lowercase variant would hand the child a value the snapshot read misses. + let mut r = record(); + r.env_vars = env(&[("buzz_acp_effort_level", "low")]); + let launch = project_record_only(&r, None); + assert_eq!(launch.value, None); + let mut launch_env = r.env_vars.clone(); + launch.apply(&mut launch_env); + assert_eq!( + launch_env.get(ACP_KEY).map(String::as_str), + Some("low"), + "mixed-case pass-through sentinel re-emitted under canonical key" + ); + assert_eq!( + launch_env.get("buzz_acp_effort_level"), + None, + "mixed-case spelling is collapsed away" + ); +} + +#[test] +fn unknown_runtime_no_column_multi_variant_preserves_windows_effective_value() { + // Pass-3 IMPORTANT (Thufir): both case spellings of the sentinel survive the + // case-sensitive layer merge. Rust `Command` writes in `BTreeMap` iteration + // order into a case-folded env map (last set wins); canonical `B` sorts before + // lowercase `b`, so the lowercase `low` is written last and wins. The carry + // selects the LAST case-insensitive match, matching that. + let mut r = record(); + r.env_vars = env(&[(ACP_KEY, "high"), ("buzz_acp_effort_level", "low")]); + let launch = project_record_only(&r, None); + assert_eq!(launch.value, None); + assert!(launch.preserve_passthrough); + let mut launch_env = r.env_vars.clone(); + launch.apply(&mut launch_env); + assert_eq!( + launch_env.get(ACP_KEY).map(String::as_str), + Some("low"), + "carry preserves the last-in-iteration-order value the Windows child receives" + ); + assert_eq!(launch_env.get("buzz_acp_effort_level"), None); + // Mutation: reverting the carry to exact-first `get_ci` selects `high`. +} + +#[test] +fn unknown_runtime_column_wins_over_mixed_case_sentinel() { + // Carl P2 (with-column): canonical column plus mixed-case sentinel. Column + // wins; the projection strips ALL case variants of the sentinel before emit. + let mut r = record(); + r.effort_level = Some("high".into()); + r.env_vars = env(&[("buzz_acp_effort_level", "low")]); + let launch = project_record_only(&r, None); + assert_eq!(launch.value.as_deref(), Some("high")); + assert_eq!(launch.key, ACP_KEY); + let mut launch_env = r.env_vars.clone(); + launch.apply(&mut launch_env); + assert_eq!( + launch_env.get(ACP_KEY).map(String::as_str), + Some("high"), + "column wins, emitted under canonical sentinel key" + ); + assert_eq!(launch_env.get("buzz_acp_effort_level"), None); + // Mutation: empty suppress set leaves `buzz_acp_effort_level=low` in child. +} + +#[test] +fn unknown_runtime_column_still_emits_under_acp_sentinel() { + // The retained compatibility emission: an unknown runtime with a canonical + // column emits it raw under the ACP sentinel (matches the PR-body decision). + let mut r = record(); + r.effort_level = Some("high".into()); + let launch = project_record_only(&r, None); + assert_eq!(launch.value.as_deref(), Some("high")); + assert_eq!(launch.key, ACP_KEY); +} + +// -------------------------------------------------------------------------- +// External review fix #3 — destination-vocabulary validation at projection +// -------------------------------------------------------------------------- + +#[test] +fn goose_off_column_skips_for_buzz_agent_destination() { + // Regression: canonical column `off` is valid Goose but NOT a buzz-agent + // effort. Switching a record with effort_level=off to buzz-agent must NOT + // emit BUZZ_AGENT_THINKING_EFFORT=off — parse_thinking_effort rejects it and + // the child exits 2. Invalid → skip as absent → no key emitted. + let mut r = record(); + r.effort_level = Some("off".into()); + let launch = project_record_only(&r, Some(buzz_agent())); + assert_eq!( + launch.value, None, + "foreign canonical `off` skipped for buzz-agent's vocabulary" + ); + + let mut launch_env = BTreeMap::new(); + launch.apply(&mut launch_env); + assert_eq!( + launch_env.get(BUZZ_AGENT_KEY), + None, + "no effort key emitted when the value is outside the destination vocabulary" + ); +} + +#[test] +fn buzz_agent_minimal_column_skips_for_goose_destination() { + // The reverse: `minimal` is a valid buzz-agent effort but invalid Goose, so + // switching to Goose skips it as absent (already covered by normalization, + // pinned here as the symmetric vocabulary case). + let mut r = record(); + r.effort_level = Some("minimal".into()); + let launch = project_record_only(&r, Some(goose())); + assert_eq!(launch.value, None); +} + +#[test] +fn buzz_agent_accepts_its_own_distinct_efforts() { + // buzz-agent keeps xhigh and max distinct (no Goose-style xhigh→max + // collapse): both are valid and pass through unchanged. + for v in ["xhigh", "max", "none", "minimal"] { + let mut r = record(); + r.effort_level = Some(v.into()); + let launch = project_record_only(&r, Some(buzz_agent())); + assert_eq!( + launch.value.as_deref(), + Some(v), + "buzz-agent accepts `{v}` verbatim (no alias collapse)" + ); + } +} + +// -------------------------------------------------------------------------- +// External review fix #4 — case-insensitive suppression / lookup +// -------------------------------------------------------------------------- + +#[test] +fn mixed_case_native_key_is_read_and_wins() { + // Windows Command case-folds env names, so `goose_thinking_effort` is the + // same variable as the canonical form. The tier reader must find it. + let mut r = record(); + r.env_vars = env(&[("goose_thinking_effort", "low")]); + r.effort_level = Some("high".into()); + let launch = project_record_only(&r, Some(goose())); + assert_eq!( + launch.value.as_deref(), + Some("low"), + "mixed-case record-native key is read and outranks the column" + ); +} + +#[test] +fn duplicate_case_native_variants_resolve_to_windows_effective_value() { + // Carl P2 (r8): both case spellings of a known native key in the record env. + // Rust `Command` writes in `BTreeMap` order into a case-folded map; canonical + // `GOOSE_THINKING_EFFORT` sorts before lowercase, so the lowercase `high` is + // written last and wins. `get_ci` must select the LAST match, not exact-case. + let mut r = record(); + r.env_vars = env(&[(GOOSE_KEY, "low"), ("goose_thinking_effort", "high")]); + let launch = project_record_only(&r, Some(goose())); + assert_eq!( + launch.value.as_deref(), + Some("high"), + "known-runtime native lookup selects the last case variant Windows Command sets" + ); + // Mutation: reverting `get_ci` to exact-first selects `low`. +} + +#[test] +fn apply_strips_mixed_case_effort_keys() { + // A hand-set mixed-case foreign effort key must be swept, not left to + // shadow the projected value once Windows case-folds it at spawn. + let mut r = record(); + r.effort_level = Some("high".into()); + let launch = project_record_only(&r, Some(goose())); + + let mut launch_env = env(&[ + ("Goose_Thinking_Effort", "stale"), + ("buzz_acp_effort_level", "stale"), + ("UNRELATED", "keep"), + ]); + launch.apply(&mut launch_env); + + // Only the canonical projected key remains; both mixed-case foreign keys + // are gone. + assert_eq!(launch_env.get(GOOSE_KEY).map(String::as_str), Some("high")); + assert_eq!(launch_env.get("Goose_Thinking_Effort"), None); + assert_eq!(launch_env.get("buzz_acp_effort_level"), None); + assert_eq!( + launch_env.get("UNRELATED").map(String::as_str), + Some("keep") + ); +} + +// Command-boundary strip and production-sequence tests are in the sibling module. +#[cfg(test)] +#[path = "effort_cmd_tests.rs"] +mod cmd_tests; 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..84eec8db33a 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,8 @@ 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 effort_accepted = runtime_meta.and_then(|m| m.effort_accepted_values); 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 +98,8 @@ pub(crate) fn read_config_surface( &acp_effort, effort_option.map(|o| o.config_id.as_str()), thinking_env_var, + effort_norm, + effort_accepted, is_pre_spawn, tiers, ), @@ -126,7 +133,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,10 +145,40 @@ pub(crate) fn read_config_surface( .flatten() .collect(); - // Tier 2a: remaining env vars not covered by normalized fields. + // 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 + && super::effort::get_ci(&record.env_vars, native) + .and_then(|v| norm.normalize_str(v)) + .is_none() + && record + .effort_level + .as_deref() + .and_then(|v| norm.normalize_str(v)) + .is_none() + && super::effort::get_ci(&record.env_vars, 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. Matching is + // ASCII-case-insensitive so a mixed-case managed key (e.g. Windows + // `goose_thinking_effort`) the launch projection already consumed is hidden + // from Advanced rather than shown as a spurious editable extra. let mut advanced = advanced; for (k, v) in &record.env_vars { - if normalized_env_keys.contains(&k.as_str()) { + if normalized_env_keys + .iter() + .any(|nk| nk.eq_ignore_ascii_case(k)) + { continue; } if file_config.extra.contains_key(k) { @@ -542,40 +579,92 @@ fn build_thinking_field( acp_effort: &Option, effort_config_id: Option<&str>, thinking_env_var: Option<&str>, + effort_norm: Option<&'static EffortNormalization>, + effort_accepted: Option<&'static [&'static str]>, 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 { + super::effort::normalize_effort(effort_norm, effort_accepted, raw) + }; - 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| super::effort::get_ci(&record.env_vars, 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(|_| super::effort::get_ci(&record.env_vars, 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 through the runtime CONTRACT only, never the + // persisted `effort_accepted` vocabulary. The ACP running value comes from + // the session's own config-option namespace (e.g. buzz-agent reports + // `default` for its live thinking-level option) — it is a descriptive + // "currently running" fact, never emitted to a spawn, so the + // destination-vocabulary gate that guards the writable tiers must not skip + // it. Goose still canonicalizes (its ACP option values ARE effort values); + // contract-less runtimes pass raw. The matched `config_id` is preserved for + // `write_via` regardless of value validity. + let acp_norm = acp_effort + .as_deref() + .and_then(|v| super::effort::normalize_effort(effort_norm, None, v)); + + // 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 +835,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 64834d1017a..34b4f1496f5 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,16 @@ 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), + effort_accepted_values: None, 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 { @@ -647,6 +650,8 @@ 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, + effort_accepted_values: 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"), @@ -958,3 +963,6 @@ fn numeric_max_tokens_inherits_from_global_env() { // ── Extended tests (split file to respect line-count ratchet) ──────────────── #[path = "reader_tests_ext.rs"] mod ext; + +#[path = "reader_tests_ext2.rs"] +mod ext2; 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 0974bf9c581..fc18a51c622 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/config_bridge/reader_tests_ext2.rs b/desktop/src-tauri/src/managed_agents/config_bridge/reader_tests_ext2.rs new file mode 100644 index 00000000000..0c5aa69c407 --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/config_bridge/reader_tests_ext2.rs @@ -0,0 +1,69 @@ +//! Additional tests for `config_bridge/reader.rs` — split out to keep +//! `reader_tests_ext.rs` under the 1000-line file-size ratchet. +//! +//! Included as `mod ext2` inside `reader_tests.rs`, so `use super::*` gives +//! access to all helpers and types from that module. + +use super::*; + +// ── Fix (external review #4): reader resolves record effort keys ───────────── +// case-insensitively, matching the launch projection. +// +// Windows `Command` case-folds env names, so a hand-set `goose_thinking_effort` +// is the same variable as its canonical form. The reader must resolve it as the +// record-native effort winner AND hide it from Advanced, or the panel disagrees +// with the child the launch projection already consumed the key for. + +/// Mixed-case native record key `goose_thinking_effort=high` wins the record +/// tier and is hidden from Advanced (not shown as a spurious editable extra). +#[test] +fn record_mixed_case_native_effort_wins_and_hidden_from_advanced_reader() { + let mut record = test_record(); + record + .env_vars + .insert("goose_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("mixed-case native key must surface as the record effort winner"); + 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(&"goose_thinking_effort"), + "consumed mixed-case native effort key must not appear in advanced; got {advanced_keys:?}" + ); +} + +/// Mixed-case legacy record key `buzz_agent_thinking_effort=high` (no native, +/// no column) supplies the record effort AND is hidden from Advanced. +#[test] +fn record_mixed_case_legacy_effort_consumed_and_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 + + 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("mixed-case legacy key 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 mixed-case legacy effort key must not appear in advanced; got {advanced_keys:?}" + ); +} diff --git a/desktop/src-tauri/src/managed_agents/discovery.rs b/desktop/src-tauri/src/managed_agents/discovery.rs index 1ee7e6e5562..84f88e406b5 100644 --- a/desktop/src-tauri/src/managed_agents/discovery.rs +++ b/desktop/src-tauri/src/managed_agents/discovery.rs @@ -15,6 +15,8 @@ mod presets; mod runtime_metadata; #[macro_use] mod windows_install; +mod catalog; +pub(crate) use catalog::KNOWN_ACP_RUNTIMES; pub use login_shell::{find_nvm_default_bin, login_shell_path}; pub(crate) use login_shell::{find_via_login_shell, refresh_login_shell_path}; #[cfg(test)] @@ -26,7 +28,10 @@ pub(crate) use presets::{ 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"; @@ -83,144 +88,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) @@ -375,7 +242,11 @@ pub fn effective_agent_command( } mod overrides; -pub use overrides::{apply_agent_command_update, create_time_agent_command_override}; +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`. @@ -1168,6 +1039,9 @@ fn discover_acp_runtime_phase1(runtime: &'static KnownAcpRuntime, force: bool) - model_env_var: runtime.model_env_var.map(str::to_string), provider_env_var: runtime.provider_env_var.map(str::to_string), thinking_env_var: runtime.thinking_env_var.map(str::to_string), + effort_canonical_values: runtime + .effort_normalization + .map(|norm| norm.canonical.iter().map(|s| s.to_string()).collect()), max_tokens_env_var: runtime.max_tokens_env_var.map(str::to_string), context_limit_env_var: runtime.context_limit_env_var.map(str::to_string), max_rounds_env_var: runtime.max_rounds_env_var.map(str::to_string), @@ -1308,6 +1182,7 @@ pub fn discover_acp_runtimes_from( model_env_var: None, provider_env_var: None, thinking_env_var: None, + effort_canonical_values: None, max_tokens_env_var: None, context_limit_env_var: None, max_rounds_env_var: None, 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..fecf792f214 --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/discovery/catalog.rs @@ -0,0 +1,156 @@ +//! 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, BUZZ_AGENT_EFFORT_VALUES, 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), + effort_accepted_values: None, // goose: validated via 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) + effort_accepted_values: None, // claude: adapter accepts any value over BUZZ_ACP_EFFORT_LEVEL + 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) + effort_accepted_values: None, // codex: adapter accepts any value over BUZZ_ACP_EFFORT_LEVEL + 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 + effort_accepted_values: Some(BUZZ_AGENT_EFFORT_VALUES), // buzz-agent: parse_thinking_effort's accepted set + 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..fa339a03b70 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/overrides.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/overrides.rs @@ -83,27 +83,85 @@ 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) { + let suppress = crate::managed_agents::config_bridge::effort::effort_suppress_keys(); + env_vars.retain(|k, _| { + !suppress + .iter() + .any(|suppressed| k.eq_ignore_ascii_case(suppressed)) + }); +} + +/// 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/presets.rs b/desktop/src-tauri/src/managed_agents/discovery/presets.rs index 61c589770ca..438cb5eb863 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/presets.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/presets.rs @@ -67,6 +67,7 @@ pub(super) fn preset_catalog_entry( model_env_var: None, provider_env_var: None, thinking_env_var: None, + effort_canonical_values: None, max_tokens_env_var: None, context_limit_env_var: None, max_rounds_env_var: None, 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..b68bc84c23f 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,69 @@ +/// 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"), + ], +}; + +/// buzz-agent's accepted persisted thinking-effort values — a validation-only +/// contract, NOT a canonicalization one. Unlike Goose, buzz-agent keeps `xhigh` +/// and `max` as *distinct* efforts, so these values are validated (invalid → +/// skip as absent) but never aliased or collapsed. +/// +/// Source of truth: `parse_thinking_effort`, `crates/buzz-agent/src/config.rs` +/// (`none|minimal|low|medium|high|xhigh|max`). A destination-vocabulary check +/// at projection time keeps a foreign canonical (e.g. Goose `off`) from being +/// emitted as `BUZZ_AGENT_THINKING_EFFORT=off`, which the parser rejects at +/// config init (child exits 2). +pub(crate) static BUZZ_AGENT_EFFORT_VALUES: &[&str] = + &["none", "minimal", "low", "medium", "high", "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 +113,35 @@ 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>, + /// Accepted persisted effort values for a runtime that has NO + /// canonicalization contract but still constrains its vocabulary + /// (buzz-agent: `parse_thinking_effort`'s accepted set). Used only for + /// destination-vocabulary validation at projection/read time — a candidate + /// outside this set is skipped as absent, so a foreign canonical (e.g. + /// Goose `off`) is never emitted under `thinking_env_var` where the + /// destination parser would reject it and crash the child. + /// + /// `None` means "no validation": Goose validates through + /// `effort_normalization`; Claude/Codex and unknown/custom runtimes accept + /// any string over the `BUZZ_ACP_EFFORT_LEVEL` transport. + pub effort_accepted_values: Option<&'static [&'static str]>, /// 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 577a780d6ca..dc155d82f5b 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/tests.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/tests.rs @@ -2,12 +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, 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, + 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, + 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, + 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; @@ -606,51 +607,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 forced_discovery; 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/mod.rs b/desktop/src-tauri/src/managed_agents/mod.rs index ad112f9ff90..392059dfb6c 100644 --- a/desktop/src-tauri/src/managed_agents/mod.rs +++ b/desktop/src-tauri/src/managed_agents/mod.rs @@ -48,13 +48,27 @@ pub(crate) use team_repair::team_persona_key; mod teams; mod types; -// Shared guard for tests that mutate or read process-global PATH. +// Shared lock for tests that call `lock_path_mutex` or `lock_env_mutex`. +// Both helpers delegate here so any two tests using either helper are mutually +// exclusive with each other. Tests in other modules that maintain their own +// independent locks (app_state_tests, agent_config_tests, reader_tests) are +// NOT in this domain and are not covered by this mutex. #[cfg(test)] -static PATH_MUTEX: std::sync::Mutex<()> = std::sync::Mutex::new(()); +static PROCESS_ENV_MUTEX: std::sync::Mutex<()> = std::sync::Mutex::new(()); +// Acquires the shared process-env lock. Call from any test in this module that +// reads, writes, or removes a process-global environment variable (including PATH). #[cfg(test)] pub(crate) fn lock_path_mutex() -> std::sync::MutexGuard<'static, ()> { - PATH_MUTEX.lock().unwrap_or_else(|e| e.into_inner()) + PROCESS_ENV_MUTEX.lock().unwrap_or_else(|e| e.into_inner()) +} + +// Delegates to the same lock as `lock_path_mutex`. Tests using either helper +// are mutually exclusive with each other; PATH and env-key mutations that go +// through these helpers cannot race. +#[cfg(test)] +pub(crate) fn lock_env_mutex() -> std::sync::MutexGuard<'static, ()> { + PROCESS_ENV_MUTEX.lock().unwrap_or_else(|e| e.into_inner()) } pub use backend::*; diff --git a/desktop/src-tauri/src/managed_agents/readiness.rs b/desktop/src-tauri/src/managed_agents/readiness.rs index 9af3c989f49..88cc7884c41 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,8 @@ mod tests { default_env: &[], supports_acp_native_config: false, thinking_env_var: None, + effort_normalization: None, + effort_accepted_values: None, max_tokens_env_var: None, context_limit_env_var: None, max_rounds_env_var: None, @@ -1241,6 +1257,8 @@ mod tests { default_env: &[], supports_acp_native_config: false, thinking_env_var: None, + effort_normalization: None, + effort_accepted_values: None, max_tokens_env_var: None, context_limit_env_var: None, max_rounds_env_var: None, @@ -1681,56 +1699,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 de1bf97b4fd..b10271ea1c7 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -2,7 +2,7 @@ use std::collections::HashMap; use tauri::{AppHandle, Manager}; -use super::agent_env::{build_buzz_agent_provider_defaults, idle_pool_sleep_env}; +use super::agent_env::idle_pool_sleep_env; use crate::{ managed_agents::{ @@ -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}; @@ -225,23 +225,14 @@ pub fn build_managed_agent_summary( } }; - // Restart badge: the running process stamped the effective spawn config - // it was launched with; recompute a prospective one from current disk - // state and report every differing field. Only the tracked live pair for - // THIS workspace can drift — stopped agents spawn fresh, adopted - // (runtime_pid-only) processes have no stamp to compare, and pairs running - // for other communities are judged in their own community (comparing them - // against this workspace's relay would flag a spurious restart on every - // community switch). - // - // Adapter-availability drift (codex only) contributes its own synthetic - // entry, so an out-of-band adapter change (manual npm install/downgrade) - // that Phase-1 auto-restart doesn't cover still shows the user what moved. - // The cache is read-only here — no subprocess is spawned. - // - // Global config drives both the prospective snapshot and the descriptor - // env layering below — the caller loads it once and passes it in, so - // list-style callers pay one disk read per call rather than one per record. + // Restart badge: the running process stamped its effective spawn config; + // recompute a prospective one from current disk state and report every + // differing field. Only the tracked live pair for THIS workspace can drift + // (stopped agents spawn fresh; adopted processes have no stamp; other- + // community pairs are judged in their own community). Adapter drift + // (codex only) contributes a synthetic entry for out-of-band npm changes. + // Global config drives both snapshot and descriptor env layering; the + // caller loads it once so list callers pay one disk read per call. // The prospective side is computed only for a tracked pair: an unstamped // agent has nothing to compare against. @@ -397,6 +388,47 @@ pub(crate) fn configure_runtime_cli( } } +/// Proof token for the effort-application outer binding. `#[must_use]`; +/// makes `let effort = apply_effort_to_spawn_command(…)` a compile-time +/// requirement — deleting the binding is a compile error because +/// `spawn_with_effort_proof` consumes it by value. +/// +/// The private field prevents any crate-local code from constructing +/// `EffortApplied` directly (same shape as `RecordFieldsApplied(())`), so +/// the only way to obtain a token is to call `apply_effort_to_spawn_command`. +#[must_use] +pub(crate) struct EffortApplied(()); + +/// Apply effort env to an agent spawn command. Called by `spawn_agent_child` +/// (production) and `effort_cmd_tests` (test seam). Inner-seam: removing +/// `apply_spawn_effort_env` below turns the production-sequence tests RED. +/// Outer-seam: the returned token is consumed by `spawn_with_effort_proof`; +/// deleting this call leaves `effort` undefined at the spawn site. +pub(crate) fn apply_effort_to_spawn_command( + cmd: &mut std::process::Command, + record: &crate::managed_agents::types::ManagedAgentRecord, + runtime: Option<&crate::managed_agents::discovery::KnownAcpRuntime>, + personas: &[crate::managed_agents::types::AgentDefinition], + persona_id: Option<&str>, + global_env: &std::collections::BTreeMap, + baked_env: &std::collections::BTreeMap, +) -> EffortApplied { + super::config_bridge::effort::apply_spawn_effort_env( + cmd, record, runtime, personas, persona_id, global_env, baked_env, + ); + EffortApplied(()) +} + +/// Spawn the agent command, consuming the `EffortApplied` proof token. +/// Deleting `apply_effort_to_spawn_command` from `spawn_agent_child` leaves +/// `effort` undefined here — a compile error CI catches before any test runs. +pub(crate) fn spawn_with_effort_proof( + cmd: &mut std::process::Command, + _effort: EffortApplied, +) -> std::io::Result { + cmd.spawn() +} + /// Spawn an agent process without holding any locks on records or runtimes. /// Returns the child process and log path on success. The caller is responsible /// for updating `ManagedAgentRecord` fields and inserting into the runtimes map. @@ -552,23 +584,16 @@ pub fn spawn_agent_child( // ── Readiness check: set setup-payload if agent is not ready ───────────── // - // Build the effective env the agent would have at start-time, run the - // readiness predicate, and if anything is missing, serialize the payload - // into BUZZ_ACP_SETUP_PAYLOAD. buzz-acp detects this env var on startup - // and enters the minimal setup-listener mode instead of the agent pool. + // Build the effective env, run the readiness predicate, and serialize any + // missing requirements into BUZZ_ACP_SETUP_PAYLOAD. buzz-acp enters + // setup-listener mode when this env var is present. // - // SECURITY: BUZZ_ACP_SETUP_PAYLOAD is in RESERVED_ENV_KEYS so user env - // cannot set it, but we also explicitly remove it after writing user env - // to guard against the parent-process environment. We then set it only - // when desktop has computed NotReady — the desktop is the sole readiness - // source and buzz-acp only transports the payload. + // SECURITY: BUZZ_ACP_SETUP_PAYLOAD is in RESERVED_ENV_KEYS (user env cannot + // set it). We also remove it after writing user env as a parent-process guard, + // then set it only when desktop computes NotReady — desktop is the sole source. // - // The JSON format mirrors `setup_mode::SetupPayload` in buzz-acp: - // { "agent_name": "...", "agent_pubkey": "...", "requirements": [{ "surface": "...", ... }] } - // - // `spawned_setup_mode` is captured outside the block so it can be stamped - // on `ManagedAgentProcess` — used by `install_acp_runtime` to target only - // stuck agents for auto-restart. + // `spawned_setup_mode` is captured outside the block to stamp + // `ManagedAgentProcess` (used by `install_acp_runtime` for auto-restart). let spawned_setup_mode; { use crate::managed_agents::readiness::EffectiveAgentEnv; @@ -741,7 +766,18 @@ pub fn spawn_agent_child( &mut command, resolve_session_title(record.display_name.as_deref(), &record.name), ); - build_buzz_agent_provider_defaults(&mut command); + // Strip all known effort keys and emit exactly one projected key. Command + // inherits the parent env — the returned EffortApplied token is consumed + // by spawn_with_effort_proof below; deleting this call is a compile error. + let effort = apply_effort_to_spawn_command( + &mut command, + record, + runtime_meta, + &personas, + record.persona_id.as_deref(), + &global.env_vars, + &super::agent_env::baked_build_env(), + ); if let Some(meta) = runtime_meta { for (key, value) in runtime_metadata_env_vars( meta.model_env_var, @@ -812,14 +848,6 @@ pub fn spawn_agent_child( let acp_session_policy = super::apply_app_acp_session_policy_env(app, &mut command); crate::build_identity::apply_demo_config_home(&mut command)?; - // 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()); - // 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 // would be ambiguous). @@ -849,10 +877,8 @@ pub fn spawn_agent_child( .env("BUZZ_MANAGED_AGENT", current_instance_id(app)) .env("BUZZ_MANAGED_AGENT_START_NONCE", &start_nonce); - // Stamp the effective spawn config from the values that populated the - // `Command` above, BEFORE spawning. Re-resolving after `spawn()` would let - // a persona/harness/global edit landing in between stamp the NEW config - // onto a child running the OLD one, silently suppressing the badge. + // Stamp spawn config from values above, BEFORE spawning — a post-spawn + // re-resolve races config edits and would stamp the wrong values. let spawn_config = super::spawn_snapshot::SpawnConfigSnapshot::from_inputs( super::spawn_snapshot::SpawnConfigInputs { record, @@ -867,8 +893,7 @@ pub fn spawn_agent_child( }, ); - // Spawn the harness in its own process group so we can kill the entire - // tree (harness + MCP servers + agent subprocesses) on shutdown. + // Spawn in its own process group (Unix) or with CREATE_NO_WINDOW (Windows). #[cfg(unix)] { use std::os::unix::process::CommandExt; @@ -884,7 +909,7 @@ pub fn spawn_agent_child( command.creation_flags(CREATE_NO_WINDOW); } - let child = command.spawn().map_err(|error| { + let child = spawn_with_effort_proof(&mut command, effort).map_err(|error| { format!( "failed to spawn `{}` for agent {}: {error}", resolved_acp_command.display(), @@ -892,14 +917,8 @@ pub fn spawn_agent_child( ) })?; - // Stamp the adapter availability for runtimes with a version gate (codex - // only). The summary builder compares this against the current cached value - // to detect out-of-band adapter changes after spawn (Phase-2 badge fallback). - // Non-codex runtimes get `None` — nothing changes for them. - // When the cache is cold (e.g. Doctor just installed and cleared the cache), - // `adapter_availability_cached()` returns `None`, so the stamp is `None` and - // the drift check is skipped until discovery warms the cache — preventing a - // false restart badge immediately after auto-restart. + // Codex: stamp adapter availability for the Phase-2 badge drift check. + // Cold cache returns `None` → drift check skipped until discovery warms it. let spawned_adapter_availability = if runtime_meta.is_some_and(|r| r.id == "codex") { super::adapter_availability_cached() } else { diff --git a/desktop/src-tauri/src/managed_agents/spawn_snapshot.rs b/desktop/src-tauri/src/managed_agents/spawn_snapshot.rs index c201c0a8a55..810ad439f29 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, @@ -134,13 +133,16 @@ 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 the effective effort in the + /// snapshot: the projection's destination 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 the + /// projection consumed surfaces as exactly one `effort_level` entry. For an + /// unknown/custom runtime the projection consumes nothing beyond the + /// sentinel, so any other effort-looking key the child receives stays in + /// `env` as ordinary state and diffs normally. pub effort_level: Option, /// The effective ACP session policy this launch applies (`channel` or /// `thread`). The harness reads `BUZZ_ACP_SESSION_POLICY` only at launch, so @@ -152,20 +154,28 @@ pub(crate) struct SpawnConfigSnapshot { pub session_policy: String, } -/// 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); + // Read case-insensitively (exact-first) so a mixed-case sentinel a custom + // runtime passed through (the projection uses an EMPTY suppress set, so a + // user-set `buzz_acp_effort_level` survives into `descriptor.env` and the + // child reads it as `BUZZ_ACP_EFFORT_LEVEL` on Windows) is captured here. + // The read must match the snapshot strip, which is also case-insensitive: + // if the read were exact-case it would miss the mixed-case sentinel, the + // strip would still remove it, and the value would land in neither + // `snapshot.env` nor `effort_level` — producing no restart diff on an edit. + super::config_bridge::effort::get_ci(&descriptor.env, dest_key).cloned() } impl SpawnConfigSnapshot { @@ -193,14 +203,27 @@ 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 - // `effort_level` entry rather than a duplicate under `env.`. + // below, always holding the projected effective value. The keys + // stripped here mirror EXACTLY what the launch projection suppressed + // for this runtime (`snapshot_suppress_keys`): a known runtime swept + // every effort key to its single destination key, so the full set is + // stripped (a no-op beyond that dest key); an unknown/custom runtime + // used an empty suppress set (external-review-#2 pass-through), so + // only the ACP-startup sentinel is stripped and every other + // effort-looking key the child actually receives (e.g. a hand-rolled + // `GOOSE_THINKING_EFFORT`) stays as ordinary env — an edit to it must + // diff the snapshot and fire the restart badge. Stripping is + // ASCII-case-insensitive to match the projection's `apply`. env: { let mut env = descriptor.env.clone(); - env.remove(EFFORT_LEVEL_ENV_VAR); + let suppress = super::config_bridge::effort::snapshot_suppress_keys( + known_acp_runtime(&descriptor.command), + ); + env.retain(|k, _| { + !suppress + .iter() + .any(|suppressed| k.eq_ignore_ascii_case(suppressed)) + }); env }, relay_url: relay_url.to_string(), @@ -230,10 +253,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), session_policy: session_policy.as_str().to_string(), } } 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 03bba90cff9..b5ee8d45224 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()), @@ -188,6 +227,163 @@ fn canonical_effort_edit_changes_snapshot() { ); } +/// A custom-command record whose runtime matches no known ACP runtime, so the +/// launch projection suppresses ONLY its own ACP sentinel (external-review-#2 +/// pass-through, r5): every foreign effort key survives untouched and the child +/// receives its raw effort env. +fn custom_command_record() -> ManagedAgentRecord { + let mut rec = record(); + rec.agent_command_override = Some("/opt/custom/my-agent".into()); + rec +} + +#[test] +fn custom_runtime_effort_env_stays_in_snapshot_and_diffs() { + // Regression (external review, Carl): for an unknown/custom runtime the + // launch projection strips only its own ACP sentinel, so the child receives + // the raw `GOOSE_THINKING_EFFORT` from the wrapper's env. The snapshot must + // retain that key as ordinary env — the projection consumed nothing into + // `effort_level` (its dest key, the ACP sentinel, is absent) — so an edit to + // it diffs the snapshot and fires the restart badge. The prior full strip + // erased the key from both places, producing NO restart diff on an effort + // edit and leaving the running agent on stale effort. + let mut high = custom_command_record(); + high.env_vars + .insert("GOOSE_THINKING_EFFORT".into(), "high".into()); + let canonical = snap(&high); + assert_eq!( + canonical + .get("env") + .and_then(|env| env.get("GOOSE_THINKING_EFFORT")) + .and_then(|v| v.as_str()), + Some("high"), + "a custom runtime's effort env must remain in the snapshot as ordinary env" + ); + assert_eq!( + canonical.get("effort_level").and_then(|v| v.as_str()), + None, + "the custom sentinel dest key is absent, so effort_level captures nothing" + ); + + let mut low = custom_command_record(); + low.env_vars + .insert("GOOSE_THINKING_EFFORT".into(), "low".into()); + assert_ne!( + snap(&low), + canonical, + "editing a custom runtime's effort env must trip the restart badge" + ); +} + +#[test] +fn known_runtime_still_strips_native_effort_env_from_snapshot() { + // The counter-case pinning the scoping: for a KNOWN runtime the full sweep + // still applies, so `GOOSE_THINKING_EFFORT` reaches the snapshot only as the + // single `effort_level` field — never as a phantom `env` entry alongside it. + let canonical = snap(&record_with_env_effort("high")); + assert_eq!( + effort_env_leaf(&canonical), + None, + "a known runtime must still strip its native effort key from the snapshot env" + ); + assert_eq!( + canonical.get("effort_level").and_then(|v| v.as_str()), + Some("high"), + "the known runtime's effort must land solely in the effort_level field" + ); +} + +#[test] +fn custom_runtime_mixed_case_sentinel_is_captured_not_lost() { + // Regression (external review, Carl, P2): for an unknown/custom runtime a + // user-set mixed-case `buzz_acp_effort_level` (no column) must not vanish. + // The launch projection now reconciles it — stripping the mixed-case + // spelling and re-emitting the pass-through value under the canonical + // `BUZZ_ACP_EFFORT_LEVEL` (see `effort_tests:: + // unknown_runtime_collapses_mixed_case_sentinel_to_canonical_when_no_column`) + // — so `descriptor.env` carries exactly one canonical sentinel. The snapshot + // captures it into `effort_level` and strips it from `env`. Before the r4/r5 + // fixes the mixed-case key survived while the exact-case read missed it, so + // the value vanished from BOTH fields and an edit produced no restart diff. + let mut high = custom_command_record(); + high.env_vars + .insert("buzz_acp_effort_level".into(), "high".into()); + let canonical = snap(&high); + assert_eq!( + canonical.get("effort_level").and_then(|v| v.as_str()), + Some("high"), + "a mixed-case pass-through sentinel must be captured into effort_level" + ); + assert_eq!( + canonical + .get("env") + .and_then(|env| env.get("buzz_acp_effort_level")), + None, + "the sentinel is the projection's dest key and is stripped from env once represented" + ); + + // The mutation pin: editing the mixed-case sentinel must trip the badge. + // Reverting the fix (exact-case read + case-insensitive strip, or an empty + // unknown-runtime suppress set) makes both snapshots carry + // `effort_level = null` with the key stripped, so they compare equal and + // this assertion fails. + let mut low = custom_command_record(); + low.env_vars + .insert("buzz_acp_effort_level".into(), "low".into()); + assert_ne!( + snap(&low), + canonical, + "editing a mixed-case custom-runtime sentinel must trip the restart badge" + ); +} + +#[test] +fn custom_runtime_canonical_column_wins_over_mixed_case_sentinel() { + // The with-canonical-column collision case Carl asked for, verified at the + // SNAPSHOT here and — decisively — at the projection/descriptor seam in + // `effort_tests::unknown_runtime_column_wins_over_mixed_case_sentinel`. The + // projection strips every case variant of the sentinel before emitting the + // column value, so `descriptor.env` carries exactly `BUZZ_ACP_EFFORT_LEVEL= + // ` and the child receives the column value on every platform (no + // lowercase variant survives for Windows to case-fold over the canonical + // key). This snapshot therefore reads the same truth the child gets: the + // column wins `effort_level` and both case variants are absent from `env`. + let mut high_col = custom_command_record(); + high_col.effort_level = Some("high".into()); + high_col + .env_vars + .insert("buzz_acp_effort_level".into(), "low".into()); + let canonical = snap(&high_col); + assert_eq!( + canonical.get("effort_level").and_then(|v| v.as_str()), + Some("high"), + "the canonical column wins effort_level over the pass-through sentinel" + ); + let env = canonical.get("env").expect("snapshot has an env object"); + assert_eq!( + env.get("BUZZ_ACP_EFFORT_LEVEL"), + None, + "the projection-emitted canonical sentinel is stripped from env" + ); + assert_eq!( + env.get("buzz_acp_effort_level"), + None, + "the user's mixed-case sentinel duplicate is stripped case-insensitively" + ); + + // Editing the authority (the column) still trips the badge. + let mut low_col = custom_command_record(); + low_col.effort_level = Some("low".into()); + low_col + .env_vars + .insert("buzz_acp_effort_level".into(), "low".into()); + assert_ne!( + snap(&low_col), + canonical, + "editing the canonical column must trip the restart badge" + ); +} + use crate::managed_agents::spawn_snapshot::{ eligible_restart_diff, prospective_spawn_config_snapshot, RestartDiffEntry, SpawnConfigSnapshot, TrackedSpawnState, diff --git a/desktop/src-tauri/src/managed_agents/types.rs b/desktop/src-tauri/src/managed_agents/types.rs index b3c9d4b53ca..2620f0337fc 100644 --- a/desktop/src-tauri/src/managed_agents/types.rs +++ b/desktop/src-tauri/src/managed_agents/types.rs @@ -466,8 +466,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, } @@ -656,6 +662,18 @@ pub struct AcpRuntimeCatalogEntry { pub provider_env_var: Option, /// Environment variable used to apply thinking effort, when supported. pub thinking_env_var: Option, + /// Canonical accepted effort values for this runtime, in display order. + /// Serialized from `KnownAcpRuntime::effort_normalization.canonical` for + /// runtimes with a static finite vocabulary (e.g. Goose). `None` for + /// runtimes with no canonicalization contract (buzz-agent uses a + /// provider/model catalog; Claude/Codex/unknown runtimes accept any string). + /// + /// The renderer uses this to drive choices and validation, replacing the + /// TS-side `GOOSE_EFFORT_CANONICAL_VALUES` duplicate. When non-null, the + /// `harnessNative` effort field uses this list exclusively — `off` and all + /// other valid Goose values are always present when this is Goose, so + /// `useEffortAutoClear` never incorrectly deletes a valid saved value. + pub effort_canonical_values: Option>, pub max_tokens_env_var: Option, pub context_limit_env_var: Option, pub max_rounds_env_var: Option, diff --git a/desktop/src-tauri/src/managed_agents/types/requests.rs b/desktop/src-tauri/src/managed_agents/types/requests.rs index a7b379ac838..824ca4ccf3a 100644 --- a/desktop/src-tauri/src/managed_agents/types/requests.rs +++ b/desktop/src-tauri/src/managed_agents/types/requests.rs @@ -260,6 +260,16 @@ pub struct UpdateManagedAgentRequest { /// normalized server-side). #[serde(default)] pub respond_to_allowlist: Option>, + /// Absent = don't touch. `null` = clear the canonical effort column + /// (revert to inherited default). `"value"` = set the column. + /// + /// When present, persisted inside the locked update/restart transaction + /// so that an access-policy-change restart snapshots and launches the new + /// effort value rather than the old one. Uses the same + /// `apply_picker_effort_level` logic (via `apply_effort_update`) so + /// the record-scope alias sweep runs atomically with the column write. + #[serde(default, deserialize_with = "crate::util::double_option")] + pub effort_level: Option>, } #[cfg(test)] diff --git a/desktop/src/features/agents/AGENTS.md b/desktop/src/features/agents/AGENTS.md index 64509387feb..39f3a3bdef7 100644 --- a/desktop/src/features/agents/AGENTS.md +++ b/desktop/src/features/agents/AGENTS.md @@ -204,21 +204,22 @@ with a TypeScript lookup table or an id comparison in a component. 14. **Thinking effort has two surfaces: a local-only WRITE control and a read-only two-facts DISPLAY.** The write control is `EffortPickerField` (`ui/EffortPickerField.tsx`), a self-contained section component mounted in - `AgentInstanceEditDialog` beside the Model block. It is direct-write, not - part of the frozen `UpdateManagedAgentInput` shape: each selection calls - `persistAgentEffortLevel` and invalidates the config-surface query, mirroring - the `setManagedAgentAutoRestart` standalone-setter precedent. Its gating and - option compute live in the pure helper `ui/effortPicker.ts` - (`effortPickerState`): the picker renders only when - `agent.backend.type === "local"` **AND** a `thought_level` `effortConfigId` - has been discovered from the running session (absent pre-first-session and - for runtimes/models without effort support). Local-only is load-bearing, not - cosmetic — the Rust command rejects non-local backends because remote effort - is set at deploy time via `policy_env`. Because it reads its inputs from the - config surface the dialog already fetches (`useAgentConfigSurface`) and owns - its own mutation, it does **not** thread new props through the dialog (see - rule 11): keep effort state inside the section component, never as - dialog-level props. The read-only display is the `thinkingEffort` + `AgentInstanceEditDialog` beside the Model block. It is **Save-gated, not + direct-write**: the control is fully controlled by the parent dialog + (`value`/`onChange`) and owns no mutation. The dialog persists the selection + by embedding `effortLevel` in the locked `update_managed_agent` IPC call, so + the effort write is atomic with any access-policy change and can never race + or survive a Cancel or failed Save. There is no standalone + `persistAgentEffortLevel` setter. Its gating and option compute live in the + pure helper `ui/effortPicker.ts` (`effortPickerState`): the picker renders + only when `agent.backend.type === "local"` **AND** a `thought_level` + `effortConfigId` has been discovered from the running session (absent + pre-first-session and for runtimes/models without effort support). Local-only + is load-bearing, not cosmetic — the Rust command rejects non-local backends + because remote effort is set at deploy time via `policy_env`. Because the + control reads its inputs from the config surface the dialog already fetches + (`useAgentConfigSurface`), it integrates into the dialog's existing field + group without additional IPC. 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, diff --git a/desktop/src/features/agents/lib/agentConfigCore.test.mjs b/desktop/src/features/agents/lib/agentConfigCore.test.mjs index 92159ff2754..520b51b99ea 100644 --- a/desktop/src/features/agents/lib/agentConfigCore.test.mjs +++ b/desktop/src/features/agents/lib/agentConfigCore.test.mjs @@ -79,20 +79,93 @@ test("Goose exposes provider, model, and its real effort application key", () => scope: "global", }); - assert.equal( - field(model, "effort").optionSource, - "legacyProviderModelCatalog", - ); + assert.equal(field(model, "effort").optionSource, "harnessNative"); assert.deepEqual(field(model, "effort").currentPersistence, { kind: "envVar", - key: "BUZZ_AGENT_THINKING_EFFORT", + key: "GOOSE_THINKING_EFFORT", }); assert.deepEqual(field(model, "effort").targetApplication, { kind: "envVar", key: "GOOSE_THINKING_EFFORT", }); + // Goose reads/writes its native key at global scope — the launch projection's + // global tier is native-only, so the legacy BUZZ_AGENT_THINKING_EFFORT in the + // config is not surfaced as the effort value (it would be silently ignored). + assert.equal(field(model, "effort").value, null); }); +// Carl (review 5036131024): global/onboarding effort persistence must use the +// runtime's native key so a selection reaches the spawn. The launch projection's +// global tier reads native-only (legacy alias is record/persona-scope), so +// persisting the legacy key for Goose round-trips in the UI but is ignored at +// spawn. Both scopes derive the same persistence/application key. +for (const scope of ["global", "onboarding"]) { + test(`effort persists to the runtime native key at ${scope} scope`, () => { + const goose = deriveAgentConfigFieldModel({ + config: { ...config, env_vars: { GOOSE_THINKING_EFFORT: "high" } }, + runtime: runtime("goose", { thinkingEnvVar: "GOOSE_THINKING_EFFORT" }), + scope, + }); + const gooseEffort = field(goose, "effort"); + assert.deepEqual(gooseEffort.currentPersistence, { + kind: "envVar", + key: "GOOSE_THINKING_EFFORT", + }); + assert.deepEqual(gooseEffort.targetApplication, { + kind: "envVar", + key: "GOOSE_THINKING_EFFORT", + }); + assert.equal(gooseEffort.value, "high"); + assert.deepEqual(structuredEnvKeys([gooseEffort]), [ + "GOOSE_THINKING_EFFORT", + ]); + + const buzz = deriveAgentConfigFieldModel({ + config, + runtime: runtime("buzz-agent", { + thinkingEnvVar: "BUZZ_AGENT_THINKING_EFFORT", + }), + scope, + }); + const buzzEffort = field(buzz, "effort"); + assert.deepEqual(buzzEffort.currentPersistence, { + kind: "envVar", + key: "BUZZ_AGENT_THINKING_EFFORT", + }); + assert.equal(buzzEffort.value, "high"); + }); +} + +// Per-agent scopes (definition/instance) intentionally keep effort on the +// generic legacy BUZZ_AGENT_THINKING_EFFORT row until PR 2.7 migrates Goose — +// currentPersistence/value stay legacy while targetApplication is native +// (agents/AGENTS.md rule 2). The scope gate must not broaden to these scopes. +for (const scope of ["definition", "instance"]) { + test(`Goose effort stays on the legacy persistence key at ${scope} scope`, () => { + const model = deriveAgentConfigFieldModel({ + config: { + ...config, + env_vars: { + BUZZ_AGENT_THINKING_EFFORT: "high", + GOOSE_THINKING_EFFORT: "low", + }, + }, + runtime: runtime("goose", { thinkingEnvVar: "GOOSE_THINKING_EFFORT" }), + scope, + }); + const effort = field(model, "effort"); + assert.deepEqual(effort.currentPersistence, { + kind: "envVar", + key: "BUZZ_AGENT_THINKING_EFFORT", + }); + assert.deepEqual(effort.targetApplication, { + kind: "envVar", + key: "GOOSE_THINKING_EFFORT", + }); + assert.equal(effort.value, "high"); + }); +} + test("Claude models effort as a deferred native ACP option", () => { const model = deriveAgentConfigFieldModel({ config, @@ -561,3 +634,90 @@ test("NUMERIC_KIND_MIN_contextLimit_is_1", () => { test("NUMERIC_KIND_MIN_maxRounds_is_0", () => { assert.equal(NUMERIC_KIND_MIN.maxRounds, 0); }); + +// ── P2 regression: Goose optionSource + isHarnessNativeEffort guard ─────────── +// +// Source-level reproduction of the P2 blocker: save global Goose defaults with +// GOOSE_THINKING_EFFORT=off, then open AI defaults. Previously, optionSource +// was "legacyProviderModelCatalog" → AgentConfigFields passed the persisted key +// to useEffortAutoClear with buzz-agent provider/model vocab → "off" not in +// that list → hook deleted the valid native value on mount. Fix: emit +// "harnessNative" so AgentConfigFields can detect isHarnessNativeEffort and +// make the hook a no-op. + +test("Goose_optionSource_is_harnessNative_not_legacyProviderModelCatalog", () => { + // The sole optionSource change (P2 fix): Goose must NOT be + // "legacyProviderModelCatalog" because that routes effort into the + // buzz-agent provider/model catalog, deleting valid Goose values on mount. + const model = deriveAgentConfigFieldModel({ + config, + runtime: runtime("goose", { thinkingEnvVar: "GOOSE_THINKING_EFFORT" }), + scope: "global", + }); + const effortField = field(model, "effort"); + assert.equal( + effortField.optionSource, + "harnessNative", + 'Goose global optionSource must be "harnessNative" — "legacyProviderModelCatalog" routes to buzz-agent vocab and deletes valid `off` on mount', + ); +}); + +test("Goose_global_off_value_is_preserved_by_harnessNative_optionSource", () => { + // A saved GOOSE_THINKING_EFFORT=off must round-trip through the field model + // without deletion. The field value reflects the config value, and + // optionSource="harnessNative" signals to AgentConfigFields that the + // auto-clear hook should be a no-op (no buzz-agent vocab gate). + const savedConfig = { + ...config, + env_vars: { GOOSE_THINKING_EFFORT: "off" }, + }; + const model = deriveAgentConfigFieldModel({ + config: savedConfig, + runtime: runtime("goose", { thinkingEnvVar: "GOOSE_THINKING_EFFORT" }), + scope: "global", + }); + const effortField = field(model, "effort"); + assert.equal( + effortField.optionSource, + "harnessNative", + "Goose effort field must use harnessNative optionSource", + ); + assert.equal( + effortField.value, + "off", + "saved GOOSE_THINKING_EFFORT=off must survive round-trip through field model (not deleted by buzz-agent vocab check)", + ); +}); + +test("Goose_onboarding_optionSource_is_harnessNative", () => { + // Same contract at onboarding scope — the persistence key is the native key + // at both global and onboarding, so both must guard against buzz-agent vocab. + const model = deriveAgentConfigFieldModel({ + config, + runtime: runtime("goose", { thinkingEnvVar: "GOOSE_THINKING_EFFORT" }), + scope: "onboarding", + }); + assert.equal( + field(model, "effort").optionSource, + "harnessNative", + "Goose onboarding optionSource must also be harnessNative", + ); +}); + +test("buzz_agent_optionSource_unchanged_still_buzzAgentCatalog", () => { + // Ensure the fix did not accidentally change buzz-agent's optionSource. + // buzz-agent's effort MUST go through the provider/model catalog for the + // per-provider effort validation to work (e.g. "none" vs "off"). + const model = deriveAgentConfigFieldModel({ + config, + runtime: runtime("buzz-agent", { + thinkingEnvVar: "BUZZ_AGENT_THINKING_EFFORT", + }), + scope: "global", + }); + assert.equal( + field(model, "effort").optionSource, + "buzzAgentCatalog", + "buzz-agent optionSource must remain buzzAgentCatalog", + ); +}); diff --git a/desktop/src/features/agents/lib/agentConfigCore.ts b/desktop/src/features/agents/lib/agentConfigCore.ts index 5a8b8cb1c37..de31e724cc1 100644 --- a/desktop/src/features/agents/lib/agentConfigCore.ts +++ b/desktop/src/features/agents/lib/agentConfigCore.ts @@ -204,19 +204,31 @@ export function deriveAgentConfigFieldModel({ }); if (runtime?.thinkingEnvVar) { + // targetApplication is always the runtime's native key — how the harness + // should receive effort. currentPersistence (where the value lives today) + // is scope-split until PR 2.7 migrates per-agent Goose/Claude: + // - global/onboarding: native key, matching the launch projection's global + // tier (native-only; the legacy alias is record/persona scope), so a + // selection actually reaches the spawn rather than persisting a key the + // projection ignores. For buzz-agent this IS BUZZ_AGENT_THINKING_EFFORT. + // - definition/instance: still the generic legacy BUZZ_AGENT_THINKING_EFFORT + // row, unchanged pending the per-agent migration. + const nativeKey = runtime.thinkingEnvVar; + const persistenceKey = + scope === "global" || scope === "onboarding" + ? nativeKey + : BUZZ_AGENT_THINKING_EFFORT; fields.push({ kind: "effort", optionSource: - runtime.id === "buzz-agent" - ? "buzzAgentCatalog" - : "legacyProviderModelCatalog", + runtime.id === "buzz-agent" ? "buzzAgentCatalog" : "harnessNative", currentPersistence: { kind: "envVar", - key: BUZZ_AGENT_THINKING_EFFORT, + key: persistenceKey, }, - targetApplication: { kind: "envVar", key: runtime.thinkingEnvVar }, + targetApplication: { kind: "envVar", key: nativeKey }, render: "control", - value: valueFromEnv(config, BUZZ_AGENT_THINKING_EFFORT), + value: valueFromEnv(config, persistenceKey), }); } else if (runtime?.id === "claude") { fields.push({ diff --git a/desktop/src/features/agents/ui/AgentConfigFields.tsx b/desktop/src/features/agents/ui/AgentConfigFields.tsx index 52a58c91343..5e0a4ab9613 100644 --- a/desktop/src/features/agents/ui/AgentConfigFields.tsx +++ b/desktop/src/features/agents/ui/AgentConfigFields.tsx @@ -86,14 +86,10 @@ type AgentConfigDisclosure = | "onboarding-essential" | "progressive-defaults"; -// Canonical behaviors (PR 2 flag cleanup). These were per-surface props; -// onboarding's values won every call and are now the only behavior: -// - auto-select a valid model when the provider changes -// - keep the model select usable during discovery -// - preserve credential env vars across provider switches (the abandoned -// provider's key stays in env_vars — visible/deletable under Advanced) -// - require a provider before model/effort are editable (no saveable -// invalid state — design principle #4) +// Canonical behaviors (formerly per-surface props; onboarding's values won +// every call and are now the only behavior). Design principle #4: require a +// provider before model/effort are editable; preserve credential env vars +// across provider switches; auto-select model on provider change. const autoSelectModelOnProviderChange = true; const disableModelSelectDuringDiscovery = false; const preserveCredentialEnvVarsOnProviderChange = true; @@ -255,6 +251,11 @@ export function AgentConfigFields({ effortField?.currentPersistence.kind === "envVar" ? effortField.currentPersistence.key : null; + // True when the runtime owns its own effort vocabulary (e.g. Goose) and + // should bypass the buzz-agent provider/model catalog: harnessNative + envVar. + const isHarnessNativeEffort = + effortField?.optionSource === "harnessNative" && + effortField?.currentPersistence.kind === "envVar"; const numericDescriptors = fieldModel.fields.filter( (d): d is NumericDescriptor => @@ -287,8 +288,7 @@ export function AgentConfigFields({ const modelField = fieldModel.fields.find( (field) => field.kind === "model" && field.render === "control", ); - // CLI-login harnesses apply this setting through ACP rather than an env var - // and provide their own default when no model override is persisted. + // CLI-login harnesses use ACP for this setting; they provide their own default. const modelIsOptional = modelField?.targetApplication.kind === "acpNative"; const modelIsValid = modelIsOptional || @@ -384,20 +384,15 @@ export function AgentConfigFields({ showCustomModelOption, }); - // Mount-time healing policy: onboarding page 4 edits the root config during - // first-run (no higher layers to inherit from), so acting on open is safe - // and intentional there — it heals stale state and picks a valid model. - // Evergreen surfaces (Settings, dialogs) edit saved data that may pair with - // higher layers (see PR #2148 review thread), so they only act after the - // user explicitly edits the provider in this session. + // Mount-time healing policy: onboarding (first-run, no higher layers) heals + // on open. Evergreen surfaces (Settings, dialogs) only heal after an explicit + // provider edit — acting on open would break multi-layer configs (PR #2148). const healOnMount = fieldModel.dependentValuePolicy.onCatalogMismatch === "onboardingCleanup"; const userEditedProviderRef = React.useRef(false); - // Advanced visibility is user-controlled. Provider changes can add required - // rows, but must not open this section without an explicit toggle click. + // Advanced visibility is user-controlled; must not auto-open on provider change. const [advancedOpen, setAdvancedOpen] = React.useState(false); - // Read inside effects via ref so biome's exhaustive-deps stays honest: - // refs are stable, and healOnMount is captured at declaration. + // Stable ref for effects; healOnMount is captured at declaration time. const mayMutateDependentFieldsRef = React.useRef(false); mayMutateDependentFieldsRef.current = healOnMount || userEditedProviderRef.current; @@ -439,14 +434,10 @@ export function AgentConfigFields({ ? (config.env_vars[effortPersistenceKey] ?? "") : ""; - // When the selected harness changes outside this component (Back → setup - // page → choose a different harness → Next), the saved model can belong to - // the old harness. In onboarding, heal that stale value as soon as the new - // harness catalog proves it is unsupported; otherwise a Codex id like - // `gpt-5.5[low]` appears as a Claude Code custom model. - // Also clear when the Model control is omitted after a confirmed successful - // empty catalog — never while discovery failed/unavailable (transient - // failures must not erase saved model/effort). + // Heal a stale model when the harness changes (e.g. Back → pick different + // harness → Next in onboarding). Clear once the new catalog proves the saved + // model unsupported; also clear when Model is omitted after a confirmed empty + // catalog. Never clear on failure/unavailable — transient errors must not erase. React.useEffect(() => { if (!healOnMount) return; const currentModel = (config.model ?? "").trim(); @@ -463,7 +454,9 @@ export function AgentConfigFields({ if (!catalogMiss && !omittedAfterSuccessfulEmpty) return; const nextEnvVars = { ...config.env_vars }; - if (effortPersistenceKey) delete nextEnvVars[effortPersistenceKey]; + // Harness-native effort is model-independent; never clear it on a catalog miss. + if (effortPersistenceKey && !isHarnessNativeEffort) + delete nextEnvVars[effortPersistenceKey]; onCustomModelEditingChange(false); onConfigChange({ ...config, env_vars: nextEnvVars, model: null }); }, [ @@ -477,28 +470,29 @@ export function AgentConfigFields({ onCustomModelEditingChange, healOnMount, effortPersistenceKey, + isHarnessNativeEffort, ]); // Orphan-model clearing follows the mount-time healing policy above: the - // backend resolves provider and model independently across layers - // (agent → definition → global), so a saved global model WITHOUT a global - // provider can be a deliberate, working pattern (provider supplied by a - // higher layer). Clearing it on page-open in evergreen surfaces silently - // breaks that agent on its next restart — see PR #2148 review thread. - // Onboarding heals on open by design (discriminating spec: "gates stale - // saved model and effort until provider selection"). + // backend resolves provider+model independently across layers, so a global + // model without a global provider can be deliberate (PR #2148). Evergreen + // surfaces only clear on explicit edit; onboarding heals on open. React.useEffect(() => { if (!mayMutateDependentFieldsRef.current) return; if (!dependentFieldsDisabled) return; + // When model is absent, harness-native effort is model-independent so no + // orphan to fix; non-native effort with no value is already clean. if ( (config.model ?? "").trim().length === 0 && - currentEffortForAutoClear.length === 0 - ) { + (isHarnessNativeEffort || currentEffortForAutoClear.length === 0) + ) return; - } const nextEnvVars = { ...config.env_vars }; - if (effortPersistenceKey) delete nextEnvVars[effortPersistenceKey]; + // Preserve harness-native effort — it is model-independent and must survive + // the provider→Custom transition. + if (effortPersistenceKey && !isHarnessNativeEffort) + delete nextEnvVars[effortPersistenceKey]; onCustomModelEditingChange(false); onConfigChange({ ...config, env_vars: nextEnvVars, model: null }); }, [ @@ -508,13 +502,16 @@ export function AgentConfigFields({ onConfigChange, onCustomModelEditingChange, effortPersistenceKey, + isHarnessNativeEffort, ]); + // `useEffortAutoClear` must not delete a valid harness-native value (e.g. + // "off" for Goose). Suppress it by passing "" as the current effort. const { validValues: effortValidForAutoClear } = getProviderEffortConfig( config.provider ?? "", config.model ?? "", ); useEffortAutoClear({ - currentEffort: currentEffortForAutoClear, + currentEffort: isHarnessNativeEffort ? "" : currentEffortForAutoClear, effortValid: effortValidForAutoClear, onClear: () => { const nextEnvVars = { ...config.env_vars }; @@ -635,6 +632,11 @@ export function AgentConfigFields({ : implicitEffortProvider; const { validValues: effortValid, defaultValue: effortDefault } = getProviderEffortConfig(effortProvider, config.model ?? ""); + // Harness-native runtimes own their effort vocabulary via the catalog entry. + const effortValidForRenderer = isHarnessNativeEffort + ? (selectedRuntime?.effortCanonicalValues ?? []) + : effortValid; + const effortDefaultForRenderer = isHarnessNativeEffort ? null : effortDefault; const currentEffort = effortPersistenceKey ? (config.env_vars[effortPersistenceKey] ?? "") : ""; @@ -853,21 +855,21 @@ export function AgentConfigFields({ currentEffort={dependentFieldsDisabled ? "" : currentEffort} disabled={dependentFieldsDisabled} emptyOptionLabel={ - // Semantic, not copy: onboarding-essential hides inheritance - // concepts (first-run users pick, they don't inherit), so the - // zero option is a plain placeholder. Full disclosure leaves - // this unset so EffortSelectField computes the inherit/default - // label ("Default (medium)", "Inherit (high)", …). + // Onboarding-essential hides inherit/default labels; show a plain + // placeholder. Full disclosure lets EffortSelectField compute + // its own label ("Default (medium)", "Inherit (high)", …). disclosure === "onboarding-essential" ? "Select effort level" : undefined } - effortDefault={effortDefault} - effortValid={effortValid} + effortDefault={effortDefaultForRenderer} + effortValid={effortValidForRenderer} fieldClassName={unstyled ? fieldClassName : undefined} htmlFor="global-agent-thinking-effort" inheritFallbackLabel={ - effortDefault !== null ? `Default (${effortDefault})` : undefined + effortDefaultForRenderer !== null + ? `Default (${effortDefaultForRenderer})` + : undefined } inheritedEffort={bakedEffort ?? undefined} label="Effort" diff --git a/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx index 62d85d385d4..205cf13a449 100644 --- a/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx +++ b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx @@ -1,9 +1,11 @@ import * as React from "react"; +import { useQueryClient } from "@tanstack/react-query"; import { ChevronDown } from "lucide-react"; import { AnimatePresence, motion, useReducedMotion } from "motion/react"; import { toast } from "sonner"; import { + agentConfigSurfaceQueryKey, useAcpRuntimesQuery, useAgentConfigSurface, useBakedBuildEnvKeysQuery, @@ -40,7 +42,6 @@ import { NO_RUNTIME_DROPDOWN_VALUE, PERSONA_FIELD_CONTROL_CLASS, PERSONA_FIELD_SHELL_CLASS, - PERSONA_LABEL_OPTIONAL_CLASS, runtimeSupportsLlmProviderSelection, shouldClearKnownModelForSelectionScope, sortPersonaRuntimes, @@ -55,6 +56,7 @@ import { envVarsEqual, isEditAgentProviderSaveValid, resolveAgentCommandUpdate, + resolveEffortSubmission, resolveInheritedRuntimeSubmission, resolveRuntimeProviderCapability, } from "./personaRuntimeModel"; @@ -74,15 +76,12 @@ import { MODEL_DISCOVERY_LOADING_VALUE, usePersonaModelDiscovery, } from "./usePersonaModelDiscovery"; -import { PersonaProviderApiKeyField } from "./PersonaProviderApiKeyField"; +import { EditAgentProviderModelFields } from "./EditAgentProviderModelFields"; import { getBakedModelInheritLabel, getBakedProviderInheritLabel, } from "./bakedEnvHelpers"; -import { - getProviderApiKeyEnvVar, - getProviderApiKeyLabel, -} from "./agentConfigOptions"; +import { getProviderApiKeyEnvVar } from "./agentConfigOptions"; import { useAgentDialogDefaults } from "./useAgentDialogDefaults"; import { AgentAiDefaultsNotice } from "./AgentAiDefaults"; import { AgentDefaultsDialog } from "./AgentDefaultsDialog"; @@ -116,6 +115,13 @@ export function AgentInstanceEditDialog({ }) { const updateMutation = useUpdateManagedAgentMutation(); const startMutation = useStartManagedAgentMutation(); + const queryClient = useQueryClient(); + // Spans the COMPLETE Save sequence (locked update + standalone setters). + // Every gate must key off this, not updateMutation.isPending alone. + const [isSaving, setIsSaving] = React.useState(false); + // Surfaces a standalone-setter failure (auto-restart or effort) that React + // Query does not track — keeps the dialog open so the user can retry Save. + const [setterError, setSetterError] = React.useState(null); const runtimesQuery = useAcpRuntimesQuery({ enabled: open }); const configSurfaceQuery = useAgentConfigSurface(open ? agent.pubkey : null); const runtimes = runtimesQuery.data ?? []; @@ -146,6 +152,13 @@ export function AgentInstanceEditDialog({ const [envVars, setEnvVars] = React.useState(agent.envVars); const [autoRestartOnConfigChange, setAutoRestartOnConfigChange] = React.useState(agent.autoRestartOnConfigChange); + // Effort picker is Save-gated: hold the pending selection in dialog state and + // embed it in the locked update payload on Save alone (see + // resolveEffortSubmission / handleSubmit — PR #4625), never on selection. + // `effortTouched` distinguishes "user picked a value" from "showing the + // config-surface effective value", so an untouched Save writes nothing. + const [effortLevel, setEffortLevel] = React.useState(null); + const effortTouched = React.useRef(false); const personasQuery = usePersonasQuery(); const linkedPersona = React.useMemo( () => @@ -195,6 +208,9 @@ export function AgentInstanceEditDialog({ setIsCustomProviderEditing(false); setEnvVars(agent.envVars); setAutoRestartOnConfigChange(agent.autoRestartOnConfigChange); + setEffortLevel(null); + effortTouched.current = false; + setSetterError(null); setRespondTo(agent.respondTo); setRespondToAllowlist(agent.respondToAllowlist); setAvatarUrl(agent.avatarUrl ?? ""); @@ -507,10 +523,10 @@ export function AgentInstanceEditDialog({ // Mark that the user has made an explicit runtime choice. The catalog-arrival // effect will no longer overwrite selectedRuntimeId after this point. runtimeTouched.current = true; - const resolvedRuntimeId = nextRuntimeId || "custom"; setSelectedRuntimeId(resolvedRuntimeId); - + effortTouched.current = false; + setEffortLevel(null); const isCustomCommand = resolvedRuntimeId === "custom"; // Only pin the harness when the selection can actually supply a command: @@ -588,6 +604,10 @@ export function AgentInstanceEditDialog({ } function handleOpenChange(next: boolean) { + // Reject user-originated dismissals (Escape, overlay, close-X, Cancel) while + // a Save is in flight — the in-flight setters must not commit to a closed dialog. + // The success path calls onOpenChange(false) directly, bypassing this guard. + if (!next && isSaving) return; onOpenChange(next); } @@ -613,10 +633,12 @@ export function AgentInstanceEditDialog({ requiredEnvKeyMissing, }) && providerValid && - !updateMutation.isPending && + !isSaving && !isAvatarUploadPending; async function handleSubmit() { + setIsSaving(true); + setSetterError(null); try { const parsedParallelism = Number.parseInt(parallelism, 10); const parsedArgs = agentArgs @@ -725,17 +747,53 @@ export function AgentInstanceEditDialog({ : undefined, }; + // Resolve effort before the update so access-change restarts can + // snapshot and launch the NEW effort value atomically. + const effortSubmission = resolveEffortSubmission({ + effortLevel, + originalEffortLevel: + configSurfaceQuery.data?.normalized.thinkingEffort?.value ?? null, + inheritTransition: agentCommandUpdate === "", + }); + // Include effort in the locked update when touched (tri-state: absent = + // don't touch; null = clear; string = set). Only when effortSubmission.persist. + if (effortTouched.current && effortSubmission.persist) { + input.effortLevel = effortSubmission.level; + } + const result = await updateMutation.mutateAsync(input); - if (autoRestartOnConfigChange !== agent.autoRestartOnConfigChange) { - // Standalone setter (mirrors start-on-app-launch) — not part of - // UpdateManagedAgentInput, so the frozen update shape stays frozen. - await setManagedAgentAutoRestart( - agent.pubkey, - autoRestartOnConfigChange, - ); + + // Standalone setters — sequenced after the locked update resolves so the + // dialog remains fully gated (isSaving) for the COMPLETE Save transaction. + // A failure here surfaces as setterError (retryable) and aborts before + // close, keeping the dialog open so the user can retry Save. + try { + if (autoRestartOnConfigChange !== agent.autoRestartOnConfigChange) { + // Mirrors start-on-app-launch; not part of UpdateManagedAgentInput so + // the frozen update shape stays frozen. + await setManagedAgentAutoRestart( + agent.pubkey, + autoRestartOnConfigChange, + ); + } + // Effort disk write happened inside the locked update. Only need to + // invalidate the cache here (when effortTouched && effortSubmission.persist). + // If effort was not included (!effortSubmission.persist), nothing to do. + if (effortTouched.current && effortSubmission.persist) { + // Disk write already done; invalidate so the panel tier reflects it. + await queryClient.invalidateQueries({ + queryKey: agentConfigSurfaceQueryKey(agent.pubkey), + }); + } + } catch (e) { + setSetterError(e instanceof Error ? e : new Error("Failed to save")); + return; } + showAgentProfileSyncWarning(result.agent.name, result.profileSyncError); - handleOpenChange(false); + // Close via onOpenChange directly — handleOpenChange guards against + // mid-save dismissal and must not block the intentional post-success close. + onOpenChange(false); onUpdated?.(result.agent); // The auto-restart policy deliberately never fires for a stopped or // failing agent (a broken agent must not auto-loop), so an edit meant @@ -761,7 +819,9 @@ export function AgentInstanceEditDialog({ }); } } catch { - // React Query stores the error; keep dialog open and render it inline. + // React Query stores the update error; keep dialog open and render it inline. + } finally { + setIsSaving(false); } } @@ -844,6 +904,11 @@ export function AgentInstanceEditDialog({ const advancedFieldsTransition = shouldReduceMotion ? { duration: 0 } : ADVANCED_FIELDS_MOTION_TRANSITION; + // Displayed inline when either the locked update or a standalone setter fails. + // setterError takes precedence — the update already committed when it fires. + const displayError = + setterError ?? + (updateMutation.error instanceof Error ? updateMutation.error : null); return ( @@ -857,7 +922,7 @@ export function AgentInstanceEditDialog({ footer={
} @@ -890,6 +955,7 @@ export function AgentInstanceEditDialog({ {onEditLinkedPersona ? (