Skip to content

Commit 00fc247

Browse files
taltastaltas
authored
Handle partial settings import failures per key (#401)
1 parent 5bf98fc commit 00fc247

4 files changed

Lines changed: 337 additions & 8 deletions

File tree

‎src/core/config/__tests__/importExport.spec.ts‎

Lines changed: 215 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -757,7 +757,7 @@ describe("importExport", () => {
757757

758758
// Should show warning message with short summary (not full details)
759759
expect(showWarningMessageSpy).toHaveBeenCalledWith(
760-
expect.stringContaining("1 profile had issues during import."),
760+
expect.stringContaining("1 item had issues during import."),
761761
)
762762
expect(showWarningMessageSpy).toHaveBeenCalledWith(
763763
expect.stringContaining("See Developer Tools console for details."),
@@ -1099,7 +1099,7 @@ describe("importExport", () => {
10991099

11001100
// Should show warning message with plural summary for multiple warnings
11011101
expect(showWarningMessageSpy).toHaveBeenCalledWith(
1102-
expect.stringContaining("2 profiles had issues during import."),
1102+
expect.stringContaining("2 items had issues during import."),
11031103
)
11041104
// Should log full details to console
11051105
expect(consoleWarnSpy).toHaveBeenCalledWith(
@@ -1113,6 +1113,219 @@ describe("importExport", () => {
11131113
showWarningMessageSpy.mockRestore()
11141114
consoleWarnSpy.mockRestore()
11151115
})
1116+
1117+
it("should normalize imageGenerationProvider roo while preserving other global settings", async () => {
1118+
;(vscode.window.showOpenDialog as Mock).mockResolvedValue([{ fsPath: "/mock/path/settings.json" }])
1119+
1120+
const mockFileContent = JSON.stringify({
1121+
providerProfiles: {
1122+
currentApiConfigName: "valid-profile",
1123+
apiConfigs: {
1124+
"valid-profile": {
1125+
apiProvider: "openai" as ProviderName,
1126+
apiKey: "test-key",
1127+
id: "valid-id",
1128+
},
1129+
},
1130+
},
1131+
globalSettings: {
1132+
imageGenerationProvider: "roo",
1133+
openRouterImageGenerationSelectedModel: "openrouter/model-1",
1134+
customInstructions: "Keep this setting",
1135+
},
1136+
})
1137+
1138+
;(fs.readFile as Mock).mockResolvedValue(mockFileContent)
1139+
mockProviderSettingsManager.export.mockResolvedValue({
1140+
currentApiConfigName: "default",
1141+
apiConfigs: { default: { apiProvider: "anthropic" as ProviderName, id: "default-id" } },
1142+
})
1143+
mockProviderSettingsManager.listConfig.mockResolvedValue([
1144+
{ name: "valid-profile", id: "valid-id", apiProvider: "openai" as ProviderName },
1145+
])
1146+
1147+
const result = await importSettings({
1148+
providerSettingsManager: mockProviderSettingsManager,
1149+
contextProxy: mockContextProxy,
1150+
customModesManager: mockCustomModesManager,
1151+
})
1152+
1153+
expect(result.success).toBe(true)
1154+
expect((result as { warnings?: string[] }).warnings).toEqual(
1155+
expect.arrayContaining([
1156+
expect.stringContaining("globalSettings.imageGenerationProvider"),
1157+
expect.stringContaining('unsupported value "roo"'),
1158+
]),
1159+
)
1160+
1161+
const importedGlobalSettings = mockContextProxy.setValues.mock.calls[0][0]
1162+
expect(importedGlobalSettings).toHaveProperty("imageGenerationProvider", undefined)
1163+
expect(importedGlobalSettings.openRouterImageGenerationSelectedModel).toBe("openrouter/model-1")
1164+
expect(importedGlobalSettings.customInstructions).toBe("Keep this setting")
1165+
})
1166+
1167+
it("should partially import valid global settings when invalid top-level keys are present", async () => {
1168+
;(vscode.window.showOpenDialog as Mock).mockResolvedValue([{ fsPath: "/mock/path/settings.json" }])
1169+
1170+
const mockFileContent = JSON.stringify({
1171+
providerProfiles: {
1172+
currentApiConfigName: "valid-profile",
1173+
apiConfigs: {
1174+
"valid-profile": {
1175+
apiProvider: "openai" as ProviderName,
1176+
apiKey: "test-key",
1177+
id: "valid-id",
1178+
},
1179+
},
1180+
},
1181+
globalSettings: {
1182+
customInstructions: "Keep this setting",
1183+
autoApprovalEnabled: true,
1184+
requestDelaySeconds: "slow",
1185+
telemetrySetting: "maybe",
1186+
},
1187+
})
1188+
1189+
;(fs.readFile as Mock).mockResolvedValue(mockFileContent)
1190+
mockProviderSettingsManager.export.mockResolvedValue({
1191+
currentApiConfigName: "default",
1192+
apiConfigs: { default: { apiProvider: "anthropic" as ProviderName, id: "default-id" } },
1193+
})
1194+
mockProviderSettingsManager.listConfig.mockResolvedValue([
1195+
{ name: "valid-profile", id: "valid-id", apiProvider: "openai" as ProviderName },
1196+
])
1197+
1198+
const result = await importSettings({
1199+
providerSettingsManager: mockProviderSettingsManager,
1200+
contextProxy: mockContextProxy,
1201+
customModesManager: mockCustomModesManager,
1202+
})
1203+
1204+
expect(result.success).toBe(true)
1205+
expect((result as { warnings?: string[] }).warnings).toEqual(
1206+
expect.arrayContaining([
1207+
expect.stringContaining("globalSettings.requestDelaySeconds"),
1208+
expect.stringContaining("globalSettings.telemetrySetting"),
1209+
]),
1210+
)
1211+
1212+
const importedGlobalSettings = mockContextProxy.setValues.mock.calls[0][0]
1213+
expect(importedGlobalSettings).toEqual({
1214+
customInstructions: "Keep this setting",
1215+
autoApprovalEnabled: true,
1216+
})
1217+
})
1218+
1219+
it("should skip invalid customModes without aborting unrelated settings import", async () => {
1220+
;(vscode.window.showOpenDialog as Mock).mockResolvedValue([{ fsPath: "/mock/path/settings.json" }])
1221+
1222+
const mockFileContent = JSON.stringify({
1223+
providerProfiles: {
1224+
currentApiConfigName: "valid-profile",
1225+
apiConfigs: {
1226+
"valid-profile": {
1227+
apiProvider: "openai" as ProviderName,
1228+
apiKey: "test-key",
1229+
id: "valid-id",
1230+
},
1231+
},
1232+
},
1233+
globalSettings: {
1234+
customInstructions: "Keep this setting",
1235+
customModes: [
1236+
{
1237+
slug: "broken-mode",
1238+
name: "",
1239+
roleDefinition: "",
1240+
groups: ["invalid-group"],
1241+
},
1242+
],
1243+
},
1244+
})
1245+
1246+
;(fs.readFile as Mock).mockResolvedValue(mockFileContent)
1247+
mockProviderSettingsManager.export.mockResolvedValue({
1248+
currentApiConfigName: "default",
1249+
apiConfigs: { default: { apiProvider: "anthropic" as ProviderName, id: "default-id" } },
1250+
})
1251+
mockProviderSettingsManager.listConfig.mockResolvedValue([
1252+
{ name: "valid-profile", id: "valid-id", apiProvider: "openai" as ProviderName },
1253+
])
1254+
1255+
const result = await importSettings({
1256+
providerSettingsManager: mockProviderSettingsManager,
1257+
contextProxy: mockContextProxy,
1258+
customModesManager: mockCustomModesManager,
1259+
})
1260+
1261+
expect(result.success).toBe(true)
1262+
expect((result as { warnings?: string[] }).warnings).toEqual(
1263+
expect.arrayContaining([expect.stringContaining("globalSettings.customModes")]),
1264+
)
1265+
expect(mockCustomModesManager.updateCustomMode).not.toHaveBeenCalled()
1266+
expect(mockContextProxy.setValues).toHaveBeenCalledWith({
1267+
customInstructions: "Keep this setting",
1268+
})
1269+
})
1270+
1271+
it("should use generic warning wording when only global settings have issues", async () => {
1272+
const filePath = "/mock/path/settings.json"
1273+
const mockFileContent = JSON.stringify({
1274+
providerProfiles: {
1275+
currentApiConfigName: "valid-profile",
1276+
apiConfigs: {
1277+
"valid-profile": {
1278+
apiProvider: "openai" as ProviderName,
1279+
apiKey: "test-key",
1280+
id: "valid-id",
1281+
},
1282+
},
1283+
},
1284+
globalSettings: {
1285+
requestDelaySeconds: "slow",
1286+
},
1287+
})
1288+
1289+
;(fs.readFile as Mock).mockResolvedValue(mockFileContent)
1290+
;(fs.access as Mock).mockResolvedValue(undefined)
1291+
mockProviderSettingsManager.export.mockResolvedValue({
1292+
currentApiConfigName: "default",
1293+
apiConfigs: { default: { apiProvider: "anthropic" as ProviderName, id: "default-id" } },
1294+
})
1295+
mockProviderSettingsManager.listConfig.mockResolvedValue([
1296+
{ name: "valid-profile", id: "valid-id", apiProvider: "openai" as ProviderName },
1297+
])
1298+
1299+
const mockProvider = {
1300+
settingsImportedAt: 0,
1301+
postStateToWebview: vi.fn().mockResolvedValue(undefined),
1302+
}
1303+
1304+
const showWarningMessageSpy = vi.spyOn(vscode.window, "showWarningMessage").mockResolvedValue(undefined)
1305+
const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {})
1306+
1307+
await importSettingsWithFeedback(
1308+
{
1309+
providerSettingsManager: mockProviderSettingsManager,
1310+
contextProxy: mockContextProxy,
1311+
customModesManager: mockCustomModesManager,
1312+
provider: mockProvider,
1313+
},
1314+
filePath,
1315+
)
1316+
1317+
expect(showWarningMessageSpy).toHaveBeenCalledWith(
1318+
expect.stringContaining("1 item had issues during import."),
1319+
)
1320+
expect(showWarningMessageSpy).not.toHaveBeenCalledWith(expect.stringContaining("profile had issues"))
1321+
expect(consoleWarnSpy).toHaveBeenCalledWith(
1322+
"Settings import completed with warnings:",
1323+
expect.arrayContaining([expect.stringContaining("globalSettings.requestDelaySeconds")]),
1324+
)
1325+
1326+
showWarningMessageSpy.mockRestore()
1327+
consoleWarnSpy.mockRestore()
1328+
})
11161329
})
11171330
})
11181331

‎src/core/config/importExport.ts‎

Lines changed: 64 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import {
1010
globalSettingsSchema,
1111
providerSettingsWithIdSchema,
1212
isProviderName,
13+
type GlobalSettings,
1314
type ProviderSettingsWithId,
1415
} from "@roo-code/types"
1516
import { TelemetryService } from "@roo-code/telemetry"
@@ -70,6 +71,57 @@ function sanitizeProviderConfig(configName: string, apiConfig: unknown): { confi
7071
return { config: apiConfig }
7172
}
7273

74+
const globalSettingsShape = globalSettingsSchema.shape as Record<keyof GlobalSettings, z.ZodTypeAny>
75+
76+
function formatZodIssues(error: ZodError): string {
77+
return error.issues.map((issue) => `[${issue.path.join(".") || "value"}]: ${issue.message}`).join(", ")
78+
}
79+
80+
function sanitizeGlobalSettings(rawGlobalSettings: unknown): {
81+
sanitizedGlobalSettings: GlobalSettings
82+
warnings: string[]
83+
} {
84+
const warnings: string[] = []
85+
const sanitizedGlobalSettings: Record<string, unknown> = {}
86+
87+
if (typeof rawGlobalSettings === "undefined") {
88+
return { sanitizedGlobalSettings: sanitizedGlobalSettings as GlobalSettings, warnings }
89+
}
90+
91+
if (typeof rawGlobalSettings !== "object" || rawGlobalSettings === null || Array.isArray(rawGlobalSettings)) {
92+
warnings.push(
93+
`Setting "globalSettings" was skipped: Expected object, received ${Array.isArray(rawGlobalSettings) ? "array" : typeof rawGlobalSettings}.`,
94+
)
95+
return { sanitizedGlobalSettings: sanitizedGlobalSettings as GlobalSettings, warnings }
96+
}
97+
98+
for (const [key, rawValue] of Object.entries(rawGlobalSettings)) {
99+
const path = `globalSettings.${key}`
100+
const schema = globalSettingsShape[key as keyof GlobalSettings]
101+
102+
if (!schema) {
103+
warnings.push(`Setting "${path}" was skipped: Unknown setting.`)
104+
continue
105+
}
106+
107+
let valueToValidate = rawValue
108+
109+
if (key === "imageGenerationProvider" && rawValue === "roo") {
110+
warnings.push(`Setting "${path}" used unsupported value "roo" and was cleared during import.`)
111+
valueToValidate = undefined
112+
}
113+
114+
const result = schema.safeParse(valueToValidate)
115+
if (result.success) {
116+
sanitizedGlobalSettings[key] = result.data
117+
} else {
118+
warnings.push(`Setting "${path}" was skipped: ${formatZodIssues(result.error)}`)
119+
}
120+
}
121+
122+
return { sanitizedGlobalSettings: sanitizedGlobalSettings as GlobalSettings, warnings }
123+
}
124+
73125
/**
74126
* Imports configuration from a specific file path
75127
* Shares base functionality for import settings for both the manual
@@ -91,14 +143,15 @@ export async function importSettingsFromPath(
91143

92144
const lenientSchema = z.object({
93145
providerProfiles: lenientProviderProfilesSchema,
94-
globalSettings: globalSettingsSchema.optional(),
146+
globalSettings: z.unknown().optional(),
95147
})
96148

97149
try {
98150
const previousProviderProfiles = await providerSettingsManager.export()
99151

100152
const rawData = JSON.parse(await fs.readFile(filePath, "utf-8"))
101-
const { providerProfiles: rawProviderProfiles, globalSettings = {} } = lenientSchema.parse(rawData)
153+
const { providerProfiles: rawProviderProfiles, globalSettings: rawGlobalSettings } =
154+
lenientSchema.parse(rawData)
102155

103156
// Track warnings for profiles that had issues
104157
const warnings: string[] = []
@@ -161,15 +214,20 @@ export async function importSettingsFromPath(
161214
},
162215
}
163216

217+
const { sanitizedGlobalSettings, warnings: globalSettingsWarnings } = sanitizeGlobalSettings(rawGlobalSettings)
218+
warnings.push(...globalSettingsWarnings)
219+
164220
await Promise.all(
165-
(globalSettings.customModes ?? []).map((mode) => customModesManager.updateCustomMode(mode.slug, mode)),
221+
(sanitizedGlobalSettings.customModes ?? []).map((mode) =>
222+
customModesManager.updateCustomMode(mode.slug, mode),
223+
),
166224
)
167225

168226
// OpenAI Compatible settings are now correctly stored in codebaseIndexConfig
169227
// They will be imported automatically with the config - no special handling needed
170228

171229
await providerSettingsManager.import(providerProfiles)
172-
await contextProxy.setValues(globalSettings)
230+
await contextProxy.setValues(sanitizedGlobalSettings)
173231

174232
// Set the current provider.
175233
const currentProviderName = providerProfiles.currentApiConfigName
@@ -187,7 +245,7 @@ export async function importSettingsFromPath(
187245

188246
return {
189247
providerProfiles,
190-
globalSettings,
248+
globalSettings: sanitizedGlobalSettings,
191249
success: true,
192250
warnings: warnings.length > 0 ? warnings : undefined,
193251
}
@@ -338,7 +396,7 @@ export const importSettingsWithFeedback = async (
338396
// Show a short summary in the toast notification
339397
const count = warnings.length
340398
const summary =
341-
count === 1 ? `1 profile had issues during import.` : `${count} profiles had issues during import.`
399+
count === 1 ? `1 item had issues during import.` : `${count} items had issues during import.`
342400
await vscode.window.showWarningMessage(
343401
`${t("common:info.settings_imported")} ${summary} See Developer Tools console for details.`,
344402
)

0 commit comments

Comments
 (0)