ci: give the full-suite fallback's biggest buckets their real owners - #4482
Conversation
ca93ed3 to
f447318
Compare
M4n5ter
left a comment
There was a problem hiding this comment.
English
Review verdict: not ready to approve. I found three open correctness issues; the first two let the required test context pass while skipping an existing authoritative regression.
-
.asf.yamlis routed away from the test that actually validates it.planTests()currently selects no heavy surface, but the unconditional workflow-policy suite does not parse.asf.yaml. The real assertions are inproduct-release.test.mjs, undercheck:release, which is now skipped. A change to required checks or release-environment admission can therefore pass the required context without its policy test. Route.asf.yamltoreleaseContract, or move its authoritative assertions into a genuinely unconditional parser test. -
Dependency patches lose their behavior regressions. The new
patches/branch setscode=truebut selects no workspace, Runtime Host, Storybook, or E2E tests. The current patch changesnode-ptyruntime behavior, while its dedicated regression ispackages/runtime/src/__tests__/node-pty-write-lifecycle.test.ts; the selected CLI smoke only checks that_ownsFileDescriptorexists. A patch can still build/package while regressing the behavior it was added to fix. Keeppatches/on the fail-safe full path until there is an explicit, evidence-backed patch-to-consumer mapping. -
The broad
native/exemption disables the unknown-path fail-safe for future native owners.path.startsWith('native/')treats every future native crate as already owned, although the workflows cover onlynative/gitoxide-helper/andnative/runtime-host-peer/. A newnative/new-helperwould select no main-CI surface and no dedicated lane. Enumerate the two owned roots and add a regression proving an unknown native root still selects full.
Non-blocking simplification: the new “can reach Gitoxide” test skips every current entry because every path already contains the word gitoxide; it proves no actual reachability and creates a misleading second authority. Replace it with the direct stale-entry regression this PR needs, or derive reachability from real inputs.
Validation: the 113 install-free planner/workflow/Windows-harness tests pass, and git diff --check is clean. Those tests do not cover the skipped consumer-policy paths above.
中文
审查结论:目前不能 approve。有 3 个仍开放的 correctness 问题;前两个会让 required test context 在跳过现有权威回归测试的情况下通过。
-
.asf.yaml被路由到了真正验证它的测试之外。planTests()当前不选择任何 heavy surface,但无条件运行的 workflow-policy suite 并不解析.asf.yaml。真正的断言位于product-release.test.mjs,由check:release执行,而现在该步骤会被跳过。因此,required checks 或 release environment admission 的错误改动可能在没有运行其 policy test 的情况下通过 required context。应把.asf.yaml路由到releaseContract,或把权威断言移到真正无条件运行、会解析该文件的测试。 -
Dependency patch 不再运行其行为回归测试。 新增的
patches/分支 只设置code=true,却不选择任何 workspace、Runtime Host、Storybook 或 E2E 测试。当前 patch 修改的是node-pty运行时行为,专门回归测试是packages/runtime/src/__tests__/node-pty-write-lifecycle.test.ts;被选择的 CLI smoke 只检查_ownsFileDescriptor字符串存在。于是 patch 即使破坏了它本来要修复的行为,仍可能成功 build/package。除非建立显式且有证据的 patch→consumer 映射,否则应让patches/保持 fail-safe full。 -
过宽的
native/豁免关闭了未来 native owner 的 unknown-path fail-safe。path.startsWith('native/')默认未来所有 native crate 都已有 owner,但当前 workflow 只覆盖native/gitoxide-helper/与native/runtime-host-peer/。新增native/new-helper时,主 CI 与专属 lane 都不会被选择。应只枚举这两个已拥有的根目录,并增加 unknown native root 仍选择 full 的回归测试。
非阻塞简化建议:新增的 “can reach Gitoxide” 测试 会因为当前每个路径都包含 gitoxide 而全部提前跳过;它没有证明真实可达性,却建立了一套误导性的第二 authority。建议改为本 PR 真正需要的 stale-entry 直接回归,或从真实输入推导可达性。
验证:113 个 install-free planner/workflow/Windows-harness 测试全部通过,git diff --check 通过;这些测试没有覆盖上面被跳过的 consumer-policy 路径。
Asking for `ubuntu-latest` and asking for `ubuntu-24.04` gets the same machine. Both labels reported `Image: ubuntu-24.04`, `Version: 20260823.283.1`, provisioner `20260819.586`, runner `2.336.0` in their `Set up job` logs on the same day. So this changes which queue a job waits in, and nothing else. The queues are not the same. Across 1183 runner-backed jobs in eleven ASF repositories, `ubuntu-latest` has a median wait of 0.05 minutes and a p90 of 19.07; `ubuntu-24.04` has a median of 0.03 and a p90 of 0.53, while carrying 3.6x the jobs (455 against 126). The alias is not slower on average — it is unpredictable, and a required context is paid at the tail. `apache/flink` sees a p90 of 105.8 minutes on the alias. The sharpest control comes from this repository. One push to #4482 created nine first-layer jobs within the same second, none of them declaring `needs`. The three on `ubuntu-latest` waited 7.5, 7.68 and 7.7 minutes. The six on `ubuntu-24.04`, `ubuntu-24.04-arm`, `windows-2025`, `macos-15`, `windows-latest` and `macos-latest` waited between 3 and 24 seconds. It is not the `-latest` alias as such, since two of the fast six are aliases. It is not our own `concurrency` groups, since every workflow's median `run_started_at - created_at` is 0.0. It is not a self-hosted split, since `runner_group_name` reads `GitHub Actions` on all nine. `ci.yml` also has a reason of its own. Its bubblewrap step disables `apparmor_restrict_unprivileged_userns` specifically because Ubuntu 24.04 gates user namespaces that way. The required context already assumes this image; the alias only left that assumption free to drift without a commit. `windows-latest` and `macos-latest` stay as they are. Across 84 same-instant groups containing both `windows-latest` and `windows-2025`, the median paired difference in wait is 0.00 minutes, so pinning them would buy nothing measurable. Nine substitutions across eight workflows; thirteen jobs in the release and packaging lanes were already pinned. Two costs, both accepted deliberately. The automatic image upgrade becomes a manual commit — `ci-workflow-policy.test.mjs` holds the rule and says how to take an exemption. And `ubuntu-latest` carries 79.1% of this repository's Ubuntu job-minutes (3141 of 3970), so this raises our demand on the pinned label 6.2x, onto a pool whose wait was measured while it carried one sixth of that. The pool absorbs 3.6x more jobs than the alias today at a p90 of 0.53 minutes, and the worst pinned figure anywhere in the sample is `apache/iceberg` at 0.43 median against 18.78 on its own alias. If the p90 on `ubuntu-24.04` passes 2 minutes after this lands, that is the signal to revisit, with both sides finally measurable. Refs #4480
652a5e8 to
c290132
Compare
me2seeks
left a comment
There was a problem hiding this comment.
Re-reviewed exact head c290132. The prior coverage risks are addressed: .asf.yaml selects the release-contract check that parses it; dependency patches remain on the fail-safe full path; and only the two owned native roots bypass JavaScript surfaces, with an unknown-native regression. The new cancellation condition is correctly limited to pull-request runs, and the workflow path filters are asserted symmetrically for the existing dual-filter lanes.
Validation: node --test --test-concurrency=1 scripts/ci-test-plan.test.mjs scripts/ci-workflow-policy.test.mjs scripts/verify-windows-harness.test.mjs passed 114/114 locally. act parsed the CI job and its pull_request/push/workflow_dispatch triggers; it could not execute containers because this host has Nerdctl rather than a Docker-API daemon.
me2seeks
left a comment
There was a problem hiding this comment.
Re-reviewed exact head c290132. The prior coverage risks are addressed: .asf.yaml selects the release-contract check that parses it; dependency patches remain on the fail-safe full path; and only the two owned native roots bypass JavaScript surfaces, with an unknown-native regression. The new cancellation condition is correctly limited to pull-request runs, and the workflow path filters are asserted symmetrically for the existing dual-filter lanes.
Validation: node --test --test-concurrency=1 scripts/ci-test-plan.test.mjs scripts/ci-workflow-policy.test.mjs scripts/verify-windows-harness.test.mjs passed 114/114 locally. act parsed the CI job and its pull_request/push/workflow_dispatch triggers; it could not execute containers because this host has Nerdctl rather than a Docker-API daemon.
me2seeks
left a comment
There was a problem hiding this comment.
Blocking follow-up: the new Gitoxide exemption assumes the admission workflow is guaranteed to run, but it is behind pull_request.paths / push.paths. GitHub evaluates path filters from only the first 300 changed files; if a large PR changes native/gitoxide-helper/** outside that window, GitHub does not create this workflow. The unconditional core test then computes its own complete checkout diff, sees the native file, and the new continue at scripts/ci-test-plan.mjs:467-472 emits heavy=false, code=false, runtime_host=false, and no standard workspaces. test is the only required context in .asf.yaml, so it can pass with no Rust or Runtime Host validation of the change.
This is a real escape, not a theoretical unknown-root case: a failed compile or behavior regression in the helper can merge in a sufficiently broad PR while the sole required check is green. The smallest fail-safe fix is to keep native/gitoxide-helper/** on the core fallback (remove only that exemption). native/runtime-host-peer/** and deny.toml can retain their narrower route because the core CI CLI-package selection itself builds the direct-peer addon; Gitoxide has no corresponding core owner. The alternative is to make the Gitoxide owner independent of GitHub path-filter truncation, which necessarily needs a reliable always-scheduled/full-diff gate.
Evidence: locally, the exact planner head returns { heavy: false, code: false, runtimeHost: false, standardWorkspaces: [] } for native/gitoxide-helper/src/main.rs, whereas the existing fallback selects heavy validation and Runtime Host tests. GitHub documents that paths filtering is limited to the first 300 changed files: https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#onpushpull_requestpull_request_targetpathspaths-ignore
Asking for `ubuntu-latest` and asking for `ubuntu-24.04` gets the same machine. Both labels reported `Image: ubuntu-24.04`, `Version: 20260823.283.1`, provisioner `20260819.586`, runner `2.336.0` in their `Set up job` logs on the same day. So this changes which queue a job waits in, and nothing else. The queues are not the same. Across 1183 runner-backed jobs in eleven ASF repositories, `ubuntu-latest` has a median wait of 0.05 minutes and a p90 of 19.07; `ubuntu-24.04` has a median of 0.03 and a p90 of 0.53, while carrying 3.6x the jobs (455 against 126). The alias is not slower on average — it is unpredictable, and a required context is paid at the tail. `apache/flink` sees a p90 of 105.8 minutes on the alias. The sharpest control comes from this repository. One push to #4482 created nine first-layer jobs within the same second, none of them declaring `needs`. The three on `ubuntu-latest` waited 7.5, 7.68 and 7.7 minutes. The six on `ubuntu-24.04`, `ubuntu-24.04-arm`, `windows-2025`, `macos-15`, `windows-latest` and `macos-latest` waited between 3 and 24 seconds. It is not the `-latest` alias as such, since two of the fast six are aliases. It is not our own `concurrency` groups, since every workflow's median `run_started_at - created_at` is 0.0. It is not a self-hosted split, since `runner_group_name` reads `GitHub Actions` on all nine. `ci.yml` also has a reason of its own. Its bubblewrap step disables `apparmor_restrict_unprivileged_userns` specifically because Ubuntu 24.04 gates user namespaces that way. The required context already assumes this image; the alias only left that assumption free to drift without a commit. `windows-latest` and `macos-latest` stay as they are. Across 84 same-instant groups containing both `windows-latest` and `windows-2025`, the median paired difference in wait is 0.00 minutes, so pinning them would buy nothing measurable. Nine substitutions across eight workflows; thirteen jobs in the release and packaging lanes were already pinned. Two costs, both accepted deliberately. The automatic image upgrade becomes a manual commit — `ci-workflow-policy.test.mjs` holds the rule and says how to take an exemption. And `ubuntu-latest` carries 79.1% of this repository's Ubuntu job-minutes (3141 of 3970), so this raises our demand on the pinned label 6.2x, onto a pool whose wait was measured while it carried one sixth of that. The pool absorbs 3.6x more jobs than the alias today at a p90 of 0.53 minutes, and the worst pinned figure anywhere in the sample is `apache/iceberg` at 0.43 median against 18.78 on its own alias. If the p90 on `ubuntu-24.04` passes 2 minutes after this lands, that is the signal to revisit, with both sides finally measurable. Refs #4480
c290132 to
ea30b76
Compare
|
Correction to my requested-fix wording: retaining the core full fallback alone is not sufficient, because core The blocking invariant is therefore: when the complete PR/main diff changes |
|
Rewritten against all three findings, and rebased onto main now that #4483 has landed. @M4n5ter @me2seeks — ready for another look. 1. 2. 3. Non-blocking simplification: agreed, and deleted. You were right that it skipped every entry and proved nothing. Rather than restate the reachability check, it is replaced by the invariant this change actually needs: any lane filtering both One thing you did not raise, which I got wrong myself. Commit 3's rationale was backwards. The coupling is real and still exists — All replay figures were recomputed after Note the |
|
Exact-head follow-up: this branch currently fails its own install-free CI policy suite, so it cannot be approved yet. On This is a real current-base integration issue, not a local dependency failure. Pin that workflow to |
A path the classifier does not recognise sets `unknownCode`, which returns the full suite. That fallback is correct as a default and is the largest single source of full-suite runs, so the fix is to give its biggest occupants an owner rather than to weaken it. `native/gitoxide-helper/` and `native/runtime-host-peer/` each have an admission lane that owns `cargo fmt` and `cargo test` for them, and the direct-peer crate additionally reaches CLI packaging, which builds it into the tarball and runs `cargo deny` against `deny.toml`. Nothing under either is read by lint, typecheck, Storybook, or a real window. They are named one crate at a time rather than by the `native/` prefix. What earns the exemption is having a lane, not being written in Rust: a crate added under `native/` has neither a lane nor a JavaScript consumer on the commit that introduces it, and this fallback is the only thing that would build it at all. A regression asserts that an unowned native root still selects full. `.asf.yaml` goes to `RELEASE_CONTRACT_FILES`. It names the required contexts and the release-environment admission rules, and `product-release.test.mjs` is the suite that parses it and asserts them — so selecting the release contract is what proves a change to the merge gate still passes its own policy test. Routing it anywhere else lets that gate change while the test that guards it is skipped. `patches/` deliberately keeps the fallback. A patch rewrites a dependency's behaviour, and which suite proves that behaviour is a property of the patch rather than of the directory: the node-pty patch is regressed by `packages/runtime/src/__tests__/node-pty-write-lifecycle.test.ts`, while the packaging smoke a build-shaped selection would run only asserts that a method name still appears in the tarball. Narrowing this bucket needs an explicit patch-to-consumer mapping; until one exists the cost here is runner minutes rather than a silent regression. Refs #4480
`cli-package-validation` had a concurrency group but no `cancel-in-progress`, which is the setting that makes a group cancel rather than queue. A run that a newer push had already invalidated therefore ran to completion at full price while its replacement waited behind it. It fans out to about fourteen jobs per run across four build targets and four install environments, so it holds more runner slots than any other workflow here — 736 jobs against CI's 523 over one measured day. In that window 14 of its 60 pull request runs were superseded while still executing, spending 275 of 1672 slot-minutes on work that was already obsolete. Scoped to `pull_request` the way `ci.yml` scopes its own group: release callers arrive through `workflow_call`, where `github.event_name` is the caller's, so a publication run is never cancelled. Refs #4480
`gitoxide-helper-admission.yml` filtered on `packages/runtime/package.json`, which selected three runners across three operating systems on 17 of the last 300 first-parent commits. Dropping it takes that to 5. The coupling that entry was added for in #3561 is real and still exists: `gitoxide-helper-invocation-internal.ts` imports `@maka/runtime/child-process-lifecycle`, a subpath that resolves only through the exports map in that manifest, added by the same commit. Editing the map can break the Gitoxide owner. What changed is not the coupling but the question of who covers it. Core CI does. A change to `packages/runtime/package.json` selects `packages/runtime-host` through the reverse dependency closure, so the import is typechecked and tested on the required context whether or not this lane runs. What the three-runner fan-out proves that core CI cannot is the Rust side and the cross-platform invocation contract, and a manifest edit changes neither. The `pull_request` and `push` lists are edited together, and a rule now holds them that way: any lane that filters both triggers must filter them on the same paths. A lane that looks at one set before a merge and another after it reports a verdict about a tree nobody validated, and that mistake is invisible in a diff showing only the list being edited. `dependency-audit` was already drifting this way — its `push` list omitted the workflow's own file — and is corrected here. Leaving one trigger unfiltered stays available, since that is a visible and sometimes deliberate choice: `windows-recovery.yml` does exactly that, so the merged result is checked even when the pull request was green against a stale base. This replaces a test that asserted every filter entry could reach Gitoxide. Every entry names the subject, so it skipped all of them and proved nothing. Refs #4480
ea30b76 to
8343c55
Compare
me2seeks
left a comment
There was a problem hiding this comment.
Approved on exact head 8343c55. The sibling runner-pin integration is now present and the complete install-free planner/workflow/Windows-harness suite passes 115/115 locally. The CI-load-reduction scope is sound: release contracts, dependency patches, unknown native roots, direct-peer packaging, Gitoxide manifest coverage, and superseded PR package-validation runs all have the intended routing.
The large-diff path-filter hardening remains a worthwhile follow-up, but it is not a blocker for this scoped optimization: the existing admission lane remains the normal native owner, and restoring core full fallback would not compile the helper. It should be addressed separately with a complete-diff-triggered cargo qualification.
Three lanes were allocating runners for inputs that could not change their verdict. Each commit reverts independently. Measurements are replays of
planTests()and of each workflow'spaths:globs over the last 300--first-parentcommits, from the survey in #4480.Nothing here touches
test, its name, or its triggers.1. The full-suite fallback's biggest occupants had owners
A path the classifier does not recognise sets
unknownCode, which returns the full suite. That default is correct and stays; what changed is that its largest occupants now reach the lane that actually validates them.Over 300 commits, 58 selected the full suite. Of those, 18 touched
native/, 7 touched.asf.yaml, and 2 toucheddeny.toml— none of which anyci.ymlstep reads.native/gitoxide-helper/andnative/runtime-host-peer/— each has an admission lane owningcargo fmtandcargo test, and the direct-peer crate additionally reaches CLI packaging, which builds it into the tarball and runscargo denyagainstdeny.toml. Named one crate at a time, not by thenative/prefix: what earns the exemption is having a lane, not being written in Rust. A newnative/new-helperhas neither a lane nor a JavaScript consumer on the commit that introduces it, and the fallback is the only thing that would build it — there is a regression asserting exactly that..asf.yaml— goes toRELEASE_CONTRACT_FILES. It names the required contexts and the release-environment admission rules, andproduct-release.test.mjsis the suite that parses them, behindcheck:release. Selecting the release contract is what proves a change to the merge gate still passes its own policy test.patches/deliberately keeps the fallback. A patch rewrites a dependency's behaviour, and which suite proves that behaviour is a property of the patch, not of the directory: the node-pty patch is regressed bypackages/runtime/src/__tests__/node-pty-write-lifecycle.test.ts, while the packaging smoke a build-shaped selection would run only asserts that a method name still appears in the tarball. Narrowing this needs an explicit patch-to-consumer mapping; until one exists the cost here is runner minutes rather than a silent regression. 5 of the remaining 40 full selections are patches.Replayed over the same 300 commits:
fullappIconsasfSourcestateRootCompatstorybookastryxSurfacee2e2.
cli-package-validationhad a concurrency group but nocancel-in-progressThat is the setting which makes a group cancel rather than queue. Without it a run that a newer push had already invalidated was not cancelled — it ran to completion at full price while its replacement waited behind it in the same group.
It fans out to ~14 jobs per run across four build targets and four install environments, which makes it the largest holder of runner slots in the repository: 736 jobs against CI's 523 over one measured day. In that window 14 of its 60 pull request runs were superseded while still executing, spending 275 of 1672 slot-minutes on work that was already obsolete. Only 5 runs were cancelled, against 76 for CI, which does set this.
Scoped to
pull_requestexactly the wayci.ymlscopes its own group: release callers arrive throughworkflow_call, wheregithub.event_namebelongs to the caller, so a publication run is never cancelled.3.
gitoxide-helper-admissionselected on a manifest core CI already coverspackages/runtime/package.jsonhas been in that filter since #3561. It selected three runners across three operating systems on 17 of the last 300 commits; after this, 5.The coupling it was added for is real and still exists.
gitoxide-helper-invocation-internal.tsimports@maka/runtime/child-process-lifecycle, a subpath that resolves only through the exports map in that manifest, added by the same commit. Editing that map can break the Gitoxide owner. What changed is not the coupling but who covers it.Core CI does. A change to
packages/runtime/package.jsonselectspackages/runtime-hostthrough the reverse dependency closure, so the import is typechecked and tested on the required context whether or not this lane runs. What the three-runner fan-out proves that core CI cannot is the Rust side and the cross-platform invocation contract, and a manifest edit changes neither.A rule now holds the two lists together. Any lane that filters both
pull_requestandpushmust filter them on the same paths — a lane looking at one set before a merge and another after it reports a verdict about a tree nobody validated, and that mistake is invisible in a diff showing only the list being edited.dependency-auditwas already drifting this way (itspushlist omitted the workflow's own file) and is corrected here. Leaving one trigger unfiltered stays available, since that is visible and sometimes deliberate:windows-recovery.ymldoes exactly that, so the merged result is checked even when the pull request was green against a stale base.This replaces a test asserting that every filter entry could reach Gitoxide. Every entry names the subject, so it skipped all of them and proved nothing.
What each change costs
packages/runtime's build script changes what this lane builds.npm run buildunder thecodesurface reaches all four workspaces first.One thing worth a maintainer's eye:
testis the only entry in.asf.yaml'srequired_status_checks.contexts. Every lane named here —gitoxide-helper-admission,cli-package-validation,release-windows-check,windows-recovery— runs on pull requests but cannot block a merge. Change 1 relies on those lanes for Rust coverage, which is the same reliance the repository already has; it does not add to it, but it does make it load-bearing in a place previously masked by the full-suite fallback. Say the word if that trade should be made differently.Verification
node --test --test-concurrency=1 scripts/ci-test-plan.test.mjs scripts/ci-workflow-policy.test.mjs scripts/verify-windows-harness.test.mjs— 114 pass, 0 fail. New assertions written failing first, and each new rule verified red as well as green: the paths-consistency rule was confirmed to fail on an entry added, removed, or renamed on either side.No repo-wide build or test suite was run locally, and no workflow was dispatched.
Refs #4480