fix(inspector): complete Node 26.5 parity - #7090
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (24)
📝 WalkthroughWalkthroughExpands Node ChangesNode inspector parity
Estimated code review effort: 5 (Critical) | ~90+ minutes Possibly related issues
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.
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
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.
proggeramlug
force-pushed
the
fix/node-inspector-node-26-parity
branch
from
August 1, 2026 06:32
e9ad802 to
2bf714a
Compare
proggeramlug
pushed a commit
to jdalton/perry
that referenced
this pull request
Aug 1, 2026
`every_dispatch_entry_has_manifest_counterpart` listed 14 missing rows at
origin/main; this branch as authored fixed 3 of them (dgram::sendto,
sqlite::serialize/deserialize). The other 11 landed on main *after* the
branch point, so merging main re-reds the very gate the branch exists to
turn green:
inspector::log/info/debug/warn/error, inspector/promises::SessionCall
— PerryTS#7090 (47040d5) inspector Node 26.5 parity
cluster::listeners/rawListeners/setMaxListeners/getMaxListeners
— PerryTS#7105 (76fa7b4) cluster Node 26 semantics
lru-cache::peek
— PerryTS#7136 (5f5006a) lru-cache faithful JS-value keys/values
Rows mirror how each surface is already modelled:
- the five `inspector.console.*` forwarders carry the dispatch table's
`class_filter: Some("console")`, so they hang off the existing
`property("inspector", "console")` instead of becoming named exports;
- `inspector/promises::SessionCall` is the synthetic row for `Session(...)`
called without `new` (js_node_inspector_session_call_without_new), so it
is `internal_method` — the .d.ts keeps advertising `Session`;
- the four `cluster` EventEmitter methods join their eleven siblings as
`internal_method` + paired `internal_property`, matching the PerryTS#3687 block
directly above them.
All rows use the plain `method`/`internal_method` helpers (params: &[],
returns: Any), which `manifest_param_counts_match_dispatch_table`
deliberately skips as the "no signature data" fallback — so no arity claim
is invented for them.
manifest_consistency: 5 passed / 0 failed (was 2 passed / 3 failed at
origin/main, 4 passed / 1 failed with only the original branch fix).
docs/src/api/reference.md regenerated: +6 rendered entries (the internal_*
rows stay out of the public surface, as cluster's existing ones already do);
docs/api/perry.d.ts is byte-unchanged, since nothing added is a named export.
proggeramlug
pushed a commit
that referenced
this pull request
Aug 1, 2026
…ble (#7159) * fix(api-manifest): reconcile API_MANIFEST with the native dispatch table The manifest_consistency drift guards in perry-codegen were red on main: - every_native_module_has_at_least_one_manifest_entry / #513: iovalkey (a NATIVE_MODULES member and well_known_bindings entry that normalizes to ioredis in dispatch) had zero API_MANIFEST rows, so strict mode never engaged for it. Add the Redis class + createClient rows, mirroring the redis alias. - every_dispatch_entry_has_manifest_counterpart: dgram Socket.sendto and sqlite DatabaseSync.serialize/deserialize existed in NATIVE_MODULE_TABLE with no manifest counterpart. Add the matching receiver-method rows. - every_well_known_binding_has_manifest_entry: the test's inline TOML parser misread the new [bindings.*.upstream] provenance sub-tables as binding names (dotenv.upstream, iovalkey.upstream, ...). Teach it to skip nested sub-tables so it only checks real routed module names. Regenerate docs/api/perry.d.ts and docs/src/api/reference.md from the manifest (api-docs-drift gate); this also folds in the previously uncommitted node-forge and tls.convertALPNProtocols entries. * chore(changelog): key the fragment to PR 7159 changelog.d fragments are named <PR>-<slug>.md (changelog.d/README.md), so in-flight PRs never collide. This one was keyed to the issue it fixes (0513) rather than the PR. Also confirms the merge resolution: regenerating both API-docs artifacts from the merged manifest reproduces the committed files byte-for-byte, and their only delta against main is the "Coverage:"/"Total:" header count — main's #7118 regenerated the artifacts without adding manifest rows, so nothing from main is dropped. * fix(api-manifest): cover the dispatch rows main added after this branch `every_dispatch_entry_has_manifest_counterpart` listed 14 missing rows at origin/main; this branch as authored fixed 3 of them (dgram::sendto, sqlite::serialize/deserialize). The other 11 landed on main *after* the branch point, so merging main re-reds the very gate the branch exists to turn green: inspector::log/info/debug/warn/error, inspector/promises::SessionCall — #7090 (47040d5) inspector Node 26.5 parity cluster::listeners/rawListeners/setMaxListeners/getMaxListeners — #7105 (76fa7b4) cluster Node 26 semantics lru-cache::peek — #7136 (5f5006a) lru-cache faithful JS-value keys/values Rows mirror how each surface is already modelled: - the five `inspector.console.*` forwarders carry the dispatch table's `class_filter: Some("console")`, so they hang off the existing `property("inspector", "console")` instead of becoming named exports; - `inspector/promises::SessionCall` is the synthetic row for `Session(...)` called without `new` (js_node_inspector_session_call_without_new), so it is `internal_method` — the .d.ts keeps advertising `Session`; - the four `cluster` EventEmitter methods join their eleven siblings as `internal_method` + paired `internal_property`, matching the #3687 block directly above them. All rows use the plain `method`/`internal_method` helpers (params: &[], returns: Any), which `manifest_param_counts_match_dispatch_table` deliberately skips as the "no signature data" fallback — so no arity claim is invented for them. manifest_consistency: 5 passed / 0 failed (was 2 passed / 3 failed at origin/main, 4 passed / 1 failed with only the original branch fix). docs/src/api/reference.md regenerated: +6 rendered entries (the internal_* rows stay out of the public surface, as cluster's existing ones already do); docs/api/perry.d.ts is byte-unchanged, since nothing added is a named export. * docs(changelog): note the post-branch dispatch rows in the 7159 fragment --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
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
2 participants
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
New Features
NetworkResourcesandDOMStoragenamespaces, notifications, and enumerable properties.log,info,debug,warn, anderror.instanceof EventEmittercompatibility and improved endpoint validation and URL lifecycle handling.Tests