Conversation
|
Warning Review limit reached
Next review available in: 8 minutes Limit details: You’ve used all 5 included reviews currently available. 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 within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — restores a dependency edge that a merge race dropped; main doesn't compile without it.
What was reviewed:
- Confirmed
bun_css::{ParserOptions, StyleSheet, PrinterOptions}are live atsrc/runtime/cli/pm_diff_normalize.rs:747-764. - Confirmed
bun_cssis a workspace member atCargo.toml:377, so.workspace = trueresolves. Cargo.lockaddition is the single expected line in thebun_runtimedeps list, alphabetically placed.
Extended reasoning...
Overview
Two-line build-manifest fix: adds bun_css.workspace = true back to src/runtime/Cargo.toml and the corresponding entry to Cargo.lock under bun_runtime. The PR description explains the merge race clearly: #37301 pruned the edge as unused, then #39229 landed code that uses it.
Security risks
None. This adds a dependency edge to an already-in-workspace, in-tree crate (src/css). No new external crates, no version changes, no code changes.
Level of scrutiny
Minimal. This is a mechanical build repair — the compiler error is deterministic and the fix is the only possible one. I verified the live call sites in pm_diff_normalize.rs and that bun_css is declared in the workspace root, so .workspace = true is the correct spelling. The lockfile hunk adds only the one expected line.
Other factors
No test is expected per REVIEW.md conventions — the build itself is the check for a manifest repair. No prior review comments to address. The bug hunting system found nothing.
| // use bun_core::output as bun_output; (module alias) | ||
| // pub mod bun_spawn { ... } (local module shadows the crate) | ||
| // Any such binding anywhere in a crate takes that name out of this lint for | ||
| // the whole crate; rustc resolves those for real. |
There was a problem hiding this comment.
🔴 The as\s+(bun_[a-z0-9_]+)\b arm matches cast/UFCS expressions like s.st_mode as bun_sys::Mode and <HTMLBundle as bun_jsc::JsClass> because \b holds between a word char and :, so those crate names land in shadowed and are skipped entirely at line 151. In bun_runtime alone this exempts bun_sys, bun_jsc, bun_core, bun_bundler, bun_ptr, bun_uws, bun_watcher, and bun_options_types from the check — add (?!\s*::) after the capture, mirroring the mod arm.
Extended reasoning...
What the bug is
LOCAL_BINDING has two arms: mod\s+(bun_…)\b(?!\s*::) and as\s+(bun_…)\b. The as arm is meant to recognise extern crate x as bun_y and use x as bun_y renames so the lint doesn't flag a name that a crate has bound locally. But a \b assertion is satisfied between a word character and :, so the arm also matches Rust cast expressions and UFCS trait paths — expr as bun_sys::Mode, <T as bun_jsc::JsClass>::from_js(…). Each match adds the captured crate name to the whole-crate shadowed set, and the check loop then skips that name unconditionally: if (shadowed.has(used)) continue;.
Step-by-step proof
Running the actual regex against real source lines:
const re = /\b(?:mod\s+(bun_[a-z0-9_]+)\b(?!\s*::)|as\s+(bun_[a-z0-9_]+)\b)/g;
[..."s.st_mode as bun_sys::Mode".matchAll(re)] // → m[2] === 'bun_sys'
[..."<HTMLBundle as bun_jsc::JsClass>::from_js(x)".matchAll(re)] // → m[2] === 'bun_jsc'Both capture the crate name into m[2], which lands in entry.shadowed at line 137. Then in the violation loop, line 151 (if (shadowed.has(used)) continue;) skips every PATH_USE hit for that name across the entire crate — not just the file the cast appeared in.
grep -rhoP '\bas\s+bun_[a-z0-9_]+::' src/runtime/ --include='*.rs' | sort -u shows the crates spuriously shadowed in bun_runtime: bun_bundler, bun_core, bun_jsc, bun_options_types, bun_ptr, bun_sys, bun_uws, bun_watcher. Live examples: src/runtime/server/DirectoryRoute.rs (s.st_mode as bun_sys::Mode), src/runtime/server/server_body.rs (<HTMLBundle as bun_jsc::JsClass>), src/runtime/server/StaticRoute.rs (<Self as bun_ptr::CellRefCounted>::deref), src/runtime/jsc_hooks.rs (<RuntimeTranspilerCache as bun_bundler::RuntimeTranspilerCacheExt>). The same pattern hits other crates too — bun_crash_handler shadows bun_zlib, bun_libarchive shadows bun_sys, bun_install shadows bun_spawn, etc.
Why existing code doesn't prevent it
The mod arm already guards against exactly this shape with (?!\s*::) — so mod bun_x::y (impossible in Rust anyway) wouldn't be captured. The as arm asymmetrically lacks the same guard. Nothing else in the pipeline distinguishes a rename-binding from a cast: the match goes straight into shadowed, and shadowed is consulted before violations.push. Notably, x as bun_sys::Mode is simultaneously a PATH_USE hit (requiring the dep) and a LOCAL_BINDING hit (suppressing the check) on the same token — the very expression that needs the edge is the one that hides its absence.
Impact
If any of the eight edges above were dropped from src/runtime/Cargo.toml (exactly the failure class this lint exists to catch — a dead-code sweep racing a new use), the lint would stay green. That is a silent false negative on the most heavily-used workspace crates. The manifest fix in this PR happens to be for bun_css, which is not in the shadowed set, so the PR's own verification ("the lint fails on main without the manifest change") passed — but the lint would not have caught the same race for bun_sys, bun_jsc, or bun_core.
Fix
Add the same negative lookahead the mod arm uses:
const LOCAL_BINDING = /\b(?:mod\s+(bun_[a-z0-9_]+)\b(?!\s*::)|as\s+(bun_[a-z0-9_]+)\b(?!\s*::))/g;This still matches use x as bun_y; and extern crate x as bun_y; (a ; or whitespace follows the identifier), and rejects … as bun_y::Item. Alternatively, anchor the arm to (?:use|extern\s+crate)\b[^;]*\bas\s+…, but the lookahead is the smaller change and symmetric with the existing arm.
| const mountedElsewhere: Record<string, string> = { | ||
| "src/jsc/generated_classes_list.rs": "bun_runtime", // src/runtime/lib.rs `#[path] pub mod generated_classes_list` | ||
| }; |
There was a problem hiding this comment.
🟡 mountedElsewhere covers src/jsc/generated_classes_list.rs but not src/bun.js.rs, which is #[path]-mounted six lines above it in src/runtime/lib.rs:49. That file lives directly under src/, so crateFor("src/bun.js.rs") returns undefined and its five bun_*:: references (bun_core, bun_jsc, bun_standalone_graph, bun_bundler, bun_options_types) are silently skipped. Add "src/bun.js.rs": "bun_runtime" to close the gap.
Extended reasoning...
What the gap is
src/runtime/lib.rs mounts two out-of-tree files into bun_runtime via #[path]:
#[path = "../bun.js.rs"]
pub mod run_main; // line 49
...
#[path = "../jsc/generated_classes_list.rs"]
pub mod generated_classes_list; // line 55mountedElsewhere maps only the second one. The first — src/bun.js.rs — is the sole tracked .rs file directly under src/ (verified via git ls-tree) and it uses bun_core::, bun_jsc::, bun_standalone_graph::, bun_bundler::, and bun_options_types::.
Why it falls through crateFor
Walk crateFor("src/bun.js.rs"):
nestedCrateDirs.some(d => rel.startsWith(d))→ false (onlysrc/install/windows-shim/).mountedElsewhere["src/bun.js.rs"]→ undefined.crates.find(c => rel.startsWith(c.dir + "/"))→ every cratedirissrc/<name>, so the test is"src/bun.js.rs".startsWith("src/<name>/"). The file has no directory component aftersrc/, so no crate matches. Returnsundefined.
The main scan then hits if (!crate) continue; and never puts the file into stripped or byCrate. Its five workspace-crate references are never checked against src/runtime/Cargo.toml.
Why the existing entry doesn't generalize
generated_classes_list.rs needed a mountedElsewhere entry because without it the file would be mis-attributed to bun_jsc (it lives under src/jsc/), producing false positives. bun.js.rs without an entry produces the mirror failure — a silent skip (false negative). Both are #[path] mounts declared in the same block of lib.rs; the PR description explicitly says the lint "understands ... files mounted into another crate with #[path]", so per the review rule "Fix the whole class in the same PR ... grep for every sibling site sharing the pattern" the sibling six lines away should be listed too.
Impact
Nothing breaks today — all five referenced crates are currently in src/runtime/Cargo.toml. But the whole point of this lint is to catch a future dead-edge sweep that drops one of them while src/bun.js.rs still uses it (the exact bun_css failure mode this PR is fixing). As written, that scenario for src/bun.js.rs would sail through the lint.
Fix
One-line addition:
const mountedElsewhere: Record<string, string> = {
"src/bun.js.rs": "bun_runtime", // src/runtime/lib.rs `#[path] pub mod run_main`
"src/jsc/generated_classes_list.rs": "bun_runtime",
};| // Every `bun_*::` path a crate's code names must have a matching dependency | ||
| // edge in that crate's Cargo.toml. cargo enforces this at compile time, but | ||
| // only for the crates a given change rebuilds — a dead-code sweep that prunes | ||
| // a "textually unreferenced" edge can race a PR that adds new uses of it, and | ||
| // both land green (#37301 removed bun_runtime's `bun_css` edge after being | ||
| // verified against a tree that predated #39229's `bun_css::StyleSheet` calls | ||
| // in pm_diff_normalize.rs; merged main then failed to build everywhere). | ||
| // | ||
| // This lint re-checks the pairing textually on every tree, so whichever PR | ||
| // merges second goes red on its own CI instead of breaking main. |
There was a problem hiding this comment.
🟡 This lint reads src/*/Cargo.toml, but .github/workflows/source-lints.yml's paths: filters (both push and pull_request) don't include src/**/Cargo.toml — so a dead-edge sweep PR that only edits a Cargo.toml (exactly the #37301 shape this lint targets) won't trigger the workflow at PR-check time. Per test/internal/source-lints/README.md, lints that read files outside the listed paths need them added; add "src/**/Cargo.toml" to both paths: lists. The bare merge_group: trigger backstops main, so this only costs PR-time feedback.
Extended reasoning...
What the bug is
test/internal/source-lints/README.md:19-22 documents an explicit contract for adding lints to this directory:
The workflow only triggers for the
paths:listed in it. A lint that reads files outside those paths (for examplepackages/bun-types/bun.d.tsor the docs) needs them added there, or a change to those files is only checked by whichever later PR happens to trigger the workflow.
The new cargo-dep-edges.test.ts reads src/*/Cargo.toml (line 33: new Bun.Glob("src/*/Cargo.toml").scanSync), but .github/workflows/source-lints.yml lines 14-33 (push) and 35-54 (pull_request) list src/**/*.rs, src/**/*.classes.ts, src/jsc/bindings/**, etc. — no Cargo.toml pattern. This PR's changed-files list also doesn't include source-lints.yml, so the pattern isn't being added.
Why it matters for this specific lint
The lint's own header comment (lines 5-14) states its purpose is to catch a race between two PR classes: one that adds a bun_*:: reference and one that prunes the corresponding dependency edge from a Cargo.toml. The PR description says it should catch "whichever PR merges second."
Consider the two orderings:
- The .rs-editing PR merges second — it touches
src/**/*.rs, which is in the paths filter → workflow runs on that PR ✓ - The Cargo.toml-editing PR merges second — this is exactly what Remove dead code from bun_ast, bun_runtime, and unused Cargo dependency edges #37301 was: a PR whose only functional change was removing
bun_css.workspace = truefromsrc/runtime/Cargo.toml. Such a PR touches onlysrc/*/Cargo.toml+ rootCargo.lock, neither of which matches any pattern in thepull_requestpaths filter → the source-lints workflow does not run on that PR's own CI, and the author gets no PR-time signal from this lint.
So one of the two race directions the lint is designed to catch doesn't get PR-level feedback.
Why this is a nit, not blocking
Two backstops mean main won't actually break:
merge_group:(line 55) has nopaths:filter, so every PR entering the merge queue runs source-lints against the merged tree regardless of what files it touches. The Cargo.toml-only PR would fail there.rust-lints.ymldoes listsrc/**/Cargo.tomlin its own paths and runscargo clippyat merge-queue time, which would catch the missing edge as a compile error on the merged tree independently.
So the only consequence of merging as-is is that the failure surfaces at merge-queue time instead of at PR-check time for Cargo.toml-only PRs. That's a degraded feedback loop, not a concrete failure — hence nit rather than blocking.
Step-by-step proof
- Author opens a PR that only removes
bun_foo.workspace = truefromsrc/bar/Cargo.toml(a dead-edge sweep, verified against a stale tree). - GitHub evaluates the
pull_requestpaths filter insource-lints.yml:35-54:src/bar/Cargo.tomlmatches none ofsrc/**/*.rs,src/**/*.classes.ts,src/codegen/class-definitions.ts,src/js/builtins.d.ts,src/jsc/bindings/**, or any other listed pattern. RootCargo.lockmatches nothing either. - The
source-lintscheck does not appear on the PR. Per README line 6, these tests are also excluded from the Buildkite shards, so no other lane runscargo-dep-edges.test.tsagainst this PR either. - The author sees green PR checks and adds the PR to the merge queue.
- Only then does the
merge_group:trigger run source-lints against the merged tree and fail — later than intended.
Fix
Add "src/**/Cargo.toml" to both the push: paths list (after line 15) and the pull_request: paths list (after line 36) in .github/workflows/source-lints.yml.
2 participants
Problem
bun bdfails on main:error[E0433]: cannot find module or crate bun_cssinsrc/runtime/cli/pm_diff_normalize.rs:747while compilingbun_runtime.bun_css.workspace = truefromsrc/runtime/Cargo.tomlas an unused edge. That was verified before Addbun pm diff#39229 (bun pm diff) merged. Addbun pm diff#39229 added calls tobun_css::StyleSheet::parseandbun_css::PrinterOptionsinpm_diff_normalize.rs:747-764. The two PRs merged in an order that left a live reference without the dependency edge.Fix
bun_css.workspace = trueinsrc/runtime/Cargo.toml, plus the matching one-lineCargo.lockentry.test/internal/source-lints/cargo-dep-edges.test.ts. It checks that everybun_*::path a crate's code names has a dependency edge in that crate's Cargo.toml. It catches this failure class on the PR that merges second, before main breaks.bun_cssedge) and passes with it.cargo check -p bun_runtimeand a fullbun bdalso pass with the change and fail without it.Background
cargo already enforces dependency edges at compile time, but only for the code each PR rebuilds. A dead-code sweep can verify an edge is unused, then merge after another PR adds new uses of it. Both PRs are green on their own CI and the combination breaks main. The lint re-checks the pairing textually on every tree. It is conservative: it counts only workspace crate names used as
name::paths in code, after comments and string literals are stripped, and it understandsextern crate x as yrenames, local modules that shadow a crate name, and files mounted into another crate with#[path].