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,32 @@ 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. 53 entries, a couple of minutes of a runner and a good deal
+  # longer on a laptop, 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 a couple of 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/INTERNALS.md

@@ -497,4 +497,5 @@ steps degrade to a stale report, which at least stays one comment.
 
 The corpus runs on the package's own paths and on a schedule rather than on every route pull request,
 because it measures the tool's resistance to laundering, which only an edit to the tool can weaken,
-and it costs four and a half minutes. The nightly covers tree drift late rather than not at all.
+and it costs a couple of minutes of a runner. The nightly covers tree drift late rather than not at
+all.

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
@@ -86,7 +91,7 @@ find there.
 
 ### What stops it being gamed
 
-`src/mutationCorpus.test.ts` applies 44 semantics-preserving or handling-deleting rewrites to the
+`src/mutationCorpus.test.ts` applies 53 semantics-preserving or handling-deleting rewrites to the
 whole route tree in a temp copy and asserts three things for each: the published global does not
 rise, the mean over the routes measured in both runs does not rise, and for a semantics-preserving
 rewrite no individual route's score rises or drops out of the measured set. Every laundering shape a
@@ -97,14 +102,17 @@ score from 19 to 8, so the metric does not pay you for removing error handling.
 in `try { ... } catch (e) { throw e }` leaves the global at 19 and raises no route, so it does not
 pay you for adding error handling that does nothing either.
 
-One hole is open and the corpus says so. A catch over `try { 0; }` is refused, but `canRaise`
+Two holes are open and the corpus says so. A catch over `try { 0; }` is refused, but `canRaise`
 accepts any call, so `try { String(0); }` reads as real error handling: it takes the tree from 19 to
 44 and raises 224 routes. Telling an inert call from one that can throw needs types the scanner does
-not have. That entry, `dead-classifying-try-with-call`, runs as an expected failure with the
-residual written out beside it, so the claim is "43 rewrites are defended and here is the one that
-is not", never "unpaddable".
-
-The corpus takes about four and a half minutes, so it is gated behind `OBS_MAP_MUTATION_CORPUS=1`
+not have. The second, `dead-conjunction-instanceof-if`, is a dead condition rather than a dead arm:
+`selectsADistinctPath` folds the arm, and `literalTruth` treats `&&` as always null on purpose so a
+live guard is never read as dead, so widening that fold is a different rule needing its own
+measurement. Both run as expected failures with the residual written out beside them, so the claim
+is "51 rewrites are defended and here are the two that are not", never "unpaddable".
+
+The corpus takes minutes rather than seconds, under two on CI's runner and closer to eight on a
+laptop, so it is gated behind `OBS_MAP_MUTATION_CORPUS=1`
 and runs as its own CI job rather than in `pnpm test`. Run it if you change this package:
 
 ```bash

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;