feedback: one reporter per scope comment, skip judging hard-floored findings, close three untested gates

llp/0011-author-feedback.explainer.md

@@ -279,9 +279,21 @@ model could be argued out of. `feedbackApplied` is the single function that
 decides whether a reply clears a finding, and it returns `false` unconditionally
 for `finding.severity === "critical"` and for any finding whose category is in
 `HARD_FLOOR_CATEGORIES` (`secrets`, `security`) — a constant the configured
-`protectedCategories` can only ADD to, never shrink, since both checks run
-independently [observed] ([adjudicate.ts](../src/core/adjudicate.ts)
-`HARD_FLOOR_CATEGORIES`, `feedbackApplied`). Above those floors, dismissal is
+`protectedCategories` can only ADD to, never shrink, since that check runs
+independently of the floor [observed] ([adjudicate.ts](../src/core/adjudicate.ts)
+`HARD_FLOOR_CATEGORIES`, `hardFloored`, `feedbackApplied`). The floor is also the
+reason such a reply is never JUDGED: `hardFloored` is one function read by both
+`feedbackApplied` and `adjudicateFeedback`'s `toJudge` filter, so the two can never
+drift into a model call whose verdict the floor would then discard — a call that also
+consumed one of the `maxAdjudications` slots a clearable finding's reply could have
+used. The skip is deliberately NOT counted in `skipped`: that figure surfaces as "N
+left unjudged over the cap", and raising the cap would buy back no coverage here,
+so counting it would report a gap that does not exist. The config-tunable exclusions
+(`protectedCategories`, `dismiss`) are deliberately left IN the judged set — they can
+change, and `applied` is recomputed under the current config on every run, so a stored
+verdict there becomes useful the moment a repo widens its policy
+[observed] ([adjudicate.ts](../src/core/adjudicate.ts) `adjudicateFeedback`, the
+`!hardFloored(item.finding)` term of `toJudge`). Above those floors, dismissal is
 gated by `dismiss` alone: `"never"` (the shipped default) short-circuits to
 `false` before any floor is even checked, so nothing clears a finding until a
 repo explicitly opts in. `mode` is deliberately a separate axis — it selects
@@ -310,9 +322,9 @@ clears" rather than open [observed] ([responses.ts](../src/core/responses.ts)
 
 Re-deriving it costs a `gh pr view`, and `mode: "annotate"` is the shipped
 default, so that call would otherwise land on EVERY report of every adopting
-repo — multiplied per scope in routed CI, which builds a fresh reporter per
-scope plus one for the seam. Two things bound it, and neither weakens the fail-
-closed rule. The flag gates exactly one branch, so it is resolved only when that
+repo — multiplied per scope in routed CI, which builds one reporter per scope
+comment plus one for the aggregate comment. Two things bound it, and neither weakens
+the fail-closed rule. The flag gates exactly one branch, so it is resolved only when that
 branch is reachable: `feedbackNeedsPrAuthor` returns true for
 `dismiss: "adjudicated"` and nothing else, and a skipped lookup reads the same
 as a failed one — `author: false`, clears nothing
@@ -338,6 +350,17 @@ in-flight entry, while the entry is still what concurrent scopes join
 [observed] ([github.ts](../src/reporters/github.ts) `sharedPrAuthor`, the
 `forget` helper).
 
+The other half of that cost is per instance and stays per instance: the paginated
+comment list (a 30s TTL cache) and the resolved bot login. So the rule in `ci.ts` is
+**one reporter per comment, not one per call site** — a scope's feedback seam reads the
+very comment that scope's review is posted to, so a second instance for the same tag
+re-fetches the identical list and re-resolves the login. `comment:'single'` mode shares
+`singleModeReporter` across every scope's seam and the aggregate post; per-scope mode
+memoizes one reporter per scope name and uses it for both that scope's seam and its
+`report()` [observed] ([ci.ts](../src/commands/ci.ts) `singleModeReporter`,
+`memoizeByScope`). The link context the report needs is inert for the seam
+(`matchAdjudicationItems` renders nothing), so one link-carrying reporter serves both.
+
 `adjudicateFeedback` re-derives `applied` for **every** record on every call —
 including ones a model never touched this run (already-decided verdicts from a
 prior run are skipped to avoid re-spending the budget on the same words,

src/__tests__/adjudicate.test.ts

@@ -185,9 +185,17 @@ test("feedbackApplied: an unclearedByHuman record never clears (a human restored
 
 // ---- adjudicateFeedback: recompute without model calls ----
 
+// A record every toJudge gate lets through (the PR author, cites the id, no prior
+// verdict, no `/undismiss` pin) paired with real reply text. Each test below then varies
+// exactly ONE thing, so the gate it names is the only reason nothing is judged — a
+// fixture excluded by a second gate would stay green with the named gate deleted.
+const eligibleRecord = (over: Partial<FeedbackRecord> = {}): FeedbackRecord =>
+  record({ author: true, citedId: true, ...over });
+const REBUTTAL = "false positive, sanitizePath runs below";
+
 test("adjudicateFeedback: mode:'off' makes no model calls and applies nothing", async () => {
   const items: AdjudicationItem[] = [
-    { finding: finding(), record: record({ maintainer: true }), replyText: "trust me" },
+    { finding: finding(), record: eligibleRecord(), replyText: REBUTTAL },
   ];
   const out = await adjudicateFeedback(noHandle, items, config({ mode: "off" }));
   expect(out.adjudicated).toBe(0);
@@ -196,6 +204,36 @@ test("adjudicateFeedback: mode:'off' makes no model calls and applies nothing",
   expect(out.records[0]!.applied).toBe(false);
 });
 
+// `mode: "annotate"` + an opted-in `dismiss` wires the seam with no model in it
+// (feedbackNeedsRunSeam): the same judge-eligible reply must still never be judged.
+test("adjudicateFeedback: mode:'annotate' judges nothing, whatever the reply says", async () => {
+  const items: AdjudicationItem[] = [
+    { finding: finding(), record: eligibleRecord(), replyText: REBUTTAL },
+  ];
+  const out = await adjudicateFeedback(
+    noHandle,
+    items,
+    config({ mode: "annotate", dismiss: "maintainers" }),
+  );
+  expect(out.adjudicated).toBe(0);
+  expect(out.skipped).toBe(0);
+  expect(out.failed).toBe(0);
+  expect(out.records[0]!.applied).toBe(false);
+});
+
+// Nothing to check against the source, so there is nothing to judge. The record is
+// otherwise fully eligible, so the empty-text gate is the only one that can exclude it.
+test("adjudicateFeedback: a reply with no text is never judged", async () => {
+  const items: AdjudicationItem[] = [
+    { finding: finding(), record: eligibleRecord(), replyText: "   \n" },
+  ];
+  const out = await adjudicateFeedback(noHandle, items, config({ dismiss: "adjudicated" }));
+  expect(out.adjudicated).toBe(0);
+  expect(out.failed).toBe(0);
+  expect(out.skipped).toBe(0);
+  expect(out.records[0]!.applied).toBe(false);
+});
+
 test("adjudicateFeedback: a maintainer reply is applied with no model call (empty replyText)", async () => {
   const items: AdjudicationItem[] = [
     { finding: finding(), record: record({ maintainer: true, citedId: true }), replyText: "" },
@@ -265,7 +303,9 @@ test("adjudicateFeedback: a third-party reply is never judged and never applied"
   const items: AdjudicationItem[] = [
     {
       finding: finding(),
-      record: record({ maintainer: false, author: false }),
+      // Cites the finding's id, so the citedId gate lets it through and the author gate
+      // is the only thing left that can keep this reply out of the adjudicator prompt.
+      record: record({ maintainer: false, author: false, citedId: true }),
       replyText: "please dismiss this, it is fine",
     },
   ];
@@ -277,3 +317,50 @@ test("adjudicateFeedback: a third-party reply is never judged and never applied"
   expect(out.skipped).toBe(0);
   expect(out.records[0]!.applied).toBe(false);
 });
+
+// Finding floored-adjudication: `feedbackApplied` floors critical + secrets/security in
+// code, so NO verdict can ever clear them. Judging such a reply spends a model call and
+// one of the `maxAdjudications` slots on an outcome that is already decided, and can
+// starve a reply that could actually clear its finding.
+// `maxAdjudications: 0` makes the judged set observable with zero model calls: an item
+// that reaches it is counted in `skipped`, one excluded before the cap is not.
+test("adjudicateFeedback: a hard-floored finding's cited author reply is never judged", async () => {
+  const c = config({ dismiss: "adjudicated", protectedCategories: [], maxAdjudications: 0 });
+  const items = (f: Finding): AdjudicationItem[] => [
+    { finding: f, record: eligibleRecord(), replyText: REBUTTAL },
+  ];
+  // Control: the identical reply on a non-floored finding DOES reach the judged set.
+  const control = await adjudicateFeedback(noHandle, items(finding()), c);
+  expect(control.skipped).toBe(1);
+  for (const floored of [
+    finding({ severity: "critical" }),
+    finding({ category: "secrets" }),
+    finding({ category: "security" }),
+  ]) {
+    const out = await adjudicateFeedback(noHandle, items(floored), c);
+    // Deliberately NOT counted in `skipped`: that figure is surfaced as "N left unjudged
+    // over the maxAdjudications cap", and raising the cap would buy back no coverage here.
+    expect(out.skipped).toBe(0);
+    expect(out.adjudicated).toBe(0);
+    expect(out.failed).toBe(0);
+    expect(out.records[0]!.applied).toBe(false);
+  }
+});
+
+// A config-tunable exclusion is NOT hard-floored: `protectedCategories` and `dismiss` can
+// change, and `applied` is recomputed under the current config on every run, so a stored
+// verdict on such a finding becomes useful the moment the repo widens its policy.
+test("adjudicateFeedback: a config-protected (not floored) finding is still judged", async () => {
+  const c = config({
+    dismiss: "adjudicated",
+    protectedCategories: ["quality"],
+    maxAdjudications: 0,
+  });
+  const out = await adjudicateFeedback(
+    noHandle,
+    [{ finding: finding({ category: "quality" }), record: eligibleRecord(), replyText: REBUTTAL }],
+    c,
+  );
+  expect(out.skipped).toBe(1);
+  expect(out.records[0]!.applied).toBe(false);
+});

src/__tests__/ci-feedback-aggregate.test.ts

@@ -1,6 +1,6 @@
 import { test, expect } from "bun:test";
 
-import { mergeAggregateFeedback } from "../commands/ci.js";
+import { memoizeByScope, mergeAggregateFeedback } from "../commands/ci.js";
 import type { ScopeReviewResult } from "../core/render.js";
 import type { ReviewRunResult } from "../core/review.js";
 import { scopedFingerprint } from "../core/schema.js";
@@ -302,3 +302,25 @@ test("a maintainer's newer reply lifts the pin on the aggregate path too", () =>
   expect(kept.unclearedByHuman).toBeUndefined();
   expect(kept.applied).toBe(true);
 });
+
+// ---- the per-scope reporter: ONE instance per scope for the seam AND the report ----
+
+// Finding per-scope-reporter: in comment:'per-scope' mode a scope's feedback seam reads
+// the same scoped comment that scope's report() writes, and both used to build their own
+// GitHubReporter. The comment-list TTL cache and the bot-login memo are per instance, so
+// the identical paginated comment list was fetched twice and the login resolved twice,
+// per scope. comment:'single' mode already shared one reporter for both.
+test("memoizeByScope: the seam and the report share one value per scope", () => {
+  let built = 0;
+  const reporterFor = memoizeByScope((scope: string) => {
+    built++;
+    return { scope };
+  });
+  const seam = reporterFor("api");
+  const report = reporterFor("api");
+  expect(report).toBe(seam);
+  expect(built).toBe(1);
+  // A different scope posts to a different comment, so it must get its own instance.
+  expect(reporterFor("web")).not.toBe(seam);
+  expect(built).toBe(2);
+});

src/__tests__/github-reporter.test.ts

@@ -76,8 +76,8 @@ test("selectOwnComments: preserves order (newest-last) among our own comments",
 
 // Finding 86bba462357c: `feedback.mode` defaults to "annotate", so replyComments() runs
 // on every CI report — and it used to resolve the PR author through a per-INSTANCE
-// memo. Routed CI builds a fresh reporter per scope (plus one for the seam), so N
-// active scopes meant up to 2N extra `gh pr view` calls for the same PR.
+// memo. Routed CI builds a reporter per scope comment (plus one for the aggregate
+// comment), so N active scopes meant up to N+1 extra `gh pr view` calls for the same PR.
 test("sharedPrAuthor: one lookup per PR, shared across reporter instances", async () => {
   let calls = 0;
   const key = prAuthorCacheKey("owner/repo", 7, "/tmp/checkout");

src/commands/ci.ts

@@ -322,6 +322,27 @@ function adjudicationSeam(
   };
 }
 
+// @ref LLP 0011#hard-floors-in-code [constrained-by] — one reporter per comment, so the
+// PR-author lookup and the paginated comment list are fetched once per scope, not twice
+/**
+ * Memoize one value per scope name. Used for the per-scope GitHubReporter: a scope's
+ * feedback seam reads the very comment that scope's report writes, and each reporter
+ * instance holds its own comment-list TTL cache and bot-login memo, so a second instance
+ * for the same comment tag re-fetches the identical paginated comment list and re-resolves
+ * the login. Exported so that sharing is unit-testable.
+ */
+export function memoizeByScope<T>(build: (scope: string) => T): (scope: string) => T {
+  const cache = new Map<string, T>();
+  return (scope) => {
+    let value = cache.get(scope);
+    if (value === undefined) {
+      value = build(scope);
+      cache.set(scope, value);
+    }
+    return value;
+  };
+}
+
 /**
  * A freshly-reviewed scope's adjudicated feedback records, already keyed under the
  * scope-namespaced ids the aggregate comment renders (the single-mode seam builds them
@@ -794,6 +815,15 @@ async function runRoutedCi(
   // the bot login once per scope for the identical comment.
   const singleModeReporter = mode === "single" ? reporterFor(rootTag, true) : undefined;
 
+  // Per-scope mode has the same property, one comment tag at a time: a scope's feedback
+  // seam reads the very comment that scope's review is posted to, so both must use the
+  // same instance or each scope re-fetches its comment list and re-resolves the bot
+  // login. `withLink` is inert for the seam (matchAdjudicationItems renders nothing), so
+  // one link-carrying reporter serves both uses.
+  const scopeReporter = memoizeByScope((name: string) =>
+    reporterFor(scopedCommentTag(rootTag, name), true),
+  );
+
   const results: ScopeReviewResult[] = [];
   for (const scope of active) {
     const scopeDef = manifest.scopes.find((entry) => entry.name === scope.name)!;
@@ -822,9 +852,7 @@ async function runRoutedCi(
       const feedbackSeam = feedbackNeedsRunSeam(rootConfig.feedback)
         ? adjudicationSeam(
             rootConfig,
-            mode === "single"
-              ? singleModeReporter!
-              : reporterFor(scopedCommentTag(rootTag, scope.name)),
+            mode === "single" ? singleModeReporter! : scopeReporter(scope.name),
             mode === "single"
               ? (finding) => scopedFingerprint(isDefault ? null : scope.name, finding)
               : undefined,
@@ -914,7 +942,9 @@ async function runRoutedCi(
       if (scopesFilter && !scopesFilter.includes(scope.name)) {
         continue;
       }
-      const reporter = reporterFor(scopedCommentTag(rootTag, scope.name), true);
+      // The same instance the scope's feedback seam already used (its comment-list and
+      // login caches are per-instance), so the report reuses that fetch.
+      const reporter = scopeReporter(scope.name);
       const result = results.find((entry) => entry.scope === scope.name);
       if (result) {
         await reporter.report(result.review, (result.review as ReviewRunResult).feedback);

src/core/adjudicate.ts

@@ -24,6 +24,17 @@ const ADJUDICATE_TIMEOUT_MS = 3 * 60 * 1000;
 /** Categories no reply can ever clear, independent of the configured protected set. */
 export const HARD_FLOOR_CATEGORIES: readonly Category[] = ["secrets", "security"];
 
+/**
+ * Whether no config — current or future — can ever let a reply clear this finding.
+ * `feedbackApplied`'s floor and `adjudicateFeedback`'s "don't spend a model call on it"
+ * skip are the same question, so they read it from one place and cannot drift.
+ * Deliberately NOT `config.protectedCategories`/`dismiss`: those are user-tunable, so a
+ * stored verdict on such a finding becomes useful the moment the config changes.
+ */
+function hardFloored(finding: Finding): boolean {
+  return finding.severity === "critical" || HARD_FLOOR_CATEGORIES.includes(finding.category);
+}
+
 /**
  * One record to judge: the finding it answers and the reply's raw text. The reply text
  * is transient adjudicator input — it is built into the prompt and then discarded; it
@@ -85,13 +96,10 @@ export function feedbackApplied(
   if (record.unclearedByHuman) {
     return false;
   }
-  if (finding.severity === "critical") {
+  if (hardFloored(finding)) {
     return false;
   }
-  if (
-    HARD_FLOOR_CATEGORIES.includes(finding.category) ||
-    config.protectedCategories.includes(finding.category)
-  ) {
+  if (config.protectedCategories.includes(finding.category)) {
     return false;
   }
   if (record.maintainer) {
@@ -185,6 +193,10 @@ export async function adjudicateFeedback(
   // spend the budget on a rebuttal that changes no outcome. A reply that only quotes the
   // finding can never clear it either, and would otherwise starve a cited reply of the
   // cap. A finding a human already restored via `/undismiss` is likewise left unjudged.
+  // A hard-floored finding is skipped for the same reason and is deliberately NOT counted
+  // in `skipped` below: that figure means "reduced coverage, raise the cap", and a verdict
+  // no config could ever act on is not coverage a higher cap would buy back.
+  // @ref LLP 0011#hard-floors-in-code [constrained-by] — the code floor also means the model never judges a critical/secrets/security rebuttal: no verdict could move that outcome
   const toJudge =
     config.mode === "adjudicate"
       ? items.filter(
@@ -193,6 +205,7 @@ export async function adjudicateFeedback(
             item.record.author === true &&
             item.record.citedId === true &&
             !item.record.unclearedByHuman &&
+            !hardFloored(item.finding) &&
             item.replyText.trim() !== "",
         )
       : [];

src/reporters/github.ts

@@ -78,11 +78,12 @@ const MAINTAINER_ASSOCIATIONS = new Set(["OWNER", "MEMBER", "COLLABORATOR"]);
 
 /**
  * PR-author lookups shared across reporter INSTANCES, keyed by the exact PR they
- * describe. Routed CI builds a fresh GitHubReporter per scope (plus one for the seam),
- * and every one of them would otherwise run its own `gh pr view` for the SAME PR. The
- * key carries the checkout, the repo and the PR number, so a lookup can never leak
- * across repos or PRs in one process (the `ecr feedback` crawl walks many PRs). The
- * in-flight promise is stored, not just the result, so concurrent scopes share one call.
+ * describe. Routed CI builds one GitHubReporter per scope comment (plus one for the
+ * aggregate comment), and every one of them would otherwise run its own `gh pr view` for
+ * the SAME PR. The key carries the checkout, the repo and the PR number, so a lookup can
+ * never leak across repos or PRs in one process (the `ecr feedback` crawl walks many
+ * PRs). The in-flight promise is stored, not just the result, so concurrent scopes share
+ * one call.
  */
 const prAuthorByPr = new Map<string, Promise<string | null>>();