fix(handoff): address PR #365 review findings - #366
Merged
Conversation
…oad off the DB lock
- cargo fmt (item.rs claim closure, types.rs schemars doc attr)
- item::claim: canonicalize both sides of the assignee/owner comparison so
an alias owner (claude:1) isn't wrongly BlockedByAssignee against its own
canonical handoff assignee (claude-code)
- verify_continuation_commit: resolve the target branch under the backend
DB lock, then run all git subprocess checks after releasing it, matching
the existing item_claim split; validate oid is a plain hex id and force
commit-type resolution (oid^{commit}) so a non-hex/flag-like value or a
blob/tree/tag can't pass as a continuation commit
- handoff_impl: reject empty completed/remaining instead of silently
accepting an empty structured payload
- mcp_prompts: document completed/remaining as required handoff fields
so generated requests don't fail deserialization
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
getappz
added a commit
that referenced
this pull request
Jul 29, 2026
…icate-item reuse, assignee freeze (#365) * feat(handoff): verified continuation commit, structured payload, duplicate-item reuse, assignee freeze Item #236 (handoff hardening, QuorumGit adoption): - HandoffRequest gains last_commit: verified via `git cat-file -e` and, when the item's task/<seq> branch already exists, `git merge-base --is-ancestor` -- a fabricated or unreachable OID is rejected, not trusted. - HandoffRequest gains required completed/remaining and optional blockers fields, stored in the asset's metadata. - A handoff without item_id now reuses an existing open item assigned to the recipient (matched by name or thread_id) instead of blindly creating a duplicate; genuinely new work still auto-creates. - agentflare_backend::item::claim gains a BlockedByAssignee outcome: a freshly handed-off, never-claimed item can only be claimed by its assignee until accepted, excluding completed/cancelled items. Deferred: shim-side branch-push denial for open handoffs (spec's own review already downgraded this to defense-in-depth, covered in large part by #234's scope enforcement and the opencode branch-guard plugin). * fix(handoff): fmt, canonicalize claim owner, validate commit oid/payload off the DB lock (#366) - cargo fmt (item.rs claim closure, types.rs schemars doc attr) - item::claim: canonicalize both sides of the assignee/owner comparison so an alias owner (claude:1) isn't wrongly BlockedByAssignee against its own canonical handoff assignee (claude-code) - verify_continuation_commit: resolve the target branch under the backend DB lock, then run all git subprocess checks after releasing it, matching the existing item_claim split; validate oid is a plain hex id and force commit-type resolution (oid^{commit}) so a non-hex/flag-like value or a blob/tree/tag can't pass as a continuation commit - handoff_impl: reject empty completed/remaining instead of silently accepting an empty structured payload - mcp_prompts: document completed/remaining as required handoff fields so generated requests don't fail deserialization * fix(test): stop with_temp_home_clears_the_override_env_var_after_returning from racing concurrent with_temp_home callers The assertion read AGENTFLARE_HOME_OVERRIDE outside GLOBAL_STATE_LOCK, the lock that guards every set/remove of it. A concurrent thread's own correctly-scoped with_temp_home call could be transiently holding the var set at the exact moment this test read it, flaking the check on a var this test was never entitled to observe in the first place. Fix: acquire the same lock before reading.
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
Fixes for the review findings on #365 (item #236 handoff hardening):
cargo fmt --checkfailures (item.rsclaim closure,types.rsschemars attribute) that were failing thefmt/CI Greenchecks on feat(handoff): verified continuation commit, structured payload, duplicate-item reuse, assignee freeze #365.crates/agentflare-backend/src/item.rs):agent_part()compared the raw owner id against the already-canonicalizedassignee_agent. An alias owner likeclaude:1didn't match the stored canonicalclaude-code, so the handoff's own assignee could get wronglyBlockedByAssigneeon their own item. Now canonicalizes both sides.src/mcp_server/handoff.rs):verify_continuation_commitran up to three git subprocess calls insidewith_backend_db, which holds a shared mutex — the same pattern the codebase already avoids initem_claim(DB resolve under lock, then blocking work after). Restructured to resolve the target branch under the lock and run all git checks after releasing it.last_commitvalidation gap (security/correctness): a leading-inoidwould be parsed by git as an option rather than a rev, and plaincat-file -eaccepts blobs/trees/tags, not just commits. Now rejects non-hex input up front and forces commit-type resolution viaoid^{commit}.completed/remainingare documented as required but weren't checked for emptiness, so""satisfied the contract silently. Added the sametrim().is_empty()guard used for the other required fields./handoffprompt didn't mentioncompleted/remainingas required fields, so agent-generated requests built from the prompt could fail to deserialize.Also added two regression tests (owner-alias claim, and updated existing handoff tests to supply
completed/remainingnow that they're enforced as non-empty).Test plan
cargo fmt --check— cleancargo clippy --workspace --all-features -- -D warnings— cleancargo test --workspace --all-features— all green (886 passed in the main crate, 60 in agentflare-backend, no failures workspace-wide)