feat(js): add Bun runtime support (bun test, run, bunx) - #1374
HenriqueSchroeder wants to merge 30 commits into
Conversation
📊 Automated PR Analysis
SummaryAdds comprehensive Bun runtime support to RTK, including filters for bun test, bun install, bun build, bun run, and bunx commands. Implements intelligent routing for bunx to specialized filters (tsc, eslint, prisma) and includes snapshot tests, token savings assertions, and clap routing tests. Review Checklist
Analyzed automatically by wshm · This is an automated analysis, not a human review. |
607535b to
18e6b1e
Compare
23184d4 to
d6493bb
Compare
|
Bumping. Great to see this coming soon. Much appreciated library btw. Been using for a few weeks with success. |
16911ea to
a87b861
Compare
|
Reviewed this locally (build + the 17 bun tests pass, clippy clean) — solid work, and it's the most complete of the open Bun PRs: the dedicated 1. Rebase needed — the branch is stale. On the current branch 2. Coordinate the Substance is good — happy to re-review once rebased. |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Bun prints (pass)/(fail) when stdout is piped (RTK's case) but the initial filter only matched TTY ✓/✗ markers. Rewrote filter_bun_test to: - handle both TTY and piped formats - buffer lines preceding a fail marker (Bun emits error context before the marker) and flush them on match - skip the trailing "N tests failed:" summary (duplicates already captured failures) Replaced synthetic fixtures with real bun test output and added bun_test_real.txt to guard the >=60% token savings target under real-world conditions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- README.md: added rtk bun test (Test Runners), rtk bun build + rtk bunx (Build & Lint), rtk bun install + rtk bun run (Package Managers) - docs/guide/what-rtk-covers.md: added bun test/install/build/run/bunx rows to the JavaScript / TypeScript table Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…, prettier, next, playwright
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
develop's test_every_subcommand_is_classified invariant requires every subcommand to be listed in RTK_META_COMMANDS or PASSTHROUGH. bun and bunx wrap real binaries, so they belong in PASSTHROUGH. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Marker/summary detection matched on bare prefixes/substrings, so ordinary
application console output could corrupt the failure report:
- "FAILED to connect..." tripped is_fail_marker (starts_with("FAIL")) and
fabricated a failure on a passing run; "FAIL" is not even a bun marker.
- "✓ ..."/"5 passengers ..." were mistaken for a pass marker / summary line
and cleared the buffered error context of a real failure.
Real bun markers always end with a "[N.NNms]" timing suffix and the summary
lines are exactly "N pass"/"N fail"/"N skip"/"N expect() calls", so anchor
on those instead of loose prefix matching.
Also flush the error buffer when no markers/summary are recognized (e.g. a
module-resolution error that aborts before any test runs) instead of
collapsing to a misleading "ok" that hid the failure.
Adds regression tests for all four cases.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`bun run <script>` executes an arbitrary user script; forcing LC_ALL=C on the whole child tree changes its behaviour (e.g. a Python step falling back to ASCII stdio), so only test/install/build keep it for consistent parsing. filter_bun_build now also keeps sentence-case "Warn"/"Warning" lines, not just lowercase "warn"/"Error". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
bb4a667 to
fd0e667
Compare
|
Thanks for the rebase. The review threads are in good shape: I re-checked the 19 resolved ones against the code and they have all landed ( Two blockers before this can go green, both from develop moving under the merge:
With those the test binary should build again. Please run Two threads are still open with a reply but no resolution; a one-line confirmation each would let them be closed:
Coordination: #2418 carries the same Once the build is green I am happy to re-review. The |
lazy_static was dropped from the crate in 5269df7, so the bun statics no longer compiled. They now follow the playwright_cmd.rs pattern. The macro used to hide unused statics from the dead_code lint; BUN_INSTALL_DONE_RE, BUN_INSTALL_SPEED_RE and NOISE_LINE_RE were never referenced and are now gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
RtkRule gained pipeline_final_safe in 523c803. The two bun rules were the only ones still listing every field, so they broke; they now spread RtkRule::DEFAULT like their neighbours. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Final pass — CONTRIBUTING / CODING_PRACTICES + fuzz testingThanks for addressing the previous review — public fns, Reviewed head Blockers1. Shared infrastructure not used. 2. Bare 3. Must fix
Requesting changes. Blocker 1 gates this on its own. Happy to re-run the same fuzz set on the next revision. |
The build and install fixtures were hand-written and never matched what bun prints: the build filter's regex expected "dist/app.js 45.2KB" while bun emits "index.js 72 bytes (entry point)". Every bun fixture is now a real capture from bun 1.3.14, adding the cases the filter had no coverage for: todo/skip/snapshot counts, --bail, --coverage, a SIGKILLed run and a 40-failure run for the cap.
Every run now goes through run_filtered_with_exit with a tee label, so nothing is dropped without a way back to the full output. Failures are grouped into blocks and capped at CAP_WARNINGS, with the remainder behind a tee hint; install and build cap their lists the same way. An empty filter result no longer prints "ok" when the command failed: a SIGKILLed test run (exit 137) prints only the banner, and reporting success there hid the crash. finish() reports the exit code instead, for all four filters. Context lines are kept verbatim. Trimming moved the caret that points at a column in expect() context and flattened toEqual diffs. The summary parser also lost "1 todo", "snapshots: +1 added", the "N snapshots, N expect() calls" variant and "Bailed out after N failure", and the --coverage table was dropped despite being explicitly requested. bun run only drops the single "$ cmd" line bun echoes; a script printing its own $-prefixed lines keeps them.
Real `bun run` lists the package scripts and bare `bunx` prints its usage. Both were turned into rtk errors instead, so a valid command failed. They now pass through, the way `rtk npm run` does.
|
All eight are in, pushed as Blocker 1. Every run goes through Blocker 2. Bare Blocker 3. 4. Replaced every bun fixture with a real capture from 1.3.14, not just build and install. The build regex now matches 5. The summary parser picks up 6. Context lines are kept verbatim. There is a test that measures the caret indentation, so a future trim will not slip back in. 7. Only the single 8. The coverage table survives. While I was in there I answered the locale question with actual data: on 1.3.14 the output of Note dropped on #2418. |
|
Thanks, re-ran the same fuzz set on One must-fix left, #7: the "drop first Approve once that's in. |
Dropping the first $-prefixed line was the wrong tool: bun writes the `$ cmd` echo to stderr while the script writes to stdout, and stdout lands first in the capture, so the heuristic ate a line of the script's own output (verified 8/8 with a script printing "$HOME is not set"). `bun run --silent` suppresses the echo and leaves the script output identical, so the filter no longer inspects $-prefixed lines at all. The flag has to precede the script name or bun forwards it to the script, which run_args now guarantees and a test pins.
|
Good catch on the ordering, that is exactly it. Reproduced before touching anything: bun writes the echo to stderr and the script to stdout, stdout lands first in the capture, and Fixed in Verified 8/8 with a script printing |
|
All blockers and must-fixes resolved. LGTM. @KuSh still pending changes |
KuSh
left a comment
There was a problem hiding this comment.
Another pass over the current head. Most of the earlier threads are genuinely addressed — the bun x alias, bun tsc / bun run tsc, the bun x special case in main, c.arg("--").arg(tool), the finish() consolidation, trim-upfront, the public test/install/build/run fns, LC_ALL=C and the Cargo.lock revert all check out. I ran the rewrite engine over the bun command forms and all four spellings (bun X, bun run X, bunx X, bun x X) resolve to the right specialized filter, so the jest/vitest/prisma/tsc threads are correctly closed.
Two things below are blocking (the biome arm never runs biome, and is_coverage_row eats failure context on any line containing ||). The rest are smaller.
Two more that I can't attach inline because the files aren't in the diff:
src/core/tracking.rs,categorize_command— not extended with"bun" | "bunx", so everyrtk bun …/rtk bunx …invocation is bucketed as"other"instead of"js"in telemetry andrtk gain.src/core/toml_filter.rs,RUST_HANDLED_COMMANDS— not extended withbun/bunx. A project TOML filter whosematch_commandmatchesbunwill compile with no "already handled in Rust" shadow warning and then silently never fire, since clap now routesrtk bunfirst.
| } | ||
| } | ||
|
|
||
| fn route_bunx(args: &[String], verbose: u8) -> Result<i32> { |
There was a problem hiding this comment.
Re-raising the factorization point — my earlier comment on this is now collapsed as outdated, and the duplication has since drifted.
route_bunx and the Commands::Npx arm (~line 2425) are the same tool→filter table, and they no longer agree:
route_bunx |
Commands::Npx |
|
|---|---|---|
| jest / vitest / biome | present | missing |
| prisma fallthrough | bun_cmd::run_tool |
~30 lines of inlined passthrough |
| final fallback | bun_cmd::run_tool |
npm_cmd::exec |
The drift is the point: the missing arms are exactly what I flagged earlier, and the &args[1..] slicing bug on the eslint/biome arm below exists identically in the npx copy — one bug, copied.
Something like route_js_tool(args, verbose, fallback) with npx passing npm_cmd::exec and bunx passing bun_cmd::run_tool fixes both call sites at once and stops the next tool from being registered in only one of them. The npx prisma branch should call that fallback too rather than hand-rolling its own passthrough.
| }; | ||
| match tool { | ||
| "tsc" | "typescript" => tsc_cmd::run(&args[1..], verbose), | ||
| "eslint" | "biome" => lint_cmd::run(&args[1..], verbose), |
There was a problem hiding this comment.
This strips the linter name, but lint_cmd::run expects it as args[0] — detect_linter reads args[0], and rtk lint biome is the documented form.
So rtk bunx biome check . calls lint_cmd::run(["check", "."]), detect_linter treats check as the linter, and rtk shells out to bunx -- check .. Verified end-to-end: bunx answers could not determine executable to run for package check, and rtk then reports Lint: 1 errors, 0 warnings. biome never runs and the failure is masked.
| "eslint" | "biome" => lint_cmd::run(&args[1..], verbose), | |
| "eslint" | "biome" => lint_cmd::run(args, verbose), |
Note bunx eslint src misroutes the same way (bunx -- src), mirroring the same slicing in the Commands::Npx arm — see the factorization thread.
| /// A row of the `--coverage` table. Every row (header, separator and per-file) | ||
| /// carries the three column separators, which ordinary test output does not. | ||
| fn is_coverage_row(line: &str) -> bool { | ||
| line.matches('|').count() >= 3 |
There was a problem hiding this comment.
This misfires on bun's own source frames. bun prints failure context as NN | <source>, so any frame whose source contains || (two pipes plus the gutter pipe = 3) or a TS union type is classified as a --coverage row: buffer.clear() throws away every preceding context line of that failure, and the line is then re-emitted after the FAILURES block.
Reproduced with a test failing on const name = user.name || user.nick || "anon"; — frame lines 10-13 were dropped, the failure block started at line 14, and line 13 reappeared under the coverage section. || is ubiquitous in JS, so this fires often.
The coverage table is a contiguous block with a ---|--- separator and fixed headers; keying on that (or on being inside a detected coverage section) would be much safer than a bare pipe count.
| BUN_TEST_COUNT_RE.is_match(line) | ||
| || BUN_TEST_EXPECT_RE.is_match(line) | ||
| || BUN_SNAPSHOT_RE.is_match(line) | ||
| || line.starts_with("Ran ") |
There was a problem hiding this comment.
BUN_TEST_COUNT_RE was deliberately anchored with $ so app output isn't read as the N pass summary, but this prefix check (and "Bailed out after " below) is still open.
Reproduced: a test logging Ran 5 migrations before assert before failing had that line hoisted into the summary section, and buffer.clear() then dropped the failure's code frame and caret. Worth anchoring the same way, e.g. ^Ran \d+ tests?\b.
| continue; | ||
| } | ||
| // Keep errors and warnings | ||
| if trimmed.contains("error") |
There was a problem hiding this comment.
A build failure loses its location. Only lines containing error/warn/Error/Warn, artifact lines and summary lines survive; bun's code frame, the caret, and the at /path/file.ts:1:15 line match none of those and are dropped.
Verified: frame + caret + error: Could not resolve: "missing" + at /home/u/p/index.ts:1:15 filters down to just the error: line. The file and line number are exactly what the LLM needs to act, so keeping the at <path>:<line>:<col> line (and ideally the frame) would be worth the tokens.
| for line in clean.lines() { | ||
| let trimmed = line.trim(); | ||
|
|
||
| if trimmed.is_empty() { |
There was a problem hiding this comment.
This drops all blank lines, which contradicts the reasoning used to keep line rather than trimmed a few lines down ("this is passthrough of arbitrary output where indentation is content"). If indentation is content here, so are the paragraph breaks.
Section A\n\n item 1\n item 2\n\nSection B collapses into four consecutive lines, destroying the structure of any report or diff a script prints.
| ..RtkRule::DEFAULT | ||
| }, | ||
| RtkRule { | ||
| pattern: r"^bun\s+(test|run|install|build|add|remove)", |
There was a problem hiding this comment.
Missing the terminator that every sibling rule has (^npm\s+(exec|run|…)(\s|$), ^(bun\s+x|bunx)(\s|$), ^ls(\s|$)), so this matches on a prefix.
I ran rewrite_command over a few forms and these all rewrite:
bun build.ts -> Some("rtk bun build.ts")
bun test.ts -> Some("rtk bun test.ts")
bun install.ts -> Some("rtk bun install.ts")
bun removeall -> Some("rtk bun removeall")
bun <file.ts> is a normal invocation since bun executes files directly, so bun build.ts isn't contrived. Execution stays correct (those land in BunCommands::Other → run_passthrough), but the hook rewrites commands that gain nothing and discover reports 80% savings for them.
| pattern: r"^bun\s+(test|run|install|build|add|remove)", | |
| pattern: r"^bun\s+(test|run|install|build|add|remove)(\s|$)", |
| rtk_cmd: "rtk bun", | ||
| rewrite_prefixes: &["bun"], | ||
| category: "PackageManager", | ||
| savings_pct: 80.0, |
There was a problem hiding this comment.
Separately from the anchoring: savings_pct: 80.0 over-reports for part of this rule's own alternation. add and remove land in BunCommands::Other → run_passthrough (0% reduction), and bun run <script> is near-passthrough. bun add zod currently classifies as 80% estimated savings, so rtk discover will over-promise. The subcmd_savings list already carries the real numbers for test/install/build — the base rate is the one that's off.
| ### Package Managers | ||
| ```bash | ||
| rtk pnpm list # Compact dependency tree | ||
| rtk uv run pytest # Preserve uv env, errors only |
There was a problem hiding this comment.
This duplicates the pre-existing line 228 (rtk uv run pytest # Preserve uv env, keep program output) with a contradictory description, three lines apart. Looks like a rebase artifact from the pre-0.37.0 base — probably wants dropping.
|
Thanks for this — solid work, especially the fixture coverage. Closing in favour of #988, which is further along in review and covers both runtimes (bun and deno) rather than bun alone. Keeping two overlapping bun implementations in flight isn't serving anyone, and #988 is the one about to land. That said, this PR has things #988 doesn't: |
|
Fair call, no argument from me. Two overlapping implementations is worse than one. I'll open the follow-up once #988 lands: One thing from your last pass is worth rescuing, since it outlives this PR: the |
|
Thanks for the reminder, @HenriqueSchroeder. I’ve opened #3812 for that. I’ll ping you here if and when #988 is merged so we can follow up. Thanks again! |
Summary
Adds Bun runtime support to RTK with filters for
bun test,bun run,bun install,bun build, andbunx.What's new
bun testfilter: strips passing tests, keeps failures with error context, compact summary. Handles both TTY (✓/✗) and piped ((pass)/(fail)) formats. Buffers lines preceding a fail marker since Bun prints error context before the marker, then flushes them on match.bun installfilter: strips progress bars, keeps summary.bun buildfilter: compact build output.bun run <script>filter: passes through script output with error-focused filtering.bunxpassthrough with intelligent routing (tsc, eslint, prisma → specialized filters).bunx tsc,bunx eslint, etc. get rewritten to specialized RTK filters.Token savings
All filters verified
>= 60%savings against realbun testoutput (seetests/fixtures/bun_test_real.txt, 140 lines of real bun output with 79 pass / 2 fail).Tests
bun_test_real.txt)bun test/run/install/buildsubcommands andbunxTest plan
cargo test— 1372 tests pass locallycargo clippy --all-targets— no new warnings inbun_cmd.rscargo fmt --all --check— cleanrtk bun testin a Bun project verifies end-to-end filteringNotes
Branch is based on an older point in history (pre-0.37.0). Merge conflicts with current
develop(notablysrc/discover/rules.rswhere Bun entries need to be combined with the expandednpx/pnpm execpatterns, andsrc/main.rsPrismaCommands derive) are expected and will need resolution before merge.🤖 Generated with Claude Code