Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions packages/types/src/__tests__/global-settings.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 })
Expand Down
5 changes: 5 additions & 0 deletions packages/types/src/__tests__/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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")
})
Expand All @@ -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", () => {
Expand Down
12 changes: 12 additions & 0 deletions packages/types/src/global-settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand All @@ -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(),
Expand Down
11 changes: 11 additions & 0 deletions packages/types/src/vscode-extension-host.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down Expand Up @@ -171,6 +172,13 @@ export interface ExtensionMessage {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
values?: Record<string, any>
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
Expand Down Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion src/__tests__/extension.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
24 changes: 23 additions & 1 deletion src/core/config/ContextProxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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[] = []
Expand Down
15 changes: 14 additions & 1 deletion src/core/config/ProviderSettingsManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,14 @@

export type ProviderProfiles = z.infer<typeof providerProfilesSchema>

/** 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"

Check warning on line 62 in src/core/config/ProviderSettingsManager.ts

View workflow job for this annotation

GitHub Actions / mutation-diff

Mutation test advisory

src/core/config/ProviderSettingsManager.ts:62: Survived StringLiteral mutant (replacement: ""). See the job summary for the complete list and resolution guidance.

Check warning on line 62 in src/core/config/ProviderSettingsManager.ts

View workflow job for this annotation

GitHub Actions / mutation-diff

Mutation test advisory

src/core/config/ProviderSettingsManager.ts:62: Survived StringLiteral mutant (replacement: ""). See the job summary for the complete list and resolution guidance.
}
}

export class ProviderSettingsManager {
private static readonly SCOPE_PREFIX = "roo_cline_config_"
private readonly defaultConfigId = this.generateId()
Expand Down Expand Up @@ -511,7 +519,7 @@
const providerProfiles = await this.load()

if (!providerProfiles.apiConfigs[name]) {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
throw new Error(`Config '${name}' not found`)
throw new ProviderSettingsNotFoundError(`Config '${name}' not found`)
}

if (Object.keys(providerProfiles.apiConfigs).length === 1) {
Expand All @@ -522,6 +530,11 @@
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}`)
}
}
Expand Down
19 changes: 19 additions & 0 deletions src/core/config/__tests__/ContextProxy.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 6 additions & 1 deletion src/core/config/__tests__/ProviderSettingsManager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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",
)
Expand Down
49 changes: 49 additions & 0 deletions src/core/config/__tests__/importExport.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
7 changes: 6 additions & 1 deletion src/core/config/importExport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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) {
Expand Down
1 change: 1 addition & 0 deletions src/core/task/__tests__/Task.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4523,6 +4523,7 @@ describe("Cline", () => {
const { ts, text, partial } = message.clineMessage
messageUpdates.push({ ts, text, partial })
}
return true
})

const task = new Task({
Expand Down
Loading
Loading