diff --git a/frontend/src/components/chat/clarification-form.test.tsx b/frontend/src/components/chat/clarification-form.test.tsx index 8a1c4ec1cf..47c1102991 100644 --- a/frontend/src/components/chat/clarification-form.test.tsx +++ b/frontend/src/components/chat/clarification-form.test.tsx @@ -17,6 +17,10 @@ const appContextMock = vi.hoisted(() => ({ filesDisabled: false, providerAvailable: true, sendMessage: vi.fn(), + state: { + commandOutcomes: {} as Record, + clarificationSubmissions: {} as Record, + }, })) const toastErrorMock = vi.hoisted(() => vi.fn()) const mcpAppsMock = vi.hoisted(() => ({ @@ -899,3 +903,481 @@ describe("ClarificationForm blank option filtering", () => { expect(blankOptionSpans(container)).toHaveLength(0) }) }) + +describe("ClarificationForm terminal command outcomes", () => { + // Issue #1500: after a reply is durably accepted, its command can still + // reach a terminal disposition before a turn is established. Whether the + // form may invite a resend is decided by the structured outcomes the + // backend broadcasts for the round's tracked commands, never by task + // state alone. The accountable submissions live in context keyed by + // request id - an ordered list, so a resubmit after an ack timeout does + // not orphan the earlier command's outcome - and the gate survives the + // submitting component instance being replaced. + const ROUND = "inputreq_r1" + + beforeEach(() => { + appContextMock.dispatch.mockReset() + appContextMock.filesDisabled = false + appContextMock.providerAvailable = true + appContextMock.sendMessage.mockReset() + appContextMock.sendMessage.mockResolvedValue(undefined) + appContextMock.state = { commandOutcomes: {}, clarificationSubmissions: {} } + toastErrorMock.mockReset() + }) + + afterEach(() => { + cleanup() + }) + + const form = (active: boolean, requestId = ROUND) => ( + + ) + + const submitButton = () => + screen.queryByRole("button", { name: "chatPage.clarification.submit" }) + + // Mirrors the reducer: RECORD appends to the round's list. + const recordedSubmissions = () => + appContextMock.dispatch.mock.calls + .map(([action]) => action) + .filter((action) => action?.type === "RECORD_CLARIFICATION_SUBMISSION") + .map((action) => action.payload as { + requestId: string + commandId: string + accepted: boolean + }) + + const mirrorSubmissions = () => { + appContextMock.state = { + ...appContextMock.state, + clarificationSubmissions: { + [ROUND]: recordedSubmissions().map(({ commandId, accepted }) => ({ + commandId, + accepted, + })), + }, + } + } + + // Drives one accepted submission and mirrors the RECORD dispatch into the + // mock context state, the way the real reducer would. + const submitAccepted = async () => { + fireEvent.change(screen.getByRole("textbox"), { target: { value: "Beijing" } }) + fireEvent.click( + screen.getByRole("button", { name: "chatPage.clarification.submit" }), + ) + await waitFor(() => + expect(appContextMock.sendMessage).toHaveBeenCalledTimes( + recordedSubmissions().length, + ), + ) + const record = recordedSubmissions().at(-1) + expect(record).toEqual({ + requestId: ROUND, + commandId: expect.any(String), + accepted: true, + }) + // The recorded command id is the client message id the delivery used. + const config = appContextMock.sendMessage.mock.lastCall?.[1] as { + clientMessageId?: string + } + expect(record!.commandId).toBe(config.clientMessageId) + mirrorSubmissions() + return record!.commandId + } + + const withOutcomes = (outcomes: Record) => { + appContextMock.state = { + ...appContextMock.state, + commandOutcomes: Object.fromEntries( + Object.entries(outcomes).map(([commandId, resendSafe]) => [ + commandId, + { resendSafe }, + ]), + ), + } + } + + it("reactivates the form and preserves the draft when the outcome proves retry safe", async () => { + const { rerender } = render(form(true)) + const commandId = await submitAccepted() + await waitFor(() => expect(screen.queryByRole("textbox")).toBeNull()) + + rerender(form(false)) + withOutcomes({ [commandId]: true }) + rerender(form(true)) + + await waitFor(() => expect(submitButton()).toBeEnabled()) + expect(screen.getByRole("textbox")).toHaveValue("Beijing") + expect( + screen.getByText("chatPage.clarification.replyNotApplied"), + ).toBeInTheDocument() + // The record is consumed so the resend is armed exactly once - and the + // CLEAR names exactly the verified command, so a concurrently recorded + // submission would survive it. + expect(appContextMock.dispatch).toHaveBeenCalledWith({ + type: "CLEAR_CLARIFICATION_SUBMISSION", + payload: { requestId: ROUND, commandIds: [commandId] }, + }) + }) + + it("keeps the form locked and surfaces the ambiguity when the outcome is not proven safe", async () => { + const { rerender } = render(form(true)) + const commandId = await submitAccepted() + await waitFor(() => expect(screen.queryByRole("textbox")).toBeNull()) + + rerender(form(false)) + withOutcomes({ [commandId]: false }) + rerender(form(true)) + + // The notice is visible, the draft is intact, and nothing invites a + // duplicate submission of the accepted reply. + const alert = await screen.findByRole("alert") + expect(alert).toHaveTextContent("chatPage.clarification.replyOutcomeUnknown") + expect(screen.getByRole("textbox")).toHaveValue("Beijing") + expect(submitButton()).toBeDisabled() + }) + + it("locks and explains a reasserted round whose accepted reply is still in flight", async () => { + const { rerender } = render(form(true)) + await submitAccepted() + await waitFor(() => expect(screen.queryByRole("textbox")).toBeNull()) + + rerender(form(false)) + rerender(form(true)) + + // No outcome means the command may still be in flight: the form opens + // to explain the lock instead of showing a bare greyed-out button, and + // nothing invites a duplicate. + expect( + await screen.findByText("chatPage.clarification.replyPending"), + ).toBeInTheDocument() + expect(submitButton()).toBeDisabled() + }) + + it("is not reopened by a late terminal event after a turn is established", async () => { + const { rerender } = render(form(true)) + const commandId = await submitAccepted() + await waitFor(() => expect(screen.queryByRole("textbox")).toBeNull()) + + rerender(form(false)) + // The turn was established, the task is running, and only then does a + // stale resend-safe terminal frame arrive: with the task not waiting, + // nothing may reopen the submitted form. + withOutcomes({ [commandId]: true }) + rerender(form(false)) + + expect(screen.queryByRole("textbox")).toBeNull() + expect(submitButton()).toBeNull() + + // Only an authoritative return to waiting_for_user consumes the + // outcome and reactivates the form - this is the resend-safe branch + // actually running, so the test fails if the gating logic is removed. + rerender(form(true)) + await waitFor(() => expect(submitButton()).toBeEnabled()) + expect( + screen.getByText("chatPage.clarification.replyNotApplied"), + ).toBeInTheDocument() + }) + + it("gates a fresh component instance for a round another instance submitted", async () => { + // The virtual waiting message and the persisted timeline message render + // the same round in different component instances; replacing the + // submitting instance must not drop the gate. + render(form(true)) + const commandId = await submitAccepted() + cleanup() + + withOutcomes({ [commandId]: false }) + render(form(true)) + + const alert = await screen.findByRole("alert") + expect(alert).toHaveTextContent("chatPage.clarification.replyOutcomeUnknown") + expect(submitButton()).toBeDisabled() + }) + + it("locks a fresh component instance while the accepted reply is still in flight", async () => { + // A freshly mounted instance starts with isSubmitted=false, so without + // the in-flight lock it would offer Submit for a round whose durably + // accepted reply has not reached a terminal outcome yet (surfaced by + // review on #2126). + render(form(true)) + await submitAccepted() + cleanup() + + render(form(true)) + + await waitFor(() => expect(submitButton()).toBeDisabled()) + expect( + screen.getByText("chatPage.clarification.replyPending"), + ).toBeInTheDocument() + }) + + it("does not lock a fresh instance for an unconfirmed ack-timeout delivery", async () => { + appContextMock.state = { + commandOutcomes: {}, + clarificationSubmissions: { + [ROUND]: [{ commandId: "maybe-sent", accepted: false }], + }, + } + render(form(true)) + + // The reply may never have been accepted at all: the composer keeps its + // advisory retry until a terminal outcome for the command arrives. + await waitFor(() => expect(submitButton()).toBeEnabled()) + }) + + it("still surfaces an earlier command's unsafe outcome after a resubmit", async () => { + // The orphaned-outcome regression: an ack-timeout records command 1, + // the advisory retry sends command 2, and only then does command 1's + // terminal outcome arrive saying it may have been committed. The round + // must still lock and surface the ambiguity - a single-slot record + // would have overwritten command 1 and shown nothing. + appContextMock.sendMessage.mockRejectedValueOnce(Object.assign( + new Error("ack timed out"), + { disposition: "outcome_unknown", userFacing: true }, + )) + const { rerender } = render(form(true)) + + fireEvent.change(screen.getByRole("textbox"), { target: { value: "Beijing" } }) + fireEvent.click( + screen.getByRole("button", { name: "chatPage.clarification.submit" }), + ) + await waitFor(() => expect(recordedSubmissions()).toHaveLength(1)) + expect(recordedSubmissions()[0].accepted).toBe(false) + mirrorSubmissions() + + // The advisory retry resubmits; the second record appends. + await waitFor(() => expect(submitButton()).toBeEnabled()) + fireEvent.click( + screen.getByRole("button", { name: "chatPage.clarification.submit" }), + ) + await waitFor(() => expect(recordedSubmissions()).toHaveLength(2)) + mirrorSubmissions() + const [first, second] = recordedSubmissions() + expect(first.commandId).not.toBe(second.commandId) + + withOutcomes({ [first.commandId]: false }) + rerender(form(true)) + + const alert = await screen.findByRole("alert") + expect(alert).toHaveTextContent("chatPage.clarification.replyOutcomeUnknown") + expect(submitButton()).toBeDisabled() + }) + + it("withholds the proven-safe unlock notice while an unconfirmed reply is unresolved", async () => { + // Command 1's fate is unknown (ack timeout, no outcome); command 2 is + // proven not applied. The round returns to the advisory stance it + // already accepted for command 1 - but without the "not applied, safe + // to resend" promise, which cannot be made for command 1, and without + // consuming the record, so command 1's late outcome still gates. + appContextMock.state = { + commandOutcomes: { "cmd-2": { resendSafe: true } }, + clarificationSubmissions: { + [ROUND]: [ + { commandId: "cmd-1", accepted: false }, + { commandId: "cmd-2", accepted: true }, + ], + }, + } + render(form(true)) + + await waitFor(() => expect(submitButton()).toBeEnabled()) + expect( + screen.queryByText("chatPage.clarification.replyNotApplied"), + ).toBeNull() + expect(appContextMock.dispatch).not.toHaveBeenCalledWith( + expect.objectContaining({ type: "CLEAR_CLARIFICATION_SUBMISSION" }), + ) + }) + + it("returns a pending-locked instance to advisory once its accepted reply is proven not applied", async () => { + // The submitting instance's sequence: ack timeout (cmd 1) -> advisory + // resubmit accepted (cmd 2) -> pending lock -> cmd 2's outcome proves + // it was not applied while cmd 1 stays unresolved. The pending notice + // is now false and the lock has no exit, so the round returns to the + // advisory stance instead of diverging from a freshly mounted instance. + appContextMock.sendMessage.mockRejectedValueOnce(Object.assign( + new Error("ack timed out"), + { disposition: "outcome_unknown", userFacing: true }, + )) + const { rerender } = render(form(true)) + + fireEvent.change(screen.getByRole("textbox"), { target: { value: "Beijing" } }) + fireEvent.click( + screen.getByRole("button", { name: "chatPage.clarification.submit" }), + ) + await waitFor(() => expect(recordedSubmissions()).toHaveLength(1)) + mirrorSubmissions() + + // Advisory resubmit succeeds and the accepted reply goes pending. + await waitFor(() => expect(submitButton()).toBeEnabled()) + fireEvent.click( + screen.getByRole("button", { name: "chatPage.clarification.submit" }), + ) + await waitFor(() => expect(recordedSubmissions()).toHaveLength(2)) + mirrorSubmissions() + rerender(form(true)) + expect( + await screen.findByText("chatPage.clarification.replyPending"), + ).toBeInTheDocument() + expect(submitButton()).toBeDisabled() + + withOutcomes({ [recordedSubmissions()[1].commandId]: true }) + rerender(form(true)) + + await waitFor(() => expect(submitButton()).toBeEnabled()) + expect( + screen.queryByText("chatPage.clarification.replyPending"), + ).toBeNull() + expect( + screen.queryByText("chatPage.clarification.replyNotApplied"), + ).toBeNull() + }) + + it("replaces a lingering send-failure alert when the ambiguity notice takes over", async () => { + // An outcome_unknown send failure leaves its alert on screen; when the + // command's terminal outcome later proves unsafe, the two notices must + // not compete as two simultaneous role="alert" regions. + appContextMock.sendMessage.mockRejectedValueOnce(Object.assign( + new Error("ack timed out"), + { disposition: "outcome_unknown", userFacing: true }, + )) + const { rerender } = render(form(true)) + + fireEvent.change(screen.getByRole("textbox"), { target: { value: "Beijing" } }) + fireEvent.click( + screen.getByRole("button", { name: "chatPage.clarification.submit" }), + ) + await waitFor(() => expect(recordedSubmissions()).toHaveLength(1)) + await screen.findByRole("alert") + mirrorSubmissions() + + withOutcomes({ [recordedSubmissions()[0].commandId]: false }) + rerender(form(true)) + + await waitFor(() => { + const alerts = screen.getAllByRole("alert") + expect(alerts).toHaveLength(1) + expect(alerts[0]).toHaveTextContent( + "chatPage.clarification.replyOutcomeUnknown", + ) + }) + }) + + it("keeps a visible send-failure alert when another round's submission is recorded", async () => { + // The untracked-round fallback must be identity-stable: a fresh [] per + // render would rerun the gating effect on ANY round's record and reach + // the tail that clears sendFailure. Trigger: this round's delivery + // fails visibly, then an older round's ack-timeout promise settles and + // records for THAT round (deliberately before the latestRequestIdRef + // guard). + appContextMock.sendMessage.mockRejectedValueOnce(Object.assign( + new Error("Durable storage is temporarily unavailable"), + { disposition: "not_sent", userFacing: true }, + )) + const { rerender } = render(form(true, "inputreq_r2")) + + fireEvent.change(screen.getByRole("textbox"), { target: { value: "Beijing" } }) + fireEvent.click( + screen.getByRole("button", { name: "chatPage.clarification.submit" }), + ) + await screen.findByRole("alert") + + // An unrelated round's record lands in context state. + appContextMock.state = { + ...appContextMock.state, + clarificationSubmissions: { + inputreq_r1: [{ commandId: "older-round-cmd", accepted: false }], + }, + } + rerender(form(true, "inputreq_r2")) + + expect(screen.getByRole("alert")).toHaveTextContent( + "Durable storage is temporarily unavailable", + ) + expect(submitButton()).toBeEnabled() + }) + + it("records the submission when the delivery outcome is unknown", async () => { + appContextMock.sendMessage.mockRejectedValue(Object.assign( + new Error("ack timed out"), + { disposition: "outcome_unknown", userFacing: true }, + )) + render(form(true)) + + fireEvent.change(screen.getByRole("textbox"), { target: { value: "Beijing" } }) + fireEvent.click( + screen.getByRole("button", { name: "chatPage.clarification.submit" }), + ) + + // The reply may still have been durably accepted, so its eventual + // terminal outcome must gate this round like an acknowledged one - + // recorded as unconfirmed, which must not lock the form while no + // terminal outcome exists. + await waitFor(() => expect(recordedSubmissions()).toHaveLength(1)) + expect(recordedSubmissions()[0]).toEqual({ + requestId: ROUND, + commandId: expect.any(String), + accepted: false, + }) + mirrorSubmissions() + expect(submitButton()).toBeEnabled() + }) + + it("still reactivates a form that never submitted, ignoring unrelated outcomes", async () => { + appContextMock.state = { + clarificationSubmissions: {}, + commandOutcomes: { + "someone-elses-command": { resendSafe: false }, + }, + } + const { rerender } = render(form(false)) + rerender(form(true)) + + await waitFor(() => expect(submitButton()).toBeEnabled()) + expect(screen.queryByRole("alert")).toBeNull() + }) + + it("resets the gate for a new clarification round", async () => { + const { rerender } = render(form(true)) + const commandId = await submitAccepted() + await waitFor(() => expect(screen.queryByRole("textbox")).toBeNull()) + + rerender(form(false)) + withOutcomes({ [commandId]: false }) + rerender(form(true)) + await screen.findByRole("alert") + + rerender(form(true, "inputreq_r2")) + + await waitFor(() => expect(submitButton()).toBeEnabled()) + expect(screen.getByRole("textbox")).toHaveValue("") + expect(screen.queryByRole("alert")).toBeNull() + }) + + it("does not record a submission for a round without a request id", async () => { + render( + , + ) + + fireEvent.change(screen.getByRole("textbox"), { target: { value: "Beijing" } }) + fireEvent.click( + screen.getByRole("button", { name: "chatPage.clarification.submit" }), + ) + + await waitFor(() => expect(appContextMock.sendMessage).toHaveBeenCalledTimes(1)) + // With no round identity to bind to, gating is skipped entirely rather + // than risking a recorded reply gating a different question. + expect(appContextMock.dispatch).not.toHaveBeenCalledWith( + expect.objectContaining({ type: "RECORD_CLARIFICATION_SUBMISSION" }), + ) + }) +}) diff --git a/frontend/src/components/chat/clarification-form.tsx b/frontend/src/components/chat/clarification-form.tsx index 6e2def03fc..729f1eb1d0 100644 --- a/frontend/src/components/chat/clarification-form.tsx +++ b/frontend/src/components/chat/clarification-form.tsx @@ -1,5 +1,10 @@ import React, { useEffect, useLayoutEffect, useMemo, useRef, useState } from "react" -import { Interaction } from "@/contexts/app-context-chat" +import { + ClarificationSubmission, + Interaction, + TerminalCommandOutcome, +} from "@/contexts/app-context-chat" +import { generateClientMessageId } from "@/lib/utils" import { Input } from "@/components/ui/input" import { Button } from "@/components/ui/button" import { Textarea } from "@/components/ui/textarea" @@ -114,6 +119,13 @@ const sendHintKey = ( ? "chatPage.clarification.sendNotSent" : null +// Stable empty fallback for rounds with no tracked submissions: minting a +// fresh [] per render would change the gating effect's dependency identity +// on ANY round's record/clear, and a rerun on an active untracked round +// reaches the tail that clears a visible send-failure alert - the exact +// regression class the boolean reduction below exists to prevent. +const NO_SUBMISSIONS: ClarificationSubmission[] = [] + // Interaction types that are "live widgets" reflecting external state (e.g. // useMcpApps()'s connection state), not a question with an answer to submit // - see the comment on isConnectAppsOnly below for why that distinction @@ -133,11 +145,17 @@ export function ClarificationForm({ }: ClarificationFormProps) { // If onSend is provided, use it (e.g., from builder chat), otherwise use useApp let sendMessage: any, dispatch: any, contextFilesDisabled: boolean | undefined; + let commandOutcomes: Record | undefined; + let clarificationSubmissions: + | Record + | undefined; try { const appCtx = useApp(); sendMessage = appCtx.sendMessage; dispatch = appCtx.dispatch; contextFilesDisabled = appCtx.filesDisabled; + commandOutcomes = appCtx.state?.commandOutcomes; + clarificationSubmissions = appCtx.state?.clarificationSubmissions; } catch { // We might not be in the app context (e.g., agent builder chat) } @@ -184,6 +202,11 @@ export function ClarificationForm({ errorCode: ClientErrorCode | null } | null >(null) + // Which outcome notice the form owes the user. Raw category only; the + // sentence is resolved at render so a locale switch reaches it. + const [outcomeNotice, setOutcomeNotice] = useState< + "notApplied" | "unconfirmed" | "pending" | null + >(null) useLayoutEffect(() => { latestRequestIdRef.current = requestId @@ -194,18 +217,134 @@ export function ClarificationForm({ setIsSubmitted(!active && !isConnectAppsOnly) setIsOpen(active || isConnectAppsOnly) setSendFailure(null) + // A new round is a new question: the previous round's outcome notice no + // longer belongs on top of it. + setOutcomeNotice(null) }, [active, isConnectAppsOnly, requestId]) + // The reply commands this round is still accountable for, read from + // context so the gate survives this component instance being replaced + // (virtual waiting message vs. persisted timeline message render the same + // round). One round can hold several: an ack-timeout entry stays + // resubmittable, so a resubmit appends rather than replaces, and every + // tracked command's outcome still gets matched. + const outstandingSubmissions = useMemo( + () => + (requestId ? clarificationSubmissions?.[requestId] : undefined) + ?? NO_SUBMISSIONS, + [requestId, clarificationSubmissions], + ) + // Reduced to booleans before entering the effect's dependency list: an + // unrelated command's outcome landing must not re-run the effect - that + // rerun used to wipe a visible failure alert. + const hasOutstanding = outstandingSubmissions.length > 0 + // Any tracked reply whose terminal outcome cannot prove non-application + // may have been committed: the round must surface that and stay locked. + const anyUnsafeOutcome = outstandingSubmissions.some((submission) => { + const outcome = commandOutcomes?.[submission.commandId] + return outcome !== undefined && outcome.resendSafe !== true + }) + // Every tracked reply is proven not applied: resending is safe. + const allProvenNotApplied = + hasOutstanding + && outstandingSubmissions.every( + (submission) => commandOutcomes?.[submission.commandId]?.resendSafe === true, + ) + // A durably acknowledged reply is still awaiting its terminal outcome. + // (An unconfirmed ack-timeout entry without an outcome deliberately does + // not count: it may never have been accepted, and the composer keeps its + // advisory retry for it - which is also why it blocks the proven-safe + // unlock above without forcing a lock here.) + const confirmedPending = outstandingSubmissions.some( + (submission) => + submission.accepted && commandOutcomes?.[submission.commandId] === undefined, + ) + // Distinguishes the round that has heard nothing yet from the one whose + // resolved replies are all proven safe while unconfirmed ones remain. + const anyOutcome = outstandingSubmissions.some( + (submission) => commandOutcomes?.[submission.commandId] !== undefined, + ) + useEffect(() => { - if (active) { - // A new clarification round reuses this component instance on the live - // turn render path, so a stale round-1 failure alert would sit on top - // of round 2's question. - setIsSubmitted(false) - setIsOpen(true) - setSendFailure(null) + if (!active) return + // Task state alone answers whether the task accepts input; it does not + // prove that repeating this round's accepted reply is safe (#1500). + // While a submission is outstanding, reactivation requires terminal + // outcomes that prove non-application for every tracked reply. + if (hasOutstanding) { + if (anyUnsafeOutcome) { + // Committed or unknown: surface the ambiguity without inviting a + // duplicate. The chat input remains available for a deliberate + // fresh message. The send-failure alert would compete with this + // one (two role="alert" regions), and this notice supersedes it. + setOutcomeNotice("unconfirmed") + setIsSubmitted(true) + setIsOpen(true) + setSendFailure(null) + return + } + if (confirmedPending) { + // Accepted and still being applied: lock even a freshly mounted + // instance (whose initial isSubmitted is false), and say why the + // form is locked instead of leaving a bare greyed-out button. + setOutcomeNotice("pending") + setIsSubmitted(true) + setIsOpen(true) + return + } + if (!allProvenNotApplied) { + if (anyOutcome) { + // Every resolved reply is proven not applied; only unconfirmed + // ack-timeout replies remain, and their duplicate risk is the + // same risk the advisory retry already accepted for them. Match + // the fresh-instance stance: return the round to advisory + // instead of leaving the submitting instance locked behind a + // now-stale pending notice. No proven-safe notice - that promise + // cannot be made while an unconfirmed reply is unresolved. + setOutcomeNotice((notice) => (notice === "pending" ? null : notice)) + setIsSubmitted(false) + setIsOpen(true) + } + // Nothing resolved yet: keep the composer's advisory retry - and + // its ack-timeout warning alert - exactly as they stand. + return + } + // Every tracked reply is proven not applied: consume the record so + // the resend is armed once, then reactivate below with the draft + // intact. The CLEAR names exactly the commands this render verified, + // so a submission recorded concurrently (another instance's resubmit + // landing between this render's snapshot and the dispatch) survives + // with its outcome still matchable. + dispatch?.({ + type: "CLEAR_CLARIFICATION_SUBMISSION", + payload: { + requestId, + commandIds: outstandingSubmissions.map( + (submission) => submission.commandId, + ), + }, + }) + setOutcomeNotice("notApplied") } - }, [active]) + // A new clarification round reuses this component instance on the live + // turn render path, so a stale round-1 failure alert would sit on top + // of round 2's question. The outcome notice is deliberately not cleared + // here: it explains why the form re-opened, and it falls away on the + // next submit or the next round. + setIsSubmitted(false) + setIsOpen(true) + setSendFailure(null) + }, [ + active, + hasOutstanding, + anyUnsafeOutcome, + allProvenNotApplied, + confirmedPending, + anyOutcome, + outstandingSubmissions, + requestId, + dispatch, + ]) const normalizedInteractions = useMemo(() => { const seenFields = new Set() @@ -377,9 +516,33 @@ export function ClarificationForm({ } }) + // Minted here, not in sendMessage, so this round knows the id the + // durable command will carry and can correlate its terminal outcome + // (#1500). The builder onSend path has no durable command behind it and + // takes no part in outcome gating; neither does a round without a + // request id - the gate would have no round identity to bind to, and a + // recorded reply could end up gating a different question. + const clientMessageId = generateClientMessageId() + // ``accepted`` separates a durably acknowledged reply from one whose + // ack timed out: only a confirmed acceptance may lock a freshly mounted + // form while its terminal outcome is still pending. + const recordSubmission = (accepted: boolean) => { + if (onSend || !dispatch) return + if (typeof submittedRequestId !== "string" || !submittedRequestId) return + dispatch({ + type: "RECORD_CLARIFICATION_SUBMISSION", + payload: { + requestId: submittedRequestId, + commandId: clientMessageId, + accepted, + }, + }) + } + try { setIsSubmitting(true) setSendFailure(null) + setOutcomeNotice(null) // If textMessage is empty but we have files, send a generic message? const outboundFiles = filesDisabled ? [] : files const finalMessage = textMessage || (outboundFiles.length > 0 ? t("chatPage.clarification.uploadedFiles") : t("chatPage.clarification.confirmed")) @@ -387,7 +550,12 @@ export function ClarificationForm({ if (onSend) { await onSend(finalMessage, outboundFiles, metadata); } else if (sendMessage) { - await sendMessage(finalMessage, { force: true, metadata }, outboundFiles) + await sendMessage( + finalMessage, + { force: true, metadata, clientMessageId }, + outboundFiles, + ) + recordSubmission(true) } if (latestRequestIdRef.current !== submittedRequestId) return @@ -397,6 +565,13 @@ export function ClarificationForm({ dispatch({ type: "UPDATE_TASK_STATUS", payload: { status: "running" } }) } } catch (error) { + if (readSendDisposition(error) === "outcome_unknown") { + // An ack timeout: the reply may still have been durably accepted, + // so its eventual terminal outcome must gate this round exactly as + // an acknowledged submission's would - but as an unconfirmed + // delivery it must not lock the form while no outcome exists. + recordSubmission(false) + } if (latestRequestIdRef.current !== submittedRequestId) return console.error("Failed to send clarification response", error) // The rejection reason ("a previous guidance message is still being @@ -713,6 +888,21 @@ export function ClarificationForm({ ))} + {outcomeNotice === "unconfirmed" && ( +
+ {t("chatPage.clarification.replyOutcomeUnknown")} +
+ )} + {outcomeNotice === "pending" && ( +
+ {t("chatPage.clarification.replyPending")} +
+ )} + {outcomeNotice === "notApplied" && ( +
+ {t("chatPage.clarification.replyNotApplied")} +
+ )} {sendFailure && (
{sendFailureMessage}
diff --git a/frontend/src/components/task/task-conversation-panel.test.tsx b/frontend/src/components/task/task-conversation-panel.test.tsx index 2ba3589dab..16ee59a300 100644 --- a/frontend/src/components/task/task-conversation-panel.test.tsx +++ b/frontend/src/components/task/task-conversation-panel.test.tsx @@ -602,6 +602,295 @@ describe("TaskConversationPanel", () => { expect(rendered[1]).toHaveAttribute("data-request-id", "inputreq_q2") }) + it("prefers the virtual bubble over an older round's form when the round id and text both miss", () => { + // Round 2's status frame arrived (new id, new question) but its ask row + // never landed. The only interactive timeline item belongs to round 1; + // activating it would misbind the reply. The virtual bubble carrying + // the current question is the correct instance. + appState.messages = [ + { + id: "q1", + role: "assistant", + content: "Which city?", + timestamp: 1000, + isResult: true, + interactions: [{ type: "text_input", field: "city", label: "City" }], + interactionRequestId: "round-1", + }, + { + id: "u1", + role: "user", + content: "City: Beijing", + timestamp: 2000, + }, + ] + appState.traceEvents = [] + appState.currentTask = { + id: "42", + title: "Preview", + description: "Preview", + status: "waiting_for_user", + createdAt: "2026-01-01T00:00:00Z", + updatedAt: "2026-01-01T00:00:00Z", + waitingQuestion: "Which hotel?", + waitingRequestId: "round-2", + waitingInteractions: [{ type: "text_input", field: "hotel", label: "Hotel" }], + } + + render() + + const active = screen.getAllByTestId("chat-message") + .filter((node) => node.getAttribute("data-active") === "true") + expect(active).toHaveLength(1) + expect(active[0]).toHaveAttribute("data-request-id", "round-2") + expect(active[0]).toHaveTextContent("Which hotel?") + }) + + it("falls back to the text match when no timeline item carries the round id", () => { + // Replayed history rows carry no interactionRequestId. With the ask as + // the last assistant message (virtual bubble suppressed), a failed + // id-match that short-circuited instead of falling through would leave + // ZERO active reply instances for a waiting task. + appState.messages = [ + { + id: "q1", + role: "assistant", + content: "Which city?", + timestamp: 1000, + isResult: true, + interactions: [{ type: "text_input", field: "city", label: "City" }], + // No interactionRequestId - the replayed row never carried one. + }, + ] + appState.traceEvents = [] + appState.currentTask = { + id: "42", + title: "Preview", + description: "Preview", + status: "waiting_for_user", + createdAt: "2026-01-01T00:00:00Z", + updatedAt: "2026-01-01T00:00:00Z", + waitingQuestion: "Which city?", + waitingRequestId: "round-1", + } + + render() + + const active = screen.getAllByTestId("chat-message") + .filter((node) => node.getAttribute("data-active") === "true") + expect(active).toHaveLength(1) + expect(active[0]).toHaveTextContent("Which city?") + }) + + it("does not leak the previous task's round id during a task switch", () => { + // During a switch, currentTask can still describe task A while + // state.taskId already points at task B; the round id read is gated on + // the ids matching, like the sibling ChatInput wiring. + appState.taskId = 99 + appState.messages = [] + appState.traceEvents = [] + appState.currentTask = { + id: "42", + title: "Previous task", + description: "Previous task", + status: "waiting_for_user", + createdAt: "2026-01-01T00:00:00Z", + updatedAt: "2026-01-01T00:00:00Z", + waitingQuestion: "Old question?", + waitingRequestId: "round-of-task-42", + } + + render() + + const leaked = screen.getAllByTestId("chat-message") + .filter((node) => node.getAttribute("data-request-id") === "round-of-task-42") + expect(leaked).toHaveLength(0) + appState.taskId = 42 + }) + + it("keeps at most one instance active for a waiting round", () => { + // The two-instances window: the round's question is persisted on the + // timeline AND an optimistic user message is the last item (so the + // virtual assistant message renders too). The timeline instance owns the + // round; the virtual copy must stay inert, or two forms accept the same + // question at once. + appState.messages = [ + { + id: "q1", + role: "assistant", + content: "Which city?", + timestamp: "1000", + isResult: true, + interactions: [{ type: "text_input", field: "city", label: "City" }], + interactionRequestId: "round-1", + }, + { + id: "u1", + role: "user", + content: "City: Beijing", + timestamp: "2000", + isOptimistic: true, + }, + ] + appState.traceEvents = [] + appState.currentTask = { + id: "42", + title: "Preview", + description: "Preview", + status: "waiting_for_user", + createdAt: "2026-01-01T00:00:00Z", + updatedAt: "2026-01-01T00:00:00Z", + waitingQuestion: "Which city?", + waitingRequestId: "round-1", + waitingInteractions: [{ type: "text_input", field: "city", label: "City" }], + } + + render() + + const active = screen.getAllByTestId("chat-message") + .filter((node) => node.getAttribute("data-active") === "true") + expect(active).toHaveLength(1) + expect(active[0]).toHaveAttribute("data-request-id", "round-1") + expect(active[0]).toHaveTextContent("Which city?") + }) + + it("gives the active id-less replayed row the resolved round id", () => { + // A replayed chat-history row carries no interactionRequestId. When the + // text fallback elects it as the active instance, it must submit with + // the round id the waiting frame delivered - not id-less. + appState.messages = [ + { + id: "q1", + role: "assistant", + content: "Which city?", + timestamp: 1000, + isResult: true, + interactions: [{ type: "text_input", field: "city", label: "City" }], + }, + { + id: "u1", + role: "user", + content: "Working on it", + timestamp: 2000, + }, + ] + appState.traceEvents = [] + appState.currentTask = { + id: "42", + title: "Preview", + description: "Preview", + status: "waiting_for_user", + createdAt: "2026-01-01T00:00:00Z", + updatedAt: "2026-01-01T00:00:00Z", + waitingQuestion: "Which city?", + waitingRequestId: "evt-round-1", + } + + render() + + const active = screen.getAllByTestId("chat-message") + .filter((node) => node.getAttribute("data-active") === "true") + expect(active).toHaveLength(1) + expect(active[0]).toHaveTextContent("Which city?") + expect(active[0]).toHaveAttribute("data-request-id", "evt-round-1") + }) + + it("never text-elects a lookalike row that carries a different round id", () => { + // Same question text asked in an earlier round: that row provably + // belongs to the other round and must not be elected over the id-less + // current row. + // The lookalike carrying the OLD round id is deliberately the NEWEST + // text-matching row: the reverse scan reaches it first, so only the + // skip guard (not election order) keeps it from being elected. + appState.messages = [ + { + id: "q1", + role: "assistant", + content: "Which city?", + timestamp: 1000, + isResult: true, + interactions: [{ type: "text_input", field: "city", label: "City" }], + }, + { + id: "q2", + role: "assistant", + content: "Which city?", + timestamp: 2000, + isResult: true, + interactions: [{ type: "text_input", field: "city", label: "City" }], + interactionRequestId: "evt-old-round", + }, + { + id: "u1", + role: "user", + content: "Working on it", + timestamp: 3000, + }, + ] + appState.traceEvents = [] + appState.currentTask = { + id: "42", + title: "Preview", + description: "Preview", + status: "waiting_for_user", + createdAt: "2026-01-01T00:00:00Z", + updatedAt: "2026-01-01T00:00:00Z", + waitingQuestion: "Which city?", + waitingRequestId: "evt-round-2", + } + + render() + + const active = screen.getAllByTestId("chat-message") + .filter((node) => node.getAttribute("data-active") === "true") + expect(active).toHaveLength(1) + // The id-less newest row is elected and speaks with the current round id. + expect(active[0]).toHaveAttribute("data-request-id", "evt-round-2") + }) + + it("activates the round-id match rather than a same-text lookalike", () => { + // Prompt-text equality can pick the wrong message when two rounds asked + // the same question; the round id is authoritative when present. + appState.messages = [ + { + id: "q1", + role: "assistant", + content: "Which city?", + timestamp: "1000", + isResult: true, + interactions: [{ type: "text_input", field: "city", label: "City" }], + interactionRequestId: "round-1", + }, + { + id: "q2", + role: "assistant", + content: "Which city?", + timestamp: "2000", + isResult: true, + interactions: [{ type: "text_input", field: "city", label: "City" }], + interactionRequestId: "round-2", + }, + ] + appState.traceEvents = [] + appState.currentTask = { + id: "42", + title: "Preview", + description: "Preview", + status: "waiting_for_user", + createdAt: "2026-01-01T00:00:00Z", + updatedAt: "2026-01-01T00:00:00Z", + waitingQuestion: "Which city?", + waitingRequestId: "round-1", + } + + render() + + const active = screen.getAllByTestId("chat-message") + .filter((node) => node.getAttribute("data-active") === "true") + expect(active).toHaveLength(1) + expect(active[0]).toHaveAttribute("data-request-id", "round-1") + }) + it("keeps an identified text-only wait separate from stale structured trace interactions", () => { appState.messages = [{ id: "user-r2", diff --git a/frontend/src/components/task/task-conversation-panel.tsx b/frontend/src/components/task/task-conversation-panel.tsx index 65db3a0cad..23a4453e6f 100644 --- a/frontend/src/components/task/task-conversation-panel.tsx +++ b/frontend/src/components/task/task-conversation-panel.tsx @@ -21,7 +21,7 @@ import { useI18n } from "@/contexts/i18n-context" import { isStreamingFinalAnswerMessage } from "@/lib/streaming-final-answer" import { getProcessGroupIndex, getUserTimelineAnchors } from "@/lib/task-timeline" import { resolveTraceProcessStatus } from "@/lib/trace-process-status" -import { cn } from "@/lib/utils" +import { cn, firstNonEmptyString } from "@/lib/utils" export type TaskConversationPanelMode = "page" | "embedded-preview" @@ -480,22 +480,71 @@ export function TaskConversationPanel({ () => findWaitingInteractions(state.currentTask, managerTraceEvents as any[]), [managerTraceEvents, state.currentTask] ) + // The waiting round's identity, delivered on the task-state frames + // themselves (live/resume waiting task_info, replay task_info, replay + // reassertion) - no client-side reconstruction. Undefined only for + // backends predating the emission, where rounds stay unidentified. + // Gated on the task actually being the one on screen: during a task + // switch, currentTask can still describe the previous task while + // state.taskId already points at the new one (same guard as the + // ChatInput wiring below). + const waitingRoundId = + state.currentTask?.status === "waiting_for_user" + && state.currentTask.id === String(state.taskId) + ? state.currentTask.waitingRequestId + : undefined const activeWaitingMessageId = useMemo(() => { if (state.currentTask?.status !== "waiting_for_user") { return null } + // When the waiting round has an identity, prefer matching by it: the + // prompt-text fallback below can pick a message whose text merely equals + // the question while the round actually lives on a different item, + // leaving TWO instances active at once (the timeline one and the + // virtual one) with independently diverging state. A FAILED id-match + // falls through rather than short-circuiting: replayed history rows + // carry no interactionRequestId, and with the ask sitting as the last + // assistant message the virtual bubble is suppressed too - returning + // null here would leave zero active reply instances for a waiting task. + if (waitingRoundId) { + for (let i = messageItems.length - 1; i >= 0; i--) { + const item = messageItems[i] + if (item.role === "assistant" && item.interactionRequestId === waitingRoundId) { + return item.id + } + } + } + if (waitingPrompt) { const normalizedPrompt = waitingPrompt.trim() for (let i = messageItems.length - 1; i >= 0; i--) { const item = messageItems[i] + // A row that carries its OWN round id different from the current + // round is a lookalike from an earlier ask - electing it would + // misbind the reply. Only id-less rows may be text-elected. + if ( + waitingRoundId + && item.interactionRequestId + && item.interactionRequestId !== waitingRoundId + ) { + continue + } if (item.role === "assistant" && typeof item.content === "string" && item.content.trim() === normalizedPrompt) { return item.id } } } + // The bare newest-interactions fallback is for rounds with NO identity + // at all. Under a known round id whose item and text both failed to + // match, it could only pick an OLDER round's form (a dropped ask frame + // for the current round), misbinding the reply - the virtual bubble + // showing the current question is the right instance there. + if (waitingRoundId) { + return null + } for (let i = messageItems.length - 1; i >= 0; i--) { const item = messageItems[i] if (item.role === "assistant" && item.interactions && item.interactions.length > 0) { @@ -504,7 +553,7 @@ export function TaskConversationPanel({ } return null - }, [messageItems, state.currentTask?.status, waitingPrompt]) + }, [messageItems, state.currentTask?.status, waitingPrompt, waitingRoundId]) useEffect(() => { messagesEndRef.current?.scrollIntoView?.({ behavior: "smooth" }) @@ -788,7 +837,19 @@ export function TaskConversationPanel({ } timestamp={item.timestamp} interactions={item.interactions} - interactionRequestId={item.interactionRequestId} + // The active waiting item speaks for the current + // round: when a replayed row carries no id of its + // own, the resolved round id keeps its reply - and + // the retry gate - bound to the ask instead of + // submitting id-less. + interactionRequestId={ + item.id === activeWaitingMessageId + ? firstNonEmptyString( + item.interactionRequestId, + waitingRoundId, + ) + : item.interactionRequestId + } interactionsActive={item.id === activeWaitingMessageId} showEmptyStatus={item.showEmptyStatus} contextBadges={item.role === "user" ? userMessageContextBadges : undefined} @@ -815,8 +876,15 @@ export function TaskConversationPanel({ processStatus={state.currentTask?.status} taskStatus={state.currentTask?.status} interactions={state.currentTask?.status === "waiting_for_user" ? waitingInteractions : undefined} - interactionRequestId={state.currentTask?.status === "waiting_for_user" ? state.currentTask.waitingRequestId : undefined} - interactionsActive={state.currentTask?.status === "waiting_for_user"} + interactionRequestId={state.currentTask?.status === "waiting_for_user" ? waitingRoundId : undefined} + // At most one instance of a waiting round is active: a + // timeline message already owning the round keeps the + // virtual copy inert, so two forms can never both + // accept a submission for the same question. + interactionsActive={ + state.currentTask?.status === "waiting_for_user" + && activeWaitingMessageId === null + } onOpenExecutionPlan={showDagPreview ? openDagPreview : undefined} onAgentExecutionClick={onAgentExecutionClick} /> @@ -864,7 +932,10 @@ export function TaskConversationPanel({ currentInteractionRequestId={ state.currentTask?.status === "waiting_for_user" && state.currentTask.id === String(state.taskId) - ? state.currentTask.waitingRequestId + // The same round identity the form path uses - including + // the ask-trace fallback - so a free-text reply after a + // reload still binds to the round. + ? waitingRoundId : undefined } isLoading={ diff --git a/frontend/src/contexts/app-context-chat.test.tsx b/frontend/src/contexts/app-context-chat.test.tsx index 69cdbbe940..117af04b9c 100644 --- a/frontend/src/contexts/app-context-chat.test.tsx +++ b/frontend/src/contexts/app-context-chat.test.tsx @@ -153,6 +153,7 @@ import { ChatStartScreen } from "@/components/chat/ChatStartScreen" import { MarkdownRenderer } from "@/components/ui/markdown-renderer" import { TASK_ERROR_EVENT, type TaskErrorEventDetail } from "@/lib/task-error-events" import type { Translate } from "@/contexts/i18n-context" +import { ClarificationForm } from "@/components/chat/clarification-form" type TaskControlMessage = Parameters[0] @@ -214,6 +215,7 @@ function StateProbe() {
{String(state.isHistoryLoading)}
{String(state.filePreview.isOpen)}
{String(state.isProcessing)}
+
{JSON.stringify(state.commandOutcomes)}
) } @@ -6646,3 +6648,1113 @@ describe("error frame display projection", () => { ).toEqual(expected) }) }) + +describe("clarification round identity (#1500)", () => { + beforeEach(() => { + webSocketOptions.current = null + webSocketOptions.all = [] + sessionControls = null + wsHarness.isConnected = true + apiRequestMock.mockReset() + routerPushMock.mockReset() + sendRawMessageMock.mockReset() + sendRawMessageMock.mockReturnValue("sent") + sendChatMessageMock.mockReset() + sendChatMessageMock.mockResolvedValue({ + client_message_id: "turn-optimistic", + turn_id: "turn-optimistic", + }) + localStorage.clear() + ;(window as typeof window & { clearDuplicateMessageCache?: () => void }) + .clearDuplicateMessageCache?.() + }) + + afterEach(() => { + cleanup() + localStorage.clear() + }) + + it("adopts the waiting frame's request_id as the round id", async () => { + // The waiting/reassert frames carry the round identity as request_id + // (backend #2232). They never emit event_id, so there is deliberately + // no event_id fallback on this handler - the ask frame's event_id is + // adopted in the agent_message trace reader instead. An empty-string + // request_id reads as no identity. + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + act(() => { + onMessage?.({ + type: "task_waiting_for_user", + timestamp: "2026-05-27T05:00:01Z", + task_id: 1, + task: { id: 1, status: "waiting_for_user" }, + message: "Which region should I use?", + request_id: "req-round-1", + interactions: [ + { type: "text", prompt: "Which region should I use?" }, + ], + } as TestWebSocketMessage) + }) + await waitFor(() => { + expect(screen.getByTestId("waiting-request-id").textContent).toBe("req-round-1") + }) + + act(() => { + onMessage?.({ + type: "task_waiting_for_user", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "waiting_for_user" }, + message: "Which hotel?", + request_id: "", + event_id: "evt-never-adopted-here", + interactions: [ + { type: "text", prompt: "Which hotel?" }, + ], + } as TestWebSocketMessage) + }) + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain("Which hotel?") + }) + expect(screen.getByTestId("waiting-request-id").textContent).toBe("") + }) + + it("adopts the live ask's event_id onto the transcript message", async () => { + // The one adoption site production always exercises: a live ask arrives + // as an agent_message trace event whose data.event_id is the round id, + // and the transcript message it produces must carry it as + // interactionRequestId. + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + act(() => { + onMessage?.({ + type: "trace_event", + timestamp: "2026-05-27T05:00:01Z", + task_id: 1, + data: { + event_id: "trace-row-1", + event_type: "agent_message", + data: { + message: "Which region should I use?", + expect_response: true, + event_id: "evt-live-ask", + metadata: { + interactions: [ + { type: "text_input", field: "region", label: "Region" }, + ], + }, + }, + }, + } as TestWebSocketMessage) + }) + + await waitFor(() => { + const messages = JSON.parse( + screen.getByTestId("messages").textContent || "[]" + ) as Array<{ content: string; interactionRequestId?: string }> + const ask = messages.find( + (m) => m.content === "Which region should I use?" + ) + expect(ask?.interactionRequestId).toBe("evt-live-ask") + }) + }) + + it("keeps a held round id when an id-less waiting task_info arrives", async () => { + // A backend predating the emission sends waiting task_info frames with + // no request_id; SET_CURRENT_TASK's merge must not wipe the id the + // waiting handler already extracted for the open round. + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + act(() => { + onMessage?.({ + type: "task_waiting_for_user", + timestamp: "2026-05-27T05:00:01Z", + task_id: 1, + task: { id: 1, status: "waiting_for_user" }, + message: "Which region should I use?", + request_id: "evt-live-round", + } as TestWebSocketMessage) + }) + await waitFor(() => { + expect(screen.getByTestId("waiting-request-id").textContent).toBe( + "evt-live-round" + ) + }) + + act(() => { + onMessage?.({ + type: "trace_event", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + data: { + event_id: "task-info-row-2", + event_type: "task_info", + data: { + id: 1, + title: "Task", + description: "Task", + status: "waiting_for_user", + }, + }, + } as TestWebSocketMessage) + }) + + await waitFor(() => { + expect(screen.getByTestId("task-status").textContent).toBe( + "waiting_for_user" + ) + }) + expect(screen.getByTestId("waiting-request-id").textContent).toBe( + "evt-live-round" + ) + }) + + it("adopts the waiting task_info frame's request_id as the round id", async () => { + // The replay/live waiting task_info now carries request_id; the task + // shaping must surface it as waitingRequestId. + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + act(() => { + onMessage?.({ + type: "trace_event", + timestamp: "2026-05-27T05:00:01Z", + task_id: 1, + data: { + event_id: "task-info-row", + event_type: "task_info", + data: { + id: 1, + title: "Task", + description: "Task", + status: "waiting_for_user", + request_id: "evt-info-round", + }, + }, + } as TestWebSocketMessage) + }) + + await waitFor(() => { + expect(screen.getByTestId("waiting-request-id").textContent).toBe( + "evt-info-round" + ) + }) + }) + + it("keeps the round id across a same-question reassertion without an id", async () => { + // Reload/reconnect reassertion frames re-send the unchanged question + // with no id; wiping the id there severs the open round's correlation. + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + act(() => { + onMessage?.({ + type: "task_waiting_for_user", + timestamp: "2026-05-27T05:00:01Z", + task_id: 1, + task: { id: 1, status: "waiting_for_user" }, + message: "Which region should I use?", + request_id: "evt-round-1", + } as TestWebSocketMessage) + }) + await waitFor(() => { + expect(screen.getByTestId("waiting-request-id").textContent).toBe("evt-round-1") + }) + + act(() => { + onMessage?.({ + type: "task_waiting_for_user", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "waiting_for_user" }, + message: "Which region should I use?", + } as TestWebSocketMessage) + }) + await waitFor(() => { + expect(screen.getByTestId("waiting-request-id").textContent).toBe("evt-round-1") + }) + }) + + it("clears the round id when an id-less frame asks a different question", async () => { + // A legacy-backend NEW round (different question, no id) must not + // inherit the previous round's id. + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + act(() => { + onMessage?.({ + type: "task_waiting_for_user", + timestamp: "2026-05-27T05:00:01Z", + task_id: 1, + task: { id: 1, status: "waiting_for_user" }, + message: "Which region should I use?", + request_id: "evt-round-1", + } as TestWebSocketMessage) + }) + await waitFor(() => { + expect(screen.getByTestId("waiting-request-id").textContent).toBe("evt-round-1") + }) + + act(() => { + onMessage?.({ + type: "task_waiting_for_user", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "waiting_for_user" }, + message: "Which hotel?", + } as TestWebSocketMessage) + }) + await waitFor(() => { + expect(screen.getByTestId("waiting-request-id").textContent).toBe("") + }) + }) + + it("adds one bubble for a duplicated terminal command frame", async () => { + // A terminal agent_error carries a durable identity (command_id + + // outcome_version); a duplicated delivery of the SAME broadcast must + // not add a second bubble, while two DISTINCT commands failing with + // identical text must both stay visible (identity-keyed, never + // text-keyed). + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + const terminalFrame = (commandId: string) => ({ + type: "agent_error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + data: { + type: "agent_error", + message: "This message was not applied to the task.", + command_id: commandId, + command_kind: "message", + outcome: "failed", + outcome_version: 1, + resend_safe: true, + }, + }) as TestWebSocketMessage + + act(() => { + onMessage?.(terminalFrame("cmd-dup")) + }) + act(() => { + onMessage?.(terminalFrame("cmd-dup")) + }) + act(() => { + onMessage?.(terminalFrame("cmd-other")) + }) + + await waitFor(() => { + const messages = JSON.parse( + screen.getByTestId("messages").textContent || "[]" + ) as Array<{ content: string }> + const bubbles = messages.filter((m) => + m.content.includes("This message was not applied to the task.") + ) + expect(bubbles).toHaveLength(2) + }) + }) + + it("keeps a stale-versioned plain error notice without rolling back task state", async () => { + // The parallel "error"-type path shares the exemption with agent_error. + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + act(() => { + onMessage?.({ + type: "task_waiting_for_user", + timestamp: "2026-05-27T05:00:01Z", + task_id: 1, + run_id: "run-1", + state_version: 6, + task: { id: 1, status: "waiting_for_user" }, + message: "Which region should I use?", + } as TestWebSocketMessage) + }) + await waitFor(() => { + expect(screen.getByTestId("task-status").textContent).toBe("waiting_for_user") + }) + + act(() => { + onMessage?.({ + type: "error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + run_id: "run-1", + state_version: 5, + task: { id: 1, status: "failed" }, + message: "Task pause is still being applied; please retry shortly.", + error_code: "task_pause_in_progress", + } as TestWebSocketMessage) + }) + + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain( + "clientErrors.taskPauseInProgress" + ) + }) + expect(screen.getByTestId("task-status").textContent).toBe("waiting_for_user") + }) + + it("still drops a stale-versioned task_error frame whole", async () => { + // task_error's bubble is the turn's terminal result; it deliberately + // stays outside the stale-frame exemption, mirroring the guard's + // unversioned rule. + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + act(() => { + onMessage?.({ + type: "task_waiting_for_user", + timestamp: "2026-05-27T05:00:01Z", + task_id: 1, + run_id: "run-1", + state_version: 6, + task: { id: 1, status: "waiting_for_user" }, + message: "Which region should I use?", + } as TestWebSocketMessage) + }) + await waitFor(() => { + expect(screen.getByTestId("task-status").textContent).toBe("waiting_for_user") + }) + + act(() => { + onMessage?.({ + type: "task_error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + run_id: "run-1", + state_version: 5, + task: { id: 1, status: "failed" }, + message: "stale terminal result", + } as TestWebSocketMessage) + }) + + // The whole frame is dropped: no bubble, no status change. + expect(screen.getByTestId("messages").textContent).not.toContain( + "stale terminal result" + ) + expect(screen.getByTestId("task-status").textContent).toBe("waiting_for_user") + }) + + it("keeps a stale-versioned error notice without rolling back task state", async () => { + // The version guard protects task state, but an error frame's body is + // not versioned state: it carries a notice (and, since #2124, the + // structured terminal command outcome) the backend sends exactly once. + // A stale control tuple must lose the state argument yet keep the + // notice. + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + act(() => { + onMessage?.({ + type: "task_waiting_for_user", + timestamp: "2026-05-27T05:00:01Z", + task_id: 1, + run_id: "run-1", + state_version: 6, + task: { id: 1, status: "waiting_for_user" }, + message: "Which region should I use?", + } as TestWebSocketMessage) + }) + await waitFor(() => { + expect(screen.getByTestId("task-status").textContent).toBe("waiting_for_user") + }) + + act(() => { + onMessage?.({ + type: "agent_error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + run_id: "run-1", + state_version: 5, + data: { + type: "agent_error", + message: "This message was not applied to the task.", + command_id: "client-msg-1", + command_kind: "message", + task: { id: 1, status: "failed" }, + }, + } as TestWebSocketMessage) + }) + + // The notice reaches the transcript; the stale status assertion does not. + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain( + "This message was not applied to the task." + ) + }) + expect(screen.getByTestId("task-status").textContent).toBe("waiting_for_user") + }) +}) + +function ClarificationSubmissionProbe() { + const { state, dispatch } = useApp() + return ( + <> +
+ {JSON.stringify(state.clarificationSubmissions)} +
+