refactor(observability-map): share or pin the rest of the duplicated rules

Sweeping the package for the defect behind the two review threads: one
question answered by two pieces of code, where only one copy gets fixed.

Shared:
- the scannable-file predicate, copied into integration.test.ts and
  webappSymbols.test.ts after it was exported to stop mutationCorpus.test.ts
  copying it
- the FIX FIRST filter and sort, byte-identical in terminal.ts and
  prComment.ts, which already imports five helpers from it; failingIds is now
  scoredFailures plus a map
- normalizeSegment, in the test that validates SENSITIVE_SEGMENTS against the
  real tree. It was splitting segments with /_+$/, the regex that function's
  own comment says not to use
- the five bare-literal node kinds, written out in literalTruth three lines
  above the literalValue that already had them

Pinned:
- contextGap and auditGap, which redo by hand what checkContributions
  computes generically, on two headline figures with nothing saying they had
  to agree. Reverting either to a different denominator or numerator now goes
  red

Left alone, with reasons recorded in the sweep report: canRaise vs
tryBlockMayThrow, the two exact true-keyword folds, the two comment
extractors, the three means, ratio vs globalWithout, and the eight AST
helpers mutations.ts keeps its own copies of so the corpus can disagree with
the scanner.

Author: 1stvamp

internal-packages/observability-map/src/report/prComment.ts

@@ -1,10 +1,9 @@
 import type { MapReport, ScoredEntry } from "../score.js";
-import { SCORED_CHECK_IDS } from "../checks/index.js";
 import {
   auditLine,
   checkContributionLines,
   contextLine,
-  contextOnly,
+  fixFirst,
   delegatedLines,
   scoredFailures,
   unknownSuppressionLines,
@@ -40,8 +39,7 @@ const MAX_DELEGATED_ROUTES = 15;
 // Scored checks only, same exclusion terminal.ts's scoredFailures makes: audit-trail fails almost
 // every sensitive mutation today, so listing it per route would nag with something unfixable
 // instead of surfacing the route-specific gaps this column exists for.
-const failingIds = (e: ScoredEntry) =>
-  e.checks.filter((c) => SCORED_CHECK_IDS.includes(c.id) && c.status === "fail").map((c) => c.id);
+const failingIds = (e: ScoredEntry) => scoredFailures(e).map((c) => c.id);
 
 function scoreLine(head: MapReport, base: MapReport | null): string {
   const headline =
@@ -197,14 +195,7 @@ function whatChangedSection(head: MapReport, base: MapReport | null): string[] {
 
 function fixFirstSection(head: MapReport): string[] {
   const lines = ["FIX FIRST"];
-  const worst = head.entries
-    .filter((e) => scoredFailures(e).length > 0 && !contextOnly(e))
-    .sort(
-      (a, b) =>
-        Number(b.sensitive) - Number(a.sensitive) ||
-        a.score - b.score ||
-        a.fileName.localeCompare(b.fileName)
-    );
+  const worst = fixFirst(head.entries);
 
   for (const e of worst.slice(0, 3)) {
     const marks = e.sensitive ? " (sensitive)" : "";

internal-packages/observability-map/src/report/terminal.ts

@@ -27,6 +27,25 @@ export const scoredFailures = (e: ScoredEntry) =>
  * An entry that fails something else as well stays in the list with all of its findings, so a
  * route like `/account/tokens` still shows the request-context gap alongside the rest.
  */
+/**
+ * The routes the FIX FIRST list is drawn from, worst first: sensitive before not, then by score,
+ * then by name. Exported because `prComment.ts` renders the same list with different bullets and
+ * had a byte-identical copy of this filter and sort, in a file that already imports
+ * `scoredFailures` and `contextOnly` from here.
+ *
+ * `contextOnly` routes are left out because `request-context` fails almost everything, so a list
+ * headed by three of them tells a reader nothing they cannot read off the gap figure.
+ */
+export const fixFirst = (entries: ScoredEntry[]): ScoredEntry[] =>
+  entries
+    .filter((e) => scoredFailures(e).length > 0 && !contextOnly(e))
+    .sort(
+      (a, b) =>
+        Number(b.sensitive) - Number(a.sensitive) ||
+        a.score - b.score ||
+        a.fileName.localeCompare(b.fileName)
+    );
+
 export const contextOnly = (e: ScoredEntry) => {
   const failures = scoredFailures(e);
   return failures.length === 1 && failures[0]!.id === "request-context";
@@ -197,14 +216,7 @@ export function renderTerminal(report: MapReport): string {
     );
   }
 
-  const worst = report.entries
-    .filter((e) => scoredFailures(e).length > 0 && !contextOnly(e))
-    .sort(
-      (a, b) =>
-        Number(b.sensitive) - Number(a.sensitive) ||
-        a.score - b.score ||
-        a.fileName.localeCompare(b.fileName)
-    );
+  const worst = fixFirst(report.entries);
 
   lines.push("");
   lines.push("FIX FIRST");

internal-packages/observability-map/src/score.test.ts

@@ -561,3 +561,69 @@ export async function action() {
     expect(after.unmeasured).toBe(0);
   });
 });
+
+/**
+ * `contextGap` and `auditGap` are the same arithmetic `checkContributions` already does for every
+ * check, written out again by hand for two named ids: `map(find).filter(status)` for the context
+ * figure, `filter(some)` for the audit one, and a third spelling of "passed" for each. Three
+ * implementations of "applicable, and how many of those passed", and nothing said they had to
+ * agree, on the two figures the report puts in front of a reader as headline numbers.
+ *
+ * Pinned rather than shared. Collapsing them would mean the gap figures reading their check's row
+ * out of `checkContributions`, which is a fine refactor and a wider blast radius than the property
+ * is worth: what matters is that they cannot disagree, and an assertion says that without moving
+ * any code the renderers read.
+ */
+describe("the hand-rolled gap figures agree with the per-check contributions", () => {
+  const SOURCE = `import { prisma } from "~/db.server";
+import { logger } from "~/services/logger.server";
+export async function action({ params }) {
+  try {
+    return await prisma.apiKey.create({ data: { orgId: params.orgId } });
+  } catch (e) {
+    logger.error("failed", { orgId: params.orgId });
+    return null;
+  }
+}`;
+
+  // A sensitive mutation that DOES record an audit event, so `withAudit` is not simply
+  // `sensitiveMutations`. Without it the audit assertion held whatever the numerator counted.
+  const AUDITED = `import { prisma } from "~/db.server";
+import { startImpersonation } from "~/models/admin.server";
+export async function action({ request, params }) {
+  const session = await startImpersonation(request, params.userId);
+  await prisma.apiKey.create({ data: { orgId: params.orgId } });
+  return redirect("/", { headers: session });
+}`;
+
+  const report = buildReport(
+    [
+      scanFile("api.v1.orgs.$orgId.apikeys.ts", SOURCE)!,
+      scanFile("api.v1.tokens.ts", SOURCE)!,
+      scanFile("resources.impersonation.ts", AUDITED)!,
+      scanFile("healthcheck.ts", `export const loader = () => new Response("ok");`)!,
+    ],
+    []
+  );
+
+  const contribution = (id: string) => report.checkContributions.find((c) => c.id === id)!;
+
+  it("reports the same request-context denominator and numerator", () => {
+    expect(report.contextGap.applicable).toBe(contribution("request-context").applicable);
+    expect(report.contextGap.naming).toBe(contribution("request-context").passed);
+  });
+
+  it("reports the same audit-trail denominator and numerator", () => {
+    expect(report.auditGap.sensitiveMutations).toBe(contribution("audit-trail").applicable);
+    expect(report.auditGap.withAudit).toBe(contribution("audit-trail").passed);
+  });
+
+  // A denominator of zero would make both assertions above hold vacuously.
+  // Both assertions above hold vacuously on a zero denominator, and the audit one holds vacuously
+  // whenever every applicable route fails, since the two counts coincide.
+  it("measured something for both of them, with the audit numerator strictly between", () => {
+    expect(report.contextGap.applicable).toBeGreaterThan(0);
+    expect(report.auditGap.withAudit).toBeGreaterThan(0);
+    expect(report.auditGap.withAudit).toBeLessThan(report.auditGap.sensitiveMutations);
+  });
+});

internal-packages/observability-map/src/sensitivity.ts

@@ -122,7 +122,7 @@ export const SENSITIVE_SEGMENTS = [
  * written the way a reader would say it. A trailing underscore opts a route out of its parent
  * layout (`resources.impersonation_.view-as.ts`) and changes nothing about what the route does.
  */
-function normalizeSegment(segment: string): string {
+export function normalizeSegment(segment: string): string {
   // Trimmed by hand rather than with /_+$/, which backtracks polynomially on a run of underscores
   // and trips CodeQL. Nothing here is attacker-controlled (the input is a filename read off disk),
   // so this is about not spending a reviewer's attention on the alert.

internal-packages/observability-map/src/webappSymbols.test.ts

@@ -3,7 +3,13 @@ import { readdirSync, readFileSync } from "node:fs";
 import { join, resolve } from "node:path";
 import { AUDIT_SYMBOLS } from "./checks/auditTrail.js";
 import { GUARDS, SOFT_GUARDS } from "./checks/authBoundary.js";
-import { ANTICIPATED_SEGMENTS, SENSITIVE_SEGMENTS, SENSITIVE_SYMBOLS } from "./sensitivity.js";
+import { isScannableFile } from "./scan.js";
+import {
+  ANTICIPATED_SEGMENTS,
+  normalizeSegment,
+  SENSITIVE_SEGMENTS,
+  SENSITIVE_SYMBOLS,
+} from "./sensitivity.js";
 
 /**
  * Every name and every path segment the tool matches on must exist in the codebase it is pointed
@@ -82,7 +88,7 @@ function walkFiles(dir: string, out: string[] = []): string[] {
   for (const entry of readdirSync(dir, { withFileTypes: true })) {
     const path = join(dir, entry.name);
     if (entry.isDirectory()) walkFiles(path, out);
-    else if (/\.tsx?$/.test(entry.name) && !entry.name.endsWith(".d.ts")) out.push(path);
+    else if (isScannableFile(entry.name)) out.push(path);
   }
   return out;
 }
@@ -137,7 +143,10 @@ function routeSegments(): Set<string> {
   const segments = new Set<string>();
   for (const entry of readdirSync(ROUTES, { withFileTypes: true })) {
     for (const part of entry.name.replace(/\.tsx?$/, "").split(".")) {
-      segments.add(part.replace(/_+$/, ""));
+      // `sensitivity.ts`'s own normalizer. This validates the vocabulary that file matches on, so
+      // a segment has to be trimmed here exactly as it is trimmed there; the local `/_+$/` was
+      // also the regex `normalizeSegment`'s own comment says not to use.
+      segments.add(normalizeSegment(part));
     }
   }
   return segments;