diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 3c9678a5a8d..9ad1433abd8 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -246,6 +246,7 @@ jobs: src/main/providers/local-pty-shell-ready.test.ts \ src/main/providers/__tests__/shell-ready-framework-example.test.ts \ src/main/pty/omp-shell-wrapper.node-pty.test.ts \ + src/main/fish-startup-environment.node-pty.test.ts \ src/renderer/src/components/terminal-pane/fish-color-scheme-child-stdin.node-pty.test.ts \ src/shared/posix-command-path-lookup.test.ts @@ -282,6 +283,7 @@ jobs: --exclude=src/main/providers/local-pty-shell-ready.test.ts \ --exclude=src/main/providers/__tests__/shell-ready-framework-example.test.ts \ --exclude=src/main/pty/omp-shell-wrapper.node-pty.test.ts \ + --exclude=src/main/fish-startup-environment.node-pty.test.ts \ --exclude=src/renderer/src/components/terminal-pane/fish-color-scheme-child-stdin.node-pty.test.ts \ --exclude=src/shared/posix-command-path-lookup.test.ts \ --exclude=tests/e2e/cross-version-wire/** \ diff --git a/config/scripts/pr-workflow-parallelism.test.mjs b/config/scripts/pr-workflow-parallelism.test.mjs index 165931dbeb1..795e7fa1ee8 100644 --- a/config/scripts/pr-workflow-parallelism.test.mjs +++ b/config/scripts/pr-workflow-parallelism.test.mjs @@ -16,7 +16,8 @@ const shellContractFiles = [ ] const patchedNodePtyContractFiles = [ 'src/main/daemon/node-pty-fd-leak.test.ts', - 'src/main/pty/omp-shell-wrapper.node-pty.test.ts' + 'src/main/pty/omp-shell-wrapper.node-pty.test.ts', + 'src/main/fish-startup-environment.node-pty.test.ts' ] const nativeShellContractFiles = [...shellContractFiles, ...patchedNodePtyContractFiles] const testFilePatterns = [ diff --git a/src/main/daemon/shell-ready.test.ts b/src/main/daemon/shell-ready.test.ts index 1739ba05c85..86cd27ba6d1 100644 --- a/src/main/daemon/shell-ready.test.ts +++ b/src/main/daemon/shell-ready.test.ts @@ -258,12 +258,22 @@ describePosix('daemon shell-ready launch config', () => { expect(init).toContain('functions -e __orca_shell_ready_marker') }) - it('keeps attribution-only fish spawns unwrapped', async () => { + it('gives attribution-only fish the shim/agent-home init command without the marker', async () => { + // Why: the markerless pane is where the user types commits by hand, so it needs the + // PATH shim most; only the marker stays gated so nothing is emitted into the pane. const { getAttributionShellLaunchConfig } = await importFreshShellReady() const config = getAttributionShellLaunchConfig('/opt/homebrew/bin/fish') - expect(config).toEqual({ args: null, env: {}, supportsReadyMarker: false }) + expect(config.supportsReadyMarker).toBe(false) + 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 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"') + expect(init).toContain('if test "$ORCA_SHELL_READY_MARKER" = 1') }) itWithFish( diff --git a/src/main/daemon/shell-ready.ts b/src/main/daemon/shell-ready.ts index 2b1f27309f8..a706136cedc 100644 --- a/src/main/daemon/shell-ready.ts +++ b/src/main/daemon/shell-ready.ts @@ -12,7 +12,7 @@ import { } from '../powershell-osc133-bootstrap' import { getPosixOmpShellWrapper } from '../pty/omp-shell-wrapper' import { - getFishShellReadyInitCommand, + getFishInitCommand, getZshEnvTemplate, getZshFinalZdotdirRestoreBlock, getZshShellReadyMarkerRegistrationBlock, @@ -419,12 +419,14 @@ function getWrappedShellLaunchConfig( } } - // Why: mirrors local-pty-shell-ready.ts; attribution-only fish stays unwrapped. - if (shellName === 'fish' && options.emitReadyMarker) { + // Why: mirrors local-pty-shell-ready.ts. Markerless fish still needs the init command — + // that is the pane where the user types commits by hand, so the shim and agent homes + // must survive config.fish there too; only the marker itself stays gated. + if (shellName === 'fish') { return { - args: ['-l', '-C', getFishShellReadyInitCommand(SHELL_READY_MARKER)], - env: { ORCA_SHELL_READY_MARKER: '1' }, - supportsReadyMarker: true + args: ['-l', '-C', getFishInitCommand(SHELL_READY_MARKER)], + env: { ORCA_SHELL_READY_MARKER: options.emitReadyMarker ? '1' : '0' }, + supportsReadyMarker: options.emitReadyMarker } } diff --git a/src/main/fish-startup-environment.node-pty.test.ts b/src/main/fish-startup-environment.node-pty.test.ts new file mode 100644 index 00000000000..6f587c96a17 --- /dev/null +++ b/src/main/fish-startup-environment.node-pty.test.ts @@ -0,0 +1,175 @@ +/** + * Real-fish coverage for the `-C` init command Orca hands every fish PTY. + * + * fish has no ZDOTDIR-style wrapper dir, so the only hook that runs AFTER the user's + * config.fish is `--init-command`. Two things depend on it: + * - Orca's routed agent home must beat a `set -gx CODEX_HOME ...` in config.fish, + * or the account switcher silently launches the agent against the wrong account. + * - the attribution shim must be FIRST in PATH, or commits from fish panes lose the + * Orca trailer. macOS fish's bundled config.fish runs /usr/libexec/path_helper, + * which rebuilds PATH and demotes the inherited shim to near-last. + * + * 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. + */ +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 { fishRequirementViolation, resolveFishBinary } from '../shared/fish-binary-requirement' + +const FISH = resolveFishBinary() +const itWithFish = FISH.available ? it : it.skip + +/** Minimal xterm-shaped answers to the probes fish blocks its first prompt on + * (~10s on the DA1 wait alone), so this suite settles in milliseconds. */ +const TERMINAL_QUERY_REPLIES: readonly (readonly [string, string])[] = [ + ['\x1b[0c', '\x1b[?6c'], + ['\x1b[?u', '\x1b[?0u'], + ['\x1b[6n', '\x1b[1;1R'], + ['\x1b]11;?', '\x1b]11;rgb:0000/0000/0000\x1b\\'] +] +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' + +/** + * Spawns fish on a real PTY with the given args and returns what the session looks + * like at its first prompt. Nothing is typed: a fish_prompt handler in config.fish + * writes the answer to disk, so the result never races the line editor. + */ +async function readFirstPromptEnvironment(args: string[]): Promise<{ + pathFirstEntry: string + codexHome: string +}> { + const pty = await import('node-pty') + const home = mkdtempSync(join(tmpdir(), 'orca-fish-startup-')) + const resultPath = join(home, 'probe-result') + const donePath = join(home, 'probe-done') + try { + mkdirSync(join(home, '.config', 'fish'), { recursive: true }) + writeFileSync( + join(home, '.config', 'fish', 'config.fish'), + [ + 'set -g fish_greeting ""', + '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}`, + '# 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`, + 'function __orca_probe --on-event fish_prompt', + ` echo "PATH1=$PATH[1]" >"${resultPath}"`, + ` echo "CODEX_HOME=$CODEX_HOME" >>"${resultPath}"`, + ` echo done >"${donePath}"`, + 'end', + '' + ].join('\n') + ) + + const proc = pty.spawn(FISH.path as string, args, { + name: 'xterm-256color', + cols: 80, + rows: 24, + cwd: home, + // Why fully specified and never spread from process.env: an inherited + // ORCA_* or XDG_* from the developer's own shell would change the outcome. + env: { + // The inherited PATH only follows so the child can still resolve the fish + // binary; both assertions read position 1, which is pinned here and then + // displaced by config.fish, so no ambient entry can decide them. + PATH: `${SHIM_DIR}:${process.env.PATH ?? '/usr/bin:/bin'}`, + HOME: home, + TERM: 'xterm-256color', + XDG_CONFIG_HOME: join(home, '.config'), + XDG_DATA_HOME: join(home, '.local', 'share'), + ORCA_ATTRIBUTION_SHIM_DIR: SHIM_DIR, + ORCA_CODEX_HOME + } + }) + + let queryCarry = '' + let settle = (): void => {} + const done = new Promise((resolve) => { + settle = resolve + }) + const deadline = setTimeout(settle, 15_000) + const poll = setInterval(() => { + if (existsSync(donePath)) { + settle() + } + }, 25) + proc.onData((chunk) => { + // Why the carry: a query split across two chunks would otherwise go unanswered + // and re-stall the prompt, while re-answering one already handled would land in + // fish's stdin as typed input. + const carriedLength = queryCarry.length + const scan = queryCarry + chunk + queryCarry = scan.slice(-QUERY_CARRY_LEN) + for (const [query, reply] of TERMINAL_QUERY_REPLIES) { + for (let at = scan.indexOf(query); at !== -1; at = scan.indexOf(query, at + query.length)) { + if (at + query.length > carriedLength) { + proc.write(reply) + } + } + } + }) + await done + clearTimeout(deadline) + clearInterval(poll) + proc.kill() + + const result = existsSync(resultPath) ? readFileSync(resultPath, 'utf8') : '' + return { + pathFirstEntry: /^PATH1=(.*)$/m.exec(result)?.[1] ?? '', + codexHome: /^CODEX_HOME=(.*)$/m.exec(result)?.[1] ?? '' + } + } finally { + rmSync(home, { recursive: true, force: true }) + } +} + +describe('fish startup environment', () => { + // Always runs, so the CI lane cannot report green with the live tests skipped. + it('has the fish this suite needs when CI requires one', () => { + expect(fishRequirementViolation(FISH)).toBeNull() + }) + + it('hands both PTY transports the same fish attribution launch config', () => { + // Why: the daemon and local-PTY copies drifted before; a fix landing in one + // transport only is invisible until a user on the other one reports it. + expect(getAttributionShellLaunchConfig('fish')).toEqual( + getLocalPtyAttributionShellLaunchConfig('fish') + ) + }) + + itWithFish( + 'overrides a config.fish agent home and re-prepends the attribution shim', + async () => { + // Why the attribution config and not the shell-ready one: the markerless pane is + // where a user types `git commit` by hand, and it got none of this before. + const config = getAttributionShellLaunchConfig(FISH.path as string) + expect(config.env.ORCA_SHELL_READY_MARKER).toBe('0') + + const withOrcaInit = await readFirstPromptEnvironment(config.args ?? []) + // Control: same fish, same config.fish, no init command. Proves the two + // assertions below are actually produced by the init command. + const withoutOrcaInit = await readFirstPromptEnvironment(['-l']) + + expect(withOrcaInit.codexHome).toBe(ORCA_CODEX_HOME) + expect(withoutOrcaInit.codexHome).toBe(USER_CODEX_HOME) + + expect(withOrcaInit.pathFirstEntry).toBe(SHIM_DIR) + // Why the exact value and not `not.toBe(SHIM_DIR)`: a probe that never ran reads + // as an empty string, which would satisfy a negative assertion for free. + expect(withoutOrcaInit.pathFirstEntry).toBe(USER_BIN_DIR) + }, + 45_000 + ) +}) diff --git a/src/main/providers/local-pty-shell-ready.test.ts b/src/main/providers/local-pty-shell-ready.test.ts index a35eae779da..c5ac2d3a264 100644 --- a/src/main/providers/local-pty-shell-ready.test.ts +++ b/src/main/providers/local-pty-shell-ready.test.ts @@ -380,12 +380,20 @@ describePosix('local PTY shell-ready launch config', () => { expect(init).toContain('functions -e __orca_shell_ready_marker') }) - it('keeps attribution-only fish spawns unwrapped', async () => { + it('gives attribution-only fish the shim/agent-home init command without the marker', async () => { + // Why: the markerless pane is where the user types commits by hand, so it needs the + // PATH shim most; only the marker stays gated so nothing is emitted into the pane. const { getAttributionShellLaunchConfig } = await importFreshLocalPtyShellReady() const config = getAttributionShellLaunchConfig('/opt/homebrew/bin/fish') - expect(config).toEqual({ args: null, env: {}, supportsReadyMarker: false }) + expect(config.supportsReadyMarker).toBe(false) + 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 CODEX_HOME "$ORCA_CODEX_HOME"') + expect(init).toContain('if test "$ORCA_SHELL_READY_MARKER" = 1') }) it('falls back to HOME for ORCA_ORIG_ZDOTDIR when inherited ZDOTDIR points at a wrapper dir', async () => { diff --git a/src/main/providers/local-pty-shell-ready.ts b/src/main/providers/local-pty-shell-ready.ts index 28c56b27d1f..58ae8030e1f 100644 --- a/src/main/providers/local-pty-shell-ready.ts +++ b/src/main/providers/local-pty-shell-ready.ts @@ -17,7 +17,7 @@ import { import { getPosixOmpShellWrapper } from '../pty/omp-shell-wrapper' import { buildStartupCommandSubmission } from '../../shared/startup-command-submission' import { - getFishShellReadyInitCommand, + getFishInitCommand, getZshEnvTemplate, getZshFinalZdotdirRestoreBlock, getZshShellReadyMarkerRegistrationBlock, @@ -402,12 +402,14 @@ function getWrappedShellLaunchConfig( } } - // Why: mirrors daemon/shell-ready.ts; attribution-only fish stays unwrapped. - if (shellName === 'fish' && options.emitReadyMarker) { + // Why: mirrors daemon/shell-ready.ts. Markerless fish still needs the init command — + // that is the pane where the user types commits by hand, so the shim and agent homes + // must survive config.fish there too; only the marker itself stays gated. + if (shellName === 'fish') { return { - args: ['-l', '-C', getFishShellReadyInitCommand(SHELL_READY_MARKER_ESCAPED)], - env: { ORCA_SHELL_READY_MARKER: '1' }, - supportsReadyMarker: true + args: ['-l', '-C', getFishInitCommand(SHELL_READY_MARKER_ESCAPED)], + env: { ORCA_SHELL_READY_MARKER: options.emitReadyMarker ? '1' : '0' }, + supportsReadyMarker: options.emitReadyMarker } } diff --git a/src/main/shell-templates.ts b/src/main/shell-templates.ts index 5b481217d90..266342a2261 100644 --- a/src/main/shell-templates.ts +++ b/src/main/shell-templates.ts @@ -171,7 +171,7 @@ fi // zle-line-init this lands just *before* fish arms `?2004h`, which PostReadyFlushGate // absorbs. `builtin printf` so a user-defined printf can't silently swallow the marker // and send every launch to the ready timeout. -export function getFishShellReadyInitCommand(escapedMarker: string): string { +function getFishShellReadyMarkerBlock(escapedMarker: string): string { return `if test "$ORCA_SHELL_READY_MARKER" = 1 function __orca_shell_ready_marker --on-event fish_prompt builtin printf "${escapedMarker}" @@ -180,6 +180,41 @@ export function getFishShellReadyInitCommand(escapedMarker: string): string { end` } +/** + * Everything Orca must apply to a fish session *after* the user's own startup + * files, delivered as one `--init-command` (fish evaluates -C after config.fish + * and conf.d, and multi-`-C` support varies by version). + * + * 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. + */ +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 + if test -n "$__orca_shim_dir"; and test "$PATH[1]" != "$__orca_shim_dir" + set -gx PATH "$__orca_shim_dir" $PATH + end + end +end +# Why: a config.fish that sets its own agent home (\`set -gx CODEX_HOME ...\`) would +# otherwise beat Orca's routed home and launch the agent against the wrong account, +# which reads in the UI as the account switcher doing nothing. +if test -n "$ORCA_OPENCODE_CONFIG_DIR" + set -gx OPENCODE_CONFIG_DIR "$ORCA_OPENCODE_CONFIG_DIR" +end +if test -n "$ORCA_MIMOCODE_HOME" + set -gx MIMOCODE_HOME "$ORCA_MIMOCODE_HOME" +end +if test -n "$ORCA_CODEX_HOME" + set -gx CODEX_HOME "$ORCA_CODEX_HOME" +end +${getFishShellReadyMarkerBlock(escapedMarker)}` +} + export function getZshFinalZdotdirRestoreBlock(homeExpression = '"${ORCA_ORIG_ZDOTDIR:-$HOME}"') { return `_orca_home=${homeExpression} case "\${_orca_home%/}" in