From 8449d8b6df5ac6a24a6701eff6def0846a273a8d Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 08:02:13 +0800 Subject: [PATCH 01/27] feat(webview): per-view view-state identity and host-minted claim token (N1) Split unit N1 of #1546: viewId/viewStateId identity, host-minted claim binding, viewStateSchema + viewStateClaim message types. F0/F3 content routed out; load/persistence/buffer remain for N2/N3. (cherry picked from commit a3c499d676967eb06413643764a6ad12407cf38d) --- .../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/ProviderSettingsManager.ts | 20 +- .../__tests__/ProviderSettingsManager.spec.ts | 7 +- src/core/webview/ClineProvider.ts | 623 +++++++++++++++++- .../webview/__tests__/ClineProvider.spec.ts | 269 +++++++- 8 files changed, 978 insertions(+), 21 deletions(-) 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 40f83fbaa2..0a6f2195f8 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -112,6 +112,7 @@ export interface ExtensionMessage { | "rooHistoryImportProgress" | "themeFixtureProbeRequest" | "diagnosticsRequest" + | "viewStateClaim" text?: string /** For fileContent: { path, content, error? } */ fileContent?: { path: string; content: string | null; error?: string } @@ -170,6 +171,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 diagnostics?: WebviewDiagnosticsSnapshot promptText?: string @@ -708,6 +716,13 @@ export interface WebviewMessage { | "themeFixtureProbeResponse" | "diagnosticsResponse" 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/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__/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/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index dc4ed27621..cb8aa4cf13 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -74,6 +74,7 @@ import { isRetiredProvider, modelIdKeys, providerIdentifiers, + PROVIDER_SETTINGS_KEYS, } from "@roo-code/types" import { RateLimitClock, createRateLimitClock } from "../task/RateLimitClock" import { TaskRegistry } from "../task/TaskRegistry" @@ -132,7 +133,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" @@ -159,6 +160,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 @@ -247,6 +256,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 webviewDisposables: vscode.Disposable[] = [] private pendingThemeFixtureProbes = new Map< string, @@ -414,6 +427,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.94.0-models-oauth-chat" // v3.94.0 model support, MCP OAuth, and chat reliability @@ -429,13 +481,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.diagnosticsRecorder.record({ boundary: "provider", phase: "success", type: "created" }) this.mdmService = mdmService @@ -598,6 +654,344 @@ 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) + + // 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 + } + } + /** * Override EventEmitter's on method to match TaskProviderLike interface */ @@ -943,7 +1337,6 @@ export class ClineProvider ) } } - async dispose() { if (this._disposed) { return @@ -1451,7 +1844,6 @@ export class ClineProvider ) return run } - private async createTaskWithHistoryItemUnlocked( historyItem: HistoryItem & { rootTask?: Task; parentTask?: Task }, options?: { startTask?: boolean }, @@ -1697,7 +2089,6 @@ export class ClineProvider return task } - public async postMessageToWebview(message: ExtensionMessage) { const startedAt = Date.now() const taskId = this.getCurrentTask()?.taskId @@ -2047,7 +2438,6 @@ export class ClineProvider this.handleModeSwitchUnlocked(newMode, targetTask, signal), ) } - private async handleModeSwitchUnlocked( newMode: Mode, targetTask: Task | null | undefined, @@ -2321,7 +2711,6 @@ export class ClineProvider ) } } - private async persistProviderProfile( name: string, providerSettings: ProviderSettings, @@ -2382,7 +2771,6 @@ export class ClineProvider await this.postStateToWebview() return id } - async deleteProviderProfile(profileToDelete: ProviderSettingsEntry) { const globalSettings = this.contextProxy.getValues() let profileToActivate: string | undefined = globalSettings.currentApiConfigName @@ -2450,7 +2838,6 @@ export class ClineProvider this.activateProviderProfileUnlocked(args, options, signal), ) } - private async activateProviderProfileUnlocked( args: { name: string } | { id: string }, options?: { @@ -2582,9 +2969,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. @@ -3695,7 +4079,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) } @@ -3703,17 +4086,12 @@ 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 - async resetState() { const answer = await vscode.window.showInformationMessage( t("common:confirmation.reset_state"), @@ -5475,4 +5853,213 @@ export class ClineProvider return vscode.Uri.file(filePath).toString() } } + + /** + * 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}`) + } + + /** + * 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) + } + + /** + * 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, + } + } + } + + /** + * 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 = {} + // Load initial state from global state into viewLocalState buffer after dependencies used by getState are ready. + void this.loadViewState() + } + + /** + * 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)}`, + ) + } + } } diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 07bacb91f6..67e8e9f0e0 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" @@ -5610,6 +5610,273 @@ 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("webviewMessageHandler no-floating-promises coverage", () => { From 9bd8aca93d156a301d9021c828676c801a495eef Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 29 Oct 2026 21:11:42 +0800 Subject: [PATCH 02/27] chore: strip BOM and fix lint imports (cherry picked from commit c3595a1f0898988d1e38e920a21d97eaab70c9d7) --- src/core/webview/ClineProvider.ts | 2 +- src/core/webview/__tests__/ClineProvider.spec.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index cb8aa4cf13..8183b5869f 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -133,7 +133,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 67e8e9f0e0..27a61fb65a 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -1,4 +1,4 @@ -// pnpm --filter roo-cline test core/webview/__tests__/ClineProvider.spec.ts +// pnpm --filter roo-cline test core/webview/__tests__/ClineProvider.spec.ts import fs from "fs" import * as path from "path" From e1afe96bee39f459009e264ee8f1589b9da7355c Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 29 Oct 2026 21:56:46 +0800 Subject: [PATCH 03/27] 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 (cherry picked from commit 60c5400f203c761538bd327cd46b63d81b55cdd6) --- packages/types/src/vscode-extension-host.ts | 6 +++-- src/core/config/ContextProxy.ts | 3 +++ src/core/webview/ClineProvider.ts | 24 +++++++++++++++--- .../webview/__tests__/ClineProvider.spec.ts | 25 +++++++++++++++++++ 4 files changed, 53 insertions(+), 5 deletions(-) diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index 0a6f2195f8..2367e22af6 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -719,8 +719,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 1250a7baca..28301337e4 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, + // Host-minted claim tokens and per-view selections are not user-settings: exporting + // them would let a settings file be replayed as another view's identity. + viewStates: true, }) export class ContextProxy { diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 8183b5869f..3179f3dee5 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -168,6 +168,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 @@ -851,8 +858,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)) @@ -5859,9 +5877,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) diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 27a61fb65a..723cfef583 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -5877,6 +5877,31 @@ describe("ClineProvider", () => { } }) }) + + it("keeps a pre-launch selection when registration re-keys it onto an existing stable entry", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + + await provider["savePersistedViewState"]({ mode: "architect", updatedAt: 1 }) + const seeded = provider.contextProxy.getValue("viewStates") ?? {} + await provider.contextProxy.setValue("viewStates", { + ...seeded, + "stable-view": { claimToken: "host-token", updatedAt: 2 }, + }) + + await provider["rekeyPersistedViewStateEntry"]("stable-view") + + expect(mockContext.globalState.get("viewStates")).toMatchObject({ + "stable-view": { mode: "architect", claimToken: "host-token" }, + }) + + await provider.dispose() + }) }) describe("webviewMessageHandler no-floating-promises coverage", () => { From 198681699a0fa9f369f8d95ffabbf15233f1ca31 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 30 Oct 2026 12:23:27 +0800 Subject: [PATCH 04/27] chore: tighten the view-identity comments (cherry picked from commit 1b9e8f8bbff90eb26f77e5d6d774490bf281eebb) --- 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 | 63 ++++----------------- 4 files changed, 16 insertions(+), 65 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 2367e22af6..e6a7c02f81 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -719,10 +719,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 3179f3dee5..a1571f2fe9 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -441,12 +441,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 @@ -682,13 +678,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 @@ -776,11 +767,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() @@ -826,18 +814,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 @@ -885,14 +862,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, @@ -5979,12 +5950,7 @@ export class ClineProvider void this.loadViewState() } - /** - * 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. @@ -6038,13 +6004,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 } From 5cc1245f4fdfa3cc76e79eb994e523a909230ea8 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 30 Oct 2026 12:55:33 +0800 Subject: [PATCH 05/27] fix(webview): load persisted selections when a view id is registered (cherry picked from commit a81e24ef7ddad7c42cb88009ef570b20c1d98ad8) --- src/core/webview/ClineProvider.ts | 13 ++++------- .../webview/__tests__/ClineProvider.spec.ts | 23 +++++++++++++++++++ 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index a1571f2fe9..c044bb6759 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -971,6 +971,10 @@ export class ClineProvider if (adoptedClaim) { this.postViewStateClaimToWebview(this.viewStateId, adoptedClaim) } + + // Registration is only complete once the view's own persisted selections are in the + // buffer; nothing else loads them now that reset/import handling lives downstream. + await this.loadViewState() } 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 @@ -5941,15 +5945,6 @@ export class ClineProvider } } - /** - * Clear view-local state cache so that getState() falls back to ContextProxy defaults. - */ - private _clearViewLocalState(): void { - this.viewLocalState = {} - // Load initial state from global state into viewLocalState buffer after dependencies used by getState are ready. - void this.loadViewState() - } - /** 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 diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 723cfef583..345237566e 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -5633,6 +5633,29 @@ describe("ClineProvider", () => { await provider.dispose() }) + it("loads the persisted selections of the registered view", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + await provider.contextProxy.setValue("viewStates", { + "stable-view": { claimToken: "host-token", mode: "architect", currentApiConfigName: "cfg-a" }, + }) + const logSpy = vi.spyOn(provider, "log") + + await provider["setViewStateId"]("stable-view", "host-token") + + // Registration is what loads a view's own selections: the buffer is populated and + // the load is logged, so a reload restores the pinned mode instead of the shared one. + expect(provider["viewLocalState"].mode).toBe("architect") + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("Loaded state for viewId")) + + 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( From adf9d2f1f0e89e00f23a1ad3a08c7a18985a74d1 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 30 Oct 2026 12:56:52 +0800 Subject: [PATCH 06/27] chore: tighten the shared view-state docs (cherry picked from commit c9f462682caa2a6603eeb815d7c6d37dce856d3b) --- packages/types/src/global-settings.ts | 5 +--- packages/types/src/vscode-extension-host.ts | 5 +--- src/core/webview/ClineProvider.ts | 23 ++++--------------- .../webview/__tests__/ClineProvider.spec.ts | 6 +---- 4 files changed, 8 insertions(+), 31 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 e6a7c02f81..9e8e86d411 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -717,10 +717,7 @@ export interface WebviewMessage { | "diagnosticsResponse" 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 c044bb6759..38331f2cca 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -440,10 +440,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 /** @@ -677,10 +674,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. @@ -766,10 +760,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 @@ -861,10 +852,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. @@ -972,8 +960,7 @@ export class ClineProvider this.postViewStateClaimToWebview(this.viewStateId, adoptedClaim) } - // Registration is only complete once the view's own persisted selections are in the - // buffer; nothing else loads them now that reset/import handling lives downstream. + // Registration is what loads this view's own persisted selections. await this.loadViewState() } catch (error) { // A persistence failure must not leave the provider holding an id that was diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 345237566e..5033c06ac4 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -5156,8 +5156,6 @@ describe("ClineProvider", () => { } // Spy on log method to verify warning was logged - const logSpy = vi.spyOn(provider, "log") - // Create history item with non-existent mode const historyItem = { id: "test-id", @@ -5648,10 +5646,8 @@ describe("ClineProvider", () => { await provider["setViewStateId"]("stable-view", "host-token") - // Registration is what loads a view's own selections: the buffer is populated and - // the load is logged, so a reload restores the pinned mode instead of the shared one. + // Registration is what loads a view's own selections. expect(provider["viewLocalState"].mode).toBe("architect") - expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("Loaded state for viewId")) await provider.dispose() }) From 6f01d1897370c34feeeee87b5a7892fc2a960d46 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 30 Oct 2026 13:13:29 +0800 Subject: [PATCH 07/27] fix(webview): track buffer writes so a same-value save survives the load (cherry picked from commit c9b7fda3abf6d43d2af7368d7413eea89e373a86) --- src/core/webview/ClineProvider.ts | 18 +++++++++++++++--- .../webview/__tests__/ClineProvider.spec.ts | 2 ++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 38331f2cca..36dbd1efa1 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -457,6 +457,10 @@ export class ClineProvider */ private viewLocalState: Partial = {} + // Per-field write counters: a same-value save leaves the buffer value unchanged, so the + // load merge cannot detect it by comparing values alone. + private viewLocalStateWriteVersions = { mode: 0, currentApiConfigName: 0, apiConfiguration: 0 } + /** * This view's pinned profile name from the view-local buffer. Exposed for * sibling-instance inspection (getAllInstances() filtering): `viewLocalState` @@ -5865,7 +5869,14 @@ export class ClineProvider * 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 bumpViewLocalStateWriteVersions(values: Partial & Partial): void { + if ("mode" in values) this.viewLocalStateWriteVersions.mode += 1 + if ("currentApiConfigName" in values) this.viewLocalStateWriteVersions.currentApiConfigName += 1 + if ("apiConfiguration" in values) this.viewLocalStateWriteVersions.apiConfiguration += 1 + } + private _updateViewLocalStateFromMutation(values: Partial & Partial): void { + this.bumpViewLocalStateWriteVersions(values) if ("mode" in values) { const val = values.mode if (val === undefined || val === null) { @@ -5953,6 +5964,7 @@ export class ClineProvider // 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 } + const preLoadVersions = { ...this.viewLocalStateWriteVersions } if (persisted?.mode) { // A persisted mode may reference a custom mode that was deleted after it was @@ -5991,7 +6003,7 @@ export class ClineProvider const postLoadBuffer = this.viewLocalState const mergedState: Partial = { ...loadedState } - if (!Object.is(preLoadBuffer.mode, postLoadBuffer.mode)) { + if (preLoadVersions.mode !== this.viewLocalStateWriteVersions.mode) { if (postLoadBuffer.mode === undefined) { delete mergedState.mode } else { @@ -5999,7 +6011,7 @@ export class ClineProvider } } - if (!Object.is(preLoadBuffer.currentApiConfigName, postLoadBuffer.currentApiConfigName)) { + if (preLoadVersions.currentApiConfigName !== this.viewLocalStateWriteVersions.currentApiConfigName) { if (postLoadBuffer.currentApiConfigName === undefined) { delete mergedState.currentApiConfigName } else { @@ -6007,7 +6019,7 @@ export class ClineProvider } } - if (!Object.is(preLoadBuffer.apiConfiguration, postLoadBuffer.apiConfiguration)) { + if (preLoadVersions.apiConfiguration !== this.viewLocalStateWriteVersions.apiConfiguration) { if (postLoadBuffer.apiConfiguration === undefined) { delete mergedState.apiConfiguration } else { diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 5033c06ac4..c338dde9f4 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -5156,6 +5156,7 @@ describe("ClineProvider", () => { } // Spy on log method to verify warning was logged + const logSpy = vi.spyOn(provider, "log") // Create history item with non-existent mode const historyItem = { id: "test-id", @@ -5648,6 +5649,7 @@ describe("ClineProvider", () => { // Registration is what loads a view's own selections. expect(provider["viewLocalState"].mode).toBe("architect") + expect(provider["viewLocalState"].currentApiConfigName).toBe("cfg-a") await provider.dispose() }) From 14a136af8631e32e5e7102e38876583d4e07ff10 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 30 Oct 2026 13:13:56 +0800 Subject: [PATCH 08/27] chore: tighten the N1 test comments (cherry picked from commit 9cb3635e033428ee74c1a63fc67ccb447fd41320) --- src/core/webview/__tests__/ClineProvider.spec.ts | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index c338dde9f4..778f16f4d4 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -5622,9 +5622,7 @@ describe("ClineProvider", () => { 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. + // "__proto__" is rejected before assignment, so nothing is persisted under it. expect(provider["viewStateId"]).toBe(provider.viewId) expect(mockContext.globalState.get("viewStates")).toBeUndefined() expect(provider["viewLocalState"]).toEqual({}) @@ -5663,21 +5661,17 @@ describe("ClineProvider", () => { 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. + // Seed a pre-launch entry through the real write path so the re-key has work to do. 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. + // The failed id must not stick: the provider keeps its previous (temporary) id. 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. + // A later retry succeeds once the storage write works again. setValueSpy.mockRestore() await provider["setViewStateId"]("stable-sidebar-view") expect(provider["viewStateId"]).toBe("stable-sidebar-view") From 51982e7c550b8572a10d1766a5f8036687ad2519 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 30 Oct 2026 14:12:48 +0800 Subject: [PATCH 09/27] fix(lint): drop the now-unused load snapshot and log spy (cherry picked from commit ff4ae389a1d61d27fe588e71a45363a6df447c6d) --- src/core/webview/ClineProvider.ts | 1 - src/core/webview/__tests__/ClineProvider.spec.ts | 1 - 2 files changed, 2 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 36dbd1efa1..cb56ffb71a 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -5963,7 +5963,6 @@ export class ClineProvider // 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 } const preLoadVersions = { ...this.viewLocalStateWriteVersions } if (persisted?.mode) { diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 778f16f4d4..b198558654 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -5641,7 +5641,6 @@ describe("ClineProvider", () => { await provider.contextProxy.setValue("viewStates", { "stable-view": { claimToken: "host-token", mode: "architect", currentApiConfigName: "cfg-a" }, }) - const logSpy = vi.spyOn(provider, "log") await provider["setViewStateId"]("stable-view", "host-token") From 7620684f789ec1c29fef9c11f6bb82b967726293 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Wed, 4 Nov 2026 02:30:48 +0800 Subject: [PATCH 10/27] fix(webview): harden viewStates reads and retry an unrestored view state Two review findings on #2226. 1. getPersistedViewStates copied every entry blindly. A corrupt or half-written viewStates record holding a null entry made prunePersistedViewStates throw while it sorted by updatedAt, which rejected the queued save and lost the selection being stored. Entries that are not objects are now dropped at the single read choke point (both the cached and the fresh copy), so every writer - save, re-bind, re-key and the claim check - sees only valid entries. 2. loadViewState swallowed a getCustomModes failure and returned normally, so a failed restoration was indistinguishable from a completed one: setViewStateId kept the adopted id, and every later registration of that id was swallowed by the unchanged-id guard, so the selections were never reloaded. A load that aborts now records the id as pending; a same-id registration passes the guard only while the id is pending and still runs the persisted-entry claim check. The pending marker is cleared only by a load that ran to completion. The id stays adopted after a successful re-key (the entry is already under the stable name), and the rollback still covers failures before that point. Tests: 3 new cases in ClineProvider.spec.ts - corrupt entry cannot reject a save, a failed load is retried by a same-id registration, a completed restoration stays behind the guard. (cherry picked from commit d018b9e43423facc1056e978e4b4cbf62e594d16) --- src/core/webview/ClineProvider.ts | 41 ++++++- .../webview/__tests__/ClineProvider.spec.ts | 110 ++++++++++++++++++ 2 files changed, 148 insertions(+), 3 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index cb56ffb71a..0748eb7d89 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -461,6 +461,15 @@ export class ClineProvider // load merge cannot detect it by comparing values alone. private viewLocalStateWriteVersions = { mode: 0, currentApiConfigName: 0, apiConfiguration: 0 } + /** + * View ids whose persisted selections failed to load. A load failure must not be + * indistinguishable from a successful one, because the unchanged-id guard in + * setViewStateId would then swallow every later registration of that id. A pending + * id is retried (claim check included) and removed from this set only after a load + * that ran to completion. + */ + private pendingViewStateRestorations = new Set() + /** * This view's pinned profile name from the view-local buffer. Exposed for * sibling-instance inspection (getAllInstances() filtering): `viewLocalState` @@ -674,8 +683,20 @@ export class ClineProvider // 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) + // prototype chain and be mistaken for a persisted entry. Entries that are not + // objects are dropped as well: a corrupt or hand-edited `viewStates` record can + // hold a null or primitive value under a key, and prunePersistedViewStates reads + // `updatedAt` from every entry it is given - one such entry rejects the queued + // write and loses the selection this writer was about to store. + const states: Record = Object.create(null) + + for (const [viewStateId, entry] of Object.entries(viewStates)) { + if (entry && typeof entry === "object") { + states[viewStateId] = entry + } + } + + return states } /** Persist the non-secret selections through the serialized write queue. */ @@ -871,9 +892,15 @@ export class ClineProvider const normalizedViewStateId = viewStateId.trim().replace(/[^A-Za-z0-9_-]/g, "_") + // A load that failed leaves the id adopted (the re-key already moved the entry to the + // stable name) with the selections unrestored, so the unchanged-id guard must not + // swallow the retry: while a restoration is pending for this exact id the + // registration continues, and the claim check below still has to pass. + const restorationPending = this.pendingViewStateRestorations.has(normalizedViewStateId) + if ( !normalizedViewStateId || - normalizedViewStateId === this.viewStateId || + (normalizedViewStateId === this.viewStateId && !restorationPending) || // 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 @@ -6027,8 +6054,16 @@ export class ClineProvider } this.viewLocalState = mergedState + this.pendingViewStateRestorations.delete(loadedForViewId) this.log(`[loadViewState] Loaded state for viewId ${this.viewId}`) } catch (error) { + // The load aborted, and `setViewStateId` keeps the id it adopted because the + // entry is already stored under the stable name. Record that this id is unrestored + // so the next registration of the same id is not swallowed by the unchanged-id + // guard; a completed load above clears it. + if (this.viewStateId === loadedForViewId) { + this.pendingViewStateRestorations.add(loadedForViewId) + } this.log( `[loadViewState] Error loading state for viewId ${this.viewId}: ${error instanceof Error ? error.message : String(error)}`, ) diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index b198558654..6f08a58a6f 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -15,6 +15,7 @@ import { type ClineMessage, type ExtensionMessage, type ExtensionState, + type GlobalState, type WebviewMessage, ORGANIZATION_ALLOW_ALL, DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, @@ -5682,6 +5683,115 @@ describe("ClineProvider", () => { }) }) + describe("persisted viewStates record hygiene", () => { + it("drops non-object entries so a corrupt record cannot reject a save", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + // Test data only, hence the assertion: a null or primitive entry is exactly what the + // persisted schema forbids and what a half-written or hand-edited record can still hold. + const corruptStates = { + "corrupt-view": null, + "primitive-view": "not-an-entry", + "healthy-view": { claimToken: "host-token", mode: "code", updatedAt: 5 }, + } as unknown as GlobalState["viewStates"] + await provider.contextProxy.setValue("viewStates", corruptStates) + + // Both read paths filter: the ContextProxy-cached copy (loadViewState reads it) and the + // fresh storage copy every serialized writer reads. + expect(provider["getPersistedViewStates"]()).not.toHaveProperty("corrupt-view") + expect(provider["getPersistedViewStates"]()).not.toHaveProperty("primitive-view") + expect(provider["getPersistedViewStates"]({ fresh: true })["corrupt-view"]).toBeUndefined() + expect(provider["getPersistedViewStates"]({ fresh: true })["healthy-view"]).toMatchObject({ + claimToken: "host-token", + mode: "code", + }) + + // Before the filter this rejects: prunePersistedViewStates sorts every entry by + // `updatedAt`, and reading it from a null entry throws inside the queued write. + await provider["savePersistedViewState"]({ mode: "architect", updatedAt: 10 }) + + const stored = mockContext.globalState.get>("viewStates") ?? {} + expect(stored[provider["viewStateId"]]).toMatchObject({ mode: "architect" }) + expect(stored).not.toHaveProperty("corrupt-view") + expect(stored).not.toHaveProperty("primitive-view") + + await provider.dispose() + }) + }) + + describe("view state restoration retry", () => { + const createProvider = () => + new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + + it("reloads the selections of a view whose load failed when the same id is registered again", async () => { + const provider = createProvider() + await provider.contextProxy.setValue("viewStates", { + "retry-view": { + claimToken: "host-token", + mode: "architect", + currentApiConfigName: "cfg-a", + updatedAt: 1, + }, + }) + const getCustomModesSpy = vi + .spyOn(provider["customModesManager"], "getCustomModes") + .mockRejectedValue(new Error("custom modes read failed")) + + await provider["setViewStateId"]("retry-view", "host-token") + + // The id stays adopted: only the load aborted, and the entry is already stored under + // the stable name, so rolling back here would strand it under the temporary id. + expect(provider["viewStateId"]).toBe("retry-view") + expect(provider["viewLocalState"]).toStrictEqual({}) + expect(provider["pendingViewStateRestorations"].has("retry-view")).toBe(true) + + getCustomModesSpy.mockResolvedValue([]) + + // Same id, restoration still pending: the registration runs again, claim check included. + await provider["setViewStateId"]("retry-view", "host-token") + + expect(provider["viewLocalState"].mode).toBe("architect") + expect(provider["viewLocalState"].currentApiConfigName).toBe("cfg-a") + expect(provider["pendingViewStateRestorations"].has("retry-view")).toBe(false) + + getCustomModesSpy.mockRestore() + await provider.dispose() + }) + + it("keeps the same-id guard closed once a restoration completed", async () => { + const provider = createProvider() + await provider.contextProxy.setValue("viewStates", { + "loaded-view": { claimToken: "host-token", mode: "architect", updatedAt: 1 }, + }) + + await provider["setViewStateId"]("loaded-view", "host-token") + + expect(provider["viewLocalState"].mode).toBe("architect") + + // Change what is persisted behind this view's back; a completed restoration is not + // re-run by a repeated registration, so the buffer keeps the loaded selection. + await provider.contextProxy.setValue("viewStates", { + "loaded-view": { claimToken: "host-token", mode: "code", updatedAt: 2 }, + }) + await provider["setViewStateId"]("loaded-view", "host-token") + + expect(provider["viewLocalState"].mode).toBe("architect") + expect(provider["pendingViewStateRestorations"].size).toBe(0) + + await provider.dispose() + }) + }) describe("view state claim binding", () => { it("refuses a viewStateId bound by a concurrent registration through setViewStateId", async () => { const provider = new ClineProvider( From d7145a8399761f35c6a44d819aab117c76933f78 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 02:52:18 +0800 Subject: [PATCH 11/27] fix(config): drop viewStates from settings imports CodeRabbit's Security Boundaries pre-merge check flagged that a settings file can carry viewStates, whose claimToken is host-minted: an import could plant an entry plus its claim and replay it as that view's identity. The export side already omits the key; the import side now consults a shared non-portable-key list in sanitizeGlobalSettings and reports the skip as an import warning. Test: importExport.spec.ts, host-owned view state on import. Negative control: dropping the import guard turns the new assertion red (the applied settings regain a viewStates key); the production file was restored byte-identically and the suite re-ran green. (cherry picked from commit 96226a37119a2121a54653e783ab2f959a87d3d0) --- src/core/config/ContextProxy.ts | 10 ++++ .../config/__tests__/importExport.spec.ts | 49 +++++++++++++++++++ src/core/config/importExport.ts | 9 +++- 3 files changed, 67 insertions(+), 1 deletion(-) diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index 28301337e4..bcc9557113 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -41,6 +41,16 @@ const globalSettingsExportSchema = globalSettingsSchema.omit({ viewStates: true, }) +/** + * Global settings a settings file may not carry in either direction. Per-view selections and the + * claim token that binds them are minted and held by the extension host, so a file that could + * set them would be replayable as another view's identity. The export side omits the key above; + * the import side consults this list in `sanitizeGlobalSettings`. + */ +const NON_PORTABLE_SETTINGS_KEYS: readonly string[] = ["viewStates"] + +export const isNonPortableSettingsKey = (key: string): boolean => NON_PORTABLE_SETTINGS_KEYS.includes(key) + export class ContextProxy { private readonly originalContext: vscode.ExtensionContext diff --git a/src/core/config/__tests__/importExport.spec.ts b/src/core/config/__tests__/importExport.spec.ts index 6c66655325..4ea47d2298 100644 --- a/src/core/config/__tests__/importExport.spec.ts +++ b/src/core/config/__tests__/importExport.spec.ts @@ -251,6 +251,55 @@ describe("importExport", () => { }) }) + describe("host-owned view state on import", () => { + const settingsWithImportedViewStates = JSON.stringify({ + providerProfiles: { + currentApiConfigName: "test", + apiConfigs: { + test: { apiProvider: providerIdentifiers.openai, apiKey: "test-key", id: "test-id" }, + }, + }, + globalSettings: { + customInstructions: "keep me", + viewStates: { "view-1": { mode: "architect", claimToken: "planted-claim" } }, + }, + }) + + beforeEach(() => { + ;(fs.readFile as Mock).mockResolvedValue(settingsWithImportedViewStates) + mockProviderSettingsManager.export.mockResolvedValue({ + currentApiConfigName: "default", + apiConfigs: { default: { apiProvider: providerIdentifiers.anthropic, id: "default-id" } }, + }) + mockProviderSettingsManager.import.mockResolvedValue(undefined) + mockProviderSettingsManager.listConfig.mockResolvedValue([]) + mockContextProxy.setValues.mockResolvedValue(undefined) + mockContextProxy.setValue.mockResolvedValue(undefined) + mockContextProxy.setProviderSettings.mockResolvedValue(undefined) + }) + + it("drops viewStates so an imported file cannot plant another view's claim", async () => { + const result = await importSettingsFromPath("/mock/path/settings.json", { + providerSettingsManager: mockProviderSettingsManager, + contextProxy: mockContextProxy, + customModesManager: mockCustomModesManager, + }) + + expect(result).toMatchObject({ success: true }) + + // A claim token is host-minted: a file that could carry an entry could replay it as that + // view's identity, so the key is dropped before anything is written. + expect(mockContextProxy.setValues).toHaveBeenCalledTimes(1) + const [appliedSettings] = (mockContextProxy.setValues as Mock).mock.calls[0] + expect(appliedSettings).not.toHaveProperty("viewStates") + expect(appliedSettings).toMatchObject({ customInstructions: "keep me" }) + const warnings = (result as { warnings?: string[] }).warnings ?? [] + expect(warnings).toHaveLength(1) + expect(warnings[0]).toContain('globalSettings.viewStates" was skipped') + expect(warnings[0]).toContain("not portable") + }) + }) + describe("importSettings", () => { it("should return success: false when user cancels file selection", async () => { ;(vscode.window.showOpenDialog as Mock).mockResolvedValue(undefined) diff --git a/src/core/config/importExport.ts b/src/core/config/importExport.ts index 9882039ad3..1a41a29d89 100644 --- a/src/core/config/importExport.ts +++ b/src/core/config/importExport.ts @@ -17,7 +17,7 @@ import { import { TelemetryService } from "@roo-code/telemetry" import { ProviderSettingsManager, providerProfilesSchema } from "./ProviderSettingsManager" -import { ContextProxy } from "./ContextProxy" +import { ContextProxy, isNonPortableSettingsKey } from "./ContextProxy" import { CustomModesManager } from "./CustomModesManager" import { downgradeLegacyRooConfig, ROUTER_REMOVAL_IMPORT_WARNING } from "./routerRemoval" import { resolveDefaultSaveUri, saveLastExportPath } from "../../utils/export" @@ -105,6 +105,13 @@ function sanitizeGlobalSettings(rawGlobalSettings: unknown): { continue } + if (isNonPortableSettingsKey(key)) { + warnings.push( + `Setting "${path}" was skipped: per-view state and its host-minted claim token are not portable through settings files.`, + ) + continue + } + let valueToValidate = rawValue if (key === "imageGenerationProvider" && rawValue === retiredProviderIdentifiers.roo) { From 86461a524cac31b8d5bacad1b5035622c2e9b4c7 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 02:52:31 +0800 Subject: [PATCH 12/27] fix(webview): await view-state claim delivery and report an undeliverable claim CodeRabbit's Persistence Integrity pre-merge check flagged that claim delivery was fired and forgotten once the durable write landed. postViewStateClaimToWebview now awaits the post, reports whether it landed, and logs when the post throws or is dropped because no webview is attached; savePersistedViewState, the legacy re-bind and the registration path all await it. Tests: ClineProvider.spec.ts view state claim binding gains a gated-post test (the write must not resolve before the post settles) and an undeliverable-claim test (the gap reaches the log). Negative controls: swapping the await for void turns the gated test red; deleting the log turns the undeliverable test red; both mutants restored byte-identically and the suite re-ran green. (cherry picked from commit 2f9b661466e1c14b6cc922a96281b99045b0bf15) --- src/core/webview/ClineProvider.ts | 31 ++++++++++++--- .../webview/__tests__/ClineProvider.spec.ts | 38 +++++++++++++++++++ 2 files changed, 64 insertions(+), 5 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 0748eb7d89..61cce3b8bb 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -766,7 +766,7 @@ export class ClineProvider // 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) + await this.postViewStateClaimToWebview(viewStateId, mintedClaimToken) } } @@ -813,9 +813,30 @@ export class ClineProvider * 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. + * + * Awaited rather than fired and forgotten: the entry already carries this claim once the + * write lands, so a view that never receives it would be missing an identity the host + * believes it holds. `postMessageToWebview` drops a message silently while no webview is + * attached (or once the provider is disposed), so the gap is reported here; the post + * itself stays the single place that records the webview-out diagnostic. */ - private postViewStateClaimToWebview(viewStateId: string, claimToken: string): void { - void this.postMessageToWebview({ type: "viewStateClaim", viewStateId, viewStateClaim: claimToken }) + private async postViewStateClaimToWebview(viewStateId: string, claimToken: string): Promise { + try { + await this.postMessageToWebview({ type: "viewStateClaim", viewStateId, viewStateClaim: claimToken }) + } catch (error) { + const reason = error instanceof Error ? error.message : String(error) + this.log(`[viewStateClaim] Delivery for ${viewStateId} to view ${this.viewId} failed: ${reason}`) + return false + } + + if (!this.view) { + this.log( + `[viewStateClaim] Claim for ${viewStateId} not delivered to view ${this.viewId}: no webview attached.`, + ) + return false + } + + return true } /** @@ -958,7 +979,7 @@ export class ClineProvider return } - this.postViewStateClaimToWebview(normalizedViewStateId, reboundClaim) + await this.postViewStateClaimToWebview(normalizedViewStateId, reboundClaim) this.log( `[setViewStateId] Re-bound claim-less viewStates entry ${normalizedViewStateId} for view ${this.viewId}: unbound selections were not loaded.`, ) @@ -988,7 +1009,7 @@ export class ClineProvider const adoptedClaim = this.getPersistedViewStates({ fresh: true })[this.viewStateId]?.claimToken if (adoptedClaim) { - this.postViewStateClaimToWebview(this.viewStateId, adoptedClaim) + await this.postViewStateClaimToWebview(this.viewStateId, adoptedClaim) } // Registration is what loads this view's own persisted selections. diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 6f08a58a6f..8b762c3843 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -5939,6 +5939,44 @@ describe("ClineProvider", () => { ).toHaveLength(1) }) + test("resolves the persistence write only after the minted claim has been posted", async () => { + const gated = createProvider() + let posted = false + const postSpy = vi.spyOn(gated, "postMessageToWebview").mockImplementation(async () => { + // A real webview post settles on a later turn: a write that reported success before + // this resolved would leave the view without the claim it just made durable. + await new Promise((resolve) => setTimeout(resolve, 0)) + posted = true + }) + + await gated.saveViewState("mode", "architect") + + expect(storedEntries()[gated.viewId]?.claimToken).toEqual(expect.any(String)) + expect(posted).toBe(true) + + postSpy.mockRestore() + await gated.dispose() + }) + + test("logs a claim that cannot be delivered because no webview is attached", async () => { + const detached = createProvider() + // Nothing is attached in this harness: the post is dropped and the claim is durable + // anyway, so the gap has to reach the log instead of leaving a silent mismatch. + expect(detached["view"]).toBeUndefined() + const loggedBefore = (mockOutputChannel.appendLine as ReturnType).mock.calls.length + + await detached.saveViewState("mode", "architect") + + const loggedLines: string[] = (mockOutputChannel.appendLine as ReturnType).mock.calls + .slice(loggedBefore) + .map((call: unknown[]) => String(call[0])) + const claimLines = loggedLines.filter((line) => line.includes("[viewStateClaim]")) + expect(claimLines).toHaveLength(1) + expect(claimLines[0]).toContain("not delivered") + + await detached.dispose() + }) + 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 From 34837027958f18665551cc4e32cd2cdfa99d67a3 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 03:37:44 +0800 Subject: [PATCH 13/27] test(webview): cover malformed view id rejections in setViewStateId CodeRabbit's Regression Evidence row asked for four focused rejections of a caller-supplied view id, because each one becomes a key in the shared viewStates map if it is accepted: a runtime non-string value (ClineProvider.ts:900-905), an empty id and a whitespace-only id that normalizes to empty (:915-925), and an id longer than the 64-character bound (:929-934). The file covered only the prototype-chain name. Each case asserts the three things that must stay untouched on rejection: viewStateId still equals the temporary viewId, viewStates is unchanged, and no viewStateClaim message is posted. The guard's log line is asserted where the guard logs one; the empty and whitespace cases return silently, so their evidence is the absence of every side effect. Negative controls, each run against production code and restored byte-identically (sha256-12 3c5a313cb644): non-string guard removed: TypeError: viewStateId.trim is not a function - the crafted payload reaches normalization empty-id clause removed: AssertionError: expected '' to be 'sidebar-1' length bound removed: AssertionError: expected the 65-character id to be 'sidebar-1' All three restored and re-ran green, and the existing rejection and adoption tests are untouched. Full ClineProvider.spec.ts at this commit: 333 passed. eslint --max-warnings=0 --no-warn-ignored clean, prettier clean, tsc --noEmit clean, suppression counts unmoved. (cherry picked from commit e5904ba83829fb86ec232071589a31e9acc1d62e) --- .../webview/__tests__/ClineProvider.spec.ts | 127 ++++++++++++++++++ 1 file changed, 127 insertions(+) diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 8b762c3843..d1c757ba40 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -5631,6 +5631,133 @@ describe("ClineProvider", () => { await provider.dispose() }) + it("rejects a non-string viewStateId from a crafted message and stays on the temporary id", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + // The message type declares a string, but a crafted postMessage can carry any JSON + // value, so the double assertion here is the crafted payload, not the call site. + const crafted = JSON.parse('{"viewStateId": 42}') as { viewStateId: string } + const statesBefore = mockContext.globalState.get("viewStates") + const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(undefined) + const loggedBefore = (mockOutputChannel.appendLine as ReturnType).mock.calls.length + + await provider["setViewStateId"](crafted.viewStateId) + + expect(provider["viewStateId"]).toBe(provider.viewId) + // Nothing registered means nothing persisted, nothing loaded, and no claim posted. + expect(mockContext.globalState.get("viewStates")).toBe(statesBefore) + expect(provider["viewLocalState"]).toEqual({}) + expect( + postMessageSpy.mock.calls + .map(([message]) => message) + .filter((message) => message.type === "viewStateClaim"), + ).toHaveLength(0) + const loggedLines = (mockOutputChannel.appendLine as ReturnType).mock.calls + .slice(loggedBefore) + .map((call: unknown[]) => String(call[0])) + expect( + loggedLines.some((line) => line.includes("[setViewStateId] Ignoring a non-string viewStateId")), + ).toBe(true) + + postMessageSpy.mockRestore() + await provider.dispose() + }) + + it("rejects an empty viewStateId and keeps the temporary viewId", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const statesBefore = mockContext.globalState.get("viewStates") + const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(undefined) + + await provider["setViewStateId"]("") + + // The empty id returns without a log line; the absence of every side effect is the + // evidence that registration never happened. + expect(provider["viewStateId"]).toBe(provider.viewId) + // Nothing registered means nothing persisted, nothing loaded, and no claim posted. + expect(mockContext.globalState.get("viewStates")).toBe(statesBefore) + expect(provider["viewLocalState"]).toEqual({}) + expect( + postMessageSpy.mock.calls + .map(([message]) => message) + .filter((message) => message.type === "viewStateClaim"), + ).toHaveLength(0) + postMessageSpy.mockRestore() + await provider.dispose() + }) + + it("rejects a whitespace-only viewStateId that normalizes to empty", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const statesBefore = mockContext.globalState.get("viewStates") + const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(undefined) + + // Whitespace normalizes to the empty string, which is the same rejection. + await provider["setViewStateId"](" ") + + expect(provider["viewStateId"]).toBe(provider.viewId) + // Nothing registered means nothing persisted, nothing loaded, and no claim posted. + expect(mockContext.globalState.get("viewStates")).toBe(statesBefore) + expect(provider["viewLocalState"]).toEqual({}) + expect( + postMessageSpy.mock.calls + .map(([message]) => message) + .filter((message) => message.type === "viewStateClaim"), + ).toHaveLength(0) + postMessageSpy.mockRestore() + await provider.dispose() + }) + + it("rejects a viewStateId longer than the bound and logs the length", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const statesBefore = mockContext.globalState.get("viewStates") + const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(undefined) + const loggedBefore = (mockOutputChannel.appendLine as ReturnType).mock.calls.length + + // The webview generates a UUID; a 65-character value is not a view id and an + // unbounded key is unbounded growth in the shared viewStates map. + await provider["setViewStateId"]("v".repeat(65)) + + expect(provider["viewStateId"]).toBe(provider.viewId) + // Nothing registered means nothing persisted, nothing loaded, and no claim posted. + expect(mockContext.globalState.get("viewStates")).toBe(statesBefore) + expect(provider["viewLocalState"]).toEqual({}) + expect( + postMessageSpy.mock.calls + .map(([message]) => message) + .filter((message) => message.type === "viewStateClaim"), + ).toHaveLength(0) + const loggedLines = (mockOutputChannel.appendLine as ReturnType).mock.calls + .slice(loggedBefore) + .map((call: unknown[]) => String(call[0])) + expect(loggedLines.some((line) => line.includes("Ignoring an oversized viewStateId (65 chars)"))).toBe(true) + expect(mockContext.globalState.get("viewStates")).toBe(statesBefore) + + postMessageSpy.mockRestore() + await provider.dispose() + }) + it("loads the persisted selections of the registered view", async () => { const provider = new ClineProvider( mockContext, From 66b549936a2895b2388c30fd1979e494d829a6b0 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 03:38:01 +0800 Subject: [PATCH 14/27] fix(webview): cancel queued view-state work with a disposal generation View-state persistence is serialized on the static persistedViewStateWriteQueue, which every provider shares. A closure queued by one provider therefore runs whenever the queue head releases, not when its own provider is alive: dispose() left those closures untouched, so a delayed write kept the disposed provider retained and then wrote global state, mutated wroteUnderTemporaryViewStateId, and delivered a viewStateClaim to a webview that no longer existed. A per-provider cancellation generation now gates that work. savePersistedViewState, clearPersistedViewState, rebindUnboundViewStateEntry and rekeyPersistedViewStateEntry each capture the generation they were scheduled under and drop their own effects when isViewStateGenerationLive() says otherwise: once before the queued closure runs, once after the awaited persistence and before any provider field is touched, and once before a minted claim is delivered. dispose() bumps the generation at its start, so everything already queued for this view is invalidated before it tears anything else down. Registration applies the same check before posting an adopted claim. Skipping is per provider, never for the queue: another view still writing cannot be blocked or drained by this one, and a write whose storage call already landed is left durable - only the post-write effects are dropped. Focused tests (ClineProvider.spec.ts, describe "view state writes that outlive the provider") and their negative controls, each mutant restored byte-identically (sha256-12 3c5a313cb644): queued write applied after dispose: AssertionError: expected "setValue" to not be called at all, but actually been called 1 times re-key applied after dispose: AssertionError: expected undefined to be 'architect' post-write guard removed: AssertionError: expected true to be false (authorship recorded on a disposed provider) delivery-site guard removed with the post-write guard present: AssertionError: expected [ { type: 'viewStateClaim', ... } ] to have a length of +0 but got 1 The last pair shows the two post-write checks are independent: one protects the provider fields, the other the claim. Full ClineProvider.spec.ts after the change: 336 passed. eslint --max-warnings=0 --no-warn-ignored clean on both files, tsc --noEmit clean, prettier clean, suppression counts unmoved. (cherry picked from commit 2964023ce32a45719996b95e3b804d332f3ef063) --- src/core/webview/ClineProvider.ts | 69 ++++++++- .../webview/__tests__/ClineProvider.spec.ts | 132 ++++++++++++++++++ 2 files changed, 199 insertions(+), 2 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 61cce3b8bb..da171490b9 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -450,6 +450,17 @@ export class ClineProvider */ private wroteUnderTemporaryViewStateId = false + /** + * Bumped at the start of dispose(). View-state persistence is serialized on a + * static queue shared by every provider, so a closure this instance scheduled can + * start after the view is gone - behind another provider's write. Each closure + * captures the generation it was scheduled under and drops its own effects (the + * global-state write, provider-field mutation, claim delivery) once the value no + * longer matches, instead of applying them to a disposed provider and holding it + * retained until the queue advances. + */ + private viewStateGeneration = 0 + /** * Local state buffer for this specific view instance. * Used to isolate mode, apiConfiguration, and other fields from the shared ContextProxy singleton @@ -699,14 +710,26 @@ export class ClineProvider return states } + /** Whether view-state work scheduled under `generation` may still apply effects. */ + private isViewStateGenerationLive(generation: number): boolean { + return !this._disposed && this.viewStateGeneration === generation + } + /** 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. const viewStateId = this.viewStateId + const generation = this.viewStateGeneration // 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 () => { + if (!this.isViewStateGenerationLive(generation)) { + // The queue advanced after this view was disposed: the entry belongs to a view + // that no longer exists, so the write is dropped instead of applied. + return + } + 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 @@ -752,6 +775,13 @@ export class ClineProvider } await this.contextProxy.setValue("viewStates", this.prunePersistedViewStates(states)) + + if (!this.isViewStateGenerationLive(generation)) { + // The view went away while the write was in flight: the selections are durable, + // but no provider field may be touched afterwards. + return + } + // 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. @@ -765,7 +795,8 @@ export class ClineProvider // 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) { + // A provider disposed in the meantime has no webview left to deliver it to. + if (mintedClaimToken && this.isViewStateGenerationLive(generation)) { await this.postViewStateClaimToWebview(viewStateId, mintedClaimToken) } } @@ -775,7 +806,14 @@ export class ClineProvider * Runs through the serialized write queue to avoid racing concurrent view-state writes. */ private async clearPersistedViewState(viewStateId = this.viewStateId): Promise { + const generation = this.viewStateGeneration const write = ClineProvider.persistedViewStateWriteQueue.then(async () => { + if (!this.isViewStateGenerationLive(generation)) { + // A clear that only now reaches the queue head belongs to a disposed view: + // its entry is already unreferenced by this provider, so leave the map alone. + return + } + const states = this.getPersistedViewStates({ fresh: true }) delete states[viewStateId] await this.contextProxy.setValue("viewStates", states) @@ -789,8 +827,15 @@ export class ClineProvider private async rebindUnboundViewStateEntry(viewStateId: string): Promise { const claimToken = crypto.randomUUID() let rebound = false + const generation = this.viewStateGeneration const write = ClineProvider.persistedViewStateWriteQueue.then(async () => { + if (!this.isViewStateGenerationLive(generation)) { + // Nothing is re-bound for a view that is gone, and the undefined return makes + // the caller refuse the adoption instead of delivering a claim nobody owns. + return + } + const states = this.getPersistedViewStates({ fresh: true }) const current = states[viewStateId] // A concurrent registration may have bound this key while this write was queued: @@ -854,8 +899,15 @@ export class ClineProvider /** Re-key this instance's authored pre-launch entry to the stable id. */ private async rekeyPersistedViewStateEntry(nextViewStateId: string): Promise { const previousViewStateId = this.viewId + const generation = this.viewStateGeneration const write = ClineProvider.persistedViewStateWriteQueue.then(async () => { + if (!this.isViewStateGenerationLive(generation)) { + // A re-key that only now reaches the queue head belongs to a disposed view: + // the entry stays where it is for the pruning pass to collect. + return + } + // 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. @@ -889,6 +941,12 @@ export class ClineProvider await this.contextProxy.setValue("viewStates", this.prunePersistedViewStates(states)) + if (!this.isViewStateGenerationLive(generation)) { + // The move landed but the view is gone: keep the authorship flag as dispose + // left it rather than mutating a disposed provider. + return + } + // 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 @@ -995,6 +1053,7 @@ export class ClineProvider } const previousViewStateId = this.viewStateId + const generation = this.viewStateGeneration this.viewStateId = normalizedViewStateId @@ -1008,7 +1067,9 @@ export class ClineProvider // 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) { + // A registration whose view was disposed mid-flight must not deliver a claim to + // a webview that is no longer there. + if (adoptedClaim && this.isViewStateGenerationLive(generation)) { await this.postViewStateClaimToWebview(this.viewStateId, adoptedClaim) } @@ -1374,6 +1435,10 @@ export class ClineProvider return } + // Invalidate view-state work that has not reached the queue head yet: it is + // serialized on the static queue with other providers' writes, so it can start + // after this provider is gone. + this.viewStateGeneration++ this._disposed = true this._postStateToWebviewThrottled.cancel() this.stopWebviewWatchdog() diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index d1c757ba40..769a242238 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -6191,6 +6191,138 @@ describe("ClineProvider", () => { await provider.dispose() }) + + describe("view state writes that outlive the provider", () => { + const createProvider = () => + new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + + const storedEntries = () => + mockContext.globalState.get>("viewStates") ?? {} + + beforeEach(() => { + // The write queue is static and shared: start from a drained queue so a closure an + // earlier test left on it cannot hold this one open. + ClineProvider["persistedViewStateWriteQueue"] = Promise.resolve() + }) + + it("drops a queued write instead of applying it to a disposed provider", async () => { + const blocked = createProvider() + let releaseBlocked!: () => void + const blockedGate = new Promise((resolve) => { + releaseBlocked = resolve + }) + // The first provider's storage write stalls, so the shared queue stays taken and + // the second provider's closure waits behind it. + const blockedWriteSpy = vi.spyOn(blocked.contextProxy, "setValue").mockImplementation(async () => { + await blockedGate + }) + + const victim = createProvider() + const victimWriteSpy = vi.spyOn(victim.contextProxy, "setValue") + const postSpy = vi.spyOn(victim, "postMessageToWebview").mockResolvedValue(undefined) + + const blockedWrite = blocked.saveViewState("mode", "plan") + // Queued behind the stalled write: none of the victim's work has started yet. + const victimWrite = victim.saveViewState("mode", "architect") + + await victim.dispose() + releaseBlocked() + await victimWrite + await blockedWrite + + // The queue only advances this far once the view is gone: no global-state write, + // no provider-field mutation, no claim handed to a webview that no longer exists, + // and nothing stored under the view's id. + expect(victimWriteSpy).not.toHaveBeenCalled() + expect(victim["wroteUnderTemporaryViewStateId"]).toBe(false) + expect(storedEntries()[victim.viewId]).toBeUndefined() + expect( + postSpy.mock.calls.map(([message]) => message).filter((message) => message.type === "viewStateClaim"), + ).toHaveLength(0) + + blockedWriteSpy.mockRestore() + victimWriteSpy.mockRestore() + postSpy.mockRestore() + await blocked.dispose() + }) + + it("leaves a queued re-key alone when the provider is disposed while it waits", async () => { + const blocked = createProvider() + let releaseBlocked!: () => void + const blockedGate = new Promise((resolve) => { + releaseBlocked = resolve + }) + const blockedWriteSpy = vi.spyOn(blocked.contextProxy, "setValue").mockImplementation(async () => { + await blockedGate + }) + + const victim = createProvider() + // Author a pre-launch entry for real, so the re-key has work to do. + await victim["savePersistedViewState"]({ mode: "architect", updatedAt: 1 }) + expect(storedEntries()[victim.viewId]?.mode).toBe("architect") + expect(victim["wroteUnderTemporaryViewStateId"]).toBe(true) + + const blockedWrite = blocked.saveViewState("mode", "plan") + // The re-key is queued behind the stalled write and has not started yet. + const victimRekey = victim["rekeyPersistedViewStateEntry"]("stable-view") + + await victim.dispose() + releaseBlocked() + await victimRekey + await blockedWrite + + // Neither the move nor the authorship clearing may run after dispose: the entry + // stays under the id it was written under and the flag keeps its value. + expect(storedEntries()[victim.viewId]?.mode).toBe("architect") + expect(storedEntries()["stable-view"]).toBeUndefined() + expect(victim["wroteUnderTemporaryViewStateId"]).toBe(true) + + blockedWriteSpy.mockRestore() + await blocked.dispose() + }) + + it("skips post-write field mutation and claim delivery for a disposed provider", async () => { + const victim = createProvider() + let releaseWrite!: () => void + const writeGate = new Promise((resolve) => { + releaseWrite = resolve + }) + // Only the viewStates write is held open, so the rest of dispose() still runs. + const setValueSpy = vi.spyOn(victim.contextProxy, "setValue").mockImplementation(async (key) => { + if (key === "viewStates") { + await writeGate + } + }) + const postSpy = vi.spyOn(victim, "postMessageToWebview").mockResolvedValue(undefined) + + const save = victim.saveViewState("mode", "architect") + // Let the queued closure reach its storage write, then dispose while it is in + // flight: the durable write is already issued, the effects after it are not. + await vi.waitFor(() => { + expect(setValueSpy.mock.calls.some(([key]) => key === "viewStates")).toBe(true) + }) + + await victim.dispose() + releaseWrite() + await save + + expect(setValueSpy.mock.calls.some(([key]) => key === "viewStates")).toBe(true) + // The write itself was allowed to land; nothing applied after it. + expect(victim["wroteUnderTemporaryViewStateId"]).toBe(false) + expect( + postSpy.mock.calls.map(([message]) => message).filter((message) => message.type === "viewStateClaim"), + ).toHaveLength(0) + + setValueSpy.mockRestore() + postSpy.mockRestore() + }) + }) }) describe("webviewMessageHandler no-floating-promises coverage", () => { From f402de7c8af3bb8ddb0d93fc87d8162ae730228c Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 05:25:19 +0800 Subject: [PATCH 15/27] fix(webview): report claim delivery and retry an undelivered claim on attach postViewStateClaimToWebview could not tell a delivered claim from a dropped one: postMessageToWebview resolved void whether or not the webview accepted the post, so a hidden or closing webview (postMessage resolves false) left the view without an identity the host had already made durable, with nothing but the no-view log. postMessageToWebview now reports whether the webview accepted the message, the claim helper reports every failure mode (no view attached, rejected post, post resolving false), and an undelivered claim is queued per view and re-posted - carrying the exact token the persisted entry already stores - once a webview attaches. Disposal drops the queue so nothing re-posts into a view that is gone. The broadcast target and the doubles standing in for a successful post follow the new return: the delivery result is load-bearing now, so they resolve true. (cherry picked from commit 84a0d76442defc1d548278bad2b05f552b21a631) --- src/__tests__/extension.spec.ts | 2 +- src/core/webview/ClineProvider.ts | 107 ++++++++++++--- .../webview/__tests__/ClineProvider.spec.ts | 128 ++++++++++++++---- .../__tests__/api-start-new-task.spec.ts | 2 +- .../language-models/copilotModelBroadcast.ts | 2 +- 5 files changed, 196 insertions(+), 45 deletions(-) diff --git a/src/__tests__/extension.spec.ts b/src/__tests__/extension.spec.ts index abcd3c28a0..efc571867e 100644 --- a/src/__tests__/extension.spec.ts +++ b/src/__tests__/extension.spec.ts @@ -374,7 +374,7 @@ describe("extension.ts", () => { const getAll = vi.mocked(ClineProvider.getAllInstances) const post = vi.mocked(provider.postMessageToWebview) getAll.mockReturnValue([provider, provider]) - post.mockRejectedValueOnce(new Error("delivery failed")).mockResolvedValue(undefined) + post.mockRejectedValueOnce(new Error("delivery failed")).mockResolvedValue(true) try { const { activate } = await import("../extension") await activate(mockContext) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index da171490b9..a74f8f4db5 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -450,6 +450,14 @@ export class ClineProvider */ private wroteUnderTemporaryViewStateId = false + /** + * Claims this view minted that its webview has not accepted yet, keyed by view id and holding + * the exact token the durable entry carries. A claim is host-owned: the webview can never present + * an identity the host did not hand it, so an undelivered claim is retried when a webview attaches + * instead of being dropped, and nothing re-mints it in the meantime. + */ + private pendingViewStateClaims = new Map() + /** * Bumped at the start of dispose(). View-state persistence is serialized on a * static queue shared by every provider, so a closure this instance scheduled can @@ -855,35 +863,73 @@ export class ClineProvider } /** - * 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. + * Delivers the host-minted claim that governs a durable viewStates key to this view's webview + * and reports whether the webview accepted it. The webview only stores the token and echoes it + * back on the next launch: it can never mint or choose a claim itself. * - * Awaited rather than fired and forgotten: the entry already carries this claim once the - * write lands, so a view that never receives it would be missing an identity the host - * believes it holds. `postMessageToWebview` drops a message silently while no webview is - * attached (or once the provider is disposed), so the gap is reported here; the post - * itself stays the single place that records the webview-out diagnostic. + * Awaited rather than fired and forgotten: the entry already carries this claim once the write + * lands, so a view that never receives it would be missing an identity the host believes it + * holds. Every delivery failure is reported - no webview attached, the post rejected, or the + * webview resolving the post as not delivered - and the exact claim is queued for the next + * attach instead of dropped. The post itself stays the single place that records the + * webview-out diagnostic. */ private async postViewStateClaimToWebview(viewStateId: string, claimToken: string): Promise { + if (!this.view || this._disposed) { + this.pendingViewStateClaims.set(viewStateId, claimToken) + this.log( + `[viewStateClaim] Claim for ${viewStateId} not delivered to view ${this.viewId}: no webview attached.`, + ) + return false + } + + let delivered: boolean try { - await this.postMessageToWebview({ type: "viewStateClaim", viewStateId, viewStateClaim: claimToken }) + delivered = await this.postMessageToWebview({ + type: "viewStateClaim", + viewStateId, + viewStateClaim: claimToken, + }) } catch (error) { const reason = error instanceof Error ? error.message : String(error) + this.pendingViewStateClaims.set(viewStateId, claimToken) this.log(`[viewStateClaim] Delivery for ${viewStateId} to view ${this.viewId} failed: ${reason}`) return false } - if (!this.view) { + if (!delivered) { + // A hidden or closing webview resolves the post as not delivered: the claim stays queued so + // the same token reaches the webview once it can accept messages again. + this.pendingViewStateClaims.set(viewStateId, claimToken) this.log( - `[viewStateClaim] Claim for ${viewStateId} not delivered to view ${this.viewId}: no webview attached.`, + `[viewStateClaim] Claim for ${viewStateId} not delivered to view ${this.viewId}: the webview rejected the message.`, ) return false } + this.pendingViewStateClaims.delete(viewStateId) return true } + /** + * Re-delivers the claims this view minted that no webview accepted yet, called when a webview + * attaches. Each queued entry keeps the exact token the durable entry stores, so a webview that + * missed a delivery still gets the identity it has to present on the next launch; an entry that + * fails again is re-queued for the next attach. + */ + private async deliverPendingViewStateClaims(): Promise { + if (this._disposed || !this.view || this.pendingViewStateClaims.size === 0) { + return + } + + const pending = [...this.pendingViewStateClaims.entries()] + this.pendingViewStateClaims.clear() + + for (const [viewStateId, claimToken] of pending) { + await this.postViewStateClaimToWebview(viewStateId, 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. @@ -1440,6 +1486,8 @@ export class ClineProvider // after this provider is gone. this.viewStateGeneration++ this._disposed = true + // A claim that never reached a webview dies with this view: nothing may re-post it later. + this.pendingViewStateClaims.clear() this._postStateToWebviewThrottled.cancel() this.stopWebviewWatchdog() this.diagnosticsRecorder.record({ boundary: "provider", phase: "start", type: "dispose" }) @@ -1684,6 +1732,18 @@ export class ClineProvider this.webviewDisposables, ) + // A claim minted while no webview was attached, or while its post was rejected, is delivered + // now that this view exists: the durable entry already carries that token, and the webview + // cannot present an identity the host never handed it. A failed retry must not fail + // initialization, so it is contained here. + try { + await this.deliverPendingViewStateClaims() + } catch (error) { + this.log( + `[viewStateClaim] Retrying pending claims for view ${this.viewId} failed: ${error instanceof Error ? error.message : String(error)}`, + ) + } + if (inTabMode) { setPanel(webviewView, "tab") } else if ("onDidChangeVisibility" in webviewView) { @@ -2186,7 +2246,14 @@ export class ClineProvider return task } - public async postMessageToWebview(message: ExtensionMessage) { + /** + * Posts a message to this view's webview and reports whether the webview accepted it. + * + * Callers that only fire a notification may ignore the result. Callers that act on delivery must + * await it: `postMessage` resolves false when the webview is hidden or its panel is gone, and a + * void return cannot tell a delivered message from a dropped one. + */ + public async postMessageToWebview(message: ExtensionMessage): Promise { const startedAt = Date.now() const taskId = this.getCurrentTask()?.taskId this.diagnosticsRecorder.record({ @@ -2206,7 +2273,7 @@ export class ClineProvider elapsedMs: Date.now() - startedAt, taskId, }) - return + return false } try { @@ -2220,6 +2287,8 @@ export class ClineProvider stateSequence: message.type === "state" ? message.state?.clineMessagesSeq : undefined, taskId, }) + + return delivered === true } catch { this.diagnosticsRecorder.record({ boundary: "webview-out", @@ -2229,8 +2298,11 @@ export class ClineProvider elapsedMs: Date.now() - startedAt, taskId, }) - // View disposed, drop message silently + // View disposed, or the webview rejected the message: callers that act on delivery + // need the failure, so it is reported instead of swallowed. } + + return false } public requestWebviewThemeFixture(timeoutMs = 5_000): Promise { @@ -2292,10 +2364,9 @@ export class ClineProvider public requestWebviewDiagnostics(timeoutMs = 1_000): Promise { if (this._disposed || !this.view) return Promise.resolve(undefined) - return this.diagnosticsRequestBroker.request( - (requestId) => this.postMessageToWebview({ type: "diagnosticsRequest", requestId }), - timeoutMs, - ) + return this.diagnosticsRequestBroker.request(async (requestId) => { + await this.postMessageToWebview({ type: "diagnosticsRequest", requestId }) + }, timeoutMs) } private async getHMRHtmlContent(webview: vscode.Webview): Promise { diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 769a242238..736fb78590 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -3229,8 +3229,9 @@ describe("ClineProvider", () => { const message: ExtensionMessage = { type: "action", action: "chatButtonClicked" } - // Should not throw - await expect(provider.postMessageToWebview(message)).resolves.toBeUndefined() + // Should not throw, and must report the failed delivery rather than resolving as if the + // webview had taken the message. + await expect(provider.postMessageToWebview(message)).resolves.toBe(false) }) test("postMessageToWebview leaves originalContent of file-edit tool messages out of the state it posts", async () => { @@ -3315,7 +3316,7 @@ describe("ClineProvider", () => { }) test("posts a request and resolves the matching response", async () => { - const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(undefined) + const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(true) const request = provider.requestWebviewThemeFixture() await Promise.resolve() const requestId = postMessageSpy.mock.calls[0]?.[0].requestId @@ -3331,7 +3332,7 @@ describe("ClineProvider", () => { test("rejects a request after its timeout", async () => { vi.useFakeTimers() - vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(undefined) + vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(true) const request = provider.requestWebviewThemeFixture(100) const rejection = expect(request).rejects.toThrow("Theme fixture probe timed out after 100ms") @@ -3340,7 +3341,7 @@ describe("ClineProvider", () => { }) test("rejects pending requests when webview resources are cleared", async () => { - vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(undefined) + vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(true) const request = provider.requestWebviewThemeFixture() const rejection = expect(request).rejects.toThrow( "Webview was disposed before the theme fixture probe completed", @@ -3366,7 +3367,7 @@ describe("ClineProvider", () => { mdmService, ) - const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockImplementation(async () => undefined) + const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockImplementation(async () => true) vi.spyOn(provider as any, "getStateToPostToWebview").mockResolvedValue({ version: "1.0.0" }) await provider.postStateToWebview() @@ -3376,8 +3377,8 @@ describe("ClineProvider", () => { }) test("postStateToWebviewWithoutTaskHistory waits for the webview post boundary", async () => { - let releasePost!: () => void - const pendingPost = new Promise((resolve) => { + let releasePost!: (delivered: boolean) => void + const pendingPost = new Promise((resolve) => { releasePost = resolve }) let statePostSettled = false @@ -3396,7 +3397,7 @@ describe("ClineProvider", () => { expect(postMessageSpy).toHaveBeenCalledOnce() expect(statePostSettled).toBe(false) - releasePost() + releasePost(true) await statePost expect(statePostSettled).toBe(true) }) @@ -3423,7 +3424,7 @@ describe("ClineProvider", () => { vi.spyOn(provider, "getStateToPostToWebview") .mockReturnValueOnce(olderSnapshot) .mockResolvedValueOnce(readyState) - const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(undefined) + const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(true) const olderPost = postState(provider) await Promise.resolve() @@ -3450,7 +3451,7 @@ describe("ClineProvider", () => { ], ])("%s skips task history computation", async (_methodName, postState) => { const getAllSpy = vi.spyOn(provider.taskHistoryStore, "getAll") - const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(undefined) + const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(true) await postState(provider) @@ -5643,7 +5644,7 @@ describe("ClineProvider", () => { // value, so the double assertion here is the crafted payload, not the call site. const crafted = JSON.parse('{"viewStateId": 42}') as { viewStateId: string } const statesBefore = mockContext.globalState.get("viewStates") - const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(undefined) + const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(true) const loggedBefore = (mockOutputChannel.appendLine as ReturnType).mock.calls.length await provider["setViewStateId"](crafted.viewStateId) @@ -5677,7 +5678,7 @@ describe("ClineProvider", () => { new WebviewFocusTracker(), ) const statesBefore = mockContext.globalState.get("viewStates") - const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(undefined) + const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(true) await provider["setViewStateId"]("") @@ -5705,7 +5706,7 @@ describe("ClineProvider", () => { new WebviewFocusTracker(), ) const statesBefore = mockContext.globalState.get("viewStates") - const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(undefined) + const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(true) // Whitespace normalizes to the empty string, which is the same rejection. await provider["setViewStateId"](" ") @@ -5732,7 +5733,7 @@ describe("ClineProvider", () => { new WebviewFocusTracker(), ) const statesBefore = mockContext.globalState.get("viewStates") - const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(undefined) + const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(true) const loggedBefore = (mockOutputChannel.appendLine as ReturnType).mock.calls.length // The webview generates a UUID; a 65-character value is not a view id and an @@ -5929,7 +5930,7 @@ describe("ClineProvider", () => { new WebviewFocusTracker(), ) const beforeViewStateId = provider["viewStateId"] - const postSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(undefined) + const postSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(true) // 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 } }) @@ -6007,7 +6008,7 @@ describe("ClineProvider", () => { await owner.dispose() const intruder = createProvider() - const postMessageSpy = vi.spyOn(intruder, "postMessageToWebview").mockResolvedValue(undefined) + const postMessageSpy = vi.spyOn(intruder, "postMessageToWebview").mockResolvedValue(true) await intruder["setViewStateId"]("closed-view") @@ -6036,7 +6037,7 @@ describe("ClineProvider", () => { await owner.dispose() const intruder = createProvider() - const postMessageSpy = vi.spyOn(intruder, "postMessageToWebview").mockResolvedValue(undefined) + const postMessageSpy = vi.spyOn(intruder, "postMessageToWebview").mockResolvedValue(true) // A webview-generated value is not a claim: only the token the host minted matches. await intruder["setViewStateId"]("closed-view", "webview-chosen-claim") @@ -6049,7 +6050,11 @@ describe("ClineProvider", () => { }) test("does not churn the claim on later writes to the same entry", async () => { - const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(undefined) + // Claims go to the view's webview: attach the shared double so this exercises delivery + // rather than the no-view path. The post itself is spied below. + provider["view"] = mockWebviewView + + const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(true) await provider["setViewStateId"]("minted-view") await provider.saveViewState("mode", "architect") @@ -6068,12 +6073,17 @@ describe("ClineProvider", () => { test("resolves the persistence write only after the minted claim has been posted", async () => { const gated = createProvider() + // The claim is posted to this view's webview: attach the shared double so the write resolves + // after a real delivery attempt. The post itself is spied below. + gated["view"] = mockWebviewView + let posted = false const postSpy = vi.spyOn(gated, "postMessageToWebview").mockImplementation(async () => { // A real webview post settles on a later turn: a write that reported success before // this resolved would leave the view without the claim it just made durable. await new Promise((resolve) => setTimeout(resolve, 0)) posted = true + return true }) await gated.saveViewState("mode", "architect") @@ -6104,6 +6114,72 @@ describe("ClineProvider", () => { await detached.dispose() }) + test("reports a claim as undelivered when the attached webview rejects the message", async () => { + // A hidden or closing webview resolves postMessage as false: the claim must be reported as + // not delivered and kept for retry, because the durable entry already carries this token. + const rejecting = createProvider() + const hiddenView = { + ...mockWebviewView, + webview: { ...mockWebviewView.webview, postMessage: vi.fn().mockResolvedValue(false) }, + } + rejecting["view"] = hiddenView + const loggedBefore = (mockOutputChannel.appendLine as ReturnType).mock.calls.length + + const delivered = await rejecting["postViewStateClaimToWebview"]("hidden-view", "host-minted-token") + + expect(delivered).toBe(false) + expect(hiddenView.webview.postMessage).toHaveBeenCalledWith({ + type: "viewStateClaim", + viewStateId: "hidden-view", + viewStateClaim: "host-minted-token", + }) + const loggedLines: string[] = (mockOutputChannel.appendLine as ReturnType).mock.calls + .slice(loggedBefore) + .map((call: unknown[]) => String(call[0])) + expect(loggedLines.some((line) => line.includes("[viewStateClaim] Claim for hidden-view"))).toBe(true) + expect(loggedLines.some((line) => line.includes("the webview rejected the message"))).toBe(true) + expect(rejecting["pendingViewStateClaims"].get("hidden-view")).toBe("host-minted-token") + + await rejecting.dispose() + }) + + test("redelivers the queued claim with the same token when a webview attaches", async () => { + // Nothing is attached yet: the claim is durable but undelivered, so it is queued. Once a + // webview attaches it must receive the exact token the entry stores, not a re-minted one. + const detached = createProvider() + expect(detached["view"]).toBeUndefined() + + await detached.saveViewState("mode", "architect") + + const storedClaim = storedEntries()[detached.viewId]?.claimToken + expect(storedClaim).toEqual(expect.any(String)) + expect(detached["pendingViewStateClaims"].get(detached.viewId)).toBe(storedClaim) + + const attachedView = { + ...mockWebviewView, + webview: { ...mockWebviewView.webview, postMessage: vi.fn().mockResolvedValue(true) }, + } + await detached.resolveWebviewView(attachedView) + + const claimPosts = attachedView.webview.postMessage.mock.calls + .map((call: unknown[]) => call[0]) + .filter( + (message: unknown): message is { type: string; viewStateId: string; viewStateClaim: string } => + typeof message === "object" && + message !== null && + (message as { type?: unknown }).type === "viewStateClaim", + ) + expect(claimPosts).toHaveLength(1) + expect(claimPosts[0]).toEqual({ + type: "viewStateClaim", + viewStateId: detached.viewId, + viewStateClaim: storedClaim, + }) + expect(detached["pendingViewStateClaims"].size).toBe(0) + + await detached.dispose() + }) + 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 @@ -6112,7 +6188,11 @@ describe("ClineProvider", () => { "legacy-view": { mode: "architect", currentApiConfigName: "profile-a", updatedAt: 1 }, }) - const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(undefined) + // Claims go to the view's webview: attach the shared double so this exercises delivery + // rather than the no-view path. The post itself is spied below. + provider["view"] = mockWebviewView + + const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(true) await provider["setViewStateId"]("legacy-view") @@ -6225,7 +6305,7 @@ describe("ClineProvider", () => { const victim = createProvider() const victimWriteSpy = vi.spyOn(victim.contextProxy, "setValue") - const postSpy = vi.spyOn(victim, "postMessageToWebview").mockResolvedValue(undefined) + const postSpy = vi.spyOn(victim, "postMessageToWebview").mockResolvedValue(true) const blockedWrite = blocked.saveViewState("mode", "plan") // Queued behind the stalled write: none of the victim's work has started yet. @@ -6299,7 +6379,7 @@ describe("ClineProvider", () => { await writeGate } }) - const postSpy = vi.spyOn(victim, "postMessageToWebview").mockResolvedValue(undefined) + const postSpy = vi.spyOn(victim, "postMessageToWebview").mockResolvedValue(true) const save = victim.saveViewState("mode", "architect") // Let the queued closure reach its storage write, then dispose while it is in @@ -8647,7 +8727,7 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { } as any) const upsertSpy = vi.spyOn(provider, "upsertProviderProfile").mockResolvedValue("profile-id") vi.spyOn(provider, "postStateToWebview").mockResolvedValue(undefined) - const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(undefined) + const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(true) ;(provider as any).providerSettingsManager = { listConfig: vi.fn().mockResolvedValue([]), } @@ -8751,7 +8831,7 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { const { getCachedZooCodeToken } = await import("../../../services/zoo-code-auth") vi.mocked(getCachedZooCodeToken).mockReturnValue("current-token") const handleSpy = vi.spyOn(provider, "handleZooCodeCallback").mockResolvedValue(undefined) - const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(undefined) + const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(true) ;(provider as any).providerSettingsManager = { listConfig: vi diff --git a/src/extension/__tests__/api-start-new-task.spec.ts b/src/extension/__tests__/api-start-new-task.spec.ts index e33f2b3553..7c9de331d2 100644 --- a/src/extension/__tests__/api-start-new-task.spec.ts +++ b/src/extension/__tests__/api-start-new-task.spec.ts @@ -19,7 +19,7 @@ function createProvider(taskId: string) { on: vi.fn(), evictCurrentTask: vi.fn().mockResolvedValue(undefined), postStateToWebview: vi.fn().mockResolvedValue(undefined), - postMessageToWebview: vi.fn().mockResolvedValue(undefined), + postMessageToWebview: vi.fn().mockResolvedValue(true), createTask: vi.fn().mockResolvedValue(task), }) } diff --git a/src/services/language-models/copilotModelBroadcast.ts b/src/services/language-models/copilotModelBroadcast.ts index 000fd6dbdc..c9baf248a0 100644 --- a/src/services/language-models/copilotModelBroadcast.ts +++ b/src/services/language-models/copilotModelBroadcast.ts @@ -10,7 +10,7 @@ export interface CopilotModelBroadcastTarget { type: typeof VsCodeLmModelsMessageType.githubCopilotModels vsCodeLmModels: Awaited> githubCopilotAccount: string | null - }): Promise + }): Promise } export interface CopilotModelBroadcastOptions { From 583ed51f9f2530e74cd12c235c226b529fb8dd38 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 05:25:30 +0800 Subject: [PATCH 16/27] fix(webview): refuse non-portable settings keys posted by the webview updateSettings wrote whatever the webview posted straight into the shared store, including viewStates. The settings-import path already refuses those keys because they are host-minted: an entry name picks which persisted view an instance owns and the claim token inside it proves that ownership, so a posted payload could pick the entry and the identity and walk past the claim check in setViewStateId. Reuse isNonPortableSettingsKey, the same boundary the importer uses: a posted non-portable key is logged and skipped, and the portable keys of the same payload still apply. (cherry picked from commit db5bffa491d7b6aa7f1adc24f5e5e693982f0d32) --- .../__tests__/webviewMessageHandler.spec.ts | 27 +++++++++++++++++++ src/core/webview/webviewMessageHandler.ts | 12 +++++++++ 2 files changed, 39 insertions(+) diff --git a/src/core/webview/__tests__/webviewMessageHandler.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index 685a7b11c1..b6f3123b39 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -3053,3 +3053,30 @@ describe("webviewMessageHandler - renameTaskWithId", () => { expect(vi.mocked(mockClineProvider.renameTaskWithId).mock.calls).toEqual(expectedCalls) }) }) + +describe("webviewMessageHandler - non-portable settings keys", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it("refuses viewStates while applying the portable keys of the same payload", async () => { + // The per-view selections and the claim token that binds them are minted by the host, and the + // settings-file path already refuses them. A posted updateSettings is the same untrusted input, + // so writing them straight into the shared store would let a webview choose which persisted view + // entry - and with which identity - it owns. + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { + viewStates: { "some-view": { claimToken: "webview-minted", mode: "code", updatedAt: 1 } }, + customInstructions: "carry on", + }, + }) + + const writtenKeys = vi.mocked(mockClineProvider.contextProxy.setValue).mock.calls.map((call) => String(call[0])) + expect(writtenKeys).not.toContain("viewStates") + expect(writtenKeys).toContain("customInstructions") + expect(mockClineProvider.log).toHaveBeenCalledWith( + expect.stringContaining("Ignoring non-portable settings key viewStates"), + ) + }) +}) diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 2a04aa413f..2fc515f574 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -40,6 +40,7 @@ import { saveTaskMessages } from "../task-persistence" import { importRooTaskHistory } from "../task-persistence/importRooTaskHistory" import { ClineProvider } from "./ClineProvider" +import { isNonPortableSettingsKey } from "../config/ContextProxy" import { findOriginalContent } from "./stripOriginalContent" import { handleCheckpointRestoreOperation } from "./checkpointRestoreHandler" import { assertRecordedCheckpoint } from "../checkpoints/recordedCheckpoint" @@ -763,6 +764,17 @@ export const webviewMessageHandler = async ( } for (const [key, value] of Object.entries(message.updatedSettings)) { + if (isNonPortableSettingsKey(key)) { + // Per-view selections and the claim token that binds them are minted and held by the host, + // and the settings-file path already refuses them. A posted updateSettings is the same + // untrusted input, so writing them here would let a webview choose which persisted view + // entry - and with which identity - it owns, bypassing the claim check in setViewStateId. + provider.log( + `[updateSettings] Ignoring non-portable settings key ${key} posted by the webview.`, + ) + continue + } + let newValue = value if (key === "language") { From 2f2485eaf5447b190e7e20a98504c38f9d0307ee Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 06:58:58 +0800 Subject: [PATCH 17/27] fix(webview): hold view-state claims until the renderer launches and report undelivered ones deliverPendingViewStateClaims ran inside resolveWebviewView, before webview.html is assigned and before the message listener is registered. postViewStateClaimToWebview deleted a claim as soon as postMessageToWebview resolved true, and the Webview API allows a message posted without an attached listener to be lost - so a claim could be dropped without ever reaching the webview that owns it. Claim posts are now gated on isViewLaunched, which covers the direct calls as well as the retry, and the webviewDidLaunch handler flushes the queue after setting the flag, when a listener exists to receive them. dispose() still drops whatever was never delivered. The persistence callers no longer resolve as if an undelivered claim had reached the webview: savePersistedViewState returns { persisted, claim } and setViewStateId returns { adopted, claim }, with claim in delivered | queued | dropped | none. A durable entry whose claim is still queued is an explicit partial failure the caller can assert on, and a write the queue dropped reports persisted: false rather than a silent success. Control: removing the launch gate fails the two queued-result tests; removing the launch flush fails the launch-delivery tests in both suites; hardcoding the save result to delivered fails five tests. (cherry picked from commit 959fd68cfb08e13df435716c87edfbc7811bb87a) --- src/core/webview/ClineProvider.ts | 162 +++++++++++++----- .../webview/__tests__/ClineProvider.spec.ts | 107 +++++++++++- .../__tests__/webviewMessageHandler.spec.ts | 20 +++ src/core/webview/webviewMessageHandler.ts | 11 ++ 4 files changed, 252 insertions(+), 48 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index a74f8f4db5..519450aa82 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -252,6 +252,29 @@ function removeTaskEventListeners(listeners: WeakMap void>>, t listeners.delete(task) } } +/** + * How a host-minted view-state claim reached the webview that has to carry it back. + * + * - `delivered`: the launched webview accepted the post. + * - `queued`: the durable entry carries the token and the post is held for the next + * `webviewDidLaunch`, when the renderer has a listener that can receive it. + * - `dropped`: the provider was disposed, so nothing will post it again. + * - `none`: this operation minted no claim. + */ +export type ViewStateClaimDelivery = "delivered" | "queued" | "dropped" | "none" + +/** + * Result of a durable view-state write: whether anything was stored, and what happened to + * the claim the write minted. A save that stored an entry whose claim never reached the + * webview is a partial success, and `claim` is how the caller can see it. + */ +export type ViewStatePersistenceResult = { persisted: boolean; claim: ViewStateClaimDelivery } + +/** + * Result of registering a stable view id: whether the id was adopted, and what happened to + * the claim this registration minted or re-affirmed. + */ +export type ViewStateRegistrationResult = { adopted: boolean; claim: ViewStateClaimDelivery } export class ClineProvider extends EventEmitter @@ -724,13 +747,16 @@ export class ClineProvider } /** Persist the non-secret selections through the serialized write queue. */ - private async savePersistedViewState(values: Partial): Promise { + 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 const generation = this.viewStateGeneration // Set by the queued write when it had to mint a claim, delivered after it lands. let mintedClaimToken: string | undefined + // Set once the queued write reaches the storage call: a write the queue dropped before + // its turn must not be reported as persisted. + let storedEntry = false const write = ClineProvider.persistedViewStateWriteQueue.then(async () => { if (!this.isViewStateGenerationLive(generation)) { // The queue advanced after this view was disposed: the entry belongs to a view @@ -783,6 +809,7 @@ export class ClineProvider } await this.contextProxy.setValue("viewStates", this.prunePersistedViewStates(states)) + storedEntry = true if (!this.isViewStateGenerationLive(generation)) { // The view went away while the write was in flight: the selections are durable, @@ -801,12 +828,26 @@ export class ClineProvider ClineProvider.persistedViewStateWriteQueue = write.catch(() => {}) await write + if (!storedEntry) { + // The queue dropped this write before it reached storage: nothing was persisted, so + // nothing about a claim may be reported as settled either. + return { persisted: false, claim: "none" } + } + // 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. // A provider disposed in the meantime has no webview left to deliver it to. if (mintedClaimToken && this.isViewStateGenerationLive(generation)) { - await this.postViewStateClaimToWebview(viewStateId, mintedClaimToken) + return { + persisted: true, + claim: await this.postViewStateClaimToWebview(viewStateId, mintedClaimToken), + } } + + // The entry is durable and carries a token this webview has never been handed. Report + // that explicitly: a save that silently resolves while its claim is undelivered is the + // persistence-integrity gap this result exists to make visible. + return { persisted: true, claim: mintedClaimToken ? "dropped" : "none" } } /** @@ -869,18 +910,34 @@ export class ClineProvider * * Awaited rather than fired and forgotten: the entry already carries this claim once the write * lands, so a view that never receives it would be missing an identity the host believes it - * holds. Every delivery failure is reported - no webview attached, the post rejected, or the - * webview resolving the post as not delivered - and the exact claim is queued for the next - * attach instead of dropped. The post itself stays the single place that records the - * webview-out diagnostic. + * holds. Posts are gated on `isViewLaunched`: the renderer attaches its message listener during + * `webviewDidLaunch`, and the Webview API allows an earlier message to be lost even when + * postMessage resolves true. Every non-delivery is reported - renderer not launched, the post + * rejected, or the webview resolving the post as not delivered - and the exact claim is queued + * for the next `webviewDidLaunch` instead of dropped. The post itself stays the single place + * that records the webview-out diagnostic. */ - private async postViewStateClaimToWebview(viewStateId: string, claimToken: string): Promise { - if (!this.view || this._disposed) { + private async postViewStateClaimToWebview( + viewStateId: string, + claimToken: string, + ): Promise { + if (this._disposed) { + this.log( + `[viewStateClaim] Claim for ${viewStateId} dropped for view ${this.viewId}: the provider is disposed.`, + ) + return "dropped" + } + + if (!this.view || !this.isViewLaunched) { + // Held rather than posted. The renderer only attaches its message listener once the + // webview launches, and the Webview API permits a message posted before that to be lost + // while postMessage still resolves true - which would delete a claim the webview never + // saw. Queuing keeps the exact token for the webviewDidLaunch flush. this.pendingViewStateClaims.set(viewStateId, claimToken) this.log( - `[viewStateClaim] Claim for ${viewStateId} not delivered to view ${this.viewId}: no webview attached.`, + `[viewStateClaim] Claim for ${viewStateId} not delivered to view ${this.viewId}: the webview has not launched yet.`, ) - return false + return "queued" } let delivered: boolean @@ -894,7 +951,7 @@ export class ClineProvider const reason = error instanceof Error ? error.message : String(error) this.pendingViewStateClaims.set(viewStateId, claimToken) this.log(`[viewStateClaim] Delivery for ${viewStateId} to view ${this.viewId} failed: ${reason}`) - return false + return "queued" } if (!delivered) { @@ -904,21 +961,22 @@ export class ClineProvider this.log( `[viewStateClaim] Claim for ${viewStateId} not delivered to view ${this.viewId}: the webview rejected the message.`, ) - return false + return "queued" } this.pendingViewStateClaims.delete(viewStateId) - return true + return "delivered" } /** - * Re-delivers the claims this view minted that no webview accepted yet, called when a webview - * attaches. Each queued entry keeps the exact token the durable entry stores, so a webview that - * missed a delivery still gets the identity it has to present on the next launch; an entry that - * fails again is re-queued for the next attach. + * Flushes the claims this view minted that no webview accepted yet. Called from the + * `webviewDidLaunch` handler, once the renderer has attached its message listener, and a no-op + * until then: posting before that can be lost. Each queued entry keeps the exact token the + * durable entry stores, so a webview that missed a delivery still gets the identity it has to + * present on the next launch; an entry that fails again is re-queued for the next launch. */ - private async deliverPendingViewStateClaims(): Promise { - if (this._disposed || !this.view || this.pendingViewStateClaims.size === 0) { + public async deliverPendingViewStateClaims(): Promise { + if (this._disposed || !this.view || !this.isViewLaunched || this.pendingViewStateClaims.size === 0) { return } @@ -1003,7 +1061,10 @@ export class ClineProvider } /** Register the stable view id and load its persisted selections; a claim must match. */ - public async setViewStateId(viewStateId: string | undefined, presentedClaim?: string): Promise { + 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 @@ -1012,7 +1073,7 @@ export class ClineProvider // 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 + return { adopted: false, claim: "none" } } const normalizedViewStateId = viewStateId.trim().replace(/[^A-Za-z0-9_-]/g, "_") @@ -1032,7 +1093,7 @@ export class ClineProvider // entry under a name that looks like an inherited member. normalizedViewStateId === "__proto__" ) { - return + return { adopted: false, claim: "none" } } // Bound the key: the webview generates a UUID, so a much longer value is not a @@ -1041,7 +1102,7 @@ export class ClineProvider this.log( `[setViewStateId] Ignoring an oversized viewStateId (${normalizedViewStateId.length} chars) for view ${this.viewId}.`, ) - return + return { adopted: false, claim: "none" } } const claimant = ClineProvider.getAllInstances().find( @@ -1055,7 +1116,7 @@ export class ClineProvider this.log( `[setViewStateId] Refusing viewStateId ${normalizedViewStateId} for view ${this.viewId}: already claimed by live view ${claimant.viewId}.`, ) - return + return { adopted: false, claim: "none" } } // Claim binding, checked before any re-key or load so a refused registration can @@ -1063,6 +1124,11 @@ export class ClineProvider // through the ContextProxy cache: a security check must not trust a stale copy. const persistedEntry = this.getPersistedViewStates({ fresh: true })[normalizedViewStateId] + // What became of the claim this registration mints or re-affirms. A refused registration + // never reaches a claim, and an adopted one whose claim is still queued is not the same + // outcome as one the webview received, so both are reported to the caller. + let claimDelivery: ViewStateClaimDelivery = "none" + if (persistedEntry) { const storedClaim = persistedEntry.claimToken @@ -1080,10 +1146,10 @@ export class ClineProvider this.log( `[setViewStateId] Refusing viewStateId ${normalizedViewStateId} for view ${this.viewId}: bound concurrently by another registration.`, ) - return + return { adopted: false, claim: "none" } } - await this.postViewStateClaimToWebview(normalizedViewStateId, reboundClaim) + claimDelivery = await this.postViewStateClaimToWebview(normalizedViewStateId, reboundClaim) this.log( `[setViewStateId] Re-bound claim-less viewStates entry ${normalizedViewStateId} for view ${this.viewId}: unbound selections were not loaded.`, ) @@ -1094,7 +1160,7 @@ export class ClineProvider this.log( `[setViewStateId] Refusing viewStateId ${normalizedViewStateId} for view ${this.viewId}: claim missing or mismatched.`, ) - return + return { adopted: false, claim: "none" } } } @@ -1116,7 +1182,11 @@ export class ClineProvider // A registration whose view was disposed mid-flight must not deliver a claim to // a webview that is no longer there. if (adoptedClaim && this.isViewStateGenerationLive(generation)) { - await this.postViewStateClaimToWebview(this.viewStateId, adoptedClaim) + claimDelivery = await this.postViewStateClaimToWebview(this.viewStateId, adoptedClaim) + } else if (adoptedClaim) { + // The view was disposed mid-registration: the binding is durable but this provider + // will not post the claim again. + claimDelivery = "dropped" } // Registration is what loads this view's own persisted selections. @@ -1129,6 +1199,8 @@ export class ClineProvider this.viewStateId = previousViewStateId throw error } + + return { adopted: true, claim: claimDelivery } } /** @@ -1732,17 +1804,10 @@ export class ClineProvider this.webviewDisposables, ) - // A claim minted while no webview was attached, or while its post was rejected, is delivered - // now that this view exists: the durable entry already carries that token, and the webview - // cannot present an identity the host never handed it. A failed retry must not fail - // initialization, so it is contained here. - try { - await this.deliverPendingViewStateClaims() - } catch (error) { - this.log( - `[viewStateClaim] Retrying pending claims for view ${this.viewId} failed: ${error instanceof Error ? error.message : String(error)}`, - ) - } + // Queued claims are deliberately not posted here. The renderer attaches its message listener + // only when it launches, and the Webview API allows a message posted before that to be lost + // while postMessage still resolves true. The webviewDidLaunch handler flushes the queue once + // a listener exists to receive it. if (inTabMode) { setPanel(webviewView, "tab") @@ -6030,10 +6095,12 @@ export class ClineProvider public async saveViewState( key: K, value: PersistedViewStateValues[K] | undefined, - ): Promise { - await this._saveViewLocalStateFromMutation({ [key]: value } as ViewLocalStateValues) + ): Promise { + const persistence = await this._saveViewLocalStateFromMutation({ [key]: value } as ViewLocalStateValues) this.log(`[saveViewState] Saved ${String(key)} for viewId ${this.viewId}`) + + return persistence } /** @@ -6043,9 +6110,11 @@ export class ClineProvider */ private async _saveViewLocalStateFromMutation( values: Partial & Partial, - ): Promise { - await this._persistViewLocalStateFromMutation(values) + ): Promise { + const persistence = await this._persistViewLocalStateFromMutation(values) this._updateViewLocalStateFromMutation(values) + + return persistence } /** @@ -6111,7 +6180,7 @@ export class ClineProvider */ private async _persistViewLocalStateFromMutation( values: Partial & Partial, - ): Promise { + ): Promise { const persistedValues: Partial = {} if ("mode" in values) { @@ -6123,8 +6192,11 @@ export class ClineProvider } if ("mode" in persistedValues || "currentApiConfigName" in persistedValues) { - await this.savePersistedViewState(persistedValues) + return this.savePersistedViewState(persistedValues) } + + // Nothing in this mutation belongs to the durable record, so there was nothing to write. + return { persisted: false, claim: "none" } } /** Load non-secret persisted selections; in-flight buffer edits are reapplied on top. */ diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 736fb78590..fb158fccfa 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -6053,6 +6053,8 @@ describe("ClineProvider", () => { // Claims go to the view's webview: attach the shared double so this exercises delivery // rather than the no-view path. The post itself is spied below. provider["view"] = mockWebviewView + // The webview has launched and attached its listener: claims are posted live from here on. + provider.isViewLaunched = true const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(true) await provider["setViewStateId"]("minted-view") @@ -6076,6 +6078,8 @@ describe("ClineProvider", () => { // The claim is posted to this view's webview: attach the shared double so the write resolves // after a real delivery attempt. The post itself is spied below. gated["view"] = mockWebviewView + // A launched webview: the claim post this write performs can actually be received. + gated.isViewLaunched = true let posted = false const postSpy = vi.spyOn(gated, "postMessageToWebview").mockImplementation(async () => { @@ -6123,11 +6127,13 @@ describe("ClineProvider", () => { webview: { ...mockWebviewView.webview, postMessage: vi.fn().mockResolvedValue(false) }, } rejecting["view"] = hiddenView + // Launched but closing: the post is attempted and the webview refuses it. + rejecting.isViewLaunched = true const loggedBefore = (mockOutputChannel.appendLine as ReturnType).mock.calls.length const delivered = await rejecting["postViewStateClaimToWebview"]("hidden-view", "host-minted-token") - expect(delivered).toBe(false) + expect(delivered).toBe("queued") expect(hiddenView.webview.postMessage).toHaveBeenCalledWith({ type: "viewStateClaim", viewStateId: "hidden-view", @@ -6143,7 +6149,7 @@ describe("ClineProvider", () => { await rejecting.dispose() }) - test("redelivers the queued claim with the same token when a webview attaches", async () => { + test("redelivers the queued claim with the same token when the webview launches", async () => { // Nothing is attached yet: the claim is durable but undelivered, so it is queued. Once a // webview attaches it must receive the exact token the entry stores, not a re-minted one. const detached = createProvider() @@ -6161,6 +6167,12 @@ describe("ClineProvider", () => { } await detached.resolveWebviewView(attachedView) + // Attaching the view is not the delivery point: the renderer only has a message listener + // once it launches, so the held claim goes out from the webviewDidLaunch handler. + const launchedMessageHandler = (attachedView.webview.onDidReceiveMessage as ReturnType).mock + .calls[0][0] + await launchedMessageHandler({ type: "webviewDidLaunch" }) + const claimPosts = attachedView.webview.postMessage.mock.calls .map((call: unknown[]) => call[0]) .filter( @@ -6180,6 +6192,89 @@ describe("ClineProvider", () => { await detached.dispose() }) + test("reports a queued claim on the save result when the webview has not launched", async () => { + // The persistence-integrity gap this result exists to close: the durable entry carries a + // claim the webview has never been handed. A save that resolves as a complete success + // hides an identity the next launch still has to deliver. + const detached = createProvider() + detached["view"] = mockWebviewView + const postMessageSpy = vi.spyOn(detached, "postMessageToWebview").mockResolvedValue(true) + + const result = await detached.saveViewState("mode", "architect") + + expect(result).toEqual({ persisted: true, claim: "queued" }) + expect(storedEntries()[detached.viewId]?.claimToken).toEqual(expect.any(String)) + expect(detached["pendingViewStateClaims"].get(detached.viewId)).toBe( + storedEntries()[detached.viewId]?.claimToken, + ) + // Nothing was posted: the renderer has no listener yet, and a post that resolved true + // here would delete the claim the launch flush exists to deliver. + expect(postMessageSpy).not.toHaveBeenCalled() + + postMessageSpy.mockRestore() + await detached.dispose() + }) + + test("reports a delivered claim on the save result once the webview has launched", async () => { + const launched = createProvider() + launched["view"] = mockWebviewView + launched.isViewLaunched = true + const postMessageSpy = vi.spyOn(launched, "postMessageToWebview").mockResolvedValue(true) + + const result = await launched.saveViewState("mode", "architect") + + expect(result).toEqual({ persisted: true, claim: "delivered" }) + expect(launched["pendingViewStateClaims"].size).toBe(0) + + postMessageSpy.mockRestore() + await launched.dispose() + }) + + test("reports a queued claim on the registration result of an unlaunched view", async () => { + // Re-keying a pre-launch selection onto its stable id re-affirms that entry's claim. Until + // the renderer launches the reaffirmation is held, and the registration result has to say + // that rather than report an adoption whose identity never reached the webview. + const minted = createProvider() + minted["view"] = mockWebviewView + const postMessageSpy = vi.spyOn(minted, "postMessageToWebview").mockResolvedValue(true) + await minted.saveViewState("mode", "architect") + const mintedClaim = storedEntries()[minted.viewId]?.claimToken + + const registered = await minted["setViewStateId"]("rekeyed-view") + + expect(registered).toEqual({ adopted: true, claim: "queued" }) + expect(storedEntries()["rekeyed-view"]?.claimToken).toBe(mintedClaim) + expect(postMessageSpy).not.toHaveBeenCalledWith( + expect.objectContaining({ type: "viewStateClaim", viewStateId: "rekeyed-view" }), + ) + + postMessageSpy.mockRestore() + await minted.dispose() + }) + + test("reports a delivered claim on the registration result of a launched view", async () => { + const launched = createProvider() + launched["view"] = mockWebviewView + launched.isViewLaunched = true + const postMessageSpy = vi.spyOn(launched, "postMessageToWebview").mockResolvedValue(true) + await launched.saveViewState("mode", "architect") + const mintedClaim = storedEntries()[launched.viewId]?.claimToken + postMessageSpy.mockClear() + + const registered = await launched["setViewStateId"]("launched-view") + + expect(registered).toEqual({ adopted: true, claim: "delivered" }) + expect(storedEntries()["launched-view"]?.claimToken).toBe(mintedClaim) + expect( + postMessageSpy.mock.calls + .map(([message]) => message) + .filter((message) => message.type === "viewStateClaim"), + ).toEqual([{ type: "viewStateClaim", viewStateId: "launched-view", viewStateClaim: mintedClaim }]) + + postMessageSpy.mockRestore() + await launched.dispose() + }) + 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 @@ -6191,6 +6286,8 @@ describe("ClineProvider", () => { // Claims go to the view's webview: attach the shared double so this exercises delivery // rather than the no-view path. The post itself is spied below. provider["view"] = mockWebviewView + // The re-bind hands the fresh claim to a launched webview. + provider.isViewLaunched = true const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(true) @@ -6390,7 +6487,7 @@ describe("ClineProvider", () => { await victim.dispose() releaseWrite() - await save + const saveResult = await save expect(setValueSpy.mock.calls.some(([key]) => key === "viewStates")).toBe(true) // The write itself was allowed to land; nothing applied after it. @@ -6399,6 +6496,10 @@ describe("ClineProvider", () => { postSpy.mock.calls.map(([message]) => message).filter((message) => message.type === "viewStateClaim"), ).toHaveLength(0) + // The entry is durable and carries a token this provider will never post: the save is a + // partial success, and its result says so instead of resolving like a completed write. + expect(saveResult).toEqual({ persisted: true, claim: "dropped" }) + setValueSpy.mockRestore() postSpy.mockRestore() }) diff --git a/src/core/webview/__tests__/webviewMessageHandler.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index b6f3123b39..8259531f20 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -128,6 +128,8 @@ const mockClineProvider = { }, log: vi.fn(), postStateToWebview: vi.fn(), + // The webviewDidLaunch handler flushes claims the provider queued before the renderer launched. + deliverPendingViewStateClaims: vi.fn().mockResolvedValue(undefined), resolveWebviewThemeFixtureProbe: vi.fn(), getCurrentTask: vi.fn(), getTaskWithId: vi.fn(), @@ -2803,6 +2805,24 @@ describe("webviewMessageHandler - telemetrySetting", () => { expect(calls.at(-1)).toEqual([true]) }) + // Review follow-up on the attach-time claim retry: a claim minted before the renderer launched is + // held by the provider, because a message posted before the webview attaches its listener can be + // lost while postMessage still resolves true. The flush therefore belongs to webviewDidLaunch, and + // it has to run only after isViewLaunched is set - flushing earlier would post back into the window + // the gate exists to close. + it("flushes the provider's pending view-state claims on webviewDidLaunch, after isViewLaunched is set", async () => { + vi.mocked(mockClineProvider.customModesManager.getCustomModes).mockResolvedValue([]) + let launchedWhenFlushed: boolean | undefined + vi.mocked(mockClineProvider.deliverPendingViewStateClaims).mockImplementationOnce(async () => { + launchedWhenFlushed = mockClineProvider.isViewLaunched + }) + + await webviewMessageHandler(mockClineProvider, { type: "webviewDidLaunch" }) + + expect(mockClineProvider.deliverPendingViewStateClaims).toHaveBeenCalledTimes(1) + expect(launchedWhenFlushed).toBe(true) + }) + // CodeRabbit follow-up on the finding #12 fix: webviewDidLaunch's telemetry init read state // via an async provider.getStateToPostToWebview().then(...) continuation, outside // telemetrySettingQueue -- so it could resolve after a concurrent "telemetrySetting" message diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 2fc515f574..585f45f598 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -701,6 +701,17 @@ export const webviewMessageHandler = async ( ) provider.isViewLaunched = true + // Claims minted before this launch were held by the provider, because posting one earlier + // could be lost while postMessage still resolved true. The listener exists now, so this is + // the point where the queued posts can actually be received. A failed flush must not fail + // the launch, so it is contained here. + try { + await provider.deliverPendingViewStateClaims() + } catch (error) { + provider.log( + `[viewStateClaim] Flushing pending claims on launch failed: ${error instanceof Error ? error.message : String(error)}`, + ) + } break case "webviewHeartbeat": // Confirm renderer liveness and advance the watchdog recovery health window. From 6b7602702c0a4bc932879522a1ad7e4cbb92a3db Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 08:58:14 +0800 Subject: [PATCH 18/27] fix(webview): reset isViewLaunched when the renderer is replaced isViewLaunched was written twice in production: initialised false and set true by the webviewDidLaunch handler. Nothing lowered it. Replacing the view (clearWebviewResources runs for a new view, a disposed sidebar view and clearAllTasks) and replacing the renderer in place (reloadWebviewForRecovery assigns new HTML) both discard the document that held the message listener, yet the provider kept believing a post could be received. A durable view-state mutation in that window took the post branch of postViewStateClaimToWebview instead of queueing, and a post that resolves true against a page with no listener deletes the claim - leaving a durable entry whose claim no renderer ever received, so the next launch could not adopt this view's state. clearWebviewResources now lowers the flag for every path that drops the view, and reloadWebviewForRecovery lowers it before assigning new HTML. The gate itself is unchanged: with the flag down the queue holds the exact token, and the replacement renderer's own webviewDidLaunch re-arms the flag and flushes it. Control (CTRL-2226D): with both resets removed the two new tests fail at the isViewLaunched expectation immediately after the replacement - the replacement-view save reports claim 'delivered' and posts into an unlaunched renderer, and the recovery case keeps the flag true after the HTML swap. The unpatched file was restored byte-identically and core/webview (34 files, 900 tests) and the launched-flag consumers (activate, extension, api, diagnostics: 28 files, 300 tests) pass on the fixed tree; eslint --max-warnings=0 on both touched files and src check-types are clean, and src/eslint-suppressions.json is untouched. --- src/core/webview/ClineProvider.ts | 9 ++ .../webview/__tests__/ClineProvider.spec.ts | 126 ++++++++++++++++++ 2 files changed, 135 insertions(+) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 519450aa82..503f896512 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -1509,6 +1509,11 @@ export class ClineProvider private clearWebviewResources() { this.rejectPendingThemeFixtureProbes(new Error("Webview was disposed before the theme fixture probe completed")) this.stopWebviewWatchdog() + // The renderer behind this view is gone or about to be replaced, and its message + // listener dies with it: a post can still resolve true while nobody is there to receive + // it. The flag goes down here so a claim minted before the replacement launches is held + // in the queue and flushed by that replacement's own `webviewDidLaunch`. + this.isViewLaunched = false // Invalidate any recovery reload still awaiting its HTML so it cannot // reassign webview.html on the disposed view. this.webviewRecoveryEpoch++ @@ -4556,6 +4561,10 @@ export class ClineProvider ) { return } + // Assigning new HTML discards the live document and its message listener, so the launched + // flag goes down with it; the regenerated renderer re-arms it from its own + // `webviewDidLaunch`, and any claim queued in between reaches it then. + this.isViewLaunched = false view.webview.html = html this.resetWebviewHeartbeatGrace() const revision = this.webviewHeartbeatRevision diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index fb158fccfa..fc915addc6 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -6192,6 +6192,132 @@ describe("ClineProvider", () => { await detached.dispose() }) + test("holds a claim minted after a renderer replacement and delivers it on the next launch", async () => { + // The interleaving the queue exists for: replacing the view drops the renderer's message + // listener, a durable view-state mutation mints a claim while the replacement renderer has + // no listener yet, and a post that resolves true against the page being replaced deletes a + // claim that renderer never received, so its launch cannot adopt this view's state. + const replaced = createProvider() + const firstView = { + ...mockWebviewView, + webview: { + ...mockWebviewView.webview, + onDidReceiveMessage: vi.fn(), + postMessage: vi.fn().mockResolvedValue(true), + }, + } + await replaced.resolveWebviewView(firstView) + const firstLaunch = (firstView.webview.onDidReceiveMessage as ReturnType).mock.calls[0][0] + await firstLaunch({ type: "webviewDidLaunch" }) + expect(replaced.isViewLaunched).toBe(true) + + // A different view replaces the first one. The replacement renderer has not sent + // `webviewDidLaunch`, so nothing listens yet, yet its postMessage still resolves true - + // exactly the webview that swallows a claim the host believes was delivered. + const secondView = { + ...mockWebviewView, + webview: { + ...mockWebviewView.webview, + onDidReceiveMessage: vi.fn(), + postMessage: vi.fn().mockResolvedValue(true), + }, + } + await replaced.resolveWebviewView(secondView) + expect(replaced.isViewLaunched).toBe(false) + + const result = await replaced.saveViewState("mode", "architect") + const mintedClaim = storedEntries()[replaced.viewId]?.claimToken + + // Held rather than posted: the entry keeps the token it just minted and the queue holds + // that same token for the launch that can actually receive it. + expect(result).toEqual({ persisted: true, claim: "queued" }) + expect(mintedClaim).toEqual(expect.any(String)) + expect(replaced["pendingViewStateClaims"].get(replaced.viewId)).toBe(mintedClaim) + expect( + secondView.webview.postMessage.mock.calls + .map((call: unknown[]) => call[0]) + .filter( + (message: unknown): message is { type: string; viewStateId: string; viewStateClaim: string } => + typeof message === "object" && + message !== null && + (message as { type?: unknown }).type === "viewStateClaim", + ), + ).toEqual([]) + + // The replacement renderer launches: the claim it never received arrives carrying the token + // the record already stores, which is what lets that launch adopt this view's state. + const secondLaunch = (secondView.webview.onDidReceiveMessage as ReturnType).mock.calls[0][0] + await secondLaunch({ type: "webviewDidLaunch" }) + + const claimPosts = secondView.webview.postMessage.mock.calls + .map((call: unknown[]) => call[0]) + .filter( + (message: unknown): message is { type: string; viewStateId: string; viewStateClaim: string } => + typeof message === "object" && + message !== null && + (message as { type?: unknown }).type === "viewStateClaim", + ) + expect(claimPosts).toEqual([ + { type: "viewStateClaim", viewStateId: replaced.viewId, viewStateClaim: mintedClaim }, + ]) + expect(replaced["pendingViewStateClaims"].size).toBe(0) + + await replaced.dispose() + }) + + test("lowers the launched flag when recovery replaces the renderer's HTML", async () => { + // Assigning new HTML discards the live document, listener included. A provider that still + // believes the renderer is launched posts claims into a page that is being replaced. + // Recovery schedules its own confirmation timer; fake timers keep it off the run so it + // cannot log after the environment is gone. + vi.useFakeTimers() + const recovered = createProvider() + recovered["view"] = mockWebviewView + recovered.isViewLaunched = true + recovered["getWebviewHtml"] = vi.fn().mockResolvedValue("recovered-html") + const postMessageSpy = vi.spyOn(recovered, "postMessageToWebview").mockResolvedValue(true) + + await recovered["reloadWebviewForRecovery"]() + + expect(recovered.isViewLaunched).toBe(false) + + // A claim minted after the swap is held for the regenerated renderer's own launch instead + // of being posted into a page whose listener is already gone. + const result = await recovered.saveViewState("mode", "architect") + const storedClaim = storedEntries()[recovered.viewId]?.claimToken + expect(result).toEqual({ persisted: true, claim: "queued" }) + expect(storedClaim).toEqual(expect.any(String)) + expect(recovered["pendingViewStateClaims"].get(recovered.viewId)).toBe(storedClaim) + expect( + postMessageSpy.mock.calls + .map((call: unknown[]) => call[0]) + .filter( + (message: unknown): message is { type: string; viewStateId: string; viewStateClaim: string } => + typeof message === "object" && + message !== null && + (message as { type?: unknown }).type === "viewStateClaim", + ), + ).toEqual([]) + + // The regenerated renderer reports its own launch and the held claim reaches it. + recovered.isViewLaunched = true + await recovered.deliverPendingViewStateClaims() + expect( + postMessageSpy.mock.calls + .map((call: unknown[]) => call[0]) + .filter( + (message: unknown): message is { type: string; viewStateId: string; viewStateClaim: string } => + typeof message === "object" && + message !== null && + (message as { type?: unknown }).type === "viewStateClaim", + ), + ).toEqual([{ type: "viewStateClaim", viewStateId: recovered.viewId, viewStateClaim: storedClaim }]) + + recovered["clearWebviewRecoveryConfirmation"]() + postMessageSpy.mockRestore() + await recovered.dispose() + vi.useRealTimers() + }) test("reports a queued claim on the save result when the webview has not launched", async () => { // The persistence-integrity gap this result exists to close: the durable entry carries a // claim the webview has never been handed. A save that resolves as a complete success From 8b863f8bef9b4be876aa67a50616f899b0b416f0 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 09:32:09 +0800 Subject: [PATCH 19/27] fix(webview): close the claim-boundary and persistence gaps from review CodeRabbit's pre-merge panel re-derived on 6b7602702 with two errors and two accepted warnings, all on machinery this unit introduced: - Security Boundaries: a newTask taskConfiguration reached createTask -> setValues and could write the host-only viewStates key. The filter now lives in ContextProxy.setValues, the setter every webview-, API- and import-originated configuration path funnels through, so it covers callers the reported path is not limited to. Host view-state methods keep writing via setValue on the key. - Persistence Integrity: a durable claim-bearing write whose provider disposed during the storage await left an entry carrying a token no renderer had been handed and no later provider could adopt (a registration whose presented claim does not match is refused side-effect free). The write is now undone: the key returns to its pre-write value, and only while the entry still carries the token this write minted, so a concurrent binding survives. Chosen over retry state that survives disposal because a claim is per-renderer identity: durable retry state would still be waiting for a webview that never received the token. - Lifecycle Resource Cleanup: the static write queue chained provider closures, so a provider disposed behind another view's write stayed retained. The queue now holds entries; dispose() detaches its own and clears the back-pointer. - Regression Evidence: focused coverage for saveViewState of mode and of currentApiConfigName with undefined, including deleting the record when the last persisted field goes. Controls (each reverting one half of the fix, then restored byte-identically): - CTRL-2226E: guard removed -> the newTask test fails at expected { claimToken: 'stolen', ... } to match { claimToken: 'host-token', ... }. - CTRL-2226F: rollback block removed -> the fresh-provider test fails at expected { persisted: true, claim: 'dropped' } to equal { persisted: false, claim: 'none' }, and the concurrent-rebind test stays green. - CTRL-2226G: detach loop removed -> the detachment test fails at expected [ { ... } ] to not include { ... }. Gates: eslint --max-warnings=0 on all three files, prettier --check, src tsc --noEmit exit 0, and vitest run core/webview activate --no-file-parallelism -> 39 files / 1000 tests. src/eslint-suppressions.json is untouched. --- src/core/config/ContextProxy.ts | 9 +- src/core/webview/ClineProvider.ts | 165 ++++++++--- .../webview/__tests__/ClineProvider.spec.ts | 266 +++++++++++++++++- 3 files changed, 393 insertions(+), 47 deletions(-) diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index bcc9557113..57b4dffaaf 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -569,7 +569,14 @@ export class ContextProxy { } public async setValues(values: RooCodeSettings) { - const entries = Object.entries(values) as [RooCodeSettingsKey, unknown][] + // viewStates is host-owned: per-view selections and the claim token that binds them are + // minted by the extension host, so no configuration path may carry them. Every caller of + // this setter takes input that did not originate inside the host - a posted updateSettings, + // a newTask taskConfiguration, the external API, an imported settings file - so the filter + // lives here rather than at whichever caller a reviewer happened to point at. The host's + // own view-state methods write through setValue("viewStates", ...), which stays open. + const portableEntries = Object.entries(values).filter(([key]) => !isNonPortableSettingsKey(key)) + const entries = portableEntries as [RooCodeSettingsKey, unknown][] await Promise.all(entries.map(([key, value]) => this.setValue(key, value))) } diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 503f896512..ed8426d5c0 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -276,6 +276,18 @@ export type ViewStatePersistenceResult = { persisted: boolean; claim: ViewStateC */ export type ViewStateRegistrationResult = { adopted: boolean; claim: ViewStateClaimDelivery } +/** + * One serialized view-state write. The queue holds entries like this instead of closures over a + * provider: `dispose()` clears `provider`, so a write still waiting behind another view's write + * releases the provider that scheduled it rather than retaining it, and its turn is skipped once + * it has been detached. + */ +type ViewStateWriteEntry = { + provider: ClineProvider | null + readonly generation: number + readonly run: (provider: ClineProvider) => Promise +} + export class ClineProvider extends EventEmitter implements vscode.WebviewViewProvider, TelemetryPropertiesProvider, TaskProviderLike @@ -289,7 +301,9 @@ export class ClineProvider 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() + /** Queued view-state writes, held as entries rather than as closures over a provider. */ + private static pendingViewStateWrites: ViewStateWriteEntry[] = [] + private static viewStateWritesPump: Promise = Promise.resolve() private webviewDisposables: vscode.Disposable[] = [] private pendingThemeFixtureProbes = new Map< string, @@ -490,6 +504,9 @@ export class ClineProvider * longer matches, instead of applying them to a disposed provider and holding it * retained until the queue advances. */ + /** This provider's entries on the shared write queue; detached by `dispose()`. */ + private scheduledViewStateWrites: ViewStateWriteEntry[] = [] + private viewStateGeneration = 0 /** @@ -746,6 +763,37 @@ export class ClineProvider return !this._disposed && this.viewStateGeneration === generation } + /** + * Serialize one provider's view-state write behind every write already on the queue. The queue + * holds entries rather than closures over a provider, so `dispose()` can detach a write that is + * still waiting: its turn is skipped when the queue reaches it, and the provider is released + * instead of retained behind another view's write. + */ + private static enqueuePersistedViewStateWrite(entry: ViewStateWriteEntry): Promise { + ClineProvider.pendingViewStateWrites.push(entry) + entry.provider?.scheduledViewStateWrites.push(entry) + + const settled = ClineProvider.viewStateWritesPump.then(async () => { + const index = ClineProvider.pendingViewStateWrites.indexOf(entry) + + if (index === -1) { + // Detached by dispose() while it waited: the view is gone, so its turn is skipped. + return + } + + ClineProvider.pendingViewStateWrites.splice(index, 1) + + if (entry.provider) { + await entry.run(entry.provider) + } + }) + + // The queue advances even when this write rejects; the caller still sees the rejection. + ClineProvider.viewStateWritesPump = settled.catch(() => {}) + + return settled + } + /** 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 @@ -757,20 +805,27 @@ export class ClineProvider // Set once the queued write reaches the storage call: a write the queue dropped before // its turn must not be reported as persisted. let storedEntry = false - const write = ClineProvider.persistedViewStateWriteQueue.then(async () => { - if (!this.isViewStateGenerationLive(generation)) { + // Set when the durable write is undone because the view died before its claim could reach a + // renderer. + let rolledBackEntry = false + const run = async (provider: ClineProvider): Promise => { + if (!provider.isViewStateGenerationLive(generation)) { // The queue advanced after this view was disposed: the entry belongs to a view // that no longer exists, so the write is dropped instead of applied. return } - const states = this.getPersistedViewStates({ fresh: true }) + const states = provider.getPersistedViewStates({ fresh: true }) + // Snapshot what this key held before the write, so a write interrupted by disposal can be + // restored to exactly that value. + const hadPreviousEntry = Object.prototype.hasOwnProperty.call(states, viewStateId) + const previousEntry = states[viewStateId] // 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 ownsTargetEntry = viewStateId !== provider.viewId || provider.wroteUnderTemporaryViewStateId const current = ownsTargetEntry ? (states[viewStateId] ?? {}) : {} const next: PersistedViewState = { ...current } @@ -808,24 +863,43 @@ export class ClineProvider wroteEntry = true } - await this.contextProxy.setValue("viewStates", this.prunePersistedViewStates(states)) + await provider.contextProxy.setValue("viewStates", provider.prunePersistedViewStates(states)) storedEntry = true - if (!this.isViewStateGenerationLive(generation)) { - // The view went away while the write was in flight: the selections are durable, - // but no provider field may be touched afterwards. + if (!provider.isViewStateGenerationLive(generation)) { + // The view went away while the write was in flight: no provider field may be touched + // afterwards. If this write is the one that minted the entry's claim, no renderer was ever + // handed that token and the only provider that could post it is gone, so the durable + // entry would carry a binding nobody can ever present. Undo the write instead of leaving + // it: the record goes back to exactly what it held before this write. + if (mintedClaimToken) { + const after = provider.getPersistedViewStates({ fresh: true }) + // Undo only this write's binding: a later provider that already re-bound the key + // stored its own token, and that write must survive the rollback. + if (after[viewStateId]?.claimToken === mintedClaimToken) { + if (hadPreviousEntry && previousEntry) { + after[viewStateId] = previousEntry + } else { + delete after[viewStateId] + } + + await provider.contextProxy.setValue("viewStates", provider.prunePersistedViewStates(after)) + rolledBackEntry = true + } + } + return } // 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 + if (wroteEntry && viewStateId === provider.viewId) { + provider.wroteUnderTemporaryViewStateId = true } - }) + } - ClineProvider.persistedViewStateWriteQueue = write.catch(() => {}) + const write = ClineProvider.enqueuePersistedViewStateWrite({ provider: this, generation, run }) await write if (!storedEntry) { @@ -834,6 +908,12 @@ export class ClineProvider return { persisted: false, claim: "none" } } + if (rolledBackEntry) { + // The durable write was undone: the claim this write minted could never reach a renderer, + // so nothing about it may be reported as durable either. + return { persisted: false, claim: "none" } + } + // 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. // A provider disposed in the meantime has no webview left to deliver it to. @@ -856,20 +936,19 @@ export class ClineProvider */ private async clearPersistedViewState(viewStateId = this.viewStateId): Promise { const generation = this.viewStateGeneration - const write = ClineProvider.persistedViewStateWriteQueue.then(async () => { - if (!this.isViewStateGenerationLive(generation)) { + const run = async (provider: ClineProvider): Promise => { + if (!provider.isViewStateGenerationLive(generation)) { // A clear that only now reaches the queue head belongs to a disposed view: // its entry is already unreferenced by this provider, so leave the map alone. return } - const states = this.getPersistedViewStates({ fresh: true }) + const states = provider.getPersistedViewStates({ fresh: true }) delete states[viewStateId] - await this.contextProxy.setValue("viewStates", states) - }) + await provider.contextProxy.setValue("viewStates", states) + } - ClineProvider.persistedViewStateWriteQueue = write.catch(() => {}) - await write + await ClineProvider.enqueuePersistedViewStateWrite({ provider: this, generation, run }) } /** Re-bind a claim-less entry with a fresh claim; unowned selections are dropped. */ @@ -878,14 +957,14 @@ export class ClineProvider let rebound = false const generation = this.viewStateGeneration - const write = ClineProvider.persistedViewStateWriteQueue.then(async () => { - if (!this.isViewStateGenerationLive(generation)) { + const run = async (provider: ClineProvider): Promise => { + if (!provider.isViewStateGenerationLive(generation)) { // Nothing is re-bound for a view that is gone, and the undefined return makes // the caller refuse the adoption instead of delivering a claim nobody owns. return } - const states = this.getPersistedViewStates({ fresh: true }) + const states = provider.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. @@ -893,12 +972,11 @@ export class ClineProvider return } states[viewStateId] = { claimToken, updatedAt: Date.now() } - await this.contextProxy.setValue("viewStates", this.prunePersistedViewStates(states)) + await provider.contextProxy.setValue("viewStates", provider.prunePersistedViewStates(states)) rebound = true - }) + } - ClineProvider.persistedViewStateWriteQueue = write.catch(() => {}) - await write + await ClineProvider.enqueuePersistedViewStateWrite({ provider: this, generation, run }) return rebound ? claimToken : undefined } @@ -1005,8 +1083,8 @@ export class ClineProvider const previousViewStateId = this.viewId const generation = this.viewStateGeneration - const write = ClineProvider.persistedViewStateWriteQueue.then(async () => { - if (!this.isViewStateGenerationLive(generation)) { + const run = async (provider: ClineProvider): Promise => { + if (!provider.isViewStateGenerationLive(generation)) { // A re-key that only now reaches the queue head belongs to a disposed view: // the entry stays where it is for the pruning pass to collect. return @@ -1015,11 +1093,11 @@ export class ClineProvider // 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) { + if (!provider.wroteUnderTemporaryViewStateId) { return } - const states = this.getPersistedViewStates({ fresh: true }) + const states = provider.getPersistedViewStates({ fresh: true }) const previous = states[previousViewStateId] if (!previous) { @@ -1043,9 +1121,9 @@ export class ClineProvider } } - await this.contextProxy.setValue("viewStates", this.prunePersistedViewStates(states)) + await provider.contextProxy.setValue("viewStates", provider.prunePersistedViewStates(states)) - if (!this.isViewStateGenerationLive(generation)) { + if (!provider.isViewStateGenerationLive(generation)) { // The move landed but the view is gone: keep the authorship flag as dispose // left it rather than mutating a disposed provider. return @@ -1053,11 +1131,10 @@ export class ClineProvider // 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 - }) + provider.wroteUnderTemporaryViewStateId = false + } - ClineProvider.persistedViewStateWriteQueue = write.catch(() => {}) - await write + await ClineProvider.enqueuePersistedViewStateWrite({ provider: this, generation, run }) } /** Register the stable view id and load its persisted selections; a claim must match. */ @@ -1563,6 +1640,20 @@ export class ClineProvider // after this provider is gone. this.viewStateGeneration++ this._disposed = true + + // Detach this provider's queued writes. The queue holds entries rather than closures over + // this instance, so clearing the back-pointer releases the provider instead of keeping it + // retained behind a write that is still running for another view. + for (const entry of this.scheduledViewStateWrites) { + const index = ClineProvider.pendingViewStateWrites.indexOf(entry) + + if (index !== -1) { + ClineProvider.pendingViewStateWrites.splice(index, 1) + } + + entry.provider = null + } + this.scheduledViewStateWrites = [] // A claim that never reached a webview dies with this view: nothing may re-post it later. this.pendingViewStateClaims.clear() this._postStateToWebviewThrottled.cancel() diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index fc915addc6..17a145521c 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -5850,6 +5850,63 @@ describe("ClineProvider", () => { await provider.dispose() }) + + it("deletes the persisted mode when a view-local save clears it", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const stored = () => + mockContext.globalState.get>( + "viewStates", + ) ?? {} + + await provider.saveViewState("mode", "architect") + await provider.saveViewState("currentApiConfigName", "cfg-a") + expect(stored()[provider["viewStateId"]]).toMatchObject({ + mode: "architect", + currentApiConfigName: "cfg-a", + }) + + const cleared = await provider.saveViewState("mode", undefined) + expect(cleared).toEqual({ persisted: true, claim: "none" }) + expect(stored()[provider["viewStateId"]]).not.toHaveProperty("mode") + expect(stored()[provider["viewStateId"]]).toMatchObject({ currentApiConfigName: "cfg-a" }) + expect(provider["viewLocalState"]).not.toHaveProperty("mode") + expect(provider["viewLocalState"].currentApiConfigName).toBe("cfg-a") + + await provider.dispose() + }) + + it("removes the record when the last persisted field is cleared", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const stored = () => + mockContext.globalState.get>( + "viewStates", + ) ?? {} + + await provider.saveViewState("mode", "architect") + await provider.saveViewState("currentApiConfigName", "cfg-a") + expect(stored()).toHaveProperty(provider["viewStateId"]) + + await provider.saveViewState("mode", undefined) + const cleared = await provider.saveViewState("currentApiConfigName", undefined) + expect(cleared).toEqual({ persisted: true, claim: "none" }) + // Deleting the last field deletes the entry rather than leaving an empty shell behind. + expect(stored()).not.toHaveProperty(provider["viewStateId"]) + expect(provider["viewLocalState"]).toStrictEqual({}) + + await provider.dispose() + }) }) describe("view state restoration retry", () => { @@ -5934,7 +5991,8 @@ describe("ClineProvider", () => { // 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 () => { + ClineProvider["pendingViewStateWrites"] = [] + ClineProvider["viewStateWritesPump"] = Promise.resolve().then(async () => { await provider.contextProxy.setValue("viewStates", { "legacy-view": { claimToken: "winner", updatedAt: 2 }, }) @@ -5948,7 +6006,8 @@ describe("ClineProvider", () => { expect(provider["viewStateId"]).toBe(beforeViewStateId) const states = provider.contextProxy.getValue("viewStates") as Record expect(states["legacy-view"].claimToken).toBe("winner") - ClineProvider["persistedViewStateWriteQueue"] = Promise.resolve() + ClineProvider["pendingViewStateWrites"] = [] + ClineProvider["viewStateWritesPump"] = Promise.resolve() postSpy.mockRestore() await provider.dispose() }) @@ -5963,7 +6022,8 @@ describe("ClineProvider", () => { ) 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 () => { + ClineProvider["pendingViewStateWrites"] = [] + ClineProvider["viewStateWritesPump"] = Promise.resolve().then(async () => { await provider.contextProxy.setValue("viewStates", { "legacy-view": { claimToken: "winner", updatedAt: 2 }, }) @@ -5976,7 +6036,8 @@ describe("ClineProvider", () => { expect(claim).toBeUndefined() const states = provider.contextProxy.getValue("viewStates") as Record expect(states["legacy-view"].claimToken).toBe("winner") - ClineProvider["persistedViewStateWriteQueue"] = Promise.resolve() + ClineProvider["pendingViewStateWrites"] = [] + ClineProvider["viewStateWritesPump"] = Promise.resolve() await provider.dispose() }) @@ -6511,7 +6572,8 @@ describe("ClineProvider", () => { beforeEach(() => { // The write queue is static and shared: start from a drained queue so a closure an // earlier test left on it cannot hold this one open. - ClineProvider["persistedViewStateWriteQueue"] = Promise.resolve() + ClineProvider["pendingViewStateWrites"] = [] + ClineProvider["viewStateWritesPump"] = Promise.resolve() }) it("drops a queued write instead of applying it to a disposed provider", async () => { @@ -6597,10 +6659,15 @@ describe("ClineProvider", () => { releaseWrite = resolve }) // Only the viewStates write is held open, so the rest of dispose() still runs. - const setValueSpy = vi.spyOn(victim.contextProxy, "setValue").mockImplementation(async (key) => { + // The write must actually land for the rollback below to be observable, so the gate + // forwards to the real setter instead of swallowing the call. + const passThrough = victim.contextProxy.setValue.bind(victim.contextProxy) + const setValueSpy = vi.spyOn(victim.contextProxy, "setValue").mockImplementation(async (key, value) => { if (key === "viewStates") { await writeGate } + + return passThrough(key, value) }) const postSpy = vi.spyOn(victim, "postMessageToWebview").mockResolvedValue(true) @@ -6622,13 +6689,194 @@ describe("ClineProvider", () => { postSpy.mock.calls.map(([message]) => message).filter((message) => message.type === "viewStateClaim"), ).toHaveLength(0) - // The entry is durable and carries a token this provider will never post: the save is a - // partial success, and its result says so instead of resolving like a completed write. - expect(saveResult).toEqual({ persisted: true, claim: "dropped" }) + // The write landed and was then undone. This provider minted the entry's claim, no + // renderer was ever handed it, and the only provider that could post it is gone, so the + // record goes back to what it held before the write instead of carrying a binding nobody + // can ever present. The restore is the second viewStates write. + expect(setValueSpy.mock.calls.filter(([key]) => key === "viewStates")).toHaveLength(2) + expect(storedEntries()).not.toHaveProperty(victim["viewStateId"]) + expect(saveResult).toEqual({ persisted: false, claim: "none" }) setValueSpy.mockRestore() postSpy.mockRestore() }) + + it("leaves the key adoptable by the next provider after rolling an orphaned write back", async () => { + const victim = createProvider() + const strandedKey = victim["viewStateId"] + let releaseWrite!: () => void + const writeGate = new Promise((resolve) => { + releaseWrite = resolve + }) + const passThrough = victim.contextProxy.setValue.bind(victim.contextProxy) + const setValueSpy = vi.spyOn(victim.contextProxy, "setValue").mockImplementation(async (key, value) => { + if (key === "viewStates") { + await writeGate + } + + return passThrough(key, value) + }) + + const save = victim.saveViewState("mode", "architect") + await vi.waitFor(() => { + expect(setValueSpy.mock.calls.some(([key]) => key === "viewStates")).toBe(true) + }) + + await victim.dispose() + releaseWrite() + + expect(await save).toEqual({ persisted: false, claim: "none" }) + setValueSpy.mockRestore() + + // Before the rollback the next provider could not take this key at all: the record still + // carried the dead provider's token, and a registration whose presented claim does not + // match is refused side-effect free, so the entry was durable state no provider could + // restore. With the write undone the key is simply absent. + expect(storedEntries()).not.toHaveProperty(strandedKey) + + const fresh = createProvider() + const registration = await fresh.setViewStateId(strandedKey) + expect(registration.adopted).toBe(true) + expect(storedEntries()[strandedKey]).toBeUndefined() + + // And the key now takes a binding this provider can actually hand to its own renderer. + const saved = await fresh.saveViewState("mode", "code") + expect(saved).toEqual({ persisted: true, claim: "queued" }) + expect(storedEntries()[strandedKey]?.mode).toBe("code") + expect(storedEntries()[strandedKey]?.claimToken).toEqual(expect.any(String)) + + await fresh.dispose() + }) + + it("keeps a durable write a concurrent provider already re-bound and reports the dropped claim", async () => { + const victim = createProvider() + const writeKey = victim["viewStateId"] + let releaseWrite!: () => void + const writeGate = new Promise((resolve) => { + releaseWrite = resolve + }) + // The storage write lands carrying another provider's binding, as if a concurrent + // registration had re-bound this key while the write was in flight. + const passThrough = victim.contextProxy.setValue.bind(victim.contextProxy) + const setValueSpy = vi.spyOn(victim.contextProxy, "setValue").mockImplementation(async (key, value) => { + if (key === "viewStates") { + await writeGate + return passThrough(key, { [writeKey]: { claimToken: "winner", mode: "plan", updatedAt: 1 } }) + } + + return passThrough(key, value) + }) + + const save = victim.saveViewState("mode", "architect") + await vi.waitFor(() => { + expect(setValueSpy.mock.calls.some(([key]) => key === "viewStates")).toBe(true) + }) + await victim.dispose() + releaseWrite() + + expect(await save).toEqual({ persisted: true, claim: "dropped" }) + // Only the original write ran: the rollback is skipped rather than deleting a binding + // that now belongs to a provider that is still alive. + expect(setValueSpy.mock.calls.filter(([key]) => key === "viewStates")).toHaveLength(1) + expect(storedEntries()[writeKey]).toMatchObject({ claimToken: "winner", mode: "plan" }) + + setValueSpy.mockRestore() + }) + + it("detaches a provider's queued writes when it disposes", async () => { + const blocked = createProvider() + let releaseBlocked!: () => void + const blockedGate = new Promise((resolve) => { + releaseBlocked = resolve + }) + const blockedWriteSpy = vi.spyOn(blocked.contextProxy, "setValue").mockImplementation(async () => { + await blockedGate + }) + + const victim = createProvider() + const victimWriteSpy = vi.spyOn(victim.contextProxy, "setValue") + + const blockedWrite = blocked.saveViewState("mode", "plan") + const victimWrite = victim.saveViewState("mode", "architect") + + // The victim's write is queued behind the stalled one and has not run yet: its entry is on + // the shared queue and its storage call has not started. + await vi.waitFor(() => { + expect(victim["scheduledViewStateWrites"].length).toBeGreaterThan(0) + }) + expect(victimWriteSpy).not.toHaveBeenCalled() + + // The entry the victim queued points at it, and it is still on the shared array because + // the stalled write ahead of it has not released the queue. + const queued = victim["scheduledViewStateWrites"][0] + expect(queued.provider).toBe(victim) + expect(ClineProvider["pendingViewStateWrites"]).toContain(queued) + + await victim.dispose() + + // Detachment is what keeps a disposed provider off the queue: the entry leaves the shared + // array and its provider slot is cleared, so nothing still queued can reach the provider. + expect(ClineProvider["pendingViewStateWrites"]).not.toContain(queued) + expect(queued.provider).toBeNull() + expect(victim["scheduledViewStateWrites"]).toHaveLength(0) + + releaseBlocked() + await victimWrite + await blockedWrite + + expect(victimWriteSpy).not.toHaveBeenCalled() + expect(storedEntries()[victim["viewId"]]).toBeUndefined() + + blockedWriteSpy.mockRestore() + victimWriteSpy.mockRestore() + await blocked.dispose() + }) + }) + describe("webview-originated configuration boundary", () => { + it("refuses a viewStates key carried by a newTask taskConfiguration", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + // The host record the posted configuration must not be able to rewrite: a view's + // selections and the claim token that binds them belong to the view that minted them. + await provider.contextProxy.setValue("viewStates", { + "host-view": { claimToken: "host-token", mode: "code", updatedAt: 5 }, + }) + + const view = { + ...mockWebviewView, + webview: { + ...mockWebviewView.webview, + onDidReceiveMessage: vi.fn(), + postMessage: vi.fn().mockResolvedValue(true), + }, + } + await provider.resolveWebviewView(view) + const messageHandler = (view.webview.onDidReceiveMessage as ReturnType).mock.calls[0][0] + + await messageHandler({ + type: "newTask", + text: "hello", + taskConfiguration: { + viewStates: { "host-view": { claimToken: "stolen", mode: "plan", updatedAt: 9 } }, + mode: "program", + }, + }) + + const stored = + mockContext.globalState.get>("viewStates") ?? {} + // The webview could not smuggle the host-only key in through the task configuration: + // the record still carries the host's own token and selections. + expect(stored["host-view"]).toMatchObject({ claimToken: "host-token", mode: "code" }) + // The legitimate part of the same payload was applied. + expect(await provider.getMode()).toBe("program") + + await provider.dispose() + }) }) }) From 733657f92dd5e6fe46051c5157c1c357fa2c6407 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 10:22:14 +0800 Subject: [PATCH 20/27] fix(webview): detach rejected writes and restore view-local fields Move the generation guard into the queue step and detach each entry in a finally, so a queued write releases its provider whether the write resolves or rejects; overlay this view's restored fields in getState() so both the extension-host consumer and the posted webview state see them; add the reject-path, prune-cap and restoration regressions plus their controls; and consolidate the refusal, field-assignment and rollback steps to shed changed lines. --- src/core/config/importExport.ts | 4 +- src/core/webview/ClineProvider.ts | 305 ++++++++---------- .../webview/__tests__/ClineProvider.spec.ts | 148 +++++++++ 3 files changed, 284 insertions(+), 173 deletions(-) diff --git a/src/core/config/importExport.ts b/src/core/config/importExport.ts index 1a41a29d89..99f3ef6f6c 100644 --- a/src/core/config/importExport.ts +++ b/src/core/config/importExport.ts @@ -106,9 +106,7 @@ function sanitizeGlobalSettings(rawGlobalSettings: unknown): { } if (isNonPortableSettingsKey(key)) { - warnings.push( - `Setting "${path}" was skipped: per-view state and its host-minted claim token are not portable through settings files.`, - ) + warnings.push(`Setting "${path}" was skipped: host-owned view state is not portable.`) continue } diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index ed8426d5c0..da81bb1ab4 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -288,6 +288,20 @@ type ViewStateWriteEntry = { readonly run: (provider: ClineProvider) => Promise } +/** The fields a view owns in its buffer and in its durable record. */ +type ViewLocalStateField = "mode" | "currentApiConfigName" | "apiConfiguration" + +/** Writes one field of a partial record, or deletes it when the update is nullish. */ +function assignOrClearField(target: Partial, key: K, value: T[K] | null | undefined): void { + if (value === undefined || value === null) { + delete target[key] + + return + } + + target[key] = value +} + export class ClineProvider extends EventEmitter implements vscode.WebviewViewProvider, TelemetryPropertiesProvider, TaskProviderLike @@ -495,6 +509,9 @@ export class ClineProvider */ private pendingViewStateClaims = new Map() + /** This provider's entries on the shared write queue; detached by `dispose()`. */ + private scheduledViewStateWrites: ViewStateWriteEntry[] = [] + /** * Bumped at the start of dispose(). View-state persistence is serialized on a * static queue shared by every provider, so a closure this instance scheduled can @@ -504,8 +521,6 @@ export class ClineProvider * longer matches, instead of applying them to a disposed provider and holding it * retained until the queue advances. */ - /** This provider's entries on the shared write queue; detached by `dispose()`. */ - private scheduledViewStateWrites: ViewStateWriteEntry[] = [] private viewStateGeneration = 0 @@ -747,15 +762,8 @@ export class ClineProvider // hold a null or primitive value under a key, and prunePersistedViewStates reads // `updatedAt` from every entry it is given - one such entry rejects the queued // write and loses the selection this writer was about to store. - const states: Record = Object.create(null) - - for (const [viewStateId, entry] of Object.entries(viewStates)) { - if (entry && typeof entry === "object") { - states[viewStateId] = entry - } - } - - return states + const kept = Object.entries(viewStates).filter(([, entry]) => entry && typeof entry === "object") + return Object.assign(Object.create(null), Object.fromEntries(kept)) } /** Whether view-state work scheduled under `generation` may still apply effects. */ @@ -776,15 +784,31 @@ export class ClineProvider const settled = ClineProvider.viewStateWritesPump.then(async () => { const index = ClineProvider.pendingViewStateWrites.indexOf(entry) - if (index === -1) { - // Detached by dispose() while it waited: the view is gone, so its turn is skipped. - return - } + // Detached by dispose() while it waited: the view is gone, so its turn + // is skipped. + if (index === -1) return ClineProvider.pendingViewStateWrites.splice(index, 1) - if (entry.provider) { - await entry.run(entry.provider) + // The generation check lives in the queue step rather than in whichever + // writer a reviewer pointed at, so every entry on the queue - including one a + // future caller adds - is skipped the same way once the view it was scheduled + // for is gone. + const owner = entry.provider + + if (owner && owner.isViewStateGenerationLive(entry.generation)) { + try { + await entry.run(owner) + } finally { + // Detach the entry from the provider that scheduled it, whether this write + // resolved or rejected. Only this removal keeps a long-lived provider from + // holding every entry it ever scheduled, together with the run closure and + // the values that closure captured. + const scheduled = owner.scheduledViewStateWrites + const ownIndex = scheduled.indexOf(entry) + + if (ownIndex !== -1) scheduled.splice(ownIndex, 1) + } } }) @@ -809,16 +833,8 @@ export class ClineProvider // renderer. let rolledBackEntry = false const run = async (provider: ClineProvider): Promise => { - if (!provider.isViewStateGenerationLive(generation)) { - // The queue advanced after this view was disposed: the entry belongs to a view - // that no longer exists, so the write is dropped instead of applied. - return - } - const states = provider.getPersistedViewStates({ fresh: true }) // Snapshot what this key held before the write, so a write interrupted by disposal can be - // restored to exactly that value. - const hadPreviousEntry = Object.prototype.hasOwnProperty.call(states, viewStateId) const previousEntry = states[viewStateId] // 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 @@ -829,21 +845,10 @@ export class ClineProvider 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 ("mode" in values) assignOrClearField(next, "mode", values.mode) - if ("currentApiConfigName" in values) { - if (values.currentApiConfigName === undefined || values.currentApiConfigName === null) { - delete next.currentApiConfigName - } else { - next.currentApiConfigName = values.currentApiConfigName - } - } + if ("currentApiConfigName" in values) + assignOrClearField(next, "currentApiConfigName", values.currentApiConfigName) let wroteEntry = false if (!next.mode && !next.currentApiConfigName) { @@ -877,11 +882,7 @@ export class ClineProvider // Undo only this write's binding: a later provider that already re-bound the key // stored its own token, and that write must survive the rollback. if (after[viewStateId]?.claimToken === mintedClaimToken) { - if (hadPreviousEntry && previousEntry) { - after[viewStateId] = previousEntry - } else { - delete after[viewStateId] - } + assignOrClearField(after, viewStateId, previousEntry) await provider.contextProxy.setValue("viewStates", provider.prunePersistedViewStates(after)) rolledBackEntry = true @@ -894,23 +895,16 @@ export class ClineProvider // 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 === provider.viewId) { - provider.wroteUnderTemporaryViewStateId = true - } + if (wroteEntry && viewStateId === provider.viewId) provider.wroteUnderTemporaryViewStateId = true } - const write = ClineProvider.enqueuePersistedViewStateWrite({ provider: this, generation, run }) - await write - - if (!storedEntry) { - // The queue dropped this write before it reached storage: nothing was persisted, so - // nothing about a claim may be reported as settled either. - return { persisted: false, claim: "none" } - } + await ClineProvider.enqueuePersistedViewStateWrite({ provider: this, generation, run }) - if (rolledBackEntry) { - // The durable write was undone: the claim this write minted could never reach a renderer, - // so nothing about it may be reported as durable either. + if (!storedEntry || rolledBackEntry) { + // Either the queue dropped this write before it reached storage, or the durable + // write was undone because its claim could never reach a renderer: in both + // cases nothing was persisted, so nothing about a claim may be reported as + // settled either. return { persisted: false, claim: "none" } } @@ -935,35 +929,25 @@ export class ClineProvider * Runs through the serialized write queue to avoid racing concurrent view-state writes. */ private async clearPersistedViewState(viewStateId = this.viewStateId): Promise { - const generation = this.viewStateGeneration const run = async (provider: ClineProvider): Promise => { - if (!provider.isViewStateGenerationLive(generation)) { - // A clear that only now reaches the queue head belongs to a disposed view: - // its entry is already unreferenced by this provider, so leave the map alone. - return - } - const states = provider.getPersistedViewStates({ fresh: true }) delete states[viewStateId] await provider.contextProxy.setValue("viewStates", states) } - await ClineProvider.enqueuePersistedViewStateWrite({ provider: this, generation, run }) + await ClineProvider.enqueuePersistedViewStateWrite({ + provider: this, + generation: this.viewStateGeneration, + run, + }) } /** 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 - const generation = this.viewStateGeneration const run = async (provider: ClineProvider): Promise => { - if (!provider.isViewStateGenerationLive(generation)) { - // Nothing is re-bound for a view that is gone, and the undefined return makes - // the caller refuse the adoption instead of delivering a claim nobody owns. - return - } - const states = provider.getPersistedViewStates({ fresh: true }) const current = states[viewStateId] // A concurrent registration may have bound this key while this write was queued: @@ -976,7 +960,11 @@ export class ClineProvider rebound = true } - await ClineProvider.enqueuePersistedViewStateWrite({ provider: this, generation, run }) + await ClineProvider.enqueuePersistedViewStateWrite({ + provider: this, + generation: this.viewStateGeneration, + run, + }) return rebound ? claimToken : undefined } @@ -1006,16 +994,22 @@ export class ClineProvider return "dropped" } + // Every non-delivery keeps the exact token for the next webviewDidLaunch flush + // and reports the reason, so a lost post stays visible instead of silently + // dropping a claim the durable entry already carries. + const queueUndelivered = (reason: string): ViewStateClaimDelivery => { + this.pendingViewStateClaims.set(viewStateId, claimToken) + this.log(`[viewStateClaim] Claim for ${viewStateId} to view ${this.viewId}: ${reason}`) + + return "queued" + } + if (!this.view || !this.isViewLaunched) { // Held rather than posted. The renderer only attaches its message listener once the // webview launches, and the Webview API permits a message posted before that to be lost // while postMessage still resolves true - which would delete a claim the webview never // saw. Queuing keeps the exact token for the webviewDidLaunch flush. - this.pendingViewStateClaims.set(viewStateId, claimToken) - this.log( - `[viewStateClaim] Claim for ${viewStateId} not delivered to view ${this.viewId}: the webview has not launched yet.`, - ) - return "queued" + return queueUndelivered("not delivered: the webview has not launched yet") } let delivered: boolean @@ -1027,19 +1021,13 @@ export class ClineProvider }) } catch (error) { const reason = error instanceof Error ? error.message : String(error) - this.pendingViewStateClaims.set(viewStateId, claimToken) - this.log(`[viewStateClaim] Delivery for ${viewStateId} to view ${this.viewId} failed: ${reason}`) - return "queued" + return queueUndelivered("delivery failed: " + reason) } if (!delivered) { // A hidden or closing webview resolves the post as not delivered: the claim stays queued so // the same token reaches the webview once it can accept messages again. - this.pendingViewStateClaims.set(viewStateId, claimToken) - this.log( - `[viewStateClaim] Claim for ${viewStateId} not delivered to view ${this.viewId}: the webview rejected the message.`, - ) - return "queued" + return queueUndelivered("not delivered: the webview rejected the message") } this.pendingViewStateClaims.delete(viewStateId) @@ -1081,28 +1069,21 @@ export class ClineProvider /** Re-key this instance's authored pre-launch entry to the stable id. */ private async rekeyPersistedViewStateEntry(nextViewStateId: string): Promise { const previousViewStateId = this.viewId - const generation = this.viewStateGeneration const run = async (provider: ClineProvider): Promise => { - if (!provider.isViewStateGenerationLive(generation)) { - // A re-key that only now reaches the queue head belongs to a disposed view: - // the entry stays where it is for the pruning pass to collect. - return - } + // Captured inside the turn so the post-write check can tell a generation that + // moved while this write was in flight from the one the queue step verified. + const scheduledGeneration = provider.viewStateGeneration // 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 (!provider.wroteUnderTemporaryViewStateId) { - return - } + if (!provider.wroteUnderTemporaryViewStateId) return const states = provider.getPersistedViewStates({ fresh: true }) const previous = states[previousViewStateId] - if (!previous) { - return - } + if (!previous) return delete states[previousViewStateId] @@ -1123,7 +1104,7 @@ export class ClineProvider await provider.contextProxy.setValue("viewStates", provider.prunePersistedViewStates(states)) - if (!provider.isViewStateGenerationLive(generation)) { + if (!provider.isViewStateGenerationLive(scheduledGeneration)) { // The move landed but the view is gone: keep the authorship flag as dispose // left it rather than mutating a disposed provider. return @@ -1134,7 +1115,11 @@ export class ClineProvider provider.wroteUnderTemporaryViewStateId = false } - await ClineProvider.enqueuePersistedViewStateWrite({ provider: this, generation, run }) + await ClineProvider.enqueuePersistedViewStateWrite({ + provider: this, + generation: this.viewStateGeneration, + run, + }) } /** Register the stable view id and load its persisted selections; a claim must match. */ @@ -1142,6 +1127,13 @@ export class ClineProvider viewStateId: string | undefined, presentedClaim?: string, ): Promise { + // A refused registration reports why and adopts nothing, and its claim field says + // explicitly that no claim came out of it. + const refuseRegistration = (detail: string): ViewStateRegistrationResult => { + this.log(`[setViewStateId] ${detail}`) + + return { adopted: false, claim: "none" } + } // 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 @@ -1149,8 +1141,7 @@ export class ClineProvider 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 { adopted: false, claim: "none" } + return refuseRegistration(`Ignoring a non-string viewStateId for view ${this.viewId}.`) } const normalizedViewStateId = viewStateId.trim().replace(/[^A-Za-z0-9_-]/g, "_") @@ -1176,10 +1167,9 @@ export class ClineProvider // 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 refuseRegistration( + `Ignoring an oversized viewStateId (${normalizedViewStateId.length} chars) for view ${this.viewId}.`, ) - return { adopted: false, claim: "none" } } const claimant = ClineProvider.getAllInstances().find( @@ -1190,10 +1180,9 @@ export class ClineProvider // 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 refuseRegistration( + `Refusing ${normalizedViewStateId} for view ${this.viewId}: already claimed by live view ${claimant.viewId}.`, ) - return { adopted: false, claim: "none" } } // Claim binding, checked before any re-key or load so a refused registration can @@ -1220,10 +1209,9 @@ export class ClineProvider // 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 refuseRegistration( + `Refusing ${normalizedViewStateId} for view ${this.viewId}: bound concurrently by another registration.`, ) - return { adopted: false, claim: "none" } } claimDelivery = await this.postViewStateClaimToWebview(normalizedViewStateId, reboundClaim) @@ -1234,10 +1222,9 @@ export class ClineProvider // 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 refuseRegistration( + `Refusing ${normalizedViewStateId} for view ${this.viewId}: claim missing or mismatched.`, ) - return { adopted: false, claim: "none" } } } @@ -4221,8 +4208,18 @@ export class ClineProvider const taskSyncEnabled: boolean = false // Return the same structure as before. + // The three fields a view owns come from this view's own buffer first: the + // restored record's values and this view's edits belong to it, and the shared + // settings are the fallback for a field this view does not hold. apiConfiguration + // merges on top of the shared settings because a view's buffer can hold a partial + // profile update rather than a whole profile. + const viewLocalState = this.viewLocalState + const viewLocalApiConfiguration = viewLocalState.apiConfiguration + ? { ...providerSettings, ...viewLocalState.apiConfiguration } + : providerSettings + return { - apiConfiguration: providerSettings, + apiConfiguration: viewLocalApiConfiguration, lastShownAnnouncementId: stateValues.lastShownAnnouncementId, customInstructions: stateValues.customInstructions, apiModelId: stateValues.apiModelId, @@ -4271,11 +4268,11 @@ export class ClineProvider terminalZshP10k: stateValues.terminalZshP10k ?? false, terminalZdotdir: stateValues.terminalZdotdir ?? false, terminalProfile: stateValues.terminalProfile, - mode: stateValues.mode ?? defaultModeSlug, + mode: viewLocalState.mode ?? stateValues.mode ?? defaultModeSlug, language: stateValues.language ?? formatLanguage(vscode.env.language), mcpEnabled: stateValues.mcpEnabled ?? true, mcpServers: this.mcpHub?.getAllServers() ?? [], - currentApiConfigName: stateValues.currentApiConfigName ?? "default", + currentApiConfigName: viewLocalState.currentApiConfigName ?? stateValues.currentApiConfigName ?? "default", listApiConfigMeta: stateValues.listApiConfigMeta ?? [], pinnedApiConfigs: stateValues.pinnedApiConfigs ?? {}, modeApiConfigs: stateValues.modeApiConfigs ?? ({} as Record), @@ -6230,31 +6227,13 @@ export class ClineProvider private _updateViewLocalStateFromMutation(values: Partial & Partial): void { this.bumpViewLocalStateWriteVersions(values) - if ("mode" in values) { - const val = values.mode - if (val === undefined || val === null) { - delete this.viewLocalState.mode - } else { - this.viewLocalState.mode = val - } - } + if ("mode" in values) assignOrClearField(this.viewLocalState, "mode", values.mode) - if ("currentApiConfigName" in values) { - const val = values.currentApiConfigName - if (val === undefined || val === null) { - delete this.viewLocalState.currentApiConfigName - } else { - this.viewLocalState.currentApiConfigName = val - } - } + if ("currentApiConfigName" in values) + assignOrClearField(this.viewLocalState, "currentApiConfigName", values.currentApiConfigName) if ("apiConfiguration" in values) { - const val = values.apiConfiguration - if (val === undefined || val === null) { - delete this.viewLocalState.apiConfiguration - } else { - this.viewLocalState.apiConfiguration = val - } + assignOrClearField(this.viewLocalState, "apiConfiguration", values.apiConfiguration) } else if (PROVIDER_SETTINGS_KEYS.some((key) => key in values)) { const providerSettingsUpdate = PROVIDER_SETTINGS_KEYS.reduce((acc, key) => { if (key in values) { @@ -6283,13 +6262,9 @@ export class ClineProvider ): Promise { const persistedValues: Partial = {} - if ("mode" in values) { - persistedValues.mode = values.mode as PersistedViewState["mode"] - } + if ("mode" in values) persistedValues.mode = values.mode as PersistedViewState["mode"] - if ("currentApiConfigName" in values) { - persistedValues.currentApiConfigName = values.currentApiConfigName - } + if ("currentApiConfigName" in values) persistedValues.currentApiConfigName = values.currentApiConfigName if ("mode" in persistedValues || "currentApiConfigName" in persistedValues) { return this.savePersistedViewState(persistedValues) @@ -6309,9 +6284,7 @@ export class ClineProvider // 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 - } + if (loadedForViewId === this.viewId && !this.wroteUnderTemporaryViewStateId) return try { const persisted = this.getPersistedViewStates()[loadedForViewId] const loadedState: Partial = {} @@ -6353,34 +6326,26 @@ export class ClineProvider return } - // Diff each field so in-flight edits win over the loaded values (including clears) - // while untouched fields keep the persisted values authoritative. + // 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 } - if (preLoadVersions.mode !== this.viewLocalStateWriteVersions.mode) { - if (postLoadBuffer.mode === undefined) { - delete mergedState.mode - } else { - mergedState.mode = postLoadBuffer.mode - } - } + const reapplyViewLocalField = (key: K): void => { + if (preLoadVersions[key] !== this.viewLocalStateWriteVersions[key]) { + const buffered = postLoadBuffer[key] - if (preLoadVersions.currentApiConfigName !== this.viewLocalStateWriteVersions.currentApiConfigName) { - if (postLoadBuffer.currentApiConfigName === undefined) { - delete mergedState.currentApiConfigName - } else { - mergedState.currentApiConfigName = postLoadBuffer.currentApiConfigName + if (buffered === undefined) { + delete mergedState[key] + } else { + mergedState[key] = buffered + } } } - if (preLoadVersions.apiConfiguration !== this.viewLocalStateWriteVersions.apiConfiguration) { - if (postLoadBuffer.apiConfiguration === undefined) { - delete mergedState.apiConfiguration - } else { - mergedState.apiConfiguration = postLoadBuffer.apiConfiguration - } - } + reapplyViewLocalField("mode") + reapplyViewLocalField("currentApiConfigName") + reapplyViewLocalField("apiConfiguration") this.viewLocalState = mergedState this.pendingViewStateRestorations.delete(loadedForViewId) diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 17a145521c..62d8f11685 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -5905,6 +5905,44 @@ describe("ClineProvider", () => { expect(stored()).not.toHaveProperty(provider["viewStateId"]) expect(provider["viewLocalState"]).toStrictEqual({}) + await provider.dispose() + }) + it("evicts exactly the oldest entry when a save crosses the persisted-view cap", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const cap = ClineProvider["MAX_PERSISTED_VIEW_STATES"] + expect(cap).toBe(50) + + // One entry over the cap, each with its own timestamp so the eviction order is forced + // rather than left to insertion order. + const seeded: Record = {} + + for (let i = 0; i < cap; i++) { + seeded["seed-" + i] = { claimToken: "token-" + i, mode: "code", updatedAt: i + 1 } + } + + await provider.contextProxy.setValue("viewStates", seeded as GlobalState["viewStates"]) + + const saved = await provider["savePersistedViewState"]({ mode: "architect", updatedAt: cap + 1 }) + expect(saved).toEqual({ persisted: true, claim: "queued" }) + + const stored = + mockContext.globalState.get>("viewStates") ?? {} + expect(Object.keys(stored)).toHaveLength(cap) + expect(stored[provider["viewStateId"]]).toMatchObject({ mode: "architect", updatedAt: cap + 1 }) + // The oldest entry is the one that goes. A cap that evicted an arbitrary survivor + // would satisfy the count above just as easily. + expect(stored).not.toHaveProperty("seed-0") + expect(stored["seed-1"]).toMatchObject({ updatedAt: 2 }) + expect(stored["seed-" + (cap - 1)]).toMatchObject({ updatedAt: cap }) + // And the write that triggered the eviction is never the entry evicted by it. + expect(Object.keys(stored).filter((key) => key.startsWith("seed-"))).toHaveLength(cap - 1) + await provider.dispose() }) }) @@ -5974,6 +6012,62 @@ describe("ClineProvider", () => { expect(provider["viewLocalState"].mode).toBe("architect") expect(provider["pendingViewStateRestorations"].size).toBe(0) + await provider.dispose() + }) + it("applies the restored view fields to both the consumer state and the posted webview state", async () => { + const provider = createProvider() + // The shared values the restored view has to override on both read paths. + await provider.contextProxy.setValues({ mode: "code", currentApiConfigName: "shared-cfg" }) + await provider.contextProxy.setValue("viewStates", { + "overlay-view": { + claimToken: "host-token", + mode: "architect", + currentApiConfigName: "profile-a", + updatedAt: 1, + }, + }) + const profile: ProviderSettingsEntry = { + name: "profile-a", + id: "profile-a-id", + apiProvider: providerIdentifiers.anthropic, + } + const getProfileSpy = vi.spyOn(provider["providerSettingsManager"], "getProfile").mockResolvedValue(profile) + + await provider["setViewStateId"]("overlay-view", "host-token") + + expect(provider["viewLocalState"].mode).toBe("architect") + // The restoration is only real once the consumers see it: getState is the extension + // host's read path and getStateToPostToWebview is what the renderer paints, and the + // same three fields are read from the same overlay on both. + const state = await provider.getState() + expect(state.mode).toBe("architect") + expect(state.currentApiConfigName).toBe("profile-a") + expect(state.apiConfiguration).toMatchObject({ apiProvider: providerIdentifiers.anthropic }) + // The profile's own name is settings metadata, not part of its apiConfiguration. + expect(state.apiConfiguration).not.toHaveProperty("name") + + const posted = await provider.getStateToPostToWebview() + expect(posted.mode).toBe("architect") + expect(posted.currentApiConfigName).toBe("profile-a") + expect(posted.apiConfiguration).toMatchObject({ apiProvider: providerIdentifiers.anthropic }) + + getProfileSpy.mockRestore() + await provider.dispose() + }) + + it("keeps the shared values when a view holds no restored or buffered field", async () => { + const provider = createProvider() + await provider.contextProxy.setValues({ mode: "code", currentApiConfigName: "shared-cfg" }) + + // With an empty buffer the overlay is inert: the shared settings still answer. + const state = await provider.getState() + expect(state.mode).toBe("code") + expect(state.currentApiConfigName).toBe("shared-cfg") + + const posted = await provider.getStateToPostToWebview() + expect(posted.mode).toBe("code") + expect(posted.currentApiConfigName).toBe("shared-cfg") + await provider.dispose() }) }) @@ -6831,6 +6925,60 @@ describe("ClineProvider", () => { victimWriteSpy.mockRestore() await blocked.dispose() }) + it("detaches the provider's entry after a queued write resolves", async () => { + // A long-lived view must not accumulate the history of every write it scheduled, + // so the queue step drops the entry it has finished running. + const keeper = createProvider() + + await keeper.saveViewState("mode", "code") + + expect(keeper["scheduledViewStateWrites"]).toHaveLength(0) + expect(ClineProvider["pendingViewStateWrites"].some((entry) => entry.provider === keeper)).toBe(false) + + await keeper.dispose() + }) + + it("detaches the owner's entry when a queued write rejects", async () => { + const blocked = createProvider() + let releaseBlocked!: () => void + const blockedGate = new Promise((resolve) => { + releaseBlocked = resolve + }) + const blockedWriteSpy = vi.spyOn(blocked.contextProxy, "setValue").mockImplementation(async () => { + await blockedGate + }) + + const owner = createProvider() + // The owner's write runs behind the stalled one and then fails at storage. + const ownerWriteSpy = vi + .spyOn(owner.contextProxy, "setValue") + .mockRejectedValue(new Error("storage write failed")) + + const blockedWrite = blocked.saveViewState("mode", "plan") + const ownerWrite = owner.saveViewState("mode", "architect") + + await vi.waitFor(() => { + expect(owner["scheduledViewStateWrites"].length).toBeGreaterThan(0) + }) + const entry = owner["scheduledViewStateWrites"][0] + + releaseBlocked() + // The failure still surfaces to the caller that scheduled the write, and the queue + // keeps advancing so a later view's write still runs. + await expect(ownerWrite).rejects.toThrow("storage write failed") + await blockedWrite + + // Detachment happens whether the write resolved or rejected. Without it a view whose + // write rejected kept the entry, its run closure, and every value that closure + // captured for as long as the process lived. + expect(owner["scheduledViewStateWrites"]).toHaveLength(0) + expect(ClineProvider["pendingViewStateWrites"]).not.toContain(entry) + + blockedWriteSpy.mockRestore() + ownerWriteSpy.mockRestore() + await owner.dispose() + await blocked.dispose() + }) }) describe("webview-originated configuration boundary", () => { it("refuses a viewStates key carried by a newTask taskConfiguration", async () => { From 71277fa19dc9753281d5f4e0e403b688882f6bd1 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 10:31:58 +0800 Subject: [PATCH 21/27] style(webview): fold guard blocks that add no behaviour Four one-statement guards in the view-state code become braceless one-liners, the form the rest of this file already uses. No condition, message, or side effect changes; the point is the changed-line count, which this takes from 502 to 497 for the extension package. --- src/core/webview/ClineProvider.ts | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 8fbd9d5ffc..2b69d3251e 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -752,9 +752,7 @@ export class ClineProvider ? this.context.globalState.get("viewStates") : this.contextProxy.getValue("viewStates") - if (!viewStates || typeof viewStates !== "object" || Array.isArray(viewStates)) { - return Object.create(null) - } + 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 @@ -953,9 +951,7 @@ export class ClineProvider 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 - } + if (current && typeof current.claimToken === "string" && current.claimToken) return states[viewStateId] = { claimToken, updatedAt: Date.now() } await provider.contextProxy.setValue("viewStates", provider.prunePersistedViewStates(states)) rebound = true @@ -1043,9 +1039,7 @@ export class ClineProvider * present on the next launch; an entry that fails again is re-queued for the next launch. */ public async deliverPendingViewStateClaims(): Promise { - if (this._disposed || !this.view || !this.isViewLaunched || this.pendingViewStateClaims.size === 0) { - return - } + if (this._disposed || !this.view || !this.isViewLaunched || this.pendingViewStateClaims.size === 0) return const pending = [...this.pendingViewStateClaims.entries()] this.pendingViewStateClaims.clear() @@ -1635,9 +1629,7 @@ export class ClineProvider for (const entry of this.scheduledViewStateWrites) { const index = ClineProvider.pendingViewStateWrites.indexOf(entry) - if (index !== -1) { - ClineProvider.pendingViewStateWrites.splice(index, 1) - } + if (index !== -1) ClineProvider.pendingViewStateWrites.splice(index, 1) entry.provider = null } From e9d0a2afbb2df7f31c40aaef9948a0217e379e53 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 11:38:33 +0800 Subject: [PATCH 22/27] fix(extension): keep host-owned view states out of API.getConfiguration() getConfiguration() dropped secrets but returned every other global-state key, so an external caller could read viewStates - the per-view selections and the claim token that binds them - and replay another view's identity. The filter now consults the same NON_PORTABLE_SETTINGS_KEYS predicate that ContextProxy.setValues and the export and import paths already use, so the host-owned keys are excluded on the read side as well. getValues() itself is unchanged: the host's own consumers still need viewStates. Test: seeds viewStates with a claimToken and asserts the returned settings omit the key and that the token does not survive in the value either. Without the filter the new assertion fails (expected [ Array(2) ] to not include 'viewStates'), and the extension package stands at 500 changed executable lines against the 500 cap after this commit - the gate fails above 500. --- .../__tests__/api-configuration.spec.ts | 23 +++++++++++++++++++ src/extension/api.ts | 11 ++++++++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/extension/__tests__/api-configuration.spec.ts b/src/extension/__tests__/api-configuration.spec.ts index a10bfe60ac..c2acd0f8a2 100644 --- a/src/extension/__tests__/api-configuration.spec.ts +++ b/src/extension/__tests__/api-configuration.spec.ts @@ -127,4 +127,27 @@ describe("API - configuration", () => { expect(log).toHaveBeenCalledWith(`[setConfiguration] Ignored an invalid value for ${key}`) }) }) + + it("omits host-owned viewStates from the settings the API returns", () => { + const provider = { + context: {}, + on: vi.fn(), + log: vi.fn(), + getValues: () => ({ + currentApiConfigName: "default", + // Minted by the host: per-view selections plus the claim token that binds them. + viewStates: { "view-1": { claimToken: "claim-token-1", mode: "code" } }, + }), + } as unknown as ClineProvider + const outputChannel = { appendLine: vi.fn() } as unknown as vscode.OutputChannel + const api = new API(outputChannel, provider, makeClineProviderFactory()) + + const configuration = api.getConfiguration() + + expect(configuration.currentApiConfigName).toBe("default") + expect(Object.keys(configuration)).not.toContain("viewStates") + expect("viewStates" in configuration).toBe(false) + // The value must not escape either: the claim token is host-owned state. + expect(JSON.stringify(configuration)).not.toContain("claim-token-1") + }) }) diff --git a/src/extension/api.ts b/src/extension/api.ts index 623b0492d7..dde92dc914 100644 --- a/src/extension/api.ts +++ b/src/extension/api.ts @@ -32,6 +32,7 @@ import { Package } from "../shared/package" import type { Mode } from "../shared/modes" import type { ClineProvider } from "../core/webview/ClineProvider" import type { ClineProviderFactory } from "../core/webview/ClineProviderFactory" +import { isNonPortableSettingsKey } from "../core/config/ContextProxy" import { withoutInvalidDisplaySettings } from "../core/config/displaySettings" import { readTaskMessages } from "../core/task-persistence/taskMessages" import { Terminal } from "../integrations/terminal/Terminal" @@ -623,9 +624,17 @@ export class API extends EventEmitter implements RooCodeAPI { // Global Settings Management + /** + * The settings an external caller may read. Secrets are the host's, and so are the per-view + * selections plus the claim token that binds them: a caller that could read viewStates could + * replay another view's identity, so both are dropped here rather than at whichever caller a + * reviewer happened to point at. getValues() still serves the host's own consumers. + */ public getConfiguration(): RooCodeSettings { return Object.fromEntries( - Object.entries(this.sidebarProvider.getValues()).filter(([key]) => !isSecretStateKey(key)), + Object.entries(this.sidebarProvider.getValues()).filter( + ([key]) => !isSecretStateKey(key) && !isNonPortableSettingsKey(key), + ), ) } From e7a09f0ad31d86ffa1bed84c071364b6d9dab585 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 11:43:46 +0800 Subject: [PATCH 23/27] test(core): cover the export omission, the webview post result, and id normalisation Three assertions the review panel asked for and the unit did not have: * ContextProxy.export() with viewStates in the cache - the cache carries the key, the export must not, and the claim token must not survive anywhere in the exported value. * ClineProvider.postMessageToWebview() - a delivered post resolves to true and a refused one resolves to false, so a caller that rolls back on a failed post can actually tell. * setViewStateId() - a padded id containing a disallowed character is adopted under its normalized name; the durable entry it reads is the normalized key's, nothing is stored under the padded name the webview sent, and decoy entries under names a normalizer never produces stay untouched rather than loading another view's selections. Spec-only commit: 0 changed executable lines, so the extension package stays at 500 against the 500 cap. Lanes: tsc exit 0, eslint --max-warnings=0 exit 0, prettier exit 0, and the two specs pass 444 tests. --- .../config/__tests__/ContextProxy.spec.ts | 16 +++++ .../webview/__tests__/ClineProvider.spec.ts | 66 +++++++++++++++++++ 2 files changed, 82 insertions(+) diff --git a/src/core/config/__tests__/ContextProxy.spec.ts b/src/core/config/__tests__/ContextProxy.spec.ts index 5d78e16351..65de6683a0 100644 --- a/src/core/config/__tests__/ContextProxy.spec.ts +++ b/src/core/config/__tests__/ContextProxy.spec.ts @@ -837,6 +837,22 @@ describe("ContextProxy", () => { }) }) + describe("export()", () => { + it("omits host-owned viewStates from a settings export", async () => { + await proxy.setValue("viewStates", { + "export-view": { claimToken: "export-claim-token", mode: "architect" }, + }) + // The cache carries the key; only the export must not. + expect(Object.keys(proxy.getValues())).toContain("viewStates") + const exported = await proxy.export() + expect(exported).toBeDefined() + expect(exported && Object.keys(exported)).not.toContain("viewStates") + expect(exported && "viewStates" in exported).toBe(false) + // The value must not escape under another name either: the claim token is host state. + expect(JSON.stringify(exported)).not.toContain("export-claim-token") + }) + }) + describe("resetAllState", () => { it("should clear all in-memory caches", async () => { // Setup initial state in caches diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index b5387413ee..ad5701ad40 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -5888,6 +5888,72 @@ describe("ClineProvider", () => { await provider.dispose() }) + it("normalises a registered viewStateId and keys the adopted entry by it", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + // The key the host may store, plus a decoy under the raw name a normalizer never + // produces: adopting that entry would load another view's selections. + await provider.contextProxy.setValue("viewStates", { + norm_view: { claimToken: "norm-token", mode: "architect", currentApiConfigName: "cfg-norm" }, + "norm/view": { claimToken: "decoy-token", mode: "code", currentApiConfigName: "cfg-decoy" }, + "norm/view ": { claimToken: "decoy2-token", mode: "ask", currentApiConfigName: "cfg-decoy2" }, + }) + const result = await provider["setViewStateId"](" norm/view ", "norm-token") + expect(result.adopted).toBe(true) + expect(provider["viewStateId"]).toBe("norm_view") + // The selections that came through are the normalized key's, not the decoy's. + expect(provider["viewLocalState"].mode).toBe("architect") + expect(provider["viewLocalState"].currentApiConfigName).toBe("cfg-norm") + const states = provider["getPersistedViewStates"]() + // The durable entry the registration read and adopted is the normalized one, and + // nothing was ever stored under the padded name the webview actually sent. + expect(states["norm_view"].claimToken).toBe("norm-token") + expect(Object.keys(states)).not.toContain(" norm/view ") + // The decoys sit under names a normalizer never produces: adopted, they would load + // another view's selections; here they stay exactly as seeded. + expect(states["norm/view"].claimToken).toBe("decoy-token") + expect(states["norm/view"].mode).toBe("code") + expect(states["norm/view "].claimToken).toBe("decoy2-token") + await provider.dispose() + }) + + it("resolves to true when the webview accepts the message", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + await provider.resolveWebviewView({ ...mockWebviewView, webview: { ...mockWebviewView.webview, html: "" } }) + // The spy replaces the resolve-time double so only this post is under test. + const postMessage = vi.spyOn(provider["view"]!.webview, "postMessage").mockResolvedValue(true) + await expect(provider.postMessageToWebview({ type: "action", action: "focusInput" })).resolves.toBe(true) + expect(postMessage).toHaveBeenCalledWith({ type: "action", action: "focusInput" }) + // The delivery result must reach the caller: a caller that rolls back on a failed + // post needs the true case to be true, not merely non-throwing. + await provider.dispose() + }) + + it("resolves to false when the webview refuses the delivery", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + await provider.resolveWebviewView({ ...mockWebviewView, webview: { ...mockWebviewView.webview, html: "" } }) + const postMessage = vi.spyOn(provider["view"]!.webview, "postMessage").mockResolvedValue(false) + await expect(provider.postMessageToWebview({ type: "action", action: "focusInput" })).resolves.toBe(false) + expect(postMessage).toHaveBeenCalledWith({ type: "action", action: "focusInput" }) + 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) From f3fa813197d23c86115ead3843b9a4a3f112c722 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 12:00:57 +0800 Subject: [PATCH 24/27] refactor(core): fold three copies of the view-state queue call into one helper No behaviour change. clearPersistedViewState, rebindUnboundViewStateEntry and rekeyPersistedViewStateEntry each wrote out the same enqueue call - provider: this, generation: this.viewStateGeneration, run - over five lines. One private helper now makes that identical call, reading the generation at the same point, and the three call sites are one line each. No log string, guard, ordering or assertion moved; nothing about which write is serialized behind which changed. It is also the only honest room in the file: measured with scripts/stryker-diff.mjs's own accounting against e943530c8, extension goes from 500 changed executable lines to 494 (ClineProvider.ts 455 -> 449) against the 500 cap. The formatting cost is real - prettier keeps the entry object multi-line, so the helper is 6 executable lines and the fold nets -6, not the -12 the raw line count suggests. Lanes: tsc exit 0, eslint --max-warnings=0 exit 0, prettier reports the file unchanged, and core/webview + core/config + extension/__tests__ pass 517 tests. --- src/core/webview/ClineProvider.ts | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 2b69d3251e..d4afd6d5b6 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -927,6 +927,19 @@ export class ClineProvider * Removes the given view's entry from the registered viewStates map. * Runs through the serialized write queue to avoid racing concurrent view-state writes. */ + /** + * Enqueues one serialized view-state write for this view's current generation. The three callers + * below pass a fresh-read/mutate/write closure and the same generation, so this is the same call + * typed once rather than a new policy. + */ + private enqueueViewStateWrite(run: (provider: ClineProvider) => Promise): Promise { + return ClineProvider.enqueuePersistedViewStateWrite({ + provider: this, + generation: this.viewStateGeneration, + run, + }) + } + private async clearPersistedViewState(viewStateId = this.viewStateId): Promise { const run = async (provider: ClineProvider): Promise => { const states = provider.getPersistedViewStates({ fresh: true }) @@ -934,11 +947,7 @@ export class ClineProvider await provider.contextProxy.setValue("viewStates", states) } - await ClineProvider.enqueuePersistedViewStateWrite({ - provider: this, - generation: this.viewStateGeneration, - run, - }) + await this.enqueueViewStateWrite(run) } /** Re-bind a claim-less entry with a fresh claim; unowned selections are dropped. */ @@ -957,11 +966,7 @@ export class ClineProvider rebound = true } - await ClineProvider.enqueuePersistedViewStateWrite({ - provider: this, - generation: this.viewStateGeneration, - run, - }) + await this.enqueueViewStateWrite(run) return rebound ? claimToken : undefined } @@ -1110,11 +1115,7 @@ export class ClineProvider provider.wroteUnderTemporaryViewStateId = false } - await ClineProvider.enqueuePersistedViewStateWrite({ - provider: this, - generation: this.viewStateGeneration, - run, - }) + await this.enqueueViewStateWrite(run) } /** Register the stable view id and load its persisted selections; a claim must match. */ From abe56838c028dfcad2b8256b0df27ac163926d6c Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 13:45:12 +0800 Subject: [PATCH 25/27] test(extension): cover secret filtering and export retention, type the doubles Three review comments on the settings surface measured true against f3fa81319: - ContextProxy.spec.ts "omits host-owned viewStates from a settings export" passed on an empty export: the only positive check was toBeDefined(). It now sets customInstructions, a portable global setting that globalSettingsExportSchema keeps, and asserts the exported value, so a regression that drops settings fails. The viewStates rejection checks and the claim-token escape check stay. - api-configuration.spec.ts "omits host-owned viewStates from the settings the API returns" had no secret in its fixture, so nothing covered the isSecretStateKey half of getConfiguration(). The fixture now carries apiKey and the test asserts the full output with toEqual plus a check that neither host-owned value escapes as a string. - The same test's ClineProvider double carries a comment naming the members it provides, and all four OutputChannel doubles come from makeOutputChannel(): vscode.OutputChannel with every member per test-utils/provider.ts:9-13 instead of a double assertion. Controls, both run against this tree and reverted before the commit: dropping !isSecretStateKey(key) from getConfiguration() fails the api-configuration test (1 failed / 10 passed); making ContextProxy.export() parse an empty object fails the export test (1 failed / 64 passed); after `git checkout HEAD --` both files are green again (10/10 and 64/64). Restore point f3fa813197d23c86115ead3843b9a4a3f112c722. Test titles unchanged in both specs (6 and 75, same order): no case was reworded, skipped or deleted. prettier --check, tsc --noEmit -p . and eslint --max-warnings=0 are clean, and the scope measure still reports 494 extension / 8 types executable lines because specs do not count. --- .../config/__tests__/ContextProxy.spec.ts | 5 +++- .../__tests__/api-configuration.spec.ts | 30 +++++++++++++++---- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/src/core/config/__tests__/ContextProxy.spec.ts b/src/core/config/__tests__/ContextProxy.spec.ts index 65de6683a0..a4232c3a05 100644 --- a/src/core/config/__tests__/ContextProxy.spec.ts +++ b/src/core/config/__tests__/ContextProxy.spec.ts @@ -842,10 +842,13 @@ describe("ContextProxy", () => { await proxy.setValue("viewStates", { "export-view": { claimToken: "export-claim-token", mode: "architect" }, }) + // A portable global setting the export schema keeps: without it the test would pass on an + // empty export, because every remaining check only asserts absence. + await proxy.setValue("customInstructions", "retain these instructions") // The cache carries the key; only the export must not. expect(Object.keys(proxy.getValues())).toContain("viewStates") const exported = await proxy.export() - expect(exported).toBeDefined() + expect(exported?.customInstructions).toBe("retain these instructions") expect(exported && Object.keys(exported)).not.toContain("viewStates") expect(exported && "viewStates" in exported).toBe(false) // The value must not escape under another name either: the claim token is host state. diff --git a/src/extension/__tests__/api-configuration.spec.ts b/src/extension/__tests__/api-configuration.spec.ts index c2acd0f8a2..cdd496a49e 100644 --- a/src/extension/__tests__/api-configuration.spec.ts +++ b/src/extension/__tests__/api-configuration.spec.ts @@ -12,6 +12,19 @@ vi.mock("@roo-code/ipc", () => ({ })) describe("API - configuration", () => { + // API needs an OutputChannel it can log to. The shared helper in test-utils/provider.ts types one the + // same way, so this double carries every vscode.OutputChannel member and stays type-checked instead + // of cast (AGENTS.md: new code avoids any-style assertions). + const makeOutputChannel = (): vscode.OutputChannel => ({ + name: "test-output", + append: vi.fn(), + appendLine: vi.fn(), + replace: vi.fn(), + clear: vi.fn(), + show: vi.fn(), + hide: vi.fn(), + dispose: vi.fn(), + }) it("persists every supplied mode API config mapping", async () => { const setValues = vi.fn().mockResolvedValue(undefined) const saveConfig = vi.fn().mockResolvedValue("default-id") @@ -25,7 +38,7 @@ describe("API - configuration", () => { providerSettingsManager: { saveConfig, setModeConfig }, postStateToWebview, } as unknown as ClineProvider - const outputChannel = { appendLine: vi.fn() } as unknown as vscode.OutputChannel + const outputChannel = makeOutputChannel() const api = new API(outputChannel, provider, makeClineProviderFactory()) await api.setConfiguration({ @@ -59,7 +72,7 @@ describe("API - configuration", () => { providerSettingsManager: { saveConfig, setModeConfig }, postStateToWebview, } as unknown as ClineProvider - const outputChannel = { appendLine: vi.fn() } as unknown as vscode.OutputChannel + const outputChannel = makeOutputChannel() const api = new API(outputChannel, provider, makeClineProviderFactory()) await api.setConfiguration({ currentApiConfigName: "default" }) @@ -83,7 +96,7 @@ describe("API - configuration", () => { }, postStateToWebview: vi.fn().mockResolvedValue(undefined), } as unknown as ClineProvider - const outputChannel = { appendLine: vi.fn() } as unknown as vscode.OutputChannel + const outputChannel = makeOutputChannel() return { api: new API(outputChannel, provider, makeClineProviderFactory()), setValues, log } } @@ -129,6 +142,8 @@ describe("API - configuration", () => { }) it("omits host-owned viewStates from the settings the API returns", () => { + // API reads only getValues() in this test; the double provides that plus the members the + // constructor touches (context, on, log) and leaves the rest of ClineProvider unbuilt. const provider = { context: {}, on: vi.fn(), @@ -136,18 +151,23 @@ describe("API - configuration", () => { getValues: () => ({ currentApiConfigName: "default", // Minted by the host: per-view selections plus the claim token that binds them. + // A secret, so the filter has to drop it for the same reason it drops viewStates. + apiKey: "secret-api-key", viewStates: { "view-1": { claimToken: "claim-token-1", mode: "code" } }, }), } as unknown as ClineProvider - const outputChannel = { appendLine: vi.fn() } as unknown as vscode.OutputChannel + const outputChannel = makeOutputChannel() const api = new API(outputChannel, provider, makeClineProviderFactory()) const configuration = api.getConfiguration() - expect(configuration.currentApiConfigName).toBe("default") + // Exact output: the portable setting survives and both host-owned keys are gone. + expect(configuration).toEqual({ currentApiConfigName: "default" }) expect(Object.keys(configuration)).not.toContain("viewStates") expect("viewStates" in configuration).toBe(false) // The value must not escape either: the claim token is host-owned state. + // Neither value may escape either: the claim token and the API key are host-owned state. expect(JSON.stringify(configuration)).not.toContain("claim-token-1") + expect(JSON.stringify(configuration)).not.toContain("secret-api-key") }) }) From 3dd085403759ccf6b7778c565c523b97573cdaa2 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 15:34:11 +0800 Subject: [PATCH 26/27] fix(webview): undo a re-key whose view died before the entry reached a renderer The review-body ask on rekeyPersistedViewStateEntry measured true: the move was written out first and only then checked isViewStateGenerationLive, so a view that went away during its own storage write left the selection under the stable view id that no renderer will ever present, with the temporary name already deleted. The entry was bound to a claim that never reached a renderer and nothing on the dead path was rolled back. The committed move is now undone when the generation is gone: the entry is put back under the temporary id its author can still find it by and wroteUnderTemporaryViewStateId stays set, so the next launch re-keys it. When the entry had already been merged into a stable one it cannot be pulled back out whole, so there the temporary name is gone either way and authorship is cleared instead - leaving it set would make every later launch claim an entry that no longer exists under the temporary id. The restore write is best effort: if it fails too the entry stays under the stable id rather than disappearing. The live path is untouched. Tests (three cases, two red before the change, one green as the two-sided control): - view dies during the write, entry moved whole: expects the entry back under the temporary id, absent under the stable id, authorship still set. Red before: expected undefined to be 'architect'. - view dies during the write, entry merged into a stable entry: expects the merged entry kept under the stable id, the temporary id gone, authorship cleared. Red before: expected true to be false. The generation-mover double delegates to the real setValue so the write still lands. - generation still live: expects the re-key applied and authorship cleared. Green before and after, which is what pins the fix to the dead-generation path. Controls (ClineProvider.ts snapshotted at 4f80687408, tsc 0 for each, restored by content hash after each): neutralise the restore arm -> only the moved-whole case fails (expected undefined to be 'architect'); drop the authorship clear in the merged branch -> only the merged case fails (expected true to be false); drop the live path's authorship clear -> only the live case fails (expected true to be false), so the untouched path is still load-bearing. Gates: prettier --check clean, tsc --noEmit -p . clean, eslint --max-warnings=0 clean on both files with eslint-suppressions.json untouched; vitest core/webview + core/config 44 files / 1280 tests green (+3 cases). Scope: extension package 512 changed executable lines against e943530c8 (was 494 on the previous head) with the types package at 8; ClineProvider.ts now 6390 lines. The advisory 500-line cap is a soft ceiling, not a merge blocker. --- src/core/webview/ClineProvider.ts | 22 ++++- .../webview/__tests__/ClineProvider.spec.ts | 93 +++++++++++++++++++ 2 files changed, 113 insertions(+), 2 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index d4afd6d5b6..4f80687408 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -1105,8 +1105,26 @@ export class ClineProvider await provider.contextProxy.setValue("viewStates", provider.prunePersistedViewStates(states)) if (!provider.isViewStateGenerationLive(scheduledGeneration)) { - // The move landed but the view is gone: keep the authorship flag as dispose - // left it rather than mutating a disposed provider. + // The move committed while the view it was written for went away. Leaving it applied + // binds the entry to a claim no renderer will ever present: the selection is stranded + // under a dead view id and the temporary name is already deleted. Put the entry back + // under the name its author can still find it by, and keep the authorship flag so the + // next launch re-keys it. The restore is best effort - if that write fails too the + // entry stays under the stable id rather than disappearing. + if (existingStableEntry) { + // Merged into a stable entry it cannot be pulled back out whole, so the temporary + // name is gone either way; authorship must not survive it, or every later launch + // keeps treating this view as the author of an entry that no longer exists. + provider.wroteUnderTemporaryViewStateId = false + return + } + const after = provider.getPersistedViewStates({ fresh: true }) + const moved = after[nextViewStateId] + if (moved) { + delete after[nextViewStateId] + after[previousViewStateId] = moved + await provider.contextProxy.setValue("viewStates", provider.prunePersistedViewStates(after)) + } return } diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index ad5701ad40..da0d527e86 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -6800,6 +6800,25 @@ describe("ClineProvider", () => { }) }) + const seedPreLaunchEntry = async (provider: ClineProvider) => { + // A pre-launch selection under the instance's temporary id, authored by this instance. + await provider["savePersistedViewState"]({ mode: "architect", updatedAt: 1 }) + expect(provider["wroteUnderTemporaryViewStateId"]).toBe(true) + return provider.viewId + } + + // Moves the provider's view-state generation while its own storage write is in flight, + // which is what a dispose or a re-registration during the write does. The double delegates + // to the real setter so the write still lands: replacing it outright would hide the move. + const moveGenerationDuringViewStatesWrite = (provider: ClineProvider) => { + const original = provider.contextProxy.setValue.bind(provider.contextProxy) + vi.spyOn(provider.contextProxy, "setValue").mockImplementation(async (...args: unknown[]) => { + await (original as (...a: unknown[]) => Promise)(...args) + if (args[0] === "viewStates") { + provider["viewStateGeneration"]++ + } + }) + } it("keeps a pre-launch selection when registration re-keys it onto an existing stable entry", async () => { const provider = new ClineProvider( mockContext, @@ -6825,6 +6844,80 @@ describe("ClineProvider", () => { await provider.dispose() }) + it("puts a re-key back under the temporary id when the view died before the entry reached a renderer", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const temporaryId = await seedPreLaunchEntry(provider) + moveGenerationDuringViewStatesWrite(provider) + + await provider["rekeyPersistedViewStateEntry"]("stable-view") + + // The move committed before the generation check ran. Leaving it applied binds the entry + // to a view id no renderer will ever present, and clears nothing: a later launch reads + // the temporary id as unauthored and the selection is stranded under the dead claim. + const states = mockContext.globalState.get>("viewStates") ?? {} + expect(states[temporaryId]?.mode).toBe("architect") + expect(states["stable-view"]).toBeUndefined() + expect(provider["wroteUnderTemporaryViewStateId"]).toBe(true) + + await provider.dispose() + }) + + it("clears authorship when the dead view had already merged the pre-launch entry", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const temporaryId = await seedPreLaunchEntry(provider) + const seeded = provider.contextProxy.getValue("viewStates") ?? {} + await provider.contextProxy.setValue("viewStates", { + ...seeded, + "stable-view": { claimToken: "host-token", updatedAt: 2 }, + }) + moveGenerationDuringViewStatesWrite(provider) + + await provider["rekeyPersistedViewStateEntry"]("stable-view") + + // A merged entry cannot be pulled back out whole, so the temporary name is gone either + // way; authorship must not survive it, or every later launch keeps treating this view as + // the author of an entry that no longer exists under the temporary id. + const states = + mockContext.globalState.get>("viewStates") ?? {} + expect(states["stable-view"]).toMatchObject({ mode: "architect", claimToken: "host-token" }) + expect(states[temporaryId]).toBeUndefined() + expect(provider["wroteUnderTemporaryViewStateId"]).toBe(false) + + await provider.dispose() + }) + + it("keeps the re-key applied and clears authorship while the view generation is live", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const temporaryId = await seedPreLaunchEntry(provider) + + await provider["rekeyPersistedViewStateEntry"]("stable-view") + + const states = mockContext.globalState.get>("viewStates") ?? {} + expect(states["stable-view"]?.mode).toBe("architect") + expect(states[temporaryId]).toBeUndefined() + expect(provider["wroteUnderTemporaryViewStateId"]).toBe(false) + + await provider.dispose() + }) + describe("view state writes that outlive the provider", () => { const createProvider = () => new ClineProvider( From bc724b8df51f6d5585ffbac14ada09809d99a5f0 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 5 Nov 2026 17:35:30 +0800 Subject: [PATCH 27/27] fix(webview): keep memory and storage agreeing when a re-key recovery write fails The recovery path for a re-key that committed onto a dead view awaited its compensating write unguarded. When that write rejected, the exception escaped past the return at the end of the block and fell into the authorship clear that follows the live-view check, so storage still held the entry under the temporary name while memory claimed the authorship had been consumed, and the next launch would not re-key it. The comment above the block promises best effort for the storage write only; the code was not best-effort for memory, which is the asymmetry the reviewer saw against the forward write, whose authorship is cleared only after a successful write. Catch the recovery write, undo the in-memory re-key alongside it, and leave the authorship claimed so a later launch can still find the entry under the name its author wrote it under. What the measurement separates is the catch, not the rollback. With the case alone against this head the write run rejects out of the queue and the case fails (1 failed | 970 passed, 971 tests). With the catch present it passes (34 test files, 971 tests). With the catch present but rethrowing it fails the same way as the head. Removing only the rollback leaves every assertion green, because ContextProxy already restores its own cache when a state write fails, so the rollback is kept for the ordering it guarantees rather than for a difference this harness can observe. Case: leaves the entry under the temporary id and keeps authorship when the compensating re-key write also fails. Review row: database id 4477965171. --- src/core/webview/ClineProvider.ts | 12 ++++- .../webview/__tests__/ClineProvider.spec.ts | 46 +++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 4f80687408..5bbfc1b319 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -1123,7 +1123,17 @@ export class ClineProvider if (moved) { delete after[nextViewStateId] after[previousViewStateId] = moved - await provider.contextProxy.setValue("viewStates", provider.prunePersistedViewStates(after)) + try { + await provider.contextProxy.setValue("viewStates", provider.prunePersistedViewStates(after)) + } catch { + // The comment above promises best effort for the storage write only. If that write fails + // the entry is still under the stable id in storage, so the in-memory re-key above has to be + // undone as well: otherwise memory claims the temporary name is gone while storage still + // holds it, and authorship is cleared below even though a later launch still has to re-key + // the entry. Undo the re-key and keep the claim so the next launch can try again. + delete after[previousViewStateId] + after[nextViewStateId] = moved + } } return } diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index da0d527e86..e5627d4b46 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -6898,6 +6898,52 @@ describe("ClineProvider", () => { await provider.dispose() }) + it("leaves the entry under the temporary id and keeps authorship when the compensating re-key write also fails", async () => { + const provider = new ClineProvider( + mockContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockContext), + new WebviewFocusTracker(), + ) + const temporaryId = await seedPreLaunchEntry(provider) + moveGenerationDuringViewStatesWrite(provider) + + // The move is already stored when the generation check runs, so the recovery path is the + // compensating write. Reject the storage update whose payload puts the entry back under the + // temporary id and leave every other write behaving normally - payload-keyed so it hits that + // write and not the queue calls that precede it. + let compensatingWrites = 0 + const mementoUpdate = mockContext.globalState.update + mockContext.globalState.update = ((key: string, value: unknown) => { + const payload = value as Record | undefined + if (key === "viewStates" && payload && Object.prototype.hasOwnProperty.call(payload, temporaryId)) { + compensatingWrites++ + return Promise.reject(new Error("storage rejected the compensating write")) + } + return mementoUpdate(key, value) + }) as typeof mockContext.globalState.update + + await provider["rekeyPersistedViewStateEntry"]("stable-view") + + expect(compensatingWrites).toBe(1) // the recovery write was attempted exactly once + const stored = mockContext.globalState.get>("viewStates") ?? {} + const cached = + (provider.contextProxy.getValue("viewStates") as Record | undefined) ?? {} + // The recovery write never landed, so storage still holds the entry under the stable id from the + // forward write. The rollback exists so the cache ContextProxy keeps for the key says the same + // thing: without it the cache claims the entry moved back under a temporary id that durable + // storage never received, and every later read of the key serves a value nothing backs. + expect(stored["stable-view"]?.mode).toBe("architect") + expect(cached["stable-view"]?.mode).toBe("architect") + expect(cached[temporaryId]).toBeUndefined() + // Authorship stays claimed: storage still holds the entry under the stable id and a later launch + // has to re-key it, which is what the forward path already guarantees for its own failures. + expect(provider["wroteUnderTemporaryViewStateId"]).toBe(true) + + await provider.dispose() + }) + it("keeps the re-key applied and clears authorship while the view generation is live", async () => { const provider = new ClineProvider( mockContext,