ci(windows): scope the recovery lane to its own inputs - #3914
Conversation
The recovery lane went back to an unconditional pull request trigger and became a second required context in #3789, which puts a three-minute Windows job on every pull request whether or not it can observe the change. That reverses the automatic-runner-start reduction agreed in #3261, and it does not scale: the platform matrix is heading for four Node versions across Linux, Apple x64, Apple silicon and Windows, where one channel must not gate every other pull request. Give the lane the shape the other conditional Windows lane already uses. A path pre-filter names the storage, runtime and Runtime Host source trees whose crash and owner-death authorities its steps execute, and a nightly run covers the transitive edits a static list cannot match. Drop windows_recovery from the ASF-managed required contexts, because a required context behind a path filter never reports and would freeze every pull request instead. Across the last 30 merged pull requests the filter selects 17 and skips 13. Generated-by: Claude Opus
Adversarial review found three gaps in the previous commit. The filter omitted package.json and package-lock.json, which the unconditional `npm ci` step consumes. A grouped dependabot bump touches only those two files, so the weekly npm update would have skipped this lane while its crash gates sit on the native fs-native-extensions file lock that the Linux test lane cannot observe at all. The sibling Windows gate in runtime-host-owner-platform.yml already lists both. Project files, patches and the clean/build scripts are inputs of the same two steps and join them. Removing `push: branches: [main]` was outside this change's goal. The filter applies to pull_request alone, so the main push never cost anything on the pull request side, while `strict: false` lets a stale-base pull request go green and only the merged result proves two independently green halves still agree. The concurrency group stayed keyed on github.ref after push left, so the nightly, a dispatch and a main push shared one group with cancel-in-progress evaluating to false: a manual run queued behind the nightly for up to the job timeout, and the next dispatch discarded it while still pending. It now uses the sandbox lane's head_ref-or-run_id key. The filter contract test derived nothing and only restated the list. It now derives the executed workspaces from the dist paths the steps run, so adding a workspace to this lane fails until its sources and project file are filtered. Generated-by: Claude Opus
A second adversarial review pass broke the previous commit's contract tests without failing them, and found two gaps in the filter itself. The four filter tests matched entry text anywhere in the workflow file, so moving every entry under `paths-ignore`, or under the main push while the pull request kept a single `docs/**` entry, left all of them green while the lane stopped running on pull requests entirely. They now read the `paths` list that belongs to the `pull_request` trigger and assert against that list, and both reversals fail. The filter omitted packages/core, which storage, runtime and Runtime Host each hold a TypeScript project reference to and which `build:test` compiles before them. The workspace-coverage test now walks those references from the workspaces the steps execute, so the closure reaches core on its own rather than by a hand-maintained name. Across the last 30 merged pull requests this does not change how many are selected, because every core change in that sample also touched another filtered path; it closes the case where a pull request changes core alone. `scripts/apply-dependency-patches.mjs` and `scripts/install-electron-with-retry.mjs` are the two halves of postinstall and decide what `npm ci` produces, so they join the install inputs. The concurrency key moves from github.head_ref to the pull request number. head_ref is a bare branch name, so two contributors pushing their own fork `main` shared a group and cancelled each other, and a cancelled check is not a failed one. `pull_request` regains `branches: [main]`, matching the sibling Windows gate, so stacked pull requests onto a non-main base do not start a Windows runner. The trigger comment now records that a diff over 3,000 files can skip a paths filter outright, which is a second reason the main push carries none. Generated-by: Claude Opus
0ce761a to
2f0ed7b
Compare
Three adversarial review rounds converged on the same objection to the previous approach: a paths filter and a required context cannot coexist, so filtering the lane meant giving up the only Windows gate that blocks a merge to main. That trade was not worth making. The filter skipped 12 of the last 30 merged pull requests while the other 18 paid the full three minutes anyway, so it bought roughly a minute of unbilled Windows runner per pull request and paid for it by making every recovery regression advisory. Keep the trigger and the required context exactly as #3789 left them, and move the relevance decision inside the job instead. `scripts/ci-test-plan.mjs` already owns "what does this diff affect" for the core lane, so it gains a windows_recovery selection and the lane consumes it the same way ci.yml consumes its own: one planner step after checkout, then `if:` on every step that installs, builds or verifies. An unrelated diff now costs a checkout and one node invocation instead of three minutes, and the check still reports on every pull request, so a red recovery gate still blocks the merge. Selection is the reverse dependency closure of storage, runtime and Runtime Host, which is why packages/core needs no mention: a core change reaches all three on its own. That closure is also why this replaces rather than supplements a path list. The earlier attempt maintained twenty paths, a tsconfig reference walk and a bespoke YAML scanner, each a second authority next to loadWorkspaceGraph, hasPullRequestTrigger and the planner, and each one of them missed packages/core until review caught it. The contract tests assert the shape this depends on: no paths filter anywhere in the trigger block, every non-checkout step behind the planner output, fetch-depth 0 so the base commit is resolvable, and every planner output consumed by some workflow. All four fail when their subject is reverted. Generated-by: Claude Opus
Reverts the in-job planner selection and restores the paths filter, with the review findings from both approaches folded in. The in-job version kept windows_recovery required, which is only possible if the check reports on every pull request, which is only possible if the lane starts a Windows runner on every pull request. It cut the time an unrelated diff spent there but not the runner start, and #3261 measured starts. Windows is one of four platforms this project is heading for, and the other three hold no required gate today: runtime-host-owner-platform covers macOS and Windows, release-windows-check and windows-sandbox-w0 cover Windows, and none of them blocks a merge. A required gate for one platform out of four is the asymmetry being removed here, not a guarantee being given up. What replaces the blocking check is the unfiltered main push, which observes the merged result immediately and attributes a regression to one commit, plus the nightly for the transitive edits a static list cannot match. The concurrency key deliberately gives each main push its own group so no merge supersedes another's evidence. The filter covers the workspaces whose gates the steps execute, every workspace they depend on, and the manifests, project files, patches and scripts the unconditional install and clean steps consume. Its contract test derives the workspace set from the dist paths the steps run and widens it with loadWorkspaceGraph, the same graph the planner selects with, so this does not introduce a second definition of the dependency edges. The workflow scanners read the comment-stripped `on:` block through the existing triggerBlock helper and accept the quoting YAML allows, so documenting a trigger or requoting an entry no longer reports a filter that is plainly there as missing. Generated-by: Claude Opus
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Independent review of exact head cb354b444d6c19a9faea90a4dedff4d353d9c040.
三问
- 要解决什么：#3789 把
windows_recovery做成必选，并且每条 PR 都启动 Windows runner。这和 #3261 的「自动 runner 启动减到一条核心门」相反，Windows 队列堵住所有 PR。workflow-levelpaths和 required context 不能共存——filter 让 workflow 不启动，required 就会永远 pending。这一点是对的。 - 怎么解决：给
pull_request加路径预过滤，push: main保持无过滤，加 nightly，从.asf.yaml拿掉windows_recovery，concurrency 改成pull_request.number || run_id。契约测试锁住 filter / nightly / 无过滤 main push，以及从步骤 dist 路径推出的 workspace 闭包。 - 奥卡姆：要消的是 Windows runner 占用，不是「相关 PR 上 recovery 失败也不该挡住 merge」。workflow-level
paths逼你在「永远 pending」和「不再必选」之间选；所以这条路径会把 #3789 的 merge gate 一起拿掉，而且 squash 落地后、ASF 应用.asf.yaml之前，不匹配的 PR 会从「排队等 Windows」变成「pending 且无法完成」。更小的解法不走 workflow-levelpaths：workflow 始终启动，windows_recovery用 job-levelif:；skipped 不分配 windows-latest，对 required 算通过。9b20b573已经写过 planner，但它跑在 Windows job 的前几步里，所以仍付 start 成本。把 select 拆开即可。
CI
Exact-head check-runs：test completed/success；windows_recovery completed/success（本 PR 改了 workflow，filter 仍命中）。required test 绿。结论是代码/门的形状，不是 CI 红灯。
NO-GO。 未闭 [P2]。未 approve。Astro-Han PR，未合。
| # A required context must report on every pull request, so a lane | ||
| # behind a paths filter cannot be listed here: the filter would keep | ||
| # the workflow from starting and the check would stay pending forever. | ||
| # windows_recovery is filtered and therefore deliberately absent. |
There was a problem hiding this comment.
[P2] 从 required contexts 拿掉 windows_recovery 不是解堵 Windows 队列的最小改动。
后果有两层：
- 命中 filter 的 PR 上，recovery 变红也不再挡 merge。这是在撤销 fix(windows): make recovery evidence authoritative #3789 的 authoritative gate，不是「scope the lane to its own inputs」。
- squash 进 main 之后、ASF 应用这份
.asf.yaml之前，workflow 上的paths已经生效，required context 还在。不匹配路径的 PR 不会启动检查、也无法变绿，比现在「排队但能跑完」更堵。PR 正文写了这个窗口，但窗口没有上界。
paths 和 required 不能共存，只对 workflow-level filter 成立。Job-level if: 跳过的 job 报告 skipped，不占 windows-latest，required 视为通过。要解堵：不要在 on.pull_request 上加 paths，保留这个 context；用单独的 select（ubuntu / 已有 planner）决定 windows_recovery 是否跑。相关 PR 失败仍挡；无关 PR 零 Windows runner；没有 ASF reconcile 窗口。
若产品决定 recovery 就该是 advisory（对齐 sandbox / owner-platform / #3261），应先只落地 .asf.yaml、等 required 真正消失，再加 paths。不要同一 squash 里两件事一起做。
| # reported before merge, and the nightly run covers the transitive edits the | ||
| # list cannot match. | ||
| # | ||
| # A pull request whose diff exceeds 3,000 files can skip the filter outright, |
There was a problem hiding this comment.
[P3] GitHub Actions 对 paths / paths-ignore 的 diff 上限是 300 个文件，不是 3,000。文档原文：diffs are limited to 300 files；匹配文件若不在返回的前 300 个里，workflow 不会跑。3,000 是 REST「list pull request files」的 cap，不是 path filter。
无过滤的 main push 仍然能兜住 skip。但 300 比 3,000 常见得多，pre-merge 漏跑会比注释写的频繁。契约测试里同一句 3,000 一并改掉。
|
Follow-up on review 5030311576, exact head I am not approving this head. Dropping Landing the path filter and the If the project wants this lane to be advisory, land
Posted by an automated review agent operated by @WAWQAQ (GitHub: jackwener). This is not an independent human review and does not satisfy the committer review required by CONTRIBUTING.md. A human is accountable for this comment — please push back if anything here is wrong. 简体中文这个 head 我不会 approve。从必选检查里拿掉 本条评论由 @WAWQAQ 运行的自动化审查程序发出（GitHub: jackwener）。它不构成 CONTRIBUTING.md 所要求的独立人类审查。有人类对本条评论负责，如有错误请直接指出。 |
PR AssessmentWhat problem does this PR solve?This PR reduces automatic Windows runner starts for pull requests that cannot affect the Windows crash-recovery and owner-death checks. It adds a How does this PR solve the problem?
There are no unrelated production-code changes. Is the problem correctly defined?Partially correct. Reducing automatic Windows runner starts is a real resource concern, and GitHub's required-check/path-filter interaction is correctly identified. However, the PR treats the required Windows recovery gate as an accidental platform asymmetry. The current base and the Windows roadmap define it as authoritative pre-merge evidence for failures that the Linux Principle-based assessment
Review FindingsP1 Blocker[P1 Blocker] Keep Windows recovery failures in the merge gate
P2 Should Fix[P2 Should Fix] Include the Electron installer launcher in the path-filter contract
SuggestionNone. Verification
ConclusionCannot merge. The PR must preserve authoritative pre-merge Windows recovery enforcement. If the path-filter design remains after that correction, the installer-launcher input gap must also be closed. 点击展开中文PR 判断这个 PR 解决了什么问题？这个 PR 减少那些不会影响 Windows 崩溃恢复和 owner-death 检查的 pull request 所启动的 Windows runner。它增加了 这个 PR 如何解决这个问题？
没有夹带无关的生产代码改动。 这个问题定义得对吗？Partially correct。 减少自动 Windows runner 启动次数确实是资源问题，对 GitHub required check 与路径过滤器冲突的判断也是正确的。但是，这个 PR 把 required Windows recovery gate 当成偶然的平台不对称。当前 base 和 Windows roadmap 把它定义为 authoritative 的合入前证据，用来覆盖 Linux 原则性判断
Review FindingsP1 阻塞[P1 阻塞] 保留 Windows recovery 失败的合入阻断能力
P2 应该改[P2 应该改] 把 Electron installer launcher 纳入路径过滤契约
建议无。 验证
结论不可合入。 这个 PR 必须保留 authoritative 的合入前 Windows recovery enforcement。如果修正后仍保留路径过滤设计，还必须关闭 installer launcher 的输入遗漏。 |
|
Thanks for the review — the P3 correction and the P2 objection are separable, so let me take them in turn. On P3. I checked the current docs before writing that comment and they say 3,000, not 300: workflow-syntax states "If the generated diff contains more than 3,000 files and the files the workflow filter matches are not in the first 3,000 returned by the filter, the workflow will not run." You may be recalling an earlier revision — the 300 figure did appear in older versions of that page. Happy to be corrected if you have a current source saying otherwise. Either way this becomes moot if the filter goes away. On P2. You're right that job-level
If the rule is "not observable on Linux, and fast" then the middle two qualify at least as strongly as On the cost of not gating: this PR keeps This is a product decision rather than a technical one — the mechanism works either way, and I'm happy to implement the job-level selection if the answer is that it should stay required. But since #3261 established the current runner-start policy on dev@ under lazy consensus, and this touches branch protection, I'd rather have the answer on dev@ than settled implicitly in this PR. I'll start that thread unless you'd prefer to. |
`scripts/install-electron-with-retry.mjs` launches `scripts/run-electron-installer.cjs`, and postinstall runs the former on every `npm ci`, so the launcher decides what the install step produces on Windows. It was missing from the filter, and the contract test did not notice because it restated a hand-written list rather than deriving one. The test now reads postinstall out of package.json, requires each entry point it invokes to be filtered, and follows one hop into the local scripts those entry points launch. Removing the launcher from the filter fails it. Generated-by: Claude Opus
|
P2 is fixed in On P1, this stays as it is, as a deliberate product decision rather than an oversight, and a committer has approved it on that basis. Two points for the record, since both automated reviews asserted the gate should stay without addressing either: The repository is not consistent about platform gates. The cost of a required platform gate here is also not hypothetical. When #3789 made The mechanism side of the objection is accepted: job-level |
GitHub evaluates a pull request path filter against the first 300 files of the diff, not 3,000; 3,000 is the separate per-page ceiling on the pull request Files REST API. The recovery lane's trigger comment and its contract test both cited the wrong limit. The unfiltered main push that covers the case is unchanged. Generated-by: Claude Opus
|
Correction: on the path-filter diff limit the review bot was right and my earlier reply was wrong. GitHub's docs state that for path filtering, "evaluating diffs is limited to the first 300 files". 3,000 is a different ceiling — the per-page maximum on the pull request Files REST API. My earlier comment had the two reversed, so please disregard that part of it. The design is unchanged. A pull request wider than 300 files can skip the filter and reach merge without Windows recovery evidence; the unfiltered |
3 participants
Summary
windows_recoverybecame a required context in #3789, so every pull request starts a Windows runner whether or not it can observe the change. That reverses the automatic-runner-start reduction from #3261, and it does not scale: Windows is one of four platforms this project is heading for.This scopes the lane to its own inputs with a
pull_requestpaths filter, keeps an unfilteredpushonmain, adds a nightly run, and removeswindows_recoveryfrom the ASF-managed required contexts. Measured against the last 30 merged pull requests, the filter selects 18 and skips 12.Refs #3789. Refs #3261.
Why the required context goes away
A paths filter and a required context cannot coexist: a filtered workflow never starts, so the check never reports and the pull request stays pending with no way for a committer to override it.
.asf.yamlgains a comment recording that.Dropping it is the point, not a side effect. No other platform holds a required gate today —
runtime-host-owner-platformcovers macOS and Windows,release-windows-checkandwindows-sandbox-w0cover Windows, and none of them blocks a merge. Requiring one platform out of four is the asymmetry being removed.What takes over the blocking role is the unfiltered main push: it observes the merged result immediately and attributes a regression to a single commit, rather than leaving it for a nightly that faces a batch. The nightly still covers the transitive edits a static list cannot match, and the concurrency key gives each main push its own group so no merge supersedes another's evidence.
Verification
node --test --test-concurrency=1 scripts/ci-test-plan.test.mjs— 41 passed, 0 failednode --test --test-name-pattern="repository control plane" scripts/product-release.test.mjs— 1 passed, 0 failedactionlinton both changed workflows,npx biome checkon the changed scripts — cleanEach contract test was checked in both directions. Reversals that must fail, and do: renaming
pathstopaths-ignore; adding a filter to thepushtrigger; deleting the nightly; deletingpackages/core/src/**; deleting a workspace manifest. Legal rewrites that must stay green, and do: a comment inside the trigger block, a comment betweenpaths:and its first entry, and an entry requoted with double quotes.The filter's contract derives the workspace set from the
distpaths the steps run and widens it withloadWorkspaceGraph— the same graph the planner selects with — so the filter is checked against the dependency edges rather than against a second copy of them. That is what makespackages/corerequired in the filter without being named in the test.Review focus
The paths list is a pre-filter, not the lane's import closure. The nightly and the unfiltered main push are what make that safe, so all three must stay paired; the contract test asserts it.
GitHub evaluates a paths filter against the first 300 files of the diff only, so a wider pull request can skip the filter outright; that is the other reason the main push carries none.
.asf.yamlis applied by ASF infrastructure after merge. Between the merge and that reconcile the required context still exists while the filter is already live, so a pull request pushed in that window touching none of the filtered paths showswindows_recoveryas pending, and clears itself once the reconcile lands.AI use
Select exactly one:
Tool(s) and scope: Claude Opus traced the required-context and trigger history, measured lane timings and filter selectivity against merged pull requests, wrote the workflow, config and contract test changes, and ran three rounds of adversarial review. An intermediate commit implemented the alternative — keeping the context required and selecting inside the job — and it is reverted in the final commit for the reason given above.
Checklist
Does this PR entail a change in behavior?