fix(observability-map): narrow the required check and the report bot's comment lookup

.github/workflows/observability-map.yml

@@ -89,8 +89,10 @@ jobs:
           found=""
           ok=""
           for attempt in 1 2 3; do
+            # Matched by login, not .user.type == "Bot": other bots and apps on the same PR are
+            # also type Bot, and login is the exact identity this token's own comments carry.
             if found=$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" --paginate \
-              --jq '[.[] | select((.body // "") | startswith("<!-- observability-map-report -->"))][0].id // empty'); then
+              --jq '[.[] | select(.user.login == "github-actions[bot]" and ((.body // "") | startswith("<!-- observability-map-report -->")))][0].id // empty'); then
               ok=1
               break
             fi
@@ -116,23 +118,31 @@ jobs:
           } >> "$GITHUB_OUTPUT"
 
   # The tree-scale mutation corpus: every known laundering shape applied to the whole route tree,
-  # asserting the score does not rise. Roughly four and a half minutes for 45 entries, which is why
-  # it is gated out of the package's default `pnpm test` and run here instead. Unlike the report
-  # job below it has no token to lose, so it runs for fork PRs too, and unlike the report job it is
+  # asserting the score does not rise. Roughly five minutes for 53 entries, which is why it is
+  # gated out of the package's default `pnpm test` and run here instead. Unlike the report job
+  # below it has no token to lose, so it runs for fork PRs too, and unlike the report job it is
   # allowed to fail the build.
   #
   # Gated to this package's own paths rather than running on every route pull request. What the
   # corpus measures is the TOOL's resistance to laundering, and only an edit to the tool can weaken
-  # that, so a routes-only change was paying four and a half minutes of a 4x runner for a result
-  # that could not differ from the last one. It was also the worst kind of job to spend that on: a
-  # red x that fires on a large share of webapp pull requests, is allowed to fail, and gates
-  # nothing, which is the shape people learn to scroll past.
+  # that, so a routes-only change was paying five minutes of a 4x runner for a result that could
+  # not differ from the last one. It was also the worst kind of job to spend that on: a red x that
+  # fires on a large share of webapp pull requests, is allowed to fail, and gates nothing, which is
+  # the shape people learn to scroll past.
   #
   # What this gives up is real and small. A route landing a shape no corpus entry has seen can make
   # a known laundering mutation start paying, and that is now caught by the nightly rather than by
   # the pull request that caused it. Tree drift accrues over months, so a day is the right
   # granularity for it; the tool's own regressions, which are the ones a single commit can cause,
   # still gate per pull request.
+  #
+  # Nothing in this repo watches whether the nightly itself succeeds: no Slack webhook and no
+  # issue-on-failure step here or in e2e-webapp-auth-full.yml, the only other scheduled workflow, so
+  # there is no house pattern to follow. A broken corpus fails quietly on the 3am cron, red only in
+  # the Actions tab,
+  # for as long as nobody checks it. Wiring up a real notification needs infrastructure (a Slack
+  # webhook secret, at minimum) that does not exist here yet, so this is a known, unfixed gap
+  # rather than a fixed one.
   mutation-corpus:
     name: 🧬 Mutation corpus
     needs: changes

.github/workflows/pr_checks.yml

@@ -104,14 +104,16 @@ jobs:
             # `unit-tests-internal.yml` runs `turbo run test --filter "@internal/*"`, which picks up
             # @internal/observability-map and runs the same vitest suite. Listing them here as well
             # ran the suite twice on every PR touching them, which was this filter's own doing.
+            #
+            # Also deliberately NOT here: pr_checks.yml, package.json, pnpm-lock.yaml,
+            # pnpm-workspace.yaml. `internal` already lists all four, so a PR touching only one of
+            # them ran this suite twice for the same reason as above. Editing pr_checks.yml no
+            # longer runs this job live as a result; integration.test.ts still asserts on its text
+            # via the `internal` job.
             obsmap:
               - 'apps/webapp/app/**'
-              - '.github/workflows/pr_checks.yml'
               - '.github/workflows/unit-tests-observability-map.yml'
               - '.github/workflows/observability-map.yml'
-              - 'package.json'
-              - 'pnpm-lock.yaml'
-              - 'pnpm-workspace.yaml'
             cli:
               - 'packages/cli-v3/**'
               - 'packages/build/**'

internal-packages/observability-map/README.md

@@ -31,8 +31,13 @@ A pull request touching `apps/webapp/app/routes` or this package gets a sticky c
 against the tip of the base branch, with the score, what changed, and the current fix list. Every
 comment names the head commit it was rendered for, as a link to the compare range, because the
 comment is edited in place across pushes and otherwise says nothing about which push it reflects. It
-is report-only: nothing here fails the build or blocks a merge. See
-`.github/workflows/observability-map.yml`.
+is report-only: nothing in the comment or its score fails the build or blocks a merge. Separately,
+this package's test suite runs as a required check, on pull requests touching `apps/webapp/app` and
+on any other package through the internal job, and fails when a symbol the tool references stops
+resolving in one of the three trees it reads (`apps/webapp/app`, `packages/plugins/src`,
+`internal-packages/rbac/src`), or when the first route with an anticipated sensitive segment lands.
+Each failure names the list to update (`src/webappSymbols.test.ts`).
+See `.github/workflows/observability-map.yml`.
 
 The workflow runs on every pull request and applies the path list as a gate inside the job rather
 than as a `paths:` filter on the trigger. GitHub evaluates one of those per workflow, so a pull

internal-packages/observability-map/fixtures/webappSymbols/app/declares.ts

@@ -0,0 +1,17 @@
+// webappSymbols.test.ts's self-test tree. One name per declaration form, never two forms of the same
+// name: a single `has` assertion cannot say which branch of the walker answered it, so a shared name
+// would let a branch be deleted with the fixture test still green. `helper` covers the function
+// declaration, `createJWT` the local, `mintSessionToken` the member. `signJWT` is read and declared
+// nowhere, so a walker that collected references as declarations would find it.
+export function helper(kind: string): boolean {
+  const createJWT = kind === "jwt";
+  return createJWT;
+}
+
+export const api = {
+  mintSessionToken: false,
+};
+
+export function reads(payload: Record<string, unknown>): unknown {
+  return payload.signJWT;
+}

internal-packages/observability-map/fixtures/webappSymbols/routes/resources.secrets.ts

@@ -0,0 +1,2 @@
+// Only the file name matters: `routeSegments` reads directory entries, never content.
+export {};

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

@@ -128,6 +128,13 @@ describe("the report workflow's one source of the comment id", () => {
     expect(lookup).toBeDefined();
     expect(lookup).toContain("exit }'");
   });
+
+  it("only reconciles a comment github-actions[bot] posted, not anyone quoting the marker", () => {
+    const lookup = steps(job("changes")).find((step) =>
+      step.includes('startswith("<!-- observability-map')
+    )!;
+    expect(lookup).toContain('.user.login == "github-actions[bot]"');
+  });
 });
 
 /**
@@ -257,6 +264,32 @@ describe("the package's tests are wired into the gate", () => {
     ).toContain('--filter "@internal/*"');
   });
 
+  // The test above only checks the package's own source path, a different overlap that was already
+  // fixed. It has no way to catch a shared *generic* path (package.json, a lockfile, this workflow
+  // file itself) added to both filters, which is its own way to run the suite twice. Asserted as the
+  // actual set intersection, not another hardcoded path, so any future shared path fails this too.
+  it("shares no path with the internal filter, so the suite runs once", () => {
+    const text = read(PR_CHECKS);
+    // Comment lines are dropped before matching: an apostrophe in prose ("this filter's own doing")
+    // otherwise pairs with a real path's quote and swallows it, which would be a silent false pass.
+    const pathsOf = (name: string, next: string) =>
+      new Set(
+        [
+          ...text
+            .split(`            ${name}:`)[1]!
+            .split(`            ${next}:`)[0]!
+            .split("\n")
+            .filter((line) => !line.trim().startsWith("#"))
+            .join("\n")
+            .matchAll(/'([^']+)'/g),
+        ].map((m) => m[1])
+      );
+    const internal = pathsOf("internal", "obsmap");
+    const obsmap = pathsOf("obsmap", "cli");
+    const shared = [...obsmap].filter((p) => internal.has(p));
+    expect(shared).toEqual([]);
+  });
+
   it("is in the all-checks needs list, or it gates nothing", () => {
     const needs = read(PR_CHECKS).split("    needs:").pop()!.split("    if: always()")[0]!;
     expect(needs).toContain("- obsmap");

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

@@ -432,10 +432,15 @@ describe("renderPrComment", () => {
     expect(renderPrComment(head, null)).not.toContain("Warning: parse failures");
   });
 
-  it("footer names the report-only rule and the readme", () => {
+  it("footer names the report-only rule, the required suite that is not it, and the readme", () => {
     const head = buildReport([scanFile("api.v1.a.ts", cleanSource)!], []);
     const out = renderPrComment(head, null);
-    expect(out).toContain("Report only, nothing here gates the merge.");
+    expect(out).toContain("report-only and never gate the merge");
+    expect(out).toContain("a required test suite");
+    // Both directions. A footer naming only the rename case sends the author who added the first
+    // `secrets` route looking for a rename they never made.
+    expect(out).toContain("renames or removes a symbol");
+    expect(out).toContain("adds the first route with a segment");
     expect(out).toContain("internal-packages/observability-map/README.md");
   });
 });

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

@@ -25,6 +25,18 @@ export type CommitContext = {
 
 const SHORT_SHA_LENGTH = 7;
 
+/**
+ * Closes every comment this job posts. The second sentence is there because the package's own test
+ * suite does gate webapp pull requests, and a blocked author's first stop is this footer, which used
+ * to tell them nothing gates anything.
+ */
+const FOOTER =
+  "The score and findings here are report-only and never gate the merge. Separately, a required " +
+  "test suite keeps this tool's symbol and route lists in sync with the code they name, and can " +
+  "fail a pull request that renames or removes a symbol they reference, or that adds the first " +
+  "route with a segment they anticipate. Each failure names the list to edit. The rules and their " +
+  "reasons: internal-packages/observability-map/README.md.";
+
 /** Directly under the heading, because the comment is edited in place across pushes and the first
  * question about it is which push it reflects. */
 function commitLines(commit: CommitContext | undefined): string[] {
@@ -255,8 +267,7 @@ export function renderResolvedComment(commit?: CommitContext): string {
     "Nothing in this pull request moves the report any more. The findings an earlier push " +
       "reported are gone.",
     "",
-    "Report only, nothing here gates the merge. The rules and their reasons: " +
-      "internal-packages/observability-map/README.md.",
+    FOOTER,
   ].join("\n");
 }
 
@@ -275,8 +286,7 @@ export function renderScanFailedComment(commit?: CommitContext): string {
     "The scan failed for this run, so there is no report. Anything above is from an earlier push " +
       "and is stale. The workflow log has the error.",
     "",
-    "Report only, nothing here gates the merge. The rules and their reasons: " +
-      "internal-packages/observability-map/README.md.",
+    FOOTER,
   ].join("\n");
 }
 
@@ -319,10 +329,7 @@ export function renderPrComment(
     lines.push("```", "", "</details>", "");
   }
 
-  lines.push(
-    "Report only, nothing here gates the merge. The rules and their reasons: " +
-      "internal-packages/observability-map/README.md."
-  );
+  lines.push(FOOTER);
 
   const headFailures = head.parseFailures.length;
   const baseFailures = base?.parseFailures.length ?? 0;

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

@@ -53,6 +53,10 @@ const MAX_EXTERNAL_GUARDS = 2;
 
 const ROUTES = resolve(REPO, "apps/webapp/app/routes");
 
+/** A tree this package owns, for proving the walkers can answer no. Proving that on the live tree
+ * meant asserting nobody in the webapp ever declares certain names, even as a local variable. */
+const FIXTURES = resolve(__dirname, "../fixtures/webappSymbols");
+
 function walkFiles(dir: string, out: string[] = []): string[] {
   for (const entry of readdirSync(dir, { withFileTypes: true })) {
     const path = join(dir, entry.name);
@@ -62,7 +66,7 @@ function walkFiles(dir: string, out: string[] = []): string[] {
   return out;
 }
 
-function declaredNames(): Set<string> {
+function declaredNames(roots: string[]): Set<string> {
   const names = new Set<string>();
   const addBinding = (name: ts.BindingName) => {
     if (ts.isIdentifier(name)) {
@@ -74,7 +78,7 @@ function declaredNames(): Set<string> {
     }
   };
 
-  const files = ROOTS.flatMap((root) => walkFiles(root));
+  const files = roots.flatMap((root) => walkFiles(root));
   for (const file of files) {
     const sf = ts.createSourceFile(file, readFileSync(file, "utf8"), ts.ScriptTarget.Latest, false);
     const visit = (node: ts.Node) => {
@@ -108,9 +112,9 @@ function declaredNames(): Set<string> {
 }
 
 /** Every dot-separated piece of every route name, flat file or directory, e.g. `billing-limits`. */
-function routeSegments(): Set<string> {
+function routeSegments(dir: string): Set<string> {
   const segments = new Set<string>();
-  for (const entry of readdirSync(ROUTES, { withFileTypes: true })) {
+  for (const entry of readdirSync(dir, { withFileTypes: true })) {
     for (const part of entry.name.replace(/\.tsx?$/, "").split(".")) {
       // `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
@@ -122,29 +126,41 @@ function routeSegments(): Set<string> {
 }
 
 describe("the names the tool matches on exist in the webapp", () => {
-  const declared = declaredNames();
-  const segments = routeSegments();
+  const declared = declaredNames(ROOTS);
+  const segments = routeSegments(ROUTES);
 
   it("found a codebase to check against", () => {
     expect(declared.size).toBeGreaterThan(5000);
     expect(segments.size).toBeGreaterThan(100);
   });
 
   it("every sensitive symbol is declared somewhere", () => {
-    expect(SENSITIVE_SYMBOLS.filter((s) => !declared.has(s))).toEqual([]);
+    expect(
+      SENSITIVE_SYMBOLS.filter((s) => !declared.has(s)),
+      "renamed or removed? update SENSITIVE_SYMBOLS in " +
+        "internal-packages/observability-map/src/sensitivity.ts in the same PR"
+    ).toEqual([]);
   });
 
   // The list this test did not cover, and it had rotted completely: all three of `auditLog`,
   // `recordAudit` and `writeAuditEvent` were exported nowhere, so `audit-trail`'s pass branch could
   // not fire and the report said "No audit helper exists in the webapp" while
   // `models/admin.server.ts` was writing `impersonationAuditLog` rows on two paths.
   it("every audit symbol is declared somewhere", () => {
-    expect(AUDIT_SYMBOLS.filter((s) => !declared.has(s))).toEqual([]);
+    expect(
+      AUDIT_SYMBOLS.filter((s) => !declared.has(s)),
+      "renamed or removed? update AUDIT_SYMBOLS in " +
+        "internal-packages/observability-map/src/checks/auditTrail.ts in the same PR"
+    ).toEqual([]);
   });
 
   it("every auth guard is declared somewhere, or is a listed dependency method", () => {
     const names = [...GUARDS, ...SOFT_GUARDS];
-    expect(names.filter((g) => !declared.has(g) && !EXTERNAL_GUARDS.has(g))).toEqual([]);
+    expect(
+      names.filter((g) => !declared.has(g) && !EXTERNAL_GUARDS.has(g)),
+      "renamed or removed? update GUARDS or SOFT_GUARDS in " +
+        "internal-packages/observability-map/src/checks/authBoundary.ts in the same PR"
+    ).toEqual([]);
   });
 
   // The escape hatch is only worth having while it stays small.
@@ -155,25 +171,41 @@ describe("the names the tool matches on exist in the webapp", () => {
 
   it("every sensitive path segment names a real route segment", () => {
     const live = SENSITIVE_SEGMENTS.filter((s) => !ANTICIPATED_SEGMENTS.includes(s));
-    expect(live.filter((s) => !segments.has(s))).toEqual([]);
+    expect(
+      live.filter((s) => !segments.has(s)),
+      "renamed or removed the last route with this segment? update SENSITIVE_SEGMENTS in " +
+        "internal-packages/observability-map/src/sensitivity.ts in the same PR"
+    ).toEqual([]);
   });
 
-  // The escape hatch is only worth having while it is small and honest about itself.
+  // The escape hatch is only worth having while it is small and honest about itself. Kept required
+  // rather than moved to the nightly on purpose: the PR adding the first such route is the one whose
+  // author knows the route exists, and the segment is scored sensitive either way.
   it("every anticipated segment really does name nothing yet", () => {
-    expect(ANTICIPATED_SEGMENTS.filter((s) => segments.has(s))).toEqual([]);
+    expect(
+      ANTICIPATED_SEGMENTS.filter((s) => segments.has(s)),
+      "added the first route with this segment? move it from ANTICIPATED_SEGMENTS into the live " +
+        "list in internal-packages/observability-map/src/sensitivity.ts in the same PR"
+    ).toEqual([]);
   });
 
-  // The checker has to be able to fail. These run the same predicates over the names the last round
-  // shipped, which is what the test exists to have caught.
-  it("would reject the symbols that named nothing", () => {
-    for (const dead of ["setImpersonation", "createJWT", "signJWT", "updateEnvVars"]) {
-      expect(declared.has(dead)).toBe(false);
-    }
+  // The checker has to be able to fail. Proven on the fixture tree rather than the live one, which
+  // is where the earlier version of these tests asserted that no webapp file declares `createJWT`
+  // even as a local variable, and failed this suite on any pull request that did.
+  it("finds a fixture name however it is declared, and rejects one that is only read", () => {
+    const names = declaredNames([join(FIXTURES, "app")]);
+    // A name per declaration form, so no branch of the walker is covered only by another's name:
+    // deleting any one of the three fails here rather than in the live-tree assertions this fixture
+    // exists to replace.
+    expect(names.has("helper")).toBe(true);
+    expect(names.has("createJWT")).toBe(true);
+    expect(names.has("mintSessionToken")).toBe(true);
+    expect(names.has("signJWT")).toBe(false);
   });
 
-  it("would reject a guard name and a path segment that name nothing", () => {
-    expect(declared.has("requireNothingAtAll")).toBe(false);
-    expect(EXTERNAL_GUARDS.has("requireNothingAtAll")).toBe(false);
-    expect(segments.has("no-such-route-segment")).toBe(false);
+  it("finds a fixture route segment, and rejects the segment's own substring", () => {
+    const fixtureSegments = routeSegments(join(FIXTURES, "routes"));
+    expect(fixtureSegments.has("secrets")).toBe(true);
+    expect(fixtureSegments.has("secret")).toBe(false);
   });
 });