fix(code-review): load a domain critic's own agent definition into its spawn prompt - #188
fix(code-review): load a domain critic's own agent definition into its spawn prompt#188peterulsteen wants to merge 1 commit into
Conversation
…s spawn prompt A domain critic declared in a project's critic-gates.json spawns as the generic code-review:code-review-worker and receives only its name as a quoted CRITIC_DOMAIN string, so `.claude/agents/<critic-name>.md` — which may define the critic's whole method — was never loaded. The only context-loading line in that prompt was the unranked "Read the repository CLAUDE.md for project context", which zero of twelve workers across three real runs obeyed. derive-spawn-spec now resolves the agent file and puts its path on the domain critic descriptor as agent_definition_file; cmd_route does the same for the fast path's PASS 3 under route.domain_critic_definitions. The spawn-reviewers skill turns that path into a non-negotiable first step in the critic's prompt. Both keys are omitted when the file does not exist, so a critic without one keeps a byte-identical descriptor, routing payload, and prompt.
|
Beginning a code-review. Please allow time for me to finish before merging the PR |
|
Starting a pinned review of |
| NON-NEGOTIABLE FIRST STEP — do this before the patches file and before forming any | ||
| opinion: Read {agent_definition_file}. That file is YOUR definition — the project wrote | ||
| it for this critic and it defines your method, your scope, and what counts as a finding | ||
| in this domain. Follow it in full; it outranks your own priors about the domain name |
There was a problem hiding this comment.
In GitHub mode this definition comes from the PR-head checkout, so a contributor can add or change the matching agent file to suppress findings, while this instruction tells the sole domain critic to follow it in full and let it outrank prior review judgment. Load doctrine from a trusted base/operator configuration for PR reviews, or treat it as untrusted and explicitly keep the shared constraints, coverage rules, and output contract non-overridable.
| # Only present when the project actually ships | ||
| # ``.claude/agents/<critic>.md``; absent otherwise, which | ||
| # leaves the stage_20 prompt byte-identical to before. | ||
| definition = _resolve_critic_definition(reviewer, critic_agents_dir) |
There was a problem hiding this comment.
This lookup defaults to the process cwd even when scope.json.review_root points at an isolated PR-head worktree, so a local PR review omits a head-only definition or loads a stale same-name definition from the operator checkout. Thread the authoritative validated review/config root through both cmd_route and spawn-spec derivation, and emit a path the worker can read from that root.
| domain name alone. | ||
| ``` | ||
|
|
||
| - **Descriptor has no `agent_definition_file`** (the common case — most critics ship no agent file) → delete the `{CRITIC_DEFINITION_STEP}` line entirely, leaving the prompt exactly as it is above without it. |
There was a problem hiding this comment.
When spawn-spec derivation falls back, stage 20 walks the static standard table without descriptors, and this rule therefore deletes the definition step even though route.domain_critic_definitions remains populated; the route map is only consulted by the fast-path instructions. Define the standard fallback substitution explicitly, using the safely resolved route entry or an equivalent fallback descriptor.
| return "" | ||
| if not stat.S_ISREG(lst.st_mode): | ||
| return "" | ||
| return str(candidate) |
There was a problem hiding this comment.
The resolver returns every regular file whose filename matches without parsing frontmatter, so review-soul.md declaring another name—or malformed frontmatter—is loaded despite the documented mismatch-rejection contract. Read the bounded frontmatter with the existing parser and require the parsed name to equal critic_name before returning the path.
| return "" | ||
| candidate = agents_dir / f"{critic_name}.md" | ||
| try: | ||
| lst = candidate.lstat() |
There was a problem hiding this comment.
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.
2 participants
Summary
A domain critic declared in a project's
.closedloop-ai/settings/critic-gates.jsonspawns assubagent_type: "code-review:code-review-worker"(skills/spawn-reviewers/SKILL.md:228) and receives only its own name as a quotedCRITIC_DOMAINstring. If the project also ships.claude/agents/<critic-name>.mddefining that critic's entire method, none of it is loaded. The only context-loading line in the domain critic prompt isSKILL.md:236— "Read the repository CLAUDE.md for project context." — an unranked soft instruction sitting after the hardFIRST…THEN…block, duplicated at:445for the fast path's PASS 3.Measured, not assumed
Across three real
/code-reviewruns inclosedloop-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-emptyCRITIC_DOMAINtoken — 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_definitionsalready takes, because the pipeline reviews untrusted checkouts._derive_spawn_agents_from_planputs the resolved path on the domain critic descriptor asagent_definition_file, only when the file exists.cmd_routedoes the same for the fast path, whose PASS 3 takes its critic names fromroute.domain_criticsrather 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 (:236standalone,:445fast-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_typeis 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
namediffers 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_fileasserts the descriptor's exact key set). The routing payload omits the new key rather than emitting an empty map, so all sixtest_prefix_goldenspawn.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 onlycode_review_helpers.pyfails 3 of the 4 — the unchanged-descriptor test passes in both states, which is the point of it.Worked example — real
derive-spawn-specrun in a scratch repo containing.claude/agents/review-soul.mdand 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_1is 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: areview-souldomain critic plus.claude/agents/review-soul.mdcarrying 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.mdentry.