From e87bd0318362a5c1bbb8c9fd8ddbc366aabd8f70 Mon Sep 17 00:00:00 2001 From: Kaio Magalhaes Date: Sun, 30 Aug 2026 22:40:35 -0300 Subject: [PATCH] feat(pr-workflow): add the pull request loop plugin, and sync the marketplace versions Adds pr-workflow, a plugin of five skills that make one pull request loop: write the pull request, review it, answer the review, and record deferred work. Also fixes a distribution bug: marketplace.json advertised qa-check 3.1.0 with the v3 description, while plugin.json and SKILL.md were both 4.0.0. Co-Authored-By: Claude Opus 5 (1M context) --- .claude-plugin/marketplace.json | 21 +- README.md | 8 +- .../pr-workflow/.claude-plugin/plugin.json | 10 + .../pr-workflow/skills/issue-create/SKILL.md | 187 ++++++++++++ .../issue-create/references/issue-bodies.md | 86 ++++++ .../skills/pr-address-comments/SKILL.md | 196 ++++++++++++ .../skills/pr-create-reviewed/SKILL.md | 146 +++++++++ plugins/pr-workflow/skills/pr-create/SKILL.md | 215 +++++++++++++ .../pr-create/references/description-guide.md | 284 ++++++++++++++++++ plugins/pr-workflow/skills/review-pr/SKILL.md | 218 ++++++++++++++ .../skills/review-pr/references/asd-ste100.md | 74 +++++ .../review-pr/references/review-criteria.md | 132 ++++++++ 12 files changed, 1571 insertions(+), 6 deletions(-) create mode 100644 plugins/pr-workflow/.claude-plugin/plugin.json create mode 100644 plugins/pr-workflow/skills/issue-create/SKILL.md create mode 100644 plugins/pr-workflow/skills/issue-create/references/issue-bodies.md create mode 100644 plugins/pr-workflow/skills/pr-address-comments/SKILL.md create mode 100644 plugins/pr-workflow/skills/pr-create-reviewed/SKILL.md create mode 100644 plugins/pr-workflow/skills/pr-create/SKILL.md create mode 100644 plugins/pr-workflow/skills/pr-create/references/description-guide.md create mode 100644 plugins/pr-workflow/skills/review-pr/SKILL.md create mode 100644 plugins/pr-workflow/skills/review-pr/references/asd-ste100.md create mode 100644 plugins/pr-workflow/skills/review-pr/references/review-criteria.md diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index b6aae16..fc8fa69 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -5,14 +5,14 @@ "email": "opensource@codelitt.com" }, "metadata": { - "version": "1.2.0", - "description": "Claude Code skills for AI-native development: quality assurance, supply chain security, and architecture patterns" + "version": "1.3.0", + "description": "Claude Code skills for AI-native development: quality assurance, supply chain security, the pull request loop, and architecture patterns" }, "plugins": [ { "name": "qa-check", - "version": "3.1.0", - "description": "Review code changes for AI Quality Paradox violations and AI-native architecture anti-patterns. Complements Claude Code's built-in /code-review: /code-review hunts correctness bugs; qa-check owns AI-specific quality decay and architecture, including rework risk, test integrity (weakened or gamed tests), dependency provenance, spec discipline, validation gaps, and maintainability smells. Ships a deterministic push gate (PreToolUse hook, no LLM calls): repos with a .qa-check-required file block git push until /qa-check has run.", + "version": "4.0.0", + "description": "Examine code changes for AI Quality Paradox problems, AI-native architecture problems, and agent-buildability. This plugin is an addition to the /code-review command in Claude Code. The /code-review command finds correctness defects. This plugin finds AI-specific quality decay and architecture defects: rework risk, test integrity (weak tests, error paths with no test, tests that use external resources), dependency source, specification discipline, secrets in the code, agent configuration files, prompt injection paths, missing data checks, and maintainability problems. It also supplies a deterministic push gate as a PreToolUse hook, with no LLM calls: in a repository that has a .qa-check-required file, git push stops until /qa-check runs.", "author": { "name": "Codelitt", "email": "opensource@codelitt.com", @@ -23,13 +23,24 @@ { "name": "supply-chain-check", "version": "1.1.0", - "description": "Single-command supply chain security audit. Queries live advisory sources (npm audit, OSV.dev, GitHub Advisory Database) and scans for dangerous version ranges, lock file issues, typosquatting, slopsquatting and hallucinated dependencies, and local IOC artifacts. Wraps npm audit with active threat heuristics.", + "description": "Single-command supply chain security audit. Scans for compromised packages, dangerous version ranges, lock file issues, CVEs, typosquatting, slopsquatting (hallucinated dependencies), and local IOC artifacts. Wraps npm audit and live advisory sources (OSV.dev, GitHub Advisory Database) with active threat heuristics.", "author": { "name": "Codelitt", "email": "opensource@codelitt.com", "url": "https://github.com/codelittinc" }, "source": "./plugins/supply-chain-check" + }, + { + "name": "pr-workflow", + "version": "1.0.0", + "description": "Five skills that make one pull request loop: write the pull request, review it, answer the review, and record the work that you defer. /pr-create writes a title and a description from the branch diff. /pr-create-reviewed opens a draft, examines the diff with subagents that have no session context, and marks the pull request ready after you triage the findings. /review-pr examines a pull request and writes the feedback in Simplified Technical English (ASD-STE100); it prints the review for your own pull request and posts comments on a pull request from a different author. /pr-address-comments collects every unresolved comment, triages each one with you, applies the fixes, and answers each thread. /issue-create files the work that you defer. The skills share two rules: the description states the end state and does not narrate the session, and the author decides each finding.", + "author": { + "name": "Codelitt", + "email": "opensource@codelitt.com", + "url": "https://github.com/codelittinc" + }, + "source": "./plugins/pr-workflow" } ] } diff --git a/README.md b/README.md index 76193ac..b5f62a4 100644 --- a/README.md +++ b/README.md @@ -35,18 +35,24 @@ Then install the plugins you want: ``` claude plugin install qa-check@ai-native-development claude plugin install supply-chain-check@ai-native-development +claude plugin install pr-workflow@ai-native-development ``` Installed plugins receive updates when new versions are published. ### Or copy individually -Copy a skill folder from `plugins//skills//` to `~/.claude/skills/` to use it without the plugin system. +Copy a skill folder from `plugins//skills//` to `~/.claude/skills/` to use it without the plugin system. The five pull request skills live in one plugin, `pr-workflow`, because they call each other; copy the whole set if you copy any of them. | Skill | Command | What it does | |-------|---------|-------------| | [qa-check](plugins/qa-check/skills/qa-check/) | `/qa-check` | Reviews code changes for rework risk, weakened or missing tests, dependency provenance, missing specs, AI slop, and validation gaps. Complements the built-in /code-review, which hunts correctness bugs. Based on the AI Quality Paradox research. | | [supply-chain-check](plugins/supply-chain-check/skills/supply-chain-check/) | `/supply-chain-check` | Single-command supply chain security audit. Queries live advisory sources (npm audit, OSV.dev, GitHub Advisory Database) and scans for dangerous version ranges, lock file issues, typosquatting, slopsquatting, and local IOC artifacts. | +| [pr-create](plugins/pr-workflow/skills/pr-create/) | `/pr-create` | Reads the branch diff, writes a title and a description, and opens the pull request. The description states the end state of the diff, inside a sentence and bullet budget. It never narrates the session. | +| [pr-create-reviewed](plugins/pr-workflow/skills/pr-create-reviewed/) | `/pr-create-reviewed` | Opens the pull request as a draft, reviews it with subagents that get only the diff, and marks it ready after you triage the findings. The reviewers get no session context, so they do not inherit the reasoning of the author. | +| [review-pr](plugins/pr-workflow/skills/review-pr/) | `/review-pr` | Reviews a pull request for code quality, security, and the rules of the repository. Writes every finding in Simplified Technical English. Prints the review for your own pull request; posts comments on someone else's, after you approve them. | +| [pr-address-comments](plugins/pr-workflow/skills/pr-address-comments/) | `/pr-address-comments` | Collects every unresolved comment from all three GitHub comment APIs, triages each one with you, applies the accepted fixes, and answers each thread. | +| [issue-create](plugins/pr-workflow/skills/issue-create/) | `/issue-create` | Files an issue that matches the conventions of the repository. Searches for a duplicate, names the mechanism and the files, and separates a confirmed fact from a hypothesis. | ## Related projects diff --git a/plugins/pr-workflow/.claude-plugin/plugin.json b/plugins/pr-workflow/.claude-plugin/plugin.json new file mode 100644 index 0000000..18ad28a --- /dev/null +++ b/plugins/pr-workflow/.claude-plugin/plugin.json @@ -0,0 +1,10 @@ +{ + "name": "pr-workflow", + "version": "1.0.0", + "description": "Five skills that make one pull request loop: write the pull request, review it, answer the review, and record the work that you defer. /pr-create writes a title and a description from the branch diff. /pr-create-reviewed opens a draft, examines the diff with subagents that have no session context, and marks the pull request ready after you triage the findings. /review-pr examines a pull request and writes the feedback in Simplified Technical English (ASD-STE100); it prints the review for your own pull request and posts comments on a pull request from a different author. /pr-address-comments collects every unresolved comment, triages each one with you, applies the fixes, and answers each thread. /issue-create files the work that you defer. The skills share two rules: the description states the end state and does not narrate the session, and the author decides each finding.", + "author": { + "name": "Codelitt", + "email": "opensource@codelitt.com", + "url": "https://github.com/codelittinc" + } +} diff --git a/plugins/pr-workflow/skills/issue-create/SKILL.md b/plugins/pr-workflow/skills/issue-create/SKILL.md new file mode 100644 index 0000000..5b1f712 --- /dev/null +++ b/plugins/pr-workflow/skills/issue-create/SKILL.md @@ -0,0 +1,187 @@ +--- +name: issue-create +version: 1.0.0 +description: | + File a GitHub issue that obeys the title, label, and body conventions of the + repository. The skill learns those conventions from the issues that exist, + and it does not use a fixed template. It works from a short description or + from the current conversation, such as a defect that you just found or a + finding that a review deferred. It searches for a duplicate before it files, + it grounds the issue in the code by naming the file and the mechanism, it + separates a confirmed fact from a hypothesis, it records where the issue came + from, and it reads the label list of the repository instead of inventing a + label. + Use this skill to file a defect, a feature request, or work that a pull + request review deferred. + Triggers: "create an issue", "file a bug", "open a GitHub issue", "track this + as an issue", "file a follow-up". +allowed-tools: + - Read + - Write + - Bash + - Grep + - Glob +--- + +# Create a GitHub issue + +File an issue that agrees with the conventions of this repository. The conventions are in the issues that exist, not in a fixed template. Read them first. + +The skill works from the context that is available: the current conversation, such as a defect that you found, a review follow-up, or an edge case that a triage deferred; or a short description in the argument. + +## Technical names in this document + +This document uses Simplified Technical English (ASD-STE100). These technical names have no approved equivalent: + +| Technical name | Meaning | +|---|---| +| Provenance | The place that the issue came from, such as a pull request review or an investigation | +| Mechanism | The code path that causes the problem, not the symptom that a user sees | +| Taxonomy | The set of labels that the repository uses | +| Load-bearing domain | The part of a repository where a defect is most expensive | + +## Usage + +```bash +/issue-create +/issue-create # take the issue from the current conversation +``` + +--- + +## Step 1: Collect the context + +**Find the subject of the issue.** + +- From the conversation: use the defect, the finding, or the follow-up that you discussed most recently. If there is more than one candidate, ask the user which one. You can also offer to file more than one issue. +- From the argument: use the description as the start, then add what you find in the code. + +**Ground the issue in the code.** A good issue names the mechanism, not only the symptom: + +- Find the files and the functions, and put each path in backticks, for example `src/charges/candidates.ts`. +- Separate what you confirmed from what you suspect. Write "Confirmed:" for a fact that you verified. Write "Suspected, not yet reproduced" for a hypothesis. +- Record the provenance: the pull request review, the investigation, or the conversation that found this. For example: "Follow-up from the review of PR #121". + +## Step 2: Learn the conventions of this repository + +Read the issues that exist. Do not assume a format. + +```bash +gh issue list --state all --limit 30 --json number,title,labels \ + --jq '.[] | [(.number|tostring), .title, ([.labels[].name] | join(","))] | @tsv' +gh issue view # read two or three bodies in full +``` + +Record these items: + +- The title format. Many repositories use `scope: a specific description`. Some use a `BUG:` prefix. Some use plain prose. +- The weight of a body. Some repositories write dense prose. Some use headings. +- If the repository has an issue template in `.github/ISSUE_TEMPLATE/`. If a template exists, **the template controls the structure**. + +## Step 3: Search for a duplicate + +Search before you file: + +```bash +gh issue list --state all --search "" --limit 10 +gh issue list --state open --limit 30 # read the titles if the search terms are unclear +``` + +- If an **open** issue agrees, stop and report it. Offer to add the new context as a comment with `gh issue comment --body-file `. +- If a **closed** issue agrees, name it in the new body, for example "This is a regression of #N". + +## Step 4: Classify the issue + +**The type controls the shape of the body:** + +- **Defect.** The behavior is wrong today. The body starts with the defect and the evidence. +- **Feature.** This is a new capability. The body starts with the scope bullets. Name the specification file if one exists. +- **Follow-up.** This is a known gap that a review deferred and that is safe today. The body starts with the provenance and the reason for the deferral. + +**Propose a priority. Do not choose one silently.** Use the priority scale of the repository, which is frequently `P0` for a blocker, then `P1`, `P2`, and `P3`. If the issue touches the load-bearing domain of the repository, say so, and propose a high priority. + +## Step 5: Choose the labels + +Read the label list on each run. Never use a list from memory, because the taxonomy changes: + +```bash +gh label list --limit 50 --json name,description --jq '.[] | [.name, .description] | @tsv' +``` + +Choose one label from each group that the repository uses, such as the area, the priority, the timeline, and the type. Use a label only if it fits. A small follow-up with no label is better than a label that is wrong. **Never create a new label without approval from the user.** + +## Step 6: Write the title + +Use the format of the repository. If the repository has no clear format, use `scope: a specific description`. The scope is the component name that a person searches by. + +- State the real defect or the real work. Do not state a vague area. +- Add the consequence in parentheses when that helps: `(this avoids many alerts on a backdate of more than one year)`. +- Give the phase for a feature that has more than one phase: `Lease renewal — Phase 1: …`. + +Correct: `move-out: a non-rent monthly charge is not prorated on a mid-month move-out` + +Correct: `reconciliation: block a reopen when the period is closed` + +Incorrect: `Fix proration bug` — this has no scope and no specifics + +Incorrect: `feat(leases): add proration` — the conventional commit format is for a pull request title, not for an issue + +## Step 7: Write the body + +Follow `references/issue-bodies.md` for the guidance for each type. The necessary parts: + +- **The provenance first**, when a review or an investigation found the issue. +- **The problem with its mechanism**: what is wrong, *where*, with the file paths in backticks, and *why it happens*. +- **The evidence**: what you confirmed, and what you suspect. +- **The recommended fix**: the approach, and where it belongs. Name the alternative in one line if there is a real choice. +- **The acceptance**: what "done" looks like. For a defect, this always includes a regression test that fails today. +- **The cross-references**: related issues as `#N`, and the specification files. + +Match the weight of the body to the weight of the issue. A follow-up of two sentences is two sentences. A small issue needs no heading. + +## Step 8: Show the issue, then create it + +Show the issue for approval before you create it: + +```markdown +## Proposed issue + +**Title**: +**Labels**: <label>, <label> + +**Body**: +<body> + +--- +Do you want me to create this issue? +``` + +After the user approves, write the body to a file with the Write tool and create the issue: + +```bash +gh issue create --title "<title>" --label "<label1>" --label "<label2>" --body-file <path-to-body> +``` + +Use `--body-file`. A body that contains a backtick or a single quotation mark breaks an inline `--body` argument. + +Then report the issue URL. If the user asked you to file the issue without a review, skip the approval step, but still show what you created. + +## Quality checklist + +- [ ] You read the issues that exist, and matched their title and body conventions +- [ ] You read `.github/ISSUE_TEMPLATE/` and used the template if one exists +- [ ] You searched for a duplicate in the open issues and the closed issues, and reported a match instead of filing again +- [ ] The title states the real defect or the real work, with its scope +- [ ] You read the label list on this run. The labels fit, and you invented none +- [ ] The body gives the provenance when a review or an investigation found the issue +- [ ] The problem statement names the files in backticks and explains the mechanism +- [ ] The body separates a confirmed fact from a hypothesis +- [ ] The body gives the recommended fix and the acceptance criteria. For a defect, it names the regression test that fails today +- [ ] An issue in the load-bearing domain names the invariant at risk, and proposes a high priority +- [ ] The body cross-references the related issues and the specification files +- [ ] You wrote the body to a file and used `--body-file` +- [ ] The user approved before you created the issue, unless the user asked you to file it directly + +## Reference + +`references/issue-bodies.md` gives the body shape for a defect, a follow-up, and a feature, plus the universal rules and the anti-patterns. diff --git a/plugins/pr-workflow/skills/issue-create/references/issue-bodies.md b/plugins/pr-workflow/skills/issue-create/references/issue-bodies.md new file mode 100644 index 0000000..97320d7 --- /dev/null +++ b/plugins/pr-workflow/skills/issue-create/references/issue-bodies.md @@ -0,0 +1,86 @@ +# How to write an issue body + +> A good issue body is dense and grounded. It is not a template with empty sections. Match the +> weight of the body to the weight of the issue: a small follow-up is one tight paragraph, and a +> defect that blocks a release gets structure. If the repository has a file in +> `.github/ISSUE_TEMPLATE/`, that template controls the structure, and this file gives the +> content guidance inside it. + +## The body of a defect + +```markdown +**Defect (blocks the release).** <!-- Use a severity marker only when the defect truly blocks. --> +<What is wrong, in one sentence, with its mechanism: the code path that misbehaves, and the +file paths in backticks.> + +**Confirmed:** <The evidence. Give counts, the records that you checked, and what you ran. +Omit this section when you did not verify the problem. Then write "Suspected, not yet +reproduced" instead.> + +**Recommended fix:** <The approach, and the place that it belongs. Name the alternative in one +line if there is a real design choice.> + +**Acceptance:** <What "done" looks like. For a defect, this always includes a regression test +that fails today. Add the cleanup of the data that is already wrong, if any exists.> + +Ref: <The provenance: a pull request review, an investigation, or a related issue #N.> +``` + +A good example names the exact files, states the confirmed effect with a count, such as "2 active +records have a rate above zero but no recurring charge", gives a recommended fix and its +alternative, and gives acceptance criteria that include the cleanup of the existing data. + +## The body of a follow-up + +A follow-up is usually one paragraph. Start with the provenance and the severity. Then give the +mechanism. Then give the request. + +```markdown +Follow-up from the review of PR #NNN (this problem exists already). <What the gap is, and +where, with `path/to/file.ts` in backticks. Why the gap is safe today, or low impact.> <What +to do about it.> <The priority.> +``` + +A good example: "From the review of PR #87 (Low). `postChargesInTx` loops from the effective +month to the current month with no upper limit (`src/charges/post.ts:88`). The loop is safe, +because it is idempotent. But a backdate of more than one year exceeds the transaction budget and +sends many alerts. Add a limit of 24 months, or add a bound on `effectiveDate` in the schema." + +## The body of a feature + +Use scope bullets, not prose. Name the specification file if one exists. Name the phase when the +work is staged. + +```markdown +<One line: what this is, and when it starts.> Spec: `<path to the spec file>`. + +- **<Capability>:** <What it does, which surface it changes, and the state transitions in + capital letters, for example Accept → PENDING_SIGNATURE> +- **<Capability>:** <…> +- <The sequence: "this waits for #23", or "this starts after Phase 1"> +``` + +## The universal rules + +- **Put each file path in backticks** when you know the mechanism. An issue is a search target for + the person who picks it up. +- **Cross-reference** a related issue as `#N`, and a specification as its path. +- **Name the domain invariant.** If the issue touches the load-bearing domain of the repository, + say which invariant is at risk, and say if the data is wrong now or only at risk. +- **State what is not broken**, when that prevents an incorrect triage. For example: "The ledger + stays correct, but the refund does not appear as cash that the customer is owed." +- **Add no filler.** Do not write an empty section such as "Steps to reproduce: N/A". Do not + repeat the title. Do not add a heading to an issue of two sentences. + +## Anti-patterns + +**A symptom with no mechanism:** "Proration is wrong sometimes on move-out." + +**A template with empty sections:** "Environment: N/A. Screenshots: none." + +**A hypothesis that reads as a confirmed fact.** Write "suspected" until you reproduce the +problem. + +**Acceptance criteria that repeat the fix:** "the bug is fixed". + +**An issue that you file without a search** for an open issue about the same mechanism. diff --git a/plugins/pr-workflow/skills/pr-address-comments/SKILL.md b/plugins/pr-workflow/skills/pr-address-comments/SKILL.md new file mode 100644 index 0000000..4ffeb65 --- /dev/null +++ b/plugins/pr-workflow/skills/pr-address-comments/SKILL.md @@ -0,0 +1,196 @@ +--- +name: pr-address-comments +version: 1.0.0 +description: | + Collect every unresolved review comment on a pull request, triage each one + into fix, defer, decline, or already fixed with the author, apply the fixes + that the author accepts, and answer each thread. The skill reads all three + GitHub comment surfaces, because inline comments, top-level comments, and + review summaries are three different APIs and one of them is easy to miss. It + uses GraphQL to skip the threads that a reviewer resolved. It reads each + comment against the code as it is now, not as the comment describes it. It + never changes the pull request description, because the description states + the end state and not the history of the review. + Use this skill after a person or a review command leaves feedback on a pull + request. + Triggers: "address the review comments", "respond to PR feedback", "fix the + review comments", "reply to the reviewer", "handle PR comments". +allowed-tools: + - Read + - Write + - Edit + - Bash + - Grep + - Glob +--- + +# Address the review comments on a pull request + +A reviewer produces findings. This skill closes the loop: collect what is open, decide each finding **with the user**, fix what the user accepts, and answer each thread, so that no comment stays unanswered. + +## Technical names in this document + +This document uses Simplified Technical English (ASD-STE100). These technical names have no approved equivalent: + +| Technical name | Meaning | +|---|---| +| Thread | One review comment and the replies below it | +| Resolved thread | A thread that a person marked as complete in the GitHub interface | +| Outdated thread | A thread whose line moved, because a later commit changed the file | +| Surface | One of the three GitHub APIs that hold review feedback | + +## Usage + +```bash +/pr-address-comments # the pull request of the current branch +/pr-address-comments <PR_NUMBER> +/pr-address-comments <PR_URL> +``` + +--- + +## Step 1: Find the pull request and the branch + +```bash +gh pr view <PR> --json number,url,title,headRefName,author,isDraft +``` + +With no argument, find the pull request from the current branch. If the branch has no pull request, ask the user for the number and stop. + +**Check out the head branch** if you are not on it. You are going to edit code, and the comments describe the state of that branch. If the branch is behind its remote, pull first. A fix on an old tree gets lost on the next push. + +## Step 2: Collect every comment surface + +Review feedback lives in three places, and they are three different APIs. Read all three. A surface that you miss is how a comment gets lost. + +```bash +# Inline comments on specific lines, with their diff context +gh api repos/{owner}/{repo}/pulls/<PR>/comments --paginate + +# Top-level comments +gh api repos/{owner}/{repo}/issues/<PR>/comments --paginate + +# Review summaries +gh api repos/{owner}/{repo}/pulls/<PR>/reviews --paginate +``` + +**Skip each thread that a reviewer resolved.** Only GraphQL gives you that state: + +```bash +gh api graphql -f query=' + query($owner:String!, $repo:String!, $pr:Int!) { + repository(owner:$owner, name:$repo) { + pullRequest(number:$pr) { + reviewThreads(first:100) { + nodes { + id isResolved isOutdated + comments(first:50) { nodes { id databaseId author{login} body path line } } + } + } + } + } + }' -F owner=<owner> -F repo=<repo> -F pr=<PR> +``` + +Remove each thread where `isResolved` is true. Keep each thread where `isOutdated` is true, and mark it. The line moved, so a later commit can have fixed the problem already. Verify the current file. Do not assume either answer. + +Also remove these items: + +- A comment that you wrote on your own pull request. Your earlier reply is not new feedback. +- Bot output that asks for nothing, such as a coverage table or a deployment link. +- A comment that you answered in a thread reply in this session. + +## Step 3: Read each comment against the code as it is now + +A review comment describes the code as it was when the reviewer wrote the comment. Before you judge the comment, open the file at the path in the comment and read the code that is there **now**. A later push can have fixed the problem, or moved it to a place that the comment does not name. + +Put comments that make the same point on different lines into one group. A group gets one decision and one fix, and then a reply on each thread. + +## Step 4: Triage with the user. Do not start a fix. + +Present one table, then stop: + +| # | Thread | file:line | What the reviewer asks | Your assessment | Proposed | +|---|---|---|---|---|---| + +`Proposed` is one of these four values: + +- **Fix.** The comment is correct, and the change belongs in this pull request. +- **Defer.** The finding is real, but it is outside the scope of this pull request. Open a GitHub issue with `/issue-create`, and put the issue link in the reply. +- **Decline.** The comment rests on an incorrect reading, or the current behavior is deliberate. Give the reason and the `file:line` that shows it. +- **Already fixed.** A later commit resolved it. Give the commit or the current line. + +Your assessment is your honest opinion. It includes "the reviewer is correct and I was wrong". Do not choose Fix to agree with the reviewer. Do not choose Decline to avoid work. An incorrect Decline costs the reviewer a second round. + +**The user decides.** Apply nothing until the user answers. Two rules limit the triage: + +- **Never defer a Critical finding.** +- If the code of the finding is already inside the scope of this pull request, fix it here. Do not open an issue for it. + +## Step 5: Apply the fixes that the user accepts + +Make one commit for each logical fix. Use the commit message convention of the repository, usually `type(scope): description`. **Never name the review round in a commit message.** State what the code now does. + +Run the checks that cover what you touched before you push. Read `package.json`, the `Makefile`, or the CI workflow to find the commands of the repository. Run the full suite if the change reaches the load-bearing domain, the schema, or a shared package. + +If a fix touches the load-bearing domain of the repository, run the specialist reviewer for that domain again on the new diff. A fix that a person makes under review pressure is exactly where a new defect enters. + +```bash +git push +``` + +## Step 6: Answer each thread + +Each comment that you collected gets an answer, and this includes each comment that you declined. Silence reads as "ignored", and it costs the reviewer a second read to find out. + +Write each reply body to a JSON file with the Write tool, because the tool escapes each string +for you. A reply that contains a backtick or a single quotation mark breaks an inline +`-f body='...'` argument. + +```json +{ "body": "Fixed in a1b2c3d. The null check moved into `isLateFeeEligible()`." } +``` + +```bash +# Reply inside the thread, so that the reply stays threaded +gh api repos/{owner}/{repo}/pulls/<PR>/comments/<comment_databaseId>/replies \ + --input <path-to-reply-json> +``` + +Keep each reply to one or two sentences: + +- **Fixed.** Give what changed and where: "Fixed in a1b2c3d. The null check moved into `isLateFeeEligible()` (`src/late-fees.ts:44`)." +- **Deferred.** Give the issue link and the reason that the fix is not in this pull request. +- **Declined.** Give the reason and the `file:line` that supports it. +- **Already fixed.** Give the commit that fixed it. + +**Do not resolve a thread that you replied to**, unless the user asks you to. The reviewer resolves their own comment. If you resolve it, you remove their opportunity to disagree. + +## Step 7: Do not change the pull request description + +**The description states the end state of the diff. It does not state the history of the review.** The `pr-create` skill applies this rule when it opens the pull request, and this skill is the most frequent place where the rule fails. + +Never add an "Addressed review comments" section, a changelog for each round, a list of what round two changed, or a note that a reviewer asked for something. + +If a fix changed what the pull request does, such as a new endpoint, a behavior that you removed, or a different migration, write the description again from the current diff with `/pr-create <PR_URL>`. That command writes the description as if the branch were the first commit. If the fix did not change what the pull request does, do not touch the description. + +The same rule applies to the QA Check Report comment. If HEAD moved, run `/qa-check` again and post a new comment. Never edit the old comment to look like it covered the new code. + +## Step 8: Report + +Print the pull request URL, one line for each comment with its decision and its outcome, each issue that you opened, and the push state of the branch. If anything stays open, such as a comment that the user did not decide or a check that fails, say so plainly. Do not imply that the loop is complete. + +## Quality checklist + +- [ ] You read all three comment surfaces: inline, top-level, and review summaries +- [ ] You skipped each resolved thread, and verified each outdated thread against the current file +- [ ] You read each comment against the code as it is **now** +- [ ] You presented the triage table, and **the user decided** before you made any edit +- [ ] No Critical finding is deferred. In-scope code is fixed here +- [ ] You ran the checks for what you touched +- [ ] You ran the domain reviewer again if a fix touched the load-bearing domain +- [ ] Each comment that you collected has a reply, and this includes each decline +- [ ] You wrote each reply to a file, and did not put it in the shell command +- [ ] You left the threads unresolved, for the reviewer to close +- [ ] **The pull request description did not change.** It has no review narration and no changelog +- [ ] You posted a new `/qa-check` comment if HEAD moved diff --git a/plugins/pr-workflow/skills/pr-create-reviewed/SKILL.md b/plugins/pr-workflow/skills/pr-create-reviewed/SKILL.md new file mode 100644 index 0000000..e92ef35 --- /dev/null +++ b/plugins/pr-workflow/skills/pr-create-reviewed/SKILL.md @@ -0,0 +1,146 @@ +--- +name: pr-create-reviewed +version: 1.0.0 +description: | + Open a pull request as a draft, examine it with subagents that have no + session context, triage the findings with the author, then mark the pull + request ready. The subagents get the pull request number, the diff, and their + lens, and nothing else: the session that wrote the code holds its own + reasons, and a reviewer that inherits those reasons agrees with them. The + skill uses /pr-create for the title and the description, opens the pull + request as a draft, fans out to the specialist reviewer agents of the + repository, presents one findings table for you to triage, applies the fixes + that you accept, limits the loop to three rounds, and marks the pull request + ready. + Use this skill for a change that a reviewer must think about. Use /pr-create + for a documentation fix or a one-line configuration change. + Triggers: "open a reviewed PR", "PR with review", "draft PR and review it", + "review before I open the PR", "self-review this branch". +# No allowed-tools list. This skill starts subagents, and a tool allowlist that omits the +# subagent tool would silently stop the review loop, which is the purpose of the skill. +--- + +# PR Create with a review + +``` +title and description → qa-check → draft PR → subagent review → you triage → QA comment → ready +``` + +The review runs in **separate agent context windows**. That is the purpose of the skill. The session that wrote the code holds its own reasons for the code, and a reviewer that inherits those reasons agrees with them. The reviewers get the pull request number and the diff, and nothing else. + +Use `/pr-create` when the change does not need this loop, such as a documentation fix or a one-line configuration change. Use this skill for a change that a reviewer must think about. + +## Technical names in this document + +This document uses Simplified Technical English (ASD-STE100). These technical names have no approved equivalent: + +| Technical name | Meaning | +|---|---| +| Subagent | A separate agent run that has its own context window | +| Lens | One area of judgement, such as security or migration safety | +| Fan out | To start one subagent for each lens that the diff touches | +| Round | One cycle of review, fix, and push | + +## Usage + +```bash +/pr-create-reviewed # the current branch +/pr-create-reviewed BRANCH_NAME +``` + +--- + +## Step 1: Write the title and the description with `/pr-create` + +Follow **steps 0 to 4 of the `pr-create` skill**: the mode, the description structure from the repository template, the check that the branch is not behind, the split proposal for more than 20 files, the analysis of the diff, the title format, and the description rules of end state and budget. + +Do not repeat or reinterpret those rules here. The `pr-create` skill owns the definition of a good title and description. This skill owns what happens around them. + +Stop at the end of step 4 of that skill, with a title, a description, and the approval of the user. + +## Step 2: Run the quality check, then open the draft + +**Run `/qa-check` before the push, not after.** If the `qa-check` plugin is installed and the repository has a `.qa-check-required` file, a PreToolUse hook stops `git push` until a report exists for HEAD. A push before the check fails. Keep the report, because Step 6 posts it, and Step 6 uses this report again if HEAD does not move. + +```bash +git push -u origin <branch> +gh pr create --draft --title "<title>" --body-file <path-to-body> +``` + +**A draft is the default.** The pull request is not ready for a person until the loop below runs and the report is posted. Open a pull request that is not a draft only when the user asks for that. + +## Step 3: Review in isolated contexts, limited to the diff + +Write the diff to a file one time, so that each reviewer reads a file instead of getting the diff again: + +```bash +gh pr diff <N> > /tmp/pr-<N>.diff +``` + +Find the specialist reviewers that this repository defines: + +```bash +ls .claude/agents/ 2>/dev/null +``` + +Start the reviewers with the Agent tool. Put them **all in one message, so that they run at the same time**: + +- **Always** run one general reviewer. +- **Also run each specialist lens whose scope the diff touches.** The routing table is in the `review-pr` skill, in `../review-pr/references/review-criteria.md`, under "Fan out by lens". Read the table there. Do not keep a second copy of it here. + +Give each agent **only** the pull request number, the path of the diff file, its lens, and the scope rule. Give it no session context, no summary of your intention, and no list of the parts that you believe are correct. An explanation of your reasoning to the reviewer is how you make the reviewer agree with you. + +The scope rule has one definition, in `../review-pr/references/review-criteria.md`, under "Scope: the diff, and only the diff". Copy that section into each agent prompt exactly. Do not put it in different words, and do not write a competing version here. + +## Step 4: Triage. The author decides. + +Merge the findings. Remove a finding that repeats another one at the same file and line. Present one table: + +| Severity | file:line | Finding | Proposed action | +|---|---|---|---| + +Then **ask the user** which findings to fix, which to defer to an issue with `/issue-create`, and which to reject. Do not start a fix before the user answers. + +Two rules limit the triage: + +- **Never defer a Critical finding.** +- If the code of the finding is already inside the scope of this pull request, fix it here. Do not open an issue for it. + +Say plainly when one of these rules applies. + +Check each finding yourself before you present it. A reviewer that runs blind is sometimes incorrect about context of the repository that it could not see. Verify that the citation points to real code, and that the failure scenario is real. Do not put an incorrect finding in the table because an agent wrote it. Do not remove a correct finding because it is inconvenient. + +## Step 5: Loop + +After the fixes, push and run Step 3 again. Use **3 rounds maximum**. From round 2, report only the findings that are Medium or above. A one-line fix must not reach round seven because of style. If round 3 still has a finding that blocks the merge, stop and tell the user. Do not loop again. + +**A fix never changes the pull request description.** A round changes the code, not the description. The description still states only the end state, as step 4 of `/pr-create` requires. This is the most frequent place where that rule fails. + +## Step 6: Post the report, then mark the pull request ready + +Use the report from Step 2 if HEAD did not move. If the loop pushed a fix, HEAD moved, and the report no longer describes this commit. Run `/qa-check` again. Never invent a report. Never post a report from a different commit. + +Post the report and mark the pull request ready. Follow **Step 6 of `/pr-create`** for the comment format: + +```bash +gh pr comment <N> --body-file <path-to-report> # the heading must be exactly: ## QA Check Report +gh pr ready <N> +``` + +Report the pull request URL, the number of rounds that ran, the fixes that you made, and each item that you deferred to an issue. If you skipped a round, or if a finding is unresolved, say so. A loop that you report as complete when it is not is worse than no loop. + +## Quality checklist + +- [ ] `/pr-create` steps 0 to 4 wrote the title and the description. This skill did not write its own +- [ ] You ran `/qa-check` **before** the push, if the plugin is installed +- [ ] You created the pull request with `--draft` +- [ ] One general reviewer and each matching lens ran as subagents, in one message +- [ ] Each agent got only the pull request number, the diff path, its lens, and the scope rule word for word +- [ ] No session context reached any reviewer prompt +- [ ] You verified each finding before you presented it +- [ ] **The user triaged the findings.** No Critical finding is deferred +- [ ] The loop used 3 rounds maximum, and reported only Medium and above from round 2 +- [ ] The review rounds did not change the pull request description +- [ ] You posted `## QA Check Report` as a comment against the current HEAD +- [ ] You ran `gh pr ready` last +- [ ] You reported the outcome honestly, and named each item that stays open diff --git a/plugins/pr-workflow/skills/pr-create/SKILL.md b/plugins/pr-workflow/skills/pr-create/SKILL.md new file mode 100644 index 0000000..2f03eda --- /dev/null +++ b/plugins/pr-workflow/skills/pr-create/SKILL.md @@ -0,0 +1,215 @@ +--- +name: pr-create +version: 1.0.0 +description: | + Read the branch diff, write a pull request title and description, and open + the pull request with the GitHub CLI. The description states the end state of + the diff. It does not narrate the session: no "initially I", no "after review + feedback", no changelog of earlier revisions. The skill reads the repository + pull request template, checks that the branch is not behind the default + branch, counts the changed files and proposes a split when the pull request + is too large to review, and holds the description to a sentence and bullet + budget. It also regenerates the title and description of a pull request that + is already open. + Use this skill for a straightforward pull request. Use /pr-create-reviewed + when you want a review loop before a person sees the pull request. + Triggers: "open a PR", "create a pull request", "write the PR description", + "PR title and body", "update the PR description". +allowed-tools: + - Read + - Write + - Bash + - Grep + - Glob +--- + +# PR Create + +Read the branch diff. Write a title and a description. Open the pull request. + +**Do you want the review loop?** `/pr-create-reviewed` opens the pull request as a draft, examines it with subagents that get only the diff, and marks it ready after you triage the findings. That skill uses steps 0 to 4 below for the title and the description, so the two skills always agree about a good description. + +## Technical names in this document + +This document uses Simplified Technical English (ASD-STE100). These technical names have no approved equivalent: + +| Technical name | Meaning | +|---|---| +| Default branch | The branch that pull requests merge into, usually `main` or `master` | +| Diff | The set of lines that the branch adds, changes, and removes | +| Stacked pull requests | Two or more small pull requests, where each one starts from the branch of the previous one | +| Trailer | A line at the end of a description that records the tool that wrote it | + +## Usage + +```bash +/pr-create # the current branch +/pr-create BRANCH_NAME +/pr-create <PR_URL> # write the title and description of a pull request that is open +``` + +--- + +## Step 0: Find the mode + +If the argument agrees with `github.com/*/pull/*`, the mode is **UPDATE**. Read the pull request and examine its head branch: + +```bash +gh pr view <PR_URL> --json number,headRefName,baseRefName,title,body,isDraft +``` + +If the argument is different, the mode is **CREATE**. Use the named branch, or the current branch. + +## Step 0.5: Find the description structure + +```bash +cat .github/PULL_REQUEST_TEMPLATE.md 2>/dev/null # read this again on each run +``` + +If the repository has a template, its headings, its order, and its checkbox blocks **are** the necessary structure. The `gh pr create --body` command does not apply the template for you. If the repository has no template, use the Summary, Changes, Implementation Details, Test Plan, and Notes structure from `references/description-guide.md`. + +For each of the two conditions, `references/description-guide.md` controls the content **inside** the sections. + +## Step 1: Examine the branch state and the size + +Find the default branch first. Do not assume the name `main`: + +```bash +BASE=$(gh repo view --json defaultBranchRef --jq .defaultBranchRef.name) +git fetch origin "$BASE" +git merge-base --is-ancestor "origin/$BASE" HEAD # exit code 1 shows that the branch is behind +git log "$BASE..HEAD" --oneline +git diff "$BASE...HEAD" --stat +git diff "$BASE...HEAD" +``` + +**If the branch is behind** (exit code 1), show `git log HEAD..origin/$BASE --oneline`. Recommend `git rebase origin/$BASE`. Ask the user before you continue. Stop if the answer is no. + +**Examine the size.** A reviewer approves a pull request that is too large to hold in the mind: + +```bash +git diff --name-only "$BASE...HEAD" \ + | grep -vE '(package-lock\.json|pnpm-lock\.yaml|yarn\.lock|go\.sum|Cargo\.lock|/generated/|\.snap$)' \ + | wc -l +``` + +For more than **20** files, list them in groups by boundary: schema and migration, backend, frontend, background jobs, shared packages, documentation. Propose a split into stacked pull requests along the seam that needs the least rework. Ask the user to split or to continue. + +This proposal is **advice**. The user decides. A split is usually correct when the groups merge independently. A split is usually incorrect when a schema change and its only consumer land in different pull requests. + +**Find the issue number** from the branch name (`123-desc`, `issue-123`) or from the commits (`#123`, `closes #123`). If you find one, read it with `gh issue view <N> --json title,body,labels`. If you find none, continue without one. + +## Step 2: Examine the diff + +- Put the change in a category: feature, fix, refactor, infrastructure, test, or documentation. +- Record the directories and packages that the change touches, and if the change stays in one area. +- Record the **reason**: the problem, the behavior that changes, and the cause of the work. +- **Record the domain risk.** Each repository has a load-bearing domain where a defect is expensive: money movement, authentication and authorization, personal data, data migration, or physical control. Read the root `CLAUDE.md`, `AGENTS.md`, `CONTRIBUTING.md`, or `REVIEW.md` to find which domain this repository protects. If the diff touches it, say so in the description, and name the rules that the change must obey. +- **Examine the dependencies.** If a manifest or a lock file changed, run `/supply-chain-check` if that plugin is installed, and state the result in the description. A test suite that passes gives no evidence about the source of a dependency. + +## Step 3: Write the title + +Use `type(scope): description`. The types are `feat`, `fix`, `refactor`, `test`, `chore`, and `docs`. The scope is the domain or the package. Omit the scope when the change is truly cross-cutting. Use the imperative. Keep the title below 70 characters. Add the `(#123)` suffix when an issue exists. + +Correct: `fix(billing): uncoded charges are not eligible for a late fee (#34)` + +Incorrect: `Update late fees` + +Incorrect: `fix(billing): updated the late fee handler and the charge code validation so that…` + +## Step 4: Write the description + +### State the end state. Do not narrate the journey. + +**The description states the diff as it is now, as if it were the first and only commit.** A reviewer examines the code in front of them. The path that you took competes with the change for their attention. This is the most frequent defect in a generated description. Obey it as a rule, not as a preference. + +Never write these items: + +- "Initially I …", "after review feedback …", "as discussed …", "per the QA report …" +- "changed X, then went back to Y" — state Y, because X does not exist +- An "Addressed review comments" section, a per-round changelog, or a reference to an earlier revision of this pull request +- A defense of a decision that no reviewer questioned +- A record of *your* process: the files that you searched, the options that you rejected + +A decision belongs in the description only when a reviewer who reads the final code asks "why this way?". Then give the decision and its reason in one line. Do not give the deliberation. + +### Budgets + +| Section | Budget | +|---|---| +| Summary | The issue link and **3 sentences or fewer**: what changes, and why | +| Changes | **12 bullets or fewer**, in groups by area | +| Implementation Details | Only a decision that is not obvious. **3 bullets or fewer.** Omit the section if there is none | +| Test Plan | The commands that you ran, and the manual steps | +| **Summary and Changes together** | **600 words or fewer** — a limit, not a target | + +The Test Plan and the Notes are additional to the 600 words. A necessary section must not compete with the prose for space. + +**The sentence limit and the bullet limit do the work.** Three sentences and twelve bullets keep a description easy to scan. The word count only finds a description that started to narrate. A change in one area is usually near 250 words. A change across many areas is usually near 450 words. If you reach 600 words with twelve necessary bullets, the pull request is probably too large for one review. That is the true finding. See the size check in Step 1. Never make good prose worse to meet a number. + +**The bullet test: a bullet is necessary only if a reviewer examines the code differently because of it.** Do not repeat what the diff shows, such as "renamed X" or "added a test for Y", unless the bullet sends the reviewer to a different place. Remove a section instead of adding filler. + +### Necessary content + +- **The issue link** on the first line of the Summary: `Closes #123` closes the issue on merge, and `Refs #123` does not. Omit the line when no issue exists. Never invent an issue number. +- **The Test Plan**: the commands that you ran. Use the commands of this repository, and read `package.json`, `Makefile`, or the CI workflow to find them. For a change in the load-bearing domain, name the invariant checks. +- **The trailer**: put `🤖 Generated with [Claude Code](https://claude.com/claude-code)` last. + +## Step 5: Run the quality checks, then open the pull request + +Show the title and the description. Ask for approval. After the user approves: + +**Run `/qa-check` before the push, not after.** If the `qa-check` plugin is installed and the repository has a `.qa-check-required` file, a PreToolUse hook stops `git push` until a report exists for HEAD. A push before the check fails. Keep the report, because Step 6 posts it. + +```bash +git push -u origin <branch> +gh pr create --title "<title>" --body-file <path-to-body> +``` + +Write the description to a file with the Write tool, then use `--body-file`. A description that contains a backtick or a single quotation mark breaks an inline `--body` argument. + +For the **UPDATE** mode, read the section below. + +## Step 6: Post the QA Check Report as a comment + +Post the report as a **comment**, not in the description. The report is long, and a description that starts with it hides the change: + +```bash +gh pr comment <N> --body-file <path-to-report> # the heading must be exactly: ## QA Check Report +``` + +Never invent a report. Never post a report from a commit that is different from HEAD. + +Report the pull request URL when you are done. + +### The UPDATE mode + +**Write the description again from the current diff. Never add to the description that exists.** Text that you add is how a description collects a changelog of the session. Steps 1 to 4 make a new description of the branch as it is now. + +Keep only the content that this skill did not write: bot sections such as coverage tables and deployment links, and reviewer notes that a person added. Copy them exactly, after the generated content, with two empty lines between them. + +```bash +gh pr edit <N> --title "<title>" --body-file <path-to-body> +``` + +## Quality checklist + +- [ ] You read `.github/PULL_REQUEST_TEMPLATE.md` again on this run, and used its structure +- [ ] You found the default branch with `gh repo view`, and did not assume the name `main` +- [ ] The issue link is present (`Closes #N` or `Refs #N`) when an issue exists, and absent when none exists +- [ ] You compared the branch against the default branch, and asked the user if the branch is behind +- [ ] You counted the files, and proposed a split for more than 20 files +- [ ] The title uses `type(scope): description`, the imperative, and fewer than 70 characters +- [ ] **The description has no session narration**: no "initially", no "after feedback", no changelog +- [ ] The Summary has 3 sentences or fewer. The Changes section has 12 bullets or fewer. Each bullet passes the bullet test +- [ ] The description states the load-bearing domain when the diff touches it +- [ ] The Test Plan lists the commands that you ran +- [ ] You ran `/qa-check` **before** the push, if the plugin is installed +- [ ] You ran `/supply-chain-check` and stated the result, if a manifest or a lock file changed +- [ ] You posted `## QA Check Report` as a comment +- [ ] You wrote the description to a file and used `--body-file` +- [ ] The Claude Code trailer is last + +## Reference + +`references/description-guide.md` gives the section-by-section writing guidance, the default structure, the anti-patterns, and worked examples. diff --git a/plugins/pr-workflow/skills/pr-create/references/description-guide.md b/plugins/pr-workflow/skills/pr-create/references/description-guide.md new file mode 100644 index 0000000..9edc16b --- /dev/null +++ b/plugins/pr-workflow/skills/pr-create/references/description-guide.md @@ -0,0 +1,284 @@ +# How to write a pull request description + +> **A note about structure.** If the repository has a `.github/PULL_REQUEST_TEMPLATE.md` file, +> the `pr-create` skill reads it at run time, and **that template controls** the headings, the +> order, and the checkboxes. The structure below is the fallback when the repository has no +> template. For each of the two conditions, this file gives the content guidance: how to write +> clear text inside the sections. + +## Two rules that outrank the remainder of this file + +**1. State the end state. Do not narrate the journey.** The description states the diff as it is +now, as if it were the first and only commit. Do not write "initially I…", "after review +feedback…", "changed X then went back to Y", an "Addressed comments" section, or a reference to +an earlier revision. A reviewer examines the code in front of them. The path to that code +competes with it for their attention. A decision is necessary only when a reviewer who reads the +final code asks "why this way?". Then give the decision and its reason, not the deliberation. + +**2. Stay inside the budget.** Length is not thoroughness. A description that a reviewer skims is +worse than a short description that they read. + +| Section | Budget | +|---|---| +| Summary | The issue link and 3 sentences or fewer | +| Changes | 12 bullets or fewer, in groups by area | +| Implementation Details | 3 bullets or fewer, or omit the section | +| **Summary and Changes together** | 600 words or fewer — a limit, not a target | + +The Test Plan and the Notes are additional to the 600 words. A necessary section must not compete +with the prose for space. + +**The sentence limit and the bullet limit do the work.** Three sentences and twelve bullets keep +a description easy to scan. The word count only finds a description that started to narrate. A +change in one area is usually near 250 words. A change across many areas is usually near 450 +words. If you reach 600 words with twelve necessary bullets, the pull request is usually too +large. That is the true finding. Making good prose worse to meet a number helps no one. + +**The bullet test:** a bullet is necessary only if a reviewer examines the code differently +because of it. Do not repeat what the diff shows, such as "renamed X" or "added a test", unless +the bullet sends the reviewer to a different place. Remove a section instead of adding filler. + +## The default structure + +```markdown +## Summary + +Closes #NNN <!-- or "Refs #NNN". Omit the line if no issue exists. --> + +[One or two sentences. State WHAT this pull request does and WHY it is necessary.] + +## Changes + +### [area — for example: api, server, or a route domain] + +- [A key change, with its context] +- [A key change, with its context] + +### [area — for example: a shared package, the database layer, or the types] + +- [A key change, with its context] + +### [area — for example: web or the background worker] + +- [A description of the change] + +## Implementation Details (optional) + +[Add this section only for an architecture decision, an approach that is not obvious, +or a trade-off that a reviewer must understand.] + +- **[Decision]**: [The explanation] +- **[The alternative that you examined]**: [The reason that you did not use it] + +## Test Plan + +- [ ] [A specific command or verification step] +- [ ] [A specific command or verification step] +- [ ] [A manual verification step, if one is necessary] + +## Notes (optional) + +- **Risks**: [A known risk, or an area that needs more review] +- **Follow-ups**: [Related work that lands separately] +- **Dependencies**: [A pull request, a migration, or an environment variable that must land first] +- **Breaking changes**: [If the change breaks a consumer] +``` + +## The Summary section + +- **Put the issue line first.** Use `Closes #NNN` when the pull request resolves the issue, because GitHub closes the issue on merge. Use `Refs #NNN` when the pull request is only related. Omit the line if no issue exists. Never invent an issue number. +- **The first sentence** states the main change. +- **The second sentence** states the problem that the change solves. +- Stay at a high level. Do not give implementation details. + +Correct: + +```markdown +Adds a manual "ignore" state to bank reconciliation, so that staff can clear book entries +that will never match a statement line, such as a voided check. +``` + +Incorrect: + +```markdown +This PR updates reconciliation. We changed some handlers and added a status. +``` + +## The Changes section + +- Put the bullets in groups by area, with the most significant group first. Use the boundaries of the repository: the API layer, the shared logic, the data layer, the shared types, the frontend, and the background jobs. +- Keep the boundary story visible. Say when logic moved from one layer to a different layer. +- Use parallel structure. Start each bullet with a verb. +- Give the file path for a complex change. +- Show a behavior change, a schema change, and a new or changed endpoint. + +Correct: + +```markdown +### api — reconciliation + +- Added `POST /v1/bank-reconciliations/:id/ignore-entry` to mark a book entry as NOT_A_BANK_ITEM +- Reopen now checks that the period is still OPEN before it changes the status (`handlers.ts:112`) + +### db + +- Added a `ReconciliationEntryStatus` enum value and its migration. A partial unique index keeps + one reconciliation in progress for each bank account +``` + +Incorrect: + +```markdown +### api + +- Updated reconciliation +- Made some changes to the handlers +- Fixed stuff +``` + +## The Implementation Details section + +Add this section for these items: + +- A technical decision that is not obvious +- A trade-off between two approaches +- A decision about concurrency or transactions, such as the isolation level, a row lock, or a retry +- A decision in the load-bearing domain of the repository +- An effect on performance or on security + +```markdown +## Implementation Details + +- **Serializable transaction**: The ignore operation uses the same SELECT FOR UPDATE lock as the + reconciliation, so that two staff members cannot change the same entry at the same time. It + retries three times on a serialization failure. + +- **No effect on the ledger**: The ignore state exists only in the reconciliation workspace. The + journal entry does not change, because a posted journal entry is immutable. +``` + +Omit this section for a simple change, for a standard pattern that the repository documents +already, and for code that explains itself. + +## The Test Plan section + +- Use a checkbox for each verification step. +- Give the exact command. Read `package.json`, the `Makefile`, or the CI workflow to find the commands of this repository. +- Limit a test run when that helps: give the path of the suite, not only the full run. +- Add a manual verification step when a person must look at a screen. +- **For a change in the load-bearing domain, name the invariants that you verified.** + +Correct: + +```markdown +- [ ] `npm test` — the unit tests and the integration tests pass +- [ ] `npm run typecheck` and `npm run lint` +- [ ] An integration test posts a payment and asserts that the debits equal the credits +- [ ] Reconciled the seeded CSV by hand and verified the totals +``` + +Incorrect: + +```markdown +- [ ] Test it +- [ ] Make sure it works +- [ ] Run tests +``` + +## The Notes section + +```markdown +**Risks**: + +- This change needs a database migration on deployment. It touches a table that holds money. +- This change alters the order of charge application. Monitor the receivables report after deployment. + +**Follow-ups**: + +- The vendor payout part lands in a separate pull request. + +**Dependencies**: + +- Set `STRIPE_WEBHOOK_SECRET` in the deployment environment before you deploy. +- Blocked on #102, which must merge first. + +**Breaking changes**: + +- `GET /v1/reports/ar-aging` renames the `bucket30` field to `bucket_30`. +- Removed the deprecated `legacyLedger` query parameter. +``` + +## Anti-patterns + +**Session narration.** This is the most frequent failure, and the most expensive one. It puts the +attention of the reviewer on your process instead of on the code. + +```markdown +## Summary + +I first tried to filter in the query, but that broke the aging report. After the review +feedback I moved the check into the shared package instead. As discussed, I added the +partial index later, to fix the performance problem that this caused. + +## Changes + +- Addressed a review comment about null handling +- Went back from the approach in the first commit +``` + +State the end state instead: + +```markdown +## Summary + +Excludes uncoded charges from late fee eligibility, so that a customer never gets a fee on a +charge that staff have not classified. + +## Changes + +- `isLateFeeEligible()` returns false for a null charge code (shared package) +- A partial index on open uncoded charges keeps the eligibility query fast (data layer) +``` + +**Text that is too vague:** + +```markdown +## Summary + +Updates some services and fixes bugs. +``` + +**Technical text with no context:** + +```markdown +## Changes + +- Refactored AbstractFactoryBuilder to use the composite pattern +- Extracted the interface IChargePoster<T> with covariant type parameters +``` + +**No structure:** + +```markdown +Changed the charges handler and also updated the core package. Also made some changes to the +schema and fixed a bug in the web app. Tests are also updated. +``` + +**A Test Plan with no content:** + +```markdown +## Test Plan + +- [ ] Run tests +``` + +**Implementation details that are not necessary:** + +```markdown +## Implementation Details + +- Used TypeScript 5.8 syntax +- Imported Decimal from decimal.js +- Created a new function with the name handleRequest() +- Used if/else instead of switch +``` diff --git a/plugins/pr-workflow/skills/review-pr/SKILL.md b/plugins/pr-workflow/skills/review-pr/SKILL.md new file mode 100644 index 0000000..de2af4e --- /dev/null +++ b/plugins/pr-workflow/skills/review-pr/SKILL.md @@ -0,0 +1,218 @@ +--- +name: review-pr +version: 1.0.0 +description: | + Examine a GitHub pull request for code quality, security problems, and the + rules of the repository. Write every finding in Simplified Technical English + (ASD-STE100), in the What, Why, Fix shape, with a file and line citation and + a concrete failure scenario. The delivery depends on the author of the pull + request: for your own pull request the skill prints the review in the + terminal and posts nothing to GitHub; for a pull request from a different + person the skill shows you each comment, waits for your approval, and then + posts them as one review with inline comments. It gives the merge verdict + first, limits the review to the diff, and runs the specialist reviewer agents + of the repository. + Use this skill to review a pull request that is open, your own or one from a + teammate. + Triggers: "review this PR", "review pull request", "code review the PR", + "look at PR 123", "leave review comments". +allowed-tools: + - Read + - Write + - Bash + - Grep + - Glob +--- + +# PR Review + +Examine a GitHub pull request for **code quality**, **security problems**, and **the rules of the repository**. Write all feedback in **ASD-STE100 Simplified Technical English**. + +The delivery depends on the author of the pull request: + +- **The pull request is yours.** Print the review in the terminal, so that you can act on it. Post nothing to GitHub. +- **The pull request belongs to a different person.** Post the findings as comments on the pull request, after the user approves them. + +## Technical names in this document + +This document uses Simplified Technical English (ASD-STE100). These technical names have no approved equivalent: + +| Technical name | Meaning | +|---|---| +| Inline comment | A comment that GitHub attaches to one line of the diff | +| Lens | One area of judgement, such as security or migration safety | +| Load-bearing domain | The part of a repository where a defect is most expensive, such as money movement or personal data | +| Merge verdict | The one-line answer to "can the author merge this?" | + +## Usage + +```bash +/review-pr <PR_URL_or_NUMBER> +``` + +- `PR_URL`: for example `https://github.com/owner/repo/pull/123` +- `PR_NUMBER`: for example `123`. The number resolves against the `origin` remote of the current repository. +- No argument: use the pull request of the current branch. If the current branch has no pull request, ask the user for the number and stop. + +--- + +## Step 0: Read the Simplified Technical English rules + +Read `references/asd-ste100.md` before you write any feedback. Each sentence that you write for this review must obey it. This applies to the terminal summary and to each GitHub comment. This is the purpose of the skill. Do not skip this step. + +## Step 1: Find the author, because the author controls the delivery + +```bash +gh api user --jq .login # your GitHub login +gh pr view <PR> --json number,url,author,title,isDraft # the pull request and its author +``` + +- If `author.login` is the same as your login, the mode is **SELF**. Print the review in the terminal. +- If the author is a bot, that is, `author.is_bot` is true or the login ends with `[bot]`, the mode is **SELF**. Do not post a comment on a pull request from a bot. +- If the author is different, the mode is **OTHER**. Post comments on the pull request. + +State the mode to the user in one line before you continue. For example: "This pull request is yours. I print the review here." Say in the same line if the pull request is a draft, because a draft can change before it is ready. + +## Step 2: Get the diff and the context + +Write the diff to a file. A bare `gh pr diff` prints each line and fills the context window on a large pull request. Read the file and search the file instead. The file headers in the diff give you the list of files, so you do not need a second command for the list. + +```bash +gh pr diff <PR> > /tmp/review-pr-<PR>.diff + +# The comments that exist, so that you do not repeat a point that a reviewer made already +gh pr view <PR> --json comments,reviews +``` + +Skip the lock files and the generated output when you read the diff, for example `package-lock.json`, `pnpm-lock.yaml`, `yarn.lock`, `go.sum`, and a snapshot file. They are noise in a review. + +Understand the change before you judge it: + +- Which directories and packages does the change touch? +- Which endpoint or route domain does the change touch, if any? +- **Does the change touch the load-bearing domain of this repository?** Read `REVIEW.md`, `CLAUDE.md`, `AGENTS.md`, or `CONTRIBUTING.md` to find which domain this repository protects. + +Read the source around the diff when the diff alone is not enough. A finding must be true in the real file, not only in the patch. + +## Step 3: Review across the three lenses + +Read **`references/review-criteria.md`** and apply it in full. That file is the shared definition of what to judge and how hard: the order in which to read the rules of the repository, the three lenses of code quality, security, and repository rules, the scope rule, and the table that fans out to specialist reviewers. + +Do not repeat the content of that file here. Change the criteria file when a criterion must change. + +For each finding, record the file, the line, the severity, and the three-part shape of What, Why, and Fix from the Simplified Technical English reference. + +## Step 4: Severity, the merge verdict, and honesty + +`references/review-criteria.md` defines the severity scale of Critical, High, Medium, Low, and Nit. It also defines the bar for a finding and the three merge verdicts. Apply them as they are written. + +This skill adds one rule about placement. Give the verdict **early**: on the first line of a SELF review, and on the second line of an OTHER summary, directly after the thank-you. + +## Step 5: Deliver the review + +### Mode SELF: print the review here + +Print the review in the terminal in this structure. Post nothing to GitHub. + +```markdown +## PR Review — #<number> <title> (yours — printed here) + +**Merge verdict**: <🚫 Blocking / ⚠️ Merge, then fix soon / ✅ Safe to merge as it is> — <one sentence.> + +**Scope reviewed**: <the areas that the diff touches> · <load-bearing domain: yes or no> + +### Critical +- **<file>:<line> — <a one-line title>.** <What.> <Why.> **Fix:** <an imperative sentence.> + +### High +- ... + +### Medium +- ... + +### Low and Nit +- ... + +### Correct areas +- <an area that is clean, in one sentence.> + +**Verdict**: <one sentence. For example: "Fix the two Critical findings before you merge."> +``` + +If the review found nothing, say so plainly and give the verdict. + +### Mode OTHER: comment on the pull request + +Post the findings to the pull request. Use **inline comments** on the exact lines, in one review, so that the author sees each point in its context. + +1. Write each comment in Simplified Technical English. Check each one against the list in `references/asd-ste100.md`. +2. **Start with a thank-you.** Start the summary with a genuine thank-you to the author, for example "Thank you for this pull request, @author." Keep the findings themselves in Simplified Technical English. +3. **Get approval before you post.** This action is outward-facing. Show the user each comment and the target pull request. Post only after the user approves. +4. Post the full review with **one** command. Choose the path by the type of your findings. + +**Path 1, inline comments. This is the normal path.** Use the GitHub review API. One request posts the summary and each inline comment as a single review. Do not also run `gh pr review`, because that posts a second review. + +Do **not** build the request with inline `-f 'comments[][body]=...'` arguments. A finding can contain a single quotation mark or a backtick, and that character breaks the shell argument. Write the payload to a JSON file instead. Write the file with the Write tool, because the tool escapes each string for you. + +```json +{ + "event": "COMMENT", + "body": "<the summary in Simplified Technical English>", + "comments": [ + { "path": "<file>", "line": 42, "body": "<the finding: What. Why. Fix.>" } + ] +} +``` + +Then post the review from that file: + +```bash +gh api repos/{owner}/{repo}/pulls/<number>/reviews --input <path-to-json> +``` + +**Path 2, no inline comment.** Every finding is cross-cutting. Post the summary alone: + +```bash +gh pr review <PR> --comment --body-file <path-to-summary> +``` + +Rules for each path: + +- Use `event=COMMENT`. Do **not** use `APPROVE` or `REQUEST_CHANGES`. This skill comments. It does not gate the merge, unless the user asks for a verdict event. +- Put a finding that has no single line, such as an architecture point, in the summary and not in an inline comment. +- The summary starts with the thank-you, then the merge verdict on its own line, then one sentence of overall assessment, then the findings that have no line. + +After you post, print the pull request URL and a short list of what you posted. + +## Quality checklist + +- [ ] You read `references/asd-ste100.md` before you wrote any feedback +- [ ] You compared `gh api user` with the pull request author, and stated the mode to the user +- [ ] The review covers code quality, security, and the rules of the repository +- [ ] You read the rule files of the repository before you judged the change +- [ ] The merge verdict is first, and it agrees with the severities that you found +- [ ] Each finding has a real file and line, and the What, Why, Fix shape +- [ ] Each finding gives a concrete failure scenario +- [ ] The review stays inside the diff. A problem outside the diff is one `Out of scope:` line +- [ ] Each sentence obeys ASD-STE100: short, active, simple tense, one idea +- [ ] SELF mode printed the review here and posted nothing to GitHub +- [ ] OTHER mode started the summary with a thank-you to the author +- [ ] OTHER mode showed the comments and got approval before it posted +- [ ] You wrote the JSON payload with the Write tool, and did not build it in the shell +- [ ] The review names the clean areas, and invents no finding + +## Examples + +**Example 1: your own pull request. The review prints here.** + +```bash +/review-pr 275 +# The author is you, so the skill prints the review in the terminal. +``` + +**Example 2: a pull request from a teammate. The review posts to GitHub.** + +```bash +/review-pr https://github.com/owner/repo/pull/266 +# The author is different, so the skill writes the comments, asks you, then posts them. +``` diff --git a/plugins/pr-workflow/skills/review-pr/references/asd-ste100.md b/plugins/pr-workflow/skills/review-pr/references/asd-ste100.md new file mode 100644 index 0000000..98761eb --- /dev/null +++ b/plugins/pr-workflow/skills/review-pr/references/asd-ste100.md @@ -0,0 +1,74 @@ +# How to write review feedback in ASD-STE100 + +Write all review feedback in ASD-STE100 Simplified Technical English. This applies to the +terminal summary and to every pull request comment. Simplified Technical English makes technical +writing clear and unambiguous for each reader, and specially for a reader whose first language is +not English. + +Apply these rules to the prose that you write. Do **not** change the code of the user, quoted log +output, or an identifier to obey these rules. Simplified Technical English controls your +sentences. It does not control the code. + +## The writing rules + +1. **Give one instruction in one sentence.** Divide a request that has two parts. + - Incorrect: "Extract the query into a helper and add a test so the logic is covered." + - Correct: "Extract the query into a helper. Add a test for the helper." + +2. **Keep the sentences short.** Use 20 words or fewer for an instruction. Use 25 words or fewer for a descriptive sentence. + +3. **Use the active voice.** Name the thing that does the action. + - Incorrect: "The lock should be acquired before the write." + - Correct: "Acquire the lock before the write." + +4. **Use the imperative for an instruction.** Start a necessary change with a verb. + - Correct: "Add a null check." "Move this call into the transaction." + +5. **Use the simple present tense or the simple past tense.** Do not use a perfect tense or a future tense when a simple tense is sufficient. Do not use an `-ing` form as the main verb. + - Incorrect: "This is going to cause a race condition." + - Correct: "This causes a race condition." + +6. **Use simple words.** Use the short common word, not the long word. + - Use `use`, not `utilize`. Use `about`, not `regarding`. Use `start`, not `initiate`. Use `end`, not `terminate`. Use `enough`, not `sufficient`. Use `make sure`, not `ensure`. Use `fix`, not `remediate`. Use `also`, not `additionally`. + +7. **Use one word for one thing.** Do not use a synonym for a concept that you named already. If you write "function", do not call the same thing a "method" or a "routine" later. + +8. **Use the articles `a`, `an`, and `the`.** Do not remove them to make the text shorter. + - Incorrect: "Add check to handler." + - Correct: "Add a check to the handler." + +9. **Write a positive statement.** Tell the reader what to do. Do not tell the reader only what to avoid. + - Incorrect: "Do not forget to close the connection." + - Correct: "Close the connection after the write." + +10. **Do not use slang, an idiom, jargon, or humor.** Write "this fails when the input is empty", not "this blows up on empty input". + +11. **Do not use a pronoun that is unclear.** Write the noun again if "it", "this", or "they" can point to more than one thing. + - Incorrect: "It calls the API and it can be null." + - Correct: "The handler calls the API. The API response can be null." + +12. **Give a reason with each finding.** State the problem. Then state the effect. + - Correct: "This query has no index. The report is slow for a large data set." + +## The shape of a finding + +Write each finding in this order, in short sentences: + +1. **What** — the problem, in one sentence. +2. **Why** — the effect or the risk, in one sentence. +3. **Fix** — the action, as an imperative. + +An example finding: + +> **Security — the authorization check is missing.** +> This route reads a customer record. The route does not check the tenant of the caller. +> A user can read a record that belongs to a different tenant. +> Add a tenant check before the query. + +## The check before you send the review + +- Does each sentence have 20 words or fewer? +- Does each sentence give one idea? +- Is the voice active and the tense simple? +- Did you use one word for each concept? +- Did you give the reason for each finding? diff --git a/plugins/pr-workflow/skills/review-pr/references/review-criteria.md b/plugins/pr-workflow/skills/review-pr/references/review-criteria.md new file mode 100644 index 0000000..0afc6dd --- /dev/null +++ b/plugins/pr-workflow/skills/review-pr/references/review-criteria.md @@ -0,0 +1,132 @@ +# Review criteria + +This file is the one definition of **what to judge**, **how severe a finding is**, and **what a +finding must contain**. + +Two callers read this file, and neither one repeats it: + +- The `review-pr` skill, which examines a pull request and delivers the result to the terminal or + to GitHub comments. +- The `pr-create-reviewed` skill, which runs the same judgement in subagents that have no session + context. + +Change this file when a criterion must change. + +## Read these files first, in this order + +The rules of the repository outrank the general rules in this file. Read the rules of the +repository before you judge anything: + +1. **`REVIEW.md`** at the root of the repository, if it exists. It holds the severity rules of + this repository, the list of items to always check, the list of items to skip, and the + verification bar. **It outranks each rule below.** +2. **`CLAUDE.md`, `AGENTS.md`, and `CONTRIBUTING.md`** at the root, and the `CLAUDE.md` file at + each directory level that the diff touches. A root file is frequently a router to more files. + Never decide that a rule does not exist because the root file does not hold it. +3. **The `.claude/rules/*.md` files whose `paths:` frontmatter agrees with the diff**, if the + repository uses them. + +If the repository has none of these files, say so in one line in the review. Then judge the +change against the three lenses below and against the patterns that the surrounding code uses. + +## The three lenses + +**A. Code quality.** Look for clear names, small functions, no dead code, no duplicated code, and +correct error handling with no error that the code swallows. Hold the change to the file size +rule of the repository. Hold the tests to this bar: a test states the intent, a public function +has a unit test, an endpoint has an integration test against a real dependency, and **no test is +made weaker, skipped, or deleted to make the change pass**. + +**B. Security.** Each endpoint checks the correct authentication level and the correct data +scope. A schema validates the input before the input reaches the database. Look for SQL +injection, a secret in the code, personal data in a log, and a value that is correct in only one +environment. Authorization must be a permission check and a scope check. A role name alone is not +an authorization check. + +**C. The rules of this repository.** These carry the most weight, because a general reviewer +cannot infer them. When the diff touches the load-bearing domain of the repository — money +movement, authentication, personal data, data migration, or physical control — hold the change to +the full rule file for that domain. Do not check a shortened list. Hold each pull request to the +boundary rules between packages, and to the documentation rule, if the repository has one: many +repositories require that a change which a user can observe updates its documentation page in the +same pull request. + +## Severity + +Rank each finding **Critical**, then **High**, then **Medium**, then **Low**, then **Nit**. + +- A correctness defect, a security hole, or a broken domain invariant is always **High** or above. +- Each item that `REVIEW.md` lists as always important is **High** or above. It is never a Nit, + and the size of the diff does not change this. + +## The bar for a finding + +**Each finding needs a `file:line` citation and a concrete failure scenario.** The scenario gives +the input or the state, and the incorrect output or the crash that the input produces. If you +cannot write that sentence, you do not have a finding. + +Never infer the behavior of a function from its name. The name `validateCharge()` is not evidence +that the function validates anything. Open the function. Read the source around the diff when the +diff alone is not enough. A finding must be true in the real file, not only in the patch. + +Report only a finding that you can defend against the real code. Do not add filler. If an area is +clean, say so in one sentence. Do not invent a nit. A review with filler costs the author more +time than it saves. + +## The merge verdict + +Give the verdict **first**, so that the author knows immediately if a change is mandatory. + +- **🚫 Blocking. Do not merge until you fix this.** The review found a Critical or a High finding. Name it. +- **⚠️ Merge, then fix soon.** The most severe finding is Medium. Say what to fix. +- **✅ Safe to merge as it is.** The review found only Low findings or Nits, or found nothing. + +Map the verdict to the severities that you found. Do not call a High finding optional. Do not +block a merge for a Nit. + +## Scope: the diff, and only the diff + +Report only on the lines that the diff adds or changes. A problem that exists outside the diff is +out of scope. Name it in one `Out of scope:` line and do not propose a fix. A reviewer who +examines unrelated code makes a reviewable pull request unreviewable, and the author must then +argue about code that they did not touch. + +There is one exception. A file that this pull request touches is in scope when the pull request +makes its existing state worse, or when the changes of the pull request push the file past a size +rule of the repository. + +## Fan out by lens + +One general pass misses the failure modes of a domain. Before you review, find the specialist +reviewers that the repository defines: + +```bash +ls .claude/agents/ 2>/dev/null +``` + +Read the description of each agent. For each agent whose scope the diff touches, run that agent +and merge its findings. Remove a finding that repeats another one at the same file and line. + +If the repository defines no agents, use these lenses. Run one subagent for each lens that the +diff touches, and give it the lens name and the criteria in this file: + +| The diff touches | The lens | +|---|---| +| The load-bearing domain of the repository, such as money, authentication, or personal data | The domain invariants | +| A database schema, a migration, or a database constraint | Deployment safety of the migration | +| A CI workflow, a Dockerfile, or an environment variable | Deployment safety | +| The frontend | The design system, accessibility, and the state that reaches the browser | +| A package boundary or a shared type | The contract between packages | +| Behavior that a user can observe | The documentation obligation | + +Always run one general reviewer as well. It covers what no lens above claims. A subagent cannot +start another subagent, so the caller owns the fan out. + +## How this review divides work with the other checks + +- `/code-review` in Claude Code finds correctness defects in the diff. +- `/qa-check` finds AI-specific quality decay and architecture defects. +- This review finds defects in code quality, in security, and against the rules of the repository, + and it delivers the result to a person or to GitHub. + +Run them together. Do not do the same work two times in one review.