diff --git a/server/src/agents/gates.rs b/server/src/agents/gates.rs index d587a3dc..4c124f1d 100644 --- a/server/src/agents/gates.rs +++ b/server/src/agents/gates.rs @@ -43,22 +43,47 @@ pub(crate) fn worktree_has_committed_work(wt_path: &Path) -> bool { /// Run `cargo check` in the given worktree directory to verify committed code compiles. /// -/// Resets any dirty (uncommitted) files first via `git checkout .` so that only -/// the committed state is evaluated. Used as part of the "work survived" check -/// when an agent crashes mid-output (bug 645). +/// Stashes any dirty (uncommitted) files first so that only the committed state +/// is evaluated, then restores them afterward. Uncommitted work in worktrees is +/// never junk — it may be the next agent session's starting point (bug 651). +/// +/// Used as part of the "work survived" check when an agent crashes mid-output +/// (bug 645). pub(crate) fn cargo_check_in_worktree(wt_path: &Path) -> bool { - // Reset uncommitted changes so cargo check evaluates only committed code. - let _ = Command::new("git") - .args(["checkout", "."]) + // Stash uncommitted changes (including untracked files) so cargo check + // evaluates only committed code. We restore them afterward. + let stashed = Command::new("git") + .args([ + "stash", + "push", + "--include-untracked", + "-m", + "cargo-check-temp", + ]) .current_dir(wt_path) - .output(); + .output() + .map(|o| { + o.status.success() + && !String::from_utf8_lossy(&o.stdout).contains("No local changes to save") + }) + .unwrap_or(false); - Command::new("cargo") + let result = Command::new("cargo") .args(["check"]) .current_dir(wt_path) .output() .map(|o| o.status.success()) - .unwrap_or(false) + .unwrap_or(false); + + // Restore stashed uncommitted changes. + if stashed { + let _ = Command::new("git") + .args(["stash", "pop"]) + .current_dir(wt_path) + .output(); + } + + result } /// Check whether the given directory has any uncommitted git changes. @@ -457,9 +482,10 @@ mod tests { // ── cargo_check_in_worktree tests ──────────────────────────────────────── - /// Bug 645: cargo_check_in_worktree resets dirty files before checking. + /// Bug 645 + 651: cargo_check_in_worktree stashes dirty files before + /// checking committed code and restores them afterward. #[test] - fn cargo_check_in_worktree_resets_dirty_files() { + fn cargo_check_in_worktree_stashes_and_restores_dirty_files() { use std::fs; let tmp = tempfile::tempdir().unwrap(); let project_root = tmp.path().join("project"); @@ -530,11 +556,24 @@ mod tests { // Now simulate a crash leaving dirty files (broken syntax). fs::write(wt_path.join("src/lib.rs"), "THIS IS BROKEN SYNTAX!!!\n").unwrap(); + // Also add an untracked file. + fs::write(wt_path.join("crash_residue.txt"), "untracked junk").unwrap(); - // cargo_check_in_worktree should reset dirty files and check committed code. + // cargo_check_in_worktree should stash dirty files, check committed code, and restore. assert!( cargo_check_in_worktree(&wt_path), - "cargo check should pass on committed code after resetting dirty files" + "cargo check should pass on committed code after stashing dirty files" + ); + + // Bug 651: dirty files must be restored after cargo check. + assert_eq!( + fs::read_to_string(wt_path.join("src/lib.rs")).unwrap(), + "THIS IS BROKEN SYNTAX!!!\n", + "modified tracked file should be restored after cargo check" + ); + assert!( + wt_path.join("crash_residue.txt").exists(), + "untracked file should be restored after cargo check" ); } } diff --git a/server/src/agents/pool/pipeline/completion.rs b/server/src/agents/pool/pipeline/completion.rs index 9d53a7f9..4fbd6fc8 100644 --- a/server/src/agents/pool/pipeline/completion.rs +++ b/server/src/agents/pool/pipeline/completion.rs @@ -235,31 +235,51 @@ pub(in crate::agents::pool) async fn run_server_owned_completion( // If the worktree is dirty, check whether committed work survived. // An agent crash (e.g. Claude Code CLI's `output.write(&bytes).is_ok()` // assertion — bug 645) can leave uncommitted files behind even though - // the agent already committed valid work. In that case, clean up the + // the agent already committed valid work. In that case, stash the // dirty files and proceed with gates on the committed code. - if let Err(dirty_msg) = crate::agents::gates::check_uncommitted_changes(&path) { - if crate::agents::gates::worktree_has_committed_work(&path) { - crate::slog!( - "[agents] Worktree dirty but committed work exists — \ - resetting uncommitted changes and proceeding with gates. \ + // Uncommitted work is never junk — it may be the next agent session's + // starting point (bug 651). + let stashed = + if let Err(dirty_msg) = crate::agents::gates::check_uncommitted_changes(&path) { + if crate::agents::gates::worktree_has_committed_work(&path) { + crate::slog!( + "[agents] Worktree dirty but committed work exists — \ + stashing uncommitted changes and proceeding with gates. \ Dirty state: {dirty_msg}" - ); - // Reset dirty files so gates run against committed code only. - let _ = std::process::Command::new("git") - .args(["checkout", "."]) - .current_dir(&path) - .output(); - let _ = std::process::Command::new("git") - .args(["clean", "-fd"]) - .current_dir(&path) - .output(); + ); + // Stash dirty files so gates run against committed code only. + // They will be restored after gates complete. + std::process::Command::new("git") + .args([ + "stash", + "push", + "--include-untracked", + "-m", + "server-completion-temp", + ]) + .current_dir(&path) + .output() + .map(|o| { + o.status.success() + && !String::from_utf8_lossy(&o.stdout) + .contains("No local changes to save") + }) + .unwrap_or(false) + } else { + return Ok((false, dirty_msg)); + } } else { - return Ok((false, dirty_msg)); - } - } + false + }; // AC5: Fail early if the coder finished with no commits on the feature branch. // This prevents empty-diff stories from advancing through QA to merge. if !crate::agents::gates::worktree_has_committed_work(&path) { + if stashed { + let _ = std::process::Command::new("git") + .args(["stash", "pop"]) + .current_dir(&path) + .output(); + } return Ok(( false, "Agent exited with no commits on the feature branch. \ @@ -267,7 +287,15 @@ pub(in crate::agents::pool) async fn run_server_owned_completion( .to_string(), )); } - crate::agents::gates::run_acceptance_gates(&path) + let result = crate::agents::gates::run_acceptance_gates(&path); + // Restore stashed uncommitted changes. + if stashed { + let _ = std::process::Command::new("git") + .args(["stash", "pop"]) + .current_dir(&path) + .output(); + } + result }) .await { @@ -667,11 +695,11 @@ mod tests { ); } - /// Bug 645: when an agent crashes leaving dirty files but committed work, - /// server-owned completion should clean up the dirty files and run gates - /// on the committed code instead of failing immediately. + /// Bug 645 + 651: when an agent crashes leaving dirty files but committed + /// work, server-owned completion should stash the dirty files during gates + /// and restore them afterward. Uncommitted work is never junk. #[tokio::test] - async fn server_owned_completion_cleans_dirty_worktree_with_committed_work() { + async fn server_owned_completion_preserves_dirty_worktree_with_committed_work() { use std::fs; use tempfile::tempdir; @@ -730,10 +758,15 @@ mod tests { ) .await; - // The dirty file should have been cleaned up. + // Bug 651: The dirty file must be PRESERVED — uncommitted work is never junk. assert!( - !wt_path.join("dirty.txt").exists(), - "dirty file should be cleaned up after server-owned completion" + wt_path.join("dirty.txt").exists(), + "dirty file should be preserved after server-owned completion (bug 651)" + ); + assert_eq!( + fs::read_to_string(wt_path.join("dirty.txt")).unwrap(), + "crash residue", + "dirty file contents should be unchanged" ); // A Done event should have been emitted (completion ran, didn't fail @@ -744,4 +777,195 @@ mod tests { "expected Done event, got: {event:?}" ); } + + /// AC3 (bug 651): simulate an agent killed by the watchdog with a + /// substantive uncommitted diff. After the orchestrator's full + /// post-termination handling (gates, completion, advance check), + /// `git status --short` must still show the same modified files. + #[tokio::test] + async fn watchdog_kill_preserves_uncommitted_diff() { + use std::fs; + use tempfile::tempdir; + + let tmp = tempdir().unwrap(); + let project_root = tmp.path().join("project"); + fs::create_dir_all(&project_root).unwrap(); + init_git_repo(&project_root); + + // Create a worktree on a feature branch with committed code. + let wt_path = tmp.path().join("wt"); + Command::new("git") + .args([ + "worktree", + "add", + &wt_path.to_string_lossy(), + "-b", + "feature/story-651_watchdog", + ]) + .current_dir(&project_root) + .output() + .unwrap(); + + // Commit some work. + fs::write(wt_path.join("committed.txt"), "committed work").unwrap(); + Command::new("git") + .args(["add", "."]) + .current_dir(&wt_path) + .output() + .unwrap(); + Command::new("git") + .args(["commit", "-m", "coder: add committed work"]) + .current_dir(&wt_path) + .output() + .unwrap(); + + // Simulate substantive uncommitted diff left by watchdog kill. + fs::write(wt_path.join("in_progress.rs"), "fn wip() {}").unwrap(); + fs::write(wt_path.join("committed.txt"), "modified after commit").unwrap(); + + // Snapshot git status before completion. + let status_before = Command::new("git") + .args(["status", "--short"]) + .current_dir(&wt_path) + .output() + .unwrap(); + let status_before = String::from_utf8_lossy(&status_before.stdout) + .trim() + .to_string(); + assert!( + !status_before.is_empty(), + "pre-condition: worktree should be dirty" + ); + + let pool = AgentPool::new_test(3001); + pool.inject_test_agent_with_path( + "651_watchdog", + "coder-1", + AgentStatus::Running, + wt_path.clone(), + ); + + run_server_owned_completion( + &pool.agents, + pool.port, + "651_watchdog", + "coder-1", + Some("sess-651".to_string()), + pool.watcher_tx.clone(), + ) + .await; + + // After full post-termination handling, git status must be unchanged. + let status_after = Command::new("git") + .args(["status", "--short"]) + .current_dir(&wt_path) + .output() + .unwrap(); + let status_after = String::from_utf8_lossy(&status_after.stdout) + .trim() + .to_string(); + + assert_eq!( + status_before, status_after, + "Bug 651: uncommitted diff must survive post-termination handling.\n\ + Before: {status_before}\nAfter: {status_after}" + ); + + // Verify file contents are intact. + assert_eq!( + fs::read_to_string(wt_path.join("in_progress.rs")).unwrap(), + "fn wip() {}", + ); + assert_eq!( + fs::read_to_string(wt_path.join("committed.txt")).unwrap(), + "modified after commit", + ); + } + + /// AC4 (bug 651 regression for 645): when an agent crashes with committed + /// work AND uncommitted noise, the auto-advance still picks up the + /// committed work. The committed-state check is authoritative; the + /// uncommitted state is just preserved on disk for the next agent. + #[tokio::test] + async fn committed_work_advances_despite_uncommitted_noise() { + use std::fs; + use tempfile::tempdir; + + let tmp = tempdir().unwrap(); + let project_root = tmp.path().join("project"); + fs::create_dir_all(&project_root).unwrap(); + init_git_repo(&project_root); + + // Create a minimal Cargo project so cargo check works. + fs::write( + project_root.join("Cargo.toml"), + "[package]\nname = \"test_proj\"\nversion = \"0.1.0\"\nedition = \"2021\"\n", + ) + .unwrap(); + fs::create_dir_all(project_root.join("src")).unwrap(); + fs::write(project_root.join("src/lib.rs"), "// empty\n").unwrap(); + Command::new("git") + .args(["add", "."]) + .current_dir(&project_root) + .output() + .unwrap(); + Command::new("git") + .args(["commit", "-m", "add cargo project"]) + .current_dir(&project_root) + .output() + .unwrap(); + + // Create a worktree on a feature branch. + let wt_path = tmp.path().join("wt"); + Command::new("git") + .args([ + "worktree", + "add", + &wt_path.to_string_lossy(), + "-b", + "feature/story-651_regression", + ]) + .current_dir(&project_root) + .output() + .unwrap(); + + // Commit valid code on the feature branch. + fs::write(wt_path.join("src/lib.rs"), "pub fn good() {}\n").unwrap(); + Command::new("git") + .args(["add", "."]) + .current_dir(&wt_path) + .output() + .unwrap(); + Command::new("git") + .args(["commit", "-m", "add good fn"]) + .current_dir(&wt_path) + .output() + .unwrap(); + + // Simulate crash leaving uncommitted noise (broken syntax in tracked file). + fs::write(wt_path.join("src/lib.rs"), "THIS IS BROKEN SYNTAX!!!\n").unwrap(); + fs::write(wt_path.join("crash_junk.tmp"), "untracked noise").unwrap(); + + // The "work survived" check should detect committed work and pass cargo check + // despite the dirty worktree, WITHOUT destroying the dirty files. + assert!( + crate::agents::gates::worktree_has_committed_work(&wt_path), + "committed work should be detected" + ); + assert!( + crate::agents::gates::cargo_check_in_worktree(&wt_path), + "cargo check should pass on committed code (stash/pop, not reset)" + ); + + // Dirty files must still exist after the check. + assert_eq!( + fs::read_to_string(wt_path.join("src/lib.rs")).unwrap(), + "THIS IS BROKEN SYNTAX!!!\n", + "uncommitted noise in tracked file must be preserved" + ); + assert!( + wt_path.join("crash_junk.tmp").exists(), + "untracked noise file must be preserved" + ); + } }