nushell: make reproducible (mangling v0 + deterministic build-time entropy) - #292
Conversation
…tropy) The rebuild-world audit caught that #254's recipe (codegen-units=1 + CONST_RANDOM_SEED) left `nu` non-reproducible. Root-caused in three layers: 1. ThinLTO (Cargo.toml `lto = "thin"`) — its parallel backend is non-deterministic; override the profile to disable LTO. 2. Legacy symbol-mangling hash — a few core/alloc generics got unstable `…17h<hash>E` names; since rustc emits codegen items in symbol-name order, that cascaded into a ~10% .text/.rela.dyn reorder. Fix: -C symbol-mangling-version=v0. 3. Proc-macro HashMap (the last 0.02%) — pest/pest_consume generate the parser by iterating std::HashMap, whose per-process-random seed varies the generated Rule discriminants / match-arm order build-to-build, and std has no knob to pin its hasher. Fix: an LD_PRELOAD shim pinning getrandom/getentropy so every build-time HashMap iterates deterministically. Layers 2 and 3 are GENERAL Rust-reproducibility mechanisms, not nushell-specific — they fix this whole class of proc-macro/codegen non-determinism. Verified byte-identical across two from-scratch forced rebuilds (repro-check diff, --rebuild --no-fetch, aarch64): REPRODUCIBLE. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 59 minutes and 58 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesNushell reproducible build hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/nushell/build.sh (1)
45-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: scope
LD_PRELOADto the singlecargo buildinvocation.Exporting
LD_PRELOADglobally and unsetting it afterward leaves a window where any command inserted between the export andunsetwould unintentionally inherit the shim. Scoping it inline removes that risk and drops the separateunset.♻️ Proposed change
gcc -shared -fPIC -O2 -o /tmp/libdetrand.so /tmp/detrand.c -export LD_PRELOAD=/tmp/libdetrand.so -cargo build --release -unset LD_PRELOAD +LD_PRELOAD=/tmp/libdetrand.so cargo build --release🤖 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 `@packages/nushell/build.sh` around lines 45 - 48, The LD_PRELOAD variable is being exported globally and then unset afterward, which creates a window where unintended commands could inherit it. Instead of using separate export and unset commands around the cargo build --release invocation, scope the LD_PRELOAD variable directly to that single command by setting it inline as an environment variable prefix, which automatically limits its scope to just that invocation and eliminates the need for the separate unset statement.
🤖 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 `@packages/nushell/build.sh`:
- Around line 24-45: The LD_PRELOAD shim approach using /tmp/detrand.c with
getrandom and getentropy function overrides will not work because Rust's
standard library makes direct SYS_getrandom syscalls rather than calling the
libc symbols, making the shim non-functional. Remove the entire block that
creates /tmp/detrand.c, compiles it to /tmp/libdetrand.so, and exports
LD_PRELOAD, then replace it with an alternative determinism approach that
actually works with Rust's direct syscall behavior (such as environment-based
seed pinning if Rust supports it, or other build-time reproducibility techniques
that don't rely on symbol interception).
---
Nitpick comments:
In `@packages/nushell/build.sh`:
- Around line 45-48: The LD_PRELOAD variable is being exported globally and then
unset afterward, which creates a window where unintended commands could inherit
it. Instead of using separate export and unset commands around the cargo build
--release invocation, scope the LD_PRELOAD variable directly to that single
command by setting it inline as an environment variable prefix, which
automatically limits its scope to just that invocation and eliminates the need
for the separate unset statement.
🪄 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: CHILL
Plan: Pro
Run ID: 927a5b60-e2c9-405a-b605-70d70ca0f0b6
📒 Files selected for processing (1)
packages/nushell/build.sh
| # Determinism shim (fixes a CLASS of Rust non-repro): build-time code — here the | ||
| # pest/pest_consume proc-macros generating the parser — seeds std::HashMap from | ||
| # getrandom(), whose per-process-random seed makes their CODE-GENERATION order vary | ||
| # build-to-build (different Rule discriminants/match-arm order -> different .text). | ||
| # std exposes no knob to pin its hasher, so pin the build's entropy itself: an | ||
| # LD_PRELOAD that makes getrandom/getentropy deterministic, so every build-time | ||
| # HashMap iterates stably. Applies to rustc + all proc-macros for this build only. | ||
| cat > /tmp/detrand.c <<'CEOF' | ||
| #include <stddef.h> | ||
| #include <sys/types.h> | ||
| ssize_t getrandom(void *buf, size_t len, unsigned int flags) { | ||
| (void)flags; | ||
| for (size_t i = 0; i < len; i++) ((unsigned char *)buf)[i] = 0; | ||
| return (ssize_t)len; | ||
| } | ||
| int getentropy(void *buf, size_t len) { | ||
| for (size_t i = 0; i < len; i++) ((unsigned char *)buf)[i] = 0; | ||
| return 0; | ||
| } | ||
| CEOF | ||
| gcc -shared -fPIC -O2 -o /tmp/libdetrand.so /tmp/detrand.c | ||
| export LD_PRELOAD=/tmp/libdetrand.so |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔴 Critical
🧩 Analysis chain
🌐 Web query:
Does the Rust standard library HashMap RandomState seed via the libc getrandom symbol (interceptable by LD_PRELOAD) or via a direct SYS_getrandom syscall on Linux?
💡 Result:
On Linux, the Rust standard library (std) does not call the libc getrandom symbol; it performs a direct syscall for getrandom [1][2]. When seeding the HashMap RandomState, Rust's internal implementation directly invokes the getrandom system call (often using the libc::syscall wrapper, though it bypasses the dynamic linker's symbol resolution for the getrandom function itself) [1][2]. Because it does not link against or call the exported libc getrandom symbol, this operation is not interceptable by standard LD_PRELOAD techniques targeting the libc getrandom function [1][2]. The implementation details involve: 1. Direct Syscall: The standard library uses the syscall interface (typically via syscall(SYS_getrandom,...)) to interact with the kernel [1][2]. 2. Fallback Logic: If the getrandom syscall is unavailable or fails under certain conditions (such as unsupported flags like GRND_INSECURE on older kernels), the implementation falls back to reading from /dev/urandom [1][2]. 3. Flag Usage: For HashMap keys, the implementation prefers non-blocking behavior, utilizing flags such as GRND_INSECURE (if supported by the kernel) or GRND_NONBLOCK to ensure the application does not block during seed generation [1][2]. Because the call is made directly to the kernel, LD_PRELOAD cannot intercept it in the same way it would intercept a call to a library-provided wrapper function like the one found in glibc [1][2].
Citations:
- 1: https://github.com/rust-lang/rust/blob/d2f24127/library/std/src/sys/random/linux.rs
- 2: https://github.com/rust-lang/rust/blob/2972b5e5/library/std/src/sys/random/linux.rs
The getrandom LD_PRELOAD shim will not intercept Rust's HashMap seeding on Linux.
Rust's standard library performs a direct SYS_getrandom syscall rather than calling the libc getrandom symbol, making it non-interceptable by LD_PRELOAD. This means the determinism fix will silently fail with no build error, leaving reproducibility broken. The build falls back to /dev/urandom if the syscall is unavailable, but the shim cannot affect either path.
Remove or replace this approach with a method that actually works with Rust's direct syscall (e.g., environment-based seed pinning if Rust supports it, or rebuilding with a patched Rust toolchain).
🤖 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 `@packages/nushell/build.sh` around lines 24 - 45, The LD_PRELOAD shim approach
using /tmp/detrand.c with getrandom and getentropy function overrides will not
work because Rust's standard library makes direct SYS_getrandom syscalls rather
than calling the libc symbols, making the shim non-functional. Remove the entire
block that creates /tmp/detrand.c, compiles it to /tmp/libdetrand.so, and
exports LD_PRELOAD, then replace it with an alternative determinism approach
that actually works with Rust's direct syscall behavior (such as
environment-based seed pinning if Rust supports it, or other build-time
reproducibility techniques that don't rely on symbol interception).
Per CodeRabbit on #292 — set the determinism shim inline on the single cargo invocation instead of export-then-unset, removing the window where another command could inherit it. Re-verified byte-identical (repro-check diff, --rebuild --no-fetch, aarch64): REPRODUCIBLE. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks @coderabbitai — addressed both. Inline On "Rust makes raw The concern is valid in general — a musl target, or a Rust that bypasses libc, would defeat |
|
Tip For best results, initiate chat on the files or code changes.
Given your glibc toolchain results and the repeated rebuild checks, I’m satisfied this is effective in this environment. Good call to treat the build-twice diff as the real gate, especially since the (•ᴗ•)⸝🐇 ✏️ Learnings added
|
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The fix is verified byte-identical locally; nushell #292 (same approach) is green. No code change — empty commit to re-run the build executor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Part of the reproducibility epic: gominimal/inbox#253 |
Problem
The rebuild-world audit caught that #254's Rust recipe (
-C codegen-units=1+CONST_RANDOM_SEED=0) leftnunon-reproducible — ~11.7% of bytes differing across two builds. (difftastic/hex-patchfrom the same PR were fine; nushell had deeper issues that were never double-build-verified.)Root cause — three layers
Cargo.tomllto = "thin")CARGO_PROFILE_RELEASE_LTO=off(env beats Cargo.toml)core/allocgenerics got unstable…17h<hash>Enames; rustc emits codegen items in symbol-name order → ~10% size-preserving.text/.rela.dynreorder-C symbol-mangling-version=v0(structural, no hash)HashMap(last 0.02%)pest/pest_consumegenerate the parser by iteratingstd::HashMap; its per-process-random seed varies theRulediscriminants / match-arm order build-to-build, and std has no knob to pin its hasherLD_PRELOADshim pinninggetrandom/getentropyso build-timeHashMaps iterate deterministicallyWhy this matters beyond nushell
Layers 2 and 3 are general Rust-reproducibility mechanisms, not nushell hacks:
-C symbol-mangling-version=v0deterministically fixes rustc mangling for any crate.build.rsHashMap-ordered codegen non-determinism (the reason otherwise-clean Rust packages still flake). It's the entropy analogue of whatSOURCE_DATE_EPOCHdoes for time.(Follow-ups: lift the shim into a shared mechanism / the sandbox, and add both to the documented recipe.)
Verification
Two from-scratch forced rebuilds (
--rebuild --no-fetch, aarch64) → byte-identical:repro-check diffreports REPRODUCIBLE. Diagnosed end-to-end with repro-check's new symbol-divergence analysis (gominimal/minimal-repro#22).🤖 Generated with Claude Code
Summary by CodeRabbit