Skip to content

One spawn-options step for both ways Orca opens a terminal, and one first-size rule - #30390

Merged
brennanb2025 merged 3 commits into
mainfrom
brennanb2025/conv-c5d-spawn-options
Oct 19, 2026
Merged

brennanb2025 merged 3 commits into
mainfrom
brennanb2025/conv-c5d-spawn-options

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​37 $\color{#cf222e}{\Huge{\mathbf{−}}}$​7 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​30
Prod 10 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​473 $\color{#cf222e}{\Huge{\mathbf{−}}}$​383 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​90

ELI5

Orca opens a terminal one of two ways: from the window, or from the host (the CLI, the phone, orchestration, agent launches, worktree startup). Each way had its own copy of the code that prepares the terminal's launch settings, and its own idea of how big the terminal starts. This PR gives both ways one copy and one size rule. Nothing changes for users today. It removes the place where the two copies drifted, and lets the callers that move to the host next keep their pane's real size.

What Changed

Problem. Every terminal launch builds the options handed to the process that starts the shell: which env keys to drop, OpenCode and Codex prep, workspace trust, shell overrides, the session id, the first size and so on. The window lane (src/main/ipc/pty/ipc/spawn-options.ts) and the host lane (src/main/ipc/pty/runtime/spawn-options.ts) each had about 30 of these steps, copied. The WSL Pi/OMP readiness wait and pane identity (#29884) show how launch preparation can drift between the two paths.

The first terminal size had the same problem:

  • window panes sent their fitted size;
  • host creates used the measured group size, else 120x40 (terminal-create.ts);
  • host splits hardcoded 120x40;
  • a host caller that had measured its own pane had no way to pass that size on.

That last gap is the "start size" difference that blocked moving window callers onto the host before (#27173): they'd start at 120x40 and then resize, wrapping their first output.

After this change.

  • One options step: src/main/ipc/pty/spawn-options-assembly.ts builds the env deletions and every provider field both lanes share. Each lane passes in the facts it resolves its own way and keeps its own reservation, hidden-delivery and execution steps. The host's Antigravity preparation stays between shared environment deletion and shared launch-option preparation. Host history settings are sampled before preparation; window history settings are sampled after OpenCode preparation. Both paths read trust settings before trust preparation, then shell and Windows settings afterwards.
  • Size restore: the provisional-size restore that both runners duplicated is one helper.
  • Hidden mark: TEMPORARY (the later commit/presentation step, C5e): retain the window's hidden re-mark after its begin step. Absent-owner cleanup can clear the preflight mark, so replacement startup must mark the session again before the provider starts. Hidden-delivery consolidation stays with C5e.
  • First size: src/main/ipc/pty/pty-first-size.ts holds one rule: the caller's measured pane, then the measured group the tab opens in, then a lone group, then 120x40. Host creates use it. TerminalCreateOptions gains an optional initialSize that callers pass from their own measured pane. No caller passes it yet; the next steps move window callers onto host creates and use it.

User-facing change: no change intended for valid existing callers. Each path preserves its own previous options, settings-read timing and hidden-delivery behavior while using the common assembler. Caller-specific facts remain distinct. Invalid internal size measurements fall through to the next valid candidate as described below. The exact-head CI workflow and real Electron comparison both passed.

Why

The common pattern is one launch assembler in the process that spawns, so a second launch path can't drift. The size policy also follows the common pattern of accepting caller measurements and choosing a fallback in the spawning process. Intended differences: Orca retains its measured target-group and lone-group fallbacks for callers without a pane measurement, and its existing 120x40 default. These preserve today's host launches; the detailed fallback and invalid-input rules are not claimed to match the common pattern exactly.

Considered and not done here:

  • Merging the pane reservation, worktree spawn gate and begin step: each lane's reservation join decides when those run, so they move together in the later begin/execute merge.
  • Merging the hidden-mark delivery: it moves with the commit/presentation merge.
  • Passing a visible start from host callers: it does nothing until the commit step decides who answers a visible terminal's startup queries, so it lands with that step.

Differences between the lanes, and what this PR does with each (inert = equal in every reachable case):

  • Shell override and shell args were gated on "no connection" in the host lane only. Inert: a connection id always names an SSH host, and both lanes refuse SSH before options run. Now one rule.
  • The Windows WSL distro and PowerShell implementation were set on every platform by the host lane, and only on Windows by the window lane. Inert: every reader checks for Windows. Now Windows-only in both; the host parity expectation drops the key for non-Windows cases.
  • The terminal handle env: the host stamps it earlier, the window here. Inert: the final env is identical. Left where each lane puts it.
  • Intended caller inputs: the new-session decision, pane identity and allowed launch intentions retain each caller's previous contract. The window forwards restore intentions; the host also accepts declared launch intentions when it has not adopted an existing pane. These are inputs to the common assembler, not a new shared policy.
  • Intended settings timing: host history is read before launch preparation; window history is read after OpenCode preparation. Both read trust settings before trust preparation, then shell arguments and Windows implementation settings afterwards. Preserving these phases keeps settings updates during an await working as before.
  • Intended preparation and API inputs: Antigravity account preparation remains at each path's original position with its original inputs and failure point. The window keeps its validated working directory and refreshed Agent Teams env deletions; the host keeps its agent-session claims, creation identity, cancellation signal and commit callback. These preserve existing caller contracts without adding them to the other path.
  • TEMPORARY lifecycle separation: reservation joins, the begin step and the worktree spawn gate remain per path until the later begin/execute consolidation (C6). Hidden marking and commit/presentation remain with C5e, including the restored window re-mark after absent-owner cleanup.
  • Size seeding (requestIsUnmeasured): TEMPORARY per-lane input. The window treats a hidden request as unmeasured; the host never does. The later commit/presentation and begin/execute steps (C5e/C6) decide who seeds size and remove this adapter difference.
  • Host group and lone-group fallbacks, and the 120x40 default, are intended retained behavior. Host splits stay at 120x40 because a new split pane has no measured size until the window lays it out.
  • Intended retained caller sizes: renderer automation dispatch stays at 120x40 and Codex detached restart at 80x24. These remain window requests; this PR changes neither caller.
  • Intended invalid-input normalization: the rule ignores a measured grid unless both dimensions are positive whole numbers, then falls through to the next candidate. Current renderer publishers already produce positive integer grids.

Testing

  • The parity suite runs every window and host producer case to the provider and must pass unchanged. One adjustment: host cases on macOS and Linux no longer expect the Windows-only WSL key.
  • The direct builder tests (Codex isolation, spawn intent, shell args, reattach size cache, managed accounts, OpenCode deletions, workspace trust, requested shell) still call each lane's builder.
  • The host size test gains one case: a caller's own measured pane size beats every group size.
  • The existing absent-owner regression now restores an initially hidden caller session whose saved owner uses the same session id; it checks the hidden mark inside replacement provider startup.
  • One settings regression replaces the settings object during the OpenCode capability await and checks late window history versus early host history.
  • Exact-head PR workflow passed for c7553d5b00cabc861c5a7cc80270eb51bf56d2fe, integrated with its recorded base. Actual CI logs confirm 3 absent-owner tests, 13 settings/trust tests, 60 launch-parity tests and 20 race-parity tests passed. Static analysis/typecheck, Linux and Windows packaging and selected Electron checks also passed. No tests ran locally; the configured local hook was unavailable, and CI provided the full static gate.
  • Real Electron QA passed on the M4Air for the current head; the comparison and screenshots are below.

QA

PASS on c7553d5b00cabc861c5a7cc80270eb51bf56d2fe. The lane owner completed real Electron QA on the M4Air using qa-gate ticket 351 and a Codex driver through the Chrome DevTools Protocol. Main 26554234df9 and this PR used the same build setup and test rig, with hidden test copies and isolated homes.

The comparison covered six flows: a new terminal from the window, a window split, a CLI-created terminal before and after showing it, a CLI split, a jcode agent launch, and a background CLI create while another tab held focus. First terminal sizes, hidden-at-start behavior, environment presence, pane and tab identities, tab placement, focus and sidebar status matched. Sizes were 79×52 for window and host terminals, 38×52 for the window split and 79×25 for the host split.

The only observed difference is the intended platform change already described above: macOS host launches omit the two Windows-only WSL and PowerShell fields. A possible missing-character artifact in one screenshot was checked with three repeat runs per side and complete terminal-buffer captures; neither side lost characters, and the original frame differed from its preceding frame only at the blinking cursor.

All six flows, main versus this PR:

All six terminal-launch flows, main versus PR

Background create repeated three times per side, immediately after return and one second later:

Background terminal creation repeat comparison

Notes

  • Windows, Linux, macOS: the only platform-gated change is the WSL/PowerShell fields, which only Windows reads.
  • SSH: unchanged (SSH spawns are refused before options in both lanes).
  • Mixed versions: initialSize is an internal create option; it isn't sent over the terminal.create wire in this PR.
  • No saved data changes. The settings accessor and sampled-history input are internal assembler parameters, with no wire changes.

Author: @BrennanKB5

…irst-size rule

The window lane (pty:spawn) and the host lane (runtime terminal create)
each built the provider's spawn options with their own copy of the same
steps. Both now call one shared assembly for the env deletions and the
shared provider fields; each lane keeps only what is its own (pane
reservation, Antigravity prep, agent-session claims). The duplicated
provisional-size restore moves to the same module, and the window lane
drops its second, identical hidden mark.

A terminal's first size now comes from one rule: the caller's measured
pane, then the measured group, then a lone group, then 120x40. Host
creates accept the caller's measured size for the callers that move
onto them next.
@brennanb2025
brennanb2025 marked this pull request as ready for review October 18, 2026 18:20
@coderabbitai

coderabbitai Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9c39b903-575c-4a86-8304-d73a46b81d09
📥 Commits

Reviewing files that changed from the base of the PR and between d90c2d1 and c7553d5.

📒 Files selected for processing: 5
  • src/main/ipc/pty-dead-owner-respawn.test.ts
  • src/main/ipc/pty/ipc/spawn-options.ts
  • src/main/ipc/pty/runtime/spawn-options.ts
  • src/main/ipc/pty/spawn-options-agent-workspace-trust.test.ts
  • src/main/ipc/pty/spawn-options-assembly.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

📝 Walkthrough

Terminal creation now accepts a caller-provided initial size and selects the first valid size from pane, group, and single-group measurements, with a 120×40 fallback. IPC and runtime PTY spawn paths delegate environment-deletion assembly, launch-option construction, and provisional-size seeding and restoration to shared helpers. The host-provider test expectation includes the WSL distro field only for Windows. Tests also cover hidden delivery during respawn and history-isolation settings sampled by the renderer and runtime spawn paths.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to c7553

This refactor consolidates terminal spawn-option assembly and first-size selection with no intended user-facing change. No concrete merge-blocking risk was identified in the supplied context. The author notes that corrected-head test execution and real-Electron QA are still pending.

Pre-merge checks: Passed 3 · Failed 2

Failed Failed checks: 2 warnings

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 13 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check Warning The description gives detailed context, implementation scope, testing results, and QA evidence. However, the required Linked Issue section is empty (Fixes #), and several template sections are missi… Add an actual linked issue to the Fixes # field. Complete the required template sections, including Review, Agent skill upstream boundary, and the Checklist. State whether AI Disclosure is applicable if the author is not an internal contr…
Passed Passed checks: 3 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 two main changes: shared spawn-option assembly and a unified initial PTY-size rule.
Full details: Description check

Explanation

The description gives detailed context, implementation scope, testing results, and QA evidence. However, the required Linked Issue section is empty (Fixes #), and several template sections are missing or incomplete, including Review, Agent skill upstream boundary, and the Checklist.

Resolution

Add an actual linked issue to the Fixes # field. Complete the required template sections, including Review, Agent skill upstream boundary, and the Checklist. State whether AI Disclosure is applicable if the author is not an internal contributor.

  • 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.

❤️ Share

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

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Orca prepared terminal launches in two separate places, allowing launch options and starting sizes to drift. This change gives window and host launches one common options assembler and one first-size rule while preserving the behavior of existing callers.

The full review loop finished in two rounds on c7553d5b00cabc861c5a7cc80270eb51bf56d2fe, including the architecture challenge and final-diff readiness review. Both confirmed findings were fixed: replacement launches restore the hidden mark before the process starts, and settings are read at the same preparation stages as before. No confirmed finding remains open. Two minor concerns remain recorded as deferred: historical rationale wording, now qualified in the description, and stricter rejection of malformed size measurements, now labelled as intended. No reachable user regression was found for that size rule.

Verified: the full PR workflow passed for this exact head integrated with its recorded base, including the PR checks passed aggregate, static analysis, typecheck, Linux and Windows packaging, and selected Electron checks. The CI logs confirm 3 absent-owner, 13 settings/trust, 60 launch-parity and 20 race-parity tests passed. The PR is out of draft, merge status is CLEAN, and there are zero unresolved review threads. No local tests, builds or apps were run in this final session.

The lane owner's real Electron QA on the M4Air passed for this head against main 26554234df9. All six flows matched for first size, hidden-at-start state, environment and identity, placement, focus and sidebar status. The sole difference was the documented omission of Windows-only fields on macOS. A possible screenshot text artifact was checked with three repeats per side and complete terminal buffers; no glyph loss occurred. This QA establishes the macOS comparison; it does not claim a real Windows, Linux or SSH interaction run.

For Brennan's review

The first image compares main and this PR for window create, window split, CLI create, CLI split, jcode launch and background CLI create. The second shows the repeated background-create check.

Six launch flows, main versus PR

Background-create repeat, main versus PR

Three deliberate boundaries to inspect:

  • Retained hidden re-mark: temporary duplication until the later presentation consolidation; required today after absent-owner cleanup.
  • Windows-only fields: the intended non-Windows payload difference observed in QA.
  • First-size policy: positive whole measurements, retained group fallbacks and 120×40 default. These retained choices are labelled differences from the common pattern; this PR does not consolidate the entire launch lifecycle.

@brennanb2025
brennanb2025 merged commit e20b9d2 into main Oct 19, 2026
38 checks passed
@brennanb2025
brennanb2025 deleted the brennanb2025/conv-c5d-spawn-options branch October 19, 2026 04:40
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