You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.mjs — hasPullRequestGate 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.
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
testcontext 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 itscripts/ci-test-plan.mjs:493-508computes a twelve-term disjunction whose only remaining production effect is one line offormatGitHubOutputsoutput, consumed at.github/workflows/ci.yml:134to skipactions/setup-nodeon 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:
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;scripts/ci-test-plan.test.mjs.Capability given up: a docs-only run pays one
setup-node. Check first that nothing outsideci.ymlreads theheavyoutput.Do this one first — it is the state that a new guard test had to be written to protect.
2.
npm ciis the gate that decides whether a step can run, and nothing derives itci-workflow-policy.test.mjsproves that every selection gating an installed step appears inrequiresHeavyValidation. But.github/workflows/ci.yml:169conditionsnpm cion a different, five-term disjunction —code || astryx_surface || asf_source || cli_package || release_contract— which omitsstate_root_compat,runtime_sandbox,runtime_host,standard_workspaces,e2e,storybookandapp_icons. Those hold today only through an undeclared implication that each of them selectscode.The implication is real but unwritten, so it can be broken silently: adding a
.github/path toDURABLE_STATE_DECODER_FILESbreaks it, because the planner'scodeloop skips.github/. A step would then be gated on a selection that runs against a checkout with nonode_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 != ''andcontains(...).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, inevery suite that runs before dependency setup imports only node builtins, scrapesscripts/[\w.-]+\.test\.mjs.[\w.-]does not match/, soscripts/computer-use/lab-root.test.mjs— run by the step atci.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 runare 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 expandnpm runscript names one hop throughpackage.json.4. Two
pull_request.pathsparsers agree on all eight filtered workflowsscripts/windows-package-source-closure.mjsparses with theyamlpackage;ci-workflow-policy.test.mjshand-scans lines because it must run beforenpm 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'syamldependency. 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.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.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.