fix(missions): a gate that gave up completed the phase green
`done_when_check` is run in exactly one place: the Stop hook inside the guest. Nothing outside it has ever re-run the command — not the evaluator (which judges the PROSE `done_when`), not capture, not delivery. The hook is capped at MAX_BLOCKS so a stuck agent cannot wedge the turn. At the cap it logs `cap: <reason>` and exits 0, releasing the agent with its check still failing. That release was invisible: rc was 0 and the work collected, so both signals the run status was decided from said "fine", and the phase completed. Green phase, unmet condition, no error anywhere — the same silent-success shape this project keeps paying for. The block COUNT cannot fix it. Three blocks then a stop that finally passed and three blocks then a surrender both report `blocks: 3`, and they are opposite outcomes. So the gate now writes a `capped` marker file, probed back out of the guest alongside the block count, and `Some(true)` fails the run on BOTH paths — solo (phase_runner) and composed (microvm_turn_executor). A marker file rather than grepping the log: a block reason embeds the check's own output, so an output line starting `cap:` would read as a release that never happened. Also corrects the comment in `per_node` that sent me looking. It claimed "the phase-level check still runs post-hoc", conflating two mechanisms — that is true of `require_changes` (via `empty_delivery_is_a_failure`) and was never true of `check`. Negative controls, both ablated and confirmed failing: drop the enforcement and `a_node_whose_gate_gave_up_fails_the_run` fails; stop writing the marker and `a_gate_that_gives_up_records_that_it_gave_up` fails. And the control against over-strictness — `a_node_that_was_blocked_and_then_succeeded_passes` — is why this keys on the marker instead of the count. 231 lib tests pass.
This commit is contained in:
@@ -309,6 +309,14 @@ const SETTINGS_PROBE: &str = "claude --help 2>&1 | grep -q -- '--settings' && ec
|
|||||||
/// How many times the stop gate refused to let the agent finish.
|
/// How many times the stop gate refused to let the agent finish.
|
||||||
const BLOCKS_PROBE: &str = "cat /root/gate/blocks 2>/dev/null || echo 0";
|
const BLOCKS_PROBE: &str = "cat /root/gate/blocks 2>/dev/null || echo 0";
|
||||||
|
|
||||||
|
/// Did the gate give up and release the agent with its condition still failing?
|
||||||
|
///
|
||||||
|
/// Asked separately from [`BLOCKS_PROBE`] because the count cannot answer it —
|
||||||
|
/// see [`crate::vm_stop_gate::CAPPED_FILE`]. `cat` of a missing file prints
|
||||||
|
/// nothing and exits non-zero, so the `||` supplies the "no" that a gate which
|
||||||
|
/// never capped never wrote.
|
||||||
|
const CAPPED_PROBE: &str = "cat /root/gate/capped 2>/dev/null || echo 0";
|
||||||
|
|
||||||
/// The command that runs the agent in the guest.
|
/// The command that runs the agent in the guest.
|
||||||
///
|
///
|
||||||
/// `settings` is the path to the stop-gate settings file, when a gate is
|
/// `settings` is the path to the stop-gate settings file, when a gate is
|
||||||
@@ -361,6 +369,14 @@ pub struct VmOutcome {
|
|||||||
/// no gate was installed. Zero means the agent got it right first time,
|
/// no gate was installed. Zero means the agent got it right first time,
|
||||||
/// which is a different fact from "there was no gate".
|
/// which is a different fact from "there was no gate".
|
||||||
pub stop_blocks: Option<u32>,
|
pub stop_blocks: Option<u32>,
|
||||||
|
/// Whether the gate ran out of blocks and let the agent stop anyway, with
|
||||||
|
/// the phase's `done_when_check` still exiting non-zero (or the tree still
|
||||||
|
/// unchanged). `None` when no gate was installed, or the probe could not run
|
||||||
|
/// — neither of which is "it did not cap".
|
||||||
|
///
|
||||||
|
/// `Some(true)` is a FAILED turn. The gate is the only thing that ever runs
|
||||||
|
/// a `done_when_check`, so if it gave up, nothing downstream will notice.
|
||||||
|
pub released_at_cap: Option<bool>,
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Boot a VM, run the phase in it, collect the result, and destroy it.
|
/// Boot a VM, run the phase in it, collect the result, and destroy it.
|
||||||
@@ -642,6 +658,18 @@ async fn run_inside(
|
|||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
|
||||||
|
// Probed only when a gate existed, so `None` never reads as "it did not cap".
|
||||||
|
let released_at_cap = match settings {
|
||||||
|
None => None,
|
||||||
|
Some(_) => match vm.exec(CAPPED_PROBE, None, 60, &[]).await {
|
||||||
|
Ok(p) => Some(p.stdout.trim() == "1"),
|
||||||
|
Err(e) => {
|
||||||
|
eprintln!("microvm_executor: cap probe failed on {}: {e}", vm.vm_id());
|
||||||
|
None
|
||||||
|
}
|
||||||
|
},
|
||||||
|
};
|
||||||
|
|
||||||
Ok(VmOutcome {
|
Ok(VmOutcome {
|
||||||
summary: format!("{}{}", out.stdout, out.stderr),
|
summary: format!("{}{}", out.stdout, out.stderr),
|
||||||
rc: out.rc,
|
rc: out.rc,
|
||||||
@@ -649,6 +677,7 @@ async fn run_inside(
|
|||||||
subagents,
|
subagents,
|
||||||
teammates,
|
teammates,
|
||||||
stop_blocks,
|
stop_blocks,
|
||||||
|
released_at_cap,
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -217,6 +217,19 @@ impl<V: PhaseVm> TurnExecutor for MicroVmTurnExecutor<V> {
|
|||||||
outcome.summary.chars().take(400).collect::<String>()
|
outcome.summary.chars().take(400).collect::<String>()
|
||||||
)));
|
)));
|
||||||
}
|
}
|
||||||
|
// Same rule as the solo path: the gate is the only thing that runs a
|
||||||
|
// `done_when_check`, so a release at the cap must fail the run rather
|
||||||
|
// than hand the next node a tree that does not satisfy the condition
|
||||||
|
// every node in this graph was told to satisfy.
|
||||||
|
if outcome.released_at_cap == Some(true) {
|
||||||
|
return Err(OrchestratorError::Executor(format!(
|
||||||
|
"node {}'s completion gate released it after {} refusal(s) with its check \
|
||||||
|
still failing: {}",
|
||||||
|
req.node_id,
|
||||||
|
crate::vm_stop_gate::MAX_BLOCKS,
|
||||||
|
outcome.summary.chars().take(400).collect::<String>()
|
||||||
|
)));
|
||||||
|
}
|
||||||
if outcome.rc != 0 {
|
if outcome.rc != 0 {
|
||||||
return Err(OrchestratorError::Executor(format!(
|
return Err(OrchestratorError::Executor(format!(
|
||||||
"node {} exited {}: {}",
|
"node {} exited {}: {}",
|
||||||
@@ -376,6 +389,7 @@ mod tests {
|
|||||||
subagents: Some(0),
|
subagents: Some(0),
|
||||||
teammates: None,
|
teammates: None,
|
||||||
stop_blocks: None,
|
stop_blocks: None,
|
||||||
|
released_at_cap: None,
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -567,6 +581,7 @@ mod tests {
|
|||||||
subagents: None,
|
subagents: None,
|
||||||
teammates: None,
|
teammates: None,
|
||||||
stop_blocks: None,
|
stop_blocks: None,
|
||||||
|
released_at_cap: None,
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -576,6 +591,64 @@ mod tests {
|
|||||||
assert!(err.contains("could not be collected"), "{err}");
|
assert!(err.contains("could not be collected"), "{err}");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// A node whose gate gave up is a FAILED run, not a completed one.
|
||||||
|
///
|
||||||
|
/// The gate is the only thing in the system that ever runs a
|
||||||
|
/// `done_when_check`. If it releases the agent at the cap and this returns
|
||||||
|
/// Ok, the check's failure is never seen again: the node reports success,
|
||||||
|
/// the next node builds on a tree that does not satisfy the condition, and
|
||||||
|
/// the phase completes green. `rc` is 0 and the work IS collected here on
|
||||||
|
/// purpose — those are the two signals that used to decide this, and both
|
||||||
|
/// say "fine".
|
||||||
|
#[tokio::test]
|
||||||
|
async fn a_node_whose_gate_gave_up_fails_the_run() {
|
||||||
|
struct Capped;
|
||||||
|
impl PhaseVm for Capped {
|
||||||
|
async fn run(&self, _p: VmPhase<'_>) -> Result<VmOutcome, String> {
|
||||||
|
Ok(VmOutcome {
|
||||||
|
summary: "I could not get the tests passing, but here is what I did".into(),
|
||||||
|
rc: 0,
|
||||||
|
collected: true,
|
||||||
|
subagents: None,
|
||||||
|
teammates: None,
|
||||||
|
stop_blocks: Some(crate::vm_stop_gate::MAX_BLOCKS),
|
||||||
|
released_at_cap: Some(true),
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
let d = a_checkout();
|
||||||
|
let e = exec(Capped, d.path().join("repo"));
|
||||||
|
let err = e.run_turn(req("n1", "worker", vec![])).await.unwrap_err().to_string();
|
||||||
|
assert!(err.contains("released it after"), "{err}");
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The negative control: the SAME number of blocks, without the cap. An
|
||||||
|
/// agent that was refused three times and then got it right on the fourth
|
||||||
|
/// try has succeeded, and reports `blocks: 3` exactly like the test above.
|
||||||
|
/// Failing on the count instead of the mark would fail this healthy run.
|
||||||
|
#[tokio::test]
|
||||||
|
async fn a_node_that_was_blocked_and_then_succeeded_passes() {
|
||||||
|
struct Recovered;
|
||||||
|
impl PhaseVm for Recovered {
|
||||||
|
async fn run(&self, _p: VmPhase<'_>) -> Result<VmOutcome, String> {
|
||||||
|
Ok(VmOutcome {
|
||||||
|
summary: "took me a few tries".into(),
|
||||||
|
rc: 0,
|
||||||
|
collected: true,
|
||||||
|
subagents: None,
|
||||||
|
teammates: None,
|
||||||
|
stop_blocks: Some(crate::vm_stop_gate::MAX_BLOCKS),
|
||||||
|
released_at_cap: Some(false),
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
let d = a_checkout();
|
||||||
|
let e = exec(Recovered, d.path().join("repo"));
|
||||||
|
e.run_turn(req("n1", "worker", vec![]))
|
||||||
|
.await
|
||||||
|
.expect("a run that recovered inside its own turn is a success");
|
||||||
|
}
|
||||||
|
|
||||||
/// A node must be told its predecessors' files are already in the tree.
|
/// A node must be told its predecessors' files are already in the tree.
|
||||||
/// Given only the text, an agent re-does work it is standing on.
|
/// Given only the text, an agent re-does work it is standing on.
|
||||||
#[test]
|
#[test]
|
||||||
|
|||||||
@@ -936,6 +936,20 @@ async fn launch_microvm_phase(
|
|||||||
Err(_) => "-".into(),
|
Err(_) => "-".into(),
|
||||||
};
|
};
|
||||||
let (status, note) = match outcome {
|
let (status, note) = match outcome {
|
||||||
|
// The gate gave up. It is the ONLY thing that runs a
|
||||||
|
// `done_when_check`, so a release at the cap means the phase's own
|
||||||
|
// completion condition was still failing when the agent stopped and
|
||||||
|
// nothing downstream will ever re-run it. Completing here is the
|
||||||
|
// silent-success shape: green phase, unmet condition, no error.
|
||||||
|
Ok(o) if o.released_at_cap == Some(true) => (
|
||||||
|
"failed",
|
||||||
|
format!(
|
||||||
|
"the completion gate released the agent after {} refusal(s) with its \
|
||||||
|
check still failing: {}",
|
||||||
|
crate::vm_stop_gate::MAX_BLOCKS,
|
||||||
|
o.summary
|
||||||
|
),
|
||||||
|
),
|
||||||
Ok(o) if o.rc == 0 && o.collected => ("completed", o.summary),
|
Ok(o) if o.rc == 0 && o.collected => ("completed", o.summary),
|
||||||
// A turn that ran and could not be collected is a failure even when
|
// A turn that ran and could not be collected is a failure even when
|
||||||
// the agent was satisfied: the work did not reach the host, so there
|
// the agent was satisfied: the work did not reach the host, so there
|
||||||
|
|||||||
@@ -43,6 +43,19 @@ use serde_json::json;
|
|||||||
/// → "your check passes" without ever approaching the turn budget.
|
/// → "your check passes" without ever approaching the turn budget.
|
||||||
pub const MAX_BLOCKS: u32 = 3;
|
pub const MAX_BLOCKS: u32 = 3;
|
||||||
|
|
||||||
|
/// The file the gate writes when it gives up and lets the agent stop with its
|
||||||
|
/// condition still failing.
|
||||||
|
///
|
||||||
|
/// A separate file rather than a line in the log, because the log is not
|
||||||
|
/// parseable for this: a block reason embeds the check's own output, and an
|
||||||
|
/// output line beginning `cap:` would read as a cap release that never happened.
|
||||||
|
///
|
||||||
|
/// It exists because the block COUNT cannot answer the question. Three blocks
|
||||||
|
/// followed by a stop that finally passed, and three blocks followed by a
|
||||||
|
/// release at the cap, both report `blocks: 3` — and they are opposite outcomes.
|
||||||
|
/// Without this, the second one completed the phase green.
|
||||||
|
pub const CAPPED_FILE: &str = "capped";
|
||||||
|
|
||||||
/// Where the gate lives in the guest.
|
/// Where the gate lives in the guest.
|
||||||
///
|
///
|
||||||
/// Under `/root`, never under the repository. Anything written into
|
/// Under `/root`, never under the repository. Anything written into
|
||||||
@@ -93,8 +106,14 @@ impl StopGate {
|
|||||||
/// `require_changes` is a property of the phase, not of every node in it: a
|
/// `require_changes` is a property of the phase, not of every node in it: a
|
||||||
/// graph whose second node reviews or verifies is *supposed* to leave the
|
/// graph whose second node reviews or verifies is *supposed* to leave the
|
||||||
/// tree alone, and a per-node gate would refuse its stop three times for
|
/// tree alone, and a per-node gate would refuse its stop three times for
|
||||||
/// doing exactly its job. The phase-level check still runs post-hoc against
|
/// doing exactly its job. Dropping it loses nothing, because
|
||||||
/// what the last node collected, so nothing is lost — only misapplied.
|
/// `empty_delivery_is_a_failure` applies the same rule post-hoc to what the
|
||||||
|
/// phase as a whole delivered.
|
||||||
|
///
|
||||||
|
/// That "post-hoc" claim used to be written as covering the `check` too. It
|
||||||
|
/// did not: nothing outside this hook has ever re-run `done_when_check`, so
|
||||||
|
/// a release at [`MAX_BLOCKS`] completed the phase green with the check
|
||||||
|
/// still failing. [`CAPPED_FILE`] is what closes that.
|
||||||
///
|
///
|
||||||
/// A declared `check` DOES apply per node: it is a command the phase author
|
/// A declared `check` DOES apply per node: it is a command the phase author
|
||||||
/// wrote, and every stage of the work should satisfy it.
|
/// wrote, and every stage of the work should satisfy it.
|
||||||
@@ -153,6 +172,7 @@ impl StopGate {
|
|||||||
"if [ -z \"$reason\" ]; then echo pass >> \"$GATE/log\"; exit 0; fi\n\
|
"if [ -z \"$reason\" ]; then echo pass >> \"$GATE/log\"; exit 0; fi\n\
|
||||||
if [ \"$N\" -ge \"$MAX\" ]; then\n\
|
if [ \"$N\" -ge \"$MAX\" ]; then\n\
|
||||||
\x20 echo \"cap: $reason\" >> \"$GATE/log\"\n\
|
\x20 echo \"cap: $reason\" >> \"$GATE/log\"\n\
|
||||||
|
\x20 echo 1 > \"$GATE/capped\"\n\
|
||||||
\x20 exit 0\n\
|
\x20 exit 0\n\
|
||||||
fi\n\
|
fi\n\
|
||||||
N=$((N+1)); echo \"$N\" > \"$GATE/blocks\"\n\
|
N=$((N+1)); echo \"$N\" > \"$GATE/blocks\"\n\
|
||||||
@@ -184,7 +204,8 @@ impl StopGate {
|
|||||||
/// not be.
|
/// not be.
|
||||||
pub fn install_command(&self, repo: &str, dir: &str) -> String {
|
pub fn install_command(&self, repo: &str, dir: &str) -> String {
|
||||||
format!(
|
format!(
|
||||||
"mkdir -p {d} && rm -f {d}/blocks {d}/log && printf '%s' {script} > {d}/stop-gate.sh \
|
"mkdir -p {d} && rm -f {d}/blocks {d}/log {d}/capped \
|
||||||
|
&& printf '%s' {script} > {d}/stop-gate.sh \
|
||||||
&& chmod +x {d}/stop-gate.sh && printf '%s' {settings} > {d}/settings.json",
|
&& chmod +x {d}/stop-gate.sh && printf '%s' {settings} > {d}/settings.json",
|
||||||
d = dir,
|
d = dir,
|
||||||
script = q(&self.script(repo, dir)),
|
script = q(&self.script(repo, dir)),
|
||||||
@@ -269,6 +290,68 @@ mod tests {
|
|||||||
assert_eq!(out.status.code(), Some(0), "{:?}", out);
|
assert_eq!(out.status.code(), Some(0), "{:?}", out);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// The gate gives up after [`MAX_BLOCKS`] and lets the agent stop — and it
|
||||||
|
/// must LEAVE A MARK when it does. Nothing outside this hook ever runs a
|
||||||
|
/// `done_when_check`, so a silent release completed the phase green with its
|
||||||
|
/// condition still failing.
|
||||||
|
///
|
||||||
|
/// The two files say different things and both are needed: `blocks` reaches
|
||||||
|
/// 3 in this test AND in a run where the agent got it right on the fourth
|
||||||
|
/// try, so the count alone cannot tell success from surrender.
|
||||||
|
#[test]
|
||||||
|
fn a_gate_that_gives_up_records_that_it_gave_up() {
|
||||||
|
let tmp = tempfile::tempdir().unwrap();
|
||||||
|
let dir = tmp.path().display().to_string();
|
||||||
|
let repo = repo_with_base(tmp.path());
|
||||||
|
let gate = StopGate {
|
||||||
|
require_changes: false,
|
||||||
|
check: Some("exit 1".into()),
|
||||||
|
};
|
||||||
|
let script = gate.script(&repo.display().to_string(), &dir);
|
||||||
|
|
||||||
|
for n in 1..=MAX_BLOCKS {
|
||||||
|
let out = sh(&script, tmp.path());
|
||||||
|
assert_eq!(out.status.code(), Some(2), "block {n} must refuse the stop");
|
||||||
|
assert!(
|
||||||
|
!tmp.path().join(CAPPED_FILE).exists(),
|
||||||
|
"the cap mark must not appear while the gate is still blocking"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
// One more stop: the gate is out of blocks and must let the agent go.
|
||||||
|
let out = sh(&script, tmp.path());
|
||||||
|
assert_eq!(out.status.code(), Some(0), "at the cap the stop is allowed");
|
||||||
|
assert_eq!(
|
||||||
|
std::fs::read_to_string(tmp.path().join(CAPPED_FILE))
|
||||||
|
.unwrap()
|
||||||
|
.trim(),
|
||||||
|
"1",
|
||||||
|
"the release must be recorded, or nothing downstream can see it"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The negative control for the mark: a gate whose check PASSES releases the
|
||||||
|
/// agent too, and that release must not be recorded as a surrender. Without
|
||||||
|
/// this, "always write the file" would pass the test above and fail every
|
||||||
|
/// healthy phase in production.
|
||||||
|
#[test]
|
||||||
|
fn a_gate_that_is_satisfied_leaves_no_cap_mark() {
|
||||||
|
let tmp = tempfile::tempdir().unwrap();
|
||||||
|
let repo = repo_with_base(tmp.path());
|
||||||
|
let gate = StopGate {
|
||||||
|
require_changes: false,
|
||||||
|
check: Some("true".into()),
|
||||||
|
};
|
||||||
|
let script = gate.script(&repo.display().to_string(), &tmp.path().display().to_string());
|
||||||
|
|
||||||
|
let out = sh(&script, tmp.path());
|
||||||
|
assert_eq!(out.status.code(), Some(0));
|
||||||
|
assert!(
|
||||||
|
!tmp.path().join(CAPPED_FILE).exists(),
|
||||||
|
"a satisfied gate must not look like one that gave up"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
/// And committed work counts too. An agent that committed leaves a CLEAN
|
/// And committed work counts too. An agent that committed leaves a CLEAN
|
||||||
/// tree, so a gate that only looked at `git status` would refuse the stop of
|
/// tree, so a gate that only looked at `git status` would refuse the stop of
|
||||||
/// a phase that had done everything asked of it.
|
/// a phase that had done everything asked of it.
|
||||||
|
|||||||
Reference in New Issue
Block a user