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 ea0bc2edf..55ceea711 100644 --- a/frontend/taskdeck-web/src/composables/useReviewProposals.ts +++ b/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-` 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([]) 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 @@ -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 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 e70da5bdc..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,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-` (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' 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 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(() => { -