diff --git a/package.json b/package.json index 1fd9ddc8fe..df3410bbc1 100644 --- a/package.json +++ b/package.json @@ -13,7 +13,7 @@ "check-types": "turbo check-types --log-order grouped --output-logs new-only", "test": "turbo test --log-order grouped --output-logs new-only", "test:mutation-ci": "node --test scripts/stryker-diff.test.mjs", - "lifecycle:model-check": "tsx scripts/check-task-lifecycle.ts && tsx scripts/check-task-store-concurrency.ts && tsx scripts/check-provider-handoff-scheduler.ts && pnpm cleanup-protocol:model-check && pnpm parser-scope:model-check && tsx scripts/check-completion-persistence.ts", + "lifecycle:model-check": "tsx scripts/check-task-lifecycle.ts && tsx scripts/check-task-store-concurrency.ts && tsx scripts/check-provider-handoff-scheduler.ts && pnpm cleanup-protocol:model-check && pnpm parser-scope:model-check && tsx scripts/check-completion-persistence.ts && tsx scripts/check-delegated-mode-readers.ts", "cleanup-protocol:model-check": "tsx scripts/check-task-cleanup-protocol.ts", "parser-scope:model-check": "node scripts/run-native-tool-call-parser-scoping.mjs", "mcp:integration-check": "tsx scripts/check-mcp-oauth-integration.ts", diff --git a/scripts/check-delegated-mode-readers.ts b/scripts/check-delegated-mode-readers.ts new file mode 100644 index 0000000000..bd77de29c1 --- /dev/null +++ b/scripts/check-delegated-mode-readers.ts @@ -0,0 +1,170 @@ +// check-delegated-mode-readers.ts +// +// Refinement check for the delegated-child mode-reader invariant (issue #1623). +// +// check-provider-handoff-scheduler.ts verifies the write side: that +// selectHandoffExecutionContext stores the task-local mode correctly. +// This script verifies the read side: that the mode observable by +// tool-validation readers is the task-local mode, not the shared provider mode. +// +// The VS Code-dependent readers (getEnvironmentDetails, +// presentAssistantMessage) are covered by their vitest regression tests. +// This script covers the pure-TS parts of the invariant chain and proves +// that the two sources of mode are observably different, so any reader +// that uses the wrong source silently produces wrong behavior. +// +// Invariant: for any delegated child task C with taskMode = M, +// toolAllowedForMode(tool, M) ≠ toolAllowedForMode(tool, providerMode) +// whenever M ≠ providerMode and the two modes differ on the tool's group. + +import assert from "node:assert/strict" + +import { DEFAULT_MODES } from "../packages/types/src/mode" + +import { TOOL_GROUPS, ALWAYS_AVAILABLE_TOOLS, TOOL_ALIASES } from "../src/shared/tools" +import { selectHandoffExecutionContext, type TaskExecutionContext } from "../src/core/task/providerHandoff" + +// --------------------------------------------------------------------------- +// Minimal inline mode-allows-tool check. +// Avoids importing src/shared/modes.ts, which pulls in VS Code. +// Only covers built-in modes (no custom modes, no file-regex options). +// That is enough to prove the behavioral divergence this check needs. +// --------------------------------------------------------------------------- + +type ModeConfig = (typeof DEFAULT_MODES)[number] +type GroupEntry = ModeConfig["groups"][number] + +function groupName(entry: GroupEntry): string { + return Array.isArray(entry) ? entry[0] : (entry as string) +} + +function toolAllowedForMode(tool: string, modeSlug: string): boolean { + const resolvedTool = (TOOL_ALIASES as Record)[tool] ?? tool + if ((ALWAYS_AVAILABLE_TOOLS as readonly string[]).includes(resolvedTool)) return true + const mode = DEFAULT_MODES.find((m) => m.slug === modeSlug) + if (!mode) return false + for (const entry of mode.groups) { + const groupTools = (TOOL_GROUPS as Record)[groupName(entry)]?.tools ?? [] + if (groupTools.includes(resolvedTool)) return true + } + return false +} + +// --------------------------------------------------------------------------- +// Scenario: parent in "orchestrator" mode delegates child to "code". +// Regression behavior: both readers used providerMode ("orchestrator"). +// Correct behavior: readers use taskMode ("code"). +// +// orchestrator groups: [] → apply_diff blocked +// code groups: [...edit] → apply_diff allowed +// --------------------------------------------------------------------------- + +const parentCtx: TaskExecutionContext = { + mode: "orchestrator", + apiConfigName: undefined, + apiConfiguration: { apiProvider: "anthropic", consecutiveMistakeLimit: 3 }, +} + +// 1. Handoff stores the task-local mode, not the parent mode. +const childCtx = selectHandoffExecutionContext(parentCtx, "code", parentCtx.mode, false, undefined) +assert.equal(childCtx.mode, "code", "handoff must store the requested task-local mode") +assert.notEqual(childCtx.mode, parentCtx.mode, "test scenario requires divergent provider and task modes") + +// 2. The two modes produce observably different tool-validation outcomes. +assert.equal(toolAllowedForMode("apply_diff", "orchestrator"), false, "orchestrator has no edit group") +assert.equal(toolAllowedForMode("apply_diff", "code"), true, "code has the edit group") + +// 3. Regression claim: a reader that consumes providerMode rejects apply_diff; +// a reader that consumes taskMode correctly allows it. +const viaProviderMode = toolAllowedForMode("apply_diff", parentCtx.mode) // "orchestrator" — wrong source +const viaTaskMode = toolAllowedForMode("apply_diff", childCtx.mode) // "code" — correct source +assert.equal(viaProviderMode, false, "stale provider mode rejects apply_diff (regression behavior)") +assert.equal(viaTaskMode, true, "task-local mode allows apply_diff (correct behavior)") + +// 4. Additional mode pairs that show the same divergence. +const DIVERGENT_PAIRS: Array<{ + label: string + providerMode: string + taskMode: string + probe: string + blockedInProvider: boolean + allowedInTask: boolean +}> = [ + // orchestrator → code: edit tools blocked at provider level, allowed at task level + { + label: "orchestrator→code apply_diff", + providerMode: "orchestrator", + taskMode: "code", + probe: "apply_diff", + blockedInProvider: true, + allowedInTask: true, + }, + // orchestrator → code: command tools blocked at provider level, allowed at task level + { + label: "orchestrator→code execute_command", + providerMode: "orchestrator", + taskMode: "code", + probe: "execute_command", + blockedInProvider: true, + allowedInTask: true, + }, + // code → ask: edit tools allowed at provider level, blocked at task level + { + label: "code→ask apply_diff", + providerMode: "code", + taskMode: "ask", + probe: "apply_diff", + blockedInProvider: false, + allowedInTask: false, + }, + // ask → code: edit tools blocked at provider level, allowed at task level + { + label: "ask→code write_to_file", + providerMode: "ask", + taskMode: "code", + probe: "write_to_file", + blockedInProvider: true, + allowedInTask: true, + }, +] + +for (const pair of DIVERGENT_PAIRS) { + const ctx = selectHandoffExecutionContext( + { ...parentCtx, mode: pair.providerMode }, + pair.taskMode, + pair.providerMode, + false, + undefined, + ) + assert.equal(ctx.mode, pair.taskMode, `${pair.label}: handoff must store task-local mode`) + assert.equal( + toolAllowedForMode(pair.probe, pair.providerMode), + !pair.blockedInProvider, + `${pair.label}: wrong provider-mode result`, + ) + assert.equal( + toolAllowedForMode(pair.probe, pair.taskMode), + pair.allowedInTask, + `${pair.label}: wrong task-mode result`, + ) + // The two sources disagree, so using the wrong one is always observable. + assert.notEqual( + toolAllowedForMode(pair.probe, pair.providerMode), + toolAllowedForMode(pair.probe, pair.taskMode), + `${pair.label}: provider and task mode must differ on this probe tool`, + ) +} + +// 5. For every built-in mode as a delegation target: selectHandoffExecutionContext +// always stores the requested mode, regardless of parent mode. +for (const mode of DEFAULT_MODES) { + const ctx = selectHandoffExecutionContext(parentCtx, mode.slug, parentCtx.mode, false, undefined) + assert.equal(ctx.mode, mode.slug, `handoff must store ${mode.slug}, not parent mode ${parentCtx.mode}`) +} + +console.log( + `Delegated mode reader check passed: ` + + `regression scenario verified, ` + + `${DIVERGENT_PAIRS.length} divergent-mode pairs checked, ` + + `${DEFAULT_MODES.length}/${DEFAULT_MODES.length} built-in modes verified`, +) diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts index 1ef25e852b..e7f4465441 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts @@ -77,6 +77,7 @@ describe("presentAssistantMessage - Custom Tool Recording", () => { }), }), }, + getTaskMode: vi.fn().mockResolvedValue("code"), say: vi.fn().mockResolvedValue(undefined), ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked" }), } @@ -122,6 +123,51 @@ describe("presentAssistantMessage - Custom Tool Recording", () => { }) }) + describe("Custom tool mode delegation regression", () => { + // Regression for issue #1623. + // Before the fix, customTool.execute received the shared provider mode + // instead of the task-local mode. A child delegated to "architect" would + // have its custom tool called with "orchestrator". + it("passes the task-local mode to customTool.execute, not the provider mode", async () => { + // Provider says "orchestrator"; task was delegated to "architect". + mockTask.providerRef = { + deref: () => ({ + getState: vi.fn().mockResolvedValue({ + mode: "orchestrator", + customModes: [], + experiments: { customTools: true }, + }), + }), + } + mockTask.getTaskMode = vi.fn().mockResolvedValue("architect") + + const executeMock = vi.fn().mockResolvedValue("result") + vi.mocked(customToolRegistry.has).mockReturnValue(true) + vi.mocked(customToolRegistry.get).mockReturnValue({ + name: "my_custom_tool", + description: "A custom tool", + execute: executeMock, + }) + + mockTask.assistantMessageContent = [ + { + type: "tool_use", + id: "call_delegation", + name: "my_custom_tool", + params: { value: "test" }, + partial: false, + }, + ] + + await presentAssistantMessage(mockTask) + + expect(executeMock).toHaveBeenCalledOnce() + const context = executeMock.mock.calls[0][1] + expect(context.mode).toBe("architect") + expect(context.task).toBe(mockTask) + }) + }) + describe("Custom tool error recording", () => { it("should record custom tool error as 'custom_tool'", async () => { const toolCallId = "tool_call_custom_error_123" diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-images.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-images.spec.ts index fcf778b8f8..7cb4c427d8 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-images.spec.ts +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-images.spec.ts @@ -57,6 +57,7 @@ describe("presentAssistantMessage - Image Handling in Native Tool Calling", () = }), }), }, + getTaskMode: vi.fn().mockResolvedValue("code"), say: vi.fn().mockResolvedValue(undefined), ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked" }), } diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-tool-usage-attribution.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-tool-usage-attribution.spec.ts index c75eb6ee18..9becd11bbe 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-tool-usage-attribution.spec.ts +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-tool-usage-attribution.spec.ts @@ -65,14 +65,17 @@ interface MockTask { recordToolError: ReturnType toolRepetitionDetector: { check: ReturnType } providerRef: { - deref: () => { - getState: ReturnType - getMcpHub?: () => { findServerNameBySanitizedName: (name: string) => string | undefined } - } + deref: () => + | { + getState: ReturnType + getMcpHub?: () => { findServerNameBySanitizedName: (name: string) => string | undefined } + } + | undefined } say: ReturnType ask: ReturnType pushToolResultToUserContent: ReturnType + getTaskMode: ReturnType } describe("presentAssistantMessage - tool usage attribution", () => { @@ -115,6 +118,7 @@ describe("presentAssistantMessage - tool usage attribution", () => { say: vi.fn().mockResolvedValue(undefined), ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked" }), pushToolResultToUserContent: vi.fn(), + getTaskMode: vi.fn().mockResolvedValue("code"), } mockTask.pushToolResultToUserContent = vi @@ -316,4 +320,71 @@ describe("presentAssistantMessage - tool usage attribution", () => { expect(TelemetryService.instance.captureToolUsage).not.toHaveBeenCalled() }) }) + + describe("undefined provider state", () => { + // Covers the `state ?? {}` fallback branch (line 347 of presentAssistantMessage.ts). + // When providerRef.deref() returns undefined, state is undefined and the + // destructure falls back to {}, so customModes / experiments / disabledTools + // are all undefined. Tool validation must still use the task-local mode. + it("falls back to empty state when provider is unavailable", async () => { + mockTask.providerRef = { deref: () => undefined } + mockTask.assistantMessageContent = [ + { + type: "tool_use", + id: "call_no_state", + name: "read_file", + params: { path: "test.ts" }, + nativeArgs: { path: "test.ts" }, + partial: false, + }, + ] + + await presentAssistantMessage(mockTask as unknown as Task) + + // validateToolUse must still be called with the task-local mode. + const calls = vi.mocked(validateToolUse).mock.calls + expect(calls.length).toBeGreaterThan(0) + expect(calls[0][1]).toBe("code") + // customModes falls back to [] (from the ?? {} path). + expect(calls[0][2]).toEqual([]) + }) + }) + + describe("mode delegation regression", () => { + // Regression for issue #1623. + // Before the fix, validateToolUse received the shared provider mode instead + // of the task-local mode, so a child delegated to "architect" mode would have + // its tools validated against "orchestrator". + it("passes the task-local mode to validateToolUse, not the provider mode", async () => { + // Provider says "orchestrator"; task was delegated to "architect". + mockTask.providerRef = { + deref: () => ({ + getState: vi.fn().mockResolvedValue({ + mode: "orchestrator", + customModes: [], + }), + }), + } + mockTask.getTaskMode = vi.fn().mockResolvedValue("architect") + + mockTask.assistantMessageContent = [ + { + type: "tool_use", + id: "call_delegation", + name: "read_file", + params: { path: "test.ts" }, + nativeArgs: { path: "test.ts" }, + partial: false, + }, + ] + + await presentAssistantMessage(mockTask as unknown as Task) + + // The key assertion: task-local mode "architect" was passed, not "orchestrator". + const calls = vi.mocked(validateToolUse).mock.calls + expect(calls.length).toBeGreaterThan(0) + expect(calls[0][0]).toBe("read_file") + expect(calls[0][1]).toBe("architect") + }) + }) }) diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts index 78a4a19e91..78af9653c7 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts @@ -60,6 +60,7 @@ describe("presentAssistantMessage - Unknown Tool Handling", () => { }), }), }, + getTaskMode: vi.fn().mockResolvedValue("code"), say: vi.fn().mockResolvedValue(undefined), ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked" }), } diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index 7b25db4e66..b5a83882be 100644 --- a/src/core/assistant-message/presentAssistantMessage.ts +++ b/src/core/assistant-message/presentAssistantMessage.ts @@ -344,7 +344,10 @@ export async function presentAssistantMessage(cline: Task) { // Fetch state early so it's available for toolDescription and validation const state = await cline.providerRef.deref()?.getState() - const { mode, customModes, experiments: stateExperiments, disabledTools } = state ?? {} + const { customModes, experiments: stateExperiments, disabledTools } = state ?? {} + // Read the task-local mode, not the shared provider mode. + // A delegated child task may run in a different mode than its parent. + const taskMode = await cline.getTaskMode() const toolDescription = (): string => { switch (block.name) { @@ -617,7 +620,7 @@ export async function presentAssistantMessage(cline: Task) { validateToolUse( block.name as ToolName, - mode ?? defaultModeSlug, + taskMode, customModes ?? [], toolRequirements, block.params, @@ -924,7 +927,7 @@ export async function presentAssistantMessage(cline: Task) { } const result = await customTool.execute(customToolArgs, { - mode: mode ?? defaultModeSlug, + mode: taskMode, task: cline, }) diff --git a/src/core/environment/__tests__/getEnvironmentDetails.spec.ts b/src/core/environment/__tests__/getEnvironmentDetails.spec.ts index df47e83c21..0b4d63fbac 100644 --- a/src/core/environment/__tests__/getEnvironmentDetails.spec.ts +++ b/src/core/environment/__tests__/getEnvironmentDetails.spec.ts @@ -117,6 +117,7 @@ describe("getEnvironmentDetails", () => { deref: vi.fn().mockReturnValue(mockProvider), [Symbol.toStringTag]: "WeakRef", } as unknown as WeakRef, + getTaskMode: vi.fn().mockResolvedValue("code"), } // Mock other dependencies. @@ -464,4 +465,29 @@ describe("getEnvironmentDetails", () => { const result = await getEnvironmentDetails(mockCline as Task, true) expect(result).toContain("File listing unavailable: unexpected string rejection") }) + + // Regression for issue #1623. + // Before the fix, the Current Mode block read the shared provider mode. + // A child delegated to "architect" mode would report "orchestrator" instead. + it("uses the task-local mode in the Current Mode block, not the provider mode", async () => { + // Provider mode stays "code"; task was delegated to "architect". + mockState.mode = "code" + ;(mockCline.getTaskMode as Mock).mockResolvedValue("architect") + ;(getFullModeDetails as Mock).mockResolvedValue({ + name: "🏗️ Architect", + roleDefinition: "You design software.", + customInstructions: "", + }) + + const result = await getEnvironmentDetails(mockCline as Task) + + expect(result).toContain("architect") + expect(result).not.toContain("code") + expect(getFullModeDetails).toHaveBeenCalledWith( + "architect", + [], + undefined, + expect.objectContaining({ cwd: mockCwd }), + ) + }) }) diff --git a/src/core/environment/getEnvironmentDetails.ts b/src/core/environment/getEnvironmentDetails.ts index 0e7d18a57a..773870c304 100644 --- a/src/core/environment/getEnvironmentDetails.ts +++ b/src/core/environment/getEnvironmentDetails.ts @@ -205,7 +205,6 @@ export async function getEnvironmentDetails(cline: Task, includeFileDetails: boo // Add current mode and any mode-specific warnings. const { - mode, customModes, customModePrompts, experiments = {} as Record, @@ -213,7 +212,9 @@ export async function getEnvironmentDetails(cline: Task, includeFileDetails: boo language, } = state ?? {} - const currentMode = mode ?? defaultModeSlug + // Read the task-local mode, not the shared provider mode. + // A delegated child task may run in a different mode than its parent. + const currentMode = await cline.getTaskMode() const modeDetails = await getFullModeDetails(currentMode, customModes, customModePrompts, { cwd: cline.cwd,