diff --git a/.changes/review/feat-8329-quality-gate-code-design-mode-4c7d2e91.md b/.changes/review/feat-8329-quality-gate-code-design-mode-4c7d2e91.md new file mode 100644 index 00000000000..c82066e4bad --- /dev/null +++ b/.changes/review/feat-8329-quality-gate-code-design-mode-4c7d2e91.md @@ -0,0 +1,7 @@ +--- +bump: minor +--- + +### Added + +- `/review:quality-gate code-design` reviews a change for design that makes the next change costly. It loads the consumer's review criteria through the standards resolution ladder and falls back to small bundled criteria (Duplicated Code, Shotgun Surgery, Speculative Generality), run by a fresh-context `review:brief-reviewer`. Repetition is CRITICAL only when the diff introduces a copy in code that no shared source or sync check keeps aligned, and the finding names the repeated fact and the later change that would make the copies diverge; Markdown repetition, change cascades and the `prototype` quality bar stay SUGGESTION. A consumer standard that contradicts a bundled default wins, and the report says `Overrides bundled default per `; with no quality bar given, the review rates as production and says so. The mode takes the phase-boundary inputs like every other. diff --git a/plugins/review/evals/qg-code-design-cascade/case.yaml b/plugins/review/evals/qg-code-design-cascade/case.yaml new file mode 100644 index 00000000000..dcf20f166e5 --- /dev/null +++ b/plugins/review/evals/qg-code-design-cascade/case.yaml @@ -0,0 +1,4 @@ +schema_version: "1.1" +name: qg-code-design-cascade +context: + scaffold_script: scaffold.sh diff --git a/plugins/review/evals/qg-code-design-cascade/graders/brief-reviewer-dispatched.md b/plugins/review/evals/qg-code-design-cascade/graders/brief-reviewer-dispatched.md new file mode 100644 index 00000000000..8def59be691 --- /dev/null +++ b/plugins/review/evals/qg-code-design-cascade/graders/brief-reviewer-dispatched.md @@ -0,0 +1,5 @@ +--- +type: tool_used +tool: Agent +input_match: '"subagent_type"\s*:\s*"(?:review:)?brief-reviewer"' +--- diff --git a/plugins/review/evals/qg-code-design-cascade/graders/critical-names-threshold.md b/plugins/review/evals/qg-code-design-cascade/graders/critical-names-threshold.md new file mode 100644 index 00000000000..16283719eb2 --- /dev/null +++ b/plugins/review/evals/qg-code-design-cascade/graders/critical-names-threshold.md @@ -0,0 +1,4 @@ +--- +type: regex +pattern: '(?:^|\n)[ \t>]*(?:(?:[-*+]|\d+[.)])[ \t]+)?(?:#+[ \t]+)?\**CRITICAL\b(?![ \t]*\**[ \t]*\d)[^\n]*(?:FREE_SHIPPING_THRESHOLD|(?>.git/info/exclude +mkdir -p src/cart src/checkout + +cat >src/pricing.js <<'JS' +export const FREE_SHIPPING_THRESHOLD = 50; +export const SHIPPING_FEE = 4.99; + +export function shippingFor(subtotal) { + return subtotal >= FREE_SHIPPING_THRESHOLD ? 0 : SHIPPING_FEE; +} +JS + +cat >package.json <<'JSON' +{ "name": "shop", "type": "module", "private": true } +JSON + +git add . +git commit -q -m "feat: shipping fee" +git rev-parse HEAD >.review-base +git checkout -q -b feat/cart-banner + +cat >src/cart/banner.js <<'JS' +export function freeShippingBanner(subtotal) { + const remaining = 50 - subtotal; + if (remaining <= 0) { + return "You get free shipping!"; + } + return `Add $${remaining.toFixed(2)} more for free shipping on orders over $50.`; +} +JS + +cat >src/checkout/summary.js <<'JS' +import { SHIPPING_FEE } from "../pricing.js"; + +export function summaryLines(subtotal) { + const shipping = subtotal >= 50 ? 0 : SHIPPING_FEE; + return [ + `Subtotal: $${subtotal.toFixed(2)}`, + `Shipping: ${shipping === 0 ? "free" : "$" + shipping.toFixed(2)}`, + `Total: $${(subtotal + shipping).toFixed(2)}`, + ]; +} +JS + +git add src +git commit -q -m "feat: free-shipping banner and checkout summary" diff --git a/plugins/review/evals/qg-code-design-markdown/case.yaml b/plugins/review/evals/qg-code-design-markdown/case.yaml new file mode 100644 index 00000000000..e8a6c230211 --- /dev/null +++ b/plugins/review/evals/qg-code-design-markdown/case.yaml @@ -0,0 +1,4 @@ +schema_version: "1.1" +name: qg-code-design-markdown +context: + scaffold_script: scaffold.sh diff --git a/plugins/review/evals/qg-code-design-markdown/graders/brief-reviewer-dispatched.md b/plugins/review/evals/qg-code-design-markdown/graders/brief-reviewer-dispatched.md new file mode 100644 index 00000000000..8def59be691 --- /dev/null +++ b/plugins/review/evals/qg-code-design-markdown/graders/brief-reviewer-dispatched.md @@ -0,0 +1,5 @@ +--- +type: tool_used +tool: Agent +input_match: '"subagent_type"\s*:\s*"(?:review:)?brief-reviewer"' +--- diff --git a/plugins/review/evals/qg-code-design-markdown/graders/markdown-repetition-is-suggestion.md b/plugins/review/evals/qg-code-design-markdown/graders/markdown-repetition-is-suggestion.md new file mode 100644 index 00000000000..4758c5c6673 --- /dev/null +++ b/plugins/review/evals/qg-code-design-markdown/graders/markdown-repetition-is-suggestion.md @@ -0,0 +1,4 @@ +--- +type: regex +pattern: '^(?![\s\S]*(?:(?:^|\n)[ \t>]*(?:(?:[-*+]|\d+[.)])[ \t]+)?(?:#+[ \t]+)?\**(?:CRITICAL|IMPORTANT)\b(?![ \t]*\**[ \t]*\d)[^\n]*(?:\b(?:setup|deploy|README)\.md\b)|(?:\|[ \t]*\**(?:CRITICAL|IMPORTANT)\**[ \t]*\||[(\[][ \t]*\**(?:CRITICAL|IMPORTANT)\**[ \t]*[)\]]|\bSeverity\**[ \t]*[:=][ \t]*\**(?:CRITICAL|IMPORTANT)\b)[^\n]*(?:\b(?:setup|deploy|README)\.md\b)|(?:\b(?:setup|deploy|README)\.md\b)[^\n]*(?:\|[ \t]*\**(?:CRITICAL|IMPORTANT)\**[ \t]*\||[(\[][ \t]*\**(?:CRITICAL|IMPORTANT)\**[ \t]*[)\]]|\bSeverity\**[ \t]*[:=][ \t]*\**(?:CRITICAL|IMPORTANT)\b)))[\s\S]*(?:\bSUGGESTION\b[^\n]*\b(?:setup|deploy|README)\.md\b|\b(?:setup|deploy|README)\.md\b[^\n]*\bSUGGESTION\b)' +--- diff --git a/plugins/review/evals/qg-code-design-markdown/graders/names-node-requirement.md b/plugins/review/evals/qg-code-design-markdown/graders/names-node-requirement.md new file mode 100644 index 00000000000..035f0654739 --- /dev/null +++ b/plugins/review/evals/qg-code-design-markdown/graders/names-node-requirement.md @@ -0,0 +1,7 @@ +--- +type: llm +--- + +PASS if the review reports, as a SUGGESTION, that docs/setup.md and docs/deploy.md each restate the README's requirement of Node.js 20 or later, and proposes keeping the fact in one place, such as linking to the README instead of repeating the version. + +FAIL if the review reports nothing about the repeated Node.js requirement; rates it CRITICAL, IMPORTANT or blocking; or flags a different repetition (the npm commands) as the only finding. diff --git a/plugins/review/evals/qg-code-design-markdown/graders/no-edit.md b/plugins/review/evals/qg-code-design-markdown/graders/no-edit.md new file mode 100644 index 00000000000..903f19fc372 --- /dev/null +++ b/plugins/review/evals/qg-code-design-markdown/graders/no-edit.md @@ -0,0 +1,6 @@ +--- +type: tool_used +tool: Edit +min: 0 +max: 0 +--- diff --git a/plugins/review/evals/qg-code-design-markdown/prompt.md b/plugins/review/evals/qg-code-design-markdown/prompt.md new file mode 100644 index 00000000000..5f6d1e279f8 --- /dev/null +++ b/plugins/review/evals/qg-code-design-markdown/prompt.md @@ -0,0 +1,13 @@ +--- +description: "A Markdown-only branch restates the README's Node.js 20 requirement in two new docs pages: the code-design review reports the repetition as SUGGESTION, never CRITICAL or IMPORTANT" +tags: [review, quality-gate, code-design, repetition, markdown] +runs: 3 +max_turns: 40 +timeout_seconds: 900 +allowed_tools: [Read, Glob, Grep, Skill, Agent, Bash, Write, Edit] +expected_outcome: "The gate dispatches review:brief-reviewer and reports that docs/setup.md and docs/deploy.md each restate the README's Node.js 20 requirement, rated SUGGESTION because the copies are Markdown, with a pointer to the README as the fix. No finding about them is CRITICAL or IMPORTANT, and nothing is edited" +--- + +I wrote setup and deploy guides on this branch. + +Run `/review:quality-gate code-design` on this branch with `--base` set to the SHA in `.review-base` and `--report-only`, and give me the report. diff --git a/plugins/review/evals/qg-code-design-markdown/samples/brief-reviewer-dispatched.json b/plugins/review/evals/qg-code-design-markdown/samples/brief-reviewer-dispatched.json new file mode 100644 index 00000000000..241eca83057 --- /dev/null +++ b/plugins/review/evals/qg-code-design-markdown/samples/brief-reviewer-dispatched.json @@ -0,0 +1,62 @@ +{ + "pass": [ + { + "answer": [ + { + "tool": "Agent", + "input": { + "description": "Code-design review", + "prompt": "You are reviewing a change for code design. Concern: code-design.", + "subagent_type": "review:brief-reviewer" + } + } + ], + "why": "the plugin's brief reviewer" + }, + { + "answer": [ + { + "tool": "Agent", + "input": { + "description": "Review", + "prompt": "Concern: code-design.", + "subagent_type": "brief-reviewer" + } + } + ], + "why": "unprefixed agent name" + } + ], + "fail": [ + { + "answer": [], + "why": "reviewed on the invoking thread" + }, + { + "answer": [ + { + "tool": "Agent", + "input": { + "description": "Review", + "prompt": "Review the design.", + "subagent_type": "general-purpose" + } + } + ], + "why": "a general-purpose agent can fan out" + }, + { + "answer": [ + { + "tool": "Agent", + "input": { + "description": "Review", + "prompt": "Review the code.", + "subagent_type": "review:code-reviewer" + } + } + ], + "why": "wrong reviewer for the code-design mode" + } + ] +} diff --git a/plugins/review/evals/qg-code-design-markdown/samples/markdown-repetition-is-suggestion.json b/plugins/review/evals/qg-code-design-markdown/samples/markdown-repetition-is-suggestion.json new file mode 100644 index 00000000000..aecd66fc280 --- /dev/null +++ b/plugins/review/evals/qg-code-design-markdown/samples/markdown-repetition-is-suggestion.json @@ -0,0 +1,30 @@ +{ + "pass": [ + { + "answer": "| 1 | SUGGESTION | high | Duplicated Code | docs/setup.md and docs/deploy.md restate the Node.js 20 requirement from README.md | docs/setup.md:3 | Link to the README |\n\nCRITICAL 0, IMPORTANT 0, SUGGESTION 1", + "why": "SUGGESTION row, counts line names no file" + }, + { + "answer": "Not CRITICAL or IMPORTANT: setup.md is Markdown.\n\n| 1 | SUGGESTION | high | Duplicated Code | docs/setup.md restates the Node.js 20 requirement from README.md | docs/setup.md:3 | Link to the README |", + "why": "negated mention of CRITICAL beside a file name" + } + ], + "fail": [ + { + "answer": "", + "why": "empty reply" + }, + { + "answer": "| 1 | CRITICAL | high | Duplicated Code | Node.js 20 restated | docs/setup.md:3 | Link to the README |", + "why": "Markdown repetition rated CRITICAL" + }, + { + "answer": "| 1 | IMPORTANT | high | Duplicated Code | Node.js 20 restated | docs/deploy.md:3 | Link to the README |", + "why": "Markdown repetition rated IMPORTANT" + }, + { + "answer": "No code-design issues found.", + "why": "repetition not reported" + } + ] +} diff --git a/plugins/review/evals/qg-code-design-markdown/samples/names-node-requirement.json b/plugins/review/evals/qg-code-design-markdown/samples/names-node-requirement.json new file mode 100644 index 00000000000..644f5275de5 --- /dev/null +++ b/plugins/review/evals/qg-code-design-markdown/samples/names-node-requirement.json @@ -0,0 +1,18 @@ +{ + "pass": [ + { + "answer": "| 1 | SUGGESTION | high | Duplicated Code | docs/setup.md:3 and docs/deploy.md:3 restate README.md's Node.js 20 requirement; a version bump would leave one stale | docs/setup.md:3 | Point at the README |", + "why": "fact, copies, SUGGESTION, pointer fix" + } + ], + "fail": [ + { + "answer": "No code-design issues found.", + "why": "misses the repetition" + }, + { + "answer": "| 1 | CRITICAL | high | Duplicated Code | Node.js 20 is restated in two guides | docs/setup.md:3 | Link to the README |", + "why": "rated CRITICAL" + } + ] +} diff --git a/plugins/review/evals/qg-code-design-markdown/samples/no-edit.json b/plugins/review/evals/qg-code-design-markdown/samples/no-edit.json new file mode 100644 index 00000000000..016533818f7 --- /dev/null +++ b/plugins/review/evals/qg-code-design-markdown/samples/no-edit.json @@ -0,0 +1,34 @@ +{ + "pass": [ + { + "answer": [], + "why": "edits nothing" + }, + { + "answer": [ + { + "tool": "Read", + "input": { + "file_path": "docs/setup.md" + } + } + ], + "why": "reads only" + } + ], + "fail": [ + { + "answer": [ + { + "tool": "Edit", + "input": { + "file_path": "docs/setup.md", + "old_string": "a", + "new_string": "b" + } + } + ], + "why": "fixes the finding instead of reporting it" + } + ] +} diff --git a/plugins/review/evals/qg-code-design-markdown/scaffold.sh b/plugins/review/evals/qg-code-design-markdown/scaffold.sh new file mode 100755 index 00000000000..a1b8cafeca4 --- /dev/null +++ b/plugins/review/evals/qg-code-design-markdown/scaffold.sh @@ -0,0 +1,45 @@ +#!/usr/bin/env bash +# main holds a README that states the Node.js requirement. The branch adds two docs pages that each +# restate it instead of pointing at the README. The change is Markdown only. The base commit SHA is in +# .review-base. +set -euo pipefail + +git init -q -b main +git config user.name eval +git config user.email eval@example.invalid +printf '.review-base\n' >>.git/info/exclude +mkdir -p docs + +cat >README.md <<'MD' +# ledger + +Requires Node.js 20 or later. + +Run `npm install`, then `npm start`. +MD + +git add . +git commit -q -m "docs: readme" +git rev-parse HEAD >.review-base +git checkout -q -b docs/setup-and-deploy + +cat >docs/setup.md <<'MD' +# Set up a development machine + +1. Install Node.js 20 or later. +2. Run `npm install`. +3. Run `npm start` and open http://localhost:3000. +MD + +cat >docs/deploy.md <<'MD' +# Deploy + +The build host needs Node.js 20 or later. + +1. Run `npm ci`. +2. Run `npm run build`. +3. Copy `dist/` to the web server. +MD + +git add docs +git commit -q -m "docs: setup and deploy guides" diff --git a/plugins/review/evals/qg-code-design-override/case.yaml b/plugins/review/evals/qg-code-design-override/case.yaml new file mode 100644 index 00000000000..75031eefe5f --- /dev/null +++ b/plugins/review/evals/qg-code-design-override/case.yaml @@ -0,0 +1,4 @@ +schema_version: "1.1" +name: qg-code-design-override +context: + scaffold_script: scaffold.sh diff --git a/plugins/review/evals/qg-code-design-override/graders/brief-reviewer-dispatched.md b/plugins/review/evals/qg-code-design-override/graders/brief-reviewer-dispatched.md new file mode 100644 index 00000000000..8def59be691 --- /dev/null +++ b/plugins/review/evals/qg-code-design-override/graders/brief-reviewer-dispatched.md @@ -0,0 +1,5 @@ +--- +type: tool_used +tool: Agent +input_match: '"subagent_type"\s*:\s*"(?:review:)?brief-reviewer"' +--- diff --git a/plugins/review/evals/qg-code-design-override/graders/emits-override-line.md b/plugins/review/evals/qg-code-design-override/graders/emits-override-line.md new file mode 100644 index 00000000000..fabcf918a3a --- /dev/null +++ b/plugins/review/evals/qg-code-design-override/graders/emits-override-line.md @@ -0,0 +1,4 @@ +--- +type: regex +pattern: 'Overrides bundled default\W{0,4}Duplicated Code\W{0,4}per\W{0,4}(?:\./)?docs/standards/service-boundaries\.md' +--- diff --git a/plugins/review/evals/qg-code-design-override/graders/no-blocking-copy-finding.md b/plugins/review/evals/qg-code-design-override/graders/no-blocking-copy-finding.md new file mode 100644 index 00000000000..9f6fa70cb8a --- /dev/null +++ b/plugins/review/evals/qg-code-design-override/graders/no-blocking-copy-finding.md @@ -0,0 +1,4 @@ +--- +type: regex +pattern: '^(?![\s\S]*(?:\b(?:CRITICAL|IMPORTANT)\b[^\n]*\bretry\.js\b|\bretry\.js\b[^\n]*\b(?:CRITICAL|IMPORTANT)\b))[\s\S]*\bOverrides bundled default\b' +--- diff --git a/plugins/review/evals/qg-code-design-override/graders/no-edit.md b/plugins/review/evals/qg-code-design-override/graders/no-edit.md new file mode 100644 index 00000000000..903f19fc372 --- /dev/null +++ b/plugins/review/evals/qg-code-design-override/graders/no-edit.md @@ -0,0 +1,6 @@ +--- +type: tool_used +tool: Edit +min: 0 +max: 0 +--- diff --git a/plugins/review/evals/qg-code-design-override/prompt.md b/plugins/review/evals/qg-code-design-override/prompt.md new file mode 100644 index 00000000000..0f8a37dd61f --- /dev/null +++ b/plugins/review/evals/qg-code-design-override/prompt.md @@ -0,0 +1,13 @@ +--- +description: "The repository's docs/standards/service-boundaries.md says a value two services need is copied into each on purpose; the branch copies billing's retry policy into the shipping service: the code-design review applies the consumer standard, reports no CRITICAL for the copy, and emits the override line naming Duplicated Code and that file" +tags: [review, quality-gate, code-design, standards, override] +runs: 3 +max_turns: 40 +timeout_seconds: 900 +allowed_tools: [Read, Glob, Grep, Skill, Agent, Bash, Write, Edit] +expected_outcome: "The gate resolves docs/standards/README.md, dispatches review:brief-reviewer with docs/standards/service-boundaries.md as consumer criteria, and its report carries the line Overrides bundled default Duplicated Code per docs/standards/service-boundaries.md. The copied MAX_RETRIES and withRetry in services/shipping/retry.js get no CRITICAL or IMPORTANT finding, and nothing is edited" +--- + +I added a retry policy to the shipping service on this branch. + +Run `/review:quality-gate code-design` on this branch with `--base` set to the SHA in `.review-base` and `--report-only`, and give me the report. diff --git a/plugins/review/evals/qg-code-design-override/samples/brief-reviewer-dispatched.json b/plugins/review/evals/qg-code-design-override/samples/brief-reviewer-dispatched.json new file mode 100644 index 00000000000..241eca83057 --- /dev/null +++ b/plugins/review/evals/qg-code-design-override/samples/brief-reviewer-dispatched.json @@ -0,0 +1,62 @@ +{ + "pass": [ + { + "answer": [ + { + "tool": "Agent", + "input": { + "description": "Code-design review", + "prompt": "You are reviewing a change for code design. Concern: code-design.", + "subagent_type": "review:brief-reviewer" + } + } + ], + "why": "the plugin's brief reviewer" + }, + { + "answer": [ + { + "tool": "Agent", + "input": { + "description": "Review", + "prompt": "Concern: code-design.", + "subagent_type": "brief-reviewer" + } + } + ], + "why": "unprefixed agent name" + } + ], + "fail": [ + { + "answer": [], + "why": "reviewed on the invoking thread" + }, + { + "answer": [ + { + "tool": "Agent", + "input": { + "description": "Review", + "prompt": "Review the design.", + "subagent_type": "general-purpose" + } + } + ], + "why": "a general-purpose agent can fan out" + }, + { + "answer": [ + { + "tool": "Agent", + "input": { + "description": "Review", + "prompt": "Review the code.", + "subagent_type": "review:code-reviewer" + } + } + ], + "why": "wrong reviewer for the code-design mode" + } + ] +} diff --git a/plugins/review/evals/qg-code-design-override/samples/emits-override-line.json b/plugins/review/evals/qg-code-design-override/samples/emits-override-line.json new file mode 100644 index 00000000000..273204979e5 --- /dev/null +++ b/plugins/review/evals/qg-code-design-override/samples/emits-override-line.json @@ -0,0 +1,26 @@ +{ + "pass": [ + { + "answer": "Quality bar: production (assumed: none given)\nCriteria: docs/standards/service-boundaries.md\nOverrides bundled default Duplicated Code per docs/standards/service-boundaries.md", + "why": "the exact line" + }, + { + "answer": "Overrides bundled default `Duplicated Code` per `docs/standards/service-boundaries.md`", + "why": "code-span formatting" + } + ], + "fail": [ + { + "answer": "", + "why": "empty reply" + }, + { + "answer": "The consumer standard permits copies across services, so the duplication is not reported.", + "why": "applies the standard without the override line" + }, + { + "answer": "Overrides bundled default Duplicated Code per README.md", + "why": "names the index, not the standards file" + } + ] +} diff --git a/plugins/review/evals/qg-code-design-override/samples/no-blocking-copy-finding.json b/plugins/review/evals/qg-code-design-override/samples/no-blocking-copy-finding.json new file mode 100644 index 00000000000..851b53ac4f8 --- /dev/null +++ b/plugins/review/evals/qg-code-design-override/samples/no-blocking-copy-finding.json @@ -0,0 +1,18 @@ +{ + "pass": [ + { + "answer": "Overrides bundled default Duplicated Code per docs/standards/service-boundaries.md\n\nNo code-design issues found.", + "why": "override applied, no finding" + } + ], + "fail": [ + { + "answer": "Overrides bundled default Duplicated Code per docs/standards/service-boundaries.md\n\n| 1 | CRITICAL | high | Duplicated Code | MAX_RETRIES copied | services/shipping/retry.js:1 | Share a module |", + "why": "line emitted but the copy still rated CRITICAL" + }, + { + "answer": "| 1 | IMPORTANT | high | Duplicated Code | withRetry copied from billing | services/shipping/retry.js:3 | Extract a shared package |", + "why": "rated IMPORTANT, no override" + } + ] +} diff --git a/plugins/review/evals/qg-code-design-override/samples/no-edit.json b/plugins/review/evals/qg-code-design-override/samples/no-edit.json new file mode 100644 index 00000000000..0ceb3a3c7b3 --- /dev/null +++ b/plugins/review/evals/qg-code-design-override/samples/no-edit.json @@ -0,0 +1,34 @@ +{ + "pass": [ + { + "answer": [], + "why": "edits nothing" + }, + { + "answer": [ + { + "tool": "Read", + "input": { + "file_path": "services/shipping/retry.js" + } + } + ], + "why": "reads only" + } + ], + "fail": [ + { + "answer": [ + { + "tool": "Edit", + "input": { + "file_path": "services/shipping/retry.js", + "old_string": "a", + "new_string": "b" + } + } + ], + "why": "fixes the finding instead of reporting it" + } + ] +} diff --git a/plugins/review/evals/qg-code-design-override/scaffold.sh b/plugins/review/evals/qg-code-design-override/scaffold.sh new file mode 100755 index 00000000000..f040f5e2fd5 --- /dev/null +++ b/plugins/review/evals/qg-code-design-override/scaffold.sh @@ -0,0 +1,70 @@ +#!/usr/bin/env bash +# main holds two independently deployed services and a standards index whose service-boundaries file +# says a value two services need is copied into each on purpose. The branch adds the shipping +# service's retry policy, copying MAX_RETRIES from billing. The bundled Duplicated Code default +# would rate that copy CRITICAL; the consumer standard permits it. The base commit SHA is in +# .review-base. +set -euo pipefail + +git init -q -b main +git config user.name eval +git config user.email eval@example.invalid +printf '.review-base\n' >>.git/info/exclude +mkdir -p docs/standards services/billing services/shipping + +cat >docs/standards/README.md <<'MD' +--- +standards-contract: 1.1.0 +--- + +# Standards index + +| Surface | Applies when | File | +|---|---|---| +| service-boundaries | code design, duplication, or reviewing any change under services/ | service-boundaries.md | +MD + +cat >docs/standards/service-boundaries.md <<'MD' +# Service boundaries + +Each directory under `services/` deploys on its own and never imports code from another service. + +A value two services both need is copied into each service on purpose, so each can change it on its +own release. A copy across services is not a review finding. A module shared across services is. +MD + +cat >services/billing/retry.js <<'JS' +export const MAX_RETRIES = 3; + +export async function withRetry(fn) { + for (let attempt = 1; ; attempt++) { + try { + return await fn(); + } catch (err) { + if (attempt >= MAX_RETRIES) throw err; + } + } +} +JS + +git add . +git commit -q -m "feat: billing retry policy" +git rev-parse HEAD >.review-base +git checkout -q -b feat/shipping-retry + +cat >services/shipping/retry.js <<'JS' +export const MAX_RETRIES = 3; + +export async function withRetry(fn) { + for (let attempt = 1; ; attempt++) { + try { + return await fn(); + } catch (err) { + if (attempt >= MAX_RETRIES) throw err; + } + } +} +JS + +git add services +git commit -q -m "feat: shipping retry policy" diff --git a/plugins/review/evals/qg-code-design-prototype/case.yaml b/plugins/review/evals/qg-code-design-prototype/case.yaml new file mode 100644 index 00000000000..b42bcf7fe85 --- /dev/null +++ b/plugins/review/evals/qg-code-design-prototype/case.yaml @@ -0,0 +1,4 @@ +schema_version: "1.1" +name: qg-code-design-prototype +context: + scaffold_script: scaffold.sh diff --git a/plugins/review/evals/qg-code-design-prototype/graders/brief-carries-prototype-bar.md b/plugins/review/evals/qg-code-design-prototype/graders/brief-carries-prototype-bar.md new file mode 100644 index 00000000000..150c258f391 --- /dev/null +++ b/plugins/review/evals/qg-code-design-prototype/graders/brief-carries-prototype-bar.md @@ -0,0 +1,5 @@ +--- +type: tool_used +tool: Agent +input_match: 'Quality bar:\s*prototype' +--- diff --git a/plugins/review/evals/qg-code-design-prototype/graders/brief-reviewer-dispatched.md b/plugins/review/evals/qg-code-design-prototype/graders/brief-reviewer-dispatched.md new file mode 100644 index 00000000000..8def59be691 --- /dev/null +++ b/plugins/review/evals/qg-code-design-prototype/graders/brief-reviewer-dispatched.md @@ -0,0 +1,5 @@ +--- +type: tool_used +tool: Agent +input_match: '"subagent_type"\s*:\s*"(?:review:)?brief-reviewer"' +--- diff --git a/plugins/review/evals/qg-code-design-prototype/graders/no-edit.md b/plugins/review/evals/qg-code-design-prototype/graders/no-edit.md new file mode 100644 index 00000000000..903f19fc372 --- /dev/null +++ b/plugins/review/evals/qg-code-design-prototype/graders/no-edit.md @@ -0,0 +1,6 @@ +--- +type: tool_used +tool: Edit +min: 0 +max: 0 +--- diff --git a/plugins/review/evals/qg-code-design-prototype/graders/prototype-repetition-is-suggestion.md b/plugins/review/evals/qg-code-design-prototype/graders/prototype-repetition-is-suggestion.md new file mode 100644 index 00000000000..887c09f11f6 --- /dev/null +++ b/plugins/review/evals/qg-code-design-prototype/graders/prototype-repetition-is-suggestion.md @@ -0,0 +1,4 @@ +--- +type: regex +pattern: '^(?![\s\S]*(?:\|[ \t]*\**(?:CRITICAL|IMPORTANT)\**[ \t]*\||(?:^|\n)[ \t>]*(?:(?:[-*+]|\d+[.)])[ \t]+)?(?:#+[ \t]+)?\**(?:CRITICAL|IMPORTANT)\b(?![ \t]*\**[ \t]*\d)[^\n]*(?:upload\.py|form\.py|config\.py|MAX_UPLOAD_MB|\b25\b)|(?:\|[ \t]*\**(?:CRITICAL|IMPORTANT)\**[ \t]*\||[(\[][ \t]*\**(?:CRITICAL|IMPORTANT)\**[ \t]*[)\]]|\bSeverity\**[ \t]*[:=][ \t]*\**(?:CRITICAL|IMPORTANT)\b)[^\n]*(?:upload\.py|form\.py|config\.py|MAX_UPLOAD_MB|\b25\b)|(?:upload\.py|form\.py|config\.py|MAX_UPLOAD_MB|\b25\b)[^\n]*(?:\|[ \t]*\**(?:CRITICAL|IMPORTANT)\**[ \t]*\||[(\[][ \t]*\**(?:CRITICAL|IMPORTANT)\**[ \t]*[)\]]|\bSeverity\**[ \t]*[:=][ \t]*\**(?:CRITICAL|IMPORTANT)\b)))[\s\S]*(?:\bSUGGESTION\b[^\n]*(?:upload\.py|form\.py|config\.py|MAX_UPLOAD_MB|\b25\b)|(?:upload\.py|form\.py|config\.py|MAX_UPLOAD_MB|\b25\b)[^\n]*\bSUGGESTION\b)' +--- diff --git a/plugins/review/evals/qg-code-design-prototype/prompt.md b/plugins/review/evals/qg-code-design-prototype/prompt.md new file mode 100644 index 00000000000..b9fbc1d2f50 --- /dev/null +++ b/plugins/review/evals/qg-code-design-prototype/prompt.md @@ -0,0 +1,13 @@ +--- +description: "A prototype branch hardcodes the 25 MB upload cap in two new files instead of reading MAX_UPLOAD_MB, reviewed with --quality-bar prototype: the code-design review reports the repetition as SUGGESTION, never CRITICAL or IMPORTANT" +tags: [review, quality-gate, code-design, repetition, quality-bar] +runs: 3 +max_turns: 40 +timeout_seconds: 900 +allowed_tools: [Read, Glob, Grep, Skill, Agent, Bash, Write, Edit] +expected_outcome: "The gate dispatches review:brief-reviewer with the line Quality bar: prototype and reports that app/upload.py and app/form.py each hardcode 25, a copy of MAX_UPLOAD_MB in config.py, rated SUGGESTION because the quality bar is prototype. No finding is CRITICAL or IMPORTANT, and nothing is edited" +--- + +This is a throwaway prototype. I added the upload check and form hint on this branch. + +Run `/review:quality-gate code-design` on this branch with `--base` set to the SHA in `.review-base`, `--quality-bar prototype` and `--report-only`, and give me the report. diff --git a/plugins/review/evals/qg-code-design-prototype/samples/brief-carries-prototype-bar.json b/plugins/review/evals/qg-code-design-prototype/samples/brief-carries-prototype-bar.json new file mode 100644 index 00000000000..f3e1c05ac7f --- /dev/null +++ b/plugins/review/evals/qg-code-design-prototype/samples/brief-carries-prototype-bar.json @@ -0,0 +1,36 @@ +{ + "pass": [ + { + "answer": [ + { + "tool": "Agent", + "input": { + "description": "Code-design review", + "prompt": "Concern: code-design.\nQuality bar: prototype\n\nRead in order: ...", + "subagent_type": "review:brief-reviewer" + } + } + ], + "why": "the brief carries the bar" + } + ], + "fail": [ + { + "answer": [], + "why": "no reviewer dispatched" + }, + { + "answer": [ + { + "tool": "Agent", + "input": { + "description": "Code-design review", + "prompt": "Concern: code-design.\nQuality bar: production (assumed: none given)", + "subagent_type": "review:brief-reviewer" + } + } + ], + "why": "the passed bar is dropped" + } + ] +} diff --git a/plugins/review/evals/qg-code-design-prototype/samples/brief-reviewer-dispatched.json b/plugins/review/evals/qg-code-design-prototype/samples/brief-reviewer-dispatched.json new file mode 100644 index 00000000000..241eca83057 --- /dev/null +++ b/plugins/review/evals/qg-code-design-prototype/samples/brief-reviewer-dispatched.json @@ -0,0 +1,62 @@ +{ + "pass": [ + { + "answer": [ + { + "tool": "Agent", + "input": { + "description": "Code-design review", + "prompt": "You are reviewing a change for code design. Concern: code-design.", + "subagent_type": "review:brief-reviewer" + } + } + ], + "why": "the plugin's brief reviewer" + }, + { + "answer": [ + { + "tool": "Agent", + "input": { + "description": "Review", + "prompt": "Concern: code-design.", + "subagent_type": "brief-reviewer" + } + } + ], + "why": "unprefixed agent name" + } + ], + "fail": [ + { + "answer": [], + "why": "reviewed on the invoking thread" + }, + { + "answer": [ + { + "tool": "Agent", + "input": { + "description": "Review", + "prompt": "Review the design.", + "subagent_type": "general-purpose" + } + } + ], + "why": "a general-purpose agent can fan out" + }, + { + "answer": [ + { + "tool": "Agent", + "input": { + "description": "Review", + "prompt": "Review the code.", + "subagent_type": "review:code-reviewer" + } + } + ], + "why": "wrong reviewer for the code-design mode" + } + ] +} diff --git a/plugins/review/evals/qg-code-design-prototype/samples/no-edit.json b/plugins/review/evals/qg-code-design-prototype/samples/no-edit.json new file mode 100644 index 00000000000..f6aa113111e --- /dev/null +++ b/plugins/review/evals/qg-code-design-prototype/samples/no-edit.json @@ -0,0 +1,34 @@ +{ + "pass": [ + { + "answer": [], + "why": "edits nothing" + }, + { + "answer": [ + { + "tool": "Read", + "input": { + "file_path": "app/upload.py" + } + } + ], + "why": "reads only" + } + ], + "fail": [ + { + "answer": [ + { + "tool": "Edit", + "input": { + "file_path": "app/upload.py", + "old_string": "a", + "new_string": "b" + } + } + ], + "why": "fixes the finding instead of reporting it" + } + ] +} diff --git a/plugins/review/evals/qg-code-design-prototype/samples/prototype-repetition-is-suggestion.json b/plugins/review/evals/qg-code-design-prototype/samples/prototype-repetition-is-suggestion.json new file mode 100644 index 00000000000..39fde0a0148 --- /dev/null +++ b/plugins/review/evals/qg-code-design-prototype/samples/prototype-repetition-is-suggestion.json @@ -0,0 +1,30 @@ +{ + "pass": [ + { + "answer": "Quality bar: prototype\n\n| 1 | SUGGESTION | high | Duplicated Code | app/upload.py and app/form.py hardcode 25, a copy of MAX_UPLOAD_MB | app/upload.py:6 | Read config.MAX_UPLOAD_MB |\n\nCRITICAL 0, IMPORTANT 0, SUGGESTION 1", + "why": "SUGGESTION under the prototype bar" + }, + { + "answer": "Quality bar: prototype\n\nNot CRITICAL: app/upload.py copies 25 under the prototype bar.\n\n| 1 | SUGGESTION | high | Duplicated Code | app/upload.py hardcodes 25, a copy of MAX_UPLOAD_MB | app/upload.py:6 | Read config.MAX_UPLOAD_MB |", + "why": "negated mention of CRITICAL beside a file name" + } + ], + "fail": [ + { + "answer": "", + "why": "empty reply" + }, + { + "answer": "| 1 | CRITICAL | high | Duplicated Code | 25 copied from MAX_UPLOAD_MB | app/upload.py:6 | Read the constant |", + "why": "CRITICAL despite the prototype bar" + }, + { + "answer": "| 1 | IMPORTANT | high | Duplicated Code | 25 copied | app/form.py:2 | Read the constant |", + "why": "IMPORTANT despite the prototype bar" + }, + { + "answer": "No code-design issues found.", + "why": "repetition not reported" + } + ] +} diff --git a/plugins/review/evals/qg-code-design-prototype/scaffold.sh b/plugins/review/evals/qg-code-design-prototype/scaffold.sh new file mode 100755 index 00000000000..1aca1ac78ed --- /dev/null +++ b/plugins/review/evals/qg-code-design-prototype/scaffold.sh @@ -0,0 +1,41 @@ +#!/usr/bin/env bash +# main holds a prototype upload service whose size cap is MAX_UPLOAD_MB in config.py. The branch adds +# an upload check and a form hint that each hardcode 25 instead of reading the constant. The base +# commit SHA is in .review-base. +set -euo pipefail + +git init -q -b main +git config user.name eval +git config user.email eval@example.invalid +printf '.review-base\n' >>.git/info/exclude +mkdir -p app + +cat >config.py <<'PY' +MAX_UPLOAD_MB = 25 +PY + +cat >app/__init__.py <<'PY' +PY + +git add . +git commit -q -m "feat: upload size cap" +git rev-parse HEAD >.review-base +git checkout -q -b feat/upload-form + +cat >app/upload.py <<'PY' +class UploadTooLarge(Exception): + pass + + +def check_size(size_bytes): + if size_bytes > 25 * 1024 * 1024: + raise UploadTooLarge("file is larger than 25 MB") +PY + +cat >app/form.py <<'PY' +def upload_hint(): + return "Files up to 25 MB." +PY + +git add app +git commit -q -m "feat: upload check and form hint" diff --git a/plugins/review/evals/qg-code-design-sanctioned/case.yaml b/plugins/review/evals/qg-code-design-sanctioned/case.yaml new file mode 100644 index 00000000000..2250402ac2e --- /dev/null +++ b/plugins/review/evals/qg-code-design-sanctioned/case.yaml @@ -0,0 +1,4 @@ +schema_version: "1.1" +name: qg-code-design-sanctioned +context: + scaffold_script: scaffold.sh diff --git a/plugins/review/evals/qg-code-design-sanctioned/graders/brief-reviewer-dispatched.md b/plugins/review/evals/qg-code-design-sanctioned/graders/brief-reviewer-dispatched.md new file mode 100644 index 00000000000..8def59be691 --- /dev/null +++ b/plugins/review/evals/qg-code-design-sanctioned/graders/brief-reviewer-dispatched.md @@ -0,0 +1,5 @@ +--- +type: tool_used +tool: Agent +input_match: '"subagent_type"\s*:\s*"(?:review:)?brief-reviewer"' +--- diff --git a/plugins/review/evals/qg-code-design-sanctioned/graders/no-critical.md b/plugins/review/evals/qg-code-design-sanctioned/graders/no-critical.md new file mode 100644 index 00000000000..54b9fb6a2f1 --- /dev/null +++ b/plugins/review/evals/qg-code-design-sanctioned/graders/no-critical.md @@ -0,0 +1,4 @@ +--- +type: regex +pattern: '^(?![\s\S]*(?:\|[ \t]*\**CRITICAL\**[ \t]*\||(?:^|\n)[ \t>]*(?:(?:[-*+]|\d+[.)])[ \t]+)?(?:#+[ \t]+)?\**CRITICAL\b(?![ \t]*\**[ \t]*\d)[^\n]*(?:\b(?:index|refund|money|charge)\.js\b)|(?:\|[ \t]*\**CRITICAL\**[ \t]*\||[(\[][ \t]*\**CRITICAL\**[ \t]*[)\]]|\bSeverity\**[ \t]*[:=][ \t]*\**CRITICAL\b)[^\n]*(?:\b(?:index|refund|money|charge)\.js\b)|(?:\b(?:index|refund|money|charge)\.js\b)[^\n]*(?:\|[ \t]*\**CRITICAL\**[ \t]*\||[(\[][ \t]*\**CRITICAL\**[ \t]*[)\]]|\bSeverity\**[ \t]*[:=][ \t]*\**CRITICAL\b)))[\s\S]*\b(?:index|refund|money|charge)\.js\b' +--- diff --git a/plugins/review/evals/qg-code-design-sanctioned/graders/no-edit.md b/plugins/review/evals/qg-code-design-sanctioned/graders/no-edit.md new file mode 100644 index 00000000000..903f19fc372 --- /dev/null +++ b/plugins/review/evals/qg-code-design-sanctioned/graders/no-edit.md @@ -0,0 +1,6 @@ +--- +type: tool_used +tool: Edit +min: 0 +max: 0 +--- diff --git a/plugins/review/evals/qg-code-design-sanctioned/graders/treats-registry-and-move-as-sanctioned.md b/plugins/review/evals/qg-code-design-sanctioned/graders/treats-registry-and-move-as-sanctioned.md new file mode 100644 index 00000000000..2ee7c119a50 --- /dev/null +++ b/plugins/review/evals/qg-code-design-sanctioned/graders/treats-registry-and-move-as-sanctioned.md @@ -0,0 +1,7 @@ +--- +type: llm +--- + +PASS if the review reports no CRITICAL or blocking finding, and either reports nothing about the registry entry and the moved file or treats them as sanctioned: the `refund` line in src/handlers/index.js is an entry in the handler registry the README declares, and src/lib/money.js is src/util/money.js moved unchanged, not a second copy of it. Any finding it does raise about them is SUGGESTION at most. + +FAIL if the review rates the registry entry, the handler name appearing in both refund.js and index.js, or the moved money.js as CRITICAL, IMPORTANT or blocking duplication; treats src/lib/money.js and src/util/money.js as two copies to merge; or asks for the registry to be generated before the change can merge. diff --git a/plugins/review/evals/qg-code-design-sanctioned/prompt.md b/plugins/review/evals/qg-code-design-sanctioned/prompt.md new file mode 100644 index 00000000000..f9a8623c508 --- /dev/null +++ b/plugins/review/evals/qg-code-design-sanctioned/prompt.md @@ -0,0 +1,13 @@ +--- +description: "The branch adds a refund handler with its entry in the README-declared handler registry and moves money.js from src/util to src/lib unchanged: the code-design review reports neither the registry entry nor the moved file as a CRITICAL repetition" +tags: [review, quality-gate, code-design, repetition] +runs: 3 +max_turns: 40 +timeout_seconds: 900 +allowed_tools: [Read, Glob, Grep, Skill, Agent, Bash, Write, Edit] +expected_outcome: "The gate dispatches review:brief-reviewer and reports no CRITICAL finding: the refund line in src/handlers/index.js is an entry in the registry the README declares, and src/lib/money.js is src/util/money.js moved, not a second copy. Anything it does report about them is SUGGESTION at most, and nothing is edited" +--- + +I added refunds on this branch and moved the money helpers. + +Run `/review:quality-gate code-design` on this branch with `--base` set to the SHA in `.review-base` and `--report-only`, and give me the report. diff --git a/plugins/review/evals/qg-code-design-sanctioned/samples/brief-reviewer-dispatched.json b/plugins/review/evals/qg-code-design-sanctioned/samples/brief-reviewer-dispatched.json new file mode 100644 index 00000000000..241eca83057 --- /dev/null +++ b/plugins/review/evals/qg-code-design-sanctioned/samples/brief-reviewer-dispatched.json @@ -0,0 +1,62 @@ +{ + "pass": [ + { + "answer": [ + { + "tool": "Agent", + "input": { + "description": "Code-design review", + "prompt": "You are reviewing a change for code design. Concern: code-design.", + "subagent_type": "review:brief-reviewer" + } + } + ], + "why": "the plugin's brief reviewer" + }, + { + "answer": [ + { + "tool": "Agent", + "input": { + "description": "Review", + "prompt": "Concern: code-design.", + "subagent_type": "brief-reviewer" + } + } + ], + "why": "unprefixed agent name" + } + ], + "fail": [ + { + "answer": [], + "why": "reviewed on the invoking thread" + }, + { + "answer": [ + { + "tool": "Agent", + "input": { + "description": "Review", + "prompt": "Review the design.", + "subagent_type": "general-purpose" + } + } + ], + "why": "a general-purpose agent can fan out" + }, + { + "answer": [ + { + "tool": "Agent", + "input": { + "description": "Review", + "prompt": "Review the code.", + "subagent_type": "review:code-reviewer" + } + } + ], + "why": "wrong reviewer for the code-design mode" + } + ] +} diff --git a/plugins/review/evals/qg-code-design-sanctioned/samples/no-critical.json b/plugins/review/evals/qg-code-design-sanctioned/samples/no-critical.json new file mode 100644 index 00000000000..03a704cf8cc --- /dev/null +++ b/plugins/review/evals/qg-code-design-sanctioned/samples/no-critical.json @@ -0,0 +1,34 @@ +{ + "pass": [ + { + "answer": "No code-design issues found. src/handlers/index.js is the declared registry and src/lib/money.js is a move.\n\nCRITICAL 0, IMPORTANT 0, SUGGESTION 0", + "why": "clean report naming the files" + }, + { + "answer": "| 1 | SUGGESTION | low | Shotgun Surgery | A handler needs its file and a registry line | src/handlers/index.js:7 | Optional: derive the registry |", + "why": "SUGGESTION only" + }, + { + "answer": "Not CRITICAL: src/handlers/index.js is the declared registry and src/lib/money.js is a move.\n\nCRITICAL 0, IMPORTANT 0, SUGGESTION 0", + "why": "negated mention of CRITICAL beside a file name" + } + ], + "fail": [ + { + "answer": "", + "why": "empty reply" + }, + { + "answer": "| 1 | CRITICAL | high | Duplicated Code | refund is named in two places | src/handlers/index.js:7 | Generate the registry |", + "why": "registry entry rated CRITICAL" + }, + { + "answer": "src/lib/money.js duplicates src/util/money.js (CRITICAL).", + "why": "move treated as a copy" + }, + { + "answer": "No issues.", + "why": "names no reviewed file" + } + ] +} diff --git a/plugins/review/evals/qg-code-design-sanctioned/samples/no-edit.json b/plugins/review/evals/qg-code-design-sanctioned/samples/no-edit.json new file mode 100644 index 00000000000..67fbc709fc0 --- /dev/null +++ b/plugins/review/evals/qg-code-design-sanctioned/samples/no-edit.json @@ -0,0 +1,34 @@ +{ + "pass": [ + { + "answer": [], + "why": "edits nothing" + }, + { + "answer": [ + { + "tool": "Read", + "input": { + "file_path": "src/handlers/index.js" + } + } + ], + "why": "reads only" + } + ], + "fail": [ + { + "answer": [ + { + "tool": "Edit", + "input": { + "file_path": "src/handlers/index.js", + "old_string": "a", + "new_string": "b" + } + } + ], + "why": "fixes the finding instead of reporting it" + } + ] +} diff --git a/plugins/review/evals/qg-code-design-sanctioned/samples/treats-registry-and-move-as-sanctioned.json b/plugins/review/evals/qg-code-design-sanctioned/samples/treats-registry-and-move-as-sanctioned.json new file mode 100644 index 00000000000..38205b3c38f --- /dev/null +++ b/plugins/review/evals/qg-code-design-sanctioned/samples/treats-registry-and-move-as-sanctioned.json @@ -0,0 +1,18 @@ +{ + "pass": [ + { + "answer": "No code-design issues found. The refund line in src/handlers/index.js is an entry in the registry the README declares, and src/lib/money.js is src/util/money.js moved unchanged.", + "why": "both recognized" + } + ], + "fail": [ + { + "answer": "| 1 | CRITICAL | high | Duplicated Code | src/lib/money.js and src/util/money.js are copies | src/lib/money.js:1 | Delete one |", + "why": "move treated as duplication" + }, + { + "answer": "| 1 | IMPORTANT | high | Duplicated Code | The handler name refund appears in refund.js and index.js | src/handlers/index.js:7 | Generate the registry |", + "why": "registry entry rated IMPORTANT" + } + ] +} diff --git a/plugins/review/evals/qg-code-design-sanctioned/scaffold.sh b/plugins/review/evals/qg-code-design-sanctioned/scaffold.sh new file mode 100755 index 00000000000..1f5df871762 --- /dev/null +++ b/plugins/review/evals/qg-code-design-sanctioned/scaffold.sh @@ -0,0 +1,75 @@ +#!/usr/bin/env bash +# main holds a payments service whose README declares src/handlers/index.js the handler registry. The +# branch adds a refund handler plus its registry entry, and moves src/util/money.js to src/lib/money.js +# unchanged, updating the one import. Neither is an introduced copy. The base commit SHA is in +# .review-base. +set -euo pipefail + +git init -q -b main +git config user.name eval +git config user.email eval@example.invalid +printf '.review-base\n' >>.git/info/exclude +mkdir -p src/handlers src/util + +cat >README.md <<'MD' +# payments + +`src/handlers/index.js` is the handler registry: it lists every handler once, by name. A new +handler adds its file under `src/handlers/` and one line to the registry. +MD + +cat >src/util/money.js <<'JS' +export function toCents(amount) { + return Math.round(amount * 100); +} + +export function fromCents(cents) { + return cents / 100; +} +JS + +cat >src/handlers/charge.js <<'JS' +import { toCents } from "../util/money.js"; + +export function charge(gateway, order) { + return gateway.capture(order.id, toCents(order.total)); +} +JS + +cat >src/handlers/index.js <<'JS' +import { charge } from "./charge.js"; + +export const handlers = { + charge, +}; +JS + +git add . +git commit -q -m "feat: charge handler" +git rev-parse HEAD >.review-base +git checkout -q -b feat/refunds + +mkdir -p src/lib +git mv src/util/money.js src/lib/money.js +sed -i 's#\.\./util/money\.js#../lib/money.js#' src/handlers/charge.js + +cat >src/handlers/refund.js <<'JS' +import { toCents } from "../lib/money.js"; + +export function refund(gateway, order, amount) { + return gateway.refund(order.id, toCents(amount)); +} +JS + +cat >src/handlers/index.js <<'JS' +import { charge } from "./charge.js"; +import { refund } from "./refund.js"; + +export const handlers = { + charge, + refund, +}; +JS + +git add -A src +git commit -q -m "feat: refund handler; move money helpers to src/lib" diff --git a/plugins/review/skills/quality-gate/SKILL.md b/plugins/review/skills/quality-gate/SKILL.md index f6cc0575fe9..5a5ef9e5970 100644 --- a/plugins/review/skills/quality-gate/SKILL.md +++ b/plugins/review/skills/quality-gate/SKILL.md @@ -1,6 +1,6 @@ --- -description: "Single-lens review checkpoint between 'code works' and 'code is ready': self, code, architecture, security, spec, close-out, downstream, pr, criteria, slice or restatement mode, each delegated to the matching reviewer. Use when the user asks to 'review this', for a 'self-review' or 'quality gate', a code, architecture or security review, whether the change 'matches the spec/issue/plan', a 'close-out review' of a shipped spec container, 'what could this break' or the 'blast radius' of a written change, or after implementation completes." -argument-hint: "[self|code|architecture|security|spec|close-out|downstream|pr|criteria|slice|restatement]" +description: "Single-lens review checkpoint between 'code works' and 'code is ready': self, code, code-design, architecture, security, spec, close-out, downstream, pr, criteria, slice or restatement mode, each delegated to its reviewer. Use when asked to 'review this', for a 'self-review' or 'quality gate', a code, architecture or security review, whether the change 'matches the spec/issue/plan', a 'close-out review' of a shipped spec container, 'what could this break' or the 'blast radius' of a written change, or after implementation completes." +argument-hint: "[self|code|code-design|architecture|security|spec|close-out|downstream|pr|criteria|slice…]" user-invocable: true disable-model-invocation: false allowed-tools: ["Bash(git branch --show-current)", "Bash(git status --porcelain | head -20)", "Bash(git rev-parse:*)", "Bash(git merge-base:*)", "Bash(git diff:*)", "Bash(git log:*)", "Bash(git show:*)", "Bash(git ls-files --others --exclude-standard)", "Bash(git ls-remote --symref origin)", "Bash(git ls-remote --symref origin:*)", "Bash(git fetch origin)", "Bash(git fetch origin:*)", "Bash(git remote get-url:*)", "Bash(node \"${CLAUDE_PLUGIN_ROOT}/skills/setup/scripts/setup-apply.mjs\" --check:*)", "Bash(node ${CLAUDE_PLUGIN_ROOT}/skills/setup/scripts/setup-apply.mjs --check:*)"] @@ -10,7 +10,7 @@ metadata: summary: Single-lens review checkpoint routed to the matching reviewer --- -**Arguments.** `[self|code|architecture|security|spec|close-out|downstream|pr|criteria|slice|restatement]`. `spec` takes `[--spec ]`, `close-out` takes `[--container ] [--dry-run]`, and `slice` takes ``. Every mode also takes the phase-boundary inputs `[--worktree ] [--base ] [--quality-bar ] [--report-only]` ("Shared inputs"). +**Arguments.** `[self|code|code-design|architecture|security|spec|close-out|downstream|pr|criteria|slice…]`; the full set ends `slice|restatement]`. `spec` takes `[--spec ]`, `close-out` takes `[--container ] [--dry-run]`, and `slice` takes ``. Every mode also takes the phase-boundary inputs `[--worktree ] [--base ] [--quality-bar ] [--report-only]` ("Shared inputs"). ## Repository context. Gather first @@ -62,6 +62,7 @@ Review is the quality checkpoint between "code works" and "code is ready." This |--------|------|-------------| | Just finished implementing, "review my work", bare invocation with uncommitted changes | **self** | [context/self.md](context/self.md) | | "review the code", "code review" | **code** | [context/code.md](context/code.md) | +| "code-design review", one fact copied into several files, one change cascading across files, a plan phase tagged `Review: code-design` | **code-design** | [context/code-design.md](context/code-design.md) | | "architecture review", new modules, cross-cutting structure | **architecture** | [context/architecture.md](context/architecture.md) | | "security review", auth/input handling, API endpoints | **security** | [context/security.md](context/security.md) | | "does this match the spec/issue/plan", "did we build what was asked", scope-creep check, `spec [--spec ]` | **spec** | [context/spec.md](context/spec.md) | diff --git a/plugins/review/skills/quality-gate/context/code-design.md b/plugins/review/skills/quality-gate/context/code-design.md new file mode 100644 index 00000000000..1db7d556137 --- /dev/null +++ b/plugins/review/skills/quality-gate/context/code-design.md @@ -0,0 +1,105 @@ +# Code-design review mode + +Reviews a change for design that makes the next change costly: one fact or rule copied into +several places, one kind of change spread across many files, and abstraction with no second use. +The consumer's criteria govern; the bundled criteria below fill in only where the consumer has +none. A fresh-context `review:brief-reviewer` runs the review, never the invoking thread. + +## Resolve the criteria + +In this order; the first source that covers a concern governs it: + +1. **Consumer criteria.** Resolve the project's standards through the + [Resolution ladder](../../../reference/standards-contract.md) as [criteria.md](criteria.md) + step 1 does, and keep the matched files that cover design, architecture, duplication or review + of the changed surfaces. The severity vocabulary is the one SKILL.md "Shared inputs" names. +2. **Bundled criteria.** The table below, for each concern no consumer file covers. + +When a matched consumer file contradicts a bundled row (it permits what the row flags, or rates it +differently), the consumer's rule applies, and the report carries one line per row it overrode, +with the row's Principle and the consumer file's repo-relative path: + +```text +Overrides bundled default per +``` + +## Quality bar + +Take it from `--quality-bar`, else from a `Quality bar: prototype | internal | production` line in +the request. With neither, rate as `production` and put +`Quality bar: production (assumed: none given)` in the report. + +## Bundled criteria + +Principles are code smells from Fowler, *Refactoring* (2nd ed.), chapter 3, "Bad Smells in Code". +Markdown means Markdown prose files; code is every other changed file. + +| Criterion | Principle | Flag | Code, `internal` or `production` | Markdown | Any surface, `prototype` | +|---|---|---|---|---|---| +| Introduced repetition | Duplicated Code | The diff adds a copy of a fact (a constant, list, schema or rule) or of logic that already exists in the diff or the codebase | CRITICAL when the cascade rule holds, else SUGGESTION | SUGGESTION | SUGGESTION | +| Change cascade | Shotgun Surgery | One kind of change already needs coordinated edits across several files, and the diff adds to that set | SUGGESTION | SUGGESTION | SUGGESTION | +| Unearned abstraction | Speculative Generality | An interface, parameter, hook or layer with one user and no second variant the change names | SUGGESTION | SUGGESTION | SUGGESTION | + +### The cascade rule + +In a code-design review this rule overrides the severity vocabulary's "code duplication" IMPORTANT +example: repetition is CRITICAL or SUGGESTION as the table rates it. + +A repetition finding is CRITICAL only when all of these hold; there is no file-count threshold: + +- the diff introduces the copy (a copy that predates the base is at most SUGGESTION); +- the finding names the repeated fact or logic, the file and line of every copy, and a later, + otherwise-correct change to one copy that leaves the others wrong (the CRITICAL test in the + severity vocabulary); +- no shared source (one definition the copies import or generate from) and no sync check (a test, + lint or CI step that fails when the copies disagree) keeps them aligned; +- the copies are code, the quality bar is not `prototype`, and no exemption below applies. + +### Exempt from CRITICAL + +These are judgment, not a review guide's rule. Each is SUGGESTION at most: + +- a passage inside `contract-restatement` markers, which a check holds to its source; +- an entry in a registry the repository declares, a file that exists to list every member of a set; +- generated files; +- test fixtures and sample data; +- a rename or move, where the text at the new path is the old text rather than a second copy. + +## Dispatch + +Dispatch `review:brief-reviewer`, never a general-purpose agent. With phase-boundary inputs the +brief carries what [phase-boundary.md](phase-boundary.md) "The reviewer's brief" lists; otherwise +the change set is `git diff ` plus `git ls-files --others --exclude-standard` +(SKILL.md "Shared inputs"). Substitute `` (SKILL.md "Spoke paths"). The brief carries +no plan rationale and no account of the change: + +```text +You are reviewing a change for code design. Concern: code-design. + + +Read in order: +1. Severity vocabulary: /context/severity.md>. +2. Consumer criteria: . +3. Bundled criteria: /skills/quality-gate/context/code-design.md, sections + "Bundled criteria", "The cascade rule" and "Exempt from CRITICAL". +4. The change set: . Read every untracked file it lists. + +Consumer criteria and severity win over the bundled ones. For each bundled row a consumer file +contradicts, apply the consumer's rule and write +"Overrides bundled default per ". +For each repetition finding, name the repeated fact or logic, the file:line of every copy, the +later change that would make the copies diverge, and whether a shared source or sync check exists. +Report every finding the criteria reach, uncertain or low-severity ones included; confidence is +high, medium or low. + +| # | Severity | Confidence | Principle | Finding | File:Line | Action | +|---|----------|------------|-----------|---------|-----------|--------| +``` + +## Report + +Verify each returned finding against the diff (SKILL.md Step 2) and check its severity against the +cascade rule, then report per SKILL.md Step 3, with the quality-bar line, the criteria source +(consumer files, or `bundled`) and every override line above the findings table. Unless +`--report-only`, write the findings artifact to the findings location as +`-code-design.md`. diff --git a/plugins/review/skills/quality-gate/context/phase-boundary.md b/plugins/review/skills/quality-gate/context/phase-boundary.md index cf5b816f655..1bf24cb46c9 100644 --- a/plugins/review/skills/quality-gate/context/phase-boundary.md +++ b/plugins/review/skills/quality-gate/context/phase-boundary.md @@ -8,8 +8,8 @@ the table says which modes read each one. | Input | Meaning | Modes that read it | Effect | |---|---|---|---| | `--worktree ` | The checkout holding the change, separate from the invoking session's | all but `pr` and `close-out` | Every git command runs as `git -C ...` and every file read resolves under ``, starting with the repository-context gather (`git -C branch --show-current`, `git -C status --porcelain \| head -20`). The invoking checkout is never read as the change. A path where `git -C rev-parse --show-toplevel` fails is a STOP. | -| `--base ` | The previous phase-boundary commit | `self`, `code`, `architecture`, `security`, `spec`, `downstream`, `slice`, `restatement`, `criteria` | The review diff base is ``. No pull-request lookup, fetch or remote read runs. The change set is `git -C diff ` (commits since the boundary plus uncommitted edits) and `git -C ls-files --others --exclude-standard`. Without `--worktree`, the same commands run in the current checkout. | -| `--quality-bar ` | The `Quality bar:` line from the plan or Brief | every mode that dispatches a reviewer: `self`, `code`, `architecture`, `security`, `spec`, `downstream`, `slice`, `restatement`, `criteria` given `--base` or `--worktree`, and, given neither, `pr` and `close-out`; `criteria` without either reviews inline and ignores it | Passed to each dispatched reviewer as the line `Quality bar: `. Absent means `production`, and the report names that assumption. Tier tests stay those of the severity vocabulary in SKILL.md "Shared inputs". With `--base` or `--worktree`, `pr` and `close-out` STOP (Mode notes). | +| `--base ` | The previous phase-boundary commit | `self`, `code`, `code-design`, `architecture`, `security`, `spec`, `downstream`, `slice`, `restatement`, `criteria` | The review diff base is ``. No pull-request lookup, fetch or remote read runs. The change set is `git -C diff ` (commits since the boundary plus uncommitted edits) and `git -C ls-files --others --exclude-standard`. Without `--worktree`, the same commands run in the current checkout. | +| `--quality-bar ` | The `Quality bar:` line from the plan or Brief | every mode that dispatches a reviewer: `self`, `code`, `code-design`, `architecture`, `security`, `spec`, `downstream`, `slice`, `restatement`, `criteria` given `--base` or `--worktree`, and, given neither, `pr` and `close-out`; `criteria` without either reviews inline and ignores it | Passed to each dispatched reviewer as the line `Quality bar: `. Absent means `production`, and the report names that assumption. Tier tests stay those of the severity vocabulary in SKILL.md "Shared inputs". With `--base` or `--worktree`, `pr` and `close-out` STOP (Mode notes). | | `--report-only` | The caller persists and acts on the result | every mode | The gate writes nothing and asks nothing: no findings artifact, no `downstream` probe (it runs as `report`), no Step 4 handoff. A point that would ask the user (an ambiguous mode, the `spec` rung-4 ask) is a STOP naming what is missing, with one exception: the standards [Resolution ladder](../../../reference/standards-contract.md) skips its offer and ask rungs and takes its non-interactive rung, and the report names that assumption. The Step 3 report is the result. | ## Pre-flight with a base SHA @@ -37,6 +37,8 @@ Step 0.5 runs against the base SHA in place of the "Shared inputs" ladder: step writes no path file: the loop reads `git diff --name-only -z ` with `read -r -d ''`, where `` is the `--base` SHA when given, else the review diff base ("Shared inputs"), and the crossings go in the gate's report. +- `code-design` resolves consumer criteria from the change's checkout and always dispatches + `review:brief-reviewer`, briefed as below. - `restatement` with `--base` or `--worktree` reviews no file inline: it dispatches a fresh-context `review:brief-reviewer` for every batch, briefed as below. - `downstream` with `--base` skips the `downstream_probe` layer read from the default branch, which @@ -63,7 +65,8 @@ Dispatch each mode's own reviewer in fresh context, as its context file names, w - the `Quality bar:` line; - the inputs the mode's own brief requires: the slice criteria file path in `slice`, the size crossings in `code`, the finding-class table and the spec text read from the local - `--spec ` in `spec`, the resolved criteria (file paths and sections) in `criteria`, and + `--spec ` in `spec`, the resolved criteria (file paths and sections) in `criteria`, the + consumer criteria paths and the bundled criteria path in `code-design`, and the paved-path file from [criteria.md](criteria.md) step 2; - this rule, verbatim: "Rate a finding only when the phase diff causes it: a line the diff adds or changes, or something the diff leaves out that the change requires. The file it names may be