Skip to content

fix(background): read legacy deadline lane-failure rows without wedging the delegation store - #2792

Merged
zaxbysauce merged 3 commits into
mainfrom
fix/delegation-store-legacy-deadline-read
Sep 15, 2026
Merged

zaxbysauce merged 3 commits into
mainfrom
fix/delegation-store-legacy-deadline-read

Conversation

@zaxbysauce

@zaxbysauce zaxbysauce commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Closes #2791

Summary

Durable delegation records written by pre-#2615 plugin builds carry the retired
workflowLaneFailureClass: 'deadline' value. Both strict readers of the
delegation store (coordinationRowsToDelegations for the SQLite coordination
namespace and foldLedgerTail for the legacy JSONL tail — both validate
through the same RecordSchema) treated that value as corruption: ONE such
row made the whole namespace read return typed uncertainty
(background delegation coordination state is uncertain), which blocked
prepare_pr_workflow_checkout, abort_pr_workflow (agent and human force),
complete_pr_workflow, dispatch_lanes_async batch uniqueness, and
pr_workflow_status in every affected project workspace. Two such rows
(written 2026-08-25, imported verbatim into SQLite 2026-09-06) wedged a live
PR_REVIEW workflow; runtime reproduction on a forensic copy of the affected
store is EXECUTION_PROVEN (fixture with only those two values rewritten reads
all 623 rows ok).

The fix widens only the READ vocabulary — a new
BackgroundDelegationPersistedWorkflowLaneFailureClass (live union +
'deadline') used by ResultSchema's enum and the record interface field —
mirroring the disclosure-side PrReviewDisclosureFailureClass precedent that
#2615 itself established. The live-producer union and its parity test are
untouched; a new negative source anchor forbids any producer of the retired
value; unknown values (anything outside the persisted vocabulary) still fail
the namespace read closed (#2511 preserved). PrReviewLatestTypedFailure
(PR-review completion evidence) widens to the disclosure vocabulary — the one
required downstream type change. coordinationRowsToDelegations now reports
which predicate failed (schema validation vs correlation/generation/status
authority binding) instead of one conflated message. No authoritative SQLite
data is mutated: affected workspaces recover the moment the fixed build loads
(the one-time JSONL shadow reconciliation is the store's designed convergence
path), then /swarm abort-pr-workflow clears a wedged gate.

Invariant audit

  • 1 (plugin init): not touched — no initialization or startup-path changes; bun run build passed.
  • 2 (runtime portability): touched — schema-adjacent source change; verified bun run build, node --input-type=module -e "await import('./dist/index.js')" OK, no new bun:/ usage, plugin shape unchanged.
  • 3 (subprocesses): not touched — no subprocess changes.
  • 4 (.swarm containment): not touched — read-path only within the existing delegation store; no new write sites, no new paths; retention registry re-pinned (bun run check:retention passed, 123 rows).
  • 5 (plan durability): not touched — no plan ledger/projection/checkpoint changes.
  • 6 (test_runner safety): not touched — validation used bounded shell commands and per-file bun test.
  • 7 (test writing): touched — new tests/unit/background/pending-delegations-legacy-deadline-read.test.ts (bun:test, canonicalMkdtemp, closeAllProjectDbs in afterEach, no mock.module; 5 tests, under the 500-line cap) and one new anchor test in the parity suite (string-scanner, no regex transport hazards); check-test-file-cap, check-test-tmpdir, check-test-clock, check-mock-cleanup all report 0 new violations.
  • 8 (session state): not touched — no session-scoped or global state changes.
  • 9 (guardrails/retry): not touched — uncertainty classification and fail-closed propagation unchanged (pinned by C5 and the new suite: unknown value still yields typed uncertainty).
  • 10 (chat/system msg): not touched.
  • 11 (tool registration): not touched — no tool, agent-map, command, or help changes.
  • 12 (release/cache): touched — release fragment docs/releases/pending/delegation-store-legacy-deadline-read.md shipped; no version files hand-edited (release-please owns them); user-facing recovery requires the fixed release to reach the three documented plugin cache layouts, after which /swarm abort-pr-workflow clears wedged gates without any data repair.

Test plan

Frozen acceptance checks (disposable-worktree replay, base 4e45e15; manifest at the issue-tracer trace):

  • C1 DISCRIMINATING: seeded legacy deadline row reads ok through both strict readers — RED LEGACY_READ_FAILED → GREEN LEGACY_READ_OK PASS
  • C2 DISCRIMINATING: value reads back verbatim + SQLite payload bytes unchanged — RED VALUE_NOT_PRESERVED → GREEN VALUE_PRESERVED PASS
  • C3 PRESERVING: lane-failure-class parity suite green at base and head (now 9 tests incl. the no-producer anchor) PASS
  • C4 PRESERVING: no deadline producer literal in src/ PASS
  • C5 PRESERVING: unknown value 'bogus' still fails the namespace closed (UNKNOWN_UNCERTAIN) PASS
  • C6 DISCRIMINATING: new regression suite — RED MISSING: → GREEN REGRESSION_SUITE_GREEN PASS
  • C7 PRESERVING: sqlite-authority + parity suites unchanged PASS
  • repro-check.sh verify-checkpoint: all 5 frozen blobs OK.

Local commands (exit 0 unless noted): bun test tests/unit/background/pending-delegations-legacy-deadline-read.test.ts (5 pass), bun test tests/unit/pr-review/lane-failure-class-parity.test.ts (9 pass), bun test tests/unit/background/pending-delegations-sqlite-authority.test.ts, bun test tests/unit/scripts/retention-registry-rows.test.ts (17 pass), per-file sweep of 10 sibling suites (completion/circuit/authority/uncertainty/exactly-once/generation — all pass), bun run typecheck, bun run build, dist ESM import, bunx @biomejs/biome ci src tests scripts, bun run check:invariants, bun run check:registry-citations, bun run check:retention, DRIFT_CHECK_ENFORCE=1 bun run drift:check (after restoring this checkout's pr-standards.yml line endings to blob form; the initial error was a pre-existing CRLF-on-disk artifact, proven absent at base in a fresh worktree), scripts/check-test-file-cap.ts, check-test-tmpdir.sh, check-test-clock.sh, check-mock-cleanup.sh, check:gate-portability, check-bash-portability.sh, check-cross-contamination.sh.

Mutation/falsifiability: seeded a genuine workflowLaneFailureClass: 'deadline' producer into src/pr-review/circuit.ts → the parity negative anchor fails at exactly expect(producers).toEqual([]); restored → 9/9 pass. The frozen RED base legs at 4e45e15 are the enum-removal mutation proof.

Independent gates: fresh-context implementation reviewer (Phase 4.5) and final critic (Phase 4.6) verdicts recorded in the issue-tracer trace; this PR is published at merge state AWAITING_USER_APPROVAL (merge only on separately recorded user approval).

Review in cubic

@github-actions

Copy link
Copy Markdown
Contributor

Drift check report

Found 2 drift finding(s): 0 error, 0 warning, 2 notice.

required-check-contract (2)

  • 🔵 notice scripts/required-check-contract.json: [RULESET_DIVERGENCE] intended-required context "drift" is not yet required by the captured ruleset
  • 🔵 notice scripts/required-check-contract.json: [RULESET_DIVERGENCE] intended-required context "drift" is not present for every expected event in captured external workflow evidence

Copilot AI 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.

🔵 Needs a closer look

It modifies the authoritative delegation-store schema and fail-closed uncertainty contract that gates every PR workflow, so the high-stakes coordination surface warrants final human review despite no defects being found.

Pull request overview

This PR fixes a critical wedging bug (issue #2791) in the background delegation store. Durable delegation records written by pre-#2615 plugin builds carry the retired workflowLaneFailureClass: 'deadline' value. Because both strict readers (coordinationRowsToDelegations for the SQLite coordination namespace and foldLedgerTail for the legacy JSONL tail) validate every row through the same RecordSchema, a single legacy row caused the whole namespace read to return typed uncertainty — fail-closing prepare_pr_workflow_checkout, abort_pr_workflow, complete_pr_workflow, dispatch_lanes_async batch uniqueness, and pr_workflow_status across the affected workspace with no operator escape. The fix widens only the read vocabulary to admit 'deadline', mirroring the disclosure-side precedent established by #2615, while keeping the live-producer union closed and preserving fail-closed behavior for genuinely unknown values (#2511).

Changes:

  • Adds BackgroundDelegationLegacyWorkflowLaneFailureClass/BackgroundDelegationPersistedWorkflowLaneFailureClass types and widens the ResultSchema enum + record field to accept the retired 'deadline' member (read-only); a new parity anchor forbids any 'deadline' producer in src/.
  • Splits coordinationRowsToDelegations' conflated failure message into distinct schema-validation vs authority-binding (correlation/generation/status) diagnostics, and widens PrReviewLatestTypedFailure.failureClass to the disclosure vocabulary.
  • Adds a regression suite, re-pins retention-registry citations for the shifted line numbers, and ships a release fragment.
File summaries
File Description
src/background/pending-delegations.ts Adds persisted-vocabulary types, widens ResultSchema enum to include 'deadline', and splits row-validation diagnostics into distinct schema/authority messages.
src/pr-review/completion.ts Widens PrReviewLatestTypedFailure.failureClass to PrReviewDisclosureFailureClass so legacy 'deadline' records flow through completion evidence.
tests/unit/background/pending-delegations-legacy-deadline-read.test.ts New suite proving both readers accept a legacy 'deadline' row verbatim, unknown values still fail closed, and the shadow projection converges without rewriting SQLite.
tests/unit/pr-review/lane-failure-class-parity.test.ts Adds a source-scan anchor asserting no 'deadline' producer exists anywhere in src/.
scripts/retention-registry.data.ts Re-pins delegation-store reader/writer citation line numbers (+43) to track the source shift.
docs/releases/pending/delegation-store-legacy-deadline-read.md Release fragment describing the read-compatibility fix and recovery path.

I verified the change end-to-end: the widened enum keeps the compile-time key-based parity guard satisfied; the PR_REVIEW_FAILURE_CLASS_SAFE_DETAILS map already keys 'deadline' (completion.ts:504) so no runtime undefined access is introduced; no downstream consumer performs an exhaustive switch on the field; foldLedgerTail shares the same RecordSchema, so both the strict and lenient paths converge correctly; the retention citations resolve to their intended definitions; and the new parity scanner produces no false positives against src/ (no source line contains both the key and 'deadline'). I found no blocking or notable defects.

Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 0
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@zaxbysauce

Copy link
Copy Markdown
Collaborator Author

Lane R01 — correctness / logic review (bound head 8147c136)

Verdict: FINDINGS — 1 LOW (test-coverage, in the PR's new guard, not in the fix).
The wedge mechanism and the fix are verified sound: the failure reproduces by code path at base, the fix sits at the single schema chokepoint that every durable reader shares, every consumer of the widened value was audited, and the new test file passes 5/5 at the bound head.


1. Wedge mechanism — CONFIRMED at base

Step Base evidence
Enum rejects the durable value src/background/pending-delegations.ts:612.enum(['contract', 'resource', 'liveness']) inside the .strict() ResultSchema consumed by RecordSchema
Whole namespace read fails coordinationRowsToDelegations validates each row with RecordSchema and threw the conflated delegation coordination row failed schema or authority binding validation for <key> (base:1120); the catch calls recordLedgerUncertainty(..., 'coordination-read') and returns null
null becomes typed uncertainty readDelegationLoadAttempt{status:'uncertain', source:'coordination-read'}; readDelegationsDetailed retries once then returns it; scanDelegationsForRecovery does the same
All-or-nothing, not one-row-degraded the row map() throws on the first bad row, so a single legacy row hides every healthy row

Proven empirically at head, not just by reading: the new suite's bogus case keeps a healthy row in the store and still gets uncertain from both strict readers.

Consumers that refuse on that uncertainty (all verified): prepare-pr-workflow-checkout.ts:664-669, pr-workflow-gate.ts:3966-3971 (abort_pr_workflow, agent + human force), pr-workflow-gate.ts:12422-12426 (complete_pr_workflow), dispatch-lanes.ts:1290-1301 (dispatch_lanes_async batch uniqueness), pr-workflow-status.ts:334-341.

2. The fix is at the right chokepoint — no second schema still rejects 'deadline'

Widening ResultSchema's enum (head:640) + the record field (head:484, BackgroundDelegationPersistedWorkflowLaneFailureClass) covers every durable validator, because they all route through ResultSchema/RecordSchema:

  • SQLite coordination rows :1138 · legacy JSONL fold :1912 · checkpoint records :1430-1431 · fallback artifact :1041 · compaction :2488 · closed-summary projection :2233.

Strict mode keeps its documented invalid record at line N uncertainty for anything outside the persisted vocabulary, and lenient mode keeps skipping (the run below still logs the lenient CRITICAL-WARN for the unknown value only).

3. Premise check — durable 'deadline' rows are real, and it is the only retired member

Era audit of writers: 4e948c0f2 introduced contract|resource|deadline with a live producer; 6e58044b5 (#2381, “make PR-review collection a non-destructive observer”) deleted the wait-deadline terminalizer and states in its own message that this removed “the only producer of workflowLaneFailureClass: 'deadline'”; 283b01977 (#2615) then replaced the member with 'liveness' in both the union and ResultSchema. The possible durable value set is therefore exactly contract|resource|deadline|liveness — the vocabulary this PR declares. Every literal producer in src today is contract/resource/liveness (grep + the unchanged parity anchor). No other member was ever retired, so nothing else can be hiding in existing stores.

4. Every reader of .workflowLaneFailureClass — audited (7 sites)

circuit.ts:246 (=== 'liveness') · pr-workflow-gate.ts:1808-1809 (=== 'liveness') · pending-delegations.ts:3685-3687 (=== 'liveness') · :3847 (verbatim equality in sameRetainedResult) · completion.ts:809 (=== 'contract') · completion.ts:909 (leaf propagation) · dispatch-lanes.ts:4919-4922 (re-emission into the tool-output field).

No record/map index, exhaustive switch, or assertNever is keyed by the value (the repo's only assertNever is in tools/secretscan.ts, unrelated). The two map indexings that do take a failure class are total over the 4-member disclosure union: PR_REVIEW_FAILURE_CLASS_SAFE_DETAILS (completion.ts:497-507), used at :634 and :1192-1194. PrReviewLatestTypedFailure.failureClass (:865) is PrReviewDisclosureFailureClass, extensionally identical to the widened record vocabulary (:369-371), so no value outside the consumer's assumed set can flow; its consumers are the disclosure enum (:515, accepts deadline), the disclosure write, and the safe-detail map (write-pr-review-artifact.ts:820 is an unvalidated JSON field). The tool-output field (dispatch-lanes.ts:811) widens consistently with the record it is derived from (:4921), and no docs/skills/tool metadata enumerate a closed 3-member list (workflow_lane_failure_class in docs/ + skills/: 0 hits).

5. New split throw sites — diagnostics only, no behavior change

The four new throws preserve the old short-circuit order (schema → correlationId → generation → status), all sit inside the same try/catch, so the all-or-nothing return and the uncertain classification are identical; parsed.data is narrowed after the !success guard (no undefined access); no partial result escapes; no error is swallowed. The new messages never reach the published reason — that path substitutes the fixed string background delegation coordination state is unreadable or over-bound — so only the health-artifact reason changes. Stale-matcher sweep: the retired text now exists only in two different namespaces untouched here (:5838 reservation rows, pr-subscriptions.ts:618), zero hits in tests/, scripts/, docs/.

6. Reads gained no writes; the newly-unblocked maintenance path is convergent

synchronizeCoordinationAfterCompaction (:2452-2509) re-serializes JSON.stringify(record) from the retention projection only on a genuine isDeepStrictEqual mismatch (:2494-2504) — same compare-and-swap shape as before, now reachable for a legacy row instead of failing closed on it. The checkpoint/compaction projection preserves the class: dropTerminalResultBody (:2268-2273) → dropResultBody (:2256-2266) strips only text/error/outputPreviewChars. The only write near the read path is the designed JSONL shadow reconciliation (:1198-1208 + projection marker).

7. Empirical run at the bound head

bun test tests/unit/background/pending-delegations-legacy-deadline-read.test.ts
→ 5 pass / 0 fail (22 expect() calls)

Proves: a legacy deadline row reads ok through readDelegationsDetailed and scanDelegationsForRecovery with all rows returned; the value is read verbatim and the authoritative SQLite payload bytes are unchanged; an unknown value still fails both strict reads closed (#2511 preserved) while the lenient reader skips it; the JSONL deadline tail is accepted by the strict scan; the shadow projection converges once and stays byte-stable.

8. Finding — LOW, test-coverage (tests/unit/pr-review/lane-failure-class-parity.test.ts:146)

The new negative anchor that forbids any producer of the retired value is line-based (isDeadlineProducerLine applied per split('\n') line), while the sibling live-producer anchor at :116 uses a newline-tolerant /workflowLaneFailureClass\s*[:=]\s*'x'/ regex over whole-file text — so the comment's “same [:=] form” claim does not hold. Replaying both matchers on five producer shapes:

producer shape line guard live-anchor regex
workflowLaneFailureClass: 'deadline', true true
x.workflowLaneFailureClass = 'deadline'; true true
workflowLaneFailureClass:'deadline', false true
x.workflowLaneFailureClass ='deadline'; false true
nested object, value on next line false true

A producer committed with the value on the following line would slip past the regression guard. Suggested fix: test each file's whole text with the same \s*-tolerant regex (or join first and scan once) instead of scanning line by line.

Not verified (honest limits)

  • No RED execution against the base build — read-only tree, no base worktree. The base failure is established by code path (base:612RecordSchema.safeParsebase:1120 throw → null → typed uncertain) plus the head suite's bogus case, which exercises exactly the branch 'deadline' hit at base.
  • No observability data proving a real workspace hit the wedge; only that the durable shape is representable by historical writers and that every reader path fails the whole namespace closed on it.
  • Acceptance criterion 6 (existing delegation / PR-review suites unchanged) was not exercised, per lane instruction not to run the full suite.

@zaxbysauce

Copy link
Copy Markdown
Collaborator Author

🤖 Multi-Stage PR Review

Pipeline: MiniMax-M2.7-highspeed (orientation) (context pack) → MiniMax-M2.7-highspeed (explorer) + MiniMax-M2.7-highspeed (explorer B) (parallel explore, distinct lenses) → GLM-5-turbo (critique) ↔ GLM-5-turbo (critique) (cross-critique) → MiniMax-M2.7-highspeed (fallback arbiter) (arbiter: blind-spot + synthesize)
Commit reviewed: 8147c136c8c1


🔍 PR Intent

Reconstructed obligation list (from PR text, issue, commits, changed tests, changed docs).

  • O-001 Accept durable 'deadline' rows in the SQLite coordination reader (coordinationRowsToDelegations + RecordSchema enum)
  • O-002 Accept durable 'deadline' rows in the legacy JSONL fold (foldLedgerTail, same RecordSchema)
  • O-003 Keep unknown failure-class values (outside persisted vocabulary) failing the namespace read closed ([Workstream H] PR 06 of 15: Preserve delegation-read uncertainty and make workflow recovery status truthful #2511)
  • O-004 Live-producer vocabulary stays closed (parity test anchors; no 'deadline' producer allowed)
  • O-005 PrReviewLatestTypedFailure.failureClass widened to disclosure vocabulary for downstream consumers
  • O-006 Authoritative SQLite rows never rewritten (verbatim read, shadow projection only)
  • O-007 JSONL shadow projection converges once; byte-stable on re-read
  • O-008 Report distinct predicates for coordination-row failures (schema validation vs authority binding)
  • O-009 Regression test suite for all above behaviors

📦 Implementation Summary

The PR widens the read vocabulary of ResultSchema.workflowLaneFailureClass from z.enum(['contract','resource','liveness']) to include 'deadline', introduces BackgroundDelegationPersistedWorkflowLaneFailureClass (live union + 'deadline') as the type of the record interface field, and uses PrReviewDisclosureFailureClass for PrReviewLatestTypedFailure.failureClass. The coordinationRowsToDelegations guard is split into four separate throw branches with distinct diagnostic messages replacing the prior single conflated message. A new regression test suite and a negative source-anchor test in the parity suite are added.


✅ / ⚠️ / ❌ Intended vs Actual

Obligation Status Evidence (file:line)
O-001 SUPPORTED pending-delegations.ts:635 — enum widened to ['contract','resource','liveness','deadline']
O-002 SUPPORTED Same RecordSchema at pending-delegations.ts:635; same schema validates both readers
O-003 SUPPORTED pending-delegations-legacy-deadline-read.test.ts:140–150'bogus' still yields uncertain
O-004 SUPPORTED lane-failure-class-parity.test.ts:122–170 — string-scanner filter finds zero producer files
O-005 SUPPORTED completion.ts:865failureClass: PrReviewDisclosureFailureClass with doc comment
O-006 SUPPORTED pending-delegations-legacy-deadline-read.test.ts:117–121 — SHA confirms payload unchanged
O-007 SUPPORTED pending-delegations-legacy-deadline-read.test.ts:153–180 — first read projects; second read is byte-identical
O-008 SUPPORTED pending-delegations.ts:1141–1163 — four distinct throw messages, one per predicate
O-009 SUPPORTED pending-delegations-legacy-deadline-read.test.ts — 5 tests, SQLite + JSONL, shadow + unknown-value cases

🚨 Confirmed Findings

[MEDIUM] Dead code: Math.max(…, 1) guard is provably redundant after the ?? 1 default on the same expression

  • Location: src/background/pending-delegations.ts:1151
  • Why it matters: The outer Math.max(x, 1) where x = parsed.data.generation ?? 1 is always ≥ 1 — the ?? 1 already guarantees the floor. Math.max(x, 1) simplifies to x unconditionally. This is not a functional bug, but it signals either a misunderstanding of the guard's intent or a residual from a prior refactor.
  • Evidence: src/background/pending-delegations.ts:1151Math.max(parsed.data.generation ?? 1, 1) !== row.generation
    • parsed.data.generation ?? 1 evaluates to a number ≥ 1 (or undefined replaced by 1).
    • Math.max(n, 1) where n ≥ 1 always returns n.
    • Therefore the outer Math.max is a pure no-op.
  • Fix direction: Replace with parsed.data.generation ?? 1 !== row.generation, or if a floor-1 guard is genuinely needed for values < 1 (not just undefined), move it to the nullish coalescing side: (parsed.data.generation ?? 1) !== row.generation then separately assert parsed.data.generation >= 1 — or document the intent if the double guard was intentional. Simpler: just drop the Math.max.

🔬 Unverified but Plausible Risks

  • Risk: PrReviewLatestTypedFailure flows to downstream consumers (e.g., reporting, audit) that may switch on failureClass using the narrower BackgroundDelegationWorkflowLaneFailureClass type at the call site.
    • Why suspicious: The type at completion.ts:859 is now PrReviewDisclosureFailureClass, which includes 'deadline'; a TypeScript switch with no deadline arm would be a compile error, but a JavaScript consumer or a type-assertion (as never) could silently misbehave.
    • What would verify it: Search all callers of PrReviewLatestTypedFailure for runtime switch logic on failureClass.

🧪 Test / Coverage Gaps

  • Gap: The new distinct error messages ("failed schema validation", "authority binding mismatch: correlationId", "authority binding mismatch: generation", "authority binding mismatch: status") have no test asserting the specific message text.
    • Evidence: coordinationRowsToDelegations at pending-delegations.ts:1141–1163; the test suite exercises status === 'uncertain' but never asserts the thrown Error message.

📋 Shipped-vs-Claimed Gaps

None — the PR's core claim (legacy 'deadline' rows read ok, unknown values stay uncertain, producers stay closed) is fully backed by the code and test suite.


📝 Merge Recommendation

[APPROVE_WITH_FIXES]

The PR correctly fixes the core wedged-delegation-store bug (O-001–O-003) and its supporting machinery (O-004–O-009). The dead-code Math.max redundancy is the only non-nit finding; it is fixable in one line and does not block merge.

Check Result
No CRITICAL findings
No unresolved STEALTH_CHANGE
No UNSUPPORTED obligations
Test coverage adequate ✅ (one minor gap: error-message specificity not asserted)
No hardcoded secrets
All async errors handled
Input validation present
No broken agent role boundaries
Prompt format contracts intact
Lockfile consistent

🔁 Validation provenance

Confirmed findings carried to review:

  • [MEDIUM] Math.max redundancyKEPT; evidence at pending-delegations.ts:1151 is structurally proven (see proof above). Not a runtime bug but real dead code.

Confirmed findings DROPPED (with reason):

  • [HIGH] Error message contract breakDROPPED: the original code threw on every failure with one conflated message; callers pattern-matching that string already failed regardless of which predicate failed. The new distinct messages are strictly more informative; no caller was relying on the conflated form.
  • [MEDIUM] Type widening for PrReviewLatestTypedFailureDROPPED: explicitly intentional and documented (completion.ts:859 doc comment), matching the established PrReviewDisclosureFailureClass precedent the PR describes. TypeScript would catch any structural consumer issue at compile time.
  • [LOW] Missing try-catch for new throwsDROPPED: the function threw before and after; nothing was relying on a non-throwing codepath.
  • [LOW] Entity key error disclosureDROPPED: entityKey is a correlationId/sessionId (opaque technical identifiers), not user data. Pre-existing pattern in the file (the original conflated throw already included it).
  • [MEDIUM] Blocking I/O in testDROPPED: fs.readFileSync in tests is a pre-existing convention in this file and the codebase. The same concern applies to every other test in lane-failure-class-parity.test.ts (e.g., readSource at line 52), making it a pre-existing pattern rather than a PR-introduced defect.

Blind-spot findings:

  • Error message text is not asserted in tests (moved to Minor).
  • No downstream runtime switch on PrReviewLatestTypedFailure.failureClass found in the diff context; plausible risk noted above but unverifiable without the full caller graph.

🔒 Reviewed by a 3-model cross-family adversarial debate (architect → dual-lens parallel explorers → cross-critique → arbiter) for high recall with low false-positive noise. Findings are advisory — verify before acting.

@zaxbysauce

Copy link
Copy Markdown
Collaborator Author

Review Synthesis — PR #2792 (run pr2792-review-20260915-051012)

Head reviewed: 8147c13 · base 4e45e15

Coverage attestation

8 lanes (6 base + 2 consolidated micro covering 7 matched families), all 11
risk families attested (7 matched with rows, 4 NOT_TRIGGERED with absence
evidence in trigger-ledger.md). WIRING/RUNTIME/SEC lanes: fix confirmed sound;
widened value flows only into consumers that accept it; strict() retained;
fail-closed preserved.

Verified findings (post-critic)

ID Severity (final) Location Finding Provenance
PRR-001 HIGH (survived 2 critic passes) src/utils/atomic-write.ts:191 WRITER_CLASSIFICATION pin :1458 stale (construction at :1501 after +43 shift); atomic-write-ratchet fails on 3 CI shards; unit-passed red → merge blocked TIF-1 + CI logs + reviewer 0.97 + critic×2
PRR-004 MEDIUM pending-delegations-legacy-deadline-read.test.ts 'bogus' tests assert uncertain but not the health-artifact lastUncertainty consequence TST-002 + reviewer 0.85
PRR-005 MEDIUM pending-delegations.ts:1141-1163 four split throw messages have zero text assertions; silent re-conflation possible TST-003/BOT-GAP-1 + reviewer 0.88
PRR-006 MEDIUM (pre-existing, same-class) retention-registry.data.ts:619,629-633 stale prose/range citations (:971, :82-95, :4508-4641) adjacent to this PR's re-pins; empirically wrong at base AND head DOC-001/002/003 + reviewer 0.90
PRR-002 LOW (downgraded from HIGH by critic) lane-failure-class-parity.test.ts:146-170 negative anchor line-based; two-line producer escapes while live-anchor regex matches; comment's "same [:=] form" claim false across newlines TST-001/R01-LOW-1 + reviewer 0.92 + critic
PRR-007 LOW (pre-existing) pending-delegations.ts:1174-1178 coordination-read reason path unbounded (fold path bounds at 200 chars); entity-key-only content POB-1 + reviewer 0.82
PRR-012 LOW docs/releases/pending/delegation-store-legacy-deadline-read.md only unprefixed fragment in the directory DOC-004 + reviewer 0.96

Rejected candidates (transparency)

  • PRR-003 (AC5 623-row shape not committed): REJECTED by critic — ran the
    shipped tests against BASE source in a worktree: 3 positive tests RED
    ('uncertain'), 5/5 GREEN at head → AC5's causal RED→GREEN is proven from
    the repo; row count causally inert; real rows carry live session ids that
    must not be committed.
  • PRR-008 (shadow write window): pre-existing, untouched.
  • PRR-009 (closeAllProjectDbs): established convention (8 sibling suites).
  • PRR-010 (writer-cannot-produce cross-check): covered by cross-file anchor.
  • PRR-011 (source discriminator): pinned by 3 existing suites.
  • PRR-013 (Math.max redundant): pre-existing, relocated verbatim (bot finding).
  • BOT-RISK-1 (narrow switches): disproven (all consumers === or total maps).

Obligation check

All 9 obligations (O-001..O-009 per the multi-stage bot's reconstruction)
SUPPORTED at head; PR body claims trace to evidence; the one external
correction: my PR body called the anchor's scanner "same [:=] form" as the
live anchor — false across newlines (fixed with PRR-002).

Verdict: REQUEST_CHANGES

Load-bearing: PRR-001 (CI-blocking, deterministic). All findings are
test/docs/registry-pins class; the fix mechanism itself is confirmed sound by
every lane and by two prior fresh-context gates.

@zaxbysauce

Copy link
Copy Markdown
Collaborator Author

Closure Ledger — PR #2792 feedback round (run pr2792-review-20260915-051012)

Fix commit: 7480b00 · sync-merge: 47ff6a0 (origin/main 2b51abf)

id source item outcome fix-ref evidence
PRR-001 CI (3 unit shards + unit-passed red) + lanes + critic×2 WRITER_CLASSIFICATION pin :1458 stale after +43-line shift FIXED 7480b00 re-pinned to :1501 in src/utils/atomic-write.ts; bun test tests/unit/utils/atomic-write-ratchet.test.ts 7/7 pass (was 6/1 fail)
PRR-002 TST-001 + R01-LOW-1 + critic (downgraded HIGH→LOW) negative anchor line-based; two-line producer escaped FIXED 7480b00 whole-text newline-tolerant isDeadlineProducerText (===/!==/=> rejected); mutation probe: seeded two-line producer FAILS the anchor, restored = green; parity suite 9/9
PRR-003 OBL lane + reviewer AC5 623-row shape not reproducible from repo REJECTED (critic pass 1) critic ran shipped tests against BASE in a worktree: 3 positive tests RED ('uncertain'), 5/5 GREEN at head — causal RED→GREEN proven from repo; row count causally inert; real session ids must not be committed
PRR-004 TST-002 + reviewer no health-artifact assertion for bogus case FIXED 7480b00 bogus test asserts lastUncertainty.reason contains 'failed schema validation for ' + source 'coordination-read'
PRR-005 TST-003 + reviewer (bot gap) split messages lack text assertions FIXED 7480b00 new test seeds correlationId≠entity_key via raw SQL; asserts health reason contains 'authority binding mismatch' + key
PRR-006 DOC lanes + reviewer 3 pre-existing stale registry prose/range citations FIXED (same-class completion) 7480b00 schemaVersion :971→:1038; writeLimits →(:102/:118/:103/:5772, enforcement :5866/:5428); citation :102-118,5866; readBound :5372-5470; lockModel :174-175; each target verified to contain the named symbol at HEAD
PRR-007 POB-1 + reviewer coordination-read reason path unbounded PRE_EXISTING (residual, no fix) catch shape verbatim at base 4e45e15:1125-1145; content entity-key-only (pseudonymous); bounding change = behavior scope expansion beyond minimal patch
PRR-008 RUN-001 + reviewer shadow write non-atomic window REJECTED (pre-existing, untouched) function byte-identical base↔head
PRR-009 RUN-002 + reviewer closeAllProjectDbs afterEach REJECTED (convention) 8 sibling suites use the identical pattern
PRR-010 TST-004 + reviewer writer-cannot-produce not cross-verified REJECTED (covered) cross-file source anchor (this PR's own test) guards the property repo-wide
PRR-011 TST-007 + reviewer source discriminator unasserted REJECTED (covered) pinned by read-uncertainty-2511/v3/checkpoint-crash suites
PRR-012 DOC-004 + reviewer fragment unprefixed FIXED 7480b00 git mv → docs/releases/pending/2791-delegation-store-legacy-deadline-read.md (100% rename)
PRR-013 multi-stage bot Math.max(…,1) redundant REJECTED (pre-existing, relocated verbatim) expression exists at base :1116; S1 split preserved it
SYNC base drift main advanced (PR #2783 merged; shared registry + atomic-write files) FIXED 47ff6a0 clean merge (no conflicts); full citation battery re-run green post-merge
CI-CI-1 gh pr checks unit macos-5/ubuntu-6/windows-4 + unit-passed red DUPLICATE of PRR-001 7480b00 same root cause; local ratchet green

Post-fix validation (orchestrator, at 47ff6a0)

  • 48/48 tests across ratchet + retention-rows + legacy-deadline + parity + sqlite-authority
  • bun run typecheck exit 0 · check:registry-citations pass · check:retention 123 rows pass · DRIFT_CHECK_ENFORCE=1 drift:check 0 errors (2 pre-promotion notices)
  • Mutation probe on hardened anchor: two-line seeded producer → anchor FAILS; restored → 9/9

Gates

  • Fresh swarm-reviewer on fix diff: (pending → recorded in gates.md)
  • Final swarm-critic: (pending → recorded in gates.md)

@zaxbysauce
zaxbysauce added this pull request to the merge queue Sep 15, 2026
@zaxbysauce

Copy link
Copy Markdown
Collaborator Author

🤖 Multi-Stage PR Review

Pipeline: MiniMax-M2.7-highspeed (orientation) (context pack) → MiniMax-M2.7-highspeed (explorer) + MiniMax-M2.7-highspeed (explorer B) (parallel explore, distinct lenses) → GLM-5-turbo (critique) ↔ GLM-5-turbo (critique) (cross-critique) → MiniMax-M2.7-highspeed (fallback arbiter) (arbiter: blind-spot + synthesize)
Commit reviewed: 47ff6a0b739d


PR Reviewer — opencode-swarm

🔍 PR Intent

Reconstructed obligation list:

  • O-001 — Widened ResultSchema.workflowLaneFailureClass enum to admit 'deadline' so strict SQLite and JSONL readers accept pre-[Workstream H] PR 12 of 15: Typed terminal failure class and completion admission for accepted-then-dead, stale and cancelled PR-review lanes #2615 durable rows without reporting uncertainty
  • O-002 — Widened BackgroundDelegationResult.workflowLaneFailureClass interface field type to persisted vocabulary (mirrors schema)
  • O-003 — Widened PrReviewLatestTypedFailure.failureClass to disclosure vocabulary (PrReviewDisclosureFailureClass) since record-side reads feed it for eventless terminal lanes
  • O-004 — Split the conflated coordinationRowsToDelegations error message into distinct predicates (schema validation vs authority binding mismatch) with row key retained in both
  • O-005 — Added negative source anchor in parity test so no producer of 'deadline' can reappear (live-producer vocabulary stays closed)
  • O-006 — New regression suite covering legacy deadline read, unknown-value fail-closed, authority-binding distinct predicate, JSONL fold, and shadow projection convergence/stability
  • O-007 — Unknown failure-class values still fail the namespace read closed (preserves [Workstream H] PR 06 of 15: Preserve delegation-read uncertainty and make workflow recovery status truthful #2511)
  • O-008 — SQLite authoritative payload bytes never mutated by a read (shadow projection is the designed convergence path, not in-place rewrite)

📦 Implementation Summary

The PR makes four targeted changes: (1) adds two new type aliases for the persisted failure-class vocabulary and exports them; (2) widens ResultSchema.workflowLaneFailureClass enum from ['contract','resource','liveness'] to ['contract','resource','liveness','deadline'] and updates the matching interface field; (3) widens PrReviewLatestTypedFailure.failureClass from the producer union to PrReviewDisclosureFailureClass; (4) splits the single multi-condition check in coordinationRowsToDelegations into four separate early-throw branches with distinct, row-key-inclusive messages. A new test suite covers legacy-read, fail-closed, predicate-split, JSONL fold, and shadow stability; the parity suite gains a string-scan negative anchor.


✅ / ⚠️ / ❌ Intended vs Actual

Obligation Status Evidence (file:line)
O-001 SUPPORTED src/background/pending-delegations.ts:639 — enum widened to include 'deadline'
O-002 SUPPORTED src/background/pending-delegations.ts:479 — field type uses BackgroundDelegationPersistedWorkflowLaneFailureClass
O-003 SUPPORTED src/pr-review/completion.ts:864 — field type widened to PrReviewDisclosureFailureClass
O-004 SUPPORTED src/background/pending-delegations.ts:1138–1166 — four distinct throw branches
O-005 SUPPORTED tests/unit/pr-review/lane-failure-class-parity.test.ts:128–170isDeadlineProducerText string scan with expect(producers).toEqual([])
O-006 SUPPORTED tests/unit/background/pending-delegations-legacy-deadline-read.test.ts — 5 tests covering all stated scenarios
O-007 SUPPORTED tests/unit/background/pending-delegations-legacy-deadline-read.test.ts:180–207'bogus' yields uncertain
O-008 SUPPORTED tests/unit/background/pending-delegations-legacy-deadline-read.test.ts:157–158 — SHA comparison proves bytes unchanged

🚨 Confirmed Findings

None. Every confirmed finding failed challenge; no new blind-spot defects were found.


🔬 Unverified but Plausible Risks

None at actionable confidence. All speculative risks are bounded by runtime guards (schema strictness, parity test, source-tree scope) already in place.


🧪 Test / Coverage Gaps

  • Gap: None — 5 legacy-read tests + 9 parity tests cover all changed behaviour paths.
    • Evidence: tests/unit/background/pending-delegations-legacy-deadline-read.test.ts (5 test() blocks); tests/unit/pr-review/lane-failure-class-parity.test.ts (9 it() blocks at head).

📋 Shipped-vs-Claimed Gaps

  • Gap: None — every claimed change maps to a concrete diff line.

📝 Merge Recommendation

[APPROVE]

All eight obligations are supported by diff evidence. The four confirmed findings were each examined and dropped: one was intentional by-design behavior (type widening is the fix), two were speculative about future maintainers bypassing existing guards (the guards are in place and tested), and the remaining ones conflated test-environment constraints with production defects. No concrete, PR-introduced, unmitigated defect was found.

Check Result
No CRITICAL findings
No unresolved STEALTH_CHANGE ✅ — type changes documented in release fragment
No UNSUPPORTED obligations
Test coverage adequate ✅ — 14 targeted tests for 4 changed behaviour surfaces
No hardcoded secrets
All async errors handled ✅ — no new async paths introduced
Input validation present ✅ — ResultSchema strictness preserved, unknown values fail closed
No broken agent role boundaries ✅ — no agent interface changes
Prompt format contracts intact ✅ — no output-format token changes
Lockfile consistent ✅ — package.json unchanged

🔁 Validation provenance

Findings challenged and dropped (1-line rationale each):

  1. [MEDIUM] Math.max false-positive claimMath.max(-999, 1) = 1 ≠ row.generation ≥ 1 so the guard throws correctly; not a defect.
  2. [HIGH] Exhaustive-switch silent fallback (completion.ts)PrReviewLatestTypedFailure is module-internal; no external API surface changed.
  3. [MEDIUM] Cast drops mismatched fieldsz.strict() rejects extra fields; as cast is safe after safeParse success.
  4. [LOW] db.query.get() undefined guard removable — guard is present; finding is speculative about future maintainers.
  5. [HIGH] Infinite recursion / symlink cycle — test file, controlled environment; bun test would catch any cycle fast.
  6. [MEDIUM] Missing I/O try/catch — test file; permission errors propagate as test failures, which is correct.
  7. [LOW] Comment-vs-implementation mismatch — comment is accurate; isSpace loops over newlines between op and quote.
  8. [MEDIUM] Exhaustive-switch silent fallback (BackgroundDelegationResult) — intentional by-design; parity test prevents regression; PR acknowledges it.
  9. [MEDIUM] Schema validates writes accepting 'deadline' — intentional; no live producer can write it (parity test blocks it).
  10. [MEDIUM] Exhaustive-switch silent fallback (PrReviewLatestTypedFailure) — same as Swarm config is completely ignored #2; module-internal, no external API.
  11. [MEDIUM] Symlink traversal outside src/path.join to REPO_ROOT bounds traversal scope regardless of symlinks.
  12. [HIGH] Computed-property producer bypass — TypeScript requires a string literal for object property values; workflowLaneFailureClass can't be a runtime-computed key in a producer context.
  13. [HIGH] Unicode/escape bypass — TypeScript string literals in source code are what the scan targets; an escape \u0064 in source ≠ 'deadline' in source.
  14. [MEDIUM] Template literal bypass — Intentionally out-of-scope per the test's own documented scope ("single- and double-quoted strings").

Blind-spot pass findings added: None.


🔒 Reviewed by a 3-model cross-family adversarial debate (architect → dual-lens parallel explorers → cross-critique → arbiter) for high recall with low false-positive noise. Findings are advisory — verify before acting.

Merged via the queue into main with commit 0c11529 Sep 15, 2026
46 checks passed
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.

Delegation store wholly unreadable when a durable row carries the retired 'deadline' workflowLaneFailureClass — wedges all PR workflows

2 participants