fix(observability-map): stop audit-trail leaking into the PR-comment fix list and never fail the CI job on a render or upsert error

Author: 1stvamp

.github/workflows/observability-map.yml

@@ -56,15 +56,22 @@ jobs:
             echo "base scan failed or the worktree could not be added; falling back to no base" >&2
           fi
 
+      # continue-on-error: this job must never block a PR. A malformed head.json or a rendering
+      # bug would otherwise turn the job red the same way a base-scan failure would not, since that
+      # step already falls back to "-" instead of failing.
       - name: 📝 Render comment
+        continue-on-error: true
         run: |
           if [ "$(cat /tmp/base.json)" = "-" ]; then
             pnpm --filter @internal/observability-map exec tsx src/report/prCommentCli.ts /tmp/head.json - > /tmp/comment.md
           else
             pnpm --filter @internal/observability-map exec tsx src/report/prCommentCli.ts /tmp/head.json /tmp/base.json > /tmp/comment.md
           fi
 
+      # continue-on-error for the same reason: a transient gh api failure (rate limit, network)
+      # must not fail the job either. Worst case, the PR gets no comment this run.
       - name: 💬 Upsert PR comment
+        continue-on-error: true
         env:
           GH_TOKEN: ${{ github.token }}
           PR_NUMBER: ${{ github.event.pull_request.number }}

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

@@ -1,12 +1,17 @@
 import type { MapReport, ScoredEntry } from "../score.js";
+import { SCORED_CHECK_IDS } from "../checks/index.js";
 import { auditLine, contextLine, contextOnly, scoredFailures } from "./terminal.js";
 
 /** First line of every comment this job posts, so the upsert step can find its own comment again. */
 export const MARKER = "<!-- observability-map-report -->";
 
 const MAX_CHANGED_ROWS = 15;
 
-const failingIds = (e: ScoredEntry) => e.checks.filter((c) => c.status === "fail").map((c) => c.id);
+// 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);
 
 function scoreLine(head: MapReport, base: MapReport | null): string {
   const headline =

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

@@ -12,6 +12,22 @@ const processIo: Io = {
   err: (s) => process.stderr.write(s),
 };
 
+/** Reads and parses one report file, raising a message naming the file rather than letting an
+ * unreadable path or malformed JSON surface as a stack trace. */
+function readReport(path: string, label: string): MapReport {
+  let raw: string;
+  try {
+    raw = readFileSync(path, "utf8");
+  } catch {
+    throw new Error(`cannot read ${label}: ${path}`);
+  }
+  try {
+    return JSON.parse(raw) as MapReport;
+  } catch {
+    throw new Error(`${label} is not valid JSON: ${path}`);
+  }
+}
+
 /** `-` or a missing second arg means no base: the CI job falls back to this when the base scan
  * itself failed, so the comment still renders rather than the job going red. */
 export function main(argv: string[], io: Io = processIo): number {
@@ -24,9 +40,15 @@ export function main(argv: string[], io: Io = processIo): number {
     return 1;
   }
 
-  const head = JSON.parse(readFileSync(headPath, "utf8")) as MapReport;
-  const base: MapReport | null =
-    !basePath || basePath === "-" ? null : JSON.parse(readFileSync(basePath, "utf8"));
+  let head: MapReport;
+  let base: MapReport | null;
+  try {
+    head = readReport(headPath, "head report");
+    base = !basePath || basePath === "-" ? null : readReport(basePath, "base report");
+  } catch (error) {
+    io.err(`${error instanceof Error ? error.message : String(error)}\n`);
+    return 1;
+  }
 
   io.out(renderPrComment(head, base));
   io.out("\n");

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

@@ -70,6 +70,74 @@ describe("renderPrComment", () => {
     expect(out).toMatch(/\| \/api\/v1\/new \| new \| \d+ \|/);
   });
 
+  // Mirrors the guard report.test.ts has for the terminal renderer: audit-trail fails almost
+  // every sensitive mutation today (no audit helper exists), so it is a headline figure, not a
+  // per-route nag. A regression here previously let it leak into the "now failing" column.
+  it("does not list audit-trail among a new sensitive entry's failing checks", () => {
+    const sensitiveMutation = scanFile(
+      "api.v1.envvars.ts",
+      `import { prisma } from "~/db.server";
+       export async function action() {
+         try {
+           return await prisma.envVar.update({ where: {}, data: {} });
+         } catch (e) {
+           return null;
+         }
+       }`
+    )!;
+    const head = buildReport([sensitiveMutation], []);
+    const base = buildReport([], []);
+    const out = renderPrComment(head, base);
+
+    const row = out.split("\n").find((l) => l.startsWith("| /api/v1/envvars |"))!;
+    expect(row).toBeDefined();
+    expect(row).toContain("new");
+    expect(row).not.toContain("audit-trail");
+    expect(row).toMatch(/error-classification|auth-boundary|request-context/);
+  });
+
+  it("sorts a sensitive entry with a small drop above a non-sensitive entry with a large drop", () => {
+    const sensitiveSmallDropBase = scanFile("api.v1.auth.tokens.ts", cleanSource)!;
+    const sensitiveSmallDropHead = scanFile(
+      "api.v1.auth.tokens.ts",
+      `import { requireUserId } from "~/services/session.server";
+       import { logger } from "~/services/logger.server";
+       import { prisma } from "~/db.server";
+       export async function action({ request }) {
+         const userId = await requireUserId(request);
+         try { return await prisma.token.create({ data: { userId } }); }
+         catch (error) { logger.error("token create failed", { error }); throw error; }
+       }`
+    )!;
+
+    const notSensitiveLargeDropBase = scanFile(
+      "resources.busy.ts",
+      `import { logger } from "~/services/logger.server";
+       import { prisma } from "~/db.server";
+       export async function loader({ params }) {
+         try { return await prisma.thing.findMany(); }
+         catch (error) { logger.error("failed", { environmentId: params.envId, error }); throw error; }
+       }`
+    )!;
+    const notSensitiveLargeDropHead = scanFile(
+      "resources.busy.ts",
+      `import { prisma } from "~/db.server";
+       export async function loader() {
+         try { return await prisma.thing.findMany(); } catch (e) { return null; }
+       }`
+    )!;
+
+    const head = buildReport([sensitiveSmallDropHead, notSensitiveLargeDropHead], []);
+    const base = buildReport([sensitiveSmallDropBase, notSensitiveLargeDropBase], []);
+    const out = renderPrComment(head, base);
+
+    const sensitiveIndex = out.indexOf("/api/v1/auth/tokens");
+    const notSensitiveIndex = out.indexOf("/resources/busy");
+    expect(sensitiveIndex).toBeGreaterThan(-1);
+    expect(notSensitiveIndex).toBeGreaterThan(-1);
+    expect(sensitiveIndex).toBeLessThan(notSensitiveIndex);
+  });
+
   it("reports a removed entry as a count line, not a row", () => {
     const head = buildReport([scanFile("api.v1.auth.tokens.ts", cleanSource)!], []);
     const base = buildReport(

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

@@ -54,4 +54,30 @@ describe("prCommentCli", () => {
     expect(r.code).toBe(1);
     expect(r.err).toContain("usage:");
   });
+
+  it("exits 1 with a one-line message, not a stack trace, when head.json does not exist", () => {
+    const r = run(join(dir, "does-not-exist.json"));
+    expect(r.code).toBe(1);
+    expect(r.err.split("\n").filter(Boolean)).toHaveLength(1);
+    expect(r.err).toContain("cannot read head report");
+    expect(r.err).not.toContain(" at ");
+  });
+
+  it("exits 1 with a one-line message, not a stack trace, when head.json is malformed", () => {
+    const malformedPath = join(dir, "malformed.json");
+    writeFileSync(malformedPath, "{ not json");
+    const r = run(malformedPath);
+    expect(r.code).toBe(1);
+    expect(r.err.split("\n").filter(Boolean)).toHaveLength(1);
+    expect(r.err).toContain("head report is not valid JSON");
+    expect(r.err).not.toContain(" at ");
+  });
+
+  it("exits 1 with a one-line message when base.json is malformed", () => {
+    const malformedBasePath = join(dir, "malformed-base.json");
+    writeFileSync(malformedBasePath, "not json at all");
+    const r = run(headPath, malformedBasePath);
+    expect(r.code).toBe(1);
+    expect(r.err).toContain("base report is not valid JSON");
+  });
 });