ci: shard slow cross-platform test lanes - #51
Conversation
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 57 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request adds deterministic Bun test sharding and integrates it with a cross-platform CI matrix. Windows tests use two shards and retry only qualifying Bun runtime crashes. Quality and typecheck steps follow matrix controls. ChangesCross-platform CI test sharding
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Matrix as GitHub Actions matrix
participant Runner as CI runner
participant ShardRunner as ci-test-shard.ts
participant BunTest as Bun test
participant Quality as Quality checks
Matrix->>Runner: select OS, shard, and control flags
Runner->>ShardRunner: run configured test shard
ShardRunner->>BunTest: execute selected files in batches
BunTest-->>ShardRunner: return process status
ShardRunner-->>Runner: propagate test result
Runner->>Quality: run conditional checks
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
yone_suke (@codesmith) independently review exact head
Fix or report every P1/P2 blocker rather than only summarizing. |
|
Got it, I'm reviewing the PR security diff first. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/ci-test-shard.ts`:
- Around line 60-70: Update runBatch in scripts/ci-test-shard.ts (lines 60-70)
to invoke the canonical scripts/test.ts orchestration, including its isolated
environment and process.execPath handling, before running Bun tests; do not
spawn bun directly with process.env. Also update the single-shard workflow path
in .github/workflows/ci.yml (lines 131-136) to use the same canonical test entry
point.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2bccace1-3ad5-4e98-b80a-0dfac88bc3c2
📒 Files selected for processing (2)
.github/workflows/ci.ymlscripts/ci-test-shard.ts
|
[code]smith (@codesmith-bot) final exact-head audit for |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Independent security review of Blockers found at
Proven invariants (helper executed for real, shard args captured via spawn interception):
Advisories (P3, no action taken): (a) when required checks get configured per MAINTAINERS.md, the three new lane names ( CI for the current head |
![View with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg)
Audit of exact head Executed proofs at
Blockers at
Two sandbox-only artifacts from my real runs are not defects: CI for the current head |
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
4117dad to
1994a6f
Compare
|
Pickup status (cloud agent): Rebased onto current Merge readiness: CI was green pre-rebase; awaiting post-rebase CI + security sign-off on workflow changes (AGENTS.md). Recommend merge first in the open train — unblocks Windows ~13m timeout pressure on #52/#53. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/ci-test-shard.ts`:
- Around line 36-40: Add focused regression tests in tests/ci-test-shard.test.ts
covering parseInteger and main validation: missing or non-integer CLI arguments,
shardCount below 1, and shardIndex outside the valid range. Reuse the existing
test patterns and verify each invalid configuration is rejected with the
expected validation error, without changing collectTestFiles or
assignBalancedShards coverage.
- Around line 62-73: Update the path arguments assembled for runBatch in
scripts/ci-test-shard.ts so every selected test file is prefixed with ./ before
being passed to Bun.spawn, ensuring Bun treats each argument as an exact path
rather than a substring filter. Preserve the existing batching and spawn command
structure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8f071ff4-8c15-4771-b299-8078c4a4dbe6
📒 Files selected for processing (4)
.github/workflows/ci.ymlscripts/ci-test-shard.tstests/ci-test-shard.test.tstests/ci-workflows.test.ts
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/ci-test-shard.test.ts (1)
37-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert source-byte balancing.
tests/ci-test-shard.test.tsLines 37-49 only prove that assignment is deterministic. A deterministic implementation can still assign files to highly uneven shards.Assert the byte totals for this fixture. The
500, 300, 300, 100fixture should produce two shards with600source bytes each.Proposed test update
- const fromOrdered = assignBalancedShards(files, 2).map(shard => shard.map(file => file.path)); + const orderedShards = assignBalancedShards(files, 2); + const fromOrdered = orderedShards.map(shard => shard.map(file => file.path)); const fromShuffled = assignBalancedShards(shuffled, 2).map(shard => shard.map(file => file.path)); expect(fromShuffled).toEqual(fromOrdered); + expect( + orderedShards.map(shard => shard.reduce((total, file) => total + file.bytes, 0)), + ).toEqual([600, 600]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ci-test-shard.test.ts` around lines 37 - 49, Extend the test named “assignment is deterministic regardless of discovery order” to also calculate each returned shard’s total source bytes from the fixture and assert that the two totals are 600 each. Keep the existing determinism assertion, and use the results from assignBalancedShards for the byte-balance check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/ci-test-shard.test.ts`:
- Around line 59-61: Extend the parameterized test for parseInteger in
“parseInteger rejects non-integer value” with a whitespace-only string, ensuring
the value.trim() === "" validation branch is exercised and still throws the
shardCount integer error.
---
Outside diff comments:
In `@tests/ci-test-shard.test.ts`:
- Around line 37-49: Extend the test named “assignment is deterministic
regardless of discovery order” to also calculate each returned shard’s total
source bytes from the fixture and assert that the two totals are 600 each. Keep
the existing determinism assertion, and use the results from
assignBalancedShards for the byte-balance check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bab2b617-7e7b-4ad4-baf4-fcc4fa42058f
📒 Files selected for processing (2)
scripts/ci-test-shard.tstests/ci-test-shard.test.ts
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
1 participant
Problem
The combined provider-security and admission suites now exceed the existing 20-minute serial job boundary on slow runners:
These are timeout cancellations, not test failures. The workflow explicitly says not to raise the ceiling again when Windows approaches it.
Change
Keep
timeout-minutes: 20and split work instead:macos-latestandwindows-latestcheck names remain attached to root-test lanes.scripts/ci-test-shard.tsdiscovers Bun test files, balances them greedily by source size, sorts deterministically, and executes bounded batches to avoid Windows command-line limits.Gates
This PR changes CI orchestration only; it does not alter product/runtime code.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is enabled.Summary by CodeRabbit
Tests
Quality Checks
Greptile Summary
This change separates slow-platform quality work from root-test work and runs the Windows root suite through two deterministic shards. Focused execution confirmed that the current shard selections cover all 459 discovered root tests exactly once, but the repository has no automated check to preserve that property. Merge should wait for a regression test that prevents future shard changes from skipping or duplicating Windows test execution.
Confidence Score: 4/5
Not merge-safe until Windows shard coverage is protected by an automated invariant.
The current two-shard execution is complete and duplicate-free, but a future change to discovery, normalization, batching, or assignment can silently omit or repeat tests because no regression test enforces the required coverage property.
Files Needing Attention: scripts/ci-test-shard.ts needs an importable or otherwise testable shard-selection surface, with a focused test aligned to the two-shard Windows workflow.
What T-Rex did
Comments Outside Diff (1)
General comment
windows-latestassignments jointly invoke Bun on every root test discovered by the script exactly once. The current focused execution passes (459 discovered; 229 + 230 assigned; zero missing and duplicates), but that behavior is unprotected against future changes to discovery, path normalization, batching, or partitioning.collectTestFilesandassignBalancedShardsare private functions in a standalone executable script, and the repository contains no test or harness that runs both configured shard indices and compares their union against discovery/Bun invocation inputs.scripts/ci-test-shard.ts(or extract its discovery/partition logic into an importable module) that builds/uses representative root tests, runs shard indices 0 and 1, captures the explicitbun test --isolatepath arguments, and asserts union equals discovery with no duplicates. Keep the assertion aligned with the CI matrix shard count of two.Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "ci: shard slow cross-platform test lanes" | Re-trigger Greptile