fix(shell): delegate VS Code profile reads to Terminal methods (issue #634)

[edelauna]

Related GitHub Issue

Closes: #634

Description

Root cause: On Windows, getShell() (used to build the system prompt) called config.get("defaultProfile.windows") which merges all VS Code scopes including workspace. Terminal.getConfiguredDefaultProfileName() (used when actually opening a terminal) calls config.inspect().globalValue — intentionally excluding workspace scope, because terminal.integrated.defaultProfile.* is an APPLICATION-scoped setting. A workspace .vscode/settings.json can contain the key, VS Code accepts it, but the terminal ignores it for security. The result: the system prompt told the model "you have PowerShell" while the terminal opened cmd.exe (or whatever VS Code autodetected), causing every shell command to fail.

Fix: Deleted the parallel implementation in shell.ts (getTerminalConfig, normalizeShellPath, getWindowsShellFromVSCode, etc.) and replaced it with a single getShellFromVSCode() that delegates to Terminal.getConfiguredDefaultProfileName() and Terminal.getConfiguredProfiles(). Both paths now read from the same inspect()-based source of truth. Also extracted preferredWindowsPowerShell() to eliminate a duplicated PS7→legacy fallback expression.

Tests:

• Rewrote shell.spec.ts stubs to use inspect() (matching the new implementation) and added a "Scope isolation" section asserting workspace-scoped profiles are ignored.
• Added shell-system-prompt-divergence.spec.ts as a regression anchor: confirms Terminal ignores workspace-scoped profiles, verifies both paths agree when a profile is set at global scope, and verifies convergence after the fix (workspace-only profile → both return the Windows no-profile fallback).

Test Procedure

Unit tests:

pnpm exec vitest run utils/__tests__/shell.spec.ts integrations/terminal/__tests__/shell-system-prompt-divergence.spec.ts

E2E mock suite (76 passing, 3 pre-existing xAI failures unrelated to this change):

pnpm --filter @roo-code/vscode-e2e test:ci:mock

Pre-Submission Checklist

• Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
• Scope: My changes are focused on the linked issue (one major feature/fix per PR).
• Self-Review: I have performed a thorough self-review of my code.
• Testing: New and/or updated tests have been added to cover my changes (if applicable).
• Visual Snapshot (UI changes only): N/A — no UI changes.
• Documentation Impact: No documentation updates required.
• Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Additional Notes

The 3 failing E2E tests are all in the xAI provider suite and fail with a 403 "credits exhausted" error — pre-existing and unrelated to this change.

Summary by CodeRabbit

• Bug Fixes
  • Improved terminal shell detection across Windows, macOS, and Linux.
  • Workspace-only terminal profile settings are now handled consistently and no longer incorrectly override applicable defaults.
  • Added more reliable fallback behavior for PowerShell, WSL, user-configured shells, environment settings, and unavailable shell paths.
  • Improved validation of configured shell paths, including Windows path handling and safe rejection of invalid or unsupported shells.

[coderabbitai]

📝 Walkthrough

Walkthrough

Shell detection now uses shared VS Code terminal profile inspection across platforms, with updated fallback and validation tests. Regression coverage verifies workspace-scope isolation and agreement between terminal detection and system-prompt shell selection.

Changes

Shell resolution

Layer / File(s)	Summary
Unified VS Code profile resolver src/utils/shell.ts	getShell() now resolves profiles through Terminal, handles source-only profiles, selects platform fallbacks, and validates resolved paths.
Resolver behavior and regression coverage src/utils/__tests__/shell.spec.ts	Tests cover profile paths, arrays, platform fallbacks, scope isolation, error handling, origin validation, WSL handling, and allowlisting.
System-prompt and terminal convergence src/integrations/terminal/__tests__/shell-system-prompt-divergence.spec.ts	Regression tests verify consistent handling of workspace-only and global terminal profiles between Terminal and getShell().

Lint metadata

Layer / File(s)	Summary
Suppression count update src/eslint-suppressions.json	The recorded no-explicit-any suppression count for safeWriteJson.test.ts is reduced from 46 to 35.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant getShell
  participant Terminal
  participant VSCode
  participant FileSystem
  getShell->>Terminal: read configured default profile
  Terminal->>VSCode: inspect global terminal settings
  Terminal->>FileSystem: validate resolved shell path
  FileSystem-->>Terminal: path availability
  Terminal-->>getShell: return shell or fallback

Loading

Possibly related issues

• [BUG] PowerShell cmdlets fail when Inline Terminal is ON — execa uses cmd.exe instead of user shell #705 — Both address unified VS Code shell-profile detection and Windows inline-terminal shell selection.
• [Feature Request] Allow users to select and configure the execution shell for the inline terminal #779 — Both update shell detection to honor VS Code terminal profiles and resolve Windows shell mismatches.

Possibly related PRs

• Zoo-Code-Org/Zoo-Code#239 — Both change Windows fallback behavior when no VS Code terminal profile is configured.
• Zoo-Code-Org/Zoo-Code#687 — Both modify VS Code default-profile resolution and defaultProfileName handling.

Suggested labels: awaiting-review

Suggested reviewers: taltas

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The code aligns shell detection with VS Code terminal profile resolution and adds regression tests for the #634 mismatch.
Out of Scope Changes check	✅ Passed	No unrelated code changes stand out; the suppression update and new tests are tied to the shell fix.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The title is concise and accurately summarizes the main shell profile delegation change tied to issue #634.
Description check	✅ Passed	It includes the required issue link, implementation summary, test procedure, checklist, and notes; only optional sections are blank.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch issue/634

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (4)

src/utils/__tests__/shell.spec.ts (3)

323-380: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Fold the workspace-scope stub into mockVsCodeConfig.

Both scope-isolation tests re-implement the helper with workspaceValue instead of globalValue; a scope: "global" | "workspace" option would remove ~40 duplicated lines and keep the two stubs from drifting.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/__tests__/shell.spec.ts` around lines 323 - 380, Update
mockVsCodeConfig to accept a scope option such as "global" or "workspace" and
populate the corresponding inspect value while leaving the other scope
undefined. Refactor both scope-isolation tests to use this helper instead of
redefining vscode.workspace.getConfiguration, preserving their platform-specific
profile and shell expectations.

---

453-460: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Test name overstates what it asserts.

The array is [<allowlisted PS7>, "pwsh"], so first-entry and first-allowlisted resolution are indistinguishable. Put a non-allowlisted entry first if the intent is to pin "first allowlisted"; otherwise rename to "uses first path in array".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/__tests__/shell.spec.ts` around lines 453 - 460, Update the test
around getShell() so it distinguishes first-allowlisted resolution from simply
selecting the first array entry: place a non-allowlisted path before the valid
PowerShell path while retaining the expected allowlisted result. Keep the test
name only if it now verifies that behavior; otherwise rename it to describe
first-array-entry selection.

---

39-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

_platformKey is now unused — the stub ignores the inspected key.

Every call site still passes "windows"/"osx"/"linux", which reads as if platform-specific keys are being exercised when they are not. Either drop the parameter or have inspect assert the expected key (defaultProfile.<platformKey> / <platformKey>) so the tests actually pin the key names the implementation reads.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/__tests__/shell.spec.ts` around lines 39 - 60, The mockVsCodeConfig
helper accepts an unused _platformKey, so the tests do not validate
platform-specific configuration keys. Update both inspect stubs in
mockVsCodeConfig to assert the expected key names using
platformKey—defaultProfile.<platformKey> for terminal.integrated and
<platformKey> for terminal.integrated.profiles—while preserving their existing
returned values.

src/utils/shell.ts (1)

138-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Optional: extract the PowerShell-7-or-legacy selection.

The same existsSync(POWERSHELL_7) ? … : POWERSHELL_LEGACY expression appears twice.

♻️ Proposed refactor

+function preferredWindowsPowerShell(): string {
+	return existsSync(SHELL_PATHS.POWERSHELL_7) ? SHELL_PATHS.POWERSHELL_7 : SHELL_PATHS.POWERSHELL_LEGACY
+}

Also applies to: 156-165

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/shell.ts` around lines 138 - 140, Extract the duplicated PowerShell
7-or-legacy path selection into a shared helper or local symbol, then reuse it
in both Windows branches of the shell-detection logic. Preserve the existing
existsSync(SHELL_PATHS.POWERSHELL_7) preference and legacy fallback behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/integrations/terminal/__tests__/shell-system-prompt-divergence.spec.ts`:
- Around line 1-13: Update the header comment in the shell-system-prompt
divergence regression test to describe the post-fix behavior: getShell() now
uses Terminal.getConfiguredDefaultProfileName() and getConfiguredProfiles()
rather than config.get(), so remove the obsolete workspace-scope divergence
explanation and revise the test description to reflect the behavior being
protected.
- Around line 151-168: The divergence spec still describes pre-fix behavior;
update
src/integrations/terminal/__tests__/shell-system-prompt-divergence.spec.ts lines
151-168 to assert convergence, with getShell() ignoring the workspace-only
profile and returning the Windows no-profile fallback. Update lines 1-13 to
document that both paths use global/default scope, and revise lines 45-53 to
correct the helper documentation and remove the obsolete terminal.integrated
get() branch.
- Around line 45-53: Update the stub helper’s documentation near getShell() to
remove the obsolete explanation that getShell() reads the merged
terminal.integrated get() value and that the get() branch is exercised. Keep
only comments describing the configuration behavior still relevant to the code
under test, including the inspect() handling if applicable.

---

Nitpick comments:
In `@src/utils/__tests__/shell.spec.ts`:
- Around line 323-380: Update mockVsCodeConfig to accept a scope option such as
"global" or "workspace" and populate the corresponding inspect value while
leaving the other scope undefined. Refactor both scope-isolation tests to use
this helper instead of redefining vscode.workspace.getConfiguration, preserving
their platform-specific profile and shell expectations.
- Around line 453-460: Update the test around getShell() so it distinguishes
first-allowlisted resolution from simply selecting the first array entry: place
a non-allowlisted path before the valid PowerShell path while retaining the
expected allowlisted result. Keep the test name only if it now verifies that
behavior; otherwise rename it to describe first-array-entry selection.
- Around line 39-60: The mockVsCodeConfig helper accepts an unused _platformKey,
so the tests do not validate platform-specific configuration keys. Update both
inspect stubs in mockVsCodeConfig to assert the expected key names using
platformKey—defaultProfile.<platformKey> for terminal.integrated and
<platformKey> for terminal.integrated.profiles—while preserving their existing
returned values.

In `@src/utils/shell.ts`:
- Around line 138-140: Extract the duplicated PowerShell 7-or-legacy path
selection into a shared helper or local symbol, then reuse it in both Windows
branches of the shell-detection logic. Preserve the existing
existsSync(SHELL_PATHS.POWERSHELL_7) preference and legacy fallback behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 22b214de-4359-4bc7-ad65-21ebf84dda4b

📥 Commits

Reviewing files that changed from the base of the PR and between 782e28e and 6a6df2c.

📒 Files selected for processing (4)

• src/eslint-suppressions.json
• src/integrations/terminal/__tests__/shell-system-prompt-divergence.spec.ts
• src/utils/__tests__/shell.spec.ts
• src/utils/shell.ts