crashlabs/huskies
0
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 26 Wiki Activity

huskies: merge 651_bug_remove_git_reset_clean_behaviour_from_bug_645_s_recovery_path_uncommitted_work_in_worktrees_is_never_junk

Browse Source
This commit is contained in:
dave
2026-04-26 16:42:10 +00:00
parent 365b907ba4
commit ff51a1a465
2 changed files with 303 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files
server/src/agents/pool/pipeline/completion.rs
+251 -27
View File
@@ -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"
);
}
}