diff --git a/.claude/skills/review-fix-merge-pr-advanced/SKILL.md b/.claude/skills/review-fix-merge-pr-advanced/SKILL.md new file mode 100644 index 00000000..9bc0d1cf --- /dev/null +++ b/.claude/skills/review-fix-merge-pr-advanced/SKILL.md @@ -0,0 +1,71 @@ +--- +name: review-fix-merge-pr-advanced +description: Multi-lens PR review that fans out parallel subagents each backed by a specialized skill (security-audit, performance, code-quality, testing-strategy, correctness), adversarially verifies findings, fixes them, loops until clean, then squash-merges. Use when the user wants a thorough, multi-dimension review-and-merge of a PR — security + performance + best practices + tests — not just a single reviewer. +--- + +# Advanced Multi-Lens Review → Verify → Fix → Merge a PR + +The thorough sibling of `review-fix-merge-pr`. Instead of one reviewer, it runs **one reviewer +per dimension in parallel**, each loading a specialized skill, then **adversarially verifies** +every finding before a fixer touches code — so noisy lens output never causes churn. Loops +review→verify→fix until a round yields zero confirmed blocking findings, then merges. + +## When to use + +The user wants a deep, multi-dimension review — "review it for security and performance and +best practices then fix and merge", "thorough review", "audit this PR across the board". For a +quick single-reviewer pass, use `review-fix-merge-pr` instead. + +## The lenses (default) + +Each maps to a repo skill; the reviewer subagent loads it (via the Skill tool, or by reading +`.claude/skills//SKILL.md`) and applies its methodology to the diff: + +| Lens | Skill | Looks for | +|--------------|--------------------|-----------| +| correctness | *(general)* | logic bugs, edge cases, missed call sites, regressions, test rigor | +| security | `security-audit` | injection, secrets, unsafe deserialization, crypto, authz, tainted input | +| performance | `performance` | hot-path regressions, N+1/repeated I/O, allocations, blocking async, complexity | +| quality | `code-quality` | ruff/mypy issues, typing, Pythonic idioms, readability | +| testing | `testing-strategy` | coverage of the change, edge/negative cases, brittle assertions | + +Override via the `dimensions` arg — e.g. add `{key:'api', skill:'api-design', focus:'...'}` or +drop lenses that don't apply to the PR. + +## Steps + +1. **Resolve the PR** (argument, or the current branch's open PR via + `gh pr list --head "$(git branch --show-current)" --state open --json number,baseRefName`). + Stop and tell the user if none. +2. **Check out** the PR branch (`gh pr checkout `), note the base branch, and write a + one-sentence **intent** from the diff/commits. +3. **Pick the lenses.** Default set above; trim lenses that are irrelevant to the change (e.g. + skip `performance` on a docs-only PR) and add `api-design`/`library-review` if apt. +4. **Run the workflow:** + ``` + Workflow({ + scriptPath: "/advanced-review-fix-loop.js", + args: { prNumber: , baseBranch: "", intent: "", + maxRounds: 4 /*, dimensions: [...], testCmd: "..." */ } + }) + ``` + It fans out the lenses in parallel each round, dedups, verifies each candidate finding with + a refute-first skeptic, fixes the confirmed ones (running tests + linter), and loops. It + never commits or pushes. +5. **Report** from the returned `history`: per-round raw→deduped→confirmed counts, the confirmed + findings by lens/severity, and what the fixer changed. Read the result — never fabricate. +6. **Merge decision:** + - If `approved === true` (0 confirmed blocking): if the fixer changed files, commit + push, + wait for green CI (`gh pr checks `), then `gh pr merge --squash`. + - If `finalBlockingCount > 0` after `maxRounds`: **do not merge** — surface the blockers. +7. **Confirm** merged state and list any leftover `minor`/`nit` findings as optional follow-ups. + +## Notes + +- Blocking = `blocker`|`major` (post-verification). `minor` is fixed opportunistically; `nit` + is reported, never blocks, never verified. +- The parallel fan-out is a genuine barrier: dedup needs every lens before verify/fix. +- Reviewers/verifiers are read-only in spirit; only the single fixer per round edits files, so + no worktree isolation is needed. Concurrency is capped by the Workflow runtime. +- More tokens than `review-fix-merge-pr` (N reviewers + M verifiers per round). Use it when + thoroughness matters; use the basic skill for quick merges. diff --git a/.claude/skills/review-fix-merge-pr-advanced/advanced-review-fix-loop.js b/.claude/skills/review-fix-merge-pr-advanced/advanced-review-fix-loop.js new file mode 100644 index 00000000..8c0f9ddf --- /dev/null +++ b/.claude/skills/review-fix-merge-pr-advanced/advanced-review-fix-loop.js @@ -0,0 +1,202 @@ +// Advanced multi-lens review<->fix loop for a PR branch that is ALREADY checked out. +// +// Pipeline per round: +// 1. FAN OUT one reviewer per dimension, in parallel. Each reviewer loads a specialized +// skill (security-audit, performance, code-quality, testing-strategy, ...) and applies +// that methodology to the PR diff. (barrier: we need every lens before dedup) +// 2. DEDUP findings across lenses (same file+line+gist collapse to one). +// 3. ADVERSARIAL VERIFY each blocking/minor finding with a skeptic that tries to refute it, +// so noisy lens output (false positives) never reaches the fixer. +// 4. FIX the confirmed findings with one fixer subagent; it runs tests + linters. +// 5. LOOP back to review with prior-round context until a round yields 0 confirmed +// blocking findings, or maxRounds is hit. +// +// args: { +// prNumber, baseBranch="main", intent, maxRounds=4, testCmd, +// dimensions: [ {key, skill|null, focus} ] // optional override of the default lens set +// } +// Returns { finalBlockingCount, approved, rounds, history }. + +export const meta = { + name: 'advanced-review-fix-loop', + description: 'Multi-lens (security/perf/quality/testing) skill-backed PR review, verify, fix, loop', + phases: [ + { title: 'Review' }, + { title: 'Verify' }, + { title: 'Fix' }, + ], +} + +const prNumber = args?.prNumber ?? '(current branch)' +const baseBranch = args?.baseBranch ?? 'main' +const intent = args?.intent ?? '(no explicit intent given -- infer from diff + commit messages)' +const MAX_ROUNDS = Number(args?.maxRounds ?? 4) +const testCmd = args?.testCmd ?? '(discover from pyproject.toml / Makefile -- typically `python -m pytest -q`)' + +// Each lens names a skill to load. skill=null means "rigorous general reviewer, no skill". +const DEFAULT_DIMENSIONS = [ + { key: 'correctness', skill: null, focus: 'logic bugs, off-by-one, edge cases, missed call sites, regression risk to unrelated paths, and whether added tests would actually fail under the OLD code' }, + { key: 'security', skill: 'security-audit', focus: 'injection, hardcoded secrets, unsafe deserialization, weak crypto, authz/authn gaps, unsafe subprocess/eval, and tainted-input flow introduced by this diff' }, + { key: 'performance', skill: 'performance', focus: 'hot-path regressions, N+1 / repeated I/O, needless allocations or copies, blocking calls in async code, and algorithmic-complexity changes' }, + { key: 'quality', skill: 'code-quality', focus: 'ruff/mypy violations, missing or wrong type hints, non-Pythonic idioms, dead code, and readability/maintainability of the new code' }, + { key: 'testing', skill: 'testing-strategy', focus: 'coverage of the changed behavior, missing edge/negative/property cases, brittle assertions, and whether the tests pin the intended contract' }, +] +const DIMENSIONS = Array.isArray(args?.dimensions) && args.dimensions.length ? args.dimensions : DEFAULT_DIMENSIONS + +const REVIEW_SCHEMA = { + type: 'object', + additionalProperties: false, + properties: { + verdict: { type: 'string', enum: ['approve', 'request_changes'] }, + summary: { type: 'string' }, + findings: { + type: 'array', + items: { + type: 'object', + additionalProperties: false, + properties: { + severity: { type: 'string', enum: ['blocker', 'major', 'minor', 'nit'] }, + file: { type: 'string' }, + line: { type: 'integer' }, + summary: { type: 'string' }, + suggested_fix: { type: 'string' }, + }, + required: ['severity', 'file', 'summary', 'suggested_fix'], + }, + }, + }, + required: ['verdict', 'summary', 'findings'], +} + +const VERDICT_SCHEMA = { + type: 'object', + additionalProperties: false, + properties: { + real: { type: 'boolean', description: 'true only if this is a genuine, in-scope defect worth fixing in THIS PR' }, + severity: { type: 'string', enum: ['blocker', 'major', 'minor', 'nit'] }, + reason: { type: 'string' }, + }, + required: ['real', 'severity', 'reason'], +} + +const FIX_SCHEMA = { + type: 'object', + additionalProperties: false, + properties: { + changes_made: { type: 'string' }, + tests_passed: { type: 'boolean' }, + lint_passed: { type: 'boolean' }, + output_tail: { type: 'string' }, + unaddressed: { type: 'string' }, + }, + required: ['changes_made', 'tests_passed', 'lint_passed', 'output_tail', 'unaddressed'], +} + +const BLOCKING = new Set(['blocker', 'major']) +const key = (f) => `${(f.file || '').trim()}:${f.line || 0}:${(f.summary || '').slice(0, 60).toLowerCase()}` + +const skillClause = (skill) => + skill + ? `Apply the "${skill}" skill's methodology. Invoke it via the Skill tool if available; otherwise read .claude/skills/${skill}/SKILL.md (or ~/.claude/skills/${skill}/SKILL.md) and follow it. Run the concrete checks/tools that skill prescribes against the changed files where it makes sense.` + : `Act as a rigorous general correctness reviewer -- no skill needed.` + +const reviewPrompt = (dim, round, prior) => `You are the **${dim.key}** reviewer for PR ${prNumber} (branch already checked out). + +${skillClause(dim.skill)} + +Steps: +1. Run: git diff ${baseBranch}...HEAD -- read the full diff. +2. Read surrounding code for the changed hunks so you review with context, and trace changed values to every consumer. +3. Review ONLY through the **${dim.key}** lens: ${dim.focus}. + +Stated intent of this PR: ${intent} +This is round ${round}.${prior} + +Report via the schema. Only report findings that are genuinely in-scope for THIS diff -- do not file pre-existing tech debt in untouched code. Set verdict=approve if you find no blocker/major issues in your lens. Every finding needs a concrete suggested_fix.` + +const verifyPrompt = (f) => `Adversarially verify this code-review finding on PR ${prNumber} (branch checked out). Try to REFUTE it. Read the actual code at ${f.file}${f.line ? ':' + f.line : ''} and the diff (git diff ${baseBranch}...HEAD) before deciding. + +Finding [${f.severity}] from the ${f.lens} lens: ${f.summary} +Proposed fix: ${f.suggested_fix} + +Decide: is this a REAL, in-scope defect that should be fixed in this PR? Default to real=false if it is speculative, pre-existing in untouched code, a pure style preference dressed up as a bug, or not actually reachable. If real, set the severity you believe is correct (you may downgrade/upgrade).` + +const fixPrompt = (findings) => `You are fixing VERIFIED review findings on the checked-out PR branch (${prNumber}). + +Address these (blocker/major mandatory; fix minor when safe and clear): + +${findings.map((f, i) => `${i + 1}. [${f.severity}] (${f.lens}) ${f.file}${f.line ? ':' + f.line : ''} -- ${f.summary}\n Suggested: ${f.suggested_fix}`).join('\n\n')} + +Rules: +- Minimal correct changes only; no unrelated refactors. +- After editing: run tests (${testCmd}) AND the linter (ruff/mypy if configured -- check pyproject.toml). Capture results. +- Do NOT commit or push. + +Report via the schema.` + +let round = 1 +let prior = '' +const history = [] +let lastConfirmedBlocking = 0 + +while (round <= MAX_ROUNDS) { + phase('Review') + // 1. Fan out lenses in parallel -- barrier: dedup needs every lens' output. + const reviews = await parallel( + DIMENSIONS.map((d) => () => + agent(reviewPrompt(d, round, prior), { label: `review:${d.key}:r${round}`, phase: 'Review', schema: REVIEW_SCHEMA }) + .then((r) => ({ dim: d.key, review: r })) + ) + ) + const raw = reviews + .filter(Boolean) + .flatMap(({ dim, review }) => (review?.findings || []).map((f) => ({ ...f, lens: dim }))) + + // 2. Dedup across lenses. + const seen = new Map() + for (const f of raw) if (!seen.has(key(f))) seen.set(key(f), f) + const deduped = [...seen.values()] + const candidates = deduped.filter((f) => BLOCKING.has(f.severity) || f.severity === 'minor') + log(`Round ${round}: ${raw.length} raw findings -> ${deduped.length} deduped, ${candidates.length} candidates to verify`) + + // 3. Adversarial verify (skip nits). + phase('Verify') + const verified = ( + await parallel( + candidates.map((f) => () => + agent(verifyPrompt(f), { label: `verify:${f.lens}:${(f.file || '').split('/').pop()}`, phase: 'Verify', schema: VERDICT_SCHEMA }) + .then((v) => (v?.real ? { ...f, severity: v.severity || f.severity, verify_reason: v.reason } : null)) + ) + ) + ).filter(Boolean) + const confirmedBlocking = verified.filter((f) => BLOCKING.has(f.severity)) + lastConfirmedBlocking = confirmedBlocking.length + history.push({ round, raw: raw.length, deduped: deduped.length, confirmed: verified, confirmedBlocking: confirmedBlocking.length }) + log(`Round ${round}: ${verified.length} confirmed (${confirmedBlocking.length} blocking)`) + + if (confirmedBlocking.length === 0) { + log(`Round ${round}: no confirmed blocking findings. Loop complete.`) + break + } + if (round === MAX_ROUNDS) { + log(`Hit maxRounds=${MAX_ROUNDS} with ${confirmedBlocking.length} blocking findings unresolved.`) + break + } + + // 4. Fix confirmed blocking + minor. + phase('Fix') + const toFix = verified.filter((f) => BLOCKING.has(f.severity) || f.severity === 'minor') + const fix = await agent(fixPrompt(toFix), { label: `fix:r${round}`, phase: 'Fix', schema: FIX_SCHEMA }) + history[history.length - 1].fix = fix + log(`Round ${round} fix: tests=${fix?.tests_passed} lint=${fix?.lint_passed}. ${fix?.changes_made?.slice(0, 120)}`) + prior = `\n\nPRIOR ROUND: a fixer applied: ${fix?.changes_made}. Unaddressed: ${fix?.unaddressed || 'none'}. Re-verify these landed correctly and hunt for regressions your lens cares about.` + round++ +} + +return { + approved: lastConfirmedBlocking === 0, + finalBlockingCount: lastConfirmedBlocking, + rounds: history.length, + dimensions: DIMENSIONS.map((d) => d.key), + history, +} diff --git a/.claude/skills/review-fix-merge-pr/SKILL.md b/.claude/skills/review-fix-merge-pr/SKILL.md new file mode 100644 index 00000000..60eec174 --- /dev/null +++ b/.claude/skills/review-fix-merge-pr/SKILL.md @@ -0,0 +1,62 @@ +--- +name: review-fix-merge-pr +description: Review a GitHub PR with a subagent, fix the findings with another subagent, and loop review<->fix until the branch is clean, then squash-merge. Use when the user wants a PR (by number, or the current branch's PR) automatically reviewed, fixed, and merged. +--- + +# Review → Fix → Merge a PR + +Runs an adversarial review↔fix loop on a PR branch using the `Workflow` tool, then merges +once the loop comes back approved with no blocking findings. + +## When to use + +The user asks to review-and-merge a PR, "review then fix then merge", or a self-correcting +review loop on a PR. Works with a PR number (`/review-fix-merge-pr 126`) or, with no argument, +the PR for the current branch. + +## Steps + +1. **Resolve the target PR.** + - If a PR number was given, use it. + - Otherwise: `gh pr list --head "$(git branch --show-current)" --state open --json number,title,headRefName,baseRefName`. + - If none is found, tell the user and stop. + +2. **Check out the PR branch** (so the fixer edits the right files): + `gh pr checkout `. Note the base branch (usually `main`) and skim the diff / + commit messages to state the PR's **intent** in one sentence — the reviewer uses it as + ground truth. + +3. **Run the loop workflow.** Invoke `Workflow` with the bundled script and `args`: + + ``` + Workflow({ + scriptPath: "/review-fix-loop.js", + args: { prNumber: , baseBranch: "", intent: "" } + }) + ``` + + Optional args: `maxRounds` (default 5), `testCmd` (override the fixer's test command). + The workflow spawns one **review** subagent and, per round with blocking findings, one + **fix** subagent, looping until `verdict=approve` with 0 blocking (`blocker`/`major`) + findings — or `maxRounds` is hit. It never commits or pushes. + +4. **Report the findings** from the returned `history` to the user (verdict, each round's + findings by severity, what the fixer changed). Do not fabricate — read the result. + +5. **Decide on merge:** + - If `finalVerdict === "approve"` and `finalBlockingCount === 0`: + - If the fixer changed files, commit them on the branch and push + (`git commit -am "fix: address review findings" && git push`), then wait for CI. + - Confirm CI is green (`gh pr checks `), then squash-merge: `gh pr merge --squash`. + - If blocking findings remain after `maxRounds`, **do NOT merge** — surface them and stop. + +6. **Confirm** the merged state (`gh pr view --json state,mergedAt,url`) back to the user, + and mention any non-blocking `minor`/`nit` findings left as optional follow-ups. + +## Notes + +- Blocking = `blocker` or `major`. `minor` findings are fixed opportunistically; `nit`s are + reported but never block a merge. +- The review and fix agents share the working directory and run sequentially (one fixer at a + time), so no worktree isolation is needed. +- To iterate on the loop logic, edit `review-fix-loop.js` in this skill directory. diff --git a/.claude/skills/review-fix-merge-pr/review-fix-loop.js b/.claude/skills/review-fix-merge-pr/review-fix-loop.js new file mode 100644 index 00000000..55bc4724 --- /dev/null +++ b/.claude/skills/review-fix-merge-pr/review-fix-loop.js @@ -0,0 +1,141 @@ +// Parameterized review<->fix loop for a PR branch that is ALREADY checked out. +// +// args: { +// prNumber: number | string (for labels/reporting only; the diff comes from git) +// baseBranch: string (default "main") -- the merge base to diff against +// intent: string (optional) -- what the PR is supposed to accomplish, +// fed to the reviewer as ground truth +// maxRounds: number (default 5) +// testCmd: string (optional) -- override the test command the fixer runs +// } +// +// Returns { finalVerdict, finalBlockingCount, rounds, history }. +// Blocking = severity blocker|major. Minor findings are fixed opportunistically; nits are +// reported but never block. The caller (skill) decides whether to merge. + +export const meta = { + name: 'review-fix-loop', + description: 'Review a checked-out PR branch, fix findings, loop review<->fix until clean', + phases: [ + { title: 'Review' }, + { title: 'Fix' }, + ], +} + +const prNumber = args?.prNumber ?? '(current branch)' +const baseBranch = args?.baseBranch ?? 'main' +const intent = args?.intent ?? '(no explicit intent provided -- infer it from the diff and commit messages)' +const MAX_ROUNDS = Number(args?.maxRounds ?? 5) +const testCmd = args?.testCmd ?? '(discover from pyproject.toml / Makefile -- typically `python -m pytest -q`)' + +const REVIEW_SCHEMA = { + type: 'object', + additionalProperties: false, + properties: { + verdict: { type: 'string', enum: ['approve', 'request_changes'] }, + summary: { type: 'string', description: 'One-paragraph overall assessment' }, + findings: { + type: 'array', + items: { + type: 'object', + additionalProperties: false, + properties: { + severity: { type: 'string', enum: ['blocker', 'major', 'minor', 'nit'] }, + file: { type: 'string' }, + line: { type: 'integer' }, + summary: { type: 'string' }, + suggested_fix: { type: 'string' }, + }, + required: ['severity', 'file', 'summary', 'suggested_fix'], + }, + }, + }, + required: ['verdict', 'summary', 'findings'], +} + +const FIX_SCHEMA = { + type: 'object', + additionalProperties: false, + properties: { + changes_made: { type: 'string' }, + tests_passed: { type: 'boolean' }, + test_output_tail: { type: 'string' }, + unaddressed: { type: 'string', description: 'Findings NOT addressed and why; empty if all addressed' }, + }, + required: ['changes_made', 'tests_passed', 'test_output_tail', 'unaddressed'], +} + +const BLOCKING = new Set(['blocker', 'major']) + +const reviewPrompt = (round, priorContext) => `You are a rigorous code reviewer reviewing PR ${prNumber} (branch is already checked out in the working directory). + +Steps: +1. Run: git diff ${baseBranch}...HEAD -- read the FULL diff. +2. Read the surrounding code for each changed hunk so you review with context, not just the diff. Trace any changed value through to every place it is consumed. +3. Read any added/changed tests and judge whether they actually lock in the intended behavior (would they FAIL under the old code?). + +Stated intent of this PR: ${intent} + +Review for: correctness, edge cases, missed call sites / surfaces, regression risk to unrelated paths, and test-coverage adequacy. Do NOT invent style nits. This is round ${round} of the review loop.${priorContext} + +Report via the structured schema. Set verdict=approve ONLY if there are no blocker/major findings. Every finding needs a concrete, actionable suggested_fix.` + +const fixPrompt = (findings) => `You are fixing code review findings on the checked-out PR branch (${prNumber}). + +Address these findings (blocker/major are mandatory; also fix minor when the fix is safe and clear): + +${findings.map((f, i) => `${i + 1}. [${f.severity}] ${f.file}${f.line ? ':' + f.line : ''} -- ${f.summary}\n Suggested: ${f.suggested_fix}`).join('\n\n')} + +Rules: +- Make the minimal correct change. Do not refactor unrelated code. +- After editing, run the tests for the affected area: ${testCmd}. Capture the result. +- Do NOT commit or push. Just edit files. + +Report via the schema: what you changed, whether tests passed, the test output tail, and anything you did NOT address (with reason).` + +phase('Review') +let round = 1 +let priorContext = '' +let lastReview = null +const history = [] + +while (round <= MAX_ROUNDS) { + const review = await agent(reviewPrompt(round, priorContext), { + label: `review:round-${round}`, + phase: 'Review', + schema: REVIEW_SCHEMA, + }) + lastReview = review + const blocking = (review?.findings || []).filter((f) => BLOCKING.has(f.severity)) + log(`Round ${round}: verdict=${review?.verdict}, findings=${review?.findings?.length || 0} (${blocking.length} blocking)`) + history.push({ round, verdict: review?.verdict, findings: review?.findings || [], summary: review?.summary }) + + if (review?.verdict === 'approve' && blocking.length === 0) { + log(`Approved on round ${round}. Loop complete.`) + break + } + if (round === MAX_ROUNDS) { + log(`Hit maxRounds=${MAX_ROUNDS} with unresolved blocking findings; stopping loop.`) + break + } + + phase('Fix') + const toFix = review.findings.filter((f) => BLOCKING.has(f.severity) || f.severity === 'minor') + const fix = await agent(fixPrompt(toFix), { + label: `fix:round-${round}`, + phase: 'Fix', + schema: FIX_SCHEMA, + }) + history[history.length - 1].fix = fix + log(`Round ${round} fix: tests_passed=${fix?.tests_passed}. ${fix?.changes_made?.slice(0, 120)}`) + priorContext = `\n\nPRIOR ROUND CONTEXT: in round ${round} a fixer applied: ${fix?.changes_made}. Unaddressed: ${fix?.unaddressed || 'none'}. Re-verify these were done correctly and check for regressions.` + phase('Review') + round++ +} + +return { + finalVerdict: lastReview?.verdict, + finalBlockingCount: (lastReview?.findings || []).filter((f) => BLOCKING.has(f.severity)).length, + rounds: history.length, + history, +}