Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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<Map<string, ClientSessionState>> | null = null

function Harness() {
const activeSessionIdRef = useRef<string | null>(SID)
const sessionStateByRuntimeIdRef = useRef(new Map<string, ClientSessionState>())
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(<Harness />)
await waitFor(() => expect(handleEvent).not.toBeNull())
}

const clarifyRequest = (payload: Record<string, unknown>) =>
act(() => handleEvent!({ payload, session_id: SID, type: 'clarify.request' }))

const toolStart = (payload: Record<string, unknown>) =>
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)
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -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 }))
}

Expand Down
8 changes: 6 additions & 2 deletions apps/desktop/src/lib/chat-messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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() : ''

Expand All @@ -369,7 +373,7 @@ function toolPartMatchValues(part: ChatMessagePart): string[] {
}

const args = part.args as Record<string, unknown>
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() : ''

Expand Down
2 changes: 2 additions & 0 deletions contributors/emails/centerid@naver.com
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
lidises
# PR salvage of #47544
40 changes: 40 additions & 0 deletions tests/test_tui_gateway_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -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([])

Expand Down
12 changes: 10 additions & 2 deletions tui_gateway/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -3271,6 +3271,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:
Expand Down Expand Up @@ -4208,7 +4216,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,
Expand Down Expand Up @@ -4266,7 +4274,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)


Expand Down
Loading