Remove Alliance defaults from core config#123
Conversation
|
Warning Review limit reached
More reviews will be available in 36 minutes and 36 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)
📝 WalkthroughWalkthroughCore resolveConfig now requires a trimmed non-empty Pinecone indexName and makes rerankModel optional. Alliance adds ALLIANCE_DEFAULT_INDEX_NAME / ALLIANCE_DEFAULT_RERANK_MODEL and delegates to core. CLI, exports, tests, docs, examples, and changelog updated to reflect the new contract. Changes Configuration Resolution and Defaulting
User Documentation and Examples
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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 #123 +/- ##
=======================================
Coverage ? 81.47%
=======================================
Files ? 38
Lines ? 1306
Branches ? 445
=======================================
Hits ? 1064
Misses ? 240
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: 4
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)
3-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify version authority to match the new Unreleased migration section.
This intro still says the [0.2.0] changelog section is authoritative, but this document now adds an Unreleased migration block. Please update the intro so readers know whether to follow
Unreleasedvs0.2.0guidance for this page revision.🤖 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 3 - 5, Update the opening paragraph in MIGRATION.md to clarify which changelog section is authoritative for this revision: state that if an "Unreleased" migration block exists in CHANGELOG.md it takes precedence and should be followed, otherwise fall back to the [0.2.0] section; reference the "Unreleased" heading and the "[0.2.0]" changelog entry by name so readers know which guidance to apply.
🤖 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 `@examples/alliance/library-embedding-demo.ts`:
- Line 5: The docstring references resolveAllianceConfig(...) but the example
code still calls resolveConfig(...) and requires PINECONE_INDEX_NAME; update the
example so code and comment match by either (A) replacing resolveConfig(...)
with resolveAllianceConfig(...) and removing the hard dependency on
PINECONE_INDEX_NAME, or (B) changing the docstring to reference
resolveConfig(...) and keep the existing Pinecone requirement; ensure the used
symbol (resolveAllianceConfig or resolveConfig) is consistent in both the
comment and the code and that any environment variable requirements
(PINECONE_INDEX_NAME) reflect the chosen resolver's contract.
In `@src/alliance/config.ts`:
- Around line 29-33: The bug is that trimOptional is applied after the
nullish-coalescing, so blank/whitespace overrides (e.g., overrides.indexName ===
' ') short-circuit env and go straight to ALLIANCE defaults; fix by trimming
each source before coalescing: call trimOptional on overrides.indexName and on
env['PINECONE_INDEX_NAME'] separately, then use ?? to fall back to
ALLIANCE_DEFAULT_INDEX_NAME (similarly for rerankModel using
env['PINECONE_RERANK_MODEL'] and ALLIANCE_DEFAULT_RERANK_MODEL), ensuring
trimOptional converts blank strings to undefined before the ?? checks.
In `@src/constants.ts`:
- Line 35: Update the SERVER_INSTRUCTIONS string in src/constants.ts to scope
the startup config requirement to the core entrypoint (or explicitly mention the
Alliance fallback) so it no longer states that PINECONE_INDEX_NAME/--index-name
is always required; change the sentence around "The server process requires
`PINECONE_INDEX_NAME` (or `--index-name`)" to say that this is required for the
core server entrypoint but that the Alliance resolver will default the index
when omitted, preserving the rest of the guidance (references: the
SERVER_INSTRUCTIONS constant).
In `@src/core/config.test.ts`:
- Around line 33-36: The test "uses indexName from overrides when set" is
leaking real process.env and can fail if PINECONE_SPARSE_INDEX_NAME is set;
update the test around resolveConfig(...) to temporarily clear or set
process.env.PINECONE_SPARSE_INDEX_NAME (e.g., save original, delete or set to
undefined, call resolveConfig, then restore) so the assertion on
cfg.sparseIndexName === 'override-index-sparse' is deterministic; reference the
test block that calls resolveConfig and the env var name
PINECONE_SPARSE_INDEX_NAME to locate where to add the setup/teardown.
---
Outside diff comments:
In `@docs/MIGRATION.md`:
- Around line 3-5: Update the opening paragraph in MIGRATION.md to clarify which
changelog section is authoritative for this revision: state that if an
"Unreleased" migration block exists in CHANGELOG.md it takes precedence and
should be followed, otherwise fall back to the [0.2.0] section; reference the
"Unreleased" heading and the "[0.2.0]" changelog entry by name so readers know
which guidance to apply.
🪄 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: b261c715-81ed-4294-92b1-9db0bc9a8609
📒 Files selected for processing (21)
.env.exampleCHANGELOG.mdREADME.mddocs/CONFIGURATION.mddocs/MIGRATION.mdexamples/alliance/.env.exampleexamples/alliance/README.mdexamples/alliance/library-embedding-demo.tsexamples/alliance/preset.tssrc/alliance/config.test.tssrc/alliance/config.tssrc/alliance/index.tssrc/alliance/setup.tssrc/cli.tssrc/constants.tssrc/core/config.test.tssrc/core/config.tssrc/core/index.tssrc/core/server/config-context.tssrc/index.tssrc/types.ts
…jonathanMLDev/pinecone-read-only-mcp-typescript into fix/require-pinecone-index-name
3 participants
Pull Request
Summary
Moves C++ Alliance deployment defaults (
rag-hybrid,bge-reranker-v2-m3) out of generic coreresolveConfigand into AllianceresolveAllianceConfig. Core now requires an explicit Pinecone index and treats rerank as opt-in; the published CLI andsetupAllianceServerstill apply Alliance defaults when env/CLI omit index/rerank, so API-key-only MCP configs are unchanged.Completes the core/alliance config boundary started in Week 4 PR A. Blocks issue_01 (
ServerContextrefactor) — merge before that PR to avoid moving Alliance defaults twice.Changes
src/core/config.tsDEFAULT_INDEX_NAME/DEFAULT_RERANK_MODEL; throw if index missing; optionalrerankModel; exporttrimOptionalsrc/core/index.tssrc/alliance/config.tsALLIANCE_DEFAULT_INDEX_NAME,ALLIANCE_DEFAULT_RERANK_MODEL, realresolveAllianceConfig; removeapplyAllianceRerankDefaultsrc/alliance/index.tsresolveAllianceConfigsrc/index.tsresolveAllianceConfigsrc/alliance/setup.tsresolveAllianceConfig({})when config omittedsrc/core/server/config-context.tsresolveAllianceConfigsrc/core/config.test.tssrc/alliance/config.test.tsexamples/alliance/.env.exampleexamples/alliance/preset.tsALLIANCE_INDEX_NAME,ALLIANCE_RERANK_MODELfor demosexamples/alliance/README.md.env.examplepointerexamples/alliance/library-embedding-demo.tsresolveAllianceConfig.env.exampleREADME.mddocs/CONFIGURATION.mdresolveConfigvsresolveAllianceConfigtabledocs/MIGRATION.mdCHANGELOG.md[Unreleased]breaking (core) + Alliance CLI notesrc/cli.tssrc/types.tsBehavior
resolveConfig/setupCoreServerMissing Pinecone index name…resolveAllianceConfigrag-hybridbge-reranker-v2-m3Acceptance criteria
DEFAULT_INDEX_NAMEremoved from core (not exported from package root)DEFAULT_RERANK_MODELremoved from core (not exported from package root)resolveConfig:PINECONE_INDEX_NAMErequired with clear errorresolveConfig: no default rerank model when unsetresolveAllianceConfig:ALLIANCE_DEFAULT_*when env/CLI omit valuessetupAllianceServeruseresolveAllianceConfigexamples/alliance/.env.example+ README core vs Alliance docsBreaking change (core only)
resolveConfig({ apiKey })withoutindexName/PINECONE_INDEX_NAMEthrows.DEFAULT_INDEX_NAMEorDEFAULT_RERANK_MODEL.PINECONE_RERANK_MODEL(orrerankModel) to enable reranking.Unchanged: MCP stdio / CLI with only
PINECONE_API_KEY(Alliance resolver applies defaults).Downstream
ServerContext): merge this PR first; config lives inresolveConfig/resolveAllianceConfiguntil singleton refactor.Test plan
npm run ci— typecheck, lint, format, build,test:coveragenpm run docs:link-checkresolveConfig({ apiKey: 'x' }, { PINECONE_API_KEY: 'x' })throws index errorcfg.rerankModel === undefinedresolveAllianceConfig({ apiKey: 'x' }, { PINECONE_API_KEY: 'x' })→rag-hybrid,bge-reranker-v2-m3PINECONE_API_KEY=… node dist/index.jslogs Alliance index and rerankChecklist
[Unreleased]CHANGELOG updateddocs/MIGRATION.mdRelated Issue
Summary by CodeRabbit
Breaking Changes
Documentation
Examples