From 5088dc3e6ba1fbc7c06ae474da19eb234bcd49b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20G=C3=B3recki?= Date: Thu, 20 Aug 2026 12:08:23 +0200 Subject: [PATCH] Add review-pr and review-open-prs skills Encode the PR code-review workflow proven on the review round of PRs #43-#51 so it is repeatable: - review-pr: the full single-PR protocol - stacked-base diff scoping, verification-first findings (golden tests, helm unittest, kubeconform, upstream web research), posting one GitHub review with diff-anchored inline comments signed "Claude Agent (CR)", and re-run deduplication. - review-open-prs: a thin orchestrator that fans review-pr out over all open PRs via parallel worktree-isolated agents, then verifies each review landed on the right PR, cleans up the worktrees and produces a cross-PR severity summary. Captures the gotchas hit in practice: shared-scratchpad payload collisions between parallel agents (unique payload filenames + anchor re-verification before POST), 422 on a single bad inline anchor, and PR-branch worktrees not being auto-removed. Co-Authored-By: Claude Fable 5 --- .claude/skills/review-open-prs/SKILL.md | 58 ++++++++++++++ .claude/skills/review-pr/SKILL.md | 102 ++++++++++++++++++++++++ CLAUDE.md | 4 + 3 files changed, 164 insertions(+) create mode 100644 .claude/skills/review-open-prs/SKILL.md create mode 100644 .claude/skills/review-pr/SKILL.md diff --git a/.claude/skills/review-open-prs/SKILL.md b/.claude/skills/review-open-prs/SKILL.md new file mode 100644 index 0000000..d1ecb4d --- /dev/null +++ b/.claude/skills/review-open-prs/SKILL.md @@ -0,0 +1,58 @@ +--- +name: review-open-prs +description: Fan-out code review of multiple/all open PRs - one parallel worktree-isolated agent per PR, each following the review-pr skill protocol, then cross-PR verification, cleanup and summary. Use when asked to review all open PRs or several PRs at once; a single PR needs only review-pr. +--- + +# Multi-agent review of open PRs + +Orchestration only — the per-PR review protocol lives in +`.claude/skills/review-pr/SKILL.md` and is NOT duplicated here. + +## 1. Discover the PRs and the stack topology + +```bash +gh pr list --state open --json number,title,headRefName,baseRefName,url --limit 50 +``` + +Note which PRs are stacked (`baseRefName` ≠ main) — pass each PR's base branch into its +agent prompt so the "own diff only" rule is explicit. + +## 2. Launch one agent per PR + +Agent tool, all launches in a single message (parallel), each with +`isolation: "worktree"` so agents can `gh pr checkout` and run the test suite without +touching the main checkout or each other. + +Each agent's prompt: + +- "You are an expert Kubernetes/Helm code reviewer. Review PR # (, base + `<baseRefName>`) and post the review to GitHub." +- "Follow the protocol in `.claude/skills/review-pr/SKILL.md` — read it first; the + **(parallel)** notes apply to you." +- PR-specific hypotheses worth chasing (see the review-focus section of `review-pr`) — + tailor them per PR; this is what separates real findings from style nits. +- "Return a severity-ranked findings report, verification results, and the posted + review URL." + +## 3. After all agents finish + +1. **Verify every review landed on the right PR** (the shared-scratchpad collision + described in `review-pr` makes this non-optional): + + ```bash + for pr in <numbers>; do echo "=== PR #$pr ==="; \ + gh api repos/shopsys/helm/pulls/$pr/reviews \ + --jq '.[] | select(.body | contains("Claude Agent (CR)")) | "\(.id) [\(.state)]"'; done + ``` + +2. **Clean up the agent worktrees** (a PR-branch checkout marks them "changed", so they + are not auto-removed): + + ```bash + git worktree list | grep agent- | awk '{print $1}' \ + | while read wt; do git worktree remove --force "$wt"; done + git branch | grep worktree-agent | xargs -r git branch -D + ``` + +3. **Summarize for the user**: cross-PR severity ranking (lead with the worst finding + overall, not per-PR), per-PR test results, and links to all posted reviews. diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md new file mode 100644 index 0000000..06002f8 --- /dev/null +++ b/.claude/skills/review-pr/SKILL.md @@ -0,0 +1,102 @@ +--- +name: review-pr +description: Detailed code review of ONE pull request - scope vs its (possibly stacked) base, verification-first findings (render + tests + web research), posting a GitHub review with anchored inline comments signed "Claude Agent (CR)". Use when asked to review a specific PR; for all open PRs at once use review-open-prs, which fans this out. +--- + +# Code review of a single PR + +Works standalone in the main checkout, or as the per-agent protocol inside a +`review-open-prs` fan-out. When running as a parallel agent, the **(parallel)** notes apply. + +## 1. Context before judgment + +Read `CLAUDE.md` and `.claude/skills/helm-kubernetes-guidelines/SKILL.md` first. Most +"obvious improvements" in this repo are intentional legacy-parity decisions — check +`docs/migrating-from-shopsys-deployment.md` before flagging behavior as wrong. + +## 2. Scope the diff + +```bash +gh pr view <N> --json title,body,baseRefName,files +gh pr diff <N> +``` + +Check `baseRefName`: PRs here are often **stacked** (base = another PR's branch). +Review ONLY this PR's own diff — `gh pr diff <N>` already diffs against the PR's base. +Never attribute base-branch changes to this PR. + +Then `gh pr checkout <N>` to inspect full files in context — the diff alone hides +interactions (hook ordering, helper includes, values layering). +**(parallel)** Only ever checkout inside your own isolated worktree. + +## 3. Review focus + +- **Will it work** — render + runtime reasoning, not just YAML shape. Form PR-specific + hypotheses and chase them (e.g. "the hook that scales cron needs an API token — does it + still get one?", "does nginx actually start as non-root with a read-only rootfs?"). + A pointed hypothesis finds real bugs; generic scanning finds style nits. +- **Completeness** — values.schema.json, docs/values.md, deviations doc entry, golden + snapshots regenerated in the SAME commit, helm-unittest coverage for new values paths. +- **Security** — least privilege, PSS "restricted" alignment, secret handling, footguns + that fail open (e.g. a mistyped toggle key silently ignored because the schema doesn't + know it). +- **Improvements** — override/null-merge behavior (verify by actually rendering the + override), consistency with the standard component keys, docs accuracy. + +## 4. Verify, don't speculate + +```bash +helm dependency build charts/shopsys-infra && helm dependency build charts/shopsys-app +./tests/run-golden-tests.sh +helm unittest charts/shopsys-app charts/shopsys-infra +# manifest-shape changes: kubeconform the rendered golden output (see CLAUDE.md) +``` + +- Upstream claims (Kubernetes semantics, ingress-nginx behavior, CVEs, image UIDs) get + confirmed via WebSearch/WebFetch against primary docs, not memory. +- Only verified findings get posted. Anything needing a real cluster is explicitly marked + unverifiable — never presented as fact. + +## 5. Post the review + +One review — summary body + inline comments in a single API call: + +```bash +gh api repos/shopsys/helm/pulls/<N>/reviews -X POST --input review-pr<N>.json +# payload: {"event":"COMMENT","body":"<summary>", +# "comments":[{"path":"...","line":L,"side":"RIGHT","body":"..."}]} +``` + +- `event: COMMENT` — never APPROVE/REQUEST_CHANGES from an agent. +- Inline `line` values MUST be lines present in this PR's diff hunks (`side: RIGHT` for + added/context lines) — cross-check against `gh pr diff <N>` immediately before posting; + one bad anchor 422s the whole review. Unanchorable findings go into the summary body. +- Summary = what was reviewed, what was verified (commands run, sources consulted), + findings ranked by severity. A clean review is still posted, saying so explicitly. +- English (repo convention); EVERY inline comment and the summary end with the signature: + `— Claude Agent (CR)`. +- **Unique payload filename** (`review-pr<N>.json`, never `payload.json`). + **(parallel)** Sibling agents share the scratchpad and WILL overwrite a generic name + between write and POST (observed: 422 with the other PR's unresolvable paths/lines) — + re-verify the file's anchors right before the POST. +- Re-run? Check for an existing "Claude Agent (CR)" review first and post only findings + that are new — no duplicates. + +## 6. Report + +Return/deliver a severity-ranked findings list, the verification results (honest — failing +tests are reported as failing), and the posted review URL: + +```bash +gh api repos/shopsys/helm/pulls/<N>/reviews \ + --jq '.[] | select(.body | contains("Claude Agent (CR)")) | "\(.id) [\(.state)]"' +``` + +Transient GitHub API `i/o timeout` happens — retry after a few seconds before concluding +the review is missing. + +## Don'ts + +- Never resolve review threads (same rule as `watch-pr`). +- Never review the stacked siblings' changes. +- Never post speculation as fact. diff --git a/CLAUDE.md b/CLAUDE.md index 03308a4..5a9fa31 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -134,6 +134,10 @@ improvising: conventions (incl. stacked PRs) - `watch-pr` — CI watching, Copilot review triage protocol (incl. suppressed comments; never resolve threads yourself) +- `review-pr` — detailed code review of one PR: stacked-base scoping, verification-first + findings, GitHub review with inline comments signed "Claude Agent (CR)" +- `review-open-prs` — fan-out of `review-pr` over multiple/all open PRs via parallel + worktree-isolated agents, plus cross-PR verification, cleanup and summary - `implement-issue` — issue lifecycle: triage, scope-deviation records, closing retrospective - `create-issue` — backlog conventions: body structure, title prefixes, label taxonomy - `sync-upstream-deployment` — porting changes from the legacy shopsys/deployment package