From cc4f271cd91f3c48194cd37a3c54fa142df0f7c4 Mon Sep 17 00:00:00 2001 From: leyoonafr Date: Mon, 7 Sep 2026 03:46:01 +0800 Subject: [PATCH 1/9] fix(frontend): give clarification rounds a real wire identity Part 1 of the #1500 re-slice (surfaced by review round 3 on #2126, finding F1: no backend emits request_id, so every consumer of the frontend's request_id plumbing was inert in production). - Adopt the ask frame's event_id - the stable per-ask identity the runtime mints (core/agent/clarification.py: 'event_id is the clarification's stable identity') and forwards on ask frames and replayed trace rows - as the clarification round id everywhere the frontend previously read only the never-emitted request_id: the task_waiting_for_user handler, the agent_message trace reader, and a new findWaitingRequestId trace fallback in the conversation panel, sourced from the same ask events the prompt and interactions already fall back to. request_id stays the preferred field so a backend that later adopts the explicit name wins. - Stop the task-state version guard from swallowing error notices (round-3 finding F3): a stale-versioned error/agent_error frame now falls through with its control tuple neutralized - the notice (and, since #2124, the structured terminal command outcome) is sent exactly once and is not versioned state, so it keeps flowing while every status side effect is suppressed. Extends the guard's own documented rule for UNversioned error frames to versioned ones. Differentially verified: the regression test fails on the unfixed code. - Key the panel's active waiting instance by round id when one exists (round-3 finding F6): prompt-text equality could leave the persisted timeline message and the virtual waiting message simultaneously active for one round; the virtual copy now stays inert whenever a timeline message owns the round, so at most one form accepts a submission. --- .../task/task-conversation-panel.test.tsx | 186 ++++++++++++++++++ .../task/task-conversation-panel.tsx | 102 +++++++++- .../src/contexts/app-context-chat.test.tsx | 133 +++++++++++++ frontend/src/contexts/app-context-chat.tsx | 60 ++++-- 4 files changed, 467 insertions(+), 14 deletions(-) diff --git a/frontend/src/components/task/task-conversation-panel.test.tsx b/frontend/src/components/task/task-conversation-panel.test.tsx index 2ba3589dab..c39b65dcac 100644 --- a/frontend/src/components/task/task-conversation-panel.test.tsx +++ b/frontend/src/components/task/task-conversation-panel.test.tsx @@ -602,6 +602,192 @@ describe("TaskConversationPanel", () => { expect(rendered[1]).toHaveAttribute("data-request-id", "inputreq_q2") }) + it("adopts the ask frame's event_id as the waiting round id when no request_id exists", () => { + // No backend emits request_id today; the stable per-ask identity on the + // wire is event_id. The trace fallback must surface it so the round is + // identified even when the status frame carried nothing. + appState.messages = [{ + id: "user-1", + role: "user", + content: "Start the question", + timestamp: 2000, + }] + appState.traceEvents = [ + { + event_type: "agent_message", + timestamp: 1000, + data: { + expect_response: true, + message: "Which region should I use?", + event_id: "evt-round-1", + metadata: { + interactions: [{ type: "text_input", field: "region", label: "Region" }], + }, + }, + }, + ] + 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 region should I use?", + } + + render() + + const activeWait = screen.getAllByTestId("chat-message").find( + (message) => message.getAttribute("data-active") === "true", + ) + expect(activeWait).toBeDefined() + expect(activeWait).toHaveAttribute("data-request-id", "evt-round-1") + }) + + 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("never adopts a client-minted trace id as the round id", () => { + // react_task_end rows in state get generateMessageId placeholders as + // their top-level event_id; adopting one would name a round no message + // carries, deactivating every instance. With no data-level id on the + // newest ask, the prompt-text match stays in charge and activates the + // persisted bubble. + appState.messages = [ + { + id: "q1", + role: "assistant", + content: "Choose a region", + timestamp: 1000, + isResult: true, + interactions: [{ type: "text_input", field: "region", label: "Region" }], + }, + { + id: "u1", + role: "user", + content: "Working on it", + timestamp: 2000, + }, + ] + appState.traceEvents = [ + { + event_id: "react-task-end-1757200000000-abc12", + event_type: "react_task_end", + timestamp: 1500, + data: { + result: { + status: "waiting_for_user", + message: "Choose a region", + }, + }, + }, + ] + 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: "Choose a region", + } + + render() + + const active = screen.getAllByTestId("chat-message") + .filter((node) => node.getAttribute("data-active") === "true") + expect(active).toHaveLength(1) + expect(active[0]).toHaveTextContent("Choose a region") + expect(active[0]).toHaveAttribute("data-request-id", "") + }) + + 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..64666d2d60 100644 --- a/frontend/src/components/task/task-conversation-panel.tsx +++ b/frontend/src/components/task/task-conversation-panel.tsx @@ -151,6 +151,73 @@ const findWaitingPrompt = (currentTask: any, traceEvents: any[]) => { return null } +// The waiting round's identity. Prefers the id the task-state handler +// already extracted (request_id, falling back to the ask frame's event_id - +// see the app context's task_waiting_for_user case); when the status frame +// carried none, falls back to the same ask trace events the prompt and +// interactions fall back to, so all three stay sourced from one ask. +type WaitingRoundTraceEvent = { + event_id?: unknown + event_type?: unknown + data?: { + request_id?: unknown + event_id?: unknown + expect_response?: unknown + result?: { + status?: unknown + request_id?: unknown + event_id?: unknown + } + } +} + +const firstRoundId = (...candidates: unknown[]): string | undefined => { + for (const candidate of candidates) { + if (typeof candidate === "string" && candidate) return candidate + } + return undefined +} + +const findWaitingRequestId = ( + currentTask: { status?: unknown; waitingRequestId?: unknown } | null | undefined, + traceEvents: WaitingRoundTraceEvent[], +): string | undefined => { + if (currentTask?.status !== "waiting_for_user") { + return undefined + } + if ( + typeof currentTask.waitingRequestId === "string" + && currentTask.waitingRequestId + ) { + return currentTask.waitingRequestId + } + + // Only data-level ids count: a trace row's top-level event_id can be a + // client-minted placeholder (react_task_end rows get generateMessageId + // ids), and adopting one would name a round no message ever carried, + // leaving no instance active. And the scan STOPS at the most recent ask + // whether or not it carries an id - reaching past it could return an + // older round's id for the current question. An id-less newest ask + // returns undefined so the prompt-text match below stays in charge. + for (let i = traceEvents.length - 1; i >= 0; i--) { + const event = traceEvents[i] + if (event.event_type === "agent_message" && event.data?.expect_response === true) { + return firstRoundId(event.data?.request_id, event.data?.event_id) + } + if ( + event.event_type === "react_task_end" + && event.data?.result?.status === "waiting_for_user" + ) { + return firstRoundId( + event.data?.result?.request_id, + event.data?.result?.event_id, + ) + } + } + + return undefined +} + const findWaitingInteractions = (currentTask: any, traceEvents: any[]) => { if (currentTask?.status !== "waiting_for_user") { return undefined @@ -480,12 +547,34 @@ export function TaskConversationPanel({ () => findWaitingInteractions(state.currentTask, managerTraceEvents as any[]), [managerTraceEvents, state.currentTask] ) + const waitingRoundId = useMemo( + () => findWaitingRequestId( + state.currentTask, + managerTraceEvents as WaitingRoundTraceEvent[], + ), + [managerTraceEvents, state.currentTask] + ) const activeWaitingMessageId = useMemo(() => { if (state.currentTask?.status !== "waiting_for_user") { return null } + // When the waiting round has an identity, match by it exactly: 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. + 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 + } + } + return null + } + if (waitingPrompt) { const normalizedPrompt = waitingPrompt.trim() for (let i = messageItems.length - 1; i >= 0; i--) { @@ -504,7 +593,7 @@ export function TaskConversationPanel({ } return null - }, [messageItems, state.currentTask?.status, waitingPrompt]) + }, [messageItems, state.currentTask?.status, waitingPrompt, waitingRoundId]) useEffect(() => { messagesEndRef.current?.scrollIntoView?.({ behavior: "smooth" }) @@ -815,8 +904,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} /> diff --git a/frontend/src/contexts/app-context-chat.test.tsx b/frontend/src/contexts/app-context-chat.test.tsx index 69cdbbe940..a3146561c9 100644 --- a/frontend/src/contexts/app-context-chat.test.tsx +++ b/frontend/src/contexts/app-context-chat.test.tsx @@ -6646,3 +6646,136 @@ 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 ask frame's event_id as the waiting round id", async () => { + // No backend emits request_id; the stable per-ask identity is event_id. + // request_id stays the preferred field so a backend that later adopts + // the explicit name wins over the fallback. + 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?", + event_id: "evt-round-1", + interactions: [ + { type: "text", prompt: "Which region should I use?" }, + ], + } 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?", + request_id: "req-explicit", + event_id: "evt-round-2", + interactions: [ + { type: "text", prompt: "Which hotel?" }, + ], + } as TestWebSocketMessage) + }) + await waitFor(() => { + expect(screen.getByTestId("waiting-request-id").textContent).toBe("req-explicit") + }) + }) + + 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") + }) +}) diff --git a/frontend/src/contexts/app-context-chat.tsx b/frontend/src/contexts/app-context-chat.tsx index 77ba9e7637..120a284118 100644 --- a/frontend/src/contexts/app-context-chat.tsx +++ b/frontend/src/contexts/app-context-chat.tsx @@ -2864,6 +2864,16 @@ export function AppProvider({ } const controlEnvelope = extractTaskControlEnvelope(message) + // A stale-versioned error frame must not roll task state back, but its + // body is not versioned state: it carries the error notice - and, since + // #2124, the structured terminal command outcome - which the backend + // sends exactly once. Dropping the whole frame silences that notice + // forever, so error frames fall through with their control tuple + // neutralized (this flag suppresses every status side effect below) + // instead of being swallowed. This extends the same reasoning + // ``canAcceptTaskControlVersion`` already applies to UNversioned error + // frames ("error frames remain informational") to versioned ones. + let staleControlErrorFrame = false if (controlEnvelope.isStateEvent && controlEnvelope.taskId !== undefined) { if ( !acceptTaskControlVersion( @@ -2871,13 +2881,16 @@ export function AppProvider({ controlEnvelope, taskStateVersionsRef.current, ) - ) return + ) { + if (message.type !== "error" && message.type !== "agent_error") return + staleControlErrorFrame = true + } // A late event may have an old semantic type (for example // ``task_paused``) after a newer run is already RUNNING. The backend // rewrites its state tuple to the canonical row; apply only that tuple // and skip the stale event-specific side effects. - if (!taskEventMatchesControlState(message, controlEnvelope)) { + if (!staleControlErrorFrame && !taskEventMatchesControlState(message, controlEnvelope)) { if (controlEnvelope.status) { dispatch({ type: "UPDATE_TASK_STATUS", @@ -3198,9 +3211,19 @@ export function AppProvider({ return } const interactions = normalizeInteractions(eventData.metadata?.interactions) - const interactionRequestId = typeof eventData.request_id === "string" - ? eventData.request_id - : undefined + // The round identity: no backend emits ``request_id`` today - the + // stable per-ask id the runtime mints and every ask frame carries + // is ``event_id`` (see core/agent/clarification.py: "event_id is + // the clarification's stable identity"). ``request_id`` stays the + // preferred field so a backend that later adopts the explicit + // name wins over the fallback. + const interactionRequestId = + (typeof eventData.request_id === "string" && eventData.request_id + ? eventData.request_id + : undefined) + ?? (typeof eventData.event_id === "string" && eventData.event_id + ? eventData.event_id + : undefined) const isAgentMessage = eventType === "agent_message" const isAiMessage = eventType === "ai_message" const expectsUserResponse = @@ -5685,10 +5708,15 @@ export function AppProvider({ const interactions = normalizeInteractions( waitingRoot.interactions ?? waitingData.interactions ) + // ``request_id`` first (the explicit name, if a backend ever emits + // it), then ``event_id`` - the stable per-ask identity the runtime + // actually mints and forwards on ask frames today. const waitingRequestIdValue = waitingRoot.request_id ?? waitingData.request_id - const waitingRequestId = typeof waitingRequestIdValue === "string" - ? waitingRequestIdValue - : undefined + ?? waitingRoot.event_id ?? waitingData.event_id + const waitingRequestId = + typeof waitingRequestIdValue === "string" && waitingRequestIdValue + ? waitingRequestIdValue + : undefined dispatch({ type: "UPDATE_TASK_STATUS", payload: { @@ -5747,7 +5775,11 @@ export function AppProvider({ const agentErrorMessage = agentErrorCode ? t(clientErrorTranslationKey(agentErrorCode)) : getWebSocketErrorMessage(message, trustLegacyErrorProse) - const agentErrorTaskStatus = getWebSocketTaskStatus(message) + // A stale-versioned frame keeps its notice but asserts nothing about + // task state - its control tuple lost to a newer version above. + const agentErrorTaskStatus = staleControlErrorFrame + ? null + : getWebSocketTaskStatus(message) if (agentErrorTaskStatus) { dispatch({ @@ -5796,11 +5828,17 @@ export function AppProvider({ controlEnvelope, }) - if (errorFrame.taskStatus) { + // Stale-versioned "error" frames keep their notice but assert + // nothing about task state (see staleControlErrorFrame above). + // task_error deliberately stays outside that exemption - its bubble + // IS the turn's terminal result, mirroring the unversioned rule in + // canAcceptTaskControlVersion - so for task_error this guard is + // vacuously true. + if (errorFrame.taskStatus && !staleControlErrorFrame) { dispatch({ type: "UPDATE_TASK_STATUS", payload: { status: errorFrame.taskStatus } }) dispatch({ type: "TRIGGER_TASK_UPDATE" }) } - if (errorFrame.stopsProcessing) { + if (errorFrame.stopsProcessing && !staleControlErrorFrame) { dispatch({ type: "SET_PROCESSING", payload: false }) } From 309e270d5e70b87f5056af5092621af6238ff726 Mon Sep 17 00:00:00 2001 From: leyoonafr Date: Mon, 7 Sep 2026 07:54:45 +0800 Subject: [PATCH 2/9] fix(frontend): take the first non-empty string as the round id Review round 1 of #2166: nullish coalescing let an empty-string (or non-string) request_id block the event_id fallback, leaving the round id-less. Both extraction sites now take the first non-empty string candidate, matching the panel helper's semantics; regression test covers the empty-string fallback. --- .../src/contexts/app-context-chat.test.tsx | 29 ++++++++++++++++++ frontend/src/contexts/app-context-chat.tsx | 30 +++++++++---------- 2 files changed, 44 insertions(+), 15 deletions(-) diff --git a/frontend/src/contexts/app-context-chat.test.tsx b/frontend/src/contexts/app-context-chat.test.tsx index a3146561c9..85f72a6246 100644 --- a/frontend/src/contexts/app-context-chat.test.tsx +++ b/frontend/src/contexts/app-context-chat.test.tsx @@ -6722,6 +6722,35 @@ describe("clarification round identity (#1500)", () => { }) }) + it("falls back to event_id when request_id is an empty string", async () => { + // Nullish coalescing alone would let an empty request_id block the + // event_id fallback and leave the round id-less. + 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: "", + event_id: "evt-round-3", + } as TestWebSocketMessage) + }) + await waitFor(() => { + expect(screen.getByTestId("waiting-request-id").textContent).toBe("evt-round-3") + }) + }) + 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 diff --git a/frontend/src/contexts/app-context-chat.tsx b/frontend/src/contexts/app-context-chat.tsx index 120a284118..1c49304588 100644 --- a/frontend/src/contexts/app-context-chat.tsx +++ b/frontend/src/contexts/app-context-chat.tsx @@ -3216,14 +3216,12 @@ export function AppProvider({ // is ``event_id`` (see core/agent/clarification.py: "event_id is // the clarification's stable identity"). ``request_id`` stays the // preferred field so a backend that later adopts the explicit - // name wins over the fallback. - const interactionRequestId = - (typeof eventData.request_id === "string" && eventData.request_id - ? eventData.request_id - : undefined) - ?? (typeof eventData.event_id === "string" && eventData.event_id - ? eventData.event_id - : undefined) + // name wins over the fallback - but only with a non-empty + // string; anything else falls through to the next candidate. + const interactionRequestId = [ + eventData.request_id, + eventData.event_id, + ].find((id): id is string => typeof id === "string" && id !== "") const isAgentMessage = eventType === "agent_message" const isAiMessage = eventType === "ai_message" const expectsUserResponse = @@ -5710,13 +5708,15 @@ export function AppProvider({ ) // ``request_id`` first (the explicit name, if a backend ever emits // it), then ``event_id`` - the stable per-ask identity the runtime - // actually mints and forwards on ask frames today. - const waitingRequestIdValue = waitingRoot.request_id ?? waitingData.request_id - ?? waitingRoot.event_id ?? waitingData.event_id - const waitingRequestId = - typeof waitingRequestIdValue === "string" && waitingRequestIdValue - ? waitingRequestIdValue - : undefined + // actually mints and forwards on ask frames today. First non-empty + // string wins: nullish coalescing alone would let an empty or + // non-string ``request_id`` block the ``event_id`` fallback. + const waitingRequestId = [ + waitingRoot.request_id, + waitingData.request_id, + waitingRoot.event_id, + waitingData.event_id, + ].find((id): id is string => typeof id === "string" && id !== "") dispatch({ type: "UPDATE_TASK_STATUS", payload: { From 146704920fdd656a4614a20a53f6e767ee83a4ac Mon Sep 17 00:00:00 2001 From: leyoonafr Date: Mon, 7 Sep 2026 13:09:02 +0800 Subject: [PATCH 3/9] fix(frontend): keep the waiting round reachable and its id alive across reasserts Review round 2 of #2166: - A failed round-id match in activeWaitingMessageId falls through to the prompt-text fallback instead of short-circuiting: replayed history rows carry no interactionRequestId, and with the ask as the last assistant message the virtual bubble is suppressed too, so the short-circuit could leave zero active reply instances for a waiting task (Critical). - The reducer keeps waitingRequestId across an id-less reassertion that re-sends the SAME question (reload/reconnect frames), while an id-less frame carrying a different question - a legacy-backend new round - still clears it (Major). - ChatInput's currentInteractionRequestId now receives waitingRoundId, the same identity (with the ask-trace fallback) the form path uses, so a free-text reply binds to the round after a reload too (Major). - findWaitingRequestId's fallback reads the replayed waiting result's real identity at result.clarification_draft.event_id and drops the agent_message arm, which never matches production trace rows (live asks land in the transcript instead); tests now use production shapes. - The stale-error exemption comment states the actual asymmetry with the guard's unversioned rule instead of claiming parity; added tests for the plain-error-type exemption, task_error's deliberate exclusion, and the stop-at-newest scan invariant. - The first-non-empty-string extraction is one shared lib/utils helper instead of three copies. --- .../task/task-conversation-panel.test.tsx | 151 ++++++++++++++- .../task/task-conversation-panel.tsx | 66 ++++--- .../src/contexts/app-context-chat.test.tsx | 178 ++++++++++++++++++ frontend/src/contexts/app-context-chat.tsx | 32 +++- frontend/src/lib/utils.ts | 14 ++ 5 files changed, 391 insertions(+), 50 deletions(-) diff --git a/frontend/src/components/task/task-conversation-panel.test.tsx b/frontend/src/components/task/task-conversation-panel.test.tsx index c39b65dcac..e2413931a6 100644 --- a/frontend/src/components/task/task-conversation-panel.test.tsx +++ b/frontend/src/components/task/task-conversation-panel.test.tsx @@ -602,10 +602,10 @@ describe("TaskConversationPanel", () => { expect(rendered[1]).toHaveAttribute("data-request-id", "inputreq_q2") }) - it("adopts the ask frame's event_id as the waiting round id when no request_id exists", () => { - // No backend emits request_id today; the stable per-ask identity on the - // wire is event_id. The trace fallback must surface it so the round is - // identified even when the status frame carried nothing. + it("adopts the replayed waiting result's clarification event_id as the round id", () => { + // No backend emits request_id today, and replayed history rows carry no + // interactionRequestId; after a reload the round identity lives at the + // replayed react_task_end's result.clarification_draft.event_id. appState.messages = [{ id: "user-1", role: "user", @@ -614,14 +614,15 @@ describe("TaskConversationPanel", () => { }] appState.traceEvents = [ { - event_type: "agent_message", + event_id: "react-task-end-1757200000000-abc12", + event_type: "react_task_end", timestamp: 1000, data: { - expect_response: true, - message: "Which region should I use?", - event_id: "evt-round-1", - metadata: { + result: { + status: "waiting_for_user", + message: "Which region should I use?", interactions: [{ type: "text_input", field: "region", label: "Region" }], + clarification_draft: { event_id: "evt-round-1" }, }, }, }, @@ -645,6 +646,138 @@ describe("TaskConversationPanel", () => { expect(activeWait).toHaveAttribute("data-request-id", "evt-round-1") }) + it("stops the round-id scan at the newest waiting result", () => { + // Reaching past an id-less newest result would return an OLDER round's + // id for the current question; the scan must stop and leave the round + // id-less so the prompt-text match stays in charge. + appState.messages = [{ + id: "user-1", + role: "user", + content: "Working on it", + timestamp: 3000, + }] + appState.traceEvents = [ + { + event_type: "react_task_end", + timestamp: 1000, + data: { + result: { + status: "waiting_for_user", + message: "Old question", + clarification_draft: { event_id: "evt-old-round" }, + }, + }, + }, + { + event_type: "react_task_end", + timestamp: 2000, + data: { + result: { + status: "waiting_for_user", + message: "Which city?", + }, + }, + }, + ] + 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?", + } + + render() + + const activeWait = screen.getAllByTestId("chat-message").find( + (message) => message.getAttribute("data-active") === "true", + ) + expect(activeWait).toBeDefined() + expect(activeWait).toHaveAttribute("data-request-id", "") + }) + + 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("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 diff --git a/frontend/src/components/task/task-conversation-panel.tsx b/frontend/src/components/task/task-conversation-panel.tsx index 64666d2d60..5d01d4702c 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" @@ -154,30 +154,21 @@ const findWaitingPrompt = (currentTask: any, traceEvents: any[]) => { // The waiting round's identity. Prefers the id the task-state handler // already extracted (request_id, falling back to the ask frame's event_id - // see the app context's task_waiting_for_user case); when the status frame -// carried none, falls back to the same ask trace events the prompt and -// interactions fall back to, so all three stay sourced from one ask. +// carried none, falls back to the replayed waiting result the prompt and +// interactions also fall back to, so all three stay sourced from one ask. type WaitingRoundTraceEvent = { - event_id?: unknown event_type?: unknown data?: { - request_id?: unknown - event_id?: unknown - expect_response?: unknown result?: { status?: unknown request_id?: unknown - event_id?: unknown + clarification_draft?: { + event_id?: unknown + } } } } -const firstRoundId = (...candidates: unknown[]): string | undefined => { - for (const candidate of candidates) { - if (typeof candidate === "string" && candidate) return candidate - } - return undefined -} - const findWaitingRequestId = ( currentTask: { status?: unknown; waitingRequestId?: unknown } | null | undefined, traceEvents: WaitingRoundTraceEvent[], @@ -192,25 +183,24 @@ const findWaitingRequestId = ( return currentTask.waitingRequestId } - // Only data-level ids count: a trace row's top-level event_id can be a - // client-minted placeholder (react_task_end rows get generateMessageId - // ids), and adopting one would name a round no message ever carried, - // leaving no instance active. And the scan STOPS at the most recent ask - // whether or not it carries an id - reaching past it could return an - // older round's id for the current question. An id-less newest ask - // returns undefined so the prompt-text match below stays in charge. + // Only react_task_end rows reach state.traceEvents for a waiting result + // (a live ask's agent_message lands in the transcript instead, already + // carrying its interactionRequestId), and the round identity on such a + // row lives at result.clarification_draft.event_id. A row's top-level + // event_id never counts: it is a client-minted placeholder, and adopting + // one would name a round no message ever carried. The scan STOPS at the + // most recent waiting result whether or not it yields an id - reaching + // past it could return an older round's id - and an id-less newest result + // returns undefined so the prompt-text match stays in charge. for (let i = traceEvents.length - 1; i >= 0; i--) { const event = traceEvents[i] - if (event.event_type === "agent_message" && event.data?.expect_response === true) { - return firstRoundId(event.data?.request_id, event.data?.event_id) - } if ( event.event_type === "react_task_end" && event.data?.result?.status === "waiting_for_user" ) { - return firstRoundId( + return firstNonEmptyString( event.data?.result?.request_id, - event.data?.result?.event_id, + event.data?.result?.clarification_draft?.event_id, ) } } @@ -560,11 +550,15 @@ export function TaskConversationPanel({ return null } - // When the waiting round has an identity, match by it exactly: the + // 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. + // 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] @@ -572,7 +566,6 @@ export function TaskConversationPanel({ return item.id } } - return null } if (waitingPrompt) { @@ -585,6 +578,14 @@ export function TaskConversationPanel({ } } + // 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) { @@ -960,7 +961,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 85f72a6246..9dab0aee2b 100644 --- a/frontend/src/contexts/app-context-chat.test.tsx +++ b/frontend/src/contexts/app-context-chat.test.tsx @@ -6751,6 +6751,184 @@ describe("clarification round identity (#1500)", () => { }) }) + 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?", + event_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?", + event_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("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 diff --git a/frontend/src/contexts/app-context-chat.tsx b/frontend/src/contexts/app-context-chat.tsx index 1c49304588..ffb9783711 100644 --- a/frontend/src/contexts/app-context-chat.tsx +++ b/frontend/src/contexts/app-context-chat.tsx @@ -354,7 +354,7 @@ import { type WebSocketConnection, type WebSocketConnectionFailure, } from "@/hooks/use-websocket" -import { generateClientMessageId, getApiUrl, getUploadApiUrl, shouldAutoOpenTaskPreview } from "@/lib/utils" +import { generateClientMessageId, getApiUrl, getUploadApiUrl, shouldAutoOpenTaskPreview, firstNonEmptyString } from "@/lib/utils" import { apiRequest, classifyUploadError, getApiErrorMessage, isJsonRecord, parseApiResponse } from "@/lib/api-wrapper" import { clientErrorTranslationKey, readClientErrorCode } from "@/lib/client-errors" import { normalizeUploadFileIds } from "@/lib/upload-file-ids" @@ -1609,8 +1609,19 @@ function projectAppState(state: AppState, action: AppAction): AppState { : undefined, waitingRequestId: isWaitingForUser ? action.payload.waitingRequestId ?? ( - action.payload.waitingQuestion === undefined - && action.payload.waitingInteractions === undefined + // An id-less payload keeps the known id when it asserts + // nothing new - and also when it re-asserts the SAME question + // text: reload/reconnect reassertion frames re-send the + // unchanged question with no id, and wiping the id there + // severs the open round's correlation. An id-less payload + // carrying a DIFFERENT question (or fresh interactions + // without one) is a legacy-backend new round and must not + // inherit the previous round's id. + (action.payload.waitingQuestion === undefined + && action.payload.waitingInteractions === undefined) + || (action.payload.waitingQuestion !== undefined + && action.payload.waitingQuestion + === state.currentTask.waitingQuestion) ? state.currentTask.waitingRequestId : undefined ) @@ -2870,9 +2881,10 @@ export function AppProvider({ // sends exactly once. Dropping the whole frame silences that notice // forever, so error frames fall through with their control tuple // neutralized (this flag suppresses every status side effect below) - // instead of being swallowed. This extends the same reasoning - // ``canAcceptTaskControlVersion`` already applies to UNversioned error - // frames ("error frames remain informational") to versioned ones. + // instead of being swallowed. Weaker than the guard's UNversioned-error + // rule, deliberately: an unversioned error frame has no version to lose + // and passes whole, control tuple included; a stale VERSIONED one lost + // the version argument and keeps only its notice. let staleControlErrorFrame = false if (controlEnvelope.isStateEvent && controlEnvelope.taskId !== undefined) { if ( @@ -3218,10 +3230,10 @@ export function AppProvider({ // preferred field so a backend that later adopts the explicit // name wins over the fallback - but only with a non-empty // string; anything else falls through to the next candidate. - const interactionRequestId = [ + const interactionRequestId = firstNonEmptyString( eventData.request_id, eventData.event_id, - ].find((id): id is string => typeof id === "string" && id !== "") + ) const isAgentMessage = eventType === "agent_message" const isAiMessage = eventType === "ai_message" const expectsUserResponse = @@ -5711,12 +5723,12 @@ export function AppProvider({ // actually mints and forwards on ask frames today. First non-empty // string wins: nullish coalescing alone would let an empty or // non-string ``request_id`` block the ``event_id`` fallback. - const waitingRequestId = [ + const waitingRequestId = firstNonEmptyString( waitingRoot.request_id, waitingData.request_id, waitingRoot.event_id, waitingData.event_id, - ].find((id): id is string => typeof id === "string" && id !== "") + ) dispatch({ type: "UPDATE_TASK_STATUS", payload: { diff --git a/frontend/src/lib/utils.ts b/frontend/src/lib/utils.ts index 1a1699f4c0..9b41475ad4 100644 --- a/frontend/src/lib/utils.ts +++ b/frontend/src/lib/utils.ts @@ -5,6 +5,20 @@ export function cn(...inputs: ClassValue[]) { return twMerge(clsx(inputs)) } +/** + * First candidate that is a non-empty string. Shared by the clarification + * round-id extraction sites: nullish coalescing alone would let an empty or + * non-string preferred field block the fallback candidates. + */ +export function firstNonEmptyString( + ...candidates: unknown[] +): string | undefined { + for (const candidate of candidates) { + if (typeof candidate === "string" && candidate !== "") return candidate + } + return undefined +} + export function generateClientMessageId(): string { return globalThis.crypto?.randomUUID?.() ?? `msg-${Date.now()}-${Math.random().toString(36).slice(2)}` From 52e8b6691ee9903038107d6bcfa2824fd0448b88 Mon Sep 17 00:00:00 2001 From: leyoonafr Date: Tue, 8 Sep 2026 12:32:30 +0800 Subject: [PATCH 4/9] fix(frontend): read the round id off the frames instead of reconstructing it Review round 3 of #2166. The round established that client-side reconstruction of the waiting round identity cannot work: the waiting task_info frames carried no id, chat-history rows carry no id, and public_trace_events strips clarification_draft from waiting react_task_end results for every audience. The companion backend PR now emits request_id on the waiting task_info and replay reassertion frames, so this PR sheds the reconstruction machinery instead of patching it: - findWaitingRequestId, its trace-event type, and the waitingRoundId scan are deleted; the round id is read directly off the task state, and the task_info shaping maps the frame's new request_id into waitingRequestId (the new consumption site). - The active timeline instance's form receives the resolved round id when its own row is id-less (replayed chat-history rows), so an elected form never submits an id-less reply (round-3 finding on the text-fallback election). - The prompt-text fallback skips lookalike rows that carry a DIFFERENT round id - only id-less rows may be text-elected. - The one adoption site production always exercises - the live ask's agent_message data.event_id becoming the transcript message's interactionRequestId - now has its own test, alongside the new task_info consumption test. Kept from round 2: the reducer's same-question id preservation, ChatInput's round id, the single-active-instance rule with its fall-through, and the shared firstNonEmptyString helper. --- .../task/task-conversation-panel.test.tsx | 178 ++++++------------ .../task/task-conversation-panel.tsx | 95 +++------- .../src/contexts/app-context-chat.test.tsx | 145 ++++++++++++++ frontend/src/contexts/app-context-chat.tsx | 9 + 4 files changed, 244 insertions(+), 183 deletions(-) diff --git a/frontend/src/components/task/task-conversation-panel.test.tsx b/frontend/src/components/task/task-conversation-panel.test.tsx index e2413931a6..38b9d22dc7 100644 --- a/frontend/src/components/task/task-conversation-panel.test.tsx +++ b/frontend/src/components/task/task-conversation-panel.test.tsx @@ -602,102 +602,6 @@ describe("TaskConversationPanel", () => { expect(rendered[1]).toHaveAttribute("data-request-id", "inputreq_q2") }) - it("adopts the replayed waiting result's clarification event_id as the round id", () => { - // No backend emits request_id today, and replayed history rows carry no - // interactionRequestId; after a reload the round identity lives at the - // replayed react_task_end's result.clarification_draft.event_id. - appState.messages = [{ - id: "user-1", - role: "user", - content: "Start the question", - timestamp: 2000, - }] - appState.traceEvents = [ - { - event_id: "react-task-end-1757200000000-abc12", - event_type: "react_task_end", - timestamp: 1000, - data: { - result: { - status: "waiting_for_user", - message: "Which region should I use?", - interactions: [{ type: "text_input", field: "region", label: "Region" }], - clarification_draft: { event_id: "evt-round-1" }, - }, - }, - }, - ] - 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 region should I use?", - } - - render() - - const activeWait = screen.getAllByTestId("chat-message").find( - (message) => message.getAttribute("data-active") === "true", - ) - expect(activeWait).toBeDefined() - expect(activeWait).toHaveAttribute("data-request-id", "evt-round-1") - }) - - it("stops the round-id scan at the newest waiting result", () => { - // Reaching past an id-less newest result would return an OLDER round's - // id for the current question; the scan must stop and leave the round - // id-less so the prompt-text match stays in charge. - appState.messages = [{ - id: "user-1", - role: "user", - content: "Working on it", - timestamp: 3000, - }] - appState.traceEvents = [ - { - event_type: "react_task_end", - timestamp: 1000, - data: { - result: { - status: "waiting_for_user", - message: "Old question", - clarification_draft: { event_id: "evt-old-round" }, - }, - }, - }, - { - event_type: "react_task_end", - timestamp: 2000, - data: { - result: { - status: "waiting_for_user", - message: "Which city?", - }, - }, - }, - ] - 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?", - } - - render() - - const activeWait = screen.getAllByTestId("chat-message").find( - (message) => message.getAttribute("data-active") === "true", - ) - expect(activeWait).toBeDefined() - expect(activeWait).toHaveAttribute("data-request-id", "") - }) - 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; @@ -824,20 +728,18 @@ describe("TaskConversationPanel", () => { expect(active[0]).toHaveTextContent("Which city?") }) - it("never adopts a client-minted trace id as the round id", () => { - // react_task_end rows in state get generateMessageId placeholders as - // their top-level event_id; adopting one would name a round no message - // carries, deactivating every instance. With no data-level id on the - // newest ask, the prompt-text match stays in charge and activates the - // persisted bubble. + 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: "Choose a region", + content: "Which city?", timestamp: 1000, isResult: true, - interactions: [{ type: "text_input", field: "region", label: "Region" }], + interactions: [{ type: "text_input", field: "city", label: "City" }], }, { id: "u1", @@ -846,19 +748,60 @@ describe("TaskConversationPanel", () => { timestamp: 2000, }, ] - appState.traceEvents = [ + 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 = [ { - event_id: "react-task-end-1757200000000-abc12", - event_type: "react_task_end", - timestamp: 1500, - data: { - result: { - status: "waiting_for_user", - message: "Choose a region", - }, - }, + 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", @@ -866,7 +809,8 @@ describe("TaskConversationPanel", () => { status: "waiting_for_user", createdAt: "2026-01-01T00:00:00Z", updatedAt: "2026-01-01T00:00:00Z", - waitingQuestion: "Choose a region", + waitingQuestion: "Which city?", + waitingRequestId: "evt-round-2", } render() @@ -874,8 +818,8 @@ describe("TaskConversationPanel", () => { const active = screen.getAllByTestId("chat-message") .filter((node) => node.getAttribute("data-active") === "true") expect(active).toHaveLength(1) - expect(active[0]).toHaveTextContent("Choose a region") - expect(active[0]).toHaveAttribute("data-request-id", "") + // 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", () => { diff --git a/frontend/src/components/task/task-conversation-panel.tsx b/frontend/src/components/task/task-conversation-panel.tsx index 5d01d4702c..fb311751e6 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, firstNonEmptyString } from "@/lib/utils" +import { cn } from "@/lib/utils" export type TaskConversationPanelMode = "page" | "embedded-preview" @@ -151,63 +151,6 @@ const findWaitingPrompt = (currentTask: any, traceEvents: any[]) => { return null } -// The waiting round's identity. Prefers the id the task-state handler -// already extracted (request_id, falling back to the ask frame's event_id - -// see the app context's task_waiting_for_user case); when the status frame -// carried none, falls back to the replayed waiting result the prompt and -// interactions also fall back to, so all three stay sourced from one ask. -type WaitingRoundTraceEvent = { - event_type?: unknown - data?: { - result?: { - status?: unknown - request_id?: unknown - clarification_draft?: { - event_id?: unknown - } - } - } -} - -const findWaitingRequestId = ( - currentTask: { status?: unknown; waitingRequestId?: unknown } | null | undefined, - traceEvents: WaitingRoundTraceEvent[], -): string | undefined => { - if (currentTask?.status !== "waiting_for_user") { - return undefined - } - if ( - typeof currentTask.waitingRequestId === "string" - && currentTask.waitingRequestId - ) { - return currentTask.waitingRequestId - } - - // Only react_task_end rows reach state.traceEvents for a waiting result - // (a live ask's agent_message lands in the transcript instead, already - // carrying its interactionRequestId), and the round identity on such a - // row lives at result.clarification_draft.event_id. A row's top-level - // event_id never counts: it is a client-minted placeholder, and adopting - // one would name a round no message ever carried. The scan STOPS at the - // most recent waiting result whether or not it yields an id - reaching - // past it could return an older round's id - and an id-less newest result - // returns undefined so the prompt-text match stays in charge. - for (let i = traceEvents.length - 1; i >= 0; i--) { - const event = traceEvents[i] - if ( - event.event_type === "react_task_end" - && event.data?.result?.status === "waiting_for_user" - ) { - return firstNonEmptyString( - event.data?.result?.request_id, - event.data?.result?.clarification_draft?.event_id, - ) - } - } - - return undefined -} - const findWaitingInteractions = (currentTask: any, traceEvents: any[]) => { if (currentTask?.status !== "waiting_for_user") { return undefined @@ -537,13 +480,14 @@ export function TaskConversationPanel({ () => findWaitingInteractions(state.currentTask, managerTraceEvents as any[]), [managerTraceEvents, state.currentTask] ) - const waitingRoundId = useMemo( - () => findWaitingRequestId( - state.currentTask, - managerTraceEvents as WaitingRoundTraceEvent[], - ), - [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. + const waitingRoundId = + state.currentTask?.status === "waiting_for_user" + ? state.currentTask.waitingRequestId + : undefined const activeWaitingMessageId = useMemo(() => { if (state.currentTask?.status !== "waiting_for_user") { @@ -572,6 +516,16 @@ export function TaskConversationPanel({ 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 } @@ -878,7 +832,16 @@ 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 + ? item.interactionRequestId ?? waitingRoundId + : item.interactionRequestId + } interactionsActive={item.id === activeWaitingMessageId} showEmptyStatus={item.showEmptyStatus} contextBadges={item.role === "user" ? userMessageContextBadges : undefined} diff --git a/frontend/src/contexts/app-context-chat.test.tsx b/frontend/src/contexts/app-context-chat.test.tsx index 9dab0aee2b..04d2fe336c 100644 --- a/frontend/src/contexts/app-context-chat.test.tsx +++ b/frontend/src/contexts/app-context-chat.test.tsx @@ -6722,6 +6722,151 @@ describe("clarification round identity (#1500)", () => { }) }) + 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?", + event_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("falls back to event_id when request_id is an empty string", async () => { // Nullish coalescing alone would let an empty request_id block the // event_id fallback and leave the round id-less. diff --git a/frontend/src/contexts/app-context-chat.tsx b/frontend/src/contexts/app-context-chat.tsx index ffb9783711..7b23dc6f2f 100644 --- a/frontend/src/contexts/app-context-chat.tsx +++ b/frontend/src/contexts/app-context-chat.tsx @@ -890,6 +890,15 @@ const taskFromTaskInfoData = ( runtimeExtensionBindings: getStringArray(taskData.runtime_extension_bindings), waitingQuestion: taskData.waiting_question as string | undefined, waitingInteractions: normalizeInteractions(taskData.waiting_interactions), + // The waiting round's identity, emitted on waiting task_info frames + // (live, resume, and replay) so a reply binds to the exact ask (#1500). + // Key present only when the frame carries one: an explicit undefined + // would let SET_CURRENT_TASK's merge wipe an id the waiting handler + // already holds when an id-less task_info (a backend predating the + // emission) arrives mid-round. + ...(firstNonEmptyString(taskData.request_id) !== undefined + ? { waitingRequestId: firstNonEmptyString(taskData.request_id) } + : {}), runId: taskData.run_id as string | null | undefined, stateVersion: parseInteger(taskData.state_version), controlState: taskData.control_state as TaskControlState | undefined, From 446e5b893fcf774fc084c9bda38c46c449bc09e7 Mon Sep 17 00:00:00 2001 From: leyoonafr Date: Wed, 9 Sep 2026 11:01:25 +0800 Subject: [PATCH 5/9] fix(frontend): round-4 hardening for the round-id read surfaces Review round 4 of #2166: - The task_waiting_for_user handler reads request_id only: its emitters (backend #2232) carry the round identity under the explicit name and never emit event_id, so the event_id candidates there were dead code testable only with hand-crafted frames. The ask frame's event_id stays adopted where it genuinely lives - the agent_message trace reader. The prior review reply claiming those candidates were exercised is corrected in the thread. - A duplicated delivery of the SAME terminal agent_error broadcast no longer adds a second bubble: dedup is keyed on the frame's durable identity (command_id + outcome_version) and never on text, so two distinct commands failing with identical redacted text both stay visible. - The panel's round-id read is gated on the task actually being on screen (currentTask.id === taskId), the same guard the sibling ChatInput wiring uses - new code must not re-enter the task-switch window tracked in #2221. - The active-item id resolution uses firstNonEmptyString for the same empty-string semantics as every other id read; the helper gains a direct unit test. --- .../task/task-conversation-panel.test.tsx | 26 +++++ .../task/task-conversation-panel.tsx | 12 +- .../src/contexts/app-context-chat.test.tsx | 108 +++++++++++------- frontend/src/contexts/app-context-chat.tsx | 55 ++++++--- frontend/src/lib/utils.test.ts | 13 ++- 5 files changed, 153 insertions(+), 61 deletions(-) diff --git a/frontend/src/components/task/task-conversation-panel.test.tsx b/frontend/src/components/task/task-conversation-panel.test.tsx index 38b9d22dc7..16ee59a300 100644 --- a/frontend/src/components/task/task-conversation-panel.test.tsx +++ b/frontend/src/components/task/task-conversation-panel.test.tsx @@ -682,6 +682,32 @@ describe("TaskConversationPanel", () => { 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 diff --git a/frontend/src/components/task/task-conversation-panel.tsx b/frontend/src/components/task/task-conversation-panel.tsx index fb311751e6..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" @@ -484,8 +484,13 @@ export function TaskConversationPanel({ // 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 @@ -839,7 +844,10 @@ export function TaskConversationPanel({ // submitting id-less. interactionRequestId={ item.id === activeWaitingMessageId - ? item.interactionRequestId ?? waitingRoundId + ? firstNonEmptyString( + item.interactionRequestId, + waitingRoundId, + ) : item.interactionRequestId } interactionsActive={item.id === activeWaitingMessageId} diff --git a/frontend/src/contexts/app-context-chat.test.tsx b/frontend/src/contexts/app-context-chat.test.tsx index 04d2fe336c..be2da9542a 100644 --- a/frontend/src/contexts/app-context-chat.test.tsx +++ b/frontend/src/contexts/app-context-chat.test.tsx @@ -6672,10 +6672,12 @@ describe("clarification round identity (#1500)", () => { localStorage.clear() }) - it("adopts the ask frame's event_id as the waiting round id", async () => { - // No backend emits request_id; the stable per-ask identity is event_id. - // request_id stays the preferred field so a backend that later adopts - // the explicit name wins over the fallback. + 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( @@ -6693,14 +6695,14 @@ describe("clarification round identity (#1500)", () => { task_id: 1, task: { id: 1, status: "waiting_for_user" }, message: "Which region should I use?", - event_id: "evt-round-1", + 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("evt-round-1") + expect(screen.getByTestId("waiting-request-id").textContent).toBe("req-round-1") }) act(() => { @@ -6710,16 +6712,17 @@ describe("clarification round identity (#1500)", () => { task_id: 1, task: { id: 1, status: "waiting_for_user" }, message: "Which hotel?", - request_id: "req-explicit", - event_id: "evt-round-2", + request_id: "", + event_id: "evt-never-adopted-here", interactions: [ { type: "text", prompt: "Which hotel?" }, ], } as TestWebSocketMessage) }) await waitFor(() => { - expect(screen.getByTestId("waiting-request-id").textContent).toBe("req-explicit") + 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 () => { @@ -6791,7 +6794,7 @@ describe("clarification round identity (#1500)", () => { task_id: 1, task: { id: 1, status: "waiting_for_user" }, message: "Which region should I use?", - event_id: "evt-live-round", + request_id: "evt-live-round", } as TestWebSocketMessage) }) await waitFor(() => { @@ -6867,35 +6870,6 @@ describe("clarification round identity (#1500)", () => { }) }) - it("falls back to event_id when request_id is an empty string", async () => { - // Nullish coalescing alone would let an empty request_id block the - // event_id fallback and leave the round id-less. - 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: "", - event_id: "evt-round-3", - } as TestWebSocketMessage) - }) - await waitFor(() => { - expect(screen.getByTestId("waiting-request-id").textContent).toBe("evt-round-3") - }) - }) - 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. @@ -6916,7 +6890,7 @@ describe("clarification round identity (#1500)", () => { task_id: 1, task: { id: 1, status: "waiting_for_user" }, message: "Which region should I use?", - event_id: "evt-round-1", + request_id: "evt-round-1", } as TestWebSocketMessage) }) await waitFor(() => { @@ -6957,7 +6931,7 @@ describe("clarification round identity (#1500)", () => { task_id: 1, task: { id: 1, status: "waiting_for_user" }, message: "Which region should I use?", - event_id: "evt-round-1", + request_id: "evt-round-1", } as TestWebSocketMessage) }) await waitFor(() => { @@ -6978,6 +6952,58 @@ describe("clarification round identity (#1500)", () => { }) }) + 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( diff --git a/frontend/src/contexts/app-context-chat.tsx b/frontend/src/contexts/app-context-chat.tsx index 7b23dc6f2f..ee957386a8 100644 --- a/frontend/src/contexts/app-context-chat.tsx +++ b/frontend/src/contexts/app-context-chat.tsx @@ -5727,16 +5727,15 @@ export function AppProvider({ const interactions = normalizeInteractions( waitingRoot.interactions ?? waitingData.interactions ) - // ``request_id`` first (the explicit name, if a backend ever emits - // it), then ``event_id`` - the stable per-ask identity the runtime - // actually mints and forwards on ask frames today. First non-empty - // string wins: nullish coalescing alone would let an empty or - // non-string ``request_id`` block the ``event_id`` fallback. + // ``request_id`` only: the waiting/reassert frames' emitters carry + // the round identity under that explicit name (backend #2232) and + // never emit ``event_id`` - an event_id fallback here would be dead + // code testable only with hand-crafted frames. The ask frame's + // ``event_id`` is adopted where it genuinely lives, in the + // agent_message trace reader. const waitingRequestId = firstNonEmptyString( waitingRoot.request_id, waitingData.request_id, - waitingRoot.event_id, - waitingData.event_id, ) dispatch({ type: "UPDATE_TASK_STATUS", @@ -5828,16 +5827,38 @@ export function AppProvider({ dispatch({ type: "SET_PROCESSING", payload: false }) } - dispatch({ - type: "ADD_MESSAGE", - payload: { - id: generateMessageId("msg"), - role: "assistant", - content: `${t('agent.logs.event.messages.errorPrefix')} ${agentErrorMessage || t('common.errors.unknown')}`, - timestamp: message.timestamp, - status: "failed", - }, - }) + const agentErrorData = asMessageRecord(message.data) + // A terminal command frame carries a durable identity + // (command_id, disambiguated by outcome_version): a duplicated + // delivery of the SAME terminal broadcast must not add a second + // bubble. Identity-keyed only - never text-keyed - so two distinct + // commands failing with identical redacted text both stay visible. + const agentErrorOccurrence = + typeof agentErrorData.command_id === "string" + && agentErrorData.command_id + ? `${agentErrorData.command_id}:${String( + agentErrorData.outcome_version ?? "", + )}` + : undefined + if ( + agentErrorOccurrence === undefined + || !isDuplicateMessageForViewedTask( + agentErrorMessage || "", + "agent-error-command", + agentErrorOccurrence, + ) + ) { + dispatch({ + type: "ADD_MESSAGE", + payload: { + id: generateMessageId("msg"), + role: "assistant", + content: `${t('agent.logs.event.messages.errorPrefix')} ${agentErrorMessage || t('common.errors.unknown')}`, + timestamp: message.timestamp, + status: "failed", + }, + }) + } break case "error": diff --git a/frontend/src/lib/utils.test.ts b/frontend/src/lib/utils.test.ts index ad2717baad..3cfd3d8249 100644 --- a/frontend/src/lib/utils.test.ts +++ b/frontend/src/lib/utils.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest" -import { resolveAgentLogoUrl } from "./utils" +import { firstNonEmptyString, resolveAgentLogoUrl } from "./utils" const bigintValue = (globalThis as { BigInt: (value: number) => bigint }).BigInt(1) @@ -175,3 +175,14 @@ describe("resolveAgentLogoUrl", () => { ) }) }) + +describe("firstNonEmptyString", () => { + it("returns the first non-empty string candidate", () => { + expect(firstNonEmptyString(undefined, null, "", 0, "id-1", "id-2")).toBe("id-1") + }) + + it("skips non-string and empty values entirely", () => { + expect(firstNonEmptyString(null, 42, {}, [], "")).toBeUndefined() + expect(firstNonEmptyString()).toBeUndefined() + }) +}) From 30843221d37026ef87c78fe6e63a9ba313b1650d Mon Sep 17 00:00:00 2001 From: leyoonafr Date: Fri, 4 Sep 2026 23:18:02 +0800 Subject: [PATCH 6/9] fix(frontend): gate clarification retries on structured terminal command outcomes A clarification form re-enabled whenever the task returned to waiting_for_user, even when the durably accepted reply's terminal outcome was unknown or possibly still in flight - inviting an accidental duplicate submission (#1500). The form now mints its own client message id (the durable command id) and records the round's accountable submission in AppContext keyed by request id, so the gate survives the submitting component instance being replaced (virtual waiting message vs. persisted timeline message). An ack-timeout (outcome_unknown) delivery records the submission too, since the reply may still have been durably accepted. The form reactivates only when the structured terminal outcome broadcast for that exact command proves the reply was not applied (resend_safe); the record is then consumed so the resend is armed once. An outcome that cannot prove non-application locks the form with a visible notice and the draft preserved; the chat input remains available for a deliberate fresh message. Rounds without a request id are not gated - with no round identity a recorded reply could gate a different question. AppContext records terminal agent_error dispositions keyed by command id; exact-id correlation is what makes stale or cross-run outcomes inert, and the reactivation effect depends on the derived per-round values so an unrelated command's outcome cannot wipe a visible failure alert. Legacy frames without the structured fields stay in the unsafe reading. Requires the wire contract from #2124 for live frames; without it every outcome reads as unknown, which is the conservative direction. --- .../chat/clarification-form.test.tsx | 254 ++++++++++++++++++ .../components/chat/clarification-form.tsx | 110 +++++++- .../src/contexts/app-context-chat.test.tsx | 235 ++++++++++++++++ frontend/src/contexts/app-context-chat.tsx | 79 ++++++ frontend/src/i18n/locales/en.ts | 2 + frontend/src/i18n/locales/zh.ts | 2 + 6 files changed, 672 insertions(+), 10 deletions(-) diff --git a/frontend/src/components/chat/clarification-form.test.tsx b/frontend/src/components/chat/clarification-form.test.tsx index 8a1c4ec1cf..38092b4727 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,253 @@ 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 outcome the + // backend broadcasts for that exact command, never by task state alone. + // The accountable submission lives in context keyed by request id, so the + // gate survives the submitting component instance being replaced. + beforeEach(() => { + appContextMock.dispatch.mockReset() + appContextMock.filesDisabled = false + appContextMock.providerAvailable = true + appContextMock.sendMessage.mockReset() + appContextMock.sendMessage.mockResolvedValue(undefined) + appContextMock.state = { commandOutcomes: {}, clarificationSubmissions: {} } + toastErrorMock.mockReset() + }) + + afterEach(() => { + appContextMock.state = { commandOutcomes: {}, clarificationSubmissions: {} } + cleanup() + }) + + const form = (active: boolean, requestId = "inputreq_r1") => ( + + ) + + const submitButton = () => + screen.queryByRole("button", { name: "chatPage.clarification.submit" }) + + // Drives one accepted submission and mirrors the RECORD dispatch into the + // mock context state, the way the real reducer would. + const submitAccepted = async (requestId = "inputreq_r1") => { + fireEvent.change(screen.getByRole("textbox"), { target: { value: "Beijing" } }) + fireEvent.click( + screen.getByRole("button", { name: "chatPage.clarification.submit" }), + ) + await waitFor(() => expect(appContextMock.sendMessage).toHaveBeenCalledTimes(1)) + // Accepted submissions collapse the form. + await waitFor(() => expect(screen.queryByRole("textbox")).toBeNull()) + const record = appContextMock.dispatch.mock.calls + .map(([action]) => action) + .find((action) => action?.type === "RECORD_CLARIFICATION_SUBMISSION") + expect(record).toEqual({ + type: "RECORD_CLARIFICATION_SUBMISSION", + payload: { requestId, commandId: expect.any(String) }, + }) + // The recorded command id is the client message id the delivery used. + const config = appContextMock.sendMessage.mock.calls[0][1] as { + clientMessageId?: string + } + expect(record.payload.commandId).toBe(config.clientMessageId) + appContextMock.state = { + ...appContextMock.state, + clarificationSubmissions: { + [requestId]: { commandId: record.payload.commandId }, + }, + } + return record.payload.commandId as string + } + + const withOutcome = (commandId: string, resendSafe: boolean) => { + appContextMock.state = { + ...appContextMock.state, + commandOutcomes: { + [commandId]: { + outcome: "failed", + resendSafe, + messageCode: "task_command_deferred", + }, + }, + } + } + + it("reactivates the form and preserves the draft when the outcome proves retry safe", async () => { + const { rerender } = render(form(true)) + const commandId = await submitAccepted() + + rerender(form(false)) + withOutcome(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. + expect(appContextMock.dispatch).toHaveBeenCalledWith({ + type: "CLEAR_CLARIFICATION_SUBMISSION", + payload: { requestId: "inputreq_r1" }, + }) + }) + + 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() + + rerender(form(false)) + withOutcome(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("does not reactivate while the accepted reply has no terminal outcome yet", async () => { + const { rerender } = render(form(true)) + await submitAccepted() + + rerender(form(false)) + rerender(form(true)) + + // No outcome means the command may still be in flight: the form stays + // collapsed instead of inviting a duplicate. + expect(screen.queryByRole("textbox")).toBeNull() + expect(submitButton()).toBeNull() + }) + + it("is not reopened by a late terminal event after a turn is established", async () => { + const { rerender } = render(form(true)) + const commandId = await submitAccepted() + + 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. + withOutcome(commandId, true) + rerender(form(false)) + + expect(screen.queryByRole("textbox")).toBeNull() + expect(submitButton()).toBeNull() + }) + + 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() + + withOutcome(commandId, false) + render(form(true)) + + const alert = await screen.findByRole("alert") + expect(alert).toHaveTextContent("chatPage.clarification.replyOutcomeUnknown") + expect(submitButton()).toBeDisabled() + }) + + 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. + await waitFor(() => { + expect(appContextMock.dispatch).toHaveBeenCalledWith({ + type: "RECORD_CLARIFICATION_SUBMISSION", + payload: { + requestId: "inputreq_r1", + commandId: expect.any(String), + }, + }) + }) + // The existing advisory behavior is unchanged until an outcome arrives. + expect(submitButton()).toBeEnabled() + }) + + it("still reactivates a form that never submitted, ignoring unrelated outcomes", async () => { + appContextMock.state = { + clarificationSubmissions: {}, + commandOutcomes: { + "someone-elses-command": { + outcome: "failed", + resendSafe: false, + messageCode: "task_command_failed", + }, + }, + } + 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() + + rerender(form(false)) + withOutcome(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" }), + ) + }) + + it("uses outcome notice keys that resolve in both locale trees", () => { + for (const key of [ + "chatPage.clarification.replyNotApplied", + "chatPage.clarification.replyOutcomeUnknown", + ] as const) { + expect(resolveTranslation("en", key)).not.toBe(key) + expect(resolveTranslation("zh", key)).not.toBe(key) + } + }) +}) diff --git a/frontend/src/components/chat/clarification-form.tsx b/frontend/src/components/chat/clarification-form.tsx index 6e2def03fc..62da56374a 100644 --- a/frontend/src/components/chat/clarification-form.tsx +++ b/frontend/src/components/chat/clarification-form.tsx @@ -1,5 +1,6 @@ import React, { useEffect, useLayoutEffect, useMemo, useRef, useState } from "react" -import { Interaction } from "@/contexts/app-context-chat" +import { 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" @@ -133,11 +134,15 @@ 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 +189,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" | null + >(null) useLayoutEffect(() => { latestRequestIdRef.current = requestId @@ -194,18 +204,60 @@ 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 command this round is still accountable for, and its terminal + // disposition, both read from context so the gate survives this component + // instance being replaced (virtual waiting message vs. persisted timeline + // message render the same round). Derived per render: the effect below + // must re-run when either changes, and must NOT re-run when an unrelated + // command's outcome lands - that rerun used to wipe a visible failure + // alert. + const outstandingSubmission = requestId + ? clarificationSubmissions?.[requestId] + : undefined + const outstandingOutcome = outstandingSubmission + ? commandOutcomes?.[outstandingSubmission.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 a terminal + // outcome for that exact command that proves non-application. + if (outstandingSubmission) { + if (!outstandingOutcome || outstandingOutcome.resendSafe !== true) { + // Committed, still in flight, or unknown: surface the ambiguity + // (when a terminal outcome exists) without inviting a duplicate. + // The chat input remains available for a deliberate fresh message. + if (outstandingOutcome) { + setOutcomeNotice("unconfirmed") + setIsSubmitted(true) + setIsOpen(true) + } + return + } + // Proven not applied: consume the record so the resend is armed once, + // then reactivate below with the draft intact. + dispatch?.({ + type: "CLEAR_CLARIFICATION_SUBMISSION", + payload: { requestId }, + }) + 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, outstandingSubmission, outstandingOutcome, requestId, dispatch]) const normalizedInteractions = useMemo(() => { const seenFields = new Set() @@ -377,9 +429,26 @@ 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() + const recordSubmission = () => { + if (onSend || !dispatch) return + if (typeof submittedRequestId !== "string" || !submittedRequestId) return + dispatch({ + type: "RECORD_CLARIFICATION_SUBMISSION", + payload: { requestId: submittedRequestId, commandId: clientMessageId }, + }) + } + 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 +456,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() } if (latestRequestIdRef.current !== submittedRequestId) return @@ -397,6 +471,12 @@ 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. + recordSubmission() + } 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 +793,16 @@ export function ClarificationForm({ ))} + {outcomeNotice === "unconfirmed" && ( +
+ {t("chatPage.clarification.replyOutcomeUnknown")} +
+ )} + {outcomeNotice === "notApplied" && ( +
+ {t("chatPage.clarification.replyNotApplied")} +
+ )} {sendFailure && (
{sendFailureMessage}
diff --git a/frontend/src/contexts/app-context-chat.test.tsx b/frontend/src/contexts/app-context-chat.test.tsx index be2da9542a..27b18e214e 100644 --- a/frontend/src/contexts/app-context-chat.test.tsx +++ b/frontend/src/contexts/app-context-chat.test.tsx @@ -214,6 +214,7 @@ function StateProbe() {
{String(state.isHistoryLoading)}
{String(state.filePreview.isOpen)}
{String(state.isProcessing)}
+
{JSON.stringify(state.commandOutcomes)}
) } @@ -7157,3 +7158,237 @@ describe("clarification round identity (#1500)", () => { expect(screen.getByTestId("task-status").textContent).toBe("waiting_for_user") }) }) + +function ClarificationSubmissionProbe() { + const { state, dispatch } = useApp() + return ( + <> +
+ {JSON.stringify(state.clarificationSubmissions)} +
+
)} + {outcomeNotice === "pending" && ( +
+ {t("chatPage.clarification.replyPending")} +
+ )} {outcomeNotice === "notApplied" && (
{t("chatPage.clarification.replyNotApplied")} diff --git a/frontend/src/contexts/app-context-chat.test.tsx b/frontend/src/contexts/app-context-chat.test.tsx index 1b2b68d8e5..1df52dcb68 100644 --- a/frontend/src/contexts/app-context-chat.test.tsx +++ b/frontend/src/contexts/app-context-chat.test.tsx @@ -7175,6 +7175,15 @@ function ClarificationSubmissionProbe() { }) } /> +