mirror of
https://github.com/block/buzz.git
synced 2026-10-31 22:41:12 +00:00
fix(runtime): make canonical effort win over user env, locally and remotely
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 <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This commit is contained in:
@@ -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<String, String> = 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.
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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"
|
||||
);
|
||||
}
|
||||
|
||||
@@ -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).
|
||||
|
||||
Reference in New Issue
Block a user