fix(hir): propagate ws Client handle class across function-call boundaries - #5493
Merged
proggeramlug merged 8 commits intoJun 21, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA new pre-scan pass ( ChangesCross-function WS Client Handle Dispatch
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/lowering_context.rs`:
- Around line 196-204: The param_native_hints HashMap key uses only
function_name and parameter index, which is not unique when the same function
name exists in different scopes (shadowed or nested functions). Replace the
String function_name component in the (String, usize) key with a stable AST
identity such as a Span or eventually a FuncId mapping that can uniquely
identify each function declaration. This ensures that native instance hints
seeded for one function declaration in lower_fn_decl do not incorrectly apply to
unrelated declarations with the same name, preventing cross-contamination of
type hints like the ("ws","Client") dispatch mismatches.
In `@crates/perry-hir/src/lower/pre_scan.rs`:
- Around line 463-493: The `is_create_server_call` function currently accepts
any callee named `createServer` or `createSecureServer` without verifying it
originates from the Node.js HTTP modules. To fix this, you need to first scan
the module's imports to build a set of verified identifiers that are aliases or
direct references to `createServer`/`createSecureServer` from `node:http`,
`node:https`, or `node:http2` modules. Then modify `is_create_server_call` to
check whether the callee (for bare calls, the identifier; for member calls, the
object being accessed) is in this verified set of safe imports. This ensures
only actual HTTP server creation calls are detected, not user-defined factories
with the same name.
- Around line 310-315: The parameter name collection in the mapping operation
needs to skip TypeScript `this` parameters to align with how `lower_fn_decl`
handles them. In the code where `fd.function.params` is being iterated and names
are collected via `cross_fn_pat_name`, add a filter to exclude parameters where
the extracted name equals "this". This same fix should also be applied to the
similar code block mentioned at lines 390-396 to ensure consistency across all
parameter enumeration paths and prevent hint index misalignment.
- Around line 645-659: The collect_calls_in_callback_body function and related
collectors are descending into nested function/arrow scopes while maintaining
taint information from outer scopes, causing incorrect tainting when parameters
are shadowed in nested functions. Modify the collectors to not recurse into
nested function bodies (both ast::Expr::Fn and nested arrows within callback
bodies) so that taint analysis respects scope boundaries and does not inherit
outer scope bindings through shadowed parameters. This applies to
collect_calls_in_callback_body, the collectors called from it, and any other
places where nested functions are recursively analyzed without scope separation.
🪄 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: 092b0dca-d37b-495e-8bae-e547184ec445
📒 Files selected for processing (6)
crates/perry-hir/src/lower/context.rscrates/perry-hir/src/lower/lower_module_fn.rscrates/perry-hir/src/lower/lowering_context.rscrates/perry-hir/src/lower/pre_scan.rscrates/perry-hir/src/lower_decl/fn_decl.rscrates/perry/tests/ws_client_handle_cross_function_dispatch.rs
Contributor
Author
|
|
This command requires write access to the repository. Only users with write or admin permissions can trigger CodeRabbit to commit or create pull requests. |
machineloop
added a commit
to machineloop/perry
that referenced
this pull request
Jun 21, 2026
CodeRabbit (PR PerryTS#5493): lower_fn_decl drops the `this:` type-only param before enumerating params, but the pre-scan kept it, so the hint index and propagated param name diverged for `function f(this: T, req, wsId)`. Skip `this` in the pre-scan param list to match — call sites never pass it positionally.
machineloop
added a commit
to machineloop/perry
that referenced
this pull request
Jun 21, 2026
CodeRabbit (PR PerryTS#5493): is_create_server_call matched any callee named createServer/createSecureServer (user factories, unrelated obj.createServer()), which could seed ("ws","Client") taint from a non-HTTP object. Restrict the seed to createServer/createSecureServer that resolve to a node:http / node:https / node:http2 import or require alias, mirroring the runtime's HttpServer provenance.
machineloop
added a commit
to machineloop/perry
that referenced
this pull request
Jun 21, 2026
CodeRabbit (PR PerryTS#5493): param_native_hints keyed by (function_name, param_index) let a hint seeded for a top-level helper mis-apply to an unrelated same-named nested/redeclared function in lower_fn_decl. Key by the function declaration's identifier span (a stable AST identity) at both the record and apply sites, so distinct declarations get distinct keys.
machineloop
added a commit
to machineloop/perry
that referenced
this pull request
Jun 21, 2026
…ugh shadowing CodeRabbit (PR PerryTS#5493): the call collectors descended into nested function/arrow bodies with a flat name-keyed taint set, so a nested `function later(wsId){…}` that shadows the outer upgrade handle would mis-tag `later`'s callees. Replace the flat collect-then-check with a scope-aware walk that drops any name a nested scope rebinds as a parameter before recursing — preserving legitimate captures (`wsId.on('message', m => handleConnection(req, wsId))`) while respecting shadowing.
machineloop
added a commit
to machineloop/perry
that referenced
this pull request
Jun 21, 2026
…5493) Regression tests for the four CodeRabbit findings: a leading TS `this:` param (index alignment), a non-http `createServer` (seed gating), a shadowing nested param (scope-aware taint), and a same-named nested function (identity keying).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-hir/src/lower/pre_scan.rs (1)
544-716: 💤 Low valueMinor gap:
ast::Expr::ClassandTsSatisfiesnot traversed.The catch-all silently skips class expressions (which can contain method bodies) and TypeScript's
satisfieswrapper. In practice, these patterns are rare inside upgrade handlers, but if a tainted handle flows through them the hint won't propagate.Likely acceptable for this PR scope given the low probability of these patterns in real-world upgrade callbacks.
🤖 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/pre_scan.rs` around lines 544 - 716, The walk_taint_expr function's match statement has a catch-all arm that silently skips ast::Expr::Class and ast::Expr::TsSatisfies expressions, preventing taint propagation through these patterns. Add explicit match arms before the catch-all for these two cases: for ast::Expr::Class, traverse the class body methods similar to how ast::Expr::Object handles method properties by pruning parameters, and for ast::Expr::TsSatisfies, traverse the inner expression like the other TypeScript assertion types (TsAs, TsNonNull, etc.) at the end of the match.
🤖 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.
Nitpick comments:
In `@crates/perry-hir/src/lower/pre_scan.rs`:
- Around line 544-716: The walk_taint_expr function's match statement has a
catch-all arm that silently skips ast::Expr::Class and ast::Expr::TsSatisfies
expressions, preventing taint propagation through these patterns. Add explicit
match arms before the catch-all for these two cases: for ast::Expr::Class,
traverse the class body methods similar to how ast::Expr::Object handles method
properties by pruning parameters, and for ast::Expr::TsSatisfies, traverse the
inner expression like the other TypeScript assertion types (TsAs, TsNonNull,
etc.) at the end of the match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 23750ec6-304f-4b19-a04b-e1dad24b1a09
📒 Files selected for processing (4)
crates/perry-hir/src/lower/lowering_context.rscrates/perry-hir/src/lower/pre_scan.rscrates/perry-hir/src/lower_decl/fn_decl.rscrates/perry/tests/ws_client_handle_cross_function_dispatch.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/perry-hir/src/lower_decl/fn_decl.rs
- crates/perry-hir/src/lower/lowering_context.rs
machineloop
force-pushed
the
fix/ws-client-handle-scope
branch
from
June 21, 2026 14:43
ae82da3 to
9b1e63d
Compare
machineloop
added a commit
to machineloop/perry
that referenced
this pull request
Jun 21, 2026
CodeRabbit (PR PerryTS#5493): lower_fn_decl drops the `this:` type-only param before enumerating params, but the pre-scan kept it, so the hint index and propagated param name diverged for `function f(this: T, req, wsId)`. Skip `this` in the pre-scan param list to match — call sites never pass it positionally.
machineloop
added a commit
to machineloop/perry
that referenced
this pull request
Jun 21, 2026
CodeRabbit (PR PerryTS#5493): is_create_server_call matched any callee named createServer/createSecureServer (user factories, unrelated obj.createServer()), which could seed ("ws","Client") taint from a non-HTTP object. Restrict the seed to createServer/createSecureServer that resolve to a node:http / node:https / node:http2 import or require alias, mirroring the runtime's HttpServer provenance.
machineloop
added a commit
to machineloop/perry
that referenced
this pull request
Jun 21, 2026
CodeRabbit (PR PerryTS#5493): param_native_hints keyed by (function_name, param_index) let a hint seeded for a top-level helper mis-apply to an unrelated same-named nested/redeclared function in lower_fn_decl. Key by the function declaration's identifier span (a stable AST identity) at both the record and apply sites, so distinct declarations get distinct keys.
machineloop
added a commit
to machineloop/perry
that referenced
this pull request
Jun 21, 2026
…ugh shadowing CodeRabbit (PR PerryTS#5493): the call collectors descended into nested function/arrow bodies with a flat name-keyed taint set, so a nested `function later(wsId){…}` that shadows the outer upgrade handle would mis-tag `later`'s callees. Replace the flat collect-then-check with a scope-aware walk that drops any name a nested scope rebinds as a parameter before recursing — preserving legitimate captures (`wsId.on('message', m => handleConnection(req, wsId))`) while respecting shadowing.
machineloop
added a commit
to machineloop/perry
that referenced
this pull request
Jun 21, 2026
…5493) Regression tests for the four CodeRabbit findings: a leading TS `this:` param (index alignment), a non-http `createServer` (seed gating), a shadowing nested param (scope-aware taint), and a same-named nested function (identity keying).
…aries
The `("ws","Client")` handle delivered to a WebSocket-upgrade callback
`server.on("upgrade", (req, wsId, head) => …)` only dispatched its
`.send()`/`.on()`/`.close()` to the dedicated `js_ws_*_client_i64` runtime
when codegen statically knew the receiver was the upgrade Client. That class
is tagged at the upgrade callback's parameter, but was NOT propagated when
`wsId` was passed to a helper (`handleConnection(req, wsId)`): inside the
callee the parameter is untyped, the dispatch table's `class_filter:
Some("Client")` rows no longer matched, and `wsId.send(...)` silently lowered
to a generic no-op — the frame was dropped with no error thrown.
Adds `pre_scan_cross_fn_native_params`, a pre-lowering pass that seeds from
the HTTP-upgrade idiom (gated on the receiver being a `createServer(...)`
result, mirroring the runtime `("http","HttpServer")` check) and follows the
handle through subsequent `userFn(…, wsId, …)` calls transitively, recording a
`(fn_name, param_index) -> (module, class)` hint. `lower_fn_decl` applies the
hint (only when the param isn't already a native instance, so explicit
annotations win) before the body lowers, so the callee's `wsId.send(...)`
dispatches to `js_ws_send_client_i64` exactly like the inline call.
Regression test (ws_client_handle_cross_function_dispatch) compiles to LLVM IR
and asserts the helper/transitive/.on/.close call sites lower to the
`js_ws_*_client_i64` runtime, plus a control proving a non-ws `.send` param
keeps running its own method (no mis-tagging).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CodeRabbit (PR PerryTS#5493): lower_fn_decl drops the `this:` type-only param before enumerating params, but the pre-scan kept it, so the hint index and propagated param name diverged for `function f(this: T, req, wsId)`. Skip `this` in the pre-scan param list to match — call sites never pass it positionally.
CodeRabbit (PR PerryTS#5493): is_create_server_call matched any callee named createServer/createSecureServer (user factories, unrelated obj.createServer()), which could seed ("ws","Client") taint from a non-HTTP object. Restrict the seed to createServer/createSecureServer that resolve to a node:http / node:https / node:http2 import or require alias, mirroring the runtime's HttpServer provenance.
CodeRabbit (PR PerryTS#5493): param_native_hints keyed by (function_name, param_index) let a hint seeded for a top-level helper mis-apply to an unrelated same-named nested/redeclared function in lower_fn_decl. Key by the function declaration's identifier span (a stable AST identity) at both the record and apply sites, so distinct declarations get distinct keys.
…ugh shadowing CodeRabbit (PR PerryTS#5493): the call collectors descended into nested function/arrow bodies with a flat name-keyed taint set, so a nested `function later(wsId){…}` that shadows the outer upgrade handle would mis-tag `later`'s callees. Replace the flat collect-then-check with a scope-aware walk that drops any name a nested scope rebinds as a parameter before recursing — preserving legitimate captures (`wsId.on('message', m => handleConnection(req, wsId))`) while respecting shadowing.
…5493) Regression tests for the four CodeRabbit findings: a leading TS `this:` param (index alignment), a non-http `createServer` (seed gating), a shadowing nested param (scope-aware taint), and a same-named nested function (identity keying).
machineloop
force-pushed
the
fix/ws-client-handle-scope
branch
from
June 21, 2026 14:44
9b1e63d to
9e56613
Compare
… handle The cross-fn propagation tags a function PARAMETER, which fixes the dispatch for that param across EVERY call site of the function — not just the upgrade-fed one. A helper reused for both the upgrade `wsId` and a non-ws value would therefore have its non-ws caller's `.send()/.on()/.close()` silently re-routed to `js_ws_*_client_i64` (a handle-map miss → dropped frame, the exact bug this pass fixes) and its own method bypassed. Add a polymorphism guard: after the taint walk, keep a hint only when the function is provably always handed the ws handle at that parameter. A function with any non-ws caller (or a spread-arg caller, whose positional mapping is unreliable) is dropped. The caller scan is scope-aware so a nested function that SHADOWS a top-level name is not mistaken for a call to the top-level one. Also propagate the guard transitively: a hint reached by following the handle THROUGH an upstream param depends on that param, so a polymorphic intermediate demotes everything downstream of it (dependency fixpoint). Tests: a polymorphic direct helper, a polymorphic intermediate (transitive), and a ws-exclusive helper with multiple ws callers (guardrail against over-correction). Also drops a stray `if` paren that tripped an unused_parens warning. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…erryTS#5493 review) CodeRabbit nitpick: walk_taint_expr's catch-all silently skipped ast::Expr::Class (method bodies can carry a tainted handle) and TypeScript's `satisfies` wrapper. Traverse class method/constructor bodies (params pruned like object methods) + field initializers + static blocks, and unwrap TsSatisfies like the other TS assertion wrappers.
Contributor
Author
|
Addressed the remaining nitpick ( |
Merged
8 tasks
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
The
("ws","Client")handle delivered to a WebSocket-upgrade callbackserver.on("upgrade", (req, wsId, head) => …)(rawnode:http) only dispatched its.send()/.on()/.close()to the dedicatedjs_ws_*_client_i64runtime when used inlinein the callback. Passing
wsIdinto a helper made those calls a silent no-op — the framewas dropped with no error. This propagates the handle's host class across the call boundary so
the helper dispatches correctly.
Changes
crates/perry-hir/src/lower/pre_scan.rs— newpre_scan_cross_fn_native_paramspass (runbefore any function body lowers). It seeds from the upgrade idiom
X.on("upgrade", (req, wsId, head) => …)— gated onXbeing acreateServer(...)result (static proxy for a("http","HttpServer")instance; mirrors the runtime check, avoids false positives onarbitrary
.on("upgrade")) — and follows the handle throughuserFn(…, wsId, …)callstransitively (fixpoint), recording a
(fn_name, param_index) -> (module, class)hint.crates/perry-hir/src/lower/lowering_context.rs+…/lower/context.rs— add theparam_native_hintsmap.crates/perry-hir/src/lower/lower_module_fn.rs— invoke the pass alongside the other pre-scans.crates/perry-hir/src/lower_decl/fn_decl.rs— apply the hint (only when the param isn'talready a native instance, so an explicit annotation wins) before the body lowers.
crates/perry/tests/ws_client_handle_cross_function_dispatch.rs— regression tests.Related issue
Test plan
A live WS round-trip needs a socket + client and Perry's raw-upgrade
wsIdhas noNode-
ws-compatible API to diff against, so the bug (which runtime symbol the call site lowersto) is asserted at the LLVM-IR level. TDD red→green: for the helper-only case the IR went from
0
call @js_ws_send_client_i64sites (silent no-op) to 1. Tests also cover transitiveforwarding,
.on/.close, and a control proving a non-ws.sendparam keeps running its ownmethod (no mis-tagging).
cargo build --release -p perryclean (the crates this change touches). The full--workspacerelease build fails only onwindows-future— a Windows-only transitivedep that can't compile on this macOS host (env limitation, unrelated to this change).
cargo test --workspace …passes — my new tests pass; the suite's only failures(
integer_locals_provenance,functional_batch2_regressions, and ablocklist_addsubnet_prefixTempDir::joincompile break) reproduce identically onclean
mainwithout this change, so they are pre-existing and unrelated.#[test]in the affected crate (crates/perry/tests/)docs/src/— N/A (internal HIR dispatch)Screenshots / output
Checklist
fix:prefix conventionSummary by CodeRabbit
send,on, andclosedispatch to the correct Client runtime even when thewsIdhandle is forwarded through helper call chains, including nested callbacks.createServerprovenance.