Skip to content

feat(ownership): optional worker renewal capability + fenced claim evidence (#1251) - #1300

Merged
joryirving merged 16 commits into
mainfrom
courier/misospace/dispatch/issue-1251
Oct 20, 2026
Merged

joryirving merged 16 commits into
mainfrom
courier/misospace/dispatch/issue-1251

Conversation

@itsmiso-ai

@itsmiso-ai itsmiso-ai commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

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 existing Lease/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):
    • Attempt fencing on the feat(recovery): add fenced capacity-exhaustion handoff #1256 Lease.claimToken (design §6.1: the attempt-token discipline generalizes claimToken — 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 (CAS where {claimToken, disposition: null}).
    • Transient-outage posture: a renewal whose lease lapsed-but-row-exists is accepted (long-running worker reconnects with its receipt); a released/pruned claim gets 409 no_active_claim; after a terminal disposition, 409 disposed.
    • Occurrence vs observation: optional worker occurredAt (invalid/future/>24h silently falls back to server clock, same posture as tasks/report's startedAt) recorded alongside the server time in both the lease columns and the lifecycle events.
    • Durable provenance: register → registeredAt/registrationOccurredAt/runRef + one deduped ownership_registered lifecycle event + audit; renew → TTL extension, renewCount, renewal times, and a full refresh of the matching active AgentWork row — 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 its AgentWorkHistory row (external-wait marker set/cleared); disposition → terminal lease columns + AgentWork terminal state + deduped ownership_dispositioned event + audit. retryAfter is accepted only with blocked dispositions.
    • register/same-kind disposition are replay-idempotent per (issue, agent, token); a different disposition after a terminal one is 409 disposition_conflict.
    • Atomicity: register and disposition commit their evidence in one prisma.$transaction (CAS + AgentWork settlement + history + lifecycle event + audit row), so a partial write cannot strand an IN_PROGRESS AgentWork next to a terminal lease or lose the required lifecycle event behind a replay that reports success.
  • Schema: 13 nullable Lease columns + migration 20261018000000_worker_ownership_capability. Clients that never adopt the capability leave them unset and stay conservatively unverified (§6.2).
  • Derived states (src/lib/ownership-state.ts, pure): UNOWNED / CLAIMED_ACTIVE / CLAIMED_LONG_RUNNING / REVIEW_OWNED / EXTERNAL_WAIT / HELD / ABANDONED_CANDIDATE / CONFLICTED / UNKNOWN with machine-readable reason codes. Conflicting evidence → CONFLICTED (the lease owner and the active AgentWork owner are each compared against the sole agent/* label owner independently, so a lease that agrees with the label cannot excuse an active AgentWork row owned by somebody else); a label-only claim with no Dispatch evidence → UNKNOWN (never ABANDONED); HELD/REVIEW_OWNED short-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).
  • Operator inspect (maintainer-only; worker tokens get 403 by central tiering, not a local check): GET /api/issues/[issueId]/ownership (live forge label read, fail-soft to labelSource: "unavailable", never a 5xx; ?live=false for cache) and GET /api/issues/ownership (bounded cached scan, ?state=, limit ≤200, truncated + per-source sourcesTruncated). Raw claim tokens are never surfaced — only claimTokenPresent.
  • Bounded scan ranks by most recent activity (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'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. 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.
  • Stale-work sweep is claim-incarnation safe (src/lib/stale-work.ts): the sweep releases the claim of a stale AgentWork row only when no live sibling row for the same (agent, issue) still owns it. Several active rows can coexist for one pair (startAgentWork releases only the CLAIMED/IN_PROGRESS ones, so a BLOCKED row 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's Lease and strip its agent/* label. "Live" is the sweeper's own staleness predicate inverted, spelled positively (lastHeartbeatAt >= cutoff AND (leaseExpiresAt >= cutoff OR leaseExpiresAt IS NULL)) because leaseExpiresAt is 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 (marked STALE, retry marker cleared, so it stops asking) 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.
  • Windows/config + docs: 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.
  • Merge of origin/main (merge commit 892fe84, this round): the conflicts were AGENTS.md and src/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 #1262 durable-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 the api:ownership lifecycle source. Resolved by keeping both sides of each (the LIFECYCLE_SOURCES array is a plain TS union over a String column, so the two sources coexist). The earlier merge (merge commit 701dffb): the only conflict was prisma/schema.prisma — this branch adds 13 nullable Lease columns, main reformatted the whole schema. Resolved as main's formatting plus the 13 columns; npx prisma format is a no-op on the result and npx prisma validate passes. Main's 20261018000000_pr_fix_recovery_disposition migration touches PrFixQueueItem, this branch's touches Lease; 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

  1. No automated path releases a claim, deletes a Lease, writes an agent/* 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-work operator release/reassign, capacity-handoff release. The stale-work sweep is the one exception, and only to make it safer: it no longer releases a claim that a live sibling AgentWork row still owns (see What lands). It still releases every claim it released before, and it never releases one it did not.
  2. A release/renewal is fenced to the exact claim token. Same rule is enforced by issue-capacity-handoff.ts (lease.claimToken !== claimToken → stale_claim_token) and by lease.ts token 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 rotates claimToken atomically 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.
  3. Bound-worker identity is token-derived and exact-match; no agent/courier* wildcards; the unbound legacy token carries no identity. Enforced centrally by worker-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; body agentName cannot spoof a bound worker.
  4. Worker-tier reach stays an explicit allowlist; everything else defaults maintainer. WORKER_ALLOWLIST in auth.ts is 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).
  5. IssueLifecycleEvent remains 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, source api:ownership) and writes through recordLifecycleEvent with dedupe keys — no duplicate claim history, no parallel table.
  6. Claim/lease/release mutations serialize on the one per-issue IssueMutationGuard anchor (feat(recovery): add fenced capacity-exhaustion handoffs #1259). The capability ops take the same guard (operation: "ownership"). Because the guard has no resume branch for ownership and 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-release resume branches are byte-for-byte unchanged.
  7. A claim-path lease write describes the CURRENT attempt only (feat(recovery): add fenced capacity-exhaustion handoff #1256 comment in lease.ts). upsertLease clears 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.
  8. Absence of telemetry is never abandonment (design §5 tier 6). The reconciler doesn't exist yet; the read model encodes the rule (absent evidence → UNKNOWN, label-only legacy claims are surfaced, not abandoned), and nothing in this diff acts on ABANDONED_CANDIDATE.
  9. The claim token is a release-fencing secret and never leaves the owner/operator boundary. Surfaces for the owner (claim response, resolveActiveWork) unchanged; the new operator reads expose only claimTokenPresent (a test asserts the raw token string never appears in a response body); audit notes carry only bounded runRef/disposition metadata.
  10. The bounded operator scan ranks by most recent ACTIVITY, and the ranking is load-bearing. A lease's activity is its last renewal, falling back to its claim time when it was never renewed; register and disposition are not renewals and deliberately leave renewedAt alone. The scan reads the lease source as two runs that are each sorted by their own activity key and merges them on that key, with id ascending 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 compares Date columns by value so equal instants actually reach the tie-break clause.

Settled decisions

  • Reuse Lease.claimToken as 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, operation field) instead of three directories; semantics live in the lib.

  • Renewals are NOT lifecycle events (design maps renew → lease TTL + AgentWorkHistory heartbeat); 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; renew deliberately does not — its TTL extension is safely retryable and its AgentWork refresh 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 or blocked disposition; 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 orderBy cannot express COALESCE(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's orderBy stays 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 AgentWork row in full: heartbeat and expiry. The sweeper reclaims on lastHeartbeatAt OR leaseExpiresAt, 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 to Lease.expiredAt, never a second new Date().

  • The renewal refreshes the FRESHEST active AgentWork row for that (agent, issue). Several active rows are reachable (startAgentWork releases only CLAIMED/IN_PROGRESS, so a BLOCKED row 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 sole agent/* label owner does not excuse an active AgentWork row owned by somebody else (design §4 CONFLICTED, 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_released event are left alone.

Local validation receipts (coordinator; each run once per change state against the integrated tree)

Command cwd Revision Result
npm run typecheck /workspace fc1da6c exit 0, no errors
npm run lint /workspace fc1da6c exit 0; 0 errors, 1 pre-existing unrelated warning (src/app/login/page.tsx)
npm test (full) /workspace fc1da6c 195 files passed / 4 skipped; 4860 passed, 24 skipped, 1 todo, 0 failed
npm run typecheck / npm run lint / npm test (full) /workspace 701dffb (merge) exit 0 / exit 0 / 195 files passed / 4 skipped; 4855 passed, 24 skipped, 1 todo, 0 failed
npx prisma validate /workspace 701dffb schema at prisma/schema.prisma is valid
npm run typecheck / npm run lint / npm test (full) /workspace 335f1e6 (previous head) exit 0 / exit 0 / 4770 passed, 0 failed
npm run typecheck / npm run lint / npm test (full) /workspace aa78b94 exit 0 / exit 0 / 195 files passed / 4 skipped; 4868 passed, 24 skipped, 1 todo, 0 failed
npm run typecheck / npm run lint / npm test (full) /workspace c1f8b76 exit 0 / exit 0 / 195 files passed / 4 skipped; 4870 passed, 24 skipped, 1 todo, 0 failed
npm run typecheck / npm run lint / npm test (full) /workspace 892fe84 (merge of origin/main) exit 0 / exit 0 / 199 files passed / 4 skipped; 4992 passed, 24 skipped, 1 todo, 0 failed
npm run typecheck / npm run lint / npm test (full) /workspace 648c5b7 exit 0 / exit 0 / 199 files passed / 4 skipped; 4999 passed, 24 skipped, 1 todo, 0 failed
npm run typecheck / npm run lint / npm test (full) /workspace c7f2116 (head) exit 0 / exit 0 / 199 files passed / 4 skipped; 5002 passed, 24 skipped, 1 todo, 0 failed

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 >= cutoff arm 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 >= cutoff arm: 2 failed / 11 passed.

  • Counting an already reclaimed STALE sibling 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/skipped counter contamination: 1 / 3 failed.

  • Restoring the pre-fix single-query ordering (createdAt first): 2 failed / 25 passed — the two scan-order regressions.

  • Test double sorting asc as desc: 1 failed / 26 passed — the id tie-break test.

  • Never-renewed run ordered by renewedAt instead of createdAt: 1 failed / 28 passed.

  • Dropping the id tie-break from the never-renewed run: 1 failed / 28 passed.

  • Dropping the id tie-break from the renewed run: 1 failed / 28 passed.

  • Renewed run ordered by createdAt, or by renewedAt ascending: 1 failed / 29 passed each.

  • Reversing the merge's id tie-break: 1 failed / 29 passed.

  • Reverting the renewal's leaseExpiresAt propagation (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 AgentWork against 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):

  • Reverting tx.* → prisma.* inside the disposition transaction: 5 failed / 35 passed (lease CAS survived rollback; lifecycle written on the root client).
  • Restoring the old renewedAt-first lease ordering: 2 failed / 23 passed.
  • The lease.test.ts rotation test fails against the pre-fix lease.ts (1 failed / 34 passed).

Limitations: no PostgreSQL available in the lane, so $transaction rollback, guard serialization, the new ORDER BY SQL, 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 additive ALTER TABLE ... ADD COLUMN statements (nullable; one NOT NULL DEFAULT 0) and matches the Prisma schema column-for-column (reviewer-verified); production applies it via prisma migrate deploy at 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 object sweepStaleWork actually passes against the AgentWork row 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-path upsertLease reuse inheriting a terminal disposition), 3 MINOR (review-owned reason mislabel; retryAfter honored on non-blocked dispositions; unbounded list scan) — all fixed in 6c383d9 with regression tests. Round 2 delta review (head 6c383d9): all five resolved, no new MAJOR/BLOCKER; 3 further MINORs (terminal non-blocked disposition not clearing a stale wait marker; candidate truncation order dropping strong signals first; no truncation signal/doc drift) — all fixed in 8e18d74. 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, register replay not idempotent, operator scan not fully bounded. All fixed in 829565d; 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 $transaction mock 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 in 335f1e6. One NIT accepted and not fixed: the new lease ORDER BY cannot 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 by createdAt before renewedAt, 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 its asc and desc branches, so the id: "asc" tie-break was not actually tested. Both fixed in a2a4049 (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 it false, so NULLs were always last, contradicting the double's own documented Postgres behaviour); the never-renewed run's orderBy was 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 stamps renewedAt, which the register and disposition paths deliberately do not; and the per-issue selectLease ordering in src/lib/ownership-inspect.ts is the same class of defect in its narrowed multi-lease case. The first three are fixed in fc1da6c (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 keep AgentWork alive for the stale-work sweeper — it refreshed only lastHeartbeatAt, leaving the 5-minute leaseExpiresAt to 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 label agent/alice + lease alice + active AgentWork bob reported CLAIMED_ACTIVE instead of CONFLICTED. Both fixed in aa78b94: the renewal now propagates the renewed expiry onto the matching active AgentWork row, 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 in c1f8b76: the renewal's AgentWork match had no orderBy and 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's AgentWork write is outside the lease CAS — a crash between them loses the renewal), #1316 (a blocked disposition leaves its settled AgentWork row 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 at fc1da6c left one INFO item — confirm the src/lib/auth.ts delta only extends worker tiering/allowlist entries: verified at this head, the only change is the anchored WORKER_ALLOWLIST entry for POST /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, but sweepStaleWork scans every stale active record and deletes the lease of the whole (agentName, issueId) pair with no claim-token/lease-expiry guard, so an older BLOCKED row could delete a recently renewed, valid same-agent claim and strip its agent/* label. Fixed in 648c5b7 by 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 stale BLOCKED row plus a freshly renewed IN_PROGRESS row and a lease — after the sweep the lease, the agent/* label and the claim survive, the stale row is reclaimed, and no claim_released event 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's OR semantics, 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 in c7f2116: 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 reclaimed STALE sibling 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)

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 as 892fe84 before any other change. Then the human review's remaining blocker on c1f8b76 was fixed in 648c5b7 (the stale-work sweep is now claim-incarnation safe: it releases a stale row's claim only when no live sibling AgentWork row for the same pair still owns it), and the adversarial review of that fix was closed in c7f2116 (the NULL-expiry fixture the coordinator's edit had displaced; the AND spelling of the live predicate pinned by two cases; a reclaimed STALE sibling pinned as not-live; the first-reclaim path's retry-marker clear asserted; the docs wording corrected). Local checks: full suite / typecheck / lint green at 892fe84, 648c5b7 and c7f2116 (see receipts). CI, AI review and human review at c7f2116 are 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 for ownership; the operator scan ranking leases by most recent activity (last renewal, else claim time) via a two-run read merged on that key with an id ascending tie-break; a renewal refreshes the matching active AgentWork row'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 at c1f8b76 (see receipts), and green at aa78b94 and at the merge commit 701dffb. 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 the fc1da6c review / the review follow-up) plus the origin/main merge. #1254 (confirmation ladder + two-sided intent release protocol) deliberately consumes ABANDONED_CANDIDATE/terminal_disposition_* evidence but must not be built here. #1312 tracks the per-issue selectLease ordering; #1315 the renewal's crash window between the lease CAS and the AgentWork refresh; #1316 the blocked disposition's lapsed AgentWork expiry; #1317 the sweeper's lease deletion by (agentName, issueId) pair — all deliberately not part of this PR.

Courier added 3 commits October 18, 2026 17:53
…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
its-saffron[bot]

This comment was marked as outdated.

@joryirving joryirving left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. Major — re-claim clears terminal evidence without rotating the fencing token (src/lib/lease.ts:72-97; regression at src/lib/lease.test.ts:212-231). When upsertLease reuses a live-but-disposed row, it clears disposition and all associated terminal/wait fields but leaves claimToken unchanged. loadGuardedLease then 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 rotate claimToken atomically 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 returns stale_claim_token, and proves the new token succeeds. The existing test checks only that disposition fields clear and misses the token fence.

  2. 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 by updateMany before updating active AgentWork, 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 returns replayed: true at the early branch and skips all those unfinished side effects forever. That can leave an IN_PROGRESS AgentWork alongside a terminal lease and/or permanently omit the required ownership_dispositioned lifecycle event. Put the lease CAS and all required local DB effects in one Prisma transaction (using the transaction client for recordLifecycleEvent), 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.

  3. 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, but activeLeased and activeWorking each use unbounded findMany with no take. 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.

Courier added 2 commits October 18, 2026 20:15
…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.
@itsmiso-ai

Copy link
Copy Markdown
Contributor Author

Addressed every finding from @joryirving's and the automated review at 8e18d74. Both land in 829565d + 335f1e6.

MAJOR — re-claim does not establish a new attempt fence (src/lib/lease.ts)

Reactivating a disposed lease row is now a new attempt, not a renewal: the same prisma.lease.update that clears the terminal fields also rotates claimToken (newClaimToken()) and resets the retired attempt's provenance (runRef, registeredAt, registrationOccurredAt, renewCount, lastRenewal*). A row with no disposition is untouched — a heartbeating worker keeps its receipt and its liveness evidence, and the existing token-stability test still passes. The claim route already returns lease.claimToken, so the new attempt's worker gets the new token.
Regressions: lease.test.ts covers rotation-on-disposed and token/evidence-retention-on-live; the ownership route test drives the real upsertLease and proves the retired token → 409 stale_claim_token while the new token registers and disposes successfully.

MAJOR — disposition not recoverable after a partial write (src/lib/worker-ownership.ts)

Everything from the terminal CAS onward now commits in one prisma.$transaction, with recordLifecycleEvent receiving the transaction client: CAS + AgentWork settlement + history + lifecycle event + audit row. A throw rolls the whole fact back, so the retry re-enters the full path instead of short-circuiting into replayed: true. Failure codes, statuses and replay semantics are unchanged. register's fresh path is transactional for the same reason.
Regressions: injected failure after the lease update and after the AgentWork update; each asserts a 500 with nothing committed, and that the retry converges to terminal AgentWork + exactly one history row + exactly one ownership_dispositioned event + exactly one audit row.

MINOR — register replay was not idempotent

An already-registered attempt returns the persisted registration with replayed: true and writes nothing — no timestamp refresh, no runRef overwrite, no second audit row. Because reactivation resets registeredAt, a fresh attempt still registers normally. RegisterOwnershipSuccess.replayed widened to boolean; nothing outside the module consumed the literal false.

MINOR — operator scan was not fully bounded

activeLeased and activeWorking now carry the same MAX_CANDIDATE_SCAN cap with deterministic ordering, and the response reports sourcesTruncated: { labeled, leased, working } (exact, via one probe row) alongside the existing truncated.

Follow-up round 335f1e6 from an independent review of the fix — no blockers, three MINORs + three NITs, all fixed:

  • The atomicity tests could not fail for the reason they claimed. The $transaction mock handed the callback the same client as prisma and rolled back globally, so swapping every tx.* for prisma.* left the suite green. 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 the rollback, that the lifecycle event is written through the transaction client, and that no write in the transaction path goes through the root client. Reverting tx.* → prisma.* now fails 5 tests.
  • The lease ordering was backwards. upsertLease's create path never sets renewedAt, so a renewedAt-primary nulls-last sort put every newly claimed lease last and the cap dropped the freshest claims. Now createdAt desc first, then renewedAt nulls-last, then id.
  • register's in-transaction lease update is now fenced on claimToken, symmetric with the disposition CAS.
  • Module rule and docs/worker-ownership-capability.md corrected on token rotation/provenance reset and the transaction + replay contract; the atomicity rule is scoped to the operations that assert a new fact (renew is deliberately excluded, with the reason).

Accepted, not fixed: the new lease ORDER BY cannot use @@index([expiredAt]), so Postgres still sorts matching active leases server-side. The cap still bounds what is transferred and derived; bounding the server-side sort needs an index change, which is out of scope for this fix.

Local validation at 335f1e6, run against the integrated tree: npm run typecheck exit 0; npm run lint exit 0 (0 errors, 1 pre-existing unrelated warning in src/app/login/page.tsx); npm test exit 0 — 191 files passed / 4 skipped, 4770 passed, 22 skipped, 1 todo, 0 failed. CI at this head is pending, not verified here.

@joryirving joryirving left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. MINOR — lease scan order is not actually most-recently-touched (src/app/api/issues/ownership/route.ts:122-130). The query sorts createdAt DESC before renewedAt 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 than MAX_CANDIDATE_SCAN rows, 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 timestamp COALESCE(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.

  2. 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, so id: "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.

its-saffron[bot]

This comment was marked as outdated.

Courier added 3 commits October 19, 2026 23:23
…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).
@itsmiso-ai

Copy link
Copy Markdown
Contributor Author

Disposition of the open findings on this PR, at head fc1da6c (merge 701dffb + a2a4049 + fc1da6c). Every finding below is either fixed here or filed as a follow-up with the reason.

Review at 335f1e6 (human + automated, same two findings)

1. MINOR — the bounded lease scan ordered by createdAt before renewedAt, so an old lease renewed moments ago could be dropped behind newer stale leases when the cap overflowed. Fixed in a2a4049.

Freshness is now the row's most recent activity: the last renewal, falling back to the claim time when the lease was never renewed. Prisma's orderBy cannot express COALESCE(renewedAt, createdAt), so the lease source is read as two runs that are each already sorted by their own activity key — renewed leases by renewedAt, never-renewed leases by createdAt — and merged on that key, with id ascending as the deterministic tie-break in both the query and the merge. The merge compares heads only and never re-sorts, so the query's orderBy stays the single source of ordering truth.

Regressions added in src/app/api/issues/ownership/route.test.ts: an old lease renewed moments ago now outranks a newer-created, never-renewed lease; the cap-overflow case keeps the recently renewed lease and drops the stale never-renewed one (the opposite of the pre-fix behaviour, where the live, long-lived claim was the row that fell off the end of the scan).

2. NIT — the test double returned descending order for both its asc and desc branches, so the id: "asc" tie-break was not actually tested. Fixed in a2a4049, and hardened further in fc1da6c.

Two things were wrong with the double, not one: the comparator treated asc as desc, and it 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 entirely. The double now compares by value and implements ascending/descending plus Postgres' null placement faithfully. New cases: the id tie-break inside the renewed run, the id tie-break inside the never-renewed run, and the same tie-break across the two runs.

Independent adversarial review of this round's diff (a2a4049, no author context)

3. MINOR — the double's null-placement fallback was defeated by ??. typeof spec === "object" && spec?.nulls is false for a plain string sort spec, and false ?? default keeps the false, so NULLs were always placed last — contradicting the double's own documented Postgres behaviour. Fixed in fc1da6c: the fallback is now an explicit conditional.

4. MINOR — 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). Fixed in fc1da6c with three cases — claim-time order, renewal-time order, and the id tie-break in each run. Every mutation now fails the suite:

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 validate on 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.

its-saffron[bot]

This comment was marked as outdated.

@itsmiso-ai

Copy link
Copy Markdown
Contributor Author

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.

@its-saffron

its-saffron Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

🔄 This PR fix item has been requeued and is active again — no human attention needed.

Updated automatically by Dispatch on 2026-10-20T16:02:09.927Z

@joryirving joryirving left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Major — ownership renewal does not keep AgentWork alive for the existing stale-work sweeper. sweepStaleWork marks a row stale when either lastHeartbeatAt or leaseExpiresAt is older than the cutoff. This renewal updates only lastHeartbeatAt, leaving AgentWork.leaseExpiresAt at 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.

  2. Major — the conflict model can hide a contradictory active AgentWork owner. With label agent/alice, lease owner alice, and active AgentWork owner bob, the lease matches the label, and the AgentWork mismatch check is skipped solely because a lease exists. The result can be CLAIMED_ACTIVE instead of CONFLICTED, 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.

Comment thread src/lib/worker-ownership.ts Outdated
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() } });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 — heartbeatMatchingAgentWork writes { lastHeartbeatAt: now, leaseExpiresAt: expiresAt }. now is the operation's own clock (input.now ?? new Date()) and expiresAt is the same value written to Lease.expiredAt (now + DISPATCH_OWNERSHIP_RENEW_TTL_MS), so the AgentWork expiry can never drift from the lease expiry. It is no longer a second new Date().
  • The lease CAS, the route, the disposition path, sweepStaleWork itself 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 blocked disposition 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.

Comment thread src/lib/ownership-state.ts Outdated
if (lease !== null && lease.agentName !== labelOwner) {
conflictReasons.push("lease_owner_mismatch");
}
if (lease === null && activeAgentWork !== null && activeAgentWork.agentName !== labelOwner) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Courier added 2 commits October 20, 2026 04:07
…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.
its-saffron[bot]

This comment was marked as outdated.

…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.
its-saffron[bot]

This comment was marked as outdated.

@itsmiso-ai

Copy link
Copy Markdown
Contributor Author

Courier: the two MAJOR findings from the review at fc1da6c are fixed. Head is now c1f8b76 on the run branch courier/misospace/dispatch/issue-1251 (open, non-draft, pushed). Both review threads have per-finding replies; this comment is the round's handoff.

What changed

1. A renewal now keeps the matching AgentWork alive for the stale-work sweep (aa78b94). sweepStaleWork reclaims on lastHeartbeatAt or leaseExpiresAt, and the renewal only moved the heartbeat — leaving the 5-minute leaseExpiresAt to pass the sweeper's cutoff, so a renewed long-running worker was still marked STALE and its Lease deleted. renewOwnership now propagates the renewed ownership expiry onto the matching active AgentWork row, using the operation's own clock and the same expiry written to Lease.expiredAt. The sweeper's predicate, the lease CAS, the route and the disposition path are unchanged.

2. The conflict model no longer hides a contradictory active AgentWork owner (aa78b94). The agent_work_owner_mismatch check no longer requires lease === null, so label agent/alice + lease alice + active AgentWork bob derives CONFLICTED instead of CLAIMED_ACTIVE. Reason-code order, ownerAgent, the sole-label-owner rule and the non-active-state rule are unchanged; the blast radius is the two maintainer-only inspect surfaces, which surface the conflict and never act on it.

3. Follow-up on the fix (c1f8b76), from a fresh adversarial review of the diff (verdict: APPROVE, both findings confirmed closed, invariants intact): the renewal's AgentWork match had no orderBy and 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". All three fixed. The reviewer's INFO item on src/lib/auth.ts is verified: the only change is the anchored WORKER_ALLOWLIST entry for POST /api/agent-work/(start|checkpoint|finish|ownership) plus its comment.

Local validation (coordinator, against the integrated tree)

Command cwd Revision Result
npm run typecheck /workspace c1f8b76 exit 0, no errors
npm run lint /workspace c1f8b76 exit 0; 0 errors, 1 pre-existing unrelated warning (src/app/login/page.tsx)
npm test (full) /workspace c1f8b76 195 files passed / 4 skipped; 4870 passed, 24 skipped, 1 todo, 0 failed

Mutation checks proving the new regressions bite: reverting the renewal's leaseExpiresAt propagation → 3 failed / 40 passed; restoring the lease === null && guard → 2 failed / 59 passed; comparing the active AgentWork against the lease owner → 1 failed / 61 passed. The same suite was green at aa78b94 (4868 passed).

Limitations: no PostgreSQL in this lane, so $transaction rollback, the new ORDER BY SQL and the two-run lease scan are exercised only through the vitest fakes. The sweeper's candidate selection is exercised without a database by evaluating the WHERE clause object sweepStaleWork actually passes against the AgentWork row the renewal actually wrote — the test does not re-implement the predicate — plus a negative control on the pre-fix write shape.

Deliberately not in this PR (follow-ups filed)

  • #1315 — the renewal's AgentWork write is outside the lease CAS: a crash between them loses the renewal.
  • #1316 — a blocked disposition leaves its settled AgentWork row with a lapsed expiry, so the sweeper reclaims the parked lease.
  • #1317 — the sweeper deletes the lease for the whole (agentName, issueId) pair when any sibling row is stale.
  • #1312 — the per-issue selectLease ordering (unchanged, as before).

Remaining external verification (operator)

  • CI checks, AI review and human review at head c1f8b76 — pending; nothing here claims they are green. (CI was green at the previous head fc1da6c — all 13 check runs succeeded — but that is evidence about the previous head, not this one.)
  • prisma migrate deploy applying 20261018000000_worker_ownership_capability on a real Postgres at deploy time, and real-Postgres execution of the two-run lease scan.

@itsmiso-ai

Copy link
Copy Markdown
Contributor Author

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 joryirving left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Courier and others added 2 commits October 20, 2026 15:08
…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>
its-saffron[bot]

This comment was marked as outdated.

…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>
@itsmiso-ai

Copy link
Copy Markdown
Contributor Author

Courier: the remaining BLOCKER from the review at c1f8b76 is fixed. Head is now c7f2116 on the run branch courier/misospace/dispatch/issue-1251 (open, non-draft, pushed). This comment is the round's handoff.

First, the branch was mid-merge when this run started. The merge with origin/main stopped on conflicts in AGENTS.md and src/lib/issue-lifecycle.ts; both were purely additive (main added the #1203 lifecycle-backfill surface and the #1262 durable-replay row to the subsystem map, this branch added the #1251 worker-ownership row and the api:ownership lifecycle source), so both sides were kept. The LIFECYCLE_SOURCES array is a plain TS union over a String column, so the two sources coexist. Committed as 892fe84, before any other change.

The blocker

renewing only freshest active AgentWork leaves older active rows eligible delete same current Lease … sweepStaleWork … 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.

Fixed in 648c5b7, with the review hardening in c7f2116.

The fix — the sweep is now claim-incarnation safe

src/lib/stale-work.ts. The sweep releases a stale row's claim only when no live sibling row for the same (agent, issue) still owns it. "Live" is the sweeper's own staleness predicate inverted, but spelled positively:

lastHeartbeatAt >= cutoff AND (leaseExpiresAt >= cutoff OR leaseExpiresAt IS NULL)

The positive spelling is load-bearing: leaseExpiresAt is nullable, and SQL three-valued logic makes NULL >= cutoff UNKNOWN rather than false, so a negated form of the sweeper's own clause would hide a fresh-heartbeat/NULL-expiry row — which is not stale under the sweeper's own OR predicate and therefore must count as live.

The lookup runs inside the same $transaction as the stale mark, before the mark. When a live sibling exists:

  • the stale row is still reclaimed (marked STALE, leaseExpiresAt moved to now) and its staleClaimReleasePending retry marker is cleared, so it stops asking for a release and the retry loop terminates;
  • the Lease is not deleted and releaseIssueClaim is not called — so the agent/<name> label survives and no claim_released event is written;
  • the sweep reports it as claimRetained (a new counter on StaleWorkReport, alongside the unchanged released/skipped/errors) and audits it as stale_agentwork_claim_retained with unchanged before/after labels.

The guard applies on the retry path too: a row already marked STALE + staleClaimReleasePending re-runs the lookup, so a retry cannot delete a lease a live sibling now owns.

When no live sibling exists, the behaviour is unchanged — same mark, same lease.deleteMany, same releaseIssueClaim options, same finalize, same stale_agentwork_released audit row, same releaseKey idempotency.

No heartbeat was faked. heartbeatMatchingAgentWork is untouched: the renewal still refreshes only the freshest active row, and the protection comes from the sweep refusing to release a claim that another live row owns — exactly the alternative the review named.

The regression

src/lib/stale-work.test.ts — the review's scenario verbatim: two active rows for one pair (an old stale BLOCKED row and a freshly renewed IN_PROGRESS row), plus a live lease. After the sweep: the lease was not deleted, the agent/alpha label survives untouched, no claim_released event was recorded, the stale row was reclaimed, and claimRetained === 1 / released === 0.

The fake evaluates the production WHERE clauses faithfully — it applies the agentName/issueId/id not/state in/lastHeartbeatAt gte/leaseExpiresAt gte-or-null conditions from the query the production code actually passes, and throws on any clause key or operator it does not implement, so it can never silently agree with a clause it does not understand.

Also pinned: the negative control (the same stale row with no live sibling still deletes the lease, removes the label and records exactly one claim_released); a stale sibling does not shield the claim; a sibling that is fresh by heartbeat but expired by lease expiry — and the converse — is not live (this is what pins the AND spelling); an already reclaimed STALE sibling is not live (counting it as live would withhold a pending release forever and strand the claim); a sibling with a NULL lease expiry is live; a sibling on another issue/agent is not; the first-reclaim path clears the retry marker; and the live sibling row is untouched by the reclaim.

Mutation checks (each applied to the working tree, run, then reverted)

Mutation Result
Neutering the live-sibling lookup entirely 3 failed / 10 passed
Dropping the lastHeartbeatAt >= cutoff arm 1 failed / 12 passed
Dropping the leaseExpiresAt >= cutoff arm 2 failed / 11 passed
Counting a reclaimed STALE sibling as live 1 failed / 12 passed
Leaving the retry marker set on the first-reclaim path 1 failed / 12 passed
Retained path still deleting the lease 7 failed
Retained path still calling releaseIssueClaim 2 failed
released/skipped counter contamination 1 / 3 failed

Local checks (coordinator, against the integrated tree)

Command cwd Revision Result
npm test (full) /workspace 892fe84 (merge) 199 files passed / 4 skipped; 4992 passed, 24 skipped, 1 todo, 0 failed
npm test (full) /workspace 648c5b7 199 files passed / 4 skipped; 4999 passed, 24 skipped, 1 todo, 0 failed
npm test (full) /workspace c7f2116 (head) 199 files passed / 4 skipped; 5002 passed, 24 skipped, 1 todo, 0 failed
npm run typecheck /workspace c7f2116 exit 0, no errors
npm run lint /workspace c7f2116 0 errors; 1 pre-existing unrelated warning (src/app/login/page.tsx)

Independent adversarial review of this round's diff

A fresh review session (no author context) on 648c5b7 found no blocker — 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's OR semantics, the lookup runs inside the mark's transaction, and the retry path is guarded. It did find one real defect in the change itself (a fixture had been displaced, leaving the NULL-expiry arm of the predicate uncovered — mutation-verified) plus four test gaps (the AND spelling was pinned by no test; a reclaimed STALE sibling was not pinned as not-live; the first-reclaim path's retry-marker clear was not asserted; the docs said the sweep "reclaims" the row only when no live sibling owns the claim, which is the opposite of what it does). All four are fixed in c7f2116.

What is deliberately not closed here (pre-existing, unchanged by this PR)

  • A renewed claim with no live sibling row is still unprotected: renewOwnership renews the lease even when it heartbeats no AgentWork row, and a STALE + staleClaimReleasePending row for the same pair is then retried and deletes that renewed lease. Closing that 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 — it is the residual of this same hazard family, and it is not introduced here.
  • The guard is a snapshot check inside a READ COMMITTED transaction and the label removal happens outside any transaction, so a renewal that commits between the lookup and the delete can still lose its lease. The sweep does not take the per-issue IssueMutationGuard anchor the ownership operations take. Pre-existing; this change narrows the window, it does not widen it.

Overlap note: this PR and PR #1319 now both touch src/lib/stale-work.ts and will conflict textually. This PR closes the blocker above with the sibling-liveness guard and additionally leaves the agent/* label alone — the half #1319 documents as out of its scope. Which of the two lands, and in what order, is the maintainer's call; I have not touched #1319.

Pending (not claimed green)

CI checks, the automated review and the human review at c7f2116 are pending — this lane does not run or wait for them, and nothing above claims they are green. CI was green at the previous head fc1da6c, but that is evidence about a different commit.

@itsmiso-ai

Copy link
Copy Markdown
Contributor Author

CI observation on head c7f2116 (single read, not polled; taken after the run's local checks).

12 of the 13 check runs completed with conclusion success: Tests, Typecheck, Lint, Workflow lint, Build, Database migrations, Database integration, Coverage, npm audit, smoke, Docker Build, Docker Build (MCP). The 13th, the review job (automated reviewer), was still in_progress at the time of the read.

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.

its-saffron[bot]

This comment was marked as outdated.

@its-saffron
its-saffron Bot dismissed stale reviews from themself October 20, 2026 16:01

Superseded by a newer automated review for this pull request.

@joryirving joryirving left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/lib/stale-work.ts Outdated
// 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({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/lib/worker-ownership.ts Outdated
// 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({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@its-saffron its-saffron Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: fixed
  • PRRT_kwDOSZt6uM6uafNE: fixed
  • PRRT_kwDOSZt6uM6uLvOt: open — fixed without evidence citing current code
  • PRRT_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.

@its-saffron
its-saffron Bot dismissed their stale review October 20, 2026 18:24

Superseded by a newer automated review for this pull request.

@joryirving joryirving left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@joryirving
joryirving merged commit a26e7d7 into main Oct 20, 2026
13 checks passed
@joryirving
joryirving deleted the courier/misospace/dispatch/issue-1251 branch October 20, 2026 18:37
@its-miso its-miso Bot mentioned this pull request Oct 20, 2026
@itsmiso-ai

Copy link
Copy Markdown
Contributor Author

Round at head 9a76a65 — review follow-up: sweeper fence + current-claim-work pick

Both findings still open on c7f2116 are addressed and pushed on this branch (376c8d2 merges origin/main again — clean, ci: enable safe /ai-review #1326; 9a76a65 carries the fixes). This comment updates the PR's invariant list (the body is left untouched this round; where it contradicts this comment, this comment supersedes it).

Finding 1 (blocker, src/lib/stale-work.ts) — the TOCTOU is now fenced on two levels

  1. Per-issue mutation fence. The whole decide-and-release pipeline for an issue-scoped claim — sibling lookup, stale mark, lease delete, external releaseIssueClaim, finalization — runs under the same IssueMutationGuard fence (operation: "stale-release") that claim / lease / ownership / capacity-release take. A concurrent renewal or fresh claim cannot commit inside the window; it gets the retryable 409 mutation_busy and re-decides against post-sweep state. A busy fence skips the candidate for this sweep (nothing is owed outside a completed fenced window). issue-mutation-guard.ts grew exactly one resume branch: same operation and same agentName, non-executing only — so a release that failed mid-way is resumed by the next sweep (every step is a conditional update keyed off persisted row state; the external release is deduped per claim epoch via releaseKey). A guard left executing by a crashed sweep process is never auto-taken-over — fail-closed like a crashed claim guard, operator-visible (same posture as feat(recovery): add fenced capacity-exhaustion handoffs #1259). Trailing audit writes are now non-fatal (surfaced via report.errors): a failed post-decision audit can no longer strand the fence behind a finalized row with no resumable candidate behind it (this was the one major an independent adversarial pass found on this delta; fixed).
  2. Claim-epoch CAS. The lease delete is a compare-and-delete against a snapshot of {claimToken, expiredAt, lastRenewalObservedAt} taken before the sibling lookup — every renew moves expiredAt+lastRenewalObservedAt, every re-claim moves one of them or rotates claimToken — plus a post-window existence recheck on the pair. With no snapshot row, no delete is attempted at all, so a fresh claim created inside the window can only be caught, never consumed (no clock comparison anywhere). Any mismatch ⇒ back off: the row is still reclaimed STALE, but the lease, label and claim survive and it counts as claimRetained, audited with reason "claim epoch moved". A terminal disposition moves none of the fence fields, so a cleanly-disposed claim is still swept (pinned by test).

Invariant 6 (updated): claim / lease / sweep-release mutations now all serialize on the one per-issue IssueMutationGuard anchor. claim/capacity-release resume branches unchanged; the new stale-release branch is covered by production-code guard tests (exact resume; mismatch matrix busy with zero writes; executing stays busy).

Superseded body line: under Remaining external verification, "the sweep does not take the per-issue IssueMutationGuard … Pre-existing; this change narrows the window" — closed at 9a76a65 as above.

Deterministic interleaving tests (src/lib/stale-work.test.ts, real-clause fake + a mutual-exclusion guard fake): renewal committing exactly between the sibling lookup and the delete → fenced delete matches nothing → lease (with the renewed expiry), agent/* label, live work and lifecycle stream all survive; fresh claim created inside the window → not deleted, not released; the real fence entry point refuses a renewal that attempts to commit mid-window (busy, its write never runs); busy-skip touches nothing; negative controls pin the release path and the exact CAS clause.

Finding 2 (major, src/lib/worker-ownership.ts) — one deterministic current-claim-work pick

New Invariant 11: several active AgentWork rows can coexist for one (agent, issue); the row that is the current claim's work is defined once — freshest lastHeartbeatAt, then createdAt desc, then id desc (CURRENT_CLAIM_WORK_ORDER_BY / findCurrentClaimWork) — and every path that names that row goes through it: renewal heartbeat, terminal disposition settlement, and the operator inspect evidence row (ownership-inspect.ts imports the same order). A disposition can no longer terminalize an older BLOCKED sibling while the current IN_PROGRESS attempt stays active; the row a renewal protects is the row a disposition settles. Regressions: two-active-row dispositions in both pool slots, tie-breaks, and the renew-side freshness control.

Not widened into this PR: checkpointAgentWork still selects an unordered active row (verified pre-existing; it can stamp an older sibling fresher than the current attempt) — filed as #1328.

Deliberately not taken: keeping staleClaimReleasePending=true on an epoch-moved back-off. The next sweep would then CAS-match the renewed lease and delete it whenever the renewal heartbeated no AgentWork row — back-off-and-stop is the safe direction. The residual label-without-lease skew an unguarded lease-cleanup deleter could leave inside the window is fail-closed and in #1317's family.

Local validation receipts at 9a76a65 (coordinator, integrated tree)

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.

@itsmiso-ai

Copy link
Copy Markdown
Contributor Author

Postscript: this PR was squash-merged as a26e7d7 at 18:37Z from head 9a76a65 — verified byte-for-byte: git diff 9a76a65 origin/main over the eight files of this round's fix (stale-work.ts, worker-ownership.ts, issue-mutation-guard.ts, ownership-inspect.ts + their tests + the capability doc) is empty, so the fence (operation: "stale-release"), the claim-epoch CAS, the non-fatal trailing audits and the shared current-claim-work selection are all in main. The previous comment's "pending" line referred to CI/AI/human checks on 9a76a65, which the merge operator ran before merging; this lane does not re-verify them. Follow-up open: #1328 (checkpointAgentWork unordered active-row selection, pre-existing).

joryirving pushed a commit that referenced this pull request Oct 21, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(ownership): persist optional worker renewal and fenced claim evidence

2 participants