Conversation
The JSValue-name INVALID_ARG_VALUE overload rendered the name through
determineSpecificType, producing "The argument 'type string ('foo')'",
and never switched to "The property" for dotted names. It now
stringifies the name and delegates to the String-name overload. Both
validateArray variants use node's reason text, "must have a length of
at least N", instead of "must be longer than N".
|
Warning Review limit reached
Next review available in: 43 minutes 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 (3)
Comment |
|
Status
|
There was a problem hiding this comment.
LGTM — small Node-compat fix for validateArray's minLength error message.
What was reviewed:
INVALID_ARG_VALUEJSValue-name overload now delegates to theWTF::Stringoverload aftertoWTFString()+RELEASE_RETURN_IF_EXCEPTION; confirmed the target overload already handles theargument/propertysplit andReceivedrendering.- Both
validateArrayvariants get Node's reason text; the changed overload has no other callers so no message regressions elsewhere. - New test asserts exact error name/code/message for both name shapes plus the unchanged non-array path; PR verified under
BUN_JSC_validateExceptionChecks=1.
Extended reasoning...
Overview
This PR fixes a malformed ERR_INVALID_ARG_VALUE message from the native validateArray when an array is shorter than minLength. Three files: ErrorCode.cpp rewrites the JSValue-name INVALID_ARG_VALUE overload to convert the name via toWTFString() and delegate to the existing WTF::String overload (which already produces Node's The argument '<name>' / The property '<dotted.name>' shape); NodeValidator.cpp updates both validateArray variants' reason text from "must be longer than N" to Node's "must have a length of at least N"; and a new test file asserts the exact messages against Node v26.3.0's output.
Security risks
None. This is error-message text formatting in a Node-compat validator. No parsing of untrusted input, no new allocations driven by user-controlled sizes, no auth/crypto/filesystem paths.
Level of scrutiny
Low. The path is only reachable via bun:internal-for-testing today (all in-tree validateArray callers pass minLength 0 or undefined per the PR description). The C++ change replaces a hand-rolled StringBuilder with delegation to an existing, already-correct overload — strictly a simplification. toWTFString() is guarded by RELEASE_RETURN_IF_EXCEPTION, and the delegated overload operates on the same ThrowScope& and does its own throwException + release(), so exception-scope discipline is preserved (verified under BUN_JSC_validateExceptionChecks=1 per the PR).
Other factors
The fix is at the right layer (the overload, not the call site), so future JSValue-name callers get the correct rendering. The old overload's use of determineSpecificType() on the name was clearly a bug — that helper describes a received value. The new test asserts name, code, and full message for the argument-name case, the dotted property-name case, and the unchanged non-array ERR_INVALID_ARG_TYPE path, plus a positive case guarding no-throw. The ASCIILiteral-name variant's text change is untested from JS but is a two-word string edit for consistency and has no live callers with nonzero minLength.
|
Updated 5:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit a56806d has some failures in 🧪 To try this PR locally: bunx bun-pr 38410That installs a local version of the PR into your bun-38410 --bun |
|
This bug was reported again during triage on 2026-08-21. Main (4448a2e) still prints One data point for a follow-up. The triage run executed node v26.3.0's |
Problem
validateArray(value, name, minLength)(the nativeinternal/validatorsexport) throws a malformedERR_INVALID_ARG_VALUEwhen the array is shorter thanminLength:The argument 'type string ('foo')' must be longer than 1. Received []The argument 'foo' must have a length of at least 1. Received []src/jsc/bindings/NodeValidator.cpp:354passes the name as aJSValue, and theJSValue-name overload ofBun::ERR::INVALID_ARG_VALUE(src/jsc/bindings/ErrorCode.cpp:1048) ran it throughdetermineSpecificType(), the helper that describes a received value (type string ('foo')). It also always wroteThe argument, where node writesThe propertyfor dotted names such asoptions.foo. That overload had no other callers.validateArrayvariants (NodeValidator.cpp:354and:375) saymust be longer than N; node'slib/internal/validators.jssaysmust have a length of at least N.bun:internal-for-testing/ vendored node tests: every in-treevalidateArraycaller passesminLength0 or undefined.Fix
JSValue-nameINVALID_ARG_VALUEoverload now converts the name withtoWTFString()and delegates to theWTF::String-name overload, which already produces node'sThe argument '<name>'/The property '<dotted.name>'form and the sameReceivedrendering. Fixing the overload rather than the call site means any futureJSValue-name caller gets the right message too.validateArrayvariants use node's reason text.ToStringsemantics, the same thing node's template literal does, so a Symbol or throwingtoString()name propagates the same error node raises.validate_arrayinsrc/runtime/node/util/validators.rsis a separate helper with its own message shape and is not touched here; itsmin_lengthis never set in-tree.test/js/node/internal/validators.test.ts(new): the twominLengthcases fail on the released binary and pass with this change; the non-array and success cases pass both ways and guard the unchanged paths.test-internal-validators-validateoneof.js,test-internal-validators-validateport.js,test-child-process-constructor.js,test-dns-setservers-type-check.js,test-process-setgroups.js,test-vm-basic.js,test-worker-process-argv.jsand theargv/execArgvtests inworker_threads.test.tspass (the callers of bothvalidateArrayvariants).BUN_JSC_validateExceptionChecks=1, including names whosetoString()throws.ASCIILiteral-name variant's new text is only reachable from C++ callers (JSWorker.cpp,BunProcess.cpp), all of which passminLength0, so it has no JS-observable test; it is changed for consistency with theJSValuevariant.Background
internal/validatorsis node's module of argument checkers (validateArray,validateOneOf, ...). Bun implements them natively insrc/jsc/bindings/NodeValidator.cpp; tests reach them throughexposedInternalsinbun:internal-for-testing, the same hook that serves vendored node tests declaring--expose-internals.Bun::ERR::*insrc/jsc/bindings/ErrorCode.cppbuilds node-style coded errors.INVALID_ARG_VALUEis overloaded on the name type (ASCIILiteral,WTF::String,JSValue); the native validators use theJSValueoverloads because they receive the name straight from JS.determineSpecificType()is the port of node's helper of the same name: it describes a received value forERR_INVALID_ARG_TYPEmessages (type string ('foo'),an instance of Array). It is the wrong tool for rendering an argument name.ERR_INVALID_ARG_VALUEmessage isThe <argument|property> '<name>' <reason>. Received <inspect(value)>, wherepropertyis used when the name contains a..