vrr/qwen-code
2
0
Fork 0
mirror of https://github.com/QwenLM/qwen-code.git synced 2026-08-31 02:06:21 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 15
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

488 lines
27 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# 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`](./declarative-agents-port.md).
Issue: [#4821](https://github.com/QwenLM/qwen-code/issues/4821). Prereq for
the follow-up to [PR #4842](https://github.com/QwenLM/qwen-code/pull/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`](https://www.npmjs.com/package/yaml) (eemeli/yaml), NOT
`js-yaml`.** Confirmed by reading `~/code/claude-code/src/utils/yaml.ts`
verbatim:
```ts
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 ~85–121 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:124–135` 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:207–211` 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:
```ts
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:
```ts
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):
```yaml
# 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'
```
```yaml
# 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
```
```yaml
# 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 200–227) titled `known limitations — nested YAML (pin until
js-yaml lands)`. The replacement MUST flip those into positive-form
nested-parsing assertions:
```ts
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`.
Reference in a new issue View git blame Copy permalink