diff --git a/web/src/lib/api/client.ts b/web/src/lib/api/client.ts index 2b23d685d..cf9afae36 100644 --- a/web/src/lib/api/client.ts +++ b/web/src/lib/api/client.ts @@ -2080,21 +2080,28 @@ export const api = { * Cross-workspace copy — the MUTATION (PLAN-2357 / TASK-2365). With * `archive_source: true` it is the MOVE. * - * ⚠ NEVER RETRY THIS CALL (DR-13). There is no idempotency key, so a - * retry after a request that already committed creates a DUPLICATE item, - * and a caller that lost the response cannot tell which happened. On a - * 500 `copy_failed` — or on ANY unstructured transport failure once this - * call has been dispatched — the outcome is unknown: tell the user to - * check the destination workspace, and do not offer a retry. + * `opKey` is sent as `op_key`, the server's idempotency key (TASK-3585): + * a second send of the same request under the same key answers the + * FIRST copy's stored result and copies nothing. A refusal releases the + * key, so a corrected resend may reuse it; a different request under a + * key that already completed is 422 `idempotency_key_reused`. Callers + * mint one key per dialog open (TASK-3597). + * + * ⚠ Still NEVER RETRY THIS CALL WITHOUT A KEY (DR-13): a keyless retry + * after a request that already committed creates a DUPLICATE item, and a + * caller that lost the response cannot tell which happened. On a 500 + * `copy_failed`, or on ANY unstructured transport failure once this call + * has been dispatched, the outcome is unknown: tell the user to check + * the destination workspace. * * Deliberately no `signal` param: aborting tells you nothing about * whether the server committed, and the shared `request` never retries - * non-idempotent methods, which is what keeps this aligned with DR-13. + * non-idempotent methods. */ - copy: (ws: string, slug: string, req: ItemCopyPreflightRequest) => + copy: (ws: string, slug: string, req: ItemCopyPreflightRequest, opKey?: string) => request(`/workspaces/${ws}/items/${slug}/copy`, { method: 'POST', - body: JSON.stringify(req) + body: JSON.stringify(opKey ? { ...req, op_key: opKey } : req) }), /** @@ -2278,18 +2285,33 @@ export const api = { * broadcast call sites (QuickActionsMenu's S4 dispatch) are * unaffected by this parameter's addition. * - * NEVER auto-retry this. Like the copy endpoint it carries no - * idempotency key, so a retry after an ambiguous failure can deliver - * the same instruction twice. A targeted miss (`delivered_sessions - * === 0` in the response) is the one exception a caller may act on - * safely — see ItemPushResult's doc comment. + * `opKey` is sent as `op_key`, the server's idempotency key (TASK-3585): + * a second send of the same push under the same key replays the stored + * answer and publishes nothing. The server stores EVERY completed + * answer, a targeted miss (`delivered_sessions === 0`) included, so a + * caller that resends after a miss needs a NEW key: the old one would + * replay the miss, or answer 422 `idempotency_key_reused` for a new + * target. A pre-publish refusal releases the key. + * + * NEVER auto-retry this without a key: a keyless retry after an + * ambiguous failure can deliver the same instruction twice. A targeted + * miss is the one outcome a caller may act on safely — see + * ItemPushResult's doc comment. */ - push: (ws: string, itemSlug: string, message: string, targetSessionId?: string) => + push: ( + ws: string, + itemSlug: string, + message: string, + targetSessionId?: string, + opKey?: string + ) => request(`/workspaces/${ws}/items/${itemSlug}/push`, { method: 'POST', - body: JSON.stringify( - targetSessionId ? { message, target_session_id: targetSessionId } : { message } - ) + body: JSON.stringify({ + message, + ...(targetSessionId ? { target_session_id: targetSessionId } : {}), + ...(opKey ? { op_key: opKey } : {}) + }) }) }, diff --git a/web/src/lib/api/pushCopyOpKey.test.ts b/web/src/lib/api/pushCopyOpKey.test.ts new file mode 100644 index 000000000..ce4555951 --- /dev/null +++ b/web/src/lib/api/pushCopyOpKey.test.ts @@ -0,0 +1,70 @@ +// TASK-3597 — the copy and push calls carry the server's idempotency key as +// `op_key` in the body (TASK-3585), and only when the caller passed one: a +// keyless call keeps the exact body it always sent. +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { api } from './client'; + +function okFetch(body: unknown) { + return vi.fn( + async (_url: string, _init?: RequestInit) => + new Response(JSON.stringify(body), { + status: 200, + headers: { 'Content-Type': 'application/json' } + }) + ); +} + +function sentBody(fn: ReturnType): unknown { + const init = fn.mock.calls[0]?.[1]; + return JSON.parse(String(init?.body)); +} + +beforeEach(() => vi.unstubAllGlobals()); +afterEach(() => vi.unstubAllGlobals()); + +describe('api.items.push body', () => { + const ok = { ref: 'TASK-5', workspace: 'ws', pushed: true, message: 'ok' }; + + it('without a target or key: exactly { message }, the pre-S5 broadcast body', async () => { + const fn = okFetch(ok); + vi.stubGlobal('fetch', fn); + await api.items.push('ws', 'task-5', 'hi'); + expect(sentBody(fn)).toStrictEqual({ message: 'hi' }); + }); + + it('an undefined target sends no target_session_id key at all', async () => { + const fn = okFetch(ok); + vi.stubGlobal('fetch', fn); + await api.items.push('ws', 'task-5', 'hi', undefined, 'k-1'); + expect(sentBody(fn)).toStrictEqual({ message: 'hi', op_key: 'k-1' }); + }); + + it('target and key both ride in the body', async () => { + const fn = okFetch(ok); + vi.stubGlobal('fetch', fn); + await api.items.push('ws', 'task-5', 'hi', 'sess-1', 'k-1'); + expect(sentBody(fn)).toStrictEqual({ + message: 'hi', + target_session_id: 'sess-1', + op_key: 'k-1' + }); + }); +}); + +describe('api.items.copy body', () => { + const req = { target_workspace: 'dst', target_collection: 'tasks' }; + + it('without a key: the request exactly as given', async () => { + const fn = okFetch({}); + vi.stubGlobal('fetch', fn); + await api.items.copy('ws', 'task-5', req as never); + expect(sentBody(fn)).toStrictEqual(req); + }); + + it('with a key: the request plus op_key', async () => { + const fn = okFetch({}); + vi.stubGlobal('fetch', fn); + await api.items.copy('ws', 'task-5', req as never, 'k-2'); + expect(sentBody(fn)).toStrictEqual({ ...req, op_key: 'k-2' }); + }); +}); diff --git a/web/src/lib/components/common/QuickActionsMenu.svelte b/web/src/lib/components/common/QuickActionsMenu.svelte index 9f7ec7ad5..902ef5909 100644 --- a/web/src/lib/components/common/QuickActionsMenu.svelte +++ b/web/src/lib/components/common/QuickActionsMenu.svelte @@ -181,8 +181,8 @@ let presence = $state(null); /** Wall-clock of the last SUCCESSFUL presence read, for the expiry above. */ let lastAnsweredAt = 0; - /** True while a push is in flight. The endpoint has NO idempotency key, so - * a second dispatch would deliver the instruction twice. */ + /** True while a push is in flight. This call site sends no idempotency + * key, so a second dispatch would deliver the instruction twice. */ let dispatching = $state(false); // Fences for the presence reads. Plain `let`s, never $state: read and @@ -360,8 +360,8 @@ // poll interval after a 'known' answer expired. const known = currentPresence(); const knownCount = known?.state === 'known' ? known.count : 0; - // IDENTITY fence (BUG-3105). The push has no idempotency key, so the - // fence refuses to REPORT an outcome under the next identity; it never + // IDENTITY fence (BUG-3105). This push carries no idempotency key, so + // the fence refuses to REPORT an outcome under the next identity; it never // re-sends. The toast describes a push the previous user made. const isSameIdentity = authStore.identityFence(); const route = routePrompt(prompt, target, known); @@ -395,14 +395,14 @@ // A push is not tied to the gesture, so a stale excerpt is ASKED about. // Only captured values are used after this await, and the identity - // fence is re-checked: the push has no idempotency key. + // fence is re-checked: this push carries no idempotency key. if (staleExcerpt && !(await pendingEditsDialog.request(itemLabel, 'excerpt', excerptSetAside ? 'set_aside' : 'pending'))) return; if (!isSameIdentity()) return; dispatching = true; try { - // NEVER retried automatically, here or anywhere else: the endpoint - // carries no idempotency key. + // NEVER retried automatically, here or anywhere else: this call + // sends no idempotency key. const result = await api.items.push(ws, target, collapsePushMessage(prompt)); if (!isSameIdentity()) return; // The SERVER's count wins over the preflight one (codex round 5). diff --git a/web/src/lib/components/fields/FieldEditor.svelte b/web/src/lib/components/fields/FieldEditor.svelte index e990be9b4..e80fd171b 100644 --- a/web/src/lib/components/fields/FieldEditor.svelte +++ b/web/src/lib/components/fields/FieldEditor.svelte @@ -662,7 +662,7 @@ handlers — onchange is never called. // optimistic-concurrency check already enforces. // // 2. A LOST RESPONSE on a create that actually committed. Real, and not - // fixable here: `item create` has no idempotency key, and titles are + // fixable here: this create sends no idempotency key, and titles are // not unique (colliding slugs just get `-2` suffixes, // `store.uniqueSlug`). Nothing auto-retries — the retry would be a // person clicking Create again, seeing the picker's current state — diff --git a/web/src/lib/components/items/CopyItemDialog.svelte b/web/src/lib/components/items/CopyItemDialog.svelte index e88351925..7e80b80dd 100644 --- a/web/src/lib/components/items/CopyItemDialog.svelte +++ b/web/src/lib/components/items/CopyItemDialog.svelte @@ -36,8 +36,9 @@ mutation dispatched?", NOT by the server returning `copy_failed`. Once the copy POST is dispatched, any UNSTRUCTURED failure (rejected fetch, non-JSON 502, timeout) is as ambiguous to the user as an explicit `copy_failed`: the server may have committed. Both render the same outcome-unknown state, which -PROHIBITS retry (there is no idempotency key) and sends the user to look at the -destination. Structured PRE-WRITE refusals — `collection_not_found`, a +offers no retry and sends the user to look at the destination. The copy does +carry an idempotency key (TASK-3597, one per opening), but it is double-submit +protection, not a recovery path. Structured PRE-WRITE refusals — `collection_not_found`, a destination `forbidden`, `plan_limit_exceeded`, the validation family — keep their own recovery paths; swallowing those into outcome-unknown would send the user hunting for an item that provably does not exist. @@ -210,6 +211,14 @@ user hunting for an item that provably does not exist. /** Outcome unknown — the copy may or may not have committed. Terminal: * Confirm stays locked, and no retry is offered (DR-13). */ let outcomeUnknown = $state(''); + /** + * The copy's idempotency key (TASK-3597): one per opening, so a second + * dispatch of the same request answers the first copy's stored result + * instead of copying twice. Every way the dialog re-arms Confirm after a + * dispatch is a refusal, which the server does not store, so the corrected + * request may reuse it. Plain `let`: only handlers read it. + */ + let opKey = ''; /** The preview changed between review and submit — advisory only (see * `reviewFingerprint`). Requires an explicit re-confirmation. */ let staleReview = $state(''); @@ -388,6 +397,7 @@ user hunting for an item that provably does not exist. submitting = false; submitError = ''; outcomeUnknown = ''; + opKey = crypto.randomUUID(); staleReview = ''; destinationLost = ''; void loadWorkspaces(); @@ -886,9 +896,9 @@ user hunting for an item that provably does not exist. const req = buildRequest(); // IDENTITY fence (BUG-3105), folded into `superseded` so every check // below also asks it. The three generations are navigation/edit fences - // and none moves on a sign-out; this path ends in a COPY with no - // idempotency key, so the refusal must land before the dispatch — a - // fence after it could only refuse to REPORT a write already made. + // and none moves on a sign-out; this path ends in a COPY, so the + // refusal must land before the dispatch — a fence after it could only + // refuse to REPORT a write already made. const isSameIdentity = authStore.identityFence(); const superseded = () => gen !== flowGen || @@ -963,12 +973,12 @@ user hunting for an item that provably does not exist. // IDENTITY fence (BUG-3105). handleConfirm refuses BEFORE this is called; // this one covers the other side of the copy's await, where the write has // already happened under the captured identity. It REFUSES to report and - // never re-sends: the copy has no idempotency key (DR-13), so a retry - // duplicates the item. Nothing here may be read as a recovery path. + // never re-sends (DR-13). Nothing here may be read as a recovery path. const isSameIdentity = authStore.identityFence(); + const key = opKey; submitting = true; try { - const result = await api.items.copy(sourceWsSlug, item.slug, req); + const result = await api.items.copy(sourceWsSlug, item.slug, req, key); if (gen !== flowGen) return; if (!isSameIdentity()) return; oncopied(result); diff --git a/web/src/lib/components/items/PushToAgentDialog.svelte b/web/src/lib/components/items/PushToAgentDialog.svelte index c3309ea3c..d57721069 100644 --- a/web/src/lib/components/items/PushToAgentDialog.svelte +++ b/web/src/lib/components/items/PushToAgentDialog.svelte @@ -203,13 +203,23 @@ always a broadcast that went out. /** * Set when a send failed in a way that leaves the OUTCOME UNKNOWN — the * request was dispatched but the server never told us what happened to it. - * Push is not re-armed in that state: the endpoint has no idempotency key, - * so a second click can publish the same instruction twice. Same rule + * Push is not re-armed in that state: the outcome is unknown, so the user + * checks their agent session rather than trusting a resend. Same rule * CopyItemDialog applies for the same reason (DR-13), and drawn at the same * line — "was the mutation dispatched?", not "did the server say it failed?". */ let outcomeUnknown = $state(false); + /** + * The push's idempotency key (TASK-3597): one per opening, so a second send + * of the same push replays the first one's answer instead of publishing + * twice. A pre-publish refusal releases it on the server, so a corrected + * resend reuses it. A targeted miss is a COMPLETED answer the server stores + * under the key, so that path mints a new one before the dialog re-arms. + * Plain `let`: only handlers read it. + */ + let opKey = ''; + /** * Fences for async writes. Plain `let`s, never $state: read and written only * inside handlers, never in reactive position. @@ -388,6 +398,7 @@ always a broadcast that went out. sending = false; sendError = ''; outcomeUnknown = false; + opKey = crypto.randomUUID(); sessions = []; presenceState = 'checking'; presenceReason = ''; @@ -452,9 +463,10 @@ always a broadcast that went out. // while `sending` disables the picker, but the pattern is load-bearing // elsewhere in this file and cheap to keep consistent here too. const target = selectedSessionId; + const key = opKey; // IDENTITY fence (BUG-3105). Neither `gen` nor `destroyed` moves on a - // sign-out. The push has no idempotency key, so this fence refuses to - // REPORT under the next identity and never re-sends. That includes the + // sign-out. This fence refuses to REPORT under the next identity and + // never re-sends. That includes the // toasts, which otherwise fire even for a destroyed instance (see // below): the push was real, but it was the PREVIOUS user's, and naming // its item to whoever is signed in now is the leak. The busy flag is @@ -464,12 +476,9 @@ always a broadcast that went out. sending = true; sendError = ''; try { - // NEVER retried automatically, at this call site or any other: the - // endpoint carries no idempotency key. Broadcast keeps the exact - // pre-S5 3-argument call — only a non-empty target adds the 4th. - const result = target - ? await api.items.push(wsSlug, itemSlug, collapsed, target) - : await api.items.push(wsSlug, itemSlug, collapsed); + // NEVER retried automatically. An empty target sends no + // target_session_id at all, the pre-S5 broadcast body. + const result = await api.items.push(wsSlug, itemSlug, collapsed, target || undefined, key); if (!isSameIdentity()) { sending = false; return; @@ -519,6 +528,9 @@ always a broadcast that went out. // live target. toastStore.show('that session is gone — refresh the list', 'error'); if (!stillMine()) return; + // The server stored this miss under `key`; a resend under it + // would replay the miss (or 422 for another target). + opKey = crypto.randomUUID(); selectedSessionId = ''; void refreshPresence(gen); return; diff --git a/web/src/lib/components/items/PushToAgentDialog.svelte.test.ts b/web/src/lib/components/items/PushToAgentDialog.svelte.test.ts index a52046cad..7cea22481 100644 --- a/web/src/lib/components/items/PushToAgentDialog.svelte.test.ts +++ b/web/src/lib/components/items/PushToAgentDialog.svelte.test.ts @@ -248,7 +248,13 @@ describe('PushToAgentDialog — message handling', () => { // Whitespace-collapsed exactly as the server would collapse it — so what // the user sees in the counter is what actually goes on the wire. - expect(pushMock).toHaveBeenCalledWith('docapp', 'fix-the-thing', 'line one line two'); + expect(pushMock).toHaveBeenCalledWith( + 'docapp', + 'fix-the-thing', + 'line one line two', + undefined, + expect.any(String) + ); }); it('blocks an over-length message client-side rather than letting it 400', async () => { @@ -549,13 +555,14 @@ describe('PushToAgentDialog — session targeting (PLAN-2558 S5, TASK-2588)', () await fireEvent.click(button('Push')); await tick(); - // No 4th argument at all — not even an explicit `undefined` — so this - // stays byte-identical to every pre-S5 broadcast call site - // (QuickActionsMenu's S4 dispatch included). + // No target: the client then sends no target_session_id at all, the + // pre-S5 broadcast body (pinned in pushCopyOpKey.test.ts). expect(pushMock).toHaveBeenCalledWith( 'docapp', 'fix-the-thing', - 'Take a look at TASK-5 — Fix the thing' + 'Take a look at TASK-5 — Fix the thing', + undefined, + expect.any(String) ); }); @@ -573,7 +580,8 @@ describe('PushToAgentDialog — session targeting (PLAN-2558 S5, TASK-2588)', () 'docapp', 'fix-the-thing', 'Take a look at TASK-5 — Fix the thing', - 'session-id-1' + 'session-id-1', + expect.any(String) ); }); @@ -674,12 +682,13 @@ describe('PushToAgentDialog — session targeting (PLAN-2558 S5, TASK-2588)', () await fireEvent.click(button('Push')); await tick(); - // The pre-S5 3-argument shape — no target_session_id at all, not - // even the now-dead 'session-id-0'. + // No target at all, not even the now-dead 'session-id-0'. expect(pushMock).toHaveBeenCalledWith( 'docapp', 'fix-the-thing', - 'Take a look at TASK-5 — Fix the thing' + 'Take a look at TASK-5 — Fix the thing', + undefined, + expect.any(String) ); }); @@ -708,7 +717,8 @@ describe('PushToAgentDialog — session targeting (PLAN-2558 S5, TASK-2588)', () 'docapp', 'fix-the-thing', 'Take a look at TASK-5 — Fix the thing', - 'session-id-0' + 'session-id-0', + expect.any(String) ); }); @@ -800,13 +810,85 @@ describe('PushToAgentDialog — connected vs accepting (PLAN-2613 S4)', () => { sessionsListMock.mockResolvedValue(armedSessions(2, 1)); await mountSettled(); - // Default is broadcast; send carries the pre-S5 3-arg shape. + // Default is broadcast; send carries no target. await fireEvent.click(button('Push')); await tick(); expect(pushMock).toHaveBeenCalledWith( 'docapp', 'fix-the-thing', - 'Take a look at TASK-5 — Fix the thing' + 'Take a look at TASK-5 — Fix the thing', + undefined, + expect.any(String) ); }); }); + +describe('PushToAgentDialog — idempotency key (TASK-3597)', () => { + const keyOf = (call: number) => pushMock.mock.calls[call]?.[4] as string | undefined; + + it('a resend after a pre-publish refusal reuses the key: the server released it, and a double send replays rather than publishes twice', async () => { + pushMock.mockRejectedValueOnce( + new PadApiError({ code: 'bad_request', message: 'message must not be empty' }) + ); + await mountSettled(); + + await fireEvent.click(button('Push')); + await tick(); + await tick(); + await fireEvent.click(button('Push')); + await tick(); + await tick(); + + expect(pushMock).toHaveBeenCalledTimes(2); + expect(keyOf(0)).toMatch(/^[0-9a-f-]{36}$/); + expect(keyOf(1)).toBe(keyOf(0)); + }); + + it('a resend after a targeted miss mints a NEW key, because the server stored the miss under the old one', async () => { + sessionsListMock.mockResolvedValue(sessions(2)); + pushMock.mockResolvedValueOnce({ + ref: 'TASK-5', + workspace: 'docapp', + pushed: true, + message: 'ok', + delivered_sessions: 0 + }); + await mountSettled(); + + const picker = targetPicker(); + if (!picker) throw new Error('expected a target picker'); + await fireEvent.change(picker, { target: { value: 'session-id-0' } }); + await fireEvent.click(button('Push')); + await tick(); + await tick(); + await fireEvent.click(button('Push')); + await tick(); + await tick(); + + expect(pushMock).toHaveBeenCalledTimes(2); + expect(keyOf(0)).toBeTruthy(); + expect(keyOf(1)).toBeTruthy(); + expect(keyOf(1)).not.toBe(keyOf(0)); + }); + + it('each opening gets its own key', async () => { + const { rerender } = await mountSettled(); + await fireEvent.click(button('Push')); + await tick(); + await tick(); + + await rerender(baseProps({ open: false })); + await tick(); + await rerender(baseProps({ open: true })); + await tick(); + await tick(); + flushSync(); + await fireEvent.click(button('Push')); + await tick(); + await tick(); + + expect(pushMock).toHaveBeenCalledTimes(2); + expect(keyOf(1)).toBeTruthy(); + expect(keyOf(1)).not.toBe(keyOf(0)); + }); +}); diff --git a/web/src/lib/push/dispatch.ts b/web/src/lib/push/dispatch.ts index 55a24511e..45db1e04f 100644 --- a/web/src/lib/push/dispatch.ts +++ b/web/src/lib/push/dispatch.ts @@ -220,8 +220,8 @@ export function describeDispatch(outcome: DispatchOutcome): DispatchMessage { // Not an error and not a success: the server gave no clear answer, so // the only true thing to say is that we don't know. Copying for the // user here would be worse than useless — if the push DID land, a - // paste delivers the same instruction twice, and the endpoint has no - // idempotency key to absorb it. So the copy is offered, not taken. + // paste delivers the same instruction twice, and a paste goes around + // the endpoint's idempotency key, so nothing absorbs it. So the copy is offered, not taken. return { message: 'The server didn’t say whether the push went through. Check your agent session before sending it again.', diff --git a/web/src/lib/types/index.ts b/web/src/lib/types/index.ts index 65c66327a..0c363a2a3 100644 --- a/web/src/lib/types/index.ts +++ b/web/src/lib/types/index.ts @@ -1221,8 +1221,8 @@ export interface ItemCopyPreflight { // Override semantics are identical too: an undeclared key is a 400, and a // `null` value unsets rather than writing null. // -// ⚠ NEVER RETRY THIS CALL AUTOMATICALLY (PLAN-2357 DR-13). There is no -// idempotency key in v1, so a retry after a request that already committed +// ⚠ NEVER RETRY THIS CALL AUTOMATICALLY (PLAN-2357 DR-13) without the same +// `op_key` (TASK-3585): a keyless retry after a request that already committed // creates a DUPLICATE item — and a client that timed out cannot tell which // happened. On 500 `copy_failed` the server is explicitly telling you the // outcome is unknown: show the message, and send the user to look at the