skills: optional rule-selector slot for the review engines

docs/superpowers/specs/2026-10-28-review-rule-selector-design.md

@@ -0,0 +1,111 @@
+# Review rule selector: design
+
+Status: approved for planning, 2026-10-28.
+
+## Why
+
+A pack can bind review criteria, but criteria are prose every reviewer reads
+whatever the diff touches. Some teams keep their review rules as structured
+files that a command can select for a given diff (only the rules whose scope
+and conditions match, with the lines that triggered them) and can later check
+a reviewer's answers against. The shared review engines gain an optional slot
+for such a command, so a pack can hand the reviewer only the rules that apply,
+get a verdict for each, and turn every failed rule into a finding.
+
+The engines stay tool-neutral: they know only the command lines a fill
+resolves to and the JSON shapes below, never which tool sits behind them.
+
+## The slot
+
+A new prop `rule-selector` with contract `rule-selector@1`, `merge: replace`,
+`default: ""`, declared in its own block in each of `review`, `self-review`
+and `receive-review`. Unbound (the default), every step below is skipped and
+the engines behave exactly as today.
+
+### rule-selector@1
+
+A fill's text, followed at run time, resolves to one of:
+
+- the single line `selector: none`; or
+- three lines:
+  - `selector: <command line>` holding the placeholder `{format}`;
+  - `verdicts: <command line>` holding `{selection}` and `{verdicts}`;
+  - `fence: <info string>`: the tag of the fenced code block the reviewer
+    returns its verdict set in.
+
+The fill may read settings or the environment to decide (a team switch that
+turns rules off resolves to `selector: none`), so the resolution happens on
+every run, not at compile time.
+
+**The selector.** Run twice, `{format}` replaced by `json` and then `text`,
+each time with the change's unified diff on stdin; stdout is captured. The
+JSON is the selection; the engine reads only `rules[]`, each with `id`,
+`severity` (`blocking` or `advisory`) and `summary`. The text is Markdown for
+the reviewer and goes into the dispatch as is; it asks for the verdict set.
+A selector that exits non-zero, prints JSON the engine cannot read, or a fill
+that resolves to neither shape is a setup observation, quoted as such, and the
+review goes on without rules. It never blocks a review.
+
+**The verdict set** the reviewer returns, one fenced block tagged with the
+`fence` value: `{ "verdicts": [{ "rule", "verdict": "pass" | "fail" |
+"not-applicable", "evidence": [{ "file", "line", "side"?: "new" | "old" }],
+"note" }] }`, other fields allowed.
+
+**The verdicts command** prints `{ "problems": [...] }` and exits 0 when there
+are none, 1 when there are some; any other exit is a failure, treated as one
+problem.
+
+## The steps
+
+### review and self-review
+
+1. **Select, before dispatch.** Resolve the fill; with a selector, run it
+   (json, then text). The JSON goes to `selection.json` in the review scratch
+   dir (made here, and reused by every later step that writes there), the text
+   goes into the dispatch payload, and the reviewer's shape asks it to end its
+   reply with the verdict set in the `fence` block.
+2. **Check verdicts, after the reviewer returns.** Extract the fenced block to
+   `verdict-set.json` (a missing or unparseable block is one problem) and run
+   the verdicts command. Problems go to one verdict-only repair: a fresh
+   reviewer gets the selection text, the first verdict set and findings, and
+   the problems, and returns a corrected verdict set only. The first review's
+   findings stay; only the verdict set is replaced. The repair is charged to
+   the budget `verdict-rounds:<depth>`. Problems left after it are listed in
+   the draft's notes, the run goes on, and rules with no valid verdict are
+   reported as unanswered.
+3. **Fails become findings.** Each `fail` becomes a finding titled with the
+   rule id and its summary, anchored at its first `new`-side evidence (an
+   `old`-side location goes in the body): a `blocking` rule's fail is
+   Important, an `advisory` rule's is Minor. Critical stays the reviewer's own
+   judgment. A fail the reviewer also reported as a finding at the same
+   location is merged into one. In review this runs after "Assemble the review
+   draft" and before "Verify each blocking finding", so the verify and demote
+   steps apply unchanged; in self-review it runs before "Assemble the
+   self-review draft".
+4. **Save the verdict set** as `review-verdicts.json` in the review scratch
+   dir always, and next to the report json when a report path was handed in.
+
+### receive-review
+
+Only step 1, before the adjudicator is dispatched, with the diff of the
+branch against its target; the selection text goes to the adjudicator as
+reference for judging threads, with no verdict request.
+
+## Decisions
+
+- The `fence` line is part of the contract so the engine never hard-codes a
+  tool's block name.
+- `verdict-rounds:<depth>` uses limit 2 with the engines' existing
+  charge-then-check pattern (charge on the problem event, repair while not
+  spent), which yields exactly one repair, as `verify-rounds:<depth>` limit 2
+  yields one re-dispatch.
+- The selector reads only the diff; it needs no checkout, so it runs on every
+  path into the reviewer dispatch, including the paths where provisioning the
+  head checkout failed.
+- No new MCP tool or app capability: the engine floor (`minEngine`) stays.
+
+## Non-goals
+
+- Fan-out to one reviewer per rule.
+- Changing what the generic review judges; rule verdicts come in addition.
+- Shipping any selector. A pack's fill supplies the command.

lib/skills/__tests__/fixtures/rule-selector/.claude-plugin/plugin.json

@@ -0,0 +1 @@
+{"name":"acme","version":"0.1.0"}

lib/skills/__tests__/fixtures/rule-selector/attachments/acme-rule-selector/SKILL.md

@@ -0,0 +1,16 @@
+---
+name: acme-rule-selector
+description: "Selects the acme review rules that apply to a diff. Loaded by the review verbs; not for direct invocation."
+disable-model-invocation: true
+metadata:
+  provides: rule-selector@1
+---
+
+# acme rule selector
+
+Resolve to `selector: none` when the `ACME_RULES` environment variable is
+`off`. Otherwise resolve to these three lines:
+
+selector: acme-rules select --format {format}
+verdicts: acme-rules verdicts --selection {selection} --verdicts {verdicts}
+fence: acme-verdicts

lib/skills/__tests__/fixtures/rule-selector/attachments/acme-wrong-contract/SKILL.md

@@ -0,0 +1,13 @@
+---
+name: acme-wrong-contract
+description: "A fill that provides another contract. Loaded by the review verbs; not for direct invocation."
+disable-model-invocation: true
+metadata:
+  provides: review-criteria@1
+---
+
+# acme criteria
+
+## Requirements
+
+The linked ticket's acceptance criteria.

lib/skills/__tests__/fixtures/rule-selector/pack/skills.jsonc

@@ -0,0 +1,8 @@
+{
+  "version": 2,
+  "bindings": {
+    "mattstack:review/rule-selector": { "rule-selector": "acme:acme-rule-selector" },
+    "mattstack:self-review/rule-selector": { "rule-selector": "acme:acme-rule-selector" },
+    "mattstack:receive-review/rule-selector": { "rule-selector": "acme:acme-rule-selector" }
+  }
+}

lib/skills/__tests__/fixtures/rule-selector/pack/stubs.jsonc

@@ -0,0 +1,7 @@
+{
+  "verbs": {
+    "review": { "engine": "review", "description": "Review a teammate's MR" },
+    "self-review": { "engine": "self-review", "description": "Self-review the current branch" },
+    "receive-review": { "engine": "receive-review", "description": "Act on review feedback on your MR" }
+  }
+}

lib/skills/__tests__/rule-selector.test.ts

@@ -0,0 +1,151 @@
+import { afterAll, afterEach, beforeAll, describe, expect, test } from "bun:test";
+import { cpSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from "fs";
+import { tmpdir } from "os";
+import { join, resolve } from "path";
+import { skillsCompile } from "../../../commands/skills.ts";
+import { setFieldContractSeverityForTest } from "../field-contract.ts";
+import { captureSkills, runExpectingCleanExit } from "./helpers.ts";
+
+const FIX = join(import.meta.dir, "fixtures", "rule-selector");
+const REPO_PLUGIN = resolve(import.meta.dir, "..", "..", "..", "plugins", "mattstack");
+const ENGINES = ["review", "self-review", "receive-review"] as const;
+const FILL_LINE = "fence: acme-verdicts";
+// A fill's seam names the engine's block and the prop it fills.
+const seam = (verb: string) => `<!-- part: prop:${verb}/rule-selector.rule-selector `;
+const SELECT = /^#+ Select the rules$/m;
+const FAILS_TO_FINDINGS = /^#+ Failed rules become findings$/m;
+
+type Compiled = Record<(typeof ENGINES)[number], string>;
+
+let root = "";
+let seq = 0;
+
+function writeBindings(pack: string, bindings: Record<string, Record<string, string>>): void {
+  writeFileSync(join(pack, "pack", "skills.jsonc"), JSON.stringify({ version: 2, bindings }, null, 2));
+}
+
+/** A scratch copy of the fixture pack, `shape` applied, and a mattstack home that points at this repo's real plugin. */
+function stagePack(shape?: (pack: string) => void): { pack: string; compileArgs: string[] } {
+  const name = `pack-${seq++}`;
+  const pack = join(root, name);
+  cpSync(FIX, pack, { recursive: true });
+  shape?.(pack);
+  const ms = join(root, `${name}-home`);
+  mkdirSync(join(ms, "plugins"), { recursive: true });
+  symlinkSync(REPO_PLUGIN, join(ms, "plugins", "mattstack"));
+  return { pack, compileArgs: ["--pack-dir", pack, "--mattstack-dir", ms, "--manifest", join(pack, "pack", "skills.jsonc")] };
+}
+
+async function compilePack(shape?: (pack: string) => void): Promise<{ pack: string; verbs: Compiled }> {
+  const { pack, compileArgs } = stagePack(shape);
+  const io = captureSkills();
+  try {
+    await skillsCompile(compileArgs);
+  } finally {
+    io.restore();
+  }
+  const read = (verb: string) => {
+    const side = existsSync(join(pack, "skills", verb)) ? "skills" : "attachments";
+    return readFileSync(join(pack, side, verb, "SKILL.md"), "utf8");
+  };
+  return { pack, verbs: Object.fromEntries(ENGINES.map((v) => [v, read(v)])) as Compiled };
+}
+
+describe("rule selector slot", () => {
+  beforeAll(() => {
+    root = mkdtempSync(join(tmpdir(), "rt-rule-selector-"));
+    // The fixture is a partial pack: the review engines read fields no stage writes here.
+    setFieldContractSeverityForTest("warn");
+  });
+  afterEach(() => {
+    process.exitCode = 0;
+  });
+  afterAll(() => {
+    setFieldContractSeverityForTest(null);
+    rmSync(root, { recursive: true, force: true });
+  });
+
+  describe("a pack that binds a selector", () => {
+    let compiled: { pack: string; verbs: Compiled };
+    beforeAll(async () => {
+      compiled = await compilePack();
+    });
+
+    test("bound: each review engine inlines the fill under the rule-selector prop", () => {
+      for (const verb of ENGINES) {
+        const body = compiled.verbs[verb];
+        expect(body).toContain(FILL_LINE);
+        expect(body).toContain(seam(verb));
+      }
+    });
+
+    test("bound: review and self-review include both bodies, receive-review only the select body", () => {
+      for (const verb of ENGINES) expect(compiled.verbs[verb]).toMatch(SELECT);
+      expect(compiled.verbs.review).toMatch(FAILS_TO_FINDINGS);
+      expect(compiled.verbs["self-review"]).toMatch(FAILS_TO_FINDINGS);
+      expect(compiled.verbs["receive-review"]).not.toMatch(FAILS_TO_FINDINGS);
+    });
+
+    test("review graph places the rule steps", () => {
+      const body = compiled.verbs.review;
+      const edge = (a: string, b: string) => new RegExp(`\\b${a} -> ${b}\\b`);
+      expect(body).toMatch(edge("select_rules", "dispatch_reviewer"));
+      expect(body).toMatch(edge("dispatch_reviewer", "check_verdicts"));
+      expect(body).toMatch(edge("assemble_draft", "fails_to_findings"));
+      expect(body).toMatch(edge("fails_to_findings", "verify_blocking"));
+      expect(body).not.toMatch(edge("dispatch_reviewer", "assemble_draft"));
+      expect(body).toMatch(edge("redispatch_reviewer", "assemble_draft"));
+      expect(body).toContain("run_budget {runDb, key: verdict-rounds:<depth>, limit: 2, stage}");
+    });
+
+    test("review graph: every path to the dispatch passes select_rules, a failed head checkout included", () => {
+      const into = [...compiled.verbs.review.matchAll(/^\s*(\w+) -> dispatch_reviewer\b/gm)].map((m) => m[1]);
+      expect(into).toEqual(["select_rules"]);
+    });
+
+    test("self-review graph places the rule steps", () => {
+      const body = compiled.verbs["self-review"];
+      const edge = (a: string, b: string) => new RegExp(`\\b${a} -> ${b}\\b`);
+      expect(body).toMatch(edge("run_depth_checks", "select_rules"));
+      expect(body).toMatch(edge("select_rules", "repro_browser"));
+      expect(body).toMatch(edge("dispatch_reviewer", "check_verdicts"));
+      expect(body).toMatch(edge("fails_to_findings", "assemble_draft"));
+      expect(body).not.toMatch(edge("run_depth_checks", "repro_browser"));
+      expect(body).not.toMatch(edge("dispatch_reviewer", "assemble_draft"));
+      expect(body).toMatch(edge("make_recorded_move", "assemble_draft"));
+      expect(body).toContain("run_budget {runDb, key: verdict-rounds:<depth>, limit: 2, stage}");
+    });
+    test("receive-review: every edge into the adjudicator passes select_rules", () => {
+      const into = [...compiled.verbs["receive-review"].matchAll(/^\s*(\w+) -> dispatch_adjudicator\b/gm)].map((m) => m[1]);
+      expect(into).toEqual(["select_rules"]);
+    });
+
+    test("receive-review selects rules before the adjudicator", () => {
+      const body = compiled.verbs["receive-review"];
+      expect(body).toMatch(/\bany_unresolved -> select_rules\b/);
+      expect(body).toMatch(/\bselect_rules -> dispatch_adjudicator\b/);
+      expect(body).toMatch(/\bcode_changes_answer -> select_rules\b/);
+      expect(body).not.toMatch(/\bany_unresolved -> dispatch_adjudicator\b/);
+      expect(body).not.toMatch(/\bcode_changes_answer -> dispatch_adjudicator\b/);
+      expect(body).not.toMatch(/\bcheck_verdicts\b/);
+    });
+  });
+
+  test("unbound: no fill text, bodies still present", async () => {
+    const unbound = await compilePack((pack) => writeBindings(pack, {}));
+    for (const verb of ENGINES) {
+      const body = unbound.verbs[verb];
+      expect(body).not.toContain("acme-verdicts");
+      expect(body).toMatch(SELECT);
+    }
+  });
+
+  test("a fill providing another contract is refused", async () => {
+    const { compileArgs } = stagePack((pack) =>
+      writeBindings(pack, { "mattstack:review/rule-selector": { "rule-selector": "acme:acme-wrong-contract" } }),
+    );
+    const { exitCode, errors } = await runExpectingCleanExit(() => skillsCompile(compileArgs));
+    expect(exitCode).toBe(1);
+    expect(errors.join("\n")).toContain('provides "review-criteria@1", but the prop\'s contract is "rule-selector@1"');
+  });
+});

plugins/mattstack/.claude-plugin/plugin.json

@@ -1,6 +1,6 @@
 {
   "name": "mattstack",
-  "version": "0.32.29",
+  "version": "0.32.30",
   "description": "The mattstack skills pack: public doors subagent-review-loop, model-tiering, editing-skills, creating-a-pack, extending-a-pack, getting-current-time, process-digraphs, work-progress, wrap-up, the pack's own compiled shepherdr verb (domain-unbound), and the three writing-style presets (sparse, conversational, structured), plus the compile-native engines (work orchestrator, pipeline stages, review verbs, forge adapters, shepherdr) and attachments (execution strategies, account pool, review include bodies, review-posting, parameterized-skill primitive) that rt skills compile inlines into team verbs at compile time -- reached by name, not by slash menu. Also ships an mcp tools reference read by path, not inlined.",
   "author": {
     "name": "Matthew Goodwin"

plugins/mattstack/CERTIFICATION.md

@@ -244,5 +244,6 @@ that quotes a shell form carries `<!-- mcp-lint: allow -->`.
 | 2026-10-20 | mattstack:review, mattstack:review-posting, mattstack:review-core-body-tail | pure | pass | n/a (descriptions unchanged) | the summary line follows the verdict: the review verb drafts `summary.comments`, one line per `outcome` option value the gate offers (`comment`, `approve`, and `request_changes` only on GitHub), only the `approve` line carrying an all-clear; `editable` needs a `comment` on every finding and exactly those keys; Compose posts the `outcome` answer's `text` (the caller's `summaryText`), else `summary.comments[<picked outcome>]`, the join unchanged; review-posting and the structured-findings field docs name `summary.comments`. RED 4/15 on V1 to V5 (3 reps each; every miss took the no-drafts branch and composed the summary, some with an all-clear), D2 0/3 (singular `summary.comment` with "Nothing here blocks the merge" for every verdict); GREEN 5/5 on V1 to V5, D1 and D2, G1 and G2 5/5, G3 9/10 (`docs/superpowers/tests/2026-10-20-review-drafted-comments/red-green.md`); certify exit 0 on all three |
 | 2026-10-23 | mattstack:work-progress | pure | pass | 5/5 scenarios, 5/5 haiku reps each (2 herd kept, epic and 2 plan added); full roster 22/23, the one miss (billing creating-a-pack) also misses on the old description | replaces the herd-only progress skill: the bundled script renders a herd (`--herd`) or plans and epics (`--plan` repeatable, `--title`, `--tree`) from rt's plan records; the skill picks the input, prints the view verbatim and never calls a plan done unless its row says so; RED: no skill loaded on plan or epic prompts, 6 hand calls and a hand-built table, plan 1 guessed done from plan 2 having started; GREEN (claude -p --plugin-dir, opus, sandboxed rt): epic prompt Skill + ls + 1 script call, plan "where is my plan at" and "how far along is it" Skill + 1 script call each, all printed verbatim, "herd progress" triggers it; the brief's description first missed "where is my plan at", fixed in 15e6d8f by adding that phrase; certify exit 0 |
 | 2026-10-23 | mattstack:shepherdr | pure | pass | n/a (description unchanged) | Progress pointer renamed to mattstack:work-progress, recompiled into skills/shepherdr; certify exit 0 |
+| 2026-10-28 | mattstack:review, mattstack:self-review, mattstack:receive-review | pure | pass | 4/4 GREEN walkthrough scenarios | optional `rule-selector` prop (`rule-selector@1`, replace, default empty): the engines select written rules before dispatch, ask the reviewer for one verdict per rule and turn failed rules into findings; unbound it is a no-op; GREEN describe-only walk of the compiled review: unbound skips every rule step, a selector exiting 1 is one setup observation and no rule steps, a reply with no verdict block gets exactly one repair then an `Unanswered rules:` line in the Assessment, an empty selection sends no verdict request and saves an empty verdict file; certify exit 0 on every touched root |
 
 (Ledger rows are appended by the orchestrating session as each lane lands -- lanes themselves never edit this file.)