Repository navigation
fix(pty): give fish login shells the Prime Agent status wrapper - #13875
brennanb2025 wants to merge 5 commits into
Conversation
Fish is not POSIX, so the bash/zsh Prime wrapper is a parse error under it. A fish login shell started cleanly and handed prime-agent the user's bare argv, so Orca's status reporting never armed (STA-3927). Add a fish-syntax wrapper written to shell-ready/fish/orca.fish and source it from the local PTY, daemon, and WSL guest launch paths.
Attribution-only fish still launched bare, so a plain fish pane lost Prime status for a hand-typed prime-agent while bash and zsh got their wrapper. Also list the live-fish suites in the one CI job that installs fish; without that they gate off a missing binary and skip silently.
📝 WalkthroughWalkthroughAdded generated Fish wrappers for Prime Agent attribution and optional shell-ready marker registration. Daemon and local PTY launches now create and source the wrapper in shell-ready and attribution-only modes. WSL Fish login commands conditionally source the wrapper with path escaping and fallback handling. Added Fish wrapper, PTY, WSL, parity, launch configuration, and CI coverage. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 56bf70f3-e31c-475c-9a72-588cc356231c
📒 Files selected for processing (15)
.github/workflows/pr.ymlsrc/main/daemon/shell-ready-fish.node-pty.test.tssrc/main/daemon/shell-ready.test.tssrc/main/daemon/shell-ready.tssrc/main/providers/local-pty-shell-ready.test.tssrc/main/providers/local-pty-shell-ready.tssrc/main/providers/windows-shell-args.test.tssrc/main/providers/wsl-fish-login-shell.test.tssrc/main/pty/prime-agent-fish-shell-wrapper.test.tssrc/main/pty/prime-agent-fish-shell-wrapper.tssrc/main/pty/prime-agent-shell-wrapper-parity.test.tssrc/main/pty/prime-agent-shell-wrapper.tssrc/main/shell-templates.tssrc/shared/wsl-login-shell-command.test.tssrc/shared/wsl-login-shell-command.ts
| # Why fish: every live-fish suite gates on the binary being present, so | ||
| # without this they skip silently and fish is covered only by config-shape | ||
| # assertions. Any new test that spawns a real fish MUST be listed in both | ||
| # the run list below and the shard --exclude list, or it never executes. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the failure mode described in the maintenance comment.
Line 189 says that omitting either list causes the test to never execute. If a test is present in the dedicated run list but missing from the shard exclusion list, it still runs in shell_contracts and may also be rediscovered by a shard. Clarify that the two lists must stay synchronized to prevent either skipped coverage or duplicate discovery.
| // Why a file, not an inlined `-C` body: the WSL launch path sources this same | ||
| // wrapper root through /mnt/c, and inlining the body into the wsl.exe argv would | ||
| // make every `$` in it collide with escapeWslShCommandForWindows. | ||
| function getFishShellReadyWrapperPath(root = getShellReadyWrapperRoot()): string { | ||
| return `${root}/fish/orca.fish` | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/main/providers/local-pty-shell-ready.ts --items all
sed -n '1,90p' src/main/providers/local-pty-shell-ready.tsRepository: stablyai/orca
Length of output: 5697
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- path imports and path construction ---'
rg -n "from 'node:path'|getFishShellReadyWrapperPath|getShellReadyWrapperRoot|getRequiredShellReadyWrapperPaths|fish/orca\.fish|shell-ready" src test tests 2>/dev/null || true
printf '%s\n' '--- relevant implementation ---'
sed -n '1,115p' src/main/providers/local-pty-shell-ready.ts
sed -n '270,360p' src/main/providers/local-pty-shell-ready.ts
printf '%s\n' '--- repository test/config context ---'
git ls-files | rg '(^|/)(package\.json|.*local-pty-shell-ready.*|.*shell-ready.*|tsconfig.*)$' | head -80Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- source path helpers and usages ---'
sed -n '1,85p' src/main/providers/local-pty-shell-ready.ts
rg -n -C 5 "getFishShellReadyWrapperPath|ensureShellReadyWrappersAt|getRequiredShellReadyWrapperPaths" src/main/providers src/shared
printf '%s\n' '--- local shell-ready tests ---'
sed -n '1,45p' src/main/providers/local-pty-shell-ready.test.ts
sed -n '270,415p' src/main/providers/local-pty-shell-ready.test.ts
printf '%s\n' '--- Windows and WSL call sites ---'
rg -n -C 8 "ensureShellReadyWrappersAt|fish/orca\.fish|ORCA_FISH_SHELL_READY_WRAPPER|_orca_shell_ready_root" src/main/providers src/sharedRepository: stablyai/orca
Length of output: 36454
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const path = require('node:path')
const roots = [
'/Users/alice/Library/Application Support/Orca/shell-ready',
'C:\\Users\\alice\\AppData\\Roaming\\Orca\\shell-ready',
'C:/Users/alice/AppData/Roaming/Orca/shell-ready'
]
for (const root of roots) {
console.log(JSON.stringify({
root,
template: `${root}/fish/orca.fish`,
join: path.join(root, 'fish', 'orca.fish'),
win32Join: path.win32.join(root, 'fish', 'orca.fish')
}))
}
JSRepository: stablyai/orca
Length of output: 1085
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- wrapper materialization ---'
sed -n '284,355p' src/main/providers/local-pty-shell-ready.ts
sed -n '365,438p' src/main/providers/local-pty-shell-ready.ts
printf '%s\n' '--- pathWin32 usage in this file ---'
rg -n -C 3 "pathWin32" src/main/providers/local-pty-shell-ready.ts
printf '%s\n' '--- Windows path behavior of the proposed change ---'
node - <<'JS'
const path = require('node:path')
const root = 'C:\\Users\\alice\\AppData\\Roaming\\Orca\\shell-ready'
const fish = path.win32.join(root, 'fish', 'orca.fish')
console.log(JSON.stringify({
fish,
currentDirectoryExtraction: fish.slice(0, fish.lastIndexOf('/')),
dirname: path.win32.dirname(fish),
sibling: path.win32.join(root, 'bash', 'rcfile')
}))
JSRepository: stablyai/orca
Length of output: 6180
Use join and dirname for wrapper paths.
Update wrapper path construction and directory extraction together. A join-only change makes the existing lastIndexOf('/') extraction fail on Windows.
Source: Coding guidelines
fish fstats fd 0 and Node's `input` pipe reports EISDIR there, so fish 3.x died with 'Unable to read input file: Is a directory' before running anything. macOS fish 4.8 tolerates the pipe, which is why this passed locally and failed on the Linux runner. Verified green against fish 3.6 in a Linux container.
|
CI caught a macOS-only assumption in the WSL guest-script test, now fixed on The harness delivered Fixed by opening a real file and passing its fd as Two things worth recording from the debugging:
|
local macOS Electron evidenceNative Windows/WSL Electron QA was unavailable because the paired endpoint timed out. This is the local macOS Electron fallback pass (re-validated with compliant UI keyboard entry). Exact commit: Input method (compliant)Commands were entered through the visible xterm ( Actions / results (PASS)Launched this PR build with unique Through the rendered Orca UI: opened New Terminal (real local Fish login shell). Wrapper materialized once at
Stub captures confirmed argv matrix + non-empty Screenshots (fresh UI-keyboard pass)
CleanupStopped only the QA Electron processes for this pass; deleted temporary profile, stub bin, extension, and captures. No product code changes; no merge. Verdict: PASS — no in-scope defects found under compliant UI keyboard reproduction. |
|
Closing because Orca must not redefine or rewrite typed Prime Agent commands. This Fish parity change extended an existing WSL command-interception workaround; the replacement will remove that interception across shells and preserve user argv unchanged. |
|
Nice PR — the fish twin of the POSIX wrapper is the right shape, and the parity test running both launch configs through one fake Two heads-ups from working #9993 today (#13904), since we collide. 1. This job installs fish 3.7, and that will bite the next 2031-dependent test
But DECSET 2031 arming lives in fish 4's Rust We both edit those same hunks, so whoever lands second takes a small mechanical conflict. Happy to be second and resolve it — your PR was up first. 2. I implemented STA-3937 and then dropped it, rather than collide with youWhile auditing fish support I hit the same gap you filed as STA-3937 and had it working: Then I read your PR and saw you'd already restructured that exact fish arm and filed the ticket, so I reverted all of it. Those two files are byte-identical to HEAD in #13904. Your delivery mechanism is better than mine anyway — a sourced wrapper file rather than my newline-joined What I did keep (no overlap with your files)
One interaction worth flagging: your change wrapping fish on the attribution path shifts what Also FYI, from the same investigation: #13892 — OSC/DSR replies get reordered behind CPR/DA1 by the POSIX echo-safe write path and strand into the next child's stdin. Hits fish hardest since it uses DA1 as its read sentinel. |
Summary
A user whose login shell is fish — on WSL, macOS, or Linux — launched Prime Agent normally but lost Orca's Prime working/status reporting. Fish is not POSIX, so
getPosixPrimeAgentShellWrapper()([[ ]],local,case/esac,shift 2) can never be sourced by it. Fish started cleanly and handedprime-agentthe user's bare argv; the--extensioninjection simply never happened. Fixes STA-3927, filed against merged #13430.What changed
src/main/pty/prime-agent-fish-shell-wrapper.ts— new. Fish twin of the POSIX wrapper:function … end,$argv/$argv[1],set -l,set -lxfor the function-scopedORCA_AGENT_HOOK_ENDPOINTexport,switch/casewith a quoted"--extension=*"(an unmatched glob is a hard error in fish),set -e argv[1..2]in place ofshift 2, andstring trim -r -c /for the HOME trailing-slash trim.shell-ready/fish/orca.fish, alongside the existingbash/rcfileandzsh/wrappers — not inlined into the launch argv. Inlining would have to surviveescapeWslShCommandForWindows(), which backslash-escapes every$.src/main/shell-templates.ts—getFishShellReadyWrapperFileContent()plusgetFishInitCommand(), which folds the wrappersourceand the existing shell-ready marker into one-Cstring. Multi--Csupport varies by fish version, so one combined init command is the safe shape.src/main/providers/local-pty-shell-ready.ts,src/main/daemon/shell-ready.ts, and a newfish)arm inbuildWslInteractiveLoginShellCommand()(src/shared/wsl-login-shell-command.ts). The fish arm is guarded by[ -f … ], so a failed wrapper write still falls through to the shared bareexec "$_orca_wsl_shell" -lrather than erroring onsource.args: nullthere, which left a plain fish pane — exactly where a user typesprime-agentby hand — unwrapped. The marker itself stays gated onORCA_SHELL_READY_MARKERinside the init text, so a markerless pane still emits nothing.Every semantic of the bash wrapper is preserved and tested: typed launches, explicit launches, bare
attachvsattach <agent>, the management-subcommand skip list, both--daemon-socketforms beforestop/rename, explicit--extension/--extension=passthrough, and the degrade-to-original-argv path when the managed extension file is missing.The existing fish shell-ready marker behavior is unchanged — it still rides
--on-event fish_promptand still fires exactly once.Deliberately out of scope (one thing per PR); follow-ups filed:
getPosixOmpShellWrapper)CODEX_HOME,OPENCODE_CONFIG_DIR,MIMOCODE_HOME, and the attribution / agent-teams shim PATHsbuildWslLoginShellCommand()routing fish to/bin/sh -lcfor non-interactive detection probesORCA_PRIME_AGENT_STATUS_EXTENSIONis missing from theneedsNoMarkerWrapperenv list (affects bash and zsh too), found while working this fixScreenshots
No visual change.
Testing
pnpm lintpnpm typecheckpnpm testpnpm build— not run. No build-surface change: this PR touches only main-process TypeScript and the shell text it generates, all of whichpnpm typecheckcovers. No packaging, native module, or asset change.src/main/pty/prime-agent-fish-shell-wrapper.test.ts(new, 33 cases)prime-agent-shell-wrapper.test.tsagainst real fish — the whole argv matrix, plus fish-specific hazards:set -e argv[1..2]when attach has no trailing args, multi-word operands surviving a bare$argv, and theHOMEtrailing-slash trimsrc/main/pty/prime-agent-shell-wrapper-parity.test.ts(new)prime-agentand asserts identical argv capturesrc/main/daemon/shell-ready-fish.node-pty.test.ts(new)prime-agentis wrapped in a real login shell). Extracted becauseshell-ready.test.tswould otherwise cross themax-linesceiling — no suppression was addedsrc/main/providers/wsl-fish-login-shell.test.ts(new)sh -c "<script>") driving a real fish, with a shadowedgetentpinning the login shell. Covers the wrapped case, the missing-wrapper fallback, and a/bin/shcontrol proving the branch is actually being steeredsrc/main/providers/windows-shell-args.test.ts,src/main/daemon/shell-ready.test.ts,src/main/providers/local-pty-shell-ready.test.ts,src/shared/wsl-login-shell-command.test.ts-C, the markerless attribution config, rewrite-on-missing, and the WSL fish arm including itsescapeWslShCommandForWindowsform.github/workflows/pr.yml--excludelist. Without this they gate off a missing binary and skip silently — see AI Review belowEvery oracle asserts through filesystem markers (a fake
prime-agentthat writes its argv to a capture file), never by scraping the terminal buffer.RED→GREEN was verified by reverting the fix in place: the parity oracle's fish capture collapses to
BEGIN\nARG=ask\nEND\n— exactly the signature an independent repro recorded at HEAD — and the guest-script execution test fails with the same shape. Both go green with the fix.Commands run:
The WSL guest-script suite was additionally run against fish 3.6 in a Linux container, not just the local fish 4.8 — the first CI run failed there on a macOS-only assumption in the harness (see the PR comment) and this is how it was reproduced and confirmed fixed.
Note for anyone reproducing locally:
src/main/ipc/pty.test.tsfails 10 tests when the suite is run from inside an Orca terminal — the ambientZDOTDIR/ORCA_*env leaks into its spawn-env assertions. It fails the same 10 atorigin/main, and passes clean underenv -i. Unrelated to this PR.AI Review Report
Ran
/code-reviewat high effort over the full diff, plus a self-audit. The review built both wrappers to a scratch dir and ran a bash-vs-fish argv differential across ~20 cases (no args,--help/-h/--version/-v, glob/empty/newline/$-bearing operands,attachwith and without operands, both--daemon-socketforms, explicit--extension/--extension=,HOMEwith trailing slashes and spaces, the WSL endpoint export). The fish wrapper came back byte-for-byte argv-identical to the bash wrapper in every case, including theset -lxexport reaching the child through the nested launch function. No correctness defect was found in the generated fish code.It raised three issues, all fixed in this PR:
shell contractsjob installs fish; it enumerates its test files explicitly. All the new fish suites landed in the shardedtestjob, which has no fish, sohasFishwas false and every one skipped silently — and moving the live-fish PTY test out ofshell-ready.test.tshad also removed the repo's only executed fish coverage. Fixed by adding all four files to the shell-contracts run list and the shard--excludelist, with a comment on the job saying any new live-fish test must be added to both.getAttributionShellLaunchConfig('fish')still returnedargs: null, so an ordinary fish pane reproduced the original symptom for a hand-typedprime-agent, while bash/zsh got their wrapper unconditionally. Fixed by dropping theemitReadyMarkergate on the fish branch in both launch paths and gating only the marker.getentbeing absent so the script would fall back to$SHELL— true on macOS, false on every CI runner, wheregetentreturns the account's real shell and the script dispatches tobash)while the assertions still pass off the bash wrapper. Fixed by shadowinggetentonPATHto pin the login shell, and adding a/bin/shcontrol that must come back unwrapped — so a shadow that stopped steering the branch fails the suite.Other risks checked in the self-audit:
set -l/set -lxscope to the enclosing block in fish, not the function. A naive port would let theORCA_AGENT_HOOK_ENDPOINTexport die at the closingendbeforeprime-agentran; the wrapper keeps the launch inside the same block (one duplicated call, commented).quoteFishSingle(escapes\and'), verified against a real path containing both'and$. The WSL path cannot use a literal at all — the root is a WSLENV-translated Windows path, fish re-expands$inside a double-quoted literal, and a single-quoted one cannot carry an apostrophe from a Windows profile name. It passes the path through an exported variable (a variable's value is never re-expanded) and scrubs the temp var withset -ein the same init command.win32exactly as before, the fish arm only executes inside a WSL guest, and no path is concatenated with a hardcoded separator (the daemon path usesjoin; the local provider keeps its existing POSIX-only template style since that root is consumed only by POSIX shells). No keyboard shortcut, accelerator, or UI label touched.string match/string trim -c,set -eslice deletion,functions -e,set -lx,switch) predates fish 3.0.--no-configis fish 3.3+ but appears only in test harnesses.Security Audit
--extension "$ORCA_PRIME_AGENT_STATUS_EXTENSION"into acommand prime-agentcall. That value originates in Orca's owntitlebar-extension-service, not user input, and is double-quoted so fish cannot word-split or glob it.commandis used throughout so a user-definedprime-agentcannot recurse into the wrapper.$in the user's profile path.shell-ready/fish/orca.fish, written mode0644into the existing user-owned userData dir by the sameensureShellReadyWrapperscode that already writes the bash and zsh wrappers, under the same try/catch that degrades to an unwrapped shell on failure. No new trust boundary.ORCA_FISH_SHELL_READY_WRAPPERexists only long enough for fish to read it and is erased inside the same init command, so it does not linger in the user's interactive shell.Follow-up needed: none for this change. The four filed tickets are pre-existing gaps, not regressions introduced here.
Notes
WSL guest execution was not validated. This was developed on macOS; real WSL cannot be exercised there. The WSL half is covered two ways short of that: assertions on the generated command string (including its
escapeWslShCommandForWindowsform), and an actual execution of the generatedshscript driving a real fish with a pinned login shell. What that does not exercise iswsl.exeitself, WSLENV/ppath translation, or/mnt/cfile access from the guest. Someone on a Windows host with a fish-login WSL distro should confirm end to end.Behavior change worth a second look: fish panes that previously launched with no args now launch as
fish -l -C '<wrapper source + marker registration>'. That brings fish in line with how bash and zsh have always been launched, and the marker stays inert unlessORCA_SHELL_READY_MARKER=1, but it is a wider blast radius than the WSL-only report implies.The RED baseline came from an independently built repro; its real-shell oracle approach — fake binary plus filesystem argv capture, no terminal scraping — is what the parity suite here is adapted from.