fix(tui): preserve failures in answer-only mode
This commit is contained in:
@@ -27,8 +27,9 @@ import { useI18n } from '@/i18n'
|
||||
import { mcpTargets, toolLabels } from '@/lib/connector-tools'
|
||||
import { generatedImageFromResult } from '@/lib/generated-images'
|
||||
import { separateGluedReasoningBlocks } from '@/lib/reasoning-blocks'
|
||||
import { isCardTool } from '@/lib/tool-render-class'
|
||||
import { isTodoToolName } from '@/lib/todos'
|
||||
import { isCardTool } from '@/lib/tool-render-class'
|
||||
import { extractToolErrorMessage } from '@/lib/tool-result-summary'
|
||||
import { useEnterAnimation } from '@/lib/use-enter-animation'
|
||||
import { cn } from '@/lib/utils'
|
||||
import { $reasoningCollapsedByDefault, $showReasoning } from '@/store/reasoning-disclosure'
|
||||
@@ -75,6 +76,34 @@ const DelegateToolPart: FC<TimelineToolCallProps> = props => {
|
||||
)
|
||||
}
|
||||
|
||||
// A failure the user still has to see: the part's isError comes from a
|
||||
// top-level payload.error the gateway's tool.complete never sets (the failure
|
||||
// sits inside result), so the same predicate the summary rows use applies —
|
||||
// with explicit success beating stale envelope errors, as in toolStatus, and
|
||||
// a non-zero exit_code counting as failure, mirroring the gateway's
|
||||
// _tool_result_needs_user (terminal reports {output, exit_code: 1, error: null}).
|
||||
const failedCallNeedsUser = ({ isError, result }: TimelineToolCallProps): boolean => {
|
||||
if (isError || result === undefined) {
|
||||
return isError === true
|
||||
}
|
||||
|
||||
const record =
|
||||
typeof result === 'object' && result !== null && !Array.isArray(result)
|
||||
? (result as Record<string, unknown>)
|
||||
: undefined
|
||||
|
||||
if (record?.success === true || record?.ok === true) {
|
||||
return false
|
||||
}
|
||||
|
||||
return Boolean(
|
||||
record?.success === false ||
|
||||
record?.ok === false ||
|
||||
extractToolErrorMessage(result) ||
|
||||
(typeof record?.exit_code === 'number' && record.exit_code !== 0)
|
||||
)
|
||||
}
|
||||
|
||||
const ChainToolFallback: FC<TimelineToolCallProps> = props => {
|
||||
const showReasoning = useStore($showReasoning)
|
||||
|
||||
@@ -144,8 +173,10 @@ const ChainToolFallback: FC<TimelineToolCallProps> = props => {
|
||||
|
||||
// Answer-only: process chrome (reads, searches, commands) stays off the
|
||||
// transcript. Cards, approvals, and failed calls the user must act on remain.
|
||||
// reasoning_effort is not a display switch.
|
||||
if (!showReasoning && !props.isError && !isCardTool(props.toolName)) {
|
||||
// reasoning_effort is not a display switch. The failure check reads the
|
||||
// result body too: gateway tool.complete carries failures inside `result`,
|
||||
// never as a top-level error that would set the part's isError.
|
||||
if (!showReasoning && !failedCallNeedsUser(props) && !isCardTool(props.toolName)) {
|
||||
return null
|
||||
}
|
||||
|
||||
|
||||
@@ -4,6 +4,7 @@ import { afterEach, beforeEach, describe, expect, it } from 'vitest'
|
||||
|
||||
import { stubThreadEnvironment, stubThreadViewportSize, ThreadRuntime } from '@/components/assistant-ui/test-utils'
|
||||
import { Thread } from '@/components/assistant-ui/thread'
|
||||
import { type GatewayEventPayload, upsertToolPart } from '@/lib/chat-messages'
|
||||
import { clearAllPrompts, setApprovalRequest } from '@/store/prompts'
|
||||
import { $showReasoning } from '@/store/reasoning-disclosure'
|
||||
import { $activeSessionId } from '@/store/session'
|
||||
@@ -74,6 +75,43 @@ function Harness() {
|
||||
)
|
||||
}
|
||||
|
||||
// Feed real gateway payloads through the store's event-to-part mapping
|
||||
// (upsertToolPart), the same path `handleToolEvent` drives on tool.complete —
|
||||
// instead of hand-setting isError on the part, which skips the mapping.
|
||||
function toolCompleteMessage(...payloads: GatewayEventPayload[]): ThreadMessage {
|
||||
const content = payloads.reduce(
|
||||
(acc, payload) => upsertToolPart(acc, payload, 'complete', 3),
|
||||
[] as ReturnType<typeof upsertToolPart>
|
||||
)
|
||||
|
||||
return {
|
||||
id: 'assistant-tool-complete',
|
||||
role: 'assistant',
|
||||
content: [
|
||||
...content,
|
||||
{
|
||||
type: 'text',
|
||||
text: 'done'
|
||||
}
|
||||
],
|
||||
status: { type: 'complete', reason: 'stop' },
|
||||
createdAt,
|
||||
metadata: {
|
||||
unstable_state: null,
|
||||
unstable_annotations: [],
|
||||
unstable_data: [],
|
||||
steps: [],
|
||||
custom: {}
|
||||
}
|
||||
} as unknown as ThreadMessage
|
||||
}
|
||||
|
||||
const completionHarness = (payload: GatewayEventPayload) => (
|
||||
<ThreadRuntime messages={[toolCompleteMessage(payload)]}>
|
||||
<Thread />
|
||||
</ThreadRuntime>
|
||||
)
|
||||
|
||||
beforeEach(() => {
|
||||
clearAllPrompts()
|
||||
$activeSessionId.set('sess-1')
|
||||
@@ -107,4 +145,71 @@ describe('answer-only display policy', () => {
|
||||
expect(await screen.findByText(/Explored 2 files/)).toBeTruthy()
|
||||
expect(container.querySelector('[data-slot="aui_thinking-disclosure"]')).not.toBeNull()
|
||||
})
|
||||
|
||||
it('keeps a failed call whose error sits inside result, from a real tool.complete payload', async () => {
|
||||
// The gateway's tool.complete never sets a top-level error: a read_file
|
||||
// failure rides inside result. The answer-only gate must still show it.
|
||||
$showReasoning.set(false)
|
||||
|
||||
const { container } = render(
|
||||
completionHarness({
|
||||
name: 'read_file',
|
||||
tool_id: 'read-fail-1',
|
||||
args: { path: '/repo/src/status.tsx' },
|
||||
result: { error: 'disk full, act now' }
|
||||
})
|
||||
)
|
||||
|
||||
expect(await screen.findByText('done')).toBeTruthy()
|
||||
const rows = container.querySelectorAll('[data-tool-row]')
|
||||
expect(rows).toHaveLength(1)
|
||||
})
|
||||
|
||||
it('keeps a failed terminal call with a non-zero exit_code, from a real tool.complete payload', async () => {
|
||||
$showReasoning.set(false)
|
||||
|
||||
const { container } = render(
|
||||
completionHarness({
|
||||
name: 'terminal',
|
||||
tool_id: 'term-fail-1',
|
||||
args: { command: 'deploy' },
|
||||
result: { output: 'Error: deploy failed', exit_code: 1, error: null }
|
||||
})
|
||||
)
|
||||
|
||||
expect(await screen.findByText('done')).toBeTruthy()
|
||||
expect(container.querySelectorAll('[data-tool-row]')).toHaveLength(1)
|
||||
})
|
||||
|
||||
it('keeps a call that reports success: false, from a real tool.complete payload', async () => {
|
||||
$showReasoning.set(false)
|
||||
|
||||
const { container } = render(
|
||||
completionHarness({
|
||||
name: 'write_file',
|
||||
tool_id: 'write-fail-1',
|
||||
args: { path: '/repo/out.txt' },
|
||||
result: { success: false }
|
||||
})
|
||||
)
|
||||
|
||||
expect(await screen.findByText('done')).toBeTruthy()
|
||||
expect(container.querySelectorAll('[data-tool-row]')).toHaveLength(1)
|
||||
})
|
||||
|
||||
it('still hides a successful call driven through the same tool.complete mapping', async () => {
|
||||
$showReasoning.set(false)
|
||||
|
||||
const { container } = render(
|
||||
completionHarness({
|
||||
name: 'read_file',
|
||||
tool_id: 'read-ok-1',
|
||||
args: { path: '/repo/src/status.tsx' },
|
||||
result: { content: 'export const Status = () => null' }
|
||||
})
|
||||
)
|
||||
|
||||
expect(await screen.findByText('done')).toBeTruthy()
|
||||
expect(container.querySelectorAll('[data-tool-row]')).toHaveLength(0)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -119,3 +119,109 @@ def test_hidden_reasoning_drops_moa_reference_chrome(monkeypatch):
|
||||
server._on_tool_progress("hide-moa", "moa.reference", "reference-a", "other model's thoughts", None)
|
||||
|
||||
assert events == []
|
||||
|
||||
|
||||
def test_hidden_reasoning_drops_moa_status_and_aggregating_chrome(monkeypatch):
|
||||
events = _capture(monkeypatch)
|
||||
_session(monkeypatch, "hide-moa-status", show_reasoning=False, effort="high")
|
||||
|
||||
server._on_tool_progress(
|
||||
"hide-moa-status", "moa.progress", "aggregator-a", None, None, moa_refs_done=1, moa_refs_total=3
|
||||
)
|
||||
server._on_tool_progress(
|
||||
"hide-moa-status", "moa.phase", "aggregator-a", None, None, moa_phase="aggregator"
|
||||
)
|
||||
server._on_tool_progress("hide-moa-status", "moa.aggregating", "aggregator-a", None, None)
|
||||
|
||||
assert events == []
|
||||
|
||||
|
||||
def test_shown_reasoning_still_emits_moa_aggregating(monkeypatch):
|
||||
events = _capture(monkeypatch)
|
||||
_session(monkeypatch, "show-moa-status", show_reasoning=True, effort="high")
|
||||
|
||||
server._on_tool_progress("show-moa-status", "moa.aggregating", "aggregator-a", None, None)
|
||||
|
||||
assert [event[0] for event in events] == ["moa.aggregating"]
|
||||
assert events[0][2]["aggregator"] == "aggregator-a"
|
||||
|
||||
|
||||
def test_hidden_reasoning_drops_subagent_thinking_text_on_parent(monkeypatch):
|
||||
events = _capture(monkeypatch)
|
||||
_session(monkeypatch, "hide-sub", show_reasoning=False)
|
||||
|
||||
server._on_tool_progress(
|
||||
"hide-sub",
|
||||
"subagent.thinking",
|
||||
"tool",
|
||||
"the child's private chain of thought",
|
||||
None,
|
||||
child_session_id="child-key",
|
||||
)
|
||||
|
||||
# The lifecycle frame still reaches the parent (the delegate card renders
|
||||
# progress), but the child's reasoning text must not ride along.
|
||||
assert [event[0] for event in events] == ["subagent.thinking"]
|
||||
assert "text" not in events[0][2]
|
||||
assert events[0][2]["child_session_id"] == "child-key"
|
||||
|
||||
|
||||
def test_shown_reasoning_keeps_subagent_thinking_text(monkeypatch):
|
||||
events = _capture(monkeypatch)
|
||||
_session(monkeypatch, "show-sub", show_reasoning=True)
|
||||
|
||||
server._on_tool_progress(
|
||||
"show-sub",
|
||||
"subagent.thinking",
|
||||
"tool",
|
||||
"the child's visible thought",
|
||||
None,
|
||||
child_session_id="child-key",
|
||||
)
|
||||
|
||||
assert [event[0] for event in events] == ["subagent.thinking"]
|
||||
assert events[0][2]["text"] == "the child's visible thought"
|
||||
|
||||
|
||||
def test_hidden_reasoning_shows_failed_terminal_exit_code(monkeypatch):
|
||||
events = _capture(monkeypatch)
|
||||
_session(monkeypatch, "hide-exit", show_reasoning=False, effort="high")
|
||||
|
||||
server._on_tool_complete(
|
||||
"hide-exit",
|
||||
"tool-exit",
|
||||
"terminal",
|
||||
{"command": "deploy"},
|
||||
json.dumps({"output": "boom", "exit_code": 1, "error": None}),
|
||||
)
|
||||
|
||||
failed = [event for event in events if event[2].get("tool_id") == "tool-exit"]
|
||||
assert [event[0] for event in failed] == ["tool.complete"]
|
||||
assert failed[0][2]["result"]["exit_code"] == 1
|
||||
|
||||
|
||||
def test_hidden_reasoning_hides_successful_terminal_exit(monkeypatch):
|
||||
events = _capture(monkeypatch)
|
||||
_session(monkeypatch, "hide-exit-ok", show_reasoning=False, effort="high")
|
||||
|
||||
server._on_tool_complete(
|
||||
"hide-exit-ok",
|
||||
"tool-exit-ok",
|
||||
"terminal",
|
||||
{"command": "deploy"},
|
||||
json.dumps({"output": "ok", "exit_code": 0, "error": None}),
|
||||
)
|
||||
|
||||
assert not any(event[2].get("tool_id") == "tool-exit-ok" for event in events)
|
||||
|
||||
|
||||
def test_tool_result_needs_user_treats_nonzero_exit_code_as_failure():
|
||||
assert server._tool_result_needs_user(json.dumps({"output": "boom", "exit_code": 1, "error": None})) is True
|
||||
assert server._tool_result_needs_user(json.dumps({"output": "ok", "exit_code": 0, "error": None})) is False
|
||||
# A boolean exit_code is not an exit status; True must not read as failure-by-1.
|
||||
assert server._tool_result_needs_user(json.dumps({"exit_code": True})) is False
|
||||
assert server._tool_result_needs_user(json.dumps({"exit_code": "1"})) is False
|
||||
assert server._tool_result_needs_user(json.dumps({"success": False})) is True
|
||||
assert server._tool_result_needs_user(json.dumps({"ok": False, "output": "denied"})) is True
|
||||
assert server._tool_result_needs_user(json.dumps({"error": "disk full"})) is True
|
||||
assert server._tool_result_needs_user("not json") is False
|
||||
|
||||
@@ -201,7 +201,12 @@ def _tool_result_needs_user(result: object) -> bool:
|
||||
if data.get("success") is False or data.get("ok") is False:
|
||||
return True
|
||||
error = data.get("error")
|
||||
return isinstance(error, str) and bool(error.strip())
|
||||
if isinstance(error, str) and bool(error.strip()):
|
||||
return True
|
||||
# terminal reports a failed command as {output, exit_code: 1, error: null}:
|
||||
# a non-zero exit is a failure the user must see even without an error string.
|
||||
exit_code = data.get("exit_code")
|
||||
return isinstance(exit_code, int) and exit_code != 0 and not isinstance(exit_code, bool)
|
||||
|
||||
|
||||
def _tool_labels(name: str, args: dict) -> list[dict] | None:
|
||||
@@ -425,7 +430,7 @@ _SUBAGENT_FIELDS = (
|
||||
)
|
||||
|
||||
|
||||
def _progress_subagent(sid, name, preview, kw, event_type):
|
||||
def _progress_subagent(sid: str, name: str, preview, kw, event_type):
|
||||
payload = {"goal": str(kw.get("goal") or ""), "task_count": int(kw.get("task_count") or 1), "task_index": int(kw.get("task_index") or 0)}
|
||||
source = {**kw, "tool_name": name, "text": preview}
|
||||
for key, present, coerce in _SUBAGENT_FIELDS:
|
||||
@@ -433,6 +438,10 @@ def _progress_subagent(sid, name, preview, kw, event_type):
|
||||
val = coerce(source[key])
|
||||
if val is not None:
|
||||
payload[key] = val
|
||||
# subagent.thinking's text is the child's chain of thought: with reasoning hidden the
|
||||
# delegate card must not leak it, same policy as the child-mirror's reasoning.delta.
|
||||
if event_type == "subagent.thinking" and not _session_show_reasoning(sid):
|
||||
payload.pop("text", None)
|
||||
if preview and event_type == "subagent.tool":
|
||||
payload["tool_preview"] = str(preview)
|
||||
payload["text"] = str(preview)
|
||||
@@ -443,12 +452,20 @@ def _progress_subagent(sid, name, preview, kw, event_type):
|
||||
_mirror_subagent_to_child(event_type, payload)
|
||||
|
||||
|
||||
def _progress_moa_aggregating(sid, name, preview, kw):
|
||||
# Aggregation is the fan-out's tail: the same answer-only policy that drops
|
||||
# moa.progress/moa.phase (status-bar counters) applies to this announcement.
|
||||
if not _session_show_reasoning(sid):
|
||||
return
|
||||
_emit("moa.aggregating", sid, {"aggregator": str(name or "")})
|
||||
|
||||
|
||||
# event_type -> (handler, requires): `requires` names the arg that must be truthy for the row to be
|
||||
# emitted at all ("name" / "preview" / None).
|
||||
_PROGRESS_HANDLERS = {
|
||||
"tool.output_risk": (_progress_output_risk, "name"), "reasoning.available": (_progress_reasoning, "preview"),
|
||||
"moa.reference": (_progress_moa_reference, "name"),
|
||||
"moa.aggregating": (lambda sid, name, preview, kw: _emit("moa.aggregating", sid, {"aggregator": str(name or "")}), None),
|
||||
"moa.aggregating": (_progress_moa_aggregating, None),
|
||||
"moa.progress": (_progress_moa_progress, None), "moa.phase": (_progress_moa_phase, None),
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user