Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions desktop/src-tauri/src/commands/agent_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -537,14 +537,19 @@ fn parse_models(raw: Option<&serde_json::Value>) -> (Vec<AcpModelEntry>, Option<

/// Persist the canonical startup effort level for a local managed agent.
///
/// B5 (v4 direct-write): the panel's EffortPicker calls this directly to set the
/// effort a spawn will apply at next session start. The value is stored on the
/// record; at spawn `runtime.rs` injects it as `BUZZ_ACP_EFFORT_LEVEL` and the
/// harness applies it via `session/set_config_option` against the adapter's
/// advertised `thought_level` configId. Pass `None` to clear (adapter default).
/// The panel's EffortPicker calls this directly to set the effort a spawn will
/// apply at next session start. The value is stored on the harness-agnostic
/// `effort_level` record column; at spawn the launch projection
/// (`config_bridge::effort`, invoked from `runtime.rs`) resolves the effective
/// value and emits it under the destination runtime's native key
/// (`GOOSE_THINKING_EFFORT`, `BUZZ_AGENT_THINKING_EFFORT`, or the
/// `BUZZ_ACP_EFFORT_LEVEL` startup sentinel for Claude/Codex and keyless
/// adapters, which apply it via `session/set_config_option` against the
/// adapter's advertised `thought_level` configId). Pass `None` to clear
/// (reverts to the inherited/adapter default).
///
/// Rejects non-local backends: remote agents receive effort through `policy_env`
/// at deploy time (see `agents_deploy.rs`), never this local persistence path —
/// Rejects non-local backends: remote agents receive effort through the launch
/// projection at deploy time (see `agents_deploy.rs`), never this local persistence path —
/// so an effort edit against a deployed agent is a caller error, not a silent
/// no-op that leaves the panel and the running agent disagreeing.
#[tauri::command]
Expand Down
6 changes: 4 additions & 2 deletions desktop/src-tauri/src/commands/agent_config_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ fn with_no_goose_config<T>(body: impl FnOnce() -> T) -> T {
}

fn goose_runtime() -> &'static KnownAcpRuntime {
&KnownAcpRuntime {
static RUNTIME: KnownAcpRuntime = KnownAcpRuntime {
id: "goose",
label: "Goose",
commands: &["goose"],
Expand All @@ -55,13 +55,15 @@ fn goose_runtime() -> &'static KnownAcpRuntime {
config_file_format: Some("yaml"),
supports_acp_native_config: true,
thinking_env_var: Some("GOOSE_THINKING_EFFORT"),
effort_normalization: Some(&crate::managed_agents::GOOSE_EFFORT_NORMALIZATION),
max_tokens_env_var: Some("GOOSE_MAX_TOKENS"),
context_limit_env_var: Some("GOOSE_CONTEXT_LIMIT"),
max_rounds_env_var: None,
required_normalized_fields: &["model", "provider"],
login_hint: None,
auth_probe_args: None,
}
};
&RUNTIME
}

fn agent_record() -> ManagedAgentRecord {
Expand Down
26 changes: 20 additions & 6 deletions desktop/src-tauri/src/commands/agent_models_update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,15 +115,17 @@ pub async fn update_managed_agent(
// Harness edit: the persona's runtime is authoritative, so an explicit
// `agent_command_override` is persisted ONLY when the user picks a
// command that diverges from the persona, and the empty/whitespace
// "Inherit from persona" sentinel clears both the pin and the
// materialized record runtime. A name-only edit
// "Inherit from persona" sentinel clears the pin, the materialized
// record runtime, AND the per-instance effort override (column here,
// env aliases after `env_vars` is applied below). A name-only edit
// (`agent_command == None`) leaves the pin intact. `harness_override`
// threads the user's explicit intent — see `apply_agent_command_update`
// and `update_time_agent_command_override` for the full resolution
// rules.
let mut inherit_transition = false;
if let Some(agent_command) = input.agent_command {
let personas = load_personas(&app).unwrap_or_default();
crate::managed_agents::apply_agent_command_update(
inherit_transition = crate::managed_agents::apply_agent_command_update(
record,
&personas,
&agent_command,
Expand All @@ -136,10 +138,22 @@ pub async fn update_managed_agent(
// mcp_command is intentionally not applied here — the effective MCP
// command is always catalog-derived (known_acp_runtime at spawn time)
// and the per-record field is never read by the runtime.
if let Some(env_vars) = input.env_vars {
crate::managed_agents::validate_user_env_keys(&env_vars)?;
record.env_vars = env_vars;
//
// Apply the caller-supplied `env_vars` (validated first), then — only on
// the pin→inherit transition — strip the record effort env aliases. The
// order is load-bearing: stripping AFTER the env replacement is what
// stops a same-request `env_vars` map from reintroducing a stale effort
// alias while the instance inherits its harness. The column was already
// cleared inside `apply_agent_command_update`. See
// `apply_env_vars_then_effort_transition` for the pinned invariant.
if let Some(ref env_vars) = input.env_vars {
crate::managed_agents::validate_user_env_keys(env_vars)?;
}
crate::managed_agents::apply_env_vars_then_effort_transition(
record,
input.env_vars,
inherit_transition,
);

// Native provider/model fields are authoritative. Keep the typed marker
// derived for new records while retaining legacy typed records for
Expand Down
90 changes: 51 additions & 39 deletions desktop/src-tauri/src/commands/agents_deploy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,13 +96,13 @@ pub(super) fn build_launch_block(
};
policy_env.insert(model_key.into(), value.to_string());
}
// I-4: remote parity for persisted startup effort. Mirrors the local spawn
// path in runtime.rs. The harness reads BUZZ_ACP_EFFORT_LEVEL into
// PoolStartup.startup_effort and applies it at first session creation via
// resolve_startup_effort().
if let Some(ref value) = record.effort_level {
policy_env.insert("BUZZ_ACP_EFFORT_LEVEL".into(), value.clone());
}
// Startup effort needs no remote-specific handling: the harness-agnostic
// effort projection already ran inside `resolve_effective_harness_descriptor`,
// so `descriptor.env` (→ `launch.env`, tier 2) carries exactly one effort key
// holding the effective value, with every foreign/legacy/transport effort key
// stripped. Tier 2 later-wins over `policy_env` (tier 1) and no authoritative
// tier-3 key collides with an effort key, so the projected value reaches the
// remote pod verbatim — identical authority to the local spawn.
if let Some(value) = record.idle_timeout_seconds {
policy_env.insert("BUZZ_ACP_IDLE_TIMEOUT".into(), value.to_string());
}
Expand All @@ -119,14 +119,6 @@ pub(super) fn build_launch_block(
policy_env.insert("BUZZ_ACP_TEAM_INSTRUCTIONS".into(), value);
}

// B5 remote parity: when a canonical effort_level is persisted, strip
// BUZZ_ACP_EFFORT_LEVEL from launch.env so it cannot shadow the canonical
// value in policy_env (tier 1). In the k8s three-tier model tier 2
// (launch.env) overwrites tier 1 (policy_env) — later-wins — so the key
// must be absent from tier 2 whenever a canonical value is present.
// When effort_level is None there is no canonical to protect, so user
// env passthrough stands (env may legitimately seed startup effort).
//
// B2 remote parity: mirror the local A1 model authority. For a Claude
// launch, ALWAYS strip BOTH BUZZ_ACP_MODEL and ANTHROPIC_MODEL from
// launch.env — the resolved canonical model rides policy_env.ANTHROPIC_MODEL
Expand All @@ -136,12 +128,15 @@ pub(super) fn build_launch_block(
// canonical model. When no canonical model is present, neither key is in
// policy_env, so stripping them keeps the remote process free of both —
// matching local, where `apply_claude_model_env(None)` removes both.
//
// Effort keys need no stripping here: the projection already reduced
// `descriptor.env` to exactly one effort key holding the effective value,
// so launch.env carries the authority directly (see the effort note above).
let is_claude = runtime.map(|r| r.id == "claude").unwrap_or(false);
let strip_key = |k: &str| {
(record.effort_level.is_some() && k.eq_ignore_ascii_case("BUZZ_ACP_EFFORT_LEVEL"))
|| (is_claude
&& (k.eq_ignore_ascii_case("BUZZ_ACP_MODEL")
|| k.eq_ignore_ascii_case("ANTHROPIC_MODEL")))
is_claude
&& (k.eq_ignore_ascii_case("BUZZ_ACP_MODEL")
|| k.eq_ignore_ascii_case("ANTHROPIC_MODEL"))
};
let launch_env: BTreeMap<String, String> = descriptor
.env
Expand Down Expand Up @@ -467,19 +462,27 @@ mod tests {
}

#[test]
fn launch_block_claude_runtime_injects_effort_level_when_set() {
// I-4: remote parity — record.effort_level → BUZZ_ACP_EFFORT_LEVEL in policy_env.
let mut record = record();
record.effort_level = Some("high".to_string());
fn launch_block_claude_runtime_carries_projected_effort_in_launch_env() {
// Under the harness-agnostic projection, effort no longer rides
// policy_env: `resolve_effective_harness_descriptor` reduces
// `descriptor.env` to exactly one effort key (for a keyless claude
// runtime, the ACP sentinel) holding the effective value, and
// build_launch_block passes that env through to launch.env verbatim.
let record = record();
let descriptor = EffectiveHarnessDescriptor {
command: "claude".into(),
args: vec![],
env: BTreeMap::new(),
// The single projected effort key the descriptor resolver emits.
env: BTreeMap::from([("BUZZ_ACP_EFFORT_LEVEL".to_string(), "high".to_string())]),
};
let launch = build_launch_block(&record, &descriptor, &[], None, None, "owner-hex");
assert_eq!(
launch["policy_env"]["BUZZ_ACP_EFFORT_LEVEL"], "high",
"claude remote must receive BUZZ_ACP_EFFORT_LEVEL when effort_level is set"
launch["env"]["BUZZ_ACP_EFFORT_LEVEL"], "high",
"the projected effort key must survive into launch.env"
);
assert!(
launch["policy_env"]["BUZZ_ACP_EFFORT_LEVEL"].is_null(),
"effort is not a policy_env value under the projection design"
);
}

Expand All @@ -506,26 +509,35 @@ mod tests {
/// authoritative.
#[test]
fn launch_block_canonical_effort_strips_user_env_collision() {
// Remote parity for the authority collision: the canonical column and a
// conflicting user `BUZZ_ACP_EFFORT_LEVEL` both present. The projection
// (run inside `resolve_effective_harness_descriptor`) resolves it —
// canonical `high` wins over the user `low` transport sentinel — and
// build_launch_block carries exactly that one value into launch.env,
// identical to the local spawn path.
let mut record = record();
record.runtime = Some("claude".into());
record.effort_level = Some("high".to_string());
let descriptor = EffectiveHarnessDescriptor {
command: "claude".into(),
args: vec![],
// User-supplied conflicting value in descriptor.env.
env: BTreeMap::from([("BUZZ_ACP_EFFORT_LEVEL".to_string(), "low".to_string())]),
};
record
.env_vars
.insert("BUZZ_ACP_EFFORT_LEVEL".into(), "low".into());
let descriptor = crate::managed_agents::resolve_effective_harness_descriptor(
&record,
&[],
&Default::default(),
)
.expect("claude descriptor resolves");
let launch = build_launch_block(&record, &descriptor, &[], None, None, "owner-hex");

// Canonical must be in policy_env (tier 1).
// The projected canonical authority is the single effort value carried.
assert_eq!(
launch["policy_env"]["BUZZ_ACP_EFFORT_LEVEL"], "high",
"canonical effort must be in policy_env when record.effort_level is Some"
launch["env"]["BUZZ_ACP_EFFORT_LEVEL"], "high",
"canonical effort must win the collision and reach launch.env"
);
// Conflicting user value must be absent from launch.env (tier 2) so it
// cannot shadow the canonical tier-1 value in build_env.
// Effort is not a policy_env value under the projection design.
assert!(
launch["env"]["BUZZ_ACP_EFFORT_LEVEL"].is_null(),
"user BUZZ_ACP_EFFORT_LEVEL must be stripped from launch.env when canonical is present"
launch["policy_env"]["BUZZ_ACP_EFFORT_LEVEL"].is_null(),
"effort is carried in launch.env, never policy_env"
);
}

Expand Down
28 changes: 4 additions & 24 deletions desktop/src-tauri/src/managed_agents/claude_config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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;
78 changes: 7 additions & 71 deletions desktop/src-tauri/src/managed_agents/claude_config/tests.rs
Original file line number Diff line number Diff line change
@@ -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).
Expand Down Expand Up @@ -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.
Loading
Loading