feat: multi-realm patch safety + handle.flush() for dispose parity (#11, #19)#40
Merged
Merged
Conversation
…11, #19) Wave 4 (PR #38) is merged; this commit lands the Wave 5 plan + spec on the implementation branch alongside the roadmap update. Wave 5 covers issues #11 (multi-realm patch model — dual-package, uninstall identity check, worker_threads docs) and #19 (Python sync vs Node async dispose parity — adds RecostHandle.flush()).
#11) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
#11) Replace five module-level `let` slots with a single `InterceptorState` object stored on `globalThis` under `Symbol.for("@recost-dev/node:interceptor-state")`. `patchedFetch`, `makeRequestWrapper`, `install`, `uninstall`, `isInstalled`, and `getRawFetch` all read/write through `getState()`. Adds 1 regression test that verifies `globalThis[STATE_KEY]` is populated with `installed=true` and the saved original/patched fetch references after `install()`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…test (review fixes for #11) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds setOnError() to wire a host-supplied callback into the interceptor state; install() now fires RecostInterceptorAlreadyInstalledError via that callback on a second call instead of silently no-oping. init() calls setOnError(config.onError ?? null) before install() so the error routes through the user's configured handler. setOnError is re-exported from src/index.ts as part of the public API. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…kage test (review fixes for #11) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ering third-party patches (#11) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tity-check block (review fixes for #11) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR implements Wave 5 (Multi-realm Patch Safety & Dispose Parity) by migrating interceptor singleton state from module-level to globalThis-keyed shared storage, enabling dual-package coordination; adding advisory error types for conflict detection; introducing a new ChangesMulti-realm Patch Safety & Dispose Parity
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@docs/superpowers/plans/2026-05-18-multi-realm-and-dispose-parity.md`:
- Line 1525: Find each unlabeled fenced code block (the triple-backtick blocks
shown in the diff) and add an explicit language identifier after the opening
````` to satisfy MD040; for example convert ``` to ```markdown for prose
samples, ```ts or ```typescript for TypeScript snippets, and ```bash or ```sh
for shell snippets. Update the fence instances referenced in the review (the
empty ``` blocks around the discussed sections) so every fenced block in this
document has a language tag.
In `@docs/superpowers/roadmap-2026-05-13-issue-waves.md`:
- Around line 106-110: The "Wave 5 PR-shape guidance" text is stale: update the
Wave 5 section so it matches the linked spec/plan
(specs/2026-05-18-multi-realm-and-dispose-parity-design.md and
plans/2026-05-18-multi-realm-and-dispose-parity.md) by replacing the phrase "two
plans, two PRs" (currently at Line 121) with wording that reflects the bundled
approach (e.g., "single plan and single PR" or "one plan/PR"), and ensure any
surrounding sentences no longer reference two separate plans/PRs so the guidance
is consistent with the linked spec/plan.
In `@docs/superpowers/specs/2026-05-18-multi-realm-and-dispose-parity-design.md`:
- Around line 92-96: The spec is inconsistent: the pseudocode sets
state.installed = false while the conflict contract requires state.installed
remain true to block re-entry after bindings are skipped; update the pseudocode
so that state.installed is not cleared when uninstall was skipped (leave
state.installed = true) and add a clarifying comment next to state.installed,
state.callback and the install() re-entry gate; alternatively, if the intended
behavior is to allow re-install, update the conflict contract text to match and
explicitly describe when state.installed is toggled—referencing state.installed,
state.callback and install() to ensure both the code path and the contract are
aligned.
In `@README.md`:
- Around line 162-163: The README sample config shows apiKey (symbol: apiKey)
without the required projectId; update the example to include projectId whenever
apiKey is set so it matches the validation rules (reference symbols: apiKey and
projectId) and prevents runtime validation errors; edit the sample config block
to add a projectId entry adjacent to apiKey and ensure the onError block remains
unchanged.
In `@src/core/interceptor.ts`:
- Around line 51-52: The global boolean guard state.inFetchWrapper is causing
unrelated concurrent http.request/https.request calls to be suppressed; replace
this global re-entrancy flag with a per-async-context guard using Node's
AsyncLocalStorage so only requests causally triggered by the current fetch are
skipped. Specifically, introduce an AsyncLocalStorage store (e.g., fetchContext)
and set a value (like {inFetch: true}) around the wrapper that replaces
originalFetch, read that store in the interceptors that currently check
state.inFetchWrapper (references: state.inFetchWrapper, originalFetch, and the
fetch wrapper code paths), and update the checks at the locations noted (the
fetch wrapper enter/exit and the http/https request interceptors) to consult
fetchContext.getStore()?.inFetch instead of the global flag; ensure the store is
correctly propagated across async boundaries and cleaned up on completion.
In `@src/init.ts`:
- Around line 204-213: The flush() method currently swallows errors from
flushAndSend(); update its catch to route failures to the configured error
handler by calling the module's onError (or config.onError) with the caught
error (and context if available) instead of silently ignoring it; keep the
disposed guard and existing idempotency but ensure the catch block invokes
onError(err) so flush() preserves the documented error contract (symbols:
flush(), flushAndSend(), onError / config.onError, disposed).
In `@tests/interceptor.test.ts`:
- Around line 989-1010: The cleanup currently restores only fetch/http and then
deletes STATE_KEY, leaving any https monkey patches in place; import node:https
in this test file and update the cleanup branch that checks s?.installed to also
restore s.originalHttpsRequest and s.originalHttpsGet onto the https module
(e.g., (https as unknown as { request: typeof https.request }).request =
s.originalHttpsRequest and similarly for get) before deleting STATE_KEY, using
the existing STATE_KEY and s symbols to locate the state object.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 76e087d0-b0eb-48c0-9058-c4b30320980c
📒 Files selected for processing (10)
README.mddocs/superpowers/plans/2026-05-18-multi-realm-and-dispose-parity.mddocs/superpowers/roadmap-2026-05-13-issue-waves.mddocs/superpowers/specs/2026-05-18-multi-realm-and-dispose-parity-design.mdsrc/core/interceptor.tssrc/core/types.tssrc/index.tssrc/init.tstests/init.test.tstests/interceptor.test.ts
| ``` | ||
|
|
||
| The main thread's `init()` does not propagate to workers, and the SDK does not detect or warn about worker spawns — instrumenting workers is the host's responsibility. | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to unlabeled fenced code blocks.
These fences trip MD040 in markdownlint. Use explicit languages (e.g., markdown, ts, bash) for each affected block to keep the doc lint-clean.
Also applies to: 1542-1542, 1567-1567, 1586-1586, 1614-1614
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 1525-1525: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for 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.
In `@docs/superpowers/plans/2026-05-18-multi-realm-and-dispose-parity.md` at line
1525, Find each unlabeled fenced code block (the triple-backtick blocks shown in
the diff) and add an explicit language identifier after the opening ````` to
satisfy MD040; for example convert ``` to ```markdown for prose samples, ```ts
or ```typescript for TypeScript snippets, and ```bash or ```sh for shell
snippets. Update the fence instances referenced in the review (the empty ```
blocks around the discussed sections) so every fenced block in this document has
a language tag.
Comment on lines
+51
to
+52
| inFetchWrapper: boolean; | ||
| originalFetch: typeof globalThis.fetch | null; |
There was a problem hiding this comment.
Global re-entrancy guard drops unrelated concurrent HTTP events.
Line 408 uses a process-wide state.inFetchWrapper flag. While one fetch() is in-flight (Line 287→380), any unrelated concurrent http.request/https.request is skipped as a false positive. This causes telemetry loss under normal concurrency.
Use a per-async-context guard (e.g., AsyncLocalStorage) so only HTTP calls causally triggered by the current fetch are suppressed.
Also applies to: 287-290, 380-381, 408-411
🤖 Prompt for 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.
In `@src/core/interceptor.ts` around lines 51 - 52, The global boolean guard
state.inFetchWrapper is causing unrelated concurrent http.request/https.request
calls to be suppressed; replace this global re-entrancy flag with a
per-async-context guard using Node's AsyncLocalStorage so only requests causally
triggered by the current fetch are skipped. Specifically, introduce an
AsyncLocalStorage store (e.g., fetchContext) and set a value (like {inFetch:
true}) around the wrapper that replaces originalFetch, read that store in the
interceptors that currently check state.inFetchWrapper (references:
state.inFetchWrapper, originalFetch, and the fetch wrapper code paths), and
update the checks at the locations noted (the fetch wrapper enter/exit and the
http/https request interceptors) to consult fetchContext.getStore()?.inFetch
instead of the global flag; ensure the store is correctly propagated across
async boundaries and cleaned up on completion.
handle.flush() was silently swallowing errors despite its documented "never rejects, errors route through onError" contract; dispose() had the same swallow with a comment falsely claiming flushAndSend routes errors itself. Extract a reportFlushError helper and use it from all five flush sites (periodic, wouldOverflow, maxBatchSize, dispose, flush). Also restore https bindings in the conflict-state afterEach cleanup so a future test that wraps https.* doesn't pollute later runs. Co-Authored-By: CodeRabbit <noreply@coderabbit.ai>
- README: add projectId to the local-mode-unavailability sample so it satisfies validateConfig when copy-pasted with apiKey set. - roadmap: replace the stale "two plans, two PRs" Wave 5 PR-shape note with the actual bundled-PR outcome. - spec: resolve the uninstall pseudocode contradiction between state.installed=false and the conflict contract requiring installed=true to gate re-install. Split clearly into conflict path (preserves installed + originals + onError) and clean path (full reset). Co-Authored-By: CodeRabbit <noreply@coderabbit.ai>
Contributor
Author
CodeRabbit triagePushed two follow-up commits (38d45b0 + 254b5b6) addressing 5 of 7 findings. Applied:
Deferred:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #11.
Closes #19.
globalThis[Symbol.for("@recost-dev/node:interceptor-state")], so a second copy of the package loaded into the same realm coordinates through the same state object. A secondinstall()firesRecostInterceptorAlreadyInstalledErrorviaonErrorand is a no-op.uninstall()now checks each binding's current value against the patched function we stored at install time. Mismatches fireRecostInterceptorPatchOverwrittenErrorwith the list of skipped bindings, leave the third-party wrapper in place, detach the callback, and refuse re-install. Recovery requires a process restart.init()themselves.dispose()is sync; Nodedispose()is async — shutdown semantics differ #19 dispose parity:RecostHandlegrows aflush(): Promise<void>method as the Node parallel of Python'sflush_blocking(). Idempotent afterdispose(). JS has no thread-blocking primitive, so the awaited promise is the contract.Cross-SDK: a corresponding Python tracking issue will be filed in
recost-dev/middleware-pythononce this PR is reviewed.Tests
handle.flush()).Test plan
🤖 Generated with Claude Code