fix(core): rebuild tool registry on subagent Config overrides so bound tools resolve to the subagent#3873
Merged
Merged
Conversation
…d 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.
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
commented
May 6, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Critical (unmapped to diff): background-agent-resume.ts:142-150 — 该文件存在一个与 agent.ts 同名的本地 createApprovalModeOverride 函数，此 PR 将 agent.ts 中的导出版本升级为异步 + 注册表重建，但未触及此本地副本。Fork 恢复路径 (createResumedForkSubagent, L877) 将该函数的结果直接传给 AgentHeadless.create，绕过了 SubagentManager.maybeOverrideContentGenerator（注册表重建在此处）。恢复的 fork 子 agent 中的 EditTool/WriteFileTool/ReadFileTool 仍将 this.config 解析为父进程 Config，使用父进程的 FileReadCache。
修复建议: 删除本地副本，从 agent.ts 导入已更新的 createApprovalModeOverride，调用点改为 await。
— deepseek-v4-pro via Qwen Code /review
wenshao
commented
May 6, 2026
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.
Collaborator
Author
|
Addressed in
Also dropped the
中文
同时去掉了
|
wenshao
commented
May 6, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: self-PR reviews cannot be approved by the PR author. — gpt-5.5 via Qwen Code /review
wenshao
commented
May 6, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Test review from deepseek-v4-pro via Qwen Code /review
Collaborator
Author
Code Review — deepseek-v4-pro via Qwen CodeCriticalFileReadCache 分裂当 当上游
上下文压缩后缓存清空对工具无效，模型重新读文件时收到 建议修复: 当 Suggestion
VerdictRequest changes (downgraded to Comment — self-PR) — deepseek-v4-pro via Qwen Code /review |
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.
tanzhenxin
approved these changes
May 7, 2026
tanzhenxin
left a comment
Collaborator
There was a problem hiding this comment.
Review
The fix is correct: rebuilding the tool registry on the override Config binds the core file tools to the per-subagent state, and the Symbol.for-keyed rebuilt-marker correctly propagates through the Object.create wrapper chain. Tests exercise the bound-tool path with real Config + real ToolRegistry and assert the load-bearing invariant (bound tool's config === override). One incidental note: the title says "bound tools resolve to the subagent", but copyDiscoveredToolsFrom reference-copies discovered tools so MCP tools still hold this.config = parent. Almost certainly deliberate to avoid re-running discovery, but a one-line note in the description would clarify scope.
Issues
1. Foreground-fork path leaks the per-spawn registry (severity: low · confidence: very high)
The PR adds a per-spawn registry on every override and adds matching cleanup in three of the four spawn paths — foreground non-fork, background non-fork, and background fork. The foreground-fork branch is the odd one out: it fires the fork as a void promise and returns the placeholder result with no try/finally around the fork body, so the per-spawn registry it built is never stopped. AgentTool / SkillTool change-listeners on the shared subagent and skill managers accumulate per foreground-fork for the rest of the session. New regression introduced by this PR — pre-PR the agent config was reused so there was nothing to release. Worth a follow-up commit.
2. PR description is out of sync with the code (severity: low · confidence: very high)
The body still says the override helper has a single call site and that the chained-override skip uses a hasOwnProperty check. Neither matches HEAD: the helper is now called from both the agent tool and the background-agent resume path, and the skip uses a Symbol.for-keyed marker — explicitly chosen so prototype-chain wrappers (the bgConfig = Object.create(agentConfig) pattern) are recognised as already-rebuilt.
3. Adjacent Config-wrapper sites still have the original bug (severity: low · confidence: high)
Out of scope here, but flagging for follow-up. The yolo-config wrapper used by the forked-agent helper, the scoped-permission wrapper used by memory extraction, and the dream-agent planner's wrapper all build an override via Object.create and hand it directly to the headless agent — bypassing the subagent manager and therefore the rebuild. Their bound tools still resolve back to the parent, so the YOLO override and the scoped permission manager are silently ignored on any bound-tool path. Memory extraction is the worst case because it stacks both wrappers.
Verdict
APPROVE — the core fix is correct and well-tested. The three notes above are low-severity follow-ups; none should block merge.
TaimoorSiddiquiOfficial
pushed a commit
to TaimoorSiddiquiOfficial/HopCode
that referenced
this pull request
May 7, 2026
…d tools resolve to the subagent (QwenLM#3873) Cherry-picked from QwenLM/qwen-code@7124c6cba. Resolved: kept HopCode's settings override logic + added upstream's tool registry rebuild to fix subagent tool resolution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
tanzhenxin
pushed a commit
that referenced
this pull request
May 7, 2026
…#3887) Follow-up to #3873 review: the foreground-fork branch in `agent.ts` fires the fork body via `void runInForkContext(runFramedFork)` and returns the placeholder result synchronously, with no try/finally around the fork body. The other three spawn paths (foreground non-fork, background fork, background non-fork) added in #3873 already stop the per-subagent ToolRegistry in their finally blocks — this one was missed, so any AgentTool / SkillTool the fork's model later instantiated leaked its change-listener on the shared SubagentManager / SkillManager for the rest of the session. Wraps the inner body in `try { await runSubagentWithHooks(...) } finally { void agentConfig.getToolRegistry().stop().catch(() => {}) }` — same shape as the background bgBody finally added in #3873. Adds a regression test in `agent.test.ts` Fork dispatch describe that drains the detached fork body and asserts the stop spy was invoked exactly once.
3 tasks
wenshao
added a commit
that referenced
this pull request
May 8, 2026
* fix(core): close bound-tool gap on runForkedAgent's YOLO wrapper Follow-up to #3873 review (#3 of the three flagged adjacent Config-wrapper sites). `runForkedAgent`'s AgentHeadless path used to build its YOLO override via a local `Object.create(parent) + getApprovalMode = YOLO` helper that did NOT rebuild the tool registry, so: 1. The YOLO approval mode was silently ignored on the bound-tool path — parent's already-bound `EditTool` / `WriteFileTool` / `ReadFileTool` resolved `this.config.getApprovalMode()` back to the parent. 2. The fork's reads / mutations went through the parent's `FileReadCache` instead of a per-fork cache. 3. Memory-extraction and dream-agent paths stack the YOLO wrapper over a `getPermissionManager`-overriding scoped wrapper. Since the bound tools resolved to the parent, BOTH overrides — the YOLO approval mode AND the scoped permission manager — were bypassed. The fix routes through the existing `createApprovalModeOverride` helper, which: - rebuilds the tool registry on the wrapper (so bound tools resolve `this.config` to the wrapper), - copies discovered tools from the upstream registry, - sets the `TOOL_REGISTRY_REBUILT` Symbol marker so any further downstream wrapper layer recognises the rebuild and skips redundant work. The memory-extraction / dream-agent composition now resolves correctly via prototype walk — the YOLO wrapper sits above the scoped wrapper, so bound tools observe `getApprovalMode() = YOLO` on the wrapper itself and `getPermissionManager() = scopedPm` one prototype level up. Adds a try/finally around the AgentHeadless run so the per-fork ToolRegistry is stopped after execution — same shape as the spawn finallys in `agent.ts` and `background-agent-resume.ts`. Without this, every AgentTool / SkillTool the fork's model later instantiates leaks its change-listener on shared SubagentManager / SkillManager. Adds `forkedAgent.agent.test.ts` covering: marker + YOLO + distinct registry on the wrapper passed to AgentHeadless.create; bound EditTool resolves to the wrapper; memory-scoped composition preserves both YOLO and scopedPm; `stop()` fires after the AgentHeadless body finishes. Uses `vi.spyOn(AgentHeadless, 'create')` rather than module mocking so the real `ContextState` / `AgentEventEmitter` keep working. `npx vitest run packages/core/src` — 269 files / 6992 passed. * test(core): cover stop() lifecycle on AgentHeadless.create + execute failure paths Self-review feedback on #3892: the stop lifecycle test only covered the success path. A future refactor could move the stop() out of the `finally` block and onto the success branch, reintroducing listener leaks when create or execute rejects, while every existing test still passes. Two new tests pin the cleanup to the `finally`: 1. `stops the per-fork ToolRegistry even when AgentHeadless.create rejects` — make `AgentHeadless.create` return a rejected promise; assert the rejection propagates and the stop spy still fires once. 2. `stops the per-fork ToolRegistry even when headless.execute rejects` — return a headless object whose `execute` rejects; same shape. Together with the success-path test these three cases cover every exit edge of the AgentHeadless body. `npx vitest run packages/core/src` — 269 files / 6994 passed.
DragonnZhang
pushed a commit
that referenced
this pull request
May 8, 2026
…d 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.
DragonnZhang
pushed a commit
that referenced
this pull request
May 8, 2026
…#3887) Follow-up to #3873 review: the foreground-fork branch in `agent.ts` fires the fork body via `void runInForkContext(runFramedFork)` and returns the placeholder result synchronously, with no try/finally around the fork body. The other three spawn paths (foreground non-fork, background fork, background non-fork) added in #3873 already stop the per-subagent ToolRegistry in their finally blocks — this one was missed, so any AgentTool / SkillTool the fork's model later instantiated leaked its change-listener on the shared SubagentManager / SkillManager for the rest of the session. Wraps the inner body in `try { await runSubagentWithHooks(...) } finally { void agentConfig.getToolRegistry().stop().catch(() => {}) }` — same shape as the background bgBody finally added in #3873. Adds a regression test in `agent.test.ts` Fork dispatch describe that drains the detached fork body and asserts the stop spy was invoked exactly once.
DragonnZhang
pushed a commit
that referenced
this pull request
May 8, 2026
* fix(core): close bound-tool gap on runForkedAgent's YOLO wrapper Follow-up to #3873 review (#3 of the three flagged adjacent Config-wrapper sites). `runForkedAgent`'s AgentHeadless path used to build its YOLO override via a local `Object.create(parent) + getApprovalMode = YOLO` helper that did NOT rebuild the tool registry, so: 1. The YOLO approval mode was silently ignored on the bound-tool path — parent's already-bound `EditTool` / `WriteFileTool` / `ReadFileTool` resolved `this.config.getApprovalMode()` back to the parent. 2. The fork's reads / mutations went through the parent's `FileReadCache` instead of a per-fork cache. 3. Memory-extraction and dream-agent paths stack the YOLO wrapper over a `getPermissionManager`-overriding scoped wrapper. Since the bound tools resolved to the parent, BOTH overrides — the YOLO approval mode AND the scoped permission manager — were bypassed. The fix routes through the existing `createApprovalModeOverride` helper, which: - rebuilds the tool registry on the wrapper (so bound tools resolve `this.config` to the wrapper), - copies discovered tools from the upstream registry, - sets the `TOOL_REGISTRY_REBUILT` Symbol marker so any further downstream wrapper layer recognises the rebuild and skips redundant work. The memory-extraction / dream-agent composition now resolves correctly via prototype walk — the YOLO wrapper sits above the scoped wrapper, so bound tools observe `getApprovalMode() = YOLO` on the wrapper itself and `getPermissionManager() = scopedPm` one prototype level up. Adds a try/finally around the AgentHeadless run so the per-fork ToolRegistry is stopped after execution — same shape as the spawn finallys in `agent.ts` and `background-agent-resume.ts`. Without this, every AgentTool / SkillTool the fork's model later instantiates leaks its change-listener on shared SubagentManager / SkillManager. Adds `forkedAgent.agent.test.ts` covering: marker + YOLO + distinct registry on the wrapper passed to AgentHeadless.create; bound EditTool resolves to the wrapper; memory-scoped composition preserves both YOLO and scopedPm; `stop()` fires after the AgentHeadless body finishes. Uses `vi.spyOn(AgentHeadless, 'create')` rather than module mocking so the real `ContextState` / `AgentEventEmitter` keep working. `npx vitest run packages/core/src` — 269 files / 6992 passed. * test(core): cover stop() lifecycle on AgentHeadless.create + execute failure paths Self-review feedback on #3892: the stop lifecycle test only covered the success path. A future refactor could move the stop() out of the `finally` block and onto the success branch, reintroducing listener leaks when create or execute rejects, while every existing test still passes. Two new tests pin the cleanup to the `finally`: 1. `stops the per-fork ToolRegistry even when AgentHeadless.create rejects` — make `AgentHeadless.create` return a rejected promise; assert the rejection propagates and the stop spy still fires once. 2. `stops the per-fork ToolRegistry even when headless.execute rejects` — return a headless object whose `execute` rejects; same shape. Together with the success-path test these three cases cover every exit edge of the AgentHeadless body. `npx vitest run packages/core/src` — 269 files / 6994 passed.
TaimoorSiddiquiOfficial
pushed a commit
to TaimoorSiddiquiOfficial/HopCode
that referenced
this pull request
May 8, 2026
…nLM#3892) * fix(core): close bound-tool gap on runForkedAgent's YOLO wrapper Follow-up to QwenLM#3873 review (#3 of the three flagged adjacent Config-wrapper sites). `runForkedAgent`'s AgentHeadless path used to build its YOLO override via a local `Object.create(parent) + getApprovalMode = YOLO` helper that did NOT rebuild the tool registry, so: 1. The YOLO approval mode was silently ignored on the bound-tool path — parent's already-bound `EditTool` / `WriteFileTool` / `ReadFileTool` resolved `this.config.getApprovalMode()` back to the parent. 2. The fork's reads / mutations went through the parent's `FileReadCache` instead of a per-fork cache. 3. Memory-extraction and dream-agent paths stack the YOLO wrapper over a `getPermissionManager`-overriding scoped wrapper. Since the bound tools resolved to the parent, BOTH overrides — the YOLO approval mode AND the scoped permission manager — were bypassed. The fix routes through the existing `createApprovalModeOverride` helper, which: - rebuilds the tool registry on the wrapper (so bound tools resolve `this.config` to the wrapper), - copies discovered tools from the upstream registry, - sets the `TOOL_REGISTRY_REBUILT` Symbol marker so any further downstream wrapper layer recognises the rebuild and skips redundant work. The memory-extraction / dream-agent composition now resolves correctly via prototype walk — the YOLO wrapper sits above the scoped wrapper, so bound tools observe `getApprovalMode() = YOLO` on the wrapper itself and `getPermissionManager() = scopedPm` one prototype level up. Adds a try/finally around the AgentHeadless run so the per-fork ToolRegistry is stopped after execution — same shape as the spawn finallys in `agent.ts` and `background-agent-resume.ts`. Without this, every AgentTool / SkillTool the fork's model later instantiates leaks its change-listener on shared SubagentManager / SkillManager. Adds `forkedAgent.agent.test.ts` covering: marker + YOLO + distinct registry on the wrapper passed to AgentHeadless.create; bound EditTool resolves to the wrapper; memory-scoped composition preserves both YOLO and scopedPm; `stop()` fires after the AgentHeadless body finishes. Uses `vi.spyOn(AgentHeadless, 'create')` rather than module mocking so the real `ContextState` / `AgentEventEmitter` keep working. `npx vitest run packages/core/src` — 269 files / 6992 passed. * test(core): cover stop() lifecycle on AgentHeadless.create + execute failure paths Self-review feedback on QwenLM#3892: the stop lifecycle test only covered the success path. A future refactor could move the stop() out of the `finally` block and onto the success branch, reintroducing listener leaks when create or execute rejects, while every existing test still passes. Two new tests pin the cleanup to the `finally`: 1. `stops the per-fork ToolRegistry even when AgentHeadless.create rejects` — make `AgentHeadless.create` return a rejected promise; assert the rejection propagates and the stop spy still fires once. 2. `stops the per-fork ToolRegistry even when headless.execute rejects` — return a headless object whose `execute` rejects; same shape. Together with the success-path test these three cases cover every exit edge of the AgentHeadless body. `npx vitest run packages/core/src` — 269 files / 6994 passed.
B-A-M-N
pushed a commit
to B-A-M-N/qwen-code
that referenced
this pull request
May 8, 2026
…nLM#3892) * fix(core): close bound-tool gap on runForkedAgent's YOLO wrapper Follow-up to QwenLM#3873 review (#3 of the three flagged adjacent Config-wrapper sites). `runForkedAgent`'s AgentHeadless path used to build its YOLO override via a local `Object.create(parent) + getApprovalMode = YOLO` helper that did NOT rebuild the tool registry, so: 1. The YOLO approval mode was silently ignored on the bound-tool path — parent's already-bound `EditTool` / `WriteFileTool` / `ReadFileTool` resolved `this.config.getApprovalMode()` back to the parent. 2. The fork's reads / mutations went through the parent's `FileReadCache` instead of a per-fork cache. 3. Memory-extraction and dream-agent paths stack the YOLO wrapper over a `getPermissionManager`-overriding scoped wrapper. Since the bound tools resolved to the parent, BOTH overrides — the YOLO approval mode AND the scoped permission manager — were bypassed. The fix routes through the existing `createApprovalModeOverride` helper, which: - rebuilds the tool registry on the wrapper (so bound tools resolve `this.config` to the wrapper), - copies discovered tools from the upstream registry, - sets the `TOOL_REGISTRY_REBUILT` Symbol marker so any further downstream wrapper layer recognises the rebuild and skips redundant work. The memory-extraction / dream-agent composition now resolves correctly via prototype walk — the YOLO wrapper sits above the scoped wrapper, so bound tools observe `getApprovalMode() = YOLO` on the wrapper itself and `getPermissionManager() = scopedPm` one prototype level up. Adds a try/finally around the AgentHeadless run so the per-fork ToolRegistry is stopped after execution — same shape as the spawn finallys in `agent.ts` and `background-agent-resume.ts`. Without this, every AgentTool / SkillTool the fork's model later instantiates leaks its change-listener on shared SubagentManager / SkillManager. Adds `forkedAgent.agent.test.ts` covering: marker + YOLO + distinct registry on the wrapper passed to AgentHeadless.create; bound EditTool resolves to the wrapper; memory-scoped composition preserves both YOLO and scopedPm; `stop()` fires after the AgentHeadless body finishes. Uses `vi.spyOn(AgentHeadless, 'create')` rather than module mocking so the real `ContextState` / `AgentEventEmitter` keep working. `npx vitest run packages/core/src` — 269 files / 6992 passed. * test(core): cover stop() lifecycle on AgentHeadless.create + execute failure paths Self-review feedback on QwenLM#3892: the stop lifecycle test only covered the success path. A future refactor could move the stop() out of the `finally` block and onto the success branch, reintroducing listener leaks when create or execute rejects, while every existing test still passes. Two new tests pin the cleanup to the `finally`: 1. `stops the per-fork ToolRegistry even when AgentHeadless.create rejects` — make `AgentHeadless.create` return a rejected promise; assert the rejection propagates and the stop spy still fires once. 2. `stops the per-fork ToolRegistry even when headless.execute rejects` — return a headless object whose `execute` rejects; same shape. Together with the success-path test these three cases cover every exit edge of the AgentHeadless body. `npx vitest run packages/core/src` — 269 files / 6994 passed.
46 tasks
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
…d tools resolve to the subagent (QwenLM#3873) * fix(core): rebuild tool registry on subagent Config overrides so bound tools resolve to the subagent PR-B (QwenLM#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 QwenLM#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 QwenLM#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.
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
…QwenLM#3887) Follow-up to QwenLM#3873 review: the foreground-fork branch in `agent.ts` fires the fork body via `void runInForkContext(runFramedFork)` and returns the placeholder result synchronously, with no try/finally around the fork body. The other three spawn paths (foreground non-fork, background fork, background non-fork) added in QwenLM#3873 already stop the per-subagent ToolRegistry in their finally blocks — this one was missed, so any AgentTool / SkillTool the fork's model later instantiated leaked its change-listener on the shared SubagentManager / SkillManager for the rest of the session. Wraps the inner body in `try { await runSubagentWithHooks(...) } finally { void agentConfig.getToolRegistry().stop().catch(() => {}) }` — same shape as the background bgBody finally added in QwenLM#3873. Adds a regression test in `agent.test.ts` Fork dispatch describe that drains the detached fork body and asserts the stop spy was invoked exactly once.
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
…nLM#3892) * fix(core): close bound-tool gap on runForkedAgent's YOLO wrapper Follow-up to QwenLM#3873 review (QwenLM#3 of the three flagged adjacent Config-wrapper sites). `runForkedAgent`'s AgentHeadless path used to build its YOLO override via a local `Object.create(parent) + getApprovalMode = YOLO` helper that did NOT rebuild the tool registry, so: 1. The YOLO approval mode was silently ignored on the bound-tool path — parent's already-bound `EditTool` / `WriteFileTool` / `ReadFileTool` resolved `this.config.getApprovalMode()` back to the parent. 2. The fork's reads / mutations went through the parent's `FileReadCache` instead of a per-fork cache. 3. Memory-extraction and dream-agent paths stack the YOLO wrapper over a `getPermissionManager`-overriding scoped wrapper. Since the bound tools resolved to the parent, BOTH overrides — the YOLO approval mode AND the scoped permission manager — were bypassed. The fix routes through the existing `createApprovalModeOverride` helper, which: - rebuilds the tool registry on the wrapper (so bound tools resolve `this.config` to the wrapper), - copies discovered tools from the upstream registry, - sets the `TOOL_REGISTRY_REBUILT` Symbol marker so any further downstream wrapper layer recognises the rebuild and skips redundant work. The memory-extraction / dream-agent composition now resolves correctly via prototype walk — the YOLO wrapper sits above the scoped wrapper, so bound tools observe `getApprovalMode() = YOLO` on the wrapper itself and `getPermissionManager() = scopedPm` one prototype level up. Adds a try/finally around the AgentHeadless run so the per-fork ToolRegistry is stopped after execution — same shape as the spawn finallys in `agent.ts` and `background-agent-resume.ts`. Without this, every AgentTool / SkillTool the fork's model later instantiates leaks its change-listener on shared SubagentManager / SkillManager. Adds `forkedAgent.agent.test.ts` covering: marker + YOLO + distinct registry on the wrapper passed to AgentHeadless.create; bound EditTool resolves to the wrapper; memory-scoped composition preserves both YOLO and scopedPm; `stop()` fires after the AgentHeadless body finishes. Uses `vi.spyOn(AgentHeadless, 'create')` rather than module mocking so the real `ContextState` / `AgentEventEmitter` keep working. `npx vitest run packages/core/src` — 269 files / 6992 passed. * test(core): cover stop() lifecycle on AgentHeadless.create + execute failure paths Self-review feedback on QwenLM#3892: the stop lifecycle test only covered the success path. A future refactor could move the stop() out of the `finally` block and onto the success branch, reintroducing listener leaks when create or execute rejects, while every existing test still passes. Two new tests pin the cleanup to the `finally`: 1. `stops the per-fork ToolRegistry even when AgentHeadless.create rejects` — make `AgentHeadless.create` return a rejected promise; assert the rejection propagates and the stop spy still fires once. 2. `stops the per-fork ToolRegistry even when headless.execute rejects` — return a headless object whose `execute` rejects; same shape. Together with the success-path test these three cases cover every exit edge of the AgentHeadless body. `npx vitest run packages/core/src` — 269 files / 6994 passed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #3774. Closes the bound-tool gap that the prior PR explicitly deferred to a later change: subagent Config overrides built via
Object.create(parent)did NOT relocate the parent's already-bound core tools (EditTool/WriteFileTool/ReadFileTool), so subagent mutations still walked through tool instances whosethis.configwas the parent — reaching the parent'sFileReadCacheand approval mode rather than the subagent's.The bug, in one paragraph
Config.createToolRegistry()runs once at parentinitialize()time. Its lazy factories close overthis, so when they later instantiateEditTooletc. they bindthis.config = parent. PR-B added a per-Configlazy-init forFileReadCacheso anObject.create(parent)wrapper would get its own cache — but only for code that reads through the wrapper Config directly. Bound tools from the parent registry kept reading from the parent. This was called out in PR-B's review #4234090906 as the follow-up to fix here.What this changes
A shared helper
rebuildToolRegistryOnOverride(override, base)inagent.tsrebuilds the tool registry on the override Config — the same patternInProcessBackend.createPerAgentConfigalready uses:Called from three spawn sites:
agent.ts:createApprovalModeOverride(foreground / background / fork from the AgentTool)subagent-manager.ts:maybeOverrideContentGenerator(inherits and non-inherits branches)background-agent-resume.tsresume path (via the now-async exportedcreateApprovalModeOverride; the local pre-rebuild copy was removed)After this, the subagent's bound tools resolve
this.configto the override Config, which in turn resolvesgetFileReadCache()/getApprovalMode()to the per-subagent state via the lazy own-property machinery.Scope note: discovered tools (MCP / command-discovered) are reference-copied from the parent registry rather than re-discovered, so MCP tool instances keep
this.config = parent. This is deliberate to avoid re-running discovery — bound-tool isolation here is specifically for the core file tools that read FileReadCache and approval mode.createApprovalModeOverridebecomesasync; its existing call sites already ran inside async blocks.Avoiding double-rebuild + listener leak
createAgentHeadlessflows are layered:agent.tswraps the parent (child1), then passes that wrapper intosubagent-manager.ts'smaybeOverrideContentGeneratorwhich would otherwise wrap again (child2). Background path adds yet another layer (bgConfig = Object.create(agentConfig)). Doing redundant rebuilds at downstream layers would:AgentTool/SkillToolfactory invocation on the second-layer registry registers a change-listener on sharedSubagentManager/SkillManager, andchild2cache while bound tools (inchild1) keep usingchild1's.maybeOverrideContentGeneratorskips the rebuild when an upstream wrapper has already done it. Detection uses aSymbol.for('qwen-code:tool-registry-rebuilt')marker installed byrebuildToolRegistryOnOverride— Symbol property reads walk the prototype chain via normal lookup, so a marker stored on any ancestor (even one or twoObject.createlayers up, likebgConfigoveragentConfig) is correctly observed. A plainhasOwnProperty(base, 'getToolRegistry')check would have missed those wrapper-on-wrapper layers and triggered redundant rebuilds.Lifecycle
The per-subagent registry is stopped in finally blocks of three spawn paths — foreground non-fork, background non-fork, background fork — so every
AgentTool/SkillToolthe model instantiated disposes its change-listener on shared managers. The foreground-fork path is missed in this PR and gets a small follow-up (#3887).Tests
Two new files exercise the bound-tool path end-to-end (real
Config+ realToolRegistry+ real tool factories — no global mocks oftool-registry/ tools):packages/core/src/tools/agent/agent-override.test.ts— verifiescreateApprovalModeOverridereturns a Config whose registry is a distinct instance, thatEditTool/WriteFileTool/ReadFileToolinstantiated from that registry havetool.config === override, that the bound tool'sgetFileReadCache()resolves to the override's cache (not the parent's), that the override approval mode is preserved through bound tools, and that discovered tools survive the rebuild viacopyDiscoveredToolsFrom. Plus a dedicatedTOOL_REGISTRY_REBUILT marker propagationdescribe that pins the Symbol-marker contract throughObject.createwrappers.packages/core/src/subagents/subagent-manager-override.test.ts— same shape forSubagentManager.maybeOverrideContentGenerator's inherits AND non-inherits branches, plus dedicated tests for the chained-override case (base already has its own registry → no rebuild → child inherits via prototype, bound tools still resolve to the upstream wrapper).Existing test fixtures in
agent.test.ts,subagent-manager.test.ts, andbackground-agent-resume.test.tswere updated to providecreateToolRegistry/tools: new Map()/stopstubs on their mock configs since the override path now exercises those methods.Test plan
npx vitest run packages/core/src— 268 files, 6943 passed, 3 skippednpm run build— cleannpx eslinton changed files — cleanKnown follow-ups
stop()cleanup added here — same shape, separate small PR.forkedAgent.ts:createYoloConfig/extractionAgentPlanner.ts:scopedConfig/dreamAgentPlanner.ts:scopedConfigall have the sameObject.create + method override + hand to AgentHeadlesspattern but bypass the subagent manager and therefore the rebuild — out of scope here, separate follow-up.中文版本 (Chinese version)
概要
#3774 的后续 PR。补完上一个 PR 明确推迟到后续的"bound-tool"漏洞：通过
Object.create(parent)构造的 subagent Config override 没有把父进程已经绑定的核心工具（EditTool/WriteFileTool/ReadFileTool）一起搬过来，所以 subagent 的写操作仍然走那些this.config = parent的工具实例 —— 读到的是父进程的FileReadCache和 approval mode，而不是 subagent 自己的。Bug 一句话总结
Config.createToolRegistry()只在父进程initialize()时跑一次。它的 lazy factory 闭包里捕获了this，所以后续实例化EditTool等工具时绑定的是this.config = parent。PR-B 给FileReadCache加了 per-Config 的 lazy-init —— 让Object.create(parent)wrapper 拿到自己的 cache —— 但这只对直接通过 wrapper Config 读取的代码生效。从父注册表里出来的 bound tool 还是读父进程的 cache。这一点 PR-B 的 review #4234090906 已经标记为 follow-up，本 PR 修复。这次改了什么（Java 类比）
可以把 Config 想成 Spring 的 ApplicationContext，ToolRegistry 想成 BeanFactory，EditTool 等是 prototype-scoped Bean —— 它们在 BeanFactory 创建时就被注入了 ApplicationContext。PR-B 相当于让 child ApplicationContext 持有自己的 cache 字段，但 BeanFactory 还是父的；本 PR 让 child 也建一个自己的 BeanFactory（
createToolRegistry），把 MCP 等"发现型 Bean"从父 BeanFactory 复制过来（copyDiscoveredToolsFrom），从而让所有 Bean 的this.config都指向 child。抽出共享 helper
rebuildToolRegistryOnOverride(override, base)，在三个 spawn 站点调用：agent.ts:createApprovalModeOverridesubagent-manager.ts:maybeOverrideContentGenerator（inherits 和非 inherits 分支）background-agent-resume.tsresume 路径（通过现在异步的导出版createApprovalModeOverride；本地的 pre-rebuild 副本已删）范围说明：MCP / command-discovered 这类 discovered tool 是引用复制过来的，不重新发现 —— 所以 MCP tool 实例的
this.config还是parent。这是有意的（避免再次握手 discovery 开销）；本 PR 的 bound-tool 隔离专门针对读 FileReadCache / approval mode 的核心 file 工具。createApprovalModeOverride变成了async；现有调用点本就在 async 块里。避免双重重建 + listener 泄漏
createAgentHeadless是分层的：agent.ts包一次（child1），再把 wrapper 传给subagent-manager.ts:maybeOverrideContentGenerator—— 如果不加判断，这一层会再包一次（child2）。background 路径还会再叠一层（bgConfig = Object.create(agentConfig)）。下游层重复重建会：AgentTool/SkillTool的 factory 一旦被 model 调用，就会在共享的SubagentManager/SkillManager上挂 change-listener；child2（空 cache），bound tool 还在child1那一层读写。maybeOverrideContentGenerator检测到上游已重建就跳过。检测用Symbol.for('qwen-code:tool-registry-rebuilt')标记，由rebuildToolRegistryOnOverride安装 —— Symbol 属性查找走原型链，所以挂在任何祖先上的标记（哪怕bgConfig在agentConfig之上一两层）也能正确识别。普通hasOwnProperty(base, 'getToolRegistry')检测会漏掉这种 wrapper-on-wrapper 场景，触发冗余重建。Lifecycle
per-subagent registry 在三条 spawn 路径的 finally 里 stop —— 前台非 fork、后台非 fork、后台 fork —— 让 model 实例化的每个
AgentTool/SkillTool在共享 manager 上 dispose 它的 change-listener。前台 fork 路径在本 PR 漏了，单独 follow-up #3887 处理。测试
新增两个测试文件，端到端跑 bound-tool path（真
Config+ 真ToolRegistry+ 真工厂，不全局 mock）：packages/core/src/tools/agent/agent-override.test.tspackages/core/src/subagents/subagent-manager-override.test.ts加了
TOOL_REGISTRY_REBUILT marker propagation子 describe，把 Symbol 标记契约穿过Object.create包装层钉死。agent.test.ts/subagent-manager.test.ts/background-agent-resume.test.ts三处现有 mock config 给 tool-registry mock 加了createToolRegistry/tools: new Map()/stop桩。Test plan
npx vitest run packages/core/src— 268 文件，6943 通过，3 skipnpm run build— 干净npx eslint— 干净已知 follow-up
stop()，单独小 PR 处理。forkedAgent.ts:createYoloConfig/extractionAgentPlanner.ts:scopedConfig/dreamAgentPlanner.ts:scopedConfig都有同形Object.create + method override + 直接给 AgentHeadless的模式但绕过 subagent manager 也就绕过了 rebuild —— 越界，单独 follow-up。