bun-src/.claude/docs/landing-prs.md

Landing PRs: situational review guidance

Companion to the "Landing PRs: What Bun Reviewers Catch" section in CLAUDE.md. These sections apply at specific moments — read the relevant one when:

• Node/Web compat — touching node:* modules, Web-standard APIs, or anything under src/runtime/node/ / src/js/node/.
• API design — adding or changing user-facing API surface (JS APIs, CLI flags, options).
• Performance — optimizing, touching hot paths, or making performance claims.
• Cross-platform — platform-gated code, FFI/ABI boundaries, or platform-sensitive tests.
• Dependencies & vendoring — bumping a dependency or vendored library, adding a dependency, editing anything under vendor/.
• Docs, types, and comments — touching docs, .d.ts files in packages/bun-types/, or JSDoc.
• PR process — before opening a PR, before requesting review, and when responding to review feedback.

Node/Web compat

• For node:* modules, real Node's observed behavior is the spec — not docs, @types/node, or intuition. (For Web-standard APIs — fetch, URL, streams — the WHATWG/W3C spec and WPT are the bar instead; Node is not the reference there.) Run the exact scenario under current Node and paste the repro + output in the PR; read the nodejs/node source for the function being emulated and cite permalinks in code comments for every magic constant and counterintuitive choice — "it matches Node" without a link does not unblock a thread. Port bug-for-bug, including grammatically wrong messages; don't modernize or ship accidental extra capabilities. If your fix only works by diverging from Node, suspect the bug is in a different layer. Deliberate divergence is never silent: raise it with reviewers and comment what Node does and why Bun differs.
• Match Node's full error contract. The exact ERR_* code, the constructor class (TypeError vs RangeError — user code does instanceof), the verbatim message text, the full property set (syscall, errno, the full access path like options.privateKeyEngine), the check ORDERING that decides which error wins on doubly-invalid input, and the delivery channel (sync throw vs error event vs rejection — for Web APIs the spec dictates this). Use the shared validators ($ERR_*, validateString, ErrorCode.ts), never hand-rolled typeof checks. Never validate stricter than Node (extra args don't throw; {opt: undefined} equals omitted). Assert err.code in tests, not just instanceof.
• The entire observable surface is compat API. Property attributes (writable/enumerable/configurable — never tightened for convenience), constructor.name and prototype chains, undocumented underscore internals ecosystem code probes (_writableState), documented defaults verbatim (511 not 512), per-instance state where Node uses factories (never module-level singletons — verify with multiple simultaneous instances). Ordering and timing are contract: state mutations relative to event emission are observable because handlers re-enter (set complete BEFORE push(null)); match sync-vs-nextTick callback timing exactly. If you can't establish Node's exact timing from its source, leave the behavior out rather than approximate. Never let user-visible behavior change as a side effect of unrelated work — dependency upgrades get patched; crashes are never fixed by deleting the feature.
• The upstream test suite is the bar. Port Node's own test files (test/js/node/test/parallel/) or WPT instead of hand-writing a small case set; write compat tests with node:test/node:assert so the identical file passes under real Node, and show both outputs. Check whether previously-disabled upstream tests can now be enabled ("does this add any passing node tests?" is a standard review question). Ported upstream files are verbatim-diffable: no edits, not even typos — necessary deviations get an inline comment or upstream-commit citation. Mark known failures in ported suites test.todo, never bare test.skip — todo flips to a failure when fixed (platform-capability gates still use test.skipIf with a reason). Never weaken ported assertions.
• Port the whole behavior, not the slice the issue mentioned. Every sibling form (sync/callback/promises — "Don't forget about fs.exists"), the spec's complete enumerations, the full input space the reference accepts (alternative spellings 'IPv4'/'ipv4'/4, fractional and negative numbers — truncate toward zero BEFORE negative-index math, absent-vs-empty distinctions), and real-world values beyond the spec (HTTP 999 exists in the wild). Malformed external input must surface as a catchable error the way Node does — never a panic. Never stub a path with a panic without checking whether real npm packages exercise it.

API design

• Make invalid states unrepresentable; declarations exactly as strict as the domain. Tagged unions over pairs of optionals; two-value enums over bare boolean parameters (nobody can read what a bare false means at a call site); explicit Option over in-band sentinels when the sentinel is a legal value; named enums on BOTH sides of FFI, never convention-interpreted ints; owned-vs-borrowed encoded in the type. A cast where the concrete type is statically known is a design smell — change the signature so misuse fails at compile time. Don't over-constrain either: accept every input form that costs nothing; never weaken a type-safety wrapper to silence a compile error — fix the call sites.
• Manage public surface deliberately, both directions. Internals behind private symbols or #fields (never underscore identifiers or Symbol.for — user code can forge them); no hidden options in Web-standard APIs; no new globals when an existing namespace fits. Ship no speculative surface: no options nobody asked for ("let someone complain about the lack of it" — an explicit ask in the issue is not speculative) — anything user-reachable becomes de-facto supported forever. But what you DO ship ships complete in the same PR: the natural surfaces for the feature (CLI flag AND programmatic API where both exist), .d.ts types, --help text, docs. Partial surface coverage is blocked at review, not deferred.
• Every accepted option does what it claims or fails loudly. Reject values the option cannot honor (a zero or negative where the semantics make it meaningless — not where it's a legal value like a hash seed) at parse time, before any I/O; throw on mutually-exclusive combinations rather than ignoring one; error when an option is accepted in a mode where it can't work; stubs return errors, not empty successes. Distinguish "user explicitly set X" from "X equals the default" before branching on it; undefined means "use the default", null means "explicitly off". Handle every accepted form identically (--flag value and --flag=value, NO_PROXY and no_proxy).
• Name from established precedent, in priority order: Web spec names for Web-standard features (lastModified, not mtime), Node's vocabulary for Node-equivalent features, npm/pnpm/yarn names for package-manager flags. Absent precedent, name for the concrete behavior, never the tool or customer that requested it. Keep camelCase JS options identical across native code, .d.ts, and docs. Public names are forever — propose the convention-matching name first.
• Never silently break existing users. Working behavior users plausibly rely on — even undocumented, even spec-noncompliant — cannot be removed or restricted as a side effect ("People use file descriptor numbers. It should be allowed"); the test is whether real code depends on it, not whether docs bless it. No new caps on application-controlled values. Renames of user-facing API keep deprecated aliases. Changing an existing default is a breaking change behind a flag; new behavior-changing options default OFF — if enabling by default breaks any existing test, it breaks users. Explicit user configuration beats newly-added inferred behavior; pin the precedence with a regression test. This contract covers only the user-observable surface: internal native code has no backwards-compat obligation — rename, restructure, and delete non-user-facing code freely.

Performance: what reviewers block

• Do each piece of work exactly once; keep the event loop responsive. Fold new validation into existing loops over the same data — the validation must still happen; what gets blocked is a separate O(n) pass when an existing loop already visits the bytes. Combined operations (getIfPropertyExists over has-then-get, getOrPut). Hoist invariant work out of ALL enclosing loop levels (fixes that move it up only one get re-flagged). On per-file/per-entry loops (installs, directory walks), count syscalls — attempt-the-operation-and-branch-on-errno instead of preflight stat/exists probes. Order compound conditions cheapest-first. In production code, no shift()/orderedRemove(0) draining of unbounded queues — accidentally-quadratic use is flagged even on cold paths. Cap bytes per event-loop turn in unbounded write loops. Never run synchronous filesystem calls on the JS thread inside async completion paths.
• Count the copies and allocations your native code makes. Write directly into the final destination — never create a temporary and copy, never build a JSString just to read its contents back. Check whether the callee already dupes before duping yourself. Reuse one scratch buffer across loop iterations. Multi-KB scratch comes from the shared pool (PathBufferPool), never tens-of-KB stack frames. Preallocate exact capacity when computable. Treat the byte size of frequently-instantiated structs as reviewed: measure size_of and state it in the PR; pack bools into existing flag bits; never embed large rarely-used buffers inline.
• The common case pays zero for rare features. Gate new allocations, subsystem init, and per-access interception on the precise condition needing them — no Proxies or getOwnPropertySlot hooks on hot paths (defeats inline caching, 10-100x slower); decide once at setup, not per event. Route the disabled case through the pre-existing code path completely unchanged. Gate debug diagnostics behind compile-time flags (cfg(debug_assertions), ASSERT_ENABLED) — an assertion whose condition calls across FFI is NOT compiled out. Fast paths must replicate every observable of the general path (shared-reference identity, state flags/locks, exotic inputs) — verify the precondition covers degenerate inputs or bail to the slow path. Never fix a rare-case bug by adding cost to the hot path.
• Performance claims need numbers; complexity fixed at the root. Before/after from the repo's bench suite (bench/) covering ALL input classes — both string encodings, short and long inputs; "faster on average" is rejected; must not regress ANY measured case; compare against the previous Bun release and Node. Never port V8/Node micro-optimizations assuming they transfer to JSC ("JavaScriptCore is a different engine. Do you have a benchmark?"). Never change optimization levels, LTO modes, or tuning knobs assuming higher is better — existing settings encode prior measurements. Treat existing perf mechanisms as load-bearing (inline directives, corking flags, odd API variants chosen to skip a copy) — re-add any fast path your rewrite drops. Never cap counts or rate-limit to hide quadratic behavior ("Do not solve quadratic behavior by limiting the count"). Verify complexity claims by doubling inputs and reporting the ratio; adversarially test any memo/cache against inputs that thrash it.
• JSC binding C++: use the engine's cached fast paths. LazyClassStructure/cached structures per global — a structure created per instance permanently defeats inline caching. vm.propertyNames/BuiltinNames instead of Identifier::fromString per call (takes a lock, shows up in profiles). MAKE_STATIC_STRING_IMPL for fixed strings. JSType tag checks over constructor-name comparisons. Internal state in WriteBarrier'd native fields, not observable JS properties. Function-local Meyers singletons over top-level statics. Throw scopes at the top of the function.

Cross-platform

• Never assume OS/ABI facts are portable. errno meanings differ (EPERM is a sharing violation on Windows), event flags differ, Windows env vars are case-insensitive, Windows has no POSIX signals, blocking syscalls retry on EINTR. FFI/ABI: explicit calling conventions on BOTH sides; fixed-width or C-ABI types, never bare int read from c_ulong (LLP64 garbage on Windows); extern declarations diff'd parameter-by-parameter against definitions — they compile cleanly per side and crash only on the platform you didn't build; complete Windows error-translation tables with fallbacks instead of force-unwraps.
• Platform parity is part of every fix. When you fix one platform backend (POSIX vs kqueue vs epoll vs libuv), audit every sibling backend for the same defect and apply symmetrically — or state why a backend is unaffected ("kqueue register path is unhandled. You only patched unregister."). A POSIX-only API addition ships its Windows equivalent in the same PR. Enabling a feature on a new platform means grepping every gate, dispatch chain, parallel platform script, test skip, and allowlist. Platform-specific CI failures in files you touched are real merge-blocking bugs, never flakes. Comment WHY on every new platform exclusion.
• Write tests to pass on every CI platform (Windows, macOS x64+arm64, Linux glibc and musl). Split on /\r?\n/ (Windows CRLF); normalize separators in path assertions; never spawn shell builtins (echo, sleep are not programs on Windows — use bunExe() -e); no hardcoded /tmp or /bin; incidental servers bind 127.0.0.1, never ::1 (CI Linux may lack IPv6 — IPv6-specific tests gate on the harness IPv6 helper); exit codes not signal names for "did not crash" (Windows has no signals); when probing a limit, exceed the LARGEST platform limit to trip the guard and stay under the SMALLEST when constructing inputs (macOS PATH_MAX is 1024). Before skipping a platform, verify it genuinely lacks the capability (Windows supports AF_UNIX). Skip narrowly via test.skipIf with a reason; never fix one platform by loosening assertions for all.
• Decide explicitly: filesystem path or URL-like identifier. Module specifiers, cache keys, sourcemap paths use forward slashes everywhere (posix path helpers). Filesystem paths use platform path APIs, never literal '/' concatenation. Windows: accept BOTH separators; drive-relative (C:foo) and UNC forms exist; PATH splits on ';'. On POSIX, backslash is a legal filename character. Splitting a posix-normalized string with the platform separator silently no-ops on Windows — feed the other separator style through every new API in tests.
• Beyond rust:check-all (required by CLAUDE.md) for platform-gated code: verify link-time symbol resolution (a POSIX extern must still resolve on Windows even if runtime-gated); audit enum switches duplicated across platform arms; distrust lint sweeps — a cast redundant on your host may be load-bearing on another target. Trick: flip the platform condition locally to force the other branch through the type-checker.

Dependencies & vendoring

• Version bumps are repo-wide, verified operations. Never merge a pin to an ephemeral artifact (preview tags, unmerged-PR builds) — swap to the merged upstream SHA and verify prebuilt artifacts exist for every platform × flavor before merge. Grep the entire repo for the old version value — build scripts, CI configs, Dockerfiles, and deliberate assertion tables — and update every duplicate in one commit. For vendored bumps: rebase every local patch and verify fetch+patch+compile from a clean state; verify the exact replacement upstream chose before mass-renames (WTF::move, not std::move — a plausible-but-wrong substitution × 300 files cost a 1570-line fixup). Codegen steps declare their input files as dependencies so outputs regenerate; build caches are keyed by compile flags too, not just OS/arch.
• Adding a dependency is a last resort. Inline trivial utilities; use the platform's own API or JSC-backed implementation over wrapper packages; every dependency must be traceable to a concrete consumer ("where are they used?"). Include license attribution in the same PR for any copied open-source code. Vendored code (vendor/, WPT fixtures, Node test files) is read-only — no style or typo fixes (copies serve as conformance baselines); exclude vendored dirs from mechanical rewrites. Vendor patches stay small with a comment explaining what upstream behavior they correct, plus the upstream issue link.
• Dependency ranges follow the audience. Repo-internal manifests (test fixtures, tooling, CI images) pin exact versions — never ^ or ~, never "tidy" an exact pin into a range. Published packages do the opposite: * for @types/node in bun-types, peerDependencies for toolchains users already have, and bundle runtime deps into shipped artifacts — the end-user machine has no node_modules. Overrides/resolutions entries are load-bearing — find out what breakage one prevents before deleting it.

Docs, types, and comments

• Sweep the same PR for everything describing the old state. When behavior, names, or contracts change — including mid-PR pivots — update or delete: comments beyond the hunk, sibling/mirror implementations, JSDoc, "see above" cross-references, READMEs, CLAUDE.md, --help text, error-message hints. A comment contradicting the code is a correctness bug, not a nit — a stale refcount comment invites a future maintainer to "restore" unref() and cause a double-free. Write comments about the code as it now is, never narrating the change.
• Comments must be load-bearing and true. Any line correct for a non-obvious reason gets a why-comment: special-case branches (with a triggering input), deliberate deviations from the reference, magic constants (cite the spec line), workarounds (link the upstream issue). When a reviewer asks "is this state possible?" — answer with a code comment, not just a thread reply; articulating the invariant routinely exposes that it doesn't hold. SAFETY comments state the precise invariant and where it's enforced — against every caller — and get re-verified after each refactor. Encode documented preconditions as debug assertions rather than prose.
• Verify every documentation claim you publish, by execution. Run each snippet end-to-end exactly as written; fetch every URL; check option names/defaults against the implementation on main; preview rendered markdown (an unbalanced fence swallows everything after it). Replace marketing language with the specific guaranteed property. Never claim full compatibility when partial — enumerate what works. Don't publish claims you haven't verified — verify, scope down, or drop them (an AI-drafted page with unverifiable claims was deleted wholesale, +9/-325). Existing docs you didn't touch are out of scope.
• Docs prose follows the voice rules in docs/project/contributing.mdx ("Voice"). Short sentences, one point each (a sentence with several commas or a dash-separated aside becomes two sentences or a list); active voice with the actor named ("Bun reads X", not "X is read"); present tense for current behavior (no "will"); "you" for the reader, never tutorial "we"/"let's"; no "easy"/"simple"/"just"/"quick"; name the subject where a bare "this" is ambiguous; say what to do rather than what not to do. Docs-wide passes have been merged to remove exactly these patterns (#28788, #33112), and #38686 rewrote pages that had reintroduced them the same day they merged; prose that breaks these rules costs a follow-up PR.
• TypeScript declarations mirror the runtime exactly, in the same PR. Declare only what's implemented — verify by running the API, never docs or the PR description; no types for stubbed APIs. Literal unions for fixed string sets ('A' | 'B' | (string & {}) for open sets); overloads so parameters are only accepted where the runtime accepts them; prop?: T | undefined for exactOptionalPropertyTypes; Uint8Array<ArrayBuffer> generics (TS 5.9+); new type parameters get defaults so existing call sites compile; no new globals colliding with lib.dom/@types/node (use the Bun namespace); never widen a type or as any to silence one call site. No */ inside JSDoc (glob patterns break the entire .d.ts parse). Validate by compiling realistic usage in bun-types fixtures under BOTH tsconfigs (with and without DOM).
• Write JSDoc for a zero-context reader. Option docs explain what the option DOES — semantics, edge behavior, sentinel meanings (0 = unlimited), when it has no effect, the equivalent CLI flag — never a wordier restatement of the name. The .d.ts JSDoc is the canonical IDE-tooltip surface; constraints documented only in .mdx are invisible at the point of discovery. Security-adjacent examples must be safe to copy verbatim (least privilege, never User=root).

PR process

• Re-read your entire diff line-by-line as a reviewer would, before requesting review. Delete all development residue: debug prints, commented-out code, forced conditionals, scratch files, leftover .only and debugging skips (a committed .only silently disables every other test in the file in CI), unused imports, AI-generated explanatory padding. Not cleaned up yet → open as draft.
• Audit the full diff for accidental ride-alongs. Submodule pointer bumps, lockfile churn from rebases, regenerated snapshots, formatter churn on untouched code, stash leakage. After every merge/rebase with main, re-diff against main: conflict resolution can silently resurrect deleted code or drop your own headline fix while keeping its test (a "one-line test tweak" commit once touched 33 files and reverted the entire production fix). Every file in the diff must be explainable from the PR's stated purpose.
• The PR description is the permanent squash-commit message — keep it true. State the root cause and make the exact fixing line identifiable apart from refactoring ("Which line was the fix?"). Name the verifying tests and state they fail on the unfixed build. "Fixes #N" must match the issue's actual repro; a partial fix says so. Re-sync title/description whenever review reworks the change. Every hunk needs an articulable one-sentence justification — pre-empt it in the description or a code comment for anything a reviewer can't explain from context. On large mechanical diffs, leave self-review comments pointing at the load-bearing hunks. Never delete unrelated code, others' TODOs, or debug tooling in passing — deletions your change orphans are required (see "Delete dead code"), but each is intentional and named.
• Treat every review suggestion — especially from bots — as an unverified hypothesis. Reproduce or check it against actual API semantics before applying or dismissing. Apply real findings; decline wrong ones in-thread with checkable evidence (file:line, run transcripts) — evidence-backed rebuttals close threads, bare dismissals don't. Never blanket-apply (blindly-applied suggestions have reintroduced known ASAN failures), never resolve threads silently in bulk (a bulk-resolve once swallowed a genuine correctness bug). When a reviewer flags a pattern once, sweep and fix every instance — Jarred leaves one substantive comment then "ditto" on each clone; fixing only the commented line guarantees another round.
• Green CI on every platform is a hard precondition. Maintainers file changes-requested reviews consisting solely of "CI is failing". Regenerate checked-in codegen outputs — again after every rebase. Every failure on your branch but not on main is yours to root-cause; "probably a flake" requires a link to the same failure on main. Check per-job results, not the aggregate icon, and confirm CI actually executed your tests — path filters silently skip them.
• One concern per PR, scoped to the narrowest change that fixes it. Fixing every instance of the same bug class is ONE concern (see "Fix the whole class"); drive-by refactors, style cleanups of adjacent code, and vendored upgrades are not. If one part triggers design debate mid-review, carve it out so the uncontroversial part merges. Diff size itself is grounds for changes-requested.
• Pre-existing bugs surfaced by review: acknowledge, scope, track. Never silently ignore, never silently widen your diff. State the mechanism in-thread, note your PR doesn't change it, file a tracking issue — "out of scope" without a tracker is not accepted. Exception: if it's the exact bug class your PR claims to eliminate, fix all instances in the same PR ("pre-existing, will follow up" for the same crash class gets "no, fix it.").