diff --git a/src/main/codex/codex-structured-item-translation.test.ts b/src/main/codex/codex-structured-item-translation.test.ts index 18eb25682f2..a894d56bc98 100644 --- a/src/main/codex/codex-structured-item-translation.test.ts +++ b/src/main/codex/codex-structured-item-translation.test.ts @@ -305,7 +305,7 @@ describe('codex item bodies', () => { }) }) - it('gives an mcp tool call a typed body with its result as output', () => { + it('gives an mcp tool call a typed body with its own arguments as input', () => { expect( codexItemBody({ type: 'mcpToolCall', @@ -318,22 +318,63 @@ describe('codex item bodies', () => { }) ).toEqual({ kind: 'tool-call', - name: 'Get Forecast', - input: { server: 'weather', tool: 'get_forecast', arguments: { city: 'Oslo' } }, + // Server-qualified, and the arguments stay top level so the row label can + // read `query`/`command`/`file_path` out of them. + name: 'weather/get_forecast', + input: { city: 'Oslo' }, state: 'completed', output: { head: '12C', byteLength: 3, truncated: false, digest: expect.any(String) } }) }) - it('keeps a namespaced mcp tool name byte-identical', () => { - for (const tool of ['mcp__server__tool', 'server/tool', 'ns.tool', 'urn:tool']) { + it('passes an mcp tool name through with no casing transform', () => { + // Downstream dispatch is exact-match on raw identifiers, so every shape — + // bare snake_case included — has to survive byte-identical. + for (const tool of ['get_forecast', 'mcp__server__tool', 'ns.tool', 'urn:tool', 'listTools']) { expect( codexItemBody({ type: 'mcpToolCall', id: 'm', tool, status: 'inProgress' }), tool ).toMatchObject({ kind: 'tool-call', name: tool, state: 'running' }) + expect( + codexItemBody({ type: 'mcpToolCall', id: 'm', server: 'srv', tool, status: 'inProgress' }), + tool + ).toMatchObject({ kind: 'tool-call', name: `srv/${tool}`, state: 'running' }) } }) + it('falls back to the bare tool, then to `mcp`, when the item is under-specified', () => { + expect( + codexItemBody({ type: 'mcpToolCall', id: 'm', tool: 'get_forecast', status: 'inProgress' }) + ).toMatchObject({ name: 'get_forecast' }) + expect( + codexItemBody({ type: 'mcpToolCall', id: 'm', server: '', tool: 'ping', status: 'completed' }) + ).toMatchObject({ name: 'ping' }) + expect( + codexItemBody({ type: 'mcpToolCall', id: 'm', server: 'weather', status: 'completed' }) + ).toMatchObject({ name: 'mcp' }) + }) + + it('keeps non-object mcp arguments addressable and absent ones empty', () => { + // `arguments` is arbitrary JSON upstream; a scalar or array must still reach + // the row rather than being dropped or unwrapped into a bare value. + expect( + codexItemBody({ type: 'mcpToolCall', id: 'm', tool: 't', arguments: 'raw text' }) + ).toMatchObject({ input: { arguments: 'raw text' } }) + expect( + codexItemBody({ type: 'mcpToolCall', id: 'm', tool: 't', arguments: [1, 2] }) + ).toMatchObject({ input: { arguments: [1, 2] } }) + // Null, not `{}`: an empty object labels the row with a literal `{}`. + expect(codexItemBody({ type: 'mcpToolCall', id: 'm', tool: 't' })).toEqual({ + kind: 'tool-call', + name: 't', + input: null, + state: 'running' + }) + expect( + codexItemBody({ type: 'mcpToolCall', id: 'm', tool: 't', arguments: null }) + ).toMatchObject({ input: null }) + }) + it('reports an mcp error as a failed call carrying the server message', () => { expect( codexItemBody({ @@ -346,7 +387,7 @@ describe('codex item bodies', () => { }) ).toMatchObject({ kind: 'tool-call', - name: 'Ping', + name: 's/ping', state: 'failed', output: { head: 'server unreachable', truncated: false } }) @@ -379,6 +420,37 @@ describe('codex item bodies', () => { }) }) + it('carries the web search hits as the call output', () => { + const results = [{ title: 'Orca 1.0', url: 'https://example.com/notes' }] + expect( + codexItemBody({ + type: 'webSearch', + id: 'w', + query: 'orca release notes', + action: { type: 'search', query: 'orca release notes', queries: null }, + results + }) + ).toMatchObject({ + kind: 'tool-call', + name: 'web_search', + state: 'completed', + output: { head: JSON.stringify(results), truncated: false } + }) + // Nothing to show is no output block at all, not an empty one. + for (const empty of [undefined, null, []]) { + expect( + codexItemBody({ + type: 'webSearch', + id: 'w', + query: 'q', + action: { type: 'search' }, + results: empty + }), + String(empty) + ).not.toHaveProperty('output') + } + }) + it('leaves subagent items on the generic row until a real renderer exists', () => { expect( codexJournalItem({ diff --git a/src/main/codex/codex-structured-item-translation.ts b/src/main/codex/codex-structured-item-translation.ts index c3e63ff6976..b490618c4aa 100644 --- a/src/main/codex/codex-structured-item-translation.ts +++ b/src/main/codex/codex-structured-item-translation.ts @@ -225,32 +225,34 @@ function fileChangeItem(item: CodexThreadItem): CodexJournalItem { } } -/** MCP tool names are namespaced as often as not (`mcp__server__tool`, - * `server/tool`, `ns.tool`) and must reach the row byte-identical; only a bare - * word is title-cased. */ -function mcpToolDisplayName(name: string): string { - if (/[:./]|__/.test(name)) { - return name - } - const words = name.split(/[\s_-]+/).filter((word) => word.length > 0) - return words.length === 0 - ? name - : words.map((word) => word[0]!.toUpperCase() + word.slice(1)).join(' ') +/** The tool name reaches the row verbatim — downstream dispatch (diff renderer, + * question parsers, input previews) matches raw identifiers, so any casing + * transform would silently miss them. `server/` qualifies it so two servers + * exposing the same tool stay distinguishable and neither shadows a built-in. */ +function mcpToolCallName(item: CodexThreadItem): string { + const tool = readString(item, 'tool') + const server = readString(item, 'server') + return tool === null ? 'mcp' : server === null ? tool : `${server}/${tool}` +} + +/** Row-label derivation only reads top-level keys, so the call's own arguments + * have to be the input itself. `arguments` is arbitrary JSON upstream: a + * non-object stays addressable under a key rather than being dropped, and an + * absent one becomes null, which labels as empty instead of `{}`. */ +function mcpToolArguments(value: unknown): unknown { + const plain = typeof value === 'object' && value !== null && !Array.isArray(value) + return plain ? value : value === null || value === undefined ? null : { arguments: value } } function mcpToolCallItem(item: CodexThreadItem): CodexJournalItem { const failure = readString(readRecord(item.error), 'message') const text = failure ?? readTextContent(readRecord(item.result), 'content') const bounded = text === null ? null : boundInlineText(text, DEFAULT_JOURNAL_PAYLOAD_LIMITS) - const tool = readString(item, 'tool') return { body: { kind: 'tool-call', - name: tool === null ? 'mcp' : mcpToolDisplayName(tool), - input: boundToolInput( - { server: item.server ?? null, tool, arguments: item.arguments ?? null }, - DEFAULT_JOURNAL_PAYLOAD_LIMITS - ), + name: mcpToolCallName(item), + input: boundToolInput(mcpToolArguments(item.arguments), DEFAULT_JOURNAL_PAYLOAD_LIMITS), state: failure === null ? commandState(item) : 'failed', ...(bounded === null ? {} : { output: bounded.bounded }) }, @@ -259,8 +261,11 @@ function mcpToolCallItem(item: CodexThreadItem): CodexJournalItem { } /** `webSearch` carries no status: Codex starts it with an empty query and a null - * action, then completes it with both, so `action` is the completion signal. */ + * action, then completes it with both, so `action` is the completion signal. + * The hits arrive on `results` and are the call's output. */ function webSearchItem(item: CodexThreadItem): CodexJournalItem { + const hits = Array.isArray(item.results) && item.results.length > 0 ? item.results : null + const bounded = hits && boundInlineText(JSON.stringify(hits), DEFAULT_JOURNAL_PAYLOAD_LIMITS) return { body: { kind: 'tool-call', @@ -269,7 +274,8 @@ function webSearchItem(item: CodexThreadItem): CodexJournalItem { { query: readString(item, 'query'), action: item.action ?? null }, DEFAULT_JOURNAL_PAYLOAD_LIMITS ), - state: item.action === null || item.action === undefined ? 'running' : 'completed' + state: item.action === null || item.action === undefined ? 'running' : 'completed', + ...(bounded === null ? {} : { output: bounded.bounded }) }, handled: true }