feat(code-review)!: one skill, driving the engine instead of repeating it

[pedromvgomes]

agtk code-review has done the whole review in Go since v0.11.0. The two skills went on doing the same work in prose, from the same session, against the same pull requests. This makes them one skill that drives the binary and analyses nothing itself.

The duplication was not only wasteful: both paths posted reviews to a PR and only one wrote a review marker, so agtk code-review approve read a head reviewed by the skill as never reviewed.

What replaces them

panel-code-review — resolves the target, reports what the panel costs before spending it, runs the engine, presents what came back, routes fixing. No prompt, no roster, no severity ladder: each lives in the manifest or the binary.

• The target is inferred, never asked up front. The one question it opens with is the genuinely ambiguous case — a bare "review my changes" on a branch with an open PR — and it names the consequence, because only one answer posts publicly.
• explain runs first: free, no model, so the panel is knowable before it is paid for. Reported, not put to a vote — ADR 0010 settles that a wrapper does not size the review.
• On a PR it re-lists nothing. The engine puts each finding on its own comment thread; a second copy in the terminal is a second record.
• Fixing a PR's findings routes to pr-review-resolver, always. Approval later requires each thread answered, and code changed without a reply leaves the PR no better off. --auto-fix skips the question about whether to fix (the resolver still shows its plan); --no-fix reports and stops. Both work in either target.
• It never approves and never merges.

Engine changes

A judge and validator per panel. A reviewer already named its provider, so "Claude locally, GPT on the PR" was expressible for reviewers — but judge: was one top-level key and the judge runs in every review, so the choice was made once for both contexts.

panels:
  local: {reviewers: [unified]}
  gpt:
    reviewers: [correctness]
    judge:     {provider: codex, model: gpt-5, prompt: builtin:judge}
    validator: {provider: codex, model: gpt-5, prompt: builtin:validator}
defaults: {worktree: local, pr: gpt}

Both are overrides — a panel naming neither uses the manifest's. Panel runners are held to the same prompt validation and capability check as the top-level ones. One rule changed shape: a posting context now requires every panel to resolve a validator, not just its default, since an escalation raises to another panel and --panel names any of them.

explain --pr resolves the pull request exactly as run --pr does rather than deriving a range from flags — a base and head worked out another way would explain a different change convincingly. It stays model-free (ADR 0002 untouched) but is no longer offline: explain --pr needs the App registration, bare explain stays hook-safe. A machine with no registration now fails before a panel runs.

exclude: — the paths only the repo knows are not worth reviewing

Lockfiles, vendored trees, generated files, binaries, pure renames and symlinks were already excluded unconditionally, with no flag to remember, and an excluded file reaches no reviewer rather than merely not counting toward sizing. What that vocabulary cannot recognise is a file a person wrote that no reviewer should spend its budget on — a hand-maintained fixture, a tree of golden files. A manifest can now say so:

exclude:
  - "**/testdata/**"
  - "src/snapshot_*.go"

Additive, so lockfiles still need no entry, and matched by the same glob engine touches uses.

A manifest key, deliberately not a flag. An exclusion shrinks a review, the opposite direction from an escalation — and rules that only ever raise are what make a mistaken one cost money rather than coverage. Something that shrinks has to be committed, so what a review skipped is versioned and readable by everyone, and read from the base ref where it posts so a branch cannot exclude itself. A glob typed on a command line would leave the PR no record that a path was skipped, while the findings it suppressed are what approve gates on.

Two patterns are refused: a rooted one (/src/gen.go) can never match a path git names from the repo root, and a rule that silently matches nothing is worse than no rule; ** alone empties the review, and an empty review reads exactly like a clean one.

A manifest exclusion carries its own reason rather than borrowing a mechanical one, and is reported ahead of a mechanical reason where both apply — only one of the two points at a line somebody can edit:

excluded (4):
  excluded by the manifest: src/snapshot_data.go
  generated: src/g.pb.go
  lockfile: package-lock.json
  vendored: vendor/dep/d.go

Verified end to end: in the assembled reviewer prompt, snapshot_data and package-lock appear 0 times and the changed-files section lists only the two real files.

What is lost, deliberately

The four language prompt sets are deleted rather than promoted to builtin:. That is the cost of the call internal/reviewrun/prompt.go already recorded: a stack prompt in the binary is one the toolkit owes every repo writing that language, forever. A repo that has the language writes a repo-local body under .agents/code-review/. The shared/ bodies were already superseded — comment hygiene and the conventions rule are in prompts/correctness.md, the calibration and evidence rule in the preamble.

Also gone with the scripts that implemented it: a path target (run has no pathspec). Exclusions are not lost — see exclude: above.

One presentation

references/findings.md is a single file symlinked into pr-review-resolver, so a reviewed finding looks the same whoever found it — same block, same continuous numbering, same selection grammar. Rendering dereferences the link, verified end-to-end: the consumer receives a real file, byte-identical. pr-review-resolver's job is unchanged; it keeps the Assessment: line, because it triages somebody else's claims.

Decisions recorded

• ADR 0011 — the review skill is a shell over the engine, with the rejected alternatives and the Windows symlink consequence.
• CONTEXT.md — Panel gains its own judge/validator; Review manifest no longer claims two engines read it; the Panel flagged ambiguity is restated now that the losing sense is deleted.
• docs/releases/v0.12.0.md.

Verification

go build ./..., go vet and go test ./... all clean. New tests cover panel-runner resolution and fallback, panel prompt/capability validation, the posting-context validator rule, the planned judge, explain --pr, --pr 0 routing, and manifest exclusions (matching, reason precedence, and every refused pattern). Each was mutation-tested — thirteen mutations, each caught by the intended test, code restored. The consumer render was checked end-to-end: panel-code-review renders, the old two are gone, permissions render, and the symlinked findings.md lands as a real file (-rw-r--r--, diff -q identical).

Fixed after review

A review of this branch found five defects in it. All five were confirmed against the running code before being fixed, and each fix is mutation-tested.

• testdata/ excluded nothing. A trailing separator leaves an empty final segment that no path segment equals, so the .gitignore habit — the pattern people actually write — matched zero files while the repo believed the tree was skipped. That is precisely the failure the rooted-pattern guard existed to prevent, and it was the case that got missed. Rooted, doubled and trailing separators are now one empty-segment check, and a trailing slash gets its own message naming testdata/**.
• **/* bypassed the catch-all guard, which compared against the literal strings ** and *. **/*, */** and **/** all match every path. Nothing downstream reports a change with zero reviewable files, so the panel would have run on an empty patch and reported nothing — indistinguishable from a clean review at the approval gate. The test is structural now: every segment being * or **.
• --pr 0 explained the working tree, because both explain and run routed on the flag's value rather than on the flag being named — and it accepted the --base the pull-request path refuses. Fixed at both sites.
• The explain pre-approval comment was false. A prefix rule cannot exclude a flag, so Bash(agtk code-review explain*) covers explain --pr, which reads GitHub and fetches the head. The grant is kept and the comment now says so: it starts no model, spends nothing, and the step it precedes is run --pr, which is not pre-approved and asks. Narrowing it instead would mean a prompt on every review — say the word if that trade is preferred.
• The skill mapped a dropped key that run --json does not emit. It is dropped_by_validator, so what validation dropped was silently missing from every local report.

Known gaps

• The symlink materialises as a path-shaped text file on a checkout without symlink support, and that would ship. Visible in the rendered skill rather than silent.
• explain --pr costs a GitHub round-trip and a fetch run --pr then repeats. Free of model spend, not of latency.
• Panel cost ordering counts reviewers × quorum, so a panel with a costlier judge does not sort higher.

[github-actions]

bulwark

• ✅ scan — no findings

• ✅ coverage — go: 81.2% (baseline 81.1%); go patch: 89.4% (126/141 new lines; baseline 81.1%)

📦 Full bulwark output — complete scan and coverage logs.