Core vs Alliance MCP instructions and suggest-flow gate#125
Merged
wpak-ai merged 3 commits intoJun 3, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR splits server instruction text into core vs Alliance variants, makes suggest-flow defaults resolver-specific (core: disabled by default; Alliance: enabled by default), passes Alliance instructions from setupAllianceServer, and updates tests, examples, and docs to reflect the contract. ChangesCore/Alliance Instruction Split and Suggest-Flow Defaults
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #125 +/- ##
=======================================
Coverage ? 82.48%
=======================================
Files ? 38
Lines ? 1313
Branches ? 440
=======================================
Hits ? 1083
Misses ? 228
Partials ? 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/core/server/tools/count-tool.test.ts (1)
108-129: ⚡ Quick winAssert the gate helper is bypassed, not just that the call succeeds.
This currently proves the request can succeed, but not that
disableSuggestFlow=trueskippedrequireSuggested. Spying onrequireSuggestedhere would make the contract explicit and avoid false passes if shared flow state is ever present.Suggested tightening
it('does not return FLOW_GATE without prior suggest_query_params', async () => { - setServerConfig(resolveConfig({ apiKey: 'sk-test', indexName: 'my-index' })); - expect(resolveConfig({ apiKey: 'sk-test', indexName: 'my-index' }).disableSuggestFlow).toBe( - true - ); + const cfg = resolveConfig({ apiKey: 'sk-test', indexName: 'my-index' }); + const requireSuggestedSpy = vi.spyOn(suggestionFlow, 'requireSuggested'); + setServerConfig(cfg); + expect(cfg.disableSuggestFlow).toBe(true); mockedGetClient.mockReturnValue({ count: vi.fn().mockResolvedValue({ count: 2, truncated: false }), } as never); @@ const envelope = raw as { isError?: boolean }; expect(envelope.isError).not.toBe(true); const body = parseToolJson(raw); expect(body['status']).toBe('success'); + expect(requireSuggestedSpy).not.toHaveBeenCalled(); expect(mockedGetClient().count).toHaveBeenCalled(); });🤖 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/count-tool.test.ts` around lines 108 - 129, Test currently only asserts success but not that the suggest-gate was bypassed; add a spy on the requireSuggested helper and assert it was not called when disableSuggestFlow is true. In the test that calls registerCountTool and invokes the 'count' handler (the block that sets resolveConfig(...).disableSuggestFlow), create a spy for the requireSuggested function used by the count tool (spyOn or vi.spyOn the module that exports requireSuggested) before invoking the handler, then assert that requireSuggested was not called after the request completes while keeping the existing assertions (mockedGetClient, parseToolJson, status checks) intact. Ensure you restore or clear the spy after the test to avoid cross-test pollution.src/alliance/config.ts (1)
38-41: ⚡ Quick winKeep
disableSuggestFlowresolution in one place.This computes the flag twice and then overwrites the core result. Feeding the Alliance default into
resolveConfigkeeps the normalization path single-sourced and avoids drift if core adds more logic around this field later.Suggested simplification
- const cfg = resolveConfig({ ...overrides, indexName, rerankModel }, env); const disableSuggestFlow = overrides.disableSuggestFlow ?? asBool(env['PINECONE_DISABLE_SUGGEST_FLOW'], false); - return { ...cfg, disableSuggestFlow }; + return resolveConfig({ ...overrides, indexName, rerankModel, disableSuggestFlow }, env);🤖 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/config.ts` around lines 38 - 41, The code computes disableSuggestFlow twice and overwrites the resolved config; instead compute the final boolean once (using overrides.disableSuggestFlow ?? asBool(env['PINECONE_DISABLE_SUGGEST_FLOW'], false)), include that value in the object passed to resolveConfig (instead of passing rerankModel only), and return the result directly (remove the post-resolve overwrite of cfg.disableSuggestFlow) so resolveConfig is the single source of truth for that field; update references to disableSuggestFlow, resolveConfig, and cfg accordingly.
🤖 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 `@src/constants.ts`:
- Line 67: The comment's gate wording is inaccurate: update the text that
references PINECONE_DISABLE_SUGGEST_FLOW (near the sentence describing the
manual flow: "list_namespaces -> suggest_query_params -> query or count") to
explicitly require the variable to be true (e.g., "unless
PINECONE_DISABLE_SUGGEST_FLOW=true" or "unless PINECONE_DISABLE_SUGGEST_FLOW is
set to true") so the described gate behavior matches the actual boolean
semantics of PINECONE_DISABLE_SUGGEST_FLOW and avoids implying presence-only
semantics.
---
Nitpick comments:
In `@src/alliance/config.ts`:
- Around line 38-41: The code computes disableSuggestFlow twice and overwrites
the resolved config; instead compute the final boolean once (using
overrides.disableSuggestFlow ?? asBool(env['PINECONE_DISABLE_SUGGEST_FLOW'],
false)), include that value in the object passed to resolveConfig (instead of
passing rerankModel only), and return the result directly (remove the
post-resolve overwrite of cfg.disableSuggestFlow) so resolveConfig is the single
source of truth for that field; update references to disableSuggestFlow,
resolveConfig, and cfg accordingly.
In `@src/core/server/tools/count-tool.test.ts`:
- Around line 108-129: Test currently only asserts success but not that the
suggest-gate was bypassed; add a spy on the requireSuggested helper and assert
it was not called when disableSuggestFlow is true. In the test that calls
registerCountTool and invokes the 'count' handler (the block that sets
resolveConfig(...).disableSuggestFlow), create a spy for the requireSuggested
function used by the count tool (spyOn or vi.spyOn the module that exports
requireSuggested) before invoking the handler, then assert that requireSuggested
was not called after the request completes while keeping the existing assertions
(mockedGetClient, parseToolJson, status checks) intact. Ensure you restore or
clear the spy after the test to avoid cross-test pollution.
🪄 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: 82e0a7e6-ed8e-449b-a637-13f8f678efef
📒 Files selected for processing (19)
.env.exampleCHANGELOG.mdREADME.mddocs/CONFIGURATION.mddocs/FAQ.mddocs/TOOLS.mdexamples/quickstart/.env.exampleexamples/quickstart/README.mdexamples/quickstart/mcp-demo.tssrc/alliance/config.test.tssrc/alliance/config.tssrc/alliance/setup.tssrc/constants.test.tssrc/constants.tssrc/core/config.test.tssrc/core/config.tssrc/core/server.test.tssrc/core/server/tools/count-tool.test.tssrc/core/setup.ts
Collaborator
Author
|
@coderabbitai full review again |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/setup.ts (1)
32-47: ⚡ Quick winFunction JSDoc now documents the wrong declaration.
The descriptive block at lines 32-38 (explaining
setupCoreServerbehavior, Alliance tool exclusion, etc.) now immediately precedesexport type SetupCoreServerOptions, so editors/IntelliSense attach it to the type rather than tosetupCoreServer. Move the function doc directly above the function.♻️ Proposed fix
-/** - * Create and configure the MCP server with generic (core) tools only. - * - * Does not register Alliance-specific tools (`suggest_query_params`, `guided_query`) - * or built-in Boost/Slack URL generators. Use {`@link` setupAllianceServer} from - * `@will-cppa/pinecone-read-only-mcp/alliance` for the full tool surface. - */ export type SetupCoreServerOptions = { /** MCP server instructions; defaults to {`@link` CORE_SERVER_INSTRUCTIONS}. */ instructions?: string; }; +/** + * Create and configure the MCP server with generic (core) tools only. + * + * Does not register Alliance-specific tools (`suggest_query_params`, `guided_query`) + * or built-in Boost/Slack URL generators. Use {`@link` setupAllianceServer} from + * `@will-cppa/pinecone-read-only-mcp/alliance` for the full tool surface. + */ export async function setupCoreServer( config?: ServerConfig, options?: SetupCoreServerOptions ): Promise<McpServer> {🤖 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 32 - 47, The JSDoc describing setupCoreServer is currently attached to the type SetupCoreServerOptions; move that descriptive comment block so it immediately precedes the function declaration export async function setupCoreServer(...) and ensure the type SetupCoreServerOptions has its own brief JSDoc (or none) so editors/IntelliSense show the correct documentation for setupCoreServer; reference symbols: SetupCoreServerOptions and setupCoreServer.
🤖 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.
Nitpick comments:
In `@src/core/setup.ts`:
- Around line 32-47: The JSDoc describing setupCoreServer is currently attached
to the type SetupCoreServerOptions; move that descriptive comment block so it
immediately precedes the function declaration export async function
setupCoreServer(...) and ensure the type SetupCoreServerOptions has its own
brief JSDoc (or none) so editors/IntelliSense show the correct documentation for
setupCoreServer; reference symbols: SetupCoreServerOptions and setupCoreServer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 03b27d69-7a99-4a65-9d25-a497bcc0c388
📒 Files selected for processing (19)
.env.exampleCHANGELOG.mdREADME.mddocs/CONFIGURATION.mddocs/FAQ.mddocs/TOOLS.mdexamples/quickstart/.env.exampleexamples/quickstart/README.mdexamples/quickstart/mcp-demo.tssrc/alliance/config.test.tssrc/alliance/config.tssrc/alliance/setup.tssrc/constants.test.tssrc/constants.tssrc/core/config.test.tssrc/core/config.tssrc/core/server.test.tssrc/core/server/tools/count-tool.test.tssrc/core/setup.ts
AuraMindNest
approved these changes
Jun 3, 2026
wpak-ai
approved these changes
Jun 3, 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
Completes the runtime contract boundary between generic
setupCoreServerembedders and the full Alliance MCP surface. Core servers no longer advertise Alliance-only tools in MCP metadata, and core embedders can callquery,count, andquery_documentswithoutsuggest_query_paramsorPINECONE_DISABLE_SUGGEST_FLOW=true.Problem
SERVER_INSTRUCTIONS— BothsetupCoreServerandsetupAllianceServertold clients to useguided_queryand mandatorysuggest_query_params, but only Alliance registers those tools.query/count/query_documentscallrequireSuggested(), yet only Alliance providessuggest_query_params. Quickstart had to setPINECONE_DISABLE_SUGGEST_FLOW=true.Solution (Option A)
MCP instructions
CORE_SERVER_INSTRUCTIONS— Quickstart for seven core tools only; noguided_query/suggest_query_params.ALLIANCE_SERVER_INSTRUCTIONS— Core text + appendix (guided query, suggest-flow, Alliance index/rerank defaults).setupCoreServer(config?, options?)— Optionalinstructions(defaults to core).setupAllianceServer— PassesALLIANCE_SERVER_INSTRUCTIONS.SERVER_INSTRUCTIONS— Deprecated alias →ALLIANCE_SERVER_INSTRUCTIONS.Suggest-flow defaults
disableSuggestFlowdefaultresolveConfig(core)trueresolveAllianceConfig/ CLIfalseAlliance re-applies its default after
resolveConfigso core’s new default does not leak into the CLI.requireSuggested/markSuggestedremain in core; tool handlers unchanged.Breaking changes (core embedders only)
instructionsforsetupCoreServerno longer mention Alliance tools.resolveConfigdefaultsdisableSuggestFlowtotrue(gate off). To enforce the gate on core, setPINECONE_DISABLE_SUGGEST_FLOW=falseordisableSuggestFlow: false.Unchanged: Alliance CLI,
resolveAllianceConfig, andsetupAllianceServerbehavior (gate on by default; full instructions).Files changed
src/constants.ts,src/core/setup.ts,src/alliance/setup.tssrc/core/config.ts,src/alliance/config.ts(+ exportedasBool)src/constants.test.ts,src/core/config.test.ts,src/alliance/config.test.ts,src/core/server/tools/count-tool.test.ts,src/core/server.test.tsexamples/quickstart/mcp-demo.ts,.env.example,README.mddocs/TOOLS.md,docs/CONFIGURATION.md,docs/FAQ.md,README.md,.env.example,CHANGELOG.mdTest plan
npm run buildnpm testsrc/constants.test.ts— core instructions exclude Alliance tool names; Alliance includes themsrc/core/config.test.ts—resolveConfig→disableSuggestFlow: true; envfalse→ gate onsrc/alliance/config.test.ts—resolveAllianceConfig→disableSuggestFlow: falsecount-tool.test.ts— count succeeds without priorsuggest_query_paramsunder core default confignpx tsx examples/quickstart/mcp-demo.ts(noPINECONE_DISABLE_SUGGEST_FLOWin.env)guided_querystill works with suggest gate onRelated
Summary by CodeRabbit
Breaking Changes
Documentation
Examples
Tests