fix(node): honor pre-assigned task assignee on claim - #275
Conversation
|
Warning Review limit reached
Next review available in: 12 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTask creation now returns the persisted task. Task claiming enforces reserved-assignee authorization. Completion and failure require actor DID matching. API and GraphQL events use persisted assignee data. Tests cover reservation, DID normalization, UCAN handling, and concurrency safeguards. ChangesTask claim and completion authorization
Repository rule link
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 labels: 🚥 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.
|
@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.
![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.
|
@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.
The needs-tests detector matched an added `#[test]` or `#[cfg(test)]` line and nothing else, so it missed every test written with a harness attribute. That is most of this repo: 345 `#[tokio::test]` and 189 `#[sqlx::test]` against 540 bare `#[test]`. Any PR whose tests are all DB or async tests got labeled needs-tests, which is what happened on #275 (three added `#[sqlx::test]` functions) and #262 (eight). Match an added attribute whose last path component contains `test`, followed by `]` or `(`. That covers the four forms in the tree today and the harnesses a contributor might reach for later (`#[rstest]`, `#[test_case]`, `#[wasm_bindgen_test]`) without enumerating them. Deliberately not keying on an added `assert!`: a production assertion is ordinary Rust, so that signal would let a testless PR clear the label silently. The label is a nudge, so it should fail loud. Labeling a PR that did add tests gets corrected by the author; clearing one that did not is invisible. Anchoring to the start of the added line also drops three false positives the old `.*` prefix accepted: a comment, a string literal, and a doc example that merely mention `#[test]`. Verified against the real patches of 14 open PRs: #275 and #262 stop being labeled, both of which do add tests, and no other verdict changes.
github-actions
Bot
removed
the
needs-tests
664a12c to
3c126b7
Compare
|
@beardthelion @jatmn — rebased onto current R3 pin-guards (your latest round on
Prior rounds (still on this head)
Scope note (unchanged): anonymous Ready for another look. |
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] Rebase onto current
mainand resolve the conflict
crates/gitlawb-node/src/db/mod.rs:7035
GitHub currently reports this head asCONFLICTING/DIRTY. The three-way merge conflicts in the peer-writer guard ledger, so this cannot merge as submitted. Please rebase, resolve the conflict while preserving the current-main security changes, and rerun the checks; the resolved head needs a fresh review.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Both round-3 asks are closed, and I checked by execution rather than by reading the replies. Deleting the claim UPDATE's assignee-slot predicate now reds claim_task_update_assignee_slot_recheck_is_load_bearing; neutering finish_task's AND assignee_did=$5 reds finish_task_sql_assignee_predicate_is_load_bearing. Last round both of those deletions left the suite green. I re-ran the full named-guard set on this head, six mutations, each red on its own assertion message: the reserved deny on REST and GraphQL, those two predicates, create_task's blank-to-NULL filter, and the COALESCE($4, $2) exact-form SET. Baseline is 21 passed.
The blocker is the conflict jatmn already flagged, and it resolves more easily than it looks. Both peer test functions already existed at your base while the ledger there omitted them, so your addition repaired a guard that was failing at that base; main fixed the same thing in af821e09 with slightly different wording. It is a duplicate add, so take main's version of the doc table and the LEDGER rows and keep the rest of your diff intact. None of your task changes conflict. #247 landed on main today and touched only the error and events modules, so the rebase has no gate to lose on your surfaces, and your GraphQL path already picked up graphql_db_err correctly.
Three small things to fold into that push.
Findings
-
[P3] Trim the assignee before putting it in the task event
crates/gitlawb-node/src/api/tasks.rs:188
I drove the REST claim handler with a reservation stored as" zKEY\t"and read the broadcast:by_didcame out as" zKEY\t"while the authenticated caller wasdid:key:zKEY. A field namedby_didshould not carry padding or a bare key.trim_assignee_didis already in scope. Same one-liner at the other five sites. -
[P3] Make the claim race assertion provable only through the SQL re-check
crates/gitlawb-node/src/test_support.rs:1159
The test assertscontains("not claimable")and both denials say exactly that: the Rust pre-check is"task not claimable: reserved for another assignee", the SQL miss is"task not claimable: not found or already claimed". If the background writer commits before the pre-read, the test passes through the pre-check and proves nothing about the predicate it is named for. It does exercise that predicate today, since removing it goes red, but the assertion should be what guarantees it. Assert the specific message, or that the error is not aTaskReservedForOtherAssignee. The finish test does not have this problem:"not in claimed state"cannot match its Rust gate's message. -
[P3] Cover the other blank characters
crates/gitlawb-node/src/test_support.rs:830
ASSIGNEE_BLANKand the SQLBTRIM(assignee_did, E' \t\n\r')are the same four characters today, and a comment says they must stay that way. The SQL blank branch is only ever seeded with''and a tab, so space, LF and CR never reach it. Extending the tab test to the remaining three is the cheapest guard against those two definitions drifting.
Nothing else holds this up. Once the rebase is clean I will re-run the guard set on the resolved head.
Both asks in this review are satisfied on 3c126b7 and I re-ran them: deleting the claim UPDATE assignee-slot predicate and neutering finish_task AND assignee_did=$5 each go red now. Superseded by my review on the current head.
3c126b7 to
e5a8468
Compare
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)
3809-3853: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake normalized profile selection deterministic.
agent_profilesuses rawdidas its primary key, soz6Mkfooanddid:key:z6Mkfoocan coexist. Both matchPROFILE_DID_CASE_SQL.Use one deterministic row selector in
get_profileandset_profile_cid. The current lookup can return either row, while the current update changes every matching 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 3809 - 3853, The PROFILE_DID_CASE_SQL predicates in get_profile and set_profile_cid must select the same deterministic profile when both bare and did:key forms coexist. Add an ordering that prefers the canonical did:key representation, limit get_profile to one row, and constrain set_profile_cid to update only that selected row rather than every match.
🧹 Nitpick comments (1)
crates/gitlawb-node/src/db/mod.rs (1)
2267-2275: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDo not swallow the existence-probe error.
unwrap_or(false)converts a transient database failure into "the row does not exist". The strict DID validation arm then runs and refuses a legacy row that is present, so an operator seesmethodNotSupportedorcannot resolve DIDinstead of the real database fault. Propagate the error with?so the caller reports the true cause.♻️ Proposed change
PeerWriteAuthority::Unproven if !sqlx::query_scalar::<_, bool>( "SELECT EXISTS(SELECT 1 FROM peers WHERE did = $1)", ) .bind(did) .fetch_one(&self.pool) - .await - .unwrap_or(false) => + .await? =>🤖 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 2267 - 2275, Update the PeerWriteAuthority::Unproven existence probe to propagate fetch_one errors with ?, removing unwrap_or(false), so database failures reach the caller instead of being treated as a missing peer.
🤖 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 @.cursor/rules/rtk-token-savings.mdc:
- Line 1: The .cursor/rules/rtk-token-savings.mdc symlink uses a
developer-specific absolute target and must be portable. Replace it with the
tracked rule content or a repository-relative symlink to the corresponding
tracked file, ensuring other checkouts can load the rule.
---
Outside diff comments:
In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 3809-3853: The PROFILE_DID_CASE_SQL predicates in get_profile and
set_profile_cid must select the same deterministic profile when both bare and
did:key forms coexist. Add an ordering that prefers the canonical did:key
representation, limit get_profile to one row, and constrain set_profile_cid to
update only that selected row rather than every match.
---
Nitpick comments:
In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 2267-2275: Update the PeerWriteAuthority::Unproven existence probe
to propagate fetch_one errors with ?, removing unwrap_or(false), so database
failures reach the caller instead of being treated as a missing peer.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eb482e20-c340-4892-acd5-3b42794815e9
📒 Files selected for processing (5)
.cursor/rules/rtk-token-savings.mdccrates/gitlawb-node/src/api/tasks.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/graphql/mutation.rscrates/gitlawb-node/src/test_support.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/gitlawb-node/src/api/tasks.rs
- crates/gitlawb-node/src/graphql/mutation.rs
- crates/gitlawb-node/src/test_support.rs
| @@ -0,0 +1 @@ | |||
| /Users/ayushkumar/.cursor/rules/rtk-token-savings.mdc No newline at end of file | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Replace the developer-specific absolute symlink target.
Line 1 points to /Users/ayushkumar/.cursor/rules/rtk-token-savings.mdc. Other checkouts will have a dangling link, so Cursor cannot load this rule. Commit the rule in the repository or use a repository-relative symlink to a tracked file.
🤖 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 @.cursor/rules/rtk-token-savings.mdc at line 1, The
.cursor/rules/rtk-token-savings.mdc symlink uses a developer-specific absolute
target and must be portable. Replace it with the tracked rule content or a
repository-relative symlink to the corresponding tracked file, ensuring other
checkouts can load the rule.
Stop strangers from overwriting a reserved assignee_did (and receiving the UCAN). Bind finish_task to the stored assignee; return 403 on reserved steals; keep exact stored DID forms; normalize blank assignees; pin production claim/finish SQL race guards with FOR UPDATE tests; trim broadcast by_did; and tighten blank/SQL re-check regressions.
e5a8468 to
1676eb0
Compare
|
@beardthelion @jatmn — rebased onto current Conflict: took main's peer-writer LEDGER wording (duplicate of our prior add from #248 fixtures). Your latest P3 round:
Prior pin-guards and behavioral fixes remain on this head. Ready for re-review when CI is green. |
Windows CI can race: git fetch connects before the shim accept loop is polling, yielding connection aborted (os error 10053) with 0 POSTs. Probe the listener after spawn so multi-round and withheld-path tests start only once the shim is ready.
|
Pushed The failure was a race on Windows CI: All other required checks were already green on |
…rors Windows CI can still abort localhost connections (os error 10053) after the shim is ready. Retry fetch_with_helper up to three times on transient connection failures before asserting.
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] Do not retry the stateful withheld-first-POST scenario against the same shim
crates/git-remote-gitlawb/tests/real_git_fetch.rs:264
fetch_with_helpernow retries every matching connection error, butreal_git_withheld_shaped_first_postreuses oneShimMode::WithheldFirstPostinstance. If the first fetch reaches its first POST and then fails with one of those transient diagnostics, that POST consumes the shim's one special NAK+full-pack response. The retry is served ordinaryupload-pack; if it succeeds, the test takes its success branch and falsely certifies the withheld-first-POST behavior without a successful fetch ever exercising that behavior. Keep retries out of this stateful case, or recreate/reset the shim and fixture for each attempt. -
[P2] Synchronize with the accept-loop thread instead of the listener backlog
crates/git-remote-gitlawb/tests/real_git_fetch.rs:131
TcpListener::bindhas already put the socket in the listening state before the accept-loop thread is spawned, soTcpStream::connect_timeoutsucceeds once the OS queues the TCP handshake; it does not require the spawned thread to have reachedaccept().wait_for_shim_readycan therefore return while the scheduling race described in its comment remains, leaving the Windows flake unsynchronized. Have the spawned thread send an explicit readiness acknowledgement (or complete a protocol-level probe) beforestart_shimreturns.
Superseded by the re-review on 0484740.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The task work is done, and I checked it by execution on this head rather than by reading the replies. All three of last round's asks landed: trim_assignee_did is applied at the by_did sites, the race test now asserts the error is not a TaskReservedForOtherAssignee so the SQL predicate is what denied it, and the blank test seeds empty, space, tab, LF and CR. I re-ran the guard set on 0484740, one gut at a time. Removing the reserved deny in claim_task reds both steal tests; deleting the claim UPDATE's assignee-slot predicate reds the race guard; neutering finish_task's AND assignee_did = $5 reds its twin; dropping create_task's blank-to-NULL filter reds the normalization test; rewriting the COALESCE($4, $2) SET to a BTRIM reds the padded-reservation test. Baseline is 22 passed. The rebase came out the way I asked and the duplicate peer-writer ledger rows are gone.
What holds this up is the Windows CI work, and the answer is to take it out rather than patch it. That covers both of jatmn's asks on this head. Separately, the open CodeRabbit thread on .cursor/rules/rtk-token-savings.mdc points at a file that is no longer in the diff, so you can resolve it.
Findings
- [P2] Revert
real_git_fetch.rsto main and let #312 fix the Windows lane
crates/git-remote-gitlawb/tests/real_git_fetch.rs:262
The retry keys on10053, and that is the signature of a real defect rather than a flake: on Windows the accepted socket inherits the listener's non-blocking state, soread_linereturnsWouldBlock, the shim drops the connection without writing a response, and the request never reaches the branch that counts POSTs. #312 fixes that at the source and is approved and mergeable, so this retries a live bug into silence. It also lets the withheld test certify a shape it never sent.postsis shim-global and the withheld response fires once, so I set that arm to never fire, which is exactly the state a retry leaves behind, and the test passed through its success branch with no note printed.wait_for_shim_readydoes not close the gap either, since a listener that is bound completes a connect withaccept()never called. The same three hunks are byte-identical in #276. Your base blob for this file matches main, so the revert is clean, and the Windows job is non-blocking, so nothing here goes red.
The retry-on-10053 and wait-for-shim probes papered over the accepted-socket inheriting the listener's non-blocking state. Gitlawb#312 fixed that at the source (normalize_accepted_stream) and is merged, so take the workarounds back out and let the real fix carry the Windows lane.
|
Re: the last review round — the Windows CI workaround is reverted on this head ( Both workaround commits ( Verified on this head:
The remaining @beardthelion @jatmn — the round-4 asks are closed; requesting re-review on the new head. CodeRabbit thread is on a file no longer in the diff (GitHub rejects API-resolving it), but it is non-blocking as you noted. |
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Last round's asks landed, and I checked them by execution on this head rather than by reading the replies. The retry is gone (grep -c retry returns 0), the readiness probe is gone, and the withheld test's outcome guards are intact. All 12 checks are green, and I confirmed the Windows result at the step level rather than the job level, since that lane runs continue-on-error: true and would report SUCCESS over a failed step. Step 5 genuinely passed.
Two asks before this merges.
Findings
-
[P2] Rebase onto current
mainso the revert's premise holds at this head
crates/git-remote-gitlawb/tests/real_git_fetch.rs
The commit message credits #312 with carrying the Windows lane, but this head does not contain #312. The file here is blob26d88f27, byte-identical to the pre-#312 base:normalize_accepted_streamappears 0 times against 10 on main, and thestream.set_nonblocking(false)that main carries at line 181 is absent.git merge-base --is-ancestor 10e0840d 68133409returns NO, and the base is 18 commits behind. The shipped state is fine, since the three-way merge resolves this file to72fdd5c2, main's exact blob. The head is what worries me: its Windows run passed on the unmitigated shape, so the green tells us the race did not fire on that run, not that the fix is present. Rebase or cherry-pick #312 and let the lane run against the code it is credited to. -
[P3] Pin the retry's absence with a committed guard
crates/git-remote-gitlawb/tests/real_git_fetch.rs:249
Nothing goes red if a fetch-level retry comes back. The shim counts POSTs only, incrementing at line 209 on the POST branch, and both assertions are lower bounds:posts >= 2at 778 andposts >= 1at 892. A second full fetch satisfies both while masking the withheld shape the test exists to observe. A GET counter asserted to be exactly one per test would make a reintroduced retry fail on the spot.
One housekeeping item, not an ask: the CodeRabbit thread on .cursor/rules/rtk-token-savings.mdc is still open, but that file is absent at this head and never appeared in the PR's net diff, so the work behind it is done and the thread can be resolved.
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