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 f5757a51c7..38d4292adb 100644 --- a/packages/types/src/global-settings.ts +++ b/packages/types/src/global-settings.ts @@ -114,6 +114,17 @@ 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 token binding this entry to the webview that wrote it. */ + claimToken: z.string().optional(), +}) + /** * GlobalSettings */ @@ -122,6 +133,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 8a006c0639..52ef624e56 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -113,6 +113,7 @@ export interface ExtensionMessage { | "rooHistoryImportProgress" | "themeFixtureProbeRequest" | "diagnosticsRequest" + | "viewStateClaim" text?: string /** For fileContent: { path, content, error? } */ fileContent?: { path: string; content: string | null; error?: string } @@ -171,6 +172,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 @@ -712,6 +720,9 @@ export interface WebviewMessage { | "themeFixtureProbeResponse" | "diagnosticsResponse" text?: string + viewStateId?: string + /** For `webviewDidLaunch`: the claim delivered for `viewStateId`, enforced by setViewStateId. */ + viewStateClaim?: string taskId?: string editedMessageContent?: string tab?: "settings" | "history" | "mcp" | "modes" | "chat" | "marketplace" | "cloud" 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/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index e53f60fb83..fb26bf9507 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -67,8 +67,21 @@ 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, }) +/** + * 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 @@ -666,7 +679,16 @@ export class ContextProxy { * rollback, must provide it themselves. */ 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 entries = Object.entries(values).filter(([key]) => !isNonPortableSettingsKey(key)) as [ + RooCodeSettingsKey, + unknown, + ][] const results = await Promise.allSettled(entries.map(([key, value]) => this.setValue(key, value))) const committed: string[] = [] diff --git a/src/core/config/ProviderSettingsManager.ts b/src/core/config/ProviderSettingsManager.ts index 01695d7994..07a5fc3d21 100644 --- a/src/core/config/ProviderSettingsManager.ts +++ b/src/core/config/ProviderSettingsManager.ts @@ -55,6 +55,14 @@ export const providerProfilesSchema = z.object({ export type ProviderProfiles = z.infer +/** Already-deleted profile: callers branch on this type instead of matching message text. */ +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 +519,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 +530,11 @@ export class ProviderSettingsManager { await this.store(providerProfiles) }) } catch (error) { + // A missing config is a caller-meaningful signal, not a failure: rethrow it + // unwrapped so callers can branch on the type instead of message text. + if (error instanceof ProviderSettingsNotFoundError) { + throw error + } throw new Error(`Failed to delete config: ${error}`) } } diff --git a/src/core/config/__tests__/ContextProxy.spec.ts b/src/core/config/__tests__/ContextProxy.spec.ts index 5d78e16351..a4232c3a05 100644 --- a/src/core/config/__tests__/ContextProxy.spec.ts +++ b/src/core/config/__tests__/ContextProxy.spec.ts @@ -837,6 +837,25 @@ 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" }, + }) + // 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?.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. + 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/config/__tests__/ProviderSettingsManager.spec.ts b/src/core/config/__tests__/ProviderSettingsManager.spec.ts index a1fc9ac7de..3b93b7cd22 100644 --- a/src/core/config/__tests__/ProviderSettingsManager.spec.ts +++ b/src/core/config/__tests__/ProviderSettingsManager.spec.ts @@ -12,7 +12,7 @@ import { import { clearAllMocks } from "../../../test-utils/reset" import { makeExtensionContext } from "../../../test-utils/vscode" -import { ProviderSettingsManager, ProviderProfiles } from "../ProviderSettingsManager" +import { ProviderSettingsManager, ProviderSettingsNotFoundError, ProviderProfiles } from "../ProviderSettingsManager" // `export()` builds an API handler per profile to read model capabilities. Mock // buildApiHandler with the real @roo-code/types model definitions so the token-field @@ -973,6 +973,11 @@ describe("ProviderSettingsManager", () => { }), ) + // The typed not-found signal is the contract callers branch on: a profile + // name containing "not found" must not be matchable via message text. + await expect(providerSettingsManager.deleteConfig("nonexistent")).rejects.toBeInstanceOf( + ProviderSettingsNotFoundError, + ) await expect(providerSettingsManager.deleteConfig("nonexistent")).rejects.toThrow( "Config 'nonexistent' not found", ) diff --git a/src/core/config/__tests__/importExport.spec.ts b/src/core/config/__tests__/importExport.spec.ts index 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..99f3ef6f6c 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,11 @@ function sanitizeGlobalSettings(rawGlobalSettings: unknown): { continue } + if (isNonPortableSettingsKey(key)) { + warnings.push(`Setting "${path}" was skipped: host-owned view state is not portable.`) + continue + } + let valueToValidate = rawValue if (key === "imageGenerationProvider" && rawValue === retiredProviderIdentifiers.roo) { diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index c56d5291f6..90170d4a27 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -4523,6 +4523,7 @@ describe("Cline", () => { const { ts, text, partial } = message.clineMessage messageUpdates.push({ ts, text, partial }) } + return true }) const task = new Task({ diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 6cec3bdbe7..f94eb1fbec 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -160,6 +160,13 @@ import { REQUESTY_BASE_URL } from "../../shared/utils/requesty" import { validateAndFixToolResultIds } from "../task/validateToolResultIds" import { PendingEditOperationStore, type PendingEditOperationInput } from "./PendingEditOperationStore" +/** + * 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 PersistedViewState = NonNullable[string] + /** * 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 @@ -237,6 +244,46 @@ 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 } + +/** + * 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 +} + +/** 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 @@ -248,6 +295,12 @@ 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 + /** 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, @@ -415,6 +468,74 @@ 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 id for persisted per-view state keys, bound by the host-minted claim. */ + 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 + + /** + * 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() + + /** 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 + * 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 + * when running in parallel (multi-tab) mode. + */ + 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 } + + /** + * 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` + * 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 = "nov-2026-v3.96.0-settings-costs-reliability" // v3.96.0 chat settings, provider costs, and reliability @@ -430,13 +551,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 @@ -599,6 +724,411 @@ 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. 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 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. */ + private isViewStateGenerationLive(generation: number): boolean { + 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) + + // Detached by dispose() while it waited: the view is gone, so its turn + // is skipped. + if (index === -1) return + + ClineProvider.pendingViewStateWrites.splice(index, 1) + + // 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) + } + } + }) + + // 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 + // 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 + // 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 => { + const states = provider.getPersistedViewStates({ fresh: true }) + // Snapshot what this key held before the write, so a write interrupted by disposal can be + 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 !== provider.viewId || provider.wroteUnderTemporaryViewStateId + const current = ownsTargetEntry ? (states[viewStateId] ?? {}) : {} + const next: PersistedViewState = { ...current } + + if ("mode" in values) assignOrClearField(next, "mode", values.mode) + + if ("currentApiConfigName" in values) + assignOrClearField(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 provider.contextProxy.setValue("viewStates", provider.prunePersistedViewStates(states)) + storedEntry = true + + 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) { + assignOrClearField(after, viewStateId, previousEntry) + + 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 === provider.viewId) provider.wroteUnderTemporaryViewStateId = true + } + + await ClineProvider.enqueuePersistedViewStateWrite({ provider: this, generation, run }) + + 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" } + } + + // 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)) { + 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" } + } + + /** + * 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 }) + delete states[viewStateId] + await provider.contextProxy.setValue("viewStates", states) + } + + await this.enqueueViewStateWrite(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 run = async (provider: ClineProvider): Promise => { + 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. + 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 + } + + await this.enqueueViewStateWrite(run) + + return rebound ? claimToken : undefined + } + + /** + * 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. 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._disposed) { + this.log( + `[viewStateClaim] Claim for ${viewStateId} dropped for view ${this.viewId}: the provider is disposed.`, + ) + 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. + return queueUndelivered("not delivered: the webview has not launched yet") + } + + let delivered: boolean + try { + delivered = await this.postMessageToWebview({ + type: "viewStateClaim", + viewStateId, + viewStateClaim: claimToken, + }) + } catch (error) { + const reason = error instanceof Error ? error.message : String(error) + 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. + return queueUndelivered("not delivered: the webview rejected the message") + } + + this.pendingViewStateClaims.delete(viewStateId) + return "delivered" + } + + /** + * 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. + */ + public async deliverPendingViewStateClaims(): Promise { + if (this._disposed || !this.view || !this.isViewLaunched || 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. + */ + 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-key this instance's authored pre-launch entry to the stable id. */ + private async rekeyPersistedViewStateEntry(nextViewStateId: string): Promise { + const previousViewStateId = this.viewId + + const run = async (provider: ClineProvider): Promise => { + // 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 + + const states = provider.getPersistedViewStates({ fresh: true }) + const previous = states[previousViewStateId] + + if (!previous) return + + delete states[previousViewStateId] + + 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 provider.contextProxy.setValue("viewStates", provider.prunePersistedViewStates(states)) + + if (!provider.isViewStateGenerationLive(scheduledGeneration)) { + // 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 + 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 + } + + // 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. + provider.wroteUnderTemporaryViewStateId = false + } + + await this.enqueueViewStateWrite(run) + } + + /** Register the stable view id and load its persisted selections; a claim must match. */ /** * Override EventEmitter's on method to match TaskProviderLike interface */ @@ -951,6 +1481,22 @@ export class ClineProvider } this._disposed = true + // Detach this provider from the shared view-state write queue and retire its generation, + // so a closure that starts after this point drops its effects instead of applying them + // to a disposed provider and holding it retained behind another view's write. + this.viewStateGeneration++ + + 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 = [] this._postStateToWebviewThrottled.cancel() this.stopWebviewWatchdog() this.diagnosticsRecorder.record({ boundary: "provider", phase: "start", type: "dispose" }) @@ -1698,8 +2244,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({ @@ -1719,7 +2271,7 @@ export class ClineProvider elapsedMs: Date.now() - startedAt, taskId, }) - return + return false } try { @@ -1733,6 +2285,8 @@ export class ClineProvider stateSequence: message.type === "state" ? message.state?.clineMessagesSeq : undefined, taskId, }) + + return delivered === true } catch { this.diagnosticsRecorder.record({ boundary: "webview-out", @@ -1742,8 +2296,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 { @@ -1805,10 +2362,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 464889418e..338c52800d 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -2799,6 +2799,156 @@ describe("ClineProvider", () => { expect(mockWebviewPanel.webview.html).toBe(html) }) + test("retires the view-state generation when the provider disposes", async () => { + await provider.resolveWebviewView(mockWebviewPanel) + const generation = provider["viewStateGeneration"] + + await provider.dispose() + + expect(provider["viewStateGeneration"]).toBe(generation + 1) + expect(provider["isViewStateGenerationLive"](generation)).toBe(false) + }) + + test("detaches this provider's queued view-state writes when it disposes", async () => { + await provider.resolveWebviewView(mockWebviewPanel) + const entry: { + provider: ClineProvider | null + generation: number + run: (owner: ClineProvider) => Promise + } = { + provider, + generation: provider["viewStateGeneration"], + run: async () => {}, + } + // The pump advances on the fake clock, so the entry is read while it still waits on the queue. + const queued = ClineProvider["enqueuePersistedViewStateWrite"](entry) + + expect(ClineProvider["pendingViewStateWrites"]).toContain(entry) + expect(provider["scheduledViewStateWrites"]).toContain(entry) + + await provider.dispose() + + expect(ClineProvider["pendingViewStateWrites"]).not.toContain(entry) + expect(provider["scheduledViewStateWrites"]).toEqual([]) + expect(entry.provider).toBeNull() + + void queued + }) + + test("leaves the shared queue untouched when dispose finds the entry already drained", async () => { + await provider.resolveWebviewView(mockWebviewPanel) + const drained: { + provider: ClineProvider | null + generation: number + run: (owner: ClineProvider) => Promise + } = { + provider, + generation: provider["viewStateGeneration"], + run: async () => {}, + } + const queuedDrained = ClineProvider["enqueuePersistedViewStateWrite"](drained) + // The queue step drops the entry from the shared list at its own turn before it asks + // whether the owner's generation is still live, so a write whose owner went stale + // stays on this provider's schedule while it is already gone from the shared list. + // That is the state below, built the way the queue step builds it. + const drainedIndex = ClineProvider["pendingViewStateWrites"].indexOf(drained) + ClineProvider["pendingViewStateWrites"].splice(drainedIndex, 1) + // A write belonging to somebody else, left on the shared queue so that a splice + // run without a match would remove it. + const neighbour: { + provider: ClineProvider | null + generation: number + run: (owner: ClineProvider) => Promise + } = { + provider: null, + generation: 0, + run: async () => {}, + } + ClineProvider["pendingViewStateWrites"].push(neighbour) + const sharedBefore = ClineProvider["pendingViewStateWrites"].slice() + + await provider.dispose() + + expect(ClineProvider["pendingViewStateWrites"]).toEqual(sharedBefore) + expect(ClineProvider["pendingViewStateWrites"]).toContain(neighbour) + expect(provider["scheduledViewStateWrites"]).toEqual([]) + expect(drained.provider).toBeNull() + expect(ClineProvider["pendingViewStateWrites"]).toHaveLength(sharedBefore.length) + + // The neighbour belongs to no provider, so this dispose leaves it on the shared + // queue; take it back out rather than hand it to the next test. + const neighbourIndex = ClineProvider["pendingViewStateWrites"].indexOf(neighbour) + if (neighbourIndex !== -1) { + ClineProvider["pendingViewStateWrites"].splice(neighbourIndex, 1) + } + + void queuedDrained + }) + + test("splices the provider entry out of the shared queue without touching the entry behind it", async () => { + await provider.resolveWebviewView(mockWebviewPanel) + const entry: { + provider: ClineProvider | null + generation: number + run: (owner: ClineProvider) => Promise + } = { + provider, + generation: provider["viewStateGeneration"], + run: async () => {}, + } + const queued = ClineProvider["enqueuePersistedViewStateWrite"](entry) + // A write from another provider that is already waiting behind this one. + const behind: { + provider: ClineProvider | null + generation: number + run: (owner: ClineProvider) => Promise + } = { + provider: null, + generation: 0, + run: async () => {}, + } + ClineProvider["pendingViewStateWrites"].push(behind) + + const entryIndex = ClineProvider["pendingViewStateWrites"].indexOf(entry) + expect(ClineProvider["pendingViewStateWrites"].indexOf(behind)).toBe(entryIndex + 1) + + await provider.dispose() + + expect(ClineProvider["pendingViewStateWrites"]).not.toContain(entry) + expect(ClineProvider["pendingViewStateWrites"]).toContain(behind) + expect(provider["scheduledViewStateWrites"]).toEqual([]) + expect(entry.provider).toBeNull() + + const behindIndex = ClineProvider["pendingViewStateWrites"].indexOf(behind) + if (behindIndex !== -1) { + ClineProvider["pendingViewStateWrites"].splice(behindIndex, 1) + } + + void queued + }) + + test("numbers this provider's view id from the shared counter", () => { + const suffix = Number(String(provider["viewId"]).split("-").pop()) + + expect(Number.isNaN(suffix)).toBe(false) + expect(ClineProvider["nextViewId"]).toBeGreaterThan(suffix) + }) + + test("keeps the per-field view-state write versions the record gates on", () => { + expect(provider["viewLocalStateWriteVersions"]).toEqual({ + mode: 0, + currentApiConfigName: 0, + apiConfiguration: 0, + }) + }) + + test("reports a message as undelivered once the provider is disposed", async () => { + await provider.resolveWebviewView(mockWebviewPanel) + await provider.dispose() + + expect(await provider.postMessageToWebview({ type: "mcpServers", mcpServers: [] })).toBe(false) + }) + test.each([false, true])( "replaces the tab host without disposing its provider or task (interrupted: %s)", async (interrupted) => { @@ -3229,8 +3379,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 +3466,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 +3482,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 +3491,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 +3517,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 +3527,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 +3547,7 @@ describe("ClineProvider", () => { expect(postMessageSpy).toHaveBeenCalledOnce() expect(statePostSettled).toBe(false) - releasePost() + releasePost(true) await statePost expect(statePostSettled).toBe(true) }) @@ -3423,7 +3574,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 +3601,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) @@ -5307,7 +5458,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", @@ -8164,7 +8314,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([]), } @@ -8268,7 +8418,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/core/webview/__tests__/webviewMessageHandler.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index 98c306a4f3..0ea700671b 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -129,6 +129,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(), @@ -2832,6 +2834,43 @@ 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([]) + // Establish the false value inside this row: every beforeEach in this file runs + // clearAllMocks, which resets call records and not properties, so a value an earlier row + // left set could make the ordering pass on its own. + mockClineProvider.isViewLaunched = false + const launchedWhenFlushed: boolean[] = [] + vi.mocked(mockClineProvider.deliverPendingViewStateClaims).mockImplementationOnce(async () => { + launchedWhenFlushed.push(mockClineProvider.isViewLaunched) + }) + + await webviewMessageHandler(mockClineProvider, { type: "webviewDidLaunch" }) + + expect(mockClineProvider.deliverPendingViewStateClaims).toHaveBeenCalledTimes(1) + expect(launchedWhenFlushed).toEqual([true]) + }) + + it("contains a rejected claim flush so webviewDidLaunch still completes", async () => { + vi.mocked(mockClineProvider.customModesManager.getCustomModes).mockResolvedValue([]) + vi.mocked(mockClineProvider.deliverPendingViewStateClaims).mockImplementationOnce(() => { + throw new Error("Claim flush failed") + }) + + await expect(webviewMessageHandler(mockClineProvider, { type: "webviewDidLaunch" })).resolves.toBeUndefined() + + expect(mockClineProvider.deliverPendingViewStateClaims).toHaveBeenCalledTimes(1) + + expect(mockClineProvider.log).toHaveBeenCalledWith( + expect.stringContaining("Flushing pending claims on launch failed"), + ) + }) + // 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 @@ -3137,8 +3176,35 @@ describe("webviewMessageHandler - renameTaskWithId", () => { }) }) -// The display settings are written like any other setting, but the webview is not trusted: a value that its own -// controls cannot produce (a hand-written message) is ignored before anything is done for it. +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"), + ) + }) +}) + +// The display settings are written like any other setting, but the webview is not trusted: a +// value its own controls cannot produce (a hand-written message) is ignored before storage. describe("updateSettings with the display settings", () => { const save = (updatedSettings: Record) => webviewMessageHandler(mockClineProvider, { type: "updateSettings", updatedSettings }) diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index c5abbc0a12..e5e1c33f11 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -41,6 +41,7 @@ import { isInvalidDisplayValue } from "../config/displaySettings" 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" @@ -701,6 +702,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. @@ -774,6 +786,18 @@ 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 + } + // An invalid display value is ignored, before anything is done for it. if (isInvalidDisplayValue(key, value)) { provider.log(`[updateSettings] Ignored an invalid value for ${key}`) diff --git a/src/extension/__tests__/api-configuration.spec.ts b/src/extension/__tests__/api-configuration.spec.ts index a10bfe60ac..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 } } @@ -127,4 +140,34 @@ describe("API - configuration", () => { expect(log).toHaveBeenCalledWith(`[setConfiguration] Ignored an invalid value for ${key}`) }) }) + + 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(), + log: vi.fn(), + 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 = makeOutputChannel() + const api = new API(outputChannel, provider, makeClineProviderFactory()) + + const configuration = api.getConfiguration() + + // 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") + }) }) 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/extension/api.ts b/src/extension/api.ts index 606bb7725d..de04787b17 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, TaskMessagesReadError } from "../core/task-persistence/taskMessages" import { getErrorCode } from "../core/task-persistence/readFileWithMissingRetry" @@ -635,9 +636,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), + ), ) } 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 {