Repository navigation
refactor(pty): one spawn preflight step for both launch lanes - #29101
Conversation
Row 3 (one spawn preflight for both launch lanes) first pins main's behaviour: - Row 8, agents started with their workspace (from the earlier row-8 work): a work item's "start agent" and the folder-workspace composer through the real producer and pane and pty:spawn: draft and note on argv, a long multi-line note, PowerShell quoting under cmd.exe, project WSL, terminal shell WSL, and a prompt-less Codex start with no shell wait. - A deleted folder: the window starts in the workspace root and says so; the host passes the folder on and the provider refuses it. - A Windows plain terminal in a folder containing a WSL-project repo, and in a \\wsl$ repo with no saved runtime: the window uses the runtime the renderer sends; the host reads only the repo's project. - A managed SSH Windows host: only the host lane opens the OpenSSH login shell. - A pane's own agent launch drops the root fallback (TEMPORARY parity).
The window's pty:spawn and the host's runtime controller each kept their own copy of the first spawn step: resolve the folder a new terminal starts in and the shell/runtime its PTY uses. resolvePtySpawnStartupTarget now does both for both lanes. Each lane passes the two inputs it still differs on, so nothing changes for users: - missingCwdFallback (TEMPORARY, row 13a): only window starts fall back to the workspace root, with a notice, when the requested folder is gone (including the WSL cwd probe). - projectRuntimeSource (TEMPORARY, row 13b): the window uses the runtime its renderer sent; the host reads the repo's project from the store. The OpenSSH login shell (#9327) now resolves for both lanes; only an SSH-launched orcad turns it on, and that process has no window, so the window lane never meets it. Both lanes resolve after fresh-spawn recovery and session-id mint (the WSL distro reads the session id). Admission, adoption, hidden mark, recovery and mint stay where they were.
Review loop status (head cbe112e)The problem this is part of. Orca starts a terminal two ways. "+" tabs, split panes and agent launches from the window go through one path. User-facing change: none intended, none found. Every entry point gives the same result as on main for the same input:
The two places where the paths still differ (the missing-folder fallback exists only on the window path, and the window sends its shell/runtime while the host reads it from saved state) are kept on purpose as two named inputs. Each one is listed in the body with the later step that removes it. The one deliberate change is that the window path can now choose the OpenSSH login shell on a managed Windows SSH host. A real app never reaches that case: the managed SSH server runs under Node and never installs the window's terminal-start handler. Review. Round 1: three adversarial reviewers plus an architecture challenger. Then a readiness pass on the final diff. No P0, P1 or P2 issues.
Verified.
QA: main (
Split into a deleted folder (terminal text blurred, it shows local paths), main then this PR:
Not verified.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
The description covers the change, rationale, testing, platform considerations, visual proof, and checklist. However, the required Linked Issue section states that no issue exists, which does not meet… | Add a valid linked issue under the Linked Issue section. If the author is a maintainer and the repository permits the documented exception, state that exception clearly instead of writing that no issue exists. |
| Docstring Coverage | |
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 10 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: consolidating PTY spawn preflight into one shared step for both launch lanes. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Description check
-
Explanation
The description covers the change, rationale, testing, platform considerations, visual proof, and checklist. However, the required Linked Issue section states that no issue exists, which does not meet the repository template requirement for an actual issue link.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
ELI5
Orca can start a terminal two ways: the window asks for it directly, or Orca's host starts it (for the CLI, agent launches and paired devices). Each way kept its own copy of the very first step: "which folder does this terminal start in, and which shell runs it?" Two copies drift apart, and that drift is why an earlier attempt to move the "+" button onto the host's path had to be reverted (#27173). This PR makes that first step one shared function. Nothing changes for users yet: the two places where the copies still disagree are passed in as explicit, temporary inputs, each with the follow-up that removes it.
What Changed
Before (on main):
pty:spawn,ipc/spawn-preflight.ts) resolved the start folder with a "fall back to the workspace root, and tell the user" rule when the folder is gone (with a WSL folder probe), and picked the shell from the project runtime the renderer sent.runtime/spawn-preflight.ts) resolved the start folder plainly (a deleted folder reaches the provider, which refuses it with "Working directory … does not exist") and picked the shell from the store (repo → project only).After:
resolvePtySpawnStartupTarget(src/main/ipc/pty/spawn-startup-target.ts) returns the start folder (plus the fallback notice and the WSL-probed folder when they apply), the shell/runtime options, the expected WSL distro and the Codex account target. Both lanes call it.missingCwdFallbackcwdFallbackstrip for pane agent launches inipc/spawn-agent-launch.ts)projectRuntimeSourcesent: the runtime the renderer sends (reads a\\wsl$path as WSL; a folder borrows its repo's)store: repo → project onlyorcadsets the env that turns it on, andorcadinstalls no IPC surface (setPtyHostBindingsruns only in the Electron desktop preflight), sopty:spawnnever runs there. On the desktop window lane it is a no-op.Users see: no change.
Why
This is row 3 of the launch-convergence plan: every later step that moves a producer onto one spawn path (rows 14–19) needs the first spawn step to be one function, and rows 13a/13b need one place to delete their temporary input from. Passing the differences as named inputs keeps this PR a pure merge of duplicated code; fixing either difference here would change what users see and belongs in its own PR.
Alternatives considered:
Linked Issue
Part of the launch-convergence plan (row 3); no issue.
Visual Proof
N/A: no user-visible change. The pins below show both lanes resolve exactly as on main.
Testing
Commit 1 pins main's behaviour before the refactor (passes on main):
pty-launch-parity.test.ts: a deleted folder (window falls back and says so; host passes the folder on); a Windows plain terminal in a folder containing a WSL-project repo and in a\\wsl$repo with no saved runtime (window uses the sent runtime; host uses the settings shell); a managed SSH Windows host (login shell).pty-agent-pane-launch.test.ts: a pane's own agent launch drops the root fallback (TEMPORARY parity inspawn-agent-launch.ts).Commit 2 keeps every existing suite unchanged and green:
pty-launch-parity(rows 1–8, 10),pty-launch-race-parity,pty-spawn-cwd-fallback,pty-wsl-cwd-validation,pty-daemon-spawn-wsl-runtime,runtime/spawn-preflight-requested-shell,pty-controller-spawn-admission, both hidden-at-spawn-mark suites,pty-pane-materialization-race, plus 19 neighbouring spawn/env suites. One pin flips on purpose: the managed-SSH case now gives the window lane the login shell too (unreachable in production, see above). Each TEMPORARY input has a pin that fails when the input is ignored (checked by forcing each input off).Platforms: logic covered on macOS, Linux and Windows paths through the platform-switching parity suites; SSH: the window lane never runs on a managed SSH host, and the host lane's login-shell behaviour is pinned.
Size
Production net +54 (+208 / −154), over the plan's +30 estimate; accepted by the lane owner with this breakdown. The generated table at the top counts the four
*.test-fixture.tsfiles (+316) as production; they are test code.spawn-startup-target.tsmissingCwdFallbackinput (the fallback step stays, on for both lanes); row 13b removesprojectRuntimeSource(thesentbranch)ipc/spawn-preflight.tsruntime/spawn-preflight.tsipc/spawn-env-codex.tsTests: row-8 characterization fixtures +329 (carried over, plan-mandated); new pins ≈+140.
Review
Agent skill upstream boundary
Notes
Collisions: #28136 (parked) rebases after this and is the natural partner of row 13b; #28180 and #23228 touch the same files textually.
Checklist
N/Awith reason for visual proofpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (CI will cover)