Repository navigation
fix(terminal): restore agent sessions after interactive shell startup - #30978
brennanb2025 wants to merge 4 commits into
Conversation
📝 Walkthrough
Priority: ⬆️ High Merge Risk: 🟠 High · up to
|
|
A startup authentication question can hold the user’s input while Orca waits to restore a terminal command. After the old deadline, the restored command can become the answer to that question. This PR lets user input through immediately and gives the shell responsibility for executing the restored command after initialization. Independent review: BLOCKED at Four confirmed regressions remain open:
Three execution findings share a cause: running ordinary commands inside prompt instrumentation. Bounded reader experiments support a fish continuation, but no complete preserving continuation is established for the required Bash and zsh modes. The tested Bash input-file alternative also lets user prompt code consume the saved command, so it was rejected. No support reduction, changed editor/default, replacement shell, or inferred terminal typing was accepted. Separate existing support gate: startup configurations that replace the shell or enter relays still lose automatic restoration. The existing replacement-shell suite remains 4 failed / 10 passed / 2 skipped; those tests and the support requirement were retained. That product decision is separate from the four findings above. The final source readiness audit found no additional proven PR defect. A preexisting cleanup concern remains deferred after a bounded source trace: another host sharing the temporary directory can sweep a staged command file older than one hour while its shell still waits for startup input. The complete two-host failure was not reproduced. Staging failure with an oversized raw environment fallback also remains an unverified concern. Verification: nine named test files passed 170 tests during the review; isolated real-shell probes reproduced the findings and compared ordinary command entry. Full current-head CI fails on seven TypeScript errors: four direct test handle constructors omit the new required ownership flag, and three environment objects infer optional undefined values. The packaged Windows test stops on the same errors before its runtime flow. Genuine saved-agent restoration through the Electron renderer, a real reply after restoration, main-versus-branch screenshots, and real SSH QA were not completed. The QA queue ticket was released without machine admission or an app launch. Automated tests and shell probes do not count as that QA. No ready-for-merge claim or completed-review marker is issued. For Brennan’s review: no main-versus-branch screenshots are available; rendered restoration evidence remains required. The three execution boundaries that need a preserving design are Bash startup execution, fish prompt-event execution, and zsh initialization execution. |
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
09c4b172-005e-4c27-b081-df7d6a37bb6c
📒 Files selected for processing: 32
src/main/__fixtures__/shell-wrapper-snapshots/daemon-bash-rcfile.txtsrc/main/__fixtures__/shell-wrapper-snapshots/daemon-zsh-zshenv.txtsrc/main/__fixtures__/shell-wrapper-snapshots/local-zsh-zshenv.txtsrc/main/daemon/agent-startup-prompt-latency.node-pty.test.tssrc/main/daemon/daemon-bash-shell-ready-rcfile.tssrc/main/daemon/daemon-zsh-shell-ready-wrapper-spec.tssrc/main/daemon/pty-subprocess-env-retention.test.tssrc/main/daemon/pty-subprocess-io-failure-cleanup.test.tssrc/main/daemon/pty-subprocess-io-failure-native.test.tssrc/main/daemon/pty-subprocess-spawn-file-foreground.test.tssrc/main/daemon/pty-subprocess.tssrc/main/daemon/pty-subprocess/shell-launch-plan.tssrc/main/daemon/pty-subprocess/subprocess-handle.tssrc/main/daemon/repro-13767-shell-ready-marker-lost-to-exec.test.tssrc/main/daemon/session-shell-ready-barrier.tssrc/main/daemon/session-shell-ready-interactive-rc.test.tssrc/main/daemon/session-subprocess-handle.tssrc/main/daemon/session.test.tssrc/main/daemon/session.tssrc/main/daemon/shell-ready.tssrc/main/daemon/terminal-host-session-create.tssrc/main/providers/local-pty-finalize-environment.tssrc/main/providers/local-pty-launch-plan.tssrc/main/providers/local-pty-provider-shell-readiness.test.tssrc/main/providers/local-pty-session-activation.tssrc/main/providers/local-pty-shell-readiness-session.tssrc/main/providers/local-pty-spawn.tssrc/main/pty/posix-shell-startup-command.tssrc/main/runtime/terminal/output/headless-terminal-state.tssrc/main/shell-templates.tssrc/main/zsh-deferred-startup-line-init.tssrc/main/zsh-startup-wrapper-builder.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.
| ${BASH_PROMPT_COMMAND_COMPOSITION_BLOCK} | ||
| __orca_prepend_prompt_command "__orca_osc133_precmd" | ||
| __orca_append_prompt_command '__orca_in_debug_capture=1; __orca_prompt_had_functrace=""; if [[ -o functrace ]]; then __orca_prompt_had_functrace=1; set +T; fi; __orca_outer_debug_trap_spec="$(trap -p DEBUG)"; [[ -z "$__orca_prompt_had_functrace" ]] || set -T; unset __orca_prompt_had_functrace __orca_in_debug_capture' | ||
| ${getBashStartupCommandPromptBlock(SHELL_READY_MARKER)} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The restored bash command runs inside PROMPT_COMMAND, so OSC 133 C/D is never emitted for it.
This line adds __orca_run_startup_command to the daemon rcfile. It is appended after __orca_osc133_precmd and before __orca_osc133_epilogue. Here is what happens:
__orca_osc133_precmdsets__orca_in_prompt_command=1.__orca_run_startup_commandrunseval "$__orca_command"while that flag is still set.__orca_osc133_preexecreturns early at[[ -z "${__orca_in_prompt_command:-}" ]] || return 0, so it never prints133;Cand never sets__orca_in_command.- At the next prompt,
__orca_osc133_precmdfinds no__orca_in_command, so it never prints133;D.
The same thing happens on bash < 5.1, where the hook runs from __orca_prompt_command_suffix inside __orca_run_prompt_command_array.
Consequence: terminal-command-lifecycle never sees the restored agent start or exit. The stale "working" status that the C/D markers exist to clear (see the comment above __orca_osc133_precmd) comes back for every restored session.
Fix: have the startup hook mark the command boundary itself before it runs eval.
Proposed change in `getBashStartupCommandPromptBlock` (src/main/pty/posix-shell-startup-command.ts)
builtin history -s "$__orca_command" 2>/dev/null || true
builtin printf '%s\n' "$__orca_command"
+ # Prompt hooks suppress preexec; mark the restored command as a foreground command.
+ unset __orca_in_prompt_command
+ builtin printf "\\033]133;C\\007"
+ __orca_in_command=1
eval "$__orca_command"Update the snapshot fixture after this change.
| // Shell-owned delivery still needs file staging to stay below exec's environment limits. | ||
| const staging = | ||
| launch.startupCommandDeliveredByWrapper && opts.command | ||
| ? stageStartupCommand({ | ||
| command: opts.command, | ||
| shellPath: launch.shellPath, | ||
| orcaBuiltLine: opts.launchAgent !== undefined | ||
| }) | ||
| : undefined | ||
| if (staging) { | ||
| env[POSIX_SHELL_STARTUP_COMMAND_ENV] = staging.command | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Confirm how the typed path normalizes terminators and whether stripSubmitTerminator is exported.
rg -nP -C5 '\bstripSubmitTerminator\b|\bbuildStartupCommandSubmission\b' src/shared src/main | head -80Repository: stablyai/orca
Length of output: 6899
🏁 Script executed:
set -eu
printf '%s\n' '--- startup-command-staging.ts ---'
nl -ba src/shared/startup-command-staging.ts | sed -n '45,145p'
printf '%s\n' '--- direct consumers of startup-command environment ---'
rg -n -F --glob '*.{ts,tsx}' -e 'POSIX_SHELL_STARTUP_COMMAND_ENV' -e 'startupCommandDeliveredByWrapper' -e 'stageStartupCommand' src
printf '%s\n' '--- local finalize and daemon subprocess context ---'
nl -ba src/main/daemon/pty-subprocess.ts | sed -n '1,140p'
nl -ba src/main/pty/local-pty-finalize-environment.ts | sed -n '70,140p'
printf '%s\n' '--- wrapper-related source blocks ---'
nl -ba src/main/pty/posix-shell-startup-command.ts | sed -n '1,180p'
nl -ba src/main/shell-templates.ts | sed -n '170,240p'
nl -ba src/main/zsh-startup-wrapper-builder.ts | sed -n '140,205p'
printf '%s\n' '--- changed files versus requested base ---'
git diff --stat 98ee2688250027353232d6d9504c828ef636525b 0e4fd67307186799fd9857c0f1e2ed89121537d1 -- src/shared/startup-command-staging.ts src/main/daemon/pty-subprocess.ts src/main/pty/local-pty-finalize-environment.ts src/main/pty/posix-shell-startup-command.ts src/main/shell-templates.ts src/main/zsh-startup-wrapper-builder.tsRepository: stablyai/orca
Length of output: 32981
🏁 Script executed:
set -eu
nl -ba src/shared/startup-command-staging.ts | sed -n '45,145p'
printf '%s\n' '--- consumers and launch paths ---'
rg -n -F --glob '*.{ts,tsx}' -e 'POSIX_SHELL_STARTUP_COMMAND_ENV' -e 'startupCommandDeliveredByWrapper' -e 'stageStartupCommand' src
nl -ba src/main/daemon/pty-subprocess.ts | sed -n '1,140p'
nl -ba src/main/pty/local-pty-finalize-environment.ts | sed -n '70,140p'
printf '%s\n' '--- wrapper consumers ---'
nl -ba src/main/pty/posix-shell-startup-command.ts | sed -n '1,180p'
nl -ba src/main/shell-templates.ts | sed -n '170,240p'
nl -ba src/main/zsh-startup-wrapper-builder.ts | sed -n '140,205p'Repository: stablyai/orca
Length of output: 32513
🏁 Script executed:
set -eu
printf '%s\n' '--- local wrapper staging ---'
nl -ba src/main/providers/local-pty-session-activation.ts | sed -n '165,220p'
nl -ba src/main/providers/local-pty-finalize-environment.ts | sed -n '85,135p'
printf '%s\n' '--- daemon host staging ---'
nl -ba src/main/daemon/terminal-host-session-create.ts | sed -n '215,270p'
printf '%s\n' '--- staging call sites and tests ---'
rg -n -F --glob '*.{ts,tsx}' -e 'staging.command' -e 'staging.delivery' -e 'startupCommandStaging' -e 'stageStartupCommand({' src/main src/shared
nl -ba src/shared/startup-command-staging.test.ts | sed -n '25,225p'Repository: stablyai/orca
Length of output: 19730
🏁 Script executed:
set -eu
nl -ba src/main/providers/local-pty-session-activation.ts | sed -n '170,215p'
nl -ba src/main/providers/local-pty-finalize-environment.ts | sed -n '90,130p'
nl -ba src/main/daemon/terminal-host-session-create.ts | sed -n '225,265p'
printf '%s\n' '--- all staging result consumers ---'
rg -n -F --glob '*.{ts,tsx}' -e 'staging.command' -e 'staging.delivery' -e 'startupCommandStaging' src/main src/sharedRepository: stablyai/orca
Length of output: 8023
Strip submit terminators from every non-file staging result.
When stageStartupCommand does not create a file, it returns the original command unchanged. Its staging-failure path does the same. Wrapper delivery then evaluates the trailing \r as part of the command string. Normalize both fallback results.
🐛 Suggested fix
if (!shouldStageStartupCommand({ ...args, platform })) {
- return { command: args.command, delivery: 'typed' }
+ return { command: stripSubmitTerminator(args.command), delivery: 'typed' }
}
...
} catch (error) {
return {
- command: args.command,
+ command: stripSubmitTerminator(args.command),
delivery: 'typed-after-stage-failed',
failure: error instanceof Error ? error.message : String(error)Add coverage for a short command ending in \r and assert that the executed command does not receive the terminator.
| `${getFishShellReadyInitCommand(SHELL_READY_MARKER, features.includes('ready'), options?.startupCommand !== undefined)}\n${getFishCodexShellLaunchPreflight() + getFishClaudeShellFunction()}` | ||
| ], | ||
| env: {}, | ||
| env: startupEnv, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Inspect the fish template and every caller that enables startup-command delivery.
rg -nP -C4 'getFishShellReadyInitCommand\s*\(' src
sed -n 195,230p src/main/shell-templates.tsRepository: stablyai/orca
Length of output: 3556
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- daemon caller ---'
sed -n '160,230p' src/main/daemon/shell-ready.ts
printf '%s\n' '--- local caller ---'
sed -n '95,170p' src/main/providers/local-pty-shell-ready.ts
printf '%s\n' '--- startup env references ---'
rg -n -F --glob '*.ts' -e 'POSIX_SHELL_STARTUP_COMMAND_ENV' -e 'startupCommand' src/main | sed -n '1,240p'
printf '%s\n' '--- fish tests and related assertions ---'
rg -n -i -F --glob '*.{ts,tsx,test,spec}' -e 'fish_prompt' -e 'getFishShellReadyInitCommand' -e 'startup command' -e 'startupCommand' src test tests 2>/dev/null | sed -n '1,260p' || true
printf '%s\n' '--- fish availability ---'
command -v fish || true
fish --version 2>/dev/null || trueRepository: stablyai/orca
Length of output: 41900
🌐 Web query:
official fish shell documentation fish_prompt event commandline -f execute fish_preexec fish_postexec event handler job control
💡 Result:
Fish’s current official docs (fish 4.9.3) distinguish these related mechanisms:
- **`fish_prompt`** is a function fish runs when it is about to display the prompt; its output becomes the prompt. Separately, fish emits a named **`fish_prompt` event**, which you can subscribe to with `function --on-event fish_prompt`. ([fishshell.com](https://fishshell.com/docs/current/cmds/fish_prompt.html?utm_source=openai))
- **`fish_preexec` / `fish_postexec`** are named events for interactive commands: preexec fires right before execution and postexec right after. Register handlers with `function --on-event fish_preexec` or `--on-event fish_postexec`; the command line is passed as the first argument. ([fishshell.com](https://fishshell.com/docs/current/cmds/function.html))
- **`commandline -f execute`** queues the `execute` input function; `execute` runs the current command line. `commandline --is-valid` reports whether that line is complete and would execute when `execute` is called. ([fishshell.com](https://fishshell.com/docs/current/cmds/commandline.html?utm_source=openai))
- **Job control is separate from those events.** A foreground job takes control of the terminal; `&` starts a job in the background, `Ctrl-Z` suspends it, and `fg`/`bg` manage it. Fish also supports job-exit event handlers via `function --on-job-exit PID`; that is not the same as `fish_postexec`, which marks completion of an interactive command. ([fishshell.com](https://fishshell.com/docs/current/language.html))
In short: use **`fish_preexec`/`fish_postexec`** to observe interactive command execution, **`fish_prompt`** to customize prompt output (or subscribe to its event), and **job-exit handlers** when you need to react to a particular job ending.
Citations:
- 1: https://fishshell.com/docs/current/cmds/fish_prompt.html?utm_source=openai
- 2: https://fishshell.com/docs/current/cmds/function.html
- 3: https://fishshell.com/docs/current/cmds/commandline.html?utm_source=openai
- 4: https://fishshell.com/docs/current/language.html
🌐 Web query:
site:fishshell.com/docs fish event handler command execution job control fish_prompt eval fish_preexec fish_postexec commandline execute queued
💡 Result:
The fish documentation distinguishes **event hooks** from **queued command-line input**:
- `fish_preexec` fires just before an interactive command executes; `fish_postexec` fires just after. Both receive the command line as an argument, and neither fires for an empty command. These are event handlers, not documented mechanisms for inserting or scheduling another command. ([fishshell.com](https://fishshell.com/docs/4.6/language.html?utm_source=openai))
- `fish_prompt` is emitted when a prompt is about to appear. The `fish_prompt` function itself runs to generate the prompt; with transient prompts enabled, it may also run immediately before a command line executes. ([fishshell.com](https://fishshell.com/docs/4.6/language.html?utm_source=openai))
- `commandline -f …` queues **input functions** to be read before further keypresses. The docs do not say this queues arbitrary shell commands. `commandline --is-valid` only reports whether the current buffer could be executed by the `execute` input function. ([fishshell.com](https://fishshell.com/docs/current/cmds/commandline.html?utm_source=openai))
- `eval` evaluates its arguments as fish commands when called; its documentation does not describe it as a command-queueing or job-control facility. ([fishshell.com](https://fishshell.com/docs/current/cmds/eval.html?utm_source=openai))
**Bottom line:** these docs establish that event hooks run around interactive commands and that `commandline -f` queues input functions. They do not establish that a `fish_preexec`/`fish_postexec` handler can safely or directly execute a queued shell command. The cited event-handler detail is from the fish 4.6 language docs, while the command pages are for fish 4.9.3; behavior for another version may differ. ([fishshell.com](https://fishshell.com/docs/4.6/language.html?utm_source=openai))
Citations:
- 1: https://fishshell.com/docs/4.6/language.html?utm_source=openai
- 2: https://fishshell.com/docs/4.6/language.html?utm_source=openai
- 3: https://fishshell.com/docs/current/cmds/commandline.html?utm_source=openai
- 4: https://fishshell.com/docs/current/cmds/eval.html?utm_source=openai
- 5: https://fishshell.com/docs/4.6/language.html?utm_source=openai
Run the restored command through fish’s command-line execution path.
The daemon and local fish paths execute the restored command with eval inside a fish_prompt event handler. This bypasses the normal interactive command-line path and can skip fish_preexec/fish_postexec instrumentation. It may also change job-control behavior for long-running agents.
Queue the command for normal fish input execution after the prompt event returns. Do not execute it directly with eval from the event handler.
| ${ | ||
| spec.startupCommandDelivery | ||
| ? ` if [[ ! -o zle ]] && (( \${+${POSIX_SHELL_STARTUP_COMMAND_ENV}} )); then | ||
| local __orca_command="\$${POSIX_SHELL_STARTUP_COMMAND_ENV}" | ||
| builtin unset ${POSIX_SHELL_STARTUP_COMMAND_ENV} | ||
| if [[ "\${__orca_emit_ready_marker-1}" == 1 ]]; then | ||
| builtin printf "${spec.readyMarkerEscaped}" | ||
| __orca_emit_ready_marker="" | ||
| fi | ||
| builtin print -r -- "$__orca_command" | ||
| eval "$__orca_command" | ||
| fi` | ||
| : '' | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
In non-ZLE zsh, the restored command runs with Orca's function scope and options.
The eval "$__orca_command" runs inside __orca_deferred_init. That function starts with builtin emulate -L zsh. This causes three problems:
- Options: the command runs with zsh default options, not the options the user's
.zshrcset (for exampleextendedglob,nonomatch,ksharrays).LOCAL_OPTIONSthen discards any option the command changes. - Scope: the command runs in function scope. A
local,typeset, orreturninside the command, or inside a staged script it sources, acts on__orca_deferred_initinstead of the top-level shell. - Lifecycle markers: the command runs from a precmd or sched hook, so
preexec_functions(including__orca_osc133_preexec) never fire. No133;Cis emitted, and no133;Dfollows.
The ZLE path (BUFFER + zle accept-line) does not have these problems. Only the unsetopt zle case is affected. The PR review notes this as P1/P2.
Fix: do not run eval inside the emulated function.
- In
__orca_deferred_init, register a one-shot precmd function that is defined outsideemulate -L. - That function prints
133;C, sets__orca_in_commandwithtypeset -g, and runs the command at the user's option state. - The next
__orca_osc133_precmdthen printsD.
Then update daemon-zsh-zshenv.txt and local-zsh-zshenv.txt.
ELI5
When a shell startup file asks for an SSH-key passphrase, restoring a saved agent session can freeze the terminal. Orca holds both the resume command and the user’s answer until shell startup finishes; after a timeout it can submit the resume command as the passphrase and the passphrase as a shell command.
Blocked — do not merge. Two delivery designs have concrete failures: shell-owned execution changes command status, suspension, carriage-return handling and shell options; typed delivery can send a command into an unanswered startup question or append it to unfinished user input. The recovery behavior needs a product decision, and full review and real restoration QA remain incomplete.
This change lets the user answer startup questions immediately. On bash, zsh, and fish, the initialized shell executes the restored command once at its command prompt instead of Orca guessing when to type it.
What Changed
Why
A timeout, process name, or terminal mode cannot establish whether a password question or a command prompt is reading input. Shell-owned execution follows the common pattern and removes that guess for integrated POSIX launches. Windows and older execution hosts keep their existing launch paths; this changes no saved fields or client/server messages.
Linked Issue
Tracked in STA-11198.
Visual Proof
Pending real Electron and SSH QA with before/after screenshots. This PR is not ready to merge.
Testing
On macOS, eight named test files passed: 118 tests passed and one optional benchmark was skipped. The real-shell regression covers bash/zsh secret questions, bash Readline questions, zsh nested editors, zsh without line editing, and fish secret questions before and after the deadline, checking the user answer, readiness, and exactly-once execution. Existing local-provider, daemon-delivery, snapshot, and startup-timing contracts also passed. The pre-commit checks passed. Earlier Linux bash/read-e checks passed (4 tests); real Electron and SSH restoration QA remain pending; independent review completed one round and is blocked on the findings below. The full workflow skipped this PR while draft; it is now opened for review to run full CI, without a ready-to-merge claim.
The staging follow-up passed 96 tests across six named files, including a 200 KB command and cleanup after fallback and app quit. Existing replacement-shell compatibility tests have 4 failures, 10 passes, and 2 unavailable-binary skips; the failures are the shipping blocker below, not waived acceptance.
Review
Independent review is blocked. Executing restored commands inside prompt callbacks changes ordinary shell behavior: Bash and zsh without line editing can lose command status events, fish cannot suspend the command with Ctrl-Z, and zsh without line editing can use the wrapper’s options instead of the user’s. Short commands ending with a carriage return also need the existing normalization restored. These remain unresolved and prevent shipping. The tested execution alternatives have not yet preserved the full initialized-shell behavior.
The follow-up commit
0e4fd6730718corrects seven compile errors and releases the spawn environment after launch rather than retaining it through staging cleanup callbacks. Five named contract files passed: 85 tests passed and four skipped. On this head, CI static analysis and typecheck passed. The shell-contract job has the same four unresolved replacement-shell recovery failures described below (450 tests passed and 30 skipped); the rest of the workflow is still running. This follow-up does not resolve the execution findings or establish a QA pass.A bounded independent check also tested keeping typed startup-command delivery while allowing user input immediately. Waiting only after user activity still sends the command into an unanswered startup question before any input arrives. Waiting for a valid command-prompt marker can concatenate the startup command with unfinished user input. These results do not establish a preserving replacement for the current execution design.
Full review path: this changes startup ordering, process launch, and recovery behavior across local and SSH terminals. It is not eligible for the light path.
Agent skill upstream boundary
Notes
Shipping blocker: replacement-shell recovery decision pending. A startup file that replaces the integrated shell with an unintegrated shell, tmux, SSH, or a terminal relay discards its automatic continuation. Existing replacement-shell recovery tests represent behavior users already have; this implementation does not yet preserve that automatic recovery. Brennan must decide whether recovery should become deliberate in those cases or whether cooperating inner-shell integration must be implemented before shipping. A technical panel rejected process/argument/terminal-mode inference as a safe fallback. No loss of support is approved, and this PR must not land until that decision and its acceptance tests are settled.
The intended difference from the common pattern is execution after the user’s startup files, so restored commands use the user’s initialized configuration.
Checklist