feature: unified-shell-resolution (2/4) - #1125
Open
myk1yt wants to merge 21 commits into
Open
Conversation
myk1yt
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
August 4, 2026 11:47
Contributor
📝 WalkthroughWalkthroughThe PR adds unified shell selection and resolution. It propagates resolved shell environments through prompts and terminal execution. It adds lifecycle ownership, scheduling, tracing, recovery, provider switching, webview settings, and extensive tests. ChangesTerminal shell feature
Supporting reports and validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
src/core/tools/ExecuteCommandTool.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
github-actions
Bot
added
the
awaiting-review
myk1yt
force-pushed
the
pr/b05-shell-resolution-v2
branch
3 times, most recently
from
August 4, 2026 20:40
fe6c9a7 to
2d862ca
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
Comment on lines
+274
to
+276
| const execTool = tools.find((t) => (t as any).function?.name === "execute_command") | ||
| expect(execTool).toBeDefined() | ||
| const desc = (execTool as any).function.description |
Contributor
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
Comment on lines
+54
to
69
| 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 | ||
| } | ||
| } |
Contributor
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.
| const closeIdleTerminalsMock = vi.fn() | ||
| vi.mock("../../../integrations/terminal/TerminalRegistry", () => ({ | ||
| TerminalRegistry: { | ||
| closeIdleTerminals: (...args: any[]) => closeIdleTerminalsMock(...args), |
Contributor
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
Comment on lines
+284
to
332
| 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) |
Contributor
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.
Comment on lines
+511
to
+520
| 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 | ||
| } |
Contributor
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.
Suggested change
| 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.
Comment on lines
+594
to
+676
| 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`, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
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.
Comment on lines
+789
to
+816
| 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() |
Contributor
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.
Suggested change
| 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.
myk1yt
force-pushed
the
pr/b05-shell-resolution-v2
branch
from
August 5, 2026 08:00
681e848 to
12775cb
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (1)
src/integrations/terminal/shell/TerminalProfileResolver.ts (1)
381-415: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the duplicated Windows name-based resolution.
resolveWellKnownProfileNamerepeats the logic inresolveProfileEntrylines 459-489. Both branches map a profile name that containspowershellorwslto the same executables and the sametrustEvidence. The only difference is the sanitizedenvfield.Extract one private helper that takes the profile name, the source, and an optional env, then call it from both sites. This prevents the two copies from diverging when the known Windows paths 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 `@src/integrations/terminal/shell/TerminalProfileResolver.ts` around lines 381 - 415, Extract a private helper for the shared Windows name-based resolution used by resolveWellKnownProfileName and resolveProfileEntry, accepting profileName, source, and optional env. Move the powershell/wsl matching and ResolvedShell construction into that helper, preserving the existing executable, family, displayName, and trustedProfile behavior while applying the optional sanitized env. Replace both duplicated branches with calls to the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/260731_0001_session_dashboard-blank-fix/164200_architect-report.md`:
- Line 39: The report must not present the 5.71 MiB webview bundle size or
current build output as verified evidence. In
docs/260731_0001_session_dashboard-blank-fix/164200_architect-report.md at lines
39-39 and 352-352, mark the value unverified or attach reproducible measurement
evidence; at lines 576-584, verify the output path exists and VSIX packaging
consumes it; at lines 605-609, remove the confirmation claim unless evidence is
recorded. In docs/feedbacks/fromarchitect/260801_missing_webview_build_path.md
at lines 11-21, retain the unmeasured status unless a reproducible build
measurement is added.
In `@docs/260801_0001_session_fork-pr-rebase-ci/222900_architect-report.md`:
- Line 160: Replace the movable backup branch command with an immutable
recovery-reference procedure, using annotated tags or a protected backup-ref
namespace for the pre-rewrite tip. Record each referenced object ID and
explicitly prohibit updates to the backup references before any branch
rewriting, including the corresponding command at the other occurrence.
In `@docs/260801_0001_session_fork-pr-rebase-ci/224000_code-report.md`:
- Around line 50-55: Replace the Next Step Recommendations’ plain rebase
workflow with rebuilding each B branch from its declared base using the
feature-commit manifest, excluding copied prerequisite and CI commits. After
rebuilding, run git range-diff and changed-file checks, then verify the branch
builds and tests pass; retain the backup-tag rollback guidance.
In `@docs/260801_0001_session_fork-pr-rebase-ci/224700_code-report.md`:
- Line 5: Keep the release status provisional until current-head remote gates
are verified: in
docs/260801_0001_session_fork-pr-rebase-ci/224700_code-report.md lines 5-5,
describe the results as local CI-equivalent checks passing; in lines 56-73,
defer overall success until GitHub Actions passes for the current head SHA; in
docs/260805_0001_session_ci-all-green/164900_code-report.md lines 28-36,
distinguish local coverage from Codecov acceptance; and in lines 44-48, record
the current-head Codecov result before marking the objective complete.
In `@docs/260801_0001_session_fork-pr-rebase-ci/232300_code-report.md`:
- Line 56: Add the bash language identifier to the fenced shell command block in
docs/260801_0001_session_fork-pr-rebase-ci/232300_code-report.md at lines 56-56,
and add the text language identifier to the dependency graph fence in
docs/260801_0001_session_fork-pr-rebase-ci/requirement-checklist.md at lines
36-36.
In `@docs/260801_0001_session_fork-pr-rebase-ci/requirement-checklist.md`:
- Around line 9-10: Update the REQ-001 and REQ-002 checklist entries in
requirement-checklist.md to checked, matching the completed sync and force-push
evidence recorded in rebase-evidence.md.
In `@docs/260805_0001_session_ci-all-green/163300_debug-coverage-b05.md`:
- Line 101: Correct the backend coverage calculation in the documented coverage
summary: update the stated additional-line gap from 3,523 to 3,534, while
preserving the surrounding totals, target, and planning context.
In `@docs/feedbacks/fromarchitect/260801_crow_recall_register_validation.md`:
- Line 1: Update each repeated top-level heading in the report, including every
occurrence of “Environment Feedback Report,” to use a unique title that
distinguishes its corresponding report while preserving the existing report
content.
In `@src/integrations/terminal/shell/TerminalProfileResolver.ts`:
- Around line 189-202: Add an explicit fallback branch to deriveDisplayName so
every ShellFamily value produces a string, preserving the function’s declared
return type when classifyShellFamily yields an unexpected or newly added family.
Use the project’s established exhaustiveness-guard pattern if available, and
ensure the fallback does not return undefined.
- Around line 390-399: Update the PowerShell resolution logic around the
executable selection and return object to verify both POWERSHELL_7_PATH and
POWERSHELL_LEGACY_PATH exist before returning a profile. Return undefined when
neither path exists, and apply the same guard to the equivalent resolution
patterns around the logic referenced at lines 464 and 515 so missing executables
fall through to the next source.
In
`@webview-ui/src/components/settings/__tests__/TerminalSettings.shell.spec.tsx`:
- Around line 263-266: Fix the vacuous auto-option assertion in the
TerminalSettings test by asserting the rendered option-auto entry count,
ensuring exactly one auto entry is present. Keep the existing PowerShell and cmd
assertions unchanged.
- Around line 171-182: Update the profile selection test around profileButton to
retrieve option-profile:PowerShell with getByTestId instead of conditionally
using queryByTestId. Remove the truthy guard so the click and both callback
assertions always execute, causing the test to fail when the option is not
rendered.
In `@webview-ui/src/i18n/locales/de/settings.json`:
- Around line 849-865: Translate every string in the terminal.inlineShell block:
update the German keys in webview-ui/src/i18n/locales/de/settings.json lines
849-865, the Korean keys in webview-ui/src/i18n/locales/ko/settings.json lines
849-865, and the Vietnamese keys in webview-ui/src/i18n/locales/vi/settings.json
lines 849-865, covering label, description, auto, customPath,
customPathPlaceholder, all effectiveShell fields, and both error messages while
preserving the existing JSON structure and keys.
---
Nitpick comments:
In `@src/integrations/terminal/shell/TerminalProfileResolver.ts`:
- Around line 381-415: Extract a private helper for the shared Windows
name-based resolution used by resolveWellKnownProfileName and
resolveProfileEntry, accepting profileName, source, and optional env. Move the
powershell/wsl matching and ResolvedShell construction into that helper,
preserving the existing executable, family, displayName, and trustedProfile
behavior while applying the optional sanitized env. Replace both duplicated
branches with calls to the helper.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e543546-8137-4aba-ae4b-7a9598d73d37
⛔ 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 (90)
apps/vscode-e2e/src/suite/tools/terminal-profile.test.tsdocs/260731_0001_session_dashboard-blank-fix/164200_architect-report.mddocs/260801_0001_session_fork-pr-rebase-ci/222900_architect-report.mddocs/260801_0001_session_fork-pr-rebase-ci/224000_code-report.mddocs/260801_0001_session_fork-pr-rebase-ci/224700_code-report.mddocs/260801_0001_session_fork-pr-rebase-ci/230415_code-report.mddocs/260801_0001_session_fork-pr-rebase-ci/232300_code-report.mddocs/260801_0001_session_fork-pr-rebase-ci/234030_code-report.mddocs/260801_0001_session_fork-pr-rebase-ci/decisions.mddocs/260801_0001_session_fork-pr-rebase-ci/rebase-evidence.mddocs/260801_0001_session_fork-pr-rebase-ci/requirement-checklist.mddocs/260805_0001_session_ci-all-green/163300_debug-coverage-b05.mddocs/260805_0001_session_ci-all-green/164900_code-report.mddocs/feedbacks/fromarchitect/260801_crow_recall_register_validation.mddocs/feedbacks/fromarchitect/260801_missing_webview_build_path.mdpackages/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__/CommandEnvironmentService.spec.tssrc/integrations/terminal/__tests__/CommandScheduler.spec.tssrc/integrations/terminal/__tests__/CommandTrace.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
🚧 Files skipped from review as they are similar to previous changes (67)
- webview-ui/src/i18n/locales/tr/settings.json
- webview-ui/src/i18n/locales/zh-TW/settings.json
- webview-ui/src/i18n/locales/zh-CN/settings.json
- src/integrations/terminal/tests/TerminalProcessExec.bash.spec.ts
- src/integrations/terminal/tests/TerminalProfile.spec.ts
- webview-ui/src/i18n/locales/pl/settings.json
- webview-ui/src/i18n/locales/ru/settings.json
- src/integrations/terminal/tests/TerminalProcessExec.pwsh.spec.ts
- src/core/task/build-tools.ts
- src/core/webview/generateSystemPrompt.ts
- src/core/prompts/system.ts
- src/integrations/terminal/tests/TerminalProcessExec.cmd.spec.ts
- src/integrations/terminal/tests/TerminalLifecycle.spec.ts
- src/integrations/terminal/tests/ShellInvocationAdapter.spec.ts
- src/core/prompts/tests/shell-environment-prompt.spec.ts
- webview-ui/src/i18n/locales/pt-BR/settings.json
- webview-ui/src/i18n/locales/fr/settings.json
- packages/types/src/terminal.ts
- src/core/tools/tests/terminal-provider-fallback.spec.ts
- src/core/tools/tests/executeCommand.spec.ts
- src/integrations/terminal/shell/types.ts
- packages/types/src/global-settings.ts
- src/core/webview/webviewMessageHandler.ts
- src/extension.ts
- webview-ui/src/i18n/locales/nl/settings.json
- webview-ui/src/i18n/locales/es/settings.json
- src/core/prompts/tools/native-tools/index.ts
- webview-ui/src/i18n/locales/id/settings.json
- packages/types/src/tests/terminal-shell-settings.spec.ts
- src/integrations/terminal/ExecaTerminalProcess.ts
- webview-ui/src/i18n/locales/en/settings.json
- src/core/webview/tests/terminal-shell-messages.spec.ts
- webview-ui/src/i18n/locales/ja/settings.json
- webview-ui/src/components/settings/tests/SettingsView.shell-selection.spec.tsx
- src/integrations/terminal/CommandScheduler.ts
- src/integrations/terminal/tests/ExecaTerminalProcess.spec.ts
- webview-ui/src/i18n/locales/hi/settings.json
- webview-ui/src/i18n/locales/ca/settings.json
- src/core/prompts/sections/system-info.ts
- src/extension/api.ts
- src/core/prompts/sections/rules.ts
- src/integrations/terminal/shell/CommandEnvironmentService.ts
- src/integrations/terminal/CommandTrace.ts
- src/utils/tests/shell.spec.ts
- src/integrations/terminal/tests/TerminalRegistry.spec.ts
- webview-ui/src/i18n/locales/it/settings.json
- src/integrations/terminal/TerminalProcess.ts
- apps/vscode-e2e/src/suite/tools/terminal-profile.test.ts
- src/integrations/terminal/tests/TerminalProcess.spec.ts
- src/eslint-suppressions.json
- src/core/prompts/tools/native-tools/execute_command.ts
- webview-ui/src/components/settings/SettingsView.tsx
- src/integrations/terminal/shell/ShellInvocationAdapter.ts
- src/integrations/terminal/types.ts
- webview-ui/src/components/settings/TerminalSettings.tsx
- packages/types/src/vscode-extension-host.ts
- src/core/webview/ClineProvider.ts
- src/utils/shell.ts
- src/integrations/terminal/Terminal.ts
- src/integrations/terminal/BaseTerminal.ts
- src/core/tools/tests/executeCommandTool.spec.ts
- src/integrations/terminal/TerminalLifecycle.ts
- src/integrations/terminal/shell/ShellResolver.ts
- src/integrations/terminal/TerminalRegistry.ts
- src/core/task/Task.ts
- src/integrations/terminal/tests/CommandScheduler.spec.ts
- src/core/tools/ExecuteCommandTool.ts
| | Today coverage | 0.644 ms | | ||
| | NDJSON size | 7.25 MiB | | ||
| | NDJSON idempotency rebuild, warm median | about 115 ms | | ||
| | Main webview JavaScript bundle | 5.71 MiB | |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not treat the webview bundle size as verified evidence.
The architecture report records 5.71 MiB and says the build output was confirmed. The environment feedback says webview-ui/build/assets was absent and the bundle size was unmeasured. Mark this value as historical, or rebuild and record the exact output path and packaged artifact before using it in the Dashboard decision.
docs/260731_0001_session_dashboard-blank-fix/164200_architect-report.md#L39-L39: mark the bundle size as unverified or attach the measurement.docs/260731_0001_session_dashboard-blank-fix/164200_architect-report.md#L352-L352: do not use the 5.71 MiB value as confirmed evidence.docs/260731_0001_session_dashboard-blank-fix/164200_architect-report.md#L576-L584: verify that the build output exists and that VSIX packaging consumes it.docs/260731_0001_session_dashboard-blank-fix/164200_architect-report.md#L605-L609: remove the claim that the current build output was confirmed unless evidence is recorded.docs/feedbacks/fromarchitect/260801_missing_webview_build_path.md#L11-L21: keep the unmeasured status unless a reproducible build measurement is added.
📍 Affects 2 files
docs/260731_0001_session_dashboard-blank-fix/164200_architect-report.md#L39-L39(this comment)docs/260731_0001_session_dashboard-blank-fix/164200_architect-report.md#L352-L352docs/260731_0001_session_dashboard-blank-fix/164200_architect-report.md#L576-L584docs/260731_0001_session_dashboard-blank-fix/164200_architect-report.md#L605-L609docs/feedbacks/fromarchitect/260801_missing_webview_build_path.md#L11-L21
🤖 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 `@docs/260731_0001_session_dashboard-blank-fix/164200_architect-report.md` at
line 39, The report must not present the 5.71 MiB webview bundle size or current
build output as verified evidence. In
docs/260731_0001_session_dashboard-blank-fix/164200_architect-report.md at lines
39-39 and 352-352, mark the value unverified or attach reproducible measurement
evidence; at lines 576-584, verify the output path exists and VSIX packaging
consumes it; at lines 605-609, remove the confirmation claim unless evidence is
recorded. In docs/feedbacks/fromarchitect/260801_missing_webview_build_path.md
at lines 11-21, retain the unmeasured status unless a reproducible build
measurement is added.
| git rev-parse upstream/main | ||
| git rev-parse myk1yt/main | ||
| git rev-list --left-right --count upstream/main...myk1yt/main | ||
| git branch backup/main-before-sync-260801 myk1yt/main |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use immutable recovery references for backups.
git branch backup/... creates a movable branch. The plan requires an immutable recovery reference for every pre-rewrite tip. Use annotated tags or a protected backup-ref procedure, record the object IDs, and prohibit updates to the backup namespace before rewriting branches.
Also applies to: 187-187
🤖 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 `@docs/260801_0001_session_fork-pr-rebase-ci/222900_architect-report.md` at
line 160, Replace the movable backup branch command with an immutable
recovery-reference procedure, using annotated tags or a protected backup-ref
namespace for the pre-rewrite tip. Record each referenced object ID and
explicitly prohibit updates to the backup references before any branch
rewriting, including the corresponding command at the other occurrence.
Comment on lines
+50
to
+55
| ## Next Step Recommendations | ||
|
|
||
| - Proceed with Sub-task 2: rebase each B branch onto the new main (`992585ff8`) | ||
| - Use `git rebase main <branch-name>` for each branch, resolving conflicts as needed | ||
| - After each successful rebase, verify the branch still builds and tests pass | ||
| - Backup tags remain available for rollback if any rebase fails |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not use a plain rebase for these branches.
The plan states that current branches contain copied prerequisite and CI commits. git rebase main <branch-name> will retain those commits and can make each PR include unrelated history. Rebuild each branch from its declared base with the feature-commit manifest, then run git range-diff and changed-file checks.
🤖 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 `@docs/260801_0001_session_fork-pr-rebase-ci/224000_code-report.md` around
lines 50 - 55, Replace the Next Step Recommendations’ plain rebase workflow with
rebuilding each B branch from its declared base using the feature-commit
manifest, excluding copied prerequisite and CI commits. After rebuilding, run
git range-diff and changed-file checks, then verify the branch builds and tests
pass; retain the backup-tag rollback guidance.
|
|
||
| ### 5. Test Execution | ||
|
|
||
| ``` |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language identifiers to both fenced Markdown blocks.
Both fences omit the language identifier required by Markdown linting.
docs/260801_0001_session_fork-pr-rebase-ci/232300_code-report.md#L56-L56: usebashfor the shell command block.docs/260801_0001_session_fork-pr-rebase-ci/requirement-checklist.md#L36-L36: usetextfor the dependency graph block.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
📍 Affects 2 files
docs/260801_0001_session_fork-pr-rebase-ci/232300_code-report.md#L56-L56(this comment)docs/260801_0001_session_fork-pr-rebase-ci/requirement-checklist.md#L36-L36
🤖 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 `@docs/260801_0001_session_fork-pr-rebase-ci/232300_code-report.md` at line 56,
Add the bash language identifier to the fenced shell command block in
docs/260801_0001_session_fork-pr-rebase-ci/232300_code-report.md at lines 56-56,
and add the text language identifier to the dependency graph fence in
docs/260801_0001_session_fork-pr-rebase-ci/requirement-checklist.md at lines
36-36.
Source: Linters/SAST tools
Comment on lines
+189
to
+202
| function deriveDisplayName(family: ShellFamily, executable: string, profileName?: string): string { | ||
| switch (family) { | ||
| case "powershell": | ||
| return /pwsh/i.test(executable) ? "PowerShell 7" : "Windows PowerShell 5.1" | ||
| case "cmd": | ||
| return "Command Prompt" | ||
| case "wsl": | ||
| return profileName ? `WSL: ${profileName}` : "WSL" | ||
| case "fish": | ||
| return "Fish" | ||
| case "posix": | ||
| return path.basename(executable) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add an exhaustiveness guard to deriveDisplayName.
The switch has no default branch. The function is typed to return string, but it returns undefined when family is not one of the five handled values. A new ShellFamily member, or an unvalidated value from classifyShellFamily, then produces displayName: undefined inside a ResolvedShell.
Add an explicit fallback so the return type stays honest.
🛡️ Proposed fallback branch
case "posix":
return path.basename(executable)
+ default:
+ return profileName ?? path.basename(executable)
}
}Run this script to confirm the ShellFamily union members:
#!/bin/bash
# Confirm the ShellFamily union and classifyShellFamily return values.
fd -t f 'types.ts' src/integrations/terminal/shell --exec cat -n
rg -n -C4 'ShellFamily' src/utils/shell.ts🤖 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 189
- 202, Add an explicit fallback branch to deriveDisplayName so every ShellFamily
value produces a string, preserving the function’s declared return type when
classifyShellFamily yields an unexpected or newly added family. Use the
project’s established exhaustiveness-guard pattern if available, and ensure the
fallback does not return undefined.
Comment on lines
+390
to
+399
| const executable = this.fs.existsSync(POWERSHELL_7_PATH) ? POWERSHELL_7_PATH : POWERSHELL_LEGACY_PATH | ||
|
|
||
| return { | ||
| executable, | ||
| family: "powershell", | ||
| displayName: deriveDisplayName("powershell", executable, profileName), | ||
| source, | ||
| profileName, | ||
| trustEvidence: "trustedProfile", | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Verify the legacy PowerShell path before you return it.
If POWERSHELL_7_PATH does not exist, the code returns POWERSHELL_LEGACY_PATH without an existence check. On a Windows host without Windows PowerShell 5.1, the resolver reports a resolved shell with trustEvidence: "trustedProfile" for an executable that is absent. The failure then surfaces later as a terminal spawn error instead of a clean fallback to the next resolution source. Lines 464 and 515 use the same pattern.
Return undefined when neither executable exists.
🐛 Proposed check
- if (nameLower.includes("powershell")) {
- const executable = this.fs.existsSync(POWERSHELL_7_PATH) ? POWERSHELL_7_PATH : POWERSHELL_LEGACY_PATH
+ if (nameLower.includes("powershell")) {
+ const executable = this.fs.existsSync(POWERSHELL_7_PATH)
+ ? POWERSHELL_7_PATH
+ : this.fs.existsSync(POWERSHELL_LEGACY_PATH)
+ ? POWERSHELL_LEGACY_PATH
+ : undefined
+ if (!executable) {
+ return undefined
+ }📝 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.
Suggested change
| const executable = this.fs.existsSync(POWERSHELL_7_PATH) ? POWERSHELL_7_PATH : POWERSHELL_LEGACY_PATH | |
| return { | |
| executable, | |
| family: "powershell", | |
| displayName: deriveDisplayName("powershell", executable, profileName), | |
| source, | |
| profileName, | |
| trustEvidence: "trustedProfile", | |
| } | |
| const executable = this.fs.existsSync(POWERSHELL_7_PATH) | |
| ? POWERSHELL_7_PATH | |
| : this.fs.existsSync(POWERSHELL_LEGACY_PATH) | |
| ? POWERSHELL_LEGACY_PATH | |
| : undefined | |
| if (!executable) { | |
| return undefined | |
| } | |
| return { | |
| executable, | |
| family: "powershell", | |
| displayName: deriveDisplayName("powershell", executable, profileName), | |
| source, | |
| profileName, | |
| trustEvidence: "trustedProfile", | |
| } |
🤖 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 390
- 399, Update the PowerShell resolution logic around the executable selection
and return object to verify both POWERSHELL_7_PATH and POWERSHELL_LEGACY_PATH
exist before returning a profile. Return undefined when neither path exists, and
apply the same guard to the equivalent resolution patterns around the logic
referenced at lines 464 and 515 so missing executables fall through to the next
source.
Comment on lines
+171
to
+182
| const profileButton = screen.queryByTestId("option-profile:PowerShell") | ||
| if (profileButton) { | ||
| act(() => { | ||
| fireEvent.click(profileButton) | ||
| }) | ||
|
|
||
| expect(onShellSelectionChange).toHaveBeenCalledWith({ | ||
| kind: "profile", | ||
| profileName: "PowerShell", | ||
| }) | ||
| expect(onTerminalProfilePickerOpened).toHaveBeenCalled() | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the profile selection assertions unconditional.
The assertions run only when profileButton is truthy. If the component stops rendering the profile:PowerShell option, this test passes with zero assertions and hides the regression. Query the element with getByTestId so a missing option fails the test.
💚 Proposed fix
- const profileButton = screen.queryByTestId("option-profile:PowerShell")
- if (profileButton) {
- act(() => {
- fireEvent.click(profileButton)
- })
-
- expect(onShellSelectionChange).toHaveBeenCalledWith({
- kind: "profile",
- profileName: "PowerShell",
- })
- expect(onTerminalProfilePickerOpened).toHaveBeenCalled()
- }
+ const profileButton = screen.getByTestId("option-profile:PowerShell")
+ act(() => {
+ fireEvent.click(profileButton)
+ })
+
+ expect(onShellSelectionChange).toHaveBeenCalledWith({
+ kind: "profile",
+ profileName: "PowerShell",
+ })
+ expect(onTerminalProfilePickerOpened).toHaveBeenCalled()📝 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.
Suggested change
| const profileButton = screen.queryByTestId("option-profile:PowerShell") | |
| if (profileButton) { | |
| act(() => { | |
| fireEvent.click(profileButton) | |
| }) | |
| expect(onShellSelectionChange).toHaveBeenCalledWith({ | |
| kind: "profile", | |
| profileName: "PowerShell", | |
| }) | |
| expect(onTerminalProfilePickerOpened).toHaveBeenCalled() | |
| } | |
| const profileButton = screen.getByTestId("option-profile:PowerShell") | |
| act(() => { | |
| fireEvent.click(profileButton) | |
| }) | |
| expect(onShellSelectionChange).toHaveBeenCalledWith({ | |
| kind: "profile", | |
| profileName: "PowerShell", | |
| }) | |
| expect(onTerminalProfilePickerOpened).toHaveBeenCalled() |
🤖 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 171 - 182, Update the profile selection test around profileButton
to retrieve option-profile:PowerShell with getByTestId instead of conditionally
using queryByTestId. Remove the truthy guard so the click and both callback
assertions always execute, causing the test to fail when the option is not
rendered.
Comment on lines
+263
to
+266
| // The "auto" option must not be re-rendered as a selectable item. | ||
| expect(screen.queryByTestId("option-auto")).toBeDefined() | ||
| expect(screen.getByTestId("option-profile:PowerShell")).toBeDefined() | ||
| expect(screen.getByTestId("option-cmd")).toBeDefined() |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the vacuous assertion on the auto option.
The comment states that auto must not be re-rendered as a selectable item, but expect(screen.queryByTestId("option-auto")).toBeDefined() passes in both cases. queryByTestId returns null when the element is absent, and expect(null).toBeDefined() succeeds. The assertion tests nothing.
If the intent is a single auto entry, assert the count instead.
💚 Proposed fix
- // The "auto" option must not be re-rendered as a selectable item.
- expect(screen.queryByTestId("option-auto")).toBeDefined()
+ // The "auto" option must appear exactly once, not duplicated from the payload.
+ expect(screen.queryAllByTestId("option-auto")).toHaveLength(1)
expect(screen.getByTestId("option-profile:PowerShell")).toBeDefined()
expect(screen.getByTestId("option-cmd")).toBeDefined()📝 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.
Suggested change
| // The "auto" option must not be re-rendered as a selectable item. | |
| expect(screen.queryByTestId("option-auto")).toBeDefined() | |
| expect(screen.getByTestId("option-profile:PowerShell")).toBeDefined() | |
| expect(screen.getByTestId("option-cmd")).toBeDefined() | |
| // The "auto" option must appear exactly once, not duplicated from the payload. | |
| expect(screen.queryAllByTestId("option-auto")).toHaveLength(1) | |
| expect(screen.getByTestId("option-profile:PowerShell")).toBeDefined() | |
| expect(screen.getByTestId("option-cmd")).toBeDefined() |
🤖 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 263 - 266, Fix the vacuous auto-option assertion in the
TerminalSettings test by asserting the rendered option-auto entry count,
ensuring exactly one auto entry is present. Keep the existing PowerShell and cmd
assertions unchanged.
Comment on lines
+849
to
+865
| "inlineShell": { | ||
| "label": "Inline Terminal Shell", | ||
| "description": "Select the shell used for inline terminal command execution. Auto follows your trusted VS Code terminal profile. Custom paths are validated by the extension host.", | ||
| "auto": "Auto (follows trusted terminal profile)", | ||
| "customPath": "Choose custom executable", | ||
| "customPathPlaceholder": "Select a shell executable...", | ||
| "effectiveShell": { | ||
| "label": "Effective shell", | ||
| "family": "Family", | ||
| "source": "Source", | ||
| "fallback": "Fallback behavior", | ||
| "fallbackDescription": "If shell integration fails, commands retry using the same shell family." | ||
| }, | ||
| "error": { | ||
| "invalid": "The selected shell is not supported. Choose a trusted profile or a valid shell executable.", | ||
| "unavailable": "Shell options are currently unavailable. The extension host may still be initializing." | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Untranslated terminal.inlineShell block in three locale files. The new English block was copied into each non-English locale without translation, so users of these locales see mixed-language text in the inline shell selector.
webview-ui/src/i18n/locales/de/settings.json#L849-L865: translatelabel,description,auto,customPath,customPathPlaceholder, theeffectiveShellfields, and botherrormessages into German.webview-ui/src/i18n/locales/ko/settings.json#L849-L865: translate the same keys into Korean.webview-ui/src/i18n/locales/vi/settings.json#L849-L865: translate the same keys into Vietnamese.
📍 Affects 3 files
webview-ui/src/i18n/locales/de/settings.json#L849-L865(this comment)webview-ui/src/i18n/locales/ko/settings.json#L849-L865webview-ui/src/i18n/locales/vi/settings.json#L849-L865
🤖 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/de/settings.json` around lines 849 - 865,
Translate every string in the terminal.inlineShell block: update the German keys
in webview-ui/src/i18n/locales/de/settings.json lines 849-865, the Korean keys
in webview-ui/src/i18n/locales/ko/settings.json lines 849-865, and the
Vietnamese keys in webview-ui/src/i18n/locales/vi/settings.json lines 849-865,
covering label, description, auto, customPath, customPathPlaceholder, all
effectiveShell fields, and both error messages while preserving the existing
JSON structure and keys.
added 14 commits
August 5, 2026 21:16
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.
added 5 commits
August 5, 2026 21:24
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.
…ice, ShellResolver edge cases, and webview inline shell selector - Add CommandTrace.spec.ts (100% line coverage for builder + collector) - Add CommandEnvironmentService.spec.ts (98% line coverage; previously 0%) - Extend ShellResolver.spec.ts with bare-name normalization, invalid-path, and Unix env-probe fallback cases (76% -> 84%) - Extend TerminalSettings.shell.spec.tsx with path/cmd selection, custom path button, and dropdown value mapping (63% -> 80%) - Add session coverage report
github-actions
Bot
added
the
has-conflicts
myk1yt
force-pushed
the
pr/b05-shell-resolution-v2
branch
from
August 5, 2026 13:15
12775cb to
928b9e0
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (6)
docs/260801_0001_session_fork-pr-rebase-ci/232300_code-report.md (1)
56-56: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd language identifiers to both fenced Markdown blocks.
docs/260801_0001_session_fork-pr-rebase-ci/232300_code-report.md#L56-L56: usebashfor the shell command block.docs/260801_0001_session_fork-pr-rebase-ci/requirement-checklist.md#L36-L36: usetextfor the dependency graph 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 `@docs/260801_0001_session_fork-pr-rebase-ci/232300_code-report.md` at line 56, Specify the appropriate language identifiers on both fenced Markdown blocks: use bash for the shell command block in docs/260801_0001_session_fork-pr-rebase-ci/232300_code-report.md at lines 56-56, and text for the dependency graph block in docs/260801_0001_session_fork-pr-rebase-ci/requirement-checklist.md at lines 36-36.Source: Linters/SAST tools
docs/260801_0001_session_fork-pr-rebase-ci/requirement-checklist.md (1)
9-10: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMark the completed sync requirements as complete.
The recorded sync evidence shows identical
upstream/mainandmyk1yt/mainSHAs with zero divergence. Mark REQ-001 and REQ-002 as checked so the checklist matches the evidence.🤖 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 `@docs/260801_0001_session_fork-pr-rebase-ci/requirement-checklist.md` around lines 9 - 10, Update the checklist entries for REQ-001 and REQ-002 to checked, reflecting the recorded evidence that upstream/main and myk1yt/main are synchronized with zero divergence.docs/260801_0001_session_fork-pr-rebase-ci/222900_architect-report.md (1)
160-160: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse immutable recovery references for every pre-rewrite tip.
git branch backup/...creates movable references. A later update or deletion can invalidate rollback targets. Use annotated or protected tags, record each object ID, and prohibit updates to the backup namespace before rewriting branches.Also applies to: 187-187
🤖 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 `@docs/260801_0001_session_fork-pr-rebase-ci/222900_architect-report.md` at line 160, Replace the movable backup reference created by the backup/main-before-sync-260801 command in the architect-report content with an immutable recovery reference: use a protected annotated tag or record the pre-rewrite object ID for myk1yt/main, and explicitly prevent updates or deletion in the backup namespace before any branch rewrite. Keep the guidance scoped to the existing recovery step and update the surrounding backup/rewrite instructions consistently wherever the same pattern appears.docs/feedbacks/fromarchitect/260801_crow_recall_register_validation.md (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winGive each feedback report a unique heading.
The file repeats
# Environment Feedback Reportfive times. Add an issue-specific suffix to each heading so Markdownlint and readers can distinguish the sections.Also applies to: 34-34, 67-67, 100-100, 133-133
🤖 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 `@docs/feedbacks/fromarchitect/260801_crow_recall_register_validation.md` at line 1, Update each repeated “Environment Feedback Report” heading in the feedback report so it includes a unique, issue-specific suffix identifying that section; preserve the heading level and ensure all five headings are distinct.Source: Linters/SAST tools
docs/260801_0001_session_fork-pr-rebase-ci/224000_code-report.md (1)
52-53: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse one precise branch-rebuild instruction.
The decision record’s generic “rebase” wording and the code report’s plain rebase command can retain copied prerequisite and CI commits.
docs/260801_0001_session_fork-pr-rebase-ci/224000_code-report.md#L52-L53: replace the plain rebase command with selective rebuild, cherry-pick, range-diff, and changed-file checks.docs/260801_0001_session_fork-pr-rebase-ci/decisions.md#L6-L6: state “selective rebuild and cherry-pick in dependency order” instead of generic rebase wording.🤖 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 `@docs/260801_0001_session_fork-pr-rebase-ci/224000_code-report.md` around lines 52 - 53, Replace the generic rebase guidance with a single precise branch-rebuild instruction: in docs/260801_0001_session_fork-pr-rebase-ci/224000_code-report.md#L52-L53, instruct selective rebuild via cherry-picking in dependency order, followed by range-diff and changed-file checks, instead of a plain rebase command; in docs/260801_0001_session_fork-pr-rebase-ci/decisions.md#L6-L6, update the wording to “selective rebuild and cherry-pick in dependency order” so both references consistently point to the same recovery workflow.docs/260731_0001_session_dashboard-blank-fix/164200_architect-report.md (1)
39-39: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not mark the webview bundle evidence as confirmed before packaging validation.
5.71 MiBis presented as measured evidence at Lines 39 and 352. Line 609 also says the current build output was confirmed. However, Line 584 still makes VSIX installation and artifact-path capture a future verification step. Mark the value as unverified, or attach reproducible build and packaged-artifact evidence.Also applies to: 352-352, 609-609
🤖 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 `@docs/260731_0001_session_dashboard-blank-fix/164200_architect-report.md` at line 39, Update the evidence status in the architect report where the Main webview JavaScript bundle size is mentioned, so the 5.71 MiB value and the current build output are not described as confirmed before packaging validation is complete. Use the existing evidence/verification language in the report to mark the bundle measurement as unverified unless reproducible build and packaged-artifact evidence is already attached, and keep the VSIX installation and artifact-path capture step as the pending verification point.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/260801_0001_session_fork-pr-rebase-ci/222900_architect-report.md`:
- Around line 453-459: Keep the B11 proof gate enforced in the architecture
report and align the decision record with it: update
docs/260801_0001_session_fork-pr-rebase-ci/222900_architect-report.md at 453-459
to preserve the three accepted outcomes as the blocking check for B12/B15/B16,
and update docs/260801_0001_session_fork-pr-rebase-ci/decisions.md at 7-7 so the
B11 assumption is marked pending proof unless you can cite the exact manifest or
upstream evidence; if B11 is already present, record that specific proof rather
than treating it as approved by default.
In `@docs/260801_0001_session_fork-pr-rebase-ci/230415_code-report.md`:
- Around line 34-39: Update the verification sections in
docs/260801_0001_session_fork-pr-rebase-ci/230415_code-report.md lines 34-39 and
docs/260801_0001_session_fork-pr-rebase-ci/232300_code-report.md lines 45-52 to
label the listed commands as local CI-equivalent checks, not GitHub CI results;
retain the pending SHA-specific GitHub status in the first report and explicitly
distinguish local results from GitHub CI in the second, including that hooks
were bypassed.
In `@packages/types/src/__tests__/terminal-shell-settings.spec.ts`:
- Around line 60-62: Rename the test case around
terminalShellSelectionSchema.parse to state that a profile with an empty
profileName is accepted, preserving the existing non-throwing expectation.
In `@src/core/tools/ExecuteCommandTool.ts`:
- Around line 361-367: The fallback execution paths in ExecuteCommandTool must
validate fallback plans before running commands. At
src/core/tools/ExecuteCommandTool.ts#L361-L367, compare
resolvedEnv.primaryPlan.family with resolvedEnv.fallbackPlan?.family before
retrying; report ShellFallbackMismatchError and skip replay when the fallback
plan is missing or belongs to another shell family. At
src/core/tools/ExecuteCommandTool.ts#L802-L812, when useFallbackPlan is true and
resolvedEnv.fallbackPlan is undefined, raise the same error instead of executing
without an invocation plan.
- Around line 373-375: Update the retry note in ExecuteCommandTool around
pushToolResult so it only reports that terminal shell integration was
unavailable and the command was automatically retried, without claiming the
command completed successfully. Keep the existing result string appended
unchanged so executeCommandInTerminal continues to convey the actual outcome,
including failures or still-running commands.
- Around line 643-657: Arm the command-output ask timer immediately when
execution starts, in addition to the existing output-driven scheduling. Update
the execution-start flow around onShellExecutionStarted (or the surrounding
command initialization) to invoke scheduleCommandOutputAsk once before waiting
for output, while preserving its guards against background runs, completed
commands, prior asks, and existing timers.
In `@webview-ui/src/i18n/locales/fr/settings.json`:
- Around line 853-869: Translate every English value under the inlineShell
object in the French settings locale, including labels, descriptions, option
text, placeholders, effectiveShell fields, and error messages. Preserve all
existing keys and placeholders while ensuring French users no longer see English
text.
---
Duplicate comments:
In `@docs/260731_0001_session_dashboard-blank-fix/164200_architect-report.md`:
- Line 39: Update the evidence status in the architect report where the Main
webview JavaScript bundle size is mentioned, so the 5.71 MiB value and the
current build output are not described as confirmed before packaging validation
is complete. Use the existing evidence/verification language in the report to
mark the bundle measurement as unverified unless reproducible build and
packaged-artifact evidence is already attached, and keep the VSIX installation
and artifact-path capture step as the pending verification point.
In `@docs/260801_0001_session_fork-pr-rebase-ci/222900_architect-report.md`:
- Line 160: Replace the movable backup reference created by the
backup/main-before-sync-260801 command in the architect-report content with an
immutable recovery reference: use a protected annotated tag or record the
pre-rewrite object ID for myk1yt/main, and explicitly prevent updates or
deletion in the backup namespace before any branch rewrite. Keep the guidance
scoped to the existing recovery step and update the surrounding backup/rewrite
instructions consistently wherever the same pattern appears.
In `@docs/260801_0001_session_fork-pr-rebase-ci/224000_code-report.md`:
- Around line 52-53: Replace the generic rebase guidance with a single precise
branch-rebuild instruction: in
docs/260801_0001_session_fork-pr-rebase-ci/224000_code-report.md#L52-L53,
instruct selective rebuild via cherry-picking in dependency order, followed by
range-diff and changed-file checks, instead of a plain rebase command; in
docs/260801_0001_session_fork-pr-rebase-ci/decisions.md#L6-L6, update the
wording to “selective rebuild and cherry-pick in dependency order” so both
references consistently point to the same recovery workflow.
In `@docs/260801_0001_session_fork-pr-rebase-ci/232300_code-report.md`:
- Line 56: Specify the appropriate language identifiers on both fenced Markdown
blocks: use bash for the shell command block in
docs/260801_0001_session_fork-pr-rebase-ci/232300_code-report.md at lines 56-56,
and text for the dependency graph block in
docs/260801_0001_session_fork-pr-rebase-ci/requirement-checklist.md at lines
36-36.
In `@docs/260801_0001_session_fork-pr-rebase-ci/requirement-checklist.md`:
- Around line 9-10: Update the checklist entries for REQ-001 and REQ-002 to
checked, reflecting the recorded evidence that upstream/main and myk1yt/main are
synchronized with zero divergence.
In `@docs/feedbacks/fromarchitect/260801_crow_recall_register_validation.md`:
- Line 1: Update each repeated “Environment Feedback Report” heading in the
feedback report so it includes a unique, issue-specific suffix identifying that
section; preserve the heading level and ensure all five headings are distinct.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2184455b-a52d-4dac-bbbc-61971bfc30cc
⛔ 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 (90)
apps/vscode-e2e/src/suite/tools/terminal-profile.test.tsdocs/260731_0001_session_dashboard-blank-fix/164200_architect-report.mddocs/260801_0001_session_fork-pr-rebase-ci/222900_architect-report.mddocs/260801_0001_session_fork-pr-rebase-ci/224000_code-report.mddocs/260801_0001_session_fork-pr-rebase-ci/224700_code-report.mddocs/260801_0001_session_fork-pr-rebase-ci/230415_code-report.mddocs/260801_0001_session_fork-pr-rebase-ci/232300_code-report.mddocs/260801_0001_session_fork-pr-rebase-ci/234030_code-report.mddocs/260801_0001_session_fork-pr-rebase-ci/decisions.mddocs/260801_0001_session_fork-pr-rebase-ci/rebase-evidence.mddocs/260801_0001_session_fork-pr-rebase-ci/requirement-checklist.mddocs/260805_0001_session_ci-all-green/163300_debug-coverage-b05.mddocs/260805_0001_session_ci-all-green/164900_code-report.mddocs/feedbacks/fromarchitect/260801_crow_recall_register_validation.mddocs/feedbacks/fromarchitect/260801_missing_webview_build_path.mdpackages/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__/CommandEnvironmentService.spec.tssrc/integrations/terminal/__tests__/CommandScheduler.spec.tssrc/integrations/terminal/__tests__/CommandTrace.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
🚧 Files skipped from review as they are similar to previous changes (78)
- webview-ui/src/i18n/locales/en/settings.json
- src/core/tools/tests/terminal-provider-fallback.spec.ts
- packages/types/src/terminal.ts
- webview-ui/src/i18n/locales/ru/settings.json
- src/extension.ts
- src/integrations/terminal/tests/CommandEnvironmentService.spec.ts
- docs/260801_0001_session_fork-pr-rebase-ci/234030_code-report.md
- webview-ui/src/i18n/locales/de/settings.json
- packages/types/src/global-settings.ts
- src/extension/api.ts
- webview-ui/src/i18n/locales/es/settings.json
- docs/feedbacks/fromarchitect/260801_missing_webview_build_path.md
- src/integrations/terminal/tests/TerminalProcessExec.cmd.spec.ts
- src/utils/tests/shell.spec.ts
- src/integrations/terminal/tests/TerminalProfile.spec.ts
- webview-ui/src/i18n/locales/tr/settings.json
- src/core/prompts/tests/shell-environment-prompt.spec.ts
- src/integrations/terminal/tests/TerminalProcessExec.bash.spec.ts
- webview-ui/src/i18n/locales/pt-BR/settings.json
- src/core/webview/generateSystemPrompt.ts
- src/core/prompts/tools/native-tools/index.ts
- webview-ui/src/i18n/locales/hi/settings.json
- src/integrations/terminal/tests/ShellInvocationAdapter.spec.ts
- src/integrations/terminal/tests/CommandTrace.spec.ts
- docs/260805_0001_session_ci-all-green/163300_debug-coverage-b05.md
- src/integrations/terminal/tests/TerminalProcessExec.pwsh.spec.ts
- src/core/webview/webviewMessageHandler.ts
- docs/260805_0001_session_ci-all-green/164900_code-report.md
- src/core/task/build-tools.ts
- src/core/prompts/system.ts
- src/integrations/terminal/ExecaTerminalProcess.ts
- webview-ui/src/i18n/locales/pl/settings.json
- src/core/prompts/tools/native-tools/execute_command.ts
- src/integrations/terminal/tests/TerminalRegistry.spec.ts
- src/integrations/terminal/tests/TerminalLifecycle.spec.ts
- webview-ui/src/components/settings/TerminalSettings.tsx
- src/core/tools/tests/executeCommand.spec.ts
- webview-ui/src/i18n/locales/ca/settings.json
- src/integrations/terminal/TerminalProcess.ts
- webview-ui/src/i18n/locales/zh-TW/settings.json
- src/integrations/terminal/tests/ExecaTerminalProcess.spec.ts
- src/integrations/terminal/shell/ShellResolver.ts
- src/core/tools/tests/executeCommandTool.spec.ts
- src/integrations/terminal/tests/ShellResolver.spec.ts
- src/integrations/terminal/shell/types.ts
- docs/260801_0001_session_fork-pr-rebase-ci/224700_code-report.md
- src/integrations/terminal/shell/TerminalProfileResolver.ts
- webview-ui/src/i18n/locales/ja/settings.json
- src/utils/shell.ts
- webview-ui/src/i18n/locales/nl/settings.json
- src/integrations/terminal/TerminalRegistry.ts
- src/integrations/terminal/CommandTrace.ts
- webview-ui/src/i18n/locales/ko/settings.json
- docs/260801_0001_session_fork-pr-rebase-ci/rebase-evidence.md
- src/integrations/terminal/CommandScheduler.ts
- webview-ui/src/i18n/locales/id/settings.json
- apps/vscode-e2e/src/suite/tools/terminal-profile.test.ts
- src/integrations/terminal/shell/ShellInvocationAdapter.ts
- src/integrations/terminal/shell/CommandEnvironmentService.ts
- src/core/prompts/sections/system-info.ts
- webview-ui/src/i18n/locales/zh-CN/settings.json
- webview-ui/src/components/settings/SettingsView.tsx
- src/integrations/terminal/tests/CommandScheduler.spec.ts
- src/core/webview/tests/terminal-shell-messages.spec.ts
- webview-ui/src/i18n/locales/vi/settings.json
- webview-ui/src/components/settings/tests/SettingsView.shell-selection.spec.tsx
- webview-ui/src/i18n/locales/it/settings.json
- src/core/task/Task.ts
- src/integrations/terminal/ExecaTerminal.ts
- src/integrations/terminal/Terminal.ts
- webview-ui/src/components/settings/tests/TerminalSettings.shell.spec.tsx
- packages/types/src/vscode-extension-host.ts
- src/integrations/terminal/types.ts
- src/core/prompts/sections/rules.ts
- src/core/webview/ClineProvider.ts
- src/integrations/terminal/tests/TerminalProcess.spec.ts
- src/integrations/terminal/TerminalLifecycle.ts
- src/integrations/terminal/BaseTerminal.ts
Comment on lines
+453
to
+459
| The assumption that B11 is integrated into B12 must be proven before B12 is pushed. Evidence must show that B12 contains all capability metadata/types and provider detection consumed by its retention policy, with no unresolved symbol or implicit fallback. | ||
|
|
||
| Accepted outcomes: | ||
|
|
||
| 1. B11 content is fully inside B12. Rename the PR dependency section to `B11 capability metadata integrated in this PR` and list exact files/tests. | ||
| 2. B11 content is already in current upstream. Cite the upstream commit and remove B11 as a dependency. | ||
| 3. B11 content is absent. Stop B12/B15/B16 and create a separate architecture decision. Do not silently stub or weaken enforcement. |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Resolve the B11 status before dependent branches proceed.
The architecture report requires explicit proof, while the decision record treats the assumption as approved.
docs/260801_0001_session_fork-pr-rebase-ci/222900_architect-report.md#L453-L459: keep the three accepted proof outcomes as the blocking gate.docs/260801_0001_session_fork-pr-rebase-ci/decisions.md#L7-L7: record the assumption as pending proof, or cite the exact manifest or upstream evidence.
📍 Affects 2 files
docs/260801_0001_session_fork-pr-rebase-ci/222900_architect-report.md#L453-L459(this comment)docs/260801_0001_session_fork-pr-rebase-ci/decisions.md#L7-L7
🤖 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 `@docs/260801_0001_session_fork-pr-rebase-ci/222900_architect-report.md` around
lines 453 - 459, Keep the B11 proof gate enforced in the architecture report and
align the decision record with it: update
docs/260801_0001_session_fork-pr-rebase-ci/222900_architect-report.md at 453-459
to preserve the three accepted outcomes as the blocking check for B12/B15/B16,
and update docs/260801_0001_session_fork-pr-rebase-ci/decisions.md at 7-7 so the
B11 assumption is marked pending proof unless you can cite the exact manifest or
upstream evidence; if B11 is already present, record that specific proof rather
than treating it as approved by default.
Comment on lines
+34
to
+39
| ### Step 5: CI verification (all 4 checks pass) | ||
|
|
||
| 1. `node scripts/find-missing-translations.js` — exit 0, all translations complete | ||
| 2. `pnpm lint` — exit 0, 11/11 tasks successful | ||
| 3. `pnpm check-types` — exit 0, 11/11 tasks successful | ||
| 4. `pnpm knip` — exit 0, no issues |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Separate local verification from GitHub CI status.
Both reports label local command results as CI success without recording SHA-specific GitHub results.
docs/260801_0001_session_fork-pr-rebase-ci/230415_code-report.md#L34-L39: describe the four local commands as CI-equivalent checks and retain the pending GitHub status.docs/260801_0001_session_fork-pr-rebase-ci/232300_code-report.md#L45-L52: distinguish local results from GitHub CI, especially because hooks were bypassed.
📍 Affects 2 files
docs/260801_0001_session_fork-pr-rebase-ci/230415_code-report.md#L34-L39(this comment)docs/260801_0001_session_fork-pr-rebase-ci/232300_code-report.md#L45-L52
🤖 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 `@docs/260801_0001_session_fork-pr-rebase-ci/230415_code-report.md` around
lines 34 - 39, Update the verification sections in
docs/260801_0001_session_fork-pr-rebase-ci/230415_code-report.md lines 34-39 and
docs/260801_0001_session_fork-pr-rebase-ci/232300_code-report.md lines 45-52 to
label the listed commands as local CI-equivalent checks, not GitHub CI results;
retain the pending SHA-specific GitHub status in the first report and explicitly
distinguish local results from GitHub CI in the second, including that hooks
were bypassed.
Comment on lines
+60
to
+62
| it("should reject profile with empty profileName", () => { | ||
| expect(() => terminalShellSelectionSchema.parse({ kind: "profile", profileName: "" })).not.toThrow() // z.string() accepts empty; validation is extension-host responsibility | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the test name or its expectation.
Line 61 accepts an empty profileName, but the test name says it should reject it. Rename the test to state that it accepts an empty value if this behavior is intentional.
Proposed fix
- it("should reject profile with empty profileName", () => {
+ it("should accept profile with empty profileName", () => {📝 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.
Suggested change
| it("should reject profile with empty profileName", () => { | |
| expect(() => terminalShellSelectionSchema.parse({ kind: "profile", profileName: "" })).not.toThrow() // z.string() accepts empty; validation is extension-host responsibility | |
| }) | |
| it("should accept profile with empty profileName", () => { | |
| 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, preserving the existing
non-throwing expectation.
Comment on lines
+361
to
+367
| try { | ||
| const [rejected, result] = await executeCommandInTerminal(task, { | ||
| ...options, | ||
| terminalShellIntegrationDisabled: true, | ||
| useFallbackPlan: !!resolvedEnv, | ||
| reuseTerminal: fallbackTerminal, | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The retry path selects a fallback plan without validating it. Both sites assume that resolvedEnv.fallbackPlan exists and belongs to the same shell family as resolvedEnv.primaryPlan. Neither site checks it. The command text is written for the primary shell syntax and can run under a different shell, or under the legacy shell: true path.
src/core/tools/ExecuteCommandTool.ts#L361-L367: before the retry, compareresolvedEnv.primaryPlan.familywithresolvedEnv.fallbackPlan?.family. When the plan is missing or the families differ, reportShellFallbackMismatchErrorand do not replay the command.src/core/tools/ExecuteCommandTool.ts#L802-L812: whenuseFallbackPlanis true andresolvedEnv.fallbackPlanisundefined, do not fall through silently. Raise the error instead of executing without an invocation plan.
📍 Affects 1 file
src/core/tools/ExecuteCommandTool.ts#L361-L367(this comment)src/core/tools/ExecuteCommandTool.ts#L802-L812
🤖 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 361 - 367, The fallback
execution paths in ExecuteCommandTool must validate fallback plans before
running commands. At src/core/tools/ExecuteCommandTool.ts#L361-L367, compare
resolvedEnv.primaryPlan.family with resolvedEnv.fallbackPlan?.family before
retrying; report ShellFallbackMismatchError and skip replay when the fallback
plan is missing or belongs to another shell family. At
src/core/tools/ExecuteCommandTool.ts#L802-L812, when useFallbackPlan is true and
resolvedEnv.fallbackPlan is undefined, raise the same error instead of executing
without an invocation plan.
Comment on lines
+373
to
+375
| pushToolResult( | ||
| `[Note: VS Code's terminal shell integration was temporarily unavailable — this is a known VS Code infrastructure issue and does not affect command results. The command was automatically retried and completed successfully.]\n\n${result}`, | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The retry note claims success even when the command failed.
executeCommandInTerminal returns a result string for both successful and failed commands. It also returns a result when the command is still running in the background. The note prefixed at Line 374 always states that the command "completed successfully". The model then reads a success claim next to a non-zero exit status.
Report only the fallback event, and let result report the outcome.
🐛 Proposed wording change
pushToolResult(
- `[Note: VS Code's terminal shell integration was temporarily unavailable — this is a known VS Code infrastructure issue and does not affect command results. The command was automatically retried and completed successfully.]\n\n${result}`,
+ `[Note: VS Code's terminal shell integration was temporarily unavailable — this is a known VS Code infrastructure issue and does not affect command results. The command was automatically retried through the fallback terminal; the outcome is reported below.]\n\n${result}`,
)📝 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.
Suggested change
| pushToolResult( | |
| `[Note: VS Code's terminal shell integration was temporarily unavailable — this is a known VS Code infrastructure issue and does not affect command results. The command was automatically retried and completed successfully.]\n\n${result}`, | |
| ) | |
| pushToolResult( | |
| `[Note: VS Code's terminal shell integration was temporarily unavailable — this is a known VS Code infrastructure issue and does not affect command results. The command was automatically retried through the fallback terminal; the outcome is reported below.]\n\n${result}`, | |
| ) |
🤖 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 373 - 375, Update the
retry note in ExecuteCommandTool around pushToolResult so it only reports that
terminal shell integration was unavailable and the command was automatically
retried, without claiming the command completed successfully. Keep the existing
result string appended unchanged so executeCommandInTerminal continues to convey
the actual outcome, including failures or still-running commands.
Comment on lines
+643
to
+657
| const scheduleCommandOutputAsk = (process: RooTerminalProcess): void => { | ||
| if (runInBackground || hasAskedForCommandOutput || completed || commandOutputAskTimer) { | ||
| return | ||
| } | ||
|
|
||
| const remainingDelay = COMMAND_OUTPUT_ASK_DELAY_MS - (Date.now() - commandStartedAt) | ||
|
|
||
| commandOutputAskTimer = setTimeout( | ||
| () => { | ||
| commandOutputAskTimer = undefined | ||
| void askForCommandOutput(process) | ||
| }, | ||
| Math.max(remainingDelay, 0), | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A silent long-running command can never trigger the command_output ask.
scheduleCommandOutputAsk is invoked only from onLine at Line 679 and from onShellExecutionStarted at Line 743, and the second call reschedules only when a timer already exists. A command that produces no output therefore never arms the timer. The user then has no way to background or interrupt it.
The two timeouts do not cover this. commandExecutionTimeout defaults to 0 at Line 261, and agentTimeout is 0 when the model sends no timeout and in CLI runtime. With both at 0, Promise.race at Line 862 waits for process completion with no bound.
Arm the timer once at execution start, in addition to the output-driven path.
🐛 Proposed fix
onShellExecutionStarted: (pid: number | undefined, process: RooTerminalProcess) => {
const now = Date.now()
traceBuilder?.markProcessIdResolvedAt(now)
traceBuilder?.markShellExecutionStartedAt(now)
const status: CommandExecutionStatus = { executionId, status: "started", pid, command }
provider?.postMessageToWebview({ type: "commandExecutionStatus", text: JSON.stringify(status) })
// Re-anchor the ask delay to actual execution start so the shell
// integration startup wait does not count against the grace period.
commandStartedAt = Date.now()
- // Output should not precede this event, but if it did, reschedule
- // the pending ask against the corrected anchor.
- if (commandOutputAskTimer) {
- clearTimeout(commandOutputAskTimer)
- commandOutputAskTimer = undefined
- scheduleCommandOutputAsk(process)
- }
+ // Re-arm against the corrected anchor. This also covers commands that
+ // never emit output, which would otherwise never arm the timer.
+ clearTimeout(commandOutputAskTimer)
+ commandOutputAskTimer = undefined
+ scheduleCommandOutputAsk(process)
},🤖 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 643 - 657, Arm the
command-output ask timer immediately when execution starts, in addition to the
existing output-driven scheduling. Update the execution-start flow around
onShellExecutionStarted (or the surrounding command initialization) to invoke
scheduleCommandOutputAsk once before waiting for output, while preserving its
guards against background runs, completed commands, prior asks, and existing
timers.
Comment on lines
+853
to
+869
| "inlineShell": { | ||
| "label": "Inline Terminal Shell", | ||
| "description": "Select the shell used for inline terminal command execution. Auto follows your trusted VS Code terminal profile. Custom paths are validated by the extension host.", | ||
| "auto": "Auto (follows trusted terminal profile)", | ||
| "customPath": "Choose custom executable", | ||
| "customPathPlaceholder": "Select a shell executable...", | ||
| "effectiveShell": { | ||
| "label": "Effective shell", | ||
| "family": "Family", | ||
| "source": "Source", | ||
| "fallback": "Fallback behavior", | ||
| "fallbackDescription": "If shell integration fails, commands retry using the same shell family." | ||
| }, | ||
| "error": { | ||
| "invalid": "The selected shell is not supported. Choose a trusted profile or a valid shell executable.", | ||
| "unavailable": "Shell options are currently unavailable. The extension host may still be initializing." | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Translate the new French locale values.
Lines 853-869 copy the English messages. French users will see English text for the new shell-selection controls and errors. Translate each value while preserving the existing keys and placeholders.
🤖 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/fr/settings.json` around lines 853 - 869,
Translate every English value under the inlineShell object in the French
settings locale, including labels, descriptions, option text, placeholders,
effectiveShell fields, and error messages. Preserve all existing keys and
placeholders while ensuring French users no longer see English text.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/eslint-suppressions.json`:
- Around line 987-989: Reduce the `@typescript-eslint/no-explicit-any` usages in
the affected test files, including executeCommandTool.spec.ts,
ExecaTerminalProcess.spec.ts, TerminalProfile.spec.ts, and
TerminalRegistry.spec.ts, by using typed test doubles or unknown with
appropriate type guards. Remove the newly added suppression entries, then
regenerate eslint-suppressions.json with ESLint pruning so existing counts
decrease where these local fixes apply and no suppression count increases.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a7088ada-d316-470f-a3be-0697099f466c
📒 Files selected for processing (2)
src/core/tools/ExecuteCommandTool.tssrc/eslint-suppressions.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/tools/ExecuteCommandTool.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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