docs(recovery): design PR-fix reactivation on fresh evidence (#1244)

[itsmiso-ai]

Summary

Adds the design deliverable for #1244: a state-transition table and recovery policy for reactivating PR-fix work on fresh evidence, plus the freshness model, ownership fencing, WAITING_EXTERNAL contract, unknown-evidence behavior, negative controls, and a current-behavior-vs-gaps inventory.

Design only. Per the issue's non-goals, this PR changes documentation only: no implementation, automatic bulk requeues, attempt-counter erasure, credential changes, timeout increases, or deployment changes. No schema, route, or budget behavior changes.

What changed

• docs/pr-fix-recovery-policy.md (new) — the design.
• docs/pr-fix-outcome-policy.md — cross-link, with "under current behavior" qualifiers so the proposed reactivation rules don't contradict the existing stickiness contract.
• docs/pr-review-fix-queue.md — cross-link, same qualification.

Key design decisions

• Durable evidence ledger + per-attempt watermark (revised after review). Freshness is computed from forge-authoritative identities (review/comment/check-run ids, head SHA, review state, check conclusion), deduped by (identity, version) in a durable per-item ledger, and scoped by an attemptEvidenceWatermark stamped on each hand-out — what the worker was actually given. A webhook delayed past settlement still records a post-watermark transition and stays fresh; a replay or evicted-then-re-read old observation mints no transition and stays inert. Forge observedAt and ingestion/re-read time are both excluded as freshness sources. disposedAt/recoveryAppliedAt/disposedEvidence remain as audit fields, not freshness predicates.
• Persistent conditions get stable per-condition baselines (revised after review). merge_state/merge_conflict use one durable ledger row per PR (merge_state:<repo>#<pr>, version (state, head)), updated on every observation and independent of any disposition; an unchanged condition re-read never reopens even after bounded-cache eviction and even when the disposition's cause was unrelated evidence. Reopening requires a transition (entry into the actionable state, or the same state at a new head) plus persistence, cooldown, and budget.
• Evidence content signature (anti-repetition). Repeated review text under a new id is recorded but not actionable; already-resolved comments need a new forge read (flagged as a gap).
• Hold classes, not status alone. FIXED (resolved), budget-available PARKED/no_progress (operational, reactivatable), budget-exhausted PARKED (operational, operator-only), fail-closed parks (infra_failure/security_failure), human BLOCKED/maintainer hold, and terminal STALE/IGNORED. Reactivation increments the existing attempt cap so park→reopen→park terminates; cap-exhausted parks set a stale-disposition flag and need an operator.
• WAITING_EXTERNAL must name a concrete dependency (kind, ref, observed evidence, recovery trigger, recheck deadline). Lack of a patch is not an external dependency. New dependency-less waits park fail-closed; legacy waits are flagged, never bulk-downgraded.
• #940 precedence is explicit. On FIXED at an unchanged head the stale-tombstone reopens (row 6) before duplicate/replay suppression; content-signature suppression applies only to repeated-text-under-new-id, and the ledger dedupe (event-driven and periodic paths) must not swallow the net.
• Ownership fencing. Settlement authority is the (id, generation) attempt token; acked leases are never reclaimed; old workers cannot settle newer feedback.
• Event-driven + bounded periodic reconciliation for missed events, with ledger idempotency (including the #940 exception on the periodic path), cooldown, budgets, and audit trails.

Invariants this change must keep

The diff is docs-only; the invariants are claims the document must hold against the code, and every other code path enforcing the same rule:

1. Freshness boundary = attempt snapshot, not disposition time. Kept by the ledger/watermark spec and negative controls 8/17/18; the old observedAt > disposedAt rule is fully removed. Other paths enforcing related rules, kept unchanged and accurately described: postDispatchEvidenceKeys + settlement revalidation (src/lib/pr-fix-queue.ts:1780, :1786, PR-fix evidence that arrives mid-attempt is absorbed and never delivered to the worker #1119) — the doc presents the ledger as their durable generalization, not a replacement of current settlement behavior.
2. Replays are inert. Current identity dedupe uniqueAppend (:306) is documented as the fast path; the doc's ledger extends, not contradicts, it. Kept by control 1/2/18.
3. Sticky holds. PARKED/WAITING_EXTERNAL stickiness (:1128) and BLOCKED/human_decision non-override stay as Current rows; docs/pr-fix-outcome-policy.md and docs/pr-review-fix-queue.md carry the matching "under current behavior" qualifiers.
4. #940 stale-FIXED net. Current code :1149/:1344; kept by row 6, the precedence note, and control 18 both directions.
5. Terminal states inert (:2226, :2294) and settlement fencing (:2712, :2765, :2774) — unchanged Current rows; the reap non-pinning is still flagged as a gap.
6. All src/lib/pr-fix-queue.ts line citations were re-derived against the post-merge tree (the capacity-handoff merge d2786d2 shifted them; it adds no PARKED write site, so the "six of seven are cap-exhaustion" claim re-verified true), and re-verified again against the current merge 9839166 (main through b8141b4, package-lock-only Next 16.4.1 bump — no source or docs impact).

Review outcome

• Human review (joryirving) on 3c6eaa3 requested changes: P1 (delayed webhook during an attempt rejected by observedAt > disposedAt) and P2 (single disposedEvidence cannot baseline timestamp-less condition observations after eviction), plus the #940 naming/precedence ask. All three are addressed in fdb793e as described above (ledger + attempt watermark; stable per-condition ledger identities; explicit #940 precedence), with new negative controls 17 (delayed first discovery must reopen despite submitted_at < disposedAt), 18 (old-known replay vs #940 both directions), and 19 (persistent condition + unrelated disposition + eviction must not reopen, with paired positive). Replies on the threads record the resolution; a follow-up reply on the P1 thread now cites the current attempt-snapshot mechanism (postDispatchEvidenceKeys + settlement revalidation, src/lib/pr-fix-queue.ts:1778-1793) that the ledger generalizes.
• Earlier independent adversarial rounds: R1 3ed675b NO-GO (1 blocker, 6 majors, addressed); R2 08377ae NO-GO (2 new majors, addressed); R3 9c00b02 GO (5 minors fixed in 3c6eaa3).
• New independent adversarial pass on fdb793e (separate agent): GO — 1 major (reconciliation idempotency bullet could swallow the #940 net on the periodic path) + 5 minors (row 7 Proposed marking, watermark overclaim vs workers reading GitHub directly, row-3 notation, Current/Proposed markers, live-item pruning vs condition baselines). All six fixed in 2e96ba3; the review verified all refreshed line citations (30+) with zero misses and confirmed the diff touches only docs/pr-fix-recovery-policy.md.
• AI review (saffron bot) on 2e96ba3: model verdict approve, but verdict_policy=strict set CHANGES_REQUESTED from 4 major requirement-trace findings + 2 minors. The 4 majors each demand production predicates/regression tests for rules design(recovery): reactivate PR fixes on fresh evidence #1244 explicitly excludes ("Non-goals: Design only. No implementation…"); the 2 minors are verbatim replays of the already-answered P1/P2 human threads. Answered in the PR comment thread; no diff change is warranted without violating the issue's non-goals. The outstanding human change request needs joryirving's own re-review (external).

Local validation receipts

• npm run lint — cwd /workspace, revision 9839166 (merge of main b8141b4; docs-only diff vs main): 0 errors, 1 pre-existing warning in src/app/login/page.tsx (unrelated to this change).
• npm run typecheck — cwd /workspace, revision 9839166: passed (tsc --noEmit, exit 0).
• Limitation: no automated test exercises documentation; the diff is docs-only, so lint/typecheck are the applicable local checks. Line citations were verified against the merged tree instead of tests.

Remaining external verification

CI/checks on head 9839166 are pending (not observed from this run) — the PR is left open (non-draft) for operator verification, together with human re-review of the P1/P2 fixes (both threads answered). No code paths are modified, so runtime behavior is unchanged.

Closes #1244.

[joryirving]

Reviewed Dispatch #1248 against issue #1244, the full design-only specification on exact head 3c6eaa3, and the current enqueuePrFixItem/attempt/hand-out/settlement behavior. CI, Build Dispatch Image, Security Audit, PR Smoke, and AI PR Review all passed, and GitHub reports the branch mergeable. The design correctly separates human decisions from operational parks; defines structured external dependencies, generation fencing, immutable history, bounded retries, and the missing reconciliation capability. No production-code scope was added.

REQUEST_CHANGES: two freshness-correctness gaps remain in the design itself. Both are documentation-only fixes.

[P1] A missed or delayed event occurring during the worker attempt is rejected by the observedAt > disposedAt test (docs/pr-fix-recovery-policy.md:149-160,223-241). Counterexample: hand out generation g at 10:00 with evidence snapshot S; a new, actionable CHANGES_REQUESTED review is submitted at 10:05; its webhook is delayed/lost, so Dispatch has not ingested it; the worker reports FIXED/no-change (or WAITING_EXTERNAL) at 10:10 and writes disposedAt=10:10; periodic reconciliation first discovers the 10:05 review at 10:15. Its forge-authoritative submitted_at=10:05 is earlier than disposedAt=10:10, so the spec declares it not fresh, even though it was absent from the attempt's input and landed after the work was dispatched. This permanently strands precisely the post-dispatch/missed-webhook feedback #1244 requires recovery to handle, and conflicts with the policy's own during-execution example (§worked example 1) if the event isn't ingested until after settlement. Freshness needs a durable attempt/evidence snapshot boundary (what the worker actually had at admission/hand-out) plus an acknowledgement/observation history, not only comparison to the eventual disposition timestamp. Design a rule that catches the late-arriving, previously unseen event while rejecting an old already-known review re-read after resolution; add both sides as negative controls. Do not simply replace forge timestamps with ingestion time, which would recreate the replay bug.

[P2] The one-entry disposedEvidence cannot establish freshness for timestamp-less condition observations after eviction (docs/pr-fix-recovery-policy.md:145-163,122-128,223-241). For merge_state and merge_conflict, the document explicitly discards any time comparison and accepts a condition when its (identity,version) differs from the single disposedEvidence. Suppose a persistent dirty merge state was already observed before an attempt, and the later operational PARKED disposition was triggered by review R; the durable record now references R, not dirty. If normal subsequent observations evict the older dirty key from the bounded 40/20-entry caches, periodic reconciliation re-reads the same unchanged, still-persistent dirty condition. It differs from disposedEvidence R, and (after a second re-read) passes persistence, so it can spuriously reopen work on old evidence, repeatedly consuming the attempt cap. Replacing one disposition-reason key does not preserve a baseline of all previously observed conditions. Specify how persistent condition transitions are versioned/baselined across dispositions and cache eviction (e.g. durable per-condition last state/head or an attempt snapshot that includes them); pin a prior-known persistent condition plus unrelated disposition + cache eviction + reconciliation case that must not reopen. This is separate from late webhooks because these event kinds have no forge timestamp at all.

Non-blocking contract clarity: §table row 6 preserves the current #940 known-evidence/same-head FIXED → QUEUED exception, while rows 3/7 declare duplicates no-op. Name the #940 exception and precedence explicitly, or mark that behavior for replacement, so implementation doesn't inadvertently remove the existing stale-FIXED safety net or contradict the new dedupe invariant.

No implementation, new exporter, or extra scheduling change requested. The structure is strong, but #1244 is primarily a correctness design for what counts as fresh; these two cases need to be pinned before it can serve as an implementation contract.

[joryirving]

P1: observedAt > disposedAt drops genuinely new work when a review arrives during a dispatched attempt but the webhook is delayed/lost until after the worker settles. Example: dispatch 10:00, review submitted 10:05, worker FIXED 10:10, periodic reconciliation first sees the review 10:15. submitted_at(10:05) <= disposedAt(10:10) blocks recovery even though the attempt never saw that review. Define freshness relative to the attempt's evidence snapshot/hand-out while also remembering already-processed events, and test delayed first discovery vs an old replay.

[itsmiso-ai]

Fixed in fdb793e. The observedAt > disposedAt rule is removed entirely. Freshness is now transition-existence in a durable per-item evidence ledger, scoped by attemptEvidenceWatermark — the ledger sequence stamped at hand-out (the durable record of what the worker was actually given; for dispositions with no dispatched attempt, the ledger sequence at park time, so nothing pre-park reads as unseen).

Your counterexample is now the spec's own case: the 10:05 review first discovered at 10:15 records its transition after the 10:00 watermark, so it is fresh despite submitted_at < disposedAt. Replays/old known reviews stay inert because re-recording a stored (identity, version) creates no transition and restamps nothing — ingestion/re-read time is explicitly excluded as a freshness source, so the replay bug is not reintroduced. Negative controls: 17 (delayed first discovery must reopen), 18 (old-known replay must not, with the #940 carve-out at FIXED/unchanged head pinned in the same control), plus revised 8. Worked example 1 gained the lost-webhook case, and the reconciliation pass's idempotency bullet is qualified so the periodic path cannot swallow the #940 net.

[itsmiso-ai]

Adding the current-code evidence the later review asked for. The mechanism this design generalizes is the per-attempt evidence snapshot + settlement revalidation in src/lib/pr-fix-queue.ts (line citations re-verified against the post-merge tree at 9839166):

• Hand-out stamps the attempt's evidence keys: postDispatchEvidenceKeys is reset per fresh attempt (:574, :1041) and appended on evidence recorded after dispatch (:1243-1245).
• Settlement revalidates against that snapshot, not a timestamp: reopenOnPostDispatch (:1778-1780) calls hasActionablePostDispatchEvidence on the attempt's keys, and writeReopen pins them under a CAS on postDispatchEvidenceKeys (:1786-1793), with the bounded re-decision fallback (:1781-1784) for keys landing between check and write.

So current behavior already asks "what did this attempt get?", never "when did the record land?" — and the design's ledger + attemptEvidenceWatermark is that same attempt-snapshot boundary made durable and reconciliation-visible. Your P1 counterexample survives as a current-behavior gap in exactly this way: evidence that is only ever recorded after settlement is never reconsulted (there is no periodic reconciliation pass over un-ingested forge events), which is the hole the ledger + bounded periodic reconciliation closes — now pinned by negative control 17 rather than reintroduced via observedAt > disposedAt.

[joryirving]

P2: A single (identity,version) disposedEvidence cannot prevent stale merge_state/merge_conflict conditions from reactivating after the bounded evidence cache evicts their keys. Those conditions lack forge timestamps, so an unchanged condition known before the current disposition passes the != disposedEvidence gate whenever the disposition was caused by a different event. Define a durable per-condition state/head transition baseline or attempt snapshot that survives cache eviction; pin a persistent-known-condition → unrelated park → eviction → re-read no-reopen case.

[itsmiso-ai]

Fixed in fdb793e. The condition kinds no longer consult disposedEvidence at all. Each kind gets one stable ledger identity per PR (merge_state:<repo>#<pr>, merge_conflict:<repo>#<pr>) with version (state, head) — the doc now also names the current identity-from-state-string derivation as the gap that prevented a baseline (confirmed at src/lib/pr-followup-ingestion.ts:747). That ledger row is the durable per-condition baseline: one row per condition kind, updated on every observation (webhook or reconciliation), independent of any disposition, never FIFO-evicted, and excepted from live-item pruning.

So your sequence — dirty known pre-attempt, park caused by unrelated review R, eviction of the old key, reconciliation re-read of unchanged dirty@sameHead — finds the version already in the baseline: no transition, no reopen, regardless of what disposedEvidence references. Transitions that may reopen are only entry into the actionable state or the same state at a new head, still gated by persistence, cooldown, and budget. Pinned as negative control 19 with the paired positive (clean→dirty / dirty-at-new-head). Predicate 4 states explicitly that no single disposition key can serve as the condition baseline.

[itsmiso-ai]

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

Note: requeue from queue page

Updated automatically by Dispatch on 2026-10-14T20:43:38.711Z

[itsmiso-ai]

Run handoff (head 2e96ba3)

Settled decisions (this round addressed the human review on 3c6eaa3):

• Freshness boundary replaced: durable per-item evidence ledger + attemptEvidenceWatermark stamped at hand-out, replacing observedAt > disposedAt; forge observedAt and ingestion time are audit-only, never freshness sources. disposedAt/recoveryAppliedAt/disposedEvidence kept as audit fields only.
• merge_state/merge_conflict given stable per-PR ledger identities with version (state, head) as durable per-condition baselines (updated on every observation, never FIFO-evicted, pruning-excepted), so unchanged conditions cannot reopen after eviction or unrelated dispositions.
• #940 stale-FIXED exception named with explicit precedence over duplicate/replay dedupe (rows 3/6/7, control 18, and the reconciliation idempotency bullet all aligned).
• New negative controls 17 (delayed webhook must reopen despite submitted_at < disposedAt), 18 (old-known replay vs #940, both directions), 19 (persistent condition + unrelated disposition + eviction must not reopen; paired positive).
• All src/lib/pr-fix-queue.ts line citations re-derived against the post-merge tree (branch now includes main 3894ab1; capacity-handoff d2786d2 adds no PARKED write site — the six-of-seven cap-exhaustion claim re-verified).

Local checks (receipts in body): npm run lint 0 errors + 1 pre-existing unrelated warning; npm run typecheck exit 0 — both at 2e96ba3, docs-only diff. Independent adversarial review at fdb793e: GO with 1 major + 5 minors, all fixed in 2e96ba3; citations re-verified by the reviewer with zero misses.

Remaining external verification: CI/checks on 2e96ba3 (not observed from this run — left pending), human re-review of the P1/P2 fixes (threads answered), and PR approval/merge. Both prior review threads answered with the resolution and the controls that pin them.

[itsmiso-ai]

Response to AI review at 2e96ba3

The 4 major "requirement not enforced" findings are not actionable in this PR. Issue #1244 scopes the deliverable to design: "Non-goals: Design only. No implementation, automatic bulk requeues, attempt-counter erasure, credentials, timeout increases, or deployment changes." Each major demands production predicates and regression tests for rules the issue explicitly defers past this deliverable:

• WAITING_EXTERNAL concrete-dependency contract — specified in full in docs/pr-fix-recovery-policy.md (kind, ref, observed evidence, recovery trigger, recheck deadline; lack of a patch is not a dependency; dependency-less waits park fail-closed; legacy waits flagged, never bulk-downgraded). The current unvalidated free-text note + timer is inventoried as a gap, exactly as the issue's "Identify existing behavior versus gaps from current code" requires.
• Operational park vs. genuine human decision — the hold-class taxonomy and non-overridable human/fail-closed holds are specified in the state-transition table and hold-classes sections; current stickiness is kept as accurately-described Current rows.
• Generation/attempt settlement fencing — the existing (id, generation) attempt-token fencing and acked-lease protection are documented against current code, with the recovery-side ownership rules specified as Proposed.
• Structured dependency fields / fail-closed downgrade — same first item: explicitly Proposed, with the current-code gap named.

The requirement-trace rail scoring a design document against production enforcement conflicts with the issue's stated non-goals; the review's own model verdict was approve, and its change-by-change analysis confirms every required deliverable (state-transition table, recovery policy, freshness model, WAITING_EXTERNAL contract, human-vs-operational holds, fencing, non-goal compliance).

The 2 minor findings are verbatim replays of the human review threads (P1 delayed-webhook freshness, P2 persistent-condition baselines). Both were fixed in fdb793e — negative controls 17/18 pin the exact P1 counterexamples (delayed first discovery must reopen; old-known replay must not, with the #940 carve-out), and control 19 pins P2's known-condition → unrelated park → eviction → re-read no-reopen case with a paired positive. Replies on both threads record the resolution.

On the threads' "fixed without evidence citing current code" note: the P2 reply already cites src/lib/pr-followup-ingestion.ts:747 (identity-from-state-string derivation — the gap that made a per-condition baseline impossible), and a follow-up reply on the P1 thread now cites the current freshness mechanism (postDispatchEvidenceKeys + settlement revalidation at src/lib/pr-fix-queue.ts:1778-1793), which the ledger + attemptEvidenceWatermark durably generalizes.

Outstanding human change request (joryirving, at 3c6eaa3): both threads answered with the resolution and the controls that pin them; the reviewer's own re-review is external and left to the operator.

Head is now 9839166 — merge of main through b8141b4 (package-lock-only Next 16.4.1 bump; no docs or code impact). Local validation re-run at that head: npm run lint 0 errors + 1 pre-existing unrelated warning (src/app/login/page.tsx); npm run typecheck exit 0. Diff remains docs-only.

Superseded by a newer automated review for this pull request.

[its-saffron]

AI Automated Review

Full PR review.

Analysis engine: (model unavailable) · pr-reviewer-action v3.4.1

AI review could not run

The configured model endpoint(s) did not return a usable review for this run (reason: Fallback model failed).

This is an automated notice, not a substantive review. Re-run the workflow once the endpoint is reachable; see the action logs for the underlying error.

[joryirving]

Review of exact head 9839166 against Dispatch #1244: APPROVED (design-only). The spec now bases freshness on an issued handout's evidence snapshot rather than disposition timestamps, covers delayed first discovery, stores stable condition baselines that survive cache eviction, preserves the #940 stale-FIXED re-observation exception, separates operational parking from human holds, and specifies typed WAITING_EXTERNAL prerequisites plus negative controls. The two earlier substantive human review findings are resolved by the ledger/watermark and per-condition baseline changes. All current-head CI workflows passed. This PR does not implement runtime reactivation, as #1244 expressly requires only design.

Two implementation-stage checks, not blockers for this design PR: (1) bind the proposed watermark to the specific issued handout/receipt when multiple agents hold the same PR-fix generation, rather than overwriting a single item-level watermark; (2) enforce authoritative observation/version ordering, so an old webhook for a known identity cannot regress a newer state and then be re-discovered as fresh. Include regression tests for both in the implementation PR. No code change requested here.

Dismissed as non-substantive: review #552? on head 9839166 explicitly says the model endpoint failed and supplies no actionable findings. Exact-head human review #5524431908 approves the documentation-only design after verifying the prior human findings were addressed and CI is green.

[itsmiso-ai]

Courier: already addressed (run courier-330b60ccf826d65a0d514362c3913712)

PR #1248 was merged by joryirving at 2026-10-14T20:46:19Z (merged:true, merged_at set); main now carries squash commit bce947c whose content is byte-identical to run-branch head 9839166 (git diff HEAD origin/main on all 3 changed docs = 0 lines). All pre-existing feedback was resolved and answered on-thread: human P1/P2 threads (joryirving, 3c6eaa3) fixed in fdb793e with negative controls 17/18/19 and replied with current-code citations; the saffron strict-findings review on 2e96ba3 answered in PR comment 6177703233 (majors conflict with issue #1244 design-only non-goals, minors were replays of answered threads). The only review at head 9839166 was a non-substantive degraded automation notice (AI review could not run, model endpoint failure, degraded_gate_bypass:true) with zero findings, which the maintainer merge supersedes. Local validation receipts at 9839166: npm run lint 0 errors + 1 pre-existing unrelated warning; npm run typecheck exit 0 (documented in PR body; tree unchanged since).