Repository navigation
Conversation
`EventEmitter` validates argument 1 with `checkListener` before it registers or
removes anything. A call that does not pass a listener at all therefore gets
`undefined` there and throws:
emitter.on() -> THREW ERR_INVALID_ARG_TYPE
emitter.on("x") -> THREW ERR_INVALID_ARG_TYPE
emitter.once("x") -> THREW ERR_INVALID_ARG_TYPE
emitter.addListener("x") -> THREW ERR_INVALID_ARG_TYPE
emitter.prependListener("x") -> THREW ERR_INVALID_ARG_TYPE
emitter.prependOnceListener("x") -> THREW ERR_INVALID_ARG_TYPE
emitter.removeListener("x") -> THREW ERR_INVALID_ARG_TYPE
emitter.off("x") / emitter.off() -> THREW ERR_INVALID_ARG_TYPE
The analysis still reported a `register` or `remove` operation for those calls,
because the listener guard only ran when argument 1 existed. The position is
empty, so no function can arrive in it. A spread argument still reaches the
position through the call, so `e.on(...pairs)` keeps its operation, as does the
already-shifted `e.on(...args, 1)` form.
Measured on Node 22.23.3; nine regressions cover the listener methods and the
two no-argument forms, and all nine fail without the guard.
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
 [Medium impact] The reviewed changes appear safe to merge. SummaryThe PR drops missing-listener operations only when the receiver is proven to use Node’s native
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Listener method call] --> B{Listener missing and no spread?}
B -- No --> C[Keep existing argument checks]
B -- Yes --> D{Proven native EventEmitter method?}
D -- Yes --> E[Drop event operation]
D -- No or uncertain --> F[Keep event candidate]
Reviews (17) · Last reviewed commit: "fix(javascript): honor initializer bindi..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 041a506470
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| listenerMethod && | ||
| listenerArgument === undefined && | ||
| !node.arguments.some((argument) => t.isSpreadElement(argument)) | ||
| ) | ||
| return; |
There was a problem hiding this comment.
For an EventEmitter subclass defining on(name, fn = () => {}) { return super.on(name, fn); }, bus.on("ready") successfully registers a listener, but this new branch discards its literal event operation. collectEvent recognizes only the property name and never proves that the receiver uses Node's native listener validation; optional-listener off implementations are affected for the same reason. The remaining super.on(name, fn) candidate has an unknown event name, so it does not preserve the ready event that previously reached projectSemanticEvents. Apply this rejection only when the native method contract is established, and retain an uncertain candidate otherwise.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed on the same root cause as the other thread, and the subclass case is now a regression test.
The guard keyed on the property name, so it applied Node's checkListener contract to receivers that never agreed to it. It now requires proof that the receiver is constructed from events.EventEmitter, so bus.on("ready") on a subclass with a defaulted listener keeps its "ready" registration, and off implementations with an optional listener are no longer discarded.
I also kept the uncertainty explicit rather than resolving it toward absence: a receiver that cannot be proven keeps its operation as an unresolved candidate, matching the behavior before this PR.
Measured on Node 22.23.3 — the subclass defaults the listener, registers "ready", and does not throw; the inherited off still throws.
…ract
A listener method called without a listener argument throws in Node, but
EventEmitter-style APIs outside Node define one-argument forms:
`socket.off("x")` removes every listener for the event, and a subclass may
default the listener position. Rejecting the operation by method name alone
dropped those real operations from the semantic graph.
Require the receiver to be a value this module constructs from
`events.EventEmitter`, resolved through imports and requires. Any other
receiver keeps its operation as an unresolved candidate.
|
The job fails at
Both files this PR changes pass |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1d30fe2e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| initializer !== undefined && | ||
| initializer.projection.length === 0 && | ||
| isNodeEventEmitterConstructor(initializer.node, state) |
There was a problem hiding this comment.
For const e = new EventEmitter() followed by e.on = (name, fn = () => {}) => EventEmitter.prototype.on.call(e, name, fn), e.on("ready") succeeds and registers a listener. The new constructor proof still returns true because it never checks the receiver's member writes, so collectEvent discards the sole literal ready operation and projectSemanticEvents loses the event. Use the shared call-site member/prototype mutation and escape facts to establish that the selected method is native, retaining an uncertain candidate otherwise.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid finding, and it exposes the same mistake in a different place: I proved where the receiver came from and treated that as proof of which definition answers the call.
const e = new EventEmitter() establishes the class, not the method. Two writes replace the method the checkListener contract belongs to, and both were still read as native:
const e = new EventEmitter();
e.on = (name, fn = () => {}) => EventEmitter.prototype.on.call(e, name, fn);
EventEmitter.prototype.on = function (name, fn = () => {}) { return this; };Both register "ready" for a one-argument e.on("ready") on Node 22.23.3, and both had their operation dropped. The proof now also requires that no member write selects the method on the receiver, and that no write replaces it on the constructor's shared prototype.
The write check uses the same affectsBuiltinPath path matching the builtin-receiver proofs already use, so an unrelated member keeps the proof: e.tag = 1 followed by e.on("ready") still rejects the missing listener. That asymmetry is deliberate — the guard should narrow exactly where the method is replaced, not everywhere an instance is written to.
Validation: three cases in javascriptSemanticAnalysis.rejected_event_listener.test.ts — the instance write, the prototype write, and the unrelated-write control. Reverting this guard fails the first two and leaves the control passing. Full JavaScript domain: 167 files, 4582 tests pass; tsc and oxfmt clean.
Separately, this head now merges current main, which brings in 9ebbe67e0 (fix(ci): await website worker tests and format Wrangler config). That was the website/wrangler.toml failure the previous head reported under Static checks, so this run should no longer carry it.
|
The |
…its class
Proving the receiver was constructed from `events.EventEmitter` says nothing
about which definition answers the call. Two writes replace the method this
contract belongs to and were still treated as native:
const e = new EventEmitter();
e.on = (name, fn = () => {}) => EventEmitter.prototype.on.call(e, name, fn);
EventEmitter.prototype.on = function (name, fn = () => {}) { return this; };
Both register `"ready"` for a one-argument `e.on("ready")`, but the operation
was dropped as an invalid listener. The proof now also requires that no member
write selects the method on the receiver, and none replaces it on the
constructor's shared prototype. A write to an unrelated member keeps the proof,
so `e.tag = 1` still rejects the missing listener.
Tests: three cases in javascriptSemanticAnalysis.rejected_event_listener.test.ts,
two of which fail without this guard. Full JavaScript domain: 167 files, 4582
tests pass; tsc and oxfmt clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6917aa175
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // replaces the method for this receiver as well. | ||
| const constructed = (initializer.node as t.NewExpression).callee; | ||
| const structural = unwrapJavaScriptExpression(constructed).node; | ||
| if (!t.isIdentifier(structural)) return true; |
There was a problem hiding this comment.
With import * as events from "node:events" and const e = new events.EventEmitter(), replacing events.EventEmitter.prototype.on after construction with function (name, fn = () => {}) { return this.addListener(name, fn); } makes e.on("ready") register a real listener (verified on Node 24.15.0). The constructor callee is a MemberExpression, so this return skips the prototype-write check. The earlier builtinDataEffectCallablePath check proves only the constructor, not prototype.on; consequently, collectEvent discards the sole literal ready operation and projectSemanticEvents loses the event. This member-constructor bypass is fresh evidence beyond the direct-identifier mutation case now covered by tests. Check the selected prototype method at the call site for every accepted constructor form, or retain the operation when that proof is unavailable.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
…eceiver may change Constructing Node's EventEmitter proves where the receiver came from, not which definition answers the call. Three shapes still lost the operation: - a namespace import or a member require builds the class through a member expression, where the prototype write is recorded on the module binding rather than on a constructor binding; - writing the prototype after a named import already worked only because the constructor happened to be an identifier; - passing the instance to another call lets that call replace the method before the operation runs. The proof now also scans every binding for a prototype.<method> write, so the constructor form no longer matters, and withdraws when the receiver itself escapes. Reading emitter.on to make the call passes the function value, and the collector records that call as an escape of the receiver, so the operation under analysis is excluded from the escape check.
|
All three findings are valid and share one cause: the proof was keyed on the constructor being an identifier.
One detail that matters for the escape check: reading Validation, on Node 22.22.2:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9617f1a590
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| binding.memberWrites.some( | ||
| ({ path, node: write }) => | ||
| path.at(-1) === method && | ||
| path.at(-2) === "prototype" && |
There was a problem hiding this comment.
For a Node-constructed e, installing a default-listener wrapper at EventEmitter.prototype[key] with const key = "on" makes e.on("ready") register successfully (confirmed on Node 24.15.0 with the wrapper delegating to a saved native method). The mutation owner represents this write as ["prototype", null], so these exact suffix comparisons miss it; inline require("node:events").EventEmitter.prototype.on writes are also missed because they live only in requiredModuleEffects. The constructor proof still succeeds, causing collectEvent to discard the sole literal ready operation and the event projection to lose it. These computed/unbound mutation forms are fresh evidence beyond the exact namespace-member write covered by the earlier thread. Use the shared call-site prototype write/escape facts, retaining the candidate for effects that may select or replace the method.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and it is the same gap in a second place. A write through an inline require(...) has no binding to hang the fact on, so it is recorded against the module rather than in memberWrites, and the scan only read bindings.
Both sources are scanned now: every binding's member writes, and the module records for events. The tail match covers the computed key and the qualified EventEmitter.prototype.on form in the same pass.
Validation: an inline require("node:events").EventEmitter.prototype.on = ... before the registration now keeps it. Measured on Node 22.22.2: the write leaves e.on("ready") storing one listener. Reverting the module scan fails the new case.
|
@codex review |
`prototype.<method>` was compared key by key, so two spellings of the same
replacement were read as native:
- `EventEmitter.prototype[key] = ...` records an unknown key, which covers
the method this call selects;
- `require("node:events").EventEmitter.prototype.on = ...` has no binding at
all and is recorded against the module.
Both leave `e.on("ready")` storing a listener, so the operation has to stay.
The receiver escape check also counted calls that run after the registration.
A call that runs later cannot change whether the registration threw, so only
earlier escapes weaken the proof now.
Measured on Node 22.22.2: each replacement form stores one listener for a
one-argument `e.on("ready")`, and a later `configure(e)` leaves the earlier
`e.on("x")` throwing `ERR_INVALID_ARG_TYPE`.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb36de817d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| binding.memberWrites.some( | ||
| ({ path, node: write }) => | ||
| affectsPrototypeMethod(path, method) && precedes(write), |
There was a problem hiding this comment.
For const e = new EventEmitter(); configure(EventEmitter.prototype); e.on("ready"), where configure(proto) installs an on wrapper that defaults the listener and delegates to a saved native on, the final call registers successfully (verified on Node 24.15.0). This prototype escape is fresh evidence beyond the direct/computed/inline writes now covered by tests. The mutation owner records it in escapedPaths, but this proof examines only memberWrites and module effects classified as writes. The constructor proof also ignores this descendant escape, and e itself never escaped, so the new guard discards the literal ready operation and its graph event. Include constructor/prototype escapes in the call-site method-validity proof, retaining the candidate when native behavior is uncertain.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Handing the prototype object to a call is a write channel the proof did not read: the record lands in the binding's escapes rather than in its member writes, so configure(EventEmitter.prototype) left the class looking native.
The prototype escape is now part of the same call-site proof, with the ordering rule the other checks use: an escape that runs after the registration leaves the guard intact, and only the prototype object itself counts. Passing the method value out (configure(proto.on)) still keeps the proof, since it cannot add a method. The module records are scanned too, so an inline require("node:events").EventEmitter.prototype escape is covered.
Validation: configure(EventEmitter.prototype) installing a defaulting wrapper now keeps the registration. Measured on Node 22.22.2: e.on("ready") does not throw afterwards. Reverting the check fails the new case; the untouched-receiver control still rejects.
| affectsBuiltinPath(path, [method]) && | ||
| !semanticTopLevelEffectFollowsCapture(node, write, state.parentsByNode), |
There was a problem hiding this comment.
After native construction, e.__proto__ = { on(name, fn = () => {}) { return saved.call(this, name, fn); } }, with saved holding the native on, makes e.on("ready") register successfully (verified on Node 24.15.0). Unlike the direct method writes already covered, this write is recorded as ["__proto__"]: it does not affect [method] under this predicate, and the subsequent prototype scan recognizes only paths containing prototype, not __proto__. Neither check withdraws the native proof, so the guard incorrectly removes a real registration from eventOperations. Check receiver-prototype mutations as well as selected-member mutations, as the sibling Map proof already does, and preserve the candidate when either may have changed.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. The receiver's own prototype chain is the other way to select a different method, and it is recorded as a __proto__ member write, which neither the selected-method check nor the prototype scan matched.
A __proto__ write on the receiver now withdraws the proof on the same terms as a direct method write, ordering included. The prototype-object escape check added in the thread above covers the shared case.
Validation: e.__proto__ = { on(name, fn = () => {}) { ... } } now keeps the registration. Measured on Node 22.22.2: e.on("ready") does not throw afterwards. Reverting the check fails the new case.
…f moves
The prototype answers the call, but only writes to it were checked. Two ways
of replacing it were read as native:
- `configure(EventEmitter.prototype)` hands the object out, and the callee can
install a defaulting wrapper on it;
- `e.__proto__ = ...` replaces the method the receiver resolves through its own
prototype chain.
Measured on Node 22.22.2: after either form, `e.on("ready")` does not throw.
Both are ordering-aware, so a call that runs after the registration leaves the
guard intact. Passing a method value out (`configure(proto.on)`) still keeps
the proof, because it cannot change the prototype.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9e604a6e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const escapesPrototypeObject = ( | ||
| path: JavaScriptSemanticPropertyPath, | ||
| ): boolean => | ||
| path.length > 0 && (path.at(-1) === "prototype" || path.at(-1) === null); |
There was a problem hiding this comment.
For const e = new EventEmitter(); configure(EventEmitter); e.on("ready"), an imported configure can install a default-listener wrapper through Ctor.prototype.on, making the final call register successfully (verified on Node 24.15.0). This is fresh evidence beyond the direct-prototype escape regression: the constructor escape is recorded at [], which this predicate excludes; passing e.__proto__ to a prototype configurator similarly records ["__proto__"] and is missed. Construction was already captured, and neither route creates a root receiver escape, so the native proof survives and the guard deletes the literal ready operation and its graph event. Withdraw the proof for escapes that can reach the shared prototype, including its containing constructor and the receiver's prototype reference.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The direct form is already covered — on the head before this change the module-wide prototype.<method> scan already sees the write that configure performs, because the parameter binding records prototype.on. The escape route still matters for the forms that write cannot describe, such as a dynamic key inside the callee, so it is handled now as well.
Rather than adding one predicate per route, the proof now requires the events class surface to be untouched: no escape of the class or its namespace, and no member write on it, whether the file reaches it directly or through the module records. That subsumes the escape route, the init hook below, and the cross-import replacement.
Validation: configure(EventEmitter) installing Ctor.prototype.on keeps the registration. Measured on Node 22.22.2: e.on("ready") does not throw afterwards.
| return selected.length === 0 || selected.at(-1) === "EventEmitter" | ||
| ? value | ||
| : null; |
There was a problem hiding this comment.
If code replaces EventEmitter.init before construction with a wrapper that calls the saved initializer and installs an own default-listener this.on, new EventEmitter() still passes this proof, but e.on("ready") registers successfully (verified on Node 24.15.0). Node's constructor dynamically invokes EventEmitter.init.call(this, opts), so native constructor identity does not establish the instance's method. The recorded constructor write is ["init"], which neither the selected-member nor prototype scans inspect, and the guard therefore deletes a real registration even though the shared prototype is untouched. Treat construction as uncertain when its initialization hook may have changed before allocation.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and it shows why the proof should not be about the constructor alone: native construction does not establish the instance's method once the hook Node calls during it can install one.
A member write on the class now withdraws the proof, init included, before the guard reads the construction. This also covers a subclass that replaces the hook through a different import of the module.
Validation: replacing EventEmitter.init with a wrapper that installs this.on keeps the registration. Measured on Node 22.22.2: e.on("ready") does not throw afterwards. Reverting the class-surface check fails the new case.
| const selected = builtinDataEffectCallablePath( | ||
| value.callee, | ||
| state, | ||
| NODE_EVENTS_SPECIFIERS, | ||
| ); |
There was a problem hiding this comment.
With import A from "events"; import B from "node:events", both bindings reference the same mutable constructor. After A.EventEmitter = class extends A { on(name, fn = () => {}) { return super.on(name, fn); } }, const e = new B.EventEmitter(); e.on("ready") registers successfully (verified on Node 24.15.0). However, semanticMutableBuiltinOrigin excludes events from MUTABLE_BUILTIN_DEFAULTS, so the write never enters shared module effects and this lookup through B still reports ["EventEmitter"]. The prototype scan also ignores the constructor-slot write, allowing the new guard to discard the real registration. Register the shared mutable identity at the builtin-mutation owner before using this resolution to prove absence.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Both specifiers resolve to one mutable module at runtime, so the proof cannot be scoped to the binding the construction happens to use.
Writes are now matched by what they name rather than by which binding owns them: any write whose path selects EventEmitter withdraws the proof, whichever import performed it, and the module records are read the same way for the inline require form.
Validation: first.EventEmitter = class extends first.EventEmitter { ... } followed by new second.EventEmitter() keeps the registration. Measured on Node 22.22.2: e.on("ready") does not throw afterwards. Reverting the class-surface check fails the new case.
Three more routes let the call reach a method this analysis did not describe,
and each one kept the guard:
- `configure(EventEmitter)` hands the class out, and the callee installs
`Ctor.prototype.on`;
- replacing `EventEmitter.init` before construction installs an own defaulting
method on every instance, because Node invokes that hook while constructing;
- `import A from "events"` and `import B from "node:events"` name the same
mutable module, so `A.EventEmitter = ...` answers a `new B.EventEmitter()`.
Adding one predicate per route leaves the next one open, so the proof now
requires the class surface to be untouched: no escape of the class or its
namespace, and no member write on it, whether the file reaches it directly or
through the module records. `__proto__` escapes of the prototype carry the same
weight as `prototype` escapes.
Measured on Node 22.22.2: all three forms leave `e.on("ready")` not throwing.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bd6c112a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| binding.escapedPaths.some( | ||
| (escape) => | ||
| escape.path.length === 0 && |
There was a problem hiding this comment.
After const e = new EventEmitter(), a direct eval('e.on = (name, fn = () => {}) => e.addListener(name, fn)') makes e.on("ready") register successfully (verified on Node 24.15.0). This is a new mutation channel beyond the explicit writes and argument escapes now covered. The shared effect owner synthesizes eval exposure as an escape in javascriptSemanticAnalysis.ts:282–287, rather than adding it to binding.escapedPaths or memberWrites, so the native-method proof still succeeds and collectEvent discards the only literal ready operation and its graph event. Use the shared call-site reference-effect query, retaining the candidate when opaque code may have changed the receiver.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. eval exposure is recorded as a reference effect rather than as a write, so the proof held for a receiver whose method the evaluated code had replaced.
The receiver proof now reads those effects too: a direct eval earlier in the same activation removes it, and one that runs later does not.
Validation: the new case fails on the previous head and passes here. Measured on Node 22.22.2: e.on("ready") leaves listenerCount("ready") === 1 after the evaluated replacement installs a defaulting method.
A member write only removes the proof when the value can name the events module, so an unrelated `config.init = true` no longer withdraws it, and class-level writes are compared against the construction they shape rather than against the call. A direct `eval` in the activation withdraws the proof as well, because evaluated code can replace the method without leaving a write record.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c964e9892f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (semanticBindingMayHaveOpaqueEffect(binding, state, call, "reference")) | ||
| return false; |
There was a problem hiding this comment.
For a named events import, a module-level eval that installs a default-listener EventEmitter.prototype.on wrapper, followed by function run() { const e = new EventEmitter(); e.on("ready"); } run();, causes a real registration to be discarded. semanticOpaqueBindingEffects starts at the receiver binding's function activation, so this query cannot see the outer eval; the immutable constructor import still resolves, and the class/prototype scans have no explicit write to find. This outer-activation prototype mutation is fresh evidence beyond the same-activation receiver-eval regression now covered. Check opaque effects on the shared class/prototype too, retaining the uncertain candidate so its ready event can reach projectSemanticEvents.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
|
Hey @gongfpp, thanks for putting this together, the fix looks useful. What's left: it has merge conflicts with main in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a7b483d9a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| semanticRequireOriginCandidate(expression, state, true)?.origin ?? | ||
| semanticBuiltinModuleEffectOrigin(expression, state); |
There was a problem hiding this comment.
When an emitter is created from a named events import and on is subsequently replaced through (await import('node:events')).EventEmitter.prototype.on, a wrapper that defaults its listener and delegates to a saved native method makes e.on('ready') register successfully (verified on Node 24.15.0). This uses the unhandled ImportExpression acquisition, unlike the now-covered require/getBuiltinModule routes: neither origin reader here recognizes it, so no shared-module mutation fact reaches the new native-method proof. Source tracing therefore leaves that proof intact, causing it to drop the sole literal ready operation and its graph event despite the real registration. Retain dynamic-import effects at the shared mutation owner, or preserve the uncertain event candidate when that origin is not modeled.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c3684c8d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (semanticPropertyPathKeyMatches(key, "constructor")) | ||
| references.push({ node: node.callee, path: remaining, inherited: true }); |
There was a problem hiding this comment.
For const records = new Map(); new Map().set('x', 1); records.get('/cache');, markReceiver treats set as an opaque receiver escape because only get/delete receive the native-call exemption. These new inherited references turn that local invocation into an ambient Map/prototype escape, so isIntrinsicMapLookup rejects the untouched records.get and source/captured-bundle analysis reports /cache as an endpoint_candidate. Native Map.prototype.set neither runs user code nor changes that shared surface here. Extend the producer's intrinsic non-escaping-call proof before propagating its receiver to the shared class, rather than invalidating unrelated instances.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
| // This await consumes the branch value, not a property, alias, | ||
| // container child, or callback result. Those paths drop it. | ||
| ...(awaitedModuleAcquisition | ||
| ? { awaitedModuleAcquisition: true } | ||
| : {}), |
There was a problem hiding this comment.
With a named EventEmitter import and const pending = import('node:events'); const ns = await pending; ns.EventEmitter.init = initialize; before construction, an imported initialize can install an own defaulting on that delegates to addListener, making e.on('ready') register successfully. Unlike the covered inline-await form, following pending's initializer drops awaitedModuleAcquisition; no module effect is retained, and neither binding has events provenance for the class-surface scan. Consequently the new guard discards the real ready operation. Preserve the await-consumption context across Promise aliases rather than restricting it to the listed syntax branches.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65d74b03ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const intrinsicMapCall = isIntrinsicNonEscapingMapCall(node, state); | ||
| if (intrinsicMapCall === undefined) deferredMapReceivers.add(node); | ||
| else if (!intrinsicMapCall) markReceiver(node.callee, node); |
There was a problem hiding this comment.
For two fresh, untouched XHR instances, const a = new XMLHttpRequest(), b = new XMLHttpRequest(); a.open('GET', '/a'); b.open('GET', '/b'); now treats the first native open as an opaque receiver escape. The new inherited references propagate that escape to XMLHttpRequest and its prototype, so isXhrOpenRequest rejects the second unchanged call. Source analysis consequently marks /b as a candidate, and captured-bundle analysis moves it out of observed endpoints. The native-call exemption handles Map operations but misses this sibling regression; establish non-escaping behavior for these fresh-XHR calls before propagating their receivers to the shared class.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
| const importedProcessGetter = | ||
| !globalProcessGetter && | ||
| evaluateSemanticExpressionProvenance(callee, state).origins.some( |
There was a problem hiding this comment.
When code extracts const { getBuiltinModule } = process, a subsequent getBuiltinModule('events').EventEmitter.prototype.on = wrapper produces no shared-module mutation record: the global check requires a member callee, while this fallback gives the global getter alias no process provenance. With a named-imported EventEmitter and a wrapper that defaults the listener and delegates to the saved native on, e.on('ready') really registers a listener (verified on Node 24.15.0), but the new guard drops the literal event. This extracted-global-getter case is fresh evidence beyond the direct-member and imported-getter forms now covered. Resolve these global aliases at the acquisition/effect owner so the event remains an uncertain candidate.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6cc7621de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| effects.prototypeEscapes.some( | ||
| (escape) => | ||
| !semanticTopLevelEffectFollowsCapture(read, escape, state.parentsByNode), |
There was a problem hiding this comment.
For a missing-listener call inside a function, including the new eventsFor fixtures, constructedReferenceValues now propagates the call's own receiver escape to EventEmitter.prototype. This predicate treats that same invocation as a possible earlier mutation because semanticTopLevelEffectFollowsCapture returns false for nodes inside functions. Consequently, nodeEventEmitterReceiver returns false and retains the impossible e.on("x") operation, so the nine added missing-listener assertions would fail. Pass the invocation into this check and exclude its own escape, as eventsClassSurfaceTouched and the Map/XHR queries already do, completing the affected-consumer refactor required by AGENTS.md.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 676c01da82
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (eventsClassSurfaceTouched(effects, construction, node, state, call)) | ||
| return false; |
There was a problem hiding this comment.
When a module assigns an imported forwarding wrapper to Function.prototype.call before constructing a named-imported EventEmitter, the wrapper can run the saved initializer and install an own on(name, fn = () => {}); e.on('ready') then registers a listener (verified on Node 24.15.0), because Node constructs via EventEmitter.init.call(this, opts). This is fresh evidence beyond the covered initializer-object escape: the mutation is retained as an ambient Function.prototype.call write, not a write or escape on an events binding, so these checks leave the native proof intact and the guard deletes the real ready operation. Query the shared ambient mutation facts for the initializer's inherited call dependency before allocation, retaining the uncertain candidate when it may have changed.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25cc26bd4e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (method !== "has" && | ||
| method !== "clear" && | ||
| // set returns this. Until that result identity is retained by its | ||
| // owner, only a discarded result proves no receiver escapes through it. | ||
| (method !== "set" || |
There was a problem hiding this comment.
For const records = new Map(); new Map().keys(); records.get('/cache');, keys falls outside this whitelist, so the mutation producer records an opaque receiver escape. The new inherited references propagate it to shared Map and its prototype, causing the unchanged lookup to become an endpoint_candidate in source and captured-bundle analysis. This iterator-creation route is fresh evidence beyond the now-exempted set case: native keys/values/entries calls with discarded results neither expose the receiver nor mutate the shared class. Include these calls in the producer's nonescaping proof before propagating their receivers.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
| const eventsInitializerPath = (path: JavaScriptSemanticPropertyPath): boolean => | ||
| path.at(-1) === "init" && eventsClassPath(path.slice(0, -1)); |
There was a problem hiding this comment.
For a named-imported EventEmitter, configure(EventEmitter.init.prototype.constructor); const e = new EventEmitter(); e.on('ready'); registers successfully when the imported configurator replaces its argument's .call with a forwarding initializer that installs a default-listener on wrapper (verified on Node 24.15.0). The fresh evidence is the initializer's prototype.constructor back-reference: it is exactly EventEmitter.init, but its recorded ['init', 'prototype', 'constructor'] escape matches neither this predicate nor the class/prototype escape predicates. Consequently, the native proof survives and the real ready operation is discarded. Preserve this identity at the reference owner, or retain the uncertain candidate.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95cd68cf5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const map = isIntrinsicNonEscapingMapCall(node, state, index); | ||
| return map === false | ||
| ? isIntrinsicNonEscapingXhrOpen(node, state, index) |
There was a problem hiding this comment.
With const a = new Array(3); a.fill(0); const urls = ['/api']; const [url] = urls; fetch(url);, this dispatcher still treats the pristine native fill as an opaque receiver escape. The newly retained inherited links turn that into a shared Array.prototype escape and set arrayIterationUnknown, so the unrelated destructuring loses /api and source analysis reports an unresolved endpoint url instead; initializing a with [] still resolves /api. This affects the array-iteration consumer beyond the now-exempted Map/XHR cases. Establish non-escaping behavior for these primitive-argument native Array calls before propagating their receivers to the shared class.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e118fc9a1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (semanticPropertyPathKeyMatches(key, "__proto__")) | ||
| references.push({ | ||
| node: node.callee, | ||
| path: ["prototype", ...remaining], |
There was a problem hiding this comment.
For new Object().hasOwnProperty("x"); const [url] = ["/api"]; fetch(url);, these new inherited links turn the harmless native receiver call into an escape of Object.prototype: the intrinsic dispatcher checks Map/XHR/Array calls, so markReceiver takes the opaque path here. That escape sets arrayIterationUnknown, making semanticKnownArrayIteration reject the unrelated destructuring and preventing endpoint analysis from resolving /api. This is fresh evidence beyond the now-exempted Array mutation case: a pristine native hasOwnProperty with a string key cannot expose its receiver. Establish that nonescaping behavior at the producer before propagating the inherited links.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
| const specifier = stringValue(call.arguments[0]); | ||
| if (specifier === undefined) return undefined; |
There was a problem hiding this comment.
With a named EventEmitter import, a top-level process.getBuiltinModule.call(null, "events").EventEmitter.prototype.on = replacement produces no events-module mutation fact. Unlike the covered direct/extracted getter forms, .call places the specifier in argument 1, so this argument-0 check returns before retaining the effect; the inline target also has no binding to carry the write. If replacement defaults its listener and delegates to the saved native on, e.on("ready") registers successfully (verified on Node 24.15.0), but the new native-method proof discards its literal event operation. Consume the effective invocation from the shared resolveJavaScriptIntrinsicInvocation owner, which already models call/apply/bind, or retain an uncertain candidate.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba119f329b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| !PRISTINE_ARRAY_METHODS.has( | ||
| semanticStaticPropertyName(callee.property, callee.computed) ?? "", | ||
| ) || |
There was a problem hiding this comment.
For new Array(3).includes(0); const [url] = ['/api']; fetch(url);, includes falls outside this whitelist, so markReceiver takes the opaque path. The added inherited references propagate that escape to Array.prototype and set arrayIterationUnknown; semanticKnownArrayIteration then rejects the unrelated literal destructuring, losing the resolved /api endpoint. This callback-free read is fresh evidence beyond the covered fill mutation: with primitive inputs it returns a boolean and cannot expose the receiver or change the shared prototype. Extend the producer's pristine-native proof to these read methods before propagating a shared escape.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b18b7007a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (semanticPropertyPathKeyMatches(key, "__proto__")) | ||
| references.push({ | ||
| node: node.callee, | ||
| path: ["prototype", ...remaining], |
There was a problem hiding this comment.
For top-level source such as const [url] = ['/api']; fetch(url); consume(new Array(3));, this inherited link propagates the final escape to Array.prototype and sets the module-wide arrayIterationUnknown flag. semanticKnownArrayIteration in javascriptSemanticReferenceSources.ts:46–52 still reads that flag without comparing effect timing, so the earlier destructuring becomes unknown and endpoint analysis loses /api. The Array consumer is fresh evidence beyond the now-timed Map/XHR consumers: an opaque call cannot retroactively change an earlier read. Migrate the array-iteration owner to retained, read-time effects as part of this representation change.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
| ) || | ||
| // Some results retain this (including iterators); no returned-reference | ||
| // proof is introduced even for methods whose result is primitive. | ||
| !t.isExpressionStatement(state.parentsByNode.get(node)) |
There was a problem hiding this comment.
For const matched = new Array(3).includes(0); const [url] = ['/api']; fetch(url);, this requirement rejects the native-call exemption solely because the boolean result is assigned. markReceiver consequently records an opaque receiver escape, the new inherited links expose Array.prototype, and arrayIterationUnknown prevents the unrelated /api destructuring from resolving. The consumed primitive result is fresh evidence beyond the discarded-read regression: native includes cannot expose its receiver or invoke coercion hooks under the existing primitive/pristine checks. Distinguish guaranteed primitive-returning methods from iterator/receiver-returning methods rather than requiring every result to be discarded.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09001af285
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| references.push({ | ||
| node: node.callee, | ||
| path: ["prototype", ...remaining], | ||
| inherited: true, |
There was a problem hiding this comment.
For const a = new Array(3); Array.isArray(a); const urls = ['/api']; const [url] = urls; fetch(url);, source analysis now loses the resolved /api endpoint and reports an unresolved url. The argument loop in collectSemanticMemberMutationsSteps unconditionally calls markEscaped, so even native Array.isArray exposes a; these inherited links then produce a shared Array.prototype escape and an array-iteration effect. The new nonescaping dispatcher handles receivers only. Object.keys(new Object()) causes the same regression, while Array.isArray([]) preserves /api. Establish nonescaping behavior for proven native argument inspections at the mutation producer before propagating their shared prototypes.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
| method !== "keys" && | ||
| method !== "values" && | ||
| method !== "entries") || | ||
| !t.isExpressionStatement(bindings.parentsByNode.get(node))))) |
There was a problem hiding this comment.
For const records = new Map(); void new Map().set('x', 1); records.get('/cache');, this check rejects the native-call exemption because the immediate parent is a UnaryExpression, even though void discards the returned receiver. The resulting receiver escape reaches the shared Map prototype, so the untouched /cache lookup is incorrectly retained as a network candidate. Non-final comma operands have the same problem, and the duplicate gates in the Array/Object helpers similarly invalidate unrelated destructuring after void new Array(3).fill(0) or void new Object().valueOf(). These are genuinely discarded receiver-returning results, unlike the earlier consumed-primitive case. Normalize result-discarding contexts once and share that fact across the intrinsic proofs.
AGENTS.md reference: AGENTS.md:L27-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be61d5c845
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| receiver.construction.arguments.length !== 0 || | ||
| !isUnmodifiedGlobalReference(receiver.construction.callee, state, "Object") |
There was a problem hiding this comment.
With untouched builtins, new Array(3).hasOwnProperty('0'); const [url] = ['/api']; fetch(url); calls native Object.prototype.hasOwnProperty, but this constructor check rejects the Array receiver, while the Array helper rejects the method name. Consequently, markReceiver records an opaque escape, the new inherited links propagate it to Array.prototype, and semanticKnownArrayIteration makes the unrelated destructuring unknown, losing the resolved /api endpoint. This inherited-method route is distinct from the direct new Object().hasOwnProperty(...) case now covered. Share the nonescaping intrinsic-method proof across compatible receiver constructors rather than requiring new Object() here.
AGENTS.md reference: AGENTS.md:L27-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 048274c7dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| semanticPrimitiveCandidates( | ||
| evaluateSemanticExpression(argument, state), | ||
| ) !== null, |
There was a problem hiding this comment.
With pristine builtins, new Array(3).includes(void 0); const [url] = ['/api']; fetch(url); takes the opaque-receiver path because evaluateUnary cannot represent void in the JSON primitive lattice, so this predicate rejects the argument. The new inherited references then escape Array.prototype before the destructuring, causing source analysis to lose the resolved /api endpoint. This fails even with a discarded call result: void 0 is guaranteed primitive and cannot introduce a coercion hook into native includes. Use the shared primitive-result/type facts, such as those already retained by primitiveOwnKeys, rather than requiring a serializable primitive value.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| invocation: t.Node, | ||
| state: JavaScriptSemanticAnalysisState, | ||
| ): boolean => | ||
| isUnmodifiedGlobalReference(construction.callee, state, "Array") && |
There was a problem hiding this comment.
For const A = Array; new A(3).fill(0); const [url] = ['/api']; fetch(url);, this syntax-local global check rejects A, so the otherwise-covered native fill call is treated as an opaque receiver escape. The mutation owner does follow the same stable constructor alias back to Array; the added inherited links therefore escape Array.prototype and invalidate the unrelated destructuring, losing /api. This is a constructor-alias gap, distinct from the supported receiver aliases. Resolve captured constructor identity through the shared ambient-reference facts before applying the unchanged-surface checks; the equivalent constructor-spelling checks in the Map/Object proofs need the same treatment.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
Preserve receiver-specific missing-listener checks and conservative child-process event candidates while integrating upstream changes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 486aade3a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const knownArray = semanticKnownArrayIteration( | ||
| slot?.value, | ||
| state, | ||
| reference.node, |
There was a problem hiding this comment.
With pristine builtins, const copy = [...new Array(3)]; const [url] = ['/api']; fetch(url); reaches this check with an unknown NewExpression value, so the iteration fallback escapes the entire array. The new inherited links then expose Array.prototype and add an iteration effect before the unrelated destructuring, causing source analysis to lose /api. Language-level spread is fresh evidence beyond the covered direct iterator-method calls: [...new Map()] likewise escapes shared Map and makes a subsequent unchanged records.get('/cache') a network candidate. Neither native iteration executes user code or exposes its receiver in these pristine cases. Prove nonescaping native iteration independently of whether the value lattice retains constant element slots before publishing the fallback root escape.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
Summary
A listener method called without a listener argument currently produces a
registerorremoveevent operation, even though Node throws before any listener exists. This is the empty-argument case of the invariant the merged #8621 guard established; there is no separate issue because the trigger and the violated rule are the same one.Problem and expected behavior
Smallest trigger:
EventEmittervalidates argument 1 withcheckListenerbefore it registers or removes anything. With no second argument that position holdsundefined, so the type check throws and no listener is created.Measured on Node 22.23.3:
Before this change the analysis reported
{ kind: "register", method: "on", eventName: "x" }fore.on("x"), and{ kind: "remove", method: "off", eventName: "x" }fore.off("x"). Expected: no event operation at all.Change and scope
In
collectEvent, when the method takes a listener and argument 1 is absent while no argument is a spread, the operation is dropped only when the receiver is proven to be constructed fromevents.EventEmitterand to still call the native method. Construction proof comes from the shared builtin resolver, so a named import, a default import, a destructuredrequire, andrequire("events").EventEmitterall count, and only a binding with a single non-conditional initializer qualifies.That proof is then withdrawn on three conditions, each of which was a review finding on an earlier head:
emitter.on = ...);configure(emitter)). A call that runs after the registration cannot change whether it threw, so later calls do not withdraw the proof;prototype.<method>, whatever form the constructor reference took:EventEmitter.prototype.on,events.EventEmitter.prototype.on, an alias, the member-require form, an unknown key (prototype[key]), or an inlinerequire("node:events").EventEmitter.prototype.on, which has no binding and is recorded against the module;e.__proto__ = ...), which selects a different method the same way a direct write does;configure(EventEmitter.prototype)), which may install the method on it. Passing the method value out (configure(proto.on)) keeps the proof, because it cannot add a method;configure(EventEmitter)), any member of the class is written (EventEmitter.init = ..., which Node invokes while constructing and which can install an own method on the instance), or the exported class is replaced through another import of the same module (import A from "events"while the receiver comes fromnode:events).The last item is deliberately coarse. Enumerating the individual routes to a replaced method leaves the next one open, so the proof now requires the class surface to be untouched rather than proving each route harmless.
Every one of these checks is ordering-aware: an effect that runs after the registration cannot change whether it threw, so it does not withdraw the proof.
Any other receiver keeps its operation as an unresolved candidate, which is the behavior before this change.
checkListenerbelongs toevents.EventEmitter, not to the method name. Socket.IO'ssocket.off("x")removes every listener for the event, and a subclass may default the listener position; a method-name-only rule discarded both real operations.Deliberately not included:
emitandremoveAllListenersare untouched.emitforwards its remaining arguments as data to listeners that already exist, andremoveAllListenerstakes only an optional event name. Measured:e.emit()ande.removeAllListeners()do not throw.e.on(...pairs)keeps its operation, because a spread can supply both the event name and the listener. The already-shiftede.on(...args, 1)form from fix(javascript): emit no event operation for a literal non-function listener #8621 is unaffected.Contract and boundary impact
collectEventand the receiver proof insrc/domain/javascript/javascriptSemanticAsyncEffects.ts, producing semantic event operations before inventory and CLI/MCP projection. No consumer or schema changes.docs/public/product-catalog.json), package, or installation impact: none.Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.
Verification
9 failed | 32 passedbefore the fix) and pass after it.require, and memberrequire.socket.off("x")), a subclass that defaults its listener, and a receiver reassigned after construction.e.on(...pairs), because a spread can still reach the listener position.prototype[key]), an inlinerequire(...)prototype write, and a receiver built from a namespace member. Each fails on the head before it, and each was measured on Node 22.22.2 as storing one listener for a one-argumente.on("ready").e.on("ready"); configure(e); e.emit("done")). Reverting the ordering rule fails it; Node throws at the registration before either later call runs.__proto__write on the receiver. Each fails on the head before it, and both were measured on Node 22.22.2 as making a one-argumente.on("ready")stop throwing.inithook, and a replacement through a second import of the same module. Theinitand cross-import cases fail on the head before this change; the class-escape case already passed there, because the write inside the callee is recorded asprototype.on, which the module-wide scan reads.initno longer withdraws the proof, a hook installed after the receiver was built no longer withdraws it, and a directevalthat may replace the method withdraws it. All three fail on the head before this change; each was measured on Node 22.22.2, where the first two still throwERR_INVALID_ARG_TYPEand the third leaves one stored listener.npm run check:fast— typecheck and lint clean;oxfmt --checkclean on both files;oxlintreports the one pre-existing complexity warning onemitterIdentity.Gaps: the runtime matrix was measured on Node 22.23.3 only; the repository engine range also allows ^24.22.1 and >=26.12.1, and I did not run those locally. The thrown check is
checkListener, which is version-stable across those majors, but that part is source-derived rather than measured. Platform-specific Node builds were not exercised. Socket.IO's one-argumentoffis taken from its documented behavior, not measured here.Author readiness
Human owner: @gongfpp
Before merge
type(scope): outcome.