Repository navigation
docs(recovery): ownership reconciliation design (#1245) - #1246
Conversation
![its-saffron[bot]](https://avatars.githubusercontent.com/in/3901073?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
joryirving
left a comment
joryirving
left a comment
There was a problem hiding this comment.
Reviewed exact head 5ef2d5e against design issue #1245, existing Lease/AgentWork/releaseIssueClaim primitives, the claim/renew and stale-work paths, and the entire design. The document covers worker-neutral optional renewal, holds, linked PRs, conflict/unknown states, bounded confirmation, operator controls, and no wildcard agent/courier* migration. The additive README link and design-only scope are appropriate. CI, Security Audit, PR Smoke, Build and AI review passed, and the branch is mergeable.
REQUEST_CHANGES [P1]: the claimed atomic ownership fence does not cover the external GitHub label deletion. §8.6 says every release is CAS-conditioned on the current owner, attempt and lease/heartbeat versions, and that release + cache update + audit commit together, but then proposes targeted removeIssueLabel over the forge adapter. A Prisma CAS/transaction cannot atomically fence GitHub's remote label removal.
Concrete race: generation g is conclusively stale; the reconciler verifies agent/alpha, marks the old claim released and commits CAS/audit; before its removeIssueLabel(issue, "agent/alpha") request executes (or while the request is in flight), the same agent reclaims the issue with a new attempt token g+1, and the GitHub label is now valid for the new live claim. The old reconciler's targeted label delete then removes agent/alpha from GitHub, silently stealing the new owner's eligibility/visibility, even though its local CAS was valid when executed. If the external deletion happens first, the opposite failure window remains: a crash/timeout after a successful GitHub delete but before local commit requires replay without accidentally deleting a newly re-established claim.
The current §8.2 step 5 ('fencing check immediately before the write'), §8.4 ('release atomically with fence') and §10 concurrent-renewal example don't define a cross-system serialization protocol for this exact race. Please specify the two-sided release/renew/claim coordination boundary, including what can be fenced at GitHub vs only in Postgres, an explicit pending-release claim/admission barrier or other sound serialization strategy, the point at which a new attempt may issue/re-add the same label, and a crash-safe recovery algorithm for GitHub-success/DB-failure that cannot delete a later attempt. If GitHub cannot support the necessary conditional mutation, the safe design is to defer ambiguous automation/require an operator, not claim atomicity from the local DB alone.
Pin negative controls for (1) renewed same-agent claim during stale release, (2) changed-owner reassign, (3) GitHub remove succeeds but DB settlement fails, and (4) retry after an old deletion with a fresh live token. This is a design-only correction, not authorization to implement new infrastructure in this PR. Existing #1251/#1254 implementation tickets can consume the corrected boundary.
Other #1245 acceptance elements look satisfied. Not merged.
…encing (#1245) Address review P1 on PR #1246: a Postgres CAS cannot fence a remote GitHub label deletion. Replace the atomicity claim in §8.6 with an explicit statement of what the CAS cannot do, add §8.7 defining the intent-based release protocol (fenced Phase A intent commit + claim-admission barrier, serialized forge delete, settle), a crash/replay matrix, fail-closed operator path, reassign handling, and four pinned negative controls. Refresh file/line citations drifted by the main merge.
Independent review found the claim path still held only by a read-then-act check against the pending intent, and the invariant overstated against owner-unclaim/force-claim direct writes. Define a single per-issue serialization anchor: the claim path commits its own claim intent + attempt token under the anchor before the remote add (superseding the current add-before-lease route order), the barrier holds any claim on the issue until settle, legacy rowless claims get an intent-only Phase A, exempt owner/operator paths are named, reassign label-routing is flagged as new behavior, and a fifth negative control pins the in-flight-claim interleaving in both lock orders. Refresh stale-work transaction citations.
#1245) Follow-up review minors: scope the invariant sentence to protocol paths and name direct assignment (POST /api/issues/actions assign_agent) as a third audited exemption with settle-or-supersede under the anchor; move claim-intent records into the §8.7 state paragraph with an explicit replay owner; add the claim-side crash row (intent committed, remote ADD missing) to the replay matrix.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…merge (#1245) Adds a requirements-traceability section (#14) mapping every #1245 requirement to its design section and negative controls, with the design-only enforcement boundary delegated to #1251/#1254. Re-pins all file:line citations drifted by the capacity-handoff (#1259) merge and notes that withIssueMutationGuard/Lease.claimToken already land the anchor shape the two-sided protocol builds on.
|
Addressing the automated review at
Independent review round 3 at |
joryirving
left a comment
joryirving
left a comment
There was a problem hiding this comment.
Reviewed exact head 086d6c9 against design issue #1245, the prior P1, and merged Dispatch #1259. APPROVED (design-only). The earlier Postgres-CAS-versus-GitHub-delete defect is now explicitly acknowledged in §8.6, and §8.7 specifies a two-sided intent/claim barrier with a per-issue serialization anchor, token fencing, idempotent crash replay, and the same-agent/new-owner interleaving tests. It reuses #1259's IssueMutationGuard/Lease.claimToken instead of introducing another lease system. §14 traces the design requirements to their sections and defers runtime enforcement to #1251/#1254, appropriately for this docs-only PR. No new actionable blocker in the requested scope. Implementation must enforce the specified anchor on all participating label-write paths, including privileged/operator paths and any in-flight remote writes; the document is not itself proof of cross-system atomicity. CI, Security Audit, Build, and PR Smoke passed; AI review was still running when submitted.
Superseded by a newer automated review for this pull request.
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: qwen3.8-flash-next-chat (openai) — fallback (primary failed) · pr-reviewer-action v3.4.1
Requirement traceability gap: the review itself completed, but one or more in-scope requirements could not be verified against enforcement or test evidence. Approval is withheld until that requirement is covered.
Findings (1 major)
| Severity | Location | Finding |
|---|---|---|
| requirement not enforced: Specify bounded reconciliation, expiration/grace periods, restart recovery and explicit operator controls. Evidence that a worker is unreachable or telemetry is missing must not immediately mean abandonment. |
Recommendation
Approve. This is a design-only PR adding docs/ownership-reconciliation.md and a README link, addressing issue PR 1245. The document thoroughly inventories existing mechanisms, defines an ownership state model, evidence hierarchy, reconciliation protocol with fencing, and explicitly addresses the P1 concern raised in previous reviews regarding cross-system atomicity between Postgres CAS and GitHub label mutations.
The classifier flagged path_handling_changes, likely due to file path references in the documentation or generic keyword matching. However, the diff contains no executable code, no file system operations, no user input handling, and no path construction logic. It is purely Markdown documentation. Therefore, path traversal risks are not applicable to this specific change set.
Required Checks Disposition
- Review for path traversal vulnerabilities:
not_applicable. The PR only modifies/adds Markdown files (README.md,docs/ownership-reconciliation.md). There is no code that processes paths, reads/writes files based on input, or constructs URLs from user-supplied data. The content discusses paths (e.g.,src/lib/...) but does not execute them. - Test with edge-case paths (null bytes, symlinks):
not_applicable. As above, there is no runtime code introduced or modified that would interpret paths. Documentation cannot be vulnerable to null byte injection or symlink attacks in the context of execution. - Treat as high priority — verify correctness carefully:
satisfied. The design document has been reviewed against the linked issue PR 1245. It correctly identifies the limitations of the current lease/label system, proposes a robust two-sided boundary (§8.7) using Postgres intents to serialize access before remote GitHub writes, and explicitly handles the race conditions described in the human review (P1). The traceability matrix (§14) maps all requirements to sections in the doc. CI checks (Lint, Typecheck, Build, Tests) have passed.
Human Review Dispositions
- Review ID 5510890265 (joryirving):
addressed. The reviewer's P1 concern was that "a Postgres CAS cannot fence the external GitHub label deletion." The updated design (§8.6 and §8.7) explicitly acknowledges this limitation ("What the CAS cannot do") and introduces a new protocol: Phase A commits a "pending intent" in Postgres under a serialization anchor (withIssueMutationGuard), which blocks concurrent claims via a barrier. Phase B performs the remote delete only while the intent exists. Phase C settles the intent. This effectively serializes the remote write against local claim attempts, resolving the race condition where a stale reconciler could strip a fresh claim's label. The design now builds on landed machinery (PR 1259IssueMutationGuard) rather than assuming atomicity across systems.
Linked Issue Fit
The PR fully addresses the design deliverables requested in PR 1245:
- Inventory: Section 3 details current
Lease,AgentWork, and label semantics. - State Model: Section 4 defines derived states like
CLAIMED_ACTIVE,ABANDONED_CANDIDATE,CONFLICTED. - Evidence Hierarchy: Section 5 establishes tiers, ensuring missing telemetry alone never triggers abandonment.
- Reconciliation & Fencing: Sections 8.1–8.7 specify bounded passes, grace periods, and the critical cross-system fencing protocol.
- Operator Controls: Section 9 outlines escape hatches.
- Non-goals: The PR strictly adheres to the "design only" constraint; no implementation code is changed.
Standards Compliance
The repository standards (AGENTS.md) emphasize strict Prisma relations and clear API contracts. While this PR adds no code, the design document aligns with these principles by proposing reuse of existing primitives (Lease, AgentWork, PrFixQueueItem patterns) rather than inventing parallel systems, and by emphasizing audit logging and fail-closed behavior.
Requirement trace
1 of 3 requirement(s) not fully traced to enforcement and a test:
req-f89d5bf2d727— unmet (no valid enforcement location): The PR documents bounded reconciliation, grace windows, restart recovery, operator controls, and conservative handling of missing telemetry, but it is explicitly design-only. No production predicate implements these behaviors, and no actual regression test or fixture exercises them; the pinned negat
Outstanding Human Change Requests
- @joryirving's change request (5ef2d5e, head moved since) is not shown addressed at this head; it needs the reviewer's own re-review.
Verdict set from open findings (verdict_policy=strict): 1 blocker/major finding(s) out of 1 open; model verdict was 'approve'.
2 participants
Summary
Design-only deliverable for #1245: reconcile abandoned claims without stealing work.
Adds
docs/ownership-reconciliation.mdand links it from the README. No implementation, no claim release, no label cleanup, no requeue, no credential migration, no deployment change — matching the issue's non-goals.Review revision (head
086d6c9)Main merged mid-review landed the fenced capacity-exhaustion handoffs (#1259), which shifts code the doc cites and — helpfully — already instantiates part of the §8.7 anchor shape. This revision is markdown-only:
req-f89d5bf2d727). The row for bounded reconciliation / grace periods / restart recovery / operator controls / missing-telemetry-not-abandonment cites §8.1 (bounded single-replica pass), §8.2 (confirmation ladder), §8.3 (grace windows), §8.7 crash/replay matrix, §9 (operator controls), and the §5 rule that missing telemetry is never abandonment evidence. Per the issue's non-goals the design is not implemented here; production predicates and regression tests land in the designated implementation children feat(ownership): persist optional worker renewal and fenced claim evidence #1251 and feat(ownership): reconcile abandoned claims with fenced audit-first release #1254, whose acceptance criteria reference this design.withIssueMutationGuard(src/lib/issue-mutation-guard.ts, persistent guard row unique perissueId) andLease.claimToken(prisma/schema.prisma:903) as the landed anchor machinery the protocol builds on rather than duplicating; §11 item 4 and References updated likewise (docs/capacity-handoff.md).file:linecitations re-pinned against the merged tree (changes from feat(recovery): add fenced capacity-exhaustion handoffs #1259):claim/route.tsaddIssueLabel:206→:222,upsertLease:248→:268-273, fail-closed block:114-143→:118-148, force-claim cleanup:185-192→:193-210;lease.tsTTL:18→:20,resolveActiveWork:155→:181-250,findLeasedIssueIds:226→:255-265;groomer/run.ts:256→:275-280;schema.prismaLease:862-878→:889-910,AgentWork:815-837→:842-864, state/checkpoint enums:795-802/:804-813→:822-829/:831-840,blockedReason:66→:67,linkedPr*:107-113→:108-114. Every remaining citation re-verified line-by-line by the independent reviewer pass at head7c1f723; its two minors (legacy-migration cross-ref §7→§12; "accepted"→"designated" for feat(ownership): persist optional worker renewal and fenced claim evidence #1251/feat(ownership): reconcile abandoned claims with fenced audit-first release #1254) fixed in086d6c9.Review revision (head
6d30488)The human-review P1 on
5ef2d5e— a Postgres CAS cannot fence the external GitHub label deletion, so §8.6's atomic-release claim allowed a stale reconciler'sremoveIssueLabelto strip a same-agent reclaim's fresh label — is settled in this revision, design-only:(repo, issue)Postgres serialization anchor; Phase A fenced intent commit (retires attempt tokeng, opens the barrier, holds any claim on the issue, intent-only variant for legacy rowless claims); Phase B targeted forge delete issued only while a matching intent exists; Phase C settle under the same anchor, lifting the barrier and applying the §8.5 status selection.g+1commit under the anchor before the remoteaddIssueLabel, superseding the current route order (addIssueLabelatclaim/route.ts:222beforeupsertLeaseat:268, post-feat(recovery): add fenced capacity-exhaustion handoffs #1259 pins). Both lock orders resolve (claim-first ⇒ Phase A CAS fails; Phase-A-first ⇒ claim409-held).HELD/needs-human; only an operator clears a stuck barrier.reassignAgentWorkwrites no labels); GitHub delete succeeds / DB settle fails; old-deletion retry vs fresh live token; in-flight claim vs Phase A in both lock orders.main, and independently audited line-by-line by the reviewer at head85f479b.Deliverable contents
agent/*labels,owner/*,status/*,Issue.blockedReason,Lease,AgentWork, claim/unclaim, stale-work sweep, operator release/reassign, PR-fix hand-out ack (fix(pr-fix): reclaim unacknowledged handouts safely #1211), worker identity (Bind worker bearer tokens to agent names (derive actor from token, not self-reported header) #1129), with file/line references and a gap summary.UNOWNED,CLAIMED_ACTIVE,CLAIMED_LONG_RUNNING,REVIEW_OWNED,EXTERNAL_WAIT,HELD,ABANDONED_CANDIDATE,CONFLICTED,RELEASED);HELD/REVIEW_OWNEDterminal,CONFLICTEDoperator-only.register/renew/disposition, attempt-token fencing; explicit safe handling for clients without it.src/lib/github.ts/github-prs.ts), never a forge-specific CLI; fail closed when unreachable.releaseIssueClaim), and the §8.7 two-sided intent protocol.Lease/AgentWork/AgentWorkHistory/PrFixQueueItemgeneration/ack machinery and feat(recovery): add fenced capacity-exhaustion handoffs #1259'sIssueMutationGuard/claimToken.CONFLICTEDsurfaced, explicit operator-declared identity migration.off/audit/enforcemode flag mirroring queue: add opt-in grooming freshness admission before autonomous worker pickup #1065.Invariants this change must keep (and every code path enforcing the same rule)
This is a design doc; the invariants it establishes, and how each existing
agent/*-label writer is accounted for:agent/*label. Enforced by: §8.7 Phase-A CAS + anchor-held claim path (new reconciler);releaseIssueClaim's existing newer-owner skip (src/lib/issue-claim.ts:144-153) — kept and cited; stale sweep (reuses the same core) — folded into the protocol by the implementation tickets.claim/route.ts:222before:268, post-feat(recovery): add fenced capacity-exhaustion handoffs #1259) is explicitly marked as superseded-by-design; no runtime change in this PR. feat(recovery): add fenced capacity-exhaustion handoffs #1259'sIssueMutationGuardis cited as the landed per-issue serialization row the protocol reuses.POST /api/issues/unclaim), maintainer force-claim label cleanup (claim/route.ts:193-210), direct assignment (POST /api/issues/actionsassign_agent).status/ready. Enforced by reusingreleaseIssueClaim's status-preserving policy (src/lib/issue-claim.ts:207-241) — §8.5.agent/courier*aliasing; identity migration is operator-declared and audited. §12; unchanged by this revision.issue-claim.ts:244-249) — §8.6/§8.7 preserve this.Settled decisions
IssueMutationGuard/claimToken; no new lease table or second lock.agent/courier*blanket aliasing; legacy identities are distinct and migration is operator-declared/audited.releaseIssueClaim's status-preserving policy; never forceready.Local validation receipts (coordinator, head
086d6c9, worktree merged with main incl. #1259)npm cinpm run typechecknpm run lintsrc/app/login/page.tsx)npm testRevisions since the merge are markdown-only; no code paths, dependencies, or config touched, so the receipts remain valid.
Independent review outcome
Three adversarial review passes at pinned heads. Round 1 (
8264af5): blockers — claim side still check-then-act; invariant contradicted by exempt direct writers; four minors — all fixed. Round 2 (85f479b): approve; two minors fixed in6d30488. Round 3 (7c1f723, post-#1259 merge): all ~45 citations verified exact against the merged tree, §14 rows confirmed against real section content, requirement 5 of #1245 confirmed traceable; no blockers/majors; its two minors (cross-ref §7→§12, "accepted"→"designated") fixed in086d6c9. Design-only scope confirmed; no implementation introduced.Remaining external verification
CI checks, AI review, and human review re-verification are pending at head
086d6c9— the operator verifies after this run exits. No known unresolved validation failures.Closes #1245.