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/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) {