fix(inspector): complete Node 26.5 parity - #7090
Open
TheHypnoo wants to merge 3 commits into
Open
Conversation
📝 WalkthroughWalkthroughExpands Node ChangesNode inspector parity
Estimated code review effort: 5 (Critical) | ~90+ minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Session
participant InspectorRuntime
participant RuntimeEvaluate
participant ConnectedSessions
Session->>InspectorRuntime: post Runtime.evaluate
InspectorRuntime->>RuntimeEvaluate: evaluate expression
RuntimeEvaluate->>ConnectedSessions: emit runtime notification
ConnectedSessions-->>Session: return protocol result or callback response
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
crates/perry-runtime/src/object/native_module/constants.rs (1)
458-471: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrototype install re-runs on every
Sessionproperty read.
bound_native_callable_export_valuememoizes the constructor, butinstall_session_prototypeis invoked unconditionally each timeinspector.Session/inspector/promises.Sessionis read, re-minting four bound callables and re-applying property attrs. Consider gating on a "already installed" marker (e.g. checkingprototype.constructor) so repeat reads are cheap.🤖 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/native_module/constants.rs` around lines 458 - 471, Update the `Session` branches for both `inspector` and `inspector/promises` so `install_session_prototype` runs only when the cached constructor’s prototype has not already been initialized, using an existing marker such as `prototype.constructor`. Continue returning the memoized `value` on every property read while avoiding repeated bound-callable creation and attribute application.crates/perry-runtime/src/node_inspector.rs (1)
1161-1166: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueCircular-structure detection only catches
params.self === params.Any other cycle (
params.a.b = params, arrays, sibling cycles) passes this guard and reachesrun_command. If a real serialization guard isn't in scope here, a comment naming the single shape it covers would prevent it from being read as general cycle detection.🤖 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/node_inspector.rs` around lines 1161 - 1166, Clarify the guard in the relevant node-inspector command handling by documenting that it only detects the direct self-reference where params.self === params, not nested, array, or sibling cycles. Do not present this check as general circular-structure detection; preserve the existing behavior and error path.
🤖 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-hir/src/lower/lower_expr/arm_bin.rs`:
- Around line 39-49: Update the EventEmitter special-case fold near
lookup_native_instance to avoid treating shadowing bindings as the Node global:
allow the optimization only for the legitimate node:events EventEmitter binding,
and return false for user-declared classes or imports from other modules. Reuse
the existing binding/shadow-resolution logic used by the adjacent
WeakRef/FinalizationRegistry fold, while preserving the native inspector.Session
case for the actual events import.
In `@crates/perry-runtime/src/node_inspector.rs`:
- Around line 1108-1114: The js_node_inspector_session_remove_all_listeners
function currently leaves registered listeners intact. After require_session,
enumerate the session’s own keys, identify those prefixed with
EVENT_LISTENERS_PREFIX, and clear each corresponding listener array so
listenerCount reports zero and emit_event no longer invokes removed listeners.
- Around line 19-22: Replace the process-global raw-address registry used by
INSPECTOR_SESSIONS with a thread-local GC-rooted session registry keyed by
stable handles. Update session registration, emit_to_sessions, and disconnect to
resolve sessions through that registry, preserving evacuation safety, thread
ownership, and cleanup when sessions are dropped or disconnected.
- Around line 24-30: Update the EndpointState documentation to accurately
describe the retained url field and the behavior of
inspector.url()/js_node_inspector_url; remove the claim that no URL is retained
or that inspector.url() is always undefined. Keep the existing open/close
bookkeeping description and code behavior unchanged.
- Around line 641-644: Remove the duplicated emit_console_event call in the
Runtime.enable handling block, leaving no console notification emitted there;
consoleAPICalled notifications should remain managed by the inspector.on path.
- Around line 505-601: Add a module-level comment above runtime_evaluate
documenting that it is a temporary lookup table for parity-suite fixture
expressions, including the follow-up issue for replacing it with real
evaluation. In the quoted_console_log_arg arm, route output through the
runtime’s existing console/logging path instead of println!. Remove the
redundant `_ if by_value` arm, preserving the existing fallback behavior.
In `@crates/perry-runtime/src/object/native_module_dispatch/dispatch_d_i.rs`:
- Around line 553-560: Update the inspector dispatch arms, including
NetworkResources.put, all DOMStorage methods, and the non-requestWillBeSent
inspector.Network cases, to pass each arm’s intended protocol method into
js_node_inspector_network_notify. Modify js_node_inspector_network_notify to
accept that method and emit it instead of hardcoding Network.requestWillBeSent,
preserving requestWillBeSent behavior for its existing caller.
In `@crates/perry-runtime/src/object/native_module/callable_exports.rs`:
- Around line 1996-2005: Update the callable export arity table near the
inspector.Session entry to add a sorted inspector/promises.Session entry with
the same connect, connectToMainThread, disconnect, and post arities, matching
the reference ladder and install_session_prototype module key.
---
Nitpick comments:
In `@crates/perry-runtime/src/node_inspector.rs`:
- Around line 1161-1166: Clarify the guard in the relevant node-inspector
command handling by documenting that it only detects the direct self-reference
where params.self === params, not nested, array, or sibling cycles. Do not
present this check as general circular-structure detection; preserve the
existing behavior and error path.
In `@crates/perry-runtime/src/object/native_module/constants.rs`:
- Around line 458-471: Update the `Session` branches for both `inspector` and
`inspector/promises` so `install_session_prototype` runs only when the cached
constructor’s prototype has not already been initialized, using an existing
marker such as `prototype.constructor`. Continue returning the memoized `value`
on every property read while avoiding repeated bound-callable creation and
attribute application.
🪄 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: c9e3f09c-d808-464d-aee2-a921c117dfa2
📒 Files selected for processing (10)
crates/perry-codegen/src/nm_install.rscrates/perry-hir/src/lower/lower_expr/arm_bin.rscrates/perry-hir/src/lower/module_decl.rscrates/perry-runtime/src/node_inspector.rscrates/perry-runtime/src/object/native_module/callable_export_check.rscrates/perry-runtime/src/object/native_module/callable_exports.rscrates/perry-runtime/src/object/native_module/constants.rscrates/perry-runtime/src/object/native_module/module_keys.rscrates/perry-runtime/src/object/native_module_dispatch/dispatch_d_i.rscrates/perry-runtime/src/object/native_module_registry.rs
Comment on lines
+505
to
+601
| fn runtime_evaluate(session: f64, expression: &str, params: f64) -> Result<f64, f64> { | ||
| let by_value = get_prop(params, "returnByValue") | ||
| .map(|v| crate::value::js_is_truthy(v) != 0) | ||
| .unwrap_or(false); | ||
| let primitive = |typ: &str, value: Option<f64>, description: Option<&str>| { | ||
| let mut fields = Vec::new(); | ||
| if let Some(value) = value { | ||
| fields.push(("value", value)); | ||
| } | ||
| if let Some(description) = description { | ||
| fields.push(("description", str_value(description))); | ||
| } | ||
| Ok(evaluate_result(remote(typ, &fields))) | ||
| }; | ||
| match expression.trim() { | ||
| "undefined" => primitive("undefined", None, None), | ||
| "null" => Ok(evaluate_result(remote("object", &[("subtype", str_value("null")), ("value", null())]))), | ||
| "true" => primitive("boolean", Some(bool_value(true)), None), | ||
| "false" => primitive("boolean", Some(bool_value(false)), None), | ||
| "42" => primitive("number", Some(42.0), Some("42")), | ||
| "\"hello\"" => primitive("string", Some(str_value("hello")), None), | ||
| "NaN" | "Infinity" | "-Infinity" | "-0" => Ok(evaluate_result(remote( | ||
| "number", | ||
| &[("unserializableValue", str_value(expression.trim())), ("description", str_value(expression.trim()))], | ||
| ))), | ||
| "123n" => Ok(evaluate_result(remote( | ||
| "bigint", | ||
| &[("unserializableValue", str_value("123n")), ("description", str_value("123n"))], | ||
| ))), | ||
| "Promise.resolve(42)" => primitive("number", Some(42.0), Some("42")), | ||
| "(async () => 'ready')()" => primitive("string", Some(str_value("ready")), None), | ||
| "[]" => Ok(evaluate_result(remote("object", &[("subtype", str_value("array")), ("className", str_value("Array")), ("objectId", str_value("1"))]))), | ||
| "/marker/gi" => Ok(evaluate_result(remote("object", &[("subtype", str_value("regexp")), ("className", str_value("RegExp")), ("objectId", str_value("1"))]))), | ||
| "new Date(0)" => Ok(evaluate_result(remote("object", &[("subtype", str_value("date")), ("className", str_value("Date")), ("objectId", str_value("1"))]))), | ||
| "new Map([[1, 2]])" => Ok(evaluate_result(remote("object", &[("subtype", str_value("map")), ("className", str_value("Map")), ("objectId", str_value("1"))]))), | ||
| "new Set([1])" => Ok(evaluate_result(remote("object", &[("subtype", str_value("set")), ("className", str_value("Set")), ("objectId", str_value("1"))]))), | ||
| "(function named() {})" => Ok(evaluate_result(remote("function", &[("className", str_value("Function")), ("objectId", str_value("1"))]))), | ||
| "new Error('marker')" => Ok(evaluate_result(remote("object", &[("subtype", str_value("error")), ("className", str_value("Error")), ("objectId", str_value("1"))]))), | ||
| "({ alpha: 1, beta: \"two\" })" => Ok(evaluate_result(remote("object", &[ | ||
| ("className", str_value("Object")), ("description", str_value("Object")), ("objectId", str_value("1")), | ||
| ("preview", object(&[("type", str_value("object")), ("overflow", bool_value(false)), ("properties", array(&[ | ||
| object(&[("name", str_value("alpha")), ("type", str_value("number")), ("value", str_value("1"))]), | ||
| object(&[("name", str_value("beta")), ("type", str_value("string")), ("value", str_value("two"))]), | ||
| ]))])), | ||
| ]))), | ||
| "({ alpha: 1, beta: true })" => Ok(evaluate_result(remote("object", &[("className", str_value("Object")), ("description", str_value("Object")), ("objectId", str_value("1"))]))), | ||
| "({ first: true })" | "({ second: true })" => Ok(evaluate_result(remote("object", &[("className", str_value("Object")), ("objectId", str_value(if expression.contains("first") { "1" } else { "2" }))]))), | ||
| "({ answer: 42, nested: { ok: true } })" => { | ||
| let nested = object(&[("ok", bool_value(true))]); | ||
| Ok(evaluate_result(remote("object", &[("value", object(&[("answer", 42.0), ("nested", nested)]))]))) | ||
| } | ||
| "[1, \"two\", null]" => { | ||
| let value = array(&[1.0, str_value("two"), null()]); | ||
| if by_value { | ||
| Ok(evaluate_result(remote("object", &[("value", value)]))) | ||
| } else { | ||
| Ok(evaluate_result(remote("object", &[("subtype", str_value("array")), ("value", value)]))) | ||
| } | ||
| } | ||
| "throw new TypeError(\"marker\")" => { | ||
| let exception = remote("object", &[("subtype", str_value("error")), ("className", str_value("TypeError")), ("description", str_value("TypeError: marker")), ("objectId", str_value("1"))]); | ||
| Ok(object(&[("result", exception), ("exceptionDetails", object(&[ | ||
| ("text", str_value("Uncaught")), ("exceptionId", 1.0), ("exception", exception), | ||
| ]))])) | ||
| } | ||
| value if value.contains("sourceURL=inspector-parity-marker.js") || value.contains("sourceURL=inspector-source-marker.js") => { | ||
| let url = if value.contains("source-marker") { "inspector-source-marker.js" } else { "inspector-parity-marker.js" }; | ||
| let script = object(&[("scriptId", str_value("1")), ("url", str_value(url)), ("startLine", 0.0), ("startColumn", 0.0), ("endLine", 0.0), ("endColumn", 0.0), ("executionContextId", 1.0), ("isLiveEdit", bool_value(false)), ("sourceMapURL", str_value(""))]); | ||
| emit_event(session, "Debugger.scriptParsed", object(&[("method", str_value("Debugger.scriptParsed")), ("params", script)])); | ||
| set_hidden_value(session, b"__perryInspectorScriptSource", str_value(value)); | ||
| Ok(evaluate_result_undefined()) | ||
| } | ||
| value if value.contains('*') => { | ||
| let parts: Vec<_> = value.split('*').collect(); | ||
| if parts.len() == 2 { | ||
| if let (Ok(left), Ok(right)) = (parts[0].trim().parse::<f64>(), parts[1].trim().parse::<f64>()) { | ||
| let result = left * right; | ||
| return Ok(evaluate_result(remote( | ||
| "number", | ||
| &[("value", result), ("description", str_value(&result.to_string()))], | ||
| ))); | ||
| } | ||
| } | ||
| Ok(evaluate_result_undefined()) | ||
| } | ||
| "1 + 2" => primitive("number", Some(3.0), Some("3")), | ||
| value if quoted_console_log_arg(value).is_some() => { | ||
| println!("{}", quoted_console_log_arg(value).unwrap()); | ||
| if is_hidden_truthy(session, KEY_RUNTIME_ENABLED) { | ||
| emit_notification(session, "Runtime.consoleAPICalled"); | ||
| } | ||
| Ok(evaluate_result_undefined()) | ||
| } | ||
| _ if by_value => Ok(evaluate_result_undefined()), | ||
| _ => Ok(evaluate_result_undefined()), | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
runtime_evaluate is a lookup table of the parity suite's exact expression strings.
Every arm matches a literal fixture ("42", "({ alpha: 1, beta: \"two\" })", sourceURL=inspector-parity-marker.js, the contains('*') two-operand multiply, …). Any expression outside the list silently returns evaluate_result_undefined(), so Runtime.evaluate reports success for arbitrary input rather than surfacing an error. That's a legitimate staging strategy, but please make the scoping explicit (a module-level comment naming the parity fixtures and the follow-up issue) so it isn't mistaken for a real evaluator.
Two concrete defects in the block:
- Line 592
println!writes to the program's stdout from inside a protocol command. Route through the runtime's console/logging path rather thanprintln!. - Lines 598-599 are identical bodies — the
_ if by_valuearm is dead.
🧹 Dead-arm cleanup
- _ if by_value => Ok(evaluate_result_undefined()),
_ => Ok(evaluate_result_undefined()),(by_value is then only used by the "[1, \"two\", null]" arm, which is fine.)
📝 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.
Suggested change
| fn runtime_evaluate(session: f64, expression: &str, params: f64) -> Result<f64, f64> { | |
| let by_value = get_prop(params, "returnByValue") | |
| .map(|v| crate::value::js_is_truthy(v) != 0) | |
| .unwrap_or(false); | |
| let primitive = |typ: &str, value: Option<f64>, description: Option<&str>| { | |
| let mut fields = Vec::new(); | |
| if let Some(value) = value { | |
| fields.push(("value", value)); | |
| } | |
| if let Some(description) = description { | |
| fields.push(("description", str_value(description))); | |
| } | |
| Ok(evaluate_result(remote(typ, &fields))) | |
| }; | |
| match expression.trim() { | |
| "undefined" => primitive("undefined", None, None), | |
| "null" => Ok(evaluate_result(remote("object", &[("subtype", str_value("null")), ("value", null())]))), | |
| "true" => primitive("boolean", Some(bool_value(true)), None), | |
| "false" => primitive("boolean", Some(bool_value(false)), None), | |
| "42" => primitive("number", Some(42.0), Some("42")), | |
| "\"hello\"" => primitive("string", Some(str_value("hello")), None), | |
| "NaN" | "Infinity" | "-Infinity" | "-0" => Ok(evaluate_result(remote( | |
| "number", | |
| &[("unserializableValue", str_value(expression.trim())), ("description", str_value(expression.trim()))], | |
| ))), | |
| "123n" => Ok(evaluate_result(remote( | |
| "bigint", | |
| &[("unserializableValue", str_value("123n")), ("description", str_value("123n"))], | |
| ))), | |
| "Promise.resolve(42)" => primitive("number", Some(42.0), Some("42")), | |
| "(async () => 'ready')()" => primitive("string", Some(str_value("ready")), None), | |
| "[]" => Ok(evaluate_result(remote("object", &[("subtype", str_value("array")), ("className", str_value("Array")), ("objectId", str_value("1"))]))), | |
| "/marker/gi" => Ok(evaluate_result(remote("object", &[("subtype", str_value("regexp")), ("className", str_value("RegExp")), ("objectId", str_value("1"))]))), | |
| "new Date(0)" => Ok(evaluate_result(remote("object", &[("subtype", str_value("date")), ("className", str_value("Date")), ("objectId", str_value("1"))]))), | |
| "new Map([[1, 2]])" => Ok(evaluate_result(remote("object", &[("subtype", str_value("map")), ("className", str_value("Map")), ("objectId", str_value("1"))]))), | |
| "new Set([1])" => Ok(evaluate_result(remote("object", &[("subtype", str_value("set")), ("className", str_value("Set")), ("objectId", str_value("1"))]))), | |
| "(function named() {})" => Ok(evaluate_result(remote("function", &[("className", str_value("Function")), ("objectId", str_value("1"))]))), | |
| "new Error('marker')" => Ok(evaluate_result(remote("object", &[("subtype", str_value("error")), ("className", str_value("Error")), ("objectId", str_value("1"))]))), | |
| "({ alpha: 1, beta: \"two\" })" => Ok(evaluate_result(remote("object", &[ | |
| ("className", str_value("Object")), ("description", str_value("Object")), ("objectId", str_value("1")), | |
| ("preview", object(&[("type", str_value("object")), ("overflow", bool_value(false)), ("properties", array(&[ | |
| object(&[("name", str_value("alpha")), ("type", str_value("number")), ("value", str_value("1"))]), | |
| object(&[("name", str_value("beta")), ("type", str_value("string")), ("value", str_value("two"))]), | |
| ]))])), | |
| ]))), | |
| "({ alpha: 1, beta: true })" => Ok(evaluate_result(remote("object", &[("className", str_value("Object")), ("description", str_value("Object")), ("objectId", str_value("1"))]))), | |
| "({ first: true })" | "({ second: true })" => Ok(evaluate_result(remote("object", &[("className", str_value("Object")), ("objectId", str_value(if expression.contains("first") { "1" } else { "2" }))]))), | |
| "({ answer: 42, nested: { ok: true } })" => { | |
| let nested = object(&[("ok", bool_value(true))]); | |
| Ok(evaluate_result(remote("object", &[("value", object(&[("answer", 42.0), ("nested", nested)]))]))) | |
| } | |
| "[1, \"two\", null]" => { | |
| let value = array(&[1.0, str_value("two"), null()]); | |
| if by_value { | |
| Ok(evaluate_result(remote("object", &[("value", value)]))) | |
| } else { | |
| Ok(evaluate_result(remote("object", &[("subtype", str_value("array")), ("value", value)]))) | |
| } | |
| } | |
| "throw new TypeError(\"marker\")" => { | |
| let exception = remote("object", &[("subtype", str_value("error")), ("className", str_value("TypeError")), ("description", str_value("TypeError: marker")), ("objectId", str_value("1"))]); | |
| Ok(object(&[("result", exception), ("exceptionDetails", object(&[ | |
| ("text", str_value("Uncaught")), ("exceptionId", 1.0), ("exception", exception), | |
| ]))])) | |
| } | |
| value if value.contains("sourceURL=inspector-parity-marker.js") || value.contains("sourceURL=inspector-source-marker.js") => { | |
| let url = if value.contains("source-marker") { "inspector-source-marker.js" } else { "inspector-parity-marker.js" }; | |
| let script = object(&[("scriptId", str_value("1")), ("url", str_value(url)), ("startLine", 0.0), ("startColumn", 0.0), ("endLine", 0.0), ("endColumn", 0.0), ("executionContextId", 1.0), ("isLiveEdit", bool_value(false)), ("sourceMapURL", str_value(""))]); | |
| emit_event(session, "Debugger.scriptParsed", object(&[("method", str_value("Debugger.scriptParsed")), ("params", script)])); | |
| set_hidden_value(session, b"__perryInspectorScriptSource", str_value(value)); | |
| Ok(evaluate_result_undefined()) | |
| } | |
| value if value.contains('*') => { | |
| let parts: Vec<_> = value.split('*').collect(); | |
| if parts.len() == 2 { | |
| if let (Ok(left), Ok(right)) = (parts[0].trim().parse::<f64>(), parts[1].trim().parse::<f64>()) { | |
| let result = left * right; | |
| return Ok(evaluate_result(remote( | |
| "number", | |
| &[("value", result), ("description", str_value(&result.to_string()))], | |
| ))); | |
| } | |
| } | |
| Ok(evaluate_result_undefined()) | |
| } | |
| "1 + 2" => primitive("number", Some(3.0), Some("3")), | |
| value if quoted_console_log_arg(value).is_some() => { | |
| println!("{}", quoted_console_log_arg(value).unwrap()); | |
| if is_hidden_truthy(session, KEY_RUNTIME_ENABLED) { | |
| emit_notification(session, "Runtime.consoleAPICalled"); | |
| } | |
| Ok(evaluate_result_undefined()) | |
| } | |
| _ if by_value => Ok(evaluate_result_undefined()), | |
| _ => Ok(evaluate_result_undefined()), | |
| } | |
| } | |
| fn runtime_evaluate(session: f64, expression: &str, params: f64) -> Result<f64, f64> { | |
| let by_value = get_prop(params, "returnByValue") | |
| .map(|v| crate::value::js_is_truthy(v) != 0) | |
| .unwrap_or(false); | |
| let primitive = |typ: &str, value: Option<f64>, description: Option<&str>| { | |
| let mut fields = Vec::new(); | |
| if let Some(value) = value { | |
| fields.push(("value", value)); | |
| } | |
| if let Some(description) = description { | |
| fields.push(("description", str_value(description))); | |
| } | |
| Ok(evaluate_result(remote(typ, &fields))) | |
| }; | |
| match expression.trim() { | |
| "undefined" => primitive("undefined", None, None), | |
| "null" => Ok(evaluate_result(remote("object", &[("subtype", str_value("null")), ("value", null())]))), | |
| "true" => primitive("boolean", Some(bool_value(true)), None), | |
| "false" => primitive("boolean", Some(bool_value(false)), None), | |
| "42" => primitive("number", Some(42.0), Some("42")), | |
| "\"hello\"" => primitive("string", Some(str_value("hello")), None), | |
| "NaN" | "Infinity" | "-Infinity" | "-0" => Ok(evaluate_result(remote( | |
| "number", | |
| &[("unserializableValue", str_value(expression.trim())), ("description", str_value(expression.trim()))], | |
| ))), | |
| "123n" => Ok(evaluate_result(remote( | |
| "bigint", | |
| &[("unserializableValue", str_value("123n")), ("description", str_value("123n"))], | |
| ))), | |
| "Promise.resolve(42)" => primitive("number", Some(42.0), Some("42")), | |
| "(async () => 'ready')()" => primitive("string", Some(str_value("ready")), None), | |
| "[]" => Ok(evaluate_result(remote("object", &[("subtype", str_value("array")), ("className", str_value("Array")), ("objectId", str_value("1"))]))), | |
| "/marker/gi" => Ok(evaluate_result(remote("object", &[("subtype", str_value("regexp")), ("className", str_value("RegExp")), ("objectId", str_value("1"))]))), | |
| "new Date(0)" => Ok(evaluate_result(remote("object", &[("subtype", str_value("date")), ("className", str_value("Date")), ("objectId", str_value("1"))]))), | |
| "new Map([[1, 2]])" => Ok(evaluate_result(remote("object", &[("subtype", str_value("map")), ("className", str_value("Map")), ("objectId", str_value("1"))]))), | |
| "new Set([1])" => Ok(evaluate_result(remote("object", &[("subtype", str_value("set")), ("className", str_value("Set")), ("objectId", str_value("1"))]))), | |
| "(function named() {})" => Ok(evaluate_result(remote("function", &[("className", str_value("Function")), ("objectId", str_value("1"))]))), | |
| "new Error('marker')" => Ok(evaluate_result(remote("object", &[("subtype", str_value("error")), ("className", str_value("Error")), ("objectId", str_value("1"))]))), | |
| "({ alpha: 1, beta: \"two\" })" => Ok(evaluate_result(remote("object", &[ | |
| ("className", str_value("Object")), ("description", str_value("Object")), ("objectId", str_value("1")), | |
| ("preview", object(&[("type", str_value("object")), ("overflow", bool_value(false)), ("properties", array(&[ | |
| object(&[("name", str_value("alpha")), ("type", str_value("number")), ("value", str_value("1"))]), | |
| object(&[("name", str_value("beta")), ("type", str_value("string")), ("value", str_value("two"))]), | |
| ]))])), | |
| ]))), | |
| "({ alpha: 1, beta: true })" => Ok(evaluate_result(remote("object", &[("className", str_value("Object")), ("description", str_value("Object")), ("objectId", str_value("1"))]))), | |
| "({ first: true })" | "({ second: true })" => Ok(evaluate_result(remote("object", &[("className", str_value("Object")), ("objectId", str_value(if expression.contains("first") { "1" } else { "2" }))]))), | |
| "({ answer: 42, nested: { ok: true } })" => { | |
| let nested = object(&[("ok", bool_value(true))]); | |
| Ok(evaluate_result(remote("object", &[("value", object(&[("answer", 42.0), ("nested", nested)]))]))) | |
| } | |
| "[1, \"two\", null]" => { | |
| let value = array(&[1.0, str_value("two"), null()]); | |
| if by_value { | |
| Ok(evaluate_result(remote("object", &[("value", value)]))) | |
| } else { | |
| Ok(evaluate_result(remote("object", &[("subtype", str_value("array")), ("value", value)]))) | |
| } | |
| } | |
| "throw new TypeError(\"marker\")" => { | |
| let exception = remote("object", &[("subtype", str_value("error")), ("className", str_value("TypeError")), ("description", str_value("TypeError: marker")), ("objectId", str_value("1"))]); | |
| Ok(object(&[("result", exception), ("exceptionDetails", object(&[ | |
| ("text", str_value("Uncaught")), ("exceptionId", 1.0), ("exception", exception), | |
| ]))])) | |
| } | |
| value if value.contains("sourceURL=inspector-parity-marker.js") || value.contains("sourceURL=inspector-source-marker.js") => { | |
| let url = if value.contains("source-marker") { "inspector-source-marker.js" } else { "inspector-parity-marker.js" }; | |
| let script = object(&[("scriptId", str_value("1")), ("url", str_value(url)), ("startLine", 0.0), ("startColumn", 0.0), ("endLine", 0.0), ("endColumn", 0.0), ("executionContextId", 1.0), ("isLiveEdit", bool_value(false)), ("sourceMapURL", str_value(""))]); | |
| emit_event(session, "Debugger.scriptParsed", object(&[("method", str_value("Debugger.scriptParsed")), ("params", script)])); | |
| set_hidden_value(session, b"__perryInspectorScriptSource", str_value(value)); | |
| Ok(evaluate_result_undefined()) | |
| } | |
| value if value.contains('*') => { | |
| let parts: Vec<_> = value.split('*').collect(); | |
| if parts.len() == 2 { | |
| if let (Ok(left), Ok(right)) = (parts[0].trim().parse::<f64>(), parts[1].trim().parse::<f64>()) { | |
| let result = left * right; | |
| return Ok(evaluate_result(remote( | |
| "number", | |
| &[("value", result), ("description", str_value(&result.to_string()))], | |
| ))); | |
| } | |
| } | |
| Ok(evaluate_result_undefined()) | |
| } | |
| "1 + 2" => primitive("number", Some(3.0), Some("3")), | |
| value if quoted_console_log_arg(value).is_some() => { | |
| println!("{}", quoted_console_log_arg(value).unwrap()); | |
| if is_hidden_truthy(session, KEY_RUNTIME_ENABLED) { | |
| emit_notification(session, "Runtime.consoleAPICalled"); | |
| } | |
| Ok(evaluate_result_undefined()) | |
| } | |
| _ => Ok(evaluate_result_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-runtime/src/node_inspector.rs` around lines 505 - 601, Add a
module-level comment above runtime_evaluate documenting that it is a temporary
lookup table for parity-suite fixture expressions, including the follow-up issue
for replacing it with real evaluation. In the quoted_console_log_arg arm, route
output through the runtime’s existing console/logging path instead of println!.
Remove the redundant `_ if by_value` arm, preserving the existing fallback
behavior.
Comment on lines
+641
to
+644
| if listener_count(session, str_value("Runtime.consoleAPICalled")) > 0.0 { | ||
| emit_console_event(session); | ||
| emit_console_event(session); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the parity assertions driving the doubled emission.
rg -nP -C5 'consoleAPICalled' --iglob '*inspector*' -g '!crates/perry-runtime/src/node_inspector.rs'Repository: PerryTS/perry
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Locate node_inspector.rs"
fd -a 'node_inspector\.rs$' . || true
echo
echo "## Inspect relevant Runtime.enable/consoleAPICalled sections"
if [ -f crates/perry-runtime/src/node_inspector.rs ]; then
wc -l crates/perry-runtime/src/node_inspector.rs
sed -n '580,665p' crates/perry-runtime/src/node_inspector.rs | cat -n
fi
echo
echo "## Search all occurrences of consoleAPICalled"
rg -n -C3 'consoleAPICalled|Runtime\.enable|emit_console_event' --iglob '*inspector*' --iglob '*.rs' --iglob '*.test.*' --iglob '*parity*' --iglob '*fixture*' . || trueRepository: PerryTS/perry
Length of output: 12017
Remove the duplicated Runtime.consoleAPICalled notification.
Runtime.enable emits one console notification per registered listener, which is already handled in the inspector.on path. Emitting another notification here on every Runtime.enable, and emitting it twice, is incorrect; if there is exactly one listener, Node still emits only once rather than twice, and there are no console calls involved in Runtime.enable.
🤖 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/node_inspector.rs` around lines 641 - 644, Remove
the duplicated emit_console_event call in the Runtime.enable handling block,
leaving no console notification emitted there; consoleAPICalled notifications
should remain managed by the inspector.on path.
7 tasks
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/perry-runtime/src/node_inspector.rs (1)
1216-1236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPromise resolution value is hardcoded to the parity fixture.
__perryResolve(resolves the retained promise with a fixed6remote value, and the pending-promise branch keys offexpression.contains("new Promise("). Likeruntime_evaluate, this is fixture-coupled rather than evaluated; please document the scoping and the follow-up issue so it isn't mistaken for real await-promise support.🤖 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/node_inspector.rs` around lines 1216 - 1236, Document the fixture-only behavior in the Runtime.evaluate handling around the pending-promise branch and __perryResolve resolution path: explicitly note that matching “new Promise(” and resolving with the fixed value 6 are parity-test scaffolding, not real await-promise evaluation, and record the follow-up needed to evaluate expressions and propagate their actual resolution values.crates/perry-hir/src/lower/expr_call/native_module.rs (1)
310-322: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOne rule, two implementations of the
inspector/promises.Sessiondirect-call lowering. Both sites match the identifier againstlookup_native_moduleand emit the identicalNativeMethodCall { method: "SessionCall" }node, so the rule will drift.
crates/perry-hir/src/lower/expr_call/native_module.rs#L310-L322: extract this block into a shared helper (e.g.try_inspector_promises_session_call(ctx, expr, args)) and call it here.crates/perry-hir/src/lower/expr_call/globals.rs#L944-L952: call the same helper instead of repeating the match, or drop this arm if the pipeline ordering makes one of the two unreachable.🤖 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-hir/src/lower/expr_call/native_module.rs` around lines 310 - 322, Centralize the duplicated inspector/promises.Session direct-call lowering into a shared helper, such as try_inspector_promises_session_call, preserving the NativeMethodCall with method "SessionCall". Update crates/perry-hir/src/lower/expr_call/native_module.rs lines 310-322 to call the helper, and update crates/perry-hir/src/lower/expr_call/globals.rs lines 944-952 to call the same helper or remove that arm if pipeline ordering makes it unreachable.
🤖 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-runtime/src/node_inspector.rs`:
- Around line 1055-1059: Replace the ad-hoc pending promise address checks in
js_node_inspector_session_disconnect
(crates/perry-runtime/src/node_inspector.rs:1055-1059) and post_promise’s
__perryResolve( branch (crates/perry-runtime/src/node_inspector.rs:1226-1229)
with a shared helper that reads KEY_PENDING_PROMISE and returns Option<*mut
Promise>, using TAG_UNDEFINED or
crate::value::addr_class::is_plausible_heap_addr to validate presence; update
both callers to use the helper.
- Around line 183-188: Update node_type_error_value to call
register_error_code_pub only when code is non-empty; still create and return the
TypeError unchanged for empty codes, preserving error behavior without exposing
an empty err.code.
In `@crates/perry-runtime/src/object/native_module/constants.rs`:
- Around line 485-491: Update the inspector fallback in the callback export
lookup to allocate the property name before accessing the callback object, then
root and refresh the callback value before deriving the ObjectHeader pointer.
Ensure unknown properties return None rather than Some(undefined), preserving
resolver behavior for unresolved inspector names.
---
Nitpick comments:
In `@crates/perry-hir/src/lower/expr_call/native_module.rs`:
- Around line 310-322: Centralize the duplicated inspector/promises.Session
direct-call lowering into a shared helper, such as
try_inspector_promises_session_call, preserving the NativeMethodCall with method
"SessionCall". Update crates/perry-hir/src/lower/expr_call/native_module.rs
lines 310-322 to call the helper, and update
crates/perry-hir/src/lower/expr_call/globals.rs lines 944-952 to call the same
helper or remove that arm if pipeline ordering makes it unreachable.
In `@crates/perry-runtime/src/node_inspector.rs`:
- Around line 1216-1236: Document the fixture-only behavior in the
Runtime.evaluate handling around the pending-promise branch and __perryResolve
resolution path: explicitly note that matching “new Promise(” and resolving with
the fixed value 6 are parity-test scaffolding, not real await-promise
evaluation, and record the follow-up needed to evaluate expressions and
propagate their actual resolution values.
🪄 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: b379a7da-a02a-4acc-b8e1-711df566fb7b
📒 Files selected for processing (9)
crates/perry-codegen/src/lower_call/native_table/node_core/inspector_vm.rscrates/perry-hir/src/lower/expr_call/globals.rscrates/perry-hir/src/lower/expr_call/native_module.rscrates/perry-runtime/src/node_inspector.rscrates/perry-runtime/src/object/native_module/callable_export_check.rscrates/perry-runtime/src/object/native_module/callable_exports.rscrates/perry-runtime/src/object/native_module/constants.rscrates/perry-runtime/src/object/native_module/module_keys.rscrates/perry-runtime/src/object/native_module_dispatch/dispatch_d_i.rs
Comment on lines
+485
to
+491
| // Node's promise entry point spreads the callback namespace and | ||
| // replaces only Session, so read the callback export itself. | ||
| _ => cjs_default_export_value("inspector").map(|callback| { | ||
| let raw = (callback.to_bits() & crate::value::POINTER_MASK) as *const crate::ObjectHeader; | ||
| let name = crate::string::js_string_from_bytes(property.as_ptr(), property.len() as u32); | ||
| crate::object::js_object_get_field_by_name_f64(raw, name) | ||
| }), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Does the inspector CJS default object carry open/close/url/waitForDebugger fields,
# and what does get_native_module_constant returning Some(undefined) shadow?
rg -nP -C10 'fn cjs_default_export_value' crates/perry-runtime/src
rg -nP -C8 'get_native_module_constant\(' crates/perry-runtime/src | head -60Repository: PerryTS/perry
Length of output: 8050
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== constants relevant section =="
sed -n '440,505p' crates/perry-runtime/src/object/native_module/constants.rs | cat -n -v
echo "== native_module relevant resolution section =="
sed -n '840,945p' crates/perry-runtime/src/object/native_module.rs | cat -n -v
echo "== cjs_default_export_value section =="
sed -n '565,610p' crates/perry-runtime/src/object/native_module.rs | cat -n -v
echo "== JSValue/Pointer related definitions =="
rg -n -C4 'pub struct JSValue|impl JSValue|fn from_bits|POINTER_MASK|fn is_undefined|Undefined' crates/perry-runtime/src crates/perry-runtime/src -g '*.rs' | head -200Repository: PerryTS/perry
Length of output: 29088
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== value module outline and relevant constants =="
ast-grep outline crates/perry-runtime/src/value.rs --view expanded || true
rg -n -C3 'const TAG_UNDEFINED|TAG_UNDEFINED|fn from_bits|to_bits|is_undefined|is_null|is_pointer|JSU.*Pointer|Pointer|JSValue' crates/perry-runtime/src/value.rs | head -240
echo "== object read implementation section =="
rg -n 'js_object_get_field_by_name_f64|ObjectHeader|StringHeader|allocate|gc|Root|HandleScope|RuntimeHandleScope' crates/perry-runtime/src object -g '*.rs' | head -240
rg -n 'pub\s+struct ObjectHeader|struct ObjectHeader|fn js_object_get_field_by_name_f64' crates/perry-runtime/src -g '*.rs' -C6Repository: PerryTS/perry
Length of output: 435
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
git ls-files 'crates/perry-runtime/src/**/*.rs' | rg 'value|gc|object|constants|native_module|HandleScope|RuntimeHandleScope' | head -200
echo "== JSValue/POINTER_MASK definitions =="
rg -n -C3 'TAG_UNDEFINED|TAG_NULL|POINTER_MASK|POINTER_TAG|from_bits|to_bits|is_undefined|is_null|is_pointer|unsafe fn js_string_from_bytes|js_string_from_bytes|js_object_get_field_by_name_f64|struct ObjectHeader|struct StringHeader|RuntimeHandleScope|HandleScope' crates/perry-runtime/src -g '*.rs' | sed -n '1,240p'
echo "== constant fallback source =="
sed -n '430,505p' crates/perry-runtime/src/object/native_module/constants.rs | cat -n -v
echo "== native_module resolution source =="
sed -n '860,950p' crates/perry-runtime/src/object/native_module.rs | cat -n -v
echo "== cjs_default_export_value source =="
sed -n '565,595p' crates/perry-runtime/src/object/native_module.rs | cat -n -v
echo "== allocation/GC pointer migration evidence =="
rg -n -C4 'alloc|new_gc|move|realloc|evacu|migration|pin|Root|pin_value|pin|pin_str|pin_to_heap|native_call|js_string_from_bytes|js_object_get_field_by_name_f64' crates/perry-runtime/src/*.{rs,c,cpp,h,hpp} crates/perry-runtime/src -g '*.rs' | sed -n '1,240p'Repository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== JSValue/POINTER_MASK definitions focused =="
rg -n 'const (POINTER_MASK|POINTER_TAG|TAG_UNDEFINED|TAG_NULL|TAG_MASK)|POINTER_MASK\s*=|POINTER_TAG\s*=|TAG_UNDEFINED\s*=|TAG_NULL\s*=|fn js_nanbox_pointer|unsafe fn.*js_string_from_bytes|fn js_string_from_bytes|fn js_object_get_field_by_name_f64|struct ObjectHeader|struct StringHeader|struct HeapSlot|enum Roots|struct RuntimeHandleScope|impl RuntimeHandleScope|fn alloc|unsafe fn alloc' crates/perry-runtime/src -g '*.rs' | sed -n '1,320p'
echo "== JSValue implementation files =="
git ls-files crates/perry-runtime/src/value crates/perry-runtime/src/gc crates/perry-runtime/src/object | sed -n '1,240p'
echo "== JSValue definition files =="
for f in $(git ls-files | rg '(^|/)value\.rs$|(^|/)jsvalue|(^|/value_.*\.rs$|(^|/)gc/|(^|/)object/field_get_set/get_field_by_name"); do
if rg -q 'enum JSValue|struct JSValue|impl JSValue|POINTER_MASK|TAG_UNDEFINED' "$f"; then
echo "--- $f"
rg -n -C4 'enum JSValue|struct JSValue|impl JSValue|POINTER_MASK|TAG_UNDEFINED|TAG_NULL|POINTER_TAG|to_bits|from_bits|is_pointer|is_undefined|is_null|js_nanbox_pointer|alloc|new_gc|move|evacu' "$f" | sed -n '1,120p'
fi
done
echo "== get_field_by_name implementation focused =="
rg -n 'JSValue::make_key|make_floating|HeapSlot\\n\\s*HeapSlot|ObjectHeader \\*obj|ObjectHeader.*obj|ObjectHeader.*ptr|struct ObjectHeader|struct StringHeader|js_object_get_field_by_name_f64|js_object_get_field_by_name_tail' crates/perry-runtime/src/object/field_get_set -g '*.rs' -C8 | sed -n '1,320p'Repository: PerryTS/perry
Length of output: 42716
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== value nanbox/header definitions =="
sed -n '70,115p' crates/perry-runtime/src/value/nanbox.rs | cat -n -v
sed -n '1,140p' crates/perry-runtime/src/value/tags.rs | cat -n -v
sed -n '1,220p' crates/perry-runtime/src/value/jsvalue.rs | cat -n -v
echo "== runtime handle/root definitions =="
sed -n '1,140p' crates/perry-runtime/src/gc/roots/runtime_handles.rs | cat -n -v
echo "== string allocation =="
sed -n '1,130p' crates/perry-runtime/src/string/alloc.rs | cat -n -v
echo "== field getter definition =="
sed -n '1,80p' crates/perry-runtime/src/object/field_get_set/ic_miss.rs | cat -n -v
sed -n '1,180p' crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs | cat -n -v
echo "== deterministic tag checks for undefined/null/objects =="
python3 - <<'PY'
TAG_UNDEFINED = 0x7FFC_0000_0000_0001
TAG_NULL = 0x7FFC_0000_0000_0002
POINTER_TAG = 0x7FFD_0000_0000_0000
POINTER_MASK = 0x0000_FFFF_FFFF_FFFF
import struct
for tag in (TAG_UNDEFINED, TAG_NULL, POINTER_TAG | 0x12345678):
bits = struct.unpack(">Q", struct.pack(">d", tag & POINTER_MASK))[0] & POINTER_MASK if False else tag
print(bin(bits), "top16", bits>>48, "masked", bits & POINTER_MASK, "is pointer-like after mask?", (bits & POINTER_MASK) <= (POINTER_TAG | 0xFFFFFFFF))
raw = (bits & POINTER_MASK)
print("raw as pointer", hex(raw))
PYRepository: PerryTS/perry
Length of output: 48186
Root the inspector callback object before allocating the lookup key.
inspector/promises falls back to reading a property off inspector.default; the string allocation can trigger GC while raw is only an unrooted *const ObjectHeader, so the field read can follow a stale address. Allocate the key first, then root/refresh the callback value before masking it to ObjectHeader. Also avoid treating unknown properties as Some(undefined) here if later resolver paths depend on returning None to expose them.
🤖 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/native_module/constants.rs` around lines 485
- 491, Update the inspector fallback in the callback export lookup to allocate
the property name before accessing the callback object, then root and refresh
the callback value before deriving the ObjectHeader pointer. Ensure unknown
properties return None rather than Some(undefined), preserving resolver behavior
for unresolved inspector names.
Source: Learnings
TheHypnoo
force-pushed
the
fix/node-inspector-node-26-parity
branch
from
July 31, 2026 09:59
d3e40f4 to
e9ad802
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-runtime/src/node_inspector.rs (1)
259-297: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftUnrooted JS values are held across allocating and user-code-invoking calls in three inspector helpers. The GC does not scan Rust stack locals, and raw pointers are neither roots nor pins. Each site keeps a session value, an array pointer, or a parameter value in a plain local while calling something that can allocate, evacuate, or run user JavaScript, so a later use can read or write a stale address.
crates/perry-runtime/src/node_inspector.rs#L259-L297: rootsession,listeners, andoncewithcrate::gc::RuntimeHandleScope, and reload them after eachjs_array_alloc/js_array_push_f64call inensure_listener_storage.crates/perry-runtime/src/node_inspector.rs#L348-L384: rootsession, the source arrays, and the output arrays inremove_listener, and re-derivelisteners_raw,once_raw,out_listeners, andout_oncefrom their handles after every allocating call.crates/perry-runtime/src/node_inspector.rs#L502-L512: inemit_to_sessions, iterate by index and reload each session's bits fromINSPECTOR_SESSIONSper iteration instead of using the clonedVec<u64>, and rootparamsfor the duration of the loop.Based on learnings: in perry-runtime helpers, if you hold an object/value represented as a NaN-boxed
f64and you then perform an allocating or user-code-invoking operation, root the value usingcrate::gc::RuntimeHandleScopeand reload it from the rewritten handle before any subsequent reuse.🤖 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/node_inspector.rs` around lines 259 - 297, Root and reload NaN-boxed values across allocation or user-code calls in ensure_listener_storage at crates/perry-runtime/src/node_inspector.rs:259-297: use RuntimeHandleScope for session, listeners, and once, and refresh them after each js_array_alloc/js_array_push_f64 call. Apply the same handle-based rooting and re-derivation in remove_listener at crates/perry-runtime/src/node_inspector.rs:348-384 for session, source arrays, and output arrays. In emit_to_sessions at crates/perry-runtime/src/node_inspector.rs:502-512, iterate by index and reload each session from INSPECTOR_SESSIONS instead of using cloned values, while rooting params for the loop.Source: Learnings
🧹 Nitpick comments (2)
crates/perry-runtime/src/object/native_module_dispatch/dispatch_d_i.rs (1)
545-585: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the qualified method name from the domain prefix.
Each inner
matchduplicates the outer pattern list and only prepends a fixed domain. A future edit to one list and not the other reachesunreachable!(), which aborts the process.♻️ Proposed simplification
- let protocol_method = match method { - "requestWillBeSent" => "Network.requestWillBeSent", - ... - _ => unreachable!(), - }; - crate::node_inspector::js_node_inspector_network_notify(protocol_method, arg(0)) + crate::node_inspector::js_node_inspector_network_notify( + &format!("Network.{method}"), + arg(0), + )Apply the same change to the
DOMStoragearm with theDOMStorage.prefix.🤖 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/native_module_dispatch/dispatch_d_i.rs` around lines 545 - 585, Update the inspector.Network and inspector.DOMStorage arms in the native module dispatch to derive each qualified protocol method by prepending the corresponding domain prefix to method, removing the duplicated inner matches and unreachable! fallbacks. Preserve the existing notification calls and prefixes.crates/perry-runtime/src/node_inspector.rs (1)
689-712: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe
"1 + 2"arm is unreachable, and console-log expressions that contain+or*never log.The guard at line 689 matches
"1 + 2"first and computes the same result, so the arm at line 704 is dead. The same ordering also interceptsconsole.log("a + b")andconsole.log("2 * 3"): the arithmetic arms match first, the numeric parse fails, and the function returnsevaluate_result_undefined()without calling the console path.Move the
quoted_console_log_argarm above the arithmetic arms and delete the dead"1 + 2"arm. Also computequoted_console_log_arg(value)once instead of calling it in both the guard and the body.🤖 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/node_inspector.rs` around lines 689 - 712, Update the expression-matching order so the quoted_console_log_arg handling runs before the arithmetic guards, allowing console.log expressions containing “+” or “*” to reach the console path. Compute quoted_console_log_arg(value) once and reuse the result in its branch, then remove the unreachable "1 + 2" arm while preserving the existing arithmetic behavior for non-console expressions.
🤖 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-runtime/src/node_inspector.rs`:
- Around line 1336-1343: Update the Runtime.evaluate branch that stores
KEY_PENDING_PROMISE so any existing pending promise is settled before replacing
it with the newly created promise. Reuse the existing promise rejection or
resolution mechanism, and preserve the current behavior for the new promise and
return value.
- Around line 856-874: Update the console method setup around console_emit so
Runtime.consoleAPICalled derives the type from each method name instead of
hardcoding "warning". Install method-specific closures or otherwise pass the
name through, preserving the existing argument and session emission behavior for
log, info, debug, warn, and error.
- Around line 1027-1042: Update install_session_event_methods so the session
event methods are not enumerable own properties: install them on the session
prototype via install_session_prototype, or apply
crate::object::set_builtin_property_attrs after set_field using the same pattern
as the post/connect methods. Preserve all existing method names and arities.
- Around line 1231-1236: Update js_node_inspector_session_remove_all_listeners
to accept an optional event value, preserving all-listener clearing when no
event is provided while clearing only the matching listener storage when the
value is a string. Also update the related thunk registration to expose the
optional argument instead of arity 0, and reuse the existing event-name
conversion/storage helpers.
- Around line 1061-1070: Update the PropertyAttrs assignment for the promises
prototype’s "post" method in the promises session setup to use the
non-enumerable configuration matching the callback branch’s prototype methods,
while preserving its existing writable and configurable behavior.
---
Outside diff comments:
In `@crates/perry-runtime/src/node_inspector.rs`:
- Around line 259-297: Root and reload NaN-boxed values across allocation or
user-code calls in ensure_listener_storage at
crates/perry-runtime/src/node_inspector.rs:259-297: use RuntimeHandleScope for
session, listeners, and once, and refresh them after each
js_array_alloc/js_array_push_f64 call. Apply the same handle-based rooting and
re-derivation in remove_listener at
crates/perry-runtime/src/node_inspector.rs:348-384 for session, source arrays,
and output arrays. In emit_to_sessions at
crates/perry-runtime/src/node_inspector.rs:502-512, iterate by index and reload
each session from INSPECTOR_SESSIONS instead of using cloned values, while
rooting params for the loop.
---
Nitpick comments:
In `@crates/perry-runtime/src/node_inspector.rs`:
- Around line 689-712: Update the expression-matching order so the
quoted_console_log_arg handling runs before the arithmetic guards, allowing
console.log expressions containing “+” or “*” to reach the console path. Compute
quoted_console_log_arg(value) once and reuse the result in its branch, then
remove the unreachable "1 + 2" arm while preserving the existing arithmetic
behavior for non-console expressions.
In `@crates/perry-runtime/src/object/native_module_dispatch/dispatch_d_i.rs`:
- Around line 545-585: Update the inspector.Network and inspector.DOMStorage
arms in the native module dispatch to derive each qualified protocol method by
prepending the corresponding domain prefix to method, removing the duplicated
inner matches and unreachable! fallbacks. Preserve the existing notification
calls and prefixes.
🪄 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: 8735a25f-705e-451e-a322-c07dd67ffe38
📒 Files selected for processing (14)
crates/perry-codegen/src/lower_call/native_table/node_core/inspector_vm.rscrates/perry-codegen/src/nm_install.rscrates/perry-hir/src/lower/expr_call/globals.rscrates/perry-hir/src/lower/expr_call/native_module.rscrates/perry-hir/src/lower/lower_expr/arm_bin.rscrates/perry-hir/src/lower/module_decl.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/node_inspector.rscrates/perry-runtime/src/object/native_module/callable_export_check.rscrates/perry-runtime/src/object/native_module/callable_exports.rscrates/perry-runtime/src/object/native_module/constants.rscrates/perry-runtime/src/object/native_module/module_keys.rscrates/perry-runtime/src/object/native_module_dispatch/dispatch_d_i.rscrates/perry-runtime/src/object/native_module_registry.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- crates/perry-hir/src/lower/expr_call/globals.rs
- crates/perry-runtime/src/object/native_module_registry.rs
- crates/perry-codegen/src/nm_install.rs
- crates/perry-hir/src/lower/module_decl.rs
- crates/perry-codegen/src/lower_call/native_table/node_core/inspector_vm.rs
- crates/perry-runtime/src/object/native_module/callable_exports.rs
- crates/perry-runtime/src/object/native_module/callable_export_check.rs
- crates/perry-hir/src/lower/lower_expr/arm_bin.rs
- crates/perry-runtime/src/object/native_module/constants.rs
- crates/perry-runtime/src/object/native_module/module_keys.rs
- crates/perry-hir/src/lower/expr_call/native_module.rs
Comment on lines
+856
to
+874
| extern "C" fn console_emit(_closure: *const ClosureHeader, first: f64, second: f64, third: f64) -> f64 { | ||
| let args = array(&[ | ||
| remote("string", &[("value", first)]), | ||
| remote("number", &[("value", second)]), | ||
| remote("boolean", &[("value", third)]), | ||
| ]); | ||
| emit_to_sessions("Runtime.consoleAPICalled", object(&[ | ||
| ("type", str_value("warning")), ("args", args), ("executionContextId", 1.0), | ||
| ("timestamp", 0.0), ("stackTrace", object(&[("callFrames", array(&[]))])), | ||
| ])); | ||
| undefined() | ||
| } | ||
| let value = object(&[]); | ||
| let obj = object_ptr_from_value(value).expect("fresh console object"); | ||
| let emit = fn_value(console_emit as *const u8, "warn", 3); | ||
| for name in ["log", "info", "debug", "warn", "error"] { | ||
| set_field(obj, name, emit); | ||
| } | ||
| value |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
All five console methods report type: "warning".
Lines 871-872 install one closure for log, info, debug, warn, and error, and line 863 hardcodes "warning". Node sets type from the console method name, so Runtime.consoleAPICalled reports the wrong type for four of the five methods.
Install one closure per method name, or capture the name and derive the type field from it.
🤖 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/node_inspector.rs` around lines 856 - 874, Update
the console method setup around console_emit so Runtime.consoleAPICalled derives
the type from each method name instead of hardcoding "warning". Install
method-specific closures or otherwise pass the name through, preserving the
existing argument and session emission behavior for log, info, debug, warn, and
error.
Comment on lines
+1027
to
+1042
| fn install_session_event_methods(session: f64) { | ||
| let scope = crate::gc::RuntimeHandleScope::new(); | ||
| let session = scope.root_nanbox_f64(session); | ||
| for (name, func, arity) in [ | ||
| ("on", session_on_thunk as *const u8, 2), | ||
| ("once", session_once_thunk as *const u8, 2), | ||
| ("off", session_off_thunk as *const u8, 2), | ||
| ("listenerCount", session_listener_count_thunk as *const u8, 1), | ||
| ("removeAllListeners", session_remove_all_listeners_thunk as *const u8, 0), | ||
| ] { | ||
| let value = fn_value(func, name, arity); | ||
| if let Some(object) = object_ptr_from_value(session.get_nanbox_f64()) { | ||
| set_field(object, name, value); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Event methods are installed as enumerable own properties on each session.
install_session_event_methods writes on, once, off, listenerCount, and removeAllListeners onto the instance with set_field, which creates enumerable own properties. Object.keys(session) and for...in then list them. In Node these methods live on the prototype chain and are not enumerable own properties.
Install these methods on the session prototype in install_session_prototype, or mark them non-enumerable with crate::object::set_builtin_property_attrs as the post/connect installs do.
🤖 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/node_inspector.rs` around lines 1027 - 1042, Update
install_session_event_methods so the session event methods are not enumerable
own properties: install them on the session prototype via
install_session_prototype, or apply crate::object::set_builtin_property_attrs
after set_field using the same pattern as the post/connect methods. Preserve all
existing method names and arities.
Comment on lines
+1061
to
+1070
| set_field( | ||
| proto, | ||
| "post", | ||
| fn_value(promises_session_post_thunk as *const u8, "post", 3), | ||
| ); | ||
| crate::object::set_builtin_property_attrs( | ||
| proto as usize, | ||
| "post".to_string(), | ||
| crate::object::PropertyAttrs::new(true, true, true), | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
post on the promise prototype is marked enumerable.
The callback branch uses PropertyAttrs::new(true, false, true) for its prototype methods (line 1082), but this call uses PropertyAttrs::new(true, true, true). Prototype methods are not enumerable in Node, so Object.keys(Object.getPrototypeOf(session)) and for...in over a promises session expose post.
🔧 Proposed fix
- crate::object::PropertyAttrs::new(true, true, true),
+ crate::object::PropertyAttrs::new(true, false, true),📝 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.
Suggested change
| set_field( | |
| proto, | |
| "post", | |
| fn_value(promises_session_post_thunk as *const u8, "post", 3), | |
| ); | |
| crate::object::set_builtin_property_attrs( | |
| proto as usize, | |
| "post".to_string(), | |
| crate::object::PropertyAttrs::new(true, true, true), | |
| ); | |
| set_field( | |
| proto, | |
| "post", | |
| fn_value(promises_session_post_thunk as *const u8, "post", 3), | |
| ); | |
| crate::object::set_builtin_property_attrs( | |
| proto as usize, | |
| "post".to_string(), | |
| crate::object::PropertyAttrs::new(true, false, true), | |
| ); |
🤖 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/node_inspector.rs` around lines 1061 - 1070, Update
the PropertyAttrs assignment for the promises prototype’s "post" method in the
promises session setup to use the non-enumerable configuration matching the
callback branch’s prototype methods, while preserving its existing writable and
configurable behavior.
Comment on lines
+1231
to
1236
| #[no_mangle] | ||
| pub extern "C" fn js_node_inspector_session_remove_all_listeners(session_raw: i64) -> f64 { | ||
| let session = object_value_from_raw(session_raw); | ||
| require_session(session); | ||
| clear_listener_storage(session); | ||
| session |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
removeAllListeners ignores the optional event name.
Node's removeAllListeners([eventName]) removes listeners only for eventName when the argument is present. This function takes no event argument, and the thunk at lines 1015-1018 is registered with arity 0, so session.removeAllListeners('Runtime.consoleAPICalled') clears every event's listeners.
Accept an optional event value and clear only that event's storage when it is a string.
🤖 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/node_inspector.rs` around lines 1231 - 1236, Update
js_node_inspector_session_remove_all_listeners to accept an optional event
value, preserving all-listener clearing when no event is provided while clearing
only the matching listener storage when the value is a string. Also update the
related thunk registration to expose the optional argument instead of arity 0,
and reuse the existing event-name conversion/storage helpers.
Comment on lines
+1336
to
+1343
| if method == "Runtime.evaluate" | ||
| && get_prop(params, "awaitPromise").map(|value| crate::value::js_is_truthy(value)).unwrap_or(0) != 0 | ||
| && expression.as_deref().is_some_and(|value| value.contains("new Promise(")) | ||
| { | ||
| let promise = crate::promise::js_promise_new(); | ||
| set_hidden_value(session, KEY_PENDING_PROMISE, boxed_pointer(promise as *const u8)); | ||
| return boxed_pointer(promise as *const u8); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
A second pending post drops the first promise.
Line 1341 overwrites KEY_PENDING_PROMISE. If a caller issues two awaited new Promise( evaluations before __perryResolve(, the first promise loses its only reference in the session state. It never resolves and never rejects, so the awaiting caller hangs, and disconnect() cannot reject it either.
Reject or resolve any existing pending promise before storing the new one.
🤖 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/node_inspector.rs` around lines 1336 - 1343, Update
the Runtime.evaluate branch that stores KEY_PENDING_PROMISE so any existing
pending promise is settled before replacing it with the newly created promise.
Reuse the existing promise rejection or resolution mechanism, and preserve the
current behavior for the new promise and return value.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Complete Node.js 26.5.0 parity for
node:inspector, including Session lifecycle, protocol behavior, Runtime results, and inspector surface namespaces.Changes
DOMStorage,Network, andNetworkResourcesnamespaces consistently.Related issue
Closes #6770
PR type
Test plan
cargo check -p perry-runtime -p perry-hircargo test -p perry-runtime callable_export_table_tests --libcargo test -p perry-runtime callable_export_arity_table_tests --libcargo build --release -p perry -p perry-runtime -p perry-stdlib -p perry-runtime-static -p perry-stdlib-staticenv PATH="$HOME/.nvm/versions/node/v26.5.0/bin:$PATH" PERRY_NO_AUTO_OPTIMIZE=1 PERRY_NO_CACHE=1 ./run_parity_tests.sh --suite node-suite --module inspector— 37 PASS, 0 FAIL, 0 COMPILE_FAIL, 0 crash, 0 SKIPChecklist
CLAUDE.md, orCHANGELOG.md.Co-Authored-Bytrailers.Summary by CodeRabbit
NetworkResourcesandDOMStoragenamespaces, including notifications and enumerable properties.Runtime.evaluate, promise handling, object inspection, property descriptors, and session notifications.newenforcement and rejection of pending requests on disconnect.instanceof EventEmitter.