fix(observability-map): make audit-trail true, and stop one dead statement clearing auth-scope

Four defects from the whole-branch review, all of them the same shape: a list or
a corpus that stopped keeping up with the code.

AUDIT_SYMBOLS named nothing. auditLog, recordAudit and writeAuditEvent are
exported nowhere in apps/webapp, packages/core or internal-packages, so the pass
branch could never fire, and both renderers printed "No audit helper exists in
the webapp" while models/admin.server.ts has been writing
prisma.impersonationAuditLog.create rows on two paths all along. The list is now
the three helpers that reach that write, and the AUDIT figure goes from 0 of 49
to 3 of 49. The pass branch had never been exercised against a real name either:
its only test imported auditLog from a module that does not exist.

It rotted because webappSymbols.test.ts covered every other name list in the
package and not this one. That came first, and it fails on the old list.

audit-trail was also the one check that did not follow the visibility rule the
README states as universal. It gated on sensitivity and hasAction alone, so on
resources.impersonation.ts, a four-statement body, auth-boundary declined to
judge because any guard would be behind the import while audit-trail accused the
route over an audit write behind that same import. It now takes the same
exemption, with a known writer read before it so presence still counts where
absence does not.

auth-scope was defeated by a dead statement. The predicate fired on any property
at all whose value was a caller id, wherever it sat, so prepending an unused
object holding user.id under an arbitrary key to every body raised settings.sso
and settings.team, the only two findings the check has ever produced and both
confirmed cross-org exposures. The property name now has to be an identity field
and the object has to be handed to a call. Two corpus entries cover both halves.

And suppress-every-check emitted directives for four checks, not five: auth-scope
was added a round after that entry was written. The corpus could not catch its
own omission, because leaving a check out of the sweep lowers the score rather
than raising it, so there is now an ungated registry assertion instead. Adding a
check without extending the corpus turns pnpm test red.

Author: 1stvamp

internal-packages/observability-map/src/checks/auditTrail.ts

@@ -1,14 +1,54 @@
 import type { CheckResult, EntryPoint } from "../types.js";
 import { classifySensitivity } from "../sensitivity.js";
+import { isTrivial } from "../triviality.js";
 
 const ID = "audit-trail";
 
-const AUDIT_SYMBOLS = ["auditLog", "recordAudit", "writeAuditEvent"];
+/**
+ * Calls that write a record of who did something.
+ *
+ * This list named nothing at all until it was checked. `auditLog`, `recordAudit` and
+ * `writeAuditEvent` are exported nowhere in apps/webapp, packages/core or internal-packages,
+ * so the pass branch below could never fire, every applicable route failed, and both renderers
+ * printed "No audit helper exists in the webapp" while `models/admin.server.ts` was writing
+ * `prisma.impersonationAuditLog.create({ action, adminId, targetId, ipAddress })` on two paths.
+ * The rot went unnoticed because `webappSymbols.test.ts` covered every other name list in the
+ * package and not this one. It covers this one now.
+ *
+ * All three names below reach that write: `redirectWithImpersonation` writes the START row,
+ * `clearImpersonation` writes the STOP row, and `startImpersonation` returns one or the other.
+ *
+ * Two of them are also in `SENSITIVE_SYMBOLS`, which is worth saying out loud because it looks like
+ * the circularity the sensitivity list was cleaned up to remove. It is not quite the same shape:
+ * `requireAdminApiRequest` was a pure mitigation counted as a hazard, whereas impersonation
+ * genuinely is the hazard AND genuinely writes the record. The consequence is real all the same, so
+ * here it is: a route made sensitive only by one of these calls cannot fail this check, because the
+ * call that put it in the cohort is the call that satisfies it.
+ *
+ * Matched against `importedNames` and `calleeNames`, so an import of one counts. Nothing matches
+ * the underlying `prisma.impersonationAuditLog.create` path directly: `calleeNames` records
+ * `create` for a member call, and no route in the tree writes the row itself.
+ */
+export const AUDIT_SYMBOLS = [
+  "redirectWithImpersonation",
+  "clearImpersonation",
+  "startImpersonation",
+];
 
 /**
- * Whether a sensitive mutation leaves a record of who did it. Nothing in the webapp writes one
- * today, so every applicable entry point fails: the check states the gap rather than measuring
- * variation between routes, which is why the score leaves it out.
+ * Whether a sensitive mutation leaves a record of who did it.
+ *
+ * Applicability follows the same rule as every other check, which it did not before: would this
+ * evidence necessarily be visible in the body if it existed? It gated on sensitivity and
+ * `hasAction` alone, so on `resources.impersonation.ts`, a four-statement body, `auth-boundary`
+ * declined to judge because any guard would be behind the import while this check accused the route
+ * over an audit write behind that same import. Two checks, opposite verdicts, one fact.
+ *
+ * So a trivial body is not-applicable here too. A delegating one is handled centrally by
+ * `scoreEntry`, which answers for every check before any of them runs, so there is no test for it
+ * here. The order matters and mirrors
+ * `auth-boundary`: a known audit call is read BEFORE the triviality exemption, because presence is
+ * evidence even where absence is not.
  */
 export const auditTrail = {
   id: ID,
@@ -18,8 +58,17 @@ export const auditTrail = {
       return { id: ID, status: "not-applicable", detail: "not a sensitive mutation" };
     }
     const symbols = new Set([...ep.importedNames, ...ep.calleeNames]);
-    return AUDIT_SYMBOLS.some((s) => symbols.has(s))
-      ? { id: ID, status: "pass", detail: "records an audit event" }
-      : { id: ID, status: "fail", detail: "sensitive mutation with no audit record" };
+    if (AUDIT_SYMBOLS.some((s) => symbols.has(s))) {
+      return { id: ID, status: "pass", detail: "records an audit event" };
+    }
+    if (isTrivial(ep)) {
+      return {
+        id: ID,
+        status: "not-applicable",
+        detail:
+          "cannot verify: no privileged work in the body, any audit write is behind an import",
+      };
+    }
+    return { id: ID, status: "fail", detail: "sensitive mutation with no audit record" };
   },
 };

internal-packages/observability-map/src/checks/authScope.ts

@@ -65,11 +65,13 @@ function builderExports(ep: EntryPoint): BuilderExport[] {
  * made this check agree with a route that resolves its target org from a URL slug and puts nothing
  * else in front of it.
  *
- * Applicable only where it is answerable: sensitive, builder-wrapped and not delegating. Outside
+ * Applicable only where it is answerable: sensitive and builder-wrapped. Outside
  * that it would be a second near-universal fail, which is the shape the `request-context` figure
  * already has and which the report has to collapse rather than list. There is no triviality test
  * here because there is nothing left for one to refuse: `isTrivial` answers false for any route
- * with an initializer callee, so a builder-wrapped route is never trivial.
+ * with an initializer callee, so a builder-wrapped route is never trivial. Nor is there a
+ * delegating test: `scoreEntry` answers not-applicable for a delegating entry before any check
+ * runs, so one here would be unreachable, and one WAS here saying otherwise.
  *
  * Three residuals, running in both directions.
  *
@@ -92,9 +94,6 @@ function builderExports(ep: EntryPoint): BuilderExport[] {
 export const authScope = {
   id: ID,
   run(ep: EntryPoint): CheckResult {
-    if (ep.delegating) {
-      return { id: ID, status: "not-applicable", detail: "delegates its body to another module" };
-    }
     if (!classifySensitivity(ep).sensitive) {
       return { id: ID, status: "not-applicable", detail: "not sensitive" };
     }

internal-packages/observability-map/src/checks/index.test.ts

@@ -1387,18 +1387,61 @@ describe("audit-trail", () => {
     expect(r.status).toBe("not-applicable");
   });
 
-  it("passes a sensitive mutation that records an audit event", () => {
+  // The pass branch had never been exercised against a real name: the fixture imported `auditLog`
+  // from `~/services/audit.server`, a helper and a module that both exist nowhere. All three names
+  // on the list now reach `prisma.impersonationAuditLog.create` in `models/admin.server.ts`.
+  it.each(["redirectWithImpersonation", "clearImpersonation", "startImpersonation"])(
+    "passes a sensitive mutation that records an audit event through %s",
+    (writer) => {
+      const r = run(
+        "audit-trail",
+        "admin.impersonate.tsx",
+        `import { ${writer} } from "~/models/admin.server";
+         import { prisma } from "~/db.server";
+         export async function action({ request }) {
+           const target = await prisma.user.findFirst({ where: { admin: false } });
+           const session = await ${writer}(request, target.id, "/");
+           return session;
+         }`
+      );
+      expect(r.status).toBe("pass");
+      expect(r.detail).toBe("records an audit event");
+    }
+  );
+
+  it("still fails a sensitive mutation that writes no record", () => {
     const r = run(
       "audit-trail",
       "api.v1.auth.jwt.ts",
-      `import { auditLog } from "~/services/audit.server";
-       import { prisma } from "~/db.server";
+      `import { prisma } from "~/db.server";
        export async function action({ request }) {
-         const token = await prisma.token.create({ data: {} });
-         await auditLog("token.created", { tokenId: token.id });
+         const token = await prisma.token.create({ data: { name: request.url } });
          return json(token);
        }`
     );
+    expect(r.status).toBe("fail");
+  });
+
+  // The coherence fix. `auth-boundary` declines to judge a trivial body because a guard would be
+  // behind the import; this check accused the same body over an audit write behind the same
+  // import. Same rule now, and presence is still read before the exemption, so a trivial body that
+  // does call a writer passes rather than sitting out.
+  it("declines to judge a trivial sensitive mutation, as auth-boundary does", () => {
+    const source = `import { doTheThing } from "~/models/admin.server";
+       export async function action({ request }) { return doTheThing(request, "/admin"); }`;
+    expect(run("audit-trail", "resources.impersonation.ts", source).status).toBe("not-applicable");
+    expect(run("auth-boundary", "resources.impersonation.ts", source).status).toBe(
+      "not-applicable"
+    );
+  });
+
+  it("still passes a trivial sensitive mutation that calls a writer", () => {
+    const r = run(
+      "audit-trail",
+      "resources.impersonation.ts",
+      `import { clearImpersonation } from "~/models/admin.server";
+       export async function action({ request }) { return clearImpersonation(request, "/admin"); }`
+    );
     expect(r.status).toBe("pass");
   });
 });
@@ -1642,6 +1685,21 @@ describe("auth-scope", () => {
     expect(r.detail).toContain("action (createActionPATApiRoute)");
   });
 
+  // Round D item 3, at the check level: the shape that cleared both real findings.
+  it("is not cleared by a dead object holding the caller id", () => {
+    const r = run(
+      "auth-scope",
+      "api.v1.orgs.$orgParam.members.ts",
+      `import { createActionPATApiRoute } from "~/services/routeBuilders/apiBuilder.server";
+       import { prisma } from "~/db.server";
+       export const action = createActionPATApiRoute({ method: "POST" }, async ({ user, params }) => {
+         const unused = { userId: user.id };
+         return json(await prisma.orgMember.deleteMany({ where: { slug: params.orgParam } }));
+       });`
+    );
+    expect(r.status).toBe("fail");
+  });
+
   it("is not applicable to a route that is not sensitive", () => {
     const r = run(
       "auth-scope",

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

@@ -4,6 +4,7 @@ import { dirname, join, resolve } from "node:path";
 import { scanDirectory } from "./scan.js";
 import { buildReport } from "./score.js";
 import { ADDITIVE_IDS, MUTATIONS, type Mutation } from "./mutations.js";
+import { CHECKS } from "./checks/index.js";
 
 /**
  * The tree-scale mutation corpus.
@@ -41,7 +42,7 @@ const ENABLED = process.env.OBS_MAP_MUTATION_CORPUS === "1";
  *
  * `dead-classifying-try-with-call` is the shape `dead-classifying-try` only looked like it closed.
  * `canRaise` accepts any call at all, so `try { String(0); }` reads as a clause guarding real work
- * and takes the tree from 15 to 42, raising 224 routes, exactly as `try { 0; }` did before it was
+ * and takes the tree from 19 to 44, raising 224 routes, exactly as `try { 0; }` did before it was
  * refused. Telling an inert call from one that can throw needs types the scanner does not have.
  * The docstrings in `scan.ts`, `types.ts` and `errorClassification.ts` say the rule refuses
  * `try { 0; }` and is defeated by one call, rather than claiming the family is closed.
@@ -195,6 +196,30 @@ function mutate(
   return { files: out, changed, sites };
 }
 
+/**
+ * Deliberately NOT gated behind `OBS_MAP_MUTATION_CORPUS`, unlike everything below it.
+ *
+ * This is the guard for the way the corpus actually failed. `auth-scope` was added a round after
+ * `suppress-every-check` was written and never added to its directive list, so the "a suppression
+ * cannot raise a score" invariant went untested at tree scale for the 19 routes that check applies
+ * to, while the entry's own description said "every check". Nothing noticed, because the entry
+ * still passed: omitting a check from the sweep leaves its failures in place, which lowers the
+ * score rather than raising it, so the corpus cannot catch its own omission by failing.
+ *
+ * A registry assertion can, and it belongs in the default suite so that adding a check without
+ * extending the corpus turns `pnpm test` red rather than a job nobody runs locally.
+ */
+describe("the corpus keeps up with the check registry", () => {
+  it("suppresses every registered check in the exhaustive sweep", () => {
+    const sweep = MUTATIONS.find((m) => m.id === "suppress-every-check")!;
+    const mutated = sweep.apply("api.v1.a.ts", "export const loader = () => null;")!.source;
+    const missing = CHECKS.map((c) => c.id).filter(
+      (id) => !mutated.includes(`obs-map-disable ${id} `)
+    );
+    expect(missing).toEqual([]);
+  });
+});
+
 const describeCorpus = ENABLED && existsSync(ROUTES) ? describe : describe.skip;
 
 /**
@@ -242,7 +267,7 @@ describeCorpus("mutation corpus over the real route tree", { timeout: ENTRY_TIME
    *
    * The guard the design asked for, verdict movement, cannot be used, though not for the reason an
    * earlier version of this comment gave. Plenty of defended entries move verdicts hard:
-   * `delete-every-catch` takes the tree from 15 to 2 and `dead-throw-after-switch` to 6. The
+   * `delete-every-catch` takes the tree from 19 to 8 and `dead-throw-after-switch` to 10. The
    * narrower true reason is that the IDEAL defended shape is one the scanner is blind to, and those
    * move nothing at all: `dead-if-false` and the ten entries beside it are defended precisely
    * because the tree comes out identical. Requiring movement would fail exactly the entries that

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

@@ -421,10 +421,14 @@ export const MUTATIONS: Mutation[] = [
   prependToEveryFile(
     "suppress-every-check",
     "prepend an obs-map-disable directive for every check to every file",
+    // Every check, which this said it was and was not: `auth-scope` was added a round after the
+    // entry was written and never added here, so the "a suppression cannot raise a score"
+    // invariant went untested at tree scale for the 19 routes it applies to.
     [
       "// obs-map-disable error-classification -- mutation corpus",
       "// obs-map-disable request-context -- mutation corpus",
       "// obs-map-disable auth-boundary -- mutation corpus",
+      "// obs-map-disable auth-scope -- mutation corpus",
       "// obs-map-disable audit-trail -- mutation corpus",
     ].join("\n")
   ),
@@ -522,6 +526,22 @@ export const MUTATIONS: Mutation[] = [
     "return obsMapResult.map(async () => {",
     "});"
   ),
+  // Round D item 3. `auth-scope` fired on any property at all whose value was a caller id, wherever
+  // it sat, so one dead statement at the head of a body cleared it. These are the two halves: the
+  // wrong property name, and the right property name in an object nothing is handed. Both raised
+  // `settings.sso` and `settings.team`, the only two findings the check has ever produced.
+  wrapEveryBody(
+    "dead-caller-scope-object",
+    "prepend a dead object holding the caller id under an arbitrary key to every route body",
+    "const obsMapDeadScope = { anything: user.id };",
+    ""
+  ),
+  wrapEveryBody(
+    "dead-caller-scope-userid",
+    "prepend a dead object holding the caller id under userId to every route body",
+    "const obsMapDeadUserId = { userId: user.id };",
+    ""
+  ),
   // C1a. `auth-boundary` matched `/^(require|authenticate)/`, so any callee at all beginning
   // `require` cleared a sensitive route. These two prepend the shapes that paid: an invented guard
   // and a real helper whose name merely contains "Authenticated" while it does a lookup by id.
@@ -850,6 +870,8 @@ export const ADDITIVE_IDS = [
   "registered-throw",
   "fake-require-guard",
   "fake-authenticated-lookup",
+  "dead-caller-scope-object",
+  "dead-caller-scope-userid",
 ];
 
 function isSingleConst(statement: ts.Statement): statement is ts.VariableStatement {

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

@@ -464,11 +464,11 @@ describe("hasDelta", () => {
   });
 
   it("is true when the audit gap closed, which no score reports", () => {
-    const audited = `import { auditLog } from "~/services/audit.server";
+    const audited = `import { clearImpersonation } from "~/models/admin.server";
        import { prisma } from "~/db.server";
        export async function action() {
          const token = await prisma.token.create({ data: {} });
-         await auditLog("token.created", { tokenId: token.id });
+         await clearImpersonation(request, "/admin");
          return json(token);
        }`;
     const unaudited = `import { prisma } from "~/db.server";

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

@@ -181,11 +181,11 @@ describe("rendering honestly when there is nothing to say", () => {
   it("does not claim no audit helper exists when one is in use", () => {
     const audited = scanFile(
       "api.v1.auth.tokens.ts",
-      `import { auditLog } from "~/services/audit.server";
+      `import { clearImpersonation } from "~/models/admin.server";
        import { prisma } from "~/db.server";
        export async function action() {
          const token = await prisma.token.create({ data: {} });
-         await auditLog("token.created", { tokenId: token.id });
+         await clearImpersonation(request, "/admin");
          return json(token);
        }`
     )!;

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

@@ -809,7 +809,7 @@ describe("scanFile: catch clause evidence", () => {
     // The ordering is the whole trick and it is easy to get backwards. The flag is raised at the
     // END of each statement, after that statement's own branch check. Raising it first makes every
     // deciding statement refuse itself, because `if (e instanceof X) return y` contains an exit by
-    // definition; that variant was measured and it takes the tree from 15 to 6 and accuses 78
+    // definition; that variant was measured against the pre-round-C tree and it takes it from 15 to 6, accusing 78
     // routes. This one leaves the real-tree report and all 240 clauses' evidence byte-identical.
     const BRANCH_EXITED: Array<[string, string]> = [
       ...EXITED,
@@ -889,7 +889,7 @@ describe("scanFile: catch clause evidence", () => {
 
   // S2. A clause whose try block cannot throw is unreachable, so it is not error handling and
   // nothing should be read off it. Crediting one was the largest hole ever found here: prepending
-  // this to a body took the real tree from 15 to 42 and raised 224 routes, because the 261 routes
+  // this to a body takes the real tree from 19 to 44 and raises 224 routes, because the routes
   // that catch nothing sat at `not-applicable` and a dead clause moved every one of them to `pass`.
   // `dead-classifying-try` in the mutation corpus is the tree-scale version.
   describe("a catch over a try block that cannot throw", () => {
@@ -2181,6 +2181,44 @@ describe("scanFile: the signals auth-scope reads", () => {
     expect(ep!.actionScopesByCaller).toBe(false);
   });
 
+  // Round D item 3. The predicate fired on any property at all whose value was a caller id, so one
+  // dead statement cleared the check. Both halves are now required: an identity property name, and
+  // an object that is handed to a call.
+  it("does not read a dead object holding the caller id as a scope", () => {
+    const arbitraryKey = scanFile(
+      "api.v1.orgs.ts",
+      `${PAT}
+       export const loader = createActionPATApiRoute({}, async ({ user }) => {
+         const unused = { anything: user.id };
+         return json(await prisma.org.findMany({ where: { slug: "x" } }));
+       });`
+    );
+    expect(arbitraryKey!.loaderScopesByCaller).toBe(false);
+
+    const identityKey = scanFile(
+      "api.v1.orgs.ts",
+      `${PAT}
+       export const loader = createActionPATApiRoute({}, async ({ user }) => {
+         const unused = { userId: user.id };
+         return json(await prisma.org.findMany({ where: { slug: "x" } }));
+       });`
+    );
+    expect(identityKey!.loaderScopesByCaller).toBe(false);
+  });
+
+  it("reads the caller id through any depth of nesting inside a call argument", () => {
+    const ep = scanFile(
+      "api.v1.orgs.ts",
+      `${PAT}
+       export const loader = createActionPATApiRoute({}, async ({ user }) => {
+         return json(await prisma.org.findMany({
+           where: { OR: [{ members: { some: { userId: user.id } } }] },
+         }));
+       });`
+    );
+    expect(ep!.loaderScopesByCaller).toBe(true);
+  });
+
   it("does not read a non-identity field off the caller as a scope", () => {
     const ep = scanFile(
       "api.v1.orgs.ts",