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/20] 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/20] 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/20] 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/20] 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 1357d5b602c4afa5e9b3f6e47a17ccbc493edb9c Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 29 Oct 2026 21:11:46 +0800 Subject: [PATCH 05/20] chore: strip BOM and fix lint imports --- .husky/_/post-checkout | 3 +++ .husky/_/post-commit | 3 +++ .husky/_/post-merge | 3 +++ .husky/_/pre-push | 3 +++ src/core/webview/ClineProvider.ts | 2 +- src/core/webview/__tests__/ClineProvider.spec.ts | 3 +-- 6 files changed, 14 insertions(+), 3 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 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 9d812cd8e9..7db54cf4ac 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 4bb3e49635..0e033397da 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" @@ -14,7 +14,6 @@ import { type ClineMessage, type ExtensionMessage, type ExtensionState, - type RooCodeSettings, type WebviewMessage, ORGANIZATION_ALLOW_ALL, DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, From c59f82f117b3281894ebec711c2db398872b20bd Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 29 Oct 2026 21:12:24 +0800 Subject: [PATCH 06/20] chore: untrack generated husky hooks again --- .husky/_/post-checkout | 3 --- .husky/_/post-commit | 3 --- .husky/_/post-merge | 3 --- .husky/_/pre-push | 3 --- 4 files changed, 12 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 "$@" From afa9ab308633cd8d008bf91f594bbe2b8b283852 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 29 Oct 2026 21:56:49 +0800 Subject: [PATCH 07/20] 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 --- .husky/_/post-checkout | 3 +++ .husky/_/post-commit | 3 +++ .husky/_/post-merge | 3 +++ .husky/_/pre-push | 3 +++ packages/types/src/vscode-extension-host.ts | 6 +++-- src/core/config/ContextProxy.ts | 14 +++++++++-- .../__tests__/ProviderSettingsManager.spec.ts | 11 +++++++++ src/core/webview/ClineProvider.ts | 24 ++++++++++++++++--- 8 files changed, 60 insertions(+), 7 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 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/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/config/__tests__/ProviderSettingsManager.spec.ts b/src/core/config/__tests__/ProviderSettingsManager.spec.ts index 3b93b7cd22..c17c241fa3 100644 --- a/src/core/config/__tests__/ProviderSettingsManager.spec.ts +++ b/src/core/config/__tests__/ProviderSettingsManager.spec.ts @@ -983,6 +983,17 @@ describe("ProviderSettingsManager", () => { ) }) + it('keeps a storage failure whose message mentions "not found" out of the typed not-found signal', async () => { + mockSecrets.get.mockRejectedValue(new Error("vault: secret not found")) + + // A message-based classifier would treat this as a missing profile. The typed + // signal must stay reserved for a missing config entry. + await expect(providerSettingsManager.deleteConfig("default")).rejects.not.toBeInstanceOf( + ProviderSettingsNotFoundError, + ) + await expect(providerSettingsManager.deleteConfig("default")).rejects.toThrow("Failed to delete config") + }) + it("should throw error when trying to delete last remaining config", async () => { mockSecrets.get.mockResolvedValue( JSON.stringify({ diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 7db54cf4ac..5b4a9aed35 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) From 86c0288fbaad33d6ba35ebb3c39a821262f9e9a7 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 30 Oct 2026 12:03:22 +0800 Subject: [PATCH 08/20] 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 576f45203f9806b13c7d83787724d32e26e33b94 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 30 Oct 2026 12:21:43 +0800 Subject: [PATCH 09/20] chore: tighten the new 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 5b4a9aed35..169daed1e0 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 }) } @@ -3950,11 +3918,7 @@ export class ClineProvider await this.contextProxy.setValues(values) } - /** - * 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 { @@ -3962,10 +3926,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 { @@ -3984,9 +3945,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 b4ca1bc9a3ea7b52476584b96f8c87f431190403 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 10/20] 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 ebe5a6af21c138d8f42dfcc163edeb9d7cee1c83 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 30 Oct 2026 12:31:28 +0800 Subject: [PATCH 11/20] 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 169daed1e0..fdd174e4e4 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 f79870c387ad96039f5067ebb6c0ac06b51a5196 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 30 Oct 2026 12:32:14 +0800 Subject: [PATCH 12/20] chore: tighten the reset-generation comments --- src/core/config/ContextProxy.ts | 6 ++---- src/core/config/__tests__/ContextProxy.spec.ts | 6 ++---- src/core/config/importExport.ts | 4 +--- 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index e6aea60685..c93b42628f 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -377,8 +377,7 @@ export class ContextProxy { try { return await this.originalContext.globalState.update(key, value) } catch (error) { - // A newer write for the same key may already own the cache: only undo this write - // while it is still the newest one. + // Only undo this write while it is still the newest for the key. if (this.stateWriteGenerations.get(key) === generation) { this.stateCache[key] = previousCachedValue } @@ -607,8 +606,7 @@ 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. + // Advance every tracked generation so a pending write cannot restore pre-reset state. for (const key of this.stateWriteGenerations.keys()) { this.stateWriteGenerations.set(key, (this.stateWriteGenerations.get(key) ?? 0) + 1) } diff --git a/src/core/config/__tests__/ContextProxy.spec.ts b/src/core/config/__tests__/ContextProxy.spec.ts index a28ca249ac..5231e730d9 100644 --- a/src/core/config/__tests__/ContextProxy.spec.ts +++ b/src/core/config/__tests__/ContextProxy.spec.ts @@ -133,8 +133,7 @@ describe("ContextProxy", () => { 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. + // The older write rejects after a newer value reached the cache. rejectFirst(new Error("disk full")) await expect(firstWrite).rejects.toThrow("disk full") @@ -155,8 +154,7 @@ describe("ContextProxy", () => { 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. + // The reset cleared the cache: the old value must not come back. expect(proxy.getGlobalState("apiProvider")).toBeUndefined() }) diff --git a/src/core/config/importExport.ts b/src/core/config/importExport.ts index 29c9497552..530e29cbb9 100644 --- a/src/core/config/importExport.ts +++ b/src/core/config/importExport.ts @@ -99,9 +99,7 @@ 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. + // Per-view selections are machine-local: they must not import across machines. if (key === "viewStates") { continue } From d52cd47a2cf03670aa58881d9f1ce35fd9c3ce9b Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 30 Oct 2026 12:32:44 +0800 Subject: [PATCH 13/20] chore: tighten the N2 test comments --- src/core/webview/__tests__/ClineProvider.spec.ts | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 0e033397da..a7adf0e440 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -428,9 +428,7 @@ type StalledProfile = { 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. + // Only the first lookup is resolvable; fail loudly on a second. if (getProfileSpy.mock.calls.length > 1) { throw new Error( "stallProviderSettingsProfile: getProfile called more than once; only one lookup is resolvable", @@ -440,9 +438,7 @@ function stallProviderSettingsProfile(provider: ClineProvider) { }) // @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. + // Wrapper keeps the reassigned resolver visible to the caller. return { getProfile: getProfileSpy, resolveProfile: (value: StalledProfile) => resolveProfile(value) } } @@ -4316,10 +4312,7 @@ describe("ClineProvider", () => { }) 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". + // No per-view entry: getState() falls back to the shared defaults. expect(state.mode).toBe("code") expect(state.currentApiConfigName).toBe("default") From f652d7e1323d38ab6728d5c75594b33c52851958 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 30 Oct 2026 12:55:49 +0800 Subject: [PATCH 14/20] chore: drop the now-unused buffer-clear helper --- src/core/webview/ClineProvider.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 516c050d81..940a846c43 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -3917,9 +3917,6 @@ export class ClineProvider } /** Clear the buffer so getState() falls back to the shared values. */ - private _clearViewLocalState(): void { - this.viewLocalState = {} - } async resetState() { const answer = await vscode.window.showInformationMessage( t("common:confirmation.reset_state"), From 7a48eed62d85279d07c8476d0570cb4eed7c95ec Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 30 Oct 2026 12:56:56 +0800 Subject: [PATCH 15/20] 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 940a846c43..559568579e 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 /** @@ -600,10 +597,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. @@ -689,10 +683,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 @@ -784,10 +775,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 00c0f8d86a1f387bbf05b4f6d6abaf7397b1298c Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 30 Oct 2026 12:57:38 +0800 Subject: [PATCH 16/20] chore: tighten the N2 comments --- src/core/config/__tests__/ContextProxy.spec.ts | 2 -- src/core/config/__tests__/importExport.spec.ts | 3 +-- src/core/webview/ClineProvider.ts | 3 --- src/core/webview/__tests__/ClineProvider.spec.ts | 6 +----- 4 files changed, 2 insertions(+), 12 deletions(-) diff --git a/src/core/config/__tests__/ContextProxy.spec.ts b/src/core/config/__tests__/ContextProxy.spec.ts index 5231e730d9..12f8c67174 100644 --- a/src/core/config/__tests__/ContextProxy.spec.ts +++ b/src/core/config/__tests__/ContextProxy.spec.ts @@ -133,7 +133,6 @@ describe("ContextProxy", () => { const firstWrite = proxy.updateGlobalState("apiProvider", "anthropic") await proxy.updateGlobalState("apiProvider", "openai") - // The older write rejects after a newer value reached the cache. rejectFirst(new Error("disk full")) await expect(firstWrite).rejects.toThrow("disk full") @@ -154,7 +153,6 @@ describe("ContextProxy", () => { rejectPending(new Error("disk full")) await expect(pending).rejects.toThrow("disk full") - // The reset cleared the cache: the old value must not come back. expect(proxy.getGlobalState("apiProvider")).toBeUndefined() }) diff --git a/src/core/config/__tests__/importExport.spec.ts b/src/core/config/__tests__/importExport.spec.ts index b8dff9c0ed..238362d5c2 100644 --- a/src/core/config/__tests__/importExport.spec.ts +++ b/src/core/config/__tests__/importExport.spec.ts @@ -370,8 +370,7 @@ describe("importExport", () => { }) 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. + // Importing must not apply another machine's view pins. expect(mockContextProxy.setValues).toHaveBeenCalledWith({ mode: "code" }) expect(result).not.toHaveProperty("globalSettings.viewStates") }) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 559568579e..d571390b26 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -452,7 +452,6 @@ 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 @@ -2565,8 +2564,6 @@ export class ClineProvider if (signal.aborted) return id - // 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`). this.updateTaskApiHandlerIfNeeded(providerSettings, { forceRebuild: true }) diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index a7adf0e440..2e1d2a0b8e 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -428,7 +428,6 @@ type StalledProfile = { function stallProviderSettingsProfile(provider: ClineProvider) { let resolveProfile: (value: StalledProfile) => void = () => {} const getProfileSpy = vi.fn(() => { - // Only the first lookup is resolvable; fail loudly on a second. if (getProfileSpy.mock.calls.length > 1) { throw new Error( "stallProviderSettingsProfile: getProfile called more than once; only one lookup is resolvable", @@ -438,7 +437,6 @@ function stallProviderSettingsProfile(provider: ClineProvider) { }) // @ts-ignore - Reassign the readonly providerSettingsManager for the test; the double only backs getProfile. provider.providerSettingsManager = { getProfile: getProfileSpy } - // Wrapper keeps the reassigned resolver visible to the caller. return { getProfile: getProfileSpy, resolveProfile: (value: StalledProfile) => resolveProfile(value) } } @@ -3891,7 +3889,6 @@ describe("ClineProvider", () => { 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() @@ -3951,8 +3948,7 @@ describe("ClineProvider", () => { 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. + // Seed through the write path so this instance authors 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({ From 5a6693ba5873bff67863fad1c931e2c60cc05273 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 30 Oct 2026 12:57:57 +0800 Subject: [PATCH 17/20] chore: tighten the N2 comments further --- src/core/config/__tests__/ContextProxy.spec.ts | 3 --- src/core/webview/ClineProvider.ts | 1 - 2 files changed, 4 deletions(-) diff --git a/src/core/config/__tests__/ContextProxy.spec.ts b/src/core/config/__tests__/ContextProxy.spec.ts index 12f8c67174..975081e92f 100644 --- a/src/core/config/__tests__/ContextProxy.spec.ts +++ b/src/core/config/__tests__/ContextProxy.spec.ts @@ -132,7 +132,6 @@ describe("ContextProxy", () => { const firstWrite = proxy.updateGlobalState("apiProvider", "anthropic") await proxy.updateGlobalState("apiProvider", "openai") - rejectFirst(new Error("disk full")) await expect(firstWrite).rejects.toThrow("disk full") @@ -147,11 +146,9 @@ describe("ContextProxy", () => { rejectPending = reject }), ) - const pending = proxy.updateGlobalState("apiProvider", "anthropic") await proxy.resetAllState() rejectPending(new Error("disk full")) - await expect(pending).rejects.toThrow("disk full") expect(proxy.getGlobalState("apiProvider")).toBeUndefined() }) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index d571390b26..562b93c8ec 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -895,7 +895,6 @@ export class ClineProvider /** Load non-secret persisted selections; in-flight buffer edits are reapplied on top. */ private async loadViewState(): Promise { - // A newer id registered while this load is in flight must win. const loadedForViewId = this.viewStateId // Before registration the key is the session-local name, which can be a stale entry From 3f5765148300b22d16cc3c50f3ab870833a6f5d2 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 14:27:04 +0800 Subject: [PATCH 18/20] fix(config): end write ownership on reset, and carry the profile name with its settings Three measured review findings on this head, landed together because they answer one save/reset path. ContextProxy.resetAllState cleared the caches and bumped a generation counter that nothing ever read: stateWriteGenerations had five references in the repository (declaration at src/core/config/ContextProxy.ts:79, increment at 420-421, reset bump at 729-730), no comparer anywhere, and ClineProvider.ts never consulted a generation. The rejection handlers gate on write ownership (stateWriteOwner / secretWriteOwner), and reset left that ownership in place, so a write still in flight read the Memento back into the cache the reset had just cleared. resetAllState now clears both owner maps; the field, its two write sites and its reset bump are deleted. ContextProxy.ts 763 -> 761 lines. The activation path wrote a profile's settings into this view's buffer while the buffered profile name stayed behind: ClineProvider.setValue is a ContextProxy pass-through, and pinnedProfileName reads viewLocalState.currentApiConfigName, so the next non-activating save of that profile compared a name nothing had updated and skipped the refresh. The buffer write now carries currentApiConfigName and apiConfiguration in one mutation. refreshSiblingsAfterProfileSave is ported unchanged from the n4 head (877ea0a00) and called from both save branches. This head had no sibling refresh at all before - zero references to refreshSiblings and the three getAllInstances uses were elsewhere - so this is a capability arriving, not a behaviour changing. ClineProvider.ts 6039 -> 6078 lines. Tests. ContextProxy.spec.ts: the case "should not restore a pre-reset value when a pending write rejects after reset" now re-arms the mocked Memento so the read-back answers with the pre-reset value; it fails against the old production (expected 'anthropic' to be undefined) and passes now. Main's thirteen read-back / rejection / reset cases in that file were re-run and are green individually, not by inference (file 66/66). ClineProvider.spec.ts: new case "keeps the buffered profile name in step with the profile it activates", plus the two sibling-overlay cases ported from the n4 head, one per save branch; on this head the sibling is pinned through saveViewState because setValue does not touch the buffer. Controls (production snapshotted and restored by content hash after each - ClineProvider.ts 8f05e399aa, ContextProxy.ts f5281bde9e): removing the non-activating branch sibling call fails only the non-activating case (1 failed | 368 skipped); removing the activating-branch call fails only the activated-profile case; reducing the buffer mutation to { apiConfiguration } fails only the name case (expected { ...2 } to match object { ...2 }); leaving write ownership in place at reset fails the reset case (expected 'anthropic' to be undefined) while main's "should reinitialize caches after reset" still passes. Gates: prettier --check clean; tsc --noEmit -p . clean; eslint --max-warnings=0 clean on all four files; vitest ContextProxy.spec.ts 66 tests, core/webview + core/config 44 files / 1267 tests green. Scope 335 -> 354 extension-package executable lines (types unchanged at 8), inside the 500 advisory. --- src/core/config/ContextProxy.ts | 12 +- .../config/__tests__/ContextProxy.spec.ts | 4 + src/core/webview/ClineProvider.ts | 45 +++++++- .../webview/__tests__/ClineProvider.spec.ts | 103 ++++++++++++++++++ 4 files changed, 154 insertions(+), 10 deletions(-) diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index cd2fc87230..f5281bde9e 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -76,7 +76,6 @@ export class ContextProxy { private readonly originalContext: vscode.ExtensionContext private stateCache: GlobalState - private stateWriteGenerations = new Map() private secretCache: SecretState private _isInitialized = false @@ -417,8 +416,6 @@ export class ContextProxy { // writes that are in flight. An update that throws at once is turned into a rejection, so it is handled the same. const owner = {} this.stateWriteOwner.set(key, owner) - const generation = (this.stateWriteGenerations.get(key) ?? 0) + 1 - this.stateWriteGenerations.set(key, generation) this.stateCache[key] = value const release = () => { if (this.stateWriteOwner.get(key) === owner) this.stateWriteOwner.delete(key) @@ -725,10 +722,11 @@ export class ContextProxy { // Clear in-memory caches this.stateCache = {} this.secretCache = {} - // Advance every tracked generation so a pending write cannot restore pre-reset state. - for (const key of this.stateWriteGenerations.keys()) { - this.stateWriteGenerations.set(key, (this.stateWriteGenerations.get(key) ?? 0) + 1) - } + // A reset also ends write ownership: a write that is still in flight reads the Memento back + // into the cache it just lost, so clearing the caches is not enough to keep pre-reset state + // out. Ownership is what the rejection handlers gate on. + this.stateWriteOwner.clear() + this.secretWriteOwner.clear() 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 240f0fe5e7..016f75118b 100644 --- a/src/core/config/__tests__/ContextProxy.spec.ts +++ b/src/core/config/__tests__/ContextProxy.spec.ts @@ -149,6 +149,10 @@ describe("ContextProxy", () => { ) const pending = proxy.updateGlobalState("apiProvider", "anthropic") await proxy.resetAllState() + // The Memento answers the read-back with the pre-reset value: only ending write ownership + // during the reset keeps the cache cleared. With the mocked store this is the difference + // between the guarantee holding and the rejected write repopulating the cache. + mockGlobalState.get.mockImplementation((key: string) => (key === "apiProvider" ? "anthropic" : undefined)) rejectPending(new Error("disk full")) await expect(pending).rejects.toThrow("disk full") expect(proxy.getGlobalState("apiProvider")).toBeUndefined() diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index b3748229eb..8f05e399aa 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -2823,12 +2823,18 @@ 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 buffer tracks the activated profile. + // Shared write: adds/updates the profile list entry and the current-profile name. this.setValue("currentApiConfigName", name), this.providerSettingsManager.setModeConfig(mode, id), this.contextProxy.setProviderSettings(providerSettings), - // setProviderSettings bypasses the buffer path: refresh this view's overlay. - this._saveViewLocalStateFromMutation({ apiConfiguration: providerSettings }), + // setProviderSettings bypasses the buffer path; the mutation below refreshes this view. + // One mutation carries both the name and the settings: pinnedProfileName reads the buffer, + // so a buffer that keeps the previous name while holding the new settings pins a profile + // this view no longer runs under. + this._saveViewLocalStateFromMutation({ + currentApiConfigName: name, + apiConfiguration: providerSettings, + }), ]) const failedActivation = activationResults.find( (result): result is PromiseRejectedResult => result.status === "rejected", @@ -2838,6 +2844,8 @@ export class ClineProvider if (signal.aborted) return id + await this.refreshSiblingsAfterProfileSave(name, providerSettings) + // Change the provider for the current task. // TODO: We should rename `buildApiHandler` for clarity (e.g. `getProviderClient`). this.updateTaskApiHandlerIfNeeded(providerSettings, { forceRebuild: true }) @@ -2856,6 +2864,10 @@ export class ClineProvider this.updateTaskApiHandlerIfNeeded(providerSettings, { forceRebuild: true }) } } + // A non-activating save changes the profile's settings too: refresh every sibling + // that buffered a copy (pinned siblings included), or their getState() keeps the pre-save + // values. This head had no sibling refresh at all before this change. + await this.refreshSiblingsAfterProfileSave(name, providerSettings) } if (signal.aborted) return id @@ -2863,6 +2875,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 diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index c9312af8dd..4a23c2cddd 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -6263,6 +6263,109 @@ describe("ClineProvider", () => { }) await provider.dispose() }) + // The activation path writes the profile settings straight into this view's buffer, so the + // buffered profile name has to move with them: pinnedProfileName reads the buffer, and a later + // non-activating save of the same profile is gated on that name. + it("keeps the buffered profile name in step with the profile it activates", async () => { + const saver = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + await saver["saveViewState"]("currentApiConfigName", "profile-a") + expect(saver["pinnedProfileName"]).toBe("profile-a") + // @ts-ignore - The double only backs the profile-store calls of the save path. + saver.providerSettingsManager = { + saveConfig: vi.fn().mockResolvedValue("profile-b-id"), + listConfig: vi.fn().mockResolvedValue([{ name: "profile-b", id: "profile-b-id" }]), + setModeConfig: vi.fn().mockResolvedValue(undefined), + } + const providerSettings = { apiProvider: providerIdentifiers.anthropic, apiKey: "activated-key" } + + await saver.upsertProviderProfile("profile-b", providerSettings, true) + + // One mutation: the name and the settings land in the buffer together, or the view pins a + // profile whose settings it is no longer holding. + expect(saver["viewLocalState"]).toMatchObject({ + currentApiConfigName: "profile-b", + apiConfiguration: providerSettings, + }) + expect(saver["pinnedProfileName"]).toBe("profile-b") + await saver.dispose() + }) + + 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, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const pinned = new ClineProvider( + mockContext, + mockOutputChannel, + "editor", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + // saveViewState is the buffer write on this head: setValue is a ContextProxy pass-through, + // so going through setValue here would leave pinnedProfileName undefined. + await pinned["saveViewState"]("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, 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. + expect(pinned["viewLocalState"].apiConfiguration).toEqual(providerSettings) + await saver.dispose() + 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(), + ) + // saveViewState is the buffer write on this head: setValue is a ContextProxy pass-through, + // so going through setValue here would leave pinnedProfileName undefined. + await pinned["saveViewState"]("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() + }) }) describe("saveViewState", () => { From f3ca33bf23f993d87ca9db9a57cb9bbc7b17673d Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 16:22:11 +0800 Subject: [PATCH 19/20] fix(config): settle in-flight secret writes before a reset ends their claims A reset deleted every secret key before a storeSecret that was still in flight had committed, so the write landed afterwards and a pre-reset secret came back from storage even though the cache was empty. resetAllState now waits for the pending settles first, then ends write ownership, then deletes, so the cleared copy is the last write to land. The claim is cleared after the settle on purpose: a write that settles while it still owns its key adopts into the cache, which the very next lines clear, and its storage write is then deleted too. A secret write issued after a reset that waited for a pending write is adopted normally, because it is a post-reset intent that owns its key. Measured before the change: at the instant resetAllState returned, storage still held the pre-reset secret while getSecret() read undefined and the delete pass had already run 37 times - awaiting the settlement after the deletes settles correctly and changes nothing. Both added cases were red against the previous head. Control: replacing the await with 'void inFlight' re-reddens the regression case; the production file was restored by content hash. Addresses row 6z9EGf on #2227. Co-Authored-By: Claude --- src/core/config/ContextProxy.ts | 26 +++++++ .../config/__tests__/ContextProxy.spec.ts | 69 +++++++++++++++++++ 2 files changed, 95 insertions(+) diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index f5281bde9e..9d1ae3d3c0 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -81,6 +81,7 @@ export class ContextProxy { constructor(context: vscode.ExtensionContext) { this.originalContext = context + this.stateCache = {} this.secretCache = {} this._isInitialized = false @@ -400,6 +401,12 @@ export class ContextProxy { */ private readonly stateWriteOwner = new Map() private readonly secretWriteOwner = new Map() + /** + * The secret writes that are still in flight, each paired with the claim it took, + * so a reset can wait for them to settle before it deletes the keys. Without the + * pairing a reset cannot tell a settled-but-orphaned write from a live one. + */ + private readonly pendingSecretWrites = new Map }>>() /** How many writes of each secret were started. Unlike the owners, it is not cleared when a write is over. */ private readonly secretWriteCount = new Map() @@ -457,7 +464,19 @@ export class ContextProxy { // cache (it stays empty until the next write, `refreshSecrets` or a restart) rather than being served. // The entry of the key is removed when the write and its read-back are over (if it is still the latest). A store // that throws at once is turned into a rejection, so it is handled the same. + const pending = this.pendingSecretWrites.get(key) ?? new Set<{ settle: Promise }>() + let settleWrite: () => void = () => {} + const entry: { settle: Promise } = { + settle: new Promise((resolve) => { + settleWrite = resolve + }), + } + pending.add(entry) + this.pendingSecretWrites.set(key, pending) const release = () => { + pending.delete(entry) + if (pending.size === 0) this.pendingSecretWrites.delete(key) + settleWrite() if (this.secretWriteOwner.get(key) === owner) this.secretWriteOwner.delete(key) } const stored = (async () => @@ -719,6 +738,13 @@ export class ContextProxy { * @returns A promise that resolves when all reset operations are complete */ public async resetAllState() { + // A secret write that is in flight at this moment has already committed nothing, but it will + // commit as soon as it settles, and the deletes below cannot un-commit a write that lands + // after them. Wait for the pending writes first, then end the claims, then delete, so the + // cleared copy is the last one to land in storage and the cache stays empty afterwards. + const inFlight = [...this.pendingSecretWrites.values()].flatMap((writes) => [...writes].map((w) => w.settle)) + await Promise.allSettled(inFlight) + // Clear in-memory caches this.stateCache = {} this.secretCache = {} diff --git a/src/core/config/__tests__/ContextProxy.spec.ts b/src/core/config/__tests__/ContextProxy.spec.ts index 016f75118b..c31062c0b5 100644 --- a/src/core/config/__tests__/ContextProxy.spec.ts +++ b/src/core/config/__tests__/ContextProxy.spec.ts @@ -157,6 +157,75 @@ describe("ContextProxy", () => { await expect(pending).rejects.toThrow("disk full") expect(proxy.getGlobalState("apiProvider")).toBeUndefined() }) + it("does not restore a pre-reset secret when a pending storeSecret settles after the reset", async () => { + const durable = new Map() + let settle: () => void = () => {} + mockSecrets.get.mockImplementation(async (key: string) => durable.get(key)) + mockSecrets.delete.mockImplementation(async (key: string) => { + durable.delete(key) + }) + mockSecrets.store.mockImplementationOnce( + (key: string, value: string) => + new Promise((resolve) => { + settle = () => { + durable.set(key, value) + resolve(undefined) + } + }), + ) + + const pending = proxy.storeSecret("apiKey", "pre-reset-secret") + setTimeout(settle, 0) + await proxy.resetAllState() + await pending + expect.soft([ + await mockSecrets.get("apiKey"), + proxy.getSecret("apiKey"), + mockSecrets.delete.mock.calls.length, + ]) + + // The pending store is held open on a double that only puts the value into storage + // when it settles, so the ordering is observable: the reset must wait for the write to + // settle and then delete the keys, so neither the cache nor the durable copy keeps the + // value that was in flight when the reset started. + expect(await mockSecrets.get("apiKey")).toBeUndefined() + expect(proxy.getSecret("apiKey")).toBeUndefined() + }) + + it("adopts a secret write issued after a reset that waited for a pending write", async () => { + const durable = new Map() + let settle: () => void = () => {} + mockSecrets.get.mockImplementation(async (key: string) => durable.get(key)) + mockSecrets.delete.mockImplementation(async (key: string) => { + durable.delete(key) + }) + mockSecrets.store.mockImplementationOnce( + (key: string, value: string) => + new Promise((resolve) => { + settle = () => { + durable.set(key, value) + resolve(undefined) + } + }), + ) + + const pending = proxy.storeSecret("apiKey", "pre-reset-secret") + const reset = proxy.resetAllState() + setTimeout(settle, 0) + await pending + await reset + + // The reset released only its own claims, so the machinery stays usable afterwards: + // a write issued once the reset has settled is adopted into the cache and reaches storage. + // Back the post-reset write with the durable double as well, so the assertion below reads + // storage rather than only the cache. + mockSecrets.store.mockImplementationOnce(async (key: string, value: string) => { + durable.set(key, value) + }) + await proxy.storeSecret("apiKey", "newer-secret") + expect(proxy.getSecret("apiKey")).toBe("newer-secret") + expect(await mockSecrets.get("apiKey")).toBe("newer-secret") + }) it("should bypass cache for pass-through state keys", async () => { // Setup mock return value From 515cc020d2a77edfd05e744f4bd6a1d1f43a8d77 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 17:44:50 +0800 Subject: [PATCH 20/20] fix(config): refuse secret writes that begin inside a reset instead of letting them survive it resetAllState waits for the secret writes that are in flight when it starts, clears both caches, drops both write claims and then deletes the keys. A storeSecret that begins after that in-flight snapshot is invisible to all of it: it puts the value into the secret cache on entry, the reset clears that cache underneath it, and the write's durable commit lands after the delete pass. The success path never touches the cache again, so the value stays out of the cache while it stays in durable storage, and the next initialize - what a restarted provider does - reads it back out and surfaces to the user a secret they had just reset away. Secret writes are therefore refused while a reset is in progress, with a SettingsWriteError naming the key, and the flag is cleared only after the reset has re-initialized so the whole window is covered. Refusing was chosen over serializing the reset: a serialized reset lets the write apply and then erases it, which discards silently a save the user just asked for, whereas a refusal leaves durable storage empty, which is what a reset promises, and hands the caller an error on a path that already reports which keys it could not store. Serialization was available and was rejected for that reason rather than omitted. Case: erases a secret whose write begins after the reset has snapshotted the pending writes. It parks the reset inside its own delete pass, starts the write in that window and reads three things: the durable double behind the secrets, this instance's cache, and the cache after a fresh initialize - the third is what makes it a security finding rather than a cache artefact. Case alone against f3ca33bf2: Test Files 1 failed | 9 passed (10), Tests 1 failed | 315 passed (316), with 'expected late-secret to be undefined' on the durable read. With the gate: 10 test files and 316 tests, 0 failed. Control, the gate condition disabled: 1 failed | 315 passed again, so the case measures the gate. Review row: database id 4478349061 (Major, Security & Privacy). --- src/core/config/ContextProxy.ts | 12 +++++ .../config/__tests__/ContextProxy.spec.ts | 50 +++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index 9d1ae3d3c0..e2a637fcd1 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -407,6 +407,9 @@ export class ContextProxy { * pairing a reset cannot tell a settled-but-orphaned write from a live one. */ private readonly pendingSecretWrites = new Map }>>() + // A reset is in progress: the caches are meaningless mid-reset, and a secret write begun + // inside it would commit after the delete pass and survive the erase. + private resetting = false /** How many writes of each secret were started. Unlike the owners, it is not cleared when a write is over. */ private readonly secretWriteCount = new Map() @@ -453,6 +456,13 @@ export class ContextProxy { } storeSecret(key: SecretStateKey, value?: string) { + if (this.resetting) { + // 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. + 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) @@ -738,6 +748,7 @@ export class ContextProxy { * @returns A promise that resolves when all reset operations are complete */ public async resetAllState() { + this.resetting = true // A secret write that is in flight at this moment has already committed nothing, but it will // commit as soon as it settles, and the deletes below cannot un-commit a write that lands // after them. Wait for the pending writes first, then end the claims, then delete, so the @@ -761,6 +772,7 @@ export class ContextProxy { ]) await this.initialize() + 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 c31062c0b5..d2c0bf6e52 100644 --- a/src/core/config/__tests__/ContextProxy.spec.ts +++ b/src/core/config/__tests__/ContextProxy.spec.ts @@ -227,6 +227,56 @@ describe("ContextProxy", () => { expect(await mockSecrets.get("apiKey")).toBe("newer-secret") }) + it("erases a secret whose write begins after the reset has snapshotted the pending writes", async () => { + const durable = new Map() + let deleteCalls = 0 + let settleFirstDelete: () => void = () => {} + let markDeletePassStarted: () => void = () => {} + const deletePassStarted = new Promise((resolve) => { + markDeletePassStarted = resolve + }) + mockSecrets.get.mockImplementation(async (key: string) => durable.get(key)) + mockSecrets.store.mockImplementation(async (key: string, value: string) => { + durable.set(key, value) + }) + // Park the reset inside its own delete pass so a write can begin in the window the in-flight + // snapshot cannot cover. Only the first delete is held open; the rest settle normally, so + // releasing the park lets the reset finish. + mockSecrets.delete.mockImplementation((key: string) => { + deleteCalls++ + durable.delete(key) + if (deleteCalls === 1) { + return new Promise((resolve) => { + markDeletePassStarted() + settleFirstDelete = () => resolve(undefined) + }) + } + return Promise.resolve(undefined) + }) + + const reset = proxy.resetAllState() + await deletePassStarted + + // The reset has taken its in-flight snapshot, cleared both caches and dropped both write + // claims, and is standing inside the delete pass. This write is invisible to all of it. + const late = proxy.storeSecret("apiKey", "late-secret") + settleFirstDelete() + await reset + await late.catch(() => undefined) + + // All three reads matter. Durable storage is what a reset is supposed to empty, the cache is + // what this provider instance shows the user, and re-initializing is what a restarted + // provider does - which is how a secret that survived the reset reaches the user after the + // fact, so the middle read alone would understate the finding. + expect(await mockSecrets.get("apiKey")).toBeUndefined() + expect(proxy.getSecret("apiKey")).toBeUndefined() + await proxy.initialize() + expect(proxy.getSecret("apiKey")).toBeUndefined() + // The caller is told: a write the reset cannot honour is refused rather than dropped, so + // the save surfaces as a failure instead of vanishing. + await expect(late).rejects.toThrow() + }) + it("should bypass cache for pass-through state keys", async () => { // Setup mock return value mockGlobalState.get.mockReturnValue("pass-through-value")