#15-update mcp with several tools - #21
Merged
Merged
Conversation
jonathanMLDev
changed the title
…d-only-mcp-typescript into dev-8 # Conflicts: # src/pinecone-client.ts
Collaborator
|
@coderabbitai Review this PR |
|
I'll analyze the code quality, architecture decisions, potential issues, test coverage, and ensure the changes align with best practices. The review will cover all modified and new files. ✅ Actions performedReview triggered.
|
|
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:
📝 WalkthroughWalkthroughModularizes server into many new modules and tools (namespace routing, suggestion flow, metadata validation, URL generation, reassembly), adds typed Pinecone client features (count, namespaces, typed hybrid search), introduces a level-based logger and CLI log-level, adds CI/Docker/npm script updates, and includes extensive tests and docs updates. Changes
Sequence DiagramsequenceDiagram
actor User
participant Guided as GuidedQueryTool
participant NSCache as NamespacesCache
participant Router as NamespaceRouter
participant Suggest as QuerySuggestion
participant Flow as SuggestionFlow
participant Pinecone as PineconeClient
participant Fmt as ResultFormatter
rect rgba(200,200,255,0.5)
User->>Guided: user_query, (optional) namespace, metadata_filter
end
Guided->>NSCache: getNamespacesWithCache()
NSCache-->>Guided: namespaces (cache_hit?)
alt namespace not provided
Guided->>Router: rankNamespacesByQuery(user_query, namespaces)
Router-->>Guided: recommended namespace
end
Guided->>NSCache: getNamespacesWithCache() for chosen namespace
NSCache-->>Guided: namespace metadata
Guided->>Suggest: suggestQueryParams(metadata, user_query)
Suggest-->>Guided: recommended_tool, suggested_fields
Guided->>Flow: markSuggested(namespace, {recommended_tool, suggested_fields, user_query})
alt recommended_tool == "count"
Guided->>Pinecone: count(query, namespace, metadata_filter)
Pinecone-->>Guided: {count, truncated}
else
Guided->>Pinecone: query/search(top_k, fields, use_reranking, metadata_filter)
Pinecone-->>Guided: search results
Guided->>Fmt: formatQueryResultRows(results, {enrichUrls?, contentMaxLength})
Fmt-->>Guided: formatted rows
end
Guided-->>User: structured JSON response (status, results or count, decision_trace)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
Collaborator
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (14)
src/server/tool-response.ts (1)
1-4:TextPayloadis not exported.Tool modules that consume
jsonResponse/jsonErrorResponsecan't annotate local variables or function signatures withTextPayloadwithout resorting toReturnType<typeof jsonResponse>.♻️ Proposed fix
-type TextPayload = { +export type TextPayload = { content: Array<{ type: 'text'; text: string }>; isError?: boolean; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tool-response.ts` around lines 1 - 4, The type TextPayload is currently declared but not exported, preventing upstream tool modules from referencing it; export the type by changing its declaration to an exported type (e.g., export type TextPayload = { ... }) in the file where TextPayload is defined and ensure any related exported helpers jsonResponse and jsonErrorResponse use or re-export TextPayload if needed so consumers can import the type directly (reference: TextPayload, jsonResponse, jsonErrorResponse).src/logger.ts (1)
56-62: Stack trace discarded inerror()— onlyerr.messageis logged.For ERROR-level events, the stack trace is the primary debugging artifact. Losing it makes production issues significantly harder to diagnose.
♻️ Proposed fix
export function error(msg: string, err?: unknown): void { if (shouldLog('ERROR')) { - const detail = err instanceof Error ? err.message : err !== undefined ? String(err) : undefined; - console.error( - formatMessage('ERROR', msg, detail !== undefined ? { error: detail } : undefined) - ); + const detail = + err instanceof Error + ? { message: err.message, stack: err.stack } + : err !== undefined + ? String(err) + : undefined; + console.error( + formatMessage('ERROR', msg, detail !== undefined ? { error: detail } : undefined) + ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/logger.ts` around lines 56 - 62, The error() logger currently only logs err.message and discards the stack; update error() so when err is an Error include its stack (err.stack) in the details passed to formatMessage (e.g., add a property like stack: err.stack) and for non-Error values include their string form as before; ensure you still call shouldLog('ERROR') and pass undefined when no error is provided so formatMessage usage (and its output shape) remains consistent with existing behavior..github/workflows/ci.yml (1)
41-42:npm run smokeredundantly rebuilds already-compiled artifacts.The "Build" step on Line 39 already runs
npm run build. Sincenpm run smokeis defined asnpm run build && node dist/index.js --help, the build step runs twice in every matrix cell. The smoke step should run the CLI check directly against the already-built output.♻️ Proposed fix
- name: Smoke test CLI - run: npm run smoke + run: node dist/index.js --help🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 41 - 42, The CI currently runs "npm run build" and then the "Smoke test CLI" step calls "npm run smoke" which redundantly re-runs the build because the "smoke" script is defined as "npm run build && node dist/index.js --help"; update the "Smoke test CLI" step to run the already-built artifact directly (replace "npm run smoke" with "node dist/index.js --help") or alternatively change the package.json "smoke" script to only run "node dist/index.js --help" so the build is not duplicated; target the "Smoke test CLI" step and the "smoke" and "build" npm scripts when making this change.src/server/tools/namespace-router-tool.ts (1)
19-30: Redundant JS-level default fortop_n.The Zod schema already has
.default(3)(line 24), which Zod v4 applies during parsing before the handler is called —params.top_nwill never beundefined. The= 3in the destructuring (line 30) is dead code.♻️ Proposed cleanup
- const { user_query, top_n = 3 } = params; + const { user_query, top_n } = params;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tools/namespace-router-tool.ts` around lines 19 - 30, The destructuring in the async handler redundantly sets a JS default for top_n even though the Zod schema (top_n: z.number().int().min(1).max(5).default(3)) already supplies a default during parsing; remove the `= 3` default from `const { user_query, top_n = 3 } = params` so the handler uses the parsed params directly (reference: the Zod schema for `top_n` and the async handler's parameter destructuring).src/server/url-generation.test.ts (1)
4-59: Add coverage for theunavailablepaths ingenerateUrlForNamespace.The implementation has two observable error paths that are not tested:
mailingnamespace with neitherdoc_idnorthread_idpresent → returns{ url: null, method: 'unavailable' }slack-Cpplangwith nosourceand at least one ofteam_id/channel_id/doc_idmissing → same✅ Suggested additional tests
it('returns unavailable for mailing when no doc_id or thread_id', () => { const r = generateUrlForNamespace('mailing', { author: 'someone' }); expect(r.url).toBeNull(); expect(r.method).toBe('unavailable'); }); it('returns unavailable for slack-Cpplang when required fields are missing', () => { const r = generateUrlForNamespace('slack-Cpplang', { team_id: 'T123', // channel_id missing, doc_id missing, no source }); expect(r.url).toBeNull(); expect(r.method).toBe('unavailable'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/url-generation.test.ts` around lines 4 - 59, Add two tests to src/server/url-generation.test.ts that cover the "unavailable" branches in generateUrlForNamespace: (1) call generateUrlForNamespace('mailing', { author: 'someone' }) and assert { url: null, method: 'unavailable' } to cover missing doc_id and thread_id; (2) call generateUrlForNamespace('slack-Cpplang', { team_id: 'T123' } ) (no source, no channel_id, no doc_id) and assert { url: null, method: 'unavailable' } to cover the Slack missing-fields path. Ensure both tests use the same expect pattern as existing tests (expect(r.url).toBeNull(); expect(r.method).toBe('unavailable')).src/server/tools/query-tool.ts (1)
75-76:logToolErroralways logs'query'regardless of which tool variant errored.When
query_fastorquery_detailedfails, the error is logged as"Error in query tool", making it harder to distinguish which variant caused the issue. Passmodeto the error log.🔧 Proposed fix
- } catch (error) { - logToolError('query', error); + } catch (error) { + logToolError(mode, error);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tools/query-tool.ts` around lines 75 - 76, The catch block currently calls logToolError('query', error) which always reports "query" even for variants; update the catch to pass the runtime mode identifier (the variable mode used when invoking the tool, e.g., 'query_fast' or 'query_detailed') to logToolError instead of the hardcoded 'query' so the error log reflects the actual tool variant that failed; locate the catch in this file where logToolError is invoked and replace the fixed string with the mode variable (ensuring mode is in scope) so logToolError(mode, error) is called.src/server/suggestion-flow.ts (1)
10-17: Stale entries are only evicted on read — no proactive cleanup.
markSuggestedadds entries but they're only removed whenrequireSuggestedis called for that specific namespace after TTL expiry. Entries for namespaces that are never queried again will persist for the process lifetime. For a stdio MCP server with a small namespace set this is negligible, but if this ever runs long-lived with many namespaces, consider a periodic sweep or an LRU cap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/suggestion-flow.ts` around lines 10 - 17, stateByNamespace is growing indefinitely because markSuggested only inserts entries and stale entries are only removed later by requireSuggested; add a proactive eviction mechanism in this module: either implement a periodic sweep (set up a timer on module load that iterates stateByNamespace and deletes entries whose updatedAt is older than the TTL used by requireSuggested) or enforce an LRU/cap on stateByNamespace (evict the least-recently-used keys when size exceeds a configured max). Update markSuggested and any accessors (e.g., requireSuggested) to touch/update LRU metadata if you choose the LRU route, and ensure the sweep timer is cleared on shutdown if applicable.src/server/namespace-router.ts (1)
69-73: Tiebreaker favors smaller namespaces — intentional?When scores are equal,
a.record_count - b.record_countsorts ascending, so smaller namespaces rank higher. This could be surprising if a user expects larger (more comprehensive) namespaces to be preferred. If this is intentional (preferring more-specific namespaces), a brief comment would clarify intent for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/namespace-router.ts` around lines 69 - 73, The tiebreaker in the sort comparator currently prefers smaller namespaces because it returns a.record_count - b.record_count when scores are equal; update the comparator to prefer larger namespaces by changing the tiebreak expression to b.record_count - a.record_count (inside the .sort((a, b) => { ... }) block that uses a.score and b.score) and add a short inline comment explaining the chosen policy (prefer larger/more comprehensive namespaces) so future maintainers understand the intent before the .slice(0, topN) selection.src/server/metadata-filter.ts (1)
49-62: Comparison operators accept non-primitive values without errorThe loop recurses into all sub-keys but only enforces an array check for
$in/$nin. For scalar comparison operators ($gt,$lt,$gte,$lte,$eq,$ne), a nested object value like{"price": {"$gt": {"bad": 1}}}passes validation and will only be rejected later by Pinecone's API. Adding a primitive check for these operators would surface the error earlier with a clearer message.♻️ Proposed addition
if (key.startsWith('$')) { if (!ALLOWED_FILTER_OPERATORS.has(key)) { return `Unsupported filter operator "${key}" at "${path.join('.')}".`; } if ((key === '$in' || key === '$nin') && !isPrimitiveArray(nestedValue)) { return `Operator "${key}" at "${path.join('.')}" must use an array of primitive values.`; } + if ( + (key === '$eq' || key === '$ne' || key === '$gt' || key === '$gte' || + key === '$lt' || key === '$lte') && + !isPrimitiveFilterValue(nestedValue) + ) { + return `Operator "${key}" at "${path.join('.')}" must use a primitive value.`; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/metadata-filter.ts` around lines 49 - 62, The validator currently only enforces array types for $in/$nin but allows non-primitive values for scalar comparison operators; update validateMetadataFilterValue to also check when key is one of the comparison operators ($gt, $lt, $gte, $lte, $eq, $ne) that nestedValue is a primitive (use the existing isPrimitive helper), and return a descriptive error like `Operator "${key}" at "${path.join('.')}" must use a primitive value.` if not; locate this logic where ALLOWED_FILTER_OPERATORS is checked and add the additional branch/condition to enforce primitives for these comparison operators.src/server/tools/guided-query-tool.ts (1)
61-64: Destructuring defaults are redundant with Zod.default()
top_k,preferred_tool, andenrich_urlsall have.default(...)in the schema, so the MCP SDK will have already applied those defaults before the handler runs. The JS-level fallback assignments are dead code.♻️ Suggested cleanup
const { user_query, namespace: inputNamespace, metadata_filter, - top_k = 10, - preferred_tool = 'auto', - enrich_urls = true, + top_k, + preferred_tool, + enrich_urls, } = params;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tools/guided-query-tool.ts` around lines 61 - 64, The JS-level default fallbacks in the params destructuring (top_k = 10, preferred_tool = 'auto', enrich_urls = true) are redundant because the request schema already applies Zod .default(...) values; remove the `= 10`, `= 'auto'`, and `= true` from the destructuring so it reads simply `const { top_k, preferred_tool, enrich_urls } = params;` (leave validation/schema code untouched) to avoid dead code and rely on the already-applied defaults.src/pinecone-client.ts (1)
394-399: Misleading comment:count()does not go throughquery()The comment on Lines 394–395 says "Allow up to COUNT_TOP_K when explicitly requested (e.g. for count tool)", but
count()callssearchIndex()directly and never callsquery(). TherequestedTopK > MAX_TOP_Kbranch is therefore unreachable from the current tooling. The comment is misleading and the path is dead code.♻️ Suggested cleanup
- // Allow up to COUNT_TOP_K when explicitly requested (e.g. for count tool); otherwise cap at MAX_TOP_K - const maxAllowed = - requestedTopK !== undefined && requestedTopK > MAX_TOP_K ? COUNT_TOP_K : MAX_TOP_K; + const maxAllowed = MAX_TOP_K;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pinecone-client.ts` around lines 394 - 399, The comment and conditional that allow COUNT_TOP_K when requestedTopK > MAX_TOP_K are misleading/unreachable because count() invokes searchIndex() directly (it never goes through query()), so the requestedTopK branch is dead; either remove the requestedTopK > MAX_TOP_K branch and the COUNT_TOP_K path and update the comment to state we always cap at MAX_TOP_K in searchIndex(), or if the intent was to let count() request a higher limit, propagate that intent by adding/using an explicit parameter from count() into searchIndex()/query() and adjust the logic in the function containing topK/maxAllowed (refer to searchIndex(), count(), and query() to locate the code) so the higher COUNT_TOP_K path is actually reachable and the comment reflects the behavior.src/server/tools/count-tool.ts (1)
52-52: Redundant!query_textguardSince
query_textisz.string()(required), it will never benull/undefined. The!query_textbranch is already fully subsumed by!query_text.trim().♻️ Suggested simplification
- if (!query_text || !query_text.trim()) { + if (!query_text.trim()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tools/count-tool.ts` at line 52, The conditional "if (!query_text || !query_text.trim())" is redundant because query_text is a required z.string(); remove the unnecessary "!query_text" check and simplify the guard to check only trimming (e.g., use "if (!query_text.trim())" or "if (query_text.trim().length === 0)"). Update the check in the function/method that uses the query_text variable in src/server/tools/count-tool.ts so the logic remains the same but without the nullable guard.src/server/format-query-result.ts (1)
48-48:.replace('.md', '')removes only the first occurrence
String.prototype.replacewith a string literal replaces only the first match. Use a trailing-extension regex to be safe:♻️ Suggested fix
- (typeof filename === 'string' ? filename.replace('.md', '').toUpperCase() : null) ?? + (typeof filename === 'string' ? filename.replace(/\.md$/i, '').toUpperCase() : null) ??🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/format-query-result.ts` at line 48, The current expression using filename.replace('.md', '') only removes the first occurrence; update the transformation of the filename variable in src/server/format-query-result.ts to strip a trailing ".md" safely (case-insensitive) — e.g., replace the string-based replace call with a trailing-extension regex or an endsWith-based slice so only a terminal ".md" is removed (use filename.replace(/\.md$/i, '') or filename.endsWith('.md') ? filename.slice(0, -3) : filename) and keep the existing null-coalescing behavior.src/types.ts (1)
99-118: Consider narrowing optional methods onSearchableIndexif SDK surface stabilizes.Every method on this interface is optional (
?), which requires runtime existence checks at every call site. All current usage properly guards these methods (typeof, conditional, ternary), but the pattern does weaken compile-time type safety. If the Pinecone SDK API surface stabilizes or versioning constraints tighten, splitting this into two interfaces — one for the top-level index and one for the namespace handle — would let call sites rely on types rather than runtime guards.If the optionality is intentional to support multiple SDK versions or test mocking flexibility, the current approach is pragmatic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types.ts` around lines 99 - 118, The interface SearchableIndex currently marks all methods optional which forces runtime guards; refactor by splitting it into two clearer interfaces: keep SearchableIndex for top-level index methods (describeIndexStats: Promise<...>, search(opts: { namespace?: string; query: Record<string, unknown>; fields?: string[] }): Promise<{ result?: { hits?: PineconeHit[] } }>, searchRecords(params: { query: Record<string, unknown> }): Promise<{ result?: { hits?: PineconeHit[] } }>) and add a NamespaceHandle (or SearchableNamespace) interface returned by namespace(name: string): NamespaceHandle that defines the namespace-specific query(opts: { topK: number; vector: number[]; includeMetadata: boolean }): Promise<{ matches?: Array<{ metadata?: Record<string, unknown> }> }>; update any usages of namespace(...).query and other call sites to reference the concrete NamespaceHandle type or guard only where truly optional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 8-41: Update the [Unreleased] changelog to accurately reflect this
PR by adding the new tools and infra changes: list the functions/endpoints
list_namespaces, namespace_router, suggest_query_params, count, query_fast,
query_detailed, query, guided_query, and generate_urls under "Added"; note
server modularization, Dockerfile, centralized logging, and CI updates under
"Changed" or "Added" as appropriate; include the timestamp-filter fix under
"Fixed"; and move previously shipped items (hybrid search, semantic reranking,
dynamic namespace discovery, etc.) into the [0.1.0] release section so
[Unreleased] only documents changes introduced by this PR.
In `@README.md`:
- Around line 293-304: The README and codebase are inconsistent about the Slack
namespace casing ('slack-cpplang' vs 'slack-Cpplang'); update the code to use
one canonical key (preferably all-lowercase 'slack-cpplang' to match keyword
hints and scoreNamespace lowercasing) and make the change in url-generation.ts
by replacing the direct equality check (if (namespace === 'slack-Cpplang')) with
a case-insensitive comparison or normalize namespace via toLowerCase() before
comparison, and update README and any other references (namespace-router.ts
keyword hints key, scoreNamespace function usage) to the chosen canonical casing
so all lookups and URL generation match.
In `@src/constants.ts`:
- Around line 12-15: COUNT_TOP_K currently caps countable matches at 10_000 and
COUNT_FIELDS includes 'url' without explanation; update the code and docs to
make the ceiling explicit and provide a way to surface capping to callers: add
JSDoc on COUNT_TOP_K that documents the ceiling and its semantic consequences
and modify the code paths that use COUNT_TOP_K (the count tool function that
reads topK) to return a structured result such as { count: number, capped:
boolean } when results hit the limit; also add a clarifying comment in the JSDoc
for COUNT_FIELDS explaining why 'url' is included for deduplication (or remove
it if unnecessary).
In `@src/pinecone-client.ts`:
- Line 133: Replace all direct console.error calls in src/pinecone-client.ts
with the centralized logger functions imported at the top: use logInfo or
logDebug for informational messages (e.g., the `Found ${namespaces.length}`
message and `Retrieved ${documents.length}`), and use logError for runtime
errors (e.g., errors during sampling records, processing a namespace, listing
namespaces, and failures in dense or sparse index searches). Locate the
occurrences referencing `namespaces`, the sampling/processing error handlers,
the list namespaces catch, the dense/sparse index search catch blocks, and the
documents retrieval block, and swap console.error(...) for the appropriate
logInfo/logDebug/logError call while preserving the original message and any
error object passed.
- Around line 472-491: count() currently falls back to hit._id when document
identifier fields (COUNT_FIELDS: document_number/url/doc_id) are missing, which
yields a chunk count; update the dedup logic in the block that builds docKeys
(uses hits, docKeys, COUNT_TOP_K) to only use the three explicit fields for the
dedup key (remove hit._id as a fallback) and add detection: if none of the hits
contain any of COUNT_FIELDS, emit a warning via the module’s existing logger
(e.g., processLogger/logger) mentioning the namespace and COUNT_FIELDS so
operators can detect namespaces missing identifiers; keep the rest of the return
shape (count and truncated) unchanged.
- Around line 332-342: The cast to (string | Record<string, string>)[] on the
rerank call hides that metadata values may be numbers/booleans/arrays; update
the call in the pc.inference.rerank usage (references: pc.inference.rerank,
this.rerankModel, results, MergedHit, PineconeMetadataValue) by either mapping
results to a safe shape (stringify or ensure the ranked field chunk_text is a
string before passing) or change the cast target to accurately reflect the HTTP
API types (e.g., use Record<string, PineconeMetadataValue> or a union that
includes number/boolean/string[] for metadata values) so the types match real
payloads without unsafe narrowing.
In `@src/server/format-query-result.ts`:
- Around line 37-41: The code currently skips enrichment when metadata['url'] is
an empty string because it only checks typeof !== 'string'; update the condition
in the enrich block (the one that calls generateUrlForNamespace) to treat empty
strings as missing by checking for absence or blank string (e.g. typeof
metadata['url'] !== 'string' || !metadata['url'] ||
String(metadata['url']).trim() === '') before assigning generated.url; keep the
existing guard for options.enrichUrls and options.namespace and then set
metadata['url'] = generated.url when generated.url exists.
In `@src/server/query-suggestion.ts`:
- Around line 24-32: Split the combined check that currently returns
namespace_found: false when either namespaceMetadataFields is missing or
available.length === 0: detect the two cases separately in query-suggestion.ts
by first testing if namespaceMetadataFields is null/undefined (namespace truly
not found) and return namespace_found: false in that branch, and in the branch
where namespace exists but available.length === 0 return namespace_found: true
(or add a distinct flag if you prefer) with suggested_fields: [] and an adjusted
explanation indicating the namespace has no metadata fields; keep other fields
(use_count_tool, recommended_tool) the same so guided-query-tool.ts can
distinguish "not found" vs "empty metadata" using namespace_found and the
explanation.
In `@src/server/url-generation.ts`:
- Around line 57-61: The Slack URLs generated by url-generation.ts are missing
the required "/client/" segment; update the return value that builds the URL
(where messageId is computed from docId and the object with url and method
'generated.slack' is returned) to insert "/client/" after
"https://app.slack.com" so the URL becomes
"https://app.slack.com/client/${teamId}/${channelId}/p${messageId}" ensuring
teamId, channelId and the derived messageId remain used as before.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 41-42: The CI currently runs "npm run build" and then the "Smoke
test CLI" step calls "npm run smoke" which redundantly re-runs the build because
the "smoke" script is defined as "npm run build && node dist/index.js --help";
update the "Smoke test CLI" step to run the already-built artifact directly
(replace "npm run smoke" with "node dist/index.js --help") or alternatively
change the package.json "smoke" script to only run "node dist/index.js --help"
so the build is not duplicated; target the "Smoke test CLI" step and the "smoke"
and "build" npm scripts when making this change.
In `@src/logger.ts`:
- Around line 56-62: The error() logger currently only logs err.message and
discards the stack; update error() so when err is an Error include its stack
(err.stack) in the details passed to formatMessage (e.g., add a property like
stack: err.stack) and for non-Error values include their string form as before;
ensure you still call shouldLog('ERROR') and pass undefined when no error is
provided so formatMessage usage (and its output shape) remains consistent with
existing behavior.
In `@src/pinecone-client.ts`:
- Around line 394-399: The comment and conditional that allow COUNT_TOP_K when
requestedTopK > MAX_TOP_K are misleading/unreachable because count() invokes
searchIndex() directly (it never goes through query()), so the requestedTopK
branch is dead; either remove the requestedTopK > MAX_TOP_K branch and the
COUNT_TOP_K path and update the comment to state we always cap at MAX_TOP_K in
searchIndex(), or if the intent was to let count() request a higher limit,
propagate that intent by adding/using an explicit parameter from count() into
searchIndex()/query() and adjust the logic in the function containing
topK/maxAllowed (refer to searchIndex(), count(), and query() to locate the
code) so the higher COUNT_TOP_K path is actually reachable and the comment
reflects the behavior.
In `@src/server/format-query-result.ts`:
- Line 48: The current expression using filename.replace('.md', '') only removes
the first occurrence; update the transformation of the filename variable in
src/server/format-query-result.ts to strip a trailing ".md" safely
(case-insensitive) — e.g., replace the string-based replace call with a
trailing-extension regex or an endsWith-based slice so only a terminal ".md" is
removed (use filename.replace(/\.md$/i, '') or filename.endsWith('.md') ?
filename.slice(0, -3) : filename) and keep the existing null-coalescing
behavior.
In `@src/server/metadata-filter.ts`:
- Around line 49-62: The validator currently only enforces array types for
$in/$nin but allows non-primitive values for scalar comparison operators; update
validateMetadataFilterValue to also check when key is one of the comparison
operators ($gt, $lt, $gte, $lte, $eq, $ne) that nestedValue is a primitive (use
the existing isPrimitive helper), and return a descriptive error like `Operator
"${key}" at "${path.join('.')}" must use a primitive value.` if not; locate this
logic where ALLOWED_FILTER_OPERATORS is checked and add the additional
branch/condition to enforce primitives for these comparison operators.
In `@src/server/namespace-router.ts`:
- Around line 69-73: The tiebreaker in the sort comparator currently prefers
smaller namespaces because it returns a.record_count - b.record_count when
scores are equal; update the comparator to prefer larger namespaces by changing
the tiebreak expression to b.record_count - a.record_count (inside the .sort((a,
b) => { ... }) block that uses a.score and b.score) and add a short inline
comment explaining the chosen policy (prefer larger/more comprehensive
namespaces) so future maintainers understand the intent before the .slice(0,
topN) selection.
In `@src/server/suggestion-flow.ts`:
- Around line 10-17: stateByNamespace is growing indefinitely because
markSuggested only inserts entries and stale entries are only removed later by
requireSuggested; add a proactive eviction mechanism in this module: either
implement a periodic sweep (set up a timer on module load that iterates
stateByNamespace and deletes entries whose updatedAt is older than the TTL used
by requireSuggested) or enforce an LRU/cap on stateByNamespace (evict the
least-recently-used keys when size exceeds a configured max). Update
markSuggested and any accessors (e.g., requireSuggested) to touch/update LRU
metadata if you choose the LRU route, and ensure the sweep timer is cleared on
shutdown if applicable.
In `@src/server/tool-response.ts`:
- Around line 1-4: The type TextPayload is currently declared but not exported,
preventing upstream tool modules from referencing it; export the type by
changing its declaration to an exported type (e.g., export type TextPayload = {
... }) in the file where TextPayload is defined and ensure any related exported
helpers jsonResponse and jsonErrorResponse use or re-export TextPayload if
needed so consumers can import the type directly (reference: TextPayload,
jsonResponse, jsonErrorResponse).
In `@src/server/tools/count-tool.ts`:
- Line 52: The conditional "if (!query_text || !query_text.trim())" is redundant
because query_text is a required z.string(); remove the unnecessary
"!query_text" check and simplify the guard to check only trimming (e.g., use "if
(!query_text.trim())" or "if (query_text.trim().length === 0)"). Update the
check in the function/method that uses the query_text variable in
src/server/tools/count-tool.ts so the logic remains the same but without the
nullable guard.
In `@src/server/tools/guided-query-tool.ts`:
- Around line 61-64: The JS-level default fallbacks in the params destructuring
(top_k = 10, preferred_tool = 'auto', enrich_urls = true) are redundant because
the request schema already applies Zod .default(...) values; remove the `= 10`,
`= 'auto'`, and `= true` from the destructuring so it reads simply `const {
top_k, preferred_tool, enrich_urls } = params;` (leave validation/schema code
untouched) to avoid dead code and rely on the already-applied defaults.
In `@src/server/tools/namespace-router-tool.ts`:
- Around line 19-30: The destructuring in the async handler redundantly sets a
JS default for top_n even though the Zod schema (top_n:
z.number().int().min(1).max(5).default(3)) already supplies a default during
parsing; remove the `= 3` default from `const { user_query, top_n = 3 } =
params` so the handler uses the parsed params directly (reference: the Zod
schema for `top_n` and the async handler's parameter destructuring).
In `@src/server/tools/query-tool.ts`:
- Around line 75-76: The catch block currently calls logToolError('query',
error) which always reports "query" even for variants; update the catch to pass
the runtime mode identifier (the variable mode used when invoking the tool,
e.g., 'query_fast' or 'query_detailed') to logToolError instead of the hardcoded
'query' so the error log reflects the actual tool variant that failed; locate
the catch in this file where logToolError is invoked and replace the fixed
string with the mode variable (ensuring mode is in scope) so logToolError(mode,
error) is called.
In `@src/server/url-generation.test.ts`:
- Around line 4-59: Add two tests to src/server/url-generation.test.ts that
cover the "unavailable" branches in generateUrlForNamespace: (1) call
generateUrlForNamespace('mailing', { author: 'someone' }) and assert { url:
null, method: 'unavailable' } to cover missing doc_id and thread_id; (2) call
generateUrlForNamespace('slack-Cpplang', { team_id: 'T123' } ) (no source, no
channel_id, no doc_id) and assert { url: null, method: 'unavailable' } to cover
the Slack missing-fields path. Ensure both tests use the same expect pattern as
existing tests (expect(r.url).toBeNull(); expect(r.method).toBe('unavailable')).
In `@src/types.ts`:
- Around line 99-118: The interface SearchableIndex currently marks all methods
optional which forces runtime guards; refactor by splitting it into two clearer
interfaces: keep SearchableIndex for top-level index methods
(describeIndexStats: Promise<...>, search(opts: { namespace?: string; query:
Record<string, unknown>; fields?: string[] }): Promise<{ result?: { hits?:
PineconeHit[] } }>, searchRecords(params: { query: Record<string, unknown> }):
Promise<{ result?: { hits?: PineconeHit[] } }>) and add a NamespaceHandle (or
SearchableNamespace) interface returned by namespace(name: string):
NamespaceHandle that defines the namespace-specific query(opts: { topK: number;
vector: number[]; includeMetadata: boolean }): Promise<{ matches?: Array<{
metadata?: Record<string, unknown> }> }>; update any usages of
namespace(...).query and other call sites to reference the concrete
NamespaceHandle type or guard only where truly optional.
added 2 commits
February 20, 2026 23:48
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/logger.ts (2)
56-71: Remove commented-out code.Lines 58–61 contain the old implementation that was replaced. Dead commented-out code adds noise; remove it to keep the module clean.
♻️ Proposed cleanup
export function error(msg: string, err?: unknown): void { if (shouldLog('ERROR')) { - // const detail = err instanceof Error ? err.message : err !== undefined ? String(err) : undefined; - // console.error( - // formatMessage('ERROR', msg, detail !== undefined ? { error: detail } : undefined) - // ); const detail = err instanceof Error ? { message: err.message, stack: err.stack }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/logger.ts` around lines 56 - 71, The logger.error function contains dead commented-out code from the previous implementation; remove the commented block inside the export function error so only the active implementation remains (keep the existing detail construction that uses err instanceof Error and the console.error call with formatMessage and shouldLog). Ensure you only delete the commented lines and leave the function signature, the shouldLog check, the detail assignment, and the console.error(formatMessage(...)) call intact.
29-36:JSON.stringifywill throw on circular references.If
datacontains circular structures (e.g., an Error with circular.cause),formatMessagewill throw an unhandledTypeError, crashing the log call. A safe fallback keeps the logger robust.🛡️ Proposed safe stringify
function formatMessage(level: string, msg: string, data?: unknown): string { const ts = new Date().toISOString(); const prefix = `[${ts}] [${level}]`; if (data !== undefined) { - return `${prefix} ${msg} ${JSON.stringify(data)}`; + try { + return `${prefix} ${msg} ${JSON.stringify(data)}`; + } catch { + return `${prefix} ${msg} [unserializable data]`; + } } return `${prefix} ${msg}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/logger.ts` around lines 29 - 36, formatMessage currently calls JSON.stringify(data) which will throw on circular structures; update the formatMessage function to safely serialize data by wrapping JSON.stringify in a try/catch and falling back to a non-throwing serializer (e.g., util.inspect(data, { depth: null }) or a simple replacer that marks circular references) so logging never throws. Locate formatMessage and replace the direct JSON.stringify usage with the safe stringify logic and ensure the returned string still uses the same prefix/msg format when data fallback is used.src/server/namespace-router.ts (1)
30-37: Substring token matching may produce false positives for short tokens.Tokens like
"us","uk", or"ai"(length ≥ 2) will match inside unrelated words (e.g.,"discuss","junk","brain"). Consider using word-boundary matching or raising the minimum token length to reduce noise:- if (token.length >= 2 && q.includes(token)) { + if (token.length >= 3 && q.includes(token)) {Or use a regex word-boundary check for 2-char tokens. This is a heuristic, so it may be acceptable as-is depending on your namespace naming conventions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/namespace-router.ts` around lines 30 - 37, The current token matching loop (using normalizedName -> nameTokens and checking if q.includes(token)) causes false positives for short tokens; update the logic in that loop to either (A) require longer tokens (raise the minimum length from 2 to 3) or (B) perform a word-boundary match for short tokens by checking the query with a word-boundary-aware test for token length == 2 (i.e., only count a match if the token appears as a separate word in q); apply this change where token is evaluated and adjust score and reasons updates (score += 2 and reasons.push(...)) accordingly so short tokens no longer match inside unrelated words.src/server/format-query-result.ts (1)
9-9: Consider exportingDEFAULT_CONTENT_MAX_LENGTH.It's currently module-private. If any tool needs to surface the limit to the caller (e.g., in a tool description or response metadata), it must hardcode
2000independently.♻️ Proposed change
-const DEFAULT_CONTENT_MAX_LENGTH = 2000; +export const DEFAULT_CONTENT_MAX_LENGTH = 2000;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/format-query-result.ts` at line 9, DEFAULT_CONTENT_MAX_LENGTH is currently module-private; export it so external callers can read the configured limit. Change the declaration of DEFAULT_CONTENT_MAX_LENGTH to a named export (e.g., export const DEFAULT_CONTENT_MAX_LENGTH = 2000) and update any barrel/index exports if your project uses them so the symbol is publicly accessible for tools that need to surface or reference the limit.
🤖 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/constants.ts`:
- Around line 34-43: SERVER_INSTRUCTIONS currently references a non-existent
tool named query_documents (also mentions list_namespaces, namespace_router,
suggest_query_params, count, query_fast/query_detailed, generate_urls) which
will confuse LLM clients; fix by either registering/implementing a
query_documents tool with the documented semantics or by removing/updating the
SERVER_INSTRUCTIONS text to stop referencing query_documents and instead point
to the correct existing tool (e.g., query_detailed or a newly added
query_documents) and ensure suggest_query_params and the recommended tool flow
remain consistent with the registered tool names (update mentions of
query_documents at the lines that describe document reassembly and usage step
4).
- Around line 18-22: The three constants DEFAULT_QUERY_DOCUMENTS_TOP_K,
MAX_QUERY_DOCUMENTS_TOP_K, and QUERY_DOCUMENTS_MAX_CHUNKS are dead code tied to
the not-yet-added query-documents-tool; remove these constants from
src/constants.ts and also remove (or stop importing) the reference to the
nonexistent query-documents-tool in src/server.ts, or alternatively move these
constants into the new query-documents-tool module and only add them back when
that module is implemented—ensure no code imports or uses those symbols until
the tool exists.
In `@src/server/tools/guided-query-tool.ts`:
- Around line 140-152: The guided-query path uses FAST_QUERY_FIELDS whenever
suggestion.suggested_fields is empty, causing the detailed mode to omit content;
change the fields selection logic around selectedTool / isFast so that if
selectedTool === 'query_fast' you use FAST_QUERY_FIELDS, but if selectedTool ===
'query_detailed' and suggestion.suggested_fields is empty you pass undefined (or
a fields set that includes chunk_text) so content is returned; update the code
where fields is computed (referencing selectedTool, isFast,
suggestion.suggested_fields and the client.query call that sets fields) to
branch by mode instead of always falling back to FAST_QUERY_FIELDS.
---
Duplicate comments:
In `@src/constants.ts`:
- Around line 12-13: The COUNT_TOP_K constant caps searches at 10,000 which can
cause under-counts; ensure the existing truncated flag computed in count-tool.ts
is propagated to callers by extending the count function's return type to
include a boolean truncated property (e.g., return { total, truncated }), update
the function (e.g., countDocuments / whatever function in count-tool.ts that
performs the Pinecone query) to set truncated = true when results hit
COUNT_TOP_K, and update all callers to inspect this truncated flag so they can
handle or surface the partial result to users.
In `@src/pinecone-client.ts`:
- Around line 327-337: The double-cast on results passed to pc.inference.rerank
(results as unknown as (string | Record<string, string>)[]) hides the actual
MergedHit shape and is unsafe; instead map or cast results to a safe metadata
union type such as Record<string, PineconeMetadataValue>[] (or transform each
MergedHit into either a string or Record<string, PineconeMetadataValue>) before
calling pc.inference.rerank so the compiler sees the real shape; locate the call
to pc.inference.rerank (using this.rerankModel and results) and replace the
double-unknown cast by either a mapping step that converts MergedHit ->
Record<string, PineconeMetadataValue> or a single widened cast to (string |
Record<string, PineconeMetadataValue>)[] so types are preserved.
---
Nitpick comments:
In `@src/logger.ts`:
- Around line 56-71: The logger.error function contains dead commented-out code
from the previous implementation; remove the commented block inside the export
function error so only the active implementation remains (keep the existing
detail construction that uses err instanceof Error and the console.error call
with formatMessage and shouldLog). Ensure you only delete the commented lines
and leave the function signature, the shouldLog check, the detail assignment,
and the console.error(formatMessage(...)) call intact.
- Around line 29-36: formatMessage currently calls JSON.stringify(data) which
will throw on circular structures; update the formatMessage function to safely
serialize data by wrapping JSON.stringify in a try/catch and falling back to a
non-throwing serializer (e.g., util.inspect(data, { depth: null }) or a simple
replacer that marks circular references) so logging never throws. Locate
formatMessage and replace the direct JSON.stringify usage with the safe
stringify logic and ensure the returned string still uses the same prefix/msg
format when data fallback is used.
In `@src/server/format-query-result.ts`:
- Line 9: DEFAULT_CONTENT_MAX_LENGTH is currently module-private; export it so
external callers can read the configured limit. Change the declaration of
DEFAULT_CONTENT_MAX_LENGTH to a named export (e.g., export const
DEFAULT_CONTENT_MAX_LENGTH = 2000) and update any barrel/index exports if your
project uses them so the symbol is publicly accessible for tools that need to
surface or reference the limit.
In `@src/server/namespace-router.ts`:
- Around line 30-37: The current token matching loop (using normalizedName ->
nameTokens and checking if q.includes(token)) causes false positives for short
tokens; update the logic in that loop to either (A) require longer tokens (raise
the minimum length from 2 to 3) or (B) perform a word-boundary match for short
tokens by checking the query with a word-boundary-aware test for token length ==
2 (i.e., only count a match if the token appears as a separate word in q); apply
this change where token is evaluated and adjust score and reasons updates (score
+= 2 and reasons.push(...)) accordingly so short tokens no longer match inside
unrelated words.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/server/tools/query-documents-tool.ts (2)
106-119:metadata_filter ?? undefinedis a no-op — simplify.On line 110,
metadata_filter ?? undefineddoesn't change behavior sinceundefined ?? undefinedis stillundefined. Use the variable directly.♻️ Suggested cleanup
return jsonResponse({ status: 'success', query: query_text.trim(), namespace, - metadata_filter: metadata_filter ?? undefined, + metadata_filter, result_count: topDocuments.length,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tools/query-documents-tool.ts` around lines 106 - 119, The metadata_filter property in the jsonResponse call is using a redundant nullish coalescing (metadata_filter ?? undefined); remove the no-op and pass the variable directly by changing the payload to use metadata_filter (e.g., in the return block inside query-documents-tool where jsonResponse(...) is built for topDocuments). No other behavioral changes are needed—just replace the expression with metadata_filter to simplify the code.
84-96:fields: undefinedis unnecessary — consider omitting the property entirely.Passing
fields: undefinedexplicitly in the query options object is redundant and slightly obscures intent. If the client'squerymethod accepts an optionalfieldsproperty, simply omit it.♻️ Suggested cleanup
const results = await client.query({ query: query_text.trim(), topK: chunkLimit, namespace, useReranking: true, metadataFilter: metadata_filter, - fields: undefined, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tools/query-documents-tool.ts` around lines 84 - 96, Remove the redundant fields: undefined property from the options passed to client.query in query-documents-tool.ts; locate the call that uses getPineconeClient() and client.query({ query: query_text.trim(), topK: chunkLimit, namespace, useReranking: true, metadataFilter: metadata_filter, fields: undefined }) and simply omit the fields key so the object only includes query, topK, namespace, useReranking and metadataFilter (keeping QUERY_DOCUMENTS_MAX_CHUNKS and CHUNKS_PER_DOCUMENT logic intact).src/server/reassemble-documents.test.ts (1)
5-87: Consider adding a few more edge-case tests for stronger coverage.The current suite doesn't cover:
best_scorecomputation (e.g., verifyMath.roundrounding to 4 decimals).- Fallback to
urlas the document key (currently onlydocument_numberanddoc_idare tested).- Custom
contentSeparatoroption.- Empty
resultsarray (should return[]).- Chunks with no ordering metadata (should preserve insertion order).
These are non-blocking but would strengthen confidence in the reassembly logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/reassemble-documents.test.ts` around lines 5 - 87, Tests for reassembleByDocument are missing edge cases: add unit tests that (1) assert best_score is computed and rounded to 4 decimals for a document (verify Math.round behavior), (2) verify fallback to metadata.url when document_number and doc_id are absent, (3) pass a custom contentSeparator option and assert merged_content uses it, (4) assert an empty results array returns [] and (5) include chunks without chunk_index to confirm insertion order is preserved; locate and extend the existing reassembleByDocument tests (references: function reassembleByDocument, option names maxChunksPerDocument and contentSeparator, and properties best_score/document_id/merged_content) to add these cases.src/server/reassemble-documents.ts (1)
26-33:locmetadata from LangChain is typically an object, not a number — it will be harmlessly skipped.The
lockey inCHUNK_ORDER_KEYSwon't match LangChain'sRecursiveCharacterTextSplitteroutput wherelocis typically{ lines: { from, to } }. This is fine since the type guards reject non-numeric values, but you could consider removing it to avoid confusion, or adding a comment noting it's there for custom schemas that store a numericloc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/reassemble-documents.ts` around lines 26 - 33, The CHUNK_ORDER_KEYS includes "loc" which is typically an object from LangChain (e.g., { lines: { from, to } }), so getChunkOrder's numeric checks silently skip it and that causes confusion; update the code by either removing "loc" from CHUNK_ORDER_KEYS or adding a clear comment next to CHUNK_ORDER_KEYS explaining that "loc" is only kept for custom schemas that store a numeric loc value and will be ignored for typical LangChain objects, and ensure getChunkOrder remains as-is to only accept numeric/string numeric values; reference CHUNK_ORDER_KEYS and the getChunkOrder function when making the change.
🤖 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/query-documents-tool.ts`:
- Around line 18-128: Prettier formatting errors were flagged in the
registerQueryDocumentsTool implementation (the exported function and its
server.registerTool callback); fix by running your formatter (e.g. prettier
--write) on this file or applying the project's Prettier config so the function
signature, object literal (inputSchema), and async callback are properly
formatted; re-run CI/lint to confirm the formatting violations are resolved
before pushing.
- Around line 15-16: Update the CHUNKS_PER_DOCUMENT comment to explicitly call
out the recall vs. performance trade-off: explain that CHUNKS_PER_DOCUMENT
(currently 50) is a heuristic to limit fetched chunks for performance, may be
insufficient for very large or finely-split documents, and that callers can
override per-call via the max_chunks_per_document parameter (default 200) when
higher recall is required; mention that increasing CHUNKS_PER_DOCUMENT raises
latency and memory usage during reassembly.
---
Nitpick comments:
In `@src/server/reassemble-documents.test.ts`:
- Around line 5-87: Tests for reassembleByDocument are missing edge cases: add
unit tests that (1) assert best_score is computed and rounded to 4 decimals for
a document (verify Math.round behavior), (2) verify fallback to metadata.url
when document_number and doc_id are absent, (3) pass a custom contentSeparator
option and assert merged_content uses it, (4) assert an empty results array
returns [] and (5) include chunks without chunk_index to confirm insertion order
is preserved; locate and extend the existing reassembleByDocument tests
(references: function reassembleByDocument, option names maxChunksPerDocument
and contentSeparator, and properties best_score/document_id/merged_content) to
add these cases.
In `@src/server/reassemble-documents.ts`:
- Around line 26-33: The CHUNK_ORDER_KEYS includes "loc" which is typically an
object from LangChain (e.g., { lines: { from, to } }), so getChunkOrder's
numeric checks silently skip it and that causes confusion; update the code by
either removing "loc" from CHUNK_ORDER_KEYS or adding a clear comment next to
CHUNK_ORDER_KEYS explaining that "loc" is only kept for custom schemas that
store a numeric loc value and will be ignored for typical LangChain objects, and
ensure getChunkOrder remains as-is to only accept numeric/string numeric values;
reference CHUNK_ORDER_KEYS and the getChunkOrder function when making the
change.
In `@src/server/tools/query-documents-tool.ts`:
- Around line 106-119: The metadata_filter property in the jsonResponse call is
using a redundant nullish coalescing (metadata_filter ?? undefined); remove the
no-op and pass the variable directly by changing the payload to use
metadata_filter (e.g., in the return block inside query-documents-tool where
jsonResponse(...) is built for topDocuments). No other behavioral changes are
needed—just replace the expression with metadata_filter to simplify the code.
- Around line 84-96: Remove the redundant fields: undefined property from the
options passed to client.query in query-documents-tool.ts; locate the call that
uses getPineconeClient() and client.query({ query: query_text.trim(), topK:
chunkLimit, namespace, useReranking: true, metadataFilter: metadata_filter,
fields: undefined }) and simply omit the fields key so the object only includes
query, topK, namespace, useReranking and metadataFilter (keeping
QUERY_DOCUMENTS_MAX_CHUNKS and CHUNKS_PER_DOCUMENT logic intact).
…nreleased] and COUNT_TOP_K JSDoc Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/pinecone-client.ts (1)
156-156: Consider a graceful guard whenstats.dimensionis unavailable instead of hardcoding 1536.The zero-vector dimension fallback
1536is the OpenAI Ada-002 size. Indexes backed by other embedding models (e.g.,text-embedding-3-largeat 3072 ormultilingual-e5-largeat 384) will emit a dimension-mismatch API error, which is then silently swallowed by the innercatchat line 180 — leavingmetadataFieldsas an empty object with no visible indication beyond alogErrorline.♻️ Suggested improvement
- const sampleQuery = - typeof nsObj.query === 'function' - ? await nsObj.query({ - topK: 5, - vector: Array(stats?.dimension ?? 1536).fill(0), - includeMetadata: true, - }) - : { matches: undefined }; + const dimension = stats?.dimension; + const sampleQuery = + typeof nsObj.query === 'function' && dimension !== undefined + ? await nsObj.query({ + topK: 5, + vector: Array(dimension).fill(0), + includeMetadata: true, + }) + : { matches: undefined };Skipping the sample when dimension is unknown is safer than sending a wrong-size zero vector that always errors; the inner
catchalready handles the{ matches: undefined }path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pinecone-client.ts` at line 156, The code currently uses a hardcoded fallback Array(stats?.dimension ?? 1536).fill(0) which can produce wrong-size vectors; instead, guard on stats?.dimension and do not emit a zero vector when dimension is unavailable. Update the logic around building the sample (the place using stats.dimension and where metadataFields is later set) to only include a vector if stats?.dimension is a positive number (or derive dimension from a validated embedding model config if available); otherwise skip adding the vector (or skip the sample) so the inner catch-path that handles { matches: undefined } remains the fallback and you avoid silent dimension-mismatch errors.src/constants.ts (1)
10-53: LGTM – all previous review threads are resolved.
COUNT_TOP_KJSDoc now documents thetruncated: truesentinel (lines 12–15).query_documents-related constants are no longer dead code now that the tool is implemented.SERVER_INSTRUCTIONSaccurately reflects the implemented tool set.One minor nit: line 50 hard-codes
"30 minutes"whileFLOW_CACHE_TTL_MS(line 11) is the authoritative source. If the TTL is ever adjusted, the instruction string silently becomes stale. Consider deriving it, e.g.`cached for ${FLOW_CACHE_TTL_MS / 60_000} minutes`, or at least add a comment cross-referencing the constant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/constants.ts` around lines 10 - 53, The SERVER_INSTRUCTIONS string hardcodes "cached for 30 minutes" which can drift from the authoritative FLOW_CACHE_TTL_MS; update SERVER_INSTRUCTIONS to derive the minutes from FLOW_CACHE_TTL_MS (e.g., interpolate FLOW_CACHE_TTL_MS / 60_000) or at minimum add a comment next to SERVER_INSTRUCTIONS referencing FLOW_CACHE_TTL_MS so future changes stay consistent; locate the constant FLOW_CACHE_TTL_MS and the SERVER_INSTRUCTIONS export and replace the hardcoded text with a computed minutes value or add the cross-reference comment.src/server/reassemble-documents.ts (1)
77-106: Output documents are returned in map-insertion order; sort bybest_scorefor deterministic rankingDocuments are pushed to
outin the insertion order of thebyDocMap, which tracks the order each document's first chunk appears inresults. Because later chunks of a document can yield a higher score than its first chunk, the effective ranking bybest_scoreis not guaranteed. Sorting the final array descending bybest_scorewould give callers a consistently ranked result and make downstream slicing (e.g., returning the top-N documents from a larger chunk pool) predictable.♻️ Proposed fix – sort output by best_score descending
- return out; + return out.sort((a, b) => b.best_score - a.best_score); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/reassemble-documents.ts` around lines 77 - 106, The output array `out` is built in iteration order of the `byDoc` Map so final ranking is non-deterministic; after the loop that constructs ReassembledDocument entries (using variables like byDoc, toMerge, merged_content, best_score, maxChunks, getChunkOrder) sort `out` in-place descending by the numeric `best_score` field before returning it so callers receive a consistently ranked list (highest best_score first) for predictable downstream slicing.src/server/metadata-filter.ts (1)
5-14: Zod schema and manual validator accept different shapes — mixed-primitive arrays expose the gap
isPrimitiveArrayat line 32 accepts any array whose every element passesisPrimitiveFilterValue, so a mixed array like["a", 1, true]returnsnull(valid) fromvalidateMetadataFilterValue. The ZodmetadataFilterSchemawould reject the same value becausez.union([z.array(z.string()), z.array(z.number())])requires a homogeneous type. If the two exports are used at different call sites, a mixed-type$inarray can silently bypass Zod-based tool-parameter validation and reach Pinecone, which will likely surface a runtime error.If the Zod schema is only for structural typing and
validateMetadataFilteris the sole validation gate, consider tighteningisPrimitiveArrayto enforce homogeneous types, or add an explicit note in the JSDoc indicating the two APIs have different semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/metadata-filter.ts` around lines 5 - 14, The manual validator allows mixed-type arrays while the Zod schema requires homogeneous arrays; update isPrimitiveArray (used by validateMetadataFilterValue) to enforce homogeneity by checking that every element passes isPrimitiveFilterValue AND all elements share the same primitive type/category (e.g., all strings, all numbers, or all booleans) so mixed arrays like ["a", 1, true] are rejected, and keep metadataFilterValueSchema and validateMetadataFilterValue semantics aligned; reference the functions isPrimitiveArray, isPrimitiveFilterValue, validateMetadataFilterValue and the Zod symbol metadataFilterValueSchema when making the change (or alternatively add a clear JSDoc note in validateMetadataFilterValue if you intentionally allow mixed arrays).
🤖 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 458-468: The count() method lacks the empty-query validation
present in query(), so blank or whitespace-only params.query can reach
searchIndex and trigger opaque Pinecone errors; update count() (in
src/pinecone-client.ts) to mirror query() by trimming params.query and
rejecting/throwing a clear error when it's empty or only whitespace before
calling ensureIndexes()/searchIndex, using the same error message/guard behavior
as query() to keep behavior consistent.
In `@src/server/reassemble-documents.ts`:
- Around line 60-62: Clamp the user-provided maxChunksPerDocument to a sensible
lower bound so we never pass 0 into the merge logic: replace the current
maxChunks assignment with a guarded/coerced value (e.g. const maxChunks =
Math.max(1, Math.floor(options?.maxChunksPerDocument ?? 200))); this ensures
variables like toMerge, merged_content and best_score calculations (where
Math.max is used) won't operate on an empty set; locate and update the maxChunks
computation in the function that returns ReassembledDocument[] (the line using
options?.maxChunksPerDocument) and ensure the rest of the logic uses this
clamped maxChunks.
- Around line 9-10: The CHUNK_ORDER_KEYS constant incorrectly lists 'loc' and
attributes it to RecursiveCharacterTextSplitter; update CHUNK_ORDER_KEYS in
reassemble-documents.ts to remove 'loc' and replace it with the numeric
'start_index' (the field added when addStartIndex: true) or, if you prefer to
keep 'loc', change the comment to state that 'loc' is produced by LangChain
loaders and must be serialized to a numeric value before using for ordering;
modify the constant and accompanying comment near CHUNK_ORDER_KEYS accordingly
so code and comment are consistent with LangChain/RecursiveCharacterTextSplitter
behavior.
- Around line 12-24: getDocumentKey currently falls back to hit.id when no
string document_number, url, or doc_id are present, causing each chunk without
those metadata keys to be treated as its own document; update the JSDoc for
getDocumentKey to explicitly state this fallback behavior (that it will return
hit.id when no other document identifier metadata exists) and also update the
ReassembledDocument.document_id JSDoc/description to mention that it may equal
the raw vector ID when source chunks lack grouping metadata so callers can
surface diagnostics or skip untagged namespaces.
---
Duplicate comments:
In `@src/pinecone-client.ts`:
- Around line 332-342: The rerank call uses a unsafe double-cast "results as
unknown as (string | Record<string, string>)[]"—remove that cast and supply
properly typed items to pc.inference.rerank instead: either change the
declaration/type of the existing results variable to match the expected (string
| Record<string,string>)[] shape or map results into a new array of that shape
(e.g., map each result to a string or an object with the chunk_text field used
by rankFields) before calling pc.inference.rerank with this.rerankModel, query,
and the mapped array; ensure types line up at compile time rather than using
unknown casts.
---
Nitpick comments:
In `@src/constants.ts`:
- Around line 10-53: The SERVER_INSTRUCTIONS string hardcodes "cached for 30
minutes" which can drift from the authoritative FLOW_CACHE_TTL_MS; update
SERVER_INSTRUCTIONS to derive the minutes from FLOW_CACHE_TTL_MS (e.g.,
interpolate FLOW_CACHE_TTL_MS / 60_000) or at minimum add a comment next to
SERVER_INSTRUCTIONS referencing FLOW_CACHE_TTL_MS so future changes stay
consistent; locate the constant FLOW_CACHE_TTL_MS and the SERVER_INSTRUCTIONS
export and replace the hardcoded text with a computed minutes value or add the
cross-reference comment.
In `@src/pinecone-client.ts`:
- Line 156: The code currently uses a hardcoded fallback Array(stats?.dimension
?? 1536).fill(0) which can produce wrong-size vectors; instead, guard on
stats?.dimension and do not emit a zero vector when dimension is unavailable.
Update the logic around building the sample (the place using stats.dimension and
where metadataFields is later set) to only include a vector if stats?.dimension
is a positive number (or derive dimension from a validated embedding model
config if available); otherwise skip adding the vector (or skip the sample) so
the inner catch-path that handles { matches: undefined } remains the fallback
and you avoid silent dimension-mismatch errors.
In `@src/server/metadata-filter.ts`:
- Around line 5-14: The manual validator allows mixed-type arrays while the Zod
schema requires homogeneous arrays; update isPrimitiveArray (used by
validateMetadataFilterValue) to enforce homogeneity by checking that every
element passes isPrimitiveFilterValue AND all elements share the same primitive
type/category (e.g., all strings, all numbers, or all booleans) so mixed arrays
like ["a", 1, true] are rejected, and keep metadataFilterValueSchema and
validateMetadataFilterValue semantics aligned; reference the functions
isPrimitiveArray, isPrimitiveFilterValue, validateMetadataFilterValue and the
Zod symbol metadataFilterValueSchema when making the change (or alternatively
add a clear JSDoc note in validateMetadataFilterValue if you intentionally allow
mixed arrays).
In `@src/server/reassemble-documents.ts`:
- Around line 77-106: The output array `out` is built in iteration order of the
`byDoc` Map so final ranking is non-deterministic; after the loop that
constructs ReassembledDocument entries (using variables like byDoc, toMerge,
merged_content, best_score, maxChunks, getChunkOrder) sort `out` in-place
descending by the numeric `best_score` field before returning it so callers
receive a consistently ranked list (highest best_score first) for predictable
downstream slicing.
…k cast, fix fields nullable Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/server/tools/guided-query-tool.ts (1)
79-79: Hardcoded3for ranked namespace count — consider extracting to a named constant.
rankNamespacesByQuery(queryText, namespaces, 3)— onlyranked[0]is used for namespace selection; the other two appear solely indecision_trace.ranked_namespaces. Extracting3to e.g.GUIDED_QUERY_RANKED_NAMESPACES = 3inconstants.tswould make the intent clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tools/guided-query-tool.ts` at line 79, Replace the hardcoded literal 3 in the call to rankNamespacesByQuery(queryText, namespaces, 3) with a named constant (e.g. GUIDED_QUERY_RANKED_NAMESPACES = 3) defined in your constants module; update guided-query-tool.ts to import GUIDED_QUERY_RANKED_NAMESPACES and pass it to rankNamespacesByQuery, and keep using ranked[0] for namespace selection while still populating decision_trace.ranked_namespaces with the full ranked array.src/server/tools/query-documents-tool.ts (1)
113-113: Redundant?? undefined—metadata_filteris alreadyundefinedwhen absent.- metadata_filter: metadata_filter ?? undefined, + metadata_filter,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tools/query-documents-tool.ts` at line 113, The property assignment uses a redundant nullish coalescing ("metadata_filter: metadata_filter ?? undefined") — simply remove "?? undefined" and assign the value directly (e.g., "metadata_filter: metadata_filter" or use the shorthand "metadata_filter") so the property will already be undefined when absent; update the object construction that contains the metadata_filter property accordingly.
🤖 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 497-508: The per-hit logWarn inside the count() loop (the call to
logWarn that references hit._id, params.namespace and COUNT_FIELDS) is redundant
and can emit up to COUNT_TOP_K warnings; remove that per-hit warning (or change
it to logDebug/logTrace) and rely on the aggregate warning that uses
idFallbackCount, params.namespace and COUNT_FIELDS to report the condition;
ensure only the aggregate warning remains so count() no longer floods logs.
---
Nitpick comments:
In `@src/server/tools/guided-query-tool.ts`:
- Line 79: Replace the hardcoded literal 3 in the call to
rankNamespacesByQuery(queryText, namespaces, 3) with a named constant (e.g.
GUIDED_QUERY_RANKED_NAMESPACES = 3) defined in your constants module; update
guided-query-tool.ts to import GUIDED_QUERY_RANKED_NAMESPACES and pass it to
rankNamespacesByQuery, and keep using ranked[0] for namespace selection while
still populating decision_trace.ranked_namespaces with the full ranked array.
In `@src/server/tools/query-documents-tool.ts`:
- Line 113: The property assignment uses a redundant nullish coalescing
("metadata_filter: metadata_filter ?? undefined") — simply remove "?? undefined"
and assign the value directly (e.g., "metadata_filter: metadata_filter" or use
the shorthand "metadata_filter") so the property will already be undefined when
absent; update the object construction that contains the metadata_filter
property accordingly.
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/pinecone-client.ts (1)
67-75:⚠️ Potential issue | 🟡 MinorGuard defaultTopK against invalid env/config values.
parseIntcan yieldNaN; later comparisons skip and propagate an invalid topK.🛠️ Proposed fix
this.rerankModel = config.rerankModel || process.env['PINECONE_RERANK_MODEL'] || DEFAULT_RERANK_MODEL; - this.defaultTopK = - config.defaultTopK || parseInt(process.env['PINECONE_TOP_K'] || String(DEFAULT_TOP_K)); + const envTopK = parseInt(process.env['PINECONE_TOP_K'] || '', 10); + const resolvedTopK = + typeof config.defaultTopK === 'number' && Number.isFinite(config.defaultTopK) + ? config.defaultTopK + : Number.isFinite(envTopK) + ? envTopK + : DEFAULT_TOP_K; + this.defaultTopK = resolvedTopK; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pinecone-client.ts` around lines 67 - 75, The constructor currently sets this.defaultTopK using parseInt which can produce NaN; update the PineconeClient constructor to validate the parsed value from config.defaultTopK or process.env['PINECONE_TOP_K'] and fall back to DEFAULT_TOP_K when the result is NaN or otherwise invalid (e.g., non-positive). Specifically, when initializing defaultTopK in the constructor, attempt to parse and coerce to a safe integer, check Number.isFinite/Number.isInteger and > 0, and if the check fails assign DEFAULT_TOP_K; keep references to the same symbols (constructor, this.defaultTopK, PINECONE_TOP_K, DEFAULT_TOP_K).
🧹 Nitpick comments (2)
src/server/url-generation.ts (1)
22-35: URL‑encode mailing doc_id/thread_id.If doc_id/thread_id contains special characters, the generated URL can be invalid; encode the path segment.
♻️ Proposed fix
return { - url: `https://lists.boost.org/archives/list/${docIdOrThread}/`, + url: `https://lists.boost.org/archives/list/${encodeURIComponent(docIdOrThread)}/`, method: 'generated.mailing', }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/url-generation.ts` around lines 22 - 35, The mailing URL generator (function generatorMailing) currently inserts docIdOrThread raw into the path which can break URLs if it contains special characters; update generatorMailing to URL-encode the path segment (use encodeURIComponent on docIdOrThread) before interpolating into the returned url so the returned url is valid while keeping the same result shape and method 'generated.mailing'.src/server/metadata-filter.ts (1)
33-35: Align primitive‑array rules between schema and runtime validation.
metadataFilterValueSchemaonly allows string/number arrays, butisPrimitiveArrayalso accepts boolean arrays. This can yield inconsistent accept/reject behavior.♻️ One way to align with the current schema
-/** True if value is an array of primitives (required for $in/$nin). */ +/** True if value is an array of string/number (required for $in/$nin). */ function isPrimitiveArray(value: unknown): boolean { - return Array.isArray(value) && value.every((item) => isPrimitiveFilterValue(item)); + return ( + Array.isArray(value) && + value.every((item) => typeof item === 'string' || typeof item === 'number') + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/metadata-filter.ts` around lines 33 - 35, The runtime check in isPrimitiveArray currently permits boolean arrays but the schema metadataFilterValueSchema only allows string and number arrays, causing inconsistencies; update isPrimitiveArray (and/or isPrimitiveFilterValue if it currently returns true for booleans) so it only returns true for arrays whose items are strings or numbers (i.e., reject booleans) to match metadataFilterValueSchema, ensuring both runtime and schema validation accept the same primitive types.
🤖 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/metadata-filter.ts`:
- Around line 52-77: validateMetadataFilterValue currently allows nested objects
with non-operator keys (e.g., { field: { foo: 1 } }) because the loop only
enforces operator rules for keys that start with '$'; update the loop in
validateMetadataFilterValue to reject any nested object where a key does not
start with '$' by returning an error like `Unsupported nested filter key "foo"
at "path"`, while keeping existing checks for ALLOWED_FILTER_OPERATORS, $in/$nin
array checks via isPrimitiveArray, and primitive checks via
isPrimitiveFilterValue; reference the function validateMetadataFilterValue, the
ALLOWED_FILTER_OPERATORS set, and helper predicates
isPrimitiveArray/isPrimitiveFilterValue when implementing the new rejection
condition.
In `@src/server/namespace-router.ts`:
- Around line 54-76: The function rankNamespacesByQuery should guard against
non‑positive or out‑of-range topN values; before using slice, normalize topN (in
rankNamespacesByQuery) to a valid range e.g. let limit = Math.min(Math.max(1,
topN), namespaces.length) and then use .slice(0, limit) so you never pass
0/negative or larger-than-available values to slice; update references in the
function where slice(0, topN) is used to slice(0, limit).
---
Outside diff comments:
In `@src/pinecone-client.ts`:
- Around line 67-75: The constructor currently sets this.defaultTopK using
parseInt which can produce NaN; update the PineconeClient constructor to
validate the parsed value from config.defaultTopK or
process.env['PINECONE_TOP_K'] and fall back to DEFAULT_TOP_K when the result is
NaN or otherwise invalid (e.g., non-positive). Specifically, when initializing
defaultTopK in the constructor, attempt to parse and coerce to a safe integer,
check Number.isFinite/Number.isInteger and > 0, and if the check fails assign
DEFAULT_TOP_K; keep references to the same symbols (constructor,
this.defaultTopK, PINECONE_TOP_K, DEFAULT_TOP_K).
---
Duplicate comments:
In `@src/server/reassemble-documents.ts`:
- Around line 9-10: The comment above CHUNK_ORDER_KEYS is misleading about the
provenance of the keys; update the comment for the constant CHUNK_ORDER_KEYS to
clearly state which keys come from text splitters (e.g., 'chunk_index' and
'start_index' often from RecursiveCharacterTextSplitter and similar splitters)
and which are produced by other tools/parsers (e.g., 'loc' may come from some
PDF/HTML parsers or upstream document loaders), or simply note that these are
common keys across various splitters and loaders used for chunk ordering so
readers won’t assume all originate from RecursiveCharacterTextSplitter.
---
Nitpick comments:
In `@src/server/metadata-filter.ts`:
- Around line 33-35: The runtime check in isPrimitiveArray currently permits
boolean arrays but the schema metadataFilterValueSchema only allows string and
number arrays, causing inconsistencies; update isPrimitiveArray (and/or
isPrimitiveFilterValue if it currently returns true for booleans) so it only
returns true for arrays whose items are strings or numbers (i.e., reject
booleans) to match metadataFilterValueSchema, ensuring both runtime and schema
validation accept the same primitive types.
In `@src/server/url-generation.ts`:
- Around line 22-35: The mailing URL generator (function generatorMailing)
currently inserts docIdOrThread raw into the path which can break URLs if it
contains special characters; update generatorMailing to URL-encode the path
segment (use encodeURIComponent on docIdOrThread) before interpolating into the
returned url so the returned url is valid while keeping the same result shape
and method 'generated.mailing'.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/server/url-generation.ts (1)
89-92:registerUrlGeneratorsilently overwrites built-in generators.Calling
registerUrlGenerator('mailing', customFn)would replace the built-in without any warning. Consider guarding against accidental overwrites of built-in generators — either by logging a warning or by throwing if the namespace already has a registered generator (with a separateoverrideUrlGeneratorfor intentional replacements).♻️ Suggested guard
export function registerUrlGenerator(namespace: string, fn: UrlGenerator): void { + if (urlGenerators.has(namespace)) { + console.warn(`[url-generation] Overwriting existing URL generator for namespace "${namespace}"`); + } urlGenerators.set(namespace, fn); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/url-generation.ts` around lines 89 - 92, registerUrlGenerator currently overwrites existing entries silently; update it to guard against accidental replacement by checking urlGenerators.has(namespace) and if a generator already exists either throw a clear Error (e.g., "URL generator for '{namespace}' already registered; use overrideUrlGenerator to replace") or log a warning and return, and add a separate overrideUrlGenerator(namespace, fn) that unconditionally sets urlGenerators.set(namespace, fn); reference the registerUrlGenerator function and the urlGenerators Map when making this change.src/server/namespace-router.ts (2)
31-37: Repeated namespace tokens inflate the score.If a namespace name contains repeated tokens after normalization (e.g.,
"ab-ab"→ tokens["ab", "ab"]), each duplicate token earns an independent+2, whilereasonscorrectly deduplicates via theSet. The score ends up higher than intended for such namespaces.A
SetovernameTokensbefore iterating would keep scoring consistent with reason deduplication:♻️ Proposed fix
- const nameTokens = normalizedName.split(/\s+/).filter(Boolean); + const nameTokens = [...new Set(normalizedName.split(/\s+/).filter(Boolean))]; for (const token of nameTokens) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/namespace-router.ts` around lines 31 - 37, The loop over nameTokens in namespace scoring can double-count duplicate normalized tokens (e.g., "ab-ab"); change the iteration to deduplicate nameTokens first (use a Set on nameTokens) before checking tokens against q so each unique token can only add its +2 once and reasons remain consistent; update the block that references normalizedName, nameTokens, q, score, and reasons to iterate over the unique token set instead of the original array.
3-8: Consider exportingRankedNamespace.
RankedNamespaceis the return element type of the publicrankNamespacesByQuery, but it is not exported. Callers who want to annotate or pass around the ranked results must resort toReturnType<typeof rankNamespacesByQuery>[number]instead of naming it directly.♻️ Proposed change
-type RankedNamespace = { +export type RankedNamespace = { namespace: string; score: number; record_count: number; reasons: string[]; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/namespace-router.ts` around lines 3 - 8, The type RankedNamespace is used as the public return element of rankNamespacesByQuery but is not exported; export it so callers can import and annotate results directly. Change the declaration of RankedNamespace to an exported type (export type RankedNamespace = { ... }) and ensure any file-level or barrel exports (if present) re-export it as needed so external callers using rankNamespacesByQuery can reference RankedNamespace by name.
🤖 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/metadata-filter.ts`:
- Line 1: Prettier is flagging formatting issues in this file; run the code
formatter on src/server/metadata-filter.ts (for example `prettier --write
src/server/metadata-filter.ts`) to fix whitespace/formatting around the import
statement (import { z } from 'zod';) and any other style violations so CI
passes; commit the reformatted file.
- Around line 34-36: The function isPrimitiveArray currently only treats string
items as primitive, causing numeric arrays (e.g., { "year": { "$in": [2019,
2020] } }) to be rejected; update isPrimitiveArray to accept number items as
well (or use a broader primitive check for typeof item === 'string' || typeof
item === 'number') so it aligns with the Zod schema (z.array(z.number())) and
stops triggering the "must use an array of primitive values" validation error;
make sure callers (the filter validation that raises that error) rely on the
revised isPrimitiveArray behavior.
- Around line 5-26: The schema and runtime validator reject Pinecone logical
operators $and/$or: update metadataFilterValueSchema (the z.lazy union) to
include z.array(metadataFilterSchema) so arrays of filter objects are allowed,
add '$and' and '$or' to ALLOWED_FILTER_OPERATORS, and modify
validateMetadataFilterValue to accept arrays of filter objects (i.e., check for
Array.isArray(value) as a valid branch that validates each element against
metadataFilterSchema) before treating arrays as unsupported.
In `@src/server/namespace-router.ts`:
- Line 59: topN can be NaN and Math.max(1, Math.floor(topN)) will produce NaN;
update the computation of limit to first coerce/validate topN (e.g., let
parsedTopN = Number(topN); if (!Number.isFinite(parsedTopN)) parsedTopN = 1;)
and then compute limit = Math.max(1, Math.floor(parsedTopN)); apply this change
where limit and topN are used in namespace-router.ts so slice(0, limit) never
receives NaN.
---
Nitpick comments:
In `@src/server/namespace-router.ts`:
- Around line 31-37: The loop over nameTokens in namespace scoring can
double-count duplicate normalized tokens (e.g., "ab-ab"); change the iteration
to deduplicate nameTokens first (use a Set on nameTokens) before checking tokens
against q so each unique token can only add its +2 once and reasons remain
consistent; update the block that references normalizedName, nameTokens, q,
score, and reasons to iterate over the unique token set instead of the original
array.
- Around line 3-8: The type RankedNamespace is used as the public return element
of rankNamespacesByQuery but is not exported; export it so callers can import
and annotate results directly. Change the declaration of RankedNamespace to an
exported type (export type RankedNamespace = { ... }) and ensure any file-level
or barrel exports (if present) re-export it as needed so external callers using
rankNamespacesByQuery can reference RankedNamespace by name.
In `@src/server/url-generation.ts`:
- Around line 89-92: registerUrlGenerator currently overwrites existing entries
silently; update it to guard against accidental replacement by checking
urlGenerators.has(namespace) and if a generator already exists either throw a
clear Error (e.g., "URL generator for '{namespace}' already registered; use
overrideUrlGenerator to replace") or log a warning and return, and add a
separate overrideUrlGenerator(namespace, fn) that unconditionally sets
urlGenerators.set(namespace, fn); reference the registerUrlGenerator function
and the urlGenerators Map when making this change.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/server/namespace-router.ts (1)
40-44: Consider adding a minimum-length guard for field names, consistent with the token check.Namespace tokens already require
token.length >= 2(Line 33), but field names have no such guard. Very short metadata field names (e.g.,"id","at","to") can produce false-positive substring matches — for example,"id"hits inside"hide","at"hits inside"data"or"what".♻️ Proposed fix
for (const field of fields) { - if (q.includes(field.toLowerCase())) { + const lf = field.toLowerCase(); + if (lf.length >= 3 && q.includes(lf)) { score += 1; reasons.push(`field hint: ${field}`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/namespace-router.ts` around lines 40 - 44, The field-matching loop in namespace-router.ts increments score for any field substring match without a minimum-length guard, causing false positives for very short names; update the for (const field of fields) { ... } block to skip fields shorter than the token minimum (match the existing token.length >= 2 rule) — e.g., check field.length >= 2 (or reuse the same minimum constant if available) before performing q.includes(field.toLowerCase()), so only fields meeting the minimum length contribute to score and reasons.
🤖 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/server/namespace-router.ts`:
- Around line 40-44: The field-matching loop in namespace-router.ts increments
score for any field substring match without a minimum-length guard, causing
false positives for very short names; update the for (const field of fields) {
... } block to skip fields shorter than the token minimum (match the existing
token.length >= 2 rule) — e.g., check field.length >= 2 (or reuse the same
minimum constant if available) before performing
q.includes(field.toLowerCase()), so only fields meeting the minimum length
contribute to score and reasons.
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (22)
src/pinecone-client.test.ts (1)
132-133: Side-effect assertion inside a mock will surface as an opaque promise rejection.
expect(options?.fields).toEqual(...)placed inside thesearchIndexstub body throws a Vitest assertion error that propagates as an unhandled rejection rather than a named test failure. Prefer verifyingfieldsafterclient.count()resolves, e.g. by capturing the received options externally.♻️ Proposed approach
+ let capturedFields: string[] | undefined; testClient.searchIndex = async (_index, _query, _topK, _ns, _filter, options) => { - expect(options?.fields).toEqual(['document_number', 'url', 'doc_id']); + capturedFields = options?.fields; return [ /* hits */ ]; }; const result = await client.count({ ... }); + expect(capturedFields).toEqual(['document_number', 'url', 'doc_id']); expect(result.count).toBe(2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pinecone-client.test.ts` around lines 132 - 133, The mock for testClient.searchIndex currently contains an inline expect which causes assertion failures to surface as unhandled promise rejections; instead, capture the incoming options into a variable in the test scope (e.g. let receivedOptions) inside the testClient.searchIndex stub and return the stubbed response, then call the code under test (client.count()), await its resolution, and assert on receivedOptions.fields (expect(receivedOptions?.fields).toEqual([...])) after the promise resolves; update references to testClient.searchIndex and client.count() in the test to implement this external-capture-and-assert pattern.src/logger.ts (1)
66-69: Remove stale commented-out code.Lines 66–69 are a superseded implementation left as dead code. Remove them to keep the module clean.
♻️ Proposed change
export function error(msg: string, err?: unknown): void { if (shouldLog('ERROR')) { - // const detail = err instanceof Error ? err.message : err !== undefined ? String(err) : undefined; - // console.error( - // formatMessage('ERROR', msg, detail !== undefined ? { error: detail } : undefined) - // ); const detail =🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/logger.ts` around lines 66 - 69, Remove the stale commented-out block in the logger module (the three commented lines that compute "detail" and call console.error with formatMessage) to clean up dead code; specifically delete the commented lines shown in the diff so only the active logging implementation remains (locate the commented block near the formatMessage/console.error usage in logger.ts and remove it).package.json (2)
40-40: Optional: prefernpx tscover the explicit path to thetscbinary.
node ./node_modules/typescript/bin/tscworks but is brittle if the TypeScript package ever reorganises its bin layout.npx tsc(or simplytscif the shell has./node_modules/.binon PATH via npm run) is more idiomatic and more resilient.♻️ Proposed change
- "build": "npm run clean && node ./node_modules/typescript/bin/tsc", + "build": "npm run clean && npx tsc",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 40, The "build" npm script currently invokes the TypeScript compiler via an explicit path ("node ./node_modules/typescript/bin/tsc"), which is brittle; update the "build" script in package.json (the "build" property) to use a resilient invocation such as "npx tsc" or just "tsc" so the script relies on the package's bin resolution (e.g., change the value of the "build" script to use "npx tsc" and keep the existing clean step).
55-55:release:checktriggers two full builds.
npm run cialready includesnpm run build. Thennpm pack --dry-runinvokes theprepackhook which runsnpm run buildagain — doubling the compilation time. Either stripbuildout ofci(unlikely desired), skipprepackduring dry-run with--ignore-scripts, or accept the duplicate build.♻️ Option: skip scripts during dry-run pack
- "release:check": "npm run ci && npm pack --dry-run", + "release:check": "npm run ci && npm pack --dry-run --ignore-scripts",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 55, The release:check script currently runs "ci" (which runs "build") and then runs "npm pack --dry-run", which triggers the prepack hook and runs "build" again; update the release:check npm script (named "release:check") to avoid the duplicate build by appending the --ignore-scripts flag to the dry-run pack invocation (i.e., run npm pack --dry-run --ignore-scripts) so prepack/build is skipped, or alternatively remove the build step from the ci script if you prefer that flow; modify the "release:check" script entry accordingly to reference the existing scripts "ci", "build", and "prepack".Dockerfile (1)
1-27: LGTM — the multi-stage build and non-root security posture are correct.One supply-chain note: both
FROMlines use a mutablenode:20-bookworm-slimtag. Pinning to a digest (e.g.,node:20-bookworm-slim@sha256:<hash>) gives reproducible builds and prevents silent base-image drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 1 - 27, The Dockerfile uses mutable tags in both FROM statements (the two occurrences of "FROM node:20-bookworm-slim AS build" and "FROM node:20-bookworm-slim AS runtime"); update both FROM lines to pin the base image to an immutable digest (e.g., replace node:20-bookworm-slim with node:20-bookworm-slim@sha256:<actual-hash>) so the multi-stage build is reproducible and immune to base-image drift; obtain the correct sha256 for the desired Node 20 Bookworm image from the registry (or `docker pull` + `docker inspect --format='{{index .RepoDigests 0}}'`) and use that digest in both FROM lines.src/server/reassemble-documents.test.ts (1)
6-38:best_scoreis computed but never asserted in any test.The score rounding (
Math.round(best_score * 10000) / 10000) and max-selection logic are untested. Test 1 has chunks with scores0.9and0.8for P1234, making it an easy addition:✅ Suggested addition to the first test
expect(p1234?.chunk_count).toBe(2); + expect(p1234?.best_score).toBe(0.9); expect(p5678?.merged_content).toBe('Other doc.');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/reassemble-documents.test.ts` around lines 6 - 38, Add assertions to the existing test that verify the computed best_score on the reassembled documents (from reassembleByDocument) is the correctly rounded maximum chunk score per document; specifically assert that for document 'P1234' best_score equals the rounded value of 0.9 (Math.round(0.9*10000)/10000) and for 'P5678' best_score equals the rounded value of 0.7, ensuring the max-selection and rounding logic is exercised.src/server/namespaces-cache.ts (1)
17-37: No stampede protection on concurrent cache misses.If multiple tool calls arrive simultaneously when the cache is cold or expired, each will independently call
client.listNamespacesWithMetadata()before any of them writes to the cache. For an MCP server with sequential request handling this is typically harmless, but worth noting if parallelism is ever added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/namespaces-cache.ts` around lines 17 - 37, getNamespacesWithCache currently suffers from cache stampede: concurrent callers can all call client.listNamespacesWithMetadata() when the cache is cold. Protect against this by introducing an in-flight promise/lock (e.g., namespacesCachePromise) that callers await if set; in getNamespacesWithCache, check namespacesCache first, then if no valid cache and namespacesCachePromise exists await it and return its result, otherwise set namespacesCachePromise = client.listNamespacesWithMetadata() (or a wrapper that also sets namespacesCache with expiresAt and clears the promise on completion or error), await it, then populate namespacesCache (using FLOW_CACHE_TTL_MS for expiresAt), clear namespacesCachePromise, and return the data; reference getNamespacesWithCache, namespacesCache, getPineconeClient, listNamespacesWithMetadata, and FLOW_CACHE_TTL_MS when implementing.src/pinecone-client.ts (2)
163-176: Missing'array' → 'string[]'upgrade path in type inference.The type upgrade on lines 170–175 only promotes
'object'→'string[]'. If the first sampled record has an empty array (inferred as'array') and a later record has a populated string array (inferred as'string[]'), the field stays typed as'array'even though it actually holdsstring[]values.♻️ Proposed fix
- } else if ( - metadataFields[key] === 'object' && - inferredType === 'string[]' - ) { + } else if ( + (metadataFields[key] === 'object' || metadataFields[key] === 'array') && + inferredType === 'string[]' + ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pinecone-client.ts` around lines 163 - 176, The type-inference merge in the loop over sampleQuery.matches fails to upgrade an existing 'array' to 'string[]' when a later sample returns a populated string array; update the merge logic in the block that handles metadataFields[key] (inside the loop over sampleQuery.matches in src/pinecone-client.ts) to also set metadataFields[key] = 'string[]' when metadataFields[key] === 'array' and inferredType === 'string[]' (in addition to the existing object → string[] case), ensuring inferMetadataFieldType results correctly normalize to string[] when observed.
227-233:logDebugfor filter application is silently suppressed wheneveroptions.fieldsis set.The guard
if (!options?.fields)means the filter-application log never fires forcount()calls (which always provide{ fields: COUNT_FIELDS }) orquery()calls with explicit field selection. This makes it harder to trace metadata-filter activity in those paths.♻️ Proposed fix
- if (metadataFilter !== undefined) { - queryPayload['filter'] = metadataFilter; - if (!options?.fields) { - logDebug('Applying metadata filter', metadataFilter); - } - } + if (metadataFilter !== undefined) { + queryPayload['filter'] = metadataFilter; + logDebug('Applying metadata filter', metadataFilter); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pinecone-client.ts` around lines 227 - 233, The code currently skips calling logDebug('Applying metadata filter', metadataFilter) whenever options?.fields is truthy (which hides logs for count() and query() calls that set fields); update the block around metadataFilter and queryPayload so that when metadataFilter !== undefined you always set queryPayload['filter'] = metadataFilter and always call logDebug('Applying metadata filter', metadataFilter) (i.e., remove or relocate the if (!options?.fields) guard), keeping the rest of the logic unchanged; reference symbols: metadataFilter, queryPayload, logDebug, options.fields, count(), and query().src/server/namespace-router.ts (1)
31-37: Optional: use word-boundary matching for token scoring.
q.includes(token)is a substring check, so a token like"cpp"would score on queries containing"cpplang"or"cppdocs". Since ranking is relative this rarely causes wrong answers, but replacing the includes check with a word-boundary regex (/\bcpp\b/) would reduce false-positive score inflation for short tokens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/namespace-router.ts` around lines 31 - 37, Replace the substring check q.includes(token) in the namespace scoring loop with a word-boundary regex so tokens only match whole words; in the block that builds nameTokens and updates score/reasons (the loop over nameTokens in namespace-router.ts using normalizedName, q, score, reasons) construct a regex like new RegExp(`\\b${escapeRegex(token)}\\b`, 'i') and test that against q, and add an escapeRegex helper (e.g., token.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')) to safely escape token characters before building the regex.src/server/url-generation.test.ts (1)
5-12: Consider adding an edge-case test for empty-stringmetadata.url.When
metadata.urlis"",asStringreturnsnull(due to thetrim().length > 0guard), so the generator is correctly invoked. A test would ensure this behavior doesn't regress:💡 Suggested test
+ it('falls through to generator when metadata.url is empty string', () => { + const r = generateUrlForNamespace('mailing', { + url: '', + doc_id: 'boost-announce@lists.boost.org/message/ABCD', + }); + expect(r.method).toBe('generated.mailing'); + expect(r.url).toContain('lists.boost.org'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/url-generation.test.ts` around lines 5 - 12, Add a test covering the edge case where metadata.url is an empty string: call generateUrlForNamespace('mailing', { url: '', doc_id: 'ignored' }) and assert that the empty-string is treated as absent (since asString returns null for trimmed empty values) by verifying the returned object does not use the metadata path (r.method !== 'metadata.url') and that r.url is generated (truthy/non-empty) by the generator; name the test to indicate "empty-string metadata.url treated as absent".src/server/format-query-result.ts (1)
46-51: Empty-stringdocument_numberprevents thefilenamefallback.
typeof "" === 'string'istrue, sopaper_numberbecomes""rather than falling through to thefilenamederivation. The??operator only bypassesnull/undefined, not empty strings.♻️ Proposed fix
- const paper_number = - (typeof docNum === 'string' ? docNum : null) ?? - (typeof filename === 'string' ? filename.replace(/\.md$/i, '').toUpperCase() : null) ?? - null; + const paper_number = + (typeof docNum === 'string' && docNum.length > 0 ? docNum : null) ?? + (typeof filename === 'string' && filename.length > 0 + ? filename.replace(/\.md$/i, '').toUpperCase() + : null) ?? + null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/format-query-result.ts` around lines 46 - 51, The current paper_number derivation treats an empty string in metadata['document_number'] as a valid value and prevents falling back to filename; update the logic around docNum and paper_number so empty or all-whitespace strings are treated as missing (e.g., check that docNum is a non-empty trimmed string or coerce/fallback), so that when docNum is empty the code falls through to using filename.replace(...). Ensure you update the checks that reference docNum, filename, and paper_number accordingly.src/server.test.ts (1)
56-63: Missing test for$inwith an array of numbers.The existing
$intests cover string arrays (accepted) and a bare string (rejected), but not an array of numbers — which Pinecone also rejects for$in/$nin. A covering test would guard against the validator being loosened inadvertently.💡 Suggested test
+ it('rejects numeric arrays for $in', () => { + const result = validateMetadataFilter({ + year: { $in: [2024, 2025] }, + }); + expect(result).toContain('must use an array of primitive values'); + });Based on learnings: Pinecone metadata filters only support arrays of strings for
$inand$ninoperators, not arrays of numbers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server.test.ts` around lines 56 - 63, Add a unit test in src/server.test.ts that calls validateMetadataFilter with an $in array of numbers (e.g., { year: { $in: [2020, 2021] } }) and assert that the validator rejects it (expect a non-null/validation error) to ensure $in/$nin only accept arrays of strings; name the test clearly (e.g., "rejects $in with array of numbers") and mirror the style of the existing tests that use validateMetadataFilter so it fails if numeric arrays are allowed.src/server/tools/generate-urls-tool.ts (1)
30-35: Consider boundingrecordsarray length.No
.max()constraint is set, so a caller could pass an arbitrarily large array. While the map is synchronous andgenerateUrlForNamespaceis fast, adding a reasonable cap (e.g.,z.array(...).max(500)) avoids runaway work from misbehaving clients.♻️ Proposed fix
- records: z - .array(z.record(z.string(), z.unknown())) - .describe( + records: z + .array(z.record(z.string(), z.unknown())) + .max(500) + .describe(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tools/generate-urls-tool.ts` around lines 30 - 35, The records schema allows unbounded arrays which can lead to runaway work; update the zod schema for records (the z.array(...) used in generate-urls-tool.ts) to set a reasonable maximum (e.g., append .max(500) to the records array validator) so callers cannot pass arbitrarily large arrays; keep the same inner type (z.record(z.string(), z.unknown())) and adjust any runtime behavior in generateUrlForNamespace or calling code if you need to handle the capped size differently (e.g., early slice or return an error when length exceeds the max).src/server/tools/suggest-query-params-tool.ts (1)
49-53: Spread ordering may silently overwritestatus/cache_hitifSuggestQueryParamsResultgains those keys.
...resultappears afterstatusandcache_hit, so any future addition of astatusorcache_hitfield toSuggestQueryParamsResultwould silently override your intended values. Prefer explicit field listing or movestatus/cache_hitto the end of the spread.♻️ Proposed safer construction
- const response = { - status: 'success' as const, - cache_hit, - ...result, - }; + const response = { + ...result, + status: 'success' as const, + cache_hit, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tools/suggest-query-params-tool.ts` around lines 49 - 53, The object literal building the response uses "...result" after setting status and cache_hit, which can be silently overwritten if SuggestQueryParamsResult later includes those keys; update the construction in the suggest-query-params response to either explicitly list all fields from result instead of spreading, or move status and cache_hit to after the spread so they override any keys from SuggestQueryParamsResult (refer to the response creation where status: 'success', cache_hit and ...result are combined and the SuggestQueryParamsResult type).src/server/suggestion-flow.ts (2)
54-60: Hardcoded"30 minutes"in error message will drift ifFLOW_CACHE_TTL_MSchanges.Derive the display string from the constant to keep them in sync.
♻️ Proposed fix
+const FLOW_CACHE_TTL_MINUTES = FLOW_CACHE_TTL_MS / (60 * 1000); // …inside requireSuggested: - message: - 'Previous suggest_query_params context expired (30 minutes). Call suggest_query_params again before query/count tools.', + message: + `Previous suggest_query_params context expired (${FLOW_CACHE_TTL_MINUTES} minutes). Call suggest_query_params again before query/count tools.`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/suggestion-flow.ts` around lines 54 - 60, The error message hardcodes "30 minutes" and can get out of sync with the FLOW_CACHE_TTL_MS constant; update the block that checks (now - state.updatedAt > FLOW_CACHE_TTL_MS) (and the return object created after stateByNamespace.delete(namespace)) to compute a human-readable duration from FLOW_CACHE_TTL_MS (e.g., convert milliseconds to minutes/hours string) and interpolate that computed string into the message instead of the literal "30 minutes" so the displayed TTL always matches the constant.
10-10:stateByNamespaceis process-local — flow state is lost on restart and not shared across instances.The singleton Map works fine for a single long-lived stdio process, but any deployment that runs multiple processes (e.g., containerized replicas, serverless cold-starts, or process restarts) will silently lose suggestion-flow context, causing
requireSuggestedto block every query untilsuggest_query_paramsis re-run.If horizontal scaling or persistence becomes a concern, consider extracting the store behind an interface so it can be swapped for Redis or another shared store without touching the flow logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/suggestion-flow.ts` at line 10, stateByNamespace (Map<string, FlowState>) is a process-local singleton so flow state is lost on restarts or across instances; refactor the storage behind a simple interface (e.g., FlowStore with get(namespace): Promise<FlowState|null>, set(namespace, FlowState): Promise<void>, delete(namespace): Promise<void>) and replace direct Map usage in suggestion-flow.ts while keeping the FlowState shape and APIs used by requireSuggested and suggest_query_params; implement an in-memory adapter that delegates to the Map for local dev and add a Redis (or other persistent) adapter that satisfies FlowStore so deployments can swap implementations via configuration without changing flow logic.src/server/tools/query-documents-tool.ts (1)
110-123: Nitpick:metadata_filter ?? undefinedis redundant.Line 114:
metadata_filteris alreadyundefinedwhen not provided, so?? undefinedis a no-op. Dropping it makes intent clearer.♻️ Proposed fix
- metadata_filter: metadata_filter ?? undefined, + metadata_filter,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tools/query-documents-tool.ts` around lines 110 - 123, Remove the redundant nullish coalescing in the jsonResponse payload: replace the field assignment `metadata_filter: metadata_filter ?? undefined` with simply `metadata_filter: metadata_filter` (or just `metadata_filter,`) inside the return block where jsonResponse is constructed (the same block that includes status, query, namespace, result_count, and documents) so the value is passed through as-is without the no-op `?? undefined`.src/server/tools/query-tool.ts (2)
28-34:!query_textguard is redundant for a typedstring.Zod guarantees
query_textis astring; it cannot benull/undefined. The first condition is alwaysfalseand can be dropped.♻️ Simplification
- if (!query_text || !query_text.trim()) { + if (!query_text.trim()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tools/query-tool.ts` around lines 28 - 34, The guard uses a redundant "!query_text" check for a typed string; update the validation in the function that constructs QueryResponse so it only checks for an empty trimmed string (i.e., replace "if (!query_text || !query_text.trim())" with a single trimmed check), keep the QueryResponse construction and jsonErrorResponse(...) return path unchanged, and ensure references to query_text, QueryResponse, and jsonErrorResponse remain correct.
128-178: Remove redundant nullish-coalescing fallbacks—Zod defaults are already applied by MCP SDK validation.The MCP SDK applies Zod schema validation (including
.default()transforms) before invoking the tool handler, soparams.top_kandparams.use_rerankingalways have values at runtime. The?? 10and?? truefallbacks are dead code and should be removed.The
baseSchemaalready definestop_k: z.number()...default(10), and both thequeryandquery_detailedtools adduse_reranking: z.boolean().default(true)in their input schemas. These defaults are applied during validation before the handler runs.♻️ Remove redundant fallbacks
async (params) => executeQuery({ ...params, - top_k: params.top_k ?? 10, - use_reranking: params.use_reranking ?? true, + top_k: params.top_k, + use_reranking: params.use_reranking, mode: 'query', }) ); server.registerTool( 'query_fast', ... async (params) => executeQuery({ ...params, - top_k: params.top_k ?? 10, + top_k: params.top_k, use_reranking: false, fields: params.fields?.length ? params.fields : [...FAST_QUERY_FIELDS], mode: 'query_fast', }) ); server.registerTool( 'query_detailed', ... async (params) => executeQuery({ ...params, - top_k: params.top_k ?? 10, - use_reranking: params.use_reranking ?? true, + top_k: params.top_k, + use_reranking: params.use_reranking, mode: 'query_detailed', }) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tools/query-tool.ts` around lines 128 - 178, Remove redundant nullish-coalescing fallbacks that duplicate Zod defaults: inside the server.registerTool handlers for 'query', 'query_fast', and 'query_detailed' remove the "params.top_k ?? 10" and the "params.use_reranking ?? true" usages and instead pass params.top_k and params.use_reranking directly into executeQuery (keep the explicit use_reranking: false in 'query_fast' since that intentionally overrides the schema), so the handlers rely on MCP SDK/Zod-validated defaults defined in baseSchema and the tool input schemas.src/server/tools/count-tool.ts (2)
76-82: Use ES6 property shorthand formetadata_filter.♻️ Shorthand
const response: CountResponse = { status: COUNT_RESPONSE_STATUS, count, truncated, namespace, - metadata_filter: metadata_filter, + metadata_filter, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tools/count-tool.ts` around lines 76 - 82, The response object construction is using verbose property assignment for metadata_filter; update the object literal in the CountResponse creation (the response constant returned in count-tool.ts) to use ES6 property shorthand by replacing "metadata_filter: metadata_filter" with just "metadata_filter" so the response remains identical but follows shorthand syntax; ensure the rest of the CountResponse fields (status, count, truncated, namespace) remain unchanged.
9-18:COUNT_RESPONSE_STATUSconstant adds indirection without benefit.
typeof COUNT_RESPONSE_STATUSresolves to'success'— equivalent to using the string literal directly. The constant is single-use and not exported.♻️ Simplification
-const COUNT_RESPONSE_STATUS = 'success' as const; type CountResponse = | { - status: typeof COUNT_RESPONSE_STATUS; + status: 'success'; count: number; truncated: boolean; namespace: string; metadata_filter?: Record<string, unknown>; } | { status: 'error'; message: string };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tools/count-tool.ts` around lines 9 - 18, Remove the unnecessary COUNT_RESPONSE_STATUS constant and use the string literal directly in the CountResponse type: replace status: typeof COUNT_RESPONSE_STATUS with status: 'success' in the success variant and delete the COUNT_RESPONSE_STATUS declaration; update any usages that referenced COUNT_RESPONSE_STATUS to use the literal 'success' or the CountResponse type instead (refer to COUNT_RESPONSE_STATUS and CountResponse to locate the changes).
🤖 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/index.ts`:
- Around line 105-114: The code reads and then overwrites
process.env['LOG_LEVEL'], causing undocumented side effects; update the rawLevel
computation to stop using process.env['LOG_LEVEL'] as a fallback and remove the
assignment process.env['LOG_LEVEL'] = logLevel so only the resolved value is
applied via setLogLevel(logLevel); adjust the rawLevel expression (referencing
rawLevel and logLevel) to use only options.logLevel and the explicit
PINECONE_READ_ONLY_MCP_LOG_LEVEL fallback, and ensure setLogLevel(logLevel)
remains the single place that applies the level.
In `@src/logger.ts`:
- Around line 33-40: formatMessage currently uses JSON.stringify(data) which
throws on circular references and types level as string; change level's
parameter type to the internal LogLevel enum/type and make data serialization
safe: wrap JSON.stringify in try/catch and on failure fall back to a
non-throwing serializer (e.g., util.inspect(data, { depth: null }) or
JSON.stringify with a circular replacer) so the logger never throws, and ensure
this behavior is used by the four public logging functions that call
formatMessage; update the function signature for formatMessage(level: LogLevel,
msg: string, data?: unknown) and add the safe serialization logic inside
formatMessage so callers need no changes.
In `@src/server/metadata-filter.ts`:
- Around line 51-58: The current array-branch in validateMetadataFilter uses
metadataFilterSchema.safeParse on each $and/$or element which lets Zod accept
number arrays (via metadataFilterValueSchema) and bypasses runtime checks like
isPrimitiveArray for $in/$nin; change the loop inside the Array.isArray(value)
branch to call validateMetadataFilter recursively for each element (e.g., call
validateMetadataFilter(value[i], [...path, String(i)]) and return any error it
produces) so nested elements go through the full runtime validation, and as a
follow-on consider removing z.array(z.number()) from metadataFilterValueSchema
to prevent number-array acceptance at the schema level.
---
Duplicate comments:
In `@CHANGELOG.md`:
- Around line 8-38: The changelog contains reviewer artifact markers at the end
of the comment block ("[approve_code_changes]" and "[duplicate_comment]") that
shouldn't be in CHANGELOG.md; open CHANGELOG.md and remove those bracketed
reviewer tags so the Unreleased section contains only the intended changelog
entries and headings.
In `@src/constants.ts`:
- Around line 44-53: The SERVER_INSTRUCTIONS block still references the tool
name query_documents; either remove that reference or register/rename the actual
tool to match it so clients don't call a non-existent tool—update the
SERVER_INSTRUCTIONS usage flow (which mentions list_namespaces,
namespace_router, suggest_query_params, count, query_fast, query_detailed,
generate_urls) to either replace query_documents with the correct registered
tool name (e.g., the actual full-document retrieval tool) or add/register a tool
named query_documents, and ensure suggest_query_params still gates calls to the
correct tool name so the usage flow and tool registry (and any tooling that
looks for query_documents) are consistent.
- Around line 28-32: These constants (DEFAULT_QUERY_DOCUMENTS_TOP_K,
MAX_QUERY_DOCUMENTS_TOP_K, QUERY_DOCUMENTS_MAX_CHUNKS) appear to be dead code
unless the query-documents tool exists and is registered; verify whether the
module "query-documents-tool" is present and wired into your tool
registry/bootstrap, and if it is not either remove these constants (or move them
into the tool module) or gate their export behind a feature flag/registration
check so they're only defined when the tool is added; if the tool exists, ensure
the tool references these symbols (or imports them) so they are actually used.
In `@src/server/reassemble-documents.ts`:
- Around line 9-10: Update the JSDoc above CHUNK_ORDER_KEYS to correctly
describe each key: note that 'loc' is produced by LangChain document loaders (an
object like { lines: { from, to } }), 'start_index' is added by
RecursiveCharacterTextSplitter when addStartIndex: true, and 'chunk_index' may
be emitted by other splitters/loaders; adjust the comment near the
CHUNK_ORDER_KEYS constant and any mention in getChunkOrder to clarify that 'loc'
is typically an object (and thus may be a no-op for numeric ordering) while
numeric keys like 'start_index' and 'chunk_index' are used for ordering.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 1-27: The Dockerfile uses mutable tags in both FROM statements
(the two occurrences of "FROM node:20-bookworm-slim AS build" and "FROM
node:20-bookworm-slim AS runtime"); update both FROM lines to pin the base image
to an immutable digest (e.g., replace node:20-bookworm-slim with
node:20-bookworm-slim@sha256:<actual-hash>) so the multi-stage build is
reproducible and immune to base-image drift; obtain the correct sha256 for the
desired Node 20 Bookworm image from the registry (or `docker pull` + `docker
inspect --format='{{index .RepoDigests 0}}'`) and use that digest in both FROM
lines.
In `@package.json`:
- Line 40: The "build" npm script currently invokes the TypeScript compiler via
an explicit path ("node ./node_modules/typescript/bin/tsc"), which is brittle;
update the "build" script in package.json (the "build" property) to use a
resilient invocation such as "npx tsc" or just "tsc" so the script relies on the
package's bin resolution (e.g., change the value of the "build" script to use
"npx tsc" and keep the existing clean step).
- Line 55: The release:check script currently runs "ci" (which runs "build") and
then runs "npm pack --dry-run", which triggers the prepack hook and runs "build"
again; update the release:check npm script (named "release:check") to avoid the
duplicate build by appending the --ignore-scripts flag to the dry-run pack
invocation (i.e., run npm pack --dry-run --ignore-scripts) so prepack/build is
skipped, or alternatively remove the build step from the ci script if you prefer
that flow; modify the "release:check" script entry accordingly to reference the
existing scripts "ci", "build", and "prepack".
In `@src/logger.ts`:
- Around line 66-69: Remove the stale commented-out block in the logger module
(the three commented lines that compute "detail" and call console.error with
formatMessage) to clean up dead code; specifically delete the commented lines
shown in the diff so only the active logging implementation remains (locate the
commented block near the formatMessage/console.error usage in logger.ts and
remove it).
In `@src/pinecone-client.test.ts`:
- Around line 132-133: The mock for testClient.searchIndex currently contains an
inline expect which causes assertion failures to surface as unhandled promise
rejections; instead, capture the incoming options into a variable in the test
scope (e.g. let receivedOptions) inside the testClient.searchIndex stub and
return the stubbed response, then call the code under test (client.count()),
await its resolution, and assert on receivedOptions.fields
(expect(receivedOptions?.fields).toEqual([...])) after the promise resolves;
update references to testClient.searchIndex and client.count() in the test to
implement this external-capture-and-assert pattern.
In `@src/pinecone-client.ts`:
- Around line 163-176: The type-inference merge in the loop over
sampleQuery.matches fails to upgrade an existing 'array' to 'string[]' when a
later sample returns a populated string array; update the merge logic in the
block that handles metadataFields[key] (inside the loop over sampleQuery.matches
in src/pinecone-client.ts) to also set metadataFields[key] = 'string[]' when
metadataFields[key] === 'array' and inferredType === 'string[]' (in addition to
the existing object → string[] case), ensuring inferMetadataFieldType results
correctly normalize to string[] when observed.
- Around line 227-233: The code currently skips calling logDebug('Applying
metadata filter', metadataFilter) whenever options?.fields is truthy (which
hides logs for count() and query() calls that set fields); update the block
around metadataFilter and queryPayload so that when metadataFilter !== undefined
you always set queryPayload['filter'] = metadataFilter and always call
logDebug('Applying metadata filter', metadataFilter) (i.e., remove or relocate
the if (!options?.fields) guard), keeping the rest of the logic unchanged;
reference symbols: metadataFilter, queryPayload, logDebug, options.fields,
count(), and query().
In `@src/server.test.ts`:
- Around line 56-63: Add a unit test in src/server.test.ts that calls
validateMetadataFilter with an $in array of numbers (e.g., { year: { $in: [2020,
2021] } }) and assert that the validator rejects it (expect a
non-null/validation error) to ensure $in/$nin only accept arrays of strings;
name the test clearly (e.g., "rejects $in with array of numbers") and mirror the
style of the existing tests that use validateMetadataFilter so it fails if
numeric arrays are allowed.
In `@src/server/format-query-result.ts`:
- Around line 46-51: The current paper_number derivation treats an empty string
in metadata['document_number'] as a valid value and prevents falling back to
filename; update the logic around docNum and paper_number so empty or
all-whitespace strings are treated as missing (e.g., check that docNum is a
non-empty trimmed string or coerce/fallback), so that when docNum is empty the
code falls through to using filename.replace(...). Ensure you update the checks
that reference docNum, filename, and paper_number accordingly.
In `@src/server/namespace-router.ts`:
- Around line 31-37: Replace the substring check q.includes(token) in the
namespace scoring loop with a word-boundary regex so tokens only match whole
words; in the block that builds nameTokens and updates score/reasons (the loop
over nameTokens in namespace-router.ts using normalizedName, q, score, reasons)
construct a regex like new RegExp(`\\b${escapeRegex(token)}\\b`, 'i') and test
that against q, and add an escapeRegex helper (e.g.,
token.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')) to safely escape token characters
before building the regex.
In `@src/server/namespaces-cache.ts`:
- Around line 17-37: getNamespacesWithCache currently suffers from cache
stampede: concurrent callers can all call client.listNamespacesWithMetadata()
when the cache is cold. Protect against this by introducing an in-flight
promise/lock (e.g., namespacesCachePromise) that callers await if set; in
getNamespacesWithCache, check namespacesCache first, then if no valid cache and
namespacesCachePromise exists await it and return its result, otherwise set
namespacesCachePromise = client.listNamespacesWithMetadata() (or a wrapper that
also sets namespacesCache with expiresAt and clears the promise on completion or
error), await it, then populate namespacesCache (using FLOW_CACHE_TTL_MS for
expiresAt), clear namespacesCachePromise, and return the data; reference
getNamespacesWithCache, namespacesCache, getPineconeClient,
listNamespacesWithMetadata, and FLOW_CACHE_TTL_MS when implementing.
In `@src/server/reassemble-documents.test.ts`:
- Around line 6-38: Add assertions to the existing test that verify the computed
best_score on the reassembled documents (from reassembleByDocument) is the
correctly rounded maximum chunk score per document; specifically assert that for
document 'P1234' best_score equals the rounded value of 0.9
(Math.round(0.9*10000)/10000) and for 'P5678' best_score equals the rounded
value of 0.7, ensuring the max-selection and rounding logic is exercised.
In `@src/server/suggestion-flow.ts`:
- Around line 54-60: The error message hardcodes "30 minutes" and can get out of
sync with the FLOW_CACHE_TTL_MS constant; update the block that checks (now -
state.updatedAt > FLOW_CACHE_TTL_MS) (and the return object created after
stateByNamespace.delete(namespace)) to compute a human-readable duration from
FLOW_CACHE_TTL_MS (e.g., convert milliseconds to minutes/hours string) and
interpolate that computed string into the message instead of the literal "30
minutes" so the displayed TTL always matches the constant.
- Line 10: stateByNamespace (Map<string, FlowState>) is a process-local
singleton so flow state is lost on restarts or across instances; refactor the
storage behind a simple interface (e.g., FlowStore with get(namespace):
Promise<FlowState|null>, set(namespace, FlowState): Promise<void>,
delete(namespace): Promise<void>) and replace direct Map usage in
suggestion-flow.ts while keeping the FlowState shape and APIs used by
requireSuggested and suggest_query_params; implement an in-memory adapter that
delegates to the Map for local dev and add a Redis (or other persistent) adapter
that satisfies FlowStore so deployments can swap implementations via
configuration without changing flow logic.
In `@src/server/tools/count-tool.ts`:
- Around line 76-82: The response object construction is using verbose property
assignment for metadata_filter; update the object literal in the CountResponse
creation (the response constant returned in count-tool.ts) to use ES6 property
shorthand by replacing "metadata_filter: metadata_filter" with just
"metadata_filter" so the response remains identical but follows shorthand
syntax; ensure the rest of the CountResponse fields (status, count, truncated,
namespace) remain unchanged.
- Around line 9-18: Remove the unnecessary COUNT_RESPONSE_STATUS constant and
use the string literal directly in the CountResponse type: replace status:
typeof COUNT_RESPONSE_STATUS with status: 'success' in the success variant and
delete the COUNT_RESPONSE_STATUS declaration; update any usages that referenced
COUNT_RESPONSE_STATUS to use the literal 'success' or the CountResponse type
instead (refer to COUNT_RESPONSE_STATUS and CountResponse to locate the
changes).
In `@src/server/tools/generate-urls-tool.ts`:
- Around line 30-35: The records schema allows unbounded arrays which can lead
to runaway work; update the zod schema for records (the z.array(...) used in
generate-urls-tool.ts) to set a reasonable maximum (e.g., append .max(500) to
the records array validator) so callers cannot pass arbitrarily large arrays;
keep the same inner type (z.record(z.string(), z.unknown())) and adjust any
runtime behavior in generateUrlForNamespace or calling code if you need to
handle the capped size differently (e.g., early slice or return an error when
length exceeds the max).
In `@src/server/tools/query-documents-tool.ts`:
- Around line 110-123: Remove the redundant nullish coalescing in the
jsonResponse payload: replace the field assignment `metadata_filter:
metadata_filter ?? undefined` with simply `metadata_filter: metadata_filter` (or
just `metadata_filter,`) inside the return block where jsonResponse is
constructed (the same block that includes status, query, namespace,
result_count, and documents) so the value is passed through as-is without the
no-op `?? undefined`.
In `@src/server/tools/query-tool.ts`:
- Around line 28-34: The guard uses a redundant "!query_text" check for a typed
string; update the validation in the function that constructs QueryResponse so
it only checks for an empty trimmed string (i.e., replace "if (!query_text ||
!query_text.trim())" with a single trimmed check), keep the QueryResponse
construction and jsonErrorResponse(...) return path unchanged, and ensure
references to query_text, QueryResponse, and jsonErrorResponse remain correct.
- Around line 128-178: Remove redundant nullish-coalescing fallbacks that
duplicate Zod defaults: inside the server.registerTool handlers for 'query',
'query_fast', and 'query_detailed' remove the "params.top_k ?? 10" and the
"params.use_reranking ?? true" usages and instead pass params.top_k and
params.use_reranking directly into executeQuery (keep the explicit
use_reranking: false in 'query_fast' since that intentionally overrides the
schema), so the handlers rely on MCP SDK/Zod-validated defaults defined in
baseSchema and the tool input schemas.
In `@src/server/tools/suggest-query-params-tool.ts`:
- Around line 49-53: The object literal building the response uses "...result"
after setting status and cache_hit, which can be silently overwritten if
SuggestQueryParamsResult later includes those keys; update the construction in
the suggest-query-params response to either explicitly list all fields from
result instead of spreading, or move status and cache_hit to after the spread so
they override any keys from SuggestQueryParamsResult (refer to the response
creation where status: 'success', cache_hit and ...result are combined and the
SuggestQueryParamsResult type).
In `@src/server/url-generation.test.ts`:
- Around line 5-12: Add a test covering the edge case where metadata.url is an
empty string: call generateUrlForNamespace('mailing', { url: '', doc_id:
'ignored' }) and assert that the empty-string is treated as absent (since
asString returns null for trimmed empty values) by verifying the returned object
does not use the metadata path (r.method !== 'metadata.url') and that r.url is
generated (truthy/non-empty) by the generator; name the test to indicate
"empty-string metadata.url treated as absent".
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 (3)
src/index.ts (2)
34-61:⚠️ Potential issue | 🟡 MinorSilent mis-parse when a flag-value argument is omitted and the next token is another flag.
If a user runs
--api-key --index-name foo,nextArgis'--index-name', which gets assigned tooptions.apiKey, and--index-nameis silently consumed byi++. Guard each value-consuming case so that a leading--token is rejected as a value.🛠️ Proposed fix (applies to all four value-consuming cases)
case '--api-key': - options.apiKey = nextArg; - i++; + if (nextArg && !nextArg.startsWith('--')) { options.apiKey = nextArg; i++; } break; case '--index-name': - options.indexName = nextArg; - i++; + if (nextArg && !nextArg.startsWith('--')) { options.indexName = nextArg; i++; } break; case '--rerank-model': - options.rerankModel = nextArg; - i++; + if (nextArg && !nextArg.startsWith('--')) { options.rerankModel = nextArg; i++; } break; case '--log-level': - options.logLevel = nextArg; - i++; + if (nextArg && !nextArg.startsWith('--')) { options.logLevel = nextArg; i++; } break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 34 - 61, The argument-parsing loop allows a flag's value to be another flag (e.g., args/nextArg assigned to options.apiKey), so update each value-consuming case (the switch handling '--api-key', '--index-name', '--rerank-model', '--log-level') to validate that nextArg exists and does not start with '--' before assigning and incrementing i; if validation fails, call printHelp() or log a clear error and exit (or throw) instead of silently consuming the next flag. Ensure the check is performed consistently for options.apiKey, options.indexName, options.rerankModel, and options.logLevel so missing values are rejected.
1-1:⚠️ Potential issue | 🟡 MinorFix Prettier formatting to unblock CI.
The CI pipeline reports a Prettier formatting violation for this file. Run:
prettier --write src/index.ts🤖 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 reports a formatting violation due to the shebang line "#!/usr/bin/env node"; run the project's Prettier formatter against the file that contains that shebang (or run prettier --write on the source) to normalize whitespace and ensure a trailing newline/expected EOL, then commit the updated file so CI passes.src/pinecone-client.ts (1)
1-510:⚠️ Potential issue | 🟡 MinorPrettier formatting failure must be fixed before merge
The CI pipeline reports a Prettier violation in this file. Run
prettier --write src/pinecone-client.ts(or the project-wide equivalent) to resolve it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pinecone-client.ts` around lines 1 - 510, Prettier reports formatting violations in this file; run the project's Prettier formatter (e.g., prettier --write) to reformat the file and fix CI failures. Ensure you format the entire file containing inferMetadataFieldType and the PineconeClient class (including methods ensureClient, ensureIndexes, listNamespacesWithMetadata, searchIndex, mergeResults, rerankResults, query, and count), fix any trailing commas/spacing/line breaks and save; rerun lint/CI to confirm the Prettier check passes before pushing.
🧹 Nitpick comments (5)
src/index.ts (2)
152-160: Signal handlers skip graceful server teardown.Both handlers call
process.exit(0)directly without invokingserver.close(), which means the MCP transport is not cleanly drained. For a stdio transport this is generally low risk, but it's still worth closing the server before exiting.♻️ Proposed refactor
- process.on('SIGINT', () => { - console.error('Server stopped by user'); - process.exit(0); - }); - - process.on('SIGTERM', () => { - console.error('Server stopped by signal'); - process.exit(0); - }); + const shutdown = async (reason: string): Promise<void> => { + console.error(`Server stopped: ${reason}`); + await server.close(); + process.exit(0); + }; + + process.on('SIGINT', () => void shutdown('user interrupt')); + process.on('SIGTERM', () => void shutdown('signal'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 152 - 160, Signal handlers currently call process.exit(0) directly and skip graceful shutdown; update both SIGINT and SIGTERM handlers to call the server instance's shutdown method (e.g., server.close() or server.close(callback)/await server.close() if it returns a promise) to drain the MCP transport, handle/ log any error in the close callback or catch, and only call process.exit(0) after the close completes; centralize the logic into a single shutdown function referenced by both handlers to avoid duplication and ensure the MCP transport is cleanly closed.
136-138: Startup messages bypass the centralized logger.Lines 136–138 use raw
console.erroraftersetLogLevel(logLevel)has already configured the singleton logger. Routing them through the project's logger (e.g.,logger.info(...)) ensures they respect the configured level and formatting consistently with the rest of the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 136 - 138, Startup messages are currently printed with console.error and bypass the configured singleton logger; change those three calls to use the project's logger (e.g., logger.info or logger.debug as appropriate) so they respect setLogLevel(logLevel) and global formatting — replace console.error(`Starting Pinecone Read-Only MCP server with stdio transport`), console.error(`Using Pinecone index: ${indexName}`) and console.error(`Log level: ${logLevel}`) with logger.info(...) (or logger.debug if you prefer lower verbosity) so messages flow through the configured logger instance.package.json (1)
40-40:npxis redundant in npm scripts — usetscdirectly for consistency.npm scripts already add
node_modules/.bintoPATH, sonpx tscis equivalent totscbut slower due to thenpxresolution overhead.typecheck(line 53) andbuild:watch(line 41) already calltscdirectly.♻️ Proposed fix
- "build": "npm run clean && npx tsc", + "build": "npm run clean && tsc",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 40, The "build" npm script currently uses "npx tsc" which is redundant; update the "build" script entry named "build" to call "tsc" directly (replace "npx tsc" with "tsc") so it matches other scripts like "typecheck" and "build:watch" and avoids npx overhead.src/server/tools/query-documents-tool.ts (1)
93-100:fields: undefinedis redundant
fieldsis an optional property inQueryParams, so explicitly passingundefinedhas the same effect as omitting it. Removing it keeps the call site cleaner.♻️ Proposed cleanup
const results = await client.query({ query: query_text.trim(), topK: chunkLimit, namespace, useReranking: true, metadataFilter: metadata_filter, - fields: undefined, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tools/query-documents-tool.ts` around lines 93 - 100, Remove the redundant "fields: undefined" from the client.query call in queryDocumentsTool (the block that builds QueryParams: query, topK, namespace, useReranking, metadataFilter) — simply omit the fields property so the optional QueryParams defaults apply, leaving the call cleaner and functionally identical.src/server/tools/count-tool.ts (1)
9-9:COUNT_RESPONSE_STATUSconstant is unnecessary indirectionThe constant is used once (Line 77), within a
CountResponse-typed object literal where TypeScript already enforcesstatus: 'success'from the union. Inline the literal directly.♻️ Proposed cleanup
-const COUNT_RESPONSE_STATUS = 'success' as const; type CountResponse = | { status: 'success'; ...const response: CountResponse = { - status: COUNT_RESPONSE_STATUS, + status: 'success', count,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tools/count-tool.ts` at line 9, The COUNT_RESPONSE_STATUS constant is unnecessary indirection; remove the COUNT_RESPONSE_STATUS declaration and replace its single usage inside the CountResponse-typed object (where status is currently assigned COUNT_RESPONSE_STATUS) with the literal 'success' so TypeScript's CountResponse union enforces the value directly; update any imports/usages if referenced elsewhere and run type-check to confirm no other references to COUNT_RESPONSE_STATUS remain.
🤖 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/index.ts`:
- Around line 105-112: The log-level validation is case-sensitive so lowercase
inputs like "--log-level debug" fall back to 'INFO'; update the handling of
rawLevel (sourced from options.logLevel or
process.env['PINECONE_READ_ONLY_MCP_LOG_LEVEL']) by normalizing it with
.toUpperCase() before the includes check and assignment to logLevel, then pass
that normalized LogLevel into setLogLevel (ensure you reference rawLevel and
logLevel variables and leave setLogLevel usage unchanged).
In `@src/pinecone-client.ts`:
- Around line 170-176: The conditional guarding the upgrade to array type is
mis-parsed due to operator precedence: (metadataFields[key] === 'object') ||
(metadataFields[key] === 'array' && inferredType === 'string[]') causes the
'object' branch to always match; change the condition in the block that updates
metadataFields[key] (the if using metadataFields[key] and inferredType) to
require inferredType === 'string[]' together with either stored value being
'object' or 'array' (i.e. group the left-hand checks with parentheses or rewrite
as (metadataFields[key] === 'object' || metadataFields[key] === 'array') &&
inferredType === 'string[]') so you only set metadataFields[key] = inferredType
when the new sample is actually a string array.
---
Outside diff comments:
In `@src/index.ts`:
- Around line 34-61: The argument-parsing loop allows a flag's value to be
another flag (e.g., args/nextArg assigned to options.apiKey), so update each
value-consuming case (the switch handling '--api-key', '--index-name',
'--rerank-model', '--log-level') to validate that nextArg exists and does not
start with '--' before assigning and incrementing i; if validation fails, call
printHelp() or log a clear error and exit (or throw) instead of silently
consuming the next flag. Ensure the check is performed consistently for
options.apiKey, options.indexName, options.rerankModel, and options.logLevel so
missing values are rejected.
- Line 1: Prettier reports a formatting violation due to the shebang line
"#!/usr/bin/env node"; run the project's Prettier formatter against the file
that contains that shebang (or run prettier --write on the source) to normalize
whitespace and ensure a trailing newline/expected EOL, then commit the updated
file so CI passes.
In `@src/pinecone-client.ts`:
- Around line 1-510: Prettier reports formatting violations in this file; run
the project's Prettier formatter (e.g., prettier --write) to reformat the file
and fix CI failures. Ensure you format the entire file containing
inferMetadataFieldType and the PineconeClient class (including methods
ensureClient, ensureIndexes, listNamespacesWithMetadata, searchIndex,
mergeResults, rerankResults, query, and count), fix any trailing
commas/spacing/line breaks and save; rerun lint/CI to confirm the Prettier check
passes before pushing.
---
Nitpick comments:
In `@package.json`:
- Line 40: The "build" npm script currently uses "npx tsc" which is redundant;
update the "build" script entry named "build" to call "tsc" directly (replace
"npx tsc" with "tsc") so it matches other scripts like "typecheck" and
"build:watch" and avoids npx overhead.
In `@src/index.ts`:
- Around line 152-160: Signal handlers currently call process.exit(0) directly
and skip graceful shutdown; update both SIGINT and SIGTERM handlers to call the
server instance's shutdown method (e.g., server.close() or
server.close(callback)/await server.close() if it returns a promise) to drain
the MCP transport, handle/ log any error in the close callback or catch, and
only call process.exit(0) after the close completes; centralize the logic into a
single shutdown function referenced by both handlers to avoid duplication and
ensure the MCP transport is cleanly closed.
- Around line 136-138: Startup messages are currently printed with console.error
and bypass the configured singleton logger; change those three calls to use the
project's logger (e.g., logger.info or logger.debug as appropriate) so they
respect setLogLevel(logLevel) and global formatting — replace
console.error(`Starting Pinecone Read-Only MCP server with stdio transport`),
console.error(`Using Pinecone index: ${indexName}`) and console.error(`Log
level: ${logLevel}`) with logger.info(...) (or logger.debug if you prefer lower
verbosity) so messages flow through the configured logger instance.
In `@src/server/tools/count-tool.ts`:
- Line 9: The COUNT_RESPONSE_STATUS constant is unnecessary indirection; remove
the COUNT_RESPONSE_STATUS declaration and replace its single usage inside the
CountResponse-typed object (where status is currently assigned
COUNT_RESPONSE_STATUS) with the literal 'success' so TypeScript's CountResponse
union enforces the value directly; update any imports/usages if referenced
elsewhere and run type-check to confirm no other references to
COUNT_RESPONSE_STATUS remain.
In `@src/server/tools/query-documents-tool.ts`:
- Around line 93-100: Remove the redundant "fields: undefined" from the
client.query call in queryDocumentsTool (the block that builds QueryParams:
query, topK, namespace, useReranking, metadataFilter) — simply omit the fields
property so the optional QueryParams defaults apply, leaving the call cleaner
and functionally identical.
wpak-ai
approved these changes
Feb 23, 2026
4 tasks
11 tasks
Merged
5 tasks
8 tasks
3 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
2 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
This PR extends the Pinecone Read-Only MCP server with a richer tool set, clearer flow (list → suggest → query/count), and production/deployment improvements. It also fixes a timestamp filter error in metadata filtering.
Changes
New MCP tools
Server refactor
Pinecone client
Other
Flow
Testing
Summary by CodeRabbit
New Features
Improvements
Bug Fixes