node:url: route url.format(URL[,opts]) to the WHATWG serializer - #32424
node:url: route url.format(URL[,opts]) to the WHATWG serializer#32424robobun wants to merge 1 commit into
Conversation
|
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:
Walkthrough
ChangesWHATWG url.format() options support
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 6:21 AM PT - Jul 23rd, 2026
✅ @robobun, your commit aa13b3ac96dcfba4c4bfae7c7c5055b6e4a03150 passed in 🧪 To try this PR locally: bunx bun-pr 32424That installs a local version of the PR into your bun-32424 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
The diff passes locally (fail-before/pass-after verified with The latest push (08d64d4) addresses the two review findings from the bot (path |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — both edge cases from the earlier review are addressed in 08d64d4 with tests; CI failures are BuildKite agent provisioning, not test failures.
Extended reasoning...
Overview
This PR adds a formatWHATWG serializer to src/js/node/url.ts so that url.format(whatwgURL, options) honors the auth/fragment/search/unicode options and stops dropping userinfo by default. It enables the previously commented-out Node-ported assertions in test/js/node/url/url-format-whatwg.test.js and adds an edge-case suite. Net change is ~115 new lines in url.ts plus ~90 lines of tests.
What changed since my last review
I previously flagged two Node-compat gaps (opaque-host case-folding under unicode: true, and loss of the /. disambiguator / empty ?# markers when reconstructing from public getters). Commit 08d64d4 addresses both:
hostnameToUnicodenow decodes only labels that literally start with lowercasexn--, leaving opaque-host bytes/case intact — matching ada's behavior. Tests coverfoo://EXAMPLE.com,foo://XN--0ZWM56D.EXAMPLE,foo://xn--0zwm56d.example, and a mixed-case multi-label host.- The no-authority branch now emits
/.whenpathnamestarts with//, and the search/fragment branches consulthrefto distinguish empty-but-present from absent. Tests coverweb+foo:/.//p,http://a/?#,http://a/#?(with and withoutsearch/fragmentoptions), etc.
Both inline threads are resolved and the bug-hunting pass on this revision found nothing.
Security risks
None. This is pure string re-serialization of an already-parsed WHATWG URL for Node.js API compatibility; no auth, crypto, FS, or network paths are touched. The new code only reads URL component getters and href.
Level of scrutiny
Medium. It is a hand-rolled serializer (Node uses ada's native one), so edge cases matter — but it has now been through a targeted review round, the fixes landed with regression tests, and the result is a strict improvement over the prior behavior where options were silently ignored and auth was always dropped.
Other factors
- No CODEOWNERS entry for
src/js/node/url.ts. - The CI failures on builds 62915/62927/62948 are all "Failed to create agent" (BuildKite image provisioning), not build or test failures; the author confirmed local fail-before/pass-after with
bun bd test. - The github-actions bot notes potential duplicate PRs (#24402, #27885); that is a merge-coordination question, not a correctness concern for this diff.
|
Pushed 04567dc to get this unstuck. Two things were keeping CI red:
Re-verified against today's main (eba370b) since the branch is two weeks old and
Differential URL corpus |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/js/node/url.ts`:
- Around line 483-497: The options handling in url.format only calls
validateObject(options, "options") inside the truthy branch, so falsy
non-undefined inputs like false, 0, "" and null bypass validation. Update the
options guard in url.format to validate any provided value except undefined, so
these invalid arguments still flow through validateObject and throw the typed
error. Use the existing url.format options destructuring and validateObject
helper to keep the fix localized.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 733a60b2-e506-45dd-844a-f029482f8f05
📒 Files selected for processing (1)
src/js/node/url.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/node/url/url-format-whatwg.test.js (1)
86-86: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStrengthen weak self-referential assertion.
This asserts two dynamically-computed values are equal to each other rather than against a known literal. If
unicode: truewere silently ignored (or mishandled) for opaque-path schemes in a way that affected both calls identically, this assertion would still pass without detecting the regression.🧪 Proposed fix
- assert.strictEqual(url.format(new URL("tel:123")), url.format(new URL("tel:123"), { unicode: true })); + assert.strictEqual(url.format(new URL("tel:123"), { unicode: true }), "tel:123");🤖 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 `@test/js/node/url/url-format-whatwg.test.js` at line 86, The assertion in the WHATWG URL format test is self-referential because it compares two computed url.format(new URL("tel:123")) results instead of validating against an expected literal. Update the test around the tel:123 case to assert the exact formatted output for the default call and the unicode: true option separately, using the url.format behavior as the source of truth. Keep the check in url-format-whatwg.test.js near the existing tel:123 coverage so regressions in opaque-path handling are caught even if both paths fail the same way.
🤖 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.
Outside diff comments:
In `@test/js/node/url/url-format-whatwg.test.js`:
- Line 86: The assertion in the WHATWG URL format test is self-referential
because it compares two computed url.format(new URL("tel:123")) results instead
of validating against an expected literal. Update the test around the tel:123
case to assert the exact formatted output for the default call and the unicode:
true option separately, using the url.format behavior as the source of truth.
Keep the check in url-format-whatwg.test.js near the existing tel:123 coverage
so regressions in opaque-path handling are caught even if both paths fail the
same way.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6c9c5fbe-c9cc-4842-9049-929f341318c1
📒 Files selected for processing (1)
test/js/node/url/url-format-whatwg.test.js
|
Addressed the self-referential assert.strictEqual(url.format(new URL("tel:123"), { unicode: true }), "tel:123");next to the existing |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/node/url/url-format-whatwg.test.js (1)
86-97: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRedundant vacuous assertion still present alongside the fix.
Line 97 correctly replaces the weak "compare against itself" check with an absolute expected string, matching the commit's own rationale that computed-vs-computed comparisons "could miss a regression." However, the original vacuous assertion at Line 86 (
url.format(new URL("tel:123"))compared tourl.format(new URL("tel:123"), { unicode: true })) is still present and now redundant — it adds no coverage beyond what Line 97 already asserts.Consider removing Line 86 (or converting it to an absolute-value assertion) to avoid keeping a known-weak pattern in the suite.
🧹 Suggested cleanup
- assert.strictEqual(url.format(new URL("tel:123")), url.format(new URL("tel:123"), { unicode: true })); + assert.strictEqual(url.format(new URL("tel:123"), { unicode: true }), "tel:123");🤖 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 `@test/js/node/url/url-format-whatwg.test.js` around lines 86 - 97, The WHATWG url.format tests still contain a redundant self-comparison for the tel URL case, which adds no real coverage now that the edge-case assertions use absolute expectations. Update the url-format-whatwg test around the WHATWG edge cases to remove the vacuous assertion comparing url.format(new URL("tel:123")) against the unicode variant, or replace it with an explicit expected string like the other assertions, so the suite only keeps meaningful checks in url.format and new URL("tel:123").
🤖 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.
Outside diff comments:
In `@test/js/node/url/url-format-whatwg.test.js`:
- Around line 86-97: The WHATWG url.format tests still contain a redundant
self-comparison for the tel URL case, which adds no real coverage now that the
edge-case assertions use absolute expectations. Update the url-format-whatwg
test around the WHATWG edge cases to remove the vacuous assertion comparing
url.format(new URL("tel:123")) against the unicode variant, or replace it with
an explicit expected string like the other assertions, so the suite only keeps
meaningful checks in url.format and new URL("tel:123").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 709d4705-d950-4d69-b7d8-9230036a9701
📒 Files selected for processing (1)
test/js/node/url/url-format-whatwg.test.js
|
Same line as the previous round, so keeping it is deliberate rather than missed. Line 86 is a verbatim port of the assertion in Node's own Happy to remove it if a maintainer prefers the suite trimmed. |
|
This is ready; the remaining failure is the automated check's environment, not the diff. In the latest automated check, the ASAN build fails with On a clean checkout of the PR head (d95b586), the check's exact command passes in 40s and writes the junit report: Fail-before still holds: |
d95b586 to
874e038
Compare
|
Correction to my previous comment: the extra files in the check's workspace are not corruption, they are current I have rebased onto current |
|
CI on the latest push is red only on failures that are unrelated to this change.
The failing lanes break down as:
For completeness, the only consumer of |
01e59fd to
8ab251f
Compare
There was a problem hiding this comment.
All of my earlier findings have been addressed and no new issues turned up this round, but this is a hand-written ~90-line WHATWG URL serializer with a documented deliberate divergence from Node's ada — worth a maintainer sign-off on the approach rather than an auto-approve.
Extended reasoning...
Overview
Adds a URL-instance branch to urlFormat in src/js/node/url.ts with two new helpers, formatWHATWG (~60 lines) and hostnameToUnicode (~25 lines), and enables/extends the previously commented-out Node-ported assertions in test/js/node/url/url-format-whatwg.test.js. Fixes three linked issues where url.format(new URL(...), options) dropped auth and ignored fragment/search/auth/unicode.
Security risks
None identified. Pure string serialization of an already-parsed URL; no I/O, no auth/crypto, no path resolution against the filesystem. The ?/# scan of href is safe because the WHATWG parser percent-encodes those bytes everywhere except the actual delimiters.
Level of scrutiny
Medium-high. This is Node-compat surface (node:url) and the implementation is a from-scratch serializer that reconstructs the URL from public getters plus href scanning, rather than calling a native serializer the way Node does via ada. It has been through five rounds of inline review here (opaque-host case preservation, the /. path disambiguator, empty-but-present ?/#, != null option semantics, bare/undecodable xn-- labels) and each was fixed with a test. The author also ran an 832-case differential against Node v26.3.0. That's a lot of confidence, but the shape of the solution is non-trivial enough that a human should ratify it.
Other factors
- One deliberate, documented divergence remains: mixed-case
xn--…labels in opaque (non-special-scheme) hosts underunicode: truego through UTS#46domainToUnicoderather than raw RFC 3492, so output matches Node's ownurl.domainToUnicodebut not Node'surl.format. I flagged this as non-blocking and the author added a source comment; a maintainer should confirm that trade-off is acceptable. - Two other open PRs (#24402, #27885) reportedly overlap with this fix — a human should decide which one lands.
- All inline threads on this PR are resolved; the bug-hunting system found nothing new this run.
8ab251f to
2c3c587
Compare
robobun
changed the title
|
Rebased onto current main (was 439 commits behind with conflicts) and added a round-trip test for |
There was a problem hiding this comment.
LGTM — all prior review rounds have been addressed and the bug hunter found nothing new on the current head.
What was reviewed:
formatWHATWGreconstruction: authority detection viahref[protocol.length..+1] === '//', the/.path disambiguator, and empty-vs-absent?/#recovery viahref.indexOf(verified?/#cannot appear earlier in a WHATWG serialization; theqIdx < hashIdxguard handles?inside a fragment).- Options handling:
if (options)truthy-gate and per-option!= nullboth match Node v26.3.0'slib/url.jsand are pinned by tests. hostnameToUnicode: per-labelxn--gate preserves opaque-host case; the one documented divergence (mixed-case content after lowercasexn--) is narrow, commented, and matches Node's owndomainToUnicode.
Extended reasoning...
Overview
Adds a WHATWG URL branch to urlFormat in src/js/node/url.ts (~115 new lines: option parsing, formatWHATWG, hostnameToUnicode) and un-comments/extends the Node-ported assertions in test/js/node/url/url-format-whatwg.test.js. Fixes three filed issues where url.format(new URL(...), opts) dropped auth and ignored fragment/search/auth/unicode.
Security risks
None identified. This is pure string re-assembly of components the WHATWG parser has already canonicalized/percent-encoded. No new parsing of untrusted bytes; domainToUnicode is the existing native binding. No filesystem, network, or credential handling.
Level of scrutiny
Medium. It's a Node-compat serializer with observable output that other packages rely on, so correctness matters, but the blast radius is limited to url.format(URL, ...) — the legacy Url path is untouched. The change has been through four review rounds here (opaque-host casing, /. disambiguator, empty ?/#, undecodable xn-- labels, != null semantics), each addressed with code + a targeted test, plus an 832-case differential against Node v26.3.0 recorded on the PR.
Other factors
- Every prior inline finding on this PR is marked resolved with a corresponding fix commit or a Node-verified rebuttal (the
!= nullbehavior I flagged turned out to match Node exactly; tests now pin it). - The remaining known divergence (UTS#46 vs raw punycode for mixed-case
xn--labels in non-special-scheme opaque hosts underunicode: true) is deliberately documented in a code comment, requires hand-crafted input to observe, and I explicitly said it was non-blocking in the earlier round. - Test file passed on all 265 CI lanes; the red lanes were unrelated main-side flakes (bake/production, zlib brotli, net-connect-memleak) that don't touch
node:url. - The bug-hunting system found nothing on the current head.
url.format() had no branch for WHATWG URL instances; they fell through
to Url.prototype.format, which reads legacy .slashes/.auth fields and
only emits "//" for the slashedProtocol table. Any scheme outside that
table (ws, wss, git+ssh, file, custom) lost its authority marker and
userinfo:
url.format(new URL("wss://h:99/x?q")) => "wss:h:99/x?q"
url.format(new URL("git+ssh://git@h/x.git")) => "git+ssh:h/x.git"
url.format(new URL("file:///a/b")) => "file:/a/b"
Add the instanceof URL branch, read the auth/fragment/search/unicode
options the same way Node does, and serialize from the URL's components
so format(u) === u.href modulo options.
Fixes #24233
Fixes #24343
Fixes #18695
2c3c587 to
aa13b3a
Compare
There was a problem hiding this comment.
LGTM — all four rounds of prior findings on this PR are addressed and the current diff has no open issues.
What was reviewed:
formatWHATWGre-serialization: authority//detection viahref, the/.path disambiguator, and empty-but-present?/#recovery — all match the WHATWG serializer and Node.hostnameToUnicode: per-labelxn--decoding preserves opaque-host case; the one deliberate divergence (mixed-case punycode under non-special schemes) is documented.- Option handling (
!= null, truthy-optionsguard) verified against Node v26.3.0; the 832-case differential in the thread is byte-for-byte. - Ruled out:
?/#cannot appear literally inhrefbefore their delimiter positions, soindexOfscans are safe; cross-realminstanceof URLwas considered and is a non-issue in practice.
Extended reasoning...
Overview
This PR adds a WHATWG-URL branch to urlFormat in src/js/node/url.ts so that url.format(new URL(...), options) no longer falls through to the legacy Url.prototype.format path (which dropped // and userinfo for any scheme outside the hardcoded slashedProtocol table and ignored the auth/fragment/search/unicode options). Two new helpers, formatWHATWG and hostnameToUnicode, implement the serializer; the test file un-comments the ported Node assertions and adds ~40 edge-case assertions covering everything raised in review.
Security risks
None. This is pure string re-serialization of an already-parsed WHATWG URL object — no filesystem, network, or eval surface. The output is derived from URL getters plus indexOf scans of .href; ? and # are percent-encoded by the WHATWG parser everywhere except their delimiter positions, so the scans cannot be confused by user-controlled path/userinfo bytes.
Level of scrutiny
Medium: it's a user-facing Node-compat API in the built-in JS layer, but no native code, no memory management, and the change is additive (a new branch guarded by instanceof URL; the legacy path is untouched). This PR has already been through four rounds of my own inline review — every finding (opaque-host case-folding, /. disambiguator, empty ?/#, != null semantics, bare/undecodable xn-- labels, mixed-case punycode) was either fixed with a test or, in the one case where matching Node exactly would require reimplementing raw RFC 3492 punycode for a hand-crafted input class, documented as a deliberate divergence that agrees with Node's own url.domainToUnicode. The author ran an 832-case differential (52 URLs × 16 option combos) against Node v26.3.0 with byte-for-byte parity.
Other factors
All prior inline threads are resolved. The CodeRabbit suggestion (validate falsy options) was correctly declined — Node itself gates on truthy options, and the test now pins that behavior for false/0/''/null. The one candidate the finder raised this run (cross-realm instanceof URL) was verified as a non-issue: a cross-realm URL falling through lands in Url.prototype.format.$call, which is exactly the pre-PR behavior for that input, so nothing regresses. CI on the previous head passed the target test on 265 lanes; the current head is a clean rebase plus one added round-trip test.
|
landed in #34660, url.format(URL, opts) matches node on main now |
3 participants
Closes #24233
Fixes #24343
Fixes #18695
What does this PR do?
url.format(url, options)fromnode:urlnow serializes WHATWGURLinstances through a WHATWG serializer instead of the legacyUrl.prototype.formatpath. This fixes two user-visible bugs://authority marker and userinfo were dropped for every scheme outside the legacyslashedProtocoltable (http/https/ftp/gopher/file). That includesws:,wss:,git+ssh:,file:with an empty host, and any custom scheme.fragment,search,auth, andunicodeoptions were silently ignored.Repro
Node returns
u.hrefunchanged forurl.format(u)with no options; Bun now matches.Cause
urlFormatinsrc/js/node/url.tshad no branch for WHATWGURLinstances and never accepted a secondoptionsparameter. AURLinstance fell through toUrl.prototype.format.$call(urlObject), which reads legacyUrlfields (.auth,.slashes,.query) that do not exist on WHATWGURL. With.slashesundefined, only the hardcodedslashedProtocoltable got//back, and with.authundefined the userinfo was always stripped.Fix
Add an
urlObject instanceof URLbranch mirroring Node'slib/url.js: readauth,fragment,search,unicodefromoptions(defaultstrue, true, true, false), validate withvalidateObject, and re-serialize from the URL's components. The authority check reads//directly out ofhrefso it works for any scheme.unicode: trueruns the hostname throughdomainToUnicodeper label.Verification
USE_SYSTEM_BUN=1 bun test test/js/node/url/url-format-whatwg.test.jsfails (4/4)bun bd test test/js/node/url/url-format-whatwg.test.jspasses (4/4)The test file previously had the full set of Node-ported assertions commented out behind
// TODO: Support these.; they are now enabled and a round-trip test forws:/wss:/git+ssh:/file:/custom schemes has been added.[review] gate passed · iteration 11 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 11
evidence per changed file