Skip to content

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

Description

@Astro-Han

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 covers in scripts/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 || in requiresHeavyValidation is dead: app_icons ⇒ code is asserted in ci-workflow-policy.test.mjs, and plan.code already covers it. Moot if item 1 lands.
  • collectWindowsPackageSourceClosure and readWindowsReleasePathPatterns are one-line forwards to the general versions, called only by tests. windowsPackageSourceEntrypoints is exported with no external consumer.
  • UNDERIVED_PACKAGE_PATTERNS is a Map whose values are never read — only .keys(). The reasons belong in comments beside each entry; a Set does the rest.
  • "Filtered pull_request trigger" is spelled three times in ci-workflow-policy.test.mjshasPullRequestGate and two inline regexes. One of the three additionally requires paths: to immediately follow pull_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-403 runs three State Root transitions in one set -e step, with current-nightly-predecessor-to-candidate — the only one a pull request can influence — last. A flaky curl on 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 reports pairs if: always() with if-no-files-found: error, so a curl failure before any tee produces a second red on an already-red job.
  • cli-package-validation.yml:298-309 runs Node 22.19 and 24 in one job; a 22.19 failure means 24 never runs. The old matrix used fail-fast: false.
  • cli-package-validation.yml:206-212 resolves the npm predecessor inside build, 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

  • collectWorkspaceSourceClosure walks static imports, so it cannot see process boundaries. root-authority.test.ts forks fixtures/root-initialization-race.js, and filesystem-worker/worker-entry.ts is bundled rather than imported; neither is in the closure. Both are free of win32 today, so the windows-recovery filter 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:399 carries a genuine Windows-only workaround with no such literal. Separately, the entry-point regex in windows-package-source-closure.test.mjs accepts only forward slashes while the equivalent scrape in ci-workflow-policy.test.mjs handles [/\\]; a suite spelled with backslashes would be dropped silently, which is the fail-open direction.

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions