From 8ebc6625308d6b76b98e7fdb2e8ce9c25aaee262 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 4 Aug 2026 02:23:25 -0700 Subject: [PATCH] fix(mobile): name search rows by their term and keep the filename in path labels (STA-3333) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups to the tool-row summary, all in the shared helper: - A Grep/Glob row labelled itself with the directory it scanned and dropped the pattern entirely, because `toolFilePath` treats `path` as a file target. That path is a scan root, so it also rendered a tap-to-open link that asked the app to open a folder. `toolFilePath` now ignores the generic `path` key for search-shaped input, which lets the pattern win the label and drops the bogus link; an explicit `file_path` still wins. - An overlong path was truncated from the head, cutting off the basename — the one part that tells two rows apart. Trim from the front instead, so the label reads `…/session/MobileNativeChatMessage.tsx`. - The primary-argument chain used `??`, so a present-but-blank key selected itself and swallowed the keys ranked after it, dropping the label all the way back to raw JSON. Take the first key that actually yields a label. Refs STA-3333. --- src/shared/native-chat-tool-summary.test.ts | 42 +++++++++++++ src/shared/native-chat-tool-summary.ts | 66 ++++++++++++++++----- 2 files changed, 93 insertions(+), 15 deletions(-) diff --git a/src/shared/native-chat-tool-summary.test.ts b/src/shared/native-chat-tool-summary.test.ts index cb710e6e71a..9ae5d1a99be 100644 --- a/src/shared/native-chat-tool-summary.test.ts +++ b/src/shared/native-chat-tool-summary.test.ts @@ -44,6 +44,48 @@ describe('describeToolInput', () => { expect(describeToolInput({ file_path: path }).length).toBeLessThanOrEqual(80) expect(describeToolInput({ file_path: path })).toContain('…') }) + + it('keeps the filename when trimming an overlong path', () => { + // Head-truncating an absolute path drops the basename — the one part that + // tells two rows apart — so the trim has to come off the front. + const path = `/Users/me/orca/workspaces/orca/sta-3333/src/shared/${'nested/'.repeat(4)}app.tsx` + const label = describeToolInput({ file_path: path, offset: 10 }) + expect(label.length).toBeLessThanOrEqual(80) + expect(label.startsWith('…')).toBe(true) + expect(label.endsWith('/app.tsx')).toBe(true) + }) + + it('distinguishes two long paths that share a deep prefix', () => { + const base = '/Users/me/orca/workspaces/orca/sta-3333-tool-summary/src/session/native' + const a = describeToolInput({ file_path: `${base}/MobileNativeChatMessage.tsx` }) + const b = describeToolInput({ file_path: `${base}/MobileNativeChatComposer.tsx` }) + expect(a).not.toBe(b) + }) + + it('names a search call by its pattern, not the directory it scanned', () => { + // `path` on a Grep/Glob is the scan root; labelling with it loses the term. + expect(describeToolInput({ pattern: 'summarizeToolInput', path: 'src/shared' })).toBe( + 'summarizeToolInput' + ) + expect(describeToolInput({ pattern: '**/*.tsx', path: 'mobile/src' })).toBe('**/*.tsx') + expect(describeToolInput({ query: 'auth flow', path: 'src' })).toBe('auth flow') + // ...and offers no open-file link, since that path is a folder. + expect(toolFilePath({ pattern: 'x', path: 'src/shared' })).toBeNull() + expect(briefToolArg({ pattern: 'x', path: 'src/shared' })).toBe('x') + }) + + it('still treats an explicit file target as one alongside a search term', () => { + expect(toolFilePath({ pattern: 'x', file_path: 'src/a.ts' })).toBe('src/a.ts') + expect(toolFilePath({ path: 'src/c.ts' })).toBe('src/c.ts') + }) + + it('skips a present-but-blank key instead of falling through to raw JSON', () => { + expect(describeToolInput({ command: '', query: 'needle' })).toBe('needle') + expect(describeToolInput({ command: ' ', url: 'https://example.com' })).toBe( + 'https://example.com' + ) + expect(briefToolArg({ cmd: '', query: 'needle' })).toBe('needle') + }) }) describe('Codex JSON-string tool arguments', () => { diff --git a/src/shared/native-chat-tool-summary.ts b/src/shared/native-chat-tool-summary.ts index 251963f5c07..b625373a242 100644 --- a/src/shared/native-chat-tool-summary.ts +++ b/src/shared/native-chat-tool-summary.ts @@ -5,6 +5,8 @@ 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 export function summarizeToolInput(input: unknown): string { const collapsed = toRawPreview(input).replace(/\s+/g, ' ').trim() @@ -20,16 +22,13 @@ export function describeToolInput(input: unknown): string { const normalized = normalizeToolInput(input) const path = toolFilePath(normalized) if (path) { - return summarizeToolInput(path) + return summarizeToolPath(path) } if (normalized && typeof normalized === 'object') { - const value = normalized as Record // Concrete target/action first; prose `description` only as a last resort. - const primary = - value.command ?? value.cmd ?? value.query ?? value.pattern ?? value.url ?? value.description - const primarySummary = summarizePrimaryToolArg(primary) - if (primarySummary) { - return primarySummary + const primary = firstPrimaryToolArg(normalized as Record, PRIMARY_ARG_KEYS) + if (primary) { + return primary } } return summarizeToolInput(normalized) @@ -70,23 +69,24 @@ export function toolFilePath(input: unknown): string | null { return null } const value = normalized as Record - const path = value.file_path ?? value.filePath ?? value.path ?? value.notebook_path + // A search call's `path` is the directory it scanned, not a file it opened — + // taking it would drop the pattern from the label and offer a link to a folder. + const directory = isSearchToolInput(value) ? undefined : value.path + const path = value.file_path ?? value.filePath ?? directory ?? value.notebook_path return typeof path === 'string' && path.length > 0 ? path : null } export function briefToolArg(input: unknown): string { const normalized = normalizeToolInput(input) if (normalized && typeof normalized === 'object') { - const value = normalized as Record - const path = value.file_path ?? value.filePath ?? value.path ?? value.notebook_path - if (typeof path === 'string' && path.length > 0) { + const path = toolFilePath(normalized) + if (path) { const parts = path.split(/[\\/]/).filter(Boolean) return parts.at(-1) ?? path } - const command = value.command ?? value.cmd ?? value.query ?? value.pattern - const commandSummary = summarizePrimaryToolArg(command) - if (commandSummary) { - return commandSummary.slice(0, 28) + const command = firstPrimaryToolArg(normalized as Record, BRIEF_ARG_KEYS) + if (command) { + return command.slice(0, 28) } } return summarizeToolInput(normalized).slice(0, 28) @@ -110,6 +110,42 @@ function normalizeToolInput(input: unknown): unknown { } } +/** A search call is named by what it looked for, so its `path` is a scan root + * rather than a file target. */ +function isSearchToolInput(value: Record): boolean { + return ( + summarizePrimaryToolArg(value.query) !== null || summarizePrimaryToolArg(value.pattern) !== null + ) +} + +/** The first key that yields a label — a present-but-blank key must not swallow + * the keys ranked after it. */ +function firstPrimaryToolArg( + value: Record, + keys: readonly string[] +): string | null { + for (const key of keys) { + const summary = summarizePrimaryToolArg(value[key]) + if (summary) { + return summary + } + } + return null +} + +/** A path is identified by its tail, so trim from the front: head-truncating an + * absolute path drops the filename, the one part that tells two rows apart. */ +function summarizeToolPath(path: string): string { + const collapsed = path.replace(/\s+/g, ' ').trim() + if (collapsed.length <= MAX_PREVIEW_LENGTH) { + return collapsed + } + const tail = collapsed.slice(collapsed.length - (MAX_PREVIEW_LENGTH - 1)) + // Start at a segment boundary so the label doesn't open mid-name. + const boundary = tail.search(/[\\/]/) + return `…${boundary > 0 ? tail.slice(boundary) : tail}` +} + /** A label-worthy primary argument: a non-blank string, or an argv array. */ function summarizePrimaryToolArg(input: unknown): string | null { if (typeof input === 'string' && input.trim()) {