From e67ab35cdd9c154d5bda7b21d35d08a76f7390af Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 29 Oct 2026 03:00:17 +0800 Subject: [PATCH 01/25] feat(webview): durable per-view viewStates pipeline (N2) Split unit N2 of #1546: viewStates read/write queue, claim rekey, prune, clear, load/save and export/import sanitization on top of N1 identity. Buffer/mutation/history content remains for N3. --- .husky/pre-commit | 26 - .husky/pre-push | 42 -- .../src/__tests__/global-settings.test.ts | 43 ++ packages/types/src/__tests__/index.test.ts | 5 + packages/types/src/global-settings.ts | 17 + packages/types/src/vscode-extension-host.ts | 15 + src/core/config/ContextProxy.ts | 3 + src/core/config/ProviderSettingsManager.ts | 20 +- .../config/__tests__/ContextProxy.spec.ts | 16 + .../__tests__/ProviderSettingsManager.spec.ts | 7 +- .../config/__tests__/importExport.spec.ts | 45 ++ src/core/config/importExport.ts | 8 + src/core/webview/ClineProvider.ts | 651 +++++++++++++++++- .../webview/__tests__/ClineProvider.spec.ts | 50 ++ 14 files changed, 861 insertions(+), 87 deletions(-) delete mode 100644 .husky/pre-commit delete mode 100644 .husky/pre-push diff --git a/.husky/pre-commit b/.husky/pre-commit deleted file mode 100644 index f8f420dac7..0000000000 --- a/.husky/pre-commit +++ /dev/null @@ -1,26 +0,0 @@ -branch="$(git rev-parse --abbrev-ref HEAD)" - -if [ "$branch" = "main" ]; then - echo "You can't commit directly to main - please check out a branch." - exit 1 -fi - -# Detect if running on Windows and use pnpm.cmd, otherwise use pnpm. -if [ "$OS" = "Windows_NT" ]; then - pnpm_cmd="pnpm.cmd" -else - if command -v pnpm >/dev/null 2>&1; then - pnpm_cmd="pnpm" - else - pnpm_cmd="npx pnpm" - fi -fi - -# Detect if running on Windows and use npx.cmd, otherwise use npx. -if [ "$OS" = "Windows_NT" ]; then - npx_cmd="npx.cmd" -else - npx_cmd="npx" -fi - -$npx_cmd lint-staged diff --git a/.husky/pre-push b/.husky/pre-push deleted file mode 100644 index 4cf91d9580..0000000000 --- a/.husky/pre-push +++ /dev/null @@ -1,42 +0,0 @@ -branch="$(git rev-parse --abbrev-ref HEAD)" - -if [ "$branch" = "main" ]; then - echo "You can't push directly to main - please check out a branch." - exit 1 -fi - -# Detect if running on Windows and use pnpm.cmd, otherwise use pnpm. -if [ "$OS" = "Windows_NT" ]; then - pnpm_cmd="pnpm.cmd" -else - if command -v pnpm >/dev/null 2>&1; then - pnpm_cmd="pnpm" - else - pnpm_cmd="npx pnpm" - fi -fi - -$pnpm_cmd run check-types - -# Use dotenvx to securely load .env.local and run commands that depend on it -if [ -f ".env.local" ]; then - # Check if RUN_TESTS_ON_PUSH is set to true and run tests with dotenvx - if npx dotenvx get RUN_TESTS_ON_PUSH -f .env.local 2>/dev/null | grep -q "^true$"; then - npx dotenvx run -f .env.local -- $pnpm_cmd run test - fi -else - # Fallback: run tests if RUN_TESTS_ON_PUSH is set in regular environment - if [ "$RUN_TESTS_ON_PUSH" = "true" ]; then - $pnpm_cmd run test - fi -fi - -# Check for new changesets. -NEW_CHANGESETS=$(find .changeset -name "*.md" ! -name "README.md" | wc -l | tr -d ' ') -echo "Changeset files: $NEW_CHANGESETS" - -if [ "$NEW_CHANGESETS" = "0" ]; then - echo "-------------------------------------------------------------------------------------" - echo "Changes detected. Please run 'pnpm changeset' to create a changeset if applicable." - echo "-------------------------------------------------------------------------------------" -fi diff --git a/packages/types/src/__tests__/global-settings.test.ts b/packages/types/src/__tests__/global-settings.test.ts index f2b2e7e8e5..b6b01cccf0 100644 --- a/packages/types/src/__tests__/global-settings.test.ts +++ b/packages/types/src/__tests__/global-settings.test.ts @@ -39,6 +39,49 @@ describe("alwaysDenyUnapprovedCommands global setting", () => { }) }) +describe("viewStates global setting", () => { + it("parses a valid per-view record", () => { + const parsed = globalSettingsSchema.parse({ + viewStates: { + "stable-view-1": { mode: "code", currentApiConfigName: "profile-a", updatedAt: 1730000000000 }, + "stable-view-2": {}, + }, + }) + expect(parsed.viewStates?.["stable-view-1"]).toEqual({ + mode: "code", + currentApiConfigName: "profile-a", + updatedAt: 1730000000000, + }) + expect(parsed.viewStates?.["stable-view-2"]).toEqual({}) + expect(GLOBAL_SETTINGS_KEYS).toContain("viewStates") + }) + + it("parses an entry that carries the host-minted claim token", () => { + // The claim binds the entry to the webview that wrote it; a legacy entry without one + // still parses, because refusing it is setViewStateId's job, not the schema's. + const parsed = globalSettingsSchema.parse({ + viewStates: { + "stable-view-1": { mode: "code", updatedAt: 1730000000000, claimToken: "7f9c-host-minted" }, + "stable-view-2": { mode: "code" }, + }, + }) + expect(parsed.viewStates?.["stable-view-1"]?.claimToken).toBe("7f9c-host-minted") + expect(parsed.viewStates?.["stable-view-2"]).toEqual({ mode: "code" }) + }) + + it("rejects invalid field values inside a view entry", () => { + expect(() => globalSettingsSchema.parse({ viewStates: { v: { mode: 42 } } })).toThrow() + expect(() => globalSettingsSchema.parse({ viewStates: { v: { currentApiConfigName: 42 } } })).toThrow() + expect(() => globalSettingsSchema.parse({ viewStates: { v: { updatedAt: "soon" } } })).toThrow() + expect(() => globalSettingsSchema.parse({ viewStates: { v: { claimToken: 42 } } })).toThrow() + }) + + it("rejects non-object viewStates values", () => { + expect(() => globalSettingsSchema.parse({ viewStates: "not-a-record" })).toThrow() + expect(() => globalSettingsSchema.parse({ viewStates: { v: "not-an-entry" } })).toThrow() + }) +}) + describe("collapseCommandOutputByDefault global setting", () => { it.each([true, false, undefined])("round-trips %s through the export/import schema", (value) => { const settings = globalSettingsSchema.parse({ collapseCommandOutputByDefault: value }) diff --git a/packages/types/src/__tests__/index.test.ts b/packages/types/src/__tests__/index.test.ts index 15441d48fd..b4cee22f8c 100644 --- a/packages/types/src/__tests__/index.test.ts +++ b/packages/types/src/__tests__/index.test.ts @@ -3,6 +3,10 @@ import { GLOBAL_STATE_KEYS } from "../index.js" describe("GLOBAL_STATE_KEYS", () => { + it("should contain registered durable per-view state", () => { + expect(GLOBAL_STATE_KEYS).toContain("viewStates") + }) + it("should contain provider settings keys", () => { expect(GLOBAL_STATE_KEYS).toContain("autoApprovalEnabled") }) @@ -13,6 +17,7 @@ describe("GLOBAL_STATE_KEYS", () => { it("should not contain secret state keys", () => { expect(GLOBAL_STATE_KEYS).not.toContain("openRouterApiKey") + expect(GLOBAL_STATE_KEYS).not.toContain("apiKey") }) it("should contain OpenAI Compatible base URL setting", () => { diff --git a/packages/types/src/global-settings.ts b/packages/types/src/global-settings.ts index eb51c1d67d..b09bae2635 100644 --- a/packages/types/src/global-settings.ts +++ b/packages/types/src/global-settings.ts @@ -114,6 +114,22 @@ export const MAX_CHECKPOINT_TIMEOUT_SECONDS = 60 */ export const DEFAULT_CHECKPOINT_TIMEOUT_SECONDS = 15 +/** + * Persisted non-secret selections for a stable webview instance. + */ +export const viewStateSchema = z.object({ + mode: z.string().optional(), + currentApiConfigName: z.string().optional(), + updatedAt: z.number().optional(), + /** + * Host-minted capability token binding this entry to the webview that wrote it. + * Minted by the extension host (`crypto.randomUUID()`), delivered to that webview and + * never accepted from webview input. An entry is only re-adopted for a webview that + * presents this token, so a view cannot read another (e.g. closed) view's selections. + */ + claimToken: z.string().optional(), +}) + /** * GlobalSettings */ @@ -122,6 +138,7 @@ export const globalSettingsSchema = z.object({ currentApiConfigName: z.string().optional(), listApiConfigMeta: z.array(providerSettingsEntrySchema).optional(), pinnedApiConfigs: z.record(z.string(), z.boolean()).optional(), + viewStates: z.record(z.string(), viewStateSchema).optional(), lastShownAnnouncementId: z.string().optional(), customInstructions: z.string().optional(), diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index a5393ddc66..bc6e6c6279 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -111,6 +111,7 @@ export interface ExtensionMessage { | "originalContent" | "rooHistoryImportProgress" | "themeFixtureProbeRequest" + | "viewStateClaim" text?: string /** For fileContent: { path, content, error? } */ fileContent?: { path: string; content: string | null; error?: string } @@ -169,6 +170,13 @@ export interface ExtensionMessage { // eslint-disable-next-line @typescript-eslint/no-explicit-any values?: Record requestId?: string + /** + * For `viewStateClaim`: the durable `viewStates` key the delivered claim governs. The + * message is only ever sent to the webview that key belongs to. + */ + viewStateId?: string + /** For `viewStateClaim`: the host-minted token the webview stores and echoes on launch. */ + viewStateClaim?: string themeFixture?: WebviewThemeFixture promptText?: string results?: @@ -669,6 +677,13 @@ export interface WebviewMessage { | "openRulesDirectory" | "themeFixtureProbeResponse" text?: string + viewStateId?: string + /** + * For `webviewDidLaunch`: the host-minted claim previously delivered for `viewStateId`. + * The webview only echoes what it was given; the host rejects an id whose stored claim + * does not match, so a view cannot adopt another view's persisted selections. + */ + viewStateClaim?: string taskId?: string editedMessageContent?: string tab?: "settings" | "history" | "mcp" | "modes" | "chat" | "marketplace" | "cloud" diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index 1250a7baca..4e20305f48 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -36,6 +36,9 @@ const globalSettingsExportSchema = globalSettingsSchema.omit({ taskHistory: true, listApiConfigMeta: true, currentApiConfigName: true, + // Per-view selection state is machine-local: it keeps flowing through the + // normal runtime and pruning paths but must not transfer between settings. + viewStates: true, }) export class ContextProxy { diff --git a/src/core/config/ProviderSettingsManager.ts b/src/core/config/ProviderSettingsManager.ts index 01695d7994..306253578a 100644 --- a/src/core/config/ProviderSettingsManager.ts +++ b/src/core/config/ProviderSettingsManager.ts @@ -55,6 +55,19 @@ export const providerProfilesSchema = z.object({ export type ProviderProfiles = z.infer +/** + * Signals that a profile's configuration no longer exists. Callers that treat an + * already-deleted profile as an idempotent no-op branch on this type instead of + * matching error message text, which a profile name containing the phrase could + * otherwise spoof. + */ +export class ProviderSettingsNotFoundError extends Error { + constructor(message: string) { + super(message) + this.name = "ProviderSettingsNotFoundError" + } +} + export class ProviderSettingsManager { private static readonly SCOPE_PREFIX = "roo_cline_config_" private readonly defaultConfigId = this.generateId() @@ -511,7 +524,7 @@ export class ProviderSettingsManager { const providerProfiles = await this.load() if (!providerProfiles.apiConfigs[name]) { - throw new Error(`Config '${name}' not found`) + throw new ProviderSettingsNotFoundError(`Config '${name}' not found`) } if (Object.keys(providerProfiles.apiConfigs).length === 1) { @@ -522,6 +535,11 @@ export class ProviderSettingsManager { await this.store(providerProfiles) }) } catch (error) { + // A missing config is a caller-meaningful signal, not a failure: rethrow it + // unwrapped so callers can branch on the type instead of message text. + if (error instanceof ProviderSettingsNotFoundError) { + throw error + } throw new Error(`Failed to delete config: ${error}`) } } diff --git a/src/core/config/__tests__/ContextProxy.spec.ts b/src/core/config/__tests__/ContextProxy.spec.ts index b4d3b1b8aa..f9fa9cdb15 100644 --- a/src/core/config/__tests__/ContextProxy.spec.ts +++ b/src/core/config/__tests__/ContextProxy.spec.ts @@ -740,4 +740,20 @@ Output only the summary of the conversation so far, without any additional comme expect(customSupportPromptsUpdateCalls.length).toBe(0) }) }) + + describe("export", () => { + it("should exclude viewStates from the exported settings", async () => { + await proxy.setValue("viewStates", { + "stable-sidebar-view": { mode: "architect", currentApiConfigName: "profile-a", updatedAt: 1 }, + }) + await proxy.setValue("customInstructions", "global instructions") + + const exported = await proxy.export() + + // Per-view selection state is machine-local and must never transfer + // between settings, while ordinary global settings keep round-tripping. + expect(exported).not.toHaveProperty("viewStates") + expect(exported?.customInstructions).toBe("global instructions") + }) + }) }) diff --git a/src/core/config/__tests__/ProviderSettingsManager.spec.ts b/src/core/config/__tests__/ProviderSettingsManager.spec.ts index a1fc9ac7de..3b93b7cd22 100644 --- a/src/core/config/__tests__/ProviderSettingsManager.spec.ts +++ b/src/core/config/__tests__/ProviderSettingsManager.spec.ts @@ -12,7 +12,7 @@ import { import { clearAllMocks } from "../../../test-utils/reset" import { makeExtensionContext } from "../../../test-utils/vscode" -import { ProviderSettingsManager, ProviderProfiles } from "../ProviderSettingsManager" +import { ProviderSettingsManager, ProviderSettingsNotFoundError, ProviderProfiles } from "../ProviderSettingsManager" // `export()` builds an API handler per profile to read model capabilities. Mock // buildApiHandler with the real @roo-code/types model definitions so the token-field @@ -973,6 +973,11 @@ describe("ProviderSettingsManager", () => { }), ) + // The typed not-found signal is the contract callers branch on: a profile + // name containing "not found" must not be matchable via message text. + await expect(providerSettingsManager.deleteConfig("nonexistent")).rejects.toBeInstanceOf( + ProviderSettingsNotFoundError, + ) await expect(providerSettingsManager.deleteConfig("nonexistent")).rejects.toThrow( "Config 'nonexistent' not found", ) diff --git a/src/core/config/__tests__/importExport.spec.ts b/src/core/config/__tests__/importExport.spec.ts index 90db81d8a1..b8dff9c0ed 100644 --- a/src/core/config/__tests__/importExport.spec.ts +++ b/src/core/config/__tests__/importExport.spec.ts @@ -331,6 +331,51 @@ describe("importExport", () => { ]) }) + it("should not apply imported viewStates to the context proxy", async () => { + const fileContent = JSON.stringify({ + providerProfiles: { + currentApiConfigName: "test", + apiConfigs: { + test: { apiProvider: providerIdentifiers.openai, apiKey: "test-key", id: "test-id" }, + }, + }, + globalSettings: { + mode: "code", + viewStates: { + "stable-sidebar-view": { + mode: "architect", + currentApiConfigName: "profile-a", + updatedAt: 1, + }, + }, + }, + }) + + ;(fs.readFile as Mock).mockResolvedValue(fileContent) + + mockProviderSettingsManager.export.mockResolvedValue({ + currentApiConfigName: "default", + apiConfigs: { default: { apiProvider: providerIdentifiers.anthropic, id: "default-id" } }, + }) + + mockProviderSettingsManager.listConfig.mockResolvedValue([ + { name: "test", id: "test-id", apiProvider: providerIdentifiers.openai }, + { name: "default", id: "default-id", apiProvider: providerIdentifiers.anthropic }, + ]) + + const result = await importSettingsFromPath("/mock/path/settings.json", { + providerSettingsManager: mockProviderSettingsManager, + contextProxy: mockContextProxy, + customModesManager: mockCustomModesManager, + }) + + expect(result.success).toBe(true) + // Per-view selection state is machine-local: importing settings must not + // apply another machine's view pins, while other settings round-trip. + expect(mockContextProxy.setValues).toHaveBeenCalledWith({ mode: "code" }) + expect(result).not.toHaveProperty("globalSettings.viewStates") + }) + it("should return success: false when file content is invalid", async () => { ;(vscode.window.showOpenDialog as Mock).mockResolvedValue([{ fsPath: "/mock/path/settings.json" }]) diff --git a/src/core/config/importExport.ts b/src/core/config/importExport.ts index 9882039ad3..29c9497552 100644 --- a/src/core/config/importExport.ts +++ b/src/core/config/importExport.ts @@ -98,6 +98,14 @@ function sanitizeGlobalSettings(rawGlobalSettings: unknown): { for (const [key, rawValue] of Object.entries(rawGlobalSettings)) { const path = `globalSettings.${key}` + + // Per-view selection state is machine-local: it round-trips through the + // normal runtime and pruning paths, but importing it would pin selections + // from another machine's views on this one. + if (key === "viewStates") { + continue + } + const schema = globalSettingsShape[key as keyof GlobalSettings] if (!schema) { diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 3d88df632b..f5f09244ff 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -59,6 +59,7 @@ import { isRetiredProvider, modelIdKeys, providerIdentifiers, + PROVIDER_SETTINGS_KEYS, } from "@roo-code/types" import { RateLimitClock, createRateLimitClock } from "../task/RateLimitClock" import { TaskRegistry } from "../task/TaskRegistry" @@ -112,7 +113,7 @@ import { buildApiHandler } from "../../api" import { forceFullModelDetailsLoad, hasLoadedFullDetails } from "../../api/providers/fetchers/lmstudio" import { ContextProxy } from "../config/ContextProxy" -import { ProviderSettingsManager } from "../config/ProviderSettingsManager" +import { ProviderSettingsManager, ProviderSettingsNotFoundError } from "../config/ProviderSettingsManager" import { CustomModesManager } from "../config/CustomModesManager" import { PendingActionSettlementError, Task } from "../task/Task" @@ -139,6 +140,14 @@ import { REQUESTY_BASE_URL } from "../../shared/utils/requesty" import { validateAndFixToolResultIds } from "../task/validateToolResultIds" import { PendingEditOperationStore, type PendingEditOperationInput } from "./PendingEditOperationStore" +type PersistedViewState = NonNullable[string] + +/** + * Values that can be held in a view-local state buffer (in-memory) and, for the + * non-secret subset, persisted durably per stable view id. + */ +type ViewLocalStateValues = Partial & Partial + /** * https://github.com/microsoft/vscode-webview-ui-toolkit-samples/blob/main/default/weather-webview/src/providers/WeatherViewProvider.ts * https://github.com/KumarVariable/vscode-extension-sidebar-html/blob/master/src/customSidebarViewProvider.ts @@ -219,6 +228,10 @@ export class ClineProvider public static readonly sideBarId = `${Package.name}.SidebarProvider` public static readonly tabPanelId = `${Package.name}.TabPanelProvider` private static activeInstances: Set = new Set() + private static nextViewId = 0 + private static readonly MAX_PERSISTED_VIEW_STATES = 50 + private static readonly MAX_VIEW_STATE_ID_LENGTH = 64 + private static persistedViewStateWriteQueue: Promise = Promise.resolve() private disposables: vscode.Disposable[] = [] private webviewDisposables: vscode.Disposable[] = [] private pendingThemeFixtureProbes = new Map< @@ -351,6 +364,45 @@ export class ClineProvider */ private clineMessagesSeq = 0 + /** + * Unique identifier for this provider instance's view. + * Based on renderContext and a monotonically increasing counter to ensure uniqueness across multiple instances. + */ + public readonly viewId: string + + /** + * Stable identifier for persisted per-view state keys. + * Defaults to viewId until the webview reports its VS Code-persisted id. + * A durable entry under this id is bound to the webview that wrote it by a host-minted + * claim token (see setViewStateId), so the entry - and the profile its pin brings back + * into the state post - can outlive this instance without becoming adoptable by any + * other webview. + */ + private viewStateId: string + + /** + * Whether this instance wrote an entry under its own session-local temporary id. + * Temporary ids restart from 0 on every extension host start, so an entry under the + * same name can belong to a different session. Only re-key entries this instance wrote. + */ + private wroteUnderTemporaryViewStateId = false + + /** + * Local state buffer for this specific view instance. + * Used to isolate mode, apiConfiguration, and other fields from the shared ContextProxy singleton + * when running in parallel (multi-tab) mode. + */ + private viewLocalState: Partial = {} + + /** + * This view's pinned profile name from the view-local buffer. Exposed for + * sibling-instance inspection (getAllInstances() filtering): `viewLocalState` + * is private and must not be reached through bracket access. + */ + get pinnedProfileName(): string | undefined { + return this.viewLocalState.currentApiConfigName + } + public isViewLaunched = false public settingsImportedAt?: number public readonly latestAnnouncementId = "oct-2026-v3.90.0-providers-tools-reasoning" // v3.90.0 providers and models, safer tool execution, reasoning and streaming @@ -366,14 +418,17 @@ export class ClineProvider mdmService?: MdmService, ) { super() + // Initialize viewId based on renderContext and monotonically increasing instance identifier for uniqueness. + // activeInstances is used for visibility/iteration checks, so we keep tracking instances separately. + this.viewId = `${renderContext}-${ClineProvider.nextViewId++}` + this.viewStateId = this.viewId + ClineProvider.activeInstances.add(this) this.currentWorkspacePath = getWorkspacePath() this.pendingEditOperations = new PendingEditOperationStore( ClineProvider.PENDING_OPERATION_TIMEOUT_MS, (message) => this.log(message), ) - ClineProvider.activeInstances.add(this) - this.mdmService = mdmService void this.updateGlobalState("codebaseIndexModels", EMBEDDING_MODEL_PROFILES) @@ -398,6 +453,9 @@ export class ClineProvider await this.postStateToWebviewWithoutClineMessages() }) + // Load initial state from global state into viewLocalState buffer after dependencies used by getState are ready. + void this.loadViewState() + // Initialize MCP Hub through the singleton manager McpServerManager.getInstance(this.context, this) .then((hub) => { @@ -520,6 +578,462 @@ export class ClineProvider } } + /** + * Reads the registered viewStates map, returning a defensive copy. + * When fresh is set, the map is read directly from globalState (bypassing the + * ContextProxy cache) so serialized writes never observe a stale in-memory value. + */ + private getPersistedViewStates(options: { fresh?: boolean } = {}): Record { + const viewStates = options.fresh + ? this.context.globalState.get("viewStates") + : this.contextProxy.getValue("viewStates") + + if (!viewStates || typeof viewStates !== "object" || Array.isArray(viewStates)) { + return Object.create(null) + } + + // Copy onto a null prototype. A view id that names an Object.prototype key + // ("constructor", "toString", "valueOf", ...) must not resolve through the + // prototype chain and be mistaken for a persisted entry. + return Object.assign(Object.create(null), viewStates) + } + + /** + * Persists this view's non-secret selections through the serialized write queue. + * The write re-reads the map fresh and merges into the existing entry, removing the + * entry entirely when nothing persistable remains, so concurrent views cannot clobber it. + * The entry is keyed by the view id active when the change was made. Writes captured + * while the provider still holds its temporary (pre-launch) id persist under that id + * and are re-keyed to the stable view id when the webview registers one, so a change + * that lands before the launch message stays durable instead of being lost. + */ + private async savePersistedViewState(values: Partial): Promise { + // Capture the id at change time: a write belongs to the view that was active + // when the change was made, even if a newer id is registered while it is queued. + const viewStateId = this.viewStateId + // Set by the queued write when it had to mint a claim, delivered after it lands. + let mintedClaimToken: string | undefined + const write = ClineProvider.persistedViewStateWriteQueue.then(async () => { + const states = this.getPersistedViewStates({ fresh: true }) + // The first write to an unowned temporary id claims the slot instead of merging + // into it: temporary names restart from 0 on every host start, so an entry under + // this name can be a leftover from an earlier session, and merging would adopt its + // fields and later re-key them under the stable id. Entries under a stable id, or + // under the temporary id this instance already authored, keep the merge semantics. + const ownsTargetEntry = viewStateId !== this.viewId || this.wroteUnderTemporaryViewStateId + const current = ownsTargetEntry ? (states[viewStateId] ?? {}) : {} + const next: PersistedViewState = { ...current } + + if ("mode" in values) { + if (values.mode === undefined || values.mode === null) { + delete next.mode + } else { + next.mode = values.mode + } + } + + if ("currentApiConfigName" in values) { + if (values.currentApiConfigName === undefined || values.currentApiConfigName === null) { + delete next.currentApiConfigName + } else { + next.currentApiConfigName = values.currentApiConfigName + } + } + + let wroteEntry = false + if (!next.mode && !next.currentApiConfigName) { + delete states[viewStateId] + } else { + next.updatedAt = values.updatedAt ?? Date.now() + // Bind the entry to this webview. An existing entry keeps the token it was + // adopted with (so a reload can still prove ownership); a new entry - or one + // written before claims existed - gets a fresh host-minted token. The token is + // never taken from `values`: it is host-owned, and webview input must not be + // able to choose or supply one. + if (!next.claimToken) { + next.claimToken = crypto.randomUUID() + mintedClaimToken = next.claimToken + } + states[viewStateId] = next + wroteEntry = true + } + + await this.contextProxy.setValue("viewStates", this.prunePersistedViewStates(states)) + // Authorship is recorded only after the durable write lands: a failed persistence + // must not mark the temporary id as owned, or a later load would adopt an entry + // that was never actually stored. + if (wroteEntry && viewStateId === this.viewId) { + this.wroteUnderTemporaryViewStateId = true + } + }) + + ClineProvider.persistedViewStateWriteQueue = write.catch(() => {}) + await write + + // Delivered only once the durable write landed: a webview must never hold a claim the + // host did not store, or the next launch would present a token the entry cannot match. + if (mintedClaimToken) { + this.postViewStateClaimToWebview(viewStateId, mintedClaimToken) + } + } + + /** + * Removes the given view's entry from the registered viewStates map. + * Runs through the serialized write queue to avoid racing concurrent view-state writes. + */ + private async clearPersistedViewState(viewStateId = this.viewStateId): Promise { + const write = ClineProvider.persistedViewStateWriteQueue.then(async () => { + const states = this.getPersistedViewStates({ fresh: true }) + delete states[viewStateId] + await this.contextProxy.setValue("viewStates", states) + }) + + ClineProvider.persistedViewStateWriteQueue = write.catch(() => {}) + await write + } + + /** + * Re-binds a claim-less viewStates entry to this webview with a freshly minted claim. + * Entries written before claims existed cannot be attributed to any webview, so their + * selections are dropped instead of being carried over: keeping them would let the new + * token re-expose them on a later launch, which is exactly the disclosure this binding + * exists to prevent. Runs through the serialized write queue like every other mutation. + */ + private async rebindUnboundViewStateEntry(viewStateId: string): Promise { + const claimToken = crypto.randomUUID() + let rebound = false + + const write = ClineProvider.persistedViewStateWriteQueue.then(async () => { + const states = this.getPersistedViewStates({ fresh: true }) + const current = states[viewStateId] + // A concurrent registration may have bound this key while this write was queued: + // never overwrite an existing claim, and let the caller refuse the stale adoption. + if (current && typeof current.claimToken === "string" && current.claimToken) { + return + } + states[viewStateId] = { claimToken, updatedAt: Date.now() } + await this.contextProxy.setValue("viewStates", this.prunePersistedViewStates(states)) + rebound = true + }) + + ClineProvider.persistedViewStateWriteQueue = write.catch(() => {}) + await write + + return rebound ? claimToken : undefined + } + + /** + * Delivers the host-minted claim that governs a durable viewStates key to this view's + * webview. The webview only stores it and echoes it back on the next launch: it can + * never mint or choose a claim itself. + */ + private postViewStateClaimToWebview(viewStateId: string, claimToken: string): void { + void this.postMessageToWebview({ type: "viewStateClaim", viewStateId, viewStateClaim: claimToken }) + } + + /** + * Keeps only the most recently updated entries of the persisted view states map, + * bounded by MAX_PERSISTED_VIEW_STATES so the global key cannot grow unboundedly. + */ + private prunePersistedViewStates(states: Record): Record { + return Object.fromEntries( + Object.entries(states) + .sort(([, a], [, b]) => (b.updatedAt ?? 0) - (a.updatedAt ?? 0)) + .slice(0, ClineProvider.MAX_PERSISTED_VIEW_STATES), + ) + } + + /** + * Re-keys this provider's temporary pre-launch viewStates entry to the newly + * registered stable id so pre-launch writes become durable under the stable key + * instead of orphaning under a session-local temporary id. Only the provider's own + * temporary id is eligible: an entry under a previously registered stable id belongs + * to that webview's storage and is left alone. When the stable entry already exists + * it wins and the temporary entry is dropped, because temporary ids are session + * counters that can collide across window reloads. Runs through the serialized write + * queue like every other viewStates mutation. An entry this instance never wrote is + * left alone, so a stale entry that happens to carry the same session-local name is + * not adopted. + */ + private async rekeyPersistedViewStateEntry(nextViewStateId: string): Promise { + const previousViewStateId = this.viewId + + const write = ClineProvider.persistedViewStateWriteQueue.then(async () => { + // An entry under this name can belong to a different session, because temporary + // ids restart from 0 on every extension host start. Only re-key entries this + // instance wrote, otherwise a stale entry from another session is adopted. + if (!this.wroteUnderTemporaryViewStateId) { + return + } + + const states = this.getPersistedViewStates({ fresh: true }) + const previous = states[previousViewStateId] + + if (!previous) { + return + } + + delete states[previousViewStateId] + + if (!states[nextViewStateId]) { + states[nextViewStateId] = previous + } + + await this.contextProxy.setValue("viewStates", this.prunePersistedViewStates(states)) + + // Only a successful write clears authorship: a failed attempt leaves the entry + // under the temporary name, and a retry must still be able to re-key it. + this.wroteUnderTemporaryViewStateId = false + }) + + ClineProvider.persistedViewStateWriteQueue = write.catch(() => {}) + await write + } + + /** + * Registers this provider's stable view identifier and loads any persisted selections it owns. + * The identifier is sanitized so it remains a safe object key in the shared viewStates map. + * + * `presentedClaim` is the host-minted token this webview was given for that id. It is the + * only proof of ownership: an entry that carries a claim is loaded only when the presented + * token matches it, so a webview cannot adopt another (e.g. disposed, still durable) view's + * id and read that view's pinned profile - and the profile secrets copied into the state + * post - back out of it. The check runs before any re-keying or loading. + */ + public async setViewStateId(viewStateId: string | undefined, presentedClaim?: string): Promise { + // The webview generates this id on purpose - it has to survive a webview reload, + // which recreates the view on both sides - so the host cannot mint it instead. + // What the host owns is which view may hold a given persisted key, so everything + // below validates the supplied value before it becomes one. + if (typeof viewStateId !== "string") { + // A crafted postMessage can carry any JSON value here even though the message + // type declares a string; the persisted key must stay a string. + this.log(`[setViewStateId] Ignoring a non-string viewStateId for view ${this.viewId}.`) + return + } + + const normalizedViewStateId = viewStateId.trim().replace(/[^A-Za-z0-9_-]/g, "_") + + if ( + !normalizedViewStateId || + normalizedViewStateId === this.viewStateId || + // Reject "__proto__" and any other prototype-chain key. The map is copied + // onto a null prototype, so these names can no longer resolve through the + // chain, but they are still rejected as ids so a caller cannot store an + // entry under a name that looks like an inherited member. + normalizedViewStateId === "__proto__" + ) { + return + } + + // Bound the key: the webview generates a UUID, so a much longer value is not a + // view id, and an unbounded key is unbounded growth in the shared viewStates map. + if (normalizedViewStateId.length > ClineProvider.MAX_VIEW_STATE_ID_LENGTH) { + this.log( + `[setViewStateId] Ignoring an oversized viewStateId (${normalizedViewStateId.length} chars) for view ${this.viewId}.`, + ) + return + } + + const claimant = ClineProvider.getAllInstances().find( + (instance) => instance !== this && instance.viewStateId === normalizedViewStateId, + ) + if (claimant) { + // Ownership check: a key another live view holds must not be adopted. Doing so + // would load that view's pinned profile, nested overlay, and mode into this + // provider, and this webview could then overwrite that view's persisted entry. + // Staying on the temporary id keeps this view working against the shared state. + this.log( + `[setViewStateId] Refusing viewStateId ${normalizedViewStateId} for view ${this.viewId}: already claimed by live view ${claimant.viewId}.`, + ) + return + } + + // Claim binding, checked before any re-key or load so a refused registration can + // neither read nor mutate a persisted entry. Read fresh from storage rather than + // through the ContextProxy cache: a security check must not trust a stale copy. + const persistedEntry = this.getPersistedViewStates({ fresh: true })[normalizedViewStateId] + + if (persistedEntry) { + const storedClaim = persistedEntry.claimToken + + if (typeof storedClaim !== "string" || !storedClaim) { + // A pre-claim entry cannot be attributed to any webview, so its selections are + // never loaded. Re-establish the binding with a fresh host-minted claim and hand + // that claim to this webview: the key becomes adoptable again (no upgrade brick) + // and the unbound selections are gone, so the new token cannot resurrect them. + const reboundClaim = await this.rebindUnboundViewStateEntry(normalizedViewStateId) + + if (reboundClaim === undefined) { + // A concurrent registration bound the key while the re-bind was queued: this + // launch adopts a key another webview now owns, so refuse it instead of + // handing out a claim that is not ours. + this.log( + `[setViewStateId] Refusing viewStateId ${normalizedViewStateId} for view ${this.viewId}: bound concurrently by another registration.`, + ) + return + } + + this.postViewStateClaimToWebview(normalizedViewStateId, reboundClaim) + this.log( + `[setViewStateId] Re-bound claim-less viewStates entry ${normalizedViewStateId} for view ${this.viewId}: unbound selections were not loaded.`, + ) + } else if (typeof presentedClaim !== "string" || presentedClaim !== storedClaim) { + // Fail closed: the entry belongs to a webview that never presented this view's + // claim. Rejection is side-effect free, so presenting an id cannot probe, mutate + // or delete another view's entry. + this.log( + `[setViewStateId] Refusing viewStateId ${normalizedViewStateId} for view ${this.viewId}: claim missing or mismatched.`, + ) + return + } + } + + const previousViewStateId = this.viewStateId + + this.viewStateId = normalizedViewStateId + + try { + // Re-key any durable entry written under the temporary pre-launch id before + // loading, so the load sees the view's own pre-registration selections. + await this.rekeyPersistedViewStateEntry(this.viewStateId) + + await this.loadViewState() + + // Re-affirm the binding for the id just adopted: a claim minted while this view was + // still on its temporary id now governs this key (the entry was re-keyed), so the + // webview is told the canonical (viewStateId, claim) pair to echo on its next launch. + const adoptedClaim = this.getPersistedViewStates({ fresh: true })[this.viewStateId]?.claimToken + + if (adoptedClaim) { + this.postViewStateClaimToWebview(this.viewStateId, adoptedClaim) + } + } catch (error) { + // A persistence failure must not leave the provider holding an id that was + // never registered: restore the previous id so a later launch retries the + // registration and the load instead of the guard above early-returning for + // the failed id. + this.viewStateId = previousViewStateId + throw error + } + } + + /** + * Loads non-secret persisted selections from the registered viewStates map. + * Missing entries are intentionally left unset so getState() falls back to shared ContextProxy values. + * Fields mutated while the async profile lookup is in flight are reapplied on top of the + * loaded state, field by field, so in-flight user selections are not clobbered by the load. + */ + private async loadViewState(): Promise { + // Capture the id this load is for: a newer id registered while an async + // profile lookup is in flight must not be overwritten by this stale load. + const loadedForViewId = this.viewStateId + + // While the stable id has not been registered yet, the key is this instance's + // session-local name. Temporary names restart from 0 on every host start, so an + // entry under the same name can belong to a different session. Load only entries + // this instance authored. + if (loadedForViewId === this.viewId && !this.wroteUnderTemporaryViewStateId) { + return + } + try { + const persisted = this.getPersistedViewStates()[loadedForViewId] + const loadedState: Partial = {} + + // Snapshot the in-memory buffer before the async profile lookup. The + // mutation paths update viewLocalState in place, so a shallow copy is + // what makes fields mutated during the load window observable below. + const preLoadBuffer = { ...this.viewLocalState } + + if (persisted?.mode) { + // A persisted mode may reference a custom mode that was deleted after it was + // pinned: restore it only when the slug still resolves, so a stale slug cannot + // shadow the shared mode from getState(). + const customModes = await this.customModesManager.getCustomModes() + if (getModeBySlug(persisted.mode, customModes)) { + loadedState.mode = persisted.mode as Mode + } else { + this.log(`[loadViewState] Ignoring unknown persisted mode "${persisted.mode}"`) + } + } + + if (persisted?.currentApiConfigName) { + loadedState.currentApiConfigName = persisted.currentApiConfigName + + try { + const { name: _name, ...apiConfiguration } = await this.providerSettingsManager.getProfile({ + name: persisted.currentApiConfigName, + }) + loadedState.apiConfiguration = apiConfiguration as ProviderSettings + } catch (error) { + this.log( + `[loadViewState] Unable to resolve API profile '${persisted.currentApiConfigName}' for viewId ${this.viewId}: ${error instanceof Error ? error.message : String(error)}`, + ) + } + } + + if (this.viewStateId !== loadedForViewId) { + this.log(`[loadViewState] Discarding stale state for superseded view id ${loadedForViewId}`) + return + } + + // Reapply the buffer fields that changed while the load was in flight, + // tracking the change instead of testing against undefined: a field cleared + // during the load window must stay cleared (the loaded value must not + // resurrect it), and a field written to a new value must win over it. + // Untouched fields keep the persisted values authoritative, and the + // pre-load buffer is never merged wholesale so stale temporary-id state + // cannot override the stable persisted state. + const postLoadBuffer = this.viewLocalState + const mergedState: Partial = { ...loadedState } + + if (!Object.is(preLoadBuffer.mode, postLoadBuffer.mode)) { + if (postLoadBuffer.mode === undefined) { + delete mergedState.mode + } else { + mergedState.mode = postLoadBuffer.mode + } + } + + if (!Object.is(preLoadBuffer.currentApiConfigName, postLoadBuffer.currentApiConfigName)) { + if (postLoadBuffer.currentApiConfigName === undefined) { + delete mergedState.currentApiConfigName + } else { + mergedState.currentApiConfigName = postLoadBuffer.currentApiConfigName + } + } + + if (!Object.is(preLoadBuffer.apiConfiguration, postLoadBuffer.apiConfiguration)) { + if (postLoadBuffer.apiConfiguration === undefined) { + delete mergedState.apiConfiguration + } else { + mergedState.apiConfiguration = postLoadBuffer.apiConfiguration + } + } + + this.viewLocalState = mergedState + this.log(`[loadViewState] Loaded state for viewId ${this.viewId}`) + } catch (error) { + this.log( + `[loadViewState] Error loading state for viewId ${this.viewId}: ${error instanceof Error ? error.message : String(error)}`, + ) + } + } + + /** + * Saves a single view-local state value. The in-memory buffer is always updated; the + * non-secret subset (mode, currentApiConfigName) is persisted durably under the view + * id active when the change was made, re-keyed to the stable id on registration. + */ + public async saveViewState( + key: K, + value: ViewLocalStateValues[K] | undefined, + ): Promise { + await this._saveViewLocalStateFromMutation({ [key]: value } as ViewLocalStateValues) + + this.log(`[saveViewState] Saved ${String(key)} for viewId ${this.viewId}`) + } + /** * Override EventEmitter's on method to match TaskProviderLike interface */ @@ -847,7 +1361,6 @@ export class ClineProvider ) } } - async dispose() { if (this._disposed) { return @@ -1232,7 +1745,6 @@ export class ClineProvider ) return run } - private async createTaskWithHistoryItemUnlocked( historyItem: HistoryItem & { rootTask?: Task; parentTask?: Task }, options?: { startTask?: boolean }, @@ -1469,7 +1981,6 @@ export class ClineProvider return task } - public async postMessageToWebview(message: ExtensionMessage) { if (this._disposed) { return @@ -1725,7 +2236,6 @@ export class ClineProvider this.handleModeSwitchUnlocked(newMode, targetTask, signal), ) } - private async handleModeSwitchUnlocked( newMode: Mode, targetTask: Task | null | undefined, @@ -2033,9 +2543,17 @@ export class ClineProvider // `allSettled` so a caller that rolls back after a failure never races a write still in flight. const activationResults = await Promise.allSettled([ this.updateGlobalState("listApiConfigMeta", await this.providerSettingsManager.listConfig()), - this.updateGlobalState("currentApiConfigName", name), + // Route through setValue so the in-memory viewLocalState buffer tracks the + // activated profile: a plain global write would leave a stale loaded + // currentApiConfigName shadowing the new value in getValues(). + this.setValue("currentApiConfigName", name), this.providerSettingsManager.setModeConfig(mode, id), this.contextProxy.setProviderSettings(providerSettings), + // setProviderSettings writes the shared store directly, bypassing the + // view-local mutation path: also refresh this view's buffer so a stale + // loaded apiConfiguration cannot keep shadowing the new settings in + // getState(). + this._saveViewLocalStateFromMutation({ apiConfiguration: providerSettings }), ]) const failedActivation = activationResults.find( (result): result is PromiseRejectedResult => result.status === "rejected", @@ -2045,6 +2563,10 @@ export class ClineProvider if (signal.aborted) return id + // Other live views may have buffered this profile's settings earlier; + // refresh them so their getState() cannot report the updated profile's + // name with stale settings. + // Change the provider for the current task. // TODO: We should rename `buildApiHandler` for clarity (e.g. `getProviderClient`). this.updateTaskApiHandlerIfNeeded(providerSettings, { forceRebuild: true }) @@ -2053,6 +2575,21 @@ export class ClineProvider await this.persistStickyProviderProfileToCurrentTask(name) } else { await this.updateGlobalState("listApiConfigMeta", await this.providerSettingsManager.listConfig()) + + // A save that does not activate still changes the profile's settings, and + // every view pinned to it buffers a full copy of them. Refresh those views + // (and this one when it is the pinned view) or their getState() keeps + // serving the settings captured before the save under the same name. + if (this.pinnedProfileName === name) { + await this._saveViewLocalStateFromMutation({ apiConfiguration: providerSettings }) + // Only rebuild when the task actually runs under the saved profile: a + // view pinned to this profile can hold a task that follows a different + // sticky profile, and the forced rebuild would silently switch that + // conversation to the saved profile's settings. + if (this.getCurrentTask()?.taskApiConfigName === name) { + this.updateTaskApiHandlerIfNeeded(providerSettings, { forceRebuild: true }) + } + } } if (signal.aborted) return id @@ -2060,7 +2597,6 @@ export class ClineProvider await this.postStateToWebview() return id } - async deleteProviderProfile(profileToDelete: ProviderSettingsEntry) { const globalSettings = this.contextProxy.getValues() let profileToActivate: string | undefined = globalSettings.currentApiConfigName @@ -2126,7 +2662,6 @@ export class ClineProvider this.activateProviderProfileUnlocked(args, options, signal), ) } - private async activateProviderProfileUnlocked( args: { name: string } | { id: string }, options?: { @@ -2249,9 +2784,6 @@ export class ClineProvider await this.upsertProviderProfile(currentApiConfigName, newConfiguration) } - - // Zoo Code Auth - async handleZooCodeCallback(token: string) { // Auth mutation (token storage, subscription check, success toast) was already // performed by handleAuthCallback() in handleUri.ts before this method was called. @@ -3301,7 +3833,6 @@ export class ClineProvider private getGlobalState(key: K) { return this.contextProxy.getValue(key) } - public async setValue(key: K, value: RooCodeSettings[K]) { await this.contextProxy.setValue(key, value) } @@ -3309,17 +3840,53 @@ export class ClineProvider public getValue(key: K) { return this.contextProxy.getValue(key) } - public getValues() { return this.contextProxy.getValues() } - public async setValues(values: RooCodeSettings) { await this.contextProxy.setValues(values) } - // dev + /** + * Persists the view-local subset of a ContextProxy mutation, then updates the in-memory + * viewLocalState buffer. Persistence is awaited first so a failed durable write cannot + * leave the local cache ahead of the persisted state. + */ + private async _saveViewLocalStateFromMutation( + values: Partial & Partial, + ): Promise { + await this._persistViewLocalStateFromMutation(values) + this._updateViewLocalStateFromMutation(values) + } + + /** + * Writes the durably persisted subset of a mutation (mode and currentApiConfigName) + * into the registered viewStates map for this view. + */ + private async _persistViewLocalStateFromMutation( + values: Partial & Partial, + ): Promise { + const persistedValues: Partial = {} + if ("mode" in values) { + persistedValues.mode = values.mode as PersistedViewState["mode"] + } + + if ("currentApiConfigName" in values) { + persistedValues.currentApiConfigName = values.currentApiConfigName + } + + if ("mode" in persistedValues || "currentApiConfigName" in persistedValues) { + await this.savePersistedViewState(persistedValues) + } + } + + /** + * Clear view-local state cache so that getState() falls back to ContextProxy defaults. + */ + private _clearViewLocalState(): void { + this.viewLocalState = {} + } async resetState() { const answer = await vscode.window.showInformationMessage( t("common:confirmation.reset_state"), @@ -4651,4 +5218,54 @@ export class ClineProvider return vscode.Uri.file(filePath).toString() } } + + /** + * Update or invalidate viewLocalState when ContextProxy is mutated via setValues, setValue, + * profile upsert/activation/deletion, or resetState. This ensures the local cache stays in + * sync with global state changes that would otherwise be invisible behind mergedStateValues. + */ + private _updateViewLocalStateFromMutation(values: Partial & Partial): void { + if ("mode" in values) { + const val = values.mode + if (val === undefined || val === null) { + delete this.viewLocalState.mode + } else { + this.viewLocalState.mode = val + } + } + + if ("currentApiConfigName" in values) { + const val = values.currentApiConfigName + if (val === undefined || val === null) { + delete this.viewLocalState.currentApiConfigName + } else { + this.viewLocalState.currentApiConfigName = val + } + } + + if ("apiConfiguration" in values) { + const val = values.apiConfiguration + if (val === undefined || val === null) { + delete this.viewLocalState.apiConfiguration + } else { + this.viewLocalState.apiConfiguration = val + } + } else if (PROVIDER_SETTINGS_KEYS.some((key) => key in values)) { + const providerSettingsUpdate = PROVIDER_SETTINGS_KEYS.reduce((acc, key) => { + if (key in values) { + return { ...acc, [key]: values[key as keyof RooCodeSettings] } + } + + return acc + }, {} as ProviderSettings) + + this.viewLocalState.apiConfiguration = + "apiProvider" in providerSettingsUpdate + ? providerSettingsUpdate + : { + ...(this.viewLocalState.apiConfiguration ?? {}), + ...providerSettingsUpdate, + } + } + } } diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 93543435e2..478cc76393 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -14,6 +14,7 @@ import { type ClineMessage, type ExtensionMessage, type ExtensionState, + type RooCodeSettings, type WebviewMessage, ORGANIZATION_ALLOW_ALL, DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, @@ -28,12 +29,15 @@ import { defaultModeSlug } from "../../../shared/modes" import { experimentDefault } from "../../../shared/experiments" import { setTtsEnabled } from "../../../utils/tts" import { ContextProxy } from "../../config/ContextProxy" +import { ProviderSettingsNotFoundError } from "../../config/ProviderSettingsManager" import { WorkspaceIndexingEnablementManager } from "../../../services/code-index/workspace-indexing-enablement-manager" import { Task, TaskOptions } from "../../task/Task" import { safeWriteJson } from "../../../utils/safeWriteJson" +import { t } from "../../../i18n" import { ClineProvider } from "../ClineProvider" import { webviewMessageHandler } from "../webviewMessageHandler" +import type { MdmService } from "../../../services/mdm/MdmService" import { Terminal } from "../../../integrations/terminal/Terminal" import { MessageManager } from "../../message-manager" import { forceFullModelDetailsLoad, hasLoadedFullDetails } from "../../../api/providers/fetchers/lmstudio" @@ -419,6 +423,52 @@ afterAll(() => { vi.restoreAllMocks() }) +/** + * Minimal profile shape the stalled getProfile double resolves to. The + * settings fields are optional so a lookup that resolves name-only (a + * profile with no configured provider) also type-checks. + */ +type StalledProfile = { + name: string + apiProvider?: string + openRouterModelId?: string +} + +/** + * Swap the provider's ProviderSettingsManager for a double whose getProfile + * stalls until the test resolves it, so tests can mutate view state while + * loadViewState is in flight. The double only backs getProfile, the member + * loadViewState awaits; the documented @ts-ignore replaces the per-test + * suppressions the inlined copies used. + */ +function stallProviderSettingsProfile(provider: ClineProvider) { + let resolveProfile: (value: StalledProfile) => void = () => {} + const getProfileSpy = vi.fn(() => { + // Only one lookup is resolvable: resolveProfile is bound to the first + // promise's resolver, so a second getProfile would strand its promise. + // Fail loudly instead of hanging. + if (getProfileSpy.mock.calls.length > 1) { + throw new Error( + "stallProviderSettingsProfile: getProfile called more than once; only one lookup is resolvable", + ) + } + return new Promise((resolve) => (resolveProfile = resolve)) + }) + // @ts-ignore - Reassign the readonly providerSettingsManager for the test; the double only backs getProfile. + provider.providerSettingsManager = { getProfile: getProfileSpy } + // Return a stable wrapper around the closure binding: resolveProfile is + // reassigned to the pending promise's resolver once getProfile is called, + // so returning the variable directly would hand the test the initial no-op. + return { getProfile: getProfileSpy, resolveProfile: (value: StalledProfile) => resolveProfile(value) } +} + +/** + * The host-minted claim stored with a durable view-states entry. A test that adopts an + * entry another provider wrote has to present it, exactly like a webview does on launch. + */ +function persistedViewStateClaim(context: vscode.ExtensionContext, viewStateId: string): string | undefined { + return context.globalState.get("viewStates")?.[viewStateId]?.claimToken +} describe("ClineProvider", () => { beforeAll(() => { vi.mocked(Task).mockImplementation(function (options: any) { From 7906df6ccda71c01620ed7d46a870ed317f79024 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 29 Oct 2026 20:58:44 +0800 Subject: [PATCH 02/25] test(webview): carry the identity and durable viewStates tests with N2 N2 spec now includes the N1 identity tests plus the persistence/load/prune/rekey/export-import coverage; the setValues buffer test moves to the buffer unit. --- .../webview/__tests__/ClineProvider.spec.ts | 833 +++++++++++++++++- 1 file changed, 810 insertions(+), 23 deletions(-) diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 67336deb47..4bb3e49635 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -1,4 +1,4 @@ -// pnpm --filter roo-cline test core/webview/__tests__/ClineProvider.spec.ts +// pnpm --filter roo-cline test core/webview/__tests__/ClineProvider.spec.ts import fs from "fs" import * as path from "path" @@ -29,15 +29,12 @@ import { defaultModeSlug } from "../../../shared/modes" import { experimentDefault } from "../../../shared/experiments" import { setTtsEnabled } from "../../../utils/tts" import { ContextProxy } from "../../config/ContextProxy" -import { ProviderSettingsNotFoundError } from "../../config/ProviderSettingsManager" import { WorkspaceIndexingEnablementManager } from "../../../services/code-index/workspace-indexing-enablement-manager" import { Task, TaskOptions } from "../../task/Task" import { safeWriteJson } from "../../../utils/safeWriteJson" -import { t } from "../../../i18n" import { ClineProvider } from "../ClineProvider" import { webviewMessageHandler } from "../webviewMessageHandler" -import type { MdmService } from "../../../services/mdm/MdmService" import { Terminal } from "../../../integrations/terminal/Terminal" import { MessageManager } from "../../message-manager" import { forceFullModelDetailsLoad, hasLoadedFullDetails } from "../../../api/providers/fetchers/lmstudio" @@ -423,24 +420,12 @@ afterAll(() => { vi.restoreAllMocks() }) -/** - * Minimal profile shape the stalled getProfile double resolves to. The - * settings fields are optional so a lookup that resolves name-only (a - * profile with no configured provider) also type-checks. - */ type StalledProfile = { name: string apiProvider?: string openRouterModelId?: string } -/** - * Swap the provider's ProviderSettingsManager for a double whose getProfile - * stalls until the test resolves it, so tests can mutate view state while - * loadViewState is in flight. The double only backs getProfile, the member - * loadViewState awaits; the documented @ts-ignore replaces the per-test - * suppressions the inlined copies used. - */ function stallProviderSettingsProfile(provider: ClineProvider) { let resolveProfile: (value: StalledProfile) => void = () => {} const getProfileSpy = vi.fn(() => { @@ -462,13 +447,6 @@ function stallProviderSettingsProfile(provider: ClineProvider) { return { getProfile: getProfileSpy, resolveProfile: (value: StalledProfile) => resolveProfile(value) } } -/** - * The host-minted claim stored with a durable view-states entry. A test that adopts an - * entry another provider wrote has to present it, exactly like a webview does on launch. - */ -function persistedViewStateClaim(context: vscode.ExtensionContext, viewStateId: string): string | undefined { - return context.globalState.get("viewStates")?.[viewStateId]?.claimToken -} describe("ClineProvider", () => { beforeAll(() => { vi.mocked(Task).mockImplementation(function (options: any) { @@ -3628,6 +3606,815 @@ describe("ClineProvider", () => { ]) }) }) + + describe("setViewStateId", () => { + it('should ignore "__proto__" and keep the temporary viewId', async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + + await provider["setViewStateId"]("__proto__") + + // "__proto__" is rejected before assignment so a per-view entry can never be + // keyed through the Object.prototype setter: the temporary id stays active and + // nothing is persisted under the reserved name. + expect(provider["viewStateId"]).toBe(provider.viewId) + expect(mockContext.globalState.get("viewStates")).toBeUndefined() + expect(provider["viewLocalState"]).toEqual({}) + + await provider.dispose() + }) + + it("restores the previous view id when the registration write fails so a later launch retries", async () => { + const contextProxy = new ContextProxy(mockContext) + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + contextProxy, + new WebviewFocusTracker(), + ) + // Seed a pre-launch entry under the temporary id through the real write path, + // so the re-key has real work to do and records this instance as its author. + await provider["savePersistedViewState"]({ mode: "architect", updatedAt: 1 }) + + const setValueSpy = vi.spyOn(contextProxy, "setValue").mockRejectedValue(new Error("storage down")) + + await expect(provider["setViewStateId"]("stable-sidebar-view")).rejects.toThrow("storage down") + + // The failed id must not stick: the provider keeps its previous (temporary) + // id so a later launch retries registration and the load instead of the + // guard early-returning for an id that was never persisted. + expect(provider["viewStateId"]).toBe(provider.viewId) + + // A later retry succeeds once the storage write works again, and the + // pre-launch entry lands under the registered id. + setValueSpy.mockRestore() + await provider["setViewStateId"]("stable-sidebar-view") + expect(provider["viewStateId"]).toBe("stable-sidebar-view") + expect(mockContext.globalState.get("viewStates")).toEqual({ + "stable-sidebar-view": { mode: "architect", updatedAt: 1, claimToken: expect.any(String) }, + }) + + await provider.dispose() + }) + }) + + describe("view state claim binding", () => { + it("refuses a viewStateId bound by a concurrent registration through setViewStateId", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const beforeViewStateId = provider["viewStateId"] + const postSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(undefined) + // A claim-less entry is adoptable at the first read, but a concurrent registration + // binds the key while this launch's queued re-bind is waiting its turn. + await provider.contextProxy.setValue("viewStates", { "legacy-view": { updatedAt: 1 } }) + ClineProvider["persistedViewStateWriteQueue"] = Promise.resolve().then(async () => { + await provider.contextProxy.setValue("viewStates", { + "legacy-view": { claimToken: "winner", updatedAt: 2 }, + }) + }) + + await provider["setViewStateId"]("legacy-view") + + // The launch must not adopt a key another webview now owns: no claim is delivered, + // the provider stays on its temporary id, and the winner's claim is untouched. + expect(postSpy).not.toHaveBeenCalledWith(expect.objectContaining({ type: "viewStateClaim" })) + expect(provider["viewStateId"]).toBe(beforeViewStateId) + const states = provider.contextProxy.getValue("viewStates") as Record + expect(states["legacy-view"].claimToken).toBe("winner") + ClineProvider["persistedViewStateWriteQueue"] = Promise.resolve() + postSpy.mockRestore() + await provider.dispose() + }) + + it("does not overwrite a claim bound by a concurrent registration", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + await provider.contextProxy.setValue("viewStates", { "legacy-view": { updatedAt: 1 } }) + // A concurrent registration binds this key while the re-bind write is queued. + ClineProvider["persistedViewStateWriteQueue"] = Promise.resolve().then(async () => { + await provider.contextProxy.setValue("viewStates", { + "legacy-view": { claimToken: "winner", updatedAt: 2 }, + }) + }) + + const claim = await provider["rebindUnboundViewStateEntry"]("legacy-view") + + // The queued re-bind must not replace the claim the other registration wrote; it + // reports that nothing was rebound so the caller can refuse the stale adoption. + expect(claim).toBeUndefined() + const states = provider.contextProxy.getValue("viewStates") as Record + expect(states["legacy-view"].claimToken).toBe("winner") + ClineProvider["persistedViewStateWriteQueue"] = Promise.resolve() + await provider.dispose() + }) + + const createProvider = () => + new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + + const storedEntries = () => + mockContext.globalState.get>("viewStates") ?? {} + + test("refuses a disposed view's id when no claim is presented, and loads nothing", async () => { + // The vulnerable path: a view registers a durable entry, is disposed, and a second + // webview presents the same id. dispose() removes the instance but leaves the entry, + // so only the claim can stop the second webview from loading the first one's pinned + // profile (and the profile's secrets) through the state post. + const owner = createProvider() + await owner["setViewStateId"]("closed-view") + await owner.saveViewState("mode", "architect") + await owner.saveViewState("currentApiConfigName", "profile-with-secrets") + + const storedClaim = storedEntries()["closed-view"]?.claimToken + expect(storedClaim).toEqual(expect.any(String)) + + await owner.dispose() + + const intruder = createProvider() + const postMessageSpy = vi.spyOn(intruder, "postMessageToWebview").mockResolvedValue(undefined) + + await intruder["setViewStateId"]("closed-view") + + // Fail closed: the id is not adopted, no selections reach this view and no state is + // posted back to the webview that asked for it. + expect(intruder["viewStateId"]).toBe(intruder.viewId) + expect(intruder["viewLocalState"]).toStrictEqual({}) + expect((await intruder.getState()).currentApiConfigName).not.toBe("profile-with-secrets") + expect(postMessageSpy).not.toHaveBeenCalled() + + // A refused adoption is side-effect free: presenting an id must not probe or rewrite + // the entry that a different webview owns. + expect(storedEntries()["closed-view"]).toMatchObject({ + mode: "architect", + currentApiConfigName: "profile-with-secrets", + claimToken: storedClaim, + }) + + await intruder.dispose() + }) + + test("refuses a mismatched claim", async () => { + const owner = createProvider() + await owner["setViewStateId"]("closed-view") + await owner.saveViewState("currentApiConfigName", "profile-with-secrets") + await owner.dispose() + + const intruder = createProvider() + const postMessageSpy = vi.spyOn(intruder, "postMessageToWebview").mockResolvedValue(undefined) + + // A webview-generated value is not a claim: only the token the host minted matches. + await intruder["setViewStateId"]("closed-view", "webview-chosen-claim") + + expect(intruder["viewStateId"]).toBe(intruder.viewId) + expect(intruder["viewLocalState"]).toStrictEqual({}) + expect(postMessageSpy).not.toHaveBeenCalled() + + await intruder.dispose() + }) + + test("does not churn the claim on later writes to the same entry", async () => { + const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(undefined) + await provider["setViewStateId"]("minted-view") + await provider.saveViewState("mode", "architect") + + const mintedClaim = storedEntries()["minted-view"]?.claimToken + expect(mintedClaim).toEqual(expect.any(String)) + + await provider.saveViewState("currentApiConfigName", "profile-b") + + // A merge into an entry this view already owns keeps its token, so the claim the + // webview holds keeps working and is not re-sent. + expect(storedEntries()["minted-view"]?.claimToken).toBe(mintedClaim) + expect( + postMessageSpy.mock.calls.map(([message]) => message).filter((m) => m.type === "viewStateClaim"), + ).toHaveLength(1) + }) + + test("re-binds a claim-less legacy entry instead of loading its selections", async () => { + // An entry written before claims existed cannot be attributed to any webview: its + // selections are refused, and the key is re-bound with a fresh host-minted claim so + // the upgrade does not brick per-view persistence. + await provider.contextProxy.setValue("viewStates", { + "legacy-view": { mode: "architect", currentApiConfigName: "profile-a", updatedAt: 1 }, + }) + + const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(undefined) + + await provider["setViewStateId"]("legacy-view") + + // No stored selection is adopted... + expect(provider["viewStateId"]).toBe("legacy-view") + expect(provider["viewLocalState"]).toStrictEqual({}) + + // ...the old entry is replaced by a claim-bearing one with the unbound selections gone... + const rebound = storedEntries()["legacy-view"] + expect(rebound).toEqual({ claimToken: expect.any(String), updatedAt: expect.any(Number) }) + expect(rebound).not.toHaveProperty("mode") + expect(rebound).not.toHaveProperty("currentApiConfigName") + + // ...and the fresh claim is handed to this webview so the next launch can prove ownership. + expect(postMessageSpy).toHaveBeenCalledWith({ + type: "viewStateClaim", + viewStateId: "legacy-view", + viewStateClaim: rebound?.claimToken, + }) + + await provider.dispose() + + const reopened = createProvider() + await reopened["setViewStateId"]("legacy-view", rebound?.claimToken) + + expect(reopened["viewStateId"]).toBe("legacy-view") + expect(reopened["viewLocalState"]).toStrictEqual({}) + + await reopened.dispose() + }) + + test("still refuses an id a live view holds, even when the claim matches", async () => { + const owner = createProvider() + await owner["setViewStateId"]("shared-live-view") + await owner.saveViewState("mode", "architect") + const claim = storedEntries()["shared-live-view"]?.claimToken + + const sibling = createProvider() + + try { + await sibling["setViewStateId"]("shared-live-view", claim) + + // The live-instance guard runs first and keeps working: a second live view never + // shares a key with the view that holds it. + expect(sibling["viewStateId"]).toBe(sibling.viewId) + expect(sibling["viewLocalState"]).toStrictEqual({}) + expect(owner["viewStateId"]).toBe("shared-live-view") + } finally { + await sibling.dispose() + await owner.dispose() + } + }) + }) + + describe("view state persistence edge cases", () => { + it("should read viewStates from the ContextProxy cache when not fresh", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + await provider.contextProxy.setValue("viewStates", { "stable-sidebar-view": { mode: "architect" } }) + expect(provider["getPersistedViewStates"]()).toEqual({ "stable-sidebar-view": { mode: "architect" } }) + await provider.dispose() + }) + + it("should treat a corrupted non-object viewStates value as an empty map", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + // A string in storage is corrupt: the fresh-read guard must not spread it. + await mockContext.globalState.update("viewStates", "corrupted-storage-value") + expect(provider["getPersistedViewStates"]({ fresh: true })).toEqual({}) + await provider.dispose() + }) + + it("should merge saved fields, drop cleared fields and delete emptied entries", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const logSpy = vi.spyOn(provider, "log") + const save = provider.saveViewState.bind(provider) as (key: string, value: unknown) => Promise + const states = () => mockContext.globalState.get>("viewStates") ?? {} + await provider["setViewStateId"]("stable-sidebar-view") + await provider.saveViewState("mode", "architect") + await provider.saveViewState("currentApiConfigName", "profile-a") + const merged = states()["stable-sidebar-view"] + expect(merged).toMatchObject({ mode: "architect", currentApiConfigName: "profile-a" }) + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("Saved mode for viewId")) + await save("mode", undefined) + expect(states()["stable-sidebar-view"]).toStrictEqual({ + currentApiConfigName: "profile-a", + updatedAt: expect.any(Number), + claimToken: expect.any(String), + }) + await save("mode", null) + expect(states()["stable-sidebar-view"]).not.toHaveProperty("mode") + await provider.saveViewState("mode", "architect") + await save("currentApiConfigName", undefined) + expect(states()["stable-sidebar-view"]).toStrictEqual({ + mode: "architect", + updatedAt: expect.any(Number), + claimToken: expect.any(String), + }) + await provider.saveViewState("currentApiConfigName", "profile-c") + await provider.saveViewState("mode", "architect") + expect(states()["stable-sidebar-view"]).toMatchObject({ + mode: "architect", + currentApiConfigName: "profile-c", + }) + await save("currentApiConfigName", null) + expect(states()["stable-sidebar-view"]).not.toHaveProperty("currentApiConfigName") + await save("mode", null) + expect(states()["stable-sidebar-view"]).toBeUndefined() + expect(provider["viewLocalState"]).toStrictEqual({}) // buffer ends fully cleared + await provider.dispose() + }) + + it("should rekey a pre-launch entry under the temporary id to the registered stable id", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + // Seed through the write path so this instance is the author of the entry under + // its temporary id; an entry authored by another session must not be adopted. + await provider["savePersistedViewState"]({ mode: "architect", updatedAt: 1 }) + await provider["setViewStateId"]("stable-sidebar-view") + expect(mockContext.globalState.get("viewStates")).toEqual({ + // The minted claim travels with the re-keyed entry, so the webview's token keeps working. + "stable-sidebar-view": { mode: "architect", updatedAt: 1, claimToken: expect.any(String) }, + }) + await provider.dispose() + }) + + it("should keep the stable entry and drop the temporary entry when both exist", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + // Author the temporary entry through the write path so this instance owns it, + // then add a stable entry that already exists in storage. + await provider["savePersistedViewState"]({ mode: "temp-mode", updatedAt: 1 }) + await provider.contextProxy.setValue("viewStates", { + [provider.viewId]: { mode: "temp-mode", updatedAt: 1 }, + "stable-sidebar-view": { mode: "stable-mode", updatedAt: 5, claimToken: "stable-claim" }, + }) + await provider["setViewStateId"]("stable-sidebar-view", "stable-claim") + expect(mockContext.globalState.get("viewStates")).toEqual({ + "stable-sidebar-view": { mode: "stable-mode", updatedAt: 5, claimToken: "stable-claim" }, + }) + await provider.dispose() + }) + + it("should prune by updatedAt regardless of insertion order", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const states = Object.fromEntries( + Array.from({ length: 55 }, (_, index) => [ + `view-${index}`, + { mode: `mode-${index}`, updatedAt: (index * 7) % 55 }, + ]), + ) + const pruned = provider["prunePersistedViewStates"](states) + expect(Object.keys(pruned)).toHaveLength(50) + // view-1/view-54 survive the true newest-50 selection; view-8 (updatedAt 1) does not. + expect(pruned["view-1"]).toEqual({ mode: "mode-1", updatedAt: 7 }) + expect(pruned["view-54"]).toEqual({ mode: "mode-54", updatedAt: 48 }) + expect(pruned["view-8"]).toBeUndefined() + await provider.dispose() + }) + + it("should log a successful empty load when no persisted entry exists", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const logSpy = vi.spyOn(provider, "log") + await provider["setViewStateId"]("stable-sidebar-view") + expect(provider["viewLocalState"]).toEqual({}) + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("Loaded state for viewId")) + await provider.dispose() + }) + + it("should keep the persisted profile name and log when the profile lookup fails", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const logSpy = vi.spyOn(provider, "log") + // @ts-ignore - Replace providerSettingsManager with a failing test double. + provider.providerSettingsManager = { getProfile: vi.fn().mockRejectedValue(new Error("profile missing")) } + await provider.saveViewState("currentApiConfigName", "my-profile") + await provider["setViewStateId"]("stable-sidebar-view") + expect(provider["viewLocalState"].currentApiConfigName).toBe("my-profile") + expect(provider["viewLocalState"]).not.toHaveProperty("apiConfiguration") + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("Unable to resolve API profile 'my-profile'")) + await provider.dispose() + }) + + it("should discard a stale load when the viewStateId changes during the profile lookup", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const logSpy = vi.spyOn(provider, "log") + // @ts-ignore - Replace providerSettingsManager with a test double that registers a newer id. + provider.providerSettingsManager = { + getProfile: vi.fn().mockImplementation(() => { + provider["viewStateId"] = "superseded-view" + return Promise.resolve({ name: "my-profile", apiProvider: providerIdentifiers.openrouter }) + }), + } + await provider.saveViewState("currentApiConfigName", "my-profile") + await provider["setViewStateId"]("stable-sidebar-view") + expect(provider["viewLocalState"]).not.toHaveProperty("apiConfiguration") + const staleMsg = expect.stringContaining("Discarding stale state for superseded view id") + expect(logSpy).toHaveBeenCalledWith(staleMsg) + await provider.dispose() + }) + + it("should not restore a persisted mode whose custom mode no longer exists", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const logSpy = vi.spyOn(provider, "log") + // @ts-ignore - Replace customModesManager with a test double (no custom modes). + provider.customModesManager = { getCustomModes: vi.fn().mockResolvedValue([]), dispose: vi.fn() } + // The file-level modes mock resolves every slug to a mode; drop the pinned slug. + const modesModule = vi.mocked(await import("../../../shared/modes")) + const originalMode = modesModule.getModeBySlug("code") + modesModule.getModeBySlug.mockImplementation(((slug: string) => + slug === "deleted-custom-mode" ? undefined : originalMode) as typeof modesModule.getModeBySlug) + try { + await provider["setViewStateId"]("stable-sidebar-view") + await provider.saveViewState("mode", "deleted-custom-mode") + // Reload the persisted entry: the custom mode was deleted in the meantime. + await provider["loadViewState"]() + // The stale slug must not be restored into the buffer. + expect(provider["viewLocalState"]).not.toHaveProperty("mode") + expect(logSpy).toHaveBeenCalledWith( + expect.stringContaining('Ignoring unknown persisted mode "deleted-custom-mode"'), + ) + } finally { + modesModule.getModeBySlug.mockReturnValue(originalMode) + } + await provider.dispose() + }) + + it("should keep every persisted field authoritative when the load is untouched", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const { getProfile: getProfileSpy, resolveProfile } = stallProviderSettingsProfile(provider) + // @ts-ignore - Replace customModesManager with a test double (no custom modes). + provider.customModesManager = { getCustomModes: vi.fn().mockResolvedValue([]), dispose: vi.fn() } + await provider.saveViewState("mode", "code") + await provider.saveViewState("currentApiConfigName", "cfg-a") + const load = provider["setViewStateId"]("stable-sidebar-view") + + // Let the stalled lookup begin so the in-flight mutation and the resolver + // target the pending promise rather than the initial no-op. + await vi.waitFor(() => expect(getProfileSpy).toHaveBeenCalledTimes(1)) + // No mutation while the lookup is in flight: the persisted values must win. + resolveProfile({ name: "cfg-a", apiProvider: providerIdentifiers.openrouter, openRouterModelId: "model-x" }) + await load + + expect(provider["viewLocalState"]).toEqual({ + mode: "code", + currentApiConfigName: "cfg-a", + apiConfiguration: { apiProvider: providerIdentifiers.openrouter, openRouterModelId: "model-x" }, + }) + await provider.dispose() + }) + }) + + describe("saveViewState", () => { + it("should update viewLocalState and persist mode through registered viewStates", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + + const contextProxySpy = vi.spyOn(provider.contextProxy, "setValue") + await provider["setViewStateId"]("stable-sidebar-view") + + await provider.saveViewState("mode", "architect") + + expect(provider["viewLocalState"].mode).toBe("architect") + expect(provider.contextProxy.getValue("viewStates")).toMatchObject({ + "stable-sidebar-view": { mode: "architect" }, + }) + expect(contextProxySpy).toHaveBeenCalledWith( + "viewStates", + expect.objectContaining({ + "stable-sidebar-view": expect.objectContaining({ + mode: "architect", + updatedAt: expect.any(Number), + }), + }), + ) + expect(contextProxySpy).not.toHaveBeenCalledWith("__view_state_stable-sidebar-view_mode", expect.anything()) + + await provider.dispose() + }) + + it("should update viewLocalState and persist currentApiConfigName through registered viewStates", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + + await provider["setViewStateId"]("stable-sidebar-view") + await provider.saveViewState("currentApiConfigName", "my-profile") + + expect(provider["viewLocalState"].currentApiConfigName).toBe("my-profile") + expect(provider.contextProxy.getValue("viewStates")).toMatchObject({ + "stable-sidebar-view": { currentApiConfigName: "my-profile" }, + }) + + await provider.dispose() + }) + + it("should update viewLocalState for apiConfiguration without persisting provider settings or secrets", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + + const testApiConfig = { + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "claude-3.5-sonnet", + openRouterApiKey: "secret-key", + } + + await provider["setViewStateId"]("stable-sidebar-view") + await provider.saveViewState("apiConfiguration", testApiConfig) + + expect(provider["viewLocalState"].apiConfiguration).toEqual(testApiConfig) + expect(provider.contextProxy.getValue("viewStates")).toBeUndefined() + + await provider.dispose() + }) + + it("should clear local override when saveViewState receives undefined", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + + await provider.saveViewState("mode", "architect") + expect(provider["viewLocalState"].mode).toBe("architect") + + await provider.saveViewState("mode", undefined) + + expect(Object.prototype.hasOwnProperty.call(provider["viewLocalState"], "mode")).toBe(false) + + await provider.dispose() + }) + + it("should clear the currentApiConfigName override when saveViewState receives undefined", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + + await provider.saveViewState("currentApiConfigName", "my-profile") + expect(provider["viewLocalState"].currentApiConfigName).toBe("my-profile") + + await provider.saveViewState("currentApiConfigName", undefined) + + expect(Object.prototype.hasOwnProperty.call(provider["viewLocalState"], "currentApiConfigName")).toBe(false) + + await provider.dispose() + }) + + it("should not update viewLocalState when durable view-state persistence fails", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + vi.spyOn(provider.contextProxy, "setValue").mockRejectedValueOnce(new Error("persist failed")) + + await provider["setViewStateId"]("stable-sidebar-view") + + await expect(provider.saveViewState("mode", "architect")).rejects.toThrow("persist failed") + expect(provider["viewLocalState"]).not.toHaveProperty("mode") + expect(provider.contextProxy.getValue("viewStates")).toBeUndefined() + + await provider.dispose() + }) + + it("should merge concurrent persisted updates from separate provider instances without lost viewStates", async () => { + const provider1 = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + + new WebviewFocusTracker(), + ) + const provider2 = new ClineProvider( + mockContext, + mockOutputChannel, + "editor", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + + await provider1["setViewStateId"]("stable-sidebar-view") + await provider2["setViewStateId"]("stable-editor-view") + + await Promise.all([ + provider1.saveViewState("mode", "architect"), + provider2.saveViewState("currentApiConfigName", "editor-profile"), + ]) + + expect(mockContext.globalState.get("viewStates")).toMatchObject({ + "stable-sidebar-view": { mode: "architect" }, + "stable-editor-view": { currentApiConfigName: "editor-profile" }, + }) + + await provider1.dispose() + await provider2.dispose() + }) + }) + + describe("loadViewState", () => { + it("should keep viewLocalState empty when no stable per-view values exist", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + + await vi.waitFor(() => { + expect(provider["viewLocalState"]).toEqual({}) + }) + + const state = await provider.getState() + // No per-view entry exists and the proxy's global-state cache is empty + // (initialize() is never called in this fixture; only "taskHistory" passes + // through to the context store), so getState() falls back to the shared + // defaults: mode "code" (defaultModeSlug) and currentApiConfigName "default". + expect(state.mode).toBe("code") + expect(state.currentApiConfigName).toBe("default") + + await provider.dispose() + }) + + it("should log and keep existing viewLocalState when loadViewState fails", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const logSpy = vi.spyOn(provider, "log") + + // Register a stable id so the load is not skipped by the ownership guard: + // only entries this instance authored are loaded under a temporary name. + await provider["setViewStateId"]("stable-sidebar-view") + + provider["viewLocalState"] = { mode: "architect" } + vi.spyOn(provider.contextProxy, "getValue").mockImplementation(() => { + throw new Error("load failed") + }) + + await provider["loadViewState"]() + + expect(provider["viewLocalState"].mode).toBe("architect") + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("Error loading state")) + + await provider.dispose() + }) + }) + + describe("persisted view state pruning", () => { + it("should keep the newest 50 persisted view states", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const states = Object.fromEntries( + Array.from({ length: 55 }, (_, index) => [ + `view-${index}`, + { mode: `mode-${index}`, updatedAt: index }, + ]), + ) + + const pruned = provider["prunePersistedViewStates"](states) + + expect(Object.keys(pruned)).toHaveLength(50) + expect(pruned["view-54"]).toEqual({ mode: "mode-54", updatedAt: 54 }) + expect(pruned["view-5"]).toEqual({ mode: "mode-5", updatedAt: 5 }) + expect(pruned["view-4"]).toBeUndefined() + + await provider.dispose() + }) + }) + + it("should merge concurrent persisted updates from separate provider instances without lost viewStates", async () => { + const provider1 = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + + new WebviewFocusTracker(), + ) + const provider2 = new ClineProvider( + mockContext, + mockOutputChannel, + "editor", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + + await provider1["setViewStateId"]("stable-sidebar-view") + await provider2["setViewStateId"]("stable-editor-view") + + await Promise.all([ + provider1.saveViewState("mode", "architect"), + provider2.saveViewState("currentApiConfigName", "editor-profile"), + ]) + + expect(mockContext.globalState.get("viewStates")).toMatchObject({ + "stable-sidebar-view": { mode: "architect" }, + "stable-editor-view": { currentApiConfigName: "editor-profile" }, + }) + + await provider1.dispose() + await provider2.dispose() + }) }) describe("webviewMessageHandler no-floating-promises coverage", () => { From f560872c380d475393ba7206076d8025f7b79b6c Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 29 Oct 2026 20:59:01 +0800 Subject: [PATCH 03/25] chore: untrack generated husky hooks --- .husky/_/post-checkout | 3 --- .husky/_/post-commit | 3 --- .husky/_/post-merge | 3 --- .husky/_/pre-push | 3 --- .husky/pre-commit | 26 -------------------------- .husky/pre-push | 42 ------------------------------------------ 6 files changed, 80 deletions(-) delete mode 100644 .husky/_/post-checkout delete mode 100644 .husky/_/post-commit delete mode 100644 .husky/_/post-merge delete mode 100644 .husky/_/pre-push delete mode 100644 .husky/pre-commit delete mode 100644 .husky/pre-push diff --git a/.husky/_/post-checkout b/.husky/_/post-checkout deleted file mode 100644 index 5abf8ed93f..0000000000 --- a/.husky/_/post-checkout +++ /dev/null @@ -1,3 +0,0 @@ -#!/bin/sh -command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'post-checkout' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } -git lfs post-checkout "$@" diff --git a/.husky/_/post-commit b/.husky/_/post-commit deleted file mode 100644 index b8b76c2c42..0000000000 --- a/.husky/_/post-commit +++ /dev/null @@ -1,3 +0,0 @@ -#!/bin/sh -command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'post-commit' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } -git lfs post-commit "$@" diff --git a/.husky/_/post-merge b/.husky/_/post-merge deleted file mode 100644 index 726f909891..0000000000 --- a/.husky/_/post-merge +++ /dev/null @@ -1,3 +0,0 @@ -#!/bin/sh -command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'post-merge' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } -git lfs post-merge "$@" diff --git a/.husky/_/pre-push b/.husky/_/pre-push deleted file mode 100644 index 5f26dc4552..0000000000 --- a/.husky/_/pre-push +++ /dev/null @@ -1,3 +0,0 @@ -#!/bin/sh -command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'pre-push' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } -git lfs pre-push "$@" diff --git a/.husky/pre-commit b/.husky/pre-commit deleted file mode 100644 index f8f420dac7..0000000000 --- a/.husky/pre-commit +++ /dev/null @@ -1,26 +0,0 @@ -branch="$(git rev-parse --abbrev-ref HEAD)" - -if [ "$branch" = "main" ]; then - echo "You can't commit directly to main - please check out a branch." - exit 1 -fi - -# Detect if running on Windows and use pnpm.cmd, otherwise use pnpm. -if [ "$OS" = "Windows_NT" ]; then - pnpm_cmd="pnpm.cmd" -else - if command -v pnpm >/dev/null 2>&1; then - pnpm_cmd="pnpm" - else - pnpm_cmd="npx pnpm" - fi -fi - -# Detect if running on Windows and use npx.cmd, otherwise use npx. -if [ "$OS" = "Windows_NT" ]; then - npx_cmd="npx.cmd" -else - npx_cmd="npx" -fi - -$npx_cmd lint-staged diff --git a/.husky/pre-push b/.husky/pre-push deleted file mode 100644 index 4cf91d9580..0000000000 --- a/.husky/pre-push +++ /dev/null @@ -1,42 +0,0 @@ -branch="$(git rev-parse --abbrev-ref HEAD)" - -if [ "$branch" = "main" ]; then - echo "You can't push directly to main - please check out a branch." - exit 1 -fi - -# Detect if running on Windows and use pnpm.cmd, otherwise use pnpm. -if [ "$OS" = "Windows_NT" ]; then - pnpm_cmd="pnpm.cmd" -else - if command -v pnpm >/dev/null 2>&1; then - pnpm_cmd="pnpm" - else - pnpm_cmd="npx pnpm" - fi -fi - -$pnpm_cmd run check-types - -# Use dotenvx to securely load .env.local and run commands that depend on it -if [ -f ".env.local" ]; then - # Check if RUN_TESTS_ON_PUSH is set to true and run tests with dotenvx - if npx dotenvx get RUN_TESTS_ON_PUSH -f .env.local 2>/dev/null | grep -q "^true$"; then - npx dotenvx run -f .env.local -- $pnpm_cmd run test - fi -else - # Fallback: run tests if RUN_TESTS_ON_PUSH is set in regular environment - if [ "$RUN_TESTS_ON_PUSH" = "true" ]; then - $pnpm_cmd run test - fi -fi - -# Check for new changesets. -NEW_CHANGESETS=$(find .changeset -name "*.md" ! -name "README.md" | wc -l | tr -d ' ') -echo "Changeset files: $NEW_CHANGESETS" - -if [ "$NEW_CHANGESETS" = "0" ]; then - echo "-------------------------------------------------------------------------------------" - echo "Changes detected. Please run 'pnpm changeset' to create a changeset if applicable." - echo "-------------------------------------------------------------------------------------" -fi From 634099884ec54f71d4d54743f5b44d7367b0f528 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 29 Oct 2026 20:59:19 +0800 Subject: [PATCH 04/25] chore: restore main tracked husky hooks --- .husky/pre-commit | 26 ++++++++++++++++++++++++++ .husky/pre-push | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+) create mode 100644 .husky/pre-commit create mode 100644 .husky/pre-push diff --git a/.husky/pre-commit b/.husky/pre-commit new file mode 100644 index 0000000000..f8f420dac7 --- /dev/null +++ b/.husky/pre-commit @@ -0,0 +1,26 @@ +branch="$(git rev-parse --abbrev-ref HEAD)" + +if [ "$branch" = "main" ]; then + echo "You can't commit directly to main - please check out a branch." + exit 1 +fi + +# Detect if running on Windows and use pnpm.cmd, otherwise use pnpm. +if [ "$OS" = "Windows_NT" ]; then + pnpm_cmd="pnpm.cmd" +else + if command -v pnpm >/dev/null 2>&1; then + pnpm_cmd="pnpm" + else + pnpm_cmd="npx pnpm" + fi +fi + +# Detect if running on Windows and use npx.cmd, otherwise use npx. +if [ "$OS" = "Windows_NT" ]; then + npx_cmd="npx.cmd" +else + npx_cmd="npx" +fi + +$npx_cmd lint-staged diff --git a/.husky/pre-push b/.husky/pre-push new file mode 100644 index 0000000000..4cf91d9580 --- /dev/null +++ b/.husky/pre-push @@ -0,0 +1,42 @@ +branch="$(git rev-parse --abbrev-ref HEAD)" + +if [ "$branch" = "main" ]; then + echo "You can't push directly to main - please check out a branch." + exit 1 +fi + +# Detect if running on Windows and use pnpm.cmd, otherwise use pnpm. +if [ "$OS" = "Windows_NT" ]; then + pnpm_cmd="pnpm.cmd" +else + if command -v pnpm >/dev/null 2>&1; then + pnpm_cmd="pnpm" + else + pnpm_cmd="npx pnpm" + fi +fi + +$pnpm_cmd run check-types + +# Use dotenvx to securely load .env.local and run commands that depend on it +if [ -f ".env.local" ]; then + # Check if RUN_TESTS_ON_PUSH is set to true and run tests with dotenvx + if npx dotenvx get RUN_TESTS_ON_PUSH -f .env.local 2>/dev/null | grep -q "^true$"; then + npx dotenvx run -f .env.local -- $pnpm_cmd run test + fi +else + # Fallback: run tests if RUN_TESTS_ON_PUSH is set in regular environment + if [ "$RUN_TESTS_ON_PUSH" = "true" ]; then + $pnpm_cmd run test + fi +fi + +# Check for new changesets. +NEW_CHANGESETS=$(find .changeset -name "*.md" ! -name "README.md" | wc -l | tr -d ' ') +echo "Changeset files: $NEW_CHANGESETS" + +if [ "$NEW_CHANGESETS" = "0" ]; then + echo "-------------------------------------------------------------------------------------" + echo "Changes detected. Please run 'pnpm changeset' to create a changeset if applicable." + echo "-------------------------------------------------------------------------------------" +fi From 98490187ee0429811c7f966482fc7ca3eec92c59 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 29 Oct 2026 21:01:54 +0800 Subject: [PATCH 05/25] feat(webview): view-local buffer reads and setValue persistence (N3) Split unit N3: setValue/getValues view-local buffer and failed-write rollback on top of N2; setValues/reset/ack/history remain for N4/N5. --- src/core/webview/ClineProvider.ts | 28 +++- .../webview/__tests__/ClineProvider.spec.ts | 122 ++++++++++++++++++ 2 files changed, 149 insertions(+), 1 deletion(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 9d812cd8e9..f3d4a0d637 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -3918,15 +3918,26 @@ export class ClineProvider private getGlobalState(key: K) { return this.contextProxy.getValue(key) } + public async setValue(key: K, value: RooCodeSettings[K]) { + const previousSharedValues = { [key]: this.contextProxy.getValue(key) } as RooCodeSettings await this.contextProxy.setValue(key, value) + try { + await this._saveViewLocalStateFromMutation({ [key]: value }) + } catch (error) { + // The per-view record did not land: restore the prior shared value so the two + // persistence layers cannot diverge, then surface the original failure. + await this._restoreSharedValuesAfterFailedViewWrite(previousSharedValues) + throw error + } } public getValue(key: K) { return this.contextProxy.getValue(key) } + public getValues() { - return this.contextProxy.getValues() + return { ...this.contextProxy.getValues(), ...this.viewLocalState } } public async setValues(values: RooCodeSettings) { await this.contextProxy.setValues(values) @@ -5353,4 +5364,19 @@ export class ClineProvider } } } + + /** + * Restore shared values captured before a setter wrote them, used when the paired + * per-view persistence failed so the shared store cannot move ahead of the per-view + * record. A failed rollback is logged; the original failure is what callers see. + */ + private async _restoreSharedValuesAfterFailedViewWrite(previous: RooCodeSettings): Promise { + try { + await this.contextProxy.setValues(previous) + } catch (rollbackError) { + this.log( + `[ClineProvider] Shared-state rollback after failed per-view write failed: ${rollbackError instanceof Error ? rollbackError.message : String(rollbackError)}`, + ) + } + } } diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 4bb3e49635..6356835cc9 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -447,6 +447,10 @@ function stallProviderSettingsProfile(provider: ClineProvider) { return { getProfile: getProfileSpy, resolveProfile: (value: StalledProfile) => resolveProfile(value) } } +function persistedViewStateClaim(context: vscode.ExtensionContext, viewStateId: string): string | undefined { + return context.globalState.get("viewStates")?.[viewStateId]?.claimToken +} + describe("ClineProvider", () => { beforeAll(() => { vi.mocked(Task).mockImplementation(function (options: any) { @@ -4415,6 +4419,124 @@ describe("ClineProvider", () => { await provider1.dispose() await provider2.dispose() }) + + describe("view state persistence edge cases", () => { + it("should load persisted mode, profile name and resolved profile into viewLocalState", async () => { + const writer = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + await writer["setViewStateId"]("shared-view") + await writer.saveViewState("mode", "architect") + await writer.saveViewState("currentApiConfigName", "my-profile") + + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "editor", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const logSpy = vi.spyOn(provider, "log") + const getProfileSpy = vi.fn().mockResolvedValue({ + name: "my-profile", + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "model-x", + }) + // @ts-ignore - Replace providerSettingsManager with a test double for the profile lookup. + provider.providerSettingsManager = { getProfile: getProfileSpy } + // The ownership check requires the writer to release the id before the + // next view adopts it; the durable entry it wrote stays in storage. + await writer.dispose() + + await provider.contextProxy.setValue( + "viewStates", + mockContext.globalState.get("viewStates"), + ) + // The host-minted claim the writer received is what lets this view re-adopt its entry. + await provider["setViewStateId"]("shared-view", persistedViewStateClaim(mockContext, "shared-view")) + expect(provider["viewLocalState"]).toEqual({ + mode: "architect", + currentApiConfigName: "my-profile", + apiConfiguration: { apiProvider: providerIdentifiers.openrouter, openRouterModelId: "model-x" }, + }) + expect(getProfileSpy).toHaveBeenCalledWith({ name: "my-profile" }) + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("Loaded state for viewId")) + await provider.dispose() + }) + + it("should apply setValue mutations to global state and keep or clear the right buffer fields", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const apiConfiguration = { apiProvider: providerIdentifiers.openrouter } + await provider.saveViewState("mode", "architect") + await provider.saveViewState("currentApiConfigName", "my-profile") + await provider.saveViewState("apiConfiguration", apiConfiguration) + // A mutation of an unrelated key reaches global state without dropping buffered fields. + await provider.setValue("writeDelayMs", 500) + expect(mockContext.globalState.get("writeDelayMs")).toBe(500) + expect(provider.getValues().writeDelayMs).toBe(500) + expect(provider["viewLocalState"].mode).toBe("architect") + expect(provider["viewLocalState"].currentApiConfigName).toBe("my-profile") + expect(provider["viewLocalState"].apiConfiguration).toBe(apiConfiguration) + await provider.setValue("mode", undefined) + expect(provider["viewLocalState"]).not.toHaveProperty("mode") + await provider.setValue("currentApiConfigName", undefined) + expect(provider["viewLocalState"]).not.toHaveProperty("currentApiConfigName") + await provider.dispose() + }) + + it("should remove the buffered apiConfiguration when it is cleared", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const save = provider.saveViewState.bind(provider) as (key: string, value: unknown) => Promise + await provider.saveViewState("apiConfiguration", { apiProvider: providerIdentifiers.openrouter }) + await provider.saveViewState("apiConfiguration", undefined) + expect(provider["viewLocalState"]).not.toHaveProperty("apiConfiguration") + await provider.saveViewState("apiConfiguration", { apiProvider: providerIdentifiers.openrouter }) + await save("apiConfiguration", null) + expect(provider["viewLocalState"]).not.toHaveProperty("apiConfiguration") + await provider.dispose() + }) + + it("should roll back the shared write when setValue per-view persistence fails", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + await provider.contextProxy.setValue("mode", "architect") + const originalUpdate = mockContext.globalState.update + mockContext.globalState.update = vi.fn().mockImplementation((key: string) => { + if (key === "viewStates") { + return Promise.reject(new Error("storage unavailable")) + } + return Promise.resolve() + }) + + await expect(provider.setValue("mode", "code")).rejects.toThrow("storage unavailable") + // The shared store must not move ahead of the per-view record that failed. + expect(provider.getValue("mode")).toBe("architect") + + mockContext.globalState.update = originalUpdate + await provider.dispose() + }) + }) }) describe("webviewMessageHandler no-floating-promises coverage", () => { From 7b610a14dde94204bb0f913632bf289eed7395a0 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 29 Oct 2026 21:03:34 +0800 Subject: [PATCH 06/25] feat(webview): setValues batch persistence and reset cleanup (N4) Split unit N4: setValues per-view persistence with failed-write rollback and resetState clears buffer/persisted entry on top of N3. --- .husky/_/post-checkout | 3 + .husky/_/post-commit | 3 + .husky/_/post-merge | 3 + .husky/_/pre-push | 3 + src/core/webview/ClineProvider.ts | 39 +++++- .../webview/__tests__/ClineProvider.spec.ts | 132 ++++++++++++++++++ 6 files changed, 182 insertions(+), 1 deletion(-) create mode 100644 .husky/_/post-checkout create mode 100644 .husky/_/post-commit create mode 100644 .husky/_/post-merge create mode 100644 .husky/_/pre-push diff --git a/.husky/_/post-checkout b/.husky/_/post-checkout new file mode 100644 index 0000000000..5abf8ed93f --- /dev/null +++ b/.husky/_/post-checkout @@ -0,0 +1,3 @@ +#!/bin/sh +command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'post-checkout' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } +git lfs post-checkout "$@" diff --git a/.husky/_/post-commit b/.husky/_/post-commit new file mode 100644 index 0000000000..b8b76c2c42 --- /dev/null +++ b/.husky/_/post-commit @@ -0,0 +1,3 @@ +#!/bin/sh +command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'post-commit' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } +git lfs post-commit "$@" diff --git a/.husky/_/post-merge b/.husky/_/post-merge new file mode 100644 index 0000000000..726f909891 --- /dev/null +++ b/.husky/_/post-merge @@ -0,0 +1,3 @@ +#!/bin/sh +command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'post-merge' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } +git lfs post-merge "$@" diff --git a/.husky/_/pre-push b/.husky/_/pre-push new file mode 100644 index 0000000000..5f26dc4552 --- /dev/null +++ b/.husky/_/pre-push @@ -0,0 +1,3 @@ +#!/bin/sh +command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'pre-push' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } +git lfs pre-push "$@" diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index f3d4a0d637..633f1a83e1 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -3939,8 +3939,34 @@ export class ClineProvider public getValues() { return { ...this.contextProxy.getValues(), ...this.viewLocalState } } + public async setValues(values: RooCodeSettings) { - await this.contextProxy.setValues(values) + const sanitizedValues = { ...values } + + if (sanitizedValues.mode !== undefined) { + // An unknown or non-string mode (e.g. from an API payload) must not be persisted: + // a new Task would read it from getState() and persist it into task history. + if ( + typeof sanitizedValues.mode !== "string" || + !getModeBySlug(sanitizedValues.mode, await this.customModesManager.getCustomModes()) + ) { + this.log(`[ClineProvider#setValues] Ignoring invalid mode "${String(sanitizedValues.mode)}"`) + delete sanitizedValues.mode + } + } + + const previousSharedValues = Object.fromEntries( + Object.keys(sanitizedValues).map((key) => [key, this.contextProxy.getValue(key as keyof RooCodeSettings)]), + ) as RooCodeSettings + await this.contextProxy.setValues(sanitizedValues) + try { + await this._saveViewLocalStateFromMutation(sanitizedValues) + } catch (error) { + // The per-view record did not land: restore the prior shared values so the two + // persistence layers cannot diverge, then surface the original failure. + await this._restoreSharedValuesAfterFailedViewWrite(previousSharedValues) + throw error + } } /** @@ -3983,6 +4009,9 @@ export class ClineProvider private _clearViewLocalState(): void { this.viewLocalState = {} } + + // dev + async resetState() { const answer = await vscode.window.showInformationMessage( t("common:confirmation.reset_state"), @@ -4007,6 +4036,14 @@ export class ClineProvider } await this.contextProxy.resetAllState() + + // Clear view-local state cache so getState() falls back to ContextProxy defaults. + this._clearViewLocalState() + + // Clear this view's persisted entry too, so the reset selections are not + // re-applied from the durable viewStates pin after a reload. + await this.clearPersistedViewState() + await this.providerSettingsManager.resetAllConfigs() await this.customModesManager.resetCustomModes() await this.removeClineFromStack() diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 6356835cc9..8841092211 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -33,6 +33,7 @@ import { WorkspaceIndexingEnablementManager } from "../../../services/code-index import { Task, TaskOptions } from "../../task/Task" import { safeWriteJson } from "../../../utils/safeWriteJson" +import { t } from "../../../i18n" import { ClineProvider } from "../ClineProvider" import { webviewMessageHandler } from "../webviewMessageHandler" import { Terminal } from "../../../integrations/terminal/Terminal" @@ -4537,6 +4538,137 @@ describe("ClineProvider", () => { await provider.dispose() }) }) + + it("should persist known modes and reject unknown or non-string modes", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const logSpy = vi.spyOn(provider, "log") + // @ts-ignore - Replace customModesManager with a test double (no custom modes). + provider.customModesManager = { getCustomModes: vi.fn().mockResolvedValue([]), dispose: vi.fn() } + // The file-level modes mock resolves every slug to a mode; narrow it to the slugs under test. + const modesModule = vi.mocked(await import("../../../shared/modes")) + const originalMode = modesModule.getModeBySlug("code") + modesModule.getModeBySlug.mockImplementation(((slug: string) => + slug === "refactor" ? { slug } : undefined) as typeof modesModule.getModeBySlug) + try { + await provider.setValues({ mode: "refactor" }) + expect(mockContext.globalState.get("mode")).toBe("refactor") + expect(provider["viewLocalState"].mode).toBe("refactor") + await provider.setValues({ mode: "bogus-mode" }) + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining('Ignoring invalid mode "bogus-mode"')) + expect(mockContext.globalState.get("mode")).toBe("refactor") + expect(provider["viewLocalState"].mode).toBe("refactor") + // A non-string mode must be rejected before persistence; Object.assign + // keeps this a single structural assertion, so it must not reach global + // state or the buffer. + await provider.setValues(Object.assign({} as RooCodeSettings, { mode: 42 })) + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining('Ignoring invalid mode "42"')) + expect(mockContext.globalState.get("mode")).toBe("refactor") + expect(provider["viewLocalState"].mode).toBe("refactor") + } finally { + modesModule.getModeBySlug.mockReturnValue(originalMode) + } + await provider.dispose() + }) + + it("should build the buffered apiConfiguration from provider settings keys", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + provider["viewLocalState"] = { apiConfiguration: { openRouterApiKey: "key-1" } } + await provider.setValues({ apiProvider: providerIdentifiers.openrouter }) + expect(provider["viewLocalState"].apiConfiguration).toStrictEqual({ + apiProvider: providerIdentifiers.openrouter, + }) + await provider.setValues({ openRouterModelId: "model-x" }) + expect(provider["viewLocalState"].apiConfiguration).toEqual({ + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "model-x", + }) + await provider.dispose() + }) + + it("should remove the buffered apiConfiguration when it is cleared", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const save = provider.saveViewState.bind(provider) as (key: string, value: unknown) => Promise + await provider.saveViewState("apiConfiguration", { apiProvider: providerIdentifiers.openrouter }) + await provider.saveViewState("apiConfiguration", undefined) + expect(provider["viewLocalState"]).not.toHaveProperty("apiConfiguration") + await provider.saveViewState("apiConfiguration", { apiProvider: providerIdentifiers.openrouter }) + await save("apiConfiguration", null) + expect(provider["viewLocalState"]).not.toHaveProperty("apiConfiguration") + await provider.dispose() + }) + + it("should clear viewLocalState and the persisted entry when resetting state", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + vi.spyOn(provider, "postStateToWebview").mockResolvedValue(undefined) + // @ts-ignore - Replace customModesManager with a test double (the real reset writes to disk). + provider.customModesManager = { resetCustomModes: vi.fn().mockResolvedValue(undefined), dispose: vi.fn() } + // The modal answer is a string label; the last-typed vscode overload expects a + // MessageItem. A double assertion is the last-resort cast here (AGENTS.md): the + // production path compares the answer against the string label directly, so the + // runtime value must stay a string and cannot be a structural MessageItem double. + vi.mocked(vscode.window.showInformationMessage).mockResolvedValue( + t("common:answers.yes") as unknown as vscode.MessageItem, + ) + await provider["setViewStateId"]("stable-sidebar-view") + await provider.saveViewState("mode", "architect") + await provider.resetState() + expect(provider["viewLocalState"]).toEqual({}) + expect(mockContext.globalState.get("viewStates")).toEqual({}) + await provider.dispose() + }) + + it("should roll back the shared writes when setValues per-view persistence fails", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + await provider.contextProxy.setValue("mode", "architect") + await provider.contextProxy.setValue("currentApiConfigName", "current-config") + const originalUpdate = mockContext.globalState.update + mockContext.globalState.update = vi.fn().mockImplementation((key: string) => { + if (key === "viewStates") { + return Promise.reject(new Error("storage unavailable")) + } + return Promise.resolve() + }) + + await expect(provider.setValues({ mode: "code", currentApiConfigName: "other-config" })).rejects.toThrow( + "storage unavailable", + ) + expect(provider.getValue("mode")).toBe("architect") + expect(provider.getValue("currentApiConfigName")).toBe("current-config") + expect(provider["viewLocalState"].mode).toBeUndefined() + + mockContext.globalState.update = originalUpdate + await provider.dispose() + }) }) describe("webviewMessageHandler no-floating-promises coverage", () => { From 7de336e4ddced8d578ff4d8df2c464bf9bb76043 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 29 Oct 2026 21:03:43 +0800 Subject: [PATCH 07/25] chore: untrack generated husky hooks --- .husky/_/post-checkout | 3 --- .husky/_/post-commit | 3 --- .husky/_/post-merge | 3 --- .husky/_/pre-push | 3 --- .husky/pre-commit | 26 -------------------------- .husky/pre-push | 42 ------------------------------------------ 6 files changed, 80 deletions(-) delete mode 100644 .husky/_/post-checkout delete mode 100644 .husky/_/post-commit delete mode 100644 .husky/_/post-merge delete mode 100644 .husky/_/pre-push delete mode 100644 .husky/pre-commit delete mode 100644 .husky/pre-push diff --git a/.husky/_/post-checkout b/.husky/_/post-checkout deleted file mode 100644 index 5abf8ed93f..0000000000 --- a/.husky/_/post-checkout +++ /dev/null @@ -1,3 +0,0 @@ -#!/bin/sh -command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'post-checkout' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } -git lfs post-checkout "$@" diff --git a/.husky/_/post-commit b/.husky/_/post-commit deleted file mode 100644 index b8b76c2c42..0000000000 --- a/.husky/_/post-commit +++ /dev/null @@ -1,3 +0,0 @@ -#!/bin/sh -command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'post-commit' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } -git lfs post-commit "$@" diff --git a/.husky/_/post-merge b/.husky/_/post-merge deleted file mode 100644 index 726f909891..0000000000 --- a/.husky/_/post-merge +++ /dev/null @@ -1,3 +0,0 @@ -#!/bin/sh -command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'post-merge' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } -git lfs post-merge "$@" diff --git a/.husky/_/pre-push b/.husky/_/pre-push deleted file mode 100644 index 5f26dc4552..0000000000 --- a/.husky/_/pre-push +++ /dev/null @@ -1,3 +0,0 @@ -#!/bin/sh -command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'pre-push' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } -git lfs pre-push "$@" diff --git a/.husky/pre-commit b/.husky/pre-commit deleted file mode 100644 index f8f420dac7..0000000000 --- a/.husky/pre-commit +++ /dev/null @@ -1,26 +0,0 @@ -branch="$(git rev-parse --abbrev-ref HEAD)" - -if [ "$branch" = "main" ]; then - echo "You can't commit directly to main - please check out a branch." - exit 1 -fi - -# Detect if running on Windows and use pnpm.cmd, otherwise use pnpm. -if [ "$OS" = "Windows_NT" ]; then - pnpm_cmd="pnpm.cmd" -else - if command -v pnpm >/dev/null 2>&1; then - pnpm_cmd="pnpm" - else - pnpm_cmd="npx pnpm" - fi -fi - -# Detect if running on Windows and use npx.cmd, otherwise use npx. -if [ "$OS" = "Windows_NT" ]; then - npx_cmd="npx.cmd" -else - npx_cmd="npx" -fi - -$npx_cmd lint-staged diff --git a/.husky/pre-push b/.husky/pre-push deleted file mode 100644 index 4cf91d9580..0000000000 --- a/.husky/pre-push +++ /dev/null @@ -1,42 +0,0 @@ -branch="$(git rev-parse --abbrev-ref HEAD)" - -if [ "$branch" = "main" ]; then - echo "You can't push directly to main - please check out a branch." - exit 1 -fi - -# Detect if running on Windows and use pnpm.cmd, otherwise use pnpm. -if [ "$OS" = "Windows_NT" ]; then - pnpm_cmd="pnpm.cmd" -else - if command -v pnpm >/dev/null 2>&1; then - pnpm_cmd="pnpm" - else - pnpm_cmd="npx pnpm" - fi -fi - -$pnpm_cmd run check-types - -# Use dotenvx to securely load .env.local and run commands that depend on it -if [ -f ".env.local" ]; then - # Check if RUN_TESTS_ON_PUSH is set to true and run tests with dotenvx - if npx dotenvx get RUN_TESTS_ON_PUSH -f .env.local 2>/dev/null | grep -q "^true$"; then - npx dotenvx run -f .env.local -- $pnpm_cmd run test - fi -else - # Fallback: run tests if RUN_TESTS_ON_PUSH is set in regular environment - if [ "$RUN_TESTS_ON_PUSH" = "true" ]; then - $pnpm_cmd run test - fi -fi - -# Check for new changesets. -NEW_CHANGESETS=$(find .changeset -name "*.md" ! -name "README.md" | wc -l | tr -d ' ') -echo "Changeset files: $NEW_CHANGESETS" - -if [ "$NEW_CHANGESETS" = "0" ]; then - echo "-------------------------------------------------------------------------------------" - echo "Changes detected. Please run 'pnpm changeset' to create a changeset if applicable." - echo "-------------------------------------------------------------------------------------" -fi From 035f1171d0604e6e093da4a8f6548f95c7d89649 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 29 Oct 2026 21:11:53 +0800 Subject: [PATCH 08/25] chore: strip BOM and fix lint imports --- .husky/_/post-checkout | 3 ++ .husky/_/post-commit | 3 ++ .husky/_/post-merge | 3 ++ .husky/_/pre-push | 3 ++ .husky/pre-commit | 26 ++++++++++++ .husky/pre-push | 42 +++++++++++++++++++ src/core/webview/ClineProvider.ts | 2 +- .../webview/__tests__/ClineProvider.spec.ts | 2 +- 8 files changed, 82 insertions(+), 2 deletions(-) create mode 100644 .husky/_/post-checkout create mode 100644 .husky/_/post-commit create mode 100644 .husky/_/post-merge create mode 100644 .husky/_/pre-push create mode 100644 .husky/pre-commit create mode 100644 .husky/pre-push diff --git a/.husky/_/post-checkout b/.husky/_/post-checkout new file mode 100644 index 0000000000..5abf8ed93f --- /dev/null +++ b/.husky/_/post-checkout @@ -0,0 +1,3 @@ +#!/bin/sh +command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'post-checkout' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } +git lfs post-checkout "$@" diff --git a/.husky/_/post-commit b/.husky/_/post-commit new file mode 100644 index 0000000000..b8b76c2c42 --- /dev/null +++ b/.husky/_/post-commit @@ -0,0 +1,3 @@ +#!/bin/sh +command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'post-commit' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } +git lfs post-commit "$@" diff --git a/.husky/_/post-merge b/.husky/_/post-merge new file mode 100644 index 0000000000..726f909891 --- /dev/null +++ b/.husky/_/post-merge @@ -0,0 +1,3 @@ +#!/bin/sh +command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'post-merge' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } +git lfs post-merge "$@" diff --git a/.husky/_/pre-push b/.husky/_/pre-push new file mode 100644 index 0000000000..5f26dc4552 --- /dev/null +++ b/.husky/_/pre-push @@ -0,0 +1,3 @@ +#!/bin/sh +command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'pre-push' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } +git lfs pre-push "$@" diff --git a/.husky/pre-commit b/.husky/pre-commit new file mode 100644 index 0000000000..f8f420dac7 --- /dev/null +++ b/.husky/pre-commit @@ -0,0 +1,26 @@ +branch="$(git rev-parse --abbrev-ref HEAD)" + +if [ "$branch" = "main" ]; then + echo "You can't commit directly to main - please check out a branch." + exit 1 +fi + +# Detect if running on Windows and use pnpm.cmd, otherwise use pnpm. +if [ "$OS" = "Windows_NT" ]; then + pnpm_cmd="pnpm.cmd" +else + if command -v pnpm >/dev/null 2>&1; then + pnpm_cmd="pnpm" + else + pnpm_cmd="npx pnpm" + fi +fi + +# Detect if running on Windows and use npx.cmd, otherwise use npx. +if [ "$OS" = "Windows_NT" ]; then + npx_cmd="npx.cmd" +else + npx_cmd="npx" +fi + +$npx_cmd lint-staged diff --git a/.husky/pre-push b/.husky/pre-push new file mode 100644 index 0000000000..4cf91d9580 --- /dev/null +++ b/.husky/pre-push @@ -0,0 +1,42 @@ +branch="$(git rev-parse --abbrev-ref HEAD)" + +if [ "$branch" = "main" ]; then + echo "You can't push directly to main - please check out a branch." + exit 1 +fi + +# Detect if running on Windows and use pnpm.cmd, otherwise use pnpm. +if [ "$OS" = "Windows_NT" ]; then + pnpm_cmd="pnpm.cmd" +else + if command -v pnpm >/dev/null 2>&1; then + pnpm_cmd="pnpm" + else + pnpm_cmd="npx pnpm" + fi +fi + +$pnpm_cmd run check-types + +# Use dotenvx to securely load .env.local and run commands that depend on it +if [ -f ".env.local" ]; then + # Check if RUN_TESTS_ON_PUSH is set to true and run tests with dotenvx + if npx dotenvx get RUN_TESTS_ON_PUSH -f .env.local 2>/dev/null | grep -q "^true$"; then + npx dotenvx run -f .env.local -- $pnpm_cmd run test + fi +else + # Fallback: run tests if RUN_TESTS_ON_PUSH is set in regular environment + if [ "$RUN_TESTS_ON_PUSH" = "true" ]; then + $pnpm_cmd run test + fi +fi + +# Check for new changesets. +NEW_CHANGESETS=$(find .changeset -name "*.md" ! -name "README.md" | wc -l | tr -d ' ') +echo "Changeset files: $NEW_CHANGESETS" + +if [ "$NEW_CHANGESETS" = "0" ]; then + echo "-------------------------------------------------------------------------------------" + echo "Changes detected. Please run 'pnpm changeset' to create a changeset if applicable." + echo "-------------------------------------------------------------------------------------" +fi diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 633f1a83e1..b073169703 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -113,7 +113,7 @@ import { buildApiHandler } from "../../api" import { forceFullModelDetailsLoad, hasLoadedFullDetails } from "../../api/providers/fetchers/lmstudio" import { ContextProxy } from "../config/ContextProxy" -import { ProviderSettingsManager, ProviderSettingsNotFoundError } from "../config/ProviderSettingsManager" +import { ProviderSettingsManager } from "../config/ProviderSettingsManager" import { CustomModesManager } from "../config/CustomModesManager" import { PendingActionSettlementError, Task } from "../task/Task" diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 8841092211..85b87b7f97 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -1,4 +1,4 @@ -// pnpm --filter roo-cline test core/webview/__tests__/ClineProvider.spec.ts +// pnpm --filter roo-cline test core/webview/__tests__/ClineProvider.spec.ts import fs from "fs" import * as path from "path" From c723553c2c3308e8b5085594dd700270f9c680f6 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 29 Oct 2026 21:56:55 +0800 Subject: [PATCH 09/25] fix(webview): address review findings on view-state durability - omit viewStates (host-minted claim tokens) from settings export - keep a pre-launch selection when registration re-keys onto an existing stable entry - narrow saveViewState keys to the durable per-view subset - skip rollback when another provider wrote a newer shared value - restore the ContextProxy cache when globalState.update rejects - clarify that the claim is enforced by setViewStateId; launch wiring follows --- packages/types/src/vscode-extension-host.ts | 6 ++- src/core/config/ContextProxy.ts | 14 +++++- src/core/webview/ClineProvider.ts | 52 ++++++++++++++++++--- 3 files changed, 61 insertions(+), 11 deletions(-) diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index 233fa7538f..c4e7788928 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -681,8 +681,10 @@ export interface WebviewMessage { viewStateId?: string /** * For `webviewDidLaunch`: the host-minted claim previously delivered for `viewStateId`. - * The webview only echoes what it was given; the host rejects an id whose stored claim - * does not match, so a view cannot adopt another view's persisted selections. + * The webview only echoes what it was given; the host enforces the match in + * `ClineProvider.setViewStateId` when a view registers this id, so a view cannot adopt + * another view's persisted selections. Forwarding these fields from the launch handler + * is wired by the follow-up launch unit. */ viewStateClaim?: string taskId?: string diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index 4e20305f48..8678e02219 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -363,13 +363,23 @@ export class ContextProxy { return value !== undefined ? value : defaultValue } - updateGlobalState(key: K, value: GlobalState[K]) { + async updateGlobalState(key: K, value: GlobalState[K]) { if (isPassThroughStateKey(key)) { return this.originalContext.globalState.update(key, value) } + const previousCachedValue = this.stateCache[key] this.stateCache[key] = value - return this.originalContext.globalState.update(key, value) + + try { + return await this.originalContext.globalState.update(key, value) + } catch (error) { + // A rejected write must not leave the cache ahead of storage: callers that then + // read the cached value (e.g. the view-state load path) would act on a selection + // that was never persisted. + this.stateCache[key] = previousCachedValue + throw error + } } private getAllGlobalState(): GlobalState { diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index b073169703..f9050d9361 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -148,6 +148,13 @@ type PersistedViewState = NonNullable[string] */ type ViewLocalStateValues = Partial & Partial +/** + * The subset of view-local values that has a durable per-view record. `saveViewState` + * and the mutation path handle exactly these keys, so the generic stays narrow enough + * for the compiler to reject e.g. `writeDelayMs`. + */ +type PersistedViewStateValues = Pick + /** * https://github.com/microsoft/vscode-webview-ui-toolkit-samples/blob/main/default/weather-webview/src/providers/WeatherViewProvider.ts * https://github.com/KumarVariable/vscode-extension-sidebar-html/blob/master/src/customSidebarViewProvider.ts @@ -774,8 +781,19 @@ export class ClineProvider delete states[previousViewStateId] - if (!states[nextViewStateId]) { + const existingStableEntry = states[nextViewStateId] + + if (!existingStableEntry) { states[nextViewStateId] = previous + } else { + // Registration can bind a claim-less legacy key just before this pre-launch entry + // is re-keyed. Keep the selections the stable entry already carries, but fill in + // anything it lacks so a mode chosen before launch is not silently dropped. + states[nextViewStateId] = { + ...previous, + ...existingStableEntry, + updatedAt: Math.max(previous.updatedAt ?? 0, existingStableEntry.updatedAt ?? 0), + } } await this.contextProxy.setValue("viewStates", this.prunePersistedViewStates(states)) @@ -1024,9 +1042,9 @@ export class ClineProvider * non-secret subset (mode, currentApiConfigName) is persisted durably under the view * id active when the change was made, re-keyed to the stable id on registration. */ - public async saveViewState( + public async saveViewState( key: K, - value: ViewLocalStateValues[K] | undefined, + value: PersistedViewStateValues[K] | undefined, ): Promise { await this._saveViewLocalStateFromMutation({ [key]: value } as ViewLocalStateValues) @@ -3927,7 +3945,9 @@ export class ClineProvider } catch (error) { // The per-view record did not land: restore the prior shared value so the two // persistence layers cannot diverge, then surface the original failure. - await this._restoreSharedValuesAfterFailedViewWrite(previousSharedValues) + await this._restoreSharedValuesAfterFailedViewWrite(previousSharedValues, { + [key]: value, + } as RooCodeSettings) throw error } } @@ -3964,7 +3984,7 @@ export class ClineProvider } catch (error) { // The per-view record did not land: restore the prior shared values so the two // persistence layers cannot diverge, then surface the original failure. - await this._restoreSharedValuesAfterFailedViewWrite(previousSharedValues) + await this._restoreSharedValuesAfterFailedViewWrite(previousSharedValues, sanitizedValues) throw error } } @@ -5407,9 +5427,27 @@ export class ClineProvider * per-view persistence failed so the shared store cannot move ahead of the per-view * record. A failed rollback is logged; the original failure is what callers see. */ - private async _restoreSharedValuesAfterFailedViewWrite(previous: RooCodeSettings): Promise { + private async _restoreSharedValuesAfterFailedViewWrite( + previous: RooCodeSettings, + attempted: RooCodeSettings, + ): Promise { + // Roll back only the keys this attempt wrote and that still hold the value it wrote. + // A concurrent provider may have written a newer value in the meantime; restoring the + // captured previous value would then clobber it for every view that reads shared state. + const rollback: RooCodeSettings = {} as RooCodeSettings + + for (const key of Object.keys(attempted) as (keyof RooCodeSettings)[]) { + if (this.contextProxy.getValue(key) === attempted[key]) { + ;(rollback as Record)[key] = previous[key] + } + } + + if (Object.keys(rollback).length === 0) { + return + } + try { - await this.contextProxy.setValues(previous) + await this.contextProxy.setValues(rollback) } catch (rollbackError) { this.log( `[ClineProvider] Shared-state rollback after failed per-view write failed: ${rollbackError instanceof Error ? rollbackError.message : String(rollbackError)}`, From faa3982229679b5865ad78b88fb41fc4fcac5666 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 30 Oct 2026 12:03:42 +0800 Subject: [PATCH 10/25] fix(config): keep a newer cached value when an older global-state write rejects Track a per-key write generation so a rejected write only rolls the cache back while it is still the newest write for that key; also stop tracking the generated .husky/_ hook files. --- .husky/_/post-checkout | 3 --- .husky/_/post-commit | 3 --- .husky/_/post-merge | 3 --- .husky/_/pre-push | 3 --- src/core/config/ContextProxy.ts | 14 +++++++++---- .../config/__tests__/ContextProxy.spec.ts | 21 +++++++++++++++++++ 6 files changed, 31 insertions(+), 16 deletions(-) delete mode 100644 .husky/_/post-checkout delete mode 100644 .husky/_/post-commit delete mode 100644 .husky/_/post-merge delete mode 100644 .husky/_/pre-push diff --git a/.husky/_/post-checkout b/.husky/_/post-checkout deleted file mode 100644 index 5abf8ed93f..0000000000 --- a/.husky/_/post-checkout +++ /dev/null @@ -1,3 +0,0 @@ -#!/bin/sh -command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'post-checkout' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } -git lfs post-checkout "$@" diff --git a/.husky/_/post-commit b/.husky/_/post-commit deleted file mode 100644 index b8b76c2c42..0000000000 --- a/.husky/_/post-commit +++ /dev/null @@ -1,3 +0,0 @@ -#!/bin/sh -command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'post-commit' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } -git lfs post-commit "$@" diff --git a/.husky/_/post-merge b/.husky/_/post-merge deleted file mode 100644 index 726f909891..0000000000 --- a/.husky/_/post-merge +++ /dev/null @@ -1,3 +0,0 @@ -#!/bin/sh -command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'post-merge' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } -git lfs post-merge "$@" diff --git a/.husky/_/pre-push b/.husky/_/pre-push deleted file mode 100644 index 5f26dc4552..0000000000 --- a/.husky/_/pre-push +++ /dev/null @@ -1,3 +0,0 @@ -#!/bin/sh -command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'pre-push' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } -git lfs pre-push "$@" diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index 8678e02219..1e7815e8dd 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -45,6 +45,7 @@ export class ContextProxy { private readonly originalContext: vscode.ExtensionContext private stateCache: GlobalState + private stateWriteGenerations = new Map() private secretCache: SecretState private _isInitialized = false @@ -369,15 +370,20 @@ export class ContextProxy { } const previousCachedValue = this.stateCache[key] + const generation = (this.stateWriteGenerations.get(key) ?? 0) + 1 + this.stateWriteGenerations.set(key, generation) this.stateCache[key] = value try { return await this.originalContext.globalState.update(key, value) } catch (error) { - // A rejected write must not leave the cache ahead of storage: callers that then - // read the cached value (e.g. the view-state load path) would act on a selection - // that was never persisted. - this.stateCache[key] = previousCachedValue + // A rejected write must not leave the cache ahead of storage, but a *newer* write + // for the same key may already have replaced the cached value: restoring here would + // then roll the cache back behind storage. Only undo this write while it is still + // the newest one for the key. + if (this.stateWriteGenerations.get(key) === generation) { + this.stateCache[key] = previousCachedValue + } throw error } } diff --git a/src/core/config/__tests__/ContextProxy.spec.ts b/src/core/config/__tests__/ContextProxy.spec.ts index f9fa9cdb15..79cb0471d2 100644 --- a/src/core/config/__tests__/ContextProxy.spec.ts +++ b/src/core/config/__tests__/ContextProxy.spec.ts @@ -120,6 +120,27 @@ describe("ContextProxy", () => { expect(result).toBe("deepseek") }) + it("should not let an older rejected write clobber a newer cached value", async () => { + let rejectFirst: (error: Error) => void = () => {} + const firstWriteResult = new Promise((_, reject) => { + rejectFirst = reject + }) + + mockGlobalState.update.mockReset() + mockGlobalState.update.mockReturnValueOnce(firstWriteResult) + mockGlobalState.update.mockResolvedValueOnce(undefined) + + const firstWrite = proxy.updateGlobalState("apiProvider", "anthropic") + await proxy.updateGlobalState("apiProvider", "openai") + + // The older write rejects after a newer value already reached the cache: rolling + // the cache back to the pre-first-write value would diverge from storage. + rejectFirst(new Error("disk full")) + await expect(firstWrite).rejects.toThrow("disk full") + + expect(proxy.getGlobalState("apiProvider")).toBe("openai") + }) + it("should bypass cache for pass-through state keys", async () => { // Setup mock return value mockGlobalState.get.mockReturnValue("pass-through-value") From 9b68e063177306308d5e96fd329e451cb609635e Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 30 Oct 2026 12:22:03 +0800 Subject: [PATCH 11/25] chore: tighten the shared review-fix comments --- src/core/config/ContextProxy.ts | 6 +-- src/core/webview/ClineProvider.ts | 79 ++++++++----------------------- 2 files changed, 21 insertions(+), 64 deletions(-) diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index 1e7815e8dd..a284c8444b 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -377,10 +377,8 @@ export class ContextProxy { try { return await this.originalContext.globalState.update(key, value) } catch (error) { - // A rejected write must not leave the cache ahead of storage, but a *newer* write - // for the same key may already have replaced the cached value: restoring here would - // then roll the cache back behind storage. Only undo this write while it is still - // the newest one for the key. + // A newer write for the same key may already own the cache: only undo this write + // while it is still the newest one. if (this.stateWriteGenerations.get(key) === generation) { this.stateCache[key] = previousCachedValue } diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index f9050d9361..68b4f147ff 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -935,21 +935,13 @@ export class ClineProvider } } - /** - * Loads non-secret persisted selections from the registered viewStates map. - * Missing entries are intentionally left unset so getState() falls back to shared ContextProxy values. - * Fields mutated while the async profile lookup is in flight are reapplied on top of the - * loaded state, field by field, so in-flight user selections are not clobbered by the load. - */ + /** Load non-secret persisted selections; in-flight buffer edits are reapplied on top. */ private async loadViewState(): Promise { - // Capture the id this load is for: a newer id registered while an async - // profile lookup is in flight must not be overwritten by this stale load. + // A newer id registered while this load is in flight must win. const loadedForViewId = this.viewStateId - // While the stable id has not been registered yet, the key is this instance's - // session-local name. Temporary names restart from 0 on every host start, so an - // entry under the same name can belong to a different session. Load only entries - // this instance authored. + // Before registration the key is the session-local name, which can be a stale entry + // from another session: load only entries this instance authored. if (loadedForViewId === this.viewId && !this.wroteUnderTemporaryViewStateId) { return } @@ -957,15 +949,12 @@ export class ClineProvider const persisted = this.getPersistedViewStates()[loadedForViewId] const loadedState: Partial = {} - // Snapshot the in-memory buffer before the async profile lookup. The - // mutation paths update viewLocalState in place, so a shallow copy is - // what makes fields mutated during the load window observable below. + // The mutation paths update viewLocalState in place; a shallow snapshot makes + // in-flight edits observable below. const preLoadBuffer = { ...this.viewLocalState } if (persisted?.mode) { - // A persisted mode may reference a custom mode that was deleted after it was - // pinned: restore it only when the slug still resolves, so a stale slug cannot - // shadow the shared mode from getState(). + // A stale custom-mode slug must not shadow the shared mode from getState(). const customModes = await this.customModesManager.getCustomModes() if (getModeBySlug(persisted.mode, customModes)) { loadedState.mode = persisted.mode as Mode @@ -994,13 +983,8 @@ export class ClineProvider return } - // Reapply the buffer fields that changed while the load was in flight, - // tracking the change instead of testing against undefined: a field cleared - // during the load window must stay cleared (the loaded value must not - // resurrect it), and a field written to a new value must win over it. - // Untouched fields keep the persisted values authoritative, and the - // pre-load buffer is never merged wholesale so stale temporary-id state - // cannot override the stable persisted state. + // Diff each field so in-flight edits win over the loaded values (including clears) + // while untouched fields keep the persisted values authoritative. const postLoadBuffer = this.viewLocalState const mergedState: Partial = { ...loadedState } @@ -1037,11 +1021,7 @@ export class ClineProvider } } - /** - * Saves a single view-local state value. The in-memory buffer is always updated; the - * non-secret subset (mode, currentApiConfigName) is persisted durably under the view - * id active when the change was made, re-keyed to the stable id on registration. - */ + /** Save one view-local value; the durable subset is re-keyed to the stable id. */ public async saveViewState( key: K, value: PersistedViewStateValues[K] | undefined, @@ -2611,16 +2591,11 @@ export class ClineProvider // `allSettled` so a caller that rolls back after a failure never races a write still in flight. const activationResults = await Promise.allSettled([ this.updateGlobalState("listApiConfigMeta", await this.providerSettingsManager.listConfig()), - // Route through setValue so the in-memory viewLocalState buffer tracks the - // activated profile: a plain global write would leave a stale loaded - // currentApiConfigName shadowing the new value in getValues(). + // Route through setValue so the buffer tracks the activated profile. this.setValue("currentApiConfigName", name), this.providerSettingsManager.setModeConfig(mode, id), this.contextProxy.setProviderSettings(providerSettings), - // setProviderSettings writes the shared store directly, bypassing the - // view-local mutation path: also refresh this view's buffer so a stale - // loaded apiConfiguration cannot keep shadowing the new settings in - // getState(). + // setProviderSettings bypasses the buffer path: refresh this view's overlay. this._saveViewLocalStateFromMutation({ apiConfiguration: providerSettings }), ]) const failedActivation = activationResults.find( @@ -2631,9 +2606,7 @@ export class ClineProvider if (signal.aborted) return id - // Other live views may have buffered this profile's settings earlier; - // refresh them so their getState() cannot report the updated profile's - // name with stale settings. + // Refresh other views pinned to this profile so their overlays cannot go stale. // Change the provider for the current task. // TODO: We should rename `buildApiHandler` for clarity (e.g. `getProviderClient`). @@ -2644,16 +2617,11 @@ export class ClineProvider } else { await this.updateGlobalState("listApiConfigMeta", await this.providerSettingsManager.listConfig()) - // A save that does not activate still changes the profile's settings, and - // every view pinned to it buffers a full copy of them. Refresh those views - // (and this one when it is the pinned view) or their getState() keeps - // serving the settings captured before the save under the same name. + // A non-activating save still changes the profile's settings: refresh the views + // that buffered a copy, or their getState() keeps the pre-save values. if (this.pinnedProfileName === name) { await this._saveViewLocalStateFromMutation({ apiConfiguration: providerSettings }) - // Only rebuild when the task actually runs under the saved profile: a - // view pinned to this profile can hold a task that follows a different - // sticky profile, and the forced rebuild would silently switch that - // conversation to the saved profile's settings. + // Only rebuild a task that actually runs under the saved profile. if (this.getCurrentTask()?.taskApiConfigName === name) { this.updateTaskApiHandlerIfNeeded(providerSettings, { forceRebuild: true }) } @@ -3989,11 +3957,7 @@ export class ClineProvider } } - /** - * Persists the view-local subset of a ContextProxy mutation, then updates the in-memory - * viewLocalState buffer. Persistence is awaited first so a failed durable write cannot - * leave the local cache ahead of the persisted state. - */ + /** Persist the durable subset first, then update the in-memory buffer. */ private async _saveViewLocalStateFromMutation( values: Partial & Partial, ): Promise { @@ -4001,10 +3965,7 @@ export class ClineProvider this._updateViewLocalStateFromMutation(values) } - /** - * Writes the durably persisted subset of a mutation (mode and currentApiConfigName) - * into the registered viewStates map for this view. - */ + /** Write the durable subset of a mutation into this view's viewStates entry. */ private async _persistViewLocalStateFromMutation( values: Partial & Partial, ): Promise { @@ -4023,9 +3984,7 @@ export class ClineProvider } } - /** - * Clear view-local state cache so that getState() falls back to ContextProxy defaults. - */ + /** Clear the buffer so getState() falls back to the shared values. */ private _clearViewLocalState(): void { this.viewLocalState = {} } From ac73edbc60d06bdc9d28e5811893e71d8f314c35 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 30 Oct 2026 12:30:34 +0800 Subject: [PATCH 12/25] fix(config): advance write generations on reset A write pending across resetAllState() must not restore its pre-reset value when it rejects afterwards; bump the generation of every tracked key during the reset, with a test. --- src/core/config/ContextProxy.ts | 5 +++++ .../config/__tests__/ContextProxy.spec.ts | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index a284c8444b..e6aea60685 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -607,6 +607,11 @@ export class ContextProxy { // Clear in-memory caches this.stateCache = {} this.secretCache = {} + // A write that started before this reset must not restore its pre-reset value when it + // rejects afterwards, so advance every generation the pending writes still hold. + for (const key of this.stateWriteGenerations.keys()) { + this.stateWriteGenerations.set(key, (this.stateWriteGenerations.get(key) ?? 0) + 1) + } await Promise.all([ ...GLOBAL_STATE_KEYS.map((key) => this.originalContext.globalState.update(key, undefined)), diff --git a/src/core/config/__tests__/ContextProxy.spec.ts b/src/core/config/__tests__/ContextProxy.spec.ts index 79cb0471d2..a28ca249ac 100644 --- a/src/core/config/__tests__/ContextProxy.spec.ts +++ b/src/core/config/__tests__/ContextProxy.spec.ts @@ -141,6 +141,25 @@ describe("ContextProxy", () => { expect(proxy.getGlobalState("apiProvider")).toBe("openai") }) + it("should not restore a pre-reset value when a pending write rejects after reset", async () => { + let rejectPending: (error: Error) => void = () => {} + mockGlobalState.update.mockReset() + mockGlobalState.update.mockReturnValueOnce( + new Promise((_, reject) => { + rejectPending = reject + }), + ) + + const pending = proxy.updateGlobalState("apiProvider", "anthropic") + await proxy.resetAllState() + rejectPending(new Error("disk full")) + + await expect(pending).rejects.toThrow("disk full") + // The reset cleared the cache: restoring the pre-reset value would resurrect state + // that storage no longer holds. + expect(proxy.getGlobalState("apiProvider")).toBeUndefined() + }) + it("should bypass cache for pass-through state keys", async () => { // Setup mock return value mockGlobalState.get.mockReturnValue("pass-through-value") From 04350e2b5514e6280e05e7b52d5e054681c0de0e Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 30 Oct 2026 12:31:34 +0800 Subject: [PATCH 13/25] chore: align the shared view-identity docs with N1 --- packages/types/src/global-settings.ts | 6 +-- packages/types/src/vscode-extension-host.ts | 5 +-- src/core/config/ProviderSettingsManager.ts | 7 +-- src/core/webview/ClineProvider.ts | 47 ++++----------------- 4 files changed, 13 insertions(+), 52 deletions(-) diff --git a/packages/types/src/global-settings.ts b/packages/types/src/global-settings.ts index b09bae2635..3da73ee0fe 100644 --- a/packages/types/src/global-settings.ts +++ b/packages/types/src/global-settings.ts @@ -122,10 +122,8 @@ export const viewStateSchema = z.object({ currentApiConfigName: z.string().optional(), updatedAt: z.number().optional(), /** - * Host-minted capability token binding this entry to the webview that wrote it. - * Minted by the extension host (`crypto.randomUUID()`), delivered to that webview and - * never accepted from webview input. An entry is only re-adopted for a webview that - * presents this token, so a view cannot read another (e.g. closed) view's selections. + * Host-minted capability token binding this entry to the webview that wrote it; never + * accepted from webview input and only a matching token re-adopts the entry. */ claimToken: z.string().optional(), }) diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index c4e7788928..c77dbb14c4 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -681,10 +681,7 @@ export interface WebviewMessage { viewStateId?: string /** * For `webviewDidLaunch`: the host-minted claim previously delivered for `viewStateId`. - * The webview only echoes what it was given; the host enforces the match in - * `ClineProvider.setViewStateId` when a view registers this id, so a view cannot adopt - * another view's persisted selections. Forwarding these fields from the launch handler - * is wired by the follow-up launch unit. + * Enforced by `ClineProvider.setViewStateId`; launch forwarding ships in the wiring unit. */ viewStateClaim?: string taskId?: string diff --git a/src/core/config/ProviderSettingsManager.ts b/src/core/config/ProviderSettingsManager.ts index 306253578a..07a5fc3d21 100644 --- a/src/core/config/ProviderSettingsManager.ts +++ b/src/core/config/ProviderSettingsManager.ts @@ -55,12 +55,7 @@ export const providerProfilesSchema = z.object({ export type ProviderProfiles = z.infer -/** - * Signals that a profile's configuration no longer exists. Callers that treat an - * already-deleted profile as an idempotent no-op branch on this type instead of - * matching error message text, which a profile name containing the phrase could - * otherwise spoof. - */ +/** Already-deleted profile: callers branch on this type instead of matching message text. */ export class ProviderSettingsNotFoundError extends Error { constructor(message: string) { super(message) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 68b4f147ff..6e40ab2b6b 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -377,12 +377,8 @@ export class ClineProvider public readonly viewId: string /** - * Stable identifier for persisted per-view state keys. - * Defaults to viewId until the webview reports its VS Code-persisted id. - * A durable entry under this id is bound to the webview that wrote it by a host-minted - * claim token (see setViewStateId), so the entry - and the profile its pin brings back - * into the state post - can outlive this instance without becoming adoptable by any - * other webview. + * Stable identifier for persisted per-view state keys; binds a durable entry to the + * webview that wrote it through the host-minted claim token. */ private viewStateId: string @@ -605,13 +601,8 @@ export class ClineProvider } /** - * Persists this view's non-secret selections through the serialized write queue. - * The write re-reads the map fresh and merges into the existing entry, removing the - * entry entirely when nothing persistable remains, so concurrent views cannot clobber it. - * The entry is keyed by the view id active when the change was made. Writes captured - * while the provider still holds its temporary (pre-launch) id persist under that id - * and are re-keyed to the stable view id when the webview registers one, so a change - * that lands before the launch message stays durable instead of being lost. + * Persist the non-secret selections through the serialized write queue, re-keying a + * pre-launch write to the stable id once the webview registers one. */ private async savePersistedViewState(values: Partial): Promise { // Capture the id at change time: a write belongs to the view that was active @@ -699,11 +690,8 @@ export class ClineProvider } /** - * Re-binds a claim-less viewStates entry to this webview with a freshly minted claim. - * Entries written before claims existed cannot be attributed to any webview, so their - * selections are dropped instead of being carried over: keeping them would let the new - * token re-expose them on a later launch, which is exactly the disclosure this binding - * exists to prevent. Runs through the serialized write queue like every other mutation. + * Re-bind a claim-less viewStates entry with a freshly minted claim; its unowned + * selections are dropped so the new token cannot re-expose them. */ private async rebindUnboundViewStateEntry(viewStateId: string): Promise { const claimToken = crypto.randomUUID() @@ -749,18 +737,7 @@ export class ClineProvider ) } - /** - * Re-keys this provider's temporary pre-launch viewStates entry to the newly - * registered stable id so pre-launch writes become durable under the stable key - * instead of orphaning under a session-local temporary id. Only the provider's own - * temporary id is eligible: an entry under a previously registered stable id belongs - * to that webview's storage and is left alone. When the stable entry already exists - * it wins and the temporary entry is dropped, because temporary ids are session - * counters that can collide across window reloads. Runs through the serialized write - * queue like every other viewStates mutation. An entry this instance never wrote is - * left alone, so a stale entry that happens to carry the same session-local name is - * not adopted. - */ + /** Re-key this instance's authored pre-launch entry to the stable id. */ private async rekeyPersistedViewStateEntry(nextViewStateId: string): Promise { const previousViewStateId = this.viewId @@ -808,14 +785,8 @@ export class ClineProvider } /** - * Registers this provider's stable view identifier and loads any persisted selections it owns. - * The identifier is sanitized so it remains a safe object key in the shared viewStates map. - * - * `presentedClaim` is the host-minted token this webview was given for that id. It is the - * only proof of ownership: an entry that carries a claim is loaded only when the presented - * token matches it, so a webview cannot adopt another (e.g. disposed, still durable) view's - * id and read that view's pinned profile - and the profile secrets copied into the state - * post - back out of it. The check runs before any re-keying or loading. + * Register the stable view id and load its persisted selections. An entry that carries a + * claim is loaded only when the presented host-minted token matches it. */ public async setViewStateId(viewStateId: string | undefined, presentedClaim?: string): Promise { // The webview generates this id on purpose - it has to survive a webview reload, From 66184ce25d737905a10fc389be020cfcc9088fa8 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 30 Oct 2026 12:56:14 +0800 Subject: [PATCH 14/25] fix(webview): invalidate in-flight loads on reset and refresh sibling overlays --- src/core/webview/ClineProvider.ts | 23 ++++++- .../webview/__tests__/ClineProvider.spec.ts | 64 +++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 83503d9b26..9704fc468e 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -396,6 +396,10 @@ export class ClineProvider */ private viewLocalState: Partial = {} + // Bumped whenever the buffer is cleared (reset/import), so an in-flight load can tell + // that its captured state was invalidated while it awaited the profile lookup. + private viewLocalStateGeneration = 0 + /** * This view's pinned profile name from the view-local buffer. Exposed for * sibling-instance inspection (getAllInstances() filtering): `viewLocalState` @@ -908,6 +912,7 @@ export class ClineProvider /** Load non-secret persisted selections; in-flight buffer edits are reapplied on top. */ private async loadViewState(): Promise { + const loadedGeneration = this.viewLocalStateGeneration // A newer id registered while this load is in flight must win. const loadedForViewId = this.viewStateId @@ -983,6 +988,13 @@ export class ClineProvider } } + // A reset/import may have cleared the buffer while the lookup was in flight: + // republishing here would restore the pre-reset selection over the cleared state. + if (this.viewLocalStateGeneration !== loadedGeneration) { + this.log(`[loadViewState] Discarding state loaded before a reset for viewId ${this.viewId}`) + return + } + this.viewLocalState = mergedState this.log(`[loadViewState] Loaded state for viewId ${this.viewId}`) } catch (error) { @@ -2577,7 +2589,15 @@ export class ClineProvider if (signal.aborted) return id - // Refresh other views pinned to this profile so their overlays cannot go stale. + // Refresh other live views pinned to this profile so their buffered overlay cannot + // keep serving the pre-save settings under the same profile name. + for (const instance of ClineProvider.getAllInstances()) { + if (instance === this || instance._disposed || instance.pinnedProfileName !== name) { + continue + } + // Direct private access: compile-time safe across sibling instances. + await instance._saveViewLocalStateFromMutation({ apiConfiguration: providerSettings }) + } // Change the provider for the current task. // TODO: We should rename `buildApiHandler` for clarity (e.g. `getProviderClient`). @@ -3957,6 +3977,7 @@ export class ClineProvider /** Clear the buffer so getState() falls back to the shared values. */ private _clearViewLocalState(): void { + this.viewLocalStateGeneration += 1 this.viewLocalState = {} } diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 85b87b7f97..c09d1471ab 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -4641,6 +4641,70 @@ describe("ClineProvider", () => { await provider.dispose() }) + it("should discard a load that finishes after reset cleared the buffer", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + await provider["setViewStateId"]("stable-sidebar-view") + await provider.contextProxy.setValue("viewStates", { + "stable-sidebar-view": { mode: "architect", updatedAt: 1 }, + }) + + let releaseModes: () => void = () => {} + vi.spyOn(provider.customModesManager, "getCustomModes").mockImplementation( + () => + new Promise((resolve) => { + releaseModes = () => resolve([]) + }), + ) + + const load = provider["loadViewState"]() + // The reset clears the buffer while the load awaits the mode lookup. + provider["_clearViewLocalState"]() + releaseModes() + await load + + expect(provider["viewLocalState"]).toEqual({}) + await provider.dispose() + }) + + it("should refresh the buffered overlay of another view pinned to the saved profile", async () => { + const saver = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const pinned = new ClineProvider( + mockContext, + mockOutputChannel, + "editor", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + await pinned.setValue("currentApiConfigName", "shared-profile") + // @ts-ignore - The double only backs the profile-store calls of the save path. + saver.providerSettingsManager = { + saveConfig: vi.fn().mockResolvedValue("shared-id"), + listConfig: vi.fn().mockResolvedValue([{ name: "shared-profile", id: "shared-id" }]), + setModeConfig: vi.fn().mockResolvedValue(undefined), + } + const providerSettings = { apiProvider: providerIdentifiers.anthropic, apiKey: "fresh-key" } + + await saver.upsertProviderProfile("shared-profile", providerSettings, true) + + // The pinned view buffered a copy of the profile's settings; a non-activating save must + // refresh it, or its getState() keeps serving the pre-save configuration. + expect(pinned["viewLocalState"].apiConfiguration).toEqual(providerSettings) + await saver.dispose() + await pinned.dispose() + }) + it("should roll back the shared writes when setValues per-view persistence fails", async () => { const provider = new ClineProvider( mockContext, From 9a01dd8fd04d4a2558f91470b2d03c1da69738af Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 30 Oct 2026 12:57:02 +0800 Subject: [PATCH 15/25] chore: tighten the shared view-state docs --- packages/types/src/global-settings.ts | 5 +---- packages/types/src/vscode-extension-host.ts | 5 +---- src/core/webview/ClineProvider.ts | 20 ++++---------------- 3 files changed, 6 insertions(+), 24 deletions(-) diff --git a/packages/types/src/global-settings.ts b/packages/types/src/global-settings.ts index 3da73ee0fe..f9fe1f1139 100644 --- a/packages/types/src/global-settings.ts +++ b/packages/types/src/global-settings.ts @@ -121,10 +121,7 @@ export const viewStateSchema = z.object({ mode: z.string().optional(), currentApiConfigName: z.string().optional(), updatedAt: z.number().optional(), - /** - * Host-minted capability token binding this entry to the webview that wrote it; never - * accepted from webview input and only a matching token re-adopts the entry. - */ + /** Host-minted token binding this entry to the webview that wrote it. */ claimToken: z.string().optional(), }) diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index c77dbb14c4..3954246e15 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -679,10 +679,7 @@ export interface WebviewMessage { | "themeFixtureProbeResponse" text?: string viewStateId?: string - /** - * For `webviewDidLaunch`: the host-minted claim previously delivered for `viewStateId`. - * Enforced by `ClineProvider.setViewStateId`; launch forwarding ships in the wiring unit. - */ + /** For `webviewDidLaunch`: the claim delivered for `viewStateId`, enforced by setViewStateId. */ viewStateClaim?: string taskId?: string editedMessageContent?: string diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 9704fc468e..ae5f7f1da9 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -376,10 +376,7 @@ export class ClineProvider */ public readonly viewId: string - /** - * Stable identifier for persisted per-view state keys; binds a durable entry to the - * webview that wrote it through the host-minted claim token. - */ + /** Stable id for persisted per-view state keys, bound by the host-minted claim. */ private viewStateId: string /** @@ -604,10 +601,7 @@ export class ClineProvider return Object.assign(Object.create(null), viewStates) } - /** - * Persist the non-secret selections through the serialized write queue, re-keying a - * pre-launch write to the stable id once the webview registers one. - */ + /** Persist the non-secret selections through the serialized write queue. */ private async savePersistedViewState(values: Partial): Promise { // Capture the id at change time: a write belongs to the view that was active // when the change was made, even if a newer id is registered while it is queued. @@ -693,10 +687,7 @@ export class ClineProvider await write } - /** - * Re-bind a claim-less viewStates entry with a freshly minted claim; its unowned - * selections are dropped so the new token cannot re-expose them. - */ + /** Re-bind a claim-less entry with a fresh claim; unowned selections are dropped. */ private async rebindUnboundViewStateEntry(viewStateId: string): Promise { const claimToken = crypto.randomUUID() let rebound = false @@ -788,10 +779,7 @@ export class ClineProvider await write } - /** - * Register the stable view id and load its persisted selections. An entry that carries a - * claim is loaded only when the presented host-minted token matches it. - */ + /** Register the stable view id and load its persisted selections; a claim must match. */ public async setViewStateId(viewStateId: string | undefined, presentedClaim?: string): Promise { // The webview generates this id on purpose - it has to survive a webview reload, // which recreates the view on both sides - so the host cannot mint it instead. From e8307ef434882b730ec2917524c22f2f8ab64eb7 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sun, 1 Nov 2026 21:54:15 +0800 Subject: [PATCH 16/25] fix(webview): address the N4 reset and profile-save findings - resetState clears the persisted view entry last and tolerates its failure so the reset always finishes - the view-local buffer half is skipped when a reset ran during the durable write, checked in the shared helper - createTask registers the batch customModes before setValues validates the mode - a profile save rebuilds the sibling task handler when that task follows the saved profile --- src/core/webview/ClineProvider.ts | 51 ++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 14 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index ae5f7f1da9..0cb8f13aaa 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -2585,6 +2585,17 @@ export class ClineProvider } // Direct private access: compile-time safe across sibling instances. await instance._saveViewLocalStateFromMutation({ apiConfiguration: providerSettings }) + + // A sibling task running under this profile holds a handler built from the old + // settings: rebuild it so its next request does not use the stale credentials. + if (instance.getCurrentTask()?.taskApiConfigName === name) { + const { + name: _recordName, + id: _recordId, + ...taskSettings + } = providerSettings as ProviderSettings & { name?: string; id?: string } + instance.updateTaskApiHandlerIfNeeded(taskSettings, { forceRebuild: true }) + } } // Change the provider for the current task. @@ -3940,7 +3951,13 @@ export class ClineProvider private async _saveViewLocalStateFromMutation( values: Partial & Partial, ): Promise { + // Captured here, not in the setters: saveViewState() and the profile paths call this + // helper directly, so a reset during their durable write must skip the buffer half too. + const generation = this.viewLocalStateGeneration await this._persistViewLocalStateFromMutation(values) + if (this.viewLocalStateGeneration !== generation) { + return + } this._updateViewLocalStateFromMutation(values) } @@ -3999,13 +4016,21 @@ export class ClineProvider // Clear view-local state cache so getState() falls back to ContextProxy defaults. this._clearViewLocalState() - // Clear this view's persisted entry too, so the reset selections are not - // re-applied from the durable viewStates pin after a reload. - await this.clearPersistedViewState() - await this.providerSettingsManager.resetAllConfigs() await this.customModesManager.resetCustomModes() await this.removeClineFromStack() + + // Clear this view's persisted entry last, and tolerate a storage failure: the shared + // stores, custom modes and the running task are already reset, so a viewStates write + // error must not abort the reset half-way or skip the state post below. + try { + await this.clearPersistedViewState() + } catch (error) { + this.log( + `[resetState] Failed to clear the persisted view state: ${error instanceof Error ? error.message : String(error)}`, + ) + } + await this.postStateToWebview() await this.postMessageToWebview({ type: "action", action: "chatButtonClicked" }) } @@ -4143,6 +4168,14 @@ export class ClineProvider configuration: RooCodeSettings = {}, ): Promise { if (configuration) { + // Register the batch's custom modes before setValues validates `mode`: a mode + // selected in the same configuration is not known to the manager yet. + if (configuration.customModes?.length) { + for (const mode of configuration.customModes) { + await this.customModesManager.updateCustomMode(mode.slug, mode) + } + } + await this.setValues(configuration) if (configuration.allowedCommands) { @@ -4170,16 +4203,6 @@ export class ClineProvider if (configuration.currentApiConfigName) { await this.setProviderProfile(configuration.currentApiConfigName) } - - // Register custom modes so the CustomModesManager knows about them. - // setValues writes to global state, but the manager overwrites that - // when it merges .roomodes + global settings on refresh. Persisting - // via updateCustomMode ensures modes survive the merge cycle. - if (configuration.customModes?.length) { - for (const mode of configuration.customModes) { - await this.customModesManager.updateCustomMode(mode.slug, mode) - } - } } const { From 5841fc1f4ced7c3590aafc49edf80a42dc959d3d Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 3 Nov 2026 01:33:39 +0800 Subject: [PATCH 17/25] fix(webview): address the N4 sibling-refresh and reset findings - pinned siblings refresh on non-activating saves too - sibling tasks that follow the saved profile are rebuilt regardless of their view pin - reset clears every live view buffer and serializes its persisted-entry cleanup with profile saves --- src/core/webview/ClineProvider.ts | 67 +++++++++++++++++++------------ 1 file changed, 42 insertions(+), 25 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 0cb8f13aaa..f159d3777d 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -2577,26 +2577,7 @@ export class ClineProvider if (signal.aborted) return id - // Refresh other live views pinned to this profile so their buffered overlay cannot - // keep serving the pre-save settings under the same profile name. - for (const instance of ClineProvider.getAllInstances()) { - if (instance === this || instance._disposed || instance.pinnedProfileName !== name) { - continue - } - // Direct private access: compile-time safe across sibling instances. - await instance._saveViewLocalStateFromMutation({ apiConfiguration: providerSettings }) - - // A sibling task running under this profile holds a handler built from the old - // settings: rebuild it so its next request does not use the stale credentials. - if (instance.getCurrentTask()?.taskApiConfigName === name) { - const { - name: _recordName, - id: _recordId, - ...taskSettings - } = providerSettings as ProviderSettings & { name?: string; id?: string } - instance.updateTaskApiHandlerIfNeeded(taskSettings, { forceRebuild: true }) - } - } + await this.refreshSiblingsAfterProfileSave(name, providerSettings) // Change the provider for the current task. // TODO: We should rename `buildApiHandler` for clarity (e.g. `getProviderClient`). @@ -2607,8 +2588,9 @@ export class ClineProvider } else { await this.updateGlobalState("listApiConfigMeta", await this.providerSettingsManager.listConfig()) - // A non-activating save still changes the profile's settings: refresh the views - // that buffered a copy, or their getState() keeps the pre-save values. + // A non-activating save still changes the profile's settings: refresh every view + // that buffered a copy (pinned siblings included), or their getState() keeps the + // pre-save values. if (this.pinnedProfileName === name) { await this._saveViewLocalStateFromMutation({ apiConfiguration: providerSettings }) // Only rebuild a task that actually runs under the saved profile. @@ -2616,6 +2598,7 @@ export class ClineProvider this.updateTaskApiHandlerIfNeeded(providerSettings, { forceRebuild: true }) } } + await this.refreshSiblingsAfterProfileSave(name, providerSettings) } if (signal.aborted) return id @@ -2623,6 +2606,33 @@ export class ClineProvider await this.postStateToWebview() return id } + /** + * Refresh the buffered overlay of every live sibling pinned to a saved profile and rebuild + * any task that follows it. The handler rebuild is independent of the pin: a task restored + * from history can follow profile A while its view pins profile B. + */ + private async refreshSiblingsAfterProfileSave(name: string, providerSettings: ProviderSettings): Promise { + for (const instance of ClineProvider.getAllInstances()) { + if (instance === this || instance._disposed || instance.pinnedProfileName !== name) { + continue + } + // Direct private access: compile-time safe across sibling instances. + await instance._saveViewLocalStateFromMutation({ apiConfiguration: providerSettings }) + } + + const { + name: _recordName, + id: _recordId, + ...taskSettings + } = providerSettings as ProviderSettings & { name?: string; id?: string } + for (const instance of ClineProvider.getAllInstances()) { + if (instance._disposed || instance.getCurrentTask()?.taskApiConfigName !== name) { + continue + } + instance.updateTaskApiHandlerIfNeeded(taskSettings, { forceRebuild: true }) + } + } + async deleteProviderProfile(profileToDelete: ProviderSettingsEntry) { const globalSettings = this.contextProxy.getValues() let profileToActivate: string | undefined = globalSettings.currentApiConfigName @@ -4013,8 +4023,13 @@ export class ClineProvider await this.contextProxy.resetAllState() - // Clear view-local state cache so getState() falls back to ContextProxy defaults. - this._clearViewLocalState() + // Clear every live view's buffer, not just this view's: a sibling's getConfiguration() + // would otherwise keep serving pre-reset selections. + for (const instance of ClineProvider.getAllInstances()) { + if (!instance._disposed) { + instance._clearViewLocalState() + } + } await this.providerSettingsManager.resetAllConfigs() await this.customModesManager.resetCustomModes() @@ -4024,7 +4039,9 @@ export class ClineProvider // stores, custom modes and the running task are already reset, so a viewStates write // error must not abort the reset half-way or skip the state post below. try { - await this.clearPersistedViewState() + // Serialize with profile saves: a save queued during the reset must not write this + // view's entry after it is cleared. + await this.enqueueProviderProfileMutation(() => this.clearPersistedViewState()) } catch (error) { this.log( `[resetState] Failed to clear the persisted view state: ${error instanceof Error ? error.message : String(error)}`, From ff26ffc8f4077ac4d330ad4bcb27612e7963f8ea Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 3 Nov 2026 01:35:02 +0800 Subject: [PATCH 18/25] fix(webview): restore a batch custom mode when createTask fails after registration Capture the modes the configuration overrides before registering them; if setValues or a later step rejects, re-install the captured modes and rethrow. --- src/core/webview/ClineProvider.ts | 68 +++++++++++++++++++++---------- 1 file changed, 46 insertions(+), 22 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index f159d3777d..9f4bce9c76 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -4185,6 +4185,14 @@ export class ClineProvider configuration: RooCodeSettings = {}, ): Promise { if (configuration) { + // Capture the modes this batch overrides before registering them: a later failure + // must restore a previously installed mode instead of leaving the batch's copy. + const savedCustomModes = configuration.customModes?.length + ? (await this.customModesManager.getCustomModes()).filter((mode) => + configuration.customModes!.some((candidate) => candidate.slug === mode.slug), + ) + : [] + // Register the batch's custom modes before setValues validates `mode`: a mode // selected in the same configuration is not known to the manager yet. if (configuration.customModes?.length) { @@ -4193,32 +4201,48 @@ export class ClineProvider } } - await this.setValues(configuration) + try { + await this.setValues(configuration) - if (configuration.allowedCommands) { - await vscode.workspace - .getConfiguration(Package.name) - .update("allowedCommands", configuration.allowedCommands, vscode.ConfigurationTarget.Global) - } + if (configuration.allowedCommands) { + await vscode.workspace + .getConfiguration(Package.name) + .update("allowedCommands", configuration.allowedCommands, vscode.ConfigurationTarget.Global) + } - if (configuration.deniedCommands) { - await vscode.workspace - .getConfiguration(Package.name) - .update("deniedCommands", configuration.deniedCommands, vscode.ConfigurationTarget.Global) - } + if (configuration.deniedCommands) { + await vscode.workspace + .getConfiguration(Package.name) + .update("deniedCommands", configuration.deniedCommands, vscode.ConfigurationTarget.Global) + } - if (configuration.commandExecutionTimeout !== undefined) { - await vscode.workspace - .getConfiguration(Package.name) - .update( - "commandExecutionTimeout", - configuration.commandExecutionTimeout, - vscode.ConfigurationTarget.Global, - ) - } + if (configuration.commandExecutionTimeout !== undefined) { + await vscode.workspace + .getConfiguration(Package.name) + .update( + "commandExecutionTimeout", + configuration.commandExecutionTimeout, + vscode.ConfigurationTarget.Global, + ) + } - if (configuration.currentApiConfigName) { - await this.setProviderProfile(configuration.currentApiConfigName) + if (configuration.currentApiConfigName) { + await this.setProviderProfile(configuration.currentApiConfigName) + } + } catch (error) { + // A failure after the registration must not leave the batch's modes installed; + // restore the ones that already existed. Newly added slugs cannot be deleted + // through the manager, so they are only reported. + for (const savedMode of savedCustomModes) { + await this.customModesManager.updateCustomMode(savedMode.slug, savedMode).catch((restoreError) => { + this.log( + `[createTask] Failed to restore custom mode '${savedMode.slug}' after the task creation failed: ${ + restoreError instanceof Error ? restoreError.message : String(restoreError) + }`, + ) + }) + } + throw error } } From a95c43cbdf921022d77b302558e093004baa6941 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Wed, 4 Nov 2026 03:16:18 +0800 Subject: [PATCH 19/25] fix(webview): own failed-write rollbacks per key and undo registered modes safely Review follow-up for #2236 (N4). - ContextProxy gains beginStateWriteGeneration/getStateWriteGeneration: the generation a key's next write will produce, and the one it holds now. Keys whose write path records no generation (a secret, or the pass-through taskHistory) report undefined, so their rollback keeps the value-only check. - ClineProvider.setValue and setValues capture that token before awaiting their write and pass it to _restoreSharedValuesAfterFailedViewWrite, which now rolls back a key only while it still holds the value the attempt wrote *and* the generation the attempt produced. A same-value write from another view during the per-view save no longer gets reverted to this attempt's earlier value. - createTask removes the custom-mode slugs a failed batch registered, through a new CustomModesManager.removeCustomModeEntry that edits the mode files and refreshes the merged state without deleteCustomMode's rules-folder deletion - that folder may predate the batch, and a rollback must not destroy it. - ContextProxy.updateGlobalState rejects "__proto__"/"constructor"/"prototype" keys, which a parsed payload can carry as own keys, instead of assigning them through a computed key and re-pointing the state cache's prototype; the rollback loop skips them too. Tests: ContextProxy 43 (3 new), CustomModesManager 50 (2 new), ClineProvider 360 (5 new). Each guard was proven by mutating the production line and observing the assertion failure; eslint, prettier and tsc are clean and no suppression count moved. --- src/core/config/ContextProxy.ts | 37 +++++ src/core/config/CustomModesManager.ts | 38 +++++ .../config/__tests__/ContextProxy.spec.ts | 53 +++++++ .../__tests__/CustomModesManager.spec.ts | 64 ++++++++ src/core/webview/ClineProvider.ts | 78 +++++++-- .../webview/__tests__/ClineProvider.spec.ts | 150 ++++++++++++++++++ 6 files changed, 407 insertions(+), 13 deletions(-) diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index e6aea60685..f3988af7eb 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -32,6 +32,13 @@ const PASS_THROUGH_STATE_KEYS = ["taskHistory"] export const isPassThroughStateKey = (key: string) => PASS_THROUGH_STATE_KEYS.includes(key) +// A parsed payload can carry these as own keys. Writing one through a computed assignment +// re-points the target object's prototype instead of storing a setting, so they are never +// accepted as state keys. +const UNSAFE_STATE_KEYS = ["__proto__", "constructor", "prototype"] + +export const isUnsafeStateKey = (key: string) => UNSAFE_STATE_KEYS.includes(key) + const globalSettingsExportSchema = globalSettingsSchema.omit({ taskHistory: true, listApiConfigMeta: true, @@ -365,6 +372,10 @@ export class ContextProxy { } async updateGlobalState(key: K, value: GlobalState[K]) { + if (isUnsafeStateKey(key)) { + return Promise.resolve() + } + if (isPassThroughStateKey(key)) { return this.originalContext.globalState.update(key, value) } @@ -386,6 +397,32 @@ export class ContextProxy { } } + /** + * The write generation the next `setValue`/`setValues` of this key will produce, or + * `undefined` when the key's write path records no generation at all - a secret, or a + * pass-through key that bypasses the state cache. Capture it before awaiting the write. + */ + public beginStateWriteGeneration(key: K): number | undefined { + if (!this.recordsWriteGeneration(key)) { + return undefined + } + + return (this.stateWriteGenerations.get(key as GlobalStateKey) ?? 0) + 1 + } + + /** + * Current write generation for a key, or `undefined` when its write path records none. A + * caller that may have to undo its own write compares it against the value captured from + * `beginStateWriteGeneration`: a newer write for the same key has already advanced it. + */ + public getStateWriteGeneration(key: K): number | undefined { + return this.recordsWriteGeneration(key) ? this.stateWriteGenerations.get(key as GlobalStateKey) : undefined + } + + private recordsWriteGeneration(key: K): boolean { + return !isSecretStateKey(key) && !isPassThroughStateKey(key) + } + private getAllGlobalState(): GlobalState { return Object.fromEntries(GLOBAL_STATE_KEYS.map((key) => [key, this.getGlobalState(key)])) } diff --git a/src/core/config/CustomModesManager.ts b/src/core/config/CustomModesManager.ts index 86bab479d7..75b0905f90 100644 --- a/src/core/config/CustomModesManager.ts +++ b/src/core/config/CustomModesManager.ts @@ -553,6 +553,44 @@ export class CustomModesManager { } } + /** + * Removes a mode's entry from the mode files and refreshes the merged state, without + * touching its rules folder. Rollback paths need this variant: `deleteCustomMode` also + * deletes `.roo/rules-` (or `~/.roo/rules-`), and a rollback cannot know + * whether that folder predates the batch whose registration it is undoing - deleting it + * would discard rules the user wrote earlier. + * + * Returns whether an entry was actually removed so a caller can report a miss instead of + * assuming cleanup happened. Errors propagate; rollback callers decide how loud to be. + */ + public async removeCustomModeEntry(slug: string): Promise { + const settingsPath = await this.getCustomModesFilePath() + const roomodesPath = await this.getWorkspaceRoomodes() + const projectModes = roomodesPath ? await this.loadModesFromFile(roomodesPath) : [] + const settingsModes = await this.loadModesFromFile(settingsPath) + const inProjectFile = projectModes.some((mode) => mode.slug === slug) + const inSettingsFile = settingsModes.some((mode) => mode.slug === slug) + + if (!inProjectFile && !inSettingsFile) { + return false + } + + await this.queueWrite(async () => { + if (roomodesPath && inProjectFile) { + await this.updateModesInFile(roomodesPath, (modes) => modes.filter((m) => m.slug !== slug)) + } + + if (inSettingsFile) { + await this.updateModesInFile(settingsPath, (modes) => modes.filter((m) => m.slug !== slug)) + } + + this.clearCache() + await this.refreshMergedState() + }) + + return true + } + /** * Deletes the rules folder for a specific mode * @param slug - The mode slug diff --git a/src/core/config/__tests__/ContextProxy.spec.ts b/src/core/config/__tests__/ContextProxy.spec.ts index a28ca249ac..042e138733 100644 --- a/src/core/config/__tests__/ContextProxy.spec.ts +++ b/src/core/config/__tests__/ContextProxy.spec.ts @@ -2,6 +2,8 @@ import * as vscode from "vscode" +import type { RooCodeSettings } from "@roo-code/types" + import { GLOBAL_STATE_KEYS, SECRET_STATE_KEYS, GLOBAL_SECRET_KEYS } from "@roo-code/types" import { clearAllMocks } from "../../../test-utils/reset" @@ -781,6 +783,57 @@ Output only the summary of the conversation so far, without any additional comme }) }) + describe("payload-shaped state keys", () => { + it("should refuse a prototype key carried by a parsed payload", async () => { + // JSON.parse gives an object an own "__proto__" key, and `setValues` iterates own keys. + // Accepting it would re-point the state cache's prototype instead of storing a setting, + // and every later read of a real key could then resolve through that prototype. + const payload = JSON.parse('{"__proto__":{"polluted":true},"mode":"code"}') as RooCodeSettings + + await proxy.setValues(payload) + + expect(proxy.getValue("mode")).toBe("code") + const stateCache = proxy["stateCache"] as Record + expect(Object.getPrototypeOf(stateCache)).toBe(Object.prototype) + expect(stateCache["polluted"]).toBeUndefined() + expect(({} as Record).polluted).toBeUndefined() + expect(mockGlobalState.update.mock.calls.some((call: unknown[]) => call[0] === "__proto__")).toBe(false) + }) + + it("should refuse the same key on the global-state write path", async () => { + // `updateGlobalState` is the function that performs the computed assignment, so it + // rejects the key itself rather than relying on its callers. + await proxy.updateGlobalState("__proto__" as never, { polluted: true } as never) + await proxy.updateGlobalState("constructor" as never, { polluted: true } as never) + + const stateCache = proxy["stateCache"] as Record + expect(Object.getPrototypeOf(stateCache)).toBe(Object.prototype) + expect(stateCache["polluted"]).toBeUndefined() + expect(mockGlobalState.update).not.toHaveBeenCalledWith("__proto__", expect.anything()) + }) + }) + + describe("state write generations", () => { + it("should report the generation the next global-state write produces", async () => { + expect(proxy.beginStateWriteGeneration("writeDelayMs")).toBe(1) + + await proxy.setValue("writeDelayMs", 10) + + expect(proxy.getStateWriteGeneration("writeDelayMs")).toBe(1) + expect(proxy.beginStateWriteGeneration("writeDelayMs")).toBe(2) + }) + + it("should record no generation for keys that bypass the state cache", () => { + // Secrets live in ExtensionContext.secrets and `taskHistory` is written straight + // through to global state. Neither path advances a generation, so a rollback of such a + // write keeps the value-only check instead of claiming a token it cannot honour. + expect(proxy.beginStateWriteGeneration("apiKey")).toBeUndefined() + expect(proxy.getStateWriteGeneration("apiKey")).toBeUndefined() + expect(proxy.beginStateWriteGeneration("taskHistory")).toBeUndefined() + expect(proxy.getStateWriteGeneration("taskHistory")).toBeUndefined() + }) + }) + describe("export", () => { it("should exclude viewStates from the exported settings", async () => { await proxy.setValue("viewStates", { diff --git a/src/core/config/__tests__/CustomModesManager.spec.ts b/src/core/config/__tests__/CustomModesManager.spec.ts index eddaec5621..4e9c797f54 100644 --- a/src/core/config/__tests__/CustomModesManager.spec.ts +++ b/src/core/config/__tests__/CustomModesManager.spec.ts @@ -764,6 +764,70 @@ describe("CustomModesManager", () => { }) }) + describe("removeCustomModeEntry", () => { + const wireSettingsFile = (initialModes: ModeConfig[]) => { + let settingsContent = { customModes: initialModes } + + ;(fs.readFile as Mock).mockImplementation(async (path: string) => { + if (path === mockSettingsPath) { + return yaml.stringify(settingsContent) + } + throw new Error("File not found") + }) + ;(fs.writeFile as Mock).mockImplementation(async (path: string, content: string) => { + if (path === mockSettingsPath) { + settingsContent = yaml.parse(content) + } + return Promise.resolve() + }) + ;(mockContext.globalState.update as Mock).mockImplementation((key: string, value: ModeConfig[]) => { + if (key === "customModes") { + settingsContent.customModes = value + } + return Promise.resolve() + }) + + return () => settingsContent.customModes + } + + it("removes the mode entry without deleting the mode's rules folder", async () => { + // The mode already had a rules folder on disk before this batch registered it - exactly + // the case a rollback has to survive. + ;(fileExistsAtPath as Mock).mockImplementation( + async (p: string) => p === mockSettingsPath || p === mockRoomodes || p.includes("rules-batch-mode"), + ) + const batchMode = { + slug: "batch-mode", + name: "Batch Mode", + roleDefinition: "Test role", + groups: ["read" as const], + source: "global" as const, + } + const readModes = wireSettingsFile([batchMode]) + + const removed = await manager.removeCustomModeEntry("batch-mode") + + expect(removed).toBe(true) + expect(readModes()).toHaveLength(0) + expect(mockContext.globalState.update).toHaveBeenCalledWith("customModes", []) + + // The discriminating assertion: `deleteCustomMode` reaches `fs.rm` for the rules folder. + // A rollback of a registration this process made must not delete rules the user wrote + // before it, so this path never touches them. + expect(fs.rm).not.toHaveBeenCalled() + }) + + it("reports a miss when no mode file holds the slug", async () => { + const readModes = wireSettingsFile([ + { slug: "keep-me", name: "Keep", roleDefinition: "r", groups: ["read" as const] }, + ]) + + expect(await manager.removeCustomModeEntry("absent-mode")).toBe(false) + expect(fs.writeFile).not.toHaveBeenCalled() + expect(readModes()).toHaveLength(1) + }) + }) + describe("updateModesInFile", () => { it("handles corrupted YAML content gracefully", async () => { const corruptedYaml = "customModes: [invalid yaml content" diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 276d4549cc..0a36c73cb8 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -4156,13 +4156,21 @@ export class ClineProvider public async setValue(key: K, value: RooCodeSettings[K]) { const previousSharedValues = { [key]: this.contextProxy.getValue(key) } as RooCodeSettings + // Sampled before the write is awaited: this write produces the next generation for the + // key, and a write that starts during the await produces the one after it. A key whose + // write path records no generation yields undefined and keeps the value-only check. + const writeGeneration = this.contextProxy.beginStateWriteGeneration(key as never) await this.contextProxy.setValue(key, value) try { await this._saveViewLocalStateFromMutation({ [key]: value } as ViewLocalStateValues) } catch (error) { - await this._restoreSharedValuesAfterFailedViewWrite(previousSharedValues, { - [key]: value, - } as RooCodeSettings) + await this._restoreSharedValuesAfterFailedViewWrite( + previousSharedValues, + { + [key]: value, + } as RooCodeSettings, + { [key]: writeGeneration }, + ) throw error } } @@ -4221,18 +4229,37 @@ export class ClineProvider * Restore shared values captured before a setter wrote them, used when the paired * per-view persistence failed so the shared store cannot move ahead of the per-view * record. A failed rollback is logged; the original failure is what callers see. + * + * `generations` carries the generation this attempt produced for each key it wrote. It is + * optional: a caller writing through a path that records no generation rolls back on the + * value check alone. */ private async _restoreSharedValuesAfterFailedViewWrite( previous: RooCodeSettings, attempted: RooCodeSettings, + generations: Record = {}, ): Promise { - // Roll back only the keys this attempt wrote and that still hold the value it wrote. - // A concurrent provider may have written a newer value in the meantime; restoring the - // captured previous value would then clobber it for every view that reads shared state. + // Roll back only the keys this attempt wrote, that still hold the value it wrote, and + // whose write generation is still the one this attempt produced. The value check alone is + // not enough: another provider can write the *same* value while this attempt's per-view + // save is in flight, and restoring the captured previous value would silently undo that + // newer write for every view that reads shared state. const rollback: RooCodeSettings = {} as RooCodeSettings - for (const key of Object.keys(attempted) as (keyof RooCodeSettings)[]) { - if (this.contextProxy.getValue(key) === attempted[key]) { + for (const keyName of Object.keys(attempted)) { + // `attempted` can originate in a parsed payload, and a parsed payload can carry + // "__proto__". Assigning that through a computed key would modify Object.prototype + // instead of recording a rollback entry. + if (keyName === "__proto__" || keyName === "constructor" || keyName === "prototype") { + continue + } + + const key = keyName as keyof RooCodeSettings + const generation = generations[keyName] + const ownsThisWrite = + generation === undefined || this.contextProxy.getStateWriteGeneration(key) === generation + + if (ownsThisWrite && this.contextProxy.getValue(key) === attempted[key]) { ;(rollback as Record)[key] = previous[key] } } @@ -4276,13 +4303,20 @@ export class ClineProvider const previousSharedValues = Object.fromEntries( Object.keys(sanitizedValues).map((key) => [key, this.contextProxy.getValue(key as keyof RooCodeSettings)]), ) as RooCodeSettings + // Per-key generations sampled before the batch is awaited: the batch produces the next + // generation for every key it writes, so a key another view writes - during the batch or + // during the per-view save below - ends up holding a later one. + const writeGenerations = Object.fromEntries( + Object.keys(sanitizedValues).map((key) => [key, this.contextProxy.beginStateWriteGeneration(key as never)]), + ) as Record await this.contextProxy.setValues(sanitizedValues) try { await this._saveViewLocalStateFromMutation(sanitizedValues) } catch (error) { // The per-view record did not land: restore the prior shared values so the two - // persistence layers cannot diverge, then surface the original failure. - await this._restoreSharedValuesAfterFailedViewWrite(previousSharedValues, sanitizedValues) + // persistence layers cannot diverge, then surface the original failure. Only keys this + // batch still owns are restored. + await this._restoreSharedValuesAfterFailedViewWrite(previousSharedValues, sanitizedValues, writeGenerations) throw error } } @@ -4948,9 +4982,9 @@ export class ClineProvider await this.setProviderProfile(configuration.currentApiConfigName) } } catch (error) { - // A failure after the registration must not leave the batch's modes installed; - // restore the ones that already existed. Newly added slugs cannot be deleted - // through the manager, so they are only reported. + // A failure after the registration must not leave the batch's modes installed: + // a slug that already existed is restored to its earlier definition, and a slug this + // batch introduced has no earlier definition to restore. for (const savedMode of savedCustomModes) { await this.customModesManager.updateCustomMode(savedMode.slug, savedMode).catch((restoreError) => { this.log( @@ -4960,6 +4994,24 @@ export class ClineProvider ) }) } + + // Removal uses `removeCustomModeEntry`, not `deleteCustomMode`: the public delete also + // removes `.roo/rules-` (`~/.roo/rules-` for a global mode) without knowing + // whether that folder predates this batch, and a rollback must not destroy rules the + // user wrote earlier. + for (const mode of configuration.customModes ?? []) { + if (savedCustomModes.some((saved) => saved.slug === mode.slug)) { + continue + } + + await this.customModesManager.removeCustomModeEntry(mode.slug).catch((removeError) => { + this.log( + `[createTask] Failed to remove the custom mode '${mode.slug}' registered by the failed task creation: ${ + removeError instanceof Error ? removeError.message : String(removeError) + }`, + ) + }) + } throw error } } diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 4c0a874e37..953ca6419a 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -6706,6 +6706,156 @@ describe("ClineProvider", () => { mockContext.globalState.update = originalUpdate await provider.dispose() }) + + it("should leave a shared value alone when another view wrote the same value meanwhile", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + await provider.contextProxy.setValue("mode", "architect") + const originalUpdate = mockContext.globalState.update + mockContext.globalState.update = vi.fn().mockImplementation(async (key: string) => { + if (key === "viewStates") { + // Another view selects the same mode while this provider's per-view save is in + // flight: the value matches this attempt, but the write does not belong to it. + await provider.contextProxy.setValue("mode", "code") + throw new Error("storage unavailable") + } + return Promise.resolve() + }) + + await expect(provider.setValue("mode", "code")).rejects.toThrow("storage unavailable") + + // The newer write stands. Restoring the captured "architect" would undo the other + // view's successful selection in every view that reads shared state. + expect(provider.getValue("mode")).toBe("code") + + mockContext.globalState.update = originalUpdate + await provider.dispose() + }) + it("should still roll back its own shared write when no newer write exists", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + await provider.contextProxy.setValue("mode", "architect") + const originalUpdate = mockContext.globalState.update + mockContext.globalState.update = vi.fn().mockImplementation((key: string) => { + if (key === "viewStates") { + return Promise.reject(new Error("storage unavailable")) + } + return Promise.resolve() + }) + + await expect(provider.setValue("mode", "code")).rejects.toThrow("storage unavailable") + + // The ownership token must not become a reason to skip a rollback this provider owns. + expect(provider.getValue("mode")).toBe("architect") + + mockContext.globalState.update = originalUpdate + await provider.dispose() + }) + it("should roll back only the batch keys it still owns after a failed per-view save", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + await provider.contextProxy.setValue("mode", "architect") + await provider.contextProxy.setValue("currentApiConfigName", "current-config") + const originalUpdate = mockContext.globalState.update + mockContext.globalState.update = vi.fn().mockImplementation(async (key: string) => { + if (key === "viewStates") { + await provider.contextProxy.setValue("mode", "code") + throw new Error("storage unavailable") + } + return Promise.resolve() + }) + + await expect(provider.setValues({ mode: "code", currentApiConfigName: "other-config" })).rejects.toThrow( + "storage unavailable", + ) + + // Key-level ownership, not batch-level: the mode a newer view selected survives, while the + // profile name this batch still owns is restored. + expect(provider.getValue("mode")).toBe("code") + expect(provider.getValue("currentApiConfigName")).toBe("current-config") + + mockContext.globalState.update = originalUpdate + await provider.dispose() + }) + it("should remove a custom mode the batch registered when the batch fails", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const existingMode = { slug: "existing-mode", name: "Existing", roleDefinition: "r", groups: ["read" as const] } + const newMode = { slug: "new-mode", name: "New", roleDefinition: "r", groups: ["read" as const] } + const manager = provider["customModesManager"] + Object.assign(manager, { + getCustomModes: vi.fn().mockResolvedValue([existingMode]), + updateCustomMode: vi.fn().mockResolvedValue(undefined), + removeCustomModeEntry: vi.fn().mockResolvedValue(true), + }) + vi.spyOn(provider, "setValues").mockRejectedValue(new Error("configuration write failed")) + + await expect( + provider.createTask("text", undefined, undefined, {}, { customModes: [existingMode, newMode] }), + ).rejects.toThrow("configuration write failed") + + // The slug that already existed is restored to its earlier definition; the slug this batch + // introduced is removed, and only through the rules-folder-safe path. + expect(manager.updateCustomMode).toHaveBeenCalledWith("existing-mode", existingMode) + expect(manager.removeCustomModeEntry).toHaveBeenCalledWith("new-mode") + expect(manager.removeCustomModeEntry).not.toHaveBeenCalledWith("existing-mode") + + await provider.dispose() + }) + + it("should keep a prototype key carried by a payload out of the rollback write", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + await provider.contextProxy.setValue("mode", "code") + // A payload-shaped value can carry an own "__proto__" next to the settings it names, and both + // maps handed to the rollback are built from those same keys. + const attempted = Object.fromEntries([ + ["__proto__", { polluted: true }], + ["mode", "code"], + ]) as RooCodeSettings + const previous = Object.fromEntries([ + ["__proto__", { polluted: true }], + ["mode", "architect"], + ]) as RooCodeSettings + const rollbackWrite = vi.spyOn(provider["contextProxy"], "setValues") + + await provider["_restoreSharedValuesAfterFailedViewWrite"](previous, attempted) + + expect(rollbackWrite).toHaveBeenCalledTimes(1) + const written = rollbackWrite.mock.calls[0][0] as Record + expect(Object.keys(written)).toEqual(["mode"]) + expect(Object.getPrototypeOf(written)).toBe(Object.prototype) + expect(written["polluted"]).toBeUndefined() + expect(({} as Record).polluted).toBeUndefined() + expect(provider.getValue("mode")).toBe("architect") + + await provider.dispose() + }) }) describe("webviewMessageHandler no-floating-promises coverage", () => { From 877ea0a004d5664453da42d79d7f96c7d5c3eed0 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 14:11:21 +0800 Subject: [PATCH 20/25] test(webview): split the profile-save overlay case and drop its duplicate copies Two of the review threads on this head measured true against e3ca230aa. The suite held two byte-identical (modulo indentation) copies of earlier cases at top level - the 33-line "should merge concurrent persisted updates from separate provider instances without lost viewStates" and the 17-line "should remove the buffered apiConfiguration when it is cleared" - alongside their in-describe originals, and it declared a second describe with the same name as one earlier in the file. Both copies were removed and the second describe, which only holds load/setValue/rollback cases, was renamed to "view-local buffer: loading, setValue mutations and rollback". The pinned-sibling overlay case claimed to exercise a non-activating save while passing activate=true; it now passes false, and a second case passes true so both save branches stay pinned. No production file changed. Controls (src/core/webview/ClineProvider.ts, each mutation reverted; its blob is d4c64fc201 before and after, verified with git checkout HEAD -- plus git hash-object): with production unchanged both cases pass; removing the else-branch refreshSiblingsAfterProfileSave call at ClineProvider.ts:2861 fails only the non-activating case (AssertionError: expected undefined to deeply equal { apiProvider: 'anthropic', ... }); removing the activating-branch call at ClineProvider.ts:2840 fails only the activated-profile case while the non-activating one still passes; setting the new case's flag to false also passes, which is why the flag now appears in both names rather than only in prose. vitest core/webview: 967 tests before (HEAD version of the spec, measured with the fix stashed) -> 966 after: two duplicate copies removed, one branch case added, all green. core/webview + core/config: 44 files / 1284 tests green. prettier --check, tsc --noEmit -p . and eslint --max-warnings=0 (spec and ClineProvider.ts) are clean; scope unchanged at 751 extension-package executable lines. --- .../webview/__tests__/ClineProvider.spec.ts | 92 ++++++++----------- 1 file changed, 37 insertions(+), 55 deletions(-) diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 12945b95db..1f7ffdc0cf 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -6527,41 +6527,7 @@ describe("ClineProvider", () => { }) }) - it("should merge concurrent persisted updates from separate provider instances without lost viewStates", async () => { - const provider1 = new ClineProvider( - mockContext, - mockOutputChannel, - "sidebar", - new ContextProxy(mockContext), - - new WebviewFocusTracker(), - ) - const provider2 = new ClineProvider( - mockContext, - mockOutputChannel, - "editor", - new ContextProxy(mockContext), - new WebviewFocusTracker(), - ) - - await provider1["setViewStateId"]("stable-sidebar-view") - await provider2["setViewStateId"]("stable-editor-view") - - await Promise.all([ - provider1.saveViewState("mode", "architect"), - provider2.saveViewState("currentApiConfigName", "editor-profile"), - ]) - - expect(mockContext.globalState.get("viewStates")).toMatchObject({ - "stable-sidebar-view": { mode: "architect" }, - "stable-editor-view": { currentApiConfigName: "editor-profile" }, - }) - - await provider1.dispose() - await provider2.dispose() - }) - - describe("view state persistence edge cases", () => { + describe("view-local buffer: loading, setValue mutations and rollback", () => { it("should load persisted mode, profile name and resolved profile into viewLocalState", async () => { const writer = new ClineProvider( mockContext, @@ -6737,24 +6703,6 @@ describe("ClineProvider", () => { await provider.dispose() }) - it("should remove the buffered apiConfiguration when it is cleared", async () => { - const provider = new ClineProvider( - mockContext, - mockOutputChannel, - "sidebar", - new ContextProxy(mockContext), - new WebviewFocusTracker(), - ) - const save = provider.saveViewState.bind(provider) as (key: string, value: unknown) => Promise - await provider.saveViewState("apiConfiguration", { apiProvider: providerIdentifiers.openrouter }) - await provider.saveViewState("apiConfiguration", undefined) - expect(provider["viewLocalState"]).not.toHaveProperty("apiConfiguration") - await provider.saveViewState("apiConfiguration", { apiProvider: providerIdentifiers.openrouter }) - await save("apiConfiguration", null) - expect(provider["viewLocalState"]).not.toHaveProperty("apiConfiguration") - await provider.dispose() - }) - it("should clear viewLocalState and the persisted entry when resetting state", async () => { const provider = new ClineProvider( mockContext, @@ -6812,7 +6760,7 @@ describe("ClineProvider", () => { await provider.dispose() }) - it("should refresh the buffered overlay of another view pinned to the saved profile", async () => { + it("should refresh the buffered overlay of another view pinned to the saved profile on a non-activating save", async () => { const saver = new ClineProvider( mockContext, mockOutputChannel, @@ -6836,7 +6784,7 @@ describe("ClineProvider", () => { } const providerSettings = { apiProvider: providerIdentifiers.anthropic, apiKey: "fresh-key" } - await saver.upsertProviderProfile("shared-profile", providerSettings, true) + await saver.upsertProviderProfile("shared-profile", providerSettings, false) // The pinned view buffered a copy of the profile's settings; a non-activating save must // refresh it, or its getState() keeps serving the pre-save configuration. @@ -6845,6 +6793,40 @@ describe("ClineProvider", () => { await pinned.dispose() }) + it("should refresh the buffered overlay of another view pinned to the activated profile", async () => { + const saver = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const pinned = new ClineProvider( + mockContext, + mockOutputChannel, + "editor", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + await pinned.setValue("currentApiConfigName", "shared-profile") + // @ts-ignore - The double only backs the profile-store calls of the save path. + saver.providerSettingsManager = { + saveConfig: vi.fn().mockResolvedValue("shared-id"), + listConfig: vi.fn().mockResolvedValue([{ name: "shared-profile", id: "shared-id" }]), + setModeConfig: vi.fn().mockResolvedValue(undefined), + } + const providerSettings = { apiProvider: providerIdentifiers.anthropic, apiKey: "fresh-key" } + + await saver.upsertProviderProfile("shared-profile", providerSettings, true) + + // The pinned view buffered a copy of the profile's settings; an activating save refreshes + // it through the sibling loop, so this row covers the activate=true branch that the case + // above no longer reaches. + expect(pinned["viewLocalState"].apiConfiguration).toEqual(providerSettings) + await saver.dispose() + await pinned.dispose() + }) + it("should roll back the shared writes when setValues per-view persistence fails", async () => { const provider = new ClineProvider( mockContext, From ec54bced5b59054522d8056ffeff6bb9f8a1f847 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 14:38:57 +0800 Subject: [PATCH 21/25] fix(webview): roll back custom-mode registrations that fail partway through a batch createTask captured the pre-batch snapshot of the mode slugs it was about to install and registered the batch's modes before the try block, so a registration that threw halfway (mode 2 of 3) escaped to the outer catch and none of the rollback ran: mode 1 stayed installed, either having overwritten a saved definition or added a slug the batch never finished using. The registration loop now runs inside the try, ahead of setValues, so the existing rollback - restore the captured definitions, remove the slugs this batch introduced, rethrow - covers it. (src/core/webview/ClineProvider.ts: the registration block was at 5013-5016 outside the try at 5019; it now sits at 5015-5021 inside it. 6253 -> 6255 lines.) Test: a new case in the buffer/rollback describe hands createTask two modes - one whose slug already exists under a different definition, one brand new - and rejects the second registration. It asserts the whole call sequence, so the restore cannot be confused with the registration: existing-mode/Overridden, new-mode/New, existing-mode/Existing, then that the new slug was removed through removeCustomModeEntry (never the saved slug) and that setValues never ran. Before the change the sequence stops at two entries. The pre-existing sibling case ("should remove a custom mode the batch registered when the batch fails") had the same blind spot: it re-registered the identical object it had snapshotted, so its restore assertion was satisfied by the registration call. It now hands in an overridden definition and asserts against the snapshot, which is the assertion it was meant to make. Strengthened, not weakened: emptying the rollback restore loop now fails it where it previously passed. Controls (ClineProvider.ts snapshotted at cafa6bca88 and restored by content hash after each): hoisting the registration back above the try fails the new case only (expected [ ...2 ] to deeply equal [ ...3 ]); moving it below setValues fails the new case through the setValuesSpy guard; emptying the restore loop fails both rollback cases; emptying the removal loop fails both through the removeCustomModeEntry guard. Gates: prettier --check clean; tsc --noEmit -p . clean; eslint --max-warnings=0 clean on both files; vitest core/webview + core/config 44 files / 1285 tests green (1284 before, +1 case). Scope unchanged at 751 extension-package executable lines (types 8): the change moves existing lines and adds coverage only. --- src/core/webview/ClineProvider.ts | 16 +++--- .../webview/__tests__/ClineProvider.spec.ts | 56 ++++++++++++++++++- 2 files changed, 62 insertions(+), 10 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index d4c64fc201..cafa6bca88 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -5008,15 +5008,17 @@ export class ClineProvider ) : [] - // Register the batch's custom modes before setValues validates `mode`: a mode - // selected in the same configuration is not known to the manager yet. - if (configuration.customModes?.length) { - for (const mode of configuration.customModes) { - await this.customModesManager.updateCustomMode(mode.slug, mode) - } - } + // Registration lives inside the try below: a mode whose write fails partway through + // the batch must trigger the same rollback as a failure after setValues. try { + // Register the batch's custom modes before setValues validates `mode`: a mode + // selected in the same configuration is not known to the manager yet. + if (configuration.customModes?.length) { + for (const mode of configuration.customModes) { + await this.customModesManager.updateCustomMode(mode.slug, mode) + } + } await this.setValues(configuration) if (configuration.allowedCommands) { diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 1f7ffdc0cf..cf9189d44d 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -6949,11 +6949,14 @@ describe("ClineProvider", () => { new ContextProxy(mockContext), new WebviewFocusTracker(), ) - const existingMode = { slug: "existing-mode", name: "Existing", roleDefinition: "r", groups: ["read" as const] } + const savedMode = { slug: "existing-mode", name: "Existing", roleDefinition: "r", groups: ["read" as const] } + // The batch hands in a different definition for the slug that already exists, so the restore + // call below cannot be satisfied by the registration call that preceded it. + const existingMode = { ...savedMode, name: "Overridden", roleDefinition: "changed" } const newMode = { slug: "new-mode", name: "New", roleDefinition: "r", groups: ["read" as const] } const manager = provider["customModesManager"] Object.assign(manager, { - getCustomModes: vi.fn().mockResolvedValue([existingMode]), + getCustomModes: vi.fn().mockResolvedValue([savedMode]), updateCustomMode: vi.fn().mockResolvedValue(undefined), removeCustomModeEntry: vi.fn().mockResolvedValue(true), }) @@ -6965,12 +6968,59 @@ describe("ClineProvider", () => { // The slug that already existed is restored to its earlier definition; the slug this batch // introduced is removed, and only through the rules-folder-safe path. - expect(manager.updateCustomMode).toHaveBeenCalledWith("existing-mode", existingMode) + expect(manager.updateCustomMode).toHaveBeenCalledWith("existing-mode", savedMode) expect(manager.removeCustomModeEntry).toHaveBeenCalledWith("new-mode") expect(manager.removeCustomModeEntry).not.toHaveBeenCalledWith("existing-mode") await provider.dispose() }) + it("should roll back the modes a registration left half-installed", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const savedMode = { slug: "existing-mode", name: "Existing", roleDefinition: "r", groups: ["read" as const] } + // The batch re-registers the slug that already exists under a different definition, so a + // restore call is distinguishable from the registration call. + const overriddenMode = { ...savedMode, name: "Overridden", roleDefinition: "changed" } + const newMode = { slug: "new-mode", name: "New", roleDefinition: "r", groups: ["read" as const] } + const calls: Array<[string, string]> = [] + const manager = provider["customModesManager"] + Object.assign(manager, { + getCustomModes: vi.fn().mockResolvedValue([savedMode]), + updateCustomMode: vi.fn((slug: string, mode: typeof savedMode) => { + calls.push([slug, mode.name]) + // The batch's second registration is the one that throws. + if (slug === "new-mode") { + return Promise.reject(new Error("mode registration failed")) + } + return Promise.resolve(undefined) + }), + removeCustomModeEntry: vi.fn().mockResolvedValue(true), + }) + const setValuesSpy = vi.spyOn(provider, "setValues").mockResolvedValue(undefined) + + await expect( + provider.createTask("text", undefined, undefined, {}, { customModes: [overriddenMode, newMode] }), + ).rejects.toThrow("mode registration failed") + + // Registration order first, then the rollback: the slug that already existed is rewritten to + // the snapshot taken before the batch, and the slug this batch introduced is removed through + // the rules-folder-safe path. Nothing downstream of the failed registration runs. + expect(calls).toEqual([ + ["existing-mode", "Overridden"], + ["new-mode", "New"], + ["existing-mode", "Existing"], + ]) + expect(manager.removeCustomModeEntry).toHaveBeenCalledWith("new-mode") + expect(manager.removeCustomModeEntry).not.toHaveBeenCalledWith("existing-mode") + expect(setValuesSpy).not.toHaveBeenCalled() + + await provider.dispose() + }) it("should keep a prototype key carried by a payload out of the rollback write", async () => { const provider = new ClineProvider( From 509f92ccd2224d9c5688ebf4cee49ffc0c8c5a27 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 18:19:25 +0800 Subject: [PATCH 22/25] test(webview): spy the profile and modes methods instead of suppressing the manager swaps The view-state tests replaced whole managers on a readonly field, and every one of those assignments needed a @ts-ignore: ten of them, at the profile-lookup helper, the failing-lookup and superseded-view cases, the empty-custom-modes and resetCustomModes cases, the profile-store save paths and the write-ownership case. The methods they stub are public on both managers, so the stubs now go on the methods with vi.spyOn and the directives go away: 17 suppression directives in the file, 7 left, all seven pre-existing and outside this PR's diff. The profile-lookup helper's stalled profile is now typed from the manager's own resolved type - Awaited> - so the stubs are checked against the real signature with no assertion and no directive; keeping id optional there is deliberate, because the doubles omit it today and the load path strips name and keeps the rest as the buffered apiConfiguration, so an id that the doubles do not supply would show up in what those tests assert. Diff: one file, 34 insertions, 39 deletions. 39 deleted lines are 10 directives, the ten replacement-object blocks and the hand-written profile fields; not one deleted line is an it(...) or an expect(...), so this is deleted-duplicate 0 and deleted-coverage 0. Test-only: it contributes no changed executable lines to the mutation-diff buckets. Two controls, run rather than described. Remove the getProfile spy that the profile-lookup helper installs and core/webview goes red: Test Files 1 failed | 33 passed (34), Tests 5 failed | 962 passed (967) - the helper stub is load-bearing. Remove the three profile-store stubs on the saver manager in the two sibling-overlay cases and the suite stays green: 34 files, 967 tests. That is a real finding rather than a clean bill: those three stubs are not what makes those two cases pass today - the real manager against the mocked memento produces the same viewLocalState - so they are kept as isolation of intent, not sold as coverage, and the question of whether those cases should assert on the store calls at all is a question for the reviewer rather than something to hide behind a green run. With the stubs restored: 34 files, 967 tests, 0 failures; tsc -p . --noEmit exit 0; eslint --max-warnings=0 --no-warn-ignored on the file exit 0; prettier --check exit 0. Two tool artefacts measured on the way. eslint --prune-suppressions rewrote src/eslint-suppressions.json with 1441 added and 1441 removed lines, dropping entries for files outside this diff (anthropic-vertex.spec.ts and others); the spec's own count stayed 195, so the pruning was unrelated cleanup and the file was reverted to HEAD rather than shipped as a side effect of checking one file. And my inserted listConfig stub exceeded the print width, so the edited file genuinely failed prettier --check while the HEAD copy of the same file passed it: formatting was applied and the gates re-run, which is the difference between a file that needed work and the junction worktree's whole-tree prettier noise. Review row: database id 4477029124 (Trivial, Maintainability & Code Quality). --- .../webview/__tests__/ClineProvider.spec.ts | 73 +++++++++---------- 1 file changed, 34 insertions(+), 39 deletions(-) diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index cf9189d44d..9cf7c02317 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -30,6 +30,7 @@ import { defaultModeSlug } from "../../../shared/modes" import { experimentDefault } from "../../../shared/experiments" import { setTtsEnabled } from "../../../utils/tts" import { ContextProxy } from "../../config/ContextProxy" +import type { ProviderSettingsManager } from "../../config/ProviderSettingsManager" import { WorkspaceIndexingEnablementManager } from "../../../services/code-index/workspace-indexing-enablement-manager" import { Task, TaskOptions } from "../../task/Task" import { safeWriteJson } from "../../../utils/safeWriteJson" @@ -426,11 +427,10 @@ afterAll(() => { vi.restoreAllMocks() }) -type StalledProfile = { - name: string - apiProvider?: string - openRouterModelId?: string -} +// The stalled profile is typed from the manager's own return type so the profile spies below need no +// assertion, and the shape stays what a real getProfile resolves to (id optional, so the doubles that omit +// it keep matching what the load path receives). +type StalledProfile = Awaited> function stallProviderSettingsProfile(provider: ClineProvider) { let resolveProfile: (value: StalledProfile) => void = () => {} @@ -445,8 +445,10 @@ function stallProviderSettingsProfile(provider: ClineProvider) { } return new Promise((resolve) => (resolveProfile = resolve)) }) - // @ts-ignore - Reassign the readonly providerSettingsManager for the test; the double only backs getProfile. - provider.providerSettingsManager = { getProfile: getProfileSpy } + // The manager field is readonly and getProfile is the only member this scenario reaches, so the spy goes on the + // method rather than onto a replacement manager: the double stays checked against the real signature, the + // one-call guard below still fails loudly on a second lookup, and no suppression directive is needed. + vi.spyOn(provider.providerSettingsManager, "getProfile").mockImplementation(() => getProfileSpy()) // Return a stable wrapper around the closure binding: resolveProfile is // reassigned to the pending promise's resolver once getProfile is called, // so returning the variable directly would hand the test the initial no-op. @@ -6181,8 +6183,7 @@ describe("ClineProvider", () => { new WebviewFocusTracker(), ) const logSpy = vi.spyOn(provider, "log") - // @ts-ignore - Replace providerSettingsManager with a failing test double. - provider.providerSettingsManager = { getProfile: vi.fn().mockRejectedValue(new Error("profile missing")) } + vi.spyOn(provider.providerSettingsManager, "getProfile").mockRejectedValue(new Error("profile missing")) await provider.saveViewState("currentApiConfigName", "my-profile") await provider["setViewStateId"]("stable-sidebar-view") expect(provider["viewLocalState"].currentApiConfigName).toBe("my-profile") @@ -6200,13 +6201,10 @@ describe("ClineProvider", () => { new WebviewFocusTracker(), ) const logSpy = vi.spyOn(provider, "log") - // @ts-ignore - Replace providerSettingsManager with a test double that registers a newer id. - provider.providerSettingsManager = { - getProfile: vi.fn().mockImplementation(() => { - provider["viewStateId"] = "superseded-view" - return Promise.resolve({ name: "my-profile", apiProvider: providerIdentifiers.openrouter }) - }), - } + vi.spyOn(provider.providerSettingsManager, "getProfile").mockImplementation(() => { + provider["viewStateId"] = "superseded-view" + return Promise.resolve({ name: "my-profile", apiProvider: providerIdentifiers.openrouter }) + }) await provider.saveViewState("currentApiConfigName", "my-profile") await provider["setViewStateId"]("stable-sidebar-view") expect(provider["viewLocalState"]).not.toHaveProperty("apiConfiguration") @@ -6224,8 +6222,7 @@ describe("ClineProvider", () => { new WebviewFocusTracker(), ) const logSpy = vi.spyOn(provider, "log") - // @ts-ignore - Replace customModesManager with a test double (no custom modes). - provider.customModesManager = { getCustomModes: vi.fn().mockResolvedValue([]), dispose: vi.fn() } + vi.spyOn(provider.customModesManager, "getCustomModes").mockResolvedValue([]) // The file-level modes mock resolves every slug to a mode; drop the pinned slug. const modesModule = vi.mocked(await import("../../../shared/modes")) const originalMode = modesModule.getModeBySlug("code") @@ -6256,8 +6253,7 @@ describe("ClineProvider", () => { new WebviewFocusTracker(), ) const { getProfile: getProfileSpy, resolveProfile } = stallProviderSettingsProfile(provider) - // @ts-ignore - Replace customModesManager with a test double (no custom modes). - provider.customModesManager = { getCustomModes: vi.fn().mockResolvedValue([]), dispose: vi.fn() } + vi.spyOn(provider.customModesManager, "getCustomModes").mockResolvedValue([]) await provider.saveViewState("mode", "code") await provider.saveViewState("currentApiConfigName", "cfg-a") const load = provider["setViewStateId"]("stable-sidebar-view") @@ -6548,13 +6544,11 @@ describe("ClineProvider", () => { new WebviewFocusTracker(), ) const logSpy = vi.spyOn(provider, "log") - const getProfileSpy = vi.fn().mockResolvedValue({ + const getProfileSpy = vi.spyOn(provider.providerSettingsManager, "getProfile").mockResolvedValue({ name: "my-profile", apiProvider: providerIdentifiers.openrouter, openRouterModelId: "model-x", }) - // @ts-ignore - Replace providerSettingsManager with a test double for the profile lookup. - provider.providerSettingsManager = { getProfile: getProfileSpy } // The ownership check requires the writer to release the id before the // next view adopts it; the durable entry it wrote stays in storage. await writer.dispose() @@ -6654,8 +6648,7 @@ describe("ClineProvider", () => { new WebviewFocusTracker(), ) const logSpy = vi.spyOn(provider, "log") - // @ts-ignore - Replace customModesManager with a test double (no custom modes). - provider.customModesManager = { getCustomModes: vi.fn().mockResolvedValue([]), dispose: vi.fn() } + vi.spyOn(provider.customModesManager, "getCustomModes").mockResolvedValue([]) // The file-level modes mock resolves every slug to a mode; narrow it to the slugs under test. const modesModule = vi.mocked(await import("../../../shared/modes")) const originalMode = modesModule.getModeBySlug("code") @@ -6712,8 +6705,8 @@ describe("ClineProvider", () => { new WebviewFocusTracker(), ) vi.spyOn(provider, "postStateToWebview").mockResolvedValue(undefined) - // @ts-ignore - Replace customModesManager with a test double (the real reset writes to disk). - provider.customModesManager = { resetCustomModes: vi.fn().mockResolvedValue(undefined), dispose: vi.fn() } + // The real resetCustomModes writes the modes file, so the method is stubbed rather than the manager replaced. + vi.spyOn(provider.customModesManager, "resetCustomModes").mockResolvedValue(undefined) // The modal answer is a string label; the last-typed vscode overload expects a // MessageItem. A double assertion is the last-resort cast here (AGENTS.md): the // production path compares the answer against the string label directly, so the @@ -6776,12 +6769,13 @@ describe("ClineProvider", () => { new WebviewFocusTracker(), ) await pinned.setValue("currentApiConfigName", "shared-profile") - // @ts-ignore - The double only backs the profile-store calls of the save path. - saver.providerSettingsManager = { - saveConfig: vi.fn().mockResolvedValue("shared-id"), - listConfig: vi.fn().mockResolvedValue([{ name: "shared-profile", id: "shared-id" }]), - setModeConfig: vi.fn().mockResolvedValue(undefined), - } + // Only these three profile-store calls are exercised by the save path, so each is stubbed on the real manager + // instead of replacing it, which keeps the stubs checked against the declared signatures. + vi.spyOn(saver.providerSettingsManager, "saveConfig").mockResolvedValue("shared-id") + vi.spyOn(saver.providerSettingsManager, "listConfig").mockResolvedValue([ + { name: "shared-profile", id: "shared-id" }, + ]) + vi.spyOn(saver.providerSettingsManager, "setModeConfig").mockResolvedValue(undefined) const providerSettings = { apiProvider: providerIdentifiers.anthropic, apiKey: "fresh-key" } await saver.upsertProviderProfile("shared-profile", providerSettings, false) @@ -6809,12 +6803,13 @@ describe("ClineProvider", () => { new WebviewFocusTracker(), ) await pinned.setValue("currentApiConfigName", "shared-profile") - // @ts-ignore - The double only backs the profile-store calls of the save path. - saver.providerSettingsManager = { - saveConfig: vi.fn().mockResolvedValue("shared-id"), - listConfig: vi.fn().mockResolvedValue([{ name: "shared-profile", id: "shared-id" }]), - setModeConfig: vi.fn().mockResolvedValue(undefined), - } + // Only these three profile-store calls are exercised by the save path, so each is stubbed on the real manager + // instead of replacing it, which keeps the stubs checked against the declared signatures. + vi.spyOn(saver.providerSettingsManager, "saveConfig").mockResolvedValue("shared-id") + vi.spyOn(saver.providerSettingsManager, "listConfig").mockResolvedValue([ + { name: "shared-profile", id: "shared-id" }, + ]) + vi.spyOn(saver.providerSettingsManager, "setModeConfig").mockResolvedValue(undefined) const providerSettings = { apiProvider: providerIdentifiers.anthropic, apiKey: "fresh-key" } await saver.upsertProviderProfile("shared-profile", providerSettings, true) From 492cfb3d28d86e19c38124b22f2887ae41c138b8 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 19:26:14 +0800 Subject: [PATCH 23/25] fix(config): refuse secret writes during a reset and clear the guard when it fails This branch had no write guard at all. There was no reset flag, storeSecret() had nothing telling it a reset was in progress, and resetAllState() cleared the two caches, advanced the state-write generations, deleted every global-state and secret key in a single Promise.all and reinitialized - with the success path as the only path. So a save issued while a reset was still inside its deletes could land after the deletes and put back a value the user had just erased. This commit introduces that guard; it is not repairing an existing one, and a reviewer reading this PR should read the refusal as new behaviour rather than as a bug fix to code that already refused writes. What lands: a resetting field, a rejection at the top of storeSecret() (SettingsWriteError with the key in failed, so a caller can tell a refused save from a committed one), resetting raised as the first statement of resetAllState() and lowered in a finally wrapping the whole body. The body itself is unchanged: this lineage has no pending-write barrier and no write-owner clears, and importing #2235's would have imported a different unit's design rather than answering this unit's defect. Two cases, because the guard has two halves and the refusal is the new observable on this branch, so it is tested on its own rather than as a phase of the leak case: - refuses a secret write issued while a reset is still inside its delete pass - accepts a secret write again after a reset whose delete pass rejected Assertions read captured call arguments (mockSecrets.store.mock.calls filtered on the key) and the cache and storage read-backs, not toHaveBeenCalledWith: that matcher compares the way toEqual does and drops undefined-valued keys, so a loose matcher would pass on a tree where nothing is refused at all. Controls, measured in four steps rather than described. Baseline at 509f92ccd2: Test Files 10 passed (10), Tests 318 passed (318). Refusal case alone with production untouched: 1 failed | 318 passed (319), AssertionError: expected undefined to be an instance of SettingsWriteError - with no refusal the mid-reset save simply succeeds. Guard and refusal with the flag lowered only on the success path: 10 passed (10), 319 passed (319), which is the shape this branch would have had if the flag were added without a finally. Leak case then added against exactly that shape - the negative control for the finally - fails 1 of 320 with the production consequence in the message: promise rejected "SettingsWriteError: Could not store apiKey: secret writes are refused while state is being reset" instead of resolving. With the try/finally: Test Files 10 passed (10), Tests 320 passed (320), identical before and after prettier. gates: tsc -p . --noEmit exit 0, eslint --max-warnings=0 --no-warn-ignored on both files exit 0, and the recorded suppression counts for the two touched files are unchanged at 1 and 5 across a --prune-suppressions run; prettier --check exit 0 on the working copies and exit 0 on the HEAD copies fed through the same invocation, so a format failure here would be this commit's. src/eslint-suppressions.json untouched. Size posture, with the pair printed rather than recalled. scripts/stryker-diff.mjs ci --base e943530c848624bd3054c960be1e4fb0b658509a (main's tip, this PR's recorded base) --head 509f92ccd2224d9c5688ebf4cee49ffc0c8c5a27 (this commit's parent) reports the extension bucket at 500 changed executable lines - exactly at MAX_CHANGED_LINES 500, with nothing to spare before this commit. The same command on the head being pushed here reports 518, so this commit is what starts the gate failing on #2236 rather than a commit continuing a failure the branch already had. The diff is 28 added / 14 deleted in src/core/config/ContextProxy.ts plus 69 lines in its spec, and the spec half is excluded from the bucket by the /__tests__/ segment at scripts/stryker-diff.mjs:85. Neither run could start Stryker locally (spawnSync node_modules\.bin\stryker ENOENT for both packages), so no mutants were scored here and only the size line is a real measurement; it belongs in the PR body or the maintainers' queue rather than being argued in a commit on a branch declared final. This is not an answer to a review row on this PR: the one live row here (ClineProvider.ts:5024, root comment 4477459238) is a custom-modes write-path question and is being read against this head separately. It ports the answer measured on vps2/n2-viewstates-durability for the same defect in this file so that the three sibling units do not ship three different answers to one bug. --- src/core/config/ContextProxy.ts | 42 +++++++---- .../config/__tests__/ContextProxy.spec.ts | 69 +++++++++++++++++++ 2 files changed, 97 insertions(+), 14 deletions(-) diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index f81c77a5bb..8cb5cf6973 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -86,6 +86,9 @@ export class ContextProxy { private stateWriteGenerations = new Map() private secretCache: SecretState private _isInitialized = false + // Raised for the whole duration of a reset and cleared in a finally, so a reset that fails + // mid-way cannot leave the store refusing every later secret write; see resetAllState. + private resetting = false constructor(context: vscode.ExtensionContext) { this.originalContext = context @@ -486,6 +489,13 @@ export class ContextProxy { } storeSecret(key: SecretStateKey, value?: string) { + // Refused rather than deferred: a deferred secret write would re-store exactly the value the + // user asked to erase, and a silently dropped one would lose the save. + if (this.resetting) { + return Promise.reject( + new SettingsWriteError([], [key], new Error("secret writes are refused while state is being reset")), + ) + } // Update cache. const owner = {} this.secretWriteOwner.set(key, owner) @@ -759,22 +769,26 @@ export class ContextProxy { * @returns A promise that resolves when all reset operations are complete */ public async resetAllState() { - // Clear in-memory caches - this.stateCache = {} - this.secretCache = {} - // A write that started before this reset must not restore its pre-reset value when it - // rejects afterwards, so advance every generation the pending writes still hold. - for (const key of this.stateWriteGenerations.keys()) { - this.stateWriteGenerations.set(key, (this.stateWriteGenerations.get(key) ?? 0) + 1) - } + this.resetting = true + try { + this.stateCache = {} + this.secretCache = {} + // A write that started before this reset must not restore its pre-reset value when it + // rejects afterwards, so advance every generation the pending writes still hold. + for (const key of this.stateWriteGenerations.keys()) { + this.stateWriteGenerations.set(key, (this.stateWriteGenerations.get(key) ?? 0) + 1) + } - await Promise.all([ - ...GLOBAL_STATE_KEYS.map((key) => this.originalContext.globalState.update(key, undefined)), - ...SECRET_STATE_KEYS.map((key) => this.originalContext.secrets.delete(key)), - ...GLOBAL_SECRET_KEYS.map((key) => this.originalContext.secrets.delete(key)), - ]) + await Promise.all([ + ...GLOBAL_STATE_KEYS.map((key) => this.originalContext.globalState.update(key, undefined)), + ...SECRET_STATE_KEYS.map((key) => this.originalContext.secrets.delete(key)), + ...GLOBAL_SECRET_KEYS.map((key) => this.originalContext.secrets.delete(key)), + ]) - await this.initialize() + await this.initialize() + } finally { + this.resetting = false + } } private static _instance: ContextProxy | null = null diff --git a/src/core/config/__tests__/ContextProxy.spec.ts b/src/core/config/__tests__/ContextProxy.spec.ts index 386c435829..468965a0b8 100644 --- a/src/core/config/__tests__/ContextProxy.spec.ts +++ b/src/core/config/__tests__/ContextProxy.spec.ts @@ -162,6 +162,75 @@ describe("ContextProxy", () => { // that storage no longer holds. expect(proxy.getGlobalState("apiProvider")).toBeUndefined() }) + // This branch carried no write guard at all before these two cases: resetAllState cleared the + // caches and deleted storage with nothing telling storeSecret that a reset was running, so a + // save issued mid-reset could land after the deletes and resurrect a value the user had just + // erased. The refusal is the new observable here rather than an existing one, so it gets its + // own case instead of being a phase of the leak case. + it("refuses a secret write issued while a reset is still inside its delete pass", async () => { + const erases: { reject: (error: Error) => void }[] = [] + mockSecrets.delete.mockImplementation((key: string) => { + if (key === "apiKey") { + return new Promise((_, reject) => { + erases.push({ reject }) + }) + } + return Promise.resolve() + }) + + const resetting = proxy.resetAllState() + // The reset yields before it reaches its delete pass, so let it get there before asking the + // store to write. + await new Promise((resolve) => { + setTimeout(resolve, 10) + }) + expect(erases.length).toBe(1) + + const refused = await proxy.storeSecret("apiKey", "saved-mid-reset").then( + () => undefined, + (error: unknown) => error, + ) + expect(refused).toBeInstanceOf(SettingsWriteError) + expect(String((refused as Error).message)).toContain("secret writes are refused while state is being reset") + // Refused rather than deferred: a deferred write would re-store the value this reset is + // in the middle of erasing. + // Refused rather than deferred, and not silently written either: the cache the reset already + // cleared stays empty and no store call was made for the key. + expect(proxy.getSecret("apiKey")).toBeUndefined() + expect(mockSecrets.store.mock.calls.filter((call: readonly unknown[]) => call[0] === "apiKey").length).toBe( + 0, + ) + + erases.forEach((erase) => { + erase.reject(new Error("keychain locked")) + }) + await expect(resetting).rejects.toThrow("keychain locked") + }) + it("accepts a secret write again after a reset whose delete pass rejected", async () => { + const durable = new Map() + mockSecrets.get.mockImplementation(async (key: string) => durable.get(key)) + mockSecrets.store.mockImplementation(async (key: string, value: string) => { + durable.set(key, value) + }) + mockSecrets.delete.mockImplementation(async (key: string) => { + if (key === "apiKey") { + throw new Error("keychain locked") + } + durable.delete(key) + }) + + await expect(proxy.resetAllState()).rejects.toThrow("keychain locked") + + // The half that bites: the delete pass is a single Promise.all, so one keychain rejection + // rejects resetAllState itself. Unless the guard is lowered on that path too, every later + // save is refused for the life of the instance while resetState() has already aborted on the + // same error, so the user cannot get a key back without restarting the extension host. + // Measured as a save that resolves and reads back through storage and cache, not as an + // absence of errors. + await expect(proxy.storeSecret("apiKey", "written-after-failed-reset")).resolves.toBeUndefined() + expect(await mockSecrets.get("apiKey")).toBe("written-after-failed-reset") + expect(proxy.getSecret("apiKey")).toBe("written-after-failed-reset") + }) it("should bypass cache for pass-through state keys", async () => { // Setup mock return value From 610b6de29c15e657ec409f215e48c3979f3066ac Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 6 Nov 2026 00:25:43 +0800 Subject: [PATCH 24/25] fix(config): single-flight the reset and settle every delete before the guard drops Two windows in resetAllState, both about the guard rather than the deletes themselves: - Overlapping resets shared one boolean. The first pass to finish lowered `resetting` while the second was still deleting, so a storeSecret admitted in that gap could be erased by the pass still running. Overlapping callers now join the pass already in flight. - Promise.all rejected on the first failed delete while the others were still pending, and the finally lowered the guard at that moment. The pass now settles every delete and rethrows the first failure. ContextProxy.spec 72 passed, tsc --noEmit 0 errors. --- src/core/config/ContextProxy.ts | 58 +++++++++++++++++++++++---------- 1 file changed, 40 insertions(+), 18 deletions(-) diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index 8cb5cf6973..64023b9b9c 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -89,6 +89,9 @@ export class ContextProxy { // Raised for the whole duration of a reset and cleared in a finally, so a reset that fails // mid-way cannot leave the store refusing every later secret write; see resetAllState. private resetting = false + // A reset that is already running. Overlapping callers join it instead of starting a second + // delete pass whose end could lower the guard while the first one is still writing. + private resetInFlight: Promise | null = null constructor(context: vscode.ExtensionContext) { this.originalContext = context @@ -768,27 +771,46 @@ export class ContextProxy { * This clears all data from both the in-memory caches and the VSCode storage. * @returns A promise that resolves when all reset operations are complete */ - public async resetAllState() { + public resetAllState(): Promise { + // Two resets can overlap (a second webview, or the same command twice). A boolean guard lets the + // first pass to finish lower it while the second is still deleting, and a secret stored in that + // gap is erased by the pass that is still running - so overlapping callers join the running pass. + if (this.resetInFlight) { + return this.resetInFlight + } + this.resetting = true - try { - this.stateCache = {} - this.secretCache = {} - // A write that started before this reset must not restore its pre-reset value when it - // rejects afterwards, so advance every generation the pending writes still hold. - for (const key of this.stateWriteGenerations.keys()) { - this.stateWriteGenerations.set(key, (this.stateWriteGenerations.get(key) ?? 0) + 1) - } + const pass = (async () => { + try { + this.stateCache = {} + this.secretCache = {} + // A write that started before this reset must not restore its pre-reset value when it + // rejects afterwards, so advance every generation the pending writes still hold. + for (const key of this.stateWriteGenerations.keys()) { + this.stateWriteGenerations.set(key, (this.stateWriteGenerations.get(key) ?? 0) + 1) + } - await Promise.all([ - ...GLOBAL_STATE_KEYS.map((key) => this.originalContext.globalState.update(key, undefined)), - ...SECRET_STATE_KEYS.map((key) => this.originalContext.secrets.delete(key)), - ...GLOBAL_SECRET_KEYS.map((key) => this.originalContext.secrets.delete(key)), - ]) + // Promise.all rejects on the first failure while the other updates and deletes are still + // pending, and the finally below would then admit a storeSecret that a still-running + // delete erases. Settle every delete first, then surface the first failure. + const deletions = await Promise.allSettled([ + ...GLOBAL_STATE_KEYS.map((key) => this.originalContext.globalState.update(key, undefined)), + ...SECRET_STATE_KEYS.map((key) => this.originalContext.secrets.delete(key)), + ...GLOBAL_SECRET_KEYS.map((key) => this.originalContext.secrets.delete(key)), + ]) + const deletionFailure = deletions.find((result) => result.status === "rejected") + if (deletionFailure) { + throw deletionFailure.reason + } - await this.initialize() - } finally { - this.resetting = false - } + await this.initialize() + } finally { + this.resetting = false + this.resetInFlight = null + } + })() + this.resetInFlight = pass + return pass } private static _instance: ContextProxy | null = null From 41bd74287d725002f7c371ee737592d0585ead4e Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 6 Nov 2026 00:28:09 +0800 Subject: [PATCH 25/25] fix(webview): restore both source definitions of a custom mode on rollback The batch rollback snapshotted getCustomModes(), which hides a global definition behind the project mode of the same slug. A batch that overwrote the global copy through that slug then restored only the project copy, leaving the global definition permanently replaced. The snapshot now reads both source files (getCustomModeDefinitions) and restores each entry to its own file. New test: a slug that exists as a project and a global mode must come back as both, and the merged read must not be enough to satisfy it. ClineProvider.spec 383 + CustomModesManager.spec 50 passed, tsc and eslint clean. --- src/core/config/CustomModesManager.ts | 22 ++++++++ src/core/webview/ClineProvider.ts | 7 ++- .../webview/__tests__/ClineProvider.spec.ts | 54 +++++++++++++++++++ 3 files changed, 81 insertions(+), 2 deletions(-) diff --git a/src/core/config/CustomModesManager.ts b/src/core/config/CustomModesManager.ts index 75b0905f90..8728696bf7 100644 --- a/src/core/config/CustomModesManager.ts +++ b/src/core/config/CustomModesManager.ts @@ -353,6 +353,28 @@ export class CustomModesManager { } } + /** + * Every definition of a mode, per source file, without the project-over-global hiding that + * `getCustomModes` applies. A rollback has to restore both: a batch that overwrites a global + * definition through a slug that also exists as a project mode would otherwise leave the global + * copy permanently replaced by the batch's version. + */ + public async getCustomModeDefinitions(slugs: readonly string[]): Promise { + const wanted = new Set(slugs) + const settingsPath = await this.getCustomModesFilePath() + const settingsModes = await this.loadModesFromFile(settingsPath) + const roomodesPath = await this.getWorkspaceRoomodes() + const roomodesModes = roomodesPath ? await this.loadModesFromFile(roomodesPath) : [] + + return [ + ...roomodesModes + .filter((mode) => wanted.has(mode.slug)) + .map((mode) => ({ ...mode, source: "project" as const })), + ...settingsModes + .filter((mode) => wanted.has(mode.slug)) + .map((mode) => ({ ...mode, source: "global" as const })), + ] + } public async getCustomModes(): Promise { // Check if we have a valid cached result. const now = Date.now() diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index cafa6bca88..8e3b9b884a 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -5002,9 +5002,12 @@ export class ClineProvider if (configuration) { // Capture the modes this batch overrides before registering them: a later failure // must restore a previously installed mode instead of leaving the batch's copy. + // Snapshot every definition of every slug this batch touches, one per source file. Filtering + // the merged list would keep only the project copy of a slug that exists in both files, and + // the rollback would then restore that copy while the global definition stayed overwritten. const savedCustomModes = configuration.customModes?.length - ? (await this.customModesManager.getCustomModes()).filter((mode) => - configuration.customModes!.some((candidate) => candidate.slug === mode.slug), + ? await this.customModesManager.getCustomModeDefinitions( + configuration.customModes.map((candidate) => candidate.slug), ) : [] diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 9cf7c02317..7a2df8d97a 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -6936,6 +6936,56 @@ describe("ClineProvider", () => { mockContext.globalState.update = originalUpdate await provider.dispose() }) + it("restores both definitions of a slug that exists as a project and a global mode", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const projectCopy = { + slug: "shared-slug", + name: "Project copy", + roleDefinition: "r", + groups: ["read" as const], + source: "project" as const, + } + const globalCopy = { + slug: "shared-slug", + name: "Global copy", + roleDefinition: "r", + groups: ["read" as const], + source: "global" as const, + } + const manager = provider["customModesManager"] + Object.assign(manager, { + // The merged read hides the global copy behind the project one; the rollback snapshot must + // not, or restoring it would write the project definition to the global file as well. + getCustomModes: vi.fn().mockResolvedValue([projectCopy]), + getCustomModeDefinitions: vi.fn().mockResolvedValue([projectCopy, globalCopy]), + updateCustomMode: vi.fn().mockResolvedValue(undefined), + removeCustomModeEntry: vi.fn().mockResolvedValue(true), + }) + vi.spyOn(provider, "setValues").mockRejectedValue(new Error("configuration write failed")) + + await expect( + provider.createTask( + "text", + undefined, + undefined, + {}, + { customModes: [{ ...globalCopy, name: "Batch copy" }] }, + ), + ).rejects.toThrow("configuration write failed") + + // Both copies are restored, each to its own file, so the batch's version cannot outlive the + // rollback in the global file while the project file gets the pre-batch definition back. + expect(manager.updateCustomMode).toHaveBeenCalledWith("shared-slug", projectCopy) + expect(manager.updateCustomMode).toHaveBeenCalledWith("shared-slug", globalCopy) + + await provider.dispose() + }) it("should remove a custom mode the batch registered when the batch fails", async () => { const provider = new ClineProvider( mockContext, @@ -6952,6 +7002,8 @@ describe("ClineProvider", () => { const manager = provider["customModesManager"] Object.assign(manager, { getCustomModes: vi.fn().mockResolvedValue([savedMode]), + // The rollback snapshot reads both source files, so the double answers that reader too. + getCustomModeDefinitions: vi.fn().mockResolvedValue([savedMode]), updateCustomMode: vi.fn().mockResolvedValue(undefined), removeCustomModeEntry: vi.fn().mockResolvedValue(true), }) @@ -6986,6 +7038,8 @@ describe("ClineProvider", () => { const manager = provider["customModesManager"] Object.assign(manager, { getCustomModes: vi.fn().mockResolvedValue([savedMode]), + // The rollback snapshot reads both source files, so the double answers that reader too. + getCustomModeDefinitions: vi.fn().mockResolvedValue([savedMode]), updateCustomMode: vi.fn((slug: string, mode: typeof savedMode) => { calls.push([slug, mode.name]) // The batch's second registration is the one that throws.