From 0503e8b4b160b5e1fcde557ad422f5e1dababc1c Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 11 Jul 2026 19:23:10 +0300 Subject: [PATCH] =?UTF-8?q?fix(ai-chat):=20W1=20=E2=80=94=20=D0=BA=D0=BB?= =?UTF-8?q?=D0=B8=D0=B5=D0=BD=D1=82=20=D1=87=D0=B8=D1=82=D0=B0=D0=B5=D1=82?= =?UTF-8?q?=20409-=D0=BF=D0=BE=D0=BB=D0=B5=20activeRunId=20(=D0=B1=D1=8B?= =?UTF-8?q?=D0=BB=D0=BE=20runId=3Dundefined)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ревью волны 1 (agent_vscode): сервер эмитит id текущего рана в activeRunId на обоих 409-бранчах (SUPERSEDE_TARGET_MISMATCH, A_RUN_ALREADY_ACTIVE), а клиентский read409 читал runId → SUPERSEDE_MISMATCH{currentRunId} всегда undefined: быстрый supersede-хинт мёртв в проде, а клиентские тесты ложно-зелёные (мокали поле runId, которого сервер не шлёт). - read409 читает activeRunId (undefined-safe); оба мока — на реальную форму + ассерт на усвоенный currentRunId (mutation: revert→runId краснит тесты). - S4 (groundwork): activeRunId из A_RUN_ALREADY_ACTIVE поглощается в runFact (to(), без бампа эпохи/ownership — инварианты #488 целы). Полная обвязка supersede чужой вкладки из фазы error требует расширения gate в sendNow — отдельным follow-up; сейчас это безопасная заготовка, не рабочая фича. - spec.md: 2 строки контракт-таблицы приведены к activeRunId. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ai-chat/components/chat-thread.test.tsx | 82 ++++++++++++++++++- .../ai-chat/components/chat-thread.tsx | 27 +++--- .../features/ai-chat/state/run-fsm.spec.md | 6 +- .../features/ai-chat/state/run-fsm.test.ts | 21 +++++ .../src/features/ai-chat/state/run-fsm.ts | 11 ++- .../ai-chat/utils/error-message.test.ts | 4 +- 6 files changed, 133 insertions(+), 18 deletions(-) diff --git a/apps/client/src/features/ai-chat/components/chat-thread.test.tsx b/apps/client/src/features/ai-chat/components/chat-thread.test.tsx index e15f8030..ba4dcb7c 100644 --- a/apps/client/src/features/ai-chat/components/chat-thread.test.tsx +++ b/apps/client/src/features/ai-chat/components/chat-thread.test.tsx @@ -414,7 +414,9 @@ describe("ChatThread — send now", () => { "fetch", vi.fn().mockResolvedValue( new Response( - JSON.stringify({ code: "SUPERSEDE_TARGET_MISMATCH", runId: "run-x" }), + // REAL server body shape: the current run id is `activeRunId`, NOT `runId` + // (see ai-chat.controller.ts SUPERSEDE_TARGET_MISMATCH branch). + JSON.stringify({ code: "SUPERSEDE_TARGET_MISMATCH", activeRunId: "run-x" }), { status: 409 }, ), ), @@ -426,6 +428,84 @@ describe("ChatThread — send now", () => { expect(h.state.getRun).toHaveBeenCalledWith("c1"); }); + it("#497/W1: the mismatch ABSORBS the server's activeRunId into runFact (fast hint is live) — a follow-up Send now CAS-targets it", async () => { + // The 409 body's current run id is `activeRunId`; read409 must feed THAT into + // SUPERSEDE_MISMATCH{currentRunId} -> runFact, else the fast hint is undefined. + // Observe the absorbed fact via the NEXT CAS supersede body. Keep the verify + // getRun PENDING so it cannot overwrite the absorbed fact with its own result. + h.state.getRun.mockReturnValue(new Promise(() => {})); // verify never resolves + startLocalStreamWithRun(); // runFact run-1, sending, local, autonomous + fireEvent.click(screen.getByTestId("queue-btn")); // X + fireEvent.click(screen.getByLabelText("Send now")); // -> superseding (target run-1) + // A's onFinish sends B and CLEARS the pending-supersede text (no-overlap). + await act(async () => { + h.state.onFinish?.({ + message: { id: "a1", role: "assistant", parts: [] }, + isAbort: true, + isDisconnect: false, + isError: false, + }); + await Promise.resolve(); + }); + // B's CAS POST -> 409 SUPERSEDE_TARGET_MISMATCH with the REAL field `activeRunId`. + vi.stubGlobal( + "fetch", + vi.fn().mockResolvedValue( + new Response( + JSON.stringify({ code: "SUPERSEDE_TARGET_MISMATCH", activeRunId: "run-x" }), + { status: 409 }, + ), + ), + ); + await act(async () => { + await h.state.transport!.fetch!("http://x", { method: "POST", body: "{}" }); + }); + // runFact is now the absorbed "run-x". SEND_LOCAL preserves it; a fresh Send now + // then CAS-supersedes THAT run — surfacing runFact through the supersede body. + fireEvent.click(screen.getByTestId("send-btn")); // SEND_LOCAL -> sending, local + fireEvent.click(screen.getByTestId("queue-btn")); // Y + fireEvent.click(screen.getByLabelText("Send now")); // CAS -> arms pendingSupersede + const { body } = h.state.transport!.prepareSendMessagesRequest!({ + messages: [], + body: {}, + }); + // MUTATION-VERIFY: revert read409 to `runId` -> currentRunId undefined -> runFact + // stays "run-1" -> the CAS targets "run-1" -> this assertion reddens. + expect((body.supersede as { runId?: string } | undefined)?.runId).toBe("run-x"); + }); + + it("#497/S4: a plain 409 A_RUN_ALREADY_ACTIVE absorbs activeRunId into runFact so Send now CAS-targets the foreign run", async () => { + startLocalStreamWithRun(); // sending, local, autonomous, runFact run-1 + // A plain (non-supersede) POST hits the one-active-run gate. The FSM must adopt + // the server's activeRunId as the run-fact — NOT stay blind. + vi.stubGlobal( + "fetch", + vi.fn().mockResolvedValue( + new Response( + JSON.stringify({ code: "A_RUN_ALREADY_ACTIVE", activeRunId: "run-foreign" }), + { status: 409 }, + ), + ), + ); + // Drive a NON-supersede POST (phase is `sending`, not `superseding`). + await act(async () => { + await h.state.transport!.fetch!("http://x", { method: "POST", body: "{}" }); + }); + // runFact is now "run-foreign". SEND_LOCAL preserves it; Send now CAS-targets it. + fireEvent.click(screen.getByTestId("send-btn")); // SEND_LOCAL -> sending, local + fireEvent.click(screen.getByTestId("queue-btn")); // Y + fireEvent.click(screen.getByLabelText("Send now")); // CAS -> arms pendingSupersede + const { body } = h.state.transport!.prepareSendMessagesRequest!({ + messages: [], + body: {}, + }); + // MUTATION-VERIFY: drop the activeRunId threading (or read the wrong field) -> + // runFact stays "run-1" -> the CAS targets "run-1" -> this assertion reddens. + expect((body.supersede as { runId?: string } | undefined)?.runId).toBe( + "run-foreign", + ); + }); + it("#488 review-3 sibling: a plain 409 A_RUN_ALREADY_ACTIVE shows the classified banner", async () => { renderThread({ autonomousRunsEnabled: true, initialRows: settledTail() }); // The SDK sets useChat error to the 409 body on the failed POST. diff --git a/apps/client/src/features/ai-chat/components/chat-thread.tsx b/apps/client/src/features/ai-chat/components/chat-thread.tsx index 24db89b6..e213da32 100644 --- a/apps/client/src/features/ai-chat/components/chat-thread.tsx +++ b/apps/client/src/features/ai-chat/components/chat-thread.tsx @@ -87,15 +87,19 @@ function isActiveRunStatus(status: string | null | undefined): boolean { return status === "pending" || status === "running"; } -/** Read the `{ code, runId }` off a JSON error response WITHOUT consuming the - * original body (reads a clone), so the caller can still return the Response - * untouched to the SDK. Any parse failure => empty. */ -async function read409(response: Response): Promise<{ code?: string; runId?: string }> { +/** Read the `{ code, activeRunId }` off a JSON error response WITHOUT consuming + * the original body (reads a clone), so the caller can still return the Response + * untouched to the SDK. `activeRunId` is the server's field for the run currently + * active on the chat — it is the name emitted on BOTH 409 branches + * (SUPERSEDE_TARGET_MISMATCH and A_RUN_ALREADY_ACTIVE, see ai-chat.controller.ts). + * Reading `runId` here (the field the server never sends) silently yields + * `undefined`. Any parse failure => empty. */ +async function read409(response: Response): Promise<{ code?: string; activeRunId?: string }> { try { - const b = (await response.clone().json()) as { code?: unknown; runId?: unknown }; + const b = (await response.clone().json()) as { code?: unknown; activeRunId?: unknown }; return { code: typeof b?.code === "string" ? b.code : undefined, - runId: typeof b?.runId === "string" ? b.runId : undefined, + activeRunId: typeof b?.activeRunId === "string" ? b.activeRunId : undefined, }; } catch { return {}; @@ -486,11 +490,11 @@ export default function ChatThread({ if (response.ok) { dispatchRef.current({ type: "SUPERSEDE_READY", epoch: ep }); } else if (response.status === 409) { - const { code, runId } = await read409(response); + const { code, activeRunId } = await read409(response); if (code === "SUPERSEDE_TARGET_MISMATCH") dispatchRef.current({ type: "SUPERSEDE_MISMATCH", - currentRunId: runId, + currentRunId: activeRunId, epoch: ep, }); else if (code === "SUPERSEDE_TIMEOUT") @@ -499,9 +503,12 @@ export default function ChatThread({ dispatchRef.current({ type: "SUPERSEDE_INVALID", epoch: ep }); } } else if (response.status === 409) { - const { code } = await read409(response); + const { code, activeRunId } = await read409(response); if (code === "A_RUN_ALREADY_ACTIVE") - dispatchRef.current({ type: "RUN_ALREADY_ACTIVE" }); + // S4: thread the server's activeRunId into the event so the FSM can + // adopt it as the run-fact — a later "Send now" then CAS-supersedes + // that (possibly foreign-tab) run instead of a blind promote+abort. + dispatchRef.current({ type: "RUN_ALREADY_ACTIVE", activeRunId }); } return response; } diff --git a/apps/client/src/features/ai-chat/state/run-fsm.spec.md b/apps/client/src/features/ai-chat/state/run-fsm.spec.md index 7287012a..4f70f744 100644 --- a/apps/client/src/features/ai-chat/state/run-fsm.spec.md +++ b/apps/client/src/features/ai-chat/state/run-fsm.spec.md @@ -56,7 +56,7 @@ Legend: **†** = command-transition (bumps `epoch`, I1). Effects in `[…]`. | `SUPERSEDE_MISMATCH{currentRunId}` (409 SUPERSEDE_TARGET_MISMATCH) | superseding | error(supersede-mismatch) | `[postRun(verify)]`, runFact←currentRunId | | `SUPERSEDE_TIMEOUT` (409 SUPERSEDE_TIMEOUT) | superseding | error(supersede-timeout) | — (composer keeps text; no auto-retry) | | `SUPERSEDE_INVALID` (409 SUPERSEDE_INVALID) | superseding | error(supersede-invalid) | — | -| `RUN_ALREADY_ACTIVE` (409 A_RUN_ALREADY_ACTIVE, plain POST) | sending | error(run-already-active) | — (composer offers supersede; NO auto-retry) | +| `RUN_ALREADY_ACTIVE{activeRunId}` (409 A_RUN_ALREADY_ACTIVE, plain POST) | sending | error(run-already-active) | runFact←activeRunId (composer offers supersede; NO auto-retry) | | `DISPOSE` (unmount) | any | idle **†** | `[abortAttach, cancelReconnect, disarmPoll]` (I1/I5 — epoch++ kills late callbacks) | **`stopping` honors any finish (re-review MEDIUM):** BEFORE the epoch filter, a @@ -90,8 +90,8 @@ share exactly the dispatched event set. | Server response | Event dispatched | error kind → banner | |---|---|---| -| 409 `A_RUN_ALREADY_ACTIVE` (plain POST) | `RUN_ALREADY_ACTIVE` | run-already-active → "already answering / interrupt & send" | -| 409 `SUPERSEDE_TARGET_MISMATCH` (+ body.runId) | `SUPERSEDE_MISMATCH{currentRunId}` | supersede-mismatch → verify via /run | +| 409 `A_RUN_ALREADY_ACTIVE` (+ body.activeRunId) | `RUN_ALREADY_ACTIVE{activeRunId}` | run-already-active → "already answering / interrupt & send" | +| 409 `SUPERSEDE_TARGET_MISMATCH` (+ body.activeRunId) | `SUPERSEDE_MISMATCH{currentRunId}` | supersede-mismatch → verify via /run | | 409 `SUPERSEDE_TIMEOUT` | `SUPERSEDE_TIMEOUT` | supersede-timeout → "couldn't interrupt in time, resend" | | 409 `SUPERSEDE_INVALID` | `SUPERSEDE_INVALID` | supersede-invalid → "couldn't interrupt this run" | | 503 `A_RUN_BEGIN_FAILED` | `FINISH_ERROR{begin-failed}` | begin-failed → "could not start, temporary" | diff --git a/apps/client/src/features/ai-chat/state/run-fsm.test.ts b/apps/client/src/features/ai-chat/state/run-fsm.test.ts index d01913db..9ff49acb 100644 --- a/apps/client/src/features/ai-chat/state/run-fsm.test.ts +++ b/apps/client/src/features/ai-chat/state/run-fsm.test.ts @@ -309,6 +309,27 @@ describe("run-fsm — commit 5: supersede CAS + error classification", () => { expect(m.phase).toEqual({ name: "error", kind: "run-already-active" }); expect(m.effects).toEqual([]); }); + + it("#497/S4: RUN_ALREADY_ACTIVE{activeRunId} ADOPTS the server's active run as the run-fact", () => { + // The server sends `activeRunId` so a later supersede can TARGET that run + // instead of a blind promote+abort. Absorb it into runFact. + const m = reduce(run(initialMachine(), { type: "SEND_LOCAL" }), { + type: "RUN_ALREADY_ACTIVE", + activeRunId: "run-foreign", + }); + expect(m.phase).toEqual({ name: "error", kind: "run-already-active" }); + expect(m.ctx.runFact).toEqual({ runId: "run-foreign" }); + expect(m.effects).toEqual([]); + }); + + it("#497/S4: RUN_ALREADY_ACTIVE without an activeRunId keeps the prior run-fact", () => { + const seeded = reduce(run(initialMachine(), { type: "SEND_LOCAL" }), { + type: "RUN_FACT", + runFact: { runId: "run-prior" }, + }); + const m = reduce(seeded, { type: "RUN_ALREADY_ACTIVE" }); + expect(m.ctx.runFact).toEqual({ runId: "run-prior" }); + }); }); // #488 F2 — a late mount `getRun → ATTACH_START` must not hijack a local turn. diff --git a/apps/client/src/features/ai-chat/state/run-fsm.ts b/apps/client/src/features/ai-chat/state/run-fsm.ts index 34c858b2..b83bbfc5 100644 --- a/apps/client/src/features/ai-chat/state/run-fsm.ts +++ b/apps/client/src/features/ai-chat/state/run-fsm.ts @@ -171,7 +171,7 @@ export type Event = | { type: "SUPERSEDE_MISMATCH"; currentRunId?: string; epoch?: number } | { type: "SUPERSEDE_TIMEOUT"; epoch?: number } | { type: "SUPERSEDE_INVALID"; epoch?: number } - | { type: "RUN_ALREADY_ACTIVE" } + | { type: "RUN_ALREADY_ACTIVE"; activeRunId?: string } // -- lifecycle -- | { type: "DISPOSE" }; @@ -567,8 +567,13 @@ export function reduce(m: Machine, event: Event): Machine { case "RUN_ALREADY_ACTIVE": // A plain POST hit the one-active-run gate. NO auto-retry — the composer - // offers "interrupt and send" (supersede) instead. - return to(m, { name: "error", kind: "run-already-active" }); + // offers "interrupt and send" (supersede) instead. #497/S4: adopt the + // server's activeRunId as the run-fact so that supersede can TARGET the + // (possibly foreign-tab) active run via the CAS, rather than a blind + // promote+abort that just 409s again. A stale/absent id keeps the prior fact. + return to(m, { name: "error", kind: "run-already-active" }, { + ctx: { runFact: event.activeRunId ? { runId: event.activeRunId } : m.ctx.runFact }, + }); // ---- lifecycle ----------------------------------------------------- case "DISPOSE": diff --git a/apps/client/src/features/ai-chat/utils/error-message.test.ts b/apps/client/src/features/ai-chat/utils/error-message.test.ts index 594503f5..04d94225 100644 --- a/apps/client/src/features/ai-chat/utils/error-message.test.ts +++ b/apps/client/src/features/ai-chat/utils/error-message.test.ts @@ -56,8 +56,10 @@ describe("describeChatError", () => { }); it("classifies SUPERSEDE_TARGET_MISMATCH (409) as run-changed", () => { + // Real server body shape: the current run id is `activeRunId` (NOT `runId`) — + // see ai-chat.controller.ts. describeChatError classifies off `code` only. const body = - '{"message":"active run does not match the supersede target","code":"SUPERSEDE_TARGET_MISMATCH","runId":"run-x","statusCode":409}'; + '{"message":"active run does not match the supersede target","code":"SUPERSEDE_TARGET_MISMATCH","activeRunId":"run-x","statusCode":409}'; expect(describeChatError(body, t).title).toBe( "Couldn't interrupt — the run changed", );