jsc: let get_optional::<JSValue> replace the Terminal and bake get_optional_value shims - #39150
Closed
robobun wants to merge 7 commits into
Closed
jsc: let get_optional::<JSValue> replace the Terminal and bake get_optional_value shims#39150robobun wants to merge 7 commits into
robobun wants to merge 7 commits into
Claude / Claude Code Review
completed
Aug 15, 2026 in 22m 46s
Code review found 1 potential issue
Found 2 candidates, confirmed 1. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 1 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | test/js/bun/terminal/terminal-spawn.test.ts:283-285 |
exit() callback in sizeSeenBySubprocess is dead code — 4ca91e1's diagnosability fix does not work |
Annotations
Check warning on line 285 in test/js/bun/terminal/terminal-spawn.test.ts
claude / Claude Code Review
exit() callback in sizeSeenBySubprocess is dead code — 4ca91e1's diagnosability fix does not work
The `exit()` callback added in 4ca91e1 never fires here: `sizeSeenBySubprocess` passes a pre-created `new Bun.Terminal` (the `existing_terminal` path, no `OWNS_TERMINAL`), so on child exit the parent still holds `slave_fd` / the ConDrv `\Reference` handle, the master reader never sees EOF, and `on_reader_finished` — the only path to `call_exit_callback` — is not reached; on the happy path `await using` sets `FINALIZED` first and the callback is skipped there too. The earlier review's diagnosabil
Loading