huskies: merge 793
This commit is contained in:
dave
@@ -0,0 +1,583 @@
|
||||
use super::super::super::AgentPool;
|
||||
use super::*;
|
||||
use crate::agents::{AgentEvent, AgentStatus, CompletionReport};
|
||||
use std::path::PathBuf;
|
||||
use std::process::Command;
|
||||
|
||||
fn init_git_repo(repo: &std::path::Path) {
|
||||
Command::new("git")
|
||||
.args(["init"])
|
||||
.current_dir(repo)
|
||||
.output()
|
||||
.unwrap();
|
||||
Command::new("git")
|
||||
.args(["config", "user.email", "test@test.com"])
|
||||
.current_dir(repo)
|
||||
.output()
|
||||
.unwrap();
|
||||
Command::new("git")
|
||||
.args(["config", "user.name", "Test"])
|
||||
.current_dir(repo)
|
||||
.output()
|
||||
.unwrap();
|
||||
Command::new("git")
|
||||
.args(["commit", "--allow-empty", "-m", "init"])
|
||||
.current_dir(repo)
|
||||
.output()
|
||||
.unwrap();
|
||||
}
|
||||
|
||||
// ── report_completion tests ────────────────────────────────────
|
||||
|
||||
#[tokio::test]
|
||||
async fn report_completion_rejects_nonexistent_agent() {
|
||||
let pool = AgentPool::new_test(3001);
|
||||
let result = pool.report_completion("no_story", "no_bot", "done").await;
|
||||
assert!(result.is_err());
|
||||
let msg = result.unwrap_err();
|
||||
assert!(msg.contains("No agent"), "unexpected: {msg}");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn report_completion_rejects_non_running_agent() {
|
||||
let pool = AgentPool::new_test(3001);
|
||||
pool.inject_test_agent("s6", "bot", AgentStatus::Completed);
|
||||
|
||||
let result = pool.report_completion("s6", "bot", "done").await;
|
||||
assert!(result.is_err());
|
||||
let msg = result.unwrap_err();
|
||||
assert!(
|
||||
msg.contains("not running"),
|
||||
"expected 'not running' in: {msg}"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn report_completion_rejects_dirty_worktree() {
|
||||
use std::fs;
|
||||
use tempfile::tempdir;
|
||||
|
||||
let tmp = tempdir().unwrap();
|
||||
let repo = tmp.path();
|
||||
|
||||
// Init a real git repo and make an initial commit
|
||||
Command::new("git")
|
||||
.args(["init"])
|
||||
.current_dir(repo)
|
||||
.output()
|
||||
.unwrap();
|
||||
Command::new("git")
|
||||
.args(["commit", "--allow-empty", "-m", "init"])
|
||||
.current_dir(repo)
|
||||
.output()
|
||||
.unwrap();
|
||||
|
||||
// Write an uncommitted file
|
||||
fs::write(repo.join("dirty.txt"), "not committed").unwrap();
|
||||
|
||||
let pool = AgentPool::new_test(3001);
|
||||
pool.inject_test_agent_with_path("s7", "bot", AgentStatus::Running, repo.to_path_buf());
|
||||
|
||||
let result = pool.report_completion("s7", "bot", "done").await;
|
||||
assert!(result.is_err());
|
||||
let msg = result.unwrap_err();
|
||||
assert!(
|
||||
msg.contains("uncommitted"),
|
||||
"expected 'uncommitted' in: {msg}"
|
||||
);
|
||||
}
|
||||
|
||||
// ── server-owned completion tests ───────────────────────────────────────────
|
||||
|
||||
#[tokio::test]
|
||||
async fn server_owned_completion_skips_when_already_completed() {
|
||||
let pool = AgentPool::new_test(3001);
|
||||
let report = CompletionReport {
|
||||
summary: "Already done".to_string(),
|
||||
gates_passed: true,
|
||||
gate_output: String::new(),
|
||||
};
|
||||
pool.inject_test_agent_with_completion(
|
||||
"s10",
|
||||
"coder-1",
|
||||
AgentStatus::Completed,
|
||||
PathBuf::from("/tmp/nonexistent"),
|
||||
report,
|
||||
);
|
||||
|
||||
// Subscribe before calling so we can check if Done event was emitted.
|
||||
let mut rx = pool.subscribe("s10", "coder-1").unwrap();
|
||||
|
||||
run_server_owned_completion(
|
||||
&pool.agents,
|
||||
pool.port,
|
||||
"s10",
|
||||
"coder-1",
|
||||
Some("sess-1".to_string()),
|
||||
pool.watcher_tx.clone(),
|
||||
)
|
||||
.await;
|
||||
|
||||
// Status should remain Completed (unchanged) — no gate re-run.
|
||||
let agents = pool.agents.lock().unwrap();
|
||||
let key = super::super::super::composite_key("s10", "coder-1");
|
||||
let agent = agents.get(&key).unwrap();
|
||||
assert_eq!(agent.status, AgentStatus::Completed);
|
||||
// Summary should still be the original, not overwritten.
|
||||
assert_eq!(agent.completion.as_ref().unwrap().summary, "Already done");
|
||||
drop(agents);
|
||||
|
||||
// No Done event should have been emitted.
|
||||
assert!(
|
||||
rx.try_recv().is_err(),
|
||||
"should not emit Done when completion already exists"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn server_owned_completion_runs_gates_on_clean_worktree() {
|
||||
use tempfile::tempdir;
|
||||
|
||||
let tmp = tempdir().unwrap();
|
||||
let repo = tmp.path();
|
||||
init_git_repo(repo);
|
||||
|
||||
let pool = AgentPool::new_test(3001);
|
||||
pool.inject_test_agent_with_path("s11", "coder-1", AgentStatus::Running, repo.to_path_buf());
|
||||
|
||||
let mut rx = pool.subscribe("s11", "coder-1").unwrap();
|
||||
|
||||
run_server_owned_completion(
|
||||
&pool.agents,
|
||||
pool.port,
|
||||
"s11",
|
||||
"coder-1",
|
||||
Some("sess-2".to_string()),
|
||||
pool.watcher_tx.clone(),
|
||||
)
|
||||
.await;
|
||||
|
||||
// Agent entry should be removed from the map after completion.
|
||||
let agents = pool.agents.lock().unwrap();
|
||||
let key = super::super::super::composite_key("s11", "coder-1");
|
||||
assert!(
|
||||
agents.get(&key).is_none(),
|
||||
"agent should be removed from map after completion"
|
||||
);
|
||||
drop(agents);
|
||||
|
||||
// A Done event should have been emitted with the session_id.
|
||||
let event = rx.try_recv().expect("should emit Done event");
|
||||
match &event {
|
||||
AgentEvent::Done { session_id, .. } => {
|
||||
assert_eq!(*session_id, Some("sess-2".to_string()));
|
||||
}
|
||||
other => panic!("expected Done event, got: {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn server_owned_completion_fails_on_dirty_worktree() {
|
||||
use std::fs;
|
||||
use tempfile::tempdir;
|
||||
|
||||
let tmp = tempdir().unwrap();
|
||||
let repo = tmp.path();
|
||||
init_git_repo(repo);
|
||||
// Create an uncommitted file.
|
||||
fs::write(repo.join("dirty.txt"), "not committed").unwrap();
|
||||
|
||||
let pool = AgentPool::new_test(3001);
|
||||
pool.inject_test_agent_with_path("s12", "coder-1", AgentStatus::Running, repo.to_path_buf());
|
||||
|
||||
let mut rx = pool.subscribe("s12", "coder-1").unwrap();
|
||||
|
||||
run_server_owned_completion(
|
||||
&pool.agents,
|
||||
pool.port,
|
||||
"s12",
|
||||
"coder-1",
|
||||
None,
|
||||
pool.watcher_tx.clone(),
|
||||
)
|
||||
.await;
|
||||
|
||||
// Agent entry should be removed from the map after completion (even on failure).
|
||||
let agents = pool.agents.lock().unwrap();
|
||||
let key = super::super::super::composite_key("s12", "coder-1");
|
||||
assert!(
|
||||
agents.get(&key).is_none(),
|
||||
"agent should be removed from map after failed completion"
|
||||
);
|
||||
drop(agents);
|
||||
|
||||
// A Done event should have been emitted.
|
||||
let event = rx.try_recv().expect("should emit Done event");
|
||||
assert!(
|
||||
matches!(event, AgentEvent::Done { .. }),
|
||||
"expected Done event, got: {event:?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn server_owned_completion_nonexistent_agent_is_noop() {
|
||||
let pool = AgentPool::new_test(3001);
|
||||
// Should not panic or error — just silently return.
|
||||
run_server_owned_completion(
|
||||
&pool.agents,
|
||||
pool.port,
|
||||
"nonexistent",
|
||||
"bot",
|
||||
None,
|
||||
pool.watcher_tx.clone(),
|
||||
)
|
||||
.await;
|
||||
}
|
||||
|
||||
/// Regression test for bug 445: a rate-limited mergemaster exits before
|
||||
/// calling start_merge_agent_work. run_server_owned_completion must be a
|
||||
/// no-op for mergemaster agents — it must not run acceptance gates and must
|
||||
/// not advance the story to 5_done/ even when a passing script/test exists.
|
||||
///
|
||||
/// Before the fix: run_server_owned_completion would call run_pipeline_advance
|
||||
/// for the Mergemaster stage, which ran post-merge tests on master (they pass
|
||||
/// because nothing changed), then called move_story_to_done — advancing the
|
||||
/// story without any squash merge having occurred.
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn server_owned_completion_is_noop_for_mergemaster() {
|
||||
use std::fs;
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
use tempfile::tempdir;
|
||||
|
||||
let tmp = tempdir().unwrap();
|
||||
let root = tmp.path();
|
||||
init_git_repo(root);
|
||||
|
||||
// Create a passing script/test so post-merge tests would succeed if
|
||||
// run_pipeline_advance were incorrectly called for this mergemaster.
|
||||
let script_dir = root.join("script");
|
||||
fs::create_dir_all(&script_dir).unwrap();
|
||||
let script_test = script_dir.join("test");
|
||||
fs::write(&script_test, "#!/usr/bin/env sh\nexit 0\n").unwrap();
|
||||
let mut perms = fs::metadata(&script_test).unwrap().permissions();
|
||||
perms.set_mode(0o755);
|
||||
fs::set_permissions(&script_test, perms).unwrap();
|
||||
|
||||
// Story in 4_merge/ — must NOT be moved to 5_done/.
|
||||
let merge_dir = root.join(".huskies/work/4_merge");
|
||||
fs::create_dir_all(&merge_dir).unwrap();
|
||||
let story_path = merge_dir.join("99_story_merge445.md");
|
||||
fs::write(&story_path, "---\nname: Merge 445 Test\n---\n").unwrap();
|
||||
|
||||
let pool = AgentPool::new_test(3001);
|
||||
pool.inject_test_agent_with_path(
|
||||
"99_story_merge445",
|
||||
"mergemaster",
|
||||
AgentStatus::Running,
|
||||
root.to_path_buf(),
|
||||
);
|
||||
|
||||
run_server_owned_completion(
|
||||
&pool.agents,
|
||||
pool.port,
|
||||
"99_story_merge445",
|
||||
"mergemaster",
|
||||
None,
|
||||
pool.watcher_tx.clone(),
|
||||
)
|
||||
.await;
|
||||
|
||||
// Wait briefly in case any background task fires.
|
||||
tokio::time::sleep(std::time::Duration::from_millis(150)).await;
|
||||
|
||||
// Story must remain in 4_merge/ — not moved to 5_done/.
|
||||
let done_path = root.join(".huskies/work/5_done/99_story_merge445.md");
|
||||
assert!(
|
||||
!done_path.exists(),
|
||||
"Story must NOT be moved to 5_done/ when run_server_owned_completion \
|
||||
is (incorrectly) called for a mergemaster agent"
|
||||
);
|
||||
assert!(
|
||||
story_path.exists(),
|
||||
"Story must remain in 4_merge/ when mergemaster completion is a no-op"
|
||||
);
|
||||
|
||||
// The agent entry should remain in the pool (lifecycle cleanup is the
|
||||
// caller's responsibility, not run_server_owned_completion's).
|
||||
let agents = pool.agents.lock().unwrap();
|
||||
let key = super::super::super::composite_key("99_story_merge445", "mergemaster");
|
||||
assert!(
|
||||
agents.get(&key).is_some(),
|
||||
"Agent must remain in pool — run_server_owned_completion is a no-op for mergemaster"
|
||||
);
|
||||
}
|
||||
|
||||
/// 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_preserves_dirty_worktree_with_committed_work() {
|
||||
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-645_test",
|
||||
])
|
||||
.current_dir(&project_root)
|
||||
.output()
|
||||
.unwrap();
|
||||
|
||||
// Commit a valid file.
|
||||
fs::write(wt_path.join("work.txt"), "done").unwrap();
|
||||
Command::new("git")
|
||||
.args(["add", "."])
|
||||
.current_dir(&wt_path)
|
||||
.output()
|
||||
.unwrap();
|
||||
Command::new("git")
|
||||
.args(["commit", "-m", "coder: add work"])
|
||||
.current_dir(&wt_path)
|
||||
.output()
|
||||
.unwrap();
|
||||
|
||||
// Now simulate crash leaving dirty files.
|
||||
fs::write(wt_path.join("dirty.txt"), "crash residue").unwrap();
|
||||
|
||||
let pool = AgentPool::new_test(3001);
|
||||
pool.inject_test_agent_with_path("645_test", "coder-1", AgentStatus::Running, wt_path.clone());
|
||||
|
||||
let mut rx = pool.subscribe("645_test", "coder-1").unwrap();
|
||||
|
||||
run_server_owned_completion(
|
||||
&pool.agents,
|
||||
pool.port,
|
||||
"645_test",
|
||||
"coder-1",
|
||||
Some("sess-645".to_string()),
|
||||
pool.watcher_tx.clone(),
|
||||
)
|
||||
.await;
|
||||
|
||||
// Bug 651: The dirty file must be PRESERVED — uncommitted work is never junk.
|
||||
assert!(
|
||||
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
|
||||
// on dirty worktree).
|
||||
let event = rx.try_recv().expect("should emit Done event");
|
||||
assert!(
|
||||
matches!(event, AgentEvent::Done { .. }),
|
||||
"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"
|
||||
);
|
||||
}
|
||||
Reference in New Issue
Block a user