fix(missions): stop three launch failures from passing as success
A verification run against the deployed stack found a chain mission whose phase 0 reported `completed` with zero files, no commit error and no push error — indistinguishable from a phase that correctly had nothing to do. Three separate defects had to line up, each of them the same shape: a failure sharing its representation with a legitimate negative result. 1. `pin_agent_workspaces` embedded the whole config in one `sh -c` argv. That works until the file grows — config gains a block per provisioned claw — then fails with `argument list too long`. Now written through the tar upload API, which has no argv limit, so the failure mode is gone rather than merely further away. 2. A failed pin was logged "(continuing)". Without the pin, agents write to their sandboxes and the committer finds nothing in /mission/repo — the mission cannot deliver, so the launch now fails where someone is still looking. The restart that applies the pin is fatal for the same reason. 3. `capture_phase_diff_at` swallowed `git diff` failures with `unwrap_or_default`, so an unreadable base landed `empty: true, files_changed: 0` — byte-identical to an honest no-op. The error is now recorded as `diff_error`, and an empty patch that came from a failed diff is no longer trusted to mean an unchanged tree. Adds scripts/verify-mission-delivery.sh, which found #1 and #2 on its first real run. Its probes are fail-closed: no placeholder values, a self-test that proves the uid probe can detect the split it looks for, and FAIL-NORUN for a scenario that never executed. Its own first version had this bug too — a `die` inside `$(...)` exited the subshell, so a run that could not authenticate printed "all checks passed" and exited 0. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 5
parent
bb274d08c6
commit
1253595ba7
@@ -227,13 +227,41 @@ pub async fn capture_phase_diff_at(
|
||||
// A repo with nothing to add is fine; keep going and let the diff be empty.
|
||||
let _ = git(&repo, &add).await;
|
||||
|
||||
// A failed `git diff` and a phase that changed nothing both yield an empty
|
||||
// string, and `unwrap_or_default` used to erase the difference: a corrupt
|
||||
// index or an unreadable base would land `empty: true, files_changed: 0` —
|
||||
// byte-identical to an honest no-op, and just as quiet. Whatever went
|
||||
// wrong is recorded so the artifact can say which of the two it was.
|
||||
let mut diff_error: Option<String> = None;
|
||||
let mut note_diff_failure = |what: &str, e: String| {
|
||||
eprintln!(
|
||||
"mission_delivery: mission {mission_id} phase {phase_id} could not compute \
|
||||
{what} against {base_sha}: {e}"
|
||||
);
|
||||
if diff_error.is_none() {
|
||||
diff_error = Some(format!("{what}: {}", e.chars().take(300).collect::<String>()));
|
||||
}
|
||||
};
|
||||
|
||||
let mut diff_args = vec!["diff", base_sha.as_str(), "--"];
|
||||
diff_args.extend(excludes.iter().map(String::as_str));
|
||||
let patch = git(&repo, &diff_args).await.unwrap_or_default();
|
||||
let patch = match git(&repo, &diff_args).await {
|
||||
Ok(p) => p,
|
||||
Err(e) => {
|
||||
note_diff_failure("patch", e);
|
||||
String::new()
|
||||
}
|
||||
};
|
||||
|
||||
let mut stat_args = vec!["diff", base_sha.as_str(), "--stat", "--"];
|
||||
stat_args.extend(excludes.iter().map(String::as_str));
|
||||
let diffstat = git(&repo, &stat_args).await.unwrap_or_default();
|
||||
let diffstat = match git(&repo, &stat_args).await {
|
||||
Ok(s) => s,
|
||||
Err(e) => {
|
||||
note_diff_failure("diffstat", e);
|
||||
String::new()
|
||||
}
|
||||
};
|
||||
|
||||
// Put the index back. `--intent-to-add` is a mutation of the agent's
|
||||
// workspace, and capture must not change what a later commit would see.
|
||||
@@ -293,9 +321,14 @@ pub async fn capture_phase_diff_at(
|
||||
// Gate, then publish. Both are best-effort on top of an artifact that has
|
||||
// already landed: a phase whose tests fail, or whose push is rejected,
|
||||
// still has its patch on disk and its work on a local branch.
|
||||
//
|
||||
// `empty` suppresses publishing, so a diff we could not COMPUTE would
|
||||
// otherwise skip the push and leave `push_error: null` — the phase looking
|
||||
// exactly like one that correctly had nothing to publish. See
|
||||
// [`untrusted_empty_reason`].
|
||||
let mut outcome: Option<TestOutcome> = None;
|
||||
let mut published: Option<Publish> = None;
|
||||
let mut publish_error: Option<String> = None;
|
||||
let mut publish_error: Option<String> = untrusted_empty_reason(empty, diff_error.as_deref());
|
||||
if let Some(c) = committed.as_ref() {
|
||||
if !empty {
|
||||
if gate == Gate::OnGreenTests {
|
||||
@@ -379,6 +412,10 @@ pub async fn capture_phase_diff_at(
|
||||
"insertions": insertions,
|
||||
"deletions": deletions,
|
||||
"empty": empty,
|
||||
// Non-null means `empty`/`files_changed` describe a failed read, not
|
||||
// an unchanged tree. Readers that treat `empty: true` as "the phase
|
||||
// did nothing" must check this first.
|
||||
"diff_error": diff_error,
|
||||
"truncated": truncated,
|
||||
"excluded_paths": EXCLUDED_PATHS,
|
||||
});
|
||||
@@ -958,6 +995,24 @@ pub async fn record_uncapturable(
|
||||
.map_err(|e| format!("register uncapturable marker: {e}"))
|
||||
}
|
||||
|
||||
/// Why an empty patch must not be believed, if it must not be believed.
|
||||
///
|
||||
/// An empty patch has two causes that produce identical bytes: the tree really
|
||||
/// did not change, or `git diff` failed and we have no idea what the tree
|
||||
/// looks like. The first is an ordinary outcome; the second is a platform
|
||||
/// fault. Returning `Some` for the second is what stops the fault from being
|
||||
/// filed under the ordinary outcome — the recurring shape where a failure and
|
||||
/// a legitimate negative share one representation.
|
||||
fn untrusted_empty_reason(empty: bool, diff_error: Option<&str>) -> Option<String> {
|
||||
match (empty, diff_error) {
|
||||
(true, Some(why)) => Some(format!(
|
||||
"not published: the diff could not be computed, so an empty patch \
|
||||
cannot be trusted to mean an unchanged tree ({why})"
|
||||
)),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
@@ -1060,6 +1115,28 @@ mod tests {
|
||||
|
||||
/// An empty stat means an empty phase, not a parse failure. This is the
|
||||
/// case that must still produce an artifact.
|
||||
/// The whole point: a tree that genuinely did not change stays silent, and
|
||||
/// a diff that could not be computed does not get to borrow that silence.
|
||||
#[test]
|
||||
fn an_uncomputable_diff_is_not_an_unchanged_tree() {
|
||||
assert_eq!(
|
||||
untrusted_empty_reason(true, None),
|
||||
None,
|
||||
"a genuinely unchanged tree must not report an error"
|
||||
);
|
||||
let reason = untrusted_empty_reason(true, Some("patch: fatal: bad object"))
|
||||
.expect("an empty patch from a FAILED diff must be reported, not accepted");
|
||||
assert!(
|
||||
reason.contains("bad object"),
|
||||
"the reason must name what went wrong, got: {reason}"
|
||||
);
|
||||
assert_eq!(
|
||||
untrusted_empty_reason(false, Some("diffstat: fatal: bad object")),
|
||||
None,
|
||||
"a non-empty patch stands on its own even if the diffstat failed"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_empty_diffstat_is_all_zeroes() {
|
||||
assert_eq!(parse_diffstat(""), (0, 0, 0));
|
||||
|
||||
Reference in New Issue
Block a user