Skip to content

Commit 574dfeb

Browse files
easonliang28easonliang28
committed
fix(webview): make mode-switch failures atomic and scope profile-save task rebuilds
handleModeSwitchUnlocked now reconciles every write it owns (shared mode, view mode pin, task mode, task history) when a failure lands after the task-mode write - including the mode-linked profile activation - so submitUserMessage delivers the submitted message on the pre-switch mode and profile instead of a half-applied switch. Non-activating profile saves no longer force-rebuild the API handler of a view whose active task follows a different sticky profile: both rebuild sites are guarded by a taskApiConfigName match. Adds regression tests for the activation-failure rollback and both rebuild guards. Test hygiene: prune assertions compare full kept entries, the two re-key storage tests seed through the durable write path so their assertions can fail, and the sticky-mode afterEach disposes the per-test provider before resetting the instance registry. Renames the webview state-payload test to describe what it asserts.
1 parent db6299d commit 574dfeb

4 files changed

Lines changed: 307 additions & 108 deletions

File tree

‎src/core/webview/ClineProvider.ts‎

Lines changed: 132 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -2285,40 +2285,26 @@ export class ClineProvider
22852285
// mode, broadcasting ModeChanged, or activating a profile on behalf of a
22862286
// background task would clobber the focused task's selection.
22872287
const viewScopedSwitch = task === undefined || task === null || this.getCurrentTask() === task
2288-
if (viewScopedSwitch) {
2289-
// Pin the view's durable per-view mode first: the pin is this view's own
2290-
// selection and must not be undone if the shared write later fails.
2291-
// If the pin write itself fails, the task-side writes above are already
2292-
// applied: roll them back before rethrowing so the caller cannot deliver
2293-
// the message with the new task mode while the view's mode and mode-linked
2294-
// API configuration stayed old.
2295-
try {
2296-
await this.saveViewState("mode", newMode)
2297-
} catch (error) {
2298-
if (rollbackTaskWrites) {
2299-
try {
2300-
await rollbackTaskWrites()
2301-
} catch (rollbackError) {
2302-
this.log(
2303-
`[handleModeSwitch] Failed to roll back task mode writes after the view mode pin write failed: ${
2304-
rollbackError instanceof Error ? rollbackError.message : String(rollbackError)
2305-
}`,
2306-
)
2307-
}
2308-
}
2309-
throw error
2310-
}
2311-
2312-
// setValue (not the deprecated updateGlobalState) so the in-memory viewLocalState
2313-
// buffer stays in sync with the durable global write: getValues() merges
2314-
// viewLocalState on top of the ContextProxy values, so an unsynced stale
2315-
// restored mode would otherwise shadow the fresh switch for consumers.
2316-
// If the durable write fails, roll the shared write back so getValues()
2317-
// cannot mix a fresh shared mode with the stale pre-switch buffer.
2318-
const previousMode = this.getValue("mode")
2319-
try {
2320-
await this.setValue("mode", newMode)
2321-
} catch (error) {
2288+
// Captured for the reconcile below: the view's pre-switch durable mode pin
2289+
// (undefined means the view never pinned a mode) and the pre-switch shared
2290+
// mode. The pin write only touches the view buffer and the per-view store, so
2291+
// reading the shared value here is unaffected by it.
2292+
const previousViewMode = this.viewLocalState.mode
2293+
const previousMode = this.getValue("mode")
2294+
let pinLanded = false
2295+
let sharedAttempted = false
2296+
2297+
// Explicit partial-failure behavior: every failure after the task-mode write
2298+
// rolls back all writes this switch owns - shared mode, view pin, task mode,
2299+
// and the task-history entry - so the caller (Task#submitUserMessage) delivers
2300+
// the submitted message on the pre-switch mode and profile instead of a
2301+
// half-applied switch that runs the new mode against the unchanged profile.
2302+
// Each restore is awaited on its own and logged on failure, never swallowed.
2303+
const reconcileSwitchWrites = async () => {
2304+
if (sharedAttempted) {
2305+
// The shared write may have landed before its promise rejected: restore
2306+
// it unconditionally so getValues() cannot mix a fresh shared mode with
2307+
// the rolled-back view state.
23222308
try {
23232309
await this.contextProxy.setValue("mode", previousMode)
23242310
} catch (rollbackError) {
@@ -2328,81 +2314,123 @@ export class ClineProvider
23282314
}`,
23292315
)
23302316
}
2331-
this.log(
2332-
`[handleModeSwitch] Failed to persist mode "${newMode}": ${error instanceof Error ? error.message : String(error)}`,
2333-
)
2334-
throw error
23352317
}
2336-
2337-
this.emit(RooCodeEventName.ModeChanged, newMode)
2318+
if (pinLanded) {
2319+
try {
2320+
await this.saveViewState("mode", previousViewMode)
2321+
} catch (rollbackError) {
2322+
this.log(
2323+
`[handleModeSwitch] Failed to roll back the view mode pin after persistence failure: ${
2324+
rollbackError instanceof Error ? rollbackError.message : String(rollbackError)
2325+
}`,
2326+
)
2327+
}
2328+
}
2329+
if (rollbackTaskWrites) {
2330+
try {
2331+
await rollbackTaskWrites()
2332+
} catch (rollbackError) {
2333+
this.log(
2334+
`[handleModeSwitch] Failed to roll back task mode writes after the view mode pin write failed: ${
2335+
rollbackError instanceof Error ? rollbackError.message : String(rollbackError)
2336+
}`,
2337+
)
2338+
}
2339+
}
23382340
}
23392341

2340-
// If workspace lock is on, keep the current API config — don't load mode-specific config
2341-
const lockApiConfigAcrossModes = this.context.workspaceState.get("lockApiConfigAcrossModes", false)
2342-
if (lockApiConfigAcrossModes) {
2343-
// Keep the original post semantics: an explicit null target (pending child)
2344-
// posts its own state.
2345-
if (targetTask !== null) {
2346-
await this.postStateToWebview()
2342+
try {
2343+
if (viewScopedSwitch) {
2344+
// Pin the view's durable per-view mode first: the pin is this view's own
2345+
// selection. If any write below fails, the reconcile above restores every
2346+
// write that landed, including this pin, so a delivered message never runs
2347+
// the new task mode against an unchanged view mode or mode-linked profile.
2348+
await this.saveViewState("mode", newMode)
2349+
pinLanded = true
2350+
2351+
// setValue (not the deprecated updateGlobalState) so the in-memory viewLocalState
2352+
// buffer stays in sync with the durable global write: getValues() merges
2353+
// viewLocalState on top of the ContextProxy values, so an unsynced stale
2354+
// restored mode would otherwise shadow the fresh switch for consumers.
2355+
sharedAttempted = true
2356+
await this.setValue("mode", newMode)
2357+
2358+
this.emit(RooCodeEventName.ModeChanged, newMode)
23472359
}
2348-
return
2349-
}
23502360

2351-
if (signal?.aborted) return
2352-
if (!viewScopedSwitch) {
2353-
return
2354-
}
2361+
// If workspace lock is on, keep the current API config — don't load mode-specific config
2362+
const lockApiConfigAcrossModes = this.context.workspaceState.get("lockApiConfigAcrossModes", false)
2363+
if (lockApiConfigAcrossModes) {
2364+
// Keep the original post semantics: an explicit null target (pending child)
2365+
// posts its own state.
2366+
if (targetTask !== null) {
2367+
await this.postStateToWebview()
2368+
}
2369+
return
2370+
}
23552371

2356-
// Load the saved API config for the new mode if it exists.
2357-
const savedConfigId = await this.providerSettingsManager.getModeConfigId(newMode)
2358-
const listApiConfig = await this.providerSettingsManager.listConfig()
2372+
if (signal?.aborted) return
2373+
if (!viewScopedSwitch) {
2374+
return
2375+
}
23592376

2360-
if (signal?.aborted) return
2377+
// Load the saved API config for the new mode if it exists.
2378+
const savedConfigId = await this.providerSettingsManager.getModeConfigId(newMode)
2379+
const listApiConfig = await this.providerSettingsManager.listConfig()
23612380

2362-
// Update listApiConfigMeta first to ensure UI has latest data.
2363-
await this.updateGlobalState("listApiConfigMeta", listApiConfig)
2381+
if (signal?.aborted) return
23642382

2365-
// If this mode has a saved config, use it.
2366-
if (savedConfigId) {
2367-
const profile = listApiConfig.find(({ id }) => id === savedConfigId)
2383+
// Update listApiConfigMeta first to ensure UI has latest data.
2384+
await this.updateGlobalState("listApiConfigMeta", listApiConfig)
23682385

2369-
if (profile?.name) {
2370-
// Check if the profile has actual API configuration (not just an id).
2371-
// In CLI mode, the ProviderSettingsManager may return empty default profiles
2372-
// that only contain 'id' and 'name' fields. Activating such a profile would
2373-
// overwrite the CLI's working API configuration with empty settings.
2374-
// Skip activation if the profile has no apiProvider set - this indicates
2375-
// an unconfigured/empty profile.
2376-
const fullProfile = await this.providerSettingsManager.getProfile({ name: profile.name })
2377-
const hasActualSettings = !!fullProfile.apiProvider
2378-
2379-
if (hasActualSettings) {
2380-
await this.activateProviderProfileUnlocked(
2381-
{ name: profile.name },
2382-
targetTask === null ? { skipCurrentTaskRebuild: true } : undefined,
2383-
signal,
2384-
)
2386+
// If this mode has a saved config, use it.
2387+
if (savedConfigId) {
2388+
const profile = listApiConfig.find(({ id }) => id === savedConfigId)
2389+
2390+
if (profile?.name) {
2391+
// Check if the profile has actual API configuration (not just an id).
2392+
// In CLI mode, the ProviderSettingsManager may return empty default profiles
2393+
// that only contain 'id' and 'name' fields. Activating such a profile would
2394+
// overwrite the CLI's working API configuration with empty settings.
2395+
// Skip activation if the profile has no apiProvider set - this indicates
2396+
// an unconfigured/empty profile.
2397+
const fullProfile = await this.providerSettingsManager.getProfile({ name: profile.name })
2398+
const hasActualSettings = !!fullProfile.apiProvider
2399+
2400+
if (hasActualSettings) {
2401+
await this.activateProviderProfileUnlocked(
2402+
{ name: profile.name },
2403+
targetTask === null ? { skipCurrentTaskRebuild: true } : undefined,
2404+
signal,
2405+
)
2406+
} else {
2407+
// The task will continue with the current/default configuration.
2408+
}
23852409
} else {
23862410
// The task will continue with the current/default configuration.
23872411
}
23882412
} else {
2389-
// The task will continue with the current/default configuration.
2390-
}
2391-
} else {
2392-
// If no saved config for this mode, save current config as default.
2393-
const currentApiConfigNameAfter = this.getGlobalState("currentApiConfigName")
2413+
// If no saved config for this mode, save current config as default.
2414+
const currentApiConfigNameAfter = this.getGlobalState("currentApiConfigName")
23942415

2395-
if (currentApiConfigNameAfter) {
2396-
const config = listApiConfig.find((c) => c.name === currentApiConfigNameAfter)
2416+
if (currentApiConfigNameAfter) {
2417+
const config = listApiConfig.find((c) => c.name === currentApiConfigNameAfter)
23972418

2398-
if (config?.id) {
2399-
await this.providerSettingsManager.setModeConfig(newMode, config.id)
2419+
if (config?.id) {
2420+
await this.providerSettingsManager.setModeConfig(newMode, config.id)
2421+
}
24002422
}
24012423
}
2402-
}
24032424

2404-
if (targetTask !== null) {
2405-
await this.postStateToWebview()
2425+
if (targetTask !== null) {
2426+
await this.postStateToWebview()
2427+
}
2428+
} catch (error) {
2429+
await reconcileSwitchWrites()
2430+
this.log(
2431+
`[handleModeSwitch] Failed to persist mode "${newMode}": ${error instanceof Error ? error.message : String(error)}`,
2432+
)
2433+
throw error
24062434
}
24072435
}
24082436

@@ -2535,7 +2563,13 @@ export class ClineProvider
25352563
// serving the settings captured before the save under the same name.
25362564
if (this.pinnedProfileName === name) {
25372565
await this._saveViewLocalStateFromMutation({ apiConfiguration: providerSettings })
2538-
this.updateTaskApiHandlerIfNeeded(providerSettings, { forceRebuild: true })
2566+
// Only rebuild when the task actually runs under the saved profile: a
2567+
// view pinned to this profile can hold a task that follows a different
2568+
// sticky profile, and the forced rebuild would silently switch that
2569+
// conversation to the saved profile's settings.
2570+
if (this.getCurrentTask()?.taskApiConfigName === name) {
2571+
this.updateTaskApiHandlerIfNeeded(providerSettings, { forceRebuild: true })
2572+
}
25392573
}
25402574
await this.refreshViewLocalStateForUpdatedProfile(name, providerSettings)
25412575
}
@@ -2882,9 +2916,13 @@ export class ClineProvider
28822916
// The active task of a pinned view holds its own API handler built from
28832917
// the profile's settings: rebuild it with the saved settings or the
28842918
// running conversation keeps sending the old token even though the
2885-
// view's buffer has caught up. Direct private access across sibling
2886-
// instances of the same class is compile-time safe.
2887-
instance.updateTaskApiHandlerIfNeeded(providerSettings, { forceRebuild: true })
2919+
// view's buffer has caught up. Only rebuild when the task actually runs
2920+
// under the saved profile: a task that follows a different sticky
2921+
// profile must keep its own settings. Direct private access across
2922+
// sibling instances of the same class is compile-time safe.
2923+
if (instance.getCurrentTask()?.taskApiConfigName === name) {
2924+
instance.updateTaskApiHandlerIfNeeded(providerSettings, { forceRebuild: true })
2925+
}
28882926
await instance.postStateToWebview()
28892927
}),
28902928
)

‎src/core/webview/__tests__/ClineProvider.spec.ts‎

Lines changed: 99 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1869,8 +1869,8 @@ describe("ClineProvider", () => {
18691869
const pruned = provider["prunePersistedViewStates"](states)
18701870

18711871
expect(Object.keys(pruned)).toHaveLength(50)
1872-
expect(pruned["view-54"]).toBeDefined()
1873-
expect(pruned["view-5"]).toBeDefined()
1872+
expect(pruned["view-54"]).toEqual({ mode: "mode-54", updatedAt: 54 })
1873+
expect(pruned["view-5"]).toEqual({ mode: "mode-5", updatedAt: 5 })
18741874
expect(pruned["view-4"]).toBeUndefined()
18751875

18761876
await provider.dispose()
@@ -8502,6 +8502,7 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => {
85028502
// minimal double without the full provider/services graph.
85038503
const siblingTask = {
85048504
apiConfiguration: { apiProvider: providerIdentifiers.anthropic, apiKey: "old-key" },
8505+
taskApiConfigName: "saved-profile",
85058506
updateApiConfiguration: vi.fn(),
85068507
}
85078508
vi.spyOn(sibling, "getCurrentTask").mockReturnValue(
@@ -8542,6 +8543,102 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => {
85428543
}
85438544
})
85448545

8546+
it("does not rebuild a pinned sibling's task that runs under another profile", async () => {
8547+
// Defensive: the doubles in this suite can leave console writes pending
8548+
// at worker teardown.
8549+
vi.spyOn(console, "log").mockImplementation(() => {})
8550+
vi.spyOn(console, "warn").mockImplementation(() => {})
8551+
vi.spyOn(console, "error").mockImplementation(() => {})
8552+
const sibling = new ClineProvider(
8553+
mockContext,
8554+
mockOutputChannel,
8555+
"editor",
8556+
new ContextProxy(mockContext),
8557+
new WebviewFocusTracker(),
8558+
)
8559+
try {
8560+
// The sibling pins the saved profile, but its active task follows a
8561+
// different sticky profile: the save must refresh the view overlay
8562+
// without switching that conversation to the saved profile's settings.
8563+
sibling["viewLocalState"].currentApiConfigName = "saved-profile"
8564+
sibling["viewLocalState"].apiConfiguration = {
8565+
apiProvider: providerIdentifiers.anthropic,
8566+
apiKey: "old-key",
8567+
}
8568+
vi.spyOn(sibling, "postStateToWebview").mockResolvedValue(undefined)
8569+
8570+
const siblingTask = {
8571+
apiConfiguration: { apiProvider: providerIdentifiers.anthropic, apiKey: "other-key" },
8572+
taskApiConfigName: "other-profile",
8573+
updateApiConfiguration: vi.fn(),
8574+
}
8575+
vi.spyOn(sibling, "getCurrentTask").mockReturnValue(
8576+
siblingTask as unknown as ReturnType<typeof sibling.getCurrentTask>,
8577+
)
8578+
vi.spyOn(provider, "postStateToWebview").mockResolvedValue(undefined)
8579+
// @ts-ignore - Replace providerSettingsManager with a test double.
8580+
provider.providerSettingsManager = {
8581+
saveConfig: vi.fn().mockResolvedValue("saved-id"),
8582+
listConfig: vi.fn().mockResolvedValue([]),
8583+
setModeConfig: vi.fn(),
8584+
}
8585+
8586+
await provider.upsertProviderProfile(
8587+
"saved-profile",
8588+
{ apiProvider: providerIdentifiers.anthropic, apiKey: "new-key" },
8589+
false,
8590+
)
8591+
8592+
// The pinned view's overlay still catches up with the saved settings...
8593+
expect(sibling["viewLocalState"].apiConfiguration).toEqual({
8594+
apiProvider: providerIdentifiers.anthropic,
8595+
apiKey: "new-key",
8596+
})
8597+
// ...but the task runs under another profile: its handler must not be
8598+
// switched to the saved profile's settings.
8599+
expect(siblingTask.updateApiConfiguration).not.toHaveBeenCalled()
8600+
// Let queued view-local persistence settle while the console spies are
8601+
// still installed, so teardown has no pending writes.
8602+
await new Promise((resolve) => setTimeout(resolve, 50))
8603+
} finally {
8604+
await sibling.dispose()
8605+
}
8606+
})
8607+
8608+
it("does not rebuild the acting view's task when it runs under another profile", async () => {
8609+
// The acting view pins the saved profile while its own task follows a
8610+
// different sticky profile: the non-activating save refreshes the view
8611+
// overlay but must leave the task's handler alone.
8612+
const task = {
8613+
apiConfiguration: { apiProvider: providerIdentifiers.anthropic, apiKey: "other-key" },
8614+
taskApiConfigName: "other-profile",
8615+
updateApiConfiguration: vi.fn(),
8616+
}
8617+
vi.spyOn(provider, "getCurrentTask").mockReturnValue(
8618+
task as unknown as ReturnType<typeof provider.getCurrentTask>,
8619+
)
8620+
vi.spyOn(provider, "postStateToWebview").mockResolvedValue(undefined)
8621+
provider["viewLocalState"].currentApiConfigName = "saved-profile"
8622+
// @ts-ignore - Replace providerSettingsManager with a test double.
8623+
provider.providerSettingsManager = {
8624+
saveConfig: vi.fn().mockResolvedValue("saved-id"),
8625+
listConfig: vi.fn().mockResolvedValue([]),
8626+
setModeConfig: vi.fn(),
8627+
}
8628+
8629+
await provider.upsertProviderProfile(
8630+
"saved-profile",
8631+
{ apiProvider: providerIdentifiers.anthropic, apiKey: "new-key" },
8632+
false,
8633+
)
8634+
8635+
expect(provider["viewLocalState"].apiConfiguration).toEqual({
8636+
apiProvider: providerIdentifiers.anthropic,
8637+
apiKey: "new-key",
8638+
})
8639+
expect(task.updateApiConfiguration).not.toHaveBeenCalled()
8640+
})
8641+
85458642
it("does not leak shared provider keys into a pinned view's api configuration", async () => {
85468643
vi.spyOn(console, "log").mockImplementation(() => {})
85478644
vi.spyOn(console, "warn").mockImplementation(() => {})

0 commit comments

Comments
 (0)