test(bun-create): skip GitHub template tests when API rate-limited#29956
Conversation
|
Updated 8:35 PM PT - Apr 29th, 2026
✅ @robobun, your commit 0d27b9c328bd098a9baa1b54cd537ca2dd8cb7e8 passed in 🧪 To try this PR locally: bunx bun-pr 29956That installs a local version of the PR into your bun-29956 --bun |
WalkthroughAdds a GitHub 403 rate-limit detector and updates Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 0/5 reviews remaining, refill in 53 minutes and 58 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/cli/install/bun-create.test.ts (2)
122-139:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrain both pipes before awaiting process exit.
await exitedruns before either stream is consumed, so a chattybun createrun can block on a full stdout/stderr buffer and hang the test. Start both reads immediately, then wait on the exit promise.🔧 Suggested fix
const { stdout, stderr, exited } = spawn({ cmd: [bunExe(), "create", "https://github.com/dylan-conway/create-test", "."], cwd: x_dir, stdout: "pipe", stdin: "inherit", stderr: "pipe", env, }); - await exited; - - const [out, err] = await Promise.all([stdout.text(), stderr.text()]); + const outPromise = stdout.text(); + const errPromise = stderr.text(); + const [out, err] = await Promise.all([outPromise, errPromise]); + await exited; if (isGithubRateLimited(err)) return; expect(out).toContain("bun dev"); expect(out).not.toContain("\n\n cd \n bun dev\n\n"); }, 20_000);As per coding guidelines, when spawning processes, expect stdout output before expecting exit code to get more useful error messages on failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli/install/bun-create.test.ts` around lines 122 - 139, The test "should not mention cd prompt when created in current directory" reads process exit (exited) before draining stdout/stderr which can deadlock; change the order in the test that uses spawn(...) so you start both reads (call stdout.text() and stderr.text() immediately and capture their promises) and then await Promise.all on those read promises together with the exited promise (or await the reads before awaiting exited) — update references in this file to ensure the promises from stdout.text() and stderr.text() are created before awaiting the exited promise (look for variables exited, stdout.text(), stderr.text(), and spawn).
141-159:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRead stdout and stderr concurrently here as well.
await stderr.text()blocks before stdout is drained, which can deadlock if the child emits enough output. Read both streams together, then inspect the error text and exit code.🔧 Suggested fix
it(`should create and install github template from ${repo}`, async () => { const { stderr, stdout, exited } = spawn({ cmd: [bunExe(), "create", repo], cwd: x_dir, stdout: "pipe", stderr: "pipe", env, }); - const err = await stderr.text(); + const [out, err] = await Promise.all([stdout.text(), stderr.text()]); if (isGithubRateLimited(err)) return; expect(err).not.toContain("error:"); - const out = await stdout.text(); expect(out).toContain("Success! dylan-conway/create-test loaded into create-test"); expect(await exists(join(x_dir, "create-test", "node_modules", "jquery"))).toBe(true); expect(await exited).toBe(0); }, 20_000);As per coding guidelines, when spawning processes, expect stdout output before expecting exit code to get more useful error messages on failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli/install/bun-create.test.ts` around lines 141 - 159, The test reads stderr before stdout which can deadlock; update the test around the spawn invocation (the call to spawn and the variables stderr, stdout, exited) to read both streams concurrently and await the exit together: collect stderr.text() and stdout.text() in parallel (e.g., Promise.all) and await exited afterwards, then inspect the resolved stderr and stdout values and the exit code (exited) for assertions instead of calling await stderr.text() first.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/cli/install/bun-create.test.ts`:
- Around line 122-139: The test "should not mention cd prompt when created in
current directory" reads process exit (exited) before draining stdout/stderr
which can deadlock; change the order in the test that uses spawn(...) so you
start both reads (call stdout.text() and stderr.text() immediately and capture
their promises) and then await Promise.all on those read promises together with
the exited promise (or await the reads before awaiting exited) — update
references in this file to ensure the promises from stdout.text() and
stderr.text() are created before awaiting the exited promise (look for variables
exited, stdout.text(), stderr.text(), and spawn).
- Around line 141-159: The test reads stderr before stdout which can deadlock;
update the test around the spawn invocation (the call to spawn and the variables
stderr, stdout, exited) to read both streams concurrently and await the exit
together: collect stderr.text() and stdout.text() in parallel (e.g.,
Promise.all) and await exited afterwards, then inspect the resolved stderr and
stdout values and the exit code (exited) for assertions instead of calling await
stderr.text() first.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cc8aa460-b2db-479a-a86e-636d06e81892
📒 Files selected for processing (1)
test/cli/install/bun-create.test.ts
The three tests that hit github.com/dylan-conway/create-test go through api.github.com/repos/.../tarball, which is limited to 60 req/hr per IP when unauthenticated. CI Windows agents running many parallel builds exhaust that quickly — builds 49246–49254 all failed bun-create.test.ts with 'GitHub returned 403' on robobun-windows-1. Capture stderr on these tests and early-return with a warning when the rate-limit error is present. We are testing bun create, not GitHub's availability.
robobun
force-pushed
the
farm/8be201e1/bun-create-github-ratelimit
branch
from
f28fc34 to
900de42
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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)
test/cli/install/bun-create.test.ts (1)
151-158:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRead both pipes before deciding to skip.
This branch returns after draining only
stderr, so the rate-limit path can leavestdoutunread and lose the command output if the subprocess starts producing enough data to back up the pipe. Collect both streams together, then inspectstderrfor the skip condition.🔧 Proposed fix
- const err = await stderr.text(); - if (isGithubRateLimited(err)) return; - expect(err).not.toContain("error:"); - const out = await stdout.text(); + const [out, err] = await Promise.all([stdout.text(), stderr.text()]); + if (isGithubRateLimited(err)) return; + expect(err).not.toContain("error:");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli/install/bun-create.test.ts` around lines 151 - 158, The test currently drains only stderr into `err` before returning on `isGithubRateLimited(err)`, which can drop `stdout` data and cause the child to block; change the logic to read both streams first (e.g. await both `stderr.text()` and `stdout.text()` together, using `Promise.all` or similar) into local variables, then check `isGithubRateLimited` against the captured `stderr` and return if needed, otherwise continue asserting on `err`, `out`, `exists(join(x_dir, "create-test", "node_modules", "jquery"))`, and `exited` as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/cli/install/bun-create.test.ts`:
- Around line 114-120: The isGithubRateLimited predicate is too broad and hides
genuine 403 errors; update isGithubRateLimited(stderr: string) to match the
exact rate-limit message emitted by create_command.zig instead of just
includes("GitHub returned 403"). Change the check to look for the full
rate-limit text (e.g. the exact substring emitted by create_command.zig such as
"GitHub returned 403: API rate limit exceeded" or a narrowly scoped regex that
matches that precise message) so only transient rate-limit failures are skipped
and real 403 regressions still fail.
---
Outside diff comments:
In `@test/cli/install/bun-create.test.ts`:
- Around line 151-158: The test currently drains only stderr into `err` before
returning on `isGithubRateLimited(err)`, which can drop `stdout` data and cause
the child to block; change the logic to read both streams first (e.g. await both
`stderr.text()` and `stdout.text()` together, using `Promise.all` or similar)
into local variables, then check `isGithubRateLimited` against the captured
`stderr` and return if needed, otherwise continue asserting on `err`, `out`,
`exists(join(x_dir, "create-test", "node_modules", "jquery"))`, and `exited` as
before.
🪄 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
Run ID: c634d560-b096-481a-9fac-3a4bf1153797
📒 Files selected for processing (1)
test/cli/install/bun-create.test.ts
|
✅ No merge conflicts detected when merging into Your branch is good to go! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/cli/install/bun-create.test.ts (1)
150-157:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnsure the rate-limit skip path still waits for process exit
This branch can
returnbeforeawait exited, so the test may finish while the spawnedbun createprocess is still tearing down. Captureexitedin the same concurrent read and reuse the resolved exit code.🔧 Proposed fix
- const [out, err] = await Promise.all([stdout.text(), stderr.text()]); + const [out, err, exitCode] = await Promise.all([stdout.text(), stderr.text(), exited]); if (isGithubRateLimited(err)) return; expect(err).not.toContain("error:"); expect(out).toContain("Success! dylan-conway/create-test loaded into create-test"); expect(await exists(join(x_dir, "create-test", "node_modules", "jquery"))).toBe(true); - expect(await exited).toBe(0); + expect(exitCode).toBe(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli/install/bun-create.test.ts` around lines 150 - 157, Test may return early on rate-limit because exited isn't awaited; capture the spawned process's exit promise alongside stdout/stderr (e.g., const [out, err, exitCode] = await Promise.all([stdout.text(), stderr.text(), exited])) then call isGithubRateLimited(err) and if skipping reuse the already-resolved exitCode for the final expectation (expect(exitCode).toBe(0)); update references to exited, stdout, stderr, and isGithubRateLimited accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/cli/install/bun-create.test.ts`:
- Around line 150-157: Test may return early on rate-limit because exited isn't
awaited; capture the spawned process's exit promise alongside stdout/stderr
(e.g., const [out, err, exitCode] = await Promise.all([stdout.text(),
stderr.text(), exited])) then call isGithubRateLimited(err) and if skipping
reuse the already-resolved exitCode for the final expectation
(expect(exitCode).toBe(0)); update references to exited, stdout, stderr, and
isGithubRateLimited accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a44ae38-9234-4687-9ac0-c96dc3e8cb96
📒 Files selected for processing (1)
test/cli/install/bun-create.test.ts
There was a problem hiding this comment.
LGTM — test-only CI flakiness fix; the stderr-surfacing nit from my earlier review was addressed in baece0d.
Extended reasoning...
Overview
This PR touches a single test file, test/cli/install/bun-create.test.ts. It adds an isGithubRateLimited() helper that inspects captured stderr for the string "GitHub returned 403" and early-returns from three GitHub-hitting bun create tests when the unauthenticated GitHub API quota (60 req/hr/IP) is exhausted. It also switches one test's stderr from "inherit" to "pipe" and drains stdout/stderr concurrently via Promise.all. No production code is touched.
Security risks
None. This is test infrastructure only — it changes how a test suite reacts to an external-service error and does not affect the bun create binary, auth, network handling, or any shipped code path.
Level of scrutiny
Low. The change is small (~20 lines net), mechanical, and confined to test reliability. The detection string was verified against src/cli/create_command.zig:290, where "GitHub returned 403" appears exactly once and only on the error.HTTPForbidden rate-limit branch — so CodeRabbit's suggestion to match the full sentence is unnecessary (the shorter prefix is already unambiguous and is more resilient to cosmetic changes in the error text). The worst-case failure mode is a test silently passing when GitHub is down, which is the explicit intent of the PR.
Other factors
My earlier inline comment asked for expect(err).not.toContain("error:") after the rate-limit check in the "cd prompt" test so non-403 failures still surface stderr in CI; commit baece0d ("review: surface stderr on non-403 failures; drain streams concurrently") addressed that and also fixed the sequential await stderr.text() / await stdout.text() ordering. The bug-hunting system found no issues this run. The PR description cites 7 consecutive Windows CI builds broken by this flake, so the motivation is well-documented.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/cli/install/bun-create.test.ts`:
- Around line 132-138: The test currently awaits Promise.all([stdout.text(),
stderr.text(), exited]) but only destructures [out, err] and never asserts the
process exit code; update the destructuring to capture the third value (e.g.,
[out, err, exitCode]) and add a final assertion expecting the exitCode to be 0
in the non-rate-limited path so non-403 failures cannot pass on partial output
checks (references: variables stdout, stderr, exited and the test block in
bun-create.test.ts).
🪄 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
Run ID: ba7eaa2e-5653-42ab-9e34-1e0cacc21cfd
📒 Files selected for processing (1)
test/cli/install/bun-create.test.ts
There was a problem hiding this comment.
LGTM — test-only CI flakiness fix; prior feedback addressed.
Extended reasoning...
Overview
This PR touches a single test file, test/cli/install/bun-create.test.ts. It adds an isGithubRateLimited() helper that inspects captured stderr for the GitHub returned 403 string emitted by src/cli/create_command.zig:290, and applies it to the three tests that hit api.github.com so they soft-skip (with a console.warn) instead of failing when CI agents exhaust the unauthenticated 60 req/hr quota. It also switches the affected tests to drain stdout/stderr/exited concurrently via Promise.all and adds expect(err).not.toContain("error:") so non-403 failures still surface stderr in the assertion diff.
Security risks
None. This is test-infrastructure only — no production code, no auth, no crypto, no user-facing surface. The change does not introduce new external calls; it only changes how an existing external-call failure is reported.
Level of scrutiny
Low. Test-only, ~30 lines net, mechanical pattern (pipe stderr → check substring → early return). The detection string was verified against the single emission site in create_command.zig, so the author's rationale for keeping the shorter prefix over CodeRabbit's full-sentence suggestion is sound. The worst-case downside is a test silently passing under rate-limit, which is the explicit intent and is mitigated by the visible console.warn.
Other factors
My prior inline comment (surface stderr on non-403 failures) was addressed in baece0d and the thread is resolved. The CodeRabbit thread was answered with a correct justification and resolved. The bug-hunting system found no issues on the current revision. The concurrent Promise.all drain is an improvement over the original sequential await exited; await stdout.text() ordering. No CODEOWNERS or critical-path code involved.
…ven-sh#29956) ## Problem `test/cli/install/bun-create.test.ts` has three tests that spawn `bun create https://github.com/dylan-conway/create-test`. Internally that fetches `https://api.github.com/repos/dylan-conway/create-test/tarball`, and unauthenticated GitHub API is capped at **60 requests/hour per IP**. When a CI agent runs many builds back-to-back it exhausts that quota and every subsequent build fails this test with: ``` error: GitHub returned 403. This usually means GitHub is rate limiting your requests. ``` This is currently failing the Windows shards on **builds 49246, 49247, 49250, 49251, 49252, 49253, 49254** — every recent PR touching `robobun-windows-1` — and none of those failures are related to the PRs' actual changes. ## Fix Capture stderr on the three GitHub-hitting tests and early-return with a `console.warn` when it contains `GitHub returned 403` (the exact string emitted by `src/cli/create_command.zig` on `error.HTTPForbidden`). The tests still run and assert normally when the API is reachable; they just stop reporting an external-service rate limit as a test failure. `bun create` already reads `GITHUB_TOKEN` / `GITHUB_ACCESS_TOKEN` from the environment (`create_command.zig:1976`), so authenticated agents are unaffected. ## Verification ``` $ bun bd test test/cli/install/bun-create.test.ts 13 pass 0 fail ``` The detection string is checked against `create_command.zig` so it won't silently rot if the error message changes. --------- Co-authored-by: robobun <robobun@users.noreply.github.com>
2 participants
Problem
test/cli/install/bun-create.test.tshas three tests that spawnbun create https://github.com/dylan-conway/create-test. Internally that fetcheshttps://api.github.com/repos/dylan-conway/create-test/tarball, and unauthenticated GitHub API is capped at 60 requests/hour per IP.When a CI agent runs many builds back-to-back it exhausts that quota and every subsequent build fails this test with:
This is currently failing the Windows shards on builds 49246, 49247, 49250, 49251, 49252, 49253, 49254 — every recent PR touching
robobun-windows-1— and none of those failures are related to the PRs' actual changes.Fix
Capture stderr on the three GitHub-hitting tests and early-return with a
console.warnwhen it containsGitHub returned 403(the exact string emitted bysrc/cli/create_command.zigonerror.HTTPForbidden). The tests still run and assert normally when the API is reachable; they just stop reporting an external-service rate limit as a test failure.bun createalready readsGITHUB_TOKEN/GITHUB_ACCESS_TOKENfrom the environment (create_command.zig:1976), so authenticated agents are unaffected.Verification
The detection string is checked against
create_command.zigso it won't silently rot if the error message changes.