feat(agents): lane-neutral agent-claim/<issue> arbitration - #2477
feat(agents): lane-neutral agent-claim/<issue> arbitration#2477cursor[bot] wants to merge 3 commits into
Conversation
> 🤖 Generated by the Agentic Engineer (Cursor cloud instance) Reserve lane-neutral agent-claim/<issue> work for monorepo#2302. Co-authored-by: Nikolai Emil Damm <devantler@users.noreply.github.com>
> 🤖 Generated by the Agentic Engineer (Cursor cloud instance) Close the cross-lane claim hole (#2302): every instance acquires a shared agent-claim/<issue> tip via agent-claim.sh before its lane work branch. Portable /dev/urandom nonce (fail-closed), tip-compare not exit-status, retire-on-PR-open, evidence-gated stale takeover. RED/GREEN covers all four proven traps. Removes the Claim protocol KNOWN HOLE note. Co-authored-by: Nikolai Emil Damm <devantler@users.noreply.github.com>
Wire agent-claim.sh's four-trap self-test into CI on both OSes so the cross-lane claim arbitration cannot regress without failing the PR gate. Co-authored-by: ned <ned@devantler.tech>
2efd368 to
0c0bb2c
Compare
@coderabbitai review Cursor-lane draft, CI green and CLEAN against base, no review at this head. Requesting the first review so this can finish — the cloud lane cannot request one itself. |
|
I’ll review PR ✅ Action performedReview finished.
|
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
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 |
User evaluation — exercised the CLI, not just the test suiteRan The cross-lane race — the hole this exists to close. Three lanes called
One winner, two losers, each told what happened and what the recovery is. Under the current work-branch protocol all three would have pushed successfully into their own namespaces and all three would have believed they won — which is the failure the contract itself records as a known unarbitrated hole. The remaining semantics, each checked in both directions:
The negative cases matter as much as the positive ones here: a claim helper that cannot refuse is a lock that never releases, and one that always refuses is a deadlock. Both directions behave. Test suite: all 23 trap assertions pass, including the two that motivated the design — a pipe-masked push whose exit status reports success on a rejected push, and two writers producing byte-identical claim commits so both pushes succeed. The nonce path fails closed when no entropy source is readable rather than silently reverting to a fixed message. Programmatic tests and user evaluation are both satisfied; a current-head external review is the one remaining readiness condition. |
@coderabbitai review Re-triggering after the earlier attempt was declined on a per-developer review limit with a short stated retry window; that window has now elapsed. CI is green at this head and the branch is CLEAN against base. Exercised before requesting: |
|
Your plan includes PR reviews subject to rate limits. More reviews will be available in 51 minutes. |
@codex review CodeRabbit declined again at this head. The retry window it stated grew from ~13 minutes to ~51 on the second attempt, which is the adaptive per-developer limit rather than anything about this change — so this advances to the next lane rather than waiting past the end of the run. Both non-review readiness conditions are already met and recorded above: the 23-trap self-test passes, and the CLI was exercised end-to-end against a real remote with three lanes racing one issue — one winner, two clean losses, plus verify/lease/takeover/retire checked in both directions. |
Pre-merge diligence: checked that the rule-3 rewrite drops no protectionThis PR rewrites a large block of the Claim protocol ( Every force-push protection survives — verified against the blob at
Two things genuinely go away, and both are coherent supersessions rather than losses. The warning never to time a lease from a branch's commit date, and the One correction to my own method, in case it matters to a reviewer: my first pass reported several of these as missing. That was a bad probe, not a real removal — Combined with the recorded test run (23/23 traps) and the CLI exercise (three lanes racing one issue, plus verify/lease/takeover/retire in both directions), the only readiness condition still outstanding is a green review at this head. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c0bb2cbd0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| git_c push --quiet "$REMOTE" ":${branch}" \ | ||
| || fail "could not delete stale ${branch} before takeover" |
There was a problem hiding this comment.
When a stale takeover passes claim_is_stale, another lane can retire and reacquire the ref before this unconditional deletion; the takeover then deletes that fresh holder and installs its own tip, so both lanes have observed a win. cmd_retire has the same unguarded deletion and can erase a post-takeover holder. Use the observed SHA for an atomic compare-and-delete; git push -h documents --force-with-lease=<refname>:<expect> as “require old value of ref to be at this value,” so a changed tip can fail closed.
AGENTS.md reference: AGENTS.md:L702-L707
Useful? React with 👍 / 👎.
| # Empty commit with a UNIQUE message (nonce). Identical messages on the same | ||
| # parent in the same second produce byte-identical commits (trap 3) — the | ||
| # nonce is the whole defence. | ||
| sha="$(git_c commit-tree "$parent^{tree}" -p "$parent" -m "chore: agent-claim #${issue} nonce=${nonce}")" |
There was a problem hiding this comment.
When the remote default branch advances after this checkout's last fetch—common for pinned submodule checkouts—ls-remote returns a SHA whose object is absent locally, so this commit-tree invocation exits 128 with not a valid object and mandatory claim acquisition cannot proceed. I reproduced this with a clone followed by one remote-only commit; git commit-tree -h also defines -p <parent> as the “id of a parent commit object.” Fetch the SHA first or anchor the claim commit on a guaranteed-local object.
AGENTS.md reference: AGENTS.md:L646-L654
Useful? React with 👍 / 👎.
| from crossing lanes. Cross-lane claim races are arbitrated on the shared `agent-claim/<issue>` tip | ||
| (see *Claim protocol*), acquired before the lane work branch. |
There was a problem hiding this comment.
In every conforming runtime, this mandatory push targets agent-claim/*, but the Writer namespaces contract authorizes only claude/*, codex/*, and cursor/* and explicitly says a role cannot invent an unrecorded fourth lane. The first claim must therefore either fail closed as an unauthorized write or violate the deployment's namespace isolation; the Cursor loader's measured capability also establishes only writes to cursor/*, not this new shared prefix. Record explicit shared coordination-ref ownership and verify each runtime's permission before requiring the push.
AGENTS.md reference: AGENTS.md:L363-L378
Useful? React with 👍 / 👎.
| before you build — lane-neutral tip FIRST:** acquire `agent-claim/<issue>` via | ||
| `.claude/scripts/agent-claim.sh acquire <issue>` (cross-lane race; LOST → stand down), then |
There was a problem hiding this comment.
When the ladder selects an issue from a submodule, the session is still at the monorepo checkout here, so this documented command leaves REPO_DIR empty and pushes the root origin; invoking it after changing into the product instead makes the relative script path unavailable, and an uninitialized submodule has no repository to operate on yet. Because issue numbers are repository-scoped, this can lock an unrelated monorepo issue while leaving the intended product issue unclaimed. git -h exposes -C <path> and the helper already exposes --repo-dir; initialize the target and invoke the root helper with that repository for acquire, verify, and retire.
AGENTS.md reference: AGENTS.md:L618-L620
Useful? React with 👍 / 👎.
All four P1s are valid — and this corrects my previous commentMy earlier diligence comment said the only outstanding readiness condition was a green review. That was wrong in substance. I checked whether the rewrite removed an existing protection, and it does not — but I never asked whether what it adds contradicts another section, and two of these findings are exactly that. Recording the correction rather than quietly moving on, because that comment reads as an endorsement. I verified the two contract-level findings directly against the blob at Unauthorized fourth namespace — confirmed. The Writer namespaces table at this head still lists exactly Wrong-repository claims — confirmed. The two script findings I take on the reviewer's reasoning, and both look right to me:
Worth being explicit about my own evaluation: I exercised acquire / verify / lease / takeover-inside-lease / stale-takeover / retire, all against a fully-fetched local remote, and everything passed. That is real but it did not cover either script defect — I never raced a retire-and-reacquire during a takeover, and every clone in my lab already had the parent object. Passing hand-run checks is not the same as covering the concurrent and partially-fetched paths, and this is a good example of the difference. HandoffThis is Cursor-lane code, so I am not pushing fixes; my handoff covers hygiene, evaluation, promotion and merge, not cross-lane commits. All four threads deliberately left open. Sequencing note for whoever picks it up: the namespace-authorization fix is the blocking one — the other three can be correct and this change still cannot ship while the contract forbids the ref it requires. |
2 participants
Why
Cross-lane claim races were never arbitrated: each instance writes its own namespace, so two writers can both push and both believe they won. That discarded real builds and left the known hole in the claim protocol.
What
Adds a shared
agent-claim/<issue>tip every lane acquires before its work branch, with portable fail-closed nonces, retire-on-PR and evidence-gated stale takeover, contract/run-loop updates, and a CI-gated four-trap self-test.Fixes #2302