Repository navigation
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe Git worktree API now returns explicit commit outcomes. MCP item completion preserves failed worktrees and claims. Successful work runs can emit an ChangesCommit outcome handling
Hold signal workflow
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 1254-1270: Replace the permission-based write protection with an
index lock: at both affected sites,
crates/flare-git-core/src/worktree.rs:1254-1270 and
src/mcp_server/tests/item_commit_failure_tests.rs:51-72, create
<git_dir>/index.lock before invoking commit_uncommitted or item_done, then
remove the lock afterward so cleanup can proceed. Remove the set_readonly logic;
both sites require this direct change.
🪄 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: c4d92750-c0b6-498e-ae90-e9a3e4882788
📒 Files selected for processing (5)
crates/flare-git-core/src/worktree.rssrc/mcp_server/item.rssrc/mcp_server/tests/item_commit_failure_tests.rssrc/mcp_server/tests/mod.rssrc/worktree.rs
| // Make the index unwritable so `git add -A` fails deterministically | ||
| // while `git status` (read-only) still succeeds. | ||
| let index_path = worktree_path.join(".git"); | ||
| let git_dir = std::fs::read_to_string(&index_path) | ||
| .ok() | ||
| .and_then(|s| s.strip_prefix("gitdir: ").map(str::trim).map(String::from)) | ||
| .map(PathBuf::from) | ||
| .unwrap_or(index_path); | ||
| let mut perms = std::fs::metadata(&git_dir).unwrap().permissions(); | ||
| perms.set_readonly(true); | ||
| std::fs::set_permissions(&git_dir, perms.clone()).unwrap(); | ||
|
|
||
| let result = commit_uncommitted(&item, &repo.path, "auto-committed"); | ||
|
|
||
| // Restore write permission so the temp dir can be cleaned up. | ||
| perms.set_readonly(false); | ||
| std::fs::set_permissions(&git_dir, perms).unwrap(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Rust std::fs::Permissions::set_readonly(true) reliably prevent creating files in a directory on Windows, and how does the Windows directory read-only attribute behave?
💡 Result:
No, rust std::fs::Permissions::set_readonly(true) does not reliably prevent the creation of files within a directory on Windows [1][2]. The behavior of the read-only attribute on Windows is fundamentally different for directories than it is for files [1][3]. While the FILE_ATTRIBUTE_READONLY flag prevents writing to or deleting a specific file, it is generally ignored by Windows for directories [3][4]. On modern versions of Windows, setting this attribute on a directory does not restrict file creation, renaming, or deletion within that directory [1][3][4]. Historically, on Windows 7 and earlier, this attribute could prevent the deletion of empty directories, but it did not prevent modification of their contents [1][5]. Modern Windows components, including Windows Explorer and most applications, ignore the read-only attribute when performing file system operations on directories [6][4]. In some contexts, Windows Explorer may treat the read-only or system attribute on a folder as a signal to check for a desktop.ini file for folder customization, rather than as a security restriction [4]. To control whether a user or process can create files in a directory on Windows, you must use Windows Access Control Lists (ACLs) [7]. Access rights, such as FILE_ADD_FILE (to permit file creation) or FILE_ADD_SUBDIRECTORY (to permit subdirectory creation), are managed through the security descriptor associated with the directory [8][7]. The read-only attribute is a legacy file system flag and is not an effective mechanism for enforcing security or write-access policies on directories [3][6].
Citations:
- 1: https://doc.rust-lang.org/std/fs/struct.Permissions.html
- 2: https://users.rust-lang.org/t/set-permissions-not-working/108480
- 3: https://learn.microsoft.com/en-us/windows/win32/fileio/file-attribute-constants
- 4: https://learn.microsoft.com/en-us/answers/questions/2574241/unable-to-change-read-only-attribute-of-folders
- 5: https://doc.rust-lang.org/nightly/std/fs/struct.Permissions.html
- 6: https://stackoverflow.com/questions/13192956/how-does-read-only-affect-a-directory-when-using-c-net
- 7: https://learn.microsoft.com/en-us/windows/win32/fileio/file-security-and-access-rights
- 8: https://devblogs.microsoft.com/oldnewthing/20251203-00/?p=111836
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target implementation ---'
sed -n '1215,1290p' crates/flare-git-core/src/worktree.rs
printf '%s\n' '--- affected integration test ---'
sed -n '1,100p' src/mcp_server/tests/item_commit_failure_tests.rs
printf '%s\n' '--- relevant symbols and lock-file handling ---'
rg -n --glob '*.rs' 'commit_uncommitted|index\.lock|git add|add\(-A\)|Command::new\("git"\)' crates srcRepository: getappz/agentflare
Length of output: 12697
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- commit_uncommitted implementation ---'
sed -n '700,780p' crates/flare-git-core/src/worktree.rs
printf '%s\n' '--- worktree path and git-directory helpers ---'
rg -n -A35 -B15 'fn (worktree_path|git_dir|resolve_worktree|status|run_git)|gitdir' crates/flare-git-core/src/worktree.rs
printf '%s\n' '--- remainder of integration test ---'
sed -n '90,180p' src/mcp_server/tests/item_commit_failure_tests.rs
printf '%s\n' '--- Git availability ---'
git --versionRepository: getappz/agentflare
Length of output: 12537
🏁 Script executed:
#!/bin/bash
set -eu
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
repo="$tmp/repo"
wt="$tmp/worktree"
git init -q -b master "$repo"
git -C "$repo" config user.email test@example.com
git -C "$repo" config user.name test
git -C "$repo" commit --allow-empty -qm initial
git -C "$repo" worktree add -q "$wt" -b task/test
printf '%s\n' '--- worktree git metadata ---'
cat "$wt/.git"
git_dir="$(sed 's/^gitdir: //' "$wt/.git")"
printf 'git_dir=%s\n' "$git_dir"
printf '%s\n' 'change' > "$wt/changed.txt"
printf '%s\n' '--- status before index.lock ---'
git -C "$wt" status --short
: > "$git_dir/index.lock"
printf '%s\n' '--- status with index.lock ---'
status_rc=0
git -C "$wt" status --short || status_rc=$?
printf 'status_exit=%s\n' "$status_rc"
printf '%s\n' '--- add with index.lock ---'
add_rc=0
git -C "$wt" add -A >"$tmp/add.stdout" 2>"$tmp/add.stderr" || add_rc=$?
printf 'add_exit=%s\n' "$add_rc"
cat "$tmp/add.stdout"
cat "$tmp/add.stderr"Repository: getappz/agentflare
Length of output: 827
Use an index lock to force git add failure.
Permissions::set_readonly(true) does not block directory writes on Windows. Both tests can let git add -A succeed and fail at unwrap_err().
Create <git_dir>/index.lock before commit_uncommitted or item_done, then remove it afterward at both affected sites.
📍 Affects 2 files
crates/flare-git-core/src/worktree.rs#L1254-L1270(this comment)src/mcp_server/tests/item_commit_failure_tests.rs#L51-L72
🤖 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 1254 - 1270, Replace the
permission-based write protection with an index lock: at both affected sites,
crates/flare-git-core/src/worktree.rs:1254-1270 and
src/mcp_server/tests/item_commit_failure_tests.rs:51-72, create
<git_dir>/index.lock before invoking commit_uncommitted or item_done, then
remove the lock afterward so cleanup can proceed. Remove the set_readonly logic;
both sites require this direct change.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/cli/work.rs`:
- Around line 832-837: Update the detect_hold_signal handling around
mcp.item_release to validate both the call result and the response’s released
field before posting the hold comment or returning success; on an error or
released == false, do not report the hold or exit successfully. Add a test
covering a failed release.
- Around line 832-837: Update the detect_hold_signal handling around
item_release to verify the worktree is clean before releasing the claim. If the
worktree is dirty, treat the run as failed and retain the claim; only invoke
item_release for a clean worktree, preserving the existing hold-signal behavior
otherwise.
- Around line 838-847: Apply cap_reply_for_comment to reply_text before
constructing the hold comment in the work hold branch, and use the capped
content for both comment creation and notify. Add a test covering an oversized
reply in the hold path to verify the persisted and notified content is capped.
- Around line 240-244: Update detect_hold_signal to inspect only the final
nonempty reply line, require that line to begin at byte zero with
AGENTFLARE_HOLD:, and return its nonempty trimmed reason; otherwise return None.
Add a regression test covering an earlier protocol-like line followed by normal
completion text so the item is released through item_done.
🪄 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: 0928de87-6fa3-4652-810a-00ca82b3ba7b
📒 Files selected for processing (1)
src/cli/work.rs
| fn detect_hold_signal(reply: &str) -> Option<&str> { | ||
| reply.lines().find_map(|line| { | ||
| let reason = line.trim().strip_prefix("AGENTFLARE_HOLD:")?.trim(); | ||
| (!reason.is_empty()).then_some(reason) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restrict hold detection to the final protocol line.
Line 240 accepts a matching line anywhere in the reply. Line 242 also accepts leading whitespace. This differs from the prompt requirement to end the reply with a line that starts exactly with AGENTFLARE_HOLD:.
A normal reply that mentions the protocol before its completion summary will release the item and bypass item_done. Inspect only the final nonempty line, require the prefix at byte zero, and add a regression test.
🤖 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 `@src/cli/work.rs` around lines 240 - 244, Update detect_hold_signal to inspect
only the final nonempty reply line, require that line to begin at byte zero with
AGENTFLARE_HOLD:, and return its nonempty trimmed reason; otherwise return None.
Add a regression test covering an earlier protocol-like line followed by normal
completion text so the item is released through item_done.
| if let Some(reason) = detect_hold_signal(&reply_text) { | ||
| let _ = mcp.item_release(ItemRequest { | ||
| action: "release".into(), | ||
| id: Some(item_id.into()), | ||
| ..Default::default() | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not report a hold when claim release fails.
The result from mcp.item_release is discarded. item_release can return an error or a successful response with "released": false. This path then posts an on-hold comment and exits with code zero while the item can remain actively claimed.
Validate both the call result and the released field before posting the hold comment or returning success. Add a test for a failed release.
🤖 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 `@src/cli/work.rs` around lines 832 - 837, Update the detect_hold_signal
handling around mcp.item_release to validate both the call result and the
response’s released field before posting the hold comment or returning success;
on an error or released == false, do not report the hold or exit successfully.
Add a test covering a failed release.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject hold signals from a dirty worktree.
The prompt requests no file changes, but this path does not enforce that condition. item_release attempts cleanup, refuses to remove a dirty worktree, and still releases the claim. A run that modifies files and emits AGENTFLARE_HOLD: leaves an unclaimed dirty worktree for a later dispatch.
Check that the worktree is clean before release. If it is dirty, treat the run as a failure and preserve the claim for recovery.
🤖 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 `@src/cli/work.rs` around lines 832 - 837, Update the detect_hold_signal
handling around item_release to verify the worktree is clean before releasing
the claim. If the worktree is dirty, treat the run as failed and retain the
claim; only invoke item_release for a clean worktree, preserving the existing
hold-signal behavior otherwise.
| let comment_body = | ||
| format!("## agentflare work — on hold\n\n{reason}\n\n{reply_text}"); | ||
| let _ = mcp.comment_impl(CommentRequest { | ||
| action: "create".into(), | ||
| item_id: Some(item_id.into()), | ||
| body: Some(comment_body.clone()), | ||
| ..Default::default() | ||
| }); | ||
| if let Some(recipient) = args.notify.as_deref() { | ||
| notify(recipient, &comment_body, item_id); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Apply the reply-size cap before creating a hold comment.
This branch persists and notifies the full reply_text. The ordinary completion path uses cap_reply_for_comment at Line 876 to prevent large raw agent output from expanding comment storage and notification payloads.
Use cap_reply_for_comment here as well. Add a hold-path test with an oversized reply.
🤖 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 `@src/cli/work.rs` around lines 838 - 847, Apply cap_reply_for_comment to
reply_text before constructing the hold comment in the work hold branch, and use
the capped content for both comment creation and notify. Add a test covering an
oversized reply in the hold path to verify the persisted and notified content is
capped.
…_done commit_uncommitted previously swallowed a failed `git commit` (e.g. a read-only .git, a rejected pre-commit hook) with no way for item_done to tell that apart from a genuinely clean tree. A dispatched job could exit 0 with real work correctly staged but never committed, and the item would silently release as if nothing needed doing. commit_uncommitted now reports a commit failure distinctly from "nothing staged"; item_done treats a failed commit as a hard failure (comment + do not release-as-clean) instead of folding it into the nothing-was-ever-committed path. The new regression test moved into its own file (item_commit_failure_tests.rs) rather than growing action_tests.rs past its 1500-line LOC gate. Agentflare-Agent: claude-code Agentflare-Branch: task/92 Agentflare-Item: 92
…t blocker prior sessions on this item reported. **What I implemented** (case 3 from the scope-broadening note — a dispatched agent correctly finding it's blocked on an unmet dependency, making zero changes, but with no way to say so distinct from "genuinely nothing to do"): - `build_prompt` (`src/cli/work.rs`) now teaches the agent an `AGENTFLARE_HOLD: <reason>` reply marker for exactly that case. - `execute_work`'s success path checks the reply for that marker (`detect_hold_signal`) *before* ever calling `item_done`. If present, it calls `item_release` instead — claim released, item state untouched, no `mark_completed` — posts a distinct "on hold" comment, and exits 0. This sidesteps the deeper root cause I found while investigating: `branch_diverged` diffs the *whole* branch against target, so a branch already carrying commits from an earlier claim attempt reads as "diverged" even when the current run changed nothing, silently routing a legitimate hold into the real-completion path (that's what actually happened to item #91). - Added 4 new tests; also confirmed case 2 (failed commit) is already correctly fixed by this branch's prior commit — `nothing_was_ever_committed`'s release path never touches item state. - Verified: `cargo build` clean, `cargo test --bin agentflare cli::work::` — 39 passed (4 new). `cargo clippy -D warnings` fails workspace-wide on a pre-existing, unrelated lint in `crates/agent-registry/src/router.rs` (last touched in PR #380, untouched here) — confirmed pre-existing on unmodified HEAD too. **Why it's uncommitted:** this session's own `.worktrees/task/92/.git` is mounted read-only (`mount` confirms `ro`), and the `mcp__flare__item done` daemon fallback also failed — `attempt to write a readonly database`. Same dual-blocked state the prior dispatch of this exact item hit. The one changed file sits uncommitted: - `src/cli/work.rs` (+83/-1) A human or unsandboxed process needs to `git add`/`git commit`/`git push` it, or re-run `mcp__flare__item done` for item 92 once the read-only mounts lift. Agentflare-Branch: task/92 Agentflare-Item: 92
Restore the git dir's exact original permissions after the readonly-simulation test instead of a blanket set_readonly(false), which clippy correctly flags as making the path world-writable on Unix. Agentflare-Agent: claude-code Agentflare-Branch: task/92 Agentflare-Item: 92
Same clippy::permissions_set_readonly_false issue as the mcp_server test fixed in the prior commit -- restore exact original permissions instead of a blanket set_readonly(false) toggle. worktree.rs's test module moved to a sibling worktree_tests.rs (pure code motion, no behavior change) since the fix pushed it past the 1500-line LOC gate -- item #92's own diff had already brought the file to the limit before this fix's few extra lines tipped it over. Agentflare-Agent: claude-code Agentflare-Branch: task/92 Agentflare-Item: 92
FILE_ATTRIBUTE_READONLY on a directory doesn't block writes to files inside it on Windows (unlike Unix permission bits), so the readonly- git-dir simulation these two tests rely on to force a commit failure doesn't hold there -- the commit silently succeeds instead, failing the test's own assertion. Confirmed as the actual cause of PR 455's Windows CI failure (auto-commit succeeded when the test expected an error). Unix-only, matching this repo's existing convention for platform-specific filesystem-permission tests. Agentflare-Agent: claude-code Agentflare-Branch: task/92 Agentflare-Item: 92
Summary
commit_uncommittedpreviously swallowed a failedgit commitwith no way foritem_doneto distinguish that from a genuinely clean tree.commit_uncommittednow reports a commit failure distinctly from "nothing staged";item_donetreats a failed commit as a hard failure (comment + does not release-as-clean) instead of folding it into the nothing-was-ever-committed path.Closes #92.
Test plan
cargo test -p flare-git-core --lib— 151 passedcargo test -p agentflare --bin agentflare mcp_server::tests::— 141 passed (includes newitem_done_reports_a_hard_error_when_auto_commit_fails_on_a_dirty_tree)action_tests.rspast the 1500-line LOC gateSummary by CodeRabbit