Skip to content

fix(review): decision-locus guard on the execute and reject receipt recorders - #2692

Merged
Chris0Jeky merged 2 commits into
mainfrom
issue-2128/locus-siblings
Sep 5, 2026
Merged

fix(review): decision-locus guard on the execute and reject receipt recorders#2692
Chris0Jeky merged 2 commits into
mainfrom
issue-2128/locus-siblings

Conversation

@Chris0Jeky

Copy link
Copy Markdown
Owner

Summary

onConfirmExecute and onConfirmReject in PaperReviewView.vue recorded their decision receipt unconditionally after the awaited confirm call. The queue rail stays interactive across that round trip, and recordDecisionReceipt also re-pins explicitActiveId, so a reviewer who selected proposal B while the execute or reject request for proposal A was still in flight was pulled back to A the moment the response landed.

Both recorders now compare explicitActiveId.value ?? activeProposal.value?.id against the decided proposal with proposalIdsEqual before recording, which is the same decision-locus guard onApply carries from #2069 and onDefer carries from PR #2629. This is the last of the three sibling recorders named in the issue.

The confirm call, the collected reject reason, the normalized status checks and the deep-link handling are unchanged in both functions.

Closes #2128 (Refs #2069, #2073, PR #2629)

Changes

  • 4083bdc7f test(review): six red-first specs in PaperReviewView.spec.ts, placed next to the onDefer pair they mirror. One trio per sibling: no receipt for a proposal left during the await, a receipt when the locus still matches, and the suppressed-receipt-with-the-hash-still-present combination that PR fix: guard deferred review receipts by decision locus #2629's review recorded as unobserved. Adds two small typed helpers, routerOf and hashOf, using the same structural-cast idiom the existing deep-link specs use for $route.
  • 7fbcb8eec fix(review): the locus guard in both recorders, in onApply's early-return shape since nothing follows the recorder in either function. 14 added lines, no deletions, no other change in the view.

Test plan

Verified, all from frontend/taskdeck-web:

  • Red-first, before the guard commit: npx vitest --run --maxWorkers=2 src/tests/views/paper/review/PaperReviewView.spec.ts -t "applied receipt|rejected receipt|deep-link hash still names" gave Test Files 1 failed (1), Tests 4 failed | 2 passed | 177 skipped (183).
  • Same command after the guard commit: Test Files 1 passed (1), Tests 6 passed | 177 skipped (183).
  • npx vitest --run --maxWorkers=2 src/tests/views/paper/review/PaperReviewView.spec.ts gives Test Files 1 passed (1), Tests 183 passed (183). That is the 177 that existed on the base commit plus the 6 new ones.
  • npm run typecheck passes with no output.
  • npx eslint src/views/paper/PaperReviewView.vue src/tests/views/paper/review/PaperReviewView.spec.ts exits 0 with no findings.
  • git diff --check exits 0.

Which assertions failed on the unmodified view, per sibling:

Execute (onConfirmExecute)

  • "does not record an applied receipt for a proposal left during the execute confirmation" failed on expect(wrapper.get('[data-testid="paper-review-main"]').text()).toContain('Second proposal'). The main pane had re-anchored to the abandoned proposal and rendered its applied record instead ("APPLIED . READ-ONLY ... First proposal").
  • "suppresses the applied receipt while the deep-link hash still names the proposal the reviewer left" failed on expect(wrapper.find('[data-testid="paper-review-decision-receipt"]').exists()).toBe(false), received true.
  • "records an applied receipt when the current decision locus still matches" passed both before and after. It is the over-suppression complement, the same role the defer matching spec plays.

Reject (onConfirmReject)

  • "does not record a rejected receipt for a proposal left during the reject confirmation" failed on the same toContain('Second proposal') assertion, with the pane showing the rejected record for the abandoned proposal.
  • "suppresses the rejected receipt while the deep-link hash still names the proposal the reviewer left" failed on exists()).toBe(false), received true.
  • "records a rejected receipt when the current decision locus still matches" passed both before and after, for the same reason as its execute counterpart.

What each sibling does with the #proposal-<id> hash today, since PR #2629's review asked for it: neither one touches it. clearProposalDeepLink is called only from onFileAway, onFileAwayBulk, onDefer, returnToReview and onQueueFilterChange. The two hash specs assert that the route hash is still #proposal-aaa-1 after the response lands and that the visibleProposals carve-out still anchors the surface on the linked row, so the unchanged behaviour is now observed rather than assumed. Those specs hold the hash on the left proposal with a router beforeEach that aborts the pending navigation, which reproduces the lagging queue-click navigation that is the stated reason the locus expression reads the explicit selection first and the hash only as a fallback. Without that ordering the guard would compare against the stale hash target and the receipt would still surface, so the two hash specs also pin the operand order.

NOT verified:

  • The full frontend vitest suite, npm run build, and Playwright were not run. This change touches two functions in one view and its own spec file, and CI covers the rest.
  • No backend check was run. Nothing outside the frontend changed.
  • No manual browser pass. The lagging-navigation window is modelled in the spec by aborting the router navigation, not by real network or router timing.
  • No other spec file was run, including the Legacy review specs, since no shared module changed.

Boundaries and risks

  • Two files. frontend/taskdeck-web/src/views/paper/PaperReviewView.vue (14 added lines, no deletions, confined to the two functions) and frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts. No composable, locale, router or component change, so nothing outside the Paper review view can be affected.
  • Legacy review view: no slice is owed there. LegacyReviewView.vue wires its dialogs' @confirm straight to confirmExecuteProposal and confirmRejectProposal from useReviewActions.ts with no local wrapper, and it never calls recordDecisionReceipt. The decision receipt is a Paper-skin surface only, rendered by views/paper/review/ReviewMain.vue. There is no unguarded Legacy sibling to follow up on.
  • Residual, pre-existing and identical to onApply and onDefer on main: if the active row leaves the filtered list mid-flight without arming the poll notice, the receipt can still re-anchor the surface. This change neither introduces nor alters that path.
  • The guard can only suppress a receipt. It cannot suppress the decision itself, which has already been committed server side by the time the recorder runs, and the success toast is unaffected.

Six specs mirroring the onDefer pair from PR #2629, one trio per sibling: no
receipt for a proposal left during the await, a receipt when the locus still
matches, and the suppressed-receipt-while-the-deep-link-hash-is-still-present
combination that PR #2629's review recorded as unobserved.

Four of the six fail against the unguarded view. Both "does not record" specs
fail on the main pane, which re-anchors to the abandoned proposal, and both
"suppresses ... deep-link hash" specs fail on the receipt, which renders for it.
The two locus-matching specs pass already and stand as the over-suppression
complements, exactly as the defer matching spec does.

The hash pair holds the route on the proposal the reviewer left with a router
guard, reproducing the lagging navigation that is the stated reason the locus
reads the explicit selection first and the hash only as a fallback.

Refs #2128
…ecorders

onConfirmExecute and onConfirmReject recorded their receipt unconditionally
after the awaited confirm. The queue stays interactive across that round trip,
and recordDecisionReceipt also re-pins explicitActiveId, so a reviewer who
selected proposal B while the execute or reject for proposal A was in flight
was pulled back to A the moment the response landed.

Both now compare explicitActiveId ?? activeProposal.id against the decided
proposal with proposalIdsEqual before recording, the same guard onApply (#2069)
and onDefer (PR #2629) already carry, in onApply's early-return shape since
nothing follows the recorder in either function.

The confirm call, the collected reject reason, the normalized status checks and
the deep-link handling are unchanged. Neither function touched the route hash
before this change and neither does now.

Refs #2128
@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

Review record (alpha product-trust lane, review-and-ship round 1 at head 7fbcb8eec).

Reviewer: one fresh-context independent reviewer subagent (read-only; Codex credits exhausted, SC-9), given the worktree at the head and the merge-base diff. Verdict: SHIP, no CRITICAL, HIGH or MEDIUM.

Lenses that found nothing: the operand order is right (explicitActiveId first, since activeProposal's own fallback is hash-first, and the hashProposalId watcher re-pins explicitActiveId on every landed hash, closing the reverse hazard); the early return skips nothing because recordDecisionReceipt is the last statement of both blocks and the dialog close, the toast and the deep link are all handled elsewhere (neither sibling ever called clearProposalDeepLink); the receipt's explicitActiveId side effect cannot be re-triggered in the same tick because canRefreshQueue vetoes the poll for the whole round trip and an in-flight poll is discarded by the array-identity check; the captured ids are read before the await and the confirm functions null their refs before awaiting; the (a) and (c) specs discriminate (without the guard the surface re-anchors to the left proposal and the receipt renders under the lagging hash), the router beforeEach abort reproduces the real window's state (the global guard is async, so every same-route hash change has that lag), and the (b) specs prove the pairing through activeDecisionReceipt's own id match; LegacyReviewView.vue wires its dialogs straight to the composable's confirm functions and contains no receipt recorder, so all four recordDecisionReceipt call sites are now guarded and #2128 is fully covered; every other receipt-asserting spec is single-proposal or same-proposal and does not regress; scope is the two files.

Findings, triaged once:

  • LOW, declined on the thread (a comment, not a behaviour): the (c) specs' comment credits the visibleProposals carve-out with anchoring the surface, but that carve-out exempts a hash target only from the deferred filter, so the just-applied or just-rejected row leaves the rail under the completed-status filter and what holds the surface is activeProposal's hash branch, which reads the full list. The assertions are correct as written; the sentence is worth correcting the next time the file is touched.
  • Informational, for the coordinator: the thirteenth block's coordination subsection (docs/STATUS.md ~289) still lists PR #2629 as open; it merged today in the D-12 sweep, which the coordinator records in its own subsection after the fifteenth block.

Not verified by the reviewer (no shell): the worker's run of PaperReviewView.spec.ts at the head (183 of 183, six new, four red-first) and typecheck stand as the executed evidence; CI at the head repeats them. Round count: 1. Merge gate: CI green at 7fbcb8eec plus the three-minute age; no fix push, so no second pass is owed.

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.

[Backend][Review] Decision-locus guard from #2069 missing on three sibling receipt recorders

1 participant