fix: shell default profile name type guard#687
Conversation
…oLowerCase() config.get<string>() has no runtime type check, so non-string values (number, boolean, array, object) from settings.json pass through and cause a TypeError. Add typeof guard in each platform's terminal config helper and add corresponding test cases.
…getTerminalConfig<T>()
daewoongoh
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults 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)
📝 WalkthroughWalkthroughReplaces three platform-specific VS Code terminal config helpers with one typed ChangesTerminal Config Refactor and Robustness
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. 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 |
daewoongoh
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
|
||
| function getLinuxTerminalConfig() { | ||
| function getTerminalConfig<T extends TerminalProfiles>( | ||
| platformKey: string, |
There was a problem hiding this comment.
Could this be narrowed to "windows" | "osx" | "linux"? Passing "darwin" instead of "osx" would silently return empty config today. Terminal.getPlatformProfileKey() already returns this union.
Taking it a step further, a mapped-type approach would also eliminate the explicit <T> at each call site and prevent mismatches like getTerminalConfig<MacTerminalProfiles>("windows"):
| platformKey: string, | |
| platformKey: "windows" | "osx" | "linux", |
There was a problem hiding this comment.
I replaced the TerminalProfiles union with a PlatformProfilesMap, so getTerminalConfig() now accepts only "windows" | "osx" | "linux" and infers the matching profile map type from the platform key.
This removes the explicit generic type argument at each call site and makes mismatches like getTerminalConfig("windows") a compile-time error.
| const profiles = config.get<LinuxTerminalProfiles>("profiles.linux") || {} | ||
| const rawProfileName = config.get<string>(`defaultProfile.${platformKey}`) | ||
| const defaultProfileName = typeof rawProfileName === "string" ? rawProfileName : null | ||
| const profiles = config.get<T>(`profiles.${platformKey}`) || ({} as T) |
There was a problem hiding this comment.
Should this be ?? instead of ||? The rest of the codebase is moving toward nullish coalescing for config reads (e.g. Task.ts:3696, extension.ts:155), and ?? is more precise — only fires on null/undefined.
| const profiles = config.get<T>(`profiles.${platformKey}`) || ({} as T) | |
| const profiles = config.get<T>(`profiles.${platformKey}`) ?? ({} as T) |
There was a problem hiding this comment.
Agreed. I changed the profiles fallback from || to ?? so we only fall back to an empty profile map for null/undefined, which is more precise for config reads.
| it("macOS: handles numeric defaultProfileName without TypeError", () => { | ||
| Object.defineProperty(process, "platform", { value: "darwin" }) | ||
| const mockConfig = { | ||
| get: vi.fn((key: string) => { | ||
| if (key === "defaultProfile.osx") return 1 | ||
| if (key === "profiles.osx") return {} | ||
| return undefined | ||
| }), | ||
| } | ||
| vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) | ||
|
|
||
| expect(getShell()).toBe("/bin/zsh") | ||
| }) | ||
|
|
||
| it("macOS: handles object defaultProfileName without TypeError", () => { | ||
| Object.defineProperty(process, "platform", { value: "darwin" }) | ||
| const mockConfig = { | ||
| get: vi.fn((key: string) => { | ||
| if (key === "defaultProfile.osx") return {} | ||
| if (key === "profiles.osx") return {} | ||
| return undefined | ||
| }), | ||
| } | ||
| vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) | ||
|
|
||
| expect(getShell()).toBe("/bin/zsh") | ||
| }) |
There was a problem hiding this comment.
Two things about the macOS/Linux non-string tests:
-
Asymmetric coverage — Windows tests all 4 non-string types (numeric, boolean, array, object) but macOS and Linux only test 2. The PR description commits to covering all four.
-
Mutation resistance — these tests won't actually catch removal of the
typeofguard.getMacShellFromVSCodenever calls.toLowerCase()ondefaultProfileName— it only uses it as an object key. So a non-string value silently misses the key lookup →undefined→normalizeShellPathreturnsnull→ falls back to/bin/zsh, which is exactly what the test asserts. Could you add a case where the non-string value, if left unguarded, would match a profile key (making the guarded vs. unguarded paths observably different)?
There was a problem hiding this comment.
I added coverage for this in 9fbc9e1.
Changes made:
- Added the missing boolean and array
defaultProfileNamecases for macOS and Linux so they now match the Windows non-string coverage. - Added mutation-resistant macOS/Linux tests where a numeric non-string value would otherwise match a real
"1"profile key if thetypeofguard were removed.
github-actions
Bot
added
the
awaiting-author
- Replace TerminalProfiles union with PlatformProfilesMap mapped type so the return type is inferred from the platform key, eliminating explicit type parameters at each call site and making mismatches a compile error - Change || to ?? when falling back to empty profiles object - Add JSDoc explaining the key constraint and inferred return type - Add missing boolean/array non-string defaultProfileName test cases for macOS and Linux to match Windows coverage - Add mutation-resistant tests for macOS and Linux that verify the typeof guard is load-bearing (numeric key matching a real profile entry)
github-actions
Bot
added
awaiting-review
2 participants
Related GitHub Issue
Closes: #686
Description
config.get<string>()insrc/utils/shell.tsprovides only a compile-time type hint — if a user sets a non-string value (number, boolean, array, object) insettings.json, it is returned as-is at runtime. This causes a TypeError when.toLowerCase()is subsequently called ondefaultProfileName.typeof === "string"runtime type guard to coerce non-string values tonullgetWindowsTerminalConfig,getMacTerminalConfig, andgetLinuxTerminalConfiginto a single genericgetTerminalConfig<T>(platformKey)Test Procedure
vitest run src/utils/__tests__/shell.spec.ts— all 53 tests passdefaultProfileNamescenarios (number, boolean, array, object) return the platform fallback shell without throwing TypeErrorPre-Submission Checklist
Screenshots / Videos
N/A
Documentation Updates
Additional Notes
config.get<T>()does not guarantee type safety at runtime due to VS Code API behavior — runtime validation at the system boundary is necessary.Get in Touch
hehegwk_23849
Summary by CodeRabbit
Tests
Refactor