diff --git a/.claude/commands/pr-describe.md b/.claude/commands/pr-describe.md new file mode 100644 index 000000000..e48c80c01 --- /dev/null +++ b/.claude/commands/pr-describe.md @@ -0,0 +1,138 @@ +--- +description: Write or update a PR description that lets reviewers judge the architecture and technical decisions without reading low-level code — high-level mechanism, mermaid diagrams, and a code-area map. +argument-hint: "(optional: PR number/URL; defaults to current branch PR or draft body)" +--- + +Write (or update) the description for this PR so a reviewer can understand what +it does and judge the architecture and technical decisions **without reading the +low-level code**. Inspect the actual diff, commits, and changed files first; +describe what the code does, not the decisions made getting there. + +## Steps + +1. Determine the target and pick the path. Decide once, here, and use the same + path for every command below — never mix PR-derived refs with the local + checkout. + + - **Numbered-PR path** — `$ARGUMENTS` is a PR number or URL. The target is + that PR, which may live in a different repository and on a branch you do + not have checked out. Resolve its refs from metadata, not from local HEAD: + `gh pr view "$ARGUMENTS" --json number,url,headRefName,baseRefName,title`. + Capture `number` and parse `owner`/`repo` from the `url` field (which is + `https://github.com///pull/`). The `url` is gh's + output, so it is safe to parse. You need `owner`/`repo` because a bare + `number` resolves in the *current* repo — a `$ARGUMENTS` URL pointing at + another repo would otherwise read one PR and edit a same-numbered PR here. + Pass `-R / ` to every `gh pr` call on this path. + - **Current-branch path** — no `$ARGUMENTS`. The target is the current + branch. `gh pr view --json number,url,headRefName,baseRefName` tells you + whether a PR already exists; if none does, you will draft the body for the + PR the user is about to open from this branch. + + After resolving refs, check whether the target is part of a **series** + (stacked or multi-part). Any one of these signals counts: `baseRefName` is + not the repository's default branch (it is stacked on a parent PR); it has + descendant PRs (others are stacked on it); or its title carries an `N/M` or + `part N` marker. A foundation PR that targets the default branch but has + descendants still counts, so do not gate on the base ref alone. Walk the + chain both ways. Upward: find the parent PR whose head is this PR's base + (`gh pr list --head "" --state all --json + number,title,url,baseRefName`, adding `-R /` on the numbered-PR + path), repeating on the parent's base until you reach the default branch. + Downward: find child PRs whose base is this PR's head (`gh pr list --base + "" --state open --json number,title,url,baseRefName`, adding `-R`), + repeating on each child's head. Record both ancestors and descendants (each + one's number, title, url) for steps 2 and 3. + +2. Read the change using the path chosen in step 1 — do not fall back to local + `git` on the numbered-PR path, since local HEAD may be an unrelated branch: + + - **Numbered-PR path:** using the `owner`/`repo`/`number` from step 1, + `gh pr diff -R /` for the full diff and + `gh pr diff -R / --name-only` for the file list. + Pull the commit list from + `gh pr view -R / --json commits`. All of these read + the PR head in its own repo, regardless of what is checked out locally. + - **Current-branch path:** fetch the base first (`git fetch origin ""`) + and diff against `origin/`, never the bare local ref, so a parent + branch that has moved (common in a stack) can't produce a stale diff: + `git diff "origin/...HEAD"` (full diff), + `git diff "origin/...HEAD" --stat`, and + `git log "origin/..HEAD" --oneline`, where `` is the + `baseRefName` from step 1 (default `main`). + + From the file list, identify which subsystems are touched (`server/`, + `client/`, `plugin/`, `proto/`, `migrations/`, `packages/proto-python-gen/`). + + If the target is part of a series (step 1), also read each ancestor PR's description + (`gh pr view --json title,body,url`, adding `-R` on the numbered-PR + path) and extract the load-bearing context this PR depends on: the contracts, + abstractions, schema, or decisions established upstream that a reviewer must + understand to judge this change. For each descendant, capture a one-line + scope from its title (skim its body only if the title is opaque) so you can + tell the reviewer what is deferred to later PRs and where it lands. + + Remaining work is often not open as a PR yet, so do not stop at descendant + PRs. Also draw on the effort's plan documents (e.g. `docs/plans/`) for the + phasing and explicit out-of-scope items, and, when running interactively, on + this conversation, which may already name the deferred scope and the tracking + issues/PRs it lands in. Record those deferred items with their tracking + references even when no PR exists for them yet (state facts about scope, not + the back-and-forth of how the work was planned). + +3. Draft the description in this structure: + + 1. **Summary** — 2-4 sentences: what this PR delivers and why it exists. + Lead with the user- or operator-facing capability, not the implementation. + If the PR is part of a series (step 1), follow the summary with a short + **Stack** note: the full chain with PR numbers/links (ancestors down to + the default branch, this PR, and any PRs stacked on top), with this PR + marked; if it has ancestors, a line stating the diff is relative to its + immediate base so the reviewer does not re-review them; the required + context from upstream, meaning the + contracts, abstractions, or decisions this PR builds on, distilled to what + a reviewer needs here rather than a re-summary of the parent PRs; and + what is intentionally out of scope here and where the remaining work + lands, drawn from descendant PRs, the plan docs, this conversation, and + any tracking issues (later phases may not be open as PRs yet). + 2. **How it works** — the end-to-end mechanism in plain language. Walk the + primary flow(s) step by step (who triggers it, what crosses each boundary, + where state is persisted, what comes back). Assume the reader does not + know Go/TS idioms; explain workflows and mechanisms, not syntax. + 3. **Diagrams** — include mermaid diagrams in fenced code blocks labeled `mermaid` so + they render on GitHub. At minimum a component/flow diagram of the main + path; add a state or sequence diagram where lifecycle or ordering matters. + Keep syntax GitHub-safe: quote labels containing special characters, avoid + fragile edge styles (e.g. dotted/labelled edges that GitHub mis-renders). + 4. **Areas of the code involved** — a table so reviewers know where to focus: + `| Area / package / file | What changed | Why it matters for review |`. + Group by subsystem. Call out new vs. modified files, and flag generated + code (`**/generated/**`, `*.pb.go`, `*.pb.ts`) as "generated — skip". + 5. **Key technical decisions & trade-offs** — bullet the choices a reviewer + should scrutinize: new abstractions, data-model/migration changes, + security or validation boundaries, backward-compat or rollout concerns. + One line each: the decision and the alternative it was chosen over. + 6. **Testing & validation** — how correctness was verified (tests added, + manual checks, migrations run) and what is explicitly NOT covered. + +4. Apply the result against the target resolved in step 1: + - If a PR exists, update **that** PR by its `number`, scoped to its repo: + `gh pr edit -R / --body-file ` (write the body + to a temp file to preserve mermaid fences and tables). The `-R` is what + keeps a cross-repo URL target from editing a same-numbered PR in the local + repo. On the current-branch path `-R` is unnecessary (the PR is local). + - If no PR exists yet (current-branch path only), output the body for the + user to use when opening it. + +## Rules + +- Mechanism and architecture over line-by-line detail. If a reviewer needs to + open a file to understand the shape of the change, the description has failed. +- Don't narrate the back-and-forth or rejected approaches — describe the final + state. +- No filler praise. Be concise; prefer tables and diagrams over long paragraphs. +- Always quote JSON-derived branch refs (`headRefName`, `baseRefName`) when + passing them to a shell command. Branch names may contain shell + metacharacters (a branch literally named `feature;rm -rf foo` is valid on + GitHub), so an unquoted ref can run unintended commands instead of just + querying `gh`/`git`. diff --git a/.claude/commands/pr-ready.md b/.claude/commands/pr-ready.md index e30e2677e..e9e1a852b 100644 --- a/.claude/commands/pr-ready.md +++ b/.claude/commands/pr-ready.md @@ -40,9 +40,17 @@ PR description they can paste — or open the PR directly if asked. imports (`client-boundaries`), edits to deployed migrations (`migration-immutability`), `--no-verify` slipping in (`lefthook-bypass-guard`). -5. Draft a PR description following the format in CONTRIBUTING.md: - - **Summary** (1–3 bullets, focus on the *why*) - - **Test Plan** (what was run, how to verify manually) +5. Draft a PR description following the **PR descriptions** standard in + AGENTS.md — the same six-part structure `/pr-describe` produces: Summary, + How it works, Diagrams (mermaid), Areas of the code involved, Key technical + decisions & trade-offs, and Testing & validation. Fold the lint and test + results from steps 2–4 into the Testing & validation section. Reuse the + step-1 diff inline only when this branch targets the default branch. If it is + stacked or part of a series (its base is not the default branch, or sibling/ + child PRs exist), do **not** reuse the `main...HEAD` scope: run `/pr-describe`, + which resolves the PR's real base, scopes the diff to the immediate base, and + adds the Stack note. Reusing `main...HEAD` there would fold in ancestor + changes and misrepresent what this PR actually changes. 6. **Open the PR only if the user's invocation explicitly asked you to** (e.g. "open it", "create the PR", "ship it"). Otherwise stop after presenting the draft so the user can edit it before running diff --git a/AGENTS.md b/AGENTS.md index 8582be948..8a5b925b4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -77,6 +77,45 @@ These are the rules that recur in code review and that contributors most often m - After a PR merges, do not reuse that branch for follow-up work — cut a fresh branch from the updated `main`. +## PR descriptions + +When opening a PR, write the description so a reviewer can judge the +architecture and technical decisions **without reading the low-level code**. +Inspect the actual diff, commits, and changed files first; describe what the +code does, not the decisions made getting there. Structure it as: + +1. **Summary** — 2-4 sentences: what this PR delivers and why. Lead with the + user- or operator-facing capability, not the implementation. For a PR in a + series (stacked on a parent, has descendant PRs, or marked `N/M`), add a + short *Stack* note: the full chain with PR links (ancestors, this PR, and any + PRs stacked on top), that the diff is relative to the immediate base when it + has ancestors, the load-bearing context from upstream PRs + a reviewer needs to judge this change, and what is intentionally out of scope + here and where the remaining work lands (from descendant PRs, the plan docs, + or tracking issues, since later phases may not be open as PRs yet). +2. **How it works** — the end-to-end mechanism in plain language. Walk the + primary flow(s): who triggers it, what crosses each boundary, where state + is persisted, what comes back. Explain workflows, not language syntax. +3. **Diagrams** — mermaid in fenced code blocks labeled `mermaid` so they render on + GitHub. At least a component/flow diagram of the main path; add a state or + sequence diagram where lifecycle or ordering matters. Keep syntax + GitHub-safe: quote labels with special characters, avoid fragile edge styles. +4. **Areas of the code involved** — a table mapping each changed area to its + role so reviewers know where to focus: `| Area / package / file | What + changed | Why it matters for review |`. Group by subsystem (`proto/`, + `server/`, domain logic, migrations, `client/`, `plugin/`); flag generated + code as "generated — skip". +5. **Key technical decisions & trade-offs** — the choices a reviewer should + scrutinize (new abstractions, data-model/migration changes, security or + validation boundaries, backward-compat or rollout concerns). One line each: + the decision and the alternative it was chosen over. +6. **Testing & validation** — how correctness was verified and what is + explicitly not covered. + +Keep it concise: tables and diagrams over long paragraphs, no filler praise, +no narration of rejected approaches. Claude Code users can generate this with +`/pr-describe`. + ## Verification - Don't pin versions, package versions, or upstream behaviors from training @@ -129,6 +168,7 @@ stays private. - `/regen` — run `just gen`, surface what changed, flag generated files to commit - `/pr-ready` — lint + targeted tests + diff summary suitable for a PR description +- `/pr-describe` — write/update a PR description (high-level mechanism, mermaid diagrams, code-area map) per the "PR descriptions" standard above - `/release-notes ` — draft release notes from commits since the previous tag - `/triage-pr <#>` — fetch PR status, summarize failing CI, propose next action - `/plan ` — scaffold a new TDD, PRD, or plan under `docs/plans/` diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 16f2d6e0a..6c4179c6b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -112,16 +112,31 @@ Prefixes: ### Pull Requests -Create PRs with a clear summary and test plan: +Write the description so a reviewer can judge the architecture and technical +decisions without reading the low-level code. Use the six-part structure +documented in the **PR descriptions** section of [AGENTS.md](./AGENTS.md): +Summary, How it works, Diagrams (mermaid, so they render on GitHub), Areas of +the code involved, Key technical decisions & trade-offs, and Testing & +validation. Scale each section to the change — a one-line fix does not need a +diagram, a new subsystem does. ```bash gh pr create --title "Brief description" --body "## Summary -- Bullet point summary of changes +- What this delivers and why -## Test Plan -- How to verify the changes work" +## How it works +- The end-to-end mechanism in plain language + +## Areas of the code involved +| Area / file | What changed | Why it matters for review | +| --- | --- | --- | + +## Testing & validation +- What was run and how to verify; what is not covered" ``` +Claude Code users can generate a conforming description with `/pr-describe`. + ## Cross-Component Workflows ### Adding a New API Endpoint