feat(core): port declarative-agent mcpServers + hooks (CC 2.1.168 parity follow-up)#4996
Merged
Conversation
…sted round-trip PR #4870 swapped `parse` over to the `yaml` library so block scalars and nested structures load correctly, but left the hand-rolled `stringify` in place. The hand-rolled serializer only walks one level of nesting and emits `[object Object]` for any value below — so any caller that does `parse → modify → stringify` (e.g. SubagentManager saving a frontmatter file, or a Claude-Code-format converter writing back a `.qwen/agents/*.md`) silently corrupts nested fields like `mcpServers` or `hooks` on disk. This commit: - Delegates `stringify` to `yaml.stringify` with `lineWidth: 0`, matching the parse side's library choice and unlocking arbitrary-depth round-trip. - Drops the now-unused `formatValue` helper. - Replaces the byte-exact escape-sequence assertions with property-based `parse(stringify(x)) === x` round-trip checks — the previous tests pinned hand-rolled quote output that eemeli/yaml legitimately chooses differently from. The contract that matters at the public API boundary is round-trip, not stable bytes. - Adds two CC-shape nested round-trip tests (mcpServers + hooks) that would have been impossible to express under the old serializer. The parse-side safety guards added in PR #4870 (schema 'core', timestamp / binary tag filtering, `Object.create(null)`, Date/Uint8Array sanitize, parseSimple fallback) are preserved untouched.
Builds on PR #4842 (which deferred these two fields) and PR #4870 (which made `parse` nested-safe) by adding the remaining surface + runtime wiring so a `.qwen/agents/*.md` with per-agent MCP servers and hooks works the same way as the equivalent `.claude/agents/*.md`. ## Schema layer `agent-frontmatter-schema.ts` gains two lenient DL7-parity parsers: - `parseAgentMcpServers` — keeps a record-of-records shape and drops per-key entries whose value is a scalar / array / null (mirrors CC's `gS8` shallow validation; per-spec union is enforced later by the MCP loader). - `parseAgentHooks` — keeps a record-of-arrays shape and drops events whose value isn't an array (mirrors CC's `TKO` / `_u`). Both return `undefined` when no entries survive shape filtering, so the caller can omit the field entirely rather than emit an empty object. ## SubagentConfig surface - `types.ts`: adds optional `mcpServers?: Record<string, unknown>` and `hooks?: Record<string, unknown>`. - `subagent-manager.ts` `parseSubagentContent`: extracts both fields with warn-and-drop on top-level shape failure, matching the existing posture for `permissionMode` / `maxTurns` / `color`. - `subagent-manager.ts` `serializeSubagent`: emits both back to YAML. The previous skip-list carve-out in `claude-converter.ts` (`NESTED_FIELDS_NOT_ROUND_TRIPPABLE`) is gone — round-trip is safe now that the YAML stringifier is eemeli/yaml. - `claude-converter.ts`: emits `mcpServers` (was missing) when converting from a CC plugin agent file. ## Runtime wiring - `hookRegistry.ts`: new public `addAgentHooks(hooks, scopeId): () => void` appends ephemeral entries tagged with `agentScope`, runs them through the same per-definition validation pipeline as session/user/project hooks (so a malformed entry is logged + dropped instead of breaking the spawn), and returns an unregister callback. The duplicate-detection key includes `agentScope` so identical hooks from different subagents — or from a subagent and the session — coexist instead of swallowing one another. v1 scope limitation: while a subagent's entries live in the registry they fire for every event of their declared type regardless of which agent is currently active. Per-agent scope filtering at firing time is a follow-up; the limitation is documented in the user-facing doc. - `subagent-manager.ts` `buildSubagentContextOverride`: now takes the `SubagentConfig` and, when per-agent `mcpServers` are present, overrides `getMcpServers()` on the subagent's Config wrapper to return the union of session + agent servers (agent wins on key collision, matching CC's `scope: 'agent'` semantics). The skip-rebuild optimization is bypassed in this case — without a fresh `rebuildToolRegistryOnOverride` anchored on the override Config, the pre-existing wrapper-owned `McpClientManager` would still see only the session set and the discovery loop below would silently no-op. After rebuild, the loop explicitly discovers each per-agent server so its tools land in the subagent's registry before `AgentHeadless.run`. - `subagent-manager.ts` `createAgentHeadless`: when `config.hooks` is set, registers via `HookRegistry.addAgentHooks` with a per-spawn scope ID (`agent:<name>:<uuid>`) and wraps the caller-provided `AgentHooks.onStop` so the unregister callback fires after the caller's handler, in a `finally` block to survive a throwing user handler. Errors from `AgentHeadless.create` itself unregister the hooks before re-throwing. ## Tests - `agent-frontmatter-schema.test.ts`: +8 tests covering shape filtering, drop-on-bad-top-level, drop-when-empty for both new parsers. - `subagent-manager.test.ts`: +7 tests covering parse, serialize, and drop-on-malformed for both fields. - `subagent-manager-override.test.ts`: +2 tests for the session+agent MCP merge and the no-override pass-through case. - `hookRegistry.test.ts`: +4 tests for `addAgentHooks` — scope tag, no-collision with existing same-identity entries, two concurrent agents keep their own copies, empty payload no-op.
- `docs/yaml-parser-replacement.md` is the new audit doc covering the PR #4870 → eemeli/yaml decision (parse-side), the security probe results (`maxAliasCount` default, `!!js/function` becomes literal string + warning, merge keys disabled by default, custom-tag filter for timestamp/binary), and the stringify-side gap this follow-up closes. - `docs/declarative-agents-port.md` status table now marks `mcpServers` and `hooks` as **shipped (follow-up)** with a one-line note on the runtime wiring strategy + v1 scope limitation for hooks. The reverse-engineering record below the table is unchanged and remains the reference for the still-deferred fields (`effort`, `memory`, `isolation`, `initialPrompt`, `skills`). - `docs/users/features/sub-agents.md` adds `mcpServers` + `hooks` rows to the CC-compatibility table and a full example frontmatter showing all four shipped fields composed together. The v1 hooks scope limitation is called out as a blockquote so users picking up per-agent hooks know to prefer fire-globally-safe handlers (logging) over behavior-mutating ones until the firing-time scope filter lands.
…CP discovery
Audited the three commits adversarially across four lenses (correctness,
security, reuse, test quality). Two real findings, two test gaps.
## Real bugs fixed
### `parseAgentMcpServers` / `parseAgentHooks` could pollute the result's prototype
Both helpers wrote into a plain `{}` while iterating an input object that
yaml-parser hands back as null-prototype. A YAML key of literal
`__proto__` survives that null-prototype guarantee as an own property,
so `Object.entries(record)` walks it, and the assignment
`result['__proto__'] = spec` triggers the inherited setter — silently
re-wiring `result`'s prototype chain to point at the attacker's spec.
Object.prototype itself stays untouched (the setter is per-instance) and
the downstream spread `{ ...result }` only copies own enumerables, so the
pollution doesn't directly leak through current callers. But returning
an object with a hijacked prototype is a latent footgun: a future caller
that uses `for…in`, `result.someKey`, or any property access that misses
the own table would pick up values from the polluted chain.
Switching to `Object.create(null)` makes the assignment a plain own
property — matching the null-prototype invariant the rest of
`yaml-parser.ts` already maintains. Two regression tests pin the
defense (one per parser), each constructing a null-prototype input the
way `yaml.parse` actually produces it (`{ __proto__: … }` in an
object literal triggers the setter at construction and so does NOT
reproduce the attack input shape).
### Per-agent MCP discovery serialised through every server
`buildSubagentContextOverride` called `discoverToolsForServer` in a
sequential `for…of` loop with `await`. A misbehaving server (stdio
command that hangs at startup, remote endpoint that times out at the
MCP layer's default 30s) blocks every following server, so the subagent
spawn paid the sum of every per-server timeout instead of the max.
Switched to `Promise.allSettled` over the server list. Each call still
carries the MCP layer's own per-server timeout (`stdio` default 30s,
remote default 5s, per-spec `discoveryTimeoutMs` override); `allSettled`
only removes the serialisation between siblings. Rejections still
log-and-drop so a single bad server doesn't block its siblings'
tools from landing in the subagent's registry.
## Test gaps closed
### `addAgentHooks` coexistence test only asserted count
The "coexists with session/user hooks of the same identity" test
asserted `getAllHooks()` had length 2 after the add but did NOT verify
which entries were present. A regression that dropped `agentScope`
from the dedup key would still leave two entries by ordering luck and
silently break concurrent-agent isolation. Replaced the bare count
check with explicit assertions for `(source: User, agentScope: undefined)`
+ `(source: Session, agentScope: 'agent:test:def')`.
### Empty-record edge cases for `parseAgentMcpServers` / `parseAgentHooks`
The existing "returns undefined when nothing survives shape filtering"
tests covered the case where every key had a bad shape. The empty
input case `parseAgent…({})` was on the same code path but never
exercised — callers rely on the `undefined → omit field` behaviour for
both. Added one test per parser.
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. |
Collaborator
|
Thanks for the PR! Template looks good ✓ — all required sections present with bilingual coverage. On direction: this is a natural, well-documented follow-up to PRs #4842 and #4870. The CC CHANGELOG confirms per-agent On approach: the scope feels right. Three logical pieces — (1) YAML stringify replacement fixing the One thing worth flagging: the 488-line Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — 所有必需章节齐全，双语覆盖完整。 方向：这是 PR #4842 和 #4870 的自然、有据可查的后续工作。CC CHANGELOG 确认 per-agent 方案：范围合理。三个逻辑部分——(1) YAML stringify 替换修复 值得提的一点：488 行的 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
Code ReviewIndependent proposal before reading the diff: replace the hand-rolled The PR matches this proposal and goes further in good ways:
No critical blockers found. Code follows project conventions — the prototype-chain override pattern for One observation: the forced tool-registry rebuild when Test ResultsUnit tests (PR-specified scope): 47 files, 1272 tests — all pass ✓ Build: clean, 0 errors ✓ TypeScript typecheck: clean across all packages ✓ Before/After: YAML Stringify Round-tripBefore (main branch — hand-rolled stringifier)The hand-rolled stringifier emits After (this PR — eemeli/yaml stringify)All three CC-shape structures (record-of-records for 中文说明代码审查独立方案（读 diff 之前）：用 PR 与此方案一致，并在几处做得更好：
无关键阻塞问题。代码遵循项目惯例——原型链覆盖模式与现有 一个观察：当存在 测试结果单元测试（PR 指定范围）： 47 文件，1272 测试 — 全部通过 ✓ 构建： 通过，0 错误 ✓ TypeScript 类型检查： 全部包通过 ✓ Before/After: YAML Stringify 往返Before（main 分支——手写 stringifier）： 嵌套值输出为 After（本 PR——eemeli/yaml stringify）： 三种 CC 结构（mcpServers 的 record-of-records、hooks 的 record-of-arrays、混合浅层+深层 frontmatter）全部干净地往返。 — Qwen Code · qwen3.7-max |
Collaborator
|
Stepping back to look at the whole picture. This PR started as "port two deferred frontmatter fields" but the real value is the YAML stringify replacement — the hand-rolled stringifier was silently corrupting any nested value on save, which is a bug independent of The implementation is solid. The runtime wiring follows the established prototype-chain override pattern, the warn-and-drop validation matches existing precedent, and the The v1 hooks limitation (fires globally during agent lifetime) is the one honest tradeoff — it's well-documented in both JSDoc and user-facing docs with a clear recommendation to use fire-globally-safe handlers. This is a reasonable scope decision for v1. All stages clean: template ✓, direction ✓, solution ✓, code review ✓, 1272 tests passing ✓, build/typecheck ✓, before/after verified ✓. Ship it. 中文说明退一步看全局。 这个 PR 名义上是"补齐两个 deferred 的 frontmatter 字段"，但实际价值在于 YAML stringify 替换——手写 stringifier 在保存时会悄悄损坏任何嵌套值，这是一个独立于 实现质量扎实。运行时接线遵循既有的原型链覆盖模式，warn-and-drop 校验与已有先例一致， hooks v1 限制（agent 运行期间全局触发）是唯一诚实的权衡——JSDoc 和用户文档都有清晰说明和建议（用全局触发安全的 handler）。作为 v1 的范围决策是合理的。 各阶段全部通过：模板 ✓、方向 ✓、方案 ✓、代码审查 ✓、1272 测试通过 ✓、构建/类型检查 ✓、before/after 已验证 ✓。可以合入。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jun 11, 2026
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
previously requested changes
Jun 11, 2026
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
[Nice to have] packages/core/src/extension/claude-converter.ts:290 — stringifyYaml(newFrontmatter) is called without .trim(). serializeSubagent in subagent-manager.ts:650 already trims the same call. Without .trim(), the converter produces an extra blank line before the closing --- delimiter in converted agent files. Fix: const newYaml = stringifyYaml(newFrontmatter).trim();
— qwen3.7-max via Qwen Code /review
Collaborator
Local runtime verification report (maintainer)Independently verified this PR on a local checkout ( Verdict: behavior matches every claim in the PR description. ✅ Recommend merge. One pre-existing nit from the bot review confirmed as cosmetic-only (details below). 1. Build + declared test suites
2. E2E per the PR's test plan (tmux, real model, real MCP child processes)Setup as described: Run: All four assertions from the test plan hold:
The timeline doubles as a hook-cleanup proof: agent-mcp spawned at 3. Adversarial checks beyond the test planStringifier before/after (the PR's core claim). Extracted the merge-base The "corrupts on save" claim is real and the fix is verified at the function level (and indirectly E2E, since the researcher agent's nested frontmatter parsed and ran). Bot's CHANGES_REQUESTED ( Malformed-input posture (warn-and-drop). Broken-server resilience (the PR's self-declared main risk area). A second agent declaring two Same-name override (more-specific-wins). A third agent declaring Perf claim ("one extra 4. Notes / non-blockers
Verification environment: macOS (Darwin 25.5.0), Node v22, |
wenshao
previously approved these changes
Jun 12, 2026
…contract
Reviewer flagged two Criticals + one Suggestion + one Nice-to-have. The two
Criticals share a root cause (subagent execute()'s inner try/finally
doesn't fire on every exit path), so they fold into a single API change.
## [Critical] Hook cleanup leak on AgentHeadless.execute() early exits
`wrapAgentHooksForCleanup` relied on `onStop` firing inside execute()'s
inner try/finally. Two early-exit paths bypass that finally:
1. `createChat()` returning null at agent-headless.ts:224-226 — returns
before the outer `try` at 233 is even entered.
2. `prepareTools()` throwing at 234 — propagates through the outer
`finally` at 335, which only calls `abortController.abort()` and
never reaches the inner finally that fires `onStop`.
The pre-fix `catch` block only guarded `AgentHeadless.create()`, not
`execute()`. Leaked HookRegistry entries fire globally for every matching
event in the session, polluting unrelated tool calls.
## [Critical] Per-agent MCP server processes leak after every spawn
`discoverToolsForServer` connects real MCP clients (stdio child
processes, HTTP/SSE sockets) in the force-rebuilt subagent ToolRegistry.
Nothing stopped that registry: `Config.shutdown` only reaches the root's
`this.toolRegistry`, and AgentTool's existing `agentConfig.getToolRegistry()
.stop()` (fg + bg + resume finally blocks) only stops the parent's
registry, not the override's distinct fresh one. Every subagent
invocation that declared `mcpServers` orphaned a child process for the
rest of the host process's lifetime.
## Shared fix — caller-driven `dispose` contract
`SubagentManager.createAgentHeadless` now returns
`{ subagent, dispose }`. Callers MUST invoke `dispose()` in the same
`finally` block that wraps `subagent.execute()`. That `finally` lives
in the caller's scope (AgentTool fg/bg, BackgroundAgentResumeService),
which is reachable on every execute() exit — including the two early-
exit paths the previous `onStop` hook never reached.
`dispose` is a single closure that calls the previously-separate
cleanup callbacks in order:
1. `unregisterAgentHooks` returned from `HookRegistry.addAgentHooks`
(when per-agent hooks were registered).
2. `disposeRegistry` returned alongside the new `buildSubagentContextOverride`
return shape `{ context, disposeRegistry }` (set only when this call
force-rebuilt the registry for `mcpServers`).
Both cleanups are wrapped in `try/finally` that logs and re-arms so an
exception in one path doesn't block the other and doesn't double-fire.
The pre-existing constructor-failure catch in `createAgentHeadless` now
runs the same closure directly — the caller never received the return
value, so it cannot fire `dispose` itself.
The three callers gain one variable + one `void dispose?.().catch()`
inside their existing finally:
- `agent.ts:2039` (foreground) — finally at `agent.ts:2904`
- `agent.ts:2131` (background) — finally at `agent.ts:2502`
- `background-agent-resume.ts:630` (resume) — finally at `:852`
Fork subagents share the parent's lifecycle; their `dispose` stays
undefined and the `?.()` is a no-op.
`wrapAgentHooksForCleanup` is removed — it was load-bearing only for
the happy + inner-reasoning-loop-failure paths and is now obsolete.
## [Suggestion] Repeated guard condition
`config.hooks && Object.keys(config.hooks).length > 0` no longer appears
in both `if` / `else if` arms. Single outer guard + nested branch on
`hookRegistry`.
## [Nice-to-have] claude-converter.ts:290 missing `.trim()`
`stringifyYaml(newFrontmatter).trim()` brings the converter into line
with `subagent-manager.ts:651`. Without trim, eemeli/yaml's trailing
newline produced an extra blank line before the closing `---`
delimiter — cosmetic (both readers tolerate it) but the asymmetry
between the two writers was a real consistency bug.
## Tests
3 new RED-first tests in `subagent-manager.test.ts` pin the dispose
contract:
1. `returns { subagent, dispose }; dispose unregisters per-agent hooks`
2. `dispose unregisters even when execute() never runs (early-exit leak fix)`
— the case where `createChat()` → null or `prepareTools()` throws
3. `dispose is a safe no-op when neither hooks nor mcpServers are declared`
Override test helper updated to destructure the new
`buildSubagentContextOverride` return shape. AgentTool + BackgroundAgent
test mocks updated to return `{ subagent, dispose }`. All 2087 in-scope
tests pass.
Collaborator
Author
|
Re: review at #4996 (review) — |
Collaborator
Author
Review round 1 — all findings addressed in 720f0e4
The two Criticals share a root cause: the inner |
DragonnZhang
previously approved these changes
Jun 12, 2026
DragonnZhang
left a comment
Collaborator
There was a problem hiding this comment.
Review: APPROVE
Commit reviewed: 720f0e4a1f2833b539fb1046211edc94775c0c84
Thorough review of all 18 changed files (1663 additions, 230 deletions). No high-confidence bugs, security issues, or logic errors found.
What was analyzed
- yaml-parser.ts:
stringifyreplacement withyaml.stringify-- correct, round-trip safe, test assertions properly moved to property-based checks. - agent-frontmatter-schema.ts:
parseAgentMcpServersandparseAgentHooks-- shallow validation is correct, null-prototype defense against__proto__pollution is properly implemented and tested. - hookRegistry.ts:
addAgentHookswithagentScope-- dedup logic correctly includesagentScopein the identity check, unregister callback filters by scope tag, tested for concurrent-agent isolation. - subagent-manager.ts:
createAgentHeadlessreturn-shape change to{ subagent, dispose }-- dispose contract is sound, inner try/catch correctly runs cleanup on constructor failure,runCleanupis idempotent (sets closures toundefinedafter firing). - subagent-manager.ts:
buildSubagentContextOverrideforce-rebuild formcpServers-- merge semantics (agent wins on collision) match CC'sscope: 'agent',Promise.allSettledfor parallel discovery is correct,disposeRegistrycallback correctly targets the subagent's own registry. - agent.ts + background-agent-resume.ts: All call sites updated to destructure
{ subagent, dispose }and invokedisposeinfinallyblocks. Fork paths correctly leavesubagentDisposeundefined. - ToolRegistry.stop() double-call safety: verified the implementation is documented idempotent and wraps all disposal in try/catch.
- McpClientManager.stop(): verified idempotent (clears client map after disconnect, second call iterates empty map).
Design notes (not blocking)
The v1 hooks scope limitation (fires globally, not agent-scoped at firing time) is well-documented in JSDoc, user docs, and the PR description. This is an acceptable trade-off for v1.
wenshao
previously approved these changes
Jun 12, 2026
Three cleanups from a multi-angle quality review (reuse / simplification / altitude lenses, all on commit 720f0e4): 1. Drop the null-out guard inside `runCleanup`. Both inner callbacks are already idempotent at the source (`HookRegistry.addAgentHooks` filters removal by `agentScope`; `ToolRegistry.stop` documents itself idempotent), so the outer `unregisterAgentHooks = undefined` / `disposeSubagentRegistry = undefined` finally blocks were buying nothing beyond a marginal short-circuit on duplicate `dispose()` calls — at the cost of 8 LOC and a "is this load-bearing?" question for readers. Comment now states the idempotency guarantee explicitly. 2. Rename the `buildSubagentContextOverride` return field `disposeRegistry` → `cleanup`. Pairs the sibling override builder `createApprovalModeOverride` whose return shape is `ApprovalModeOverrideHandle = { config, cleanup }`. The `context` field stays as-is because `config` would shadow the same-named parameter inside this method's scope (the parent helper doesn't take a `config` parameter, which is why it can use that name). 3. Add a 4th test pinning the constructor-failure cleanup path. The `try { ... } catch { await runCleanup(); throw }` block at the end of `createAgentHeadless` runs when `AgentHeadless.create` rejects — at which point the caller has not received `{ subagent, dispose }` and cannot run cleanup itself. The three existing tests covered the happy-path and execute()-never-runs scenarios; this one closes the "constructor blows up after hooks were registered" gap. No behavior change to callers — same `{ subagent, dispose }` return shape, same dispose semantics. Findings skipped: - Parallelizing the two cleanups inside `runCleanup`: synchronous unregister + async registry stop, the `await` only blocks the registry stop; the order has no measurable cost. - Parallelizing the parent/agent registry stops at the 3 call sites: they already run concurrently because the call sites use `void X.stop().catch(...)` (fire-and-forget), not `await`. - Extracting a `executeHeadlessSubagent` helper that owns the dispose lifecycle: real win against future call-site drift, but reaches well outside the round-1 review diff into AgentTool's three execution shapes (fg sync / bg fire-and-forget / resume embedded). - Fixing `AgentHeadless.execute()`'s early-exit paths upstream: the round-1 commit's explicit altitude choice; revisiting it would re-open a settled design call.
Collaborator
Local runtime re-verification at
|
| # | Checkpoint | Headless | Interactive (tmux) |
|---|---|---|---|
| 1 | Parent scope excludes per-agent server | ✅ init event mcp_servers = session-mcp only |
✅ /mcp panel before task: 1 server — session-mcp · ✓ connected |
| 2 | Subagent calls per-agent tool directly | ✅ single tool_use: mcp__agent-mcp__agent_quote → agent-quote-secret-marker-9999, no shell fallback |
✅ researcher 1 tool · 5.5s, same marker |
| 3 | One spawn, full handshake | ✅ one SPAWN; initialize → initialized → tools/list ×2 → tools/call, timestamped mid-prompt |
✅ identical log pattern |
| 4 | Hook fires once, only in the subagent's register→unregister window | ✅ hook-fired.log = exactly 1 line |
✅ exactly 1 line; parent's two session_motto calls fired nothing |
3. Direct validation of 720f0e4a1's dispose contract
The new commit's whole point is that per-spawn resources (agent-scope hook entries, the force-rebuilt ToolRegistry owning MCP child processes) are released via an explicit dispose in the caller's finally. Observed:
- After the headless run exits:
pgrep -fl mcp-server.cjs→ nothing. - In the interactive session, while the parent kept running after the subagent finished: agent-mcp's child process was already gone (only session-mcp alive, as it should be), and the spawn log shows no respawn/retry churn.
- After
/quit: all MCP child processes reaped. - Hook scoping doubles as cleanup proof: the parent's second
session_mottoran after the subagent terminated and did not fire the per-agent hook — the unregister path executes.
Also confirmed the round-1 bot nit is fixed: claude-converter.ts now .trim()s the stringified frontmatter (with an explanatory comment), so the converter output matches serializeSubagent's layout.
4. New finding (minor, non-blocking): stale 1 MCP offline footer pill
Only visible in the interactive round — headless mode (both my round 1 and the author's E2E) cannot see it, which is why it went unnoticed.
After the subagent finishes, the footer permanently shows 1 MCP offline, while /mcp still lists only session-mcp · ✓ connected. The user can't tell which server is "offline" and can't clear the pill short of restarting.
Root cause: per-agent servers report into the module-global serverStatuses map (packages/core/src/tools/mcp-client.ts L421). The dispose path (ToolRegistry.stop() → McpClientManager.stop()) flips agent-mcp to DISCONNECTED but leaves the map entry, and the footer's MCPHealthPill counts DISCONNECTED entries. removeMCPServerStatus — added precisely so intentional removals stop showing as offline (#3895) — is only wired to the disable/remove paths (disableMcpServer, manager removeServer), not to per-agent dispose.
Suggested follow-up (fine outside this PR): call removeMCPServerStatus on dispose for per-agent-only keys. One edge: when a per-agent key overrides a same-named session server, deleting the entry would clobber the session server's own pill state — that case should restore/re-resolve the session-side status instead.
5. Consistency notes
- The
notifications/cancelled~14 ms aftertools/callI noted in round 1 reproduces identically at the new head (result already delivered; protocol noise, no functional impact). tools/listarriving twice matches the back-fill-copy + explicitdiscoverToolsForServerdual path; idempotent.- Still not covered: Windows/Linux E2E (unit layer is CI's job), concurrent subagents with different per-agent hook sets (documented v1 limitation), non-stdio per-agent transports (SSE/HTTP).
Environment: macOS (Darwin 25.5.0), Node v22.22.2, branch @ 720f0e4a1, merge-base ac040d0a6, npm run bundle + node dist/cli.js, tmux-driven, --approval-mode yolo, no sandbox. Fresh scratch tree at /tmp/pr4996-e2e/.
中文版（Chinese version）
本地运行时复验 @ 720f0e4a1（维护者，第 2 轮）
接续我针对 head 791b38bf5 的首轮报告。作者随后推送了 720f0e4a1（review round 1 — 显式 dispose 契约修复泄漏）。我针对新 head 从零重跑了全部验证——构建、声明的测试套件、真实模型（qwen3.7-max）+ 真实 stdio MCP 子进程的 E2E——并补了首轮没做的一轮：用 tmux 驱动真实交互式 TUI（而不只是 headless JSON 模式）。交互轮暴露了一个新的轻微问题（见下），其余全绿。
结论：维持 ✅ 建议合并。 新的 dispose 契约可验证地生效，首轮的 cosmetic nit 已修复，新发现属外观问题，follow-up 处理即可。
1. 新 head 的构建 + 测试
npm install && npm run bundle→ 干净，dist/cli.jsv0.17.1- 声明套件：47 文件 / 1275 测试全过，0 跳过（首轮 47/1272,+3 来自新增的 dispose 契约测试；描述里的 48/1280 仍是过时数字，无实质影响）
- 截稿时 CI：Lint ✅、CodeQL ✅、三平台 Test matrix 在新 push 上运行中（首轮 head 三平台全绿）
2. E2E 复跑 — 4 个检查点在新 head 全部成立
fixture 设计同首轮（session-mcp → session_motto；researcher.md 声明 per-agent mcpServers: agent-mcp → agent_quote + PreToolUse 日志 hook；两个 server 带时间戳记录每次 spawn 和 JSON-RPC 方法）。两轮：headless --output-format json + tmux 驱动的真实交互式会话。
- Parent 作用域不含 per-agent server ✅：headless
init事件mcp_servers仅 session-mcp；交互式任务前/mcp面板1 server — session-mcp · ✓ connected - Subagent 直接调用 per-agent 工具 ✅：单次
tool_use: mcp__agent-mcp__agent_quote→agent-quote-secret-marker-9999，无 shell fallback；交互式 researcher1 tool · 5.5s同 marker - 单次 spawn + 完整握手 ✅：一次
SPAWN，initialize → initialized → tools/list ×2 → tools/call，时间戳在 prompt 中段；两轮日志模式一致 - Hook 仅在 subagent 的注册→注销窗口内 fire 一次 ✅：
hook-fired.log两轮各恰好 1 行；parent 的两次session_motto均未触发
3. 对 720f0e4a1 dispose 契约的直接验证
该 commit 的核心是 per-spawn 资源（agent 作用域 hook 条目、强制重建的 ToolRegistry 及其 MCP 子进程）经调用方 finally 中的显式 dispose 释放。实测：
- headless 运行退出后：
pgrep -fl mcp-server.cjs→ 无残留 - 交互式会话中，subagent 结束、parent 仍在运行时：agent-mcp 子进程已回收（仅 session-mcp 存活，符合预期），spawn 日志无重启/重试
/quit后：全部 MCP 子进程回收- hook 作用域兼作清理证明：parent 的第二次
session_motto发生在 subagent 终止之后，未触发 per-agent hook——注销路径确实执行
同时确认首轮 bot nit 已修：claude-converter.ts 对 stringify 结果加了 .trim()（带解释注释），converter 输出与 serializeSubagent 布局一致。
4. 新发现（轻微、非阻塞）：footer 残留 1 MCP offline
只有交互轮可见——headless 模式（我的首轮和作者的 E2E）看不到 footer，所以此前未被发现。
subagent 结束后 footer 常驻显示 1 MCP offline，而 /mcp 面板仍只有 session-mcp · ✓ connected。用户既看不出哪个 server offline，也无法清除（重启才消失）。
根因：per-agent server 的状态写入模块级全局 serverStatuses map（packages/core/src/tools/mcp-client.ts L421）。dispose 路径（ToolRegistry.stop() → McpClientManager.stop()）把 agent-mcp 置为 DISCONNECTED 但不删条目，footer 的 MCPHealthPill 按 DISCONNECTED 条目计数。专为"主动移除不再显示 offline"加的 removeMCPServerStatus（#3895）只接在 disable/remove 路径（disableMcpServer、manager removeServer）上，per-agent dispose 未调用。
建议 follow-up（无需阻塞本 PR）：dispose 时对 per-agent 独有 key 调 removeMCPServerStatus。注意边界：当 per-agent key override 同名 session server 时，直接删条目会清掉 session server 自身的状态——该情形应恢复/重新解析 session 侧状态。
5. 一致性备注
- 首轮记录的
tools/call后 ~14msnotifications/cancelled在新 head 完全复现（结果已送达，协议噪音，无功能影响） tools/list出现两次与 back-fill 复制 + 显式discoverToolsForServer双路径一致，幂等- 仍未覆盖：Windows/Linux E2E（单测层由 CI 覆盖）、并发多 subagent 不同 hook 集（已文档化的 v1 限制）、per-agent 非 stdio 传输（SSE/HTTP）
环境：macOS (Darwin 25.5.0)、Node v22.22.2、分支 @ 720f0e4a1、merge-base ac040d0a6、npm run bundle + node dist/cli.js、tmux 驱动、--approval-mode yolo、无沙箱。全新 scratch 目录 /tmp/pr4996-e2e/。
wenshao
approved these changes
Jun 12, 2026
This was referenced Jun 12, 2026
DragonnZhang
approved these changes
Jun 12, 2026
This was referenced Jun 12, 2026
qwen-code-ci-bot
requested changes
Jun 12, 2026
| const before = this.entries.length; | ||
| this.processHooksConfiguration( | ||
| hooks, | ||
| HooksConfigSource.Session, |
Collaborator
There was a problem hiding this comment.
[Critical] Per-agent hooks bypass the trusted-hooks consent mechanism.
addAgentHooks registers ephemeral entries with HooksConfigSource.Session, which bypasses the trust gate that processHooksFromConfig applies to project-level hooks (isTrustedFolder() + TrustedHooksManager.getUntrustedHooks()). Project-level agent files (.qwen/agents/*.md) are loaded by SubagentManager.listSubagentsAtLevel('project') without any isTrustedFolder() check, so a malicious repo can ship .qwen/agents/evil.md with command hooks that execute arbitrary shell commands without a trust prompt.
This is compounded by the v1 global-firing scope: while registered, per-agent hooks fire for every event of their declared type regardless of which agent is active. A PermissionRequest hook with decision: 'allow' output auto-approves tool calls for the parent agent and any concurrent agents for the subagent's lifetime — a privilege-escalation vector from "agent the user chose to invoke" to "global session permission bypass."
Suggested fix: Either (a) gate project-level agent file loading on isTrustedFolder() in listSubagentsAtLevel, or (b) register per-agent hooks with HooksConfigSource.Project when the agent file lives at project level so the existing trust check applies. Additionally, refuse to register per-agent hooks for PermissionRequest and PermissionDenied events, since those are the only events whose hook output directly overrides user decisions.
— qwen3.7-max via Qwen Code /review
| // MCPServerConfig but the type assertion at this boundary is intentional | ||
| // — the discovery layer downstream will refuse malformed specs at | ||
| // connect time, surfacing a precise error instead of a typecheck noise. | ||
| const merged: Record<string, MCPServerConfig> = { |
Collaborator
There was a problem hiding this comment.
[Critical] Spread merge defeats the __proto__ null-prototype defense from parseAgentMcpServers.
parseAgentMcpServers (agent-frontmatter-schema.ts:148) deliberately constructs its return value using Object.create(null) so that a YAML key of literal __proto__ becomes a plain own property rather than triggering the __proto__ setter on Object.prototype. There is a dedicated test asserting Object.getPrototypeOf(result).toBeNull().
However, the downstream merge here uses a plain object-literal spread:
const merged: Record<string, MCPServerConfig> = {
...sessionServers,
...(config.mcpServers as Record<string, MCPServerConfig>),
};Spreading a null-prototype object whose own keys include __proto__ into a plain {} invokes Object.setPrototypeOf on the target (the spread operator uses [[Set]] on the target, and [[Set]] of __proto__ on a plain object triggers the prototype setter). After the merge, Object.getPrototypeOf(merged) is the attacker-controlled server spec. Downstream merged[serverName] lookups for names that are not own properties resolve through the poisoned prototype.
Suggested change
| const merged: Record<string, MCPServerConfig> = { | |
| const merged = Object.create(null) as Record<string, MCPServerConfig>; | |
| Object.assign(merged, sessionServers); | |
| Object.assign(merged, config.mcpServers as Record<string, MCPServerConfig>); |
The same pattern should be applied if any future code spreads config.hooks (which also comes from a null-prototype parseAgentHooks result) into a plain object.
— qwen3.7-max via Qwen Code /review
| subagentRegistry.discoverToolsForServer(name), | ||
| ), | ||
| ); | ||
| for (let i = 0; i < results.length; i++) { |
Collaborator
There was a problem hiding this comment.
[Critical] Per-agent MCP server discovery failures are silently dropped from the user's perspective.
Promise.allSettled rejections are logged only via debugLogger.warn, which is not user-visible in normal TUI operation. When a per-agent MCP server fails to start (wrong command, hung stdio, unreachable URL), the subagent spawns successfully, and then the agent gets opaque "tool not found" errors during execution. There is no user-visible connection between the symptom (missing tool) and the root cause (MCP discovery rejected at spawn time).
This is the most likely production failure mode for the feature: a user declares mcpServers in their agent frontmatter, the server fails to start, and the debugging trail requires DEBUG=subagent* to find the relevant log entry.
Suggested fix: Surface at least one failed-server name in a user-visible warning (e.g., via the FeedbackEmitter or an event on the AgentEventEmitter) so the TUI shows something like Agent "foo": MCP server "bar" failed to connect — tools from this server are unavailable. Alternatively, accumulate the failed names and return them alongside the context so the caller in agent.ts can include them in the tool result.
— qwen3.7-max via Qwen Code /review
This was referenced Jun 12, 2026
wenshao
pushed a commit
that referenced
this pull request
Jun 13, 2026
…'worktree'}) (#4721) (#5034) * feat(core): Workflow P3 — agent({schema, agentType, model, isolation:'worktree'}) (#4721) Adds the P3 dispatch options to the workflow runtime, completing the contract qwen-code's workflow tool matches against upstream Claude Code 2.1.168. P1/P2 stubs (workflow-sandbox.ts:508-527) are replaced with production paths routed through `SubagentManager.createAgentHeadless` so per-call model overrides go through `buildRuntimeContentGeneratorView` (provider routing), per-agent MCP servers / hooks get isolated lifecycles, and worktree-isolated subagents run against a rebound Config. - agent({agentType: 'X'}) resolves against the declarative-agents registry (#4842 + #4996) via findSubagentByName; unresolved names throw "agent({agentType}): agent type 'X' not found" verbatim from upstream. - agent({model: 'qwen3-max'}) is threaded into SubagentConfig.model so the runtime view sees it (modelConfigOverrides alone would only swap the model name within the existing provider's view). - Workflow's disallowed-tool floor [SendMessage, ExitPlanMode] is unioned with the agentType's own disallowedTools so a permissive agentType cannot re-enable them for a workflow subagent. - agent({isolation: 'worktree'}) provisions a fresh worktree via GitWorktreeService.createUserWorktree (slug agent-<7hex>, mirrors AgentTool 1849-1963), rebinds cwd/getTargetDir/getFileService/ getWorkspaceContext on a prototype-chained Config override, and on completion auto-removes the worktree if clean or preserves the path + branch (appended to the result string) when the subagent left changes. Parent-dirty trees are refused with a clear error to avoid silently running the subagent against a stale HEAD. - agent({isolation: 'remote'}) throws "agent({isolation:'remote'}) is not available in this build" verbatim (upstream 2.1.168 parity). - agent({schema: S}) injects a per-call SyntheticOutputTool (existing tools/syntheticOutput.ts, AJV-backed) into a fresh per-subagent ToolRegistry built via rebuildToolRegistryOnOverride, then watches AgentEventEmitter TOOL_CALL/TOOL_RESULT events for `structured_output` invocations. A successful call's args are captured as the dispatch return value (object, not string); after two failed attempts the third failure aborts the dispatch and throws "subagent completed without calling StructuredOutput (after 2 in-conversation nudges)" verbatim. No agent-core.ts changes — the entire 2-nudge counter lives in the dispatch layer so the shared subagent loop is unaffected. The sandbox's agent() wrapper now revives per-call object returns into the vm realm (JSON round-trip inside the vm runInContext block), closing the same T1/T8/T14 host-prototype-escape vector that P2's per-element revival closed for parallel/pipeline. Two new sandbox security tests (constructor-chain probe + non-JSON-serializable collapse) regress this. WorkflowAgentResult widens from `string` to `string | object`; the fast-path (no agentType/model/isolation/schema) is preserved byte-for-byte to keep P1/P2 zero-overhead. Tests: 159 workflow-suite tests + 217 adjacent (subagents / syntheticOutput / agent-override) all green. Real-LLM E2E follow-up planned (mirroring P2's 13/13 qwen3-max validation). Related #4721 (parent design — multi-phase, not closed by this PR) Related #4732 (P1 merged) #4947 (P2 merged) #4842 #4996 (declarative agents) * chore(core): P3 self-review R1 — align worktree suffix wording + 6 test gaps R1 of pre-push adversarial self-review on PR #5034 surfaced 6 confirmed findings across 6 diverse lenses (correctness / security / reuse-altitude / self-invariant / consumer-breakage / test-gaps). Each finding faced 2 independent skeptics defaulting to refuted=true; 6 survived majority challenge. Source code: - Worktree-preserved suffix wording now matches AgentTool's formatWorktreeSuffix (agent.ts:1700-1719) verbatim, including the `git worktree add <path> <branch>` recovery hint for the directory- removed-but-branch-preserved race. Test gaps closed: - schema-mode success after 1 nudge (round-2 args captured) - schema-mode success after 2 nudges (round-3 args captured) - schema-mode + agentType together — floor disallowedTools still unioned - schema-mode caller-abort takes priority over the StructuredOutput terminal error (signal.aborted check at workflow-orchestrator.ts:489-490) - override path dispose() runs in finally on the success path - override path dispose() runs in finally on the terminate-mode-error path Declined R1 finding: negative tests for invalid opt types (schema/model/ agentType passed null/number/empty-string). Adding upfront type validation is scope creep — upstream does not, P1/P2 do not, and the workflow tool is model-authored where these inputs are extremely unlikely. Existing AJV / SubagentManager downstream errors are descriptive enough. Will revisit if R2 makes a stronger case. 166/166 tests pass (workflow suite + adjacent + workflow-orchestrator). typecheck + lint clean across packages/core, packages/cli, integration-tests, sdk, webui. * chore(core): P3 self-review R2 — vm-realm opts revive + error-msg sanitize + 12 tests R2 of pre-push adversarial self-review on PR #5034. 6 diverse-lens finders (60 agents, ~2.5M tokens, 24 min) over the R1-fix-applied code, with 2 independent skeptics defaulting to refuted=true. 12 confirmed survivors after adversarial verify; decisions below. Security (FIX): - agent() wrapper in workflow-sandbox.ts now JSON-revives agentOpts inside the vm runInContext block BEFORE passing them to the host dispatch. Closes a Proxy/inherited-getter escape that P3 introduced along with the user-supplied schema object: a script could have wrapped agentOpts.schema in a Proxy whose getter ran host-side code during SyntheticOutputTool construction / AJV compile. Same mechanism as args / parallel-result revival. - runOverridePath now sanitizes opts.agentType through sanitizeForErrorMessage() (control chars → space) before interpolation into the "agent type 'X' not found" error message. Prevents a model-authored agentType containing CRLF / NUL from fragmenting a single-line error across log records / OTLP fields. Reuse-altitude (FIX): - Added JSDoc block to WorkflowWorktreeIsolation interface documenting each field's role for cleanup. Test gaps (FIX, 12 new tests): - agentType control-char sanitization regression - dispose() runs in finally when subagent.execute throws - isolation:'worktree' provision error branches (5): nested parent / git unavailable / not a git repo / parent dirty / createUserWorktree returns failure - isolation:'worktree' cleanup branches (3): removeUserWorktree fails / branchPreserved race / removeUserWorktree throws — each preserves the worktree (or branch) with the right user-facing suffix - combinations (2): model + isolation:'worktree' threads model AND provisions worktree; schema + isolation:'worktree' returns structured payload verbatim (preserved suffix only on string return) Test infrastructure: vi.mock'd GitWorktreeService at the module level (partial mock; preserves the existing exports the unrelated worktreeCleanup.ts depends on) with a per-test beforeEach reset. Declined R2 findings (kept the R1 line): - [major] Schema parameter upfront validation: same scope-creep decline as R1. Upstream doesn't do it; AJV's downstream error is descriptive enough. - [major] Worktree provision extracted to shared util with AgentTool: agreed in principle but out of P3 scope. A separate refactor PR should land that with AgentTool maintainers in the loop. 178/178 tests pass (workflow + adjacent suites). typecheck + lint clean across packages/core, packages/cli, integration-tests, sdk, webui. * fix(core): address wenshao R1+R2 review on Workflow P3 (PR #5034) Round 1 (15:41) + Round 2 (17:24) review from wenshao surfaced 7 inline findings across schema-mode dispatch correctness, worktree cleanup coverage, and error attribution. Each fix is paired with a regression test that was RED before the change landed. T0 [Critical] Worktree leak when schema setup throws after provision workflow-orchestrator.ts: outer try MOVED to start immediately after provisionWorkflowWorktree. Previously the try opened only after createSchemaConfigOverride / createSchemaModeState / signal listener attachment — so any throw in those three (broken MCP server during the per-call ToolRegistry rebuild was the trigger wenshao cited) orphaned the just-provisioned worktree under .qwen/worktrees/. Test: "isolation:'worktree' + schema setup throws → worktree is still cleaned up" — simulates createToolRegistry failure during createSchemaConfigOverride; asserts removeUserWorktree was called. T1 [Critical] / T4 [H1] agentType + schema silently dead-ended workflow-orchestrator.ts: schema-mode augmented config now (a) appends ToolNames.STRUCTURED_OUTPUT to baseConfig.tools when the allowlist is restricted (no '*' and doesn't already contain it), so prepareTools / getFunctionDeclarationsFiltered doesn't filter structured_output out of the subagent's surface; (b) preserves the resolved agentType's persona by APPENDING the schema-contract instruction block instead of replacing the systemPrompt outright. Replace remains only on the ephemeral no-agentType path where baseConfig.systemPrompt IS WORKFLOW_SUBAGENT_SYSTEM_PROMPT (schema variant is its strict superset; avoids two near-identical prompts). Tests: structured_output appears in the allowlist alongside the agentType's existing tools; persona prompt is contained in the effective systemPrompt. T2 [Suggestion] / T5 [M1] Parent-abort listener leaked per schema call workflow-orchestrator.ts: named listener stored at outer scope, removed in the outer finally regardless of how the dispatch ended. Previous `{ once: true }` only auto-removed on actual parent abort; the happy-path schema dispatch — success capture / 3-failure abort fires the CHILD controller without the parent ever aborting — left the listener stuck on the per-run signal. With N schema calls per workflow N listeners + N child-controller closures accumulated. Test: 5 sequential schema dispatches over the same parent signal end with zero live listeners. T6 [M2] Terminate mode misdiagnosed as nudge exhaustion workflow-orchestrator.ts: schema path now distinguishes terminateMode before attributing failure to schema mode. TIMEOUT / MAX_TURNS / ERROR throw the existing "did not complete (terminate mode: X)" message that the non-schema path uses. Only the actual schema-failure cases produce schema wording, and those are split: attempts > 2 keeps the upstream-verbatim "(after 2 in-conversation nudges)" wording; attempts === 0 throws an accurate "no validation attempt — model produced plain-text content" instead of misleadingly citing nudges that never happened. (The existing 0-call test was updated to match the new accurate message; the 3-failure test retains the verbatim wording.) Tests: parametric over TIMEOUT/MAX_TURNS/ERROR asserting "did not complete"; companion test pinning the verbatim wording to the 3-failure path. T3 [Suggestion] Schema-mode JSON revival sentinel — clarified workflow-sandbox.ts: added a block comment documenting that the JSON-round-trip + null-on-throw is a SECURITY backstop (errors-as-data convention from parallel/pipeline) rather than a contract path — unreachable in production schema mode because the host return is LLM tool_call args, always JSON-serializable. No behavior change. Tests: 75/75 orchestrator + 111/111 sandbox/tool/limiter green. typecheck + lint clean across packages/core and packages/cli. R1+R2 self-review commits (e1c5ec7 / 62624a9) precede this commit on the same branch — they predate wenshao's review and address distinct findings; reviewer L1 (worktree-lifecycle unit coverage) is already closed by R2's 11 worktree tests.
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
4 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.
What this PR does
Follow-up to PR #4842 and PR #4870 that closes the remaining gap for Claude Code 2.1.168 declarative-agent compatibility:
mcpServersandhooksfrontmatter fields now parse, round-trip safely on save, and actually fire at runtime when a subagent runs. Also replaces the YAML stringifier with eemeli/yamlso PR #4870's parser change is paired with a symmetric writer (the hand-rolled stringifier left in place by #4870 emits[object Object]for any value below one level of nesting).The change is split across three commits for review-friendliness: (1) yaml-parser stringify replacement; (2)
SubagentConfigsurface + runtime wiring + tests; (3) docs (docs/yaml-parser-replacement.mdaudit,docs/declarative-agents-port.mdstatus update,docs/users/features/sub-agents.mdexamples + v1 hooks scope-limitation callout).Why it's needed
PR #4842 deferred
mcpServersandhooksexplicitly because the local YAML parser at the time could not represent nested CC shapes. PR #4870 then made the parse side nested-aware. With the writer still hand-rolled, a.qwen/agents/*.mdwith nested fields would parse correctly but get corrupted on save — and there was no runtime path for either field even if it survived round-trip. This PR makes a CC agent file with per-agentmcpServersandhookswork end-to-end the same way it does in CC: drop it into.qwen/agents/, the per-agent MCP server is reachable from inside the subagent's tool registry, and the per-agent hook fires for the subagent's tool calls.Reviewer Test Plan
How to verify
Unit + integration tests:
End-to-end (real model + real MCP child process):
npm run bundleto builddist/cli.js./tmp/qwen-mcp-hooks-e2e/):.qwen/settings.jsonregisterssession-mcp(exposessession_motto)..qwen/agents/researcher.mdfrontmatter declares per-agentmcpServers: agent-mcp(exposesagent_quote) and aPreToolUsehook that appends a line tohook-fired.logon every tool call.node dist/cli.js \"Do exactly these 3 steps in order: (1) call session_motto, report its result. (2) Use the researcher subagent to fetch the secret quote. (3) Call session_motto again, report its result.\" --approval-mode yolo --output-format json.mcp_serversinit list contains onlysession-mcp, notagent-mcp(per-agent server is scoped to the subagent only).mcp__agent-mcp__agent_quotedirectly (no shell fallback) and returnsagent-quote-secret-marker-9999.agent-mcp-spawned.logshows exactly one spawn with a full MCP handshake (initialize → tools/list → tools/call).hook-fired.loghas exactly one entry (the subagent's single tool call). The parent's two pre/postsession_mottocalls and the Task spawn itself do not fire the per-agent hook.Evidence (Before & After)
mcpServersparsed from frontmatterhooksparsed from frontmatterserializeSubagent)mcpServers:\\n filesystem: [object Object](handrolled stringifier; verified bynode -e ...)yaml.stringifymcp__agent-mcp__agent_quoteshows up in subagent'stool_search, spawn handshake completes once, tool call routes through proper clientTested on
Environment
Local: macOS 25.5.0, Node via packaged runtime,
npm run bundle && node dist/cli.js. No sandbox;--approval-mode yolofor the E2E.Risk & Scope
buildSubagentContextOverridewhen per-agentmcpServersare present. This bypasses the skip-rebuild optimization thatcreateApprovalModeOverriderelies on. The justification is that without a fresh registry anchored on the override Config, the pre-existing wrapper-ownedMcpClientManagerresolvescliConfig.getMcpServers()to the parent's session list and never sees the merged set — the per-agent discovery loop downstream silently no-ops. Discovered by E2E debugging; spelled out as a comment at the rebuild site. Cost is one extracreateToolRegistryper subagent spawn that declaresmcpServers, only.hooksentries live in the registry, they fire for every event of their declared type, regardless of which agent is currently active. Two concurrent subagents with different per-agent hooks would see each other's. Per-agent scope filtering at firing time is left to a follow-up. Documented inHookRegistry.addAgentHooksJSDoc and as a blockquote in the user-facing doc, so users picking up the feature know to prefer fire-globally-safe handlers (logging) over behavior-mutating ones.crypto.randomUUID, no native modules) and the MCP child-process model is shared across platforms, so I don't expect platform-specific regressions, but I haven't verified directly.SubagentConfig. Existing.qwen/agents/*.mdfiles that don't declaremcpServers/hookssee no behavior change.Linked Issues
Closes #4821 (the deferred-fields portion that PR #4842 documented as follow-up work).
中文说明
这个 PR 做了什么
PR #4842 和 PR #4870 之后的收尾工作，把 Claude Code 2.1.168 declarative-agent 兼容性剩下的最后一块补齐：
mcpServers和hooks两个 frontmatter 字段现在解析、序列化回盘、以及实际在 subagent 运行时生效都通了。同时把 yaml-parser 的stringify也换成 eemeli/yaml,让 PR #4870 的 parse 端改动有一个对称的写端 —— #4870 保留的手写 stringify 对任何二层以下嵌套都输出[object Object]。变更拆成三个 commit 方便 review:(1) yaml-parser stringify 替换;(2)
SubagentConfig字段补全 + 运行时接线 + 测试;(3) docs (docs/yaml-parser-replacement.md审计、docs/declarative-agents-port.md状态更新、docs/users/features/sub-agents.md使用示例 + hooks v1 scope 限制说明)。为什么需要
PR #4842 明确把
mcpServers和hooksdeferred,因为当时本地 yaml-parser 不能表达嵌套 CC 结构。PR #4870 修了 parse 端。但 stringify 还是手写的,所以带嵌套字段的.qwen/agents/*.md能读对、写出来就坏了 —— 而且即使 round-trip 没事,运行时也根本没有路径让这两个字段生效。这个 PR 让 CC agent 文件里的 per-agentmcpServers和hooks在 qwen-code 里端到端工作的方式和在 CC 里一致:把文件丢到.qwen/agents/,subagent 的 tool registry 里就能调到 per-agent MCP server 的工具,per-agent hook 也会在 subagent 的 tool call 上触发。Reviewer Test Plan
怎么验证
单测 + 集成测试:
端到端(真模型 + 真 MCP 子进程): 见上方英文 "How to verify" 步骤。
/tmp/qwen-mcp-hooks-e2e/下有完整的 fixture 布局。重点检查 4 条:parent scope 看不到 per-agent MCP server、subagent 直接调到mcp__agent-mcp__agent_quote不走 shell fallback、agent-mcp 进程只被 spawn 一次握手完整、hook log 只有一行匹配 subagent 的那次 tool call。Before / After
见上方英文表格,无需重复。
Risk & Scope
buildSubagentContextOverride里当 per-agentmcpServers存在时强制重建 tool registry,绕过了createApprovalModeOverride依赖的 skip-rebuild 优化。理由是不重建的话 wrapper 持有的McpClientManager用的是 parent 的 session list,根本看不到 merged 集合,下游 discovery loop 会静默 no-op。这是 E2E 调试时发现的,代码里有详细注释。代价只是 declare 了mcpServers的 subagent 多跑一次createToolRegistry。HookRegistry.addAgentHooksJSDoc 和用户文档里都有显式说明,建议 v1 阶段只用对全局触发安全的 handler(如日志)。crypto.randomUUID,无 native),MCP 子进程模型跨平台共享,不预期有平台特定回归,但没直接验过。SubagentConfig新增的是可选字段,不声明mcpServers/hooks的现存.qwen/agents/*.md行为不变。Linked Issues
Closes #4821(PR #4842 文档里 deferred 的字段部分)。