From e0db46933a86b74790dc04b20ca233ccda7262d8 Mon Sep 17 00:00:00 2001 From: shiva Date: Mon, 10 Aug 2026 14:34:24 +0530 Subject: [PATCH] fix(work,item): auto-commit uncommitted changes on item done, instruct agents to commit A headless work run that edited real files but never ran git commit looked identical, from the outside, to a genuine no-op: the branch never diverged from target, so item_done's nothing_was_ever_committed check treated it the same as a design-only run with zero code changes, reported success, and left the real edits stranded uncommitted in the worktree (items #52, #54). item_done now commits any uncommitted worktree changes itself, using the agent's own summary as the commit message, before the push/PR and no-op classification run -- so a forgotten commit becomes a real commit+push+PR instead of a silent success with stranded edits. build_prompt's headless instructions now also explicitly tell the agent to commit its own work before finishing, while still allowing a legitimate zero-commit outcome when no code change is warranted (items #41, #53). Agentflare-Agent: claude-code Agentflare-Branch: task/57 Agentflare-Item: 57 --- crates/flare-git-core/src/worktree.rs | 67 +++++++++++++++++++ src/cli/work.rs | 20 +++++- src/mcp_server/item.rs | 17 +++++ src/mcp_server/tests/action_tests.rs | 92 +++++++++++++++++++++++++-- src/worktree.rs | 10 +++ 5 files changed, 201 insertions(+), 5 deletions(-) diff --git a/crates/flare-git-core/src/worktree.rs b/crates/flare-git-core/src/worktree.rs index db686ee8c..c0b674fe7 100644 --- a/crates/flare-git-core/src/worktree.rs +++ b/crates/flare-git-core/src/worktree.rs @@ -701,6 +701,35 @@ pub fn cleanup_item_worktree(item: &Item, repo_root: &Path) -> bool { !gc_orphans(repo_root, &[name]).is_empty() } +/// Commits any uncommitted changes sitting in `item`'s worktree checkout. +/// +/// An agent can make real file edits and still exit without ever running +/// `git commit` itself -- with no commit, the branch never diverges from +/// its target, so `item_done` (main binary) can't tell that apart from a +/// genuine no-op and the edits are silently stranded while `done` reports +/// success (item #57). Called before that divergence check runs, so a +/// forgotten commit gets made here first instead of falling through to the +/// "nothing was committed" path. +/// +/// No-ops (returns `false`) when the worktree doesn't exist, is already +/// clean, or `add`/`commit` fails for some other reason -- the caller's +/// existing dirty-tree handling (`cleanup_item_worktree` refusing to +/// remove it) still covers all of those exactly as it did before. +pub fn commit_uncommitted(item: &Item, repo_root: &Path, message: &str) -> bool { + let worktree_path = repo_root + .join(".worktrees") + .join("task") + .join(item.sequence_id.to_string()); + match run_git_in(&worktree_path, &["status", "--porcelain"]) { + Ok(out) if !out.trim().is_empty() => {} + _ => return false, + } + if run_git_in(&worktree_path, &["add", "-A"]).is_err() { + return false; + } + run_git_in(&worktree_path, &["commit", "-m", message]).is_ok() +} + /// Remove a worktree directory, with retry + Windows fallback. /// /// On Windows, background processes (rust-analyzer, proc-macro-srv) may @@ -1085,6 +1114,44 @@ mod tests { assert!(!cleanup_item_worktree(&item, &repo.path)); } + #[test] + fn commit_uncommitted_commits_a_dirty_worktree() { + let repo = init_repo(); + let item = test_item(1); + let target = resolve_default_branch(&repo.path); + let worktree_path = create_worktree(&item, &repo.path, &target, None).unwrap(); + std::fs::write(worktree_path.join("forgot_to_commit.txt"), "x").unwrap(); + + assert!(commit_uncommitted(&item, &repo.path, "auto-committed")); + + let status = run_git_in(&worktree_path, &["status", "--porcelain"]).unwrap(); + assert!(status.is_empty(), "worktree must be clean after commit"); + let branch = format!("task/{}", item.sequence_id); + assert!(branch_diverged(&repo.path, &branch, &target)); + } + + #[test] + fn commit_uncommitted_is_a_noop_on_a_clean_worktree() { + let repo = init_repo(); + let item = test_item(1); + let target = resolve_default_branch(&repo.path); + create_worktree(&item, &repo.path, &target, None).unwrap(); + + assert!(!commit_uncommitted(&item, &repo.path, "auto-committed")); + assert!(!branch_diverged( + &repo.path, + &format!("task/{}", item.sequence_id), + &target + )); + } + + #[test] + fn commit_uncommitted_is_a_noop_when_no_worktree_exists() { + let repo = init_repo(); + let item = test_item(1); + assert!(!commit_uncommitted(&item, &repo.path, "auto-committed")); + } + #[test] fn create_worktree_reuses_a_branch_that_exists_with_no_owning_worktree() { // The task/29 collision: a plain local branch survives (e.g. from a diff --git a/src/cli/work.rs b/src/cli/work.rs index 5e3925e55..17f64f865 100644 --- a/src/cli/work.rs +++ b/src/cli/work.rs @@ -96,7 +96,12 @@ fn build_prompt( prompt.push_str(&format!("- [{}] {}\n", c.author_agent, c.body)); } } - prompt.push_str("\nWhen you are done, summarize what you changed and why.\n"); + prompt.push_str( + "\nIf you made any file changes, commit them (git add + git commit) before you \ + finish -- do not leave edits uncommitted. If you investigate and decide no code \ + change is warranted, it's fine to finish with zero commits. When you are done, \ + summarize what you changed and why.\n", + ); prompt } @@ -766,6 +771,19 @@ mod tests { assert!(!prompt.contains("Prior discussion")); } + #[test] + fn build_prompt_instructs_the_agent_to_commit_before_finishing() { + // Item #57: a headless run that edited files but never ran `git + // commit` looked identical to a genuine no-op from outside. The + // prompt itself must tell the agent to commit its own work, on top + // of the `item_done`-side auto-commit safety net. + let item = test_item(); + let prompt = build_prompt(&item, &[]); + assert!(prompt.contains("commit")); + assert!(prompt.contains("do not leave edits uncommitted")); + assert!(prompt.contains("it's fine to finish with zero commits")); + } + #[test] fn build_prompt_frames_a_github_bridge_items_description_as_untrusted() { // The description on a bridge-originated item is a GitHub issue diff --git a/src/mcp_server/item.rs b/src/mcp_server/item.rs index 89eebc972..9a4ae86e1 100644 --- a/src/mcp_server/item.rs +++ b/src/mcp_server/item.rs @@ -608,6 +608,23 @@ impl AgentflareMcp { })??; let should_push = req.push.unwrap_or(true); let summary = req.summary.as_deref(); + // An agent can make real file edits and still exit without ever + // running `git commit` itself -- with no commit, the branch never + // diverges from target, so `nothing_was_ever_committed` below can't + // tell that apart from a genuine no-op, and the edits are silently + // stranded in the worktree while `done` still reports success + // (item #57). Commit them here, before push/PR or the "nothing was + // committed" classification ever run, using the agent's own summary + // as the commit message when there is one. + if let Some(item) = &item { + let message = summary + .map(str::trim) + .filter(|s| !s.is_empty()) + .unwrap_or("Auto-committed by item done: uncommitted changes at completion"); + if crate::worktree::commit_uncommitted(item, &repo_root, message) { + eprintln!("worktree: auto-committed uncommitted changes for item {item_id}"); + } + } let pr_url = match (&item, &target_branch) { (Some(item), Some(target)) if should_push => PROGRESS_SENDER .try_with(|ps| { diff --git a/src/mcp_server/tests/action_tests.rs b/src/mcp_server/tests/action_tests.rs index 420c02526..aaaac9ac7 100644 --- a/src/mcp_server/tests/action_tests.rs +++ b/src/mcp_server/tests/action_tests.rs @@ -585,9 +585,87 @@ fn item_release_leaves_a_dirty_worktree_in_place() { } #[test] -fn item_done_leaves_a_dirty_worktree_in_place() { - // The one case cleanup must refuse: uncommitted changes exist ONLY in - // that checkout, so deleting it would be destructive, not tidy. +fn item_done_auto_commits_uncommitted_changes_instead_of_stranding_them() { + // Item #57: a headless run that edited real files but never ran `git + // commit` itself used to look identical, from the outside, to a + // genuine no-op -- `done` reported success while the actual edits sat + // stranded, uncommitted, in the worktree forever. `done` must now + // commit any uncommitted changes itself before treating the claim as + // having nothing to show for it. + let tmp = tempfile::tempdir().unwrap(); + let repo_dir = tempfile::tempdir().unwrap(); + let repo_root = repo_dir.path().to_path_buf(); + let run_git = |dir: &std::path::Path, args: &[&str]| { + std::process::Command::new("git") + .args(args) + .current_dir(dir) + .output() + .unwrap() + }; + run_git(&repo_root, &["init", "-b", "master"]); + run_git(&repo_root, &["config", "user.email", "test@test.com"]); + run_git(&repo_root, &["config", "user.name", "Test"]); + run_git(&repo_root, &["commit", "--allow-empty", "-m", "initial"]); + + let s = AgentflareMcp { + backend_db_override: Some(tmp.path().join("backend.db")), + backend_project_link_override: Some(tmp.path().join("project.json")), + worktree_repo_root_override: Some(repo_root.clone()), + ..Default::default() + }; + + let created: serde_json::Value = + serde_json::from_str(&s.item(Parameters(empty_item_create("Test"))).unwrap()).unwrap(); + let item_id = created["id"].as_str().unwrap().to_string(); + let sequence_id = created["sequence_id"].as_i64().unwrap(); + + let claimed: serde_json::Value = serde_json::from_str( + &s.item(Parameters(ItemRequest { + action: "claim".into(), + id: Some(item_id.clone()), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + let worktree_path = std::path::PathBuf::from(claimed["worktree_path"].as_str().unwrap()); + std::fs::write(worktree_path.join("forgot_to_commit.txt"), "real work").unwrap(); + + // push: false avoids needing a real remote in this test -- the auto- + // commit safety net itself doesn't depend on whether the result gets + // pushed. + let result: serde_json::Value = serde_json::from_str( + &s.item(Parameters(ItemRequest { + action: "done".into(), + id: Some(item_id), + summary: Some("Did the real work, just forgot to commit it.".into()), + push: Some(false), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + + assert_eq!(result["done"], true, "{result:?}"); + assert_eq!(result["status"], "completed"); + + let branch = format!("task/{sequence_id}"); + let log = run_git( + &repo_root, + &["log", &branch, "--name-only", "--pretty=format:"], + ); + assert!( + String::from_utf8_lossy(&log.stdout).contains("forgot_to_commit.txt"), + "the agent's uncommitted edit must have landed in a real commit, not been dropped" + ); +} + +#[test] +fn item_done_leaves_a_dirty_worktree_in_place_when_auto_commit_cannot_run() { + // The remaining case cleanup must still refuse: when the auto-commit + // safety net itself can't run (git commands failing in the checkout), + // `done` must fall back to the old behavior -- leaving a dirty + // worktree in place rather than guessing it's safe to delete. let tmp = tempfile::tempdir().unwrap(); let repo_dir = tempfile::tempdir().unwrap(); let repo_root = repo_dir.path().to_path_buf(); @@ -626,6 +704,12 @@ fn item_done_leaves_a_dirty_worktree_in_place() { let worktree_path = std::path::PathBuf::from(claimed["worktree_path"].as_str().unwrap()); std::fs::write(worktree_path.join("uncommitted.txt"), "not yet committed").unwrap(); + // Break the checkout's gitdir pointer so `git status`/`add`/`commit` + // inside it all fail deterministically -- exercising the fallback path + // rather than the happy one above, without depending on environment- + // specific git author-identity fallbacks. + std::fs::write(worktree_path.join(".git"), "gitdir: /nonexistent").unwrap(); + s.item(Parameters(ItemRequest { action: "done".into(), id: Some(item_id), @@ -635,7 +719,7 @@ fn item_done_leaves_a_dirty_worktree_in_place() { assert!( worktree_path.join("uncommitted.txt").exists(), - "a dirty worktree must be left in place, not deleted out from under uncommitted work" + "a worktree whose changes could not be auto-committed must be left in place" ); } diff --git a/src/worktree.rs b/src/worktree.rs index 7a308d6b6..ef4eb4737 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -49,6 +49,16 @@ pub fn cleanup_worktree(item: &agentflare_backend::item::Item, repo_root: &Path) flare_git_core::worktree::cleanup_item_worktree(item, repo_root); } +/// Commits any uncommitted changes in `item`'s worktree. See +/// `flare_git_core::worktree::commit_uncommitted`. +pub fn commit_uncommitted( + item: &agentflare_backend::item::Item, + repo_root: &Path, + message: &str, +) -> bool { + flare_git_core::worktree::commit_uncommitted(item, repo_root, message) +} + /// Checks whether `item`'s branch already has a merged PR — the promotion /// signal `check_merge` uses to move an item out of "in_review" (item /// #420). Soft-fails like `push_and_open_pr`: no GitHub credentials, no