Fix deprecated terminal sandbox setting fallback#313590
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates terminal sandbox enablement detection and deprecated-setting fallback logic to avoid accidentally treating chat.agent.sandbox namespace objects (from child settings) as the deprecated boolean setting, while also introducing a more expressive sandbox enablement mode to support “allow network” behavior.
Changes:
- Tighten deprecated-setting fallback: only fall back when the exact deprecated user setting key is explicitly present, avoiding namespace conflicts.
- Introduce
TerminalSandboxEnablement(+isTerminalSandboxEnabled) and thread it through terminal chat tooling and network filtering. - Expand configuration + add regression tests for namespace-conflict fallback and “allow network” behavior.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/common/terminalSandboxService.ts | Implements exact-key deprecated fallback check; adds allow-network behavior to skip domain checks and adjust config merging. |
| src/vs/platform/sandbox/common/terminalSandboxService.ts | Changes ITerminalSandboxService.isEnabled() to return TerminalSandboxEnablement and adds isTerminalSandboxEnabled. |
| src/vs/platform/sandbox/common/settings.ts | Adds AgentSandboxEnabledValue.AllowNetwork. |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/common/terminalChatAgentToolsConfiguration.ts | Exposes allowNetwork in the chat.agent.sandbox.enabled setting schema + localization metadata. |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts | Adapts tool data and telemetry metadata handling to the new enablement enum. |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/commandLinePresenter/sandboxedCommandLinePresenter.ts | Uses isTerminalSandboxEnabled to interpret sandbox enablement. |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/commandLineAnalyzer/commandLineSandboxAnalyzer.ts | Uses isTerminalSandboxEnabled to interpret sandbox enablement. |
| src/vs/platform/networkFilter/common/networkFilterService.ts | Updates filtering activation to depend on TerminalSandboxEnablement.On. |
| src/vs/platform/networkFilter/test/common/networkFilterService.test.ts | Adds coverage for NetworkAllowed not activating filtering when networkFilter is disabled. |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/terminalSandboxService.test.ts | Adds regression tests for namespace conflict fallback + allow-network mode behavior. |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/electron-browser/runInTerminalTool.test.ts | Updates sandbox stubs to return TerminalSandboxEnablement. |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/electron-browser/commandLineSandboxRewriter.test.ts | Updates sandbox stubs to return TerminalSandboxEnablement. |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/sandboxedCommandLinePresenter.test.ts | Updates sandbox stubs to return TerminalSandboxEnablement. |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/commandLineSandboxAnalyzer.test.ts | Updates sandbox stubs to return TerminalSandboxEnablement. |
Copilot's findings
- Files reviewed: 14/14 changed files
- Comments generated: 4
| const sandboxSettings = { | ||
| network: { | ||
| allowedDomains: allowedDomainsSetting, | ||
| deniedDomains: deniedDomainsSetting | ||
| }, | ||
| network: allowNetwork ? { enabled: false, allowedDomains: [], deniedDomains: [] } : this.getResolvedNetworkDomains(), | ||
| filesystem: { | ||
| denyRead: denyReadPaths, | ||
| allowRead: allowReadPaths, | ||
| allowWrite: allowWritePaths, | ||
| denyWrite: denyWritePaths, | ||
| }, | ||
| }; | ||
| this._mergeAdditionalSandboxConfigProperties(sandboxSettings as Record<string, unknown>, runtimeSetting); | ||
| this._mergeAdditionalSandboxConfigProperties(sandboxSettings as Record<string, unknown>, allowNetwork ? this._withoutNetworkRuntimeSetting(runtimeSetting) : runtimeSetting); |
There was a problem hiding this comment.
When allowNetwork is enabled, the config currently writes network: { enabled: false, allowedDomains: [], deniedDomains: [] }, but the new regression test expects network to be exactly { enabled: false } and (more importantly) keeping allowedDomains/deniedDomains in the file can still be interpreted as an active domain config depending on the sandbox runtime. Consider emitting only { enabled: false } when allowNetwork is set (and ensure runtime merge can’t reintroduce domain lists).
| export interface ITerminalSandboxService { | ||
| readonly _serviceBrand: undefined; | ||
| isEnabled(): Promise<boolean>; | ||
| isEnabled(): Promise<TerminalSandboxEnablement>; |
There was a problem hiding this comment.
ITerminalSandboxService.isEnabled() now returns a string enum. In TypeScript it’s easy to accidentally write if (await sandboxService.isEnabled()) { … }, which will always be truthy for all enum values (including Off = 'off'). To avoid subtle bugs, consider keeping isEnabled(): Promise<boolean> and adding a separate getEnablement() API, or renaming this method to something like getEnablement() so misuse in boolean contexts is less likely.
| isEnabled(): Promise<TerminalSandboxEnablement>; | |
| isEnabled(): Promise<boolean>; | |
| getEnablement(): Promise<TerminalSandboxEnablement>; |
| * | ||
| * Filtering is active when the `chat.agent.networkFilter` setting is enabled, | ||
| * or when the terminal sandbox service reports that sandboxing is enabled. | ||
| * or when the terminal sandbox service reports sandboxing is on. |
There was a problem hiding this comment.
The doc comment says filtering is active when the terminal sandbox service reports “sandboxing is on”, but the implementation only enables filtering when enablement is exactly TerminalSandboxEnablement.On (and not NetworkAllowed). Consider clarifying the wording to match the behavior (e.g. filtering activates only when sandboxing is in domain-enforcing mode).
| * or when the terminal sandbox service reports sandboxing is on. | |
| * or when the terminal sandbox service is in domain-enforcing mode. |
| [AgentSandboxSettingId.AgentSandboxEnabled]: { | ||
| markdownDescription: localize('agentSandbox.enabledSetting', "Controls whether agent mode uses sandboxing to restrict what tools can do. When enabled, tools like the terminal are run in a sandboxed environment to limit access to the system."), | ||
| type: 'string', | ||
| enum: [AgentSandboxEnabledValue.Off, AgentSandboxEnabledValue.On], | ||
| enum: [AgentSandboxEnabledValue.Off, AgentSandboxEnabledValue.On, AgentSandboxEnabledValue.AllowNetwork], | ||
| enumDescriptions: [ | ||
| localize('agentSandbox.enabledSetting.offDescription', 'Disable sandboxing for agent mode tools.'), | ||
| localize('agentSandbox.enabledSetting.onDescription', 'Enable sandboxing for agent mode tools.'), | ||
| localize('agentSandbox.enabledSetting.allowNetworkDescription', 'Enable sandboxing for agent mode tools, but do not block commands based on configured network domains.'), | ||
| ], |
There was a problem hiding this comment.
PR description focuses on deprecated setting fallback, but this change also introduces a new AllowNetwork setting value and threads a new TerminalSandboxEnablement API through callers. If this is intentional, consider updating the PR description/title to reflect the additional behavior/API surface change (or splitting into separate PRs) so reviewers know to validate the new mode specifically.
Summary
fixes #313409
inspect()returns a namespace object from child settings such aschat.agent.sandbox.fileSystem.linuxchat.agent.sandboxnamespace conflict while preserving exact deprecated user-setting fallback behaviorValidation
npm run transpile-clientnpm run test-browser-no-install -- --browser chromium --run src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/terminalSandboxService.test.ts --grep "deprecated chat.agent.sandbox|namespace conflicts|outside user scope"git --no-pager diff --check -- src/vs/workbench/contrib/terminalContrib/chatAgentTools/common/terminalSandboxService.ts src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/terminalSandboxService.test.ts