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:

• _resolve_critic_definition() (tools/python/code_review_helpers.py) resolves .claude/agents/<critic-name>.md, returning "" when there is no such file. Name is grammar-checked before a path is built from it (operator config, no separators), and symlinks / non-regular files are refused — the same posture _scan_agent_definitions already takes, because the pipeline reviews untrusted checkouts.
• _derive_spawn_agents_from_plan puts the resolved path on the domain critic descriptor as agent_definition_file, only when the file exists.
• cmd_route does the same for the fast path, whose PASS 3 takes its critic names from route.domain_critics rather than from a spawn-spec descriptor: route.domain_critic_definitions, a map that is omitted entirely when no selected critic has an agent file.
• skills/spawn-reviewers/SKILL.md — both prompt sites (:236 standalone, :445 fast-path PASS 3) gain a {CRITIC_DEFINITION_STEP} line. With a path, it expands to a non-negotiable first step: read the definition before the patches file, follow it in full, it outranks your priors, and say so in your findings output if the read fails. 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).

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

Matching is by filename convention; a definition whose frontmatter name differs from its filename is not resolved.

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_no_agent_file asserts the descriptor's exact key set). The routing payload omits the new key rather than emitting an empty map, so all six test_prefix_golden spawn.json fixtures are untouched.

Test plan

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

uv run pytest plugins/ → 2107 passed, 3 skipped. uv run ruff check . → clean. uv run pyright → 0 errors.

New TestDomainCriticAgentDefinition (4 tests): descriptor carries the path when the file exists; descriptor key set is unchanged when it does not; the route map appears only for critics with files and the key vanishes when the file is deleted; the resolver refuses a symlink, a traversal-shaped name, an empty name, and a missing file. Reverting only code_review_helpers.py fails 3 of the 4 — the unchanged-descriptor test passes in both states, which is the point of it.

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.1 with a root CHANGELOG.md entry.