fix(ses): Cyclic star export with renaming reexport (issue #59) by kriskowal · Pull Request #3276 · endojs/endo

kriskowal · 2026-05-21T17:48:48Z

Closes: #59

Description

When a module re-exports * from another module, and that other module re-exports a binding from the first under a different exported name (export { y as x } from './mod1.js'), the linker visited the first module while its star-imported notifier for y had not yet been wired. The synchronous wireUp at the cycle's back-edge then passed undefined as the upstream notifier, which manifested as TypeError: notify is not a function. The original 2019 issue described the failure as SyntaxError: ... does not provide an export named 'y'; the surface symptom evolved as the linker matured, but the underlying defect is the same.

wireUpExportNotifier now installs a deferred forwarding notifier for re-exports whose upstream notifier is not yet present. The forwarding queues subscribers until the upstream resolves, then drains them through. By the time any module downstream subscribes to the re-export, the upstream module has completed its candidate-all walk and the upstream notifier exists, so the chain converges.

Security Considerations

None. The change is internal to the SES module linker: a previously unreachable subscriber path that threw on a missing notifier now installs a forwarding notifier and drains it once the upstream resolves. No new authority is exposed and no trust boundary moves.

Scaling Considerations

Negligible. The forwarding notifier allocates a small queue per re-export whose upstream is not yet wired, and the queue is drained synchronously the first time the upstream resolves. The fix runs once per qualifying re-export at link time, not per use, and does not affect steady-state import or evaluation cost.

Documentation Considerations

No user-facing documentation change. The behavior the fix restores is the behavior the module spec already prescribes; the previous behavior was a defect in the linker, not a documented capability.

Testing Considerations

Two regression surfaces accompany the fix. SES's import-gauntlet adds cyclic star export with renaming reexport (issue #59) exercising the exact reproducer from the issue against the SES linker directly. A compartment-mapper test port drives a three-module fixture (star-reexporter re-exports * from export-renamer; export-renamer re-exports y as x from star-reexporter) through loadLocation, importLocation, the archive round-trip pair, and makeArchiveFromMap. A Node.js parity test imports the same fixture under plain Node.js (no SES, no compartment mapper) and asserts identical expected values from a shared assertions module, pinning the compartment mapper's behavior to Node.js's reference behavior so the expected values are defined in one place.

Reverting the wireUpExportNotifier change while keeping either test surface reproduces TypeError: notify is not a function (nine of the eleven compartment-mapper import-path variants fail; the two archive-integrity variants pass because they do not import the fixture; the Node.js parity test is unaffected). Re-applying the fix restores all twelve passing tests.

Compatibility Considerations

The fix turns a previously-throwing import shape into one that resolves to the binding the module spec prescribes. No code that worked before this change is broken by it. Downstream code that depended on the TypeError (or, historically, the SyntaxError) as a signal of an unrelated condition is the only conceivable casualty; we are not aware of any such consumer.

Upgrade Considerations

No upgrade ceremony. A consumer who has been working around the defect by avoiding the cyclic-rename shape can drop the workaround on adoption; no other action is required. A patch-level changeset accompanies the fix.

changeset-bot · 2026-05-21T17:48:54Z

🦋 Changeset detected

Latest commit: 1725258

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
ses	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

kriscendobot · 2026-05-29T02:44:50Z

Mirror of endojs/endo-but-for-bots#336 (head f4aad15).

naugtur · 2026-05-29T09:41:21Z

+      // through.
+      const pendingUpdaters = [];
+      let resolvedUpstreamNotify;
+      notify = update => {


Is a situation possible where all calls to the deferring notify happen before upstreamNotify can be obtained? I was trying to come up with when that could happen and I thought of unused live bindings.

I (loosely) have added tests that cover the unused live binding case. This was possible with an uninitialized export var.

naugtur · 2026-05-29T09:43:50Z

@@ -0,0 +1,7 @@
+import { x } from './star-reexporter.js';
+import * as ns1 from './star-reexporter.js';
+import * as ns2 from './export-renamer.js';


I see the change is only in makeModuleInstance. Would it still work if the cycle was longer and a cjs module was involved in it?

Would you be so kind to ask a clanker for a fixture for that?

We (dually) have added fixtures that cover {esm,cjs}x{esm,cjs}, with {node,endo} parity validation.

naugtur · 2026-05-29T09:48:11Z

+    if (notifiers[exportName] || notify === false) {
+      return;
+    }
+    if (notify === undefined) {


I know this is vague, but it'd feel nicer design-wise to create all notifiers ahead of time and give them an ability to forward to or pull from another notifier, in which case the reexport * would be a matter of connecting notifiers in a loop.

If we could use this as an opportunity to design a fancier notifier primitive to share between the two implementations makeModuleInstance and makeVirtualModuleInstance we'd avoid some of the risk of interop failure

I managed to convince my inanimate engineering partner to extract a notifier primitive, which is spiritually similar to Promise.withResolvers but with synchronous propagation. I didn’t find a way to integrate it in virtual module instances, which don’t support live bindings by design.

Companion regression for endojs/endo#59 addressing review feedback on endojs/endo#3276: a sibling fixture where the live binding `y` is declared but never assigned. Node.js reads every projection of the cycle as `undefined` for this shape (verified directly with `node`), so the SES linker must match. The deferring closure introduced by the fix either resolves (when a wireUp higher in the chain re-references the binding) or stays pending; the namespace reads agree with Node.js in either case. This commit adds executable evidence; no source change.

Introduce a synchronous variant of Promise.withResolvers. The helper returns { notify, resolve }: subscribers attached via notify(update) before resolve(targetNotify) is called are queued; once resolve is called, queued updaters are replayed to the target notifier and subsequent notify(update) calls forward directly through. resolve is one-shot (idempotent for repeat calls), which lets a caller invoke it lazily on each notify and have only the first invocation take effect. Apply the helper to the cycle-resolver branch of wireUpExportNotifier in module-instance.js, replacing the inline pendingUpdaters[] + resolvedUpstreamNotify state machine. The two patterns are now syntactically separated from their call site, reducing the chance that the local state machine will drift away from any future second use. Refs: endojs/endo#3276 (kriskowal review)

Add a regression test in import-cjs.test.js verifying that a cyclic star-export topology in which the "reexporter" is a CommonJS module behaves consistent with Node.js. Node.js rejects ESM-in-CJS-cycle outright (ERR_REQUIRE_CYCLE_MODULE), so the parity comparison is against the pure-CJS cycle: snapshot-at-call-time semantics for the CJS side (property capture sees whatever the renamer had assigned by the re-entry instant), live-binding semantics for the ESM side. Both namespaces project the same shapes the test pins. Addresses naugtur's review feedback on endojs/endo#3276 asking for a test of the cyclic star-export with a CommonJS reexporting module. Refs: endojs/endo#3276 (naugtur review)

…59 follow-up) Add the parity test pair kriskowal asked for on #379 (review comment 3338677487): the unused-live-binding shape of the cyclic star-export with renaming reexport (endojs/endo#59) needs a parity test substantiating the Node.js parity claim, ideally with a shared fixture as with cycle-rename parity. The companion populated-binding shape was already covered by cycle-rename.test.js and cycle-rename-node-parity.test.js through _cycle-rename-assertions.js; this commit lands the analogous trio for the unused-live-binding shape: fixtures-cycle-rename-unused/node_modules/app/ - three modules; the renamer's `export var y` has no initializer. _cycle-rename-unused-assertions.js - shared assertion module; expected projections are { x: undefined, y: undefined } and captured: undefined. cycle-rename-unused.test.js - compartment-mapper scaffold exercise. cycle-rename-unused-node-parity.test.js - Node.js exercise of the same fixture; parity is verified by construction when both pass. Also update the in-process SES regression's prose in packages/ses/test/import-gauntlet.test.js to reference the new parity pair, matching the cross-reference pattern the populated-binding test already uses for cycle-rename. Refs: endojs/endo#3276 (naugtur), #379 (kriskowal)

Audit of naugtur's 3 inline asks on endojs/endo#3276 and kriskowal's 5 mirror-side asks on endojs/endo-but-for-bots#379. Seven of eight asks are genuinely addressed by prior commits; one ask (kriskowal #3 on import-gauntlet.test.js) had a missing shared-fixture parity pair for the unused-live-binding shape, closed with new compartment-mapper head f1a7dfb60. The eighth ask (kriskowal's gardener-meta follow-up) is queued as a message-fixer-40ac9b entry for the next gardener dispatch. Top-level PR audit comment posted at endojs/endo-but-for-bots#379-issuecomment-4609535322.

…mmits onto endojs/endo#3276 (strip Refs trailers)

Companion regression for #59 addressing review feedback on #3276: a sibling fixture where the live binding `y` is declared but never assigned. Node.js reads every projection of the cycle as `undefined` for this shape (verified directly with `node`), so the SES linker must match. The deferring closure introduced by the fix either resolves (when a wireUp higher in the chain re-references the binding) or stays pending; the namespace reads agree with Node.js in either case. This commit adds executable evidence; no source change.

Add a regression test in import-cjs.test.js verifying that a cyclic star-export topology in which the "reexporter" is a CommonJS module behaves consistent with Node.js. Node.js rejects ESM-in-CJS-cycle outright (ERR_REQUIRE_CYCLE_MODULE), so the parity comparison is against the pure-CJS cycle: snapshot-at-call-time semantics for the CJS side (property capture sees whatever the renamer had assigned by the re-entry instant), live-binding semantics for the ESM side. Both namespaces project the same shapes the test pins. Addresses naugtur's review feedback on #3276 asking for a test of the cyclic star-export with a CommonJS reexporting module.

…head e3f111d19)

…ndojs/endo#3276 (naugtur reviews persist)

When a module re-exports `*` from another module, and that other module re-exports a binding from the first under a *different* exported name (`export { y as x } from './mod1.js'`), the linker visited the first module while its star-imported notifier for `y` had not yet been wired. The synchronous wireUp at the cycle's back-edge then passed `undefined` as the upstream notifier, which manifested as `TypeError: notify is not a function` at `packages/ses/src/module-instance.js`. The original 2019 issue described the failure as `SyntaxError: ... does not provide an export named 'y'`; the surface symptom evolved as the linker matured, but the underlying defect is the same. `wireUpExportNotifier` now installs a deferred forwarding notifier for re-exports whose upstream notifier is not yet present. The forwarding queues subscribers until the upstream resolves, then drains them through. By the time any module downstream subscribes to the re-export, the upstream module has completed its candidate-all walk and the upstream notifier exists, so the chain converges. Two regression surfaces accompany the fix. In SES, `import-gauntlet` adds `cyclic star export with renaming reexport (issue #59)` exercising the exact reproducer from the issue against the SES linker directly. In the compartment mapper, a three-module fixture (`star-reexporter` re- exports `*` from `export-renamer`; `export-renamer` re-exports `y as x` from `star-reexporter`) drives the same shape through `loadLocation`, `importLocation`, the archive round-trip pair, and `makeArchiveFromMap`, plus a Node.js parity test that imports the same fixture under plain Node.js (no SES, no compartment mapper) and asserts identical expected values from a shared assertions module. Pinning the compartment mapper's behavior to Node.js's reference behavior keeps the expected values defined in one place and teases linker behavior out of SES rather than asserting it against itself. Reverting the `wireUpExportNotifier` change while keeping either test surface reproduces `TypeError: notify is not a function` (nine of the eleven compartment-mapper import-path variants fail; the two archive- integrity variants pass because they do not import the fixture; the Node.js parity test is unaffected). Re-applying the fix restores all twelve passing tests. Fixes #59

Companion regression for #59 addressing review feedback on #3276: a sibling fixture where the live binding `y` is declared but never assigned. Node.js reads every projection of the cycle as `undefined` for this shape (verified directly with `node`), so the SES linker must match. The deferring closure introduced by the fix either resolves (when a wireUp higher in the chain re-references the binding) or stays pending; the namespace reads agree with Node.js in either case. This commit adds executable evidence; no source change.

Introduce a synchronous variant of Promise.withResolvers. The helper returns { notify, resolve }: subscribers attached via notify(update) before resolve(targetNotify) is called are queued; once resolve is called, queued updaters are replayed to the target notifier and subsequent notify(update) calls forward directly through. resolve is one-shot (idempotent for repeat calls), which lets a caller invoke it lazily on each notify and have only the first invocation take effect. Apply the helper to the cycle-resolver branch of wireUpExportNotifier in module-instance.js, replacing the inline pendingUpdaters[] + resolvedUpstreamNotify state machine. The two patterns are now syntactically separated from their call site, reducing the chance that the local state machine will drift away from any future second use.

Add a regression test in import-cjs.test.js verifying that a cyclic star-export topology in which the "reexporter" is a CommonJS module behaves consistent with Node.js. Node.js rejects ESM-in-CJS-cycle outright (ERR_REQUIRE_CYCLE_MODULE), so the parity comparison is against the pure-CJS cycle: snapshot-at-call-time semantics for the CJS side (property capture sees whatever the renamer had assigned by the re-entry instant), live-binding semantics for the ESM side. Both namespaces project the same shapes the test pins. Addresses naugtur's review feedback on #3276 asking for a test of the cyclic star-export with a CommonJS reexporting module.

… tests (#59 follow-up) Mirror the cycle-rename parity-test layout for the pure-CommonJS cyclic reexporter scenario, in which Node.js and SES agree. The fixture under fixtures-cycle-cjs-reexporter/node_modules/app/ expresses the star-reexporter and renaming reexporter as on-disk .cjs modules with a live getter for the renamed export. cycle-cjs-reexporter.test.js runs the fixture through the compartment-mapper scaffold; cycle-cjs-reexporter-node-parity.test.js runs the same fixture under plain Node.js. Both tests assert through the shared _cycle-cjs-reexporter-assertions.js module, so parity is verified by construction: if both tests pass, the compartment mapper's CommonJS cycle behavior matches Node.js for this case.

…follow-up) Add a parity-test pair that programmatically verifies the divergence between Node.js and SES for the ESM-in-CommonJS-cycle topology. The fixture under fixtures-cycle-esm-in-cjs/node_modules/app/ has a CJS bridge module that require()s an ESM peer module that imports back from the bridge. cycle-esm-in-cjs.test.js asserts SES allows the topology and the namespace projects the live binding (bridgeValue === 42). cycle-esm-in-cjs-node-parity.test.js spawns a fresh Node.js process on the same fixture and asserts Node rejects with ERR_REQUIRE_CYCLE_MODULE. Together the two tests pin the divergence as a verified property rather than narrative prose.

…tment-mapper parity Rewrite the JSDoc on the cyclic CommonJS reexporter test in import-cjs.test.js and the companion unused-live-binding test in import-gauntlet.test.js so each block is primarily an explanation of what the test verifies (the shapes of the projected namespaces, the specific snapshot vs live-binding distinction, the parity property), not the procedural history of how the test came to be. The in-process SES regression is retained on the import-cjs.test.js side because it exercises the module-instance linker directly through the Compartment API with inline ModuleSources, a path the compartment-mapper parity suite does not cover. The prose now points at the parity suite for the parity-with-Node substantiation: packages/compartment-mapper/test/cycle-cjs-reexporter.test.js and its node-parity sibling for the pure-CommonJS agreement case; packages/compartment-mapper/test/cycle-esm-in-cjs.test.js and its node-parity sibling for the ESM-in-CJS-cycle divergence (ERR_REQUIRE_CYCLE_MODULE on Node, allowed on SES).

…59 follow-up) Add the parity test pair kriskowal asked for on endojs/endo-but-for-bots#379 (review comment 3338677487): the unused-live-binding shape of the cyclic star-export with renaming reexport (#59) needs a parity test substantiating the Node.js parity claim, ideally with a shared fixture as with cycle-rename parity. The companion populated-binding shape was already covered by cycle-rename.test.js and cycle-rename-node-parity.test.js through _cycle-rename-assertions.js; this commit lands the analogous trio for the unused-live-binding shape: fixtures-cycle-rename-unused/node_modules/app/ - three modules; the renamer's `export var y` has no initializer. _cycle-rename-unused-assertions.js - shared assertion module; expected projections are { x: undefined, y: undefined } and captured: undefined. cycle-rename-unused.test.js - compartment-mapper scaffold exercise. cycle-rename-unused-node-parity.test.js - Node.js exercise of the same fixture; parity is verified by construction when both pass. Also update the in-process SES regression's prose in packages/ses/test/import-gauntlet.test.js to reference the new parity pair, matching the cross-reference pattern the populated-binding test already uses for cycle-rename.

kriskowal mentioned this pull request May 21, 2026

fix(ses): cyclic star export with renaming reexport (issue #59) endojs/endo-but-for-bots#336

Closed

kriskowal changed the title ~~fix(ses): cyclic star export with renaming reexport (issue #59)~~ fix(ses): Cyclic star export with renaming reexport (issue #59) May 21, 2026

kriskowal marked this pull request as ready for review May 21, 2026 20:45

kriskowal force-pushed the kriskowal-star-export-cycle-rename branch 2 times, most recently from cbbb3dc to f4aad15 Compare May 22, 2026 00:43

kriskowal requested review from boneskull and naugtur May 22, 2026 02:36

naugtur reviewed May 29, 2026

View reviewed changes

kriscendobot mentioned this pull request May 29, 2026

fix(ses): cyclic star export with renaming reexport (issue #59) - refresh for #3276 feedback endojs/endo-but-for-bots#379

Open

0xpatrickbot mentioned this pull request Jun 3, 2026

fix(ses, module-source): support pi-mono in confined Endo Compartments endojs/endo-but-for-bots#297

Open

kriskowal pushed a commit to kriskowal/garden that referenced this pull request Jun 5, 2026

dispatch: liaison — boatman Shape-3 ff-append of bots#379's 7 test co…

ee0e1ea

…mmits onto endojs/endo#3276 (strip Refs trailers)

kriskowal pushed a commit to kriskowal/garden that referenced this pull request Jun 5, 2026

result: boatman fast-forward append 7 commits onto endojs/endo#3276 (…

f4063dc

…head e3f111d19)

kriskowal pushed a commit to kriskowal/garden that referenced this pull request Jun 5, 2026

result: liaison — boatman ff-appended bots#379's 7 test commits onto e…

bbc9530

…ndojs/endo#3276 (naugtur reviews persist)

kriskowal requested a review from naugtur June 5, 2026 04:53

kriskowal added 5 commits June 5, 2026 22:04

kriskowal added 3 commits June 5, 2026 22:04

kriskowal force-pushed the kriskowal-star-export-cycle-rename branch from e3f111d to 1725258 Compare June 6, 2026 05:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ses): Cyclic star export with renaming reexport (issue #59)#3276

fix(ses): Cyclic star export with renaming reexport (issue #59)#3276
kriskowal wants to merge 8 commits into
masterfrom
kriskowal-star-export-cycle-rename

kriskowal commented May 21, 2026

Uh oh!

changeset-bot Bot commented May 21, 2026 •

edited

Loading

Uh oh!

kriscendobot commented May 29, 2026

Uh oh!

naugtur May 29, 2026 •

edited

Loading

Uh oh!

kriskowal Jun 5, 2026

Uh oh!

naugtur May 29, 2026 •

edited

Loading

Uh oh!

kriskowal Jun 5, 2026

Uh oh!

naugtur May 29, 2026

Uh oh!

kriskowal Jun 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

kriskowal commented May 21, 2026

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Compatibility Considerations

Upgrade Considerations

Uh oh!

changeset-bot Bot commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

kriscendobot commented May 29, 2026

Uh oh!

naugtur May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kriskowal Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

naugtur May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kriskowal Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

naugtur May 29, 2026

Choose a reason for hiding this comment

Uh oh!

kriskowal Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

changeset-bot Bot commented May 21, 2026 •

edited

Loading

naugtur May 29, 2026 •

edited

Loading

naugtur May 29, 2026 •

edited

Loading