review-pr · git:20260909.87e70c0 · 2026-09-09 · sha256 4c0a3ba4f4f46e97
review-pr git:20260909.87e70c0A
Immutable. This exact content is served forever at /api/v1/blob/4c0a3ba4f4f46e97.
---
name: review-pr
description: Recommend the right reviewer count for a PR based on size + bias factors. Outputs a concrete plan (inline spot-check / 1 reviewer / 3-axis parallel) plus ready-to-paste Agent dispatch prompts when reviewers are warranted.
argument-hint: "<PR-number>"
---
# PR Review Recommendation
Decide how much review rigor a PR warrants and surface the dispatch prompts.
The tiers say what a PR needs AT MINIMUM.
**The recommended tier is a FLOOR, not a cap, and wall-clock / token cost is
never a reason to come in under it or to stop at it** (CLAUDE.md → "Cost is
not a tiebreaker"): when unsure which tier applies, take the higher one.
Reviewers are read-only agents that run in parallel.
The skill itself never spawns reviewers — it reads PR stats, applies the
heuristic, and prints a recommendation; the **main session orchestrator**
issues the `Agent` calls.
## Steps
0. **A PUSH is not a round-completion signal — a REPLY is. Check for one
before opening a round on a head you have not reviewed.**
Decide it from WHOSE TURN IT IS, never from a push timestamp. A round is a
conversation turn: your comment ends yours, the author's ends theirs. Two
push-timestamp spellings were tried and both skip the wait SILENTLY — a
`committed` timeline event carries the COMMIT date, not the push time (and
that is the ordinary non-force-push shape, so the error is the common
case), and an empty `$PUSH` makes jq's `.created_at > ""` true for every
comment ever written, which reads as "they replied". Neither errors.
```bash
PR=<N>; REPO=go-to-k/cdkd
ME=$(gh api user -q .login)
THEM=$(gh pr view "$PR" --repo "$REPO" --json author -q .author.login)
# All three surfaces: issue comments, review BODIES, and review-thread
# replies. A contributor answering from "Files changed" -- GitHub's default
# -- writes only the last two, and `issues/<N>/comments` never shows them.
said() { # said <login> -> newest ISO timestamp, or empty
{ gh api "repos/$REPO/issues/$PR/comments" --paginate -q ".[]|select(.user.login==\"$1\")|.created_at"
gh api "repos/$REPO/pulls/$PR/comments" --paginate -q ".[]|select(.user.login==\"$1\")|.created_at"
gh api "repos/$REPO/pulls/$PR/reviews" --paginate -q ".[]|select(.user.login==\"$1\" and .submitted_at!=null)|.submitted_at"
} | sort | tail -1
}
MINE=$(said "$ME"); THEIRS=$(said "$THEM")
```
Then, in order — the first match wins:
- `MINE` empty → **no round has happened**; nothing to complete. Proceed.
- `THEIRS` newer than `MINE` → **their turn ended**. Proceed. ISO-8601
sorts lexically, so compare with bash's `[[ "$THEIRS" > "$MINE" ]]` — NOT
`[ ... ]`, whose `>` is undefined for strings and which zsh rejects
outright (`condition expected: >`, measured while writing this). In a
shell you do not control:
`[ "$(printf '%s\n%s\n' "$MINE" "$THEIRS" | sort | tail -1)" = "$THEIRS" ]`
with an added `!=` guard for the equal case.
- otherwise → **you spoke last and they have not answered**. Compute
`now - MINE`; under ~30 minutes, WAIT. Waiting means arming a signal, not
a bare intention: a foreground `sleep` is blocked, so start a background
poll (or a `Monitor`) and end the turn as WAITING naming it. At ~30
minutes or more, proceed and say in the comment which head you reviewed.
Residual, stated rather than papered over: a contributor who replies and
THEN pushes again reads as "their turn ended", so that push is reviewed
immediately. That is the pre-rule behaviour, and erring toward reviewing is
the safe direction — an unnecessary round costs a comment, a missed one
costs the contributor a stall.
A contributor pushes MID-round routinely — to bank work before a rebase,
to start CI on a partial change, after a fix they know is not the whole
round. Nothing distinguishes those from the push that finishes a round, so
a push alone does not say "your turn": go-to-k/cdkd#2840 measured 7 pairs
of adjacent opposite-author comments under 5 minutes across 15 PRs, the
tightest 39 s, and the concrete cost on go-to-k/cdkd#2753 was a review
round answering a head the author had already moved past plus a second
comment repeating requests their reply had crossed.
**The rule is on the REVIEWER side on purpose.** Both contributor-side
signals proposed in that issue — mark the PR draft while responding, or
apply a `review ok` label — were refused: nothing can enforce either, the
label needs triage permission a fork contributor does not have (measured:
403 on go-to-k/cdkd#2809), and both fail SILENTLY, so a contributor who
forgets the ritual is punished by a review that never arrives and no way
to see why. This version requires nothing of the contributor, is not
opt-in, and applies to every contributor uniformly.
The ~30 minute bound is measured, not picked: across this contributor's
PRs, 41 of 43 replies landed within 30 minutes of the push they answered.
Skip the wait outright, each condition decidable from a command rather than
a judgement: the maintainer asked for this review in the current turn; the
PR is yours (`[ "$ME" = "$THEM" ]`); or `MINE` is empty, which is the same
first-branch test as above — there is no round for the push to be
completing.
When you do open a round, name the head sha you reviewed. That is what
lets a crossed reply be recognised as crossed instead of re-litigated.
1. **Fetch PR stats**:
```bash
gh pr view <N> --json additions,deletions,changedFiles,title,headRefName,files \
-q '{a: .additions, d: .deletions, fc: .changedFiles, title: .title, branch: .headRefName, paths: [.files[].path]}'
```
`loc = a + d`, **minus auto-generated LOC** — `docs/_generated/**` and
lockfiles inflate LOC without reviewer surface (reviewers audit the SCRIPT
that produced them):
```bash
excluded=$(gh pr view <N> --json files \
-q '[.files[] | select(.path | test("^docs/_generated/|(^|/)pnpm-lock\\.yaml$|(^|/)package-lock\\.json$|(^|/)yarn\\.lock$")) | .additions + .deletions] | add // 0')
loc=$(( a + d - excluded ))
```
(PR #404: 4286 raw LOC → ~1100 substantive after exclusion. `fc` is NOT
adjusted — a 12-file diff is still cross-cutting when 2 files are
generated. The lockfile patterns are `(^|/)`-anchored because the lockfile
lives at repo ROOT — a bare `/pnpm-lock\.yaml$` never matched it, PR #1082.
The same exclusion lives in `.claude/hooks/pr-review-gate.sh`; keep the two
regexes in sync.)
**Then gather each touched `src/` file's recent HISTORY in the same pass**
— the recent-defect up-bias in step 3 is the one trigger that cannot be read off
`paths`, so a step that does not fetch it leaves the trigger to be
remembered rather than evaluated. Measured on go-to-k/cdkd#2612: the loop
below scores its two `src/` files 2 and 3, and the tier was still raised
from standing memory rather than from a query.
```bash
BASE=$(gh pr view <N> --json baseRefOid -q .baseRefOid) # NOT plain HEAD --
# run on the PR branch, an unanchored log counts the PR's OWN fix-back
# commits and a 2-fix-back PR self-trips the bias.
git fetch -q origin
# The `if` is the point, not the `echo`. baseRefOid can be missing locally
# (shallow clone; a base that exists only on the remote), and then `git log`
# dies, `|| true` swallows it, and every file prints 0 -- a VOID probe whose
# output is character-identical to a clean one. Warning BESIDE the loop does
# not fix that; the loop must not run at all.
if git rev-parse --verify -q "$BASE^{commit}" >/dev/null; then
for f in $(gh pr view <N> --json files -q '.files[].path' | grep -E '^src/'); do
# `|| true` because `grep -c` exits 1 when the count is zero.
n=$(git log --oneline -3 "$BASE" -- "$f" | grep -cE '^[a-f0-9]+ fix(\(|:)' || true)
printf '%s\t%s\n' "$f" "$n"
done # n >= 2 of the last 3 on a file = evaluate the step-3 bias
else
echo "base $BASE is not local -- history probe VOID, not zero"
fi
```
**That loop is `^src/`-filtered, and for `.claude/**` no prefix count works
at all — which is where the stakes are highest.** A `feat:` / `fix:` title
with no `src/**` file is refused (in CI since go-to-k/cdkd#2717), so an
agent-instruction change lands as `chore:` (or `docs:` / `test:`) and scores
zero however many times the file has been corrected: measured on this file,
whose last five commits carry no `fix:` while the go-to-k/cdkd#2595 run's
retro was correcting a gap in text go-to-k/cdkd#2596 had added to it one run
earlier. A nonzero is no better, because a commit staging `src/**` AND a
`.claude/**` file may carry `fix:` and score the instruction file for a
defect that was never in it (`git log --oneline -5 origin/main --
.claude/rules/testing.md` returns go-to-k/cdkd#2450, a `fix(state):`).
So for a `.claude/**` path, read the SUBJECTS rather than a count
(`git log --oneline -5 "$BASE" -- "$f"`) and treat consecutive retro /
correction commits on the same file as the recency signal.
2. **Base tier** from `(loc, fc)`:
| Condition | Base tier |
|-----------|-----------|
| `loc < 300` OR `fc < 5` | **inline** (spot-check by the orchestrator) |
| `300 <= loc < 1000` AND `5 <= fc < 10` | **1-reviewer** (single code-quality pass) |
| `loc >= 1000` OR `fc >= 10` | **3-axis** (spec + code + test in parallel) |
The boundary overlap is intentional: a 200-LOC / 12-file PR is 3-axis via
file count (cross-cutting risk regardless of LOC).
3. **Bias factors** from the `paths` list:
**Up-bias triggers** (tier UP one step, clamped at 3-axis):
- Any path matches **security / process-launch surface**:
- `src/utils/role-arn.ts`
- `src/utils/docker-cmd.ts`
- `src/local/cognito-jwt.ts`
- `src/local/authorizer-resolver.ts`
- `src/local/authorizer-cache.ts`
- `src/local/sigv4-verify.ts`
- `src/local/agentcore-sigv4-sign.ts`
- `src/local/docker-runner.ts`
- `src/local/docker-image-builder.ts`
- `src/local/ecr-puller.ts`
- `src/local/ecs-secrets-resolver.ts`
- `src/local/ecs-task-runner.ts`
**What belongs here**: a file is listed when it (a) verifies or mints
authn material or loads credentials, (b) resolves secret material, or
(c) launches a process or derives the executable path one is launched
from. Consumers of those primitives are NOT listed. Several entries are
thin re-export shims over cdk-local — they stay listed on purpose: a
shim edit changes WHICH implementation cdkd consumes. The list rots two
silent ways (an entry stops existing; a live surface never gets added —
both seen in issue #1972);
`tests/unit/scripts/security-surface-list-sync.test.ts` fences the first
and the four-copy sync, the second needs the (a)/(b)/(c) test re-applied
when this area changes.
- Any path under `src/provisioning/providers/**` (deletion-sensitive —
`integ-destroy` scope; real-AWS regressions cost cleanup time)
- **> 1 fix-back ROUND on the PR** ("multiple sub-agents wrote the diff").
Count DISTINCT `fix:` SUBJECTS across the PR's commits AND every commit
its timeline recorded as a former HEAD. `git log main..<branch>` alone is
what a FLATTEN erases, so that spelling killed the trigger on exactly the
PRs rewritten most — and this repo flattens by policy
(`flatten-before-rebase-gate.sh`). go-to-k/cdkd#2638. Subjects rather than
shas because an amend re-shas one round. Replaying both hooks over the 60
most recently merged PRs, the resolved TIER differs on 4; exactly one of
those, go-to-k/cdkd#2557, needs the timeline, and a merged PR's branch is
deleted, which flatters the other three.
`pr-review-gate.sh` takes the MAX of this and the branch's own `fix:`
commit count, so it can resolve HIGHER than the number below; and it skips
the query entirely once the tier can no longer change, so it can also
report 0 where this prints more. Read this as the tier-relevant floor, not
as the hook's internal count:
```bash
gh api graphql -F owner=go-to-k -F repo=cdkd -F number=<N> -f query='
query($owner:String!,$repo:String!,$number:Int!){repository(owner:$owner,name:$repo){
pullRequest(number:$number){
commits(first:100){nodes{commit{messageHeadline}}}
timelineItems(first:100,itemTypes:[HEAD_REF_FORCE_PUSHED_EVENT]){nodes{
... on HeadRefForcePushedEvent{beforeCommit{messageHeadline} afterCommit{messageHeadline}}}}}}}' \
| jq '[(.data.repository.pullRequest.commits.nodes[]?.commit.messageHeadline),
(.data.repository.pullRequest.timelineItems.nodes[]?
| .beforeCommit.messageHeadline, .afterCommit.messageHeadline)]
| map(select(. != null and test("^fix(\\(|:)"))) | unique | length'
```
Exact paths, not `.. | .messageHeadline?`. GraphQL returns `data` and
`errors` together with HTTP 200, and a recursive-descent read harvests
every `messageHeadline` in the document — including blocks this query did
not ask for — which up-biases a PR with one round. The HOOK's copy of this
read is fenced by the `graphql-malformed` case in
`.claude/hooks/pr-review-gate.test.sh`; this snippet is prose, so keep the
two spellings in step by hand.
- **The code this PR edits shipped a defect in a RECENT PR.** A judgement
trigger, not a path list: `pr-review-gate.sh` reads the diff's stats and
paths plus the BRANCH's own commit subjects, and has no view of the
edited file's HISTORY — so it cannot see this and may not require the
marker. Raise the tier anyway and say why. Read the tell off the history
step 1 gathered — 2 or more `fix:` commits among a touched file's last 3
— rather than from what you remember about the area. Measured on
go-to-k/cdkd#2593: the heuristic said `inline`,
while that log on `src/deployment/recreate-targets.ts` showed the two
preceding merges were both data-loss-guard fixes and the nearer one
(go-to-k/cdkd#2565) had fixed a fail-open reading in the very function
this PR edits again. Code + security ran, and both converged on the same
two untested response shapes. Recency is evidence about the code, the
same way a security path is.
**Security add-on reviewer (additive — NOT part of the tier ladder).**
Whenever ANY security / process-launch path matches (surface list +
`src/provisioning/providers/**`), OR the PR is a **security fix** (secrets
/ credentials, redaction / masking / escaping, sensitive-value
persistence, GHSA-tied) — ALSO dispatch **`pr-security-reviewer`**, in
addition to the size tier. The size tier decides BREADTH; a security
defect is a DEPTH concern a tiny PR can carry — the GHSA-p5qg-v9gv-hc7w
rollback blocker was surfaced by the generic reviewer only on a second,
prompted round; a standing security lens catches that class in round one.
**Down-bias triggers** (tier DOWN one step, clamped at inline) — only when
ALL paths fall in the listed buckets:
- **Pure INERT docs**: every path matches `.gitignore`, `README.md`,
`docs/**`, `package.json` (top-level dep bumps only), `tests/**/*.md`.
**Agent-instruction files are deliberately NOT here** — `CLAUDE.md`,
`.claude/rules/**`, `.claude/skills/**`, `.claude/agents/**`,
`.claude/hooks/**`, `.claude/settings*.json`, `.markgate.yml` change how
every future session behaves; a defect there has a wider blast radius
than most code. (`tests/**/*.md` catches integ READMEs; it is
deliberately NOT a blanket `**/*.md`, which re-admitted every excluded
agent-instruction file. The hook's `DOWN_DOCS_REGEX` carries the same
list minus `tests/**/*.md` — it reaches that shape through its
tests-only bucket; keep the two in sync.)
- **Test-only**: every path matches `tests/**`
Both fire → up wins (security wins).
**Down-bias is a statement about RISK, never about budget.** If a
"docs-only" diff changes a rule the agent will follow, or a "test-only"
diff changes what a checker ACCEPTS, the low-risk premise is false — do
not down-bias, and say why.
4. **Apply the bias**: inline+up→1-reviewer; 1-reviewer+up→3-axis; 3-axis+up
→3-axis (clamp); 3-axis+down→1-reviewer; 1-reviewer+down→inline;
inline+down→inline (clamp).
5. **Render the recommendation** (format below).
6. **Dispatch reviewers + set the marker** (only for `1-reviewer` / `3-axis`):
the orchestrator dispatches the recommended reviewers via the Agent tool,
waits for all, and synthesizes:
- Any **blocker** → the marker is NOT set; address the blockers and
re-run `/review-pr <N>` **from step 0**, on the PR's CURRENT stats —
step 0 and not step 1, because a fix-round re-review IS the case step 0
exists for: go-to-k/cdkd#2753's crossing happened on exactly this
re-entry, so starting at step 1 leaves the rule inert on its own
motivating case. A
fix round adds LOC and files AND a `fix:` commit, so the tier is not
fixed for the life of a PR and neither is whether `pr-review-gate.sh`
requires the marker at all: go-to-k/cdkd#2593 opened at 306 LOC / 4
files (`inline`, no marker required) and its review-fix commit took it
to 406 LOC / 6 files — `1-reviewer` by size, and `3-axis` once the
hook's second-`fix:`-commit up-bias fires on the same push. Caught only
by recomputing before the merge; re-read the stats after every fix
round, in both directions.
- Every finding minor / nit / clean → set the marker bound to the PR's
current HEAD sha:
```bash
# The pr-review gate's scope is the sentinel file at repo root, so
# writing the PR HEAD sha into it before `markgate set` binds the marker
# to that sha — a later push invalidates it. Sentinel + markgate state
# land in the CURRENT worktree; set markers from the worktree you intend
# to merge from.
gh pr view <N> --json headRefOid -q .headRefOid > .markgate-pr-review-sha
mise exec -- markgate set pr-review
```
For `inline`, the marker is NOT set — the gate's heuristic also outputs
`inline`, so no enforcement fires.
**Security add-on dispatch**: when the trigger fired, dispatch
`pr-security-reviewer` in the same parallel batch and fold its findings in
— a security blocker blocks the marker like any other. This applies at
EVERY tier, `inline` included (a skill-level requirement, not yet a hard
gate).
**NEVER set the marker without dispatching the reviewers first** — the
gate exists so an un-reviewed large PR cannot reach main
(`pr-review-gate.sh` blocks `gh pr merge` until the marker is fresh AND
the recorded sha matches HEAD).
## Output template
```
Recommendation: <inline | 1-reviewer | 3-axis>
PR #<N>: <title>
Stats: +<additions> / -<deletions> = <loc> LOC, <fc> files
Branch: <branch>
Base tier (from stats): <base>
Bias factors:
- <factor 1, or "none">
Applied bias: <up / down / none>
Final tier: <final>
Rationale: <one line>
```
Then, **if final tier is `1-reviewer`**, emit:
```
Dispatch this single reviewer (run via Agent tool in the main session):
Agent {
subagent_type: "general-purpose",
description: "PR <N> code review",
prompt: |
Read your role definition at `.claude/agents/pr-code-reviewer.md` (relative to the repo root) and follow it.
Inputs:
- PR number: <N>
- Branch: <branch>
}
```
**If final tier is `3-axis`**, emit the same block three times in ONE
parallel message, for `.claude/agents/pr-spec-reviewer.md` (add
`- Design doc: <path>` — ask the user; spec review is meaningless without
one, and if none exists, downgrade to 1-reviewer instead),
`.claude/agents/pr-code-reviewer.md`, and
`.claude/agents/pr-test-reviewer.md`.
**If final tier is `inline`**, emit:
```
No reviewer dispatch — orchestrator should spot-check inline:
- `gh pr diff <N>` — read the full diff in one pass
- For each changed file, ask: is it correct, complete, necessary?
If the inline read surfaces a non-obvious bug class (cross-cutting state
machine, race, security-sensitive logic), STOP and dispatch a code reviewer.
```
**ADDITIONALLY, if the security add-on trigger fired**, append (at ANY tier,
same parallel batch):
```
Agent {
subagent_type: "general-purpose",
description: "PR <N> security review",
prompt: |
Read your role definition at `.claude/agents/pr-security-reviewer.md` (relative to the repo root) and follow it.
Inputs:
- PR number: <N>
- Branch: <branch>
- Security concern to focus on: <name the sensitive value(s) / surface this PR touches — e.g. "the redacted secret expression persisted to state + journal; trace every reader". With no named value, the reviewer defaults to enumerating all sensitive values in the diff.>
}
```
## Important
- **Never auto-dispatch** from inside this skill — it recommends, the
orchestrator acts. Extend reviewer prompts with PR-specific context; the
blocks are starting templates.
- Thresholds are heuristics. When in doubt, go UP — the question is "would I
be comfortable being wrong about this reaching main?".
## Dry-run reference (sanity check)
Calibration set — the skill run against these should produce:
| PR | Stats | Base tier | Bias | Final |
|----|-------|-----------|------|-------|
| #240 | 390 LOC, 4 files (`.claude/hooks/*`, `CLAUDE.md`, `.claude/settings.json`) | inline (fc < 5) | none (agent-instruction files are NOT in the docs bucket) | **inline** |
| #237 | 4515 LOC, 24 files (incl. `src/local/cognito-jwt.ts`) | 3-axis | up (security surface) → clamps | **3-axis** |
| #236 | 269 LOC, 9 files (incl. `src/local/docker-image-builder.ts`, `ecr-puller.ts`) | inline (loc < 300) | up (process-launch surface) | **1-reviewer** |
| #344 | 1488 LOC, 13 files (all `.md`, incl. `CLAUDE.md`) | 3-axis | none — `CLAUDE.md` fails the inert-docs premise | **3-axis** |
| #404 | 4286 raw → ~1100 LOC after auto-gen exclusion, 19 files | 3-axis (`fc >= 10`, file count not adjusted) | none (mixed paths) | **3-axis** |
Divergence from these rows means the heuristic or trigger lists drifted —
re-read this file before trusting the recommendation. **The rows are
RECALCULATED expectations, not history** (#240 / #344 were originally decided
under a wider docs bucket); do not "restore" the old bucket to make a row
match.