From 4cf09716937d4bbfbb1dcd9d016522e328485712 Mon Sep 17 00:00:00 2001 From: ciregenz Date: Mon, 10 Aug 2026 14:45:11 -0700 Subject: [PATCH] [eric] tool-ui: asks pair by tool_use_id, early clicks held, stops release waits, dead asks go quiet --- backend/apps/agents/agents.py | 3 + .../manager/streaming/post_tool_hook.py | 3 + backend/apps/agents/show_ui_mcp_server.py | 13 +++- backend/apps/agents/ui_request_bridge.py | 65 ++++++++++++++++--- backend/main.py | 4 +- backend/tests/test_ui_request_bridge.py | 64 ++++++++++++++++++ .../src/app/pages/AgentChat/AgentChat.tsx | 33 ++++++++-- .../pages/AgentChat/tool-ui/AskUiBubble.tsx | 22 +++++-- .../pages/AgentChat/tool-ui/showUiPayload.ts | 32 ++++++++- 9 files changed, 213 insertions(+), 26 deletions(-) create mode 100644 backend/tests/test_ui_request_bridge.py diff --git a/backend/apps/agents/agents.py b/backend/apps/agents/agents.py index ed653edb..03717b6c 100644 --- a/backend/apps/agents/agents.py +++ b/backend/apps/agents/agents.py @@ -194,6 +194,9 @@ async def send_message(session_id: str, body: dict): @agents.router.post("/sessions/{session_id}/stop") async def stop_agent(session_id: str): await agent_manager.stop_agent(session_id) + # A stopped turn's parked AskUI waits would otherwise zombie for 600s and eat the next click (ENG-232). + from backend.apps.agents.ui_request_bridge import cancel_session_waits + cancel_session_waits(session_id) return {"ok": True} @agents.router.post("/approval") diff --git a/backend/apps/agents/manager/streaming/post_tool_hook.py b/backend/apps/agents/manager/streaming/post_tool_hook.py index bfadff6f..01bdae83 100644 --- a/backend/apps/agents/manager/streaming/post_tool_hook.py +++ b/backend/apps/agents/manager/streaming/post_tool_hook.py @@ -136,6 +136,9 @@ async def post_tool_hook(ctx: HookContext, input_data: dict, tool_use_id, contex content = f"{content}\n\n" + wrap_platform_note("\n\n".join(notes)) result_payload = {"text": content} + # Pairing by array index mispairs any parallel batch (first-completing result lands on the first call); the provider id lets the frontend pair by identity (ENG-232). + if tool_use_id: + result_payload["tool_use_id"] = str(tool_use_id) hook_tool_name = input_data.get("tool_name", "") if hook_tool_name: result_payload["tool_name"] = hook_tool_name diff --git a/backend/apps/agents/show_ui_mcp_server.py b/backend/apps/agents/show_ui_mcp_server.py index f6aa55fc..5d49d8b8 100644 --- a/backend/apps/agents/show_ui_mcp_server.py +++ b/backend/apps/agents/show_ui_mcp_server.py @@ -44,6 +44,7 @@ COMPONENT_SPECS.update({name: entry["hint"] for name, entry in GENERATED.items() INTERACTIVE_COMPONENTS = ( "option-list", "question-flow", "parameter-slider", "preferences-panel", "approval-card", + "message-draft", ) TOOLS = [ @@ -250,8 +251,9 @@ def handle_ask_ui(arguments: dict) -> dict: return {"content": [{"type": "text", "text": "props must be an object."}], "isError": True} if component not in INTERACTIVE_COMPONENTS: return {"content": [{"type": "text", "text": f"AskUI only supports: {', '.join(INTERACTIVE_COMPONENTS)}. Use ShowUI for display-only components."}], "isError": True} - component_id = str(props.get("id", "")).strip() - if not component_id: + # Registered RAW, not stripped: the frontend responds with the untrimmed id, and a mismatched key means the answer never lands. + component_id = str(props.get("id", "")) + if not component_id.strip(): return {"content": [{"type": "text", "text": "props.id (a stable string) is required so the answer can be correlated."}], "isError": True} problem = validate(component, props) if problem: @@ -273,6 +275,13 @@ def handle_tool_call(tool_name: str, arguments: dict) -> dict: props = arguments.get("props") if not isinstance(props, dict): return {"content": [{"type": "text", "text": "props must be an object."}], "isError": True} + # A display-only render wires no click handlers, so an interactive component via ShowUI draws dead buttons (message-draft even animates a send that sent nothing); teach instead of rendering a lie. + if component in INTERACTIVE_COMPONENTS: + return {"content": [{"type": "text", "text": ( + f"Not rendered: '{component}' is interactive and ShowUI is display-only, so its buttons " + "would be dead. Call AskUI with the same component and props (props.id required) to " + "collect the user's answer." + )}], "isError": True} problem = validate(component, props) if problem: return {"content": [{"type": "text", "text": f"Not rendered: {problem}"}], "isError": True} diff --git a/backend/apps/agents/ui_request_bridge.py b/backend/apps/agents/ui_request_bridge.py index faa78b08..1dcd91c3 100644 --- a/backend/apps/agents/ui_request_bridge.py +++ b/backend/apps/agents/ui_request_bridge.py @@ -3,13 +3,19 @@ answers in the transcript (or the wait times out). Keyed by (session_id, compone so the frontend can respond without ever learning a server-side request id.""" import asyncio -from typing import Any, Dict, Optional, Tuple +import time +from typing import Any, Dict, Literal, Optional, Tuple from pydantic import BaseModel, ConfigDict, InstanceOf from typeguard import typechecked MAX_PENDING = 50 MAX_WAIT_SECONDS = 600.0 +# The card is clickable the moment its tool_call broadcasts, but the wait only parks after the CLI hook round-trip, stdio dispatch and an HTTP hop; an instant click landing in that gap must not be dropped (ENG-232 D4). +EARLY_ANSWER_TTL_SECONDS = 45.0 +MAX_EARLY = 50 + +RespondOutcome = Literal["delivered", "buffered", "gone"] class PendingUiRequest(BaseModel): @@ -18,15 +24,32 @@ class PendingUiRequest(BaseModel): response: Optional[Dict[str, Any]] = None +class EarlyAnswer(BaseModel): + model_config = ConfigDict(validate_assignment=True) + stamp: float + response: Dict[str, Any] + + p_pending: Dict[Tuple[str, str], PendingUiRequest] = {} +p_early: Dict[Tuple[str, str], EarlyAnswer] = {} + + +@typechecked +def p_prune_early(now: float) -> None: + for key in [k for k, v in p_early.items() if now - v.stamp > EARLY_ANSWER_TTL_SECONDS]: + p_early.pop(key, None) @typechecked async def wait_for_ui_response(session_id: str, component_id: str, timeout_s: float) -> Optional[Dict[str, Any]]: """Registers the request and blocks until respond_to_ui_request fires it; None on timeout.""" + key = (session_id, component_id) + p_prune_early(time.monotonic()) + early = p_early.pop(key, None) + if early is not None: + return early.response if len(p_pending) >= MAX_PENDING: raise ValueError("too many pending UI requests") - key = (session_id, component_id) # A retried tool call for the same component replaces the stale wait; the old waiter times out. pending = PendingUiRequest(event=asyncio.Event()) p_pending[key] = pending @@ -41,11 +64,35 @@ async def wait_for_ui_response(session_id: str, component_id: str, timeout_s: fl @typechecked -def respond_to_ui_request(session_id: str, component_id: str, response: Dict[str, Any]) -> bool: - """Delivers the user's answer to the parked wait; False when nothing is waiting.""" +def respond_to_ui_request(session_id: str, component_id: str, response: Dict[str, Any]) -> RespondOutcome: + """Delivers the user's answer to the parked wait, or holds it briefly for a wait still en route.""" pending = p_pending.get((session_id, component_id)) - if pending is None: - return False - pending.response = response - pending.event.set() - return True + if pending is not None: + pending.response = response + pending.event.set() + return "delivered" + now = time.monotonic() + p_prune_early(now) + if len(p_early) >= MAX_EARLY: + return "gone" + p_early[(session_id, component_id)] = EarlyAnswer(stamp=now, response=response) + return "buffered" + + +@typechecked +def reset_ui_bridge() -> None: + p_pending.clear() + p_early.clear() + + +@typechecked +def cancel_session_waits(session_id: str) -> int: + """Releases every parked wait for a stopped session so its cards can't eat later clicks (ENG-232 D5).""" + released = 0 + for key, pending in list(p_pending.items()): + if key[0] == session_id: + pending.event.set() + released += 1 + for key in [k for k in p_early if k[0] == session_id]: + p_early.pop(key, None) + return released diff --git a/backend/main.py b/backend/main.py index 785c227d..b7d16295 100644 --- a/backend/main.py +++ b/backend/main.py @@ -939,10 +939,10 @@ async def ui_request_respond(request: Request): if not session_id or not component_id or not isinstance(response, dict): return JSONResponse({"error": "session_id, component_id and response object are required"}, status_code=400) from backend.apps.agents.ui_request_bridge import respond_to_ui_request - delivered = respond_to_ui_request(session_id, component_id, response) + outcome = respond_to_ui_request(session_id, component_id, response) # A consumed/expired request is a normal outcome (replayed transcript, agent moved on), not an # error; 200 + gone keeps Chromium's console free of red 404 noise while the UI shows its orphaned state. - return JSONResponse({"ok": delivered, "gone": not delivered}) + return JSONResponse({"ok": outcome != "gone", "buffered": outcome == "buffered", "gone": outcome == "gone"}) @app.post("/api/invoke-agent/run") diff --git a/backend/tests/test_ui_request_bridge.py b/backend/tests/test_ui_request_bridge.py new file mode 100644 index 00000000..527e1076 --- /dev/null +++ b/backend/tests/test_ui_request_bridge.py @@ -0,0 +1,64 @@ +"""The AskUI bridge: early clicks are held, stops release zombies, answers land (ENG-232).""" + +import asyncio + +import pytest + +from backend.apps.agents.ui_request_bridge import ( + cancel_session_waits, + reset_ui_bridge, + respond_to_ui_request, + wait_for_ui_response, +) + + +@pytest.fixture(autouse=True) +def fresh_bridge(): + reset_ui_bridge() + yield + reset_ui_bridge() + + +@pytest.mark.asyncio +async def test_answer_reaches_a_parked_wait(): + task = asyncio.ensure_future(wait_for_ui_response("s1", "q1", 5.0)) + await asyncio.sleep(0.05) + assert respond_to_ui_request("s1", "q1", {"action": "select", "value": "a"}) == "delivered" + assert await task == {"action": "select", "value": "a"} + + +@pytest.mark.asyncio +async def test_click_before_the_wait_parks_is_held_not_dropped(): + """The card is clickable before the wait registers; that click used to be discarded.""" + assert respond_to_ui_request("s1", "q1", {"action": "confirm"}) == "buffered" + assert await wait_for_ui_response("s1", "q1", 5.0) == {"action": "confirm"} + # Consumed exactly once: the next wait for the same id parks and times out instead of replaying it. + assert await wait_for_ui_response("s1", "q1", 0.1) is None + + +@pytest.mark.asyncio +async def test_stop_releases_parked_waits_so_they_cannot_eat_later_clicks(): + task = asyncio.ensure_future(wait_for_ui_response("s1", "q1", 30.0)) + other = asyncio.ensure_future(wait_for_ui_response("s2", "q1", 30.0)) + await asyncio.sleep(0.05) + assert cancel_session_waits("s1") == 1 + assert await task is None + # The other session's wait is untouched and still answerable. + assert respond_to_ui_request("s2", "q1", {"action": "x"}) == "delivered" + assert await other == {"action": "x"} + + +@pytest.mark.asyncio +async def test_stop_also_drops_buffered_answers(): + assert respond_to_ui_request("s1", "q1", {"action": "confirm"}) == "buffered" + cancel_session_waits("s1") + assert await wait_for_ui_response("s1", "q1", 0.1) is None + + +@pytest.mark.asyncio +async def test_a_stale_buffered_answer_expires(monkeypatch): + import backend.apps.agents.ui_request_bridge as B + monkeypatch.setattr(B, "EARLY_ANSWER_TTL_SECONDS", 0.0) + assert respond_to_ui_request("s1", "q1", {"action": "confirm"}) == "buffered" + # TTL zero: the next wait prunes it and parks instead of replaying a dead click. + assert await wait_for_ui_response("s1", "q1", 0.2) is None diff --git a/frontend/src/app/pages/AgentChat/AgentChat.tsx b/frontend/src/app/pages/AgentChat/AgentChat.tsx index d57a9b82..ec2a347a 100644 --- a/frontend/src/app/pages/AgentChat/AgentChat.tsx +++ b/frontend/src/app/pages/AgentChat/AgentChat.tsx @@ -65,7 +65,7 @@ import ToolCallBubble, { ToolPair } from './tool-bubbles/ToolCallBubble'; import ToolGroupBubble, { RenderItem, ToolGroup, ToolGroupEntry, isToolGroup, isToolPair } from './tool-bubbles/ToolGroupBubble'; import ToolUiBubble from './tool-ui/ToolUiBubble'; import AskUiBubble from './tool-ui/AskUiBubble'; -import { isShowUiPair, isAskUiPair, extractPendingAskUi } from './tool-ui/showUiPayload'; +import { isShowUiPair, isAskUiPair, extractPendingAskUi, callToolUseId, resultToolUseId, isDeadAskResult } from './tool-ui/showUiPayload'; import { composerPlaceholder } from './composerPlaceholder'; import ApprovalBar, { BatchApprovalBar } from './shell/ApprovalBar'; import ForceStopAgentBar from './ForceStopAgentBar'; @@ -1190,12 +1190,22 @@ const AgentChat: React.FC = ({ sessionId: sessionIdProp, onClose const allCalls = group.filter((m) => m.role === 'tool_call'); const results = group.filter((m) => m.role === 'tool_result'); - const allPairs: ToolPair[] = allCalls.map((call, idx) => ({ - type: 'tool_pair' as const, - id: `pair-${call.id}`, - call, - result: results[idx] || null, - })); + // Index pairing mispairs any parallel batch (first-completing result lands on the first call, killing a live AskUI card, ENG-232); pair by tool_use_id when the result carries one, index only for legacy unkeyed results. + const resultById = new Map(); + for (const r of results) { + const rid = resultToolUseId(r); + if (rid && !resultById.has(rid)) resultById.set(rid, r); + } + const allPairs: ToolPair[] = allCalls.map((call, idx) => { + const byId = resultById.get(callToolUseId(call)); + const indexed = results[idx] || null; + return { + type: 'tool_pair' as const, + id: `pair-${call.id}`, + call, + result: byId ?? (indexed && !resultToolUseId(indexed) ? indexed : null), + }; + }); // ShowUI/AskUI calls render as inline components, never buried inside a collapsed group. // They typically cap a run of work, so the quiet group row stays above the widget. @@ -1803,6 +1813,15 @@ const AgentChat: React.FC = ({ sessionId: sessionIdProp, onClose if (isToolPair(item)) { const isPending = item.result === null && sessionRunning; if (isAskUiPair(item)) { + // A dead ask (timeout prose or a validation bounce) is not answerable; rendering it full-size is the "two identical cards, only one works" dupe (ENG-232). + if (isDeadAskResult(item)) { + return ( + + + {compactionChip} + + ); + } // Only the LATEST unanswered question is live (the backend parks one component); an // older pending ask renders as a quiet row instead of a second clickable form. if (item.result === null && lastPendingAskCallId !== null && item.call.id !== lastPendingAskCallId) { diff --git a/frontend/src/app/pages/AgentChat/tool-ui/AskUiBubble.tsx b/frontend/src/app/pages/AgentChat/tool-ui/AskUiBubble.tsx index 1c51f56a..5e83e179 100644 --- a/frontend/src/app/pages/AgentChat/tool-ui/AskUiBubble.tsx +++ b/frontend/src/app/pages/AgentChat/tool-ui/AskUiBubble.tsx @@ -70,12 +70,14 @@ function AskUiBubble({ pair, sessionId, isPending, suppressReveal }: AskUiBubble setOrphaned(true); } }) - .catch(() => { setSubmitted(false); setLocalChoice(undefined); }); + .catch(() => { setSubmitted(false); setLocalChoice(undefined); setOrphaned(true); }); }, [submitted, sessionId, componentId], ); const waiting = pair.result === null && !submitted; + // A result that isn't the JSON answer envelope (timeout prose, validation bounce) means this ask is dead; it must not look answerable (ENG-232). + const expired = pair.result !== null && answered === null; // Their embedded-actions contract: onAction(actionId, state) delivers the component's full state, // and the components ship their own footer actions (Clear/Confirm), so we only wire the callback. @@ -91,12 +93,17 @@ function AskUiBubble({ pair, sessionId, isPending, suppressReveal }: AskUiBubble : { choice: (answered?.choice as string) ?? (localChoice as string | undefined) }; } if (waiting) { - return { + const base = { onAction: (actionId: string, state: unknown) => { if (actionId === 'cancel') return; respond({ action: actionId, value: state ?? null }); }, }; + // message-draft's send flow fires onSend (after its undo grace), not onAction; without this the send animation completes while nothing reaches the agent. + if (payload.name === 'message-draft') { + return { ...base, onSend: () => respond({ action: 'send', value: null }) }; + } + return base; } // A free-text answer isn't an option id; passing it as `choice` would fail their contract. if (freeTextAnswer !== null) return {}; @@ -139,7 +146,14 @@ function AskUiBubble({ pair, sessionId, isPending, suppressReveal }: AskUiBubble )} )} - + + + + {expired && ( + + This question expired before it was answered; if the agent asked again, use the newer card. + + )} {waiting && FREE_TEXT_COMPONENTS.has(payload.name) && ( - No agent is waiting for this answer (the request expired or this is an old transcript). + This answer didn't reach the agent (it may have stopped, expired, or the connection dropped). Try again. )} diff --git a/frontend/src/app/pages/AgentChat/tool-ui/showUiPayload.ts b/frontend/src/app/pages/AgentChat/tool-ui/showUiPayload.ts index 4dde7d68..3f9c0f5f 100644 --- a/frontend/src/app/pages/AgentChat/tool-ui/showUiPayload.ts +++ b/frontend/src/app/pages/AgentChat/tool-ui/showUiPayload.ts @@ -81,6 +81,26 @@ export function isAskUiPair(pair: ToolPair): boolean { return /(^|__)AskUI$/.test(tool); } +/** The provider tool_use id a call carries in its content; '' when absent. */ +export function callToolUseId(msg: { content: unknown }): string { + const c = (typeof msg.content === 'object' && msg.content !== null ? msg.content : {}) as { id?: unknown }; + return typeof c.id === 'string' ? c.id : ''; +} + +/** The provider tool_use id a result says it answers; '' on legacy results that carry none. */ +export function resultToolUseId(msg: { content: unknown }): string { + const c = (typeof msg.content === 'object' && msg.content !== null ? msg.content : {}) as { tool_use_id?: unknown }; + return typeof c.tool_use_id === 'string' ? c.tool_use_id : ''; +} + +/** A result that is not the JSON answer envelope (timeout prose, validation bounce, AskUI failure): the ask is dead, not answered, and must not render as a clickable form. */ +export function isDeadAskResult(pair: ToolPair): boolean { + if (pair.result === null) return false; + const rc = pair.result.content as unknown; + const text = typeof rc === 'string' ? rc : (typeof rc === 'object' && rc !== null ? String((rc as { text?: unknown }).text ?? '') : ''); + return !text.trim().startsWith('{'); +} + /** Latest ShowUI payload anywhere in a transcript; the collapsed card pins this artifact under its pill. */ /** The newest UNANSWERED AskUI call, so a collapsed card can surface the live question under its pill (a blocking question beats every other artifact; the agent is literally waiting on it). */ @@ -90,8 +110,16 @@ export function extractPendingAskUi(messages: Array<{ id: string; role: string; if (msg.role !== 'tool_call') continue; const body = (typeof msg.content === 'object' && msg.content !== null ? msg.content : {}) as { tool?: unknown }; if (!/(^|__)AskUI$/.test(String(body.tool || ''))) continue; - const next = messages[i + 1]; - if (next && next.role === 'tool_result') return null; + // Parallel batches break next-message adjacency, so match the answering result by tool_use_id when results carry one; adjacency stays the legacy fallback. + const callId = callToolUseId(msg); + let answered = false; + for (let j = i + 1; j < messages.length && !answered; j++) { + const m = messages[j]; + if (m.role !== 'tool_result') continue; + const rid = resultToolUseId(m); + answered = rid ? rid === callId : j === i + 1; + } + if (answered) return null; return { type: 'tool_pair', id: msg.id, call: msg as ToolPair['call'], result: null }; } return null;