Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions crates/flare-git-core/src/worktree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Comment on lines +704 to +731

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Add a timeout for the automatic commit.

Line 730 calls run_git_in, which waits indefinitely for git commit. A commit hook or signing program can block this MCP completion request indefinitely. Use the existing timeout helper and return false on timeout.

Proposed fix
-    run_git_in(&worktree_path, &["commit", "-m", message]).is_ok()
+    matches!(
+        run_output_timeout(
+            crate::shell::git_binary(),
+            &["commit", "-m", message],
+            &worktree_path,
+            30,
+        ),
+        Ok(out) if out.status.success()
+    )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// 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()
}
/// 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;
}
matches!(
run_output_timeout(
crate::shell::git_binary(),
&["commit", "-m", message],
&worktree_path,
30,
),
Ok(out) if out.status.success()
)
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/flare-git-core/src/worktree.rs` around lines 704 - 731, Update
commit_uncommitted’s automatic git commit step to use the existing timeout-aware
helper instead of the indefinitely waiting run_git_in call. Preserve the current
boolean behavior by returning false when the commit times out or otherwise
fails, while leaving status and staging behavior unchanged.


/// Remove a worktree directory, with retry + Windows fallback.
///
/// On Windows, background processes (rust-analyzer, proc-macro-srv) may
Expand Down Expand Up @@ -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
Expand Down
20 changes: 19 additions & 1 deletion src/cli/work.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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
Expand Down
17 changes: 17 additions & 0 deletions src/mcp_server/item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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| {
Expand Down
92 changes: 88 additions & 4 deletions src/mcp_server/tests/action_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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),
Expand All @@ -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"
);
}

Expand Down
10 changes: 10 additions & 0 deletions src/worktree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading