Skip to content

Commit 6566073

Browse files
allquixoticclaude
allquixotic
and
claude
committed
fix(concurrency): scope background mode switches to the invoking task
SwitchModeTool and RunSlashCommandTool called provider.handleModeSwitch() via a fallback path when task.switchTaskMode existed but the method itself never resolved the mode-bound provider profile for background conversations, so a background task's mode switch silently kept the wrong API profile. Task.switchTaskMode now mirrors delegateParentAndOpenChild's task-scoped profile resolution for the non-visible path, and both tools call it unconditionally instead of falling back to the global handler. chore: bump version to 4.99.2 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent fb664d9 commit 6566073

7 files changed

Lines changed: 313 additions & 24 deletions

File tree

‎src/__tests__/task-mode-switch.spec.ts‎

Lines changed: 224 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,224 @@
1+
// npx vitest run __tests__/task-mode-switch.spec.ts
2+
3+
import { describe, it, expect, vi } from "vitest"
4+
import type { HistoryItem } from "@roo-code/types"
5+
import { RooCodeEventName } from "@roo-code/types"
6+
import { Task } from "../core/task/Task"
7+
8+
const taskHistoryItem: HistoryItem = {
9+
id: "task-1",
10+
task: "Background work",
11+
tokensIn: 0,
12+
tokensOut: 0,
13+
totalCost: 0,
14+
} as unknown as HistoryItem
15+
16+
/** Task-scoped providerSettingsManager stub; by default no mode-bound profile resolves. */
17+
const makeProviderSettingsManager = (
18+
overrides: Partial<{
19+
getModeConfigId: ReturnType<typeof vi.fn>
20+
listConfig: ReturnType<typeof vi.fn>
21+
getProfile: ReturnType<typeof vi.fn>
22+
setModeConfig: ReturnType<typeof vi.fn>
23+
}> = {},
24+
) => ({
25+
getModeConfigId: vi.fn().mockResolvedValue(undefined),
26+
listConfig: vi.fn().mockResolvedValue([]),
27+
getProfile: vi.fn(),
28+
setModeConfig: vi.fn(),
29+
...overrides,
30+
})
31+
32+
/** Provider double with everything switchTaskMode touches. */
33+
function makeProvider(overrides: Record<string, unknown> = {}) {
34+
return {
35+
isTaskVisible: vi.fn().mockReturnValue(false),
36+
handleModeSwitch: vi.fn().mockResolvedValue(undefined),
37+
updateTaskHistory: vi.fn().mockResolvedValue(undefined),
38+
taskHistoryStore: { get: vi.fn().mockReturnValue(taskHistoryItem) },
39+
context: { workspaceState: { get: vi.fn().mockReturnValue(false) } },
40+
providerSettingsManager: makeProviderSettingsManager(),
41+
log: vi.fn(),
42+
...overrides,
43+
}
44+
}
45+
46+
/**
47+
* Task double with the methods/fields switchTaskMode reads from `this`.
48+
* The real prototype method is invoked with this double, mirroring the
49+
* provider-double pattern in provider-delegation.spec.ts.
50+
*/
51+
function makeTask(provider: unknown) {
52+
return {
53+
taskId: "task-1",
54+
providerRef: { deref: vi.fn(() => provider) },
55+
setTaskMode: vi.fn(),
56+
emit: vi.fn(),
57+
updateApiConfiguration: vi.fn(),
58+
setTaskApiConfigName: vi.fn(),
59+
postTaskStateToWebview: vi.fn().mockResolvedValue(undefined),
60+
}
61+
}
62+
63+
const invoke = (task: unknown, mode = "architect") => (Task.prototype as any).switchTaskMode.call(task, mode)
64+
65+
describe("Task.switchTaskMode()", () => {
66+
it("routes a VISIBLE task through the global handleModeSwitch after task-scoped persistence", async () => {
67+
const provider = makeProvider({ isTaskVisible: vi.fn().mockReturnValue(true) })
68+
const task = makeTask(provider)
69+
70+
await invoke(task)
71+
72+
expect(task.setTaskMode).toHaveBeenCalledWith("architect")
73+
// History persisted without broadcast before the global switch.
74+
expect(provider.updateTaskHistory).toHaveBeenCalledWith(
75+
{ ...taskHistoryItem, mode: "architect" },
76+
{ broadcast: false },
77+
)
78+
expect(provider.handleModeSwitch).toHaveBeenCalledWith("architect")
79+
80+
// The visible path delegates profile handling to the global handler:
81+
// no direct task-scoped profile resolution or API handler mutation.
82+
expect(provider.providerSettingsManager.getModeConfigId).not.toHaveBeenCalled()
83+
expect(task.updateApiConfiguration).not.toHaveBeenCalled()
84+
expect(task.setTaskApiConfigName).not.toHaveBeenCalled()
85+
})
86+
87+
it("never calls the global handleModeSwitch for a BACKGROUND task", async () => {
88+
const provider = makeProvider()
89+
const task = makeTask(provider)
90+
91+
await invoke(task)
92+
93+
expect(provider.handleModeSwitch).not.toHaveBeenCalled()
94+
expect(task.setTaskMode).toHaveBeenCalledWith("architect")
95+
expect(provider.updateTaskHistory).toHaveBeenCalledWith(
96+
{ ...taskHistoryItem, mode: "architect" },
97+
{ broadcast: false },
98+
)
99+
// The per-task event still fires for API observers (the global path
100+
// emits it inside handleModeSwitch).
101+
expect(task.emit).toHaveBeenCalledWith(RooCodeEventName.TaskModeSwitched, "task-1", "architect")
102+
expect(task.postTaskStateToWebview).toHaveBeenCalledTimes(1)
103+
})
104+
105+
it("resolves the mode-bound profile task-scoped for a background task and applies it to the task only", async () => {
106+
const providerSettingsManager = makeProviderSettingsManager({
107+
getModeConfigId: vi.fn().mockResolvedValue("cfg-arch"),
108+
listConfig: vi.fn().mockResolvedValue([
109+
{ id: "cfg-other", name: "other-profile" },
110+
{ id: "cfg-arch", name: "architect-profile" },
111+
]),
112+
getProfile: vi.fn().mockResolvedValue({
113+
id: "cfg-arch",
114+
name: "architect-profile",
115+
apiProvider: "anthropic",
116+
apiModelId: "architect-model",
117+
}),
118+
})
119+
const provider = makeProvider({ providerSettingsManager })
120+
const task = makeTask(provider)
121+
122+
await invoke(task)
123+
124+
expect(providerSettingsManager.getModeConfigId).toHaveBeenCalledWith("architect")
125+
expect(providerSettingsManager.getProfile).toHaveBeenCalledWith({ name: "architect-profile" })
126+
127+
// The resolved profile (id/name stripped) is applied to THIS task's API handler.
128+
expect(task.updateApiConfiguration).toHaveBeenCalledWith({
129+
apiProvider: "anthropic",
130+
apiModelId: "architect-model",
131+
})
132+
expect(task.setTaskApiConfigName).toHaveBeenCalledWith("architect-profile")
133+
134+
// Profile name persisted to this task's history without broadcast.
135+
expect(provider.updateTaskHistory).toHaveBeenCalledWith(
136+
{ ...taskHistoryItem, apiConfigName: "architect-profile" },
137+
{ broadcast: false },
138+
)
139+
140+
// Never routed through the global mode/profile switch.
141+
expect(provider.handleModeSwitch).not.toHaveBeenCalled()
142+
})
143+
144+
it("keeps the current config when the resolved profile has no apiProvider", async () => {
145+
const providerSettingsManager = makeProviderSettingsManager({
146+
getModeConfigId: vi.fn().mockResolvedValue("cfg-empty"),
147+
listConfig: vi.fn().mockResolvedValue([{ id: "cfg-empty", name: "empty-profile" }]),
148+
getProfile: vi.fn().mockResolvedValue({ id: "cfg-empty", name: "empty-profile" }),
149+
})
150+
const provider = makeProvider({ providerSettingsManager })
151+
const task = makeTask(provider)
152+
153+
await invoke(task)
154+
155+
expect(task.updateApiConfiguration).not.toHaveBeenCalled()
156+
expect(task.setTaskApiConfigName).not.toHaveBeenCalled()
157+
// Mode itself still switches.
158+
expect(task.setTaskMode).toHaveBeenCalledWith("architect")
159+
expect(task.postTaskStateToWebview).toHaveBeenCalledTimes(1)
160+
})
161+
162+
it("does not write a mode→config binding when the mode has no saved config (background)", async () => {
163+
const provider = makeProvider()
164+
const task = makeTask(provider)
165+
166+
await invoke(task)
167+
168+
// handleModeSwitch's else-branch would call setModeConfig; a background
169+
// conversation must not mutate that user-level setting.
170+
expect(provider.providerSettingsManager.setModeConfig).not.toHaveBeenCalled()
171+
expect(task.updateApiConfiguration).not.toHaveBeenCalled()
172+
expect(task.setTaskApiConfigName).not.toHaveBeenCalled()
173+
})
174+
175+
it("skips mode-bound profile resolution entirely when lockApiConfigAcrossModes is set", async () => {
176+
const providerSettingsManager = makeProviderSettingsManager({
177+
getModeConfigId: vi.fn().mockResolvedValue("cfg-arch"),
178+
})
179+
const workspaceStateGet = vi.fn((key: string, defaultValue?: unknown) =>
180+
key === "lockApiConfigAcrossModes" ? true : defaultValue,
181+
)
182+
const provider = makeProvider({
183+
providerSettingsManager,
184+
context: { workspaceState: { get: workspaceStateGet } },
185+
})
186+
const task = makeTask(provider)
187+
188+
await invoke(task)
189+
190+
expect(workspaceStateGet).toHaveBeenCalledWith("lockApiConfigAcrossModes", false)
191+
expect(providerSettingsManager.getModeConfigId).not.toHaveBeenCalled()
192+
expect(providerSettingsManager.listConfig).not.toHaveBeenCalled()
193+
expect(providerSettingsManager.getProfile).not.toHaveBeenCalled()
194+
expect(task.updateApiConfiguration).not.toHaveBeenCalled()
195+
// Mode itself still switches.
196+
expect(task.setTaskMode).toHaveBeenCalledWith("architect")
197+
})
198+
199+
it("treats a failed profile resolution as non-fatal: mode switches, state still posts", async () => {
200+
const resolutionError = new Error("settings store unavailable")
201+
const providerSettingsManager = makeProviderSettingsManager({
202+
getModeConfigId: vi.fn().mockRejectedValue(resolutionError),
203+
})
204+
const provider = makeProvider({ providerSettingsManager })
205+
const task = makeTask(provider)
206+
207+
await expect(invoke(task)).resolves.toBeUndefined()
208+
209+
expect(task.setTaskMode).toHaveBeenCalledWith("architect")
210+
expect(task.updateApiConfiguration).not.toHaveBeenCalled()
211+
expect(provider.log).toHaveBeenCalledWith(expect.stringContaining("Task-scoped profile resolution failed"))
212+
expect(task.postTaskStateToWebview).toHaveBeenCalledTimes(1)
213+
})
214+
215+
it("is a no-op when the provider reference is gone", async () => {
216+
const task = makeTask(undefined)
217+
218+
await invoke(task)
219+
220+
expect(task.setTaskMode).not.toHaveBeenCalled()
221+
expect(task.emit).not.toHaveBeenCalled()
222+
expect(task.postTaskStateToWebview).not.toHaveBeenCalled()
223+
})
224+
})

‎src/core/task/Task.ts‎

Lines changed: 53 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -966,10 +966,61 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
966966
}
967967

968968
if (provider.isTaskVisible(this.taskId)) {
969+
// Visible conversation: the global handler keeps global state and the
970+
// webview in sync and activates the mode-bound provider profile.
969971
await provider.handleModeSwitch(mode as any)
970-
} else {
971-
await this.postTaskStateToWebview()
972+
return
972973
}
974+
975+
// Background conversation: global state belongs to the visible conversation,
976+
// so everything below must stay task-scoped. The global path emits this
977+
// event inside handleModeSwitch; emit it here for API observers.
978+
this.emit(RooCodeEventName.TaskModeSwitched, this.taskId, mode)
979+
980+
// Resolve the mode-bound provider profile TASK-SCOPED (mirrors
981+
// ClineProvider.delegateParentAndOpenChild): routing through the global
982+
// handleModeSwitch/activateProviderProfile would clobber the visible
983+
// conversation's mode/profile and race concurrent switches.
984+
try {
985+
const lockApiConfigAcrossModes = provider.context.workspaceState.get("lockApiConfigAcrossModes", false)
986+
if (!lockApiConfigAcrossModes) {
987+
const savedConfigId = await provider.providerSettingsManager.getModeConfigId(mode)
988+
if (savedConfigId) {
989+
const listApiConfig = await provider.providerSettingsManager.listConfig()
990+
const profileName = listApiConfig.find(({ id }) => id === savedConfigId)?.name
991+
if (profileName) {
992+
const {
993+
id: _profileId,
994+
name: _profileName,
995+
...providerSettings
996+
} = await provider.providerSettingsManager.getProfile({ name: profileName })
997+
if (providerSettings.apiProvider) {
998+
this.updateApiConfiguration(providerSettings)
999+
this.setTaskApiConfigName(profileName)
1000+
1001+
const latestHistoryItem = provider.taskHistoryStore.get(this.taskId)
1002+
if (latestHistoryItem) {
1003+
await provider.updateTaskHistory(
1004+
{ ...latestHistoryItem, apiConfigName: profileName },
1005+
{ broadcast: false },
1006+
)
1007+
}
1008+
}
1009+
}
1010+
}
1011+
}
1012+
// When the mode has no saved config, handleModeSwitch would bind the
1013+
// current global config to it — a user-level setting that a background
1014+
// conversation must not mutate, so no setModeConfig here.
1015+
} catch (error) {
1016+
provider.log(
1017+
`[Task#${this.taskId}] Task-scoped profile resolution failed for mode '${mode}' (keeping current config): ${
1018+
error instanceof Error ? error.message : String(error)
1019+
}`,
1020+
)
1021+
}
1022+
1023+
await this.postTaskStateToWebview()
9731024
}
9741025

9751026
public async switchTaskProviderProfile(name: string): Promise<void> {

‎src/core/tools/RunSlashCommandTool.ts‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -111,11 +111,9 @@ export class RunSlashCommandTool extends BaseTool<"run_slash_command"> {
111111
const provider = task.providerRef.deref()
112112
const targetMode = getModeBySlug(command.mode, (await provider?.getState())?.customModes)
113113
if (targetMode) {
114-
if (typeof (task as any).switchTaskMode === "function") {
115-
await task.switchTaskMode(command.mode)
116-
} else {
117-
await provider?.handleModeSwitch(command.mode as any)
118-
}
114+
// Task-scoped switch: only touches global state when this task
115+
// is the visible conversation.
116+
await task.switchTaskMode(command.mode)
119117
}
120118
}
121119

‎src/core/tools/SwitchModeTool.ts‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -63,12 +63,10 @@ export class SwitchModeTool extends BaseTool<"switch_mode"> {
6363
return
6464
}
6565

66-
// Switch the mode using shared handler
67-
if (typeof (task as any).switchTaskMode === "function") {
68-
await task.switchTaskMode(mode_slug)
69-
} else {
70-
await task.providerRef.deref()?.handleModeSwitch(mode_slug as any)
71-
}
66+
// Task-scoped switch: only touches global state when this task is the
67+
// visible conversation (a global handleModeSwitch here would mutate the
68+
// VISIBLE task's mode when invoked from a background conversation).
69+
await task.switchTaskMode(mode_slug)
7270

7371
pushToolResult(
7472
`Successfully switched from ${getModeBySlug(currentMode)?.name ?? currentMode} mode to ${

‎src/core/tools/__tests__/runSlashCommandTool.spec.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ describe("runSlashCommandTool", () => {
2323
recordToolError: vi.fn(),
2424
sayAndCreateMissingParamError: vi.fn().mockResolvedValue("Missing parameter error"),
2525
ask: vi.fn().mockResolvedValue({}),
26+
switchTaskMode: vi.fn().mockResolvedValue(undefined),
2627
cwd: "/test/project",
2728
providerRef: {
2829
deref: vi.fn().mockReturnValue({
@@ -471,7 +472,11 @@ Deploy application to production`,
471472

472473
await runSlashCommandTool.handle(mockTask as Task, block, mockCallbacks)
473474

474-
expect(mockHandleModeSwitch).toHaveBeenCalledWith("debug")
475+
// Task-scoped switch on the invoking task, never the provider-global
476+
// handler (which would mutate the VISIBLE task's mode when the command
477+
// runs in a background conversation).
478+
expect(mockTask.switchTaskMode).toHaveBeenCalledWith("debug")
479+
expect(mockHandleModeSwitch).not.toHaveBeenCalled()
475480
expect(mockCallbacks.pushToolResult).toHaveBeenCalledWith(
476481
`Command: /debug-app
477482
Description: Debug the application
@@ -518,6 +523,7 @@ Start debugging the application`,
518523

519524
await runSlashCommandTool.handle(mockTask as Task, block, mockCallbacks)
520525

526+
expect(mockTask.switchTaskMode).not.toHaveBeenCalled()
521527
expect(mockHandleModeSwitch).not.toHaveBeenCalled()
522528
})
523529

0 commit comments

Comments
 (0)