fix(review): a malformed deep-link target no longer freezes the queue refresh, and refused pins stop logging as API errors - #2658
Conversation
The background queue refresh reads the list, then re-authorizes a hash-pinned proposal the list omitted. A by-id read that answered 400 fell to the queue-level failure branch, which returns before the list answer is assigned. The list read had already succeeded, so a bad deep link discarded a good queue. It did so silently. A 400 is not transient, so the consecutive-failure counter reset instead of climbing to the degraded threshold, and no warning ever rose. The pin leg re-fires on every tick while the target sits outside the list, so the freeze lasted as long as the malformed id stayed in the URL. A 400 on this route is a model-binding failure: GetProposal(Guid id) under [ApiController] never reaches its handler for a non-GUID id, and nothing client-side validates the hash. That is a permanent fact about the requested target, the same class as a 403 or a 404, so it takes the same pinUnavailable outcome: the list lands, the pin is marked unavailable, the failure counter is untouched and no degraded state rises. The predicate is 400 alone. Only a malformed id is provably unusable; 405 would be a whole-surface routing defect and 404 already covers gone. The explicit openProposalFromHash path is unchanged, and every other pin-leg failure shape keeps its current behaviour. Refs #2214
The shared response interceptor logs any error status a call has not named as expected under 'API Error:'. The background poll's by-id pin read turns 400, 403 and 404 into an explicit "this pin is unavailable" outcome, so each one was reported as a failure while being handled as a result, on every tick that the pinned target sat outside the list page. AutomationReadOptions gains expectedStatuses and forwards it the way proposalDeepReviewApi.getProvenanceMetadata already does. The options object was already passed to http.get verbatim, so no other caller's call shape moves. Only the background pin read names those statuses. openProposalFromHash is a read the reviewer asked for and keeps its logging and its toast, which the spec now pins explicitly rather than by objectContaining. Refs #2214
…rget The composable spec proves the outcome; this proves the reviewer sees it. The view mounts with the target in the queue so the explicit deep-link read never fires, then the next background read drops it from the list and its by-id re-read answers 400. Without the pin-leg fix the queue answer is discarded, so the dropped row is still on screen and no panel appears. Verified red against that state before this landed. Refs #2214
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Review record (alpha product-trust lane, review-and-ship round 1 at head 2d6dc3b). Reviewer: one fresh-context independent reviewer subagent (read-only), input = merge-base..head diff plus the worktree at the head. Verdict: SHIP, no CRITICAL or HIGH. Confirmed clean: isMalformedTargetError is referenced exactly once, inside the try that wraps only the by-id read, so a list-read 400 cannot be swallowed as an unavailable pin; the backend 400 on GET /api/automation/proposals/{id} is model binding on a non-GUID id and nothing else (no ApiBehaviorOptions override; the handler's failure paths are 401, 403 and 404 only; the 400 mapping is reachable only from ValidationError and WipLimitExceeded, neither on this route); expectedStatuses gates only the console log in http.ts, the 401 session interception sits outside that gate and 401 is not listed, the retry interceptor is unaffected, and the option matches the shipped cardsApi precedent; every other getProposal caller keeps its arity and no Paper spec asserts exact pin-read args; the 400 path reaches recordQueueRefreshSuccess like 403 and 404, so the #2630 recovery signal is honest (the list read succeeded); the settled-row notice suppression matches 403 and 404; both skins consume the same unavailableProposalId state; PaperReviewView.vue and every Paper spec untouched; no backend change. Triage:
Merge gate: ci-required green at the round-2 head, aged three minutes, then merge commit; no further review round because round 2 changes comments only. |
Review round 2, comment text only, no logic and no spec change. The Legacy panel comment still named 403/404 as the whole set; it renders for a 400 target too. The unavailableProposalId comment still said "a confirmed 404"; the ref holds 404, a by-id 403 and a 400, plus the fail-closed wrong-identity and cross-board cases. isMalformedTargetError gains the boundary that makes it sound: it reads the by-id leg only. A 400 from the LIST read means the query was rejected, which says nothing about a pinned target, so moving the predicate to the outer catch would silently downgrade a whole-queue failure into a pin-level outcome. Refs #2214
1 participant
Summary
Refs #2214. This closes residual items 7 and 8 of the 2026-09-03 reconciliation comment. Item 2 stays open for a later slice that needs both review views.
The background review-queue poll is a composite read: it reads the list, then re-reads a hash-pinned proposal by id when the list page omits it. Both legs were already handled for 403 and 404, which mark only the pin unavailable and let the list answer land. Every other by-id failure went to the queue-level failure branch, which returns before
proposals.value = next.Item 8. A
#proposal-<id>whose id is not a GUID is answered 400 by the by-id route.GetProposal(Guid id)sits under[ApiController], so model binding refuses a non-GUID route value before the handler runs, and nothing client-side validates the hash. That 400 took the queue-level branch, so a list read that had already succeeded was thrown away. It was also silent:isTransientQueueRefreshFailurecounts only a missing response, 408, 429 and 5xx, so the 400 reset the consecutive-failure counter instead of climbing to the degraded threshold andqueueRefreshStalenever rose. The pin leg re-fires on every tick while the target sits outside the list, and a malformed id is never in the list, so the queue refresh froze with no indication for as long as the bad link stayed in the URL.Item 7. The shared response interceptor in
api/http.tslogs any error status a call has not listed inexpectedStatusesasAPI Error:. The background pin read turns 400, 403 and 404 into an explicit pin-unavailable outcome, so each handled result was reported as a failure on every tick.Predicate chosen:
status === 400, on the background pin leg only. 405 and 410 are deliberately excluded. Only a malformed id is provably unusable: a 405 would be a routing defect affecting the whole surface rather than one target, and 404 already carries the gone meaning here. Neither status is emitted by this route, so giving them pin-level meaning would be guessing. The explicitopenProposalFromHashpath is untouched: its non-404 failures still toast, and it does not passexpectedStatuses.Unchanged: the 403 branch, teardown and abort handling, supersession guards, the 8-second request deadline, the three-failure degraded threshold, and the recovery signal from #2630. A 5xx or a no-response on the pin leg is still transient and still keeps the current queue; a 401 keeps its existing handling.
Changes
04ff09f08fix(review): a malformed deep-link target marks only the pin unavailablesrc/composables/useReviewProposals.ts: new module-levelisMalformedTargetErrorpredicate (400 only) with the reasoning above; the pin-leg catch takes the existingpinUnavailable = truebranch for it, alongside 403 and 404. The consequences are the shipped ones: the fresh list lands,unavailableProposalIdis set so the already-shipped Legacy and Paper unavailable-pin states render,recordQueueRefreshSuccessruns so the failure counter is not touched and no degraded state rises, andonQueueReplacedByPollis not fired.src/tests/composables/useReviewProposals.spec.ts: a dedicated red-first case, plus 400 added to the existingit.eachthat already pinned 403 and 404 through the recovery half.d68e1248dfix(review): a refused background pin read stops logging as an API errorsrc/api/automationApi.ts:AutomationReadOptionsgainsexpectedStatuses?: number[]. Both readers already forward the options object verbatim, and only when one was supplied, so no other caller'shttp.getarity or arguments move.src/composables/useReviewProposals.ts: the background pin read passesexpectedStatuses: [400, 403, 404]. This is theproposalDeepReviewApi.getProvenanceMetadatapattern.src/tests/api/automationApi.spec.ts: the option is forwarded when given, and the key is absent when it is not.src/tests/composables/useReviewProposals.spec.ts: the exact-options assertion at the pin call site now includes it, and the explicit deep-link read is pinned as not carrying it.2d6dc3b95test(review): pin the Legacy unavailable panel for a malformed 400 targetsrc/tests/views/ReviewView.spec.ts: the reviewer-visible half. Mounts with the target in the queue so the explicit read never fires, then the next background read drops it and its by-id re-read answers 400.PaperReviewView.vueand its spec are held by #2629 and are untouched here.Test plan
Verified, from
frontend/taskdeck-web:Red-first, before the source change:
npx vitest --run --maxWorkers=2 src/tests/composables/useReviewProposals.spec.ts -t "malformed deep-link target answers 400"— 1 failed, 128 skipped.AssertionError: expected [ { id: 'p-existing', ...(16) } ] not to be [ { id: 'p-existing', ...(16) } ]atexpect(rp.proposals.value).not.toBe(queueBeforePoll). The queue that had just been read was discarded and the pre-poll array was still the rendered authority: the freeze, exactly.npm run typecheckwith the newautomationApi.spec.tscases and no type change —src/tests/api/automationApi.spec.ts(91,7): error TS2353: Object literal may only specify known properties, and 'expectedStatuses' does not exist in type 'AutomationReadOptions'.expected false to be trueatexpect(unavailable.exists()).toBe(true)— no panel, and the dropped row still on screen. The probe was reverted before the commit; the committed source diff contains no probe.Green, at head
2d6dc3b95:npx vitest --run --maxWorkers=2 src/tests/composables/useReviewProposals.spec.ts src/tests/api/automationApi.spec.ts src/tests/views/ReviewView.spec.ts— 3 files, 201 passed.npx vitest --run --maxWorkers=2 src/tests/composables/useReviewProposals.spec.ts src/tests/composables/useReviewActions.spec.ts src/tests/views/ReviewView.spec.ts src/tests/views/ReviewView.coverage.spec.ts src/tests/api— 37 files, 531 passed.npx vitest --run --maxWorkers=2 src/tests/views/paper/reviewas a read-only regression run — 14 files, 346 passed. No file under it was edited.npm run typecheck— clean.npx eslintover the five changed files — clean, exit 0.npm run build— exit 0, no error lines.git diff --check origin/main...HEAD— clean.NOT verified:
[ApiController]plus[HttpGet("{id}")] GetProposal(Guid id)inbackend/src/Taskdeck.Api/Controllers/AutomationProposalsController.cs, not observed against a running API. If that route ever answered a malformed id with something other than 400, this slice would not cover it, and the freeze would return for that status.PaperReviewView.spec.tsis held by fix: guard deferred review receipts by decision locus #2629.backend/changed.Boundaries and risks
expectedStatusesis per-request config, so the suppression is scoped to this one call site. The list leg, the explicit deep-link read, and every other automation call keep their normal error logging.GET /automation/proposals/{id}that returns 400 for a reason other than a malformed id would be read as "this pin is unavailable" rather than logged. Today the route takes no query or body, so a 400 there can only be model binding.it.eachname for the 403/404 case now reads 400, 403 and 404. No other spec depends on that name.dist/andnode_modules/, both regenerable. Nothing was copied out; nothing needs to be.