From 9ef63244e0cbf46a2e1e3aec6223a2d9fd8b151e Mon Sep 17 00:00:00 2001 From: Wes Mason Date: Tue, 4 Aug 2026 18:23:08 +0100 Subject: [PATCH 1/3] fix(observability-map): narrow the required check and the report bot's comment lookup Four findings from Nick's post-merge review of #4455. The report bot found its own comment by marker text with no author predicate, so it silently edited a human's comment that happened to quote the marker. GitHub gates comment editing on write access, not authorship, so this never failed loudly. Matched on .user.login now, the same identity helm-prerelease.yml pins through peter-evans/find-comment. The package's test suite asserted that nobody anywhere in the webapp declares createJWT, signJWT, setImpersonation or updateEnvVars, walking locals and parameters and object keys. Naming a local variable failed a required check with a message that pointed at nothing. Those two negative self-tests move onto a fixture tree, which proves the predicate can fail without holding the webapp's namespace hostage. The positive assertions stay required, because their absence is what rotted the tool before, but each now names the list to edit. The obsmap and internal path filters shared four generic paths, so any lockfile bump ran this suite twice. Dropped from obsmap, where internal already covers them. The test that should have caught it only checked the package's own source path, so it now asserts the filters' set intersection is empty. Editing pr_checks.yml alone no longer fires the obsmap job live, which is the accepted trade-off. The PR comment's footer said nothing here gates the merge. True of the report, misleading now that the suite gates webapp pull requests, so it names both failure directions and where to read the rules. --- .github/workflows/observability-map.yml | 26 +++++-- .github/workflows/pr_checks.yml | 10 ++- internal-packages/observability-map/README.md | 9 ++- .../fixtures/webappSymbols/app/declares.ts | 16 ++++ .../webappSymbols/routes/resources.secrets.ts | 2 + .../observability-map/src/integration.test.ts | 33 ++++++++ .../src/report/prComment.test.ts | 9 ++- .../observability-map/src/report/prComment.ts | 23 ++++-- .../src/webappSymbols.test.ts | 75 +++++++++++++------ 9 files changed, 157 insertions(+), 46 deletions(-) create mode 100644 internal-packages/observability-map/fixtures/webappSymbols/app/declares.ts create mode 100644 internal-packages/observability-map/fixtures/webappSymbols/routes/resources.secrets.ts diff --git a/.github/workflows/observability-map.yml b/.github/workflows/observability-map.yml index 3336492d5a..3f7506c6af 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,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 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/README.md b/internal-packages/observability-map/README.md index fe337cfc18..6fda85c627 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 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..2ef4e4d603 --- /dev/null +++ b/internal-packages/observability-map/fixtures/webappSymbols/app/declares.ts @@ -0,0 +1,16 @@ +// 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 the member branch be deleted with the fixture test still green. `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("