review: a non-blocking inline budget, and the collapsed tail names its best finding

.changeset/pra7-posting-surface.md

@@ -0,0 +1,7 @@
+---
+"review": minor
+---
+
+A cap on how many non-blocking findings post as inline comments per review, plus a collapsed-section summary that names its top-ranked finding. Four posting-surface changes, all deterministic (no model behavior changes): at most 3 non-blocking findings post inline per review (the ROUTING `non-blocking-budget` line tunes it; blocking findings are uncapped up to the engine's 20), `nitpick (non-blocking)` never posts inline, documentation-label findings are exempt from the budget (the documentation autofix selects its work by parsing that label off posted threads, so budgeting them would silently shrink a shipped feature's scope), and the collapsed section's summary line now names its top-ranked entry's location, label, and subject instead of a bare count. The motivating case for the disclosure: three 2026-08-24 approving re-reviews on this repo collapsed correctness findings behind "Non-blocking observations (N)", including Khan/actions#367's report that the acknowledgment feature's own reply guard never fires. Nothing is dropped and the verdict still counts every validated claim; every shed is disclosed in the plan notes, per reason (budget, nitpick ban), and a non-default budget shows in the version footer.
+
+Expected output-shape effect: fewer inline comments per review (at most 3 non-blocking plus blocking, down from every claim at >=0.5 confidence up to 20; at the measured 2.91 findings/run the cap binds rarely), an unchanged median comment body, a slightly longer top comment or review body where a collapsed section now rides with its named-top summary line, and zero inline nitpick comments. The nitpick ban also means nitpick findings stop becoming threads, which removes them from the thread-sourced `autofix: nits` work list until the companion autofix change (the body-sourced work list, same release train) restores that reach.

workflows/autofix/README.md

@@ -16,7 +16,7 @@ Two ways to arm it, and they are peers. Neither is a shorthand for the other.
 | Label              | Fixes                                                      |
 | ------------------ | ---------------------------------------------------------- |
 | `autofix: blocking` | The reviewer's open blocking threads (`issue (blocking)`, `issue (blocking, best-practice)`, `todo (blocking)`) |
-| `autofix: nits`     | The reviewer's open non-blocking threads (suggestions, nitpicks, questions, thoughts, notes) |
+| `autofix: nits`     | The reviewer's open non-blocking threads (suggestions, nitpicks, questions, thoughts, notes). The reviewer's posting surface never posts `nitpick (non-blocking)` findings as inline threads (review-v1.20+, the non-blocking budget change), so nitpick-class items reach this scope only once the work list also reads the review body's collapsed section. |
 | `autofix: docs`     | Only the `documentation` reviewer's threads (`suggestion (non-blocking, documentation)`) — a subset of `nits`, see below |
 
 **Or comment on the PR:**

workflows/review/README.md

@@ -366,6 +366,7 @@ rule per line:
 # <pattern> [lens=<lens>,…] [tier=trivial|low|medium|high] [direction-dependent]
 # enable <reviewer>[,<reviewer>…]
 # re-review full|scoped|flip-gated|fast [blocking-only]
+# non-blocking-budget <n>
 services/**/migrations/**  tier=high lens=data-migrations
 **/*.graphql               lens=api-federation-compat
 pkg/auth/**                tier=high direction-dependent lens=security-auth
@@ -396,6 +397,17 @@ re-review scoped
   optional `blocking-only` modifier changes the repeat review's posting
   surface (see [Re-review modes](#re-review-modes-the-runs-per-pr-cost-lever));
   an unknown modifier warns and is ignored, and the mode still applies.
+- `non-blocking-budget` sets how many non-blocking findings may post as inline
+  comments per review (default 3). Blocking findings never count against it;
+  `nitpick (non-blocking)` findings never post inline at all; documentation
+  findings are exempt (the documentation reviewer self-caps, and its autofix
+  selects work by the label on posted threads). Findings over budget collapse
+  into a `<details>` block riding the top-ranked inline comment (or the review
+  body, when nothing posts inline or a reduced-depth modifier applies) whose
+  summary names the top-ranked entry; nothing is dropped and the verdict
+  counts every validated finding. A malformed value warns and keeps the
+  previous value (the default when no earlier line set one); when several
+  lines set it, the last one wins.
 
 Glob semantics are a practical subset of gitignore/CODEOWNERS: `**` crosses
 directories, `*` and `?` stay within a segment, a trailing `/` matches everything
@@ -875,14 +887,16 @@ run files (never composed by the model):
 
 ```
 <details><summary><sub>review details</sub></summary>
-<sub>review-v<major>.<minor>.<patch> | schema <n> | depth <depth> | re-review <mode> [blocking-only] | enable <reviewer,...></sub>
+<sub>review-v<major>.<minor>.<patch> | schema <n> | depth <depth> | re-review <mode> [blocking-only] | enable <reviewer,...> | non-blocking-budget <n></sub>
 </details>
 ```
 
 `schema` is the finding-schema version (`FINDING_SCHEMA_VERSION` in
 `lib/finding-schema.ts`) the run was on; `depth` is the EXECUTED re-review depth;
-the `re-review` and `enable` segments echo the repo's ROUTING configuration, so a
-posted review attributes both the release and the config it ran under. A segment
+the `re-review`, `enable`, and `non-blocking-budget` segments echo the repo's
+ROUTING configuration, so a posted review attributes both the release and the
+config it ran under (`non-blocking-budget` appears only at a non-default value:
+the footer states configuration, not defaults). A segment
 the staging cannot state is omitted rather than guessed. A bad reviewer release
 rolls back by re-pinning the previous tag; the footer on each posted review makes
 attribution immediate.

workflows/review/lib/attribution.ts

@@ -58,7 +58,7 @@ export const renderCollapsedFooter = (content: string): string =>
  * non-greedy match. Escape the HTML-significant characters; GitHub renders
  * the entities back as the literal characters.
  */
-const escapeHtml = (text: string): string =>
+export const escapeHtml = (text: string): string =>
     text.replace(/&/g, "&amp;").replace(/</g, "&lt;").replace(/>/g, "&gt;");
 
 const flaggedBy = (entry: AlsoFlagged): string => {

workflows/review/lib/render-comment.ts

@@ -52,12 +52,20 @@ export const BLOCKING_LABELS = [
  */
 export const DOCUMENTATION_LABEL = "suggestion (non-blocking, documentation)";
 
+/**
+ * The nitpick label. Named for the same reason {@link DOCUMENTATION_LABEL}
+ * is: it is a selection key, not only a description — the posting surface
+ * (`submission.ts`) never posts nitpick-class findings inline, so the string
+ * is imported rather than re-spelled downstream.
+ */
+export const NITPICK_LABEL = "nitpick (non-blocking)";
+
 /** Every other Conventional-Comment label; none of these block. */
 export const NON_BLOCKING_LABELS = [
     "suggestion (non-blocking)",
     "suggestion (non-blocking, best-practice)",
     DOCUMENTATION_LABEL,
-    "nitpick (non-blocking)",
+    NITPICK_LABEL,
     "question (non-blocking)",
     "thought (non-blocking)",
     "note (non-blocking)",

workflows/review/lib/router-non-blocking-budget.test.ts

@@ -0,0 +1,111 @@
+import {describe, it, expect} from "vitest";
+
+import {runCli} from "./router";
+import {parseRoutingConfig, ROUTING_CONFIG_PATH} from "./routing-config";
+
+/**
+ * The `non-blocking-budget <n>` directive's parse and CLI wiring, split from
+ * router.test.ts for its max-lines budget (the router-rereview-blocking-only
+ * precedent). The posting-surface behavior the number drives lives in
+ * submission-trial-followups.test.ts; here we pin only that the ROUTING line
+ * parses and that `routing.json` carries the value to the submission CLI.
+ * The fs fixture is a small local copy of router.test.ts's.
+ */
+
+const fakeFs = (inputs: Record<string, string>) => {
+    const written: Record<string, string> = {};
+    const fs = {
+        readFileSync: (p: string, _enc: "utf8"): string => {
+            const content = inputs[p];
+            if (content === undefined) {
+                throw new Error(`unexpected read: ${p}`);
+            }
+            return content;
+        },
+        writeFileSync: (p: string, data: string): void => {
+            written[p] = data;
+        },
+        existsSync: (p: string): boolean =>
+            p in inputs ||
+            Object.keys(inputs).some((key) => key.startsWith(`${p}/`)),
+        mkdirSync: (_p: string, _opts: {recursive: boolean}): void => {},
+        readdirSync: (p: string): string[] => {
+            if (p in inputs) {
+                throw new Error(`ENOTDIR: not a directory, scandir '${p}'`);
+            }
+            const prefix = p.endsWith("/") ? p : `${p}/`;
+            const names = new Set<string>();
+            for (const key of Object.keys(inputs)) {
+                if (key.startsWith(prefix)) {
+                    names.add(key.slice(prefix.length).split("/")[0]);
+                }
+            }
+            return [...names];
+        },
+    };
+    return {fs, written};
+};
+
+describe("parseRoutingConfig: non-blocking-budget directive", () => {
+    it("defaults to 3", () => {
+        expect(
+            parseRoutingConfig("docs/** tier=trivial").nonBlockingInlineBudget,
+        ).toBe(3);
+    });
+
+    it("parses a non-negative integer, zero included", () => {
+        expect(
+            parseRoutingConfig("non-blocking-budget 5").nonBlockingInlineBudget,
+        ).toBe(5);
+        expect(
+            parseRoutingConfig("non-blocking-budget 0").nonBlockingInlineBudget,
+        ).toBe(0);
+    });
+
+    it("warns on a malformed value and keeps the default", () => {
+        for (const value of ["three", "-1", "2.5"]) {
+            const config = parseRoutingConfig(`non-blocking-budget ${value}`);
+            expect(config.nonBlockingInlineBudget).toBe(3);
+            expect(config.warnings.join("\n")).toContain(
+                "non-negative integer",
+            );
+        }
+    });
+
+    it("skips a line with the wrong arity", () => {
+        const config = parseRoutingConfig("non-blocking-budget 2 4");
+        expect(config.nonBlockingInlineBudget).toBe(3);
+        expect(config.warnings.join("\n")).toContain("exactly one number");
+    });
+
+    it("lets the last of duplicate lines win, with a warning", () => {
+        const config = parseRoutingConfig(
+            "non-blocking-budget 5\nnon-blocking-budget 2",
+        );
+        expect(config.nonBlockingInlineBudget).toBe(2);
+        expect(config.warnings.join("\n")).toContain(
+            "duplicate non-blocking-budget",
+        );
+    });
+});
+
+describe("runCli: non-blocking budget", () => {
+    it("surfaces the configured budget in routing.json", () => {
+        const {fs} = fakeFs({
+            ["/tmp/gh-aw/review/files.json"]: JSON.stringify([
+                {path: "a.ts", status: "modified"},
+            ]),
+            [ROUTING_CONFIG_PATH]: "non-blocking-budget 1",
+        });
+        expect(runCli(fs).nonBlockingInlineBudget).toBe(1);
+    });
+
+    it("defaults to 3 without a ROUTING config", () => {
+        const {fs} = fakeFs({
+            ["/tmp/gh-aw/review/files.json"]: JSON.stringify([
+                {path: "a.ts", status: "modified"},
+            ]),
+        });
+        expect(runCli(fs).nonBlockingInlineBudget).toBe(3);
+    });
+});

workflows/review/lib/router.ts

@@ -44,6 +44,7 @@ import {
 } from "./lens-payloads";
 import {
     DEFAULT_DISPATCH_MODE,
+    DEFAULT_NON_BLOCKING_INLINE_BUDGET,
     DEFAULT_RE_REVIEW_MODE,
     ENABLEABLE_REVIEWERS,
     parseRoutingConfig,
@@ -65,6 +66,7 @@ import type {
 // entry point for routing vocabulary and the ROUTING parser.
 export {DEFAULT_MISROUTED_FLOOR_TIER, DEFAULT_TIER_BUDGETS};
 export {
+    DEFAULT_NON_BLOCKING_INLINE_BUDGET,
     DEFAULT_RE_REVIEW_MODE,
     ENABLEABLE_REVIEWERS,
     parseRoutingConfig,
@@ -752,6 +754,12 @@ export type RoutingJson = {
      * collapse into the review body (`submission.ts` reads this).
      */
     reReviewBlockingOnly: boolean;
+    /**
+     * `non-blocking-budget` line in `ROUTING` (default 3): how many
+     * non-blocking findings may post inline per review; the overflow
+     * collapses into the review body (`submission.ts` reads this).
+     */
+    nonBlockingInlineBudget: number;
     /**
      * The repo's dispatch mode (`dispatch` line in `ROUTING`; `task` when
      * absent). `scripted` opts the repo into the deterministic dispatcher
@@ -782,6 +790,7 @@ export const toRoutingJson = (
     reReviewMode: ReReviewMode = DEFAULT_RE_REVIEW_MODE,
     dispatchMode: DispatchMode = DEFAULT_DISPATCH_MODE,
     reReviewBlockingOnly = false,
+    nonBlockingInlineBudget: number = DEFAULT_NON_BLOCKING_INLINE_BUDGET,
 ): RoutingJson => {
     const owners: Record<string, string[]> = {};
     const generatedFiles: string[] = [];
@@ -808,6 +817,7 @@ export const toRoutingJson = (
         enabledReviewers,
         reReviewMode,
         reReviewBlockingOnly,
+        nonBlockingInlineBudget,
         dispatchMode,
         routingConfig,
     };
@@ -903,6 +913,7 @@ export const runCli = (
               enabledReviewers: [],
               reReviewMode: DEFAULT_RE_REVIEW_MODE,
               reReviewBlockingOnly: false,
+              nonBlockingInlineBudget: DEFAULT_NON_BLOCKING_INLINE_BUDGET,
               dispatchMode: DEFAULT_DISPATCH_MODE,
               warnings: [
                   `routing config missing (${ROUTING_CONFIG_PATH}): no ` +
@@ -968,6 +979,7 @@ export const runCli = (
         routingFileConfig.reReviewMode,
         routingFileConfig.dispatchMode,
         routingFileConfig.reReviewBlockingOnly,
+        routingFileConfig.nonBlockingInlineBudget,
     );
 
     fs.mkdirSync(REVIEW_DIR, {recursive: true});

workflows/review/lib/routing-config.ts

@@ -107,6 +107,25 @@ export const DEFAULT_RE_REVIEW_MODE: ReReviewMode = "full";
  */
 export const RE_REVIEW_MODIFIERS = ["blocking-only"] as const;
 
+/**
+ * How many non-blocking findings may post as inline comments per review (the
+ * P1 comment budget). Blocking findings never count against it, and two label
+ * classes sit outside it: `nitpick (non-blocking)` never posts inline at all,
+ * and the documentation label is exempt (the documentation reviewer
+ * self-caps at five per review, and the documentation autofix selects its
+ * work by parsing that label off posted threads, so collapsing those would
+ * silently empty the autofix scope). Everything over budget collapses into
+ * the review body's <details> block; nothing is dropped and the verdict
+ * still counts every claim.
+ *
+ * 3 is set by fiat (the quiet-the-human-surface lane's Q8 decision): at the
+ * measured 2.91 findings/run it binds rarely and acts as a backstop against
+ * the wall-of-comments failure mode (webapp#41440: 13 non-blocking inline
+ * comments in one review). Consumers tune it with a `non-blocking-budget`
+ * line in ROUTING.
+ */
+export const DEFAULT_NON_BLOCKING_INLINE_BUDGET = 3;
+
 /**
  * How Step 3 runs: the orchestrator invokes the deterministic dispatcher
  * (`lib/dispatch.ts`) once, which runs Step 3's phases as code. `scripted`
@@ -130,6 +149,9 @@ export type RoutingFileConfig = {
     /** `re-review <mode> blocking-only`: repeat reviews post only blocking
      * findings inline (see {@link RE_REVIEW_MODIFIERS}). */
     reReviewBlockingOnly: boolean;
+    /** `non-blocking-budget <n>`: how many non-blocking findings may post
+     * inline per review (see {@link DEFAULT_NON_BLOCKING_INLINE_BUDGET}). */
+    nonBlockingInlineBudget: number;
     /** The dispatch mode: always `scripted`. */
     dispatchMode: DispatchMode;
     /** Fixed-format parse warnings (unknown lens/tier, no-op rule). */
@@ -145,6 +167,7 @@ const KNOWN_LENS_SET: ReadonlySet<string> = new Set(KNOWN_LENSES);
  *     <pattern> [lens=<lens>[,<lens>…]] [tier=trivial|low|medium|high] [direction-dependent]
  *     enable <reviewer>[,<reviewer>…]
  *     re-review full|scoped|flip-gated|fast [blocking-only]
+ *     non-blocking-budget <n>
  *
  * `lens=` names specialist lenses to spawn when the pattern is touched (multiple
  * matching rules union their lenses). `tier=` assigns a risk tier; when several
@@ -162,8 +185,12 @@ const KNOWN_LENS_SET: ReadonlySet<string> = new Set(KNOWN_LENSES);
  * ({@link RE_REVIEW_MODIFIERS}) makes repeat reviews post only blocking
  * findings inline; an unknown modifier warns and is ignored (the mode still
  * applies), and `full blocking-only` warns that the modifier never applies
- * at full depth. A leftover `dispatch` line from the
- * retired dial warns and is ignored (scripted is the only mode).
+ * at full depth. `non-blocking-budget` sets how many non-blocking findings
+ * post inline per review ({@link DEFAULT_NON_BLOCKING_INLINE_BUDGET});
+ * a malformed value warns and keeps the previous value, and when several
+ * lines set it the last one wins (with a warning). A leftover `dispatch`
+ * line from the retired dial warns and is ignored (scripted is the only
+ * mode).
  *
  * 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
@@ -177,6 +204,8 @@ export const parseRoutingConfig = (content: string): RoutingFileConfig => {
     let reReviewMode: ReReviewMode = DEFAULT_RE_REVIEW_MODE;
     let reReviewBlockingOnly = false;
     let reReviewLineSeen = false;
+    let nonBlockingInlineBudget = DEFAULT_NON_BLOCKING_INLINE_BUDGET;
+    let budgetLineSeen = false;
     let dispatchLineSeen = false;
     const warnings: string[] = [];
 
@@ -266,6 +295,34 @@ export const parseRoutingConfig = (content: string): RoutingFileConfig => {
             continue;
         }
 
+        if (pattern === "non-blocking-budget") {
+            if (fields.length !== 1) {
+                warnings.push(
+                    `ROUTING line ${lineNo}: non-blocking-budget takes ` +
+                        `exactly one number (line skipped)`,
+                );
+                continue;
+            }
+            const value = Number(fields[0]);
+            if (!Number.isInteger(value) || value < 0) {
+                warnings.push(
+                    `ROUTING line ${lineNo}: non-blocking-budget must be a ` +
+                        `non-negative integer, got "${fields[0]}" (kept ` +
+                        `${nonBlockingInlineBudget})`,
+                );
+                continue;
+            }
+            if (budgetLineSeen) {
+                warnings.push(
+                    `ROUTING line ${lineNo}: duplicate non-blocking-budget ` +
+                        `line (last one wins)`,
+                );
+            }
+            nonBlockingInlineBudget = value;
+            budgetLineSeen = true;
+            continue;
+        }
+
         if (pattern === "dispatch") {
             // The dial is retired: scripted dispatch always runs. A leftover
             // line is tolerated (never a crashed run); any value other than
@@ -361,6 +418,7 @@ export const parseRoutingConfig = (content: string): RoutingFileConfig => {
         ),
         reReviewMode,
         reReviewBlockingOnly,
+        nonBlockingInlineBudget,
         dispatchMode: DEFAULT_DISPATCH_MODE,
         warnings,
     };