Repository navigation
Description
Follow-up to #4461, from an adversarial review of its final head. None of these blocks that PR — the review found no P0 and no P1, and could not make the required test context go red, stall, or stop reporting. What it did find is that some of the contracts #4461 added prove a weaker premise than they claim, and that one authority it went out of its way to protect may not need to exist.
Grouped because they share one cause: an assertion derived from an authority is only as strong as the authority it picked, and picking the wrong one is invisible — the test passes either way.
1. Delete requiresHeavyValidation, and the guard test that exists to watch it
scripts/ci-test-plan.mjs:493-508 computes a twelve-term disjunction whose only remaining production effect is one line of formatGitHubOutputs output, consumed at .github/workflows/ci.yml:134 to skip actions/setup-node on a documentation-only pull request. That saves a few seconds on a runner the job is already holding, which is orthogonal to what #4461 argues is scarce — the slot, not the minute.
Deleting it also removes:
- the twelve-term disjunction that must be updated whenever a selection is added;
every selection that gates an installed step is one heavy validation coversinscripts/ci-workflow-policy.test.mjs, whose sole purpose is to keep that disjunction in sync — a test ci: allocate a runner only when its inputs changed #4461 added;- six assertions in
scripts/ci-test-plan.test.mjs.
Capability given up: a docs-only run pays one setup-node. Check first that nothing outside ci.yml reads the heavy output.
Do this one first — it is the state that a new guard test had to be written to protect.
2. npm ci is the gate that decides whether a step can run, and nothing derives it
ci-workflow-policy.test.mjs proves that every selection gating an installed step appears in requiresHeavyValidation. But .github/workflows/ci.yml:169 conditions npm ci on a different, five-term disjunction — code || astryx_surface || asf_source || cli_package || release_contract — which omits state_root_compat, runtime_sandbox, runtime_host, standard_workspaces, e2e, storybook and app_icons. Those hold today only through an undeclared implication that each of them selects code.
The implication is real but unwritten, so it can be broken silently: adding a .github/ path to DURABLE_STATE_DECODER_FILES breaks it, because the planner's code loop skips .github/. A step would then be gated on a selection that runs against a checkout with no node_modules.
The same test's scrape, steps\.plan\.outputs\.(\w+) ==, also recognises only ==, so it does not see the two other forms already in the file: standard_workspaces != '' and contains(...).
If item 1 lands, this becomes the only install gate, which is the right place to assert.
3. The install-free import contract silently excludes a suite it claims to cover
ci-workflow-policy.test.mjs, in every suite that runs before dependency setup imports only node builtins, scrapes scripts/[\w.-]+\.test\.mjs. [\w.-] does not match /, so scripts/computer-use/lab-root.test.mjs — run by the step at ci.yml:102 — is dropped. The derivation yields ten suites where the install-free section names eleven.
Four more install-free entry points reached through npm run are outside the set as well: windows-test-inventory.{mjs,test.mjs}, asf-npm-workflow-policy.test.mjs, check-app-shell-hooks.mjs, asf-license-headers.mjs. All five are clean today, so this is a latent gap rather than a live defect, and its blast radius is smaller than the test's own comment claims — a devDependency import added to one of them turns the introducing pull request red, not every pull request.
Fix: widen to scripts/[\w./-]+\.test\.mjs, and expand npm run script names one hop through package.json.
4. Two pull_request.paths parsers agree on all eight filtered workflows
scripts/windows-package-source-closure.mjs parses with the yaml package; ci-workflow-policy.test.mjs hand-scans lines because it must run before npm ci. Compared across all eight workflows carrying a filter, the two produce identical output.
The install-free constraint means the hand-written one has to stay, so extract it into a node:-only shared module and drop the other along with that module's yaml dependency. Capability given up: tolerance for flow-sequence and other legal YAML spellings — which several existing assertions already assume absent.
5. Smaller items
plan.appIcons ||inrequiresHeavyValidationis dead:app_icons ⇒ codeis asserted inci-workflow-policy.test.mjs, andplan.codealready covers it. Moot if item 1 lands.collectWindowsPackageSourceClosureandreadWindowsReleasePathPatternsare one-line forwards to the general versions, called only by tests.windowsPackageSourceEntrypointsis exported with no external consumer.UNDERIVED_PACKAGE_PATTERNSis aMapwhose values are never read — only.keys(). The reasons belong in comments beside each entry; aSetdoes the rest.- "Filtered
pull_requesttrigger" is spelled three times inci-workflow-policy.test.mjs—hasPullRequestGateand two inline regexes. One of the three additionally requirespaths:to immediately followpull_request:. Confirm which is intended before merging them.
6. Folding matrices into single steps traded away independent execution
Three places, all the same shape and all failing loudly rather than silently, so they cost a rerun rather than protection:
cli-package-validation.yml:340-403runs three State Root transitions in oneset -estep, withcurrent-nightly-predecessor-to-candidate— the only one a pull request can influence — last. A flakycurlon either frozen tarball means it never executes. Either move the candidate transition first, or record failures and exit once at the end. Related:Preserve the qualification reportspairsif: always()withif-no-files-found: error, so acurlfailure before anyteeproduces a second red on an already-red job.cli-package-validation.yml:298-309runs Node 22.19 and 24 in one job; a 22.19 failure means 24 never runs. The old matrix usedfail-fast: false.cli-package-validation.yml:206-212resolves the npm predecessor insidebuild, so registry flakiness now fails the most expensive job in the workflow instead of a job that took seconds.
7. Two limits worth documenting rather than fixing
collectWorkspaceSourceClosurewalks static imports, so it cannot see process boundaries.root-authority.test.tsforksfixtures/root-initialization-race.js, andfilesystem-worker/worker-entry.tsis bundled rather than imported; neither is in the closure. Both are free ofwin32today, so thewindows-recoveryfilter is complete — but "generated, not curated" overstates what the derivation guarantees.- The Windows-branching criterion is
includes('win32').packages/storage/src/git-worktree-child-executor.ts:399carries a genuine Windows-only workaround with no such literal. Separately, the entry-point regex inwindows-package-source-closure.test.mjsaccepts only forward slashes while the equivalent scrape inci-workflow-policy.test.mjshandles[/\\]; a suite spelled with backslashes would be dropped silently, which is the fail-open direction.
Activity
Astro-Han commentedon Sep 1, 2026
take