Repository navigation
feat(ownership): optional worker renewal capability + fenced claim evidence (#1251) - #1300
Conversation
…ip states (#1251) Implements the accepted optional register/renew/disposition capability from docs/ownership-reconciliation.md $6 on the existing Lease row, fenced on the #1256 claimToken, plus the $4 derived ownership read model and $9 operator inspect surfaces. No claim release, label cleanup, or requeue in this change. - Lease: nullable ownership columns (registration/renewal occurrence+observed times, renewCount, external-wait marker, terminal disposition) + migration - POST /api/agent-work/ownership (worker-tier, identity-scoped; unbound legacy token refused); renewal CAS under IssueMutationGuard; replay-idempotent register/disposition; late renewal accepted, stale/incarnation tokens rejected - lib/ownership-state.ts pure derivation (9 states incl. UNKNOWN/CONFLICTED for absent/conflicting evidence; absence never means abandonment) - GET /api/issues/{id}/ownership + GET /api/issues/ownership (maintainer-only, claim token never surfaced) - lifecycle taxonomy: ownership_registered/ownership_dispositioned + api:ownership - docs/worker-ownership-capability.md, AGENTS.md env table, design status
- issue-mutation-guard: remove a freshly created "ownership" guard when the operation throws (local, idempotent writes — same posture as "lease"), so one transient failure can no longer permanently busy-lock claim/lease/ ownership on the issue - lease.upsertLease: a claim-path lease write clears terminal disposition and external-wait columns so a same-agent re-claim of a live row never inherits the retired attempt's terminal evidence - ownership-state: REVIEW_OWNED via only an open PR-fix row now reports open_pr_fix_queue (was mislabeled open_linked_pr) - capability route/lib: retryAfter accepted only with disposition "blocked" (a terminal completed/released record must not classify as EXTERNAL_WAIT) - operator list route: bounded candidate scan (1000) + capped, chunked per-candidate derivation (500 / 25) - docs: disposed-but-live lease posture; regression tests for each fix
- disposition non-blocked clears a stale retryAfter/waitReason so a gone owner is never parked as EXTERNAL_WAIT over its terminal evidence - operator list: seed candidates from active lease/AgentWork first so the bounded slice drops label-only candidates before strong ownership signals - expose truncated on the list response and document the bounded semantics - tests pin the disposition wait-clear and the truncated flag
joryirving
left a comment
There was a problem hiding this comment.
REQUEST CHANGES on exact head 8e18d74. This is a substantial and generally well-structured implementation of the optional ownership capability: identity scope is exact-match, token values are redacted from inspect responses, operator endpoints are not worker-allowlisted, state derivation is pure, and the regression coverage is broad. Exact-head CI has typecheck, tests, lint, DB integration/migrations, build, coverage and audit green; the automated review is still in progress and smoke is queued. I found two major correctness defects and one bounded-read gap.
-
Major — re-claim clears terminal evidence without rotating the fencing token (
src/lib/lease.ts:72-97; regression atsrc/lib/lease.test.ts:212-231). WhenupsertLeasereuses a live-but-disposed row, it clearsdispositionand all associated terminal/wait fields but leavesclaimTokenunchanged.loadGuardedLeasethen validates only that same token (worker-ownership.ts:167-176), so the retired worker's old receipt becomes valid again and can register/renew/dispose the new active assertion. This contradicts the #1256 attempt-fencing contract and the PR's own claim that re-claim clears old-attempt evidence. Either do not reactivate a terminal row through an idempotent claim, or rotateclaimTokenatomically when creating the fresh attempt; ensure the claim response returns the new token. Add a regression that reclaims a live disposed row, proves the old token returnsstale_claim_token, and proves the new token succeeds. The existing test checks only that disposition fields clear and misses the token fence. -
Major — disposition is not recoverable after a partial database write (
src/lib/worker-ownership.ts:415-471, especially replay short-circuit at 397-401;issue-mutation-guard.ts:59-69). The lease's terminal disposition is committed byupdateManybefore updating activeAgentWork, writing its history, recording the lifecycle event and writing the audit row. If any later write throws, the route returns 500 and the ownership guard is deleted. A retry with the same disposition immediately returnsreplayed: trueat the early branch and skips all those unfinished side effects forever. That can leave anIN_PROGRESSAgentWork alongside a terminal lease and/or permanently omit the requiredownership_dispositionedlifecycle event. Put the lease CAS and all required local DB effects in one Prisma transaction (using the transaction client forrecordLifecycleEvent), or make replay explicitly repair missing projections/events idempotently. Add failure-injection regressions for failure after the lease update and after AgentWork update; retry must converge to terminal AgentWork/history + exactly one lifecycle event and not report success while required evidence is missing. -
Minor — operator scan is not fully bounded (
src/app/api/issues/ownership/route.ts:101-110). The open-label source is capped at 1000 and derived inspections at 500, butactiveLeasedandactiveWorkingeach use unboundedfindManywith notake. The endpoint is described as a bounded scan, so these source reads can still materialize the entire active lease and active-work population before the 500-item cap. Bound the source queries and propagate a truncation signal (and make the candidate-selection policy deterministic) rather than relying on the live population staying small.
The remaining architecture is promising; please keep the fixes focused on attempt fencing, transactional/idempotent disposition persistence, and the claimed bounded-read contract. No merge until the two major paths have regression coverage and exact-new-head CI is green.
…1251) Addresses the two MAJOR and two MINOR findings on the review of #1300 at head 8e18d74. - lease.ts: reactivating a live-but-disposed lease row is a NEW attempt, so it now rotates claimToken atomically with the terminal-field clear and resets the retired attempt's registration/renewal provenance. A row with no disposition (an ordinary renewal) keeps its token and evidence, so a heartbeating worker still keeps its receipt. - worker-ownership.ts: the terminal lease CAS and every required local effect (AgentWork settlement, history, lifecycle event, audit row) commit in one prisma.$transaction, with the lifecycle event written through the transaction client. The ownership guard has no resume branch and is deleted on throw, so a CAS committed alone would make the replay branch report success for a disposition whose evidence never landed. - worker-ownership.ts: a register replay returns the persisted registration with replayed: true and writes nothing (no timestamp refresh, no runRef overwrite, no second audit row). The fresh path is transactional too. - issues/ownership route: the active-lease and active-AgentWork candidate sources are now bounded at MAX_CANDIDATE_SCAN with deterministic most-recent-first ordering, and the response reports a per-source sourcesTruncated {labeled, leased, working} signal.
Follow-up to the independent review of 829565d (no blockers; three MINORs and three NITs). - ownership route test: the $transaction mock handed the callback the same client as prisma and rolled back globally, so the atomicity tests could not fail for the reason they claim. It now hands back a distinct tagged client and replays only that object's journal on throw; new tests assert an out-of-transaction write survives rollback, that the lifecycle event is written through the transaction client, and that the disposition retry converges to terminal AgentWork + one history row + one ownership_dispositioned event + one audit row. Reverting tx.* to prisma.* now fails 5 of these. - issues/ownership route: order active leases by createdAt desc FIRST. The create path of upsertLease never sets renewedAt, so a renewedAt-primary nulls-last sort put every newly claimed lease last and the 1000-row cap dropped the freshest claims. - worker-ownership: register's in-transaction lease update is fenced on claimToken, symmetric with the disposition CAS. - module rule and docs: atomicity rule scoped to the operations that assert a new fact (renew is deliberately excluded and says why), docs corrected on token rotation/provenance reset and the transaction + replay contract.
|
Addressed every finding from @joryirving's and the automated review at MAJOR — re-claim does not establish a new attempt fence (
|
joryirving
left a comment
There was a problem hiding this comment.
REQUEST CHANGES on exact head 335f1e6. The two previous MAJOR findings are fixed: disposed-row reclaims rotate the claim token and reset provenance, and register/disposition writes are transactionally atomic with lifecycle/audit evidence. The new rollback harness is materially better, and the final register write is token-fenced. I found one remaining bounded-scan correctness issue and one test-mock issue.
-
MINOR — lease scan order is not actually most-recently-touched (
src/app/api/issues/ownership/route.ts:122-130). The query sortscreatedAt DESCbeforerenewedAt DESC. That prevents never-renewed new claims from falling behind the cap, but it also means an old lease renewed moments ago sorts behind newer-created leases that have not renewed for a long time. With more thanMAX_CANDIDATE_SCANrows, a currently heartbeating long-lived owner can be dropped while stale newer claims are retained. This contradicts the route's “most-recently-touched” contract and the goal of prioritizing current ownership evidence. Order by the effective activity timestampCOALESCE(renewedAt, createdAt) DESC(or implement an equivalent bounded merge of renewed and never-renewed lease sources), with deterministic ID tie-breaking. Add a regression where an old lease renewed recently outranks a newer-created lease with stale/no renewal, including the cap-overflow case. -
NIT — test sorting treats ascending as descending (
src/app/api/issues/ownership/route.test.ts:45-46). Both branches of the comparator return the same descending ordering, soid: "asc"is not simulated faithfully. Correct the comparator and add/tighten a tie-break test so the test double actually enforces the route's deterministic Prisma ordering.
The current head's checks are still running (typecheck and lint have passed; build/tests/coverage/database checks and smoke are not all complete), so this is not merge-ready yet. Please fix the scan order and mock comparator, then rerun exact-head CI. No redesign of the ownership capability requested.
…patch/issue-1251 # Conflicts: # prisma/schema.prisma
…1251) The operator list route scanned active leases with `createdAt` first and `renewedAt` second, so a long-lived claim that its owner was still renewing sorted behind newer claims that had never been renewed. Past the scan cap the live owner was dropped while the stale one was kept — the opposite of what "who owns what right now" needs. Freshness is now the row's most recent ACTIVITY: the last renewal, falling back to the claim time when the claim was never renewed. Prisma's `orderBy` cannot express that combined timestamp, so the lease source is read as two runs (renewed leases by `renewedAt`, never-renewed leases by `createdAt`) and merged on the same key, with `id` ascending as the deterministic tie-break in both the query and the merge. The list route's test double compared `Date` columns by reference, so two rows holding the same instant never compared equal and the `id` tie-break was skipped; it now compares by value and implements ascending/descending (and Postgres' null placement) faithfully, so the tie-break is genuinely enforced.
Follow-up to the lease-scan ordering fix, from an independent adversarial review of the diff: - The list route's test double derived its null placement from `typeof spec === "object" && spec?.nulls`, which is `false` for a plain string sort spec — so `false ?? default` kept the false and NULLs were always placed last, contradicting the double's own documented Postgres behaviour. The fallback is now an explicit conditional. - The test double compared `Date` columns by reference, so two rows holding the same instant never compared equal and the next `orderBy` clause (the id tie-break) was skipped. It now compares by value. - The never-renewed run's `orderBy` was not covered by any test: swapping its sort key or dropping its id tie-break left the suite green. Added cases for the claim-time order, the renewal-time order, and the id tie-break in both runs; each of those mutations now fails the suite. - The activity comment claimed every lease write stamps `renewedAt`; the register and disposition paths deliberately do not. Corrected. - The two runs now read concurrently, narrowing the window in which a lease renewed between them would fall into neither bucket. - The overflow condition is the merged pool's length: it holds every row both runs returned, so it is the exact signal (the per-run clauses were subsumed by it).
|
Disposition of the open findings on this PR, at head Review at
|
| Mutation | Result |
|---|---|
Pre-fix single-query ordering (createdAt first) |
2 failed / 25 passed |
Double sorting asc as desc |
1 failed / 26 passed |
Never-renewed run ordered by renewedAt |
1 failed / 28 passed |
id tie-break dropped from the never-renewed run |
1 failed / 28 passed |
id tie-break dropped from the renewed run |
1 failed / 28 passed |
Renewed run ordered by createdAt |
1 failed / 29 passed |
Renewed run ordered by renewedAt ascending |
1 failed / 29 passed |
| Merge's id tie-break reversed | 1 failed / 29 passed |
5. MINOR — the activity comment claimed every lease write stamps renewedAt. It does not: register and disposition are not renewals and deliberately leave it alone. Corrected in fc1da6c.
6. MINOR — the per-issue selectLease in src/lib/ownership-inspect.ts orders active leases by renewedAt with nulls last, so it is the same class of defect as finding 1 in its narrowed multi-lease case (an issue can carry two active leases — @@unique([agentName, issueId]) is unique per agent — and the scan and the evidence view can then disagree about which lease is operative). Not changed in this PR, and filed as #1312. It changes which lease the per-issue evidence view reports (and therefore ownerAgent and the derived state for such issues), which is a separate behavioural change from the scan ordering this PR was asked to fix; widening the blast radius of this PR to cover it would make the review harder without making the requested fix better. It is the reviewer's own recommendation to treat it as a follow-up.
Two taste-level notes from the same review, folded in
- The two runs now read concurrently, narrowing the window in which a lease renewed between them would fall into neither bucket and vanish from the scan.
- The overflow condition is now the merged pool's length (it holds every row both runs returned, so it is the exact signal); the per-run clauses were subsumed by it.
One note from the same review, not changed
The branch's migration directory 20261018000000_worker_ownership_capability shares its timestamp prefix with main's 20261018000000_pr_fix_recovery_disposition. Prisma orders migrations by the full directory name, so the order is deterministic, and the two migrations touch different tables (Lease vs PrFixQueueItem). Renaming a published migration path would churn the PR body and any local database that has already applied it, so it is left as is.
Local validation at fc1da6c (coordinator, integrated tree)
npm run typecheck— exit 0.npm run lint— exit 0; 0 errors, 1 pre-existing unrelated warning (src/app/login/page.tsx).npm test(full) — 195 files passed / 4 skipped; 4860 passed, 24 skipped, 1 todo, 0 failed.npx prisma validateon the merge commit — schema valid.
CI, the AI review, and the human review at this head are pending — not run by this lane, and not claimed green. No PostgreSQL is available here, so the migration, the transactions, and the two-run scan are exercised only through the vitest fake client.
|
Courier: already addressed (run courier-f245800ec64cf4e395223ae24dc4d3f5) PR #1300 (misospace/dispatch) already complete at head fc1da6c on run branch courier/misospace/dispatch/issue-1251: open, non-draft, pushed, branch up to date with origin/main. All 13 CI check runs at this SHA succeeded (Tests, Coverage, Build, Lint, Typecheck, Docker, Database integration/migrations, etc.). No open review threads; earlier findings were addressed and documented in the PR disposition comment at fc1da6c (follow-up #1312 filed). Coordinator re-verified locally: npm run typecheck exit 0; npm run lint 0 errors (1 pre-existing unrelated warning in src/app/login/page.tsx). Full test receipt reused from prior session at identical SHA (npm test 4860 passed / 0 failed), not invalidated — tree unchanged. |
Updated automatically by Dispatch on 2026-10-20T16:02:09.927Z |
joryirving
left a comment
There was a problem hiding this comment.
REQUEST CHANGES on exact head fc1da6cbfc0748a9e0382c59888bb9b3c238e3c4. The earlier token-rotation, transactional-disposition, and bounded-scan-ordering findings appear fixed, and the five reported workflows pass. I found two remaining correctness issues in the new ownership contract:
-
Major — ownership renewal does not keep
AgentWorkalive for the existing stale-work sweeper.sweepStaleWorkmarks a row stale when eitherlastHeartbeatAtorleaseExpiresAtis older than the cutoff. This renewal updates onlylastHeartbeatAt, leavingAgentWork.leaseExpiresAtat the old five-minute deadline. Once that deadline is old enough, the sweep still marks the freshly renewed worker STALE and deletes its Lease. That defeats the long-running/no-progress renewal capability. Update the AgentWork expiry to the renewed ownership expiry (or otherwise reconcile the stale-work predicate), and add a regression that runs the stale-work eligibility check after a capability renewal. -
Major — the conflict model can hide a contradictory active AgentWork owner. With label
agent/alice, lease owneralice, and active AgentWork ownerbob, the lease matches the label, and the AgentWork mismatch check is skipped solely because a lease exists. The result can beCLAIMED_ACTIVEinstead ofCONFLICTED, contrary to the design's requirement to surface an AgentWork owner that differs from the label owner. Compare active AgentWork against the sole label owner independently of whether a lease exists, and add this exact regression.
The known per-issue lease-selection mismatch tracked by #1312 is still present, but I have not made it a blocker in this review because it already has a separate follow-up issue.
| where: { agentName: input.agentName, issueId: input.issueId, state: { in: ["CLAIMED", "IN_PROGRESS", "BLOCKED"] } }, | ||
| }); | ||
| if (!work) return false; | ||
| await prisma.agentWork.update({ where: { id: work.id }, data: { lastHeartbeatAt: new Date() } }); |
There was a problem hiding this comment.
This only refreshes lastHeartbeatAt. The existing sweepStaleWork predicate is lastHeartbeatAt < cutoff OR leaseExpiresAt < cutoff; it can still mark this AgentWork STALE and delete the Lease once the unchanged leaseExpiresAt passes the five-minute threshold, even though this capability just renewed the Lease for 30 minutes. Please propagate the renewed expiry to AgentWork.leaseExpiresAt (or change the stale-work contract so a fresh capability renewal protects it) and add a regression proving the stale-work sweep skips a recently renewed long-running worker.
There was a problem hiding this comment.
Fixed in aa78b94 (plus the c1f8b76 follow-up).
The fix. renewOwnership now propagates the renewed ownership expiry onto the matching active AgentWork row, so the renewal's write neutralizes both arms of the sweeper's predicate:
src/lib/worker-ownership.ts—heartbeatMatchingAgentWorkwrites{ lastHeartbeatAt: now, leaseExpiresAt: expiresAt }.nowis the operation's own clock (input.now ?? new Date()) andexpiresAtis the same value written toLease.expiredAt(now + DISPATCH_OWNERSHIP_RENEW_TTL_MS), so the AgentWork expiry can never drift from the lease expiry. It is no longer a secondnew Date().- The lease CAS, the route, the disposition path,
sweepStaleWorkitself and the AgentWork start/checkpoint TTLs are unchanged — the sweeper's predicate is untouched; the renewed worker simply stops matching it.
The regression (src/app/api/agent-work/ownership/route.test.ts) exercises the sweeper's real candidate selection rather than a hand-copied predicate: the test evaluates the WHERE clause object sweepStaleWork actually passes against the AgentWork row the renewal actually wrote, and asserts the sweep finds nothing (examined/released/skipped = 0). A negative control pins that the same clause does select the pre-fix write shape (old leaseExpiresAt, old heartbeat), so "examined === 0" is not vacuous. A second test asserts the write itself carries the renewed expiry and the operation clock.
Mutation check: reverting the leaseExpiresAt propagation fails 3 of the route tests and passes the other 40.
One thing this review also surfaced, now fixed in c1f8b76: the renewal's AgentWork match had no orderBy, so it could refresh a sibling active row (a BLOCKED row survives startAgentWork's release) while the row the sweeper would reclaim kept a lapsed expiry. It now refreshes the freshest active row (orderBy: { lastHeartbeatAt: "desc" }, the same pattern as src/lib/ownership-inspect.ts).
Two related hazards are deliberately not in this PR, filed as follow-ups instead:
- #1315 — the renewal's AgentWork write is outside the lease CAS, so a crash between them loses the renewal.
- #1317 — the sweeper deletes the lease for the whole
(agentName, issueId)pair when any sibling row is stale. - #1316 — a
blockeddisposition leaves its settled AgentWork row with a lapsed expiry (same family, on the disposition path).
docs/worker-ownership-capability.md and docs/ownership-reconciliation.md §3.3 now state that a renewal refreshes the AgentWork expiry and why.
| if (lease !== null && lease.agentName !== labelOwner) { | ||
| conflictReasons.push("lease_owner_mismatch"); | ||
| } | ||
| if (lease === null && activeAgentWork !== null && activeAgentWork.agentName !== labelOwner) { |
There was a problem hiding this comment.
This suppresses agent_work_owner_mismatch whenever a lease exists. Counterexample: labels contain agent/alice, the lease owner is alice, but the active AgentWork row belongs to bob. The lease check passes, this branch is skipped, and the model can report CLAIMED_ACTIVE despite contradictory active ownership evidence. The design explicitly calls for a conflict when the active AgentWork owner differs from the label owner; check that independently of lease presence and add this regression.
There was a problem hiding this comment.
Fixed in aa78b94 (plus the c1f8b76 follow-up).
The fix. src/lib/ownership-state.ts — the agent_work_owner_mismatch check no longer requires lease === null. The active AgentWork owner is now compared against the sole agent/* label owner independently of whether a lease exists, so your counterexample (label agent/alice, lease owner alice, active AgentWork owner bob) derives CONFLICTED with reason agent_work_owner_mismatch instead of CLAIMED_ACTIVE.
Nothing else in the conflict model moved: the comparison still applies only when there is exactly one distinct agent/* label owner (0 or >1 owners change nothing), non-active AgentWork states (DONE/RELEASED/STALE) still do not count, the reason-code order is unchanged (lease_owner_mismatch before agent_work_owner_mismatch), and ownerAgent still resolves to the sole label owner. The blast radius is the two maintainer-only inspect surfaces — nothing in the queue, gate, release or sweep paths consumes deriveOwnershipState, so the extra conflict is surfaced, never acted on automatically.
The regressions (src/lib/ownership-state.test.ts): your exact case (label alice + lease alice + active AgentWork bob → CONFLICTED, ["agent_work_owner_mismatch"]), a control where the AgentWork owner agrees with the label (not conflicted), and a case where both the lease and the AgentWork disagree (["lease_owner_mismatch", "agent_work_owner_mismatch"], order preserved).
One gap this review surfaced, fixed in c1f8b76: none of those cases pinned that the comparison is against the LABEL owner — an implementation of activeAgentWork.agentName !== lease?.agentName would have passed every one of them. Added the case that pins it (label agent/alice, lease carol, active AgentWork alice → ["lease_owner_mismatch"] only). Mutation check: swapping the comparison to the lease owner fails that test (1 failed / 61 passed); restoring the old lease === null && guard fails 2 of the state tests (2 failed / 59 passed).
The per-issue selectLease ordering you mention is still tracked separately in #1312 and is deliberately not part of this PR — this change does not alter which lease the per-issue evidence view reports.
…licts (#1251) Two MAJOR findings from the human review at fc1da6c: 1. A capability renewal refreshed only AgentWork.lastHeartbeatAt, leaving AgentWork.leaseExpiresAt at the 5-minute execution expiry. The stale-work sweeper reclaims on `lastHeartbeatAt` OR `leaseExpiresAt`, so a renewed long-running worker was still marked STALE and its Lease deleted. The renewal now propagates the renewed ownership expiry onto the matching active AgentWork row, using the operation's own clock and the same expiry the Lease received. 2. The conflict model skipped the active-AgentWork-vs-label-owner comparison whenever a lease existed, so a lease owned by the label owner could hide an active AgentWork row owned by somebody else and report CLAIMED_ACTIVE. The comparison is now independent of lease presence (design §4 CONFLICTED). Regressions: the renewal's AgentWork write is asserted, the sweeper's real candidate clause is evaluated against the row the renewal wrote (plus a negative control on the pre-fix shape), and the contradictory-owner case is pinned in the derived-state model.
…conflicts (#1251) Follow-up to the two MAJOR fixes, from the adversarial review of that diff: - The renewal's AgentWork match had no `orderBy`, so it could refresh a sibling active row (a BLOCKED row survives `startAgentWork`'s release) while the row the sweeper would reclaim kept a lapsed expiry. It now refreshes the freshest active row. - The ownership comparison in the derived-state model was not pinned against the LABEL owner: comparing it against the lease owner would pass every test. Added the case (label alice, lease carol, AgentWork alice) that pins it. - The stale-work clause evaluator in the route test threw only for unsupported operators, so an unimplemented clause key could read as "no candidates" instead of failing loudly. It now throws for either. - docs/ownership-reconciliation.md §3.3 now states that a capability renew also refreshes the AgentWork expiry, so the design doc matches the code.
|
Courier: the two MAJOR findings from the review at What changed1. A renewal now keeps the matching 2. The conflict model no longer hides a contradictory active 3. Follow-up on the fix ( Local validation (coordinator, against the integrated tree)
Mutation checks proving the new regressions bite: reverting the renewal's Limitations: no PostgreSQL in this lane, so Deliberately not in this PR (follow-ups filed)
Remaining external verification (operator)
|
|
Courier: already addressed (run courier-0d9c991971830b4fb14943defa33f520) PR #1300 (misospace/dispatch) already complete at head c1f8b76 on run branch courier/misospace/dispatch/issue-1251: open, non-draft, pushed, branch up to date with origin/main. All 13 CI check runs at head are success (Actions run 39371704596/39371704731/39371704738/39371704624/39371704668), its-saffron[bot] APPROVED at c1f8b76. Both review threads from the human CHANGES_REQUESTED at fc1da6c are addressed in aa78b94+c1f8b76 with per-thread replies (renewal propagates leaseExpiresAt to AgentWork + sweeper regression; agent_work_owner_mismatch no longer gated on lease==null + label-owner regression, mutation-checked). Local validation this run on clean worktree at c1f8b76: npm run typecheck pass; npm run lint 0 errors (1 pre-existing warning, CI lint green); npm test ownership-state.test.ts + agent-work route tests = 132/132 pass (6 files). No new feedback beyond the two already-addressed threads. |
joryirving
left a comment
There was a problem hiding this comment.
REQUEST CHANGES on exact head c1f8b76. The two previous majors are now fixed and covered by regressions: renew mirrors the lease expiry into one AgentWork, and an active AgentWork with a mismatched owner triggers CONFLICTED even if the lease matches the label. I found one remaining closely related correctness hole in the actual multi-row stale-work interaction.
Blocker: renewing only the freshest active AgentWork leaves older active rows eligible to delete the same current Lease. heartbeatMatchingAgentWork explicitly acknowledges multiple active records can coexist for (agentName, issueId) (e.g. a prior BLOCKED row survives while a new IN_PROGRESS row starts), then calls findFirst(orderBy: lastHeartbeatAt desc) and updates just that row. sweepStaleWork independently scans every stale active record by heartbeat OR leaseExpiresAt and for any such record calls tx.lease.deleteMany({where:{agentName: work.agentName, issueId: work.issueId}}) with no claim-token/lease-expiry guard; it also releases GitHub's agent label. Therefore an older BLOCKED row whose heartbeat/expiry are past the cutoff can delete a recently renewed, valid same-agent/current-claim Lease, even though this PR claims renewal prevents that outcome. The current regression proves only the selected renewed row is no longer sweep-eligible; it doesn't include an older active sibling.
Please make the stale-work reconciliation claim-incarnation safe, or ensure all stale sibling active rows cannot release the current claim when the fresh active worker renews. Add an integration/regression with two active rows for the same agent+issue, an old stale BLOCKED row plus a freshly renewed IN_PROGRESS row and lease, then sweep: the current live lease/label must survive. Avoid simply heartbeating all old rows as if their work is still active without proving their ownership.
…patch/issue-1251 # Conflicts: # AGENTS.md # src/lib/issue-lifecycle.ts
…1251) The stale-work sweep reclaims a stale AgentWork row and then releases the claim of its (agent, issue) pair: it deletes the Lease and removes the `agent/<name>` label. But several active AgentWork rows can coexist for one pair — `startAgentWork` releases only the CLAIMED/IN_PROGRESS ones, so a BLOCKED row survives — and a renewed worker only refreshes the freshest of them. An older BLOCKED row that falls past the sweep cutoff therefore released the lease and the label of a freshly renewed, still-live worker. The sweep is now claim-incarnation safe: a row is released only when no live sibling row for the same (agent, issue) still owns the claim. "Live" is the sweeper's own staleness predicate inverted, spelled positively (`lastHeartbeatAt >= cutoff AND (leaseExpiresAt >= cutoff OR leaseExpiresAt IS NULL)`) so the nullable expiry cannot hide a live row behind SQL three-valued logic. A retained row is still reclaimed (marked STALE, retry marker cleared) but its lease, its label and its `claim_released` event are left alone; the sweep reports it as `claimRetained` and audits it as `stale_agentwork_claim_retained`. Co-Authored-By: opencode <opencode@opencode.ai>
…cs (#1251) Follow-up to 648c5b7. An adversarial review of the claim-incarnation guard found that its central claim — "live" is the sweeper's own staleness predicate inverted, spelled positively — was not pinned by any test, and that the docs described it wrongly. - Two new cases pin the AND spelling: a sibling that is fresh by heartbeat but whose expiry lapsed is NOT live, and neither is one that is fresh by expiry but whose heartbeat lapsed. Both are stale under the sweeper's own clause, so neither may shield the claim. - A new case pins that an already reclaimed STALE sibling is not live: counting it as live would withhold a pending release forever and strand the claim. - The primary case now asserts the reclaimed row's retry marker is down (the first-reclaim path), that the live sibling is untouched by the reclaim, and that the live sibling is not a reclaim candidate at all. - `docs/worker-ownership-capability.md`: the sweep still RECLAIMS the stale row in both cases — only the release is withheld. The wording said the opposite, and contradicted the next sentence. Mutation check on the new cases (each reverted independently, then restored): dropping the heartbeat arm fails 1 test, dropping the expiry arm fails 2, counting STALE as live fails 1, keeping the retry marker set on the first reclaim fails 1. Co-Authored-By: opencode <opencode@opencode.ai>
|
Courier: the remaining BLOCKER from the review at First, the branch was mid-merge when this run started. The merge with The blocker
Fixed in The fix — the sweep is now claim-incarnation safe
The positive spelling is load-bearing: The lookup runs inside the same
The guard applies on the retry path too: a row already marked When no live sibling exists, the behaviour is unchanged — same mark, same No heartbeat was faked. The regression
The fake evaluates the production WHERE clauses faithfully — it applies the Also pinned: the negative control (the same stale row with no live sibling still deletes the lease, removes the label and records exactly one Mutation checks (each applied to the working tree, run, then reverted)
Local checks (coordinator, against the integrated tree)
Independent adversarial review of this round's diffA fresh review session (no author context) on What is deliberately not closed here (pre-existing, unchanged by this PR)
Overlap note: this PR and PR #1319 now both touch Pending (not claimed green)CI checks, the automated review and the human review at |
|
CI observation on head 12 of the 13 check runs completed with conclusion This is a point-in-time observation of the head the operator will look at, not a guarantee — and the automated and human reviews of this head are still outstanding. Nothing here claims they are green. |
Superseded by a newer automated review for this pull request.
joryirving
left a comment
There was a problem hiding this comment.
REQUEST CHANGES on latest head c7f2116. The two original findings (renewed AgentWork.leaseExpiresAt; active-work-vs-label owner conflicts) remain fixed, and the previous stale-BLOCKED + fresh-IN_PROGRESS sibling case now correctly retains both the current lease and GitHub agent label with coverage. All five reported workflows pass. I independently verified two outstanding correctness gaps:
1. Blocker: the live-sibling check is a TOCTOU guard, not an atomic fence against claim/renewal. In src/lib/stale-work.ts, findLiveSiblingWork(tx, ..., cutoff) is a normal findFirst under the sweep's transaction. The same transaction then marks the old work STALE and executes lease.deleteMany({ where: { agentName, issueId } }), followed by releaseIssueClaim outside the transaction. renewOwnership / re-claim take IssueMutationGuard, but the sweeper does not. A sibling that is stale when the lookup reads it can be renewed and have its lease extended after that lookup but before the unconditional pair-based delete, at which point the sweep deletes the newly renewed claim and subsequently removes its GitHub label. A new claim/sibling created in the same window has the same problem. READ COMMITTED alone does not serialize the negative sibling read with those writers. Please put the sweep's decision and lease/label cleanup under the same issue-scoped mutation fence used by claim/ownership renewal, and/or use a claim-token/epoch CAS with a safe protocol for the external label write. Add a deterministic interleaving test with renewal between sibling lookup and delete, asserting lease, label, and live work all survive. A static two-row fixture is not enough.
2. Major: terminal disposition can settle the wrong active AgentWork row. dispositionOwnership at src/lib/worker-ownership.ts:543 uses an unordered tx.agentWork.findFirst by agent+issue+active states, then terminalizes only that row. Multiple active rows are explicitly supported/known (older BLOCKED survives startAgentWork while current IN_PROGRESS exists). If findFirst picks the old BLOCKED row for a completed/released disposition, the current IN_PROGRESS row remains active even though the lease now has a terminal disposition and replay returns success without completing it. Correlate the disposition to the current claim/work incarnation rather than picking an arbitrary active sibling, and add a regression with both rows in the same issue. Avoid blindly terminalizing unrelated work.
The automated review raised the same two issues, but these findings are grounded in the current implementation and caller/sweeper transaction boundaries. I would not merge until they are fixed.
| // Releasing the lease in the same transaction as the stale marker | ||
| // keeps the database-side queue blockers consistent across retries. | ||
| if (work.issueId) { | ||
| await tx.lease.deleteMany({ |
There was a problem hiding this comment.
The preceding findLiveSiblingWork is only a non-locking snapshot check; claim/renewal uses IssueMutationGuard, but this sweep does not. A renewal/new claim can commit after the sibling query and before this pair-wide delete, so this statement can remove a newly renewed lease; then the external releaseIssueClaim removes its shared GitHub label. Make the check-to-delete and external release claim-epoch fenced/serialized, and test that exact interleaving.
There was a problem hiding this comment.
Fixed in 9a76a65, on both axes you named.
Serialized fence. The sweep's decision and its lease/label cleanup now run inside the per-issue IssueMutationGuard (operation: "stale-release", same anchor claim/lease/ownership/capacity-release take): sibling lookup → stale mark → lease delete → external releaseIssueClaim → finalize all happen while the fence is held, so no renewal/re-claim/fresh-claim writer can commit "between the check and the delete" — they get the retryable 409 mutation_busy and re-decide against post-sweep state. Busy skips the candidate for this sweep (nothing is owed outside a completed fenced window). The guard gained an exact stale-release resume branch (same operation + same agentName, non-executing only) so a mid-way failure is resumed by the next sweep; a crash-executing guard stays fail-closed like a crashed claim guard. Trailing audits are non-fatal so a failed audit can't strand the fence behind a finalized row with no resumable candidate.
Epoch CAS for the delete itself. The pair delete is a compare-and-delete against a snapshot of {claimToken, expiredAt, lastRenewalObservedAt} read before the sibling lookup — renew moves expiredAt+lastRenewalObservedAt, re-claim moves one or rotates claimToken — plus a post-window existence recheck (with no snapshot row, no delete is attempted, so a fresh claim inside the window can only be caught, never consumed). Mismatch ⇒ back off: row still STALE, lease/label/claim survive, claimRetained with audit reason "claim epoch moved". A terminal disposition moves none of those fields, so disposed claims are still swept (pinned).
Deterministic interleaving tests (not static fixtures): a scripted renewal commits exactly between the lookup and the delete — fenced delete matches nothing, and the test asserts the lease keeps its renewed expiry, the agent/* label survives, no claim_released event is written and the live work survives; a fresh claim created inside the window is neither deleted nor released; and with the sweep holding the fence, a renewal routed through the real withIssueMutationGuard entry point comes back busy with its write never executed. Negative controls pin the release path and the exact CAS clause. Mutation-checked: reverting to the unconditional pair delete reddens 4 tests; dropping the recheck reddens the fresh-claim test; removing the guard wrap reddens 3.
| // Settle a matching active AgentWork (if any) to its terminal state so | ||
| // the execution record agrees with the declared disposition. History is | ||
| // written by the same call; the lease row itself is retained as evidence. | ||
| const work = await tx.agentWork.findFirst({ |
There was a problem hiding this comment.
findFirst has no claim-epoch correlation or ordering. Older BLOCKED + newer IN_PROGRESS rows can coexist for the same (agentName, issueId); if this chooses the older row, a completed/released disposition commits terminal lease evidence while leaving the current IN_PROGRESS row active, and replay never fixes it. Identify the current claim's work before settling or fail closed on ambiguity; add a two-active-row regression.
There was a problem hiding this comment.
Fixed in 9a76a65.
dispositionOwnership no longer terminalizes an arbitrary active row. There is now one definition of the current claim's work — freshest lastHeartbeatAt, then createdAt desc, then id desc (CURRENT_CLAIM_WORK_ORDER_BY / findCurrentClaimWork in src/lib/worker-ownership.ts) — and every path that names "the row this claim owns" selects through it: the renewal heartbeat (previously freshest-by-heartbeat only, now the same deterministic order), the terminal disposition settlement, and the operator inspect evidence row (src/lib/ownership-inspect.ts imports the shared order). So your counterexample — older BLOCKED + current IN_PROGRESS — settles the IN_PROGRESS row, the BLOCKED sibling is left untouched, and replay can never report success over a stranded active row.
Regressions (src/app/api/agent-work/ownership/route.test.ts, fake upgraded to a multi-row pool with a faithful orderBy comparator that throws on unimplemented keys/directions): disposition settles the freshest row in both pool slots (a mirror test pins that selection is freshness, not pool position — under the old unordered findFirst it is red); tie tests pin createdAt desc then id desc; and a renew-side control pins that the row the renewal protects is the row a disposition settles. Mutation check: dropping the orderBy reddens 3 tests.
One adjacent producer was checked and deliberately not changed here: checkpointAgentWork still selects an unordered active row (pre-existing, no issue scoping) and could stamp an older sibling fresher than the current attempt — filed as #1328 so it gets its own scoped fix rather than widening this PR.
…laim's work (#1251 review) Review follow-ups on PR #1300 (head c7f2116): 1. Blocker — the sweeper's live-sibling check -> lease deleteMany -> external GitHub label release was a TOCTOU against claim/renewal. The whole decide-and-release pipeline for an issue-scoped claim now runs under the per-issue IssueMutationGuard fence (operation "stale-release", with an exact operation+agent resume branch), and the lease delete is a claim-epoch compare-and-delete against a snapshot of {claimToken, expiredAt, lastRenewalObservedAt} with a post-window existence recheck. Any renew/re-claim/fresh claim that wins the window makes the sweep back off: the row is still reclaimed but lease, label and claim are reported retained. Trailing audit writes are non-fatal so a failed audit never strands the fence behind a finalized row. 2. Major — dispositionOwnership terminalized an arbitrary active AgentWork row. Renewal heartbeat, terminal disposition and the operator inspect view now share one deterministic current-claim-work rule (freshest lastHeartbeatAt, then createdAt desc, then id desc). Deterministic interleaving tests (renewal and fresh-claim inside the window), fence serialization and busy-skip tests, guard resume-matrix tests, and two-active-row disposition regressions; mutation-checked. Docs: docs/worker-ownership-capability.md records the fence, the CAS, the fail-closed crashed-guard posture, and the current-claim-work rule. Follow-up filed: #1328 (checkpointAgentWork unordered active-row selection predates and relocates finding 2).
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: glm-5.3-flash (openai) — fallback (primary failed) · pr-reviewer-action v3.6.0
Findings (2 minor, 1 info)
| Severity | Location | Finding |
|---|---|---|
| Info | src/lib/stale-work.test.ts |
Confirm the deterministic interleaving test (renewal committing between sibling lookup and lease delete, asserting lease/label/live work survive) exists as the capability doc claims. |
| Minor | src/lib/ownership-state.ts |
This suppresses `agent_work_owner_mismatch` whenever a lease exists. Counterexample: labels contain `agent/alice`, the lease owner is `alice`, but the active AgentWork row belongs to `bob`. The lease check passes, this branch is skipped, and the model can report CLAIMED_ACTIVE despite contradictory |
| Minor | src/lib/worker-ownership.ts |
This only refreshes `lastHeartbeatAt`. The existing `sweepStaleWork` predicate is `lastHeartbeatAt < cutoff OR leaseExpiresAt < cutoff`; it can still mark this AgentWork STALE and delete the Lease once the unchanged `leaseExpiresAt` passes the five-minute threshold, even though this capability just |
Recommendation: approve. The migration is additive-only (13 nullable columns + one defaulted integer on Lease), the auth surface change is covered by an explicit tiering test matrix, and the outstanding human blocker (sweep TOCTOU) is addressed in the current head by the per-issue IssueMutationGuard fence plus claim-epoch CAS described in docs/worker-ownership-capability.md and implemented in the modified src/lib/stale-work.ts with regression coverage in src/lib/stale-work.test.ts.
Change-by-change: Migration adds only nullable/defaulted columns — no data loss. All four prior review threads are resolved at this head: the sweep now runs decide-and-release under the same mutation fence as claim/renewal with a {claimToken, expiredAt, lastRenewalObservedAt} compare-and-delete backstop; renewal propagates the renewed expiry to the current claim's AgentWork.leaseExpiresAt; current-claim work is selected by a deterministic freshness rule (heartbeat, createdAt, id) shared by renewal, disposition, and inspect; and agent_work_owner_mismatch is now evaluated independently of lease presence. The human change request's requested fence and interleaving protection are present in the diff.
Required checks: Migration data-loss review — satisfied (additive nullable columns only). Migration tested on production-like schema — satisfied (CI Database migrations and Database integration green against a Postgres service). Path traversal — not applicable (no path handling in this change; the path risk flag names no files). Edge-case path tests — not applicable for the same reason. Auth flow regression — satisfied (src/lib/auth.ts +4/-1; route tests cover 401, unbound-token 403, cross-agent bound-token 403, maintainer actor resolution). Session/claim token handling — satisfied (claim-token fencing: stale_claim_token 409, CAS on {id, claimToken, disposition: null}, token rotation on re-claim; NextAuth session handling untouched).
Linked Issue Fit: Matches PR 1251's done-when items: fenced renewal, late-renewal tolerance, derived states with UNKNOWN-not-abandoned, identity boundaries, extensive tests including replay and permission boundaries; no release/requeue/rotation shipped, as required.
Unknowns or Needs Verification: The fence/CAS behavior is verified through the diff's documentation and test files rather than a line-by-line read of the full stale-work.ts body (diff truncated); the info finding below asks the author to confirm the interleaving test exists as described.
Unresolved Review Threads
PRRT_kwDOSZt6uM6uafNM: fixedPRRT_kwDOSZt6uM6uafNE: fixedPRRT_kwDOSZt6uM6uLvOt: open — fixed without evidence citing current codePRRT_kwDOSZt6uM6uLvOq: open — fixed without evidence citing current code
Outstanding Human Change Requests
- @joryirving's change request (c7f2116, head moved since) is not shown addressed at this head; it needs the reviewer's own re-review.
Superseded by a newer automated review for this pull request.
joryirving
left a comment
There was a problem hiding this comment.
Blocker-only re-review on exact head 9a76a6526160721458b814333c28a147d9851c85: APPROVE. The two current-head blockers are fixed. The stale-work decide-and-release path now runs under the same per-issue mutation fence as claim/ownership writers and backs that serialization with a claim-epoch compare-and-delete over claimToken, expiredAt, and lastRenewalObservedAt; the deterministic interleaving regressions cover renewal/fresh-claim races and preserve lease + label + claim. Terminal disposition now selects the current claim's active AgentWork via the shared deterministic freshness ordering instead of an arbitrary sibling. Exact-head Security Audit, Build Dispatch Image, CI, PR Smoke, and AI PR Review all pass. Remaining minor/follow-up concerns are dogfoodable under the revised merge-biased policy.
Round at head
|
| Check | cwd | Revision | Result |
|---|---|---|---|
vitest run (full) |
/workspace | 9a76a65 |
199 files passed / 4 skipped; 5020 passed, 24 skipped, 1 todo, 0 failed |
npm run typecheck |
/workspace | 9a76a65 |
exit 0, no errors |
npm run lint |
/workspace | 9a76a65 |
0 errors; 1 pre-existing unrelated warning (src/app/login/page.tsx) |
Mutation receipts: unconditional pair delete restored → 4 tests red; guard wrap removed → 3 red; orderBy removed from findCurrentClaimWork → 3 red; existence recheck removed → fresh-claim-in-window test red. Limitation unchanged: no PostgreSQL in the lane — transaction/guard behaviour is exercised through the faithful fakes, not a real DB.
CI / AI review / human review at 9a76a65 are pending — not claimed green; the operator verifies after this run exits.
|
Postscript: this PR was squash-merged as |
…ite atomically (#1315) (#1336) * fix(ownership): renew commits its lease CAS and AgentWork liveness write atomically (#1315) renewOwnership wrote the fenced lease TTL extension and the matching AgentWork liveness write (heartbeat + renewed expiry + history) as two separate commits. A crash between them left the lease renewed while the AgentWork row kept its old lapsed 5-minute leaseExpiresAt; the stale-work sweeper reclaims on leaseExpiresAt too, so it marked the renewed worker STALE and deleted its lease — the renewal was lost. Since PR #1300 that write is load-bearing for the worker's survival, not best-effort evidence. Wrap the CAS (kept as the transaction's first statement so the lost-race early return commits nothing), the row re-read and the AgentWork write in one prisma.$transaction on the transaction's client. A failure rolls the whole renewal back; the route 500s and never reports success. Fencing (claimToken + disposition: null CAS), replay semantics (a retry just re-extends) and agentWorkHeartbeated's truthful absence reporting are unchanged. * docs(ownership): split the atomicity rationale between replay (register/disposition) and sweeper (renew) Review follow-up on #1315: the shared closing sentence read as if the replay-guard rationale justified renew's transaction too. Renew has no replay branch; its constraint is that a committed lease CAS must never lose its liveness write. --------- Co-authored-by: Courier <courier@localhost>
Summary
Implements the accepted optional worker ownership capability from
docs/ownership-reconciliation.md§6 and the §4 derived ownership read model with §9 operator inspect surfaces. Built entirely on the existingLease/AgentWork/lifecycle machinery — no parallel lease table, no new lock. No claim release, no label cleanup, no requeue, no credential rotation (those are #1254, still design-only).Closes #1251.
What lands
POST /api/agent-work/ownership(operation: register | renew | disposition), worker-tier and identity-scoped (src/lib/worker-ownership.ts):Lease.claimToken(design §6.1: the attempt-token discipline generalizesclaimToken— no new column needed). Stale/incarnation-mismatched renewals and dispositions are rejected (409 stale_claim_token); pre-feat(recovery): add fenced capacity-exhaustion handoff #1256 token-less leases fail closed; a terminal disposition is never resurrected (CASwhere {claimToken, disposition: null}).409 no_active_claim; after a terminal disposition,409 disposed.occurredAt(invalid/future/>24h silently falls back to server clock, same posture astasks/report'sstartedAt) recorded alongside the server time in both the lease columns and the lifecycle events.register→registeredAt/registrationOccurredAt/runRef+ one dedupedownership_registeredlifecycle event + audit;renew→ TTL extension,renewCount, renewal times, and a full refresh of the matching activeAgentWorkrow — heartbeat and expiry, so the stale-work sweep (which reclaims on the expiry as well as on the heartbeat) cannot reclaim a renewed long-running worker — plus itsAgentWorkHistoryrow (external-wait marker set/cleared);disposition→ terminal lease columns +AgentWorkterminal state + dedupedownership_dispositionedevent + audit.retryAfteris accepted only withblockeddispositions.register/same-kinddispositionare replay-idempotent per (issue, agent, token); a different disposition after a terminal one is409 disposition_conflict.registeranddispositioncommit their evidence in oneprisma.$transaction(CAS + AgentWork settlement + history + lifecycle event + audit row), so a partial write cannot strand anIN_PROGRESSAgentWorknext to a terminal lease or lose the required lifecycle event behind a replay that reports success.Leasecolumns + migration20261018000000_worker_ownership_capability. Clients that never adopt the capability leave them unset and stay conservatively unverified (§6.2).src/lib/ownership-state.ts, pure):UNOWNED / CLAIMED_ACTIVE / CLAIMED_LONG_RUNNING / REVIEW_OWNED / EXTERNAL_WAIT / HELD / ABANDONED_CANDIDATE / CONFLICTED / UNKNOWNwith machine-readable reason codes. Conflicting evidence →CONFLICTED(the lease owner and the activeAgentWorkowner are each compared against the soleagent/*label owner independently, so a lease that agrees with the label cannot excuse an activeAgentWorkrow owned by somebody else); a label-only claim with no Dispatch evidence →UNKNOWN(neverABANDONED);HELD/REVIEW_OWNEDshort-circuit before any staleness ladder. Windows are configuration:DISPATCH_OWNERSHIP_ACTIVE_WINDOW_MS(15m),DISPATCH_OWNERSHIP_LONG_RUN_GRACE_MS(45m),DISPATCH_OWNERSHIP_RENEW_TTL_MS(30m).GET /api/issues/[issueId]/ownership(live forge label read, fail-soft tolabelSource: "unavailable", never a 5xx;?live=falsefor cache) andGET /api/issues/ownership(bounded cached scan,?state=,limit≤200,truncated+ per-sourcesourcesTruncated). Raw claim tokens are never surfaced — onlyclaimTokenPresent.src/app/api/issues/ownership/route.ts): a lease's activity is its last renewal, falling back to its claim time when it was never renewed. Prisma'sorderBycannot express that combined timestamp, so the lease source is read as two runs (renewed leases byrenewedAt, never-renewed leases bycreatedAt) and merged on the same key, withidascending as the deterministic tie-break in both the query and the merge. A long-lived claim that its owner is still renewing now outranks a newer claim that was never renewed, so the scan cap cannot drop a live owner while keeping a stale one.src/lib/stale-work.ts): the sweep releases the claim of a staleAgentWorkrow only when no live sibling row for the same(agent, issue)still owns it. Several active rows can coexist for one pair (startAgentWorkreleases only theCLAIMED/IN_PROGRESSones, so aBLOCKEDrow survives a re-claim) and a renewal refreshes only the freshest of them — so reclaiming an older stale row used to delete the renewed claim'sLeaseand strip itsagent/*label. "Live" is the sweeper's own staleness predicate inverted, spelled positively (lastHeartbeatAt >= cutoff AND (leaseExpiresAt >= cutoff OR leaseExpiresAt IS NULL)) becauseleaseExpiresAtis nullable and SQL three-valued logic would otherwise hide a fresh-heartbeat/NULL-expiry row behind a negated form. A retained row is still reclaimed (markedSTALE, retry marker cleared, so it stops asking) but its lease, its label and itsclaim_releasedevent are left alone; the sweep reports it asclaimRetainedand audits it asstale_agentwork_claim_retained.docs/worker-ownership-capability.md, design-doc status updated to "partially implemented (§6/§4/§9 landed; §8 reconciliation still design-only)", AGENTS.md env table + subsystem row, README link.origin/main(merge commit892fe84, this round): the conflicts wereAGENTS.mdandsrc/lib/issue-lifecycle.ts, both purely additive — main added the feat(lifecycle): link authoritative PR merges and expose claim-to-merge timelines #1203 lifecycle-backfill surface and the#1262durable-replay row to the subsystem map, this branch added the feat(ownership): persist optional worker renewal and fenced claim evidence #1251 worker-ownership row and theapi:ownershiplifecycle source. Resolved by keeping both sides of each (theLIFECYCLE_SOURCESarray is a plain TS union over aStringcolumn, so the two sources coexist). The earlier merge (merge commit701dffb): the only conflict wasprisma/schema.prisma— this branch adds 13 nullableLeasecolumns, main reformatted the whole schema. Resolved as main's formatting plus the 13 columns;npx prisma formatis a no-op on the result andnpx prisma validatepasses. Main's20261018000000_pr_fix_recovery_dispositionmigration touchesPrFixQueueItem, this branch's touchesLease; both apply in lexicographic order.Forge access: the capability operations consult no forge surface — fencing is entirely against the claim receipt and the lease row — so they behave deterministically during forge outages and write nothing outside Postgres. The inspect surface reads labels only through the configured forge capability (
claim-gate.getLiveIssueLabels), never a CLI.Invariants this change must keep — and every other code path enforcing the same rule
Lease, writes anagent/*label, requeues, or rotates credentials in this issue. Enforced by: this diff contains zero GitHub calls and zero lease deletions (tests assert lease retention and audit/lifecycle-only effects). Other writers of the same state are untouched:POST /api/issues/unclaim+releaseIssueClaim(only path that removes labels),agent-workoperator release/reassign, capacity-handoff release. Thestale-worksweep is the one exception, and only to make it safer: it no longer releases a claim that a live siblingAgentWorkrow still owns (see What lands). It still releases every claim it released before, and it never releases one it did not.issue-capacity-handoff.ts(lease.claimToken !== claimToken → stale_claim_token) and bylease.tstoken mint/rotation (feat(recovery): add fenced capacity-exhaustion handoff #1256). Kept: ownership renewal preserves the token (a heartbeating worker keeps its receipt), CAS pins{id, claimToken, disposition: null}, and feat(recovery): add fenced capacity-exhaustion handoff #1256's fence semantics are unchanged (capacity-handoff never reads the new columns; renewing cannot forge or dodge a capacity release). Reactivating a disposed row is the one exception: it is a new attempt, so it rotatesclaimTokenatomically with the terminal-field clear and resets the retired attempt's provenance — otherwise the retired incarnation's receipt would validate against the new attempt. Verified end to end: retired token →409 stale_claim_token, new token succeeds.agent/courier*wildcards; the unbound legacy token carries no identity. Enforced centrally byworker-identity.ts::enforceWorkerAgentScope(same call claim/unclaim/ack use); the capability route calls it before any lease lookup. Tests: bound-beta-for-alpha→ 403 with zero writes; unbound worker token → 403; bodyagentNamecannot spoof a bound worker.WORKER_ALLOWLISTinauth.tsis the single table. Kept: exactly one new anchored entry (POST /api/agent-work/(start|checkpoint|finish|ownership)); both GET ownership surfaces deliberately absent → worker 403 (tested).IssueLifecycleEventremains the single claim/ownership history stream (feat(lifecycle): persist idempotent claim and status transition evidence #1202). Other writers (claim,unclaim,move,status,groom,stale-recovery,sync,webhook,agent-report) unchanged; this change only adds taxonomy entries (ownership_registered,ownership_dispositioned, sourceapi:ownership) and writes throughrecordLifecycleEventwith dedupe keys — no duplicate claim history, no parallel table.IssueMutationGuardanchor (feat(recovery): add fenced capacity-exhaustion handoffs #1259). The capability ops take the same guard (operation: "ownership"). Because the guard has no resume branch forownershipand is never time-expired, the guard's failure path deletes a freshly created ownership guard on throw (same posture as"lease", both local/idempotent with no partial remote effect) — otherwise a single transient failure would permanently 409-lock claim/lease/ownership on that issue.claim/capacity-releaseresume branches are byte-for-byte unchanged.lease.ts).upsertLeaseclears a retired attempt's terminal evidence on re-claim and — when the row was disposed — rotates the claim token and resets registration/renewal provenance, so the fresh attempt neither inherits the retired one's evidence nor leaves its owner unfenced. Callers: claim route (intended), groomer (no-op — its leases never carry dispositions). Terminal evidence for the retired attempt lives in the lifecycle stream.UNKNOWN, label-only legacy claims are surfaced, not abandoned), and nothing in this diff acts onABANDONED_CANDIDATE.resolveActiveWork) unchanged; the new operator reads expose onlyclaimTokenPresent(a test asserts the raw token string never appears in a response body); audit notes carry only bounded runRef/disposition metadata.registeranddispositionare not renewals and deliberately leaverenewedAtalone. The scan reads the lease source as two runs that are each sorted by their own activity key and merges them on that key, withidascending as the tie-break. Regression tests fail if either run's sort key, either run's tie-break, or the merge's tie-break changes, and the test double comparesDatecolumns by value so equal instants actually reach the tie-break clause.Settled decisions
Reuse
Lease.claimTokenas the attempt token rather than adding a second column (design §8.7: feat(recovery): add fenced capacity-exhaustion handoffs #1259 landed the anchor; §6.1 generalizes it).One capability route (
POST /api/agent-work/ownership,operationfield) instead of three directories; semantics live in the lib.Renewals are NOT lifecycle events (design maps renew → lease TTL +
AgentWorkHistoryheartbeat); register/disposition ARE (deduped per claim token).Late renewal (lapsed row, matching token) is accepted; that IS the transient-outage/restart story. Re-claim of a released/pruned claim is a fresh attempt with a fresh token.
Reactivating a disposed row rotates the claim token and resets provenance (new attempt), while an ordinary renewal of a live row keeps token and evidence (same attempt).
Operations that assert a new fact (
register,disposition) commit their evidence in one transaction;renewdeliberately does not — its TTL extension is safely retryable and itsAgentWorkrefresh is best-effort evidence whose absence it reports truthfully (agentWorkHeartbeated: false). The crash window between the committed lease renewal and that refresh is tracked as #1315.External wait markers ride the lease (
retryAfter/waitReason) via renew orblockeddisposition; the §4 storage note's "no home for EXTERNAL_WAIT" gap closes here.Windows are env config with clamps, per design §8.3; no reconciliation behavior added (that's feat(ownership): reconcile abandoned claims with fenced audit-first release #1254).
Scan freshness is the row's most recent activity, not its creation time. Prisma's
orderBycannot expressCOALESCE(renewedAt, createdAt), so the lease source is read as two pre-sorted runs and merged — the bounded-merge alternative the review named explicitly. The merge compares heads only (it never re-sorts), so the query'sorderBystays the single source of ordering truth and the id tie-break is identical on both sides of the merge.A renewal refreshes the matching active
AgentWorkrow in full: heartbeat and expiry. The sweeper reclaims onlastHeartbeatAtORleaseExpiresAt, so a renewal that moved only the heartbeat would still lose a renewed long-running worker. The refreshed expiry is the renewal's own expiry — the same value written toLease.expiredAt, never a secondnew Date().The renewal refreshes the FRESHEST active
AgentWorkrow for that (agent, issue). Several active rows are reachable (startAgentWorkreleases onlyCLAIMED/IN_PROGRESS, so aBLOCKEDrow survives), and the sweeper reclaims whichever row fell past its cutoff.The active-
AgentWork-vs-label-owner conflict comparison is independent of lease presence: a lease that agrees with the soleagent/*label owner does not excuse an activeAgentWorkrow owned by somebody else (design §4CONFLICTED,docs/ownership-reconciliation.md:214).The stale-work sweep releases a stale row's claim only when no live sibling row for the same (agent, issue) still owns it; a retained row is still reclaimed, but its lease, its label and its
claim_releasedevent are left alone.Local validation receipts (coordinator; each run once per change state against the integrated tree)
npm run typecheckfc1da6cnpm run lintfc1da6csrc/app/login/page.tsx)npm test(full)fc1da6cnpm run typecheck/npm run lint/npm test(full)701dffb(merge)npx prisma validate701dffbschema at prisma/schema.prisma is validnpm run typecheck/npm run lint/npm test(full)335f1e6(previous head)npm run typecheck/npm run lint/npm test(full)aa78b94npm run typecheck/npm run lint/npm test(full)c1f8b76npm run typecheck/npm run lint/npm test(full)892fe84(merge oforigin/main)npm run typecheck/npm run lint/npm test(full)648c5b7npm run typecheck/npm run lint/npm test(full)c7f2116(head)Mutation checks proving the new regressions bite (run by the coordinator, reported here):
This round (
src/lib/stale-work.test.ts, 13 tests; each mutation applied to the working tree, run, and reverted):Neutering the live-sibling lookup entirely: 3 failed / 10 passed — the renewed-sibling case, the NULL-expiry case and the retry-path case.
Dropping the
lastHeartbeatAt >= cutoffarm of the live predicate: 1 failed / 12 passed — "needs BOTH arms of the live predicate: a fresh expiry alone is not live".Dropping the
leaseExpiresAt >= cutoffarm: 2 failed / 11 passed.Counting an already reclaimed
STALEsibling as live: 1 failed / 12 passed — "does not treat an already reclaimed STALE sibling as live".Leaving the retry marker set on the first-reclaim path: 1 failed / 12 passed — the renewed-sibling case.
Keeping the pending marker on the retry path: 1 failed.
Retained path still deleting the lease: 7 failed; still calling
releaseIssueClaim: 2 failed.released/skippedcounter contamination: 1 / 3 failed.Restoring the pre-fix single-query ordering (
createdAtfirst): 2 failed / 25 passed — the two scan-order regressions.Test double sorting
ascasdesc: 1 failed / 26 passed — the id tie-break test.Never-renewed run ordered by
renewedAtinstead ofcreatedAt: 1 failed / 28 passed.Dropping the
idtie-break from the never-renewed run: 1 failed / 28 passed.Dropping the
idtie-break from the renewed run: 1 failed / 28 passed.Renewed run ordered by
createdAt, or byrenewedAtascending: 1 failed / 29 passed each.Reversing the merge's id tie-break: 1 failed / 29 passed.
Reverting the renewal's
leaseExpiresAtpropagation (src/lib/worker-ownership.ts): 3 failed / 40 passed in the ownership route test.Restoring the
lease === null &&guard on the AgentWork/label comparison (src/lib/ownership-state.ts): 2 failed / 59 passed in the ownership-state test.Comparing the active
AgentWorkagainst the LEASE owner instead of the LABEL owner: 1 failed / 61 passed in the ownership-state test.Mutation checks from the earlier rounds (run by the fix workers, reported here):
tx.*→prisma.*inside the disposition transaction: 5 failed / 35 passed (lease CAS survived rollback; lifecycle written on the root client).renewedAt-first lease ordering: 2 failed / 23 passed.lease.test.tsrotation test fails against the pre-fixlease.ts(1 failed / 34 passed).Limitations: no PostgreSQL available in the lane, so
$transactionrollback, guard serialization, the newORDER BYSQL, and the two-run lease scan are exercised only through the vitest fake client — there is no real-DB evidence. The migration SQL was likewise not executed here; it is 13 additiveALTER TABLE ... ADD COLUMNstatements (nullable; oneNOT NULL DEFAULT 0) and matches the Prisma schema column-for-column (reviewer-verified); production applies it viaprisma migrate deployat container start. No GitHub sandbox run against a real issue. The sweeper's candidate selection is exercised without a database by evaluating the WHERE clause objectsweepStaleWorkactually passes against theAgentWorkrow the renewal actually wrote (plus a negative control on the pre-fix write shape) — the test does not re-implement the predicate.Review outcome
Internal adversarial review (no author context). Round 1 (head
b3080cb): no blockers; 2 MAJOR (guard never resumed/deleted on throw → permanent issue wedge; claim-pathupsertLeasereuse inheriting a terminal disposition), 3 MINOR (review-owned reason mislabel;retryAfterhonored on non-blockeddispositions; unbounded list scan) — all fixed in6c383d9with regression tests. Round 2 delta review (head6c383d9): all five resolved, no new MAJOR/BLOCKER; 3 further MINORs (terminal non-blockeddisposition not clearing a stale wait marker; candidate truncation order dropping strong signals first; no truncation signal/doc drift) — all fixed in8e18d74. Round 1 also independently verified: identity scoping with no spoof path, anchored allowlist entry, CAS fencing incl. null-token fail-closed, #1256 capacity-handoff unaffected, token non-leakage, migration↔schema parity, and zero release/reconciler behavior in the diff.PR review at
8e18d74(automated review + human review): 2 MAJOR + 2 MINOR — attempt fencing not re-established on re-claim of a disposed row, disposition not recoverable after a partial write,registerreplay not idempotent, operator scan not fully bounded. All fixed in829565d; see the PR comment for the per-finding reply.Review of the fix (
829565d): no blockers; 3 MINOR + 3 NITs — the atomicity tests could not fail for the reason they claimed (the$transactionmock handed back the root client and rolled back globally), the new lease ordering put never-renewed (i.e. newest) leases last, plus register-CAS symmetry, doc drift, and a transaction-return footnote. All fixed in335f1e6. One NIT accepted and not fixed: the new leaseORDER BYcannot use@@index([expiredAt]), so Postgres still sorts matching active leases server-side — the cap bounds transfer and derivation; bounding the sort needs an index change, out of scope here.PR review at
335f1e6(human + automated, both raised the same two findings): 1 MINOR — the bounded lease scan ordered bycreatedAtbeforerenewedAt, so an old lease renewed moments ago could be dropped behind newer stale leases when the cap overflowed — and 1 NIT — the list route's test double returned descending order for both itsascanddescbranches, so theid: "asc"tie-break was not actually tested. Both fixed ina2a4049(activity-ordered two-run scan + merge; faithful comparator).Independent adversarial review of this round's diff (
a2a4049, no author context): 4 MINOR — the test double's null-placement fallback was defeated by??(a string sort spec made itfalse, so NULLs were always last, contradicting the double's own documented Postgres behaviour); the never-renewed run'sorderBywas not covered by any test (swapping its sort key or dropping its tie-break left the suite green); the activity comment claimed every lease write stampsrenewedAt, which the register and disposition paths deliberately do not; and the per-issueselectLeaseordering insrc/lib/ownership-inspect.tsis the same class of defect in its narrowed multi-lease case. The first three are fixed infc1da6c(with the new regression cases, each of which now fails under the corresponding mutation); the fourth is filed as #1312 rather than changed here, because it alters which lease the per-issue evidence view reports. Two of that review's taste-level notes are also folded in: the two runs now read concurrently (narrowing the window in which a lease renewed between them would fall into neither bucket), and the overflow condition is the merged pool's length, which is the exact signal.PR review at
fc1da6c(human, 2 MAJOR): (1) the ownership renewal did not keepAgentWorkalive for the stale-work sweeper — it refreshed onlylastHeartbeatAt, leaving the 5-minuteleaseExpiresAtto pass the sweeper's cutoff, so a renewed long-running worker was still marked STALE and its Lease deleted; (2) the conflict model skipped the active-AgentWork-vs-label-owner comparison whenever a lease existed, so labelagent/alice+ leasealice+ activeAgentWorkbobreportedCLAIMED_ACTIVEinstead ofCONFLICTED. Both fixed inaa78b94: the renewal now propagates the renewed expiry onto the matching activeAgentWorkrow, and the comparison is independent of lease presence. Regressions run the sweeper's own candidate clause against the row the renewal wrote (plus a negative control on the pre-fix shape) and pin the counterexample.Adversarial review of that fix (
aa78b94, no author context): APPROVE — both findings confirmed closed, no false conflicts introduced, the PR's invariants intact (no release, no label write, no credential rotation, the claim token never surfaced), and the new regressions verified to fail under the corresponding mutations. Three in-scope items it raised are fixed inc1f8b76: the renewal'sAgentWorkmatch had noorderByand could refresh a sibling row; the ownership comparison was not pinned against the LABEL owner (an implementation comparing it against the lease owner would pass every test); and the route test's clause evaluator threw only for unsupported operators, so an unimplemented clause key could read as "no candidates". Its three out-of-scope findings are filed as follow-ups rather than changed here: #1315 (the renewal'sAgentWorkwrite is outside the lease CAS — a crash between them loses the renewal), #1316 (ablockeddisposition leaves its settledAgentWorkrow with a lapsed expiry, so the sweeper reclaims the parked lease), and #1317 (the sweeper deletes the lease for the whole(agentName, issueId)pair when any sibling row is stale). The automated review atfc1da6cleft one INFO item — confirm thesrc/lib/auth.tsdelta only extends worker tiering/allowlist entries: verified at this head, the only change is the anchoredWORKER_ALLOWLISTentry forPOST /api/agent-work/(start|checkpoint|finish|ownership)plus its comment; no other auth behaviour is touched.PR review at
c1f8b76(human, 1 BLOCKER): "renewing only freshest active AgentWork leaves older active rows eligible delete same current Lease" — the renewal refreshes one active row, butsweepStaleWorkscans every stale active record and deletes the lease of the whole(agentName, issueId)pair with no claim-token/lease-expiry guard, so an olderBLOCKEDrow could delete a recently renewed, valid same-agent claim and strip itsagent/*label. Fixed in648c5b7by making the sweep claim-incarnation safe (see What lands), with the reviewer's exact scenario as the regression: two active rows for one pair, an old staleBLOCKEDrow plus a freshly renewedIN_PROGRESSrow and a lease — after the sweep the lease, theagent/*label and the claim survive, the stale row is reclaimed, and noclaim_releasedevent is written.Adversarial review of that fix (
648c5b7, fresh session, no author context): the blocker is closed — the live-sibling predicate is the exact logical complement of the sweeper's own candidate clause (a candidate row can never be its own sibling), the NULL-expiry spelling matches the sweeper'sORsemantics, the lookup runs inside the same transaction as the mark, and the retry path is guarded too. It found no blocker, and raised one real defect in the change itself plus four test gaps, all fixed inc7f2116: a coordinator edit had landed on the wrong fixture and had silently replaced the NULL-expiry fixture (the NULL arm of the predicate was uncovered — mutation-verified), the central claim "live is the sweeper's predicate inverted, spelled positively" was pinned by no test (dropping the heartbeat arm, or OR-ing the arms instead of AND-ing them, left the suite green), an already reclaimedSTALEsibling was not pinned as not-live (counting it as live would withhold a pending release forever and strand the claim), the first-reclaim path's retry-marker clear was not asserted, and the docs said the sweep "reclaims" the stale row only when no live sibling owns the claim — the opposite of what it does (the row is always reclaimed; only the release is withheld). Its residual findings are pre-existing and out of this PR's scope, and are listed under Remaining external verification.Remaining external verification (operator)
c7f2116— pending, not run by this lane; nothing here claims they are green. (CI was green at the previous headfc1da6c— all 13 check runs succeeded — but that is evidence about the previous head, not this one.)renewOwnershiprenews the lease even when it heartbeats noAgentWorkrow (agentWorkHeartbeated: false), and aSTALE + staleClaimReleasePendingrow for the same pair is then retried and deletes that renewed lease. Closing it needs a lease↔claim-epoch correlation, which is the scope of #1317 / PR fix(agent-work): narrow stale sweep lease release to the marked claim epoch (#1317) #1319.IssueMutationGuardanchor the ownership operations take. Pre-existing; this change narrows the window, it does not widen it.src/lib/stale-work.tsand will conflict textually. This PR closes the reviewer's blocker on feat(ownership): optional worker renewal capability + fenced claim evidence (#1251) #1300 with the sibling-liveness guard, which additionally leaves theagent/*label alone — the half PR fix(agent-work): narrow stale sweep lease release to the marked claim epoch (#1317) #1319 documents as out of its scope. Reconciling the two is the maintainer's call.prisma migrate deployapplying20261018000000_worker_ownership_capabilityon a real Postgres at deploy time (alongside main's20261018000000_pr_fix_recovery_disposition).ORDER BYSQL.Handoff for a follow-up run
This round. The branch was mid-merge with
origin/main; the conflicts (AGENTS.md,src/lib/issue-lifecycle.ts) were resolved additively and committed as892fe84before any other change. Then the human review's remaining blocker onc1f8b76was fixed in648c5b7(the stale-work sweep is now claim-incarnation safe: it releases a stale row's claim only when no live siblingAgentWorkrow for the same pair still owns it), and the adversarial review of that fix was closed inc7f2116(the NULL-expiry fixture the coordinator's edit had displaced; the AND spelling of the live predicate pinned by two cases; a reclaimedSTALEsibling pinned as not-live; the first-reclaim path's retry-marker clear asserted; the docs wording corrected). Local checks: full suite / typecheck / lint green at892fe84,648c5b7andc7f2116(see receipts). CI, AI review and human review atc7f2116are pending — the operator verifies them after this run exits.Earlier.
Settled: token-fenced capability semantics on
Lease; reactivating a disposed row rotates the token and resets provenance while an ordinary renewal keeps both; renew ≠ checkpoint; late-renewal acceptance; disposition evidence-only and atomic with its projections; register replay writes nothing; windows as env config; the guard delete-on-throw posture forownership; the operator scan ranking leases by most recent activity (last renewal, else claim time) via a two-run read merged on that key with anidascending tie-break; a renewal refreshes the matching activeAgentWorkrow's heartbeat AND its expiry (the renewed expiry), so the stale-work sweep cannot reclaim a renewed long-running worker; and the active-AgentWork-vs-label-owner conflict comparison is independent of lease presence. Local checks: full suite/typecheck/lint green atc1f8b76(see receipts), and green ataa78b94and at the merge commit701dffb. The branch's commits are scoped (feature / review round 1 / review round 2 / PR-review fix / fix-review hardening / scan-order fix / scan-fix follow-up / the two MAJOR fixes of thefc1da6creview / the review follow-up) plus theorigin/mainmerge. #1254 (confirmation ladder + two-sided intent release protocol) deliberately consumesABANDONED_CANDIDATE/terminal_disposition_*evidence but must not be built here. #1312 tracks the per-issueselectLeaseordering; #1315 the renewal's crash window between the lease CAS and theAgentWorkrefresh; #1316 theblockeddisposition's lapsedAgentWorkexpiry; #1317 the sweeper's lease deletion by(agentName, issueId)pair — all deliberately not part of this PR.