Conversation
…t 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
📝 WalkthroughWalkthroughThe change adds automatic commits for dirty item worktrees. It exposes the commit operation, updates agent prompts, invokes commits during ChangesWorktree auto-commit
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Agent
participant item_done
participant src_worktree
participant flare_git_core
participant Git
Agent->>item_done: Complete item with summary
item_done->>src_worktree: commit_uncommitted(item, repo_root, message)
src_worktree->>flare_git_core: Commit pending worktree changes
flare_git_core->>Git: Status, stage, and commit
Git-->>flare_git_core: Commit result
flare_git_core-->>item_done: Success or failure
item_done->>Git: Continue push or completion processing
``
</details>
<!-- walkthrough_end -->
<!-- pre_merge_checks_walkthrough_start -->
<details>
<summary>🚥 Pre-merge checks | ✅ 5</summary>
<details>
<summary>✅ Passed checks (5 passed)</summary>
| Check name | Status | Explanation |
| :------------------------: | :------- | :----------------------------------------------------------------------------------------------------------------------------------------------- |
| Title check | ✅ Passed | The title clearly identifies the primary fix: dispatched work leaves edits uncommitted and reports success incorrectly. |
| Description check | ✅ Passed | The description explains the cause, fix, tests, and edge cases, but it omits the template's explicit Test plan and Notes for reviewers headings. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
</details>
</details>
<!-- pre_merge_checks_walkthrough_end -->
<!-- finishing_touch_checkbox_start -->
<details>
<summary>✨ Finishing Touches</summary>
<details>
<summary>📝 Generate docstrings</summary>
- [ ] <!-- {"checkboxId": "7962f53c-55bc-4827-bfbf-6a18da830691"} --> Create stacked PR
- [ ] <!-- {"checkboxId": "3e1879ae-f29b-4d0d-8e06-d12b7ba33d98"} --> Commit on current branch
</details>
<details>
<summary>🧪 Generate unit tests (beta)</summary>
- [ ] <!-- {"checkboxId": "f47ac10b-58cc-4372-a567-0e02b2c3d479", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Create PR with unit tests
- [ ] <!-- {"checkboxId": "6ba7b810-9dad-11d1-80b4-00c04fd430c8", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Commit unit tests in branch `task/57`
</details>
</details>
<!-- finishing_touch_checkbox_end -->
<!-- tips_start -->
---
<sub>Comment `@coderabbitai help` to get the list of available commands.</sub>
<!-- tips_end -->
|
Agentflare-Agent: claude-code Agentflare-Branch: task/57 Agentflare-Item: 57
Agentflare-Agent: claude-code Agentflare-Branch: task/57 Agentflare-Item: 57
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@crates/flare-git-core/src/worktree.rs`:
- Around line 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.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b139e48f-e9b5-4f9d-9d91-9643c0a8dc20
📒 Files selected for processing (5)
crates/flare-git-core/src/worktree.rssrc/cli/work.rssrc/mcp_server/item.rssrc/mcp_server/tests/action_tests.rssrc/worktree.rs
| /// 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() | ||
| } |
There was a problem hiding this comment.
🩺 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.
| /// 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.
Summary
Root cause confirmed:
item_done'snothing_was_ever_committedcheck (based on branch divergence) couldn't distinguish "agent edited files but forgotgit commit" from "agent legitimately made no code changes" — both produced a clean/no-divergence signal, sodonereported success while real edits sat stranded in the worktree.Fix, two parts:
item_donenow auto-commits. Addedcommit_uncommittedincrates/flare-git-core/src/worktree.rs(thin-wrapped insrc/worktree.rs) — checksgit status --porcelainin the item's worktree and, if dirty, runsgit add -A && git commit -m <message>using the agent's ownsummaryas the commit message. Wired intoitem_done(src/mcp_server/item.rs) right before the push/PR and no-op classification run, so a forgotten commit becomes a real commit → push → PR instead of a silent "unchanged" success. I put this at theitem_donechoke point rather than inexecute_work(as the item suggested) since it's the single place both the daemon-dispatchedworkpath and any interactivemcp__flare__item donecall go through — one fix instead of two.build_prompt(src/cli/work.rs) explicitly tells the agent to commit before finishing, while still permitting a legitimate zero-commit outcome when no code change is warranted.The #41/#53 no-op path is untouched — a clean worktree short-circuits before any commit is attempted.
Tests: 3 new unit tests for
commit_uncommittedinflare-git-core, a rewrittenitem_donetest proving a dirty worktree now lands as a real commit andstatus: "completed"(plus a fallback test for when auto-commit itself can't run), and abuild_prompttest for the new instruction. Full touched-module test runs (166 tests acrossitem_done,cli::work,worktree,mcp_server::tests) all pass;cargo clippy/cargo fmtclean.Summary by CodeRabbit
New Features
Bug Fixes
Tests