fix(code-review): load a domain critic's own agent definition into its spawn prompt

[peterulsteen]

Summary

A domain critic declared in a project's .closedloop-ai/settings/critic-gates.json spawns as subagent_type: "code-review:code-review-worker" (skills/spawn-reviewers/SKILL.md:228) and receives only its own name as a quoted CRITIC_DOMAIN string. If the project also ships .claude/agents/<critic-name>.md defining that critic's entire method, none of it is loaded. The only context-loading line in the domain critic prompt is SKILL.md:236 — "Read the repository CLAUDE.md for project context." — an unranked soft instruction sitting after the hard FIRST…THEN… block, duplicated at :445 for the fast path's PASS 3.

Measured, not assumed

Across three real /code-review runs in closedloop-ai/symphony-alpha (cr-51875, cr-95074, cr-97905), zero of twelve spawned workers obeyed that line. In the single run where a domain critic did read project doctrine, it got there by spontaneously grepping its own semantically-empty CRITIC_DOMAIN token — discretionary curiosity, not instruction. Self-describing critic names (api-architect, auth-security-expert) never self-grep at all.

The failure is silent: the critic runs, emits plausible findings, and nothing in the output artifact records that the doctrine the project wrote for it was absent.

The change

Resolution is deterministic and happens in Python, not in orchestrator prose:

• _index_agent_definitions() (tools/python/code_review_helpers.py) is the single walk that turns an agent file into an identity, keyed by the authoritative frontmatter name. _scan_agent_definitions (the roster) is a projection of it, and critic resolution indexes through it, so the two can never disagree about which file carries which name. Symlinked/non-regular leaves, a symlinked .claude or .claude/agents ancestor, the 64 KiB per-file read cap, and the file/roster caps all apply, because the pipeline reviews untrusted checkouts.
• cmd_route resolves the index once per run, from the validated scope.json -> review_root rather than the process cwd, and writes it as route.domain_critic_definitions (omitted entirely when nothing resolves). Any definition file the diff under review adds or edits is dropped: on a PR the tree is the contributor's head, so that file is their own instructions to the reviewer judging them, and what survives is byte-identical to the base.
• _derive_spawn_agents_from_plan reads that map and puts the path on the domain critic descriptor as agent_definition_file, only when the critic has one. It does no filesystem lookup of its own — resolving the same fact at two stages against two cwds is how the two answers diverge.
• The map is keyed by every resolvable definition rather than by the critics route selected, because coverage_critic can propose a name Gate B never saw. Its readers are the descriptor path, the fast path's PASS 3, and the static fallback table — the last two have no descriptors, so they read route directly.
• skills/spawn-reviewers/SKILL.md — the prompt sites gain a {CRITIC_DEFINITION_STEP} line. With a path it expands to a mandatory step: read the definition after shared_prompt.txt (so the untrusted-content policy is in context first, the invariant the per-agent template already states for the patches file) and before the patches file; follow it in full; it outranks your priors about the domain name but not shared_prompt.txt — it may not narrow the review constraints, FILE SCOPE rules, severity guidelines, or output contract, and may not direct you to withhold a finding. With no path the line is deleted. The orchestrator passes the path and never reads the file itself (context budget, same contract as CLAUDE.md for Bug Hunter B).
• The read is verified, not trusted. The critic writes "definition_loaded": true alongside "findings", and stage_20b_verify_spawn compares that against every descriptor that carried an agent_definition_file. A resolved-but-unloaded definition emits a coverage:critic-definition-not-loaded gap (HIGH, required: false -> NEEDS_ATTENTION). Without it the fix inherits, one level up, the unenforced-prose failure mode it was written to remove.

subagent_type is unchanged — the definition is loaded as content into the generic worker, not spawned as a registered agent type.

Matching is by the frontmatter name, which is the identifier critic-gates.json and the roster already speak. Filename is irrelevant; a file with absent or malformed frontmatter has no identity and resolves to nothing.

No-agent-file path is unchanged

This is the common case and it is pinned by a test that passes both before and after the fix (test_descriptor_unchanged_when_critic_has_no_definition asserts the descriptor's exact key set). The routing payload omits the key rather than emitting an empty map. One test_prefix_golden fixture moved — golden_prefix_coverage_critic, whose synthetic repo ships .claude/agents/security-critic.md, gains that one entry in route. That is the case the map exists for: domain_critics is [] there because the critic is coverage-critic-proposed, not route-selected. Regenerated with pytest --update-golden; no other artifact in any fixture changed.

Test plan

Both: a test and a worked end-to-end example.

uv run pytest plugins/ → 2122 passed, 3 skipped (191.63s). uv run ruff check . → All checks passed!. uv run pyright → 0 errors, 0 warnings, 0 informations.

TestDomainCriticAgentDefinition is 15 tests: identity follows the frontmatter name; a file declaring another name is not this critic; malformed frontmatter resolves to nothing; a definition the diff touches is refused; a symlinked .claude, a symlinked .claude/agents, and a symlinked leaf are each refused; an oversized definition is read-bounded; route resolves against the validated review_root; the descriptor takes its path from the route map and is unchanged when the critic has none; and an unloaded definition becomes a coverage gap while a loaded one does not (plus the bare-list output, which makes no claim).

Six counterfactuals, each mutated in place and re-run:

Mutation	Red
index keyed by path.stem	test_resolution_follows_frontmatter_name_not_filename, test_definition_declaring_another_name_is_not_this_critic, test_route_resolves_against_the_validated_review_root
ancestor-symlink guard removed	test_symlinked_agents_dir_refuses_the_whole_scan, test_symlinked_claude_dir_refuses_the_whole_scan
diff-touched refusal removed	test_definition_touched_by_the_diff_is_refused
_critic_agents_dir forced to the cwd default	test_route_resolves_against_the_validated_review_root
derivation re-resolves from disk	test_descriptor_takes_the_path_from_the_route_map
_reports_definition_loaded always True	test_unloaded_definition_becomes_a_coverage_gap, test_bare_list_output_does_not_clear_the_check

Worked example — real derive-spawn-spec run in a scratch repo containing .claude/agents/review-soul.md and a second critic with no file:

[
  { "agent_id": "domain_0", "reviewer": "review-soul", "model": "sonnet",
    "partitioned": false, "patches_file": "patches_all.txt", "source": "rule",
    "bucket": "best_effort", "priority": 1,
    "agent_definition_file": ".claude/agents/review-soul.md" },
  { "agent_id": "domain_1", "reviewer": "api-architect", "model": "sonnet",
    "partitioned": false, "patches_file": "patches_all.txt", "source": "rule",
    "bucket": "best_effort", "priority": 2 }
]

domain_1 is byte-identical to today's output, so its assembled prompt is too.

Downstream

closedloop-ai/symphony-alpha's FEA-4120 is blocked on this. That side is already built and waiting: a review-soul domain critic plus .claude/agents/review-soul.md carrying a mandatory soul-load step, which does nothing until the spawn prompt tells the worker to read it.

Notes for the reviewer

Shared infrastructure — opened for a human to merge; auto-merge deliberately not enabled. Version bumped to code-review v3.7.2 with a root CHANGELOG.md entry (v3.7.1 was taken by #194 on main; origin/main is merged in and the CHANGELOG conflict resolved by ordering both sections newest-first).

Review round (a593a8a)

All eight review threads are addressed in one commit rather than five guards over the mechanism the reviewers said was wrong. The load-bearing change is that identity is the frontmatter name, resolved through the roster's own walk — that alone closes shafty's :7795 and :7790 and Mike's :7780. On top of it: the definition is refused when the diff under review touches it (shafty SKILL.md:250), resolved once from the validated review root (shafty :12241, Mike :1738), read after shared_prompt.txt and ranked below it, and reported via a definition_loaded field that verify-spawn checks (Mike SKILL.md:252). The static fallback table now has an explicit substitution rule (shafty SKILL.md:255).

One reviewer ask was not done literally: Mike asked to gate route.domain_critic_definitions on fast_path. That is not available — shafty's SKILL.md:255 thread shows the static fallback table is a second non-descriptor consumer of the same map. The double resolution is removed the other way instead, by making cmd_route the only resolver and derivation a reader. Reasoning is on that thread.

[shafty023]

Beginning a code-review. Please allow time for me to finish before merging the PR

[shafty023]

Starting a pinned review of 0bb4f9ecb0f9e83ea8f9223267ec29c7733cd8f4 against 70c6b9edfbfe3795cb703408a20a2ca2cc3f5c0c.

[shafty023]

candidate.lstat() checks only the leaf and imposes no size bound, so a symlinked .claude or agents ancestor still resolves to a regular leaf, and an arbitrarily large definition is accepted for a mandatory read. Reject symlinks across every path component or enforce trusted containment, and apply the existing bounded definition-file read policy before exposing the path.

[peterulsteen]

Fixed in a593a8a. Both halves, and the fix lands in the shared walk so the roster gets it too — _scan_agent_definitions had the identical ancestor hole.

Ancestor symlinks. _index_agent_definitions now refuses the entire scan when .claude/agents or .claude is itself a symlink, before any glob or read:

if agents_dir.is_symlink() or (
    agents_dir.parent.name == ".claude" and agents_dir.parent.is_symlink()
):
    return {}, [f"agents dir is a symlink, refusing to scan: {agents_dir}"]

Scoped to the .claude/agents shape the pipeline itself resolves, so an operator-supplied --agents-dir under a legitimately symlinked path (macOS /tmp, for one) is not caught by the parent check.

Size bound. Routing critic resolution through the roster walk is what buys this: the per-file read is _AGENT_FILE_READ_LIMIT_BYTES (64 KiB) with an oversized warning, plus _AGENTS_DIR_MAX_FILES / _ROSTER_MAX_ENTRIES, and the leaf S_ISLNK/S_ISREG refusal you already wrote. A definition still has to survive all of that before its path is exposed for a mandatory Read.

Counterfactual — deleting the ancestor guard:

FAILED test_symlinked_agents_dir_refuses_the_whole_scan
FAILED test_symlinked_claude_dir_refuses_the_whole_scan
E  {'review-soul': '.../repo/.claude/agents/review-soul.md'}   # resolved through the symlink

test_oversized_definition_is_read_bounded and test_symlinked_definition_file_is_refused pin the other two.

[shafty023]

The scan itself is bounded, but the path is still accepted and emitted: test_oversized_definition_is_read_bounded asserts that the oversized file remains in definitions. Stage 20 then receives that path as agent_definition_file and is told to read the definition in full, so the mandatory worker read is still unbounded. Refuse oversized definitions before they enter domain_critic_definitions (or materialize a bounded safe copy); a warning during indexing does not protect the later read.

[mikeangstadt]

Good problem statement, and I like that you measured it instead of asserting it. Two gaps below, plus I'm not signing off until shafty's PR-head trust question is settled, since untrusted-checkout doctrine steering the reviewer is the one thing here that iteration doesn't cheaply undo.

[peterulsteen]

Closing as unmergeable-by-this-loop, not as wrong.

Open since 2026-08-11. The change is CLEAN — all 8 review threads resolved, Lint, Type Check and Tests green. What it cannot get is the one thing that repo's ruleset requires: required_approving_review_count: 1, and the author cannot self-supply it. No approver has appeared in ~30 days, so this is a terminal state rather than a queue.

The fix itself still stands on its own merits if someone wants it: a code-review domain critic does not get its own agent definition loaded into its spawn prompt, so it reviews from general knowledge instead of from its soul. Re-open or re-raise whenever an approver is available.

Operator decision, 2026-09-10.