feat(setup): explicit context API for multi-instance ServerContext embedding - #150
Merged
wpak-ai merged 5 commits intoJun 11, 2026
Merged
Conversation
…tracking and { config?, context?, instructions? } options so multiple isolated servers can coexist in one process while preserving legacy positional setup.
|
Warning Review limit reached
More reviews will be available in 51 minutes and 10 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThis PR makes ServerContext instance-scoped: core/alliance setup accept optional context/options, per-context tool-registration and injected-client guards were added, docs/examples/CLI updated to an instance-first embedding flow, and tests validate multi-instance isolation and lifecycle. ChangesMulti-instance ServerContext and Instance-first Setup API
Sequence Diagram(s)sequenceDiagram
participant Embedder
participant createServer
participant ctx as ServerContext
participant setupCoreServer
participant setupAllianceServer
participant MCP as MCP Router
Embedder->>createServer: createServer(config)
createServer-->>ctx: new ServerContext
Embedder->>ctx: ctx.setClient(pineconeClient)
Embedder->>setupCoreServer: setupCoreServer({ config, context: ctx })
setupCoreServer->>ctx: ctx.assertCanRegisterTools()
setupCoreServer->>MCP: register core tools & handlers
setupCoreServer->>ctx: ctx.markToolsRegistered()
setupCoreServer-->>Embedder: ServerHandle
Embedder->>setupAllianceServer: setupAllianceServer({ config, context: ctx })
setupAllianceServer->>setupCoreServer: delegate (reuse ctx)
setupAllianceServer->>MCP: register URL generators & Alliance tools
setupAllianceServer-->>Embedder: ServerHandle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/MIGRATION.md (1)
43-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMigration “recommended” snippets use a risky call shape (Lines 43-55, 63-66).
These examples pass both
configandcontextafterctx.setClient(...), which can clear injected client state. Please migrate examples to context-only setup calls when the context is already configured.Also applies to: 63-66
🤖 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/MIGRATION.md` around lines 43 - 55, The migration example currently calls ctx.setClient(...) then calls setupAllianceServer with both config and context, which can overwrite or clear the injected client state; change the call to only pass the context after configuring the ctx (i.e., call setupAllianceServer({ context: ctx }) instead of setupAllianceServer({ config, context: ctx })), and update the other snippet referenced (lines 63-66) similarly; locate usages of createServer, ctx.setClient, and setupAllianceServer to make this change.
🧹 Nitpick comments (2)
src/alliance/setup.ts (1)
33-47: ⚖️ Poor tradeoffNormalization silently accepts invalid input objects.
Same issue as
src/core/setup.ts: ifconfigOrOptionsis an object matching neitherServerConfignorSetupAllianceServerOptions, the fallback on line 46 returnslegacyOptions ?? {}, silently discarding invalid input.🛡️ Suggested defensive check
function normalizeSetupAllianceArgs( configOrOptions?: ServerConfig | SetupAllianceServerOptions, legacyOptions?: Pick<SetupAllianceServerOptions, 'instructions'> ): SetupAllianceServerOptions { if (configOrOptions === undefined) { return legacyOptions ?? {}; } if (isServerConfig(configOrOptions)) { return { config: configOrOptions, ...legacyOptions }; } if (isSetupAllianceServerOptions(configOrOptions)) { return { ...configOrOptions, ...legacyOptions }; } + throw new TypeError('configOrOptions must be a ServerConfig or SetupAllianceServerOptions'); - return legacyOptions ?? {}; }🤖 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/alliance/setup.ts` around lines 33 - 47, The normalizeSetupAllianceArgs function currently swallows unknown object shapes and returns legacyOptions, so add a defensive check: after the existing isServerConfig and isSetupAllianceServerOptions branches, detect when configOrOptions is a non-null object that matches neither predicate and throw a clear TypeError (or assert) indicating invalid input to normalizeSetupAllianceArgs and include the offending value; use the existing type guard functions isServerConfig and isSetupAllianceServerOptions to decide and reference them in the error message so callers get immediate feedback instead of silently losing data.src/core/setup.ts (1)
59-73: ⚖️ Poor tradeoffNormalization silently accepts invalid input objects.
If
configOrOptionsis an object that matches neitherServerConfignorSetupCoreServerOptions(e.g.,{ foo: 'bar' }), the fallback on line 72 returnslegacyOptions ?? {}, silently discarding the invalid input without error. This loses type safety and could hide bugs when callers pass malformed objects.🛡️ Suggested defensive check
function normalizeSetupCoreServerArgs( configOrOptions?: ServerConfig | SetupCoreServerOptions, legacyOptions?: Pick<SetupCoreServerOptions, 'instructions'> ): SetupCoreServerOptions { if (configOrOptions === undefined) { return legacyOptions ?? {}; } if (isServerConfig(configOrOptions)) { return { config: configOrOptions, ...legacyOptions }; } if (isSetupCoreServerOptions(configOrOptions)) { return { ...configOrOptions, ...legacyOptions }; } + throw new TypeError('configOrOptions must be a ServerConfig or SetupCoreServerOptions'); - return legacyOptions ?? {}; }🤖 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/setup.ts` around lines 59 - 73, The normalizeSetupCoreServerArgs function currently swallows invalid objects (e.g., {foo:'bar'}) by falling back to legacyOptions; change it to detect when configOrOptions is an object but neither isServerConfig(configOrOptions) nor isSetupCoreServerOptions(configOrOptions) and throw a clear TypeError (or similar) instead of returning legacyOptions; update the function to perform this defensive check after the two predicate checks and include the offending value (or its type) in the error message so callers and tests can detect malformed inputs.
🤖 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/CONFIGURATION.md`:
- Around line 66-69: The docs currently show calling setupAllianceServer({
config, context: ctx }) after creating a ctx and injecting the client; update
the instructions to avoid passing both config and context together once the
context has been fully configured. Change the examples to build ServerConfig via
resolveConfig(...) or resolveAllianceConfig(...), call const ctx =
createServer(config) and ctx.setClient(...), then call setupAllianceServer({
context: ctx }) (and likewise use setupCoreServer({ context: ctx }) for generic
tools) so only the already-configured context is passed; reference ServerConfig,
resolveConfig, resolveAllianceConfig, createServer, ctx.setClient,
setupAllianceServer, and setupCoreServer when making the edit.
In `@examples/alliance/library-embedding-demo.ts`:
- Line 44: The call to setupAllianceServer is passing both config and a
pre-configured ctx which can reset context state and drop injected clients;
update the invocation of setupAllianceServer (the example using
setupAllianceServer({ config, context: ctx })) to pass only the pre-configured
context (setupAllianceServer({ context: ctx })) after you create ctx and call
ctx.setClient(...) so the already-initialized context isn’t overwritten.
- Line 44: The call to setupAllianceServer passes both config and context which
can reset context state and drop injected clients; update the invocation of
setupAllianceServer so it only passes the context (ctx) and remove the config
argument (or ensure config does not include a pre-configured context) — locate
the setupAllianceServer(...) call and change it to pass just the context
parameter, keeping ctx as the injected client carrier.
In `@README.md`:
- Around line 139-144: The README shows passing a config that already contains a
pre-configured context which can clear injected clients and break embedding
flows; update the example so you do not embed the created server/context into
the config—call resolveAllianceConfig to build the config, createServer to build
the context and call ctx.setClient as needed, but when calling
setupAllianceServer pass only context via the context parameter (i.e.,
setupAllianceServer({ context: ctx })) and do not pass the config object that
contains the pre-configured ctx; update both occurrences around the example
lines and line 176 accordingly.
In `@src/alliance/setup.ts`:
- Around line 66-72: The bug is that setupAllianceServer passes a resolved
default config into setupCoreServer even when the caller did not provide
opts.config, which causes resolveSetupContext/context.setConfig to overwrite and
clear an injected client; change the call site in setupAllianceServer so that
setupCoreServer only receives a config property when opts.config is explicitly
provided (i.e., forward opts.config as-is, not opts.config ??
resolveAllianceConfig({})), preserving resolvedCtx and avoiding calling
context.setConfig implicitly; locate the call to setupCoreServer and the
surrounding resolvedCtx/opts handling to implement this conditional forwarding.
In `@src/core/setup.ts`:
- Around line 77-79: When both opts.config and opts.context are passed into
setupCoreServer, calling opts.context.setConfig(opts.config) can silently clear
an injected client via invalidateConfigDerivedState()/this.client; add a
defensive runtime guard in src/core/setup.ts before calling
opts.context.setConfig: check the context for an existing client (e.g., call
opts.context.getClient() or inspect opts.context.client accessor) and if a
client is present, throw a clear error (or at minimum log/warn and refuse to
overwrite) instructing callers to omit config when reusing a pre-configured
context; implement this check around the current
opts.context.setConfig(opts.config) call and include the symbols opts.config,
opts.context, setConfig, invalidateConfigDerivedState, and this.client in the
error message to aid debugging.
---
Outside diff comments:
In `@docs/MIGRATION.md`:
- Around line 43-55: The migration example currently calls ctx.setClient(...)
then calls setupAllianceServer with both config and context, which can overwrite
or clear the injected client state; change the call to only pass the context
after configuring the ctx (i.e., call setupAllianceServer({ context: ctx })
instead of setupAllianceServer({ config, context: ctx })), and update the other
snippet referenced (lines 63-66) similarly; locate usages of createServer,
ctx.setClient, and setupAllianceServer to make this change.
---
Nitpick comments:
In `@src/alliance/setup.ts`:
- Around line 33-47: The normalizeSetupAllianceArgs function currently swallows
unknown object shapes and returns legacyOptions, so add a defensive check: after
the existing isServerConfig and isSetupAllianceServerOptions branches, detect
when configOrOptions is a non-null object that matches neither predicate and
throw a clear TypeError (or assert) indicating invalid input to
normalizeSetupAllianceArgs and include the offending value; use the existing
type guard functions isServerConfig and isSetupAllianceServerOptions to decide
and reference them in the error message so callers get immediate feedback
instead of silently losing data.
In `@src/core/setup.ts`:
- Around line 59-73: The normalizeSetupCoreServerArgs function currently
swallows invalid objects (e.g., {foo:'bar'}) by falling back to legacyOptions;
change it to detect when configOrOptions is an object but neither
isServerConfig(configOrOptions) nor isSetupCoreServerOptions(configOrOptions)
and throw a clear TypeError (or similar) instead of returning legacyOptions;
update the function to perform this defensive check after the two predicate
checks and include the offending value (or its type) in the error message so
callers and tests can detect malformed inputs.
🪄 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
Run ID: 7af0ccaa-0fb5-4dd9-912e-71ce57e2d21f
📒 Files selected for processing (11)
README.mddocs/CONFIGURATION.mddocs/MIGRATION.mdexamples/alliance/library-embedding-demo.tssrc/alliance/index.tssrc/alliance/setup.tssrc/core/index.tssrc/core/server/server-context.tssrc/core/setup-multi-instance.test.tssrc/core/setup.tssrc/index.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #150 +/- ##
=======================================
Coverage ? 86.73%
=======================================
Files ? 39
Lines ? 1598
Branches ? 552
=======================================
Hits ? 1386
Misses ? 210
Partials ? 2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
added 2 commits
June 12, 2026 00:34
AuraMindNest
approved these changes
Jun 11, 2026
wpak-ai
approved these changes
Jun 11, 2026
3 tasks
9 tasks
This was referenced Jul 9, 2026
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
3 participants
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
setup now accepts
{ config?, context?, instructions? }so multiple isolatedServerContextinstances can register tools in one process. The process-globalmcpServerInitializedguard is replaced with per-context registration tracking; legacy positionalsetupCoreServer(config)andteardownServer()remain supported.Changes
setupCoreServer/setupAllianceServer— options object + backward-compatible positional overloadsServerContext—toolsRegisteredguard (assertCanRegisterTools,markToolsRegistered); legacy double-setup still throws viapeekDefaultServerContext()contextat setup preservesctx.setClient(...)without pulling from the process defaultctxpassed to core setupcreateServer(config)→ctx.setClient(...)→setupAllianceServer({ config, context: ctx })library-embedding-demo.tssetup-multi-instance.test.ts— coexistence, URL/suggest-flow/cache isolation, injected client,await usingAcceptance criteria
setupCoreServer({ config?, context?, instructions? })setupAllianceServer({ config?, context?, instructions? })— same ctx for core + AllianceteardownServer()contextis providedteardownServer()valid for legacy single-server flows (documented)Test plan
npm test— 245 tests passnpm run build— passsetup-multi-instance.test.ts— multi-setup, isolation, injected client,await usingserver.test.ts/lifecycle.context.test.ts— legacy positional setup +teardownServerpathNotes for reviewers
contextstill usescreateServer+ client transfer fromsetPineconeClient; Alliance delegates that tosetupCoreServerwhencontextis omitted.configandcontextcallssetConfig, which clears an injected client — embedders should omitconfigwhen the context is already configured.Related Issue
Summary by CodeRabbit
Documentation
New Features
Tests