fix(web): the copy and push dialogs send an idempotency key, one per opening (TASK-3597) (#2090)

The server has taken `op_key` on copy and push since TASK-3585; the web
dialogs never sent one. Each opening of CopyItemDialog and PushToAgentDialog
now mints a key, so a double dispatch replays the first answer instead of
copying or publishing twice. A refusal releases the key server-side, so a
corrected resend reuses it. A targeted push miss is a completed answer the
server stores, so the push dialog mints a new key before it re-arms;
otherwise the resend would replay the miss, or 422 for a new target.

The client methods take an optional opKey and add `op_key` to the body only
when given, so keyless bodies are byte-identical (pinned in
pushCopyOpKey.test.ts). Comments that said the copy or push endpoint has no
idempotency key now say the call site sends none.

Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS
This commit is contained in:
xarmian
2026-10-11 21:10:08 -04:00
committed by GitHub
parent 199fd208a8
commit 3595a3c034
9 changed files with 256 additions and 60 deletions
+40 -18
View File
@@ -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<ItemCopyResult>(`/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<ItemPushResult>(`/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 } : {})
})
})
},
+70
View File
@@ -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<typeof okFetch>): 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' });
});
});
@@ -181,8 +181,8 @@
let presence = $state<PushPresence | null>(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).
@@ -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 —
@@ -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);
@@ -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;
@@ -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));
});
});
+2 -2
View File
@@ -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.',
+2 -2
View File
@@ -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