Conversation
jeffmaury
requested review from
benoitf and
gastoner
and removed request for
a team
📝 WalkthroughWalkthroughAdds a new Milvus RAG provider extension: DI bindings, container-based ConnectionManager and MilvusConnection implementation, utilities and tests, Vite/TS build config and package manifest, and integrates the extension build into the repository root scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant Host as Extension Host
participant Ext as extension.ts (activate)
participant MilvusExt as MilvusExtension
participant DI as InversifyBinding
participant ConnMgr as ConnectionManager
participant ContainerAPI as Container Extension API
participant Docker as Docker/Dockerode
participant Milvus as Milvus Container
Host->>Ext: activate()
Ext->>MilvusExt: instantiate & activate()
MilvusExt->>DI: initBindings()
DI->>DI: create Container & bind symbols
MilvusExt->>ConnMgr: resolve & init()
ConnMgr->>ContainerAPI: list endpoints / containers
ContainerAPI-->>ConnMgr: endpoints / existing containers
ConnMgr->>ConnMgr: discoverExistingContainers() / register connections
alt create new Milvus instance
ConnMgr->>Docker: checkMilvusImage()
alt image missing
ConnMgr->>Docker: pullMilvusImage()
Docker-->>ConnMgr: image pulled
end
ConnMgr->>ConnMgr: allocate host port & create storage/config files
ConnMgr->>Docker: run container (env, volumes, labels, port)
Docker-->>Milvus: container started
ConnMgr->>ConnMgr: register MilvusConnection and start lifecycle
ConnMgr->>ContainerAPI: expose RagProviderConnection
Milvus-->>ConnMgr: ready at host:port
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.18.1)extensions/milvus/README.mdmarkdownlint-cli2 v0.18.1 (markdownlint v0.38.0) ... [truncated 1142 characters] ... 4:11) 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
extensions/milvus/package.json (1)
35-38: Consider aligning inversify version with root package.The extension uses
inversify@^7.10.4while the rootpackage.jsonuses^7.7.1. Consider aligning versions for consistency across the codebase, unless the newer version is specifically required for features used in this extension.extensions/milvus/src/milvus-extension.ts (1)
76-78: Consider disposing InversifyBinding to release DI container resources.The
deactivatemethod disposesconnectionManagerbut does not callthis.#inversifyBinding?.dispose(). TheInversifyBindingclass has adispose()method that unbinds all bindings from the container.async deactivate(): Promise<void> { this.#connectionManager?.dispose(); + await this.#inversifyBinding?.dispose(); }extensions/milvus/src/inject/inversify-binding.ts (1)
52-53: Remove stale comment.The comment "Get inference model manager" appears to be copied from another extension and doesn't apply to the Milvus extension context.
await this.#container.load(managersModule); - // Get inference model manager return this.#container;extensions/milvus/src/api/milvus-connection.ts (2)
91-95: New MilvusClient created on each operation.
getMilvusClient()creates a new client instance for everyindex()anddeindex()call. Consider caching the client instance for the lifetime of the connection to reduce connection overhead.+ private milvusClient: MilvusClient | undefined; + private getMilvusClient(): MilvusClient { - return new MilvusClient({ - address: `localhost:${this.port}`, - }); + if (!this.milvusClient) { + this.milvusClient = new MilvusClient({ + address: `localhost:${this.port}`, + }); + } + return this.milvusClient; }
203-205: Indexing errors are silently logged without propagation.When indexing fails, the error is logged but not thrown. Callers won't know if their indexing operation succeeded. Consider propagating the error or returning a result indicating success/failure.
} catch (err: unknown) { console.error('Error indexing document:', err); + throw err; }extensions/milvus/src/manager/connection-manager.ts (1)
231-238: Misleading log message.The log says "Using podman CLI" but the code uses the dockerode API, not CLI commands. This could confuse users reading the logs.
- logger?.log('Using podman CLI to create container...'); - - // Check if podman is available + logger?.log('Checking container endpoint...'); const containerConnection = this.containerExtensionAPI.getEndpoints()[0];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
extensions/milvus/milvus-icon-color.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
extensions/milvus/README.md(1 hunks)extensions/milvus/package.json(1 hunks)extensions/milvus/src/api/milvus-connection.ts(1 hunks)extensions/milvus/src/api/milvus-container.ts(1 hunks)extensions/milvus/src/extension.spec.ts(1 hunks)extensions/milvus/src/extension.ts(1 hunks)extensions/milvus/src/inject/inversify-binding.ts(1 hunks)extensions/milvus/src/inject/symbol.ts(1 hunks)extensions/milvus/src/manager/_manager-module.ts(1 hunks)extensions/milvus/src/manager/connection-manager.spec.ts(1 hunks)extensions/milvus/src/manager/connection-manager.ts(1 hunks)extensions/milvus/src/milvus-extension.ts(1 hunks)extensions/milvus/src/util/config.ts(1 hunks)extensions/milvus/tsconfig.json(1 hunks)extensions/milvus/vite.config.js(1 hunks)package.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
extensions/*/src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
extensions/*/src/**/*.ts: Use @kortex-app/api for extension interactions, which provides TypeScript definitions for provider registration, configuration management, command and menu contributions, and UI components
Use the @kortex-app/api package to interact with Kortex core capabilities from extensions
Files:
extensions/milvus/src/api/milvus-container.tsextensions/milvus/src/inject/inversify-binding.tsextensions/milvus/src/manager/_manager-module.tsextensions/milvus/src/inject/symbol.tsextensions/milvus/src/manager/connection-manager.spec.tsextensions/milvus/src/milvus-extension.tsextensions/milvus/src/extension.spec.tsextensions/milvus/src/extension.tsextensions/milvus/src/manager/connection-manager.tsextensions/milvus/src/util/config.tsextensions/milvus/src/api/milvus-connection.ts
extensions/*/package.json
📄 CodeRabbit inference engine (CLAUDE.md)
extensions/*/package.json: Extensions must declareengines.kortexversion compatibility inpackage.json
Extensionpackage.jsonmust havemainfield pointing to./dist/extension.js
Configuration properties should be defined in extensionpackage.jsonundercontributes.configuration.propertieswith appropriate scopes (DEFAULT, ContainerProviderConnection, KubernetesProviderConnection, InferenceProviderConnection, InferenceProviderConnectionFactory)
extensions/*/package.json: Extensions must be located in the extensions/ directory with a standard structure including a package.json with main pointing to ./dist/extension.js and must declare engines.kortex version compatibility
Extension package.json must declare engines.kortex version compatibility
Extension main entry point must point to ./dist/extension.js in package.json
Extensions should contribute inference providers, flow providers, MCP registries, and/or configuration properties through the package.json contributes field
Configuration properties should be defined in extension package.json under contributes.configuration.properties with proper scopes (DEFAULT, ContainerProviderConnection, KubernetesProviderConnection, InferenceProviderConnection, InferenceProviderConnectionFactory)
Configuration scopes limit exposure of sensitive data and should be used appropriately when defining configuration properties
Files:
extensions/milvus/package.json
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests should be co-located with source code as
*.spec.tsfilesUnit tests should be co-located with source code using the *.spec.ts naming convention
Files:
extensions/milvus/src/manager/connection-manager.spec.tsextensions/milvus/src/extension.spec.ts
extensions/*/vite.config.js
📄 CodeRabbit inference engine (AGENTS.md)
Extensions should be built using Vite with build output to ./dist/extension.js
Files:
extensions/milvus/vite.config.js
extensions/*/src/extension.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Extensions must export a standard
activate()function insrc/extension.tsExtensions must export a standard activation API through their entry point
Files:
extensions/milvus/src/extension.ts
🧠 Learnings (26)
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Applies to packages/main/src/**/*.ts : Container operations must include the `engineId` parameter to identify the container engine when using `ContainerProviderRegistry` methods like `listContainers`, `startContainer`, `stopContainer`, `deleteContainer`, `pullImage`, `buildImage`, `deleteImage`, `createPod`, `startPod`, `stopPod`, and `removePod`
Applied to files:
extensions/milvus/src/api/milvus-container.ts
📚 Learning: 2025-11-05T16:31:22.355Z
Learnt from: benoitf
Repo: kortex-hub/kortex PR: 678
File: extensions/container/packages/api/vite.config.js:34-40
Timestamp: 2025-11-05T16:31:22.355Z
Learning: In the kortex repository, the `kortex-app/container-extension-api` package at `extensions/container/packages/api/` is a declaration-only package that ships pure TypeScript declaration files (.d.ts) without any compiled JavaScript. It intentionally has an empty build script and does not require library build configuration in vite.config.js.
Applied to files:
extensions/milvus/src/api/milvus-container.tsextensions/milvus/src/inject/inversify-binding.tsextensions/milvus/package.jsonextensions/milvus/vite.config.js
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to packages/main/src/**/*.ts : Container operations should use ContainerProviderRegistry with engineId parameter to identify the container engine, supporting operations like listContainers, startContainer, stopContainer, deleteContainer, pullImage, buildImage, deleteImage, createPod, startPod, stopPod, removePod
Applied to files:
extensions/milvus/src/api/milvus-container.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Use Inversify dependency injection for the plugin system implementation, with the core PluginSystem class in packages/main/src/plugin/index.ts orchestrating extension loading and service registration
Applied to files:
extensions/milvus/src/inject/inversify-binding.tsextensions/milvus/src/manager/_manager-module.ts
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Use Inversify dependency injection container in `packages/main/src/plugin/index.ts` to register all major services (ProviderRegistry, ContainerProviderRegistry, KubernetesClient, MCPManager, FlowManager, ChatManager, ConfigurationRegistry) as singletons
Applied to files:
extensions/milvus/src/inject/inversify-binding.tsextensions/milvus/src/manager/_manager-module.tsextensions/milvus/src/manager/connection-manager.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Register all major services (ProviderRegistry, ContainerProviderRegistry, KubernetesClient, MCPManager, MCPRegistry, FlowManager, ChatManager, ConfigurationRegistry) as singletons in the Inversify DI container during initialization
Applied to files:
extensions/milvus/src/inject/inversify-binding.tsextensions/milvus/src/manager/_manager-module.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to extensions/*/src/**/*.ts : Use kortex-app/api for extension interactions, which provides TypeScript definitions for provider registration, configuration management, command and menu contributions, and UI components
Applied to files:
extensions/milvus/src/inject/inversify-binding.tsextensions/milvus/src/inject/symbol.tsextensions/milvus/package.jsonextensions/milvus/src/milvus-extension.tsextensions/milvus/src/extension.ts
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Extensions can contribute inference providers (Gemini, OpenAI-compatible, OpenShift AI), flow providers (Goose), MCP registries, and configuration properties through the extension API
Applied to files:
extensions/milvus/src/inject/symbol.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to extensions/*/package.json : Extensions should contribute inference providers, flow providers, MCP registries, and/or configuration properties through the package.json contributes field
Applied to files:
extensions/milvus/package.jsonpackage.json
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to extensions/*/package.json : Configuration properties should be defined in extension package.json under contributes.configuration.properties with proper scopes (DEFAULT, ContainerProviderConnection, KubernetesProviderConnection, InferenceProviderConnection, InferenceProviderConnectionFactory)
Applied to files:
extensions/milvus/package.json
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Applies to extensions/*/package.json : Configuration properties should be defined in extension `package.json` under `contributes.configuration.properties` with appropriate scopes (DEFAULT, ContainerProviderConnection, KubernetesProviderConnection, InferenceProviderConnection, InferenceProviderConnectionFactory)
Applied to files:
extensions/milvus/package.json
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to extensions/*/package.json : Extensions must be located in the extensions/ directory with a standard structure including a package.json with main pointing to ./dist/extension.js and must declare engines.kortex version compatibility
Applied to files:
extensions/milvus/package.jsonpackage.json
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Applies to extensions/*/package.json : Extension `package.json` must have `main` field pointing to `./dist/extension.js`
Applied to files:
extensions/milvus/package.jsonpackage.json
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Extensions should be built using Vite and output to `dist/extension.js`
Applied to files:
extensions/milvus/package.jsonextensions/milvus/vite.config.jspackage.json
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Applies to extensions/*/package.json : Extensions must declare `engines.kortex` version compatibility in `package.json`
Applied to files:
extensions/milvus/package.json
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to extensions/*/package.json : Extension package.json must declare engines.kortex version compatibility
Applied to files:
extensions/milvus/package.json
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Applies to extensions/*/src/extension.ts : Extensions must export a standard `activate()` function in `src/extension.ts`
Applied to files:
extensions/milvus/src/milvus-extension.tsextensions/milvus/src/extension.spec.tsextensions/milvus/src/extension.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to extensions/*/src/extension.ts : Extensions must export a standard activation API through their entry point
Applied to files:
extensions/milvus/src/milvus-extension.tsextensions/milvus/src/extension.spec.tsextensions/milvus/src/extension.ts
📚 Learning: 2025-11-08T14:36:17.423Z
Learnt from: benoitf
Repo: kortex-hub/kortex PR: 692
File: extensions/ramalama/src/inject/inversify-binding.spec.ts:38-38
Timestamp: 2025-11-08T14:36:17.423Z
Learning: In the kortex repository, tests use `vi.mock(import('module-path'))` syntax instead of `vi.mock('module-path')` to ensure TypeScript validates the module path at compile time, providing refactoring safety if modules are moved or removed.
Applied to files:
extensions/milvus/src/extension.spec.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to extensions/*/vite.config.js : Extensions should be built using Vite with build output to ./dist/extension.js
Applied to files:
extensions/milvus/vite.config.jspackage.json
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to extensions/*/package.json : Configuration scopes limit exposure of sensitive data and should be used appropriately when defining configuration properties
Applied to files:
extensions/milvus/vite.config.js
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to extensions/*/src/**/*.ts : Use the kortex-app/api package to interact with Kortex core capabilities from extensions
Applied to files:
extensions/milvus/src/extension.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to extensions/*/package.json : Extension main entry point must point to ./dist/extension.js in package.json
Applied to files:
package.json
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Follow the standard Electron architecture with three main processes: Main Process in `packages/main` for Node.js backend and system integration, Renderer Process in `packages/renderer` for Svelte-based UI, and Preload Scripts in `packages/preload` and `packages/preload-webview` for secure IPC communication
Applied to files:
package.json
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Follow the standard Electron architecture with three main processes: Main Process (packages/main), Renderer Process (packages/renderer), and Preload Scripts (packages/preload and packages/preload-webview)
Applied to files:
package.json
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to packages/main/src/**/*.ts : Configuration values should be retrieved and stored with dot notation (e.g., gemini.factory.apiKey)
Applied to files:
extensions/milvus/src/util/config.ts
🧬 Code graph analysis (6)
extensions/milvus/src/inject/inversify-binding.ts (3)
extensions/container/packages/api/src/container-extension-api.d.ts (1)
ContainerExtensionAPI(32-40)__mocks__/@kortex-app/api.js (1)
provider(27-29)extensions/milvus/src/inject/symbol.ts (3)
ExtensionContextSymbol(22-22)ContainerExtensionAPISymbol(21-21)MilvusProvider(20-20)
extensions/milvus/src/milvus-extension.ts (1)
extensions/container/packages/api/src/container-extension-api.d.ts (1)
ContainerExtensionAPI(32-40)
extensions/milvus/src/extension.spec.ts (3)
packages/extension-api/src/extension-api.d.ts (1)
ExtensionContext(184-204)extensions/milvus/src/milvus-extension.ts (3)
MilvusExtension(27-79)activate(37-70)deactivate(76-78)extensions/milvus/src/extension.ts (2)
activate(24-27)deactivate(29-32)
extensions/milvus/vite.config.js (1)
eslint.config.mjs (1)
__dirname(40-40)
extensions/milvus/src/extension.ts (2)
extensions/milvus/src/milvus-extension.ts (3)
MilvusExtension(27-79)activate(37-70)deactivate(76-78)packages/extension-api/src/extension-api.d.ts (1)
ExtensionContext(184-204)
extensions/milvus/src/api/milvus-connection.ts (1)
packages/extension-api/src/extension-api.d.ts (5)
RagProviderConnection(687-694)ProviderConnectionStatus(360-360)MCPServer(680-683)ProviderConnectionLifecycle(375-386)LifecycleContext(371-373)
🪛 Biome (2.1.2)
extensions/milvus/src/manager/connection-manager.ts
[error] 151-151: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Linux
- GitHub Check: macOS
- GitHub Check: smoke e2e tests (production)
- GitHub Check: linter, formatters
- GitHub Check: smoke e2e tests (development)
- GitHub Check: Windows
🔇 Additional comments (19)
extensions/milvus/tsconfig.json (1)
1-22: TypeScript configuration is well-structured and appropriate.This configuration appropriately supports the Milvus extension requirements: strict type checking, decorator support for inversify-based dependency injection (lines 12–13), modern ES2024 runtime, and clean module resolution for Node.js. The path alias (line 18) improves import readability, and source maps (line 6) support debugging. All settings align with the extension's stated purpose.
package.json (1)
18-22: LGTM!The Milvus extension build script is correctly added and integrated into the
build:extensionschain, following the established alphabetical ordering pattern used by other extensions.extensions/milvus/vite.config.js (2)
37-55: LGTM!The build configuration correctly:
- Outputs to
dist/extension.jsas required by coding guidelines- Uses CJS format for extension compatibility
- Marks
@kortex-app/apiand Node.js builtins as external dependencies
22-22: Verify__dirnameavailability in the Vite config.The use of
__dirnamemay cause issues if this file is treated as an ES module. Since the rootpackage.jsonspecifies"type": "module",.jsfiles are interpreted as ESM where__dirnameis not defined.Consider using the ESM-compatible approach:
-import { join } from 'path'; +import { join, dirname } from 'path'; +import { fileURLToPath } from 'url'; import { builtinModules } from 'module'; -const PACKAGE_ROOT = __dirname; +const __filename = fileURLToPath(import.meta.url); +const PACKAGE_ROOT = dirname(__filename);extensions/milvus/src/api/milvus-container.ts (1)
18-24: LGTM!The
MilvusContainerinterface is well-defined with appropriate types for container metadata. The fields (path,id,name,port,running) provide the necessary information for container lifecycle management.extensions/milvus/README.md (2)
1-33: Documentation provides clear usage instructions.The README effectively documents the extension's features, usage workflow, and configuration. Consider adding a note about the
:latestimage tag and whether users can specify a different Milvus version.
18-18: Storage path references "podman-desktop" instead of "kortex".The documented storage path mentions
podman-desktopwhich appears inconsistent with the Kortex product. Verify this is the intended path or update to reflect the actual Kortex extension storage location.- - Create a storage folder under `~/.local/share/containers/podman-desktop/extensions-storage/milvus/<name>` + - Create a storage folder under `~/.local/share/kortex/extensions-storage/milvus/<name>`Likely an incorrect or invalid review comment.
extensions/milvus/src/manager/_manager-module.ts (1)
19-27: LGTM!The Inversify
ContainerModulecorrectly bindsConnectionManageras a singleton, following the established DI patterns in the codebase. Based on learnings, registering managers as singletons is the recommended approach.extensions/milvus/src/inject/symbol.ts (1)
19-22: LGTM!The injection symbols are properly defined using
Symbol.for()for global symbol registry, enabling consistent DI token resolution across module boundaries. The naming convention is clear and follows the established pattern for Inversify tokens.extensions/milvus/package.json (1)
1-28: Verify configuration scopeRagProviderConnectionFactoryagainst Kortex scope definitions.The extension properly declares
engines.kortex(line 8-10),mainfield (line 14), and configuration contribution structure. However, the configuration scopeRagProviderConnectionFactory(line 24) is not documented in the standard Kortex configuration scopes (DEFAULT, ContainerProviderConnection, KubernetesProviderConnection, InferenceProviderConnection, InferenceProviderConnectionFactory). Confirm whether this is a valid new scope for RAG providers or if it should use one of the documented scopes.extensions/milvus/src/extension.spec.ts (2)
27-36: Well-structured test setup with correct mock syntax.The test uses
vi.mock(import('./milvus-extension'))syntax which ensures TypeScript validates the module path at compile time. ThebeforeEachcorrectly resets mocks and sets up the extension context. Based on learnings, this is the preferred approach in the kortex repository.
70-73: Good defensive test for edge case.Testing
deactivate()beforeactivate()ensures the extension handles graceful shutdown even when not fully initialized. This aligns with the optional chaining inextension.ts(milvusExtension?.deactivate()).extensions/milvus/src/extension.ts (1)
24-27: Activation errors are silently logged; consider propagating for extension loader awareness.The
activatefunction firesmilvusExtension.activate()without awaiting it, and errors are only logged to console. While this prevents blocking, the extension loader won't know if activation failed. This may be intentional, but callers expecting a fully-activated extension afterawait activate()may be surprised.Is this fire-and-forget pattern intentional for the extension lifecycle? If the activation fails, should the extension remain in a "failed" state that the system can query?
extensions/milvus/src/manager/connection-manager.spec.ts (1)
230-233: Path construction assertion may not match implementation behavior.The test expects
join('/test', 'storage', 'test-milvus')butextensionContextMock.storagePathis set to'/test/storage'(line 62). If the implementation usesjoin(storagePath, name)instead ofjoin('/test', 'storage', name), the mock assertion will fail despite both paths resolving to the same normalized value. Verify that the actualdeleteConnectionimplementation path construction matches the test expectation.extensions/milvus/src/util/config.ts (1)
28-35: Ensure the storage directory exists before writing config files.The function writes to
storagePathwithout ensuring the directory exists first. Node.jsfs.promises.writeFilethrows anENOENTerror if parent directories don't exist. Either create the directory before writing (e.g.,await mkdir(storagePath, { recursive: true })before thewriteFilecalls) or verify that all callers guarantee the directory is created beforehand. If the latter is true, document this requirement.extensions/milvus/src/manager/connection-manager.ts (4)
22-40: LGTM - Proper use of @kortex-app/api for extension interactions.The imports correctly use
@kortex-app/apifor provider registration and lifecycle management, following the coding guidelines for Kortex extensions.
54-88: LGTM - Well-structured dependency injection and connection registration.The class correctly uses inversify decorators for DI and maintains connections with composite keys (
path::id) to handle multiple container endpoints. Lifecycle hooks are properly bound to instance methods.
247-273: LGTM - Container configuration follows Milvus requirements.The container creation options correctly configure environment variables for standalone mode, bind mounts with SELinux context (
:Z), port bindings, and discovery labels.
118-130: Verify that connection names are sanitized before path construction.The
entry.connection.nameis used directly injoin(this.extensionContext.storagePath, entry.connection.name)for a recursive delete operation. Sincepath.join()does not strip path traversal sequences, malicious names containing../could potentially access files outside the intended storage directory.
| // Create index on the embedding field | ||
| await client.createIndex({ | ||
| collection_name: collectionName, | ||
| field_name: 'embedding', | ||
| index_type: 'IVF_FLAT', | ||
| metric_type: 'L2', | ||
| params: { nlist: 128 }, | ||
| }); |
There was a problem hiding this comment.
Index created on non-existent 'embedding' field.
The schema (lines 103-126) defines fields: id, doc_uri, text, and sparse. However, this code attempts to create an index on embedding, which doesn't exist in the schema. This will cause a runtime error when ensuring the collection.
Either remove this index creation (if using only BM25 sparse search) or add the embedding vector field to the schema:
- // Create index on the embedding field
- await client.createIndex({
- collection_name: collectionName,
- field_name: 'embedding',
- index_type: 'IVF_FLAT',
- metric_type: 'L2',
- params: { nlist: 128 },
- });Or if dense embeddings are needed, add the field to the schema first.
📝 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.
| // Create index on the embedding field | |
| await client.createIndex({ | |
| collection_name: collectionName, | |
| field_name: 'embedding', | |
| index_type: 'IVF_FLAT', | |
| metric_type: 'L2', | |
| params: { nlist: 128 }, | |
| }); |
🤖 Prompt for AI Agents
In extensions/milvus/src/api/milvus-connection.ts around lines 159-166 (schema
is defined earlier at lines 103-126), the code attempts to create an index on a
non-existent field 'embedding'; either remove the createIndex call or add the
embedding vector field to the collection schema before creating the index. To
fix: if you need dense embeddings, update the schema block at lines ~103-126 to
include an 'embedding' field with appropriate type and dimension (e.g.,
VectorFloat or VectorBinary and dim), ensure the collection is created/updated
with that field, then keep the createIndex call; if you only need BM25 sparse
search, simply delete or guard the createIndex call so it does not run for
schemas without an 'embedding' field.
| port: parseInt(milvusPort, 10), | ||
| running: container.State === 'running', | ||
| }); | ||
| existingContainers.splice(existingContainers.indexOf(`${endpoint.path}::${container.Id}`), 1); | ||
| } |
There was a problem hiding this comment.
Bug: splice with indexOf returning -1 removes wrong element.
When indexOf returns -1 (container not in existingContainers), splice(-1, 1) removes the last element of the array, corrupting the stale container tracking.
Also, use Number.parseInt per ES2015 conventions.
if (milvusName && milvusPort) {
this.registerConnection({
path: endpoint.path,
id: container.Id,
name: milvusName,
- port: parseInt(milvusPort, 10),
+ port: Number.parseInt(milvusPort, 10),
running: container.State === 'running',
});
- existingContainers.splice(existingContainers.indexOf(`${endpoint.path}::${container.Id}`), 1);
+ const idx = existingContainers.indexOf(`${endpoint.path}::${container.Id}`);
+ if (idx !== -1) {
+ existingContainers.splice(idx, 1);
+ }
}📝 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.
| port: parseInt(milvusPort, 10), | |
| running: container.State === 'running', | |
| }); | |
| existingContainers.splice(existingContainers.indexOf(`${endpoint.path}::${container.Id}`), 1); | |
| } | |
| port: Number.parseInt(milvusPort, 10), | |
| running: container.State === 'running', | |
| }); | |
| const idx = existingContainers.indexOf(`${endpoint.path}::${container.Id}`); | |
| if (idx !== -1) { | |
| existingContainers.splice(idx, 1); | |
| } | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 151-151: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
🤖 Prompt for AI Agents
In extensions/milvus/src/manager/connection-manager.ts around lines 151 to 155,
the code calls splice(existingContainers.indexOf(...), 1) without checking the
index, so when indexOf returns -1 it removes the last array element; also use
Number.parseInt per ES2015. Fix by computing const idx =
existingContainers.indexOf(`${endpoint.path}::${container.Id}`), only call
existingContainers.splice(idx, 1) if idx >= 0, and replace parseInt(milvusPort,
10) with Number.parseInt(milvusPort, 10).
|
small nit 
the milvus collection name i think can be on a new line |
|
tested on windows |
There was a problem hiding this comment.
overall looks good! I agree with some of the code rabbit issues that it brought up!
Non blocking: do you think we can add a health check for the milvus container?
async function waitForReady(port: number, logger?: Logger, timeout = 60000): Promise<void> {
const start = Date.now();
logger?.log(`Waiting for Milvus to be ready on port ${port}...`);
while (Date.now() - start < timeout) {
try {
const client = new MilvusClient({ address: `localhost:${port}` });
await client.checkHealth();
logger?.log('Milvus is ready!');
return;
} catch {
// Milvus not ready yet, wait and retry
await new Promise(r => setTimeout(r, 2000));
}
}
throw new Error(`Milvus container failed to become ready within ${timeout / 1000} seconds`);
}
something like this in connction-manager.ts? Or this can be done via when using the collection instead
I think just fixing that small ui error and addressing some of the code rabbit stuff and its good to go!
Unrelated to this PR will open an issue |
gastoner
left a comment
gastoner
left a comment
There was a problem hiding this comment.
Are there some requirements for creating the resource? - tried with Podman Desktop running no change
Cant create the resource on Linux :(
Screencast_20251202_084121.webm
It is based on the containers extension that does not support Linux |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
extensions/milvus/src/api/milvus-connection.ts (1)
159-166: Index creation references non-existent 'embedding' field.The schema (lines 103-126) defines fields
id,doc_uri,text, andsparse, but this code attempts to create an index onembeddingwhich doesn't exist. This will cause a runtime error when ensuring the collection.Either remove the index creation (if using only BM25 sparse search) or add the
embeddingvector field to the schema:- // Create index on the embedding field - await client.createIndex({ - collection_name: collectionName, - field_name: 'embedding', - index_type: 'IVF_FLAT', - metric_type: 'L2', - params: { nlist: 128 }, - });extensions/milvus/src/manager/connection-manager.ts (2)
102-116: Silent failure when container is not found remains.If
getContainerreturnsundefined, these methods silently return without starting/stopping or notifying the user. This can make debugging difficult. Consider throwing an error or at minimum logging a warning.async startConnection(connection: MilvusConnection, startContext: LifecycleContext): Promise<void> { const container = this.getContainer(connection); - if (container) { - await container.start(); - return connection.start(startContext); + if (!container) { + console.warn(`Container not found for connection '${connection.name}'`); + return; } + await container.start(); + return connection.start(startContext); }
235-235: Log message is misleading.The message says "Using podman CLI to create container..." but the code uses Dockerode API, not the podman CLI directly. This appears to be a copy-paste artifact.
- logger?.log('Using podman CLI to create container...'); + logger?.log('Using container API to create container...');
🧹 Nitpick comments (4)
extensions/milvus/src/manager/connection-manager.spec.ts (1)
66-68: Redundant mock reset calls.
vi.resetAllMocks()already resets mock state including call history, sovi.clearAllMocks()immediately after is redundant.beforeEach(async () => { vi.resetAllMocks(); - vi.clearAllMocks();extensions/milvus/src/api/milvus-connection.ts (2)
91-95: Consider caching the MilvusClient instance.
getMilvusClient()creates a new client on every call. If the Milvus SDK doesn't internally pool connections, this could lead to connection overhead. Consider caching the client instance and reusing it.+ private milvusClient: MilvusClient | undefined; + private getMilvusClient(): MilvusClient { - return new MilvusClient({ - address: `localhost:${this.port}`, - }); + if (!this.milvusClient) { + this.milvusClient = new MilvusClient({ + address: `localhost:${this.port}`, + }); + } + return this.milvusClient; }
38-64: Hardcoded MCP server version may become stale.The version
'0.1.1.dev8'is hardcoded in two places (lines 40 and 46). Consider extracting this to a constant or configuration to make updates easier.+const MCP_SERVER_VERSION = '0.1.1.dev8'; + export class MilvusConnection implements api.RagProviderConnection { // ... constructor(...) { const server: MCPServerDetail = { name: 'mcp-server-milvus', - version: '0.1.1.dev8', + version: MCP_SERVER_VERSION, // ... packages: [ { // ... - version: '0.1.1.dev8', + version: MCP_SERVER_VERSION,extensions/milvus/src/manager/connection-manager.ts (1)
151-151: UseNumber.parseIntper ES2015 conventions.Static analysis flags this as using the global
parseIntinstead ofNumber.parseInt.- port: parseInt(milvusPort, 10), + port: Number.parseInt(milvusPort, 10),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
extensions/milvus/src/api/milvus-connection.ts(1 hunks)extensions/milvus/src/manager/connection-manager.spec.ts(1 hunks)extensions/milvus/src/manager/connection-manager.ts(1 hunks)extensions/milvus/src/milvus-extension.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- extensions/milvus/src/milvus-extension.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests should be co-located with source code as
*.spec.tsfilesUnit tests should be co-located with source code using the *.spec.ts naming convention
Files:
extensions/milvus/src/manager/connection-manager.spec.ts
extensions/*/src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
extensions/*/src/**/*.ts: Use @kortex-app/api for extension interactions, which provides TypeScript definitions for provider registration, configuration management, command and menu contributions, and UI components
Use the @kortex-app/api package to interact with Kortex core capabilities from extensions
Files:
extensions/milvus/src/manager/connection-manager.spec.tsextensions/milvus/src/api/milvus-connection.tsextensions/milvus/src/manager/connection-manager.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Extensions can contribute inference providers (Gemini, OpenAI-compatible, OpenShift AI), flow providers (Goose), MCP registries, and configuration properties through the extension API
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Applies to **/*.spec.ts : Unit tests should be co-located with source code as `*.spec.ts` files
Applied to files:
extensions/milvus/src/manager/connection-manager.spec.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to packages/main/src/**/*.ts : Container operations should use ContainerProviderRegistry with engineId parameter to identify the container engine, supporting operations like listContainers, startContainer, stopContainer, deleteContainer, pullImage, buildImage, deleteImage, createPod, startPod, stopPod, removePod
Applied to files:
extensions/milvus/src/manager/connection-manager.ts
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Applies to packages/main/src/**/*.ts : Container operations must include the `engineId` parameter to identify the container engine when using `ContainerProviderRegistry` methods like `listContainers`, `startContainer`, `stopContainer`, `deleteContainer`, `pullImage`, `buildImage`, `deleteImage`, `createPod`, `startPod`, `stopPod`, and `removePod`
Applied to files:
extensions/milvus/src/manager/connection-manager.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to packages/main/src/plugin/index.ts : IPC communication handlers should follow the naming convention <registry-name>:<action> (e.g., container-provider-registry:listContainers) in packages/main/src/plugin/index.ts
Applied to files:
extensions/milvus/src/manager/connection-manager.ts
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Use Inversify dependency injection container in `packages/main/src/plugin/index.ts` to register all major services (ProviderRegistry, ContainerProviderRegistry, KubernetesClient, MCPManager, FlowManager, ChatManager, ConfigurationRegistry) as singletons
Applied to files:
extensions/milvus/src/manager/connection-manager.ts
🧬 Code graph analysis (1)
extensions/milvus/src/manager/connection-manager.ts (5)
extensions/milvus/src/api/milvus-connection.ts (1)
MilvusConnection(25-235)extensions/milvus/src/inject/symbol.ts (3)
ExtensionContextSymbol(22-22)ContainerExtensionAPISymbol(21-21)MilvusProvider(20-20)packages/extension-api/src/extension-api.d.ts (2)
ExtensionContext(184-204)Logger(362-369)extensions/milvus/src/api/milvus-container.ts (1)
MilvusContainer(18-24)extensions/milvus/src/util/config.ts (1)
createConfigFile(28-36)
🪛 Biome (2.1.2)
extensions/milvus/src/manager/connection-manager.ts
[error] 151-151: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: smoke e2e tests (development)
- GitHub Check: smoke e2e tests (production)
- GitHub Check: typecheck
- GitHub Check: linter, formatters
- GitHub Check: Windows
- GitHub Check: macOS
- GitHub Check: Linux
🔇 Additional comments (8)
extensions/milvus/src/manager/connection-manager.spec.ts (3)
344-358: ThegetImagemock has been corrected.The test now correctly uses
mockImplementation(() => { throw new Error(...) })instead ofmockRejectedValue, properly reflecting thatDockerode.prototype.getImage()is synchronous and throws rather than returning a rejected Promise. This addresses the previous review feedback.
378-399: Tests verify graceful handling of missing containers.These tests correctly verify that the lifecycle methods don't throw when a container is not found. This aligns with the current implementation's defensive behavior. However, consider whether silent failures are the desired behavior (see related comment in
connection-manager.ts).
121-128: Good test coverage for initialization and listener setup.The test properly verifies that
init()sets up the required listeners and factory.extensions/milvus/src/api/milvus-connection.ts (1)
217-222: Filter injection vulnerability has been addressed.The escaping correctly handles both backslashes and double quotes in the proper order (backslashes first, then quotes), preventing filter injection in the Milvus delete operation.
extensions/milvus/src/manager/connection-manager.ts (4)
50-52: Port range has been corrected.The upper bound is now correctly set to
65536(exclusive), ensuring valid ports 1024-65535 are generated.
202-209: Correct control flow after reject.The
returnstatement afterpromise.reject(err)correctly preventspromise.resolve()from being called in error cases.
154-157: Array splice bug has been fixed.The code now correctly checks
idx >= 0before callingsplice, preventing the removal of wrong elements when the container isn't found.
54-65: Well-structured DI setup with Inversify.The class properly uses
@injectable()decorator and@inject()for dependencies, following the established patterns in the codebase.
|
On mac seems like the |
MarsKubeX
left a comment
MarsKubeX
left a comment
There was a problem hiding this comment.
LGTM. Tested on Mac and everything working fine. the command uvx mcp-server-milvus --milvus-uri http://localhost:$port didn't work for me, I had to directly run the inspector npx @modelcontextprotocol/inspector and add the url with port in the UI. Good job.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
extensions/milvus/src/api/milvus-connection.ts (1)
159-166: Index created on non-existent 'embedding' field will cause runtime error.The schema (lines 103-126) defines fields
id,doc_uri,text, andsparse, but this code attempts to create an index onembeddingwhich doesn't exist. This will fail whenensureCollectionis called.Since this collection uses BM25 sparse search (the
sparsefield withSPARSE_INVERTED_INDEX), remove this index creation block:- // Create index on the embedding field - await client.createIndex({ - collection_name: collectionName, - field_name: 'embedding', - index_type: 'IVF_FLAT', - metric_type: 'L2', - params: { nlist: 128 }, - }); -extensions/milvus/src/manager/connection-manager.ts (1)
109-123: Silent failure when container is not found.If
getContainerreturnsundefined, these methods silently return without starting/stopping or notifying the user. Consider throwing an error or logging a warning so users understand why the operation didn't proceed.async startConnection(connection: MilvusConnection, startContext: LifecycleContext): Promise<void> { const container = this.getContainer(connection); - if (container) { - await container.start(); - return connection.start(startContext); + if (!container) { + console.warn(`Container not found for connection '${connection.name}'`); + return; } + await container.start(); + return connection.start(startContext); } async stopConnection(connection: MilvusConnection, stopContext: LifecycleContext): Promise<void> { const container = this.getContainer(connection); - if (container) { - await container.stop(); - return connection.stop(stopContext); + if (!container) { + console.warn(`Container not found for connection '${connection.name}'`); + return; } + await container.stop(); + return connection.stop(stopContext); }
🧹 Nitpick comments (1)
extensions/milvus/src/manager/connection-manager.ts (1)
158-158: UseNumber.parseIntinstead of globalparseInt.Per ES2015 conventions and the static analysis hint, prefer
Number.parseIntover the global function.- port: parseInt(milvusPort, 10), + port: Number.parseInt(milvusPort, 10),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
extensions/milvus/src/api/milvus-connection.ts(1 hunks)extensions/milvus/src/inject/inversify-binding.ts(1 hunks)extensions/milvus/src/manager/connection-manager.spec.ts(1 hunks)extensions/milvus/src/manager/connection-manager.ts(1 hunks)extensions/milvus/src/util/config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- extensions/milvus/src/inject/inversify-binding.ts
🧰 Additional context used
📓 Path-based instructions (2)
extensions/*/src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
extensions/*/src/**/*.ts: Use @kortex-app/api for extension interactions, which provides TypeScript definitions for provider registration, configuration management, command and menu contributions, and UI components
Use the @kortex-app/api package to interact with Kortex core capabilities from extensions
Files:
extensions/milvus/src/api/milvus-connection.tsextensions/milvus/src/manager/connection-manager.tsextensions/milvus/src/manager/connection-manager.spec.tsextensions/milvus/src/util/config.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests should be co-located with source code as
*.spec.tsfilesUnit tests should be co-located with source code using the *.spec.ts naming convention
Files:
extensions/milvus/src/manager/connection-manager.spec.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Extensions can contribute inference providers (Gemini, OpenAI-compatible, OpenShift AI), flow providers (Goose), MCP registries, and configuration properties through the extension API
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to packages/main/src/**/*.ts : Container operations should use ContainerProviderRegistry with engineId parameter to identify the container engine, supporting operations like listContainers, startContainer, stopContainer, deleteContainer, pullImage, buildImage, deleteImage, createPod, startPod, stopPod, removePod
Applied to files:
extensions/milvus/src/manager/connection-manager.ts
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Applies to packages/main/src/**/*.ts : Container operations must include the `engineId` parameter to identify the container engine when using `ContainerProviderRegistry` methods like `listContainers`, `startContainer`, `stopContainer`, `deleteContainer`, `pullImage`, `buildImage`, `deleteImage`, `createPod`, `startPod`, `stopPod`, and `removePod`
Applied to files:
extensions/milvus/src/manager/connection-manager.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to packages/main/src/plugin/index.ts : IPC communication handlers should follow the naming convention <registry-name>:<action> (e.g., container-provider-registry:listContainers) in packages/main/src/plugin/index.ts
Applied to files:
extensions/milvus/src/manager/connection-manager.ts
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Use Inversify dependency injection container in `packages/main/src/plugin/index.ts` to register all major services (ProviderRegistry, ContainerProviderRegistry, KubernetesClient, MCPManager, FlowManager, ChatManager, ConfigurationRegistry) as singletons
Applied to files:
extensions/milvus/src/manager/connection-manager.tsextensions/milvus/src/util/config.ts
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Applies to **/*.spec.ts : Unit tests should be co-located with source code as `*.spec.ts` files
Applied to files:
extensions/milvus/src/manager/connection-manager.spec.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to **/*.spec.ts : Unit tests should be co-located with source code using the *.spec.ts naming convention
Applied to files:
extensions/milvus/src/manager/connection-manager.spec.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Register all major services (ProviderRegistry, ContainerProviderRegistry, KubernetesClient, MCPManager, MCPRegistry, FlowManager, ChatManager, ConfigurationRegistry) as singletons in the Inversify DI container during initialization
Applied to files:
extensions/milvus/src/util/config.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Use Inversify dependency injection for the plugin system implementation, with the core PluginSystem class in packages/main/src/plugin/index.ts orchestrating extension loading and service registration
Applied to files:
extensions/milvus/src/util/config.ts
🪛 Biome (2.1.2)
extensions/milvus/src/manager/connection-manager.ts
[error] 158-158: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
🔇 Additional comments (9)
extensions/milvus/src/manager/connection-manager.spec.ts (3)
348-357: Mock forgetImagenow throws synchronously—verify this reflects actual behavior.The test now uses
mockImplementation(() => { throw new Error(...) })which correctly reflects thatgetImageis synchronous. However, the actual implementation at line 197 ofconnection-manager.tscallsdockerode.getImage(MILVUS_IMAGE).inspect(), whereinspect()is the async operation. The current mock approach works because throwing fromgetImageprevents.inspect()from being called, but it's testing a different failure mode than "image not found" (which would fail on.inspect()).This is acceptable for testing the pull-on-missing-image path, but consider whether a more precise mock would better document the expected behavior.
377-398: Tests verify silent failure for missing containers.These tests document the current behavior where missing containers are handled gracefully (silent return). A past review suggested throwing errors instead. The current approach is valid if callers don't need to know about missing containers, but consider whether silent failures could mask bugs in production.
121-128: LGTM!The test suite provides comprehensive coverage of
ConnectionManagerfunctionality including initialization, connection registration, lifecycle operations, disposal, container discovery, and factory behavior. The test structure follows the coding guidelines with proper co-location as*.spec.ts.extensions/milvus/src/util/config.ts (1)
30-39: LGTM!The
ConfigHelperclass is correctly decorated with@injectable()for DI. The singleton scope is appropriately applied at the binding site (ininversify-binding.ts), not in the class decorator itself. Error propagation via promise rejection is appropriate here since callers should handle file creation failures.extensions/milvus/src/api/milvus-connection.ts (2)
203-206: Error propagation now correctly implemented.The catch block now properly rethrows the error after logging, ensuring callers are notified of indexing failures. This addresses the previous review concern.
217-223: Filter injection vulnerability properly addressed.The escaping logic correctly handles both backslashes and double quotes in the proper order (
\\first, then"), preventing injection attacks in the Milvus filter expression.extensions/milvus/src/manager/connection-manager.ts (3)
224-288: Factory implementation looks solid.The factory method properly:
- Validates required parameters
- Creates storage directories and config files
- Checks for and pulls the Milvus image if needed
- Creates containers with appropriate labels for discovery
- Registers the new connection
54-56: Port range fix verified.The random port generation now correctly uses
65536as the exclusive upper bound, ensuring valid TCP ports (1024-65535).
204-222: Image pull error handling is now correct.The
pullMilvusImagemethod properly:
- Returns after rejecting on
onFinishederror (line 212)- Rejects the promise in the catch block (line 219)
This ensures the promise settles correctly in all error scenarios.
jeffmaury
force-pushed
the
GH-805
branch
2 times, most recently
from
d3bd64f to
c03caa8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
extensions/milvus/src/manager/connection-manager.ts (2)
140-174: UseNumber.parseIntinstead of globalparseIntin discovery.Static analysis is right: prefer
Number.parseIntfor consistency with ES2015 style, now that you’ve already fixed theindexOf/splicebug.- port: parseInt(milvusPort, 10), + port: Number.parseInt(milvusPort, 10),
105-119: Avoid silently ignoring start/stop when the container is missing.If
getContainer(connection)returnsundefined,startConnection/stopConnectionjust return without logging or signaling an error. From a UX standpoint this looks like a successful operation even though nothing happened.Consider at least logging or, preferably, throwing:
async startConnection(connection: MilvusConnection, startContext: LifecycleContext): Promise<void> { const container = this.getContainer(connection); - if (container) { - await container.start(); - return connection.start(startContext); - } + if (!container) { + throw new Error(`Container not found for Milvus connection '${connection.name}'`); + } + await container.start(); + return connection.start(startContext); } async stopConnection(connection: MilvusConnection, stopContext: LifecycleContext): Promise<void> { const container = this.getContainer(connection); - if (container) { - await container.stop(); - return connection.stop(stopContext); - } + if (!container) { + throw new Error(`Container not found for Milvus connection '${connection.name}'`); + } + await container.stop(); + return connection.stop(stopContext); }extensions/milvus/src/api/milvus-connection.ts (1)
97-170: Fix index creation on non-existentembeddingfield.The collection schema defines
id,doc_uri,text, andsparse, butcreateIndextargetsfield_name: 'embedding'. This will fail at runtime whenensureCollectionruns.Either:
- Drop the dense index entirely if you're only using BM25 sparse search, or
- Add an
embeddingvector field to the schema (with appropriateDataTypeanddim) and index that.Example minimal fix if you don't need dense vectors yet:
- // Create index on the embedding field - await client.createIndex({ - collection_name: collectionName, - field_name: 'embedding', - index_type: 'IVF_FLAT', - metric_type: 'L2', - params: { nlist: 128 }, - });
🧹 Nitpick comments (1)
extensions/milvus/src/manager/connection-manager.ts (1)
220-284: Factory logic for creating Milvus containers looks coherent, with one assumption about endpoint selection.The factory correctly:
- Validates
milvus.name,- Prepares per-connection storage and config via
ConfigHelper,- Ensures the Milvus image is present (check + pull),
- Picks a random host port and sets up port bindings, volumes, and labels,
- Starts the container and registers the connection.
The only behavioral assumption is using the first endpoint (
getEndpoints()[0]). That’s fine for an initial implementation, but if multiple endpoints are common, consider allowing users to pick or using a more explicit default later.
📜 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 (5)
extensions/milvus/src/api/milvus-connection.ts(1 hunks)extensions/milvus/src/inject/inversify-binding.ts(1 hunks)extensions/milvus/src/manager/connection-manager.spec.ts(1 hunks)extensions/milvus/src/manager/connection-manager.ts(1 hunks)extensions/milvus/src/util/config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- extensions/milvus/src/manager/connection-manager.spec.ts
- extensions/milvus/src/inject/inversify-binding.ts
- extensions/milvus/src/util/config.ts
🧰 Additional context used
📓 Path-based instructions (1)
extensions/*/src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
extensions/*/src/**/*.ts: Use @kortex-app/api for extension interactions, which provides TypeScript definitions for provider registration, configuration management, command and menu contributions, and UI components
Use the @kortex-app/api package to interact with Kortex core capabilities from extensions
Files:
extensions/milvus/src/api/milvus-connection.tsextensions/milvus/src/manager/connection-manager.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Extensions can contribute inference providers (Gemini, OpenAI-compatible, OpenShift AI), flow providers (Goose), MCP registries, and configuration properties through the extension API
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to packages/main/src/**/*.ts : Container operations should use ContainerProviderRegistry with engineId parameter to identify the container engine, supporting operations like listContainers, startContainer, stopContainer, deleteContainer, pullImage, buildImage, deleteImage, createPod, startPod, stopPod, removePod
Applied to files:
extensions/milvus/src/manager/connection-manager.ts
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Applies to packages/main/src/**/*.ts : Container operations must include the `engineId` parameter to identify the container engine when using `ContainerProviderRegistry` methods like `listContainers`, `startContainer`, `stopContainer`, `deleteContainer`, `pullImage`, `buildImage`, `deleteImage`, `createPod`, `startPod`, `stopPod`, and `removePod`
Applied to files:
extensions/milvus/src/manager/connection-manager.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to packages/main/src/plugin/index.ts : IPC communication handlers should follow the naming convention <registry-name>:<action> (e.g., container-provider-registry:listContainers) in packages/main/src/plugin/index.ts
Applied to files:
extensions/milvus/src/manager/connection-manager.ts
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Use Inversify dependency injection container in `packages/main/src/plugin/index.ts` to register all major services (ProviderRegistry, ContainerProviderRegistry, KubernetesClient, MCPManager, FlowManager, ChatManager, ConfigurationRegistry) as singletons
Applied to files:
extensions/milvus/src/manager/connection-manager.ts
🧬 Code graph analysis (1)
extensions/milvus/src/api/milvus-connection.ts (1)
packages/extension-api/src/extension-api.d.ts (5)
RagProviderConnection(687-694)ProviderConnectionStatus(360-360)MCPServer(680-683)ProviderConnectionLifecycle(375-386)LifecycleContext(371-373)
🪛 Biome (2.1.2)
extensions/milvus/src/manager/connection-manager.ts
[error] 154-154: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: smoke e2e tests (production)
- GitHub Check: smoke e2e tests (development)
- GitHub Check: Windows
- GitHub Check: linter, formatters
- GitHub Check: typecheck
- GitHub Check: Linux
🔇 Additional comments (4)
extensions/milvus/src/api/milvus-connection.ts (2)
30-77: MCP server registration and connection wiring look consistent with the extension API.The constructor correctly initializes
connectionStatus, registers the Milvus MCP server viaapi.mcpRegistry.registerServer, and exposes anmcpServerobject compatible withRagProviderConnection. This wiring looks coherent with the@kortex-app/apitypes.
173-231: Index/deindex flows and error/escape handling look good.
index()ensures the collection exists, inserts only whendata.length > 0, flushes, and rethrows on error so callers see failures.deindex()checks collection existence, deletes bydoc_uriwith proper backslash + quote escaping in the filter, and flushes deletions.This behavior is sane for RAG indexing/deindexing.
extensions/milvus/src/manager/connection-manager.ts (2)
70-91: Connection registration and lifecycle wiring look solid.
registerConnectioncorrectly reuses existing connections to update status, or creates a newMilvusConnection, wires lifecycle callbacks (start/stop/delete), and registers it viamilvusProvider.registerRagProviderConnection. The keying on${path}::${id}and cleanup through the storedDisposableis coherent.
200-218: The concern aboutPromise.withResolverscompatibility is unfounded. The Kortex runtime requires Node.js >=22.0.0 (as specified in the rootpackage.jsonengines field), andPromise.withResolversis stable and fully available in Node.js v22+. No polyfill or helper is needed.Likely an incorrect or invalid review comment.
Fixes kortex-hub#805 Signed-off-by: Jeff MAURY <jmaury@redhat.com>
Signed-off-by: Jeff MAURY <jmaury@redhat.com>
Signed-off-by: Jeff MAURY <jmaury@redhat.com>
Signed-off-by: Jeff MAURY <jmaury@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
extensions/milvus/src/extension.spec.ts (1)
29-31: Consider simplifying mock cleanup.Both
vi.restoreAllMocks()andvi.resetAllMocks()are called. Based on the past review comment,resetAllMocksalone may be sufficient for cleaning up mock state between tests.extensions/milvus/src/manager/connection-manager.spec.ts (1)
343-357: LGTM!The test correctly verifies the image pull flow when the image is not available locally. The mock setup using
mockImplementationto throw synchronously is appropriate for simulating the image-not-found scenario.Note: The past review comment about this test appears to be based on a misunderstanding of the test's intent. The synchronous throw is the correct way to simulate an unavailable image.
extensions/milvus/src/manager/connection-manager.ts (1)
105-119: Consider logging or throwing when container is not found.Both methods silently return when
getContainerreturnsundefined, which means lifecycle operations fail without notifying the caller. While the tests (lines 377-391 in the spec file) expect this behavior, it may lead to confusion when operations don't take effect.Consider at minimum adding a warning log:
async startConnection(connection: MilvusConnection, startContext: LifecycleContext): Promise<void> { const container = this.getContainer(connection); if (container) { await container.start(); return connection.start(startContext); + } else { + console.warn(`Cannot start connection '${connection.name}': container not found`); } } async stopConnection(connection: MilvusConnection, stopContext: LifecycleContext): Promise<void> { const container = this.getContainer(connection); if (container) { await container.stop(); return connection.stop(stopContext); + } else { + console.warn(`Cannot stop connection '${connection.name}': container not found`); } }extensions/milvus/src/api/milvus-connection.ts (1)
159-166: Remove index creation on non-existent 'embedding' field.The schema (lines 103-126) defines fields
id,doc_uri,text, andsparse, but this code attempts to create an index onembedding, which doesn't exist. This will cause a runtime error during collection initialization.Apply this diff to remove the invalid index creation:
await client.createCollection({ collection_name: collectionName, schema, index_params, functions, }); - // Create index on the embedding field - await client.createIndex({ - collection_name: collectionName, - field_name: 'embedding', - index_type: 'IVF_FLAT', - metric_type: 'L2', - params: { nlist: 128 }, - }); - // Load collection into memory await client.loadCollection({ collection_name: collectionName });If you need dense embeddings in addition to BM25 sparse vectors, first add an
embeddingfield to the schema before creating the index.
🧹 Nitpick comments (2)
extensions/milvus/src/inject/inversify-binding.ts (1)
44-56: Remove unnecessary optional chaining and fix misleading comment.Line 50 uses optional chaining (
this.#container?.bind) but#containerwas just created on line 45, so it's always defined. Line 54's comment mentions "Get inference model manager" but the code simply returns the container without retrieving any manager.Apply this diff:
this.#container.bind(ExtensionContextSymbol).toConstantValue(this.#extensionContext); this.#container.bind(ContainerExtensionAPISymbol).toConstantValue(this.#containerExtensionAPI); this.#container.bind(MilvusProvider).toConstantValue(this.#provider); - this.#container?.bind(ConfigHelper).toSelf().inSingletonScope(); + this.#container.bind(ConfigHelper).toSelf().inSingletonScope(); await this.#container.load(managersModule); - // Get inference model manager + // Return configured container return this.#container;extensions/milvus/src/manager/connection-manager.ts (1)
135-174: Use Number.parseInt per ES2015 conventions.Line 154 uses the global
parseIntfunction. ES2015 moved this into theNumbernamespace for consistency.Apply this diff:
this.registerConnection({ path: endpoint.path, id: container.Id, name: milvusName, - port: parseInt(milvusPort, 10), + port: Number.parseInt(milvusPort, 10), running: container.State === 'running', });Otherwise, the dispose and container discovery logic correctly handles lifecycle and cleanup, including proper index checking before splice operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
extensions/milvus/milvus-icon-color.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
extensions/milvus/README.md(1 hunks)extensions/milvus/package.json(1 hunks)extensions/milvus/src/api/milvus-connection.ts(1 hunks)extensions/milvus/src/api/milvus-container.ts(1 hunks)extensions/milvus/src/extension.spec.ts(1 hunks)extensions/milvus/src/extension.ts(1 hunks)extensions/milvus/src/inject/inversify-binding.ts(1 hunks)extensions/milvus/src/inject/symbol.ts(1 hunks)extensions/milvus/src/manager/_manager-module.ts(1 hunks)extensions/milvus/src/manager/connection-manager.spec.ts(1 hunks)extensions/milvus/src/manager/connection-manager.ts(1 hunks)extensions/milvus/src/milvus-extension.ts(1 hunks)extensions/milvus/src/util/config.ts(1 hunks)extensions/milvus/tsconfig.json(1 hunks)extensions/milvus/vite.config.js(1 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- extensions/milvus/tsconfig.json
- extensions/milvus/README.md
🧰 Additional context used
📓 Path-based instructions (5)
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests should be co-located with source code as
*.spec.tsfilesUnit tests should be co-located with source code using the *.spec.ts naming convention
Files:
extensions/milvus/src/manager/connection-manager.spec.tsextensions/milvus/src/extension.spec.ts
extensions/*/src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
extensions/*/src/**/*.ts: Use @kortex-app/api for extension interactions, which provides TypeScript definitions for provider registration, configuration management, command and menu contributions, and UI components
Use the @kortex-app/api package to interact with Kortex core capabilities from extensions
Files:
extensions/milvus/src/manager/connection-manager.spec.tsextensions/milvus/src/manager/_manager-module.tsextensions/milvus/src/util/config.tsextensions/milvus/src/api/milvus-container.tsextensions/milvus/src/inject/inversify-binding.tsextensions/milvus/src/api/milvus-connection.tsextensions/milvus/src/inject/symbol.tsextensions/milvus/src/extension.tsextensions/milvus/src/manager/connection-manager.tsextensions/milvus/src/milvus-extension.tsextensions/milvus/src/extension.spec.ts
extensions/*/vite.config.js
📄 CodeRabbit inference engine (AGENTS.md)
Extensions should be built using Vite with build output to ./dist/extension.js
Files:
extensions/milvus/vite.config.js
extensions/*/src/extension.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Extensions must export a standard
activate()function insrc/extension.tsExtensions must export a standard activation API through their entry point
Files:
extensions/milvus/src/extension.ts
extensions/*/package.json
📄 CodeRabbit inference engine (CLAUDE.md)
extensions/*/package.json: Extensions must declareengines.kortexversion compatibility inpackage.json
Extensionpackage.jsonmust havemainfield pointing to./dist/extension.js
Configuration properties should be defined in extensionpackage.jsonundercontributes.configuration.propertieswith appropriate scopes (DEFAULT, ContainerProviderConnection, KubernetesProviderConnection, InferenceProviderConnection, InferenceProviderConnectionFactory)
extensions/*/package.json: Extensions must be located in the extensions/ directory with a standard structure including a package.json with main pointing to ./dist/extension.js and must declare engines.kortex version compatibility
Extension package.json must declare engines.kortex version compatibility
Extension main entry point must point to ./dist/extension.js in package.json
Extensions should contribute inference providers, flow providers, MCP registries, and/or configuration properties through the package.json contributes field
Configuration properties should be defined in extension package.json under contributes.configuration.properties with proper scopes (DEFAULT, ContainerProviderConnection, KubernetesProviderConnection, InferenceProviderConnection, InferenceProviderConnectionFactory)
Configuration scopes limit exposure of sensitive data and should be used appropriately when defining configuration properties
Files:
extensions/milvus/package.json
🧠 Learnings (25)
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Use Inversify dependency injection container in `packages/main/src/plugin/index.ts` to register all major services (ProviderRegistry, ContainerProviderRegistry, KubernetesClient, MCPManager, FlowManager, ChatManager, ConfigurationRegistry) as singletons
Applied to files:
extensions/milvus/src/manager/_manager-module.tsextensions/milvus/src/util/config.tsextensions/milvus/src/inject/inversify-binding.tsextensions/milvus/src/manager/connection-manager.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Use Inversify dependency injection for the plugin system implementation, with the core PluginSystem class in packages/main/src/plugin/index.ts orchestrating extension loading and service registration
Applied to files:
extensions/milvus/src/manager/_manager-module.tsextensions/milvus/src/util/config.tsextensions/milvus/src/inject/inversify-binding.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to extensions/*/vite.config.js : Extensions should be built using Vite with build output to ./dist/extension.js
Applied to files:
extensions/milvus/vite.config.js
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Extensions should be built using Vite and output to `dist/extension.js`
Applied to files:
extensions/milvus/vite.config.jsextensions/milvus/package.json
📚 Learning: 2025-11-05T16:31:22.355Z
Learnt from: benoitf
Repo: kortex-hub/kortex PR: 678
File: extensions/container/packages/api/vite.config.js:34-40
Timestamp: 2025-11-05T16:31:22.355Z
Learning: In the kortex repository, the `kortex-app/container-extension-api` package at `extensions/container/packages/api/` is a declaration-only package that ships pure TypeScript declaration files (.d.ts) without any compiled JavaScript. It intentionally has an empty build script and does not require library build configuration in vite.config.js.
Applied to files:
extensions/milvus/vite.config.jsextensions/milvus/src/api/milvus-container.tsextensions/milvus/package.json
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to extensions/*/package.json : Configuration scopes limit exposure of sensitive data and should be used appropriately when defining configuration properties
Applied to files:
extensions/milvus/vite.config.jsextensions/milvus/package.json
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to packages/main/src/**/*.ts : Configuration values should be retrieved and stored with dot notation (e.g., gemini.factory.apiKey)
Applied to files:
extensions/milvus/src/util/config.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Register all major services (ProviderRegistry, ContainerProviderRegistry, KubernetesClient, MCPManager, MCPRegistry, FlowManager, ChatManager, ConfigurationRegistry) as singletons in the Inversify DI container during initialization
Applied to files:
extensions/milvus/src/util/config.tsextensions/milvus/src/inject/inversify-binding.ts
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Applies to packages/main/src/**/*.ts : Container operations must include the `engineId` parameter to identify the container engine when using `ContainerProviderRegistry` methods like `listContainers`, `startContainer`, `stopContainer`, `deleteContainer`, `pullImage`, `buildImage`, `deleteImage`, `createPod`, `startPod`, `stopPod`, and `removePod`
Applied to files:
extensions/milvus/src/api/milvus-container.tsextensions/milvus/src/manager/connection-manager.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to packages/main/src/**/*.ts : Container operations should use ContainerProviderRegistry with engineId parameter to identify the container engine, supporting operations like listContainers, startContainer, stopContainer, deleteContainer, pullImage, buildImage, deleteImage, createPod, startPod, stopPod, removePod
Applied to files:
extensions/milvus/src/api/milvus-container.tsextensions/milvus/src/manager/connection-manager.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to extensions/*/src/**/*.ts : Use kortex-app/api for extension interactions, which provides TypeScript definitions for provider registration, configuration management, command and menu contributions, and UI components
Applied to files:
extensions/milvus/src/inject/inversify-binding.tsextensions/milvus/src/inject/symbol.tsextensions/milvus/src/extension.tsextensions/milvus/src/milvus-extension.tsextensions/milvus/package.json
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Extensions can contribute inference providers (Gemini, OpenAI-compatible, OpenShift AI), flow providers (Goose), MCP registries, and configuration properties through the extension API
Applied to files:
extensions/milvus/src/inject/symbol.ts
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Applies to extensions/*/src/extension.ts : Extensions must export a standard `activate()` function in `src/extension.ts`
Applied to files:
extensions/milvus/src/extension.tsextensions/milvus/src/milvus-extension.tsextensions/milvus/src/extension.spec.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to extensions/*/src/extension.ts : Extensions must export a standard activation API through their entry point
Applied to files:
extensions/milvus/src/extension.tsextensions/milvus/src/milvus-extension.tsextensions/milvus/src/extension.spec.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to extensions/*/src/**/*.ts : Use the kortex-app/api package to interact with Kortex core capabilities from extensions
Applied to files:
extensions/milvus/src/extension.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to packages/main/src/plugin/index.ts : IPC communication handlers should follow the naming convention <registry-name>:<action> (e.g., container-provider-registry:listContainers) in packages/main/src/plugin/index.ts
Applied to files:
extensions/milvus/src/manager/connection-manager.ts
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Applies to extensions/*/package.json : Extensions must declare `engines.kortex` version compatibility in `package.json`
Applied to files:
extensions/milvus/src/milvus-extension.tsextensions/milvus/package.json
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to extensions/*/package.json : Extensions must be located in the extensions/ directory with a standard structure including a package.json with main pointing to ./dist/extension.js and must declare engines.kortex version compatibility
Applied to files:
extensions/milvus/src/milvus-extension.tsextensions/milvus/package.json
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to extensions/*/package.json : Extension package.json must declare engines.kortex version compatibility
Applied to files:
extensions/milvus/src/milvus-extension.tsextensions/milvus/package.json
📚 Learning: 2025-11-08T14:36:17.423Z
Learnt from: benoitf
Repo: kortex-hub/kortex PR: 692
File: extensions/ramalama/src/inject/inversify-binding.spec.ts:38-38
Timestamp: 2025-11-08T14:36:17.423Z
Learning: In the kortex repository, tests use `vi.mock(import('module-path'))` syntax instead of `vi.mock('module-path')` to ensure TypeScript validates the module path at compile time, providing refactoring safety if modules are moved or removed.
Applied to files:
extensions/milvus/src/extension.spec.ts
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to extensions/*/package.json : Extensions should contribute inference providers, flow providers, MCP registries, and/or configuration properties through the package.json contributes field
Applied to files:
extensions/milvus/package.json
📚 Learning: 2025-11-25T14:41:44.065Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:41:44.065Z
Learning: Applies to extensions/*/package.json : Configuration properties should be defined in extension package.json under contributes.configuration.properties with proper scopes (DEFAULT, ContainerProviderConnection, KubernetesProviderConnection, InferenceProviderConnection, InferenceProviderConnectionFactory)
Applied to files:
extensions/milvus/package.json
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Applies to extensions/*/package.json : Configuration properties should be defined in extension `package.json` under `contributes.configuration.properties` with appropriate scopes (DEFAULT, ContainerProviderConnection, KubernetesProviderConnection, InferenceProviderConnection, InferenceProviderConnectionFactory)
Applied to files:
extensions/milvus/package.json
📚 Learning: 2025-11-25T14:41:20.640Z
Learnt from: CR
Repo: kortex-hub/kortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:41:20.640Z
Learning: Applies to extensions/*/package.json : Extension `package.json` must have `main` field pointing to `./dist/extension.js`
Applied to files:
extensions/milvus/package.json
📚 Learning: 2025-11-07T15:50:51.762Z
Learnt from: benoitf
Repo: kortex-hub/kortex PR: 691
File: extensions/container/packages/api/package.json:1-25
Timestamp: 2025-11-07T15:50:51.762Z
Learning: API packages at paths like `extensions/*/packages/api/` that export only TypeScript declaration files (.d.ts) and have empty build scripts in package.json are declaration-only packages and do not require the `engines.kortex` field, as they are not loaded at runtime.
Applied to files:
extensions/milvus/package.json
🧬 Code graph analysis (6)
extensions/milvus/vite.config.js (1)
eslint.config.mjs (1)
__dirname(40-40)
extensions/milvus/src/inject/inversify-binding.ts (3)
__mocks__/@kortex-app/api.js (1)
provider(27-29)extensions/milvus/src/inject/symbol.ts (3)
ExtensionContextSymbol(22-22)ContainerExtensionAPISymbol(21-21)MilvusProvider(20-20)extensions/milvus/src/manager/_manager-module.ts (1)
managersModule(27-27)
extensions/milvus/src/extension.ts (2)
extensions/milvus/src/milvus-extension.ts (3)
MilvusExtension(27-79)activate(37-70)deactivate(76-78)packages/extension-api/src/extension-api.d.ts (1)
ExtensionContext(184-204)
extensions/milvus/src/manager/connection-manager.ts (5)
extensions/milvus/src/api/milvus-connection.ts (1)
MilvusConnection(25-236)extensions/milvus/src/inject/symbol.ts (3)
ExtensionContextSymbol(22-22)ContainerExtensionAPISymbol(21-21)MilvusProvider(20-20)packages/extension-api/src/extension-api.d.ts (4)
ExtensionContext(184-204)ProviderConnectionLifecycle(375-386)LifecycleContext(371-373)Logger(362-369)extensions/container/packages/api/src/container-extension-api.d.ts (1)
ContainerExtensionAPI(32-40)extensions/milvus/src/api/milvus-container.ts (1)
MilvusContainer(18-24)
extensions/milvus/src/milvus-extension.ts (3)
packages/extension-api/src/extension-api.d.ts (1)
ExtensionContext(184-204)extensions/milvus/src/inject/inversify-binding.ts (1)
InversifyBinding(27-63)extensions/container/packages/api/src/container-extension-api.d.ts (1)
ContainerExtensionAPI(32-40)
extensions/milvus/src/extension.spec.ts (3)
packages/extension-api/src/extension-api.d.ts (1)
ExtensionContext(184-204)extensions/milvus/src/milvus-extension.ts (3)
MilvusExtension(27-79)activate(37-70)deactivate(76-78)extensions/milvus/src/extension.ts (2)
activate(24-27)deactivate(29-32)
🪛 Biome (2.1.2)
extensions/milvus/src/manager/connection-manager.ts
[error] 154-154: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
🔇 Additional comments (25)
extensions/milvus/src/manager/_manager-module.ts (1)
23-25: LGTM: Proper Inversify module setup.The ConnectionManager is correctly bound as a singleton using the standard Inversify pattern.
extensions/milvus/src/api/milvus-container.ts (1)
18-24: LGTM: Clean interface definition.The MilvusContainer interface provides a clear contract for container metadata.
extensions/milvus/src/extension.spec.ts (1)
38-84: LGTM: Comprehensive test coverage.The test suite properly validates:
- Extension lifecycle (activate/deactivate)
- Error handling during activation
- Singleton instance reuse
- Safe deactivation before activation
extensions/milvus/src/inject/symbol.ts (1)
20-22: LGTM: Standard DI symbol definitions.The injection symbols follow the standard Inversify pattern for type-safe dependency injection.
extensions/milvus/src/milvus-extension.ts (1)
37-70: LGTM: Robust activation flow.The activation sequence properly:
- Validates container extension availability
- Creates and registers the Milvus provider
- Initializes Inversify bindings
- Resolves and initializes the ConnectionManager
- Handles errors appropriately
extensions/milvus/package.json (2)
8-14: LGTM: Extension manifest follows guidelines.The package.json correctly declares:
engines.kortexversion compatibilitymainfield pointing to./dist/extension.js- Extension dependency on
kortex.container
20-24: The configuration scope "RagProviderConnectionFactory" is valid and properly defined in the extension API.Likely an incorrect or invalid review comment.
extensions/milvus/vite.config.js (1)
37-55: LGTM: Build configuration follows extension guidelines.The Vite build config correctly:
- Outputs to
./dist/extension.js- Uses CommonJS format
- Externalizes
@kortex-app/apiand Node builtins- Enables inline sourcemaps
extensions/milvus/src/util/config.ts (1)
30-38: LGTM: ConfigHelper properly structured for DI.The class is correctly decorated with
@injectable()and the singleton binding is handled in the container module setup. ThecreateConfigFilemethod appropriately generates the required etcd and user configuration files.extensions/milvus/src/inject/inversify-binding.ts (2)
34-42: LGTM!The constructor properly initializes all required dependencies for DI binding setup.
58-62: LGTM!The dispose method properly cleans up the Inversify container bindings.
extensions/milvus/src/manager/connection-manager.spec.ts (3)
66-119: LGTM!The test setup comprehensively mocks all dependencies and establishes the Inversify container with proper bindings, following DI best practices.
Based on learnings, Inversify DI is used correctly for wiring the ConnectionManager and its dependencies.
122-207: LGTM!The lifecycle tests comprehensively verify connection initialization, registration (both new and update paths), and start/stop operations with proper mock expectations.
209-398: LGTM!The remaining tests provide excellent coverage of cleanup operations (delete, dispose), container discovery with proper label filtering, factory creation flows, and graceful error handling for missing containers.
extensions/milvus/src/extension.ts (1)
29-32: LGTM!The deactivate function properly awaits the deactivation and clears the singleton reference.
extensions/milvus/src/api/milvus-connection.ts (4)
30-77: LGTM!The constructor properly initializes the connection status and MCP server configuration, including package arguments for the Milvus URI.
79-95: LGTM!The lifecycle methods (status, start, stop) and Milvus client factory are correctly implemented.
173-207: LGTM!The index method correctly reads document chunks, prepares data for insertion, and handles errors appropriately by logging and rethrowing.
209-235: LGTM!The deindex method correctly escapes the URI to prevent filter injection vulnerabilities (line 219), and the updateStatus method properly updates internal state.
extensions/milvus/src/manager/connection-manager.ts (6)
50-52: LGTM!The random port generation correctly uses the valid TCP port range 1024-65535 (with 65536 as exclusive upper bound).
70-91: LGTM!The registerConnection method correctly handles both new connection creation and status updates for existing connections, properly wiring lifecycle hooks.
93-103: LGTM!The getContainer helper correctly resolves the Dockerode container by looking up the connection and finding the matching endpoint.
121-133: LGTM!The deleteConnection method properly removes the container, disposes the connection, and cleans up the storage directory.
176-218: LGTM!The initialization wires up container discovery with change listeners and registers the factory. The image checking and pulling logic correctly handles errors and progress tracking.
220-284: LGTM!The factory method comprehensively handles Milvus connection creation, including parameter validation, storage setup, image availability, container creation with proper configuration (environment, binds, labels), and connection registration.
| export async function activate(extensionContext: ExtensionContext): Promise<void> { | ||
| milvusExtension ??= new MilvusExtension(extensionContext); | ||
| milvusExtension.activate().catch(console.error); | ||
| } |
There was a problem hiding this comment.
Await the activation promise to ensure proper lifecycle.
The activate function returns a Promise<void> but doesn't await milvusExtension.activate(), causing the function to return before activation completes. This breaks the extension activation contract and may lead to race conditions.
Apply this diff:
export async function activate(extensionContext: ExtensionContext): Promise<void> {
milvusExtension ??= new MilvusExtension(extensionContext);
- milvusExtension.activate().catch(console.error);
+ await milvusExtension.activate();
}If you want to catch and log activation errors without propagating them, use try-catch:
export async function activate(extensionContext: ExtensionContext): Promise<void> {
milvusExtension ??= new MilvusExtension(extensionContext);
- milvusExtension.activate().catch(console.error);
+ try {
+ await milvusExtension.activate();
+ } catch (err) {
+ console.error('Failed to activate Milvus extension:', err);
+ throw err;
+ }
}Based on learnings, extensions must export a standard activate() function that properly awaits initialization.
📝 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.
| export async function activate(extensionContext: ExtensionContext): Promise<void> { | |
| milvusExtension ??= new MilvusExtension(extensionContext); | |
| milvusExtension.activate().catch(console.error); | |
| } | |
| export async function activate(extensionContext: ExtensionContext): Promise<void> { | |
| milvusExtension ??= new MilvusExtension(extensionContext); | |
| await milvusExtension.activate(); | |
| } |
🤖 Prompt for AI Agents
In extensions/milvus/src/extension.ts around lines 24 to 27, the exported
activate function calls milvusExtension.activate() but does not await it,
causing the function to return before activation completes; update the function
to await the activation promise and handle errors (e.g., wrap the await in a
try-catch and log any errors) so the activate() lifecycle completes only after
initialization finishes and activation failures are logged.
| import { join } from 'path'; | ||
| import { builtinModules } from 'module'; | ||
|
|
||
| const PACKAGE_ROOT = __dirname; |
There was a problem hiding this comment.
__dirname is undefined in ES modules.
Line 22 uses __dirname, but since this file uses ES module syntax (import), __dirname is not available. This will cause a runtime error when the config is loaded.
Apply this diff to define __dirname for ES modules:
import { join } from 'path';
+import { fileURLToPath } from 'url';
+import { dirname } from 'path';
import { builtinModules } from 'module';
+const __filename = fileURLToPath(import.meta.url);
const PACKAGE_ROOT = __dirname;
+const PACKAGE_ROOT = dirname(__filename);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In extensions/milvus/vite.config.js around lines 19 to 22, __dirname is used but
undefined in ES modules; update the file to derive __dirname from
import.meta.url by importing fileURLToPath from 'url' and dirname from 'path'
and then set const __dirname = dirname(fileURLToPath(import.meta.url)); replace
the current __dirname usage with this value so the config works under ESM.
Signed-off-by: Jeff MAURY <jmaury@redhat.com>
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
6 participants
Add a new Milvus extension.
To test it, go to Resource, you should see a new Milvus resource, click Create, give a name, it should:
One you have the port, you should be able to launch the MCP server using
uvx mcp-server-milvus --milvus-uri http://localhost:$portCheck with the MCP inspector that you can create collections,....
Fixes #805