fix(observability-map): name the suppression directive honestly, and tighten the tests

obs-map-disable-next-line applied to the whole entry point, so a directive on
the last line of a file switched a check off for everything above it. Scoping
it to a line is not available, since a finding is attached to an entry point and
carries no line number to match against, and inventing a proximity rule would
silently drop legitimate suppressions. So the name is now obs-map-disable, which
is what it does. The old spelling is not honoured and a test says so.

The CONTEXT line now says how many of the collapsed entries are sensitive, 18 of
333, so a reader knows to open the JSON rather than trusting the list.

Three tests were passing by luck. The FIX FIRST ordering test sliced on a string
the report no longer prints, so its assertions ran against the whole tail. The
--no-write test never checked that no file was written, so it would have written
into the repo root if the flag broke. A suppression test asserted toContain('1')
against a report full of digits, and another never asserted the measured flag it
was named for.

README corrected: the score is 18, auth-boundary applies to 23 entry points
rather than 26 and gates on sensitivity before the one-hop limit, and the
invariant section now states both directions.

Author: 1stvamp

internal-packages/observability-map/README.md

@@ -4,7 +4,7 @@ Scores every webapp entry point on whether it could explain itself during an inc
 the ones worth fixing. An entry point is a Remix `loader` or `action` under
 `apps/webapp/app/routes`, 427 of them at the time of writing.
 
-The number it prints today is 19 out of 100. That is not a bug, and the rest of this file is mostly
+The number it prints today is 18 out of 100. That is not a bug, and the rest of this file is mostly
 about why you should believe it.
 
 ## Running it
@@ -20,7 +20,7 @@ suppresses. The single-route mode takes either the route path the report prints
 the file name (`api.v1.token.ts`). An exact match wins over the routes it is a prefix of, and an
 ambiguous prefix warns and names the alternatives rather than silently picking one.
 
-## What 19 means
+## What 18 means
 
 It is the mean score of the 412 entry points that had at least one applicable check, where an
 entry's score is the share of its applicable checks that passed. It is low because the webapp does
@@ -30,9 +30,18 @@ route and the request id and nothing about whose request it was.
 
 The score was 76 until we stopped crediting routes for the error handling they do not do. Emptying
 every catch clause in the tree used to score it 100, which meant the metric paid you for deleting
-error handling. Now removing the catches takes 19 down to 8, and removing the logs as well takes it
-to 2. If you change this package, keep that property: mutate the tree to remove error handling and
-check the score falls.
+error handling.
+
+Two invariants hold now, and both are asserted in `test/score.test.ts` rather than measured once:
+
+- **Removing error handling must not raise the score.** Deleting every catch clause takes 18 to 8,
+  and deleting the logs as well takes it to 2.
+- **Adding error handling that does nothing must not raise the score.** Wrapping every body in
+  `try { ... } catch (e) { throw e }` leaves it at 18, with no entry moving in either direction.
+  That mutation used to be worth 27 points across the tree, because a rethrow-only clause counted
+  as a pass while no catch at all was not-applicable, and the two are observationally identical.
+
+If you change this package, check both directions still hold.
 
 So the number is deliberately unflattering, and one platform change would move most of it. Nothing
 central attaches a tenant: `logger` pushes `{ requestId, path, host, method }` onto every line
@@ -43,8 +52,9 @@ rather than celebrating.
 
 ## The four checks
 
-- **error-classification**: does every catch clause decide what it caught, by rethrowing, by
-  branching on the error, or by guarding a parse it can answer for.
+- **error-classification**: does every catch clause decide what it caught, by branching on the
+  error or by guarding a parse it can answer for. A clause that only rethrows decides nothing and
+  is read as though there were no catch, so it neither passes nor fails.
 - **auth-boundary**: does a route handling credentials, tokens, billing or impersonation check who
   is asking.
 - **request-context**: when this entry point's failure is reported, is the tenant named.
@@ -62,6 +72,11 @@ are reported as a figure: the `AUDIT` and `CONTEXT` lines. 333 entry points fail
 fails `request-context` *and* something else keeps both findings and stays in the list, so
 `/account/tokens` still shows the whole picture.
 
+18 of those 333 are sensitive, including `/admin/impersonate`, the API-key regeneration route and
+four envvars routes, so the `CONTEXT` line says how many. Read them out of
+`observability-map.json`, where every entry keeps its full check results, rather than assuming the
+list is the whole story.
+
 `request-context` is still scored, unlike `audit-trail`. The gap it measures is real and the score
 is meant to show it. Only the presentation collapses.
 
@@ -94,11 +109,17 @@ support.
 ## Suppression
 
 ```ts
-// obs-map-disable-next-line auth-boundary -- public by design, see ADR 12
+// obs-map-disable auth-boundary -- public by design, see ADR 12
 ```
 
 The reason is mandatory: a suppression without one is ignored. The directive is read from comments
-only, line by line, so a string literal quoting it does not switch a check off.
+only, so a string literal quoting it does not switch a check off.
+
+It applies to the whole entry point, not to the line under it. It was called
+`obs-map-disable-next-line`, which was untrue in a way that mattered: a directive on the last line
+of a file switched a check off for everything above it. Genuine line scoping is not available,
+because a finding is attached to an entry point and carries no line number to match against, so the
+name was corrected instead. The old spelling is not honoured, and there is a test saying so.
 
 A suppression cannot raise a score. The suppressed check leaves the numerator and the denominator,
 and the result is capped by what the entry would have scored unsuppressed, so suppressing a failing
@@ -112,11 +133,16 @@ Read these before trusting a specific verdict.
 
 - **One hop, same file only.** If a loader delegates to a helper in the same file, that helper's
   statements, catches and calls count as the route's. A helper's own helpers do not, and nothing
-  imported from another module is ever opened. Most of what a route does is behind an import, which
-  is why `auth-boundary` applies to 26 entry points rather than 427.
+  imported from another module is ever opened. `auth-boundary` applies to 23 entry points: it gates
+  on sensitivity first, which is 26 routes, and the one-hop limit accounts for the other 3, which
+  hand their work to an imported helper and are reported as unverified rather than unguarded.
 - **Loggers are matched by spelling.** A call counts as logging when the callee reads `logger.*` or
   `log.*`. An aliased logger, one wrapped in a helper, or `console.error` is invisible, so a route
   can be reported as recording nothing while it records plenty.
+- **A catch that logs and rethrows reads as though it only rethrows.** The clause evidence cannot
+  say whether a clause does anything besides rethrow, so `error-classification` withholds credit
+  rather than granting it. Crediting it would reopen the free-points path a single `logger.error`
+  line wide.
 - **Only the first object-literal argument is read** for identifier fields, and only its property
   names. `logger.error("failed", ctx)` where `ctx` is a variable contributes nothing, and neither
   does a second object.

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

@@ -68,12 +68,14 @@ export function renderTerminal(report: MapReport): string {
 
   const { applicable, naming } = report.contextGap;
   if (applicable > 0) {
-    const collapsed = report.entries.filter(contextOnly).length;
+    const collapsed = report.entries.filter(contextOnly);
+    const sensitive = collapsed.filter((e) => e.sensitive).length;
     lines.push("");
     lines.push(
       `CONTEXT   ${naming} of ${applicable} entry points name a tenant on a failure path.` +
-        (collapsed > 0
-          ? ` ${collapsed} appear${collapsed === 1 ? "s" : ""} only here, not in the list below.`
+        (collapsed.length > 0
+          ? ` ${collapsed.length} appear${collapsed.length === 1 ? "s" : ""} only here, ` +
+            `${sensitive} of them sensitive, in the JSON rather than the list below.`
           : "")
     );
   }

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

@@ -2,8 +2,16 @@
  * The directive, and the reason that must follow it. The reason runs to the end of the line: `.`
  * does not match a newline, so a suppression on one line cannot pick up a reason from the next.
  * A trailing block-comment terminator is trimmed off so it does not end up inside the reason.
+ *
+ * It was `obs-map-disable-next-line`, which was a lie: a check applies to a whole entry point, so
+ * the directive did too, and one on the last line of a file switched a check off for everything
+ * above it. The honest options were to scope it to a line or to rename it, and scoping is not
+ * available: a `CheckResult` carries no line number, and neither does an `EntryPoint`, so there is
+ * nothing to match a line against. Scoping it would mean inventing a proximity rule that silently
+ * drops legitimate suppressions. So the name now says what it does. Real line scoping needs
+ * positions on the findings, which is scanner work.
  */
-const PATTERN = /obs-map-disable-next-line\s+([a-z-]+)\s+--\s+(.+)/;
+const PATTERN = /obs-map-disable\s+([a-z-]+)\s+--\s+(.+)/;
 
 /**
  * The comment part of a line, or null if there is none.

internal-packages/observability-map/test/cli.test.ts

@@ -1,5 +1,9 @@
+import { existsSync, rmSync } from "node:fs";
+import { resolve } from "node:path";
 import { main, type Io } from "../src/cli.js";
 
+const REPORT_FILE = resolve(__dirname, "../../../observability-map.json");
+
 const capture = () => {
   const out: string[] = [];
   const err: string[] = [];
@@ -51,10 +55,17 @@ describe("map <target>", () => {
 });
 
 describe("map", () => {
+  // The flag is the only thing standing between a test run and a file written into the repo root,
+  // so the test has to check the file, not just the exit code.
   it("renders the whole report without writing when asked not to", () => {
+    const existedBefore = existsSync(REPORT_FILE);
+    if (existedBefore) rmSync(REPORT_FILE);
+
     const r = run("--no-write");
+
     expect(r.code).toBe(0);
     expect(r.out).toContain("COVERAGE");
     expect(r.out).toContain("FIX FIRST");
+    expect(existsSync(REPORT_FILE)).toBe(false);
   });
 });

internal-packages/observability-map/test/report.test.ts

@@ -32,15 +32,14 @@ describe("renderTerminal", () => {
   it("surfaces suppressions so laundering is visible rather than silent", () => {
     const suppressed = scanFile(
       "api.v1.d.ts",
-      `// obs-map-disable-next-line error-classification -- deliberate, see ticket
+      `// obs-map-disable error-classification -- deliberate, see ticket
        import { prisma } from "~/db.server";
        export async function loader() {
          try { return await prisma.thing.findMany(); } catch (e) { return null; }
        }`
     )!;
     const out = renderTerminal(buildReport([suppressed], []));
-    expect(out).toMatch(/suppress/i);
-    expect(out).toContain("1");
+    expect(out).toMatch(/SUPPRESSED\s+1 check across 1 entry point/);
   });
 
   it("does not mention suppressions when there are none", () => {
@@ -118,7 +117,11 @@ describe("renderTerminal", () => {
       buildReport([sensitiveThirtyThree, sensitiveZero, notSensitiveZero, sensitiveAuditOnly], [])
     );
 
-    const fixFirst = out.slice(out.indexOf("FIX FIRST"), out.indexOf("already solid"));
+    // Slice to the end of the list, not to a string the I5 fix deleted: `indexOf` returned -1 for
+    // "already solid" and the assertions were quietly running against the whole tail.
+    const listEnd = out.indexOf("no findings:");
+    expect(listEnd).toBeGreaterThan(-1);
+    const fixFirst = out.slice(out.indexOf("FIX FIRST"), listEnd);
     const idxZero = fixFirst.indexOf("api.v1.envvars.ts");
     const idxThirtyThree = fixFirst.indexOf("api.v1.auth.tokens.ts");
     const idxNotSensitive = fixFirst.indexOf("resources.busy.ts");
@@ -238,6 +241,22 @@ describe("collapsing the house-style finding", () => {
     expect(out).toMatch(/CONTEXT\s+0 of 1 entry points name a tenant on a failure path/);
   });
 
+  // NEW-3. 18 of the collapsed entries are sensitive, including /admin/impersonate and the envvars
+  // routes, so the line has to say a reader should go and look at them.
+  it("says how many of the collapsed entries are sensitive", () => {
+    const sensitiveAndSilent = scanFile(
+      "api.v1.auth.jwt.ts",
+      `import { requireUserId } from "~/services/session.server";
+       import { prisma } from "~/db.server";
+       export async function loader({ request }) {
+         const userId = await requireUserId(request);
+         return prisma.token.findMany({ where: { userId } });
+       }`
+    )!;
+    const out = renderTerminal(buildReport([sensitiveAndSilent], []));
+    expect(out).toMatch(/1 appears? only here[^\n]*1 of them sensitive/i);
+  });
+
   it("says how many entries the collapse took out of the list", () => {
     const out = renderTerminal(buildReport([namesNobody(), namesNobodyAndSwallows()], []));
     expect(out).toMatch(/1 appears? only here/i);

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

@@ -54,7 +54,7 @@ describe("scoreEntry", () => {
   });
 
   it("counts a suppressed check as not-applicable", () => {
-    const suppressed = `// obs-map-disable-next-line error-classification -- health probe
+    const suppressed = `// obs-map-disable error-classification -- health probe
 ${RAW}`;
     const scored = scoreEntry(scanFile("api.v1.b.ts", suppressed)!);
     const ec = scored.checks.find((c) => c.id === "error-classification")!;
@@ -65,12 +65,16 @@ ${RAW}`;
     // error-classification would fail here; auth-boundary is not-applicable (not sensitive).
     // Suppressing the only applicable scored check must not be indistinguishable from an entry
     // point nothing applies to: it is still reported, just not scored on that axis.
-    const suppressed = `// obs-map-disable-next-line error-classification -- health probe
+    const suppressed = `// obs-map-disable error-classification -- health probe
 ${BUSY_AND_FAILING}`;
     const scored = scoreEntry(scanFile("api.v1.c.ts", suppressed)!);
     expect(scored.checks.find((c) => c.id === "error-classification")!.status).toBe(
       "not-applicable"
     );
+    // The point of the test, which it did not previously assert: request-context still applies, so
+    // the entry is still measured and still counted in the mean.
+    expect(scored.checks.find((c) => c.id === "request-context")!.status).toBe("fail");
+    expect(scored.measured).toBe(true);
   });
 
   // I1. `score = passed / applicable` meant removing a failing check from the denominator raised
@@ -88,7 +92,7 @@ export async function action({ request }) {
     const suppressed = scoreEntry(
       scanFile(
         "api.v1.auth.tokens.ts",
-        `// obs-map-disable-next-line error-classification -- deliberate, see ticket
+        `// obs-map-disable error-classification -- deliberate, see ticket
 ${source}`
       )!
     );
@@ -104,8 +108,8 @@ ${source}`
     const suppressed = scoreEntry(
       scanFile(
         "api.v1.b.ts",
-        `// obs-map-disable-next-line error-classification -- health probe
-// obs-map-disable-next-line request-context -- nothing to name here
+        `// obs-map-disable error-classification -- health probe
+// obs-map-disable request-context -- nothing to name here
 ${BUSY_AND_FAILING}`
       )!
     );
@@ -116,8 +120,8 @@ ${BUSY_AND_FAILING}`
     const suppressed = scoreEntry(
       scanFile(
         "api.v1.b.ts",
-        `// obs-map-disable-next-line error-classification -- health probe
-// obs-map-disable-next-line request-context -- nothing to name here
+        `// obs-map-disable error-classification -- health probe
+// obs-map-disable request-context -- nothing to name here
 ${BUSY_AND_FAILING}`
       )!
     );
@@ -167,7 +171,7 @@ describe("buildReport", () => {
       [
         scanFile(
           "api.v1.b.ts",
-          `// obs-map-disable-next-line error-classification -- health probe
+          `// obs-map-disable error-classification -- health probe
 ${BUSY_AND_FAILING}`
         )!,
         scanFile("api.v1.c.ts", BUSY_AND_FAILING)!,

internal-packages/observability-map/test/suppression.test.ts

@@ -3,26 +3,26 @@ import { suppressedChecks } from "../src/suppression.js";
 describe("suppressedChecks", () => {
   it("reads a suppression with its reason", () => {
     const m = suppressedChecks(
-      `// obs-map-disable-next-line error-classification -- liveness probe, deliberately silent
+      `// obs-map-disable error-classification -- liveness probe, deliberately silent
        export async function loader() { return { ok: true }; }`
     );
     expect(m.get("error-classification")).toBe("liveness probe, deliberately silent");
   });
 
   it("ignores a suppression with no reason", () => {
-    const m = suppressedChecks(`// obs-map-disable-next-line error-classification`);
+    const m = suppressedChecks(`// obs-map-disable error-classification`);
     expect(m.size).toBe(0);
   });
 
   it("ignores a suppression whose reason is only whitespace", () => {
-    const m = suppressedChecks(`// obs-map-disable-next-line error-classification --    `);
+    const m = suppressedChecks(`// obs-map-disable error-classification --    `);
     expect(m.size).toBe(0);
   });
 
   it("reads several suppressions in one file", () => {
     const m = suppressedChecks(
-      `// obs-map-disable-next-line error-classification -- liveness probe
-       // obs-map-disable-next-line request-context -- no identifiers exist here
+      `// obs-map-disable error-classification -- liveness probe
+       // obs-map-disable request-context -- no identifiers exist here
        export async function loader() { return { ok: true }; }`
     );
     expect(m.size).toBe(2);
@@ -36,7 +36,7 @@ describe("suppressedChecks", () => {
 
   it("does not carry a reason across lines", () => {
     const m = suppressedChecks(
-      `// obs-map-disable-next-line error-classification
+      `// obs-map-disable error-classification
        // some other comment -- with a dash
        export async function loader() { return 1; }`
     );
@@ -47,15 +47,15 @@ describe("suppressedChecks", () => {
   // merely quotes it, in a test fixture or an error message, silently suppressed a real check.
   it("ignores the directive inside a string literal", () => {
     const m = suppressedChecks(
-      `const example = "obs-map-disable-next-line error-classification -- not a real suppression";
+      `const example = "obs-map-disable error-classification -- not a real suppression";
        export async function loader() { return 1; }`
     );
     expect(m.size).toBe(0);
   });
 
   it("reads the directive from a block comment", () => {
     const m = suppressedChecks(
-      `/* obs-map-disable-next-line auth-boundary -- public by design, see ADR 12 */
+      `/* obs-map-disable auth-boundary -- public by design, see ADR 12 */
        export async function loader() { return 1; }`
     );
     expect(m.get("auth-boundary")).toBe("public by design, see ADR 12");
@@ -64,7 +64,7 @@ describe("suppressedChecks", () => {
   it("reads the directive from a jsdoc line", () => {
     const m = suppressedChecks(
       `/**
-        * obs-map-disable-next-line request-context -- nothing tenant-scoped here
+        * obs-map-disable request-context -- nothing tenant-scoped here
         */
        export async function loader() { return 1; }`
     );
@@ -73,9 +73,20 @@ describe("suppressedChecks", () => {
 
   it("ignores code that happens to follow a comment on the same line", () => {
     const m = suppressedChecks(
-      `const x = 1; // obs-map-disable-next-line error-classification -- fine
+      `const x = 1; // obs-map-disable error-classification -- fine
        export async function loader() { return x; }`
     );
     expect(m.get("error-classification")).toBe("fine");
   });
+
+  // The directive was called `-next-line` while applying to the whole entry point, so a comment on
+  // the last line of a file switched a check off for everything above it. Renamed rather than
+  // scoped, because a finding has no line number to scope it to. The old spelling is not honoured.
+  it("does not honour the old -next-line spelling", () => {
+    const m = suppressedChecks(
+      `// obs-map-disable-next-line error-classification -- stale directive
+       export async function loader() { return 1; }`
+    );
+    expect(m.size).toBe(0);
+  });
 });