/review-pr

name: review-pr
description: Review someone else's gcx pull request, produce a ranked report with a verdict, and optionally post it to GitHub as line-anchored inline comments. Covers what to report, in what order, when to stop, how to rank over-engineering findings, how to judge whether a large diff is justified, and how to turn the report into review comments with an APPROVE/COMMENT/REQUEST_CHANGES recommendation. Trigger on "review this PR", "review #NNNN", "code review this branch", "is this diff over-engineered", "post the review", "add review comments to this PR". NOT for self-review before pushing your own work — use integrate-with-gcx for that.

Reviewing a gcx pull request

Use this skill to review work you did not write. It owns the **report**: what goes in it, in what order, and when to stop.

It defines no checks of its own. Run the checks where they already live:

• `.claude/skills/integrate-with-gcx/references/self-review.md`. Read

**Evidence discipline** first. It sets what a finding may conclude. Then run the triggers that fire for the diff. [T12](../integrate-with-gcx/references/self-review.md#t12-over-engineering) is the over-engineering rubric. [T5](../integrate-with-gcx/references/self-review.md#t5-shared-infrastructure) covers code that repeats something the repo already has.

• AGENTS.md, for the compliance hierarchy. Check all four levels in order.

Authors run the same triggers before they push. Assume they did. Treat what you find as missed, not as dismissed.

Two passes, one set of findings

Run both passes, then combine them:

1. **`/code-review`**, for correctness bugs. Do not pass `--comment`. The findings must return to you instead of going straight to the PR, or you have nothing left to combine. It signs off with "no `--comment` argument was provided, so stopping here without posting" — that is it handing findings back, not the end of your review. Do not repeat the line or stop on it. 2. **The triggers above** that fire for this diff, plus the compliance hierarchy.

Reconcile the two sets before you report anything. Remove duplicates. Keep whichever version of a finding states the failure more precisely.

The two passes can disagree. One calls a line a bug and the other calls the same line correct. Settle it against the code and report one conclusion. Never report both and leave the author to decide.

The same skill runs on a developer's machine and in the review workflow. A PR gets the same treatment either way.

What blocks a merge here

The severity split matters more than any single finding. It decides what the author must act on. Reserve the blocking tier for four things.

**A violation of `CONSTITUTION.md` or `DESIGN.md`.** Two cases recur. The agent output contract: a command declared `finite` in `cmd/gcx/root/testdata/output_classes.json` must write exactly one JSON value to stdout. A flag that writes a file and leaves stdout empty breaks that contract. Stream routing: a status line or a fallback notice on stdout lands inside a user's redirected output.

**A regression to a released command surface.** A removed or narrowed flag value, a changed exit code, or a changed output shape that existing `--json` or `--jq` callers depend on. The surface is stable within a major version.

**Behaviour that changes inside a diff the author describes as a refactor.**

**A credential that escapes.** An error message that quotes a token, an authorization code, or a password. A prompt that leaves one in the terminal input queue when the flow exits early, where the shell then reads it. Where a diff adds a control for this, check that it runs on every exit path and not only on success.

Report findings about naming, structure, duplication, and test shape as nits. Do that even when the argument for them is strong.

Do not report

• Anything CI already enforces: `mise run lint`, the test suite,

`reference-drift`, `validate-skills`, and the conformance suites in `cmd/gcx/root/`.

• Generated files under `docs/reference/cli/`, and anything in `vendor/`.
• Missing test coverage in files the diff did not touch.

Report shape

Use bold text for headings in the report, not markdown headings. Each section should be at most two sentences.

1. **Intent.** The problem, and how the change solves it. Say whether the approach is sound before you list what is wrong with it. Do not give an opinion non whether the approach is sound or not here - that will become obvious from the rest. Keep this as short as possible. 2. **Blocking.** Fix before merge: correctness bugs, regressions to shipped behaviour, safety defects, and violations of CONSTITUTION.md or DESIGN.md. Also: Behaviour that changes inside a described refactor. A call to a function, field, or option that does not exist. Overengineering issues from T12 and T5. If nothing blocks the merge, omit this section. 3. **Other findings.** Everything that does not block, worst first. Include documentation that contradicts the code it describes. Give each finding the space its argument needs and no more. Some take a paragraph. Most take a line. The label shows the difference, so a one-line nit and a worth-fixing defect can share the list. 5. **Fix summary** One combined remedy. Omit this section when section 2 and 3 are empty. 6. **Verdict.** Approve or request changes. Name the findings that decide it. If you would merge a reduced version, say so, and say how much smaller.

Rules that keep the report honest

**One finding per code unit, not per check.** Name the symbol, file, or flag. Two findings that name the same symbol are one finding. Combine them and list every check the unit trips. State the count as evidence. Never split one unit across sections. A split unit turns one problem into five complaints about the same file.

**Rank section 4 by lock-in.** Lock-in is what the thing costs to remove after release:

1. Exported API with fewer than two callers 2.