fix(core): close bound-tool gap on runForkedAgent's YOLO wrapper#3892
Merged
Conversation
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.
wenshao
commented
May 7, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
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. |
…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.
tanzhenxin
approved these changes
May 8, 2026
tanzhenxin
left a comment
Collaborator
There was a problem hiding this comment.
Review
Closes the third site flagged by the #3873 review — runForkedAgent's AgentHeadless path was building its YOLO wrapper via a local Object.create + getApprovalMode = YOLO helper that didn't rebuild the tool registry, so the parent's already-bound Edit/WriteFile/ReadFile tool instances resolved this.config to the parent through prototype lookup. Routing through the existing createApprovalModeOverride helper rebuilds the registry on the wrapper, copies discovered tools, and sets the TOOL_REGISTRY_REBUILT marker — the same shape as the rest of the spawn path.
The memory-extraction stacking case is the worst symptom this fix unblocks: layering scopedConfig (own getPermissionManager) under yoloConfig (own getApprovalMode) now resolves correctly via prototype walk, where before both overrides were silently bypassed on the bound-tool path. Wrapping the AgentHeadless run in try/finally so the per-fork ToolRegistry.stop() fires also matches the spawn-path pattern in agent.ts and background-agent-resume.ts and prevents listener leaks. Tests cover the marker, the YOLO + per-fork FileReadCache invariant, the memory-scoped composition end-to-end, and that stop() fires on both happy-path and rejection.
Verdict
APPROVE — Correct, well-tested, consistent with the established pattern.
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
…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 #3873 review — addresses #3 of the three flagged adjacent Config-wrapper sites.
runForkedAgent's AgentHeadless path built its YOLO override via a localObject.create(parent) + getApprovalMode = YOLOhelper that did NOT rebuild the tool registry, so the parent's already-boundEditTool/WriteFileTool/ReadFileToolinstances kept resolvingthis.configto the parent. Three concrete consequences:this.config.getApprovalMode()which walked through the wrapper's prototype to the parent, returning the parent's mode rather than YOLO.FileReadCachebypassed — reads/mutations were tracked in the parent's cache.extractionAgentPlanneranddreamAgentPlannerbuild agetPermissionManager-overridingscopedConfig, then hand it torunForkedAgentwhich used to wrap with the broken YOLO helper. Bound tools resolved to the parent, so both overrides — the YOLO mode AND the scoped permission manager — were bypassed.Fix
Route through the existing
createApprovalModeOverridehelper (#3873):The helper:
Object.create(base),this.configto the wrapper),TOOL_REGISTRY_REBUILTSymbol marker so wrapper-on-wrapper layers downstream skip redundant rebuilds.Memory-extraction / dream-agent composition now resolves correctly via prototype walk:
Tools bound to
yoloConfigseethis.config.getApprovalMode() === YOLO(own),this.config.getPermissionManager() === scopedPm(one proto level up), andthis.config.getFileReadCache()lazy-installs a per-fork cache.Wraps the AgentHeadless run in try/finally so the per-fork
ToolRegistryisstop()'d after execution — same shape as the spawn finallys inagent.tsandbackground-agent-resume.ts. Without this, everyAgentTool/SkillToolthe fork's model later instantiates leaks its change-listener on sharedSubagentManager/SkillManager.Tests
New
forkedAgent.agent.test.tscovering:AgentHeadless.createcarries theTOOL_REGISTRY_REBUILTSymbol marker, has YOLO approval mode, and exposes a registry distinct from the parent's.EditToolfrom the wrapper registry hastool.config === wrapperand reads YOLO + the wrapper's own FileReadCache.getPermissionManager-overridingscopedConfigproduces bound tools that see YOLO from the wrapper ANDscopedPmvia prototype walk.stop()fires once after the AgentHeadless body finishes.Uses
vi.spyOn(AgentHeadless, 'create')rather than module mocking so the realContextState/AgentEventEmitterkeep working — the re-export layer inagents/index.jsdoesn't reliably forwardexport *re-exports through...actual, which made the alternative module-mock approach brittle.Test plan
npx vitest run packages/core/src— 269 files / 6992 passednpx eslinton changed files — cleanCloses the #3873 follow-up triad
extractionAgentPlanner.ts/dreamAgentPlanner.ts) need NO changes: theircreateMemoryScopedAgentConfigonly addsgetPermissionManager, and the downstreamrunForkedAgentrebuild handles the bound-tool isolation. The composition is verified by the memory-scoped test.中文
概要
#3873 review 的 #3 follow-up（reviewer 标的三个相邻 wrapper 站点的最后一个）。
runForkedAgent的 AgentHeadless 路径用本地Object.create(parent) + getApprovalMode = YOLOhelper 建 YOLO override，没重建 tool registry。Parent 早已绑好的EditTool/WriteFileTool/ReadFileTool仍把this.config解析回 parent。三个具体后果：this.config.getApprovalMode()走 wrapper 原型链回到 parent，返回的是 parent 的 mode 而不是 YOLO。FileReadCache被绕过 —— 读写都记录到 parent 的 cache。extractionAgentPlanner/dreamAgentPlanner先建getPermissionManageroverride 的scopedConfig，再交给runForkedAgent，老 YOLO helper 套上去后 bound tool 仍解析回 parent，两个 override（YOLO mode + scoped permission manager）都被绕过。修法
走 #3873 已有的
createApprovalModeOverride：rebuild registry + copy discovered tools + set Symbol marker。memory-extraction / dream-agent 的组合现在通过原型链正确解析：
绑到
yoloConfig的工具看到this.config.getApprovalMode() === YOLO（own）、this.config.getPermissionManager() === scopedPm（上溯一层）、this.config.getFileReadCache()lazy 装一个 per-fork 的 cache。AgentHeadless 跑完后用 try/finally 调
stop()—— 跟agent.ts/background-agent-resume.ts的 spawn finally 同形。否则 fork 的 model 实例化的AgentTool/SkillTool会在SubagentManager/SkillManager上挂 listener 一直泄漏到 session 结束。测试
新增
forkedAgent.agent.test.ts4 个测试：TOOL_REGISTRY_REBUILTsymbol 标记 + YOLO approval mode + 独立 registryEditTooltool.config === wrapper，读 YOLO + wrapper 自己的 FileReadCachegetPermissionManageroverride 的scopedConfig之上，bound tool 同时看到 YOLO +scopedPmstop()调一次用
vi.spyOn(AgentHeadless, 'create')而不是 module mock ——agents/index.js的export *在 vitest mock 下...actual不能可靠转发，module mock 太脆。已完整覆盖 #3873 三连 follow-up
extractionAgentPlanner.ts/dreamAgentPlanner.ts）无需改动：createMemoryScopedAgentConfig只加getPermissionManager，下游runForkedAgentrebuild 会处理 bound-tool 隔离。组合正确性由 memory-scoped 测试验证。