Skip to content

Commit 7124c6c

Browse files
wenshaowenshao
authored
fix(core): rebuild tool registry on subagent Config overrides so bound tools resolve to the subagent (#3873)
* fix(core): rebuild tool registry on subagent Config overrides so bound tools resolve to the subagent PR-B (#3774) added per-Config FileReadCache isolation via Object.create overrides at two subagent spawn sites — agent.ts:createApprovalModeOverride and subagent-manager.ts:maybeOverrideContentGenerator. The override shielded code that read FileReadCache directly through the Config instance, but missed the bound-tool path: Config.createToolRegistry runs once at parent initialise time, so the parent's EditTool / WriteFileTool / ReadFileTool instances are bound with `this.config = parent`. The subagent's Object.create wrapper inherited getToolRegistry via the prototype chain, reaching the parent registry whose bound tools then read FileReadCache and approval mode from the parent. This change closes that gap by rebuilding the tool registry on the override at both sites — the same pattern InProcessBackend.createPerAgentConfig already uses: - override.createToolRegistry(undefined, { skipDiscovery: true }) - registry.copyDiscoveredToolsFrom(base.getToolRegistry()) - override.getToolRegistry = () => registry createApprovalModeOverride becomes async; its single call site already ran inside an async block. maybeOverrideContentGenerator skips the rebuild when the upstream Config already has its own getToolRegistry (real-world case: agent.ts wrapper passed through createAgentHeadless), avoiding wasted work, listener accumulation on shared SubagentManager / SkillManager, and a cache split where the bound tools' registry layer diverges from the runtime context's lazy-init cache. Includes regression tests in agent-override.test.ts and subagent-manager-override.test.ts that exercise the bound-tool path: they instantiate the lazy factories on the override registry and assert that EditTool / WriteFileTool / ReadFileTool resolve this.config to the override Config (and thus to the override's FileReadCache / approval mode), not the parent. * fix(core): close bound-tool gap on resumed background agents too Follow-up audit on PR #3873 surfaced a duplicate, pre-rebuild copy of `createApprovalModeOverride` living in `background-agent-resume.ts` (L142-150). Resumed fork agents go through `createResumedForkSubagent` which bypasses `SubagentManager.maybeOverrideContentGenerator` (where the registry rebuild now lives), so the resumed fork's `EditTool` / `WriteFileTool` / `ReadFileTool` were still resolving `this.config` to the parent and reading the parent's `FileReadCache`. The non-fork resume path went through `subagent-manager` and worked correctly only because `maybeOverrideContentGenerator` saw no upstream own-registry on `bgConfig` and rebuilt one — but with that fallback the fork path could never benefit. This change deletes the local copy and switches `background-agent-resume.ts` to import the now-async exported `createApprovalModeOverride` from `agent.ts`. Drops the previous `?: this.config` short-circuit so the resumed agent ALWAYS gets a wrapper Config — the same behaviour `agent.ts` already enforces; reusing the parent directly defeats the per-Config FileReadCache isolation. Updates `background-agent-resume.test.ts` mock config with the `createToolRegistry` / `getToolRegistry` stubs the rebuild path now exercises. * fix(core): address bound-tool isolation review feedback Three independent fixes from PR #3873 review feedback: 1. Switch the upstream-rebuild guard from `hasOwnProperty(base, 'getToolRegistry')` to a Symbol-keyed marker `TOOL_REGISTRY_REBUILT`. The own-property check missed the case where the override is reached via an Object.create wrapper above the rebuilt Config (e.g. `bgConfig = Object.create(agentConfig)` in the agent.ts background path) — it would falsely report "no upstream rebuild" and cause a redundant third rebuild that wastes work and doubles the listener-leak surface. Symbol property reads walk the prototype chain via normal lookup, so a marker stored on any ancestor is correctly observed. Extracts the shared rebuild logic into `rebuildToolRegistryOnOverride(override, base)` so the three spawn sites (agent.ts:createApprovalModeOverride, the inherits branch, the non-inherit branch) cannot drift apart. 2. Stop the per-subagent ToolRegistry in the lifecycle finally blocks: - agent.ts foreground finally (after the inner try wrapping `runFramed`) - agent.ts background bgBody finally (after `bgSubagent.execute` resolves) - background-agent-resume.ts resume body finally (same shape) Without this, every AgentTool / SkillTool the model instantiates from the per-subagent registry registers a change-listener on shared SubagentManager / SkillManager, and repeated subagent runs accumulate listeners for the rest of the session. Stop is fire-and-forget, matching `InProcessBackend.cleanup` and `stopAgent`. 3. Add bound-tool isolation tests for the non-inherit branch (explicit-model selector). The original PR only covered the inherits branch directly; the non-inherit branch now goes through the same helper, but a dedicated test pins `tool.config === override` and the FileReadCache binding so a regression cannot leave explicit-model subagents reading the parent's cache while existing model-override tests still pass. Tests now exercise: - Symbol marker propagation via Object.create chain (3 cases) - Non-inherit rebuild + bound-tool isolation - Non-inherit skip-rebuild when upstream wrapper has the marker - Pre-existing inherits / chained-override / approval-mode propagation - Mock configs in agent.test.ts / subagent-manager.test.ts / background-agent-resume.test.ts gain `stop` and `tools: Map` stubs to model the registry contract the override path now exercises. `npx vitest run packages/core/src` — 268 files / 6943 passed.
1 parent b9f56e4 commit 7124c6c

8 files changed

Lines changed: 862 additions & 41 deletions

File tree

‎packages/core/src/agents/background-agent-resume.test.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,17 @@ describe('BackgroundAgentResumeService', () => {
5858
fireSubagentStartEvent: vi.fn().mockResolvedValue(undefined),
5959
fireSubagentStopEvent: vi.fn().mockResolvedValue(undefined),
6060
};
61+
// Stub registry exposed on both `parent.getToolRegistry()` and the
62+
// override built by `createApprovalModeOverride` (which now rebuilds
63+
// the tool registry on the resumed agent's Config so bound tools
64+
// resolve to the resumed agent — see PR #3873). Without these
65+
// mocks the override helper throws and every resume test fails.
66+
const stubToolRegistry = {
67+
copyDiscoveredToolsFrom: vi.fn(),
68+
getAllTools: vi.fn().mockReturnValue([]),
69+
getAllToolNames: vi.fn().mockReturnValue([]),
70+
stop: vi.fn().mockResolvedValue(undefined),
71+
};
6172
const config = {
6273
storage: {
6374
getProjectDir: () => tempDir,
@@ -72,6 +83,8 @@ describe('BackgroundAgentResumeService', () => {
7283
getGeminiClient: () => undefined,
7384
getSkipStartupContext: () => true,
7485
getTranscriptPath: () => path.join(tempDir, 'session.jsonl'),
86+
getToolRegistry: () => stubToolRegistry,
87+
createToolRegistry: vi.fn().mockResolvedValue(stubToolRegistry),
7588
} as unknown as Config;
7689

7790
return {

‎packages/core/src/agents/background-agent-resume.ts‎

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@ import { getInitialChatHistory } from '../utils/environmentContext.js';
2828
import { getGitBranch } from '../utils/gitUtils.js';
2929
import { PermissionMode, type StopHookOutput } from '../hooks/types.js';
3030
import { runWithAgentContext } from '../tools/agent/agent-context.js';
31+
import { createApprovalModeOverride } from '../tools/agent/agent.js';
32+
import type { ApprovalMode } from '../config/config.js';
3133
import {
3234
FORK_AGENT,
3335
FORK_SUBAGENT_TYPE,
@@ -139,16 +141,6 @@ function reconcileResumedApprovalMode(
139141
return 'default';
140142
}
141143

142-
function createApprovalModeOverride(
143-
base: Config,
144-
mode: ApprovalModeValue,
145-
): Config {
146-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
147-
const override = Object.create(base) as any;
148-
override.getApprovalMode = () => mode;
149-
return override as Config;
150-
}
151-
152144
function persistBackgroundCancellation(
153145
metaPath: string,
154146
persistedStatus: 'running' | 'cancelled',
@@ -527,10 +519,18 @@ export class BackgroundAgentResumeService {
527519
parentApprovalMode,
528520
this.config.isTrustedFolder(),
529521
);
530-
const agentConfig =
531-
resolvedApprovalMode !== this.config.getApprovalMode()
532-
? createApprovalModeOverride(this.config, resolvedApprovalMode)
533-
: this.config;
522+
// Always wrap, even when the resolved approval mode matches the
523+
// parent's. The wrapper rebuilds the tool registry on the
524+
// override Config so bound `EditTool` / `WriteFileTool` /
525+
// `ReadFileTool` instances resolve `this.config` to the resumed
526+
// agent and use the resumed agent's `FileReadCache`, instead of
527+
// continuing to read the parent's. Reusing `this.config`
528+
// directly here would short-circuit that isolation. See the
529+
// matching wrapper in `agent.ts:createApprovalModeOverride`.
530+
const agentConfig = await createApprovalModeOverride(
531+
this.config,
532+
resolvedApprovalMode as ApprovalMode,
533+
);
534534
// eslint-disable-next-line @typescript-eslint/no-explicit-any
535535
const bgConfig = Object.create(agentConfig) as any;
536536
bgConfig.getShouldAvoidPermissionPrompts = () => true;
@@ -771,6 +771,16 @@ export class BackgroundAgentResumeService {
771771
bgEmitter.off(AgentEventType.TOOL_CALL, onToolCall);
772772
bgEmitter.off(AgentEventType.USAGE_METADATA, onUsageMetadata);
773773
cleanupJsonl?.();
774+
// Release the per-subagent ToolRegistry the resumed agent's
775+
// wrapper Config built in `createApprovalModeOverride` so any
776+
// AgentTool / SkillTool the model instantiated during this
777+
// run disposes its change-listeners on shared
778+
// SubagentManager / SkillManager. Without this, every resume
779+
// accumulates listeners for the rest of the session.
780+
void agentConfig
781+
.getToolRegistry()
782+
.stop()
783+
.catch(() => {});
774784
}
775785
};
776786

0 commit comments

Comments
 (0)