ref(server-utils): Ensure injected modules can reliably be picked up#22386
ref(server-utils): Ensure injected modules can reliably be picked up#22386mydea wants to merge 3 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| const INSTRUMENTATION_FN_SYMBOL = Symbol.for('InstrumentationFn'); | ||
| // oxlint-disable-next-line typescript/no-explicit-any | ||
| type InstrumentationFn = ((...args: any[]) => void) & { [INSTRUMENTATION_FN_SYMBOL]?: boolean }; |
There was a problem hiding this comment.
Unguarded any in new helper
Low Severity
New InstrumentationFn uses any with only an oxlint disable and no comment explaining why a safer type is impossible. Project review rules require that explanation for new any in SDK source.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit dcccddd. Configure here.
size-limit report 📦
|
The Turbopack value-injection seeded `__SENTRY_ORCHESTRION__` by assigning a
fresh `{ bundler: [] }` object, replacing any existing marker. Unlike the Bun
banner and `buildInjectPrologue`, this was not merge-safe, so a hybrid
runtime+bundler setup could wipe already-recorded runtime module names before
integrations read them.
The value-injection loader can only emit plain assignments, so it now accepts an
optional raw `prefixCode` and the marker is injected as a merge-safe snippet that
only creates the global/`bundler` array when absent.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1ba8963 to
3e8c1ea
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| ';(function(){try{' + | ||
| 'var g=(globalThis.__SENTRY_ORCHESTRION__=globalThis.__SENTRY_ORCHESTRION__||{});' + | ||
| `var m=${names};` + | ||
| 'g.bundler=m;' + |
There was a problem hiding this comment.
Bug: The buildInjectBootSnippet function uses a destructive assignment (g.bundler=m;) which could overwrite pre-existing module records, unlike other parts of the code that use a merge-safe pattern.
Severity: LOW
Suggested Fix
To ensure consistency and prevent potential data loss, modify the assignment in buildInjectBootSnippet to be merge-safe. Change 'g.bundler=m;' to a pattern that appends to the existing array, such as 'g.bundler=[...((g.bundler)||[]),...m];'. This aligns with the defensive coding practices used elsewhere in the pull request.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/server-utils/src/orchestrion/bundler/inject.ts#L50
Potential issue: The `buildInjectBootSnippet` function uses a destructive assignment
(`'g.bundler=m;'`) to set the bundler modules. This will overwrite any pre-existing
values on the global `__SENTRY_ORCHESTRION__.bundler` object. While the current
architecture may separate bundler paths (e.g., Turbopack vs. Webpack), preventing this
issue in typical setups, it's an inconsistent and non-defensive pattern. Other parts of
the codebase, such as `buildInjectPrologue`, were explicitly updated to use a merge-safe
approach. This oversight could lead to silent loss of module registration data in more
complex or future hybrid build configurations.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6098cab. Configure here.
| export function getOrchestrionInjectedModules(): string[] { | ||
| const { runtime, bundler } = GLOBAL_OBJ.__SENTRY_ORCHESTRION__ ?? {}; | ||
| return [...(runtime ?? []), ...(bundler ?? [])]; | ||
| } |
There was a problem hiding this comment.
Legacy bundler marker crashes module list
Medium Severity
getOrchestrionInjectedModules spreads runtime and bundler after a nullish check only. Legacy Bun banners (and other callers) still use bundler: true, and true ?? [] stays true, so [...true] throws at runtime. That will break invokeOrchestrionInstrumentation for apps that upgrade the SDK without rebuilding.
Reviewed by Cursor Bugbot for commit 6098cab. Configure here.
2 participants
Makes orchestrion-injected modules reliably discoverable at runtime, regardless of how (and when) they were injected.
This fixes two scenarios:
Turbopack
Here, only loaders are accepted, not plugins, so there is no build lifecucle hook to emit the aggregate list of instrumented modules at boot the way the webpack plugin does.
Other Bundlers
While here we can/do inject the aggregate list of modules at build time, depending on how the SDK is initialized this may or may not be defined before
Sentry.init()is called - wheninit()is called ahead of the bundled server code with--import, the module list may not be available yet at this point.The Fix
To fix this, injected modules now announce themselves through a shared set of runtime globals:
buildInjectPrologue) per instrumented module (Turbopack) or a boot-time snippet with the full module list (webpack/rollup/vite/esbuild). Both record the module in__SENTRY_ORCHESTRION__.bundlerand call the SDK bridge (__SENTRY_ORCHESTRION_ON_INJECT__) so channel subscribers are wired up before the module publishes — this matters whenSentry.init()runs ahead of the bundled server (e.g. via--require).invokeOrchestrionInstrumentationhelper bridges these announcements to integration setup: it invokes the instrumentation callback immediately for already-injected (build-time) modules, or later when runtime injection happens, and guarantees the callback runs at most once. Bun/Deno register immediately since module tracking is unavailable and channel limits don't apply there.This is the base of a stack; the follow-up migrates all channel integrations onto this helper.