From 67cc962cb57d871014253d6814fd1a2a6d070183 Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Fri, 4 Sep 2026 17:33:39 -0700 Subject: [PATCH] feat(native-chat): label Codex tool rows by what the command actually did Codex's app-server `commandExecution` item carries `commandActions`, which already classifies each command as a read, a search, or a directory listing with the target path, name, or query extracted. Orca ignored the field, so every shell call rendered as an undifferentiated row of raw argv. Read it and name the row by its class, keeping the raw command and cwd for the expanded view. Unclassified commands are untouched: absent, null, or malformed `commandActions` produces byte-identical output to before. Rank the search term above the command in the shared label keys so a classified search row reads by what it looked for rather than the shell text that ran it. No first-party tool input carries both keys today, so this only reaches the new rows; an MCP tool supplying both would prefer its search term. Note `commandActions` is the app-server spelling. `parsedCmd` is the rollout-file shape and never arrives on this lane; a test pins that it stays ignored. --- .../codex-structured-item-translation.test.ts | 146 ++++++++++++++++++ .../codex-structured-item-translation.ts | 47 +++++- src/shared/native-chat-tool-summary.test.ts | 20 +++ src/shared/native-chat-tool-summary.ts | 6 +- 4 files changed, 215 insertions(+), 4 deletions(-) diff --git a/src/main/codex/codex-structured-item-translation.test.ts b/src/main/codex/codex-structured-item-translation.test.ts index f0f843e4ed6..16fff5455f4 100644 --- a/src/main/codex/codex-structured-item-translation.test.ts +++ b/src/main/codex/codex-structured-item-translation.test.ts @@ -195,6 +195,152 @@ describe('codex item bodies', () => { }) }) + it('names a classified read command by its class and keeps the raw command', () => { + expect( + codexItemBody({ + type: 'commandExecution', + id: 'item-read', + command: "sed -n '1,200p' notes.txt", + cwd: '/repo', + status: 'completed', + exitCode: 0, + commandActions: [ + { + type: 'read', + command: "sed -n '1,200p' notes.txt", + name: 'notes.txt', + path: '/repo/notes.txt' + } + ] + }) + ).toEqual({ + kind: 'tool-call', + name: 'read', + input: { + command: "sed -n '1,200p' notes.txt", + cwd: '/repo', + path: '/repo/notes.txt', + name: 'notes.txt' + }, + state: 'completed' + }) + }) + + it('carries a classified search query so the row labels by term, not scan root', () => { + expect( + codexItemBody({ + type: 'commandExecution', + id: 'item-search', + command: 'rg -n --no-heading beta .', + cwd: '/repo', + status: 'inProgress', + commandActions: [ + { type: 'search', command: 'rg -n --no-heading beta .', query: 'beta', path: '.' } + ] + }) + ).toEqual({ + kind: 'tool-call', + name: 'search', + input: { command: 'rg -n --no-heading beta .', cwd: '/repo', query: 'beta', path: '.' }, + state: 'running' + }) + }) + + it('omits a null classified field rather than standing it in as a target', () => { + expect( + codexItemBody({ + type: 'commandExecution', + id: 'item-search-bare', + command: 'rg beta', + cwd: '/repo', + status: 'completed', + exitCode: 0, + commandActions: [{ type: 'search', command: 'rg beta', query: null, path: null }] + }) + ).toEqual({ + kind: 'tool-call', + name: 'search', + input: { command: 'rg beta', cwd: '/repo' }, + state: 'completed' + }) + }) + + it('names a classified listFiles command `list`', () => { + expect( + codexItemBody({ + type: 'commandExecution', + id: 'item-list', + command: 'ls', + cwd: '/repo', + status: 'completed', + exitCode: 0, + commandActions: [{ type: 'listFiles', command: 'ls', path: null }] + }) + ).toEqual({ + kind: 'tool-call', + name: 'list', + input: { command: 'ls', cwd: '/repo' }, + state: 'completed' + }) + }) + + it('skips unclassified actions to reach the first classified one', () => { + expect( + codexItemBody({ + type: 'commandExecution', + id: 'item-piped', + command: 'true && cat a.ts', + cwd: '/repo', + status: 'completed', + exitCode: 0, + commandActions: [ + { type: 'unknown', command: 'true' }, + { type: 'read', command: 'cat a.ts', name: 'a.ts', path: 'a.ts' } + ] + }) + ).toMatchObject({ name: 'read', input: { path: 'a.ts', name: 'a.ts' } }) + }) + + it('falls back to the unclassified shell row for absent or malformed commandActions', () => { + const shellRow = { + kind: 'tool-call', + name: 'shell', + input: { command: 'ls', cwd: '/tmp' }, + state: 'completed' + } + const base = { + type: 'commandExecution', + id: 'item-fallback', + command: 'ls', + cwd: '/tmp', + status: 'completed', + exitCode: 0 + } + + expect(codexItemBody(base)).toEqual(shellRow) + expect(codexItemBody({ ...base, commandActions: null })).toEqual(shellRow) + expect(codexItemBody({ ...base, commandActions: [] })).toEqual(shellRow) + expect( + codexItemBody({ ...base, commandActions: [{ type: 'unknown', command: 'ls' }] }) + ).toEqual(shellRow) + expect(codexItemBody({ ...base, commandActions: 'read' })).toEqual(shellRow) + expect(codexItemBody({ ...base, commandActions: [null, 7, 'read', {}, { type: 5 }] })).toEqual( + shellRow + ) + // The classification table is a Map because an object index answers + // `__proto__`/`constructor` with a truthy non-string tool name. + expect( + codexItemBody({ ...base, commandActions: [{ type: '__proto__', command: 'ls' }] }) + ).toEqual(shellRow) + expect( + codexItemBody({ ...base, commandActions: [{ type: 'constructor', command: 'ls' }] }) + ).toEqual(shellRow) + // The rollout-file shape is a different lane and never reaches app-server. + expect( + codexItemBody({ ...base, parsedCmd: [{ type: 'read', cmd: 'ls', path: 'a.ts' }] }) + ).toEqual(shellRow) + }) + it('accepts snake-case command completion output and preserves blob evidence', () => { const output = 'x'.repeat(1_100_000) const translated = codexJournalItem({ diff --git a/src/main/codex/codex-structured-item-translation.ts b/src/main/codex/codex-structured-item-translation.ts index 19052dca365..e82ba7a3d23 100644 --- a/src/main/codex/codex-structured-item-translation.ts +++ b/src/main/codex/codex-structured-item-translation.ts @@ -175,15 +175,58 @@ export type CodexJournalItem = { handled: boolean } +/** + * Codex's own classification of a shell call: the tool name to show, and the + * fields worth lifting into `input` for the shared label helper (`path` as a + * target, `query` as a search term, `name` as the read target's basename). + * A `Map`, not an object — an object index answers `__proto__` with a truthy + * non-string. Every other action type stays an unclassified `shell` row. + */ +const COMMAND_ACTION_CLASSES = new Map([ + ['read', { name: 'read', keys: ['path', 'name'] }], + ['search', { name: 'search', keys: ['query', 'path'] }], + ['listFiles', { name: 'list', keys: ['path'] }] +]) + +/** The first classified `commandActions` entry; null leaves the row exactly as + * a Codex that sends no classification renders it. */ +function commandActionFacts( + item: CodexThreadItem +): { name: string; fields: Record } | null { + const actions = item.commandActions + if (!Array.isArray(actions)) { + return null + } + for (const action of actions) { + const record = readRecord(action) + const type = readString(record, 'type') + const classified = type === null ? undefined : COMMAND_ACTION_CLASSES.get(type) + if (classified === undefined) { + continue + } + const fields: Record = {} + for (const key of classified.keys) { + const value = readString(record, key) + if (value !== null) { + fields[key] = value + } + } + return { name: classified.name, fields } + } + return null +} + function commandItem(item: CodexThreadItem): CodexJournalItem { const output = readFirstString(item, ['aggregatedOutput', 'aggregated_output']) const bounded = output === null ? null : boundInlineText(output, DEFAULT_JOURNAL_PAYLOAD_LIMITS) + const parsed = commandActionFacts(item) return { body: { kind: 'tool-call', - name: 'shell', + name: parsed?.name ?? 'shell', + // Raw command and cwd stay so the expanded view still shows what ran. input: boundToolInput( - { command: item.command ?? null, cwd: item.cwd ?? null }, + { command: item.command ?? null, cwd: item.cwd ?? null, ...parsed?.fields }, DEFAULT_JOURNAL_PAYLOAD_LIMITS ), state: commandState(item), diff --git a/src/shared/native-chat-tool-summary.test.ts b/src/shared/native-chat-tool-summary.test.ts index 57cc5eff074..5fad8fedaef 100644 --- a/src/shared/native-chat-tool-summary.test.ts +++ b/src/shared/native-chat-tool-summary.test.ts @@ -133,6 +133,26 @@ describe('describeToolInput', () => { 'https://example.com' ) expect(briefToolArg({ cmd: '', query: 'needle' })).toBe('needle') + // Inverted, so the skip is still exercised now that the search keys rank first. + expect(describeToolInput({ query: '', command: 'git status' })).toBe('git status') + expect(briefToolArg({ pattern: ' ', cmd: 'git status' })).toBe('git status') + }) + + it('labels a classified search row by its term, not the command that ran it', () => { + // Codex `commandActions` rows are the only input carrying both keys: the + // search term identifies the row, the raw command stays for the detail view. + const search = { command: 'rg -n --no-heading beta .', cwd: '/repo', query: 'beta', path: '.' } + + expect(describeToolInput(search)).toBe('beta') + expect(briefToolArg(search)).toBe('beta') + expect(toolFilePath(search)).toBeNull() + }) + + it('leaves a command-only input labelled by its command', () => { + // Bash and Codex's unclassified shell rows carry no search key at all. + expect(describeToolInput({ command: 'pnpm test', description: 'Run tests' })).toBe('pnpm test') + expect(briefToolArg({ command: 'pnpm test' })).toBe('pnpm test') + expect(describeToolInput({ cmd: 'git status --short' })).toBe('git status --short') }) }) diff --git a/src/shared/native-chat-tool-summary.ts b/src/shared/native-chat-tool-summary.ts index 9954521bed4..9ab3003938c 100644 --- a/src/shared/native-chat-tool-summary.ts +++ b/src/shared/native-chat-tool-summary.ts @@ -5,8 +5,10 @@ const MAX_PREVIEW_STRING_INPUT = 160 const MAX_PREVIEW_COLLECTION_ITEMS = 8 const MAX_PREVIEW_DEPTH = 2 const MAX_TOOL_RUN_SUMMARY_PARTS = 3 -const PRIMARY_ARG_KEYS = ['command', 'cmd', 'query', 'pattern', 'url', 'description'] as const -const BRIEF_ARG_KEYS = ['command', 'cmd', 'query', 'pattern'] as const +// Search term before command: a classified search row carries both, and the +// term is what identifies it. No other tool input supplies the two together. +const PRIMARY_ARG_KEYS = ['query', 'pattern', 'command', 'cmd', 'url', 'description'] as const +const BRIEF_ARG_KEYS = ['query', 'pattern', 'command', 'cmd'] as const export const MAX_TOOL_DETAIL_LENGTH = 4000 export type ToolInputDisplay = {