From bfa86b086ffade654a05c87b266d7853b2036008 Mon Sep 17 00:00:00 2001 From: Franz Daubner Date: Wed, 14 Oct 2026 09:17:50 +0200 Subject: [PATCH 1/9] fix: enforce and persist workspace-scoped auto-approve command lists (#1939) Workspace-scope zoo-code.allowedCommands/deniedCommands settings were never enforced, and saving the auto-approve dialog flattened the merged dialog list into user-global settings, leaking project entries into every workspace. Activation additionally seeded global state from the effective config, permanently copying workspace overrides into global settings. - Read/enforcement: new src/core/auto-approval/commandScope.ts unions the extension global-state layer with the workspace layer from inspect().workspaceFolderValue ?? workspaceValue; ClineProvider.getState() and getStateToPostToWebview() share the same merged value so enforcement and display cannot drift (old mergeCommandLists deleted). - Write/scope-leak: splitCommandListScopes() + resolveWorkspaceWriteTarget() keep workspace entries in their folder/workspace scope, route new entries to Global, remove deleted entries from the owning scope, and persist only the global layer to extension global state. - Activation seed leak: seedAllowedCommands.ts reads inspect().globalValue only, so workspace overrides are no longer folded into (and copied permanently to) global state at activation. - Dead code: remove vestigial allowedCommands/deniedCommands webview message handlers (zero senders), their WebviewMessage union members, and the orphaned commands payload. Closes #1939 --- packages/types/src/vscode-extension-host.ts | 3 - src/__tests__/extension.spec.ts | 71 ++++- .../__tests__/commandScope.spec.ts | 247 ++++++++++++++++++ src/core/auto-approval/commandScope.ts | 139 ++++++++++ src/core/webview/ClineProvider.ts | 73 +----- .../webview/__tests__/ClineProvider.spec.ts | 69 +++++ .../__tests__/webviewMessageHandler.spec.ts | 90 +++++++ src/core/webview/webviewMessageHandler.ts | 80 +++--- src/extension.ts | 9 +- .../__tests__/seedAllowedCommands.spec.ts | 121 +++++++++ src/utils/seedAllowedCommands.ts | 40 +++ 11 files changed, 824 insertions(+), 118 deletions(-) create mode 100644 src/core/auto-approval/__tests__/commandScope.spec.ts create mode 100644 src/core/auto-approval/commandScope.ts create mode 100644 src/utils/__tests__/seedAllowedCommands.spec.ts create mode 100644 src/utils/seedAllowedCommands.ts diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index 2f1222f8ee..95316f5e74 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -603,9 +603,7 @@ export interface WebviewMessage { | "getDismissedUpsells" | "openMarkdownPreview" | "updateSettings" - | "allowedCommands" | "getTaskWithAggregatedCosts" - | "deniedCommands" | "openDebugApiHistory" | "openDebugUiHistory" | "downloadErrorDiagnostics" @@ -662,7 +660,6 @@ export interface WebviewMessage { stepIndex?: number isLaunchAction?: boolean forceShow?: boolean - commands?: string[] audioType?: AudioType serverName?: string toolName?: string diff --git a/src/__tests__/extension.spec.ts b/src/__tests__/extension.spec.ts index d0a8b274b8..ba8c885c0a 100644 --- a/src/__tests__/extension.spec.ts +++ b/src/__tests__/extension.spec.ts @@ -18,6 +18,7 @@ vi.mock("vscode", () => ({ registerTextDocumentContentProvider: vi.fn(), getConfiguration: vi.fn().mockReturnValue({ get: vi.fn().mockReturnValue([]), + inspect: vi.fn(), }), createFileSystemWatcher: vi.fn().mockReturnValue({ onDidCreate: vi.fn(), @@ -222,15 +223,17 @@ vi.mock("../api/providers/fetchers/modelCache", () => ({ describe("extension.ts", () => { let mockContext: vscode.ExtensionContext + let globalStateValues: Map let settingsUpdatedHandler: ((data: Record) => void | Promise) | undefined beforeEach(() => { vi.clearAllMocks() + globalStateValues = new Map() mockContext = { extensionPath: "/test/path", globalState: { - get: vi.fn().mockReturnValue(undefined), + get: vi.fn((key: string) => globalStateValues.get(key)), update: vi.fn(), }, subscriptions: [], @@ -404,6 +407,72 @@ describe("extension.ts", () => { expect(dotenv.config).toHaveBeenCalledTimes(1) }) + describe("allowedCommands activation seeding", () => { + function setConfigScope( + vscode: typeof import("vscode"), + scope: { globalValue?: string[]; workspaceValue?: string[] }, + ) { + const { globalValue, workspaceValue } = scope + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ + // Key-scoped so proxy/other activation reads still see no value. + get: vi.fn((key: string) => + key === "allowedCommands" ? [...(globalValue ?? []), ...(workspaceValue ?? [])] : undefined, + ), + inspect: vi.fn().mockReturnValue({ + key: "allowedCommands", + globalValue, + workspaceValue, + }), + update: vi.fn(), + } as unknown as vscode.WorkspaceConfiguration) + } + + beforeEach(() => { + vi.resetModules() + }) + + afterEach(async () => { + const vscode = await import("vscode") + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ + get: vi.fn().mockReturnValue([]), + inspect: vi.fn(), + } as unknown as vscode.WorkspaceConfiguration) + }) + + test("does not seed global state from a workspace-scoped override", async () => { + const vscode = await import("vscode") + setConfigScope(vscode, { workspaceValue: ["docker"] }) + + const { activate } = await import("../extension") + await activate(mockContext) + + expect(mockContext.globalState.update).not.toHaveBeenCalledWith("allowedCommands", expect.anything()) + expect(globalStateValues.get("allowedCommands")).toBeUndefined() + }) + + test("seeds global state from the user-scoped value when it is empty", async () => { + const vscode = await import("vscode") + setConfigScope(vscode, { globalValue: ["git"] }) + + const { activate } = await import("../extension") + await activate(mockContext) + + expect(mockContext.globalState.update).toHaveBeenCalledWith("allowedCommands", ["git"]) + }) + + test("does not overwrite global state that is already populated", async () => { + const vscode = await import("vscode") + setConfigScope(vscode, { globalValue: ["git"] }) + globalStateValues.set("allowedCommands", ["npm"]) + + const { activate } = await import("../extension") + await activate(mockContext) + + expect(mockContext.globalState.update).not.toHaveBeenCalledWith("allowedCommands", expect.anything()) + expect(globalStateValues.get("allowedCommands")).toEqual(["npm"]) + }) + }) + describe("cloud organization settings handling", () => { beforeEach(() => { vi.resetModules() diff --git a/src/core/auto-approval/__tests__/commandScope.spec.ts b/src/core/auto-approval/__tests__/commandScope.spec.ts new file mode 100644 index 0000000000..7784c4a1bd --- /dev/null +++ b/src/core/auto-approval/__tests__/commandScope.spec.ts @@ -0,0 +1,247 @@ +import * as vscode from "vscode" + +import { readCommandListScopes, resolveWorkspaceWriteTarget, splitCommandListScopes } from "../commandScope" + +vi.mock("vscode", () => ({ + ConfigurationTarget: { + Global: 1, + Workspace: 2, + WorkspaceFolder: 3, + }, + workspace: { + getConfiguration: vi.fn(), + }, +})) + +interface ConfigDouble { + inspect?: { + key: string + defaultValue?: string[] + globalValue?: string[] + workspaceValue?: string[] + workspaceFolderValue?: string[] + } + effective?: string[] +} + +function mockConfiguration({ inspect, effective = [] }: ConfigDouble = {}) { + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ + get: vi.fn().mockReturnValue(effective), + inspect: vi.fn().mockReturnValue(inspect), + update: vi.fn(), + } as unknown as vscode.WorkspaceConfiguration) +} + +describe("readCommandListScopes", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it("reads the workspace layer from the workspace-scoped configuration value", () => { + mockConfiguration({ inspect: { key: "deniedCommands", workspaceValue: ["docker"] } }) + + const scopes = readCommandListScopes("deniedCommands", []) + + expect(scopes.workspace).toEqual(["docker"]) + expect(scopes.merged).toEqual(["docker"]) + }) + + it("prefers the workspace-folder value over the workspace value", () => { + mockConfiguration({ + inspect: { + key: "deniedCommands", + workspaceValue: ["docker"], + workspaceFolderValue: ["pnpm docker"], + }, + }) + + const scopes = readCommandListScopes("deniedCommands", []) + + expect(scopes.workspace).toEqual(["pnpm docker"]) + expect(scopes.merged).toEqual(["pnpm docker"]) + }) + + it("ignores the effective get() value so global-scoped entries are not double-counted", () => { + // A real VS Code get() folds globalValue into the effective result, so + // reading the workspace layer from get() would leak "npm" (a + // user-settings-only entry) into the workspace layer. + mockConfiguration({ + inspect: { key: "allowedCommands", globalValue: ["npm"], workspaceValue: ["docker"] }, + effective: ["npm", "docker"], + }) + + const scopes = readCommandListScopes("allowedCommands", []) + + expect(scopes.workspace).toEqual(["docker"]) + expect(scopes.merged).toEqual(["docker"]) + }) + + it("returns an empty workspace layer when only globalValue is set", () => { + mockConfiguration({ + inspect: { key: "allowedCommands", globalValue: ["npm"] }, + effective: ["npm"], + }) + + const scopes = readCommandListScopes("allowedCommands", []) + + expect(scopes.workspace).toEqual([]) + expect(scopes.merged).toEqual([]) + }) + + it("unions global and workspace layers with deduplication and stable order", () => { + mockConfiguration({ inspect: { key: "deniedCommands", workspaceValue: ["docker", "rm"] } }) + + const scopes = readCommandListScopes("deniedCommands", ["git", "docker"]) + + expect(scopes.global).toEqual(["git", "docker"]) + expect(scopes.workspace).toEqual(["docker", "rm"]) + expect(scopes.merged).toEqual(["git", "docker", "rm"]) + }) + + it("drops blank and non-string entries from both layers", () => { + mockConfiguration({ inspect: { key: "deniedCommands", workspaceValue: ["docker", " "] } }) + + const scopes = readCommandListScopes("deniedCommands", ["git", "", " "] as string[]) + + expect(scopes.global).toEqual(["git"]) + expect(scopes.workspace).toEqual(["docker"]) + expect(scopes.merged).toEqual(["git", "docker"]) + }) + + it("treats a missing inspect result as an empty workspace layer", () => { + mockConfiguration({ inspect: undefined }) + + const scopes = readCommandListScopes("deniedCommands", ["git"]) + + expect(scopes.workspace).toEqual([]) + expect(scopes.merged).toEqual(["git"]) + }) + + it("falls back to an empty workspace layer when the configuration lookup throws", () => { + vi.mocked(vscode.workspace.getConfiguration).mockImplementation(() => { + throw new Error("configuration unavailable") + }) + + const scopes = readCommandListScopes("deniedCommands", ["git"]) + + expect(scopes.workspace).toEqual([]) + expect(scopes.merged).toEqual(["git"]) + }) +}) + +describe("splitCommandListScopes", () => { + it("keeps workspace entries in the workspace layer and routes new entries to global", () => { + // The issue #1939 leak: the dialog sends the merged list back, and the + // workspace-only entry "docker" must not end up in the global write. + const result = splitCommandListScopes(["docker", "aws"], ["docker"], []) + + expect(result.workspaceWrite).toEqual(["docker"]) + expect(result.globalWrite).toEqual(["aws"]) + }) + + it("keeps a global entry in the global layer", () => { + const result = splitCommandListScopes(["git"], [], ["git"]) + + expect(result.workspaceWrite).toEqual([]) + expect(result.globalWrite).toEqual(["git"]) + }) + + it("keeps an entry present in both layers in both layers when retained", () => { + // Semantics: do not silently delete the user's global entry just + // because the current folder happens to define it too; the entry was + // deliberately authored in both scopes and the user kept it. + const result = splitCommandListScopes(["docker"], ["docker"], ["docker"]) + + expect(result.workspaceWrite).toEqual(["docker"]) + expect(result.globalWrite).toEqual(["docker"]) + }) + + it("removes a deleted entry from the workspace layer without touching global", () => { + // The user kept "git" and deleted workspace-only "docker"; "git" must + // survive at its own (global) layer. + const result = splitCommandListScopes(["git"], ["docker"], ["git"]) + + expect(result.workspaceWrite).toEqual([]) + expect(result.globalWrite).toEqual(["git"]) + }) + + it("removes a deleted entry from every layer that contained it", () => { + // "docker" lived in both layers and the user deleted it; kept "rm" + // stays at the workspace layer only. + const result = splitCommandListScopes(["rm"], ["docker", "rm"], ["docker"]) + + expect(result.workspaceWrite).toEqual(["rm"]) + expect(result.globalWrite).toEqual([]) + }) + + it("routes entries the dialog seeded to the layer they came from", () => { + const result = splitCommandListScopes( + ["git", "docker", "pnpm docker", "aws"], + ["docker", "pnpm docker"], + ["git"], + ) + + expect(result.workspaceWrite).toEqual(["docker", "pnpm docker"]) + expect(result.globalWrite).toEqual(["git", "aws"]) + }) + + it("orders the global write as existing global entries then new entries in dialog order", () => { + const result = splitCommandListScopes(["aws", "git", "npm"], ["docker"], ["git"]) + + expect(result.globalWrite).toEqual(["git", "aws", "npm"]) + }) + + it("drops blank and non-string entries from the incoming list", () => { + const result = splitCommandListScopes(["git", "", " ", 42 as unknown as string], [], ["git"]) + + expect(result.globalWrite).toEqual(["git"]) + expect(result.workspaceWrite).toEqual([]) + }) + + it("deduplicates layers so re-writing cannot introduce duplicates", () => { + const result = splitCommandListScopes(["docker", "docker"], ["docker", "docker"], ["docker"]) + + expect(result.workspaceWrite).toEqual(["docker"]) + expect(result.globalWrite).toEqual(["docker"]) + }) +}) + +describe("resolveWorkspaceWriteTarget", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it("returns the workspace-folder target when a folder value is set", () => { + mockConfiguration({ + inspect: { key: "deniedCommands", workspaceValue: ["docker"], workspaceFolderValue: ["pnpm docker"] }, + }) + + expect(resolveWorkspaceWriteTarget("deniedCommands")).toBe(vscode.ConfigurationTarget.WorkspaceFolder) + }) + + it("returns the workspace target when only a workspace value is set", () => { + mockConfiguration({ inspect: { key: "deniedCommands", workspaceValue: ["docker"] } }) + + expect(resolveWorkspaceWriteTarget("deniedCommands")).toBe(vscode.ConfigurationTarget.Workspace) + }) + + it("returns undefined when only a global value is set, so no workspace layer is created", () => { + mockConfiguration({ inspect: { key: "deniedCommands", globalValue: ["npm"] } }) + + expect(resolveWorkspaceWriteTarget("deniedCommands")).toBeUndefined() + }) + + it("returns undefined when there is no inspect result", () => { + mockConfiguration({ inspect: undefined }) + + expect(resolveWorkspaceWriteTarget("deniedCommands")).toBeUndefined() + }) + + it("returns undefined when the configuration lookup throws", () => { + vi.mocked(vscode.workspace.getConfiguration).mockImplementation(() => { + throw new Error("configuration unavailable") + }) + + expect(resolveWorkspaceWriteTarget("deniedCommands")).toBeUndefined() + }) +}) diff --git a/src/core/auto-approval/commandScope.ts b/src/core/auto-approval/commandScope.ts new file mode 100644 index 0000000000..55c895e8c3 --- /dev/null +++ b/src/core/auto-approval/commandScope.ts @@ -0,0 +1,139 @@ +import * as vscode from "vscode" + +import { Package } from "../../shared/package" + +export type CommandListConfigKey = "allowedCommands" | "deniedCommands" + +export interface CommandListScopes { + global: string[] + workspace: string[] + merged: string[] +} + +/** + * Reads an auto-approval command list from both configuration layers and + * composes the list that should be enforced and displayed. + * + * The workspace layer is read via `inspect()` rather than `get()` on purpose: + * `get()` returns the effective value, which already folds `globalValue` into + * the result. Unioning that with the global layer would double-count entries + * and make removals from global settings unable to shrink the enforced set. + * + * @param configKey - The extension configuration key to read. + * @param globalList - The list persisted in extension global state. + * @returns The per-layer lists and their deduplicated set union. + */ +export function readCommandListScopes( + configKey: CommandListConfigKey, + globalList: string[] | undefined, +): CommandListScopes { + const global = sanitizeCommandList(globalList) + let workspace: string[] = [] + + try { + const inspected = vscode.workspace.getConfiguration(Package.name).inspect(configKey) + // The folder value wins over the workspace value, matching VS Code's own + // resolution precedence for multi-root workspaces. + workspace = sanitizeCommandList(inspected?.workspaceFolderValue ?? inspected?.workspaceValue) + } catch { + // Configuration access can fail (for example before the workspace is + // ready); an absent workspace layer must not break enforcement. + workspace = [] + } + + // Set union: an entry present in both layers collapses to one, so + // composition cannot change prefix-match outcomes. + const merged = [...new Set([...global, ...workspace])] + + return { global, workspace, merged } +} + +export interface CommandListWriteScopes { + globalWrite: string[] + workspaceWrite: string[] +} + +/** + * Splits the merged command list the settings dialog saves back into the two + * configuration layers it was composed from, so saving the dialog cannot + * flatten workspace-scoped entries into the user's global settings. + * + * Semantics: + * - An entry the user kept that exists in the workspace layer stays in the + * workspace layer. If the same entry also exists in the global layer it + * stays there too: dropping it from the global write would silently delete + * a user's global entry just because the current folder happens to define + * it as well. + * - An entry the user removed is dropped from every layer that contained it, + * which makes workspace-scoped entries deletable from the dialog. + * - A brand-new entry (in neither layer) lands in the global layer, matching + * the pre-existing behavior of routing dialog edits to + * `ConfigurationTarget.Global`. + * + * @param incoming - The full merged list sent by the webview on save. + * @param workspaceLayer - The current workspace-scope configuration value. + * @param globalLayer - The list currently persisted in extension global state. + * @returns The per-layer values to write back. + */ +export function splitCommandListScopes( + incoming: string[], + workspaceLayer: string[], + globalLayer: string[], +): CommandListWriteScopes { + const kept = new Set(sanitizeCommandList(incoming)) + const workspace = sanitizeCommandList(workspaceLayer) + const global = sanitizeCommandList(globalLayer) + const known = new Set([...workspace, ...global]) + + // Deduplicated so re-writing a layer cannot introduce duplicates. + const workspaceWrite = [...new Set(workspace.filter((command) => kept.has(command)))] + const globalWrite = [ + ...new Set(global.filter((command) => kept.has(command))), + // Preserve the dialog's ordering for entries the user just added. + ...[...kept].filter((command) => !known.has(command)), + ] + + return { globalWrite, workspaceWrite } +} + +/** + * Picks the configuration target that owns the current workspace-scope layer + * for `configKey`, mirroring how `readCommandListScopes` reads it via an + * unscoped `getConfiguration(Package.name)` (which resolves folder values + * from the first workspace folder). + * + * - `workspaceFolderValue` set -> write at `WorkspaceFolder` scope. + * - Otherwise `workspaceValue` set -> write at `Workspace` scope. + * - Neither set -> `undefined`: no workspace layer exists, so none should be + * created (an empty workspace layer must not materialize in settings.json). + * + * @param configKey - The extension configuration key to inspect. + * @returns The workspace-scoped target to write, or `undefined` when no + * workspace layer exists. + */ +export function resolveWorkspaceWriteTarget( + configKey: CommandListConfigKey, +): vscode.ConfigurationTarget.WorkspaceFolder | vscode.ConfigurationTarget.Workspace | undefined { + try { + const inspected = vscode.workspace.getConfiguration(Package.name).inspect(configKey) + if (inspected?.workspaceFolderValue !== undefined) { + return vscode.ConfigurationTarget.WorkspaceFolder + } + if (inspected?.workspaceValue !== undefined) { + return vscode.ConfigurationTarget.Workspace + } + return undefined + } catch { + // Same defensive posture as readCommandListScopes: configuration + // access failures mean we create no workspace layer. + return undefined + } +} + +function sanitizeCommandList(list: unknown): string[] { + if (!Array.isArray(list)) { + return [] + } + + return list.filter((command): command is string => typeof command === "string" && command.trim().length > 0) +} diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 874e2f8f08..78ac7c4870 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -112,6 +112,7 @@ import { forceFullModelDetailsLoad, hasLoadedFullDetails } from "../../api/provi import { ContextProxy } from "../config/ContextProxy" import { ProviderSettingsManager } from "../config/ProviderSettingsManager" import { CustomModesManager } from "../config/CustomModesManager" +import { readCommandListScopes } from "../auto-approval/commandScope" import { PendingActionSettlementError, Task } from "../task/Task" import { webviewMessageHandler } from "./webviewMessageHandler" @@ -2528,62 +2529,6 @@ export class ClineProvider } } - /** - * Merges allowed commands from global state and workspace configuration - * with proper validation and deduplication - */ - private mergeAllowedCommands(globalStateCommands?: string[]): string[] { - return this.mergeCommandLists("allowedCommands", "allowed", globalStateCommands) - } - - /** - * Merges denied commands from global state and workspace configuration - * with proper validation and deduplication - */ - private mergeDeniedCommands(globalStateCommands?: string[]): string[] { - return this.mergeCommandLists("deniedCommands", "denied", globalStateCommands) - } - - /** - * Common utility for merging command lists from global state and workspace configuration. - * Implements the Command Denylist feature's merging strategy with proper validation. - * - * @param configKey - VSCode workspace configuration key - * @param commandType - Type of commands for error logging - * @param globalStateCommands - Commands from global state - * @returns Merged and deduplicated command list - */ - private mergeCommandLists( - configKey: "allowedCommands" | "deniedCommands", - commandType: "allowed" | "denied", - globalStateCommands?: string[], - ): string[] { - try { - // Validate and sanitize global state commands - const validGlobalCommands = Array.isArray(globalStateCommands) - ? globalStateCommands.filter((cmd) => typeof cmd === "string" && cmd.trim().length > 0) - : [] - - // Get workspace configuration commands - const workspaceCommands = vscode.workspace.getConfiguration(Package.name).get(configKey) || [] - - // Validate and sanitize workspace commands - const validWorkspaceCommands = Array.isArray(workspaceCommands) - ? workspaceCommands.filter((cmd) => typeof cmd === "string" && cmd.trim().length > 0) - : [] - - // Combine and deduplicate commands - // Global state takes precedence over workspace configuration - const mergedCommands = [...new Set([...validGlobalCommands, ...validWorkspaceCommands])] - - return mergedCommands - } catch (error) { - console.error(`Error merging ${commandType} commands:`, error) - // Return empty array as fallback to prevent crashes - return [] - } - } - async getStateToPostToWebview({ includeTaskHistory = true }: GetStateOptions = {}): Promise { // Ensure the store is initialized before reading task history await this.taskHistoryStore.initialized @@ -2705,8 +2650,6 @@ export class ClineProvider const telemetryKey = process.env.POSTHOG_API_KEY const machineId = vscode.env.machineId const vscodeTelemetryEnabled = vscode.env.isTelemetryEnabled - const mergedAllowedCommands = this.mergeAllowedCommands(allowedCommands) - const mergedDeniedCommands = this.mergeDeniedCommands(deniedCommands) const cwd = this.cwd const currentTask = this.getCurrentTask() let currentTaskMode: string | undefined @@ -2786,8 +2729,8 @@ export class ClineProvider checkpointTimeout: checkpointTimeout ?? DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, shouldShowAnnouncement: telemetrySetting !== "unset" && lastShownAnnouncementId !== this.latestAnnouncementId, - allowedCommands: mergedAllowedCommands, - deniedCommands: mergedDeniedCommands, + allowedCommands: allowedCommands ?? [], + deniedCommands: deniedCommands ?? [], soundVolume: soundVolume ?? 0.5, writeDelayMs: writeDelayMs ?? DEFAULT_WRITE_DELAY_MS, diffFuzzyThreshold: diffFuzzyThreshold ?? DEFAULT_DIFF_FUZZY_THRESHOLD, @@ -3016,8 +2959,11 @@ export class ClineProvider autoCondenseContext: stateValues.autoCondenseContext ?? true, autoCondenseContextPercent: stateValues.autoCondenseContextPercent ?? 100, taskHistory: includeTaskHistory ? this.taskHistoryStore.getAll() : [], - allowedCommands: stateValues.allowedCommands, - deniedCommands: stateValues.deniedCommands, + // Enforce and display the set union of the global-state layer and the + // workspace-scope configuration layer; getStateToPostToWebview shows + // exactly these values, so display cannot drift from enforcement. + allowedCommands: readCommandListScopes("allowedCommands", stateValues.allowedCommands).merged, + deniedCommands: readCommandListScopes("deniedCommands", stateValues.deniedCommands).merged, soundEnabled: stateValues.soundEnabled ?? false, ttsEnabled: stateValues.ttsEnabled ?? false, ttsSpeed: stateValues.ttsSpeed ?? 1.0, @@ -3350,6 +3296,9 @@ export class ClineProvider if (configuration) { await this.setValues(configuration) + // Deliberately Global-targeted: these writes carry "start this task with + // exactly this auto-approval policy" semantics (e.g. from handoff/delegation + // payloads), unlike the scope-split persistence in the settings dialog. if (configuration.allowedCommands) { await vscode.workspace .getConfiguration(Package.name) diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index b5f304fe56..4a96fadde0 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -1615,6 +1615,75 @@ describe("ClineProvider", () => { expect(state.alwaysDenyUnapprovedCommands).toBe(false) }) + describe("command list scope merging", () => { + // getConfiguration overrides must not leak past these tests: + // clearAllMocks does not remove mockReturnValue defaults. + afterEach(() => { + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ + get: vi.fn().mockReturnValue([]), + update: vi.fn(), + } as unknown as vscode.WorkspaceConfiguration) + }) + + test("getState enforces workspace-scope denied commands even when global state is empty", async () => { + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ + get: vi.fn().mockReturnValue([]), + inspect: vi + .fn() + .mockImplementation((key: string) => + key === "deniedCommands" ? { key, workspaceValue: ["docker"] } : undefined, + ), + update: vi.fn(), + } as unknown as vscode.WorkspaceConfiguration) + + const state = await provider.getState() + + expect(state.deniedCommands).toEqual(["docker"]) + }) + + test("getStateToPostToWebview shows the same merged command lists as getState", async () => { + await provider.resolveWebviewView(mockWebviewView) + await provider.contextProxy.setValue("deniedCommands", ["git"]) + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ + get: vi.fn().mockReturnValue(["git", "docker"]), + inspect: vi + .fn() + .mockImplementation((key: string) => + key === "deniedCommands" ? { key, workspaceValue: ["docker"] } : undefined, + ), + update: vi.fn(), + } as unknown as vscode.WorkspaceConfiguration) + + const state = await provider.getStateToPostToWebview() + + // Display is the set union of global state and workspace scope, matching + // what getState() enforces. + expect(state.deniedCommands).toEqual(["git", "docker"]) + expect(state.allowedCommands).toEqual([]) + }) + + test("getState prefers the workspace-folder value for multi-root workspaces", async () => { + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ + get: vi.fn().mockReturnValue(["docker", "pnpm docker"]), + inspect: vi + .fn() + .mockImplementation((key: string) => + key === "deniedCommands" + ? { key, workspaceValue: ["docker"], workspaceFolderValue: ["pnpm docker"] } + : undefined, + ), + update: vi.fn(), + } as unknown as vscode.WorkspaceConfiguration) + + const state = await provider.getState() + + // The folder value replaces (not unions with) the workspace value, and + // the effective get() value is ignored so global-scoped entries do not + // leak into the workspace layer. + expect(state.deniedCommands).toEqual(["pnpm docker"]) + }) + }) + test("getState returns the saved allowed read files", async () => { await provider.contextProxy.setValue("allowedReadFiles", ["notes.md"]) diff --git a/src/core/webview/__tests__/webviewMessageHandler.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index dcd70f92f1..3d2f6abc21 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -178,6 +178,11 @@ vi.mock("vscode", () => { const showTextDocument = vi.fn().mockResolvedValue(undefined) return { + ConfigurationTarget: { + Global: 1, + Workspace: 2, + WorkspaceFolder: 3, + }, window: { showInformationMessage, showErrorMessage, @@ -1429,6 +1434,91 @@ describe("webviewMessageHandler - allowlists together", () => { }) }) +describe("webviewMessageHandler - command list scope-aware persistence", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + // Mirrors the real VS Code configuration API: `inspect()` exposes the + // per-scope values, and `update()` records the target it was written to. + // Extension global state is left unset (getValue returns undefined), i.e. + // the global layer starts empty. + function mockCommandConfiguration(inspected: { + globalValue?: string[] + workspaceValue?: string[] + workspaceFolderValue?: string[] + }) { + const update = vi.fn() + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ + get: vi.fn().mockReturnValue([]), + inspect: vi.fn().mockReturnValue(inspected), + update, + } as never) + return update + } + + it("keeps workspace-scoped denied commands out of the global write when saving the merged list", async () => { + const update = mockCommandConfiguration({ workspaceValue: ["docker"] }) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: ["docker", "aws"] }, + }) + + // "docker" stays at the workspace scope instead of leaking into the + // user's global settings (issue #1939 leg B). + expect(update).toHaveBeenCalledWith("deniedCommands", ["docker"], vscode.ConfigurationTarget.Workspace) + expect(update).toHaveBeenCalledWith("deniedCommands", ["aws"], vscode.ConfigurationTarget.Global) + // Extension global state holds only the global layer, so the next save + // cannot double-home the workspace entry. + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("deniedCommands", ["aws"]) + }) + + it("writes the workspace-folder value at the folder target in multi-root workspaces", async () => { + const update = mockCommandConfiguration({ workspaceValue: ["npm"], workspaceFolderValue: ["docker"] }) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: ["docker", "aws"] }, + }) + + expect(update).toHaveBeenCalledWith("deniedCommands", ["docker"], vscode.ConfigurationTarget.WorkspaceFolder) + expect(update).toHaveBeenCalledWith("deniedCommands", ["aws"], vscode.ConfigurationTarget.Global) + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("deniedCommands", ["aws"]) + }) + + it("clears a deleted workspace entry at the workspace scope without adding it to global", async () => { + const update = mockCommandConfiguration({ workspaceValue: ["docker"] }) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: [] }, + }) + + // Deleting a project entry must clear it where it lives... + expect(update).toHaveBeenCalledWith("deniedCommands", [], vscode.ConfigurationTarget.Workspace) + // ...and must not copy it into global settings. + expect(update).not.toHaveBeenCalledWith("deniedCommands", ["docker"], vscode.ConfigurationTarget.Global) + expect(update).toHaveBeenCalledWith("deniedCommands", [], vscode.ConfigurationTarget.Global) + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("deniedCommands", []) + }) + + it("does not create a workspace layer when none exists", async () => { + const update = mockCommandConfiguration({}) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { allowedCommands: ["git"] }, + }) + + // Only the Global write happens; saving must not materialize an empty + // workspace-scoped value in settings.json. + expect(update).toHaveBeenCalledTimes(1) + expect(update).toHaveBeenCalledWith("allowedCommands", ["git"], vscode.ConfigurationTarget.Global) + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("allowedCommands", ["git"]) + }) +}) + describe("webviewMessageHandler - terminalProfile", () => { beforeEach(() => { vi.clearAllMocks() diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 4ba94d454c..374427f966 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -59,6 +59,11 @@ import { } from "./rulesMessageHandler" import { changeLanguage, t } from "../../i18n" import { Package } from "../../shared/package" +import { + readCommandListScopes, + resolveWorkspaceWriteTarget, + splitCommandListScopes, +} from "../auto-approval/commandScope" import { type RouterName, toRouterName } from "../../shared/api" import { MessageEnhancer } from "./messageEnhancer" @@ -751,26 +756,43 @@ export const webviewMessageHandler = async ( if (key === "language") { newValue = value ?? "en" changeLanguage(newValue as Language) - } else if (key === "allowedCommands") { + } else if (key === "allowedCommands" || key === "deniedCommands") { const commands = value ?? [] - newValue = Array.isArray(commands) + const incoming = Array.isArray(commands) ? commands.filter((cmd) => typeof cmd === "string" && cmd.trim().length > 0) : [] - await vscode.workspace - .getConfiguration(Package.name) - .update("allowedCommands", newValue, vscode.ConfigurationTarget.Global) - } else if (key === "deniedCommands") { - const commands = value ?? [] + // The webview always sends the full merged list (it displays + // the union of the global-state and workspace-scope layers). + // Read both layers BEFORE any write and split the incoming + // list by origin scope, so saving the dialog cannot flatten + // workspace-scoped entries into the user's global settings. + const currentScopes = readCommandListScopes(key, getGlobalState(key)) + const { globalWrite, workspaceWrite } = splitCommandListScopes( + incoming, + currentScopes.workspace, + currentScopes.global, + ) + const workspaceTarget = resolveWorkspaceWriteTarget(key) + const config = vscode.workspace.getConfiguration(Package.name) + + // Write the workspace layer first, at the target that owns it. + // When no workspace layer exists the write is skipped entirely + // so saving never materializes one; when entries were deleted + // the `[]` write clears them at that scope. + if (workspaceTarget !== undefined) { + await config.update(key, workspaceWrite, workspaceTarget) + } - newValue = Array.isArray(commands) - ? commands.filter((cmd) => typeof cmd === "string" && cmd.trim().length > 0) - : [] + await config.update(key, globalWrite, vscode.ConfigurationTarget.Global) - await vscode.workspace - .getConfiguration(Package.name) - .update("deniedCommands", newValue, vscode.ConfigurationTarget.Global) + // Extension global state keeps only the global layer: + // enforcement re-adds the workspace layer through + // readCommandListScopes, so storing the merged list here + // would double-home workspace entries and re-leak them on the + // next save. + newValue = globalWrite } else if (key === "allowedReadFiles" || key === "allowedWriteFiles") { const patterns = value ?? [] @@ -1712,38 +1734,6 @@ export const webviewMessageHandler = async ( // Cancel any pending auto-approval timeout for the current task provider.getCurrentTask()?.cancelAutoApprovalTimeout() break - case "allowedCommands": { - // Validate and sanitize the commands array - const commands = message.commands ?? [] - const validCommands = Array.isArray(commands) - ? commands.filter((cmd) => typeof cmd === "string" && cmd.trim().length > 0) - : [] - - await updateGlobalState("allowedCommands", validCommands) - - // Also update workspace settings. - await vscode.workspace - .getConfiguration(Package.name) - .update("allowedCommands", validCommands, vscode.ConfigurationTarget.Global) - - break - } - case "deniedCommands": { - // Validate and sanitize the commands array - const commands = message.commands ?? [] - const validCommands = Array.isArray(commands) - ? commands.filter((cmd) => typeof cmd === "string" && cmd.trim().length > 0) - : [] - - await updateGlobalState("deniedCommands", validCommands) - - // Also update workspace settings. - await vscode.workspace - .getConfiguration(Package.name) - .update("deniedCommands", validCommands, vscode.ConfigurationTarget.Global) - - break - } case "openCustomModesSettings": { const customModesFilePath = await provider.customModesManager.getCustomModesFilePath() diff --git a/src/extension.ts b/src/extension.ts index 1d71f834df..080ca5bae9 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -38,6 +38,7 @@ import { CodeIndexManagerRegistry } from "./services/code-index/code-index-manag import { CodeIndexScope } from "./services/code-index/code-index-scope" import { MdmService } from "./services/mdm/MdmService" import { migrateSettings } from "./utils/migrateSettings" +import { seedAllowedCommands } from "./utils/seedAllowedCommands" import { autoImportSettings } from "./utils/autoImportSettings" import { API } from "./extension/api" @@ -163,13 +164,7 @@ export async function activate(context: vscode.ExtensionContext) { // Initialize Zoo Code auth service for extension session token management. await initZooCodeAuth(context) - // Get default commands from configuration. - const defaultCommands = vscode.workspace.getConfiguration(Package.name).get("allowedCommands") || [] - - // Initialize global state if not already set. - if (!context.globalState.get("allowedCommands")) { - context.globalState.update("allowedCommands", defaultCommands) - } + await seedAllowedCommands(context.globalState) const contextProxy = await ContextProxy.getInstance(context) const updateTelemetryState = () => { diff --git a/src/utils/__tests__/seedAllowedCommands.spec.ts b/src/utils/__tests__/seedAllowedCommands.spec.ts new file mode 100644 index 0000000000..19c2623dfb --- /dev/null +++ b/src/utils/__tests__/seedAllowedCommands.spec.ts @@ -0,0 +1,121 @@ +// npx vitest run utils/__tests__/seedAllowedCommands.spec.ts + +import * as vscode from "vscode" + +import { seedAllowedCommands } from "../seedAllowedCommands" + +vi.mock("vscode", () => ({ + workspace: { + getConfiguration: vi.fn(), + }, +})) + +interface ConfigDouble { + inspect?: { + key: string + defaultValue?: string[] + globalValue?: string[] + workspaceValue?: string[] + workspaceFolderValue?: string[] + } + effective?: string[] + inspectThrows?: boolean +} + +function mockConfiguration({ inspect, effective = [], inspectThrows = false }: ConfigDouble = {}) { + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ + get: vi.fn().mockReturnValue(effective), + inspect: vi.fn().mockImplementation(() => { + if (inspectThrows) { + throw new Error("configuration access failed") + } + return inspect + }), + update: vi.fn(), + } as unknown as vscode.WorkspaceConfiguration) +} + +function mockMemento(existing: unknown = undefined) { + const store = new Map() + if (existing !== undefined) { + store.set("allowedCommands", existing) + } + + return { + get: vi.fn((key: string) => store.get(key)), + update: vi.fn(async (key: string, value: unknown) => { + store.set(key, value) + }), + } +} + +describe("seedAllowedCommands", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it("leaves global state unset when only a workspace-scoped value exists", async () => { + // A real VS Code get() folds the workspace override into the effective + // value, so this is the leak scenario: reading get() would seed + // ["docker"] (from .vscode/settings.json) into global state. + mockConfiguration({ + inspect: { key: "allowedCommands", globalValue: undefined, workspaceValue: ["docker"] }, + effective: ["docker"], + }) + const globalState = mockMemento() + + await seedAllowedCommands(globalState as unknown as vscode.Memento) + + expect(globalState.update).not.toHaveBeenCalled() + expect(globalState.get("allowedCommands")).toBeUndefined() + }) + + it("seeds from the global-scope value when global state is empty", async () => { + mockConfiguration({ inspect: { key: "allowedCommands", globalValue: ["git"] }, effective: ["git"] }) + const globalState = mockMemento() + + await seedAllowedCommands(globalState as unknown as vscode.Memento) + + expect(globalState.update).toHaveBeenCalledExactlyOnceWith("allowedCommands", ["git"]) + expect(globalState.get("allowedCommands")).toEqual(["git"]) + }) + + it("does not overwrite global state that is already populated", async () => { + mockConfiguration({ inspect: { key: "allowedCommands", globalValue: ["git"] }, effective: ["git"] }) + const globalState = mockMemento(["npm"]) + + await seedAllowedCommands(globalState as unknown as vscode.Memento) + + expect(globalState.update).not.toHaveBeenCalled() + expect(globalState.get("allowedCommands")).toEqual(["npm"]) + }) + + it("leaves global state untouched when there is no inspect result at all", async () => { + mockConfiguration({ inspect: undefined, effective: ["git"] }) + const globalState = mockMemento() + + await seedAllowedCommands(globalState as unknown as vscode.Memento) + + expect(globalState.update).not.toHaveBeenCalled() + }) + + it("skips the seed without throwing when configuration access fails", async () => { + mockConfiguration({ inspectThrows: true }) + const globalState = mockMemento() + + await expect(seedAllowedCommands(globalState as unknown as vscode.Memento)).resolves.toBeUndefined() + + expect(globalState.update).not.toHaveBeenCalled() + }) + + it("does not share a reference with the inspected configuration value", async () => { + const globalValue = ["git"] + mockConfiguration({ inspect: { key: "allowedCommands", globalValue } }) + const globalState = mockMemento() + + await seedAllowedCommands(globalState as unknown as vscode.Memento) + + expect(globalState.update).toHaveBeenCalledWith("allowedCommands", expect.arrayContaining(["git"])) + expect(globalState.update.mock.calls[0][1]).not.toBe(globalValue) + }) +}) diff --git a/src/utils/seedAllowedCommands.ts b/src/utils/seedAllowedCommands.ts new file mode 100644 index 0000000000..6a56173e47 --- /dev/null +++ b/src/utils/seedAllowedCommands.ts @@ -0,0 +1,40 @@ +import * as vscode from "vscode" + +import { Package } from "../shared/package" + +/** + * Seeds the extension's global-state allowlist from the `allowedCommands` + * setting, but only when global state holds no list yet (first run). + * + * The seed reads the global-scope value via `inspect()` rather than the + * effective `get()` on purpose: `get()` folds in workspace-scoped overrides, + * so a project's `.vscode/settings.json` would otherwise be copied + * permanently into global state the first time the extension activates inside + * that project — a leak that survives closing the project. + * + * @param globalState - The extension global-state memento to seed. + */ +export async function seedAllowedCommands(globalState: vscode.Memento): Promise { + if (globalState.get("allowedCommands")) { + return + } + + let globalValue: string[] | undefined + try { + globalValue = vscode.workspace.getConfiguration(Package.name).inspect("allowedCommands")?.globalValue + } catch { + // Configuration access can fail (for example before the workspace is + // ready); skipping the seed only defers it to a later activation. + return + } + + // An absent global-scope value leaves global state untouched: seeding the + // package.json default or a workspace override would defeat the point of + // scoping the seed to the user's own settings. + if (!Array.isArray(globalValue)) { + return + } + + // Copy so global state never shares a reference with the configuration object. + await globalState.update("allowedCommands", [...globalValue]) +} From 420cbe80f46de74fb8df54273186eb4ee34c320e Mon Sep 17 00:00:00 2001 From: Franz Daubner Date: Wed, 14 Oct 2026 18:29:01 +0200 Subject: [PATCH 2/9] fix: compensate failed command-list saves and scope config access per review (#1939) - Rewind the workspace-layer command-list write when the global-layer write fails: the raw previous value is captured before saving, the failed key skips its extension global-state update while the other keys still save, and a failed rewind is reported alongside the original error. - Read and write the command lists through one configuration object scoped to the first workspace folder, so workspace-folder targets are legal and resolve against the right folder's settings (the VS Code API rejects folder-targeted updates on an unscoped configuration). - Narrow the try/catch around configuration lookups so a failed lookup stays distinct from an absent value; behavior is unchanged, the guarded failure mode is now testable. - Add regression tests for the compensation path, the scoped reads and writes, and the seed guard; tighten comments. --- src/__tests__/extension.spec.ts | 4 + .../__tests__/commandScope.spec.ts | 52 +++++- src/core/auto-approval/commandScope.ts | 66 +++++--- .../__tests__/webviewMessageHandler.spec.ts | 151 ++++++++++++++++-- src/core/webview/webviewMessageHandler.ts | 73 +++++++-- .../__tests__/seedAllowedCommands.spec.ts | 61 ++++--- src/utils/seedAllowedCommands.ts | 12 +- 7 files changed, 349 insertions(+), 70 deletions(-) diff --git a/src/__tests__/extension.spec.ts b/src/__tests__/extension.spec.ts index ba8c885c0a..3b52363763 100644 --- a/src/__tests__/extension.spec.ts +++ b/src/__tests__/extension.spec.ts @@ -424,6 +424,8 @@ describe("extension.ts", () => { workspaceValue, }), update: vi.fn(), + // The double scripts key-scoped get() and per-scope inspect() + // shapes the shared helper cannot express without obscuring them. } as unknown as vscode.WorkspaceConfiguration) } @@ -436,6 +438,8 @@ describe("extension.ts", () => { vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ get: vi.fn().mockReturnValue([]), inspect: vi.fn(), + // Restores the neutral default after the scope fixtures above; + // update is intentionally absent because no test runs after this. } as unknown as vscode.WorkspaceConfiguration) }) diff --git a/src/core/auto-approval/__tests__/commandScope.spec.ts b/src/core/auto-approval/__tests__/commandScope.spec.ts index 7784c4a1bd..58c48fbac3 100644 --- a/src/core/auto-approval/__tests__/commandScope.spec.ts +++ b/src/core/auto-approval/__tests__/commandScope.spec.ts @@ -1,7 +1,16 @@ import * as vscode from "vscode" +import { Package } from "../../../shared/package" +import { makeUri, makeWorkspaceConfiguration } from "../../../test-utils/vscode" import { readCommandListScopes, resolveWorkspaceWriteTarget, splitCommandListScopes } from "../commandScope" +// Scriptable stand-in for `vscode.workspace.workspaceFolders`: the module +// mock's getter reads through this object, so a test can change the open +// folders without re-mocking the module. +const workspaceState = vi.hoisted(() => ({ + workspaceFolders: undefined as { uri: unknown }[] | undefined, +})) + vi.mock("vscode", () => ({ ConfigurationTarget: { Global: 1, @@ -9,6 +18,9 @@ vi.mock("vscode", () => ({ WorkspaceFolder: 3, }, workspace: { + get workspaceFolders() { + return workspaceState.workspaceFolders + }, getConfiguration: vi.fn(), }, })) @@ -25,11 +37,12 @@ interface ConfigDouble { } function mockConfiguration({ inspect, effective = [] }: ConfigDouble = {}) { - vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ - get: vi.fn().mockReturnValue(effective), - inspect: vi.fn().mockReturnValue(inspect), - update: vi.fn(), - } as unknown as vscode.WorkspaceConfiguration) + const config = makeWorkspaceConfiguration() + // The shared double is scripted after creation so the per-test fixture + // values need no type assertion at the construction site. + vi.mocked(config.get).mockReturnValue(effective) + vi.mocked(config.inspect).mockReturnValue(inspect) + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(config) } describe("readCommandListScopes", () => { @@ -192,6 +205,8 @@ describe("splitCommandListScopes", () => { }) it("drops blank and non-string entries from the incoming list", () => { + // Deliberate wrong runtime type: configuration lists arrive as unknown, + // so the sanitizer must drop non-strings instead of throwing on them. const result = splitCommandListScopes(["git", "", " ", 42 as unknown as string], [], ["git"]) expect(result.globalWrite).toEqual(["git"]) @@ -209,6 +224,7 @@ describe("splitCommandListScopes", () => { describe("resolveWorkspaceWriteTarget", () => { beforeEach(() => { vi.clearAllMocks() + workspaceState.workspaceFolders = undefined }) it("returns the workspace-folder target when a folder value is set", () => { @@ -244,4 +260,30 @@ describe("resolveWorkspaceWriteTarget", () => { expect(resolveWorkspaceWriteTarget("deniedCommands")).toBeUndefined() }) + + it("resolves the target from the configuration scoped to the first workspace folder", () => { + const folderA = makeUri("/mock/folder-a") + const folderB = makeUri("/mock/folder-b") + workspaceState.workspaceFolders = [{ uri: folderA }, { uri: folderB }] + mockConfiguration({ inspect: { key: "deniedCommands", workspaceFolderValue: ["pnpm docker"] } }) + + expect(resolveWorkspaceWriteTarget("deniedCommands")).toBe(vscode.ConfigurationTarget.WorkspaceFolder) + expect(vscode.workspace.getConfiguration).toHaveBeenCalledWith(Package.name, folderA) + // Only the first folder is ever in scope: reading any other folder's + // configuration would silently target the wrong folder's settings file. + expect(vscode.workspace.getConfiguration).not.toHaveBeenCalledWith(Package.name, folderB) + }) + + it.each([undefined, [] as { uri: unknown }[]])( + "falls back to an unscoped configuration when no workspace folder is open (%s)", + (workspaceFolders) => { + workspaceState.workspaceFolders = workspaceFolders + mockConfiguration({ inspect: { key: "deniedCommands", workspaceValue: ["docker"] } }) + + // With no resource to resolve against, no folder value can be + // reported, so the folder target stays unreachable. + expect(resolveWorkspaceWriteTarget("deniedCommands")).toBe(vscode.ConfigurationTarget.Workspace) + expect(vscode.workspace.getConfiguration).toHaveBeenCalledWith(Package.name, undefined) + }, + ) }) diff --git a/src/core/auto-approval/commandScope.ts b/src/core/auto-approval/commandScope.ts index 55c895e8c3..4778ac1f3e 100644 --- a/src/core/auto-approval/commandScope.ts +++ b/src/core/auto-approval/commandScope.ts @@ -10,6 +10,20 @@ export interface CommandListScopes { merged: string[] } +/** + * The configuration object for every command-list read and write, scoped to + * the first workspace folder. Sharing one scoped access path keeps the two + * sides symmetric: `inspect()` can report `workspaceFolderValue`, and + * `ConfigurationTarget.WorkspaceFolder` updates are legal (the API contract + * throws for that target when the configuration is not scoped to a + * resource). In a multi-root workspace only the first folder's values are in + * scope; with no folder open the configuration is unscoped and reports no + * folder value. + */ +export function commandListConfiguration(): vscode.WorkspaceConfiguration { + return vscode.workspace.getConfiguration(Package.name, vscode.workspace.workspaceFolders?.[0]?.uri) +} + /** * Reads an auto-approval command list from both configuration layers and * composes the list that should be enforced and displayed. @@ -28,19 +42,23 @@ export function readCommandListScopes( globalList: string[] | undefined, ): CommandListScopes { const global = sanitizeCommandList(globalList) - let workspace: string[] = [] + // Only the configuration lookup itself is guarded: it can fail (for example + // before the workspace is ready), and an absent workspace layer must not + // break enforcement. The value reads below have no failure mode of their + // own, so the catch covers exactly the failure it exists to absorb. + let inspected: { workspaceValue?: string[]; workspaceFolderValue?: string[] } | undefined try { - const inspected = vscode.workspace.getConfiguration(Package.name).inspect(configKey) - // The folder value wins over the workspace value, matching VS Code's own - // resolution precedence for multi-root workspaces. - workspace = sanitizeCommandList(inspected?.workspaceFolderValue ?? inspected?.workspaceValue) + inspected = commandListConfiguration().inspect(configKey) } catch { - // Configuration access can fail (for example before the workspace is - // ready); an absent workspace layer must not break enforcement. - workspace = [] + inspected = undefined } + // The folder value wins over the workspace value, matching VS Code's + // folder-over-workspace precedence. An absent inspect result composes to + // an empty workspace layer: enforcement must not break. + const workspace = sanitizeCommandList(inspected?.workspaceFolderValue ?? inspected?.workspaceValue) + // Set union: an entry present in both layers collapses to one, so // composition cannot change prefix-match outcomes. const merged = [...new Set([...global, ...workspace])] @@ -98,9 +116,11 @@ export function splitCommandListScopes( /** * Picks the configuration target that owns the current workspace-scope layer - * for `configKey`, mirroring how `readCommandListScopes` reads it via an - * unscoped `getConfiguration(Package.name)` (which resolves folder values - * from the first workspace folder). + * for `configKey`, inspecting the same `commandListConfiguration()` object + * `readCommandListScopes` reads. Because that configuration is scoped to the + * first workspace folder, folder values resolve against that folder's + * settings; with no folder open the configuration is unscoped and reports no + * folder value, so no folder target can be selected. * * - `workspaceFolderValue` set -> write at `WorkspaceFolder` scope. * - Otherwise `workspaceValue` set -> write at `Workspace` scope. @@ -114,20 +134,22 @@ export function splitCommandListScopes( export function resolveWorkspaceWriteTarget( configKey: CommandListConfigKey, ): vscode.ConfigurationTarget.WorkspaceFolder | vscode.ConfigurationTarget.Workspace | undefined { + // Same defensive posture as readCommandListScopes: only the configuration + // lookup itself may fail, and a failure means we create no workspace layer. + let inspected: { workspaceValue?: string[]; workspaceFolderValue?: string[] } | undefined try { - const inspected = vscode.workspace.getConfiguration(Package.name).inspect(configKey) - if (inspected?.workspaceFolderValue !== undefined) { - return vscode.ConfigurationTarget.WorkspaceFolder - } - if (inspected?.workspaceValue !== undefined) { - return vscode.ConfigurationTarget.Workspace - } - return undefined + inspected = commandListConfiguration().inspect(configKey) } catch { - // Same defensive posture as readCommandListScopes: configuration - // access failures mean we create no workspace layer. - return undefined + inspected = undefined + } + + if (inspected?.workspaceFolderValue !== undefined) { + return vscode.ConfigurationTarget.WorkspaceFolder + } + if (inspected?.workspaceValue !== undefined) { + return vscode.ConfigurationTarget.Workspace } + return undefined } function sanitizeCommandList(list: unknown): string[] { diff --git a/src/core/webview/__tests__/webviewMessageHandler.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index 3d2f6abc21..ab906d9c45 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -71,6 +71,8 @@ vi.mock("@roo-code/telemetry", () => ({ import type { ModelRecord } from "@roo-code/types" +import { Package } from "../../../shared/package" +import { makeWorkspaceConfiguration } from "../../../test-utils/vscode" import { webviewMessageHandler } from "../webviewMessageHandler" import type { ClineProvider } from "../ClineProvider" import { flushModels, getModels } from "../../../api/providers/fetchers/modelCache" @@ -1439,21 +1441,44 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { vi.clearAllMocks() }) + // The file-level vscode mock opens one workspace folder, so the command + // list configuration resolves to its uri. + const mockWorkspaceFolderUri = { fsPath: "/mock/workspace" } + // Mirrors the real VS Code configuration API: `inspect()` exposes the // per-scope values, and `update()` records the target it was written to. // Extension global state is left unset (getValue returns undefined), i.e. // the global layer starts empty. - function mockCommandConfiguration(inspected: { - globalValue?: string[] - workspaceValue?: string[] - workspaceFolderValue?: string[] - }) { + // + // `updateSequence` / `inspectSequence` let a failure test script what + // successive `update()` / `inspect()` calls return (each `update()` return + // is awaited by the handler, so a rejected promise fails that write), in + // call order; a plain value resolves like the default mock. + function mockCommandConfiguration( + inspected: { + globalValue?: string[] + workspaceValue?: string[] + workspaceFolderValue?: string[] + }, + updateSequence?: unknown[], + inspectSequence?: unknown[], + ) { const update = vi.fn() - vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ - get: vi.fn().mockReturnValue([]), - inspect: vi.fn().mockReturnValue(inspected), - update, - } as never) + if (updateSequence) { + update.mockImplementation(() => updateSequence.shift()) + } + const inspect = vi.fn() + if (inspectSequence) { + inspect.mockImplementation(() => inspectSequence.shift()) + } else { + inspect.mockReturnValue(inspected) + } + // The shared structurally complete double keeps the configuration type + // correct; the scripted members are installed per test. + const config = makeWorkspaceConfiguration() + config.update = update + config.inspect = inspect + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(config) return update } @@ -1466,7 +1491,7 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { }) // "docker" stays at the workspace scope instead of leaking into the - // user's global settings (issue #1939 leg B). + // user's global settings (issue #1939: persist without scope leakage). expect(update).toHaveBeenCalledWith("deniedCommands", ["docker"], vscode.ConfigurationTarget.Workspace) expect(update).toHaveBeenCalledWith("deniedCommands", ["aws"], vscode.ConfigurationTarget.Global) // Extension global state holds only the global layer, so the next save @@ -1482,6 +1507,10 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { updatedSettings: { deniedCommands: ["docker", "aws"] }, }) + // Read, target resolution, and write all run on the configuration + // scoped to the first workspace folder — the folder-target update is + // only legal on a resource-scoped configuration. + expect(vscode.workspace.getConfiguration).toHaveBeenCalledWith(Package.name, mockWorkspaceFolderUri) expect(update).toHaveBeenCalledWith("deniedCommands", ["docker"], vscode.ConfigurationTarget.WorkspaceFolder) expect(update).toHaveBeenCalledWith("deniedCommands", ["aws"], vscode.ConfigurationTarget.Global) expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("deniedCommands", ["aws"]) @@ -1517,6 +1546,106 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { expect(update).toHaveBeenCalledWith("allowedCommands", ["git"], vscode.ConfigurationTarget.Global) expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("allowedCommands", ["git"]) }) + + it("restores the previous workspace value when the global write fails", async () => { + // update() call order: workspace write succeeds, global write rejects. + // The raw folder-layer value differs from the sanitized post-split layer + // (and from the workspace-layer value), so the restore call is uniquely + // identifiable and provably reads the folder value for a folder target. + const update = mockCommandConfiguration( + { workspaceValue: ["npm"], workspaceFolderValue: ["pnpm docker", "docker"] }, + [undefined, Promise.reject(new Error("disk on fire"))], + ) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + // A second key pins that only the failed command-list key is skipped. + updatedSettings: { deniedCommands: ["docker", "aws"], alwaysDenyUnapprovedCommands: true }, + }) + + // The capture, the failed write, and the restore all ran on the + // configuration scoped to the first workspace folder. + expect(vscode.workspace.getConfiguration).toHaveBeenCalledWith(Package.name, mockWorkspaceFolderUri) + // The raw previous folder value (entries this save dropped included) is + // written back at the target the failed save chose. + expect(update).toHaveBeenCalledWith( + "deniedCommands", + ["pnpm docker", "docker"], + vscode.ConfigurationTarget.WorkspaceFolder, + ) + expect(update).toHaveBeenCalledTimes(3) + // The failed key's extension global-state write is skipped... + expect(mockClineProvider.contextProxy.setValue).not.toHaveBeenCalledWith("deniedCommands", expect.anything()) + // ...while the other key of the same payload still persists. + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("alwaysDenyUnapprovedCommands", true) + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("Failed to save deniedCommands: disk on fire") + expect(mockClineProvider.postStateToWebview).toHaveBeenCalled() + }) + + it("does not write global or extension state when the workspace write fails", async () => { + // The first update() call is the workspace write; it rejects, so the + // global write must never be attempted for this key. The rejection is a + // plain string to pin the non-Error rendering of the failure message. + const update = mockCommandConfiguration({ workspaceValue: ["docker"] }, [Promise.reject("workspace locked")]) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: ["docker", "aws"] }, + }) + + // Nothing was changed yet, so there is nothing to restore: only the + // failed workspace write happened, and no compensation write followed. + expect(update).toHaveBeenCalledTimes(1) + expect(update).not.toHaveBeenCalledWith("deniedCommands", expect.anything(), vscode.ConfigurationTarget.Global) + expect(mockClineProvider.contextProxy.setValue).not.toHaveBeenCalled() + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("Failed to save deniedCommands: workspace locked") + }) + + it("saves both layers when the capture of the previous value finds no inspect result", async () => { + // inspect() call order: readCommandListScopes, resolveWorkspaceWriteTarget, + // then the compensation capture — only the capture call sees no result. + // A missing capture result must not abort the save: the writes proceed, + // and only a later failure could find nothing to restore. + const inspected = { workspaceValue: ["docker"] } + const update = mockCommandConfiguration(inspected, undefined, [inspected, inspected, undefined]) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: ["docker", "aws"] }, + }) + + expect(update).toHaveBeenCalledTimes(2) + expect(update).toHaveBeenCalledWith("deniedCommands", ["docker"], vscode.ConfigurationTarget.Workspace) + expect(update).toHaveBeenCalledWith("deniedCommands", ["aws"], vscode.ConfigurationTarget.Global) + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("deniedCommands", ["aws"]) + expect(vscode.window.showErrorMessage).not.toHaveBeenCalled() + }) + + it("reports partial failure when the compensating restore also fails", async () => { + // update() call order: workspace write succeeds, global write rejects, + // restore write rejects too. The raw workspace value differs from the + // post-split layer so the restore call is uniquely identifiable. + const update = mockCommandConfiguration({ workspaceValue: ["docker", "old"] }, [ + undefined, + Promise.reject(new Error("disk on fire")), + Promise.reject(new Error("restore also failed")), + ]) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: ["docker", "aws"] }, + }) + + // The restore was attempted at the owning target even though it failed. + expect(update).toHaveBeenCalledWith("deniedCommands", ["docker", "old"], vscode.ConfigurationTarget.Workspace) + expect(update).toHaveBeenCalledTimes(3) + expect(mockClineProvider.contextProxy.setValue).not.toHaveBeenCalled() + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith( + "Failed to save deniedCommands: disk on fire. The workspace-scoped restore also failed: Error: restore also failed. Review the command lists in your user and workspace settings.", + ) + // No exception escaped the handler: the state post still ran. + expect(mockClineProvider.postStateToWebview).toHaveBeenCalled() + }) }) describe("webviewMessageHandler - terminalProfile", () => { diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 374427f966..92c651d381 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -60,6 +60,7 @@ import { import { changeLanguage, t } from "../../i18n" import { Package } from "../../shared/package" import { + commandListConfiguration, readCommandListScopes, resolveWorkspaceWriteTarget, splitCommandListScopes, @@ -775,17 +776,69 @@ export const webviewMessageHandler = async ( currentScopes.global, ) const workspaceTarget = resolveWorkspaceWriteTarget(key) - const config = vscode.workspace.getConfiguration(Package.name) - - // Write the workspace layer first, at the target that owns it. - // When no workspace layer exists the write is skipped entirely - // so saving never materializes one; when entries were deleted - // the `[]` write clears them at that scope. - if (workspaceTarget !== undefined) { - await config.update(key, workspaceWrite, workspaceTarget) - } + const config = commandListConfiguration() + + // The two configuration writes below are not atomic, so the raw + // previous workspace value is captured before the write: a failed + // global write rewinds the workspace layer to exactly what the + // configuration held beforehand, including the entries this save + // drops. The raw inspected value is used rather than the sanitized + // layer so the rewind reproduces the file contents instead of a + // cleaned-up approximation of them. + let previousWorkspaceValue: string[] | undefined + let workspaceWriteApplied = false + try { + // Capture the raw previous value for the compensation path + // before anything is written, from the target that owns the + // workspace layer, so the rewind reproduces exactly what that + // target held. With no inspect result there is nothing + // trustworthy to rewind to; the layer can only have been + // created or changed by this save's own writes, so the + // compensation write clears it. + const previousInspected = config.inspect(key) + if (previousInspected !== undefined) { + previousWorkspaceValue = + workspaceTarget === vscode.ConfigurationTarget.WorkspaceFolder + ? previousInspected.workspaceFolderValue + : previousInspected.workspaceValue + } - await config.update(key, globalWrite, vscode.ConfigurationTarget.Global) + // Write the workspace layer first, at the target that owns it. + // When no workspace layer exists the write is skipped entirely + // so saving never materializes one; when entries were deleted + // the `[]` write clears them at that scope. + if (workspaceTarget !== undefined) { + await config.update(key, workspaceWrite, workspaceTarget) + workspaceWriteApplied = true + } + + await config.update(key, globalWrite, vscode.ConfigurationTarget.Global) + } catch (error) { + const failure = error instanceof Error ? error.message : String(error) + + // A failed workspace write leaves nothing changed, so only a + // global-write failure has a changed workspace layer to rewind, + // and the rewind goes to the same target the write used. + if (workspaceWriteApplied) { + try { + await config.update(key, previousWorkspaceValue, workspaceTarget) + } catch (restoreError) { + // The rewind itself failed: the configuration keeps the + // partially written values and the extension global-state + // write is skipped, so stored state never reflects a save + // that did not complete. The user hears about both failures. + vscode.window.showErrorMessage( + `Failed to save ${key}: ${failure}. The workspace-scoped restore also failed: ${restoreError}. Review the command lists in your user and workspace settings.`, + ) + continue + } + } + + // Only this key's save is abandoned; the remaining keys of the + // payload persist normally. + vscode.window.showErrorMessage(`Failed to save ${key}: ${failure}`) + continue + } // Extension global state keeps only the global layer: // enforcement re-adds the workspace layer through diff --git a/src/utils/__tests__/seedAllowedCommands.spec.ts b/src/utils/__tests__/seedAllowedCommands.spec.ts index 19c2623dfb..6467cb1ef8 100644 --- a/src/utils/__tests__/seedAllowedCommands.spec.ts +++ b/src/utils/__tests__/seedAllowedCommands.spec.ts @@ -2,6 +2,7 @@ import * as vscode from "vscode" +import { makeWorkspaceConfiguration } from "../../test-utils/vscode" import { seedAllowedCommands } from "../seedAllowedCommands" vi.mock("vscode", () => ({ @@ -23,16 +24,17 @@ interface ConfigDouble { } function mockConfiguration({ inspect, effective = [], inspectThrows = false }: ConfigDouble = {}) { - vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ - get: vi.fn().mockReturnValue(effective), - inspect: vi.fn().mockImplementation(() => { - if (inspectThrows) { - throw new Error("configuration access failed") - } - return inspect - }), - update: vi.fn(), - } as unknown as vscode.WorkspaceConfiguration) + const config = makeWorkspaceConfiguration() + // The shared double is scripted after creation so the per-test fixture + // values need no type assertion at the construction site. + vi.mocked(config.get).mockReturnValue(effective) + vi.mocked(config.inspect).mockImplementation(() => { + if (inspectThrows) { + throw new Error("configuration access failed") + } + return inspect + }) + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(config) } function mockMemento(existing: unknown = undefined) { @@ -41,9 +43,12 @@ function mockMemento(existing: unknown = undefined) { store.set("allowedCommands", existing) } + // Structurally complete Memento double (keys/get/update), so the call + // sites need no type assertion. return { - get: vi.fn((key: string) => store.get(key)), - update: vi.fn(async (key: string, value: unknown) => { + keys: vi.fn((): readonly string[] => [...store.keys()]), + get: vi.fn().mockImplementation((key: string) => store.get(key)), + update: vi.fn().mockImplementation(async (key: string, value: unknown) => { store.set(key, value) }), } @@ -64,7 +69,7 @@ describe("seedAllowedCommands", () => { }) const globalState = mockMemento() - await seedAllowedCommands(globalState as unknown as vscode.Memento) + await seedAllowedCommands(globalState) expect(globalState.update).not.toHaveBeenCalled() expect(globalState.get("allowedCommands")).toBeUndefined() @@ -74,7 +79,7 @@ describe("seedAllowedCommands", () => { mockConfiguration({ inspect: { key: "allowedCommands", globalValue: ["git"] }, effective: ["git"] }) const globalState = mockMemento() - await seedAllowedCommands(globalState as unknown as vscode.Memento) + await seedAllowedCommands(globalState) expect(globalState.update).toHaveBeenCalledExactlyOnceWith("allowedCommands", ["git"]) expect(globalState.get("allowedCommands")).toEqual(["git"]) @@ -84,17 +89,37 @@ describe("seedAllowedCommands", () => { mockConfiguration({ inspect: { key: "allowedCommands", globalValue: ["git"] }, effective: ["git"] }) const globalState = mockMemento(["npm"]) - await seedAllowedCommands(globalState as unknown as vscode.Memento) + await seedAllowedCommands(globalState) expect(globalState.update).not.toHaveBeenCalled() expect(globalState.get("allowedCommands")).toEqual(["npm"]) }) + it("inspects the global value under the extension's own section key", async () => { + // The seed must read the extension's own section; asking VS Code about + // a different (or empty) section key would silently never find the + // user's global list. + mockConfiguration({ inspect: { key: "allowedCommands", globalValue: ["git"] } }) + // Structurally complete Memento double (keys/get/update), so the call + // site needs no type assertion. + const globalState: vscode.Memento = { + keys: () => [], + get: vi.fn(), + update: vi.fn(), + } + + await seedAllowedCommands(globalState) + + const config = vi.mocked(vscode.workspace.getConfiguration).mock.results[0].value + expect(config.inspect).toHaveBeenCalledWith("allowedCommands") + expect(globalState.update).toHaveBeenCalledExactlyOnceWith("allowedCommands", ["git"]) + }) + it("leaves global state untouched when there is no inspect result at all", async () => { mockConfiguration({ inspect: undefined, effective: ["git"] }) const globalState = mockMemento() - await seedAllowedCommands(globalState as unknown as vscode.Memento) + await seedAllowedCommands(globalState) expect(globalState.update).not.toHaveBeenCalled() }) @@ -103,7 +128,7 @@ describe("seedAllowedCommands", () => { mockConfiguration({ inspectThrows: true }) const globalState = mockMemento() - await expect(seedAllowedCommands(globalState as unknown as vscode.Memento)).resolves.toBeUndefined() + await expect(seedAllowedCommands(globalState)).resolves.toBeUndefined() expect(globalState.update).not.toHaveBeenCalled() }) @@ -113,7 +138,7 @@ describe("seedAllowedCommands", () => { mockConfiguration({ inspect: { key: "allowedCommands", globalValue } }) const globalState = mockMemento() - await seedAllowedCommands(globalState as unknown as vscode.Memento) + await seedAllowedCommands(globalState) expect(globalState.update).toHaveBeenCalledWith("allowedCommands", expect.arrayContaining(["git"])) expect(globalState.update.mock.calls[0][1]).not.toBe(globalValue) diff --git a/src/utils/seedAllowedCommands.ts b/src/utils/seedAllowedCommands.ts index 6a56173e47..3deab4897b 100644 --- a/src/utils/seedAllowedCommands.ts +++ b/src/utils/seedAllowedCommands.ts @@ -19,18 +19,22 @@ export async function seedAllowedCommands(globalState: vscode.Memento): Promise< return } - let globalValue: string[] | undefined + // Only the configuration lookup itself is guarded: it can fail (for example + // before the workspace is ready), and skipping the seed then only defers it + // to a later activation. The `?.globalValue` read below stays outside the + // guarded region so a genuine lookup failure stays distinguishable from an + // absent value rather than being swallowed as one. + let inspected: { globalValue?: string[] } | undefined try { - globalValue = vscode.workspace.getConfiguration(Package.name).inspect("allowedCommands")?.globalValue + inspected = vscode.workspace.getConfiguration(Package.name).inspect("allowedCommands") } catch { - // Configuration access can fail (for example before the workspace is - // ready); skipping the seed only defers it to a later activation. return } // An absent global-scope value leaves global state untouched: seeding the // package.json default or a workspace override would defeat the point of // scoping the seed to the user's own settings. + const globalValue = inspected?.globalValue if (!Array.isArray(globalValue)) { return } From 95ed76acf885726b6dd6a67c086dc6163da6621d Mon Sep 17 00:00:00 2001 From: Franz Daubner Date: Wed, 14 Oct 2026 21:32:29 +0200 Subject: [PATCH 3/9] fix: derive command-list save scope from a single settings read (#1939) Saving an allowed/denied command list now inspects the configuration once per key and derives the workspace layer, the rollback value and the write target from that one snapshot, instead of three reads that could disagree. When the inspection is unavailable the key is skipped with a visible error and nothing is written for it, which also removes a path where a workspace-only entry could be re-copied into user settings; the other keys of the same save still persist. Regression tests cover the single-read invariant and both abort cases. --- .../__tests__/commandScope.spec.ts | 25 ++++++ src/core/auto-approval/commandScope.ts | 72 +++++++++++----- .../__tests__/webviewMessageHandler.spec.ts | 82 ++++++++++++++----- src/core/webview/webviewMessageHandler.ts | 72 +++++++++------- 4 files changed, 180 insertions(+), 71 deletions(-) diff --git a/src/core/auto-approval/__tests__/commandScope.spec.ts b/src/core/auto-approval/__tests__/commandScope.spec.ts index 58c48fbac3..f09c4ecad2 100644 --- a/src/core/auto-approval/__tests__/commandScope.spec.ts +++ b/src/core/auto-approval/__tests__/commandScope.spec.ts @@ -140,6 +140,19 @@ describe("readCommandListScopes", () => { expect(scopes.workspace).toEqual([]) expect(scopes.merged).toEqual(["git"]) }) + + it("takes the workspace layer from a caller-supplied snapshot without reading the configuration", () => { + // The save path threads one captured inspection through every + // derivation, so the snapshot is authoritative: the configuration's own + // value must never be consulted a second time. + mockConfiguration({ inspect: { key: "deniedCommands", workspaceValue: ["stale"] } }) + + const scopes = readCommandListScopes("deniedCommands", [], { workspaceValue: ["docker"] }) + + expect(scopes.workspace).toEqual(["docker"]) + expect(scopes.merged).toEqual(["docker"]) + expect(vscode.workspace.getConfiguration).not.toHaveBeenCalled() + }) }) describe("splitCommandListScopes", () => { @@ -261,6 +274,18 @@ describe("resolveWorkspaceWriteTarget", () => { expect(resolveWorkspaceWriteTarget("deniedCommands")).toBeUndefined() }) + it("resolves the target from a caller-supplied snapshot without reading the configuration", () => { + // The snapshot alone decides the target, so a folder value the live + // configuration does not report still selects the folder target — + // keeping split, write, and rollback on one captured state. + mockConfiguration({ inspect: { key: "deniedCommands", workspaceValue: ["docker"] } }) + + expect(resolveWorkspaceWriteTarget("deniedCommands", { workspaceFolderValue: ["pnpm docker"] })).toBe( + vscode.ConfigurationTarget.WorkspaceFolder, + ) + expect(vscode.workspace.getConfiguration).not.toHaveBeenCalled() + }) + it("resolves the target from the configuration scoped to the first workspace folder", () => { const folderA = makeUri("/mock/folder-a") const folderB = makeUri("/mock/folder-b") diff --git a/src/core/auto-approval/commandScope.ts b/src/core/auto-approval/commandScope.ts index 4778ac1f3e..d57e77bf7d 100644 --- a/src/core/auto-approval/commandScope.ts +++ b/src/core/auto-approval/commandScope.ts @@ -10,6 +10,17 @@ export interface CommandListScopes { merged: string[] } +/** + * The workspace-scoped layers of a command-list key as reported by + * `WorkspaceConfiguration.inspect()`. Capturing it once lets a save derive + * the read layers, the write target, and a rollback value from a single + * configuration state instead of three independent reads that could disagree. + */ +export interface CommandListInspection { + workspaceValue?: string[] + workspaceFolderValue?: string[] +} + /** * The configuration object for every command-list read and write, scoped to * the first workspace folder. Sharing one scoped access path keeps the two @@ -35,23 +46,33 @@ export function commandListConfiguration(): vscode.WorkspaceConfiguration { * * @param configKey - The extension configuration key to read. * @param globalList - The list persisted in extension global state. + * @param inspectionOverride - A snapshot captured by the caller before any + * write; when present it is used as-is and no fresh lookup happens, so a + * save's reads, target resolution, and rollback stay derived from one + * configuration state. Without it the lookup runs guarded (lenient): this is + * the enforcement/display path, where an unavailable workspace layer must + * degrade to empty rather than fail. * @returns The per-layer lists and their deduplicated set union. */ export function readCommandListScopes( configKey: CommandListConfigKey, globalList: string[] | undefined, + inspectionOverride?: CommandListInspection, ): CommandListScopes { const global = sanitizeCommandList(globalList) - // Only the configuration lookup itself is guarded: it can fail (for example - // before the workspace is ready), and an absent workspace layer must not - // break enforcement. The value reads below have no failure mode of their - // own, so the catch covers exactly the failure it exists to absorb. - let inspected: { workspaceValue?: string[]; workspaceFolderValue?: string[] } | undefined - try { - inspected = commandListConfiguration().inspect(configKey) - } catch { - inspected = undefined + let inspected: CommandListInspection | undefined = inspectionOverride + if (inspected === undefined) { + // Only the configuration lookup itself is guarded: it can fail (for + // example before the workspace is ready), and an absent workspace layer + // must not break enforcement. The value reads below have no failure mode + // of their own, so the catch covers exactly the failure it exists to + // absorb. + try { + inspected = commandListConfiguration().inspect(configKey) + } catch { + inspected = undefined + } } // The folder value wins over the workspace value, matching VS Code's @@ -116,11 +137,12 @@ export function splitCommandListScopes( /** * Picks the configuration target that owns the current workspace-scope layer - * for `configKey`, inspecting the same `commandListConfiguration()` object - * `readCommandListScopes` reads. Because that configuration is scoped to the - * first workspace folder, folder values resolve against that folder's - * settings; with no folder open the configuration is unscoped and reports no - * folder value, so no folder target can be selected. + * for `configKey`. Reading the layers runs through the same + * `commandListConfiguration()` object `readCommandListScopes` uses; because + * that configuration is scoped to the first workspace folder, folder values + * resolve against that folder's settings; with no folder open the + * configuration is unscoped and reports no folder value, so no folder target + * can be selected. * * - `workspaceFolderValue` set -> write at `WorkspaceFolder` scope. * - Otherwise `workspaceValue` set -> write at `Workspace` scope. @@ -128,19 +150,27 @@ export function splitCommandListScopes( * created (an empty workspace layer must not materialize in settings.json). * * @param configKey - The extension configuration key to inspect. + * @param inspectionOverride - A snapshot captured by the caller; when present + * it alone decides the target and no fresh lookup happens, keeping a save's + * split, write, and rollback on one configuration state. Without it the + * lookup is guarded leniently: a failure means no workspace layer exists. * @returns The workspace-scoped target to write, or `undefined` when no * workspace layer exists. */ export function resolveWorkspaceWriteTarget( configKey: CommandListConfigKey, + inspectionOverride?: CommandListInspection, ): vscode.ConfigurationTarget.WorkspaceFolder | vscode.ConfigurationTarget.Workspace | undefined { - // Same defensive posture as readCommandListScopes: only the configuration - // lookup itself may fail, and a failure means we create no workspace layer. - let inspected: { workspaceValue?: string[]; workspaceFolderValue?: string[] } | undefined - try { - inspected = commandListConfiguration().inspect(configKey) - } catch { - inspected = undefined + let inspected: CommandListInspection | undefined = inspectionOverride + if (inspected === undefined) { + // Same defensive posture as readCommandListScopes: only the + // configuration lookup itself may fail, and a failure means we create no + // workspace layer. + try { + inspected = commandListConfiguration().inspect(configKey) + } catch { + inspected = undefined + } } if (inspected?.workspaceFolderValue !== undefined) { diff --git a/src/core/webview/__tests__/webviewMessageHandler.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index ab906d9c45..af26431b4c 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -1453,7 +1453,8 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { // `updateSequence` / `inspectSequence` let a failure test script what // successive `update()` / `inspect()` calls return (each `update()` return // is awaited by the handler, so a rejected promise fails that write), in - // call order; a plain value resolves like the default mock. + // call order; a plain value resolves like the default mock, and an Error + // element makes that call throw. function mockCommandConfiguration( inspected: { globalValue?: string[] @@ -1469,7 +1470,13 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { } const inspect = vi.fn() if (inspectSequence) { - inspect.mockImplementation(() => inspectSequence.shift()) + inspect.mockImplementation(() => { + const next = inspectSequence.shift() + if (next instanceof Error) { + throw next + } + return next + }) } else { inspect.mockReturnValue(inspected) } @@ -1479,11 +1486,11 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { config.update = update config.inspect = inspect vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(config) - return update + return { update, inspect } } it("keeps workspace-scoped denied commands out of the global write when saving the merged list", async () => { - const update = mockCommandConfiguration({ workspaceValue: ["docker"] }) + const { update, inspect } = mockCommandConfiguration({ workspaceValue: ["docker"] }) await webviewMessageHandler(mockClineProvider, { type: "updateSettings", @@ -1497,10 +1504,14 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { // Extension global state holds only the global layer, so the next save // cannot double-home the workspace entry. expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("deniedCommands", ["aws"]) + // Exactly one snapshot per key: the scope read, write target, and + // rollback all derive from the same inspect(), so a helper re-reading + // the configuration would raise this count and could diverge. + expect(inspect).toHaveBeenCalledTimes(1) }) it("writes the workspace-folder value at the folder target in multi-root workspaces", async () => { - const update = mockCommandConfiguration({ workspaceValue: ["npm"], workspaceFolderValue: ["docker"] }) + const { update } = mockCommandConfiguration({ workspaceValue: ["npm"], workspaceFolderValue: ["docker"] }) await webviewMessageHandler(mockClineProvider, { type: "updateSettings", @@ -1517,7 +1528,7 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { }) it("clears a deleted workspace entry at the workspace scope without adding it to global", async () => { - const update = mockCommandConfiguration({ workspaceValue: ["docker"] }) + const { update } = mockCommandConfiguration({ workspaceValue: ["docker"] }) await webviewMessageHandler(mockClineProvider, { type: "updateSettings", @@ -1533,7 +1544,7 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { }) it("does not create a workspace layer when none exists", async () => { - const update = mockCommandConfiguration({}) + const { update } = mockCommandConfiguration({}) await webviewMessageHandler(mockClineProvider, { type: "updateSettings", @@ -1552,7 +1563,7 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { // The raw folder-layer value differs from the sanitized post-split layer // (and from the workspace-layer value), so the restore call is uniquely // identifiable and provably reads the folder value for a folder target. - const update = mockCommandConfiguration( + const { update } = mockCommandConfiguration( { workspaceValue: ["npm"], workspaceFolderValue: ["pnpm docker", "docker"] }, [undefined, Promise.reject(new Error("disk on fire"))], ) @@ -1586,7 +1597,9 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { // The first update() call is the workspace write; it rejects, so the // global write must never be attempted for this key. The rejection is a // plain string to pin the non-Error rendering of the failure message. - const update = mockCommandConfiguration({ workspaceValue: ["docker"] }, [Promise.reject("workspace locked")]) + const { update } = mockCommandConfiguration({ workspaceValue: ["docker"] }, [ + Promise.reject("workspace locked"), + ]) await webviewMessageHandler(mockClineProvider, { type: "updateSettings", @@ -1601,31 +1614,56 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("Failed to save deniedCommands: workspace locked") }) - it("saves both layers when the capture of the previous value finds no inspect result", async () => { - // inspect() call order: readCommandListScopes, resolveWorkspaceWriteTarget, - // then the compensation capture — only the capture call sees no result. - // A missing capture result must not abort the save: the writes proceed, - // and only a later failure could find nothing to restore. - const inspected = { workspaceValue: ["docker"] } - const update = mockCommandConfiguration(inspected, undefined, [inspected, inspected, undefined]) + it("aborts the key without writing when the configuration inspection has no result", async () => { + // The save derives scopes, write target, and rollback from one inspect() + // snapshot, so a missing snapshot means the workspace layer is unknown — + // proceeding would classify the retained workspace entry "docker" as new + // and re-copy it into global settings (issue #1939's leak). The key is + // aborted instead: no configuration write, no extension global state. + const { update } = mockCommandConfiguration({ workspaceValue: ["docker"] }, undefined, [undefined]) await webviewMessageHandler(mockClineProvider, { type: "updateSettings", updatedSettings: { deniedCommands: ["docker", "aws"] }, }) - expect(update).toHaveBeenCalledTimes(2) - expect(update).toHaveBeenCalledWith("deniedCommands", ["docker"], vscode.ConfigurationTarget.Workspace) - expect(update).toHaveBeenCalledWith("deniedCommands", ["aws"], vscode.ConfigurationTarget.Global) - expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("deniedCommands", ["aws"]) - expect(vscode.window.showErrorMessage).not.toHaveBeenCalled() + expect(update).not.toHaveBeenCalled() + // Leak pin: the workspace entry must not land in the global layer. + expect(update).not.toHaveBeenCalledWith("deniedCommands", ["docker", "aws"], vscode.ConfigurationTarget.Global) + expect(mockClineProvider.contextProxy.setValue).not.toHaveBeenCalled() + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith( + "Failed to save deniedCommands: the configuration value could not be read", + ) + // No exception escaped the handler: the state post still ran. + expect(mockClineProvider.postStateToWebview).toHaveBeenCalled() + }) + + it("aborts the command-list key when the inspection throws, while other keys of the payload still save", async () => { + const { update } = mockCommandConfiguration({ workspaceValue: ["docker"] }, undefined, [ + new Error("configuration unavailable"), + ]) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + // A second key pins that only the aborted command-list key is skipped. + updatedSettings: { deniedCommands: ["docker", "aws"], alwaysDenyUnapprovedCommands: true }, + }) + + // The aborted key wrote nothing at any scope... + expect(update).not.toHaveBeenCalled() + expect(mockClineProvider.contextProxy.setValue).not.toHaveBeenCalledWith("deniedCommands", expect.anything()) + // ...while the other key of the same payload still persists. + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("alwaysDenyUnapprovedCommands", true) + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith( + "Failed to save deniedCommands: configuration unavailable", + ) }) it("reports partial failure when the compensating restore also fails", async () => { // update() call order: workspace write succeeds, global write rejects, // restore write rejects too. The raw workspace value differs from the // post-split layer so the restore call is uniquely identifiable. - const update = mockCommandConfiguration({ workspaceValue: ["docker", "old"] }, [ + const { update } = mockCommandConfiguration({ workspaceValue: ["docker", "old"] }, [ undefined, Promise.reject(new Error("disk on fire")), Promise.reject(new Error("restore also failed")), diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 92c651d381..6d198f57fe 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -60,6 +60,7 @@ import { import { changeLanguage, t } from "../../i18n" import { Package } from "../../shared/package" import { + type CommandListInspection, commandListConfiguration, readCommandListScopes, resolveWorkspaceWriteTarget, @@ -766,43 +767,58 @@ export const webviewMessageHandler = async ( // The webview always sends the full merged list (it displays // the union of the global-state and workspace-scope layers). - // Read both layers BEFORE any write and split the incoming - // list by origin scope, so saving the dialog cannot flatten - // workspace-scoped entries into the user's global settings. - const currentScopes = readCommandListScopes(key, getGlobalState(key)) + // Splitting that list by origin scope needs the workspace + // layer, the write target, and the rollback value to describe + // the SAME configuration state, so they are all derived from + // one inspect() snapshot taken before any write. Without a + // snapshot the key's save is aborted rather than guessed at: + // reading the workspace layer as empty would classify a + // workspace-scoped entry as brand new and re-copy it into + // global settings. + // Only the snapshot read is guarded: a failing + // commandListConfiguration() lookup propagates instead of being + // downgraded to a per-key save failure. + const config = commandListConfiguration() + let inspection: CommandListInspection | undefined + let inspectionFailure: string | undefined + try { + inspection = config.inspect(key) + } catch (error) { + inspectionFailure = error instanceof Error ? error.message : String(error) + } + if (inspection === undefined) { + // No snapshot: the workspace layer could only be guessed + // at, and a guessed-empty layer re-copies workspace entries + // into global settings, so this key's save is abandoned + // while the remaining keys of the payload persist normally. + vscode.window.showErrorMessage( + `Failed to save ${key}: ${inspectionFailure ?? "the configuration value could not be read"}`, + ) + continue + } + + const currentScopes = readCommandListScopes(key, getGlobalState(key), inspection) const { globalWrite, workspaceWrite } = splitCommandListScopes( incoming, currentScopes.workspace, currentScopes.global, ) - const workspaceTarget = resolveWorkspaceWriteTarget(key) - const config = commandListConfiguration() + const workspaceTarget = resolveWorkspaceWriteTarget(key, inspection) // The two configuration writes below are not atomic, so the raw - // previous workspace value is captured before the write: a failed - // global write rewinds the workspace layer to exactly what the - // configuration held beforehand, including the entries this save - // drops. The raw inspected value is used rather than the sanitized - // layer so the rewind reproduces the file contents instead of a - // cleaned-up approximation of them. - let previousWorkspaceValue: string[] | undefined + // previous workspace value comes from the same snapshot the split + // used, taken from the target that owns the workspace layer: a + // failed global write rewinds that target to exactly what it held + // beforehand, including the entries this save drops. The raw + // inspected value is used rather than the sanitized layer so the + // rewind reproduces the file contents instead of a cleaned-up + // approximation of them. + const previousWorkspaceValue = + workspaceTarget === vscode.ConfigurationTarget.WorkspaceFolder + ? inspection.workspaceFolderValue + : inspection.workspaceValue let workspaceWriteApplied = false try { - // Capture the raw previous value for the compensation path - // before anything is written, from the target that owns the - // workspace layer, so the rewind reproduces exactly what that - // target held. With no inspect result there is nothing - // trustworthy to rewind to; the layer can only have been - // created or changed by this save's own writes, so the - // compensation write clears it. - const previousInspected = config.inspect(key) - if (previousInspected !== undefined) { - previousWorkspaceValue = - workspaceTarget === vscode.ConfigurationTarget.WorkspaceFolder - ? previousInspected.workspaceFolderValue - : previousInspected.workspaceValue - } - // Write the workspace layer first, at the target that owns it. // When no workspace layer exists the write is skipped entirely // so saving never materializes one; when entries were deleted From 7c294cc9ac5d1573d50418eef2eae94350b33549 Mon Sep 17 00:00:00 2001 From: Franz Daubner Date: Thu, 15 Oct 2026 00:16:04 +0200 Subject: [PATCH 4/9] fix: restore default command allowlist on fresh installs (#1939) Fresh installs keep the built-in git command defaults again: with no user, workspace, or folder value set, the declared default list is seeded into global state. A workspace or folder override still wins and nothing is written over it. Scope layers are read through the folder-aware settings lookup shared with the command-list paths. --- src/__tests__/extension.spec.ts | 4 + .../__tests__/seedAllowedCommands.spec.ts | 152 ++++++++++++++++-- src/utils/seedAllowedCommands.ts | 55 +++++-- 3 files changed, 181 insertions(+), 30 deletions(-) diff --git a/src/__tests__/extension.spec.ts b/src/__tests__/extension.spec.ts index 3b52363763..6a2e2596dd 100644 --- a/src/__tests__/extension.spec.ts +++ b/src/__tests__/extension.spec.ts @@ -420,6 +420,10 @@ describe("extension.ts", () => { ), inspect: vi.fn().mockReturnValue({ key: "allowedCommands", + // Real inspect() always reports the declared package default; + // including it here lets the workspace-override test prove the + // default stays unseeded beside an override, not just absent. + defaultValue: ["git log", "git diff", "git show"], globalValue, workspaceValue, }), diff --git a/src/utils/__tests__/seedAllowedCommands.spec.ts b/src/utils/__tests__/seedAllowedCommands.spec.ts index 6467cb1ef8..a62ce469a9 100644 --- a/src/utils/__tests__/seedAllowedCommands.spec.ts +++ b/src/utils/__tests__/seedAllowedCommands.spec.ts @@ -2,11 +2,22 @@ import * as vscode from "vscode" -import { makeWorkspaceConfiguration } from "../../test-utils/vscode" +import { Package } from "../../shared/package" +import { makeUri, makeWorkspaceConfiguration } from "../../test-utils/vscode" import { seedAllowedCommands } from "../seedAllowedCommands" +// Scriptable stand-in for `vscode.workspace.workspaceFolders`: the module +// mock's getter reads through this object, so a test can change the open +// folders without re-mocking the module. +const workspaceState = vi.hoisted(() => ({ + workspaceFolders: undefined as { uri: unknown }[] | undefined, +})) + vi.mock("vscode", () => ({ workspace: { + get workspaceFolders() { + return workspaceState.workspaceFolders + }, getConfiguration: vi.fn(), }, })) @@ -24,17 +35,31 @@ interface ConfigDouble { } function mockConfiguration({ inspect, effective = [], inspectThrows = false }: ConfigDouble = {}) { - const config = makeWorkspaceConfiguration() - // The shared double is scripted after creation so the per-test fixture - // values need no type assertion at the construction site. - vi.mocked(config.get).mockReturnValue(effective) - vi.mocked(config.inspect).mockImplementation(() => { - if (inspectThrows) { - throw new Error("configuration access failed") - } - return inspect - }) - vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(config) + // Real VS Code reports `workspaceFolderValue` only for a configuration + // looked up against a resource; an unscoped lookup reports no folder + // layer no matter what any folder's settings contain. Scripting that + // split per call is what lets these tests tell a scoped lookup apart + // from an unscoped one. + vi.mocked(vscode.workspace.getConfiguration).mockImplementation( + (_section?: string, scope?: vscode.ConfigurationScope | null) => { + const config = makeWorkspaceConfiguration() + // The shared double is scripted per call so the per-test fixture + // values need no type assertion at the construction site. + vi.mocked(config.get).mockReturnValue(effective) + vi.mocked(config.inspect).mockImplementation(() => { + if (inspectThrows) { + throw new Error("configuration access failed") + } + if (inspect === undefined) { + return undefined + } + // No resource to resolve against => no folder layer, matching + // the real contract for unscoped (and explicit-null) lookups. + return scope === undefined || scope === null ? { ...inspect, workspaceFolderValue: undefined } : inspect + }) + return config + }, + ) } function mockMemento(existing: unknown = undefined) { @@ -57,14 +82,23 @@ function mockMemento(existing: unknown = undefined) { describe("seedAllowedCommands", () => { beforeEach(() => { vi.clearAllMocks() + workspaceState.workspaceFolders = undefined }) it("leaves global state unset when only a workspace-scoped value exists", async () => { // A real VS Code get() folds the workspace override into the effective // value, so this is the leak scenario: reading get() would seed - // ["docker"] (from .vscode/settings.json) into global state. + // ["docker"] (from .vscode/settings.json) into global state. The + // defaultValue mirrors real VS Code, where the declared default is + // always present: the workspace override replaced it deliberately, so + // re-granting it from global state is also forbidden. mockConfiguration({ - inspect: { key: "allowedCommands", globalValue: undefined, workspaceValue: ["docker"] }, + inspect: { + key: "allowedCommands", + defaultValue: ["git log", "git diff", "git show"], + globalValue: undefined, + workspaceValue: ["docker"], + }, effective: ["docker"], }) const globalState = mockMemento() @@ -75,6 +109,92 @@ describe("seedAllowedCommands", () => { expect(globalState.get("allowedCommands")).toBeUndefined() }) + it("leaves global state unset when only a workspace-folder-scoped value exists", async () => { + // A folder override is visible only to a lookup resolved against that + // folder's resource, so this fixture opens a folder and scripts the + // folder layer (which the resource-aware double reports only for the + // scoped call). An unscoped seed would read "no value anywhere" here + // and re-grant the default the folder removed — this test fails on + // that regression. + const folderA = makeUri("/mock/folder-a") + workspaceState.workspaceFolders = [{ uri: folderA }] + mockConfiguration({ + inspect: { + key: "allowedCommands", + defaultValue: ["git log", "git diff", "git show"], + globalValue: undefined, + workspaceFolderValue: ["docker"], + }, + }) + const globalState = mockMemento() + + await seedAllowedCommands(globalState) + + // The folder layer only reaches the seed through a resource-scoped + // lookup, so the seed must request one (same pin as the save path). + expect(vscode.workspace.getConfiguration).toHaveBeenCalledWith(Package.name, folderA) + expect(globalState.update).not.toHaveBeenCalled() + expect(globalState.get("allowedCommands")).toBeUndefined() + }) + + it("resolves the folder layer against the first workspace folder only", async () => { + // Reading any other folder's configuration would silently decide the + // seed from the wrong folder's settings file. + const folderA = makeUri("/mock/folder-a") + const folderB = makeUri("/mock/folder-b") + workspaceState.workspaceFolders = [{ uri: folderA }, { uri: folderB }] + mockConfiguration({ + inspect: { + key: "allowedCommands", + defaultValue: ["git log", "git diff", "git show"], + workspaceFolderValue: ["docker"], + }, + }) + const globalState = mockMemento() + + await seedAllowedCommands(globalState) + + expect(vscode.workspace.getConfiguration).toHaveBeenCalledWith(Package.name, folderA) + expect(vscode.workspace.getConfiguration).not.toHaveBeenCalledWith(Package.name, folderB) + expect(globalState.update).not.toHaveBeenCalled() + }) + + it("treats an explicitly empty workspace value as an override that suppresses the default", async () => { + // Presence is checked with `undefined`, not truthiness: a workspace that + // set the list to [] opted out of the default as deliberately as one + // that set ["docker"]. + mockConfiguration({ + inspect: { + key: "allowedCommands", + defaultValue: ["git log", "git diff", "git show"], + globalValue: undefined, + workspaceValue: [], + }, + }) + const globalState = mockMemento() + + await seedAllowedCommands(globalState) + + expect(globalState.update).not.toHaveBeenCalled() + expect(globalState.get("allowedCommands")).toBeUndefined() + }) + + it("seeds the declared default on a fresh install with no value set at any scope", async () => { + const defaultValue = ["git log", "git diff", "git show"] + mockConfiguration({ + inspect: { key: "allowedCommands", defaultValue }, + effective: defaultValue, + }) + const globalState = mockMemento() + + await seedAllowedCommands(globalState) + + expect(globalState.update).toHaveBeenCalledExactlyOnceWith("allowedCommands", defaultValue) + // Copy, like the global-value path: global state must not share a + // reference with the configuration object. + expect(globalState.update.mock.calls[0][1]).not.toBe(defaultValue) + }) + it("seeds from the global-scope value when global state is empty", async () => { mockConfiguration({ inspect: { key: "allowedCommands", globalValue: ["git"] }, effective: ["git"] }) const globalState = mockMemento() @@ -98,7 +218,8 @@ describe("seedAllowedCommands", () => { it("inspects the global value under the extension's own section key", async () => { // The seed must read the extension's own section; asking VS Code about // a different (or empty) section key would silently never find the - // user's global list. + // user's global list. With no folder open there is no resource to + // scope against, so the explicit unscoped call is (section, undefined). mockConfiguration({ inspect: { key: "allowedCommands", globalValue: ["git"] } }) // Structurally complete Memento double (keys/get/update), so the call // site needs no type assertion. @@ -110,6 +231,7 @@ describe("seedAllowedCommands", () => { await seedAllowedCommands(globalState) + expect(vscode.workspace.getConfiguration).toHaveBeenCalledWith(Package.name, undefined) const config = vi.mocked(vscode.workspace.getConfiguration).mock.results[0].value expect(config.inspect).toHaveBeenCalledWith("allowedCommands") expect(globalState.update).toHaveBeenCalledExactlyOnceWith("allowedCommands", ["git"]) diff --git a/src/utils/seedAllowedCommands.ts b/src/utils/seedAllowedCommands.ts index 3deab4897b..1afa0bbfcb 100644 --- a/src/utils/seedAllowedCommands.ts +++ b/src/utils/seedAllowedCommands.ts @@ -1,16 +1,30 @@ import * as vscode from "vscode" -import { Package } from "../shared/package" +import { commandListConfiguration } from "../core/auto-approval/commandScope" /** * Seeds the extension's global-state allowlist from the `allowedCommands` * setting, but only when global state holds no list yet (first run). * - * The seed reads the global-scope value via `inspect()` rather than the - * effective `get()` on purpose: `get()` folds in workspace-scoped overrides, - * so a project's `.vscode/settings.json` would otherwise be copied - * permanently into global state the first time the extension activates inside - * that project — a leak that survives closing the project. + * The seed reads scoped values via `inspect()` rather than the effective + * `get()` on purpose: `get()` folds in workspace-scoped overrides, so a + * project's `.vscode/settings.json` would otherwise be copied permanently + * into global state the first time the extension activates inside that + * project — a leak that survives closing the project. The lookup goes + * through `commandListConfiguration()`, the folder-scoped accessor shared + * with the command-list read and write paths: only a configuration resolved + * against a resource reports `workspaceFolderValue` at all, so the folder + * layer is visible here exactly where enforcement can see it. In a + * multi-root workspace only the first folder is in scope; with no folder + * open the configuration is unscoped and reports no folder value. + * + * Selection: the global-scope value wins. With nothing set at any scope (a + * fresh install) the declared package default is seeded, because the + * enforcement path reads only global state plus workspace scopes and never + * falls back to that default on its own. A workspace-scoped value is an + * explicit choice and suppresses the default: array settings replace across + * scopes, so seeding it would re-grant the auto-approvals the override + * removed. * * @param globalState - The extension global-state memento to seed. */ @@ -21,24 +35,35 @@ export async function seedAllowedCommands(globalState: vscode.Memento): Promise< // Only the configuration lookup itself is guarded: it can fail (for example // before the workspace is ready), and skipping the seed then only defers it - // to a later activation. The `?.globalValue` read below stays outside the + // to a later activation. The `inspected` reads below stay outside the // guarded region so a genuine lookup failure stays distinguishable from an // absent value rather than being swallowed as one. - let inspected: { globalValue?: string[] } | undefined + let inspected: + | { + globalValue?: string[] + defaultValue?: string[] + workspaceValue?: string[] + workspaceFolderValue?: string[] + } + | undefined try { - inspected = vscode.workspace.getConfiguration(Package.name).inspect("allowedCommands") + inspected = commandListConfiguration().inspect("allowedCommands") } catch { return } - // An absent global-scope value leaves global state untouched: seeding the - // package.json default or a workspace override would defeat the point of - // scoping the seed to the user's own settings. - const globalValue = inspected?.globalValue - if (!Array.isArray(globalValue)) { + // The `undefined` checks, not truthiness: an explicitly empty array still + // counts as an override. + const seedValue = + inspected?.globalValue ?? + (inspected?.workspaceValue === undefined && inspected?.workspaceFolderValue === undefined + ? inspected?.defaultValue + : undefined) + + if (!Array.isArray(seedValue)) { return } // Copy so global state never shares a reference with the configuration object. - await globalState.update("allowedCommands", [...globalValue]) + await globalState.update("allowedCommands", [...seedValue]) } From 0cf1628953271db530d2d75fbaf59bd5b7ac11e7 Mon Sep 17 00:00:00 2001 From: Franz Daubner Date: Thu, 15 Oct 2026 11:11:32 +0200 Subject: [PATCH 5/9] test: pin command-list seed and settings-lookup edge cases (#1939) Add three tests, no production change: an explicitly empty seeded allowlist is left untouched instead of re-seeded, a settings lookup that cannot return aborts the whole update-settings message instead of partially applying it, and a workspace override added after a fresh-install seed composes as a union with the seeded defaults. --- .../__tests__/webviewMessageHandler.spec.ts | 26 ++++++++++ .../__tests__/seedAllowedCommands.spec.ts | 51 +++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/src/core/webview/__tests__/webviewMessageHandler.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index af26431b4c..1a3ffaa3ae 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -1659,6 +1659,32 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { ) }) + it("propagates a configuration lookup failure instead of degrading it to a per-key abort", async () => { + // The commandListConfiguration() lookup sits outside the guarded + // snapshot read on purpose: a lookup that cannot return a configuration + // object at all is a different failure class than an unreadable value, + // so it aborts the entire message rather than the one key. + vi.mocked(vscode.workspace.getConfiguration).mockImplementation(() => { + throw new Error("workspace not ready") + }) + + await expect( + webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + // A second key pins that the whole message aborts, not just the + // command-list key: with the guarded per-key failure paths this + // key would still persist. + updatedSettings: { deniedCommands: ["docker", "aws"], alwaysDenyUnapprovedCommands: true }, + }), + ).rejects.toThrow("workspace not ready") + + // Nothing persisted for any key, no per-key error handling ran, and the + // state post was skipped — the rejection surfaced to the caller instead. + expect(mockClineProvider.contextProxy.setValue).not.toHaveBeenCalled() + expect(vscode.window.showErrorMessage).not.toHaveBeenCalled() + expect(mockClineProvider.postStateToWebview).not.toHaveBeenCalled() + }) + it("reports partial failure when the compensating restore also fails", async () => { // update() call order: workspace write succeeds, global write rejects, // restore write rejects too. The raw workspace value differs from the diff --git a/src/utils/__tests__/seedAllowedCommands.spec.ts b/src/utils/__tests__/seedAllowedCommands.spec.ts index a62ce469a9..50c1dd5fc6 100644 --- a/src/utils/__tests__/seedAllowedCommands.spec.ts +++ b/src/utils/__tests__/seedAllowedCommands.spec.ts @@ -2,6 +2,7 @@ import * as vscode from "vscode" +import { readCommandListScopes } from "../../core/auto-approval/commandScope" import { Package } from "../../shared/package" import { makeUri, makeWorkspaceConfiguration } from "../../test-utils/vscode" import { seedAllowedCommands } from "../seedAllowedCommands" @@ -215,6 +216,56 @@ describe("seedAllowedCommands", () => { expect(globalState.get("allowedCommands")).toEqual(["npm"]) }) + it("leaves an explicitly empty global-state list untouched", async () => { + // The guard treats presence, not emptiness: saving [] is a deliberate + // choice to auto-approve nothing, and re-seeding the package default + // would silently re-grant the approvals the user just revoked. + mockConfiguration({ inspect: { key: "allowedCommands", defaultValue: ["git log", "git diff", "git show"] } }) + const globalState = mockMemento([]) + + await seedAllowedCommands(globalState) + + expect(globalState.update).not.toHaveBeenCalled() + expect(globalState.get("allowedCommands")).toEqual([]) + }) + + it("composes a later workspace override as a union with the seeded defaults", async () => { + // Full schedule: a fresh activation seeds the declared default, then a + // workspace override appears in a later activation. The seed + // early-returns because global state already holds a list, so the + // override is never persisted — and the enforcement read composes the + // two layers as a set union. Neither list replaces the other; the + // longest-prefix matcher decides which entry governs a given command. + const defaultValue = ["git log", "git diff", "git show"] + mockConfiguration({ + inspect: { key: "allowedCommands", defaultValue }, + effective: defaultValue, + }) + const globalState = mockMemento() + + await seedAllowedCommands(globalState) + expect(globalState.update).toHaveBeenCalledExactlyOnceWith("allowedCommands", defaultValue) + + // A later activation in a workspace whose settings file sets the list: + // the guard early-returns before any inspection, so no second write. + mockConfiguration({ + inspect: { key: "allowedCommands", defaultValue, workspaceValue: ["docker ps"] }, + effective: ["docker ps"], + }) + + await seedAllowedCommands(globalState) + expect(globalState.update).toHaveBeenCalledTimes(1) + expect(globalState.get("allowedCommands")).toEqual(defaultValue) + + // The enforcement view (ClineProvider.getState) keeps both layers and + // merges them: the override adds entries, it does not replace the + // seeded defaults, and vice versa. + const scopes = readCommandListScopes("allowedCommands", globalState.get("allowedCommands")) + expect(scopes.global).toEqual(defaultValue) + expect(scopes.workspace).toEqual(["docker ps"]) + expect(scopes.merged).toEqual([...defaultValue, "docker ps"]) + }) + it("inspects the global value under the extension's own section key", async () => { // The seed must read the extension's own section; asking VS Code about // a different (or empty) section key would silently never find the From 474dfd6936677a68c0debb0f6acb9ca0304b593a Mon Sep 17 00:00:00 2001 From: Franz Daubner Date: Sun, 1 Nov 2026 18:21:23 +0100 Subject: [PATCH 6/9] fix: gate workspace command lists and give user-level entries precedence (#1939) A user-level command-list entry now decides any command it matches: an entry from a workspace's .vscode/settings.json applies only where no user-level entry (allow or deny) matches, in both directions. The whole workspace layer sits behind a new setting, zoo-code.workspaceCommandListsEnabled, default off. It is declared with "scope": "application" in the extension manifest, so only user settings resolve it and a repository file cannot enable it; the settings save path also refuses a gate value supplied by the webview. Entries hidden while the gate is off are hidden from the approval dialog as well, and saving from the dialog preserves the workspace entries it does not show. The setting's description resolves from the nls bundles, translated for all 17 locales. Covers allow-side state propagation in tests. Addresses reviewer feedback on #2056. --- packages/types/src/global-settings.ts | 19 ++ packages/types/src/vscode-extension-host.ts | 1 + .../__tests__/commandScope.spec.ts | 131 +++++++++++++- .../auto-approval/__tests__/commands.spec.ts | 87 ++++++++- src/core/auto-approval/commandScope.ts | 75 ++++++++ src/core/auto-approval/commands.ts | 28 +++ src/core/webview/ClineProvider.ts | 33 +++- .../webview/__tests__/ClineProvider.spec.ts | 88 ++++++++- .../__tests__/webviewMessageHandler.spec.ts | 170 ++++++++++++++++-- src/core/webview/webviewMessageHandler.ts | 34 +++- src/package.json | 7 + src/package.nls.ca.json | 3 +- src/package.nls.de.json | 3 +- src/package.nls.es.json | 3 +- src/package.nls.fr.json | 3 +- src/package.nls.hi.json | 3 +- src/package.nls.id.json | 3 +- src/package.nls.it.json | 3 +- src/package.nls.ja.json | 3 +- src/package.nls.json | 3 +- src/package.nls.ko.json | 3 +- src/package.nls.nl.json | 3 +- src/package.nls.pl.json | 3 +- src/package.nls.pt-BR.json | 3 +- src/package.nls.ru.json | 3 +- src/package.nls.tr.json | 3 +- src/package.nls.vi.json | 3 +- src/package.nls.zh-CN.json | 3 +- src/package.nls.zh-TW.json | 3 +- 29 files changed, 681 insertions(+), 46 deletions(-) diff --git a/packages/types/src/global-settings.ts b/packages/types/src/global-settings.ts index 54ae7af089..9cca42c877 100644 --- a/packages/types/src/global-settings.ts +++ b/packages/types/src/global-settings.ts @@ -49,6 +49,17 @@ export const DEFAULT_DIFF_FUZZY_THRESHOLD = 1.0 export const DEFAULT_DESTRUCTIVE_COMMAND_GUARD_ENABLED = false +/** + * Whether workspace-scoped command-list layers (`zoo-code.allowedCommands` / + * `zoo-code.deniedCommands` values set in workspace settings) participate in + * command auto-approval at all. Opt-in: while off, workspace-layer entries + * contribute nothing to enforcement or display. The manifest declares the key + * with `scope: "application"`, so only user settings can change it; this + * constant is the single default shared by every consumer reading the + * setting. + */ +export const DEFAULT_WORKSPACE_COMMAND_LISTS_ENABLED = false + /** * Whether commands that are not explicitly auto-approved are automatically * denied (with a structured reason sent to the model) instead of prompting the @@ -175,6 +186,14 @@ export const globalSettingsSchema = z.object({ followupAutoApproveTimeoutMs: z.number().optional(), allowedCommands: z.array(z.string()).optional(), deniedCommands: z.array(z.string()).optional(), + /** + * Gate for the workspace-scoped command-list layers. While false (the + * default), workspace-set `allowedCommands`/`deniedCommands` entries + * contribute nothing; when true they are composed under the global layer, + * which outranks them wherever it matches a command. + * @default false + */ + workspaceCommandListsEnabled: z.boolean().optional(), commandExecutionTimeout: z.number().optional(), commandTimeoutAllowlist: z.array(z.string()).optional(), preventCompletionWithOpenTodos: z.boolean().optional(), diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index 95316f5e74..689066ccd0 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -287,6 +287,7 @@ export type ExtensionState = Pick< | "followupAutoApproveTimeoutMs" | "allowedCommands" | "deniedCommands" + | "workspaceCommandListsEnabled" | "allowedMaxRequests" | "allowedMaxCost" | "ttsEnabled" diff --git a/src/core/auto-approval/__tests__/commandScope.spec.ts b/src/core/auto-approval/__tests__/commandScope.spec.ts index f09c4ecad2..4d1064d23d 100644 --- a/src/core/auto-approval/__tests__/commandScope.spec.ts +++ b/src/core/auto-approval/__tests__/commandScope.spec.ts @@ -1,8 +1,18 @@ +import * as fs from "fs" +import * as path from "path" + +import { DEFAULT_WORKSPACE_COMMAND_LISTS_ENABLED, globalSettingsSchema } from "@roo-code/types" import * as vscode from "vscode" import { Package } from "../../../shared/package" import { makeUri, makeWorkspaceConfiguration } from "../../../test-utils/vscode" -import { readCommandListScopes, resolveWorkspaceWriteTarget, splitCommandListScopes } from "../commandScope" +import { + readCommandListScopes, + resolveEffectiveCommandLists, + resolveWorkspaceWriteTarget, + splitCommandListScopes, + workspaceCommandListsEnabled, +} from "../commandScope" // Scriptable stand-in for `vscode.workspace.workspaceFolders`: the module // mock's getter reads through this object, so a test can change the open @@ -312,3 +322,122 @@ describe("resolveWorkspaceWriteTarget", () => { }, ) }) + +function mockGateConfiguration(gate: unknown) { + const config = makeWorkspaceConfiguration() + vi.mocked(config.get).mockReturnValue(gate) + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(config) + return config +} + +// Raw per-key layers in the shape readCommandListScopes returns them. +const listScopes = (globalList: string[], workspaceList: string[]) => ({ + global: globalList, + workspace: workspaceList, + merged: [...new Set([...globalList, ...workspaceList])], +}) + +it("workspaceCommandListsEnabled: enables the workspace layers only when the gate configuration reads true", () => { + vi.clearAllMocks() + const config = mockGateConfiguration(true) + + expect(workspaceCommandListsEnabled()).toBe(true) + expect(config.get).toHaveBeenCalledWith("workspaceCommandListsEnabled") +}) + +it("workspaceCommandListsEnabled: keeps the workspace layers disabled when the gate lookup throws", () => { + vi.clearAllMocks() + // Fail-closed: a broken configuration read must never widen what the + // workspace layer can auto-approve. + vi.mocked(vscode.workspace.getConfiguration).mockImplementation(() => { + throw new Error("configuration unavailable") + }) + + expect(workspaceCommandListsEnabled()).toBe(DEFAULT_WORKSPACE_COMMAND_LISTS_ENABLED) + expect(workspaceCommandListsEnabled()).toBe(false) +}) + +// The manifest declaration is what makes the gate workspace-unsettable; +// VS Code itself refusing a workspace-layer value is platform behavior. +it("manifest: declares the gate as a default-off, application-scoped boolean", () => { + const manifest = JSON.parse(fs.readFileSync(path.join(__dirname, "../../../package.json"), "utf8")) + const gate = manifest.contributes.configuration.properties["zoo-code.workspaceCommandListsEnabled"] + + expect(gate.type).toBe("boolean") + expect(gate.default).toBe(false) + expect(gate.scope).toBe("application") +}) + +it("manifest: keeps the command-list keys workspace-settable so the gated layer can exist", () => { + // The gate scopes the workspace layers' effect; scoping the list keys + // themselves would remove the workspace layer entirely. + const manifest = JSON.parse(fs.readFileSync(path.join(__dirname, "../../../package.json"), "utf8")) + const properties = manifest.contributes.configuration.properties + + expect(properties["zoo-code.allowedCommands"].scope).toBeUndefined() + expect(properties["zoo-code.deniedCommands"].scope).toBeUndefined() +}) + +it("persisted-settings schema: defaults the gate to disabled", () => { + expect(globalSettingsSchema.parse({}).workspaceCommandListsEnabled).toBeUndefined() + expect(globalSettingsSchema.parse({ workspaceCommandListsEnabled: true }).workspaceCommandListsEnabled).toBe(true) + expect(DEFAULT_WORKSPACE_COMMAND_LISTS_ENABLED).toBe(false) +}) + +it("resolveEffectiveCommandLists: hides every workspace entry, allow and deny alike, while the gate is disabled", () => { + const effective = resolveEffectiveCommandLists(listScopes([], ["docker"]), listScopes([], ["rm"]), false) + + expect(effective.allowed).toEqual([]) + expect(effective.denied).toEqual([]) + expect(effective.hiddenWorkspaceAllow).toEqual(["docker"]) + expect(effective.hiddenWorkspaceDeny).toEqual(["rm"]) +}) + +it("resolveEffectiveCommandLists: treats unshadowed workspace entries as the sole layer when the global layer is empty", () => { + const effective = resolveEffectiveCommandLists(listScopes(["git"], ["docker", "aws"]), listScopes([], ["rm"]), true) + + expect(effective.allowed).toEqual(["git", "docker", "aws"]) + expect(effective.denied).toEqual(["rm"]) + expect(effective.hiddenWorkspaceAllow).toEqual([]) + expect(effective.hiddenWorkspaceDeny).toEqual([]) +}) + +it("resolveEffectiveCommandLists: lets a global allow entry decide a command a longer workspace deny entry also matches", () => { + // Cross-directional shadowing: the workspace deny entry cannot decide + // anything the global allow layer already covers, so it never reaches + // the matcher. + const effective = resolveEffectiveCommandLists(listScopes(["git"], []), listScopes([], ["git push"]), true) + + expect(effective.allowed).toEqual(["git"]) + expect(effective.denied).toEqual([]) + expect(effective.hiddenWorkspaceDeny).toEqual(["git push"]) +}) + +it("resolveEffectiveCommandLists: lets a global deny entry veto a longer workspace allow entry", () => { + const effective = resolveEffectiveCommandLists(listScopes([], ["git push"]), listScopes(["git"], []), true) + + expect(effective.allowed).toEqual([]) + expect(effective.denied).toEqual(["git"]) + expect(effective.hiddenWorkspaceAllow).toEqual(["git push"]) +}) + +it("resolveEffectiveCommandLists: silences every workspace deny entry behind a global wildcard allow entry", () => { + const effective = resolveEffectiveCommandLists(listScopes(["*"], []), listScopes([], ["git push", "rm -rf"]), true) + + expect(effective.allowed).toEqual(["*"]) + expect(effective.denied).toEqual([]) + expect(effective.hiddenWorkspaceDeny).toEqual(["git push", "rm -rf"]) +}) + +it("resolveEffectiveCommandLists: shadows workspace entries case-insensitively, the way the matcher matches", () => { + const effective = resolveEffectiveCommandLists( + listScopes(["Git"], ["git status"]), + listScopes([], ["DOCKER"]), + true, + ) + + expect(effective.allowed).toEqual(["Git"]) + expect(effective.denied).toEqual(["DOCKER"]) + expect(effective.hiddenWorkspaceAllow).toEqual(["git status"]) + expect(effective.hiddenWorkspaceDeny).toEqual([]) +}) diff --git a/src/core/auto-approval/__tests__/commands.spec.ts b/src/core/auto-approval/__tests__/commands.spec.ts index ade53c2b20..87f6f44f5b 100644 --- a/src/core/auto-approval/__tests__/commands.spec.ts +++ b/src/core/auto-approval/__tests__/commands.spec.ts @@ -1,4 +1,10 @@ -import { containsDangerousSubstitution, getCommandDecision, getCommandDecisionDetailed } from "../commands" +import { resolveEffectiveCommandLists } from "../commandScope" +import { + containsDangerousSubstitution, + getCommandDecision, + getCommandDecisionDetailed, + isShadowedByAnyEntry, +} from "../commands" describe("containsDangerousSubstitution", () => { describe("zsh array assignments (should NOT be flagged)", () => { @@ -215,3 +221,82 @@ describe("getCommandDecisionDetailed", () => { } }) }) + +it("isShadowedByAnyEntry: shadows an entry whose every match a prefixing entry also matches", () => { + expect(isShadowedByAnyEntry("git push", ["git"])).toBe(true) + expect(isShadowedByAnyEntry("git push", ["git push"])).toBe(true) +}) + +it("isShadowedByAnyEntry: keeps an entry that decides matches no shadowing entry covers", () => { + expect(isShadowedByAnyEntry("git", ["git push"])).toBe(false) +}) + +it("isShadowedByAnyEntry: shadows every entry behind the universal wildcard", () => { + expect(isShadowedByAnyEntry("git push", ["*"])).toBe(true) + expect(isShadowedByAnyEntry("anything at all", ["*"])).toBe(true) +}) + +it("isShadowedByAnyEntry: compares case-insensitively, the way the matcher matches", () => { + expect(isShadowedByAnyEntry("Git Push", ["git"])).toBe(true) + expect(isShadowedByAnyEntry("git push", ["GIT"])).toBe(true) +}) + +it("isShadowedByAnyEntry: compares entries as stored, the way the matcher compares them", () => { + // The matcher trims commands, never entries, and shadowing mirrors that: + // both sides compare as stored. A trimmed comparison would let an inert + // padded entry erase a decidable one (and the reverse), deciding with a + // rule enforcement never applies. + expect(isShadowedByAnyEntry(" git push ", ["git"])).toBe(false) + expect(isShadowedByAnyEntry("git push", [" git "])).toBe(false) +}) + +it("layered decisions: a padded global deny entry cannot erase a workspace deny entry", () => { + // The padded global entry is inert at enforcement, so it must not shadow + // the workspace entry the matcher does honor: the deny survives. + const scopes = (globalList: string[], workspaceList: string[]) => ({ + global: globalList, + workspace: workspaceList, + merged: [...new Set([...globalList, ...workspaceList])], + }) + const effective = resolveEffectiveCommandLists(scopes([" git"], []), scopes([], ["git push"]), true) + + expect(getCommandDecision("git push origin", effective.allowed, effective.denied)).toBe("auto_deny") +}) + +it("layered decisions: a padded global allow entry cannot erase a workspace allow entry", () => { + // Mirror of the deny side: the inert padded entry leaves the workspace + // allow entry effective, so the command is approved rather than downgraded + // to a prompt for a match no entry can produce. + const scopes = (globalList: string[], workspaceList: string[]) => ({ + global: globalList, + workspace: workspaceList, + merged: [...new Set([...globalList, ...workspaceList])], + }) + const effective = resolveEffectiveCommandLists(scopes([" git"], ["git push"]), scopes([], []), true) + + expect(getCommandDecision("git push origin", effective.allowed, effective.denied)).toBe("auto_approve") +}) + +it("isShadowedByAnyEntry: never treats an empty entry as covered", () => { + // Compared naively, an empty entry prefix-matches every command, so + // against an empty shadow entry it would report itself covered. The + // empty-entry guard keeps such an entry visible instead. Command-list + // layers sanitize blanks before shadowing, so this pins the function's + // contract directly. + expect(isShadowedByAnyEntry("", [""])).toBe(false) + expect(isShadowedByAnyEntry("", ["git"])).toBe(false) +}) + +it("layered decisions: reproduces the global-decides precedence through the scope-blind matcher", () => { + // Global deny "git" vs workspace allow "git push": the composed lists + // drop the shadowed allow entry, so the scope-blind matcher — never told + // about layers — returns the global layer's deny. + const scopes = (globalList: string[], workspaceList: string[]) => ({ + global: globalList, + workspace: workspaceList, + merged: [...new Set([...globalList, ...workspaceList])], + }) + const effective = resolveEffectiveCommandLists(scopes([], ["git push"]), scopes(["git"], []), true) + + expect(getCommandDecision("git push origin", effective.allowed, effective.denied)).toBe("auto_deny") +}) diff --git a/src/core/auto-approval/commandScope.ts b/src/core/auto-approval/commandScope.ts index d57e77bf7d..03d464559f 100644 --- a/src/core/auto-approval/commandScope.ts +++ b/src/core/auto-approval/commandScope.ts @@ -1,6 +1,8 @@ +import { DEFAULT_WORKSPACE_COMMAND_LISTS_ENABLED } from "@roo-code/types" import * as vscode from "vscode" import { Package } from "../../shared/package" +import { isShadowedByAnyEntry } from "./commands" export type CommandListConfigKey = "allowedCommands" | "deniedCommands" @@ -182,6 +184,79 @@ export function resolveWorkspaceWriteTarget( return undefined } +/** + * Reads the `zoo-code.workspaceCommandListsEnabled` gate that decides whether + * the workspace layers of the command-list keys take part in enforcement and + * display at all. The configuration declares the key with `scope: + * "application"`, so only user settings can flip it. The lookup is guarded + * leniently like the command-list reads: a configuration failure keeps the + * workspace layers inert, which cannot widen what auto-approval allows. + */ +export function workspaceCommandListsEnabled(): boolean { + try { + return commandListConfiguration().get("workspaceCommandListsEnabled") === true + } catch { + return DEFAULT_WORKSPACE_COMMAND_LISTS_ENABLED + } +} + +export interface EffectiveCommandLists { + allowed: string[] + denied: string[] + /** + * Workspace allow entries absent from the effective allowlist — hidden by + * the gate or shadowed by a global entry — which a dialog save must not be + * able to delete by omission. + */ + hiddenWorkspaceAllow: string[] + /** Deny-side counterpart of {@link hiddenWorkspaceAllow}. */ + hiddenWorkspaceDeny: string[] +} + +/** + * Composes the raw per-key layers into the pair of lists to enforce and + * display, keeping display byte-equal to enforcement. + * + * Gate off: the workspace layers contribute nothing and the global layers are + * effective verbatim. Gate on: a workspace entry survives only when no entry + * of either global list shadows it, so a global entry decides every command it + * matches while workspace entries apply where the global layer is silent. + * Shadowing is cross-directional because a global allow entry silences a + * workspace deny entry (and vice versa) just as much as a same-list shadow. + * + * Composing here — not inside {@link readCommandListScopes} — keeps the raw + * layers intact for the save path, which derives the write split from them and + * must never see entries filtered out of the workspace layer. + * + * @param allowScopes - Raw `allowedCommands` layers from readCommandListScopes. + * @param denyScopes - Raw `deniedCommands` layers from readCommandListScopes. + * @param enabled - The value of the `workspaceCommandListsEnabled` gate. + * @returns The effective allow/deny lists and the workspace entries hidden + * from each. + */ +export function resolveEffectiveCommandLists( + allowScopes: CommandListScopes, + denyScopes: CommandListScopes, + enabled: boolean, +): EffectiveCommandLists { + const globalEntries = [...allowScopes.global, ...denyScopes.global] + + const keptAllow = enabled + ? allowScopes.workspace.filter((entry) => !isShadowedByAnyEntry(entry, globalEntries)) + : [] + const keptDeny = enabled ? denyScopes.workspace.filter((entry) => !isShadowedByAnyEntry(entry, globalEntries)) : [] + + const allowed = [...new Set([...allowScopes.global, ...keptAllow])] + const denied = [...new Set([...denyScopes.global, ...keptDeny])] + + return { + allowed, + denied, + hiddenWorkspaceAllow: allowScopes.workspace.filter((entry) => !allowed.includes(entry)), + hiddenWorkspaceDeny: denyScopes.workspace.filter((entry) => !denied.includes(entry)), + } +} + function sanitizeCommandList(list: unknown): string[] { if (!Array.isArray(list)) { return [] diff --git a/src/core/auto-approval/commands.ts b/src/core/auto-approval/commands.ts index 9d7289c265..8f3020a5f0 100644 --- a/src/core/auto-approval/commands.ts +++ b/src/core/auto-approval/commands.ts @@ -113,6 +113,34 @@ export function findLongestPrefixMatch(command: string, prefixes: string[]): str return longestMatch } +/** + * Whether one command-list entry is covered by any entry of `shadowing`. + * + * "Covered" applies the matcher's comparison — case-insensitive `startsWith` + * with `"*"` as universal prefix — directly to the two stored entries: only + * commands reach the matcher trimmed, entries never are, so an entry whose + * stored form carries leading whitespace matches no command and can shadow + * only other entries that match nothing either. An entry whose every possible + * match is also matched by some `shadowing` entry cannot decide anything on + * its own: that layer's decision stands. Dropping covered entries when + * composing layers reproduces a global-decides precedence without the matcher + * knowing about layers. + * + * @param entry - The entry that may be covered. + * @param shadowing - The entries whose match coverage decides. + * @returns true if some `shadowing` entry covers `entry`. An empty `entry` + * counts as covered only by `"*"`: the guard that stops it prefix-matching + * every command keeps a blank entry visible, as it cannot be enforced. + */ +export function isShadowedByAnyEntry(entry: string, shadowing: string[]): boolean { + const lowerEntry = entry.toLowerCase() + + return shadowing.some((shadow) => { + const lowerShadow = shadow.toLowerCase() + return lowerShadow === "*" || (lowerEntry !== "" && lowerEntry.startsWith(lowerShadow)) + }) +} + /** * Check if a single command should be auto-approved. * Returns true only for commands that explicitly match the allowlist diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 78ac7c4870..ef75a01cc9 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -54,6 +54,7 @@ import { ORGANIZATION_ALLOW_ALL, DEFAULT_MODES, DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, + DEFAULT_WORKSPACE_COMMAND_LISTS_ENABLED, getModelId, isRetiredProvider, providerIdentifiers, @@ -112,7 +113,11 @@ import { forceFullModelDetailsLoad, hasLoadedFullDetails } from "../../api/provi import { ContextProxy } from "../config/ContextProxy" import { ProviderSettingsManager } from "../config/ProviderSettingsManager" import { CustomModesManager } from "../config/CustomModesManager" -import { readCommandListScopes } from "../auto-approval/commandScope" +import { + readCommandListScopes, + resolveEffectiveCommandLists, + workspaceCommandListsEnabled, +} from "../auto-approval/commandScope" import { PendingActionSettlementError, Task } from "../task/Task" import { webviewMessageHandler } from "./webviewMessageHandler" @@ -2549,6 +2554,7 @@ export class ClineProvider alwaysDenyUnapprovedCommands, allowedCommands, deniedCommands, + workspaceCommandListsEnabled: gateEnabled, alwaysAllowMcp, alwaysAllowModeSwitch, alwaysAllowSubtasks, @@ -2729,8 +2735,11 @@ export class ClineProvider checkpointTimeout: checkpointTimeout ?? DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, shouldShowAnnouncement: telemetrySetting !== "unset" && lastShownAnnouncementId !== this.latestAnnouncementId, - allowedCommands: allowedCommands ?? [], - deniedCommands: deniedCommands ?? [], + // getState() always resolves these to the effective command lists + // (never undefined), so no fallback is needed here. + allowedCommands, + deniedCommands, + workspaceCommandListsEnabled: gateEnabled ?? DEFAULT_WORKSPACE_COMMAND_LISTS_ENABLED, soundVolume: soundVolume ?? 0.5, writeDelayMs: writeDelayMs ?? DEFAULT_WRITE_DELAY_MS, diffFuzzyThreshold: diffFuzzyThreshold ?? DEFAULT_DIFF_FUZZY_THRESHOLD, @@ -2930,6 +2939,14 @@ export class ClineProvider const taskSyncEnabled: boolean = false + // One gate read and one resolver pass per getState() call, so the + // effective lists and the gate value posted together describe a single + // configuration state. + const gateEnabled = workspaceCommandListsEnabled() + const allowScopes = readCommandListScopes("allowedCommands", stateValues.allowedCommands) + const denyScopes = readCommandListScopes("deniedCommands", stateValues.deniedCommands) + const effectiveCommandLists = resolveEffectiveCommandLists(allowScopes, denyScopes, gateEnabled) + // Return the same structure as before. return { apiConfiguration: providerSettings, @@ -2959,11 +2976,13 @@ export class ClineProvider autoCondenseContext: stateValues.autoCondenseContext ?? true, autoCondenseContextPercent: stateValues.autoCondenseContextPercent ?? 100, taskHistory: includeTaskHistory ? this.taskHistoryStore.getAll() : [], - // Enforce and display the set union of the global-state layer and the - // workspace-scope configuration layer; getStateToPostToWebview shows + // Enforce and display the effective lists that + // resolveEffectiveCommandLists composes from the global-state and + // workspace-scope configuration layers; getStateToPostToWebview shows // exactly these values, so display cannot drift from enforcement. - allowedCommands: readCommandListScopes("allowedCommands", stateValues.allowedCommands).merged, - deniedCommands: readCommandListScopes("deniedCommands", stateValues.deniedCommands).merged, + allowedCommands: effectiveCommandLists.allowed, + deniedCommands: effectiveCommandLists.denied, + workspaceCommandListsEnabled: gateEnabled, soundEnabled: stateValues.soundEnabled ?? false, ttsEnabled: stateValues.ttsEnabled ?? false, ttsSpeed: stateValues.ttsSpeed ?? 1.0, diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 4a96fadde0..abae55de55 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -18,6 +18,7 @@ import { ORGANIZATION_ALLOW_ALL, DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, DEFAULT_DIFF_FUZZY_THRESHOLD, + DEFAULT_WORKSPACE_COMMAND_LISTS_ENABLED, DEFAULT_WRITE_DELAY_MS, providerIdentifiers, openAiModelInfoSaneDefaults, @@ -27,6 +28,7 @@ import { TelemetryService } from "@roo-code/telemetry" import { defaultModeSlug } from "../../../shared/modes" import { experimentDefault } from "../../../shared/experiments" import { setTtsEnabled } from "../../../utils/tts" +import { makeWorkspaceConfiguration } from "../../../test-utils/vscode" import { ContextProxy } from "../../config/ContextProxy" import { WorkspaceIndexingEnablementManager } from "../../../services/code-index/workspace-indexing-enablement-manager" import { Task, TaskOptions } from "../../task/Task" @@ -1625,9 +1627,11 @@ describe("ClineProvider", () => { } as unknown as vscode.WorkspaceConfiguration) }) + // The gate is a synchronous configuration read, so the doubles make the + // `get` lookup key-aware: only the gate key reports true here. test("getState enforces workspace-scope denied commands even when global state is empty", async () => { vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ - get: vi.fn().mockReturnValue([]), + get: vi.fn().mockImplementation((key: string) => key === "workspaceCommandListsEnabled"), inspect: vi .fn() .mockImplementation((key: string) => @@ -1641,11 +1645,11 @@ describe("ClineProvider", () => { expect(state.deniedCommands).toEqual(["docker"]) }) - test("getStateToPostToWebview shows the same merged command lists as getState", async () => { + test("getStateToPostToWebview shows the same effective command lists as getState", async () => { await provider.resolveWebviewView(mockWebviewView) await provider.contextProxy.setValue("deniedCommands", ["git"]) vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ - get: vi.fn().mockReturnValue(["git", "docker"]), + get: vi.fn().mockImplementation((key: string) => key === "workspaceCommandListsEnabled"), inspect: vi .fn() .mockImplementation((key: string) => @@ -1656,15 +1660,16 @@ describe("ClineProvider", () => { const state = await provider.getStateToPostToWebview() - // Display is the set union of global state and workspace scope, matching - // what getState() enforces. + // The workspace entry survives because no global entry shadows it, so + // display is the global layer plus that entry — exactly what + // getState() enforces. expect(state.deniedCommands).toEqual(["git", "docker"]) expect(state.allowedCommands).toEqual([]) }) test("getState prefers the workspace-folder value for multi-root workspaces", async () => { vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ - get: vi.fn().mockReturnValue(["docker", "pnpm docker"]), + get: vi.fn().mockImplementation((key: string) => key === "workspaceCommandListsEnabled"), inspect: vi .fn() .mockImplementation((key: string) => @@ -1682,6 +1687,77 @@ describe("ClineProvider", () => { // leak into the workspace layer. expect(state.deniedCommands).toEqual(["pnpm docker"]) }) + + // A workspace-only allow entry is exactly what the gate decides: enabled + // it joins the effective lists enforcement uses; disabled enforcement ignores it. + function mockGateConfiguration(enabled: boolean) { + // The shared double reports scripted values through `get`, which + // is how the gate reader sees its value; the per-key `inspect` + // override supplies the allow-side workspace layer. + const config = makeWorkspaceConfiguration({ workspaceCommandListsEnabled: enabled }) + const inspect = vi.fn() + inspect.mockImplementation((key: string) => + key === "allowedCommands" ? { key, workspaceValue: ["docker"] } : undefined, + ) + config.inspect = inspect + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(config) + } + + test("a workspace-only allow entry surfaces through getState and getStateToPostToWebview when the gate is on", async () => { + await provider.resolveWebviewView(mockWebviewView) + mockGateConfiguration(true) + + const state = await provider.getState() + const postedState = await provider.getStateToPostToWebview() + + expect(state.allowedCommands).toEqual(["docker"]) + expect(postedState.allowedCommands).toEqual(["docker"]) + }) + + test("a workspace-only allow entry is absent from getState and getStateToPostToWebview when the gate is off", async () => { + await provider.resolveWebviewView(mockWebviewView) + mockGateConfiguration(false) + + const state = await provider.getState() + const postedState = await provider.getStateToPostToWebview() + + expect(state.allowedCommands).toEqual([]) + expect(postedState.allowedCommands).toEqual([]) + }) + + test("getState returns the saved workspace command-list gate value", async () => { + mockGateConfiguration(true) + + const state = await provider.getState() + + expect(state.workspaceCommandListsEnabled).toBe(true) + }) + + test("getState defaults the workspace command-list gate to the shared constant", async () => { + mockGateConfiguration(false) + + const state = await provider.getState() + + expect(state.workspaceCommandListsEnabled).toBe(DEFAULT_WORKSPACE_COMMAND_LISTS_ENABLED) + }) + + test("getStateToPostToWebview returns the saved workspace command-list gate value", async () => { + await provider.resolveWebviewView(mockWebviewView) + mockGateConfiguration(true) + + const state = await provider.getStateToPostToWebview() + + expect(state.workspaceCommandListsEnabled).toBe(true) + }) + + test("getStateToPostToWebview defaults the workspace command-list gate to the shared constant", async () => { + await provider.resolveWebviewView(mockWebviewView) + mockGateConfiguration(false) + + const state = await provider.getStateToPostToWebview() + + expect(state.workspaceCommandListsEnabled).toBe(DEFAULT_WORKSPACE_COMMAND_LISTS_ENABLED) + }) }) test("getState returns the saved allowed read files", async () => { diff --git a/src/core/webview/__tests__/webviewMessageHandler.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index 1a3ffaa3ae..a4096753c1 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -1455,14 +1455,21 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { // is awaited by the handler, so a rejected promise fails that write), in // call order; a plain value resolves like the default mock, and an Error // element makes that call throw. + type InspectedCommandList = { + globalValue?: string[] + workspaceValue?: string[] + workspaceFolderValue?: string[] + } + + // `inspected` may be a function of the requested key so a two-key payload + // test can give each list its own layers. `gateEnabled` scripts the + // `workspaceCommandListsEnabled` configuration read; leaving it unset + // keeps the gate off, matching the extension default. function mockCommandConfiguration( - inspected: { - globalValue?: string[] - workspaceValue?: string[] - workspaceFolderValue?: string[] - }, + inspected: InspectedCommandList | ((key: string) => InspectedCommandList), updateSequence?: unknown[], inspectSequence?: unknown[], + gateEnabled?: boolean, ) { const update = vi.fn() if (updateSequence) { @@ -1478,11 +1485,13 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { return next }) } else { - inspect.mockReturnValue(inspected) + inspect.mockImplementation((key: string) => (typeof inspected === "function" ? inspected(key) : inspected)) } // The shared structurally complete double keeps the configuration type // correct; the scripted members are installed per test. - const config = makeWorkspaceConfiguration() + const config = makeWorkspaceConfiguration( + gateEnabled === undefined ? {} : { workspaceCommandListsEnabled: gateEnabled }, + ) config.update = update config.inspect = inspect vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(config) @@ -1504,10 +1513,11 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { // Extension global state holds only the global layer, so the next save // cannot double-home the workspace entry. expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("deniedCommands", ["aws"]) - // Exactly one snapshot per key: the scope read, write target, and - // rollback all derive from the same inspect(), so a helper re-reading - // the configuration would raise this count and could diverge. - expect(inspect).toHaveBeenCalledTimes(1) + // One snapshot per command-list key: this key's scope read, write + // target, and rollback all derive from the same inspect(), and the + // shadowing check reads the opposite key through its own guarded + // snapshot, so the count is two, never more. + expect(inspect).toHaveBeenCalledTimes(2) }) it("writes the workspace-folder value at the folder target in multi-root workspaces", async () => { @@ -1528,7 +1538,10 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { }) it("clears a deleted workspace entry at the workspace scope without adding it to global", async () => { - const { update } = mockCommandConfiguration({ workspaceValue: ["docker"] }) + // Gate on: the entry is visible in the dialog, and only visible entries + // are deletable there (a hidden entry is re-added by the preservation + // rule instead). + const { update } = mockCommandConfiguration({ workspaceValue: ["docker"] }, undefined, undefined, true) await webviewMessageHandler(mockClineProvider, { type: "updateSettings", @@ -1558,6 +1571,30 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("allowedCommands", ["git"]) }) + it("restores the raw workspace value when a workspace-target global write fails", async () => { + // With no folder layer the rewind must read the workspace value, not + // the absent folder value. The raw value holds the entry this save + // dropped, so the restore call is distinguishable from the workspace + // write itself and provably uses the raw inspected value. + const { update } = mockCommandConfiguration({ workspaceValue: ["npm docker", "docker"] }, [ + undefined, + Promise.reject(new Error("disk on fire")), + ]) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: ["docker", "aws"] }, + }) + + expect(update).toHaveBeenCalledWith( + "deniedCommands", + ["npm docker", "docker"], + vscode.ConfigurationTarget.Workspace, + ) + expect(update).toHaveBeenCalledTimes(3) + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("Failed to save deniedCommands: disk on fire") + }) + it("restores the previous workspace value when the global write fails", async () => { // update() call order: workspace write succeeds, global write rejects. // The raw folder-layer value differs from the sanitized post-split layer @@ -1710,6 +1747,115 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { // No exception escaped the handler: the state post still ran. expect(mockClineProvider.postStateToWebview).toHaveBeenCalled() }) + + // Entries the dialog never displayed — hidden while the gate is off, or + // shadowed by a global entry with it on — cannot be deleted by omission + // from a list the user could not see, so a save re-adds them to the + // workspace layer it rewrites. + it("re-adds gate-hidden workspace entries to the workspace write instead of deleting them", async () => { + const { update } = mockCommandConfiguration({ workspaceValue: ["docker", "npm"] }, undefined, undefined, false) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + // The gate is off, so the dialog displayed nothing for this key. + updatedSettings: { deniedCommands: [] }, + }) + + expect(update).toHaveBeenCalledWith("deniedCommands", ["docker", "npm"], vscode.ConfigurationTarget.Workspace) + expect(update).not.toHaveBeenCalledWith("deniedCommands", [], vscode.ConfigurationTarget.Workspace) + }) + + it("keeps hidden workspace entries out of the global write", async () => { + const { update } = mockCommandConfiguration({ workspaceValue: ["docker", "npm"] }, undefined, undefined, false) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: [] }, + }) + + // Preservation is workspace-side only: re-adding hidden entries must + // not leak them into the user's global settings. + expect(update).not.toHaveBeenCalledWith("deniedCommands", ["docker", "npm"], vscode.ConfigurationTarget.Global) + expect(update).toHaveBeenCalledWith("deniedCommands", [], vscode.ConfigurationTarget.Global) + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("deniedCommands", []) + }) + + it("re-adds a workspace deny entry shadowed by a global allow entry when the gate is on", async () => { + vi.mocked(mockClineProvider.contextProxy.getValue).mockImplementation((key: string) => + key === "allowedCommands" ? ["git"] : undefined, + ) + const { update } = mockCommandConfiguration( + (key) => (key === "deniedCommands" ? { workspaceValue: ["git push"] } : {}), + undefined, + undefined, + true, + ) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + // "git push" is shadowed by the global allow "git", so the dialog + // displayed nothing deletable. + updatedSettings: { deniedCommands: [] }, + }) + + expect(update).toHaveBeenCalledWith("deniedCommands", ["git push"], vscode.ConfigurationTarget.Workspace) + expect(update).toHaveBeenCalledWith("deniedCommands", [], vscode.ConfigurationTarget.Global) + }) + + it("re-adds a shadowed workspace allow entry alongside the user's visible deletions", async () => { + vi.mocked(mockClineProvider.contextProxy.getValue).mockImplementation((key: string) => + key === "deniedCommands" ? ["git"] : undefined, + ) + const { update } = mockCommandConfiguration( + (key) => (key === "allowedCommands" ? { workspaceValue: ["git", "docker"] } : {}), + undefined, + undefined, + true, + ) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + // "docker" stayed visible and kept; "git" was hidden by the global + // deny, and a workspace entry the user never saw is not deletable. + updatedSettings: { allowedCommands: ["docker"] }, + }) + + expect(update).toHaveBeenCalledWith("allowedCommands", ["docker", "git"], vscode.ConfigurationTarget.Workspace) + expect(update).toHaveBeenCalledWith("allowedCommands", [], vscode.ConfigurationTarget.Global) + }) + + it("does not persist a webview-supplied workspaceCommandListsEnabled value", async () => { + mockCommandConfiguration({}) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + // A settings payload is not a second source for the gate: the + // synchronous user-scope configuration read is. + updatedSettings: { workspaceCommandListsEnabled: true, alwaysDenyUnapprovedCommands: true }, + }) + + expect(mockClineProvider.contextProxy.setValue).not.toHaveBeenCalledWith( + "workspaceCommandListsEnabled", + expect.anything(), + ) + // Sibling keys of the same payload are unaffected by the refusal. + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("alwaysDenyUnapprovedCommands", true) + }) + + it("abandons a payload carrying only the gate key without writing configuration", async () => { + const { update } = mockCommandConfiguration({}) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { workspaceCommandListsEnabled: true }, + }) + + expect(update).not.toHaveBeenCalled() + expect(mockClineProvider.contextProxy.setValue).not.toHaveBeenCalled() + // The state post still runs, so the dialog re-reads the effective value + // instead of keeping the refused one. + expect(mockClineProvider.postStateToWebview).toHaveBeenCalled() + }) }) describe("webviewMessageHandler - terminalProfile", () => { diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 6d198f57fe..9e9e0ef6ea 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -63,8 +63,10 @@ import { type CommandListInspection, commandListConfiguration, readCommandListScopes, + resolveEffectiveCommandLists, resolveWorkspaceWriteTarget, splitCommandListScopes, + workspaceCommandListsEnabled, } from "../auto-approval/commandScope" import { type RouterName, toRouterName } from "../../shared/api" import { MessageEnhancer } from "./messageEnhancer" @@ -758,6 +760,13 @@ export const webviewMessageHandler = async ( if (key === "language") { newValue = value ?? "en" changeLanguage(newValue as Language) + } else if (key === "workspaceCommandListsEnabled") { + // The gate's effective value is the synchronous + // user-scope configuration read that enforcement and + // display already perform; a value arriving in a + // settings payload is not a second source that can flip + // it, so it is never persisted here. + continue } else if (key === "allowedCommands" || key === "deniedCommands") { const commands = value ?? [] @@ -798,11 +807,34 @@ export const webviewMessageHandler = async ( } const currentScopes = readCommandListScopes(key, getGlobalState(key), inspection) - const { globalWrite, workspaceWrite } = splitCommandListScopes( + const { globalWrite, workspaceWrite: keptWorkspaceWrite } = splitCommandListScopes( incoming, currentScopes.workspace, currentScopes.global, ) + + // Shadowing is cross-directional, so the hidden-entry + // set needs the opposite key's global layer as input. + // That opposite-key read is a fresh guarded read: it + // only feeds the additive preservation below and never + // this key's split, target, or rollback, which stay on + // the single snapshot. + const otherKey = key === "allowedCommands" ? "deniedCommands" : "allowedCommands" + const otherScopes = readCommandListScopes(otherKey, getGlobalState(otherKey)) + const { hiddenWorkspaceAllow, hiddenWorkspaceDeny } = resolveEffectiveCommandLists( + key === "allowedCommands" ? currentScopes : otherScopes, + key === "deniedCommands" ? currentScopes : otherScopes, + workspaceCommandListsEnabled(), + ) + // Entries hidden from the dialog — while the gate is off, + // or shadowed by a global entry with it on — cannot be + // deleted through a list that never displayed them, so a + // save re-adds them to the workspace layer it rewrites. + // Users remove those directly in the workspace settings + // file. + const hiddenWorkspaceEntries = + key === "allowedCommands" ? hiddenWorkspaceAllow : hiddenWorkspaceDeny + const workspaceWrite = [...new Set([...keptWorkspaceWrite, ...hiddenWorkspaceEntries])] const workspaceTarget = resolveWorkspaceWriteTarget(key, inspection) // The two configuration writes below are not atomic, so the raw diff --git a/src/package.json b/src/package.json index 0931ca779d..c3993fd984 100644 --- a/src/package.json +++ b/src/package.json @@ -317,6 +317,13 @@ "default": [], "description": "%commands.deniedCommands.description%" }, + "zoo-code.workspaceCommandListsEnabled": { + "type": "boolean", + "default": false, + "scope": "application", + "description": "%settings.workspaceCommandListsEnabled.description%", + "markdownDescription": "%settings.workspaceCommandListsEnabled.description%" + }, "zoo-code.commandExecutionTimeout": { "type": "number", "default": 0, diff --git a/src/package.nls.ca.json b/src/package.nls.ca.json index b1397adf71..69da12747e 100644 --- a/src/package.nls.ca.json +++ b/src/package.nls.ca.json @@ -48,5 +48,6 @@ "settings.debugProxy.tlsInsecure.description": "Accepta certificats auto-signats del proxy. **Requerit per a la inspecció MITM.** ⚠️ Insegur — utilitza-ho només per a debugging local.", "settings.workspace.rootResolution.description": "Com resol Zoo l'arrel de l'espai de treball en un espai de treball multi-arrel. L'arrel s'utilitza per localitzar `.roomodes`, `.roo/mcp.json`, `.roo/rules/` i altra configuració d'àmbit de projecte. Canviar aquest paràmetre només afecta les cerques futures; les tasques en execució conserven la seva arrel original.", "settings.workspace.rootResolution.activeEditor.description": "Utilitza la carpeta de l'espai de treball que conté l'editor actiu; recorre a la primera carpeta de l'espai de treball. (Per defecte — conserva el comportament heretat.)", - "settings.workspace.rootResolution.firstFolder.description": "Utilitza sempre la primera carpeta de l'espai de treball (workspaceFolders[0]). Determinista — independent del fitxer que tingui el focus en aquest moment." + "settings.workspace.rootResolution.firstFolder.description": "Utilitza sempre la primera carpeta de l'espai de treball (workspaceFolders[0]). Determinista — independent del fitxer que tingui el focus en aquest moment.", + "settings.workspaceCommandListsEnabled.description": "Activa les llistes de comandes aportades pels ajustos de l'espai de treball (els valors de `zoo-code.allowedCommands` i `zoo-code.deniedCommands` als fitxers d'ajustos de l'espai de treball). Mentre estigui desactivada, les llistes de comandes de l'espai de treball no tenen cap efecte. Quan estigui activada, les entrades de nivell d'usuari tenen preferència: una entrada de nivell d'usuari decideix qualsevol comanda que coincideixi, i les entrades de l'espai de treball només s'apliquen a les comandes amb què cap entrada de nivell d'usuari no coincideix. Aquest paràmetre només es pot canviar als [paràmetres d'usuari](https://code.visualstudio.com/docs/getstarted/settings)." } diff --git a/src/package.nls.de.json b/src/package.nls.de.json index b4cd8440ab..59a4f82034 100644 --- a/src/package.nls.de.json +++ b/src/package.nls.de.json @@ -48,5 +48,6 @@ "settings.debugProxy.tlsInsecure.description": "Akzeptiere selbstsignierte Zertifikate vom Proxy. **Erforderlich für MITM-Inspektion.** ⚠️ Unsicher – verwende das nur für lokales Debugging.", "settings.workspace.rootResolution.description": "Wie Zoo das Workspace-Stammverzeichnis in einem Multi-Root-Workspace auflöst. Das Stammverzeichnis wird verwendet, um `.roomodes`, `.roo/mcp.json`, `.roo/rules/` und andere projektbezogene Konfiguration zu finden. Eine Änderung dieser Einstellung wirkt sich nur auf zukünftige Suchen aus; laufende Aufgaben behalten ihr ursprüngliches Stammverzeichnis.", "settings.workspace.rootResolution.activeEditor.description": "Verwende den Workspace-Ordner, der den aktiven Editor enthält; greife auf den ersten Workspace-Ordner zurück. (Standard – behält das bisherige Verhalten bei.)", - "settings.workspace.rootResolution.firstFolder.description": "Verwende immer den ersten Workspace-Ordner (workspaceFolders[0]). Deterministisch – unabhängig davon, welche Datei gerade fokussiert ist." + "settings.workspace.rootResolution.firstFolder.description": "Verwende immer den ersten Workspace-Ordner (workspaceFolders[0]). Deterministisch – unabhängig davon, welche Datei gerade fokussiert ist.", + "settings.workspaceCommandListsEnabled.description": "Aktiviert die von Workspace-Einstellungen beigesteuerten Befehlslisten (die Werte von `zoo-code.allowedCommands` und `zoo-code.deniedCommands` in Workspace-Einstellungsdateien). Solange deaktiviert, haben Workspace-Befehlslisten keine Auswirkung. Wenn aktiviert, haben Einträge auf Benutzerebene Vorrang: Ein Eintrag auf Benutzerebene entscheidet über jeden Befehl, mit dem er übereinstimmt, und Workspace-Einträge gelten nur für Befehle, mit denen kein Eintrag auf Benutzerebene übereinstimmt. Diese Einstellung kann nur in den [Benutzereinstellungen](https://code.visualstudio.com/docs/getstarted/settings) geändert werden." } diff --git a/src/package.nls.es.json b/src/package.nls.es.json index 1d86a70980..86ca2c380a 100644 --- a/src/package.nls.es.json +++ b/src/package.nls.es.json @@ -48,5 +48,6 @@ "settings.debugProxy.tlsInsecure.description": "Aceptar certificados autofirmados del proxy. **Necesario para la inspección MITM.** ⚠️ Inseguro: úsalo solo para depuración local.", "settings.workspace.rootResolution.description": "Cómo resuelve Zoo la raíz del espacio de trabajo en un espacio de trabajo multi-raíz. La raíz se usa para localizar `.roomodes`, `.roo/mcp.json`, `.roo/rules/` y otra configuración del ámbito del proyecto. Cambiar este ajuste solo afecta a las búsquedas futuras; las tareas en ejecución conservan su raíz original.", "settings.workspace.rootResolution.activeEditor.description": "Usa la carpeta del espacio de trabajo que contiene el editor activo; recurre a la primera carpeta del espacio de trabajo. (Predeterminado: conserva el comportamiento heredado.)", - "settings.workspace.rootResolution.firstFolder.description": "Usa siempre la primera carpeta del espacio de trabajo (workspaceFolders[0]). Determinista: independiente del archivo que tenga el foco en ese momento." + "settings.workspace.rootResolution.firstFolder.description": "Usa siempre la primera carpeta del espacio de trabajo (workspaceFolders[0]). Determinista: independiente del archivo que tenga el foco en ese momento.", + "settings.workspaceCommandListsEnabled.description": "Habilita las listas de comandos aportadas por la configuración del espacio de trabajo (los valores de `zoo-code.allowedCommands` y `zoo-code.deniedCommands` en los archivos de configuración del espacio de trabajo). Mientras esté deshabilitada, las listas de comandos del espacio de trabajo no tienen ningún efecto. Cuando esté habilitada, las entradas de nivel de usuario tienen prioridad: una entrada de nivel de usuario decide cualquier comando que coincida, y las entradas del espacio de trabajo solo se aplican a los comandos con los que ninguna entrada de nivel de usuario coincide. Este ajuste solo se puede cambiar en la [configuración de usuario](https://code.visualstudio.com/docs/getstarted/settings)." } diff --git a/src/package.nls.fr.json b/src/package.nls.fr.json index 85207df975..40b15fd067 100644 --- a/src/package.nls.fr.json +++ b/src/package.nls.fr.json @@ -48,5 +48,6 @@ "settings.debugProxy.tlsInsecure.description": "Accepter les certificats auto-signés du proxy. **Requis pour l'inspection MITM.** ⚠️ Non sécurisé — à utiliser uniquement pour le debug local.", "settings.workspace.rootResolution.description": "Comment Zoo résout la racine de l'espace de travail dans un espace de travail multi-racine. La racine sert à localiser `.roomodes`, `.roo/mcp.json`, `.roo/rules/` et d'autres configurations propres au projet. Modifier ce paramètre n'affecte que les recherches futures ; les tâches en cours conservent leur racine d'origine.", "settings.workspace.rootResolution.activeEditor.description": "Utilise le dossier de l'espace de travail contenant l'éditeur actif ; revient au premier dossier de l'espace de travail. (Par défaut — conserve le comportement hérité.)", - "settings.workspace.rootResolution.firstFolder.description": "Utilise toujours le premier dossier de l'espace de travail (workspaceFolders[0]). Déterministe — indépendant du fichier actuellement ciblé." + "settings.workspace.rootResolution.firstFolder.description": "Utilise toujours le premier dossier de l'espace de travail (workspaceFolders[0]). Déterministe — indépendant du fichier actuellement ciblé.", + "settings.workspaceCommandListsEnabled.description": "Active les listes de commandes issues des paramètres de l'espace de travail (les valeurs de `zoo-code.allowedCommands` et `zoo-code.deniedCommands` dans les fichiers de paramètres de l'espace de travail). Lorsqu'elle est désactivée, les listes de commandes de l'espace de travail n'ont aucun effet. Lorsqu'elle est activée, les entrées de niveau utilisateur ont la priorité : une entrée de niveau utilisateur décide de toute commande à laquelle elle correspond, et les entrées de l'espace de travail ne s'appliquent qu'aux commandes pour lesquelles aucune entrée de niveau utilisateur ne correspond. Ce paramètre ne peut être modifié que dans les [paramètres utilisateur](https://code.visualstudio.com/docs/getstarted/settings)." } diff --git a/src/package.nls.hi.json b/src/package.nls.hi.json index f4f283bb65..92bd9f82fb 100644 --- a/src/package.nls.hi.json +++ b/src/package.nls.hi.json @@ -48,5 +48,6 @@ "settings.debugProxy.tlsInsecure.description": "Proxy से आने वाले self-signed certificates accept करो। **MITM inspection के लिए ज़रूरी।** ⚠️ Insecure — सिर्फ local debugging के लिए इस्तेमाल करो।", "settings.workspace.rootResolution.description": "Multi-root workspace में Zoo workspace root को कैसे resolve करता है। Root का इस्तेमाल `.roomodes`, `.roo/mcp.json`, `.roo/rules/` और दूसरी project-scoped configuration ढूंढने के लिए होता है। इस setting को बदलने का असर सिर्फ आगे की lookups पर पड़ता है; चल रहे tasks अपना original root बनाए रखते हैं।", "settings.workspace.rootResolution.activeEditor.description": "उस workspace folder का इस्तेमाल करो जिसमें active editor है; पहले workspace folder पर fall back करो। (Default — legacy behavior बनाए रखता है।)", - "settings.workspace.rootResolution.firstFolder.description": "हमेशा पहले workspace folder (workspaceFolders[0]) का इस्तेमाल करो। Deterministic — इस बात से स्वतंत्र कि अभी कौन सी file focused है।" + "settings.workspace.rootResolution.firstFolder.description": "हमेशा पहले workspace folder (workspaceFolders[0]) का इस्तेमाल करो। Deterministic — इस बात से स्वतंत्र कि अभी कौन सी file focused है।", + "settings.workspaceCommandListsEnabled.description": "Workspace settings द्वारा दी गई command lists को सक्रिय करো (workspace settings files में `zoo-code.allowedCommands` और `zoo-code.deniedCommands` के मान)। जब तक इह निष्क्रिय है, workspace command lists का कौनो प्रभाव नाहीं होतै। सक्रिय करतै पर user-level entries कौ प्राथमिकत मिलतै है: हर user-level entry उन सब्ह commands कौ तय करतै है जिनसै वह मेल खातै है, अऊर workspace entries केवल उन commands पर लागू होतै हैं जिनसै कौनो user-level entry मेल नाहीं खातै। इस setting कौ केवल [user settings](https://code.visualstudio.com/docs/getstarted/settings) में बदल जा सकतै है।" } diff --git a/src/package.nls.id.json b/src/package.nls.id.json index c49e54a34e..a7da9e8d62 100644 --- a/src/package.nls.id.json +++ b/src/package.nls.id.json @@ -48,5 +48,6 @@ "settings.debugProxy.tlsInsecure.description": "Terima sertifikat self-signed dari proxy. **Diperlukan untuk inspeksi MITM.** ⚠️ Tidak aman — gunakan hanya untuk debugging lokal.", "settings.workspace.rootResolution.description": "Bagaimana Zoo menentukan root workspace di workspace multi-root. Root digunakan untuk menemukan `.roomodes`, `.roo/mcp.json`, `.roo/rules/`, dan konfigurasi lain yang bersifat project-scoped. Mengubah pengaturan ini hanya memengaruhi pencarian berikutnya; task yang sedang berjalan tetap memakai root aslinya.", "settings.workspace.rootResolution.activeEditor.description": "Gunakan folder workspace yang berisi editor aktif; kembali ke folder workspace pertama. (Default — mempertahankan perilaku lama.)", - "settings.workspace.rootResolution.firstFolder.description": "Selalu gunakan folder workspace pertama (workspaceFolders[0]). Deterministik — tidak bergantung pada file mana yang sedang difokuskan." + "settings.workspace.rootResolution.firstFolder.description": "Selalu gunakan folder workspace pertama (workspaceFolders[0]). Deterministik — tidak bergantung pada file mana yang sedang difokuskan.", + "settings.workspaceCommandListsEnabled.description": "Aktifkan daftar perintah yang disediakan oleh pengaturan workspace (nilai `zoo-code.allowedCommands` dan `zoo-code.deniedCommands` di file pengaturan workspace). Saat dinonaktifkan, daftar perintah workspace tidak memiliki efek apa pun. Saat diaktifkan, entri level pengguna lebih diutamakan: entri level pengguna menentukan perintah mana pun yang cocok dengannya, dan entri workspace hanya berlaku untuk perintah yang tidak cocok dengan entri level pengguna mana pun. Pengaturan ini hanya dapat diubah di [pengaturan pengguna](https://code.visualstudio.com/docs/getstarted/settings)." } diff --git a/src/package.nls.it.json b/src/package.nls.it.json index 2a4b95f954..a3b557de7d 100644 --- a/src/package.nls.it.json +++ b/src/package.nls.it.json @@ -48,5 +48,6 @@ "settings.debugProxy.tlsInsecure.description": "Accetta certificati autofirmati dal proxy. **Necessario per l'ispezione MITM.** ⚠️ Non sicuro — usalo solo per il debugging locale.", "settings.workspace.rootResolution.description": "Come Zoo determina la radice del workspace in un workspace multi-radice. La radice viene usata per individuare `.roomodes`, `.roo/mcp.json`, `.roo/rules/` e altra configurazione a livello di progetto. Modificare questa impostazione influisce solo sulle ricerche future; i task in esecuzione mantengono la loro radice originale.", "settings.workspace.rootResolution.activeEditor.description": "Usa la cartella del workspace che contiene l'editor attivo; ripiega sulla prima cartella del workspace. (Predefinito — mantiene il comportamento legacy.)", - "settings.workspace.rootResolution.firstFolder.description": "Usa sempre la prima cartella del workspace (workspaceFolders[0]). Deterministico — indipendente dal file attualmente attivo." + "settings.workspace.rootResolution.firstFolder.description": "Usa sempre la prima cartella del workspace (workspaceFolders[0]). Deterministico — indipendente dal file attualmente attivo.", + "settings.workspaceCommandListsEnabled.description": "Abilita gli elenchi di comandi forniti dalle impostazioni del workspace (i valori di `zoo-code.allowedCommands` e `zoo-code.deniedCommands` nei file delle impostazioni del workspace). Finché è disabilitata, gli elenchi di comandi del workspace non hanno alcun effetto. Quando è abilitata, le voci a livello utente hanno la precedenza: una voce a livello utente decide qualsiasi comando con cui corrisponde, e le voci del workspace si applicano solo ai comandi con cui nessuna voce a livello utente corrisponde. Questa impostazione può essere modificata solo nelle [impostazioni utente](https://code.visualstudio.com/docs/getstarted/settings)." } diff --git a/src/package.nls.ja.json b/src/package.nls.ja.json index 0f3f4a3cee..c5592cecb2 100644 --- a/src/package.nls.ja.json +++ b/src/package.nls.ja.json @@ -48,5 +48,6 @@ "settings.debugProxy.tlsInsecure.description": "プロキシからの自己署名証明書を許可します。**MITM インスペクションに必須です。** ⚠️ 危険な設定なので、ローカルでのデバッグにだけ使用してください。", "settings.workspace.rootResolution.description": "マルチルートワークスペースで Zoo がワークスペースルートをどのように解決するか。ルートは `.roomodes`、`.roo/mcp.json`、`.roo/rules/` などのプロジェクト単位の設定を見つけるために使われます。この設定の変更は今後の検索にのみ影響し、実行中のタスクは元のルートを保持します。", "settings.workspace.rootResolution.activeEditor.description": "アクティブなエディタを含むワークスペースフォルダを使用し、最初のワークスペースフォルダにフォールバックします。（デフォルト — 従来の動作を維持します。）", - "settings.workspace.rootResolution.firstFolder.description": "常に最初のワークスペースフォルダ（workspaceFolders[0]）を使用します。決定的で、現在フォーカスされているファイルに依存しません。" + "settings.workspace.rootResolution.firstFolder.description": "常に最初のワークスペースフォルダ（workspaceFolders[0]）を使用します。決定的で、現在フォーカスされているファイルに依存しません。", + "settings.workspaceCommandListsEnabled.description": "ワークスペース設定が提供するコマンドリスト（ワークスペースの設定ファイル内の `zoo-code.allowedCommands` と `zoo-code.deniedCommands` の値）を有効にします。無効の間は、ワークスペースのコマンドリストは効果を持ちません。有効にすると、ユーザーレベルのエントリが優先されます。ユーザーレベルのエントリは、一致するコマンドをすべて決定し、ワークスペースのエントリはユーザーレベルのエントリが一致しないコマンドにのみ適用されます。この設定は[ユーザー設定](https://code.visualstudio.com/docs/getstarted/settings)でのみ変更できます。" } diff --git a/src/package.nls.json b/src/package.nls.json index fe10839819..bc4b7d15ca 100644 --- a/src/package.nls.json +++ b/src/package.nls.json @@ -48,5 +48,6 @@ "settings.debugProxy.tlsInsecure.description": "Accept self-signed certificates from the proxy. **Required for MITM inspection.** ⚠️ Insecure — only use for local debugging.", "settings.workspace.rootResolution.description": "How Zoo resolves the workspace root in a multi-root workspace. The root is used to locate `.roomodes`, `.roo/mcp.json`, `.roo/rules/`, and other project-scoped configuration. Changing this setting only affects future lookups; running tasks keep their original root.", "settings.workspace.rootResolution.activeEditor.description": "Use the workspace folder containing the active editor; fall back to the first workspace folder. (Default — preserves legacy behavior.)", - "settings.workspace.rootResolution.firstFolder.description": "Always use the first workspace folder (workspaceFolders[0]). Deterministic — independent of which file is currently focused." + "settings.workspace.rootResolution.firstFolder.description": "Always use the first workspace folder (workspaceFolders[0]). Deterministic — independent of which file is currently focused.", + "settings.workspaceCommandListsEnabled.description": "Enable command lists contributed by workspace settings (`zoo-code.allowedCommands` and `zoo-code.deniedCommands` values in workspace settings files). While disabled, workspace command lists have no effect. When enabled, user-level entries take precedence: a user-level entry decides any command it matches, and workspace entries only apply to commands no user-level entry matches. This setting can only be changed in [user settings](https://code.visualstudio.com/docs/getstarted/settings)." } diff --git a/src/package.nls.ko.json b/src/package.nls.ko.json index 99a064cd6c..ec6c1a610b 100644 --- a/src/package.nls.ko.json +++ b/src/package.nls.ko.json @@ -48,5 +48,6 @@ "settings.debugProxy.tlsInsecure.description": "프록시의 self-signed 인증서를 허용합니다. **MITM 검사에 필요합니다.** ⚠️ 안전하지 않으므로 로컬 디버깅에만 사용하세요.", "settings.workspace.rootResolution.description": "멀티 루트 워크스페이스에서 Zoo가 워크스페이스 루트를 어떻게 결정하는지 설정합니다. 루트는 `.roomodes`, `.roo/mcp.json`, `.roo/rules/` 및 기타 프로젝트 범위 구성을 찾는 데 사용됩니다. 이 설정을 변경해도 이후의 조회에만 영향을 주며, 실행 중인 작업은 원래 루트를 유지합니다.", "settings.workspace.rootResolution.activeEditor.description": "활성 편집기가 있는 워크스페이스 폴더를 사용하고, 첫 번째 워크스페이스 폴더로 폴백합니다. (기본값 — 기존 동작을 유지합니다.)", - "settings.workspace.rootResolution.firstFolder.description": "항상 첫 번째 워크스페이스 폴더(workspaceFolders[0])를 사용합니다. 결정적이며, 현재 포커스된 파일과 무관합니다." + "settings.workspace.rootResolution.firstFolder.description": "항상 첫 번째 워크스페이스 폴더(workspaceFolders[0])를 사용합니다. 결정적이며, 현재 포커스된 파일과 무관합니다.", + "settings.workspaceCommandListsEnabled.description": "워크스페이스 설정이 제공하는 명령 목록을 활성화합니다(워크스페이스 설정 파일의 `zoo-code.allowedCommands` 및 `zoo-code.deniedCommands` 값). 비활성화된 동안에는 워크스페이스 명령 목록이 효과가 없습니다. 활성화하면 사용자 수준 항목이 우선합니다: 사용자 수준 항목은 자신이 일치시키는 모든 명령을 결정하며, 워크스페이스 항목은 사용자 수준 항목이 일치시키지 않는 명령에만 적용됩니다. 이 설정은 [사용자 설정](https://code.visualstudio.com/docs/getstarted/settings)에서만 변경할 수 있습니다." } diff --git a/src/package.nls.nl.json b/src/package.nls.nl.json index 27948f933a..9704fc084c 100644 --- a/src/package.nls.nl.json +++ b/src/package.nls.nl.json @@ -48,5 +48,6 @@ "settings.debugProxy.tlsInsecure.description": "Accepteer zelfondertekende certificaten van de proxy. **Vereist voor MITM-inspectie.** ⚠️ Onveilig — gebruik dit alleen voor lokale debugging.", "settings.workspace.rootResolution.description": "Hoe Zoo de workspace-root bepaalt in een multi-root workspace. De root wordt gebruikt om `.roomodes`, `.roo/mcp.json`, `.roo/rules/` en andere project-scoped configuratie te vinden. Deze instelling wijzigen heeft alleen invloed op toekomstige zoekopdrachten; lopende taken behouden hun oorspronkelijke root.", "settings.workspace.rootResolution.activeEditor.description": "Gebruik de workspace-map die de actieve editor bevat; val terug op de eerste workspace-map. (Standaard — behoudt het oude gedrag.)", - "settings.workspace.rootResolution.firstFolder.description": "Gebruik altijd de eerste workspace-map (workspaceFolders[0]). Deterministisch — onafhankelijk van welk bestand op dit moment de focus heeft." + "settings.workspace.rootResolution.firstFolder.description": "Gebruik altijd de eerste workspace-map (workspaceFolders[0]). Deterministisch — onafhankelijk van welk bestand op dit moment de focus heeft.", + "settings.workspaceCommandListsEnabled.description": "Schakel commandolijsten in die worden bijgedragen door werkruimte-instellingen (de waarden van `zoo-code.allowedCommands` en `zoo-code.deniedCommands` in instellingenbestanden van de werkruimte). Terwijl uitgeschakeld hebben commandolijsten van de werkruimte geen effect. Wanneer ingeschakeld hebben items op gebruikersniveau voorrang: een item op gebruikersniveau beslist elk commando dat ermee overeenkomt, en werkruimte-items zijn alleen van toepassing op commando's waar geen item op gebruikersniveau mee overeenkomt. Deze instelling kan alleen worden gewijzigd in de [gebruikersinstellingen](https://code.visualstudio.com/docs/getstarted/settings)." } diff --git a/src/package.nls.pl.json b/src/package.nls.pl.json index 8fb8b6eb83..73755bdcfd 100644 --- a/src/package.nls.pl.json +++ b/src/package.nls.pl.json @@ -48,5 +48,6 @@ "settings.debugProxy.tlsInsecure.description": "Akceptuj certyfikaty self-signed z proxy. **Wymagane do inspekcji MITM.** ⚠️ Niezabezpieczone — używaj tylko do lokalnego debugowania.", "settings.workspace.rootResolution.description": "Jak Zoo ustala katalog główny workspace w workspace z wieloma katalogami głównymi. Katalog główny służy do lokalizowania `.roomodes`, `.roo/mcp.json`, `.roo/rules/` i innej konfiguracji na poziomie projektu. Zmiana tego ustawienia wpływa tylko na przyszłe wyszukiwania; uruchomione zadania zachowują swój pierwotny katalog główny.", "settings.workspace.rootResolution.activeEditor.description": "Użyj folderu workspace zawierającego aktywny edytor; w razie potrzeby wróć do pierwszego folderu workspace. (Domyślnie — zachowuje dotychczasowe zachowanie.)", - "settings.workspace.rootResolution.firstFolder.description": "Zawsze używaj pierwszego folderu workspace (workspaceFolders[0]). Deterministyczne — niezależne od tego, który plik jest aktualnie aktywny." + "settings.workspace.rootResolution.firstFolder.description": "Zawsze używaj pierwszego folderu workspace (workspaceFolders[0]). Deterministyczne — niezależne od tego, który plik jest aktualnie aktywny.", + "settings.workspaceCommandListsEnabled.description": "Włącza listy poleceń dostarczane przez ustawienia workspace (wartości `zoo-code.allowedCommands` i `zoo-code.deniedCommands` w plikach ustawień workspace). Gdy jest wyłączona, listy poleceń workspace nie mają żadnego efektu. Gdy jest włączona, wpisy na poziomie użytkownika mają pierwszeństwo: wpis na poziomie użytkownika rozstrzyga każde polecenie, z którym się dopasuje, a wpisy workspace dotyczą tylko poleceń, z którymi nie dopasuje się żaden wpis na poziomie użytkownika. To ustawienie można zmienić tylko w [ustawieniach użytkownika](https://code.visualstudio.com/docs/getstarted/settings)." } diff --git a/src/package.nls.pt-BR.json b/src/package.nls.pt-BR.json index b4cae4644b..eb5370e919 100644 --- a/src/package.nls.pt-BR.json +++ b/src/package.nls.pt-BR.json @@ -48,5 +48,6 @@ "settings.debugProxy.tlsInsecure.description": "Aceitar certificados self-signed do proxy. **Necessário para inspeção MITM.** ⚠️ Inseguro — use apenas para depuração local.", "settings.workspace.rootResolution.description": "Como o Zoo resolve a raiz do workspace em um workspace multi-raiz. A raiz é usada para localizar `.roomodes`, `.roo/mcp.json`, `.roo/rules/` e outras configurações no escopo do projeto. Alterar esta configuração afeta apenas as buscas futuras; as tarefas em execução mantêm sua raiz original.", "settings.workspace.rootResolution.activeEditor.description": "Usa a pasta do workspace que contém o editor ativo; recorre à primeira pasta do workspace. (Padrão — preserva o comportamento legado.)", - "settings.workspace.rootResolution.firstFolder.description": "Usa sempre a primeira pasta do workspace (workspaceFolders[0]). Determinístico — independente de qual arquivo está em foco no momento." + "settings.workspace.rootResolution.firstFolder.description": "Usa sempre a primeira pasta do workspace (workspaceFolders[0]). Determinístico — independente de qual arquivo está em foco no momento.", + "settings.workspaceCommandListsEnabled.description": "Ativa as listas de comandos fornecidas pelas configurações do workspace (os valores de `zoo-code.allowedCommands` e `zoo-code.deniedCommands` nos arquivos de configurações do workspace). Enquanto desativada, as listas de comandos do workspace não têm efeito. Quando ativada, as entradas em nível de usuário têm precedência: uma entrada em nível de usuário decide qualquer comando com o qual corresponda, e as entradas do workspace se aplicam apenas a comandos com os quais nenhuma entrada em nível de usuário corresponde. Essa configuração só pode ser alterada nas [configurações de usuário](https://code.visualstudio.com/docs/getstarted/settings)." } diff --git a/src/package.nls.ru.json b/src/package.nls.ru.json index 0e61002848..141e1cdb0d 100644 --- a/src/package.nls.ru.json +++ b/src/package.nls.ru.json @@ -48,5 +48,6 @@ "settings.debugProxy.tlsInsecure.description": "Принимать self-signed сертификаты от прокси. **Требуется для MITM-инспекции.** ⚠️ Небезопасно — используй только для локальной отладки.", "settings.workspace.rootResolution.description": "Как Zoo определяет корень рабочей области в рабочей области с несколькими корнями. Корень используется для поиска `.roomodes`, `.roo/mcp.json`, `.roo/rules/` и другой конфигурации уровня проекта. Изменение этой настройки влияет только на последующие поиски; выполняющиеся задачи сохраняют свой исходный корень.", "settings.workspace.rootResolution.activeEditor.description": "Использовать папку рабочей области, содержащую активный редактор; при необходимости вернуться к первой папке рабочей области. (По умолчанию — сохраняет прежнее поведение.)", - "settings.workspace.rootResolution.firstFolder.description": "Всегда использовать первую папку рабочей области (workspaceFolders[0]). Детерминированно — не зависит от того, какой файл сейчас в фокусе." + "settings.workspace.rootResolution.firstFolder.description": "Всегда использовать первую папку рабочей области (workspaceFolders[0]). Детерминированно — не зависит от того, какой файл сейчас в фокусе.", + "settings.workspaceCommandListsEnabled.description": "Включает списки команд, задаваемые в настройках рабочей области (значения `zoo-code.allowedCommands` и `zoo-code.deniedCommands` в файлах настроек рабочей области). Пока отключено, списки команд рабочей области ни на что не влияют. Когда включено, записи уровня пользователя имеют приоритет: запись уровня пользователя решает любую подошедшую ей команду, а записи рабочей области применяются только к командам, которые не совпали ни с одной записью уровня пользователя. Этот параметр можно изменить только в [настройках пользователя](https://code.visualstudio.com/docs/getstarted/settings)." } diff --git a/src/package.nls.tr.json b/src/package.nls.tr.json index 1842156d75..5c9c06dd9a 100644 --- a/src/package.nls.tr.json +++ b/src/package.nls.tr.json @@ -48,5 +48,6 @@ "settings.debugProxy.tlsInsecure.description": "Proxy'den gelen self-signed sertifikaları kabul et. **MITM incelemesi için gerekli.** ⚠️ Güvensiz — yalnızca lokal debugging için kullan.", "settings.workspace.rootResolution.description": "Zoo'nun çok köklü bir çalışma alanında çalışma alanı kökünü nasıl belirlediği. Kök; `.roomodes`, `.roo/mcp.json`, `.roo/rules/` ve diğer proje kapsamındaki yapılandırmayı bulmak için kullanılır. Bu ayarı değiştirmek yalnızca sonraki aramaları etkiler; çalışan görevler özgün köklerini korur.", "settings.workspace.rootResolution.activeEditor.description": "Aktif düzenleyiciyi içeren çalışma alanı klasörünü kullan; ilk çalışma alanı klasörüne geri dön. (Varsayılan — eski davranışı korur.)", - "settings.workspace.rootResolution.firstFolder.description": "Her zaman ilk çalışma alanı klasörünü (workspaceFolders[0]) kullan. Deterministik — o anda hangi dosyanın odakta olduğundan bağımsız." + "settings.workspace.rootResolution.firstFolder.description": "Her zaman ilk çalışma alanı klasörünü (workspaceFolders[0]) kullan. Deterministik — o anda hangi dosyanın odakta olduğundan bağımsız.", + "settings.workspaceCommandListsEnabled.description": "Çalışma alanı ayarlarının sağladığı komut listelerini etkinleştirir (çalışma alanı ayar dosyalarındaki `zoo-code.allowedCommands` ve `zoo-code.deniedCommands` değerleri). Devre dışıyken çalışma alanı komut listelerinin hiçbir etkisi olmaz. Etkinleştirildiğinde kullanıcı düzeyi girdiler önceliklidir: bir kullanıcı düzeyi girdi eşleştiği her komutu kararlaştırır ve çalışma alanı girdileri yalnızca hiçbir kullanıcı düzeyi girdinin eşleşmediği komutlara uygulanır. Bu ayar yalnızca [kullanıcı ayarlarında](https://code.visualstudio.com/docs/getstarted/settings) değiştirilebilir." } diff --git a/src/package.nls.vi.json b/src/package.nls.vi.json index c846978793..73b4be5dee 100644 --- a/src/package.nls.vi.json +++ b/src/package.nls.vi.json @@ -48,5 +48,6 @@ "settings.debugProxy.tlsInsecure.description": "Chấp nhận chứng chỉ self-signed từ proxy. **Bắt buộc cho việc kiểm tra MITM.** ⚠️ Không an toàn — chỉ dùng cho debug cục bộ.", "settings.workspace.rootResolution.description": "Cách Zoo xác định gốc workspace trong một workspace nhiều gốc. Gốc được dùng để tìm `.roomodes`, `.roo/mcp.json`, `.roo/rules/` và các cấu hình khác ở phạm vi dự án. Thay đổi cài đặt này chỉ ảnh hưởng đến các lần tra cứu về sau; các tác vụ đang chạy vẫn giữ gốc ban đầu của chúng.", "settings.workspace.rootResolution.activeEditor.description": "Dùng thư mục workspace chứa trình soạn thảo đang hoạt động; quay lại thư mục workspace đầu tiên. (Mặc định — giữ nguyên hành vi cũ.)", - "settings.workspace.rootResolution.firstFolder.description": "Luôn dùng thư mục workspace đầu tiên (workspaceFolders[0]). Mang tính xác định — không phụ thuộc vào tệp nào đang được focus." + "settings.workspace.rootResolution.firstFolder.description": "Luôn dùng thư mục workspace đầu tiên (workspaceFolders[0]). Mang tính xác định — không phụ thuộc vào tệp nào đang được focus.", + "settings.workspaceCommandListsEnabled.description": "Bật các danh sách lệnh do cài đặt workspace đóng góp (giá trị `zoo-code.allowedCommands` và `zoo-code.deniedCommands` trong các tệp cài đặt workspace). Khi bị tắt, các danh sách lệnh của workspace không có tác dụng. Khi được bật, các mục cấp người dùng được ưu tiên: một mục cấp người dùng quyết định bất kỳ lệnh nào mà nó khớp, và các mục của workspace chỉ áp dụng cho các lệnh không có mục cấp người dùng nào khớp. Cài đặt này chỉ có thể thay đổi trong [cài đặt người dùng](https://code.visualstudio.com/docs/getstarted/settings)." } diff --git a/src/package.nls.zh-CN.json b/src/package.nls.zh-CN.json index e2705e9e2f..b62e8cdb64 100644 --- a/src/package.nls.zh-CN.json +++ b/src/package.nls.zh-CN.json @@ -48,5 +48,6 @@ "settings.debugProxy.tlsInsecure.description": "接受来自代理的 self-signed 证书。**MITM 检查所必需。** ⚠️ 不安全——只在本地调试时使用。", "settings.workspace.rootResolution.description": "Zoo 在多根工作区中如何解析工作区根目录。根目录用于定位 `.roomodes`、`.roo/mcp.json`、`.roo/rules/` 以及其他项目范围的配置。更改此设置仅影响后续查找；正在运行的任务会保留其原始根目录。", "settings.workspace.rootResolution.activeEditor.description": "使用包含活动编辑器的工作区文件夹；回退到第一个工作区文件夹。（默认——保留旧行为。）", - "settings.workspace.rootResolution.firstFolder.description": "始终使用第一个工作区文件夹（workspaceFolders[0]）。具有确定性——与当前聚焦的文件无关。" + "settings.workspace.rootResolution.firstFolder.description": "始终使用第一个工作区文件夹（workspaceFolders[0]）。具有确定性——与当前聚焦的文件无关。", + "settings.workspaceCommandListsEnabled.description": "启用由工作区设置提供的命令列表（工作区设置文件中的 `zoo-code.allowedCommands` 和 `zoo-code.deniedCommands` 值）。禁用期间，工作区命令列表不产生任何影响。启用后，用户级条目具有优先权：用户级条目决定其匹配的任何命令，工作区条目仅适用于没有任何用户级条目匹配的命令。此设置只能在[用户设置](https://code.visualstudio.com/docs/getstarted/settings)中更改。" } diff --git a/src/package.nls.zh-TW.json b/src/package.nls.zh-TW.json index 723d8d08ad..904867e1b4 100644 --- a/src/package.nls.zh-TW.json +++ b/src/package.nls.zh-TW.json @@ -48,5 +48,6 @@ "settings.debugProxy.tlsInsecure.description": "接受來自代理的 self-signed 憑證。**MITM 檢查所必需。** ⚠️ 不安全——只在本機偵錯時使用。", "settings.workspace.rootResolution.description": "Zoo 在多根工作區中如何解析工作區根目錄。根目錄用於定位 `.roomodes`、`.roo/mcp.json`、`.roo/rules/` 以及其他專案範圍的設定。變更此設定僅影響後續查找；正在執行的工作會保留其原始根目錄。", "settings.workspace.rootResolution.activeEditor.description": "使用包含使用中編輯器的工作區資料夾；回退到第一個工作區資料夾。（預設——保留舊行為。）", - "settings.workspace.rootResolution.firstFolder.description": "一律使用第一個工作區資料夾（workspaceFolders[0]）。具決定性——與目前聚焦的檔案無關。" + "settings.workspace.rootResolution.firstFolder.description": "一律使用第一個工作區資料夾（workspaceFolders[0]）。具決定性——與目前聚焦的檔案無關。", + "settings.workspaceCommandListsEnabled.description": "啟用由工作區設定所提供的工作區指令清單（工作區設定檔案中的 `zoo-code.allowedCommands` 與 `zoo-code.deniedCommands` 值）。停用時，工作區指令清單不會產生任何影響。啟用後，使用者層級的條目具有優先權：使用者層級的條目會決定其符合的任何指令，工作區條目僅適用於沒有任何使用者層級條目符合的指令。此設定只能在[使用者設定](https://code.visualstudio.com/docs/getstarted/settings)中變更。" } From 10e945669fa4af15ed19cd7d76bc118dd9db9306 Mon Sep 17 00:00:00 2001 From: Franz Daubner Date: Thu, 5 Nov 2026 19:43:40 +0100 Subject: [PATCH 7/9] fix: harden command-list save path and resolve lists per task folder (#1939) Review fixes on the workspace-scoped command-list feature: - The hidden-entry check during a settings save now compares the settings file against a snapshot of each command list's global layer taken before the save loop, so the result no longer depends on which key is processed first. - The final extension-state write at the end of a settings save is guarded: if it is rejected, the error names the layers that did and did not receive the update instead of failing silently, and the save continues. - Workspace command lists for an approval decision now resolve from the workspace folder that contains the task's working directory instead of always the first folder; a working directory that names no folder falls back to the first-folder configuration. The state sent to the webview carries the global layers alongside the window-scoped pair. - An open settings dialog merges external command-list updates into its buffer, holding back a list the user is editing, and a save sends the lists it last displayed as a baseline. Entries the settings file gained while the dialog was open are therefore preserved on save and named in a warning instead of being erased; deletions made in the dialog still apply. - Test coverage added for the unset gate value, both key orders of the save snapshot, the rejected state write, per-folder resolution, and the dialog reseed/baseline behavior. --- packages/types/src/vscode-extension-host.ts | 20 ++ .../__tests__/commandScope.spec.ts | 162 ++++++++- src/core/auto-approval/commandScope.ts | 60 +++- src/core/auto-approval/index.ts | 34 +- src/core/webview/ClineProvider.ts | 17 +- .../__tests__/webviewMessageHandler.spec.ts | 307 +++++++++++++++++- src/core/webview/webviewMessageHandler.ts | 72 +++- .../src/components/settings/SettingsView.tsx | 110 +++++++ .../SettingsView.change-detection.spec.tsx | 125 ++++++- .../settings/__tests__/SettingsView.spec.tsx | 26 ++ .../SettingsView.unsaved-changes.spec.tsx | 132 ++++++++ 11 files changed, 1026 insertions(+), 39 deletions(-) diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index bfe5632af6..e9d94e3e55 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -384,6 +384,18 @@ export type ExtensionState = Pick< toolRequirements?: Record // Map of tool names to their requirements (e.g. {"apply_diff": true}) cwd?: string // Current working directory + + /** + * The user-scope command lists as persisted in extension global state, + * before any workspace-scoped composition. Auto-approval composes the + * pair it enforces from these raw layers plus the folder the task runs + * in, because the merged `allowedCommands`/`deniedCommands` pair follows + * the window's first folder and would judge a task by another root's + * lists. Not user settings: runtime state, written by the provider's + * state bake, never by the settings dialog. + */ + globalAllowedCommands?: string[] + globalDeniedCommands?: string[] telemetrySetting: TelemetrySetting telemetryKey?: string machineId?: string @@ -809,6 +821,14 @@ export interface WebviewMessage { codebaseIndexOpenRouterApiKey?: string } updatedSettings?: RooCodeSettings + /** + * The command lists as the settings dialog last displayed them, sent alongside the + * save that rewrites them. The settings file can gain entries while the dialog is + * open, and without this baseline the save path cannot tell those additions apart + * from list entries the user deleted: both look like "in the file, absent from the + * submitted list". Senders that do not display the lists omit it. + */ + commandListBaseline?: { allowedCommands?: string[]; deniedCommands?: string[] } /** Task configuration applied via `createTask()`. */ taskConfiguration?: RooCodeSettings // Worktree properties diff --git a/src/core/auto-approval/__tests__/commandScope.spec.ts b/src/core/auto-approval/__tests__/commandScope.spec.ts index 4d1064d23d..15598f4caf 100644 --- a/src/core/auto-approval/__tests__/commandScope.spec.ts +++ b/src/core/auto-approval/__tests__/commandScope.spec.ts @@ -13,12 +13,17 @@ import { splitCommandListScopes, workspaceCommandListsEnabled, } from "../commandScope" +import { checkAutoApproval } from "../index" +import { baseState, type State } from "./fixtures" // Scriptable stand-in for `vscode.workspace.workspaceFolders`: the module // mock's getter reads through this object, so a test can change the open -// folders without re-mocking the module. +// folders without re-mocking the module. `foldersByPath` backs +// `getWorkspaceFolder`: keys are folder paths, values the folder objects, so +// a test controls which cwds resolve to a root. const workspaceState = vi.hoisted(() => ({ workspaceFolders: undefined as { uri: unknown }[] | undefined, + foldersByPath: new Map(), })) vi.mock("vscode", () => ({ @@ -27,11 +32,25 @@ vi.mock("vscode", () => ({ Workspace: 2, WorkspaceFolder: 3, }, + Uri: { + // Structural local stand-in: the resolver only carries the path + // through to `getWorkspaceFolder`. + file: (fsPath: string) => ({ scheme: "file", authority: "", path: fsPath, fsPath, query: "", fragment: "" }), + }, workspace: { get workspaceFolders() { return workspaceState.workspaceFolders }, getConfiguration: vi.fn(), + getWorkspaceFolder: vi.fn((resource?: { fsPath?: string }) => { + const fsPath = resource?.fsPath ?? "" + for (const [folderPath, uri] of workspaceState.foldersByPath) { + if (fsPath === folderPath || fsPath.startsWith(`${folderPath}/`)) { + return { uri } + } + } + return undefined + }), }, })) @@ -345,6 +364,20 @@ it("workspaceCommandListsEnabled: enables the workspace layers only when the gat expect(config.get).toHaveBeenCalledWith("workspaceCommandListsEnabled") }) +it("workspaceCommandListsEnabled: is off when the gate is unset", () => { + vi.clearAllMocks() + // Unset is the state of every installation that has never opted in, so it + // must read as disabled: treating unset as enabled would widen + // auto-approval by default. The equality against the declared default also + // keeps this expectation attached to the constant that documents the + // shipped default, not to an independently hard-coded boolean. + const config = mockGateConfiguration(undefined) + + expect(workspaceCommandListsEnabled()).toBe(false) + expect(workspaceCommandListsEnabled()).toBe(DEFAULT_WORKSPACE_COMMAND_LISTS_ENABLED) + expect(config.get).toHaveBeenCalledWith("workspaceCommandListsEnabled") +}) + it("workspaceCommandListsEnabled: keeps the workspace layers disabled when the gate lookup throws", () => { vi.clearAllMocks() // Fail-closed: a broken configuration read must never widen what the @@ -441,3 +474,130 @@ it("resolveEffectiveCommandLists: shadows workspace entries case-insensitively, expect(effective.hiddenWorkspaceAllow).toEqual(["git status"]) expect(effective.hiddenWorkspaceDeny).toEqual([]) }) + +// Command-branch enforcement composes its allow/deny pair from the raw +// global layers at the folder the task's cwd resolves to, so a task in +// another root is judged by that root's lists rather than the window's +// first folder. The window-scoped pair on the state stays what the dialog +// displays. + +function openMultiRootWindow() { + const folderA = makeUri("/mock/folder-a") + const folderB = makeUri("/mock/folder-b") + workspaceState.workspaceFolders = [{ uri: folderA }, { uri: folderB }] + workspaceState.foldersByPath.set(folderA.fsPath, folderA) + workspaceState.foldersByPath.set(folderB.fsPath, folderB) + return { folderA, folderB } +} + +// Scripts each folder's configuration independently so the test can tell +// which folder's settings the enforcement path actually read. +function mockFolderConfigurations(scripts: Record) { + vi.mocked(vscode.workspace.getConfiguration).mockImplementation( + (_section?: string, scope?: vscode.ConfigurationScope | null) => { + const entry = scripts[scope && "fsPath" in scope ? scope.fsPath : ""] + const config = makeWorkspaceConfiguration({ workspaceCommandListsEnabled: entry?.gate ?? false }) + vi.mocked(config.inspect).mockImplementation((key: string) => + key === "deniedCommands" && entry?.deny ? { key, workspaceFolderValue: entry.deny } : undefined, + ) + return config + }, + ) +} + +// Raw global layers empty: every enforcement decision then comes from the +// folder layer, so a decision can only be right for one folder. The baked +// pair on the state is set to values no folder composes, so a check that +// consumed it instead of re-composing decides differently. +const taskFolderState = (bakedAllowed: string[]): State => ({ + ...baseState, + alwaysAllowExecute: true, + allowedCommands: bakedAllowed, + deniedCommands: [], + globalAllowedCommands: [], + globalDeniedCommands: [], +}) + +it("checkAutoApproval command: enforces the deny entries of the folder the task's cwd resolves to", async () => { + vi.clearAllMocks() + const { folderA, folderB } = openMultiRootWindow() + // Folder A denies "echo", folder B denies "rm"; the task runs in B. + mockFolderConfigurations({ + [folderA.fsPath]: { gate: true, deny: ["echo"] }, + [folderB.fsPath]: { gate: true, deny: ["rm"] }, + }) + + // The baked pair would approve exactly what folder B's layer denies. + const result = await checkAutoApproval({ + state: taskFolderState(["rm"]), + cwd: folderB.fsPath, + ask: "command", + text: "rm file", + }) + + expect(result).toEqual({ decision: "deny", autoDeny: { kind: "denylist", command: "rm file", pattern: "rm" } }) + expect(vscode.workspace.getConfiguration).toHaveBeenCalledWith(Package.name, folderB) +}) + +it("checkAutoApproval command: does not apply another root's deny entries to a task outside it", async () => { + vi.clearAllMocks() + const { folderA, folderB } = openMultiRootWindow() + mockFolderConfigurations({ + [folderA.fsPath]: { gate: true, deny: ["echo"] }, + [folderB.fsPath]: { gate: true, deny: ["rm"] }, + }) + + // "echo" is denied only in the first folder; judging this B-rooted task + // there would auto-deny it. Nothing allows it either, so the correct + // multi-root decision is the plain user prompt. + const result = await checkAutoApproval({ + state: taskFolderState(["echo"]), + cwd: folderB.fsPath, + ask: "command", + text: "echo hi", + }) + + expect(result).toEqual({ decision: "ask" }) +}) + +it("checkAutoApproval command: falls back to the first folder when the cwd resolves to no workspace root", async () => { + vi.clearAllMocks() + const { folderA, folderB } = openMultiRootWindow() + mockFolderConfigurations({ + [folderA.fsPath]: { gate: true, deny: ["echo"] }, + [folderB.fsPath]: { gate: true, deny: ["rm"] }, + }) + + // A removed or relocated folder names no root; enforcement stays on the + // first-folder configuration it used before per-task scoping existed. + const result = await checkAutoApproval({ + state: taskFolderState(["echo"]), + cwd: "/mock/removed-folder", + ask: "command", + text: "echo hi", + }) + + expect(result).toEqual({ decision: "deny", autoDeny: { kind: "denylist", command: "echo hi", pattern: "echo" } }) + expect(vscode.workspace.getConfiguration).toHaveBeenCalledWith(Package.name, folderA) + expect(vscode.workspace.getConfiguration).not.toHaveBeenCalledWith(Package.name, folderB) +}) + +it("checkAutoApproval command: in a single-folder window reads exactly the same configuration as before", async () => { + vi.clearAllMocks() + const { folderA } = openMultiRootWindow() + workspaceState.workspaceFolders = [{ uri: folderA }] + workspaceState.foldersByPath.delete(makeUri("/mock/folder-b").fsPath) + // The one open folder is both the task folder and the first folder, so + // the resolved scope and the no-arg default are the same resource. + mockFolderConfigurations({ [folderA.fsPath]: { gate: true, deny: ["echo"] } }) + + const result = await checkAutoApproval({ + state: taskFolderState(["echo"]), + cwd: folderA.fsPath, + ask: "command", + text: "echo hi", + }) + + expect(result).toEqual({ decision: "deny", autoDeny: { kind: "denylist", command: "echo hi", pattern: "echo" } }) + expect(vscode.workspace.getConfiguration).toHaveBeenCalledWith(Package.name, folderA) +}) diff --git a/src/core/auto-approval/commandScope.ts b/src/core/auto-approval/commandScope.ts index 03d464559f..3ceed09a9f 100644 --- a/src/core/auto-approval/commandScope.ts +++ b/src/core/auto-approval/commandScope.ts @@ -24,17 +24,52 @@ export interface CommandListInspection { } /** - * The configuration object for every command-list read and write, scoped to - * the first workspace folder. Sharing one scoped access path keeps the two - * sides symmetric: `inspect()` can report `workspaceFolderValue`, and - * `ConfigurationTarget.WorkspaceFolder` updates are legal (the API contract - * throws for that target when the configuration is not scoped to a - * resource). In a multi-root workspace only the first folder's values are in - * scope; with no folder open the configuration is unscoped and reports no - * folder value. + * The configuration object for every command-list read and write. Scoped to + * `folderResource` when the caller resolved one — per-task enforcement reads + * the folder that task runs in — and to the first workspace folder otherwise, + * the default the display, save, and seeding callers keep. Sharing one scoped + * access path keeps the two sides symmetric: `inspect()` can report + * `workspaceFolderValue`, and `ConfigurationTarget.WorkspaceFolder` updates + * are legal (the API contract throws for that target when the configuration + * is not scoped to a resource). With no folder open — or when a supplied + * resource resolved to nothing, see {@link commandListFolderForResource} — + * the configuration is unscoped or defaults to the first folder and reports + * only what that lookup can see. */ -export function commandListConfiguration(): vscode.WorkspaceConfiguration { - return vscode.workspace.getConfiguration(Package.name, vscode.workspace.workspaceFolders?.[0]?.uri) +export function commandListConfiguration(folderResource?: vscode.Uri): vscode.WorkspaceConfiguration { + return vscode.workspace.getConfiguration( + Package.name, + folderResource ?? vscode.workspace.workspaceFolders?.[0]?.uri, + ) +} + +/** + * Resolves the workspace folder a task's `cwd` lives in, so command-list + * enforcement scopes its configuration reads to the folder the task belongs + * to instead of the window's first folder. A child task inherits the working + * directory of the task it came from, so it resolves to the same folder + * without separate plumbing. + * + * @param cwd - The task's working directory. A blank cwd, or one that names + * no workspace root (a removed or relocated folder), yields `undefined`, + * which leaves every read on its first-folder default — the behavior such a + * cwd had before per-task scoping existed, never a guess at a different one. + * @returns The containing folder's URI, or `undefined` when cwd resolves to + * no root. + */ +export function commandListFolderForResource(cwd?: string): vscode.Uri | undefined { + if (!cwd) { + return undefined + } + + // Guarded like the configuration reads this feeds: a lookup that cannot + // run stands in for a cwd naming no root, so enforcement degrades to the + // first-folder default instead of failing the approval decision. + try { + return vscode.workspace.getWorkspaceFolder(vscode.Uri.file(cwd))?.uri + } catch { + return undefined + } } /** @@ -54,12 +89,15 @@ export function commandListConfiguration(): vscode.WorkspaceConfiguration { * configuration state. Without it the lookup runs guarded (lenient): this is * the enforcement/display path, where an unavailable workspace layer must * degrade to empty rather than fail. + * @param folderResource - Workspace folder the configuration is scoped to; + * `undefined` keeps the first-folder default. * @returns The per-layer lists and their deduplicated set union. */ export function readCommandListScopes( configKey: CommandListConfigKey, globalList: string[] | undefined, inspectionOverride?: CommandListInspection, + folderResource?: vscode.Uri, ): CommandListScopes { const global = sanitizeCommandList(globalList) @@ -71,7 +109,7 @@ export function readCommandListScopes( // of their own, so the catch covers exactly the failure it exists to // absorb. try { - inspected = commandListConfiguration().inspect(configKey) + inspected = commandListConfiguration(folderResource).inspect(configKey) } catch { inspected = undefined } diff --git a/src/core/auto-approval/index.ts b/src/core/auto-approval/index.ts index 3539eef24d..212f021f2c 100644 --- a/src/core/auto-approval/index.ts +++ b/src/core/auto-approval/index.ts @@ -15,6 +15,12 @@ import { ClineAskResponse } from "../../shared/WebviewMessage" import { isWriteToolAction, isReadOnlyToolAction } from "./tools" import { isMcpToolAlwaysAllowed } from "./mcp" import { containsDangerousSubstitution, getCommandDecisionDetailed } from "./commands" +import { + commandListFolderForResource, + readCommandListScopes, + resolveEffectiveCommandLists, + workspaceCommandListsEnabled, +} from "./commandScope" import { isFileMatchedByPatterns } from "./filePatterns" import { type AutoDenyDetail } from "./autoDenyReason" @@ -40,6 +46,8 @@ export type AutoApprovalStateOptions = | "mcpServers" // For `alwaysAllowMcp`. | "allowedCommands" // For `alwaysAllowExecute`. | "deniedCommands" + | "globalAllowedCommands" // Raw global layer feeding the per-folder command-list composition. + | "globalDeniedCommands" | "destructiveCommandGuardEnabled" | "alwaysDenyUnapprovedCommands" // For `alwaysAllowExecute` (blanket auto-deny). @@ -304,10 +312,32 @@ export async function checkAutoApproval({ return { decision: "approve" } } + // The pair on `state` was composed against the window's first + // folder; a task rooted in another folder must be judged by that + // folder's lists, so enforcement re-composes from the raw global + // layers at the folder the task's `cwd` resolves to (a cwd that + // names no workspace root keeps the first-folder configuration). + // Without the raw layers the pre-composed pair is all the state + // carries, and that pair is what enforcement consumes. + const taskFolder = commandListFolderForResource(cwd) + const effectiveLists = + state.globalAllowedCommands !== undefined && state.globalDeniedCommands !== undefined + ? resolveEffectiveCommandLists( + readCommandListScopes( + "allowedCommands", + state.globalAllowedCommands, + undefined, + taskFolder, + ), + readCommandListScopes("deniedCommands", state.globalDeniedCommands, undefined, taskFolder), + workspaceCommandListsEnabled(), + ) + : { allowed: state.allowedCommands || [], denied: state.deniedCommands || [] } + const { decision, offendingCommand, matchedPattern, parseError } = getCommandDecisionDetailed( text, - state.allowedCommands || [], - state.deniedCommands || [], + effectiveLists.allowed, + effectiveLists.denied, ) if (decision === "auto_approve") { diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 3755aa48b3..dc7f149132 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -3445,13 +3445,22 @@ export class ClineProvider autoCondenseContext: stateValues.autoCondenseContext ?? true, autoCondenseContextPercent: stateValues.autoCondenseContextPercent ?? 100, taskHistory: includeTaskHistory ? this.taskHistoryStore.getAll() : [], - // Enforce and display the effective lists that - // resolveEffectiveCommandLists composes from the global-state and - // workspace-scope configuration layers; getStateToPostToWebview shows - // exactly these values, so display cannot drift from enforcement. + // The effective lists composed from the global-state and + // workspace-scope configuration layers, scoped to the window's + // first folder; getStateToPostToWebview shows exactly these values, + // so the dialog cannot drift from what this state posts. In a + // multi-root window a task in another folder is judged by that + // folder's lists instead — enforcement re-composes the pair from + // the raw global layers below, so the displayed pair describes the + // window, not the task. allowedCommands: effectiveCommandLists.allowed, deniedCommands: effectiveCommandLists.denied, workspaceCommandListsEnabled: gateEnabled, + // The global layers before workspace composition, so command + // enforcement can re-compose the pair against each task's own + // folder; the window-scoped pair above stays what display shows. + globalAllowedCommands: allowScopes.global, + globalDeniedCommands: denyScopes.global, soundEnabled: stateValues.soundEnabled ?? false, ttsEnabled: stateValues.ttsEnabled ?? false, ttsSpeed: stateValues.ttsSpeed ?? 1.0, diff --git a/src/core/webview/__tests__/webviewMessageHandler.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index 8a7d0cebc2..725e0131a9 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -77,7 +77,7 @@ vi.mock("@roo-code/telemetry", () => ({ }, })) -import type { ModelRecord } from "@roo-code/types" +import type { ModelRecord, WebviewMessage } from "@roo-code/types" import * as vsCodeLmProvider from "../../../api/providers/vscode-lm" import * as copilotProvider from "../../../api/providers/github-copilot" @@ -776,9 +776,16 @@ describe("webviewMessageHandler - theme fixture probes", () => { import { t } from "../../../i18n" +// Mutable so a test can script a multi-root workspace; every other test sees the +// single folder the command-list configuration resolves against by default. +const mockWorkspaceFolders = vi.hoisted(() => ({ + folders: [{ uri: { fsPath: "/mock/workspace" } }] as { uri: { fsPath: string } }[], +})) + vi.mock("vscode", () => { const showInformationMessage = vi.fn() const showErrorMessage = vi.fn() + const showWarningMessage = vi.fn() const openTextDocument = vi.fn().mockResolvedValue({}) const showTextDocument = vi.fn().mockResolvedValue(undefined) @@ -791,10 +798,13 @@ vi.mock("vscode", () => { window: { showInformationMessage, showErrorMessage, + showWarningMessage, showTextDocument, }, workspace: { - workspaceFolders: [{ uri: { fsPath: "/mock/workspace" } }], + get workspaceFolders() { + return mockWorkspaceFolders.folders + }, openTextDocument, getConfiguration: vi.fn(() => ({ get: vi.fn() })), }, @@ -2104,6 +2114,7 @@ describe("webviewMessageHandler - allowlists together", () => { describe("webviewMessageHandler - command list scope-aware persistence", () => { beforeEach(() => { vi.clearAllMocks() + mockWorkspaceFolders.folders = [{ uri: { fsPath: "/mock/workspace" } }] }) // The file-level vscode mock opens one workspace folder, so the command @@ -2521,6 +2532,298 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { // instead of keeping the refused one. expect(mockClineProvider.postStateToWebview).toHaveBeenCalled() }) + + // A payload can carry both command-list keys; the second-processed key's + // hidden-entry check must judge shadowing against the state as it was when + // the save started, not as the same loop's earlier key left it. The double + // below mirrors ContextProxy: setValue publishes the in-memory copy before + // resolving, so a fresh read after the first key's awaited write would + // observe it and let the never-displayed entry be deleted. + function mockCommandListStateStore(initial: Partial>) { + const store: Partial> = { ...initial } + vi.mocked(mockClineProvider.contextProxy.getValue).mockImplementation((key: string) => + key === "allowedCommands" || key === "deniedCommands" ? store[key] : undefined, + ) + vi.mocked(mockClineProvider.contextProxy.setValue).mockImplementation(async (key: string, value: unknown) => { + if (key === "allowedCommands" || key === "deniedCommands") { + store[key] = value as string[] | undefined + } + }) + return store + } + + it("preserves a workspace deny entry shadowed by a global allow the same save removes", async () => { + mockCommandListStateStore({ allowedCommands: ["git"] }) + const { update } = mockCommandConfiguration( + (key) => (key === "deniedCommands" ? { workspaceValue: ["git push"] } : {}), + undefined, + undefined, + true, + ) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + // "allowedCommands" is processed first and drops the shadowing + // global "git"; "git push" was hidden from the dialog, so an empty + // deny list must not delete it. + updatedSettings: { allowedCommands: [], deniedCommands: [] }, + }) + + expect(update).toHaveBeenCalledWith("deniedCommands", ["git push"], vscode.ConfigurationTarget.Workspace) + expect(update).not.toHaveBeenCalledWith("deniedCommands", [], vscode.ConfigurationTarget.Workspace) + expect(mockClineProvider.postStateToWebview).toHaveBeenCalled() + }) + + it("preserves a workspace allow entry shadowed by a global deny the same save removes", async () => { + mockCommandListStateStore({ deniedCommands: ["git"] }) + const { update } = mockCommandConfiguration( + (key) => (key === "allowedCommands" ? { workspaceValue: ["git push"] } : {}), + undefined, + undefined, + true, + ) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + // The mirror order: the deny key is processed first, so this pins + // that preservation does not depend on which key arrives first. + updatedSettings: { deniedCommands: [], allowedCommands: [] }, + }) + + expect(update).toHaveBeenCalledWith("allowedCommands", ["git push"], vscode.ConfigurationTarget.Workspace) + expect(update).not.toHaveBeenCalledWith("allowedCommands", [], vscode.ConfigurationTarget.Workspace) + expect(mockClineProvider.postStateToWebview).toHaveBeenCalled() + }) + + it("reports a rejected extension-state write without aborting the save", async () => { + // The configuration writes succeeded, so silently dropping the state + // write would leave the settings files and the stored list disagreeing + // with nothing shown to the user. The rejection is queued (once) so it + // consumes exactly this key's write and the sibling key still persists. + vi.mocked(mockClineProvider.contextProxy.setValue).mockRejectedValueOnce(new Error("state store unavailable")) + const { update } = mockCommandConfiguration({}) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: ["aws"], alwaysDenyUnapprovedCommands: true }, + }) + + // The configuration leg did persist... + expect(update).toHaveBeenCalledWith("deniedCommands", ["aws"], vscode.ConfigurationTarget.Global) + // ...so the message names the leg that failed... + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith( + "Failed to save deniedCommands: state store unavailable. The configuration was updated, but the extension's stored copy of the list was not, so the two disagree until a later save succeeds.", + ) + // ...the save continued to the next key... + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("alwaysDenyUnapprovedCommands", true) + // ...and the dialog was re-synced instead of keeping an unsaved value. + expect(mockClineProvider.postStateToWebview).toHaveBeenCalled() + }) + + it("still propagates a rejected state write for non-command-list keys", async () => { + // The partial-failure report is scoped to the command-list keys, whose + // configuration legs succeeded first; other keys have no half-completed + // save to explain, so their failures keep propagating as before. The + // rejection is queued (once) so it stays scoped to this test. + vi.mocked(mockClineProvider.contextProxy.setValue).mockRejectedValueOnce(new Error("state store unavailable")) + + await expect( + webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { alwaysDenyUnapprovedCommands: true }, + }), + ).rejects.toThrow("state store unavailable") + + expect(vscode.window.showErrorMessage).not.toHaveBeenCalled() + expect(mockClineProvider.postStateToWebview).not.toHaveBeenCalled() + }) + + it("keeps a settings-file entry the dialog never displayed and warns that it was preserved", async () => { + // An entry the settings file gained while the dialog was open has the same + // shape as a user deletion (in the file, absent from the submitted list); + // the baseline tells them apart, so the new entry is kept in the layer the + // snapshot found it in and the user is told instead of losing it silently. + vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(undefined) + const { update } = mockCommandConfiguration({ workspaceValue: ["aws", "rm -rf"] }, undefined, undefined, true) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: ["aws"] }, + commandListBaseline: { deniedCommands: ["aws"] }, + }) + + expect(update).toHaveBeenCalledWith("deniedCommands", ["aws", "rm -rf"], vscode.ConfigurationTarget.Workspace) + expect(update).toHaveBeenCalledWith("deniedCommands", [], vscode.ConfigurationTarget.Global) + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("deniedCommands", []) + expect(vscode.window.showWarningMessage).toHaveBeenCalledTimes(1) + expect(vscode.window.showWarningMessage).toHaveBeenCalledWith(expect.stringContaining("rm -rf")) + expect(vscode.window.showWarningMessage).toHaveBeenCalledWith(expect.stringContaining("1 entry was")) + }) + + it("applies a user deletion of a displayed entry without warning", async () => { + // The counterpart classification: an entry the baseline shows the dialog + // displayed and the submitted list omits is a deliberate deletion and must + // be dropped from every layer that held it, with nothing disclosed. + vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(undefined) + const { update } = mockCommandConfiguration({ workspaceValue: ["aws", "rm -rf"] }, undefined, undefined, true) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: ["aws"] }, + commandListBaseline: { deniedCommands: ["aws", "rm -rf"] }, + }) + + expect(update).toHaveBeenCalledWith("deniedCommands", ["aws"], vscode.ConfigurationTarget.Workspace) + expect(update).toHaveBeenCalledWith("deniedCommands", [], vscode.ConfigurationTarget.Global) + expect(update).not.toHaveBeenCalledWith("deniedCommands", ["rm -rf"], vscode.ConfigurationTarget.Global) + expect(vscode.window.showWarningMessage).not.toHaveBeenCalled() + }) + + it("keeps a file-added entry while applying a displayed deletion in one save, warning once", async () => { + // Both divergence classes in one payload: "git" was displayed and the user + // deleted it (deletion applies), "curl" arrived after the dialog opened + // (preserved). The warning counts only the preserved entry, once per save. + vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(undefined) + const { update } = mockCommandConfiguration({ workspaceValue: ["git", "curl"] }, undefined, undefined, true) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: [] }, + commandListBaseline: { deniedCommands: ["git"] }, + }) + + expect(update).toHaveBeenCalledWith("deniedCommands", ["curl"], vscode.ConfigurationTarget.Workspace) + expect(update).toHaveBeenCalledWith("deniedCommands", [], vscode.ConfigurationTarget.Global) + expect(vscode.window.showWarningMessage).toHaveBeenCalledTimes(1) + expect(vscode.window.showWarningMessage).toHaveBeenCalledWith(expect.stringContaining("curl")) + expect(vscode.window.showWarningMessage).not.toHaveBeenCalledWith(expect.stringContaining("git")) + }) + + it("saves command lists unchanged when the message carries no baseline", async () => { + // Backward-compatibility witness: senders that omit the baseline keep the + // exact pre-existing behavior - the omitted entry is dropped as a deletion + // and no warning is produced. + vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(undefined) + const { update } = mockCommandConfiguration({ workspaceValue: ["aws", "rm -rf"] }, undefined, undefined, true) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: ["aws"] }, + }) + + expect(update).toHaveBeenCalledWith("deniedCommands", ["aws"], vscode.ConfigurationTarget.Workspace) + expect(update).toHaveBeenCalledWith("deniedCommands", [], vscode.ConfigurationTarget.Global) + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("deniedCommands", []) + expect(vscode.window.showWarningMessage).not.toHaveBeenCalled() + }) + + it("routes a preserved entry to the layer the save-start snapshot found it in", async () => { + // A preserved entry must land where it lives, not be flattened into the + // global layer: "rm -rf" is in the workspace snapshot, "npm" in the + // extension's stored global layer, and neither was ever displayed. + vi.mocked(mockClineProvider.contextProxy.getValue).mockImplementation((key: string) => + key === "deniedCommands" ? ["npm"] : undefined, + ) + const { update } = mockCommandConfiguration({ workspaceValue: ["rm -rf"] }, undefined, undefined, true) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: [] }, + commandListBaseline: { deniedCommands: [] }, + }) + + expect(update).toHaveBeenCalledWith("deniedCommands", ["rm -rf"], vscode.ConfigurationTarget.Workspace) + expect(update).toHaveBeenCalledWith("deniedCommands", ["npm"], vscode.ConfigurationTarget.Global) + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("deniedCommands", ["npm"]) + expect(vscode.window.showWarningMessage).toHaveBeenCalledTimes(1) + expect(vscode.window.showWarningMessage).toHaveBeenCalledWith(expect.stringContaining("2 entries were")) + }) + + it("treats a malformed command-list baseline exactly like a missing one", async () => { + // The save message crosses a JSON boundary, so a baseline whose key holds a + // non-array is reachable at runtime even though the declared type forbids it. + // The classification guard treats it as no baseline at all: the submitted list + // is written unclassified and no entry the dialog never displayed is claimed + // as user-deleted, matching the no-baseline witness exactly. + const wireMessage: WebviewMessage = JSON.parse( + JSON.stringify({ + type: "updateSettings", + updatedSettings: { deniedCommands: ["aws"] }, + commandListBaseline: { deniedCommands: "aws" }, + }), + ) + vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(undefined) + const { update } = mockCommandConfiguration({ workspaceValue: ["aws", "rm -rf"] }, undefined, undefined, true) + + await webviewMessageHandler(mockClineProvider, wireMessage) + + expect(update).toHaveBeenCalledWith("deniedCommands", ["aws"], vscode.ConfigurationTarget.Workspace) + expect(update).toHaveBeenCalledWith("deniedCommands", [], vscode.ConfigurationTarget.Global) + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("deniedCommands", []) + expect(vscode.window.showWarningMessage).not.toHaveBeenCalled() + }) + + it("composes and writes only the first workspace folder's layer in a multi-root save", async () => { + // The dialog composes against the first folder, so a save must never read or + // write the second folder's per-folder layer: folder B's entries survive a + // folder A save untouched. + const secondFolderUri = { fsPath: "/mock/second-folder" } + mockWorkspaceFolders.folders = [{ uri: mockWorkspaceFolderUri }, { uri: secondFolderUri }] + const { update } = mockCommandConfiguration( + { workspaceValue: ["npm"], workspaceFolderValue: ["docker"] }, + undefined, + undefined, + true, + ) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: ["docker", "aws"] }, + }) + + expect(vscode.workspace.getConfiguration).toHaveBeenCalledWith(Package.name, mockWorkspaceFolderUri) + expect(vscode.workspace.getConfiguration).not.toHaveBeenCalledWith(Package.name, secondFolderUri) + // The folder-target write the save does make goes to the first folder's layer. + expect(update).toHaveBeenCalledWith("deniedCommands", ["docker"], vscode.ConfigurationTarget.WorkspaceFolder) + expect(update).toHaveBeenCalledWith("deniedCommands", ["aws"], vscode.ConfigurationTarget.Global) + }) + + it("aggregates preserved entries from both command-list keys into a single save warning", async () => { + // A payload carrying both list keys classifies per key, so two file entries the + // dialog never displayed - one per key - must survive in their own layers and be + // disclosed together in exactly one warning for the save. + vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(undefined) + const { update } = mockCommandConfiguration( + (key) => + key === "allowedCommands" + ? { workspaceValue: ["git", "curl"] } + : { workspaceValue: ["rm -rf", "docker"] }, + undefined, + undefined, + true, + ) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { allowedCommands: ["git"], deniedCommands: ["rm -rf"] }, + commandListBaseline: { allowedCommands: ["git"], deniedCommands: ["rm -rf"] }, + }) + + // "curl" stays in the workspace allow layer, "docker" in the workspace deny layer. + expect(update).toHaveBeenCalledWith("allowedCommands", ["git", "curl"], vscode.ConfigurationTarget.Workspace) + expect(update).toHaveBeenCalledWith( + "deniedCommands", + ["rm -rf", "docker"], + vscode.ConfigurationTarget.Workspace, + ) + expect(update).toHaveBeenCalledWith("allowedCommands", [], vscode.ConfigurationTarget.Global) + expect(update).toHaveBeenCalledWith("deniedCommands", [], vscode.ConfigurationTarget.Global) + // One aggregated warning naming both preserved entries and counting them together. + expect(vscode.window.showWarningMessage).toHaveBeenCalledTimes(1) + expect(vscode.window.showWarningMessage).toHaveBeenCalledWith(expect.stringContaining("2 entries were")) + expect(vscode.window.showWarningMessage).toHaveBeenCalledWith(expect.stringContaining("curl")) + expect(vscode.window.showWarningMessage).toHaveBeenCalledWith(expect.stringContaining("docker")) + }) }) describe("webviewMessageHandler - terminalProfile", () => { diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index dfb7e2e8a0..97ec1f0eb9 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -770,6 +770,17 @@ export const webviewMessageHandler = async ( } } + // Shadowing is cross-directional, and a payload can carry both + // command-list keys, so the second-processed key's hidden-entry + // check would otherwise read the layer the first key's iteration + // just wrote. Both global layers are captured before any write so + // a hidden entry's preservation cannot depend on payload key order. + const commandListGlobalsAtSaveStart = { + allowedCommands: getGlobalState("allowedCommands"), + deniedCommands: getGlobalState("deniedCommands"), + } + const externallyAddedCommandEntries: string[] = [] + for (const [key, value] of Object.entries(message.updatedSettings)) { let newValue = value @@ -823,20 +834,34 @@ export const webviewMessageHandler = async ( } const currentScopes = readCommandListScopes(key, getGlobalState(key), inspection) + // An entry the settings file gained while the dialog was open has + // the same shape as an entry the user deleted: present in the file, + // absent from the submitted list. Only the dialog's last-displayed + // baseline separates the two, so file entries outside that baseline + // are re-admitted before the split (deletions of baseline entries + // still apply). A sender that shows no lists sends no baseline, and + // its submitted list is written unclassified. + const baseline = message.commandListBaseline?.[key] + const externallyAdded = Array.isArray(baseline) + ? [...new Set([...currentScopes.workspace, ...currentScopes.global])].filter( + (entry) => !baseline.includes(entry) && !incoming.includes(entry), + ) + : [] const { globalWrite, workspaceWrite: keptWorkspaceWrite } = splitCommandListScopes( - incoming, + [...incoming, ...externallyAdded], currentScopes.workspace, currentScopes.global, ) - // Shadowing is cross-directional, so the hidden-entry - // set needs the opposite key's global layer as input. - // That opposite-key read is a fresh guarded read: it - // only feeds the additive preservation below and never - // this key's split, target, or rollback, which stay on - // the single snapshot. + // Shadowing is cross-directional, so the hidden-entry set + // needs the opposite key's global layer as input. It comes + // from the save-start snapshot, not a fresh read: an + // earlier iteration of this loop may already have written + // that key. The layer only feeds the additive preservation + // below and never this key's split, target, or rollback, + // which stay on the per-key configuration snapshot. const otherKey = key === "allowedCommands" ? "deniedCommands" : "allowedCommands" - const otherScopes = readCommandListScopes(otherKey, getGlobalState(otherKey)) + const otherScopes = readCommandListScopes(otherKey, commandListGlobalsAtSaveStart[otherKey]) const { hiddenWorkspaceAllow, hiddenWorkspaceDeny } = resolveEffectiveCommandLists( key === "allowedCommands" ? currentScopes : otherScopes, key === "deniedCommands" ? currentScopes : otherScopes, @@ -910,6 +935,10 @@ export const webviewMessageHandler = async ( // would double-home workspace entries and re-leak them on the // next save. newValue = globalWrite + // Accumulated only after both configuration writes succeeded: the + // disclosure must not claim preservation for a key whose save was + // abandoned by a failed write. + externallyAddedCommandEntries.push(...externallyAdded) } else if (key === "allowedReadFiles" || key === "allowedWriteFiles") { const patterns = value ?? [] @@ -994,7 +1023,32 @@ export const webviewMessageHandler = async ( } } - await provider.contextProxy.setValue(key as keyof RooCodeSettings, newValue) + if (key === "allowedCommands" || key === "deniedCommands") { + // The configuration writes above already succeeded, so a + // rejected state write leaves the configuration and the + // extension's stored list disagreeing until a later save + // overwrites it. Letting the rejection escape would also + // skip the state post that re-syncs the dialog, so the key + // reports which leg failed and the save continues. + try { + await provider.contextProxy.setValue(key as keyof RooCodeSettings, newValue) + } catch (error) { + const failure = error instanceof Error ? error.message : String(error) + vscode.window.showErrorMessage( + `Failed to save ${key}: ${failure}. The configuration was updated, but the extension's stored copy of the list was not, so the two disagree until a later save succeeds.`, + ) + continue + } + } else { + await provider.contextProxy.setValue(key as keyof RooCodeSettings, newValue) + } + } + + if (externallyAddedCommandEntries.length > 0) { + const count = externallyAddedCommandEntries.length + vscode.window.showWarningMessage( + `Your settings file changed while the settings dialog was open. ${count} ${count === 1 ? "entry was" : "entries were"} added there during this session and kept by this save: ${externallyAddedCommandEntries.join(", ")}. Reopen the settings dialog to see the current command lists.`, + ) } await provider.postStateToWebview() diff --git a/webview-ui/src/components/settings/SettingsView.tsx b/webview-ui/src/components/settings/SettingsView.tsx index 5f1769a230..492f66512b 100644 --- a/webview-ui/src/components/settings/SettingsView.tsx +++ b/webview-ui/src/components/settings/SettingsView.tsx @@ -95,6 +95,25 @@ export const settingsTabTrigger = "whitespace-nowrap overflow-hidden min-w-0 h-12 px-4 py-3 box-border flex items-center border-l-2 border-transparent text-vscode-foreground opacity-70 hover:bg-vscode-list-hoverBackground data-[compact=true]:w-12 data-[compact=true]:p-4" export const settingsTabTriggerActive = "opacity-100 border-vscode-focusBorder bg-vscode-list-activeSelectionBackground" +// Command-list snapshot of a settings state object. Entries are compared by value because every +// state post delivers freshly deserialized arrays; reference identity carries no meaning here. +interface CommandListSnapshot { + allowedCommands?: string[] + deniedCommands?: string[] + workspaceCommandListsEnabled?: boolean +} + +const commandListSnapshot = (state: CommandListSnapshot): string => + JSON.stringify([ + state.allowedCommands ?? [], + state.deniedCommands ?? [], + state.workspaceCommandListsEnabled ?? false, + ]) + +const commandListsEquivalent = (left?: string[], right?: string[]) => + left === right || + (!!left && !!right && left.length === right.length && left.every((entry, index) => entry === right[index])) + export interface SettingsViewRef { checkUnsaveChanges: (then: () => void) => void } @@ -153,6 +172,13 @@ const SettingsView = forwardRef(({ onDone, t const [cachedState, setCachedState] = useState(() => extensionState) + // Command-list values the dialog last displayed, so a later state post can be recognized as + // an external change rather than a restatement of what is already shown. + const lastDisplayedCommandSnapshot = useRef(commandListSnapshot(extensionState)) + // An in-flight user edit of either list claims ownership of the lists until Save, discard, or + // a full reseed; external list updates must not silently overwrite it. + const commandListsTouched = useRef(false) + const { alwaysAllowReadOnly, alwaysAllowReadOnlyOutsideWorkspace, @@ -235,6 +261,8 @@ const SettingsView = forwardRef(({ onDone, t setCachedState((prevCachedState) => ({ ...prevCachedState, ...extensionState })) prevApiConfigName.current = currentApiConfigName prevMode.current = mode + lastDisplayedCommandSnapshot.current = commandListSnapshot(extensionState) + commandListsTouched.current = false setChangeDetected(false) }, [currentApiConfigName, mode, extensionState]) @@ -246,15 +274,79 @@ const SettingsView = forwardRef(({ onDone, t handledSettingsImportedAt.current = settingsImportedAt setCachedState((prevCachedState) => ({ ...prevCachedState, ...extensionState })) + lastDisplayedCommandSnapshot.current = commandListSnapshot(extensionState) + commandListsTouched.current = false setChangeDetected(false) }, [settingsImportedAt, extensionState]) + // The command lists can change in the extension host while this dialog is open (for example, + // a settings-file edit landing mid-session). Fold exactly those fields into the edit buffer on + // every state post that changes them, so a later Save cannot rewrite the configuration layer + // from a list that never showed the new entry. List fields are held back while the user has an + // in-flight list edit (in-flight edits win); every other buffered field is left untouched and + // the dirty flag is never cleared here, keeping the buffer's Save-gated semantics. + useEffect(() => { + const snapshot = commandListSnapshot(extensionState) + if (snapshot === lastDisplayedCommandSnapshot.current) { + return + } + + setCachedState((prevState) => { + const merged: CommandListSnapshot = {} + let changedFields = 0 + + if (!commandListsTouched.current) { + if (!commandListsEquivalent(prevState.allowedCommands, extensionState.allowedCommands)) { + merged.allowedCommands = extensionState.allowedCommands + changedFields++ + } + if (!commandListsEquivalent(prevState.deniedCommands, extensionState.deniedCommands)) { + merged.deniedCommands = extensionState.deniedCommands + changedFields++ + } + } + if (prevState.workspaceCommandListsEnabled !== extensionState.workspaceCommandListsEnabled) { + merged.workspaceCommandListsEnabled = extensionState.workspaceCommandListsEnabled + changedFields++ + } + + // The stamp records the lists this merge leaves on screen, never the posted + // state outright: an entry held back by an in-flight edit was never displayed, + // and a baseline claiming it had been lets the save erase it as a user + // deletion without warning. While the lists are held back the buffered lists + // are themselves unposted, so the stamp keeps its prior lists instead - a + // deletion inside the buffered edit must stay in the baseline for the save to + // apply it rather than resurrect the deleted entry as an external addition. + const [stampAllowed, stampDenied] = commandListsTouched.current + ? (JSON.parse(lastDisplayedCommandSnapshot.current) as [string[], string[], boolean]) + : [ + merged.allowedCommands ?? prevState.allowedCommands, + merged.deniedCommands ?? prevState.deniedCommands, + ] + lastDisplayedCommandSnapshot.current = commandListSnapshot({ + allowedCommands: stampAllowed, + deniedCommands: stampDenied, + workspaceCommandListsEnabled: + merged.workspaceCommandListsEnabled ?? prevState.workspaceCommandListsEnabled, + }) + + if (changedFields === 0) { + return prevState + } + return { ...prevState, ...merged } + }) + }, [extensionState]) + const setCachedStateField: SetCachedStateField = useCallback((field, value) => { setCachedState((prevState) => { if (prevState[field] === value) { return prevState } + if (field === "allowedCommands" || field === "deniedCommands") { + commandListsTouched.current = true + } + setChangeDetected(true) return { ...prevState, [field]: value } }) @@ -382,8 +474,17 @@ const SettingsView = forwardRef(({ onDone, t const handleSubmit = () => { if (isSettingValid) { + const [lastDisplayedAllowed, lastDisplayedDenied] = JSON.parse(lastDisplayedCommandSnapshot.current) as [ + string[], + string[], + boolean, + ] vscode.postMessage({ type: "updateSettings", + commandListBaseline: { + allowedCommands: lastDisplayedAllowed, + deniedCommands: lastDisplayedDenied, + }, updatedSettings: { language, alwaysAllowReadOnly: alwaysAllowReadOnly ?? undefined, @@ -465,6 +566,13 @@ const SettingsView = forwardRef(({ onDone, t vscode.postMessage({ type: "telemetrySetting", text: telemetrySetting }) vscode.postMessage({ type: "debugSetting", bool: cachedState.debug }) + // Saving commits the buffered lists, so they no longer hold off external updates. + commandListsTouched.current = false + lastDisplayedCommandSnapshot.current = commandListSnapshot({ + allowedCommands, + deniedCommands, + workspaceCommandListsEnabled: cachedState.workspaceCommandListsEnabled, + }) setChangeDetected(false) } } @@ -488,6 +596,8 @@ const SettingsView = forwardRef(({ onDone, t if (confirm) { // Discard changes: Reset state and flag setCachedState(extensionState) // Revert to original state + lastDisplayedCommandSnapshot.current = commandListSnapshot(extensionState) + commandListsTouched.current = false // Buffered edits are gone, so live lists are not held off setChangeDetected(false) // Reset change flag confirmDialogHandler.current?.() // Execute the pending action (e.g., tab switch) } diff --git a/webview-ui/src/components/settings/__tests__/SettingsView.change-detection.spec.tsx b/webview-ui/src/components/settings/__tests__/SettingsView.change-detection.spec.tsx index 6704aa1cf0..f8e4b95737 100644 --- a/webview-ui/src/components/settings/__tests__/SettingsView.change-detection.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/SettingsView.change-detection.spec.tsx @@ -472,7 +472,7 @@ describe("SettingsView - Change Detection Fix", () => { it("should not show unsaved changes when no changes are made", async () => { const onDone = vi.fn() - ;(useExtensionState as any).mockReturnValue(createExtensionState()) + ;(useExtensionState as ReturnType).mockReturnValue(createExtensionState()) renderWithExtensionState(, { queryClient }) @@ -497,7 +497,7 @@ describe("SettingsView - Change Detection Fix", () => { }, 10000) it("persists a normalized reasoning default through Save", async () => { - ;(useExtensionState as any).mockReturnValue(createExtensionState()) + ;(useExtensionState as ReturnType).mockReturnValue(createExtensionState()) renderWithExtensionState(, { queryClient }) await waitFor(() => expect(screen.getByTestId("save-button")).toBeDisabled()) @@ -544,7 +544,7 @@ describe("SettingsView - Change Detection Fix", () => { }, }) - ;(useExtensionState as any).mockImplementation(() => extensionState) + ;(useExtensionState as ReturnType).mockImplementation(() => extensionState) const { rerender } = renderWithExtensionState(, { queryClient }) @@ -580,7 +580,7 @@ describe("SettingsView - Change Detection Fix", () => { basetenApiKey: "test-baseten-key", }, }) - ;(useExtensionState as any).mockImplementation(() => extensionState) + ;(useExtensionState as ReturnType).mockImplementation(() => extensionState) rerender() }) @@ -615,7 +615,7 @@ describe("SettingsView - Change Detection Fix", () => { }, }) - ;(useExtensionState as any).mockImplementation(() => extensionState) + ;(useExtensionState as ReturnType).mockImplementation(() => extensionState) const { rerender } = renderWithExtensionState(, { queryClient }) @@ -635,7 +635,7 @@ describe("SettingsView - Change Detection Fix", () => { basetenApiKey: "imported-baseten-key", }, }) - ;(useExtensionState as any).mockImplementation(() => extensionState) + ;(useExtensionState as ReturnType).mockImplementation(() => extensionState) rerender() }) @@ -660,7 +660,7 @@ describe("SettingsView - Change Detection Fix", () => { }, }) - ;(useExtensionState as any).mockImplementation(() => extensionState) + ;(useExtensionState as ReturnType).mockImplementation(() => extensionState) const { rerender } = renderWithExtensionState(, { queryClient }) @@ -720,7 +720,7 @@ describe("SettingsView - Change Detection Fix", () => { apiModelId: "friendli-model", }, }) - ;(useExtensionState as any).mockImplementation(() => extensionState) + ;(useExtensionState as ReturnType).mockImplementation(() => extensionState) rerender() }) @@ -743,7 +743,7 @@ describe("SettingsView - Change Detection Fix", () => { }, }) - ;(useExtensionState as any).mockImplementation(() => extensionState) + ;(useExtensionState as ReturnType).mockImplementation(() => extensionState) const { rerender } = renderWithExtensionState(, { queryClient }) @@ -766,7 +766,7 @@ describe("SettingsView - Change Detection Fix", () => { apiModelId: "gpt-4.1", }, }) - ;(useExtensionState as any).mockImplementation(() => extensionState) + ;(useExtensionState as ReturnType).mockImplementation(() => extensionState) rerender() }) @@ -775,4 +775,109 @@ describe("SettingsView - Change Detection Fix", () => { expect(screen.getByTestId("provider-value")).toHaveTextContent("baseten") }, 20000) }) + + describe("command-list refresh while the dialog is open", () => { + it("merges an externally added deny entry into the buffer before a later save", async () => { + let extensionState = createExtensionState({ deniedCommands: [] }) + ;(useExtensionState as ReturnType).mockImplementation(() => extensionState) + + const { rerender } = renderWithExtensionState(, { queryClient }) + await waitFor(() => expect(screen.getByTestId("save-button")).toBeDisabled()) + + // Buffer an unrelated edit so a Save happens later in this dialog session. + fireEvent.click(screen.getByTestId("set-provider-baseten")) + await waitFor(() => expect(screen.getByTestId("save-button")).toBeEnabled()) + + // A deny entry lands externally while the dialog is open. + await act(async () => { + extensionState = createExtensionState({ deniedCommands: ["rm -rf"] }) + ;(useExtensionState as ReturnType).mockImplementation(() => extensionState) + rerender() + }) + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)) + }) + + // Save must submit the list that includes the entry the dialog now displays, not the + // stale one captured when the dialog opened. + fireEvent.click(screen.getByTestId("save-button")) + expect(mockPostMessage).toHaveBeenCalledWith( + expect.objectContaining({ + type: "updateSettings", + updatedSettings: expect.objectContaining({ deniedCommands: ["rm -rf"] }), + }), + ) + }, 10000) + + it("baselines the save on the post-merge lists, not the lists shown when the dialog opened", async () => { + let extensionState = createExtensionState({ deniedCommands: [] }) + ;(useExtensionState as ReturnType).mockImplementation(() => extensionState) + + const { rerender } = renderWithExtensionState(, { queryClient }) + await waitFor(() => expect(screen.getByTestId("save-button")).toBeDisabled()) + + // Buffer an unrelated edit so a Save happens later in this dialog session. + fireEvent.click(screen.getByTestId("set-provider-baseten")) + await waitFor(() => expect(screen.getByTestId("save-button")).toBeEnabled()) + + // A deny entry lands externally and the merge puts it into the displayed lists. + await act(async () => { + extensionState = createExtensionState({ deniedCommands: ["rm -rf"] }) + ;(useExtensionState as ReturnType).mockImplementation(() => extensionState) + rerender() + }) + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)) + }) + + // The baseline must be the list the dialog NOW displays (post-merge), so the + // merge's entry counts as shown - not the empty list present when the dialog + // opened, which would make the merge's own entry look externally added. + fireEvent.click(screen.getByTestId("save-button")) + expect(mockPostMessage).toHaveBeenCalledWith( + expect.objectContaining({ + type: "updateSettings", + commandListBaseline: { allowedCommands: [], deniedCommands: ["rm -rf"] }, + }), + ) + }, 10000) + + it("merges without clearing unsaved changes when the command-list state flips externally", async () => { + let extensionState = createExtensionState() + ;(useExtensionState as ReturnType).mockImplementation(() => extensionState) + + const { rerender } = renderWithExtensionState(, { queryClient }) + await waitFor(() => expect(screen.getByTestId("save-button")).toBeDisabled()) + + // Buffer an edit that touches neither command list, so the dialog is dirty. + fireEvent.click(screen.getByTestId("set-provider-baseten")) + await waitFor(() => expect(screen.getByTestId("save-button")).toBeEnabled()) + + // One state post flips the workspace command-list gate and adds a deny entry, which is + // what enabling the gate does: workspace entries become part of the displayed lists. + await act(async () => { + extensionState = createExtensionState({ + deniedCommands: ["git push"], + workspaceCommandListsEnabled: true, + }) + ;(useExtensionState as ReturnType).mockImplementation(() => extensionState) + rerender() + }) + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)) + }) + + // The merge must not clear the dirty flag or discard the buffered provider edit... + expect(screen.getByTestId("provider-value")).toHaveTextContent("baseten") + await waitFor(() => expect(screen.getByTestId("save-button")).toBeEnabled()) + // ...while the flip's list change still reaches the buffer and the next Save. + fireEvent.click(screen.getByTestId("save-button")) + expect(mockPostMessage).toHaveBeenCalledWith( + expect.objectContaining({ + type: "updateSettings", + updatedSettings: expect.objectContaining({ deniedCommands: ["git push"] }), + }), + ) + }, 10000) + }) }) diff --git a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx index 0835f255f9..ae81e16686 100644 --- a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx @@ -691,6 +691,32 @@ describe("SettingsView - Allowed Commands", () => { ) }) + it("sends the lists displayed at open as the save baseline, unmoved by an in-flight list edit", () => { + // The save path classifies divergence between "user deleted this" and "the + // settings file gained this" from the baseline, so the payload must carry what + // the dialog displayed BEFORE this save - not the edited list it is submitting, + // and not the committed value the post-save bookkeeping writes back. + renderWithExtensionState(, { + state: { allowedCommands: ["git"], alwaysAllowExecute: true }, + }) + + const content = screen.getByTestId("settings-content") + // An in-flight addition lands in the edit buffer only. + const input = within(content).getByTestId("command-input") + fireEvent.change(input, { target: { value: "npm test" } }) + fireEvent.click(within(content).getByTestId("add-command-button")) + + fireEvent.click(screen.getByTestId("save-button")) + + expect(vscode.postMessage).toHaveBeenCalledWith( + expect.objectContaining({ + type: "updateSettings", + commandListBaseline: { allowedCommands: ["git"], deniedCommands: [] }, + updatedSettings: expect.objectContaining({ allowedCommands: ["git", "npm test"] }), + }), + ) + }) + it("removes command from the list", () => { // Render once and get the activateTab helper const { activateTab, getSettingsContent } = renderSettingsView() diff --git a/webview-ui/src/components/settings/__tests__/SettingsView.unsaved-changes.spec.tsx b/webview-ui/src/components/settings/__tests__/SettingsView.unsaved-changes.spec.tsx index 1bc4ab1afb..04e0c7e18a 100644 --- a/webview-ui/src/components/settings/__tests__/SettingsView.unsaved-changes.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/SettingsView.unsaved-changes.spec.tsx @@ -249,6 +249,7 @@ vi.mock("../SettingsSearch", () => ({ import { useExtensionState } from "@src/context/ExtensionStateContext" import ApiOptions from "../ApiOptions" +import { AutoApproveSettings } from "../AutoApproveSettings" describe("SettingsView - Unsaved Changes Detection", () => { let queryClient: QueryClient @@ -811,6 +812,137 @@ describe("SettingsView - Unsaved Changes Detection", () => { }) }) + it("keeps an in-flight deny-list edit when an external list update arrives while the dialog is open", async () => { + // Drive the real cached-state setter through a stand-in auto-approve section so the + // buffered list edit is indistinguishable from a user typing in the deny list. + vi.mocked(AutoApproveSettings).mockImplementation( + ({ setCachedStateField }: React.ComponentProps) => ( + + ), + ) + const view = renderWithExtensionState(, { + queryClient, + }) + + fireEvent.click(await screen.findByTestId("buffer-deny-edit")) + await waitFor(() => expect(screen.getByTestId("save-button")).toBeEnabled()) + + // The deny list changes in the extension host while the dialog is open. + ;(useExtensionState as ReturnType).mockReturnValue({ + ...defaultExtensionState, + deniedCommands: ["rm -rf"], + }) + view.rerender() + + // The user's unsaved list edit wins over the external update, and Save submits it. + fireEvent.click(screen.getByTestId("save-button")) + expect(postMessage).toHaveBeenCalledWith( + expect.objectContaining({ + type: "updateSettings", + updatedSettings: expect.objectContaining({ deniedCommands: ["git status"] }), + }), + ) + expect(postMessage).not.toHaveBeenCalledWith( + expect.objectContaining({ + type: "updateSettings", + updatedSettings: expect.objectContaining({ deniedCommands: ["rm -rf"] }), + }), + ) + }) + + it("baselines the save on the pre-edit displayed lists when a state post lands during an in-flight list edit", async () => { + // The baseline feeds the save's classification of every entry the submitted + // list omits, so it must hold the lists the dialog displayed BEFORE the + // in-flight edit. Folding the posted state in would claim the never-displayed + // entry had been shown, letting the save erase it as a user deletion; folding + // the edited buffer in would drop an in-flight deletion from the baseline, + // letting the save resurrect it as an external addition. + vi.mocked(AutoApproveSettings).mockImplementation( + ({ setCachedStateField }: React.ComponentProps) => ( + + ), + ) + const view = renderWithExtensionState(, { + queryClient, + }) + + fireEvent.click(await screen.findByTestId("buffer-deny-edit")) + await waitFor(() => expect(screen.getByTestId("save-button")).toBeEnabled()) + + // A deny entry lands externally while the list edit holds the merge back. + ;(useExtensionState as ReturnType).mockReturnValue({ + ...defaultExtensionState, + deniedCommands: ["rm -rf"], + }) + view.rerender() + + fireEvent.click(screen.getByTestId("save-button")) + + // The submit keeps the in-flight edit, and the baseline stays at the lists + // displayed before the edit - neither the held-back post's list nor the + // edited buffer's. + expect(postMessage).toHaveBeenCalledWith( + expect.objectContaining({ + type: "updateSettings", + commandListBaseline: { allowedCommands: [], deniedCommands: [] }, + updatedSettings: expect.objectContaining({ deniedCommands: ["git status"] }), + }), + ) + }) + + it("keeps a deleted displayed entry in the save baseline when a state post lands during the deletion", async () => { + // The deletion counterpart: an entry the dialog displayed and the user then + // deleted must stay in the baseline, so the save classifies it as a user + // deletion and applies it. A baseline folded from the deletion-carrying + // buffer would make the entry look externally added and re-admit it. + ;(useExtensionState as ReturnType).mockReturnValue({ + ...defaultExtensionState, + deniedCommands: ["rm -rf"], + }) + vi.mocked(AutoApproveSettings).mockImplementation( + ({ setCachedStateField }: React.ComponentProps) => ( + + ), + ) + const view = renderWithExtensionState(, { + queryClient, + }) + + fireEvent.click(await screen.findByTestId("clear-deny-list")) + await waitFor(() => expect(screen.getByTestId("save-button")).toBeEnabled()) + + // A coincidental post re-bakes the deny list and adds an entry the dialog + // never displayed; the held merge keeps the buffered deletion. + ;(useExtensionState as ReturnType).mockReturnValue({ + ...defaultExtensionState, + deniedCommands: ["rm -rf", "docker"], + }) + view.rerender() + + fireEvent.click(screen.getByTestId("save-button")) + + // The baseline retains the deleted (displayed) entry so the save applies the + // deletion, and excludes the never-displayed "docker" so the save keeps that + // entry and warns about it instead. + expect(postMessage).toHaveBeenCalledWith( + expect.objectContaining({ + type: "updateSettings", + commandListBaseline: { allowedCommands: [], deniedCommands: ["rm -rf"] }, + updatedSettings: expect.objectContaining({ deniedCommands: [] }), + }), + ) + }) + it("discards IO Intelligence cached edits and restores the extension values", async () => { const onDone = vi.fn() ;(useExtensionState as ReturnType).mockReturnValue({ From 97e5ab1322561c201515332ea660587754596f6d Mon Sep 17 00:00:00 2001 From: Franz Daubner Date: Thu, 5 Nov 2026 22:16:47 +0100 Subject: [PATCH 8/9] fix: stop false settings-changed warning for hidden command entries Workspace command-list entries hidden from the settings dialog - by the default-off workspace gate or by a shadowing entry in the opposite list - were classified as added outside the dialog, so every save showed a false "settings file changed" warning naming them. Compute the hidden-entry set before classifying external additions and keep those entries out of the warning; the save keeps preserving them in the workspace layer unchanged. --- .../__tests__/webviewMessageHandler.spec.ts | 104 ++++++++++++++++++ src/core/webview/webviewMessageHandler.ts | 50 +++++---- 2 files changed, 134 insertions(+), 20 deletions(-) diff --git a/src/core/webview/__tests__/webviewMessageHandler.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index 61c3d35d75..fbf68ed511 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -2828,6 +2828,110 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { expect(vscode.window.showWarningMessage).toHaveBeenCalledWith(expect.stringContaining("curl")) expect(vscode.window.showWarningMessage).toHaveBeenCalledWith(expect.stringContaining("docker")) }) + + // A file-side extra the hidden-entry re-add already preserves was never + // displayed, so naming it in the changed-settings warning would repeat that + // warning on every save while the file stays unchanged. + it.each([ + { + name: "hidden while the workspace gate is off", + gateEnabled: false, + hiddenEntry: "docker", + prepare: () => { + vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(undefined) + }, + }, + { + name: "shadowed by a global allow while the gate is on", + gateEnabled: true, + hiddenEntry: "git push", + prepare: () => { + vi.mocked(mockClineProvider.contextProxy.getValue).mockImplementation((key: string) => + key === "allowedCommands" ? ["git"] : undefined, + ) + }, + }, + ])( + "re-adds a $name deny entry without warning that the settings file changed", + async ({ gateEnabled, hiddenEntry, prepare }) => { + prepare() + const { update } = mockCommandConfiguration( + { workspaceValue: [hiddenEntry] }, + undefined, + undefined, + gateEnabled, + ) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: [] }, + // The dialog displayed the effective deny list, which never + // contained the hidden entry, so the baseline omits it. + commandListBaseline: { deniedCommands: [] }, + }) + + expect(update).toHaveBeenCalledWith("deniedCommands", [hiddenEntry], vscode.ConfigurationTarget.Workspace) + expect(vscode.window.showWarningMessage).not.toHaveBeenCalled() + }, + ) + + it("warns only about the genuine file addition when a hidden entry is also present", async () => { + // Mixed divergence in one save: "git push" is hidden by the global allow + // "git" and preserved by the re-add, while "wget" arrived in the visible + // part of the file after the dialog opened. The warning must name only + // the genuine addition. + vi.mocked(mockClineProvider.contextProxy.getValue).mockImplementation((key: string) => + key === "allowedCommands" ? ["git"] : undefined, + ) + const { update } = mockCommandConfiguration( + (key) => (key === "deniedCommands" ? { workspaceValue: ["git push", "curl", "wget"] } : {}), + undefined, + undefined, + true, + ) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: ["curl"] }, + // The dialog displayed the shadow-filtered deny list: "curl" was + // visible and kept, "git push" was never displayed, and "wget" + // arrived in the settings file after the dialog opened. + commandListBaseline: { deniedCommands: ["curl"] }, + }) + + // "git push" survives through the hidden-entry re-add, "wget" through + // the external-addition re-admission; the global layer stays empty. + expect(update).toHaveBeenCalledWith( + "deniedCommands", + ["curl", "wget", "git push"], + vscode.ConfigurationTarget.Workspace, + ) + expect(update).toHaveBeenCalledWith("deniedCommands", [], vscode.ConfigurationTarget.Global) + expect(vscode.window.showWarningMessage).toHaveBeenCalledTimes(1) + expect(vscode.window.showWarningMessage).toHaveBeenCalledWith(expect.stringContaining("1 entry was")) + expect(vscode.window.showWarningMessage).toHaveBeenCalledWith(expect.stringContaining("wget")) + expect(vscode.window.showWarningMessage).not.toHaveBeenCalledWith(expect.stringContaining("git push")) + }) + + it("still warns about a visible workspace addition alongside the hidden-entry exclusion", async () => { + // Regression witness for the hidden-entry exclusion: an entry that + // arrived in the displayed part of the file is a genuine external + // addition, is kept in the layer the snapshot found it in, and is named + // in the warning. + vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(undefined) + const { update } = mockCommandConfiguration({ workspaceValue: ["git", "curl"] }, undefined, undefined, true) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { allowedCommands: ["git"] }, + commandListBaseline: { allowedCommands: ["git"] }, + }) + + expect(update).toHaveBeenCalledWith("allowedCommands", ["git", "curl"], vscode.ConfigurationTarget.Workspace) + expect(update).toHaveBeenCalledWith("allowedCommands", [], vscode.ConfigurationTarget.Global) + expect(vscode.window.showWarningMessage).toHaveBeenCalledTimes(1) + expect(vscode.window.showWarningMessage).toHaveBeenCalledWith(expect.stringContaining("curl")) + }) }) describe("webviewMessageHandler - terminalProfile", () => { diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index c20867cd78..52eb8d861b 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -852,32 +852,15 @@ export const webviewMessageHandler = async ( } const currentScopes = readCommandListScopes(key, getGlobalState(key), inspection) - // An entry the settings file gained while the dialog was open has - // the same shape as an entry the user deleted: present in the file, - // absent from the submitted list. Only the dialog's last-displayed - // baseline separates the two, so file entries outside that baseline - // are re-admitted before the split (deletions of baseline entries - // still apply). A sender that shows no lists sends no baseline, and - // its submitted list is written unclassified. - const baseline = message.commandListBaseline?.[key] - const externallyAdded = Array.isArray(baseline) - ? [...new Set([...currentScopes.workspace, ...currentScopes.global])].filter( - (entry) => !baseline.includes(entry) && !incoming.includes(entry), - ) - : [] - const { globalWrite, workspaceWrite: keptWorkspaceWrite } = splitCommandListScopes( - [...incoming, ...externallyAdded], - currentScopes.workspace, - currentScopes.global, - ) // Shadowing is cross-directional, so the hidden-entry set // needs the opposite key's global layer as input. It comes // from the save-start snapshot, not a fresh read: an // earlier iteration of this loop may already have written // that key. The layer only feeds the additive preservation - // below and never this key's split, target, or rollback, - // which stay on the per-key configuration snapshot. + // below and this key's external-addition classification, and + // never this key's split, target, or rollback, which stay on + // the per-key configuration snapshot. const otherKey = key === "allowedCommands" ? "deniedCommands" : "allowedCommands" const otherScopes = readCommandListScopes(otherKey, commandListGlobalsAtSaveStart[otherKey]) const { hiddenWorkspaceAllow, hiddenWorkspaceDeny } = resolveEffectiveCommandLists( @@ -893,6 +876,33 @@ export const webviewMessageHandler = async ( // file. const hiddenWorkspaceEntries = key === "allowedCommands" ? hiddenWorkspaceAllow : hiddenWorkspaceDeny + + // An entry the settings file gained while the dialog was open has + // the same shape as an entry the user deleted: present in the file, + // absent from the submitted list. Only the dialog's last-displayed + // baseline separates the two, so file entries outside that baseline + // are re-admitted before the split (deletions of baseline entries + // still apply). Hidden entries never appear in that baseline, yet + // the workspace-layer re-add below preserves them, so classifying + // them as external additions would repeat a false "settings file + // changed" warning on every save. A sender that shows no lists + // sends no baseline, and its submitted list is written + // unclassified. + const baseline = message.commandListBaseline?.[key] + const externallyAdded = Array.isArray(baseline) + ? [...new Set([...currentScopes.workspace, ...currentScopes.global])].filter( + (entry) => + !baseline.includes(entry) && + !incoming.includes(entry) && + !hiddenWorkspaceEntries.includes(entry), + ) + : [] + const { globalWrite, workspaceWrite: keptWorkspaceWrite } = splitCommandListScopes( + [...incoming, ...externallyAdded], + currentScopes.workspace, + currentScopes.global, + ) + const workspaceWrite = [...new Set([...keptWorkspaceWrite, ...hiddenWorkspaceEntries])] const workspaceTarget = resolveWorkspaceWriteTarget(key, inspection) From 9736c1d9b1ca595ca84335541b4b0f338f6f5d1c Mon Sep 17 00:00:00 2001 From: Franz Daubner Date: Fri, 6 Nov 2026 13:48:04 +0100 Subject: [PATCH 9/9] fix: promote re-added hidden command entries to the global write When a settings save submitted an entry that sits in the workspace layer but is hidden from the settings dialog - by the default-off workspace gate or by a shadowing entry in the opposite list - the code that splits entries between the global and workspace writes treated it as already known, so the save succeeded while the entry stayed unenforced. The save path now excludes hidden entries from the workspace layer it passes to the split, so a re-added hidden entry is promoted to the global write, while the existing re-add keeps it in the workspace layer. Two save-path tests pin the promotion for both hide reasons. Tests also pin the raw global command lists surfaced to the extension host when the workspace layer differs, and that an explicitly empty stored list seeds empty instead of the defaults. --- .../webview/__tests__/ClineProvider.spec.ts | 25 ++++++++ .../__tests__/webviewMessageHandler.spec.ts | 57 ++++++++++++++++++- src/core/webview/webviewMessageHandler.ts | 16 ++++-- .../__tests__/seedAllowedCommands.spec.ts | 19 +++++++ 4 files changed, 110 insertions(+), 7 deletions(-) diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 8c46daf85e..55f9ebf994 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -4147,6 +4147,31 @@ describe("ClineProvider", () => { expect(state.allowedCommands).toEqual([]) }) + test("getState returns the raw global command lists when the workspace layer differs", async () => { + await provider.contextProxy.setValue("allowedCommands", ["git"]) + await provider.contextProxy.setValue("deniedCommands", ["curl"]) + const config = makeWorkspaceConfiguration({ workspaceCommandListsEnabled: true }) + const inspect = vi.fn() + inspect.mockImplementation((key: string) => ({ + key, + workspaceValue: key === "allowedCommands" ? ["docker"] : ["pnpm"], + })) + config.inspect = inspect + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(config) + + const state = await provider.getState() + + // The raw global layers are the feed per-folder enforcement + // re-composes from, so they must stay distinguishable from the + // window-composed pair: with the layers diverging here, a dropped + // field or a composed value would silently reroute enforcement to + // the first-folder lists. + expect(state.globalAllowedCommands).toEqual(["git"]) + expect(state.globalDeniedCommands).toEqual(["curl"]) + expect(state.allowedCommands).toEqual(["git", "docker"]) + expect(state.deniedCommands).toEqual(["curl", "pnpm"]) + }) + test("getState prefers the workspace-folder value for multi-root workspaces", async () => { vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ get: vi.fn().mockImplementation((key: string) => key === "workspaceCommandListsEnabled"), diff --git a/src/core/webview/__tests__/webviewMessageHandler.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index fbf68ed511..1fbc13e2dd 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -2176,7 +2176,10 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { } it("keeps workspace-scoped denied commands out of the global write when saving the merged list", async () => { - const { update, inspect } = mockCommandConfiguration({ workspaceValue: ["docker"] }) + // Gate on: the workspace entry is visible in the dialog, so keeping it + // is not a re-add. (With the gate off every submitted workspace entry + // is a re-add and is promoted to the global layer instead.) + const { update, inspect } = mockCommandConfiguration({ workspaceValue: ["docker"] }, undefined, undefined, true) await webviewMessageHandler(mockClineProvider, { type: "updateSettings", @@ -2198,7 +2201,14 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { }) it("writes the workspace-folder value at the folder target in multi-root workspaces", async () => { - const { update } = mockCommandConfiguration({ workspaceValue: ["npm"], workspaceFolderValue: ["docker"] }) + // Gate on for the same reason as the merged-list split test: the kept + // folder entry is visible in the dialog, so it stays at its scope. + const { update } = mockCommandConfiguration( + { workspaceValue: ["npm"], workspaceFolderValue: ["docker"] }, + undefined, + undefined, + true, + ) await webviewMessageHandler(mockClineProvider, { type: "updateSettings", @@ -2480,6 +2490,49 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { expect(update).toHaveBeenCalledWith("deniedCommands", [], vscode.ConfigurationTarget.Global) }) + it("promotes a gate-hidden workspace entry the dialog re-adds to the global write", async () => { + // With the gate off the dialog shows only the global layer, so a + // submitted entry the workspace layer already holds hidden is one the + // user just added: it must reach the enforced global layer instead of + // being treated as existing knowledge, while the re-add below still + // keeps it in the workspace file. + const { update } = mockCommandConfiguration({ workspaceValue: ["docker"] }, undefined, undefined, false) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: ["docker", "aws"] }, + }) + + expect(update).toHaveBeenCalledWith("deniedCommands", ["docker"], vscode.ConfigurationTarget.Workspace) + expect(update).toHaveBeenCalledWith("deniedCommands", ["docker", "aws"], vscode.ConfigurationTarget.Global) + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("deniedCommands", ["docker", "aws"]) + }) + + it("promotes a shadowed workspace entry the dialog re-adds to the global write", async () => { + // The global allow "git" shadows the workspace deny "git push", so the + // dialog never displayed the deny entry; a user who re-adds it submits a + // list containing it, and the entry must land in the global layer where + // the gate makes it effective, while the re-add preserves the workspace + // copy. + vi.mocked(mockClineProvider.contextProxy.getValue).mockImplementation((key: string) => + key === "allowedCommands" ? ["git"] : undefined, + ) + const { update } = mockCommandConfiguration( + (key) => (key === "deniedCommands" ? { workspaceValue: ["git push"] } : {}), + undefined, + undefined, + true, + ) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: ["git push"] }, + }) + + expect(update).toHaveBeenCalledWith("deniedCommands", ["git push"], vscode.ConfigurationTarget.Workspace) + expect(update).toHaveBeenCalledWith("deniedCommands", ["git push"], vscode.ConfigurationTarget.Global) + }) + it("re-adds a shadowed workspace allow entry alongside the user's visible deletions", async () => { vi.mocked(mockClineProvider.contextProxy.getValue).mockImplementation((key: string) => key === "deniedCommands" ? ["git"] : undefined, diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 52eb8d861b..e75df27a16 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -857,10 +857,11 @@ export const webviewMessageHandler = async ( // needs the opposite key's global layer as input. It comes // from the save-start snapshot, not a fresh read: an // earlier iteration of this loop may already have written - // that key. The layer only feeds the additive preservation - // below and this key's external-addition classification, and - // never this key's split, target, or rollback, which stay on - // the per-key configuration snapshot. + // that key. The layer only feeds the hidden-entry mechanism + // below — additive preservation, external-addition + // classification, and the split's promotion filter — + // and never this key's target or rollback, which stay + // on the per-key configuration snapshot. const otherKey = key === "allowedCommands" ? "deniedCommands" : "allowedCommands" const otherScopes = readCommandListScopes(otherKey, commandListGlobalsAtSaveStart[otherKey]) const { hiddenWorkspaceAllow, hiddenWorkspaceDeny } = resolveEffectiveCommandLists( @@ -899,7 +900,12 @@ export const webviewMessageHandler = async ( : [] const { globalWrite, workspaceWrite: keptWorkspaceWrite } = splitCommandListScopes( [...incoming, ...externallyAdded], - currentScopes.workspace, + // Hidden entries are not existing knowledge for promotion + // purposes: the dialog never displayed them, so an entry the + // user just added is new even when the workspace layer + // already holds it hidden. The re-add below restores the + // hidden entries this reduced layer drops. + currentScopes.workspace.filter((entry) => !hiddenWorkspaceEntries.includes(entry)), currentScopes.global, ) diff --git a/src/utils/__tests__/seedAllowedCommands.spec.ts b/src/utils/__tests__/seedAllowedCommands.spec.ts index 50c1dd5fc6..c5ddd11767 100644 --- a/src/utils/__tests__/seedAllowedCommands.spec.ts +++ b/src/utils/__tests__/seedAllowedCommands.spec.ts @@ -206,6 +206,25 @@ describe("seedAllowedCommands", () => { expect(globalState.get("allowedCommands")).toEqual(["git"]) }) + it("seeds an explicitly empty global value instead of the populated default", async () => { + // An explicitly empty global list is a deliberate opt-out: the seed + // persists [] so global state counts as seeded, rather than treating + // the empty allowlist as unset and re-granting the declared defaults. + mockConfiguration({ + inspect: { + key: "allowedCommands", + defaultValue: ["git log", "git diff", "git show"], + globalValue: [], + }, + }) + const globalState = mockMemento() + + await seedAllowedCommands(globalState) + + expect(globalState.update).toHaveBeenCalledExactlyOnceWith("allowedCommands", []) + expect(globalState.get("allowedCommands")).toEqual([]) + }) + it("does not overwrite global state that is already populated", async () => { mockConfiguration({ inspect: { key: "allowedCommands", globalValue: ["git"] }, effective: ["git"] }) const globalState = mockMemento(["npm"])