build: pass --locked to cargo for vendored Rust deps#29095
Conversation
emitCargo() now passes --locked so cargo refuses to re-resolve transitive crates if the dep's shipped Cargo.lock is missing or out of sync with its Cargo.toml. lolhtml (the only cargo dep today) ships c-api/Cargo.lock in its tarball, so this is a no-op for the current pin — it just turns silent re-resolution (e.g. on a yanked crate) into a build error, keeping the set of crates linked into liblolhtml.a deterministic across builds. :house: Remote-Dev: homespace
|
Updated 10:49 AM PT - Apr 10th, 2026
❌ @ant-kurt, your commit 2c9ef58 has 5 failures in
🧪 To try this PR locally: bunx bun-pr 29095That installs a local version of the PR into your bun-29095 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughWhitespace/formatting change in Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — simple, well-reasoned one-liner that improves build reproducibility.
Extended reasoning...
Overview
Single-file change to scripts/build/source.ts: adds --locked to the cargo build args array in emitCargo(), plus a JSDoc comment on CargoBuild formalizing the contract that the dep's tarball must ship a Cargo.lock.
Security risks
None. This is a build-script change that makes dependency resolution more deterministic. It reduces supply-chain risk (prevents silent re-resolution against crates.io when crates are yanked) rather than introducing any.
Level of scrutiny
Low. The change is a single element prepended to an args array in a build orchestration script. The --locked flag is a standard cargo practice for reproducible builds. lolhtml (the only cargo dep) already ships c-api/Cargo.lock at the pinned commit, so CI should confirm this is a no-op today.
Other factors
No bugs found. No existing reviewer comments outstanding. The PR description is thorough and the author verified the upstream lockfile exists. CI is running (#44796).
There was a problem hiding this comment.
LGTM — adding --locked to cargo is the right call for reproducible builds, and the only cargo dep (lolhtml) already ships c-api/Cargo.lock.
Extended reasoning...
Overview
The PR touches two build script files: a cosmetic line-break reformat in config.ts and a one-line functional change in source.ts (inserting "--locked" into the cargo args array in emitCargo()), plus a new JSDoc block on the CargoBuild interface.
Security Risks
None. This is a build-script hardening change that restricts what cargo is allowed to resolve — it's additive safety, not a relaxation of any constraint.
Level of Scrutiny
Low. The functional diff is a single string prepended to an args array. The logic is: without --locked, a yanked crate or drifted lockfile silently re-resolves against crates.io; with --locked, that fails loudly. This is standard practice for vendored Rust deps and is well-documented in the PR description.
Other Factors
- The only bug found is a documentation nit: the JSDoc says "Cargo.lock in
manifestDir" when it should saymanifestDirmust be the workspace root. This is already noted as an inline comment and is genuinely minor — the current code is correct for lolhtml, and any violation fails at build time with a clear cargo error. - CI failures visible in the timeline are on unrelated files (
scripts/build/ci.ts,vendor/libuv/src/unix/udp.c) and appear to be pre-existing platform issues, not caused by this change. - CodeRabbit found no actionable comments.
The 2.7.2 release commit (e3aa547) shipped a c-api/Cargo.lock that still pinned lol_html at 2.7.1, so cargo build --locked rejects it. 77127cd is the immediately-following upstream commit that regenerated the lockfile (only c-api/Cargo.lock changes between the two). Same lol-html source, now passes --locked. :house: Remote-Dev: homespace
🏠 Remote-Dev: homespace
🏠 Remote-Dev: homespace
🏠 Remote-Dev: homespace
There was a problem hiding this comment.
LGTM — my previous JSDoc nit was addressed by dropping the comments.
Extended reasoning...
Overview
Three files touched: cosmetic line-break in config.ts, commit hash bump in lolhtml.ts, and --locked added to cargo args in source.ts.
Security risks
None. This is a build-hardening change that makes dependency resolution more deterministic, not less.
Level of scrutiny
Low. The core functional change is a single flag addition to a well-understood cargo invocation. The lolhtml bump is a no-op semantically (same lol-html 2.7.2 source, only c-api/Cargo.lock regenerated upstream). My earlier concern about imprecise JSDoc was addressed by removing the comments outright.
Other factors
No bugs found by the automated system. Previous inline comment resolved. Straightforward build-system hardening.
🏠 Remote-Dev: homespace
There was a problem hiding this comment.
LGTM — the JSDoc concern from my previous review was addressed by removing the imprecise comment, the CI failures are fixed, and the changes are correct.
Extended reasoning...
Overview
The PR adds --locked to cargo build invocations in emitCargo() (scripts/build/source.ts) to prevent silent re-resolution of transitive Rust crates. It bumps LOLHTML_COMMIT from e3aa547 to 77127cd because the old pin had a stale c-api/Cargo.lock (upstream omitted it for 2.7.2); the new pin is the upstream fix commit with only the lockfile regenerated. The process.versions test is updated to match the new hash, and there is one trivial whitespace formatting change in config.ts.
Security risks
None. This is a build-system change that makes dependency resolution stricter (fail-fast if lockfile would change), which is a security improvement, not a regression.
Level of scrutiny
Low. The change is mechanical: one flag added, one commit hash bumped, one test hash updated. The logic is straightforward and self-consistent. The only concern from my prior review (a slightly imprecise JSDoc comment about where Cargo.lock must live) was resolved by removing that comment entirely in commit 9ff2d8f.
Other factors
My prior inline comment is marked resolved. The CI failures were caused by the test hash not being updated alongside the commit bump; that was fixed in commit 2c9ef58. No outstanding reviewer comments remain unaddressed.
3 participants
Pass
--lockedtocargo buildinemitCargo()so transitive Rust crate versions can't silently re-resolve against crates.io.Bumps
LOLHTML_COMMITone commit forward (e3aa547→77127cd) because the old pin'sc-api/Cargo.lockwas stale (upstream forgot to regenerate it for 2.7.2). The new pin is the upstream lockfile fix; onlyc-api/Cargo.lockdiffers between the two — same lol-html 2.7.2 source.