diff --git a/code-review/README.md b/code-review/README.md index a3b6535..dea2c22 100644 --- a/code-review/README.md +++ b/code-review/README.md @@ -1,24 +1,42 @@ # Code Review Workflow -An AI-driven code review workflow that reviews uncommitted changes, presents findings for human decision, and iterates until approved or the user declares done. +An AI-driven code review workflow with two modes: + +- **Local review** -- reviews uncommitted changes, presents findings for human decision, and iterates until approved or the user declares done. +- **PR review** -- reviews GitHub Pull Requests with deep cross-file analysis and optionally posts findings as GitHub review comments. ## Prerequisites -| Tool | Required | Purpose | -|------|----------|---------| -| Git | Yes | Diff analysis, branch detection | +| Tool | Required For | Purpose | +|------|-------------|---------| +| Git | Local review | Diff analysis, branch detection | +| `gh` (GitHub CLI) | PR review | Fetch PR contents, post review comments | -No external services (Jira, GitHub CLI) are required. The workflow operates entirely on local uncommitted changes. If the project has discoverable lint or test commands, they are run during `/continue` to validate changes. +Local review operates entirely on local uncommitted changes with no external services. PR review requires an authenticated `gh` CLI with access to the target repository. If the project has discoverable lint or test commands, they are run during `/continue` to validate changes. ## Phases +### Local Review + | Phase | Command | Purpose | Artifact(s) | |-------|---------|---------|-------------| | Start | `/start` | Discover project, review changes, present findings | `00-reviewer-profile.md`, `01-change-summary.md`, `code-review-001.md`, `review-metadata.json`, `decisions-001.json` | | Continue | `/continue` | Implement accepted changes, re-review | `review-response-{NNN}.md`, `code-review-{NNN}.md`, `decisions-{NNN}.json` | + +### PR Review + +| Phase | Command | Purpose | Artifact(s) | +|-------|---------|---------|-------------| +| PR | `/pr` | Review a GitHub PR with deep analysis | `pr-review-metadata.json`, `pr-review-001.md` | +| PR Continue | `/pr-continue` | Re-review after author pushes fixes | Updated `pr-review-metadata.json`, `pr-review-{NNN}.md` | + +### Shared + +| Phase | Command | Purpose | Artifact(s) | +|-------|---------|---------|-------------| | Clean | `/clean` | Remove artifacts from abandoned reviews | (removes artifact directory) | -## Typical Flow +## Typical Flow -- Local Review ```text /start [optional focus guidance] @@ -44,15 +62,39 @@ No external services (Jira, GitHub CLI) are required. The workflow operates enti -> removes .artifacts/code-review/{branch}/ ``` +## Typical Flow -- PR Review + +```text +/pr https://github.com/owner/repo/pull/123 + -> fetches PR metadata, diff, and full file contents via gh + -> loads existing review comments to avoid duplication + -> reads project conventions + -> performs deep cross-file analysis + -> presents findings conversationally (no severity labels or formal tables) + -> offers to post findings as GitHub review comments + +(author pushes fixes) + +/pr-continue + -> fetches changes since last review (interdiff) + -> checks what was fixed, what remains, what's new + -> presents updated findings with progress notes + -> offers to post follow-up review or approve + +(repeat until satisfied) + +/clean (optional -- removes .artifacts/code-review/pr-{number}/) +``` + ## How It Works ### Project Discovery -On first run, the workflow reads the project's AGENTS.md, CLAUDE.md, CONTRIBUTING.md, linting configs, and CI workflows to build a reviewer profile. This profile determines what the reviewer focuses on and what conventions it enforces. No manual initialization needed. +On first run, the workflow reads the project's AGENTS.md, CLAUDE.md, CONTRIBUTING.md, linting configs, and CI workflows to build a reviewer profile. This profile determines what the reviewer focuses on and what conventions it enforces. No manual initialization needed. Both local and PR review modes use this discovery step. -### The Decision Table +### Local Review: The Decision Table -After each review round, findings are presented in a structured table with both the reviewer's finding and the implementor's independent assessment: +After each local review round, findings are presented in a structured table with both the reviewer's finding and the implementor's independent assessment: ```text | # | Severity | Category | Finding | Implementor Assessment | Recommendation | @@ -63,21 +105,35 @@ After each review round, findings are presented in a structured table with both The user makes the final call on every finding. +### PR Review: Conversational Findings + +PR review uses a different presentation model. Findings are presented conversationally without severity labels, formal tables, or implementor assessments. Each finding states what the issue is, why it matters, and what to change. The goal is findings the author would actually fix, not a comprehensive checklist. + +PR review also reads full file contents (not just diffs) to catch issues that only emerge from understanding the surrounding code. + ### Reviewer Independence -When the AI runtime supports subagents, the review is performed by a separate agent with its own context. This reduces the tendency to rationalize decisions made during implementation, though a same-model subagent shares the same weights and training biases — it is not equivalent to an independent human reviewer or a different tool. The subagent is strongest at catching mechanical issues: convention violations, obvious bugs, inconsistencies with surrounding code, and missed edge cases. +When the AI runtime supports subagents, the local review is performed by a separate agent with its own context. This reduces the tendency to rationalize decisions made during implementation, though a same-model subagent shares the same weights and training biases — it is not equivalent to an independent human reviewer or a different tool. The subagent is strongest at catching mechanical issues: convention violations, obvious bugs, inconsistencies with surrounding code, and missed edge cases. When subagents are not available, the review is performed sequentially within the same context. The file-based protocol is the same either way. +PR review does not use the dual-role model -- it operates as a single reviewer perspective. + For genuinely independent review, pair this workflow with external tools (e.g., coderabbit) and human reviewers. ### Automatic Cleanup -When the reviewer approves and the user confirms, all artifacts in `.artifacts/code-review/{branch}/` are removed. The `/clean` command exists only as an escape hatch for reviews that are started but never completed. +For local review, when the reviewer approves and the user confirms, all artifacts in `.artifacts/code-review/{branch}/` are removed. + +For PR review, cleanup is offered but not automatic -- the user may want to keep the review history across rounds. + +The `/clean` command works for both modes as an escape hatch for reviews that are started but never completed. ## Artifacts -All artifacts are stored in `.artifacts/code-review/{branch}/`. +### Local Review + +Stored in `.artifacts/code-review/{branch}/`: ```text .artifacts/code-review/feature-xyz/ @@ -91,6 +147,18 @@ All artifacts are stored in `.artifacts/code-review/{branch}/`. ... ``` +### PR Review + +Stored in `.artifacts/code-review/pr-{number}/`: + +```text +.artifacts/code-review/pr-123/ + pr-review-metadata.json (PR number, head SHA, round, owner/repo) + pr-review-001.md (findings from round 1) + pr-review-002.md (findings from round 2, after author pushes) + ... +``` + ## Optional Focus Guidance When starting a review, you can provide focus guidance: @@ -129,12 +197,16 @@ code-review/ README.md # This file skills/ controller.md # Phase dispatcher and transitions - start.md # Project discovery + initial review - continue.md # Implement changes + re-review + start.md # Local: project discovery + initial review + continue.md # Local: implement changes + re-review + pr.md # PR: deep analysis + optional GitHub posting + pr-continue.md # PR: re-review after author pushes fixes clean.md # Remove abandoned review artifacts commands/ - start.md # /start command - continue.md # /continue command + start.md # /start command (local review) + continue.md # /continue command (local review) + pr.md # /pr command (PR review) + pr-continue.md # /pr-continue command (PR re-review) clean.md # /clean command ``` @@ -148,4 +220,13 @@ code-review/ ./install.sh all ``` -Then in your project, make some changes and run the `code-review` workflow's `start` command. +**Local review:** Make some changes and run `/start`. + +**PR review:** Run `/pr` with a PR URL or number: + +```text +/pr https://github.com/owner/repo/pull/123 +/pr 123 +``` + +After the author pushes fixes, run `/pr-continue` to re-review. diff --git a/code-review/SKILL.md b/code-review/SKILL.md index be91146..483ca62 100644 --- a/code-review/SKILL.md +++ b/code-review/SKILL.md @@ -1,18 +1,21 @@ --- name: code-review description: >- - AI-driven code review workflow that reviews uncommitted changes using a - discoverable reviewer profile, presents findings for human decision, and - iterates until approved. Supports --unattended for automated iteration. - Use when reviewing code before commit or PR. - Activated by commands: /start, /continue, /clean. + AI-driven code review workflow. Reviews uncommitted local changes or GitHub + Pull Requests. Local mode uses a discoverable reviewer profile with + iterative human decisions; PR mode performs deep cross-file analysis and + optionally posts findings as GitHub review comments. Supports --unattended + for local review automation. + Use when reviewing code before commit, reviewing PRs, or posting review + comments to GitHub. + Activated by commands: /start, /continue, /pr, /pr-continue, /clean. --- # Code Review Workflow Orchestrator ## Quick Start -1. If the user invoked a specific command (e.g., `/start`, `/continue`), read - `commands/{command}.md` and follow it. +1. If the user invoked a specific command (e.g., `/start`, `/continue`, `/pr`, + `/pr-continue`), read `commands/{command}.md` and follow it. 2. Otherwise, read `skills/controller.md` to load the workflow controller and follow its dispatch logic. diff --git a/code-review/commands/pr-continue.md b/code-review/commands/pr-continue.md new file mode 100644 index 0000000..7e1245a --- /dev/null +++ b/code-review/commands/pr-continue.md @@ -0,0 +1,11 @@ +--- +name: code-review:pr-continue +description: "Re-review a PR after the author pushes fixes, building on previous round context" +--- +# /pr-continue + +Read `../skills/controller.md` and follow it. + +Dispatch the **pr-continue** phase. Context: + +$ARGUMENTS diff --git a/code-review/commands/pr.md b/code-review/commands/pr.md new file mode 100644 index 0000000..5d0da5a --- /dev/null +++ b/code-review/commands/pr.md @@ -0,0 +1,11 @@ +--- +name: code-review:pr +description: "Review a GitHub Pull Request with deep analysis and optional comment posting" +--- +# /pr + +Read `../skills/controller.md` and follow it. + +Dispatch the **pr** phase. Context: + +$ARGUMENTS diff --git a/code-review/guidelines.md b/code-review/guidelines.md index 969ed1b..01272db 100644 --- a/code-review/guidelines.md +++ b/code-review/guidelines.md @@ -14,6 +14,16 @@ are specific to the interactive code-review workflow. - **Scope is about relevance, not mechanics.** The review covers all uncommitted changes, but not every file in the workspace is necessarily part of the change. Determine relevance by examining each file's content and its relationship to the logical change — not by a mechanical staged/unstaged filter. - **The review is private.** All artifacts stay in `.artifacts/` (gitignored). Review iterations are working documents, not public records. +### PR Review Principles + +These apply specifically to `/pr` and `/pr-continue`: + +- **PR review is read-only.** No local code changes, no git mutations. The author fixes their own code on their branch. +- **Single perspective.** No implementor assessment or dual-role debate. One sharp reviewer voice. The user is reading your review to decide whether to relay it to the PR author, not to mediate an internal discussion. +- **Respect existing comments.** Read existing review comments before analyzing. Do not duplicate what has already been said unless the issue was not fixed. +- **No quota.** If the PR is clean, say so. Do not manufacture findings to fill a template or justify the review's existence. +- **Depth over breadth.** A few findings the author will actually fix are worth more than a long list they will dismiss. Prioritize correctness bugs, silent failures, and cross-file inconsistencies over style preferences and theoretical concerns. + ## Hard Limits - No auto-advancing between phases. Always wait for the user. @@ -22,6 +32,8 @@ are specific to the interactive code-review workflow. - No fabricated findings. Every issue cited must reference a specific file and location in the actual diff. - No scope creep. The reviewer reviews what changed, not what didn't. - No mutating git operations (commit, push, branch, checkout) during `/start`. Read-only git commands (`git diff`, `git log`, `git ls-files`) are expected. Code changes happen only in `/continue`. +- **PR reviews: no GitHub comments without preview.** `/pr` and `/pr-continue` must show a full preview of what will be posted and wait for user confirmation before posting any review comments to GitHub. +- **PR reviews: no local code changes.** `/pr` and `/pr-continue` never modify local files. They are strictly read-only phases that analyze remote code. ## Safety @@ -29,6 +41,7 @@ are specific to the interactive code-review workflow. - Verify that each finding references real code. If a finding cites a file or line that doesn't exist in the diff, discard it. - When the implementor disagrees with a finding, present both perspectives to the user with evidence. Do not silently drop findings or silently accept them. - Before applying code changes in `/continue`, read the affected file to confirm the change is still valid (the file may have been modified between rounds). +- For PR reviews, verify that referenced files and lines exist in the PR diff before presenting findings or posting comments. The `gh api` content fetch may fail for deleted files or renamed paths -- handle gracefully. ## Quality @@ -45,6 +58,8 @@ Stop and request human guidance when: - The reviewer and implementor disagree on a CRITICAL finding - The changes affect security-sensitive code and the reviewer is uncertain - The project has no discoverable conventions and the reviewer cannot calibrate +- For PR reviews: the `gh` CLI is not authenticated or the PR is not accessible +- For PR reviews: the PR has 50+ changed files (suggest reviewing in logical groups) ## Working With the Project diff --git a/code-review/skills/clean.md b/code-review/skills/clean.md index e852add..a78e912 100644 --- a/code-review/skills/clean.md +++ b/code-review/skills/clean.md @@ -10,47 +10,59 @@ review that was abandoned or is no longer needed. ## Your Role -Find and remove review artifacts for the current branch. This command is -only needed when a review was started but not completed through the normal -`/continue` approval flow (which cleans up automatically). +Find and remove review artifacts from local reviews (branch-scoped) or PR +reviews (PR-number-scoped). This command is only needed when a review was +started but not completed through the normal approval flow. ## Critical Rules - **Only delete artifacts.** Do not modify any project files. - **Confirm before deleting.** Show the user what will be removed and wait for confirmation. -- **Branch-scoped.** Only clean artifacts for the current branch unless the - user specifies otherwise. +- **Scoped cleanup.** Clean artifacts for the current context (branch or PR + number) unless the user specifies otherwise. ## Process -### Step 1: Identify Current Branch +### Step 1: Determine Cleanup Target + +Check `$ARGUMENTS` for a PR number or URL. If present, extract the PR +number and set the target to `.artifacts/code-review/pr-{number}/`. + +If `$ARGUMENTS` is empty or does not contain a PR reference, determine the +current branch: ```bash git branch --show-current ``` +Check for artifacts in both locations: +- `.artifacts/code-review/{branch}/` (local review) +- `.artifacts/code-review/pr-*/` (PR reviews -- list any that exist) + +If both local and PR artifacts exist, present them separately and ask the +user which to clean (or both). + ### Step 2: Check for Artifacts -Check if `.artifacts/code-review/{branch}/` exists. +For the identified target(s), check if the directory exists. -If it does not exist, tell the user there are no review artifacts to clean -for this branch. +If no artifacts exist, tell the user there are no review artifacts to clean. ### Step 3: Show What Will Be Removed List all files in the artifact directory: ```bash -ls -la .artifacts/code-review/{branch}/ +ls -la {artifact_directory} ``` -Present the list to the user: +Present the list to the user. For local review: ```markdown ## Review artifacts to remove -Branch: {branch} +**Local review** -- Branch: {branch} | File | Description | |------|-------------| @@ -64,12 +76,28 @@ Branch: {branch} Confirm removal? (This cannot be undone.) ``` +For PR review: + +```markdown +## Review artifacts to remove + +**PR review** -- PR #{number} + +| File | Description | +|------|-------------| +| pr-review-metadata.json | PR review state | +| pr-review-001.md | PR review round 1 | +| pr-review-002.md | PR review round 2 | + +Confirm removal? (This cannot be undone.) +``` + ### Step 4: Remove Artifacts After user confirmation: ```bash -rm -rf .artifacts/code-review/{branch} +rm -rf {artifact_directory} ``` Clean up any empty parent directories left behind (handles branch names @@ -83,7 +111,8 @@ Tell the user the artifacts have been removed. ## Output -- Removed `.artifacts/code-review/{branch}/` directory +- Removed artifact directory (local: `.artifacts/code-review/{branch}/`, + PR: `.artifacts/code-review/pr-{number}/`) ## When This Phase Is Done diff --git a/code-review/skills/controller.md b/code-review/skills/controller.md index a7a9d4e..372ce1a 100644 --- a/code-review/skills/controller.md +++ b/code-review/skills/controller.md @@ -10,6 +10,8 @@ workflow by executing phases and handling transitions between them. ## Phases +### Local Review (uncommitted changes) + 1. **Start** (`/start`) -- `start.md` Discover the project context, build a reviewer profile, analyze uncommitted changes, run the initial review, and present findings @@ -20,16 +22,35 @@ workflow by executing phases and handling transitions between them. present new findings. Repeatable until approved. Cleans up artifacts automatically on final approval. -3. **Clean** (`/clean`) -- `clean.md` - Remove review artifacts from an abandoned review. Only needed when - the user wants to discard a review without completing it. +### PR Review (GitHub Pull Requests) + +1. **PR** (`/pr`) -- `pr.md` + Review a GitHub Pull Request. Fetches PR contents via `gh`, reads + full files for deep cross-file analysis, and presents findings in a + conversational format. Optionally posts findings as GitHub review + comments. + +2. **PR Continue** (`/pr-continue`) -- `pr-continue.md` + Re-review a PR after the author pushes fixes. Fetches the interdiff, + checks what was fixed, identifies remaining and new issues, and + presents an updated assessment. Optionally posts a follow-up review. + +### Shared + +1. **Clean** (`/clean`) -- `clean.md` + Remove review artifacts from an abandoned review. Works for both + local review and PR review artifacts. ## Workspace -All review artifacts live in `.artifacts/code-review/{branch}/` (gitignored). -Code changes happen directly in the working tree during `/continue`. +**Local review** artifacts live in `.artifacts/code-review/{branch}/` +(gitignored). Code changes happen directly in the working tree during +`/continue`. + +**PR review** artifacts live in `.artifacts/code-review/pr-{number}/` +(gitignored). No local code changes -- the author fixes their own branch. -### Artifact Directory +### Local Review Artifacts | Artifact | File | Written by | |----------|------|------------| @@ -40,6 +61,13 @@ Code changes happen directly in the working tree during `/continue`. | Response (round N) | `review-response-{NNN}.md` | `/continue` | | Decisions (round N) | `decisions-{NNN}.json` | `/start`, `/continue` | +### PR Review Artifacts + +| Artifact | File | Written by | +|----------|------|------------| +| PR review metadata | `pr-review-metadata.json` | `/pr`, `/pr-continue` | +| PR review (round N) | `pr-review-{NNN}.md` | `/pr`, `/pr-continue` | + ## How to Execute a Phase 1. **Announce** the phase to the user: *"Starting /start."* @@ -57,13 +85,13 @@ After each phase completes, present the user with **options** -- not just one next step. Use the typical flow as a baseline, but adapt to what actually happened. -### Typical Flow (Attended) +### Typical Flow -- Local Review (Attended) ```text start --> user decisions --> [continue loop] --> approved (auto-cleanup) ``` -### Typical Flow (Unattended) +### Typical Flow -- Local Review (Unattended) ```text start --unattended --> [auto continue loop] --> approved (auto-cleanup) --> summary @@ -74,9 +102,19 @@ The implementor's value-based recommendations are used as decisions, and the loop continues until the reviewer approves. The only exception is a disagreement on a CRITICAL finding, which escalates to the user. +### Typical Flow -- PR Review + +```text +/pr {URL or number} --> findings --> [optional: post to GitHub] + --> author pushes fixes --> /pr-continue --> updated findings --> [optional: post] + --> repeat until satisfied --> [optional: cleanup] +``` + +PR review does not have an unattended mode. The user drives each round. + ### What to Recommend -**In attended mode:** +**Local review (attended mode):** - `/start` completed --> recommend the user review the findings table and decide which to accept, reject, or modify. Once decided, `/continue` to @@ -88,12 +126,22 @@ disagreement on a CRITICAL finding, which escalates to the user. - `/continue` completed (not approved) --> present the new findings for user decision, then another `/continue` round. -**In unattended mode:** +**Local review (unattended mode):** Next-step recommendations are not needed — the workflow auto-advances. The controller only intervenes if a CRITICAL disagreement escalates to the user. +**PR review:** + +- `/pr` completed --> offer to post findings to GitHub. Mention + `/pr-continue` for re-reviewing after the author pushes fixes. + Mention `/clean` to discard artifacts if the review is done. +- `/pr-continue` completed (issues remain) --> offer to post findings. + Mention `/pr-continue` again for the next round after the author pushes. +- `/pr-continue` completed (all clear) --> suggest posting an `APPROVE` + review. Offer to clean up artifacts. + ### How to Present Options Lead with your top recommendation, then list alternatives briefly: @@ -113,6 +161,8 @@ Before dispatching any phase, check if the project has its own `AGENTS.md` or `CLAUDE.md`. If so, read it -- it may contain project-specific conventions, testing standards, or other guidance that affects how the review operates. +**Local review:** + When the user runs `/start`, execute the start phase. If the user runs `/start` and artifacts already exist for the current branch, warn that a review is already in progress and ask whether to continue or restart. @@ -122,6 +172,16 @@ check whether review artifacts exist for the current branch. If they do, default to `/continue` instead of `/start` — the most common intent when artifacts exist is to resume, not restart. +**PR review:** + +When the user runs `/pr`, execute the pr phase. The `$ARGUMENTS` must +contain a PR URL or number. If the user runs `/pr` and artifacts already +exist for that PR number, warn that a review is in progress and ask +whether to run `/pr-continue` instead or restart. + +When the user runs `/pr-continue`, execute the pr-continue phase. If no +artifacts exist for the PR, tell the user to run `/pr` first. + ## Error Handling If any phase fails: @@ -154,6 +214,10 @@ There are two distinct uses of subagents in this workflow: Both are recommendations, not requirements -- not all AI runtimes support subagent spawning. +PR review (`/pr`, `/pr-continue`) does not use the reviewer subagent +pattern. It operates as a single reviewer perspective by design -- there +is no implementor role to separate from. + ## Rules - **Never auto-advance in attended mode.** Always wait for the user @@ -161,8 +225,9 @@ subagent spawning. is the point of the mode. - **Recommendations come from this file, not from skills.** Skills report findings; this controller decides what to recommend next. -- **No project file changes during /start or /clean.** Both phases - operate only on artifacts. Code changes happen only in `/continue`. +- **No project file changes during /start, /pr, /pr-continue, or /clean.** + `/start`, `/pr`, `/pr-continue`, and `/clean` operate only on artifacts + and read-only commands. Code changes happen only in `/continue`. - **Cleanup is automatic on approval.** When the reviewer approves and the user confirms (or approves in unattended mode), `/continue` removes all artifacts. No separate cleanup needed. diff --git a/code-review/skills/pr-continue.md b/code-review/skills/pr-continue.md new file mode 100644 index 0000000..516d068 --- /dev/null +++ b/code-review/skills/pr-continue.md @@ -0,0 +1,204 @@ +--- +name: pr-continue +description: Re-review a PR after the author pushes fixes, building on context from previous rounds. +--- + +# PR Continue Review Skill + +You are a code reviewer returning to a Pull Request after the author has +pushed new changes. Your job is to check what was fixed, identify remaining +issues, catch anything new, and present an updated assessment. + +## Your Role + +Same single reviewer perspective as `/pr`. You have memory of what you +flagged before. Acknowledge what was fixed, focus deeper on what remains, +and check whether the fixes introduced new issues. + +## Critical Rules + +Read `../guidelines.md` for the full set of principles, hard limits, and +safety rules. The evaluation criteria in `../../_shared/review-protocol.md` +apply here -- use them to calibrate what matters, but present findings +conversationally (same format as `/pr`). + +- **Read-only.** Same as `/pr` -- no local code changes, no git mutations. +- **Single perspective.** No dual-role model. You are the reviewer. +- **Build on previous context.** Read your previous findings. Do not start + from scratch or repeat issues that were fixed. +- **Acknowledge progress.** When the author fixed something you flagged, + say so. Do not silently drop previous findings. +- **Posting requires confirmation.** Same as `/pr` -- preview before posting. + +## Process + +### Step 1: Read Previous Context + +Read `.artifacts/code-review/pr-{number}/pr-review-metadata.json` to get: +- `{owner}`, `{repo}`, `{number}` +- `{headSHA}` (the commit reviewed in the previous round) +- `{round}` (the current round number) + +If the metadata file does not exist, tell the user no previous PR review was +found. Suggest running `/pr` first and stop. + +Read the latest findings file +`.artifacts/code-review/pr-{number}/pr-review-{NNN}.md` (where NNN is the +current round number, zero-padded to 3 digits). + +### Step 2: Check for New Commits + +```bash +gh pr view {number} --json headRefOid --jq '.headRefOid' +``` + +Compare with the stored `{headSHA}`. + +If the SHA has not changed, tell the user there are no new commits since the +last review. Offer to re-review the existing code at the same SHA if they +want a fresh pass, but do not proceed automatically. + +Set the new SHA as `{newHeadSHA}`. + +### Step 3: Fetch the Interdiff + +Get what changed between the previously reviewed commit and the new head: + +```bash +gh api repos/{owner}/{repo}/compare/{headSHA}...{newHeadSHA} --jq '.files[] | {filename, status, additions, deletions, patch}' +``` + +Also fetch the full current diff for context: + +```bash +gh pr diff {number} +``` + +And the updated file list: + +```bash +gh pr diff {number} --name-only +``` + +### Step 4: Fetch Updated Full Files + +For files that changed in the interdiff (Step 3), fetch their full current +content: + +```bash +gh api repos/{owner}/{repo}/contents/{path}?ref={newHeadSHA} --jq '.content' | base64 -d +``` + +For files that did not change since the last review, the previous context +is still valid -- no need to re-fetch. + +### Step 5: Load Existing Review Comments + +Same as `/pr` Step 4 -- fetch current comments to see the full conversation: + +```bash +gh api repos/{owner}/{repo}/pulls/{number}/comments --paginate +``` + +### Step 6: Analyze Changes + +Work through the previous round's findings systematically: + +**For each previous finding:** +- Was it fixed? Check the interdiff and the current file state. +- Was it partially fixed? Note what remains. +- Was it not addressed? Carry it forward. +- Did the fix introduce a new issue? Flag it. + +**Then, review the interdiff for new issues:** +- New code added as part of fixes may have its own problems. +- The author may have made unrelated changes in the same push. +- Apply the same quality bar as `/pr`: correctness bugs, silent failures, + inconsistencies, missed edge cases. + +**Full-file context still applies.** For any file with substantial changes, +read the full file to understand how the new code fits. + +### Step 7: Present Findings + +Structure the presentation around progress: + +**Start with what improved.** List previous findings that were fixed. +Keep it brief -- one line each. + +**Then present remaining and new issues.** Same conversational format as +`/pr`: file, location, what the issue is, why it matters, concrete +suggestion. Do not repeat the full explanation for carried-forward issues +that were not addressed -- reference the previous round and note that it +remains open. + +**If everything was fixed and no new issues emerged,** say the PR looks +good. Offer to post an approving review. + +### Step 8: Update Artifacts + +Increment the round number. + +Update `.artifacts/code-review/pr-{number}/pr-review-metadata.json`: + +```json +{ + "owner": "{owner}", + "repo": "{repo}", + "number": {number}, + "headSHA": "{newHeadSHA}", + "round": {round + 1}, + "reviewed_at": "{ISO 8601 timestamp}", + "findings_count": {count}, + "previous_rounds": [ + {"round": {round}, "headSHA": "{headSHA}", "findings_count": {previous_count}} + ] +} +``` + +If `previous_rounds` does not exist in the metadata (i.e., this is the first +`/pr-continue` after an initial `/pr`), initialize it as an empty array +before appending. Preserve the array across rounds, appending each completed +round. This gives future rounds the full history without needing to read all +finding files. + +Write `.artifacts/code-review/pr-{number}/pr-review-{NNN}.md` (using the +new round number) with the findings presented to the user. + +### Step 9: Offer to Post to GitHub + +Same flow as `/pr` Step 10: + +1. Ask the user whether to post a follow-up review +2. Choose event type (`COMMENT`, `REQUEST_CHANGES`, or `APPROVE`) +3. Compute diff positions for line-level comments using the current full diff +4. Show preview, wait for confirmation +5. Post via `gh api` + +If the author fixed everything and the reviewer is satisfied, suggest +`APPROVE` as the event type. + +### Step 10: Clean Up (on approval) + +If the review is complete (the user posted an `APPROVE` or declares done), +offer to clean up artifacts: + +```bash +rm -rf .artifacts/code-review/pr-{number} +``` + +Only clean up if the user confirms. Some users may want to keep the review +history. + +## Output + +- Updated `.artifacts/code-review/pr-{number}/pr-review-metadata.json` +- `.artifacts/code-review/pr-{number}/pr-review-{NNN}.md` +- Optionally: GitHub review comments posted to the PR +- Optionally: cleaned-up artifact directory + +## When This Phase Is Done + +Present the findings to the user. If posted to GitHub, confirm success. + +Then **re-read the controller** (`controller.md`) for next-step guidance. diff --git a/code-review/skills/pr.md b/code-review/skills/pr.md new file mode 100644 index 0000000..8d9e053 --- /dev/null +++ b/code-review/skills/pr.md @@ -0,0 +1,282 @@ +--- +name: pr +description: Review a GitHub Pull Request with deep cross-file analysis and optional GitHub comment posting. +--- + +# PR Review Skill + +You are a code reviewer examining a GitHub Pull Request. Your job is to fetch +the PR contents, understand the full context of the changes, perform deep +analysis, and present findings the author would actually fix. + +## Your Role + +One sharp reviewer perspective. No implementor counter-assessment, no decision +tables, no severity labels. Find the things that matter and explain why they +matter. + +## Critical Rules + +Read `../guidelines.md` for the full set of principles, hard limits, and +safety rules. The evaluation criteria in `../../_shared/review-protocol.md` +apply to PR review -- use them to calibrate what matters, but present +findings conversationally (no severity labels or formal tables). + +- **Read-only.** No local code changes, no git mutations, no file edits. + You are reviewing someone else's work on a remote branch. +- **Single perspective.** No dual-role reviewer/implementor model. You are the + reviewer. Present your findings directly. +- **Full-file context.** Read complete files, not just diffs. The diff shows + what changed; the full file reveals whether the change fits. +- **No manufactured findings.** If the PR is clean, say so. Do not generate + findings to fill a template or meet a quota. +- **Every finding must be actionable.** The author should be able to read your + comment and know exactly what to change and why. +- **Posting requires confirmation.** Never post GitHub comments without showing + a preview and getting user approval. + +## Process + +### Step 1: Parse Input + +`$ARGUMENTS` contains either: +- A full PR URL: `https://github.com/owner/repo/pull/123` +- A bare PR number: `123` + +If a full URL, extract `{owner}`, `{repo}`, and `{number}` from the path. + +If a bare number, derive owner/repo: + +```bash +gh repo view --json nameWithOwner --jq '.nameWithOwner' +``` + +Split on `/` to get `{owner}` and `{repo}`. + +If `$ARGUMENTS` is empty or unparseable, ask the user for the PR URL or number +and stop. + +### Step 2: Fetch PR Metadata + +```bash +gh pr view {number} --json title,body,baseRefName,headRefName,headRefOid,state,author,additions,deletions,changedFiles,labels +``` + +If the PR is closed or merged, warn the user and ask whether to proceed. + +Record the `headRefOid` as `{headSHA}` -- this is the exact commit being +reviewed. + +### Step 3: Fetch the Diff and File List + +```bash +gh pr diff {number} +``` + +```bash +gh pr diff {number} --name-only +``` + +If the diff is empty, tell the user and stop. + +### Step 4: Load Existing Review Comments + +Fetch existing review comments to avoid duplicating feedback: + +```bash +gh api repos/{owner}/{repo}/pulls/{number}/comments --paginate +``` + +```bash +gh api repos/{owner}/{repo}/pulls/{number}/reviews --paginate +``` + +Build a mental map of what has already been said. When analyzing the code, +skip issues that existing comments already cover unless the existing comment +is wrong or the issue was not actually fixed. + +### Step 5: Fetch Full File Contents + +For each changed file from Step 3, fetch the complete file content from the +PR branch: + +```bash +gh api repos/{owner}/{repo}/contents/{path}?ref={headSHA} --jq '.content' | base64 -d +``` + +Skip files that are: +- Binary (the API response will indicate `encoding: none` or similar) +- Generated (e.g., `*.gen.go`, `*.pb.go`, `vendor/`, `node_modules/`, + lock files) +- Trivially changed (e.g., only whitespace or import ordering) + +**Large files (>1MB):** The GitHub Contents API returns a 403 for files +exceeding 1MB and omits the `content` field. If this happens, use the +`download_url` from the API response instead, or fetch with the raw media +type: `gh api -H "Accept: application/vnd.github.raw+json" repos/{owner}/{repo}/contents/{path}?ref={headSHA}`. +Handle 403 responses gracefully -- skip the file and note it was too large +to fetch. + +For large PRs (30+ files), prioritize fetching full contents for files with +substantial logic changes. For files with only minor changes (e.g., a single +import addition), the diff context is sufficient. + +### Step 6: Read Project Conventions + +Read whichever of these exist in the local checkout: + +1. `AGENTS.md` or `CLAUDE.md` +2. `CONTRIBUTING.md` +3. Linting configuration files +4. CI/CD workflows (`.github/workflows/`) + +These inform what conventions to check against. Adopt the project's standards, +not generic preferences. + +### Step 7: Deep Analysis + +This is the core of the review. Go beyond pattern-matching the diff: + +**Cross-file reasoning:** +- How do changes in one file interact with changes in another? +- If a function signature changed, are all callers updated? +- If a new error type was added, is it handled everywhere it can surface? + +**System-level understanding:** +- Does this change fit the project's architecture? +- Does it introduce a pattern inconsistent with the rest of the codebase? +- Are there implicit assumptions between components that should be explicit? + +**Full-file context:** +- Does a new function duplicate existing functionality in the same file or + nearby files? +- Is error handling consistent with the rest of the file? +- Does the change interact correctly with surrounding code not shown in the + diff? + +**What to prioritize:** +- Correctness bugs (logic errors, off-by-one, nil/null dereferences) +- Silent failures (errors swallowed, conditions that fail open) +- Inconsistencies between related changes across files +- Missed edge cases in the system design +- Missing or incorrect error handling +- Race conditions or concurrency issues +- Security concerns with practical impact + +**What to skip:** +- Style preferences the project doesn't enforce +- Naming nitpicks unless genuinely confusing +- Theoretical concerns without practical impact +- Things already covered by existing review comments +- Suggestions that amount to "I would have written it differently" + +### Step 8: Present Findings + +Present findings in a conversational tone. No severity labels, no formal +tables, no category tags. For each finding: + +1. **File and location** -- which file, which function or line range +2. **What the issue is** -- stated plainly +3. **Why it matters** -- the practical impact (what breaks, what fails + silently, what becomes hard to maintain) +4. **Suggestion** -- a concrete fix, not "consider improving this" + +Group related findings together when they share a root cause. + +If the PR is clean and well-written, say so. Mention what was done well if +it's notable (a good test strategy, clean error handling, thoughtful API +design). Do not manufacture praise -- only mention it if genuine. + +If relevant, check CI status: + +```bash +gh pr checks {number} +``` + +If CI is failing on something related to your findings, mention it. + +### Step 9: Write Artifacts + +Create minimal artifacts for continuity with `/pr-continue`: + +```bash +mkdir -p .artifacts/code-review/pr-{number} +``` + +Write `.artifacts/code-review/pr-{number}/pr-review-metadata.json`: + +```json +{ + "owner": "{owner}", + "repo": "{repo}", + "number": {number}, + "headSHA": "{headSHA}", + "round": 1, + "reviewed_at": "{ISO 8601 timestamp}", + "findings_count": {count} +} +``` + +Write `.artifacts/code-review/pr-{number}/pr-review-001.md` with a record of +the findings presented to the user (for reference in future rounds). + +### Step 10: Offer to Post to GitHub + +Ask the user whether they want to post the findings as a GitHub review. + +If no, the review is complete. Report that artifacts were saved and mention +`/pr-continue` for re-reviewing after the author pushes fixes. + +If yes: + +1. Ask the user to choose the review event type: + - `COMMENT` -- neutral feedback + - `REQUEST_CHANGES` -- blocking review + - `APPROVE` -- approve with comments (if there are minor suggestions) + +2. For each finding, determine the diff position for line-level commenting. + Map the finding's file and line to the correct `position` in the diff + (the line number within the diff hunk, not the file line number). Use the + diff from Step 3 to compute positions. + +3. Show a preview of the review body and each line comment before posting. + Wait for user confirmation. + +4. Construct the review payload as a JSON object and write it to a + temporary file. This avoids shell quoting issues with special characters + (quotes, newlines, backticks) in comment bodies: + +```json +{ + "event": "{event}", + "body": "{review summary}", + "comments": [ + {"path": "{file}", "position": {diff_position}, "body": "{comment}"}, + ... + ] +} +``` + +```bash +gh api repos/{owner}/{repo}/pulls/{number}/reviews \ + --method POST \ + --input review-payload.json +``` + +Write the file to the current directory or the artifact directory. Remove +it after posting (or on failure). + +If posting fails, report the error and offer to retry or skip. + +## Output + +- `.artifacts/code-review/pr-{number}/pr-review-metadata.json` +- `.artifacts/code-review/pr-{number}/pr-review-001.md` +- Optionally: GitHub review comments posted to the PR + +## When This Phase Is Done + +Present the findings to the user. If they were posted to GitHub, confirm +the post was successful. + +Then **re-read the controller** (`controller.md`) for next-step guidance.