bun_core: use strerror() texts in the macOS coreutils_error_map - #39264
bun_core: use strerror() texts in the macOS coreutils_error_map#39264robobun wants to merge 3 commits into
Conversation
The macOS rows of coreutils_error_map were transcribed from the comments in Apple's <sys/errno.h>, several with the header's section headings glued on, instead of from strerror(). Shell builtins and Output.err printed them: on macOS `mkdir existing` reported "File or folder exists" where bash and Linux bun say "File exists", EBUSY was "Device / Resource busy", EPROCLIM "quotas & mush. Too many processes", and so on for 17 of Darwin's 106 errnos. EOPNOTSUPP (102 on Darwin) also fell through to glibc's text. Correct the ten rows whose Darwin text differs from glibc, drop the seven rows whose real Darwin text is the glibc text already in BASE, drop the EWOULDBLOCK row (Darwin's SystemErrno has no such variant), and add EOPNOTSUPP. Every Darwin errno now resolves to Apple's errlst.c text; the FreeBSD table already did. Add a source lint that checks BASE + the macOS DELTA against Apple's sys_errlist and both DELTAs against the SystemErrno enums they are keyed by (the tables are cfg'd per OS, so no runtime test on Linux can see the macOS rows), a shell test for the mkdir EEXIST message, and per-platform assertions in bun_errno's unit tests.
|
Warning Review limit reached
Next review available in: 5 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. 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 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Status: CI is green on head 1b3e619 (Buildkite build 99190, 179/179 jobs; source lints, clippy and miri workflows pass). Ready for review. Reproduced by auditing the macOS The missing |
There was a problem hiding this comment.
LGTM — data-only correction of the macOS errno string table to match Apple's strerror(), with three layers of tests.
What was reviewed:
- Verified each deleted DELTA row (EAGAIN, EDOM, EEXIST, ENETDOWN, ENOMEM, ENOTSOCK, ENOTSUP) falls through to a BASE text that already matches Darwin's
sys_errlist, and thatEWOULDBLOCKis absent fromdarwin_errno.rs(dead row). - Checked the new
EOPNOTSUPProw: Darwin errno 102 is a distinctSystemErrnovariant, so it needs its own text. - Confirmed the bunshell mkdir test relies on the file-level
$.nothrow(), so the nonzero exit doesn't throw before the assertion;tempDir/join/$are already imported. - The source-lint test's row regex matches the actual
comptime_string_map!andSystemErrnoenum syntax in the source files it parses.
Extended reasoning...
Overview
This PR corrects the #[cfg(target_os = "macos")] DELTA table in bun_core::coreutils_error_map (src/bun_core/result.rs), which maps errno values to the short text shell builtins print. The old table was transcribed from comments in Apple's <sys/errno.h> (with section headings glued on: "quotas & mush. Too many processes") rather than from strerror(). The fix corrects 10 texts, deletes 7 rows that duplicated BASE and one dead row, and adds EOPNOTSUPP. Three tests are added: a source-lint that parses the Rust tables and checks them against Apple's sys_errlist (so Linux CI covers the cfg-gated macOS rows), a per-platform Rust unit test, and an end-to-end mkdir test in bunshell.test.ts.
Security risks
None. This is a static string table used for user-facing error messages. No parsing of untrusted input, no auth/crypto, no memory management.
Level of scrutiny
Low-to-medium. The runtime change is pure data in a platform-gated block — no control flow, no logic. The strings are externally verifiable against Apple Libc's errlst.c and Go's generated zerrors_darwin_*.go. I spot-checked the deleted rows against BASE and confirmed each falls through to the correct Darwin text; I confirmed EWOULDBLOCK is not a Darwin SystemErrno variant and EOPNOTSUPP (102) is. The source-lint test is more elaborate but follows the established pattern in test/internal/source-lints/ (regex-parsing Rust source), and its regexes match the actual syntax of comptime_string_map! rows and the SystemErrno enum.
Other factors
- No CODEOWNERS cover the touched files.
- The bunshell test uses the file-level global
$.nothrow()(set inbeforeAll), so awaiting a failingmkdirreturns{stdout, stderr, exitCode}rather than throwing — the assertion is reachable. All required imports (tempDir,join,$) already exist in the file. - The PR description states the mkdir test's expected text ("File exists") is identical on every platform; BASE holds
"EEXIST" => "File exists"and Windows/Linux use BASE verbatim, so no platform guard is needed. - The FreeBSD DELTA was audited and left unchanged; the new lint checks it names real variants and diverges from BASE.
- The PR is exceptionally well-documented, cites the upstream source of every text, and includes the failing lint output on main.
The FreeBSD check only looked at the rows present in its DELTA, so a deleted or re-worded override (the bug class the macOS table had) passed. Express FreeBSD's errlst as its eight differences from Darwin's and run the same two checks over both BSD tables.
There was a problem hiding this comment.
LGTM — data-only correction of the macOS coreutils_error_map DELTA to match Apple's actual strerror() output.
What was reviewed
- Each of the 10 corrected / 8 deleted / 1 added rows against the DARWIN_SYS_ERRLIST reference; the 7 deleted rows do fall through to identical BASE text, and the deleted
EWOULDBLOCKrow was unreachable (no such variant indarwin_errno.rs). - The new
bunshell.test.tsmkdir case on Windows:ERROR_ALREADY_EXISTSmaps toEEXISTinwindows_errno.rs:708, BASE hasEEXIST => "File exists", anderr.pathis set to the same absolutetargetthe test asserts, so no platform guard is needed. - The source-lint's regexes against the actual shape of
result.rsanddarwin_errno.rs/freebsd_errno.rs—SUCCESS = 0is correctly excluded, and a row the row-regex fails to parse would surface as a mismatch in the completeness check rather than pass silently. - File-level
$.nothrow()atbunshell.test.ts:36means.quiet()without.nothrow()on the failing mkdir won't throw.
Extended reasoning...
Overview
This PR corrects the macOS DELTA of bun_core::coreutils_error_map in src/bun_core/result.rs — a static string→string lookup table that supplies the text shell builtins print for an errno. 10 rows are re-worded to Apple Libc's sys_errlist text, 7 rows whose corrected text equals BASE are deleted, the unreachable EWOULDBLOCK row is deleted, and EOPNOTSUPP (a distinct Darwin errno, 102) is added. A one-line comment is added above the block. src/errno/lib.rs gains a #[cfg]-branched Rust unit test. Two test files are added/extended: a new source-lint that parses BASE + both DELTAs and both BSD SystemErrno enums out of source and checks them exhaustively against the OS's sys_errlist, and a shell integration test that mkdir on an existing directory prints mkdir: <path>: File exists.
Security risks
None. The change is string-literal content in a static error-message table; no parsing, no allocation, no user input handling, no control flow.
Level of scrutiny
Low. The only runtime change is inside #[cfg(target_os = "macos")] and affects error-message wording only — the failure mode of a wrong row is a slightly-wrong message, not a crash or behavioral difference. The strings are sourced from Apple's errlst.c and cross-checked against Go's generated zerrors_darwin_*.go (which is produced by calling strerror() on macOS). The source-lint test transcribes Darwin's full 106-row sys_errlist and asserts BASE+DELTA resolve every one exactly, so any transcription error would fail there.
Other factors
- I traced the new
bunshell.test.tscase for Windows correctness:mkdir_non_recursive→Syscall::mkdirreturnsERROR_ALREADY_EXISTS, mapped toSystemErrno::EEXIST(src/errno/windows_errno.rs:708), which BASE resolves to "File exists";ShellMkdirTask::run_from_thread_poolsetserr.pathto the absolute input (filepath.as_bytes()), which is exactly thetargetthe test asserts. The file sets$.nothrow()globally (line 36), so the un-.nothrow()'d.quiet()call is fine. - The source-lint's
parseStringMapregex/^\s*"(\w+)" => "([^"\\]*)",/gmwon't match rows containing quotes/backslashes, but since the completeness checks require every enum errno to resolve and every DELTA key to be in the divergent set, a row it silently skipped would surface as a failure elsewhere rather than a false pass.parseSystemErrno'sE\w+correctly excludesSUCCESS = 0. - The two comment-cop bot comments were addressed in 1b3e619 (both threads resolved). No prior claude[bot] review on this PR.
- The FreeBSD DELTA is not changed but is now pinned by the same lint (commit 09fdad7); freebsd_errno.rs contains ECAPMODE/EDOOFUS/EINTEGRITY/ENOTCAPABLE, matching the differences table.
Problem
Output.errprint made-up errno texts:mkdir existingreportsmkdir: /path/existing: File or folder existswhere bash, the BSD coreutils and Linux bun all sayFile exists; EBUSY printsDevice / Resource busy, EPROCLIMquotas & mush. Too many processes, ESTALENetwork File System. Stale NFS file handle, and so on. Linux and Windows are unaffected.DELTAofcoreutils_error_mapinsrc/bun_core/result.rs:190was transcribed from the comments in Apple's<sys/errno.h>(several with the header's section headings glued on) instead of fromstrerror(). 17 of Darwin's 106 errnos came out wrong: 16DELTArows, plus EOPNOTSUPP (a distinct errno, 102, on Darwin) which has no row and falls through to glibc's text.EWOULDBLOCKrow names an errno Darwin'sSystemErrnodoes not have, so it was unreachable.#[cfg(target_os)]'d, so a test running on Linux never sees the macOS rows. Found while reviewing shell(mkdir): accept --verbose instead of the misspelling --vebose #39221, whose mkdir test accepts both spellings of EEXIST until this lands.Fix
src/bun_core/result.rs: correct the 10 rows whose Darwin text really differs from glibc (EBUSY, EPROCLIM, ESTALE, EPWROFF, EDEVERR, EBADEXEC, EBADMACHO, EMULTIHOP, ENOLINK, ENOPOLICY), delete the 7 rows whose Darwin text is the BASE text and the dead EWOULDBLOCK row, add EOPNOTSUPP. 43 rows remain; all 106 Darwin errnos now resolve to Apple'serrlst.ctext. The FreeBSD DELTA was audited the same way and is already correct.strerror(), and on macOSstrerror()returns Apple Libc'ssys_errlist. The new texts were checked against Go'ssyscall/zerrors_darwin_*.go, which is generated by callingstrerror()on macOS (it lowercases the first letter, nothing else).EMULTIHOP (Reserved)andENOLINK (Reserved)look odd but are literally what macOSstrerror()returns.test/internal/source-lints/coreutils-error-map.test.ts(new): parses BASE and both DELTAs out ofresult.rsand the three POSIXSystemErrnoenums out ofsrc/errno/, and checks, for macOS and for FreeBSD, that BASE + DELTA resolve every errno of the OS's enum to the OS'ssys_errlisttext and that the DELTA holds exactly the errnos whose text differs from BASE (so a missing, redundant, dead or re-worded row all fail). Apple's list is transcribed in full, in errno order; FreeBSD's is expressed as its eight differences from Apple's (both descend from 4.4BSD's errlst). A last check keeps BASE covering every Linux errno with no unreachable rows. Reading source is the only way to cover these rows from Linux; it runs in the source-lints workflow, which already triggers onsrc/**/*.rs. On main the two macOS checks fail with the 17 wrong rows and the 8 extra keys (output below) and the FreeBSD ones pass; with this change everything passes.test/js/bun/shell/bunshell.test.ts:mkdiron an existing directory printsmkdir: <path>: File exists, the same text on every platform, so it needs no platform guard; it fails on macOS before this change.src/errno/lib.rs:cargo test -p bun_errnoasserts EEXIST's text everywhere and the macOS/FreeBSD/glibc EBUSY wording per platform (plus EOPNOTSUPP and EBADEXEC on macOS). Passes on Linux;cargo check --testsforaarch64-apple-darwin,x86_64-unknown-freebsdandx86_64-pc-windows-msvccompiles.bun bd test test/js/bun/shell/bunshell.test.ts(whole file) andbun test test/internal/source-lints/pass.Background
coreutils_error_map(src/bun_core/result.rs) maps an errno to the short text GNU/BSD coreutils print for it, i.e.strerror()'s text.bun_sys::coreutils_error_mapprojects it onto the typedSystemErrnoenum; shell builtins (Builtin::task_error_to_string,Error::to_shell_system_error) andOutput.errformat errors through it. Node-facing errors use the separate libuv table and are not involved here.BASEtable holding glibc's texts (used as-is on Linux and Windows) plus a#[cfg(target_os)]DELTAfor macOS and for FreeBSD; lookup consults DELTA first, then BASE. A DELTA row is therefore only meaningful when the OS's text differs from glibc's or the errno does not exist on Linux.SystemErrno(src/errno/<os>_errno.rs) is the per-OS errno enum; the tables are keyed by its variant names, which is why a row naming a variant the OS lacks (the oldEWOULDBLOCKrow) can never be reached.Lint output on main (before 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/shell/bunshell.test.ts