Repository navigation
ci: bound mini execution with parallel hosted macOS fallback - #1126
Conversation
XuPeng-SH
left a comment
There was a problem hiding this comment.
Request changes: two actionable integration defects, detailed in the inline comments.
Independent one-off review of the entire PR (all 16 changed files and all four commits), against head 8305e835dd76cea2fe2e7d13dca99eb6b8f5a908 and base 8cbabcce326e4cdb8f1f84cf5f5a37964ae242fd. I revalidated the open/non-draft PR, authenticated reviewer XuPeng-SH (separate from the author), unchanged head, and empty existing reviews/inline comments immediately before submission. The current synthetic merge 5ba28ffd847164f73ad2c6c4d82b6f282df229dd has exactly the ordered base/head parents.
The two blockers are (1) hosted individual/failed-only reruns can execute with reused admission, bypassing the attempt/expiry enforcement, and (2) real fork run responses with an empty PR association cannot complete the ordinary App-owned checks. These are implementation/integration defects in the disabled-by-default migration; missing live activation acceptance is not itself a finding.
I traced routing, App/check source binding, source refresh, runner-group admission and root-owned helper boundaries, privileged execution/cache separation, fallback, publication, expiry and recovery. The canonical classifier and extracted lane script preserve the useful existing coverage and avoid duplicating lane commands. The substantial controller complexity addresses concrete trust/recovery requirements, but the passing policy-unit tests do not establish the two workflow/API integrations above. I found no additional actionable issue in the ctutils-only lockfile update: the 0.4.4 registry archive SHA-256 matches Cargo.lock, and the current RustSec advisory lists >=0.4.4 as patched (https://github.com/RustSec/advisory-db/blob/main/crates/ctutils/RUSTSEC-2026-0385.md).
Verification:
- Migration suite: 45/45 passed. CI contract suites: 50/50 passed.
- Two additional isolated offline reproductions confirmed the findings; no remote CI was rerun.
- Actionlint 1.7.12 with the repository's narrow compatibility filters, repository metadata validation and diff whitespace checks passed.
- Full local CI Python suite: 199 tests, four failures. One unchanged build-metadata fixture attempted to use the sandbox's read-only default Cargo directory. Three harness process/cancellation tests also reproduce on the unchanged base. These are disclosed limits, not attributed to this PR.
- Full locked offline Cargo resolution was unavailable because the saved registry index lacks locked rustls 0.23.46. Full Rust/macOS execution, fresh cargo-audit, runner installation, actual App-token/cache enforcement, host isolation and capacity qualification were not performed.
Change accounting (additions/deletions/net, no pure file moves): implementation/configuration/dependency 1107/62/+1045; tests 692/30/+662; documentation 318/0/+318.
Current exact-head CI: all 41 checks were inspected (complete single page): 38 success, one skipped, one neutral, one failure. Sources are GitHub Actions App 15368 and Mergify App 10562. The failed live harness is still 16/18: flash_child_question_parent_answer timed out with child-count/adoption mismatch; work_two_step_execution_journey failed journal assertions despite judge acceptance. I read the current job logs; neither flakiness nor PR causation is established (https://github.com/matrixorigin/Astra/actions/runs/39981298500/job/120508972896).
The accessible ruleset list was fully enumerated and both ruleset details inspected. Effective target-rule lookup is unavailable through this connector, and I did not retry or bypass the previously denied classic branch-protection administration endpoint. Required-context/source-binding and protection configuration therefore remain unknown; this review does not certify merge gates or claim all required CI passed. Migration remains an opt-in implementation with separate live administrative/runner/capacity acceptance.
This was a manual one-off review. No automatic review loop was restarted, and no activation, production/admin change, branch push/synchronization, deployment or direct merge was performed.
8305e83 to
50466af
Compare
e0ad690 to
80ff991
Compare
XuPeng-SH
left a comment
There was a problem hiding this comment.
Request changes: the two prior findings (hosted rerun admission and empty fork PR associations) appear fixed, but I found a separate result-ownership defect for different PRs that share one head SHA. The ordinary and protected paths write the same three App check names against that SHA. I reproduced an external PR's successful ordinary result replacing a member PR's failed protected results; reconciliation skips the member PR because the latest plan names the other PR, and the older publisher cannot restore its result. The required checks therefore no longer prove that the member PR's protected path passed. See the inline P1 for the concrete trigger and requested regression coverage.
Validation at this head: all 51 migration tests pass; the 205-test Python suite passes with 6 existing conditional skips. This review does not certify live App/runner activation or capacity.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Request changes: the same persistent self-hosted runner is shared by allowlisted pull requests and trusted main/develop pushes, allowing pre-merge PR code to contaminate later trusted CI. See the P1 inline finding. I have not included the earlier allowlist-modification finding as a blocker.
The Rust toolchain change also disables sccache on the current macOS lanes because the setup installs rustup with no default toolchain, while the sccache probe invokes rustc without an explicit toolchain. This is a non-blocking build-time regression; see the P2 inline finding.
Validation at this head: CI Python suite (160 tests; 6 skipped) and actionlint passed. Exact-head GitHub checks currently have one unrelated live harness failure; I did not attribute it to this PR.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed the complete PR at 58b1c7fab476c098a9f31e1083672ae64a3d7f03 against main.
The previously reported shared trusted-runner state issue is addressed: only eligible PRs use the mini, while main/develop pushes use hosted macOS. The rustup/sccache toolchain propagation is now covered through the production health probe. The earlier App-specific result ownership/rerun findings no longer apply to this simplified ordinary-workflow path.
No new blocking code finding. Exact-head checks currently show 41 successes and one neutral Mergify check. The current Test Suite run selected the real hosted fallback after the mini workflow lookup returned HTTP 404 because the new worker/controller workflows are not on the default branch yet. The mini dispatch/admission path therefore still needs the documented post-merge pilot, including runner-group access and queue/cancellation behavior.
|
This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again. |
Capability assessment: Unchanged: CI scheduling and test infrastructure change; no product capability status or runtime profile is added. The dependency audit fix preserves the existing JWT algorithms.
Summary
Give the single self-hosted ARM Mac a bounded opportunity to run macOS tests, with real parallel hosted execution when it cannot serve the run. The explicit allowlist contains the eleven selected Astra contributor account IDs, including Dependabot. Trusted main/develop pushes use hosted macOS; release platforms remain unchanged. Only allowlisted PRs can use the persistent mini; other or unknown PR authors use hosted macOS directly.
Five Mac lanes share one candidate checkout, toolchain setup and cache restore on the mini: default reader/Bash, reflow, workspace coordination, vendored units and dev-tty. Their commands have one owner,
scripts/ci/run-macos-lanes.sh, used by both mini and hosted/Linux execution. Cargo invocations stay separate, with dev-tty last.Scheduling and required results
Linux execution depends only on scope, so it starts independently of the mini wait. The original GitHub Actions required-check names remain in Test Suite. The Mac jobs on Ubuntu require a verified mini success; on fallback they run the real tests on
macos-15. There are no custom App contexts or new branch-protection bindings.Implementation and review invariants
macos-scheduler.ymlis a default-branchworkflow_runcontroller using the built-in token's Actions write permission. It only dispatches/cancels workers and never executes candidate code.macos-mini.ymluses a read-only token andcache-mode: read. It rejects an expired source gate or a worker rerun before candidate checkout.pull_requests: []work without an organization-membership query.in_progressjob that may already be doing post-job cleanup.GITHUB_ENV, so sccache's compiler probe works outside the checkout even when rustup has no default. Dependency-audit updates and PTY fixture stabilization remain. JWT uses AWS LC to remove the unpatched crypto-bigint path; HMAC and 2048-bit RSA/JWK interoperability/rejection cases were previously checked. AWS LC's RSA key range is 2048–8192 bits.Latest review fixes
Bootstrap and operational limits
The two new workflows must enter main before GitHub will trigger the controller. Before that, this PR exercises hosted fallback. No App installation, secret, migration flag or custom admission hook is needed. The runner/group must allow
macos-mini.yml@mainwith labels[self-hosted, macOS, ARM64, mac-arm64-mini].The 180-second budget includes polling/discovery, with separately bounded API requests. Service/API outages may prevent immediate child cancellation; the parent still takes hosted fallback and the completion observer retries cleanup. Native service availability and queueing still affect elapsed time.
One registration runs one bundle at a time. Bundling removes repeated preparation, not the physical capacity limit. On preceding head
b7efd37a, the five individual mini jobs all passed and occupied about 25m34s from first creation to last completion. That is a baseline, not a measurement of this new bundle/cache mode. Queue delay, fallback rate, resource headroom and net hosted-minute savings still need a post-merge pilot.If old controller configuration was installed, retire its unused App required contexts/admission hook while preserving the existing GitHub Actions checks and fork approvals. The administrator handbook is maintained separately.
Validation and review status
python3 -m unittest discover -s scripts/ci: 192 tests, 0 failures/errors, 6 conditional skips.git diff --check: passed.name.b7efd37a, all five mini lanes and static checks passed. The live harness was 17/18:flash_child_question_parent_answerfailed withBehaviorContractViolation. This remains a failing validation; no causal attribution or assertion relaxation is claimed.58b1c7fa, Test Suite reached hosted fallback and all five Mac jobs started in parallel within one second of each other. The full live CI is still running. Changes requested reviews are not treated as approval.Refs #1106.