diff --git a/docs/declarative-agents-port.md b/docs/declarative-agents-port.md index db84291bf04..ed357f06eb9 100644 --- a/docs/declarative-agents-port.md +++ b/docs/declarative-agents-port.md @@ -8,7 +8,36 @@ coordinating with the workflow port in issue [#4721][i4721] / PR [#4732][p4732]. [i4721]: https://github.com/QwenLM/qwen-code/issues/4721 [p4732]: https://github.com/QwenLM/qwen-code/pull/4732 -**Implementation status:** PR [#4842](https://github.com/QwenLM/qwen-code/pull/4842) ships `permissionMode`, `maxTurns`, and a tightened `color` allowlist. The other fields documented below are reference material for follow-up PRs once their prerequisite infra exists (`effort` → model-layer param; `mcpServers`/`hooks` → nested YAML parser; `memory` → scoped memory subsystem; `isolation` → workflow PR #4732; `initialPrompt` → `--agent` flag; `skills` → SkillManager wiring). +## Implementation status (vertical-sliced) + +PR [#4842][p4842] shipped the fields with an end-to-end runtime path at the +time. PR [#4870][p4870] then replaced the YAML parser to support block +scalars. This follow-up PR builds on both: it replaces the YAML +**stringifier** (PR #4870 left it hand-rolled — see +`docs/yaml-parser-replacement.md`), surfaces `mcpServers` + `hooks` on +`SubagentConfig`, and wires them to the runtime so per-agent MCP servers +and hooks actually fire when a subagent runs. + +| Field | Status | Notes | +| ----------------- | ----------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `permissionMode` | **shipped (#4842)** | bridges to existing qwen `approvalMode` at parse time | +| `maxTurns` | **shipped (#4842)** | wired into existing `runConfig.max_turns` runtime path | +| `color` allowlist | **shipped (#4842)** | tightens existing field to CC's `_Y` set + `auto` legacy sentinel handling | +| `mcpServers` | **shipped (follow-up)** | nested YAML round-trip safe via eemeli/`yaml` stringify; runtime override merges session + agent servers via subagent Config wrapper + forced tool-registry rebuild | +| `hooks` | **shipped (follow-up)** | ephemeral HookRegistry entries registered at subagent spawn, removed via `onStop`; v1 fires globally (no agent-scope filter) | +| `effort` | deferred | no model-layer `effort` parameter exists yet in qwen providers | +| `memory` | deferred | qwen's auto-memory has no `user`/`project`/`local` scope distinction yet | +| `isolation` | deferred | workflow PR #4732 owns the runtime; per-agent default lands when that lands | +| `initialPrompt` | deferred | requires `--agent` CLI flag (no main-session-agent infra in qwen) | +| `skills` | deferred | requires SkillManager consumption of `config.skills` | + +The full reverse-engineering record below is retained as the design reference +for the deferred fields — schema constants, DL7/Ig5 semantics, error +messages, and the coordination matrix with workflow are still load-bearing +for that work. + +[p4842]: https://github.com/QwenLM/qwen-code/pull/4842 +[p4870]: https://github.com/QwenLM/qwen-code/pull/4870 --- diff --git a/docs/users/features/sub-agents.md b/docs/users/features/sub-agents.md index cdae9c966d3..3f08d3532ac 100644 --- a/docs/users/features/sub-agents.md +++ b/docs/users/features/sub-agents.md @@ -282,13 +282,15 @@ can drop a CC agent file into `.qwen/agents/` and have the supported fields parse identically. Optional fields with invalid values are silently dropped at parse time rather than rejected — the same lenient posture CC uses. -| Field | Type | Notes | -| ---------------- | ---------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `permissionMode` | enum string | `acceptEdits`, `auto`, `bypassPermissions`, `default`, `dontAsk`, `plan`. Mapped to `approvalMode` at parse time; when both are set, the explicit `approvalMode` wins. | -| `maxTurns` | positive integer | Caps the agent's turn budget. Wired into `runConfig.max_turns` at runtime; when both are set, the top-level field wins. | -| `color` | enum string | Display color. Allowlist: `red`, `blue`, `green`, `yellow`, `purple`, `orange`, `pink`, `cyan` (mirrors CC's `_Y`). The legacy qwen sentinel `auto` is also preserved for backward compatibility. Other values are silently dropped on parse. | +| Field | Type | Notes | +| ---------------- | ---------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `permissionMode` | enum string | `acceptEdits`, `auto`, `bypassPermissions`, `default`, `dontAsk`, `plan`. Mapped to `approvalMode` at parse time; when both are set, the explicit `approvalMode` wins. | +| `maxTurns` | positive integer | Caps the agent's turn budget. Wired into `runConfig.max_turns` at runtime; when both are set, the top-level field wins. The legacy nested value is pruned from the on-disk file on save to avoid two sources of truth. | +| `color` | enum string | Display color. Allowlist: `red`, `blue`, `green`, `yellow`, `purple`, `orange`, `pink`, `cyan` (mirrors CC's `_Y`). The legacy qwen sentinel `auto` is preserved for backward compatibility. Other values are silently dropped on parse. | +| `mcpServers` | record of specs | Per-agent MCP server overrides. Merged with the session-level MCP server set when the agent spawns; on key collision the agent's spec wins (matching CC's `scope: 'agent'` semantics). Malformed entries are dropped per-key with a warning rather than failing the whole agent. | +| `hooks` | record of arrays | Per-agent hooks. Keys are CC hook event names (`PreToolUse`, `PostToolUse`, `UserPromptSubmit`, …); values are arrays of `{ matcher?, hooks: [...] }` definitions in the same shape as `settings.json`'s `hooks` field. Registered while the agent runs, removed when it stops. | -Example: +Example with all of the above: ``` --- @@ -301,6 +303,17 @@ tools: - read_file - grep_search - glob +mcpServers: + filesystem: + type: stdio + command: node + args: [/usr/local/lib/mcp-fs/server.js] +hooks: + PreToolUse: + - matcher: Bash + hooks: + - type: command + command: echo "review-agent about to run a shell command" --- You are a code reviewer. Analyze the code thoroughly and report findings @@ -308,11 +321,19 @@ ordered by severity. ``` The remaining CC frontmatter fields — `effort`, `skills`, `initialPrompt`, -`memory`, `isolation`, `mcpServers`, `hooks` — are documented in the -declarative-agent design doc and land in follow-up PRs once the prerequisite -infrastructure exists (`effort` needs a model-layer parameter; `memory` -needs a scoped memory subsystem; `mcpServers` / `hooks` need a nested-aware -YAML parser; `--agent` CLI flag enables `initialPrompt`; etc.). +`memory`, `isolation` — are documented in the declarative-agent design doc +and land in follow-up PRs once the prerequisite infrastructure exists +(`effort` needs a model-layer parameter; `memory` needs a scoped memory +subsystem; `--agent` CLI flag enables `initialPrompt`; etc.). + +> **`hooks` v1 limitation.** While a subagent declaring `hooks` is running, +> its hook entries fire for every matching event in the session, not only +> for that subagent's own tool calls. If two subagents with different +> per-agent hook sets run concurrently, both sets fire for both agents. +> Per-agent scope filtering at hook-firing time is left to a follow-up; +> for v1, prefer per-agent hooks that are safe to fire globally for the +> duration of the agent's run (e.g. logging) over hooks that mutate +> behavior. #### Example Usage diff --git a/docs/yaml-parser-replacement.md b/docs/yaml-parser-replacement.md new file mode 100644 index 00000000000..472c61d5e5b --- /dev/null +++ b/docs/yaml-parser-replacement.md @@ -0,0 +1,488 @@ +# 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`, `cwd?: string` | +| `"sse"` | `url: string` | Plus `headers?: Record` | +| `"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` (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> +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` (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; +export function stringify( + obj: Record, + 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 { + const parsed = yaml.parse(yamlString); + if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { + return parsed as Record; + } + return {}; +} + +export function stringify( + obj: Record, + 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` — 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`. diff --git a/packages/core/src/agents/background-agent-resume.test.ts b/packages/core/src/agents/background-agent-resume.test.ts index 934ced3b5f7..e2149e5fc99 100644 --- a/packages/core/src/agents/background-agent-resume.test.ts +++ b/packages/core/src/agents/background-agent-resume.test.ts @@ -430,7 +430,10 @@ describe('BackgroundAgentResumeService', () => { }; const { service, subagentManager, hookSystem } = createService(); - subagentManager.createAgentHeadless.mockResolvedValue(subagent); + subagentManager.createAgentHeadless.mockResolvedValue({ + subagent, + dispose: vi.fn().mockResolvedValue(undefined), + }); hookSystem.fireSubagentStartEvent.mockResolvedValue({ getAdditionalContext: () => 'resume-context', }); @@ -518,7 +521,10 @@ describe('BackgroundAgentResumeService', () => { }; const { service, subagentManager } = createService(); - subagentManager.createAgentHeadless.mockResolvedValue(subagent); + subagentManager.createAgentHeadless.mockResolvedValue({ + subagent, + dispose: vi.fn().mockResolvedValue(undefined), + }); const resumed = await service.resumeBackgroundAgent(agentId, 'continue'); @@ -653,7 +659,10 @@ describe('BackgroundAgentResumeService', () => { }; const { service, subagentManager, hookSystem } = createService(); - subagentManager.createAgentHeadless.mockResolvedValue(subagent); + subagentManager.createAgentHeadless.mockResolvedValue({ + subagent, + dispose: vi.fn().mockResolvedValue(undefined), + }); const resumed = await service.resumeBackgroundAgent(agentId, 'continue'); @@ -734,7 +743,10 @@ describe('BackgroundAgentResumeService', () => { const { service, subagentManager, hookSystem } = createService({ stopHookBlockingCap: 2, }); - subagentManager.createAgentHeadless.mockResolvedValue(subagent); + subagentManager.createAgentHeadless.mockResolvedValue({ + subagent, + dispose: vi.fn().mockResolvedValue(undefined), + }); hookSystem.fireSubagentStopEvent.mockResolvedValue(stopOutput); const resumed = await service.resumeBackgroundAgent(agentId, 'continue'); @@ -801,15 +813,18 @@ describe('BackgroundAgentResumeService', () => { }); const createAgentHeadless = vi.fn().mockResolvedValue({ - execute: vi.fn(async () => undefined), - setExternalMessageProvider: vi.fn(), - getCore: () => ({ getEventEmitter: () => new AgentEventEmitter() }), - getExecutionSummary: () => ({ - totalTokens: 0, - totalDurationMs: 0, - }), - getTerminateMode: () => AgentTerminateMode.GOAL, - getFinalText: () => 'done', + subagent: { + execute: vi.fn(async () => undefined), + setExternalMessageProvider: vi.fn(), + getCore: () => ({ getEventEmitter: () => new AgentEventEmitter() }), + getExecutionSummary: () => ({ + totalTokens: 0, + totalDurationMs: 0, + }), + getTerminateMode: () => AgentTerminateMode.GOAL, + getFinalText: () => 'done', + }, + dispose: vi.fn().mockResolvedValue(undefined), }); const { service, subagentManager } = createService(); @@ -890,7 +905,10 @@ describe('BackgroundAgentResumeService', () => { }; const { service, subagentManager } = createService(); - subagentManager.createAgentHeadless.mockResolvedValue(subagent); + subagentManager.createAgentHeadless.mockResolvedValue({ + subagent, + dispose: vi.fn().mockResolvedValue(undefined), + }); const first = service.resumeBackgroundAgent(agentId, 'first message'); const second = service.resumeBackgroundAgent(agentId, 'second message'); @@ -971,7 +989,10 @@ describe('BackgroundAgentResumeService', () => { getFinalText: () => 'done', }; const { service, subagentManager, monitorRegistry } = createService(); - subagentManager.createAgentHeadless.mockResolvedValue(subagent); + subagentManager.createAgentHeadless.mockResolvedValue({ + subagent, + dispose: vi.fn().mockResolvedValue(undefined), + }); const resume = service.resumeBackgroundAgent(agentId, 'continue'); await vi.waitFor(() => { @@ -1080,7 +1101,10 @@ describe('BackgroundAgentResumeService', () => { getFinalText: () => 'done', }; const { service, subagentManager, monitorRegistry } = createService(); - subagentManager.createAgentHeadless.mockResolvedValue(subagent); + subagentManager.createAgentHeadless.mockResolvedValue({ + subagent, + dispose: vi.fn().mockResolvedValue(undefined), + }); await expect( service.resumeBackgroundAgent(agentId, 'continue'), @@ -1484,7 +1508,10 @@ describe('BackgroundAgentResumeService', () => { }; const { service, subagentManager } = createService(); - subagentManager.createAgentHeadless.mockResolvedValue(subagent); + subagentManager.createAgentHeadless.mockResolvedValue({ + subagent, + dispose: vi.fn().mockResolvedValue(undefined), + }); const resumed = await service.resumeBackgroundAgent(agentId, 'continue'); expect(resumed).toBeDefined(); @@ -1559,7 +1586,10 @@ describe('BackgroundAgentResumeService', () => { }; const { service, subagentManager } = createService(); - subagentManager.createAgentHeadless.mockResolvedValue(subagent); + subagentManager.createAgentHeadless.mockResolvedValue({ + subagent, + dispose: vi.fn().mockResolvedValue(undefined), + }); const resumed = await service.resumeBackgroundAgent(agentId, 'continue'); expect(resumed).toBeDefined(); @@ -1654,7 +1684,10 @@ describe('BackgroundAgentResumeService', () => { }; const { service, subagentManager } = createService(); - subagentManager.createAgentHeadless.mockResolvedValue(subagent); + subagentManager.createAgentHeadless.mockResolvedValue({ + subagent, + dispose: vi.fn().mockResolvedValue(undefined), + }); await service.resumeBackgroundAgent(agentId, 'continue work'); diff --git a/packages/core/src/agents/background-agent-resume.ts b/packages/core/src/agents/background-agent-resume.ts index a0af85727d5..5ece8730bfb 100644 --- a/packages/core/src/agents/background-agent-resume.ts +++ b/packages/core/src/agents/background-agent-resume.ts @@ -618,21 +618,32 @@ export class BackgroundAgentResumeService { } const bgEventEmitter = new AgentEventEmitter(); - const subagent = target.isFork - ? await this.createResumedForkSubagent( - bgConfig as Config, - bgEventEmitter, - resumeHistory ?? [], - recovery.forkBootstrap!, - ) - : await this.config - .getSubagentManager() - .createAgentHeadless(target.subagentConfig!, bgConfig as Config, { - eventEmitter: bgEventEmitter, - promptConfigOverrides: { - initialMessages: resumeHistory, - }, - }); + // Per-spawn cleanup from `SubagentManager.createAgentHeadless` — + // the resume `finally` invokes this so per-agent hook entries and + // the force-rebuilt ToolRegistry don't leak across the resume + // boundary. Stays undefined on the fork-resume path (forks share + // the parent's registry + hook lifecycle). + let subagentDispose: (() => Promise) | undefined; + let subagent: AgentHeadless; + if (target.isFork) { + subagent = await this.createResumedForkSubagent( + bgConfig as Config, + bgEventEmitter, + resumeHistory ?? [], + recovery.forkBootstrap!, + ); + } else { + const result = await this.config + .getSubagentManager() + .createAgentHeadless(target.subagentConfig!, bgConfig as Config, { + eventEmitter: bgEventEmitter, + promptConfigOverrides: { + initialMessages: resumeHistory, + }, + }); + subagent = result.subagent; + subagentDispose = result.dispose; + } const projectRoot = this.config.getProjectRoot(); cleanupJsonl = attachJsonlTranscriptWriter(bgEventEmitter, outputFile, { @@ -840,6 +851,11 @@ export class BackgroundAgentResumeService { .getToolRegistry() .stop() .catch(() => {}); + // Per-spawn cleanup from `createAgentHeadless`: releases agent- + // scope hook entries and stops the per-agent ToolRegistry that + // the force rebuild created for `mcpServers`. Distinct from the + // parent registry above (no-op when target.isFork). + void subagentDispose?.().catch(() => {}); // Restore parent PermissionManager's dangerous allow rules if // this override stripped them. See createApprovalModeOverride // strip-lifecycle comment in agent.ts. diff --git a/packages/core/src/extension/claude-converter.ts b/packages/core/src/extension/claude-converter.ts index 0d298651335..a83a72f1b47 100644 --- a/packages/core/src/extension/claude-converter.ts +++ b/packages/core/src/extension/claude-converter.ts @@ -67,8 +67,10 @@ export interface ClaudeAgentConfig { permissionMode?: string; /** Skills to load into the subagent's context at startup */ skills?: string[]; - /** Hooks configuration */ + /** Hooks configuration (CC `TKO` shape; nested per HookEventName) */ hooks?: unknown; + /** Per-agent MCP server overrides (CC `gS8` shape; record of server-name → spec) */ + mcpServers?: unknown; /** System prompt content */ systemPrompt?: string; /** subagent color */ @@ -209,6 +211,9 @@ export function convertClaudeAgentConfig( if (claudeAgent.hooks) { qwenAgent['hooks'] = claudeAgent.hooks; } + if (claudeAgent.mcpServers) { + qwenAgent['mcpServers'] = claudeAgent.mcpServers; + } if (claudeAgent.skills && claudeAgent.skills.length > 0) { qwenAgent['skills'] = claudeAgent.skills; } @@ -262,7 +267,10 @@ async function convertAgentFiles(agentsDir: string): Promise { model: frontmatter['model'] as string | undefined, permissionMode: frontmatter['permissionMode'] as string | undefined, skills: parseStringOrArray(frontmatter['skills']), - hooks: frontmatter['hooks'], + hooks: frontmatter['hooks'] as ClaudeAgentConfig['hooks'], + mcpServers: frontmatter[ + 'mcpServers' + ] as ClaudeAgentConfig['mcpServers'], color: frontmatter['color'] as string | undefined, systemPrompt: body.trim(), }; @@ -270,7 +278,7 @@ async function convertAgentFiles(agentsDir: string): Promise { // Convert to Qwen format const qwenAgent = convertClaudeAgentConfig(claudeAgent); - // Build new frontmatter (excluding systemPrompt as it goes in body) + // Build new frontmatter (excluding systemPrompt as it goes in body). const newFrontmatter: Record = {}; for (const [key, value] of Object.entries(qwenAgent)) { if (key !== 'systemPrompt' && value !== undefined) { @@ -278,8 +286,12 @@ async function convertAgentFiles(agentsDir: string): Promise { } } - // Write converted content back - const newYaml = stringifyYaml(newFrontmatter); + // Write converted content back. Trim to drop the trailing newline + // `yaml.stringify` appends so the assembled file has the same single + // blank line between the closing `---` and the body that + // `subagent-manager.ts:serializeSubagent` produces — without `.trim()` + // the converter emits an extra blank line before the closing `---`. + const newYaml = stringifyYaml(newFrontmatter).trim(); const systemPrompt = (qwenAgent['systemPrompt'] as string) || body.trim(); const newContent = `--- ${newYaml} diff --git a/packages/core/src/hooks/hookRegistry.test.ts b/packages/core/src/hooks/hookRegistry.test.ts index 3472a8a5e9a..ac4854dfde9 100644 --- a/packages/core/src/hooks/hookRegistry.test.ts +++ b/packages/core/src/hooks/hookRegistry.test.ts @@ -874,6 +874,117 @@ describe('HookRegistry', () => { }); }); + describe('addAgentHooks — per-agent frontmatter ephemeral entries', () => { + it('appends entries tagged with agentScope and returns an unregister callback', async () => { + const registry = new HookRegistry(mockConfig); + await registry.initialize(); + expect(registry.getAllHooks()).toHaveLength(0); + + const unregister = registry.addAgentHooks( + { + [HookEventName.PreToolUse]: [ + { + matcher: 'Bash', + hooks: [ + { + type: HookType.Command, + command: 'echo per-agent', + name: 'agent-hook', + }, + ], + }, + ], + }, + 'agent:test:abc', + ); + + const after = registry.getAllHooks(); + expect(after).toHaveLength(1); + expect(after[0].source).toBe(HooksConfigSource.Session); + expect(after[0].agentScope).toBe('agent:test:abc'); + + unregister(); + expect(registry.getAllHooks()).toHaveLength(0); + }); + + it('coexists with session/user hooks of the same identity', async () => { + const userHooks: Parameters[0] = { + [HookEventName.PreToolUse]: [ + { + matcher: 'Bash', + hooks: [ + { type: HookType.Command, command: 'echo same', name: 'shared' }, + ], + }, + ], + }; + mockConfig.getUserHooks = vi.fn().mockReturnValue(userHooks); + + const registry = new HookRegistry(mockConfig); + await registry.initialize(); + expect(registry.getAllHooks()).toHaveLength(1); + + // Same identity, different source path (Session + agentScope) — must + // NOT be deduped against the user-source entry. + registry.addAgentHooks(userHooks, 'agent:test:def'); + const after = registry.getAllHooks(); + expect(after).toHaveLength(2); + // Assert the scope tag itself participates in the dedup key, not just + // the count. A regression that drops `agentScope` from the dedup + // check would still produce 2 entries by ordering luck — this + // assertion catches that. + expect( + after.some( + (e) => + e.source === HooksConfigSource.User && e.agentScope === undefined, + ), + ).toBe(true); + expect( + after.some( + (e) => + e.source === HooksConfigSource.Session && + e.agentScope === 'agent:test:def', + ), + ).toBe(true); + }); + + it('two concurrent agents each keep their own copy of an identical hook', async () => { + const registry = new HookRegistry(mockConfig); + await registry.initialize(); + + const sameHooks: Parameters[0] = { + [HookEventName.PostToolUse]: [ + { + hooks: [ + { type: HookType.Command, command: 'echo done', name: 'h' }, + ], + }, + ], + }; + + const u1 = registry.addAgentHooks(sameHooks, 'agent:a:1'); + const u2 = registry.addAgentHooks(sameHooks, 'agent:b:2'); + + expect(registry.getAllHooks()).toHaveLength(2); + u1(); + const remaining = registry.getAllHooks(); + expect(remaining).toHaveLength(1); + expect(remaining[0].agentScope).toBe('agent:b:2'); + u2(); + expect(registry.getAllHooks()).toHaveLength(0); + }); + + it('silently keeps entries when the hooks payload is empty', async () => { + const registry = new HookRegistry(mockConfig); + await registry.initialize(); + const unregister = registry.addAgentHooks({}, 'agent:empty:0'); + expect(registry.getAllHooks()).toHaveLength(0); + // No-op unregister should not throw + unregister(); + expect(registry.getAllHooks()).toHaveLength(0); + }); + }); + describe('getAllHooks', () => { it('should return a copy of entries array', async () => { const hooksConfig = { diff --git a/packages/core/src/hooks/hookRegistry.ts b/packages/core/src/hooks/hookRegistry.ts index 44f3af5b099..bb74fda06dc 100644 --- a/packages/core/src/hooks/hookRegistry.ts +++ b/packages/core/src/hooks/hookRegistry.ts @@ -51,6 +51,13 @@ export interface HookRegistryEntry { matcher?: string; sequential?: boolean; enabled: boolean; + /** + * Identifier for ephemeral entries attached at runtime by a specific + * subagent (via {@link HookRegistry.addAgentHooks}). Used by the matching + * unregister callback to remove the entries when the subagent ends. Plain + * (session/user/project/extension) entries leave this undefined. + */ + agentScope?: string; } /** @@ -97,6 +104,51 @@ export class HookRegistry { return [...this.entries]; } + /** + * Append ephemeral hook entries scoped to a specific subagent. Used by + * `SubagentManager` to wire the `hooks` field from a declarative agent + * frontmatter into the live registry when the subagent spawns. + * + * The hooks are validated through the same per-definition pipeline as + * session/user/project hooks (`processHookDefinition`), so a malformed + * entry is logged and dropped instead of breaking the spawn. Returns an + * unregister callback that removes exactly the entries added by this call; + * the caller is responsible for invoking it when the subagent finishes. + * + * v1 scope limitation: entries added here fire for every event of their + * declared type while they remain in the registry, regardless of which + * agent is currently active. If two subagents with different per-agent + * hook sets run concurrently, both sets fire for both agents. Proper + * per-agent scope filtering at firing time is left to a follow-up. + */ + addAgentHooks( + hooks: { [K in HookEventName]?: HookDefinition[] }, + agentScope: string, + ): () => void { + const before = this.entries.length; + this.processHooksConfiguration( + hooks, + HooksConfigSource.Session, + agentScope, + ); + const addedCount = this.entries.length - before; + debugLogger.debug( + `Registered ${addedCount} ephemeral hook entries for agent scope "${agentScope}"`, + ); + return () => { + const sizeBefore = this.entries.length; + this.entries = this.entries.filter( + (entry) => entry.agentScope !== agentScope, + ); + const removed = sizeBefore - this.entries.length; + if (removed > 0) { + debugLogger.debug( + `Removed ${removed} ephemeral hook entries for agent scope "${agentScope}"`, + ); + } + }; + } + /** * Enable or disable a specific hook */ @@ -189,6 +241,7 @@ export class HookRegistry { private processHooksConfiguration( hooksConfig: { [K in HookEventName]?: HookDefinition[] }, source: HooksConfigSource, + agentScope?: string, ): void { for (const [eventName, definitions] of Object.entries(hooksConfig)) { if (HOOKS_CONFIG_FIELDS.includes(eventName)) { @@ -213,7 +266,12 @@ export class HookRegistry { } for (const definition of definitions) { - this.processHookDefinition(definition, typedEventName, source); + this.processHookDefinition( + definition, + typedEventName, + source, + agentScope, + ); } } } @@ -225,6 +283,7 @@ export class HookRegistry { definition: HookDefinition, eventName: HookEventName, source: HooksConfigSource, + agentScope?: string, ): void { if ( !definition || @@ -247,11 +306,15 @@ export class HookRegistry { const hookIdentity = this.getHookIdentity({ config: hookConfig }); const hookName = this.getHookName({ config: hookConfig }); - // Check for duplicate hooks (same identity+source+eventName+matcher+sequential) + // Check for duplicate hooks. `agentScope` participates in the key so + // a per-agent hook does not get swallowed when an identical + // session/user/project hook already exists, and so two concurrent + // subagents declaring the same hook each keep their own copy. const isDuplicate = this.entries.some( (existing) => existing.eventName === eventName && existing.source === source && + existing.agentScope === agentScope && this.getHookIdentity(existing) === hookIdentity && existing.matcher === definition.matcher && existing.sequential === definition.sequential, @@ -275,6 +338,7 @@ export class HookRegistry { matcher: definition.matcher, sequential: definition.sequential, enabled: true, + ...(agentScope !== undefined ? { agentScope } : {}), }); } else { // Invalid hooks are logged and discarded here, they won't reach HookRunner diff --git a/packages/core/src/subagents/agent-frontmatter-schema.test.ts b/packages/core/src/subagents/agent-frontmatter-schema.test.ts index 12c3225b820..80c78ca763c 100644 --- a/packages/core/src/subagents/agent-frontmatter-schema.test.ts +++ b/packages/core/src/subagents/agent-frontmatter-schema.test.ts @@ -9,6 +9,8 @@ import { PERMISSION_MODE_VALUES, COLOR_VALUES, claudePermissionModeToApprovalMode, + parseAgentHooks, + parseAgentMcpServers, parseMaxTurns, isPermissionMode, isColor, @@ -131,4 +133,115 @@ describe('agent-frontmatter-schema', () => { expect(isColor(undefined)).toBe(false); }); }); + + describe('parseAgentMcpServers — CC gS8 shallow validation', () => { + it('keeps a record-of-records as-is', () => { + const input = { + filesystem: { type: 'stdio', command: 'node' }, + github: { type: 'http', url: 'https://example.com' }, + }; + expect(parseAgentMcpServers(input)).toEqual(input); + }); + + it('drops scalar / array entries inside the record', () => { + const input = { + good: { type: 'stdio', command: 'node' }, + scalarBad: 'a-string', + arrayBad: [1, 2, 3], + nullBad: null, + }; + expect(parseAgentMcpServers(input)).toEqual({ + good: { type: 'stdio', command: 'node' }, + }); + }); + + it('returns undefined for non-object top-level', () => { + expect(parseAgentMcpServers(undefined)).toBeUndefined(); + expect(parseAgentMcpServers(null)).toBeUndefined(); + expect(parseAgentMcpServers('a-string')).toBeUndefined(); + expect(parseAgentMcpServers(['arr'])).toBeUndefined(); + expect(parseAgentMcpServers(42)).toBeUndefined(); + }); + + it('returns undefined when no entries survive shape filtering', () => { + const input = { onlyBad: 'string', alsoBad: [1] }; + expect(parseAgentMcpServers(input)).toBeUndefined(); + }); + + it('returns undefined for an empty record', () => { + expect(parseAgentMcpServers({})).toBeUndefined(); + }); + + it('returns a null-prototype object so a literal __proto__ key cannot pollute the prototype chain', () => { + // The repo's yaml-parser wraps parsed objects in `Object.create(null)` + // (see `yaml-parser.ts:stripNullValues`), which lets a literal YAML key + // of `__proto__` survive as an own property instead of triggering the + // `Object.prototype` setter. Reproduce that input shape exactly here — + // an object-literal `{ __proto__: X }` invokes the setter instead of + // defining an own property, which is NOT what `yaml.parse` produces. + const input = Object.create(null) as Record; + input['good'] = { type: 'stdio', command: 'good' }; + input['__proto__'] = { type: 'stdio', command: 'evil' }; + const result = parseAgentMcpServers(input); + expect(result).toBeDefined(); + // A plain `{}` result would now have its prototype set to the evil + // spec; the null-prototype defense keeps the chain clean. + expect(Object.getPrototypeOf(result!)).toBeNull(); + // The polluted key is preserved as an own property so the caller + // sees the attack surface; it just can't reach via prototype walk. + expect(Object.hasOwn(result!, '__proto__')).toBe(true); + // Object.prototype must remain untouched. + expect(({} as Record)['command']).toBeUndefined(); + }); + }); + + describe('parseAgentHooks — CC TKO shallow validation', () => { + it('keeps a record-of-arrays as-is', () => { + const input = { + PreToolUse: [ + { matcher: 'Bash', hooks: [{ type: 'command', command: 'echo' }] }, + ], + PostToolUse: [{ matcher: '*', hooks: [] }], + }; + expect(parseAgentHooks(input)).toEqual(input); + }); + + it('drops non-array values per event', () => { + const input = { + PreToolUse: [{ matcher: 'Bash', hooks: [] }], + BogusEvent: 'not-an-array', + AlsoBad: { not: 'an array' }, + }; + expect(parseAgentHooks(input)).toEqual({ + PreToolUse: [{ matcher: 'Bash', hooks: [] }], + }); + }); + + it('returns undefined for non-object top-level', () => { + expect(parseAgentHooks(undefined)).toBeUndefined(); + expect(parseAgentHooks(null)).toBeUndefined(); + expect(parseAgentHooks('PreToolUse')).toBeUndefined(); + expect(parseAgentHooks(['x'])).toBeUndefined(); + }); + + it('returns undefined when no events survive shape filtering', () => { + const input = { PreToolUse: 'wrong shape' }; + expect(parseAgentHooks(input)).toBeUndefined(); + }); + + it('returns undefined for an empty record', () => { + expect(parseAgentHooks({})).toBeUndefined(); + }); + + it('returns a null-prototype object so a literal __proto__ key cannot pollute the prototype chain', () => { + // See parseAgentMcpServers's __proto__ test for the rationale. + const input = Object.create(null) as Record; + input['PreToolUse'] = [{ matcher: 'Bash', hooks: [] }]; + input['__proto__'] = [{ matcher: 'Evil', hooks: [] }]; + const result = parseAgentHooks(input); + expect(result).toBeDefined(); + expect(Object.getPrototypeOf(result!)).toBeNull(); + expect(Object.hasOwn(result!, '__proto__')).toBe(true); + }); + }); }); diff --git a/packages/core/src/subagents/agent-frontmatter-schema.ts b/packages/core/src/subagents/agent-frontmatter-schema.ts index e3624254495..2db8999ccc1 100644 --- a/packages/core/src/subagents/agent-frontmatter-schema.ts +++ b/packages/core/src/subagents/agent-frontmatter-schema.ts @@ -117,3 +117,72 @@ export function isColor(value: unknown): value is ColorValue { (COLOR_VALUES as readonly string[]).includes(value) ); } + +/** + * Parse a frontmatter `mcpServers` value into the record-of-specs shape + * qwen-code's MCP layer expects. Matches CC `gS8`'s shallow validation: + * + * - non-object / array / null → undefined (whole field dropped) + * - string (CC's server-name reference form) → undefined; qwen-code does + * not support the reference form yet, so it is rejected at this layer + * rather than silently passed through and later confusing the MCP loader + * - record-of-records → keep entries whose value is a plain object, + * drop entries whose value is a scalar / array / null + * + * The deep `{ type, command, args, ... }` validation per spec is intentionally + * deferred to the runtime MCP loader (which already owns the union for + * stdio/sse/http/etc.). This mirrors CC, where Ig5 keeps mcpServers as + * `z.unknown()` at parse time and gS8 / DL7 run per-item `safeParse` at + * registration time. Drop-the-whole-field is preferred over throw so a + * malformed mcpServers block doesn't kill the entire agent. + */ +export function parseAgentMcpServers( + value: unknown, +): Record | undefined { + if (value === undefined || value === null) return undefined; + if (typeof value !== 'object' || Array.isArray(value)) return undefined; + const record = value as Record; + // `Object.create(null)` so a YAML key of literal `__proto__` lands as a + // plain own property (instead of triggering the `__proto__` setter on + // `Object.prototype` and silently mutating the result's prototype chain). + // Matches the null-prototype guarantee `yaml-parser.ts:stripNullValues` + // already enforces on the input record we receive from yaml.parse. + const result = Object.create(null) as Record; + for (const [name, spec] of Object.entries(record)) { + if (spec !== null && typeof spec === 'object' && !Array.isArray(spec)) { + result[name] = spec; + } + } + return Object.keys(result).length > 0 ? result : undefined; +} + +/** + * Parse a frontmatter `hooks` value into the record-of-event-matchers shape + * qwen-code's hook layer expects. Matches CC `TKO` / `_u`'s shallow + * validation: + * + * - non-object / array / null → undefined (whole field dropped) + * - record → keep entries whose value is an array, drop entries whose + * value is a non-array (a scalar / object / null is never a valid + * HookMatcher list) + * + * Per-matcher / per-hook `{ type, command, ... }` validation is deferred to + * the runtime hook subsystem (`SessionHooksManager` already owns the discriminated + * union for command/http/function/prompt). Drop-the-whole-field is preferred + * over throw, matching the rest of the DL7 lenient posture. + */ +export function parseAgentHooks( + value: unknown, +): Record | undefined { + if (value === undefined || value === null) return undefined; + if (typeof value !== 'object' || Array.isArray(value)) return undefined; + const record = value as Record; + // See `parseAgentMcpServers` for why this uses a null-prototype object. + const result = Object.create(null) as Record; + for (const [eventName, matchers] of Object.entries(record)) { + if (Array.isArray(matchers)) { + result[eventName] = matchers; + } + } + return Object.keys(result).length > 0 ? result : undefined; +} diff --git a/packages/core/src/subagents/subagent-manager-override.test.ts b/packages/core/src/subagents/subagent-manager-override.test.ts index 72413350c05..f06e3873b88 100644 --- a/packages/core/src/subagents/subagent-manager-override.test.ts +++ b/packages/core/src/subagents/subagent-manager-override.test.ts @@ -7,6 +7,7 @@ import { describe, it, expect } from 'vitest'; import { Config, ApprovalMode } from '../config/config.js'; import { SubagentManager } from './subagent-manager.js'; +import type { SubagentConfig } from './types.js'; import { ToolNames } from '../tools/tool-names.js'; import { EditTool } from '../tools/edit.js'; import { ReadFileTool } from '../tools/read-file.js'; @@ -39,16 +40,31 @@ describe('SubagentManager.buildSubagentContextOverride bound-tool isolation', () // The method is `private`. Cast via `unknown` to invoke it directly — // testing through the public `createAgentHeadless` pathway would also // work but pulls in a much larger graph (file IO, hooks, etc.). - function callBuildOverride( + async function callBuildOverride( manager: SubagentManager, base: Config, + config?: Partial, ): Promise { const fn = ( manager as unknown as { - buildSubagentContextOverride: (b: Config) => Promise; + buildSubagentContextOverride: ( + b: Config, + c: SubagentConfig, + ) => Promise<{ + context: Config; + cleanup?: () => Promise; + }>; } ).buildSubagentContextOverride.bind(manager); - return fn(base); + const fullConfig: SubagentConfig = { + name: 'test-agent', + description: 'test', + systemPrompt: '', + level: 'session', + ...config, + }; + const result = await fn(base, fullConfig); + return result.context; } it('returns a Config whose registry is distinct from the parent and binds Edit/Read to the override', async () => { @@ -199,4 +215,61 @@ describe('SubagentManager.buildSubagentContextOverride bound-tool isolation', () const boundConfig = (childEdit as any).config as Config; expect(boundConfig.getApprovalMode()).toBe(ApprovalMode.AUTO_EDIT); }); + + describe('per-agent mcpServers override', () => { + it('exposes session + agent servers via getMcpServers, with agent winning on key collision', async () => { + const parent = new Config(baseParams); + const parentRegistry = await parent.createToolRegistry(undefined, { + skipDiscovery: true, + }); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (parent as any).toolRegistry = parentRegistry; + // Pre-seed a session-level MCP server so the merge has something to + // shadow. addMcpServers must be called before initialization, which + // bareMode skips for us. + parent.addMcpServers({ + 'session-only': { type: 'stdio', command: 'node-a' } as never, + shared: { type: 'stdio', command: 'session-version' } as never, + }); + + const manager = new SubagentManager(parent); + const child = await callBuildOverride(manager, parent, { + mcpServers: { + 'agent-only': { type: 'stdio', command: 'node-b' }, + shared: { type: 'stdio', command: 'agent-version' }, + }, + }); + + const merged = child.getMcpServers(); + expect(Object.keys(merged ?? {}).sort()).toEqual([ + 'agent-only', + 'session-only', + 'shared', + ]); + // Agent wins on collision (CC `scope: 'agent'` semantics). + expect((merged?.['shared'] as { command: string }).command).toBe( + 'agent-version', + ); + // Session server passes through unchanged. + expect((merged?.['session-only'] as { command: string }).command).toBe( + 'node-a', + ); + }); + + it('leaves getMcpServers untouched when no per-agent servers are declared', async () => { + const parent = new Config(baseParams); + const parentRegistry = await parent.createToolRegistry(undefined, { + skipDiscovery: true, + }); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (parent as any).toolRegistry = parentRegistry; + parent.addMcpServers({ + 'session-only': { type: 'stdio', command: 'node' } as never, + }); + const manager = new SubagentManager(parent); + const child = await callBuildOverride(manager, parent); + // Child has no own getMcpServers; prototype resolves to parent's. + expect(child.getMcpServers()).toEqual(parent.getMcpServers()); + }); + }); }); diff --git a/packages/core/src/subagents/subagent-manager.test.ts b/packages/core/src/subagents/subagent-manager.test.ts index c0b52199409..96d5811ada2 100644 --- a/packages/core/src/subagents/subagent-manager.test.ts +++ b/packages/core/src/subagents/subagent-manager.test.ts @@ -684,6 +684,71 @@ You are an agent. expect(config.maxTurns).toBeUndefined(); }); + it('should parse nested mcpServers as a record', () => { + const mcpServers = { + filesystem: { type: 'stdio', command: 'node' }, + github: { type: 'http', url: 'https://example.com' }, + }; + mockParseYaml.mockReturnValueOnce({ + name: 'a', + description: 'd', + mcpServers, + }); + const config = manager.parseSubagentContent( + '---\nname: a\ndescription: d\nmcpServers:\n filesystem:\n type: stdio\n command: node\n---\nx', + validConfig.filePath!, + 'project', + ); + expect(config.mcpServers).toEqual(mcpServers); + }); + + it('should drop mcpServers of the wrong top-level shape', () => { + mockParseYaml.mockReturnValueOnce({ + name: 'a', + description: 'd', + mcpServers: 'just-a-string', + }); + const config = manager.parseSubagentContent( + '---\nname: a\ndescription: d\nmcpServers: just-a-string\n---\nx', + validConfig.filePath!, + 'project', + ); + expect(config.mcpServers).toBeUndefined(); + }); + + it('should parse nested hooks as a record of arrays', () => { + const hooks = { + PreToolUse: [ + { matcher: 'Bash', hooks: [{ type: 'command', command: 'echo' }] }, + ], + }; + mockParseYaml.mockReturnValueOnce({ + name: 'a', + description: 'd', + hooks, + }); + const config = manager.parseSubagentContent( + '---\nname: a\ndescription: d\nhooks:\n PreToolUse:\n - matcher: Bash\n hooks:\n - type: command\n command: echo\n---\nx', + validConfig.filePath!, + 'project', + ); + expect(config.hooks).toEqual(hooks); + }); + + it('should drop hooks with non-array values per event', () => { + mockParseYaml.mockReturnValueOnce({ + name: 'a', + description: 'd', + hooks: { PreToolUse: 'not-an-array' }, + }); + const config = manager.parseSubagentContent( + '---\nname: a\ndescription: d\nhooks:\n PreToolUse: not-an-array\n---\nx', + validConfig.filePath!, + 'project', + ); + expect(config.hooks).toBeUndefined(); + }); + it('should preserve color from allowlist', () => { mockParseYaml.mockReturnValueOnce({ name: 'a', @@ -805,6 +870,40 @@ You are an agent. expect(serialized).not.toContain('background'); }); + it('should include mcpServers in the frontmatter object passed to stringifyYaml', () => { + const mcpServers = { + filesystem: { type: 'stdio', command: 'node' }, + }; + mockStringifyYaml.mockClear(); + manager.serializeSubagent({ ...validConfig, mcpServers }); + const frontmatterArg = mockStringifyYaml.mock.calls[0][0]; + expect(frontmatterArg.mcpServers).toEqual(mcpServers); + }); + + it('should include hooks in the frontmatter object passed to stringifyYaml', () => { + const hooks = { + PreToolUse: [ + { matcher: 'Bash', hooks: [{ type: 'command', command: 'echo' }] }, + ], + }; + mockStringifyYaml.mockClear(); + manager.serializeSubagent({ ...validConfig, hooks }); + const frontmatterArg = mockStringifyYaml.mock.calls[0][0]; + expect(frontmatterArg.hooks).toEqual(hooks); + }); + + it('should omit mcpServers / hooks when the record is empty', () => { + mockStringifyYaml.mockClear(); + manager.serializeSubagent({ + ...validConfig, + mcpServers: {}, + hooks: {}, + }); + const frontmatterArg = mockStringifyYaml.mock.calls[0][0]; + expect(frontmatterArg.mcpServers).toBeUndefined(); + expect(frontmatterArg.hooks).toBeUndefined(); + }); + it('should roundtrip background through serialize and parse', () => { const configWithBackground: SubagentConfig = { ...validConfig, @@ -1856,5 +1955,159 @@ System prompt 3`); expect(runtimeView).toBeUndefined(); }); }); + + describe('createAgentHeadless — caller-driven dispose contract', () => { + // Regression for self-inflicted leaks (review #4996 round 1): + // 1. `wrapAgentHooksForCleanup` relied on `AgentHeadless.execute()`'s + // inner finally firing `onStop`. Two execute() early-exit paths + // (`createChat()` → null and `prepareTools()` throwing) bypass + // that finally, so ephemeral hook entries leaked into the global + // registry for the rest of the session. + // 2. The forced tool-registry rebuild for per-agent `mcpServers` + // spawned real MCP client connections (stdio child processes, + // sockets) on a registry distinct from the parent's, but nothing + // stopped it — every subagent invocation declaring `mcpServers` + // orphaned its server processes. + // + // The unified fix is to return `{ subagent, dispose }` from + // `createAgentHeadless` and have callers run `dispose()` in a + // `finally` that they already own around `subagent.execute()`. These + // tests assert that contract. + + const baseConfig: SubagentConfig = { + name: 'cleanup-agent', + description: 'dispose contract test', + systemPrompt: 'You are a test agent.', + level: 'session' as const, + }; + + beforeEach(() => { + mockAgentHeadlessCreate.mockResolvedValue({ + execute: vi.fn(), + getResult: vi.fn(), + }); + }); + + afterEach(() => { + mockAgentHeadlessCreate.mockReset(); + }); + + it('returns { subagent, dispose }; dispose unregisters per-agent hooks', async () => { + const unregisterSpy = vi.fn(); + const addAgentHooksSpy = vi.fn().mockReturnValue(unregisterSpy); + vi.spyOn(mockConfig, 'getHookSystem').mockReturnValue({ + getRegistry: () => ({ addAgentHooks: addAgentHooksSpy }), + } as unknown as ReturnType); + + const result = await manager.createAgentHeadless( + { + ...baseConfig, + hooks: { + PreToolUse: [ + { + matcher: 'Bash', + hooks: [{ type: 'command', command: 'echo' }], + }, + ], + }, + }, + mockConfig, + ); + + // The whole point: callers need an explicit cleanup handle they can + // invoke from the outer `finally`. A return shape of just + // `AgentHeadless` (the pre-fix contract) gives them no way to do + // that, because the inner onStop wrap doesn't fire on every + // execute() exit path. + expect(result).toHaveProperty('subagent'); + expect(result).toHaveProperty('dispose'); + expect(typeof result.dispose).toBe('function'); + expect(addAgentHooksSpy).toHaveBeenCalledTimes(1); + expect(unregisterSpy).not.toHaveBeenCalled(); + + await result.dispose(); + + expect(unregisterSpy).toHaveBeenCalledTimes(1); + }); + + it('dispose unregisters even when execute() never runs (early-exit leak fix)', async () => { + // Caller pattern: + // const { subagent, dispose } = await createAgentHeadless(...); + // try { await subagent.execute(...); } finally { await dispose(); } + // We never call execute() in this test — that simulates the + // createChat-returns-null and prepareTools-throws paths where the + // pre-fix `onStop` wrapping never fired its cleanup. + const unregisterSpy = vi.fn(); + vi.spyOn(mockConfig, 'getHookSystem').mockReturnValue({ + getRegistry: () => ({ + addAgentHooks: vi.fn().mockReturnValue(unregisterSpy), + }), + } as unknown as ReturnType); + + const { dispose } = await manager.createAgentHeadless( + { + ...baseConfig, + hooks: { + PreToolUse: [ + { + matcher: '*', + hooks: [{ type: 'command', command: 'echo' }], + }, + ], + }, + }, + mockConfig, + ); + + await dispose(); + expect(unregisterSpy).toHaveBeenCalledTimes(1); + }); + + it('dispose is a safe no-op when neither hooks nor mcpServers are declared', async () => { + const result = await manager.createAgentHeadless( + baseConfig, + mockConfig, + ); + expect(typeof result.dispose).toBe('function'); + // Must not throw — the caller's `finally` always invokes dispose, + // even for agents that triggered no cleanup-bearing setup. + await expect(result.dispose()).resolves.toBeUndefined(); + }); + + it('runs cleanup when AgentHeadless.create throws — caller never gets dispose', async () => { + // Constructor-failure path inside createAgentHeadless: the caller + // never receives `{ subagent, dispose }`, so the inner catch must + // run the same cleanup itself. Without that, a transient + // AgentHeadless.create failure (e.g. ContentGenerator init blows + // up) would orphan the hook entries we just registered. + const unregisterSpy = vi.fn(); + vi.spyOn(mockConfig, 'getHookSystem').mockReturnValue({ + getRegistry: () => ({ + addAgentHooks: vi.fn().mockReturnValue(unregisterSpy), + }), + } as unknown as ReturnType); + mockAgentHeadlessCreate.mockRejectedValueOnce( + new Error('synthetic constructor failure'), + ); + + await expect( + manager.createAgentHeadless( + { + ...baseConfig, + hooks: { + PreToolUse: [ + { + matcher: '*', + hooks: [{ type: 'command', command: 'echo' }], + }, + ], + }, + }, + mockConfig, + ), + ).rejects.toThrow(/synthetic constructor failure/); + expect(unregisterSpy).toHaveBeenCalledTimes(1); + }); + }); }); }); diff --git a/packages/core/src/subagents/subagent-manager.ts b/packages/core/src/subagents/subagent-manager.ts index ce0702f60c4..eb7d2582d3a 100644 --- a/packages/core/src/subagents/subagent-manager.ts +++ b/packages/core/src/subagents/subagent-manager.ts @@ -7,6 +7,7 @@ import * as fs from 'fs/promises'; import * as path from 'path'; import * as os from 'os'; +import { randomUUID } from 'crypto'; import { parse as parseYaml, stringify as stringifyYaml, @@ -31,8 +32,9 @@ import type { AgentEventEmitter, AgentHooks, } from '../agents/runtime/agent-events.js'; -import type { Config } from '../config/config.js'; +import type { Config, MCPServerConfig } from '../config/config.js'; import { APPROVAL_MODES } from '../config/config.js'; +import type { HookDefinition, HookEventName } from '../hooks/types.js'; import type { RuntimeContentGeneratorView } from '../agents/runtime/agent-context.js'; import { createRuntimeContentGeneratorView } from '../models/content-generator-config.js'; import { createDebugLogger } from '../utils/debugLogger.js'; @@ -48,6 +50,8 @@ import { COLOR_VALUES, isColor, isPermissionMode, + parseAgentHooks, + parseAgentMcpServers, parseMaxTurns, claudePermissionModeToApprovalMode, } from './agent-frontmatter-schema.js'; @@ -630,6 +634,16 @@ export class SubagentManager { frontmatter['maxTurns'] = config.maxTurns; } + // Nested CC fields. Safe to round-trip with the eemeli/yaml parser; the + // previous skip-list carve-out is gone (see docs/yaml-parser-replacement.md). + if (config.mcpServers && Object.keys(config.mcpServers).length > 0) { + frontmatter['mcpServers'] = config.mcpServers; + } + + if (config.hooks && Object.keys(config.hooks).length > 0) { + frontmatter['hooks'] = config.hooks; + } + // Serialize to YAML const yamlContent = stringifyYaml(frontmatter, { lineWidth: 0, // Disable line wrapping @@ -641,11 +655,29 @@ export class SubagentManager { } /** - * Creates an AgentHeadless from a subagent configuration. + * Creates an AgentHeadless from a subagent configuration and returns a + * `dispose` callback that releases the per-spawn cleanup-bearing resources + * (ephemeral hook entries registered against the session's HookRegistry, + * the per-agent tool registry created when `mcpServers` triggers a force + * rebuild and the MCP child processes / sockets it owns). + * + * Callers MUST invoke `dispose` in a `finally` block around the + * `subagent.execute()` call. This is the only reliable way to clean up + * across every execute() exit path: the inner try/finally inside + * `AgentHeadless.execute()` does not fire `onStop` on the early-exit + * paths (`createChat()` returning null, `prepareTools()` throwing), and a + * leaked HookRegistry entry would fire globally for every matching event + * for the rest of the session; a leaked ToolRegistry would leave stdio + * child processes alive until process exit. + * + * `dispose` is idempotent — calling it twice is safe (the unregister + * callback filters by `agentScope` and is a no-op the second time; the + * registry's `stop()` is itself documented idempotent). * * @param config - Subagent configuration * @param runtimeContext - Runtime context - * @returns Promise resolving to AgentHeadless + * @returns the AgentHeadless and a `dispose` callback to run in the + * caller's `finally` block. */ async createAgentHeadless( config: SubagentConfig, @@ -658,7 +690,38 @@ export class SubagentManager { runConfigOverrides?: Partial; toolConfigOverride?: ToolConfig; }, - ): Promise { + ): Promise<{ subagent: AgentHeadless; dispose: () => Promise }> { + // Track per-spawn cleanup callbacks declared outside the inner + // `try/catch` so the catch can fire them on a constructor failure + // before the caller ever receives the return value. The successful + // path puts the same callbacks behind `dispose`. Both inner callbacks + // are idempotent at the source (`HookRegistry.addAgentHooks` filters + // by `agentScope`; `ToolRegistry.stop` is documented idempotent), so + // `runCleanup` doesn't need its own null-out guards — a duplicate + // invocation is at worst wasted work, never a re-fire of side effects. + let unregisterAgentHooks: (() => void) | undefined; + let disposeSubagentRegistry: (() => Promise) | undefined; + const runCleanup = async (): Promise => { + if (unregisterAgentHooks) { + try { + unregisterAgentHooks(); + } catch (error) { + debugLogger.warn( + `Subagent "${config.name}": failed to unregister per-agent hooks: ${error instanceof Error ? error.message : String(error)}`, + ); + } + } + if (disposeSubagentRegistry) { + try { + await disposeSubagentRegistry(); + } catch (error) { + debugLogger.warn( + `Subagent "${config.name}": failed to stop per-agent ToolRegistry: ${error instanceof Error ? error.message : String(error)}`, + ); + } + } + }; + try { const runtimeConfig = await this.convertToRuntimeConfig( config, @@ -688,20 +751,57 @@ export class SubagentManager { runtimeContext, ); - const subagentContext = - await this.buildSubagentContextOverride(runtimeContext); + const { context: subagentContext, cleanup } = + await this.buildSubagentContextOverride(runtimeContext, config); + disposeSubagentRegistry = cleanup; + + // Register per-agent frontmatter hooks. The returned unregister callback + // is invoked from `dispose` (and from the catch block below on a + // constructor failure). v1 limitation: while the entries live in the + // registry they fire for every event of their declared type, regardless + // of which agent is currently active — proper per-agent scope filtering + // is deferred. + const hookSystem = runtimeContext.getHookSystem(); + const hookRegistry = hookSystem?.getRegistry(); + if (config.hooks && Object.keys(config.hooks).length > 0) { + if (hookRegistry) { + const agentScope = `agent:${config.name}:${randomUUID()}`; + unregisterAgentHooks = hookRegistry.addAgentHooks( + config.hooks as { [K in HookEventName]?: HookDefinition[] }, + agentScope, + ); + } else { + // Single outer guard; nested branch on hookRegistry. The pre-fix + // structure repeated the `config.hooks && Object.keys(...).length` + // predicate across two `if`/`else if` arms, which made it easy to + // drift one side during future edits. + debugLogger.warn( + `Subagent "${config.name}" declares hooks but the host has no HookSystem; ignoring per-agent hooks.`, + ); + } + } - return await AgentHeadless.create( - config.name, - subagentContext, - promptConfig, - modelConfig, - runConfig, - toolConfig, - options?.eventEmitter, - options?.hooks, - runtimeView, - ); + try { + const subagent = await AgentHeadless.create( + config.name, + subagentContext, + promptConfig, + modelConfig, + runConfig, + toolConfig, + options?.eventEmitter, + options?.hooks, + runtimeView, + ); + return { subagent, dispose: runCleanup }; + } catch (innerError) { + // The caller never received the return value — `dispose` cannot + // possibly fire. Run the cleanup ourselves so the registered hook + // entries and the rebuilt ToolRegistry don't leak past this + // constructor failure. + await runCleanup(); + throw innerError; + } } catch (error) { if (error instanceof Error) { throw new SubagentError( @@ -737,13 +837,97 @@ export class SubagentManager { */ private async buildSubagentContextOverride( runtimeContext: Config, - ): Promise { + config: SubagentConfig, + ): Promise<{ + context: Config; + /** + * Set only when this call force-rebuilt the registry to land per-agent + * MCP server connections. The freshly built registry owns stdio child + * processes / sockets that the parent's `Config.shutdown` cannot reach, + * so the caller (`createAgentHeadless`) carries this callback through + * to its `dispose` closure and runs it when the subagent terminates. + * + * Field name matches the `cleanup` field on + * `ApprovalModeOverrideHandle` (the sibling override-builder return + * shape) for cross-API consistency. + */ + cleanup?: () => Promise; + }> { // eslint-disable-next-line @typescript-eslint/no-explicit-any const subagentContext = Object.create(runtimeContext) as any as Config; - if (!hasRebuiltToolRegistry(runtimeContext)) { + + // Per-agent MCP server overrides. Frontmatter `mcpServers` entries shadow + // session-level servers on key collision (more-specific-wins, matching + // CC's `scope: 'agent'` semantics). The runtime MCP loader still owns + // the per-spec discriminated union validation; this only widens the set + // of servers visible to the subagent's tool registry. + const hasAgentMcpServers = + config.mcpServers && Object.keys(config.mcpServers).length > 0; + if (hasAgentMcpServers) { + const sessionServers = runtimeContext.getMcpServers() ?? {}; + // Cast: per-frontmatter specs share the same record-of-records shape as + // MCPServerConfig but the type assertion at this boundary is intentional + // — the discovery layer downstream will refuse malformed specs at + // connect time, surfacing a precise error instead of a typecheck noise. + const merged: Record = { + ...sessionServers, + ...(config.mcpServers as Record), + }; + subagentContext.getMcpServers = () => merged; + } + + // The skip-rebuild optimization (`hasRebuiltToolRegistry`) is bypassed + // when per-agent `mcpServers` are present: without a fresh rebuild + // anchored on `subagentContext`, the existing wrapper-owned registry's + // McpClientManager would resolve `cliConfig.getMcpServers()` to the + // parent's session list and never see our merged override, so the + // discovery loop below would silently no-op. Forcing a rebuild here + // ties the manager to `subagentContext`, which is the only config in + // the chain that knows about the per-agent servers. + if (hasAgentMcpServers || !hasRebuiltToolRegistry(runtimeContext)) { await rebuildToolRegistryOnOverride(subagentContext, runtimeContext); } - return subagentContext; + + // The freshly rebuilt subagent ToolRegistry is constructed with + // `skipDiscovery: true` and then back-fills tools by copying from the + // parent's registry — which only knows about the session-level MCP + // servers. Per-agent servers (or per-agent overrides of an existing + // server) therefore need explicit discovery here so their tools land in + // the subagent's registry before AgentHeadless runs. The discovery + // method is idempotent and de-dupes in-flight calls, so a key shared + // with the session set is safe to discover again — it picks up the + // override spec rather than the session one. + if (hasAgentMcpServers && config.mcpServers) { + const subagentRegistry = subagentContext.getToolRegistry(); + const serverNames = Object.keys(config.mcpServers); + // Parallel discovery: one misbehaving server (e.g. a stdio command + // that hangs at startup) shouldn't serialise behind the others and + // delay the subagent spawn by the sum of every per-server timeout. + // Each call still carries the MCP layer's own per-server connect + // timeout (stdio default 30s, remote default 5s, per-spec override + // via `discoveryTimeoutMs`); `allSettled` only removes the + // serialisation between siblings. Rejections are logged-and-dropped + // so a single bad server doesn't block the others' tools from + // landing in the subagent's registry. + const results = await Promise.allSettled( + serverNames.map((name) => + subagentRegistry.discoverToolsForServer(name), + ), + ); + for (let i = 0; i < results.length; i++) { + const r = results[i]; + if (r.status === 'rejected') { + debugLogger.warn( + `Failed to discover per-agent MCP server "${serverNames[i]}" for subagent "${config.name}": ${r.reason instanceof Error ? r.reason.message : String(r.reason)}`, + ); + } + } + return { + context: subagentContext, + cleanup: () => subagentRegistry.stop(), + }; + } + return { context: subagentContext }; } /** @@ -1247,6 +1431,26 @@ function parseSubagentContent( ); } + // mcpServers: record-of-records shape (CC `gS8` shallow validation). + // Strict per-spec union is deferred to the runtime MCP loader. + const mcpServersRaw = frontmatter['mcpServers']; + const mcpServers = parseAgentMcpServers(mcpServersRaw); + if (mcpServersRaw !== undefined && mcpServers === undefined) { + debugLogger.warn( + `Agent file ${filePath} has invalid mcpServers (expected an object of server-name → spec). Dropping field.`, + ); + } + + // hooks: record-of-arrays shape (CC `TKO` shallow validation). + // Strict per-matcher union is deferred to the runtime hook subsystem. + const hooksRaw = frontmatter['hooks']; + const hooks = parseAgentHooks(hooksRaw); + if (hooksRaw !== undefined && hooks === undefined) { + debugLogger.warn( + `Agent file ${filePath} has invalid hooks (expected an object of HookEventName → matcher array). Dropping field.`, + ); + } + const config: SubagentConfig = { name, description, @@ -1262,6 +1466,8 @@ function parseSubagentContent( ...(background ? { background } : {}), ...(permissionMode !== undefined ? { permissionMode } : {}), ...(maxTurns !== undefined ? { maxTurns } : {}), + ...(mcpServers !== undefined ? { mcpServers } : {}), + ...(hooks !== undefined ? { hooks } : {}), }; // Validate the parsed configuration diff --git a/packages/core/src/subagents/types.ts b/packages/core/src/subagents/types.ts index 3d3c2d27289..6170bcf0d48 100644 --- a/packages/core/src/subagents/types.ts +++ b/packages/core/src/subagents/types.ts @@ -126,6 +126,25 @@ export interface SubagentConfig { */ maxTurns?: number; + /** + * Optional per-agent MCP server overrides. CC 2.1.168 declarative-agent + * field `mcpServers` (`gS8`); carried verbatim so `.claude/agents/*.md` + * round-trips. Validated shallowly at parse time (record-of-records shape, + * see `parseAgentMcpServers`); the per-spec union (`stdio` / `sse` / `http` + * / ...) is enforced by the runtime MCP loader when the subagent spawns. + */ + mcpServers?: Record; + + /** + * Optional per-agent hook overrides. CC 2.1.168 declarative-agent field + * `hooks` (`TKO`); carried verbatim so `.claude/agents/*.md` round-trips. + * Validated shallowly at parse time (record-of-arrays shape, see + * `parseAgentHooks`); the per-matcher discriminated union is enforced by + * `SessionHooksManager` when the subagent spawns. Keys are + * `HookEventName` literals (`PreToolUse`, `PostToolUse`, ...). + */ + hooks?: Record; + /** * Indicates whether this is a built-in agent. * Built-in agents cannot be modified or deleted. diff --git a/packages/core/src/tools/agent/agent.test.ts b/packages/core/src/tools/agent/agent.test.ts index 4a725344be4..31738b1def6 100644 --- a/packages/core/src/tools/agent/agent.test.ts +++ b/packages/core/src/tools/agent/agent.test.ts @@ -648,9 +648,10 @@ describe('AgentTool', () => { vi.mocked(mockSubagentManager.loadSubagent).mockResolvedValue( mockSubagents[0], ); - vi.mocked(mockSubagentManager.createAgentHeadless).mockResolvedValue( - mockAgent, - ); + vi.mocked(mockSubagentManager.createAgentHeadless).mockResolvedValue({ + subagent: mockAgent, + dispose: vi.fn().mockResolvedValue(undefined), + }); }); it('should execute subagent successfully', async () => { @@ -1437,9 +1438,10 @@ describe('AgentTool', () => { vi.mocked(mockSubagentManager.loadSubagent).mockResolvedValue( mockSubagents[0], ); - vi.mocked(mockSubagentManager.createAgentHeadless).mockResolvedValue( - mockAgent, - ); + vi.mocked(mockSubagentManager.createAgentHeadless).mockResolvedValue({ + subagent: mockAgent, + dispose: vi.fn().mockResolvedValue(undefined), + }); mockHookSystem = { fireSubagentStartEvent: vi.fn().mockResolvedValue(undefined), @@ -1619,9 +1621,10 @@ describe('AgentTool', () => { vi.mocked(mockSubagentManager.loadSubagent).mockResolvedValue( mockSubagents[0], ); - vi.mocked(mockSubagentManager.createAgentHeadless).mockResolvedValue( - mockAgent, - ); + vi.mocked(mockSubagentManager.createAgentHeadless).mockResolvedValue({ + subagent: mockAgent, + dispose: vi.fn().mockResolvedValue(undefined), + }); mockHookSystem = { fireSubagentStartEvent: vi.fn().mockResolvedValue(undefined), @@ -1952,9 +1955,10 @@ describe('AgentTool', () => { emitDuringExecute(capturedInvocation.eventEmitter); }); - vi.mocked(mockSubagentManager.createAgentHeadless).mockResolvedValue( - mockAgent, - ); + vi.mocked(mockSubagentManager.createAgentHeadless).mockResolvedValue({ + subagent: mockAgent, + dispose: vi.fn().mockResolvedValue(undefined), + }); const params: AgentParams = { description: 'Edit files', @@ -2265,9 +2269,10 @@ describe('AgentTool', () => { ] = vi.fn(); vi.mocked(mockSubagentManager.loadSubagent).mockResolvedValue(bgSubagent); - vi.mocked(mockSubagentManager.createAgentHeadless).mockResolvedValue( - mockAgent, - ); + vi.mocked(mockSubagentManager.createAgentHeadless).mockResolvedValue({ + subagent: mockAgent, + dispose: vi.fn().mockResolvedValue(undefined), + }); }); it('should run in background when agent definition has background: true', async () => { diff --git a/packages/core/src/tools/agent/agent.ts b/packages/core/src/tools/agent/agent.ts index 3500a4610b3..c9170be984e 100644 --- a/packages/core/src/tools/agent/agent.ts +++ b/packages/core/src/tools/agent/agent.ts @@ -2031,16 +2031,26 @@ class AgentToolInvocation extends BaseToolInvocation { let subagent: AgentHeadless; let taskPrompt: string; + // Per-spawn cleanup the subagent manager returns. The caller MUST + // invoke this in the same `finally` block that wraps `execute()` — + // see SubagentManager.createAgentHeadless's JSDoc for the leak + // scenarios it covers (ephemeral HookRegistry entries, force-rebuilt + // ToolRegistry owning per-agent MCP child processes / sockets). + // Fork subagents share the parent's lifecycle and need no per-spawn + // dispose, so this stays undefined on the fork path. + let subagentDispose: (() => Promise) | undefined; if (isFork) { const fork = await this.createForkSubagent(agentConfig); subagent = fork.subagent; taskPrompt = fork.taskPrompt; } else { - subagent = await this.subagentManager.createAgentHeadless( + const result = await this.subagentManager.createAgentHeadless( subagentConfig, agentConfig, { eventEmitter: this.eventEmitter }, ); + subagent = result.subagent; + subagentDispose = result.dispose; taskPrompt = this.params.prompt; } @@ -2117,6 +2127,11 @@ class AgentToolInvocation extends BaseToolInvocation { let bgTaskPrompt: string; let bgPromptConfig: PromptConfig | undefined; let bgToolConfig: ToolConfig | undefined; + // Per-spawn cleanup from `createAgentHeadless` (background path). + // The bg `finally` below invokes this alongside the existing + // parent-registry stop; see the foreground call site for the leak + // scenarios it covers. + let bgSubagentDispose: (() => Promise) | undefined; if (isFork) { const fork = await this.createForkSubagent( bgConfig as Config, @@ -2128,11 +2143,13 @@ class AgentToolInvocation extends BaseToolInvocation { bgPromptConfig = fork.promptConfig; bgToolConfig = fork.toolConfig; } else { - bgSubagent = await this.subagentManager.createAgentHeadless( + const bgResult = await this.subagentManager.createAgentHeadless( subagentConfig, bgConfig as Config, { eventEmitter: bgEventEmitter }, ); + bgSubagent = bgResult.subagent; + bgSubagentDispose = bgResult.dispose; bgTaskPrompt = this.params.prompt; } @@ -2486,6 +2503,12 @@ class AgentToolInvocation extends BaseToolInvocation { .getToolRegistry() .stop() .catch(() => {}); + // Per-spawn cleanup from `SubagentManager.createAgentHeadless` + // (background path). Mirrors the foreground finally: releases + // agent-scope hook entries and stops the per-agent ToolRegistry + // owning MCP child processes; not redundant with the parent + // registry stop above. + void bgSubagentDispose?.().catch(() => {}); // Restore parent PermissionManager's dangerous allow rules // if this AUTO override stripped them. Background path: // restore fires when the bg agent terminates (complete / @@ -2891,6 +2914,13 @@ class AgentToolInvocation extends BaseToolInvocation { .getToolRegistry() .stop() .catch(() => {}); + // Per-spawn cleanup from `SubagentManager.createAgentHeadless`: + // releases the agent-scope hook entries registered for this + // invocation and stops the per-agent ToolRegistry that the force + // rebuild created to land `mcpServers` discovery. The parent + // `getToolRegistry().stop()` above only reaches the parent's + // registry — the per-agent one is distinct. + void subagentDispose?.().catch(() => {}); // Restore parent PermissionManager's dangerous allow rules if // this AUTO override stripped them on creation. No-op for non- // AUTO overrides and for AUTO overrides when parent was already diff --git a/packages/core/src/utils/yaml-parser.test.ts b/packages/core/src/utils/yaml-parser.test.ts index 80f94b6b945..7129d916459 100644 --- a/packages/core/src/utils/yaml-parser.test.ts +++ b/packages/core/src/utils/yaml-parser.test.ts @@ -157,16 +157,18 @@ describe('yaml-parser', () => { }); describe('stringify', () => { + // Stringify now delegates to eemeli/yaml's serializer, which appends a + // trailing newline and is free to choose among equivalent serializations + // depending on content. Assertions therefore check round-trip rather + // than byte-exact output. it('should stringify simple objects', () => { const obj = { name: 'test', description: 'A test config' }; - const result = stringify(obj); - expect(result).toBe('name: test\ndescription: A test config'); + expect(parse(stringify(obj))).toEqual(obj); }); it('should stringify arrays', () => { const obj = { tools: ['file', 'shell'] }; - const result = stringify(obj); - expect(result).toBe('tools:\n - file\n - shell'); + expect(parse(stringify(obj))).toEqual(obj); }); it('should stringify nested objects', () => { @@ -176,98 +178,62 @@ describe('yaml-parser', () => { maxTokens: 1000, }, }; - const result = stringify(obj); - expect(result).toBe( - 'modelConfig:\n temperature: 0.7\n maxTokens: 1000', - ); + expect(parse(stringify(obj))).toEqual(obj); }); - describe('string escaping security', () => { - it('should properly escape strings with quotes', () => { - const obj = { key: 'value with "quotes"' }; - const result = stringify(obj); - expect(result).toBe('key: "value with \\"quotes\\""'); - }); - - it('should properly escape strings with backslashes', () => { - const obj = { key: 'value with \\ backslash' }; - const result = stringify(obj); - expect(result).toBe('key: "value with \\\\ backslash"'); - }); - - it('should properly escape strings with backslash-quote sequences', () => { - // This is the critical security test case - const obj = { key: 'value with \\" sequence' }; - const result = stringify(obj); - // Should escape backslashes first, then quotes - expect(result).toBe('key: "value with \\\\\\" sequence"'); + describe('round-trip integrity', () => { + // Property-based: parse(stringify(x)) === x. We no longer pin the + // exact YAML bytes — eemeli/yaml's stringify legitimately chooses + // among equivalent plain / quoted / block-scalar representations + // depending on content. The contract that matters at the API + // boundary is round-trip. + it.each([ + ['simple string', 'simplevalue'], + ['with quotes', 'value with "quotes"'], + ['with backslash', 'value with \\ backslash'], + ['with backslash-quote', 'value with \\" sequence'], + ['windows-style path', 'C:\\Program Files\\"App"\\file.txt'], + ['containing colon', 'value:with:colons'], + ['containing hash', 'value#with#hash'], + ['leading/trailing spaces', ' value with spaces '], + ['multiline newlines', 'line one\nline two\nline three'], + ['unicode', '中文 — naïve café'], + ])('round-trips: %s', (_label, str) => { + const obj = { key: str }; + expect(parse(stringify(obj))).toEqual(obj); }); + }); - it('should handle complex escaping scenarios', () => { - const testCases = [ - { - input: { path: 'C:\\Program Files\\"App"\\file.txt' }, - expected: 'path: "C:\\\\Program Files\\\\\\"App\\"\\\\file.txt"', + describe('nested round-trip for mcpServers / hooks', () => { + // The previous hand-rolled stringifier emitted `[object Object]` for + // any value below the first level of nesting. With yaml.stringify + // the CC-shape `mcpServers` (record-of-records) and `hooks` + // (record-of-array-of-records) now round-trip cleanly. + it('round-trips a CC-shape mcpServers block', () => { + const obj = { + mcpServers: { + filesystem: { + type: 'stdio', + command: 'node', + args: ['/path/to/server.js'], + }, }, - { - input: { message: 'He said: \\"Hello\\"' }, - expected: 'message: "He said: \\\\\\"Hello\\\\\\""', - }, - { - input: { complex: 'Multiple \\\\ backslashes \\" and " quotes' }, - expected: - 'complex: "Multiple \\\\\\\\ backslashes \\\\\\" and \\" quotes"', - }, - ]; - - testCases.forEach(({ input, expected }) => { - const result = stringify(input); - expect(result).toBe(expected); - }); + }; + expect(parse(stringify(obj))).toEqual(obj); }); - it('should maintain round-trip integrity for escaped strings', () => { - const testStrings = [ - 'simple string', - 'string with "quotes"', - 'string with \\ backslash', - 'string with \\" sequence', - 'path\\to\\"file".txt', - 'He said: \\"Hello\\"', - 'Multiple \\\\ backslashes \\" and " quotes', - ]; - - testStrings.forEach((testString) => { - // Force quoting by adding a colon - const originalObj = { key: testString + ':' }; - const yamlString = stringify(originalObj); - const parsedObj = parse(yamlString); - expect(parsedObj).toEqual(originalObj); - }); - }); - - it('should not quote strings that do not need quoting', () => { - const obj = { key: 'simplevalue' }; - const result = stringify(obj); - expect(result).toBe('key: simplevalue'); - }); - - it('should quote strings with colons', () => { - const obj = { key: 'value:with:colons' }; - const result = stringify(obj); - expect(result).toBe('key: "value:with:colons"'); - }); - - it('should quote strings with hash symbols', () => { - const obj = { key: 'value#with#hash' }; - const result = stringify(obj); - expect(result).toBe('key: "value#with#hash"'); - }); - - it('should quote strings with leading/trailing whitespace', () => { - const obj = { key: ' value with spaces ' }; - const result = stringify(obj); - expect(result).toBe('key: " value with spaces "'); + it('round-trips a CC-shape hooks block', () => { + const obj = { + hooks: { + PreToolUse: [ + { + matcher: 'Bash', + hooks: [{ type: 'command', command: 'echo before' }], + }, + ], + }, + }; + expect(parse(stringify(obj))).toEqual(obj); }); }); diff --git a/packages/core/src/utils/yaml-parser.ts b/packages/core/src/utils/yaml-parser.ts index 41bfe2b9fd0..ead17bbb652 100644 --- a/packages/core/src/utils/yaml-parser.ts +++ b/packages/core/src/utils/yaml-parser.ts @@ -198,37 +198,23 @@ function parseSimple(yamlString: string): Record { } /** - * Converts a JavaScript object to a simple YAML string. + * Serializes a record back to YAML using the full eemeli/yaml stringifier so + * arbitrarily nested values (e.g. CC-style `mcpServers` / `hooks`) round-trip + * cleanly. The previous hand-rolled formatter only walked one level of + * nesting and emitted `[object Object]` for anything deeper, corrupting the + * file on save — see `docs/yaml-parser-replacement.md` for the audit. * - * @param obj - Object to stringify - * @param options - Stringify options - * @returns YAML string + * `lineWidth: 0` disables automatic line wrapping so multi-line strings are + * preserved as-is, matching the stable-output posture the test suite assumes. */ export function stringify( obj: Record, - _options?: { lineWidth?: number; minContentWidth?: number }, + options?: { lineWidth?: number; minContentWidth?: number }, ): string { - const lines: string[] = []; - - for (const [key, value] of Object.entries(obj)) { - if (Array.isArray(value)) { - lines.push(`${key}:`); - for (const item of value) { - lines.push(` - ${formatValue(item)}`); - } - } else if (typeof value === 'object' && value !== null) { - lines.push(`${key}:`); - for (const [subKey, subValue] of Object.entries( - value as Record, - )) { - lines.push(` ${subKey}: ${formatValue(subValue)}`); - } - } else { - lines.push(`${key}: ${formatValue(value)}`); - } - } - - return lines.join('\n'); + return yaml.stringify(obj, { + lineWidth: options?.lineWidth ?? 0, + minContentWidth: options?.minContentWidth ?? 20, + }); } /** @@ -256,25 +242,3 @@ function parseValue(value: string): unknown { // Return as string return value; } - -/** - * Formats a value for YAML output. - */ -function formatValue(value: unknown): string { - if (typeof value === 'string') { - // Quote strings that might be ambiguous or contain special characters - if ( - value.includes(':') || - value.includes('#') || - value.includes('"') || - value.includes('\\') || - value.trim() !== value - ) { - // Escape backslashes THEN quotes - return `"${value.replace(/\\/g, '\\\\').replace(/"/g, '\\"')}"`; - } - return value; - } - - return String(value); -}