Conversation
With from_libuv the stored errno is the negated UV code. The ErrName impl passed it through as-is, so the label table saw UV_ENOENT and the description was dropped. Resolve once and pass the SystemErrno discriminant.
|
Warning Review limit reached
On-demand reviews are free for the next 23 days. After that, they cost $0.25 per reviewed file. Or wait 4 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
|
Updated 2:11 PM PT - Aug 28th, 2026
❌ @robobun, your commit 8271b19 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 40852That installs a local version of the PR into your bun-40852 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — thanks for switching the test helper to return { stdout, stderr, exitCode } and asserting the combined object.
What was reviewed:
as_sys_err_info()now resolves viaget_error_code_tag_name()and passes theSystemErrnodiscriminant; theNonefallback preserves the old raw-errno behavior, andname()is behavior-identical after theUNKNOWN_TAG_NAMEdedup.- The test-only host fn: argument validation,
u16::try_frombounds, non-Windows early return whenfrom_libuvis true, and#[cfg]gating sofrom_libuvis used on both platforms without warnings. - New tests follow harness conventions (spawn with
-e,bunEnv/bunExe,test.concurrent,Promise.allpipe drain, combined-object assertion, Windows-only caseskipIf-gated).
Extended reasoning...
Overview
This PR fixes a latent inconsistency in bun_sys::Error::as_sys_err_info(): on Windows, when an error was constructed with from_libuv=true, the raw UV_E* magnitude (e.g. 4058) was passed as SysErrInfo.errno to bun_core::output::err, so the coreutils label lookup missed and the description ("No such file or directory") was dropped from the printed line. The fix resolves once through get_error_code_tag_name() — the same libuv-aware path name() already used — and passes the resulting SystemErrno discriminant instead. A UNKNOWN_TAG_NAME constant deduplicates the fallback literal between name() and as_sys_err_info(). A test-only host function sys_error_output_err(errno, fromLibuv) is added to make this observable, wired through internal-for-testing.ts and dispatch_js2native.rs, and two subprocess-spawning tests are added to the existing error-name-from-libuv.test.ts.
Security risks
None. The change is confined to error-message formatting (which errno discriminant is used to look up a static description string) and a debug-only test hook gated behind bun:internal-for-testing. No user-controlled input reaches new code paths outside tests; no auth, crypto, filesystem, or network surface is touched.
Level of scrutiny
Low-to-moderate. The core change is ~5 lines in Error.rs and is a straightforward "use the already-resolved value instead of the raw stored one" fix. The None arm falls back to i32::from(self.errno), matching prior behavior for the unresolvable case. On non-Windows and for non-from_libuv Windows errors, the resolved discriminant equals the stored errno, so output is unchanged there. The test hook validates argument types and range (u16::try_from), returns early on non-Windows when from_libuv is requested, and the #[cfg(windows)] on the struct field plus the #[cfg(not(windows))] early-return together ensure from_libuv is used on every platform (no unused-variable warning). No CODEOWNERS entries cover the changed paths.
Other factors
My prior inline nit — that the test helper asserted exitCode before returning stderr, hiding the diagnostic on child crash — was addressed in commit 12d97ae: outputErr now returns { stdout, stderr, exitCode } and each test asserts the whole object with .toEqual, per the REVIEW.md convention. Subsequent commits only shortened doc comments. Tests follow the harness patterns from CLAUDE.md (bunExe/bunEnv, -e spawn, test.concurrent for independent subprocesses, Promise.all to drain both pipes, skipIf(!isWindows) for the platform-specific case). The Windows-only assertion relies on CI for verification, which is the normal path for platform-gated tests in this repo.
|
CI status at 8271b19: every Windows x64 lane passed, which is where the new from_libuv test runs. The red lanes are unrelated to this change and fail on main too:
The diff only touches error formatting in bun_sys::Error, a test-only hook, and the test file. |
|
Superseded by #40860. It removed the Closing. |
2 participants
Problem
Output::err(sys_error, ...)for an error built withfrom_libuvprintsENOENT: <msg> (open)and drops the description (No such file or directory).ErrNameimpl forbun_sys::Error(src/sys/Error.rs:524) resolves the tag name through the libuv-awareresolve_system_errno(), but puts the raw stored value (theUV_E*magnitude, 4058) inSysErrInfo.errno.err_with_body(src/bun_core/output.rs:2403) looks the label up with it. On Windows 4058 namesUV_ENOENT, which has no coreutils label.Fix
get_error_code_tag_name()and pass the resolvedSystemErrnodiscriminant (2) inSysErrInfo.errno. The ZigOutput.errdid the same:coreutils_error_map.get(sys_errno)on the resolved errno.from_libuvon Windows, the resolved discriminant equals the stored errno. Output does not change there.bun:internal-for-testinghooksysErrorOutputErr(errno, fromLibuv). It runsOutput::erron a constructed error, likesysErrorNameFromLibuvdoes forname().test/js/bun/sys/error-name-from-libuv.test.tson windows-x64. Without theError.rschange the new test getsENOENT: sysErrorOutputErr (open), with itENOENT: No such file or directory: sysErrorOutputErr (open). Also passes on Linux (thefrom_libuvcase skips).Background
bun_sys::Error.errnoholds either anEdiscriminant (the sync syscall wrappers translate eagerly) or, withfrom_libuvset (the node_fs async uv callbacks), the negatedUV_E*code.resolve_system_errno()reads the flag to tell them apart.SysErrInfois the view of abun_sys::Errorthatbun_core::output::errconsumes.bun_corecannot namebun_sys::Error(crate tiers), sobun_sysfills it.bun_core::coreutils_error_map::get(i32)maps a number to itsSystemErrnoname, then to the glibc text. The WindowsSystemErrnoenum has aUV_*tail, so 4058 namesUV_ENOENT.Notes
Fd::closesetfrom_libuv, and those errors go to JS, not toOutput::err. So this is a latent inconsistency found by reading, not a user report. The hook is what makes it testable.Error::name()now shares theUNKNOWN_TAG_NAMEconstant with theErrNameimpl instead of repeating the literal.Error::from_uv_rc64stores the rawUV_E*magnitude withfrom_libuvunset, soReturnCodeI64::to_errorerrors get names likeUV_EBADF. That is Report EBADF instead of UV_EBADF for async libuv fs errors on Windows #37465.EUNKNOWNexpectation). The two changes touch different lines.bun_sys,bun_sys_jscandbun_coreon Linux, and forbun_sys_jscon Windows (bun_syshas pre-existing clippy errors on Windows unrelated to this change).no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/sys/error-name-from-libuv.test.ts