fix(node): honor pre-assigned task assignee on claim - #275
Conversation
Stop any signer from overwriting a reserved assignee_did (and receiving the UCAN). Bind finish_task to the stored assignee so complete/fail cannot race past the handler check.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughTask creation now returns the persisted task. Task claiming enforces reserved-assignee authorization and returns distinct forbidden errors. Completion and failure require actor DID matching. API and GraphQL events use persisted assignee data. Tests cover reservation, DID normalization, UCAN handling, and completion safeguards. ChangesTask claim and completion authorization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant GraphQLMutation
participant Db
Caller->>GraphQLMutation: claim task
GraphQLMutation->>Db: claim_task(id, caller_did)
Db-->>Db: validate pending status and reservation
alt unauthorized reservation
Db-->>GraphQLMutation: TaskReservedForOtherAssignee
GraphQLMutation-->>Caller: reservation error without UCAN
else authorized claim
Db-->>GraphQLMutation: persisted claimed task and UCAN
GraphQLMutation-->>Caller: claimed task
end
Caller->>GraphQLMutation: complete or fail task
GraphQLMutation->>Db: finish_task(id, status, result, actor_did)
Db-->>Db: verify exact persisted assignee
Db-->>GraphQLMutation: updated task
GraphQLMutation-->>Caller: event attributed to persisted assignee
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
needs-tests
|
Thanks for the contribution. A couple of things will help us review this faster:
See CONTRIBUTING.md. Update the PR and these notes will clear automatically. |
beardthelion
added
crate:node
There was a problem hiding this comment.
The claim gate holds up. I reverted the did_matches reservation check and both new tests went RED, so the premise is covered. Findings are about the denial's shape and about what the race half of the change actually proves.
Ignore the needs-tests label: the triage job looks for an added #[test] or #[cfg(test)] line and does not know about #[sqlx::test], which is how this repo writes DB tests. Your three tests are there. That regex is ours to fix, not yours, and #277 does it.
One scope note, since it affects how much your tests can claim: the anonymous read of ucan_token through GET /api/v1/tasks/{id} and the GraphQL task query is tracked in #268 and is not in scope here. Your UCAN assertions bind the claim path, which is the right thing for this PR to do, but the token is readable without claiming anything until #268 lands.
Findings
-
[P2] Return 403 for a reserved-task claim, not 409
crates/gitlawb-node/src/api/tasks.rs:177
The reservation denial atdb/mod.rs:2624is ananyhowerror, and the handler maps everyclaim_taskerror toCONFLICT. Driving a thief through the route gives409 {"error":"task not claimable: reserved for another assignee"}, while the signer-binding denial three lines above usesforbidden()and returns403 {"error":"forbidden","message":...}. Same concept, two statuses and two body shapes, and a permanent authorization denial is now indistinguishable from a lost race, so a retrying client never stops. I verified a fix: a marker error type on the reserved branch plus adowncast_refin the handler gives 403 for the steal and leaves an ordinary already-claimed race at409 {"error":"task not claimable: not found or already claimed"}. The assertion attest_support.rs:504flips with it; that was the only test that changed. -
[P2] Add a direct test for the
finish_taskassignee gate
crates/gitlawb-node/src/db/mod.rs:2670
Both handlers already reject a non-assignee with 403 before calling in, so nothing committed reaches the new DB-layer check. I deleted the whole gate (thedid_matchesbranch, theAND assignee_did=$5, and the bind) and all six task tests stayed green, which leaves the signature change unproven. A directdb.finish_task(id, "completed", None, stranger)assertingErrand that the row staysclaimedcovers it; I wrote that test and confirmed it goes RED with the gate removed and GREEN with it in place. -
[P2] Keep the stored DID form when claiming a reserved task
crates/gitlawb-node/src/db/mod.rs:2635
The UPDATE writesassignee_did=$2, the caller-presented form, even when the row already held a different-but-matching one. Executed: a reservation stored aszBAREKEY...claimed asdid:key:zBAREKEY...succeeds and rewrites the row to thedid:key:form.list_tasksfilters on exactassignee_did=$1(db/mod.rs:2572,2589), so the delegator who reserved the task by the bare key stops finding it afterwards.SET assignee_did=COALESCE(assignee_did, $2)fixes it; I ran it and confirmed a reserved task keeps its stored form while an open task still takes the claimer's DID. -
[P3] Treat a blank reservation as open
crates/gitlawb-node/src/db/mod.rs:2624
create_taskbindsassignee_didfrom the body without validation, so""reaches the gate asSome(""), enters the reserved branch and matches nobody. Executed: every claimer getstask not claimable: reserved for another assignee, permanently. Filtering blank at the gate (.as_deref().filter(|r| !r.trim().is_empty())) makes it claimable again; verified. -
[P3] Narrow the pre-check to the columns the decision needs
crates/gitlawb-node/src/db/mod.rs:2615
claim_tasknow loads the full row, includingpayload,resultanducan_token, before rejecting a caller who is not the reservation holder. The old single-statement UPDATE rejected without returning anything, so a permissionless caller can replay a denial and make the node read the whole row each time. Bounded by the default body limit rather than unbounded, hence P3, but aSELECT status, assignee_didwould keep the denial path cheap.
Not an ask, recorded only: the doc comment and the PR body both credit the new assignee clause in the UPDATE with closing the race. assignee_did has one writer, claim_task's own UPDATE, which requires status='pending', so the pre-existing status predicate already closes that window. I removed the new clause and all six task tests stayed green. It is reasonable defense-in-depth against a future writer; the wording is what overstates it.
Map reserved-assignee denial to 403 via a downcastable marker error. Keep the stored assignee DID form on reserved claims, treat blank reservations as open, narrow the pre-check SELECT, and cover finish_task assignee binding directly at the DB layer.
|
@beardthelion addressed your review findings:
Also softened the race wording to defense-in-depth (status predicate remains the primary window closer). Ready for another look. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Findings
-
[P2] Required PR checks did not run on the new head
GitHub Actions run 30586459176 forcdbee5a0completed withaction_requiredand zero jobs (test (stable),fmt + clippy,build --release, etc. are absent fromstatusCheckRollup). Only triage and CodeRabbit report success. Please have a maintainer approve/trigger the fork workflow and confirm green checks on this head before merge. -
[P3] Align Rust and SQL whitespace handling for blank/open reservations
crates/gitlawb-node/src/db/mod.rs:2638,2651-2652
The""case is fixed, but Rusttrim().is_empty()and PostgreSQLBTRIM()still disagree on tab/newline-onlyassignee_didvalues ("\t","\n"). Rust treats them as open (reserved_exact = None); SQL still sees an occupied slot, so claim always returns 409 and the row stayspending. Apply the same blank definition in Rust and SQL (or normalize whitespace-only values toNULLon create). Also trim consistently infinish_taskdid_matches(db/mod.rs:2690) to match the claim gate (2640) so tab-padded stored values cannot claim then fail complete/fail. -
[P3] Emit
TaskEventBroadcast.by_didfrom the persisted assignee
crates/gitlawb-node/src/api/tasks.rs:188,242,296;crates/gitlawb-node/src/graphql/mutation.rs:80,122,164
The COALESCE fix keeps bare-keyassignee_didin the row while broadcasts still send the signer's fulldid:key:…form. That skew is new with this PR (previously claim overwrote to caller form). Subscribers or filters doing exact-string correlation withlist_taskswill miss events. Please setby_didfrom the returned task'sassignee_didon claim, complete, and fail. -
[P3] Match GraphQL steal test parity with REST
crates/gitlawb-node/src/graphql/mutation.rs:326
REST reloads the row after a failed stranger claim and assertsstatus == "pending"and unchangedassignee_did(test_support.rs:302-304). The GraphQL test only checks error strings and UCAN absence. Please add the sameget_taskassertions.
Match Rust/SQL blank slots via BTRIM of space/tab/newline/CR, normalize whitespace-only assignees on create, trim finish_task matching, emit TaskEventBroadcast.by_did from the persisted assignee, and assert GraphQL steal leaves pending+assignee untouched. Co-authored-by: Cursor <cursoragent@cursor.com>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/gitlawb-node/src/db/mod.rs (1)
2543-2569: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winNormalize
assignee_didbefore returning create responses.
create_taskstores whitespace-only assignment requests asNULL, but bothcreate_task(&task)call sites returntask_to_json(&task)/AgentTaskType::from(task)from the same pre-normalized input struct. If a request sendsassignee_did: " "for a non-pre-set value, the response exposes it as reserved while the persisted row and later task reads expose it as open. Normalize the returned task struct in both GraphQL and REST create paths, or mutate it inDb::create_taskand return the normalized value from the stored row.🤖 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/gitlawb-node/src/db/mod.rs` around lines 2543 - 2569, Normalize whitespace-only assignee_did consistently in both create response paths that call Db::create_task, covering task_to_json and AgentTaskType::from conversions. Ensure the returned task uses None/null when assignee_slot_blank detects a blank value, matching the persisted database row and subsequent reads.
🤖 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/gitlawb-node/src/db/mod.rs`:
- Around line 2662-2680: Update the UPDATE statement in the task-claim flow to
use the already-read exact reserved value parameter ($4) directly in the SET
expression, preserving the stored assignee DID including whitespace; keep the
existing blank-assignee fallback behavior for unreserved claims. Extend
claim_task_keeps_stored_assignee_did_form with a whitespace-padded reserved DID
to verify the persisted value remains unchanged.
---
Outside diff comments:
In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 2543-2569: Normalize whitespace-only assignee_did consistently in
both create response paths that call Db::create_task, covering task_to_json and
AgentTaskType::from conversions. Ensure the returned task uses None/null when
assignee_slot_blank detects a blank value, matching the persisted database row
and subsequent reads.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b2c0922b-9cd4-4545-9099-ceea98e047d8
📒 Files selected for processing (4)
crates/gitlawb-node/src/api/tasks.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/graphql/mutation.rscrates/gitlawb-node/src/test_support.rs
|
@jatmn @beardthelion addressed the latest review on head
CI (P2): fork PR Checks still need a maintainer to approve workflows on the latest head — please approve when you can so the suite can run against this commit. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Run and satisfy the required checks on this head
.github/workflows/pr-checks.yml/ current head139b8da
The only check runs attached to this head are the two triage jobs and CodeRabbit; the Rust test, fmt/clippy, release-build, audit, MSRV, and Docker checks have not run. The fork workflow needs maintainer approval/triggering before merge. In addition,cargo fmt --all -- --checkcurrently fails on this submitted head (including the new task changes), so the format job will fail once it runs. Please format the patch and get the full required workflow green on this head. -
[P2] Preserve the exact nonblank reservation when claiming
crates/gitlawb-node/src/db/mod.rs:2664
The authorization and race predicate bind$4to the exact storedassignee_did, but the update storesBTRIM(assignee_did, ...)instead. A reservation such as" did:key:z...\t"is accepted for its matching signer and then silently rewritten without the surrounding whitespace. That contradicts the new stored-form preservation contract and makes exactlist_tasks(..., assignee_did=<original>)filters lose the task after claim. Use the exact reserved value for the reserved branch, and apply the blank normalization only when filling an open slot; add a padded-reservation regression test. -
[P2] Return the normalized assignee value from task creation
crates/gitlawb-node/src/db/mod.rs:2543,crates/gitlawb-node/src/api/tasks.rs:126,crates/gitlawb-node/src/graphql/mutation.rs:58
create_tasknow persists an all-whitespaceassignee_didas SQLNULL, but both REST and GraphQL serialize the original in-memoryAgentTask. Consequently, a create response forassignee_did: " \t"says the task is reserved, while an immediate GET/list reports it as open and another agent can claim it. Normalize the returned task (or return the persisted row) in both paths and cover both response contracts.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The claim gate still holds on 139b8dac. Reverting the reserved did_matches deny still reds both steal tests. jatmn's two code points are true on this head: the claim SET re-derives through BTRIM instead of keeping the reserved form, and create returns the input assignee after the DB has nulled a blank. cargo fmt --all -- --check fails too, and every hunk is in files this PR touches.
One scope note carried from last round: the anonymous read of ucan_token through GET /api/v1/tasks/{id} and the GraphQL task query is still open (#268). The new "UCAN must not leak on a failed steal" assertions only inspect the 403 envelope, which never carried task fields, so they pass while the token stays readable without claiming. Keep the claim-path assertions; the leak framing still overclaims.
Findings
-
[P2] Keep the exact nonblank reservation on claim, and trim REST finish if you do
crates/gitlawb-node/src/db/mod.rs:2664
Same ask as jatmn and CodeRabbit. The SET is stillCOALESCE(NULLIF(BTRIM(assignee_did, E' \t\n\r'), ''), $2), so a padded reservation is rewritten on claim and an exact list filter loses it. The docstring at:2630still says the reserved form is kept viaCOALESCE. Do not landCOALESCE($4, $2)alone: REST complete/fail still compare untrimmed while GraphQL trims, and that couple creates a cross-surface 403 for the real assignee. Related: reserve bare, claim asdid:key:<same>, then list by the claimer's full DID returns 0 rows while the stored bare form returns 1, becauselist_tasks(:2582) is plain equality while claim went throughdid_matches. Either keepBTRIMand fix the docstring, or keep the stored form and trim REST on the same ASCII blank set. Add a padded-reservation regression, and a bare-then-full list regression, either way. -
[P2] Return the normalized assignee from create
crates/gitlawb-node/src/api/tasks.rs:120
Same as jatmn.Db::create_tasknulls a whitespace-only assignee; REST still returnstask_to_json(&task)and GraphQLcreate_task(mutation.rs:55) still returnsAgentTaskType::from(task), so create can show""while the row is NULL/open. Normalize or re-read before serializing; cover both surfaces. -
[P2] Pin the named guards with fail-on-remove proofs
crates/gitlawb-node/src/db/mod.rs:2633
The claim UPDATE assignee re-check,create_task's blank filter, andfinish_task'sAND assignee_did=$5can each be removed with the task suite staying green (the finish case because the Rust gate returns first). Related:claim_task_blank_reservation_is_openseedsSome("")throughcreate_task, which now stores NULL, so it never reaches either blank branch it is named for. Seed the row directly, and add proofs that fail when each of those three guards is removed. -
[P2] Format the touched files
crates/gitlawb-node/src/api/tasks.rs
cargo fmt --all -- --checkexits 1 on this head. The six diffs are inapi/tasks.rs,db/mod.rs, andgraphql/mutation.rs, so this is yours to clear before the next push. -
[P3] Surface the reserved deny distinctly in GraphQL
claimTask
crates/gitlawb-node/src/graphql/mutation.rs:77
REST now returns 403 for a reserved steal and 409 for a lost race.claimTaskstill maps every db error throughe.to_string()with no downcast, so a GraphQL client cannot tell those apart without parsing the message. That is also why the new test has to assert oncontains("reserved") || contains("not claimable").
One process note, not a finding: PR Checks is action_required with zero jobs on this fork head. That is mine to approve, not yours to clear. The fmt finding above is the part you can fix on your own.
Use COALESCE($4, $2) so padded reservations are not BTRIM-rewritten on claim; return normalized assignee from create_task for REST/GraphQL; trim REST finish on the same ASCII blank set; surface reserved GraphQL denies distinctly; add padded/blank/SQL-guard regressions; rustfmt. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@beardthelion @jatmn addressed review 4831516189 on head
UCAN anonymous-read framing note acknowledged — still #268; claim-path assertions kept. CI: please approve fork PR Checks on this head when you can. |
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The claim gate still holds on 62172cf. Gutting the reserved did_matches deny reds both steal tests again (REST stranger lands 200). The r2 behavioral asks landed: claim SET is COALESCE($4, $2), padded reservations survive and can finish after trim_assignee_did, create returns the normalized row (REST create with " \t\n " returns assignee_did: null), GraphQL surfaces the reserved deny distinctly, and cargo fmt --check is clean. create_task_normalizes_whitespace_assignee goes RED when the blank filter is removed, and the padded test goes RED when SET is reverted to BTRIM.
Two named guards from the r2 pin-guards ask are still unpinned. Replacing the claim UPDATE assignee slot re-check with AND (true) leaves all ten filtered task tests green. Removing AND assignee_did=$5 from Db::finish_task leaves both finish tests green: the Rust gate returns first, and finish_task_sql_assignee_predicate_is_load_bearing runs a parallel UPDATE that never calls production.
One scope note carried again: anonymous GET /api/v1/tasks/{id} and the GraphQL task query still serve ucan_token (#268). Keep the claim-path assertions; the deny-envelope "must not leak" framing still overclaims.
Findings
-
[P2] Pin the claim UPDATE assignee re-check with a fail-on-remove proof
crates/gitlawb-node/src/db/mod.rs:2673
The docstring still calls theAND (($4 IS NULL AND open) OR assignee_did = $4)clause defense-in-depth against a writer racing the pre-check. I deleted that clause (leftAND (true)) and the claim/finish/create task filter suite stayed 10/10 green, including both steal tests (the Rust reserved deny fires first). Add a proof that fails when that production predicate is removed. -
[P2] Pin
Db::finish_task'sAND assignee_did=$5, not a parallel query
crates/gitlawb-node/src/test_support.rs:807
finish_task_sql_assignee_predicate_is_load_bearingruns its ownUPDATE ... AND assignee_did=$4with a mismatched bind and asserts zero rows. That stays green after I removeAND assignee_did=$5fromdb/mod.rs:2720, because the test never callsDb::finish_taskand the stranger path is stopped by the Rustdid_matchesgate at:2713. Replace it with a proof that fails when the production clause is gone.
Not an ask, recorded only: collision band is EXPECT-REBASE (mechanical overlap on db/mod.rs); expect a rebase, not a redone review of the claim gate.
Superseded by re-review on 62172cf
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Pin the claim UPDATE assignee re-check with a fail-on-remove proof
crates/gitlawb-node/src/db/mod.rs:2673
The re-check is still documented as defense-in-depth against a writer changing the reservation after the pre-check, but no current test reaches that production predicate. Replacing the whole condition withAND trueleaves the task filter suite green because the existing steal tests stop at the earlier Rust denial. Add a production-path race (or focused equivalent) that changes the pending row's assignee slot after the pre-read and asserts that this claim cannot update it. -
[P2] Make the claimed fail-on-remove test exercise
finish_task
crates/gitlawb-node/src/test_support.rs:807
This test still executes a second hand-writtenUPDATErather thanDb::finish_task, so it remains green ifAND assignee_did=$5is removed from the production update. The other new DB test also stops at the Rustdid_matchescheck, leaving the check-then-act protection untested. Please test the production path with an assignee change between its read and update (for example through a controlled concurrent update/trigger) and assert that it leaves the task claimed; otherwise the new race guard can be deleted without a regression signal.
3 participants
Summary
claim_taskpreviously updated anypendingrow and overwroteassignee_did, so a stranger could steal a task pre-assigned to someone else and receive itsucan_token/ payload.NULL) tasks; the UPDATE re-checks the assignee slot to close the race.finish_tasknow binds the stored assignee in SQL so complete/fail cannot win a check-then-act race after the handler gate.Why (direct PR)
Real authz gap on the agent-task surface (REST + GraphQL + DB). Not tracked by an existing issue/PR (distinct from #268 anonymous UCAN reads).
Test plan
claim_task_honors_preassigned_assignee(REST) — thief gets 409, no UCAN; reserved assignee claims and receives UCANclaim_task_open_still_first_claimer_wins— open pending tasks still claimableclaim_task_honors_preassigned_assignee(GraphQL) — same steal/admit behaviorcomplete_task_authorizes_assignee_only/ GraphQL complete tests still passcargo clippy -p gitlawb-node --all-targets -- -D warningscleantest (stable)with PostgresSummary by CodeRabbit