fix(chat): an answered question is not a failed tool call
The answer channel is a denial — the only PreToolUse decision that can carry a result — and the harness stamps every denied tool `is_error`. So a perfectly good answer arrived looking like a failure: red in the transcript, and read by the agent as something that went wrong rather than as the reply to its own question. Confirmed on the first live question through the new channel, which came back as `<error>Show it in Officer</error>`. The parser already sees the tool_use that opened the question, so it records the id and clears the flag when the matching result lands. A timeout or a lost session lands here too and is equally not an error: its output is a sentence explaining itself, which the agent should read rather than retry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -407,3 +407,56 @@ describe('parseStream', () => {
|
||||
expect(state.gotResult).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('an answered question is not a failed tool call', () => {
|
||||
test("the denial that carries the user's answer does not reach the transcript as an error", () => {
|
||||
const events: ChatEvent[] = [];
|
||||
const state = createParseState();
|
||||
const onEvent = (e: ChatEvent) => events.push(e);
|
||||
|
||||
processLine(
|
||||
JSON.stringify({
|
||||
type: 'assistant',
|
||||
message: { content: [{ type: 'tool_use', id: 'toolu_q1', name: 'AskUserQuestion', input: {} }] },
|
||||
}),
|
||||
state,
|
||||
{ onEvent, onSessionId: () => {} },
|
||||
);
|
||||
processLine(
|
||||
JSON.stringify({
|
||||
type: 'user',
|
||||
message: { content: [{ type: 'tool_result', tool_use_id: 'toolu_q1', content: 'Teal', is_error: true }] },
|
||||
}),
|
||||
state,
|
||||
{ onEvent, onSessionId: () => {} },
|
||||
);
|
||||
|
||||
const result = events.find((e) => e.type === 'tool:result');
|
||||
expect(result).toMatchObject({ toolCallId: 'toolu_q1', output: 'Teal', isError: false });
|
||||
});
|
||||
|
||||
test('while an ordinary tool that really failed keeps its flag', () => {
|
||||
const events: ChatEvent[] = [];
|
||||
const state = createParseState();
|
||||
const onEvent = (e: ChatEvent) => events.push(e);
|
||||
|
||||
processLine(
|
||||
JSON.stringify({
|
||||
type: 'assistant',
|
||||
message: { content: [{ type: 'tool_use', id: 'toolu_b1', name: 'Bash', input: {} }] },
|
||||
}),
|
||||
state,
|
||||
{ onEvent, onSessionId: () => {} },
|
||||
);
|
||||
processLine(
|
||||
JSON.stringify({
|
||||
type: 'user',
|
||||
message: { content: [{ type: 'tool_result', tool_use_id: 'toolu_b1', content: 'no such file', is_error: true }] },
|
||||
}),
|
||||
state,
|
||||
{ onEvent, onSessionId: () => {} },
|
||||
);
|
||||
|
||||
expect(events.find((e) => e.type === 'tool:result')).toMatchObject({ isError: true });
|
||||
});
|
||||
});
|
||||
|
||||
@@ -24,8 +24,20 @@ type ParseState = {
|
||||
*/
|
||||
textBuffers: Map<string, string>;
|
||||
gotResult: boolean;
|
||||
/**
|
||||
* Tool call ids belonging to questions the user is being asked.
|
||||
*
|
||||
* Kept so their results can be un-flagged: the sidecar answers `AskUserQuestion` by DENYING it with the
|
||||
* user's words (the only hook decision that can carry a result), and the harness stamps every denied tool
|
||||
* `is_error`. So a perfectly good answer arrives looking like a failed tool call — red in the transcript,
|
||||
* and read by the agent as something that went wrong rather than as the reply to its own question.
|
||||
*/
|
||||
questionCallIds: Set<string>;
|
||||
};
|
||||
|
||||
/** Both harnesses ask the human the same way and name the tool differently. */
|
||||
const QUESTION_TOOLS = new Set(['question', 'AskUserQuestion']);
|
||||
|
||||
/** '' when a message is the main agent's own. Keys the buffer map and stamps every event we emit. */
|
||||
function parentOf(msg: Record<string, unknown>): string {
|
||||
const id = msg.parent_tool_use_id;
|
||||
@@ -68,6 +80,7 @@ function handleAssistant(msg: Record<string, unknown>, state: ParseState, onEven
|
||||
state.textBuffers.delete(parent);
|
||||
} else if (block.type === 'tool_use') {
|
||||
flushTextBuffer(state, parent, onEvent);
|
||||
if (QUESTION_TOOLS.has((block.name as string) ?? '')) state.questionCallIds.add((block.id as string) ?? '');
|
||||
onEvent({
|
||||
type: 'tool:start',
|
||||
toolCallId: (block.id as string) ?? '',
|
||||
@@ -79,7 +92,7 @@ function handleAssistant(msg: Record<string, unknown>, state: ParseState, onEven
|
||||
}
|
||||
}
|
||||
|
||||
function handleUser(msg: Record<string, unknown>, onEvent: (event: ChatEvent) => void): void {
|
||||
function handleUser(msg: Record<string, unknown>, state: ParseState, onEvent: (event: ChatEvent) => void): void {
|
||||
const message = msg.message as Record<string, unknown> | undefined;
|
||||
const content = message?.content as Array<Record<string, unknown>> | undefined;
|
||||
if (!Array.isArray(content)) return;
|
||||
@@ -97,13 +110,14 @@ function handleUser(msg: Record<string, unknown>, onEvent: (event: ChatEvent) =>
|
||||
.map((c) => c.text as string)
|
||||
.join('\n');
|
||||
}
|
||||
onEvent({
|
||||
type: 'tool:result',
|
||||
toolCallId: (block.tool_use_id as string) ?? '',
|
||||
output,
|
||||
isError: (block.is_error as boolean) ?? false,
|
||||
...parented(parent),
|
||||
});
|
||||
const toolCallId = (block.tool_use_id as string) ?? '';
|
||||
// An answered question is not a failed tool call. See `questionCallIds`: the answer channel is a
|
||||
// denial, so the flag says only that the tool was prevented from resolving itself — which is the
|
||||
// mechanism working, not a fault. A question that timed out or lost its session lands here too, and
|
||||
// is equally not an error: its output is a sentence explaining that, which the agent should read as
|
||||
// information rather than as something to retry.
|
||||
const isError = state.questionCallIds.has(toolCallId) ? false : ((block.is_error as boolean) ?? false);
|
||||
onEvent({ type: 'tool:result', toolCallId, output, isError, ...parented(parent) });
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -208,7 +222,7 @@ export function processMessage(
|
||||
handleAssistant(msg, state, callbacks.onEvent);
|
||||
break;
|
||||
case 'user':
|
||||
handleUser(msg, callbacks.onEvent);
|
||||
handleUser(msg, state, callbacks.onEvent);
|
||||
break;
|
||||
case 'system':
|
||||
handleSystem(msg, callbacks);
|
||||
@@ -244,7 +258,7 @@ export function processLine(line: string, state: ParseState, callbacks: StreamPa
|
||||
* Create a fresh parse state for a new stream.
|
||||
*/
|
||||
export function createParseState(): ParseState {
|
||||
return { textBuffers: new Map(), gotResult: false };
|
||||
return { textBuffers: new Map(), gotResult: false, questionCallIds: new Set() };
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user