ci: make each CI gate and contract cover what it claims - #4477
Open
Astro-Han wants to merge 8 commits into
Open
Conversation
`requiresHeavyValidation` was a twelve-term disjunction over every other selection, and its only production effect was skipping `actions/setup-node` on a documentation-only pull request. That saves seconds on a runner the job is already holding, which is not what is scarce: the slot is. In exchange every new selection had to remember to join the disjunction, and a test was added to `ci-workflow-policy.test.mjs` for the sole purpose of noticing when one did not. Deleting the output deletes the reason that guard test existed, so it goes too. The six planner assertions phrased over it are replaced by reading the selections off the plan: `documentation-only changes select nothing at all` now covers `storageStress` and `full` as well, which the disjunction never did, and a selection added later joins it without anyone editing the test. Capability given up: a documentation-only run pays one `setup-node`.
With `heavy` gone, `npm ci` is the only thing that decides whether a step has anything to run against, and its condition is a five-term disjunction that omits `state_root_compat`, `runtime_sandbox`, `runtime_host`, `standard_workspaces`, `e2e`, `storybook` and `app_icons`. Those hold today only through an implication nothing writes down — each of them also selects `code` — so moving one decoder path under `.github/`, which the planner's `code` loop skips, would gate a step on a selection that runs against a checkout with no `node_modules`, and every assertion phrased over the two lists would stay green. The assertion is now the implication itself, checked over the repository paths the planner's own source names, so a path added to one of its sets is exercised by the edit that adds it. The gate scrape also reads any selection named inside an `if:` rather than only `X == `, which is what dropped `standard_workspaces` — spelled `!= ''` and `contains(...)` — from the set.
…d match The install-free import contract scraped `scripts/[\w.-]+\.test\.mjs`. That class cannot match a `/`, so `scripts/computer-use/lab-root.test.mjs` — run by a step above the install — was dropped, and the derivation yielded ten suites where the section names eleven. Four more entry points are reached through `npm run` rather than named as files: `windows-test-inventory.mjs`, `asf-npm-workflow-policy.test.mjs`, `check-app-shell-hooks.mjs` and `asf-license-headers.mjs`. Widening the class and expanding `npm run` one hop through `package.json` brings the set to eighteen, including the two planner-adjacent scripts the job itself runs. All of them are clean today, so this closes a latent gap rather than a live failure.
`windows-package-source-closure.mjs` parsed with the `yaml` package and `ci-workflow-policy.test.mjs` hand-scanned lines, because it runs before `npm ci` installs a parser. Compared across all eight workflows carrying a filter the two produce identical output, so one of them is redundant — and the install-free constraint decides which one has to stay. `scripts/workflow-pull-request-paths.mjs` is that reader, `node:` only. `readWindowsReleasePathPatterns` and `collectWindowsPackageSourceClosure` went with it: both were one-line forwards to the general version, called only from the suite next to them. Capability given up: tolerance for flow sequences (`paths: [a, b]`) and other legal YAML spellings of the same list. Several assertions over these filters already assume one entry per line.
`UNDERIVED_PACKAGE_PATTERNS` was a `Map` whose values nothing read — the assertion takes `.keys()`. The reasons those five entries earn a Windows runner are worth keeping, so they stay as comments beside each entry, where a stale one is visible; the collection is a `Set`.
Two limits that reading the code does not reveal, recorded rather than
fixed. `collectWorkspaceSourceClosure` walks static imports, so a process
boundary is invisible to it — `root-authority.test.ts` forks its race
fixture and `worker-entry.ts` is bundled, and neither joins the closure.
Both are `win32`-free today, so the `windows-recovery` filter is complete;
"generated, not curated" overstated why. And `includes('win32')` is the
criterion a machine can check, not the set of Windows-only paths:
`git-worktree-child-executor.ts` carries one with no such literal.
One fix alongside them: the executed-suite scrape accepted only forward
slashes while the equivalent scrape in `ci-workflow-policy.test.mjs` handles
`[/\\]`, and these steps run under pwsh where both are legal. A suite
spelled with backslashes was dropped from the set that decides the filter,
which is the fail-open direction. It derives the same fourteen suites today.
`check:release` is the only thing that runs `windows-package-source-closure.test.mjs`, which is now the only reader of `release-windows-check.yml`'s filter, and it reaches it through `workflow-pull-request-paths.mjs`. Without this the module could change and the suite that depends on it would not run.
Three places in `cli-package-validation.yml` where collapsing a matrix into one job or one step made a first failure hide everything behind it. None of them fails silently, so each costs a rerun rather than protection — but the rerun is of the most expensive job in the workflow. The predecessor now resolves on `state-root-qualification`, the only job that reads it. Registry flakiness used to forfeit `build` and everything downstream; it still costs no runner of its own, and callers are unaffected because a reusable workflow publishes its outputs only once every job has finished. `current-nightly-predecessor-to-candidate` runs before the two frozen transitions. It is the only one a pull request can influence, and all three share one `set -e`, so a flaky `curl` on either frozen tarball meant it never executed. `Preserve the qualification reports` drops to `if-no-files-found: warn` on an already-failed job, where a `curl` that failed before any `tee` added a second, unrelated red; a green run still treats an empty directory as the broken path it is. The two supported Node versions no longer depend on each other: the first smoke is `continue-on-error` and its outcome is re-raised after the second has run, which is what the matrix's `fail-fast: false` used to buy. All four are contracts in `release-cli-workflow-policy.test.mjs` now, each verified by constructing the regression it forbids.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4475.
#4461 narrowed CI, and several of the gates and contracts it left behind claim more coverage than they deliver — with nothing to report the difference, because the test passes and the run goes green either way. A gate that decides nothing but has a test guarding it; assertions hung on the wrong authority, or on a scrape too narrow to see what they name; one fact with two authorities; folded execution where the first failure hides the rest.
Each item points the assertion at the authority that actually decides, or deletes the thing whose subject no longer exists. One commit per item, in the issue's order.
What each item gives up
1 —
requiresHeavyValidationis gone (a73fa5d)A twelve-term disjunction over every other selection whose only production effect was skipping
actions/setup-nodeon a documentation-only pull request. Given up: a docs-only run now pays onesetup-node— seconds on a runner the job is already holding, while what #4461 argues is scarce is the slot. Confirmed first that nothing outsideci.ymlread theheavyoutput.Deleting it deletes the reason
every selection that gates an installed step is one heavy validation coversexisted, so that guard test goes with it. The mutation that test caught — a gate out of sync with the disjunction — is no longer a defect, because there is no disjunction to be out of sync with; and it never caught the one that matters, since moving a decoder path under.github/leavesplan.stateRootCompatsitting in the disjunction untouched.The six planner assertions phrased over it are replaced by reading the selections off the plan object.
documentation-only changes select nothing at allnow also coversstorageStressandfull, which the disjunction never named, and a selection added later joins it without anyone editing the test.2 — the install gate derives from
npm ci(df78ab0)npm ciwas conditioned on five terms;state_root_compat,runtime_sandbox,runtime_host,standard_workspaces,e2e,storybookandapp_iconsheld only through an unwritten implication that each also selectscode. The assertion is now that implication itself, checked over the repository paths the planner's own source names, so a path added to one of its sets is exercised by the edit that adds it.Verified by mutation: adding a
.github/path toDURABLE_STATE_DECODER_FILESnow fails with.github/workflows/state-root-decoder.yml selects state_root_compat, which gates a step run against no node_modules. The old test stayed green on it.The gate scrape reads any selection named inside an
if:rather than onlyX ==, which is what had droppedstandard_workspaces— spelled!= ''andcontains(...)— from the set entirely. A block-scalarif:would hide its own gates, so that now fails too. Given up: nothing.3 — the install-free import contract covers eighteen entry points, not ten (
d4fd6ed)scripts/[\w.-]+\.test\.mjscannot match a/, soscripts/computer-use/lab-root.test.mjswas silently outside the set. Widened to[\w./-], andnpm runscript names expand one hop throughpackage.json, which brings inwindows-test-inventory.{mjs,test.mjs},asf-npm-workflow-policy.test.mjs,check-app-shell-hooks.mjsandasf-license-headers.mjs, plus the two planner scripts the job itself runs. All clean today, so this closes a latent gap. Verified by mutation in both a subdirectory suite and annpm runentry point. Given up: nothing.4 — one
pull_request.pathsreader (4a6ec56)Confirmed the two parsers produce identical output on all eight filtered workflows, then kept the one the install-free constraint requires:
scripts/workflow-pull-request-paths.mjs,node:only.windows-package-source-closure.mjsloses itsyamlimport (the package stays a devDependency — five other scripts and two packages use it). Given up: tolerance for flow sequences (paths: [a, b]) and other legal YAML spellings, which several assertions over these filters already assume absent.5 — the smaller items (
4a6ec56,1a354ea)plan.appIcons ||— moot, item 1 removed the function.collectWindowsPackageSourceClosureandreadWindowsReleasePathPatternsdeleted; both were one-line forwards called only from the suite beside them.windowsPackageSourceEntrypointsnow has that suite as its consumer.UNDERIVED_PACKAGE_PATTERNSis aSet; the reasons live as comments beside each entry, where a stale one is visible.pull_requesttrigger" regexes are now one call to the shared parser.hasPullRequestGateis deliberately not merged into them: it means "carries a plainpull_request:trigger and is a real validation lane", which is a different predicate from "carries a paths filter" — the allowlist and timeout-tier tests need the former.7 — two limits, recorded (
014ff81)collectWorkspaceSourceClosurewalks static imports, so a process boundary is invisible to it —root-authority.test.tsforks its race fixture andworker-entry.tsis bundled. Both arewin32-free today, so thewindows-recoveryfilter is complete; "generated, not curated" overstated why. Andincludes('win32')is the criterion a machine can check, not the set of Windows-only paths.One fix went with them: the executed-suite scrape accepted only forward slashes while the equivalent scrape in
ci-workflow-policy.test.mjshandles[/\\], and these steps run under pwsh where both are legal. That was the fail-open direction. It derives the same fourteen suites today.Plus (
fecc601):scripts/workflow-pull-request-paths.mjsjoinsRELEASE_CONTRACT_FILES, becausecheck:releaseis the only gate that runs the suite depending on it.6 — the folded matrices give back independent execution (
11dc81a)Three places where collapsing a matrix into one job or one step made a first failure hide everything behind it. None fails silently, so each cost a rerun rather than protection — but the rerun is of the most expensive job in the workflow.
state-root-qualification, the only job that reads it. Registry flakiness used to forfeitbuildand every job downstream. It still costs no runner of its own — the resolve rides a job that was already going to run — andnpm-publicationandrelease-cli-stageare unaffected, because a reusable workflow publishes its outputs only once every job has finished.current-nightly-predecessor-to-candidateruns before the two frozen transitions. It is the only one a pull request can influence and all three share oneset -e, so a flakycurlon either frozen tarball meant it never executed. Reordering rather than accumulating failures is deliberate:set -eis disabled inside a function or subshell whose status is tested, so a|| failed+=(...)form would let a failedcurlfall through to the qualifier and return the exit status ofecho.Preserve the qualification reportsdrops toif-no-files-found: warnon an already-failed job, where acurlthat failed before anyteeadded a second, unrelated red. A green run still treats an empty directory as the broken path it is.continue-on-errorand its outcome is re-raised after the second has run — what the matrix'sfail-fast: falseused to buy.All four are contracts in
release-cli-workflow-policy.test.mjsnow. Given up: nothing.Verification
Ran locally, all green:
ci-test-plan.test.mjs,ci-workflow-policy.test.mjs,verify-windows-harness.test.mjs,script-entrypoints.test.mjs,asf-npm-workflow-policy.test.mjs,release-cli-workflow-policy.test.mjs,npm run check:asf-headers,npm run windows:inventory.actionlintis clean on both changed workflows, and everyjobs.*.outputs.*/needs.*.outputs.*reference incli-package-validation.ymlresolves.windows-package-source-closure.test.mjs: the two path-filter tests pass;the recovery filter is exactly the Windows-branching closure of its testscannot run in this checkout — esbuild cannot resolvemodel-metadata.generated.js, which is not built locally. It fails identically on6668af3f55with no changes applied.Every deleted assertion was checked by first constructing the mutation it claimed to catch and confirming the replacement goes red on it; every new contract was checked the same way.