Repository navigation
ci: move PR analysis and Linux unit tests to Blacksmith - #29362
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 9 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title clearly identifies the primary change: moving PR analysis and Linux unit tests to Blacksmith. It is concise and specific. Description check The description is detailed and covers the required sections, including the change, rationale, linked issues, testing, review notes, compatibility considerations, and checklist. It explicitly explains…
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
…stablyai/orca into nwparker/ci-blacksmith-linux-capacity
…h-linux-capacity # Conflicts: # mobile/src/session/MobileNativeChatView.test.ts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔇 Additional comments: 7
config/scripts/ci-dependency-download-cache.test.mjs: 1
298-298: LGTM!Also applies to: 349-349, 359-370
config/scripts/pr-preflight-gates.test.mjs: 1
19-19: LGTM!.github/workflows/ci-cache-warmup-platform-jobs.yml: 1
18-18: 🩺 Stability & AvailabilityThe supplied evidence does not show that
actionlintreported an unknown runner label. Existing repository workflows already use Blacksmith labels, includingblacksmith-4vcpu-ubuntu-2404, but runner availability cannot be established from the provided source..github/workflows/unit-tests.yml: 2
178-178: LGTM!
98-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Patch of
/etc/profile.d/blacksmith.shcan silently do nothing.The Python step replaces an exact string. If the image changes that line,
updated == originaland the step succeeds without a change. The next step then fails with "Runner login profile overrides fixture HOME". That failure is detectable, so the risk is low. Log a notice when the file is missing or the pattern is not found. This makes the root cause visible.config/scripts/ci-unit-module-cache.test.mjs: 1
4-4: LGTM!Also applies to: 150-168
config/oxlint-anti-slop.json: 1
75-80: LGTM!
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
2fb6d5b1-774b-46b2-b956-8427e33e3b2e
📒 Files selected for processing: 7
.github/workflows/ci-cache-warmup-platform-jobs.yml.github/workflows/pr.yml.github/workflows/unit-tests.ymlconfig/oxlint-anti-slop.jsonconfig/scripts/ci-dependency-download-cache.test.mjsconfig/scripts/ci-unit-module-cache.test.mjsconfig/scripts/pr-preflight-gates.test.mjs
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.
nwparker
changed the title
ELI5
PRs wait for busy GitHub runners even when checks themselves take only minutes. Move the PR detector, lint/typecheck job, five Linux unit-test shards, and final result to Blacksmith. Keep Linux packaging, relay, Windows, compatibility checks, and other workflows on GitHub.
What Changed
/etc/profile.d/blacksmith.sh; the stock script resets it to/home/runner. Verify Electron startup and an actual login-shell fixture before starting the suite. Fail early if either contract is missing. No tests are skipped or assertions weakened.Why
On #29246, the successful commit took 107m37s to reach the required gate: about 95m15s waiting for runners and 12m22s executing its critical path. The five-second aggregate alone waited 17m21s. The daily demand report estimates 548 PR-check runner-hours and 454 GitHub ARM runner-hours across the repository for the sampled day.
Target the repeated Linux work instead of moving every workflow or increasing shard fan-out. The ARM Blacksmith qualification started jobs within 10–14 seconds but shards ran about 15 minutes and exposed missing Electron libraries, shell-profile differences, and timing failures. Qualify x64 instead: Blacksmith documents its x64 image as following GitHub's x64 image and using gaming CPUs. The 8-CPU preflight leaves memory for overlapping compiler checks; unit shards retain four workers.
Published Linux x64 rates are $0.004/min for 2 CPUs, $0.008/min for 4 CPUs, and $0.016/min for 8 CPUs. On the final commit, all five unit shards passed in 6m30s–6m41s, with 10–11 seconds between admission and runner start. Preflight passed in 1m38s after an 11-second pickup. With minute rounding, the migrated jobs cost approximately $0.32 per full run before any free allowance, excluding retained GitHub jobs. This is usage-based spend, not a configured budget cap. These are observed runs, not a controlled same-commit benchmark against GitHub.
Linked Issue
Maintainer CI maintenance following the backlog observed in #29246; related #29337. No new issue needed.
Visual Proof
N/A — CI runner routing only; no application UI or behavior change.
Testing
461 tests pass across fourteen workflow/cache/aggregate/admission/lint-scope and affected scanner/hook suites. The full anti-pattern scan also passes locally. Changed-code quality, formatting, precommit lint, and
git diff --checkpass. The worker regression evaluates both GitHub/Blacksmith ARM and Blacksmith x64, and confirms other x64 callers retain their default. On final headd240809f16126733a8931ac153aa1329125cc7fc, hosted qualification has passed preflight and all five unit shards. All five shards passed 120,714 tests. Retained GitHub checks and the full aggregate remain pending. The repository owner explicitly instructed merging immediately and skipping pending CI because the queue backlog is the problem this migration addresses; this is an exception to the normal exact-head aggregate requirement.Review
Reuses documented runner labels and existing dependency/native actions. Cache keys still include architecture and native-runtime identity. The omitted Linux warmers' installation and Electron preparation run in the real PR consumers; Windows warmer validation remains separate. Architecture-specific and release checks retain their existing runners. No application behavior or wire protocol changes. The same-size/preserved-mtime generation suite also passes, preserving coverage beyond the updated fixtures. The two later filesystem/timer failures passed ten repeated focused runs after fixing their fixture timing.
Agent skill upstream boundary
Notes
Existing queued runs keep their original workflow; routing takes effect for new runs after merging. This change does not reroute already queued work. Nine stale closed/merged-PR runs were separately requested to cancel after rechecking their state and identity; a still-active merged-PR run was force-cancelled after normal cancellation did not release it.
Checklist