refactor: introduce ServerContext and createServer#130
Merged
wpak-ai merged 5 commits intoJun 4, 2026
Conversation
|
Warning Review limit reached
More reviews will be available in 38 minutes and 27 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughIntroduces a ServerContext instance API (Phase 1) that encapsulates runtime state (client, config, URL registry, suggestion flow, namespaces cache), provides process-default helpers for backward compatibility, threads an optional context into core tools, adds tests, updates setup/export wiring, and documents migration steps. ChangesServerContext instance API and backward compatibility
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
added 2 commits
June 4, 2026 03:11
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/server/tools/query-tool.ts (1)
57-72:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftFix incorrect root-cause claim:
ServerContext.requireSuggested()only returns{ ok: true }whendisableSuggestFlowis enabled (otherwise it returns{ ok: false }when no flow state exists), so the “no suggest-flow state” case should hitFLOW_GATEbeforeclient.query().
src/core/server/server-context.tsshowsrequireSuggested()returnsok: trueonly underthis.getConfig().disableSuggestFlow; otherwise it returnsok: falsewhenthis.suggestionFlow.get(key)is missing, so reachingqueryOutcome.resultsatsrc/core/server/tools/query-tool.tsline ~72 implies either the injected context is configured withdisableSuggestFlowor the test/mock forctx.requireSuggesteddoesn’t actually return the expected{ ok: false }path for that case.🤖 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/server/tools/query-tool.ts` around lines 57 - 72, The failing claim is due to tests/mocks or injected ServerContext not exercising the `{ ok: false }` path of ServerContext.requireSuggested, so update the test or mock for requireSuggested (or the injected ctx) to return `{ ok: false, message: ... }` when suggestion flow state is absent, or configure the injected context's getConfig().disableSuggestFlow appropriately; this will ensure flowCheck (from requireSuggested) evaluates to false and the code path returns flowGateToolError/FLOW_GATE before calling client.query (symbols to check: ServerContext.requireSuggested, flowCheck, flowGateToolError, client.query, getPineconeClient).
🧹 Nitpick comments (2)
src/core/server/server-context.ts (1)
230-243: 💤 Low valueCache returns the stored array by reference.
Both the cache-hit (Line 232) and cache-miss paths hand back the same
dataarray stored innamespacesCache. A caller mutating the returned array mutates the cached entry for subsequentcache_hitreads. Consider returning a shallow copy if downstream consumers may mutate the result.🤖 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/server/server-context.ts` around lines 230 - 243, The cached array is being returned by reference so callers can mutate the cache; update the method to always return a shallow copy of the namespaces array while keeping the original in this.namespacesCache. Specifically, when returning on a cache hit (this.namespacesCache.data) and after fetching on a cache miss (data from getClient().listNamespacesWithMetadata()), return a copy (e.g., data.slice() or [...data]) instead of the stored array; keep storing the original data in this.namespacesCache to preserve caching semantics (references: this.namespacesCache, getClient().listNamespacesWithMetadata(), getConfig().cacheTtlMs).docs/deprecation-policy.md (1)
69-75: 💤 Low valueConsider adjusting verb tense for consistency.
Line 69 states Phase 1 "is available," but lines 71-75 use future tense ("That work will:"). Since Phase 1 has been implemented in this PR, consider either:
- Keeping future tense if this describes the full multi-phase plan (not just Phase 1), or
- Using present/past tense to describe what Phase 1 has already done
Current wording slightly obscures whether "that work" refers to Phase 1 (already done) or future phases (planned).
📝 Optional clarification
If describing what Phase 1 has already accomplished:
-That work will: +Phase 1 has: -- Add new instance APIs without removing legacy getters in the same release. -- Document legacy getters under `### Deprecated` with a named removal target per this policy. -- Link migration steps from [MIGRATION.md](./MIGRATION.md) to this document. +- Added new instance APIs without removing legacy getters. +- Maintained legacy getters for backward compatibility (deprecation planned for future releases). +- Linked migration steps from [MIGRATION.md](./MIGRATION.md).Or if describing the overall multi-phase plan:
-That work will: +The full refactor will:🤖 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/deprecation-policy.md` around lines 69 - 75, The wording mixes tenses: line mentioning Phase 1 "is available" then "That work will:" uses future tense and may confuse whether Phase 1 (ServerContext / createServer(config)) is completed; update the phrasing for consistency by making the second sentence reflect the completed Phase 1 (use present/past tense) or explicitly state this is a multi-phase plan (keep future tense) — adjust the paragraph around **ServerContext** / **createServer(config)** and the bullet list so the tense matches (e.g., "That work adds new instance APIs..." or "This plan will: ...") and ensure the link text to MIGRATION.md remains intact.
🤖 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/MIGRATION.md`:
- Around line 28-36: The comment claiming setupAllianceServer(config) "passes
ctx into migrated tools" is misleading; either make setupAllianceServer accept
the explicit ctx (change the example to setupAllianceServer(config, ctx) and
update the function signature/implementation of setupAllianceServer to accept
and use the provided ServerContext) or, if setupAllianceServer relies on the
process-default installed by createServer, update the comment to say that
createServer installs the process-default context which setupAllianceServer will
read implicitly; update the example and docs text accordingly and ensure
references to createServer, ctx, and setupAllianceServer are consistent.
In `@src/core/server/server-context.ts`:
- Around line 185-195: The branch is checking
this.configValue?.disableSuggestFlow directly which skips operator-set values
when config is lazy; change the check to call getConfig() (which triggers
resolveConfig({})) and read disableSuggestFlow from the returned config instead.
Update the branch in the method that returns the suggest-flow object to use
const cfg = await this.getConfig(); if (cfg.disableSuggestFlow) { ... }
(referencing getConfig, resolveConfig, and disableSuggestFlow) so the
lazy/default config path honors the operator-set flag.
---
Outside diff comments:
In `@src/core/server/tools/query-tool.ts`:
- Around line 57-72: The failing claim is due to tests/mocks or injected
ServerContext not exercising the `{ ok: false }` path of
ServerContext.requireSuggested, so update the test or mock for requireSuggested
(or the injected ctx) to return `{ ok: false, message: ... }` when suggestion
flow state is absent, or configure the injected context's
getConfig().disableSuggestFlow appropriately; this will ensure flowCheck (from
requireSuggested) evaluates to false and the code path returns
flowGateToolError/FLOW_GATE before calling client.query (symbols to check:
ServerContext.requireSuggested, flowCheck, flowGateToolError, client.query,
getPineconeClient).
---
Nitpick comments:
In `@docs/deprecation-policy.md`:
- Around line 69-75: The wording mixes tenses: line mentioning Phase 1 "is
available" then "That work will:" uses future tense and may confuse whether
Phase 1 (ServerContext / createServer(config)) is completed; update the phrasing
for consistency by making the second sentence reflect the completed Phase 1 (use
present/past tense) or explicitly state this is a multi-phase plan (keep future
tense) — adjust the paragraph around **ServerContext** /
**createServer(config)** and the bullet list so the tense matches (e.g., "That
work adds new instance APIs..." or "This plan will: ...") and ensure the link
text to MIGRATION.md remains intact.
In `@src/core/server/server-context.ts`:
- Around line 230-243: The cached array is being returned by reference so
callers can mutate the cache; update the method to always return a shallow copy
of the namespaces array while keeping the original in this.namespacesCache.
Specifically, when returning on a cache hit (this.namespacesCache.data) and
after fetching on a cache miss (data from
getClient().listNamespacesWithMetadata()), return a copy (e.g., data.slice() or
[...data]) instead of the stored array; keep storing the original data in
this.namespacesCache to preserve caching semantics (references:
this.namespacesCache, getClient().listNamespacesWithMetadata(),
getConfig().cacheTtlMs).
🪄 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: 7f5e53fb-8ffe-4576-bb06-cd7d5aae6161
📒 Files selected for processing (19)
docs/MIGRATION.mddocs/deprecation-policy.mdexamples/alliance/library-embedding-demo.tssrc/core/index.tssrc/core/server/client-context.tssrc/core/server/config-context.tssrc/core/server/namespaces-cache.tssrc/core/server/server-context.test.tssrc/core/server/server-context.tssrc/core/server/suggestion-flow.tssrc/core/server/tools/count-tool.context.test.tssrc/core/server/tools/count-tool.tssrc/core/server/tools/list-namespaces-tool.context.test.tssrc/core/server/tools/list-namespaces-tool.tssrc/core/server/tools/query-tool.context.test.tssrc/core/server/tools/query-tool.tssrc/core/server/tools/test-helpers.tssrc/core/server/url-registry.tssrc/core/setup.ts
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #130 +/- ##
=======================================
Coverage ? 84.12%
=======================================
Files ? 39
Lines ? 1405
Branches ? 464
=======================================
Hits ? 1182
Misses ? 221
Partials ? 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Collaborator
Author
|
@coderabbitai full review again |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
examples/alliance/library-embedding-demo.ts (1)
6-6: 💤 Low valueConsider adding a reference to migration docs or demonstrating the new pattern.
The comment now documents both the legacy
setPineconeClientpattern and the newcreateServer(config)+ctx.setClient(...)pattern, but the example code only demonstrates the legacy approach. Users interested in the new pattern would benefit from either:
- A pointer to MIGRATION.md for the new pattern example
- A commented-out code block showing the new pattern
- Adding
createServerto the imports on line 15 (even if unused) to show where it comes from📚 Optional enhancement: Add migration doc reference
/** * Library embedding: build the MCP server from a Node script (not the CLI). * * Pattern (mirrors `src/index.ts`): * 1. `resolveAllianceConfig({ apiKey, indexName, ... })` — Alliance index/rerank defaults when unset. - * 2. `new PineconeClient({ ... })` + `setPineconeClient(client)` (legacy), or `createServer(config)` + `ctx.setClient(...)`. + * 2. `new PineconeClient({ ... })` + `setPineconeClient(client)` (legacy), or `createServer(config)` + `ctx.setClient(...)` (see MIGRATION.md). * 3. `await setupAllianceServer(config)` then `server.connect(transport)`.🤖 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 `@examples/alliance/library-embedding-demo.ts` at line 6, The example mentions both the legacy setPineconeClient pattern and the new createServer + ctx.setClient pattern but only shows the legacy usage; update the example by either (a) adding a short commented-out snippet demonstrating createServer(...) and ctx.setClient(...) using the same variable names, (b) adding createServer to the imports to show its origin, or (c) adding a one-line pointer to MIGRATION.md that shows the new pattern; reference the symbols setPineconeClient, createServer, and ctx.setClient so readers can find and understand the new flow.
🤖 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/MIGRATION.md`:
- Around line 35-49: The example is missing the PineconeClient import causing a
runtime error; update the snippet to import PineconeClient and use it with the
shown flow (where resolveAllianceConfig, createServer, ctx.setClient, and
setupAllianceServer are used). Add an import for PineconeClient at the top of
the example and ensure the new PineconeClient({...}) call matches the config
fields used by resolveAllianceConfig so the instance creation and
ctx.setClient(...) succeed.
In `@src/core/server/server-context.ts`:
- Around line 74-76: setConfig currently only swaps configValue which leaves
derived state (lazy-built client, namespacesCache, etc.) stale; change setConfig
to also reset/clear any cached/derived members created from the previous
config—e.g., invalidate the Pinecone client instance, clear namespacesCache, and
any other lazy flags so subsequent access rebuilds with the new config; ensure
setPendingServerConfig continues to call setConfig and that callers relying on
lazy builders (client creation, namespace population) will reconstruct using the
updated config.
In `@src/core/server/tools/test-helpers.ts`:
- Around line 117-123: The test helper resolveTestConfig currently lets
cacheTtlMs come from resolveConfig(), causing nondeterministic TTL in tests;
update resolveTestConfig to explicitly set a stable cacheTtlMs (e.g. a large
non-zero fixed millisecond value) in the returned object so tests using
ServerContext.requireSuggested() and ServerContext.getNamespacesWithCache() have
deterministic caching behavior; modify the call in resolveTestConfig that
invokes resolveConfig(...) to include cacheTtlMs: <fixed value> alongside
apiKey/indexName/disableSuggestFlow so the test config always pins TTL.
---
Nitpick comments:
In `@examples/alliance/library-embedding-demo.ts`:
- Line 6: The example mentions both the legacy setPineconeClient pattern and the
new createServer + ctx.setClient pattern but only shows the legacy usage; update
the example by either (a) adding a short commented-out snippet demonstrating
createServer(...) and ctx.setClient(...) using the same variable names, (b)
adding createServer to the imports to show its origin, or (c) adding a one-line
pointer to MIGRATION.md that shows the new pattern; reference the symbols
setPineconeClient, createServer, and ctx.setClient so readers can find and
understand the new flow.
🪄 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: e574bab8-7301-4e4d-9386-e6827022b94d
📒 Files selected for processing (19)
docs/MIGRATION.mddocs/deprecation-policy.mdexamples/alliance/library-embedding-demo.tssrc/core/index.tssrc/core/server/client-context.tssrc/core/server/config-context.tssrc/core/server/namespaces-cache.tssrc/core/server/server-context.test.tssrc/core/server/server-context.tssrc/core/server/suggestion-flow.tssrc/core/server/tools/count-tool.context.test.tssrc/core/server/tools/count-tool.tssrc/core/server/tools/list-namespaces-tool.context.test.tssrc/core/server/tools/list-namespaces-tool.tssrc/core/server/tools/query-tool.context.test.tssrc/core/server/tools/query-tool.tssrc/core/server/tools/test-helpers.tssrc/core/server/url-registry.tssrc/core/setup.ts
AuraMindNest
approved these changes
Jun 4, 2026
wpak-ai
approved these changes
Jun 4, 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
First step of the singleton-to-instance refactor: introduce
ServerContextandcreateServer(config)to hold client, config, URL registry, suggest-flow gate, and namespaces cache in one object.Legacy module helpers (
getPineconeClient,requireSuggested, etc.) still work — they delegate to a process-default context. No breaking changes.Changes
ServerContext+createServer()/getDefaultServerContext()/teardownDefaultServerContext()query,count,list_namespacessetupCoreServerto create/pass context; preserve pre-set client viasetPineconeClient()*.context.test.ts); legacy tests unchangeddocs/MIGRATION.mdTest plan
npm run ciserver-context.test.ts+*.context.test.tspass with injected contextRelated
Summary by CodeRabbit
New Features
Documentation