Skip to content

test(wsl): pin the stopped-distro hold for a non-WSL shell in a WSL UNC folder - #35969

Merged
nwparker merged 2 commits into
mainfrom
nwparker/wsl-unc-cwd-hold-tests
Oct 29, 2026
Merged

nwparker merged 2 commits into
mainfrom
nwparker/wsl-unc-cwd-hold-tests

Conversation

@nwparker

@nwparker nwparker commented Oct 29, 2026 •

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

ELI5

This PR adds tests only. A review of #35938 asked whether a PowerShell, cmd or Git Bash terminal opened in a WSL folder (\\wsl.localhost\... or \\wsl$\...) could skip the stopped-distro check on restore. It can't. These tests lock that in.

What Changed

Nothing changes for users; there is no source change.

  • Local terminal path: the launch plan sees a WSL UNC folder and runs wsl.exe in that distro whatever shell was chosen, so the hold always knows the distro.
  • Daemon path: the distro comes from the UNC folder first, before the session or the shell.

The new tests cover both paths, each with a PowerShell shell override in a non-WSL workspace:

  • a passive restore from either UNC spelling is held while the distro is stopped, and nothing is spawned;
  • a launch the user opens isn't held and doesn't read the distro state;
  • a Windows (host) folder is left alone.

The local test file's ../wsl mock now uses the real shared UNC parser instead of a regex that only knew \\wsl.localhost.

Why

The gap reported during review would only have existed if a non-WSL shell kept a UNC folder without a distro. Tests on both providers are cheaper than new code, and they would catch a future change that stops forcing wsl.exe for UNC folders.

Linked Issue

Internal WSL rewrite, follow-up to the review of #35938.

Visual Proof

N/A. Tests only.

Testing

  • I manually tested these changes locally
  • Automated tests added/updated, or explained why not below

Both test files pass: 43 tests. typecheck:ci, oxlint and check:code-quality:changed pass. Tested on macOS with Windows mocked.

Review

  • Flag-off: no change (tests only).
  • User-risk: none (tests only).

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

No impact on SSH, folder workspaces, mobile or performance; this PR only adds tests.

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)

@greptile-apps

greptile-apps Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

[Low impact] This test-only PR appears safe to merge.

Summary

Adds tests for a PowerShell terminal opened in a WSL UNC folder.

  • Passive WSL restores stay on hold for either UNC path spelling.

No actionable issues found. Tests were not run during this review.

Reviews (1) · Last reviewed commit: "test(wsl): pin the UNC-cwd hold for a no..." · Reviewed by Greptile

@coderabbitai

coderabbitai Bot commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent 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: 68f9d77a-4e29-4ae7-8d64-42d8195a445e
Commits

Reviewing files that changed from the base of the PR and between 3f8b15d and 67f9695.

Files selected for processing: 2
  • src/main/daemon/daemon-pty-adapter-wsl-passive-restore-hold.test.ts
  • src/main/providers/local-pty-provider-windows-shell-launch.test.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.

Walkthrough

The tests cover passive restores from non-WSL shells with WSL UNC working directories. They check that a stopped distro produces a hold without spawning, while a user-initiated launch does not read distro state. They also check that host-path working directories do not trigger a distro-state read. The provider test mock now uses the real WSL UNC parser to cover both supported path spellings.

Priority: Low

Merge Risk: Minimal · up to 67f96

The change adds test coverage without identified merge-blocking risk.

Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed The title clearly identifies the test coverage for stopped-distro holds in WSL UNC folders with non-WSL shells.
Description check Passed The description is mostly complete and explains the scope, test coverage, rationale, validation, and cross-platform impact. The Linked Issue section does not provide an actual issue link or Fixes refe…
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
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: Description check

Explanation

The description is mostly complete and explains the scope, test coverage, rationale, validation, and cross-platform impact. The Linked Issue section does not provide an actual issue link or Fixes reference, and the AI Disclosure section is omitted.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. 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 0 functions across 2 files.

Finishing Touches
📝 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.

@nwparker
nwparker merged commit 64c0992 into main Oct 29, 2026
39 of 40 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