feat: add keyword_search tool for pinecone-rag-sparse index (#29) - #31
Merged
Conversation
feat: add keyword_search tool for pinecone-rag-sparse index (#29) - Add PineconeClient.keywordSearch() for sparse-only (lexical) search - Register keyword_search MCP tool (query_text, namespace, top_k, optional metadata_filter, fields) - Config: PINECONE_SPARSE_INDEX_NAME / --sparse-index-name (default: pinecone-rag-sparse) - Document tool in README; add Test 5 in scripts/test-search.ts - No reranking; returns same result shape as query tool (reranked: false)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a keyword_search feature that queries a dedicated sparse Pinecone index (default Changes
Sequence DiagramsequenceDiagram
participant Client as "Client"
participant MCPServer as "MCP Server"
participant KeywordTool as "KeywordSearchTool"
participant PineconeClient as "PineconeClient"
participant SparseIndex as "Sparse Index (pinecone-rag-sparse)"
Client->>MCPServer: call keyword_search(params)
MCPServer->>KeywordTool: invoke handler
KeywordTool->>KeywordTool: validate inputs (query_text, namespace, filters)
KeywordTool->>PineconeClient: keywordSearch(params)
PineconeClient->>PineconeClient: clampTopK & ensureKeywordIndex()
PineconeClient->>SparseIndex: execute sparse-only search
SparseIndex-->>PineconeClient: return hits
PineconeClient-->>KeywordTool: return SearchResult[]
KeywordTool-->>MCPServer: formatted KeywordSearchResponse
MCPServer-->>Client: deliver keyword_search results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
1-1:⚠️ Potential issue | 🟡 MinorFix Prettier formatting to unblock CI.
The pipeline reports a Prettier formatting failure on this file. Run
prettier --write src/index.ts(ornpm run format) to resolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` at line 1, Prettier has flagged formatting in src/index.ts (starts with the shebang "#!/usr/bin/env node"); run the project's formatter (e.g., `prettier --write src/index.ts` or `npm run format`) to reformat the file to project standards and then stage & commit the updated file so CI passes.
🧹 Nitpick comments (3)
src/server/tools/keyword-search-tool.ts (1)
117-123: Redundant?? 10fallback —top_kis always defined here.
top_khas.default(10)in the Zod schema (line 102), soparams.top_kis always anumberat this point; the?? 10branch is dead code.🛠️ Proposed cleanup
- top_k: params.top_k ?? 10, + top_k: params.top_k,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tools/keyword-search-tool.ts` around lines 117 - 123, The call to executeKeywordSearch uses a redundant fallback (params.top_k ?? 10) even though the Zod schema sets a default for top_k, so remove the dead-code fallback and pass params.top_k directly; update the invocation in keyword-search-tool.ts (the executeKeywordSearch call) to use top_k: params.top_k and ensure no other places rely on the nullish fallback for top_k.src/pinecone-client.ts (1)
497-503: Minor duplication:topKnormalization is copy-pasted fromquery().Consider extracting to a private helper (e.g.,
clampTopK(requested: number | undefined): number) to avoid keeping two identical blocks in sync.♻️ Suggested helper
+ private clampTopK(requested: number | undefined): number { + const topK = requested !== undefined ? requested : this.defaultTopK; + if (topK < 1) throw new Error('topK must be at least 1'); + return Math.min(topK, MAX_TOP_K); + }Then in both
query()andkeywordSearch():- let topK = requestedTopK !== undefined ? requestedTopK : this.defaultTopK; - if (topK < 1) { - throw new Error('topK must be at least 1'); - } - if (topK > MAX_TOP_K) { - topK = MAX_TOP_K; - } + const topK = this.clampTopK(requestedTopK);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pinecone-client.ts` around lines 497 - 503, Extract the repeated topK normalization into a private helper (e.g., clampTopK(requested: number | undefined): number) that uses this.defaultTopK and MAX_TOP_K to compute the final topK and validates topK >= 1; then replace the duplicated blocks in both query() and keywordSearch() to call clampTopK(requestedTopK) (or the appropriate param name) so validation and clamping are centralized.scripts/test-search.ts (1)
155-175:duration5is never printed in the performance summary.The final summary block (lines 178–184) shows timings for Tests 2, 3, and 4 but omits Test 5. This is a minor gap; callers already see the inline
duration5ms line, but the summary is inconsistent.🛠️ Proposed addition
+ let duration5: number | undefined; try { const startTime5 = Date.now(); const results5 = await client.keywordSearch({ ... }); - const duration5 = Date.now() - startTime5; + duration5 = Date.now() - startTime5; ... }And in the summary:
if (duration3 !== undefined) { console.log(` With metadata filter: ${duration3}ms`); } + if (duration5 !== undefined) { + console.log(` Keyword search: ${duration5}ms`); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-search.ts` around lines 155 - 175, Add Test 5's timing to the final performance summary by including duration5 (from the keywordSearch call) alongside the other durations; update the summary printing logic that currently reports durations for Test 2, Test 3, and Test 4 to also print Test 5 and reference duration5 and whether results5 was returned (or skipped) so the summary matches the inline log for the keywordSearch invocation in scripts/test-search.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/tools/keyword-search-tool.ts`:
- Line 1: The file fails Prettier formatting; run your formatter (e.g., prettier
--write or npm run format) on the file containing the import of McpServer
(import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js') to
fix spacing/linebreaks and any other style issues; ensure the import line and
surrounding code conform to project Prettier rules, then re-run CI.
- Around line 66-74: The response currently hardcodes DEFAULT_SPARSE_INDEX_NAME
in KeywordSearchResponse, which ignores runtime configuration; update code to
use the active sparse index from PineconeClient instead: add a readonly getter
(e.g., get sparseIndexName()) to the PineconeClient implementation that returns
the configured index (falling back to DEFAULT_SPARSE_INDEX_NAME), then replace
DEFAULT_SPARSE_INDEX_NAME in src/server/tools/keyword-search-tool.ts with
PineconeClient.sparseIndexName (or the instance reference used there) so the
response.index reflects the actual runtime value.
---
Outside diff comments:
In `@src/index.ts`:
- Line 1: Prettier has flagged formatting in src/index.ts (starts with the
shebang "#!/usr/bin/env node"); run the project's formatter (e.g., `prettier
--write src/index.ts` or `npm run format`) to reformat the file to project
standards and then stage & commit the updated file so CI passes.
---
Nitpick comments:
In `@scripts/test-search.ts`:
- Around line 155-175: Add Test 5's timing to the final performance summary by
including duration5 (from the keywordSearch call) alongside the other durations;
update the summary printing logic that currently reports durations for Test 2,
Test 3, and Test 4 to also print Test 5 and reference duration5 and whether
results5 was returned (or skipped) so the summary matches the inline log for the
keywordSearch invocation in scripts/test-search.ts.
In `@src/pinecone-client.ts`:
- Around line 497-503: Extract the repeated topK normalization into a private
helper (e.g., clampTopK(requested: number | undefined): number) that uses
this.defaultTopK and MAX_TOP_K to compute the final topK and validates topK >=
1; then replace the duplicated blocks in both query() and keywordSearch() to
call clampTopK(requestedTopK) (or the appropriate param name) so validation and
clamping are centralized.
In `@src/server/tools/keyword-search-tool.ts`:
- Around line 117-123: The call to executeKeywordSearch uses a redundant
fallback (params.top_k ?? 10) even though the Zod schema sets a default for
top_k, so remove the dead-code fallback and pass params.top_k directly; update
the invocation in keyword-search-tool.ts (the executeKeywordSearch call) to use
top_k: params.top_k and ensure no other places rely on the nullish fallback for
top_k.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.mdscripts/test-search.tssrc/constants.tssrc/index.tssrc/pinecone-client.tssrc/server.tssrc/server/tools/keyword-search-tool.tssrc/types.ts
Made-with: Cursor
…, test summary) fix: address CodeRabbit feedback for keyword_search and tooling - Prettier: format keyword-search-tool.ts and index.ts - response.index: add getSparseIndexName() on PineconeClient; use in keyword_search - pinecone-client: extract clampTopK(), use in query() and keywordSearch() - keyword-search-tool: remove redundant top_k ?? 10 (Zod default) - test-search: add Test 5 timing to performance summary (duration5 / skipped)
This reverts commit 5b97159.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/pinecone-client.ts (1)
523-541: Consider extracting shared hit-to-SearchResult transformation.This hit transformation logic duplicates lines 336-352 in
mergeResults(). Both extractchunk_textas content and build a metadata object from remaining fields.♻️ Suggested extraction
private hitToPartial(hit: PineconeHit): { content: string; metadata: Record<string, PineconeMetadataValue> } { const fields = hit.fields || {}; let content = ''; const metadata: Record<string, PineconeMetadataValue> = {}; for (const [key, value] of Object.entries(fields)) { if (key === 'chunk_text') { content = typeof value === 'string' ? value : ''; } else { metadata[key] = value as PineconeMetadataValue; } } return { content, metadata }; }Then reuse in both
mergeResults()andkeywordSearch().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pinecone-client.ts` around lines 523 - 541, Duplicate logic that converts a Pinecone hit into content+metadata appears in keywordSearch() (lines shown) and mergeResults(); extract it into a single private helper (e.g., private hitToPartial(hit: PineconeHit): { content: string; metadata: Record<string, PineconeMetadataValue> }) that contains the fields parsing and 'chunk_text' handling, then replace the inline loops in both keywordSearch() and mergeResults() to call hitToPartial() and construct the SearchResult objects from its return value; keep the returned shape and types identical to avoid changing downstream code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/test-search.ts`:
- Around line 155-184: The test uses testNamespace (populated from
listNamespacesWithMetadata()) which queries the dense index but then calls
client.keywordSearch() against the sparse index (pinecone-rag-sparse), so the
namespace may not exist there; update the test to fetch namespaces from the
sparse index before running Test 5 (e.g., call the same namespace-listing helper
but pointed at the sparse index or add a new listNamespacesFromSparseIndex() and
fall back to a documented default), then use that sparse-derived namespace for
keywordSearch, or if none exist skip Test 5 with a clear message; update
references to testNamespace, listNamespacesWithMetadata(), and the keywordSearch
invocation accordingly.
---
Nitpick comments:
In `@src/pinecone-client.ts`:
- Around line 523-541: Duplicate logic that converts a Pinecone hit into
content+metadata appears in keywordSearch() (lines shown) and mergeResults();
extract it into a single private helper (e.g., private hitToPartial(hit:
PineconeHit): { content: string; metadata: Record<string, PineconeMetadataValue>
}) that contains the fields parsing and 'chunk_text' handling, then replace the
inline loops in both keywordSearch() and mergeResults() to call hitToPartial()
and construct the SearchResult objects from its return value; keep the returned
shape and types identical to avoid changing downstream code.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.mdscripts/test-search.tssrc/index.tssrc/pinecone-client.tssrc/server/tools/keyword-search-tool.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/server/tools/keyword-search-tool.ts
- README.md
- src/index.ts
fix: use sparse index namespaces for Test 5 keyword search - Add PineconeClient.listNamespacesFromKeywordIndex() to list namespaces from the keyword (sparse) index - Test 5 now uses a namespace from the sparse index for keywordSearch; skip with clear message if sparse index has no namespaces - Avoids using dense-derived testNamespace for sparse-only query (CodeRabbit feedback)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/pinecone-client.ts (1)
546-564: Consider extracting a helper for hit-to-SearchResult mapping.The logic that transforms
PineconeHitintoSearchResult(extractingchunk_textascontent, copying remaining fields tometadata) is duplicated here, inquery()(lines 498-504), and in thererankResults()fallback (lines 427-433).Extracting a small helper would reduce duplication and make future changes easier to maintain.
♻️ Example helper extraction
+ /** + * Transform a raw Pinecone hit into a SearchResult. + */ + private hitToSearchResult(hit: PineconeHit, reranked: boolean): SearchResult { + const fields = hit.fields || {}; + let content = ''; + const metadata: Record<string, PineconeMetadataValue> = {}; + for (const [key, value] of Object.entries(fields)) { + if (key === 'chunk_text') { + content = typeof value === 'string' ? value : ''; + } else { + metadata[key] = value as PineconeMetadataValue; + } + } + return { + id: hit._id || '', + content, + score: hit._score || 0, + metadata, + reranked, + }; + }Then in
keywordSearch():- const documents: SearchResult[] = hits.map((hit) => { - const fields = hit.fields || {}; - let content = ''; - const metadata: Record<string, PineconeMetadataValue> = {}; - for (const [key, value] of Object.entries(fields)) { - if (key === 'chunk_text') { - content = typeof value === 'string' ? value : ''; - } else { - metadata[key] = value as PineconeMetadataValue; - } - } - return { - id: hit._id || '', - content, - score: hit._score || 0, - metadata, - reranked: false, - }; - }); + const documents: SearchResult[] = hits.map((hit) => this.hitToSearchResult(hit, false));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pinecone-client.ts` around lines 546 - 564, Extract the repeated PineconeHit->SearchResult mapping into a single helper (e.g., mapHitToSearchResult or convertPineconeHitToSearchResult) that takes a PineconeHit and returns a SearchResult by pulling 'chunk_text' into content (string or ''), copying all other fields into metadata (as PineconeMetadataValue), setting id to hit._id || '', score to hit._score || 0, and reranked to false; then replace the duplicated mapping logic in keywordSearch(), query(), and the rerankResults() fallback with calls to this helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/pinecone-client.ts`:
- Around line 546-564: Extract the repeated PineconeHit->SearchResult mapping
into a single helper (e.g., mapHitToSearchResult or
convertPineconeHitToSearchResult) that takes a PineconeHit and returns a
SearchResult by pulling 'chunk_text' into content (string or ''), copying all
other fields into metadata (as PineconeMetadataValue), setting id to hit._id ||
'', score to hit._score || 0, and reranked to false; then replace the duplicated
mapping logic in keywordSearch(), query(), and the rerankResults() fallback with
calls to this helper.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/test-search.tssrc/pinecone-client.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/test-search.ts
jonathanMLDev
self-requested a review
March 4, 2026 15:57
…ewer feedback)
refactor: use single sparse index for hybrid and keyword search
- Remove keywordIndex; use sparseIndex from ensureIndexes() for keywordSearch and listNamespacesFromKeywordIndex
- Remove sparseIndexName config (PINECONE_SPARSE_INDEX_NAME, --sparse-index-name) and DEFAULT_SPARSE_INDEX_NAME
- getSparseIndexName() now returns ${indexName}-sparse. Addresses reviewer: sparse_index and keyword_index are the same index
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pinecone-client.ts`:
- Around line 86-95: clampTopK currently allows NaN/Infinity because simple
numeric comparisons don't reject non-finite numbers; update the
clampTopK(requested: number | undefined) implementation to explicitly reject
non-finite values (use Number.isFinite(requested) or isFinite) before any
comparisons and throw an Error like 'topK must be a finite number >= 1' if
requested is non-finite, then proceed to defaulting to this.defaultTopK, enforce
>=1 and cap at MAX_TOP_K (referencing clampTopK and MAX_TOP_K to locate the
change).
In `@src/server/tools/keyword-search-tool.ts`:
- Around line 39-79: Trim and normalize inputs before use: create
normalizedQuery = query_text.trim() (reuse instead of calling trim inline) and
normalizedNamespace = namespace?.trim(); treat normalizedNamespace === '' as
undefined (or validate/error if desired) and pass normalizedNamespace to
client.keywordSearch and include normalizedQuery and normalizedNamespace in the
returned KeywordSearchResponse (update response.query and response.namespace).
Ensure fields, metadata_filter, formatQueryResultRows(results, { namespace:
normalizedNamespace }) and client.getSparseIndexName() remain unchanged and that
validateMetadataFilter(metadata_filter) logic still runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bdfd6c0f-2f29-46fe-9c0c-2735445d50ad
📒 Files selected for processing (7)
README.mdscripts/test-search.tssrc/constants.tssrc/index.tssrc/pinecone-client.tssrc/server/tools/keyword-search-tool.tssrc/types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/types.ts
- scripts/test-search.ts
- src/index.ts
…mpTopK, keyword_search normalization
wpak-ai
approved these changes
Mar 9, 2026
4 tasks
11 tasks
This was referenced May 13, 2026
Merged
Merged
5 tasks
4 tasks
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.
Closes #29
feat: add keyword_search tool for pinecone-rag-sparse index (#29)
Summary by CodeRabbit