fix(review): keep revision metadata truthful across an in-flight GET

[Chris0Jeky]

Summary

Two truth defects in useProposalRevisions found by the fresh-context review of PR #2516.

1. publishPersistedRevisionMetadata bumped loadGeneration before it checked completeness, which
   dropped a revision GET that was already in flight. When another session saved a newer revision while
   this session's POST was travelling, that GET was the only carrier of the newer revision, so the
   composable published a complete looking prefix and the next edit was built on a superseded revision
   until the roughly 15 s poll moved latestRevisionId. The bump is removed. Merging a GET response can
   only add to what the POST proved, so a pre-save answer still cannot lower the published state, and
   loadRevisionState re-checks both the generation and the active proposal before it publishes.

2. history.invalid was never cleared, so one inconsistent response left a proposal's revision
   metadata unknown for the whole composable lifetime. Badges and diff panes stayed hidden and previews
   kept refetching until a remount. mergeLoadedRevisions now validates a GET response on its own terms:
   an internally consistent response is authoritative for the numbers it reports and clears the flag, and
   an inconsistent one stores nothing and leaves the metadata unknown until a consistent answer arrives.
   Numbers the response does not cover keep the revisions their POST responses proved, so a GET that
   predates a save still cannot erase it.

Refs #2524. The two MEDIUM defects and two of the three LOW follow-ups are fixed and proven. The
remaining LOW is stated under Boundaries and risks.

Changes

frontend/taskdeck-web/src/composables/useProposalRevisions.ts

• publishPersistedRevisionMetadata no longer increments loadGeneration.
• New isUsableRevision helper carries the shared proposal-id and revision-number validation.
• mergeLoadedRevisions builds the response into its own map first. A foreign proposalId, a
  malformed revision number, or one number reported under two ids marks the history invalid and
  loaded, and stores nothing from that response. A consistent response resets invalid to false,
  overwrites the numbers it covers, and leaves the numbers it does not cover untouched.
• mergeRevision keeps its existing conflict detection and remains the POST path, so a POST response
  that contradicts known state still marks the history invalid and a later consistent GET recovers it.
• Two comments that described the removed generation bump are corrected.

frontend/taskdeck-web/src/tests/composables/useProposalRevisions.spec.ts

• New: a revision GET in flight when a save lands is merged, so a third revision saved by another
  session becomes the published latest instead of being discarded.
• New: a GET that reports a revision belonging to another proposal leaves the metadata unknown.
• New: an inconsistent response followed by a consistent one recovers the metadata.
• Extended: the A1-before-A2 convergence test now asserts the intermediate publish after A1 lands and
  that the count only rises when A2 lands.

The exported surface of the composable is unchanged, so PaperReviewView.vue needs no edit.

Test plan

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

• Red first. npx vitest --run --maxWorkers=2 src/tests/composables/useProposalRevisions.spec.ts on the
  pre-fix source: 29 tests, 2 failed, 27 passed. The failures were the newer-in-flight-GET merge
  (expected 3, received 2) and the invalid-then-recovered path (expected 2, received 0). The
  cross-proposal guard and the new intermediate-publish assertions passed before the fix, as coverage.
• Green after the fix. Same command: 1 file, 29 tests passed.
• Neighbours and consumer.
  npx vitest --run --maxWorkers=2 src/tests/composables/useReviewProposals.spec.ts src/tests/composables/usePaperReviewSelectors.spec.ts src/tests/views/paper/review/PaperReviewView.spec.ts:
  3 files, 319 tests passed.
• Baseline before any edit, for comparison:
  npx vitest --run --maxWorkers=2 src/tests/composables/useProposalRevisions.spec.ts src/tests/composables/useReviewProposals.spec.ts src/tests/composables/usePaperReviewSelectors.spec.ts:
  3 files, 185 tests passed.
• npm run typecheck: clean.
• npx eslint src/composables/useProposalRevisions.ts src/tests/composables/useProposalRevisions.spec.ts:
  no findings, exit 0.
• npm run build: succeeded.
• git diff --check: clean.

NOT verified:

• No Playwright or browser-mode run. The change is composable-internal and has no rendered surface of
  its own, and the e2e suite needs a running stack.
• The full frontend vitest suite was not run. A bare full run OOMs on this box, so the run was scoped
  to the changed composable, its two neighbour composables, and the one view spec that consumes it.
• No backend run. Nothing outside the frontend composable changed.
• Real concurrent-session behaviour against a live API was not exercised. The overlap is proven with
  controlled promise resolution in the spec, not against the server.

Boundaries and risks

• Files touched: useProposalRevisions.ts and its spec only. PaperReviewView.vue,
  useProposalDisplayNames.ts, ReviewProposalCard.vue and their specs are owned by PR fix(review): enrich incomplete move-card headlines #2541 and were
  not edited. The Inbox store and orchestrator were not touched.
• Remaining LOW, not addressed here: revisionHistoryByProposal is still unbounded. It gains one entry
  per proposal visited, so a long queue walk grows it for the lifetime of the composable instance.
  Bounding it needs care because the map exists precisely so a delayed POST response can be reconciled
  after an A to B to A navigation, and evicting an entry would reintroduce the loss it prevents.
• Behaviour change to be aware of during review: a GET response that disagrees with a POST-remembered
  revision at the same number no longer latches the history invalid. The authoritative GET wins for the
  numbers it covers. This is what makes recovery possible, and it is the reason an internally
  inconsistent response is now rejected wholesale rather than partially stored.
• The indeterminate save path still bumps loadGeneration through invalidateActiveRevisionMetadata.
  That bump is load bearing and was left alone: a pre-write GET cannot know about a revision the failed
  POST may have committed, so it must not be allowed to publish a complete looking chain.

Round 2 (head b042110)

The fresh-context review found one HIGH, confirmed by execution: with the generation bump removed, a revision GET that was in flight when a POST published metadata could reject afterwards and leave revisionsLoaded true with revisionCount 0. Fixed: the catch now clears count and latest only when the metadata was already non-authoritative (revisionsLoaded false), which is behaviour-identical to main on every pre-existing path and closes the new one; republishing from the proven history was rejected because it would restore false authority in the resync case that the #2215 round-1 M-1 spec forbids. Regression: the rejected reopened-A GET after a proven save keeps count 2, latest rev-saved, loaded true (red 1 failed / 29 passed before the fix, green after).

Also in this round: history.invalid is cleared only by a response that is a whole chain of its own (a partial answer is merged but proves nothing about a disputed number), with a spec that isolates the rule (red without it); the pre-save load test is renamed to describe the additive merge; both doc comments now describe the failure path; the aborted-GET spec uses a scripted mockImplementation so a red cannot leak unconsumed once-values into later tests.

Supersedes the Boundaries statement above that described the invalid clearing as unconditional.

Verified at this head: useProposalRevisions.spec.ts 31 passed; useProposalRevisions, useReviewProposals, usePaperReviewSelectors and PaperReviewView specs 350 passed across 4 files; npm run typecheck; scoped ESLint; npm run build; git diff --check. NOT verified: Playwright, full vitest run.

[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 7bb19fc7e (merge base 61e94f672). Verdict: FIX-FIRST, returned to the branch owner for one fix round.

HIGH, confirmed by execution. Removing the loadGeneration bump from publishPersistedRevisionMetadata lets a revision GET still be in flight when a POST publishes revisionsLoaded = true. If that GET rejects (network, timeout, 403/404, or the retry interceptor exhausting its backoff), loadRevisionState's catch passes both guards and sets revisionCount = 0, latestRevision = null while revisionsLoaded stays true: the state the composable's own comment forbids. Consequences at the surface: PaperReviewView renders the diff pane as no-operations for a proposal that carries revisions, blocks Apply with a false zero-op toast and never reloads, and openRevisionEditor pins the editor to the raw pre-revision operations so the next save silently discards the proven revision. On main the catch is unreachable while revisionsLoaded === true. Confirmation: the existing A-B-A spec with the reopened GET rejecting instead of resolving fails on this head (loaded: true, count: 0, latest: null) and passes with main's composable. Fix direction: publish from the history the POST already proved in the catch (or at minimum reset revisionsLoaded), add the interleaving as a regression, and correct the two doc comments that describe the success path only.

Non-blocking:

• LOW, fix if cheap: history.invalid is cleared by any internally consistent response, even one that does not cover the disputed number; needs an already-broken server (unique (ProposalId, RevisionNumber) index), so tracked otherwise.
• LOW: stale test title at spec ~412 ("ignores a pre-save revision load" now merges additively).

Confirmed clean: monotonicity of count/latest on the success path, the A-B-A guard, mid-chain gaps and duplicate numbers left unknown, proposalId mismatch discarded, POST-path conflict detection retained, and the post-revision truth barrier keys on the wire proposal's revision identity rather than the composable's published fields. Round count: 1 of 2.

[Chris0Jeky]

Round 2: scoped second-pass fresh-context review of the fix diff (7bb19fc..b042110), verdict SHIP. The round-1 HIGH is closed: the reviewer enumerated all eight loadRevisionState call sites and every one either sets revisionsLoaded false immediately before the call or is guarded by !revisionsLoaded, and PaperReviewView (the sole consumer) never writes the flag, so at catch time revisionsLoaded === true can only mean a POST proved a complete chain during the GET's flight; the retained triple is a proven contiguous chain. The resync-over-publish interleaving produces the same state as main. The toast still fires (silent still suppresses it); the completeness gate can only withhold the invalid-to-valid transition; additive merge plus max-key selection cannot lower the count; an empty list publishes count 0 only when the stored history is empty too. Disposition: MEDIUM (the completeness gate does not cover the disputed number; precondition is a self-contradicting server, which the unique (ProposalId, RevisionNumber) index rules out short of corruption) and two LOWs (failure-toast noise after a proven save; the clear half of the catch rule is unpinned) tracked as #2579 together with #2524's remaining unbounded-map LOW. Nothing fixed in this round. Round count: 2. Merge after ci-required is green at b042110.