diff --git a/.changeset/pra7-posting-surface.md b/.changeset/pra7-posting-surface.md new file mode 100644 index 00000000..a0acfcf7 --- /dev/null +++ b/.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. diff --git a/workflows/autofix/README.md b/workflows/autofix/README.md index 07eabf40..c2f4ece5 100644 --- a/workflows/autofix/README.md +++ b/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:** diff --git a/workflows/review/README.md b/workflows/review/README.md index 5e77daee..85383729 100644 --- a/workflows/review/README.md +++ b/workflows/review/README.md @@ -366,6 +366,7 @@ rule per line: # [lens=,…] [tier=trivial|low|medium|high] [direction-dependent] # enable [,…] # re-review full|scoped|flip-gated|fast [blocking-only] +# non-blocking-budget 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 `
` 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): ```
review details -review-v.. | schema | depth | re-review [blocking-only] | enable +review-v.. | schema | depth | re-review [blocking-only] | enable | non-blocking-budget
``` `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. diff --git a/workflows/review/lib/attribution.ts b/workflows/review/lib/attribution.ts index 2d79645e..76582b49 100644 --- a/workflows/review/lib/attribution.ts +++ b/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, "&").replace(//g, ">"); const flaggedBy = (entry: AlsoFlagged): string => { diff --git a/workflows/review/lib/render-comment.ts b/workflows/review/lib/render-comment.ts index 53ec4788..74b22229 100644 --- a/workflows/review/lib/render-comment.ts +++ b/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)", diff --git a/workflows/review/lib/router-non-blocking-budget.test.ts b/workflows/review/lib/router-non-blocking-budget.test.ts new file mode 100644 index 00000000..f25b074b --- /dev/null +++ b/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 ` 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) => { + const written: Record = {}; + 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(); + 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); + }); +}); diff --git a/workflows/review/lib/router.ts b/workflows/review/lib/router.ts index 2543ed2c..172a93d5 100644 --- a/workflows/review/lib/router.ts +++ b/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 = {}; 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}); diff --git a/workflows/review/lib/routing-config.ts b/workflows/review/lib/routing-config.ts index bea5d224..59fc5fa4 100644 --- a/workflows/review/lib/routing-config.ts +++ b/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
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 blocking-only`: repeat reviews post only blocking * findings inline (see {@link RE_REVIEW_MODIFIERS}). */ reReviewBlockingOnly: boolean; + /** `non-blocking-budget `: 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 = new Set(KNOWN_LENSES); * [lens=[,…]] [tier=trivial|low|medium|high] [direction-dependent] * enable [,…] * re-review full|scoped|flip-gated|fast [blocking-only] + * non-blocking-budget * * `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 = 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, }; diff --git a/workflows/review/lib/submission-blocking-only.test.ts b/workflows/review/lib/submission-blocking-only.test.ts index 499c1184..922aa682 100644 --- a/workflows/review/lib/submission-blocking-only.test.ts +++ b/workflows/review/lib/submission-blocking-only.test.ts @@ -109,7 +109,12 @@ describe("runSubmissionCli: re-review blocking-only", () => { expect(plan.comments[0].body).not.toContain( "Non-blocking observations", ); - expect(plan.body).toContain("Non-blocking observations (2)"); + // The pr-level note outranks the nitpick for the summary slot + // (nitpicks rank last; the collapsed list re-sorts with pr-level + // claims included). + expect(plan.body).toContain( + "Non-blocking observations (2; top: note (non-blocking): A cross-file observation.)", + ); expect(plan.body).toContain( "- `a.ts:9` nitpick (non-blocking): Rename the helper. " + "(correctness-reviewer)", @@ -160,8 +165,8 @@ describe("runSubmissionCli: re-review blocking-only", () => { depth: "full", claims: [ claim({ - id: "nit", - label: "nitpick (non-blocking)", + id: "sug", + label: "suggestion (non-blocking)", confidence: 0.9, }), ], @@ -181,8 +186,8 @@ describe("runSubmissionCli: re-review blocking-only", () => { depth: "scoped", claims: [ claim({ - id: "nit", - label: "nitpick (non-blocking)", + id: "sug", + label: "suggestion (non-blocking)", confidence: 0.9, }), ], @@ -215,7 +220,9 @@ describe("runSubmissionCli: re-review blocking-only", () => { const plan = runSubmissionCli(fs); expect(plan.comments).toHaveLength(20); expect(plan.body).not.toContain("Non-blocking observations"); - expect(plan.body).toContain("Lower-confidence observations (1)"); + expect(plan.body).toContain( + "Lower-confidence observations (1; top: `a.ts:21` issue (blocking): s)", + ); expect(plan.body).toContain("issue (blocking)"); expect(plan.notes.join(" ")).toContain( "1 claim(s) collapsed below the inline bar", @@ -243,9 +250,36 @@ describe("runSubmissionCli: re-review blocking-only", () => { const plan = runSubmissionCli(fs); expect(plan.event).toBe("APPROVE"); expect(plan.comments).toEqual([]); - expect(plan.body).toContain("Non-blocking observations (1)"); + expect(plan.body).toContain( + "Non-blocking observations (1; top: `a.ts:2` nitpick (non-blocking): Rename the helper.)", + ); // A body carrying the collapsed section is never the bare approve // line, so the redundant-approval skip cannot swallow it. expect(plan.skipSubmission).toBe(false); }); }); + +describe("the collapsed summary's pr-level arm", () => { + it("names a pr-level top entry by label and subject (no anchor to show)", () => { + const fs = makeFakeFs( + staged( + { + depth: "scoped", + claims: [ + claim({ + id: "pr-note", + path: undefined, + line: undefined, + label: "note (non-blocking)", + subject: "A cross-file observation.", + }), + ], + }, + true, + ), + ); + expect(runSubmissionCli(fs).body).toContain( + "Non-blocking observations (1; top: note (non-blocking): A cross-file observation.)", + ); + }); +}); diff --git a/workflows/review/lib/submission-render.ts b/workflows/review/lib/submission-render.ts new file mode 100644 index 00000000..d228208e --- /dev/null +++ b/workflows/review/lib/submission-render.ts @@ -0,0 +1,175 @@ +/** + * Claim rendering for the submission plan (split from `submission.ts` by its + * max-lines budget, the dispatch-contracts precedent): the Conventional + * Comment renderer driven by a claim's post-validation label, the pr-level + * body fold, the drop-in-suggestion gate, and the label-token vocabulary + * helpers. Everything here sits inside the determinism boundary: CODE owns + * the wrapping and the gates, MODELS own the prose, which is copied + * verbatim. + */ + +import type {Claim} from "./dispatch-contracts"; + +/** + * How many lines a committable suggestion may replace the anchored line + * with; anything longer is a sketch, not a drop-in. + */ +const MAX_SUGGESTION_LINES = 8; + +/** + * The base token of a Conventional-Comment label (`nitpick` from + * `nitpick (non-blocking)`). Same parse {@link labelAdmitsSketch} uses; + * factored so the two agree on what a label's token IS. + */ +export const labelToken = (label: string): string => + (label.trim().split(/[\s(:]/, 1)[0] ?? "").toLowerCase(); + +/** + * A pr-level claim's discussion folds into the body verbatim only up to + * this length; past it, the body carries the claim's subject line and the + * full discussion moves into a
block. webapp#41290 review + * 4867627688 folded a ~2,600-char single-paragraph finding directly into + * the body, burying the accountability section and the note lines around + * it; a short paragraph is the most a fold can carry without doing that. + */ +export const MAX_VERBATIM_FOLD_CHARS = 400; + +/** + * Render a pr-level claim for the review body: verbatim while it reads as + * a short paragraph, subject line plus a collapsed full finding once it + * does not. + */ +export const renderPrLevelFold = (claim: Claim): string => { + if (claim.discussion.length <= MAX_VERBATIM_FOLD_CHARS) { + return `**${claim.label}:** ${claim.discussion}`; + } + return [ + `**${claim.label}:** ${claim.subject}`, + "
", + "Full finding", + "", + claim.discussion, + "", + "
", + ].join("\n"); +}; + +/** + * The one-line source tag for collapsed/hold list entries: the same + * attribution the full comments carry, in the smallest form that fits a + * one-liner (a whole collapsed footer per list entry would bury the list). + * `` is sanitizer-allowed, and attribution.ts's stripFooters removes + * the span before any text-similarity comparison against posted bodies. + */ +export const sourceTag = (claim: Claim): string => + `(${claim.source})`; + +const lineHasCodeSignal = (line: string): boolean => + /\w\(/.test(line) || // a call + /[{};]/.test(line) || // block/statement punctuation + /:=|=>|->/.test(line) || // assignment/arrow operators + /^\s*(\/\/|#|\/\*|\*)/.test(line) || // a comment marker + /^\t/.test(line); // code-convention indentation + +const looksLikeProse = (line: string): boolean => { + // Deliberately NOT vetoed by lineHasCodeSignal: run 29901690493 posted + // "Use ctx.Time().Now().AddDate(0, 0, -MemoryTTLDays), and add a test + // that ..." as a committable fence because the embedded call defeated + // the prose check. A sentence that names code is still a sentence. + const words = line.trim().split(/\s+/); + if (words.length < 6) { + return false; + } + const plain = words.filter((word) => + /^\(?[A-Za-z][A-Za-z']*[.,;:!?)]?$/.test(word), + ); + return plain.length / words.length >= 0.75; +}; + +/** + * Whether a claim's suggestion is plausibly a committable replacement of + * the anchored line: small and code-shaped. Trial run 29897276810 posted an + * English sentence and a 30-line test function inside `suggestion` fences + * (Khan/webapp#41009 comments r3628128268 / r3628128224), both of which a + * single click would have committed verbatim into the file. + */ +export const isDropInSuggestion = (suggestion: string): boolean => { + const lines = suggestion.replace(/\n$/, "").split("\n"); + const content = lines.filter((line) => line.trim() !== ""); + if (content.length === 0 || lines.length > MAX_SUGGESTION_LINES) { + return false; + } + return content.some(lineHasCodeSignal) && !content.some(looksLikeProse); +}; + +/** + * The base label tokens whose comments propose a fix, and so may carry a + * sketch block. `issue` and `suggestion` are the fix-proposing labels; + * `todo (blocking)` is verdict-equivalent to `issue (blocking)` (see + * render-comment.ts), so stripping its fix would remove the sketch from a + * blocking finding. `question`, `thought`, `note`, and `nitpick` raise a + * point rather than propose a fix: measured on Khan/webapp (2026-08-11/12), + * 31 of 57 posted comments carried a sketch, including questions and + * thoughts whose sketch restated the prose without adding information. + * + * Deliberate consequence: a dispute-capped claim relabeled to + * `question (non-blocking)` by applyVerifications keeps its `suggestion` + * field but posts without the sketch block. The gate is about information + * loss, not the label's tone: a sketch restates prose (the measured + * sample), so dropping it under a non-fix label costs length, not content, + * whereas a drop-in fence IS the fix in committable form and renders under + * any label (see renderClaimComment). So a disputed claim keeps its + * one-click fix and loses only the restatement. + */ +const SKETCH_LABEL_TOKENS: ReadonlySet = new Set([ + "issue", + "todo", + "suggestion", +]); + +/** + * Whether a claim's label admits a sketch block. Matches on the base label + * token so every variant counts (`suggestion (non-blocking, documentation)` + * is a suggestion). An unparseable label is sketch-eligible: fail toward + * more information, never toward silently dropping an authored fix. + */ +export const labelAdmitsSketch = (label: string): boolean => { + const token = labelToken(label); + return token === "" || SKETCH_LABEL_TOKENS.has(token); +}; + +/** + * Render one claim as its Conventional Comment (the renderComment layout, + * driven by the claim's post-validation label rather than a recomputed one). + * A suggestion only becomes a committable `suggestion` fence when it is + * plausibly drop-in; otherwise it renders as a plain fenced sketch, and only + * under a fix-proposing label ({@link labelAdmitsSketch}): a question or + * thought proposes no fix, so a sketch under it adds length, not + * information. + */ +export const renderClaimComment = (claim: Claim): string => { + const lines: string[] = [`**${claim.label}:** ${claim.discussion}`]; + if (claim.rule_quote !== undefined) { + const [first, ...rest] = claim.rule_quote.split("\n"); + lines.push( + "", + `> **Rule:** ${first}`, + ...rest.map((line) => (line === "" ? ">" : `> ${line}`)), + ); + } + if (claim.suggestion !== undefined) { + if (isDropInSuggestion(claim.suggestion)) { + lines.push("", "```suggestion", claim.suggestion, "```"); + } else if (labelAdmitsSketch(claim.label)) { + lines.push( + "", + "A sketch, not a committable replacement:", + "", + "````", + claim.suggestion, + "````", + ); + } + } + return lines.join("\n"); +}; diff --git a/workflows/review/lib/submission-trial-followups.test.ts b/workflows/review/lib/submission-trial-followups.test.ts index 853c7ad0..50a75b73 100644 --- a/workflows/review/lib/submission-trial-followups.test.ts +++ b/workflows/review/lib/submission-trial-followups.test.ts @@ -3,10 +3,11 @@ import {describe, it, expect} from "vitest"; import {runSubmissionCli, type SubmissionFs} from "./submission"; /** - * Submission-plan tests for the post-trial follow-ups: risks/patterns key - * staging, the inline posting bar, and the open-thread suppression verdict - * floor. Split from submission.test.ts by the max-lines budget; the fixtures - * below are small local copies of that file's helpers. + * Submission-plan tests for the post-trial follow-ups and the P1 posting + * budget: risks/patterns key staging, the inline posting bar, the + * non-blocking budget and nitpick rules, and the open-thread suppression + * verdict floor. Split from submission.test.ts by the max-lines budget; the + * fixtures below are small local copies of that file's helpers. */ const REVIEW = "/tmp/gh-aw/review"; @@ -126,14 +127,14 @@ describe("the inline posting bar (the Step 5 cap, as code)", () => { ); expect(plan.comments).toHaveLength(20); expect(plan.comments[0].body).toContain( - "Lower-confidence observations (2)", + "Lower-confidence observations (2; top: `a.ts:21` issue (blocking): finding 21)", ); expect(plan.comments[0].body).toContain("`a.ts:21`"); expect(plan.comments[0].body).toContain("`a.ts:22`"); // A collapsed blocking claim still drives the verdict. expect(plan.event).toBe("REQUEST_CHANGES"); expect(plan.notes).toContainEqual( - "2 claim(s) collapsed below the inline bar (cap 20, medium-confidence floor)", + "2 claim(s) collapsed below the inline bar (cap 20, medium-confidence floor, non-blocking budget 3)", ); }); @@ -153,15 +154,93 @@ describe("the inline posting bar (the Step 5 cap, as code)", () => { const plan = runSubmissionCli( makeFakeFs(staged({depth: "full", claims})), ); - expect(plan.comments).toHaveLength(20); - // The blocking claim posts inline first; the weakest non-blocking - // claim is the one collapsed. + // The blocking claim posts inline first; the non-blocking budget + // (default 3) admits the next three in ranked order, and the rest + // collapse with the budget-shed note. + expect(plan.comments).toHaveLength(4); expect(plan.comments[0].line).toBe(99); expect(plan.comments[0].body).toContain( - "Lower-confidence observations (1)", + "Lower-confidence observations (17; top: `a.ts:4` suggestion (non-blocking): finding 4)", + ); + expect(plan.notes).toContainEqual( + "17 non-blocking claim(s) collapsed over the inline budget (non-blocking budget 3)", ); }); + it("reads the non-blocking budget from routing.json", () => { + const claims = manyClaims(3, { + label: "suggestion (non-blocking)", + confidence: 0.9, + }); + const plan = runSubmissionCli( + makeFakeFs( + staged( + {depth: "full", claims}, + { + [`${REVIEW}/routing.json`]: JSON.stringify({ + nonBlockingInlineBudget: 1, + }), + }, + ), + ), + ); + expect(plan.comments).toHaveLength(1); + expect(plan.notes).toContainEqual( + "2 non-blocking claim(s) collapsed over the inline budget (non-blocking budget 1)", + ); + }); + + it("never posts a nitpick inline, budget or no budget", () => { + const claims = [ + claim({ + id: "nit", + line: 1, + label: "nitpick (non-blocking)", + confidence: 0.95, + subject: "rename it", + }), + claim({ + id: "sug", + line: 2, + label: "suggestion (non-blocking)", + confidence: 0.6, + }), + ]; + const plan = runSubmissionCli( + makeFakeFs(staged({depth: "full", claims})), + ); + // The lower-confidence suggestion posts; the nitpick collapses + // despite outranking it on confidence. + expect(plan.comments).toHaveLength(1); + expect(plan.comments[0].line).toBe(2); + expect(plan.comments[0].body).toContain( + "Lower-confidence observations (1; top: `a.ts:1` nitpick (non-blocking): rename it)", + ); + }); + + it("exempts documentation-label claims from the budget (autofix selects by posted label)", () => { + const claims = [ + ...manyClaims(3, { + label: "suggestion (non-blocking)", + confidence: 0.9, + }), + claim({ + id: "doc", + line: 30, + label: "suggestion (non-blocking, documentation)", + confidence: 0.6, + }), + ]; + const plan = runSubmissionCli( + makeFakeFs(staged({depth: "full", claims})), + ); + // Three suggestions spend the whole budget; the documentation claim + // still posts inline (it must become a thread for the documentation + // autofix to see it). + expect(plan.comments).toHaveLength(4); + expect(plan.comments.map((entry) => entry.line)).toContain(30); + }); + it("collapses sub-medium-confidence non-blocking claims even under the cap", () => { const claims = [ claim({ @@ -201,7 +280,9 @@ describe("the inline posting bar (the Step 5 cap, as code)", () => { makeFakeFs(staged({depth: "full", claims})), ); expect(plan.comments).toEqual([]); - expect(plan.body).toContain("Lower-confidence observations (1)"); + expect(plan.body).toContain( + "Lower-confidence observations (1; top: `a.ts:2` thought (non-blocking): a hunch)", + ); expect(plan.body).toContain("a hunch"); }); }); @@ -289,3 +370,193 @@ describe("open-thread suppression verdict floor (trial suggestion g)", () => { expect(runSubmissionCli(fs).event).toBe("APPROVE"); }); }); + +describe("the nitpick posting rules", () => { + it("ranks nitpicks last in the collapse, whatever their confidence, and notes the shed", () => { + const claims = [ + claim({ + id: "nit", + line: 1, + label: "nitpick (non-blocking)", + confidence: 0.95, + subject: "rename it", + }), + claim({ + id: "weak-thought", + line: 2, + label: "thought (non-blocking)", + confidence: 0.3, + subject: "a hunch", + }), + ]; + const plan = runSubmissionCli( + makeFakeFs(staged({depth: "full", claims})), + ); + // Both collapse (the nitpick by the ban, the thought by the + // confidence floor), and the disclosure's top slot goes to the + // thought: the class this surface never posts must not win the + // summary line built for the tail's best finding. + expect(plan.comments).toEqual([]); + expect(plan.body).toContain( + "Lower-confidence observations (2; top: `a.ts:2` thought (non-blocking): a hunch)", + ); + expect(plan.notes).toContainEqual( + "1 nitpick claim(s) collapsed (nitpick-class never posts inline)", + ); + }); +}); + +describe("the budget's edge values", () => { + it("a zero budget posts blocking and documentation claims only", () => { + const claims = [ + claim({id: "blocking", line: 1, label: "issue (blocking)"}), + claim({ + id: "doc", + line: 2, + label: "suggestion (non-blocking, documentation)", + confidence: 0.9, + }), + claim({ + id: "sug", + line: 3, + label: "suggestion (non-blocking)", + confidence: 0.9, + }), + ]; + const plan = runSubmissionCli( + makeFakeFs( + staged( + {depth: "full", claims}, + { + [`${REVIEW}/routing.json`]: JSON.stringify({ + nonBlockingInlineBudget: 0, + }), + }, + ), + ), + ); + expect(plan.comments.map((entry) => entry.line).sort()).toEqual([1, 2]); + expect(plan.notes).toContainEqual( + "1 non-blocking claim(s) collapsed over the inline budget (non-blocking budget 0)", + ); + }); + + it("a pr-level claim can be the tail's named top entry", () => { + const claims = [ + claim({ + id: "pr-level", + path: undefined, + line: undefined, + label: "note (non-blocking)", + confidence: 0.9, + subject: "A cross-file observation.", + }), + claim({ + id: "weak", + line: 2, + label: "thought (non-blocking)", + confidence: 0.3, + subject: "a hunch", + }), + ]; + const plan = runSubmissionCli( + makeFakeFs( + staged( + {depth: "scoped", claims}, + { + [`${REVIEW}/routing.json`]: JSON.stringify({ + reReviewBlockingOnly: true, + }), + }, + ), + ), + ); + // Both collapse under blocking-only; the pr-level note outranks the + // low-confidence anchored thought for the summary slot. + expect(plan.body).toContain( + "Non-blocking observations (2; top: note (non-blocking): A cross-file observation.)", + ); + }); +}); + +describe("the budget's spend order", () => { + it("spends the budget on the highest-confidence non-blocking claims", () => { + const claims = [ + claim({ + id: "weakest", + line: 1, + label: "suggestion (non-blocking)", + confidence: 0.55, + }), + claim({ + id: "strongest", + line: 2, + label: "suggestion (non-blocking)", + confidence: 0.95, + }), + claim({ + id: "middle", + line: 3, + label: "suggestion (non-blocking)", + confidence: 0.75, + }), + ]; + const plan = runSubmissionCli( + makeFakeFs( + staged( + {depth: "full", claims}, + { + [`${REVIEW}/routing.json`]: JSON.stringify({ + nonBlockingInlineBudget: 2, + }), + }, + ), + ), + ); + // The budget spends in ranked order, so the two strongest post and + // the weakest is the one collapsed. + expect(plan.comments.map((entry) => entry.line).sort()).toEqual([2, 3]); + // The shed claim lands in the collapsed section riding the + // top-ranked comment (this branch predates the always-in-body move). + expect(plan.comments[0].body).toContain("`a.ts:1`"); + }); +}); + +describe("the named-top tag's escaping", () => { + it("escapes a model-authored subject and truncates a long one", () => { + const hostile = "Breaks out
of the block"; + const claims = [ + claim({ + id: "hostile", + line: 2, + label: "thought (non-blocking)", + confidence: 0.3, + subject: hostile, + }), + claim({ + id: "long", + line: 3, + label: "thought (non-blocking)", + confidence: 0.2, + subject: "x".repeat(150), + }), + ]; + const plan = runSubmissionCli( + makeFakeFs(staged({depth: "full", claims})), + ); + // The hostile subject ranks first (higher confidence): the SUMMARY + // line carries entities, never the raw tag. (The list entries below + // it share the exposure but predate this PR; the finding's own + // scope note excludes them.) + const summaryLine = plan.body + .split("\n") + .find((line) => line.includes("Lower-confidence observations")); + expect(summaryLine).toContain( + "Breaks out</summary></details> <b>of the block</b>", + ); + expect(summaryLine).not.toContain("out"); + // The long subject keeps its full text in the list entry; only the + // summary tag truncates. + expect(plan.body).toContain("x".repeat(150)); + }); +}); diff --git a/workflows/review/lib/submission.ts b/workflows/review/lib/submission.ts index 6dc275ed..027669e4 100644 --- a/workflows/review/lib/submission.ts +++ b/workflows/review/lib/submission.ts @@ -60,18 +60,27 @@ * under review. */ -import {renderAttributionFooter} from "./attribution"; +import {escapeHtml, renderAttributionFooter} from "./attribution"; import {computeRisksPatternsKey, RISKS_PATTERNS_KEY_PATH} from "./cache-record"; import type {Claim} from "./dispatch-contracts"; import {DEFAULT_FINDERS, TRIAGE_DIMENSION} from "./dispatch-roster"; import {runCli as runNotifiedCli} from "./notified"; import { + DOCUMENTATION_LABEL, HOLD_HEAD, HOLD_UNSTUCK_LINES, isBlockingLabel, + NITPICK_LABEL, renderReviewBody, } from "./render-comment"; +import {DEFAULT_NON_BLOCKING_INLINE_BUDGET} from "./routing-config"; import {runRereviewCli, type RereviewCliFs} from "./rereview"; +import { + labelToken, + renderClaimComment, + renderPrLevelFold, + sourceTag, +} from "./submission-render"; import {normalizeBody} from "./sanitizer-normalize"; import { findLatestStamp, @@ -244,14 +253,16 @@ const readCacheMemoryRecord = (fs: SubmissionFs): unknown => { }; /* -------------------------------------------------------------------------- */ -/* Rendering */ +/* Rendering (submission-render.ts; re-exported for the existing consumers) */ /* -------------------------------------------------------------------------- */ -/** - * How many lines a committable suggestion may replace the anchored line - * with; anything longer is a sketch, not a drop-in. - */ -const MAX_SUGGESTION_LINES = 8; +export { + isDropInSuggestion, + labelAdmitsSketch, + MAX_VERBATIM_FOLD_CHARS, + renderClaimComment, + renderPrLevelFold, +} from "./submission-render"; /** * At most this many inline comments post; the rest collapse (the Step 5 cap, @@ -265,154 +276,8 @@ export const MAX_INLINE_COMMENTS = 20; /** The medium-confidence inline floor (the Step 5 posting bar). */ const MIN_INLINE_CONFIDENCE = 0.5; -/** - * A pr-level claim's discussion folds into the body verbatim only up to - * this length; past it, the body carries the claim's subject line and the - * full discussion moves into a
block. webapp#41290 review - * 4867627688 folded a ~2,600-char single-paragraph finding directly into - * the body, burying the accountability section and the note lines around - * it; a short paragraph is the most a fold can carry without doing that. - */ -export const MAX_VERBATIM_FOLD_CHARS = 400; - -/** - * Render a pr-level claim for the review body: verbatim while it reads as - * a short paragraph, subject line plus a collapsed full finding once it - * does not. - */ -export const renderPrLevelFold = (claim: Claim): string => { - if (claim.discussion.length <= MAX_VERBATIM_FOLD_CHARS) { - return `**${claim.label}:** ${claim.discussion}`; - } - return [ - `**${claim.label}:** ${claim.subject}`, - "
", - "Full finding", - "", - claim.discussion, - "", - "
", - ].join("\n"); -}; - -/** - * The one-line source tag for collapsed/hold list entries: the same - * attribution the full comments carry, in the smallest form that fits a - * one-liner (a whole collapsed footer per list entry would bury the list). - * `` is sanitizer-allowed, and attribution.ts's stripFooters removes - * the span before any text-similarity comparison against posted bodies. - */ -const sourceTag = (claim: Claim): string => `(${claim.source})`; - -const lineHasCodeSignal = (line: string): boolean => - /\w\(/.test(line) || // a call - /[{};]/.test(line) || // block/statement punctuation - /:=|=>|->/.test(line) || // assignment/arrow operators - /^\s*(\/\/|#|\/\*|\*)/.test(line) || // a comment marker - /^\t/.test(line); // code-convention indentation - -const looksLikeProse = (line: string): boolean => { - // Deliberately NOT vetoed by lineHasCodeSignal: run 29901690493 posted - // "Use ctx.Time().Now().AddDate(0, 0, -MemoryTTLDays), and add a test - // that ..." as a committable fence because the embedded call defeated - // the prose check. A sentence that names code is still a sentence. - const words = line.trim().split(/\s+/); - if (words.length < 6) { - return false; - } - const plain = words.filter((word) => - /^\(?[A-Za-z][A-Za-z']*[.,;:!?)]?$/.test(word), - ); - return plain.length / words.length >= 0.75; -}; - -/** - * Whether a claim's suggestion is plausibly a committable replacement of - * the anchored line: small and code-shaped. Trial run 29897276810 posted an - * English sentence and a 30-line test function inside `suggestion` fences - * (Khan/webapp#41009 comments r3628128268 / r3628128224), both of which a - * single click would have committed verbatim into the file. - */ -export const isDropInSuggestion = (suggestion: string): boolean => { - const lines = suggestion.replace(/\n$/, "").split("\n"); - const content = lines.filter((line) => line.trim() !== ""); - if (content.length === 0 || lines.length > MAX_SUGGESTION_LINES) { - return false; - } - return content.some(lineHasCodeSignal) && !content.some(looksLikeProse); -}; - -/** - * The base label tokens whose comments propose a fix, and so may carry a - * sketch block. `issue` and `suggestion` are the fix-proposing labels; - * `todo (blocking)` is verdict-equivalent to `issue (blocking)` (see - * render-comment.ts), so stripping its fix would remove the sketch from a - * blocking finding. `question`, `thought`, `note`, and `nitpick` raise a - * point rather than propose a fix: measured on Khan/webapp (2026-08-11/12), - * 31 of 57 posted comments carried a sketch, including questions and - * thoughts whose sketch restated the prose without adding information. - * - * Deliberate consequence: a dispute-capped claim relabeled to - * `question (non-blocking)` by applyVerifications keeps its `suggestion` - * field but posts without the sketch block. The gate is about information - * loss, not the label's tone: a sketch restates prose (the measured - * sample), so dropping it under a non-fix label costs length, not content, - * whereas a drop-in fence IS the fix in committable form and renders under - * any label (see renderClaimComment). So a disputed claim keeps its - * one-click fix and loses only the restatement. - */ -const SKETCH_LABEL_TOKENS: ReadonlySet = new Set([ - "issue", - "todo", - "suggestion", -]); - -/** - * Whether a claim's label admits a sketch block. Matches on the base label - * token so every variant counts (`suggestion (non-blocking, documentation)` - * is a suggestion). An unparseable label is sketch-eligible: fail toward - * more information, never toward silently dropping an authored fix. - */ -export const labelAdmitsSketch = (label: string): boolean => { - const token = label.trim().split(/[\s(:]/, 1)[0] ?? ""; - return token === "" || SKETCH_LABEL_TOKENS.has(token.toLowerCase()); -}; - -/** - * Render one claim as its Conventional Comment (the renderComment layout, - * driven by the claim's post-validation label rather than a recomputed one). - * A suggestion only becomes a committable `suggestion` fence when it is - * plausibly drop-in; otherwise it renders as a plain fenced sketch, and only - * under a fix-proposing label ({@link labelAdmitsSketch}): a question or - * thought proposes no fix, so a sketch under it adds length, not - * information. - */ -export const renderClaimComment = (claim: Claim): string => { - const lines: string[] = [`**${claim.label}:** ${claim.discussion}`]; - if (claim.rule_quote !== undefined) { - const [first, ...rest] = claim.rule_quote.split("\n"); - lines.push( - "", - `> **Rule:** ${first}`, - ...rest.map((line) => (line === "" ? ">" : `> ${line}`)), - ); - } - if (claim.suggestion !== undefined) { - if (isDropInSuggestion(claim.suggestion)) { - lines.push("", "```suggestion", claim.suggestion, "```"); - } else if (labelAdmitsSketch(claim.label)) { - lines.push( - "", - "A sketch, not a committable replacement:", - "", - "````", - claim.suggestion, - "````", - ); - } - } - return lines.join("\n"); -}; +/** The collapsed summary's named-top subject cap: one line, not a wall. */ +const TOP_SUBJECT_MAX_CHARS = 120; /* -------------------------------------------------------------------------- */ /* The plan */ @@ -486,7 +351,11 @@ export const runSubmissionCli = ( : []; const depth = typeof dispatch.depth === "string" ? dispatch.depth : "full"; const routing = readJson(fs, `${REVIEW_DIR}/routing.json`) as - | {teams?: {owners?: unknown}; reReviewBlockingOnly?: unknown} + | { + teams?: {owners?: unknown}; + reReviewBlockingOnly?: unknown; + nonBlockingInlineBudget?: unknown; + } | undefined; // The ROUTING `re-review blocking-only` modifier: a repeat review // at a reduced depth posts only blocking findings inline; validated @@ -745,27 +614,85 @@ export const runSubmissionCli = ( // MAX_INLINE_COMMENTS post inline: the frontmatter caps the // create-pull-request-review-comment safe output at the same number, so // a longer plan would have the engine reject the overflow and the - // conformance gate red the run after full spend. Everything else - // collapses to one terse line each in a single
block riding - // the highest-ranked inline comment (or the review body when nothing - // posts inline), so it is surfaced without scattering noise. The - // verdict is computed from ALL claims, so a collapsed blocking claim - // (a 21st blocking finding) still blocks. - const ranked = [...anchored].sort((a, b) => { + // conformance gate red the run after full spend. + // + // Three further rules shape the non-blocking surface (the P1 comment + // budget, quiet-the-human-surface lane): + // - `nitpick (non-blocking)` never posts inline: the label names the + // class the lane demotes wholesale (naming, doc-comment nits). + // - At most `nonBlockingInlineBudget` other non-blocking claims post + // inline (routing.json's `non-blocking-budget` line, default 3); + // the budget spends in ranked order, so what sheds is chosen. + // - The documentation label is exempt from the budget: the + // documentation reviewer self-caps at five per review, and the + // documentation autofix selects its work by parsing that label off + // posted threads, so budgeting those would silently shrink a + // shipped feature's scope (until the autofix learns to read the + // collapsed section, at which point the exemption goes). + // + // Everything else collapses to one terse line each in a single + //
block riding the highest-ranked inline comment (or the + // review body when nothing posts inline), so it is surfaced without + // scattering noise. The verdict is computed from ALL claims, so a + // collapsed blocking claim (a 21st blocking finding) still blocks. + const budgetRaw = routing?.nonBlockingInlineBudget; + const nonBlockingBudget = + typeof budgetRaw === "number" && + Number.isInteger(budgetRaw) && + budgetRaw >= 0 + ? budgetRaw + : DEFAULT_NON_BLOCKING_INLINE_BUDGET; + const isNitpick = (claim: Claim): boolean => + labelToken(claim.label) === labelToken(NITPICK_LABEL); + // Named so the collapsed list below can re-sort with the same rank + // once the pr-level claims join it: the disclosure names the tail's + // first entry, so the ordering IS the disclosure's selection rule. + const rankClaims = (a: Claim, b: Claim): number => { const blocking = Number(isBlockingLabel(b.label)) - Number(isBlockingLabel(a.label)); - return blocking !== 0 ? blocking : b.confidence - a.confidence; + if (blocking !== 0) { + return blocking; + } + // Nitpicks rank last among non-blocking claims, whatever their + // confidence: without the demotion the class this surface + // deliberately never posts would routinely win the summary slot + // built for the tail's best finding. + const nitpick = Number(isNitpick(a)) - Number(isNitpick(b)); + return nitpick !== 0 ? nitpick : b.confidence - a.confidence; + }; + const ranked = [...anchored].sort(rankClaims); + let budgetLeft = nonBlockingBudget; + let budgetShed = 0; + let nitpickShed = 0; + const inlineWorthy = ranked.filter((claim) => { + if (isBlockingLabel(claim.label)) { + return true; + } + if (blockingOnly || claim.confidence < MIN_INLINE_CONFIDENCE) { + return false; + } + if (isNitpick(claim)) { + nitpickShed++; + return false; + } + if (claim.label === DOCUMENTATION_LABEL) { + return true; + } + if (budgetLeft > 0) { + budgetLeft--; + return true; + } + budgetShed++; + return false; }); - const inlineWorthy = ranked.filter( - (claim) => - isBlockingLabel(claim.label) || - (!blockingOnly && claim.confidence >= MIN_INLINE_CONFIDENCE), - ); const inlineClaims = new Set(inlineWorthy.slice(0, MAX_INLINE_COMMENTS)); + // Re-sorted rather than appended: a pr-level claim joins the tail at + // its rank, so the disclosure's named top entry is the tail's best + // claim, not merely its best ANCHORED claim. const collapsed = [ ...ranked.filter((claim) => !inlineClaims.has(claim)), ...prLevelCollapsed, - ]; + ].sort(rankClaims); const inlineList = [...inlineClaims]; const inline: PlannedComment[] = inlineList.map((claim) => ({ path: claim.path as string, @@ -796,10 +723,36 @@ export const runSubmissionCli = ( const collapsedNonBlockingOnly = !collapsed.some((entry) => isBlockingLabel(entry.label), ); + // The disclosure names the tail's top-ranked entry, not only the + // count: today's collapsed sections hid, verbatim, "the reply guard + // never fires" behind "Non-blocking observations (6)" on an + // approving review (Khan/actions#367), and a collapsed line only + // costs near-zero attention if the summary line says when it is + // worth spending more. `collapsed` re-sorts with rankClaims after + // the pr-level claims join it, so entry 0 is the best of the whole + // tail, pr-level included. + // The subject rides the tag, not only the location and label: the + // subject is the claim itself, and it is what tells a reader whether + // the expando is worth opening. It is model-authored text landing + // inside a , so it is HTML-escaped (attribution.ts's rule: + // a literal would break the collapse open) and truncated + // to keep the summary one line; the full subject is the first list + // entry inside the block anyway. The location gets the same + // backticks the section's own lines use. + const top = collapsed[0]; + const topSubject = escapeHtml( + top.subject.length > TOP_SUBJECT_MAX_CHARS + ? `${top.subject.slice(0, TOP_SUBJECT_MAX_CHARS)}...` + : top.subject, + ); + const topTag = + top.path !== undefined && top.line !== undefined + ? `; top: \`${top.path}:${top.line}\` ${top.label}: ${topSubject}` + : `; top: ${top.label}: ${topSubject}`; const summary = blockingOnly && collapsedNonBlockingOnly - ? `Non-blocking observations (${collapsed.length})` - : `Lower-confidence observations (${collapsed.length})`; + ? `Non-blocking observations (${collapsed.length}${topTag})` + : `Lower-confidence observations (${collapsed.length}${topTag})`; const section = [ "
", `${summary}`, @@ -824,8 +777,21 @@ export const runSubmissionCli = ( notes.push( blockingOnly && collapsedNonBlockingOnly ? `${collapsed.length} non-blocking claim(s) collapsed into the body (re-review blocking-only)` - : `${collapsed.length} claim(s) collapsed below the inline bar (cap ${MAX_INLINE_COMMENTS}, medium-confidence floor)`, + : `${collapsed.length} claim(s) collapsed below the inline bar (cap ${MAX_INLINE_COMMENTS}, medium-confidence floor, non-blocking budget ${nonBlockingBudget})`, ); + if (budgetShed > 0) { + notes.push( + `${budgetShed} non-blocking claim(s) collapsed over the inline budget (non-blocking budget ${nonBlockingBudget})`, + ); + } + if (nitpickShed > 0) { + // The no-silent-caps rule, per shed reason: the nitpick ban is + // its own posting rule, so its shed gets its own line rather + // than hiding inside the budget's. + notes.push( + `${nitpickShed} nitpick claim(s) collapsed (nitpick-class never posts inline)`, + ); + } } // The per-comment attribution footer (attribution.ts): which reviewer diff --git a/workflows/review/lib/version-footer.test.ts b/workflows/review/lib/version-footer.test.ts index ead34b29..a4cf756e 100644 --- a/workflows/review/lib/version-footer.test.ts +++ b/workflows/review/lib/version-footer.test.ts @@ -51,14 +51,31 @@ describe("renderVersionFooter", () => { reReviewMode: "scoped", blockingOnly: true, enabledReviewers: ["holistic", "completeness"], + nonBlockingInlineBudget: 5, }), ).toBe( wrapped( - "review-v1.13.0 | schema 2 | depth scoped | re-review scoped blocking-only | enable holistic,completeness", + "review-v1.13.0 | schema 2 | depth scoped | re-review scoped blocking-only | enable holistic,completeness | non-blocking-budget 5", ), ); }); + it("omits the non-blocking budget at its default (the footer states configuration, not defaults)", () => { + expect( + renderVersionFooter({ + version: "1.13.0", + schemaVersion: 2, + depth: "full", + reReviewMode: "full", + blockingOnly: false, + enabledReviewers: [], + nonBlockingInlineBudget: 3, + }), + ).toBe( + wrapped("review-v1.13.0 | schema 2 | depth full | re-review full"), + ); + }); + it("omits the blocking-only modifier when unset", () => { expect( renderVersionFooter({ @@ -68,6 +85,7 @@ describe("renderVersionFooter", () => { reReviewMode: "full", blockingOnly: false, enabledReviewers: [], + nonBlockingInlineBudget: null, }), ).toBe( wrapped("review-v1.13.0 | schema 2 | depth full | re-review full"), @@ -83,6 +101,7 @@ describe("renderVersionFooter", () => { reReviewMode: null, blockingOnly: true, enabledReviewers: [], + nonBlockingInlineBudget: null, }), ).toBe(wrapped("schema 2")); }); @@ -130,6 +149,19 @@ describe("runVersionFooterCli", () => { expect(runVersionFooterCli(fs, LIB)).toContain("depth scoped"); }); + it("carries a non-default routing.json budget into the footer", () => { + const files = fullStaging(); + files[`${REVIEW}/routing.json`] = JSON.stringify({ + reReviewMode: "scoped", + reReviewBlockingOnly: true, + enabledReviewers: ["holistic", "documentation"], + nonBlockingInlineBudget: 5, + }); + expect(runVersionFooterCli(makeFakeFs(files), LIB)).toContain( + "| non-blocking-budget 5", + ); + }); + it("omits the depth segment when dispatch has not run", () => { // No fallback to the planned depth: a plan is a guess about what // executed, and every unstateable segment drops rather than guesses. diff --git a/workflows/review/lib/version-footer.ts b/workflows/review/lib/version-footer.ts index 23b32bd0..d9ea742f 100644 --- a/workflows/review/lib/version-footer.ts +++ b/workflows/review/lib/version-footer.ts @@ -27,6 +27,7 @@ import {renderCollapsedFooter} from "./attribution"; import {FINDING_SCHEMA_VERSION} from "./finding-schema"; +import {DEFAULT_NON_BLOCKING_INLINE_BUDGET} from "./routing-config"; const REVIEW_DIR = "/tmp/gh-aw/review"; @@ -53,6 +54,10 @@ export type VersionFooterInputs = { blockingOnly: boolean; /** The ROUTING `enable` list (canonical order, from routing.json). */ enabledReviewers: string[]; + /** The ROUTING `non-blocking-budget` value; null drops the segment, and + * the default value is omitted too (the footer states configuration, + * not defaults). */ + nonBlockingInlineBudget: number | null; }; /** @@ -80,6 +85,12 @@ export const renderVersionFooter = (inputs: VersionFooterInputs): string => { if (inputs.enabledReviewers.length > 0) { segments.push(`enable ${inputs.enabledReviewers.join(",")}`); } + if ( + typeof inputs.nonBlockingInlineBudget === "number" && + inputs.nonBlockingInlineBudget !== DEFAULT_NON_BLOCKING_INLINE_BUDGET + ) { + segments.push(`non-blocking-budget ${inputs.nonBlockingInlineBudget}`); + } return renderCollapsedFooter(segments.join(" | ")); }; @@ -130,6 +141,7 @@ export const runVersionFooterCli = ( reReviewMode?: unknown; reReviewBlockingOnly?: unknown; enabledReviewers?: unknown; + nonBlockingInlineBudget?: unknown; } | undefined; const footer = renderVersionFooter({ @@ -151,6 +163,10 @@ export const runVersionFooterCli = ( (entry): entry is string => typeof entry === "string", ) : [], + nonBlockingInlineBudget: + typeof routing?.nonBlockingInlineBudget === "number" + ? routing.nonBlockingInlineBudget + : null, }); fs.writeFileSync(FOOTER_OUT, footer); return footer; diff --git a/workflows/review/review.md b/workflows/review/review.md index 87fb7f27..9f2b1065 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -899,10 +899,16 @@ per validated claim, rule quotes and suggestion fences included, human-thread skip lines and open-thread suppression already applied. The posting bar is code too: the plan ranks claims (blocking first, then confidence descending), posts at most 20 inline (matching this workflow's -`create-pull-request-review-comment` `max:`), and folds the remainder plus +`create-pull-request-review-comment` `max:`), spends the non-blocking inline +budget in ranked order (the ROUTING `non-blocking-budget` line, default 3; +blocking claims never count against it, `nitpick (non-blocking)` never posts +inline, and the documentation label is exempt because the documentation +autofix selects its work off posted threads), and folds the remainder plus any sub-medium-confidence claims into a single collapsed section riding the top-ranked comment (or the review body), so the plan never exceeds what the -engine will emit. Emit the plan's `comments` verbatim — one +engine will emit. The collapsed section's summary line names its top-ranked +entry, so an approving review cannot hide its best finding behind a bare +count. Emit the plan's `comments` verbatim — one `create-pull-request-review-comment` per entry, all in one batched turn; never add, drop, reword, or re-anchor one.