Fix Windows AI Vault resumes using the wrong shell - #13420
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughWSL PTY startup now distinguishes WSL-owned POSIX paths from Windows paths during directory validation. It probes WSL UNC directories asynchronously and applies worktree fallback only when the requested directory is missing and the workspace root exists. AI Vault resume construction now resolves startup shells and returns startup working directories separately from command text. Copy commands still embed working directories. New-tab launches forward session working directories to web sessions and regular tab startup state. Tests cover WSL, Windows, POSIX, SSH, UNC, remote, Codex, and OMP resume cases. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
d514279 to
ff99af3
Compare
![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: Pro Plus
Run ID: b7617526-fe13-4790-8cdb-2e7fc635810e
📒 Files selected for processing (6)
src/main/ipc/pty.test.tssrc/main/ipc/pty.tssrc/main/providers/local-pty-provider.test.tssrc/main/providers/local-pty-provider.tssrc/main/wsl.test.tssrc/main/wsl.ts
| cacheKey: 'repo-1:wsl' | ||
| } | ||
| } | ||
| })) as { startupCwdFallback?: { kind: string; cwd: string } } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the repeated type assertions.
The repeated })) as ... lines make this test file invalid TypeScript.
src/main/ipc/pty.test.ts#L12211-L12211: keep one closing type assertion for the spawn result.src/main/ipc/pty.test.ts#L12275-L12275: keep one closing type assertion for the parameterized spawn result.src/main/ipc/pty.test.ts#L12363-L12363: keep one closing type assertion for the Windows-path spawn result.
📍 Affects 1 file
src/main/ipc/pty.test.ts#L12211-L12211(this comment)src/main/ipc/pty.test.ts#L12275-L12275src/main/ipc/pty.test.ts#L12363-L12363
ff99af3 to
38ceab0
Compare
1 participant
Closes #13095
Summary
cd ... &&in queued shell text.Deterministic repro: on Windows, set the global terminal shell to
wsl.exe, pin the project runtime towindows-host, and resume an AI Vault session whose cwd isC:\Users\alice\repo. Before this change Orca queued POSIXcd ... &&syntax into the PowerShell tab; PowerShell 5.1 rejects&&. The launch now queues only the agent invocation and sends cwd separately.Screenshots
No visual change.
Testing
pnpm lintpnpm typecheckpnpm testpnpm buildAdditional validation:
git diff --checkpass.&&parse failure and accepts the new startup path.The complete
pty.test.tsfile has three unrelated Windows-only failures outside the changedcwdFallbackbranch (two POSIX-path expectations and one existing Codex provenance expectation); 439 tests passed and 19 skipped in that pre-rebase run.AI Review Report
Three adversarial review passes covered Windows host/WSL/cmd/Git Bash behavior, SSH and runtime launches, folder workspaces, drag/drop compatibility, Codex-home/provider-session handling, daemon boundaries, mixed versions, and performance.
The first review found that actual-shell metadata was attempting to solve physical executable fallback incompletely, so that entire daemon/preload/transport tranche was removed and the PR was narrowed to #13095. It also found an over-broad WSL cwd fallback bypass; the guard was narrowed to WSL-owned POSIX namespaces and a discriminating Windows-path fallback test was added. Two fresh independent correctness reviewers then returned CLEAN on the same final round. The performance reviewer found no hot-path, IPC, process-probe, or retention regression.
Cross-platform review explicitly covered macOS, Linux, Windows, native Windows host shells, WSL path translation, Git Bash/cmd quoting, SSH, runtime-owned workspaces, folder workspaces, and Electron PTY startup behavior. No keyboard shortcut or UI-label behavior changed.
Security Audit
No new dependency, privilege, network, secret, or external process surface is introduced. Session cwd remains input to the existing PTY startup-cwd resolver and provider validation. The review checked command construction, quoting, path namespaces, Codex account-home routing, environment deletion, provider-session provenance, IPC boundaries, SSH isolation, and remote wire compatibility. No new wire field is introduced.
Notes
This intentionally does not attempt to reserialize startup commands after a configured executable physically falls back to a different shell family. A correct solution for that separate problem needs complete per-shell command and launch-config variants selected by the provider for each spawn attempt; command-only rewriting would be incomplete.
Linked PR #13238 was reviewed but not used because it covers cold restore rather than Resume in Worktree and its UUID-only test does not distinguish PowerShell from POSIX quoting.