Skip to content

ci: make each CI gate and contract cover what it claims - #4477

Merged
Astro-Han merged 8 commits into
apache:mainfrom
Astro-Han:refactor/4475-derive-ci-install-contracts
Sep 2, 2026
Merged

ci: make each CI gate and contract cover what it claims#4477
Astro-Han merged 8 commits into
apache:mainfrom
Astro-Han:refactor/4475-derive-ci-install-contracts

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

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 — requiresHeavyValidation is gone (a73fa5d)

A twelve-term disjunction over every other selection whose only production effect was skipping actions/setup-node on a documentation-only pull request. Given up: a docs-only run now pays one setup-node — seconds on a runner the job is already holding, while what #4461 argues is scarce is the slot. Confirmed first that nothing outside ci.yml read the heavy output.

Deleting it deletes the reason every selection that gates an installed step is one heavy validation covers existed, 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/ leaves plan.stateRootCompat sitting 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 all now also covers storageStress and full, 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 ci was conditioned on five terms; state_root_compat, runtime_sandbox, runtime_host, standard_workspaces, e2e, storybook and app_icons held only through an unwritten implication that each also selects code. 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 to DURABLE_STATE_DECODER_FILES now 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 only X ==, which is what had dropped standard_workspaces — spelled != '' and contains(...) — from the set entirely. A block-scalar if: 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\.mjs cannot match a /, so scripts/computer-use/lab-root.test.mjs was silently outside the set. Widened to [\w./-], and npm run script names expand one hop through package.json, which brings in windows-test-inventory.{mjs,test.mjs}, asf-npm-workflow-policy.test.mjs, check-app-shell-hooks.mjs and asf-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 an npm run entry point. Given up: nothing.

4 — one pull_request.paths reader (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.mjs loses its yaml import (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.
  • collectWindowsPackageSourceClosure and readWindowsReleasePathPatterns deleted; both were one-line forwards called only from the suite beside them. windowsPackageSourceEntrypoints now has that suite as its consumer.
  • UNDERIVED_PACKAGE_PATTERNS is a Set; the reasons live as comments beside each entry, where a stale one is visible.
  • The two inline "filtered pull_request trigger" regexes are now one call to the shared parser. hasPullRequestGate is deliberately not merged into them: it means "carries a plain pull_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)

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. 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.

One fix went with 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. That was the fail-open direction. It derives the same fourteen suites today.

Plus (fecc601): scripts/workflow-pull-request-paths.mjs joins RELEASE_CONTRACT_FILES, because check:release is 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.

  • The predecessor resolves on state-root-qualification, the only job that reads it. Registry flakiness used to forfeit build and every job downstream. It still costs no runner of its own — the resolve rides a job that was already going to run — and npm-publication and release-cli-stage 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. Reordering rather than accumulating failures is deliberate: set -e is disabled inside a function or subshell whose status is tested, so a || failed+=(...) form would let a failed curl fall through to the qualifier and return the exit status of echo.
  • 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 — what the matrix's fail-fast: false used to buy.

All four are contracts in release-cli-workflow-policy.test.mjs now. 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. actionlint is clean on both changed workflows, and every jobs.*.outputs.* / needs.*.outputs.* reference in cli-package-validation.yml resolves.

windows-package-source-closure.test.mjs: the two path-filter tests pass; the recovery filter is exactly the Windows-branching closure of its tests cannot run in this checkout — esbuild cannot resolve model-metadata.generated.js, which is not built locally. It fails identically on 6668af3f55 with 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.

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 1, 2026
@Astro-Han Astro-Han changed the title ci: derive the install contracts from the gates that decide them ci: make each CI gate and contract cover what it claims Sep 1, 2026
@github-actions github-actions Bot added effort/L Under 1000 readable lines and removed effort/M Under 500 readable lines labels Sep 2, 2026
`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.
@Astro-Han
Astro-Han force-pushed the refactor/4475-derive-ci-install-contracts branch from 11dc81a to fb998d5 Compare September 2, 2026 13:44

@MicroGery MicroGery left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Astro-Han
Astro-Han merged commit e2cb8a0 into apache:main Sep 2, 2026
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/L Under 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Strengthen the CI contracts added in #4461, and delete the authority one of them guards

2 participants