Repository navigation
Keep Orca CLI first after shell startup - #25130
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe changes set Priority: ⬆️ High Merge Risk: 🔵 Low · up to In this uncommon Zsh configuration, the ambient CLI may remain ahead of Orca and startup readiness may not be installed. The risk is bounded, but merits owner awareness. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change keeps command lookup tied to the selected local or remote installation. No new privilege grant or cross-host bypass was established, but uncommon startup and recovery cases remain uncertain. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 23 files. (3 skipped: 3 unsupported.)
✨ 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: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
1a727570-2c60-4882-b07c-4eab0d8ffb34
📒 Files selected for processing (18)
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-bash-rcfile.txtsrc/main/__fixtures__/shell-wrapper-snapshots/local-zsh-zshenv.txtsrc/main/__fixtures__/shell-wrapper-snapshots/relay-bash-rcfile.txtsrc/main/__fixtures__/shell-wrapper-snapshots/relay-zsh-zshenv.txtsrc/main/cli/orca-cli-child-path.test.tssrc/main/cli/orca-cli-child-path.tssrc/main/daemon/daemon-bash-shell-ready-rcfile.tssrc/main/daemon/shell-ready-bash-wrapper.test.tssrc/main/providers/local-pty-shell-ready-bash-rcfile.tssrc/main/providers/ssh-pty-spawn-env.test.tssrc/main/providers/ssh-pty-spawn-env.tssrc/main/shell-startup-feature-channel.test.tssrc/main/shell-startup-features.tssrc/main/zsh-startup-wrapper-builder.tssrc/relay/pty-shell-overlay-wrappers.tssrc/shared/orca-cli-shell-path.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Two existing SSH provider tests fail once buildSshPtySpawnEnv starts injecting ORCA_CLI_BIN_DIR — their expected env objects were not updated, so this reds CI. One design question on the overlay-key broadening is noted inline.
Reviewed changes
prependOrcaCliDirToChildPathnow records the CLI directory it selects (dev launcher dir, packaged Linux shim dir, packaged macOSResources/bin) in a newORCA_CLI_BIN_DIRenv key on non-Windows platforms.- A shared POSIX snippet re-prepends that directory in every local/daemon/relay bash rcfile and zsh
.zshenv, so a profile that reset or reorderedPATHcannot hide this app's CLI. SSH replaces the client value with the remote bridge dir; Windows clears it. ORCA_CLI_BIN_DIRis added toOVERLAY_ENV_KEYS; six wrapper snapshots are regenerated and Bash/zsh/SSH tests were added.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No critical issues — one minor maintainability suggestion.
Reviewed changes
Reviewed the two commits added since the prior pullfrog review (5a2479b7b6, c6bc30f831).
- Added a one-shot zsh
zle-line-initfallback that runs Orca's deferred init even when a user config replacesprecmd_functions, saving the prior widget in__orca_saved_line_initand restoring it before the readiness hook captures it. - Moved
ORCA_CLI_POSIX_PATH_RESTOREout of the zshoverlayguard so any wrapped pane re-prepends the app CLI dir, and removedORCA_CLI_BIN_DIRfromOVERLAY_ENV_KEYSso a bare pane still selects no features. - Updated the SSH spawn-env expectations and added SSH/WSL/plain-pane controls, strengthened the scoped-histfile live test, added a chained-prompt live test, and regenerated the six wrapper snapshots.
ℹ️ Nitpicks
src/main/zsh-deferred-startup-line-init.ts:11—local __orca_prev_line_init_fn=""is a load-bearing recursion guard (it keeps__orca_prompt_markfrom re-entering the add-zle-hook-widget dispatcher that is already on the stack). It is the only statement in this file without aWhy:comment; a one-line note would keep a future reader from removing it as redundant.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
|
The original Linux shell failure is now reproduced on Ubuntu 24.04 ARM with stock zsh 5.9-6ubuntu2 in the isolated diagnostic run. The exact published c6 shell code initializes correctly in three controls. When the stock global zshrc replaces the keyboard initialization widget and the user clears the prompt-hook array, both initialization paths are lost: CLI lookup selects the controlled ambient executable and deferred initialization never runs. This is cause evidence for the current failure, not a successful repair. The diagnostic captures real Linux shell initialization and CLI lookup only; it does not launch OpenCode or a model task on Linux. The production successor is still private while we verify that it preserves user prompt hooks, their previous-command status and zsh options. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
e924e6d4-c161-4bed-b886-e1f33cff0eda
📒 Files selected for processing (11)
.github/workflows/pr.ymlconfig/scripts/ci-unit-files.mjsconfig/scripts/pr-workflow-parallelism.test.mjssrc/main/__fixtures__/shell-wrapper-snapshots/daemon-zsh-zshenv.txtsrc/main/__fixtures__/shell-wrapper-snapshots/local-zsh-zshenv.txtsrc/main/__fixtures__/shell-wrapper-snapshots/relay-zsh-zshenv.txtsrc/main/codex/codex-structured-child-environment.test.tssrc/main/codex/codex-structured-session-adapter.test.tssrc/main/runtime/structured-session-child-identity-env.test.tssrc/main/zsh-deferred-startup-line-init.live-shell.test.tssrc/main/zsh-deferred-startup-line-init.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the four commits added since the prior pullfrog review (f4bd404827, db9634c0c3, 5098be93ec, 0716bd6fc2), which replace the precmd-override fallback with a one-shot sched callback and expand the live-shell controls.
- Replaced the
__orca_deferred_precmd/__orca_saved_precmdmonkey-patch offunctions[precmd]with__orca_deferred_sched_init, a one-shotsched +0callback that runs Orca's deferred init after the user's own prompt hooks and preserves the incoming$?so%?in the prompt is not clobbered. __orca_arm_deferred_line_initnow also arms that callback whenzsh/schedloads, while keeping thezle-line-initbinding for shells where the module is unavailable or the widget is chained throughadd-zle-hook-widget.- Cleanup unfunctions the callback immediately when
schedis unavailable, otherwise lets the callback retire itself; the new append guard keeps__orca_osc133_precmdinprecmd_functionswhen a config replaced the array. - Expanded the live-shell test matrix to cover status/option preservation, replaced and reordered
precmdhooks, scheduled-event cleanup and the unavailable-module path; addedORCA_CLI_BIN_DIRexpectations to the Codex/structured-session tests and registered the new live-shell file in the shell-contract CI lists.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No critical issues — one documentation suggestion.
Reviewed changes
Reviewed the single commit added since the prior pullfrog review (811a3593d3), which keeps completed zsh startup hooks retired when the wrapper .zshenv is sourced again.
- Guarded the re-registration of
__orca_deferred_initand the__orca_arm_deferred_line_initcall on_orca_deferred_init_donebeing unset, so a repeat source of the wrapper cannot re-bindzle-line-initto__orca_deferred_line_init(which recursed throughzle zle-line-init) or leave a dead hook inprecmd_functions. - Regenerated the daemon/local/relay zsh wrapper fixtures to match.
- Added
repeat-history/repeat-startuplive-shell controls that source the wrapper twice and assert nojob table full or recursion limit exceededand no__orca_deferred_initleft inprecmd_functions; narrowed the unavailable-module output assertion to the specificzsh/sched/ arm-function diagnostics.
ℹ️ Nitpicks
src/main/shell-templates.ts:105— the new_orca_deferred_init_doneguard has no rationale, while the block's JSDoc "Why guarded on absence" explains only the innerprecmd_functions[(Ie)]check. A one-lineWhyon the outer guard (keep completed startup hooks retired on re-source, so re-arming cannot recurse) would match the file's convention.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
* fix(daemon): bump the terminal daemon protocol to v40 Daemon-side changes landed under v39 after v1.4.220 shipped v39: the #25130 and #24636 shell-wrapper changes and a wider agent list (jcode, qoder-cn, dsb; resume claims for qoder-cn, qwen-code, cursor and jcode). Without a bump, an update keeps new tabs on the running v39 daemon, which misses the wrapper fixes and rejects those resume claims with agent_session_identity_required. * fix(local-builds): record daemon protocol v40 in the local build compatibility contract
Daemon-side changes landed under v39 after v1.4.220 shipped v39: the stablyai#25130 and stablyai#24636 shell-wrapper changes and a wider agent list (jcode, qoder-cn, dsb; resume claims for qoder-cn, qwen-code, cursor and jcode). Without a bump, an update keeps new tabs on the running v39 daemon, which misses the wrapper fixes and rejects those resume claims with agent_session_identity_required. (cherry picked from commit a9d4587)
* fix(daemon): bump the terminal daemon protocol to v40 Daemon-side changes landed under v39 after v1.4.220 shipped v39: the stablyai#25130 and stablyai#24636 shell-wrapper changes and a wider agent list (jcode, qoder-cn, dsb; resume claims for qoder-cn, qwen-code, cursor and jcode). Without a bump, an update keeps new tabs on the running v39 daemon, which misses the wrapper fixes and rejects those resume claims with agent_session_identity_required. * fix(local-builds): record daemon protocol v40 in the local build compatibility contract (cherry picked from commit 1787585)
ELI5
Agents launched by Orca could select an older globally installed CLI after shell startup reordered PATH. Re-sourcing the zsh wrapper could also replace the user's restored line editor with a recursive Orca callback. Orca now restores its selected CLI and leaves completed startup initialization retired when the wrapper is sourced again.
What Changed
The execution host publishes its selected CLI directory. Local, daemon and relay Bash wrappers restore it after profiles; the shared zsh wrapper restores it after user startup. A one-shot scheduler invokes the existing deferred initializer after user prompt hooks, preserving the user's hooks, exit status, options, history and scheduled jobs. Registration and arming now share the existing completed-initialization guard, preventing re-sourcing from installing that callback again.
SSH replaces or clears the client's directory with the execution host's directory. The optional marker retains its feature-selection contract, and Windows retains WSL handling. Generated fixtures and shell-contract selection cover the same behavior.
Why
Profiles can undo the initial PATH prepend. Reusing host path selection and deferred initialization restores the app runtime after stock zsh startup, even when it clears the prompt-hook array. Reusing the initialization guard also preserves the user's line editor after startup without adding another state mechanism.
Linked Issue
Found during the OpenCode worker-dispatch reproduction for #19980. This startup repair does not close that broader issue on its own.
Visual Proof
N/A — shell startup and command lookup have no rendered UI changes. Raw private PTY before/after transcripts are retained.
Testing
811a3593passes 426 tests across 25 suites, with four existing platform skips; all three desktop typechecks, full owned formatting, changed-code checks and the configured repository scan pass. Full owned lint retains one unchanged directive warning.0716bd6f; each has zero after this correction, with the user's widget retained, no saved widget or dead prompt hook, and startup occurring once. The same final regression tests fail on the parent and pass after the fix.42, sticky options, user hook order and future scheduled jobs while executing the installed OpenCode version command exactly once. Scheduler unavailability now checks its own diagnostic precisely and retains the complete raw output.The new repeat-source runtime controls run on macOS, not Linux. Mac controls execute OpenCode's version command; the prior Linux controls establish lookup and startup behavior without running OpenCode. Model/task completion and fresh/nested startup remain separately demonstrated on their frozen combined source.
Review
Independent reviews verify all 30 owned files, all 32,337 other source files against parent
0716bd6f, the complete physical source and raw checks. Only the shell template, its live-shell test and three generated fixtures change in this correction. All 329 recorded correction process observations are freshly absent; the parent qualification retains its separate cleanup bounds. The worker preamble and wire opcodes remain unchanged. Fresh public CI must pass before merge.Agent skill upstream boundary
Notes
Executed platforms are macOS and the source-pinned parent on native Ubuntu ARM. SSH/Windows/WSL ownership cases are automated; no live SSH or Windows shell run is claimed. Older wrappers ignore the optional environment field. Temporary Linux diagnostic branches are evidence carriers and will not be merged.
Checklist