mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-26 17:38:36 +00:00
fix(desktop): keep clarify answerable across reconnect/hydration + tool-progress off (#69795)
* fix(desktop): keep clarify lifecycle when tool progress is off * fix(desktop): render clarify prompt from the request event Re-authored onto the current use-message-stream/gateway-event.ts (the original patched the pre-split use-message-stream.ts). When the tool.start row that normally mounts the inline clarify UI is missed (stream reconnect / hydration race), upsert a stable pending clarify tool row from clarify.request itself so the prompt stays answerable; a real tool.start/complete with the same request id merges rather than duplicates. Co-authored-by: 정수환 <centerid@naver.com> * chore(contributors): map centerid@naver.com -> lidises Attribution mapping for the salvaged #47544 commit. * fix(desktop): correlate clarify rows by question so hydration can't duplicate The hydrated row (from clarify.request's request_id) and the real tool.start row (the model's tool_call_id) have different ids, so id-only matching appended a second clarify card in the normal path (caught by the BLOCKING_CLARIFY e2e: 'question' resolved to 2 elements). Add 'question' to the tool match-value keys so a clarify upsert merges into the existing pending clarify row regardless of id (same request<->args correlation ClarifyToolPending already uses); when no row exists yet (reconnect/hydration) it still creates one. --------- Co-authored-by: 정수환 <centerid@naver.com>
This commit is contained in:
parent
7d28e84e72
commit
d21165c2f0
6 changed files with 191 additions and 10 deletions
|
|
@ -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)
|
||||
})
|
||||
})
|
||||
|
|
@ -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 }))
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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<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() : ''
|
||||
|
||||
|
|
|
|||
2
contributors/emails/centerid@naver.com
Normal file
2
contributors/emails/centerid@naver.com
Normal file
|
|
@ -0,0 +1,2 @@
|
|||
lidises
|
||||
# PR salvage of #47544
|
||||
|
|
@ -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([])
|
||||
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue