diff --git a/packages/core/src/agents/background-agent-resume.test.ts b/packages/core/src/agents/background-agent-resume.test.ts index 801181a9f2c..2319deeebb4 100644 --- a/packages/core/src/agents/background-agent-resume.test.ts +++ b/packages/core/src/agents/background-agent-resume.test.ts @@ -58,6 +58,17 @@ describe('BackgroundAgentResumeService', () => { fireSubagentStartEvent: vi.fn().mockResolvedValue(undefined), fireSubagentStopEvent: vi.fn().mockResolvedValue(undefined), }; + // Stub registry exposed on both `parent.getToolRegistry()` and the + // override built by `createApprovalModeOverride` (which now rebuilds + // the tool registry on the resumed agent's Config so bound tools + // resolve to the resumed agent — see PR #3873). Without these + // mocks the override helper throws and every resume test fails. + const stubToolRegistry = { + copyDiscoveredToolsFrom: vi.fn(), + getAllTools: vi.fn().mockReturnValue([]), + getAllToolNames: vi.fn().mockReturnValue([]), + stop: vi.fn().mockResolvedValue(undefined), + }; const config = { storage: { getProjectDir: () => tempDir, @@ -72,6 +83,8 @@ describe('BackgroundAgentResumeService', () => { getGeminiClient: () => undefined, getSkipStartupContext: () => true, getTranscriptPath: () => path.join(tempDir, 'session.jsonl'), + getToolRegistry: () => stubToolRegistry, + createToolRegistry: vi.fn().mockResolvedValue(stubToolRegistry), } as unknown as Config; return { diff --git a/packages/core/src/agents/background-agent-resume.ts b/packages/core/src/agents/background-agent-resume.ts index 452d636e185..f7631b31cbf 100644 --- a/packages/core/src/agents/background-agent-resume.ts +++ b/packages/core/src/agents/background-agent-resume.ts @@ -28,6 +28,8 @@ import { getInitialChatHistory } from '../utils/environmentContext.js'; import { getGitBranch } from '../utils/gitUtils.js'; import { PermissionMode, type StopHookOutput } from '../hooks/types.js'; import { runWithAgentContext } from '../tools/agent/agent-context.js'; +import { createApprovalModeOverride } from '../tools/agent/agent.js'; +import type { ApprovalMode } from '../config/config.js'; import { FORK_AGENT, FORK_SUBAGENT_TYPE, @@ -139,16 +141,6 @@ function reconcileResumedApprovalMode( return 'default'; } -function createApprovalModeOverride( - base: Config, - mode: ApprovalModeValue, -): Config { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const override = Object.create(base) as any; - override.getApprovalMode = () => mode; - return override as Config; -} - function persistBackgroundCancellation( metaPath: string, persistedStatus: 'running' | 'cancelled', @@ -527,10 +519,18 @@ export class BackgroundAgentResumeService { parentApprovalMode, this.config.isTrustedFolder(), ); - const agentConfig = - resolvedApprovalMode !== this.config.getApprovalMode() - ? createApprovalModeOverride(this.config, resolvedApprovalMode) - : this.config; + // Always wrap, even when the resolved approval mode matches the + // parent's. The wrapper rebuilds the tool registry on the + // override Config so bound `EditTool` / `WriteFileTool` / + // `ReadFileTool` instances resolve `this.config` to the resumed + // agent and use the resumed agent's `FileReadCache`, instead of + // continuing to read the parent's. Reusing `this.config` + // directly here would short-circuit that isolation. See the + // matching wrapper in `agent.ts:createApprovalModeOverride`. + const agentConfig = await createApprovalModeOverride( + this.config, + resolvedApprovalMode as ApprovalMode, + ); // eslint-disable-next-line @typescript-eslint/no-explicit-any const bgConfig = Object.create(agentConfig) as any; bgConfig.getShouldAvoidPermissionPrompts = () => true; @@ -771,6 +771,16 @@ export class BackgroundAgentResumeService { bgEmitter.off(AgentEventType.TOOL_CALL, onToolCall); bgEmitter.off(AgentEventType.USAGE_METADATA, onUsageMetadata); cleanupJsonl?.(); + // Release the per-subagent ToolRegistry the resumed agent's + // wrapper Config built in `createApprovalModeOverride` so any + // AgentTool / SkillTool the model instantiated during this + // run disposes its change-listeners on shared + // SubagentManager / SkillManager. Without this, every resume + // accumulates listeners for the rest of the session. + void agentConfig + .getToolRegistry() + .stop() + .catch(() => {}); } }; diff --git a/packages/core/src/subagents/subagent-manager-override.test.ts b/packages/core/src/subagents/subagent-manager-override.test.ts new file mode 100644 index 00000000000..ec795d66723 --- /dev/null +++ b/packages/core/src/subagents/subagent-manager-override.test.ts @@ -0,0 +1,377 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi } from 'vitest'; +import { Config, ApprovalMode } from '../config/config.js'; +import { SubagentManager } from './subagent-manager.js'; +import type { SubagentConfig } from './types.js'; +import { ToolNames } from '../tools/tool-names.js'; +import { EditTool } from '../tools/edit.js'; +import { ReadFileTool } from '../tools/read-file.js'; +import { createApprovalModeOverride } from '../tools/agent/agent.js'; + +// The non-inherit (explicit-model) branch in maybeOverrideContentGenerator +// builds a fresh ContentGenerator. We don't want the test to actually +// reach the OpenAI / Anthropic SDK — replacing the factory with a stub +// is enough to exercise the code path. +vi.mock('../core/contentGenerator.js', async () => { + const actual = await vi.importActual< + typeof import('../core/contentGenerator.js') + >('../core/contentGenerator.js'); + return { + ...actual, + createContentGenerator: vi.fn().mockResolvedValue({ + generateContent: vi.fn(), + generateContentStream: vi.fn(), + }), + }; +}); + +vi.mock('../models/content-generator-config.js', async () => { + const actual = await vi.importActual< + typeof import('../models/content-generator-config.js') + >('../models/content-generator-config.js'); + return { + ...actual, + buildAgentContentGeneratorConfig: vi.fn().mockReturnValue({ + model: 'override-model', + authType: 'openai', + apiKey: 'override-key', + }), + }; +}); + +/** + * Companion to `tools/agent/agent-override.test.ts`. Same regression: + * Object.create(parent) by itself is not enough to isolate a subagent's + * core tools from the parent's bound `EditTool` / `WriteFileTool` / + * `ReadFileTool`. The subagent path that flows through + * `SubagentManager.maybeOverrideContentGenerator` must rebuild the + * tool registry on the override Config so bound tools resolve + * `this.config` to the subagent rather than the parent — otherwise + * mutations executed via the bound tool reach the parent's + * FileReadCache and silently weaken prior-read enforcement. + */ +describe('SubagentManager.maybeOverrideContentGenerator bound-tool isolation', () => { + // Bare mode keeps the registry small (ReadFile / Edit / Shell only) and + // avoids needing extra setup for optional tools. + const baseParams = { + cwd: '/tmp', + targetDir: '/tmp', + debugMode: false, + model: 'test-model', + usageStatisticsEnabled: false, + bareMode: true, + }; + + // The method is `private`. Cast via `unknown` to invoke it directly — + // testing through the public `createAgentHeadless` pathway would also + // work but pulls in a much larger graph (file IO, hooks, etc.). + function callMaybeOverride( + manager: SubagentManager, + config: SubagentConfig, + base: Config, + ): Promise { + const fn = ( + manager as unknown as { + maybeOverrideContentGenerator: ( + c: SubagentConfig, + b: Config, + ) => Promise; + } + ).maybeOverrideContentGenerator.bind(manager); + return fn(config, base); + } + + it('inherits branch: returns a Config whose registry is distinct from the parent and binds Edit/Read to the override', async () => { + const parent = new Config(baseParams); + const parentRegistry = await parent.createToolRegistry(undefined, { + skipDiscovery: true, + }); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (parent as any).toolRegistry = parentRegistry; + + const manager = new SubagentManager(parent); + + const subagentConfig: SubagentConfig = { + name: 'inheriting-agent', + description: 'Inherits parent model', + systemPrompt: 'You are a helpful assistant.', + level: 'project', + filePath: '/test/project/.qwen/agents/inheriting-agent.md', + // model omitted -> inherits=true branch + }; + + const child = await callMaybeOverride(manager, subagentConfig, parent); + + expect(child).not.toBe(parent); + expect(child.getToolRegistry()).not.toBe(parentRegistry); + + const childEdit = await child.getToolRegistry().ensureTool(ToolNames.EDIT); + const childRead = await child + .getToolRegistry() + .ensureTool(ToolNames.READ_FILE); + + expect(childEdit).toBeInstanceOf(EditTool); + expect(childRead).toBeInstanceOf(ReadFileTool); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect((childEdit as any).config).toBe(child); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect((childRead as any).config).toBe(child); + + // The bound tool's FileReadCache must be the child's, not the parent's. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const boundConfig = (childEdit as any).config as Config; + expect(boundConfig.getFileReadCache()).toBe(child.getFileReadCache()); + expect(boundConfig.getFileReadCache()).not.toBe(parent.getFileReadCache()); + }); + + it('inherits branch: parent and child caches are independent', async () => { + const parent = new Config(baseParams); + const parentRegistry = await parent.createToolRegistry(undefined, { + skipDiscovery: true, + }); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (parent as any).toolRegistry = parentRegistry; + + const manager = new SubagentManager(parent); + const subagentConfig: SubagentConfig = { + name: 'inheriting-agent', + description: 'Inherits parent model', + systemPrompt: 'You are a helpful assistant.', + level: 'project', + filePath: '/test/project/.qwen/agents/inheriting-agent.md', + }; + + const child = await callMaybeOverride(manager, subagentConfig, parent); + + // Record a read on parent. Child must not see it. + const fakeStats = { + dev: 1, + ino: 100, + mtimeMs: 1_000_000, + size: 42, + } as unknown as import('node:fs').Stats; + + parent.getFileReadCache().recordRead('/tmp/parent.ts', fakeStats, { + full: true, + cacheable: true, + }); + + expect(parent.getFileReadCache().size()).toBe(1); + expect(child.getFileReadCache().size()).toBe(0); + }); + + it('inherits branch: skips rebuild and inherits registry via prototype when the base already has its own registry (real-world chained-override case)', async () => { + // This mirrors the real-world flow: agent.ts wraps the parent in + // `createApprovalModeOverride` (which builds R1 on the wrapper), + // then passes that wrapper — sometimes wrapped one more level in + // `bgConfig = Object.create(agentConfig)` for the background path — + // through `createAgentHeadless` → `maybeOverrideContentGenerator`. + // We do NOT want the second layer to build a redundant R2 — that + // would (a) waste work, (b) leak listeners on every later + // AgentTool/SkillTool factory invocation, and (c) split the cache + // so client-level clears target an empty R2 cache while the bound + // tools (still in R1) keep using R1's. + // + // Detection is via the `TOOL_REGISTRY_REBUILT` symbol marker that + // `createApprovalModeOverride` sets on its return value; Symbol + // property lookup walks the prototype chain so even an Object.create + // wrapper above the rebuilt Config is correctly recognised as + // having an upstream rebuild. + const parent = new Config(baseParams); + const parentRegistry = await parent.createToolRegistry(undefined, { + skipDiscovery: true, + }); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (parent as any).toolRegistry = parentRegistry; + + // Layer 1: actual createApprovalModeOverride (sets the marker). + const upstreamWrapper = await createApprovalModeOverride( + parent, + ApprovalMode.AUTO_EDIT, + ); + const upstreamRegistry = upstreamWrapper.getToolRegistry(); + + // Layer 2: simulate `bgConfig = Object.create(agentConfig)` from + // the background path — own properties added on this layer should + // not hide the marker on the prototype. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const bgWrapper = Object.create(upstreamWrapper) as any; + bgWrapper.getShouldAvoidPermissionPrompts = () => true; + + const manager = new SubagentManager(parent); + const subagentConfig: SubagentConfig = { + name: 'inheriting-agent', + description: 'Inherits parent model', + systemPrompt: 'You are a helpful assistant.', + level: 'project', + filePath: '/test/project/.qwen/agents/inheriting-agent.md', + }; + + const child = await callMaybeOverride( + manager, + subagentConfig, + bgWrapper as Config, + ); + + // child is still a distinct instance (Object.create) so the + // FileReadCache lazy-init still works, but its registry must + // resolve via the prototype back to upstreamRegistry — we did not + // build a new one. + expect(child).not.toBe(bgWrapper); + expect(child.getToolRegistry()).toBe(upstreamRegistry); + + // Critically: tools the model later instantiates from the registry + // are bound to upstreamWrapper, NOT the second-layer child. That + // is what the optimization is for — the bound tool still resolves + // `this.config.getFileReadCache()` to upstreamWrapper's cache, + // which is the cache the rest of the subagent execution actually + // uses. + const childEdit = await child.getToolRegistry().ensureTool(ToolNames.EDIT); + expect(childEdit).toBeInstanceOf(EditTool); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect((childEdit as any).config).toBe(upstreamWrapper); + }); + + it('non-inherit branch (explicit-model selector): rebuilds registry and binds Edit/Read to the override Config', async () => { + // The non-inherit branch swaps the ContentGenerator (so the + // subagent talks to the model the selector requests). It must + // ALSO rebuild the tool registry — without that step explicit-model + // subagents would still resolve their core tools' `this.config` to + // the parent and read the parent's FileReadCache. + const parent = new Config(baseParams); + // Even though bare mode skips most tools, the non-inherit branch + // requires getContentGeneratorConfig() to return something for the + // authType fallback. Stub it minimally. + vi.spyOn(parent, 'getContentGeneratorConfig').mockReturnValue({ + model: 'parent-model', + authType: 'openai', + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any); + const parentRegistry = await parent.createToolRegistry(undefined, { + skipDiscovery: true, + }); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (parent as any).toolRegistry = parentRegistry; + + const manager = new SubagentManager(parent); + const subagentConfig: SubagentConfig = { + name: 'explicit-model-agent', + description: 'Uses an explicit model selector', + systemPrompt: 'You are a helpful assistant.', + level: 'project', + filePath: '/test/project/.qwen/agents/explicit-model-agent.md', + // Bare model ID -> non-inherits branch (parses to {modelId, + // inherits:false}). + model: 'override-model', + }; + + const child = await callMaybeOverride(manager, subagentConfig, parent); + + expect(child).not.toBe(parent); + expect(child.getToolRegistry()).not.toBe(parentRegistry); + + const childEdit = await child.getToolRegistry().ensureTool(ToolNames.EDIT); + const childRead = await child + .getToolRegistry() + .ensureTool(ToolNames.READ_FILE); + + expect(childEdit).toBeInstanceOf(EditTool); + expect(childRead).toBeInstanceOf(ReadFileTool); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect((childEdit as any).config).toBe(child); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect((childRead as any).config).toBe(child); + + // The bound EditTool's FileReadCache must be the override's, not + // the parent's. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const boundConfig = (childEdit as any).config as Config; + expect(boundConfig.getFileReadCache()).toBe(child.getFileReadCache()); + expect(boundConfig.getFileReadCache()).not.toBe(parent.getFileReadCache()); + }); + + it('non-inherit branch: skips rebuild when an upstream wrapper has already rebuilt the registry', async () => { + const parent = new Config(baseParams); + vi.spyOn(parent, 'getContentGeneratorConfig').mockReturnValue({ + model: 'parent-model', + authType: 'openai', + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any); + const parentRegistry = await parent.createToolRegistry(undefined, { + skipDiscovery: true, + }); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (parent as any).toolRegistry = parentRegistry; + + const upstreamWrapper = await createApprovalModeOverride( + parent, + ApprovalMode.AUTO_EDIT, + ); + const upstreamRegistry = upstreamWrapper.getToolRegistry(); + + const manager = new SubagentManager(parent); + const subagentConfig: SubagentConfig = { + name: 'explicit-model-agent', + description: 'Uses an explicit model selector', + systemPrompt: 'You are a helpful assistant.', + level: 'project', + filePath: '/test/project/.qwen/agents/explicit-model-agent.md', + model: 'override-model', + }; + + const child = await callMaybeOverride( + manager, + subagentConfig, + upstreamWrapper, + ); + + // Upstream rebuild was detected via the symbol marker, so the + // override has no own registry — it inherits via the prototype. + expect(child.getToolRegistry()).toBe(upstreamRegistry); + + // Bound tools resolve to upstreamWrapper, not the second-layer + // child — same as the inherits branch's chained-override case. + const childEdit = await child.getToolRegistry().ensureTool(ToolNames.EDIT); + expect(childEdit).toBeInstanceOf(EditTool); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect((childEdit as any).config).toBe(upstreamWrapper); + }); + + it('inherits branch: the override approval mode (inherited via prototype) still resolves via the override Config', async () => { + const parent = new Config(baseParams); + const parentRegistry = await parent.createToolRegistry(undefined, { + skipDiscovery: true, + }); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (parent as any).toolRegistry = parentRegistry; + + const manager = new SubagentManager(parent); + const subagentConfig: SubagentConfig = { + name: 'inheriting-agent', + description: 'Inherits parent model', + systemPrompt: 'You are a helpful assistant.', + level: 'project', + filePath: '/test/project/.qwen/agents/inheriting-agent.md', + }; + + const child = await callMaybeOverride(manager, subagentConfig, parent); + + // Child has no own getApprovalMode; falls through prototype to parent. + // Verify mutating parent's mode via setter is observed by child. + parent.setApprovalMode(ApprovalMode.AUTO_EDIT); + expect(child.getApprovalMode()).toBe(ApprovalMode.AUTO_EDIT); + + // And the bound EditTool sees the same mode. + const childEdit = await child.getToolRegistry().ensureTool(ToolNames.EDIT); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const boundConfig = (childEdit as any).config as Config; + expect(boundConfig.getApprovalMode()).toBe(ApprovalMode.AUTO_EDIT); + }); +}); diff --git a/packages/core/src/subagents/subagent-manager.test.ts b/packages/core/src/subagents/subagent-manager.test.ts index 649d50f563c..06c37672bc4 100644 --- a/packages/core/src/subagents/subagent-manager.test.ts +++ b/packages/core/src/subagents/subagent-manager.test.ts @@ -72,6 +72,11 @@ describe('SubagentManager', () => { { name: 'write_file', displayName: 'Write File' }, { name: 'grep', displayName: 'Search Files' }, ]), + // `maybeOverrideContentGenerator` now rebuilds the tool registry on + // its override and copies discovered tools from this parent + // registry. The real implementation iterates `source.tools.values()`, + // so the stub needs a `tools` Map to avoid a TypeError. + tools: new Map(), } as unknown as ToolRegistry; // Create mock Config object using test utility diff --git a/packages/core/src/subagents/subagent-manager.ts b/packages/core/src/subagents/subagent-manager.ts index 011f191e059..d18b519e5b2 100644 --- a/packages/core/src/subagents/subagent-manager.ts +++ b/packages/core/src/subagents/subagent-manager.ts @@ -48,6 +48,10 @@ import { parseSubagentModelSelection } from './model-selection.js'; const debugLogger = createDebugLogger('SUBAGENT_MANAGER'); import { BuiltinAgentRegistry } from './builtin-agents.js'; import { ToolDisplayNamesMigration } from '../tools/tool-names.js'; +import { + hasRebuiltToolRegistry, + rebuildToolRegistryOnOverride, +} from '../tools/agent/agent.js'; const QWEN_CONFIG_DIR = '.qwen'; const AGENT_CONFIG_DIR = 'agents'; @@ -701,22 +705,38 @@ export class SubagentManager { base: Config, ): Promise { const selection = parseSubagentModelSelection(config.model); + // Skip the registry rebuild if any wrapper above `base` already + // rebuilt one (typically `agent.ts:createApprovalModeOverride`, + // which marks itself via Symbol-keyed flag — Symbol property lookup + // walks the prototype chain, so this also catches + // wrapper-on-wrapper layering like + // `bgConfig = Object.create(agentConfig)` passed in from the + // background path). Rebuilding a second time would waste work, + // leak listeners on shared managers (any AgentTool / SkillTool the + // second registry later instantiates registers a change-listener + // and the short-lived registry has no explicit stop() site), and + // split the cache so client-level cache clears target an empty + // second-layer cache while bound tools (still in the upstream + // layer's registry) keep using the upstream cache. + const upstreamRebuilt = hasRebuiltToolRegistry(base); + if (selection.inherits) { // Thin prototype-delegation override: no method changes, but a // distinct instance triggers the lazy-init in // `Config.getFileReadCache()` so the subagent gets its own // cache rather than inheriting the parent's. // - // Same caveat as in `agent.ts:createApprovalModeOverride`: the - // tool registry was bound on the parent at initialise time, so - // tool invocations still resolve `this.config` to the parent - // and reach the parent's cache. `InProcessBackend.createPerAgentConfig` - // already rebuilds the registry via `override.createToolRegistry()` - // + `copyDiscoveredToolsFrom(base.getToolRegistry())`; doing - // that here is the follow-up that closes the bound-tool path. - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const isolated = Object.create(base) as any; - return isolated as Config; + // When no upstream rebuild has happened, also rebuild the tool + // registry so `EditTool` / `WriteFileTool` / `ReadFileTool` are + // bound to the override and resolve `this.config` to the subagent + // — without that step, the parent's cached tool instances still + // reach the parent's FileReadCache and silently weaken prior-read + // enforcement on the subagent's mutation paths. + const isolated = Object.create(base) as Config; + if (!upstreamRebuilt) { + await rebuildToolRegistryOnOverride(isolated, base); + } + return isolated; } const authType = @@ -745,6 +765,14 @@ export class SubagentManager { agentGeneratorConfig.authType; override.getModel = (): string => agentGeneratorConfig.model; + // Rebuild the tool registry on the override so core tools resolve + // `this.config` to the subagent — but only if the upstream caller + // did not already build one. See the comment at the top of this + // function for the reasoning. + if (!upstreamRebuilt) { + await rebuildToolRegistryOnOverride(override as Config, base); + } + debugLogger.info( `Created per-agent ContentGenerator for subagent "${config.name}": authType=${authType}, model=${agentGeneratorConfig.model}`, ); diff --git a/packages/core/src/tools/agent/agent-override.test.ts b/packages/core/src/tools/agent/agent-override.test.ts new file mode 100644 index 00000000000..6185ce3a9b7 --- /dev/null +++ b/packages/core/src/tools/agent/agent-override.test.ts @@ -0,0 +1,291 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect } from 'vitest'; +import { Config, ApprovalMode } from '../../config/config.js'; +import { + createApprovalModeOverride, + hasRebuiltToolRegistry, + rebuildToolRegistryOnOverride, + TOOL_REGISTRY_REBUILT, +} from './agent.js'; +import { ToolNames } from '../tool-names.js'; +import { EditTool } from '../edit.js'; +import { WriteFileTool } from '../write-file.js'; +import { ReadFileTool } from '../read-file.js'; + +/** + * Regression: Object.create(parent) is not enough to isolate a subagent's + * core tools. The parent's tool registry caches `EditTool` / + * `WriteFileTool` / `ReadFileTool` instances bound at parent-init time + * with `this.config = parent`, so any subagent that walks up the + * prototype chain to read `getToolRegistry()` ends up invoking those + * parent-bound tools — which then read FileReadCache / approval mode + * from the parent rather than the subagent. + * + * `createApprovalModeOverride` must rebuild the registry on the override + * Config so the core tools resolve `this.config` to the override. + */ +describe('createApprovalModeOverride bound-tool isolation', () => { + // Use bare mode so createToolRegistry() registers only ReadFile / Edit / + // Shell — keeps the test focused on the bound-tool path without dragging + // in optional tools that may need extra setup (LSP, ripgrep, MCP, …). + const baseParams = { + cwd: '/tmp', + targetDir: '/tmp', + debugMode: false, + model: 'test-model', + usageStatisticsEnabled: false, + bareMode: true, + }; + + it('returns a Config whose registry is a distinct instance from the parent', async () => { + const parent = new Config(baseParams); + const parentRegistry = await parent.createToolRegistry(undefined, { + skipDiscovery: true, + }); + // Parent's getToolRegistry() is what subagents would walk through if + // we did NOT rebuild — make it return parentRegistry so the comparison + // is meaningful. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (parent as any).toolRegistry = parentRegistry; + + const child = await createApprovalModeOverride( + parent, + ApprovalMode.AUTO_EDIT, + ); + const childRegistry = child.getToolRegistry(); + + expect(childRegistry).toBeDefined(); + expect(childRegistry).not.toBe(parentRegistry); + }); + + it('binds Edit / WriteFile / ReadFile on the override registry to the override Config, not the parent', async () => { + const parent = new Config(baseParams); + const parentRegistry = await parent.createToolRegistry(undefined, { + skipDiscovery: true, + }); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (parent as any).toolRegistry = parentRegistry; + + const child = await createApprovalModeOverride( + parent, + ApprovalMode.AUTO_EDIT, + ); + const childRegistry = child.getToolRegistry(); + + // Force lazy factories to instantiate their tools on both registries. + const parentEdit = await parentRegistry.ensureTool(ToolNames.EDIT); + const childEdit = await childRegistry.ensureTool(ToolNames.EDIT); + const parentRead = await parentRegistry.ensureTool(ToolNames.READ_FILE); + const childRead = await childRegistry.ensureTool(ToolNames.READ_FILE); + + expect(parentEdit).toBeInstanceOf(EditTool); + expect(childEdit).toBeInstanceOf(EditTool); + expect(parentRead).toBeInstanceOf(ReadFileTool); + expect(childRead).toBeInstanceOf(ReadFileTool); + + // The crux: parent-bound tool resolves to parent, child-bound tool + // resolves to child. The parent and child are distinct Config + // instances, so this also implies their FileReadCaches and + // ApprovalModes are independent. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect((parentEdit as any).config).toBe(parent); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect((childEdit as any).config).toBe(child); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect((parentRead as any).config).toBe(parent); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect((childRead as any).config).toBe(child); + }); + + it('routes child tools through the child FileReadCache, not the parent', async () => { + const parent = new Config(baseParams); + const parentRegistry = await parent.createToolRegistry(undefined, { + skipDiscovery: true, + }); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (parent as any).toolRegistry = parentRegistry; + + const child = await createApprovalModeOverride( + parent, + ApprovalMode.AUTO_EDIT, + ); + const childRegistry = child.getToolRegistry(); + + const childEdit = await childRegistry.ensureTool(ToolNames.EDIT); + expect(childEdit).toBeInstanceOf(EditTool); + + // The bound tool's `this.config.getFileReadCache()` must resolve to + // the child's lazy own-property cache, not the parent's. We don't + // call EditTool's execute here (it would reach the filesystem); we + // just observe that the cache instance the bound tool would touch + // is the child's, not the parent's. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const boundConfig = (childEdit as any).config as Config; + expect(boundConfig.getFileReadCache()).toBe(child.getFileReadCache()); + expect(boundConfig.getFileReadCache()).not.toBe(parent.getFileReadCache()); + }); + + it('preserves the override approval mode on the bound tools', async () => { + const parent = new Config(baseParams); + const parentRegistry = await parent.createToolRegistry(undefined, { + skipDiscovery: true, + }); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (parent as any).toolRegistry = parentRegistry; + + expect(parent.getApprovalMode()).toBe(ApprovalMode.DEFAULT); + + const child = await createApprovalModeOverride( + parent, + ApprovalMode.YOLO, + ); + expect(child.getApprovalMode()).toBe(ApprovalMode.YOLO); + + const childEdit = await child + .getToolRegistry() + .ensureTool(ToolNames.EDIT); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const boundConfig = (childEdit as any).config as Config; + expect(boundConfig.getApprovalMode()).toBe(ApprovalMode.YOLO); + }); + + it('copies discovered tools from the parent registry without re-discovering', async () => { + const parent = new Config(baseParams); + const parentRegistry = await parent.createToolRegistry(undefined, { + skipDiscovery: true, + }); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (parent as any).toolRegistry = parentRegistry; + + // Bare mode keeps the parent registry small; this test mostly + // guards that copyDiscoveredToolsFrom is invoked. We verify the + // hook is reachable by introspecting the parent registry first. + const beforeNames = parentRegistry.getAllToolNames().sort(); + + const child = await createApprovalModeOverride( + parent, + ApprovalMode.AUTO_EDIT, + ); + // Force registration of all lazy factories on the child so + // getAllToolNames() reflects core tools too. (Without warming, only + // already-resolved tools and discovered tools show up.) + await child.getToolRegistry().warmAll(); + await parentRegistry.warmAll(); + + const childNames = child.getToolRegistry().getAllToolNames().sort(); + + // After warmAll the core tool sets must match — the child registry + // is built from the same Config (just the override), and we copied + // any discovered tools across. So the name set should equal parent's. + expect(childNames).toEqual(parentRegistry.getAllToolNames().sort()); + // And the parent's pre-warm names must be a subset of the post-warm + // names — sanity check that warmAll didn't lose anything. + const beforeSet = new Set(beforeNames); + for (const name of beforeSet) { + expect(childNames).toContain(name); + } + + // Sanity: WriteFile is registered in non-bare mode only, so bare mode + // should NOT have it. + expect(childNames).not.toContain(ToolNames.WRITE_FILE); + + // Spy-side check via plain reflection: ensure WriteFile import path + // is wired correctly by switching to non-bare and re-running. + const parentNonBare = new Config({ ...baseParams, bareMode: false }); + const parentNonBareRegistry = await parentNonBare.createToolRegistry( + undefined, + { skipDiscovery: true }, + ); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (parentNonBare as any).toolRegistry = parentNonBareRegistry; + + const childNonBare = await createApprovalModeOverride( + parentNonBare, + ApprovalMode.AUTO_EDIT, + ); + const childNonBareWrite = await childNonBare + .getToolRegistry() + .ensureTool(ToolNames.WRITE_FILE); + expect(childNonBareWrite).toBeInstanceOf(WriteFileTool); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect((childNonBareWrite as any).config).toBe(childNonBare); + }); + + describe('TOOL_REGISTRY_REBUILT marker propagation', () => { + // Reviewer raised a concern that + // `Object.prototype.hasOwnProperty.call(base, 'getToolRegistry')` + // returns false when `base` is an Object.create wrapper above the + // rebuilt Config (e.g. `bgConfig = Object.create(agentConfig)`), + // causing a redundant rebuild. Switching to a Symbol-keyed marker + // fixes that because Symbol property reads walk the prototype + // chain through normal lookup. These tests pin that contract. + + it('hasRebuiltToolRegistry returns true even when checked on an Object.create wrapper above the rebuilt Config', async () => { + const parent = new Config(baseParams); + const parentRegistry = await parent.createToolRegistry(undefined, { + skipDiscovery: true, + }); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (parent as any).toolRegistry = parentRegistry; + + const upstream = await createApprovalModeOverride( + parent, + ApprovalMode.AUTO_EDIT, + ); + expect(hasRebuiltToolRegistry(upstream)).toBe(true); + + // bgConfig pattern: Object.create wrapper above the rebuilt + // Config, with a method override layered on top. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const bgWrapper = Object.create(upstream) as any; + bgWrapper.getShouldAvoidPermissionPrompts = () => true; + + // The plain own-property check would miss this — Symbol lookup + // doesn't. + expect( + Object.prototype.hasOwnProperty.call(bgWrapper, 'getToolRegistry'), + ).toBe(false); + expect(hasRebuiltToolRegistry(bgWrapper as Config)).toBe(true); + }); + + it('hasRebuiltToolRegistry returns false on a fresh Config and on a wrapper that was not rebuilt', () => { + const parent = new Config(baseParams); + expect(hasRebuiltToolRegistry(parent)).toBe(false); + + // Plain Object.create wrapper without a rebuild — must still + // report false so the downstream caller knows it has to rebuild. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const plainWrapper = Object.create(parent) as any; + plainWrapper.getApprovalMode = () => ApprovalMode.AUTO_EDIT; + expect(hasRebuiltToolRegistry(plainWrapper as Config)).toBe(false); + }); + + it('rebuildToolRegistryOnOverride installs the marker and an own getToolRegistry', async () => { + const parent = new Config(baseParams); + const parentRegistry = await parent.createToolRegistry(undefined, { + skipDiscovery: true, + }); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (parent as any).toolRegistry = parentRegistry; + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const override = Object.create(parent) as any; + override.getApprovalMode = () => ApprovalMode.YOLO; + await rebuildToolRegistryOnOverride(override as Config, parent); + + expect( + Object.prototype.hasOwnProperty.call(override, 'getToolRegistry'), + ).toBe(true); + expect( + Object.prototype.hasOwnProperty.call(override, TOOL_REGISTRY_REBUILT), + ).toBe(true); + expect(override[TOOL_REGISTRY_REBUILT]).toBe(true); + expect(hasRebuiltToolRegistry(override as Config)).toBe(true); + }); + }); +}); diff --git a/packages/core/src/tools/agent/agent.test.ts b/packages/core/src/tools/agent/agent.test.ts index 150b0f03c94..ba430bc449e 100644 --- a/packages/core/src/tools/agent/agent.test.ts +++ b/packages/core/src/tools/agent/agent.test.ts @@ -111,6 +111,19 @@ describe('AgentTool', () => { queueMessage: vi.fn(), appendActivity: vi.fn(), }; + // Stub registry exposed on both `parent.getToolRegistry()` and the + // override built by `createApprovalModeOverride`. The override path + // calls `createToolRegistry` on the override Config (Object.create + // walks the prototype chain to this mock) and then + // `copyDiscoveredToolsFrom(parent.getToolRegistry())`. Without these + // mocks the override helper throws and every subagent test that + // exercises foreground execution fails. + const stubToolRegistry = { + copyDiscoveredToolsFrom: vi.fn(), + getAllTools: vi.fn().mockReturnValue([]), + getAllToolNames: vi.fn().mockReturnValue([]), + stop: vi.fn().mockResolvedValue(undefined), + }; config = { getProjectRoot: vi.fn().mockReturnValue('/test/project'), getSessionId: vi.fn().mockReturnValue('test-session-id'), @@ -122,6 +135,8 @@ describe('AgentTool', () => { getApprovalMode: vi.fn().mockReturnValue('default'), isTrustedFolder: vi.fn().mockReturnValue(true), getBackgroundTaskRegistry: vi.fn().mockReturnValue(stubRegistry), + getToolRegistry: vi.fn().mockReturnValue(stubToolRegistry), + createToolRegistry: vi.fn().mockResolvedValue(stubToolRegistry), } as unknown as Config; changeListeners = []; diff --git a/packages/core/src/tools/agent/agent.ts b/packages/core/src/tools/agent/agent.ts index 4adf21c51dd..97150ebf9ea 100644 --- a/packages/core/src/tools/agent/agent.ts +++ b/packages/core/src/tools/agent/agent.ts @@ -167,14 +167,82 @@ function permissionModeToApprovalMode(mode: PermissionMode): ApprovalMode { } } +/** + * Marker that signals "this Config wrapper has rebuilt its own tool + * registry so bound EditTool / WriteFileTool / ReadFileTool resolve to + * the wrapper instead of the parent". Stored as a Symbol-keyed property + * so that JavaScript's normal property lookup (which walks the + * prototype chain) lets a downstream wrapper detect a rebuild that + * happened on any ancestor without manually walking the chain. + * + * `Symbol.for` is used so the marker survives bundle-deduping; two + * independent imports of this module observe the same Symbol identity. + */ +export const TOOL_REGISTRY_REBUILT: unique symbol = Symbol.for( + 'qwen-code:tool-registry-rebuilt', +); + +/** + * `true` if any Config in this wrapper's prototype chain has already + * rebuilt its tool registry via {@link rebuildToolRegistryOnOverride}. + * + * Used by spawn sites that may be called with a wrapper-on-wrapper + * argument (e.g. `subagent-manager.ts:maybeOverrideContentGenerator` + * receiving `bgConfig = Object.create(agentConfig)` from the + * background-agent path) to skip a redundant rebuild. + */ +export function hasRebuiltToolRegistry(config: Config): boolean { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return (config as any)[TOOL_REGISTRY_REBUILT] === true; +} + +/** + * Rebuilds the tool registry on `override` so core tools resolve + * `this.config` to `override` instead of `base`. Used by both + * {@link createApprovalModeOverride} and + * `subagent-manager.ts:maybeOverrideContentGenerator` to avoid + * duplicated rebuild logic. + * + * - `override.createToolRegistry(...)` runs on the override (so the + * lazy factories close over `this = override`). + * - Discovered tools (MCP / command-discovered) are copied from `base` + * rather than re-discovered, since discovery is expensive. + * - The {@link TOOL_REGISTRY_REBUILT} marker is set so wrapper-of-wrapper + * layers downstream skip the rebuild via {@link hasRebuiltToolRegistry}. + */ +export async function rebuildToolRegistryOnOverride( + override: Config, + base: Config, +): Promise { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const ov = override as any; + const agentRegistry = await ov.createToolRegistry(undefined, { + skipDiscovery: true, + }); + agentRegistry.copyDiscoveredToolsFrom(base.getToolRegistry()); + ov.getToolRegistry = () => agentRegistry; + ov[TOOL_REGISTRY_REBUILT] = true; +} + /** * Creates a Config override with a different approval mode. - * Uses prototype delegation to avoid mutating the parent config. + * + * Uses prototype delegation (Object.create) to avoid mutating the parent + * config, then delegates to {@link rebuildToolRegistryOnOverride} so the + * override's tool registry has core tools bound to the override rather + * than to the parent. Without that rebuild, the parent's cached tool + * instances continue to resolve `this.config` to the parent, defeating + * per-Config isolation of FileReadCache / approval mode for any code + * path that goes through the bound tool. */ -function createApprovalModeOverride(base: Config, mode: ApprovalMode): Config { +export async function createApprovalModeOverride( + base: Config, + mode: ApprovalMode, +): Promise { // eslint-disable-next-line @typescript-eslint/no-explicit-any const override = Object.create(base) as any; override.getApprovalMode = (): ApprovalMode => mode; + await rebuildToolRegistryOnOverride(override as Config, base); return override as Config; } @@ -996,21 +1064,14 @@ class AgentToolInvocation extends BaseToolInvocation { // `this.config` directly here would short-circuit that // isolation for the same-mode path, which is the common case. // - // Known partial fix: `Config.createToolRegistry` (called once at - // initialise time on the parent) bound `EditTool` / `WriteFileTool` - // instances against the parent. The subagent's - // `runtimeContext.getToolRegistry()` walks the prototype chain - // back to that parent registry, so tool invocations resolve - // `this.config` to the parent and reach the parent's - // FileReadCache rather than the wrapper's lazy-init one. - // `InProcessBackend.createPerAgentConfig` already does the right - // thing (`override.createToolRegistry()` + `copyDiscoveredToolsFrom`); - // bringing that here is a follow-up. Pre-PR there was no - // enforcement on subagent mutations at all, so the wrapper here - // is still strictly an improvement (the cache lazy-init does - // shield code that *consumes the Config directly* rather than - // through a parent-bound tool). - const agentConfig = createApprovalModeOverride( + // The override also rebuilds its own tool registry so core + // tools (`EditTool` / `WriteFileTool` / `ReadFileTool`) are + // bound to the override Config rather than the parent. Without + // that rebuild, the parent's cached tool instances continue to + // resolve `this.config` to the parent, reaching the parent's + // FileReadCache rather than the subagent's. See + // `createApprovalModeOverride` above for details. + const agentConfig = await createApprovalModeOverride( this.config, resolvedApprovalMode, ); @@ -1332,6 +1393,16 @@ class AgentToolInvocation extends BaseToolInvocation { bgEmitter.off(AgentEventType.TOOL_CALL, onToolCall); bgEmitter.off(AgentEventType.USAGE_METADATA, onUsageMetadata); cleanupJsonl?.(); + // Release the per-subagent ToolRegistry now that the + // background agent has finished — see the matching call in + // the foreground finally for why. Stopping here, after + // bgSubagent.execute resolves, is safe: by this point the + // detached body cannot invoke any more tool factories on + // this registry. + void agentConfig + .getToolRegistry() + .stop() + .catch(() => {}); } }; // Wrap in the agent-identity frame so nested `agent` tool calls @@ -1492,6 +1563,17 @@ class AgentToolInvocation extends BaseToolInvocation { // this in finally guarantees we clean up on success, failure, // cancel, AND any unexpected throw inside runFramed. registry.unregisterForeground(hookOpts.agentId); + // Release the per-subagent ToolRegistry so any AgentTool / + // SkillTool the model instantiated during execution disposes + // its change-listeners on shared SubagentManager / SkillManager. + // Without this, repeated foreground subagent runs accumulate + // listeners for the rest of the session. Fire-and-forget; the + // subagent has already returned its result, and stop() logs its + // own errors. + void agentConfig + .getToolRegistry() + .stop() + .catch(() => {}); } } catch (error) { const errorMessage =