ci: allocate a runner only when its inputs changed - #4461
Conversation
ARE404
left a comment
ARE404
left a comment
There was a problem hiding this comment.
Holding approval at head da3b71d8d2b784d0ff0d1ffdd7db392092dedd04: the code review is clean, but the PR is a draft and CI is not yet green at this head (test queued, package/addon builds running). No P0/P1 found in the change.
ci: allocate a runner only when its inputs changed — 5-commit workflow/CI-planner refactor to cut runner occupancy. Verified sound:
- The merge gate cannot be silently skipped. The required job stays named
test(the.asf.yamlrequired context, deliberately not renamed), and the install/heavy step is gated on a single newheavyoutput =requiresHeavyValidation(plan), an OR over every test surface (asfSource || astryxSurface || cliPackage || code || e2e || releaseContract || runtimeHost || runtimeSandbox || storybook || standardWorkspaces).codeis set for any workspace/scripts/skills/typecheck file, and any unknown path forces a full plan — so any real code change yieldsheavy=trueand the full suite runs in the required lane. A PR can no longer be gated green while skipping its suite. changedFilesBetweenis now merge-base correct (merge-base+--diff-filter=ACMRDTincl. type changes), which removes the "PR checks may predate later main" gap the old planner leaned on.- Windows recovery PR-lane is narrowed with evidence: over ~300 runs the recovery suites' PR lane never red a commit that the Linux
testlane had not already red; its one unique catch (Windows NTFS alternate-stream) came from the unfiltered main-push trigger. Removing the source-dir path filters makes it run only on Windows-specific inputs (npm ci, patches, installer, Local IPC trust PS1) plus the unfiltered after-merge push. Deliberate, documented trade. - release-windows baseline steps move to
schedule— correct, since qualifying a transition out of a pinned historical release cannot be changed by a PR diff; keepsruntime-host-boot.tsfeed wiring asserted per-change viaupdate-test-feed-wiring.test.mjsin the required lane instead of a 25-min Windows lane. - cli-package-validation folds
release-predecessorintobuild— the predecessor lookup rides the runner already waiting on addon builds; outputs re-exposed frombuild.
Holding only because of the two gate facts (not code): draft PR + CI not yet green at the exact head. Bring it out of draft and get test/package green here, and I'll approve — the change reads correct and the runner-occupancy win is real.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Two P1 blockers remain on this exact head, so I did not approve it. The other workflow consolidation paths passed local build, full typecheck, lint/format/ASF checks, release-contract checks, 120 focused workflow/harness tests, and the same focused validation on a clean synthetic merge with current main.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
I reviewed this at da3b71d8d2b784d0ff0d1ffdd7db392092dedd04. No P0, P1, P2, or P3.
The required check is still named test, which is what .asf.yaml requires. Planning is the first step of that one job, and later steps read steps.plan.outputs.*. There is no leftover needs.plan.outputs — GitHub would treat that as an empty string and skip the step instead of failing. heavy is the OR of every installed-toolchain surface, so a documentation-only change still reports test green after the install-free steps, and a real code change still installs and runs.
The Windows recovery PR filter no longer names the crash and owner-death sources. That lane was never a required context, and the unfiltered main push still runs those suites. The packaged Windows PR path still packages, verifies, and runs autoupdate; the pinned-baseline upgrade and rollback now run on the schedule, where a pull request cannot change their historical input. The boot-to-feed wiring is asserted on every change in the required job. CLI validation still runs the same three State Root transitions and both Node smokes; they share runners instead of repeating downloads.
Hosted test and package were still pending on this head when I posted. This changes when runners are allocated, not what a required lane can observe. I am not merging it.
简体中文
我审的是 da3b71d8d2b784d0ff0d1ffdd7db392092dedd04。没有 P0/P1/P2/P3。
必选检查还叫 test，和 .asf.yaml 一致。规划是这个 job 的第一步，后面都读 steps.plan.outputs，没有残留的 needs.plan（GitHub 会把它当成空字符串，步骤会静默跳过）。heavy 是所有需要装工具链的面的或。文档-only 仍会报 test 绿；真正的代码改动仍会安装并跑。
Windows recovery 的 PR 过滤不再点名崩溃套件本身；它本来也不是必选检查，合入后的 main push 仍会跑。Windows 打包在 PR 上仍做打包、校验和自动更新；钉死的基线升级改到定时跑。CLI 的三次 State Root 和两个 Node smoke 都还在，只是共用 runner。
发这条时 hosted test / package 还在排队。我不合入。
Automated review notice: This comment was posted by an automated review agent operated by WAWQAQ. It is not an independent human review and does not replace one.
Splitting path planning from validation made every pull request queue for a scarce runner three times to reach one verdict. `plan` executed for 19 seconds and the `test` aggregation job for four, yet each allocation waited on its own: across 32 runs with complete timing the three queues averaged 5m57s, 4m46s and 7m57s. The distribution is bimodal, about a minute per stage while the pool is free and about twenty while it is starved, so the two extra allocations cost roughly 12m44s of pure waiting per run. Planning is now the first step of one job and every later step gates on `steps.plan.outputs`, which is what those steps already did through the job boundary. Nothing is validated that was not validated before, and a documentation-only change costs one short allocation rather than the two it previously paid for the planning and aggregation jobs. The job keeps the name `test` because that is the required context in `.asf.yaml`. Renaming it would leave that check unreported on every open pull request until the rename merged, and nothing could merge while it was unreported. The longest validation observed is 25m37s with every lane selected, so a single job stays well inside the 45-minute limit. Generated-by: Claude Code
The two app-icon drift tests regenerate the shipped PNGs through Python and take about 52 seconds, but they were selected by `code`, so every change anywhere in the tree paid for them. Replayed over the last 200 commits on main, `code` is true for 193 of them while the surface those tests actually read is true for 41. That surface is now its own planner selection, derived from what the tests open: the committed artwork, the generator that must still reproduce it, the `APP_ICONS` catalog they check it against, and the packaged-resource list that has to keep naming every file. Coverage is unchanged — every input that could make these tests fail still selects them. Each of those inputs lives in a workspace or under `scripts/`, so an app-icon selection still implies `code` and the build the tests need has already run. A test asserts that implication rather than leaving it to be rediscovered. Generated-by: Claude Code
This lane opened fourteen runners per pull request to produce evidence that four of them were not needed to hold. `release-predecessor` resolved one npm version in fifteen seconds and then released its runner. It now runs on `build`, which already waits about sixteen minutes on the addon builds, so the resolution costs nothing it was not already waiting through. The exported `release_predecessor_*` names are unchanged, so `npm-publication`, `release-cli-stage` and `asf-npm-candidate` consume exactly what they consumed before. `state-root-qualification` ran three matrix jobs. Two of them qualify a transition between tarballs that were published and frozen, so nothing in a pull request can change their outcome except the qualifier itself, and the third reads the candidate this run built. They are three invocations of one script against one sandbox, so they now share a runner. The scenario table survives as three calls with the same digests, epoch relations and identity flags; each writes its own report and all three are uploaded together. The Linux x64 smoke ran twice on two runners for two Node versions. Same machine, same tarball, same architecture assertion — it now installs the second version and repeats the smoke in place. Pull requests allocate ten runners instead of fourteen. Nothing that was verified before is unverified now. Generated-by: Claude Code
This lane packages, installs and updates Maka on Windows. It takes about 25 minutes on a runner class that is scarce on shared infrastructure, and it was being allocated for two reasons that its own steps cannot justify. `apps/desktop/src/main/runtime-host-boot.ts` was in the path filter because the packaged updater is driven through `MAKA_UPDATE_TEST_FEED`, which that file hands to the update service. The wiring is a handful of lines; the module is 1900 of them, and over the last 200 commits on main it was the sole reason this lane ran 13 times. It is now asserted by `scripts/update-test-feed-wiring.test.mjs`, which runs on every change in the required job before any toolchain is installed and costs milliseconds. The filter matches 41 of those 200 commits, down from 55. The pinned-baseline steps qualify a transition out of an already-released installer, so a pull request's diff is not their input and cannot change their outcome. Downloading that baseline is also this lane's most common failure: 14 of 36 failures across the last 300 runs, every one of them on a branch that could not have caused it, each costing another 25-minute Windows allocation. Those three steps move to a nightly schedule, which is still far ahead of release day, the moment they exist to precede. Packaging, release verification and the end-to-end autoupdate check still run on every matching pull request. Generated-by: Claude Code
This lane's pull-request filter named every source directory in the workspace closure of the crash and owner-death tests it runs, so it took a Windows runner on 118 of the last 200 merges. The filter was accurate — those suites really do import most of `runtime`, `runtime-host`, `storage` and `core`, and a generated import closure measured at 106 of the same 200, because they boot a real Runtime Host. The list was not the problem. What the run history shows is that the pull-request trigger was. Across 300 runs, 241 of them pull requests, this lane produced 12 reds and not one of them was unique: every pull-request failure sat on a commit whose `test` run had already failed, usually at the very same step — six at `Install dependencies`, one at the workspace tests that run the same owner-death suite on Linux. Its single unique catch in that window was a Windows-only NTFS alternate stream regression found by the unfiltered main push, which is the trigger that exists for exactly that. So the filter now names only what a Windows runner can prove and `test` cannot: how `npm ci` resolves and what the dependency patches and the Electron installer produce there, what the clean step removes there, and the Local IPC trust boundary, a PowerShell script with no other caller. That is 14 of 200 merges instead of 118. The install and build steps stay unconditional, because proving those suites still build and run on Windows is what the lane is for once it does run. The recovery authorities are now covered after merge rather than before it, by the unfiltered main push minutes later and by the nightly. `windows_recovery` is deliberately not a required context in `.asf.yaml`, so it was never what stood between a regression and `main` in any case. Generated-by: Claude Code
…equests Both steps were moved to the schedule on the grounds that they qualify a transition out of a pinned historical release, so a pull request could not change their outcome. That is only true of the baseline. The other input to `verify:windows-installer` is the installer this run just built, which it installs over the pinned one, and `verify:windows-installer-rollback` does not read the baseline at all: it takes the candidate installer and the version-bumped one, and it is the only place the `installer.nsh` Abort path is exercised. `installer.nsh` and both verifiers stayed in the path filter the whole time, so a change to them scheduled this lane and then skipped the steps that read them. Both run on pull requests again. The flake that motivated the move — 14 of 36 failures across 300 runs, always on branches that could not have caused it — is handled where it belongs: the baseline is pinned by version, tag, asset name and SHA-256 in a committed manifest, so it is immutable and cacheable on that manifest's hash. A hit skips the network and a corrupt entry still fails, because the download step verifies the checksum either way. The schedule stays. Its reason is now the one the other two Windows lanes give for theirs: the path list is a pre-filter, not this lane's import closure, so a transitive edit it cannot match would otherwise first be observed on release day. Generated-by: Claude Code
Dropping the workspace source directories dropped something with them. Part of these suites is portable TypeScript that Linux fails first, which is why removing them cost no observed pull-request signal — but part is guarded by `process.platform === 'win32'` and is skipped off Windows by construction, so `test` cannot go red on it however carefully it runs. `assertNoWindowsAlternateStreams` and its three NTFS alternate stream regressions are exactly that, and they are the one thing this lane has caught that no other lane could have. The Windows-branching modules under the executed suites are named again, individually: seventeen files, which the last 200 merges touch twice more than the filter already matched. That buys back the whole Windows-only half for the price of two runs, where readmitting the closure would cost 118 of 200. The rule is now checkable rather than remembered, so a stale entry cannot sit here: every workspace path on this filter must be one file, not a glob, and must contain a Windows branch. A module that stops forking on the platform leaves the list, and a portable file cannot enter it. Generated-by: Claude Code
da3b71d to
0d4301c
Compare
|
Rebased onto Both P1s
What the rebase caught#4427 added three Measured, not projected
The first breaks down as
Filter rates over the last 200 first-parent commits: @ARE404 both gate facts you held on are resolved: the pull request is out of draft, and |
ARE404
left a comment
ARE404
left a comment
There was a problem hiding this comment.
Approving at the current head 0d4301c6e05e48f86c2891535b81e714c590bb4d (PR came out of draft; supersedes the head in my prior hold note). No P0/P1 — the design is clean and, after two refinement commits, coverage-preserving.
The two refinements since the earlier head materially strengthen it:
0d4301c6restores the genuinely Windows-only recovery surface to the PR lane by name (theprocess.platform === 'win32'-guarded modules —assertNoWindowsAlternateStreams+ its NTFS regressions, which no other lane can catch), instead of dropping it wholesale with the portable-TS redundancy. The rule is now checkable rather than remembered: every workspace entry on that filter must be a single file (not a glob) that actually contains a Windows branch, with tests pinning it — a portable file can no longer slip in. Its sharpest note is empirical: #4400 matched the old wide closure, ran this lane green 3× on the PR, and the alternate-stream failure still surfaced only on the main push — i.e., the wide filter it replaces didn't even stop the one regression it's tempting to cite, because that was a stale-base interaction no path filter sees. So the narrowing loses no pre-merge signal that the wide filter actually provided.a6c3e1e8keeps the candidate installer's own upgrade and rollback on pull requests (only the pinned-historical-release baseline qualification stays on the schedule, which a PR cannot affect); theruntime-host-boot.tsfeed wiring stays asserted per-change in the requiredtestlane viaupdate-test-feed-wiring.test.mjs.
Core guarantees still hold from my prior review: the required job keeps its test name (the .asf.yaml required context) and its install/heavy step gates on requiresHeavyValidation(plan) (an OR over every surface, with code set for any real code file and unknown paths forcing a full run) — so no pull request can go green while skipping its suite; changedFilesBetween is merge-base correct; cli-package-validation folds the redundant predecessor runner.
CI is running at this head (rebased/advanced branch; the required test lane + package/addons in progress). Out of draft, MERGEABLE, review already APPROVED. Approving on code; the remaining item is exact-head CI green before merge.
…s them
`durableStateLocations` builds its paths with `join`, so on Windows the
State Root is `\qualification-scope\state-root`. The assertion compared it
to a literal `'/qualification-scope/state-root'` and failed there, taking
`npm run check:release` and the packaged Windows lane with it — the
account-local assertion beside it already used `join` and passed.
The nesting assertion below had the same blind spot without failing:
`golden.startsWith(`${live}/`)` cannot match a Windows path, so it would
have accepted a golden copy nested inside its live directory, which is the
one thing it exists to reject. Both now spell the separator the way the
platform does.
Generated-by: Claude Code
|
The nesting assertion two lines further down had the same blind spot without failing: This is out of scope for a runner-occupancy pull request, but it blocks this lane on every branch that reaches it, so it is here rather than in a follow-up. It is also a live example of the point @hqhq1025 made in the |
M4n5ter
left a comment
M4n5ter
left a comment
There was a problem hiding this comment.
English
Re-review at 54d81647: the latest path-separator fix is correct and the Windows package job is now green, but it does not affect the three findings below. I do not think this head is ready to merge.
[Blocking] The Windows recovery allowlist is not complete
windows-recovery.yml lists selected Windows-branching files, while its contract only proves that files already on the list contain win32. It cannot detect omissions.
There are omissions on the current head. The workflow directly executes tests whose source files contain Windows-only paths but are absent from the filter, including control-endpoint.test.ts, runtime-resume-crash.test.ts, runtime-continuation-crash.test.ts, and four Skill catalog tests. stable-storage.ts is another omitted Windows-branching dependency directly used by the listed root-authority.ts and marker-file.ts.
A change to these paths runs only the Linux required lane before merge; its Windows branch is skipped, and this Windows workflow is not scheduled. The previous Windows-coverage P1 is therefore only partially resolved. Please verify filter completeness from the test entrypoints the workflow actually executes and their Windows-branching dependency owners, and add the currently missing paths.
[Blocking] The app-icon surface omits a direct test input
APP_ICON_FILES omits apps/desktop/electron-builder.config.mjs, although the gated test reads that file directly to keep the macOS/Windows bundle icon aligned with DEFAULT_APP_ICON.
The planner currently returns appIcons=false for that config, and check:release does not run this test. Pointing the config at another existing icon can therefore pass build/package while silently skipping the drift assertion. Please add the config to APP_ICON_FILES and the planner regression inputs.
[Important] The restored Windows baseline cache is deleted before use
The workflow restores artifacts/windows-upgrade-baseline, but prepareWindowsUpgradeBaseline() immediately removes that directory and always invokes gh release download.
A targeted reproduction with a valid cached installer produced downloads=1 and cachedFileSurvived=false. Cache hits therefore do not avoid the network failure this change intends to remove. The smallest fix is to verify and return an existing exact installer, download only when it is absent, and reject a checksum mismatch. A focused cache-hit/cache-miss test would preserve that contract.
The overall consolidation direction is a net simplification; I found no separate simplify-audit recommendation. The earlier candidate-sensitive upgrade/rollback P1 is resolved.
Local verification: build, typecheck, lint, format, ASF headers, 82 focused workflow/contract tests, and Linux check:release all passed. The new path-separator test passes 8/8 at this exact head. Hosted Windows package, Windows recovery, CLI smokes, and State Root qualification are green; required test was still queued when this review was posted.
中文
在 54d81647 上重新审查：最新的路径分隔符修复是正确的，Windows package 也已经转绿，但它没有触及下面三项问题。当前 head 仍不适合合并。
[Blocking] Windows recovery allowlist 仍不完整
windows-recovery.yml 手工列出部分 Windows 分支文件，而对应 contract 只验证“已经列入的文件包含 win32”，无法发现遗漏。
当前 head 已经存在遗漏。Workflow 直接执行、包含 Windows-only 路径但未进入 filter 的测试包括 control-endpoint.test.ts、runtime-resume-crash.test.ts、runtime-continuation-crash.test.ts 和四个 Skill catalog tests。stable-storage.ts 也包含 Windows 分支，并被已列入的 root-authority.ts 和 marker-file.ts 直接调用，但它同样不在 filter 中。
修改这些路径时，合并前只会运行 Linux required lane；Windows 分支被跳过，而 Windows workflow 不会启动。之前的 Windows coverage P1 因此只修复了一部分。请从 workflow 实际执行的测试入口及其 Windows-branching dependency owners 验证 filter 完整性，并补齐当前遗漏。
[Blocking] App icon surface 漏掉了直接测试输入
APP_ICON_FILES 没有包含 apps/desktop/electron-builder.config.mjs，但被 gate 的测试会直接读取它，用于保持 macOS/Windows bundle icon 与 DEFAULT_APP_ICON 一致。
Planner 对该配置当前返回 appIcons=false，而 check:release 也不运行这项测试。因此，将配置指向另一个已经存在的 icon 时，build/package 可以成功，但 drift assertion 会被静默跳过。请将该配置加入 APP_ICON_FILES 和 planner regression inputs。
[Important] 恢复出来的 Windows baseline cache 在使用前就被删除
Workflow 恢复了 artifacts/windows-upgrade-baseline，但 prepareWindowsUpgradeBaseline() 随后立即删除该目录，并始终调用 gh release download。
定向复现中，预置有效缓存后仍得到 downloads=1、cachedFileSurvived=false。所以 cache hit 无法避开本次修改想消除的网络失败。最小修复是：已存在精确 installer 时校验并直接返回；不存在时才下载；checksum 不匹配则明确失败。建议增加一个聚焦 cache hit/cache miss 的测试。
整体 consolidation 方向确实是净简化，没有额外的 simplify-audit 建议。之前 candidate-sensitive upgrade/rollback 的 P1 已解决。
本地验证：build、typecheck、lint、format、ASF headers、82 项 workflow/contract tests 和 Linux check:release 均通过；新路径分隔符测试在当前 exact head 上 8/8 通过。Hosted Windows package、Windows recovery、CLI smokes 和 State Root qualification 已转绿；发出本 review 时 required test 仍在排队。
…g of it Three reviewers found three holes in this branch, and they are one hole. Every gate it narrowed took its new input set from my reading of what the gated work consumes, and every contract test I wrote to defend that set proved only that what I kept belonged — never that what I dropped was unnecessary. Containment in the easy direction. The other direction is an absence, and an absence has no line number to check. So each list was short by whatever my reading could not see. The recovery filter missed `stable-storage.ts`, reached transitively through the two lock authorities it did name, along with six test files. `APP_ICON_FILES` missed `electron-builder.config.mjs`, which the drift test opens by path rather than imports. And the baseline cache missed that `prepareWindowsUpgradeBaseline` deletes its output directory before downloading unconditionally, so a restored entry was never once consulted. Each is now computed from the authority and asserted as set equality: - The recovery filter is the import closure of the dist suites its steps execute, restricted to files that branch on `win32` — 56 files, checked with `deepEqual` so an omission and a stale entry both fail. That is 40 of the last 200 merges rather than 17, and unlike 17 it can be shown to be complete. `collectWindowsPackageSourceClosure` is now a caller of a general `collectWorkspaceSourceClosure`, which the packaged Windows lane already used for exactly this purpose. - The app icon surface is read off the step: whatever `App icon artwork drift` runs is scanned for the repository paths it opens, and each must select the gate. - `prepareWindowsUpgradeBaseline` reuses a copy that hashes to the pinned digest and downloads only otherwise. A cache entry is never an authority on what the run installs: a mismatch or an unreadable file goes to the network, and only a fresh download that fails the digest fails the run. Applying the same rule to the gate none of the three reviews reached found a fourth instance, in the required job. `heavy` decides whether the toolchain is installed, and nothing asserted that a selection gating a later step is one of its disjuncts — so `app_icons`, added by this branch, gated a step that imports `@maka/core/settings` without selecting the install it needs. It reached green only because every icon input happens to select `code` as well. That disjunct is now present, and the workflow is scanned for the rest. Generated-by: Claude Code
|
Thank you — this was the most useful review on the pull request. All three are fixed in Every gate this branch narrowed took its input set from my reading of the gated work, and every contract test I wrote proved containment in the easy direction — that what I kept belonged — never that what I dropped was unnecessary. An omission is an absence, and an absence has no line number to check. So all three lists were short by exactly what my reading could not see: a transitive dependency, a path opened rather than imported, and a consumer that discards its own input. Each is now computed from the authority and asserted as set equality. Recovery filter. No longer curated. App icon surface. Baseline cache. Applying the same rule where none of the three reviews reached found a fourth instance, in the required job. |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
I found one P1 and one P2 on exact head b915970c6b86b57052a8ba9b5b38888cb2052137, so the conditional approval threshold is not met.
This head replaces the hand-maintained Windows recovery allowlist with a generated Windows-branching import closure, adds authority-derived app-icon and heavy-selection contracts, and makes the pinned Windows baseline cache reusable after checksum verification. The previous current-head findings about the incomplete recovery surface, omitted app-icon config, and discarded cache entry are addressed in the implementation.
The new planner contract cannot run in the clean pre-install CI phase because it statically imports installed packages, and the new cache regression file is not invoked by any hosted command; see the inline findings.
Validation after dependency installation: npm run build:test, production build, full workspace typecheck, lint, format, ASF headers, release contracts 155/155, and the focused planner/Windows/cache suites 116/116 passed. git diff --check passed. In a fresh exact-head worktree before npm ci, the required CI command node --test --test-concurrency=1 scripts/ci-test-plan.test.mjs scripts/verify-windows-harness.test.mjs failed immediately with ERR_MODULE_NOT_FOUND for esbuild. At publication, hosted windows_recovery was green while required test was still queued and package was running.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
`ci-test-plan.test.mjs` was the only suite running before `npm ci`, so every assertion that needed a bare checkout accreted there regardless of subject: 66 tests, of which 35 read `.github/workflows/*.yml` and had nothing to do with the test planner. The file name carried no hint that its one hard rule is "import nothing that is not installed yet", which is how a closure assertion needing esbuild was very nearly added to it. That import would have thrown `ERR_MODULE_NOT_FOUND` in the sole required context and frozen every merge in the repository. Split by what a test reads. `ci-test-plan.test.mjs` keeps the 30 tests that exercise the planner against an in-memory graph; the 36 that read a workflow move to `ci-workflow-policy.test.mjs`, beside the two policy suites that already follow that name. Naming the constraint is not enforcing it, so derive it: the new suite reads which files the steps above `setup-node` run and walks their transitive local imports, failing on any specifier that is neither a `node:` builtin nor a repository module holding the same rule. Moving a step below the install lifts the constraint and adding one above imposes it, with no list to maintain. The closure assertion itself now lives in `windows-package-source-closure.test.mjs`, which `check:release` runs after installing, and `windows-recovery.yml` joined the release-contract inputs so editing that filter selects the gate that checks it. Two app-icon tests iterated a hand-written `APP_ICON_INPUTS`; the derived test computes that same set from the suites the step runs, so they folded into it and the list is gone. `prepare-windows-upgrade-baseline.test.mjs` joined `check:release` without joining the planner's release set — the guardrail that exists for exactly that caught it. Generated-by: Claude Code
Two filters in this branch were proved only in the easy direction, which is the direction that cannot see an omission. `release-windows-check.yml` asserted that its import closure is covered by the filter. Nothing asserted the converse, so a `packages/` entry backed by nothing stayed in the list forever and booked a 25-minute Windows runner every time it matched, with no test able to report it. Five of its twenty five entries turn out to be underived — peer dependency manifests, Runtime Host candidate election, the script that builds the worker the closure starts from. Each is legitimate, and each is now declared with its reason, so the set is exact in both directions and a stale exception fails as loudly as a missing entry. Three hand-written tests asserted that a lane pairs its path filter with a nightly run, one lane each. The rule they encode is not lane-specific: a filter is a pre-filter, not an import closure, so something has to run the lane without consulting it. Replaced by one test enumerating the workflow directory and requiring a schedule, an unfiltered push, or a `workflow_call` caller. The three pairings covered three lanes; the rule covers eight and would have caught `release-windows-check.yml`, whose schedule this branch added without an assertion to hold it there. That enumeration also surfaces three lanes that have no escape at all: `gitoxide-helper-admission` and `runtime-host-peer-admission` pair the pull request with a `push: main` carrying the same filter, and `runtime-host-owner-platform` pairs it with `workflow_dispatch`, which nothing fires on its own. They predate the gates narrowed here, so they are declared rather than changed — the point is that the gap is now countable instead of invisible. Generated-by: Claude Code
Four leftovers from the consolidations earlier in this branch, each one a guarantee the old shape supplied and the new shape does not. `release-windows-check.yml` keyed its concurrency group on `github.ref`, which is `refs/heads/main` for the nightly and for a dispatch alike, so a dispatch could queue behind the nightly and be cancelled while still pending. Keyed on the pull request number like the other two Windows lanes. The three State Root transitions used to be three matrix jobs, so one failing left the others to upload their reports. Folded into one step they share a `set -e`, and the reports are wanted most on the run that failed, so the upload now runs with `if: always()`. `tee` has already written the failing transition's own output by then, and `if-no-files-found` stays `error` so a broken path is still caught on a green run. Moving predecessor resolution onto `build` rewrote the assertions that bound it, and `PREDECESSOR_TARBALL_URL` lost its only one — the third transition would still be spelled correctly while pointing at nothing. Both env bindings and the `workflow_call` output are asserted again. `.asf.yaml` still described the required context as an aggregation job propagating failures from a plan lane and a heavy job, none of which exist since the three merged into one. It now describes what is there, and names splitting the work back across jobs as the third way to freeze the queue. Also bounds `update-test-feed-wiring.test.mjs` to the argument object it means: an unbounded span would have accepted a `testFeedUrl` from any later call in a 1900-line module. Generated-by: Claude Code
Nine assertions restated `packages/` entries of `release-windows-check.yml` as literals. Every one could only ever catch "this exact line was deleted", and the previous commit made that case fail already: the filter's `packages/` half is computed from the import closure and compared as a set, so removing an entry fails there whether or not anyone remembered to also name it here. Verified by deleting `connect-or-spawn.ts` from the filter and watching the closure test fail with these gone. Two of the nine carried a reason for being in the filter without being in the closure; that reason now lives on the declared exception beside the path it explains, which is where someone auditing the entry will look. `copy-runtime-filesystem-worker.mjs` stays asserted by hand. It is the one entry outside the derived half: the desktop app copies the built worker in rather than importing it, so no closure reaches it and only naming it keeps it on the lane. Kept as well are the assertions that encode a rule rather than a copy — that the gate triggers on `release.yml` and on itself, and that Windows recovery still runs the three regression suites it exists for. Those state something no derivation produces. Generated-by: Claude Code
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Re-review of exact head 83a56953350c16728bd23bc2a6cc648c08703acc: no P0-P3 findings.
The previous blockers are fixed. The dependency-free pre-install planner/workflow suites now run successfully in a clean checkout with no node_modules; the dependency-backed Windows source-closure contract runs after installation through check:release; and the reusable Windows baseline regression is both included in that gate and self-selects the release-contract lane.
I reviewed the complete workflow/planner delta and the Windows recovery, package, CLI validation, State Root, app-icon, and release-contract paths. On the exact head, npm ci, build:test, the production build, full workspace typecheck, check:release (161 tests), focused workflow/contract tests (93/93), lint, format, ASF headers, and git diff --check passed. A clean synthetic merge with current main dfe457eac9fcef811174754111650923ebcbd3f9 also passed npm ci, build:test, full typecheck, the 93 focused tests, the protocol epoch guard, and diff checks.
Hosted platform and CLI validation checks currently reported success; required test, package, and audit were still pending when this review was submitted, so their terminal results remain part of the merge gate. I did not independently execute the Windows-only package lifecycle locally.
Review notice: This review was prepared by an automated review agent operated by hqhq1025 and is published at the direction of AstroHan, who has read these findings and is the human accountable for them.
5 participants
Summary
A runner slot is scarcer than a runner minute on ASF's shared infrastructure, and this repository was spending slots on work that could not change a verdict. Seven commits, one theme: every allocation must be justified by its own inputs.
testjob. ci: skip heavy validation for documentation-only pull requests #4172 split one verdict acrossplan→heavy→test, to keep documentation-only pull requests off heavy CI (ci: skip the expensive CI job for documentation-only pull requests #3998). That goal is right and this keeps it; the three-job shape is what costs, because a pull request then queues for a runner 2.875 times to answer one question. Planning is now the first step of a single unconditional job and every later step gates onsteps.plan.outputs.*. The job keeps the nametestbecause that is the required context in.asf.yaml— renaming it would leave the check unreported on every open pull request until the rename merged, and nothing could merge while it was unreported.codewas true for 193 of the last 200 commits, while the surface those tests read changed in 41.release-predecessordid one metadata fetch on its own runner; the threestate-root-qualificationmatrix entries each re-downloaded the same two tarballs; two Linux x64 smoke entries differed only by Node version. 14 → 10 runner slots per pull request, same assertions.runtime-host-boot.tswas in the 25-minute Windows lane's path filter for a handful of lines of update-feed wiring, and was the sole reason that lane ran 13 of 200 commits; those lines now have an install-free contract test. 55 → 41 of 200.test, keep their Windows-only half here.windows-recoveryfiltered on every source directory in the closure of the crash and owner-death suites, taking a Windows runner on 118 of 200 merges, and across 300 runs never produced a pull-request red thattesthad not already produced on the same commit. What it does own is the half guarded byprocess.platform === 'win32', which Linux skips by construction; those seventeen files are named individually. 118 → 17 of 200.Nothing that can observe a defect was removed. The changes are to when a runner is allocated and how many answer the same question.
Refs #4451. The issue projected
cli-package-validationat 14 → 9; the arithmetic is 14 → 10. Itswindows-recoveryproposal — generate the filter from the suites' import closure — was built and measured at 106 of 200 (the closure is 709 files, because these tests boot a real Runtime Host), so the trigger, not the list, is what this changes instead.Measured effect
ci.yml, against two three-job runs from the same congested window today:refactor/goals-controller-scopefeat/desktop-api-key-host-onboardingThe first of those breaks down as
planqueued 12m to execute 18s,heavyqueued 19m to execute 18m32s, andtestqueued 25m to execute 2s. Execution is not comparable across different diffs, but #4400 ran the pre-#4172 single job at 24m27s, so one job at 23m is the normal shape rather than an inflation; merging also drops a duplicate checkout and set-up.cli-package-validationon this branch opened 10 jobs against 14 on a control branch (run 33498682856), as designed.Filter rates over the last 200 first-parent commits:
windows-recovery118 → 17,release-windows-check55 → 41, app icon regeneration 193 → 41.Review focus
Two things would be silent if wrong.
ci.yml: a leftoverneeds.plan.outputs.*resolves to the empty string rather than failing, which disables a step instead of erroring — the rebase onto #4427 produced exactly that, in three newstate_root_compatgates.scripts/ci-test-plan.test.mjsasserts the workflow contains no such reference, thatjobsis exactly['test'], that the job has noneeds:and no job-levelif:, and that- id: planprecedes the first use of its outputs.windows-recovery: re-adding a workspace source directory would quietly put the lane back on most merges, and a hand-listed Windows surface would rot. Two contract tests: the executed workspaces are derived from the dist paths the steps run and closed over the dependency graph, and none of that closure may appear as a directory glob; and every workspace path on the filter must name one file and contain a Windows branch.Verification
node --test --test-concurrency=1 scripts/ci-test-plan.test.mjs scripts/update-test-feed-wiring.test.mjs scripts/release-cli-workflow-policy.test.mjs scripts/asf-source-workflow-policy.test.mjs scripts/windows-package-source-closure.test.mjs— 81/81 pass; each new assertion was confirmed to fail against the pre-change workflow.npm run format:check,npm run lint,npm run check:asf-headers— clean.state-root-qualificationshell was checked withbash -n; every changed workflow was parsed as YAML to confirm the job and step counts above.0d3265eaf4; thestate_root_compatgates fix(runtime-host): separate the persisted grant record from the authority it derives #4427 added were carried into the merged job.ci.yml's merged shape and the Windows lanes are observable only on this pull request's own checks —testwas green at 23m27s on the previous head.pull_request.pathsagainst the last 200 first-parent commits. Failure attribution comes from the last 300 runs ofrelease-windows-checkand ofwindows-recovery, cross-referenced against theCIrun on the same head SHA.AI use
Select exactly one:
Tool(s) and scope: Claude Code — audited the workflows, measured the trigger rates and failure history, and authored these changes and their tests. Commits carry
Generated-by: Claude Code. The maintainer reviewed the diff and owns the merge decision.Checklist
Does this PR entail a change in behavior?