fix(missions): a retry's work is no longer silently destroyed
Two independent bugs, either of which loses everything a retried phase
produced, and neither of which reports a failure.
1. Capture is suppressed forever on a retry. Both
`capture_finished_coding_phases` and the sweeper's last-chance
`capture_outstanding_phases` skip any phase that already has a
`code_diff` artifact. That guard is right for a phase that ran once and
catastrophic for a retried one: the artifact from the FAILED attempt
suppresses capture of the new attempt, the container is reaped on its
normal grace, and everything the agents committed inside it is gone.
The UI keeps showing the old diff, so the mission reads as delivered.
`retry_phase` now clears the reopened phases' captures in the same
transaction that reopens them, which is what makes its own doc comment
("the phase card starts fresh on the retry") true of the artifacts too.
2. `git add` exits non-zero over a gitignored path while staging correctly.
Measured: with a populated `target/`, `git add -- . :(exclude)target`
exits 1 and stages the right files; `-c advice.addIgnoredFile=false`,
`--ignore-errors`, `-A` and `:/` all behave identically. Propagating
that with `?` aborted the commit AFTER a successful staging — no branch,
no commit, no push — for every Rust repo an agent has built in.
`capture_phase_diff_at` already treats the same command as advisory;
the commit path now does too, and the staged index decides.
Mission 01a00538 hit both: it completed research and coding on the retry,
11 agent commits and all, delivered a patch dated the previous day, and
lost the commits when the container was reaped. The remote was never
touched — its HEAD still equalled the mission's own base_sha.
Covered by a test that drives real git and asserts the files are staged
regardless of the exit code.
Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 5
parent
fd5e71ccfe
commit
10341cf7fe
@@ -698,7 +698,26 @@ pub async fn commit_phase_work(
|
||||
.map(|p| format!(":(exclude){p}"))
|
||||
.collect();
|
||||
add.extend(excludes.iter().map(String::as_str));
|
||||
git(repo, &add).await?;
|
||||
// `git add` EXITS 1 whenever the pathspec walked over a gitignored path,
|
||||
// even though it staged everything else correctly and even though we
|
||||
// excluded that path ourselves. Measured against git 2.x: `-c
|
||||
// advice.addIgnoredFile=false`, `--ignore-errors`, `-A` and `:/` all still
|
||||
// exit 1, and all still stage the right files. There is no flag that makes
|
||||
// this command's exit code mean "nothing was staged".
|
||||
//
|
||||
// Propagating it with `?` therefore aborted the commit AFTER a successful
|
||||
// staging, so the branch was never made, nothing was committed and nothing
|
||||
// was pushed — for any repo with a populated `target/`, which is every Rust
|
||||
// repo an agent has built in. `capture_phase_diff_at` above already treats
|
||||
// the same command as advisory (`let _ = ...`); this is the same command
|
||||
// and gets the same policy. The index is the source of truth, and the
|
||||
// `diff --cached` immediately below is what actually reads it.
|
||||
if let Err(e) = git(repo, &add).await {
|
||||
eprintln!(
|
||||
"mission_delivery: `git add` reported {e} — continuing, the staged \
|
||||
index below is what decides whether there is anything to commit"
|
||||
);
|
||||
}
|
||||
|
||||
// `--cached` compares the index against HEAD: empty means the agents left
|
||||
// nothing unstaged for us, which is the normal case when they committed
|
||||
@@ -1324,6 +1343,72 @@ mod tests {
|
||||
assert_eq!(Gate::OnGreenTests.branch_suffix(None), "-wip");
|
||||
}
|
||||
|
||||
/// The regression that destroyed a mission's work.
|
||||
///
|
||||
/// `git add -- . :(exclude)target` EXITS NON-ZERO when the tree contains a
|
||||
/// gitignored `target/`, while correctly staging everything else. The
|
||||
/// commit path used to propagate that exit with `?`, so a successful
|
||||
/// staging still aborted the commit — no branch, no commit, no push — and
|
||||
/// the agents' work was reaped with the container.
|
||||
///
|
||||
/// This test asserts the git behaviour itself, because the fix is only
|
||||
/// correct for as long as the behaviour holds: if a future git makes this
|
||||
/// command exit 0, this test fails and tells the next reader the workaround
|
||||
/// can go. What must never regress is the second assertion — that the files
|
||||
/// ARE staged regardless of the exit code, which is why the index and not
|
||||
/// the exit code is the source of truth.
|
||||
#[tokio::test]
|
||||
async fn git_add_exits_nonzero_over_an_ignored_path_yet_still_stages() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let repo = dir.path();
|
||||
let run = |args: Vec<&str>| {
|
||||
std::process::Command::new("git")
|
||||
.args(&args)
|
||||
.current_dir(repo)
|
||||
.output()
|
||||
.expect("git runs")
|
||||
};
|
||||
run(vec!["init", "-q", "."]);
|
||||
run(vec!["config", "user.email", "[email protected]"]);
|
||||
run(vec!["config", "user.name", "T"]);
|
||||
std::fs::write(repo.join(".gitignore"), "target\n").unwrap();
|
||||
run(vec!["add", ".gitignore"]);
|
||||
run(vec!["commit", "-qm", "base"]);
|
||||
|
||||
// The shape every Rust repo an agent has built in ends up with.
|
||||
std::fs::create_dir_all(repo.join("target")).unwrap();
|
||||
std::fs::write(repo.join("target/build.bin"), "junk").unwrap();
|
||||
std::fs::create_dir_all(repo.join("research")).unwrap();
|
||||
std::fs::write(repo.join("research/summary.md"), "findings").unwrap();
|
||||
|
||||
let mut add: Vec<&str> = vec!["add", "--", "."];
|
||||
let excludes: Vec<String> = EXCLUDED_PATHS
|
||||
.iter()
|
||||
.map(|p| format!(":(exclude){p}"))
|
||||
.collect();
|
||||
add.extend(excludes.iter().map(String::as_str));
|
||||
let out = run(add);
|
||||
|
||||
assert!(
|
||||
!out.status.success(),
|
||||
"if this now succeeds, git changed and the advisory handling in \
|
||||
commit_and_branch can be simplified"
|
||||
);
|
||||
|
||||
let staged = run(vec!["diff", "--cached", "--name-only"]);
|
||||
let staged = String::from_utf8_lossy(&staged.stdout);
|
||||
assert!(
|
||||
staged.contains("research/summary.md"),
|
||||
"the work MUST be staged despite the non-zero exit; this is the \
|
||||
assertion that stops a phase's output being silently discarded \
|
||||
again. staged: {staged:?}"
|
||||
);
|
||||
assert!(
|
||||
!staged.contains("target/"),
|
||||
"the exclude pathspec must still keep build output out: {staged:?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_command_is_discovered_from_the_tree() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
|
||||
Reference in New Issue
Block a user