feature: unified-shell-resolution (2/4) - #1125
Conversation
Merge feature/unified-shell-resolution into pr/b04-shell-contracts-v2. Combines B04's command_output ask delay with B05's shell resolution system (ShellResolver, ShellInvocationAdapter, TerminalProfileResolver, CommandEnvironmentService, CommandScheduler). Conflict resolution in ExecuteCommandTool.ts: - Kept B05 ShellFallbackMismatchError + enhanced getTerminalProviderForExecution - Kept B04 COMMAND_OUTPUT_ASK_DELAY_MS + command_output ask delay logic - Merged onShellExecutionStarted signature (process param from B04 + traceBuilder from B05) - Combined commandStartedAt fallback with ExecaTerminal shell invocation plan Conflict resolution in executeCommandTool.spec.ts: - Kept both B04 command_output ask policy tests and B05 cwd parameter validation tests Note: no-explicit-any lint errors are pre-existing in feature/unified-shell-resolution
…s for new test files, update counts for modified files
…onmentService - fixes e2e terminal-profile test where no VS Code terminal was created because provider was hardcoded to execa
- reserveTerminal: guard integration-ready self-transition when reusing a
terminal already in integration-ready state (fixes IllegalTransitionError
in e2e shell-race tests; the "404 No fixture matched" OpenRouter errors
were a downstream symptom).
- classifyShellFamily: use separator-agnostic basename instead of
path.basename so Windows paths classify correctly on POSIX hosts
(fixes ubuntu getProfileShell("win32") returning undefined for Git Bash).
- ExecaTerminal.runCommand: transition from creating/idle to fallback-ready
so setActiveStream's -> running transition is legal for directly
constructed terminals (fixes ubuntu ExecaTerminal onLine not firing).
- TerminalRegistry: replace two as-any casts with proper types
(removes no-explicit-any lint errors without touching suppressions).
…ode-sync cachedState reset - Terminal.ts: When resolvedEnv is present, also check Terminal.getProfileShell() for shellArgs and pass them to vscode.window.createTerminal(). This fixes the e2e-mock terminal-profile test where creationOptions.shellArgs was missing --noprofile/--norc from the configured Bash profile. - SettingsView.tsx: Re-apply mode-based cachedState sync from ac0ed1b that was reverted by a68ac23 (B05 merge). The useEffect now resets cachedState when either currentApiConfigName OR mode changes, fixing platform-unit-test failures on both ubuntu and windows.
… os-name in shell-env prompt spec - Terminal.ts waitForShellIntegration: skip integration-ready/integration-pending transitions when already in integration-ready/fallback-ready. Reused VS Code terminals promoted by the registry fire the readiness path while already in integration-ready, causing IllegalTransitionError (integration-ready → integration-ready) and 6 e2e-mock failures (long-running-silent-command, terminal-reuse-shell-race, zero-chunk-shell-race). - shell-environment-prompt.spec.ts: mock os-name to avoid spawning PowerShell per test. Under coverage instrumentation on windows-latest this exceeded the 20s test timeout (8 getSystemInfoSection failures). Matches all sibling prompt specs.
…d env resolution Task.resolveCommandEnvironment() only read terminalProfile from persisted provider state, ignoring programmatic overrides set via api.setTerminalProfile(). This caused the ShellResolver to resolve the default shell instead of the profile override, leading to e2e test timeout in terminal-profile.test.ts. Fix: fall back to Terminal.getTerminalProfile() when state.terminalProfile is undefined, and invalidate the CommandEnvironmentService cache in api.setTerminalProfile() so the next task re-resolves with the new profile.
…rminalProfile The mock sidebarProvider in unit tests may not have getCommandEnvironmentService. Use ?.() optional call syntax to tolerate missing method.
… tests The profile-override test flaked in CI (run 30752014262): the custom --noprofile/--norc bash terminal did not emit the OSC 633;A shell-integration marker within the default 5s window on a loaded runner, aborting with SI_ACTIVATION_TIMEOUT and hitting the 90s waitUntilCompleted budget. Set terminalShellIntegrationTimeout to 30s in both Terminal Profile task configurations so shell integration has time to activate.
…al-profile e2e Root cause of persistent Terminal Profile e2e flake (runs 30752014262, 30760530287): the previous fix set terminalShellIntegrationTimeout via the per-task startNewTask configuration, but that settings key is only applied through the webview config-applier (ClineProvider). The extension-host API setConfiguration path (contextProxy.setValues) never reaches Terminal.setShellIntegrationTimeout, so the activation window stayed at the default 5s and the --noprofile/--norc bash profile terminal aborted with SI_ACTIVATION_TIMEOUT on loaded CI runners (terminal create -> abort exactly 5.000s). - Add API.setShellIntegrationTimeout(timeoutMs) that updates the Terminal static immediately, and declare it on the RooCodeAPI interface. - terminal-profile.test.ts now calls setShellIntegrationTimeout(30_000) in suiteSetup (restored to 5_000 in suiteTeardown) and drops the ineffective per-task config keys.
The --noprofile/--norc bash profile depends on VS Code injecting shell integration via the shell startup path. On loaded CI runners that injection intermittently exceeds even a 30s activation window (run 30761508190: terminal created 18:40:49.05, abort 18:41:19.05 = exactly 30s, SI never fired). Each mocha retry runs the test against a freshly created terminal, which typically lets SI activate. Matches the retries:3 pattern already used by apply-diff.
…ARCH-TERMINAL-002) Remove --norc from the terminal-profile E2E test so VS Code can inject shell integration through the Bash startup path. --norc disables .bashrc reading, which makes shell integration physically impossible. - Change profile args from --noprofile --norc to --noprofile - Remove Mocha retries (the failure was deterministic, not flaky) - Remove 30s shell-integration timeout override (test-only API) - Remove setShellIntegrationTimeout from RooCodeAPI and extension facade Split the single contradictory assertion into two contracts: 1. Compatible profile: proves profile selection + shell integration works 2. Incompatible profile (--norc): will prove typed Execa fallback (B07) Refs: ARCH-TERMINAL-002
--noprofile also blocks VS Code's bash shell integration injection (just like --norc). Use --login instead, which is safe for shell integration while still proving custom profile args pass-through.
The shell dropdown's onShellSelectionChange only updated the pending selection state; the Save button stayed disabled unless the unrelated onTerminalProfilePickerOpened hook happened to fire. Wrap the handler so a shell selection change explicitly calls setChangeDetected(true), enabling Save on shell-only changes. Behavior is otherwise identical.
📝 WalkthroughWalkthroughThe PR adds configurable terminal shell selection and deterministic shell resolution. It connects resolved shell environments to prompts and command execution, adds terminal lifecycle, scheduling, tracing, recovery, and provider fallback behavior, and exposes shell selection through the extension settings UI. ChangesTerminal shell contracts and resolution
Prompt and task integration
Terminal lifecycle and execution
Settings UI and webview
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
apps/vscode-e2e/src/suite/tools/terminal-profile.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. packages/types/src/__tests__/terminal-shell-settings.spec.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. packages/types/src/global-settings.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
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 Warning |
github-actions
Bot
added
the
awaiting-review
889f969 to
fe6c9a7
Compare
fe6c9a7 to
2d862ca
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: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (13)
packages/types/src/__tests__/terminal-shell-settings.spec.ts-60-62 (1)
60-62: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename the test to match the assertion.
The test name states rejection, but the assertion is
.not.toThrow(). The comment confirms thatz.string()accepts an empty string. Rename the test so the intent matches the behavior.♻️ Proposed rename
- it("should reject profile with empty profileName", () => { + it("should accept profile with empty profileName (host validates)", () => { expect(() => terminalShellSelectionSchema.parse({ kind: "profile", profileName: "" })).not.toThrow() // z.string() accepts empty; validation is extension-host responsibility })🤖 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 `@packages/types/src/__tests__/terminal-shell-settings.spec.ts` around lines 60 - 62, Rename the test case around terminalShellSelectionSchema.parse to state that a profile with an empty profileName is accepted, matching the existing not.toThrow assertion and explanatory comment.src/core/webview/__tests__/terminal-shell-messages.spec.ts-227-235 (1)
227-235: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep Auto option metadata consistent with resolution.
Line 232 and Line 306 set the Auto option family to
powershell, butmockEnv.primaryPlan.familyisposix. Line 340 accepts the incorrect value. Derive the option family from the resolved environment.Proposed fix
- family: "powershell", + family: env.primaryPlan.family, ... - family: "powershell", + family: env.primaryPlan.family, ... - family: "powershell", + family: "posix",Also applies to: 302-310, 337-343
🤖 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/core/webview/__tests__/terminal-shell-messages.spec.ts` around lines 227 - 235, Update the Auto terminal option setup in the affected test cases around the options arrays and assertions to derive family from the resolved environment, using mockEnv.primaryPlan.family instead of hard-coding "powershell". Ensure the assertions validate the resolved family consistently with the option metadata.webview-ui/src/i18n/locales/pt-BR/settings.json-850-864 (1)
850-864: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate the
terminal.inlineShellvalues.These non-English locale files contain English-only values. This creates a mixed-language settings UI.
webview-ui/src/i18n/locales/pt-BR/settings.json#L850-L864: Translate allterminal.inlineShellvalues into Brazilian Portuguese.webview-ui/src/i18n/locales/ru/settings.json#L850-L864: Translate allterminal.inlineShellvalues into Russian.webview-ui/src/i18n/locales/tr/settings.json#L850-L864: Translate allterminal.inlineShellvalues into Turkish.webview-ui/src/i18n/locales/vi/settings.json#L850-L864: Translate allterminal.inlineShellvalues into Vietnamese.webview-ui/src/i18n/locales/zh-CN/settings.json#L850-L864: Translate allterminal.inlineShellvalues into Simplified Chinese.webview-ui/src/i18n/locales/zh-TW/settings.json#L877-L891: Translate allterminal.inlineShellvalues into Traditional Chinese.🤖 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 `@webview-ui/src/i18n/locales/pt-BR/settings.json` around lines 850 - 864, Translate every value in the terminal.inlineShell object, including nested effectiveShell and error entries, into the target locale language without changing keys or structure: Brazilian Portuguese in webview-ui/src/i18n/locales/pt-BR/settings.json lines 850-864; Russian in webview-ui/src/i18n/locales/ru/settings.json lines 850-864; Turkish in webview-ui/src/i18n/locales/tr/settings.json lines 850-864; Vietnamese in webview-ui/src/i18n/locales/vi/settings.json lines 850-864; Simplified Chinese in webview-ui/src/i18n/locales/zh-CN/settings.json lines 850-864; and Traditional Chinese in webview-ui/src/i18n/locales/zh-TW/settings.json lines 877-891.webview-ui/src/components/settings/__tests__/TerminalSettings.shell.spec.tsx-164-175 (1)
164-175: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire the profile option before testing selection.
The conditional permits this test to pass when
profile:PowerShellis not rendered. UsegetByTestId("option-profile:PowerShell")and always execute the click and callback assertions.🤖 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 `@webview-ui/src/components/settings/__tests__/TerminalSettings.shell.spec.tsx` around lines 164 - 175, Update the profile selection test to use getByTestId("option-profile:PowerShell") instead of queryByTestId, removing the conditional guard so the click and onShellSelectionChange/onTerminalProfilePickerOpened assertions always execute and fail when the option is missing.webview-ui/src/components/settings/TerminalSettings.tsx-317-327 (1)
317-327: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRender
effectiveShell.label.The effective-shell panel shows only the family and source. It does not show the supplied executable label. Users cannot distinguish shells in the same family, such as
pwsh.exeandpowershell.exe.🤖 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 `@webview-ui/src/components/settings/TerminalSettings.tsx` around lines 317 - 327, Update the effective-shell panel in TerminalSettings to also render the supplied executable label using the existing effectiveShell.label translation and shellOptions.effectiveShell.label value, alongside the family and source fields.webview-ui/src/components/settings/TerminalSettings.tsx-334-341 (1)
334-341: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winShow an availability error for option discovery failures.
shellErrorcomes fromTerminalShellOptionsPayload.error, which reports shell-option discovery failure. The UI always renderserror.invalid, so an unavailable extension-host service is reported as an invalid user selection.Render
error.unavailablefor this payload path. Add an assertion for the displayed translation key.🤖 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 `@webview-ui/src/components/settings/TerminalSettings.tsx` around lines 334 - 341, Update the shellError rendering in TerminalSettings so this option-discovery failure displays the settings:terminal.inlineShell.error.unavailable translation instead of error.invalid. Add or update the component assertion for terminal-inline-shell-error to verify the unavailable translation key is shown.webview-ui/src/i18n/locales/ca/settings.json-849-865 (1)
849-865: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate the new
inlineShellvalues.The Catalan, German, Spanish, and French locale files contain English UI text. The terminal settings view changes language inside one section.
webview-ui/src/i18n/locales/ca/settings.json#L849-L865: add Catalan translations.webview-ui/src/i18n/locales/de/settings.json#L849-L865: add German translations.webview-ui/src/i18n/locales/es/settings.json#L849-L865: add Spanish translations.webview-ui/src/i18n/locales/fr/settings.json#L849-L865: add French translations.🤖 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 `@webview-ui/src/i18n/locales/ca/settings.json` around lines 849 - 865, Translate every English value in the inlineShell section into the appropriate locale language: Catalan in webview-ui/src/i18n/locales/ca/settings.json lines 849-865, German in webview-ui/src/i18n/locales/de/settings.json lines 849-865, Spanish in webview-ui/src/i18n/locales/es/settings.json lines 849-865, and French in webview-ui/src/i18n/locales/fr/settings.json lines 849-865. Preserve the existing keys and JSON structure while translating labels, descriptions, options, placeholders, and error messages.src/integrations/terminal/ExecaTerminalProcess.ts-23-27 (1)
23-27: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the terminal dereference in the
completedhandler.The
terminalgetter throws when theWeakReftarget is collected. This handler runs fromthis.emit("completed", ...)at the end ofrun, which is outside thetryblock. A throw there rejects therunpromise, andExecaTerminal.runCommanddoes not observe that rejection. Read the reference defensively in the handler.🛡️ Proposed fix
this.once("completed", () => { // Lifecycle: transition to idle on completion. // (architect report Section 1.4: ExecaTerminalProcess completion → idle) - this.terminal.lifecycle.resetToIdle() + this.terminalRef.deref()?.lifecycle.resetToIdle() })🤖 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/ExecaTerminalProcess.ts` around lines 23 - 27, Update the completed handler in ExecaTerminalProcess to read the terminal reference defensively before calling lifecycle.resetToIdle, avoiding the throwing terminal getter when the WeakRef target has been collected. Only reset the lifecycle when a terminal instance is available, while preserving the existing completion behavior otherwise.src/integrations/terminal/TerminalRegistry.ts-377-392 (1)
377-392: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRemove the broken terminal from the registry.
When a healthy idle VS Code terminal has lost shell integration, this branch marks it broken, disposes the VS Code terminal, and then only continues the loop. The wrapper stays in
this.terminals.getAllTerminalsremoves entries only whenisClosed()is true, andexitStatusis not set synchronously afterdispose(). The disposed wrapper is therefore re-evaluated on every later search and its ZDOTDIR map entry stays alive until the close event arrives. Remove it directly.♻️ Proposed fix
terminal.lifecycle.markBroken() if (terminal instanceof Terminal) { terminal.terminal.dispose() - ShellIntegrationManager.zshCleanupTmpDir(terminal.id) } + this.removeTerminal(terminal.id) continue
removeTerminalalready callsShellIntegrationManager.zshCleanupTmpDir.🤖 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/TerminalRegistry.ts` around lines 377 - 392, Update the broken-terminal branch in the registry scan to remove the affected terminal wrapper directly after marking it broken, instead of only disposing it and continuing. Reuse the existing removeTerminal method for this cleanup, and avoid separately calling ShellIntegrationManager.zshCleanupTmpDir because removeTerminal already handles it.src/integrations/terminal/TerminalRegistry.ts-850-858 (1)
850-858: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
priorTerminalStatealways reportsdisposed.The trace reads
source.lifecycle.stateafter Line 850 transitions the source todisposed. The field therefore never carries the state that preceded the switch, which removes the diagnostic value of the trace. Capture the state before thefailedtransition and pass the captured value.♻️ Proposed fix
+ const priorTerminalState = source.lifecycle.state + // 1. Transition source to failed. source.lifecycle.transition("failed", executionId)- priorTerminalState: source.lifecycle.state, + priorTerminalState,🤖 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/TerminalRegistry.ts` around lines 850 - 858, Capture the source terminal lifecycle state before the transition to "disposed" in the terminal switch flow, then pass that captured value as priorTerminalState in emitCommandTrace. Keep the existing transition and trace emission behavior unchanged while ensuring the field reflects the state preceding disposal.src/integrations/terminal/CommandTrace.ts-170-174 (1)
170-174: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
markShellIntegrationActivatedAtoverwritesshellIntegrationInitiallyAvailable.
shellIntegrationInitiallyAvailablerecords whether shell integration was already available when the terminal was acquired.markShellIntegrationActivatedAtsets it totrue, which reports a late activation as an initial availability.ExecuteCommandToolsets the flag explicitly at terminal acquisition (markShellIntegrationInitiallyAvailable), and a later activation event then overwrites that value. This makes cold-start measurements unreliable.Record activation only in the timestamp field.
🔧 Proposed fix
markShellIntegrationActivatedAt(ts: number): this { this.trace.shellIntegrationActivatedAt = ts - this.trace.shellIntegrationInitiallyAvailable = true return this }🤖 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/CommandTrace.ts` around lines 170 - 174, Update markShellIntegrationActivatedAt in CommandTrace so it only records the activation timestamp in shellIntegrationActivatedAt. Remove the assignment that changes shellIntegrationInitiallyAvailable, preserving the value established by markShellIntegrationInitiallyAvailable during terminal acquisition.src/core/tools/__tests__/executeCommandTool.spec.ts-159-178 (1)
159-178: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThese tests no longer exercise
unescapeHtmlEntities.Each input now contains the literal character instead of the HTML entity, and the expected value equals the input. The assertions pass for any implementation, including an identity function. The test titles still describe entity decoding.
Restore entity inputs so the tests verify the decoding of
<,>, and&.💚 Proposed fix
- it("should unescape < to < character", () => { - const input = "echo <test>" + it("should unescape < to < character", () => { + const input = "echo <test>" const expected = "echo <test>" expect(unescapeHtmlEntities(input)).toBe(expected) })🤖 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/core/tools/__tests__/executeCommandTool.spec.ts` around lines 159 - 178, Update the tests around unescapeHtmlEntities so each input contains the corresponding encoded entity (<, >, and &) while expected values retain the decoded characters. Adjust the mixed-entity case similarly, preserving the existing test coverage and titles.src/integrations/terminal/__tests__/TerminalProcessExec.pwsh.spec.ts-100-101 (1)
100-101: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSet
resolvedShellFamilyin the PowerShell exec setup.
TerminalProcess.runnow derivesshellKind.isPowerShellfromthis.terminal.resolvedShellFamily, and that test’s reconstructedTerminaldefaults to"posix"because it passes no profile/shell context. Add the PowerShell marker beforeterminalProcess.run(), or assert the mocked command with the wrapper used by this path.🤖 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__/TerminalProcessExec.pwsh.spec.ts` around lines 100 - 101, Update the PowerShell test setup around mockTerminalInfo and TerminalProcess.run so the reconstructed Terminal has resolvedShellFamily configured as PowerShell before execution, or mock/assert the command through the wrapper used by this path. Preserve the existing lifecycle state and command expectations while ensuring the test exercises the PowerShell branch.
🧹 Nitpick comments (20)
packages/types/src/global-settings.ts (1)
111-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider requiring non-empty strings in the schema.
profileNameandpathaccept empty strings. The extension host currently rejects an empty path inShellResolver.tryResolveExplicitPath, so this is not exploitable today. A schema-levelmin(1)makes the contract self-enforcing for every future consumer.♻️ Proposed schema tightening
export const terminalShellSelectionSchema = z.discriminatedUnion("kind", [ z.object({ kind: z.literal("auto") }), - z.object({ kind: z.literal("profile"), profileName: z.string() }), - z.object({ kind: z.literal("path"), path: z.string() }), + z.object({ kind: z.literal("profile"), profileName: z.string().min(1) }), + z.object({ kind: z.literal("path"), path: z.string().min(1) }), ])Note: the existing test at
packages/types/src/__tests__/terminal-shell-settings.spec.tsline 61 asserts that an emptyprofileNameparses. Update that test if you apply this change.🤖 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 `@packages/types/src/global-settings.ts` around lines 111 - 115, Update terminalShellSelectionSchema so the profileName and path fields require non-empty strings using the schema’s minimum-length validation. Adjust the terminal-shell settings test to expect empty profileName values to be rejected while preserving valid selection behavior.src/utils/shell.ts (2)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExport
SHELL_ALLOWLISTas a read-only type.
Set<string>is exported mutably, so any importer can callSHELL_ALLOWLIST.add(...)and widen the trust boundary thatisShellPathAllowedenforces. This is hardening, not an exploitable path, because an attacker who can run code in the extension host already has that authority. Annotate the export asReadonlySet<string>so accidental mutation fails at compile time.🛡️ Proposed change
-export const SHELL_ALLOWLIST = new Set<string>([ +const SHELL_ALLOWLIST_ENTRIES = new Set<string>([Then add after the literal:
export const SHELL_ALLOWLIST: ReadonlySet<string> = SHELL_ALLOWLIST_ENTRIES🤖 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` at line 9, Update the SHELL_ALLOWLIST export in src/utils/shell.ts to use the ReadonlySet<string> type, preserving its existing entries and behavior while preventing consumers from mutating it through methods such as add.
460-475: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog ShellResolver fallback failures and update the documented shell-resolution chain.
resolveExecutable({})intentionally omits steps 2–5, but thegetShell()docstring still lists the full eight-step chain as if it applies. Update that section, addresolveExecutable()settings where callers need user-selected shells, and log anyTerminalProfileResolver.forRuntime()/ShellResolver.forRuntime()failures instead of falling through silently.🤖 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 460 - 475, Update getShell() documentation to describe only the resolution steps performed by resolveExecutable({}), and document the full chain separately only where applicable. Pass resolveExecutable() settings from callers that require user-selected shells, and replace the silent catch around TerminalProfileResolver.forRuntime() and ShellResolver.forRuntime() with logging of the failure before retaining legacy fallback behavior.Source: Coding guidelines
src/integrations/terminal/types.ts (1)
155-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
fromDetailsrecomputes defaults that the constructor already applies.Lines 158-159 duplicate the
outcomeandretryDispositiondefaults from lines 137-139. Pass the optional values through and let the constructor apply the defaults, so the two default sets cannot diverge.♻️ Proposed simplification
static fromDetails(details: ShellIntegrationErrorDetails, options?: { causeName?: string }): ShellIntegrationError { const code = details.code ?? "SI_ACTIVATION_TIMEOUT" - const commandSubmitted = details.commandSubmitted - const defaultOutcome: TerminalErrorOutcome = commandSubmitted ? "unknown" : "not-started" - const defaultRetry: TerminalErrorRetryDisposition = commandSubmitted ? "never" : "same-terminal-once" - - return new ShellIntegrationError(details.message, commandSubmitted, code, { - phase: details.phase ?? "prepare", - provider: details.provider ?? "vscode", + + return new ShellIntegrationError(details.message, details.commandSubmitted, code, { + phase: details.phase, + provider: details.provider, terminalId: details.terminalId, - outcome: details.outcome ?? defaultOutcome, - retryDisposition: details.retryDisposition ?? defaultRetry, + outcome: details.outcome, + retryDisposition: details.retryDisposition, causeName: options?.causeName ?? details.causeName, }) }🤖 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/types.ts` around lines 155 - 169, Update ShellIntegrationError.fromDetails to stop computing defaultOutcome and defaultRetry; pass details.outcome and details.retryDisposition through unchanged and let the ShellIntegrationError constructor apply its existing defaults, while preserving the remaining field mappings.src/integrations/terminal/shell/TerminalProfileResolver.ts (1)
381-415: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the name-based PowerShell and WSL detection.
resolveWellKnownProfileName(lines 389-412) andresolveProfileEntry(lines 459-489) contain the same win32 name matching and the same hardcoded executable selection.resolveSourceProfile(lines 514-546) repeats the executable selection a third time. The only difference is that theresolveProfileEntrybranches also attachenv: this.sanitizeEnv(entry.env).Extract one helper that takes the profile name and the optional entry env, and call it from all three sites. This keeps the three paths from drifting when the PowerShell path list changes.
🤖 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/shell/TerminalProfileResolver.ts` around lines 381 - 415, The PowerShell and WSL name-resolution logic is duplicated across resolveWellKnownProfileName, resolveProfileEntry, and resolveSourceProfile. Extract a shared helper accepting the profile name and optional entry environment, centralize win32 matching and executable selection there, preserve sanitizeEnv(entry.env) for profile entries, and update all three methods to use the helper.packages/types/src/vscode-extension-host.ts (1)
435-446: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport the shell-family union once and reuse it.
TerminalShellOption.familyrepeats the literal union thatShellFamilydeclares insrc/integrations/terminal/shell/types.ts.src/core/webview/ClineProvider.ts(lines 3093-3180) already needs a cast:env.primaryPlan.family as "powershell" | "cmd" | "posix" | "fish" | "wsl". If a family is added later, the two lists drift and the cast hides the mismatch.Declare the union in
packages/typesand letShellFamilyalias it, so the extension-side type imports from@roo-code/typesand the cast is no longer required.♻️ Proposed direction
+/** Shell family controlling invocation semantics and command chaining. */ +export type TerminalShellFamily = "powershell" | "cmd" | "posix" | "fish" | "wsl" + export interface TerminalShellOption { id: string label: string - /** Shell family controlling invocation semantics and command chaining. */ - family: "powershell" | "cmd" | "posix" | "fish" | "wsl" + family: TerminalShellFamily source: string available: boolean }Then in
src/integrations/terminal/shell/types.ts:import type { TerminalShellFamily } from "`@roo-code/types`" export type ShellFamily = TerminalShellFamily🤖 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 `@packages/types/src/vscode-extension-host.ts` around lines 435 - 446, Declare and export a shared TerminalShellFamily union in the packages/types definitions, then update TerminalShellOption.family to use it. Change integrations/terminal/shell/types.ts so ShellFamily aliases the imported TerminalShellFamily, and update the ClineProvider primaryPlan.family usage to remove the redundant literal-union cast while preserving type safety.webview-ui/src/components/settings/__tests__/SettingsView.shell-selection.spec.tsx (1)
35-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace broad test-double types with precise types.
The new tests use
null as any,anymessage state, and untyped component mocks. These types hide prop-contract regressions in the shell-selection flow.
webview-ui/src/components/settings/__tests__/SettingsView.shell-selection.spec.tsx#L35-L43: type capturedTerminalSettingsprops withoutas any.webview-ui/src/components/settings/__tests__/SettingsView.shell-selection.spec.tsx#L269-L291: define a narrow extension-state fixture type.webview-ui/src/components/settings/__tests__/TerminalSettings.shell.spec.tsx#L19-L71: type message spies and UI mock props with precise test-double interfaces.After the change, run
pnpm --dir src exec eslint --prune-suppressions --max-warnings=0 <relative-file>for each edited file and confirm suppression counts do not increase. As per coding guidelines, “Avoidas any; use typed APIs, bracket notation for private members where appropriate, or precise test doubles andunknownwith type guards.”🤖 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 `@webview-ui/src/components/settings/__tests__/SettingsView.shell-selection.spec.tsx` around lines 35 - 43, Replace broad any-based test doubles with precise interfaces in SettingsView.shell-selection.spec.tsx lines 35-43 by typing captured TerminalSettings props, in lines 269-291 by defining a narrow extension-state fixture type, and in TerminalSettings.shell.spec.tsx lines 19-71 by typing message spies and mocked UI component props. Preserve existing test behavior while removing null as any and untyped mock props; run the specified eslint command for each edited file and ensure suppression counts do not increase.Source: Coding guidelines
src/integrations/terminal/__tests__/TerminalRegistry.spec.ts (2)
587-609: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared
resolvedEnvfixture.Both provider-switch tests build an identical
fallbackPlanandResolvedCommandEnvironment. Extract one factory in thedescribeblock and override only the fields a test needs. This keeps the two tests in sync whenResolvedCommandEnvironmentgains required fields.Also applies to: 642-664
🤖 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__/TerminalRegistry.spec.ts` around lines 587 - 609, The provider-switch tests duplicate the fallbackPlan and ResolvedCommandEnvironment fixtures. Add a shared factory within the describe block, such as around the existing test setup, that returns the common resolved environment and accepts overrides for test-specific fields; update both provider-switch tests to use it while preserving their individual overrides.
157-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
releaseOwnercall afterresetToIdleis a silent no-op.
TerminalLifecycle.resetToIdlealready clears_ownerExecutionId.first.lifecycle.ownerExecutionId!therefore evaluates toundefined, andreleaseOwner(undefined)passes its own guard only becauseundefined !== undefinedis false. The non-null assertion hides that. The setup reads as if it releases a real owner, and it would start throwing ifreleaseOwnerlater rejectedundefined. Release ownership before the reset, or drop the call. The same pattern repeats at Lines 172-173, 187-188, 202-203, 215-216, 264-265, and 282-283.♻️ Proposed fix
- first.lifecycle.resetToIdle() - first.lifecycle.releaseOwner(first.lifecycle.ownerExecutionId!) - first.lifecycle.markHealthy() + first.lifecycle.releaseOwner(first.lifecycle.ownerExecutionId!) + first.lifecycle.resetToIdle() + first.lifecycle.markHealthy()Consider a shared
makeReusable(terminal)helper so all six sites stay consistent.🤖 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__/TerminalRegistry.spec.ts` around lines 157 - 161, Update the repeated terminal setup sequences around TerminalRegistry.getOrCreateTerminal so ownership is released before lifecycle.resetToIdle, or remove the redundant releaseOwner call when resetToIdle is sufficient. Apply the same correction at all listed sites, and consider extracting a shared makeReusable helper to keep the setup consistent without passing the cleared ownerExecutionId.src/integrations/terminal/__tests__/ExecaTerminalProcess.spec.ts (1)
139-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore mutated
process.envvalues and covernullplan env.These tests assign
process.env.EXISTING_VAR,process.env.LANG, andprocess.env.LC_ALLand never restore them. The values persist for every later test in this worker, so the suite is order dependent. Usevi.stubEnvwithvi.unstubAllEnvsin a teardown hook, or save and restore the previous values. Add one case for aplan.enventry set tonull, becauseShellInvocationPlandocumentsnullas "unset variable".♻️ Proposed fix
it("should preserve existing environment variables when plan is provided", async () => { - process.env.EXISTING_VAR = "existing" + vitest.stubEnv("EXISTING_VAR", "existing") terminalProcess = new ExecaTerminalProcess(mockTerminal)it("should override existing LANG and LC_ALL values when plan is provided", async () => { - process.env.LANG = "C" - process.env.LC_ALL = "POSIX" + vitest.stubEnv("LANG", "C") + vitest.stubEnv("LC_ALL", "POSIX") terminalProcess = new ExecaTerminalProcess(mockTerminal)Add the teardown hook in the enclosing
describe:afterEach(() => { vitest.unstubAllEnvs() })🤖 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__/ExecaTerminalProcess.spec.ts` around lines 139 - 161, Update the tests around ExecaTerminalProcess to stub environment variables instead of mutating process.env directly, and add the enclosing describe teardown to call vitest.unstubAllEnvs after each test. Add a case covering a plan.env entry with a null value and assert that the corresponding variable is unset in the Execa options.src/integrations/terminal/__tests__/TerminalLifecycle.spec.ts (1)
414-455: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for
resetToIdleandforceState.The suite covers
resetForReusebut notresetToIdleorforceState. Both are production cleanup paths:BaseTerminal.shellExecutionComplete, the legacybusy/runningsetters,ExecaTerminalProcesscompletion, andTerminalRegistry.recoverStaleTerminalall depend on them. Add cases for the no-op behavior onfailed,disposed, andidle, for clearing ownership and submission state from a pre-idle state, and forforceStatewith and without an owner.🤖 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__/TerminalLifecycle.spec.ts` around lines 414 - 455, Extend the TerminalLifecycle test suite with coverage for resetToIdle and forceState: verify resetToIdle is a no-op for failed, disposed, and idle states, and clears ownership and command-submission state when returning a pre-idle lifecycle to idle. Add forceState cases that validate state changes both without an owner and with an owner, including the expected ownership behavior.src/integrations/terminal/__tests__/ShellInvocationAdapter.spec.ts (1)
1-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport the Vitest globals explicitly.
This file uses
describe,it, andexpectwithout importing them. The sibling suitesrc/integrations/terminal/__tests__/TerminalLifecycle.spec.tsimports them fromvitest. The file compiles only whileglobalsstays enabled in the Vitest config. Add the explicit import for consistency.♻️ Proposed fix
+import { describe, it, expect } from "vitest" + import { ShellInvocationAdapter } from "../shell/ShellInvocationAdapter"🤖 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__/ShellInvocationAdapter.spec.ts` around lines 1 - 6, Update the imports in ShellInvocationAdapter.spec.ts to explicitly import describe, it, and expect from vitest, matching the sibling TerminalLifecycle.spec.ts suite; leave the test behavior unchanged.src/core/tools/__tests__/executeCommandTool.spec.ts (1)
713-801: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicated
cwd parameter validationdescribe block.Lines 621-711 already define a
describe("cwd parameter validation")block with the same four invalid-cwd cases and the same four tests. Lines 713-801 repeat it exactly. The duplicate adds no coverage and doubles the runtime of this section. Delete the second block.🤖 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/core/tools/__tests__/executeCommandTool.spec.ts` around lines 713 - 801, Remove the later duplicated describe("cwd parameter validation") block in the test file, including its repeated invalid-cwd cases and acceptance/terminal-acquisition tests. Preserve the earlier cwd validation block and all unique test coverage.src/core/tools/__tests__/terminal-provider-fallback.spec.ts (1)
116-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese tests assert only on the local
makeEnvhelper.The
same-family fallbackandcross-family rejectionblocks call no production code. They verify the values thatmakeEnvhardcodes at Lines 35-46. The suite name suggests that the resolver produces same-family fallbacks and that mismatches are rejected, but neither behavior is exercised.Assert against the real resolver output, or move these cases to the resolver test suite.
🤖 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/core/tools/__tests__/terminal-provider-fallback.spec.ts` around lines 116 - 137, The fallback tests only validate values hardcoded by makeEnv instead of exercising production behavior. Update the same-family fallback and cross-family rejection cases to invoke the real fallback resolver and assert its returned plans and mismatch handling, or relocate these cases to the resolver test suite; retain the expected PowerShell same-family result and cmd/PowerShell mismatch rejection.src/integrations/terminal/__tests__/CommandScheduler.spec.ts (1)
410-444: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore real timers in a hook, not at the end of each test.
Each test calls
vi.useFakeTimers()andvi.useRealTimers()inline. If an assertion fails,vi.useRealTimers()never runs. Fake timers then leak into the following tests in this file and cause unrelated failures. Move the switch intobeforeEach/afterEachfor this describe block, or callvi.useRealTimers()inafterEach.♻️ Proposed change
afterEach(() => { scheduler.dispose() + vi.useRealTimers() })Also applies to: 446-470, 549-568
🤖 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__/CommandScheduler.spec.ts` around lines 410 - 444, Move fake-timer setup and restoration for the affected CommandScheduler tests into the surrounding describe block’s beforeEach/afterEach hooks, removing each test’s inline vi.useFakeTimers and vi.useRealTimers calls. Ensure afterEach always restores real timers even when assertions fail, including the tests around the cooldown cases and the additional referenced ranges.src/integrations/terminal/Terminal.ts (1)
545-553: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
resolvedExecutableparameter.
resolveShellFamilynever readsresolvedExecutable. The documented priority list also does not use it. Drop the parameter and the argument at Line 109 so the signature matches the behavior.🤖 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/Terminal.ts` around lines 545 - 553, Remove the unused resolvedExecutable parameter from the resolveShellFamily method and remove the corresponding argument at its call site. Preserve the existing shell-family resolution behavior and remaining parameter order.src/integrations/terminal/__tests__/ShellResolver.spec.ts (2)
286-294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the resolution outcome unconditionally.
The only assertion sits inside
if (result.ok). Ifresolvereturns a failure, this test passes without checking anything. Assertresult.okfirst, then assert the source.♻️ Proposed change
const result = resolver.resolve({ terminalProfile: "malicious-workspace-profile", }) - // Should fall through — not resolve the workspace profile - if (result.ok) { - expect(result.shell.source).not.toBe("zooProfile") - } + // Should fall through — not resolve the workspace profile. + expect(result.ok).toBe(true) + if (result.ok) { + expect(result.shell.source).not.toBe("zooProfile") + }🤖 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__/ShellResolver.spec.ts` around lines 286 - 294, Update the test around resolver.resolve for "malicious-workspace-profile" to assert result.ok unconditionally before accessing result.shell.source, then assert that the source is not "zooProfile"; remove the conditional guard so failures cannot pass silently.
39-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the untyped test doubles with typed ones, or explain the assertions.
entry: any,source: any,as unknown as TerminalProfileResolver(Line 58), andsettings as any(Line 182) drop type checking in the test. UseShellResolutionSourceforsource,ShellResolverSettingsforsettings, and aPartial<TerminalProfileResolver>typed double. If a double assertion stays necessary, add a comment that states the reason.As per coding guidelines: "Avoid
as any; use typed APIs, bracket notation for private members where appropriate, or precise test doubles andunknownwith type guards. Use double assertions only as a last resort and explain them with a comment."Also applies to: 182-182
🤖 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__/ShellResolver.spec.ts` around lines 39 - 59, Replace the untyped test doubles in createProfileResolverMock with ShellResolutionSource for source and a typed entry shape, and construct the mock as Partial<TerminalProfileResolver> before satisfying the resolver type; if a double assertion remains, add a comment explaining its necessity. Update the settings as any usage near the referenced test to use ShellResolverSettings directly, avoiding any and preserving type checking.Source: Coding guidelines
src/integrations/terminal/__tests__/TerminalProfile.spec.ts (1)
592-592: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnchor both alternatives in the regex.
/pwsh\.exe|powershell\.exe$/ianchors only the second alternative. The first alternative matchespwsh.exeanywhere in the path. Group the alternation so both ends are anchored.♻️ Proposed change
- expect(result?.shellPath).toMatch(/pwsh\.exe|powershell\.exe$/i) + expect(result?.shellPath).toMatch(/(?:pwsh|powershell)\.exe$/i)🤖 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__/TerminalProfile.spec.ts` at line 592, Update the shellPath assertion in the TerminalProfile test to group the pwsh.exe and powershell.exe alternatives under a single end anchor, ensuring the match ends with either executable name rather than allowing pwsh.exe anywhere in the path.src/integrations/terminal/TerminalProcess.ts (1)
74-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog the swallowed transition error.
The
catchblock discards the error fromlifecycle.transition("failed"). If the transition table rejects the current state, the failure becomes invisible during diagnosis. Log the error atwarnlevel, and pass theexecutionIdwhen it is available so the lifecycle records the owner.🤖 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/TerminalProcess.ts` around lines 74 - 82, Update the catch block around lifecycle.transition("failed") in the TerminalProcess failure handling to capture the transition error and log it at warn level. Include the available executionId in the lifecycle warning context, while preserving the existing behavior of ignoring the transition failure and setting lastError to SI_NEVER_AVAILABLE.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c96df25-09f2-43a7-bdcd-ed083f15f16b
⛔ Files ignored due to path filters (7)
src/core/prompts/__tests__/__snapshots__/add-custom-instructions/architect-mode-prompt.snapis excluded by!**/*.snapsrc/core/prompts/__tests__/__snapshots__/add-custom-instructions/ask-mode-prompt.snapis excluded by!**/*.snapsrc/core/prompts/__tests__/__snapshots__/add-custom-instructions/mcp-server-creation-disabled.snapis excluded by!**/*.snapsrc/core/prompts/__tests__/__snapshots__/add-custom-instructions/no-mcp-servers.snapis excluded by!**/*.snapsrc/core/prompts/__tests__/__snapshots__/system-prompt/consistent-system-prompt.snapis excluded by!**/*.snapsrc/core/prompts/__tests__/__snapshots__/system-prompt/with-mcp-hub-provided.snapis excluded by!**/*.snapsrc/core/prompts/__tests__/__snapshots__/system-prompt/with-undefined-mcp-hub.snapis excluded by!**/*.snap
📒 Files selected for processing (74)
apps/vscode-e2e/src/suite/tools/terminal-profile.test.tspackages/types/src/__tests__/terminal-shell-settings.spec.tspackages/types/src/global-settings.tspackages/types/src/terminal.tspackages/types/src/vscode-extension-host.tssrc/core/prompts/__tests__/shell-environment-prompt.spec.tssrc/core/prompts/sections/rules.tssrc/core/prompts/sections/system-info.tssrc/core/prompts/system.tssrc/core/prompts/tools/native-tools/execute_command.tssrc/core/prompts/tools/native-tools/index.tssrc/core/task/Task.tssrc/core/task/build-tools.tssrc/core/tools/ExecuteCommandTool.tssrc/core/tools/__tests__/executeCommand.spec.tssrc/core/tools/__tests__/executeCommandTool.spec.tssrc/core/tools/__tests__/terminal-provider-fallback.spec.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/terminal-shell-messages.spec.tssrc/core/webview/generateSystemPrompt.tssrc/core/webview/webviewMessageHandler.tssrc/eslint-suppressions.jsonsrc/extension.tssrc/extension/api.tssrc/integrations/terminal/BaseTerminal.tssrc/integrations/terminal/CommandScheduler.tssrc/integrations/terminal/CommandTrace.tssrc/integrations/terminal/ExecaTerminal.tssrc/integrations/terminal/ExecaTerminalProcess.tssrc/integrations/terminal/Terminal.tssrc/integrations/terminal/TerminalLifecycle.tssrc/integrations/terminal/TerminalProcess.tssrc/integrations/terminal/TerminalRegistry.tssrc/integrations/terminal/__tests__/CommandScheduler.spec.tssrc/integrations/terminal/__tests__/ExecaTerminalProcess.spec.tssrc/integrations/terminal/__tests__/ShellInvocationAdapter.spec.tssrc/integrations/terminal/__tests__/ShellResolver.spec.tssrc/integrations/terminal/__tests__/TerminalLifecycle.spec.tssrc/integrations/terminal/__tests__/TerminalProcess.spec.tssrc/integrations/terminal/__tests__/TerminalProcessExec.bash.spec.tssrc/integrations/terminal/__tests__/TerminalProcessExec.cmd.spec.tssrc/integrations/terminal/__tests__/TerminalProcessExec.pwsh.spec.tssrc/integrations/terminal/__tests__/TerminalProfile.spec.tssrc/integrations/terminal/__tests__/TerminalRegistry.spec.tssrc/integrations/terminal/shell/CommandEnvironmentService.tssrc/integrations/terminal/shell/ShellInvocationAdapter.tssrc/integrations/terminal/shell/ShellResolver.tssrc/integrations/terminal/shell/TerminalProfileResolver.tssrc/integrations/terminal/shell/types.tssrc/integrations/terminal/types.tssrc/utils/__tests__/shell.spec.tssrc/utils/shell.tswebview-ui/src/components/settings/SettingsView.tsxwebview-ui/src/components/settings/TerminalSettings.tsxwebview-ui/src/components/settings/__tests__/SettingsView.shell-selection.spec.tsxwebview-ui/src/components/settings/__tests__/TerminalSettings.shell.spec.tsxwebview-ui/src/i18n/locales/ca/settings.jsonwebview-ui/src/i18n/locales/de/settings.jsonwebview-ui/src/i18n/locales/en/settings.jsonwebview-ui/src/i18n/locales/es/settings.jsonwebview-ui/src/i18n/locales/fr/settings.jsonwebview-ui/src/i18n/locales/hi/settings.jsonwebview-ui/src/i18n/locales/id/settings.jsonwebview-ui/src/i18n/locales/it/settings.jsonwebview-ui/src/i18n/locales/ja/settings.jsonwebview-ui/src/i18n/locales/ko/settings.jsonwebview-ui/src/i18n/locales/nl/settings.jsonwebview-ui/src/i18n/locales/pl/settings.jsonwebview-ui/src/i18n/locales/pt-BR/settings.jsonwebview-ui/src/i18n/locales/ru/settings.jsonwebview-ui/src/i18n/locales/tr/settings.jsonwebview-ui/src/i18n/locales/vi/settings.jsonwebview-ui/src/i18n/locales/zh-CN/settings.jsonwebview-ui/src/i18n/locales/zh-TW/settings.json
| const execTool = tools.find((t) => (t as any).function?.name === "execute_command") | ||
| expect(execTool).toBeDefined() | ||
| const desc = (execTool as any).function.description |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
pnpm --dir src exec eslint --prune-suppressions --max-warnings=0 core/prompts/__tests__/shell-environment-prompt.spec.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 1595
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the target test range plus nearby context.
sed -n '230,310p' src/core/prompts/__tests__/shell-environment-prompt.spec.ts | cat -n
printf '\n--- TypeScript references to execTool/function/name/description ---\n'
rg -n "execTool|function\\?\\.|function\\.|execute_command|ChatCompletionFunctionTool|ChatCompletionTool" src/core/prompts/__tests__/shell-environment-prompt.spec.ts
printf '\n--- OpenAI type/package availability ---\n'
node - <<'JS'
try {
const { OpenAI } = require('openai');
console.log('openai module available, version', require('openai/package.json').version);
} catch (e) {
console.log('openai module unavailable:', e.message);
}
try {
const content = require('fs').readFileSync('node_modules/openai/index.d.ts', 'utf8');
const matched = content.match(/interface\s+ChatCompletionFunctionTool[\s\S]{0,800}/);
console.log('openai type definitions loaded; ChatCompletionFunctionTool present:', !!matched);
if (matched) console.log(matched[0].substring(0, 800));
} catch (e) {
console.log('openai index.d.ts unavailable:', e.message);
}
JS
printf '\n--- tsconfig type settings ---\n'
sed -n '1,220p' src/tsconfig.json 2>/dev/null || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 6287
Remove as any from the native-tool tests.
execTool is a ChatCompletionTool union, so lines 274 and 282 suppress type checking. Narrow function tools with a type predicate, guard execTool, then access execTool.function.description directly for lines 276 and 284.
🤖 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/core/prompts/__tests__/shell-environment-prompt.spec.ts` around lines 274
- 276, Update the native-tool tests around the execTool lookups to remove both
as any casts: narrow the ChatCompletionTool union with a type predicate for
function tools, guard that execTool was found, then access
execTool.function.description directly while preserving the existing assertions.
Source: Coding guidelines
| private async getSystemPrompt(): Promise<string> { | ||
| // Resolve the command environment for this request so the system prompt | ||
| // uses the same shell info that runtime execution will use. | ||
| await this.resolveCommandEnvironment() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Forward resolvedCommandEnvironment to SYSTEM_PROMPT.
Line 3816 resolves the environment, but the SYSTEM_PROMPT call ends at Line 3889 without its final resolvedEnv argument. getRulesSection and getSystemInfoSection then use the legacy shell fallback, while tool construction uses this.resolvedCommandEnvironment. This can give the model shell guidance that differs from runtime execution.
Proposed fix
this.api.getModel().id,
provider.getSkillsManager(),
+ this.resolvedCommandEnvironment,
)Based on the supplied prompt-wiring context, the webview preview forwards resolvedEnv, but this task path does not.
🤖 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/core/task/Task.ts` around lines 3813 - 3816, Update getSystemPrompt and
its SYSTEM_PROMPT invocation to pass this.resolvedCommandEnvironment as the
final resolvedEnv argument, matching the webview preview wiring. Preserve the
existing prompt construction while ensuring getRulesSection and
getSystemInfoSection receive the same environment used by tool construction.
| export class ShellFallbackMismatchError extends Error { | ||
| readonly code = "SHELL_FALLBACK_MISMATCH" as const | ||
| readonly primaryFamily: string | ||
| readonly fallbackFamily: string | undefined | ||
|
|
||
| constructor(primaryFamily: string, fallbackFamily: string | undefined) { | ||
| super( | ||
| `SHELL_FALLBACK_MISMATCH: Primary shell family "${primaryFamily}" has no compatible fallback` + | ||
| (fallbackFamily ? ` (fallback family: "${fallbackFamily}")` : " (no fallback plan available)") + | ||
| ". Command was not executed.", | ||
| ) | ||
| this.name = "ShellFallbackMismatchError" | ||
| this.primaryFamily = primaryFamily | ||
| this.fallbackFamily = fallbackFamily | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The fallback path never checks shell-family compatibility.
ShellFallbackMismatchError states that a command must not be retried under a different shell family. This file exports the class, and terminal-provider-fallback.spec.ts shows an environment where primaryPlan.family is cmd and fallbackPlan.family is powershell. The fallback branch at Line 318 sets useFallbackPlan: !!resolvedEnv without comparing the two families, and no code path throws ShellFallbackMismatchError. When the families differ, the command text produced for the primary shell syntax runs under a different shell. This can change command semantics.
Compare resolvedEnv.primaryPlan.family with resolvedEnv.fallbackPlan?.family before the retry, and report ShellFallbackMismatchError instead of replaying the command.
🐛 Proposed guard
if (error.retryDisposition === "fallback-safe" && !error.commandSubmitted) {
const terminalId = typeof error.terminalId === "number" ? error.terminalId : undefined
+
+ if (resolvedEnv && resolvedEnv.fallbackPlan?.family !== resolvedEnv.primaryPlan.family) {
+ const mismatch = new ShellFallbackMismatchError(
+ resolvedEnv.primaryPlan.family,
+ resolvedEnv.fallbackPlan?.family,
+ )
+ pushToolResult(formatResponse.toolError(mismatch.message))
+ return
+ }Also applies to: 297-324
🤖 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/core/tools/ExecuteCommandTool.ts` around lines 54 - 69, Update the
fallback retry branch around the logic that sets useFallbackPlan to compare
resolvedEnv.primaryPlan.family with resolvedEnv.fallbackPlan?.family before
replaying the command. When the families differ, stop the retry and report
ShellFallbackMismatchError using the primary and fallback family values; only
set useFallbackPlan when the shell families are compatible.
| // When a resolved environment is available, use its primary plan provider. | ||
| // This ensures the execution provider matches what the system prompt told the model. | ||
| if (env) { | ||
| const terminalProvider = env.primaryPlan.provider | ||
| const isCmdExeFallback = terminalProvider === "execa" && env.primaryPlan.family === "cmd" | ||
| return { terminalProvider, isCmdExeFallback } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the resolver accounts for terminalShellIntegrationDisabled when choosing primaryPlan.provider.
fd -t f 'ShellResolver.ts|CommandEnvironmentService.ts|ShellInvocationAdapter.ts' src -x rg -n -C 6 'terminalShellIntegrationDisabled|provider\s*[:=]\s*"(vscode|execa)"' {}Repository: Zoo-Code-Org/Zoo-Code
Length of output: 1678
🏁 Script executed:
#!/bin/bash
set -u
echo "== candidate files =="
git ls-files | rg 'ShellResolver|CommandEnvironmentService|ShellInvocationAdapter|ExecuteCommandTool|CommandEnvironment' || true
echo "== occurrences terminalShellIntegrationDisabled ========="
rg -n 'terminalShellIntegrationDisabled' src || true
echo "== ExecuteCommandTool relevant section =="
sed -n '1,180p' src/core/tools/ExecuteCommandTool.ts
echo "== command environment section relevant occurrences =="
rg -n -C 8 'terminalShellIntegrationDisabled|primaryProvider|createPrimaryPlan|provider|execa|vscode' src/core/tools src/core -g '!**/*.test.*' -g '!**/*.spec.*' | head -n 240Repository: Zoo-Code-Org/Zoo-Code
Length of output: 28323
🏁 Script executed:
#!/bin/bash
set -u
echo "== ShellResolver.ts outline =="
ast-grep outline src/integrations/terminal/shell/ShellResolver.ts --view expanded || true
echo "== ShellResolver.ts relevant sections =="
wc -l src/integrations/terminal/shell/ShellResolver.ts src/integrations/terminal/shell/CommandEnvironmentService.ts src/integrations/terminal/shell/types.ts
sed -n '1,260p' src/integrations/terminal/shell/ShellResolver.ts
sed -n '1,240p' src/integrations/terminal/shell/types.ts
sed -n '1,240p' src/integrations/terminal/shell/ShellInvocationAdapter.ts
echo "== exact provider/plan creation snippets =="
rg -n -C 10 'createPlan|primaryProvider|provider' src/integrations/terminal/shell/ShellResolver.ts src/integrations/terminal/shell/CommandEnvironmentService.ts src/integrations/terminal/shell/types.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 32459
Keep the resolved command environment in sync with terminalShellIntegrationDisabled.
CommandEnvironmentService.createResolvedEnvironment() currently sets primaryProvider = primaryShell.family === "cmd" ? "execa" : "vscode" and ignores the setting. Since getTerminalProviderForExecution() uses env.primaryPlan.provider, an enabled terminalShellIntegrationDisabled setting can still result in vscode execution when the resolved shell is bash, zsh, or another non-cmd shell. Force provider = "execa" for the primary plan when shell integration is disabled, and make the resolved env propagate that setting.
🤖 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/core/tools/ExecuteCommandTool.ts` around lines 101 - 107, Update
CommandEnvironmentService.createResolvedEnvironment() so the resolved primary
plan uses provider "execa" whenever terminalShellIntegrationDisabled is enabled,
including non-cmd shells; otherwise preserve the existing cmd-to-execa and
other-shell-to-vscode mapping. Ensure the resolved environment also propagates
terminalShellIntegrationDisabled, so getTerminalProviderForExecution() and
env.primaryPlan.provider remain consistent.
| const closeIdleTerminalsMock = vi.fn() | ||
| vi.mock("../../../integrations/terminal/TerminalRegistry", () => ({ | ||
| TerminalRegistry: { | ||
| closeIdleTerminals: (...args: any[]) => closeIdleTerminalsMock(...args), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file stats =="
wc -l src/core/webview/__tests__/terminal-shell-messages.spec.ts src/eslint-suppressions.json 2>/dev/null || true
echo "== target file outline =="
ast-grep outline src/core/webview/__tests__/terminal-shell-messages.spec.ts --view expanded 2>/dev/null || true
echo "== relevant any/unknown/message occurrences =="
rg -n "\bany\b|as\s+unknown|as\s+WebviewMessage|interface .*Message|type .*Message|sendMessage|closeIdleTerminals|createProvider|provider" src/core/webview/__tests__/terminal-shell-messages.spec.ts || true
echo "== eslint suppression snapshot =="
if [ -f src/eslint-suppressions.json ]; then
python3 - <<'PY'
import json
p='src/eslint-suppressions.json'
data=json.load(open(p, encoding='utf-8'))
print(json.dumps(data, indent=2)[:4000])
PY
fi
echo "== diff against original branch if available =="
git diff -- src/core/webview/__tests__/terminal-shell-messages.spec.ts | sed -n '1,220p' || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 5739
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file stats =="
wc -l src/core/webview/__tests__/terminal-shell-messages.spec.ts src/eslint-suppressions.json 2>/dev/null || true
echo "== target file outline =="
ast-grep outline src/core/webview/__tests__/terminal-shell-messages.spec.ts --view expanded 2>/dev/null || true
echo "== relevant any/unknown/message occurrences =="
rg -n "\bany\b|as\s+unknown|as\s+WebviewMessage|interface .*Message|type .*Message|sendMessage|closeIdleTerminals|createProvider|provider" src/core/webview/__tests__/terminal-shell-messages.spec.ts || true
echo "== eslint suppression snapshot =="
if [ -f src/eslint-suppressions.json ]; then
python3 - <<'PY'
import json
p='src/eslint-suppressions.json'
data=json.load(open(p, encoding='utf-8'))
print(json.dumps(data, indent=2, sort_keys=True)[:5000])
PY
fi
echo "== target lines with context =="
sed -n '50,160p;200,275p;320,345p;360,410p;418,455p' src/core/webview/__tests__/terminal-shell-messages.spec.ts
echo "== lint script availability =="
if [ -f package.json ]; then
jq '.scripts' package.json
fiRepository: Zoo-Code-Org/Zoo-Code
Length of output: 17725
Remove explicit any from the terminal-shell messaging tests.
Use typed mocks/fixtures for the provider and terminal registry/profile resolver callbacks, and type message objects directly instead of as any. Define malformed payloads as unknown with a type guard instead.
Also applies to: 78-99, 142, 210-263, 329-331, 370-385, 400-414, 422-425, 446-463.
🤖 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/core/webview/__tests__/terminal-shell-messages.spec.ts` at line 69,
Replace explicit any usage throughout terminal-shell-messages.spec.ts with typed
mocks and fixtures, including callbacks such as closeIdleTerminals and the
provider, terminal registry, and profile resolver interactions. Type message
objects directly, and represent malformed payloads as unknown validated through
a type guard rather than using as any, covering the referenced test sections
while preserving their behavior.
Source: Coding guidelines
| public waitForShellIntegration(timeoutMs: number, executionId?: string, abortSignal?: AbortSignal): Promise<void> { | ||
| if (this.terminal.shellIntegration) { | ||
| // A reused terminal may already be in `integration-ready` (promoted by the | ||
| // registry during reservation) while shellIntegration is still defined. | ||
| // `integration-ready → integration-ready` is not a legal self-transition, | ||
| // so only promote when not already ready. | ||
| if (this.lifecycle.state !== "integration-ready") { | ||
| this.lifecycle.transition("integration-ready", executionId) | ||
| } | ||
| this.lifecycle.markHealthy() | ||
| return Promise.resolve() | ||
| } | ||
|
|
||
| // Only move to `integration-pending` from a state where that transition is | ||
| // legal. From `integration-ready`/`fallback-ready` the forward table does | ||
| // not allow `→ integration-pending`; in that case leave the state as-is and | ||
| // rely on the readiness event (or timeout) to drive the next transition. | ||
| if (this.lifecycle.state !== "integration-ready" && this.lifecycle.state !== "fallback-ready") { | ||
| this.lifecycle.transition("integration-pending", executionId) | ||
| } | ||
| this.shellIntegrationAbortController = new AbortController() | ||
| const abortController = this.shellIntegrationAbortController | ||
|
|
||
| if (abortSignal) { | ||
| abortSignal.addEventListener("abort", () => abortController.abort(), { once: true }) | ||
| } | ||
|
|
||
| return new Promise<void>((resolve, reject) => { | ||
| const onAbort = () => { | ||
| clearTimeout(timer) | ||
| ref.disposable?.dispose() | ||
| const err = new Error("Shell integration wait cancelled") | ||
| err.name = "AbortError" | ||
| reject(err) | ||
| } | ||
|
|
||
| if (abortController.signal.aborted) { | ||
| onAbort() | ||
| return | ||
| } | ||
|
|
||
| abortController.signal.addEventListener("abort", onAbort, { once: true }) | ||
|
|
||
| const ref = { disposable: null as vscode.Disposable | null } | ||
| const timer = setTimeout(() => { | ||
| ref.disposable?.dispose() | ||
| abortController.signal.removeEventListener("abort", onAbort) | ||
| reject(new Error(`Shell integration did not activate within ${timeoutMs / 1000}s`)) | ||
| }, timeoutMs) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Handle an already-aborted abortSignal, and move onAbort after timer/ref.
Two defects exist in waitForShellIntegration:
- Line 308 adds an
abortlistener toabortSignal. If the caller passes a signal that is already aborted, the listener never fires. The internalabortControllerthen stays unaborted, so the wait runs untiltimeoutMsand rejects with a timeout error instead of anAbortError. The caller inrunCommand(Line 249) then emitsSI_ACTIVATION_TIMEOUTfor a cancelled wait. onAbortreadstimerandref, which areconstbindings declared after theif (abortController.signal.aborted) { onAbort(); return }check on Line 320. If that branch ever runs,onAbortthrows aReferenceErrorfrom the temporal dead zone instead of rejecting withAbortError.
Also clear shellIntegrationAbortController when the wait settles. Otherwise cancelShellIntegrationWait() aborts a controller that belongs to a wait that already finished.
🐛 Proposed fix
this.shellIntegrationAbortController = new AbortController()
const abortController = this.shellIntegrationAbortController
if (abortSignal) {
- abortSignal.addEventListener("abort", () => abortController.abort(), { once: true })
+ if (abortSignal.aborted) {
+ abortController.abort()
+ } else {
+ abortSignal.addEventListener("abort", () => abortController.abort(), { once: true })
+ }
}
return new Promise<void>((resolve, reject) => {
+ const ref = { disposable: null as vscode.Disposable | null }
+ let timer: NodeJS.Timeout | undefined
+
const onAbort = () => {
clearTimeout(timer)
ref.disposable?.dispose()
+ this.shellIntegrationAbortController = undefined
const err = new Error("Shell integration wait cancelled")
err.name = "AbortError"
reject(err)
}
if (abortController.signal.aborted) {
onAbort()
return
}
abortController.signal.addEventListener("abort", onAbort, { once: true })
- const ref = { disposable: null as vscode.Disposable | null }
- const timer = setTimeout(() => {
+ timer = setTimeout(() => {
ref.disposable?.dispose()
abortController.signal.removeEventListener("abort", onAbort)
+ this.shellIntegrationAbortController = undefined
reject(new Error(`Shell integration did not activate within ${timeoutMs / 1000}s`))
}, timeoutMs)🤖 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/Terminal.ts` around lines 284 - 332, Update
waitForShellIntegration so an already-aborted abortSignal immediately aborts the
internal controller and rejects with AbortError. In the Promise setup, declare
ref and timer before onAbort so its cleanup references are initialized before
any immediate abort path. Clear shellIntegrationAbortController whenever the
wait settles, including abort, timeout, readiness, and other completion paths,
so cancelShellIntegrationWait cannot affect a completed wait.
| resetToIdle(): void { | ||
| if (this._state === "disposed" || this._state === "failed" || this._state === "idle") { | ||
| return | ||
| } | ||
| this._state = "idle" | ||
| this._stateChangedAt = this._now() | ||
| this._ownerExecutionId = undefined | ||
| this._commandSubmittedAt = undefined | ||
| this._recoveryAttempts = 0 | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show every resetToIdle and legacy busy=false call site with context.
rg -nP --type=ts -C 6 '\bresetToIdle\s*\(|\bbusy\s*=\s*false' srcRepository: Zoo-Code-Org/Zoo-Code
Length of output: 25906
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== TerminalLifecycle relevant methods =="
sed -n '460,525p' src/integrations/terminal/TerminalLifecycle.ts
echo
echo "== TerminalRegistry end-event block =="
sed -n '178,215p' src/integrations/terminal/TerminalRegistry.ts
echo
echo "== TerminalRegistry reusable terminal path around 740-770 =="
sed -n '738,768p' src/integrations/terminal/TerminalRegistry.ts
echo
echo "== BaseTerminal busy setter and shellExecutionComplete =="
sed -n '40,165p' src/integrations/terminal/BaseTerminal.ts
echo
echo "== TerminalRegistry process reservation paths =="
rg -n --type=ts -C 4 'activeShellExecution|process\.executionId|executionId|busy = true|busy = false' src/integrations/terminal/TerminalRegistry.ts src/integrations/terminal/Terminal.ts src/integrations/terminal/BaseTerminal.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 33893
Guard resetToIdle against ownership changes.
resetToIdle always clears _ownerExecutionId, _commandSubmittedAt, and _recoveryAttempts from non-disposed/non-failed/non-idle states. Reused terminals are acquired before the endpoint is ready, so a stale end event path such as terminal.busy = false can drop the currently reserved execution’s ownership after a later command. Add an optional executionId parameter and return when the caller is not the current owner.
🛡️ Proposed guard
- resetToIdle(): void {
+ resetToIdle(executionId?: string): void {
+ if (
+ executionId !== undefined &&
+ this._ownerExecutionId !== undefined &&
+ this._ownerExecutionId !== executionId
+ ) {
+ return
+ }
if (this._state === "disposed" || this._state === "failed" || this._state === "idle") {
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resetToIdle(): void { | |
| if (this._state === "disposed" || this._state === "failed" || this._state === "idle") { | |
| return | |
| } | |
| this._state = "idle" | |
| this._stateChangedAt = this._now() | |
| this._ownerExecutionId = undefined | |
| this._commandSubmittedAt = undefined | |
| this._recoveryAttempts = 0 | |
| } | |
| resetToIdle(executionId?: string): void { | |
| if ( | |
| executionId !== undefined && | |
| this._ownerExecutionId !== undefined && | |
| this._ownerExecutionId !== executionId | |
| ) { | |
| return | |
| } | |
| if (this._state === "disposed" || this._state === "failed" || this._state === "idle") { | |
| return | |
| } | |
| this._state = "idle" | |
| this._stateChangedAt = this._now() | |
| this._ownerExecutionId = undefined | |
| this._commandSubmittedAt = undefined | |
| this._recoveryAttempts = 0 | |
| } |
🤖 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/TerminalLifecycle.ts` around lines 511 - 520,
Update TerminalLifecycle.resetToIdle to accept an optional executionId and,
before changing state or clearing ownership fields, return when a provided
caller ID does not match the current _ownerExecutionId. Preserve existing
behavior for callers without an ID and for the current owner, including
resetting state, timestamps, and recovery counters.
| private static runWatchdog(): void { | ||
| const now = Date.now() | ||
| const shellIntegrationTimeout = Terminal.getShellIntegrationTimeout() | ||
|
|
||
| // Iterate over the raw terminals array so the watchdog can see closed | ||
| // terminals and recover them before getAllTerminals() filters them out. | ||
| for (const terminal of [...this.terminals]) { | ||
| const lifecycle = terminal.lifecycle | ||
| const ownerExecutionId = lifecycle.ownerExecutionId | ||
| if (ownerExecutionId === undefined) { | ||
| continue | ||
| } | ||
|
|
||
| const state = lifecycle.state | ||
| const process = terminal.process | ||
| const terminalClosed = terminal.isClosed() | ||
|
|
||
| // Evidence 1: terminal closed while owned. | ||
| if (terminalClosed) { | ||
| console.info( | ||
| `[TerminalRegistry/watchdog] Terminal ${terminal.id} closed while owned by ${ownerExecutionId}; recovering`, | ||
| ) | ||
| this.recoverStaleTerminal(terminal.id, ownerExecutionId, "TERMINAL_DISPOSED") | ||
| continue | ||
| } | ||
|
|
||
| // Evidence 2: attached process belongs to a different execution. | ||
| if ( | ||
| process && | ||
| "executionId" in process && | ||
| process.executionId !== undefined && | ||
| process.executionId !== ownerExecutionId | ||
| ) { | ||
| console.info( | ||
| `[TerminalRegistry/watchdog] Terminal ${terminal.id} process belongs to ${process.executionId} but owner is ${ownerExecutionId}; recovering`, | ||
| ) | ||
| this.recoverStaleTerminal(terminal.id, ownerExecutionId, "TERMINAL_BUSY_STALE") | ||
| continue | ||
| } | ||
|
|
||
| // Evidence 3: pre-submission states exceeded their deadline. | ||
| const elapsed = now - lifecycle.stateChangedAt | ||
| const preSubmissionDeadline = shellIntegrationTimeout + 1_000 | ||
|
|
||
| if (state === "creating" || state === "process-started" || state === "integration-pending") { | ||
| if (elapsed > preSubmissionDeadline) { | ||
| console.info( | ||
| `[TerminalRegistry/watchdog] Terminal ${terminal.id} pre-submission state ${state} exceeded deadline (${elapsed}ms); recovering`, | ||
| ) | ||
| this.recoverStaleTerminal(terminal.id, ownerExecutionId, "TERMINAL_BUSY_STALE") | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| if (state === "integration-ready" || state === "fallback-ready") { | ||
| if (elapsed > READY_RESERVATION_DEADLINE_MS) { | ||
| console.info( | ||
| `[TerminalRegistry/watchdog] Terminal ${terminal.id} ready reservation exceeded ${READY_RESERVATION_DEADLINE_MS}ms; recovering`, | ||
| ) | ||
| this.recoverStaleTerminal(terminal.id, ownerExecutionId, "TERMINAL_BUSY_STALE") | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| // Evidence 4: owned but no process in a state that requires one. | ||
| if (state === "running" && !process) { | ||
| console.info( | ||
| `[TerminalRegistry/watchdog] Terminal ${terminal.id} is running but has no process; recovering`, | ||
| ) | ||
| this.recoverStaleTerminal(terminal.id, ownerExecutionId, "TERMINAL_BUSY_STALE") | ||
| continue | ||
| } | ||
|
|
||
| // Running with a matching process is intentionally NOT reset by time. | ||
| if (state === "running") { | ||
| if (elapsed > 10_000) { | ||
| console.info( | ||
| `[TerminalRegistry/watchdog] Terminal ${terminal.id} has been running for ${elapsed}ms with a matching process; diagnostic only`, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
The watchdog never reaps an owned terminal in idle or failed.
runWatchdog handles creating, process-started, integration-pending, integration-ready, fallback-ready, and running. It has no branch for idle or failed while ownerExecutionId is set. Two reachable paths leave a terminal in exactly that shape:
TerminalLifecycle.resetToIdlereturns early forfailed, so it does not clear ownership. In the Execa branch ofrecoverStaleTerminal(Line 763), afailedterminal keeps its owner.- The same Execa branch performs no reset at all when a process is attached or when the process has no
executionId, so the owner remains after recovery.
An owned terminal in these states fails canReuse forever and the watchdog ignores it, so it leaks for the lifetime of the extension host. Add an evidence branch for an owned terminal in idle or failed, and release ownership in the Execa recovery path.
Also applies to: 761-767
🤖 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/TerminalRegistry.ts` around lines 594 - 676, Update
runWatchdog to detect owned terminals whose lifecycle state is idle or failed
and recover them through recoverStaleTerminal, preserving the existing recovery
reason and logging pattern. In the Execa recovery path of recoverStaleTerminal,
ensure ownership is cleared/reset even when the terminal is failed, has an
attached process, or its process lacks an executionId, so recovery cannot leave
the terminal permanently unreusable.
| if (commandSubmitted) { | ||
| return { | ||
| terminal: this.getTerminalById(terminalId)!, | ||
| provider: fromProvider, | ||
| } | ||
| } | ||
| if (!resolvedEnv.fallbackPlan) { | ||
| throw new Error("TERMINAL/PROVIDER_SWITCH/003: fallback plan is required") | ||
| } | ||
|
|
||
| const source = this.getTerminalById(terminalId) | ||
| if (!source) { | ||
| throw new Error(`TERMINAL/PROVIDER_SWITCH/004: source terminal ${terminalId} not found`) | ||
| } | ||
| if (source.provider !== "vscode") { | ||
| throw new Error("TERMINAL/PROVIDER_SWITCH/005: source terminal is not a VS Code terminal") | ||
| } | ||
| if (source.lifecycle.ownerExecutionId !== executionId) { | ||
| throw new Error( | ||
| `TERMINAL/PROVIDER_SWITCH/006: owner mismatch (expected ${executionId}, got ${source.lifecycle.ownerExecutionId})`, | ||
| ) | ||
| } | ||
|
|
||
| // 1. Transition source to failed. | ||
| source.lifecycle.transition("failed", executionId) | ||
|
|
||
| // 2. Cancel shell-integration wait. | ||
| ;(source as Terminal).cancelShellIntegrationWait() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate the source terminal before the non-null assertion and the cast.
Two unchecked assumptions exist in this precondition block:
- Line 791 uses
this.getTerminalById(terminalId)!.getTerminalByIdreturnsundefinedwhen the terminal is closed or absent, soresult.terminalcan beundefinedwhile the return type claimsRooTerminal. The caller then dereferencesundefined. - Line 816 casts with
(source as Terminal)and callscancelShellIntegrationWait(). Every other step in this method usesinstanceof Terminal. ARooTerminalthat reportsprovider === "vscode"but is not aTerminalinstance throws aTypeErrorhere.
🛡️ Proposed fix
if (commandSubmitted) {
- return {
- terminal: this.getTerminalById(terminalId)!,
- provider: fromProvider,
- }
+ const current = this.getTerminalById(terminalId)
+ if (!current) {
+ throw new Error(`TERMINAL/PROVIDER_SWITCH/007: source terminal ${terminalId} not found`)
+ }
+ return { terminal: current, provider: fromProvider }
} // 2. Cancel shell-integration wait.
- ;(source as Terminal).cancelShellIntegrationWait()
+ if (source instanceof Terminal) {
+ source.cancelShellIntegrationWait()
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (commandSubmitted) { | |
| return { | |
| terminal: this.getTerminalById(terminalId)!, | |
| provider: fromProvider, | |
| } | |
| } | |
| if (!resolvedEnv.fallbackPlan) { | |
| throw new Error("TERMINAL/PROVIDER_SWITCH/003: fallback plan is required") | |
| } | |
| const source = this.getTerminalById(terminalId) | |
| if (!source) { | |
| throw new Error(`TERMINAL/PROVIDER_SWITCH/004: source terminal ${terminalId} not found`) | |
| } | |
| if (source.provider !== "vscode") { | |
| throw new Error("TERMINAL/PROVIDER_SWITCH/005: source terminal is not a VS Code terminal") | |
| } | |
| if (source.lifecycle.ownerExecutionId !== executionId) { | |
| throw new Error( | |
| `TERMINAL/PROVIDER_SWITCH/006: owner mismatch (expected ${executionId}, got ${source.lifecycle.ownerExecutionId})`, | |
| ) | |
| } | |
| // 1. Transition source to failed. | |
| source.lifecycle.transition("failed", executionId) | |
| // 2. Cancel shell-integration wait. | |
| ;(source as Terminal).cancelShellIntegrationWait() | |
| if (commandSubmitted) { | |
| const current = this.getTerminalById(terminalId) | |
| if (!current) { | |
| throw new Error(`TERMINAL/PROVIDER_SWITCH/007: source terminal ${terminalId} not found`) | |
| } | |
| return { terminal: current, provider: fromProvider } | |
| } | |
| if (!resolvedEnv.fallbackPlan) { | |
| throw new Error("TERMINAL/PROVIDER_SWITCH/003: fallback plan is required") | |
| } | |
| const source = this.getTerminalById(terminalId) | |
| if (!source) { | |
| throw new Error(`TERMINAL/PROVIDER_SWITCH/004: source terminal ${terminalId} not found`) | |
| } | |
| if (source.provider !== "vscode") { | |
| throw new Error("TERMINAL/PROVIDER_SWITCH/005: source terminal is not a VS Code terminal") | |
| } | |
| if (source.lifecycle.ownerExecutionId !== executionId) { | |
| throw new Error( | |
| `TERMINAL/PROVIDER_SWITCH/006: owner mismatch (expected ${executionId}, got ${source.lifecycle.ownerExecutionId})`, | |
| ) | |
| } | |
| // 1. Transition source to failed. | |
| source.lifecycle.transition("failed", executionId) | |
| // 2. Cancel shell-integration wait. | |
| if (source instanceof Terminal) { | |
| source.cancelShellIntegrationWait() | |
| } |
🤖 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/TerminalRegistry.ts` around lines 789 - 816,
Validate the terminal returned by getTerminalById before returning it from the
commandSubmitted path, throwing the same missing-source error instead of using
the non-null assertion. In the fallback path, require source to be an actual
Terminal instance before calling cancelShellIntegrationWait, and replace the
unchecked (source as Terminal) cast with the validated instance.
| // Send pending terminal shell selection (uses a separate message | ||
| // type with validation that isn't part of the updateSettings flow). | ||
| // Note: Do NOT reset pendingTerminalShellSelection here. Resetting it | ||
| // immediately causes the prop to TerminalSettings to temporarily revert | ||
| // to the stale state_terminalShellSelection (before postStateToWebview | ||
| // arrives), which triggers the useEffect that overwrites the user's | ||
| // selection and makes the dropdown show "Auto". Instead, let the | ||
| // pending value persist until the extension host syncs the updated | ||
| // state back via postStateToWebview(). | ||
| if (pendingTerminalShellSelection) { | ||
| vscode.postMessage({ | ||
| type: "setTerminalShellSelection", | ||
| terminalShellSelection: pendingTerminalShellSelection, | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)SettingsView\.tsx$|terminalShellSelection|setTerminalShellSelection|postStateToWebview' || true
echo "== SettingsView outline =="
ast-grep outline webview-ui/src/components/settings/SettingsView.tsx --view expanded || true
echo "== relevant SettingsView sections =="
wc -l webview-ui/src/components/settings/SettingsView.tsx
sed -n '1,180p' webview-ui/src/components/settings/SettingsView.tsx
sed -n '420,510p' webview-ui/src/components/settings/SettingsView.tsx
sed -n '880,950p' webview-ui/src/components/settings/SettingsView.tsx
echo "== search terminal shell selection definitions/usages =="
rg -n "terminalShellSelection|pendingTerminalShellSelection|setTerminalShellSelection|state_terminalShellSelection|postStateToWebview" -S .Repository: Zoo-Code-Org/Zoo-Code
Length of output: 47008
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== codebase references for terminal shell selection flow =="
rg -n "terminalShellSelection|pendingTerminalShellSelection|setTerminalShellSelection|state_terminalShellSelection|postStateToWebview|updateSettings" -S .
echo "== candidate SettingsView excerpts =="
if [ -f webview-ui/src/components/settings/SettingsView.tsx ]; then
wc -l webview-ui/src/components/settings/SettingsView.tsx
sed -n '1,180p' webview-ui/src/components/settings/SettingsView.tsx
sed -n '420,510p' webview-ui/src/components/settings/SettingsView.tsx
sed -n '880,950p' webview-ui/src/components/settings/SettingsView.tsx
fiRepository: Zoo-Code-Org/Zoo-Code
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== WebviewContext message handling =="
rg -n "useEffect\\(\\(|postMessage|onDidChange|vscode\\. |extensionState|cachedState|terminalShellSelection|messageQueue" webview-ui/src -S
echo "== WebviewContext files =="
git ls-files webview-ui/src | rg 'context|Vscode|vscode|useExtensionState|Webview'
echo "== WebviewContext candidate excerpts =="
fd -a '.*' webview-ui/src | sed 's#^\./##' | rg '(^|/)WebviewContext|useExtensionState|Vscode' | while read -r f; do
echo "--- $f $(wc -l < "$f")"
sed -n '1,260p' "$f"
done
echo "== extension host message handler shell selection =="
sed -n '1800,1845p' src/core/webview/webviewMessageHandler.ts
echo "== ClineProvider handleSetTerminalShellSelection =="
sed -n '3040,3175p' src/core/webview/ClineProvider.ts
echo "== ClineProvider postState payload =="
sed -n '2720,2765p' src/core/webview/ClineProvider.ts
echo "== extension listener =="
sed -n '196,214p' src/extension.ts
echo "== tests for normalized invalid terminal shell values =="
sed -n '395,470p' src/core/webview/__tests__/terminal-shell-messages.spec.ts
sed -n '1,140p' webview-ui/src/components/settings/__tests__/SettingsView.shell-selection.spec.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ExtensionStateContext message listener and cached hydration =="
sed -n '420,510p' webview-ui/src/context/ExtensionStateContext.tsx
sed -n '220,255p' webview-ui/src/components/settings/SettingsView.tsx
echo "== save block =="
sed -n '365,485p' webview-ui/src/components/settings/SettingsView.tsx
echo "== terminal shell handler and ClineProvider response =="
sed -n '1816,1834p' src/core/webview/webviewMessageHandler.ts
sed -n '3080,3148p' src/core/webview/ClineProvider.ts
sed -n '3156,3172p' src/core/webview/ClineProvider.ts
echo "== message protocol definitions =="
sed -n '760,800p' packages/types/src/vscode-extension-host.ts
sed -n '280,305p' packages/types/src/vscode-extension-host.ts
echo "== focused tests =="
sed -n '330,350p' webview-ui/src/components/settings/__tests__/SettingsView.shell-selection.spec.tsx
sed -n '395,470p' src/core/webview/__tests__/terminal-shell-messages.spec.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 18834
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== current message handler implementation =="
sed -n '142,490p' webview-ui/src/context/ExtensionStateContext.tsx
echo "== terminal settings pending refresh =="
sed -n '1,145p' webview-ui/src/components/settings/TerminalSettings.tsx
echo "== ExtensionStateContext tests that cover message hydration =="
rg -n "terminalShellSelection|setTerminalShellSelection|terminalShellOptions|extensionState|window\\.addEventListener|postMessageToWebview|vscode\\.postMessage|cache not" webview-ui/__tests__ webview-ui/src/__tests__ webview-ui/src/components webview-ui/src/context -S
echo "== focused tests around pending shell selection =="
rg -n "pendingTerminalShellSelection|terminalShellOptions|effectiveShell|SHELL/handleSetTerminalShellSelection|shell-selection" webview-ui/src -SRepository: Zoo-Code-Org/Zoo-Code
Length of output: 50377
Reconcile the pending shell selection after save.
Save sends setTerminalShellSelection, but SettingsView continues to render pendingTerminalShellSelection while cachedState.terminalShellSelection does not refresh from ordinary extension-state updates. Add handling for setTerminalShellSelection’s success/failure response, update the cached selection only for a valid response, and clear pendingTerminalShellSelection. Add coverage for responses that reject or normalize the submitted selection.
🤖 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 `@webview-ui/src/components/settings/SettingsView.tsx` around lines 463 - 478,
The save flow around pendingTerminalShellSelection must reconcile the extension
response instead of leaving the pending value indefinitely. Handle the
success/failure response for setTerminalShellSelection, update
cachedState.terminalShellSelection only when the response is valid, and clear
pendingTerminalShellSelection after handling it; preserve the existing value
when the response rejects or normalizes the submitted selection. Add tests
covering both rejected and normalized responses.
Source: Coding guidelines
1 participant
Stack Position
feature/unified-shell-resolutionDescription
https://youtube.com/shorts/-cm4pnaoXD0
Full Feature Description
feature/unified-shell-resolutionterminal.ts,global-settings.ts,vscode-extension-host.ts, the settings UITerminalSettings.tsxandSettingsView.tsx, the backend terminal layersrc/integrations/terminal, and the task/tool/API wiringTask.ts,ExecuteCommandTool.ts,api.ts.SettingsView.tsxbind tocachedState, not live extension state.terminal-profile.test.ts. Manually run the same command in default, PowerShell, Command Prompt, and where available WSL/POSIX profiles, comparing the selected executable, output, exit code, cancellation, and cleanup.Why Split Into 17 PRs
Instead of submitting this feature as a single unified PR, it was split into individual PRs because as code size grows, safely reviewing a PR becomes very difficult. The feature was broken into mutually exclusive individual PRs so that each can be reviewed independently.
What This PR Specifically Changes
Adds CLI/user/legacy/VS Code/OS/fallback priority resolver, platform shell classification, typed result/error, executable and safe argument array. Does not include scheduler, registry, or task wiring.
Included Files
src/integrations/terminal/shell/ShellResolver.tssrc/integrations/terminal/shell/ShellInvocationAdapter.tssrc/integrations/terminal/shell/TerminalProfileResolver.tssrc/utils/shell.tsExclusion Scope
src/integrations/terminal/CommandScheduler.tssrc/integrations/terminal/TerminalLifecycle.tssrc/integrations/terminal/TerminalRegistry.tssrc/integrations/terminal/CommandTrace.tsSummary by CodeRabbit