fix(agents): scrub GRAPH_STATE* from adversary verification commands; raise review-suite max_agent_depth
FIX A: run_checks.py passed no env= to Popen, so verification commands
inherited the live GRAPH_STATE/GRAPH_STATE_FILE. Every suite script's
load_state() prefers GRAPH_STATE_FILE, so when the adversary reviews a repo
whose own tests exec these scripts (this repo's R5 tests) the leaked file
silently overrode the test state -> false failures. Popen now gets an env
with all GRAPH_STATE* keys removed. The five sibling script-test helpers
(review-gauntlet, code-reviewer, finding-verifier, step-runner,
deep-research) gain .env_remove("GRAPH_STATE_FILE") for hermeticity. New
R5 test loads state via GRAPH_STATE_FILE and asserts a shell-neutral
python3 -c probe sees both vars UNSET.
FIX B: the depth check compares the child's absolute depth against the
SPAWNER's own max_agent_depth (root = 0), so under
architect(0)->sisyphus(1)->review-gauntlet(2)->code-reviewer(3) the
code-review lane faulted "Max agent depth exceeded (4/3)". Limits are now
child depth + 1 headroom along the documented chains, with the chain in a
comment next to each key: code-reviewer 3->5, domain-reviewer 2->6,
architecture-reviewer 2->4, step-runner default->4 (explicit),
review-gauntlet stays 4. domain-reviewer's file-reviewer hop is documented
but inert (can_spawn_agents unset) - left unwired by decision; noted in
config and pinned via a DOCUMENTED_BUT_UNWIRED exemption in the new
bundled-config test, which pins every value and asserts per-hop headroom +
reachability along four chains.
This commit is contained in:
7 files changed
+184
-3
No files matched your search
@@ -25,6 +25,11 @@ its own session (start_new_session) and a timeout SIGKILLs the whole process
|
|||||||
group, so grandchildren of a hung command cannot linger. On non-POSIX
|
group, so grandchildren of a hung command cannot linger. On non-POSIX
|
||||||
platforms only the direct shell child is killed — grandchildren of a
|
platforms only the direct shell child is killed — grandchildren of a
|
||||||
timed-out command may survive (documented limitation).
|
timed-out command may survive (documented limitation).
|
||||||
|
|
||||||
|
Verification commands run with GRAPH_STATE* (GRAPH_STATE, GRAPH_STATE_FILE)
|
||||||
|
scrubbed from their environment: the reviewed repo's own tests may exec these very
|
||||||
|
scripts with their own GRAPH_STATE, and load_state() prefers
|
||||||
|
GRAPH_STATE_FILE, so an inherited live state file would silently win.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
import json
|
import json
|
||||||
@@ -57,6 +62,7 @@ def run_one(cmd, proj, budget):
|
|||||||
"""Run one command, hard-bounded by `budget` seconds; return its record."""
|
"""Run one command, hard-bounded by `budget` seconds; return its record."""
|
||||||
start = time.monotonic()
|
start = time.monotonic()
|
||||||
popen_kwargs = {"start_new_session": True} if os.name == "posix" else {}
|
popen_kwargs = {"start_new_session": True} if os.name == "posix" else {}
|
||||||
|
env = {k: v for k, v in os.environ.items() if not k.startswith("GRAPH_STATE")}
|
||||||
p = subprocess.Popen(
|
p = subprocess.Popen(
|
||||||
cmd,
|
cmd,
|
||||||
shell=True,
|
shell=True,
|
||||||
@@ -64,6 +70,7 @@ def run_one(cmd, proj, budget):
|
|||||||
stdout=subprocess.PIPE,
|
stdout=subprocess.PIPE,
|
||||||
stderr=subprocess.PIPE,
|
stderr=subprocess.PIPE,
|
||||||
text=True,
|
text=True,
|
||||||
|
env=env,
|
||||||
**popen_kwargs,
|
**popen_kwargs,
|
||||||
)
|
)
|
||||||
try:
|
try:
|
||||||
|
|||||||
@@ -12,7 +12,11 @@ spawnable_agents:
|
|||||||
- explore
|
- explore
|
||||||
- oracle
|
- oracle
|
||||||
max_concurrent_agents: 4
|
max_concurrent_agents: 4
|
||||||
max_agent_depth: 2
|
# The limit is consulted by the SPAWNER against the child's absolute depth
|
||||||
|
# (root = 0). Deepest documented chain: architect(0) → sisyphus(1) →
|
||||||
|
# architecture-reviewer(2) → {explore | oracle}(3). This agent spawns depth-3
|
||||||
|
# children; 4 = one level of headroom.
|
||||||
|
max_agent_depth: 4
|
||||||
inject_spawn_instructions: true
|
inject_spawn_instructions: true
|
||||||
|
|
||||||
skills_enabled: true
|
skills_enabled: true
|
||||||
|
|||||||
@@ -18,7 +18,12 @@ description: |
|
|||||||
version: '1.0'
|
version: '1.0'
|
||||||
|
|
||||||
can_spawn_agents: true
|
can_spawn_agents: true
|
||||||
max_agent_depth: 3
|
# The limit is consulted by the SPAWNER against the child's absolute depth
|
||||||
|
# (root = 0). Deepest documented chain: architect(0) → sisyphus(1) →
|
||||||
|
# review-gauntlet | step-runner(2) → code-reviewer(3) →
|
||||||
|
# {domain-reviewer(4) → file-reviewer(5) | finding-verifier(4)}.
|
||||||
|
# This agent spawns depth-4 children; 5 = one level of headroom.
|
||||||
|
max_agent_depth: 5
|
||||||
max_concurrent_agents: 16
|
max_concurrent_agents: 16
|
||||||
|
|
||||||
variables:
|
variables:
|
||||||
|
|||||||
@@ -23,7 +23,13 @@ enabled_skills:
|
|||||||
spawnable_agents:
|
spawnable_agents:
|
||||||
- file-reviewer
|
- file-reviewer
|
||||||
max_concurrent_agents: 8
|
max_concurrent_agents: 8
|
||||||
max_agent_depth: 2
|
# The limit is consulted by the SPAWNER against the child's absolute depth
|
||||||
|
# (root = 0). Deepest documented chain: architect(0) → sisyphus(1) →
|
||||||
|
# review-gauntlet | step-runner(2) → code-reviewer(3) → domain-reviewer(4) →
|
||||||
|
# file-reviewer(5). This agent spawns depth-5 children; 6 = one level of headroom.
|
||||||
|
# NOTE: can_spawn_agents is not yet set, so the file-reviewer hop is documented
|
||||||
|
# but inert; 6 keeps the limit forward-compatible for when it is wired.
|
||||||
|
max_agent_depth: 6
|
||||||
|
|
||||||
variables:
|
variables:
|
||||||
- name: project_dir
|
- name: project_dir
|
||||||
|
|||||||
@@ -19,6 +19,11 @@ description: |
|
|||||||
|
|
||||||
version: '1.0'
|
version: '1.0'
|
||||||
|
|
||||||
|
# The limit is consulted by the SPAWNER against the child's absolute depth
|
||||||
|
# (root = 0). Deepest documented chain: architect(0) → sisyphus(1) →
|
||||||
|
# review-gauntlet(2) → code-reviewer(3) →
|
||||||
|
# {domain-reviewer(4) → file-reviewer(5) | finding-verifier(4)}.
|
||||||
|
# This agent spawns depth-3 children; 4 = one level of headroom.
|
||||||
max_agent_depth: 4
|
max_agent_depth: 4
|
||||||
max_concurrent_agents: 12
|
max_concurrent_agents: 12
|
||||||
|
|
||||||
|
|||||||
@@ -6,6 +6,12 @@ description: |
|
|||||||
review -> evidence-backed handoff -> user approval gate. Designed to be
|
review -> evidence-backed handoff -> user approval gate. Designed to be
|
||||||
delegated to by sisyphus.
|
delegated to by sisyphus.
|
||||||
version: '1.0'
|
version: '1.0'
|
||||||
|
# The limit is consulted by the SPAWNER against the child's absolute depth
|
||||||
|
# (root = 0). Deepest documented chain: architect(0) → sisyphus(1) →
|
||||||
|
# step-runner(2) → code-reviewer(3) →
|
||||||
|
# {domain-reviewer(4) → file-reviewer(5) | finding-verifier(4)}.
|
||||||
|
# This agent spawns depth-3 children; 4 = one level of headroom (default 3 had none).
|
||||||
|
max_agent_depth: 4
|
||||||
global_tools:
|
global_tools:
|
||||||
- ast_grep.sh
|
- ast_grep.sh
|
||||||
- fs_cat.sh
|
- fs_cat.sh
|
||||||
|
|||||||
@@ -8223,6 +8223,102 @@ mod tests {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// The spawner's own `max_agent_depth` is checked against the child's
|
||||||
|
// absolute depth (root = 0), so every hop along a chain needs its own
|
||||||
|
// limit >= child depth; +1 leaves one level of headroom.
|
||||||
|
#[test]
|
||||||
|
#[serial]
|
||||||
|
fn bundled_review_suite_depth_limits_cover_documented_chain() {
|
||||||
|
use crate::graph::GraphParser;
|
||||||
|
|
||||||
|
fn bundled_agent_config(name: &str) -> AgentConfig {
|
||||||
|
let dir = paths::agents_data_dir().join(name);
|
||||||
|
let graph_path = dir.join("graph.yaml");
|
||||||
|
if graph_path.exists() {
|
||||||
|
let graph = GraphParser::new(&dir)
|
||||||
|
.load_from_file(&graph_path)
|
||||||
|
.unwrap_or_else(|e| panic!("graph.yaml for '{name}' failed to parse: {e}"));
|
||||||
|
return AgentConfig::from_graph(name, &graph);
|
||||||
|
}
|
||||||
|
AgentConfig::load(&dir.join("config.yaml"))
|
||||||
|
.unwrap_or_else(|e| panic!("config.yaml for '{name}' failed to load: {e}"))
|
||||||
|
}
|
||||||
|
|
||||||
|
let _guard = TestConfigDirGuard::new();
|
||||||
|
Agent::install_builtin_agents(false).unwrap();
|
||||||
|
|
||||||
|
for (name, expected) in [
|
||||||
|
("architect", 10),
|
||||||
|
("sisyphus", 5),
|
||||||
|
("review-gauntlet", 4),
|
||||||
|
("step-runner", 4),
|
||||||
|
("code-reviewer", 5),
|
||||||
|
("domain-reviewer", 6),
|
||||||
|
("architecture-reviewer", 4),
|
||||||
|
] {
|
||||||
|
assert_eq!(
|
||||||
|
bundled_agent_config(name).max_agent_depth,
|
||||||
|
expected,
|
||||||
|
"bundled '{name}' max_agent_depth drifted"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
// User decision: domain-reviewer's file-reviewer fan-out stays unwired
|
||||||
|
// for now. Remove it from this list when `can_spawn_agents: true` is added.
|
||||||
|
const DOCUMENTED_BUT_UNWIRED: &[&str] = &["domain-reviewer"];
|
||||||
|
|
||||||
|
let chains: [&[&str]; 4] = [
|
||||||
|
&[
|
||||||
|
"architect",
|
||||||
|
"sisyphus",
|
||||||
|
"review-gauntlet",
|
||||||
|
"code-reviewer",
|
||||||
|
"domain-reviewer",
|
||||||
|
"file-reviewer",
|
||||||
|
],
|
||||||
|
&[
|
||||||
|
"architect",
|
||||||
|
"sisyphus",
|
||||||
|
"step-runner",
|
||||||
|
"code-reviewer",
|
||||||
|
"domain-reviewer",
|
||||||
|
"file-reviewer",
|
||||||
|
],
|
||||||
|
&[
|
||||||
|
"architect",
|
||||||
|
"sisyphus",
|
||||||
|
"review-gauntlet",
|
||||||
|
"code-reviewer",
|
||||||
|
"finding-verifier",
|
||||||
|
],
|
||||||
|
&["architect", "sisyphus", "architecture-reviewer", "explore"],
|
||||||
|
];
|
||||||
|
for chain in chains {
|
||||||
|
for (i, pair) in chain.windows(2).enumerate() {
|
||||||
|
let (spawner, child) = (pair[0], pair[1]);
|
||||||
|
let cfg = bundled_agent_config(spawner);
|
||||||
|
assert!(
|
||||||
|
cfg.max_agent_depth >= i + 2,
|
||||||
|
"{spawner} (depth {i}) needs max_agent_depth >= {} to spawn {child} at depth {} with one level of headroom",
|
||||||
|
i + 2,
|
||||||
|
i + 1
|
||||||
|
);
|
||||||
|
if !DOCUMENTED_BUT_UNWIRED.contains(&spawner) {
|
||||||
|
assert!(
|
||||||
|
cfg.can_spawn_agents,
|
||||||
|
"{spawner} must have can_spawn_agents to reach {child}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
if let Some(list) = &cfg.spawnable_agents {
|
||||||
|
assert!(
|
||||||
|
list.iter().any(|s| s == child),
|
||||||
|
"{spawner} spawnable_agents must include {child}, got {list:?}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
#[serial]
|
#[serial]
|
||||||
fn bundled_graph_agents_parse_and_validate() {
|
fn bundled_graph_agents_parse_and_validate() {
|
||||||
@@ -8592,6 +8688,53 @@ mod tests {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn adversary_run_checks_scrubs_graph_state_from_verification_commands() {
|
||||||
|
if !cmd_available("python3") {
|
||||||
|
eprintln!("skipping: python3 not available");
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
let unique = SystemTime::now()
|
||||||
|
.duration_since(UNIX_EPOCH)
|
||||||
|
.unwrap()
|
||||||
|
.as_nanos();
|
||||||
|
let state_file =
|
||||||
|
env::temp_dir().join(format!("coyote-adversary-run-checks-state-{unique}.json"));
|
||||||
|
// python3 probe rather than `echo "${VAR:-UNSET}"`: run_checks.py uses
|
||||||
|
// shell=True, which is cmd.exe on the Windows CI leg.
|
||||||
|
let state = json!({
|
||||||
|
"verification_commands": [
|
||||||
|
"python3 -c \"import os; print('file=' + os.environ.get('GRAPH_STATE_FILE', 'UNSET') + ' inline=' + os.environ.get('GRAPH_STATE', 'UNSET'))\""
|
||||||
|
]
|
||||||
|
});
|
||||||
|
write(&state_file, state.to_string()).unwrap();
|
||||||
|
|
||||||
|
// The script itself must load from GRAPH_STATE_FILE (the envs are
|
||||||
|
// applied after the helper's env_remove), so an inline dummy state
|
||||||
|
// proves the file-preferred path is the one exercised.
|
||||||
|
let out = run_adversary_script_env(
|
||||||
|
"run_checks.py",
|
||||||
|
&json!({}),
|
||||||
|
&[("GRAPH_STATE_FILE", state_file.to_str().unwrap())],
|
||||||
|
);
|
||||||
|
let _ = std::fs::remove_file(&state_file);
|
||||||
|
|
||||||
|
let results = out["exec_results"].as_array().unwrap();
|
||||||
|
assert_eq!(
|
||||||
|
results.len(),
|
||||||
|
1,
|
||||||
|
"state must come from the file: {results:?}"
|
||||||
|
);
|
||||||
|
assert_eq!(results[0]["exit"], 0, "{results:?}");
|
||||||
|
assert!(
|
||||||
|
results[0]["tail"]
|
||||||
|
.as_str()
|
||||||
|
.unwrap()
|
||||||
|
.contains("file=UNSET inline=UNSET"),
|
||||||
|
"verification command must not inherit GRAPH_STATE*: {results:?}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn adversary_run_checks_runner_error_degrades_to_environment_marker() {
|
fn adversary_run_checks_runner_error_degrades_to_environment_marker() {
|
||||||
if !cmd_available("python3") {
|
if !cmd_available("python3") {
|
||||||
@@ -9297,6 +9440,7 @@ mod tests {
|
|||||||
let out = std::process::Command::new("python3")
|
let out = std::process::Command::new("python3")
|
||||||
.arg(&path)
|
.arg(&path)
|
||||||
.env("GRAPH_STATE", state.to_string())
|
.env("GRAPH_STATE", state.to_string())
|
||||||
|
.env_remove("GRAPH_STATE_FILE")
|
||||||
.output()
|
.output()
|
||||||
.unwrap_or_else(|e| panic!("failed to invoke python3 {script}: {e}"));
|
.unwrap_or_else(|e| panic!("failed to invoke python3 {script}: {e}"));
|
||||||
assert!(
|
assert!(
|
||||||
@@ -9787,6 +9931,7 @@ mod tests {
|
|||||||
let out = std::process::Command::new("python3")
|
let out = std::process::Command::new("python3")
|
||||||
.arg(&path)
|
.arg(&path)
|
||||||
.env("GRAPH_STATE", raw_state)
|
.env("GRAPH_STATE", raw_state)
|
||||||
|
.env_remove("GRAPH_STATE_FILE")
|
||||||
.output()
|
.output()
|
||||||
.unwrap_or_else(|e| panic!("failed to invoke python3 {script}: {e}"));
|
.unwrap_or_else(|e| panic!("failed to invoke python3 {script}: {e}"));
|
||||||
assert!(
|
assert!(
|
||||||
@@ -10375,6 +10520,7 @@ mod tests {
|
|||||||
let out = std::process::Command::new("python3")
|
let out = std::process::Command::new("python3")
|
||||||
.arg(&path)
|
.arg(&path)
|
||||||
.env("GRAPH_STATE", raw_state)
|
.env("GRAPH_STATE", raw_state)
|
||||||
|
.env_remove("GRAPH_STATE_FILE")
|
||||||
.output()
|
.output()
|
||||||
.unwrap_or_else(|e| panic!("failed to invoke python3 {script}: {e}"));
|
.unwrap_or_else(|e| panic!("failed to invoke python3 {script}: {e}"));
|
||||||
assert!(
|
assert!(
|
||||||
@@ -10606,6 +10752,7 @@ mod tests {
|
|||||||
let out = std::process::Command::new("bash")
|
let out = std::process::Command::new("bash")
|
||||||
.arg(&path)
|
.arg(&path)
|
||||||
.env("GRAPH_STATE", state.to_string())
|
.env("GRAPH_STATE", state.to_string())
|
||||||
|
.env_remove("GRAPH_STATE_FILE")
|
||||||
.output()
|
.output()
|
||||||
.unwrap_or_else(|e| panic!("failed to invoke bash {script}: {e}"));
|
.unwrap_or_else(|e| panic!("failed to invoke bash {script}: {e}"));
|
||||||
assert!(
|
assert!(
|
||||||
@@ -10908,6 +11055,7 @@ mod tests {
|
|||||||
let out = std::process::Command::new("python3")
|
let out = std::process::Command::new("python3")
|
||||||
.arg(&path)
|
.arg(&path)
|
||||||
.env("GRAPH_STATE", raw_state)
|
.env("GRAPH_STATE", raw_state)
|
||||||
|
.env_remove("GRAPH_STATE_FILE")
|
||||||
.output()
|
.output()
|
||||||
.unwrap_or_else(|e| panic!("failed to invoke python3 {script}: {e}"));
|
.unwrap_or_else(|e| panic!("failed to invoke python3 {script}: {e}"));
|
||||||
assert!(
|
assert!(
|
||||||
|
|||||||
Reference in new issue
Block a user