diff --git a/src/main/daemon/shell-ready.test.ts b/src/main/daemon/shell-ready.test.ts index 86cd27ba6d1..4d6afeb7340 100644 --- a/src/main/daemon/shell-ready.test.ts +++ b/src/main/daemon/shell-ready.test.ts @@ -269,7 +269,7 @@ describePosix('daemon shell-ready launch config', () => { expect(config.env).toEqual({ ORCA_SHELL_READY_MARKER: '0' }) expect(config.args?.slice(0, 2)).toEqual(['-l', '-C']) const init = config.args?.[2] ?? '' - expect(init).toContain('set -gx PATH "$__orca_shim_dir" $PATH') + expect(init).toContain('set -gx PATH "$__orca_shim_dir" $__orca_kept_path') expect(init).toContain('set -gx OPENCODE_CONFIG_DIR "$ORCA_OPENCODE_CONFIG_DIR"') expect(init).toContain('set -gx MIMOCODE_HOME "$ORCA_MIMOCODE_HOME"') expect(init).toContain('set -gx CODEX_HOME "$ORCA_CODEX_HOME"') diff --git a/src/main/fish-launch-config-parity.test.ts b/src/main/fish-launch-config-parity.test.ts new file mode 100644 index 00000000000..7f6e7d51714 --- /dev/null +++ b/src/main/fish-launch-config-parity.test.ts @@ -0,0 +1,97 @@ +/** + * fish is launched from three independent places — the local PTY provider, the + * daemon/SSH transport, and the relay running on a remote host. Each one used to grow + * its own idea of how to start a shell, and the relay simply had no fish branch at all + * (it fell through to bare `-l`), so an SSH user on fish lost the routed agent homes and + * the shim PATH entry that the same code fixes locally. + * + * This file is the drift alarm: it fails if any one site starts fish differently from + * the others, or stops sourcing its init text from `getFishInitCommand`. + */ +import { describe, expect, it } from 'vitest' +import { + getAttributionShellLaunchConfig as getDaemonAttributionShellLaunchConfig, + getShellReadyLaunchConfig as getDaemonShellReadyLaunchConfig +} from './daemon/shell-ready' +import { + getAttributionShellLaunchConfig as getLocalPtyAttributionShellLaunchConfig, + getShellReadyLaunchConfig as getLocalPtyShellReadyLaunchConfig +} from './providers/local-pty-shell-ready' +import { getRelayShellLaunchConfig } from '../relay/pty-shell-launch' +import { getFishInitCommand } from './shell-templates' + +const FISH_PATH = '/opt/homebrew/bin/fish' +const SHELL_READY_MARKER = '\\033]777;orca-shell-ready\\007' +const RELAY_ENV = { HOME: '/home/tester' } + +type LaunchShape = { args: string[]; env: Record } + +function shape(config: { args: string[] | null; env: Record }): LaunchShape { + return { args: config.args ?? [], env: config.env } +} + +function launchShapes(emitReadyMarker: boolean): Record { + return { + local: shape( + emitReadyMarker + ? getLocalPtyShellReadyLaunchConfig(FISH_PATH) + : getLocalPtyAttributionShellLaunchConfig(FISH_PATH) + ), + daemon: shape( + emitReadyMarker + ? getDaemonShellReadyLaunchConfig(FISH_PATH) + : getDaemonAttributionShellLaunchConfig(FISH_PATH) + ), + relay: shape(getRelayShellLaunchConfig(FISH_PATH, RELAY_ENV, 'linux', { emitReadyMarker })) + } +} + +describe('fish launch config parity across the three launch sites', () => { + for (const emitReadyMarker of [true, false]) { + const label = emitReadyMarker ? 'shell-ready' : 'attribution-only' + + it(`starts ${label} fish identically from the local, daemon, and relay sites`, () => { + const { local, daemon, relay } = launchShapes(emitReadyMarker) + expect(daemon).toEqual(local) + expect(relay).toEqual(local) + }) + + it(`hands ${label} fish the shared init text rather than a per-site copy`, () => { + const expected = ['-l', '-C', getFishInitCommand(SHELL_READY_MARKER)] + for (const [site, config] of Object.entries(launchShapes(emitReadyMarker))) { + expect(config.args, site).toEqual(expected) + // Why: the marker block is inert unless this is set, so a site that drops it + // would look correct in the args diff and never emit a ready marker. + expect(config.env, site).toEqual({ + ORCA_SHELL_READY_MARKER: emitReadyMarker ? '1' : '0' + }) + } + }) + } + + it('re-prepends every shim variable the three sites can plant', () => { + const init = getFishInitCommand(SHELL_READY_MARKER) + // ORCA_REMOTE_CLI_BIN_DIR is the relay's own name for the shim dir; the local sites + // never set it and fish expands the unset name to nothing, which is what lets one + // text serve all three. + for (const variable of [ + 'ORCA_ATTRIBUTION_SHIM_DIR', + 'ORCA_AGENT_TEAMS_SHIM_DIR', + 'ORCA_REMOTE_CLI_BIN_DIR' + ]) { + expect(init).toContain(`$${variable}`) + } + for (const variable of ['ORCA_OPENCODE_CONFIG_DIR', 'ORCA_MIMOCODE_HOME', 'ORCA_CODEX_HOME']) { + expect(init).toContain(`$${variable}`) + } + }) + + it('keeps non-fish relay shells on their existing launch shapes', () => { + // Why: the fish branch sits ahead of the bash/zsh wrapper work, so a basename + // mismatch there would silently disarm the overlay wrappers for everyone else. + expect(getRelayShellLaunchConfig('/usr/bin/dash', RELAY_ENV, 'linux')).toEqual({ + args: ['-l'], + env: {} + }) + }) +}) diff --git a/src/main/fish-startup-environment.node-pty.test.ts b/src/main/fish-startup-environment.node-pty.test.ts index 6f587c96a17..e6a64d4d6c2 100644 --- a/src/main/fish-startup-environment.node-pty.test.ts +++ b/src/main/fish-startup-environment.node-pty.test.ts @@ -11,13 +11,18 @@ * * Both are asserted against a control spawn with the init command removed, so a * regression that makes the init command a no-op cannot pass this file. + * + * Every path here is deliberately spacey (and one carries glob characters), because the + * dirs this text moves around are really "~/Library/Application Support/...". */ +import { spawnSync } from 'node:child_process' import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { describe, expect, it } from 'vitest' import { getAttributionShellLaunchConfig } from './daemon/shell-ready' import { getAttributionShellLaunchConfig as getLocalPtyAttributionShellLaunchConfig } from './providers/local-pty-shell-ready' +import { getFishInitCommand } from './shell-templates' import { fishRequirementViolation, resolveFishBinary } from '../shared/fish-binary-requirement' const FISH = resolveFishBinary() @@ -33,10 +38,25 @@ const TERMINAL_QUERY_REPLIES: readonly (readonly [string, string])[] = [ ] const QUERY_CARRY_LEN = Math.max(...TERMINAL_QUERY_REPLIES.map(([query]) => query.length)) -const SHIM_DIR = '/orca-test/attribution-shim' -const USER_BIN_DIR = '/orca-test/user-bin' -const ORCA_CODEX_HOME = '/orca-test/routed-codex-home' -const USER_CODEX_HOME = '/orca-test/user-codex-home' +// Spaces on purpose: "~/Library/Application Support/..." is the real shape of an Orca +// shim dir on macOS, and an unquoted fish `set` would silently split it into two entries. +const SHIM_DIR = '/orca test/attribution shim' +const TEAMS_SHIM_DIR = '/orca test/agent teams shim' +// Glob characters on purpose: fish treats an unmatched glob as a hard error, so a filter +// written with `string match` instead of a plain compare would take the whole init down. +const RELAY_SHIM_DIR = '/orca test/relay [cli] bin' +const USER_BIN_DIR = '/orca test/user bin' +const ORCA_CODEX_HOME = '/orca test/routed codex home' +const USER_CODEX_HOME = '/orca test/user codex home' +const SHELL_READY_MARKER = '\\033]777;orca-shell-ready\\007' + +/** + * fish single quotes are NOT POSIX single quotes: `\\` and `\'` still escape inside them, + * and a trailing backslash before the closing quote is a syntax error. Verified on 4.7.1. + */ +function fishQuote(value: string): string { + return `'${value.replace(/\\/g, '\\\\').replace(/'/g, "\\'")}'` +} /** * Spawns fish on a real PTY with the given args and returns what the session looks @@ -48,7 +68,7 @@ async function readFirstPromptEnvironment(args: string[]): Promise<{ codexHome: string }> { const pty = await import('node-pty') - const home = mkdtempSync(join(tmpdir(), 'orca-fish-startup-')) + const home = mkdtempSync(join(tmpdir(), 'orca fish startup ')) const resultPath = join(home, 'probe-result') const donePath = join(home, 'probe-done') try { @@ -60,14 +80,14 @@ async function readFirstPromptEnvironment(args: string[]): Promise<{ 'function fish_prompt; printf "> "; end', 'function fish_right_prompt; end', '# A user who routes their own agent home: Orca must still win.', - `set -gx CODEX_HOME ${USER_CODEX_HOME}`, + `set -gx CODEX_HOME ${fishQuote(USER_CODEX_HOME)}`, '# Stands in for macOS path_helper (which really does run here on macOS):', '# whatever Orca prepended at spawn is no longer first once config.fish ran.', - `set -gx PATH ${USER_BIN_DIR} $PATH`, + `set -gx PATH ${fishQuote(USER_BIN_DIR)} $PATH`, 'function __orca_probe --on-event fish_prompt', - ` echo "PATH1=$PATH[1]" >"${resultPath}"`, - ` echo "CODEX_HOME=$CODEX_HOME" >>"${resultPath}"`, - ` echo done >"${donePath}"`, + ` echo "PATH1=$PATH[1]" >${fishQuote(resultPath)}`, + ` echo "CODEX_HOME=$CODEX_HOME" >>${fishQuote(resultPath)}`, + ` echo done >${fishQuote(donePath)}`, 'end', '' ].join('\n') @@ -172,4 +192,57 @@ describe('fish startup environment', () => { }, 45_000 ) + + itWithFish('runs clean and unchanged when a pane re-runs it', () => { + // Why: a re-initialized pane can evaluate this text a second time in the same + // session. A plain prepend would grow PATH by one duplicate per shim per run. + const home = mkdtempSync(join(tmpdir(), 'orca fish idempotent ')) + try { + const initPath = join(home, 'init.fish') + writeFileSync(initPath, getFishInitCommand(SHELL_READY_MARKER)) + const result = spawnSync( + FISH.path as string, + [ + // --no-config so the outcome is the init text alone, not a developer's config.fish. + '--no-config', + '-c', + [ + 'set -gx PATH /usr/bin /bin', + `source ${fishQuote(initPath)}`, + `source ${fishQuote(initPath)}`, + 'for __probe_entry in $PATH; echo "PATH=$__probe_entry"; end', + 'echo "CODEX_HOME=$CODEX_HOME"', + // Nothing the init text declares may survive into the user's session. + 'echo "LEAKED=$__orca_shim_dir$__orca_kept_path$__orca_path_entry"' + ].join('\n') + ], + { + encoding: 'utf8', + env: { + PATH: process.env.PATH ?? '/usr/bin:/bin', + HOME: home, + // All three shim variables, because the duplication only shows up once a + // later shim displaces an earlier one from PATH[1]. ORCA_REMOTE_CLI_BIN_DIR + // is the relay's name for it and is only ever set on an SSH host. + ORCA_ATTRIBUTION_SHIM_DIR: SHIM_DIR, + ORCA_AGENT_TEAMS_SHIM_DIR: TEAMS_SHIM_DIR, + ORCA_REMOTE_CLI_BIN_DIR: RELAY_SHIM_DIR, + ORCA_CODEX_HOME, + ORCA_SHELL_READY_MARKER: '0' + } + } + ) + + expect(result.stderr).toBe('') + expect(result.status).toBe(0) + const pathEntries = [...result.stdout.matchAll(/^PATH=(.*)$/gm)].map((match) => match[1]) + // Two `source`s, one entry each — and the relay shim ends up first, matching where + // the relay's own bash/zsh wrappers put it. + expect(pathEntries).toEqual([RELAY_SHIM_DIR, TEAMS_SHIM_DIR, SHIM_DIR, '/usr/bin', '/bin']) + expect(result.stdout).toContain(`CODEX_HOME=${ORCA_CODEX_HOME}`) + expect(result.stdout).toContain('LEAKED=\n') + } finally { + rmSync(home, { recursive: true, force: true }) + } + }) }) diff --git a/src/main/providers/local-pty-shell-ready.test.ts b/src/main/providers/local-pty-shell-ready.test.ts index c5ac2d3a264..1550967abca 100644 --- a/src/main/providers/local-pty-shell-ready.test.ts +++ b/src/main/providers/local-pty-shell-ready.test.ts @@ -391,7 +391,7 @@ describePosix('local PTY shell-ready launch config', () => { expect(config.env).toEqual({ ORCA_SHELL_READY_MARKER: '0' }) expect(config.args?.slice(0, 2)).toEqual(['-l', '-C']) const init = config.args?.[2] ?? '' - expect(init).toContain('set -gx PATH "$__orca_shim_dir" $PATH') + expect(init).toContain('set -gx PATH "$__orca_shim_dir" $__orca_kept_path') expect(init).toContain('set -gx CODEX_HOME "$ORCA_CODEX_HOME"') expect(init).toContain('if test "$ORCA_SHELL_READY_MARKER" = 1') }) diff --git a/src/main/shell-templates.ts b/src/main/shell-templates.ts index 266342a2261..6e408392766 100644 --- a/src/main/shell-templates.ts +++ b/src/main/shell-templates.ts @@ -187,16 +187,29 @@ end` * * The POSIX wrappers do this from their rc files; fish has no such hook, so * without this a fish pane silently loses both halves of the contract. + * + * One text serves all three launch sites (local PTY, daemon/SSH, relay): fish expands + * an unset variable to nothing, so each site's unused shim variables drop out of the + * loop. Forking a per-site copy is how the POSIX call sites drifted apart before. */ export function getFishInitCommand(escapedMarker: string): string { return `begin # Why: macOS fish's bundled config.fish runs path_helper, which rebuilds PATH and # appends inherited entries last — the attribution shim lands near-last and commits # from fish panes lose the Orca trailer. The begin block keeps the loop variable out - # of the user's session. Order matches the POSIX wrappers: attribution, then teams. - for __orca_shim_dir in $ORCA_ATTRIBUTION_SHIM_DIR $ORCA_AGENT_TEAMS_SHIM_DIR + # of the user's session. Order matches the POSIX wrappers: attribution, then teams, + # and the relay's own shim (ORCA_REMOTE_CLI_BIN_DIR) last so it ends up first. + for __orca_shim_dir in $ORCA_ATTRIBUTION_SHIM_DIR $ORCA_AGENT_TEAMS_SHIM_DIR $ORCA_REMOTE_CLI_BIN_DIR if test -n "$__orca_shim_dir"; and test "$PATH[1]" != "$__orca_shim_dir" - set -gx PATH "$__orca_shim_dir" $PATH + # Why drop existing copies instead of a plain prepend: this text can run twice in + # one session (a re-initialized pane), and a plain prepend grows PATH by one + # duplicate per shim per run. Filtered by string compare, not \`string match\`, + # because a shim path may contain glob characters. + set -l __orca_kept_path + for __orca_path_entry in $PATH + test "$__orca_path_entry" = "$__orca_shim_dir"; or set -a __orca_kept_path "$__orca_path_entry" + end + set -gx PATH "$__orca_shim_dir" $__orca_kept_path end end end diff --git a/src/relay/pty-shell-launch.test.ts b/src/relay/pty-shell-launch.test.ts index 04f7a261583..2cda2022496 100644 --- a/src/relay/pty-shell-launch.test.ts +++ b/src/relay/pty-shell-launch.test.ts @@ -1,10 +1,13 @@ -import { mkdtempSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { existsSync, mkdtempSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { spawnSync } from 'node:child_process' import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { getFishInitCommand } from '../main/shell-templates' import { getRelayShellLaunchConfig } from './pty-shell-launch' +const SHELL_READY_MARKER_ESCAPED = '\\033]777;orca-shell-ready\\007' + const hasBash = process.platform !== 'win32' && spawnSync('bash', ['--version']).status === 0 const itWithBash = hasBash ? it : it.skip @@ -122,6 +125,42 @@ describe('getRelayShellLaunchConfig', () => { }) }) + it('restores agent homes and the shim PATH entry on a remote whose shell is fish', () => { + // Why: fish fell through to a bare `-l` here, so an SSH host on fish kept none of the + // overlay env the relay planted — the account switcher looked like a no-op remotely + // and remote commits lost the Orca trailer. + const config = getRelayShellLaunchConfig( + '/usr/bin/fish', + { HOME: homeDir, ORCA_REMOTE_CLI_BIN_DIR: '/tmp/orca-relay-bin' }, + 'linux' + ) + + expect(config.args.slice(0, 2)).toEqual(['-l', '-C']) + expect(config.args[2]).toBe(getFishInitCommand(SHELL_READY_MARKER_ESCAPED)) + expect(config.env).toEqual({ ORCA_SHELL_READY_MARKER: '0' }) + }) + + it('arms the fish ready marker only when the caller asks for it', () => { + const args = ['/usr/bin/fish', { HOME: homeDir }, 'linux'] as const + expect( + getRelayShellLaunchConfig(...args, { emitReadyMarker: true }).env.ORCA_SHELL_READY_MARKER + ).toBe('1') + // Markerless is still wrapped: that is the pane where a user types `git commit`. + const markerless = getRelayShellLaunchConfig(...args, { emitReadyMarker: false }) + expect(markerless.env.ORCA_SHELL_READY_MARKER).toBe('0') + expect(markerless.args).toHaveLength(3) + }) + + it('writes no persistent wrapper files for fish', () => { + // Why: fish carries everything on `-C`, so it must not create ~/.orca-relay on a + // remote or leave stale rc files behind for a shell that never reads them. + getRelayShellLaunchConfig('/usr/bin/fish', { HOME: homeDir }, 'linux', { + emitReadyMarker: true + }) + + expect(existsSync(join(homeDir, '.orca-relay'))).toBe(false) + }) + it.skipIf(process.platform === 'win32')('rewrites stale persistent wrapper files', () => { const zshRoot = join(homeDir, '.orca-relay', 'shell-ready', 'zsh') mkdirSync(zshRoot, { recursive: true }) diff --git a/src/relay/pty-shell-launch.ts b/src/relay/pty-shell-launch.ts index 7a6d7d7a097..444a9b9afcf 100644 --- a/src/relay/pty-shell-launch.ts +++ b/src/relay/pty-shell-launch.ts @@ -3,6 +3,7 @@ import { homedir } from 'node:os' import { dirname, join } from 'node:path' import { getPosixOmpShellWrapper } from '../main/pty/omp-shell-wrapper' import { + getFishInitCommand, getZshFinalZdotdirRestoreBlock, getZshShellReadyMarkerRegistrationBlock, SHELL_STARTUP_IDENTITY_MARKER_BLOCK, @@ -288,6 +289,18 @@ export function getRelayShellLaunchConfig( } } + // Why: mirrors src/main/daemon/shell-ready.ts and providers/local-pty-shell-ready.ts. + // A remote whose login shell is fish gets no wrapper rc file, so `-C` is the only hook + // that runs after the remote's config.fish — without it the routed agent homes and the + // shim PATH entry the relay planted are silently lost on this host. Ungated like the + // local sites: the markerless pane is where the user types `git commit` by hand. + if (shellName === 'fish') { + return { + args: [...POSIX_LOGIN_ARGS, '-C', getFishInitCommand(SHELL_READY_MARKER_ESCAPED)], + env: { ORCA_SHELL_READY_MARKER: emitReadyMarker ? '1' : '0' } + } + } + if (shellName !== 'zsh' && shellName !== 'bash') { return { args: POSIX_LOGIN_ARGS, env: {} } }