Skip to content

fix(terminal): wrap zsh when scoped history is the only reason to (STA-4682) - #15197

Closed
nwparker wants to merge 3 commits into
mainfrom
nwparker/sta-4682-gate-zsh-wrapper-on-scoped-history
Closed

nwparker wants to merge 3 commits into
mainfrom
nwparker/sta-4682-gate-zsh-wrapper-on-scoped-history

Conversation

@nwparker

@nwparker nwparker commented Aug 18, 2026 •

Copy link
Copy Markdown
Contributor

ELI5

Orca gives each worktree its own shell history file. On macOS, /etc/zshrc overwrites HISTFILE on every zsh startup, so Orca launches zsh through a small wrapper that puts the right value back afterwards. But that wrapper was only turned on when something else needed it (a Codex/OpenCode overlay, or a startup command). A plain terminal pane got no wrapper — so its per-worktree history was quietly thrown away and every worktree shared ~/.zsh_history. This turns the wrapper on when a scoped history is the only reason to need it, without changing anything else about how those panes behave.

What Changed

ORCA_HISTFILE (set only when this spawn injected a worktree-scoped history) now counts as a reason to launch zsh through the ZDOTDIR wrapper, on all three launch paths:

  • src/relay/pty-shell-launch.ts — one line in hasOverlayRestoreEnv. Ordering was already correct on both relay paths: injectRelayHistoryEnv runs before getRelayShellLaunchConfig in the spawn path and in the revive path.
  • src/main/daemon/pty-subprocess.ts — added to the no-marker gate. ORCA_HISTFILE is injected client-side in daemon-pty-adapter.ts (withHistoryIsolation) before opts cross into the subprocess, so it is present when the gate runs.
  • src/main/providers/local-pty-provider.ts — injectHistoryEnv runs ~65 lines after the wrapper gate, so ORCA_HISTFILE does not exist yet there. Rather than reorder injection (its output feeds the WSL/Codex WSLENV branches), a second, narrow decision runs right after injection and applies the wrapper only when the first gate declined. It also sets getFallbackShellReadyConfig so a zsh→bash fallback stays consistent with updateHistoryEnvForFallback.
  • New src/main/pty/zsh-history-restore-wrapper.ts holds the shared predicate (ORCA_HISTFILE present and shell basename is exactly zsh) so desktop and daemon cannot drift.

Review-driven corrections are in the follow-up commits:

1. An inherited ORCA_HISTFILE can no longer wrap a pane (regression guard for #11146)

The gate keys off ORCA_HISTFILE, and both the daemon fork and the relay server merge their own process.env into every spawn env. Orca launched from inside an Orca pane inherits that pane's exported ORCA_HISTFILE. Before this fix, a pane that took an early return — history scoping OFF, no worktreeId, or a reconnect where sessionId !== undefined && isNewSession !== true — would have been wrapped and pointed at the launching worktree's history file. Pre-PR that inherited value was inert on an unwrapped pane; the new gate made it live.

  • src/main/daemon/pty-subprocess.ts now calls removeInheritedOrcaHistFile(env, opts.env) next to removeUnspecifiedPaneIdentityEnv — the value survives only when this spawn's opts.env carries it.
  • src/relay/pty-handler.ts deletes ORCA_HISTFILE at the end of buildSpawnEnv, so only injectRelayHistoryEnv (which runs after) can set it. A client-sent one names a path on the client and is dropped too.
  • The desktop already had this guard (local-pty-provider.ts, "Credit: fix(terminal): keep worktree HISTFILE after macOS /etc/zshrc #11146"); it is now pinned by a test that seeds process.env.ORCA_HISTFILE with scoping off.

2. History-only wrapping no longer enrolls a pane in OSC 133

The wrapper .zshrc registered the OSC 133 precmd/preexec hooks unconditionally (outside the [[ ! -o login ]] guard, so login panes too). Turning the wrapper on for plain panes would therefore have made every command in nearly every zsh pane emit 133;A/C/D, which feeds handleCommandFinished → paneForegroundAgentTracker.onCommandFinished()/dropCommandFinishedStatusIfSameTurn and dispatchTerminalCommandFinishedEvent → the git-status push-signal refresh. That is a real behavior change for panes that emitted no markers at all before, and it is not what this PR is for.

  • getZshOsc133RegistrationBlock() in src/main/shell-templates.ts now owns that block (it was duplicated verbatim in the desktop and daemon rcfiles) and gates it on ORCA_SHELL_COMMAND_MARKERS — absent or non-0 means register, exactly as today.
  • getHistoryRestoreShellLaunchConfig() (desktop + daemon) is the markerless zsh wrapper plus ORCA_SHELL_COMMAND_MARKERS=0, and is used only when a scoped history is the sole reason to wrap. Any pane wrapped for an overlay or a startup command is untouched and still registers OSC 133.
  • Non-zsh shells get no wrapper from this path at all, so a zsh→bash fallback stays exactly as it was.
  • The relay needs nothing here: its zsh overlay wrapper never had an OSC 133 block (only its bash rcfile does).

3. The OSC 133 suppression no longer escapes the pane that needs it

ORCA_SHELL_COMMAND_MARKERS=0 goes into the pane's PTY env, so zsh exports it to every child of that pane, and nothing unset or scrubbed it. Since this PR makes a plain zsh pane history-only wrapped, running pn dev or the orca CLI from one handed that 0 to the new Orca's process.env — and both local-pty-provider.ts ({...process.env, ...args.env}) and pty-subprocess.ts merge process.env into every spawn env, while getWrappedShellLaunchConfig pinned ORCA_SHELL_READY_MARKER/ORCA_SHELL_STARTUP_IDENTITY but not this one. The inherited 0 therefore survived Object.assign(finalEnv, shellLaunch.env) and suppressed the hooks for every Codex/OpenCode/agent-teams overlay pane and every startup-command pane that instance spawned: no 133;A/C/D, so handleCommandFinished → paneForegroundAgentTracker.onCommandFinished() never fired and agent rows stayed stuck on "working". Same inheritance-leak class as correction 1, which was fixed in production for ORCA_HISTFILE but only in the test harness for this flag.

Two layers, because they close different paths:

  • Pinned ORCA_SHELL_COMMAND_MARKERS: '1' in the zsh branch of getWrappedShellLaunchConfig (desktop and daemon), beside the two markers already pinned there. This is what protects an Orca instance whose process.env is already polluted — by an older build, or by any route other than the wrapper. Sitting next to the existing pins also makes it much harder to drop silently in a later edit.
  • unset ORCA_SHELL_COMMAND_MARKERS at the end of getZshOsc133RegistrationBlock(), right after the gate is read. This stops the export at its source, so nothing downstream of a history-only pane ever sees it — not just Orca, but relay servers, daemons, and user scripts. Mirrors the existing unset ORCA_SHELL_STARTUP_IDENTITY in SHELL_STARTUP_IDENTITY_MARKER_BLOCK. Only the one gate reads the variable, so unsetting after it is free.

Deliberately not added: a third scrub beside removeInheritedOrcaHistFile. It would be dead weight — the pin already overrides the inherited value on every wrapped path, and on an unwrapped pane the variable has no reader at all. The relay needs nothing either: its zsh overlay wrapper has no OSC 133 block.

4. A failed wrapper write no longer costs the user their whole zsh config

ensureShellReadyWrappersAt swallows write errors and logs "Shell will launch without wrapper", but getWrappedShellLaunchConfig returned ZDOTDIR: <root>/zsh regardless. On a persistent failure (read-only userData, bad perms, full disk) zsh would launch pointed at a directory with no .zshenv/.zshrc and none of the user's zsh config would be sourced. Pre-existing, but this PR would have extended it from agent-only panes to essentially every macOS/Linux zsh pane.

getHistoryRestoreShellLaunchConfig (desktop + daemon) now returns the plain unwrapped launch when shellReadyWrappersExist() is false. Deliberately scoped to this PR's new path only: gating the ready-marker and overlay configs the same way would silently disable startup-command delivery for agent panes on a false negative, which is a separate blast radius and belongs in its own change. The cost here is losing scoped history for that pane — exactly the pre-PR behavior — instead of losing the user's shell config.

For accuracy on one point from review: the git-status consumer is not undebounced. useGitStatusPushSignalRefresh's fetchStatus is statusSchedulerRef.current.signal(), which goes through createGitStatusRefreshScheduler (activity debounce plus an activityMinGapMs floor, #7983). The change was still worth avoiding — the agent-status path and the per-command fan-out are real — but the consequence is bounded by that scheduler.

Test hermeticity

  • src/main/providers/local-pty-provider-test-harness.ts and src/main/daemon/pty-subprocess-test-harness.ts now clear inherited ZDOTDIR/HISTFILE/ORCA_* wrapper and overlay markers. The runner is normally itself a child of an Orca zsh pane, so those vars sit in process.env; without this, wrapper assertions pass vacuously on a dev machine and unwrapped ones fail for unrelated reasons (I hit exactly this while writing the tests, and again in the new relay test — which scrubs the overlay keys locally for the same reason).

Why

/etc/zshrc on macOS contains an unguarded HISTFILE=${ZDOTDIR:-$HOME}/.zsh_history and runs before any file the user controls, which is why ZSH_HISTFILE_RESTORE_BLOCK exists (#11044). Verified on this machine:

$ HISTFILE=/tmp/orca-scoped/zsh_history ORCA_HISTFILE=/tmp/orca-scoped/zsh_history \
  HOME=/tmp/fakehome-orca zsh -li -c 'echo $HISTFILE'
/tmp/fakehome-orca/.zsh_history

The restore block only runs from the wrapper .zshrc/.zlogin, and the wrapper is only reached via ZDOTDIR. So on the desktop path — history isolation ON (default, terminalScopeHistoryByWorktree: true), no Codex managed home, no OpenCode overlay, no agent-teams shim, no startup command — the feature was a no-op: every worktree shared one history. The relay had the same hole but far narrower, since ORCA_REMOTE_CLI_BIN_DIR is set whenever the handshake reports hostPlatform; it only opened against an older host that reports none.

Blast radius (the main review risk — please read)

History isolation is default-ON, so this puts nearly every plain macOS/Linux zsh pane onto the ZDOTDIR wrapper — the exact path the fast path existed to avoid. Stating that plainly rather than burying it:

  • Startup cost, measured on this machine (/bin/zsh -li -c exit, empty $HOME, 20 launches each, real generated wrappers): plain 12 ms, wrapped 14 ms → ~2 ms fixed overhead. The wrapper sources four extra small files; the user's own rc files run either way and dominate.
  • Command lifecycle: unchanged. History-only wrapping ships ORCA_SHELL_COMMAND_MARKERS=0, so these panes emit no OSC 133 and drive no command-finished consumers — same as before the PR. Panes wrapped for an overlay or a startup command are unaffected by the gate: the flag is pinned to '1' in every wrapped launch config, and the wrapper unsets it after reading it, so the suppression cannot reach a child process or a later Orca instance (see correction 3).
  • Wrapper generation failure is now a clean degrade for this path. If the wrapper files cannot be written, a history-only pane launches unwrapped rather than with a ZDOTDIR pointing at an empty directory. The ready-marker and overlay configs still return their ZDOTDIR unconditionally on that failure — pre-existing, unchanged here, and called out in correction 4.
  • Output: unchanged. Markerless config (ORCA_SHELL_READY_MARKER=0), so no OSC 777 ready marker either.
  • ZDOTDIR-sensitive user configs: the wrapper re-exports the user's ZDOTDIR before sourcing each of their startup files and restores it at the end (getZshFinalZdotdirRestoreBlock), the same contract already in production for every agent pane.
  • Scoped to zsh only. bash is untouched on the desktop/daemon paths even though injectHistoryEnv sets ORCA_HISTFILE for it too: no common /etc/bashrc clobbers HISTFILE, and wrapping bash would be a much larger behavior change (it also switches the pane to --rcfile).
  • Not reached when isolation is off (ORCA_HISTFILE is deleted on all three paths), when the caller supplied its own HISTFILE (injection early-returns without setting ORCA_HISTFILE), or when the history dir could not be created.
  • Relay attach-only revive still gets no ORCA_HISTFILE and therefore no wrapper — unchanged, and deliberately not asserted in tests.

Linked Issue

STA-4682 (claim 3). Follow-up to #15018. Same failure mode as #11044; regression guard for #11146.

Fixes #

Visual Proof

N/A — shell launch configuration only; no UI surface changes.

Testing

Platform: macOS (Apple Silicon, Darwin 25.3), real /bin/zsh 5.9 — every command and vacuity check below was run there and nowhere else. Not manually exercised on Linux/Windows/SSH; the daemon and relay paths are covered by unit tests, and the Windows/WSL paths are explicitly excluded by the gate.

npx oxlint <15 changed files>                                       # clean
pnpm typecheck                                                      # node + cli clean;
    # web project fails on src/renderer/.../open-tab-search-retention.test.ts (2x TS2741)
    # and useSessionRestoredBannerDismiss.test.tsx (TS2740) — pre-existing,
    # reproduced on a clean origin/main checkout via git stash
npx vitest run --config config/vitest.config.ts \
  src/main/zsh-scoped-histfile.live-shell.test.ts \
  src/main/daemon/pty-subprocess-managed-agent-env.test.ts \
  src/main/daemon/pty-subprocess.test.ts src/main/daemon/shell-ready.test.ts \
  src/main/daemon/pty-subprocess-env-inheritance.test.ts \
  src/main/daemon/pty-subprocess-wsl-launch.test.ts \
  src/main/daemon/repro-13767-shell-ready-marker-lost-to-exec.test.ts \
  src/main/providers/local-pty-provider-spawn-env.test.ts \
  src/main/providers/local-pty-shell-ready-wrapper-generation.test.ts \
  src/main/terminal-history.test.ts \
  src/relay/pty-shell-launch.test.ts src/relay/pty-handler-spawn-environment.test.ts \
  src/relay/terminal-history.test.ts
    # all passed

# round 3 (leak fix), macOS:
npx oxlint <8 changed files>                                        # clean
npx tsc --noEmit -p config/tsconfig.node.json                       # clean
npx vitest run --config config/vitest.config.ts \
  src/main/providers src/main/daemon src/relay
    # 3630 passed; the 10 failures are pre-existing terminal-snapshot/headless-emulator
    # cases (terminal-snapshot-osc8-roundtrip, terminal-snapshot-color-parity,
    # headless-emulator-fidelity.fuzz) — reproduced identically on a detached
    # origin/main checkout in the same worktree

Vacuity check (each new test fails without the fix)

All rows below were measured on macOS (Apple Silicon, Darwin 25.3, /bin/zsh 5.9). Not re-measured on Linux or Windows. That matters for the live-shell rows: their premise is that a system /etc/zshrc clobbers an injected HISTFILE, which macOS always does and a stock Linux image usually does not. zsh-scoped-histfile.live-shell.test.ts now probes for that clobber at module load and skips the cases that depend on it (itWithHistfileClobber) when it does not happen, so those cases report "skipped" on such a host instead of passing vacuously. The OSC 133 case does not depend on the clobber and runs everywhere zsh exists.

Test Source reverted Result without fix
pty-shell-launch.test.ts › wraps zsh when a worktree-scoped history must survive shell startup src/relay/pty-shell-launch.ts FAIL
zsh-scoped-histfile.live-shell.test.ts › keeps the scoped HISTFILE for a relay plain pane (real zsh) src/relay/pty-shell-launch.ts FAIL — real zsh returned the /etc/zshrc default instead of the scoped path
local-pty-provider-spawn-env.test.ts › uses shell wrapper when worktree-scoped history must survive shell startup src/main/providers/local-pty-provider.ts FAIL
pty-subprocess-managed-agent-env.test.ts › uses shell wrapper when worktree-scoped history must survive shell startup src/main/daemon/pty-subprocess.ts FAIL
zsh-scoped-histfile.live-shell.test.ts › keeps the scoped HISTFILE for a local plain pane (real zsh) requiresZshHistoryRestoreWrapper forced to false FAIL — the test then launches the unwrapped login shell the old gate picked, and /etc/zshrc clobbers HISTFILE
pty-handler-spawn-environment.test.ts › drops an inherited ORCA_HISTFILE when the pane has history scoping off delete result.ORCA_HISTFILE in buildSpawnEnv FAIL — pane wrapped, ZDOTDIR set under the temp HOME
pty-subprocess-managed-agent-env.test.ts › drops an ORCA_HISTFILE the daemon inherited from the pane that launched it removeInheritedOrcaHistFile call FAIL
local-pty-shell-ready-wrapper-generation.test.ts › OSC 133 block gated on ORCA_SHELL_COMMAND_MARKERS gate replaced with if [[ 1 == 1 ]] FAIL
zsh-scoped-histfile.live-shell.test.ts › local plain pane registers no __orca_osc133_precmd (real zsh, reads $precmd_functions) gate removed / ORCA_SHELL_COMMAND_MARKERS not set by the launch config FAIL
local-pty-provider-spawn-env.test.ts + pty-subprocess-managed-agent-env.test.ts › ORCA_SHELL_COMMAND_MARKERS === '0' for history-only wrapping ORCA_SHELL_COMMAND_MARKERS renamed in both launch configs FAIL
local-pty-provider-spawn-env.test.ts › ignores an inherited command-marker suppression for an overlay pane / for a startup-command pane ORCA_SHELL_COMMAND_MARKERS: '1' pin in local-pty-shell-ready.ts FAIL — inherited '0' reached the spawn env
pty-subprocess-managed-agent-env.test.ts › ignores a command-marker suppression the daemon inherited from its launcher same pin in daemon/shell-ready.ts FAIL
zsh-scoped-histfile.live-shell.test.ts › keeps a history-only pane out of OSC 133, without leaking that to children (real zsh) unset ORCA_SHELL_COMMAND_MARKERS in getZshOsc133RegistrationBlock() FAIL — real zsh still had ORCA_SHELL_COMMAND_MARKERS=0 set (and would export it)
local-pty-shell-ready-wrapper-generation.test.ts › leaves a history-only pane unwrapped when the wrapper files could not be written shellReadyWrappersExist() guard in local-pty-shell-ready.ts FAIL — returned the wrapper ZDOTDIR anyway
pty-subprocess-managed-agent-env.test.ts › leaves a history-only pane unwrapped when the wrapper files could not be written same guard in daemon/shell-ready.ts FAIL

Negative tests added alongside each (plain zsh with no Orca env still takes the unwrapped fast path; history scoping off ⇒ no wrapper even with an inherited ORCA_HISTFILE; an overlay-wrapped pane still registers OSC 133; a caller-injected ORCA_HISTFILE is preserved).

The live-shell cases are the coverage that was missing: the original ones pre-write the wrapper ZDOTDIR and can only prove the wrapper content is right. The new ones build the launch config through the real config functions and spawn real zsh with exactly that args/env pair. Their docblock now says so explicitly — they do not drive LocalPtyProvider.spawn; which panes reach those functions is unit-covered in local-pty-provider-spawn-env.test.ts and pty-subprocess-managed-agent-env.test.ts.

  • I manually tested these changes locally
  • Automated tests added/updated, or explained why not below

AI Disclosure

Claude Opus.

Review

Highest-value things to check:

  1. The ORCA_SHELL_COMMAND_MARKERS polarity and containment: absent means register, so every existing wrapper user is unchanged and only the new history-only launch opts out. It is pinned to '1' in both wrapped launch configs and unset by the wrapper immediately after the gate reads it, so an inherited value can neither reach a wrapper nor escape the pane that set it. Wrapper files are rewritten on the first ensureShellReadyWrappers call of each process, so there is no stale-file window.
  2. The two inherited-ORCA_HISTFILE scrubs: daemon keeps a value only when opts.env carries it (mirrors removeUnspecifiedPaneIdentityEnv); the relay drops it unconditionally in buildSpawnEnv because only injectRelayHistoryEnv, which runs after, may set it there.
  3. The desktop placement: the new decision runs after injectHistoryEnv and only when the first gate produced no config (posixShellLaunch === null). Nothing between there and spawnShellWithFallback mutates shellPath, and for zsh the wrapper's args are ['-l'], identical to the existing default — only env changes.

Agent skill upstream boundary

  • Not applicable.

Notes

Deliberately scoped out of this PR (STA-4682 has five claims):

  • Claim 5 (inherited fish_history) — separate PR.
  • Claim 2 (Windows→WSL over the relay gets no scoped history injection) — a real gap, deliberately not fixed here. Injecting fish_history into a WSL guest creates a file only a guest-side wsl.exe --exec fish can find; deleteRelayHistory hardcodes ['bash_history','zsh_history'] under a single root; and the relay revive path calls resolveDefaultShell() and ignores the stored shell override. Injecting without the matching deletion counterpart would be strictly worse than today's no-op, so it needs its own design.
  • Claim 1 — investigated, not a defect: no caller can make the relay spawn env's HOME/XDG_DATA_HOME differ from the relay process's, so the path is unreachable.
  • Claim 4 — investigated, not a defect: a documented, measured tradeoff.
  • bash HISTFILE restoration — out of scope, see blast radius.
  • Relay revive with a persisted HISTFILE — injectRelayHistoryEnv early-returns on a caller-supplied HISTFILE, so a revived pane can end up with a scoped HISTFILE and no ORCA_HISTFILE. That behavior predates this PR and is unchanged by it.

Cross-platform: the desktop hook is guarded by !wslInfo && process.platform !== 'win32'; the daemon and relay gates only fire for a shell basename of exactly zsh (so never wsl.exe, cmd.exe, pwsh). Folder workspaces behave exactly like git worktrees here — the gate keys off worktreeId, which both have. Remote wire compatibility: no protocol change; ORCA_HISTFILE was already crossing the wire, only the host's local interpretation of it changed, and an old host simply keeps today's behavior. ORCA_SHELL_COMMAND_MARKERS is host-local env consumed by a wrapper file the same build writes, so it never crosses a version boundary.

Checklist

  • This PR is small and focused
  • I explained what changed and why (including ELI5)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, targeted vitest pass locally (full suite left to CI; pnpm build not run)

…A-4682)

zsh reached the ZDOTDIR wrapper only when an OpenCode/MiMo/Codex/agent-teams
overlay or a startup command asked for it. A plain pane whose sole Orca-injected
env was a worktree-scoped ORCA_HISTFILE took the unwrapped fast path, so nothing
ran the restore block and macOS /etc/zshrc reassigned HISTFILE back to
~/.zsh_history — worktree history isolation was a silent no-op on the default
path (#11044 all over again).

Gate the wrapper on the injected ORCA_HISTFILE in all three launch paths:
desktop (LocalPtyProvider), daemon (pty-subprocess), and relay. On the desktop
the decision is re-run after injectHistoryEnv, since ORCA_HISTFILE does not
exist yet where the original gate is evaluated.
@coderabbitai

coderabbitai Bot commented Aug 18, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The change adds scoped zsh history detection and restoration for local and relay PTY launches. It removes unauthorized inherited ORCA_HISTFILE values, applies markerless history wrappers when required, and preserves command markers for overlays and startup commands. Shared OSC 133 registration is now used by zsh wrappers. Tests cover fallback behavior, environment cleanup, local and relay launches, and live zsh startup.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the main change: wrapping zsh when scoped history is the only reason to use the wrapper.
Description check ✅ Passed The description is detailed and covers the change, rationale, testing, risks, cross-platform impact, and required checklist items.
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

…h wrappers out of OSC 133

The scoped-history wrapper gate keys off ORCA_HISTFILE, which the daemon fork and
the relay both inherit from the Orca pane that launched them — reintroducing the
#11146 cross-worktree leak for panes with scoping OFF. Drop it on both sides
unless the spawn itself injected it, mirroring the desktop guard.

Wrapping for history alone also enrolled every plain zsh pane in OSC 133, which
drives agent-status drops and the git-status refresh signal. Gate the zsh
wrapper's OSC 133 registration on ORCA_SHELL_COMMAND_MARKERS and launch
history-only panes with it off, so their command lifecycle is unchanged.

Also scrub the wrapper/history env in the daemon test harness so those tests are
hermetic on a machine running inside a wrapped Orca pane.
… to children

The gate flag went into the pane's PTY env, so it was exported to every child. An Orca started from a history-only pane inherited it and silenced 133;A/C/D for every overlay and startup pane it spawned, stranding agent rows on 'working'.

Pin it in both wrapped launch configs and unset it in the wrapper after the gate is read. Also stop returning a wrapper ZDOTDIR for a history-only pane when wrapper generation failed, which would have dropped the user's whole zsh config.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/main/zsh-scoped-histfile.live-shell.test.ts (1)

241-253: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Add scoped-history handling to the relay launch configuration.

src/relay/pty-shell-launch.ts wraps zsh only for overlays or startup markers. With the environment at Line 244, it returns an unwrapped login launch and does not preserve HISTFILE on hosts that clobber it. Include ORCA_HISTFILE in the zsh wrapper condition. Set ORCA_SHELL_COMMAND_MARKERS to '0' only when scoped history is the sole wrapper reason.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5149a15e-2333-40c9-9ba6-58d3e8a857df

📥 Commits

Reviewing files that changed from the base of the PR and between 8615ea8 and 19dd9cf.

📒 Files selected for processing (16)
  • src/main/daemon/daemon-zsh-shell-ready-rcfile.ts
  • src/main/daemon/pty-subprocess-managed-agent-env.test.ts
  • src/main/daemon/pty-subprocess-test-harness.ts
  • src/main/daemon/pty-subprocess.ts
  • src/main/daemon/shell-ready.ts
  • src/main/providers/local-pty-provider-spawn-env.test.ts
  • src/main/providers/local-pty-provider-test-harness.ts
  • src/main/providers/local-pty-provider.ts
  • src/main/providers/local-pty-shell-ready-wrapper-generation.test.ts
  • src/main/providers/local-pty-shell-ready-wrapper-generation.ts
  • src/main/providers/local-pty-shell-ready.ts
  • src/main/pty/zsh-history-restore-wrapper.ts
  • src/main/shell-templates.ts
  • src/main/zsh-scoped-histfile.live-shell.test.ts
  • src/relay/pty-handler-spawn-environment.test.ts
  • src/relay/pty-handler.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/main/providers/local-pty-provider-spawn-env.test.ts
  • src/main/providers/local-pty-provider.ts
  • src/main/providers/local-pty-provider-test-harness.ts

Included review availability: Your plan includes up to 10 reviews per rolling hour; 2 remain after this review.

Comment on lines +162 to +163
const blocked = join(mkdtempSync(join(tmpdir(), 'orca-daemon-userdata-')), 'userdata')
writeFileSync(blocked, '')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Remove the temporary test directory.

Each run leaves the directory created by mkdtempSync in the system temporary directory. Keep its parent path and remove it in finally.

Proposed fix
-    const blocked = join(mkdtempSync(join(tmpdir(), 'orca-daemon-userdata-')), 'userdata')
+    const userDataPath = mkdtempSync(join(tmpdir(), 'orca-daemon-userdata-'))
+    const blocked = join(userDataPath, 'userdata')
@@
     } finally {
       consoleError.mockRestore()
       process.env.ORCA_USER_DATA_PATH = previousUserDataPath
+      rmSync(userDataPath, { recursive: true, force: true })
       if (platform) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const blocked = join(mkdtempSync(join(tmpdir(), 'orca-daemon-userdata-')), 'userdata')
writeFileSync(blocked, '')
const userDataPath = mkdtempSync(join(tmpdir(), 'orca-daemon-userdata-'))
const blocked = join(userDataPath, 'userdata')

@nwparker

Copy link
Copy Markdown
Contributor Author

Flagging this for a human review rather than merging it with the rest of the batch.

The underlying defect is real and confirmed: hasOverlayRestoreEnv / the desktop and daemon wrapper gates check five overlay vars but not ORCA_HISTFILE, so a plain zsh pane skips the restore block and /etc/zshrc's unguarded HISTFILE=${ZDOTDIR:-$HOME}/.zsh_history wins. That is the #11044 failure on the path where the block never runs, and it had no test coverage.

What gives me pause is not any single finding, it is the shape of the review history. Each round fixed its predecessor's findings and introduced a new one in the same class:

  1. Gate keyed off ORCA_HISTFILE, but nothing scrubbed an inherited one — a pane opened with scoping explicitly off would write into the launching worktree's history, reintroducing the fix(terminal): keep worktree HISTFILE after macOS /etc/zshrc #11146 leak.
  2. Suppressing OSC 133 via ORCA_SHELL_COMMAND_MARKERS=0 put that value in the pane's PTY env, where it exported to every child — an Orca launched from such a pane gave its agent panes no OSC 133, leaving agent rows stuck on "working".
  3. Round 4 came back clean.

Both regressions were inheritance leaks through the pane env, which is the same mechanism as the bug being fixed. That is a sign the change sits on a surface where env flows are easy to get wrong, not that any individual fix was careless.

Blast radius is also the largest of the batch: this routes nearly every macOS/Linux zsh pane through the ZDOTDIR wrapper, a path that previously only agent panes took.

Two open nits from the final review, neither blocking:

  • src/relay/pty-shell-launch.ts — the relay wrapper generator has no try/catch around its mkdirSync/writeFileSync, unlike the local and daemon generators, and this PR newly routes plain remote zsh panes through it.
  • src/main/daemon/pty-subprocess-managed-agent-env.test.ts:162 — mkdtempSync result is never removed.

The other four PRs in this batch (#15193, #15194, #15195, #15196) are contained and went through the readiness checklist with zero P0. This one has not been through it.

🤖 Reviewed with Claude Code

@nwparker

Copy link
Copy Markdown
Contributor Author

Superseded by #15258, which is merged.

This PR's approach was abandoned rather than iterated. The root defect was structural: it used a negative, exported switch (ORCA_SHELL_COMMAND_MARKERS=0) as a cross-process control channel, so correctness depended on finding every place process.env is merged into a spawn env. It introduced a new regression in each of three review rounds, twice by that same mechanism.

#15258 inverts both properties. One ORCA_SHELL_FEATURES variable carrying a positive allowlist, consumed into a non-exported array and builtin unset in the wrapper's first executable lines — so it physically cannot reach a child, and an absent variable means nothing is enabled rather than everything.

It also lands the things this PR never got to:

  • ORCA_HISTFILE is consume-and-destroy, removing the root cause of fix(terminal): keep worktree HISTFILE after macOS /etc/zshrc #11146 instead of patching it with delete env.… calls.
  • Wrapper generation is fail-closed: a write failure yields a plain login zsh rather than ZDOTDIR pointing at an empty dir with the user's config unsourced.
  • A pane wrapped only for history gets no markers, so it is observably identical to today's unwrapped pane — no OSC 133, no git-status fetch per command. That was the main objection here.
  • Panes degrade to exactly the unwrapped behaviour when the user's config enters sh/ksh emulation, rather than to something worse.
  • A namespace rule test enforces that no generated wrapper file writes a shell global outside Orca's namespace, which closes the collision class this stack kept rediscovering (REPLY was the last instance).

The two nits raised here — the unguarded relay wrapper generator and the leaked mkdtempSync — are addressed there too.

Thanks for the review pressure on this one; the objections raised here are what produced the design that shipped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant