Add per-source descriptions and per-namespace declared schemas from p… - #204
Merged
wpak-ai merged 6 commits intoJul 9, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds private-config source descriptions and namespace schema declarations, then threads structured namespace results and warnings through PineconeClient, ServerContext, source-registry, and the ChangesPrivate config descriptions and schema declarations
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ServerContext
participant PineconeClient
participant PineconeIndexSession
participant listNamespacesTool
ServerContext->>PineconeClient: listNamespacesWithMetadata(declaredSchemas, declaredNamespaceNames)
PineconeClient->>PineconeIndexSession: listNamespacesWithMetadata(declaredSchemas, declaredNamespaceNames)
PineconeIndexSession-->>PineconeClient: { namespaces, warnings }
PineconeClient-->>ServerContext: { namespaces, warnings }
ServerContext-->>listNamespacesTool: cacheResult with warnings and schema_source
listNamespacesTool-->>ServerContext: validatedJsonResponse(listNamespacesResponseSchema)
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #204 +/- ##
=======================================
Coverage ? 84.99%
=======================================
Files ? 46
Lines ? 2399
Branches ? 814
=======================================
Hits ? 2039
Misses ? 359
Partials ? 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/test-search.ts (1)
47-54: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winIncomplete migration:
namespacesInfostill treated as a bare array in four locations.Lines 38–43 were updated to the new
{ namespaces, warnings }return shape, but the rest of the script still indexesnamespacesInfodirectly as an array. This will cause runtime errors:
- Line 47:
namespacesInfo.length→undefined(object has no.length)- Line 54:
namespacesInfo[0].namespace→TypeError: Cannot read properties of undefined- Line 98:
namespacesInfo.find(...)→TypeError: namespacesInfo.find is not a function- Line 152:
namespacesInfo.map(...)→TypeError: namespacesInfo.map is not a function🐛 Proposed fix for all four references
// Line 47 - if (namespacesInfo.length === 0) { + if (namespacesInfo.namespaces.length === 0) { // Line 54 - const testNamespace = namespacesInfo[0].namespace; + const testNamespace = namespacesInfo.namespaces[0].namespace; // Line 98 - const wg21Namespace = namespacesInfo.find((ns) => ns.namespace === 'wg21-papers'); + const wg21Namespace = namespacesInfo.namespaces.find((ns) => ns.namespace === 'wg21-papers'); // Line 152 - ` Available namespaces: ${namespacesInfo.map((ns) => ns.namespace).join(', ')}` + ` Available namespaces: ${namespacesInfo.namespaces.map((ns) => ns.namespace).join(', ')}`Also applies to: 98-98, 152-152
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test-search.ts` around lines 47 - 54, `namespacesInfo` in `scripts/test-search.ts` is now the new `{ namespaces, warnings }` object, but the script still treats it like an array in the later checks. Update the usages in the main test flow and the helper logic to read from `namespacesInfo.namespaces` instead of calling array methods directly, including the empty-state check, `testNamespace` selection, the `find` lookup, and the `map` iteration. Use the existing `namespacesInfo` variable and adjust any `namespace` property access accordingly so all four references work with the migrated return shape.
🧹 Nitpick comments (3)
src/core/server/source-config.test.ts (1)
107-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for namespace-level non-string description validation.
validateNamespacesthrows when a namespace'sdescriptionis non-string (lines 78–83 in source-config.ts), but no test covers this path. The malformedmetadata_schematest (lines 156–173) covers the schema validation error, but the description type check remains untested.🧪 Suggested test addition
it('parseSourcesConfigFile throws on non-string namespace description', () => { const dir = mkdtempSync(join(tmpdir(), 'pinecone-sources-')); const filePath = join(dir, 'bad-ns-desc.json'); writeFileSync( filePath, JSON.stringify({ defaultSource: 'api_key_1', sources: { api_key_1: { apiKey: 'k', indexName: 'idx', namespaces: { ns1: { description: 123 } }, }, }, }) ); expect(() => parseSourcesConfigFile(filePath, {})).toThrow(/description must be a string/); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/server/source-config.test.ts` around lines 107 - 179, Add a test in source-config.test.ts for the validateNamespaces path where a namespace description is not a string, since parseSourcesConfigFile currently only verifies malformed metadata_schema and back-compat cases. Extend the existing parseSourcesConfigFile coverage by creating a bad namespace fixture with a numeric description and asserting the parser throws the expected description type error, using parseSourcesConfigFile, validateNamespaces, and the namespaces object shape to locate the code.src/core/pinecone/indexes.ts (1)
118-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate inline return-type literal across files.
The
{ namespaces: Array<{...}>; warnings: string[] }return type is repeated verbatim inindexes.ts,pinecone-client.ts, anddemo-mock-pinecone-client.ts. Extracting a shared exported type (e.g.NamespacesWithMetadataResult) would reduce the risk of the three copies drifting apart on a future field addition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/pinecone/indexes.ts` around lines 118 - 128, The inline return type for listNamespacesWithMetadata is duplicated across indexes.ts, pinecone-client.ts, and demo-mock-pinecone-client.ts; extract it into a shared exported type such as NamespacesWithMetadataResult. Update listNamespacesWithMetadata and the corresponding methods in PineconeClient and DemoMockPineconeClient to use the shared type so all three stay aligned if fields change.src/core/server/server-context.ts (1)
266-279: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
listSourceDetails()fallback logic is sound.The method correctly delegates to
sourceRegistrywhen present and falls back tocfg.sourcesotherwise. The conditional spread ofdescriptionensures backward-compatible output shape (nodescriptionkey when undefined).One minor note:
this.sourceRegistry!inside the.map()callback requires a non-null assertion because TypeScript can't narrow class fields across closures. Assigning to a local variable before the callback would eliminate this, but it's safe as-is in single-threaded JS.♻️ Optional: use local variable to avoid non-null assertion
listSourceDetails(): { name: string; description?: string }[] { - if (this.sourceRegistry) { - return this.sourceRegistry.listSources().map((name) => { - const def = this.sourceRegistry!.getDefinition(name); + const registry = this.sourceRegistry; + if (registry) { + return registry.listSources().map((name) => { + const def = registry.getDefinition(name); return { name, ...(def.description !== undefined ? { description: def.description } : {}) }; }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/server/server-context.ts` around lines 266 - 279, The fallback logic in listSourceDetails() is fine, but the sourceRegistry branch uses a non-null assertion inside the .map() callback. Update the server-context.ts implementation by assigning this.sourceRegistry to a local variable before calling listSources().map(), then use that local inside the callback so the code no longer relies on this.sourceRegistry! while keeping the same behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/SECURITY.md`:
- Around line 26-29: Broaden the private config warning in SECURITY.md to
explicitly include namespace names as sensitive too, since they can reveal
private corpus structure. Update the existing “Private config content” guidance
so it matches the wording and risk level in CONFIGURATION.md, and keep the
examples/tests guidance pointing to generic placeholders only.
In `@src/constants.test.ts`:
- Around line 61-81: The example test in constants.test.ts only checks raw
strings, so it can pass even if pinecone-sources.json.example is invalid JSON.
Update the test around examplePath/readFileSync to first parse the file contents
as JSON (using the existing example file read flow) before asserting the
placeholder markers and schema fields, so the example is validated structurally
as well as by string match. Keep the existing CORE_SERVER_INSTRUCTIONS and
ALLIANCE_SERVER_INSTRUCTIONS checks in the same test.
In `@src/core/pinecone-client.ts`:
- Around line 80-93: The return shape from listNamespacesWithMetadata is wrapped
in a namespaces object, but scripts/test-search.ts is still treating it like a
raw array. Update the call sites that read namespacesInfo so they use
namespacesInfo.namespaces.length and namespacesInfo.namespaces[0] (and keep the
empty-index guard in the same flow) to align with
PineconeClient.listNamespacesWithMetadata and avoid falling through on empty
results.
In `@src/core/server/source-config.ts`:
- Around line 258-261: The source-level description handling in source-config
should match the namespace validation behavior instead of silently dropping
invalid values. Update the logic around the description assignment in the source
config parsing path to explicitly validate cfg.description as a string, and
raise the same kind of error used by validateNamespaces when a non-string value
is provided. Keep trimOptional for valid strings, but ensure malformed
descriptions are surfaced rather than converted to undefined.
---
Outside diff comments:
In `@scripts/test-search.ts`:
- Around line 47-54: `namespacesInfo` in `scripts/test-search.ts` is now the new
`{ namespaces, warnings }` object, but the script still treats it like an array
in the later checks. Update the usages in the main test flow and the helper
logic to read from `namespacesInfo.namespaces` instead of calling array methods
directly, including the empty-state check, `testNamespace` selection, the `find`
lookup, and the `map` iteration. Use the existing `namespacesInfo` variable and
adjust any `namespace` property access accordingly so all four references work
with the migrated return shape.
---
Nitpick comments:
In `@src/core/pinecone/indexes.ts`:
- Around line 118-128: The inline return type for listNamespacesWithMetadata is
duplicated across indexes.ts, pinecone-client.ts, and
demo-mock-pinecone-client.ts; extract it into a shared exported type such as
NamespacesWithMetadataResult. Update listNamespacesWithMetadata and the
corresponding methods in PineconeClient and DemoMockPineconeClient to use the
shared type so all three stay aligned if fields change.
In `@src/core/server/server-context.ts`:
- Around line 266-279: The fallback logic in listSourceDetails() is fine, but
the sourceRegistry branch uses a non-null assertion inside the .map() callback.
Update the server-context.ts implementation by assigning this.sourceRegistry to
a local variable before calling listSources().map(), then use that local inside
the callback so the code no longer relies on this.sourceRegistry! while keeping
the same behavior.
In `@src/core/server/source-config.test.ts`:
- Around line 107-179: Add a test in source-config.test.ts for the
validateNamespaces path where a namespace description is not a string, since
parseSourcesConfigFile currently only verifies malformed metadata_schema and
back-compat cases. Extend the existing parseSourcesConfigFile coverage by
creating a bad namespace fixture with a numeric description and asserting the
parser throws the expected description type error, using parseSourcesConfigFile,
validateNamespaces, and the namespaces object shape to locate the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06eaded7-b92f-4635-b11d-b6f06ab24272
📒 Files selected for processing (32)
CHANGELOG.mdbenchmarks/latency.tsdocs/CONFIGURATION.mddocs/MIGRATION.mddocs/SECURITY.mddocs/TOOLS.mdexamples/alliance/demo-mock-pinecone-client.tsexamples/multi-source/pinecone-sources.json.examplescripts/test-search.tssrc/alliance/tools/isolated-context.context.test.tssrc/alliance/tools/suggest-query-params-tool.context.test.tssrc/constants.test.tssrc/core/pinecone-client.tssrc/core/pinecone/indexes.test.tssrc/core/pinecone/indexes.tssrc/core/server/response-schemas.test.tssrc/core/server/response-schemas.tssrc/core/server/server-context.composition.test.tssrc/core/server/server-context.test.tssrc/core/server/server-context.tssrc/core/server/source-config.test.tssrc/core/server/source-config.tssrc/core/server/source-registry.test.tssrc/core/server/source-registry.tssrc/core/server/tools/guided-query-tool.context.test.tssrc/core/server/tools/list-namespaces-tool.context.test.tssrc/core/server/tools/list-namespaces-tool.tssrc/core/server/tools/list-sources-tool.test.tssrc/core/server/tools/list-sources-tool.tssrc/core/server/tools/namespace-router-tool.context.test.tssrc/core/server/tools/test-helpers.tssrc/core/setup-multi-instance.test.ts
…tion, harden tests/docs Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/constants.test.ts (1)
88-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
not.toContaininstead ofnew RegExpto silence lint warnings.
corpusPlaceholderis a static trusted constant, so the ReDoS flags from static analysis are false positives. However, replacing.not.toMatch(new RegExp(...))with.not.toContain(...)is simpler, avoids the lint noise, and achieves the same substring check.♻️ Optional simplification
- expect(CORE_SERVER_INSTRUCTIONS).not.toMatch(new RegExp(corpusPlaceholder)); - expect(ALLIANCE_SERVER_INSTRUCTIONS).not.toMatch(new RegExp(corpusPlaceholder)); + expect(CORE_SERVER_INSTRUCTIONS).not.toContain(corpusPlaceholder); + expect(ALLIANCE_SERVER_INSTRUCTIONS).not.toContain(corpusPlaceholder);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/constants.test.ts` around lines 88 - 89, The assertions in the constants test are doing a regex-based substring check that triggers lint noise; update the test in constants.test.ts to use the existing substring matcher instead of constructing a RegExp from corpusPlaceholder. Keep the checks against CORE_SERVER_INSTRUCTIONS and ALLIANCE_SERVER_INSTRUCTIONS, but change the assertions to the simpler containment form so the intent stays the same without the static-analysis warning.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/constants.test.ts`:
- Around line 88-89: The assertions in the constants test are doing a
regex-based substring check that triggers lint noise; update the test in
constants.test.ts to use the existing substring matcher instead of constructing
a RegExp from corpusPlaceholder. Keep the checks against
CORE_SERVER_INSTRUCTIONS and ALLIANCE_SERVER_INSTRUCTIONS, but change the
assertions to the simpler containment form so the intent stays the same without
the static-analysis warning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4113c1e6-9191-4bea-950c-245fc2ad9ebb
📒 Files selected for processing (25)
CHANGELOG.mddocs/CONFIGURATION.mddocs/SECURITY.mddocs/TOOLS.mdexamples/alliance/demo-mock-pinecone-client.tsscripts/test-search.tssrc/alliance/tools/isolated-context.context.test.tssrc/alliance/tools/suggest-query-params-tool.context.test.tssrc/constants.test.tssrc/core/index.tssrc/core/pinecone-client.tssrc/core/pinecone/indexes.test.tssrc/core/pinecone/indexes.tssrc/core/server/namespaces-cache.tssrc/core/server/response-schemas.tssrc/core/server/server-context.tssrc/core/server/source-config.test.tssrc/core/server/source-config.tssrc/core/server/source-registry.test.tssrc/core/server/source-registry.tssrc/core/server/tools/guided-query-tool.context.test.tssrc/core/server/tools/list-namespaces-tool.context.test.tssrc/core/server/tools/list-namespaces-tool.test.tssrc/core/server/tools/list-namespaces-tool.tssrc/core/server/tools/namespace-router-tool.context.test.ts
✅ Files skipped from review due to trivial changes (4)
- docs/CONFIGURATION.md
- docs/SECURITY.md
- docs/TOOLS.md
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (12)
- src/alliance/tools/isolated-context.context.test.ts
- src/core/server/tools/list-namespaces-tool.ts
- examples/alliance/demo-mock-pinecone-client.ts
- src/core/server/tools/guided-query-tool.context.test.ts
- src/core/server/tools/namespace-router-tool.context.test.ts
- src/core/server/source-config.test.ts
- src/core/server/response-schemas.ts
- src/alliance/tools/suggest-query-params-tool.context.test.ts
- src/core/server/server-context.ts
- src/core/server/source-registry.ts
- src/core/server/source-config.ts
- src/core/server/tools/list-namespaces-tool.context.test.ts
AuraMindNest
approved these changes
Jul 9, 2026
…: "this exact text must not be in the instructions."
wpak-ai
requested changes
Jul 9, 2026
wpak-ai
left a comment
wpak-ai
left a comment
Collaborator
There was a problem hiding this comment.
server-context.ts:632-650 + source-registry.ts:115-134: the per-source row-mapping/warnings logic and the CacheEntry type are duplicated across both files, and the server-context copy is an unreachable branch (any config with sources routes through the registry). Delete the dead block and hoist a shared helper. Non-blocking, but it's a silent-drift trap.
wpak-ai
approved these changes
Jul 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
descriptionandnamespacesfields to JSON multi-source config (PINECONE_CONFIG_FILEonly; not inlinePINECONE_SOURCES)descriptionvialist_sourcesas{ name, description? }[]metadata_schemadeclarations that skip live Pinecone sampling when presentconfig_warningsinlist_namespacesschema_source(declared|sampled) per namespace row inlist_namespacesBreaking changes
list_sources:sources: string[]→sources: { name, description? }[]PineconeClient.listNamespacesWithMetadata(): bare array →{ namespaces, warnings }Security / back-compat
description/namespacescontinue to work unchangedTest plan
npm test(351 tests)npm run buildnpm run lintRelated issues
Summary by CodeRabbit
Summary by CodeRabbit
New Features
list_sourcesnow includes optional per-sourcedescription.list_namespacesnow provides per-namespaceschema_sourceand returnsconfig_warningsfor stale/mismatched private config declarations.descriptionand per-namespacenamespaces(including optionalmetadata_schema).Bug Fixes
Breaking Changes
list_sources:sourceschanges fromstring[]to{ name, description? }[].list_namespaces: response changes from an array to{ namespaces, warnings }.Documentation