[pipeline-dcdad92d][slice-6/12] Roster framework: always-on...

.changeset/review-roster-framework.md

@@ -0,0 +1,5 @@
+---
+"review": minor
+---
+
+Restructure the reviewer roster: five always-on reviewers (holistic, completeness, test-adequacy, first-principles, conventions) with explicit trust and advisory constraints and named mandates; launch-default model and effort assignments per role (Opus 4.8 workhorse, xhigh for the security lens and claim validation, Fable 5 for first-principles); and the kept gates (pattern-triage, claim-validator plus refuter panel, deterministic dedup, verdict bookends, thread-reconciler) wired to the new roster.

workflows/review/README.md

@@ -86,15 +86,22 @@ rule per line:
 
 ```
 # <pattern> [lens=<lens>,…] [tier=trivial|low|medium|high] [direction-dependent]
+# enable <reviewer>[,<reviewer>…]
 services/**/migrations/**  tier=high lens=data-migrations
 **/*.graphql               lens=api-federation-compat
 pkg/auth/**                tier=high direction-dependent lens=security-auth
 services/**/testdata/**    tier=trivial
 docs/**                    tier=trivial
+enable holistic,test-adequacy
 ```
 
 - `lens=` names the specialist lenses to spawn when the pattern is touched; when
   several rules match a path their lenses are unioned (lenses are additive).
+- `enable` lines turn on the opt-in whole-change reviewers (`holistic`,
+  `completeness`, `test-adequacy`, `first-principles`, `conventions`). Neither
+  lenses nor opt-in reviewers run anywhere by default: a repo opts into each
+  explicitly, and the policy is that a reviewer earns its line here through the
+  eval suite.
 - `tier=` assigns the path a risk tier. When several rules match, the **last
   matching rule in file order wins** (gitignore/CODEOWNERS-style): write the broad
   rule first and its exceptions after it, as with `services/**` and
@@ -114,6 +121,34 @@ and are skipped; routing degrades to fewer lenses, never to a crashed review.
 stays the model-facing prose about file *contents*; team ownership stays in
 `.github/REVIEWERS`, unchanged.
 
+### Models and effort per role
+
+Each sub-agent pins its model in its own definition inside `review.md` (with a
+launch-default `effort:` annotation; the gh-aw Claude engine exposes no per-agent
+effort field yet). The orchestrator prompt deliberately says nothing about
+sub-agent models — this table is the human-facing summary:
+
+| Role | Model | Effort | Why |
+| --- | --- | --- | --- |
+| orchestrator | `claude-opus-4-8` | high | Owns every GitHub/safe-output decision |
+| `pattern-triage` | `claude-sonnet-4-6` | medium | Cheap first-pass triage |
+| `thread-reconciler` | `claude-opus-4-8` | medium | Reconciliation |
+| `correctness-reviewer` | `claude-opus-4-8` | high | Whole-change reviewer |
+| `skill-auditor` | `claude-opus-4-8` | high | Whole-change reviewer |
+| `holistic` | `claude-opus-4-8` | high | Opt-in whole-change reviewer (`enable` in `ROUTING`) |
+| `completeness` | `claude-opus-4-8` | high | Opt-in whole-change reviewer (`enable` in `ROUTING`) |
+| `test-adequacy` | `claude-opus-4-8` | high | Opt-in whole-change reviewer (`enable` in `ROUTING`) |
+| `conventions` | `claude-opus-4-8` | medium | Opt-in advisory targeted check (`enable` in `ROUTING`) |
+| `first-principles` | `claude-fable-5` | high | Opt-in advisory-only; reviews the change's justification |
+| `claim-validator` | `claude-opus-4-8` | xhigh | Adversarial claim validation |
+| specialist lenses | `claude-opus-4-8` | high | Opt-in via `lens=` in `ROUTING`; the security & auth lens is xhigh |
+
+Only the orchestrator and the default roster (`pattern-triage`,
+`correctness-reviewer`, `skill-auditor`, `thread-reconciler`, `claim-validator`)
+run by default; every other row is opt-in via `ROUTING` and earns its line through
+the eval suite. Per-role Fable-5 / Sonnet experiment arms are eval-suite
+measurements to run after the suite exists.
+
 ### Required secrets / variables
 
 - `ANTHROPIC_API_KEY` — used by the `claude` engine.

workflows/review/lib/finding-schema.ts

@@ -47,11 +47,15 @@ export const KNOWN_LENSES = [
     "deploy-infra-config",
     "money-payments",
     "content-i18n",
-    // Always-on / whole-change reviewers and triage.
+    // Whole-change reviewers (the default correctness pass plus the opt-in
+    // reviewers a repo can `enable` in its ROUTING file) and triage.
     "correctness",
     "conventions",
     "pattern-triage",
     "first-principles",
+    "holistic",
+    "completeness",
+    "test-adequacy",
 ] as const;
 
 export type Lens = typeof KNOWN_LENSES[number];

workflows/review/lib/router.test.ts

@@ -755,3 +755,57 @@ describe("parseRoutingConfig", () => {
         expect(config.warnings.join("\n")).toContain("no lens= or tier=");
     });
 });
+
+describe("parseRoutingConfig: enable directives", () => {
+    it("collects enabled reviewers in canonical order, deduped", () => {
+        const config = parseRoutingConfig(
+            ["enable test-adequacy", "enable holistic,test-adequacy"].join(
+                "\n",
+            ),
+        );
+        expect(config.enabledReviewers).toEqual(["holistic", "test-adequacy"]);
+        expect(config.warnings).toEqual([]);
+    });
+
+    it("warns on an unknown reviewer and keeps the known ones", () => {
+        const config = parseRoutingConfig("enable holistic,skynet");
+        expect(config.enabledReviewers).toEqual(["holistic"]);
+        expect(config.warnings.join("\n")).toContain(
+            'unknown reviewer "skynet"',
+        );
+    });
+
+    it("warns on a bare enable line", () => {
+        const config = parseRoutingConfig("enable");
+        expect(config.enabledReviewers).toEqual([]);
+        expect(config.warnings.join("\n")).toContain("names no reviewer");
+    });
+
+    it("defaults to no enabled reviewers", () => {
+        expect(
+            parseRoutingConfig("docs/** tier=trivial").enabledReviewers,
+        ).toEqual([]);
+    });
+});
+
+describe("runCli: enabled reviewers", () => {
+    it("surfaces enable directives in routing.json", () => {
+        const {fs} = fakeFs({
+            ["/tmp/gh-aw/review/files.json"]: JSON.stringify([
+                {path: "a.ts", status: "modified"},
+            ]),
+            [ROUTING_CONFIG_PATH]: "enable holistic,first-principles",
+        });
+        const json = runCli(fs);
+        expect(json.enabledReviewers).toEqual(["holistic", "first-principles"]);
+    });
+
+    it("emits no enabled reviewers without a ROUTING config", () => {
+        const {fs} = fakeFs({
+            ["/tmp/gh-aw/review/files.json"]: JSON.stringify([
+                {path: "a.ts", status: "modified"},
+            ]),
+        });
+        expect(runCli(fs).enabledReviewers).toEqual([]);
+    });
+});

workflows/review/lib/router.ts

@@ -33,11 +33,13 @@
 import {KNOWN_LENSES} from "./finding-schema";
 import type {Lens} from "./finding-schema";
 import {
+    ENABLEABLE_REVIEWERS,
     parseRoutingConfig,
     RISK_TIERS,
     ROUTING_CONFIG_PATH,
 } from "./routing-config";
 import type {
+    EnableableReviewer,
     LensRule,
     RiskRule,
     RiskTier,
@@ -46,25 +48,41 @@ import type {
 
 // Re-exported so consumers (and the tests) can treat the router as the single
 // entry point for routing vocabulary and the ROUTING parser.
-export {parseRoutingConfig, RISK_TIERS, ROUTING_CONFIG_PATH};
-export type {LensRule, RiskRule, RiskTier, RoutingFileConfig};
+export {
+    ENABLEABLE_REVIEWERS,
+    parseRoutingConfig,
+    RISK_TIERS,
+    ROUTING_CONFIG_PATH,
+};
+export type {
+    EnableableReviewer,
+    LensRule,
+    RiskRule,
+    RiskTier,
+    RoutingFileConfig,
+};
 
 /* -------------------------------------------------------------------------- */
 /* Lens taxonomy                                                              */
 /* -------------------------------------------------------------------------- */
 
 /**
- * The always-on / whole-change reviewers and triage. These run every review
- * regardless of routing, so they are NOT part of the router's `lensesToSpawn`
- * (which names only the *specialist* lenses gated by touched paths). Kept here
- * as the complement of {@link SPECIALIST_LENSES} so the two lists cannot drift
- * from the canonical `KNOWN_LENSES`.
+ * The whole-change reviewers and triage: never path-gated, so NOT part of the
+ * router's `lensesToSpawn` (which names only the *specialist* lenses gated by
+ * touched paths). Whether each runs is a separate question — the default
+ * roster always does, and the opt-in ones ({@link ENABLEABLE_REVIEWERS}) run
+ * only when the repo's ROUTING file enables them (`enabledReviewers`). Kept
+ * here as the complement of {@link SPECIALIST_LENSES} so the two lists cannot
+ * drift from the canonical `KNOWN_LENSES`.
  */
 export const ALWAYS_ON_LENSES = [
     "correctness",
     "conventions",
     "pattern-triage",
     "first-principles",
+    "holistic",
+    "completeness",
+    "test-adequacy",
 ] as const;
 
 // `satisfies readonly Lens[]` is the natural spelling, but the repo's
@@ -761,6 +779,11 @@ export type RoutingJson = {
     perFileTier: Record<string, DisplayTier>;
     runBudget: RunBudget;
     pendingRiskQuestions: RiskQuestion[];
+    /**
+     * Opt-in whole-change reviewers the repo's `ROUTING` file enables
+     * (`enable <reviewer>` lines). Empty means the default roster only.
+     */
+    enabledReviewers: EnableableReviewer[];
     /**
      * Whether the consumer `ROUTING` file was found, plus any parse warnings
      * (or the missing-file warning). The orchestrator surfaces these in the
@@ -781,6 +804,7 @@ export type RoutingJson = {
 export const toRoutingJson = (
     result: RoutingResult,
     routingConfig: RoutingJson["routingConfig"] = {present: true, warnings: []},
+    enabledReviewers: EnableableReviewer[] = [],
 ): RoutingJson => {
     const owners: Record<string, string[]> = {};
     for (const file of result.perFile) {
@@ -800,6 +824,7 @@ export const toRoutingJson = (
         perFileTier,
         runBudget: result.runBudget,
         pendingRiskQuestions: result.pendingRiskQuestions,
+        enabledReviewers,
         routingConfig,
     };
 };
@@ -878,6 +903,7 @@ export const runCli = (fs: FsLike, repoRoot = "."): RoutingJson => {
         : {
               lensRules: [],
               riskRules: [],
+              enabledReviewers: [],
               warnings: [
                   `routing config missing (${ROUTING_CONFIG_PATH}): no ` +
                       `specialist lenses will run; always-on reviewers only ` +
@@ -903,10 +929,14 @@ export const runCli = (fs: FsLike, repoRoot = "."): RoutingJson => {
         lensRules: routingFileConfig.lensRules,
         riskRules: routingFileConfig.riskRules,
     });
-    const json = toRoutingJson(result, {
-        present: routingConfigPresent,
-        warnings: routingFileConfig.warnings,
-    });
+    const json = toRoutingJson(
+        result,
+        {
+            present: routingConfigPresent,
+            warnings: routingFileConfig.warnings,
+        },
+        routingFileConfig.enabledReviewers,
+    );
 
     fs.mkdirSync(REVIEW_DIR, {recursive: true});
     fs.writeFileSync(ROUTING_OUT, JSON.stringify(json, null, 2));

workflows/review/lib/routing-config.ts

@@ -44,10 +44,29 @@ export type RiskRule = {
 /** Where a consuming repo keeps its routing map, next to its review config. */
 export const ROUTING_CONFIG_PATH = ".github/aw/review/ROUTING";
 
+/**
+ * The opt-in whole-change reviewers a repo may `enable` in its ROUTING file.
+ * None run by default: each costs credits on every PR, so a repo turns one on
+ * only once the eval suite shows it earns its keep. (The default roster —
+ * correctness, skill audit, triage, reconciliation, validation — needs no
+ * enabling.)
+ */
+export const ENABLEABLE_REVIEWERS = [
+    "holistic",
+    "completeness",
+    "test-adequacy",
+    "first-principles",
+    "conventions",
+] as const;
+
+export type EnableableReviewer = typeof ENABLEABLE_REVIEWERS[number];
+
 /** Parsed `.github/aw/review/ROUTING` config. */
 export type RoutingFileConfig = {
     lensRules: LensRule[];
     riskRules: RiskRule[];
+    /** Opt-in whole-change reviewers this repo enables (canonical order). */
+    enabledReviewers: EnableableReviewer[];
     /** Fixed-format parse warnings (unknown lens/tier, no-op rule). */
     warnings: string[];
 };
@@ -56,9 +75,10 @@ const KNOWN_LENS_SET: ReadonlySet<string> = new Set(KNOWN_LENSES);
 
 /**
  * Parse the consumer-owned routing map. Line grammar, `REVIEWERS`-style —
- * blanks and `#` comments skipped, one rule per line:
+ * blanks and `#` comments skipped, one rule or directive per line:
  *
  *     <pattern> [lens=<lens>[,<lens>…]] [tier=trivial|low|medium|high] [direction-dependent]
+ *     enable <reviewer>[,<reviewer>…]
  *
  * `lens=` names specialist lenses to spawn when the pattern is touched (multiple
  * matching rules union their lenses). `tier=` assigns a risk tier; when several
@@ -68,14 +88,17 @@ const KNOWN_LENS_SET: ReadonlySet<string> = new Set(KNOWN_LENSES);
  * subtree beneath it). `direction-dependent` marks a tier
  * that cannot be finalised from the path alone (tightening vs. loosening; see
  * {@link RiskRule.diffDirectionDependent}) and requires `tier=`.
+ * `enable` turns on an opt-in whole-change reviewer
+ * ({@link ENABLEABLE_REVIEWERS}) for every review in this repo.
  *
- * Malformed fields and unknown lens names produce a warning and skip the lens or
- * line rather than aborting the run: routing degrades to fewer lenses, never to
- * a crashed review.
+ * Malformed fields and unknown lens/reviewer names produce a warning and skip
+ * the lens or line rather than aborting the run: routing degrades to fewer
+ * reviewers, never to a crashed review.
  */
 export const parseRoutingConfig = (content: string): RoutingFileConfig => {
     const lensRules: LensRule[] = [];
     const riskRules: RiskRule[] = [];
+    const enabled = new Set<EnableableReviewer>();
     const warnings: string[] = [];
 
     const lines = content.split(/\r?\n/);
@@ -87,6 +110,31 @@ export const parseRoutingConfig = (content: string): RoutingFileConfig => {
         const lineNo = index + 1;
         const [pattern, ...fields] = line.split(/\s+/);
 
+        if (pattern === "enable") {
+            const names = fields.flatMap((field) => field.split(","));
+            if (names.length === 0) {
+                warnings.push(
+                    `ROUTING line ${lineNo}: enable names no reviewer (line skipped)`,
+                );
+                continue;
+            }
+            for (const name of names) {
+                if (name === "") {
+                    continue;
+                }
+                if (
+                    (ENABLEABLE_REVIEWERS as readonly string[]).includes(name)
+                ) {
+                    enabled.add(name as EnableableReviewer);
+                } else {
+                    warnings.push(
+                        `ROUTING line ${lineNo}: unknown reviewer "${name}" (skipped)`,
+                    );
+                }
+            }
+            continue;
+        }
+
         const lenses = new Set<Lens>();
         let tier: RiskTier | undefined;
         let directionDependent = false;
@@ -154,5 +202,12 @@ export const parseRoutingConfig = (content: string): RoutingFileConfig => {
         }
     }
 
-    return {lensRules, riskRules, warnings};
+    return {
+        lensRules,
+        riskRules,
+        enabledReviewers: ENABLEABLE_REVIEWERS.filter((reviewer) =>
+            enabled.has(reviewer),
+        ),
+        warnings,
+    };
 };