feat(ui): dedicated chat components for tool search/select/query - #430
feat(ui): dedicated chat components for tool search/select/query#430Zerlight wants to merge 12 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the chat transcript UI for tool calls by introducing dedicated rendering for “search-like” tool calls (including Claude’s ToolSearch) and by normalizing Codex MCP tool titles to the shared mcp__<server>__<tool> slug so MCP server identity is consistently preserved in the UI and in history replays.
Changes:
- Add ToolSearch-specific parsing + rendering (inline tool-name rows, humanized headers, preserved context across states).
- Humanize generic search tool headers to summarize counts (matches/files) while keeping raw queries in the expandable body only.
- Normalize Codex MCP tool-call titles (live adapter + history replay) to
mcp__<server>__<tool>, including plugin-app namespace unwrapping; update mocks/tests and fix git fixture signing config for integration tests.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/presentation/ui/src/tool-utils.ts | Adds toolCallSearchCounts, adjusts search header/body behavior, and integrates ToolSearch handling into body visibility logic. |
| packages/presentation/ui/src/chat/tool-search.tsx | New ToolSearch result renderer that displays loaded tool names inline with MCP server suffix. |
| packages/presentation/ui/src/chat/tool-result-preview.tsx | Routes ToolSearch calls to the dedicated renderer; updates search preview card icon/behavior for empty-output searches. |
| packages/presentation/ui/src/chat/tool-result-content.ts | Adds toolSearchPresentation parsing to classify ToolSearch output into rows vs. prose message. |
| packages/presentation/ui/src/chat/tool-kind-icons.ts | Updates the search kind icon to TextSearchIcon. |
| packages/presentation/ui/src/chat/tool-call-item.tsx | Humanizes search headers (ToolSearch verbs + generic search counts), suppresses search metadata badges in the body. |
| packages/presentation/ui/src/chat/activity-run.tsx | Swaps activity “explore” icon to TelescopeIcon. |
| packages/presentation/ui/src/chat/tests/tool-search.test.tsx | Adds unit tests covering ToolSearch header/body behavior across states and MCP name splitting. |
| packages/presentation/ui/src/chat/tests/tool-result-content.test.ts | Adds unit tests for toolSearchPresentation parsing behavior. |
| packages/presentation/ui/src/chat/tests/tool-call-metadata.test.tsx | Updates tests to validate search header summarization/count extraction and query placement. |
| packages/presentation/i18n/src/locales/en.ts | Adds localized strings for ToolSearch header verbs and search count summaries. |
| packages/presentation/i18n/src/locales/zh-cn.ts | Adds localized strings for ToolSearch header verbs and search count summaries (zh-CN). |
| packages/host/engine/tests/integration/git-mutations.test.ts | Disables GPG signing in the git fixture to make commits reliable on signing-enabled machines. |
| packages/host/agent-adapter/src/native/codex/history-tools.ts | Normalizes rollout/history MCP function calls into mcp__<server>__<tool> titles. |
| packages/host/agent-adapter/src/native/codex/adapter.ts | Normalizes live Codex MCP tool-call titles to the shared MCP slug and unwraps plugin namespace. |
| packages/host/agent-adapter/src/tests/codex-mcp-tools.test.ts | Adds tests for Codex MCP slug normalization and plugin namespace unwrapping. |
| packages/host/agent-adapter/src/tests/codex-history.test.ts | Adds tests ensuring history replay uses the same MCP slug format as live events. |
| packages/client/workbench/src/mock/data/showcase.ts | Updates mock showcase tool bursts to exercise new search/ToolSearch presentation paths. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
[!IMPORTANT]
The new
ToolSearchrendering is solid, but thetoolCallHeaderSummarysearch-case deletion strips header context from search tools that don't emit match counts —WebSearchcollapses to a bareWebSearchlabel. Verified by rendering throughToolCallItem.
Reviewed changes
This PR adds dedicated presentation for Claude Code's deferred-tool ToolSearch calls and normalizes Codex MCP tool titles.
- New
ToolSearchResultcomponent (packages/presentation/ui/src/chat/tool-search.tsx) plustoolSearchPresentation()intool-result-content.ts, splittingselect:mode from keyword mode and renderingmcp__server__toolastool (server). tool-call-item.tsxgrows a header branch computingtoolSearch/searchCountsand swaps inToolCaseIcon; search-kind rows now suppress generic metadata.tool-utils.tsaddstoolCallSearchCounts()and changestoolCallHeaderSummary'scase 'search'from returning the query tobreak;SearchRowsdrops its result-count badge.- Codex adapter titles change from
${server}.${tool}tomcp__${server}__${tool}, with a newcodexMcpToolName()handling the rollout-JSONL replay path. - i18n keys added to both
zh-cn.tsanden.tswith matching key sets and valid ICU plurals.
ℹ️ The Codex title change is safe — confirming so it isn't re-litigated
I traced whether mcp__${server}__${tool} could break anything downstream that keys off toolCall.title. It can't: the engine reads title only for approval toolTitle (session-event-processor.ts:150), Codex MCP tools announce as kind: 'other' with no approval path, and deduplication is by toolCallId throughout conversation.ts. codexToolSettle spreads ...existing, so the announce title survives the settle. No action needed — noting it as verified negative evidence.
ℹ️ Nitpicks
The PR bundles three loosely-related concerns — the UI search presentation, the Codex MCP title normalization, and the commit.gpgsign=false git fixture. They're each small, so this isn't worth splitting retroactively, but the title only describes the first.
Separately, the description promises "humanized query + scope" in the header, while the implemented behavior shows counts and removes the query. Worth reconciling the description with what shipped, since the two read as contradictory to a future reader.
Finally, the "I ran the affected surface and observed the change working" checklist item is unchecked — and the search-header path above is exactly the surface where a manual pass would have caught the regression.
Claude Opus ｜ 𝕏
lucas77778
left a comment
lucas77778
left a comment
There was a problem hiding this comment.
Blocking findings:
-
packages/presentation/ui/src/tool-utils.ts:203-206removes the query from every generic search header, whileToolCallItemonly substitutes counts whenrawOutputhas a supported count envelope.WebSearchis classified askind: searchbytoolKindFromName, and Pi/OpenCode-style searches also commonly have nonumMatches/numFiles, so their collapsed rows become a bare tool name (and all in-progress searches are indistinguishable). I reproduced the resulting shape as{kind:"search", headerSummary:undefined, counts:undefined}for aWebSearchcall with querylinkcode release notes. KeeptoolCallSearchQueryas the fallback summary (or append counts to it), while retaining the dedicated ToolSearch branch that hidesselect:queries; add coverage for an output-less generic search and a counted search. -
packages/host/agent-adapter/src/native/codex/adapter.ts:1287-1299embeds the raw Codex MCP server name inmcp__<server>__<tool>, butmcpToolNamesplits on the first__. LinkCode accepts arbitrary non-empty custom MCP names, and the pinned Codex CLI acceptsrepo__prod(verified with an isolatedCODEX_HOME:codex mcp add repo__prod -- /bin/trueandcodex mcp list --jsonboth succeeded). A call from serverrepo__prod, toolsearch_filesis emitted asmcp__repo__prod__search_filesand rendered as serverrepo, toolprod__search_files, defeating the stated identity-preservation goal. Preserve a non-slug fallback for delimiter-containing Codex names (live and history), or introduce an unambiguous representation; add live/history parser cases for such a valid name.
e072cb6 to
c4fa4a1
Compare
|
Both addressed:
|
There was a problem hiding this comment.
ℹ️ No new issues — one unused-export nit inline.
Reviewed changes
Three commits since the last pullfrog review at e072cb6, all of them fixes to round-one feedback. Both the header-summary regression and the codex slug mis-split are addressed, and each fix ships a test that fails without it.
- Restored the search query as the header fallback —
toolCallHeaderSummary'scase 'search'now assignstoolCallSearchQuery(toolCall)whenevertoolCallSearchCountscomes back empty, so counted settles still humanize to counts whileWebSearch, Pi/opencode-shaped outputs, and in-progress searches keep their query. - Extracted
codexMcpSlug()intotool-view.ts— the livemcpToolCallcase and the rollout-replay path now share one slug builder instead of two copies of the template literal, andCODEX_PLUGIN_APPS_SERVERmoved along with it. - Kept codex's raw dotted title for
__-bearing server names —codexMcpSlugreturns${server}.${tool}when the server would mis-split, matchingmcpToolName's first-__split rule exactly. - Added parser coverage for both paths —
repo__prodcases incodex-mcp-tools.test.ts(live) andcodex-history.test.ts(replay), plus aWebSearchheader test and an updatedtoolCallHeaderSummaryexpectation.
I traced live-vs-replay title convergence input by input rather than trusting the tests: ordinary servers, the codex_apps dotted unwrap, the stray-trailing-__ namespace form, __-bearing server names, and codex_apps combined with a __-bearing app name all produce identical titles from both paths. The five touched test files pass locally, 73/73.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/presentation/ui/src/chat/tool-search.tsx:15
- When ToolSearch settles with prose (e.g., multi-line error details), rendering it in a plain
collapses newlines/whitespace, so the message can become hard to read or misleading. Preserve line breaks (similar to how other tool text is rendered) by using whitespace-pre-wrap (and optionally break-words).
if (names.length === 0) {
return message ? <p className="text-muted-foreground text-sm">{message}</p> : null;
}
794b935 to
176a75e
Compare
|
Your Claude subscription has hit its usage limit. It resets at 3:10am (UTC). Re-trigger Pullfrog after the reset, or add an Add repo secret → · Model settings → · Setup docs → · Ask in Discord →
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/presentation/ui/src/tool-utils.ts:240
hasToolBody()treats ToolSearch calls as body-less unlessrawOutput.messageis set. But ToolSearch failures (and even successful tool lists) are carried incontent/toolSearchPresentation().message|names, so the header becomes non-expandable and the ToolSearchResult/message can’t be shown fromToolCallItem.
if (toolSearchPresentation(toolCall)) {
return toolCallFailureMessage(toolCall) !== undefined;
}
return toolCallMetadata(toolCall).length > 0 || toolCallFailureMessage(toolCall) !== undefined;
…rch results inline
176a75e to
098729b
Compare
|
Your Claude subscription has hit its usage limit. It resets at 3:10am (UTC). Re-trigger Pullfrog after the reset, or add an Add repo secret → · Model settings → · Setup docs → · Ask in Discord →
|
lucas77778
left a comment
lucas77778
left a comment
There was a problem hiding this comment.
Latest-head review found one blocking history-identity issue; the UI acceptance path itself passes.
- Blocking — Codex MCP replay reconstructs raw identity from lossy callable names.
codexMcpToolName()treats rolloutfunction_call.namespace/nameas the original server/tool and strips a trailing__(packages/host/agent-adapter/src/native/codex/history-tools.ts:141-152). In Codex 0.144.1 those fields are model-visible callable identifiers: server/tool characters may be sanitized, collisions gain hashes, and the pair may be truncated to the 64-character Responses API limit. A valid custom server namedacme__is a concrete collision: live rendersacme__.tool, while replay readsmcp__acme__, strips the suffix, and rendersmcp__acme__tool, changing the server toacme.
Codex already persists an authoritative completed mcpToolCall item (paginated history) or legacy McpToolCallEnd, keyed by the same call ID and carrying raw server/tool. Please pre-index those records and use them for replay; keep function_call.namespace/name only as an explicitly lossy fallback without stripping trailing delimiters. Add cases for a trailing-__ server plus sanitized/truncated callable names.
- Non-blocking — multiline ToolSearch prose loses line breaks.
packages/presentation/ui/src/chat/tool-search.tsx:13-14renders the preserved message in a normal<p>, collapsing newlines from multi-line failures/notices. Addwhitespace-pre-wrap break-wordsand a two-line component test.
Verified on this head:
- The prior
hasToolBody()concern is a false alarm: normalized result content is checked first, so successful and content-backed failed ToolSearch rows remain expandable. - Generic uncounted searches retain the query fallback.
- Focused suite passes: 6 files / 78 tests.
- Mock UI acceptance passes for counted search, selected-tool identities, and zero-match messaging.
|
Resolved both findings from review 4905679467:
Implemented in |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/presentation/ui/src/chat/tool-result-content.ts:89
- toolSearchPresentation() splits on "\n" without trimming, so CRLF output ("\r\n") will leave a trailing "\r" on each line and fail TOOL_NAME_LINE_RE, causing ToolSearch name-per-line settles to be treated as prose messages.
const text = toolCallDisplayText(toolCall);
const lines = [...new Set(text.split('\n').filter((line) => line.length > 0))];
There was a problem hiding this comment.
ℹ️ No blocking issues — one dead-field nit inline.
Reviewed changes
Both of the round-three findings are fixed, and each ships a test that fails without it.
- Pre-indexed codex's authoritative MCP identity —
collectCodexMcpIdentities()(history.ts:557) walks the rollout once, keyingcall_id→ raw{server, tool}fromitem_completed/McpToolCallitems and legacymcp_tool_call_end.invocation.codexReplayMcpIdentity()now returnspersisted ?? callable, so the lossy callable is a genuine fallback rather than the primary source. - Removed the trailing-
__strip fromcodexMcpToolName, exactly as requested —mcp__acme__/lookupnow replays asacme__.lookup, matching the live path instead of silently renaming the server toacme. - Kept the one legacy Codex Apps case recoverable — a persisted
codex_apps/github_create_issuepair defers to the callable only when the namespace, the leading_, and the exact${server}_${tool}reconstruction all agree; anything else keeps the persisted identity verbatim. - Recovered Claude announce snapshots across cursor pages —
ClaudeTranscriptSupplementgainstoolUses, harvested from raw assistant rows, and the settle path falls back to it (announced.get(id) ?? toolUses?.get(id)) when a page begins at atool_resultwhosetool_uselived on the previous page.claudeToolCallFromUse()extracts the shared announce shape so both paths cannot drift. - Preserved multiline ToolSearch prose —
whitespace-pre-wrap break-wordson the message paragraph, with a two-line component test asserting both the exacttextContentand the classes.
On the 'McpToolCall' PascalCase check — verified correct, noting it so it isn't re-litigated
history.ts:567 tests 'McpToolCall' while the live adapter's item type is 'mcpToolCall', and PascalCase appears nowhere else in the repo except this code and its own fixtures — the shape of an assumption a test was written to agree with. It isn't. There are two codex protocols with different serde: the app-server ThreadItem is internally tagged camelCase (confirmed via codex app-server generate-json-schema), while the rollout's event_msg/item_completed payload carries TurnItem, an internally tagged enum whose variants serialize PascalCase — confirmed by dumping the vendored 0.144.1 binary, which yields …FileChangeMcpToolCallContextCompaction immediately adjacent to internally tagged enum TurnItem. I also checked that McpToolCallEndEvent really nests invocation { server, tool } and that McpToolCallItem exposes single-word id/server/tool, so both read paths are casing-safe.
Two more things I traced rather than trusting the tests: collectCodexMcpIdentities is order-independent by construction (item_completed sets unconditionally, mcp_tool_call_end is guarded by !has, so the authoritative completed item always wins whichever row lands first), and all three Codex Apps branches produce the expected titles by hand — legacy match, server/app mismatch, and modern dotted.
The six touched test files pass locally, 95/95.
Claude Opus ｜ 𝕏
| } | ||
|
|
||
| function harvestToolUses(toolUses: Map<string, ToolCall>, row: Record<string, unknown>): void { | ||
| const parentToolCallId = stringField(row, 'parent_tool_use_id'); |
There was a problem hiding this comment.
stringField(row, 'parent_tool_use_id') reads a snake_case key off a raw transcript row, but raw rows are camelCase — every sibling read in this function uses isMeta / isSidechain / teamName / parentUuid. snake_case is the SDK's SessionMessage projection shape, not the on-disk one.
I checked a real transcript rather than reasoning about it: zero rows in a ~/.claude/projects/<…>.jsonl carry a top-level parent_tool_use_id. The CLI's own name for the field is parentToolUseID, and its disk→stream projection hard-codes parent_tool_use_id: null — as does the rows.push projection 23 lines up at claude-code.ts:1825. So parentToolCallId here is always undefined in production.
It's harmless: the settle path degrades to parent ?? existing?.parentToolCallId, i.e. exactly the pre-PR behavior. It may even be unreachable by construction — sidechain rows, the only ones that would carry a parent tool id, are excluded on the line directly above. The reason it's worth a line: the new tests fabricate the key (claude-code-compaction.test.ts:473 and :535) and then assert parentToolCallId on the recovered ToolCall, so that one field is validated against a shape the adapter never actually sees.
Either drop the argument and let the settle fall back on its own, or read the camelCase key the transcript actually writes — but the fabricated field should come out of the fixtures either way, since the rest of both tests (the announce-snapshot recovery they exist to prove) is genuinely load-bearing.
Summary
Search-type tool calls get their own chat presentation (Closes CODE-412):
history-tools.ts) — normalize to the sharedmcp__<server>__<tool>slug so server identity survives into these rows.commit.gpgsign=falseso integration tests pass on signing machines.Verification
pnpm check:ci(0 errors) and fullpnpm exec vitest run(2741 passed) at this tip.tool-search.test.tsx,tool-call-metadata.test.tsx,tool-result-content.test.ts,codex-mcp-tools.test.ts,codex-history.test.ts.dev:mockconversation showcase.Checklist
pnpm check:ciandpnpm testboth pass (pluscargo fmt/clippy/testfor Rust changes)WIRE_PROTOCOL_VERSIONis bumped (no wire change in this PR)