From 0eb3a12b7c1088bf71113380d4f8b1fdedd4ef83 Mon Sep 17 00:00:00 2001 From: Asaf Ben Natan Date: Thu, 6 Aug 2026 15:11:20 +0300 Subject: [PATCH 1/3] Add pr-review skill for reviewing remote GitHub PRs and GitLab MRs Reuses the code-review workflow's review protocol and human-in-the-loop approval loop, but operates on a remote PR/MR URL: checks it out into a disposable git worktree, drafts inline comments with permalinks, snippets, and provider-correct suggestion blocks, and posts them only after explicit local approval. Co-authored-by: Cursor --- pr-review/README.md | 200 ++++++++++++ pr-review/SKILL.md | 28 ++ pr-review/commands/clean.md | 11 + pr-review/commands/continue.md | 11 + pr-review/commands/publish.md | 11 + pr-review/commands/revise.md | 11 + pr-review/commands/start.md | 11 + pr-review/guidelines.md | 132 ++++++++ pr-review/skills/clean.md | 139 +++++++++ pr-review/skills/continue.md | 180 +++++++++++ pr-review/skills/controller.md | 158 ++++++++++ pr-review/skills/publish.md | 188 +++++++++++ pr-review/skills/revise.md | 132 ++++++++ pr-review/skills/start.md | 447 +++++++++++++++++++++++++++ pr-review/templates/comment-style.md | 105 +++++++ 15 files changed, 1764 insertions(+) create mode 100644 pr-review/README.md create mode 100644 pr-review/SKILL.md create mode 100644 pr-review/commands/clean.md create mode 100644 pr-review/commands/continue.md create mode 100644 pr-review/commands/publish.md create mode 100644 pr-review/commands/revise.md create mode 100644 pr-review/commands/start.md create mode 100644 pr-review/guidelines.md create mode 100644 pr-review/skills/clean.md create mode 100644 pr-review/skills/continue.md create mode 100644 pr-review/skills/controller.md create mode 100644 pr-review/skills/publish.md create mode 100644 pr-review/skills/revise.md create mode 100644 pr-review/skills/start.md create mode 100644 pr-review/templates/comment-style.md diff --git a/pr-review/README.md b/pr-review/README.md new file mode 100644 index 0000000..ad42495 --- /dev/null +++ b/pr-review/README.md @@ -0,0 +1,200 @@ +# PR Review Workflow + +An AI-driven review of a remote GitHub pull request or GitLab merge request, +given its URL. Checks the PR/MR out into a disposable git worktree, always +explains its context before critiquing it, and drafts inline review comments +(links, snippets, suggestion blocks) in a suggestive, pluggable tone. Nothing +is posted until the local user explicitly approves the exact draft content — +this workflow never changes the reviewed code and never performs a +host-level PR/MR approval. + +## Phase Flow + +```mermaid +graph TD + start([start: PR/MR url]) --> draft{Draft review} + draft --> revise[revise: Q&A / edits] + revise --> draft + draft -->|local approval-to-post| publish[publish: post review] + publish -->|PR/MR gets new commits| continue[continue: refresh worktree, incremental re-review] + continue --> draft + clean([clean]) --> removed([worktree + artifacts removed]) +``` + +## Prerequisites + +| Tool | Required | Purpose | +|------|----------|---------| +| Git | Yes | Worktree setup, diff analysis | +| `gh` (GitHub CLI), authenticated | For GitHub PRs | Fetch PR metadata/comments, post the review | +| `glab` (GitLab CLI), authenticated | For GitLab MRs | Fetch MR metadata/discussions, post comments | + +The provider is auto-detected from the URL — you only need whichever CLI +matches the PR/MR you're reviewing. + +## Phases + +| Phase | Command | Purpose | Artifact(s) | +|-------|---------|---------|-------------| +| Start | `/start {url}` | Detect provider, check out a worktree, gather PR/MR context, draft a review | `00-reviewer-profile.md`, `01-pr-context.md`, `02-draft-review-001.md`, `review-metadata.json`, `decisions-001.json` | +| Revise | `/revise` | Answer questions, apply edits, add user findings, re-present | `02-draft-review-{NNN}.md`, `decisions-{NNN}.json` | +| Publish | `/publish` | Post the approved draft as inline review comments | `publish-metadata.json` | +| Continue | `/continue` | After new commits, refresh the worktree and draft the incremental review | Updated `01-pr-context.md`, `02-draft-review-{NNN}.md`, `decisions-{NNN}.json` | +| Clean | `/clean` | Remove the worktree, scratch clone (if any), and all artifacts | (removes worktree + artifact directory) | + +## Typical Flow + +```text +/start https://github.com/{owner}/{repo}/pull/{n} + (or a GitLab MR URL — provider is auto-detected) + -> checks out the PR/MR into a git worktree + -> gathers PR/MR context (title, author, linked issues, key decisions, + existing discussion) and always shows it first + -> builds a reviewer profile from the target project's own conventions + -> obtains a structured review, extended with permalinks, snippets, and + suggestion blocks for every finding + -> independently assesses each finding + -> drafts inline comments in a suggestive tone + -> presents the full draft for local approval-to-post + +/revise (repeatable) + -> answers your questions about specific comments + -> applies edits you request (reword, drop, change suggestions) + -> drafts any new findings you describe + -> re-presents the updated draft + +/publish (only after you approve posting) + -> posts the review as inline comments on the host + -> worktree is left in place — nothing is torn down here + +/continue (after the PR/MR receives new commits) + -> refreshes the same worktree in place + -> reviews only what's new, notes which prior comments look addressed + -> feeds back into /revise -> /publish + +/clean (once you're fully done with this PR/MR) + -> the only phase that removes the worktree, branch/scratch-clone, and + artifacts +``` + +## How It Works + +### Approve-to-Post, Not PR Approval + +"Approve," everywhere in this workflow, means the local user signing off in +chat on the exact draft content so `/publish` may post it. It is unrelated +to — and this workflow never performs — a host-level PR/MR approval. Every +posted GitHub review uses `event: COMMENT` (never `APPROVE`/ +`REQUEST_CHANGES`); the GitLab approve/unapprove endpoint is never called. + +### Context Before Critique + +Every session starts by writing and presenting `01-pr-context.md`: title, +author, base↔head, linked issues, a commit narrative, inferred design +decisions, and a summary of existing discussion. Findings are never shown +without this context first. + +### Findings Extended for Posting + +Beyond the standard finding format from `../_shared/review-protocol.md`, +every kept finding here also carries: + +- A **permalink** to the exact line(s) at the PR/MR's head SHA +- The **quoted snippet** at that location +- A **suggested-change block** (a fenced ` ```suggestion ` block) when the + fix is a concrete, mechanical replacement — omitted for conceptual or + design-level findings + +### Pluggable Comment Style + +Posted tone and structure default to suggestive framing ("Should we...", +never "Do X") with no severity/category labels or "Finding N" headers, and +a fixed `See comments below` review summary — see +`templates/comment-style.md`. A reviewed project can override this by +committing its own `.pr-review/templates/comment-style.md`. + +### The Worktree Persists Until `/clean` + +Unlike a typical scratch worktree, this one is **not** torn down after +`/publish`. It stays in place so `/continue` can refresh it (fetch + reset) +instead of re-cloning from scratch, across as many review rounds as the +PR/MR goes through. Only `/clean` removes it — run it once you're done +reviewing a given PR/MR. + +### GitHub vs. GitLab + +The workflow is almost entirely generic `git` operations plus prose review +logic. Only a few touchpoints talk to the host directly: + +| Touchpoint | GitHub (`gh`) | GitLab (`glab`) | +|---|---|---| +| Detect provider | URL path is `/pull/{n}` | URL path is `/-/merge_requests/{n}` | +| Fetch PR/MR metadata | `gh pr view` | `glab mr view` / `glab api` | +| Fetch the worktree ref | `refs/pull/{n}/head` | `refs/merge-requests/{n}/head` | +| List existing comments | `gh api .../pulls/{n}/comments` + `.../reviews` | `glab api .../discussions` | +| Post the review | One batched review via `gh api .../pulls/{n}/reviews` | One discussion per comment via `glab api .../discussions`, plus a separate summary note | + +Everything else — worktree setup, diff analysis, reviewer profile +discovery, finding drafting, tone/style, the revise loop — has no provider +branching at all. + +## Artifacts + +All artifacts and the worktree are stored in +`.artifacts/pr-review/{context}/`, where `{context}` is a sanitized +`{owner-or-namespace}-{repo-or-project}-{number}`. + +```text +.artifacts/pr-review/openai-example-repo-1234/ + 00-reviewer-profile.md (target project's conventions and review focus) + 01-pr-context.md (title, author, linked issues, key decisions, existing discussion) + review-metadata.json (provider, refs, iteration, state, timestamps) + decisions-001.json (local decisions per round) + 02-draft-review-001.md (draft review, round 1) + 02-draft-review-002.md (draft review, round 2 — after /revise or /continue) + ... + publish-metadata.json (record of what was posted, once /publish runs) + worktree/ (git worktree checked out at the PR/MR head) + _scratch-repo/ (only if no local clone of the target repo could be reused) +``` + +## Directory Structure + +```text +pr-review/ + SKILL.md # Workflow entry point + guidelines.md # Behavioral rules and hard limits + README.md # This file + templates/ + comment-style.md # Default tone/structure — pluggable + skills/ + controller.md # Phase dispatcher and transitions + start.md # URL parsing, worktree setup, context, initial draft + revise.md # Q&A, edits, user-added findings + publish.md # Post the approved review + continue.md # Refresh worktree, incremental re-review + clean.md # Remove worktree + artifacts + commands/ + start.md # /start command + revise.md # /revise command + publish.md # /publish command + continue.md # /continue command + clean.md # /clean command +``` + +## Getting Started + +```bash +# Install the workflow +./install.sh claude --workflows pr-review + +# Or install all workflows +./install.sh all +``` + +Then run the `pr-review` workflow's `start` command with a PR or MR URL: + +```text +/start https://github.com/{owner}/{repo}/pull/{number} +/start https://gitlab.com/{namespace}/{project}/-/merge_requests/{number} +``` diff --git a/pr-review/SKILL.md b/pr-review/SKILL.md new file mode 100644 index 0000000..d78fd4f --- /dev/null +++ b/pr-review/SKILL.md @@ -0,0 +1,28 @@ +--- +name: pr-review +version: 0.1.0 +description: >- + AI-driven review of a remote pull request or merge request, given its URL + (GitHub or GitLab, auto-detected). Checks out the PR/MR into a git worktree, + explains PR context and key decisions, evaluates changes using the shared + code-review protocol, and drafts inline review comments (with code links, + snippets, and suggested-change blocks) in a pluggable, suggestive tone. + Always presents the draft for local approval before posting a review, never + changes the reviewed code, supports revision based on questions or new + findings, and can resume after the PR/MR receives new commits. Use when + asked to review, comment on, or give feedback on a GitHub PR or GitLab MR + URL. Activated by commands: /start, /revise, /publish, /continue, /clean. +--- +# PR Review Workflow Orchestrator + +## Quick Start + +1. If the user invoked a specific command (e.g., `/start`, `/revise`), read + `commands/{command}.md` and follow it. +2. Otherwise, read `skills/controller.md` to load the workflow controller and + follow its dispatch logic. + +If a step fails or produces unexpected output, stop and report the error to +the user. Do not advance to the next phase. Offer to retry or escalate. + +For principles, hard limits, safety, quality, and escalation rules, see `guidelines.md`. diff --git a/pr-review/commands/clean.md b/pr-review/commands/clean.md new file mode 100644 index 0000000..639e526 --- /dev/null +++ b/pr-review/commands/clean.md @@ -0,0 +1,11 @@ +--- +name: pr-review:clean +description: "Remove the worktree and all review artifacts for a PR/MR" +--- +# /clean + +Read `../skills/controller.md` and follow it. + +Dispatch the **clean** phase. Context: + +$ARGUMENTS diff --git a/pr-review/commands/continue.md b/pr-review/commands/continue.md new file mode 100644 index 0000000..3869499 --- /dev/null +++ b/pr-review/commands/continue.md @@ -0,0 +1,11 @@ +--- +name: pr-review:continue +description: "Refresh the worktree after new commits and draft a review of what's new" +--- +# /continue + +Read `../skills/controller.md` and follow it. + +Dispatch the **continue** phase. Context: + +$ARGUMENTS diff --git a/pr-review/commands/publish.md b/pr-review/commands/publish.md new file mode 100644 index 0000000..d8dbce7 --- /dev/null +++ b/pr-review/commands/publish.md @@ -0,0 +1,11 @@ +--- +name: pr-review:publish +description: "Post the approved draft as a review with inline comments on the host" +--- +# /publish + +Read `../skills/controller.md` and follow it. + +Dispatch the **publish** phase. Context: + +$ARGUMENTS diff --git a/pr-review/commands/revise.md b/pr-review/commands/revise.md new file mode 100644 index 0000000..f582acc --- /dev/null +++ b/pr-review/commands/revise.md @@ -0,0 +1,11 @@ +--- +name: pr-review:revise +description: "Answer questions, apply edits, add findings, and re-present the draft review" +--- +# /revise + +Read `../skills/controller.md` and follow it. + +Dispatch the **revise** phase. Context: + +$ARGUMENTS diff --git a/pr-review/commands/start.md b/pr-review/commands/start.md new file mode 100644 index 0000000..efad5b7 --- /dev/null +++ b/pr-review/commands/start.md @@ -0,0 +1,11 @@ +--- +name: pr-review:start +description: "Check out a PR/MR from its URL into a worktree, gather context, and draft a review" +--- +# /start + +Read `../skills/controller.md` and follow it. + +Dispatch the **start** phase. Context: + +$ARGUMENTS diff --git a/pr-review/guidelines.md b/pr-review/guidelines.md new file mode 100644 index 0000000..f478820 --- /dev/null +++ b/pr-review/guidelines.md @@ -0,0 +1,132 @@ +# PR Review Workflow Guidelines + +## Shared Review Protocol + +Read and follow `../_shared/review-protocol.md` for evaluation criteria, +finding format, severity definitions, and core review principles. Those +shared standards apply to this workflow. The principles and rules below are +specific to reviewing a remote PR/MR. + +## Terminology + +- **"PR"** means "PR or MR" throughout this workflow's files — GitHub pull + requests and GitLab merge requests are treated identically except at the + handful of host-API touchpoints called out in `skills/start.md` and + `skills/publish.md`. +- **"Approve" means approve-to-post, not approve-the-PR.** Everywhere this + workflow says the user "approved" or "approves," it means the local user + signed off in chat on the draft review's content so `/publish` may post + it. That is a decision made entirely outside the reviewed host — it is + never a GitHub/GitLab PR/MR approval, and this workflow never performs one + (see the hard limits below). + +## Principles + +- **The review is external.** This workflow never modifies the reviewed + repository's code, ever — not even with approval. It reads a worktree and + posts comments; nothing else. This is stronger than `code-review`'s "no + changes without approval," since there is no implementation phase here at + all. +- **Context before critique.** Every review presentation leads with the PR's + context and key design decisions (title, author, linked issues, commit + narrative, existing discussion) before any findings. A reviewer who + doesn't understand what a PR is trying to do isn't ready to critique it. +- **The worktree is ephemeral, but not per-round.** The PR/MR is always + checked out into a git worktree, never reviewed by mutating the user's own + checkout. The worktree persists across `/start` -> `/revise` -> `/publish` + -> `/continue` rounds so later rounds can refresh it in place; it is + removed only by `/clean`. +- **Comments propose, they never command.** Posted comment tone is + suggestive ("Should we...", "What if...") by default, and pluggable per + `templates/comment-style.md`. Never phrase a posted comment as an + instruction ("Do X", "Please fix Y"). +- **Nothing is posted without local sign-off.** The exact rendered content + of every comment — link, snippet, suggestion block, and comment text — is + shown to the local user before `/publish` runs. Silence is not consent; + wait for an explicit go-ahead. +- **The human decides.** The reviewer proposes findings; the local user + decides which become posted comments, same as `code-review`'s "the + reviewer proposes, the user approves" principle, adapted since there is no + implementor role here. + +## Hard Limits + +- **No code changes to the reviewed repository, ever.** Not in the + worktree, not upstream. This workflow only reads code and drafts/posts + comments. +- **No auto-advancing between phases.** Always wait for the user between + `/start`, `/revise`, and `/publish`. +- **No posting without explicit local approval-to-post** of the exact + content that will be sent to the host. +- **No mutating host operations beyond the approved review post.** No + merge, close, edit of PR/MR metadata, or formal approve/request-changes + action. On GitHub, every posted review uses `event: COMMENT` — never + `APPROVE` or `REQUEST_CHANGES`. On GitLab, never call the approve/unapprove + endpoint. +- **No fabricated findings.** Every finding must cite a real file and a real + line inside the PR/MR diff, with a working permalink and an accurate + snippet read from the worktree. +- **No fabricated suggestions.** A `suggestion` code block is only included + when the fix is a concrete, mechanical replacement of the flagged lines. + Conceptual or design findings get an explanatory comment with no + suggestion block — never invent a diff just to fill the field. +- **The worktree is removed only by `/clean`.** `/publish` and `/continue` + read from and refresh the worktree but never delete it. +- **No mutating git operations against the reviewed repo's remote.** Only + local `git fetch`/`worktree`/`diff`/`merge-base` operations against a + scratch clone or a local ref — never `git push` to the reviewed repo. + +## Safety + +- Verify the matching host CLI is authenticated (`gh auth status` / + `glab auth status`) before doing anything else. +- Read the target project's own `AGENTS.md`, `CLAUDE.md`, and contribution + guidelines (from the worktree) before reviewing. The reviewed project's + conventions override general preferences. +- Verify every finding references a real file and a real, currently-diffed + line before it is drafted as a comment. If a finding cites a location that + doesn't exist in the diff, discard it. +- Before posting, confirm every comment still anchors to a line inside the + current diff — the PR/MR may have changed since the draft was written. +- If the host rejects part of a post, report exactly what failed and ask the + user how to proceed. Never silently drop or silently retry with altered + content. + +## Quality + +- Evaluation criteria are defined in `../_shared/review-protocol.md`; the + reviewer must cover all listed categories. +- Every kept finding must have a permalink, a quoted snippet, and (when the + fix is mechanical) a suggestion block — findings without a citable + location are discarded before the user ever sees them. +- Tone and structure follow `templates/comment-style.md` (or the project's + override, if one exists) for every posted comment, with no exceptions. +- The review summary posted to the host is always the literal string + `See comments below` — never restate finding counts or severities there. + +## Escalation + +Stop and request human guidance when: + +- The PR/MR is too large to review meaningfully in one pass (recommend + splitting the review into focus areas). +- The host CLI is not authenticated, or the PR/MR cannot be fetched + (private repo without access, deleted PR/MR, etc.). +- `git worktree add` fails for a reason other than "already checked out" + (see `skills/start.md`). +- The host rejects one or more comments when posting (a line moved outside + the diff, permissions issue, etc.). +- The project has no discoverable conventions and the reviewer cannot + calibrate. + +## Working With the Reviewed Project + +This workflow reviews a project it does not own. Respect it: + +- Read and follow the reviewed project's own `AGENTS.md`/`CLAUDE.md` (from + the worktree) for conventions and review focus areas. +- Honor a project-level comment-style override + (`.pr-review/templates/comment-style.md` in the reviewed repo) if one + exists — see `templates/comment-style.md` for the resolution order. +- Do not impose the local user's personal style preferences over the + reviewed project's own documented conventions. diff --git a/pr-review/skills/clean.md b/pr-review/skills/clean.md new file mode 100644 index 0000000..925a1b1 --- /dev/null +++ b/pr-review/skills/clean.md @@ -0,0 +1,139 @@ +--- +name: clean +description: Remove the worktree, scratch clone (if any), and all review artifacts. The only phase that tears down the worktree. +--- + +# Clean PR Review Skill + +You are a cleanup utility. Your job is to remove the git worktree and all +artifacts for a PR/MR review, once the user is fully done with it. + +## Your Role + +This is the **only** phase that removes the worktree -- `/publish` and +`/continue` deliberately leave it in place so later `/continue` rounds can +reuse it. Run this when the user is done reviewing a given PR/MR (no more +`/continue` rounds expected), or to abandon an in-progress review. + +## Critical Rules + +- **Only delete artifacts and the worktree.** Never modify or delete + anything in the reviewed repository's actual history -- only the local + disposable worktree/branch/scratch-clone this workflow created. +- **Confirm before deleting.** Show the user what will be removed and wait + for confirmation. +- **Context-scoped.** Only clean artifacts for the specified (or, if + unambiguous, current) PR/MR context unless the user asks to clean all + `pr-review` artifacts. + +## Process + +### Step 1: Identify the Context + +If the user specified a PR/MR URL or context, resolve `{context}` the same +way as `start.md` Step 1. Otherwise, if exactly one +`.artifacts/pr-review/{context}/` directory exists, use that. If multiple +exist and the user didn't specify which, list them and ask. + +### Step 2: Check for Artifacts + +Check if `.artifacts/pr-review/{context}/` exists. If it does not, tell the +user there is nothing to clean for this PR/MR. + +### Step 3: Read Worktree Metadata + +Read `.artifacts/pr-review/{context}/review-metadata.json` (if present) for +`base_repo`, `base_repo_is_scratch`, and `context` -- needed to know exactly +what to tear down. If metadata is missing (e.g., an interrupted `/start`), +infer what exists by checking for +`.artifacts/pr-review/{context}/worktree/` and +`.artifacts/pr-review/{context}/_scratch-repo/` directly. + +### Step 4: Show What Will Be Removed + +```bash +git -C .artifacts/pr-review/{context}/worktree status 2>/dev/null +ls -la .artifacts/pr-review/{context}/ +``` + +Present the list to the user: + +```markdown +## PR review artifacts to remove + +Context: {context} + +| Item | Description | +|------|-------------| +| worktree/ | Git worktree checked out at the PR/MR head | +| pr-review/{context} branch | Local branch backing the worktree (in {base-repo}) | +| _scratch-repo/ | Scratch clone of the target repo (only if one was created) | +| 00-reviewer-profile.md | Reviewer profile | +| 01-pr-context.md | PR/MR context summary | +| 02-draft-review-*.md | Draft review(s) | +| decisions-*.json | Round decisions | +| publish-metadata.json | Record of the posted review (if published) | +| review-metadata.json | Review state | + +Confirm removal? (This cannot be undone. Anything already posted to the +host is unaffected -- this only removes local files.) +``` + +### Step 5: Remove the Worktree and Branch + +After user confirmation: + +```bash +git -C {base-repo} worktree remove --force .artifacts/pr-review/{context}/worktree +``` + +If `base_repo_is_scratch` is `false` (the base repo was reused, not +scratch-cloned), also drop the local tracking branch: + +```bash +git -C {base-repo} branch -D pr-review/{context} +``` + +If `base_repo_is_scratch` is `true`, the branch lives inside the scratch +clone that Step 6 removes entirely -- no separate branch-delete needed. + +If `git worktree remove --force` fails (e.g., already removed manually), +note it and continue with cleanup rather than aborting. + +### Step 6: Remove the Scratch Clone (if any) + +Only if `base_repo_is_scratch` is `true`: + +```bash +rm -rf .artifacts/pr-review/{context}/_scratch-repo +``` + +### Step 7: Remove Artifacts + +```bash +rm -rf .artifacts/pr-review/{context} +``` + +Clean up any empty parent directories left behind: + +```bash +find .artifacts/pr-review -type d -empty -delete 2>/dev/null +``` + +Tell the user the worktree and artifacts have been removed. Remind them +that anything already posted to the host (comments, discussions) is +unaffected -- this only removes local files. + +## Output + +- Removed `.artifacts/pr-review/{context}/worktree/` (git worktree) +- Removed local `pr-review/{context}` branch (if `{base-repo}` was reused) +- Removed `.artifacts/pr-review/{context}/_scratch-repo/` (if one was + created) +- Removed `.artifacts/pr-review/{context}/` directory + +## When This Phase Is Done + +Report what was cleaned up. + +Then **re-read the controller** (`controller.md`) for next-step guidance. diff --git a/pr-review/skills/continue.md b/pr-review/skills/continue.md new file mode 100644 index 0000000..4a8e9d7 --- /dev/null +++ b/pr-review/skills/continue.md @@ -0,0 +1,180 @@ +--- +name: continue +description: After the PR/MR receives new commits, refresh the existing worktree in place and draft a review of what's new. +--- + +# Continue PR Review Skill + +You are re-reviewing a PR/MR that already had a review posted, after it +received new commits. Your job is to refresh the worktree in place, figure +out what's actually new, check whether previously posted comments look +addressed, and draft comments only for the net-new material. + +## Your Role + +Pick up where `/publish` left off: same worktree, same artifact directory, +new commits to look at. Produce a new draft round that feeds back into the +same `/revise` -> `/publish` loop as `/start` did. + +## Critical Rules + +- **Read-only against the reviewed repository.** Same as `/start` and + `/revise` -- refreshing the worktree (`fetch` + `reset --hard`) is the + only "mutation," and it only touches the local disposable worktree, never + the reviewed repo's actual history. +- **Don't re-propose what's already an open thread.** Only draft comments + for genuinely new findings; existing threads on the host are already + visible there. +- **Same validation and style rules as `/start`.** Every new finding still + needs a real file/line inside the current diff, a permalink, a snippet, + and (when mechanical) a suggestion block, rendered in the resolved + comment style. +- **No posting here.** This phase only produces a new draft; `/publish` + posts it, after another round of local approval. + +## Process + +### Step 1: Read Prior State + +Read `.artifacts/pr-review/{context}/review-metadata.json` and +`publish-metadata.json`. If `publish-metadata.json` doesn't exist, `/publish` +was never run (or was interrupted) -- tell the user and suggest resolving +that first (`/revise` then `/publish`) rather than running `/continue`. + +### Step 2: Check for New Commits + +Fetch current PR/MR metadata with the provider's command (same as +`start.md` Step 1) to get the current head SHA: + +```bash +# github +gh pr view {number} --repo {owner}/{repo} --json headRefName,commits,state + +# gitlab +glab mr view {number} --repo {namespace}/{project} -F json +``` + +Compare the current head SHA to `head_sha_reviewed` from +`publish-metadata.json`. If they match, tell the user there's nothing new +to review and stop here. + +### Step 3: Refresh the Worktree + +If `.artifacts/pr-review/{context}/worktree` still exists (the normal case, +since only `/clean` removes it), refresh it in place using exactly +`start.md` Step 2's refresh form (fetch the head ref, fast-forward the +local branch, then `reset --hard` the worktree to it). + +If the worktree was somehow removed outside of `/clean` (e.g., manually +deleted), fall through to `start.md` Step 2's full setup procedure instead +(check for a reusable local repo or scratch-clone, then create the +worktree fresh) before proceeding. + +Recompute the merge base and new head SHA: + +```bash +git -C {base-repo} merge-base origin/{base_ref_name} pr-review/{context} +git -C {base-repo} rev-parse pr-review/{context} +``` + +### Step 4: Determine What's New + +Two diffs matter here: + +- **Full PR/MR diff** (for context): `git -C {worktree} diff {new-merge-base-sha} HEAD` +- **Incremental diff** (what's actually new since the last review): + `git -C {worktree} diff {head_sha_reviewed} HEAD` + +Also re-check previously posted comments against the current code, using +the provider's listing command (same as `start.md` Step 3): + +```bash +# github +gh api repos/{owner}/{repo}/pulls/{number}/comments --paginate + +# gitlab (project ID is the URL-encoded "namespace/project" path) +glab api "projects/{namespace}%2F{project}/merge_requests/{number}/discussions" --paginate +``` + +For each previously posted comment, note whether it looks **addressed** +(the flagged code changed in a way that resolves the concern), **still +open** (the code is unchanged or the concern remains), or has a reply from +the author worth factoring in. + +### Step 5: Update PR Context + +Refresh `.artifacts/pr-review/{context}/01-pr-context.md`'s commit +narrative and existing-discussion sections to reflect the new commits and +any new discussion, following the same format as `start.md` Step 3. + +### Step 6: Obtain the Incremental Review + +Follow the same subagent pattern as `start.md` Step 6, scoped to the +**incremental diff**, but give the reviewer `00-reviewer-profile.md`, the +full diff, and `01-pr-context.md` too for context. If a subagent was used +originally (`reviewer_agent_id` in metadata) and the runtime supports +resuming it, do so -- this gives it memory of the previous round's findings +and previously posted comments. Otherwise spawn fresh, loaded with +`00-reviewer-profile.md` and the previous round's `02-draft-review-{NNN}.md` +files so it doesn't re-flag what's already posted, and record the new +`reviewer_agent_id` (see Step 8). + +Ask the reviewer to also flag, from the previously-open-comments list in +Step 4, any that the new commits clearly resolved (for the user's +awareness in the presentation -- this workflow doesn't reply to or resolve +threads itself, since that's outside its scope). + +Extend every new finding with a permalink, snippet, and suggestion block +exactly as `start.md` Step 6 does, anchored to the new head SHA. Validate +and assess exactly as `start.md` Step 7 does. + +### Step 7: Draft the Incremental Review + +Resolve the comment style the same way as `start.md` Step 8. Increment the +round number from `review-metadata.json`. Write +`.artifacts/pr-review/{context}/02-draft-review-{NNN}.md` in the same +format as `start.md` Step 8, containing only the net-new candidate +comments -- do not re-include comments from the previous round that are +already posted and open on the host. + +### Step 8: Update Metadata + +Update `.artifacts/pr-review/{context}/review-metadata.json`: bump +`iteration`, `head_sha`, `merge_base_sha`, `last_updated`, and set `state` +to `awaiting_decision`. If Step 6 spawned a fresh subagent rather than +resuming the original one, overwrite `reviewer_agent_id` with its new ID so +a later `/continue` round can resume it in turn. + +### Step 9: Present + +Present the same way as `start.md` Step 9, plus a short summary of the +previously-posted-comment status from Step 4 (addressed / still open / +has a reply): + +```markdown +## Since the Last Review +- {N} new commit(s) since the last posted review +- Previously posted comments: {N} look addressed, {N} still open, {N} have a reply worth noting +``` + +Persist decisions to `decisions-{NNN}.json` (matching the incremented +round), same schema as `start.md`/`revise.md`. + +If there are no net-new candidate comments after assessment, tell the user +the new commits look fine and no additional comments are proposed -- +there's nothing to `/publish` this round. + +## Output + +- Updated `.artifacts/pr-review/{context}/worktree/` (refreshed in place) +- Updated `01-pr-context.md` +- `.artifacts/pr-review/{context}/02-draft-review-{NNN}.md` +- `.artifacts/pr-review/{context}/decisions-{NNN}.json` +- Updated `review-metadata.json` + +## When This Phase Is Done + +Present the incremental draft (or "nothing new to propose") and the +previously-posted-comment status to the user. + +Then **re-read the controller** (`controller.md`) for next-step guidance. diff --git a/pr-review/skills/controller.md b/pr-review/skills/controller.md new file mode 100644 index 0000000..014e38d --- /dev/null +++ b/pr-review/skills/controller.md @@ -0,0 +1,158 @@ +--- +name: controller +description: Top-level workflow controller that manages phase transitions for reviewing a remote PR/MR. +--- + +# PR Review Workflow Controller + +You are the workflow controller. Your job is to manage the PR review +workflow by executing phases and handling transitions between them. "PR" +means "PR or MR" throughout — see `../guidelines.md` for the GitHub/GitLab +terminology note. + +## Phases + +1. **Start** (`/start`) -- `start.md` + Parse the PR URL, detect the provider, check out the PR into a git + worktree, gather PR context, build a reviewer profile, run the initial + review, and present a draft review for local approval-to-post. + +2. **Revise** (`/revise`) -- `revise.md` + Answer the local user's questions about the draft, apply requested edits, + add user-authored findings, and re-present. Repeatable until the user + approves posting. + +3. **Publish** (`/publish`) -- `publish.md` + Post the approved draft as a review with inline comments on the host. + Never removes the worktree. + +4. **Continue** (`/continue`) -- `continue.md` + After the PR/MR receives new commits, refresh the existing worktree in + place and run an incremental review of what's new. Feeds back into + `/revise` -> `/publish`. + +5. **Clean** (`/clean`) -- `clean.md` + Remove the worktree and all review artifacts. This is the only phase + that tears down the worktree — run it once the user is done with a PR. + +## Workspace + +All artifacts and the worktree live in `.artifacts/pr-review/{context}/` +(gitignored), where `{context}` is a sanitized +`{owner-or-namespace}-{repo-or-project}-{number}`. The worktree persists +across `/start` -> `/revise` -> `/publish` -> `/continue` rounds; it is +removed only by `/clean` (see `start.md`'s worktree setup procedure and +`clean.md`). + +### Artifact Directory + +| Artifact | File | Written by | +|----------|------|------------| +| Reviewer profile | `00-reviewer-profile.md` | `/start` | +| PR context | `01-pr-context.md` | `/start` (refreshed by `/continue`) | +| Review metadata | `review-metadata.json` | `/start`, `/continue` | +| Draft review (round N) | `02-draft-review-{NNN}.md` | `/start`, `/revise`, `/continue` | +| Decisions (round N) | `decisions-{NNN}.json` | `/start`, `/revise` | +| Publish metadata | `publish-metadata.json` | `/publish` | +| Git worktree | `worktree/` | `/start` (created), `/continue` (refreshed), `/clean` (removed) | +| Scratch clone (if needed) | `_scratch-repo/` | `/start` (created only if no local clone could be reused), `/clean` (removed) | + +## How to Execute a Phase + +1. **Announce** the phase to the user: *"Starting /start."* +2. **Locate** the skill file — read and follow + `../../_shared/recipes/phase-override-resolution.md` with + WORKFLOW=`pr-review`, PHASE_FILE=`{phase}.md`. +3. **Read** the resolved skill file. +4. **Execute** the skill's steps -- the user should see your progress. +5. When the skill is done, it will tell you to report results and re-read + this controller. Do that -- then use "Recommending Next Steps" below to + offer options. +6. Present the skill's results and your recommendations to the user. +7. **Stop and wait** for the user to tell you what to do next. + +## Recommending Next Steps + +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 + +```text +start --> local approval-to-post decisions --> [revise loop] --> publish +publish --> (later, on new commits) --> continue --> [revise loop] --> publish +--> ... --> clean (once done with this PR) +``` + +### What to Recommend + +- `/start` completed --> recommend the user review the draft and decide + which comments to keep, drop, or edit, ask any questions, or add new + findings. Once satisfied, `/publish` to post. +- `/revise` completed --> same as above: review the updated draft, iterate + again with `/revise`, or `/publish` once approved. +- `/publish` completed --> workflow is done for this round. The worktree + stays in place. If the PR/MR later receives new commits, recommend + `/continue`. If the user is fully done with this PR, recommend `/clean`. +- `/continue` completed (no new commits found) --> nothing to do; suggest + checking back later or running `/clean` if the review is finished. +- `/continue` completed (new draft produced) --> same as `/start` completed: + review, `/revise` if needed, then `/publish`. + +### How to Present Options + +Lead with your top recommendation, then list alternatives briefly: + +```text +Recommended next step: /publish -- post the approved draft as a review. + +Other options: +- /revise -- ask a question or request a change to one of the comments +- /clean -- abandon this review and remove the worktree + artifacts +``` + +## Starting the Workflow + +When the user runs `/start` with a PR/MR URL and artifacts already exist for +that PR's context, warn that a review is already in progress and ask +whether to resume (`/revise` or `/publish`, depending on state) or restart. + +When invoked without a specific command (e.g., just "review this PR: +{url}"), treat it as `/start` with that URL -- including the existing-review +check above if artifacts already exist for the resolved context. + +## Error Handling + +If any phase fails: + +1. **Stop immediately.** Do not advance to the next phase. +2. **Report the error** to the user with the specific error message. +3. **Offer options:** retry the failed step, skip the phase (if optional), + or escalate. + +Do not fabricate results when a tool call fails. Do not silently continue +past errors -- see `../guidelines.md`'s Escalation section. + +## Context Management + +**Reviewer subagents** (`start.md`'s "Obtain the Review" step, and its +equivalent in `continue.md`): the review is performed by a subagent when the +AI runtime supports it, to keep the reviewer's read of the diff independent +of the controller's own running context. This mirrors `code-review`'s +reviewer-subagent pattern. Not all AI runtimes support subagent spawning -- +this is a recommendation, not a requirement. + +## Rules + +- **Never auto-advance.** Always wait for the user between phases -- there + is no unattended mode in this workflow (every posted comment requires + local approval-to-post). +- **Recommendations come from this file, not from skills.** Skills report + results; this controller decides what to recommend next. +- **No code changes in any phase.** This workflow never edits the reviewed + repository -- see `../guidelines.md`'s hard limits. +- **The worktree is removed only by `/clean`.** No other phase deletes it, + regardless of outcome. +- **The user is the decision-maker.** No comment is posted, and no draft is + finalized, without the local user's explicit approval-to-post. diff --git a/pr-review/skills/publish.md b/pr-review/skills/publish.md new file mode 100644 index 0000000..cb2c30e --- /dev/null +++ b/pr-review/skills/publish.md @@ -0,0 +1,188 @@ +--- +name: publish +description: Post the approved draft as a review with inline comments on the host. Never removes the worktree. +--- + +# Publish PR Review Skill + +You are posting an already-approved draft review to the host. Your job is +to turn the approved draft into the host's native review/comment objects, +post them, and record what was posted -- nothing more. + +## Your Role + +Translate `02-draft-review-{NNN}.md`'s kept comments into the provider's +posting mechanics (see `../guidelines.md`'s terminology note and the table +below), post them, and report the result. The worktree is left exactly as +it is. + +## Critical Rules + +- **Precondition: local approval-to-post.** Only run this after the user + has explicitly approved the current draft's exact content (from `/start` + or `/revise`) -- never a host-level PR/MR approval. If there is no clear + approval in the conversation, stop and ask before posting anything. +- **Post only what's in the approved draft.** The posted comment bodies are + exactly the "Comment (as it will be posted)" text from the draft -- + never the internal severity/category/assessment notes. +- **Review summary is always fixed.** `See comments below`, verbatim, + regardless of comment count or severity. +- **Never a formal approve/request-changes action.** GitHub reviews always + use `event: COMMENT`. GitLab: never call the approve/unapprove endpoint. +- **No silent partial-post.** If the host rejects any comment, report + exactly which ones and why, and ask the user how to proceed -- don't + invent a fallback (e.g., posting as a generic top-level comment instead) + unless the user asks for that. +- **The worktree is not touched here.** Do not remove it, do not modify it + beyond what Step 2 needs to re-verify line anchors. + +## Process + +### Step 1: Read Context + +Read `.artifacts/pr-review/{context}/review-metadata.json` for `provider`, +`owner_or_namespace`, `repo_or_project`, `number`, `head_sha`, +`merge_base_sha`, and the worktree location, plus the latest draft +(`02-draft-review-{NNN}.md`, per the current `iteration`) for the kept +comments to post. For GitLab, derive `{namespace}` and `{project}` from +`owner_or_namespace`/`repo_or_project`. + +If `review-metadata.json` is missing, or `state` isn't `awaiting_decision` +or a later approved state, stop and tell the user there's no approved draft +to publish yet -- run `/start` or `/revise` first. + +### Step 2: Re-Verify Line Anchors + +The PR/MR may have changed since the draft was written (a `/revise` round +can span time). Refresh what's needed and re-check that every kept comment +still anchors to a line inside the current diff: + +```bash +git -C {worktree} diff {merge-base-sha} HEAD --name-status +``` + +If a comment's file or line no longer exists in the diff, do not post it +silently and do not silently drop it either -- tell the user which +comment(s) are affected and ask whether to drop, relocate, or abort. + +### Step 3: Build the Payload + +Write the payload to a temp file first (same pattern as other workflows' +`gh api`/`glab api` calls in this repo) to avoid shell-escaping issues, then +delete the temp file after posting. + +**GitHub** -- one batched review object, +`.artifacts/pr-review/{context}/tmp-review-payload.json`: + +```json +{ + "commit_id": "{head-sha}", + "event": "COMMENT", + "body": "See comments below", + "comments": [ + {"path": "{path}", "line": {end-line}, "side": "RIGHT", "body": "{comment text}"} + ] +} +``` + +For a multi-line comment, add `"start_line": {start-line}, "start_side": "RIGHT"` alongside `"line"` (the end line). + +Post it: + +```bash +gh api repos/{owner}/{repo}/pulls/{number}/reviews --method POST --input .artifacts/pr-review/{context}/tmp-review-payload.json +``` + +**GitLab** -- no batched review object exists. First fetch the diff refs +needed to anchor each discussion (base/start/head SHAs from the MR's own +diff, not just the worktree's). The project ID is the URL-encoded +"namespace/project" path, which GitLab's REST API accepts directly in +place of the numeric ID: + +```bash +glab api "projects/{namespace}%2F{project}/merge_requests/{number}" | jq '.diff_refs' +``` + +Then, for each kept comment, POST one discussion, +`.artifacts/pr-review/{context}/tmp-discussion-{n}.json`: + +```json +{ + "body": "{comment text}", + "position": { + "position_type": "text", + "base_sha": "{diff_refs.base_sha}", + "start_sha": "{diff_refs.start_sha}", + "head_sha": "{diff_refs.head_sha}", + "new_path": "{path}", + "new_line": {line} + } +} +``` + +```bash +glab api "projects/{namespace}%2F{project}/merge_requests/{number}/discussions" --method POST --input .artifacts/pr-review/{context}/tmp-discussion-{n}.json +``` + +Then post one separate top-level note carrying the fixed summary: + +```bash +glab mr note {number} --repo {namespace}/{project} --message "See comments below" +``` + +Only the rendered posted text goes in any `body` field -- no internal +severity/category metadata, in either provider's payload. + +### Step 4: Handle Rejections + +If the host rejects a comment (e.g., a line fell outside the diff, a +permissions error, a malformed position object), do not retry with altered +content and do not skip it silently. Report: + +- Which comment(s) failed +- The host's error message +- Whether any comments were successfully posted before the failure + +Then ask the user how to proceed (fix and retry, drop the failed ones and +post the rest, or abort). Do not invent a resolution on their behalf. + +### Step 5: Write Publish Metadata + +On success (all kept comments posted, or the user explicitly accepted a +partial post), write `.artifacts/pr-review/{context}/publish-metadata.json`: + +```json +{ + "posted_at": "{ISO 8601 timestamp}", + "head_sha_reviewed": "{head-sha}", + "comments_posted": {N}, + "review_id": "{GitHub review id, if provider = github}", + "discussion_ids": ["{GitLab discussion id, ...}", "..."], + "review_url": "{URL to view the posted review/discussions}" +} +``` + +Update `review-metadata.json`'s `state` to `published`. + +Clean up any temp payload files created in Step 3. + +### Step 6: Report + +Tell the user: + +- How many comments were posted, and the review URL +- That the worktree is still in place (not removed) -- `/continue` can + reuse it later if the PR/MR receives new commits; run `/clean` once + they're fully done with this PR. + +## Output + +- Review/discussions posted to the host +- `.artifacts/pr-review/{context}/publish-metadata.json` +- Updated `review-metadata.json` + +## When This Phase Is Done + +Report the posted review URL and comment count. + +Then **re-read the controller** (`controller.md`) for next-step guidance. diff --git a/pr-review/skills/revise.md b/pr-review/skills/revise.md new file mode 100644 index 0000000..140cb59 --- /dev/null +++ b/pr-review/skills/revise.md @@ -0,0 +1,132 @@ +--- +name: revise +description: Answer questions about the draft review, apply requested edits, add user-authored findings, and re-present until the user approves posting. +--- + +# Revise PR Review Skill + +You are addressing the local user's feedback on a draft PR/MR review before +it gets posted. Your job is to answer their questions, apply the edits they +asked for, incorporate any findings they want added, and re-present an +updated draft -- repeatable until they approve posting. + +## Your Role + +Treat the current draft as a living document that isn't posted yet. Nothing +here is sent to the host; this phase only produces a new, better draft. + +## Critical Rules + +- **Read-only against the reviewed repository.** Like `/start`, this phase + may re-inspect `{worktree}` for more context when answering a question or + validating a user-authored finding, but never edits anything in it. +- **No posting.** Only `/publish` posts, and only on a separate, + fully-approved run. +- **Every edit still honors the resolved comment style.** Reworded comments + and user-authored findings must still follow `../templates/comment-style.md` + (or the project's override) -- suggestive tone, no severity labels, no + "Finding N" headers in posted text. +- **User-authored findings are validated like automated ones.** A finding + the user asks to add must still cite a real file and a real line inside + the PR/MR diff before it becomes a draft comment -- see + `../../_shared/review-protocol.md`'s validation rules. +- **Nothing is dropped silently.** Every item from the previous round -- + kept, dropped, or edited -- must be accounted for in the new draft or + explicitly noted as removed. + +## Process + +### Step 1: Read Context + +Read `.artifacts/pr-review/{context}/review-metadata.json` for the current +iteration and worktree location, the latest draft +(`02-draft-review-{NNN}.md`), and the matching decisions file +(`decisions-{NNN}.json`). + +If the user's requested changes for this round aren't already clear from +the conversation, ask them to state their decisions on the current draft +first (which comments to keep/drop/edit, any questions, any new findings to +add). + +### Step 2: Answer Questions + +For each question in `decisions-{NNN}.json` (or raised directly in +conversation) about a specific proposed comment: + +1. Re-inspect `{worktree}` as needed (read more surrounding context, check + git history in the worktree, etc.) to give a grounded answer. +2. Answer the question directly and concretely. +3. If the answer reveals the original finding was wrong or overstated, + adjust the comment's wording or drop it -- don't leave a comment + standing that your own answer just undermined. + +### Step 3: Apply Requested Edits + +For each comment the user asked to reword, soften, sharpen, or otherwise +change (including changing or removing its suggestion block): rewrite it, +re-checking it still follows the resolved comment style. For each comment +the user asked to drop: remove it from the draft. + +### Step 4: Add User-Authored Findings + +For each new finding the user describes (a file/line and a concern they +want raised, with or without a suggested fix): + +1. Read the cited location in `{worktree}` to confirm it exists and falls + inside the PR/MR diff (`git -C {worktree} diff {merge-base-sha} HEAD`). + If it doesn't -- wrong file, line outside the diff, line doesn't exist + -- tell the user and ask for a corrected location rather than fabricating + one. +2. Build the permalink and quote the snippet exactly as `/start` Step 7 + does. +3. Draft the comment in the resolved comment style, including a suggestion + block only if the user's request implies a concrete mechanical fix. +4. Add it to the draft with its own internal note (category and rationale + the user gave, or your own assessment if they didn't specify one). + +### Step 5: Re-Render the Draft + +Increment the round number. Write +`.artifacts/pr-review/{context}/02-draft-review-{NNN}.md` (matching the +format from `start.md` Step 8) reflecting every kept, edited, and +newly-added comment. Cross-check against the previous round's draft: every +prior comment must appear here as kept (possibly edited) or be accounted +for as dropped -- nothing should silently vanish. + +### Step 6: Update Metadata + +Update `.artifacts/pr-review/{context}/review-metadata.json`: bump +`iteration` to the new round number, update `last_updated`, and set `state` +to `awaiting_decision`. + +### Step 7: Present the Updated Draft + +Present the same way as `start.md` Step 9 (PR context recap, decision +table including any items still "Disagree"/dropped for transparency, full +comment blocks for every kept comment) plus: + +```markdown +## What Changed This Round +{brief list: comments dropped, comments reworded, comments added, and +questions answered} +``` + +Prompt the user the same way as `start.md` Step 9. Persist their new +decisions to `.artifacts/pr-review/{context}/decisions-{NNN}.json` (matching +the incremented round number). + +If the user approves posting as-is, tell them to run `/publish`. If they +have further changes, they can run `/revise` again -- there is no round +limit; this loop continues until the local user is satisfied. + +## Output + +- `.artifacts/pr-review/{context}/02-draft-review-{NNN}.md` +- `.artifacts/pr-review/{context}/decisions-{NNN}.json` +- Updated `review-metadata.json` + +## When This Phase Is Done + +Present the updated draft and decision table to the user. + +Then **re-read the controller** (`controller.md`) for next-step guidance. diff --git a/pr-review/skills/start.md b/pr-review/skills/start.md new file mode 100644 index 0000000..6572fc9 --- /dev/null +++ b/pr-review/skills/start.md @@ -0,0 +1,447 @@ +--- +name: start +description: Parse the PR/MR URL, check it out into a worktree, gather context, run the initial review, and present a draft for local approval-to-post. +--- + +# Start PR Review Skill + +You are the orchestrator of a remote PR/MR review. Your job is to resolve +the PR/MR from its URL, check it out into a disposable-until-`/clean` git +worktree, gather enough context to explain the PR before critiquing it, +obtain a structured review, and present a draft review for the local user +to approve, adjust, or question before anything is posted. + +"PR" means "PR or MR" throughout -- see `../guidelines.md`. + +## Your Role + +Detect the host, set up the worktree, build a reviewer profile from the +target project's own conventions, obtain a review extended with permalinks, +snippets, and suggestion blocks, independently assess each finding, and +draft a review in the resolved comment style -- then present it for local +approval-to-post. + +## Critical Rules + +- **Read-only against the reviewed repository.** This phase never edits + files in the worktree and never pushes anywhere. The only git operations + are `clone`, `fetch`, `worktree add`, `diff`, `merge-base`, and `log`. +- **No posting.** This phase only drafts. Posting happens in `/publish`, + and only after explicit local approval-to-post. +- **Context before critique.** `01-pr-context.md` is always produced and + always shown to the user before any findings. +- **Every finding must cite a specific file and a specific line inside the + PR/MR diff.** Discard any finding that cannot be traced to the actual + diff, or that anchors to a line outside the diff (the host cannot place + an inline comment there). +- **Assess independently.** After obtaining the review, form your own + opinion on each finding's value before presenting it. +- **Optional user focus.** If the user provided focus guidance alongside + the URL (e.g., "focus on error handling"), apply it, but still surface + CRITICAL/HIGH findings in other categories. + +## Process + +### Step 1: Parse the URL, Detect the Provider, and Check for an Existing Review + +Accept a GitHub PR URL (`https://github.com/{owner}/{repo}/pull/{number}`, +or the shorthand `{owner}/{repo}#{number}`) or a GitLab MR URL +(`https://{host}/{namespace}/{project}/-/merge_requests/{number}`, where +`{namespace}` may contain nested subgroups). Detect the provider from the +input's shape, not from a hardcoded host list: + +- No `://` and it matches `{owner}/{repo}#{number}` (a single `/` before + the `#`) -> **provider = github**, CLI = `gh`. Split on `#` for + `{number}`, then on `/` for `{owner}`/`{repo}`. +- Path contains `/pull/{n}` -> **provider = github**, CLI = `gh`. +- Path contains `/-/merge_requests/{n}` -> **provider = gitlab**, CLI = + `glab`. Works for `gitlab.com` and self-hosted GitLab instances alike. + +If the input matches none of these shapes, stop and ask the user for a +valid PR/MR URL. + +Verify the matching CLI is authenticated: + +```bash +gh auth status # if provider = github +glab auth status # if provider = gitlab +``` + +If authentication fails, stop and report it -- do not proceed. + +Fetch PR/MR metadata with the provider's command: + +```bash +# github +gh pr view {number} --repo {owner}/{repo} --json title,body,author,baseRefName,headRefName,headRepositoryOwner,url,commits,additions,deletions,changedFiles,state,comments,reviews + +# gitlab +glab mr view {number} --repo {namespace}/{project} -F json +``` + +If the PR/MR cannot be fetched (private without access, deleted, wrong +number), stop and report the error. + +Set `{context}` to a sanitized `{owner-or-namespace}-{repo-or-project}-{number}` +(lowercase, `/` replaced with `-`). Use this for all artifact paths under +`.artifacts/pr-review/{context}/`. + +If `.artifacts/pr-review/{context}/review-metadata.json` already exists, a +review is already in progress for this PR/MR. Stop and tell the user: + +- A review is already in progress for this PR/MR. +- They can run `/revise` or `/publish` to continue where it left off, or + confirm they want to restart. + +If the user confirms a restart, remove the existing worktree per +`../guidelines.md` (`git worktree remove --force`, drop the +`pr-review/{context}` branch or scratch clone as applicable -- same +commands as `clean.md`) and delete the artifact directory before +proceeding. + +### Step 2: Set Up the Worktree + +```bash +mkdir -p .artifacts/pr-review/{context} +``` + +Verify `.artifacts/` is covered by the project's `.gitignore` in the +**current** repository (where this workflow runs from); warn the user if +not. + +1. If `.artifacts/pr-review/{context}/worktree` already exists (recovering + from an interrupted session that wasn't cleaned), skip to step 4's + refresh form below instead of recreating it. +2. Otherwise, check whether the current directory is a git repo whose + remote matches the PR/MR's repo: + ```bash + git rev-parse --show-toplevel + git remote -v + ``` + If a remote matches (normalize both to compare, since one may be SSH and + the other HTTPS), use that toplevel path as `{base-repo}`. No clone + needed. +3. Otherwise, clone the target repo fresh: + ```bash + git clone {clone-url} .artifacts/pr-review/{context}/_scratch-repo + ``` + Use `.artifacts/pr-review/{context}/_scratch-repo` as `{base-repo}` and + record `base_repo_is_scratch: true` in metadata (Step 10) -- this tells + `/clean` to remove the scratch clone alongside the worktree. +4. Fetch the PR/MR head and base ref, then create the worktree. The ref + path is the one provider-specific detail here (both are plain `git + fetch`, no `gh`/`glab` involved): + ```bash + # github + git -C {base-repo} fetch origin "pull/{number}/head:refs/heads/pr-review/{context}" + # gitlab + git -C {base-repo} fetch origin "merge-requests/{number}/head:refs/heads/pr-review/{context}" + + git -C {base-repo} fetch origin {baseRefName} + git -C {base-repo} worktree add .artifacts/pr-review/{context}/worktree "pr-review/{context}" + ``` + **To refresh an existing worktree instead** (step 1's branch): re-run + the same `fetch origin "{ref}:refs/heads/pr-review/{context}"` command + (fast-forwards the local branch to the new head), then: + ```bash + git -C .artifacts/pr-review/{context}/worktree reset --hard pr-review/{context} + ``` +5. Compute the merge base and record `{head-sha}`: + ```bash + git -C {base-repo} merge-base origin/{baseRefName} pr-review/{context} + git -C {base-repo} rev-parse pr-review/{context} + ``` + The PR/MR diff is `git -C {worktree} diff {merge-base-sha} HEAD` + (equivalent to the host's shown diff). +6. If `git worktree add` fails for any reason other than "already checked + out in the main working tree": stop, report the error, and ask the user + to run `git worktree list` to inspect and `git worktree remove --force` + to clean up any stale entries before retrying. + +### Step 3: Gather PR Context (always shown before findings) + +Using the metadata from Step 1 plus the commit list and existing +discussion, write `.artifacts/pr-review/{context}/01-pr-context.md`: + +```markdown +# PR Context -- {title} + +## Overview +- **Author:** {author} +- **Base <-> Head:** {baseRefName} <- {headRefName} +- **URL:** {url} +- **State:** {open/closed/merged/draft} + +## Linked Issues / Tickets +{issue/ticket references found in the description, commit messages, or +branch name -- e.g., "Fixes #123", a Jira key, a GitLab issue link. If none +found: "None found."} + +## Commit Narrative +{brief summary of how the PR evolved across its commits -- not just a raw +list, but what each significant commit changed and why, inferred from +commit messages and diffs} + +## Key Design Decisions +{decisions inferred from the description and commits -- why this approach +over alternatives, trade-offs the author called out. If none evident: +"No explicit design rationale found in the description or commits."} + +## Existing Discussion +{summary of any comments/discussions/reviews already on the PR/MR -- list +existing comments per the provider commands below. If none: "No existing +discussion."} +``` + +List existing comments/discussions with the provider's command: + +```bash +# github +gh api repos/{owner}/{repo}/pulls/{number}/comments --paginate +gh api repos/{owner}/{repo}/pulls/{number}/reviews --paginate + +# gitlab (project ID is the URL-encoded "namespace/project" path -- +# GitLab's REST API accepts this directly in place of the numeric ID) +glab api "projects/{namespace}%2F{project}/merge_requests/{number}/discussions" --paginate +``` + +Present `01-pr-context.md` to the user before moving on -- this satisfies +the workflow's "context before critique" principle. Never skip this step. + +### Step 4: Build a Reviewer Profile + +Same discovery as `code-review`'s equivalent step, but read from +`{worktree}` (the target project), not the current project: + +1. `AGENTS.md` or `CLAUDE.md` in the worktree +2. `CONTRIBUTING.md` in the worktree +3. Linting configuration files +4. CI/CD workflows (`.github/workflows/`, `.gitlab-ci.yml`) +5. Test configuration + +Write `.artifacts/pr-review/{context}/00-reviewer-profile.md`: + +```markdown +# Reviewer Profile -- {project name} + +## Tech Stack +{languages, frameworks, key dependencies} + +## Conventions +{coding standards, naming patterns, project-specific rules} + +## Quality Gates +{lint command, test command, coverage requirements -- informational only; +this workflow never runs them, since it never changes code} + +## Review Focus Areas +{what this project's guidelines emphasize} + +## Sources +{list of files read to build this profile} +``` + +### Step 5: Analyze the Diff + +```bash +git -C {worktree} diff {merge-base-sha} HEAD --name-status +``` + +Unlike `code-review`, there is no relevance-filtering step -- every changed +file in the PR/MR diff is in scope by definition; this is someone else's +already-scoped change, not a mixed local working tree. + +### Step 6: Obtain the Review + +Follow the same subagent pattern as `code-review`'s equivalent step: + +**If the AI runtime supports subagents:** spawn a subagent loaded with the +reviewer profile, `01-pr-context.md`, the full diff +(`git -C {worktree} diff {merge-base-sha} HEAD`), the target project's +`AGENTS.md`/`CLAUDE.md`, and this workflow's `../guidelines.md`. + +**If subagents are not available:** re-read `../guidelines.md` to +calibrate, then review sequentially. Read full files in `{worktree}` around +changed sections, not just the diff in isolation -- the diff shows what +changed, the surrounding code reveals whether it fits. + +Evaluate all categories defined in `../../_shared/review-protocol.md`. For +each finding, capture (internally -- this is not the posted format yet): + +- File, location (line range), severity, category, issue, suggestion (per + `../../_shared/review-protocol.md`'s finding format) +- **Permalink** at `{head-sha}`: + - GitHub: `https://github.com/{owner}/{repo}/blob/{head-sha}/{path}#L{start}-L{end}` + - GitLab: `https://{host}/{namespace}/{project}/-/blob/{head-sha}/{path}#L{start}-{end}` +- **Snippet** -- the quoted code at that location, read from `{worktree}` +- **Suggested change** -- a concrete code block when the fix is a + mechanical replacement of the flagged lines, fenced per + `../templates/comment-style.md`'s provider-specific syntax (a bare + ` ```suggestion ` on GitHub; ` ```suggestion:-{lines_above}+{lines_below} ` + on GitLab); otherwise no suggestion block, just the explanation + +Only findings anchored to a line inside the diff (an added line or a line +within a diff hunk) are eligible -- the host can only anchor inline +comments there. Discard anything else the same way `_shared/review-protocol.md` +already requires discarding hallucinated references. + +### Step 7: Validate and Assess Findings + +Same two-part process as `code-review`: + +**7a: Validate.** Confirm every cited file and line actually exists in the +diff at that location. Discard silently (internal note only, not shown to +the user) anything that doesn't check out. + +**7b: Assess on value.** For each validated finding, form an honest +assessment per `_shared/review-protocol.md`'s "Assess on value, not +severity" principle: + +- **Agree** -- the finding adds real value worth surfacing as a draft + comment. +- **Disagree** -- it doesn't add value, or the code is fine as-is. State + why concretely. +- **Partially agree** -- the issue is real but the suggestion could be + better; propose the improved version. + +Findings assessed "Agree" or "Partially agree" become candidate draft +comments in Step 8. "Disagree" findings are noted internally but are still +shown to the user in Step 9's table (transparency -- see below), just not +pre-selected as "keep." + +### Step 8: Draft the Review + +Resolve the comment style: read and follow `../templates/comment-style.md`'s +"Resolution" section (checks `{worktree}/.pr-review/templates/comment-style.md` +first, then the built-in default). Announce if a project override is used. + +For each candidate finding, render the comment exactly as it would be +posted, following the resolved style's tone and structure rules. + +Write `.artifacts/pr-review/{context}/02-draft-review-001.md`: + +```markdown +# Draft PR Review -- Round 1 + +## PR Context +{brief recap -- title, author, one-line summary of what the PR does, from +01-pr-context.md} + +## Review Summary (posted to the host) +See comments below + +## Proposed Inline Comments + +### Comment 1 -- {file}:{line-range} +- **Link:** {permalink} +- **Snippet:** + ```{lang} + {quoted code} + ``` +- **Comment (as it will be posted):** + + {rendered comment text, suggestive tone} + + {optional fenced suggestion block} +- **Internal note (not posted):** {SEVERITY} / {CATEGORY} -- {Agree|Disagree|Partially agree}: {rationale} + +### Comment 2 -- ... +``` + +### Step 9: Present for Local Approval-to-Post + +Show, in this order: + +1. `01-pr-context.md`'s content (or a faithful summary of it) -- context + always comes first. +2. A compact table of every candidate, including "Disagree" ones (full + transparency, same as `code-review`): + +```markdown +| # | File:Line | Severity | Category | Finding | Assessment | Recommendation | +|---|-----------|----------|----------|---------|-------------|-----------------| +| 1 | foo.py:42 | HIGH | Correctness | {short description} | Agree -- {rationale} | Keep | +| 2 | bar.go:10 | LOW | Naming | {short description} | Disagree -- {rationale} | Drop | +``` + +3. The full comment blocks from `02-draft-review-001.md` for every + candidate recommended "Keep" (link, snippet, exact posted text, + suggestion block). + +Then prompt: + +```markdown +Review the draft above and let me know your decisions. You can: +- Approve posting as-is +- Keep/drop/edit specific comments (e.g., "drop #2, reword #1") +- Ask a question about any comment before deciding +- Add a comment of your own on a specific file/line + +Run /revise to apply any changes and see an updated draft, or /publish +once you approve posting as-is. +``` + +This local approval-to-post is never a host-level PR/MR approval -- see +`../guidelines.md`. + +Persist decisions to `.artifacts/pr-review/{context}/decisions-001.json`: + +```json +{ + "round": 1, + "decisions": [ + {"comment": 1, "decision": "keep", "guidance": null}, + {"comment": 2, "decision": "drop", "reason": "user rationale"} + ], + "questions": [ + {"comment": 1, "question": "user's question text", "answer": null} + ], + "additions_requested": [ + {"file": "{path}", "line": {N}, "guidance": "concern to raise, and an optional suggested fix, in the user's own words"} + ] +} +``` + +`questions[].answer` starts `null` and is filled in by `/revise` Step 2 once +answered. `additions_requested[]` items become new draft comments via +`/revise` Step 4 -- leave both arrays empty (`[]`) when there are none. + +### Step 10: Write Review Metadata + +Write `.artifacts/pr-review/{context}/review-metadata.json`: + +```json +{ + "provider": "{provider}", + "owner_or_namespace": "{owner-or-namespace}", + "repo_or_project": "{repo-or-project}", + "number": {number}, + "context": "{context}", + "base_repo": "{base-repo path}", + "base_repo_is_scratch": false, + "base_ref_name": "{baseRefName}", + "head_sha": "{head-sha}", + "merge_base_sha": "{merge-base-sha}", + "iteration": 1, + "state": "awaiting_decision", + "started": "{ISO 8601 timestamp}", + "last_updated": "{ISO 8601 timestamp}", + "reviewer_agent_id": "{agent ID if a subagent was spawned, null otherwise}" +} +``` + +## Output + +- `.artifacts/pr-review/{context}/00-reviewer-profile.md` +- `.artifacts/pr-review/{context}/01-pr-context.md` +- `.artifacts/pr-review/{context}/02-draft-review-001.md` +- `.artifacts/pr-review/{context}/decisions-001.json` +- `.artifacts/pr-review/{context}/review-metadata.json` +- `.artifacts/pr-review/{context}/worktree/` (git worktree, left in place) + +## When This Phase Is Done + +Present the PR context and the draft decision table to the user, along with +your recommendations. + +Then **re-read the controller** (`controller.md`) for next-step guidance. diff --git a/pr-review/templates/comment-style.md b/pr-review/templates/comment-style.md new file mode 100644 index 0000000..bdc01c3 --- /dev/null +++ b/pr-review/templates/comment-style.md @@ -0,0 +1,105 @@ +# Comment Style (Default) + +Default tone and structure rules for every comment this workflow drafts and +posts. `skills/start.md`, `skills/revise.md`, and `skills/publish.md` all +resolve and apply this file (or a project override — see "Resolution" +below) when rendering the text that will actually be posted to the PR/MR. + +## Resolution + +Check for a project-level override before falling back to this built-in +default. Use the first match found: + +1. **`{worktree}/.pr-review/templates/comment-style.md`** — the reviewed + repository's own preference, checked into its own repo (analogous to how + the `prd` workflow's own template can be overridden by a project). Since + this workflow comments on a project it doesn't own, the project's own + conventions take priority over this default. +2. **This file** — the workflow's built-in default. + +If a candidate override is missing, unreadable, or empty, warn the user and +fall back to this built-in default. If using a project override, announce +it: *"Using project override for comment style."* + +## Tone + +- **Suggestive, not directive.** Frame every comment as a question or an + option, never as an instruction. + - Use: "Should we add a nil check here?", "What if this used a map + instead?", "Would it make sense to extract this into a helper?" + - Avoid: "Add a nil check here.", "You must use a map instead.", "Please + extract this into a helper." +- **State the concern, then the option.** Lead with what could go wrong or + what could improve, phrased plainly, then offer the suggested direction as + a question — don't bury the concern inside a compound question. +- **No hedging filler.** Suggestive does not mean vague. State the specific + issue; only the framing of the fix is a question, not the existence of the + issue. + +## Structure + +- **No severity or category labels in posted content.** `CRITICAL`, + `HIGH`, `Correctness`, `Security`, etc. are internal-only labels used + while presenting the draft for local approval (see `skills/start.md` Step + 10) — they never appear in the text that gets posted to the PR/MR. +- **No "Finding N" headers in posted content.** Each posted comment is + anchored to its own line by the host's own UI; it doesn't need a numbered + heading to stand apart from other comments. +- **Self-contained.** Each posted comment includes the concern and (when + applicable) the suggested-change block — a reader should not need to + cross-reference another comment to understand this one. +- **Review summary is always fixed.** The top-level body of the posted + review (GitHub) or the separate summary note (GitLab) is always the + literal string `See comments below` — regardless of how many comments + there are, their severity, or their category. Never restate counts, + severities, or an overall verdict there. + +## Suggested-change blocks + +- Include a suggestion block only when the fix is a concrete, mechanical + replacement of the flagged lines (e.g., a rename, a null check, a + corrected condition). +- Omit the block entirely for conceptual or design-level findings (e.g., "is + this abstraction worth its complexity?") — do not fabricate a diff just to + fill the field. The suggestive-tone explanation stands alone in that case. +- **The fence syntax differs by provider — this is not cosmetic, using the + wrong one means the host will not recognize the block as an applicable + suggestion:** + - **GitHub:** a bare ` ```suggestion ` fence works for both single-line + and multi-line replacements. + - **GitLab:** the fence must carry a line-offset annotation, + ` ```suggestion:-{lines_above}+{lines_below} `, where the offsets are + relative to the anchored comment line. Use `-0+0` when the suggestion + replaces only the anchored line itself; use `-0+2`, for example, when it + replaces the anchored line plus the two lines below it. A bare + ` ```suggestion ` with no offset is GitHub-only syntax and will not + apply correctly on GitLab. + +## Example (built-in default) + +GitHub: + +`````markdown +This loop re-reads `config.json` on every iteration, which could get slow +for large inputs. Should we hoist the read outside the loop? + +```suggestion +config = load_config("config.json") +for item in items: + process(item, config) +``` +````` + +The same finding on GitLab, replacing the anchored line plus the one below +it (`-0+1`): + +`````markdown +This loop re-reads `config.json` on every iteration, which could get slow +for large inputs. Should we hoist the read outside the loop? + +```suggestion:-0+1 +config = load_config("config.json") +for item in items: + process(item, config) +``` +````` From 16e030926e4b71c62a3e4b1121f980a12dc9f2df Mon Sep 17 00:00:00 2001 From: Asaf Ben Natan Date: Thu, 6 Aug 2026 15:55:31 +0300 Subject: [PATCH 2/3] Fix CI failures and CodeRabbit findings on pr-review skill - Register pr-review in the repo-level AGENTS.md and README.md so the structure-check and markdown-lint CI jobs pass. - Fix a nested-fence bug in start.md that silently broke Step 9's numbered list, plus a stray heading with trailing punctuation. - Skip generated/vendored/binary files and de-duplicate against existing PR/MR discussion during the initial /start pass (parity with the prior code-review PR #46 attempt). - Replace the refs/heads/pr-review/{context} branch with a plain refs/pr-review/{context} ref: fetching into a checked-out branch is refused by Git, which broke every worktree refresh in /start and /continue. - publish.md now filters posts through decisions-{NNN}.json instead of posting every drafted candidate, re-verifies line anchors against real diff hunks instead of --name-status, and persists GitLab discussion IDs incrementally so a retry after a partial failure can't double-post. - Persist and use the GitLab host and a fully URL-encoded project path everywhere, so self-hosted instances and nested subgroups work. - clean.md now derives base_repo/base_repo_is_scratch when metadata is missing, verifies the ref before deleting it, and stops instead of rm -rf'ing artifacts if worktree removal genuinely fails. - Assorted doc-only fixes: safety/auth scoping for /clean, restart semantics in controller.md, prerequisite-failure handling in revise.md/continue.md, and relative-path/step-reference fixes in comment-style.md. Bumps pr-review to 0.2.0 (behavioral changes across most skill files). Co-authored-by: Cursor --- AGENTS.md | 2 + README.md | 4 + pr-review/README.md | 8 +- pr-review/SKILL.md | 2 +- pr-review/guidelines.md | 5 +- pr-review/skills/clean.md | 68 ++++++++++++---- pr-review/skills/continue.md | 40 ++++++--- pr-review/skills/controller.md | 5 ++ pr-review/skills/publish.md | 117 ++++++++++++++++++++------- pr-review/skills/revise.md | 7 ++ pr-review/skills/start.md | 115 ++++++++++++++++++-------- pr-review/templates/comment-style.md | 8 +- 12 files changed, 285 insertions(+), 96 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 6c360f6..da179a9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -16,6 +16,7 @@ This repository contains reusable AI coding workflows that can be installed glob - **e2e** — Story-to-tests workflow for [QE] stories (ingest, plan, revise, code, validate, publish, respond) - **implement** — Story-to-code workflow (ingest, plan, revise, code, validate, publish, respond) - **kcs** — KCS Solution article workflow (gather, draft, validate, handoff) +- **pr-review** — AI-driven review of a remote GitHub PR or GitLab MR by URL (start, revise, publish, continue, clean) - **prd** — Requirements-to-PRD workflow (ingest, clarify, draft, revise, publish, respond) - **rebase-stack** — Rebase a stacked-branch chain with conflict guidance, per-branch validation, and push (start, continue, validate, push) - **sizing** — Pre-cycle Feature sizing with T-shirt sizes and team effort breakdowns (ingest, assess, apply) @@ -161,6 +162,7 @@ ai-workflows/ ├── e2e/ ├── implement/ ├── kcs/ +├── pr-review/ ├── prd/ ├── rebase-stack/ ├── sizing/ diff --git a/README.md b/README.md index b6b5dbd..a6088c8 100644 --- a/README.md +++ b/README.md @@ -30,6 +30,9 @@ Reusable AI coding workflows a team member can install globally or per-project, - **Code Review** -- AI-driven code review for uncommitted changes: discover project conventions, review with an independent reviewer perspective, present findings with honest implementor assessments for human decision, iterate until approved. Supports unattended mode for fully automated review-fix-iterate cycles. See [code-review/README.md](code-review/README.md). +- **PR Review** -- AI-driven review of a remote GitHub PR or GitLab MR by URL: check it out into a disposable git worktree, explain context before critiquing, draft inline comments with permalinks/snippets/suggestion blocks, and post only after local approval. Never changes the reviewed code. + See [pr-review/README.md](pr-review/README.md). + - **CVE Fix** -- Automated CVE remediation: read vulnerability details from Jira, apply multi-strategy dependency fixes, validate, create pull requests, backport to release branches, and close Jira tickets. Language-agnostic. See [cve-fix/README.md](cve-fix/README.md). @@ -112,6 +115,7 @@ Each workflow is intended for a specific project or use case: - **bugfix** -- the **Flight Control** projects ([flightctl](https://github.com/flightctl/flightctl), [flightctl-ui](https://github.com/flightctl/flightctl-ui)) - **code-review** -- any project; reviews uncommitted changes against discovered project conventions +- **pr-review** -- any project; reviews a remote GitHub PR or GitLab MR by URL - **docs-writer** -- the [edge-manager](https://gitlab.cee.redhat.com/red-hat-enterprise-openshift-documentation/edge-manager) downstream docs project - **prd** -- teams drafting Product Requirements Documents from Jira features - **design** -- teams creating technical design documents and Jira-ready epic/story breakdowns from PRDs diff --git a/pr-review/README.md b/pr-review/README.md index ad42495..7e85ff9 100644 --- a/pr-review/README.md +++ b/pr-review/README.md @@ -101,9 +101,11 @@ every kept finding here also carries: - A **permalink** to the exact line(s) at the PR/MR's head SHA - The **quoted snippet** at that location -- A **suggested-change block** (a fenced ` ```suggestion ` block) when the - fix is a concrete, mechanical replacement — omitted for conceptual or - design-level findings +- A **suggested-change block** when the fix is a concrete, mechanical + replacement — omitted for conceptual or design-level findings. The fence + syntax is provider-specific: a bare ` ```suggestion ` block on GitHub, or + ` ```suggestion:-{lines_above}+{lines_below} ` on GitLab (see + `templates/comment-style.md`). ### Pluggable Comment Style diff --git a/pr-review/SKILL.md b/pr-review/SKILL.md index d78fd4f..9e15352 100644 --- a/pr-review/SKILL.md +++ b/pr-review/SKILL.md @@ -1,6 +1,6 @@ --- name: pr-review -version: 0.1.0 +version: 0.2.0 description: >- AI-driven review of a remote pull request or merge request, given its URL (GitHub or GitLab, auto-detected). Checks out the PR/MR into a git worktree, diff --git a/pr-review/guidelines.md b/pr-review/guidelines.md index f478820..98bcdfc 100644 --- a/pr-review/guidelines.md +++ b/pr-review/guidelines.md @@ -79,7 +79,10 @@ specific to reviewing a remote PR/MR. ## Safety - Verify the matching host CLI is authenticated (`gh auth status` / - `glab auth status`) before doing anything else. + `glab auth status`) before any phase that calls a host API (`/start`, + `/publish`, `/continue`). `/clean` only reads local metadata and removes + local files -- it never calls a host API, so it must not be blocked by + missing or expired host credentials. - Read the target project's own `AGENTS.md`, `CLAUDE.md`, and contribution guidelines (from the worktree) before reviewing. The reviewed project's conventions override general preferences. diff --git a/pr-review/skills/clean.md b/pr-review/skills/clean.md index 925a1b1..9122cee 100644 --- a/pr-review/skills/clean.md +++ b/pr-review/skills/clean.md @@ -19,7 +19,7 @@ reuse it. Run this when the user is done reviewing a given PR/MR (no more - **Only delete artifacts and the worktree.** Never modify or delete anything in the reviewed repository's actual history -- only the local - disposable worktree/branch/scratch-clone this workflow created. + disposable worktree/ref/scratch-clone this workflow created. - **Confirm before deleting.** Show the user what will be removed and wait for confirmation. - **Context-scoped.** Only clean artifacts for the specified (or, if @@ -44,10 +44,26 @@ user there is nothing to clean for this PR/MR. Read `.artifacts/pr-review/{context}/review-metadata.json` (if present) for `base_repo`, `base_repo_is_scratch`, and `context` -- needed to know exactly -what to tear down. If metadata is missing (e.g., an interrupted `/start`), -infer what exists by checking for -`.artifacts/pr-review/{context}/worktree/` and -`.artifacts/pr-review/{context}/_scratch-repo/` directly. +what to tear down. + +If metadata is missing (e.g., an interrupted `/start`), derive both values +instead of guessing: + +- `base_repo_is_scratch`: `true` if + `.artifacts/pr-review/{context}/_scratch-repo/` exists, else `false`. +- `base_repo`: if scratch, it's `.artifacts/pr-review/{context}/_scratch-repo`. + Otherwise, derive it from the worktree itself (if + `.artifacts/pr-review/{context}/worktree/` exists): + ```bash + git -C .artifacts/pr-review/{context}/worktree rev-parse --path-format=absolute --git-common-dir + ``` + `{base-repo}` is that path's parent directory (strip the trailing `/.git`). + +If neither the worktree nor a scratch clone exists, there is nothing +requiring `{base-repo}` -- skip straight to Step 7 (artifact removal). +If `{base-repo}` still can't be determined from the above, stop and ask the +user for it rather than guessing; do not proceed to Step 5's worktree/ref +removal without it. ### Step 4: Show What Will Be Removed @@ -66,7 +82,7 @@ Context: {context} | Item | Description | |------|-------------| | worktree/ | Git worktree checked out at the PR/MR head | -| pr-review/{context} branch | Local branch backing the worktree (in {base-repo}) | +| refs/pr-review/{context} | Custom ref backing the worktree (in {base-repo}) | | _scratch-repo/ | Scratch clone of the target repo (only if one was created) | | 00-reviewer-profile.md | Reviewer profile | | 01-pr-context.md | PR/MR context summary | @@ -79,7 +95,7 @@ Confirm removal? (This cannot be undone. Anything already posted to the host is unaffected -- this only removes local files.) ``` -### Step 5: Remove the Worktree and Branch +### Step 5: Remove the Worktree and Ref After user confirmation: @@ -87,18 +103,42 @@ After user confirmation: git -C {base-repo} worktree remove --force .artifacts/pr-review/{context}/worktree ``` +Then confirm it's actually gone before doing anything else destructive: + +```bash +git -C {base-repo} worktree list --porcelain +``` + +If the removed path no longer appears in that list, proceed -- this covers +both a clean removal and the "already removed manually" case. If the +command failed **and** the path still appears (a genuine removal failure, +not just "already gone"), **stop here**: report the exact error, leave the +artifact directory in place (don't run Step 6/7), and ask the user how to +proceed -- don't `rm -rf` a directory Git still thinks is a registered +worktree. + If `base_repo_is_scratch` is `false` (the base repo was reused, not -scratch-cloned), also drop the local tracking branch: +scratch-cloned), also drop the custom ref -- but only after confirming it +still points to the commit this workflow last recorded (`head_sha` in +`review-metadata.json`, if available), so a coincidentally-named, +unrelated ref is never touched: + +```bash +git -C {base-repo} rev-parse refs/pr-review/{context} +``` + +If that matches (or metadata was missing and the ref simply exists from +this same context), delete it: ```bash -git -C {base-repo} branch -D pr-review/{context} +git -C {base-repo} update-ref -d refs/pr-review/{context} ``` -If `base_repo_is_scratch` is `true`, the branch lives inside the scratch -clone that Step 6 removes entirely -- no separate branch-delete needed. +If it doesn't match, skip the delete and tell the user, rather than +guessing. -If `git worktree remove --force` fails (e.g., already removed manually), -note it and continue with cleanup rather than aborting. +If `base_repo_is_scratch` is `true`, the ref lives inside the scratch +clone that Step 6 removes entirely -- no separate ref-delete needed. ### Step 6: Remove the Scratch Clone (if any) @@ -127,7 +167,7 @@ unaffected -- this only removes local files. ## Output - Removed `.artifacts/pr-review/{context}/worktree/` (git worktree) -- Removed local `pr-review/{context}` branch (if `{base-repo}` was reused) +- Removed local `refs/pr-review/{context}` ref (if `{base-repo}` was reused) - Removed `.artifacts/pr-review/{context}/_scratch-repo/` (if one was created) - Removed `.artifacts/pr-review/{context}/` directory diff --git a/pr-review/skills/continue.md b/pr-review/skills/continue.md index 4a8e9d7..c4301fe 100644 --- a/pr-review/skills/continue.md +++ b/pr-review/skills/continue.md @@ -37,9 +37,16 @@ same `/revise` -> `/publish` loop as `/start` did. ### Step 1: Read Prior State Read `.artifacts/pr-review/{context}/review-metadata.json` and -`publish-metadata.json`. If `publish-metadata.json` doesn't exist, `/publish` -was never run (or was interrupted) -- tell the user and suggest resolving -that first (`/revise` then `/publish`) rather than running `/continue`. +`publish-metadata.json`. + +- If `review-metadata.json` is missing, there's no review in progress for + this context -- tell the user and suggest `/start` instead. +- If `publish-metadata.json` doesn't exist, `/publish` was never run (or + was interrupted) -- tell the user and suggest resolving that first + (`/revise` then `/publish`) rather than running `/continue`. +- If either file exists but is malformed or missing a field this phase + needs (`head_sha_reviewed`, `provider`, worktree location), stop and + report exactly which field is missing rather than guessing a value. ### Step 2: Check for New Commits @@ -51,7 +58,7 @@ Fetch current PR/MR metadata with the provider's command (same as gh pr view {number} --repo {owner}/{repo} --json headRefName,commits,state # gitlab -glab mr view {number} --repo {namespace}/{project} -F json +glab mr view {number} --repo "https://{host}/{namespace}/{project}" -F json ``` Compare the current head SHA to `head_sha_reviewed` from @@ -62,19 +69,25 @@ to review and stop here. If `.artifacts/pr-review/{context}/worktree` still exists (the normal case, since only `/clean` removes it), refresh it in place using exactly -`start.md` Step 2's refresh form (fetch the head ref, fast-forward the -local branch, then `reset --hard` the worktree to it). +`start.md` Step 2's refresh form (fetch the head ref into +`refs/pr-review/{context}`, then `reset --hard` the worktree to it -- safe +to fetch into even though the worktree is checked out, since it's a plain +ref, not a branch). If the worktree was somehow removed outside of `/clean` (e.g., manually deleted), fall through to `start.md` Step 2's full setup procedure instead (check for a reusable local repo or scratch-clone, then create the worktree fresh) before proceeding. +If any of these Git operations fail (fetch error, reset failure, missing +`{base-repo}`), stop and report the exact error rather than continuing +with a possibly-stale worktree. + Recompute the merge base and new head SHA: ```bash -git -C {base-repo} merge-base origin/{base_ref_name} pr-review/{context} -git -C {base-repo} rev-parse pr-review/{context} +git -C {base-repo} merge-base "origin/{base_ref_name}" refs/pr-review/{context} +git -C {base-repo} rev-parse refs/pr-review/{context} ``` ### Step 4: Determine What's New @@ -92,10 +105,17 @@ the provider's listing command (same as `start.md` Step 3): # github gh api repos/{owner}/{repo}/pulls/{number}/comments --paginate -# gitlab (project ID is the URL-encoded "namespace/project" path) -glab api "projects/{namespace}%2F{project}/merge_requests/{number}/discussions" --paginate +# gitlab ({project_path} is the fully URL-encoded namespace/project path +# from `start.md` Step 1) +glab api --hostname {host} "projects/{project_path}/merge_requests/{number}/discussions" --paginate ``` +An empty result here is a normal, expected case (no one has commented) -- +treat it as "0 addressed, 0 still open, 0 with a reply," not an error. If +the command itself fails (auth expired, host unreachable), stop and report +the exact error rather than proceeding with a stale or assumed comment +list. + For each previously posted comment, note whether it looks **addressed** (the flagged code changed in a way that resolves the concern), **still open** (the code is unchanged or the concern remains), or has a reply from diff --git a/pr-review/skills/controller.md b/pr-review/skills/controller.md index 014e38d..f927c4e 100644 --- a/pr-review/skills/controller.md +++ b/pr-review/skills/controller.md @@ -117,6 +117,11 @@ Other options: When the user runs `/start` with a PR/MR URL and artifacts already exist for that PR's context, warn that a review is already in progress and ask whether to resume (`/revise` or `/publish`, depending on state) or restart. +**Restart** means: confirmed cleanup of the existing worktree/ref/artifacts +(the same removal `/clean` performs), followed immediately by a fresh +`/start` on the same URL -- see `start.md` Step 1 for the exact procedure. +It never silently reuses or overwrites existing state without that +cleanup. When invoked without a specific command (e.g., just "review this PR: {url}"), treat it as `/start` with that URL -- including the existing-review diff --git a/pr-review/skills/publish.md b/pr-review/skills/publish.md index cb2c30e..4a5dbf6 100644 --- a/pr-review/skills/publish.md +++ b/pr-review/skills/publish.md @@ -41,29 +41,48 @@ it is. ### Step 1: Read Context Read `.artifacts/pr-review/{context}/review-metadata.json` for `provider`, -`owner_or_namespace`, `repo_or_project`, `number`, `head_sha`, -`merge_base_sha`, and the worktree location, plus the latest draft -(`02-draft-review-{NNN}.md`, per the current `iteration`) for the kept -comments to post. For GitLab, derive `{namespace}` and `{project}` from -`owner_or_namespace`/`repo_or_project`. - -If `review-metadata.json` is missing, or `state` isn't `awaiting_decision` -or a later approved state, stop and tell the user there's no approved draft -to publish yet -- run `/start` or `/revise` first. +`host` (GitLab only), `owner_or_namespace`, `repo_or_project`, `number`, +`head_sha`, `merge_base_sha`, and the worktree location. For GitLab, derive +`{namespace}` and `{project}` from `owner_or_namespace`/`repo_or_project`, +and `{project_path}` the same way `start.md` Step 1 does (every `/` in +`{namespace}/{project}` replaced by `%2F`). + +If `review-metadata.json` is missing, stop and tell the user there's no +review in progress for this context -- run `/start` first. The only valid +pre-publish `state` is `awaiting_decision`; any other value is explicit: +- `published`: this round was already posted. Report that instead of + posting again (see the idempotency note in Step 5) -- don't silently + re-post. +- Anything else (or the field missing): stop and tell the user there's no + approved draft to publish yet -- run `/start` or `/revise` first. + +**Determine what's actually approved -- the draft alone is not enough.** +Read the decisions file matching the current `iteration` +(`decisions-{NNN}.json`). `02-draft-review-{NNN}.md` documents every +candidate for transparency, including ones assessed "Disagree" and never +selected to post -- only comments whose matching entry in +`decisions-{NNN}.json` has `"decision": "keep"` may be posted. If a draft +comment has no matching decision entry, treat it as not approved: stop and +ask the user to confirm keep/drop for it rather than guessing either way. ### Step 2: Re-Verify Line Anchors The PR/MR may have changed since the draft was written (a `/revise` round -can span time). Refresh what's needed and re-check that every kept comment -still anchors to a line inside the current diff: +can span time). `--name-status` only proves a file is still touched, not +that a specific line is still inside a hunk -- for each kept comment's +`{path}`, pull the actual hunk ranges instead: ```bash -git -C {worktree} diff {merge-base-sha} HEAD --name-status +git -C {worktree} diff --unified=0 {merge-base-sha} HEAD -- {path} ``` -If a comment's file or line no longer exists in the diff, do not post it -silently and do not silently drop it either -- tell the user which -comment(s) are affected and ask whether to drop, relocate, or abort. +Parse the `@@ -a,b +c,d @@` hunk headers and confirm the comment's line +(and, for multi-line comments, its full `start_line`-`line` range) falls +within one of the `+c,d` ranges on the new side. If a comment's file no +longer appears in the diff at all, or its line falls outside every hunk, +do not post it silently and do not silently drop it either -- tell the +user which comment(s) are affected and ask whether to drop, relocate, or +abort. ### Step 3: Build the Payload @@ -95,15 +114,24 @@ gh api repos/{owner}/{repo}/pulls/{number}/reviews --method POST --input .artifa **GitLab** -- no batched review object exists. First fetch the diff refs needed to anchor each discussion (base/start/head SHAs from the MR's own -diff, not just the worktree's). The project ID is the URL-encoded -"namespace/project" path, which GitLab's REST API accepts directly in -place of the numeric ID: +diff, not just the worktree's). `{project_path}` is the fully URL-encoded +`namespace/project` path from Step 1, which GitLab's REST API accepts +directly in place of the numeric ID; every call must also target `{host}` +explicitly for self-hosted instances: ```bash -glab api "projects/{namespace}%2F{project}/merge_requests/{number}" | jq '.diff_refs' +glab api --hostname {host} "projects/{project_path}/merge_requests/{number}" | jq '.diff_refs' ``` -Then, for each kept comment, POST one discussion, +Before posting, check whether +`.artifacts/pr-review/{context}/publish-metadata.json` already exists **for +this `iteration`** (a prior `/publish` run was interrupted partway through +this exact round). If so, skip any kept comment whose number already +appears in its `posted_comments` -- only post the ones still missing. This +is what keeps a retry from double-posting discussions that already +succeeded. + +Then, for each remaining kept comment, POST one discussion, `.artifacts/pr-review/{context}/tmp-discussion-{n}.json`: ```json @@ -121,13 +149,21 @@ Then, for each kept comment, POST one discussion, ``` ```bash -glab api "projects/{namespace}%2F{project}/merge_requests/{number}/discussions" --method POST --input .artifacts/pr-review/{context}/tmp-discussion-{n}.json +glab api --hostname {host} "projects/{project_path}/merge_requests/{number}/discussions" --method POST --input .artifacts/pr-review/{context}/tmp-discussion-{n}.json ``` -Then post one separate top-level note carrying the fixed summary: +After each successful POST, immediately write (don't wait for Step 5) the +comment number and returned discussion ID into +`.artifacts/pr-review/{context}/publish-metadata.json`'s `posted_comments` +array, with `iteration` set to the current round. Persisting this on every +success, not just at the end, is what makes a retry after a partial +failure safe. + +Then post one separate top-level note carrying the fixed summary (only +after every discussion above succeeded -- see Step 5's idempotency note): ```bash -glab mr note {number} --repo {namespace}/{project} --message "See comments below" +glab mr note {number} --repo "https://{host}/{namespace}/{project}" --message "See comments below" ``` Only the rendered posted text goes in any `body` field -- no internal @@ -146,23 +182,44 @@ content and do not skip it silently. Report: Then ask the user how to proceed (fix and retry, drop the failed ones and post the rest, or abort). Do not invent a resolution on their behalf. -### Step 5: Write Publish Metadata +### Step 5: Finalize Publish Metadata + +For GitHub, the single batched review call from Step 3 either creates the +whole review or fails creating nothing -- write +`.artifacts/pr-review/{context}/publish-metadata.json` fresh on success: + +```json +{ + "iteration": {N}, + "posted_at": "{ISO 8601 timestamp}", + "head_sha_reviewed": "{head-sha}", + "comments_posted": {N}, + "review_id": "{GitHub review id}", + "review_url": "{URL to view the posted review}" +} +``` -On success (all kept comments posted, or the user explicitly accepted a -partial post), write `.artifacts/pr-review/{context}/publish-metadata.json`: +For GitLab, `publish-metadata.json` was already being written incrementally +in Step 3 (`posted_comments`); once every kept comment has a discussion ID +and the summary note posted successfully, finalize it: ```json { + "iteration": {N}, "posted_at": "{ISO 8601 timestamp}", "head_sha_reviewed": "{head-sha}", "comments_posted": {N}, - "review_id": "{GitHub review id, if provider = github}", - "discussion_ids": ["{GitLab discussion id, ...}", "..."], - "review_url": "{URL to view the posted review/discussions}" + "posted_comments": [{"comment": 1, "discussion_id": "{id}"}], + "review_url": "{URL to view the posted discussions}" } ``` -Update `review-metadata.json`'s `state` to `published`. +Only set `review-metadata.json`'s `state` to `published` once this file is +complete for every kept comment (and, for GitLab, the summary note has +posted) -- not on a partial post. If the user explicitly accepted a +partial post (Step 4), leave `state` as `awaiting_decision` so a later +`/publish` retry is still recognized as having unfinished work, and note in +`publish-metadata.json` which comments were intentionally left unposted. Clean up any temp payload files created in Step 3. diff --git a/pr-review/skills/revise.md b/pr-review/skills/revise.md index 140cb59..b7874ed 100644 --- a/pr-review/skills/revise.md +++ b/pr-review/skills/revise.md @@ -43,6 +43,13 @@ iteration and worktree location, the latest draft (`02-draft-review-{NNN}.md`), and the matching decisions file (`decisions-{NNN}.json`). +If `review-metadata.json` is missing, there's no review in progress for +this context -- tell the user and suggest `/start` instead. If the draft or +decisions file for the current iteration is missing or unreadable, or the +worktree directory referenced in metadata no longer exists, stop and report +exactly what's missing rather than guessing its contents or fabricating a +draft to edit. + If the user's requested changes for this round aren't already clear from the conversation, ask them to state their decisions on the current draft first (which comments to keep/drop/edit, any questions, any new findings to diff --git a/pr-review/skills/start.md b/pr-review/skills/start.md index 6572fc9..7a3f2c1 100644 --- a/pr-review/skills/start.md +++ b/pr-review/skills/start.md @@ -23,9 +23,15 @@ approval-to-post. ## Critical Rules -- **Read-only against the reviewed repository.** This phase never edits - files in the worktree and never pushes anywhere. The only git operations - are `clone`, `fetch`, `worktree add`, `diff`, `merge-base`, and `log`. +- **Read-only against the reviewed repository's real history.** This phase + never edits tracked files in the worktree and never pushes to the + reviewed repo's remote. Operations against this workflow's own disposable + local ref/worktree (`fetch`, `worktree add`/`remove`, `reset --hard` on + the `refs/pr-review/{context}` ref only) are fine -- they touch only a + scratch ref this workflow created, never the PR/MR's real commits + upstream. Also allowed: `clone` (for a scratch base-repo clone), + `rev-parse`, `remote -v` (read-only inspection), `diff`, `merge-base`, + and `log`. - **No posting.** This phase only drafts. Posting happens in `/publish`, and only after explicit local approval-to-post. - **Context before critique.** `01-pr-context.md` is always produced and @@ -39,6 +45,11 @@ approval-to-post. - **Optional user focus.** If the user provided focus guidance alongside the URL (e.g., "focus on error handling"), apply it, but still surface CRITICAL/HIGH findings in other categories. +- **Don't duplicate existing discussion.** Cross-reference candidate + findings against the comments/reviews gathered in Step 3. Drop a finding + that duplicates an existing, still-open comment -- unless that comment is + factually wrong or the concern clearly wasn't addressed by the current + diff, in which case keep the new finding and say so in its internal note. ## Process @@ -53,9 +64,19 @@ input's shape, not from a hardcoded host list: - No `://` and it matches `{owner}/{repo}#{number}` (a single `/` before the `#`) -> **provider = github**, CLI = `gh`. Split on `#` for `{number}`, then on `/` for `{owner}`/`{repo}`. -- Path contains `/pull/{n}` -> **provider = github**, CLI = `gh`. +- Path contains `/pull/{n}` -> **provider = github**, CLI = `gh`. Extract + `{owner}`/`{repo}` from the path segments before `/pull/`. - Path contains `/-/merge_requests/{n}` -> **provider = gitlab**, CLI = `glab`. Works for `gitlab.com` and self-hosted GitLab instances alike. + Extract `{host}` (the URL's hostname) and `{namespace}`/`{project}` from + the path segments before `/-/merge_requests/` (`{namespace}` may contain + nested `/` subgroups -- everything between the host and the final project + segment is the namespace). `{host}` matters for self-hosted instances: + every `glab` call below must target it explicitly rather than assuming + `gitlab.com`. Also compute `{project_path}` = `{namespace}/{project}` + with **every** `/` replaced by `%2F` -- GitLab's REST API requires the + full path URL-encoded as a single segment; replacing only the + namespace/project separator breaks nested subgroups. If the input matches none of these shapes, stop and ask the user for a valid PR/MR URL. @@ -63,20 +84,22 @@ valid PR/MR URL. Verify the matching CLI is authenticated: ```bash -gh auth status # if provider = github -glab auth status # if provider = gitlab +gh auth status # if provider = github +glab auth status --hostname {host} # if provider = gitlab ``` If authentication fails, stop and report it -- do not proceed. -Fetch PR/MR metadata with the provider's command: +Fetch PR/MR metadata with the provider's command. For GitLab, pass the full +URL form to `--repo` so `glab` always targets `{host}` -- never assume +`gitlab.com` for self-hosted instances: ```bash # github gh pr view {number} --repo {owner}/{repo} --json title,body,author,baseRefName,headRefName,headRepositoryOwner,url,commits,additions,deletions,changedFiles,state,comments,reviews # gitlab -glab mr view {number} --repo {namespace}/{project} -F json +glab mr view {number} --repo "https://{host}/{namespace}/{project}" -F json ``` If the PR/MR cannot be fetched (private without access, deleted, wrong @@ -95,7 +118,7 @@ review is already in progress for this PR/MR. Stop and tell the user: If the user confirms a restart, remove the existing worktree per `../guidelines.md` (`git worktree remove --force`, drop the -`pr-review/{context}` branch or scratch clone as applicable -- same +`refs/pr-review/{context}` ref or scratch clone as applicable -- same commands as `clean.md`) and delete the artifact directory before proceeding. @@ -128,28 +151,33 @@ not. Use `.artifacts/pr-review/{context}/_scratch-repo` as `{base-repo}` and record `base_repo_is_scratch: true` in metadata (Step 10) -- this tells `/clean` to remove the scratch clone alongside the worktree. -4. Fetch the PR/MR head and base ref, then create the worktree. The ref - path is the one provider-specific detail here (both are plain `git - fetch`, no `gh`/`glab` involved): +4. Fetch the PR/MR head and base ref into `refs/pr-review/{context}` -- + deliberately **not** under `refs/heads/`, since that would make it a + branch, and Git refuses to fetch into a branch that's checked out in a + worktree (a later refresh would then fail). A plain custom ref has no + such restriction and still gives a stable, inspectable name. The ref + path fetched is the one provider-specific detail here (both are plain + `git fetch`, no `gh`/`glab` involved); quote every interpolated ref: ```bash # github - git -C {base-repo} fetch origin "pull/{number}/head:refs/heads/pr-review/{context}" + git -C {base-repo} fetch origin "pull/{number}/head:refs/pr-review/{context}" # gitlab - git -C {base-repo} fetch origin "merge-requests/{number}/head:refs/heads/pr-review/{context}" + git -C {base-repo} fetch origin "merge-requests/{number}/head:refs/pr-review/{context}" - git -C {base-repo} fetch origin {baseRefName} - git -C {base-repo} worktree add .artifacts/pr-review/{context}/worktree "pr-review/{context}" + git -C {base-repo} fetch origin "{baseRefName}" + git -C {base-repo} worktree add --detach .artifacts/pr-review/{context}/worktree "refs/pr-review/{context}" ``` **To refresh an existing worktree instead** (step 1's branch): re-run - the same `fetch origin "{ref}:refs/heads/pr-review/{context}"` command - (fast-forwards the local branch to the new head), then: + the same `fetch origin "{ref}:refs/pr-review/{context}"` command + (updates the local ref to the new head -- safe even though the worktree + is checked out, since this isn't a branch), then: ```bash - git -C .artifacts/pr-review/{context}/worktree reset --hard pr-review/{context} + git -C .artifacts/pr-review/{context}/worktree reset --hard refs/pr-review/{context} ``` 5. Compute the merge base and record `{head-sha}`: ```bash - git -C {base-repo} merge-base origin/{baseRefName} pr-review/{context} - git -C {base-repo} rev-parse pr-review/{context} + git -C {base-repo} merge-base "origin/{baseRefName}" refs/pr-review/{context} + git -C {base-repo} rev-parse refs/pr-review/{context} ``` The PR/MR diff is `git -C {worktree} diff {merge-base-sha} HEAD` (equivalent to the host's shown diff). @@ -200,9 +228,10 @@ List existing comments/discussions with the provider's command: gh api repos/{owner}/{repo}/pulls/{number}/comments --paginate gh api repos/{owner}/{repo}/pulls/{number}/reviews --paginate -# gitlab (project ID is the URL-encoded "namespace/project" path -- -# GitLab's REST API accepts this directly in place of the numeric ID) -glab api "projects/{namespace}%2F{project}/merge_requests/{number}/discussions" --paginate +# gitlab ({project_path} is the fully URL-encoded namespace/project path +# from Step 1 -- GitLab's REST API accepts it directly in place of the +# numeric project ID) +glab api --hostname {host} "projects/{project_path}/merge_requests/{number}/discussions" --paginate ``` Present `01-pr-context.md` to the user before moving on -- this satisfies @@ -251,6 +280,16 @@ Unlike `code-review`, there is no relevance-filtering step -- every changed file in the PR/MR diff is in scope by definition; this is someone else's already-scoped change, not a mixed local working tree. +**Exception: skip generated, vendored, and binary files.** Exclude files +matching common generated/vendored patterns (e.g. `*.gen.go`, `*.pb.go`, +`*_pb2.py`, `vendor/`, `node_modules/`, `dist/`, `build/`, lock files like +`package-lock.json`/`go.sum`/`Cargo.lock`) and any file the diff or `git +-C {worktree} diff --stat` shows as binary. Confirm against the target +project's own conventions (`.gitattributes`, `AGENTS.md`) where available -- +a project may generate files this default list doesn't recognize. Never +draft findings against a skipped file; mention which files were skipped +and why when presenting the draft in Step 9. + ### Step 6: Obtain the Review Follow the same subagent pattern as `code-review`'s equivalent step: @@ -265,6 +304,11 @@ calibrate, then review sequentially. Read full files in `{worktree}` around changed sections, not just the diff in isolation -- the diff shows what changed, the surrounding code reveals whether it fits. +Either way, brief the reviewer with `01-pr-context.md`'s "Existing +Discussion" section and instruct it not to re-raise a concern an existing +comment or review already covers, unless that comment is wrong or the +current diff clearly hasn't addressed it (per the Critical Rules above). + Evaluate all categories defined in `../../_shared/review-protocol.md`. For each finding, capture (internally -- this is not the posted format yet): @@ -320,7 +364,7 @@ posted, following the resolved style's tone and structure rules. Write `.artifacts/pr-review/{context}/02-draft-review-001.md`: -```markdown +````markdown # Draft PR Review -- Round 1 ## PR Context @@ -345,8 +389,9 @@ See comments below {optional fenced suggestion block} - **Internal note (not posted):** {SEVERITY} / {CATEGORY} -- {Agree|Disagree|Partially agree}: {rationale} -### Comment 2 -- ... -``` +### Comment N -- {file}:{line-range} +{repeat the same structure for every remaining candidate} +```` ### Step 9: Present for Local Approval-to-Post @@ -357,16 +402,19 @@ Show, in this order: 2. A compact table of every candidate, including "Disagree" ones (full transparency, same as `code-review`): -```markdown -| # | File:Line | Severity | Category | Finding | Assessment | Recommendation | -|---|-----------|----------|----------|---------|-------------|-----------------| -| 1 | foo.py:42 | HIGH | Correctness | {short description} | Agree -- {rationale} | Keep | -| 2 | bar.go:10 | LOW | Naming | {short description} | Disagree -- {rationale} | Drop | -``` + ```markdown + | # | File:Line | Severity | Category | Finding | Assessment | Recommendation | + |---|-----------|----------|----------|---------|-------------|-----------------| + | 1 | foo.py:42 | HIGH | Correctness | {short description} | Agree -- {rationale} | Keep | + | 2 | bar.go:10 | LOW | Naming | {short description} | Disagree -- {rationale} | Drop | + ``` 3. The full comment blocks from `02-draft-review-001.md` for every candidate recommended "Keep" (link, snippet, exact posted text, suggestion block). +4. If Step 5 skipped any generated/vendored/binary files, a short note + listing which ones and why -- so the user knows they weren't silently + missed. Then prompt: @@ -413,6 +461,7 @@ Write `.artifacts/pr-review/{context}/review-metadata.json`: ```json { "provider": "{provider}", + "host": "{host, if provider is gitlab; null if github}", "owner_or_namespace": "{owner-or-namespace}", "repo_or_project": "{repo-or-project}", "number": {number}, diff --git a/pr-review/templates/comment-style.md b/pr-review/templates/comment-style.md index bdc01c3..4bee2ad 100644 --- a/pr-review/templates/comment-style.md +++ b/pr-review/templates/comment-style.md @@ -1,8 +1,8 @@ # Comment Style (Default) Default tone and structure rules for every comment this workflow drafts and -posts. `skills/start.md`, `skills/revise.md`, and `skills/publish.md` all -resolve and apply this file (or a project override — see "Resolution" +posts. `../skills/start.md`, `../skills/revise.md`, and `../skills/publish.md` +all resolve and apply this file (or a project override — see "Resolution" below) when rendering the text that will actually be posted to the PR/MR. ## Resolution @@ -40,8 +40,8 @@ it: *"Using project override for comment style."* - **No severity or category labels in posted content.** `CRITICAL`, `HIGH`, `Correctness`, `Security`, etc. are internal-only labels used - while presenting the draft for local approval (see `skills/start.md` Step - 10) — they never appear in the text that gets posted to the PR/MR. + while presenting the draft for local approval (see `../skills/start.md` + Step 9) — they never appear in the text that gets posted to the PR/MR. - **No "Finding N" headers in posted content.** Each posted comment is anchored to its own line by the host's own UI; it doesn't need a numbered heading to stand apart from other comments. From 4c8214a06b4dcbc30eac6ade9165962700a15fb1 Mon Sep 17 00:00:00 2001 From: Asaf Ben Natan Date: Thu, 6 Aug 2026 16:05:54 +0300 Subject: [PATCH 3/3] fix(pr-review): serialize comment bodies with jq instead of string interpolation CodeRabbit correctly narrowed the earlier "illustrative JSON" pushback to a real gap: publish.md's payload-building steps didn't tell the agent how to safely embed comment text (which routinely contains quotes, backslashes, and newlines) into the JSON files posted to gh/glab api. Switch to jq --rawfile for body text so escaping is handled correctly instead of relying on naive string interpolation. Bump pr-review to 0.2.1 (behavioral fix to skills/publish.md). Co-authored-by: Cursor --- pr-review/SKILL.md | 2 +- pr-review/skills/publish.md | 61 +++++++++++++++++++++++++++++-------- 2 files changed, 49 insertions(+), 14 deletions(-) diff --git a/pr-review/SKILL.md b/pr-review/SKILL.md index 9e15352..a988c15 100644 --- a/pr-review/SKILL.md +++ b/pr-review/SKILL.md @@ -1,6 +1,6 @@ --- name: pr-review -version: 0.2.0 +version: 0.2.1 description: >- AI-driven review of a remote pull request or merge request, given its URL (GitHub or GitLab, auto-detected). Checks out the PR/MR into a git worktree, diff --git a/pr-review/skills/publish.md b/pr-review/skills/publish.md index 4a5dbf6..5464440 100644 --- a/pr-review/skills/publish.md +++ b/pr-review/skills/publish.md @@ -86,12 +86,41 @@ abort. ### Step 3: Build the Payload -Write the payload to a temp file first (same pattern as other workflows' -`gh api`/`glab api` calls in this repo) to avoid shell-escaping issues, then -delete the temp file after posting. +Comment bodies are rendered Markdown and will routinely contain quotes, +backslashes, or literal newlines (code snippets, suggestion blocks). Never +build the JSON by string-interpolating that text between quotes -- a single +`"`, `\`, or newline produces invalid JSON or a silently corrupted body. +Build every payload with `jq`, which serializes each string correctly: +write the exact comment text (as it will be posted) to its own plain-text +temp file, then pass it in with `jq`'s `--rawfile` so `jq` handles escaping +-- never `--arg` from a shell-interpolated variable holding the same text. +Delete all temp files (bodies, per-comment JSON, assembled payload) after +posting. + +**GitHub** -- one batched review object. Build each comment as its own +JSON object from its body file, collect them into an array, then assemble +the final payload: -**GitHub** -- one batched review object, -`.artifacts/pr-review/{context}/tmp-review-payload.json`: +```bash +# per kept comment, {n} = its position in the kept list +jq -n --rawfile body .artifacts/pr-review/{context}/tmp-body-{n}.txt \ + --arg path "{path}" --argjson line {end-line} \ + '{path: $path, line: $line, side: "RIGHT", body: $body}' \ + > .artifacts/pr-review/{context}/tmp-comment-{n}.json +# multi-line comment: add --argjson start_line {start-line} and merge in +# {start_line: $start_line, start_side: "RIGHT"} + +jq -s '.' .artifacts/pr-review/{context}/tmp-comment-*.json \ + > .artifacts/pr-review/{context}/tmp-comments-array.json + +jq -n --arg commit_id "{head-sha}" \ + --slurpfile comments .artifacts/pr-review/{context}/tmp-comments-array.json \ + '{commit_id: $commit_id, event: "COMMENT", body: "See comments below", comments: $comments[0]}' \ + > .artifacts/pr-review/{context}/tmp-review-payload.json +``` + +Illustrative shape of the assembled payload (the commands above produce +this -- don't hand-write it): ```json { @@ -104,8 +133,6 @@ delete the temp file after posting. } ``` -For a multi-line comment, add `"start_line": {start-line}, "start_side": "RIGHT"` alongside `"line"` (the end line). - Post it: ```bash @@ -131,8 +158,20 @@ appears in its `posted_comments` -- only post the ones still missing. This is what keeps a retry from double-posting discussions that already succeeded. -Then, for each remaining kept comment, POST one discussion, -`.artifacts/pr-review/{context}/tmp-discussion-{n}.json`: +Then, for each remaining kept comment, build the discussion payload the +same `--rawfile` way as the GitHub comments above, then POST it: + +```bash +jq -n --rawfile body .artifacts/pr-review/{context}/tmp-body-{n}.txt \ + --arg base_sha "{diff_refs.base_sha}" --arg start_sha "{diff_refs.start_sha}" \ + --arg head_sha "{diff_refs.head_sha}" --arg new_path "{path}" --argjson new_line {line} \ + '{body: $body, position: {position_type: "text", base_sha: $base_sha, start_sha: $start_sha, head_sha: $head_sha, new_path: $new_path, new_line: $new_line}}' \ + > .artifacts/pr-review/{context}/tmp-discussion-{n}.json + +glab api --hostname {host} "projects/{project_path}/merge_requests/{number}/discussions" --method POST --input .artifacts/pr-review/{context}/tmp-discussion-{n}.json +``` + +Illustrative shape of the assembled payload (for reference only): ```json { @@ -148,10 +187,6 @@ Then, for each remaining kept comment, POST one discussion, } ``` -```bash -glab api --hostname {host} "projects/{project_path}/merge_requests/{number}/discussions" --method POST --input .artifacts/pr-review/{context}/tmp-discussion-{n}.json -``` - After each successful POST, immediately write (don't wait for Step 5) the comment number and returned discussion ID into `.artifacts/pr-review/{context}/publish-metadata.json`'s `posted_comments`