Skip to content

fix(wsl): hold a passive WSL restore before the cwd probe, whichever provider spawns - #35895

Merged
nwparker merged 1 commit into
mainfrom
nwparker/wsl-hold-any-provider
Oct 29, 2026
Merged

nwparker merged 1 commit into
mainfrom
nwparker/wsl-hold-any-provider

Conversation

@nwparker

@nwparker nwparker commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​132 0 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​132
Prod 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​26 $\color{#cf222e}{\Huge{\mathbf{−}}}$​2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​24

Flag-off: applies with the flag on or off, to WSL bridge panes restored without a user act (#35813's wslPassiveRestore).
User-risk: Low. A restored WSL pane on a stopped distro now waits with Start in every daemon state: healthy, degraded or off. Before, a degraded or off daemon, or the startup cwd probe, could start the distro. A pane the user opens is not held.

ELI5

When Orca restores a terminal you didn't ask for (launch restore, a reload, a remount), it must not start a stopped WSL distro. #35813 added that check, but in the wrong layer. If the terminal daemon was off or unhealthy, Orca spawned the shell itself and skipped the check. Even with the daemon healthy, an earlier step that checks the terminal's folder ran a command inside the distro first. The check now runs before both, so it holds in every case.

What Changed

  • Before: holdPassiveRestoreOfStoppedWslDistro ran only inside the daemon adapter's spawn (daemon-pty-session-spawn.ts).
    • With the daemon off or degraded, LocalPtyProvider and the fresh-spawn fallback never read wslPassiveRestore, so a restore booted the distro.
    • With the daemon healthy, resolvePtySpawnStartupTarget had already run the startup cwd probe (wslUncDirectoryExistsAsync, a wsl.exe -d <distro> --exec test) for a POSIX restore cwd. That too could boot it.
  • After: the hold runs in the shared resolvePtySpawnStartupTarget (spawn-startup-target.ts), after the runtime options are known and before the cwd probe and any provider spawn.
    • The IPC preflight passes passiveWslRestore: { sessionId, isNewSession, attachOnly } only when the renderer marked the request wslPassiveRestore.
    • The distro comes from the same resolveWslSessionContext the adapter uses.
    • The adapter's own hold stays as a second check.
    • It reuses holdPassiveRestoreOfStoppedWslDistro unchanged: a settled running read, where a stopped or unanswered distro holds with the existing "WSL distro … is stopped" message.

Why

  • Hold at the shared step every provider passes through, rather than in each provider. A new or fallback provider can't miss it, and it also covers the cwd probe, which runs before any provider is chosen.
  • Keep the adapter's check. It's cheap and still guards any direct adapter caller.

Linked Issue

Internal WSL tracker, E9. e9-reviewer found this gap on #35865; it already existed for #35813's wslPassiveRestore. wslBackgroundStart from #35865 will get the same hold once #35865 merges.

Visual Proof

N/A: no new UI. A held pane shows #35813's existing Start card.

Testing

New pty-spawn-wsl-passive-restore-hold.test.ts, through the real pty:spawn IPC with a mocked running read. The first two cases fail without the change.

  • With the default local provider (daemon off) and a stopped distro: the restore is refused, the cwd probe never runs, and node-pty never spawns.
  • With the daemon provider: the restore is refused before its cwd probe and before the provider spawns.
  • Once the distro runs, the restore spawns. A request the user made (no wslPassiveRestore) is never held.

Also run:

  • the pty and daemon suites: 4,607 pass;

  • the load rule: pnpm tc, typecheck:ci, check:code-quality:changed, the anti-slop lint and the ast-grep gates.

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

Review

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

  • Platforms: Windows only in effect. Off Windows, no session resolves a WSL distro.
  • Wire: no wire change. wslPassiveRestore is an existing IPC field.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • 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, pnpm test, and pnpm build pass (or CI will cover; local preferred)

…provider spawns

The passive-restore hold lived in the daemon adapter, so a degraded or
off daemon (LocalPtyProvider) never applied it, and even with the daemon
the startup cwd probe (wsl.exe --exec test -d) ran first and could boot
the distro. The hold now runs in the shared startup-target resolution,
before that probe and before any provider spawns (E9).
@greptile-apps

greptile-apps Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 4/5

[Medium impact] The change reaches passive WSL restores on Windows across both provider routes.

Fix the attach-only bypass before merging so a passive restore cannot start a stopped WSL distro through the cwd probe.

Findings

  1. P1 Passive attach starts WSL ▶

Summary

Moves the passive WSL restore hold into shared startup preparation, before the cwd probe and provider dispatch.

  • Passive WSL restores check the distro before startup continues.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Passive WSL restore] --> B[Shared startup preparation]
  B --> C{Attach-only request?}
  C -->|No| D[Read distro running state]
  D -->|Stopped or unanswered| E[Hold pane for Start]
  D -->|Running| F[Check startup cwd]
  C -->|Yes: hold skipped| F
  F --> G[Provider spawn or attach]
Loading

Reviews (1) · Last reviewed commit: "fix(wsl): hold a passive WSL restore bef..." · Reviewed by Greptile

passiveWslRestore: {
sessionId: ctx.effectiveSessionId,
isNewSession: ctx.ownsNewIncarnation,
attachOnly: args.attachOnlyView === true

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Passive attach starts WSL

Passing attachOnly: true makes holdPassiveRestoreOfStoppedWslDistro return without checking the distro. But this preflight still checks the cwd before attempting the attach. When no pane owner is recorded and a passive restore has a POSIX cwd in WSL, that check runs wsl.exe -d <distro> --exec and starts the stopped distro without the user pressing Start.

Either skip the cwd probe for attach-only requests or keep the shared hold active before it. The adapter's attach-only exemption is safe only after these earlier checks.

@coderabbitai

coderabbitai Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Walkthrough

When wslPassiveRestore is explicitly enabled, preflight passes passive-restore session options to startup-target resolution. For a non-pre-adopted PTY, the resolver awaits the stopped-distro hold before startup-cwd resolution. Tests cover stopped distros with local and daemon providers, a running distro, and a restore without the passive-restore flag.

Priority: Normal

Merge Risk: Moderate · up to 5c041

The change adds a hold so that restoring a session does not boot a stopped WSL distro, but several reachable paths still bypass it or start the distro before it runs. The effect is limited to Windows users with WSL and causes unwanted distro startups rather than data loss. Address these gaps, or explicitly accept them, before merging.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title names the main change: a passive WSL restore hold now runs before the cwd probe, regardless of which provider spawns the PTY. It matches the raw summary and the objectives, which both descri…
Description check Passed The description follows the template's main sections: ELI5, What Changed, Why, Linked Issue, Visual Proof, Testing, AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist. It expla…
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.
Full details: Title check

Explanation

The title names the main change: a passive WSL restore hold now runs before the cwd probe, regardless of which provider spawns the PTY. It matches the raw summary and the objectives, which both describe moving the hold into shared startup-target resolution. The title is long (84 characters), but it stays specific and on topic.

Full details: Description check

Explanation

The description follows the template's main sections: ELI5, What Changed, Why, Linked Issue, Visual Proof, Testing, AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist. It explains the before and after behavior, the mechanism, the reason for the change, and the test coverage. The Linked Issue section is weak. It cites an internal tracker ('Internal WSL tracker, E9') and has no 'Fixes #' issue link, which the template says outside contributors must include. The Visual Proof section is N/A with a reason. The Testing section lists automated tests but leaves the manual-test checkbox unchecked. The pnpm checklist item is also unchecked.

Resolution

Consider these edits: replace the internal tracker reference in Linked Issue with a real GitHub issue number (Fixes #...) or confirm that the maintainer exception applies. Fill in the AI Disclosure section if an AI model helped write the change. Clarify whether pnpm lint, typecheck, test, and build passed, since only a subset of the gates is listed.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 3 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

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.

@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: 3

Caution

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

⚠️ Outside diff range comments (1)

Major · Run the hold before the folder workspace path check. · spawn-preflight.ts:34-38

src/main/ipc/pty/ipc/spawn-preflight.ts:34-38
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Run the hold before the folder workspace path check.

For a folder workspace on \\wsl.localhost, this admission check calls stat on the workspace path before the new hold runs. Accessing that path can start the stopped distro, so the later hold can observe it as running. Check the distro before this filesystem access. (raw.githubusercontent.com)


Review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 2d59ba25-c672-4ad0-aab3-1987e5192a2a
Commits

Reviewing files that changed from the base of the PR and between 84a4995 and 5c0410b.

Files selected for processing: 3
  • src/main/ipc/pty/ipc/spawn-preflight.ts
  • src/main/ipc/pty/pty-spawn-wsl-passive-restore-hold.test.ts
  • src/main/ipc/pty/spawn-startup-target.ts

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

passiveWslRestore: {
sessionId: ctx.effectiveSessionId,
isNewSession: ctx.ownsNewIncarnation,
attachOnly: args.attachOnlyView === true

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.

Major · Functional Correctness · Heavy lift

Do not exempt attach-only requests from the cwd probe hold.

The helper skips the hold when this flag is true, but preflight still runs the cwd probe before attach-only dispatch. That probe can start a stopped distro. Also, attachOnlyViewOf ignores the flag when main-owned layout is disabled, so the request can proceed to a normal spawn. Skip cwd probing for an effective attach-only request and retain the hold otherwise. (raw.githubusercontent.com)

Comment on lines +78 to +79
await holdPassiveRestoreOfStoppedWslDistro(
{ ...args.passiveWslRestore, wslPassiveRestore: true },

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.

Minor · Functional Correctness · Quick win

Supported by static analysis

Script executed:

rg -n 'holdPassiveRestoreOfStoppedWslDistro|wslPassiveRestore' src --glob '!*.test.ts'

Repository: stablyai/orca

Length of output: 2143


Script executed:

set -eu
printf '%s\n' '--- helper ---'
nl -ba src/main/daemon/daemon-wsl-passive-restore-hold.ts
printf '%s\n' '--- startup target ---'
nl -ba src/main/ipc/pty/spawn-startup-target.ts | sed -n '1,180p'
printf '%s\n' '--- spawn types ---'
nl -ba src/main/ipc/pty/ipc/spawn-types.ts | sed -n '1,150p'
printf '%s\n' '--- spawn preflight ---'
nl -ba src/main/ipc/pty/ipc/spawn-preflight.ts | sed -n '1,150p'
printf '%s\n' '--- daemon caller ---'
nl -ba src/main/daemon/daemon-pty-session-spawn.ts | sed -n '150,220p'
printf '%s\n' '--- renderer producer blocks ---'
nl -ba src/renderer/src/components/terminal-pane/ipc-pty-spawn-request.ts | sed -n '90,165p'
nl -ba src/renderer/src/components/terminal-pane/pty-connection/wsl-distro-stopped-hold-pane.ts | sed -n '1,80p'
printf '%s\n' '--- relevant references ---'
rg -n -C 5 -F -- 'wslPassiveRestore' src/renderer src/main/ipc/pty src/main/daemon --glob '*.ts' --glob '!*.test.ts'

Repository: stablyai/orca

Length of output: 42185


Script executed:

set -eu
printf '%s\n' '--- complete preflight relevant references ---'
rg -n -C 12 -F -- 'resolvePtySpawnStartupTarget' src/main/ipc/pty/ipc src/main/ipc/pty --glob '*.ts' || test "$?" -eq 1
printf '%s\n' '--- preflight continuation ---'
nl -ba src/main/ipc/pty/ipc/spawn-preflight.ts | sed -n '50,190p'
printf '%s\n' '--- spawn startup args construction ---'
rg -n -C 10 -e 'passiveWslRestore|isNewSession|attachOnly' src/main/ipc/pty/ipc src/main/ipc/pty --glob '*.ts' || test "$?" -eq 1
printf '%s\n' '--- renderer connect/request declarations ---'
nl -ba src/renderer/src/components/terminal-pane/pty-connection/wsl-distro-stopped-hold-pane.ts | sed -n '1,55p'
rg -n -C 12 -e 'wslRestoreStartArgs|ipcPtySpawn|connectOptions|sessionId' src/renderer/src/components/terminal-pane/pty-connection src/renderer/src/components/terminal-pane/ipc-pty-spawn-request.ts --glob '*.ts' --glob '!*.test.ts' || test "$?" -eq 1
printf '%s\n' '--- main spawn options and provider contract ---'
nl -ba src/main/ipc/pty/ipc/spawn-options.ts | sed -n '1,100p'
nl -ba src/main/providers/pty-provider-contract.ts | sed -n '70,115p'

Repository: stablyai/orca

Length of output: 15690


Script executed:

set -eu
printf '%s\n' '--- IPC preflight exact forwarding ---'
nl -ba src/main/ipc/pty/ipc/spawn-preflight.ts | sed -n '80,115p'
printf '%s\n' '--- IPC preflight session initialization ---'
nl -ba src/main/ipc/pty/ipc/spawn-preflight.ts | sed -n '15,30p'
printf '%s\n' '--- renderer passive producer ---'
nl -ba src/renderer/src/components/terminal-pane/pty-connection/wsl-distro-stopped-hold-pane.ts | sed -n '1,75p'
printf '%s\n' '--- renderer spawn request ---'
nl -ba src/renderer/src/components/terminal-pane/ipc-pty-spawn-request.ts | sed -n '1,165p'
printf '%s\n' '--- renderer references ---'
rg -n -C 8 --glob '*.ts' --glob '!*.test.ts' -e 'wslRestoreStartArgs' -e 'ipcPtySpawnRequest' -e 'wslPassiveRestore' src/renderer/src/components/terminal-pane

Repository: stablyai/orca

Length of output: 27411


Script executed:

set -eu
printf '%s\n' '--- deferred reattach request construction ---'
nl -ba src/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-connect.ts | sed -n '85,165p'
printf '%s\n' '--- deferred reattach callers and admitted identity ---'
rg -n -C 12 --glob '*.ts' --glob '!*.test.ts' -e 'startDeferredSessionReattach' -e 'spawnIpcPty' -e 'admittedSessionId' src/renderer/src/components/terminal-pane/pty-connection src/renderer/src/components/terminal-pane
printf '%s\n' '--- connect session identity declarations ---'
rg -n -C 8 --glob '*.ts' --glob '!*.test.ts' -e 'sessionId:' -e 'cacheKey' src/renderer/src/components/terminal-pane/pty-connection/connect-pane-pty-session.ts src/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-connect.ts

Repository: stablyai/orca

Length of output: 42180


Script executed:

set -eu
printf '%s\n' '--- startup cwd probe ---'
nl -ba src/main/ipc/pty/host-env/startup-cwd-directory-exists.ts
printf '%s\n' '--- WSL UNC existence probe references ---'
rg -n -C 10 --glob '*.ts' --glob '!*.test.ts' -e 'wslUncDirectoryExistsAsync' -e 'function wslUncDirectoryExistsAsync' src
printf '%s\n' '--- WSL implementation context ---'
rg -n -C 15 --glob '*.ts' --glob '!*.test.ts' -e 'export.*wslUncDirectoryExistsAsync' -e 'wslUncDirectoryExistsAsync' src/main/wsl* src/shared 2>/dev/null || true

Repository: stablyai/orca

Length of output: 30889


Do not skip the passive hold for sessionless or new-session requests.

holdPassiveRestoreOfStoppedWslDistro returns before reading the distro when sessionId is absent or isNewSession is true. The IPC contract permits wslPassiveRestore: true without sessionId, so local requests can reach the cwd probe without the hold. Daemon requests without an ID receive a minted ID but still skip through isNewSession.

For a WSL UNC cwd, the probe invokes wsl.exe for the distro. That command can start a stopped distro.

The current renderer reattach producer always supplies a session ID. The IPC boundary does not enforce that invariant, so the helper should apply the passive hold based on the flag and distro, not session ownership.

Suggested fix
 export async function holdPassiveRestoreOfStoppedWslDistro(
-  opts: Pick<PtySpawnOptions, 'wslPassiveRestore' | 'sessionId' | 'isNewSession' | 'attachOnly'>,
+  opts: Pick<PtySpawnOptions, 'wslPassiveRestore' | 'attachOnly'>,
   wslDistro: string | undefined
 ): Promise<void> {
   if (
     opts.wslPassiveRestore !== true ||
     !wslDistro ||
-    !opts.sessionId ||
-    opts.isNewSession === true ||
     opts.attachOnly === true
   ) {

Comment on lines +80 to +82
resolveWslSessionContext({
cwd: args.cwd,
sessionId: args.sessionId,

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.

Major · Functional Correctness · Heavy lift

Resolve the probe’s distro before applying the hold.

For a WSL worktree with a POSIX cwd and a local numeric session ID, this lookup can return no distro. resolveStartupCwdWithFallback instead derives the distro from the workspace path and runs wsl.exe to probe it. A stopped distro can therefore start before the hold. Resolve the workspace-derived distro before the probe and check that distro. (raw.githubusercontent.com)

@nwparker
nwparker merged commit d865074 into main Oct 29, 2026
50 of 51 checks passed
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