feat(core): declarative agent frontmatter v1 — permissionMode bridge + maxTurns wiring + color allowlist (CC 2.1.168 parity)#4842
Merged
Conversation
…new fields Adds agent-frontmatter-schema.ts as the single source of truth for the CC 2.1.168 declarative-agent enum constants (EFFORT_VALUES, PERMISSION_MODE_VALUES, MEMORY_VALUES, ISOLATION_VALUES, COLOR_VALUES) and lenient parsers (parseEffort, parseMaxTurns, parseBackground, parseStringOrArray) that mirror DL7's warn-and-drop posture. Also adds a permissionModeToApprovalMode bridge to map CC's permission modes onto qwen-code's existing approvalMode semantics. Extends SubagentConfig with 9 optional fields carried verbatim from CC frontmatter: permissionMode, effort, maxTurns, skills, initialPrompt, memory, isolation, mcpServers, hooks. Runtime semantics for the metadata-only fields are deferred to follow-up PRs; this lands the data carrier. Refs #4821 #4721 #4732
…ields Extends parseSubagentContent and serializeSubagent to round-trip the 9 CC 2.1.168 fields previously added to the SubagentConfig type. Parsing follows DL7's lenient warn-and-drop posture (vs the existing strict throw posture used for approvalMode), so a Claude Code agent file with an invalid optional field still parses with that field dropped — matching CC behavior so users can drop CC agent files into .qwen/agents/ unchanged. Adds a permissionMode → approvalMode bridge: when frontmatter has permissionMode but no approvalMode, the bridge resolves the approvalMode at parse time using the same mapping as claude-converter.ts. If both are set, approvalMode wins (already-explicit values take precedence over inferred ones). Tests: - 22 new parser cases covering happy paths, invalid drops, permissionMode bridge precedence, color allowlist (with auto sentinel preserved), and loose-validation passthrough for mcpServers / hooks. - 9 new serializer cases asserting round-trip and omit-when-unset behavior. Refs #4821 #4721 #4732
…ert time Wires the new top-level `maxTurns` field into the existing runtime configuration pipeline so the value declared in agent frontmatter actually limits the agent's turn budget at run time. When both the top-level `maxTurns` and the legacy nested `runConfig.max_turns` are set, the top-level field wins (more specific and matches the CC frontmatter shape upstream agent files use). When only the nested field is set, behavior is unchanged. Refs #4821 #4721 #4732
…ay in claude-converter Replaces the inline claudeToQwenMode table and the local parseStringOrArray helper in claude-converter.ts with the shared exports from agent-frontmatter-schema.ts. One source of truth for the CC ↔ qwen mapping keeps the two import paths (.qwen/agents/*.md frontmatter and Claude plugin import) in sync — when the upstream CC schema changes, only one table needs updating. Adds explicit tests for the bridge mapping (six known modes + unknown fallback). The fallback now happens at the call site rather than inside the table, but the observable behavior is unchanged. Refs #4821
Adds a Claude Code Compatibility Fields section to the user-facing subagents reference, covering the 9 new frontmatter fields landed in this PR. The intent is for users with existing Claude Code agents to know they can drop the files into `.qwen/agents/` and have them parse identically. Refs #4821
Replaces the inline boolean-or-string lenient parse for the background field with the shared parseBackground util added in the schema module. Same observable behavior; one fewer place to keep in sync when the upstream shape changes. Refs #4821
Four self-review findings caught and fixed before opening the PR: 1. **mcpServers/hooks round-trip claim was broken.** The local yaml-parser only formats one level of nesting, so serializing an agent with nested mcpServers or hooks would mangle the value into '[object Object]'. The fields are still carried verbatim in memory (read path is fine), but the serializer no longer emits them — losing the field through '/agents' edits is strictly safer than corrupting it. Documented the limitation in docs/users/features/sub-agents.md. 2. **approvalMode throw vs permissionMode lenience asymmetry.** The pre-existing approvalMode parser threw on invalid values, killing the entire agent file. With the new permissionMode bridge, a CC-imported file with 'permissionMode: bypassPermissions' + 'approvalMode: tpyo' would reject the file instead of dropping the typo and using the bridge. Demoted approvalMode to the same DL7-parity warn-and-drop posture all the new fields use; the bridge now runs when approvalMode is invalid. 3. **parseEffort missed numeric strings.** CC's DL7 falls back to parseInt for non-enum effort strings so 'effort: "5"' (quoted YAML) round-trips like 'effort: 5'. Added the same fallback and tests for floats / partial numeric strings / valid numeric strings. 4. **convertAgentFiles stripped 6/9 of the new fields.** The Claude plugin import path read frontmatter into a fixed ClaudeAgentConfig shape, so 'effort', 'maxTurns', 'initialPrompt', 'memory', 'isolation', 'mcpServers', and 'background' were silently dropped when installing a CC plugin agent. Build the rewritten frontmatter from the original keys first, then overlay the converter's transformations for the keys it owns — unknown CC fields now passthrough verbatim, future-proofing against later CC additions. Refs #4821 #4721 #4732
Collaborator
|
Thanks for the PR @LaZzyMan — this is a thorough and well-researched port. Template: The PR body doesn't follow the PR template structure. Missing sections: "What this PR does", "Why it's needed", "Reviewer Test Plan" (with "How to verify", "Evidence (Before & After)", "Tested on" table), "Risk & Scope", and "中文说明". The content is all there — excellent motivation, coordination matrix, and adversarial self-review — but the headings need to map to the template so reviewers can find things quickly. Could you restructure when you get a chance? Not blocking the review. Direction: Strong alignment. Claude Code's declarative-agent frontmatter is a well-established feature (CHANGELOG shows multiple iterations: Approach: The scope is well-scoped despite the large diff (1830 additions). Stripping it down: ~200 lines of schema constants, ~100 lines of parser/serializer changes, ~60 lines on types, ~40 lines in the converter, and the rest is tests + the design doc. The "data carrier only, runtime semantics deferred" posture is the right cut — it ships the schema contract that the workflow PR (#4732) needs without trying to wire everything at once. The One observation worth flagging: demoting Moving on to code review. 🔍 中文说明感谢 @LaZzyMan 的贡献——这是一个深入且研究充分的移植。 模板： PR 正文未遵循 PR 模板 结构，缺少 "What this PR does"、"Why it's needed"、"Reviewer Test Plan"（含 "How to verify"、"Evidence (Before & After)"、"Tested on" 表格）、"Risk & Scope" 和 "中文说明" 等章节。内容本身很充实——优秀的动机说明、协调矩阵和对抗性自审——但标题需要对应模板以便审查者快速定位。方便时请重新组织一下结构，不会阻塞审查。 方向： 高度对齐。Claude Code 的声明式 agent frontmatter 是一个成熟的功能（CHANGELOG 显示多次迭代），移植到 方案： 尽管 diff 很大（1830 行新增），范围控制得当。约 200 行 schema 常量、100 行解析器/序列化器改动、60 行类型、40 行转换器，其余是测试和设计文档。"仅数据载体，运行时语义延后"的立场是正确的切割。 值得注意的一点：将 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
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. |
Second adversarial review surfaced 5 more findings; all fixed: 1. **convertAgentFiles passthrough corrupted nested objects.** Round-1 fixed serializeSubagent to skip emitting mcpServers/hooks (the local yaml-parser collapses nested values to '[object Object]'). The plugin-import passthrough I added to claude-converter.ts had the same mangle bug — a CC plugin agent with nested mcpServers got written to disk as 'mcpServers:\n - [object Object]'. Now skips both fields via a NESTED_FIELDS_NOT_ROUND_TRIPPABLE set. 2. **parseEffort accepted 0 and negative integers.** Docs say 'positive integer' and parseMaxTurns already rejects <= 0. parseEffort didn't, accepting '0' / '-5' / 0 / -1 as valid efforts. Aligned both helpers and added tests for the boundary values. 3. **Function name collision: permissionModeToApprovalMode.** packages/core/src/ tools/agent/agent.ts has a module-private permissionModeToApprovalMode that maps the qwen PermissionMode enum to ApprovalMode enum (different domain entirely). My new exported helper used the same name; IDE auto-import would silently return undefined for every qwen enum value. Renamed the export to claudePermissionModeToApprovalMode and updated 4 callers. 4. **color: 'auto' round-trip asymmetry.** Round-1 added a test pinning that the parser preserves the legacy 'auto' sentinel. But serializeSubagent already had a pre-existing 'skip emit when auto' branch, so a parse → serialize → parse cycle dropped the sentinel. The CLI helpers (shouldShowColor / getColorForDisplay) already treat 'auto' identically to undefined, so normalizing 'auto' to undefined at parse time has no downstream effect AND makes round-trip cleanly idempotent. 5. **Converter approvalMode precedence diverged from loader.** When a CC source file had both 'permissionMode: bypassPermissions' AND 'approvalMode: default', the convertClaudeAgentConfig path wrote 'approvalMode: yolo' (bridge wins). The loader's rule is 'approvalMode wins over bridge'. Extended ClaudeAgentConfig with an approvalMode field and gated the bridge emit on 'source approvalMode is unset', aligning convert and load precedence. Refs #4821 #4721 #4732
…contract Three quality findings from the round-3 adversarial pass (after rounds 1+2 fixed 9 issues each). All low-severity but cheap; the runtime was correct, the file-on-disk + public-API surface needed tightening. 1. **Stale runConfig.max_turns duplicated on every write.** Top-level maxTurns was promoted in 9b903ee but the serializer still emitted runConfig.max_turns verbatim. A file with both fields kept both indefinitely. The runtime already prefers top-level (so behavior is correct), but a third-party tool or future viewer reading the legacy nested field would act on stale data. Prune nested max_turns from the serialized runConfig when top-level maxTurns is set; drop the runConfig block entirely if pruning leaves it empty. Three new tests pin the prune, the drop-empty-block path, and the no-top-level fallback. 2. **14 internal symbols leaked into the public package API.** The PR re-exported every helper from agent-frontmatter-schema.ts via subagents/index.ts. grep against packages/cli + packages/vscode-ide-companion finds zero cross-package consumers — both internal users (subagent-manager, claude-converter) already import directly from the schema file. Locking constant names like EFFORT_VALUES and parseEffort in the public API would constrain follow-up PRs (e.g. when js-yaml lands and the schema shape changes). Removed the entire export block; left a comment explaining the choice so a future PR knows the bar for re-introducing it. 3. **convertClaudeAgentConfig silently dropped a standalone approvalMode.** Round-2 gated the permissionMode bridge on !claudeAgent.approvalMode for precedence parity with the loader, but never added the explicit copy for approvalMode-only callers. Result: caller passes {approvalMode: 'plan'} → output has no approvalMode at all. The in-tree convertAgentFiles path recovered via the unowned-key passthrough, but exported direct-call surface needs the explicit copy. Two new tests pin both the standalone case and the precedence-when-both case. Also: pinned yaml-parser limitation tests (added in the round-3 e2e independent check) document that the lightweight parser cannot round-trip nested mcpServers/hooks; documentation + types comments updated to call out the carve-out so users know the field works only for flat forms until js-yaml lands. Refs #4821 #4721 #4732
Reduces this PR to ship only the fields that have an end-to-end runtime path today: permissionMode (bridges to existing approvalMode), maxTurns (wires into runConfig.max_turns), and a tightened color allowlist. Everything else gets deferred to follow-up PRs that first land the prerequisite infra they need. Removed: - effort (no model-layer effort param in qwen providers) - mcpServers / hooks (need nested-aware YAML parser — yaml-parser.ts is hand-rolled and only handles 1 level) - memory (qwen's auto-memory has no user/project/local scope distinction) - isolation (workflow PR #4732 owns the runtime) - initialPrompt (needs --agent CLI flag, no main-session-agent infra) - skills (needs SkillManager consumption of config.skills) This drops 7 fields from SubagentConfig + types.ts, deletes their parser / serializer / test code, deletes the unused enum constants and helpers from agent-frontmatter-schema.ts (EFFORT_VALUES, EFFORT_ALIASES, MEMORY_VALUES, ISOLATION_VALUES, parseEffort, isMemory, isIsolation), and trims the user docs to the supported field set with a pointer to the design doc for the deferred fields. Why ship the design doc alongside such a narrow v1: the full reverse-engineering record in docs/declarative-agents-port.md (DL7/Ig5/GN/_Y constants, error messages, schema parity decisions, coordination matrix with workflow PR #4732) stays load-bearing for the follow-up PRs. A status table at the top discloses what's deferred and why. Net: -559 LOC across 7 files. Final PR is permissionMode bridge + maxTurns wiring + color allowlist + design reference doc + the yaml-parser nested-limitation pin tests added during round-3 review. Refs #4821 #4721 #4732
The previous shrink left round-1/2/3 fixes in place that were originally for the wider 9-field scope; with the carry-only fields gone they're no longer needed. Reverting them all back to pre-PR behaviour: - approvalMode strict throw (round-1 demoted to lenient; defer the symmetry fix to a separate PR if wanted) - runConfig.max_turns prune on serialize (round-3 cleanup; defer) - color: 'auto' normalised to undefined (round-2 round-trip fix; defer) - claude-converter NESTED_FIELDS_NOT_ROUND_TRIPPABLE + CONVERTER_OWNED_KEYS + passthrough loop (round-1/2 — all for the deleted carry-only fields) - ClaudeAgentConfig.approvalMode + precedence gating (round-3) - parseBackground in shared schema module (the dedup wasn't pulling weight) - parseStringOrArray in shared schema module (same) The shared bridge claudePermissionModeToApprovalMode + the disambiguated name stay — converter and loader use the same map and the name collision risk is real. Net: -336 LOC vs the prior shrink commit.
The previous shrink left two stale claims in docs/users/features/sub-agents.md that referenced behaviour reverted in the round-X-revert commit: - maxTurns row claimed 'the legacy nested value is pruned from the on-disk file on save to avoid two sources of truth' — the prune was reverted - color row claimed 'auto is accepted on read and normalised to undefined for round-trip parity' — the normalisation was reverted; auto is preserved as-is for backward compat Updated both rows to match what the parser actually does now. The design doc (docs/declarative-agents-port.md) is unchanged: its top-line disclaimer already labels the body as reference material for follow-up PRs, so the references to the wider plan (D7 bridge, P1-P4 phases) are accurate-as-reference even though some details are deferred.
DragonnZhang
reviewed
Jun 8, 2026
Collaborator
Author
E2E Test ReportReal-world end-to-end runtime test for the 3 shipped fields. Setup writes real Result: 21/21 pass. Test breakdown
|
| # | Scenario | Expected | Result |
|---|---|---|---|
| 1 | permissionMode: bypassPermissions |
config.approvalMode === 'yolo' |
✓ |
| 2 | permissionMode: plan |
config.approvalMode === 'plan' |
✓ |
| 3 | permissionMode: acceptEdits |
config.approvalMode === 'auto-edit' |
✓ |
| 4 | permissionMode: dontAsk (restrictive intent) |
config.approvalMode === 'default' (not auto-edit) |
✓ |
| 5 | both permissionMode: bypassPermissions AND explicit approvalMode: plan |
config.approvalMode === 'plan' (explicit wins) |
✓ |
| 6 | permissionMode: not-a-mode |
config.permissionMode === undefined, no bridge fired |
✓ |
maxTurns runtime wiring (4/4)
| # | Scenario | Expected | Result |
|---|---|---|---|
| 7 | maxTurns: 5 |
convertToRuntimeConfig().runConfig.max_turns === 5 |
✓ |
| 8 | top-level maxTurns: 99 + nested runConfig.max_turns: 5 |
runtime.runConfig.max_turns === 99 (top-level wins) |
✓ |
| 9 | only runConfig.max_turns: 7 (legacy) |
runtime.runConfig.max_turns === 7 (back-compat) |
✓ |
| 10 | maxTurns: -5 |
dropped, no runtime override | ✓ |
color allowlist (10/10)
| # | Scenario | Expected | Result |
|---|---|---|---|
| 11-18 | CC _Y values: red, blue, green, yellow, purple, orange, pink, cyan |
preserved on config.color |
✓×8 |
| 19 | legacy qwen sentinel auto |
preserved (back-compat) | ✓ |
| 20 | color: magenta (not in allowlist) |
silently dropped | ✓ |
Drop-in CC compatibility (1/1)
| # | Scenario | Expected | Result |
|---|---|---|---|
| 21 | Real CC .md file with 11 frontmatter fields including all 7 deferred ones (effort, skills, initialPrompt, memory, isolation, mcpServers, background) and 4 shipped ones (permissionMode, maxTurns, color, plus the existing background honoured here) |
Does NOT throw. Shipped fields take effect (approvalMode === 'auto-edit', maxTurns === 50, color === 'purple', runtime.runConfig.max_turns === 50). Deferred fields NOT surfaced on the typed SubagentConfig. |
✓ |
What this proves
- A CC plugin author's
.claude/agents/foo.mdfile withpermissionMode: bypassPermissions, dropped into a qwen-code project's.qwen/agents/, will load and run withapprovalMode: yoloend-to-end. The bridge is not just a constant — the parsedSubagentConfigcarries the resolvedapprovalMode, and the existing approval system honours it as it does for any other source. - A file with
maxTurns: 50actually caps the agent at 50 turns at runtime, not just on paper.convertToRuntimeConfigis the same function theAgenttool calls before spawning a subagent. - The
colorallowlist tightens what's accepted but does not break the legacyautosentinel that the CLI display helpers (shouldShowColor,getColorForDisplay) already key off. - The 7 deferred CC fields are silently ignored — drop-in compatibility is real even when only 3 of 15 fields have runtime semantics.
Methodology note
The e2e suite is a dedicated test file (__pr4842_e2e__.test.ts) run via vitest with mock-free production code paths. The file is not committed — it was scaffolding to produce this report. The same code paths are also covered by the unit tests already in subagent-manager.test.ts (94 cases) and agent-frontmatter-schema.test.ts (13 cases), which mock parseYaml for speed but exercise the same logic. The e2e suite added value by reading real files through the real YAML parser, eliminating the "mock might lie" failure mode.
Setup
- Build:
packages/coretypecheck clean; bundle build is blocked by aninkversion mismatch in the worktree (unrelated to this PR), so the test ran through vitest directly. - Runtime: 3.57s total, 12ms test execution.
- Environment: node 22.21.1 / vitest 3.2.4 / darwin-arm64.
…pe-chain footgun)
The PERMISSION_MODE_TO_APPROVAL_MODE table was a plain object accessed with
`[key]`. Calling claudePermissionModeToApprovalMode('__proto__') walked the
prototype chain and returned Object.prototype — a non-string value that
violated the declared return type. The current loader path was safe because
isPermissionMode() filtered prototype-key strings before the bridge ran, but
the exported function itself was a latent footgun for any future caller
that didn't know to pre-filter.
Switched the lookup table to a Map so prototype keys cannot be reached.
Added explicit tests for __proto__, constructor, hasOwnProperty, toString.
Refs #4821
Two findings: 1. `serializeSubagent` wrote both `permissionMode` and the bridge-derived `approvalMode` into the same frontmatter block. On the next load the parser takes `approvalMode` (explicit wins over bridge), so `permissionMode` silently became dead frontmatter — a user editing it later in the file would be ignored. Skip the `permissionMode` emit when `approvalMode` is also being written; `permissionMode` still round-trips when it's the user's only intent. Two new tests pin both branches. 2. `subagents/index.ts` had a NOTE comment referencing `EFFORT_VALUES` and `parseEffort` as example schema helpers that intentionally aren't re-exported. Those symbols were removed during the v1 scope shrink (637b8b7) and don't exist in the current module. Updated the comment to reference `claudePermissionModeToApprovalMode`, `parseMaxTurns`, and `isPermissionMode`, which are the actual current contents. Sibling-drift note: the same dual-emit pattern exists for `maxTurns` vs `runConfig.max_turns` — round-X revert (70a876d) explicitly deferred the `runConfig.max_turns` prune. Not addressed here per that earlier decision. Refs #4842
Collaborator
Author
Review round 2 — triage summaryResolving commit:
Net change this round: 3 files / +34 / −6 LOC. 205 tests passing, typecheck clean. Sibling-drift note: the same dual-emit pattern Thread #1 flags also exists for |
wenshao
reviewed
Jun 9, 2026
Collaborator
Author
Review round 3 — triage summaryAll 4 findings from @wenshao's
Round-weighted discipline: v1 round 3 Suggestion → overthinking. Two of the four (#1 and #4) are surfaced siblings of items the v1 shrink explicitly deferred — accepting them would re-establish the round-of-polish drift the shrink was meant to prevent. CI infrastructure note: the |
wenshao
approved these changes
Jun 10, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅ — qwen3.7-max via Qwen Code /review
| // claudePermissionModeToApprovalMode, parseMaxTurns, isPermissionMode) | ||
| // live in `agent-frontmatter-schema.ts` and are intentionally NOT | ||
| // re-exported here — they are internal to the `SubagentManager` / | ||
| // `claude-converter` parse paths and locking their names in the |
Collaborator
There was a problem hiding this comment.
[Suggestion] The comment says these helpers are "internal to the SubagentManager / claude-converter parse paths", but claude-converter.ts does not import anything from agent-frontmatter-schema.ts — it retains its own inline mapping table. The comment names a consumer that does not exist, which could mislead a future developer into thinking consolidation has already happened.
Suggested change
| // `claude-converter` parse paths and locking their names in the | |
| // NOTE: declarative-agent schema helpers (e.g. | |
| // claudePermissionModeToApprovalMode, parseMaxTurns, isPermissionMode) | |
| // live in `agent-frontmatter-schema.ts` and are intentionally NOT | |
| // re-exported here — they are internal to the `SubagentManager` | |
| // parse path and locking their names in the package's public API | |
| // would constrain follow-up PRs (e.g. when `js-yaml` lands and the | |
| // schema shape changes). Re-introduce specific exports here when a | |
| // cross-package caller actually needs them. |
— qwen3.7-max via Qwen Code /review
| * Top-level promotion of the legacy `runConfig.max_turns` field; when both | ||
| * are set, top-level `maxTurns` wins. | ||
| */ | ||
| maxTurns?: number; |
Collaborator
There was a problem hiding this comment.
[Suggestion] The new top-level maxTurns field bypasses the existing >100 warning in SubagentValidator.validateConfig(). The validator checks config.runConfig.max_turns and warns about high turn limits, but never inspects config.maxTurns. Since the design doc positions maxTurns as the replacement for runConfig.max_turns, users migrating to the promoted field lose the resource-protection guardrail. Concretely: maxTurns: 5000 in frontmatter produces no warning, while runConfig: { max_turns: 5000 } does.
Add a maxTurns check in SubagentValidator.validateConfig(), mirroring the existing >100 warning:
if (config.maxTurns !== undefined && config.maxTurns > 100) {
warnings.push('Very high turn limit (>100) may cause long execution times');
}— qwen3.7-max via Qwen Code /review
Collaborator
Local verification reportIndependently verified this PR locally on Linux (Node v22.22.2) at head 1. Unit suites + types (PR code)2. Red/green — the tests actually pin the fixReverted the four impl files to the PR's parent commit
Restored PR code afterwards; suite back to 205/205. 3. E2E against the real compiled artifact (dist)Drove
4. Real CLI run (built binary, tmux-driven TUI)Launched
5. Merge readiness
Non-blocking observation (pre-existing, out of scope)The core allowlist now accepts CC's full 8-color set, but the CLI's display palette ( Verdict: behavior matches the PR description on every claim I could test locally; safe to merge from a functional standpoint. |
DragonnZhang
approved these changes
Jun 10, 2026
DragonnZhang
left a comment
Collaborator
There was a problem hiding this comment.
No additional findings. PR has been thoroughly reviewed across multiple passes — all findings resolved or explicitly deferred with clear justification. CI green (Test ×3 OS, Lint, CodeQL, Coverage). Tests are comprehensive (205 passing, red/green proven). Code quality is high with defensive parsing, prototype-chain safety, and good documentation.
Ready to merge.
— Qwen Code automated review
LaZzyMan
added a commit
that referenced
this pull request
Jun 12, 2026
…ity follow-up) (#4996) * fix(core): replace yaml-parser stringify with eemeli/yaml for safe nested 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. * feat(core): port declarative-agent mcpServers + hooks end-to-end 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(core): declarative agents follow-up + yaml-parser audit - `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. * fix(core): self-review round 1 — proto-pollution defense + parallel MCP 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. * fix(core): pr #4996 review round 1 — leak fixes via explicit dispose 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. * refactor(core): /simplify cleanup pass on the dispose contract 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.
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
5 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
Adds a small, vertically-sliced v1 of Claude Code 2.1.168 declarative agent frontmatter parity:
permissionMode(CC enum:acceptEdits/auto/bypassPermissions/default/dontAsk/plan) — bridged to qwen's existingapprovalModeat parse time so a CC agent file's permission mode resolves to qwen semantics. Bridge uses the same mapping asclaude-converter.ts(single source of truth inagent-frontmatter-schema.ts).maxTurns(top-level positive integer) — wired into the existingrunConfig.max_turnsruntime path so the limit is actually enforced. Top-level wins when both are set.colorallowlist — tightens the existing field to CC's_Yset (red, blue, green, yellow, purple, orange, pink, cyan) with the legacy qwenautosentinel preserved for backward compat. Other values warn-and-drop on parse.Plus an internal design doc at
docs/declarative-agents-port.mdcapturing the full reverse-engineering of CC 2.1.168'sDL7loader,Ig5shadow zod, the$E/GN/_Yenum constants, and the coordination matrix with workflow port PR #4732 — load-bearing reference for the follow-up PRs.What's NOT in this PR (and why)
Issue #4821 lists 15 fields. This PR ships 3 (
permissionMode,maxTurns,colorallowlist). The other 12 are deferred because each needs prerequisite infra that doesn't exist yet:efforteffort/reasoning_effortparameter (qwen providers don't expose one)mcpServers,hooksyaml-parser.tsonly handles 1 level — pin tests inyaml-parser.test.tsdocument the limit)memoryuser/project/localscope distinctionisolationinitialPrompt--agentCLI flag (no main-session-agent infra)skillsconfig.skillsA follow-up task for
mcpServers+hooks(referencing CC's YAML parsing approach) is queued.Coordination with workflow port (#4721 / PR #4732)
The agent registry (
SubagentManager.loadSubagent) is the contract — workflow'sopts.agentTypelooks up against the same key (name) this PR's fields flow through, so per-agent defaults compose with per-call workflow overrides cleanly once #4732 P3 lands. Tool-name canonical form for workflow's hardcodeddisallowedToolsfloor isToolNames.SEND_MESSAGE+ToolNames.EXIT_PLAN_MODE(verified against PR #4732 diff).LOC breakdown (1038 total, 9 files)
claude-converter.tsis untouched — the dedup wasn't pulling weight.Test plan
npx vitest run src/subagents/ src/extension/claude-converter.test.ts src/utils/yaml-parser.test.ts— 202 passingnpx tsc --noEmit -p packages/core/tsconfig.json— cleanRefs: #4821 #4721 #4732 #2409