fix(ci): perf-k6 was an invalid workflow file, and nothing lints workflows - #441
Conversation
Correction: the "0 findings with shellcheck" claim in the description was wrongThe Installed shellcheck 0.11.0 locally, reproduced all five, and fixed them instead of lowering the gate:
All three are behaviour-preserving. The more useful outcome: the job now asserts
|
…flows `perf-k6.yml` had failed on every push for weeks. It was not the load-test job running and breaking — its `on:` never declared `push` at all. The `perf-gate` job's `if:` referenced `env.PERF_K6_FULL_RUN`, and the `env` context does not exist in a job-level `if:` (only `github`, `inputs`, `needs`, `vars`). That does not evaluate to empty: it makes the entire file unparseable, so GitHub never resolved `name:` or any job and recorded a bare failed run against every event, `on:` filters included. The tell was the API reporting the run's `name` as the file path with an empty job list. Fixed by using `vars.PERF_K6_FULL_RUN`, which is available at job level. The workflow now honours `on:` and stops running on push. Set the repo variable `PERF_K6_FULL_RUN=true` to let the nightly schedule run the full compose + k6 job; unset stops the scheduled run after `validate-scripts`. Dropped the workflow-level `env` entry, which nothing else read. The more useful fix is the second one. The reason a broken workflow stayed broken for weeks is that nothing checked this class: an unparseable workflow looks, from the outside, identical to a workflow that ran and failed — and because it is not a required check, no gate objected. New `workflow-lint` job runs `actionlint`, pinned by version and SHA-256 rather than adding another third-party action SHA to keep current. Zero findings across `.github/workflows` today, shellcheck included over the `run:` blocks that drive Postgres, Redis and the deploy steps. Not adding a DoD section for this: the guard is mechanical and self-enforcing, which DoD 6 prefers over a checklist item. Also raises the `@types/react-dom` override from `19.2.4` to `19.2.5`. The pin is exact, so it wins over whatever the manifests declare — which is why the pending minor-and-patch group bump (manifests to `^19.2.5`) fails `drift:check`: the ranges stopped intersecting. Raising the override first clears that without the group PR touching it, and `19.2.5` still satisfies the current `^19.2.4` declarations, so it changes nothing for anyone not on the group bump. Verified in the lockfile: every importer now resolves 19.2.5, and the whole lockfile diff is that propagation. Backlog B31 closes with the root cause recorded. actionlint 0 findings, `pnpm check:all` green, typecheck 17/17, build 9/9, `pnpm audit --prod --audit-level high` clean, `pnpm install --frozen-lockfile` verified against a fresh checkout of main.
…faced
The `workflow-lint` job failed on its first real run, and the failure was
correct. actionlint runs shellcheck over `run:` blocks only when the binary is
on PATH and **skips it silently** otherwise — shellcheck was not installed on
my machine, so the local run reported clean while the runner (ubuntu-latest
ships it) found five pre-existing issues. Installed shellcheck 0.11.0 locally,
reproduced all five, fixed them rather than lowering the gate:
- `deploy-cms.yml` — `run: pnpm ... run build:${TARGET_ENV}` and the matching
`deploy:` step, both unquoted (SC2086). Behaviourally a no-op today because
TARGET_ENV is `production`/`staging`, but it is the exact class of quoting bug
the comment on this job claims to care about.
- `perf-k6.yml` — two `for i in $(seq ...)` readiness loops that never use `i`
(SC2034), now `for _`.
- `release.yml` — three consecutive `>> release-notes.md` redirects (SC2129),
now one grouped `{ ... } >> release-notes.md`. Output is byte-identical:
`git log --pretty=format:` emits no trailing newline, so the closing `echo`
is still there.
Also makes the gate unable to weaken quietly: the job now asserts
`shellcheck --version` before running actionlint. Without that, a future runner
image dropping shellcheck would silently stop checking every `run:` block —
which is the same failure mode as B30, and I just demonstrated it on myself.
actionlint exit 0 across `.github/workflows` with shellcheck actually present.
Rebasing this branch hit a conflict because #434 and #436 both assigned `B30`, to unrelated findings — a `localStorage` flake in `analytics-consent.test.tsx` and the dead `version-check.mjs` guard. Whichever merged second had to be renumbered by hand (`B33`), and nothing announced the collision; it showed up only as a rebase conflict. That is precisely the failure `registry:check` already prevents for the `#` column of the Setup Impact Registry, which once carried duplicate #20/#31/#32 rows. The backlog table was simply never included. It matters more than a cosmetic id clash: backlog ids are referenced *by id* from other rows ("Nối tiếp B13", "cùng class với B10", "Xem B24") and from CHANGELOG entries, so a silent renumber breaks cross-references that no test covers. `check-registry-numbering.mjs` now walks both tables — `#` in setup-impact.md and `B<n>` in out-of-scope-backlog.md — reporting the offending line numbers, the next safe id, and a reminder to keep whichever occurrence other rows cite. Both tables still fail closed if their shape changes and the scan parses zero rows, so the guard cannot quietly stop guarding (the B30/B33 lesson, applied to the guard itself). Verified both directions: injecting a duplicate `B30` exits 1 naming lines 53 and 56 and suggesting B34; removing it exits 0. Already reachable through `pnpm check:all`, so pre-commit and CI pick it up with no wiring change. Logged as B34 (fixed).
fbad9da to
5e72dba
Compare
Rebased onto
|
1 participant
Three things, in order of how much they matter.
1.
perf-k6.ymlwas invalid, not failingIt had failed on every push for weeks (30+ consecutive runs, predating the dependabot batch). The obvious reading — "the k6 load job needs a load environment it doesn't have in CI" — is wrong. Its
on:never declaredpushat all:The actual cause is one expression.
perf-gate's job-levelif:readenv.PERF_K6_FULL_RUN, and theenvcontext does not exist in a job-levelif:— onlygithub,inputs,needs,varsdo. That does not evaluate to empty and skip the job. It makes the entire file unparseable, so GitHub never resolvesname:or any job, and records a bare failed run against whatever event fired —on:filters included, because it never got far enough to read them.The diagnostic tell, for next time:
namecoming back as the file path with an empty job list means GitHub could not parse the file. Confirmed independently with actionlint:Fix: use
vars.PERF_K6_FULL_RUN, which is available at job level, and drop the workflow-levelenventry that nothing else read. The workflow now honourson:and no longer runs on push. Behaviour is unchanged otherwise — set the repo variablePERF_K6_FULL_RUN=trueto let the nightly schedule run the full compose + k6 job; unset stops the scheduled run aftervalidate-scripts.Worth noting:
validate-scriptshas therefore never executed. I checked its assertions by hand —apps/cms/k6/{smoke,load-deliver,load-items}.jsandseed.tsall exist — so it should pass on its first real run.2. Nothing lints the workflows — that is why this lasted weeks
This is the part I'd actually defend. An unparseable workflow is externally indistinguishable from a workflow that ran and failed, and because
perf-k6is not a required check, no gate objected and no one had reason to look. Same shape asB30in the last PR: a guard that isn't wired up is worse than no guard, because its silence reads as approval.New
workflow-lintjob inci.ymlrunsactionlintover.github/workflows. Pinned by version and SHA-256 of the release tarball rather than pulling a third-party action — one feweruses:SHA to keep current, and the binary is verified before it runs. shellcheck is left enabled, since therun:blocks here drive Postgres, Redis and deploy steps and a quoting bug there is the same class of silent breakage.Zero findings across all workflows as of this commit.
Per DoD §6 I am not adding a new DoD section for this: the guard is mechanical and self-enforcing, which §6 explicitly prefers over a checklist item humans have to remember.
3.
@types/react-domoverride raised19.2.4→19.2.5Requested as a prerequisite for the pending minor-and-patch group bump (now #435). That PR raises the manifests to
^19.2.5while the override pins exact19.2.4; the ranges stop intersecting, the override silently wins, anddrift:checkfails — correctly. This is theB20class the drift guard exists for.Raising the override here clears it without #435 having to touch either declaration site, and
19.2.5still satisfies the current^19.2.4declarations, so it is a no-op for anyone not on the group bump. Both declaration sites updated (package.jsonfor pnpm 9,pnpm-workspace.yamlfor pnpm 10+), sosettings:checkstays green.Verified by lockfile, not manifest: every importer now resolves
19.2.5(@types/react@19.2.18), and the entire 102-line lockfile diff is that one propagation — no other package moved.Verification
actionlint(1.7.10, with shellcheck) → 0 findings across.github/workflowspnpm check:allgreen —settings:check16 overrides,drift:check20 declarations vs 16 overridespnpm typecheck17/17 (relevant here: the React DOM types bump touches studio/docs/landing/consumer/ui)pnpm build9/9 ·pnpm test12/12 tasks, 0 failures (serialized — seeB32)pnpm audit --prod --audit-level high→ no known vulnerabilitiespnpm install --frozen-lockfileon a fresh checkout ofmain(09373a2c, including chore(deps): raise the Node floor jsdom 30 requires, retire the dead nanoid@5 override #436) → exit 0, no driftBacklog:
B31→fixed, with the root cause recorded rather than just the symptom.