diff --git a/apps/desktop/src/app/session/hooks/use-message-stream/clarify-hydration.test.tsx b/apps/desktop/src/app/session/hooks/use-message-stream/clarify-hydration.test.tsx new file mode 100644 index 000000000000..6af488231fe9 --- /dev/null +++ b/apps/desktop/src/app/session/hooks/use-message-stream/clarify-hydration.test.tsx @@ -0,0 +1,116 @@ +import { QueryClient } from '@tanstack/react-query' +import { act, cleanup, render, waitFor } from '@testing-library/react' +import { type MutableRefObject, useEffect, useRef } from 'react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +import type { ClientSessionState } from '@/app/types' +import { createClientSessionState } from '@/lib/chat-runtime' +import { clearClarifyRequest } from '@/store/clarify' +import type { RpcEvent } from '@/types/hermes' + +import { useMessageStream } from './index' + +// A `clarify.request` must leave an answerable inline row even when the +// `tool.start` that normally mounts it was missed (stream reconnect / +// hydration race). Without it the sidebar says "needs input" but the +// transcript has nowhere to render the choices, so the agent blocks forever. + +const SID = 'session-1' + +let handleEvent: ((event: RpcEvent) => void) | null = null +let stateRef: MutableRefObject> | null = null + +function Harness() { + const activeSessionIdRef = useRef(SID) + const sessionStateByRuntimeIdRef = useRef(new Map()) + const queryClientRef = useRef(new QueryClient()) + + const stream = useMessageStream({ + activeSessionIdRef, + hydrateFromStoredSession: vi.fn(async () => undefined), + queryClient: queryClientRef.current, + refreshHermesConfig: vi.fn(async () => undefined), + refreshSessions: vi.fn(async () => undefined), + sessionStateByRuntimeIdRef, + updateSessionState: (sessionId, updater) => { + const current = sessionStateByRuntimeIdRef.current.get(sessionId) ?? createClientSessionState() + const next = updater(current) + sessionStateByRuntimeIdRef.current.set(sessionId, next) + + return next + } + }) + + useEffect(() => { + handleEvent = stream.handleGatewayEvent + stateRef = sessionStateByRuntimeIdRef + }, [stream.handleGatewayEvent]) + + return null +} + +async function mountStream() { + render() + await waitFor(() => expect(handleEvent).not.toBeNull()) +} + +const clarifyRequest = (payload: Record) => + act(() => handleEvent!({ payload, session_id: SID, type: 'clarify.request' })) + +const toolStart = (payload: Record) => + act(() => handleEvent!({ payload, session_id: SID, type: 'tool.start' })) + +function clarifyParts() { + const messages = stateRef?.current.get(SID)?.messages ?? [] + + return messages.flatMap(m => m.parts).filter(p => p.type === 'tool-call' && p.toolName === 'clarify') +} + +describe('clarify.request stream hydration', () => { + beforeEach(() => { + handleEvent = null + stateRef = null + clearClarifyRequest() + }) + + afterEach(() => { + cleanup() + clearClarifyRequest() + vi.restoreAllMocks() + }) + + it('mounts an answerable clarify row when the tool.start row was missed', async () => { + await mountStream() + + clarifyRequest({ choices: ['yes', 'no'], question: 'Ship it?', request_id: 'req-1' }) + + const parts = clarifyParts() + expect(parts).toHaveLength(1) + expect(parts[0].type === 'tool-call' && parts[0].toolCallId).toBe('req-1') + expect(parts[0].type === 'tool-call' && parts[0].args).toMatchObject({ + choices: ['yes', 'no'], + question: 'Ship it?' + }) + }) + + it('merges with the real tool.start row even though its id differs from the request id', async () => { + await mountStream() + + // Reality: tool.start carries the model's tool_call_id, clarify.request a + // separately-generated request_id. They must still collapse to ONE card + // (correlated by question), not two. + toolStart({ args: { choices: ['a'], question: 'Pick' }, name: 'clarify', tool_id: 'call-abc' }) + clarifyRequest({ choices: ['a'], question: 'Pick', request_id: 'req-2' }) + + expect(clarifyParts()).toHaveLength(1) + }) + + it('does not duplicate when clarify.request arrives before the tool.start row', async () => { + await mountStream() + + clarifyRequest({ choices: ['a'], question: 'Pick', request_id: 'req-3' }) + toolStart({ args: { choices: ['a'], question: 'Pick' }, name: 'clarify', tool_id: 'call-xyz' }) + + expect(clarifyParts()).toHaveLength(1) + }) +}) diff --git a/apps/desktop/src/app/session/hooks/use-message-stream/gateway-event.ts b/apps/desktop/src/app/session/hooks/use-message-stream/gateway-event.ts index 950cccf60a7b..801678c26644 100644 --- a/apps/desktop/src/app/session/hooks/use-message-stream/gateway-event.ts +++ b/apps/desktop/src/app/session/hooks/use-message-stream/gateway-event.ts @@ -679,19 +679,30 @@ export function useGatewayEventHandler(deps: GatewayEventDeps) { const question = typeof payload?.question === 'string' ? payload.question : '' if (requestId && question) { + const choices = Array.isArray(payload?.choices) ? payload!.choices!.filter(c => typeof c === 'string') : null + setClarifyRequest({ requestId, question, - choices: Array.isArray(payload?.choices) ? payload!.choices!.filter(c => typeof c === 'string') : null, + choices, sessionId: sessionId ?? null }) - // The transcript only renders the active session, so a background - // clarify is otherwise invisible (the row just keeps spinning like - // it's working). Flag the session so the sidebar shows a persistent - // "needs input" indicator on its row — works for the active session - // too, and survives alt-tab / window blur (unlike a toast). if (sessionId) { + // `clarify.request` is the blocking event the Python side waits on, + // while the inline UI normally mounts from the earlier `tool.start` + // row. If that row was missed (stream reconnect / hydration race) the + // sidebar still says "needs input" but there is nowhere to render the + // choices. Upsert a stable pending clarify tool row from the request + // itself so the prompt stays answerable; a real tool.start/complete + // with the same request id merges rather than duplicates. + upsertToolCall(sessionId, { args: { choices, question }, name: 'clarify', tool_id: requestId }, 'running') + + // The transcript only renders the active session, so a background + // clarify is otherwise invisible (the row just keeps spinning like + // it's working). Flag the session so the sidebar shows a persistent + // "needs input" indicator on its row — works for the active session + // too, and survives alt-tab / window blur (unlike a toast). updateSessionState(sessionId, state => ({ ...state, needsInput: true })) } diff --git a/apps/desktop/src/lib/chat-messages.ts b/apps/desktop/src/lib/chat-messages.ts index 828021b21eaa..06ef904f2dda 100644 --- a/apps/desktop/src/lib/chat-messages.ts +++ b/apps/desktop/src/lib/chat-messages.ts @@ -356,7 +356,11 @@ function collectToolMatchValues(query: string, context: string, preview: string) function toolPayloadMatchValues(payload: GatewayEventPayload | undefined): string[] { const payloadArgs = liveToolArgs(payload) - const query = firstStringField(payloadArgs, ['search_term', 'query']) + // `question` is clarify's identifying arg: a synthetic row hydrated from + // `clarify.request` (a fresh request id) must correlate with the `tool.start` + // row (the model's tool_call_id) so the two ids don't produce a duplicate + // clarify card — same correlation ClarifyToolPending uses for request↔args. + const query = firstStringField(payloadArgs, ['search_term', 'query', 'question']) const context = typeof payload?.context === 'string' ? payload.context.trim() : '' const preview = typeof payload?.preview === 'string' ? payload.preview.trim() : '' @@ -369,7 +373,7 @@ function toolPartMatchValues(part: ChatMessagePart): string[] { } const args = part.args as Record - const query = firstStringField(args, ['search_term', 'query']) + const query = firstStringField(args, ['search_term', 'query', 'question']) const context = typeof args.context === 'string' ? args.context.trim() : '' const preview = typeof args.preview === 'string' ? args.preview.trim() : '' diff --git a/contributors/emails/centerid@naver.com b/contributors/emails/centerid@naver.com new file mode 100644 index 000000000000..37ddc7854012 --- /dev/null +++ b/contributors/emails/centerid@naver.com @@ -0,0 +1,2 @@ +lidises +# PR salvage of #47544 diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index eb295799270d..161bdd671652 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -865,6 +865,46 @@ def test_tui_tool_output_risk_event_exposes_metadata_without_raw_output(monkeypa assert "result" not in events[0][2] +def test_tui_clarify_lifecycle_events_emit_when_tool_progress_off(monkeypatch): + events: list[tuple[str, str, dict]] = [] + monkeypatch.setattr( + server, "_emit", lambda event_type, sid, payload: events.append((event_type, sid, payload)) + ) + monkeypatch.setitem( + server._sessions, + "clarify-off-test", + {"tool_progress_mode": "off", "tool_started_at": {}}, + ) + + args = {"question": "Pick one", "choices": ["A", "B"]} + result = '{"question":"Pick one","choices_offered":["A","B"],"user_response":"A"}' + + server._on_tool_start("clarify-off-test", "tool-clarify", "clarify", args) + server._on_tool_complete("clarify-off-test", "tool-clarify", "clarify", args, result) + + assert [event[0] for event in events] == ["tool.start", "tool.complete"] + assert events[0][2]["name"] == "clarify" + assert events[0][2]["tool_id"] == "tool-clarify" + assert events[1][2]["result"]["user_response"] == "A" + + +def test_tui_non_interactive_tool_lifecycle_stays_hidden_when_tool_progress_off(monkeypatch): + events: list[tuple[str, str, dict]] = [] + monkeypatch.setattr( + server, "_emit", lambda event_type, sid, payload: events.append((event_type, sid, payload)) + ) + monkeypatch.setitem( + server._sessions, + "terminal-off-test", + {"tool_progress_mode": "off", "tool_started_at": {}}, + ) + + server._on_tool_start("terminal-off-test", "tool-1", "terminal", {"command": "pwd"}) + server._on_tool_complete("terminal-off-test", "tool-1", "terminal", {"command": "pwd"}, "done") + + assert events == [] + + def test_dispatch_rejects_non_object_request(): resp = server.dispatch([]) diff --git a/tui_gateway/server.py b/tui_gateway/server.py index e1a415708022..0488a5753b2d 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -3299,6 +3299,14 @@ def _tool_progress_enabled(sid: str) -> bool: return _session_tool_progress_mode(sid) != "off" +def _tool_lifecycle_required_for_ui(name: str) -> bool: + """Return True for tool events that are interactive UI, not optional chrome.""" + # Desktop renders the clarify choices/question from the tool-call part, then + # wires request_id from clarify.request. If tool progress is off, suppressing + # clarify's lifecycle events leaves only the sidebar attention dot visible. + return name == "clarify" + + def _restart_slash_worker(sid: str, session: dict): worker = session.get("slash_worker") if worker: @@ -4236,7 +4244,7 @@ def _on_tool_start(sid: str, tool_call_id: str, name: str, args: dict): except Exception: pass session.setdefault("tool_started_at", {})[tool_call_id] = time.time() - if _tool_progress_enabled(sid): + if _tool_progress_enabled(sid) or _tool_lifecycle_required_for_ui(name): payload = { "tool_id": tool_call_id, "name": name, @@ -4294,7 +4302,7 @@ def _on_tool_complete(sid: str, tool_call_id: str, name: str, args: dict, result payload["inline_diff"] = "\n".join(rendered) except Exception: pass - if _tool_progress_enabled(sid) or payload.get("inline_diff"): + if _tool_progress_enabled(sid) or payload.get("inline_diff") or _tool_lifecycle_required_for_ui(name): _emit("tool.complete", sid, payload)