jsc: use 'property' wording in get_optional_slice type error - #32355
Conversation
get_optional_slice reads a property from an options object, so the ERR_INVALID_ARG_TYPE message should say 'property' (matching the Zig reference getOptional ZigString.Slice path via throwErrInvalidArgType) rather than 'argument'. Routes through throw_invalid_property_type, the same helper get_boolean_strict already uses. Also fixes the identical local shim in csrf_jsc.rs.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough
ChangesNon-string property error helper alignment
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward error-message wording fix with a test; the two inline nits are optional cleanups, not blockers.
Extended reasoning...
Overview
This PR swaps a single error-throwing call in JSValue::get_optional_slice (and its duplicate local shim in csrf_jsc.rs) from throw_invalid_argument_type_value to throw_invalid_property_type, so that type errors on options-object properties say "property ... got" instead of "argument ... Received" — matching the Zig reference implementation. A regression test is added in terminal.test.ts asserting the exact {code, name, message} shape. Net diff: 2 lines changed in source, 4 lines deleted, 14 lines of test added.
Security risks
None. This only affects the text of a TypeError thrown on the validation-failure path. The error code (ERR_INVALID_ARG_TYPE) and error class are unchanged, no new inputs are accepted, and the happy path is untouched.
Level of scrutiny
Low. The change is cosmetic (error-message wording), type-checks against the existing throw_invalid_property_type(name: impl AsRef<[u8]>, ...) signature, and follows the established pattern already used by get_boolean_strict a few lines below. Both relevant test suites (terminal.test.ts, csrf.test.ts) pass per the PR description.
Other factors
The bug-hunter surfaced two nits: (1) sibling algorithm/expiresIn/maxAge checks in the same CSRF file still use "argument" wording, and (2) the local get_optional_slice shim is now byte-identical to the public method and could be deleted. Both are valid cleanup suggestions but neither is a correctness issue or a regression — they're pre-existing inconsistencies that this PR partially improves. Fine to land as-is and address in a follow-up.
The shim became identical to JSValue::get_optional_slice after the previous commit; delete it and call the method directly.
|
Updated 4:48 PM PT - Jun 15th, 2026
✅ @robobun, your commit 1c058d4b0156df58e70d6f161c02e72f6701c307 passed in 🧪 To try this PR locally: bunx bun-pr 32355That installs a local version of the PR into your bun-32355 --bun |
There was a problem hiding this comment.
LGTM — thanks for deleting the shim; the remaining algorithm/expiresIn wording nit is cosmetic and fine for a follow-up.
Extended reasoning...
Overview
One-line change in src/jsc/JSValue.rs swapping throw_invalid_argument_type_value → throw_invalid_property_type inside get_optional_slice, so type errors for option-bag properties read "property … got" (matching the Zig reference and get_boolean_strict) instead of "argument … Received". src/runtime/api/csrf_jsc.rs deletes the now-redundant local get_optional_slice shim and points its 3 callers at the JSValue method (addressed my earlier nit in 506f58f). A new test in terminal.test.ts pins the exact {code, name, message} shape.
Security risks
None. This only changes the text of a TypeError thrown on the invalid-input path; the error code (ERR_INVALID_ARG_TYPE) and class are unchanged. No auth/crypto/permission logic is touched — csrf_jsc.rs edits are a pure refactor of which helper is called, with the token generate/verify logic untouched.
Level of scrutiny
Low. The new helper already exists (JSGlobalObject::throw_invalid_property_type, takes impl AsRef<[u8]> + &str + JSValue), so the signature swap is type-checked by rustc. The shim deletion is behaviorally identical (I verified line-by-line equivalence in my prior review), and get_optional_slice is already called as a method elsewhere in bun_runtime. None of the touched files are under CODEOWNERS.
Other factors
Author ran both affected suites (terminal.test.ts 88 pass, csrf.test.ts 24 pass). Bug-hunting found nothing this round. My other prior nit — that algorithm / get_optional_int_u64 in csrf_jsc.rs still use the "argument" wording — was explicitly cosmetic and non-blocking; leaving it out of scope here is a reasonable call.
There was a problem hiding this comment.
LGTM — thanks for deleting the shim.
Extended reasoning...
Overview
One-line swap in JSValue::get_optional_slice (src/jsc/JSValue.rs:1213) from throw_invalid_argument_type_value to throw_invalid_property_type, so type errors for option-bag properties say "property … got" instead of "argument … Received", matching the Zig reference. The follow-up commit (506f58f) deletes the now-redundant local get_optional_slice shim in src/runtime/api/csrf_jsc.rs and rewrites three call sites to use the JSValue method directly. A new test in terminal.test.ts pins the exact {code, name, message} shape.
Security risks
None. This only changes error-message text; the error code (ERR_INVALID_ARG_TYPE) and class (TypeError) are unchanged. No new inputs, no new control flow, no auth/crypto logic touched (the CSRF file edit is a pure call-site rename to an identical helper).
Level of scrutiny
Low. The JSValue.rs change is a one-token helper swap whose target (throw_invalid_property_type) already exists, takes impl AsRef<[u8]> for the name (so the &[u8] property binding still fits), and is already used by the sibling get_boolean_strict. The csrf_jsc.rs change is mechanical dead-code removal I suggested in the prior review round; the three call sites now invoke a method whose body is byte-identical to the deleted shim.
Other factors
Both of my earlier nits are resolved: the shim deletion was applied, and the author gave a reasonable reference-matching rationale for leaving algorithm / get_optional_int_u64 as-is (they track different Zig paths). The bug-hunting pass found nothing on the latest revision, CodeRabbit had no actionable comments, and the PR carries passing test runs for both affected suites.
|
CI status: the diff is green.
Ready to merge when a maintainer is satisfied with the CI state. |
2 participants
Repro
Before:
After (matches Zig 1.3.14):
Cause
JSValue::get_optional_sliceinsrc/jsc/JSValue.rsreads a property from an options object but calledthrow_invalid_argument_type_value, which produces the "argument ... Received" format. The Zig reference (getOptional(... ZigString.Slice)insrc/jsc/JSValue.zig) routes throughjsc.Node.validators.throwErrInvalidArgType, which produces the "property ... got" format.This affects every caller of
get_optional_slice:Bun.Terminal(name),Bun.build(target,outdir,banner,footer,root,publicPath,naming.*,jsx.*),Bun.servestatic routes (index,dir), andBun.CSRF(sessionId,secret).Fix
Route through
throw_invalid_property_type, which already exists and is whatget_boolean_strictuses for the identical situation. Also fixed the localget_optional_sliceshim insrc/runtime/api/csrf_jsc.rswhich had the same bug.Verification
Added a test to
test/js/bun/terminal/terminal.test.tsasserting the exact{code, name, message}shape.