ci: repair three Windows lane contracts - #3317
Merged
Merged
Conversation
The lane's `paths` filter had grown to `packages/runtime/**` plus `packages/core/**`, which put a windows-2025 job — capped at 25 minutes — on the pull request for 21 of the last 60 first-parent commits. Exactly 1 of those 60 touched anything this evidence can observe. It had no concurrency group either, so every push to those pull requests started another one. The filter grew that way because it was asked to be a correctness claim, and it cannot be one: the lane's real input is the import closure of packages/runtime/src/sandbox and packages/runtime/src/filesystem-worker, which reaches 9 files at the top of packages/runtime/src and 5 @maka/core modules, inside source roots holding more than 130 top-level files each. No hand-written list is that closure, and one that tried would drift silently. So stop claiming it is one. The filter now names the directories that own the sandbox — matching 1 of those same 60 commits, so a change there still blocks before merge — and a nightly run covers the transitive edits the filter cannot match, after they land. Three packaging inputs came out of the list at the same time: no step here reads them, and release-windows-check already owns the packaged sandbox. The lane stays separate from windows-baseline. Baseline sets continue-on-error at the job level and on each diagnostic step, so a failure there does not fail the workflow; it carries a known Phase 1 backlog. Folding these assertions in would trade a regression signal for one fewer file. Give it the concurrency group it never had, keyed so that pull request pushes supersede each other while scheduled and manual runs each get a unique group and cannot discard one another, and cache npm now that it also installs on a schedule. The trigger allowlist that replaces the old contract reads each workflow's `on:` block rather than naming lanes that must stay off pull requests. The previous version named three and omitted this one, which is how the lane kept an unbounded trigger through a CI reduction that touched every workflow around it; matching the block also means `on: [pull_request]` cannot slip past it. The second contract pins both halves of the design, because a filter without the nightly run silently loses the transitive edits, and a nightly run without the filter puts the whole runtime back on pull requests. Generated-by: Claude Code
`npm run windows:inventory` regenerates the Windows test skip inventory and diffs it. It carried `continue-on-error: true`, so a drifted inventory reported a green run and merged — a gate that cannot fail the job is not a gate. The check is clean today (62 declarations), so removing it blocks nothing that passes now. Leave it unconditional and move it beside the planner test. It needs only the checkout: it runs on Node's built-ins, in about a third of a second, without `npm ci`. That is the same shape as `Test CI planner`, which already runs before dependency setup and can fail, so the two now sit together and are covered by one contract. The alternative was to gate it on the `code` surface like the Astryx checks next to it, but those are gated because they are expensive, and copying the shape without the reason costs more than it saves: `code` does not cover this check's own output under docs/, so the planner would need a special case for a generated file, and a pull request that only regenerates the inventory would then pay for install, lint, build, typecheck and knip to run a sub-second diff. Generated-by: Claude Code
Ten workflows check out the repository and eight of them disable persist-credentials. windows-baseline and windows-recovery did not, so both left the job token reachable through the checkout's git config for the rest of the run — and both then execute npm ci with lifecycle scripts, the desktop smoke, and the full storage and runtime suites. Neither pushes, calls gh, or reads a token anywhere, so nothing needed it. Assert the property across the whole directory rather than on the two files being fixed, and slice each checkout step so the assertion is per checkout: a bare one cannot be balanced out by a sibling step that opts out, or by the string appearing in a comment. Generated-by: Claude Code
Astro-Han
marked this pull request as ready for review
August 20, 2026 11:32
tisonkun
approved these changes
Aug 20, 2026
7 tasks
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
2 participants
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.
Summary
Three CI contracts that were stated somewhere but enforced nowhere, all on the Windows lanes.
W0 sandbox lane. Its
pathshad grown topackages/runtime/**pluspackages/core/**, putting a windows-2025 job on the pull request for 21 of the last 60 first-parent commits — 1 of which touched anything the lane observes. It had no concurrency group either, so every push started another one. That filter cannot be a correctness claim: the lane's real input is an import closure reaching 9 files at the top ofpackages/runtime/srcand 5@maka/coremodules, inside source roots of more than 130 top-level files each. So it now names the directories that own the sandbox, still blocking before merge on a change there, and a nightly run covers the transitive edits it cannot match. Three packaging inputs no step here reads also came out of the list;release-windows-checkowns those.Windows inventory check. A regenerate-and-diff gate carrying
continue-on-error: true, so a drifted inventory reported green and merged. It now runs unconditionally besideTest CI plannerand can fail the job — it needs only the checkout, running on Node built-ins in about 0.3s withoutnpm ci.Credential hardening.
windows-baselineandwindows-recoverywere the two of ten checkouts still persisting the job token, and both then runnpm cilifecycle scripts, the desktop smoke, and the full storage and runtime suites. Neither pushes, callsgh, or reads a token.Each contract is now enforced across the whole workflow directory rather than on the files being fixed. The trigger allowlist reads each workflow's
on:block instead of naming the lanes that must stay off pull requests — the previous version named three and omitted W0, which is how that lane kept an unbounded trigger through a CI reduction that touched every workflow around it.Verification
node --test --test-concurrency=1 scripts/ci-test-plan.test.mjs— 21 passingnpm run windows:inventory— exit 0, current at 62 declarations, so making it blocking fails nothing that passes todayactionlint .github/workflows/*.ymlandnpx biome check scripts/— cleanon: [pull_request], a deletedschedule, a deletedpaths, a restoredcontinue-on-error, a removedpersist-credentials: falseReview focus
The W0 filter is deliberately not the lane's dependency closure. A change to a transitive dependency such as
packages/runtime/src/path-containment.tsorpackages/core/src/windows-path.tsis caught by the nightly run after it merges, not before. That is the trade: pre-merge coverage for the sources that own the sandbox, post-merge coverage for what the filter cannot express. If pre-merge coverage of the full closure is required, this is the wrong shape and the planner should own the decision instead.AI use
Tool(s) and scope: Claude Code authored the workflow and test changes and the commit messages; commits carry
Generated-by: Claude Code. The design was revised twice under adversarial review by Claude (Fable) and Codex, which independently verified the statistics and ownership claims above — the inventory fix in particular reached its current form because that review showed the first attempt fixed it at the wrong layer. The human contributor reviews the final diff and owns the merge decision.Checklist
Does this PR entail a change in behavior?