refactor mcp #4
refactor mcp #4
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis update introduces a comprehensive hybrid search and code generation enhancement across the MCP server. It adds semantic vector search (Qdrant-based and in-memory fallback), a new plugin-based code generator architecture, OpenTelemetry tracing, Prometheus metrics, and a suite of configuration and template files. The changes refactor search, documentation, and code generation to support hybrid approaches, observability, and extensibility. Changes
Sequence Diagram(s)Hybrid Search Flow (API Search Example)sequenceDiagram
participant Client
participant APISearchService
participant SearchService
participant ApiVectorService
Client->>APISearchService: discoverAPI(query)
APISearchService->>APISearchService: hybridSearch(query)
APISearchService->>SearchService: lexicalSearch(query)
APISearchService->>ApiVectorService: semanticSearch(query)
APISearchService->>APISearchService: Combine lexical & semantic results (weighted)
APISearchService-->>Client: Top combined results
Vector Store Adapter InitializationsequenceDiagram
participant Server
participant AdapterFactory
participant QdrantVectorAdapter
participant OpenAIEmbeddingProvider
Server->>AdapterFactory: createVectorAdapter()
AdapterFactory->>QdrantVectorAdapter: new QdrantVectorAdapter()
QdrantVectorAdapter->>OpenAIEmbeddingProvider: initialize()
QdrantVectorAdapter-->>AdapterFactory: Adapter instance
AdapterFactory-->>Server: Adapter instance
Code Generation via Plugin RegistrysequenceDiagram
participant Client
participant CodeGenerationService
participant PluginRegistry
participant CodeGeneratorPlugin
Client->>CodeGenerationService: generateSetupCode(language, features)
CodeGenerationService->>PluginRegistry: createCodeGenerator(language)
PluginRegistry->>CodeGeneratorPlugin: create()
CodeGeneratorPlugin-->>PluginRegistry: CodeGenerator instance
PluginRegistry-->>CodeGenerationService: CodeGenerator instance
CodeGenerationService->>CodeGeneratorPlugin: generateSetup(features)
CodeGenerationService-->>Client: Generated code
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 35
🔭 Outside diff range comments (5)
packages/mcp-server/src/server.ts (1)
94-121: 🛠️ Refactor suggestion
ensureInitializedis not concurrency-safeIf two tool calls arrive before initialization completes, both will run the lengthy async initialisation in parallel. Guard with a promise latch:
- private async ensureInitialized(): Promise<void> { - if (this.initialized) return; + private initPromise?: Promise<void>; + + private async ensureInitialized(): Promise<void> { + if (this.initialized) return this.initPromise; + if (this.initPromise) return this.initPromise; + + this.initPromise = (async () => { ... - this.initialized = true; + this.initialized = true; })(); + return this.initPromise; }packages/mcp-server/package.json (1)
67-76: 🛠️ Refactor suggestion
ts-nodeis used in scripts but not declared
vector:clearrelies onts-node; add it todevDependenciesto prevent runtime failures on fresh installs.+ "ts-node": "^10.9.2",packages/mcp-server/src/services/documentation/vector-service.ts (1)
90-113:⚠️ Potential issueAdapter initialisation is skipped when OpenAI key is absent
If a
VectorStoreAdapteris provided,initialize()returns early whenopenaiApiKeyis missing, never callingadapter.initialize().
Hybrid deployments that rely solely on the adapter (e.g. Qdrant with its own embedding provider) will remain un-initialised andisAvailable()will returnfalse.async initialize(): Promise<void> { - if (!this.config.openaiApiKey) { - console.warn('OpenAI API key not provided. Vector search will be disabled.'); - return; - } + if (this.adapter) { + await this.adapter.initialize(); + this.initialized = true; + eventBus.emit('vector.initialized'); + return; + } + + if (!this.config.openaiApiKey) { + console.warn('OpenAI API key not provided and no adapter supplied – vector search disabled.'); + return; + }packages/mcp-server/src/tools.ts (1)
46-50:⚠️ Potential issueType mismatch between runtime schema and TypeScript interface
GenerateSetupParams.featuresis typed asstring[], yet the Zod schema now validates it asarray(FeatureSchema)(a stricter enum).
Update the interface to reflect the enum type to keep compile-time and runtime contracts in sync.-interface GenerateSetupParams { - language: string; - features: string[]; -} +interface GenerateSetupParams { + language: string; + features: z.infer<typeof FeatureSchema>[]; +}packages/mcp-server/src/services/documentation/documentation-index.ts (1)
238-256:⚠️ Potential issueCritical: getRelatedDocuments implementation is broken.
The method creates an empty
relatedarray and never populates it, making lines 247-255 dead code. This appears to be an incomplete refactoring that breaks the related documents functionality.Either:
- Remove the dead code and document that this feature is intentionally disabled:
async getRelatedDocuments(docId: string): Promise<DocumentationResult[]> { this.ensureInitialized(); const document = this.documents.get(docId); if (!document) return []; - const related: RelatedDocument[] = []; - - // Sort by score and return top results - const topRelated = related.sort((a, b) => b.score - a.score).slice(0, 5); - - return topRelated.map((rel) => { - const doc = this.documents.get(rel.id); - if (!doc) { - throw new Error(`Document not found: ${rel.id}`); - } - return this.convertToDocumentationResult(doc); - }); + // TODO: Implement related documents using vector similarity + return []; }
- Or implement the actual logic to find related documents using the vector service.
♻️ Duplicate comments (1)
packages/mcp-server/src/services/search/workflow-search-engine.ts (1)
604-613: Same semantic-only drop as in SearchService.Documents present exclusively in vector results are ignored. Apply the same fix or extract a shared merge helper to avoid divergence.
🧹 Nitpick comments (51)
packages/mcp-server/src/common/schemas.ts (1)
19-20: Consider exporting aFeaturetype alias alongside the schema
LanguageSchema,SkillLevelSchema, andFrameworkSchemaare normally accompanied by a correspondingtype X = z.infer<typeof XSchema>elsewhere in the code-base. Addingexport type Feature = z.infer<typeof FeatureSchema>;right after the enum keeps the pattern consistent and avoids repeating
z.infer<typeof FeatureSchema>at call-sites.typedoc.json (1)
1-8: Exclude generated docs from VCS & tighten Typedoc output
- Make sure
docs/api/anddocs/api.jsonare in.gitignore; they’re build artefacts.- Unless you intentionally want private / external symbols, consider:
{ … + "excludePrivate": true, + "excludeInternal": true, + "excludeExternals": true, "includeVersion": true, … }These flags keep the public docs concise.
packages/mcp-server/src/services/vector/embedding-provider.ts (1)
11-15: Optional: coupletextsandmetadatalengths explicitlyIf
metadatais provided, its length should matchtexts. The interface contract could document / enforce that:embedDocuments( texts: string[], metadata?: Record<string, unknown>[] ): Promise<number[][]>; // ← clarify that metadata.length === texts.lengthNot blocking, but worth codifying to prevent subtle mismatches later.
packages/mcp-server/src/services/search/workflow-templates.ts (1)
10-58: Minor consistency tweaks for messaging workflowSmall polish items to keep identifiers predictable:
- id: 'setup-endpoint', + id: 'init-endpoint', - apiMethod: 'Endpoint.createThreadApi', + apiMethod: 'Endpoint.createThreadAPI', // matches actual method name(No hard blocker—verify actual API signatures and naming.)
packages/mcp-server/src/services/code-generators/plugin-registry.ts (1)
51-53: Heavy synchronous I/O at module-load timeThe recursive
readdirSync&statSyncrun on every import of this module, potentially in serverless cold-starts and CLI tools alike.
Moving auto-loading behind an explicitinitPlugins()call (or at least switching to async I/O) will keep startup latency predictable.packages/mcp-server/scripts/vector/clear.ts (1)
4-14: Lack of top-level error handlingAny reject (network, auth, etc.) will leave an unhandled promise rejection.
Wrapmain()in a catch-all or addprocess.on('unhandledRejection')to exit with non-zero status.apps/chat/package.json (1)
32-35: Duplicate OpenTelemetry deps across workspaces?
@opentelemetry/*packages are already present in@privmx/mcp-server.
If both versions diverge, yarn/pnpm may hoist two copies, inflating bundle size and breaking context propagation.
Align versions in the rootpackage.jsonor use workspace protocol to keep them in sync.packages/mcp-server/src/services/search/__tests__/bm25-search.spec.ts (1)
6-13: Avoid usingdeleteonprocess.env– restore via assignment instead
delete process.env.USE_BM25triggers V8’s slow-properties path and is flagged by Biome.
Simply reassignundefinedto restore the variable without the performance hit.- if (originalEnv !== undefined) process.env.USE_BM25 = originalEnv; - else delete process.env.USE_BM25; + if (originalEnv !== undefined) { + process.env.USE_BM25 = originalEnv; + } else { + process.env.USE_BM25 = undefined as unknown as string; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 12-12: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
apps/chat/instrumentation.ts (1)
3-8: Make registration idempotent to avoid duplicate OTLP exportersIf
register()can be called by both the app entrypoint and unit tests, multiple invocations will create duplicate exporters/resources.
Guard with a module-level flag:+let registered = false; + export function register() { + if (registered) return; registerOTel({ serviceName: process.env.OTEL_SERVICE_NAME || 'privmx-chat', }); + registered = true; }packages/mcp-server/src/server.ts (2)
21-22: Side-effect imports belong near other infra wiringMetric & tracing imports are runtime-conditional; consider lazy-loading them (like
httpininitMetricsServer) to keep cold-start cost minimal, especially for CLI usage.
164-185: Validation schema is rebuilt on every tool call
z.object(tool.schema)compiles a new Zod object each invocation, adding ~1-2 ms overhead per call. Cache the compiled schema when buildingtoolMap.- const validationResult = z.object(tool.schema).safeParse(args); + const validationResult = tool.compiledSchema.safeParse(args);…and populate
compiledSchemaonce ingetTools.packages/mcp-server/src/services/search/__tests__/hybrid-search.spec.ts (2)
35-41: ResetTEXT_ALGOafter the test to avoid bleed-overSubsequent tests (even in other files) might rely on their own algorithm selection.
const service = new SearchService(); await service.initialize(apiData); const results = await service.search('send message'); expect(results[0].id).toContain('sendMessage'); +delete process.env.TEXT_ALGO;
43-49: Duplicate service initialisation – share viabeforeEachBoth tests rebuild identical
SearchServiceindices, doubling runtime. Extract to abeforeEachhook and reuse the instance unless you explicitly need a fresh one.packages/mcp-server/src/services/code-generators/typescript-generator.ts (1)
11-13: Prefer project logger overconsole.warnfor consistencyAll other generators use the shared logger; switch to maintain uniform log formatting and central control.
- console.warn('[CodeGen] TS template missing, fallback to JS template'); + logger.warn('[CodeGen] TS template missing, falling back to JS template');packages/mcp-server/src/services/vector/adapter-factory.ts (2)
6-20: Cache the adapter to avoid repeated connections
createVectorAdapter()instantiates a fresh adapter every time it’s called, which will re-initialise the underlying client/embeddings on every invocation. In practice the service layer normally needs just one long-lived adapter per process.+let cachedAdapter: VectorStoreAdapter | null = null; + export function createVectorAdapter(): VectorStoreAdapter { - const backend = (process.env.VECTOR_BACKEND || 'qdrant').toLowerCase(); + if (cachedAdapter) return cachedAdapter; + + const backend = (process.env.VECTOR_BACKEND || 'qdrant').toLowerCase(); switch (backend) { case 'qdrant': - return new QdrantVectorAdapter(); + cachedAdapter = new QdrantVectorAdapter(); + break; … - } + } + + return cachedAdapter!; }
15-17: Prefer project logger overconsole.warnAll other services seem to rely on the centralized logger; switching to it keeps log formatting & sinks consistent.
- console.warn( + logger.warn( `Unknown VECTOR_BACKEND="${backend}". Falling back to Qdrant.` );packages/mcp-server/src/services/documentation/frontmatter-schema.ts (1)
6-13: Enumerations risk going staleHard-coding language / framework lists means every new tutorial requires a code change. You may want to accept any string but warn on unknown values, or at least expose the lists via env/config so docs can evolve independently of the server.
packages/mcp-server/src/templates/codegen/javascript/setup.hbs (1)
26-28:isReady()may leak truthy non-boolean valueBecause
connectionIdis returned (a string/number) the expressionthis.isConnected && this.endpoint && this.connectionIdyields that value, not a strict boolean. Wrap withBoolean()for clarity and to avoid subtle bugs in strict comparisons.- return this.isConnected && this.endpoint && this.connectionId; + return Boolean(this.isConnected && this.endpoint && this.connectionId);packages/mcp-server/package.json (2)
26-26:eslint --fixon everylintrun can commit unwanted changesConsider separating lint & auto-fix to avoid surprise diffs in CI:
-"lint": "pnpm eslint 'src/**/*.{ts,js}' --fix", +"lint": "pnpm eslint 'src/**/*.{ts,js}'", +"lint:fix": "pnpm eslint 'src/**/*.{ts,js}' --fix",
30-32:rm -rfis non-portable; userimrafWindows users can’t run
clean. Addrimrafto dev deps and update script:-"clean": "rm -rf dist", +"clean": "rimraf dist",packages/mcp-server/src/templates/codegen/javascript/partials/thread.hbs (1)
18-26: Add defensive error handling around remote calls
sendMessage(and likewisecreateThread) directly propagates any exception coming fromthis.endpoint.thread.*. Wrapping the call in atry / catchthat re-throws with additional context (e.g. thread id, user id) would make debugging network / backend issues much easier for client developers without affecting the happy path.if (!this.isReady()) throw new Error('PrivMX not initialized'); - // snippet: {{methodSnippet "ThreadApi" "sendMessage"}} - return this.endpoint.thread.sendMessage( - threadId, - JSON.stringify(meta), - new TextEncoder().encode(message), - ); + // snippet: {{methodSnippet "ThreadApi" "sendMessage"}} + try { + return await this.endpoint.thread.sendMessage( + threadId, + JSON.stringify(meta), + new TextEncoder().encode(message), + ); + } catch (e) { + throw new Error(`sendMessage failed for thread ${threadId}: ${e instanceof Error ? e.message : e}`); + }packages/mcp-server/src/templates/codegen/javascript/partials/inbox.hbs (1)
18-29: Ensure consistent data encoding betweensendToInboxand other APIs
sendToInboxforwards the rawdatastring whilethread.sendMessageencodes the payload withTextEncoder.
If the backend expects aUint8Arrayfor binary-safe transport, consider applying the same encoding here to prevent subtle UTF-8 issues.- return this.endpoint.inbox.sendEntry( - inboxId, - JSON.stringify(publicMeta), - JSON.stringify(privateMeta), - data, - files, - ); + return this.endpoint.inbox.sendEntry( + inboxId, + JSON.stringify(publicMeta), + JSON.stringify(privateMeta), + new TextEncoder().encode(data), + files, + );packages/mcp-server/src/templates/codegen/javascript/partials/store.hbs (1)
7-15: Consider propagating endpoint errors with contextual messageBoth methods rely on
this.endpoint.store.*. Wrapping the call in a smalltry/catchthat re-throws with extra context (createStore failed,uploadFile failed) makes troubleshooting much easier when the SDK is used in prod.packages/mcp-server/src/services/code-generators/csharp-template-generator.ts (1)
4-10: Missingoverride/ explicit return typesDeclaring the methods with
override(TS 4.3+) and explicit: stringimproves readability and catches signature drift when the base class evolves.- generateSetup(features: string[]): string { + override generateSetup(features: string[]): string {Apply similarly to the other
generate*methods.packages/mcp-server/src/services/documentation/hierarchical-text-splitter.ts (1)
11-14: Remove the no-op constructorStatic analysis is right – the constructor only forwards the args.
Removing it simplifies the class and keeps TS happy.- constructor(options: { chunkSize: number; chunkOverlap: number }) { - super(options); - }🧰 Tools
🪛 Biome (1.9.4)
[error] 11-14: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
packages/mcp-server/env.example (3)
9-9: Use a clearly fake token in example files to avoid accidental leaksDevelopers sometimes copy/paste from
env.exampleinto real.envfiles without editing. Replace the realistic-looking placeholder with something unmistakably fake (e.g.OPENAI_API_KEY=sk-xxxxx).-OPENAI_API_KEY=your-openai-api-key +OPENAI_API_KEY=sk-xxxxx-your-openai-api-key
35-37: Document the defaultVECTOR_BACKENDin one place onlyThe comment says “default: qdrant” but the variable is still commented out. This can confuse new users because the runtime default lives in code.
Add the concrete default next to the variable and/or uncomment it so the value is explicit.-# VECTOR_BACKEND=qdrant +VECTOR_BACKEND=qdrant # comment out to fallback to the built-in memory store
93-104: Clarify boolean env values are stringsNode considers every env value a string;
OTEL_ENABLED=falseis truthy unless you coerce it.
Add a short note (or switch to0/1) to avoid support questions.packages/mcp-server/src/templates/codegen/swift/setup.hbs (2)
9-14: Make the assets path configurableHard-coding
"/assets"ties every generated project to a fixed bundle structure.
Expose it as a parameter with a sensible default to improve portability.- func initialize(privKey: String, solutionId: String, url: String) throws { - try Endpoint.setup(path: "/assets") + func initialize( + privKey: String, + solutionId: String, + url: String, + assetsPath: String = "/assets" + ) throws { + try Endpoint.setup(path: assetsPath)
16-18:isReady()can be simplifiedChecking both optionals duplicates the intent of
connectionId != nil.
IfconnectionIdexists,endpointmust have been assigned in the same call.-return endpoint != nil && connectionId != nil +return connectionId != nilpackages/mcp-server/src/templates/codegen/typescript/setup.hbs (1)
22-27: Pass only defined fields toconnect
platformUrlbecomesundefinedwhenbridgeUrlis omitted, which may violate SDK typings.
Filter out undefined keys to keep the call site clean.- this.connectionId = await this.endpoint.connect({ - userPrivKey: config.userPrivateKey, - solutionId: config.solutionId, - platformUrl: config.bridgeUrl, - }); + const { userPrivateKey, solutionId, bridgeUrl } = config; + const connOpts = { + userPrivKey: userPrivateKey, + solutionId, + ...(bridgeUrl ? { platformUrl: bridgeUrl } : {}), + }; + this.connectionId = await this.endpoint.connect(connOpts);packages/mcp-server/src/common/metrics.ts (2)
13-19: Prometheus naming: use seconds, not millisecondsHistograms ending in
_secondsare expected by many dashboards.
Record durations in seconds (or convert) to stay Prom-compatible.- name: 'privmx_search_duration_ms', - help: 'Duration of search operations', + name: 'privmx_search_duration_seconds', + help: 'Duration of search operations', ... - buckets: [25, 50, 100, 200, 500, 1000, 2000, 5000], + buckets: [0.025, 0.05, 0.1, 0.2, 0.5, 1, 2, 5],Repeat for
codegenDuration.
45-65: Handle server errors & shutdown gracefullyIf the port is occupied or the process receives
SIGTERM, the current code will throw or leave the socket open.
Add minimal error handling and a close hook.- server.listen(port, () => { + server.on('error', err => { + console.error('Metrics server failed:', err); + }); + server.listen(port, () => { console.log( `📈 Prometheus metrics exposed at http://localhost:${port}/metrics` ); }); + + process.on('SIGTERM', () => server.close());packages/mcp-server/src/services/code-generators/api-spec-loader.ts (3)
4-7: Remove unusedMethodSpecinterface
MethodSpecis never referenced. Dead code makes the module harder to maintain and can confuse static-analysis tools.
9-10: Avoidanyin the cache
specCache: Record<string, unknown>(or a dedicatedApiSpectype) keeps the cache type-safe and prevents accidental misuse down-stream.
11-24: Extend language-to-spec mapping & deduplicate casesThe switch covers TS by falling back to the JS spec and lacks C#/dotnet. Consider:
- case 'typescript': - return 'spec/api/js/out.js.json'; + case 'typescript': + case 'ts': + return 'spec/api/js/out.js.json'; + case 'csharp': + case 'c#': + return 'spec/api/csharp/PrivMXEndpoint.json';Consolidating
'javascript'|'js'and adding aliases avoids surprises when the caller passes short codes.packages/mcp-server/src/common/service-manager.ts (1)
21-24: Use class name instead ofthisinside static contextStatic ESLint rule (
noThisInStatic) flagsthis._instance. Replace withServiceManager._instancefor clarity.- if (!this._instance) this._instance = new ServiceManager(); - return this._instance; + if (!ServiceManager._instance) ServiceManager._instance = new ServiceManager(); + return ServiceManager._instance;🧰 Tools
🪛 Biome (1.9.4)
[error] 22-22: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 22-22: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 23-23: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
packages/mcp-server/src/services/code-generators/modules/thread-feature-js.ts (1)
11-16: Trailing comma in function call
..., users, )relies on ES2017 trailing-comma support in function calls. If the target environment includes older bundlers/runtimes this will throw. Drop the extra comma for broader compatibility.- users, + userspackages/mcp-server/src/services/vector/openai-embedding-provider.ts (1)
44-49: Clarify unusedmetadataparameter
embedDocumentsacceptsmetadatabut silently ignores it when delegating tolangchain– this can surprise integrators. Please either document that the argument is currently unused or pass it onceOpenAIEmbeddingssupports metadata.packages/mcp-server/src/services/code-generators/template-renderer.ts (1)
90-93: Remove redundantgenerateJsDoccall
generateJsDocis invoked but its output is immediately ignored, costing extra parsing on every render. The param list can be built directly fromgetMethodInfo:- const info = generateJsDoc(lang, className, methodName); // we actually need param list - const mi = getMethodInfo(className, methodName, lang); + const mi = getMethodInfo(className, methodName, lang);packages/mcp-server/src/common/otel.ts (1)
93-100: Ensure attribute values match OpenTelemetry spec
setSpanAttributesforwards arbitrary values; non-primitive objects will be dropped or throw in some exporters. Consider stringifying complex types or validating primitives before callingsetAttribute.packages/mcp-server/scripts/bench/search.ts (1)
88-91: Avoidunknowncast – preserve type safetyThe down-cast to
unknowndefeats compile-time checks:- await service.initialize(apiData as unknown as Map<string, unknown>); + await service.initialize(apiData);Adjust the
SearchServicesignature if needed rather than weakening types.packages/mcp-server/src/services/documentation/vector-service.ts (1)
448-451: Use optional chaining for cleaner optional call- if (this.adapter && (this.adapter as any).clearCollection) { - await (this.adapter as any).clearCollection(); + if (this.adapter?.clearCollection) { + await this.adapter.clearCollection(); }Eliminates the
anycast and reads more clearly.packages/mcp-server/src/services/code-generators/javascript-generator.ts (1)
7-11:threadImportsis imported but never usedThe imported symbol is currently unused in this file.
Either remove the import or incorporate it (e.g., prepend it to thegenerateThreadsFeature()output) to avoid TS/ES-lint “unused import” warnings.packages/mcp-server/src/tools.ts (1)
673-685: OptionalvectorWeightcan push total weight above 1When
vectorWeightis supplied explicitly, no guard preventstextWeight + vectorWeightfrom exceeding 1, potentially skewing hybrid scoring.
Consider validating that the sum ≤ 1 (or normalising internally).packages/mcp-server/src/services/code-generators/index.ts (1)
40-41: Loss of explicit type safety for supported languages
SupportedLanguageis nowstring; earlier it was a discriminated union.
This removes compile-time protection against typos when external callers pass a language.
If fully dynamic plugins are required, consider exportingtype SupportedLanguage = ReturnType<typeof getRegisteredLanguages>[number]for better intellisense.packages/mcp-server/src/services/search/search-service.ts (1)
213-216: Conflicting weight env-vars can yield sum ≠ 1.
API_TEXT_WEIGHT&API_VECTOR_WEIGHTmay both be set, producing e.g. 0.8 + 0.9 = 1.7.
Normalise or validate to keep the combined influence predictable.packages/mcp-server/src/services/generation/code-generation-service.ts (1)
83-90: Zod validation – duplicate work & missing feature dedup.Parsing
featureswithz.array(FeatureSchema)already guarantees each entry is valid; considerz.array(FeatureSchema).min(1).nonempty()andnew Set(parsedFeatures)to enforce presence & remove duplicates in one step.packages/mcp-server/src/api/parser.ts (2)
298-307:generateMethodKeymis-labels optionality for array/generic params.Only the top-level
optionalboolean is passed; array or generic markers (isArray,generics) are lost, leading to non-unique keys (stringvsstring[]). Include the derived signature fromparseType.
671-708: Unused parameters & weak constant typing.
namespace&languageare accepted but unused inparseTypeDefinition/parseConstant. Also,constType.optionalis hard-codedfalse; real optionality unknown. Consider:type: this.parseType(item.valueType ?? {name:'unknown', optional:false}),packages/mcp-server/src/services/documentation/documentation-index.ts (1)
50-52: Consider using a proper logger instead of console.log.While the informational logging is helpful, consider using a structured logger for better observability in production environments.
if (useHier) { - console.log('🔀 Using HierarchicalTextSplitter for MDX processing'); + // Use your logging framework here + logger.info('Using HierarchicalTextSplitter for MDX processing', { splitterType: 'hierarchical' }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (62)
.github/workflows/ci.yml(1 hunks)apps/chat/instrumentation.ts(1 hunks)apps/chat/package.json(1 hunks)package.json(2 hunks)packages/mcp-server/env.example(3 hunks)packages/mcp-server/package.json(3 hunks)packages/mcp-server/scripts/bench/search.ts(1 hunks)packages/mcp-server/scripts/vector/clear.ts(1 hunks)packages/mcp-server/src/api/parser.ts(8 hunks)packages/mcp-server/src/api/types.ts(1 hunks)packages/mcp-server/src/api/utils.ts(1 hunks)packages/mcp-server/src/common/event-bus.ts(1 hunks)packages/mcp-server/src/common/logger.ts(1 hunks)packages/mcp-server/src/common/metrics.ts(1 hunks)packages/mcp-server/src/common/otel.ts(1 hunks)packages/mcp-server/src/common/schemas.ts(1 hunks)packages/mcp-server/src/common/service-manager.ts(1 hunks)packages/mcp-server/src/server.ts(3 hunks)packages/mcp-server/src/services/api/api-search-service.ts(5 hunks)packages/mcp-server/src/services/code-generators/api-spec-loader.ts(1 hunks)packages/mcp-server/src/services/code-generators/csharp-template-generator.ts(1 hunks)packages/mcp-server/src/services/code-generators/index.ts(1 hunks)packages/mcp-server/src/services/code-generators/java-template-generator.ts(1 hunks)packages/mcp-server/src/services/code-generators/javascript-generator.ts(2 hunks)packages/mcp-server/src/services/code-generators/modules/thread-feature-js.ts(1 hunks)packages/mcp-server/src/services/code-generators/plugin-registry.ts(1 hunks)packages/mcp-server/src/services/code-generators/swift-template-generator.ts(1 hunks)packages/mcp-server/src/services/code-generators/template-renderer.ts(1 hunks)packages/mcp-server/src/services/code-generators/typescript-generator.ts(1 hunks)packages/mcp-server/src/services/documentation/documentation-index.ts(6 hunks)packages/mcp-server/src/services/documentation/frontmatter-schema.ts(1 hunks)packages/mcp-server/src/services/documentation/hierarchical-text-splitter.ts(1 hunks)packages/mcp-server/src/services/documentation/mdx-processor.ts(2 hunks)packages/mcp-server/src/services/documentation/vector-service.ts(9 hunks)packages/mcp-server/src/services/generation/code-generation-service.ts(10 hunks)packages/mcp-server/src/services/generation/workflow-generator-factory.ts(2 hunks)packages/mcp-server/src/services/knowledge/knowledge-service.ts(7 hunks)packages/mcp-server/src/services/search/__tests__/bm25-search.spec.ts(1 hunks)packages/mcp-server/src/services/search/__tests__/hybrid-search.spec.ts(1 hunks)packages/mcp-server/src/services/search/api-vector-service.ts(1 hunks)packages/mcp-server/src/services/search/basic-search-engine.ts(7 hunks)packages/mcp-server/src/services/search/core-search-engine.ts(1 hunks)packages/mcp-server/src/services/search/enhanced-search-engine.ts(0 hunks)packages/mcp-server/src/services/search/search-service.ts(5 hunks)packages/mcp-server/src/services/search/workflow-search-engine.ts(7 hunks)packages/mcp-server/src/services/search/workflow-templates.ts(1 hunks)packages/mcp-server/src/services/vector/adapter-factory.ts(1 hunks)packages/mcp-server/src/services/vector/embedding-provider.ts(1 hunks)packages/mcp-server/src/services/vector/openai-embedding-provider.ts(1 hunks)packages/mcp-server/src/services/vector/qdrant-adapter.ts(1 hunks)packages/mcp-server/src/services/vector/vector-adapter.ts(1 hunks)packages/mcp-server/src/services/workflow/interactive-session-service.ts(2 hunks)packages/mcp-server/src/templates/codegen/java/setup.hbs(1 hunks)packages/mcp-server/src/templates/codegen/javascript/partials/crypto.hbs(1 hunks)packages/mcp-server/src/templates/codegen/javascript/partials/inbox.hbs(1 hunks)packages/mcp-server/src/templates/codegen/javascript/partials/store.hbs(1 hunks)packages/mcp-server/src/templates/codegen/javascript/partials/thread.hbs(1 hunks)packages/mcp-server/src/templates/codegen/javascript/setup.hbs(1 hunks)packages/mcp-server/src/templates/codegen/swift/setup.hbs(1 hunks)packages/mcp-server/src/templates/codegen/typescript/setup.hbs(1 hunks)packages/mcp-server/src/tools.ts(4 hunks)typedoc.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/mcp-server/src/services/search/enhanced-search-engine.ts
🧰 Additional context used
🧬 Code Graph Analysis (22)
packages/mcp-server/scripts/vector/clear.ts (1)
packages/mcp-server/src/services/vector/qdrant-adapter.ts (1)
QdrantVectorAdapter(23-163)
packages/mcp-server/src/server.ts (2)
packages/mcp-server/src/common/metrics.ts (1)
initMetricsServer(45-65)packages/mcp-server/src/common/otel.ts (1)
startSpan(72-91)
packages/mcp-server/src/services/workflow/interactive-session-service.ts (1)
apps/chat/src/lib/services/service-manager.ts (1)
ServiceManager(27-281)
packages/mcp-server/src/services/search/__tests__/hybrid-search.spec.ts (2)
packages/mcp-server/src/api/types.ts (2)
APIMethod(26-48)APINamespace(89-102)packages/mcp-server/src/services/search/search-service.ts (1)
SearchService(18-243)
packages/mcp-server/src/services/vector/adapter-factory.ts (2)
packages/mcp-server/src/services/vector/vector-adapter.ts (1)
VectorStoreAdapter(11-42)packages/mcp-server/src/services/vector/qdrant-adapter.ts (1)
QdrantVectorAdapter(23-163)
packages/mcp-server/src/services/code-generators/java-template-generator.ts (2)
packages/mcp-server/src/services/code-generators/index.ts (2)
JavaTemplateGenerator(71-71)BaseCodeGenerator(69-69)packages/mcp-server/src/services/code-generators/template-renderer.ts (1)
renderTemplate(34-47)
packages/mcp-server/src/services/search/workflow-templates.ts (2)
packages/mcp-server/src/services/search/workflow-search-engine.ts (4)
createMessagingWorkflow(325-327)createFileStorageWorkflow(332-334)createInboxWorkflow(336-338)createEventHandlingWorkflow(340-342)packages/mcp-server/src/types/index.ts (1)
WorkflowSuggestion(165-173)
packages/mcp-server/src/services/code-generators/csharp-template-generator.ts (2)
packages/mcp-server/src/services/code-generators/index.ts (2)
CSharpTemplateGenerator(73-73)BaseCodeGenerator(69-69)packages/mcp-server/src/services/code-generators/template-renderer.ts (1)
renderTemplate(34-47)
packages/mcp-server/src/services/vector/vector-adapter.ts (1)
packages/mcp-server/src/types/documentation-types.ts (1)
VectorSearchResult(328-341)
packages/mcp-server/src/services/vector/openai-embedding-provider.ts (1)
packages/mcp-server/src/services/vector/embedding-provider.ts (1)
EmbeddingProvider(6-21)
packages/mcp-server/src/services/knowledge/knowledge-service.ts (1)
packages/mcp-server/src/common/otel.ts (1)
startSpan(72-91)
packages/mcp-server/src/services/code-generators/typescript-generator.ts (3)
packages/mcp-server/src/services/code-generators/index.ts (2)
TypeScriptGenerator(74-74)JavaScriptGenerator(70-70)packages/mcp-server/src/services/code-generators/javascript-generator.ts (1)
JavaScriptGenerator(14-270)packages/mcp-server/src/services/code-generators/template-renderer.ts (1)
renderTemplate(34-47)
packages/mcp-server/src/services/documentation/vector-service.ts (3)
packages/mcp-server/src/services/vector/vector-adapter.ts (1)
VectorStoreAdapter(11-42)packages/mcp-server/src/common/otel.ts (1)
startSpan(72-91)packages/mcp-server/src/types/documentation-types.ts (1)
VectorSearchResult(328-341)
packages/mcp-server/src/services/code-generators/javascript-generator.ts (2)
packages/mcp-server/src/services/code-generators/template-renderer.ts (1)
renderTemplate(34-47)packages/mcp-server/src/services/code-generators/modules/thread-feature-js.ts (2)
threadImplementation(5-19)threadExample(21-25)
packages/mcp-server/src/services/search/api-vector-service.ts (3)
packages/mcp-server/src/config/vector-config.ts (1)
getVectorConfig(65-70)packages/mcp-server/src/api/types.ts (3)
APINamespace(89-102)APIMethod(26-48)APIClass(50-69)packages/mcp-server/src/common/otel.ts (1)
startSpan(72-91)
packages/mcp-server/src/services/code-generators/template-renderer.ts (1)
packages/mcp-server/src/services/code-generators/api-spec-loader.ts (4)
getMethodSnippet(75-82)generateJsDoc(84-101)getMethodInfo(55-73)getReturnType(103-111)
packages/mcp-server/src/services/search/basic-search-engine.ts (1)
packages/mcp-server/src/types/index.ts (1)
SearchResult(66-77)
packages/mcp-server/src/tools.ts (1)
packages/mcp-server/src/common/schemas.ts (1)
FeatureSchema(20-20)
packages/mcp-server/src/services/api/api-search-service.ts (3)
packages/mcp-server/src/services/search/search-service.ts (1)
SearchService(18-243)packages/mcp-server/src/types/index.ts (1)
SearchResult(66-77)packages/mcp-server/src/common/otel.ts (2)
startSpan(72-91)setSpanAttributes(93-100)
packages/mcp-server/src/services/generation/code-generation-service.ts (6)
packages/mcp-server/src/common/schemas.ts (2)
LanguageSchema(3-10)FeatureSchema(20-20)packages/mcp-server/src/services/code-generators/index.ts (2)
isLanguageSupported(63-65)getSupportedLanguages(56-58)packages/mcp-server/src/services/knowledge/knowledge-repository.ts (1)
getSupportedLanguages(114-116)packages/mcp-server/src/common/metrics.ts (2)
codegenCounter(37-42)codegenDuration(29-35)packages/mcp-server/src/common/otel.ts (1)
startSpan(72-91)packages/mcp-server/src/services/generation/generation-types.ts (1)
WorkflowRequest(40-45)
packages/mcp-server/src/services/vector/qdrant-adapter.ts (4)
packages/mcp-server/src/services/vector/vector-adapter.ts (1)
VectorStoreAdapter(11-42)packages/mcp-server/src/services/vector/embedding-provider.ts (1)
EmbeddingProvider(6-21)packages/mcp-server/src/services/vector/openai-embedding-provider.ts (1)
OpenAIEmbeddingProvider(11-59)packages/mcp-server/src/types/documentation-types.ts (1)
VectorSearchResult(328-341)
packages/mcp-server/src/api/parser.ts (3)
packages/mcp-server/src/api/types.ts (3)
APIConstant(104-109)APITypeDefinition(111-116)APIType(14-19)packages/mcp-server/src/types/index.ts (3)
APIConstant(57-57)APITypeDefinition(58-58)APIType(51-51)packages/mcp-server/src/api/utils.ts (1)
generateMethodKey(1-13)
🪛 Biome (1.9.4)
packages/mcp-server/src/services/search/__tests__/bm25-search.spec.ts
[error] 12-12: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/mcp-server/src/common/service-manager.ts
[error] 22-22: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 22-22: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 23-23: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
packages/mcp-server/src/services/documentation/hierarchical-text-splitter.ts
[error] 11-14: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🪛 actionlint (1.7.7)
.github/workflows/ci.yml
19-19: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
35-35: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
43-43: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cursor BugBot
🔇 Additional comments (24)
package.json (1)
28-30: Typedoc integration looks good – double-check CI & cacheThe new
docs:apiscript andtypedocdevDependency are straightforward.
Two quick confirmations before this merges:
- The CI job (added in
.github/workflows/ci.yml) should cachedocs/apionly as an artifact, not commit it.- If your turborepo pipeline uses incremental hashing, add
typedoc.jsonto the pipeline’s cache inputs so documentation is rebuilt when the config changes.No blocking issues—just pointing these out so we don’t end up with stale docs.
Also applies to: 45-46
packages/mcp-server/src/services/code-generators/plugin-registry.ts (1)
24-31:process.cwd()can point to the wrong workspace folderIn a monorepo the working directory may be
packages/mcp-serverwhilenode_moduleslives at the repo root, causing silent plugin miss-detection.
Consider usingrequire.resolve('.')or walking up the directory tree untilnode_modulesis found (similar to howpkg-upworks) to make discovery location-agnostic.packages/mcp-server/src/api/types.ts (1)
26-34:Details
❓ Verification inconclusive
keyfield is required – ensure all producers are updated
keyis now mandatory, which will break any place that instantiatesAPIMethodwithout it.
Please grep the codebase for object literals of shape{ name:, description:, … }to ensure they includekey.
🏁 Script executed:
#!/bin/bash # List potential offenders (missing "key" property) for manual inspection rg -t ts -n $'{\\sname:\\s.*\\n[^}]*}' | grep -v '"key"' | head -n 50Length of output: 163
Verify all APIMethod literals include the new
keyfieldThe
keyproperty onAPIMethodis now required. Please ensure every object literal that defines an API method includeskey, or update those producers accordingly. Automated search with the previous regex failed, so manual review is needed:• Search for any object literal containing the properties
name,description,snippet,methodType,parameters, andreturns.
• Confirm each of these literals also has akeyentry.
• Pay special attention to functions or constants annotated with: APIMethodthat return or assign an object.packages/mcp-server/scripts/vector/clear.ts (1)
1-2: Importing.jsfrom a.tsfile breaks when invoked viats-nodeWith
ts-nodethe file extension must point to an existing source file.
Either drop the extension or use.tsso that runtime resolution succeeds:-import { QdrantVectorAdapter } from '../../src/services/vector/qdrant-adapter.js'; +import { QdrantVectorAdapter } from '../../src/services/vector/qdrant-adapter.js'; // after build +// or, for direct ts-node execution: +// import { QdrantVectorAdapter } from '../../src/services/vector/qdrant-adapter.ts';Verify which execution path (built JS vs. ts-node) your tooling uses and adjust accordingly.
packages/mcp-server/src/services/search/core-search-engine.ts (1)
1-1: LGTM – thin re-export is clearRe-exporting keeps import paths stable with zero overhead.
packages/mcp-server/src/services/search/__tests__/bm25-search.spec.ts (1)
15-17: Potential test pollution – isolate env-flag mutations per testOnly
beforeAll/afterAllare used, so parallel Jest runs (or other spec files executed after this one) will still observeUSE_BM25='true'while this suite executes. PreferbeforeEach/afterEachor wrap insidejest.isolateModulesto guarantee isolation.packages/mcp-server/src/server.ts (1)
87-89:initMetricsServer()may bind the port twice in multi-process setupsBecause it’s invoked from the constructor, any accidental second instantiation of
PrivMXMCPServer(e.g., in tests) will try to reuse the same port and crash.
Expose the function to the caller or protect with a singleton guard insideinitMetricsServer.packages/mcp-server/src/services/code-generators/typescript-generator.ts (1)
4-15: Catch block swallows unexpected errorsThe blanket
catchmasks real template parsing errors unrelated to “missing file”. Narrow the guard by first checkingfs.existsSyncon the template path, or re-throw when the error code isn’tENOENT..github/workflows/ci.yml (2)
18-23: Bump marketplace actions to latest major versions
actions/setup-node@v3is flagged by actionlint as deprecated; likewiseactions/upload-artifact@v3. Migrating avoids sudden CI breakage when GitHub retires older runners.- - name: Use Node.js ${{ matrix.node-version }} - uses: actions/setup-node@v3 + - name: Use Node.js ${{ matrix.node-version }} + uses: actions/setup-node@v4 … - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4🧰 Tools
🪛 actionlint (1.7.7)
19-19: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
30-38: Coverage artefact path may be empty
jest --coveragedefaults tocoverage/at repo root, but the upload step points topackages/mcp-server/coverage. If no customcoverageDirectoryis configured the artefact upload will silently fail.Confirm jest config or adjust the path accordingly.
🧰 Tools
🪛 actionlint (1.7.7)
35-35: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
packages/mcp-server/src/api/utils.ts (1)
8-13: Function looks goodThe implementation is concise and covers optional parameters correctly. 👍
packages/mcp-server/src/templates/codegen/javascript/partials/crypto.hbs (1)
13-16: Undeclared parameters in template body
signDatareferencesdataandprivateKeybut the argument list is produced by{{paramList …}}. If that helper emits destructuring ({ data, privateKey }) the internal call must use the same identifiers; otherwise it will beundefined. Verify helper output or map explicitly:packages/mcp-server/src/common/logger.ts (1)
4-21: Verify Pino version & optional dependency availability
pino.stdTimeFunctions.isoTimeis only available from Pino v8.
If your runtime still ships with ≤v7 the import will crash at startup.
Likewise, the dev-only transport requirespino-pretty; missing it yields a runtime error.Please confirm that
package.jsonpinspino ^8and listspino-prettyindevDependencies, or add a guarded dynamic import.packages/mcp-server/src/services/documentation/mdx-processor.ts (1)
48-66: Good use of OpenTelemetry span and Zod validation
Wrapping the whole parse flow instartSpanand soft-failing on front-matter issues gives observability without breaking docs ingestion 👍packages/mcp-server/src/services/documentation/hierarchical-text-splitter.ts (1)
22-34: Heading text extraction may crash on non-text child nodes
(node as any).children.map(c => c.value)assumes every child has.value.
When the heading contains emphasis/inlineCode the value is nested deeper, causingundefinedentries.Consider using
unist-util-visitto collecttextnodes inside the heading or fallback totoString(node).packages/mcp-server/src/services/vector/vector-adapter.ts (1)
37-42: Interface looks solidThe abstraction is clean and leaves room for future adapters (Pinecone, FAISS, etc.).
OptionalclearCollectionis a nice touch.packages/mcp-server/env.example (1)
104-113: Warn that the weight variables must sum to 1The comment says “should sum to 1” but nothing enforces it.
Mention that the server will normalise or throw, whichever behaviour you implemented, so ops teams know what to expect.packages/mcp-server/src/services/knowledge/knowledge-service.ts (1)
87-105: Great use ofstartSpanwrapperThe new tracing calls are concise and readable—nice job integrating OpenTelemetry without cluttering business logic.
packages/mcp-server/src/templates/codegen/java/setup.hbs (1)
30-40:Details
❓ Verification inconclusive
Stub signatures may not match actual API
createThread/createStorereturnStringwith placeholder body, while snippets likely return complex objects (e.g.Thread). Mismatched types will break compilation once helpers replace the stub comment. Align the return type with the real API or keep the methodvoidwith aTODOto avoid compile errors.
Ensure Java stub signatures match actual API
The stubs in packages/mcp-server/src/templates/codegen/java/setup.hbs (lines 30–40) default to returningString, but the realThreadApi.createThreadandStoreApi.createStoremethods will likely return model objects (e.g.Thread,Store) rather than a plain string. This mismatch will cause compile errors when the snippet helper injects the real method bodies.• Verify the actual return types in your API spec or generated code.
• Update the stubs’ return types to match (e.g.Thread,Store, etc.), or usevoidwith a// TODObody to prevent compilation failures.packages/mcp-server/src/services/code-generators/modules/thread-feature-js.ts (1)
3-3:threadImportscurrently only injects a commentIf the generated file expects real imports (e.g.
import { ThreadApi } from '@privmx/sdk';) this placeholder will compile but fail at runtime. Confirm that a build-time template step injects the actual import lines.packages/mcp-server/src/services/search/api-vector-service.ts (1)
68-70:addDocumentswithout upsert handlingIf the collection already contains the same IDs,
addDocumentswill error. Useupsert=trueor delete first to make initialisation idempotent.packages/mcp-server/src/services/code-generators/template-renderer.ts (1)
38-42: Path resolution may break on Windows
new URL(import.meta.url).pathnamecan contain a leading/and URL-encoded characters on Windows. ConsiderfileURLToPath(import.meta.url)fromurlto produce a clean, OS-safe path.packages/mcp-server/src/services/code-generators/javascript-generator.ts (1)
22-26: Missing thread-level imports in generated snippet
generateThreadsFeature()returns onlythreadImplementation.
If that implementation relies on additional imports (e.g., the unusedthreadImportsabove), the generated code will be incomplete and fail to compile.
Double-check the template or append the required imports inside this method.packages/mcp-server/src/services/search/basic-search-engine.ts (1)
238-245: Potential key collision usingmethod.key
method.keyis used as the sole map key but may not be globally unique across namespaces/languages.
If two methods share the same key string in different namespaces, one list will hold both, but retrieval by ID later is namespace-specific, which could give misleading matches.
Consider prefixing with${language}:${namespace}:for uniqueness.
| UserResponse, | ||
| UserContext, | ||
| } from '../../types/mcp-types.js'; | ||
| import ServiceManager from '../../common/service-manager.js'; |
There was a problem hiding this comment.
Incorrect import – ServiceManager is a named export
service-manager.ts exports the class, not a default. The default-import form produces undefined at runtime.
-import ServiceManager from '../../common/service-manager.js';
+import { ServiceManager } from '../../common/service-manager.js';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import ServiceManager from '../../common/service-manager.js'; | |
| -import ServiceManager from '../../common/service-manager.js'; | |
| +import { ServiceManager } from '../../common/service-manager.js'; |
🤖 Prompt for AI Agents
In packages/mcp-server/src/services/workflow/interactive-session-service.ts at
line 15, the import of ServiceManager uses a default import but ServiceManager
is a named export. Change the import statement to use named import syntax with
curly braces around ServiceManager to correctly import the class and avoid
runtime undefined errors.
packages/mcp-server/src/services/search/workflow-search-engine.ts
Outdated
Show resolved
Hide resolved
packages/mcp-server/src/services/documentation/documentation-index.ts
Outdated
Show resolved
Hide resolved
packages/mcp-server/src/services/documentation/documentation-index.ts
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/mcp-server/src/common/event-bus.ts (1)
9-17: TypedEventEmitterstill not used – IDE safety missing
You addedMcpEvents, buteventBusis still created as an un-typedEventEmitter, so TypeScript will not warn about misspelled event names (exactly the issue raised in the previous review).-const eventBus = new EventEmitter(); - -export default eventBus as EventEmitter; +// Node ≥18 type definitions allow generics: EventEmitter<EventMap> +const eventBus = new EventEmitter<McpEvents>(); + +export default eventBus;🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 12-12: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 13-13: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 14-14: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 15-15: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🧹 Nitpick comments (9)
packages/mcp-server/src/common/event-bus.ts (1)
10-16:voidin interface values trips Biome linter
voidoutside a return position is flagged as “confusing”. Useundefinedwhich has identical runtime behaviour but satisfies the linter.- 'vector.initialized': void; - 'vector.index.start': void; - 'vector.index.complete': void; - 'search.started': void; - 'search.completed': void; + 'vector.initialized': undefined; + 'vector.index.start': undefined; + 'vector.index.complete': undefined; + 'search.started': undefined; + 'search.completed': undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 12-12: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 13-13: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 14-14: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 15-15: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/mcp-server/src/services/documentation/hierarchical-text-splitter.ts (2)
11-14: Useless constructor – drop for terser class
The constructor does nothing except pass its arguments tosuper. Omitting it makes the file smaller and removes the Biome warning.-export class HierarchicalTextSplitter extends TextSplitter { - constructor(options: { chunkSize: number; chunkOverlap: number }) { - super(options); - } +export class HierarchicalTextSplitter extends TextSplitter {🧰 Tools
🪛 Biome (1.9.4)
[error] 11-14: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
42-53: Potential oversize final chunk
The last push (chunks.push(buffer.join('\n'))) can exceedthis.chunkSizebecause no length check is done after the loop. You might want to guard it the same way you do inside the loop to guarantee all chunks respectchunkSize.packages/mcp-server/src/common/service-manager.ts (1)
27-30: Minor: use class name in static getter for clarity
thisin a static context is valid but flagged by Biome.ServiceManagerreads clearer.- if (!this._instance) this._instance = new ServiceManager(); - return this._instance; + if (!ServiceManager._instance) ServiceManager._instance = new ServiceManager(); + return ServiceManager._instance;🧰 Tools
🪛 Biome (1.9.4)
[error] 28-28: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 28-28: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 29-29: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
packages/mcp-server/src/services/search/workflow-search-engine.ts (1)
328-337: Unused_languageparam – remove or use
createMessagingWorkflow(_language?: string)takes a language it never uses. Either drop the parameter or forward it tocreateMessagingWorkflowto avoid confusion.packages/mcp-server/src/services/search/search-service.ts (1)
213-217: Weight sanity check
API_TEXT_WEIGHTcan be set to >1 or <0 which would makevectorWeightnegative. Clamp or validate to keep weights in [0,1].const lexicalWeight = clamp(Number(process.env.API_TEXT_WEIGHT ?? '0.5'), 0, 1); const vectorWeight = 1 - lexicalWeight;(implement
clampor inline the Math.)packages/mcp-server/src/api/parser.ts (1)
696-705:valueTypeprobing is fragile – fall back gracefully
parseConstantrelies on(item as any).valueType?.name.
When the source spec omitsvalueType,namewill beundefined, leaking an"unknown"type downstream. Consider inferring the type from the literal value instead (number, string, boolean).- const constType: APIType = { - name: (item as any).valueType?.name || 'unknown', - optional: false, - } as APIType; + const inferred = + typeof (item as any).value === 'number' + ? 'number' + : typeof (item as any).value === 'boolean' + ? 'boolean' + : 'string'; + + const constType: APIType = { name: inferred, optional: false };packages/mcp-server/src/services/documentation/documentation-index.ts (2)
140-178: Span error recording should preserve semantic contextGreat to see telemetry!
Minor nit: you calltrace.getActiveSpan()and then record the exception, but forget to add custom attributes (you add them later). Consider combining for clarity:const span = trace.getActiveSpan(); if (span) { span.recordException(error); span.setStatus({ code: SpanStatusCode.ERROR, message: 'Semantic search failed' }); span.setAttribute('semanticSearchFailed', true); }Keeps span decoration in one place.
225-240: Zero-semantic path still allocates weightWhen the semantic engine is unavailable (or failed)
semWmay still be non-zero, inflating lexical scores (current + sem.score * semWwheresem.scoreis missing).
Option: ifsemanticResults.length === 0, forcesemW = 0andlexW = 1to avoid accidental dampening.Not critical but improves score interpretability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/chat/package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
.github/workflows/ci.yml(1 hunks)packages/mcp-server/package.json(3 hunks)packages/mcp-server/scripts/vector/clear.ts(1 hunks)packages/mcp-server/src/api/parser.ts(8 hunks)packages/mcp-server/src/common/event-bus.ts(1 hunks)packages/mcp-server/src/common/service-manager.ts(1 hunks)packages/mcp-server/src/services/api/api-search-service.ts(5 hunks)packages/mcp-server/src/services/code-generators/plugin-registry.ts(1 hunks)packages/mcp-server/src/services/documentation/documentation-index.ts(7 hunks)packages/mcp-server/src/services/documentation/hierarchical-text-splitter.ts(1 hunks)packages/mcp-server/src/services/documentation/mdx-processor.ts(2 hunks)packages/mcp-server/src/services/documentation/vector-service.ts(8 hunks)packages/mcp-server/src/services/generation/code-generation-service.ts(10 hunks)packages/mcp-server/src/services/generation/workflow-generator-factory.ts(2 hunks)packages/mcp-server/src/services/knowledge/knowledge-service.ts(8 hunks)packages/mcp-server/src/services/search/api-vector-service.ts(1 hunks)packages/mcp-server/src/services/search/basic-search-engine.ts(7 hunks)packages/mcp-server/src/services/search/search-service.ts(5 hunks)packages/mcp-server/src/services/search/workflow-search-engine.ts(7 hunks)packages/mcp-server/src/services/search/workflow-templates.ts(1 hunks)packages/mcp-server/src/services/vector/embedding-provider.ts(1 hunks)packages/mcp-server/src/services/vector/qdrant-adapter.ts(1 hunks)packages/mcp-server/src/services/workflow/interactive-session-service.ts(2 hunks)packages/mcp-server/src/templates/codegen/javascript/partials/crypto.hbs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/mcp-server/scripts/vector/clear.ts
- .github/workflows/ci.yml
- packages/mcp-server/src/services/generation/workflow-generator-factory.ts
- packages/mcp-server/src/services/documentation/mdx-processor.ts
- packages/mcp-server/src/services/vector/embedding-provider.ts
- packages/mcp-server/src/templates/codegen/javascript/partials/crypto.hbs
- packages/mcp-server/src/services/search/workflow-templates.ts
- packages/mcp-server/src/services/knowledge/knowledge-service.ts
- packages/mcp-server/src/services/code-generators/plugin-registry.ts
- packages/mcp-server/package.json
- packages/mcp-server/src/services/workflow/interactive-session-service.ts
- packages/mcp-server/src/services/search/api-vector-service.ts
- packages/mcp-server/src/services/search/basic-search-engine.ts
- packages/mcp-server/src/services/vector/qdrant-adapter.ts
- packages/mcp-server/src/services/documentation/vector-service.ts
- packages/mcp-server/src/services/api/api-search-service.ts
- packages/mcp-server/src/services/generation/code-generation-service.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/mcp-server/src/common/service-manager.ts
[error] 28-28: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 28-28: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 29-29: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
packages/mcp-server/src/common/event-bus.ts
[error] 11-11: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 12-12: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 13-13: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 14-14: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 15-15: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/mcp-server/src/services/documentation/hierarchical-text-splitter.ts
[error] 11-14: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
| this._knowledgeInit = (async () => { | ||
| const service = new KnowledgeService(); | ||
| await service.initialize(); | ||
| this._knowledgeService = service; | ||
| return service; | ||
| })(); | ||
|
|
||
| return this._knowledgeInit; |
There was a problem hiding this comment.
Initialization failure leaves singleton in broken state
If KnowledgeService.initialize() (or the analogous calls for the other services) throws, _knowledgeInit keeps a rejected promise. All subsequent calls will immediately reject and the manager can never recover without a process restart.
this._knowledgeInit = (async () => {
try {
const service = new KnowledgeService();
await service.initialize();
this._knowledgeService = service;
return service;
} finally {
- // nothing
+ // Reset so a new attempt can be made after a failure
+ if (!this._knowledgeService) {
+ this._knowledgeInit = undefined;
+ }
}
})();Apply the same pattern to _searchInit, _codeGenInit, and _sessionInit.
🤖 Prompt for AI Agents
In packages/mcp-server/src/common/service-manager.ts around lines 40 to 47, the
current initialization pattern assigns a rejected promise to _knowledgeInit if
KnowledgeService.initialize() throws, causing the singleton to remain in a
broken state. To fix this, wrap the initialization logic in a try-catch block
inside the async IIFE, reset _knowledgeInit to undefined on failure to allow
retries, and rethrow the error. Apply this same pattern to _searchInit,
_codeGenInit, and _sessionInit to ensure all service initializations can recover
from failures.
| for (const r of lexicalResults) { | ||
| const norm = r.score / (maxLex || 1); | ||
| combined.set(r.id, { res: r, score: norm * lexicalWeight }); | ||
| } | ||
|
|
||
| for (const s of semanticRes) { | ||
| const existing = combined.get(s.id); | ||
| if (existing) { | ||
| existing.score += s.score * vectorWeight; | ||
| } else { | ||
| const lex = lexicalResults.find((r) => r.id === s.id); | ||
| if (lex) | ||
| combined.set(lex.id, { res: lex, score: s.score * vectorWeight }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Semantic-only hits still discarded – hybrid search incomplete
hybridSearch ignores vector results that are absent from lexical results (same bug fixed in SearchService, still present here). Users will never see purely semantic matches.
- } else {
- const lex = lexicalResults.find((r) => r.id === s.id);
- if (lex)
- combined.set(lex.id, { res: lex, score: s.score * vectorWeight });
+ } else {
+ const lex = lexicalResults.find((r) => r.id === s.id);
+ if (lex) {
+ combined.set(lex.id, { res: lex, score: s.score * vectorWeight });
+ } else {
+ // promote pure semantic hit
+ combined.set(s.id, { res: s, score: s.score * vectorWeight });
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const r of lexicalResults) { | |
| const norm = r.score / (maxLex || 1); | |
| combined.set(r.id, { res: r, score: norm * lexicalWeight }); | |
| } | |
| for (const s of semanticRes) { | |
| const existing = combined.get(s.id); | |
| if (existing) { | |
| existing.score += s.score * vectorWeight; | |
| } else { | |
| const lex = lexicalResults.find((r) => r.id === s.id); | |
| if (lex) | |
| combined.set(lex.id, { res: lex, score: s.score * vectorWeight }); | |
| } | |
| } | |
| for (const r of lexicalResults) { | |
| const norm = r.score / (maxLex || 1); | |
| combined.set(r.id, { res: r, score: norm * lexicalWeight }); | |
| } | |
| for (const s of semanticRes) { | |
| const existing = combined.get(s.id); | |
| if (existing) { | |
| existing.score += s.score * vectorWeight; | |
| } else { | |
| const lex = lexicalResults.find((r) => r.id === s.id); | |
| if (lex) { | |
| combined.set(lex.id, { res: lex, score: s.score * vectorWeight }); | |
| } else { | |
| // promote pure semantic hit | |
| combined.set(s.id, { res: s, score: s.score * vectorWeight }); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/mcp-server/src/services/search/workflow-search-engine.ts between
lines 602 and 616, the current code discards semantic-only search results by
only adding semantic scores to existing lexical results or those found in
lexicalResults. To fix this, modify the loop over semanticRes to add
semantic-only results to the combined map even if they don't exist in
lexicalResults, ensuring purely semantic matches are included in the final
results.
|
|
||
| private parseTypeDefinition( | ||
| item: RawAPISpec[string][0]['content'][0], | ||
| namespace: string, | ||
| language: string | ||
| ): APITypeDefinition | null { | ||
| if (!item || item.type !== 'type') return null; | ||
|
|
||
| const def: APITypeDefinition = { | ||
| name: item.name, | ||
| description: item.description || `${item.name} type`, | ||
| definition: item.snippet || `type ${item.name} = any`, | ||
| language, | ||
| }; | ||
|
|
||
| return def; | ||
| } | ||
|
|
||
| private parseConstant( | ||
| item: RawAPISpec[string][0]['content'][0], | ||
| namespace: string, | ||
| language: string | ||
| ): APIConstant | null { | ||
| if (!item || item.type !== 'constant') return null; | ||
|
|
||
| const constType: APIType = { | ||
| name: (item as any).valueType?.name || 'unknown', | ||
| optional: false, | ||
| } as APIType; | ||
|
|
||
| const constant: APIConstant = { | ||
| name: item.name, | ||
| description: item.description || `${item.name} constant`, | ||
| type: constType, | ||
| value: (item as any).value ?? null, | ||
| }; | ||
|
|
||
| return constant; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Namespace information is silently discarded and the namespace parameter is unused
parseTypeDefinition and parseConstant both accept a namespace argument yet never use it.
If APITypeDefinition / APIConstant are expected to carry the namespace (very likely, given the rest of the model), this omission will:
- Lose important contextual data for later consumers (search, telemetry, code-gen).
- Trigger the
noUnusedParameters/noUnusedLocalscompiler rules.
Patch suggestion:
- private parseTypeDefinition(
- item: RawAPISpec[string][0]['content'][0],
- namespace: string,
- language: string
+ private parseTypeDefinition(
+ item: RawAPISpec[string][0]['content'][0],
+ namespace: string,
+ language: string
): APITypeDefinition | null {
if (!item || item.type !== 'type') return null;
const def: APITypeDefinition = {
name: item.name,
description: item.description || `${item.name} type`,
definition: item.snippet || `type ${item.name} = any`,
+ namespace,
language,
};Do the same for parseConstant.
This both removes the unused-parameter lint error and keeps the data complete.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private parseTypeDefinition( | |
| item: RawAPISpec[string][0]['content'][0], | |
| namespace: string, | |
| language: string | |
| ): APITypeDefinition | null { | |
| if (!item || item.type !== 'type') return null; | |
| const def: APITypeDefinition = { | |
| name: item.name, | |
| description: item.description || `${item.name} type`, | |
| definition: item.snippet || `type ${item.name} = any`, | |
| language, | |
| }; | |
| return def; | |
| } | |
| private parseConstant( | |
| item: RawAPISpec[string][0]['content'][0], | |
| namespace: string, | |
| language: string | |
| ): APIConstant | null { | |
| if (!item || item.type !== 'constant') return null; | |
| const constType: APIType = { | |
| name: (item as any).valueType?.name || 'unknown', | |
| optional: false, | |
| } as APIType; | |
| const constant: APIConstant = { | |
| name: item.name, | |
| description: item.description || `${item.name} constant`, | |
| type: constType, | |
| value: (item as any).value ?? null, | |
| }; | |
| return constant; | |
| } | |
| private parseTypeDefinition( | |
| item: RawAPISpec[string][0]['content'][0], | |
| namespace: string, | |
| language: string | |
| ): APITypeDefinition | null { | |
| if (!item || item.type !== 'type') return null; | |
| const def: APITypeDefinition = { | |
| name: item.name, | |
| description: item.description || `${item.name} type`, | |
| definition: item.snippet || `type ${item.name} = any`, | |
| namespace, | |
| language, | |
| }; | |
| return def; | |
| } |
🤖 Prompt for AI Agents
In packages/mcp-server/src/api/parser.ts between lines 670 and 708, the
namespace parameter is accepted by parseTypeDefinition and parseConstant but
never used, causing loss of context and lint errors. To fix this, add the
namespace property to the returned APITypeDefinition and APIConstant objects
respectively, assigning it the value of the namespace parameter. This will
preserve the namespace information and eliminate the unused parameter warnings.
| // 3. Combine scores | ||
| const lexicalWeight = Math.max( | ||
| 0, | ||
| Math.min(1, Number(process.env.TEXT_WEIGHT ?? DEFAULT_LEXICAL_WEIGHT)) | ||
| ); | ||
| const semanticWeight = Math.max( | ||
| 0, | ||
| Math.min( | ||
| 1, | ||
| Number(process.env.VECTOR_WEIGHT ?? DEFAULT_SEMANTIC_WEIGHT) | ||
| ) | ||
| ); | ||
|
|
There was a problem hiding this comment.
Environment weight parsing can yield NaN, breaking score normalisation
Number(process.env.TEXT_WEIGHT) (and similar) returns NaN for malformed or empty values.
Math.max/Math.min propagate NaN, so lexicalWeight, semanticWeight, weightSum, lexW, semW all become NaN, producing NaN relevance scores and undefined sort order.
Robust fix:
-const lexicalWeight = Math.max(
- 0,
- Math.min(1, Number(process.env.TEXT_WEIGHT ?? DEFAULT_LEXICAL_WEIGHT))
-);
-const semanticWeight = Math.max(
- 0,
- Math.min(
- 1,
- Number(process.env.VECTOR_WEIGHT ?? DEFAULT_SEMANTIC_WEIGHT)
- )
-);
+const parseWeight = (envVar: string | undefined, fallback: number) => {
+ const n = Number(envVar);
+ return Number.isFinite(n) ? Math.max(0, Math.min(1, n)) : fallback;
+};
+
+const lexicalWeight = parseWeight(process.env.TEXT_WEIGHT, DEFAULT_LEXICAL_WEIGHT);
+const semanticWeight = parseWeight(process.env.VECTOR_WEIGHT, DEFAULT_SEMANTIC_WEIGHT);This guarantees finite weights and prevents NaN contamination of downstream maths.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 3. Combine scores | |
| const lexicalWeight = Math.max( | |
| 0, | |
| Math.min(1, Number(process.env.TEXT_WEIGHT ?? DEFAULT_LEXICAL_WEIGHT)) | |
| ); | |
| const semanticWeight = Math.max( | |
| 0, | |
| Math.min( | |
| 1, | |
| Number(process.env.VECTOR_WEIGHT ?? DEFAULT_SEMANTIC_WEIGHT) | |
| ) | |
| ); | |
| // 3. Combine scores | |
| const parseWeight = (envVar: string | undefined, fallback: number) => { | |
| const n = Number(envVar); | |
| return Number.isFinite(n) ? Math.max(0, Math.min(1, n)) : fallback; | |
| }; | |
| const lexicalWeight = parseWeight( | |
| process.env.TEXT_WEIGHT, | |
| DEFAULT_LEXICAL_WEIGHT | |
| ); | |
| const semanticWeight = parseWeight( | |
| process.env.VECTOR_WEIGHT, | |
| DEFAULT_SEMANTIC_WEIGHT | |
| ); |
🤖 Prompt for AI Agents
In packages/mcp-server/src/services/documentation/documentation-index.ts around
lines 196 to 208, the parsing of environment variables for weights can produce
NaN if the values are malformed or empty, causing all subsequent calculations to
become NaN. To fix this, validate the parsed numbers by checking if they are
finite; if not, fallback to the default weight constants before applying
Math.max and Math.min. This ensures lexicalWeight and semanticWeight are always
valid finite numbers, preventing NaN propagation in score normalization.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/mcp-server/src/services/search/basic-search-engine.ts (1)
254-276:⚠️ Potential issueOverloaded methods collide – later overloads are silently dropped
searchResult.iduses onlymethod.name, so two overloads of the same method (common in Java/C#, e.g.add(int)vsadd(String)) generate identical IDs.
The duplicate check at 270-276 then discards every overload after the first, making them unsearchable – a data-loss bug.- id: `method:${language}:${namespace}:${className || ''}:${method.name}`, + // Include the unique key (usually namespace + signature) to disambiguate overloads + id: `method:${language}:${namespace}:${className || ''}:${method.key}`,Remember to update any downstream consumers that parse the ID string.
♻️ Duplicate comments (1)
apps/chat/src/app/api/privmx/[transport]/route.ts (1)
30-31: 🛠️ Refactor suggestionAccessor now tied to brittle global promise
getMCPController()just returns the cached promise, inheriting the irrecoverable-failure risk noted above. After implementing the resilientinit()helper (see previous comment), reduce this to:async function getMCPController() { return init(); }Keeps the API identical while fixing recovery.
🧹 Nitpick comments (2)
packages/mcp-server/src/services/search/basic-search-engine.ts (2)
13-18: Clarify env-var precedence to avoid misconfiguration
TEXT_ALGOandUSE_BM25derive from two different env vars whose semantics overlap (TEXT_ALGO,USE_BM25). A deployment that sets both (e.g.TEXT_ALGO=tfidfandUSE_BM25=true) will silently ignore the latter. Consider documenting the precedence or collapsing to a single env flag to remove ambiguity.
125-142: BM25 branch: score scaling & limit are hard-coded
The BM25 path always asks for 15 hits and then truncates to 10. This deviates from the TF-IDF fallback (which effectively returnswords.length-scaled scores). Exposingkand/or normalising scores would let callers compose hybrid scoring more predictably.-const hits = this.bm25.search(functionality, 15); +const topK = Number(process.env.BM25_TOP_K ?? '15'); +const hits = this.bm25.search(functionality, topK); ... -.slice(0, 10) +.slice(0, Number(process.env.BM25_RETURN_K ?? '10'))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
apps/chat/src/app/api/privmx/[transport]/route.ts(1 hunks)package.json(2 hunks)packages/mcp-server/src/services/search/basic-search-engine.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/chat/src/app/api/privmx/[transport]/route.ts (1)
apps/chat/src/features/mcp/mcp-controller.ts (1)
MCPController(25-229)
packages/mcp-server/src/services/search/basic-search-engine.ts (1)
packages/mcp-server/src/types/index.ts (1)
SearchResult(66-77)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cursor BugBot
| const initializationPromise: Promise<MCPController> = (async () => { | ||
| mcpController = mcpController ?? new MCPController(); | ||
| await mcpController.initialize(); | ||
| return mcpController; | ||
| })(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialization promise becomes a permanent single-point-of-failure
If mcpController.initialize() throws once, the rejected initializationPromise is cached forever and every subsequent request will instantly fail.
Re-introduce resilience:
-let mcpController: MCPController | null = null;
-// Shared promise ensures initialization happens only once per module instance
-const initializationPromise: Promise<MCPController> = (async () => {
- mcpController = mcpController ?? new MCPController();
- await mcpController.initialize();
- return mcpController;
-})();
+let mcpController: MCPController | null = null;
+let initializationPromise: Promise<MCPController> | null = null;
+
+function init(): Promise<MCPController> {
+ if (mcpController?.isReady()) return Promise.resolve(mcpController);
+ if (initializationPromise) return initializationPromise;
+
+ initializationPromise = (async () => {
+ try {
+ mcpController = new MCPController();
+ await mcpController.initialize();
+ return mcpController;
+ } catch (err) {
+ // allow a new attempt on the next request
+ initializationPromise = null;
+ throw err;
+ }
+ })();
+
+ return initializationPromise;
+}This keeps the “only-once” guarantee while allowing recovery after transient failures.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const initializationPromise: Promise<MCPController> = (async () => { | |
| mcpController = mcpController ?? new MCPController(); | |
| await mcpController.initialize(); | |
| return mcpController; | |
| })(); | |
| let mcpController: MCPController | null = null; | |
| let initializationPromise: Promise<MCPController> | null = null; | |
| function init(): Promise<MCPController> { | |
| if (mcpController?.isReady()) { | |
| return Promise.resolve(mcpController); | |
| } | |
| if (initializationPromise) { | |
| return initializationPromise; | |
| } | |
| initializationPromise = (async () => { | |
| try { | |
| mcpController = new MCPController(); | |
| await mcpController.initialize(); | |
| return mcpController; | |
| } catch (err) { | |
| // allow a new attempt on the next request | |
| initializationPromise = null; | |
| throw err; | |
| } | |
| })(); | |
| return initializationPromise; | |
| } |
🤖 Prompt for AI Agents
In apps/chat/src/app/api/privmx/[transport]/route.ts around lines 20 to 24, the
initializationPromise caches a rejected promise if mcpController.initialize()
throws, causing all future requests to fail immediately. To fix this, modify the
initialization logic to catch initialization errors, reset the
initializationPromise on failure, and allow retries on subsequent calls while
preserving the single initialization attempt at a time.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Bug: Windows URL Pathname Handling Issue
On Windows, new URL(import.meta.url).pathname returns a URL path (e.g., /C:/...) with forward slashes. When passed to path.dirname() and path.resolve(), this URL format is misinterpreted, leading to incorrect template path resolution and subsequent template loading failures. The url.fileURLToPath() utility should be used instead.
packages/mcp-server/src/services/code-generators/template-renderer.ts#L37-L43
Was this report helpful? Give feedback by reacting with 👍 or 👎
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores
These changes deliver more relevant search results, better code generation, and improved system observability for end-users.