Skip to content

fix(review): preserve revision state after definite save rejection - #2510

Merged
Chris0Jeky merged 3 commits into
mainfrom
issue-2455/rev-reject
Sep 4, 2026
Merged

fix(review): preserve revision state after definite save rejection#2510
Chris0Jeky merged 3 commits into
mainfrom
issue-2455/rev-reject

Conversation

@Chris0Jeky

Copy link
Copy Markdown
Owner

Summary

  • distinguish definite HTTP 4xx revision-save rejections from ambiguous timeout, network, and 5xx failures
  • preserve known revision metadata, the retryable editor draft, and focus after a definite rejection
  • keep the existing queue/read invalidation barrier for genuinely indeterminate outcomes

Closes #2455

Verification

  • npm ci --no-audit --no-fund
  • npm test -- --maxWorkers=2 src/tests/composables/useProposalRevisions.spec.ts src/tests/views/paper/review/PaperReviewView.spec.ts — 177 passed
  • npm run typecheck
  • npx eslint src/composables/useProposalRevisions.ts src/views/paper/PaperReviewView.vue src/tests/composables/useProposalRevisions.spec.ts src/tests/views/paper/review/PaperReviewView.spec.ts --max-warnings=0
  • git diff --check

Review notes

  • definite 4xx responses do not call onRevisionStateUncertain or schedule the revision refresh barrier
  • timeout/network/5xx behavior remains covered by the existing indeterminate regressions
  • local Mock provider; retired Gemini environment variables scrubbed from the process

@chatgpt-codex-connector

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Owner Author

The connector usage-limit notice is informational, not a review finding. This PR is being checked through the independent coordinator review path; no code action is requested from this notice.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Review gate (Codex credits exhausted, SC-9): one fresh-context reviewer. Verdict: FIX-FIRST — parked for the branch owner; the stacked PR #2516 parks with it.

HIGH — a 409 (or 404) from POST {id}/revisions is treated as a definite "server state unchanged" rejection, so both invalidateActiveRevisionMetadata and the #2448 requireRevisionReviewRefresh barrier are skipped. useProposalRevisions.ts:23-33, 209-217 classify every 4xx as rejected; PaperReviewView.vue:1924-1936 returns before line 1941. But the backend maps both Conflict and InvalidOperation to 409 (ResultExtensions.cs:23-24) and the revision endpoint reaches both: the concurrent-writer collision on (ProposalId, RevisionNumber) (ProposalRevisionService.cs:97-101 — the OTHER writer's revision N+1 is committed) and "proposal no longer PendingReview" (:41-44); a deleted proposal is 404 (:38-39).

Failure scenario: sessions A and B both hold proposal P at revision 1; B saves revision 2; A saves → 409. A keeps revisionsLoaded=true, revisionCount=1, its stale preview diff (the rejected branch skips clearPreviewDiff at 1945-1948) and records no refresh epoch. A clicks Apply: onApply (PaperReviewView.vue:1466-1472) finds no required epoch, skips the mandatory refresh preflight, and ApproveProposalAsync pins the server's latest revision — B's — as ApprovedRevisionId (AutomationProposalService.cs:489-495, 525). The reviewer approves content they never saw, the invariant #2448/#2215 exist to prevent. Before this PR the 409 forced an authoritative refresh first.

Suggested fix shape: classify only statuses that imply "server state unchanged" (400/401/403/413/422/429) as rejected; route 404/409 to a third outcome that keeps the retryable draft and focus (the real win here) but still calls onRevisionStateUncertain() / invalidateActiveRevisionMetadata() and requireRevisionReviewRefresh() and clears a matching preview.

MEDIUM — the doc comment at useProposalRevisions.ts:23-27 lists "revision conflicts" among the 4xx cases declared safe; correct it with the fix. MEDIUM — no test pins the 4xx/5xx boundary (add a 500 → indeterminate case and a 409 case encoding the fix). LOW — hand-built { response: { status } } errors rather than axios shapes (declined).

Clean: the ambiguous path is byte-identical; focus after rejection is session-scoped per #2423; 401 and 429 handling is sound.

@Chris0Jeky Chris0Jeky left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Post-fix adversarial pass at head 9123a4c: SHIP. The 4xx classification is now an explicit allowlist (400/401/403/413/422/429); 404, 409, and 500 take the uncertainty path, invalidate metadata, notify the review refresh barrier, and retain the retryable draft. Added regression coverage for 404/409/500 plus a PaperReview 409 barrier case. Local proof: vitest composable + PaperReview 181/181, npm run typecheck, npx eslint on touched files, git diff --check. No new CRITICAL/HIGH findings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[Frontend][Review] Preserve known revision state after definite save rejection

1 participant