fix(review): a malformed deep-link target no longer freezes the queue refresh, and refused pins stop logging as API errors

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 = {

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

@@ -158,6 +158,32 @@ 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-<id>` 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.
+ *
+ * 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
+  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,
@@ -189,9 +215,15 @@ export function useReviewProposals() {
 
   const proposals = ref<ApiProposal[]>([])
   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<string | null>(null)
   // Set when a background read is refused with 403 (board access revoked
   // mid-session). The surfaces swap the ordinary "Nothing waiting" empty state
@@ -832,7 +864,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 },
@@ -864,9 +909,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

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<string, unknown>
+    expect(config).not.toHaveProperty('expectedStatuses')
+  })
+
   it('sends idempotency key when executing proposal', async () => {
     vi.mocked(http.post).mockResolvedValue({ data: { id: 'p1' } })
 

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,16 +1245,25 @@ 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()
       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 +1307,68 @@ describe('useReviewProposals', () => {
       },
     )
 
+    // #2214 item 8: a malformed `#proposal-<id>` (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'

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-<id>` 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

frontend/taskdeck-web/src/views/LegacyReviewView.vue

@@ -381,8 +381,9 @@ onUnmounted(() => {
       </div>
     </div>
 
-    <!-- A hash-pinned proposal the server refused (403/404) is an identity
-         failure of the link the reviewer followed, not an empty queue (#2214).
+    <!-- A hash-pinned proposal the server refused or could not bind (400/403/404)
+         is an identity failure of the link the reviewer followed, not an empty
+         queue (#2214).
          Saying so — and offering the way back — is the whole difference between
          "your link is dead" and "there is nothing to review". Ordered before
          the empty state, and still requiring an empty render, so a pin that has