diff --git a/.github/workflows/observability-map.yml b/.github/workflows/observability-map.yml index 3336492d5a..2d403166c4 100644 --- a/.github/workflows/observability-map.yml +++ b/.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(""))][0].id // empty'); then + --jq '[.[] | select(.user.login == "github-actions[bot]" and ((.body // "") | startswith("")))][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 diff --git a/.github/workflows/pr_checks.yml b/.github/workflows/pr_checks.yml index e3df9b416f..7de0531946 100644 --- a/.github/workflows/pr_checks.yml +++ b/.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/**' diff --git a/internal-packages/observability-map/INTERNALS.md b/internal-packages/observability-map/INTERNALS.md index c1dd52e2fb..eecae76493 100644 --- a/internal-packages/observability-map/INTERNALS.md +++ b/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. diff --git a/internal-packages/observability-map/README.md b/internal-packages/observability-map/README.md index fe337cfc18..563b80f757 100644 --- a/internal-packages/observability-map/README.md +++ b/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 diff --git a/internal-packages/observability-map/fixtures/webappSymbols/app/declares.ts b/internal-packages/observability-map/fixtures/webappSymbols/app/declares.ts new file mode 100644 index 0000000000..04702222ba --- /dev/null +++ b/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): unknown { + return payload.signJWT; +} diff --git a/internal-packages/observability-map/fixtures/webappSymbols/routes/resources.secrets.ts b/internal-packages/observability-map/fixtures/webappSymbols/routes/resources.secrets.ts new file mode 100644 index 0000000000..2ed014069d --- /dev/null +++ b/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 {}; diff --git a/internal-packages/observability-map/src/integration.test.ts b/internal-packages/observability-map/src/integration.test.ts index de2147505d..195636b260 100644 --- a/internal-packages/observability-map/src/integration.test.ts +++ b/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("