feat(agent): support configurable Windows command shells - #2109
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds configurable Windows command-shell support across shell resolution, prompts, permissions, deferred execution, paths, skills, process spawning, settings, synchronization, localization, and tests. Shell profiles are validated and propagated per turn. ChangesWindows command-shell support
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/agent/deepchat/runtime/deferredToolExecutor.ts (1)
326-363: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRequire the shell profile when a one-shot command grant is supplied.
oneShotCommandGrantIdis forwarded at line 432 even whencommandShellProfileisundefined. In that case line 360 resolves the current turn shell, which can differ from the shell used to build the grant signature. The grant then does not describe the shell that executes the command.The caller in
interactionCoordinator.tsvalidates the profile before it mints the grant, so this pairing holds today. Enforce it here so the contract does not depend on caller discipline.🛡️ Proposed guard
if ( !parsedCommandShellProfile && - targetServerName === 'agent-filesystem' + (targetServerName === 'agent-filesystem' || oneShotCommandGrantId) ) { return { - responseText: 'Deferred file execution is missing its shell profile.', + responseText: 'Deferred execution is missing its shell profile.', isError: true, invoked } }🤖 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/main/agent/deepchat/runtime/deferredToolExecutor.ts` around lines 326 - 363, Require a non-empty parsedCommandShellProfile whenever a oneShotCommandGrantId is supplied, before resolving the command shell in the deferred execution flow. Reject the request with the existing error response pattern if the grant lacks its matching shell profile, while preserving the current resolveForTurn behavior for executions without a one-shot grant.
🧹 Nitpick comments (12)
test/main/cli/agentCommandAccess.test.ts (1)
103-141: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd CMD token-expansion coverage.
Add a test with
%deepchat_cli_agent_token%andCMD_COMMAND_SHELL. Assert thatcreateEnvironmentreturns the unprivileged environment and that the authority issues no token. The current CMD test only verifies caret control syntax. It does not verify the case-insensitive token branch for CMD.As per coding guidelines, “Add the smallest regression test for user-visible behavior or a documented contract.”
🤖 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 `@test/main/cli/agentCommandAccess.test.ts` around lines 103 - 141, Add a focused CMD test alongside the existing caret-syntax case using `%deepchat_cli_agent_token%` in the command. Assert that `AgentCliCommandAccess.createEnvironment` returns the unprivileged environment with an empty local token and that `AgentCliTokenAuthority.snapshot()` reports no issued tokens or conversations, covering CMD’s case-insensitive token-expansion path.Source: Coding guidelines
src/shared/commandShell.ts (1)
107-110: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider preserving
preferencewhen only the override is invalid.
normalizeAgentCommandShellConfigdiscards the complete config on any parse failure. A corruptedgitBashExecutableOverridetherefore resets an explicitgit-bashpreference toauto. That silently downgrades the selected profile, whiledocs/features/windows-command-shell/plan.mdLine 105 states that explicit profile resolution must not silently downgrade. The route boundary rejects invalid writes, so this only affects corrupted stored settings.♻️ Optional refinement
export function normalizeAgentCommandShellConfig(value: unknown): AgentCommandShellConfig { const parsed = AgentCommandShellConfigSchema.safeParse(value) - return parsed.success ? parsed.data : DEFAULT_AGENT_COMMAND_SHELL_CONFIG + if (parsed.success) return parsed.data + const preference = AgentCommandShellPreferenceSchema.safeParse( + (value as { preference?: unknown } | null | undefined)?.preference + ) + return preference.success + ? { preference: preference.data } + : DEFAULT_AGENT_COMMAND_SHELL_CONFIG }🤖 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/shared/commandShell.ts` around lines 107 - 110, Update normalizeAgentCommandShellConfig so an invalid gitBashExecutableOverride falls back only to the default override while preserving a valid explicit preference. Keep valid configurations unchanged, retain auto as the preference only when it is absent or invalid, and ensure corrupted stored settings do not silently downgrade a valid git-bash profile.src/shared/types/core/agent-events.ts (1)
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInline
import(...)type references forCommandShellProfilein both shared event type files. Both files reference the newCommandShellProfiletype through an inlineimport('../../commandShell')expression instead of a top-levelimport type. Both files already use top-level imports for their other types.
src/shared/types/core/agent-events.ts#L42-L42: addimport type { CommandShellProfile } from '../../commandShell'at the top of the file and change the field toshellProfile?: CommandShellProfile.src/shared/types/core/llm-events.ts#L332-L332: add the same top-levelimport typeand change the field toshellProfile?: CommandShellProfile.🤖 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/shared/types/core/agent-events.ts` at line 42, Replace the inline CommandShellProfile type references with a top-level type import in both affected files: add import type { CommandShellProfile } from '../../commandShell' in src/shared/types/core/agent-events.ts (line 42) and src/shared/types/core/llm-events.ts (line 332), then use CommandShellProfile for each shellProfile field.src/main/agent/deepchat/runtime/dispatch.ts (1)
1186-1188: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the parsed value instead of casting the raw input.
safeParsealready produces a typed value. Readingrequest?.shellProfileagain and casting it removes the guarantee the parse provides.♻️ Proposed refactor
+ const parsedShellProfile = CommandShellProfileSchema.safeParse(request?.shellProfile) ... - shellProfile: CommandShellProfileSchema.safeParse(request?.shellProfile).success - ? (request?.shellProfile as CommandShellProfile) - : undefined, + shellProfile: parsedShellProfile.success ? parsedShellProfile.data : undefined,🤖 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/main/agent/deepchat/runtime/dispatch.ts` around lines 1186 - 1188, Update the shellProfile assignment to retain and use the typed value returned by CommandShellProfileSchema.safeParse instead of casting request?.shellProfile. Preserve the undefined result when parsing fails, while ensuring successful parsing passes the parsed data onward.test/main/tool/agentTools/agentToolManagerRead.test.ts (1)
161-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the validation error message for the malformed shell spec.
rejects.toThrow()accepts any error. A future unrelated failure, for example a missing mock, would keep this test green. The sibling test at Lines 142-147 asserts the message text. Use the same approach here so the test pins the shell-validation contract.♻️ Proposed change to assert the validation message
await expect( callToolWithoutCommandShell('read', { path: 'note.txt' }, 'conv1', { commandShell: malformedCommandShell as never }) - ).rejects.toThrow() + ).rejects.toThrow(/command shell/i) await expect( preCheckWithoutCommandShell('read', { path: 'note.txt' }, 'conv1', { commandShell: malformedCommandShell as never }) - ).rejects.toThrow() + ).rejects.toThrow(/command shell/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 `@test/main/tool/agentTools/agentToolManagerRead.test.ts` around lines 161 - 170, Update the rejection assertions for callToolWithoutCommandShell and preCheckWithoutCommandShell to verify the specific malformed shell validation error message, matching the sibling test’s assertion near lines 142-147. Keep both cases covered while replacing broad rejects.toThrow() checks with message-specific expectations.test/main/tool/agentTools/agentBashHandler.test.ts (1)
242-246: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a typed cast instead of
as neverto reach the private method.
(handler as never).prepareCommand(...)relies onneveraccepting any property access. A cast that names the method keeps the test readable and fails to compile if the signature changes.♻️ Proposed change
- const prepared = await (handler as never).prepareCommand( - 'Get-ChildItem', - {}, - WINDOWS_POWERSHELL_COMMAND_SHELL - ) + const prepared = await ( + handler as unknown as { + prepareCommand: ( + command: string, + env: Record<string, string>, + commandShell: typeof WINDOWS_POWERSHELL_COMMAND_SHELL + ) => Promise<{ command: string; rtkFallbackReason?: string }> + } + ).prepareCommand('Get-ChildItem', {}, WINDOWS_POWERSHELL_COMMAND_SHELL)🤖 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 `@test/main/tool/agentTools/agentBashHandler.test.ts` around lines 242 - 246, Replace the `(handler as never)` cast in the test call to `prepareCommand` with a typed cast that explicitly exposes the private `prepareCommand` method and its signature, preserving the existing invocation and ensuring signature changes are caught at compile time.test/main/app/sessionPermissionAdapter.test.ts (2)
108-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the file and settings approval branches.
The current non-command test only covers the broker path at Line 73 of
src/main/app/sessionPermissionAdapter.ts. Theagent-filesystembranch and thedeepchat-settingsbranch are untested. A test for awriteapproval withpathswould assert thatfilePermissionService.approvereceives the session ID, the paths, andfalse. That case is the main non-command path used by deferred file approvals.🤖 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 `@test/main/app/sessionPermissionAdapter.test.ts` around lines 108 - 118, Extend the non-command approval tests around port.approvePermission to cover the agent-filesystem and deepchat-settings branches. Add a write approval with paths and assert filePermissionService.approve receives the session ID, paths, and false, while also adding coverage for the settings approval branch.
40-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
vi.resetAllMocks()for shared module-level mocks.
vi.clearAllMocks()clears call history but keeps implementations, including any queuedmockReturnValueOncevalues that a test did not consume. The mocks at Lines 6-29 are shared by every test in this file. An unconsumed queued value would leak into the next test. Usevi.resetAllMocks()to remove queued implementations as well.♻️ Proposed change
beforeEach(() => { - vi.clearAllMocks() + vi.resetAllMocks() })🤖 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 `@test/main/app/sessionPermissionAdapter.test.ts` around lines 40 - 42, Replace vi.clearAllMocks() in the shared beforeEach setup with vi.resetAllMocks() so module-level mocks and any queued mockReturnValueOnce implementations are reset between tests.src/main/tool/permission/commandPermissionService.ts (1)
191-221: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
hasCmdControlSyntaxnever resets the expansion flags.
pendingPercentExpansionandpendingDelayedExpansionare set on the first occurrence and are never cleared. A command such asecho 50% off 20%reports control syntax. The result is fail-closed, so it only forces an extra approval prompt for CMD commands that contain two literal%or!characters. Consider pairing the markers per token if the extra prompts prove noisy.🤖 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/main/tool/permission/commandPermissionService.ts` around lines 191 - 221, Update hasCmdControlSyntax so pendingPercentExpansion and pendingDelayedExpansion are cleared when their corresponding expansion marker is not immediately paired, preventing separate literal % or ! characters in the same command from being treated as expansions. Preserve detection of genuinely paired markers and existing control-character handling.test/main/agent/shared/process/rtkRuntimeService.test.ts (1)
1-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReset the
spawnmock between tests.The last test asserts
expect(spawn).toHaveBeenCalledTimes(2). That assertion depends on no earlier test callingspawn. Today the other tests injectrunCommand, so the count stays clean. A future test that reaches the real spawn path would make this assertion fail for an unrelated reason.Add a
beforeEachthat callsvi.mocked(spawn).mockReset().🤖 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 `@test/main/agent/shared/process/rtkRuntimeService.test.ts` around lines 1 - 11, Reset the child-process mock before each test by importing beforeEach and adding a beforeEach hook that calls vi.mocked(spawn).mockReset(). Keep the existing spawn mock and test behavior unchanged.src/main/tool/agentTools/agentToolManager.ts (1)
2386-2396: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant second
requireCommandShellcall.Line 2353 already validates
options.commandShelland assigns the result tocommandShell. Line 2395 validates that same validated value again. UsecommandShelldirectly.♻️ Proposed refactor
- const requiredCommandShell = this.requireCommandShell(commandShell)Then replace the later
requiredCommandShellreferences withcommandShell.🤖 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/main/tool/agentTools/agentToolManager.ts` around lines 2386 - 2396, Remove the redundant requireCommandShell call in the exec branch of the agent tool manager. Reuse the already validated commandShell value from the earlier options.commandShell validation, replacing all subsequent requiredCommandShell references while preserving the existing execution behavior.src/main/skill/skillExecutionService.ts (1)
702-731: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the duplicate
cmdandpowershellbranches.Both branches perform the same work. Only the error message differs. A single branch with a dialect-to-label lookup removes the duplication.
♻️ Proposed refactor
- if (commandShell.dialect === 'cmd') { - if (plan.spawnMode === 'shell') { - throw new Error('Skill shell execution is unavailable under Command Prompt') - } - return { - ...plan, - shellCommand: undefined, - env: await rtkRuntimeService.prepareExecutionEnv(plan.env) - } - } - - if (commandShell.dialect === 'powershell') { - if (plan.spawnMode === 'shell') { - throw new Error('Skill shell execution is unavailable under Windows PowerShell') - } - return { - ...plan, - shellCommand: undefined, - env: await rtkRuntimeService.prepareExecutionEnv(plan.env) - } - } + const directOnlyShellLabels: Partial<Record<CommandShellDialect, string>> = { + cmd: 'Command Prompt', + powershell: 'Windows PowerShell' + } + const directOnlyLabel = directOnlyShellLabels[commandShell.dialect] + if (directOnlyLabel) { + if (plan.spawnMode === 'shell') { + throw new Error(`Skill shell execution is unavailable under ${directOnlyLabel}`) + } + return { + ...plan, + shellCommand: undefined, + env: await rtkRuntimeService.prepareExecutionEnv(plan.env) + } + }🤖 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/main/skill/skillExecutionService.ts` around lines 702 - 731, Merge the duplicate dialect handling in preparePlanForExecution into one branch for cmd and powershell, using a dialect-to-label lookup to produce the dialect-specific error message. Preserve the existing shell-mode rejection and returned plan behavior, including shellCommand removal and environment preparation.
🤖 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/main/agent/deepchat/harness/createDeepChatAgentHarness.ts`:
- Around line 485-492: Update the ACP prompt assembly flow around
buildSystemPrompt to resolve the turn-scoped command shell once and reuse that
same resolved shell for every prompt build, rather than calling
commandShell.resolveForTurn() on each invocation. Preserve the shell instance
through refreshed prompts and subsequent tool execution so prompt guidance
remains consistent with the executing shell.
In `@src/main/agent/deepchat/runtime/dispatch.ts`:
- Around line 1242-1246: Protect the revokeOneShotCommandPermission call in the
finally block so failures cannot replace the value or error produced by run.
Catch revocation errors and log them through the existing logging mechanism,
while preserving the current grant?.kind === 'command' guard and one-shot
permission arguments.
In `@src/main/tool/index.ts`:
- Around line 398-399: Resolve a concrete default command shell before dispatch
in callFileSystemTool and preCheckToolPermission, then pass that resolved value
to AgentBashHandler.executeCommand and AgentFileSystemHandler instead of the
optional commandShell. Preserve explicitly supplied shells while ensuring
omitted values always provide the required commandShellPathStyle.
In `@src/renderer/settings/components/common/CommandShellSettingsSection.vue`:
- Around line 261-282: Update the interaction between saveOverride and
updatePreference so moving focus by keyboard from a dirty executable input to
the shell selector does not leave the selector unusable; either suppress the
blur save during selector focus transitions or serialize the override and
preference into one update. Add the smallest focused keyboard regression test
covering this user-visible behavior.
In `@test/main/agent/deepchat/loop/loopRun.test.ts`:
- Around line 30-48: Add a separate assertion in the createLoopRun test that
uses an invalid resources fixture with commandShell omitted, and verify it
throws. Keep the existing contradictory-shell assertion unchanged so both
missing and inconsistent command shell contracts are covered.
In `@test/main/agent/deepchat/resources/systemPromptBuilder.test.ts`:
- Around line 12-18: Update the test around buildSystemPromptWithSkills to
provide an observable optional prompt contributor, such as a mocked contributor
that records invocation, while retaining the invalid commandShell input. Assert
both that validation rejects and that the contributor was not called, proving
validation occurs before optional contributors execute.
In `@test/main/agent/shared/process/backgroundExecSessionManager.test.ts`:
- Around line 548-562: Update the parameterized test around manager.start to pin
process.platform to a non-Windows value before asserting missing or
contradictory command shells are rejected, ensuring the
WINDOWS_POWERSHELL_COMMAND_SHELL case remains invalid on every runner.
---
Outside diff comments:
In `@src/main/agent/deepchat/runtime/deferredToolExecutor.ts`:
- Around line 326-363: Require a non-empty parsedCommandShellProfile whenever a
oneShotCommandGrantId is supplied, before resolving the command shell in the
deferred execution flow. Reject the request with the existing error response
pattern if the grant lacks its matching shell profile, while preserving the
current resolveForTurn behavior for executions without a one-shot grant.
---
Nitpick comments:
In `@src/main/agent/deepchat/runtime/dispatch.ts`:
- Around line 1186-1188: Update the shellProfile assignment to retain and use
the typed value returned by CommandShellProfileSchema.safeParse instead of
casting request?.shellProfile. Preserve the undefined result when parsing fails,
while ensuring successful parsing passes the parsed data onward.
In `@src/main/skill/skillExecutionService.ts`:
- Around line 702-731: Merge the duplicate dialect handling in
preparePlanForExecution into one branch for cmd and powershell, using a
dialect-to-label lookup to produce the dialect-specific error message. Preserve
the existing shell-mode rejection and returned plan behavior, including
shellCommand removal and environment preparation.
In `@src/main/tool/agentTools/agentToolManager.ts`:
- Around line 2386-2396: Remove the redundant requireCommandShell call in the
exec branch of the agent tool manager. Reuse the already validated commandShell
value from the earlier options.commandShell validation, replacing all subsequent
requiredCommandShell references while preserving the existing execution
behavior.
In `@src/main/tool/permission/commandPermissionService.ts`:
- Around line 191-221: Update hasCmdControlSyntax so pendingPercentExpansion and
pendingDelayedExpansion are cleared when their corresponding expansion marker is
not immediately paired, preventing separate literal % or ! characters in the
same command from being treated as expansions. Preserve detection of genuinely
paired markers and existing control-character handling.
In `@src/shared/commandShell.ts`:
- Around line 107-110: Update normalizeAgentCommandShellConfig so an invalid
gitBashExecutableOverride falls back only to the default override while
preserving a valid explicit preference. Keep valid configurations unchanged,
retain auto as the preference only when it is absent or invalid, and ensure
corrupted stored settings do not silently downgrade a valid git-bash profile.
In `@src/shared/types/core/agent-events.ts`:
- Line 42: Replace the inline CommandShellProfile type references with a
top-level type import in both affected files: add import type {
CommandShellProfile } from '../../commandShell' in
src/shared/types/core/agent-events.ts (line 42) and
src/shared/types/core/llm-events.ts (line 332), then use CommandShellProfile for
each shellProfile field.
In `@test/main/agent/shared/process/rtkRuntimeService.test.ts`:
- Around line 1-11: Reset the child-process mock before each test by importing
beforeEach and adding a beforeEach hook that calls vi.mocked(spawn).mockReset().
Keep the existing spawn mock and test behavior unchanged.
In `@test/main/app/sessionPermissionAdapter.test.ts`:
- Around line 108-118: Extend the non-command approval tests around
port.approvePermission to cover the agent-filesystem and deepchat-settings
branches. Add a write approval with paths and assert
filePermissionService.approve receives the session ID, paths, and false, while
also adding coverage for the settings approval branch.
- Around line 40-42: Replace vi.clearAllMocks() in the shared beforeEach setup
with vi.resetAllMocks() so module-level mocks and any queued mockReturnValueOnce
implementations are reset between tests.
In `@test/main/cli/agentCommandAccess.test.ts`:
- Around line 103-141: Add a focused CMD test alongside the existing
caret-syntax case using `%deepchat_cli_agent_token%` in the command. Assert that
`AgentCliCommandAccess.createEnvironment` returns the unprivileged environment
with an empty local token and that `AgentCliTokenAuthority.snapshot()` reports
no issued tokens or conversations, covering CMD’s case-insensitive
token-expansion path.
In `@test/main/tool/agentTools/agentBashHandler.test.ts`:
- Around line 242-246: Replace the `(handler as never)` cast in the test call to
`prepareCommand` with a typed cast that explicitly exposes the private
`prepareCommand` method and its signature, preserving the existing invocation
and ensuring signature changes are caught at compile time.
In `@test/main/tool/agentTools/agentToolManagerRead.test.ts`:
- Around line 161-170: Update the rejection assertions for
callToolWithoutCommandShell and preCheckWithoutCommandShell to verify the
specific malformed shell validation error message, matching the sibling test’s
assertion near lines 142-147. Keep both cases covered while replacing broad
rejects.toThrow() checks with message-specific expectations.
🪄 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: 0ee60492-cd5c-4346-994a-c5011e9ba533
📒 Files selected for processing (117)
docs/features/windows-command-shell/plan.mddocs/features/windows-command-shell/spec.mddocs/features/windows-command-shell/tasks.mdscripts/build-cli.mjssrc/main/agent/deepchat/harness/createDeepChatAgentHarness.tssrc/main/agent/deepchat/harness/runtimeServices.tssrc/main/agent/deepchat/loop/loopRun.tssrc/main/agent/deepchat/loop/ports.tssrc/main/agent/deepchat/resources/systemEnvPromptBuilder.tssrc/main/agent/deepchat/resources/systemPromptBuilder.tssrc/main/agent/deepchat/runtime/compactionRuntimeCoordinator.tssrc/main/agent/deepchat/runtime/deepChatLoopRunner.tssrc/main/agent/deepchat/runtime/deferredToolExecutor.tssrc/main/agent/deepchat/runtime/dispatch.tssrc/main/agent/deepchat/runtime/interactionCoordinator.tssrc/main/agent/deepchat/runtime/process.tssrc/main/agent/deepchat/runtime/promptAssemblyService.tssrc/main/agent/deepchat/runtime/turnCoordinator.tssrc/main/agent/deepchat/runtime/types.tssrc/main/agent/shared/process/backgroundExecSessionManager.tssrc/main/agent/shared/process/commandShellPath.tssrc/main/agent/shared/process/commandShellService.tssrc/main/agent/shared/process/processTree.tssrc/main/agent/shared/process/rtkRuntimeService.tssrc/main/agent/shared/process/shellOutputEncoding.tssrc/main/app/composition.tssrc/main/app/sessionPermissionAdapter.tssrc/main/app/settingsRoutes.tssrc/main/cli/agentCommandAccess.tssrc/main/config/settingsStore.tssrc/main/remote/conversation/interaction.tssrc/main/remote/types.tssrc/main/session/contracts.tssrc/main/skill/skillExecutionService.tssrc/main/sync/index.tssrc/main/tool/agentTools/agentBashHandler.tssrc/main/tool/agentTools/agentFffSearchHandler.tssrc/main/tool/agentTools/agentFileSystemHandler.tssrc/main/tool/agentTools/agentToolManager.tssrc/main/tool/index.tssrc/main/tool/permission/commandPermissionCache.tssrc/main/tool/permission/commandPermissionService.tssrc/main/tool/permission/index.tssrc/renderer/api/SettingsClient.tssrc/renderer/settings/components/CommonSettings.vuesrc/renderer/settings/components/common/CommandShellSettingsSection.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/de-DE/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/es-ES/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/id-ID/settings.jsonsrc/renderer/src/i18n/it-IT/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/ms-MY/settings.jsonsrc/renderer/src/i18n/pl-PL/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/tr-TR/settings.jsonsrc/renderer/src/i18n/vi-VN/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/shared/commandShell.tssrc/shared/contracts/events.tssrc/shared/contracts/events/settings.events.tssrc/shared/contracts/routes.tssrc/shared/contracts/routes/settings.routes.tssrc/shared/types/core/agent-events.tssrc/shared/types/core/llm-events.tssrc/shared/types/core/mcp.tssrc/shared/types/mcp.tssrc/shared/types/tool.d.tstest/helpers/commandShell.tstest/main/agent/deepchat/harness/deepChatAgentHarness.test.tstest/main/agent/deepchat/instance/deepChatAgentRuntime.test.tstest/main/agent/deepchat/loop/contextCoordinator.test.tstest/main/agent/deepchat/loop/deepChatLoopEngine.test.tstest/main/agent/deepchat/loop/loopRun.test.tstest/main/agent/deepchat/resources/systemEnvPromptBuilder.test.tstest/main/agent/deepchat/resources/systemPromptBuilder.test.tstest/main/agent/deepchat/runtime/compactionRuntimeCoordinator.test.tstest/main/agent/deepchat/runtime/deferredToolExecutor.test.tstest/main/agent/deepchat/runtime/dispatch.test.tstest/main/agent/deepchat/runtime/messageProjectionService.test.tstest/main/agent/deepchat/runtime/process.test.tstest/main/agent/deepchat/runtime/promptAssemblyService.test.tstest/main/agent/deepchat/runtime/runLifecycleCoordinator.test.tstest/main/agent/shared/process/backgroundExecSessionManager.test.tstest/main/agent/shared/process/commandShellPath.test.tstest/main/agent/shared/process/commandShellService.test.tstest/main/agent/shared/process/processTree.test.tstest/main/agent/shared/process/rtkRuntimeService.test.tstest/main/agent/shared/process/shellOutputEncoding.test.tstest/main/app/sessionPermissionAdapter.test.tstest/main/cli/agentCommandAccess.test.tstest/main/evals/nativeAgent/harness.tstest/main/routes/contracts.test.tstest/main/routes/dispatcher.test.tstest/main/scripts/buildCli.test.tstest/main/session/runtimeIntegration.test.tstest/main/session/session.integration.test.tstest/main/skill/skillExecutionService.test.tstest/main/sync/syncService.test.tstest/main/tool/agentTools/agentBashHandler.test.tstest/main/tool/agentTools/agentBashHandlerEncoding.test.tstest/main/tool/agentTools/agentFffSearchHandler.test.tstest/main/tool/agentTools/agentFileSystemHandler.test.tstest/main/tool/agentTools/agentToolManagerFffSearch.test.tstest/main/tool/agentTools/agentToolManagerRead.test.tstest/main/tool/agentTools/agentToolManagerSkillAccess.test.tstest/main/tool/permission/commandPermissionService.test.tstest/renderer/api/clients.test.tstest/renderer/components/CommandShellSettingsSection.test.ts
zerob13
requested changes
Aug 9, 2026
Collaborator
There was a problem hiding this comment.
Update: I additionally validated the feature locally on Windows. Switching between Git Bash and Windows PowerShell works correctly, so the functional path is good. I am withdrawing the packaged-Windows/manual-validation concern from this review. The design direction and user-facing behavior are sound.
The remaining change request is limited to test portability.
On Windows at 36921f239ede33c70b43fccfd52e455819454862, I ran all 37 main-process test files changed by this PR. The result was 1131 passed, 10 failed, and 1 skipped. Six failures reproduce unchanged on dev, so I am not attributing those to this PR. Four failures are introduced here:
backgroundExecSessionManager.test.ts: the env merge, output spooling, and split UTF-8 tests now passPOSIX_COMMAND_SHELL, which the manager correctly rejects onwin32.agentFileSystemHandler.test.ts: the new POSIX case-sensitivity test runs against Windows containment semantics and expects the wrong result.
Please make those fixtures platform-valid: pin a non-Windows platform for tests that specifically assert POSIX semantics, or use the Windows shell/path fixture where the behavior is platform-independent. No additional test cases are needed. Once these four fixtures pass on Windows, I have no remaining blocker from this review.
zerob13
self-requested a review
August 10, 2026 01:23
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
2 participants
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.
Summary
Add an explicit Windows command-shell contract for Agent execution, with support for:
Auto, preserving the existing Windows PowerShell/CMD selection behaviorA single immutable
ResolvedCommandShellis now resolved for each turn and propagated through prompt generation, permission analysis, filesystem path handling, skills, deferred execution, RPC, and process spawning.What Changed
Command shell resolution
Autobehavior without implicitly selecting PowerShell 7.where git.Settings UI
Add Windows-only command-shell settings with:
UI layout
Before
After
Prompt and execution consistency
Permission safety
Git Bash path handling
/c/workspace/file.txt.Persistence and compatibility
agentCommandShelldevice-local.Auto.Summary by CodeRabbit
New Features
Bug Fixes
Documentation