feat(mcp): item comments (comment/comment_edit/comment_delete/comment_list) - #173
Merged
Conversation
…_list) Adds item_comments table and comment/comment_edit/comment_delete/ comment_list actions to the item MCP tool, so agents can leave threaded notes on work items instead of only assign+attach. - migration 0004_item_comments.sql, agentflare-backend::comment module (create/get/update/delete/list_by_item/is_latest) - edit/delete gated: author-only, latest-comment-only, and blocked while another agent holds an active claim on the item (claim::has_active_claim_by_other) - is_latest breaks created_at ties on id (UUIDv7, time-ordered) instead of comparing the second-resolution timestamp alone, since two comments posted in the same second otherwise both read as latest and the edit/delete gate silently stops enforcing anything - ownership check compares claims::agent_of(owner) (the agent name, stripped of its ':<instance>' suffix) rather than the raw owner id, so an agent doesn't lose the ability to edit its own comments the moment its CLI process/session restarts and gets a new instance id - new tests: rejection by a genuinely different agent, cross-session edit by the same agent, and the same-second latest-tiebreak case
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds SQLite-backed item comments with MCP create, list, edit, and delete actions. It also adds claim-aware permission checks and git-backed worktree creation for acquired items, including worktree paths in claim responses. ChangesItem comment operations
Claimed item worktrees
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant AgentflareMcp
participant comment
participant claim
participant SQLite
MCPClient->>AgentflareMcp: comment edit or delete request
AgentflareMcp->>comment: load comment and check latest status
comment->>SQLite: query item_comments
AgentflareMcp->>claim: check active claim by another owner
claim->>SQLite: query claim ledger
AgentflareMcp->>comment: update or delete comment
comment->>SQLite: write item_comments
sequenceDiagram
participant MCPClient
participant AgentflareMcp
participant worktree
participant Git
MCPClient->>AgentflareMcp: claim item
AgentflareMcp->>worktree: resolve target branch
worktree->>Git: inspect repository and branch state
AgentflareMcp->>worktree: create worktree for acquired item
worktree->>Git: add task branch and worktree
worktree-->>AgentflareMcp: worktree path or None
AgentflareMcp-->>MCPClient: acquired response with optional worktree_path
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🧹 Nitpick comments (3)
crates/agentflare-backend/src/comment.rs (1)
79-88: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winComments are hard-deleted — no audit trail.
Every other backend entity in this crate (
item,asset,label,workspace,project,webhook,state) soft-deletes via adeleted_atcolumn, preserving history.delete()here issues a hardDELETE, so once a comment is removed there's no way to recover it or audit what was said — notable given this feature's own emphasis on ownership/authorship auditability (latest-comment tracking, cross-session author identity).♻️ Sketch of a soft-delete alternative
-pub fn delete(conn: &Connection, id: &str) -> Result<()> { - let changed = conn.execute( - "DELETE FROM item_comments WHERE id = ?1", - rusqlite::params![id], - )?; +pub fn delete(conn: &Connection, id: &str) -> Result<()> { + let ts = now(); + let changed = conn.execute( + "UPDATE item_comments SET deleted_at = ?2, updated_at = ?2 WHERE id = ?1 AND deleted_at IS NULL", + rusqlite::params![id, ts], + )?; if changed == 0 { return Err(crate::error::Error::NotFound(id.to_string())); } Ok(()) }(requires adding a nullable
deleted_atcolumn via migration and filtering it inget/list_by_item/is_latest)🤖 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/agentflare-backend/src/comment.rs` around lines 79 - 88, Update comment deletion in the `delete` function to soft-delete by setting a nullable `deleted_at` timestamp instead of removing the row. Add the required schema migration, and ensure `get`, `list_by_item`, and `is_latest` exclude comments whose `deleted_at` is set while preserving `NotFound` behavior for already-deleted or missing comments.src/mcp_server.rs (2)
4853-4884: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test exercises the active-claim block itself.
item_comment_edit_succeeds_when_latest_and_own_and_unclaimed_by_otheronly covers the "no claim exists" path. There's no test that actually acquires a claim from a different owner and assertscomment_edit/comment_deleteare rejected with "another agent has started work" — the one negative case the PR objective explicitly calls out ("no active claim by another agent") isn't directly verified.🤖 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/mcp_server.rs` around lines 4853 - 4884, Add a test covering the active-claim rejection path alongside item comment editing tests: create a comment, acquire its claim as a different owner, then assert both comment_edit and comment_delete are rejected with the “another agent has started work” message. Keep the existing latest/own/unclaimed success test unchanged and use the established claim API and harness helpers.
2552-2653: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated author/latest/claim gate between comment_edit and comment_delete.
The three checks (own-comment, is-latest, no-active-claim-by-other) are copy-pasted almost verbatim between the two branches, differing only in the final action and error wording. Extracting a shared helper (e.g.
fn authorize_comment_mutation(conn, &comment_id, &owner, now, ttl, verb: &str) -> Result<ItemComment, ErrorData>) would remove the duplication and keep the two gates from drifting apart.🤖 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/mcp_server.rs` around lines 2552 - 2653, Extract the duplicated authorization checks from the comment_edit and comment_delete branches into a shared helper, such as authorize_comment_mutation, accepting the connection, comment ID, owner, timestamp, TTL, and operation verb, and returning the loaded comment. Preserve the existing ownership, latest-comment, and active-claim validations while using the verb for operation-specific error messages, then have both branches perform only their respective update or delete action after authorization.
🤖 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/mcp_server.rs`:
- Around line 2552-2653: The comment_edit and comment_delete branches perform
latest/claim validation separately from their writes, allowing concurrent
changes to violate the latest-comment invariant. Wrap each with_backend_db
operation’s get, ownership/latest/claim checks, and update or delete call in a
single conn.unchecked_transaction(), preserving the existing validations and
response behavior.
---
Nitpick comments:
In `@crates/agentflare-backend/src/comment.rs`:
- Around line 79-88: Update comment deletion in the `delete` function to
soft-delete by setting a nullable `deleted_at` timestamp instead of removing the
row. Add the required schema migration, and ensure `get`, `list_by_item`, and
`is_latest` exclude comments whose `deleted_at` is set while preserving
`NotFound` behavior for already-deleted or missing comments.
In `@src/mcp_server.rs`:
- Around line 4853-4884: Add a test covering the active-claim rejection path
alongside item comment editing tests: create a comment, acquire its claim as a
different owner, then assert both comment_edit and comment_delete are rejected
with the “another agent has started work” message. Keep the existing
latest/own/unclaimed success test unchanged and use the established claim API
and harness helpers.
- Around line 2552-2653: Extract the duplicated authorization checks from the
comment_edit and comment_delete branches into a shared helper, such as
authorize_comment_mutation, accepting the connection, comment ID, owner,
timestamp, TTL, and operation verb, and returning the loaded comment. Preserve
the existing ownership, latest-comment, and active-claim validations while using
the verb for operation-specific error messages, then have both branches perform
only their respective update or delete action after authorization.
🪄 Autofix (Beta)
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 Plus
Run ID: c618f6ab-7849-4f05-8d05-77d5f0789ba3
📒 Files selected for processing (7)
crates/agentflare-backend/src/claim.rscrates/agentflare-backend/src/comment.rscrates/agentflare-backend/src/db.rscrates/agentflare-backend/src/lib.rscrates/agentflare-backend/src/migrations/0004_item_comments.sqlsrc/claims.rssrc/mcp_server.rs
New src/worktree.rs module:
- resolve_target_branch: parent metadata.branch → repo default
- already_isolated_for: git-dir vs common-dir detection
- ensure_worktrees_ignored: .gitignore scan + commit
- create_for_item: git worktree add .worktrees/task/{seq_id}
Hooked into item(claim) Acquired arm — worktree_path added to
JSON response. Soft-fails via eprintln on all git errors.
8 tests (7 unit + 1 MCP-level), clippy + fmt clean.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/worktree.rs (1)
158-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the branch tests hermetic and exercise
origin/HEAD.Both tests currently cover the same no-remote case, while
git initmay selectmaindepending on global configuration. Initialize an explicit branch and configure a remote symbolicorigin/HEADin the first 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/worktree.rs` around lines 158 - 178, Update init_repo to initialize an explicit branch, such as master, so tests do not depend on Git’s global default. Modify resolve_default_branch_resolves_from_origin_head to configure a remote and its symbolic origin/HEAD reference, then assert resolution from that reference; keep resolve_default_branch_falls_back_when_no_remote focused on the no-remote fallback.
🤖 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/mcp_server.rs`:
- Around line 5082-5094: Update the test cleanup around AgentflareMcp::repo_root
to use an injected temporary repository root for the MCP harness, ensuring
worktree and branch creation occur in that repository. Perform the git worktree
removal and branch deletion against only this isolated temporary root, never the
repository running the tests, while preserving the existing cleanup behavior.
- Around line 2444-2447: Refactor the flow around create_for_item so
with_backend_db only resolves the item and target branch while holding the
backend mutex. Move the create_for_item call, including its blocking filesystem
and Git work, outside the closure after the lock is released, preserving the
existing worktree result behavior.
In `@src/worktree.rs`:
- Around line 81-108: Update ensure_worktrees_ignored so claiming a worktree
never runs git add or git commit and cannot commit unrelated staged files or
existing .gitignore edits. Prefer writing the .worktrees/ rule to the
repository-local .git/info/exclude file, preserving existing entries and
formatting; otherwise leave the ignore change uncommitted.
- Around line 43-55: Update resolve_default_branch to avoid unconditionally
returning "master" when origin/HEAD and main are unavailable. Verify master
exists, then fall back to the repository’s current symbolic branch; if no valid
branch can be determined, return an explicit failure using the surrounding API’s
established error convention.
- Around line 117-118: Update the already_isolated_for branch in the worktree
path resolution to return repo_root directly instead of the process current
directory, preserving the existing fallback behavior for non-isolated cases.
- Around line 69-73: Update the superproject detection in already_isolated_for
around run_git_in_ok to inspect the command’s stdout rather than relying only on
its successful exit status. Treat the worktree as superproject-backed and return
false only when --show-superproject-working-tree produces a non-empty path;
preserve isolation behavior when stdout is empty.
---
Nitpick comments:
In `@src/worktree.rs`:
- Around line 158-178: Update init_repo to initialize an explicit branch, such
as master, so tests do not depend on Git’s global default. Modify
resolve_default_branch_resolves_from_origin_head to configure a remote and its
symbolic origin/HEAD reference, then assert resolution from that reference; keep
resolve_default_branch_falls_back_when_no_remote focused on the no-remote
fallback.
🪄 Autofix (Beta)
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 Plus
Run ID: f91394ac-d7fc-46fe-b059-6cb7fe434b30
📒 Files selected for processing (4)
.gitignoresrc/main.rssrc/mcp_server.rssrc/worktree.rs
Comment on lines
+117
to
+118
| if already_isolated_for(&branch, repo_root) { | ||
| return Some(std::env::current_dir().unwrap_or_else(|_| repo_root.to_path_buf())); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Return the worktree root, not the process directory.
When the MCP server starts in a subdirectory, current_dir() produces an incorrect worktree_path. repo_root already identifies the correct root.
if already_isolated_for(&branch, repo_root) {
- return Some(std::env::current_dir().unwrap_or_else(|_| repo_root.to_path_buf()));
+ return Some(repo_root.to_path_buf());
}📝 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
| if already_isolated_for(&branch, repo_root) { | |
| return Some(std::env::current_dir().unwrap_or_else(|_| repo_root.to_path_buf())); | |
| if already_isolated_for(&branch, repo_root) { | |
| return Some(repo_root.to_path_buf()); |
🤖 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/worktree.rs` around lines 117 - 118, Update the already_isolated_for
branch in the worktree path resolution to return repo_root directly instead of
the process current directory, preserving the existing fallback behavior for
non-isolated cases.
…ment tool Review findings on the item-comments and worktree-on-claim features: - comment_edit/comment_delete: author/latest/claim checks and the write now run inside one conn.unchecked_transaction() (matching item::claim's own precedent) instead of four separate round trips, closing a TOCTOU window under concurrent multi-agent access. - worktree create_worktree (was create_for_item) no longer runs git operations while the backend DB mutex is held: item::claim now resolves the item + target branch under the lock, then creates the worktree after releasing it. - resolve_default_branch no longer assumes 'master' as a bare fallback — checks main and master explicitly, then falls back to whatever branch is actually checked out, so trunk/develop-named repos with no origin still resolve to a real branch. - already_isolated_for checked only the exit status of 'rev-parse --show-superproject-working-tree', which exits 0 with empty stdout inside a plain linked worktree (not just submodules) — the 'already isolated' fast path never actually fired. Now checks for non-empty output. - ensure_worktrees_ignored no longer commits to the caller's repository (could sweep up unrelated staged files and uncommitted .gitignore edits into an unwanted commit). Writes to .git/info/exclude instead, uncommitted. - create_worktree returns the actual worktree path on the already-isolated fast path instead of std::env::current_dir(), which was wrong whenever the MCP server starts in a subdirectory. - item_claim_response_includes_worktree_path ran real 'git worktree'/branch operations, including force-delete, against the actual repository running the test suite. Now runs against an isolated temp repo via a new worktree_repo_root_override test hook. Also consolidates comment/comment_edit/comment_delete/comment_list — four of item's sixteen actions — into their own comment tool (action: create|edit|delete|list), matching the asset tool's precedent of a dedicated consolidated tool rather than folding unrelated concerns into item's already-large action dispatch.
This was referenced Jul 14, 2026
3 tasks
4 tasks
getappz
added a commit
that referenced
this pull request
Aug 27, 2026
…ning free text (#624) Item #170's false-positive class hit twice more in one session (items #192, #173): a description that merely mentions "design-spec" (e.g. referencing another item's spec) forces the review-only prompt even for a genuine implementation task, because nothing ever set the structured metadata.task_type signal detect_review_only already knows how to trust. handoff now accepts an optional task_type and merges it into the item's existing metadata (without clobbering other keys) both when targeting an existing item_id and when creating a new one. Agentflare-Agent: claude-code Agentflare-Branch: task/task-type-metadata-review-only-fix Agentflare-Session: e77fc32e-33d0-4884-ab55-fdda48fe45fd Co-authored-by: shiva <shiva@gosysinfo.tech>
4 tasks
getappz
added a commit
that referenced
this pull request
Aug 28, 2026
…e redispatch does (#627) Reproduced live twice in one PM-mode session (items #192, #173): handoff onto an item still carrying `dispatched` from a prior attempt only ever *added* ready-for-work, never cleared the stale label. run_discovery_tick's own downstream claim-liveness gate then treated the item as still spoken for, so the fresh handoff silently did nothing until someone remembered to call item(action="redispatch") instead. handoff now clears REDISPATCH_CLEARED_LABELS (now pub, reused from item::claim::redispatch) unconditionally on an existing-item_id handoff, before the existing live-claim check that still gates the ready-for-work re-attach. A plain handoff onto an already-dispatched item now behaves the same as redispatch, instead of requiring the caller to know which tool to reach for. Agentflare-Agent: claude-code Agentflare-Branch: task/handoff-clears-stale-dispatch-labels Agentflare-Session: e77fc32e-33d0-4884-ab55-fdda48fe45fd Co-authored-by: shiva <shiva@gosysinfo.tech>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an
item_commentstable andcomment/comment_edit/comment_delete/comment_listactions to theitemMCP tool, so agents can leave threaded notes on work items instead of only assign+attach.What's here
0004_item_comments.sql,agentflare-backend::commentmodule (create/get/update/delete/list_by_item/is_latest).comment_edit/comment_deleteare gated: author-only, latest-comment-only, and blocked while another agent holds an active claim on the item (claim::has_active_claim_by_other).Review findings fixed before merge
Reviewed the original implementation and found (and fixed) two correctness bugs plus a test-coverage gap:
is_latesttimestamp collision —created_atis second-resolution, so two comments posted in the same second both read as "latest" under aMAX(created_at)equality check, silently defeating the edit/delete gate. Now breaks ties onid(UUIDv7, time-ordered).owner_id()(agent:instance, instance = PID orAGENTFLARE_SESSION), so an agent lost the ability to edit its own comments the moment its process/session restarted. Addedclaims::agent_of()to strip the instance suffix and compare stable agent identity instead.item_comment_edit_rejected_when_not_own_comment) that actually only exercised the not-found path, and added real coverage for cross-agent rejection, cross-session same-agent success, and the same-second tiebreak.Testing
cargo test --workspace— 373 passed, 0 failedcargo clippy --workspace --all-targets -- -D warnings -A unsafe_code -A clippy::pedantic— cleancargo fmt --check— cleanSummary by CodeRabbit
worktree_pathin claim “acquired” responses when available.