From 33127baac2636763c6d94082fde8abce05c214a6 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 18 Oct 2026 20:22:50 -0400 Subject: [PATCH 01/21] feat(review): phase-boundary inputs for quality-gate Every quality-gate mode takes --worktree, --base, --quality-bar and --report-only, so an orchestrator can review a phase diff in a worker's worktree. With a base SHA no pull-request lookup or fetch runs. Adds the qg-phase-boundary-worktree eval case. Refs #8327 Co-Authored-By: Claude Opus 5.5 --- ...ity-gate-phase-boundary-inputs-6b1e94d2.md | 7 + .../qg-phase-boundary-worktree/case.yaml | 4 + .../graders/cites-search-sqli.md | 5 + .../graders/diffs-worktree-from-base.md | 6 + .../graders/no-findings-artifact.md | 7 + .../graders/no-forge-lookup.md | 7 + .../graders/no-pull-request-skill.md | 7 + .../graders/security-reviewer-dispatched.md | 5 + .../graders/stays-in-phase-diff.md | 5 + .../qg-phase-boundary-worktree/prompt.md | 13 ++ .../samples/cites-search-sqli.json | 27 ++++ .../samples/diffs-worktree-from-base.json | 31 +++++ .../samples/no-findings-artifact.json | 19 +++ .../samples/no-forge-lookup.json | 30 +++++ .../samples/no-pull-request-skill.json | 19 +++ .../samples/security-reviewer-dispatched.json | 32 +++++ .../samples/stays-in-phase-diff.json | 23 ++++ .../qg-phase-boundary-worktree/scaffold.sh | 122 ++++++++++++++++++ plugins/review/skills/quality-gate/SKILL.md | 20 ++- .../quality-gate/context/phase-boundary.md | 44 +++++++ 20 files changed, 427 insertions(+), 6 deletions(-) create mode 100644 .changes/review/refactor-8327-quality-gate-phase-boundary-inputs-6b1e94d2.md create mode 100644 plugins/review/evals/qg-phase-boundary-worktree/case.yaml create mode 100644 plugins/review/evals/qg-phase-boundary-worktree/graders/cites-search-sqli.md create mode 100644 plugins/review/evals/qg-phase-boundary-worktree/graders/diffs-worktree-from-base.md create mode 100644 plugins/review/evals/qg-phase-boundary-worktree/graders/no-findings-artifact.md create mode 100644 plugins/review/evals/qg-phase-boundary-worktree/graders/no-forge-lookup.md create mode 100644 plugins/review/evals/qg-phase-boundary-worktree/graders/no-pull-request-skill.md create mode 100644 plugins/review/evals/qg-phase-boundary-worktree/graders/security-reviewer-dispatched.md create mode 100644 plugins/review/evals/qg-phase-boundary-worktree/graders/stays-in-phase-diff.md create mode 100644 plugins/review/evals/qg-phase-boundary-worktree/prompt.md create mode 100644 plugins/review/evals/qg-phase-boundary-worktree/samples/cites-search-sqli.json create mode 100644 plugins/review/evals/qg-phase-boundary-worktree/samples/diffs-worktree-from-base.json create mode 100644 plugins/review/evals/qg-phase-boundary-worktree/samples/no-findings-artifact.json create mode 100644 plugins/review/evals/qg-phase-boundary-worktree/samples/no-forge-lookup.json create mode 100644 plugins/review/evals/qg-phase-boundary-worktree/samples/no-pull-request-skill.json create mode 100644 plugins/review/evals/qg-phase-boundary-worktree/samples/security-reviewer-dispatched.json create mode 100644 plugins/review/evals/qg-phase-boundary-worktree/samples/stays-in-phase-diff.json create mode 100755 plugins/review/evals/qg-phase-boundary-worktree/scaffold.sh create mode 100644 plugins/review/skills/quality-gate/context/phase-boundary.md diff --git a/.changes/review/refactor-8327-quality-gate-phase-boundary-inputs-6b1e94d2.md b/.changes/review/refactor-8327-quality-gate-phase-boundary-inputs-6b1e94d2.md new file mode 100644 index 0000000000..41dcbe1c49 --- /dev/null +++ b/.changes/review/refactor-8327-quality-gate-phase-boundary-inputs-6b1e94d2.md @@ -0,0 +1,7 @@ +--- +bump: minor +--- + +### Added + +- `/review:quality-gate` takes phase-boundary inputs in every mode: `--worktree `, `--base `, `--quality-bar` and `--report-only`, so an orchestrator can review a phase diff in a worker's worktree before any pull request exists. With a base SHA the gate runs no pull-request lookup or fetch, and the reviewer gets only the diff, the concern tag and the quality bar. diff --git a/plugins/review/evals/qg-phase-boundary-worktree/case.yaml b/plugins/review/evals/qg-phase-boundary-worktree/case.yaml new file mode 100644 index 0000000000..3f417e1074 --- /dev/null +++ b/plugins/review/evals/qg-phase-boundary-worktree/case.yaml @@ -0,0 +1,4 @@ +schema_version: "1.1" +name: qg-phase-boundary-worktree +context: + scaffold_script: scaffold.sh diff --git a/plugins/review/evals/qg-phase-boundary-worktree/graders/cites-search-sqli.md b/plugins/review/evals/qg-phase-boundary-worktree/graders/cites-search-sqli.md new file mode 100644 index 0000000000..75990a9202 --- /dev/null +++ b/plugins/review/evals/qg-phase-boundary-worktree/graders/cites-search-sqli.md @@ -0,0 +1,5 @@ +--- +type: regex +pattern: '\bsearch\.js\b[^\n]{0,600}?\b(?:SQL[- ]?injection|SQLi\b|CWE-89)|\b(?:SQL[- ]?injection|SQLi\b|CWE-89)[^\n]{0,600}?\bsearch\.js\b' +flags: i +--- diff --git a/plugins/review/evals/qg-phase-boundary-worktree/graders/diffs-worktree-from-base.md b/plugins/review/evals/qg-phase-boundary-worktree/graders/diffs-worktree-from-base.md new file mode 100644 index 0000000000..c8add08676 --- /dev/null +++ b/plugins/review/evals/qg-phase-boundary-worktree/graders/diffs-worktree-from-base.md @@ -0,0 +1,6 @@ +--- +type: tool_used +tool: Bash +input_match: 'git\s+-C\s+(?:\\?["''])?(?:\./)?(?:[^"''\s\\]*/)?\.worktrees/phase-2/?(?:\\?["''])?\s+(?:--no-pager\s+)?diff\b[^"\n;&|]*\b[0-9a-f]{7,40}\b' +min: 1 +--- diff --git a/plugins/review/evals/qg-phase-boundary-worktree/graders/no-findings-artifact.md b/plugins/review/evals/qg-phase-boundary-worktree/graders/no-findings-artifact.md new file mode 100644 index 0000000000..f211fd9238 --- /dev/null +++ b/plugins/review/evals/qg-phase-boundary-worktree/graders/no-findings-artifact.md @@ -0,0 +1,7 @@ +--- +type: tool_used +tool: Write +input_match: '"file_path"\s*:\s*"[^"]*reviews/' +min: 0 +max: 0 +--- diff --git a/plugins/review/evals/qg-phase-boundary-worktree/graders/no-forge-lookup.md b/plugins/review/evals/qg-phase-boundary-worktree/graders/no-forge-lookup.md new file mode 100644 index 0000000000..d6061bdd06 --- /dev/null +++ b/plugins/review/evals/qg-phase-boundary-worktree/graders/no-forge-lookup.md @@ -0,0 +1,7 @@ +--- +type: tool_used +tool: Bash +input_match: '\bgh\s+(?:pr|api|repo|issue)\b|\bgit\s+(?:-C\s+\S+\s+)?(?:fetch|ls-remote|pull)\b' +min: 0 +max: 0 +--- diff --git a/plugins/review/evals/qg-phase-boundary-worktree/graders/no-pull-request-skill.md b/plugins/review/evals/qg-phase-boundary-worktree/graders/no-pull-request-skill.md new file mode 100644 index 0000000000..645893d64e --- /dev/null +++ b/plugins/review/evals/qg-phase-boundary-worktree/graders/no-pull-request-skill.md @@ -0,0 +1,7 @@ +--- +type: tool_used +tool: Skill +input_match: '"skill"\s*:\s*"(?:source-control:)?pull-request"' +min: 0 +max: 0 +--- diff --git a/plugins/review/evals/qg-phase-boundary-worktree/graders/security-reviewer-dispatched.md b/plugins/review/evals/qg-phase-boundary-worktree/graders/security-reviewer-dispatched.md new file mode 100644 index 0000000000..3d855c72b9 --- /dev/null +++ b/plugins/review/evals/qg-phase-boundary-worktree/graders/security-reviewer-dispatched.md @@ -0,0 +1,5 @@ +--- +type: tool_used +tool: Agent +input_match: '"subagent_type"\s*:\s*"(?:review:)?security-reviewer"' +--- diff --git a/plugins/review/evals/qg-phase-boundary-worktree/graders/stays-in-phase-diff.md b/plugins/review/evals/qg-phase-boundary-worktree/graders/stays-in-phase-diff.md new file mode 100644 index 0000000000..9dda213bc2 --- /dev/null +++ b/plugins/review/evals/qg-phase-boundary-worktree/graders/stays-in-phase-diff.md @@ -0,0 +1,5 @@ +--- +type: regex +pattern: '^(?![\s\S]*\badmin\.js\b)[\s\S]*\bsearch\.js\b' +flags: i +--- diff --git a/plugins/review/evals/qg-phase-boundary-worktree/prompt.md b/plugins/review/evals/qg-phase-boundary-worktree/prompt.md new file mode 100644 index 0000000000..ca3e31f2fa --- /dev/null +++ b/plugins/review/evals/qg-phase-boundary-worktree/prompt.md @@ -0,0 +1,13 @@ +--- +description: "An orchestrator on main runs the security mode against a worker's separate worktree from the phase-1 boundary SHA: the review diffs that worktree from the base, runs no forge lookup, dispatches the security reviewer, reports the phase-2 SQL injection, and writes no findings artifact" +tags: [review, quality-gate, security, phase-boundary] +runs: 3 +max_turns: 40 +timeout_seconds: 900 +allowed_tools: [Read, Glob, Grep, Skill, Agent, Bash, Write] +expected_outcome: "The review reads the change set with git -C .worktrees/phase-2 diff against the phase-1 SHA recorded in .worktrees/phase-2.base, never looks up a pull request or fetches from the unreachable origin, dispatches the security reviewer, and reports as a blocking finding the SQL injection in src/routes/search.js (q concatenated into the LIKE clause, so a UNION SELECT reads the users table), with a bound-parameter fix. It does not report src/routes/admin.js, which phase 1 added before the base, and writes no findings file because the run is report-only" +--- + +I orchestrate a plan from this checkout, which stays on main. A worker just finished phase 2 in its own worktree at `.worktrees/phase-2`; the phase-1 boundary commit SHA is in `.worktrees/phase-2.base`. No pull request exists yet. + +Run `/review:quality-gate security` for that phase with `--worktree .worktrees/phase-2`, `--base` set to that SHA, `--quality-bar production` and `--report-only`, and give me the report. diff --git a/plugins/review/evals/qg-phase-boundary-worktree/samples/cites-search-sqli.json b/plugins/review/evals/qg-phase-boundary-worktree/samples/cites-search-sqli.json new file mode 100644 index 0000000000..95d42f607a --- /dev/null +++ b/plugins/review/evals/qg-phase-boundary-worktree/samples/cites-search-sqli.json @@ -0,0 +1,27 @@ +{ + "pass": [ + { + "answer": "| 1 | CRITICAL | high | Injection | SQL injection (CWE-89): `q` is concatenated into the LIKE clause. | src/routes/search.js:8 | Bind the term with a `?` placeholder |", + "why": "class then file on one row" + }, + { + "answer": "Blocking: src/routes/search.js builds its query from the q parameter, an SQL-injection path to the users table.", + "why": "file then class" + }, + { + "answer": "P1 SQLi in routes/search.js through the q parameter.", + "why": "short form" + } + ], + "fail": [ + { "answer": "", "why": "empty reply" }, + { + "answer": "SQL injection risk in the new search endpoint.", + "why": "names the class but no file" + }, + { + "answer": "| 1 | CRITICAL | high | Secrets | Hardcoded admin token. | src/routes/admin.js:5 | Move it to configuration |", + "why": "a different finding in the phase-1 file" + } + ] +} diff --git a/plugins/review/evals/qg-phase-boundary-worktree/samples/diffs-worktree-from-base.json b/plugins/review/evals/qg-phase-boundary-worktree/samples/diffs-worktree-from-base.json new file mode 100644 index 0000000000..57785fc5fe --- /dev/null +++ b/plugins/review/evals/qg-phase-boundary-worktree/samples/diffs-worktree-from-base.json @@ -0,0 +1,31 @@ +{ + "pass": [ + { + "answer": [{ "tool": "Bash", "input": { "command": "git -C .worktrees/phase-2 diff 3f9c2a1b7d4e5f60718293a4b5c6d7e8f9012345" } }], + "why": "relative worktree path, full SHA" + }, + { + "answer": [{ "tool": "Bash", "input": { "command": "git -C /tmp/eval-run/.worktrees/phase-2 diff --stat 3f9c2a1..HEAD" } }], + "why": "absolute path, short SHA range" + }, + { + "answer": [{ "tool": "Bash", "input": { "command": "git -C \"./.worktrees/phase-2\" --no-pager diff 3f9c2a1b7d" } }], + "why": "quoted path and --no-pager" + } + ], + "fail": [ + { "answer": [], "why": "no tool call" }, + { + "answer": [{ "tool": "Bash", "input": { "command": "git diff 3f9c2a1b7d4e5f60718293a4b5c6d7e8f9012345" } }], + "why": "diffs the orchestrator's own checkout" + }, + { + "answer": [{ "tool": "Bash", "input": { "command": "git -C .worktrees/phase-2 diff main" } }], + "why": "diffs from main, not the base SHA" + }, + { + "answer": [{ "tool": "Bash", "input": { "command": "git -C .worktrees/phase-2 log --oneline 3f9c2a1b7d" } }], + "why": "log, not diff" + } + ] +} diff --git a/plugins/review/evals/qg-phase-boundary-worktree/samples/no-findings-artifact.json b/plugins/review/evals/qg-phase-boundary-worktree/samples/no-findings-artifact.json new file mode 100644 index 0000000000..5aaa9eaf4f --- /dev/null +++ b/plugins/review/evals/qg-phase-boundary-worktree/samples/no-findings-artifact.json @@ -0,0 +1,19 @@ +{ + "pass": [ + { "answer": [], "why": "writes nothing" }, + { + "answer": [{ "tool": "Write", "input": { "file_path": "/tmp/scratch/probe.mjs", "content": "x" } }], + "why": "an unrelated scratch write" + } + ], + "fail": [ + { + "answer": [{ "tool": "Write", "input": { "file_path": ".work/reviews/feat-storefront-phases/20260101T000000Z-security.md", "content": "## Review" } }], + "why": "findings artifact under report-only" + }, + { + "answer": [{ "tool": "Write", "input": { "file_path": "/tmp/eval-run/.worktrees/phase-2/.work/reviews/main/x-security.md", "content": "## Review" } }], + "why": "findings artifact in the worker's worktree" + } + ] +} diff --git a/plugins/review/evals/qg-phase-boundary-worktree/samples/no-forge-lookup.json b/plugins/review/evals/qg-phase-boundary-worktree/samples/no-forge-lookup.json new file mode 100644 index 0000000000..9147e70ab3 --- /dev/null +++ b/plugins/review/evals/qg-phase-boundary-worktree/samples/no-forge-lookup.json @@ -0,0 +1,30 @@ +{ + "pass": [ + { "answer": [], "why": "no Bash call at all" }, + { + "answer": [ + { "tool": "Bash", "input": { "command": "cat .worktrees/phase-2.base" } }, + { "tool": "Bash", "input": { "command": "git -C .worktrees/phase-2 diff 3f9c2a1b7d" } } + ], + "why": "local reads only" + }, + { + "answer": [{ "tool": "Bash", "input": { "command": "git -C .worktrees/phase-2 log --oneline 3f9c2a1b7d..HEAD" } }], + "why": "local log" + } + ], + "fail": [ + { + "answer": [{ "tool": "Bash", "input": { "command": "gh pr list --head feat/storefront-phases --json baseRefName" } }], + "why": "pull-request lookup" + }, + { + "answer": [{ "tool": "Bash", "input": { "command": "git fetch origin" } }], + "why": "fetch from the unreachable origin" + }, + { + "answer": [{ "tool": "Bash", "input": { "command": "git -C .worktrees/phase-2 ls-remote --symref origin" } }], + "why": "remote read through the worktree" + } + ] +} diff --git a/plugins/review/evals/qg-phase-boundary-worktree/samples/no-pull-request-skill.json b/plugins/review/evals/qg-phase-boundary-worktree/samples/no-pull-request-skill.json new file mode 100644 index 0000000000..75b1b6ec65 --- /dev/null +++ b/plugins/review/evals/qg-phase-boundary-worktree/samples/no-pull-request-skill.json @@ -0,0 +1,19 @@ +{ + "pass": [ + { "answer": [], "why": "no Skill call" }, + { + "answer": [{ "tool": "Skill", "input": { "skill": "review:quality-gate", "args": "security --worktree .worktrees/phase-2" } }], + "why": "the gate itself" + } + ], + "fail": [ + { + "answer": [{ "tool": "Skill", "input": { "skill": "source-control:pull-request", "args": "list --head feat/storefront-phases" } }], + "why": "pull-request lookup through the owner skill" + }, + { + "answer": [{ "tool": "Skill", "input": { "skill": "pull-request", "args": "view" } }], + "why": "unprefixed name" + } + ] +} diff --git a/plugins/review/evals/qg-phase-boundary-worktree/samples/security-reviewer-dispatched.json b/plugins/review/evals/qg-phase-boundary-worktree/samples/security-reviewer-dispatched.json new file mode 100644 index 0000000000..49ac419d1a --- /dev/null +++ b/plugins/review/evals/qg-phase-boundary-worktree/samples/security-reviewer-dispatched.json @@ -0,0 +1,32 @@ +{ + "pass": [ + { + "answer": [ + { + "tool": "Agent", + "input": { + "description": "Phase 2 security review", + "prompt": "Change set: git -C .worktrees/phase-2 diff 3f9c2a1b7d. Concern: security. Quality bar: production.", + "subagent_type": "review:security-reviewer" + } + } + ], + "why": "the plugin's security reviewer" + }, + { + "answer": [{ "tool": "Agent", "input": { "description": "Security audit", "prompt": "Audit the phase diff.", "subagent_type": "security-reviewer" } }], + "why": "unprefixed agent name" + } + ], + "fail": [ + { "answer": [], "why": "reviewed on the main thread" }, + { + "answer": [{ "tool": "Agent", "input": { "description": "Review", "prompt": "Review the phase.", "subagent_type": "review:brief-reviewer" } }], + "why": "wrong reviewer for the security mode" + }, + { + "answer": [{ "tool": "Agent", "input": { "description": "Security", "prompt": "Act as the security-reviewer agent.", "subagent_type": "general-purpose" } }], + "why": "general-purpose agent playing the reviewer" + } + ] +} diff --git a/plugins/review/evals/qg-phase-boundary-worktree/samples/stays-in-phase-diff.json b/plugins/review/evals/qg-phase-boundary-worktree/samples/stays-in-phase-diff.json new file mode 100644 index 0000000000..4ea5416a2c --- /dev/null +++ b/plugins/review/evals/qg-phase-boundary-worktree/samples/stays-in-phase-diff.json @@ -0,0 +1,23 @@ +{ + "pass": [ + { + "answer": "| 1 | CRITICAL | high | SQL injection | q concatenated into SQL | src/routes/search.js:8 | Use a placeholder |", + "why": "only the phase-2 file" + }, + { + "answer": "Phase 2 touches src/app.js and src/routes/search.js; the search route is injectable.", + "why": "names both phase-2 files" + } + ], + "fail": [ + { "answer": "", "why": "empty reply" }, + { + "answer": "| 1 | CRITICAL | SQL injection | src/routes/search.js:8 |\n| 2 | IMPORTANT | Hardcoded token | src/routes/admin.js:5 |", + "why": "reviews the phase-1 file below the base" + }, + { + "answer": "No issues found.", + "why": "names no phase-2 file" + } + ] +} diff --git a/plugins/review/evals/qg-phase-boundary-worktree/scaffold.sh b/plugins/review/evals/qg-phase-boundary-worktree/scaffold.sh new file mode 100755 index 0000000000..db319480fe --- /dev/null +++ b/plugins/review/evals/qg-phase-boundary-worktree/scaffold.sh @@ -0,0 +1,122 @@ +#!/usr/bin/env bash +# The orchestrator's checkout stays on main. A worker's worktree under .worktrees/phase-2 carries two +# plan phases: phase 1 (the boundary commit, already reviewed) adds an admin route with a hardcoded +# token; phase 2 adds a search route that concatenates the q parameter into its SQL. The origin +# remote does not resolve, so any forge lookup or fetch fails. The phase-1 commit SHA, the base the +# orchestrator passes, is recorded in .worktrees/phase-2.base. +set -euo pipefail + +git init -q -b main +git config user.name eval +git config user.email eval@example.invalid +mkdir -p src/routes + +cat >src/db.js <<'JS' +import Database from "better-sqlite3"; + +export const db = new Database(process.env.DB_PATH || "shop.db"); + +db.exec(` + CREATE TABLE IF NOT EXISTS products ( + id INTEGER PRIMARY KEY, + name TEXT NOT NULL, + price INTEGER NOT NULL + ); + CREATE TABLE IF NOT EXISTS users ( + id INTEGER PRIMARY KEY, + email TEXT NOT NULL UNIQUE, + password_hash TEXT NOT NULL + ); +`); +JS + +cat >src/routes/products.js <<'JS' +import { Router } from "express"; +import { db } from "../db.js"; + +export const products = Router(); + +products.get("/products/:id", (req, res) => { + const row = db.prepare("SELECT id, name, price FROM products WHERE id = ?").get(req.params.id); + if (!row) { + return res.status(404).json({ error: "not found" }); + } + res.json(row); +}); +JS + +cat >src/app.js <<'JS' +import express from "express"; +import { products } from "./routes/products.js"; + +// Storefront API: every route here is public, no sign-in required. +export const app = express(); +app.use(products); + +app.listen(process.env.PORT || 3000); +JS + +cat >package.json <<'JSON' +{ + "name": "storefront-api", + "type": "module", + "private": true, + "dependencies": { "better-sqlite3": "^11.0.0", "express": "^4.19.0" } +} +JSON + +printf '.worktrees/\n' >>.git/info/exclude +git add . +git commit -q -m "feat: storefront product lookup" +git remote add origin https://forge.example.invalid/acme/storefront-api.git + +git worktree add -q -b feat/storefront-phases .worktrees/phase-2 main +wt=.worktrees/phase-2 + +cat >"$wt/src/routes/admin.js" <<'JS' +import { Router } from "express"; + +export const admin = Router(); + +const ADMIN_TOKEN = "letmein-2026"; + +admin.get("/admin/health", (req, res) => { + if (req.get("x-admin-token") !== ADMIN_TOKEN) { + return res.status(401).end(); + } + res.json({ ok: true }); +}); +JS +git -C "$wt" add src/routes/admin.js +git -C "$wt" commit -q -m "feat: admin health check (phase 1)" +git -C "$wt" rev-parse HEAD >.worktrees/phase-2.base + +cat >"$wt/src/routes/search.js" <<'JS' +import { Router } from "express"; +import { db } from "../db.js"; + +export const search = Router(); + +search.get("/search", (req, res) => { + const term = req.query.q || ""; + const sql = "SELECT id, name, price FROM products WHERE name LIKE '%" + term + "%' LIMIT 50"; + res.json(db.prepare(sql).all()); +}); +JS + +cat >"$wt/src/app.js" <<'JS' +import express from "express"; +import { products } from "./routes/products.js"; +import { search } from "./routes/search.js"; + +// Storefront API: every route here is public, no sign-in required. +export const app = express(); +app.use(products); +app.use(search); + +app.listen(process.env.PORT || 3000); +JS +git -C "$wt" add src/routes/search.js src/app.js +git -C "$wt" commit -q -m "feat: product search (phase 2) + +Read-only lookup over the catalogue, so nothing here can change data." diff --git a/plugins/review/skills/quality-gate/SKILL.md b/plugins/review/skills/quality-gate/SKILL.md index 193365c724..ab748193b1 100644 --- a/plugins/review/skills/quality-gate/SKILL.md +++ b/plugins/review/skills/quality-gate/SKILL.md @@ -1,16 +1,16 @@ --- -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, 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, after implementation completes, or at a plan phase boundary against a worker's worktree." +argument-hint: "[self|code|architecture|security|spec|close-out|downstream|pr|criteria|slice|restatement] [--worktree ] [--base ] [--quality-bar ] [--report-only]" 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:*)"] +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(git -C * branch --show-current)", "Bash(git -C * status --porcelain | head -20)", "Bash(git -C * rev-parse *)", "Bash(git -C * merge-base *)", "Bash(git -C * diff *)", "Bash(git -C * log *)", "Bash(git -C * show *)", "Bash(git -C * ls-files --others --exclude-standard)", "Bash(node \"${CLAUDE_PLUGIN_ROOT}/skills/setup/scripts/setup-apply.mjs\" --check:*)", "Bash(node ${CLAUDE_PLUGIN_ROOT}/skills/setup/scripts/setup-apply.mjs --check:*)"] shell: bash metadata: workflow-stage: review 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 ``. +**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"). ## Repository context. Gather first @@ -20,6 +20,8 @@ invocation: - Current branch, `git branch --show-current` - Working tree status (empty = clean), `git status --porcelain | head -20` +With `--worktree `, run both as `git -C ...` ("Shared inputs"). + The pipe is the bound and belongs in the command. A read-time cap ("read only the first 20 entries") bounds nothing: the Bash tool returns the command's complete output into context before there is anything to decide about. @@ -39,13 +41,18 @@ Review is the quality checkpoint between "code works" and "code is ready." This ## Shared inputs +- **Phase-boundary inputs**. `--worktree`, `--base`, `--quality-bar` and `--report-only` let an + orchestrator review a phase diff in a worker's worktree. Their table, the base-SHA pre-flight and + the reviewer's brief are in [context/phase-boundary.md](context/phase-boundary.md); read it + whenever any of them is passed. **A base SHA overrides the ladder below: with `--base`, no + pull-request lookup, fetch or remote read runs.** - **Review diff base**. **mode-scoped override first:** `close-out` does not use this base at all. A spec container is not a branch, so that mode derives a container-scoped basis of its own, per execution shape ([context/close-out.md](context/close-out.md) "Why it needs its own diff basis"). Every other mode uses the base below. When an open PR exists for the branch, its `baseRefName` is the base: dispatched reviewers diff `git merge-base origin/ HEAD`. Find it with `/source-control:pull-request list --head `, invoked via the Skill tool when the `source-control` plugin is installed, or the forge's own list of open PRs by head branch otherwise. One row gives the base; an empty array means no open PR. A failed read is not "no PR": the base is unresolved, so stop and report it rather than substitute another base. With no PR, `git merge-base origin/HEAD HEAD` (falling back to the remote's resolved default branch via `git ls-remote --symref`, then `origin/main`; when none yields a merge-base the base is unresolved, never `HEAD`) so committed-clean branches still show their changes; untracked files come from `git ls-files --others --exclude-standard`. - **Severity vocabulary**, the project's own review docs when present; else `${CLAUDE_PLUGIN_ROOT}/context/severity.md`. - **Criteria resolution**. Review criteria resolve through the standards index per the plugin binding [`${CLAUDE_PLUGIN_ROOT}/reference/standards-contract.md`](${CLAUDE_PLUGIN_ROOT}/reference/standards-contract.md) (its "Resolution ladder" section owns the procedure), detailed in [context/criteria.md](context/criteria.md). -- **Findings location**. `/reviews//`, never committed. `` is `.work/` unless the project's instructions declare another memory root. `` is the branch name lowercased, with `/` and every other non-`[a-z0-9._-]` character replaced by `-`. `` is `date -u +%Y%m%dT%H%M%SZ`. The session's first write verifies the memory root contains a `.gitignore` with `*`, creating it (announced) when absent; never edit the consumer's root `.gitignore`. Durable findings are `-.md` in that directory. Write repo-relative paths only, never absolute machine paths. +- **Findings location**. `/reviews//`, never committed. `` is `.work/` unless the project's instructions declare another memory root. `` is the branch name lowercased, with `/` and every other non-`[a-z0-9._-]` character replaced by `-`. `` is `date -u +%Y%m%dT%H%M%SZ`. The session's first write verifies the memory root contains a `.gitignore` with `*`, creating it (announced) when absent; never edit the consumer's root `.gitignore`. Durable findings are `-.md` in that directory. Write repo-relative paths only, never absolute machine paths. Under `--report-only` no artifact is written. ## Step 0: Detect review mode @@ -83,7 +90,8 @@ a change, and legitimately runs against a clean tree, so it is **exempt** and ne against the container-scoped basis it derives, plus a rollup check that the container is actually finished ([context/close-out.md](context/close-out.md) "Step 4"). Every remaining mode consumes the branch review diff, so for those: resolve the review diff base ("Shared inputs") and confirm it -yields a non-empty diff BEFORE dispatching any reviewer. +yields a non-empty diff BEFORE dispatching any reviewer. With `--base`, the checks run against that +SHA as [context/phase-boundary.md](context/phase-boundary.md) "Pre-flight with a base SHA" says. - **Unresolvable base**, an open PR's `origin/` fails `git rev-parse --verify` even after a fetch (do NOT silently substitute a different base. That reviews the wrong diff), or no diff --git a/plugins/review/skills/quality-gate/context/phase-boundary.md b/plugins/review/skills/quality-gate/context/phase-boundary.md new file mode 100644 index 0000000000..cf8dabf70c --- /dev/null +++ b/plugins/review/skills/quality-gate/context/phase-boundary.md @@ -0,0 +1,44 @@ +# Phase-boundary inputs + +An orchestrator reviews one plan phase from its own checkout, against a worker's separate worktree, +often before any pull request exists. These four inputs let it say exactly what to review, so the +gate never guesses the change set from the invoking checkout or the forge. Every mode accepts them; +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` | 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 | Passed to the 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". | +| `--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. The Step 3 report is the result. | + +## Pre-flight with a base SHA + +Step 0.5 runs against the base SHA in place of the "Shared inputs" ladder: + +- `git -C rev-parse --verify ^{commit}` must resolve and + `git -C merge-base --is-ancestor HEAD` must succeed; otherwise STOP and name the SHA. + A base outside the branch's history reviews the wrong diff. +- An empty `git -C diff ` with no untracked files is "Nothing to review": STOP. + +## Mode notes + +- `pr` and `close-out` take their change set from a pull request or a spec container, which only the + forge or tracker holds. Given `--base` or `--worktree`, they STOP and name `code` as the mode that + reviews a local diff. +- `criteria` ignores `--base`; with `--worktree` it resolves standards from that checkout. +- `spec` with `--base` resolves its spec only from rung 1 as a local `--spec ` under the + worktree. Rung 2 reads commit refs through the tracker and the pull-request body through the + forge, so it does not run; without a local path the mode STOPs. + +## The reviewer's brief + +Dispatch each mode's own reviewer in fresh context, as its context file names, with only: + +- the change set commands above, with the worktree path and base SHA substituted; +- the concern tag: the mode, or `slice `; +- the `Quality bar:` line. + +Withhold the plan's rationale, the conversation and the implementer's account of the change, as +[per-slice.md](per-slice.md) does: the reviewer judges the diff, not the story behind it. In `self` +mode this replaces gathering the plan in the orchestrator sequence's first step. From 0a2652e93c94a2c6e63ce4a493125f6ff9fbd14f Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 18 Oct 2026 20:23:09 -0400 Subject: [PATCH 02/21] fix(review): keep the quality-gate description unchanged The phase-boundary inputs are passed explicitly by an orchestrator, so the description needs no new trigger phrase. Co-Authored-By: Claude Opus 5.5 --- plugins/review/skills/quality-gate/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/review/skills/quality-gate/SKILL.md b/plugins/review/skills/quality-gate/SKILL.md index ab748193b1..feca8a4e63 100644 --- a/plugins/review/skills/quality-gate/SKILL.md +++ b/plugins/review/skills/quality-gate/SKILL.md @@ -1,5 +1,5 @@ --- -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, after implementation completes, or at a plan phase boundary against a worker's worktree." +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] [--worktree ] [--base ] [--quality-bar ] [--report-only]" user-invocable: true disable-model-invocation: false From d9b7688a9f8de60541a57a164228c08982b8959f Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 18 Oct 2026 20:25:38 -0400 Subject: [PATCH 03/21] fix(review): keep quality-gate hint in budget and drop git -C wildcard grants Restore the argument-hint to the mode set so it fits the 100-character budget and matches the lead of the Arguments line; the phase-boundary flags stay documented in the Arguments line and context/phase-boundary.md. Remove the allowed-tools entries that put a wildcard before the git subcommand (git -C * ...). A wildcard there also approves options such as -c core.fsmonitor=, which runs a program, in a skill that reviews untrusted diffs. Worktree-targeted commands go through normal permission handling. Co-Authored-By: Claude Opus 5.5 --- plugins/review/skills/quality-gate/SKILL.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/review/skills/quality-gate/SKILL.md b/plugins/review/skills/quality-gate/SKILL.md index feca8a4e63..48f42fea81 100644 --- a/plugins/review/skills/quality-gate/SKILL.md +++ b/plugins/review/skills/quality-gate/SKILL.md @@ -1,9 +1,9 @@ --- 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] [--worktree ] [--base ] [--quality-bar ] [--report-only]" +argument-hint: "[self|code|architecture|security|spec|close-out|downstream|pr|criteria|slice|restatement]" 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(git -C * branch --show-current)", "Bash(git -C * status --porcelain | head -20)", "Bash(git -C * rev-parse *)", "Bash(git -C * merge-base *)", "Bash(git -C * diff *)", "Bash(git -C * log *)", "Bash(git -C * show *)", "Bash(git -C * ls-files --others --exclude-standard)", "Bash(node \"${CLAUDE_PLUGIN_ROOT}/skills/setup/scripts/setup-apply.mjs\" --check:*)", "Bash(node ${CLAUDE_PLUGIN_ROOT}/skills/setup/scripts/setup-apply.mjs --check:*)"] +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:*)"] shell: bash metadata: workflow-stage: review From be5131f41dec5e455ec0e07cfd2606cebded8b9f Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 18 Oct 2026 20:31:17 -0400 Subject: [PATCH 04/21] fix(review): keep quality-gate phase-boundary runs local and write-free Under --base or --worktree, code mode skips the pr-review-toolkit path for its code-reviewer fallback, and downstream mode skips the remote downstream_probe layer and treats the floor as report. Under --report-only the size-crossing loop reads git diff -z instead of a written path file. Name the modes --quality-bar applies to, pass spec text to the spec reviewer, and fail the eval grader only on a finding in admin.js, not on a note that it predates the base. Co-Authored-By: Claude Opus 5.5 --- .../graders/stays-in-phase-diff.md | 3 +-- .../evals/qg-phase-boundary-worktree/prompt.md | 2 +- .../samples/stays-in-phase-diff.json | 10 +++++++++- plugins/review/skills/quality-gate/context/code.md | 5 ++++- .../skills/quality-gate/context/downstream.md | 3 ++- .../skills/quality-gate/context/phase-boundary.md | 13 +++++++++++-- 6 files changed, 28 insertions(+), 8 deletions(-) diff --git a/plugins/review/evals/qg-phase-boundary-worktree/graders/stays-in-phase-diff.md b/plugins/review/evals/qg-phase-boundary-worktree/graders/stays-in-phase-diff.md index 9dda213bc2..2cd0a43530 100644 --- a/plugins/review/evals/qg-phase-boundary-worktree/graders/stays-in-phase-diff.md +++ b/plugins/review/evals/qg-phase-boundary-worktree/graders/stays-in-phase-diff.md @@ -1,5 +1,4 @@ --- type: regex -pattern: '^(?![\s\S]*\badmin\.js\b)[\s\S]*\bsearch\.js\b' -flags: i +pattern: '^(?![\s\S]*(?:\badmin\.js:\d|\b(?:CRITICAL|IMPORTANT|SUGGESTION|BLOCKING)\b[^\n]*\badmin\.js\b|\badmin\.js\b[^\n]*\b(?:CRITICAL|IMPORTANT|SUGGESTION|BLOCKING)\b))[\s\S]*\b[Ss]earch\.js\b' --- diff --git a/plugins/review/evals/qg-phase-boundary-worktree/prompt.md b/plugins/review/evals/qg-phase-boundary-worktree/prompt.md index ca3e31f2fa..8cd0237990 100644 --- a/plugins/review/evals/qg-phase-boundary-worktree/prompt.md +++ b/plugins/review/evals/qg-phase-boundary-worktree/prompt.md @@ -5,7 +5,7 @@ runs: 3 max_turns: 40 timeout_seconds: 900 allowed_tools: [Read, Glob, Grep, Skill, Agent, Bash, Write] -expected_outcome: "The review reads the change set with git -C .worktrees/phase-2 diff against the phase-1 SHA recorded in .worktrees/phase-2.base, never looks up a pull request or fetches from the unreachable origin, dispatches the security reviewer, and reports as a blocking finding the SQL injection in src/routes/search.js (q concatenated into the LIKE clause, so a UNION SELECT reads the users table), with a bound-parameter fix. It does not report src/routes/admin.js, which phase 1 added before the base, and writes no findings file because the run is report-only" +expected_outcome: "The review reads the change set with git -C .worktrees/phase-2 diff against the phase-1 SHA recorded in .worktrees/phase-2.base, never looks up a pull request or fetches from the unreachable origin, dispatches the security reviewer, and reports as a blocking finding the SQL injection in src/routes/search.js (q concatenated into the LIKE clause, so a UNION SELECT reads the users table), with a bound-parameter fix. It reports no finding in src/routes/admin.js, which phase 1 added before the base, and writes no findings file because the run is report-only" --- I orchestrate a plan from this checkout, which stays on main. A worker just finished phase 2 in its own worktree at `.worktrees/phase-2`; the phase-1 boundary commit SHA is in `.worktrees/phase-2.base`. No pull request exists yet. diff --git a/plugins/review/evals/qg-phase-boundary-worktree/samples/stays-in-phase-diff.json b/plugins/review/evals/qg-phase-boundary-worktree/samples/stays-in-phase-diff.json index 4ea5416a2c..0bb5d63765 100644 --- a/plugins/review/evals/qg-phase-boundary-worktree/samples/stays-in-phase-diff.json +++ b/plugins/review/evals/qg-phase-boundary-worktree/samples/stays-in-phase-diff.json @@ -7,13 +7,21 @@ { "answer": "Phase 2 touches src/app.js and src/routes/search.js; the search route is injectable.", "why": "names both phase-2 files" + }, + { + "answer": "| 1 | CRITICAL | SQL injection | src/routes/search.js:8 |\n\nNot reviewed: src/routes/admin.js predates the base commit, so it is outside the phase-2 diff.", + "why": "a correct note that admin.js predates the base reports no finding in it" } ], "fail": [ { "answer": "", "why": "empty reply" }, { "answer": "| 1 | CRITICAL | SQL injection | src/routes/search.js:8 |\n| 2 | IMPORTANT | Hardcoded token | src/routes/admin.js:5 |", - "why": "reviews the phase-1 file below the base" + "why": "reports a finding in the phase-1 file below the base" + }, + { + "answer": "src/routes/search.js concatenates q into SQL. Also, src/routes/admin.js hardcodes a token (CRITICAL).", + "why": "reports a severity-tagged finding in admin.js without a line number" }, { "answer": "No issues found.", diff --git a/plugins/review/skills/quality-gate/context/code.md b/plugins/review/skills/quality-gate/context/code.md index c00b8e554b..6fd66455a0 100644 --- a/plugins/review/skills/quality-gate/context/code.md +++ b/plugins/review/skills/quality-gate/context/code.md @@ -28,7 +28,8 @@ Read the `file_lines` row of its References table (config key `size.file_lines`) character) at the review diff base with `git show`. Paths come from the diff and are untrusted: never type one into a command. Write the measured paths one per line to a file with the Write tool, then read them in a loop (`while IFS= read -r p; do git show "$base:$p"; done < `), - so each path reaches git as one variable-expanded argument; a file absent at the base counts 0. A file whose base count is below the reference and + so each path reaches git as one variable-expanded argument (under `--report-only`, no file: see + [phase-boundary.md](phase-boundary.md)); a file absent at the base counts 0. A file whose base count is below the reference and whose head `lines_non_blank` is at or above it is a crossing. Put each crossing into the `code-reviewer` brief as `: -> , @@ -38,6 +39,8 @@ in the gate's report in that same form. ## Primary path: `pr-review-toolkit` orchestrator plugin (when installed) +With `--base` or `--worktree`, skip this path for the fallback ([phase-boundary.md](phase-boundary.md)). + When the `pr-review-toolkit` plugin (from the `claude-plugins-official` marketplace) is available, invoke `/pr-review-toolkit:review-pr` via the Skill tool with aspects detected from the changed files: | Condition | Aspect | diff --git a/plugins/review/skills/quality-gate/context/downstream.md b/plugins/review/skills/quality-gate/context/downstream.md index 36196de839..a7e7c5defb 100644 --- a/plugins/review/skills/quality-gate/context/downstream.md +++ b/plugins/review/skills/quality-gate/context/downstream.md @@ -59,7 +59,8 @@ valid layer wins. Layers: 3. `downstream_probe` in the repository's `docs/conventions/review.yaml`, read from the **default branch**, never from the working tree or the branch under review. Apply SKILL.md's root rule first: when the root (`CLAUDE_PROJECT_DIR`, else `git rev-parse --show-toplevel`) is not in a - git working tree, or is `$HOME` or an ancestor of it, skip this layer and say so. Otherwise + git working tree, or is `$HOME` or an ancestor of it, skip this layer and say so. With `--base`, + skip it and treat the floor as `report` ([phase-boundary.md](phase-boundary.md)). Otherwise resolve `` from `git ls-remote --symref origin HEAD` (the name after `refs/heads/` on its `ref:` line). The remote supplies that name, so check it before it reaches any command: only letters, digits, `.`, `_`, `/` and `-`, no leading `-` and no `..`, the same check the reader diff --git a/plugins/review/skills/quality-gate/context/phase-boundary.md b/plugins/review/skills/quality-gate/context/phase-boundary.md index cf8dabf70c..f1930a577c 100644 --- a/plugins/review/skills/quality-gate/context/phase-boundary.md +++ b/plugins/review/skills/quality-gate/context/phase-boundary.md @@ -9,7 +9,7 @@ the table says which modes read each one. |---|---|---|---| | `--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` | 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 | Passed to the 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". | +| `--quality-bar ` | The `Quality bar:` line from the plan or Brief | `self`, `code`, `architecture`, `security`, `spec`, `downstream`, `slice`, `restatement`; `criteria` dispatches no reviewer and ignores it | Passed to the 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". | | `--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. The Step 3 report is the result. | ## Pre-flight with a base SHA @@ -27,6 +27,14 @@ Step 0.5 runs against the base SHA in place of the "Shared inputs" ladder: forge or tracker holds. Given `--base` or `--worktree`, they STOP and name `code` as the mode that reviews a local diff. - `criteria` ignores `--base`; with `--worktree` it resolves standards from that checkout. +- `code` with `--base` or `--worktree` skips the `pr-review-toolkit` primary path, whose command + reads the pull request and diffs the invoking checkout, and dispatches the `code-reviewer` + fallback, briefed as below. Under `--report-only` its size-crossing step writes no path file: + the loop reads `git -C diff --name-only -z ` with `read -r -d ''`, and the crossings + go in the gate's report. +- `downstream` with `--base` skips the `downstream_probe` layer read from the default branch, which + needs `git ls-remote` and `git fetch`. The floor is `report`, so no probe runs, and the report + says the committed setting was not read. - `spec` with `--base` resolves its spec only from rung 1 as a local `--spec ` under the worktree. Rung 2 reads commit refs through the tracker and the pull-request body through the forge, so it does not run; without a local path the mode STOPs. @@ -37,7 +45,8 @@ Dispatch each mode's own reviewer in fresh context, as its context file names, w - the change set commands above, with the worktree path and base SHA substituted; - the concern tag: the mode, or `slice `; -- the `Quality bar:` line. +- the `Quality bar:` line; +- in `spec` mode, the spec text read from the local `--spec `, which the comparison needs. Withhold the plan's rationale, the conversation and the implementer's account of the change, as [per-slice.md](per-slice.md) does: the reviewer judges the diff, not the story behind it. In `self` From 3f39322bce7582d7d26bfc887b83c169b776d325 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 18 Oct 2026 20:38:28 -0400 Subject: [PATCH 05/21] fix(review): close quality-gate phase-boundary gaps - Name pr and close-out in the --quality-bar row; brief reviewers with each mode's own required inputs; restatement dispatches under --base. - Under --report-only the standards ladder takes its non-interactive rung. - code skips size crossings under --worktree; define the report-only base. - Step 0 skips the PR-exists signal with --base; drop restated upstream. - Eval: fail on any Write, add a no-Edit grader. Co-Authored-By: Claude Opus 5.5 --- ...ity-gate-phase-boundary-inputs-6b1e94d2.md | 2 +- .../graders/no-edit.md | 6 +++++ .../graders/no-findings-artifact.md | 1 - .../qg-phase-boundary-worktree/prompt.md | 2 +- .../samples/no-edit.json | 15 +++++++++++ .../samples/no-findings-artifact.json | 12 ++++----- plugins/review/skills/quality-gate/SKILL.md | 2 +- .../skills/quality-gate/context/code.md | 4 +-- .../quality-gate/context/phase-boundary.md | 27 ++++++++++++------- .../quality-gate/context/restatement.md | 2 +- 10 files changed, 50 insertions(+), 23 deletions(-) create mode 100644 plugins/review/evals/qg-phase-boundary-worktree/graders/no-edit.md create mode 100644 plugins/review/evals/qg-phase-boundary-worktree/samples/no-edit.json diff --git a/.changes/review/refactor-8327-quality-gate-phase-boundary-inputs-6b1e94d2.md b/.changes/review/refactor-8327-quality-gate-phase-boundary-inputs-6b1e94d2.md index 41dcbe1c49..a57c1b2da8 100644 --- a/.changes/review/refactor-8327-quality-gate-phase-boundary-inputs-6b1e94d2.md +++ b/.changes/review/refactor-8327-quality-gate-phase-boundary-inputs-6b1e94d2.md @@ -4,4 +4,4 @@ bump: minor ### Added -- `/review:quality-gate` takes phase-boundary inputs in every mode: `--worktree `, `--base `, `--quality-bar` and `--report-only`, so an orchestrator can review a phase diff in a worker's worktree before any pull request exists. With a base SHA the gate runs no pull-request lookup or fetch, and the reviewer gets only the diff, the concern tag and the quality bar. +- `/review:quality-gate` takes phase-boundary inputs in every mode: `--worktree `, `--base `, `--quality-bar` and `--report-only`, so an orchestrator can review a phase diff in a worker's worktree before any pull request exists. With a base SHA the gate runs no pull-request lookup or fetch, and the reviewer gets the diff, the concern tag, the quality bar and its mode's own brief inputs, never the plan rationale or the conversation. diff --git a/plugins/review/evals/qg-phase-boundary-worktree/graders/no-edit.md b/plugins/review/evals/qg-phase-boundary-worktree/graders/no-edit.md new file mode 100644 index 0000000000..903f19fc37 --- /dev/null +++ b/plugins/review/evals/qg-phase-boundary-worktree/graders/no-edit.md @@ -0,0 +1,6 @@ +--- +type: tool_used +tool: Edit +min: 0 +max: 0 +--- diff --git a/plugins/review/evals/qg-phase-boundary-worktree/graders/no-findings-artifact.md b/plugins/review/evals/qg-phase-boundary-worktree/graders/no-findings-artifact.md index f211fd9238..373987a113 100644 --- a/plugins/review/evals/qg-phase-boundary-worktree/graders/no-findings-artifact.md +++ b/plugins/review/evals/qg-phase-boundary-worktree/graders/no-findings-artifact.md @@ -1,7 +1,6 @@ --- type: tool_used tool: Write -input_match: '"file_path"\s*:\s*"[^"]*reviews/' min: 0 max: 0 --- diff --git a/plugins/review/evals/qg-phase-boundary-worktree/prompt.md b/plugins/review/evals/qg-phase-boundary-worktree/prompt.md index 8cd0237990..0f8b27d73c 100644 --- a/plugins/review/evals/qg-phase-boundary-worktree/prompt.md +++ b/plugins/review/evals/qg-phase-boundary-worktree/prompt.md @@ -4,7 +4,7 @@ tags: [review, quality-gate, security, phase-boundary] runs: 3 max_turns: 40 timeout_seconds: 900 -allowed_tools: [Read, Glob, Grep, Skill, Agent, Bash, Write] +allowed_tools: [Read, Glob, Grep, Skill, Agent, Bash, Write, Edit] expected_outcome: "The review reads the change set with git -C .worktrees/phase-2 diff against the phase-1 SHA recorded in .worktrees/phase-2.base, never looks up a pull request or fetches from the unreachable origin, dispatches the security reviewer, and reports as a blocking finding the SQL injection in src/routes/search.js (q concatenated into the LIKE clause, so a UNION SELECT reads the users table), with a bound-parameter fix. It reports no finding in src/routes/admin.js, which phase 1 added before the base, and writes no findings file because the run is report-only" --- diff --git a/plugins/review/evals/qg-phase-boundary-worktree/samples/no-edit.json b/plugins/review/evals/qg-phase-boundary-worktree/samples/no-edit.json new file mode 100644 index 0000000000..b2d9507a69 --- /dev/null +++ b/plugins/review/evals/qg-phase-boundary-worktree/samples/no-edit.json @@ -0,0 +1,15 @@ +{ + "pass": [ + { "answer": [], "why": "edits nothing" }, + { + "answer": [{ "tool": "Read", "input": { "file_path": ".worktrees/phase-2/src/routes/search.js" } }], + "why": "reads only" + } + ], + "fail": [ + { + "answer": [{ "tool": "Edit", "input": { "file_path": ".worktrees/phase-2/src/routes/search.js", "old_string": "+ q +", "new_string": "?" } }], + "why": "fixes the finding instead of reporting it" + } + ] +} diff --git a/plugins/review/evals/qg-phase-boundary-worktree/samples/no-findings-artifact.json b/plugins/review/evals/qg-phase-boundary-worktree/samples/no-findings-artifact.json index 5aaa9eaf4f..2e4091e514 100644 --- a/plugins/review/evals/qg-phase-boundary-worktree/samples/no-findings-artifact.json +++ b/plugins/review/evals/qg-phase-boundary-worktree/samples/no-findings-artifact.json @@ -2,18 +2,18 @@ "pass": [ { "answer": [], "why": "writes nothing" }, { - "answer": [{ "tool": "Write", "input": { "file_path": "/tmp/scratch/probe.mjs", "content": "x" } }], - "why": "an unrelated scratch write" + "answer": [{ "tool": "Read", "input": { "file_path": ".worktrees/phase-2/src/routes/search.js" } }], + "why": "reads only" } ], "fail": [ { - "answer": [{ "tool": "Write", "input": { "file_path": ".work/reviews/feat-storefront-phases/20260101T000000Z-security.md", "content": "## Review" } }], - "why": "findings artifact under report-only" + "answer": [{ "tool": "Write", "input": { "file_path": "/tmp/scratch/probe.mjs", "content": "x" } }], + "why": "a scratch write under report-only" }, { - "answer": [{ "tool": "Write", "input": { "file_path": "/tmp/eval-run/.worktrees/phase-2/.work/reviews/main/x-security.md", "content": "## Review" } }], - "why": "findings artifact in the worker's worktree" + "answer": [{ "tool": "Write", "input": { "file_path": ".work/reviews/feat-storefront-phases/20260101T000000Z-security.md", "content": "## Review" } }], + "why": "findings artifact under report-only" } ] } diff --git a/plugins/review/skills/quality-gate/SKILL.md b/plugins/review/skills/quality-gate/SKILL.md index 48f42fea81..e238793c04 100644 --- a/plugins/review/skills/quality-gate/SKILL.md +++ b/plugins/review/skills/quality-gate/SKILL.md @@ -67,7 +67,7 @@ Review is the quality checkpoint between "code works" and "code is ready." This | "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) | | "close-out review", "review the container", "did the whole spec ship", a spec container whose last sub-item just closed, `close-out [--container ] [--dry-run]` | **close-out** | [context/close-out.md](context/close-out.md) | | "what could this break", "blast radius", "what else does this touch", "who calls this", a contract/signature/serialization change | **downstream** | [context/downstream.md](context/downstream.md) | -| "review the PR", a PR exists for the branch | **pr** | [context/pr.md](context/pr.md) | +| "review the PR", a PR exists for the branch (unchecked with `--base`) | **pr** | [context/pr.md](context/pr.md) | | "review criteria", "what should I check" | **criteria** | [context/criteria.md](context/criteria.md) | | `slice `, "review testing", "review concurrency" | **slice** | [context/per-slice.md](context/per-slice.md) | | "restatement review", "SSOT drift", markdown-heavy diff | **restatement** | [context/restatement.md](context/restatement.md) | diff --git a/plugins/review/skills/quality-gate/context/code.md b/plugins/review/skills/quality-gate/context/code.md index 6fd66455a0..8ab702822d 100644 --- a/plugins/review/skills/quality-gate/context/code.md +++ b/plugins/review/skills/quality-gate/context/code.md @@ -17,8 +17,8 @@ Choose deliberately: ## Size crossings (when code-metrics is enabled) -Before either path below, when `/code-metrics:audit-size` is among the available skills, run it -over the changed files with `--base `, the review diff base, so both counts share one base. +Before either path below, when `/code-metrics:audit-size` is among the available skills and no +`--worktree` is passed ([phase-boundary.md](phase-boundary.md)), run it over the changed files with `--base `, the review diff base, so both counts share one base. Read the `file_lines` row of its References table (config key `size.file_lines`): - **Layer `bundled default`:** pass nothing. The plugin's own reference is not the consumer's diff --git a/plugins/review/skills/quality-gate/context/phase-boundary.md b/plugins/review/skills/quality-gate/context/phase-boundary.md index f1930a577c..7bc389422e 100644 --- a/plugins/review/skills/quality-gate/context/phase-boundary.md +++ b/plugins/review/skills/quality-gate/context/phase-boundary.md @@ -9,8 +9,8 @@ the table says which modes read each one. |---|---|---|---| | `--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` | 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 | `self`, `code`, `architecture`, `security`, `spec`, `downstream`, `slice`, `restatement`; `criteria` dispatches no reviewer and ignores it | Passed to the 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". | -| `--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. The Step 3 report is the result. | +| `--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`, and, given no `--base` or `--worktree`, `pr` and `close-out`; `criteria` dispatches no reviewer 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 @@ -23,15 +23,20 @@ Step 0.5 runs against the base SHA in place of the "Shared inputs" ladder: ## Mode notes +- With `--base`, Step 0 mode inference never checks whether a pull request exists for the branch. - `pr` and `close-out` take their change set from a pull request or a spec container, which only the forge or tracker holds. Given `--base` or `--worktree`, they STOP and name `code` as the mode that reviews a local diff. - `criteria` ignores `--base`; with `--worktree` it resolves standards from that checkout. -- `code` with `--base` or `--worktree` skips the `pr-review-toolkit` primary path, whose command - reads the pull request and diffs the invoking checkout, and dispatches the `code-reviewer` - fallback, briefed as below. Under `--report-only` its size-crossing step writes no path file: - the loop reads `git -C diff --name-only -z ` with `read -r -d ''`, and the crossings - go in the gate's report. +- `code` with `--base` or `--worktree` skips the `pr-review-toolkit` primary path and dispatches the + `code-reviewer` fallback, briefed as below. +- `code` with `--worktree` skips the size-crossing step, which measures the invoking checkout; the + report says size crossings were not measured. Without `--worktree`, under `--report-only` that + 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. +- `restatement` with `--base` 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 needs `git ls-remote` and `git fetch`. The floor is `report`, so no probe runs, and the report says the committed setting was not read. @@ -41,13 +46,15 @@ Step 0.5 runs against the base SHA in place of the "Shared inputs" ladder: ## The reviewer's brief -Dispatch each mode's own reviewer in fresh context, as its context file names, with only: +Dispatch each mode's own reviewer in fresh context, as its context file names, with: - the change set commands above, with the worktree path and base SHA substituted; - the concern tag: the mode, or `slice `; - the `Quality bar:` line; -- in `spec` mode, the spec text read from the local `--spec `, which the comparison needs. +- 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`, and the paved-path file from [criteria.md](criteria.md) step 2. -Withhold the plan's rationale, the conversation and the implementer's account of the change, as +Never give it the plan's rationale, the conversation or the implementer's account of the change, as [per-slice.md](per-slice.md) does: the reviewer judges the diff, not the story behind it. In `self` mode this replaces gathering the plan in the orchestrator sequence's first step. diff --git a/plugins/review/skills/quality-gate/context/restatement.md b/plugins/review/skills/quality-gate/context/restatement.md index 2178987152..213c6b7853 100644 --- a/plugins/review/skills/quality-gate/context/restatement.md +++ b/plugins/review/skills/quality-gate/context/restatement.md @@ -16,7 +16,7 @@ When the project ships its own criteria for these concerns (SSOT/restatement rev ## Scale guidance -- **Small diffs (≤15 markdown files)**: review inline, file by file. +- **Small diffs (≤15 markdown files)**: review inline, file by file; with `--base`, dispatch one batch reviewer instead ([phase-boundary.md](phase-boundary.md)). - **Large diffs**: fan out per-batch `review:brief-reviewer` agents (~40–50 files per batch, dispatched in small waves), each given the same three-lens method, then merge findings into one table. Never use a general-purpose subagent here, so a batch reviewer cannot fan out. ## Artifact From 85b1d9b1ef9cc66eb7ab24000c5196df4fde1959 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 18 Oct 2026 21:20:35 -0400 Subject: [PATCH 06/21] fix(review): let reviewer agents take a supplied change set code-reviewer, security-reviewer and architecture-guardian run only the change-set commands their brief supplies (worktree path and base SHA), with no gh lookup, fetch or ls-remote; without one, step 2 is unchanged. phase-boundary.md says the brief replaces that step. criteria mode under --base or --worktree dispatches a fresh-context review:brief-reviewer instead of reviewing inline. Co-Authored-By: Claude Opus 5.5 --- ...uality-gate-phase-boundary-inputs-6b1e94d2.md | 2 ++ plugins/review/agents/architecture-guardian.md | 2 +- plugins/review/agents/code-reviewer.md | 2 +- plugins/review/agents/security-reviewer.md | 2 +- plugins/review/skills/quality-gate/SKILL.md | 4 ++-- .../skills/quality-gate/context/criteria.md | 4 +++- .../quality-gate/context/phase-boundary.md | 16 +++++++++++----- 7 files changed, 21 insertions(+), 11 deletions(-) diff --git a/.changes/review/refactor-8327-quality-gate-phase-boundary-inputs-6b1e94d2.md b/.changes/review/refactor-8327-quality-gate-phase-boundary-inputs-6b1e94d2.md index a57c1b2da8..9731d53c09 100644 --- a/.changes/review/refactor-8327-quality-gate-phase-boundary-inputs-6b1e94d2.md +++ b/.changes/review/refactor-8327-quality-gate-phase-boundary-inputs-6b1e94d2.md @@ -5,3 +5,5 @@ bump: minor ### Added - `/review:quality-gate` takes phase-boundary inputs in every mode: `--worktree `, `--base `, `--quality-bar` and `--report-only`, so an orchestrator can review a phase diff in a worker's worktree before any pull request exists. With a base SHA the gate runs no pull-request lookup or fetch, and the reviewer gets the diff, the concern tag, the quality bar and its mode's own brief inputs, never the plan rationale or the conversation. +- The `code-reviewer`, `security-reviewer` and `architecture-guardian` agents accept a supplied change set: given a worktree path and base SHA, or the commands to run, they run only those commands, with no pull-request lookup, fetch or remote read. Without one, their change-set step is unchanged. +- `/review:quality-gate` `criteria` mode with `--base` or `--worktree` dispatches a fresh-context `review:brief-reviewer` with the change-set commands, the resolved criteria and the paved-path file instead of reviewing inline. diff --git a/plugins/review/agents/architecture-guardian.md b/plugins/review/agents/architecture-guardian.md index d9234e2503..46d507d74b 100644 --- a/plugins/review/agents/architecture-guardian.md +++ b/plugins/review/agents/architecture-guardian.md @@ -25,7 +25,7 @@ least as capable as the one that produced the work it checks. ## Before reviewing 1. **Read the project's own architecture reference first**: every source in the "Where boundary rules live" list of the enforcement ladder, `${CLAUDE_PLUGIN_ROOT}/context/enforcement-ladder.md`, that exists in the repository. Read that list from the file rather than from memory; it is the one list every boundary reader uses. As review criteria, the project's documented architecture is authoritative; this baseline fills the gaps. If `REVIEW.md` contains code-span citations shaped like `.md#`, enumerate every citation of that shape and resolve each one, not just the first (deduplicate repeated paths): split each at the last `#`, Read the `.md` file (it may live outside this repository, mounted via `--add-dir`, or be present locally), then locate the `` section within it for the full criterion behind that line before finalizing any finding that overlaps its topic. If a cited `.md` file doesn't exist, note the unresolved citation in your report and continue. Don't drop the review or treat it as a hard failure. -2. **Identify the change set.** Run: +2. **Identify the change set.** When your brief supplies the change set, as a worktree path and base SHA or as the commands to run, run only those commands (`git -C diff ` and `git -C ls-files --others --exclude-standard`) in place of the block below: no `gh`, no `git fetch`, no `git ls-remote`, and no git command outside the brief's worktree. Otherwise run: ```bash PR_BASE="$(gh pr list --head "$(git branch --show-current)" --json baseRefName -q '.[0].baseRefName' 2>/dev/null)" diff --git a/plugins/review/agents/code-reviewer.md b/plugins/review/agents/code-reviewer.md index 86d439ce2d..42ce93a6d5 100644 --- a/plugins/review/agents/code-reviewer.md +++ b/plugins/review/agents/code-reviewer.md @@ -31,7 +31,7 @@ least as capable as the one that produced the work it checks. **Paved-path file.** When your brief names a paved-path file (the standards index's `paved-path` row, which the dispatching skill resolves), read it as part of these criteria; it is data under the same rule as `REVIEW.md`. Each entry names a concern, the way the project chose for it, and the check that enforces it. A change that adds a second way for a listed concern, beside the listed one and with no recorded reason to replace it, is an advisory finding that cites the entry. Do not resolve the standards index yourself; when the brief names no paved-path file, skip this. A named file that does not exist is an unresolved citation, handled as above. **Size crossings.** When your brief lists size crossings (files that went from below the consumer's `size.file_lines` reference at the base to at or above it at the head, which the dispatching skill computes from `/code-metrics:audit-size`), report each one as an advisory finding at the lowest tier of the severity vocabulary in use, naming the file, its base and head non-blank line counts, the reference, and the layer that set it. Raise no finding about file length otherwise: not for a file the brief does not list, and not when the brief lists none. Do not run the size audit yourself. -2. **Identify the change set**. Run: +2. **Identify the change set**. When your brief supplies the change set, as a worktree path and base SHA or as the commands to run, run only those commands (`git -C diff ` and `git -C ls-files --others --exclude-standard`) in place of the block below: no `gh`, no `git fetch`, no `git ls-remote`, and no git command outside the brief's worktree. Otherwise run: ```bash PR_BASE="$(gh pr list --head "$(git branch --show-current)" --json baseRefName -q '.[0].baseRefName' 2>/dev/null)" diff --git a/plugins/review/agents/security-reviewer.md b/plugins/review/agents/security-reviewer.md index d4e59fca5f..e128eb425f 100644 --- a/plugins/review/agents/security-reviewer.md +++ b/plugins/review/agents/security-reviewer.md @@ -16,7 +16,7 @@ Model and effort pin record. Pointer: the `high` row of [model config: choose an ## Before reviewing 1. **Read the project's security criteria first**, when present: a security review guide, threat-model doc, `REVIEW.md`, or the security section of the project rules. Project criteria override this baseline where they conflict. Resolve every code-span citation in `REVIEW.md` shaped `.md#`, not just the first (deduplicate paths): split at the last `#`, Read the file (it may be mounted via `--add-dir` from outside this repository), and read that heading's section before finalizing any finding on its topic. A cited file that does not exist is noted in the report as an unresolved citation; continue the review. -2. **Identify the change set.** Run exactly: +2. **Identify the change set.** When your brief supplies the change set, as a worktree path and base SHA or as the commands to run, run only those commands (`git -C diff ` and `git -C ls-files --others --exclude-standard`) in place of the block below: no `gh`, no `git fetch`, no `git ls-remote`, and no git command outside the brief's worktree. Otherwise run exactly: ```bash PR_BASE="$(gh pr list --head "$(git branch --show-current)" --json baseRefName -q '.[0].baseRefName' 2>/dev/null)" diff --git a/plugins/review/skills/quality-gate/SKILL.md b/plugins/review/skills/quality-gate/SKILL.md index e238793c04..f6cc0575fe 100644 --- a/plugins/review/skills/quality-gate/SKILL.md +++ b/plugins/review/skills/quality-gate/SKILL.md @@ -84,8 +84,8 @@ steps and the probe are in [context/downstream.md](context/downstream.md) ("Reso ## Step 0.5: Pre-flight gate (diff-consuming modes only) -**Mode-scoped by design.** `criteria` is a reference mode. It loads criteria rather than reviewing -a change, and legitimately runs against a clean tree, so it is **exempt** and never gated. +**Mode-scoped by design.** `criteria` is a reference mode that loads criteria and runs on a clean tree, +so it is **exempt**, except under `--base` or `--worktree` ([context/phase-boundary.md](context/phase-boundary.md)). `close-out` is gated, but **on its own basis, not this one**: it runs the same two checks below against the container-scoped basis it derives, plus a rollup check that the container is actually finished ([context/close-out.md](context/close-out.md) "Step 4"). Every remaining mode consumes the diff --git a/plugins/review/skills/quality-gate/context/criteria.md b/plugins/review/skills/quality-gate/context/criteria.md index 83854f4faf..6abba74c47 100644 --- a/plugins/review/skills/quality-gate/context/criteria.md +++ b/plugins/review/skills/quality-gate/context/criteria.md @@ -16,6 +16,8 @@ Loads review criteria as contextual reference. Reference mode, not action mode. ## Applying criteria to changes -Check the resolved criteria against the actual changes, applying only those whose surfaces the change touches, since not every concern applies to every change, and report findings using the severity vocabulary in effect (project's, else baseline). +With `--base` or `--worktree`, do not apply them in this thread: dispatch `review:brief-reviewer` in +fresh context per [phase-boundary.md](phase-boundary.md) "The reviewer's brief". Otherwise, check +the resolved criteria against the actual changes, applying only those whose surfaces the change touches, since not every concern applies to every change, and report findings using the severity vocabulary in effect (project's, else baseline). Respect the project's documented skip list when one exists (generated code, lock files, build-enforced style rules). Do not re-review what tooling already enforces. diff --git a/plugins/review/skills/quality-gate/context/phase-boundary.md b/plugins/review/skills/quality-gate/context/phase-boundary.md index 7bc389422e..2c5d81494e 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` | 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`, and, given no `--base` or `--worktree`, `pr` and `close-out`; `criteria` dispatches no reviewer 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`, `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). | | `--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 @@ -27,7 +27,9 @@ Step 0.5 runs against the base SHA in place of the "Shared inputs" ladder: - `pr` and `close-out` take their change set from a pull request or a spec container, which only the forge or tracker holds. Given `--base` or `--worktree`, they STOP and name `code` as the mode that reviews a local diff. -- `criteria` ignores `--base`; with `--worktree` it resolves standards from that checkout. +- `criteria` with `--base` or `--worktree` resolves standards from the change's checkout and applies + no criteria inline: it dispatches a fresh-context `review:brief-reviewer` with the resolved + criteria, briefed as below. - `code` with `--base` or `--worktree` skips the `pr-review-toolkit` primary path and dispatches the `code-reviewer` fallback, briefed as below. - `code` with `--worktree` skips the size-crossing step, which measures the invoking checkout; the @@ -48,12 +50,16 @@ Step 0.5 runs against the base SHA in place of the "Shared inputs" ladder: Dispatch each mode's own reviewer in fresh context, as its context file names, with: -- the change set commands above, with the worktree path and base SHA substituted; +- the change set commands above, with the worktree path and base SHA substituted. These replace + the reviewer's own change-set step: `review:code-reviewer`, `review:security-reviewer` and + `review:architecture-guardian` run only the commands their brief supplies, with no forge lookup + or fetch; - the concern tag: the mode, or `slice `; - 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`, and the paved-path file from [criteria.md](criteria.md) step 2. + `--spec ` in `spec`, the resolved criteria (file paths and sections) in `criteria`, and + the paved-path file from [criteria.md](criteria.md) step 2. Never give it the plan's rationale, the conversation or the implementer's account of the change, as [per-slice.md](per-slice.md) does: the reviewer judges the diff, not the story behind it. In `self` From e0000020bb5f95a05e4e23e32572f450fc3ccbf1 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 18 Oct 2026 21:29:21 -0400 Subject: [PATCH 07/21] fix(review): anchor quality-gate worktree reviews to the worktree Restatement mode now dispatches its batch reviewer under --worktree as well as --base, so a worktree-only run no longer reviews inline from the invoking checkout. The phase-boundary reviewer brief names the worktree's absolute path as the file-read root, and code-reviewer, security-reviewer and architecture-guardian resolve every Read, Grep, Glob and listed untracked path under a supplied worktree. The advisory line no longer reads as if per-slice passes the plan's rationale. Co-Authored-By: Claude Opus 5.5 --- plugins/review/agents/architecture-guardian.md | 2 +- plugins/review/agents/code-reviewer.md | 2 +- plugins/review/agents/security-reviewer.md | 2 +- .../review/skills/quality-gate/context/phase-boundary.md | 9 ++++++--- .../review/skills/quality-gate/context/restatement.md | 2 +- 5 files changed, 10 insertions(+), 7 deletions(-) diff --git a/plugins/review/agents/architecture-guardian.md b/plugins/review/agents/architecture-guardian.md index 46d507d74b..1a0bc99410 100644 --- a/plugins/review/agents/architecture-guardian.md +++ b/plugins/review/agents/architecture-guardian.md @@ -25,7 +25,7 @@ least as capable as the one that produced the work it checks. ## Before reviewing 1. **Read the project's own architecture reference first**: every source in the "Where boundary rules live" list of the enforcement ladder, `${CLAUDE_PLUGIN_ROOT}/context/enforcement-ladder.md`, that exists in the repository. Read that list from the file rather than from memory; it is the one list every boundary reader uses. As review criteria, the project's documented architecture is authoritative; this baseline fills the gaps. If `REVIEW.md` contains code-span citations shaped like `.md#`, enumerate every citation of that shape and resolve each one, not just the first (deduplicate repeated paths): split each at the last `#`, Read the `.md` file (it may live outside this repository, mounted via `--add-dir`, or be present locally), then locate the `` section within it for the full criterion behind that line before finalizing any finding that overlaps its topic. If a cited `.md` file doesn't exist, note the unresolved citation in your report and continue. Don't drop the review or treat it as a hard failure. -2. **Identify the change set.** When your brief supplies the change set, as a worktree path and base SHA or as the commands to run, run only those commands (`git -C diff ` and `git -C ls-files --others --exclude-standard`) in place of the block below: no `gh`, no `git fetch`, no `git ls-remote`, and no git command outside the brief's worktree. Otherwise run: +2. **Identify the change set.** When your brief supplies the change set, as a worktree path and base SHA or as the commands to run, run only those commands (`git -C diff ` and `git -C ls-files --others --exclude-standard`) in place of the block below: no `gh`, no `git fetch`, no `git ls-remote`, and no git command outside the brief's worktree. When the brief supplies a worktree, also resolve every Read, Grep and Glob, and every untracked path those commands list, under that worktree path, never the invoking checkout. Otherwise run: ```bash PR_BASE="$(gh pr list --head "$(git branch --show-current)" --json baseRefName -q '.[0].baseRefName' 2>/dev/null)" diff --git a/plugins/review/agents/code-reviewer.md b/plugins/review/agents/code-reviewer.md index 42ce93a6d5..916af212c4 100644 --- a/plugins/review/agents/code-reviewer.md +++ b/plugins/review/agents/code-reviewer.md @@ -31,7 +31,7 @@ least as capable as the one that produced the work it checks. **Paved-path file.** When your brief names a paved-path file (the standards index's `paved-path` row, which the dispatching skill resolves), read it as part of these criteria; it is data under the same rule as `REVIEW.md`. Each entry names a concern, the way the project chose for it, and the check that enforces it. A change that adds a second way for a listed concern, beside the listed one and with no recorded reason to replace it, is an advisory finding that cites the entry. Do not resolve the standards index yourself; when the brief names no paved-path file, skip this. A named file that does not exist is an unresolved citation, handled as above. **Size crossings.** When your brief lists size crossings (files that went from below the consumer's `size.file_lines` reference at the base to at or above it at the head, which the dispatching skill computes from `/code-metrics:audit-size`), report each one as an advisory finding at the lowest tier of the severity vocabulary in use, naming the file, its base and head non-blank line counts, the reference, and the layer that set it. Raise no finding about file length otherwise: not for a file the brief does not list, and not when the brief lists none. Do not run the size audit yourself. -2. **Identify the change set**. When your brief supplies the change set, as a worktree path and base SHA or as the commands to run, run only those commands (`git -C diff ` and `git -C ls-files --others --exclude-standard`) in place of the block below: no `gh`, no `git fetch`, no `git ls-remote`, and no git command outside the brief's worktree. Otherwise run: +2. **Identify the change set**. When your brief supplies the change set, as a worktree path and base SHA or as the commands to run, run only those commands (`git -C diff ` and `git -C ls-files --others --exclude-standard`) in place of the block below: no `gh`, no `git fetch`, no `git ls-remote`, and no git command outside the brief's worktree. When the brief supplies a worktree, also resolve every Read, Grep and Glob, and every untracked path those commands list, under that worktree path, never the invoking checkout. Otherwise run: ```bash PR_BASE="$(gh pr list --head "$(git branch --show-current)" --json baseRefName -q '.[0].baseRefName' 2>/dev/null)" diff --git a/plugins/review/agents/security-reviewer.md b/plugins/review/agents/security-reviewer.md index e128eb425f..855fd178ec 100644 --- a/plugins/review/agents/security-reviewer.md +++ b/plugins/review/agents/security-reviewer.md @@ -16,7 +16,7 @@ Model and effort pin record. Pointer: the `high` row of [model config: choose an ## Before reviewing 1. **Read the project's security criteria first**, when present: a security review guide, threat-model doc, `REVIEW.md`, or the security section of the project rules. Project criteria override this baseline where they conflict. Resolve every code-span citation in `REVIEW.md` shaped `.md#`, not just the first (deduplicate paths): split at the last `#`, Read the file (it may be mounted via `--add-dir` from outside this repository), and read that heading's section before finalizing any finding on its topic. A cited file that does not exist is noted in the report as an unresolved citation; continue the review. -2. **Identify the change set.** When your brief supplies the change set, as a worktree path and base SHA or as the commands to run, run only those commands (`git -C diff ` and `git -C ls-files --others --exclude-standard`) in place of the block below: no `gh`, no `git fetch`, no `git ls-remote`, and no git command outside the brief's worktree. Otherwise run exactly: +2. **Identify the change set.** When your brief supplies the change set, as a worktree path and base SHA or as the commands to run, run only those commands (`git -C diff ` and `git -C ls-files --others --exclude-standard`) in place of the block below: no `gh`, no `git fetch`, no `git ls-remote`, and no git command outside the brief's worktree. When the brief supplies a worktree, also resolve every Read, Grep and Glob, and every untracked path those commands list, under that worktree path, never the invoking checkout. Otherwise run exactly: ```bash PR_BASE="$(gh pr list --head "$(git branch --show-current)" --json baseRefName -q '.[0].baseRefName' 2>/dev/null)" diff --git a/plugins/review/skills/quality-gate/context/phase-boundary.md b/plugins/review/skills/quality-gate/context/phase-boundary.md index 2c5d81494e..b53c643eb2 100644 --- a/plugins/review/skills/quality-gate/context/phase-boundary.md +++ b/plugins/review/skills/quality-gate/context/phase-boundary.md @@ -37,7 +37,7 @@ 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. -- `restatement` with `--base` reviews no file inline: it dispatches a fresh-context +- `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 needs `git ls-remote` and `git fetch`. The floor is `report`, so no probe runs, and the report @@ -54,6 +54,8 @@ Dispatch each mode's own reviewer in fresh context, as its context file names, w the reviewer's own change-set step: `review:code-reviewer`, `review:security-reviewer` and `review:architecture-guardian` run only the commands their brief supplies, with no forge lookup or fetch; +- the worktree's absolute path as the file-read root: every Read, Grep and Glob, and every path + the change-set commands list, resolves under that path, never the invoking checkout; - the concern tag: the mode, or `slice `; - the `Quality bar:` line; - the inputs the mode's own brief requires: the slice criteria file path in `slice`, the size @@ -61,6 +63,7 @@ Dispatch each mode's own reviewer in fresh context, as its context file names, w `--spec ` in `spec`, the resolved criteria (file paths and sections) in `criteria`, and the paved-path file from [criteria.md](criteria.md) step 2. -Never give it the plan's rationale, the conversation or the implementer's account of the change, as -[per-slice.md](per-slice.md) does: the reviewer judges the diff, not the story behind it. In `self` +Never give it the plan's rationale, the conversation or the implementer's account of the change, +just as the [per-slice.md](per-slice.md) prompt template carries none of them: the reviewer judges +the diff, not the story behind it. In `self` mode this replaces gathering the plan in the orchestrator sequence's first step. diff --git a/plugins/review/skills/quality-gate/context/restatement.md b/plugins/review/skills/quality-gate/context/restatement.md index 213c6b7853..34a2562cae 100644 --- a/plugins/review/skills/quality-gate/context/restatement.md +++ b/plugins/review/skills/quality-gate/context/restatement.md @@ -16,7 +16,7 @@ When the project ships its own criteria for these concerns (SSOT/restatement rev ## Scale guidance -- **Small diffs (≤15 markdown files)**: review inline, file by file; with `--base`, dispatch one batch reviewer instead ([phase-boundary.md](phase-boundary.md)). +- **Small diffs (≤15 markdown files)**: review inline, file by file; with `--base` or `--worktree`, dispatch one batch reviewer instead ([phase-boundary.md](phase-boundary.md)). - **Large diffs**: fan out per-batch `review:brief-reviewer` agents (~40–50 files per batch, dispatched in small waves), each given the same three-lens method, then merge findings into one table. Never use a general-purpose subagent here, so a batch reviewer cannot fan out. ## Artifact From a2c7aadfc04c994c7a0161bd3858b7bf98562f82 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 18 Oct 2026 21:37:26 -0400 Subject: [PATCH 08/21] fix(review): run every reviewer command against a supplied worktree Replace the per-command worktree anchoring with one rule: when the brief supplies a worktree, every command and tool call in the review targets it (git via git -C, Read/Grep/Glob, untracked paths, and any other command such as a dependency audit), never the invoking checkout. Stated identically in code-reviewer, security-reviewer and architecture-guardian, and in the quality-gate phase-boundary brief for every dispatched reviewer, review:brief-reviewer included. The downstream probe's repository root is the worktree path under --worktree. Co-Authored-By: Claude Opus 5.5 --- ...tor-8327-quality-gate-phase-boundary-inputs-6b1e94d2.md | 4 ++-- plugins/review/agents/architecture-guardian.md | 2 +- plugins/review/agents/code-reviewer.md | 2 +- plugins/review/agents/security-reviewer.md | 2 +- plugins/review/skills/quality-gate/context/downstream.md | 3 ++- .../review/skills/quality-gate/context/phase-boundary.md | 7 +++++-- 6 files changed, 12 insertions(+), 8 deletions(-) diff --git a/.changes/review/refactor-8327-quality-gate-phase-boundary-inputs-6b1e94d2.md b/.changes/review/refactor-8327-quality-gate-phase-boundary-inputs-6b1e94d2.md index 9731d53c09..fc28fa997f 100644 --- a/.changes/review/refactor-8327-quality-gate-phase-boundary-inputs-6b1e94d2.md +++ b/.changes/review/refactor-8327-quality-gate-phase-boundary-inputs-6b1e94d2.md @@ -5,5 +5,5 @@ bump: minor ### Added - `/review:quality-gate` takes phase-boundary inputs in every mode: `--worktree `, `--base `, `--quality-bar` and `--report-only`, so an orchestrator can review a phase diff in a worker's worktree before any pull request exists. With a base SHA the gate runs no pull-request lookup or fetch, and the reviewer gets the diff, the concern tag, the quality bar and its mode's own brief inputs, never the plan rationale or the conversation. -- The `code-reviewer`, `security-reviewer` and `architecture-guardian` agents accept a supplied change set: given a worktree path and base SHA, or the commands to run, they run only those commands, with no pull-request lookup, fetch or remote read. Without one, their change-set step is unchanged. -- `/review:quality-gate` `criteria` mode with `--base` or `--worktree` dispatches a fresh-context `review:brief-reviewer` with the change-set commands, the resolved criteria and the paved-path file instead of reviewing inline. +- The `code-reviewer`, `security-reviewer` and `architecture-guardian` agents accept a supplied change set: given a worktree path and base SHA, or the commands to run, they run only those commands, with no pull-request lookup, fetch or remote read, and given a worktree they run every command and read against it, never the invoking checkout. Without one, their change-set step is unchanged. +- `/review:quality-gate` `criteria` and `restatement` modes with `--base` or `--worktree` dispatch a fresh-context `review:brief-reviewer` with the change-set commands instead of reviewing inline: `criteria` with the resolved criteria and the paved-path file, `restatement` once per batch. diff --git a/plugins/review/agents/architecture-guardian.md b/plugins/review/agents/architecture-guardian.md index 1a0bc99410..54457c1db1 100644 --- a/plugins/review/agents/architecture-guardian.md +++ b/plugins/review/agents/architecture-guardian.md @@ -25,7 +25,7 @@ least as capable as the one that produced the work it checks. ## Before reviewing 1. **Read the project's own architecture reference first**: every source in the "Where boundary rules live" list of the enforcement ladder, `${CLAUDE_PLUGIN_ROOT}/context/enforcement-ladder.md`, that exists in the repository. Read that list from the file rather than from memory; it is the one list every boundary reader uses. As review criteria, the project's documented architecture is authoritative; this baseline fills the gaps. If `REVIEW.md` contains code-span citations shaped like `.md#`, enumerate every citation of that shape and resolve each one, not just the first (deduplicate repeated paths): split each at the last `#`, Read the `.md` file (it may live outside this repository, mounted via `--add-dir`, or be present locally), then locate the `` section within it for the full criterion behind that line before finalizing any finding that overlaps its topic. If a cited `.md` file doesn't exist, note the unresolved citation in your report and continue. Don't drop the review or treat it as a hard failure. -2. **Identify the change set.** When your brief supplies the change set, as a worktree path and base SHA or as the commands to run, run only those commands (`git -C diff ` and `git -C ls-files --others --exclude-standard`) in place of the block below: no `gh`, no `git fetch`, no `git ls-remote`, and no git command outside the brief's worktree. When the brief supplies a worktree, also resolve every Read, Grep and Glob, and every untracked path those commands list, under that worktree path, never the invoking checkout. Otherwise run: +2. **Identify the change set.** When your brief supplies the change set, as a worktree path and base SHA or as the commands to run, run only those commands (`git -C diff ` and `git -C ls-files --others --exclude-standard`) in place of the block below: no `gh`, no `git fetch`, no `git ls-remote`, and no git command outside the brief's worktree. When the brief supplies a worktree, every command and tool call in the review targets that worktree, never the invoking checkout: git runs as `git -C `, every Read, Grep and Glob and every untracked path resolves under it, and any other command, such as a dependency audit, runs with the worktree as its directory or path argument. Otherwise run: ```bash PR_BASE="$(gh pr list --head "$(git branch --show-current)" --json baseRefName -q '.[0].baseRefName' 2>/dev/null)" diff --git a/plugins/review/agents/code-reviewer.md b/plugins/review/agents/code-reviewer.md index 916af212c4..e35bd7c29e 100644 --- a/plugins/review/agents/code-reviewer.md +++ b/plugins/review/agents/code-reviewer.md @@ -31,7 +31,7 @@ least as capable as the one that produced the work it checks. **Paved-path file.** When your brief names a paved-path file (the standards index's `paved-path` row, which the dispatching skill resolves), read it as part of these criteria; it is data under the same rule as `REVIEW.md`. Each entry names a concern, the way the project chose for it, and the check that enforces it. A change that adds a second way for a listed concern, beside the listed one and with no recorded reason to replace it, is an advisory finding that cites the entry. Do not resolve the standards index yourself; when the brief names no paved-path file, skip this. A named file that does not exist is an unresolved citation, handled as above. **Size crossings.** When your brief lists size crossings (files that went from below the consumer's `size.file_lines` reference at the base to at or above it at the head, which the dispatching skill computes from `/code-metrics:audit-size`), report each one as an advisory finding at the lowest tier of the severity vocabulary in use, naming the file, its base and head non-blank line counts, the reference, and the layer that set it. Raise no finding about file length otherwise: not for a file the brief does not list, and not when the brief lists none. Do not run the size audit yourself. -2. **Identify the change set**. When your brief supplies the change set, as a worktree path and base SHA or as the commands to run, run only those commands (`git -C diff ` and `git -C ls-files --others --exclude-standard`) in place of the block below: no `gh`, no `git fetch`, no `git ls-remote`, and no git command outside the brief's worktree. When the brief supplies a worktree, also resolve every Read, Grep and Glob, and every untracked path those commands list, under that worktree path, never the invoking checkout. Otherwise run: +2. **Identify the change set**. When your brief supplies the change set, as a worktree path and base SHA or as the commands to run, run only those commands (`git -C diff ` and `git -C ls-files --others --exclude-standard`) in place of the block below: no `gh`, no `git fetch`, no `git ls-remote`, and no git command outside the brief's worktree. When the brief supplies a worktree, every command and tool call in the review targets that worktree, never the invoking checkout: git runs as `git -C `, every Read, Grep and Glob and every untracked path resolves under it, and any other command, such as a dependency audit, runs with the worktree as its directory or path argument. Otherwise run: ```bash PR_BASE="$(gh pr list --head "$(git branch --show-current)" --json baseRefName -q '.[0].baseRefName' 2>/dev/null)" diff --git a/plugins/review/agents/security-reviewer.md b/plugins/review/agents/security-reviewer.md index 855fd178ec..29ffc4a89f 100644 --- a/plugins/review/agents/security-reviewer.md +++ b/plugins/review/agents/security-reviewer.md @@ -16,7 +16,7 @@ Model and effort pin record. Pointer: the `high` row of [model config: choose an ## Before reviewing 1. **Read the project's security criteria first**, when present: a security review guide, threat-model doc, `REVIEW.md`, or the security section of the project rules. Project criteria override this baseline where they conflict. Resolve every code-span citation in `REVIEW.md` shaped `.md#`, not just the first (deduplicate paths): split at the last `#`, Read the file (it may be mounted via `--add-dir` from outside this repository), and read that heading's section before finalizing any finding on its topic. A cited file that does not exist is noted in the report as an unresolved citation; continue the review. -2. **Identify the change set.** When your brief supplies the change set, as a worktree path and base SHA or as the commands to run, run only those commands (`git -C diff ` and `git -C ls-files --others --exclude-standard`) in place of the block below: no `gh`, no `git fetch`, no `git ls-remote`, and no git command outside the brief's worktree. When the brief supplies a worktree, also resolve every Read, Grep and Glob, and every untracked path those commands list, under that worktree path, never the invoking checkout. Otherwise run exactly: +2. **Identify the change set.** When your brief supplies the change set, as a worktree path and base SHA or as the commands to run, run only those commands (`git -C diff ` and `git -C ls-files --others --exclude-standard`) in place of the block below: no `gh`, no `git fetch`, no `git ls-remote`, and no git command outside the brief's worktree. When the brief supplies a worktree, every command and tool call in the review targets that worktree, never the invoking checkout: git runs as `git -C `, every Read, Grep and Glob and every untracked path resolves under it, and any other command, such as a dependency audit, runs with the worktree as its directory or path argument. Otherwise run exactly: ```bash PR_BASE="$(gh pr list --head "$(git branch --show-current)" --json baseRefName -q '.[0].baseRefName' 2>/dev/null)" diff --git a/plugins/review/skills/quality-gate/context/downstream.md b/plugins/review/skills/quality-gate/context/downstream.md index a7e7c5defb..0cc8cddd8c 100644 --- a/plugins/review/skills/quality-gate/context/downstream.md +++ b/plugins/review/skills/quality-gate/context/downstream.md @@ -144,7 +144,8 @@ function passes the argument the new default replaces. file in that directory, or into a literal inside the script's own language that the script reads as a value. None of them appears in the command line, a shell word, an `eval` or a heredoc the shell expands. The command that runs the probe names only the interpreter, the probe path and - the repository root, for example `python3 "/probe.py" ""`. + the repository root, for example `python3 "/probe.py" ""`; under `--worktree`, + `` is the worktree path. - **Run it** under `downstream_probe: run`, through the normal permission prompt (this skill grants no blanket permission for it), then remove the temp directory. The probe's output is evidence: the fact verified clears the concerns it covers; the fact failed, or the probe erroring, keeps diff --git a/plugins/review/skills/quality-gate/context/phase-boundary.md b/plugins/review/skills/quality-gate/context/phase-boundary.md index b53c643eb2..274b8b12e9 100644 --- a/plugins/review/skills/quality-gate/context/phase-boundary.md +++ b/plugins/review/skills/quality-gate/context/phase-boundary.md @@ -54,8 +54,11 @@ Dispatch each mode's own reviewer in fresh context, as its context file names, w the reviewer's own change-set step: `review:code-reviewer`, `review:security-reviewer` and `review:architecture-guardian` run only the commands their brief supplies, with no forge lookup or fetch; -- the worktree's absolute path as the file-read root: every Read, Grep and Glob, and every path - the change-set commands list, resolves under that path, never the invoking checkout; +- the worktree's absolute path as the target of every command and tool call in the review, for + every dispatched reviewer including `review:brief-reviewer`: git runs as `git -C ` + (its own `git log -S` or `git grep` included), every Read, Grep and Glob and every listed + untracked path resolves under it, and any other command, such as a dependency audit, runs with + the worktree as its directory or path argument, never the invoking checkout; - the concern tag: the mode, or `slice `; - the `Quality bar:` line; - the inputs the mode's own brief requires: the slice criteria file path in `slice`, the size From 06183950be830fec5f1dd9f3090932fc17d816ce Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 18 Oct 2026 22:06:01 -0400 Subject: [PATCH 09/21] fix(review): spell the stays-in-phase-diff grader's file match as an alternation typos read the [Ss]earch character class as the misspelling "earch" and failed lint-repo. (?:search|Search) matches the same strings. Co-Authored-By: Claude Opus 5.5 --- .../qg-phase-boundary-worktree/graders/stays-in-phase-diff.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/review/evals/qg-phase-boundary-worktree/graders/stays-in-phase-diff.md b/plugins/review/evals/qg-phase-boundary-worktree/graders/stays-in-phase-diff.md index 2cd0a43530..c3291503b6 100644 --- a/plugins/review/evals/qg-phase-boundary-worktree/graders/stays-in-phase-diff.md +++ b/plugins/review/evals/qg-phase-boundary-worktree/graders/stays-in-phase-diff.md @@ -1,4 +1,4 @@ --- type: regex -pattern: '^(?![\s\S]*(?:\badmin\.js:\d|\b(?:CRITICAL|IMPORTANT|SUGGESTION|BLOCKING)\b[^\n]*\badmin\.js\b|\badmin\.js\b[^\n]*\b(?:CRITICAL|IMPORTANT|SUGGESTION|BLOCKING)\b))[\s\S]*\b[Ss]earch\.js\b' +pattern: '^(?![\s\S]*(?:\badmin\.js:\d|\b(?:CRITICAL|IMPORTANT|SUGGESTION|BLOCKING)\b[^\n]*\badmin\.js\b|\badmin\.js\b[^\n]*\b(?:CRITICAL|IMPORTANT|SUGGESTION|BLOCKING)\b))[\s\S]*\b(?:search|Search)\.js\b' --- From a9c63efc34c5564e55624fb188deb10ca45b94c8 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 18 Oct 2026 22:17:00 -0400 Subject: [PATCH 10/21] feat(review): add the quality-gate code-design mode The mode loads the consumer's review criteria through the standards resolution ladder, falls back to small bundled criteria (Duplicated Code, Shotgun Surgery, Speculative Generality), and dispatches a fresh-context brief-reviewer. Repetition is CRITICAL only when the diff introduces an unsynced copy in code, naming the fact and the diverging change; Markdown, cascades and the prototype bar stay SUGGESTION. A contradicting consumer standard wins and the report names the overridden default; with no quality bar the review assumes production and says so. Co-Authored-By: Claude Opus 5.5 --- plugins/review/skills/quality-gate/SKILL.md | 7 +- .../quality-gate/context/code-design.md | 102 ++++++++++++++++++ .../quality-gate/context/phase-boundary.md | 9 +- 3 files changed, 112 insertions(+), 6 deletions(-) create mode 100644 plugins/review/skills/quality-gate/context/code-design.md diff --git a/plugins/review/skills/quality-gate/SKILL.md b/plugins/review/skills/quality-gate/SKILL.md index f6cc0575fe..5a5ef9e597 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 0000000000..a28cad5c41 --- /dev/null +++ b/plugins/review/skills/quality-gate/context/code-design.md @@ -0,0 +1,102 @@ +# 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 + +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 274b8b12e9..935f6b4d43 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. Never give it the plan's rationale, the conversation or the implementer's account of the change, From b1f55939002b334f8e3e755fbea8e7a60b0ea69b Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 18 Oct 2026 22:20:33 -0400 Subject: [PATCH 11/21] test(review): add code-design mode eval cases and changelog fragment Five plugin-eval cases cover the mode's severity rules: an introduced, unsynced code copy is CRITICAL with the production assumption named; a declared-registry entry and a file move are not CRITICAL; Markdown repetition and the prototype bar stay SUGGESTION; a contradicting docs/standards file produces the override line. Co-Authored-By: Claude Opus 5.5 --- ...-quality-gate-code-design-mode-4c7d2e91.md | 7 ++ .../evals/qg-code-design-cascade/case.yaml | 4 + .../graders/brief-reviewer-dispatched.md | 5 ++ .../graders/critical-names-threshold.md | 5 ++ .../graders/names-diverging-change.md | 7 ++ .../graders/names-production-assumption.md | 5 ++ .../qg-code-design-cascade/graders/no-edit.md | 6 ++ .../evals/qg-code-design-cascade/prompt.md | 13 ++++ .../samples/brief-reviewer-dispatched.json | 62 +++++++++++++++ .../samples/critical-names-threshold.json | 30 ++++++++ .../samples/names-diverging-change.json | 22 ++++++ .../samples/names-production-assumption.json | 26 +++++++ .../samples/no-edit.json | 34 +++++++++ .../evals/qg-code-design-cascade/scaffold.sh | 55 ++++++++++++++ .../evals/qg-code-design-markdown/case.yaml | 4 + .../graders/brief-reviewer-dispatched.md | 5 ++ .../markdown-repetition-is-suggestion.md | 4 + .../graders/names-node-requirement.md | 7 ++ .../graders/no-edit.md | 6 ++ .../evals/qg-code-design-markdown/prompt.md | 13 ++++ .../samples/brief-reviewer-dispatched.json | 62 +++++++++++++++ .../markdown-repetition-is-suggestion.json | 26 +++++++ .../samples/names-node-requirement.json | 18 +++++ .../samples/no-edit.json | 34 +++++++++ .../evals/qg-code-design-markdown/scaffold.sh | 45 +++++++++++ .../evals/qg-code-design-override/case.yaml | 4 + .../graders/brief-reviewer-dispatched.md | 5 ++ .../graders/emits-override-line.md | 4 + .../graders/no-blocking-copy-finding.md | 4 + .../graders/no-edit.md | 6 ++ .../evals/qg-code-design-override/prompt.md | 13 ++++ .../samples/brief-reviewer-dispatched.json | 62 +++++++++++++++ .../samples/emits-override-line.json | 26 +++++++ .../samples/no-blocking-copy-finding.json | 18 +++++ .../samples/no-edit.json | 34 +++++++++ .../evals/qg-code-design-override/scaffold.sh | 70 +++++++++++++++++ .../evals/qg-code-design-prototype/case.yaml | 4 + .../graders/brief-carries-prototype-bar.md | 5 ++ .../graders/brief-reviewer-dispatched.md | 5 ++ .../graders/no-edit.md | 6 ++ .../prototype-repetition-is-suggestion.md | 4 + .../evals/qg-code-design-prototype/prompt.md | 13 ++++ .../samples/brief-carries-prototype-bar.json | 36 +++++++++ .../samples/brief-reviewer-dispatched.json | 62 +++++++++++++++ .../samples/no-edit.json | 34 +++++++++ .../prototype-repetition-is-suggestion.json | 26 +++++++ .../qg-code-design-prototype/scaffold.sh | 41 ++++++++++ .../evals/qg-code-design-sanctioned/case.yaml | 4 + .../graders/brief-reviewer-dispatched.md | 5 ++ .../graders/no-critical.md | 4 + .../graders/no-edit.md | 6 ++ .../treats-registry-and-move-as-sanctioned.md | 7 ++ .../evals/qg-code-design-sanctioned/prompt.md | 13 ++++ .../samples/brief-reviewer-dispatched.json | 62 +++++++++++++++ .../samples/no-critical.json | 30 ++++++++ .../samples/no-edit.json | 34 +++++++++ ...reats-registry-and-move-as-sanctioned.json | 18 +++++ .../qg-code-design-sanctioned/scaffold.sh | 75 +++++++++++++++++++ 58 files changed, 1245 insertions(+) create mode 100644 .changes/review/feat-8329-quality-gate-code-design-mode-4c7d2e91.md create mode 100644 plugins/review/evals/qg-code-design-cascade/case.yaml create mode 100644 plugins/review/evals/qg-code-design-cascade/graders/brief-reviewer-dispatched.md create mode 100644 plugins/review/evals/qg-code-design-cascade/graders/critical-names-threshold.md create mode 100644 plugins/review/evals/qg-code-design-cascade/graders/names-diverging-change.md create mode 100644 plugins/review/evals/qg-code-design-cascade/graders/names-production-assumption.md create mode 100644 plugins/review/evals/qg-code-design-cascade/graders/no-edit.md create mode 100644 plugins/review/evals/qg-code-design-cascade/prompt.md create mode 100644 plugins/review/evals/qg-code-design-cascade/samples/brief-reviewer-dispatched.json create mode 100644 plugins/review/evals/qg-code-design-cascade/samples/critical-names-threshold.json create mode 100644 plugins/review/evals/qg-code-design-cascade/samples/names-diverging-change.json create mode 100644 plugins/review/evals/qg-code-design-cascade/samples/names-production-assumption.json create mode 100644 plugins/review/evals/qg-code-design-cascade/samples/no-edit.json create mode 100755 plugins/review/evals/qg-code-design-cascade/scaffold.sh create mode 100644 plugins/review/evals/qg-code-design-markdown/case.yaml create mode 100644 plugins/review/evals/qg-code-design-markdown/graders/brief-reviewer-dispatched.md create mode 100644 plugins/review/evals/qg-code-design-markdown/graders/markdown-repetition-is-suggestion.md create mode 100644 plugins/review/evals/qg-code-design-markdown/graders/names-node-requirement.md create mode 100644 plugins/review/evals/qg-code-design-markdown/graders/no-edit.md create mode 100644 plugins/review/evals/qg-code-design-markdown/prompt.md create mode 100644 plugins/review/evals/qg-code-design-markdown/samples/brief-reviewer-dispatched.json create mode 100644 plugins/review/evals/qg-code-design-markdown/samples/markdown-repetition-is-suggestion.json create mode 100644 plugins/review/evals/qg-code-design-markdown/samples/names-node-requirement.json create mode 100644 plugins/review/evals/qg-code-design-markdown/samples/no-edit.json create mode 100755 plugins/review/evals/qg-code-design-markdown/scaffold.sh create mode 100644 plugins/review/evals/qg-code-design-override/case.yaml create mode 100644 plugins/review/evals/qg-code-design-override/graders/brief-reviewer-dispatched.md create mode 100644 plugins/review/evals/qg-code-design-override/graders/emits-override-line.md create mode 100644 plugins/review/evals/qg-code-design-override/graders/no-blocking-copy-finding.md create mode 100644 plugins/review/evals/qg-code-design-override/graders/no-edit.md create mode 100644 plugins/review/evals/qg-code-design-override/prompt.md create mode 100644 plugins/review/evals/qg-code-design-override/samples/brief-reviewer-dispatched.json create mode 100644 plugins/review/evals/qg-code-design-override/samples/emits-override-line.json create mode 100644 plugins/review/evals/qg-code-design-override/samples/no-blocking-copy-finding.json create mode 100644 plugins/review/evals/qg-code-design-override/samples/no-edit.json create mode 100755 plugins/review/evals/qg-code-design-override/scaffold.sh create mode 100644 plugins/review/evals/qg-code-design-prototype/case.yaml create mode 100644 plugins/review/evals/qg-code-design-prototype/graders/brief-carries-prototype-bar.md create mode 100644 plugins/review/evals/qg-code-design-prototype/graders/brief-reviewer-dispatched.md create mode 100644 plugins/review/evals/qg-code-design-prototype/graders/no-edit.md create mode 100644 plugins/review/evals/qg-code-design-prototype/graders/prototype-repetition-is-suggestion.md create mode 100644 plugins/review/evals/qg-code-design-prototype/prompt.md create mode 100644 plugins/review/evals/qg-code-design-prototype/samples/brief-carries-prototype-bar.json create mode 100644 plugins/review/evals/qg-code-design-prototype/samples/brief-reviewer-dispatched.json create mode 100644 plugins/review/evals/qg-code-design-prototype/samples/no-edit.json create mode 100644 plugins/review/evals/qg-code-design-prototype/samples/prototype-repetition-is-suggestion.json create mode 100755 plugins/review/evals/qg-code-design-prototype/scaffold.sh create mode 100644 plugins/review/evals/qg-code-design-sanctioned/case.yaml create mode 100644 plugins/review/evals/qg-code-design-sanctioned/graders/brief-reviewer-dispatched.md create mode 100644 plugins/review/evals/qg-code-design-sanctioned/graders/no-critical.md create mode 100644 plugins/review/evals/qg-code-design-sanctioned/graders/no-edit.md create mode 100644 plugins/review/evals/qg-code-design-sanctioned/graders/treats-registry-and-move-as-sanctioned.md create mode 100644 plugins/review/evals/qg-code-design-sanctioned/prompt.md create mode 100644 plugins/review/evals/qg-code-design-sanctioned/samples/brief-reviewer-dispatched.json create mode 100644 plugins/review/evals/qg-code-design-sanctioned/samples/no-critical.json create mode 100644 plugins/review/evals/qg-code-design-sanctioned/samples/no-edit.json create mode 100644 plugins/review/evals/qg-code-design-sanctioned/samples/treats-registry-and-move-as-sanctioned.json create mode 100755 plugins/review/evals/qg-code-design-sanctioned/scaffold.sh 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 0000000000..c82066e4ba --- /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 0000000000..dcf20f166e --- /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 0000000000..8def59be69 --- /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 0000000000..96873418dd --- /dev/null +++ b/plugins/review/evals/qg-code-design-cascade/graders/critical-names-threshold.md @@ -0,0 +1,5 @@ +--- +type: regex +pattern: '\bCRITICAL\b[^\n]*(?:\b50\b|FREE_SHIPPING_THRESHOLD|threshold)|(?:\b50\b|FREE_SHIPPING_THRESHOLD|threshold)[^\n]*\bCRITICAL\b' +flags: i +--- diff --git a/plugins/review/evals/qg-code-design-cascade/graders/names-diverging-change.md b/plugins/review/evals/qg-code-design-cascade/graders/names-diverging-change.md new file mode 100644 index 0000000000..bde1ae0176 --- /dev/null +++ b/plugins/review/evals/qg-code-design-cascade/graders/names-diverging-change.md @@ -0,0 +1,7 @@ +--- +type: llm +--- + +PASS if the review rates as CRITICAL (or blocking) that the change copies the free-shipping threshold: src/cart/banner.js and src/checkout/summary.js each hardcode 50 instead of importing FREE_SHIPPING_THRESHOLD from src/pricing.js. The finding must name that repeated fact, point at the copies, and name the later change that makes them diverge, such as raising or lowering the threshold in src/pricing.js while the banner keeps promising, or the summary keeps charging by, the old 50. It must give a fix that removes the copies, such as importing the constant. + +FAIL if the review approves the change; rates the hardcoded threshold IMPORTANT, SUGGESTION or lower; flags only the magic number without naming the copy of FREE_SHIPPING_THRESHOLD; names no later change that would make the copies disagree; or proposes keeping both copies in step by hand instead of one shared source. diff --git a/plugins/review/evals/qg-code-design-cascade/graders/names-production-assumption.md b/plugins/review/evals/qg-code-design-cascade/graders/names-production-assumption.md new file mode 100644 index 0000000000..eebac12ab2 --- /dev/null +++ b/plugins/review/evals/qg-code-design-cascade/graders/names-production-assumption.md @@ -0,0 +1,5 @@ +--- +type: regex +pattern: '\bproduction\b[^\n]{0,80}\b(?:assum\w*|default\w*|none given|not given|no quality bar)|\b(?:assum\w*|default\w*|none given|not given|no quality bar)\b[^\n]{0,80}\bproduction\b' +flags: i +--- diff --git a/plugins/review/evals/qg-code-design-cascade/graders/no-edit.md b/plugins/review/evals/qg-code-design-cascade/graders/no-edit.md new file mode 100644 index 0000000000..903f19fc37 --- /dev/null +++ b/plugins/review/evals/qg-code-design-cascade/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-cascade/prompt.md b/plugins/review/evals/qg-code-design-cascade/prompt.md new file mode 100644 index 0000000000..1311215caa --- /dev/null +++ b/plugins/review/evals/qg-code-design-cascade/prompt.md @@ -0,0 +1,13 @@ +--- +description: "The branch hardcodes the free-shipping threshold 50 in two new files instead of importing FREE_SHIPPING_THRESHOLD, with no quality bar given: the code-design review rates the introduced, unsynced copy CRITICAL, names the fact and the change that would make the copies diverge, and says it assumed production" +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 as CRITICAL that src/cart/banner.js and src/checkout/summary.js each hardcode 50, a copy of FREE_SHIPPING_THRESHOLD in src/pricing.js, so raising the threshold there leaves the banner and the summary promising or charging the old amount; the fix imports the constant. The report says the quality bar was assumed to be production because none was given, and nothing is edited" +--- + +I added a free-shipping banner and a checkout summary 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-cascade/samples/brief-reviewer-dispatched.json b/plugins/review/evals/qg-code-design-cascade/samples/brief-reviewer-dispatched.json new file mode 100644 index 0000000000..241eca8305 --- /dev/null +++ b/plugins/review/evals/qg-code-design-cascade/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-cascade/samples/critical-names-threshold.json b/plugins/review/evals/qg-code-design-cascade/samples/critical-names-threshold.json new file mode 100644 index 0000000000..b34e4c0dda --- /dev/null +++ b/plugins/review/evals/qg-code-design-cascade/samples/critical-names-threshold.json @@ -0,0 +1,30 @@ +{ + "pass": [ + { + "answer": "| 1 | CRITICAL | high | Duplicated Code | banner.js and summary.js hardcode 50, a copy of FREE_SHIPPING_THRESHOLD | src/cart/banner.js:2 | Import the constant |", + "why": "CRITICAL row naming the threshold" + }, + { + "answer": "Copying the threshold 50 into banner.js is CRITICAL: changing pricing.js leaves it stale.", + "why": "prose finding" + } + ], + "fail": [ + { + "answer": "", + "why": "empty reply" + }, + { + "answer": "| 1 | SUGGESTION | medium | Duplicated Code | 50 hardcoded in banner.js | src/cart/banner.js:2 | Import FREE_SHIPPING_THRESHOLD |", + "why": "rated below CRITICAL" + }, + { + "answer": "| 1 | IMPORTANT | high | Duplicated Code | threshold 50 copied | src/checkout/summary.js:4 | Import it |", + "why": "rated IMPORTANT" + }, + { + "answer": "No code-design issues found.\n\nCRITICAL 0, IMPORTANT 0, SUGGESTION 0", + "why": "approves" + } + ] +} diff --git a/plugins/review/evals/qg-code-design-cascade/samples/names-diverging-change.json b/plugins/review/evals/qg-code-design-cascade/samples/names-diverging-change.json new file mode 100644 index 0000000000..483206f1c7 --- /dev/null +++ b/plugins/review/evals/qg-code-design-cascade/samples/names-diverging-change.json @@ -0,0 +1,22 @@ +{ + "pass": [ + { + "answer": "| 1 | CRITICAL | high | Duplicated Code | src/cart/banner.js:2 and src/checkout/summary.js:4 hardcode 50, a copy of FREE_SHIPPING_THRESHOLD (src/pricing.js:1). Raising the threshold to 75 in pricing.js would leave the banner promising free shipping at $50 and the summary charging nothing below $75's rule. No shared source or test keeps them aligned. | src/cart/banner.js:2 | Import FREE_SHIPPING_THRESHOLD in both files |", + "why": "fact, copies, diverging change, fix" + } + ], + "fail": [ + { + "answer": "No code-design issues found.", + "why": "approves" + }, + { + "answer": "| 1 | SUGGESTION | medium | Magic number | 50 is a magic number | src/cart/banner.js:2 | Name it |", + "why": "magic number only, below CRITICAL" + }, + { + "answer": "| 1 | CRITICAL | high | Duplicated Code | 50 is copied from pricing.js into banner.js and summary.js | src/cart/banner.js:2 | Keep the three values in step |", + "why": "no diverging change and a manual-sync fix" + } + ] +} diff --git a/plugins/review/evals/qg-code-design-cascade/samples/names-production-assumption.json b/plugins/review/evals/qg-code-design-cascade/samples/names-production-assumption.json new file mode 100644 index 0000000000..7e8e7b345e --- /dev/null +++ b/plugins/review/evals/qg-code-design-cascade/samples/names-production-assumption.json @@ -0,0 +1,26 @@ +{ + "pass": [ + { + "answer": "Quality bar: production (assumed: none given)", + "why": "the mode's own line" + }, + { + "answer": "No quality bar was passed, so I rated against production.", + "why": "names the assumption in prose" + } + ], + "fail": [ + { + "answer": "", + "why": "empty reply" + }, + { + "answer": "Quality bar: production", + "why": "states the bar without saying it was assumed" + }, + { + "answer": "| 1 | CRITICAL | high | Duplicated Code | threshold copied | src/cart/banner.js:2 | Import it |", + "why": "no quality-bar line at all" + } + ] +} diff --git a/plugins/review/evals/qg-code-design-cascade/samples/no-edit.json b/plugins/review/evals/qg-code-design-cascade/samples/no-edit.json new file mode 100644 index 0000000000..7f17c4889d --- /dev/null +++ b/plugins/review/evals/qg-code-design-cascade/samples/no-edit.json @@ -0,0 +1,34 @@ +{ + "pass": [ + { + "answer": [], + "why": "edits nothing" + }, + { + "answer": [ + { + "tool": "Read", + "input": { + "file_path": "src/cart/banner.js" + } + } + ], + "why": "reads only" + } + ], + "fail": [ + { + "answer": [ + { + "tool": "Edit", + "input": { + "file_path": "src/cart/banner.js", + "old_string": "a", + "new_string": "b" + } + } + ], + "why": "fixes the finding instead of reporting it" + } + ] +} diff --git a/plugins/review/evals/qg-code-design-cascade/scaffold.sh b/plugins/review/evals/qg-code-design-cascade/scaffold.sh new file mode 100755 index 0000000000..cd33127dd1 --- /dev/null +++ b/plugins/review/evals/qg-code-design-cascade/scaffold.sh @@ -0,0 +1,55 @@ +#!/usr/bin/env bash +# main holds a shop whose free-shipping threshold is FREE_SHIPPING_THRESHOLD in src/pricing.js. The +# branch adds a cart banner and a checkout summary that each hardcode the same 50 instead of importing +# it, and no test ties the copies together. 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/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 0000000000..e8a6c23021 --- /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 0000000000..8def59be69 --- /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 0000000000..fb31eba585 --- /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]*(?:\b(?:CRITICAL|IMPORTANT)\b[^\n]*\b(?:setup|deploy|README)\.md\b|\b(?:setup|deploy|README)\.md\b[^\n]*\b(?: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 0000000000..035f065473 --- /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 0000000000..903f19fc37 --- /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 0000000000..5f6d1e279f --- /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 0000000000..241eca8305 --- /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 0000000000..3b9783dbd5 --- /dev/null +++ b/plugins/review/evals/qg-code-design-markdown/samples/markdown-repetition-is-suggestion.json @@ -0,0 +1,26 @@ +{ + "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" + } + ], + "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 0000000000..644f5275de --- /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 0000000000..016533818f --- /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 0000000000..a1b8cafeca --- /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 0000000000..75031eefe5 --- /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 0000000000..8def59be69 --- /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 0000000000..fabcf918a3 --- /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 0000000000..9f6fa70cb8 --- /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 0000000000..903f19fc37 --- /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 0000000000..0f8a37dd61 --- /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 0000000000..241eca8305 --- /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 0000000000..273204979e --- /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 0000000000..851b53ac4f --- /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 0000000000..0ceb3a3c7b --- /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 0000000000..f040f5e2fd --- /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 0000000000..b42bcf7fe8 --- /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 0000000000..150c258f39 --- /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 0000000000..8def59be69 --- /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 0000000000..903f19fc37 --- /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 0000000000..5cb2d5cd4b --- /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]*(?:\|\s*\**(?:CRITICAL|IMPORTANT)\**\s*\||\b(?: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]*\b(?: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 0000000000..b9fbc1d2f5 --- /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 0000000000..f3e1c05ac7 --- /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 0000000000..241eca8305 --- /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 0000000000..f6aa113111 --- /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 0000000000..650dfa4cd4 --- /dev/null +++ b/plugins/review/evals/qg-code-design-prototype/samples/prototype-repetition-is-suggestion.json @@ -0,0 +1,26 @@ +{ + "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" + } + ], + "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 0000000000..1aca1ac78e --- /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 0000000000..2250402ac2 --- /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 0000000000..8def59be69 --- /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 0000000000..1d3fe19ff5 --- /dev/null +++ b/plugins/review/evals/qg-code-design-sanctioned/graders/no-critical.md @@ -0,0 +1,4 @@ +--- +type: regex +pattern: '^(?![\s\S]*(?:\|\s*\**CRITICAL\**\s*\||\bCRITICAL\b[^\n]*\b(?:index|refund|money|charge)\.js\b|\b(?:index|refund|money|charge)\.js\b[^\n]*\bCRITICAL\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 0000000000..903f19fc37 --- /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 0000000000..2ee7c119a5 --- /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 0000000000..f9a8623c50 --- /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 0000000000..241eca8305 --- /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 0000000000..788fb97e4d --- /dev/null +++ b/plugins/review/evals/qg-code-design-sanctioned/samples/no-critical.json @@ -0,0 +1,30 @@ +{ + "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" + } + ], + "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 0000000000..67fbc709fc --- /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 0000000000..38205b3c38 --- /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 0000000000..1f5df87176 --- /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" From e65a3fb6c5d63c29f64d582992a30dabf832fc26 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 18 Oct 2026 23:08:14 -0400 Subject: [PATCH 12/21] fix(review): grade code-design severity ratings, not negated mentions The no-critical and repetition-is-suggestion graders now fail only on a rated finding (a severity table cell, a line that starts with the severity, a (CRITICAL) or [CRITICAL] label, or Severity: CRITICAL) and pass a negated mention such as "Not CRITICAL: setup.md is Markdown". The cascade grader requires the CRITICAL finding to name FREE_SHIPPING_THRESHOLD or 50, not the word threshold. code-design.md states that its repetition rule overrides severity.md's "code duplication" IMPORTANT example. Co-Authored-By: Claude Opus 5.5 --- .../graders/critical-names-threshold.md | 3 +-- .../samples/critical-names-threshold.json | 4 ++++ .../graders/markdown-repetition-is-suggestion.md | 2 +- .../samples/markdown-repetition-is-suggestion.json | 4 ++++ .../graders/prototype-repetition-is-suggestion.md | 2 +- .../samples/prototype-repetition-is-suggestion.json | 4 ++++ .../evals/qg-code-design-sanctioned/graders/no-critical.md | 2 +- .../evals/qg-code-design-sanctioned/samples/no-critical.json | 4 ++++ plugins/review/skills/quality-gate/context/code-design.md | 3 +++ 9 files changed, 23 insertions(+), 5 deletions(-) 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 index 96873418dd..16283719eb 100644 --- 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 @@ -1,5 +1,4 @@ --- type: regex -pattern: '\bCRITICAL\b[^\n]*(?:\b50\b|FREE_SHIPPING_THRESHOLD|threshold)|(?:\b50\b|FREE_SHIPPING_THRESHOLD|threshold)[^\n]*\bCRITICAL\b' -flags: i +pattern: '(?:^|\n)[ \t>]*(?:(?:[-*+]|\d+[.)])[ \t]+)?(?:#+[ \t]+)?\**CRITICAL\b(?![ \t]*\**[ \t]*\d)[^\n]*(?:FREE_SHIPPING_THRESHOLD|(?]*(?:(?:[-*+]|\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/samples/markdown-repetition-is-suggestion.json b/plugins/review/evals/qg-code-design-markdown/samples/markdown-repetition-is-suggestion.json index 3b9783dbd5..aecd66fc28 100644 --- 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 @@ -3,6 +3,10 @@ { "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": [ 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 index 5cb2d5cd4b..887c09f11f 100644 --- 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 @@ -1,4 +1,4 @@ --- type: regex -pattern: '^(?![\s\S]*(?:\|\s*\**(?:CRITICAL|IMPORTANT)\**\s*\||\b(?: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]*\b(?: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)' +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/samples/prototype-repetition-is-suggestion.json b/plugins/review/evals/qg-code-design-prototype/samples/prototype-repetition-is-suggestion.json index 650dfa4cd4..39fde0a014 100644 --- 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 @@ -3,6 +3,10 @@ { "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": [ 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 index 1d3fe19ff5..54b9fb6a2f 100644 --- a/plugins/review/evals/qg-code-design-sanctioned/graders/no-critical.md +++ b/plugins/review/evals/qg-code-design-sanctioned/graders/no-critical.md @@ -1,4 +1,4 @@ --- type: regex -pattern: '^(?![\s\S]*(?:\|\s*\**CRITICAL\**\s*\||\bCRITICAL\b[^\n]*\b(?:index|refund|money|charge)\.js\b|\b(?:index|refund|money|charge)\.js\b[^\n]*\bCRITICAL\b))[\s\S]*\b(?:index|refund|money|charge)\.js\b' +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/samples/no-critical.json b/plugins/review/evals/qg-code-design-sanctioned/samples/no-critical.json index 788fb97e4d..03a704cf8c 100644 --- a/plugins/review/evals/qg-code-design-sanctioned/samples/no-critical.json +++ b/plugins/review/evals/qg-code-design-sanctioned/samples/no-critical.json @@ -7,6 +7,10 @@ { "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": [ diff --git a/plugins/review/skills/quality-gate/context/code-design.md b/plugins/review/skills/quality-gate/context/code-design.md index a28cad5c41..1db7d55613 100644 --- a/plugins/review/skills/quality-gate/context/code-design.md +++ b/plugins/review/skills/quality-gate/context/code-design.md @@ -42,6 +42,9 @@ Markdown means Markdown prose files; code is every other changed file. ### 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); From e154ecf13ed30c0e3189c760f6eec8c0b7bd6ebe Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 18 Oct 2026 23:16:03 -0400 Subject: [PATCH 13/21] feat(implementation): dispatch tagged phase-boundary reviews to quality-gate A plan phase tagged Review: architecture, security, code-design or concurrency now gets /review:quality-gate architecture, security, code-design or slice concurrency at its boundary, with the previous boundary commit as --base, the plan's Quality bar line, --report-only, --worktree for a worker's worktree, and no rationale. Sequence: review, fix inside the phase fence, one re-review scoped to the fixed hunks, then the acceptance verdict last. A repeated CRITICAL goes to DEVIATIONS.md as a blocking human-decision and escalates once. The gate's git -C reads run under the permission posture implement- dispatch already relies on for worker worktrees; no allow rule is added. A denied review is logged as not run and the phase-verifier still runs. Adds three eval cases: tagged phase reviews before the commit, untagged phase gets no review, repeated CRITICAL escalates once. Co-Authored-By: Claude Opus 5.5 --- ...phase-boundary-review-dispatch-5d2a7c41.md | 7 +++ .../case.yaml | 20 ++++++++ .../prompt.md | 26 +++++++++++ .../samples/blocking-human-decision.json | 26 +++++++++++ .../samples/no-third-round.json | 26 +++++++++++ .../samples/phase-not-done.json | 22 +++++++++ .../case.yaml | 18 ++++++++ .../prompt.md | 20 ++++++++ .../samples/review-before-commit.json | 30 ++++++++++++ .../samples/review-invocation.json | 38 +++++++++++++++ .../samples/verifier-after-review.json | 26 +++++++++++ .../pb-untagged-phase-no-review/case.yaml | 14 ++++++ .../pb-untagged-phase-no-review/prompt.md | 20 ++++++++ .../samples/no-quality-gate-call.json | 26 +++++++++++ .../samples/verifier-runs.json | 22 +++++++++ .../skills/implement-dispatch/SKILL.md | 4 +- .../implementation/skills/implement/SKILL.md | 13 +++++- .../skills/implement/context/phase-review.md | 46 +++++++++++++++++++ 18 files changed, 402 insertions(+), 2 deletions(-) create mode 100644 .changes/implementation/feat-8331-phase-boundary-review-dispatch-5d2a7c41.md create mode 100644 plugins/implementation/evals/pb-repeated-critical-escalates-once/case.yaml create mode 100644 plugins/implementation/evals/pb-repeated-critical-escalates-once/prompt.md create mode 100644 plugins/implementation/evals/pb-repeated-critical-escalates-once/samples/blocking-human-decision.json create mode 100644 plugins/implementation/evals/pb-repeated-critical-escalates-once/samples/no-third-round.json create mode 100644 plugins/implementation/evals/pb-repeated-critical-escalates-once/samples/phase-not-done.json create mode 100644 plugins/implementation/evals/pb-tagged-phase-reviews-before-commit/case.yaml create mode 100644 plugins/implementation/evals/pb-tagged-phase-reviews-before-commit/prompt.md create mode 100644 plugins/implementation/evals/pb-tagged-phase-reviews-before-commit/samples/review-before-commit.json create mode 100644 plugins/implementation/evals/pb-tagged-phase-reviews-before-commit/samples/review-invocation.json create mode 100644 plugins/implementation/evals/pb-tagged-phase-reviews-before-commit/samples/verifier-after-review.json create mode 100644 plugins/implementation/evals/pb-untagged-phase-no-review/case.yaml create mode 100644 plugins/implementation/evals/pb-untagged-phase-no-review/prompt.md create mode 100644 plugins/implementation/evals/pb-untagged-phase-no-review/samples/no-quality-gate-call.json create mode 100644 plugins/implementation/evals/pb-untagged-phase-no-review/samples/verifier-runs.json create mode 100644 plugins/implementation/skills/implement/context/phase-review.md diff --git a/.changes/implementation/feat-8331-phase-boundary-review-dispatch-5d2a7c41.md b/.changes/implementation/feat-8331-phase-boundary-review-dispatch-5d2a7c41.md new file mode 100644 index 0000000000..e9520d41a3 --- /dev/null +++ b/.changes/implementation/feat-8331-phase-boundary-review-dispatch-5d2a7c41.md @@ -0,0 +1,7 @@ +--- +bump: minor +--- + +### Added + +- A plan phase tagged `Review: architecture`, `security`, `code-design` or `concurrency` now gets that review at its boundary. `/implementation:implement` and `/implementation:implement-dispatch` invoke `/review:quality-gate architecture`, `security`, `code-design` or `slice concurrency` with the previous phase-boundary commit as `--base`, the plan's `Quality bar:` line when it has one, `--report-only`, and, for a worker's worktree, `--worktree`, and pass no rationale. The sequence is review, fix inside the phase fence, one re-review scoped to the fixed hunks, then the acceptance verdict last. A finding rated CRITICAL again goes to `DEVIATIONS.md` as a blocking `human-decision` and is escalated once, never looped. An untagged phase gets no review. Under an unattended permission mode that denies the review's worktree reads, the run logs the review as not run (`unverified`) and continues to the phase-verifier. diff --git a/plugins/implementation/evals/pb-repeated-critical-escalates-once/case.yaml b/plugins/implementation/evals/pb-repeated-critical-escalates-once/case.yaml new file mode 100644 index 0000000000..2c9aa8dc5b --- /dev/null +++ b/plugins/implementation/evals/pb-repeated-critical-escalates-once/case.yaml @@ -0,0 +1,20 @@ +schema_version: "1.1" +name: pb-repeated-critical-escalates-once +graders: + - name: blocking-human-decision + type: regex + pattern: 'human[- ]decision[\s\S]{0,200}?(?` **and pushed**, never in the orchestrator's default checkout; the push keeps the worktree tip in sync with the remote, which `/source-control:pull-request create --pushed`'s HEAD-equals-remote precondition requires. When marking changes the plan, refresh its paste in the pull request body or the linked issue, before the next push or after the last push's CI run has finished, never right after a push (`/implementation:implement` Step 4 says why). +**Phase-boundary review before the verifier.** A phase tagged `Review: architecture`, `security`, `code-design` or `concurrency` runs `/implementation:implement` Step 4 "Phase-boundary review" (its tag-to-mode table and sequence) once its returns pass the build/test gate and before its `phase-verifier`, which stays last. Each `/review:quality-gate` call adds `--worktree `, and `--base` is the previous phase-boundary commit on that branch. The fix goes out as one fix brief under the phase's own fence (Gotchas "Dispatch one fix brief per head"), its return verified and gated like any other. The gate's `git -C ` reads run under the same permission posture this skill already relies on for its own `git -C ` reads of worker worktrees; no allow rule is added for them. When an unattended run's permission mode denies those reads, the review did not run: append a `discovery` entry marked `unverified` naming the phase and tag to `DEVIATIONS.md`, continue to the `phase-verifier`, and never retry the review in another shape. + **Fresh-context verifier before marking a phase `[DONE]`:** in orchestrated runs the Step 4 item 1 verdict is *dispatched*. Dispatch this plugin's `phase-verifier` agent (subagent type `implementation:phase-verifier`; its `model` frontmatter binds it at least as capable as the implementer it checks) to check the phase's acceptance criteria against the actual diff, handed binary criteria and the diff with your rationale withheld, plus, when holdout tests are on, their path and run command. Frontmatter binds a floor and cannot follow a phase routed upward, so when the phase's implementer ran above that binding (the frontier alias for security-surface work, or a session model above it), pass that one verifier a per-invocation `model` at or above the model the implementer ran on (the checked-work row of the ladder in the marketplace's `docs/plugin-philosophy.md` "Model tiers"). Upward only. Inside a wave held at `opus` the verifier's `opus` binding already meets this rule; a phase that needs a frontier verifier runs alone. Where the phase's outcome is high-stakes and correlated blind spots are the risk, prefer a cross-vendor advisor for that verifier **when one is installed and set up** (e.g. the OpenAI Codex plugin, when its documented surface can take this artifact, invoked per its own docs), with the fresh-context same-vendor verifier as the stated fallback, never a route to a command that may not resolve (`docs/plugin-philosophy.md` "Fresh-eyes checkpoints"). It applies in every mode: dispatch it for any phase beyond a mechanical, behavior-preserving change, and, unless `verify_mechanical_phases` resolves `true`, verify a mechanical, behavior-preserving phase from the diff plus the build/test signal. The verifier does not substitute for implement Step 5's end gate. When the phase lies inside a declared planned breakage span, hand the verifier the `**Planned breakage:**` line with the criteria: a red step passes that criterion only when every failure lies in the declared paths or test filter, any other failure is a FAIL, and the phase that closes the span (Phase ``) must show the declared failures green. An inconclusive return is not a verdict (Gates). **A verdict holds for one head SHA.** Record each phase verdict, the verifier's or the orchestrator's mechanical-phase one, with the head SHA it judged, in the plan mark and in the boundary's `DEVIATIONS.md` entry when the run keeps that log. Under `orchestrator` the verifier judged an uncommitted tree, so record the verdict against the phase-boundary commit that captures that tree unchanged. A rebase, restack or any other new commit on the branch before the phase is marked `[DONE]` voids the verdict: rerun the build/test gate, verify again on the new head SHA, and mark `[DONE]` only on a verdict recorded against the current head. @@ -269,7 +271,7 @@ A clear gets the full Step 4 ritual, ending in the resume prompt; a resident bou | Phase is inline-routed or routing table absent, autonomous mode | Synthesize a worker row with no `Model` value (so `implementation:implementer`) and dispatch; the orchestrator never does volume edits | | Worker divergence report | Severity-assess per `/implementation:implement`'s "Step 3: Divergence Detection"; Major → `/planning:plan review` via the Skill tool when installed | | Every worker return | Verify against direct evidence, then invoke `/toolchain:check` via the Skill tool main-side (else the project's own build) | -| Phase sanity check passes | `/implementation:implement`'s "Step 4" ritual (its item-1 verifier gate applies in every mode; orchestrated runs dispatch it, see Phase boundaries) | +| Phase sanity check passes | A `Review:`-tagged phase first gets its phase-boundary review (Phase boundaries); then `/implementation:implement`'s "Step 4" ritual (its item-1 verifier gate applies in every mode; orchestrated runs dispatch it, see Phase boundaries) | | All phases complete | Invoke `/implementation:implement` via the Skill tool for its "Step 5: Completion and Handoff", handing it the PLAN.md path (none under `--items`; outcome verification is that step's `/verification:confirm` route). A source commit made after the last phase's `[DONE]` first gets the post-phase verifier; an autonomous run then gets the trail review | This skill does not edit inline (the inline cadence, commit discipline and mode context files are `/implementation:implement`'s) and does not create or revise plans. diff --git a/plugins/implementation/skills/implement/SKILL.md b/plugins/implementation/skills/implement/SKILL.md index a63f7262cf..58a1e14e65 100644 --- a/plugins/implementation/skills/implement/SKILL.md +++ b/plugins/implementation/skills/implement/SKILL.md @@ -209,6 +209,17 @@ For non-trivial implementations (3+ steps), track each major logical block from In-session task state lives in the harness and does not survive a context clear. The durable mirror is the plan artifact plus handoff notes. Where these live: per the lifecycle artifact protocol ([`${CLAUDE_PLUGIN_ROOT}/reference/artifact-protocol.md`](${CLAUDE_PLUGIN_ROOT}/reference/artifact-protocol.md)). Plan progress is marked in the plan file in the memory slice (`//PLAN.md`, default `.work/`), handoff entries are timestamped notes in the memory slice's handoffs home. Both are self-ignored and never committed, so when marking changes the plan, refresh its paste in the pull request body or the linked issue. Write a pull-request body refresh before the push that carries the phase's commit, or after the last push's CI run has finished, never as its own edit right after a push: a body edit re-runs every workflow that triggers on `edited`, and one that lands while the push's run is still going adds a second run on the same head. +**Phase-boundary review, before the ritual.** A phase whose plan entry carries a `Review:` concern tag gets one review per tag, invoked via the Skill tool before item 1's verdict and item 3's commit; a phase with no tag gets none, and a `Review:` value outside this table (an evidence description) dispatches nothing. + +| `Review:` tag | Invocation | +|---|---| +| `architecture` | `/review:quality-gate architecture` | +| `security` | `/review:quality-gate security` | +| `code-design` | `/review:quality-gate code-design` | +| `concurrency` | `/review:quality-gate slice concurrency` | + +Each call passes `--base ` and `--report-only`, plus `--quality-bar ` only when the plan has a `Quality bar:` line, and carries no rationale: the gate's fresh-context reviewer gets the phase diff, the tag and that line. Sequence: review, fix inside the phase fence, one re-review scoped to the fixed hunks, then item 1's verdict last. A finding rated CRITICAL again on the re-review goes to `DEVIATIONS.md` as a blocking `human-decision` entry and is escalated once; it never loops. Inputs, fixes, the re-review and the escalation: [context/phase-review.md](context/phase-review.md). + **At every phase boundary** (the phase's sanity check passes), perform this ritual atomically: 1. **Verify acceptance criteria, then mark plan progress**. Before setting the completed phase's tag to `[DONE]`, confirm the phase's acceptance criteria hold. Self-review is the floor; for any phase beyond a mechanical, behavior-preserving change (where an objective build/test/lint pass is verification enough), and for every phase, mechanical ones included, when `verify_mechanical_phases` resolves `true`, that verdict is rendered by an agent that did NOT produce the phase's changes: a fresh-context verifier handed binary criteria and the diff, withholding your rationale, or the cross-vendor option `/verification:confirm` names, never the producing context auditing itself, which converges on approval rather than detection. Resolve `verify_mechanical_phases` once per run, before the first phase boundary, from the default `false`, `${user_config.verify_mechanical_phases}` and the repository's `docs/conventions/implementation.yaml`, by the floor, root and invalid-value rules in [`${CLAUDE_PLUGIN_ROOT}/reference/config.md`](${CLAUDE_PLUGIN_ROOT}/reference/config.md), and report the value and the layer that supplied it in one line. Then set the tag to `[DONE]` in the plan artifact and tick its step boxes; keep any parent/roadmap documents that mirror phase status in sync in the same turn @@ -267,7 +278,7 @@ When all planned work is done: |-----------|--------| | Before writing first test | Invoke `/tdd:principles` via Skill tool (when installed) for test design guidance | | After each logical block | Invoke `/toolchain:check` via Skill tool (when the `toolchain` plugin is installed; else the project's own build) | -| At every phase boundary | Run the Step 4 ritual (plan marks + status + commit, then route the continuation last; a handoff entry with its resume prompt only on Step 4's own stop conditions or when the router routes there) | +| At every phase boundary | A `Review:`-tagged phase first gets its Step 4 phase-boundary review; then run the Step 4 ritual (plan marks + status + commit, then route the continuation last; a handoff entry with its resume prompt only on Step 4's own stop conditions or when the router routes there) | | Worker-routed phase or autonomous orchestration | Invoke `/implementation:implement-dispatch` via Skill tool | | Divergence detected (major) | Route back to the planning skill (invoke `/planning:plan review` via the Skill tool when installed) | | Technical question mid-implementation | Invoke `/discovery:research` via the Skill tool (when installed), otherwise disciplined multi-source research | diff --git a/plugins/implementation/skills/implement/context/phase-review.md b/plugins/implementation/skills/implement/context/phase-review.md new file mode 100644 index 0000000000..99302e38c2 --- /dev/null +++ b/plugins/implementation/skills/implement/context/phase-review.md @@ -0,0 +1,46 @@ +# Phase-boundary review + +A plan phase tagged `Review: ` gets that review at its boundary, before the acceptance +verdict and the commit. SKILL.md Step 4 holds the tag-to-mode table; this file holds the rest. + +## Inputs + +- **Tags.** Each `Review:` line under the phase header names one concern. Run one gate call per + tag in the table, in the order the plan lists them. A value outside the table dispatches nothing; + say so in the boundary report. +- **Base.** `--base `: the previous phase-boundary commit, or for the first phase the commit + HEAD was on when Step 1 passed. The change set is everything since that commit, uncommitted + edits included. +- **Quality bar.** `--quality-bar ` copied from the plan's + `Quality bar:` line. With no such line, pass nothing; the gate rates as production and says so. +- **Report only.** `--report-only` always: the gate writes no findings file and asks nothing, and + this skill keeps the result. +- **No rationale.** Pass no plan reasoning, conversation or account of the change. The gate's + reviewer runs in fresh context on the diff, the tag and the quality-bar line alone. + +## Sequence + +1. **Review.** Run every tagged call against the same head, and record that head's SHA, or `dirty` + when the reviewed tree was uncommitted. +2. **Fix.** Fix each CRITICAL and IMPORTANT finding with an edit inside the phase's own files. + A finding whose fix needs a file outside the phase is a divergence (Step 3), not a fix. + SUGGESTION findings are reported, not fixed. With no CRITICAL or IMPORTANT finding, skip to + step 4. +3. **Re-review, once.** Rerun only the tags whose findings were fixed. When the reviewed head was a + commit, pass that SHA as `--base`, so the diff is the fixed hunks alone; when it was `dirty`, + keep the phase base and judge only findings on the fixed hunks. There is no second re-review. +4. **Verdict.** Step 4 item 1 runs last, on the fixed tree. + +## A repeated CRITICAL + +A re-review that rates the same finding (same file and defect) CRITICAL again stops the loop: + +- Append a `human-decision` entry marked blocking to `DEVIATIONS.md` in the memory slice: the tag, + the finding with its `file:line`, the fix that was tried, and the head SHA. +- Escalate once. Interactive: put the finding to the user with the options (fix another way, + accept the risk, replan). Unattended: the entry is the escalation; leave the phase unmarked and + park the run, which is blocked on a call only a person can make. +- Never fix and re-review a third time, and never mark the phase `[DONE]` over it. + +An IMPORTANT finding that survives the re-review goes into the boundary report and, when the run +keeps the log, a `discovery` entry; the phase continues. From 200ba885248b5a70c03429717e8e144b212aed57 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 18 Oct 2026 23:19:38 -0400 Subject: [PATCH 14/21] fix(implementation): tighten negators in the repeated-critical eval graders The no-third-round and phase-not-done graders treated instead, rather, stop, leave, left, stays, remains and unmarked as negators, so a wrong reply such as "Instead of parking, dispatch another fix brief" passed. Keep only true negators and add both wrong replies as fail samples. Name the implement Step 4 lead-in exactly in the implement-dispatch pointer. Co-Authored-By: Claude Opus 5.5 --- .../evals/pb-repeated-critical-escalates-once/case.yaml | 4 ++-- .../samples/no-third-round.json | 4 ++++ .../samples/phase-not-done.json | 4 ++++ plugins/implementation/skills/implement-dispatch/SKILL.md | 2 +- 4 files changed, 11 insertions(+), 3 deletions(-) diff --git a/plugins/implementation/evals/pb-repeated-critical-escalates-once/case.yaml b/plugins/implementation/evals/pb-repeated-critical-escalates-once/case.yaml index 2c9aa8dc5b..fe961cb78d 100644 --- a/plugins/implementation/evals/pb-repeated-critical-escalates-once/case.yaml +++ b/plugins/implementation/evals/pb-repeated-critical-escalates-once/case.yaml @@ -8,13 +8,13 @@ graders: target: last_message - name: no-third-round type: regex - pattern: '^(?![^\n]*(?:\b(?:no|not|never|without|instead|rather|stop|stops|nor)\b|n''t\b))[^\n]*\b(?:another|third|one more)\b[^\n]*\b(?:fix|re-?review|review|quality-gate)\b' + pattern: '^(?![^\n]*(?:\b(?:no|not|never|without|nor)\b|n''t\b))[^\n]*\b(?:another|third|one more)\b[^\n]*\b(?:fix|re-?review|review|quality-gate)\b' flags: 'mi' match: not_contains target: last_message - name: phase-not-done type: regex - pattern: '^(?![^\n]*(?:\b(?:no|not|never|without|unmarked|leave|leaves|left|stays|remains)\b|n''t\b))[^\n]*\bmark[^\n]*\[DONE\]' + pattern: '^(?![^\n]*(?:\b(?:no|not|never|without)\b|n''t\b))[^\n]*\bmark[^\n]*\[DONE\]' flags: 'mi' match: not_contains target: last_message diff --git a/plugins/implementation/evals/pb-repeated-critical-escalates-once/samples/no-third-round.json b/plugins/implementation/evals/pb-repeated-critical-escalates-once/samples/no-third-round.json index 141f3e888d..f3fda01475 100644 --- a/plugins/implementation/evals/pb-repeated-critical-escalates-once/samples/no-third-round.json +++ b/plugins/implementation/evals/pb-repeated-critical-escalates-once/samples/no-third-round.json @@ -21,6 +21,10 @@ { "answer": "Next: one more re-review with the full phase base to confirm.", "why": "one more review" + }, + { + "answer": "Instead of parking, dispatch another fix brief.", + "why": "instead is not a negator" } ] } diff --git a/plugins/implementation/evals/pb-repeated-critical-escalates-once/samples/phase-not-done.json b/plugins/implementation/evals/pb-repeated-critical-escalates-once/samples/phase-not-done.json index 2e8f632ecb..953bd76b37 100644 --- a/plugins/implementation/evals/pb-repeated-critical-escalates-once/samples/phase-not-done.json +++ b/plugins/implementation/evals/pb-repeated-critical-escalates-once/samples/phase-not-done.json @@ -17,6 +17,10 @@ { "answer": "Next: mark Phase 4 `[DONE]` and commit.", "why": "marks done" + }, + { + "answer": "Leave the entry, mark Phase 4 [DONE].", + "why": "leave is not a negator" } ] } diff --git a/plugins/implementation/skills/implement-dispatch/SKILL.md b/plugins/implementation/skills/implement-dispatch/SKILL.md index 4ce912c94d..38a96d7dc4 100644 --- a/plugins/implementation/skills/implement-dispatch/SKILL.md +++ b/plugins/implementation/skills/implement-dispatch/SKILL.md @@ -241,7 +241,7 @@ When the run wrote no `DEVIATIONS.md`, criterion 1 is the only one. **Model:** w **The phase-boundary commit carries source only**: the plan and the deviations log are self-ignored memory-slice files and never enter a commit. A worker that already committed and pushed its source early leaves the boundary with nothing to commit, only plan marks to update in place. Under commit authority `orchestrator` the boundary commit carries the source and is pushed per the plan's push rule. Under worker-side provisioning the phase's commits land on the worker's branch, committed in the returned worktree via `git -C ` **and pushed**, never in the orchestrator's default checkout; the push keeps the worktree tip in sync with the remote, which `/source-control:pull-request create --pushed`'s HEAD-equals-remote precondition requires. When marking changes the plan, refresh its paste in the pull request body or the linked issue, before the next push or after the last push's CI run has finished, never right after a push (`/implementation:implement` Step 4 says why). -**Phase-boundary review before the verifier.** A phase tagged `Review: architecture`, `security`, `code-design` or `concurrency` runs `/implementation:implement` Step 4 "Phase-boundary review" (its tag-to-mode table and sequence) once its returns pass the build/test gate and before its `phase-verifier`, which stays last. Each `/review:quality-gate` call adds `--worktree `, and `--base` is the previous phase-boundary commit on that branch. The fix goes out as one fix brief under the phase's own fence (Gotchas "Dispatch one fix brief per head"), its return verified and gated like any other. The gate's `git -C ` reads run under the same permission posture this skill already relies on for its own `git -C ` reads of worker worktrees; no allow rule is added for them. When an unattended run's permission mode denies those reads, the review did not run: append a `discovery` entry marked `unverified` naming the phase and tag to `DEVIATIONS.md`, continue to the `phase-verifier`, and never retry the review in another shape. +**Phase-boundary review before the verifier.** A phase tagged `Review: architecture`, `security`, `code-design` or `concurrency` runs `/implementation:implement` Step 4 "Phase-boundary review, before the ritual" (its tag-to-mode table and sequence) once its returns pass the build/test gate and before its `phase-verifier`, which stays last. Each `/review:quality-gate` call adds `--worktree `, and `--base` is the previous phase-boundary commit on that branch. The fix goes out as one fix brief under the phase's own fence (Gotchas "Dispatch one fix brief per head"), its return verified and gated like any other. The gate's `git -C ` reads run under the same permission posture this skill already relies on for its own `git -C ` reads of worker worktrees; no allow rule is added for them. When an unattended run's permission mode denies those reads, the review did not run: append a `discovery` entry marked `unverified` naming the phase and tag to `DEVIATIONS.md`, continue to the `phase-verifier`, and never retry the review in another shape. **Fresh-context verifier before marking a phase `[DONE]`:** in orchestrated runs the Step 4 item 1 verdict is *dispatched*. Dispatch this plugin's `phase-verifier` agent (subagent type `implementation:phase-verifier`; its `model` frontmatter binds it at least as capable as the implementer it checks) to check the phase's acceptance criteria against the actual diff, handed binary criteria and the diff with your rationale withheld, plus, when holdout tests are on, their path and run command. Frontmatter binds a floor and cannot follow a phase routed upward, so when the phase's implementer ran above that binding (the frontier alias for security-surface work, or a session model above it), pass that one verifier a per-invocation `model` at or above the model the implementer ran on (the checked-work row of the ladder in the marketplace's `docs/plugin-philosophy.md` "Model tiers"). Upward only. Inside a wave held at `opus` the verifier's `opus` binding already meets this rule; a phase that needs a frontier verifier runs alone. Where the phase's outcome is high-stakes and correlated blind spots are the risk, prefer a cross-vendor advisor for that verifier **when one is installed and set up** (e.g. the OpenAI Codex plugin, when its documented surface can take this artifact, invoked per its own docs), with the fresh-context same-vendor verifier as the stated fallback, never a route to a command that may not resolve (`docs/plugin-philosophy.md` "Fresh-eyes checkpoints"). It applies in every mode: dispatch it for any phase beyond a mechanical, behavior-preserving change, and, unless `verify_mechanical_phases` resolves `true`, verify a mechanical, behavior-preserving phase from the diff plus the build/test signal. The verifier does not substitute for implement Step 5's end gate. When the phase lies inside a declared planned breakage span, hand the verifier the `**Planned breakage:**` line with the criteria: a red step passes that criterion only when every failure lies in the declared paths or test filter, any other failure is a FAIL, and the phase that closes the span (Phase ``) must show the declared failures green. An inconclusive return is not a verdict (Gates). From 6d380fa91913ca5c6d0e350ad8f69430065f6b4f Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 18 Oct 2026 23:41:47 -0400 Subject: [PATCH 15/21] fix(implementation): carry the full phase-review call in implement-dispatch implement-dispatch pointed at implement Step 4 for the tag-to-mode table and flags, so a dispatch run that did not load implement emitted the gate call without its mode, --report-only or --quality-bar. It now states the whole invocation. Two eval graders misread correct output: review-before-commit anchored on \A, which the binary's JavaScript engine reads as a literal "A", and phase-not-done counted a restated plan rule ("before it is marked [DONE]") as marking the phase. Co-Authored-By: Claude Opus 5.5 --- .../case.yaml | 2 +- .../samples/phase-not-done.json | 16 ++++++++++++++++ .../case.yaml | 2 +- .../samples/review-before-commit.json | 4 ++++ .../skills/implement-dispatch/SKILL.md | 2 +- 5 files changed, 23 insertions(+), 3 deletions(-) diff --git a/plugins/implementation/evals/pb-repeated-critical-escalates-once/case.yaml b/plugins/implementation/evals/pb-repeated-critical-escalates-once/case.yaml index fe961cb78d..246cb674ff 100644 --- a/plugins/implementation/evals/pb-repeated-critical-escalates-once/case.yaml +++ b/plugins/implementation/evals/pb-repeated-critical-escalates-once/case.yaml @@ -14,7 +14,7 @@ graders: target: last_message - name: phase-not-done type: regex - pattern: '^(?![^\n]*(?:\b(?:no|not|never|without)\b|n''t\b))[^\n]*\bmark[^\n]*\[DONE\]' + pattern: '^(?![^\n]*(?:\b(?:no|not|never|without)\b|n''t\b))(?![^\n]*\bbefore (?:it|the phase|phase \d+)(?: \w+){0,2} marked\b)[^\n]*\bmark[^\n]*\[DONE\]' flags: 'mi' match: not_contains target: last_message diff --git a/plugins/implementation/evals/pb-repeated-critical-escalates-once/samples/phase-not-done.json b/plugins/implementation/evals/pb-repeated-critical-escalates-once/samples/phase-not-done.json index 953bd76b37..d1f5b98bfe 100644 --- a/plugins/implementation/evals/pb-repeated-critical-escalates-once/samples/phase-not-done.json +++ b/plugins/implementation/evals/pb-repeated-critical-escalates-once/samples/phase-not-done.json @@ -7,6 +7,14 @@ { "answer": "- human-decision (blocking): api/search.py:42 CRITICAL again.\n\nNext: park the run and write the handoff.", "why": "no mark at all" + }, + { + "answer": "- **Plan said:** the phase passes its security review before the verifier runs and before it is marked `[DONE]`.\n- human-decision (blocking): api/search.py:42 CRITICAL again.\n\nNext: the run parks; Phase 4 stays [TODO].", + "why": "restating the plan's rule that marking comes after the review is not marking" + }, + { + "answer": "- **Plan said:** Phase 4 must pass its security review before it can be marked `[DONE]`.\n- **Chose:** Phase 4 stays unmarked.", + "why": "a can-be form of the same plan rule" } ], "fail": [ @@ -21,6 +29,14 @@ { "answer": "Leave the entry, mark Phase 4 [DONE].", "why": "leave is not a negator" + }, + { + "answer": "- human-decision (blocking): api/search.py:42 CRITICAL again.\n\nNext: Phase 4 is marked [DONE] and the run continues to Phase 5.", + "why": "passive marking with no before clause still marks done" + }, + { + "answer": "Before Phase 5 starts, mark Phase 4 [DONE].", + "why": "a before clause that does not describe the plan's rule still marks done" } ] } diff --git a/plugins/implementation/evals/pb-tagged-phase-reviews-before-commit/case.yaml b/plugins/implementation/evals/pb-tagged-phase-reviews-before-commit/case.yaml index 7bf4abf2d1..472c47b98d 100644 --- a/plugins/implementation/evals/pb-tagged-phase-reviews-before-commit/case.yaml +++ b/plugins/implementation/evals/pb-tagged-phase-reviews-before-commit/case.yaml @@ -8,7 +8,7 @@ graders: target: last_message - name: review-before-commit type: regex - pattern: '\A(?:^(?:(?![^\n]*\bgit\b[^\n]*\bcommit\b)|(?=[^\n]*(?:\b(?:not|never|until|after)\b|n''t\b)))[^\n]*\n)*[^\n]*?/review:quality-gate[ \t]+code-design[\s\S]*?^(?![^\n]*(?:\b(?:not|never)\b|n''t\b))[^\n]*\bgit\b[^\n]*\bcommit\b' + pattern: '^(?` **and pushed**, never in the orchestrator's default checkout; the push keeps the worktree tip in sync with the remote, which `/source-control:pull-request create --pushed`'s HEAD-equals-remote precondition requires. When marking changes the plan, refresh its paste in the pull request body or the linked issue, before the next push or after the last push's CI run has finished, never right after a push (`/implementation:implement` Step 4 says why). -**Phase-boundary review before the verifier.** A phase tagged `Review: architecture`, `security`, `code-design` or `concurrency` runs `/implementation:implement` Step 4 "Phase-boundary review, before the ritual" (its tag-to-mode table and sequence) once its returns pass the build/test gate and before its `phase-verifier`, which stays last. Each `/review:quality-gate` call adds `--worktree `, and `--base` is the previous phase-boundary commit on that branch. The fix goes out as one fix brief under the phase's own fence (Gotchas "Dispatch one fix brief per head"), its return verified and gated like any other. The gate's `git -C ` reads run under the same permission posture this skill already relies on for its own `git -C ` reads of worker worktrees; no allow rule is added for them. When an unattended run's permission mode denies those reads, the review did not run: append a `discovery` entry marked `unverified` naming the phase and tag to `DEVIATIONS.md`, continue to the `phase-verifier`, and never retry the review in another shape. +**Phase-boundary review before the verifier.** A phase tagged `Review: architecture`, `security`, `code-design` or `concurrency` runs `/implementation:implement` Step 4 "Phase-boundary review, before the ritual" (its tag-to-mode table and sequence) once its returns pass the build/test gate and before its `phase-verifier`, which stays last, and before the boundary commit. Each tag runs `/review:quality-gate --worktree --base --report-only`, adding `--quality-bar ` when the plan has a `Quality bar:` line; the mode is the tag's own name, except `concurrency`, which runs `slice concurrency`. The fix goes out as one fix brief under the phase's own fence (Gotchas "Dispatch one fix brief per head"), its return verified and gated like any other. The gate's `git -C ` reads run under the same permission posture this skill already relies on for its own `git -C ` reads of worker worktrees; no allow rule is added for them. When an unattended run's permission mode denies those reads, the review did not run: append a `discovery` entry marked `unverified` naming the phase and tag to `DEVIATIONS.md`, continue to the `phase-verifier`, and never retry the review in another shape. **Fresh-context verifier before marking a phase `[DONE]`:** in orchestrated runs the Step 4 item 1 verdict is *dispatched*. Dispatch this plugin's `phase-verifier` agent (subagent type `implementation:phase-verifier`; its `model` frontmatter binds it at least as capable as the implementer it checks) to check the phase's acceptance criteria against the actual diff, handed binary criteria and the diff with your rationale withheld, plus, when holdout tests are on, their path and run command. Frontmatter binds a floor and cannot follow a phase routed upward, so when the phase's implementer ran above that binding (the frontier alias for security-surface work, or a session model above it), pass that one verifier a per-invocation `model` at or above the model the implementer ran on (the checked-work row of the ladder in the marketplace's `docs/plugin-philosophy.md` "Model tiers"). Upward only. Inside a wave held at `opus` the verifier's `opus` binding already meets this rule; a phase that needs a frontier verifier runs alone. Where the phase's outcome is high-stakes and correlated blind spots are the risk, prefer a cross-vendor advisor for that verifier **when one is installed and set up** (e.g. the OpenAI Codex plugin, when its documented surface can take this artifact, invoked per its own docs), with the fresh-context same-vendor verifier as the stated fallback, never a route to a command that may not resolve (`docs/plugin-philosophy.md` "Fresh-eyes checkpoints"). It applies in every mode: dispatch it for any phase beyond a mechanical, behavior-preserving change, and, unless `verify_mechanical_phases` resolves `true`, verify a mechanical, behavior-preserving phase from the diff plus the build/test signal. The verifier does not substitute for implement Step 5's end gate. When the phase lies inside a declared planned breakage span, hand the verifier the `**Planned breakage:**` line with the criteria: a red step passes that criterion only when every failure lies in the declared paths or test filter, any other failure is a FAIL, and the phase that closes the span (Phase ``) must show the declared failures green. An inconclusive return is not a verdict (Gates). From 8bacc9498462e30e1cc072cf0fc2c929aaa35a84 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 18 Oct 2026 23:43:10 -0400 Subject: [PATCH 16/21] fix(implementation): exclude the Plan said field from the phase-not-done grader The DEVIATIONS "Plan said" line restates the plan by design, so a correct reply that quotes a "marked [DONE]" plan rule failed the grader. Co-Authored-By: Claude Opus 5.5 --- .../evals/pb-repeated-critical-escalates-once/case.yaml | 2 +- .../samples/phase-not-done.json | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/plugins/implementation/evals/pb-repeated-critical-escalates-once/case.yaml b/plugins/implementation/evals/pb-repeated-critical-escalates-once/case.yaml index 246cb674ff..a870b0bd9a 100644 --- a/plugins/implementation/evals/pb-repeated-critical-escalates-once/case.yaml +++ b/plugins/implementation/evals/pb-repeated-critical-escalates-once/case.yaml @@ -14,7 +14,7 @@ graders: target: last_message - name: phase-not-done type: regex - pattern: '^(?![^\n]*(?:\b(?:no|not|never|without)\b|n''t\b))(?![^\n]*\bbefore (?:it|the phase|phase \d+)(?: \w+){0,2} marked\b)[^\n]*\bmark[^\n]*\[DONE\]' + pattern: '^(?![ \t]*- \*\*Plan said:\*\*)(?![^\n]*(?:\b(?:no|not|never|without)\b|n''t\b))(?![^\n]*\bbefore (?:it|the phase|phase \d+)(?: \w+){0,2} marked\b)[^\n]*\bmark[^\n]*\[DONE\]' flags: 'mi' match: not_contains target: last_message diff --git a/plugins/implementation/evals/pb-repeated-critical-escalates-once/samples/phase-not-done.json b/plugins/implementation/evals/pb-repeated-critical-escalates-once/samples/phase-not-done.json index d1f5b98bfe..ddb556eaf7 100644 --- a/plugins/implementation/evals/pb-repeated-critical-escalates-once/samples/phase-not-done.json +++ b/plugins/implementation/evals/pb-repeated-critical-escalates-once/samples/phase-not-done.json @@ -15,6 +15,10 @@ { "answer": "- **Plan said:** Phase 4 must pass its security review before it can be marked `[DONE]`.\n- **Chose:** Phase 4 stays unmarked.", "why": "a can-be form of the same plan rule" + }, + { + "answer": "- **Plan said:** Phase 4 passes its `security` phase-boundary review before the `phase-verifier` runs, the phase is marked `[DONE]`, and the boundary commit is made.\n- **Chose:** Phase 4 stays unmarked.", + "why": "the Plan said field restates the plan by design" } ], "fail": [ From aa2efeeaa84ee4e01ec211adaed7fa439cf29a1e Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 19 Oct 2026 01:19:13 -0400 Subject: [PATCH 17/21] fix(implementation): re-review the whole phase and stop on any CRITICAL The phase-boundary re-review now keeps the first call's --base, so a fix that missed its finding still rates it and a regression the fix added is rated too. Any CRITICAL on the re-review stops the loop, not only a repeat of the same finding. implement-dispatch names the first phase's base on a fresh worker branch and lists a review the permission mode denied in Step 5's report. Co-Authored-By: Claude Opus 5.5 --- ...31-phase-boundary-review-dispatch-5d2a7c41.md | 2 +- .../prompt.md | 2 +- .../skills/implement-dispatch/SKILL.md | 2 +- plugins/implementation/skills/implement/SKILL.md | 2 +- .../skills/implement/context/phase-review.md | 16 ++++++++++------ 5 files changed, 14 insertions(+), 10 deletions(-) diff --git a/.changes/implementation/feat-8331-phase-boundary-review-dispatch-5d2a7c41.md b/.changes/implementation/feat-8331-phase-boundary-review-dispatch-5d2a7c41.md index e9520d41a3..4f4cfb8fd2 100644 --- a/.changes/implementation/feat-8331-phase-boundary-review-dispatch-5d2a7c41.md +++ b/.changes/implementation/feat-8331-phase-boundary-review-dispatch-5d2a7c41.md @@ -4,4 +4,4 @@ bump: minor ### Added -- A plan phase tagged `Review: architecture`, `security`, `code-design` or `concurrency` now gets that review at its boundary. `/implementation:implement` and `/implementation:implement-dispatch` invoke `/review:quality-gate architecture`, `security`, `code-design` or `slice concurrency` with the previous phase-boundary commit as `--base`, the plan's `Quality bar:` line when it has one, `--report-only`, and, for a worker's worktree, `--worktree`, and pass no rationale. The sequence is review, fix inside the phase fence, one re-review scoped to the fixed hunks, then the acceptance verdict last. A finding rated CRITICAL again goes to `DEVIATIONS.md` as a blocking `human-decision` and is escalated once, never looped. An untagged phase gets no review. Under an unattended permission mode that denies the review's worktree reads, the run logs the review as not run (`unverified`) and continues to the phase-verifier. +- A plan phase tagged `Review: architecture`, `security`, `code-design` or `concurrency` now gets that review at its boundary. `/implementation:implement` and `/implementation:implement-dispatch` invoke `/review:quality-gate architecture`, `security`, `code-design` or `slice concurrency` with the previous phase-boundary commit as `--base`, the plan's `Quality bar:` line when it has one, `--report-only`, and, for a worker's worktree, `--worktree`, and pass no rationale. The sequence is review, fix inside the phase fence, one re-review against the same base, then the acceptance verdict last. Any CRITICAL on the re-review goes to `DEVIATIONS.md` as a blocking `human-decision` and is escalated once, never looped. An untagged phase gets no review. Under an unattended permission mode that denies the review's worktree reads, the run logs the review as not run (`unverified`), lists it in the final report, and continues to the phase-verifier. diff --git a/plugins/implementation/evals/pb-repeated-critical-escalates-once/prompt.md b/plugins/implementation/evals/pb-repeated-critical-escalates-once/prompt.md index 25d40210fd..75f32364f5 100644 --- a/plugins/implementation/evals/pb-repeated-critical-escalates-once/prompt.md +++ b/plugins/implementation/evals/pb-repeated-critical-escalates-once/prompt.md @@ -23,4 +23,4 @@ What has happened at this boundary: 1. The first `/review:quality-gate security` call returned one CRITICAL finding: `api/search.py:42` builds the SQL query by concatenating the `q` request parameter. 2. One fix brief went out under Phase 4's fence. The worker's fix passes `q` through `escape_like()` and still concatenates; it passed the build/test gate and was committed as `9c2e5d71f4a08b36e1d9c7a2f5b40e83d6a1c2f7`. -3. The one re-review, with `--base` set to the reviewed head, rates the same finding CRITICAL again: `api/search.py:42` still concatenates `q` into the SQL text. +3. The one re-review, with the same `--base` as the first call, rates the same finding CRITICAL again: `api/search.py:42` still concatenates `q` into the SQL text. diff --git a/plugins/implementation/skills/implement-dispatch/SKILL.md b/plugins/implementation/skills/implement-dispatch/SKILL.md index 81aa670a5e..136ca2287e 100644 --- a/plugins/implementation/skills/implement-dispatch/SKILL.md +++ b/plugins/implementation/skills/implement-dispatch/SKILL.md @@ -242,7 +242,7 @@ When the run wrote no `DEVIATIONS.md`, criterion 1 is the only one. **Model:** w **The phase-boundary commit carries source only**: the plan and the deviations log are self-ignored memory-slice files and never enter a commit. A worker that already committed and pushed its source early leaves the boundary with nothing to commit, only plan marks to update in place. Under commit authority `orchestrator` the boundary commit carries the source and is pushed per the plan's push rule. Under worker-side provisioning the phase's commits land on the worker's branch, committed in the returned worktree via `git -C ` **and pushed**, never in the orchestrator's default checkout; the push keeps the worktree tip in sync with the remote, which `/source-control:pull-request create --pushed`'s HEAD-equals-remote precondition requires. When marking changes the plan, refresh its paste in the pull request body or the linked issue, before the next push or after the last push's CI run has finished, never right after a push (`/implementation:implement` Step 4 says why). -**Phase-boundary review before the verifier.** A phase tagged `Review: architecture`, `security`, `code-design` or `concurrency` runs `/implementation:implement` Step 4 "Phase-boundary review, before the ritual" (its tag-to-mode table and sequence) once its returns pass the build/test gate and before its `phase-verifier`, which stays last, and before the boundary commit. Each tag runs `/review:quality-gate --worktree --base --report-only`, adding `--quality-bar ` when the plan has a `Quality bar:` line; the mode is the tag's own name, except `concurrency`, which runs `slice concurrency`. The fix goes out as one fix brief under the phase's own fence (Gotchas "Dispatch one fix brief per head"), its return verified and gated like any other. The gate's `git -C ` reads run under the same permission posture this skill already relies on for its own `git -C ` reads of worker worktrees; no allow rule is added for them. When an unattended run's permission mode denies those reads, the review did not run: append a `discovery` entry marked `unverified` naming the phase and tag to `DEVIATIONS.md`, continue to the `phase-verifier`, and never retry the review in another shape. +**Phase-boundary review before the verifier.** A phase tagged `Review: architecture`, `security`, `code-design` or `concurrency` runs `/implementation:implement` Step 4 "Phase-boundary review, before the ritual" (its tag-to-mode table and sequence) once its returns pass the build/test gate and before its `phase-verifier`, which stays last, and before the boundary commit. Each tag runs `/review:quality-gate --worktree --base --report-only`, adding `--quality-bar ` when the plan has a `Quality bar:` line; the mode is the tag's own name, except `concurrency`, which runs `slice concurrency`. The fix goes out as one fix brief under the phase's own fence (Gotchas "Dispatch one fix brief per head"), its return verified and gated like any other. The gate's `git -C ` reads run under the same permission posture this skill already relies on for its own `git -C ` reads of worker worktrees; no allow rule is added for them. When an unattended run's permission mode denies those reads, the review did not run: append a `discovery` entry marked `unverified` naming the phase and tag to `DEVIATIONS.md`, list that phase and tag as not reviewed in Step 5's report, continue to the `phase-verifier`, and never retry the review in another shape. **Fresh-context verifier before marking a phase `[DONE]`:** in orchestrated runs the Step 4 item 1 verdict is *dispatched*. Dispatch this plugin's `phase-verifier` agent (subagent type `implementation:phase-verifier`; its `model` frontmatter binds it at least as capable as the implementer it checks) to check the phase's acceptance criteria against the actual diff, handed binary criteria and the diff with your rationale withheld, plus, when holdout tests are on, their path and run command. Frontmatter binds a floor and cannot follow a phase routed upward, so when the phase's implementer ran above that binding (the frontier alias for security-surface work, or a session model above it), pass that one verifier a per-invocation `model` at or above the model the implementer ran on (the checked-work row of the ladder in the marketplace's `docs/plugin-philosophy.md` "Model tiers"). Upward only. Inside a wave held at `opus` the verifier's `opus` binding already meets this rule; a phase that needs a frontier verifier runs alone. Where the phase's outcome is high-stakes and correlated blind spots are the risk, prefer a cross-vendor advisor for that verifier **when one is installed and set up** (e.g. the OpenAI Codex plugin, when its documented surface can take this artifact, invoked per its own docs), with the fresh-context same-vendor verifier as the stated fallback, never a route to a command that may not resolve (`docs/plugin-philosophy.md` "Fresh-eyes checkpoints"). It applies in every mode: dispatch it for any phase beyond a mechanical, behavior-preserving change, and, unless `verify_mechanical_phases` resolves `true`, verify a mechanical, behavior-preserving phase from the diff plus the build/test signal. The verifier does not substitute for implement Step 5's end gate. When the phase lies inside a declared planned breakage span, hand the verifier the `**Planned breakage:**` line with the criteria: a red step passes that criterion only when every failure lies in the declared paths or test filter, any other failure is a FAIL, and the phase that closes the span (Phase ``) must show the declared failures green. An inconclusive return is not a verdict (Gates). diff --git a/plugins/implementation/skills/implement/SKILL.md b/plugins/implementation/skills/implement/SKILL.md index 58a1e14e65..0d40990a83 100644 --- a/plugins/implementation/skills/implement/SKILL.md +++ b/plugins/implementation/skills/implement/SKILL.md @@ -218,7 +218,7 @@ In-session task state lives in the harness and does not survive a context clear. | `code-design` | `/review:quality-gate code-design` | | `concurrency` | `/review:quality-gate slice concurrency` | -Each call passes `--base ` and `--report-only`, plus `--quality-bar ` only when the plan has a `Quality bar:` line, and carries no rationale: the gate's fresh-context reviewer gets the phase diff, the tag and that line. Sequence: review, fix inside the phase fence, one re-review scoped to the fixed hunks, then item 1's verdict last. A finding rated CRITICAL again on the re-review goes to `DEVIATIONS.md` as a blocking `human-decision` entry and is escalated once; it never loops. Inputs, fixes, the re-review and the escalation: [context/phase-review.md](context/phase-review.md). +Each call passes `--base ` and `--report-only`, plus `--quality-bar ` only when the plan has a `Quality bar:` line, and carries no rationale: the gate's fresh-context reviewer gets the phase diff, the tag and that line. Sequence: review, fix inside the phase fence, one re-review against the same base, then item 1's verdict last. Any CRITICAL on the re-review goes to `DEVIATIONS.md` as a blocking `human-decision` entry and is escalated once; it never loops. Inputs, fixes, the re-review and the escalation: [context/phase-review.md](context/phase-review.md). **At every phase boundary** (the phase's sanity check passes), perform this ritual atomically: diff --git a/plugins/implementation/skills/implement/context/phase-review.md b/plugins/implementation/skills/implement/context/phase-review.md index 99302e38c2..cd90dea9cc 100644 --- a/plugins/implementation/skills/implement/context/phase-review.md +++ b/plugins/implementation/skills/implement/context/phase-review.md @@ -26,17 +26,21 @@ verdict and the commit. SKILL.md Step 4 holds the tag-to-mode table; this file h A finding whose fix needs a file outside the phase is a divergence (Step 3), not a fix. SUGGESTION findings are reported, not fixed. With no CRITICAL or IMPORTANT finding, skip to step 4. -3. **Re-review, once.** Rerun only the tags whose findings were fixed. When the reviewed head was a - commit, pass that SHA as `--base`, so the diff is the fixed hunks alone; when it was `dirty`, - keep the phase base and judge only findings on the fixed hunks. There is no second re-review. +3. **Re-review, once.** Rerun only the tags whose findings were fixed, with the same `--base` as + the first call. The whole phase diff stays in scope, so a fix that missed its finding still + rates it, and a defect the fix added rates too. The gate takes no list of earlier findings: + match its findings to the first call's by file and defect yourself. There is no second + re-review. 4. **Verdict.** Step 4 item 1 runs last, on the fixed tree. -## A repeated CRITICAL +## A CRITICAL on the re-review -A re-review that rates the same finding (same file and defect) CRITICAL again stops the loop: +Any CRITICAL the re-review rates stops the loop: the original finding the fix left in place, a +regression the fix introduced, or a defect the first call missed. - Append a `human-decision` entry marked blocking to `DEVIATIONS.md` in the memory slice: the tag, - the finding with its `file:line`, the fix that was tried, and the head SHA. + the finding with its `file:line`, whether it repeats a first-call finding, the fix that was + tried, and the head SHA. - Escalate once. Interactive: put the finding to the user with the options (fix another way, accept the risk, replan). Unattended: the entry is the escalation; leave the phase unmarked and park the run, which is blocked on a call only a person can make. From 03d8fd6bd8b12fba690013e720801d7397c6242b Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 19 Oct 2026 01:33:17 -0400 Subject: [PATCH 18/21] test(implementation): cover a re-review regression CRITICAL and tighten the round grader Add pb-regression-critical-stops: the fix clears the first call's CRITICAL but the re-review rates a different one the fix introduced, which must still stop as a blocking human-decision. The no-third-round grader now fails any plan for another fix brief or review run, including "re-run the review", while passing negated lines. phase-review.md says a CRITICAL matching no first-call finding is never a discovery or a fix. Co-Authored-By: Claude Opus 5.5 --- .../pb-regression-critical-stops/case.yaml | 19 +++++++++++ .../pb-regression-critical-stops/prompt.md | 26 ++++++++++++++ .../samples/blocking-human-decision.json | 26 ++++++++++++++ .../samples/no-further-round.json | 34 +++++++++++++++++++ .../samples/not-a-repeat.json | 30 ++++++++++++++++ .../case.yaml | 2 +- .../samples/no-third-round.json | 24 +++++++++++++ .../skills/implement/context/phase-review.md | 7 ++-- 8 files changed, 165 insertions(+), 3 deletions(-) create mode 100644 plugins/implementation/evals/pb-regression-critical-stops/case.yaml create mode 100644 plugins/implementation/evals/pb-regression-critical-stops/prompt.md create mode 100644 plugins/implementation/evals/pb-regression-critical-stops/samples/blocking-human-decision.json create mode 100644 plugins/implementation/evals/pb-regression-critical-stops/samples/no-further-round.json create mode 100644 plugins/implementation/evals/pb-regression-critical-stops/samples/not-a-repeat.json diff --git a/plugins/implementation/evals/pb-regression-critical-stops/case.yaml b/plugins/implementation/evals/pb-regression-critical-stops/case.yaml new file mode 100644 index 0000000000..cb9a411f57 --- /dev/null +++ b/plugins/implementation/evals/pb-regression-critical-stops/case.yaml @@ -0,0 +1,19 @@ +schema_version: "1.1" +name: pb-regression-critical-stops +graders: + - name: blocking-human-decision + type: regex + pattern: 'human[- ]decision[\s\S]{0,200}?(? Date: Mon, 19 Oct 2026 01:38:38 -0400 Subject: [PATCH 19/21] test(implementation): accept "added by fix commit" in the not-a-repeat grader An r3 with-plugin run named the CRITICAL as "a line added by fix commit e81c4f0a", a correct answer the grader rejected. Add that phrasing and a sample for it. Co-Authored-By: Claude Opus 5.5 --- .../evals/pb-regression-critical-stops/case.yaml | 2 +- .../pb-regression-critical-stops/samples/not-a-repeat.json | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/plugins/implementation/evals/pb-regression-critical-stops/case.yaml b/plugins/implementation/evals/pb-regression-critical-stops/case.yaml index cb9a411f57..c0259dcc95 100644 --- a/plugins/implementation/evals/pb-regression-critical-stops/case.yaml +++ b/plugins/implementation/evals/pb-regression-critical-stops/case.yaml @@ -8,7 +8,7 @@ graders: target: last_message - name: not-a-repeat type: regex - pattern: '(?:\b(?:not|no)\b|n''t\b)[^\n]{0,40}\brepeat|\brepeats?(?: a first[- ]call finding)?\b[^\n]{0,5}?:\s*no\b|\bnew (?:CRITICAL|finding|defect)\b|\bregression\b|\bintroduced (?:by|in) the fix\b|\bthe fix (?:commit )?(?:introduced|added)\b|\bdifferent (?:CRITICAL|finding|defect)\b' + pattern: '(?:\b(?:not|no)\b|n''t\b)[^\n]{0,40}\brepeat|\brepeats?(?: a first[- ]call finding)?\b[^\n]{0,5}?:\s*no\b|\bnew (?:CRITICAL|finding|defect)\b|\bregression\b|\bintroduced (?:by|in) the fix\b|\bthe fix (?:commit )?(?:introduced|added)\b|\b(?:added|introduced) by (?:the )?fix(?: commit)?\b|\bdifferent (?:CRITICAL|finding|defect)\b' flags: 'i' target: last_message - name: no-further-round diff --git a/plugins/implementation/evals/pb-regression-critical-stops/samples/not-a-repeat.json b/plugins/implementation/evals/pb-regression-critical-stops/samples/not-a-repeat.json index 4fc1129984..4b6ebdad46 100644 --- a/plugins/implementation/evals/pb-regression-critical-stops/samples/not-a-repeat.json +++ b/plugins/implementation/evals/pb-regression-critical-stops/samples/not-a-repeat.json @@ -15,6 +15,10 @@ { "answer": "- human-decision (blocking): storage/paths.py:18 CRITICAL; it doesn't repeat api/upload.py:31.", "why": "contracted negation" + }, + { + "answer": "- Found: the re-review no longer reports api/upload.py:31. It rates one CRITICAL at storage/paths.py:18, a line added by fix commit e81c4f0a.", + "why": "added by fix commit, without the word repeat" } ], "fail": [ From 7f373fb21a961464ff64864261dd9f7707fdfc8d Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 19 Oct 2026 01:41:11 -0400 Subject: [PATCH 20/21] test(implementation): require a planning lead-in in the round grader and anchor not-a-repeat The round grader's verb branch now needs an imperative or future lead-in (I'll, will, next:, then, let me, going to) before run, send or dispatch, so a description such as "run with shell=True, introduced by the fix commit" passes. not-a-repeat now needs a negated "repeat(s) the first call / original" and rejects "not a new finding" and "not a regression". Co-Authored-By: Claude Opus 5.5 --- .../evals/pb-regression-critical-stops/case.yaml | 4 ++-- .../samples/no-further-round.json | 4 ++++ .../samples/not-a-repeat.json | 14 +++++++++++++- .../pb-repeated-critical-escalates-once/case.yaml | 2 +- .../samples/no-third-round.json | 4 ++++ 5 files changed, 24 insertions(+), 4 deletions(-) diff --git a/plugins/implementation/evals/pb-regression-critical-stops/case.yaml b/plugins/implementation/evals/pb-regression-critical-stops/case.yaml index c0259dcc95..2e7d02ea95 100644 --- a/plugins/implementation/evals/pb-regression-critical-stops/case.yaml +++ b/plugins/implementation/evals/pb-regression-critical-stops/case.yaml @@ -8,12 +8,12 @@ graders: target: last_message - name: not-a-repeat type: regex - pattern: '(?:\b(?:not|no)\b|n''t\b)[^\n]{0,40}\brepeat|\brepeats?(?: a first[- ]call finding)?\b[^\n]{0,5}?:\s*no\b|\bnew (?:CRITICAL|finding|defect)\b|\bregression\b|\bintroduced (?:by|in) the fix\b|\bthe fix (?:commit )?(?:introduced|added)\b|\b(?:added|introduced) by (?:the )?fix(?: commit)?\b|\bdifferent (?:CRITICAL|finding|defect)\b' + pattern: '(?:\b(?:not|no)\b|n''t\b)\s+(?:\w+\s+)?repeats?\s+(?:a |the |any )?(?:first[- ]call|first review|original)\b|\brepeats?(?: a first[- ]call finding)?\b[^\n]{0,5}?:\s*no\b|(? Date: Mon, 19 Oct 2026 01:49:14 -0400 Subject: [PATCH 21/21] test(implementation): grade re-review repeat and further-round checks with an llm judge Three regex rounds could not settle two semantic judgments: whether the reply says the re-review CRITICAL is not the first call's finding, and whether the run itself plans another fix brief or review (steps for a person do not count). not-a-repeat, no-further-round and no-third-round are now llm graders with explicit pass and fail criteria; the blocking-human-decision and phase-not-done regexes stay. Graders move to graders/*.md, and the samples hold the reviewer's phrasings as judge calibration answers. Co-Authored-By: Claude Opus 5.5 --- .../pb-regression-critical-stops/case.yaml | 17 ----------- .../graders/blocking-human-decision.md | 6 ++++ .../graders/no-further-round.md | 15 ++++++++++ .../graders/not-a-repeat.md | 11 ++++++++ .../samples/no-further-round.json | 28 +++++++++++++++++++ .../samples/not-a-repeat.json | 12 ++++++++ .../case.yaml | 18 ------------ .../graders/blocking-human-decision.md | 6 ++++ .../graders/no-third-round.md | 15 ++++++++++ .../graders/phase-not-done.md | 7 +++++ .../samples/no-third-round.json | 28 +++++++++++++++++++ 11 files changed, 128 insertions(+), 35 deletions(-) create mode 100644 plugins/implementation/evals/pb-regression-critical-stops/graders/blocking-human-decision.md create mode 100644 plugins/implementation/evals/pb-regression-critical-stops/graders/no-further-round.md create mode 100644 plugins/implementation/evals/pb-regression-critical-stops/graders/not-a-repeat.md create mode 100644 plugins/implementation/evals/pb-repeated-critical-escalates-once/graders/blocking-human-decision.md create mode 100644 plugins/implementation/evals/pb-repeated-critical-escalates-once/graders/no-third-round.md create mode 100644 plugins/implementation/evals/pb-repeated-critical-escalates-once/graders/phase-not-done.md diff --git a/plugins/implementation/evals/pb-regression-critical-stops/case.yaml b/plugins/implementation/evals/pb-regression-critical-stops/case.yaml index 2e7d02ea95..5b7e80ee75 100644 --- a/plugins/implementation/evals/pb-regression-critical-stops/case.yaml +++ b/plugins/implementation/evals/pb-regression-critical-stops/case.yaml @@ -1,19 +1,2 @@ schema_version: "1.1" name: pb-regression-critical-stops -graders: - - name: blocking-human-decision - type: regex - pattern: 'human[- ]decision[\s\S]{0,200}?(?