Skip to content

fix(runtime): expose onResolve import kinds - #42952

Open
steipete wants to merge 1 commit into
oven-sh:mainfrom
steipete:claude/runtime-onresolve-import-kinds
Open

steipete wants to merge 1 commit into
oven-sh:mainfrom
steipete:claude/runtime-onresolve-import-kinds

Conversation

@steipete

@steipete steipete commented Sep 16, 2026 •

Copy link
Copy Markdown
Collaborator

What does this PR do?

Exposes runtime onResolve import kinds through #44473's resolve-once loader. Dynamic imports pass their kind into the first resolver dispatch; that marker is consumed before calling plugins, so nested resolutions and static imports keep their own kinds. The Rust/C++ boundary reports dynamic-import, require-call, require-resolve, or import-statement as appropriate.

Bare-alias admission uses the same guarded dispatch as #44593, adapted from @robobun's #40398. Builtins, nested bare imports and custom search paths retain their existing exclusions. No PluginRunner or second loader resolve is restored.

How did you verify your code works?

Built on macOS arm64 against main 9bd19c9. The full plugin file passes 97 tests, retaining the original kind assertions and adding static-import/nested-resolution coverage. The query-string resolution file passes 15 tests. Independent review through P2 is clean. No new cross-platform or ASAN qualification is claimed.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai

coderabbitai Bot commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 86047ac7-bceb-4def-b133-080b0c7797bd
📥 Commits

Reviewing files that changed from the base of the PR and between d08c5c0 and 0b6811f.

📒 Files selected for processing (10)
  • docs/runtime/plugins.mdx
  • src/js/builtins/BunBuiltinNames.h
  • src/jsc/JSGlobalObject.rs
  • src/jsc/VirtualMachine.rs
  • src/jsc/bindings/BunPlugin.cpp
  • src/jsc/bindings/BunPlugin.h
  • src/jsc/bindings/ZigGlobalObject.cpp
  • src/jsc/bindings/ZigGlobalObject.h
  • src/jsc/bindings/headers.h
  • test/js/bun/plugin/plugins.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.


Walkthrough

The resolver distinguishes dynamic imports from other ESM resolution, passes import-kind labels to onResolve callbacks, and adds plugin resolution for eligible bare package specifiers. Tests and plugin documentation cover callback kinds and bare-name behavior.

Changes

Resolver plugin flow

Layer / File(s) Summary
Dynamic import mode
src/jsc/bindings/ZigGlobalObject.cpp, src/jsc/bindings/ZigGlobalObject.h, src/jsc/bindings/headers.h, src/jsc/JSGlobalObject.rs, src/jsc/VirtualMachine.rs
The module loader marks dynamic-import resolution. The resolver selects DynamicImport or Esm, and DynamicImport maps to the dynamic import kind.
Import-kind delivery to plugins
src/jsc/VirtualMachine.rs, src/jsc/JSGlobalObject.rs, src/jsc/bindings/BunPlugin.cpp, src/jsc/bindings/BunPlugin.h, src/js/builtins/BunBuiltinNames.h, test/js/bun/plugin/plugins.test.ts
Resolver calls pass the import kind through the Rust and C++ plugin interfaces to onResolve callback parameters. Tests check kinds for dynamic imports, require, require.resolve, Bun.resolveSync, and import.meta.resolve.
Bare package plugin fallback
src/jsc/VirtualMachine.rs, docs/runtime/plugins.mdx
Eligible bare package specifiers reach plugins with an empty namespace. The fallback skips the listed cases, including nested runtime onResolve calls and configured custom directory paths. The documentation describes bare-name redirection and exclusions.

Suggested reviewers: robobun

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 0b681

The identified concerns do not block merging. The callback kind is documented, and a panic cannot leave a running process with the reported stale resolver state.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 0b681

Registered plugins can redirect more package imports, while builtin, nested-resolution, and custom-search-path safeguards remain. No introduced security flaw was established. Dynamic-import classification still depends on a timing assumption that could not be fully verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — An attacker-controlled eligible package specifier can newly trigger a matching, already-registered callback and influence which file or virtual module the runtime loads. The effective scope is module selection within the affected runtime and its existing authority. This establishes expanded interception, not an untrusted plugin-registration path or privilege escalation.

Trust Boundaries and Controls

  • observed — The new fallback rejects empty specifiers and importers, non-package paths, active onResolve nesting, custom directory search paths, and hardcoded aliases. Dispatch additionally requires a registered namespace group and matching filter. Plugin results retain existing path and namespace validation rather than gaining a new result authority.

Resilience and Maintainability Implications

  • observed — The recursion counter starts at zero through VM initialization and is decremented before callback errors propagate. The dynamic-import flag starts false, is scoped around requestImportModule with deferred=false, and is consumed before resolver early returns or plugin reentry. Local cleanup prevents stale state on inspected return paths, but correct classification still depends on the external loader performing its first resolution within that scope; that dependency was not independently verified.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: exposing import kinds to runtime onResolve callbacks.
Description check ✅ Passed The description includes both required sections. It explains the change, key resolution behavior, preserved exclusions, and verification performed.

Comment @coderabbitai help to get the list of available commands.

Carry caller intent across the Rust/C++ plugin boundary and consume the
synchronous dynamic-import marker before callbacks can reenter resolution.
Retain static, require, require.resolve and runtime resolver distinctions.

Use oven-sh#44593's bare-alias guards adapted from @robobun's oven-sh#40398. Preserve
original assertions and add static-import and nested-resolution coverage.
@steipete
steipete force-pushed the claude/runtime-onresolve-import-kinds branch from d08c5c0 to 0b6811f Compare October 5, 2026 10:29
@steipete

steipete commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

Reworked kind propagation at #44473's single runtime resolve. The dynamic-import marker is consumed before plugin reentry; static and nested resolution retain their own kind. Bare aliases use #44593/#40398's guarded dispatch. On macOS arm64 against 9bd19c9, the original plugin file had 95 pass / 1 fail; the refreshed file has 97 pass / 0 fail, including added static/nested coverage. Query resolution passes 15 tests; P2 review is clean.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant