diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index e0ff5ad9a9f..8cb8dc758a3 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -249,6 +249,7 @@ jobs: src/main/terminal-history-fish-session.node-pty.test.ts \ src/renderer/src/components/terminal-pane/fish-color-scheme-child-stdin.node-pty.test.ts \ src/shared/fish-draft-prefill-teardown.node-pty.test.ts \ + src/shared/fish-startup-arg-quoting.live-fish.test.ts \ src/shared/posix-command-path-lookup.test.ts test: @@ -287,6 +288,7 @@ jobs: --exclude=src/main/terminal-history-fish-session.node-pty.test.ts \ --exclude=src/renderer/src/components/terminal-pane/fish-color-scheme-child-stdin.node-pty.test.ts \ --exclude=src/shared/fish-draft-prefill-teardown.node-pty.test.ts \ + --exclude=src/shared/fish-startup-arg-quoting.live-fish.test.ts \ --exclude=src/shared/posix-command-path-lookup.test.ts \ --exclude=tests/e2e/cross-version-wire/** \ --shard=${{ matrix.shard }}/${{ matrix.shard_total }} diff --git a/src/main/fish-history-session.ts b/src/main/fish-history-session.ts index 862c9d01fdf..d24119450d1 100644 --- a/src/main/fish-history-session.ts +++ b/src/main/fish-history-session.ts @@ -1,6 +1,6 @@ import { homedir } from 'node:os' -import { isAbsolute, join } from 'node:path' -import { rmSync } from 'node:fs' +import { basename, isAbsolute, join } from 'node:path' +import { existsSync, rmSync } from 'node:fs' /** * Per-worktree fish history, which fish models as a session NAME rather than a path. @@ -45,17 +45,62 @@ export function resolveFishHistoryFilePath( return join(dataHome, 'fish', `${session}_history`) } -/** Best-effort removal of one worktree's fish history file. */ -export function deleteFishHistoryFile(session: string, env: NodeJS.ProcessEnv = process.env): void { - const path = resolveFishHistoryFilePath(session, env) - if (!path) { +/** + * Accepts a path recorded at spawn time only if it still names this session's own + * history file, so a tampered meta.json cannot steer `rmSync` somewhere else. + */ +function isTrustedRecordedPath(session: string, path: string | null | undefined): path is string { + return Boolean( + path && + SAFE_SESSION_NAME.test(session) && + isAbsolute(path) && + basename(path) === `${session}_history` + ) +} + +/** + * Removes one worktree's fish history file. + * + * `recordedPath` is the path resolved from the PTY's own spawn env when the + * session was minted; the main process env is only a fallback, because the two + * disagree whenever Orca was launched with a different `XDG_DATA_HOME`/`HOME` + * than the shells it spawns. + * + * Neither can see a `set -gx XDG_DATA_HOME …` that lives inside the user's + * config.fish — fish resolves that after we have handed off. That file cannot be + * found from here, so it is reported instead of being dropped silently. + */ +export function deleteFishHistoryFile( + session: string, + options: { recordedPath?: string | null; env?: NodeJS.ProcessEnv } = {} +): void { + const candidates = new Set() + if (isTrustedRecordedPath(session, options.recordedPath)) { + candidates.add(options.recordedPath) + } + const fromEnv = resolveFishHistoryFilePath(session, options.env ?? process.env) + if (fromEnv) { + candidates.add(fromEnv) + } + if (candidates.size === 0) { return } - try { - rmSync(path, { force: true }) - } catch (err) { + let removed = false + for (const path of candidates) { + try { + removed ||= existsSync(path) + rmSync(path, { force: true }) + } catch (err) { + console.warn( + `[pty:history] Failed to delete fish history ${path}: ${err instanceof Error ? err.message : String(err)}` + ) + } + } + if (!removed) { + // Explicit: a config.fish-set XDG_DATA_HOME leaves a file we cannot locate, + // and silence would hide one leaked history file per deleted worktree. console.warn( - `[pty:history] Failed to delete fish history ${session}: ${err instanceof Error ? err.message : String(err)}` + `[pty:history] No fish history file found for session ${session}; if fish keeps history outside ${[...candidates].join(' or ')} (e.g. XDG_DATA_HOME set in config.fish) that file is left behind.` ) } } diff --git a/src/main/terminal-history-deletion.ts b/src/main/terminal-history-deletion.ts index 5db4e04d7e2..8300a839fd2 100644 --- a/src/main/terminal-history-deletion.ts +++ b/src/main/terminal-history-deletion.ts @@ -97,9 +97,11 @@ function scheduleHistoryTreeRemoval(dir: string): void { export function scheduleWorktreeHistoryTreeDeletion(dir: string, historyRoot: string): boolean { // Why first: fish keeps its history in the user's fish data dir, outside this tree, // so the meta.json naming the session must still be readable when we look it up. - const fishSession = readHistoryMeta(dir)?.fishSession - if (fishSession) { - deleteFishHistoryFile(fishSession) + const meta = readHistoryMeta(dir) + if (meta?.fishSession) { + deleteFishHistoryFile(meta.fishSession, { + recordedPath: meta.fishHistoryPath + }) } const tombstone = tombstoneHistoryTree(dir, historyRoot) if (!tombstone) { diff --git a/src/main/terminal-history.test.ts b/src/main/terminal-history.test.ts index 248ad645506..31e1744c0a4 100644 --- a/src/main/terminal-history.test.ts +++ b/src/main/terminal-history.test.ts @@ -247,6 +247,32 @@ describe('terminal-history', () => { ) }) + it('records the history path from the SPAWN env, not this process env', () => { + // Pinned so the assertion cannot accidentally read the developer's own value. + const originalDataHome = process.env.XDG_DATA_HOME + process.env.XDG_DATA_HOME = ['', 'main', 'process', 'data'].join(sep) + try { + const env: Record = { + XDG_DATA_HOME: ['', 'spawn', 'data'].join(sep) + } + const result = injectHistoryEnv(env, 'repo-1::/path/wt', '/usr/bin/fish', '/path/wt') + const meta = JSON.parse(writeFileSyncMock.mock.calls.at(-1)?.[1] as string) as Record< + string, + string + > + + expect(meta.fishHistoryPath).toBe( + ['', 'spawn', 'data', 'fish', `${result.fishSession}_history`].join(sep) + ) + } finally { + if (originalDataHome === undefined) { + delete process.env.XDG_DATA_HOME + } else { + process.env.XDG_DATA_HOME = originalDataHome + } + } + }) + it('gives two fish worktrees different history sessions', () => { const envA: Record = {} injectHistoryEnv(envA, 'repo-1::/path/wt-a', '/usr/bin/fish', '/path/wt-a') @@ -387,6 +413,79 @@ describe('terminal-history', () => { await flushPendingWorktreeHistoryDeletions() }) + it('prefers the spawn-env path recorded in meta.json over this process env', async () => { + const originalDataHome = process.env.XDG_DATA_HOME + process.env.XDG_DATA_HOME = ['', 'main', 'data'].join(sep) + const recorded = ['', 'spawn', 'data', 'fish', 'orca_abc123_history'].join(sep) + try { + existsSyncMock.mockReturnValue(true) + readFileSyncMock.mockReturnValue( + JSON.stringify({ + worktreeId: 'repo-1::/path/wt', + fishSession: 'orca_abc123', + fishHistoryPath: recorded + }) + ) + deleteWorktreeHistoryDir('repo-1::/path/wt') + expect(rmSyncMock).toHaveBeenCalledWith(recorded, expect.objectContaining({ force: true })) + // The main-process guess stays as a fallback for pre-existing meta.json files. + expect(rmSyncMock).toHaveBeenCalledWith( + ['', 'main', 'data', 'fish', 'orca_abc123_history'].join(sep), + expect.objectContaining({ force: true }) + ) + } finally { + if (originalDataHome === undefined) { + delete process.env.XDG_DATA_HOME + } else { + process.env.XDG_DATA_HOME = originalDataHome + } + } + await flushPendingWorktreeHistoryDeletions() + }) + + it('refuses a recorded path that does not name its own session file', async () => { + const originalDataHome = process.env.XDG_DATA_HOME + process.env.XDG_DATA_HOME = ['', 'main', 'data'].join(sep) + try { + existsSyncMock.mockReturnValue(true) + readFileSyncMock.mockReturnValue( + JSON.stringify({ + worktreeId: 'repo-1::/path/wt', + fishSession: 'orca_abc123', + fishHistoryPath: ['', 'etc', 'passwd'].join(sep) + }) + ) + deleteWorktreeHistoryDir('repo-1::/path/wt') + expect(rmSyncMock).not.toHaveBeenCalledWith( + ['', 'etc', 'passwd'].join(sep), + expect.anything() + ) + } finally { + if (originalDataHome === undefined) { + delete process.env.XDG_DATA_HOME + } else { + process.env.XDG_DATA_HOME = originalDataHome + } + } + await flushPendingWorktreeHistoryDeletions() + }) + + it('warns instead of going quiet when no fish history file is found', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + try { + readFileSyncMock.mockReturnValue( + JSON.stringify({ worktreeId: 'repo-1::/path/wt', fishSession: 'orca_abc123' }) + ) + // The history dir exists (so the tree is deleted) but the fish file does not. + existsSyncMock.mockImplementation((path: string) => !path.includes('orca_abc123_history')) + deleteWorktreeHistoryDir('repo-1::/path/wt') + expect(warn).toHaveBeenCalledWith(expect.stringContaining('No fish history file found')) + } finally { + warn.mockRestore() + } + await flushPendingWorktreeHistoryDeletions() + }) + it('leaves the live directory alone when the tombstone rename fails', async () => { existsSyncMock.mockReturnValue(true) renameSyncMock.mockImplementation(() => { diff --git a/src/main/terminal-history.ts b/src/main/terminal-history.ts index 74a5c4fe22b..ad75835c3fe 100644 --- a/src/main/terminal-history.ts +++ b/src/main/terminal-history.ts @@ -1,6 +1,6 @@ import { join, basename } from 'node:path' import { mkdirSync, existsSync, readFileSync, writeFileSync } from 'node:fs' -import { fishHistorySessionName } from './fish-history-session' +import { fishHistorySessionName, resolveFishHistoryFilePath } from './fish-history-session' import { parseWslPath, toLinuxPath } from './wsl' import { getHistoryRoot, getHistoryRootWsl, hashWorktreeId } from './terminal-history-paths' @@ -74,11 +74,20 @@ export function ensureHistoryDir(worktreeHash: string, wslDistro?: string): stri /** Write meta.json alongside history files, for debuggability and for GC. * `fishSession` is load-bearing, not diagnostic: fish history lives outside this * directory, so deletion can only find it by the name recorded here. */ -function writeMetaFile(dir: string, worktreeId: string, fishSession?: string): void { +function writeMetaFile( + dir: string, + worktreeId: string, + fish?: { session: string; historyPath: string | null } +): void { try { const metaPath = join(dir, 'meta.json') const existing = existsSync(metaPath) ? readHistoryMeta(dir) : null - if (existing && (!fishSession || existing.fishSession === fishSession)) { + if ( + existing && + (!fish || + (existing.fishSession === fish.session && + existing.fishHistoryPath === (fish.historyPath ?? undefined))) + ) { return } writeFileSync( @@ -86,7 +95,8 @@ function writeMetaFile(dir: string, worktreeId: string, fishSession?: string): v JSON.stringify({ worktreeId, createdAt: existing?.createdAt ?? new Date().toISOString(), - ...(fishSession ? { fishSession } : {}) + ...(fish ? { fishSession: fish.session } : {}), + ...(fish?.historyPath ? { fishHistoryPath: fish.historyPath } : {}) }), { mode: 0o600 } ) @@ -100,6 +110,8 @@ export type HistoryDirMeta = { createdAt?: string /** fish session name whose history file lives in the user's fish data dir. */ fishSession?: string + /** That file's path resolved from the PTY's spawn env, which the main process env can contradict. */ + fishHistoryPath?: string } /** Read one history directory's meta.json, or null when it is absent or unparseable. */ @@ -173,7 +185,12 @@ export function injectHistoryEnv( // find the session file fish keeps in its own data dir. fish never runs as the // inner WSL shell, so histDir needs no /mnt conversion here. const session = fishHistorySessionName(worktreeHash) - writeMetaFile(histDir, worktreeId, session) + // Resolve from the SPAWN env: that is the XDG_DATA_HOME/HOME fish will see, + // which need not match the one this process was launched with. + writeMetaFile(histDir, worktreeId, { + session, + historyPath: resolveFishHistoryFilePath(session, spawnEnv) + }) spawnEnv.fish_history = session result.fishSession = session result.historyDir = histDir diff --git a/src/shared/agent-resume-argv-drop.ts b/src/shared/agent-resume-argv-drop.ts index df8c16aae4f..55134fe45a4 100644 --- a/src/shared/agent-resume-argv-drop.ts +++ b/src/shared/agent-resume-argv-drop.ts @@ -13,7 +13,9 @@ export type AgentResumeArgvDrop = | { status: 'absent' } | { status: 'unrecognized' } -const RESUME_ARGV_SHELLS: readonly AgentStartupShell[] = ['posix', 'powershell', 'cmd'] +// fish is listed because its quoting diverges from sh; without it a fish-quoted +// suffix reads as `unrecognized` and the caller refuses the launch outright. +const RESUME_ARGV_SHELLS: readonly AgentStartupShell[] = ['posix', 'fish', 'powershell', 'cmd'] function resumeArgvSuffixCandidates(resumeArgs: readonly string[]): string[] { const candidates = new Set([resumeArgs.join(' ')]) diff --git a/src/shared/agent-resume-launch-command.ts b/src/shared/agent-resume-launch-command.ts index 1093735d266..cda40878c28 100644 --- a/src/shared/agent-resume-launch-command.ts +++ b/src/shared/agent-resume-launch-command.ts @@ -1,5 +1,6 @@ import type { ResumableTuiAgent } from './agent-session-resume' import { + isPosixStartupShell, quoteStartupArg, tokenizeStartupCommand, type AgentStartupShell @@ -37,9 +38,9 @@ function findClaudeExecutableIndex(tokens: readonly string[], shell: AgentStartu return i } if ( - // Why: `NAME=value cmd` is posix-only syntax; on cmd/PowerShell such a - // token is just a bogus executable name, not a prefix to skip. - (shell === 'posix' && /^[A-Za-z_][A-Za-z0-9_]*=/.test(token)) || + // Why: `NAME=value cmd` is sh-family syntax (fish included, 3.1+); on + // cmd/PowerShell such a token is a bogus executable name, not a prefix. + (isPosixStartupShell(shell) && /^[A-Za-z_][A-Za-z0-9_]*=/.test(token)) || (shell === 'powershell' && token === '&' && i === 0) ) { continue diff --git a/src/shared/ai-vault-resume-command.ts b/src/shared/ai-vault-resume-command.ts index 13c33b8145e..9e029456ec3 100644 --- a/src/shared/ai-vault-resume-command.ts +++ b/src/shared/ai-vault-resume-command.ts @@ -72,7 +72,7 @@ export function buildAiVaultResumeShellCommand(args: { }) } - const resumeCommand = `${codexHomeEnvPrefix(codexHome?.trim() || null, platform)}${ + const resumeCommand = `${codexHomeEnvPrefix(codexHome?.trim() || null, platform, shell)}${ args.resumeCommand }` if (platform === 'win32' && shell === 'cmd') { @@ -89,7 +89,7 @@ export function buildAiVaultResumeShellCommand(args: { return `cmd /d /s /c ${quoteWindowsCmdArg(inner)}` } - return `cd ${quoteShellArg(cwd, platform)} && ${resumeCommand}` + return `cd ${quoteResumeArg(cwd, platform, shell)} && ${resumeCommand}` } function buildResumeShellCommandForShell(args: { @@ -184,14 +184,28 @@ function buildAgentResumeInvocation( } } -function codexHomeEnvPrefix(codexHome: string | null, platform: NodeJS.Platform): string { +function codexHomeEnvPrefix( + codexHome: string | null, + platform: NodeJS.Platform, + shell?: AgentStartupShell +): string { if (!codexHome) { return '' } if (platform === 'win32') { return `set ${quoteWindowsCmdArg(`CODEX_HOME=${codexHome}`)} && ` } - return `CODEX_HOME=${quoteShellArg(codexHome, platform)} ` + // fish accepts the `NAME=value cmd` prefix (3.1+), but not sh's quoting. + return `CODEX_HOME=${quoteResumeArg(codexHome, platform, shell)} ` +} + +/** Quotes for the live shell when one is known, else for the platform's default. */ +function quoteResumeArg( + value: string, + platform: NodeJS.Platform, + shell?: AgentStartupShell +): string { + return shell ? quoteStartupArg(value, shell) : quoteShellArg(value, platform) } function quoteShellArg(value: string, platform: NodeJS.Platform): string { diff --git a/src/shared/fish-command-tokenizer.test.ts b/src/shared/fish-command-tokenizer.test.ts new file mode 100644 index 00000000000..e8e818b3136 --- /dev/null +++ b/src/shared/fish-command-tokenizer.test.ts @@ -0,0 +1,86 @@ +import { describe, expect, it } from 'vitest' +import { tokenizeFishStartupCommand } from './fish-command-tokenizer' +import { tokenizeStartupCommand } from './tui-agent-startup-shell' + +function tokens(value: string): string[] { + const result = tokenizeFishStartupCommand(value) + if (!result.ok) { + throw new Error(result.error) + } + return result.tokens +} + +function spanFlags(value: string): boolean[] { + const result = tokenizeFishStartupCommand(value) + if (!result.ok) { + throw new Error(result.error) + } + return result.spans.map((span) => span.divergesFromShell) +} + +describe('tokenizeFishStartupCommand', () => { + it('treats a backslash inside single quotes as an escape, unlike sh', () => { + expect(tokens(String.raw`'a\\b'`)).toEqual([String.raw`a\b`]) + expect(tokens(String.raw`'a\'b'`)).toEqual([`a'b`]) + // Every other backslash stays literal inside single quotes. + expect(tokens(String.raw`'a\nb'`)).toEqual([String.raw`a\nb`]) + }) + + it('rejects a trailing backslash before the closing single quote', () => { + // fish reads the escaped quote as literal, so the string never closes. + expect(tokenizeFishStartupCommand(String.raw`'ends\'`)).toEqual({ + ok: false, + error: 'Unclosed quote in command template.' + }) + }) + + it('escapes only \\ $ " and a continuation inside double quotes', () => { + expect(tokens(String.raw`"a\\b" "a\$b" "a\"b" "a\nb" "a\zb"`)).toEqual([ + String.raw`a\b`, + 'a$b', + 'a"b', + String.raw`a\nb`, + String.raw`a\zb` + ]) + }) + + it('decodes the unquoted escape set fish actually implements', () => { + expect(tokens(String.raw`a\nb`)).toEqual(['a\nb']) + expect(tokens(String.raw`a\tb`)).toEqual(['a\tb']) + expect(tokens(String.raw`\x41 \X41 \101 A \U00000041`)).toEqual(['A', 'A', 'A', 'A', 'A']) + expect(tokens(String.raw`\cA`)).toEqual(['']) + // An unknown escape drops the backslash and keeps the byte. + expect(tokens(String.raw`a\zb`)).toEqual(['azb']) + }) + + it('keeps quoted regions attached to the surrounding token', () => { + expect(tokens(`a"b"c'd'e`)).toEqual(['abcde']) + expect(tokens(`--flag='has space'`)).toEqual(['--flag=has space']) + }) + + it('reports an unclosed quote of either kind', () => { + expect(tokenizeFishStartupCommand(`a 'b`).ok).toBe(false) + expect(tokenizeFishStartupCommand(`a "b`).ok).toBe(false) + }) + + it('flags live fish syntax a span splice cannot model', () => { + // Command substitution, brace expansion and globs are all live in fish; an + // unmatched wildcard is a hard error rather than a literal. + expect(spanFlags('claude (echo hi)')).toEqual([false, true, true]) + expect(spanFlags('claude {a,b}')).toEqual([false, true]) + expect(spanFlags('claude *.ts')).toEqual([false, true]) + expect(spanFlags('claude a?b')).toEqual([false, true]) + expect(spanFlags('claude x; y')).toEqual([false, true, false]) + expect(spanFlags('claude trailing\\')).toEqual([false, true]) + // Plain words and quoted metacharacters stay modelable. + expect(spanFlags(`claude --resume 'a*b' "c;d"`)).toEqual([false, false, false, false]) + }) + + it('is what tokenizeStartupCommand uses for the fish dialect', () => { + const line = String.raw`claude --prompt 'match \\d+'` + expect(tokenizeStartupCommand(line, 'fish')).toEqual(tokenizeFishStartupCommand(line)) + // The sh tokenizer keeps both backslashes, which is not what fish does. + const posix = tokenizeStartupCommand(line, 'posix') + expect(posix.ok && posix.tokens.at(-1)).toBe(String.raw`match \\d+`) + }) +}) diff --git a/src/shared/fish-command-tokenizer.ts b/src/shared/fish-command-tokenizer.ts new file mode 100644 index 00000000000..46973ed73de --- /dev/null +++ b/src/shared/fish-command-tokenizer.ts @@ -0,0 +1,247 @@ +import type { CommandTokenSpan } from './commit-message-prompt' + +/** + * Splits a command line the way fish does, which is NOT the way sh does. + * + * fish single quotes are not literal: `\\` and `\'` are escapes inside them, so + * a trailing backslash before the closing quote is a syntax error rather than a + * literal byte. Unquoted backslashes decode the full C-style escape set + * (`\n`, `\t`, `\x41`, `\101`, `A`, `\cA`, …) instead of just protecting the + * next byte, and inside double quotes only `\\`, `\$`, `\"` and a line + * continuation are escapes — every other backslash stays literal. + * + * Verified against fish 4.7.1. Feeding these strings through the sh tokenizer + * silently drops or invents backslashes, which is how a regex or a UNC path in a + * user's CLI-args string gets corrupted before it ever reaches the agent. + */ +export type FishStartupCommandTokens = + | { ok: true; tokens: string[]; spans: CommandTokenSpan[] } + | { ok: false; error: string } + +const UNCLOSED_QUOTE_ERROR = 'Unclosed quote in command template.' + +const SIMPLE_ESCAPES: Record = { + a: '\x07', + b: '\b', + e: '\x1b', + f: '\f', + n: '\n', + r: '\r', + t: '\t', + v: '\v', + '\\': '\\' +} + +type EscapeDecode = { text: string; next: number; diverges: boolean } + +function readRadix(value: string, start: number, pattern: RegExp): string { + return pattern.exec(value.slice(start))?.[0] ?? '' +} + +/** Decodes the escape whose backslash sits at `start - 1`. */ +function decodeFishEscape(value: string, start: number): EscapeDecode { + const char = value[start] + const simple = SIMPLE_ESCAPES[char] + if (simple !== undefined) { + return { text: simple, next: start + 1, diverges: false } + } + const fromCode = (digits: string, radix: number, prefix: number): EscapeDecode => { + const code = Number.parseInt(digits, radix) + // Why: fish rejects an out-of-range escape outright, and a NUL truncates the + // token; neither is something this tokenizer's value can represent. + if (!Number.isFinite(code) || code === 0 || code > 0x10ffff) { + return { + text: value.slice(start, start + prefix + digits.length), + next: start + prefix + digits.length, + diverges: true + } + } + return { + text: String.fromCodePoint(code), + next: start + prefix + digits.length, + diverges: false + } + } + if (char === 'x' || char === 'X') { + const digits = readRadix(value, start + 1, /^[0-9a-fA-F]{1,2}/) + return digits ? fromCode(digits, 16, 1) : { text: char, next: start + 1, diverges: false } + } + if (char === 'u' || char === 'U') { + const digits = readRadix( + value, + start + 1, + char === 'u' ? /^[0-9a-fA-F]{1,4}/ : /^[0-9a-fA-F]{1,8}/ + ) + return digits ? fromCode(digits, 16, 1) : { text: char, next: start + 1, diverges: false } + } + if (char >= '0' && char <= '7') { + const digits = readRadix(value, start, /^[0-7]{1,3}/) + // fish caps an octal escape at one byte and errors above it. + return Number.parseInt(digits, 8) > 0xff + ? { + text: value.slice(start, start + digits.length), + next: start + digits.length, + diverges: true + } + : fromCode(digits, 8, 0) + } + if (char === 'c' && value[start + 1]) { + return { + text: String.fromCharCode(value.charCodeAt(start + 1) & 0x1f), + next: start + 2, + diverges: false + } + } + return { text: char, next: start + 1, diverges: false } +} + +export function tokenizeFishStartupCommand(value: string): FishStartupCommandTokens { + const tokens: string[] = [] + const spans: CommandTokenSpan[] = [] + let token = '' + let tokenStart = 0 + let started = false + let diverges = false + let index = 0 + + const begin = (at: number): void => { + if (!started) { + tokenStart = at + } + started = true + } + + while (index < value.length) { + const char = value[index] + + if (char === "'") { + begin(index) + index += 1 + let closed = false + while (index < value.length) { + const inner = value[index] + if (inner === '\\' && (value[index + 1] === '\\' || value[index + 1] === "'")) { + token += value[index + 1] + index += 2 + continue + } + if (inner === "'") { + closed = true + index += 1 + break + } + token += inner + index += 1 + } + if (!closed) { + return { ok: false, error: UNCLOSED_QUOTE_ERROR } + } + continue + } + + if (char === '"') { + begin(index) + index += 1 + let closed = false + while (index < value.length) { + const inner = value[index] + if (inner === '\\') { + const next = value[index + 1] + if (next === '\\' || next === '$' || next === '"') { + token += next + index += 2 + continue + } + if (next === '\n') { + // A continuation joins words the caller's span splice cannot model. + diverges = true + index += 2 + continue + } + if (next === undefined) { + break + } + token += inner + index += 1 + continue + } + if (inner === '"') { + closed = true + index += 1 + break + } + // Why: `$(…)` and `${…}` reopen a nested parsing context this loop does not model. + diverges ||= inner === '$' && '({'.includes(value[index + 1] ?? '\0') + token += inner + index += 1 + } + if (!closed) { + return { ok: false, error: UNCLOSED_QUOTE_ERROR } + } + continue + } + + if (char === '\\') { + const next = value[index + 1] + if (next === undefined) { + // A trailing backslash swallows whatever a consumer appends after the base. + diverges = true + begin(index) + token += char + index += 1 + continue + } + if (next === '\n') { + diverges = true + begin(index) + index += 2 + continue + } + begin(index) + const decoded = decodeFishEscape(value, index + 1) + diverges ||= decoded.diverges + token += decoded.text + index = decoded.next + continue + } + + if (/\s/.test(char)) { + if (started) { + tokens.push(token) + spans.push({ + start: tokenStart, + end: index, + divergesFromShell: diverges + }) + token = '' + started = false + diverges = false + } + index += 1 + continue + } + + const atTokenStart = !started + begin(index) + diverges ||= + ';&|<>'.includes(char) || + // Command substitution `(…)`/`$(…)` and brace expansion, all live fish syntax. + '(){}'.includes(char) || + // An unmatched wildcard is a HARD ERROR in fish, and a matched one splits the word. + '*?['.includes(char) || + (char === '#' && atTokenStart) || + (char === '$' && '({'.includes(value[index + 1] ?? '\0')) + token += char + index += 1 + } + + if (started) { + tokens.push(token) + spans.push({ + start: tokenStart, + end: value.length, + divergesFromShell: diverges + }) + } + return { ok: true, tokens, spans } +} diff --git a/src/shared/fish-startup-arg-quoting.live-fish.test.ts b/src/shared/fish-startup-arg-quoting.live-fish.test.ts new file mode 100644 index 00000000000..707b81ef1bd --- /dev/null +++ b/src/shared/fish-startup-arg-quoting.live-fish.test.ts @@ -0,0 +1,231 @@ +/** + * Round-trips `quoteStartupArg(value, 'fish')` and `tokenizeStartupCommand(…, 'fish')` + * through a REAL fish, because fish's quoting is only observable in fish. + * + * fish single quotes are not literal the way sh's are: `\\` and `\'` are escapes, + * so sh's `'\''` idiom silently halves every backslash and a trailing backslash is + * a hard syntax error. A string assertion against a hand-written expectation would + * happily encode the same wrong rule, so the shell itself is the oracle. + * + * fish reads each case from a script file and echoes its argv back NUL-separated + * (`string join0`), which is the only separator no test input can forge. + */ +import { execFileSync } from 'node:child_process' +import { chmodSync, copyFileSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import path from 'node:path' +import { afterAll, beforeAll, describe, expect, it } from 'vitest' +import { fishRequirementViolation, resolveFishBinary } from './fish-binary-requirement' +import { planHermesStartupQuery } from './hermes-startup-query' +import { buildAgentStartupPlan } from './tui-agent-startup' +import { quoteStartupArg, tokenizeStartupCommand } from './tui-agent-startup-shell' + +const FISH = resolveFishBinary(4) +const itWithFish = FISH.available ? it : it.skip + +let home: string | null = null + +/** Runs `commandLine` in a config-free fish and returns the argv it produced. */ +function fishArgv(commandLine: string): string[] { + const root = home as string + const scriptPath = path.join(root, 'probe.fish') + const outPath = path.join(root, 'argv.bin') + // Removed first so a run that dies before writing surfaces as ENOENT rather + // than as the previous case's argv. `exit 0` only masks `string join`'s + // nothing-to-join status 1; a syntax error still aborts the file with 127. + rmSync(outPath, { force: true }) + writeFileSync(scriptPath, `string join0 -- ${commandLine} > ${outPath}\nexit 0\n`) + execFileSync(FISH.path as string, [scriptPath], { + stdio: ['ignore', 'pipe', 'pipe'], + env: { + PATH: process.env.PATH ?? '/usr/bin:/bin', + HOME: root, + XDG_CONFIG_HOME: root, + XDG_DATA_HOME: path.join(root, 'data'), + TERM: 'dumb', + LANG: 'en_US.UTF-8' + } + }) + const raw = readFileSync(outPath, 'utf8') + // join0 terminates the last element too, so the trailing empty split is not an argument. + return raw.length === 0 ? [] : raw.split('\0').slice(0, -1) +} + +const ADVERSARIAL_ARGS: readonly [name: string, value: string][] = [ + ['regex backslashes', 'use \\d+ or \\\\d and \\w'], + ['UNC path', '\\\\server\\share\\dir'], + ['windows drive path', 'C:\\Users\\nw\\Application Data'], + ['apostrophes', "it's the user's own prompt"], + ['trailing backslash', 'ends with a backslash\\'], + ['lone backslash', '\\'], + ['mixed quotes', `he said "hi" and 'bye'`], + ['newlines', 'first line\nsecond line\n'], + ['dollar expansions', '$PATH and ${HOME} and $(id -u)'], + ['glob characters', '*.ts ?? [a-z] **/*.zzz'], + ['command substitution parens', 'call (echo hi) and `echo hi`'], + ['operators', 'a; b && c || d | e > f < g & h'], + ['comment and tilde', '# not a comment ~ not a home'], + ['tabs and unicode', 'tab\there 🐟 é'], + ['escaped-quote soup', `a\\'b'c\\\\d"e`] +] + +describe('quoteStartupArg round-trips through real fish', () => { + beforeAll(() => { + home = mkdtempSync(path.join(tmpdir(), 'orca-fish-quote-')) + }) + + afterAll(() => { + if (home) { + rmSync(home, { recursive: true, force: true }) + home = null + } + }) + + // Always runs, so the CI lane cannot report green with everything below skipped. + it('has the fish this suite needs when CI requires one', () => { + expect(fishRequirementViolation(FISH)).toBeNull() + }) + + for (const [name, value] of ADVERSARIAL_ARGS) { + itWithFish(`survives ${name}`, () => { + expect(fishArgv(quoteStartupArg(value, 'fish'))).toEqual([value]) + }) + } + + itWithFish('keeps multiple arguments separate', () => { + const args = ADVERSARIAL_ARGS.map(([, value]) => value) + expect(fishArgv(args.map((arg) => quoteStartupArg(arg, 'fish')).join(' '))).toEqual(args) + }) + + itWithFish('proves the sh quoting this replaced was broken in fish', () => { + // Corrupted, not rejected: the backslashes silently halve. + expect(fishArgv(quoteStartupArg('\\\\server\\share', 'posix'))).toEqual(['\\server\\share']) + // And a trailing backslash is a hard syntax error that would kill the launch. + expect(() => fishArgv(quoteStartupArg('ends with a backslash\\', 'posix'))).toThrow() + }) + + itWithFish('proves the sh tokenizer this replaced disagreed with fish', () => { + const line = String.raw`claude --prompt 'match \d+ in \\server\share'` + const posix = tokenizeStartupCommand(line, 'posix') + expect(posix.ok).toBe(true) + if (posix.ok) { + expect(posix.tokens).not.toEqual(fishArgv(line)) + } + }) + + itWithFish('tokenizes a fish command line the way fish splits it', () => { + const lines = [ + String.raw`claude --append-system-prompt 'match \d+ in \\server\share'`, + String.raw`codex --cd "C:\Users\nw" --note 'it\'s mine'`, + String.raw`hermes chat --query a\nb --flag "keep \n literal"`, + String.raw`pi --path 'C:\\' --other "a\$b" --third x\ty`, + String.raw`agent --hex \x41 --octal \101 --ctrl \cA` + ] + for (const line of lines) { + const tokenized = tokenizeStartupCommand(line, 'fish') + expect(tokenized.ok, line).toBe(true) + if (!tokenized.ok) { + continue + } + expect(tokenized.tokens, line).toEqual(fishArgv(line)) + // Re-quoting the parsed tokens must reproduce the same argv. + expect( + fishArgv(tokenized.tokens.map((token) => quoteStartupArg(token, 'fish')).join(' ')), + line + ).toEqual(tokenized.tokens) + } + }) + + // The user-facing break: a composer prompt is quoted here and parsed by fish. + itWithFish('delivers a composer prompt to the agent argv intact', () => { + const root = home as string + const capturePath = path.join(root, 'claude-argv.json') + const claudePath = path.join(root, 'claude') + const prompt = [ + String.raw`Use \d+ for digits and read \\server\share\notes.`, + String.raw`Keep 'single' and "double" quotes, $PATH, *.ts and a ; too.`, + 'This last line ends in a backslash\\' + ].join('\n') + copyFileSync(process.execPath, claudePath) + chmodSync(claudePath, 0o755) + writeFileSync( + path.join(root, 'capture.js'), + `require('node:fs').writeFileSync(${JSON.stringify(capturePath)}, JSON.stringify(process.argv.slice(2)))` + ) + rmSync(capturePath, { force: true }) + + const plan = buildAgentStartupPlan({ + agent: 'claude', + prompt, + cmdOverrides: { claude: `${quoteStartupArg(claudePath, 'fish')} capture.js` }, + platform: 'darwin', + shell: 'fish' + }) + expect(plan).not.toBeNull() + + const scriptPath = path.join(root, 'launch.fish') + writeFileSync(scriptPath, `${plan?.launchCommand}\n`) + execFileSync(FISH.path as string, [scriptPath], { + cwd: root, + stdio: ['ignore', 'pipe', 'pipe'], + env: { + PATH: process.env.PATH ?? '/usr/bin:/bin', + HOME: root, + XDG_CONFIG_HOME: root, + XDG_DATA_HOME: path.join(root, 'data'), + TERM: 'dumb', + LANG: 'en_US.UTF-8' + } + }) + + expect(JSON.parse(readFileSync(capturePath, 'utf8'))).toEqual([prompt]) + }) + + // The `sh -c` wrapper's OUTER quoting is parsed by fish, and its payload is a + // solid run of `\0NNN` octal escapes — the single worst case for the sh spelling. + itWithFish('delivers a hermes startup query through the fish-quoted sh wrapper', () => { + const root = home as string + const capturePath = path.join(root, 'hermes-argv.json') + const hermesPath = path.join(root, 'hermes') + const prompt = [ + String.raw`Match \d+ inside \\server\share and keep 'quotes' plus "doubles".`, + 'A blank $PATH stays literal, as does *.ts.', + 'And this line ends with a backslash\\' + ].join('\n') + // A copy of node named `hermes` runs the `chat` script and records its argv. + copyFileSync(process.execPath, hermesPath) + chmodSync(hermesPath, 0o755) + writeFileSync( + path.join(root, 'chat'), + `require('node:fs').writeFileSync(${JSON.stringify(capturePath)}, JSON.stringify(process.argv.slice(2)))` + ) + rmSync(capturePath, { force: true }) + + const plan = planHermesStartupQuery({ + baseCommand: quoteStartupArg(hermesPath, 'fish'), + agentArgs: '--yolo', + prompt, + platform: 'darwin', + shell: 'fish' + }) + expect(plan).not.toBeNull() + + const scriptPath = path.join(root, 'hermes.fish') + writeFileSync(scriptPath, `${plan?.command}\n`) + execFileSync(FISH.path as string, [scriptPath], { + cwd: root, + stdio: ['ignore', 'pipe', 'pipe'], + env: { + PATH: process.env.PATH ?? '/usr/bin:/bin', + HOME: root, + XDG_CONFIG_HOME: root, + XDG_DATA_HOME: path.join(root, 'data'), + TERM: 'dumb', + LANG: 'en_US.UTF-8', + ...plan?.env + } + }) + + expect(JSON.parse(readFileSync(capturePath, 'utf8'))).toContain(`--query=${prompt}`) + }) +}) diff --git a/src/shared/hermes-startup-query.ts b/src/shared/hermes-startup-query.ts index a120305e1ee..a8536f91601 100644 --- a/src/shared/hermes-startup-query.ts +++ b/src/shared/hermes-startup-query.ts @@ -159,7 +159,11 @@ function buildQueryCommand(argv: string[], shell: AgentStartupShell): string { // Why: a fixed single-quote-safe wrapper parses in POSIX shells and pwsh; // the dynamic argv is decoded only after entering the known `sh` grammar. const script = `${POSIX_QUERY_VARIABLE}="\${${ORCA_HERMES_STARTUP_QUERY_ENV}}"; unset ${ORCA_HERMES_STARTUP_QUERY_ENV}; eval "$(printf %b "${encodedInvocation}")"` - return `sh -c ${quoteStartupArg(script, 'posix')}` + // Why `shell` and not 'posix': the body runs under `sh`, but the quoting around + // it is parsed by the shell that types the line. Today's payload survives sh + // quoting in fish only because its escapes happen to be `\0NNN` and never `\\` + // — quote for the real dialect so that stays an implementation detail. + return `sh -c ${quoteStartupArg(script, shell)}` } export function planHermesStartupQuery(args: { @@ -181,7 +185,10 @@ export function planHermesStartupQuery(args: { return null } const command = buildQueryCommand(argv, args.shell) - const env = { ...args.agentEnv, [ORCA_HERMES_STARTUP_QUERY_ENV]: args.prompt } + const env = { + ...args.agentEnv, + [ORCA_HERMES_STARTUP_QUERY_ENV]: args.prompt + } const envSize = Object.entries(env).reduce((total, [key, value]) => { if (args.platform === 'win32') { return total + key.length + value.length + 2 diff --git a/src/shared/tui-agent-startup-shell.test.ts b/src/shared/tui-agent-startup-shell.test.ts index 5511da44de9..1827698f29e 100644 --- a/src/shared/tui-agent-startup-shell.test.ts +++ b/src/shared/tui-agent-startup-shell.test.ts @@ -76,18 +76,43 @@ describe('fish startup shell dialect', () => { ) }) - it('shares POSIX quoting, tokenizing and chaining', () => { - expect(quoteStartupArg("it's", 'fish')).toBe(quoteStartupArg("it's", 'posix')) + it('shares POSIX word splitting and chaining but NOT quoting', () => { + // fish single quotes are not literal: `\\` and `\'` are escapes inside them. + expect(quoteStartupArg("it's", 'fish')).toBe(String.raw`'it\'s'`) + expect(quoteStartupArg(String.raw`\\server\share`, 'fish')).toBe( + String.raw`'\\\\server\\share'` + ) + // The sh spelling would have halved those backslashes when fish read it back. + expect(quoteStartupArg(String.raw`\\server\share`, 'posix')).toBe(String.raw`'\\server\share'`) + // A trailing backslash is a hard syntax error in fish unless it is escaped. + expect(quoteStartupArg('ends\\', 'fish')).toBe(String.raw`'ends\\'`) expect(buildShellCommandFromArgv(['codex', 'resume', 'a b'], 'fish')).toBe( buildShellCommandFromArgv(['codex', 'resume', 'a b'], 'posix') ) - expect(tokenizeStartupCommand('codex --arg "a b"', 'fish')).toEqual( - tokenizeStartupCommand('codex --arg "a b"', 'posix') - ) expect(commandSeparator('fish')).toBe('; ') expect(isPosixStartupShell('fish')).toBe(true) }) + it('tokenizes with fish escape rules rather than sh ones', () => { + const line = String.raw`codex --arg 'a\\b'` + expect(tokenizeStartupCommand(line, 'fish')).not.toEqual(tokenizeStartupCommand(line, 'posix')) + const fishTokens = tokenizeStartupCommand(line, 'fish') + expect(fishTokens.ok && fishTokens.tokens.at(-1)).toBe(String.raw`a\b`) + }) + + it('round-trips an adversarial argument through quote then tokenize', () => { + for (const value of [ + String.raw`use \d+ and \\server\share`, + "it's mine", + 'ends\\', + 'a "b" c', + '$PATH *.ts' + ]) { + const tokenized = tokenizeStartupCommand(quoteStartupArg(value, 'fish'), 'fish') + expect(tokenized.ok && tokenized.tokens).toEqual([value]) + } + }) + it('maps login shell paths to their dialect', () => { expect(resolveLoginShellStartupDialect('/opt/homebrew/bin/fish')).toBe('fish') expect(resolveLoginShellStartupDialect('/usr/local/bin/FISH')).toBe('fish') diff --git a/src/shared/tui-agent-startup-shell.ts b/src/shared/tui-agent-startup-shell.ts index ef23b837e0b..4b626d293ea 100644 --- a/src/shared/tui-agent-startup-shell.ts +++ b/src/shared/tui-agent-startup-shell.ts @@ -1,12 +1,20 @@ import { tokenizeCustomCommandTemplate, type CommandTokenSpan } from './commit-message-prompt' +import { tokenizeFishStartupCommand } from './fish-command-tokenizer' -// Why: fish shares POSIX word splitting, quoting and `;` chaining, so it is a -// separate dialect only where its grammar actually diverges (env clearing). +// Why: fish shares POSIX word splitting and `;` chaining, but NOT quoting or +// escaping — see quoteStartupArg and fish-command-tokenizer.ts. export type AgentStartupShell = 'posix' | 'fish' | 'powershell' | 'cmd' type WindowsStartupShell = Extract -/** True for shells parsed with POSIX quoting/word rules (sh family + fish). */ +/** + * True for the sh-family grammar: `NAME=value cmd` prefixes, `sh -c` wrapping and + * `;` chaining all parse. fish qualifies for those. + * + * NOT a claim that quoting matches — fish's does not. Anything that builds an + * argument must pass the real dialect to `quoteStartupArg`/`tokenizeStartupCommand`, + * never hardcode `'posix'` behind this check. + */ export function isPosixStartupShell(shell: AgentStartupShell): boolean { return shell === 'posix' || shell === 'fish' } @@ -154,9 +162,10 @@ export function tokenizeStartupCommand( value: string, shell: AgentStartupShell ): StartupCommandTokens { - return isWindowsStartupShell(shell) - ? tokenizeWindowsStartupCommand(value, shell) - : tokenizeCustomCommandTemplate(value) + if (isWindowsStartupShell(shell)) { + return tokenizeWindowsStartupCommand(value, shell) + } + return shell === 'fish' ? tokenizeFishStartupCommand(value) : tokenizeCustomCommandTemplate(value) } export function resolveStartupShell( @@ -173,6 +182,14 @@ export function quoteStartupArg(value: string, shell: AgentStartupShell): string if (shell === 'cmd') { return `"${value.replace(/([\^&|<>()%!"])/g, '^$1')}"` } + // Why: fish single quotes are NOT literal. `\\` and `\'` are escapes inside + // them, so the sh `'\''` idiom collapses every backslash (a `\d+` regex, a + // `\\server\share` UNC path) and a trailing backslash is a hard syntax error + // that kills the launch. Escaping both metacharacters round-trips instead; + // verified against fish 4.7.1. + if (shell === 'fish') { + return `'${value.replace(/\\/g, '\\\\').replace(/'/g, "\\'")}'` + } return `'${value.replace(/'/g, `'\\''`)}'` }