qwen-code/docs/yaml-parser-replacement.md
顾盼 e25d7eec04
feat(core): port declarative-agent mcpServers + hooks (CC 2.1.168 parity 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 720f0e4a1):

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.
2026-06-12 14:15:51 +08:00

27 KiB
Raw Blame History

YAML parser replacement — research findings

Internal design document for replacing the hand-rolled 192-line YAML parser at packages/core/src/utils/yaml-parser.ts with a real library, so the deferred mcpServers and hooks fields from Claude Code's declarative-agent schema can round-trip safely through subagent / skill / converter code paths.

Companion to docs/declarative-agents-port.md. Issue: #4821. Prereq for the follow-up to PR #4842.

Phase 0 — Sources verified

Source Version / Date Why authoritative
~/code/claude-code/src/utils/yaml.ts older CC snapshot (pre-2.1.168) direct source — 15-line wrapper that names the library
~/code/claude-code/src/utils/frontmatterParser.ts same snapshot direct source — 370-line frontmatter splitter + 2-pass recovery
/private/tmp/cc-2.1.168/claude.strings extracted from CC 2.1.168 authoritative for current behavior — strings carry obfuscated symbol names but contain the JSON schema and error message text
packages/core/src/utils/yaml-parser.ts (this repo) HEAD of lazzy/gifted-hamilton-684741 the parser being replaced
live node -e probes against yaml@2.8.1 in this tree 2026-06-08 empirical security behavior — anchors, merge keys, !!js/function, billion-laughs, maxAliasCount (results inline in Phase 4)

Confidence labels: C confirmed by direct evidence; I inferred from multiple confirmed facts; O open question.

Phase 1 — Which YAML library does CC use?

Answer: yaml (eemeli/yaml), NOT js-yaml. Confirmed by reading ~/code/claude-code/src/utils/yaml.ts verbatim:

export function parseYaml(input: string): unknown {
  if (typeof Bun !== 'undefined') {
    return Bun.YAML.parse(input);
  }
  // eslint-disable-next-line @typescript-eslint/no-require-imports
  return (require('yaml') as typeof import('yaml')).parse(input);
}
  • Library: yaml npm package. C
  • API: top-level .parse(input). Uses the package's default schema (which is YAML 1.2 core — JSON-superset, no JS extensions). C
  • Bun shortcut: when running under Bun, CC uses Bun.YAML.parse() to avoid bundling ~270 KB of YAML parser. C Not relevant to qwen-code (we don't target Bun runtime).
  • Schema mode: NOT explicitly set anywhere in CC. Relies on yaml package's default behavior, plus zod validation at the consumer layer (DL7, gS8, TKO/_u per docs/declarative-agents-port.md). C

Why yaml rather than js-yaml

Dimension js-yaml 4.x yaml (eemeli) 2.x
Default schema DEFAULT_SAFE_SCHEMA (since 4.x) — safe; older versions had DEFAULT_FULL_SCHEMA with JS core (YAML 1.2 spec) — JSON types only
!!js/function tag NOT supported in 4.x (was in 3.x) Never supported
Billion-laughs guard None (manual responsibility) Built-in maxAliasCount: 100 default
Merge keys (<<) Supported (must opt-out via MERGE_SCHEMA or filtering) Disabled by default, opt-in via { merge: true }
Already a qwen-code dep? js-yaml@4.1.1 yaml@2.8.1 ✓ (already imported by skill-manager)

Both are reasonable choices in 2026, but the original task brief recommended js-yaml's FAILSAFE_SCHEMA / CORE_SCHEMA. We are deviating from that guidance for three concrete reasons:

  1. CC parity. The whole point of porting CC's frontmatter schema is to let users drop a CC agent file into .qwen/agents/ and have it parse identically. Using the same parser CC uses minimizes drift on edge-case YAML constructs (multi-doc streams, flow vs block scalars, tag handling).
  2. yaml is already a direct user inside skill-manager.ts — see packages/core/src/skills/skill-manager.ts:13 (import * as yaml from 'yaml'). Standardizing on yaml eliminates one of two duplicate YAML stacks in the same package. C (grep result documented in Phase 6).
  3. Safer defaults than js-yaml. yaml's built-in maxAliasCount blocks billion-laughs without manual configuration; merge keys are disabled by default; arbitrary tags become literal strings with a YAMLWarning rather than triggering callable resolvers. Empirical evidence in Phase 4.

If a future maintainer wants to drop the yaml dependency and unify on js-yaml, the migration is mechanical: replace yaml.parse / yaml.stringify with jsYaml.load(s, { schema: jsYaml.CORE_SCHEMA }) / jsYaml.dump. The two libraries agree on output for the 100% subset that CC and qwen-code actually use (key-value pairs, lists, nested maps, scalar booleans/numbers). Track that decision separately if it comes up.

Phase 2 — Frontmatter parsing pipeline (CC)

~/code/claude-code/src/utils/frontmatterParser.ts is 370 lines. Key findings:

Step Logic Source
Delimiter match Regex /^---\s*\n([\s\S]*?)\n---\s*\n?/ — opens at column 0, body is non-greedy, closing --- must be on its own line frontmatterParser.ts:~123 (line numbers from old snapshot; treat as approximate) C
Pass 1 parse Call parseYaml(body). If success → return parsed object + content remainder. same file, top of try block C
Pass 2 recovery On YAMLException, walk lines, auto-quote values that look like dates/colons/specials, retry parseYaml once. lines ~85121 in old snapshot C (tab → 2 spaces normalisation, ISO-date heuristic, colon-trap)
Failure fallthrough Both passes failed → log via logForDebugging, return { data: {}, content: text }. Agent loads with empty frontmatter. end of function C
Telemetry Wrapped further upstream — tengu_frontmatter_shadow_unknown_key / _mismatch events fire from ug5.agent (Ig5 schema) claude.strings:308120, 309074, 309076 (cross-cited in docs/declarative-agents-port.md Phase 1)

Implication for qwen-code: we do NOT need to clone the 2-pass recovery. qwen-code's subagent-manager.ts already enforces stricter "throw on malformed frontmatter at top level" semantics for its loader (see parseSubagentContent), and the 2-pass recovery is specifically there to forgive old hand-edited CC agent files. Porting a stricter posture is fine; we just need to not crash the whole loader when nested fields are malformed. See Phase 5 for the warn-and-drop posture.

Phase 3 — Nested validation via zod (CC)

The relevant CC validators per docs/declarative-agents-port.md Phase 1 + binary strings cross-check:

mcpServers (CC symbol gS8 / JSON-shadow jL7)

mcpServers: z.union([
  z.string(),                                            // server name reference
  z.record(z.string(), McpServerConfigSchema()),         // inline { name: spec }
])

McpServerConfigSchema() (from claude.strings:124135 ref) is a discriminated union over type:

type Required fields Notes
"stdio" command: string, args?: string[] Plus env?: Record<string,string>, cwd?: string
"sse" url: string Plus headers?: Record<string,string>
"http" url: string Plus headers?, method?
"websocket" url: string qwen-code parity unknown — defer until needed
"sdk" varies Internal CC use; we do NOT need to support
"claudeai-proxy" varies Internal CC use; we do NOT need to support

For qwen-code v1: validate as Record<string, unknown> (lenient DL7-style), and let the downstream merge into Config.getMcpServers() do the shape coercion. qwen-code already has MCPServerConfig class with type discrimination — we reuse that converter instead of duplicating the zod schema. See Phase 4 of the runtime-wiring plan in docs/declarative-agents-port.md.

hooks (CC symbol TKO / _u)

hooks: Partial<Record<HookEvent, HookMatcher[]>>
HookMatcher: { matcher?: string, hooks: HookConfig[] }
HookConfig (discriminated union on `type`):
  - { type: 'command', command: string, timeout?: number, ... }
  - { type: 'prompt',  prompt: string, ... }
  - { type: 'agent',   agent: string, ... }
  - { type: 'http',    url: string, headers?, ... }

The hook-event keys per the strings cross-check are the same set qwen-code already supports: PreToolUse, PostToolUse, UserPromptSubmit, SessionStart, SessionEnd, Stop, SubagentStart, SubagentStop, Notification — plus a few qwen-only events (TodoCreated, TodoCompleted) that CC does not have.

For qwen-code v1: validate as Record<string, unknown> (lenient), then hand off to qwen-code's existing SessionHooksManager validators, which already implement the HookDefinition[] per-event shape (see packages/core/src/hooks/types.ts:207211 per the Phase-1 runtime mapping).

Why both validators are z.unknown() at the Ig5 shadow level

Ig5 is the telemetry shadow schema — it fires tengu_frontmatter_shadow_unknown_key events when a YAML key isn't in the known set, and _mismatch events when a known key has the wrong type. It deliberately uses z.unknown() for mcpServers and hooks because Ig5 runs at PARSE time and would emit spurious mismatch events for every inline mcpServers spec. The real validation is delegated to:

  • gS8 (for mcpServers) — called at agent registration time from DL7 per-item safeParse
  • TKO (for hooks) — called at hook firing time from _u().safeParse

This lazy validation is the model qwen-code should mimic: keep the frontmatter parser permissive (z.unknown() equivalent in TS), validate at the point of use. Trying to bring the full zod tree forward into SubagentConfig would force us to also import qwen's MCPServerConfig class and HookDefinition type into a layer where they don't currently live, and would require us to invent fake validators for type: 'sdk' / type: 'claudeai-proxy' which we don't actually support.

Phase 4 — Security posture

Empirical verification of yaml@2.8.1 defaults in this qwen-code tree:

Probe results

$ node -e "const y=require('yaml'); console.log(y.parse('a: 1').constructor.name, y.parseDocument('a: 1').schema?.name)"
Object core

→ default schema is 'core' (YAML 1.2 JSON-superset). C

$ node -e "const y=require('yaml'); console.log(y.parse('!!js/function \"function(){}\"'))"
function(){}
(node:18525) [TAG_RESOLVE_FAILED] YAMLWarning: Unresolved tag: tag:yaml.org,2002:js/function

!!js/function tag does NOT execute. The value resolves to the literal string "function(){}" (not a callable function object), and emits a non-fatal YAMLWarning. Adversary cannot achieve RCE via this vector. C

$ node -e "const y=require('yaml'); const bomb = 'a: &a [hi,hi]\nb: &b [*a,*a,*a,*a,*a,*a,*a,*a,*a,*a]\nc: &c [*b,*b,*b,*b,*b,*b,*b,*b,*b,*b]\nd: [*c,*c,*c,*c,*c,*c,*c,*c,*c,*c]'; try { y.parse(bomb) } catch(e){ console.log('REJECTED:', e.message) }"
REJECTED: Excessive alias count indicates a resource exhaustion attack

→ alias-expansion / billion-laughs is REJECTED by default. The library ships with maxAliasCount: 100 (the failed parse counts 1+10+100 = 111 aliases). C

$ node -e "const y=require('yaml'); console.log(JSON.stringify(y.parse('defaults: &d\n  a: 1\nfoo:\n  <<: *d\n  b: 2')))"
{"defaults":{"a":1},"foo":{"<<":{"a":1},"b":2}}

→ merge key (<<) is parsed as a literal key string by default, NOT expanded. The << parser is opt-in via { merge: true }. We will NOT enable it. C

$ node -e "const y=require('yaml'); const yml='mcpServers:\n  filesystem:\n    type: stdio\n    command: node\n    args:\n      - /path/to/server.js'; console.log(JSON.stringify(y.parse(yml), null, 2))"
{
  "mcpServers": {
    "filesystem": { "type": "stdio", "command": "node", "args": ["/path/to/server.js"] }
  }
}

→ CC-shape nested mcpServers parses correctly into deeply-nested object/array. C

Safety summary

Vector yaml@2.8.1 default Action needed in qwen-code
Arbitrary JS execution Impossible — no eval None
!!js/function tag Becomes literal string + warning None
Billion laughs Rejected (maxAliasCount: 100) None — keep default
Merge keys (<<) Treated as literal key None — keep default (do NOT pass merge: true)
Anchors / aliases (normal use) Allowed, useful for CC-shape data None
Arbitrary unknown tags String + YAMLWarning Optionally redirect warnings to a logger (see Phase 6)

Conclusion: yaml package's stock behavior is already safer than what the original task brief asked for via js-yaml's FAILSAFE_SCHEMA. No schema lockdown call is required.

Phase 5 — Recovery semantics

CC chooses graceful warn-and-drop at every layer:

  1. YAML parser throws → frontmatter parser logs + returns {} (empty data)
  2. Field has wrong shape (e.g., mcpServers: "this is a string") → safeParse fails → field is dropped from the emitted config
  3. Field has nearly wrong shape (e.g., individual mcpServers item is a string when the schema wants an object) → per-item safeParse drops just that item, keeps the rest

qwen-code already implements the per-field warn-and-drop posture for permissionMode, maxTurns, color, effort (see packages/core/src/subagents/agent-frontmatter-schema.ts). We extend the same pattern to mcpServers and hooks.

What we DO NOT clone from CC:

  • 2-pass YAML recovery with auto-quoting. This is dead weight for qwen-code — we're a new project, no legacy hand-edited frontmatter files to forgive. A clean error is more useful than a guessed reinterpretation.
  • tengu_* telemetry events. Replaced by qwen-code's own logger / whatever telemetry layer the rest of the loader uses.

Phase 6 — Recommendation for qwen-code

Library choice

  • Use yaml@^2.8.1 (already a transitive — promote to a direct packages/core/package.json dep so we don't break under stricter resolution modes; also lets us pin the major).
  • Use default schema (core), no schema flag.
  • Do not pass { merge: true }. Do not enable any non-default option.
  • For deterministic stringify output (test snapshots), pass { lineWidth: 0, defaultStringType: 'PLAIN' } to yaml.stringify so the library doesn't wrap long lines or arbitrarily switch to block-scalar quoting based on content length.

API surface to preserve

Current packages/core/src/utils/yaml-parser.ts exports:

export function parse(yamlString: string): Record<string, unknown>;
export function stringify(
  obj: Record<string, unknown>,
  options?: { lineWidth?: number; minContentWidth?: number },
): string;

The replacement keeps both signatures identical so the 5 callers (subagent-manager.ts, claude-converter.ts, rulesDiscovery.ts, skill-manager.ts, skill-load.ts) and the index.ts re-export require zero call-site changes.

Implementation sketch:

import * as yaml from 'yaml';

export function parse(yamlString: string): Record<string, unknown> {
  const parsed = yaml.parse(yamlString);
  if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) {
    return parsed as Record<string, unknown>;
  }
  return {};
}

export function stringify(
  obj: Record<string, unknown>,
  options?: { lineWidth?: number; minContentWidth?: number },
): string {
  return yaml.stringify(obj, {
    lineWidth: options?.lineWidth ?? 0,
    minContentWidth: options?.minContentWidth ?? 20,
  });
}

Why coerce non-object top-levels to {}: every existing caller assumes a record. A YAML file that parses to null (empty file), ["foo"] (a list), or "hello" (a bare scalar) would currently crash downstream destructuring. Returning {} preserves the old hand-rolled parser's behavior on the same inputs. Document this as a deliberate guardrail in a one-line comment.

Callers that need no changes

File Usage Compatible?
packages/core/src/index.ts:360 re-exports * from yaml-parser yes — same names
packages/core/src/subagents/subagent-manager.ts:15 parse, stringify yes
packages/core/src/extension/claude-converter.ts:26 parse, stringify yes — round-trip is now safe for mcpServers + hooks (see Phase 3)
packages/core/src/utils/rulesDiscovery.ts:20 parse as parseYaml yes
packages/core/src/skills/skill-manager.ts:13 parse as parseYaml (and import * as yaml from 'yaml' separately) yes — and the duplicate import * as yaml can be removed in a follow-up
packages/core/src/skills/skill-load.ts:11 parse as parseYaml yes

Test fixtures needed

Three concrete YAML snippets that the current hand-rolled parser fails on and the replacement must handle (one per nested shape):

# Fixture 1 — mcpServers (record of records)
mcpServers:
  filesystem:
    type: stdio
    command: node
    args:
      - /path/to/server.js
    env:
      DEBUG: '1'
  github:
    type: http
    url: https://mcp.example.com/github
    headers:
      Authorization: 'Bearer xxx'
# Fixture 2 — hooks (record of arrays of records, two levels of nesting under the event name)
hooks:
  PreToolUse:
    - matcher: 'Read|Write'
      hooks:
        - type: command
          command: echo before
          timeout: 5000
  PostToolUse:
    - matcher: '*'
      hooks:
        - type: command
          command: echo after
# Fixture 3 — mixed shallow + deep, plus everything PR #4842 already supports
name: agent-x
description: test
permissionMode: acceptEdits
maxTurns: 5
color: cyan
tools:
  - Read
  - Write
mcpServers:
  filesystem:
    type: stdio
    command: node
hooks:
  PreToolUse:
    - matcher: Bash
      hooks:
        - type: command
          command: log

Tests that must change

packages/core/src/utils/yaml-parser.test.ts has 2 "pin tests" at the bottom (lines 200227) titled known limitations — nested YAML (pin until js-yaml lands). The replacement MUST flip those into positive-form nested-parsing assertions:

it('parses array-of-records', () => {
  const yaml =
    'mcpServers:\n  - filesystem:\n      type: stdio\n      command: node';
  expect(parse(yaml)).toEqual({
    mcpServers: [{ filesystem: { type: 'stdio', command: 'node' } }],
  });
});

it('parses record-of-records', () => {
  const yaml = 'hooks:\n  PreToolUse:\n    - matcher: Read';
  expect(parse(yaml)).toEqual({
    hooks: { PreToolUse: [{ matcher: 'Read' }] },
  });
});

These two assertions plus the three fixtures above are the acceptance gate for Phase 2 of the implementation plan. Anything else (escaping edge cases, quoted-vs-unquoted booleans, numeric strings) is regression coverage from the existing test suite and should pass unchanged.

Round-trip parity check

Existing test should maintain round-trip integrity for escaped strings (line 111-129) exercises 7 strings through stringify → parse. yaml's default stringify produces slightly different output than the hand-rolled formatter (more aggressive quoting in some cases, different escape sequences). Two acceptable outcomes:

  1. Adjust the test fixtures to assert behavior under the new parser — the round-trip property (parse(stringify(x)) === x) is what matters, not byte-identical YAML output.
  2. Leave the bytewise-identical assertions and let them fail visibly, then update them to reflect yaml's output verbatim. Easier to review diff.

Recommendation: option 1 — change the assertions to property-based (expect(parse(stringify(obj))).toEqual(obj)) since byte-identical YAML output is not a documented contract of the module.

Breaking changes for callers — none expected, but verify

  • subagent-manager.ts re-serializes the parsed object back to YAML for the saveSubagent path. With the new parser, mcpServers and hooks will round-trip cleanly. Update NESTED_FIELDS_NOT_ROUND_TRIPPABLE in claude-converter.ts (Phase 3 of the implementation) to drop these two field names.
  • skill-manager.ts already imports yaml directly (separate from the hand-rolled parser). Once yaml-parser.ts is also using yaml, the duplicate import is removable as a tiny follow-up — out of scope here.

Migration risk

Low. The 5 callers all destructure a Record<string, unknown> — same return type. The 2 deliberate "garbles" pin tests are the only failures expected; they're known and we flip them on purpose. Wider regression coverage comes from the existing test suites in packages/core/src/subagents/, packages/core/src/skills/, and packages/core/src/extension/.

Open questions

# Question Blocking? Resolution path
Q1 Does yaml.parse need an explicit logger to redirect YAMLWarning (e.g., Unresolved tag) to qwen-code's logger instead of process.emitWarning? No — defer If logs get noisy in CI, plumb { logLevel: 'silent' } or a custom onWarning callback. Not load-bearing for v1.
Q2 Should parse() continue to return {} for empty-string / null-document YAML, or throw? No — preserve current behavior Current hand-rolled returns {}; we keep that. Add a regression test pinning the choice.
Q3 When mcpServers is malformed at the top level (e.g., mcpServers: "string"), should the whole agent fail to load, or load with that field dropped? Yes — drives the warn-and-drop posture in Phase 3 of the implementation Resolution: drop the field, emit a console warning (parity with CC DL7 per Phase 3 of docs/declarative-agents-port.md).
Q4 Same as Q3 but for hooks: drop the field, the event, or just the individual matcher? Yes — drives the warn-and-drop posture Resolution: drop the whole hooks field on top-level shape failure. Per-event / per-matcher granularity is deferred to a future PR if a real user surfaces a need.
Q5 Does the Bun.YAML.parse shortcut from CC's helper apply to qwen-code? No qwen-code does not target Bun runtime. Skip.

Status: research complete, ready to implement Phase 2 (replace yaml-parser.ts) and Phase 3 (re-surface mcpServers + hooks on SubagentConfig) per docs/declarative-agents-port.md.