fix: recover main's red CI gates + perry check false pass - #6891
fix: recover main's red CI gates + perry check false pass#6891proggeramlug wants to merge 3 commits into
perry check false pass#6891Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthroughThis PR extracts object spill storage and readline pumping into dedicated modules, fixes parse-error reporting, updates readline behavior and GC handling, adjusts parity and regression tests, and documents Linux toolchain prerequisites. ChangesObject overflow storage
Readline pump extraction
Verification and prerequisites
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant stdin_reader
participant readline_pump
participant callbacks
stdin_reader->>readline_pump: queue bytes or lines
readline_pump->>readline_pump: reassemble escape sequences
readline_pump->>callbacks: dispatch data, keypress, line, or close
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (7)
crates/perry-runtime/src/object/spill.rs (1)
122-142: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove the learned-inline-fields hook off the hot write path.
note_learned_inline_fieldsruns on everyspill_setcall (including the in-capacity fast path at Lines 128-138), which forces athread_localaccess exactly on the path this module was built to keep TLS-free (per the doc at Lines 52-58 calling out TLS probes as "the hot leaves of round-robin overflow writes"). Since a new high-water field index can only be set the first time it exceeds current capacity, gating the call tospill_set_slowpreserves identical learned-maximum semantics while removing the TLS touch from every subsequent hot-path write.⚡ Proposed fix
pub(crate) fn spill_set(obj_ptr: usize, field_index: usize, vbits: u64) { unsafe { let obj = obj_ptr as *mut ObjectHeader; - // Learn the class's true width so FUTURE instances allocate it - // inline (same hook as the legacy path). - note_learned_inline_fields((*obj).class_id, (field_index as u32).saturating_add(1)); // Hot path: meta and buffer already exist with capacity — the // in-range barriered store cannot allocate or move anything, so no // handle scope is needed. This is every write after the first to a // given width (e.g. round-robin updates across an object array). let meta = (*obj).meta;fn spill_set_slow(obj_ptr: usize, field_index: usize, vbits: u64) { unsafe { let obj = obj_ptr as *mut ObjectHeader; + // Learn the class's true width so FUTURE instances allocate it + // inline (same hook as the legacy path). Only needs to run here: + // any field_index that sets a new high-water mark must first + // exceed current capacity, which always routes through this path. + note_learned_inline_fields((*obj).class_id, (field_index as u32).saturating_add(1)); // Root the owner: meta/buffer allocation below can trigger a moving // minor GC. Reload through the handle after every allocation. let scope = crate::gc::RuntimeHandleScope::new();🤖 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 `@crates/perry-runtime/src/object/spill.rs` around lines 122 - 142, Move the note_learned_inline_fields call out of spill_set’s entry path and into spill_set_slow, after confirming the write requires slow-path growth. Keep the existing high-water update semantics for first-time capacity expansion while ensuring in-capacity writes return without any TLS access.crates/perry-stdlib/src/readline/mod.rs (4)
1002-1002: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
SAVED.lock().unwrap()panics on a poisoned mutex.Every other lock site in this file recovers (
unwrap_or_else(|p| p.into_inner())/if let Ok). If a panic ever unwinds whileSAVEDis held,setRawModeaborts the process instead of degrading. Useunwrap_or_else(|p| p.into_inner())here too.Also applies to: 1030-1030, 1091-1091, 1113-1113
🤖 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 `@crates/perry-stdlib/src/readline/mod.rs` at line 1002, Update every SAVED mutex lock in setRawMode and the additional affected sites to recover from poisoning with unwrap_or_else(|p| p.into_inner()) instead of unwrap(). Preserve the existing lock usage and behavior otherwise.
523-542: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winInterface slots are never reclaimed.
NEXT_READLINE_HANDLEincrements forever andclose_custom_interfaceonly setsclosed = true, so theREADLINE_INTERFACESvector grows monotonically and every closed interface's state (plus its rootedinput/output/callbacks) is kept alive by the GC scanner for the process lifetime. A long-running program that creates one interface per child process leaks steadily. Consider setting the slot toNoneon close and reusing free indices.🤖 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 `@crates/perry-stdlib/src/readline/mod.rs` around lines 523 - 542, The readline interface lifecycle must reclaim closed custom-interface slots instead of retaining their state indefinitely. Update close_custom_interface to remove the closed interface from READLINE_INTERFACES, and update allocate_interface to reuse available None slots before incrementing NEXT_READLINE_HANDLE; preserve the fixed STDIN_READLINE_HANDLE behavior.
1825-1886: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests share process-global state with a real reader thread;
reset()can't undo that.
READER_STARTEDis sticky and several tests (raw_mode_toggle_flips_atomic,stdin_pause_resume_gates_data_dispatchviajs_readline_stdin_resume) actually spawn the background thread, which then blocks on the runner's stdin and flipsEOF_REACHEDat arbitrary points — the comment at Line 1973 already documents having to work around it. Consider gatingensure_reader_startedbehind a test-only flag so the suite never spawns a real stdin reader.🤖 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 `@crates/perry-stdlib/src/readline/mod.rs` around lines 1825 - 1886, Prevent tests from spawning the real stdin reader by adding a test-only gate checked by ensure_reader_started and disabling reader startup while the test suite runs. Update reset() or the test setup to enable the mocked/test path instead, preserving existing state-reset behavior and avoiding background changes to EOF_REACHED.
906-956: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftPer-byte blocking reads and an unbounded pending queue.
reader.read(&mut byte)with a 1-byte buffer issues a syscall per keystroke/byte; for piped input (cat big.txt | app) that is one syscall and oneVecpush per byte. Combined withPENDING_DATA/PENDING_LINEShaving no cap, a paused or destroyed-but-still-blocked consumer lets the queue grow to the size of the whole input stream. Consider a buffered read (e.g.BufReader+readinto a 4 KiB buffer, splitting on\nin cooked mode) and a bounded queue with backpressure.🤖 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 `@crates/perry-stdlib/src/readline/mod.rs` around lines 906 - 956, Update the stdin reader thread around PENDING_DATA and PENDING_LINES to use a buffered reader and a multi-byte buffer instead of calling reader.read with a one-byte buffer; preserve raw-mode byte ordering and cooked-mode newline/CRLF behavior while processing each buffered chunk. Bound both pending queues and apply backpressure when they are full, including avoiding unbounded line_buf growth, so paused or destroyed consumers cannot retain input indefinitely.crates/perry-stdlib/src/readline/pump.rs (2)
22-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
if cond { JSValue::bool(true) } else { JSValue::bool(false) }→JSValue::bool(cond).♻️ Proposed refactor
- js_object_set_field( - obj, - 1, - if ctrl { - JSValue::bool(true) - } else { - JSValue::bool(false) - }, - ); - js_object_set_field( - obj, - 2, - if shift { - JSValue::bool(true) - } else { - JSValue::bool(false) - }, - ); - js_object_set_field( - obj, - 3, - if meta { - JSValue::bool(true) - } else { - JSValue::bool(false) - }, - ); + js_object_set_field(obj, 1, JSValue::bool(ctrl)); + js_object_set_field(obj, 2, JSValue::bool(shift)); + js_object_set_field(obj, 3, JSValue::bool(meta));🤖 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 `@crates/perry-stdlib/src/readline/pump.rs` around lines 22 - 48, In the object field assignments, simplify the boolean conversions in the relevant function by passing ctrl, shift, and meta directly to JSValue::bool instead of using redundant if/else expressions. Preserve the existing field indices and values.
165-189: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRe-locking and re-parsing inside the per-chunk loop.
DATA_CALLBACKSandKEYPRESS_CALLBACKSare cloned under lock once per chunk, andparse_keypress(which allocates twoStrings) runs once per callback rather than once per chunk. Hoist both out of the loop.♻️ Proposed refactor
+ let data_callbacks = DATA_CALLBACKS.lock().map(|v| v.clone()).unwrap_or_default(); + let keypress_callbacks = KEYPRESS_CALLBACKS + .lock() + .map(|v| v.clone()) + .unwrap_or_default(); for chunk in chunks { - // 'data' callback receives the raw bytes as a string. - let data_callbacks = DATA_CALLBACKS.lock().map(|v| v.clone()).unwrap_or_default(); - for cb_i64 in data_callbacks { + for cb_i64 in &data_callbacks { let arg = stdin_chunk_value(&chunk); - let closure = cb_i64 as *const ClosureHeader; - js_closure_call1(closure, arg); + js_closure_call1(*cb_i64 as *const ClosureHeader, arg); fired += 1; } - // 'keypress' callback receives (sequence_string, key_object). - let keypress_callbacks = KEYPRESS_CALLBACKS - .lock() - .map(|v| v.clone()) - .unwrap_or_default(); - for cb_i64 in keypress_callbacks { - if let Some((name, ctrl, shift, meta, seq)) = parse_keypress(&chunk) { + if let Some((name, ctrl, shift, meta, seq)) = parse_keypress(&chunk) { + for cb_i64 in &keypress_callbacks { let seq_str = js_string_from_bytes(seq.as_ptr(), seq.len() as u32); let arg1 = f64::from_bits(JSValue::string_ptr(seq_str).bits()); let arg2 = build_keypress_object(&name, ctrl, shift, meta, &seq); - let closure = cb_i64 as *const ClosureHeader; - js_closure_call2(closure, arg1, arg2); + js_closure_call2(*cb_i64 as *const ClosureHeader, arg1, arg2); fired += 1; } } }🤖 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 `@crates/perry-stdlib/src/readline/pump.rs` around lines 165 - 189, Update the pump callback dispatch flow to clone DATA_CALLBACKS and KEYPRESS_CALLBACKS once before iterating chunks, and parse each chunk’s keypress data once before invoking callbacks. Reuse the per-chunk parsed keypress result for all KEYPRESS_CALLBACKS while preserving the existing data and keypress callback arguments and fired count.
🤖 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 `@crates/perry-stdlib/src/readline/mod.rs`:
- Around line 1269-1287: Update js_readline_pause and js_readline_resume to
branch on the interface state’s uses_custom_stream flag: preserve forwarding to
node_stream pause/resume for custom streams, and for stdin-backed interfaces set
or clear the shared STDIN_PAUSED state so line delivery stops and resumes.
- Around line 1247-1267: Update js_readline_close on the stdin path to clear
both LINE_CALLBACK and QUESTION_CALLBACK when setting EOF_REACHED. Preserve the
existing custom-stream handling and close callback behavior, ensuring subsequent
has_line_callbacks and js_readline_process_pending calls cannot emit late line
events after closure.
- Around line 772-796: The async iterator construction in
crates/perry-stdlib/src/readline/mod.rs:772-796 must root obj with
RuntimeHandleScope and reload it after every allocating call before field writes
and iter_val creation; preserve the existing NaN-boxed value handling and
symbol-property setup. In crates/perry-stdlib/src/readline/pump.rs:16-21, move
the name_str and seq_str allocations before js_object_alloc_with_shape, or root
and reload obj across those string allocations.
- Around line 811-846: In attach_custom_input, keep the data and close closure
allocations rooted across subsequent js_closure_alloc and boxed_str calls, using
the runtime’s established rooting mechanism before constructing data_value and
close_value. Ensure both closure values remain valid until registration through
js_node_stream_method_on or js_native_call_value completes.
- Around line 1381-1399: Register the GC scanner before any root-bearing state
is stored: update js_readline_stdin_on to call ensure_gc_scanner_registered() at
entry, and update js_readline_create_interface plus js_readline_iterator so
state.input/state.output are covered even when question or on is unused. Apply
the changes at crates/perry-stdlib/src/readline/mod.rs lines 1381-1399 and
1155-1164, preserving the existing NaN-boxed value representation and registered
module-global root invariants.
- Around line 1515-1520: Update destroy() to also lock and clear
READABLE_CALLBACKS alongside DATA_CALLBACKS and KEYPRESS_CALLBACKS, ensuring no
readable callbacks remain active after destruction.
- Around line 1313-1320: Update js_readline_write to send the converted text
through call_write_value using state.output, matching the output behavior in
js_readline_prompt, while preserving the existing cursor_cols update and return
value.
- Around line 1533-1805: Remove build_keypress_object, parse_keypress,
js_readline_process_pending, and js_readline_has_active from readline/mod.rs,
then declare the pump module and re-export its public symbols from mod.rs.
Ensure the existing readline::* export path continues exposing these APIs while
keeping their single implementations in pump.rs and bringing mod.rs below the
2,000-line limit.
- Around line 635-645: Update the non-async dispatch path around
with_interface_mut and fire_line_or_question: borrow the interface only long
enough to extract or clone the callbacks/state required for delivery, then
release that borrow before invoking any callbacks. Preserve the existing line
ordering and async_iter behavior, while ensuring fire_line_or_question and any
nested JS execution run after the READLINE_INTERFACES borrow ends.
In `@crates/perry-stdlib/src/readline/pump.rs`:
- Around line 54-58: Update js_readline_process_pending and the related
parse_keypress flow to reassemble ESC-prefixed ANSI sequences across separate
byte chunks before decoding, so \x1b, [, A produces one up keypress. Remove or
correct the parse_keypress documentation’s reference to a nonexistent
pending_escape accumulator, and add an end-to-end test covering the three-chunk
input and single up result.
- Around line 155-163: Update js_readline_process_pending so READABLE_CALLBACKS
are invoked only when newly read chunks are available, or once when EOF is
reached, rather than on every event-loop tick. Ensure paused or destroyed stdin
states do not dispatch readable callbacks, and prevent the fired count from
keeping the loop active when no eligible readable event occurred.
---
Nitpick comments:
In `@crates/perry-runtime/src/object/spill.rs`:
- Around line 122-142: Move the note_learned_inline_fields call out of
spill_set’s entry path and into spill_set_slow, after confirming the write
requires slow-path growth. Keep the existing high-water update semantics for
first-time capacity expansion while ensuring in-capacity writes return without
any TLS access.
In `@crates/perry-stdlib/src/readline/mod.rs`:
- Line 1002: Update every SAVED mutex lock in setRawMode and the additional
affected sites to recover from poisoning with unwrap_or_else(|p| p.into_inner())
instead of unwrap(). Preserve the existing lock usage and behavior otherwise.
- Around line 523-542: The readline interface lifecycle must reclaim closed
custom-interface slots instead of retaining their state indefinitely. Update
close_custom_interface to remove the closed interface from READLINE_INTERFACES,
and update allocate_interface to reuse available None slots before incrementing
NEXT_READLINE_HANDLE; preserve the fixed STDIN_READLINE_HANDLE behavior.
- Around line 1825-1886: Prevent tests from spawning the real stdin reader by
adding a test-only gate checked by ensure_reader_started and disabling reader
startup while the test suite runs. Update reset() or the test setup to enable
the mocked/test path instead, preserving existing state-reset behavior and
avoiding background changes to EOF_REACHED.
- Around line 906-956: Update the stdin reader thread around PENDING_DATA and
PENDING_LINES to use a buffered reader and a multi-byte buffer instead of
calling reader.read with a one-byte buffer; preserve raw-mode byte ordering and
cooked-mode newline/CRLF behavior while processing each buffered chunk. Bound
both pending queues and apply backpressure when they are full, including
avoiding unbounded line_buf growth, so paused or destroyed consumers cannot
retain input indefinitely.
In `@crates/perry-stdlib/src/readline/pump.rs`:
- Around line 22-48: In the object field assignments, simplify the boolean
conversions in the relevant function by passing ctrl, shift, and meta directly
to JSValue::bool instead of using redundant if/else expressions. Preserve the
existing field indices and values.
- Around line 165-189: Update the pump callback dispatch flow to clone
DATA_CALLBACKS and KEYPRESS_CALLBACKS once before iterating chunks, and parse
each chunk’s keypress data once before invoking callbacks. Reuse the per-chunk
parsed keypress result for all KEYPRESS_CALLBACKS while preserving the existing
data and keypress callback arguments and fired count.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 15ea0ba2-9fe8-4823-9414-c279ddf12977
📒 Files selected for processing (4)
changelog.d/6891-linux-verification-sweep.mdcrates/perry-runtime/src/object/spill.rscrates/perry-stdlib/src/readline/mod.rscrates/perry-stdlib/src/readline/pump.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 11
🧹 Nitpick comments (7)
crates/perry-runtime/src/object/spill.rs (1)
122-142: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove the learned-inline-fields hook off the hot write path.
note_learned_inline_fieldsruns on everyspill_setcall (including the in-capacity fast path at Lines 128-138), which forces athread_localaccess exactly on the path this module was built to keep TLS-free (per the doc at Lines 52-58 calling out TLS probes as "the hot leaves of round-robin overflow writes"). Since a new high-water field index can only be set the first time it exceeds current capacity, gating the call tospill_set_slowpreserves identical learned-maximum semantics while removing the TLS touch from every subsequent hot-path write.⚡ Proposed fix
pub(crate) fn spill_set(obj_ptr: usize, field_index: usize, vbits: u64) { unsafe { let obj = obj_ptr as *mut ObjectHeader; - // Learn the class's true width so FUTURE instances allocate it - // inline (same hook as the legacy path). - note_learned_inline_fields((*obj).class_id, (field_index as u32).saturating_add(1)); // Hot path: meta and buffer already exist with capacity — the // in-range barriered store cannot allocate or move anything, so no // handle scope is needed. This is every write after the first to a // given width (e.g. round-robin updates across an object array). let meta = (*obj).meta;fn spill_set_slow(obj_ptr: usize, field_index: usize, vbits: u64) { unsafe { let obj = obj_ptr as *mut ObjectHeader; + // Learn the class's true width so FUTURE instances allocate it + // inline (same hook as the legacy path). Only needs to run here: + // any field_index that sets a new high-water mark must first + // exceed current capacity, which always routes through this path. + note_learned_inline_fields((*obj).class_id, (field_index as u32).saturating_add(1)); // Root the owner: meta/buffer allocation below can trigger a moving // minor GC. Reload through the handle after every allocation. let scope = crate::gc::RuntimeHandleScope::new();🤖 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 `@crates/perry-runtime/src/object/spill.rs` around lines 122 - 142, Move the note_learned_inline_fields call out of spill_set’s entry path and into spill_set_slow, after confirming the write requires slow-path growth. Keep the existing high-water update semantics for first-time capacity expansion while ensuring in-capacity writes return without any TLS access.crates/perry-stdlib/src/readline/mod.rs (4)
1002-1002: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
SAVED.lock().unwrap()panics on a poisoned mutex.Every other lock site in this file recovers (
unwrap_or_else(|p| p.into_inner())/if let Ok). If a panic ever unwinds whileSAVEDis held,setRawModeaborts the process instead of degrading. Useunwrap_or_else(|p| p.into_inner())here too.Also applies to: 1030-1030, 1091-1091, 1113-1113
🤖 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 `@crates/perry-stdlib/src/readline/mod.rs` at line 1002, Update every SAVED mutex lock in setRawMode and the additional affected sites to recover from poisoning with unwrap_or_else(|p| p.into_inner()) instead of unwrap(). Preserve the existing lock usage and behavior otherwise.
523-542: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winInterface slots are never reclaimed.
NEXT_READLINE_HANDLEincrements forever andclose_custom_interfaceonly setsclosed = true, so theREADLINE_INTERFACESvector grows monotonically and every closed interface's state (plus its rootedinput/output/callbacks) is kept alive by the GC scanner for the process lifetime. A long-running program that creates one interface per child process leaks steadily. Consider setting the slot toNoneon close and reusing free indices.🤖 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 `@crates/perry-stdlib/src/readline/mod.rs` around lines 523 - 542, The readline interface lifecycle must reclaim closed custom-interface slots instead of retaining their state indefinitely. Update close_custom_interface to remove the closed interface from READLINE_INTERFACES, and update allocate_interface to reuse available None slots before incrementing NEXT_READLINE_HANDLE; preserve the fixed STDIN_READLINE_HANDLE behavior.
1825-1886: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests share process-global state with a real reader thread;
reset()can't undo that.
READER_STARTEDis sticky and several tests (raw_mode_toggle_flips_atomic,stdin_pause_resume_gates_data_dispatchviajs_readline_stdin_resume) actually spawn the background thread, which then blocks on the runner's stdin and flipsEOF_REACHEDat arbitrary points — the comment at Line 1973 already documents having to work around it. Consider gatingensure_reader_startedbehind a test-only flag so the suite never spawns a real stdin reader.🤖 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 `@crates/perry-stdlib/src/readline/mod.rs` around lines 1825 - 1886, Prevent tests from spawning the real stdin reader by adding a test-only gate checked by ensure_reader_started and disabling reader startup while the test suite runs. Update reset() or the test setup to enable the mocked/test path instead, preserving existing state-reset behavior and avoiding background changes to EOF_REACHED.
906-956: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftPer-byte blocking reads and an unbounded pending queue.
reader.read(&mut byte)with a 1-byte buffer issues a syscall per keystroke/byte; for piped input (cat big.txt | app) that is one syscall and oneVecpush per byte. Combined withPENDING_DATA/PENDING_LINEShaving no cap, a paused or destroyed-but-still-blocked consumer lets the queue grow to the size of the whole input stream. Consider a buffered read (e.g.BufReader+readinto a 4 KiB buffer, splitting on\nin cooked mode) and a bounded queue with backpressure.🤖 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 `@crates/perry-stdlib/src/readline/mod.rs` around lines 906 - 956, Update the stdin reader thread around PENDING_DATA and PENDING_LINES to use a buffered reader and a multi-byte buffer instead of calling reader.read with a one-byte buffer; preserve raw-mode byte ordering and cooked-mode newline/CRLF behavior while processing each buffered chunk. Bound both pending queues and apply backpressure when they are full, including avoiding unbounded line_buf growth, so paused or destroyed consumers cannot retain input indefinitely.crates/perry-stdlib/src/readline/pump.rs (2)
22-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
if cond { JSValue::bool(true) } else { JSValue::bool(false) }→JSValue::bool(cond).♻️ Proposed refactor
- js_object_set_field( - obj, - 1, - if ctrl { - JSValue::bool(true) - } else { - JSValue::bool(false) - }, - ); - js_object_set_field( - obj, - 2, - if shift { - JSValue::bool(true) - } else { - JSValue::bool(false) - }, - ); - js_object_set_field( - obj, - 3, - if meta { - JSValue::bool(true) - } else { - JSValue::bool(false) - }, - ); + js_object_set_field(obj, 1, JSValue::bool(ctrl)); + js_object_set_field(obj, 2, JSValue::bool(shift)); + js_object_set_field(obj, 3, JSValue::bool(meta));🤖 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 `@crates/perry-stdlib/src/readline/pump.rs` around lines 22 - 48, In the object field assignments, simplify the boolean conversions in the relevant function by passing ctrl, shift, and meta directly to JSValue::bool instead of using redundant if/else expressions. Preserve the existing field indices and values.
165-189: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRe-locking and re-parsing inside the per-chunk loop.
DATA_CALLBACKSandKEYPRESS_CALLBACKSare cloned under lock once per chunk, andparse_keypress(which allocates twoStrings) runs once per callback rather than once per chunk. Hoist both out of the loop.♻️ Proposed refactor
+ let data_callbacks = DATA_CALLBACKS.lock().map(|v| v.clone()).unwrap_or_default(); + let keypress_callbacks = KEYPRESS_CALLBACKS + .lock() + .map(|v| v.clone()) + .unwrap_or_default(); for chunk in chunks { - // 'data' callback receives the raw bytes as a string. - let data_callbacks = DATA_CALLBACKS.lock().map(|v| v.clone()).unwrap_or_default(); - for cb_i64 in data_callbacks { + for cb_i64 in &data_callbacks { let arg = stdin_chunk_value(&chunk); - let closure = cb_i64 as *const ClosureHeader; - js_closure_call1(closure, arg); + js_closure_call1(*cb_i64 as *const ClosureHeader, arg); fired += 1; } - // 'keypress' callback receives (sequence_string, key_object). - let keypress_callbacks = KEYPRESS_CALLBACKS - .lock() - .map(|v| v.clone()) - .unwrap_or_default(); - for cb_i64 in keypress_callbacks { - if let Some((name, ctrl, shift, meta, seq)) = parse_keypress(&chunk) { + if let Some((name, ctrl, shift, meta, seq)) = parse_keypress(&chunk) { + for cb_i64 in &keypress_callbacks { let seq_str = js_string_from_bytes(seq.as_ptr(), seq.len() as u32); let arg1 = f64::from_bits(JSValue::string_ptr(seq_str).bits()); let arg2 = build_keypress_object(&name, ctrl, shift, meta, &seq); - let closure = cb_i64 as *const ClosureHeader; - js_closure_call2(closure, arg1, arg2); + js_closure_call2(*cb_i64 as *const ClosureHeader, arg1, arg2); fired += 1; } } }🤖 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 `@crates/perry-stdlib/src/readline/pump.rs` around lines 165 - 189, Update the pump callback dispatch flow to clone DATA_CALLBACKS and KEYPRESS_CALLBACKS once before iterating chunks, and parse each chunk’s keypress data once before invoking callbacks. Reuse the per-chunk parsed keypress result for all KEYPRESS_CALLBACKS while preserving the existing data and keypress callback arguments and fired count.
🤖 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 `@crates/perry-stdlib/src/readline/mod.rs`:
- Around line 1269-1287: Update js_readline_pause and js_readline_resume to
branch on the interface state’s uses_custom_stream flag: preserve forwarding to
node_stream pause/resume for custom streams, and for stdin-backed interfaces set
or clear the shared STDIN_PAUSED state so line delivery stops and resumes.
- Around line 1247-1267: Update js_readline_close on the stdin path to clear
both LINE_CALLBACK and QUESTION_CALLBACK when setting EOF_REACHED. Preserve the
existing custom-stream handling and close callback behavior, ensuring subsequent
has_line_callbacks and js_readline_process_pending calls cannot emit late line
events after closure.
- Around line 772-796: The async iterator construction in
crates/perry-stdlib/src/readline/mod.rs:772-796 must root obj with
RuntimeHandleScope and reload it after every allocating call before field writes
and iter_val creation; preserve the existing NaN-boxed value handling and
symbol-property setup. In crates/perry-stdlib/src/readline/pump.rs:16-21, move
the name_str and seq_str allocations before js_object_alloc_with_shape, or root
and reload obj across those string allocations.
- Around line 811-846: In attach_custom_input, keep the data and close closure
allocations rooted across subsequent js_closure_alloc and boxed_str calls, using
the runtime’s established rooting mechanism before constructing data_value and
close_value. Ensure both closure values remain valid until registration through
js_node_stream_method_on or js_native_call_value completes.
- Around line 1381-1399: Register the GC scanner before any root-bearing state
is stored: update js_readline_stdin_on to call ensure_gc_scanner_registered() at
entry, and update js_readline_create_interface plus js_readline_iterator so
state.input/state.output are covered even when question or on is unused. Apply
the changes at crates/perry-stdlib/src/readline/mod.rs lines 1381-1399 and
1155-1164, preserving the existing NaN-boxed value representation and registered
module-global root invariants.
- Around line 1515-1520: Update destroy() to also lock and clear
READABLE_CALLBACKS alongside DATA_CALLBACKS and KEYPRESS_CALLBACKS, ensuring no
readable callbacks remain active after destruction.
- Around line 1313-1320: Update js_readline_write to send the converted text
through call_write_value using state.output, matching the output behavior in
js_readline_prompt, while preserving the existing cursor_cols update and return
value.
- Around line 1533-1805: Remove build_keypress_object, parse_keypress,
js_readline_process_pending, and js_readline_has_active from readline/mod.rs,
then declare the pump module and re-export its public symbols from mod.rs.
Ensure the existing readline::* export path continues exposing these APIs while
keeping their single implementations in pump.rs and bringing mod.rs below the
2,000-line limit.
- Around line 635-645: Update the non-async dispatch path around
with_interface_mut and fire_line_or_question: borrow the interface only long
enough to extract or clone the callbacks/state required for delivery, then
release that borrow before invoking any callbacks. Preserve the existing line
ordering and async_iter behavior, while ensuring fire_line_or_question and any
nested JS execution run after the READLINE_INTERFACES borrow ends.
In `@crates/perry-stdlib/src/readline/pump.rs`:
- Around line 54-58: Update js_readline_process_pending and the related
parse_keypress flow to reassemble ESC-prefixed ANSI sequences across separate
byte chunks before decoding, so \x1b, [, A produces one up keypress. Remove or
correct the parse_keypress documentation’s reference to a nonexistent
pending_escape accumulator, and add an end-to-end test covering the three-chunk
input and single up result.
- Around line 155-163: Update js_readline_process_pending so READABLE_CALLBACKS
are invoked only when newly read chunks are available, or once when EOF is
reached, rather than on every event-loop tick. Ensure paused or destroyed stdin
states do not dispatch readable callbacks, and prevent the fired count from
keeping the loop active when no eligible readable event occurred.
---
Nitpick comments:
In `@crates/perry-runtime/src/object/spill.rs`:
- Around line 122-142: Move the note_learned_inline_fields call out of
spill_set’s entry path and into spill_set_slow, after confirming the write
requires slow-path growth. Keep the existing high-water update semantics for
first-time capacity expansion while ensuring in-capacity writes return without
any TLS access.
In `@crates/perry-stdlib/src/readline/mod.rs`:
- Line 1002: Update every SAVED mutex lock in setRawMode and the additional
affected sites to recover from poisoning with unwrap_or_else(|p| p.into_inner())
instead of unwrap(). Preserve the existing lock usage and behavior otherwise.
- Around line 523-542: The readline interface lifecycle must reclaim closed
custom-interface slots instead of retaining their state indefinitely. Update
close_custom_interface to remove the closed interface from READLINE_INTERFACES,
and update allocate_interface to reuse available None slots before incrementing
NEXT_READLINE_HANDLE; preserve the fixed STDIN_READLINE_HANDLE behavior.
- Around line 1825-1886: Prevent tests from spawning the real stdin reader by
adding a test-only gate checked by ensure_reader_started and disabling reader
startup while the test suite runs. Update reset() or the test setup to enable
the mocked/test path instead, preserving existing state-reset behavior and
avoiding background changes to EOF_REACHED.
- Around line 906-956: Update the stdin reader thread around PENDING_DATA and
PENDING_LINES to use a buffered reader and a multi-byte buffer instead of
calling reader.read with a one-byte buffer; preserve raw-mode byte ordering and
cooked-mode newline/CRLF behavior while processing each buffered chunk. Bound
both pending queues and apply backpressure when they are full, including
avoiding unbounded line_buf growth, so paused or destroyed consumers cannot
retain input indefinitely.
In `@crates/perry-stdlib/src/readline/pump.rs`:
- Around line 22-48: In the object field assignments, simplify the boolean
conversions in the relevant function by passing ctrl, shift, and meta directly
to JSValue::bool instead of using redundant if/else expressions. Preserve the
existing field indices and values.
- Around line 165-189: Update the pump callback dispatch flow to clone
DATA_CALLBACKS and KEYPRESS_CALLBACKS once before iterating chunks, and parse
each chunk’s keypress data once before invoking callbacks. Reuse the per-chunk
parsed keypress result for all KEYPRESS_CALLBACKS while preserving the existing
data and keypress callback arguments and fired count.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 15ea0ba2-9fe8-4823-9414-c279ddf12977
📒 Files selected for processing (4)
changelog.d/6891-linux-verification-sweep.mdcrates/perry-runtime/src/object/spill.rscrates/perry-stdlib/src/readline/mod.rscrates/perry-stdlib/src/readline/pump.rs
🛑 Comments failed to post (9)
crates/perry-stdlib/src/readline/mod.rs (9)
635-645: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Line dispatch runs JS while the
READLINE_INTERFACESborrow is held.The comment at Lines 616-618 says delivery must happen outside the borrow, but the non-async branch calls
fire_line_or_question(→js_closure_call1) insidewith_interface_mut. Any callback that re-enters the interface (rl.close(),rl.setPrompt(),rl.iterator(), anotherappend_custom_inputfrom a nested emit) hitsRefCellborrow_muttwice → panic. Additionally,scan_readline_roots_mutusestry_borrow_mutand will silently skip the interface slots while this borrow is live, so a moving GC during callback allocation leavesinput/output/callback slots pointing at stale from-space addresses.Extract the callbacks first, then invoke.
🐛 Proposed fix: dispatch outside the borrow
} else { - with_interface_mut(handle, |state| { - for line in lines { - fire_line_or_question(state, line); - } - }); + for line in lines { + let cb = with_interface_mut(handle, |state| { + state.line.clear(); + state + .question_callback + .take() + .or(state.line_callback) + }) + .flatten(); + if let Some(cb_i64) = cb { + js_closure_call1(cb_i64 as *const ClosureHeader, callback_arg(&line)); + } + } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if async_iter { for line in lines { deliver_async_iter_line(handle, line); } } else { for line in lines { let cb = with_interface_mut(handle, |state| { state.line.clear(); state .question_callback .take() .or(state.line_callback) }) .flatten(); if let Some(cb_i64) = cb { js_closure_call1(cb_i64 as *const ClosureHeader, callback_arg(&line)); } } }🤖 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 `@crates/perry-stdlib/src/readline/mod.rs` around lines 635 - 645, Update the non-async dispatch path around with_interface_mut and fire_line_or_question: borrow the interface only long enough to extract or clone the callbacks/state required for delivery, then release that borrow before invoking any callbacks. Preserve the existing line ordering and async_iter behavior, while ensuring fire_line_or_question and any nested JS execution run after the READLINE_INTERFACES borrow ends.
772-796: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Raw object pointers are held live across allocating runtime calls without a
RuntimeHandleScope. In both places a pointer returned byjs_object_alloc_with_shapeis kept in a local and then used after further allocations (closures, strings, symbols), any of which can trigger a moving minor GC and evacuate the object — the laterjs_object_set_fieldwrites then land in from-space and the returned value is stale.crates/perry-runtime/src/object/spill.rsLines 152-155 shows the required pattern: root the pointer and reload it through the handle after every allocation.
crates/perry-stdlib/src/readline/mod.rs#L772-L796: rootobjin a handle scope and reload before eachjs_object_set_field/ before computingiter_val, sincejs_closure_alloc,well_known_symbolandjs_object_set_symbol_propertyall allocate.crates/perry-stdlib/src/readline/pump.rs#L16-L21: allocatename_strandseq_strbeforejs_object_alloc_with_shape, or rootobjacross the twojs_string_from_bytescalls.As per coding guidelines: "Preserve the NaN-boxed JavaScript value representation and GC-rooting invariants when modifying runtime code, including correct pointer/string/BigInt tags and registered module-global roots."
📍 Affects 2 files
crates/perry-stdlib/src/readline/mod.rs#L772-L796(this comment)crates/perry-stdlib/src/readline/pump.rs#L16-L21🤖 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 `@crates/perry-stdlib/src/readline/mod.rs` around lines 772 - 796, The async iterator construction in crates/perry-stdlib/src/readline/mod.rs:772-796 must root obj with RuntimeHandleScope and reload it after every allocating call before field writes and iter_val creation; preserve the existing NaN-boxed value handling and symbol-property setup. In crates/perry-stdlib/src/readline/pump.rs:16-21, move the name_str and seq_str allocations before js_object_alloc_with_shape, or root and reload obj across those string allocations.Source: Coding guidelines
811-846: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
data_value/close_valueare unrooted across subsequent allocations.
datais allocated at Line 815 and its NaN-boxed value captured at 817, thenjs_closure_alloc(818) and threeboxed_strcalls (821-823) can each move the young closure before it is finally handed tojs_node_stream_method_on/js_native_call_value. Root the closures (or allocate the event strings first, immediately before use) so a minor GC between allocation and registration can't install a stale listener pointer.🤖 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 `@crates/perry-stdlib/src/readline/mod.rs` around lines 811 - 846, In attach_custom_input, keep the data and close closure allocations rooted across subsequent js_closure_alloc and boxed_str calls, using the runtime’s established rooting mechanism before constructing data_value and close_value. Ensure both closure values remain valid until registration through js_node_stream_method_on or js_native_call_value completes.
1247-1267: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
js_readline_closeon the stdin path doesn't clearLINE_CALLBACK/QUESTION_CALLBACK.After
rl.close()the loop is expected to quiesce, buthas_line_callbacksstill reports true, andjs_readline_process_pendingwill still deliver a late line to the'line'handler even though the interface is closed. Node stops emitting'line'after close. Clear both cells alongside settingEOF_REACHED.🤖 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 `@crates/perry-stdlib/src/readline/mod.rs` around lines 1247 - 1267, Update js_readline_close on the stdin path to clear both LINE_CALLBACK and QUESTION_CALLBACK when setting EOF_REACHED. Preserve the existing custom-stream handling and close callback behavior, ensuring subsequent has_line_callbacks and js_readline_process_pending calls cannot emit late line events after closure.
1269-1287: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
rl.pause()/rl.resume()are no-ops for the stdin-backed interface.Both only forward to
node_streamwhenstate.inputis a stream pointer. For the defaultcreateInterface({input: process.stdin})singleton,inputis not a node stream, so pausing an interface reading stdin does not stop'line'delivery. Consider gating onuses_custom_streamand flippingSTDIN_PAUSEDfor the stdin path.🤖 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 `@crates/perry-stdlib/src/readline/mod.rs` around lines 1269 - 1287, Update js_readline_pause and js_readline_resume to branch on the interface state’s uses_custom_stream flag: preserve forwarding to node_stream pause/resume for custom streams, and for stdin-backed interfaces set or clear the shared STDIN_PAUSED state so line delivery stops and resumes.
1313-1320: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
rl.write(chunk)never writes anything.It only bumps
cursor_cols. Node'sInterface.write()writes the chunk to the interface's output stream. Route the text throughcall_write_value(state.output, &text)asjs_readline_promptdoes.🐛 Proposed fix
pub extern "C" fn js_readline_write(handle: i64, chunk: f64) -> f64 { let text = value_to_string(chunk); with_interface_mut(handle, |state| { state.cursor_cols = state.cursor_cols.max(text.chars().count() as i32); + call_write_value(state.output, &text); }); undefined() }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.#[no_mangle] pub extern "C" fn js_readline_write(handle: i64, chunk: f64) -> f64 { let text = value_to_string(chunk); with_interface_mut(handle, |state| { state.cursor_cols = state.cursor_cols.max(text.chars().count() as i32); call_write_value(state.output, &text); }); undefined() }🤖 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 `@crates/perry-stdlib/src/readline/mod.rs` around lines 1313 - 1320, Update js_readline_write to send the converted text through call_write_value using state.output, matching the output behavior in js_readline_prompt, while preserving the existing cursor_cols update and return value.
1381-1399: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Two public entry points install GC-visible closure/value slots without ever registering the root scanner.
ensure_gc_scanner_registered()is called fromstdin_on_op,js_readline_questionandjs_readline_on, but not from the externs codegen actually lowersprocess.stdin.on(...)andreadline.createInterface(...)to. A program that uses only those paths stores raw closure pointers and NaN-boxed stream values that the moving collector never rewrites — the exact stale-pointer dispatch the scanner comment at Lines 178-195 was written to prevent.
crates/perry-stdlib/src/readline/mod.rs#L1381-L1399: callensure_gc_scanner_registered()at the top ofjs_readline_stdin_on, before pushing intoDATA_CALLBACKS/KEYPRESS_CALLBACKS/READABLE_CALLBACKS.crates/perry-stdlib/src/readline/mod.rs#L1155-L1164: callensure_gc_scanner_registered()injs_readline_create_interface(and injs_readline_iterator) sostate.input/state.outputare rewritten across a moving GC even whenquestion/onare never used.As per coding guidelines: "Preserve the NaN-boxed JavaScript value representation and GC-rooting invariants when modifying runtime code, including correct pointer/string/BigInt tags and registered module-global roots."
📍 Affects 1 file
crates/perry-stdlib/src/readline/mod.rs#L1381-L1399(this comment)crates/perry-stdlib/src/readline/mod.rs#L1155-L1164🤖 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 `@crates/perry-stdlib/src/readline/mod.rs` around lines 1381 - 1399, Register the GC scanner before any root-bearing state is stored: update js_readline_stdin_on to call ensure_gc_scanner_registered() at entry, and update js_readline_create_interface plus js_readline_iterator so state.input/state.output are covered even when question or on is unused. Apply the changes at crates/perry-stdlib/src/readline/mod.rs lines 1381-1399 and 1155-1164, preserving the existing NaN-boxed value representation and registered module-global root invariants.Source: Coding guidelines
1515-1520: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
destroy()leavesREADABLE_CALLBACKSpopulated.
DATA_CALLBACKSandKEYPRESS_CALLBACKSare cleared but thereadablelist is not, sojs_readline_has_activestill seeshas_stdin_callbacks == trueand the pump still iterates and invokes those closures (seepump.rsLines 155-163, which run regardless ofSTDIN_DESTROYED). Clear it here too.🐛 Proposed fix
if let Ok(mut v) = KEYPRESS_CALLBACKS.lock() { v.clear(); } + if let Ok(mut v) = READABLE_CALLBACKS.lock() { + v.clear(); + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if let Ok(mut v) = DATA_CALLBACKS.lock() { v.clear(); } if let Ok(mut v) = KEYPRESS_CALLBACKS.lock() { v.clear(); } if let Ok(mut v) = READABLE_CALLBACKS.lock() { v.clear(); }🤖 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 `@crates/perry-stdlib/src/readline/mod.rs` around lines 1515 - 1520, Update destroy() to also lock and clear READABLE_CALLBACKS alongside DATA_CALLBACKS and KEYPRESS_CALLBACKS, ensuring no readable callbacks remain active after destruction.
1533-1805: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Confirm pump.rs is wired as a submodule and that the symbols are defined twice. fd -t f 'mod.rs' crates/perry-stdlib/src/readline rg -n '^\s*(pub\s+)?mod\s+pump\s*;' crates/perry-stdlib/src echo '--- duplicate no_mangle externs ---' rg -n -B1 'pub extern "C" fn (js_readline_process_pending|js_readline_has_active)' crates/perry-stdlib/src echo '--- duplicate helpers ---' rg -n 'fn (build_keypress_object|parse_keypress)\b' crates/perry-stdlib/src echo '--- file sizes vs CI cap ---' wc -l crates/perry-stdlib/src/readline/*.rs rg -rn --iglob '*file*size*' -e '2000' scripts .github 2>/dev/null | head -20Repository: PerryTS/perry
Length of output: 1634
🏁 Script executed:
#!/bin/bash # Inspect module declarations and exact duplicates with a deterministic probe. set -euo pipefail echo '--- readline module declarations ---' rg -n '^(pub )?mod ' crates/perry-stdlib/src/readline/mod.rs | head -20 echo '--- readline pump paths mentioned in source ---' rg -n 'readline::|pump\.rs|mod pump|pump::' crates/perry-stdlib/src | head -80 echo '--- exact helper duplicate status ---' python3 - <<'PY' from pathlib import Path mods = Path('crates/perry-stdlib/src/readline/mod.rs') pump = Path('crates/perry-stdlib/src/readline/pump.rs') for text in [mods.read_text(), pump.read_text()]: print('build_keypress_object count', text.count('fn build_keypress_object')) print('parse_keypress count', text.count('fn parse_keypress')) print('js_readline_process_pending count', text.count('pub extern "C" fn js_readline_process_pending')) print('js_readline_has_active count', text.count('pub extern "C" fn js_readline_has_active')) # Extract source blocks, strip comments/whitespace for quick same-content check import re def extract_body(path, prefix): text = path.read_text() for pat, name in [(r'fn build_keypress_object\([^)]*\).*?\n\}', 'build')]: m = re.search(pat, text, re.S) if m: print(name, 'found ' + name) PY echo '--- scripts/check_file_size.sh nearby ---' wc -l scripts/check_file_size.sh sed -n '1,220p' scripts/check_file_size.shRepository: PerryTS/perry
Length of output: 12680
Prevent readline’s duplicate extern definitions and file-size violation.
crates/perry-stdlib/src/readline/mod.rsstill definesjs_readline_process_pending,js_readline_has_active,build_keypress_object, andparse_keypressalongsidecrates/perry-stdlib/src/readline/pump.rs. Sincecrates/perry-stdlib/src/lib.rsre-exportsreadline::*, these symbols are exposed frommod.rswhilepump.rsis still a sibling file. Remove the pumped block frommod.rsand wiremod pump;with re-exports, so the externs are no longer duplicated andreadline/mod.rsstays under the 2,000-line cap.🤖 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 `@crates/perry-stdlib/src/readline/mod.rs` around lines 1533 - 1805, Remove build_keypress_object, parse_keypress, js_readline_process_pending, and js_readline_has_active from readline/mod.rs, then declare the pump module and re-export its public symbols from mod.rs. Ensure the existing readline::* export path continues exposing these APIs while keeping their single implementations in pump.rs and bringing mod.rs below the 2,000-line limit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
9ab8608 to
15fae2f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@CONTRIBUTING.md`:
- Around line 25-30: Update the stale Node version reference in the parity-suite
guidance around the existing inline comment near line 88: replace the hardcoded
“Node 22” requirement with a reference to the exact version pinned in
.node-version, keeping the surrounding parity-test instructions unchanged.
In `@crates/perry/src/commands/check.rs`:
- Around line 197-211: Update the parse-error branch in the file-checking flow
to increment checked_files before continuing, alongside the Diagnostic::error
pushed to all_diagnostics. Ensure invalid files count as checked for
files_checked and progress accounting while preserving the existing blocking
diagnostic and continue behavior.
In `@docs/src/getting-started/installation.md`:
- Around line 13-36: Update the Debian/Ubuntu installation command in the Linux
toolchain section to install a supported clang version (15 or newer, such as the
documented versioned package) and the required libclang development package for
bindgen. Keep the existing linker/toolchain package and align the command with
the prerequisites documented in building.md.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b32b95b-afd0-452d-b5a2-5d62096b4cce
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
CLAUDE.mdCONTRIBUTING.mdCargo.tomlchangelog.d/6891-linux-verification-sweep.mdcrates/perry-codegen/tests/native_proof_regressions.rscrates/perry-hir/tests/c262_parity.rscrates/perry-runtime/src/object/mod.rscrates/perry-runtime/src/object/spill.rscrates/perry-stdlib/src/crypto/random.rscrates/perry-stdlib/src/readline/mod.rscrates/perry-stdlib/src/readline/pump.rscrates/perry/src/commands/check.rscrates/perry/src/commands/compile/collect_modules.rsdocs/src/contributing/building.mddocs/src/getting-started/installation.mdrun_parity_tests.shscripts/addr_class_ratchet_baseline.txt
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/perry-runtime/src/object/spill.rs
- crates/perry-stdlib/src/readline/pump.rs
- crates/perry-stdlib/src/readline/mod.rs
| // A file that does not parse is a check FAILURE, not a file to | ||
| // skip. Before this, the error was printed only under `-v` and | ||
| // the file was dropped without touching `all_diagnostics`, so | ||
| // `error_count()` stayed 0 and `perry check` reported "All | ||
| // checks passed!" with exit 0 on syntactically invalid code — | ||
| // while `perry compile` correctly rejected the same file. | ||
| // Record it as a real diagnostic so the text summary, the JSON | ||
| // `success` field and the exit code all agree. | ||
| all_diagnostics.push( | ||
| Diagnostic::error( | ||
| DiagnosticCode::ParseError, | ||
| format!("{}: {}", canonical.display(), e), | ||
| ) | ||
| .build(), | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Count parse failures as checked files.
This branch records a blocking diagnostic but continues before checked_files += 1. A project containing only an invalid file therefore reports files_checked: 0, and progress accounting undercounts attempted files.
Proposed fix
Err(e) => {
+ checked_files += 1;
// A file that does not parse is a check FAILURE, not a file to📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // A file that does not parse is a check FAILURE, not a file to | |
| // skip. Before this, the error was printed only under `-v` and | |
| // the file was dropped without touching `all_diagnostics`, so | |
| // `error_count()` stayed 0 and `perry check` reported "All | |
| // checks passed!" with exit 0 on syntactically invalid code — | |
| // while `perry compile` correctly rejected the same file. | |
| // Record it as a real diagnostic so the text summary, the JSON | |
| // `success` field and the exit code all agree. | |
| all_diagnostics.push( | |
| Diagnostic::error( | |
| DiagnosticCode::ParseError, | |
| format!("{}: {}", canonical.display(), e), | |
| ) | |
| .build(), | |
| ); | |
| checked_files += 1; | |
| // A file that does not parse is a check FAILURE, not a file to | |
| // skip. Before this, the error was printed only under `-v` and | |
| // the file was dropped without touching `all_diagnostics`, so | |
| // `error_count()` stayed 0 and `perry check` reported "All | |
| // checks passed!" with exit 0 on syntactically invalid code — | |
| // while `perry compile` correctly rejected the same file. | |
| // Record it as a real diagnostic so the text summary, the JSON | |
| // `success` field and the exit code all agree. | |
| all_diagnostics.push( | |
| Diagnostic::error( | |
| DiagnosticCode::ParseError, | |
| format!("{}: {}", canonical.display(), e), | |
| ) | |
| .build(), | |
| ); |
🤖 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 `@crates/perry/src/commands/check.rs` around lines 197 - 211, Update the
parse-error branch in the file-checking flow to increment checked_files before
continuing, alongside the Diagnostic::error pushed to all_diagnostics. Ensure
invalid files count as checked for files_checked and progress accounting while
preserving the existing blocking diagnostic and continue behavior.
|
Pushed Actionable findings fixed:
Nitpicks:
Skipped (1):
Verification: |
Found while verifying a fresh Linux checkout end-to-end. Everything here except the `perry check` bug was already red on `main`. - `perry check` reported "All checks passed!" and exit 0 on code that does not parse: the parse error was printed only under `-v`, then `continue`d without touching `all_diagnostics`, so `error_count()` stayed 0. Record a P001 diagnostic instead, so the text summary, the JSON `success` field and the exit code agree. `perry compile` already rejected these files. - File-size gate (2000-line cap): split `object/mod.rs` 2039 -> 1709 by extracting the #6812 spill/overflow storage into `object/spill.rs`, and `readline.rs` 2066 -> 1796 by extracting the drain/pump + keypress decoding into `readline/pump.rs`. Code is unchanged; only visibility was widened so the existing `use super::*` sites keep resolving. - rustfmt: blank line after the trailing comment in `collect_modules.rs` so rustfmt stops indenting the following comment block to column 40. - Three stale test assertions, each verified behaviourally against Node 26.5.0 first (no user-visible defect in any of them): * perry-hir expected the `??=` RHS store as `PutValueSet`; lowering emits `PropertySet` for a plain member target. Accept both — the invariant is *where* the store sits, not which node encodes it. * perry-codegen sliced `fast.inner.body`..`fast.inner.exit` for the numeric stores, but #6812's spill lanes moved them into `fast.store.spill.*` / `fast.store.inline.*` successors. Collect every `object_array_write.loop.fast.*` block instead. * perry-stdlib asserted `randomBytes`' callback fires synchronously; #6430 deliberately deferred it to a macrotask to match Node's libuv timing. Drain the immediate queue, and assert it does NOT fire inline. - run_parity_tests.sh: the default `test-files/` suite under PERRY_NO_AUTO_OPTIMIZE linked the prebuilt `full` stdlib with no `external-*` pumps, producing 7 false "NEW gap failure" reports (events/http/net/fetch) that all pass with auto-optimize. node-suite already compensated; do the same for the default suite. - Docs: libclang and rustc >= 1.94 were undocumented build prerequisites, and installation.md required "clang >= 15 for codegen" while every per-distro command below it installed no clang. Also point Node at .node-version rather than the stale "22". - addr-class ratchet baseline: record the already-fixed accessors.rs site and the readline path move. Gap suite 386/395 with every remaining failure triaged in known_failures.json. cargo test green for perry-parser/-hir/-transform/ -codegen/-dispatch/-api-manifest, perry-runtime --lib (1468), perry-stdlib --lib, perry --bins (734). Both clippy scopes clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
changelog.d/6891-linux-verification-sweep.md (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit the long CI-gates sentence for readability.
This line combines several independent changes; use two sentences so the changelog is easier to scan.
🤖 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 `@changelog.d/6891-linux-verification-sweep.md` at line 3, Split the single long changelog sentence beginning “Recovered three red CI gates on main” into two sentences, grouping the related extraction changes separately from the formatting and addr-class baseline updates. Preserve all existing details and wording aside from the sentence boundary.Source: Linters/SAST tools
🤖 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 `@crates/perry-stdlib/src/readline/mod.rs`:
- Around line 606-614: The close flow must not route a repeated close of a
custom-stream interface into the singleton stdin path after READLINE_INTERFACES
loses its slot. Preserve the closed custom-interface state in
READLINE_INTERFACES so js_readline_close continues selecting
close_custom_interface, whose existing state.closed guard makes subsequent
closes no-ops; add a regression test for double-closing a custom interface if
the surrounding test structure supports it.
- Around line 850-908: Remove the initial raw_ptr_from_value(input).is_none()
early return in attach_custom_input so child_process stdio pipes can reach the
!stream_is_readable(input_handle.get_nanbox_f64()) fallback and register
listeners through their own .on method. Preserve the native node_stream
registration path for readable streams, including its existing per-call raw
pointer validation.
---
Nitpick comments:
In `@changelog.d/6891-linux-verification-sweep.md`:
- Line 3: Split the single long changelog sentence beginning “Recovered three
red CI gates on main” into two sentences, grouping the related extraction
changes separately from the formatting and addr-class baseline updates. Preserve
all existing details and wording aside from the sentence boundary.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d5a17b0e-7b47-4d52-855e-a276db60bdc3
📒 Files selected for processing (4)
changelog.d/6891-linux-verification-sweep.mdcrates/perry-runtime/src/object/spill.rscrates/perry-stdlib/src/readline/mod.rscrates/perry-stdlib/src/readline/pump.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/perry-stdlib/src/readline/pump.rs
- crates/perry-runtime/src/object/spill.rs
| // Release the slot so the GC scanner stops rooting the closed | ||
| // interface's input/output/callbacks. Handles are NOT reused: a stale | ||
| // handle to a closed interface must hit the `None` slot (a no-op, like | ||
| // Node's ERR_USE_AFTER_CLOSE), not alias a newer interface. | ||
| READLINE_INTERFACES.with(|interfaces| { | ||
| if let Some(slot) = interfaces.borrow_mut().get_mut(handle as usize) { | ||
| *slot = None; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Clearing the interface slot on close breaks routing for a later close() on the same handle, corrupting the unrelated stdin-backed interface's global state.
close_custom_interface now sets READLINE_INTERFACES[handle] = None (Line 611-613). But js_readline_close decides which close path to run via with_interface(_handle, |state| state.uses_custom_stream).unwrap_or(false) (Line 1322) — once the slot is None, this lookup fails and defaults to false (non-custom), so a second close() on the same handle (a plain double-close(), or an explicit rl.close() after the stream's own 'end'/'close' already invoked custom_input_close → close_custom_interface) falls straight into the singleton-stdin close path below: it sets the global EOF_REACHED (killing js_readline_has_active/event-loop liveness for the entire stdin), wipes the shared QUESTION_CALLBACK/LINE_CALLBACK (used by the real stdin-backed interface, not this custom one), and can mis-fire the shared CLOSE_CALLBACK.
Before this change, the slot stayed populated with closed: true, so a repeat close still routed correctly to close_custom_interface, which already no-ops via if state.closed { None } else { ... }. Removing the slot instead of relying on that flag reintroduces the exact hazard the flag existed to prevent.
🐛 Proposed fix: don't let a missing custom-interface slot fall through to the stdin close path
pub extern "C" fn js_readline_close(_handle: i64) -> f64 {
- if with_interface(_handle, |state| state.uses_custom_stream).unwrap_or(false) {
- close_custom_interface(_handle);
- return undefined();
- }
+ match with_interface(_handle, |state| state.uses_custom_stream) {
+ Some(true) => {
+ close_custom_interface(_handle);
+ return undefined();
+ }
+ None if _handle != STDIN_READLINE_HANDLE => {
+ // Slot already cleared by a prior close_custom_interface() call
+ // (double close(), or close() after the backing stream's own
+ // 'end'/'close' already tore it down) — a no-op, not a reason
+ // to run the stdin-backed close path against unrelated global
+ // state.
+ return undefined();
+ }
+ _ => {}
+ }
EOF_REACHED.store(true, Ordering::Release);Consider adding a regression test that calls close() twice on a custom-stream interface and asserts EOF_REACHED/QUESTION_CALLBACK are untouched.
Also applies to: 1321-1345
🤖 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 `@crates/perry-stdlib/src/readline/mod.rs` around lines 606 - 614, The close
flow must not route a repeated close of a custom-stream interface into the
singleton stdin path after READLINE_INTERFACES loses its slot. Preserve the
closed custom-interface state in READLINE_INTERFACES so js_readline_close
continues selecting close_custom_interface, whose existing state.closed guard
makes subsequent closes no-ops; add a regression test for double-closing a
custom interface if the surrounding test structure supports it.
pump.rs: - Reassemble ANSI escape sequences that the raw-mode reader queues as 1-byte chunks (ESC, '[', 'A' -> one 'up' keypress) via a PENDING_ESCAPE accumulator carried across ticks; a bare ESC flushes on the next tick (tick-granularity stand-in for Node's escapeCodeTimeout), and the held prefix counts as pending data so the flush tick runs. - 'readable' listeners fire only when a tick delivers new chunks, plus a one-shot at EOF, instead of on every event-loop iteration forever. - Listener lists are cloned once per drain and each chunk parsed once (not once per callback); the keypress sequence string is rooted across the key-object build; build_keypress_object roots the object across its string allocations; JSValue::bool(cond) simplification. readline/mod.rs: - Line dispatch pulls callbacks under a short interface borrow and invokes them after it drops — a re-entrant close()/emit would panic the RefCell, and the GC scanner skips borrowed slots. Same treatment for js_readline_prompt / js_readline_write (via call_write_value). - js_readline_iterator and attach_custom_input root values in a RuntimeHandleScope across allocating calls; attach_custom_input also recomputes the raw stream pointer per .on call. - ensure_gc_scanner_registered() now also runs from js_readline_stdin_on, js_readline_create_interface and js_readline_iterator. - rl.close() clears LINE/QUESTION callbacks (no late 'line' after close); rl.pause()/resume() gate STDIN_PAUSED for the stdin-backed interface and the pump holds queued lines while paused; rl.write() writes the chunk to the output stream; destroy() clears READABLE_CALLBACKS and the escape accumulator; closed custom interfaces release their slot (handles are not reused — a stale handle must no-op, not alias a newer interface); SAVED termios locks recover from poisoning. - Tests never spawn the real stdin reader (cfg(test) gate), so reset() clears READER_STARTED; new tests: split-escape reassembly, bare-ESC flush, readable gating. object/spill.rs: - note_learned_inline_fields moves off the steady-state hot path: only writes that raise the spill buffer's high-water mark (or take the slow path) record it. Slow-path-only would under-learn inside the power-of-two capacity headroom; the length gate keeps learned widths identical while making round-robin overflow writes TLS-free.
1d075c9 to
81bb06d
Compare
1 participant
Found while verifying a fresh Linux checkout end-to-end (build → gap suite → cargo test → lint gates). Everything here except the
perry checkbug was already red onmain.The one real user-facing bug
perry checkreturned exit 0 and "All checks passed!" on code that does not parse.check.rsprinted the parse error only under-v, thencontinued without touchingall_diagnostics, soerror_count()stayed 0 and the success branch ran.perry compilerejected the same file correctly. Now recorded as aP001diagnostic, so the text summary, the JSONsuccessfield and the exit code agree. There was no test coverage forperry checkexit codes anywhere incrates/perry/tests/.Red CI gates on
maincheck_file_size.shobject/mod.rs2039,readline.rs2066 (cap 2000)object/spill.rs(→1709) andreadline/pump.rs(→1796)cargo fmt --checkcollect_modules.rsaddr_class_inventory.pyaccessors.rssite + readline path moveBoth splits move code verbatim; only visibility widened so existing
use super::*sites keep resolving.Three stale test assertions
Each verified behaviourally against Node 26.5.0 before touching — none was a real defect:
logical_property_assignment_*— expected the??=RHS store asPutValueSet; lowering emitsPropertySetfor a plain member target (PutValueSetis the parenthesized/cast path from runtime: parenthesized /as-cast assignment target silently mutates aconst(no TypeError) #6300). Accepts both now; the invariant is where the store sits.??=/||=/&&=incl. RHS side-effect suppression match Node exactly.nested_same_shape_object_writes_*— slicedfast.inner.body..fast.inner.exitfor the numeric stores, but perf: generalize guarded object-write fast paths beyond the #6811 two-field loop #6812's spill lanes moved them intofast.store.spill.*/fast.store.inline.*successors. Now collects everyobject_array_write.loop.fast.*block, which is also stable under block reordering. 1/2/4-field writes at n=3/100/5000 (incl. past-capacity growth) match Node.native_dispatch_random_bytes_*— asserted the callback fires synchronously, but fix(crypto): run WebCrypto digest + async randomBytes on a macrotask like Node #6430 deliberately deferred it to a macrotask to match Node's libuv-threadpool timing. Now drains the immediate queue and additionally asserts it does not fire inline.Two of these live in
crates/*/tests/*.rs, which don't run per-PR (nightly/tag only) — the #5960 failure mode. TherandomBytesone is a--libunit test that does run per-PR.Harness: 7 false regressions
Under
PERRY_NO_AUTO_OPTIMIZEthe defaulttest-files/suite linked the prebuiltfullstdlib, which has noexternal-*pumps — so everyevents/http/net/fetchgap test failed and was reported as an untriaged NEW gap failure with no hint the run mode caused it. All 7 pass with auto-optimize (verified individually).node-suitealready compensated; the default suite now does too.This matters locally because a cold auto-optimize run isn't feasible on one machine — it mints a ThinLTO variant per feature combo (measured: 12 variants / 71 GB after 30 of 411 tests, extrapolating to ~1 TB).
Docs
libclang(bindgen →libsqlite3-sys) and rustc ≥ 1.94 (sqlx 0.9.0's MSRV; nothing pins a toolchain) were undocumented build prerequisites — both hard-fail a fresh checkout with confusing errors.installation.mdalso stated Linux needs "clang ≥ 15 for codegen" while every per-distro command below it installed no clang. Node now points at.node-versionrather than the stale "22".Verification
known_failures.jsoncargo testgreen: perry-parser, -hir, -transform, -codegen, -dispatch, -api-manifest, perry-runtime--lib(1468), perry-stdlib--lib, perry--bins(734)Not fixed — needs a GC owner
run_memory_stability_tests.shexits 1 on 8 telemetry-attribution assertions (e.g. expectedBarriersInactive, gotNotAttempted). These are pre-existing and not safety failures: the safety assertions in the same tests pass (the candidate is neither moved nor forwarded), both real correctness canaries (evacuation verifier surfaces,copying minor rewrites) pass, and all 32 RSS runs pass includingforce-evac+verifywithPERRY_GC_VERIFY_EVACUATION=1.Lead worth checking:
gc/mod.rs:120returns to a full mark-sweep viacopied_minor_promotion_handoff_duebefore the copying fast path evaluates eligibility, which would leavecopying_nursery.fallback_reasonat itsNotAttempteddefault and never reach theBarriersInactivegate. I deliberately did not touch this — relaxing the assertion could mask a real ordering regression, and changing the policy needs the owner's intent.🤖 Generated with Claude Code
Summary by CodeRabbit
perry checknow correctly reports parse failures in text and JSON results and returns a failing exit code.crypto.randomBytes.