From 012c1c463806f9d8d82deffa2f124c054cf6fa12 Mon Sep 17 00:00:00 2001 From: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz> Date: Fri, 7 Aug 2026 14:25:41 -0400 Subject: [PATCH] fix(runtime): make canonical effort win over user env, locally and remotely MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes P2-1: BUZZ_ACP_EFFORT_LEVEL was written before descriptor.env, so a user-supplied value in agent/persona/global env silently overrode the canonical persisted effort_level. The panel and persistence continued to report the canonical value while the harness started at the user-supplied one — the same silent-divergence class the PR exists to eliminate. Local fix (runtime.rs): extract apply_effort_env() and call it after the descriptor.env loop, mirroring the A1 model-authority pattern exactly. When effort_level is Some, the canonical value overwrites any user-supplied entry. When None, the call is a no-op so user env from the loop survives as legitimate startup seeding (no canonical to contradict). Remote fix (agents_deploy.rs): when a canonical effort_level is present, strip BUZZ_ACP_EFFORT_LEVEL from launch.env before serialisation. In the k8s three-tier model, tier 2 (launch.env) overwrites tier 1 (policy_env) via later-wins — so the key must be absent from tier 2 whenever a canonical value is already authoritative in tier 1. The strip is done on the Desktop side, preserving the k8s engine's declared semantics rather than adding a special case to build_env. Tests: three local tests via apply_effort_env() in claude_config/tests.rs (collision canonical wins, basic injection, None passthrough preserved) and two remote tests in agents_deploy.rs (collision strips tier-2 entry, None lets user entry survive in launch.env). Ownership: desktop/src-tauri/src/managed_agents/runtime.rs, claude_config/{mod,tests}.rs, commands/agents_deploy.rs. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../src-tauri/src/commands/agents_deploy.rs | 75 ++++++++++++++++++- .../src/managed_agents/claude_config/mod.rs | 19 +++++ .../src/managed_agents/claude_config/tests.rs | 74 +++++++++++++++++- .../src-tauri/src/managed_agents/runtime.rs | 16 ++-- 4 files changed, 175 insertions(+), 9 deletions(-) diff --git a/desktop/src-tauri/src/commands/agents_deploy.rs b/desktop/src-tauri/src/commands/agents_deploy.rs index 59573ee404..d67e073287 100644 --- a/desktop/src-tauri/src/commands/agents_deploy.rs +++ b/desktop/src-tauri/src/commands/agents_deploy.rs @@ -119,10 +119,28 @@ 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). + let launch_env: BTreeMap = if record.effort_level.is_some() { + descriptor + .env + .iter() + .filter(|(k, _)| !k.eq_ignore_ascii_case("BUZZ_ACP_EFFORT_LEVEL")) + .map(|(k, v)| (k.clone(), v.clone())) + .collect() + } else { + descriptor.env.clone() + }; + serde_json::json!({ "command": descriptor.command, "args": descriptor.args, - "env": descriptor.env, + "env": launch_env, "policy_env": policy_env, "owner_pubkey": owner_pubkey, }) @@ -376,6 +394,61 @@ mod tests { ); } + /// B5 remote parity: when a canonical effort_level is persisted, a conflicting + /// user-supplied BUZZ_ACP_EFFORT_LEVEL in descriptor.env must NOT shadow it. + /// The canonical value in policy_env (tier 1) must win in the final build_env + /// output — the key must be absent from launch.env (tier 2) so tier 1 is + /// authoritative. + #[test] + fn launch_block_canonical_effort_strips_user_env_collision() { + let mut record = record(); + 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())]), + }; + let launch = build_launch_block(&record, &descriptor, &[], None, None, "owner-hex"); + + // Canonical must be in policy_env (tier 1). + assert_eq!( + launch["policy_env"]["BUZZ_ACP_EFFORT_LEVEL"], "high", + "canonical effort must be in policy_env when record.effort_level is Some" + ); + // Conflicting user value must be absent from launch.env (tier 2) so it + // cannot shadow the canonical tier-1 value in build_env. + assert!( + launch["env"]["BUZZ_ACP_EFFORT_LEVEL"].is_null(), + "user BUZZ_ACP_EFFORT_LEVEL must be stripped from launch.env when canonical is present" + ); + } + + /// B5 remote parity: when no canonical effort is persisted (effort_level is + /// None), a user-supplied BUZZ_ACP_EFFORT_LEVEL in descriptor.env survives + /// into launch.env — passthrough preserved for startup seeding. + #[test] + fn launch_block_user_effort_env_survives_when_no_canonical_value() { + let record = record(); // effort_level is None + let descriptor = EffectiveHarnessDescriptor { + command: "claude".into(), + args: vec![], + env: BTreeMap::from([("BUZZ_ACP_EFFORT_LEVEL".to_string(), "low".to_string())]), + }; + let launch = build_launch_block(&record, &descriptor, &[], None, None, "owner-hex"); + + // No canonical — key must NOT appear in policy_env. + assert!( + launch["policy_env"]["BUZZ_ACP_EFFORT_LEVEL"].is_null(), + "policy_env must NOT contain BUZZ_ACP_EFFORT_LEVEL when effort_level is None" + ); + // User value must survive in launch.env so the harness can use it. + assert_eq!( + launch["env"]["BUZZ_ACP_EFFORT_LEVEL"], "low", + "user-supplied effort must survive in launch.env when no canonical value" + ); + } + /// OpenClaw descriptor: `launch.policy_env["BUZZ_ACP_AGENTS"]` must be "5" /// even when the record's requested parallelism is 10. This is the direct /// `launch.policy_env` seam test — the executable contract for remote providers. 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 2a728003dd..42b856b377 100644 --- a/desktop/src-tauri/src/managed_agents/claude_config/mod.rs +++ b/desktop/src-tauri/src/managed_agents/claude_config/mod.rs @@ -3,6 +3,10 @@ //! A1 contract: `ANTHROPIC_MODEL` is the single startup model authority for //! 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. /// 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. @@ -23,6 +27,21 @@ 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("BUZZ_ACP_EFFORT_LEVEL", 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 326fb8c34e..f6f0f90cb2 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; +use super::{apply_claude_model_env, apply_effort_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). @@ -53,3 +53,75 @@ fn a1_anthropic_model_removed_when_no_effective_model() { "BUZZ_ACP_MODEL must always be absent after A1 policy" ); } + +// ── B5 effort-authority contract tests ────────────────────────────────────── +// +// 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" + ); +} diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index c9d28aef9a..e12bf390d3 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -14,7 +14,7 @@ use crate::{ util::now_iso, }; -use super::claude_config::apply_claude_model_env; +use super::claude_config::{apply_claude_model_env, apply_effort_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}; @@ -719,12 +719,6 @@ pub fn spawn_agent_child( } else { command.env_remove("BUZZ_ACP_MODEL"); } - // B5: carry persisted effort; harness resolves thought_level configId at first session. - if let Some(ref effort) = record.effort_level { - command.env("BUZZ_ACP_EFFORT_LEVEL", effort); - } else { - command.env_remove("BUZZ_ACP_EFFORT_LEVEL"); - } // Session title for the harness to pass out-of-band on `session/new`. The // adapter names the session after it; it never reaches the prompt, so this // is display metadata only. The spawn-config snapshot records the same @@ -802,6 +796,14 @@ pub fn spawn_agent_child( command.env(key, value); } + // B5: carry persisted effort; harness resolves thought_level configId at first session. + // Written AFTER descriptor.env so the canonical persisted value wins over any + // user-supplied BUZZ_ACP_EFFORT_LEVEL entry, mirroring the A1 model-authority pattern + // (ANTHROPIC_MODEL is applied post-loop for the same reason). When effort_level is + // None there is no canonical value to assert, so env passthrough stands — user env + // legitimately seeds startup effort in that case. + apply_effort_env(&mut command, record.effort_level.as_deref()); + // 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).