fix: compute agent supported types on the inference connections factories - #2149
Merged
Merged
Conversation
…ries Fixes openkaiden#2148 Signed-off-by: Jeff MAURY <jmaury@redhat.com>
jeffmaury
requested review from
benoitf and
gastoner
and removed request for
a team
June 12, 2026 10:10
Contributor
📝 WalkthroughWalkthroughThis PR extends the plugin architecture to recognize inference connection factories as a first-class connection type and wires AgentRegistry to derive supported model types from provider factory metadata, resolving the issue where supported types were not computed without catalog models. ChangesInference Factory Type and Event Flow
Sequence DiagramsequenceDiagram
participant Provider as ProviderImpl
participant Registry as ProviderRegistry
participant AgentReg as AgentRegistry
Provider->>Registry: onDidSetConnectionFactoryCallback(type: 'inference', ...)
Registry->>AgentReg: signal factory set event
AgentReg->>AgentReg: invalidate() cache
Provider->>Registry: onDidUnsetConnectionFactoryCallback(type: 'inference', ...)
Registry->>AgentReg: signal factory unset event
AgentReg->>AgentReg: invalidate() cache
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/main/src/plugin/provider-registry.ts (2)
1740-1775:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
getConnectionFactories()omitsinferencefactories despite the expanded contract.After adding
factoryType: 'inference'support,getConnectionFactories()still returns only container/kubernetes/vm entries. This creates a runtime contract gap: consumers querying current factories won’t see inference factories, even though set/unset events now advertise them.Suggested fix
getConnectionFactories(): ConnectionFactoryDetails[] { const factories: ConnectionFactoryDetails[] = []; this.providers.forEach(provider => { @@ if (provider.vmProviderConnectionFactory?.create) { factories.push({ providerId: provider.id, type: 'vm', creationDisplayName: provider.vmProviderConnectionFactory?.creationDisplayName, creationButtonTitle: provider.vmProviderConnectionFactory?.creationButtonTitle, emptyConnectionMarkdownDescription: provider.emptyConnectionMarkdownDescription, images: provider.images, }); } + if (provider.inferenceProviderConnectionFactory?.create) { + factories.push({ + providerId: provider.id, + type: 'inference', + creationDisplayName: provider.inferenceProviderConnectionFactory?.creationDisplayName, + creationButtonTitle: provider.inferenceProviderConnectionFactory?.creationButtonTitle, + emptyConnectionMarkdownDescription: provider.emptyConnectionMarkdownDescription, + images: provider.images, + }); + } }); return factories; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/main/src/plugin/provider-registry.ts` around lines 1740 - 1775, getConnectionFactories() currently only emits container/kubernetes/vm factory entries, so add handling for providers that expose an inference factory; detect provider.inferenceProviderConnectionFactory?.create and push a ConnectionFactoryDetails with providerId: provider.id, type: 'inference' (or factoryType: 'inference' if your ConnectionFactoryDetails shape uses that field), and copy creationDisplayName, creationButtonTitle, emptyConnectionMarkdownDescription, and images from the provider just like the other branches; update any property name used by your contract (type vs factoryType) to match the expanded contract so inference factories are returned to consumers and align with the set/unset events.
1853-1859:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInference unregister callback fires the wrong event emitter.
On Line 1858,
onDidUnregisterInferenceConnectionCallbackcalls_onDidUnregisterKubernetesConnection.fire(...)instead of_onDidUnregisterInferenceConnection.fire(...). This misroutes events and prevents inference unsubscribe listeners from being notified correctly.Suggested fix
onDidUnregisterInferenceConnectionCallback( provider: ProviderImpl, inferenceProviderConnection: InferenceProviderConnection, ): void { this.apiSender.send('provider-unregister-inference-connection', { name: inferenceProviderConnection.name }); - this._onDidUnregisterKubernetesConnection.fire({ providerId: provider.id }); + this._onDidUnregisterInferenceConnection.fire({ providerId: provider.id }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/main/src/plugin/provider-registry.ts` around lines 1853 - 1859, The unregister callback onDidUnregisterInferenceConnectionCallback is firing the wrong emitter: replace the call to this._onDidUnregisterKubernetesConnection.fire({ providerId: provider.id }) with this._onDidUnregisterInferenceConnection.fire({ providerId: provider.id }) so inference listeners are notified; locate the method onDidUnregisterInferenceConnectionCallback and change the emitter reference (use _onDidUnregisterInferenceConnection) while keeping the existing apiSender.send(...) call and the same payload structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/main/src/plugin/provider-registry.ts`:
- Around line 1740-1775: getConnectionFactories() currently only emits
container/kubernetes/vm factory entries, so add handling for providers that
expose an inference factory; detect
provider.inferenceProviderConnectionFactory?.create and push a
ConnectionFactoryDetails with providerId: provider.id, type: 'inference' (or
factoryType: 'inference' if your ConnectionFactoryDetails shape uses that
field), and copy creationDisplayName, creationButtonTitle,
emptyConnectionMarkdownDescription, and images from the provider just like the
other branches; update any property name used by your contract (type vs
factoryType) to match the expanded contract so inference factories are returned
to consumers and align with the set/unset events.
- Around line 1853-1859: The unregister callback
onDidUnregisterInferenceConnectionCallback is firing the wrong emitter: replace
the call to this._onDidUnregisterKubernetesConnection.fire({ providerId:
provider.id }) with this._onDidUnregisterInferenceConnection.fire({ providerId:
provider.id }) so inference listeners are notified; locate the method
onDidUnregisterInferenceConnectionCallback and change the emitter reference (use
_onDidUnregisterInferenceConnection) while keeping the existing
apiSender.send(...) call and the same payload structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 82614c50-f622-4320-853f-eea91c8b4e17
📒 Files selected for processing (6)
packages/extension-api/src/extension-api.d.tspackages/main/src/plugin/agent-registry.spec.tspackages/main/src/plugin/agent-registry.tspackages/main/src/plugin/provider-impl.tspackages/main/src/plugin/provider-registry.spec.tspackages/main/src/plugin/provider-registry.ts
📜 Review details
⏰ 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). (10)
- GitHub Check: macOS
- GitHub Check: Linux
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: unit tests / windows-2022
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: linter, formatters
- GitHub Check: Windows
- GitHub Check: typecheck
- GitHub Check: unit tests / macos-15
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
/@/path aliases instead of relative paths for imports outside the current directory's module group; use relative imports only for sibling modules within the same directory
Files:
packages/main/src/plugin/provider-impl.tspackages/main/src/plugin/agent-registry.tspackages/extension-api/src/extension-api.d.tspackages/main/src/plugin/provider-registry.spec.tspackages/main/src/plugin/provider-registry.tspackages/main/src/plugin/agent-registry.spec.ts
packages/main/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/main/src/**/*.{ts,tsx}: UseipcHandle()to expose handlers in the main process with naming convention<registry-name>:<action>(e.g.,container-provider-registry:listContainers)
UseapiSender.send()to send events from main process to renderer for real-time updates
Long-running operations should useTaskManager.createTask()with title and action configuration
Files:
packages/main/src/plugin/provider-impl.tspackages/main/src/plugin/agent-registry.tspackages/main/src/plugin/provider-registry.spec.tspackages/main/src/plugin/provider-registry.tspackages/main/src/plugin/agent-registry.spec.ts
packages/{main,renderer,preload}/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Container operations must include
engineIdparameter to identify the container engine
Files:
packages/main/src/plugin/provider-impl.tspackages/main/src/plugin/agent-registry.tspackages/main/src/plugin/provider-registry.spec.tspackages/main/src/plugin/provider-registry.tspackages/main/src/plugin/agent-registry.spec.ts
**/*.spec.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.spec.{ts,tsx,js,jsx}: Usetest()instead ofit()for test cases in Vitest unit tests
Usevi.mock(import('...'))for auto-mocking modules in unit tests; avoid manual mock factories when possible
Usevi.resetAllMocks()inbeforeEachhooks instead ofvi.clearAllMocks()for resetting mocks between tests
When an auto-mocked function or class method needs a real implementation, usevi.mocked(...)with the prototype pattern for class methods:vi.mocked(MyClass.prototype.myMethod).mockImplementation(...)
Files:
packages/main/src/plugin/provider-registry.spec.tspackages/main/src/plugin/agent-registry.spec.ts
🧠 Learnings (2)
📚 Learning: 2026-03-09T08:47:09.657Z
Learnt from: benoitf
Repo: kortex-hub/kortex PR: 1077
File: packages/main/src/plugin/skill/skill-manager.ts:80-109
Timestamp: 2026-03-09T08:47:09.657Z
Learning: In the kortex-hub/kortex repository, IPC handlers (via ipcHandle()) may be registered directly inside feature manager/service classes (e.g., SkillManager in packages/main/src/plugin/skill/skill-manager.ts) rather than exclusively in packages/main/src/plugin/index.ts. Treat this as an accepted design pattern for files under the plugin directory. Reviewers should not require centralization in index.ts; allow IPC registration proximity to the feature that owns the handler. When reviewing code, accept direct ipcHandle() registrations inside feature managers and ensure the pattern is consistently applied across similar feature-manager modules.
Applied to files:
packages/main/src/plugin/provider-impl.tspackages/main/src/plugin/agent-registry.tspackages/main/src/plugin/provider-registry.spec.tspackages/main/src/plugin/provider-registry.tspackages/main/src/plugin/agent-registry.spec.ts
📚 Learning: 2026-05-12T17:14:02.153Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1850
File: packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte:66-70
Timestamp: 2026-05-12T17:14:02.153Z
Learning: When reviewing code that uses `AgentWorkspaceSummaryUI.runtime`, treat it as a required, non-null `string` per the `openkaiden/kdn-api` 0.12.0 schema. Therefore, code like `a.runtime.localeCompare(b.runtime)` is safe and should not trigger warnings about possible `undefined`/`null` values or suggestions to use nullish coalescing/optional chaining for `runtime` (unless the current local types still mark `runtime` as optional, indicating a schema/version mismatch).
Applied to files:
packages/main/src/plugin/provider-impl.tspackages/main/src/plugin/agent-registry.tspackages/extension-api/src/extension-api.d.tspackages/main/src/plugin/provider-registry.spec.tspackages/main/src/plugin/provider-registry.tspackages/main/src/plugin/agent-registry.spec.ts
🔇 Additional comments (6)
packages/extension-api/src/extension-api.d.ts (1)
1204-1204: LGTM!packages/main/src/plugin/provider-registry.ts (1)
1715-1719: LGTM!Also applies to: 1730-1733
packages/main/src/plugin/provider-impl.ts (1)
331-335: LGTM!packages/main/src/plugin/provider-registry.spec.ts (1)
204-251: LGTM!packages/main/src/plugin/agent-registry.ts (1)
23-23: LGTM!Also applies to: 41-41, 44-53, 71-76
packages/main/src/plugin/agent-registry.spec.ts (1)
24-24: LGTM!Also applies to: 28-28, 40-44, 71-79, 83-83, 85-85, 244-279
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2148