From 04ff09f0835f469895fc853d66cccb407726f4f0 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 07:12:54 +0100 Subject: [PATCH 1/4] fix(review): a malformed deep-link target marks only the pin unavailable 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 --- .../src/composables/useReviewProposals.ts | 27 +++++++- .../composables/useReviewProposals.spec.ts | 68 ++++++++++++++++++- 2 files changed, 92 insertions(+), 3 deletions(-) diff --git a/frontend/taskdeck-web/src/composables/useReviewProposals.ts b/frontend/taskdeck-web/src/composables/useReviewProposals.ts index ea0bc2edf..b0c0a607c 100644 --- a/frontend/taskdeck-web/src/composables/useReviewProposals.ts +++ b/frontend/taskdeck-web/src/composables/useReviewProposals.ts @@ -158,6 +158,27 @@ function isForbiddenError(err: unknown): boolean { return (err as { response?: { status?: number } }).response?.status === 403 } +/** + * A 400 on a PROPOSAL-LEVEL read means the id the URL names is not one this + * route can bind: `GetProposal(Guid id)` under `[ApiController]` answers a + * non-GUID `#proposal-` with a model-binding 400 before the handler runs, + * and nothing client-side validates the hash. That is a permanent fact about + * the requested target, exactly like a 403 or a 404 — no later tick makes a + * malformed id readable. Routing it to the queue-level failure branch instead + * threw away a list answer that had already arrived, and did so silently: a 400 + * is not transient, so the failure counter reset rather than climbing to the + * degraded threshold, and the refresh froze with no indication (#2214 item 8). + * + * Deliberately 400 alone. Only a malformed id is provably unusable; a 405 would + * be a routing defect affecting the whole surface rather than one target, and + * "gone" is already what a 404 says here. Neither is emitted by this route, so + * giving them pin-level meaning would be guessing. + */ +function isMalformedTargetError(err: unknown): boolean { + if (typeof err !== 'object' || err === null) return false + return (err as { response?: { status?: number } }).response?.status === 400 +} + function isTransientQueueRefreshFailure(err: unknown): boolean { const status = (err as { response?: { status?: number } } | null)?.response?.status // Network failures and deadline errors have no HTTP response. For HTTP, @@ -864,9 +885,11 @@ export function useReviewProposals() { // transient accounting below. if (controller.signal.aborted && !refreshTimedOut) return if (!isCurrentRead() && !refreshTimedOut) return - if (isForbiddenError(e) || isHttpNotFound(e)) { + if (isForbiddenError(e) || isHttpNotFound(e) || isMalformedTargetError(e)) { // The list itself succeeded, so only the pin is unavailable. Do not - // turn a proposal-level refusal into whole-queue revocation. + // turn a proposal-level refusal — or a target this route cannot even + // bind — into whole-queue revocation, and do not discard a queue + // answer that already arrived. pinUnavailable = true } else { // The composite read is incomplete. Preserve the exact queue and diff --git a/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts b/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts index e70da5bdc..28096bf3a 100644 --- a/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts +++ b/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts @@ -1248,7 +1248,11 @@ describe('useReviewProposals', () => { rp.stopQueueRefresh() }) - it.each([403, 404])( + // 400 shares this outcome exactly (#2214 item 8): the id in the hash is not + // one the by-id route can bind, which is as permanent a fact about that + // target as a refusal or a deletion. Running it through the same case keeps + // the three statuses provably identical, recovery half included. + it.each([400, 403, 404])( 'drops and marks only an omitted deep-link unavailable when its by-id read returns %s', async (status) => { vi.useFakeTimers() @@ -1292,6 +1296,68 @@ describe('useReviewProposals', () => { }, ) + // #2214 item 8: a malformed `#proposal-` (anything that is not a GUID) + // makes the by-id route answer with a model-binding 400. That 400 used to + // fall through to the transient-failure branch, which RETURNS before + // `proposals.value = next` -- so the list answer that had already arrived + // was discarded, and because a 400 is not transient the failure counter + // reset instead of climbing to the degraded threshold. The queue froze with + // no indication, on every tick, for as long as the bad link stayed in the + // URL. The list read succeeded, so its answer is the queue; only the pin is + // unusable, which is exactly the 403/404 outcome. + it('lands the readable queue and marks only the pin unavailable when a malformed deep-link target answers 400', async () => { + vi.useFakeTimers() + mockRoute.hash = '#proposal-not-a-guid' + mockAutomationApi.getProposals.mockResolvedValueOnce([ + makeProposal({ id: 'p-existing', createdAt: '2026-01-02T00:00:00Z' }), + ]) + mockAutomationApi.getProposal.mockRejectedValue({ response: { status: 400 } }) + const rp = useReviewProposals() + await rp.loadProposals() + // The explicit deep-link path is deliberately unchanged by this: only a + // 404 marks the target there, so a 400 still surfaces as a failure the + // reviewer asked for. + expect(rp.unavailableProposalId.value).toBeNull() + mockToast.error.mockClear() + + const queueBeforePoll = rp.proposals.value + mockAutomationApi.getProposals.mockResolvedValue([ + makeProposal({ id: 'p-server-new', createdAt: '2026-01-03T00:00:00Z' }), + ]) + const onQueueReplaced = vi.fn() + rp.startQueueRefresh(undefined, { onQueueReplaced }) + await rp.refreshProposals() + + expect(rp.proposals.value).not.toBe(queueBeforePoll) + expect(rp.proposals.value.map((p: any) => p.id)).toEqual(['p-server-new']) + expect(rp.unavailableProposalId.value).toBe('not-a-guid') + // A proposal-level refusal is not whole-queue revocation, and the queue on + // screen is the freshly read one, so nothing is degraded. + expect(rp.queueAccessRevoked.value).toBe(false) + expect(rp.queueRefreshStale.value).toBe(false) + // The explicit unavailable state owns this transition; the settled-row + // notice would otherwise win the render branch and hide it. + expect(onQueueReplaced).not.toHaveBeenCalled() + expect(mockToast.error).not.toHaveBeenCalled() + + // The consecutive-failure counter is private. The only thing it drives is + // the degraded warning at THRESHOLD consecutive transient failures, so + // polling that many more times with the same 400 pin proves it is not + // being incremented. + const pinReadsAfterFirstPoll = mockAutomationApi.getProposal.mock.calls.length + for (let i = 0; i < REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD; i += 1) { + await rp.refreshProposals() + } + expect(rp.queueRefreshStale.value).toBe(false) + expect(rp.proposals.value.map((p: any) => p.id)).toEqual(['p-server-new']) + // Matching the shipped 403/404 behaviour: nothing suppresses the pin read + // while the target stays outside the list, so every later tick retries it. + expect(mockAutomationApi.getProposal.mock.calls.length).toBe( + pinReadsAfterFirstPoll + REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD, + ) + rp.stopQueueRefresh() + }) + it('rechecks an unavailable deep-link and restores it when it becomes readable outside the list page', async () => { vi.useFakeTimers() mockRoute.hash = '#proposal-p-open' From d68e1248d9aafe6b11287a82415855e625835c60 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 07:15:29 +0100 Subject: [PATCH 2/4] fix(review): a refused background pin read stops logging as an API error 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 --- .../taskdeck-web/src/api/automationApi.ts | 10 +++++ .../src/composables/useReviewProposals.ts | 15 +++++++- .../src/tests/api/automationApi.spec.ts | 37 +++++++++++++++++++ .../composables/useReviewProposals.spec.ts | 11 ++++++ 4 files changed, 72 insertions(+), 1 deletion(-) diff --git a/frontend/taskdeck-web/src/api/automationApi.ts b/frontend/taskdeck-web/src/api/automationApi.ts index be5afbab8..85f5ef3e4 100644 --- a/frontend/taskdeck-web/src/api/automationApi.ts +++ b/frontend/taskdeck-web/src/api/automationApi.ts @@ -12,6 +12,16 @@ import { buildQueryString } from '../utils/queryBuilder' interface AutomationReadOptions { signal?: AbortSignal skipRetry?: boolean + /** + * Statuses that are an expected part of THIS call's contract, so the shared + * response interceptor stops logging them as 'API Error:' (#2214 item 7). + * The background review poll re-reads a hash-pinned proposal by id and turns + * 400/403/404 into an explicit "pin unavailable" outcome; logging those as + * failures reports a handled result as a defect on every tick. Opt-in per + * call site, exactly like `proposalDeepReviewApi.getProvenanceMetadata` -- + * the explicit deep-link read still logs and surfaces its failures. + */ + expectedStatuses?: number[] } export const automationApi = { diff --git a/frontend/taskdeck-web/src/composables/useReviewProposals.ts b/frontend/taskdeck-web/src/composables/useReviewProposals.ts index b0c0a607c..5062aeb7a 100644 --- a/frontend/taskdeck-web/src/composables/useReviewProposals.ts +++ b/frontend/taskdeck-web/src/composables/useReviewProposals.ts @@ -853,7 +853,20 @@ export function useReviewProposals() { // This is part of the background poll, not an explicit navigation. // Keep it in the same cancellation and fail-fast envelope as the // list request so teardown cannot leave a retry chain behind. - { skipRetry: true, signal: controller.signal }, + // + // The three statuses below are this call's own contract, not + // failures: each one is turned into the explicit "pin + // unavailable" outcome a few lines down. Without naming them the + // shared interceptor logs every refused or unbindable pin as + // 'API Error:' on every tick, which reports a handled result as a + // defect and buries the real ones (#2214 item 7). Scoped to this + // background read alone -- `openProposalFromHash` is a read the + // reviewer asked for and keeps its logging. + { + skipRetry: true, + signal: controller.signal, + expectedStatuses: [400, 403, 404], + }, ), controller, () => { refreshTimedOut = true }, diff --git a/frontend/taskdeck-web/src/tests/api/automationApi.spec.ts b/frontend/taskdeck-web/src/tests/api/automationApi.spec.ts index 7f943ec2d..44b244759 100644 --- a/frontend/taskdeck-web/src/tests/api/automationApi.spec.ts +++ b/frontend/taskdeck-web/src/tests/api/automationApi.spec.ts @@ -74,6 +74,43 @@ describe('automationApi', () => { expect(vi.mocked(http.get).mock.calls[0]).toEqual(['/automation/proposals/p1']) }) + // #2214 item 7. `expectedStatuses` tells the shared response interceptor which + // statuses are part of THIS call's contract, so it stops logging them as + // 'API Error:'. The background review poll re-reads a hash-pinned proposal by + // id and treats 400/403/404 as "this pin is unavailable" -- an outcome, not a + // failure -- so those answers must not read as errors in the console or in + // Sentry. The option only has that effect if it reaches the request config, + // which is what the first case pins. + it('forwards expectedStatuses on a single proposal read', async () => { + vi.mocked(http.get).mockResolvedValue({ data: { id: 'p1' } }) + const controller = new AbortController() + + await automationApi.getProposal('p1', { + skipRetry: true, + signal: controller.signal, + expectedStatuses: [400, 403, 404], + }) + + expect(http.get).toHaveBeenCalledWith('/automation/proposals/p1', { + skipRetry: true, + signal: controller.signal, + expectedStatuses: [400, 403, 404], + }) + }) + + // The option is opt-in per call site. A caller that does not name a status as + // expected must keep the interceptor's ordinary error logging, so the key must + // be absent rather than sent as undefined. + it('sends no expectedStatuses key when a read does not name one', async () => { + vi.mocked(http.get).mockResolvedValue({ data: { id: 'p1' } }) + const controller = new AbortController() + + await automationApi.getProposal('p1', { skipRetry: true, signal: controller.signal }) + + const config = vi.mocked(http.get).mock.calls[0][1] as Record + expect(config).not.toHaveProperty('expectedStatuses') + }) + it('sends idempotency key when executing proposal', async () => { vi.mocked(http.post).mockResolvedValue({ data: { id: 'p1' } }) diff --git a/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts b/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts index 28096bf3a..7ad9424a5 100644 --- a/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts +++ b/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts @@ -778,6 +778,12 @@ describe('useReviewProposals', () => { 'p-remote', expect.objectContaining({ signal: controller.signal, skipRetry: true }), ) + // It must NOT carry the background poll's `expectedStatuses` (#2214 item + // 7). This read was asked for, so its failures stay loggable; only the + // poll's pin leg turns those statuses into a handled outcome. + expect(mockAutomationApi.getProposal.mock.calls.at(-1)?.[1]).not.toHaveProperty( + 'expectedStatuses', + ) }) it('does not report landed until its deep-link lookup completes', async () => { @@ -1239,9 +1245,14 @@ describe('useReviewProposals', () => { summary: 'current summary', latestRevisionId: 'revision-2', })) + // Exact options, so a silently dropped one is caught here. The + // `expectedStatuses` entry is what keeps a refused or unbindable pin -- + // an outcome this leg handles explicitly -- from being logged as an API + // error on every tick (#2214 item 7). expect(mockAutomationApi.getProposal).toHaveBeenCalledWith('p-open', { skipRetry: true, signal: expect.any(AbortSignal), + expectedStatuses: [400, 403, 404], }) expect(onQueueReplaced).toHaveBeenCalledTimes(1) expect(rp.unavailableProposalId.value).toBeNull() From 2d6dc3b95d1703b0c377b2bfefd3a2609cff687b Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 07:17:12 +0100 Subject: [PATCH 3/4] test(review): pin the Legacy unavailable panel for a malformed 400 target 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 --- .../src/tests/views/ReviewView.spec.ts | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts b/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts index b8db4bd7f..652ce3d07 100644 --- a/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts +++ b/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts @@ -1570,6 +1570,47 @@ describe('ReviewView', () => { } }) + it('names a malformed pin the background read cannot bind, instead of freezing the queue (#2214)', async () => { + // A `#proposal-` that is not a GUID is answered 400 by the by-id route + // (model binding, before the handler). The background pin leg used to send + // that to the queue-level failure branch, which returns before the list + // answer is assigned: the row on screen stayed frozen forever and nothing + // said why. The pin is unusable; the queue read is not. + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'Date'] }) + try { + // Mount with the target IN the queue, so the explicit deep-link read + // never fires and this exercises the background leg alone. + const pinned = buildProposal({ id: 'proposal-not-a-guid' }) + mocks.getProposals.mockResolvedValue([pinned]) + + const { wrapper } = await mountAt('/workspace/review#proposal-proposal-not-a-guid') + expect(wrapper.find('#proposal-proposal-not-a-guid').exists()).toBe(true) + expect(wrapper.find('[data-testid="review-unavailable-target"]').exists()).toBe(false) + + // The next queue read no longer carries it, so the poll re-reads it by id + // and gets a 400 back. + mocks.getProposals.mockResolvedValue([]) + mocks.getProposal.mockRejectedValue({ response: { status: 400 } }) + + vi.advanceTimersByTime(REVIEW_QUEUE_REFRESH_MS) + await flushPromises() + await wrapper.vm.$nextTick() + + const unavailable = wrapper.find('[data-testid="review-unavailable-target"]') + expect(unavailable.exists()).toBe(true) + expect(unavailable.text()).toContain('proposal-not-a-guid') + // The list answer landed: the row it dropped is gone rather than retained + // by a discarded read. + expect(wrapper.find('#proposal-proposal-not-a-guid').exists()).toBe(false) + // A poll the reviewer never asked for stays silent, and a pin-level + // outcome is not a queue-level permission failure. + expect(mocks.errorToast).not.toHaveBeenCalled() + expect(wrapper.find('.td-review-empty').exists()).toBe(false) + } finally { + vi.useRealTimers() + } + }) + it('announces nothing from the queue live region while the queue is loading (#2214)', async () => { // The live region sits above the skeleton, so an ungated one reads "0 // proposals awaiting review." under the loading state and then the real From ab1ebcdccaa39c02f125bddd38082d073d113bca Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 07:30:49 +0100 Subject: [PATCH 4/4] docs(review): describe the three outcomes the unavailable pin now holds 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 --- .../src/composables/useReviewProposals.ts | 17 ++++++++++++++--- .../taskdeck-web/src/views/LegacyReviewView.vue | 5 +++-- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/frontend/taskdeck-web/src/composables/useReviewProposals.ts b/frontend/taskdeck-web/src/composables/useReviewProposals.ts index 5062aeb7a..55ceea711 100644 --- a/frontend/taskdeck-web/src/composables/useReviewProposals.ts +++ b/frontend/taskdeck-web/src/composables/useReviewProposals.ts @@ -173,6 +173,11 @@ function isForbiddenError(err: unknown): boolean { * be a routing defect affecting the whole surface rather than one target, and * "gone" is already what a 404 says here. Neither is emitted by this route, so * giving them pin-level meaning would be guessing. + * + * Sound for the BY-ID leg only, and it must not be moved to the outer catch: a + * 400 from the LIST read means the query was rejected (a malformed `boardId`, + * say), which says nothing about any pinned target and would silently downgrade + * a whole-queue failure into a pin-level outcome. */ function isMalformedTargetError(err: unknown): boolean { if (typeof err !== 'object' || err === null) return false @@ -210,9 +215,15 @@ export function useReviewProposals() { const proposals = ref([]) const proposalsLoading = ref(false) - // A deep link is an explicit request, not a selection preference. Preserve a - // confirmed 404 separately so Paper can say what happened instead of - // presenting the ordinary empty queue or a different actionable proposal. + // A deep link is an explicit request, not a selection preference. Hold the + // requested id separately whenever the server has settled its fate, so the + // surfaces can say what happened instead of presenting the ordinary empty + // queue or a different actionable proposal. Three outcomes reach it, all + // permanent facts about that one target rather than about the queue: a 404 + // (no such proposal, or it is gone), a 403 on the by-id read (this reviewer + // may not see it), and a 400 (the id in the hash is not one the by-id route + // can bind). A wrong-identity or cross-board answer fails closed into it too. + // Explicit navigation is narrower on purpose and still marks only the 404. const unavailableProposalId = ref(null) // Set when a background read is refused with 403 (board access revoked // mid-session). The surfaces swap the ordinary "Nothing waiting" empty state diff --git a/frontend/taskdeck-web/src/views/LegacyReviewView.vue b/frontend/taskdeck-web/src/views/LegacyReviewView.vue index 2638beb47..73ab1f305 100644 --- a/frontend/taskdeck-web/src/views/LegacyReviewView.vue +++ b/frontend/taskdeck-web/src/views/LegacyReviewView.vue @@ -381,8 +381,9 @@ onUnmounted(() => { -