fix(review): bound the post-revision truth refresh and keep it retryable

[Chris0Jeky]

Summary

The post-revision truth barrier (PR #2448) holds the shared decision lock across two reads: one authoritative proposal-queue read and the six core evidence reads for the exact revision key. Neither read had a caller-owned deadline. A leg that never answered held applyGuardBusy and revisionReviewRefreshBusy forever, so the decision rail stayed disabled, the keymap stayed inert, and the reviewer had no path back to their own decision and no explanation. This bounds the whole attempt and makes every non-landed ending retryable.

Barrier sequence before this change:

1. A persisted or indeterminate revision save arms a per-proposal epoch.
2. The next explicit Approve or Apply consumes the action and locks the rail.
3. await loadProposalsWithOutcome() runs with no signal and no deadline.
4. Anything but landed returns silently, with no persistent guidance.
5. await selectors.waitForCoreBatch(id, revisionIdentity) runs with no caller signal and no deadline.
6. failed shows one message; superseded and unavailable are silent.
7. On success the DTO is re-verified for identity, status, expiry, defer state and effective revision, the epoch is checked and deleted, and the rail unlocks.
8. If step 3 or step 5 never settled, the finally never ran and the rail never unlocked.

Barrier sequence after this change:

1. Unchanged: a persisted or indeterminate save arms the per-proposal epoch.
2. The next explicit Approve or Apply consumes the action, locks the rail, and starts one attempt that owns one AbortController and one deadline (POST_REVISION_REVIEW_DEADLINE_MS, 12 s) covering both reads.
3. The queue read receives that signal. The attempt races it against the deadline.
4. The six core evidence reads receive the same signal. The attempt races them against the same deadline.
5. The attempt ends as exactly one of refreshed, failed, timed-out, aborted or superseded.
6. refreshed requires the same identity, status, expiry, defer state and effective revision on the re-read DTO, a fully landed exact-key selector batch, and a matching epoch. Only then is the epoch deleted.
7. Every other ending retains the epoch, so the next Approve or Apply refreshes again and never approves cached pre-revision operations.
8. The rail unlocks on every ending. failed and timed-out show distinct persistent notes and toasts. aborted and superseded stay silent, as superseded did before.
9. An answer that arrives after its attempt was abandoned is dropped: an aborted queue read does not become the rendered queue, and an attempt generation stops a late attempt from unlocking a rail or writing a note another attempt owns.

The deadline both aborts the shared controller and resolves a race marker. Aborting alone is not enough, because a transport that ignores cancellation is exactly the stall being guarded against.

Preview == Apply, the explicit approve and explicit execute separation, and the requirement of a second explicit action after a clean refresh are unchanged. No approve or execute request is issued by any barrier path.

Closes #2460

Changes

• frontend/taskdeck-web/src/views/paper/PaperReviewView.vue — barrier region only. Adds POST_REVISION_REVIEW_DEADLINE_MS with a comment in the spirit of BOARD_REQUEST_TIMEOUT_MS, startRevisionReviewAttempt() (one controller plus one deadline per attempt), the five-member RevisionReviewRefreshOutcome, an attempt generation guard, and a reason-keyed unavailable map so timed out and failed get different copy. refreshRevisionReviewBeforeApply is split into runRevisionReviewRefresh (decides the outcome), applyRevisionReviewOutcome (acts on it) and the lock or focus wrapper. The identity, status, expiry, defer and epoch checks are carried over unchanged.
• frontend/taskdeck-web/src/composables/useReviewProposals.ts — loadProposalsWithOutcome(options?) accepts a caller signal, forwards it to automationApi.getProposals and to the deep-link leg, and reports the new aborted member instead of failed. An aborted read raises no toast, does not become the rendered queue, and does not clear the stale-queue indication. The second argument is forwarded only when a signal is supplied, so every existing call site keeps its exact one-argument shape.
• frontend/taskdeck-web/src/composables/usePaperReviewSelectors.ts — waitForCoreBatch(id, identity, options?) accepts a caller signal, adds the aborted outcome member, and cancels the reads it was holding open. The cancellation is scoped to the waited key so an abandoned wait cannot tear down a batch the reviewer has since moved to. The same-action retry of fix(review): retry unavailable evidence in the same action #2528 is factored into runCoreBatchWithSameActionRetry and is otherwise unchanged.
• frontend/taskdeck-web/src/locales/{en,it,es}/review.ts — adds review.toast.revisionReviewTimedOut in all three languages.
• Specs in PaperReviewView.spec.ts, useReviewProposals.spec.ts and usePaperReviewSelectors.spec.ts.

No API layer change was needed: automationApi already took { signal, skipRetry } and proposalDeepReviewApi already took { signal }.

Test plan

Verified, from frontend/taskdeck-web after npm ci:

• npx vitest --run --maxWorkers=2 src/tests/views/paper/review/PaperReviewView.spec.ts src/tests/composables/useReviewProposals.spec.ts src/tests/composables/usePaperReviewSelectors.spec.ts src/tests/composables/useProposalRevisions.spec.ts src/tests/views/paper/review/ReviewDecisionRail.spec.ts src/tests/views/paper/review/ReviewMain.spec.ts — 6 files, 394 tests passed (319 in the three touched files before this change, 165 + 121 + 45 in them now).
• npx vitest --run --maxWorkers=2 src/tests/i18n/catalogs.spec.ts src/tests/guards/nativeBrowserDialogs.spec.ts src/tests/guards/primaryActionGuards.spec.ts — 3 files, 28 tests passed (locale parity for the new key).
• npm run typecheck — clean.
• npx eslint over every changed file — clean.
• npm run build — succeeded.
• git diff --check — clean.

New specs and their pre-change state. Source files were reverted to the base commit 8c51120 with the specs left in place, and the suite re-run:

Red against the pre-change code (5 of 5 composable specs, 2 of 5 view specs):

• useReviewProposals: reports a caller-aborted explicit load as aborted rather than failed; does not issue an explicit load whose caller has already given up; reports an aborted deep-link leg as aborted and raises no lookup error.
• usePaperReviewSelectors: reports a caller-cancelled wait as aborted and stops the reads it held open (pre-change it hung for the full 5 s test timeout); reports a wait whose caller has already given up as aborted without reading.
• PaperReviewView: releases the rail on its deadline when the authoritative queue read never answers; releases the rail on its deadline when a core evidence read never answers. Both cover stalled transport with fake timers, the retry succeeding on the next explicit action, late-response suppression after the timeout, and no approve or execute before the successful second explicit action.

Green against the pre-change code, added as regression guards rather than as proofs of new behaviour:

• PaperReviewView: names a partial core evidence failure as a failure, never as a timeout; names a total core evidence failure as a failure and retains the barrier; disarms its deadline once the refresh lands, so no late timeout note appears. The failure behaviour itself shipped in fix(review): require authoritative refresh after revisions #2448 and fix(review): retry unavailable evidence in the same action #2528; what is new in these is the assertion that failure copy and timeout copy never substitute for each other, and that all six reads rejecting behaves like one rejecting.
• usePaperReviewSelectors: still reports a genuine read failure as failed when a signal is supplied; does not cancel the batch the reviewer moved to when an abandoned wait is cancelled. The second was written after the scoping fix in commit d80fb7e and is red against the intermediate unscoped version, not against the base.

NOT verified:

• Playwright tests/e2e/review-proposals.spec.ts, which holds the fix(review): require authoritative refresh after revisions #2448 browser proof. It needs a running stack, which this worktree does not have. The spec was not edited and the data-testids it uses (decision-apply, decision-lock-note, paper-review-evidence-unavailable) are unchanged.
• The full frontend vitest suite. Only the six required spec files plus the three guard and locale specs were run.
• Backend, and any real-network behaviour of the deadline. Every stall in these specs is a test double that never settles.
• The 12 s value itself against real latency. It is a judgement call, documented at the constant, and it is retained-barrier safe in both directions: too short costs one extra explicit action, never a decision on stale evidence.

Boundaries and risks

• Four files held by sibling PRs were not touched: useProposalRevisions.ts (fix(review): keep revision metadata truthful across an in-flight GET #2565), ReviewDecisionRail.vue and ReviewMain.vue (fix(review): associate the refresh-lock reason with every disabled decision #2568), and the e2e spec. activeRevisionReviewUnavailable stays a boolean computed precisely so the evidence-unavailable props into ReviewMain keep their current shape; the new reason only feeds the note text inside PaperReviewView.vue. The lock-reason id reaching the rail is the one PaperReviewView.vue already passes.
• PaperReviewView.vue is large; only the barrier region was changed. No opportunistic refactors.
• Behaviour change outside the barrier: loadProposalsWithOutcome and waitForCoreBatch gained an optional argument and a new outcome member. Every existing caller passes no options and so cannot observe aborted; the exhaustive branches in the view were updated. The getProposals call keeps its exact one-argument shape when no signal is supplied, which is what automationApi.spec.ts pins.
• An aborted batch whose transport genuinely never settles leaves selectors.loading true until that promise resolves, because Promise.allSettled cannot resolve earlier. That is an honest reflection of a read that never came back; it does not gate the rail, which is released by the deadline, and the barrier note states that no decision was made.
• The attempt generation guard is defence in depth. On the current code path a timed-out attempt returns at its race and cannot resume, so no test can drive two overlapping attempts through the public surface. The guard is there so a future change that resumes after a deadline cannot silently unlock a rail another attempt owns.
• No auth, permission or security policy surface was touched. No canonical doc was updated: shipped behaviour of the review loop is unchanged apart from the barrier now being bounded and legible, which docs/STATUS.md does not enumerate at that granularity.

Round 2

Review verdict was SHIP with no CRITICAL or HIGH; both invariants held under attack. This round fixes the three MEDIUM findings and the two LOW takes on this PR's own new paths. Head 35b260b25.

MEDIUM 1 — an unattributable barrier report could be read as an instruction to decide on another proposal. applyRevisionReviewOutcome toasted unconditionally while only the note was guarded by "is this still the active proposal". The queue rail is not disabled by the decision lock, so a reviewer could select B while A's barrier was reading, then see "Refreshing the review took too long ... Choose the current action again to retry" over B. Following that on B, which has no barrier, falls straight through to handleApproveProposal(B). The note and the toast are now one report behind one guard, and the guard covers the refreshed path too: "Check the current evidence, then choose the action again" read on B is the same hazard. Withholding costs nothing, because the barrier state is written separately from the report, so returning to A still refreshes.

MEDIUM 2 — a barrier attempt outlived its route. onUnmounted stopped the clock, the poll and the collaboration read but left the attempt running, so its setTimeout fired after navigation and reported on whatever page the reviewer had moved to. dispose() also only cleared the timer, so an early failed or superseded ending left its reads running. dispose() now aborts the controller as well, the current attempt is tracked in activeRevisionReviewAttempt, and onUnmounted bumps the attempt generation first and then disposes, so nothing can be applied after unmount.

MEDIUM 3 — retries could spend the deadline and turn a recoverable failure into a reported timeout. The barrier's queue read and its deep-link leg now pass skipRetry: true alongside the signal, matching the background poll. With MAX_RETRIES 3 and 1 s doubling backoff, a single transient failure could otherwise consume roughly 7 s of the 12 s cap, and a Retry-After can exceed it outright.

The six core evidence reads were left retrying, and the constant's doc comment now states that assumption instead of implying retries are skipped everywhere. Two reasons, as invited: proposalDeepReviewApi.RequestOptions carries only signal and each of the seven methods hardcodes { signal: options?.signal }, so a per-call opt-out means changing the shared API surface; and more decisively, ensureCoreBatch reuses a batch the automatic watcher may already have started for the same key, which is the common path here because the queue read lands a new revision identity and triggers that watcher. A per-call flag would therefore apply only when the barrier happened to be the batch's creator, making the same user action retry or not depending on a race. Uniform retrying is the better of the two, so the budget documents that it must still cover one interceptor retry pass on the evidence leg.

LOW take — the barrier clear now sits behind the attempt-generation guard. revisionReviewRefreshEpochs.delete(key) ran inside runRevisionReviewRefresh, ahead of the generation check. The runner now decides the outcome and writes nothing; the new clearRevisionReviewBarrier performs the delete in the caller, after the generation check, and re-reads the epoch rather than trusting the verification block, so a save that landed while the attempt was resolving cannot have its newer barrier dropped. Unreachable today, ordering hygiene as noted.

LOW take — the deep-link leg now has an exact-args assertion that it receives the same signal and skipRetry, mirroring the getProposals one.

Not changed, per the review: the note keyed to the pre-save revision identity vanishing when the 15 s poll lands the new identity and leaving a stale map entry, and runCoreBatchWithSameActionRetry re-issuing an uncancellable six-read batch after a deadline abort.

Round 2 test plan

Verified, from frontend/taskdeck-web:

• npx vitest --run --maxWorkers=2 src/tests/views/paper/review/PaperReviewView.spec.ts src/tests/composables/useReviewProposals.spec.ts src/tests/composables/usePaperReviewSelectors.spec.ts src/tests/composables/useProposalRevisions.spec.ts src/tests/views/paper/review/ReviewDecisionRail.spec.ts src/tests/views/paper/review/ReviewMain.spec.ts src/tests/i18n/catalogs.spec.ts — 7 files, 416 tests passed. The six-file set alone was 394 in round 1 and is 398 now; catalogs.spec.ts adds 18.
• npm run typecheck — clean. npx eslint over every changed file — clean. npm run build — succeeded. git diff --check — clean.

Four new specs, all red against the round-1 head d80fb7eb7 with the source files reverted and the specs left in place:

• PaperReviewView: reports nothing when the reviewer moved to another proposal before the deadline — arms A, stalls the queue read, selects B, fires the deadline, asserts no toast and no note on B, then returns to A and asserts its barrier still refreshes instead of approving.
• PaperReviewView: cancels an in-flight attempt on unmount so its deadline never reports — asserts the barrier's signal is aborted by the unmount and that advancing twice the deadline afterwards produces no toast at all.
• useReviewProposals: forwards a caller opt-out of the shared retry interceptor.
• useReviewProposals: reports an aborted deep-link leg as aborted and raises no lookup error — extended with the exact-args assertion on getProposal.

Still NOT verified, unchanged from round 1: Playwright tests/e2e/review-proposals.spec.ts (needs a running stack, not edited), the full frontend vitest suite, the backend, and any real-network behaviour of the deadline or of skipRetry. Every stall in these specs is a test double that never settles.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[Chris0Jeky]

Review gate (Codex credits exhausted, SC-9): one fresh-context adversarial reviewer on head d80fb7eb7 (merge base 8c511205d). Verdict: SHIP. Both load-bearing invariants held under direct attack: the epoch is deleted at exactly one site, reachable only after a landed queue read, the post-read DTO re-check, a settled exact-key selector batch and the full identity/status/expiry/defer/effective-revision re-verify; settled provably means this revision's six reads landed; the deadline-versus-reads race cannot be mis-mapped; overlapping attempts are impossible because the busy guard and the flag set are synchronous; the unlock runs in finally on every ending including thrown exceptions. Abort scoping, queue-read backward compatibility, the boolean evidenceUnavailable prop shape (so PR #2568 does not conflict), the en/es/it copy, the live region and test determinism (12 s below the 15 s poll and the 60 s expiry clock) were confirmed clean.

Findings by bin:

• Fixed in the same round (each sits in a path this PR added or extended): MEDIUM, the failed/timed-out toast is not guarded by the active-proposal check that guards the note, so an attempt that times out after the reviewer moved to another proposal (queue-rail row select is not disabled by busy) toasts retry guidance over the wrong proposal; MEDIUM, the attempt is not aborted or disposed on unmount, so the 12 s timer outlives the route and dispose() never aborts the controller; MEDIUM, the barrier's reads pass signal but not skipRetry: true, unlike the background poll, so one transient failure's retry backoff can consume the 12 s cap and report a recoverable composite as timed out. Two LOWs folded in: the epoch delete now sits after the attempt-generation check; the aborted deep-link spec asserts the forwarded signal.
• Tracked (issue to follow): the failed/timed-out note is keyed to the pre-save revision identity and disappears when the poll lands the new identity, leaving a stale map entry; after a deadline abort the same-action retry re-issues a six-read batch no caller holds.
• Declined: the three view specs that pass against pre-change sources stay as regression guards; the two stall specs are the proofs, as the PR body already says.

Evidence at the reviewed head (from the implementation run): 394 tests across six Review files (331 in the three touched files, 319 before), locale parity, typecheck, ESLint, build, diff check; 5 of 5 new composable specs and the two stall specs red against the pre-change sources. Not verified: Playwright, real-network deadline behaviour. Merge after the fix commit ages and ci-required is green at the new head.

[Chris0Jeky]

Round 2: scoped second-pass fresh-context review of the fix diff (d80fb7e..35b260b), verdict SHIP. Confirmed: the epoch delete is now gated in series on the attempt generation, a refreshed outcome (which needs the full identity re-verification) and a re-read of the epoch, so a save landing mid-attempt keeps its newer barrier and the attempt returns superseded silently (safe direction, terminates); the guard extension is behaviour-preserving on screen and only withholds the three reports once the reviewer moved; unmount bumps the generation before dispose so the aborted attempt's finally writes nothing; dispose cannot tear down the shared core batch on a normal ending because the wait's abort listener is removed in the race's finally; skipRetry reaches only the barrier's queue read and deep-link leg (the poll and the plain loads are pinned by an untouched arity assertion); all four new specs are red against the round-1 sources for the right reason; four files touched, none of useProposalRevisions.ts, ReviewDecisionRail.vue, ReviewMain.vue or usePaperReviewSelectors.ts.

Three LOWs, tracked on #2581, none actioned: an unmount during the evidence leg can trigger the same-action retry's six fresh GETs that nobody aborts (network only, no state write); with skipRetry the deep-link leg can surface review.toast.loadProposalFailed on a single transient blip while the barrier outcome stays safe; one comment sentence about withholding is exact only for the reachable cases. Round count: 2. Merge after ci-required is green at 35b260b and the head has aged; the Smart CI / Plan red on this head is the shadow lane's stacked-base planner shape (#2562) and is advisory.