fix(observability-map): narrow the required check and the report bot's comment lookup (#4507)
## Findings addressed - **Report bot edited the wrong comment.** The comment-lookup step matched on the marker body text with no author predicate, so it would silently PATCH a human's comment that happened to quote the marker (GitHub gates comment editing on write access, not authorship, so it never 403'd). Now constrained to `.user.login == "github-actions[bot]"`, the same identity `helm-prerelease.yml` already pins. - **A required check asserted facts about the whole webapp namespace.** `webappSymbols.test.ts` asserted that nobody anywhere in `apps/webapp` (walking locals, params, object keys) declares names like `createJWT`/`updateEnvVars`, so an unrelated PR naming a local variable failed a required check with a message pointing at nothing. Those negative self-tests move onto a package-owned fixture tree; the positive resolution assertions stay required (their absence rotted the tool before) but now name the list to edit. - **The suite ran twice on shared paths.** `obsmap` and `internal` path filters shared four generic paths (`package.json`, both lockfiles, `pr_checks.yml`), so any lockfile bump ran the observability-map suite in both jobs. Dropped from `obsmap` (where `internal` already covers them). The test that should have caught it only checked the package's own source path; it now asserts the two filters' path intersection is empty. - **PR-comment footer** reworded: it said the report gates nothing, which is true of the report but misled now that the tool's test suite does gate webapp PRs. Names both failure directions and where to read the rules. - **Nightly corpus** comment corrected (stale entry count; the failure-notification gap is documented, not silently implied). ## Review Two adversarial reviewers ran over the diff; both findings were verified and fixed: a hollow fixture assertion (a shared name satisfied either walker branch — now one name per declaration form, revert-confirmed) and a filter-intersection test that could be fooled by apostrophes in comment prose (now strips comment lines first). Full package suite green (877 passed), typecheck and format clean.
This commit is contained in:
Wes Mason
@@ -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
|
||||
|
||||
@@ -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/**'
|
||||
|
||||
Reference in New Issue
Block a user