Conversation
|
Updated 10:02 PM PT - Jul 8th, 2026
❌ @robobun, your commit 0152a86 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33793That installs a local version of the PR into your bun-33793 --bun |
WalkthroughThis PR changes export-value retrieval in BunPlugin.cpp and ObjectModule.cpp to propagate thrown getter exceptions immediately instead of substituting undefined. New tests verify the behavior for object-loaded modules and mock.module. ChangesThrowing getter propagation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/jsc/bindings/BunPlugin.cpp`:
- Around line 662-674: The export override path in BunPlugin.cpp delays the
`MarkedArgumentBuffer::hasOverflowed()` check until after the loop that reads
`object->get(...)`, unlike the fail-fast pattern used elsewhere such as
`OnResolve::run`. Move the overflow check immediately after
`values.ensureCapacity(names.size())` in the
`moduleNamespaceObject->overrideExportValue` flow, and keep the existing
post-loop check so `object->get` is never invoked if reserving the buffer
already failed.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4505aafc-437e-40f9-95fa-693f17239602
📒 Files selected for processing (5)
src/jsc/bindings/BunPlugin.cppsrc/jsc/modules/ObjectModule.cpptest/js/bun/plugin/plugins.test.tstest/js/bun/test/mock/mock-module-getter-throw-fixture.tstest/js/bun/test/mock/mock-module.test.ts
There was a problem hiding this comment.
Both earlier review threads are addressed and I don't see any remaining issues, but this changes user-visible behavior (imports/mocks that previously resolved with undefined now throw) and restructures a GC-sensitive loop in JSMock__jsModuleMock, so it's worth a quick maintainer glance before merging.
Extended reasoning...
Overview
Two native changes plus tests:
src/jsc/modules/ObjectModule.cpp: ingenerateObjectModuleSourceCode, replaces the per-propertytryClearException()+jsUndefined()fallback withRETURN_IF_EXCEPTION(throwScope, void()), matching the siblinggenerateObjectModuleSourceCodeForJSONin the same file.src/jsc/bindings/BunPlugin.cpp: themock.module()ESM-override loop is restructured into two phases — collect all export values into aMarkedArgumentBuffer(withRETURN_IF_EXCEPTIONpergetandhasOverflowed()checks matchingOnResolve::run), then applyoverrideExportValue— so a throwing getter surfaces before the live namespace is mutated.- New tests in
plugins.test.ts(spawned subprocess coveringimport()andrequire()throughbuild.module+loader: "object") andmock-module.test.ts(two-export fixture proving the namespace is left untouched on throw).
Security risks
None identified. No auth/crypto/permissions surface; input is a user-supplied JS object whose getters are already being invoked today — the change only stops swallowing their exceptions. The MarkedArgumentBuffer roots collected values across the read loop, and overflow is checked both after ensureCapacity and after the append loop.
Level of scrutiny
Medium. The ObjectModule.cpp hunk is a mechanical 4→1 line swap onto an existing throwScope and mirrors adjacent code exactly. The BunPlugin.cpp hunk is larger (~20 lines) and sits in JSC bindings where GC rooting and exception-scope discipline matter, but it copies the ensureCapacity/hasOverflowed pattern already used elsewhere in the same file. The main reason for a human look is that this is a behavior change: Bun.plugin loader: "object" imports and mock.module() calls that previously succeeded (with undefined for the throwing export) will now reject/throw. The removed comment ("consistent with regular esm handling code") suggests the swallow was once deliberate, and while the PR argues convincingly that propagation is correct and consistent with sibling paths, that product call is worth a maintainer confirming.
Other factors
All three prior review threads (my two and CodeRabbit's overflow-ordering nit) were addressed in follow-up commits and are resolved. The bug-hunting pass found nothing on the current revision. Test coverage now exercises both changed code paths and the atomicity guarantee. The analogous swallow in JSCommonJSModule.cpp is explicitly and correctly left alone (documented Node-compat behavior).
When a runtime plugin returns { exports, loader: "object" } and one of
the enumerable properties on the exports object has a getter that
throws, the exception was being cleared and the export was silently set
to undefined. The import resolved successfully with the wrong value and
nothing surfaced to the importer or the plugin.
generateObjectModuleSourceCode now propagates the exception via
RETURN_IF_EXCEPTION, matching what generateObjectModuleSourceCodeForJSON
and generateInternalModuleSourceCode already do for the same per-property
Get, and what this function already did for a throwing ownKeys trap.
The mock.module() ESM override path in BunPlugin.cpp carried a copy of
the same swallow with a comment pointing at the code above; it now
propagates as well.
The BunPlugin.cpp change is only reachable via mock.module() on a specifier that is already linked in the ESM registry. The plugins.test.ts case goes through build.module() and never touches that loop, so add a dedicated test that imports a fixture at module scope and then mocks it with a factory whose getter throws.
Previously a throwing getter on a later property would leave the module namespace with earlier properties already overridden. Collect every value into a MarkedArgumentBuffer first, then apply the overrides in a second pass so a getter exception leaves the live namespace unchanged. The test fixture now exports two bindings and the mock supplies a plain value for the first so the untouched-namespace assertion would fail on a torn state.
Matches the sibling pattern a few hundred lines below in OnResolve::run.
eb6e064 to
0152a86
Compare
There was a problem hiding this comment.
Looks correct and both earlier concerns are addressed, but this changes user-visible error semantics for loader: "object" / mock.module() (silently-undefined exports now reject the import) in the module-loading path — worth a human confirming that's the intended contract.
Extended reasoning...
Overview
Two C++ changes in JSC bindings plus tests:
src/jsc/modules/ObjectModule.cpp:generateObjectModuleSourceCodenow propagates getter exceptions viaRETURN_IF_EXCEPTION(throwScope, void())instead of clearing them and substitutingjsUndefined(). This aligns it with the siblinggenerateObjectModuleSourceCodeForJSONimmediately below in the same file.src/jsc/bindings/BunPlugin.cpp: the ESM-override loop inJSMock__jsModuleMockwas reworked into a two-phase read-then-write using aMarkedArgumentBuffer(withensureCapacity+hasOverflowedchecks matchingOnResolve::runin the same file), so a throwing getter surfaces before any live-namespace mutation.- Tests cover both entry points:
Bun.pluginbuild.module()via a spawned subprocess inplugins.test.ts, andmock.module()via a two-export fixture inmock-module.test.tsthat would fail on a torn state.
Both of my earlier inline concerns (missing coverage for the mock.module path; torn-namespace on partial failure) were addressed in follow-up commits, and the CodeRabbit fail-fast overflow-check nit was applied.
Security risks
None identified. No untrusted-input parsing, no auth/crypto/permissions surface. The change tightens error handling rather than loosening it.
Level of scrutiny
Medium-high. The mechanical change is small and matches established sibling patterns, but it lives in the module-loader / plugin path (generateObjectModuleSourceCode backs loader: "object" for all runtime plugins) and it is a user-visible behavior change: exports objects with throwing getters that previously produced undefined silently will now reject the import. The PR description explicitly leaves the analogous CJS swallow in JSCommonJSModule.cpp untouched as intentional Node-compat, which is a design distinction a maintainer should ratify.
Other factors
- No bugs surfaced by the bug-hunting pass on the current revision.
- Tests are well-constructed (subprocess isolation, sentinel identity check, atomicity assertion on a two-binding fixture) and were verified to fail on stock bun / pass on the branch per the author's replies.
- The
BunPlugin.cpphunk grew from a one-line swap into ~20 lines of newMarkedArgumentBuffer+ overflow-handling logic during review; while it mirrors an existing pattern in the file, it's enough new C++ in a JSC-bindings hot area that I'd rather a human confirm than auto-approve.
|
Diff is green: the The red on build #70781 is unrelated to this change:
None of these touch the plugin or |
|
Checked whether #37026 made this redundant. It did not: #37026 handles a throwing getter on the On current main (165dc9f), both tests from this PR still fail with a debug build:
For contrast, #37026's scenario (getter on the result's |
|
Cross-checked this PR against #39804, which touches the same two files. Result: #39804 covers part of this PR, so this one stays open for a rebase after #39804 lands. Covered by #39804:
Not covered by #39804:
Rebase plan once #39804 is in: drop the |
|
Both hunks of this PR landed on main in #39804 (commit 025570f). I applied the test additions from this PR to main at 40ef811 with no The one piece that did not land is the Closing as superseded. |
) ### Problem - #39804 (commit 025570f) changed `generateObjectModuleSourceCode` (`src/jsc/modules/ObjectModule.cpp:25`). A throwing getter on the exports object of a `loader: "object"` result now fails the import. Before, the loader exported `undefined` for it. - #39804 added tests for the `mock.module()` entry point only. Its description lists this change under "no repro", but a plugin reaches it: bun `1.4.0-canary.1+6e906e468` (before #39804) prints `boom=undefined` for the cases below. - This replaces #33793. Its code change landed through #39804. Its plugin test did not. ### Fix - Test only. Adds `describe("object loader with a throwing getter on an export")` to `test/js/bun/plugin/plugins.test.ts`, next to the #37026 block for a throwing getter on the result's `exports` property. - Three cases: `import()` and `require()` of a `build.module()` result, and `import()` of a `build.onLoad()` result. Each checks that the caught error is the object the getter threw. The getter sits between two plain exports. - Verified: all three fail with `USE_SYSTEM_BUN=1` (`1.4.0-canary.1+6e906e468`) and pass with a debug build of main at 40ef811. The full file passes there (45 tests). ### Background - A `loader: "object"` result becomes a synthetic module. `ModuleLoader.cpp:442` passes its `exports` object to `generateObjectModuleSourceCode`, which reads each own enumerable property once into the namespace. `require()` takes the same path after the `__esModule` check at `ModuleLoader.cpp:422`. - `mock.module()` of a module that is not loaded yet uses the same generator. That is the entry point #39804 tests. - #37026 covers the layer above: a getter on the `exports` property of the result object (`ModuleLoader.cpp:155`). <details><summary>Notes</summary> - Output on the old binary: `imported boom=undefined` for the two `import()` cases and `required boom=undefined` for the `require()` case. - #33793's test ran `import()` and `require()` in one subprocess. This version mirrors the shape of the #37026 block: one subprocess per entry point, exact `stdout`, empty `stderr`, exit code 0. - #33793's `mock.module()` test is not carried over. #39804 added two tests for that entry point in `test/js/bun/test/mock/mock-module.test.ts` (the import failure and the untouched namespace). Both of #33793's test cases pass on a debug build of main with no `src/` change. That is the basis for closing #33793. - Commands: `bun bd test test/js/bun/plugin/plugins.test.ts` (45 pass) and `USE_SYSTEM_BUN=1 bun test test/js/bun/plugin/plugins.test.ts -t "throwing getter on an export"` (3 fail). </details>
### Problem - GitHub closes only the first reference after a keyword, so "Fixes #1, #2" leaves #2 open. "Supersedes #3" links nothing, and no reference closes a pull request. - The last 1000 merged PRs name 274 such references. PR #32292 is open although merged #36135 says "Supersedes #32292". ### Fix - `.github/workflows/close-linked-issues.yml` runs on `pull_request_target` `closed` (a merge into the default branch of `oven-sh/bun`) and on `workflow_dispatch` with a PR number and `dry_run`. Everything is inline in one `actions/github-script` step, with no checkout. - Each open target is closed as `completed` with the comment "Closed as completed by #N." or "Superseded by #N.". Closed or missing targets, the PR itself and other repositories are skipped. - The parser has no regex. A closing keyword (close, fix, resolve, supersede, replace, any tense) must lead the reference, alone or in a list. A negated, hedged or noun keyword, or one whose subject is another reference, does not count ("may fix", "the rm fix #1", "#100 supersedes #1"). - Verified: `test/internal/close-linked-issues.test.ts` (333 cases) runs the YAML's script against fake `github`, `context` and `core`. Also the 1000-PR parse (Notes). ### Background - GitHub's own keywords are close, fix and resolve (-s, -ed). Each links one reference, and only a merge into the default branch closes it. - `pull_request_target` runs in the base repository with a write token, also for fork PRs. That is safe only when no PR-controlled code runs. Here the description is the only PR input, parsed as text. <details><summary>Notes</summary> A close through the API does not create the "closed this in #N" timeline link that GitHub makes for its own closes. The comment carries the PR number instead. How the parser was calibrated. I pulled the descriptions of the last 1000 merged PRs and listed every line with a keyword next to a reference. The keyword families, list shapes and reference forms in the script are the ones that appear there. A reference is `#1`, `owner/repo#1`, an issue or pull URL (bare or in `<>`), or a markdown link. Four lines would have been wrong with a plain keyword-then-reference rule, and each led to a rule: - "the open `rm` fix #37521" (#38379): "fix" as a noun. Base forms (fix, close, resolve, supersede, replace) count only at the start of a sentence or line, or after will, should, does, and, and a few similar words. "to" is not one of them ("unable to fix #1", "how to fix #1"). - "May also fix #12318 / #10046, untested" (#38242): hedged. may, might, could, would, partially and the negations disqualify the keyword, looking past adverbs such as "also". - "Supersedes the closed #26040" (#36289) and "a comment on closed #35351" (#35365): "closed" as an adjective. A determiner or preposition before the keyword disqualifies it. - "supersedes #33130's optimisation" (#35843): a number that continues into a word is not a reference. Review added: a reference before the keyword is the subject ("#100 supersedes #1"), also through "which" or "that" ("reverts #100, which fixed #1") and across a removed span ("#100 ~~also~~ fixes #1"). A hedge two words before the keyword disqualifies it ("hopefully this fixes #1", "could this fix #1?"). A clause that starts with if, when, once, until or unless is not a statement. The tokenizer keeps a line break as a token so that "Fixes #1" on one line and "Fixes #2" on the next stay two statements. Code spans, fences, indented code, blockquotes, HTML comments and strikethrough are skipped. The block stripping follows CommonMark for fences (also inside a blockquote), indented code, blockquotes with lazy continuation, setext underlines and HTML comments, and GFM for `~~` flanking. Result over the 1000 descriptions: 274 distinct references in 135 PRs. I checked the current state of all of them through GraphQL. All but one are closed (202 issues completed, 5 duplicates, 66 pull requests). The one open target is PR #32292, superseded by merged #36135. No open target is a false positive. Every review change kept this result. Patterns that are deliberately not handled: a bulleted list under "Closes:" on its own line (not seen in the sample), references separated by whitespace only ("#1 #2"), "fix for #1", and GH-1 style references. A `?` after the list is not treated as a question. The block parser tracks no list containers, so a second paragraph of a list item indented by four spaces is read as an indented code block and skipped. A removed span or inline comment reads as one word, so "Fixes <!-- n --> #1" finds nothing. The test suite covers: the phrases above, stopping at the right place in real sentences, CRLF descriptions, URLs with fragments or a `/files` suffix, case-insensitive `Owner/Repo#1`, the fake API where a lookup, an update or a comment fails, the `dry_run` input, an invalid `pr_number` input, an unmerged PR, a PR merged into a non-default branch, the merge event body against a later edit, and a description with no closing statement. The first revision of this PR checked out the repository and ran `scripts/close-linked-issues.ts`. Jarred asked for no checkout and no script file, so the script moved inline into the workflow and the test now reads it out of the YAML. </details> <!-- robobun:evidence:begin --> --- **[stamp-90s]** gate passed · iteration 9 · 2 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/internal/close-linked-issues.test.ts' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/internal/close-linked-issues.test.ts bun test v1.4.1 (4448a2e) test/internal/close-linked-issues.test.ts: (pass) finds "Fixes #39852" [176.21ms] (pass) finds "Closes #31772. Fixes #31771." [22.28ms] (pass) finds "- Fixes #39930" [12.28ms] (pass) finds "Fixes: #30429" [10.46ms] (pass) finds "FIXES #1" [7.86ms] (pass) finds "(Fixes #1)" [8.97ms] (pass) finds "**Fixes #1**" [10.20ms] (pass) finds "__Fixes #1__" [9.83ms] (pass) finds "_Fixes #1_" [11.25ms] (pass) finds "Fixes **#1**" [9.72ms] (pass) finds "**Fixes** #1" [7.13ms] (pass) finds "**Fixes:** #1" [8.11ms] (pass) finds "Fixes #1 and **#2**" [11.47ms] (pass) finds "Fixes **#1**, **#2**" [9.13ms] (pass) finds "## Why (fixes #13771, closes #30543)" [16.08ms] (pass) finds "Closes #11418" [19.46ms] (pass) finds "Resolves #1. Resolved #2. Resolve #3." [12.09ms] (pass) finds "Fixes #34055, #30327, #24394, #20816, #32403, #11898, #10056." [17.11ms] (pass) finds "Fixes #18192 and #31675 as a consequence" [10.45ms] (pass) finds "Fixes #1, #2, and #3" [10.96ms] (pass) finds "Fixes #1 & #2" [7.63ms] (pass) finds "Closes #33280, Closes #32864 and Closes #29696 (the timer in #32949 is orthogonal)" [20.29ms] (pass) finds "Closes #33182 and #32947 on top of current main (which already has #36304 for catalogs)." [16.12ms] (pass) finds "Fixes #1,\n#2" [7.76ms] (pass) finds "Fixes #1, #2,\nand #3" [9.27ms] (pass) finds "Fixes #1\nand #2" [8.57ms] (pass) finds "Fixes #1\n& #2" [6.80ms] (pass) finds "Fixes #1 and\n#2" [7.31ms] (pass) finds "Supersedes #39908 (same change, moved from a fork branch)" [13.21ms] (pass) finds "Supersedes #38778 and #38391. Carries the entry point arm of #35053." [14.43ms] (pass) finds "Supersedes #39193 and keeps its three tests." [11.48ms] (pass) finds "This supersedes #33306 and #32803. Their tests are kept here." [13.73ms] (pass) finds "- This replaces #33793. Its ... (truncated) Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` .github/workflows/close-linked-issues.yml | 950 ++++++++++++++++++++++++++++++ test/internal/close-linked-issues.test.ts | 598 +++++++++++++++++++ 2 files changed, 1548 insertions(+) ``` </details> **gate history** · 29 passed · 0 rejected · iteration 9 <details><summary>evidence per changed file</summary> ``` file reads edits tests .github/workflows/close-linked-issues.yml 6 12 0 test/internal/close-linked-issues.test.ts 3 11 0 ``` </details> <!-- robobun:evidence:end -->
1 participant
Repro
The import resolves, the
boomexport exists, and its value isundefined. Nothing is surfaced to the importer or the plugin. The same object handed to a plain JS spread ({ ...exportsObj }) throws, and a throwing ProxyownKeystrap on the same object already rejects the import; only the per-propertyGetexception was being dropped.Cause
generateObjectModuleSourceCodeinsrc/jsc/modules/ObjectModule.cppiterates the exports object's own property names and callsobject->get(globalObject, entry)for each. If theGetthrew, it cleared the exception and storedjsUndefined():The sibling
generateObjectModuleSourceCodeForJSONin the same file andgenerateInternalModuleSourceCodeinModuleLoader.cppboth useRETURN_IF_EXCEPTIONafter the samegetcall, and this function already usesRETURN_IF_EXCEPTIONaftergetOwnPropertyNames.Fix
Replace the clear-and-default with
RETURN_IF_EXCEPTION(throwScope, void()), so the getter's exception propagates and the import rejects with it.The
mock.module()ESM-override path insrc/jsc/bindings/BunPlugin.cpphad the same swallow with a comment reading "consistent with regular esm handling code" (i.e. the code above). That copy is updated to propagate as well so the two paths stay consistent.The analogous swallow in
JSCommonJSModule.cppis left untouched: it is documented as matching Node.js behaviour for CJS named-export enumeration and is a different code path.Verification
[review] gate passed · iteration 2 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 2
evidence per changed file