From 6f99040e8bf6face67e39df912c095f561f99648 Mon Sep 17 00:00:00 2001 From: abose Date: Mon, 7 Sep 2026 21:17:36 +0530 Subject: [PATCH] fix(ai): an answered question is not a failed tool call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A question the user had just answered showed a red "failed" badge. The cause is the transport: AskUserQuestion is answered by DENYING the tool call and handing the user's choice back as the denial reason (the PreToolUse hook), so the CLI reports it as an error result — and the panel paints any error result red. Two invisible consequences rode along: histEntry.failed was set, so restored history showed the failure too, and every question ever asked was counted as "toolErr" in metrics. Fixed where node forwards the result rather than in the UI, so the badge, the history entry and the metric all agree. Node now keeps a tool_use id → tool name map beside the existing id → counter map and suppresses the error flag for AskUserQuestion, with a comment explaining that the deny is our transport and not a failure. Unanswered questions are covered too: cancelling or stopping the turn is not a failure either. Verified in the desktop app on all three answer paths — clicking an option, typing into the card's own free-text box (the reported case), and cancelling via Stop: isError false, no badge, and the answer reaches the model. Also registers the new "questions" suite with run_ai_test_suite and puts QF-1/QF-6 in the quick set so this cannot regress unnoticed. --- phoenix-builder-mcp/mcp-tools.js | 5 +++-- src-node/claude-code-agent.js | 19 ++++++++++++++++++- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/phoenix-builder-mcp/mcp-tools.js b/phoenix-builder-mcp/mcp-tools.js index 6a9ff65ec7..871ede1176 100644 --- a/phoenix-builder-mcp/mcp-tools.js +++ b/phoenix-builder-mcp/mcp-tools.js @@ -38,14 +38,15 @@ const AI_TEST_SUITES = { "unsaved-buffers": "suite-unsaved-buffers.md", "self-sufficiency": "suite-self-sufficiency.md", "bug-fixing": "suite-bug-fixing.md", + "questions": "suite-questions.md", "plan-mode": "suite-plan-mode.md", "permissions": "suite-permissions.md" }; // The four model runs that have caught every regression seen so far, plus the // free deterministic/piggyback checks. See model_tests.md, "Deterministic first". const AI_TEST_QUICK = { - suites: ["editor-context", "unsaved-buffers", "self-sufficiency", "bug-fixing"], - tests: ["UB-1", "EC-5", "EC-2", "SS-4", "EC-1", "UB-2", "SS-1", "BF-1"] + suites: ["editor-context", "unsaved-buffers", "self-sufficiency", "bug-fixing", "questions"], + tests: ["UB-1", "EC-5", "EC-2", "SS-4", "QF-6", "EC-1", "UB-2", "SS-1", "BF-1", "QF-1"] }; function _gitInfo(cwd) { diff --git a/src-node/claude-code-agent.js b/src-node/claude-code-agent.js index 82aea2933d..9c70c91b81 100644 --- a/src-node/claude-code-agent.js +++ b/src-node/claude-code-agent.js @@ -1115,6 +1115,9 @@ async function _runQuery(requestId, prompt, projectPath, model, signal, locale, // SDK tool_use id (e.g. "toolu_01...") → our sequential toolCounter so a // tool_result block can be mapped back to its indicator on the browser. const _toolUseIdToCounter = {}; + // tool_use id → SDK tool name, so a tool_result can be interpreted in the + // light of which tool produced it (see the AskUserQuestion note below). + const _toolUseIdToName = {}; // Set true once the user clicks "Allow & Switch to Edit Mode" on a // plan-mode write confirmation. Subsequent Edit/Write attempts in the same // turn skip the prompt and use the cached "allow" decision so a multi-edit @@ -2396,6 +2399,7 @@ async function _runQuery(requestId, prompt, projectPath, model, signal, locale, toolCounter++; if (block.id) { _toolUseIdToCounter[block.id] = toolCounter; + _toolUseIdToName[block.id] = block.name; } _log("Subagent tool:", block.name, "#" + toolCounter, "parent=#" + (parentToolId !== undefined ? parentToolId : "?")); @@ -2598,6 +2602,7 @@ async function _runQuery(requestId, prompt, projectPath, model, signal, locale, // correlate later tool_result blocks back to the indicator. if (event.content_block.id) { _toolUseIdToCounter[event.content_block.id] = activeToolCounter; + _toolUseIdToName[event.content_block.id] = activeToolName; } _log("Tool start:", activeToolName, "#" + activeToolCounter); nodeConnector.triggerPeer("aiProgress", { @@ -2713,10 +2718,22 @@ async function _runQuery(requestId, prompt, projectPath, model, signal, locale, // on the corresponding tool indicator (errored vs ran). const counterId = _toolUseIdToCounter[block.tool_use_id]; if (counterId !== undefined) { + // A question is answered by DENYING the tool call and + // handing the user's answer back as the denial reason + // (see the AskUserQuestion PreToolUse hook). The CLI + // reports every deny as an error result, so without + // this the panel painted a red "failed" badge on a + // question the user had just answered normally — and + // counted every question as a tool error in metrics. + // The deny is our transport, not a failure; an + // unanswered question means the user cancelled or + // stopped the turn, which is not a failure either. + const resultToolName = _toolUseIdToName[block.tool_use_id]; + const isAnsweredByDeny = resultToolName === "AskUserQuestion"; nodeConnector.triggerPeer("aiToolResult", { requestId: requestId, toolId: counterId, - isError: !!block.is_error, + isError: !!block.is_error && !isAnsweredByDeny, preview: preview }); }