jsc: let get_optional::<JSValue> replace the Terminal and bake get_optional_value shims - #39150
jsc: let get_optional::<JSValue> replace the Terminal and bake get_optional_value shims#39150robobun wants to merge 7 commits into
Conversation
|
Warning Review limit reached
Next review available in: 13 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 (2)
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 (6)
💤 Files with no reviewable changes (1)
WalkthroughThe change adds identity coercion for ChangesTyped optional access
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:07 PM PT - Aug 15th, 2026
🔄 @robobun, the build for your commit |
…tional_value shims Terminal.rs and bake_body.rs each carried a private get_optional_value (get the property, treat undefined/null as absent, hand back the raw JSValue). bun_jsc already has the shared version of this as JSValue::get_optional<T: CoerceTo>, but CoerceTo was only implemented for i32 and i64, so neither site could use it for the untyped case. Implement CoerceTo for JSValue as the identity and switch both files to get_optional::<JSValue>. Terminal's get_optional_i32 was the same code as get_optional::<i32>, so the whole JSValueTerminalExt trait goes away. No behavior change. mordant-baseline.toml loses the reimplemented_helper entry for Terminal.rs, which is what regenerating it produces now.
20b8090 to
e95c234
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical dedup collapsing two local get_optional_value shims onto the shared JSValue::get_optional::<T> via an identity CoerceTo for JSValue impl.
What was reviewed:
- Confirmed
get_optional<T>(JSValue.rs:1532) isget→ dropundefined/null→T::coerce_from, and the newCoerceTo for JSValuereturnsOk(v), soget_optional::<JSValue>is byte-equivalent to both deleted shims. - Confirmed
get_optional_i32was already exactlyget_optional::<i32>(same filter, samecoerce::<i32>dispatch), so the two Terminalcols/rowssites are unchanged in behavior. - The mordant-baseline entry removal only tightens the ratchet; the other bake shims (
get_optional_slice,get_boolean_*,get_function) are correctly left alone since they differ from thebun_jscversions.
Extended reasoning...
Overview
This PR removes two duplicate helpers — the JSValueTerminalExt extension trait in Terminal.rs (with get_optional_value/get_optional_i32) and the free get_optional_value in bake_body.rs — by adding an identity impl CoerceTo for JSValue in src/jsc/JSValue.rs. That makes the existing generic JSValue::get_optional::<T> usable with T = JSValue (returning the property unless absent/undefined/null), which is exactly what both shims did. 5 call sites in Terminal.rs and 8 in bake_body.rs switch to the shared method; one mordant-baseline.toml reimplemented_helper entry is dropped.
Security risks
None. This is pure Rust-side option-parsing plumbing with no change to what values are accepted, how they're validated, or what side effects run. The identity coercion cannot throw or run user JS.
Level of scrutiny
Low. The change is a textbook "move the guard into the shared helper" dedup. I read get_optional at JSValue.rs:1532–1544 and coerce at :834 to confirm the composed behavior is identical to the deleted match arms (get already maps absent/undefined → None; get_optional adds the is_undefined_or_null() filter; coerce::<JSValue> is now Ok(v)). The i32 path was already the same body as CoerceTo for i32::coerce_from composed with the same filter.
Other factors
The PR intentionally leaves the remaining bake shims (get_optional_slice, get_boolean_strict/loose, get_function) in place because they differ semantically from the bun_jsc methods of the same name — I spot-checked get_optional_slice (calls to_slice, coercing) vs bun_jsc's stricter variant and agree. The mordant baseline change is deletion-only, which can never loosen the ratchet. The author reports the terminal and bake test suites pass on a debug build. No prior human review comments to address.
These are the call sites that now read through JSValue::get_optional: null must keep meaning "not set", false must still be a value, and Terminal cols/rows must still be coerced rather than type-checked.
There was a problem hiding this comment.
LGTM — the ConPTY early-resolve concern from the previous pass is addressed in c6a26e8 (the helper now matches and resolves on the full [\d+x\d+] token).
What was reviewed:
get_optional<T>body vs. the deletedget_optional_value/get_optional_i32shims — sameget→ null-filter →coercesequence, so all 13 rewired sites are behavior-identical.- The identity
impl CoerceTo for JSValue— no prior callers ofcoerce::<JSValue>, so no unintended side effects. - New tests:
app-options.test.tsis hermetic (subprocess,port: 0,tempDir);sizeSeenBySubprocessnow keys on a child-unique token like the neighboring tests.
Extended reasoning...
Overview
This PR removes two duplicate local shims (JSValueTerminalExt::{get_optional_value,get_optional_i32} in Terminal.rs and the free get_optional_value in bake_body.rs) by adding an identity impl CoerceTo for JSValue so the existing shared JSValue::get_optional::<T> covers the "return the raw value unless absent/undefined/null" case. Thirteen call sites are rewired to the shared helper, one mordant reimplemented_helper baseline line is dropped, and two tests pin the null/coercion contract at the rewired sites.
I confirmed get_optional at src/jsc/JSValue.rs:1532 does exactly what both shims did: self.get() (already maps absent/undefined → None), then is_undefined_or_null() filter, then coerce::<T>(). With CoerceTo for JSValue as Ok(v), the T = JSValue path is byte-for-byte equivalent to the deleted shims, and T = i32 was already what get_optional_i32 open-coded.
Security risks
None. Option-parsing plumbing only; no auth, crypto, filesystem paths, or untrusted-input validation semantics changed. The identity coercion cannot throw and the null filter is unchanged.
Level of scrutiny
Low-to-medium. This is a mechanical dedup driven by a mordant lint hit; the shared helper already existed and was already used for i32/i64. The only new code is an 8-line trait impl that returns its input. The rewired call sites in bake_body.rs and Terminal.rs are option-parsing paths with existing test coverage plus the new pinning tests.
Other factors
- Prior review addressed: my earlier inline comment flagged
sizeSeenBySubprocessresolving on bare](which ConPTY OSC sequences also contain). Commit c6a26e8 changed the resolve gate tooutput.match(/\[\d+x\d+\]/)and resolves with the matched token directly — the concern no longer applies and the thread is marked resolved. - Test quality: the new
app-options.test.tsfollows harness conventions (tempDir,bunEnv,bunExe, subprocess with piped stdout/stderr drained viaPromise.all,port: 0,server.stop(true)). The terminal tests useawait usingfor cleanup and await the actual observable condition. - No CODEOWNERS gate the touched paths.
- The PR description explicitly enumerates which sibling bake shims (
get_optional_slice,get_function, etc.) were left alone and why — they intentionally differ from thebun_jscmethods of the same name.
|
Status: ready to merge from my side, blocked only on CI bookkeeping.
|
There was a problem hiding this comment.
LGTM — the ConPTY resolve-gate concern from my earlier review is addressed in the current sizeSeenBySubprocess (matches the full /\[\d+x\d+\]/ token and resolves with it).
What was reviewed:
get_optional<T>atsrc/jsc/JSValue.rs:1532— with the identityCoerceTo for JSValue, it is byte-for-byte equivalent to both deletedget_optional_valueshims;get_optional::<i32>was already identical to the deletedget_optional_i32.- All 8 rewired
bake_body.rssites and 5Terminal.rssites — same filter, same coercion, no behavior change. - New tests:
app-options.test.tspins null-as-absent for the rewired bake keys;terminal-spawn.test.tscovers null defaults and string coercion for cols/rows.
Extended reasoning...
Overview
Mechanical dedup that resolves a mordant reimplemented_helper finding. Adds an identity impl CoerceTo for JSValue (8 lines in src/jsc/JSValue.rs) so the existing JSValue::get_optional<T> becomes usable with T = JSValue, then deletes the two local shims that duplicated it: the JSValueTerminalExt trait in Terminal.rs (both get_optional_value and get_optional_i32) and the free get_optional_value in bake_body.rs. 13 call sites are rewired 1:1. One line removed from mordant-baseline.toml. Two test additions pin the null/coercion contract at the rewired sites.
Behavioral equivalence
I read get_optional at JSValue.rs:1532-1544: it does get() → return None on absent/undefined → return None on is_undefined_or_null() → coerce::<T>(). With CoerceTo for JSValue returning Ok(v), this is line-for-line the deleted get_optional_value bodies (match self.get(...)? { Some(v) if !v.is_undefined_or_null() => Ok(Some(v)), _ => Ok(None) }). The deleted get_optional_i32 was already identical to get_optional::<i32> (same filter + coerce::<i32>). Adding a new CoerceTo impl cannot change any existing call site — it only makes T = JSValue compile where it previously would not.
Prior review follow-up
My earlier inline comment flagged that sizeSeenBySubprocess resolved on a bare ], which ConPTY OSC sequences also contain. The current diff gates on output.match(/\[\d+x\d+\]/) and resolves with seen[0], so the promise cannot settle before the child's payload arrives. That thread is resolved and the fix matches what I recommended.
Security risks
None. Option-parsing helper consolidation; no new inputs accepted, no validation loosened, no auth/crypto/permission code touched.
Level of scrutiny
Low-to-moderate. The Rust change is a pure refactor with a trivially-correct identity impl and mechanical call-site rewires; the deleted code and its replacement are provably equivalent by reading the shared helper. Test additions follow harness conventions (tempDir, bunEnv, drain pipes concurrently, port: 0, await using). CI on the pre-merge commit had 177/179 jobs pass with the remaining two canceled (not failed).
Other factors
No outstanding reviewer comments. The PR description explicitly scopes out the other bake shims (get_optional_slice, get_function) as intentionally different from their bun_jsc namesakes, which I confirmed (bun_jsc's get_optional_slice throws on non-string; bake's coerces).
There was a problem hiding this comment.
LGTM — the earlier ConPTY ]-matching concern was addressed in c6a26e8, and the remaining inline note is a diagnosability nit that matches the file's existing convention.
Extended reasoning...
Overview
This PR adds an identity impl CoerceTo for JSValue in src/jsc/JSValue.rs, then deletes two local shims that reimplemented the same filter-then-return pattern: the JSValueTerminalExt trait (get_optional_value + get_optional_i32) in Terminal.rs and the free get_optional_value in bake_body.rs. All 13 call sites are rewired to get_optional::<JSValue> / get_optional::<i32>. One line is removed from mordant-baseline.toml. Two test additions pin the null/coercion contract at the rewired sites.
Behavior preservation
I compared the shared get_optional<T> body (src/jsc/JSValue.rs:1532-1544) against both deleted shims. get_optional::<JSValue> is byte-for-byte equivalent to get_optional_value (get → filter is_undefined_or_null → return); get_optional::<i32> is byte-for-byte equivalent to get_optional_i32 (same filter, then coerce::<i32>). The identity CoerceTo impl is Ok(v), so no coercion side effect is introduced. No behavior change.
Security risks
None. Option-parsing dedup only; no new user-controlled paths, no auth/crypto/permission surface.
Level of scrutiny
Low-to-moderate: a mechanical helper consolidation driven by a mordant lint. The prior review's ConPTY early-resolve issue was fixed (the helper now matches the full [\d+x\d+] token and resolves with it). The current run's only finding is a nit that sizeSeenBySubprocess doesn't wire exit → reject; this matches the neighboring tests in the same file and the failure mode is still a harness-timeout failure, not a silent pass.
Other factors
The new test/bake/app-options.test.ts covers the null-as-absent contract at every rewired bake site including the separateSSRGraph boolean-strict path, and the terminal test covers both get_optional::<i32> sites. Other bake shims (get_optional_slice, get_function, get_boolean_*) are correctly left in place since they differ from the bun_jsc methods of the same name.
There was a problem hiding this comment.
LGTM — the refactor is mechanically behavior-preserving. The inline nit about the exit() callback in sizeSeenBySubprocess being unreachable on the pre-created-terminal path is real but non-blocking (worst case remains a harness-timeout failure, and it matches the file's dominant pattern); fine to address as a follow-up or drop.
Extended reasoning...
Overview
This PR adds impl CoerceTo for JSValue (identity) so JSValue::get_optional::<JSValue> becomes usable, then deletes two duplicate local shims — JSValueTerminalExt in Terminal.rs (both get_optional_value and get_optional_i32) and the free get_optional_value in bake_body.rs — rewiring their 5 + 8 call sites onto the shared inherent method. The mordant baseline drops the corresponding reimplemented_helper entry. Two test additions pin the null/coercion contract at the rewired sites.
Behavior equivalence
I compared the removed shims against get_optional<T> at src/jsc/JSValue.rs:1532: both are get() → filter is_undefined_or_null → coerce::<T>, and with the new identity CoerceTo for JSValue the T = JSValue case returns the value unchanged, while T = i32 runs the same coerce::<i32> the old get_optional_i32 called. No semantic drift. The other bake shims (get_optional_slice, get_boolean_*, get_function) are correctly left alone since they diverge from the shared helpers.
Security risks
None. This is option-parsing plumbing with no auth/crypto/untrusted-input surface change; the same values reach the same downstream validators.
Level of scrutiny
Low-to-medium: a mechanical dedup that a lint (reimplemented_helper) flagged, with the shared helper's body verified byte-equivalent. The added tests (app-options.test.ts covering null/0/false for the rewired bake keys; two terminal size tests for null and string coercion on cols/rows) directly exercise the filter/coerce edges.
Other factors
I previously flagged two test issues on this PR; both were addressed. The remaining inline nit points out that 4ca91e1's exit() reject is unreachable on the existing_terminal (non-OWNS_TERMINAL) path, so the diagnosability improvement doesn't actually land. That's accurate but explicitly nit-level: the failure scenario (a bunExe() one-liner crashing on a real PTY) is implausible, several neighboring tests in this file share the same hang-on-failure shape, and the worst case is still a test failure via harness timeout — never a silent pass. It does not affect the refactor's correctness or the tests' ability to catch a regression in get_optional's null/coercion semantics.
…n fire A terminal created up front and passed to Bun.spawn stays open after the child exits, and `await using` suppresses the callback on close, so the exit -> reject wiring never ran. With the inline option the subprocess owns the terminal and the reader reaches EOF once the child is gone.
There was a problem hiding this comment.
LGTM — 9060c66 switches the helper to an inline terminal: {...} option so OWNS_TERMINAL is set and the exit callback is now reachable, addressing the last open comment.
What was reviewed:
- Confirmed
get_optional<T>'s body (get → filter undefined/null →coerce::<T>) is byte-for-byte equivalent to both removed shims; the newCoerceTo for JSValueidentity impl has no other consumers. - Inline-terminal spawn path still routes through
TerminalOptions::parse_from_js(js_bun_spawn_bindings.rs:822), so the rewiredcols/rowsreads are still exercised by the new tests. - Happy-path ordering:
datafires beforeexiton both POSIX (drain_and_close_slave_fdreads before closing) and Windows (reader stays ref'd until conhost EOF), so the reject-on-exit is a no-op on a settled promise, not a spurious failure.
Extended reasoning...
Overview
Mechanical dedupe of two local get_optional_value shims (Terminal.rs, bake_body.rs) into the shared JSValue::get_optional::<T> by adding an identity impl CoerceTo for JSValue. Eight call sites in bake_body.rs and five in Terminal.rs are rewired; the JSValueTerminalExt trait and the bake free function are deleted. One line drops from mordant-baseline.toml. Two test additions pin the null/coercion contract at the rewired sites.
Security risks
None. Option-parsing refactor with no change to what values are accepted or how they're validated; the new trait impl is a pure identity.
Level of scrutiny
Low-medium. The Rust change is a mechanical substitution — I diffed get_optional's body against both removed shims and they're equivalent (get → drop undefined/null → coerce, where coerce for JSValue is now identity). The only real review surface was the new test helper, which went through three iterations of feedback (ConPTY ] matching, exit-wired reject, inline vs. pre-created terminal for OWNS_TERMINAL); all are now resolved in 9060c66.
Other factors
All three prior inline comments from this review are addressed and the threads resolved. The bug hunter found nothing new on this revision. I verified the inline-terminal spawn path still calls Options::parse_from_js so the test still covers the changed code, and traced the happy-path callback ordering to confirm the exit-reject on an already-resolved promise is harmless. CoerceTo has no other implementors that could be affected.
2 participants
Problem
reimplemented_helperonsrc/runtime/api/bun/Terminal.rs:<JSValue as JSValueTerminalExt>::get_optional_valueandbake_body::get_optional_valuehave the same signature and body (get the property, treatundefined/nullas absent, return the rawJSValue). A fix to one would miss the other.JSValue::get_optional<T: CoerceTo>insrc/jsc/JSValue.rsdoes exactly this and then runs the value throughcoerce::<T>(). Both shims describe themselves as stand-ins for it. It was unusable here only becauseCoerceTois implemented fori32andi64and nothing else.Fix
impl CoerceTo for JSValueas the identity, soget_optional::<JSValue>(global, name)returns the property unless it is absent,undefinedornull. Same filter as both shims (get()already maps absent andundefinedtoNone;get_optionaladds thenullcheck).bake_body.rs: the free function is deleted and its 8 call sites useget_optional::<JSValue>.Terminal.rs: theJSValueTerminalExttrait is deleted.get_optional_valuegoes the same way; its other method,get_optional_i32, was line for lineget_optional::<i32>(filter, thencoerce::<i32>), so those 2 call sites switch too instead of leaving a one-method trait behind.get_optional_slice,get_function, ...) intentionally differ from thebun_jscmethods of the same name (they coerce or filter wherebun_jscthrows) and are left alone.mordant-baseline.toml: regenerating withbun run rust:mordant:baselineon this branch produces current main's file minus thereimplemented_helper:src/runtime/api/bun/Terminal.rsline, so that is the only baseline change here.nulland coercion semantics at these sites, which previously had no direct coverage:test/bake/app-options.test.ts(new; no existing file coversappoption parsing):nullforbundlerOptions, itsserver/client/ssr,client.sourcemap,client.minifyandframework.pluginsis accepted as "not set",separateSSRGraph: nullreports the key as missing while0is rejected as a non-boolean andfalseis consumed as a value, andsourcemap: 0is still rejected. Thesourcemap: nullandseparateSSRGraph: nullrows are the ones that would change if the helper stopped filteringnull.test/js/bun/terminal/terminal-spawn.test.ts:cols/rowsofnullleave the 80x24 default and numeric strings are coerced (theget_optional::<i32>path), checked through the size the subprocess sees.test/js/bun/terminal/,test/bake/dev/production.test.ts(bundlerOptionswith real values),test/bake/dev/plugins.test.tsandtest/bake/dev/bundle.test.ts(separateSSRGraphtrue/false), all passing.cargo fmt --all -- --checkclean.Background
JSValue::getreturnsOk(None)when the property is missing orundefined, otherwiseOk(Some(value));nullcomes back as a value. Option parsing code usually wantsnulltreated like missing, which is what these helpers add.CoerceTois the dispatch trait behindJSValue::coerce::<T>();get_optional<T>is "get, dropundefined/null, thencoerce::<T>". With the identity impl,T = JSValueis the no-coercion case, mirroring the ZiggetOptional(.., JSValue)these shims were ported from.mordant-baseline.tomlholds per-(lint, file) counts of pre-existing findings and a PR fails only if it adds one, so fixing a site means regenerating the file to delete its entry.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/terminal/terminal-spawn.test.ts