diff --git a/docs/releases/UNRELEASED.md b/docs/releases/UNRELEASED.md index 399e219f..182b511e 100644 --- a/docs/releases/UNRELEASED.md +++ b/docs/releases/UNRELEASED.md @@ -119,6 +119,11 @@ reset this file. scrollable "view all" dialog. Active runs are deliberately never capped: run ids sort by start time, so a strict newest-N page would hide a long-running run — exactly the one still worth acting on. +- MCP and tool rows in chat carry their arguments, result and a real terminal + status behind the new default-off `mcpToolDetail` flag (#362, #365): the row + resolves in place from WORKING to done or failed instead of staying + permanently "running", its chevron opens the full payload, and the toggle now + survives scrolling away and back. - Fixed a race in the default-off `changesReview` re-fold (#368): a turn that started while the re-fold was fetching history had its just-sent message wiped from the timeline, because the commit replaced the timeline wholesale diff --git a/src/components/chat/ChatTimeline.tsx b/src/components/chat/ChatTimeline.tsx index 44f28ea0..12f4ea3a 100644 --- a/src/components/chat/ChatTimeline.tsx +++ b/src/components/chat/ChatTimeline.tsx @@ -97,6 +97,44 @@ function expansionToggle( // when the flag is on. Flag off: the reducer never sets // `turnComplete`/groups events either, so this is already the legacy // per-event card; the explicit flag check here is defense in depth. +/** `mcpToolDetail` gates the richer rows (#362, #365): a persisted row's shape + * changed and the parser only emits completions on newer builds, so main stays + * releasable while this is dark. Same precedent as `changesReview` gating the + * receipt card. Split out of `renderItem` to keep it under the complexity cap. */ +function renderToolUseItem( + item: Extract, + rowKey: string, + expansion: RowExpansion | undefined, +): JSX.Element { + return ( + + ); +} + +function renderMcpItem( + item: Extract, + rowKey: string, + expansion: RowExpansion | undefined, +): JSX.Element { + const gated = () => flagEnabled("mcpToolDetail"); + return ( + + ); +} + function renderFileChangeItem( item: Extract, rowKey: string, @@ -176,10 +214,14 @@ function renderItem( ); case "fileChange": return renderFileChangeItem(item, rowKey, chatId, expansion, projectRoot, changesReceipts); + // `mcpToolDetail` gates the richer rows (#362, #365): a persisted row's + // shape changed, and the parser only emits completions on newer builds, so + // main stays releasable while this is dark. Same precedent as + // `changesReview` gating the receipt card above. case "toolUse": - return ; + return renderToolUseItem(item, rowKey, expansion); case "mcpToolCall": - return ; + return renderMcpItem(item, rowKey, expansion); case "webSearch": return ; case "plan": diff --git a/src/components/chat/McpCard.tsx b/src/components/chat/McpCard.tsx index a33aae8a..57a40730 100644 --- a/src/components/chat/McpCard.tsx +++ b/src/components/chat/McpCard.tsx @@ -1,15 +1,27 @@ import { type JSX, Show, createSignal } from "solid-js"; import { compactInline, hasHiddenDetail } from "../../lib/chatDisplay"; import { IconChevronRight } from "../icons"; -import { Disclosure } from "../ui"; +import type { ToolCallStatus } from "../../stores/agentChat"; +import { Disclosure, StatusPill, type StatusIntent } from "../ui"; import "./chat.css"; +const STATUS_INTENT: Record = { + inProgress: "warning", + completed: "connected", + failed: "error", +}; + export function McpCard(props: { server: string; tool: string; detail?: string | null; + status?: ToolCallStatus; + open?: boolean; + onToggle?: () => void; }): JSX.Element { - const [open, setOpen] = createSignal(false); + const [localOpen, setLocalOpen] = createSignal(false); + const open = () => props.open ?? localOpen(); + const toggle = () => (props.onToggle ? props.onToggle() : setLocalOpen((v) => !v)); const detail = () => props.detail ?? ""; const canExpand = () => hasHiddenDetail(detail(), 120); @@ -21,7 +33,7 @@ export function McpCard(props: { aria-expanded={open()} aria-label={open() ? "Hide MCP call details" : "Show MCP call details"} disabled={!canExpand()} - onClick={() => canExpand() && setOpen((v) => !v)} + onClick={() => canExpand() && toggle()} > + + {(status) => ( + + + + )} + diff --git a/src/components/chat/ToolUseCard.tsx b/src/components/chat/ToolUseCard.tsx index 4ffdc740..9e546f9e 100644 --- a/src/components/chat/ToolUseCard.tsx +++ b/src/components/chat/ToolUseCard.tsx @@ -1,14 +1,28 @@ import { type JSX, Show, createSignal } from "solid-js"; import { compactInline, hasHiddenDetail } from "../../lib/chatDisplay"; import { IconChevronRight } from "../icons"; -import { Disclosure } from "../ui"; +import type { ToolCallStatus } from "../../stores/agentChat"; +import { Disclosure, StatusPill, type StatusIntent } from "../ui"; import "./chat.css"; +const STATUS_INTENT: Record = { + inProgress: "warning", + completed: "connected", + failed: "error", +}; + export function ToolUseCard(props: { name: string; detail?: string | null; + status?: ToolCallStatus; + /** Hoisted so the toggle survives virtualization remount, the same contract + * `CommandCard` and `ThinkingBubble` already use (#362). */ + open?: boolean; + onToggle?: () => void; }): JSX.Element { - const [open, setOpen] = createSignal(false); + const [localOpen, setLocalOpen] = createSignal(false); + const open = () => props.open ?? localOpen(); + const toggle = () => (props.onToggle ? props.onToggle() : setLocalOpen((v) => !v)); const detail = () => props.detail ?? ""; const canExpand = () => hasHiddenDetail(detail(), 120); @@ -20,7 +34,7 @@ export function ToolUseCard(props: { aria-expanded={open()} aria-label={open() ? "Hide tool details" : "Show tool details"} disabled={!canExpand()} - onClick={() => canExpand() && setOpen((v) => !v)} + onClick={() => canExpand() && toggle()} > + + {(status) => ( + + + + )} + diff --git a/src/stores/agentChat.ts b/src/stores/agentChat.ts index c3a2e963..efe9f2bf 100644 --- a/src/stores/agentChat.ts +++ b/src/stores/agentChat.ts @@ -84,8 +84,25 @@ export type AgentTimelineItem = * through the existing `FileChangeCard` until it closes. */ turnComplete: boolean; } - | { type: "toolUse"; seq: number; itemId: string; name: string; detail: string | null } - | { type: "mcpToolCall"; seq: number; itemId: string; server: string; tool: string } + | { + type: "toolUse"; + seq: number; + itemId: string; + name: string; + detail: string | null; + /** Absent on rows persisted before the parser learned to resolve + * non-Bash tools (#365), so a replayed history stays readable. */ + status?: ToolCallStatus; + } + | { + type: "mcpToolCall"; + seq: number; + itemId: string; + server: string; + tool: string; + detail?: string | null; + status?: ToolCallStatus; + } | { type: "webSearch"; seq: number; itemId: string; query: string } | { type: "plan"; seq: number; items: PlanItem[] } | { @@ -107,6 +124,10 @@ export type AgentApprovalQuestion = { options: { label: string; description?: string }[]; }; +/** Mirrors the wire event's status. Kept as its own alias so the two card + * types and the reducers cannot drift apart. */ +export type ToolCallStatus = "inProgress" | "completed" | "failed"; + export type AgentApproval = { approvalId: string; kind: "command" | "fileChange" | "toolUse" | "question"; @@ -931,7 +952,15 @@ function reduceMcpToolCall( const timeline = chat.timeline.map((item) => { if (item.type !== "mcpToolCall" || item.itemId !== event.itemId) return item; matched = true; - return { ...item, server: event.server, tool: event.tool }; + return { + ...item, + server: event.server, + tool: event.tool, + // A completion carries the result; keep the arg summary when it does + // not, so resolving a row never blanks what it already showed. + detail: event.detail ?? item.detail, + status: event.status, + }; }); if (matched) return withTimeline(chat, timeline); return withTimeline(chat, [ @@ -942,6 +971,8 @@ function reduceMcpToolCall( itemId: event.itemId, server: event.server, tool: event.tool, + detail: event.detail, + status: event.status, }, ]); } @@ -958,7 +989,10 @@ function reduceToolUse( return { ...item, name: event.name, - detail: event.detail, + // A completion carries the result; keep the arg summary when it does not, + // so resolving a row never blanks what it already showed. + detail: event.detail ?? item.detail, + status: event.status, }; }); if (matched) return withTimeline(chat, timeline); @@ -970,6 +1004,7 @@ function reduceToolUse( itemId: event.itemId, name: event.name, detail: event.detail, + status: event.status, }, ]); } diff --git a/src/stores/flags.ts b/src/stores/flags.ts index 759ef2e7..28d973e6 100644 --- a/src/stores/flags.ts +++ b/src/stores/flags.ts @@ -54,6 +54,7 @@ const definitions = { pikitLanes: { description: "Pi-kit lanes visibility in Settings (#274 slice 3, #288)" }, pikitContext: { description: "PickForge context surfaced in Pi sessions (#299 slice 4)" }, changesReview: { description: "In-chat changes receipt and Review-changes action (#231)" }, + mcpToolDetail: { description: "MCP/tool row args, results and terminal status (#362, #365)" }, flatChatList: { description: "Flat chat-first sidebar sorted by state (#306 PR1)" }, orchestraBoard: { description: "Orchestra column-per-status board scaffold (#319, #196 PR1)" }, messageQueue: { description: "Queue messages while an agent turn is running (#357)" }, diff --git a/tests/unit/agentChat.test.ts b/tests/unit/agentChat.test.ts index bc4dacfa..63f9978c 100644 --- a/tests/unit/agentChat.test.ts +++ b/tests/unit/agentChat.test.ts @@ -1360,6 +1360,74 @@ describe("agentChat store reducer", () => { expect(rows[0].itemId).toBe("mcp-1"); }); + it("carries an MCP call's status and detail, and resolves in place (#362)", async () => { + const { chatId, emit } = await startChat(); + + emit({ + kind: "mcpToolCall", + itemId: "mcp-1", + server: "pickforge-lanes", + tool: "lanes_wait", + status: "inProgress", + detail: "run: run-7", + }); + expect(timeline(chatId).find((item) => item.type === "mcpToolCall")).toMatchObject({ + status: "inProgress", + detail: "run: run-7", + }); + + emit({ + kind: "mcpToolCall", + itemId: "mcp-1", + server: "pickforge-lanes", + tool: "lanes_wait", + status: "completed", + detail: "lane finished", + }); + + const rows = timeline(chatId).filter((item) => item.type === "mcpToolCall"); + expect(rows).toHaveLength(1); + expect(rows[0]).toMatchObject({ status: "completed", detail: "lane finished" }); + }); + + it("keeps the arg summary when a completion carries no result (#362)", async () => { + // Resolving a row must never blank what it already showed. + const { chatId, emit } = await startChat(); + + emit({ + kind: "mcpToolCall", + itemId: "mcp-1", + server: "srv", + tool: "do", + status: "inProgress", + detail: "a: 1", + }); + emit({ + kind: "mcpToolCall", + itemId: "mcp-1", + server: "srv", + tool: "do", + status: "completed", + detail: null, + }); + + expect(timeline(chatId).find((item) => item.type === "mcpToolCall")).toMatchObject({ + status: "completed", + detail: "a: 1", + }); + }); + + it("carries a generic tool's terminal status (#365)", async () => { + const { chatId, emit } = await startChat(); + + emit({ kind: "toolUse", itemId: "t1", name: "Task", status: "inProgress", detail: "scout" }); + emit({ kind: "toolUse", itemId: "t1", name: "Task", status: "failed", detail: "boom" }); + + const rows = timeline(chatId).filter((item) => item.type === "toolUse"); + expect(rows).toHaveLength(1); + expect(rows[0]).toMatchObject({ status: "failed", detail: "boom" }); + }); + it("still appends a distinct MCP call as its own row (#362)", async () => { const { chatId, emit } = await startChat(); diff --git a/tests/unit/flags.test.ts b/tests/unit/flags.test.ts index 39135800..5d73aad1 100644 --- a/tests/unit/flags.test.ts +++ b/tests/unit/flags.test.ts @@ -40,6 +40,7 @@ describe("flags", () => { "pikitLanes", "pikitContext", "changesReview", + "mcpToolDetail", "flatChatList", "orchestraBoard", "messageQueue",