/sdlc:pr-review

Generated from solutions/ontological/plugin/plugins/sdlc/skills/pr-review/pr-review.md.

Description

Run an adversarial code review of an open pull request’s diff and post the findings as a PR comment. Given a PR number, fetches the diff via gh, judges it against the project’s configured rubric (orchestrator.review.rubric_path in sdlc.yaml; falls back to a generic inlined rubric when unset), and posts a summary comment through the same self-posted-comment wrapper /sdlc:pr-respond uses, so the orchestrator’s own comment cursor filters it out on the next /sdlc:pr-check tick. Read-only on the repo (no commits, no code changes, no worktree needed) — the posted comment is its only write. This is the sdlc chain’s review-stage workflow ([[D-0019-sdlc-chain-stages-and-labels]]); the prs orchestrator tick dispatches it for a PR it has never classified before (merges never dispatches pr-review — only pr-respond, for a merge conflict), under the engine workflows.pr-review.engine names.

Allowed tools

• Bash
• Read

Source

Usage:

• /sdlc:pr-review <pr-number> — review the given PR’s diff and post a findings comment. The number is a bare integer (38), not the #38 form.

The caller is typically /sdlc:orchestrate’s prs tick, dispatching the built-in pr-review workflow (the chain’s review stage, [[D-0019-sdlc-chain-stages-and-labels]]) for a PR it has never classified before — see lib/services/orchestrator/ticks/prs.ts’s doc comment. An operator may also invoke it directly against any open PR.

Engine

This skill runs inside whatever session dispatched it — session launch picks the engine BEFORE this skill starts, from workflows.pr-review.engine in sdlc.yaml (lib/config/load.ts; falls back to 'claude' when unset) — the same per-workflow setting every other workflow name resolves through (lib/services/orchestrator/step-runner.ts’s resolveEngine), so an unattended review can run a different model from the one that authored the PR. This skill does not itself pick an engine or dispatch a further sub-agent for the judgment — the review IS the work this session does directly, under whichever engine launched it.

References:

• orchestrator.review.rubric_path (sdlc.yaml) — the project-root-relative path to the rubric doc Step 2 reads in full, if the project has set one. This repository sets it to [[S-0017-code-review-rubric]] (docs/planning/standards/S-0017-code-review-rubric.md). A project that leaves it unset gets Step 2’s own inlined generic rubric instead — this skill ships under solutions/ontological/, so it must work for a consuming project that has no such doc on disk (S-0002).
• ${CLAUDE_PLUGIN_ROOT}/skills/pr-check/post_self_comment.sh — the comment-posting wrapper reused in Step 4, the same one /sdlc:pr-respond uses to reply to review threads.

Output contract — deterministic marker

Exactly one terminal line on stdout, of the form:

PR-REVIEW-COMPLETE: pr=<pr-number> findings=<n>

<n> is the count of findings posted (0 for a clean pass — this skill always posts a comment, even when it has nothing to flag, so the review stage leaves a visible record that it ran). /sdlc:orchestrate keys off the PR-REVIEW-COMPLETE: prefix only.

Or, on a failure that stops the run before a comment is posted:

ERROR reason="<one-line description>"

1. Fetch the PR’s diff and context

gh pr view <pr-number> --json title,url,author,baseRefName,headRefName,state
gh pr diff <pr-number>

• gh not found, unauthenticated, or the PR doesn’t exist (non-zero exit) → ERROR reason="gh pr view <n>: <stderr>".
• state other than OPEN → still review it (a closed/merged PR can be reviewed retroactively when invoked directly); note the state in the posted comment. The orchestrator only ever dispatches this skill for open PRs.
• Empty diff (no changed lines) → skip straight to Step 3 with zero findings — there is nothing to judge.

2. Judge the diff against the configured rubric

Check <project-root>/sdlc.yaml for orchestrator.review.rubric_path (project-root relative). When it’s set and the file it names exists, read that rubric doc in full and apply it directly to the diff fetched in Step 1 — including any scope note it carries (this repository’s own rubric, [[S-0017-code-review-rubric]], scopes itself to solutions/ontological/** via its applies_to.paths; a diff outside that scope still gets judged against the same eight dimensions and severity scale below as a matter of house style, but note in the posted comment that the project rubric doesn’t formally govern that path). When rubric_path is unset, or names a file that doesn’t exist, judge the diff directly against the eight dimensions and severity scale below with no extra project-specific guidance, and note in the posted comment that no project rubric was configured.

Either way, the job is adversarial: find real problems, not rubber-stamp the PR author’s own framing of the change.

• Score against each of eight dimensions — functions, modularity/ boundaries, duplication, comments, structure, naming, dead code, tests — using the three-level severity scale (violation / tension / drift). Do not invent a fourth level.
• One finding per concrete instance, not one per dimension.
• Cite evidence: name the file (and line, when addressable) and quote or paraphrase the code that grounds each finding.
• Judge the diff, not the whole file — flag only what the diff adds, changes, or makes newly relevant; pre-existing code the diff leaves untouched is out of scope.

3. Compose the findings comment

Build one Markdown comment body. The header names the rubric actually applied in Step 2 — S-0017 (or whatever rubric_path named) when a project rubric was read, or generic when none was configured:

• No findings: **Automated review (<rubric>):** no findings.

• One or more findings: a header line (**Automated review (<rubric>):** <n> finding(s).) followed by one bullet per finding, grouped by severity (violation first, then tension, then drift), each naming the dimension, the file (and line), and the finding itself:

  **Automated review (S-0017):** 2 finding(s).
  
  **violation**
  - *Duplication* — `lib/foo.ts:42`: `formatDate` re-implements
    `lib/util/date.ts`'s `todayUtc`; import it instead of re-declaring it.
  
  **tension**
  - *Tests* — `lib/foo.test.ts`: the new error branch has no test
    exercising it.

Keep the comment to the findings themselves — this skill posts a report, it does not open a discussion thread per finding (that granularity is /sdlc:pr-respond’s job, reacting to whatever a human or this comment prompts next).

4. Post the comment

Reuse /sdlc:pr-respond’s comment-posting mechanism — do not shell out to a raw gh pr comment, which would not record the post in the pr-check cursor and would re-fire NEEDS-RESPONSE on the orchestrator’s own review comment next tick:

${CLAUDE_PLUGIN_ROOT}/skills/pr-check/post_self_comment.sh <pr-number> "<comment-body-from-step-3>"

For a multi-line body, write it to a tempfile and pass --body-file (mirrors the wrapper’s own documented usage):

body_file=$(mktemp)
cat > "$body_file" <<'EOF'
<comment body>
EOF
${CLAUDE_PLUGIN_ROOT}/skills/pr-check/post_self_comment.sh <pr-number> --body-file "$body_file"
rm -f "$body_file"

Non-zero exit → surface the wrapper’s stderr and exit with ERROR reason="post_self_comment.sh: <stderr>"; do not silently drop the review.

5. Emit the terminal marker

PR-REVIEW-COMPLETE: pr=<pr-number> findings=<n>

<n> is the count from Step 3. This is the only marker that signals the whole /sdlc:pr-review flow finished; a run that stops at an earlier ERROR never reaches it.

Failure modes

If gh is unavailable, the user is unauthenticated, or the PR doesn’t exist, Step 1 emits ERROR reason="<reason>" and the run stops before posting anything.

If post_self_comment.sh fails (network, gh auth, cursor-write permissions), the review was computed but never posted — surface the error rather than emitting PR-REVIEW-COMPLETE: for a comment that never landed.

A diff too large to review in one pass is not specially handled by this skill (unlike sdlc verify changes, which shards by file count) — judge what the diff contains; sharding is future work if this proves to be a real limitation.

Notes

• Read-only on the repo apart from the posted PR comment: no commits, no code changes, no worktree required — the diff is fetched directly via gh, so this skill never needs a local checkout of the PR’s branch.
• One PR per invocation, like /sdlc:pr-check and /sdlc:pr-respond. The orchestrator iterates; this skill does not fan out.
• This skill’s comment goes through the pr-check cursor’s self_posted_at filtering (via post_self_comment.sh), so it never triggers NEEDS-RESPONSE on its own review comment.
• The prs tick dispatches pr-review once today, for a PR it has never classified before (see lib/services/orchestrator/ticks/prs.ts’s doc comment) — not on every later commit push to the same PR. A repeat review on subsequent pushes is out of scope for this skill; see that tick’s own doc comment for the tracked limitation.