feat: create secret information based on new configuration - #2073
Merged
Merged
Conversation
jeffmaury
requested review from
benoitf and
fbricon
and removed request for
a team
June 4, 2026 14:00
Contributor
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (5)**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.spec.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
extensions/*/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/main/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/{main,renderer,preload}/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (6)📚 Learning: 2026-05-05T17:44:50.991ZApplied to files:
📚 Learning: 2026-05-12T10:01:14.248ZApplied to files:
📚 Learning: 2026-05-12T17:14:02.153ZApplied to files:
📚 Learning: 2026-03-09T08:47:09.657ZApplied to files:
📚 Learning: 2026-05-05T17:30:20.418ZApplied to files:
📚 Learning: 2026-06-08T14:22:17.045ZApplied to files:
🛑 Comments failed to post (2)
🔇 Additional comments (8)
📝 WalkthroughWalkthroughStandardize per-connection configuration keys to fully-qualified namespaced keys across Cursor, Mistral, and OpenAI; add ProviderRegistry model->connection lookup and AgentWorkspaceManager logic to derive and create model secrets from provider connection configuration stored in extension safe storage. ChangesConfiguration and Secret Creation Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@extensions/mistral/src/manager/mistral-inference-manager.ts`:
- Around line 117-119: The config clearing uses a different scope than the write
path causing mismatched keys; ensure the same scope is used when clearing by
either (A) calling configuration.getConfiguration(undefined, connection) and
updating 'mistral.connection._type' and 'mistral.connection.token' (matching the
writes), or (B) calling configuration.getConfiguration('mistral.connection',
connection) and updating '_type' and 'token' (without the 'mistral.connection.'
prefix); update the code around the config variable in
mistral-inference-manager.ts to use one consistent approach for getConfiguration
and update to avoid leaving stale keys.
In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts`:
- Around line 140-144: The empty finally is being used to swallow errors from
ensureModelSecretFromConfig in agent-workspace-manager.ts which is unclear;
replace the try...finally with an explicit try/catch around await
this.ensureModelSecretFromConfig(options) and in the catch block either rethrow
or intentionally suppress with a clear comment and a debug-level log (e.g.,
processLogger.debug or this.logger.debug) that records the caught error and why
it is safe to ignore; keep the original return behavior when
ensureModelSecretFromConfig resolves truthy and only suppress errors if that was
the original intent so callers' behavior remains unchanged.
In `@packages/main/src/plugin/provider-registry.ts`:
- Around line 2263-2282: Extract the duplicated model-resolution loop in
getInferenceConnection and getInferenceConnectionCredentials into a new private
helper (e.g. findInferenceConnectionForModel) that takes modelId and returns the
matching provider and connection ({ provider: ProviderImpl; connection:
InferenceProviderConnection } | undefined); have the helper perform the same
parsing of modelId (metadataName, modelLabel, endpoint) and the same matching
logic (connection.llmMetadata?.name, connection.endpoint,
connection.models.some(m => m.label === modelLabel)); then update
getInferenceConnectionCredentials to call the helper and return
credentials/llmMetadataName/endpoint from found.connection, and update
getInferenceConnection to call the helper and return connection and
provider.extensionId—remove the duplicated loop from both methods.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2f5dc5cf-2a93-4ba7-aabb-2806e698aca3
📒 Files selected for processing (9)
extensions/cursor/src/manager/cursor-inference-manager.spec.tsextensions/cursor/src/manager/cursor-inference-manager.tsextensions/mistral/src/manager/mistral-inference-manager.spec.tsextensions/mistral/src/manager/mistral-inference-manager.tsextensions/openai-compatible/src/openAI.spec.tsextensions/openai-compatible/src/openAI.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.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: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: unit tests / windows-2025
- GitHub Check: unit tests / macos-15
- GitHub Check: linter, formatters
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: Linux
- GitHub Check: macOS
- GitHub Check: Windows
- GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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:
extensions/mistral/src/manager/mistral-inference-manager.spec.tsextensions/cursor/src/manager/cursor-inference-manager.tsextensions/cursor/src/manager/cursor-inference-manager.spec.tspackages/main/src/plugin/provider-registry.tsextensions/openai-compatible/src/openAI.tsextensions/mistral/src/manager/mistral-inference-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tsextensions/openai-compatible/src/openAI.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:
extensions/mistral/src/manager/mistral-inference-manager.spec.tsextensions/cursor/src/manager/cursor-inference-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tsextensions/openai-compatible/src/openAI.spec.ts
extensions/*/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Register inference, container, and Kubernetes providers through the
ProviderRegistryvia extension APIs
Files:
extensions/mistral/src/manager/mistral-inference-manager.spec.tsextensions/cursor/src/manager/cursor-inference-manager.tsextensions/cursor/src/manager/cursor-inference-manager.spec.tsextensions/openai-compatible/src/openAI.tsextensions/mistral/src/manager/mistral-inference-manager.tsextensions/openai-compatible/src/openAI.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-registry.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.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-registry.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
🧠 Learnings (31)
📓 Common learnings
Learnt from: bmahabirbu
Repo: openkaiden/kaiden PR: 1517
File: packages/main/src/plugin/kdn-cli/kdn-cli.ts:181-189
Timestamp: 2026-05-05T03:38:00.098Z
Learning: In the openkaiden/kaiden repository, the explicit design decision for `writeWorkspaceConfig` (packages/main/src/plugin/kdn-cli/kdn-cli.ts) is: only non-secret runtime config flags (e.g. UV_SYSTEM_CERTS) are written as plaintext env vars to workspace.json for command/package-spawned MCPs. When a spawned MCP requires a secret env var, the planned approach is to route it through `kdn secret create` and write a secret reference (not the raw value) into workspace.json. This will be implemented in a separate follow-up PR.
Learnt from: bmahabirbu
Repo: openkaiden/kaiden PR: 1517
File: packages/main/src/plugin/kdn-cli/kdn-cli.ts:167-177
Timestamp: 2026-05-03T09:13:45.692Z
Learning: In the openkaiden/kaiden repository, URL-based (remote) MCPs that require API tokens use the secret manager (`SafeStorageRegistry`) for credential storage. Command/package-spawned MCPs (e.g., uvx, npx) generally do not require API keys, so their env vars (e.g., UV_SYSTEM_CERTS) are safely persisted in plaintext in workspace.json. Flagging plaintext env persistence in `writeWorkspaceConfig` (packages/main/src/plugin/kdn-cli/kdn-cli.ts) as a security issue for command-based MCPs is not currently applicable, but may be worth a follow-up if command-based MCPs ever need secrets.
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1671
File: extensions/vertex-ai/src/vertex-ai.ts:272-302
Timestamp: 2026-05-05T17:30:24.999Z
Learning: In the `openkaiden/kaiden` repository, cloud provider extensions (Gemini, Claude, Mistral, OpenAI-compatible, Vertex AI) use `ProviderConnectionStatus = 'unknown'` when registering inference provider connections. This means "connection was set up but is not continuously monitored." Only Ollama uses `'started'` because it actively polls a local server. Do not flag `'unknown'` status as incorrect for cloud provider extension connections in `extensions/*/src/*.ts`.
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 2001
File: packages/main/src/plugin/provider-registry.ts:1966-1971
Timestamp: 2026-05-29T13:43:13.292Z
Learning: In `openkaiden/kaiden`, `getInferenceSDK`, `getInferenceConnectionType`, and `getInferenceConnectionEndpoint` in `packages/main/src/plugin/provider-registry.ts` intentionally resolve inference connections by `name` (not `id`), because downstream consumers (InferenceParameters, chat history DB) persist `connectionName` rather than `connectionId`. Migrating these helpers to id-based lookup is deferred to the storage format redesign epic (`#1917`). Do not flag these name-based lookups as issues in reviews until that epic is addressed.
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1603
File: packages/renderer/src/lib/guided-setup/guided-setup-steps.ts:35-45
Timestamp: 2026-05-04T17:36:14.229Z
Learning: In openkaiden/kaiden, `OnboardingModelSelection` (packages/renderer/src/lib/guided-setup/guided-setup-steps.ts) intentionally omits `connectionName` because the Claude extension sets `connectionName` to the raw API key value. Persisting `connectionName` would write the raw API key to settings.json, which is a security risk. The `providerId + label` pair is sufficient for the CLI `--model` flag and for workspace creation; `connectionName` must not be re-added to this interface unless a safe (non-secret) identifier can be substituted.
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1671
File: extensions/vertex-ai/src/vertex-ai.ts:363-387
Timestamp: 2026-05-05T17:45:03.618Z
Learning: In the openkaiden/kaiden repo, no extension currently uses the optional `logger` or `CancellationToken` parameters in their `InferenceProviderConnectionFactory.create` factory method implementations (including Vertex AI, Gemini, Claude, Mistral, OpenAI-compatible extensions). Do not flag the omission of these parameters as an issue in factory method signatures for this codebase.
Learnt from: CR
Repo: openkaiden/kaiden PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-12T16:35:51.592Z
Learning: Applies to extensions/*/src/**/*.{ts,tsx} : Register inference, container, and Kubernetes providers through the `ProviderRegistry` via extension APIs
Learnt from: bmahabirbu
Repo: openkaiden/kaiden PR: 1379
File: packages/renderer/src/stores/model-catalog.ts:26-28
Timestamp: 2026-04-17T20:27:11.322Z
Learning: In `packages/renderer/src/stores/model-catalog.ts`, provider IDs used as the first component of `modelKey(providerId, label)` are always simple colon-free slug strings (e.g. `gemini`, `claude`, `openai`, `openshiftai`). The `:` separator in `modelKey` does not risk key collisions because provider IDs are guaranteed never to contain a colon by convention. Do not flag this as a collision risk in future reviews.
Learnt from: jeffmaury
Repo: openkaiden/kaiden PR: 1524
File: extensions/openshift-ai/src/openshiftai.ts:233-235
Timestamp: 2026-04-30T12:38:39.371Z
Learning: In `extensions/openshift-ai/src/openshiftai.ts` (openkaiden/kaiden), `getInferenceServices()` intentionally swallows all API/auth/network exceptions and returns `[]`. This is by design: a cluster may restrict visibility of certain resources via RBAC, so an empty result is a valid unified signal for both "no inference services exist" and "no inference services are visible to this user." Do not flag this error-swallowing as hiding failures — the caller (`registerInferenceProviderConnection`) then throws a meaningful error when `connectionInfos.length === 0`.
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 2018
File: extensions/claude/src/manager/claude-inference-manager.ts:91-94
Timestamp: 2026-06-01T13:20:13.832Z
Learning: In the Claude extension (`extensions/claude/src/manager/claude-inference-manager.ts`), `removeConnection` intentionally filters stored `StoredConnection[]` records by token rather than by connection ID. This is safe because the `connections.has(tokenHash)` guard in `registerInferenceProviderConnection` throws if a duplicate token is already registered, making it impossible for two `StoredConnection` records with the same token to coexist in secret storage. Token-based removal is therefore equivalent to ID-based removal in practice. Do not flag this as a bug in future reviews.
📚 Learning: 2026-06-01T13:20:13.832Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 2018
File: extensions/claude/src/manager/claude-inference-manager.ts:91-94
Timestamp: 2026-06-01T13:20:13.832Z
Learning: In the Claude extension (`extensions/claude/src/manager/claude-inference-manager.ts`), `removeConnection` intentionally filters stored `StoredConnection[]` records by token rather than by connection ID. This is safe because the `connections.has(tokenHash)` guard in `registerInferenceProviderConnection` throws if a duplicate token is already registered, making it impossible for two `StoredConnection` records with the same token to coexist in secret storage. Token-based removal is therefore equivalent to ID-based removal in practice. Do not flag this as a bug in future reviews.
Applied to files:
extensions/mistral/src/manager/mistral-inference-manager.spec.tsextensions/cursor/src/manager/cursor-inference-manager.tsextensions/cursor/src/manager/cursor-inference-manager.spec.tspackages/main/src/plugin/provider-registry.tsextensions/openai-compatible/src/openAI.tsextensions/mistral/src/manager/mistral-inference-manager.tsextensions/openai-compatible/src/openAI.spec.ts
📚 Learning: 2026-05-13T09:26:42.026Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1858
File: packages/renderer/src/lib/secret-vault/columns/SecretVaultActions.spec.ts:70-79
Timestamp: 2026-05-13T09:26:42.026Z
Learning: In `packages/renderer/src/lib/secret-vault/columns/SecretVaultActions.spec.ts` (and the parallel `SecretVaultDetails.spec.ts`), when testing that a cancel response from a mocked `window.showMessageBox` (resolving with `{ response: 1 }`) prevents `window.removeSecret` from being called, it is correct and sufficient to assert directly after `await fireEvent.click(removeButton)` without wrapping in `waitFor`. `fireEvent.click` is wrapped in `act()` by Testing Library, which flushes the microtask queue including the resolved mock promise, so the confirmation logic completes before the assertion runs. Do not flag this pattern as potentially flaky or suggest adding `waitFor` around the negative assertion.
Applied to files:
extensions/mistral/src/manager/mistral-inference-manager.spec.tsextensions/cursor/src/manager/cursor-inference-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
📚 Learning: 2026-05-05T17:30:24.999Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1671
File: extensions/vertex-ai/src/vertex-ai.ts:272-302
Timestamp: 2026-05-05T17:30:24.999Z
Learning: In the `openkaiden/kaiden` repository, cloud provider extensions (Gemini, Claude, Mistral, OpenAI-compatible, Vertex AI) use `ProviderConnectionStatus = 'unknown'` when registering inference provider connections. This means "connection was set up but is not continuously monitored." Only Ollama uses `'started'` because it actively polls a local server. Do not flag `'unknown'` status as incorrect for cloud provider extension connections in `extensions/*/src/*.ts`.
Applied to files:
extensions/mistral/src/manager/mistral-inference-manager.spec.tsextensions/cursor/src/manager/cursor-inference-manager.tspackages/main/src/plugin/provider-registry.tsextensions/mistral/src/manager/mistral-inference-manager.ts
📚 Learning: 2026-04-20T14:30:15.867Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1396
File: packages/renderer/src/lib/guided-setup/CodingAgentStep.spec.ts:30-45
Timestamp: 2026-04-20T14:30:15.867Z
Learning: In `packages/renderer/src/lib/guided-setup/CodingAgentStep.spec.ts`, the `stubOllama` helper intentionally only re-stubs `fetch` and relies on `beforeEach`'s `stubRamalama(false)` call to keep `getProviderInfos` in place. This is a deliberate standard Vitest `beforeEach` + per-test override pattern. Do not flag the implicit dependency between `stubOllama` and the `beforeEach` ramalama stub as a robustness issue.
Applied to files:
extensions/mistral/src/manager/mistral-inference-manager.spec.tsextensions/cursor/src/manager/cursor-inference-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tsextensions/openai-compatible/src/openAI.spec.ts
📚 Learning: 2026-05-05T17:44:50.991Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1671
File: extensions/vertex-ai/src/vertex-ai.ts:363-387
Timestamp: 2026-05-05T17:44:50.991Z
Learning: In this repo (openkaiden/kaiden), do not raise a code review issue when an extension’s `InferenceProviderConnectionFactory.create` factory method implementation omits (or does not use) the optional `logger` and/or `CancellationToken` parameters in its method signature/implementation. Current extensions (e.g., Vertex AI, Gemini, Claude, Mistral, OpenAI-compatible) follow this pattern, so reviewers should treat it as acceptable for `extensions/*` TypeScript source files.
Applied to files:
extensions/mistral/src/manager/mistral-inference-manager.spec.tsextensions/cursor/src/manager/cursor-inference-manager.tsextensions/cursor/src/manager/cursor-inference-manager.spec.tsextensions/openai-compatible/src/openAI.tsextensions/mistral/src/manager/mistral-inference-manager.tsextensions/openai-compatible/src/openAI.spec.ts
📚 Learning: 2026-05-12T10:01:14.248Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1810
File: extensions/kdn/src/kdn-extension.ts:43-46
Timestamp: 2026-05-12T10:01:14.248Z
Learning: In this repo’s extension code, when logging from binary discovery/resolution logic (e.g., choosing/validating custom paths, extension storage locations, or bundled resource paths), it’s intentional to include full filesystem paths in `console.log`/`console.warn` (such as in `extensions/**/src/*-extension.ts`). During review, do not flag these specific full-path messages as a privacy/security issue as long as they are clearly part of the binary resolution steps. If full-path logging appears outside binary discovery/resolution, review/flag it as usual.
Applied to files:
extensions/mistral/src/manager/mistral-inference-manager.spec.tsextensions/cursor/src/manager/cursor-inference-manager.tsextensions/cursor/src/manager/cursor-inference-manager.spec.tsextensions/openai-compatible/src/openAI.tsextensions/mistral/src/manager/mistral-inference-manager.tsextensions/openai-compatible/src/openAI.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:
extensions/mistral/src/manager/mistral-inference-manager.spec.tsextensions/cursor/src/manager/cursor-inference-manager.tsextensions/cursor/src/manager/cursor-inference-manager.spec.tspackages/main/src/plugin/provider-registry.tsextensions/openai-compatible/src/openAI.tsextensions/mistral/src/manager/mistral-inference-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tsextensions/openai-compatible/src/openAI.spec.ts
📚 Learning: 2026-05-29T13:43:13.292Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 2001
File: packages/main/src/plugin/provider-registry.ts:1966-1971
Timestamp: 2026-05-29T13:43:13.292Z
Learning: In `openkaiden/kaiden`, `getInferenceSDK`, `getInferenceConnectionType`, and `getInferenceConnectionEndpoint` in `packages/main/src/plugin/provider-registry.ts` intentionally resolve inference connections by `name` (not `id`), because downstream consumers (InferenceParameters, chat history DB) persist `connectionName` rather than `connectionId`. Migrating these helpers to id-based lookup is deferred to the storage format redesign epic (`#1917`). Do not flag these name-based lookups as issues in reviews until that epic is addressed.
Applied to files:
extensions/cursor/src/manager/cursor-inference-manager.tsextensions/cursor/src/manager/cursor-inference-manager.spec.tspackages/main/src/plugin/provider-registry.tsextensions/openai-compatible/src/openAI.tsextensions/mistral/src/manager/mistral-inference-manager.ts
📚 Learning: 2026-04-17T20:26:32.946Z
Learnt from: bmahabirbu
Repo: openkaiden/kaiden PR: 1379
File: packages/renderer/src/lib/models/models-utils.ts:11-20
Timestamp: 2026-04-17T20:26:32.946Z
Learning: In `packages/renderer/src/lib/models/models-utils.ts` (openkaiden/kaiden), `InferenceConnectionSummary.connectionType` is intentionally optional. It is only `undefined` for the single synthetic `'not-configured'` entry (emitted when a provider has `inferenceProviderConnectionCreation` but no active `inferenceConnections`). All consumers guard with optional chaining. A discriminated union was considered but deferred as unnecessary complexity for v1, since the invariant (`connectionType` is defined iff `status !== 'not-configured'`) is self-evident from the `status` field. Do not flag this as a type-safety issue.
Applied to files:
extensions/cursor/src/manager/cursor-inference-manager.tsextensions/cursor/src/manager/cursor-inference-manager.spec.tspackages/main/src/plugin/provider-registry.tsextensions/openai-compatible/src/openAI.tsextensions/mistral/src/manager/mistral-inference-manager.ts
📚 Learning: 2026-04-13T15:59:29.742Z
Learnt from: jeffmaury
Repo: openkaiden/kaiden PR: 1295
File: packages/extension-api/src/extension-api.d.ts:651-651
Timestamp: 2026-04-13T15:59:29.742Z
Learning: In the openkaiden/kaiden repository, the project relies on TypeScript's static type checking (not runtime validation) to enforce type correctness for `InferenceProviderConnection.type` (`InferenceProviderConnectionType`). Runtime normalization/validation guards for this field are not needed or desired.
Applied to files:
extensions/cursor/src/manager/cursor-inference-manager.tsextensions/cursor/src/manager/cursor-inference-manager.spec.tspackages/main/src/plugin/provider-registry.ts
📚 Learning: 2026-05-04T17:36:14.229Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1603
File: packages/renderer/src/lib/guided-setup/guided-setup-steps.ts:35-45
Timestamp: 2026-05-04T17:36:14.229Z
Learning: In openkaiden/kaiden, `OnboardingModelSelection` (packages/renderer/src/lib/guided-setup/guided-setup-steps.ts) intentionally omits `connectionName` because the Claude extension sets `connectionName` to the raw API key value. Persisting `connectionName` would write the raw API key to settings.json, which is a security risk. The `providerId + label` pair is sufficient for the CLI `--model` flag and for workspace creation; `connectionName` must not be re-added to this interface unless a safe (non-secret) identifier can be substituted.
Applied to files:
extensions/cursor/src/manager/cursor-inference-manager.tspackages/main/src/plugin/provider-registry.tsextensions/openai-compatible/src/openAI.tsextensions/mistral/src/manager/mistral-inference-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
📚 Learning: 2026-06-01T15:06:34.983Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 2025
File: extensions/vertex-ai/src/vertex-ai.ts:166-170
Timestamp: 2026-06-01T15:06:34.983Z
Learning: In openkaiden/kaiden, the Vertex AI extension's `removeConnection` (previously `removeConnectionConfig`) intentionally removes stored entries by config hash rather than by persisted `id`. This is safe because `factory()` (line ~423) has an in-memory duplicate guard (`this.connections.has(this.getConfigHash(config))`) that rejects same-config calls before any `saveConnection` write occurs, making the hash-collision/race scenario impossible. Switching to ID-based removal is considered a future design improvement, not a correctness fix, and was explicitly scoped out of PR `#2025` (issue `#1942`).
Applied to files:
extensions/cursor/src/manager/cursor-inference-manager.spec.tsextensions/openai-compatible/src/openAI.tsextensions/mistral/src/manager/mistral-inference-manager.tsextensions/openai-compatible/src/openAI.spec.ts
📚 Learning: 2026-04-20T14:31:10.155Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1396
File: packages/renderer/src/lib/guided-setup/CodingAgentStep.spec.ts:114-123
Timestamp: 2026-04-20T14:31:10.155Z
Learning: In `packages/renderer/src/lib/guided-setup/CodingAgentStep.spec.ts`, the team's convention is that a test's name plus its stub setup lines serve as sufficient documentation of intent; inline comments restating what the stubs do are considered redundant and are intentionally omitted. Do not flag the absence of such comments as a clarity issue.
Applied to files:
extensions/cursor/src/manager/cursor-inference-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tsextensions/openai-compatible/src/openAI.spec.ts
📚 Learning: 2026-05-12T16:35:51.592Z
Learnt from: CR
Repo: openkaiden/kaiden PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-12T16:35:51.592Z
Learning: Applies to extensions/*/src/**/*.{ts,tsx} : Register inference, container, and Kubernetes providers through the `ProviderRegistry` via extension APIs
Applied to files:
packages/main/src/plugin/provider-registry.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
📚 Learning: 2026-04-30T12:38:39.371Z
Learnt from: jeffmaury
Repo: openkaiden/kaiden PR: 1524
File: extensions/openshift-ai/src/openshiftai.ts:233-235
Timestamp: 2026-04-30T12:38:39.371Z
Learning: In `extensions/openshift-ai/src/openshiftai.ts` (openkaiden/kaiden), `getInferenceServices()` intentionally swallows all API/auth/network exceptions and returns `[]`. This is by design: a cluster may restrict visibility of certain resources via RBAC, so an empty result is a valid unified signal for both "no inference services exist" and "no inference services are visible to this user." Do not flag this error-swallowing as hiding failures — the caller (`registerInferenceProviderConnection`) then throws a meaningful error when `connectionInfos.length === 0`.
Applied to files:
packages/main/src/plugin/provider-registry.tsextensions/openai-compatible/src/openAI.ts
📚 Learning: 2026-04-17T20:27:11.322Z
Learnt from: bmahabirbu
Repo: openkaiden/kaiden PR: 1379
File: packages/renderer/src/stores/model-catalog.ts:26-28
Timestamp: 2026-04-17T20:27:11.322Z
Learning: In `packages/renderer/src/stores/model-catalog.ts`, provider IDs used as the first component of `modelKey(providerId, label)` are always simple colon-free slug strings (e.g. `gemini`, `claude`, `openai`, `openshiftai`). The `:` separator in `modelKey` does not risk key collisions because provider IDs are guaranteed never to contain a colon by convention. Do not flag this as a collision risk in future reviews.
Applied to files:
packages/main/src/plugin/provider-registry.ts
📚 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-registry.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
📚 Learning: 2026-05-05T17:30:20.418Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1671
File: extensions/vertex-ai/src/vertex-ai.ts:272-302
Timestamp: 2026-05-05T17:30:20.418Z
Learning: In the openkaiden/kaiden repo, for cloud inference provider extension code under `extensions/*/src/*.ts`, treat `ProviderConnectionStatus = 'unknown'` as a valid/expected value when registering provider connections (e.g., Gemini/Claude/Mistral/OpenAI-compatible/Vertex AI). `'unknown'` indicates the connection was set up but is not continuously monitored—so do not flag it as incorrect. Only Ollama is expected to use `'started'` because it actively polls a local server.
Applied to files:
extensions/openai-compatible/src/openAI.tsextensions/openai-compatible/src/openAI.spec.ts
📚 Learning: 2026-05-03T09:13:45.692Z
Learnt from: bmahabirbu
Repo: openkaiden/kaiden PR: 1517
File: packages/main/src/plugin/kdn-cli/kdn-cli.ts:167-177
Timestamp: 2026-05-03T09:13:45.692Z
Learning: In the openkaiden/kaiden repository, URL-based (remote) MCPs that require API tokens use the secret manager (`SafeStorageRegistry`) for credential storage. Command/package-spawned MCPs (e.g., uvx, npx) generally do not require API keys, so their env vars (e.g., UV_SYSTEM_CERTS) are safely persisted in plaintext in workspace.json. Flagging plaintext env persistence in `writeWorkspaceConfig` (packages/main/src/plugin/kdn-cli/kdn-cli.ts) as a security issue for command-based MCPs is not currently applicable, but may be worth a follow-up if command-based MCPs ever need secrets.
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
📚 Learning: 2026-04-30T12:45:43.072Z
Learnt from: fbricon
Repo: openkaiden/kaiden PR: 1509
File: packages/api/src/agent-workspace-info.ts:53-53
Timestamp: 2026-04-30T12:45:43.072Z
Learning: In `packages/api/src/agent-workspace-info.ts`, the `model` field on `AgentWorkspaceCreateOptions` is intentionally typed as `model?: string` (not a narrowed template-literal type). The current CLI-side filtering (only forwarding `ollama::` / `ramalama::` prefixes) in `kdn-cli.ts` is a temporary measure until broader agent support lands (tracked in openkaiden/kdn#354). Do not suggest narrowing this type to a union or template-literal type in future reviews.
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
📚 Learning: 2026-05-05T03:38:00.098Z
Learnt from: bmahabirbu
Repo: openkaiden/kaiden PR: 1517
File: packages/main/src/plugin/kdn-cli/kdn-cli.ts:181-189
Timestamp: 2026-05-05T03:38:00.098Z
Learning: In the openkaiden/kaiden repository, the explicit design decision for `writeWorkspaceConfig` (packages/main/src/plugin/kdn-cli/kdn-cli.ts) is: only non-secret runtime config flags (e.g. UV_SYSTEM_CERTS) are written as plaintext env vars to workspace.json for command/package-spawned MCPs. When a spawned MCP requires a secret env var, the planned approach is to route it through `kdn secret create` and write a secret reference (not the raw value) into workspace.json. This will be implemented in a separate follow-up PR.
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
📚 Learning: 2026-05-12T16:35:51.592Z
Learnt from: CR
Repo: openkaiden/kaiden PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-12T16:35:51.592Z
Learning: Applies to packages/main/src/**/*.{ts,tsx} : Use `ipcHandle()` to expose handlers in the main process with naming convention `<registry-name>:<action>` (e.g., `container-provider-registry:listContainers`)
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
📚 Learning: 2026-05-12T16:35:51.592Z
Learnt from: CR
Repo: openkaiden/kaiden PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-12T16:35:51.592Z
Learning: Applies to **/*.spec.{ts,tsx,js,jsx} : Use `vi.resetAllMocks()` in `beforeEach` hooks instead of `vi.clearAllMocks()` for resetting mocks between tests
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
📚 Learning: 2026-05-12T16:35:51.592Z
Learnt from: CR
Repo: openkaiden/kaiden PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-12T16:35:51.592Z
Learning: Applies to **/*.spec.{ts,tsx,js,jsx} : When an auto-mocked function or class method needs a real implementation, use `vi.mocked(...)` with the prototype pattern for class methods: `vi.mocked(MyClass.prototype.myMethod).mockImplementation(...)`
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
📚 Learning: 2026-05-12T16:35:51.592Z
Learnt from: CR
Repo: openkaiden/kaiden PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-12T16:35:51.592Z
Learning: Applies to **/*.spec.{ts,tsx,js,jsx} : Use `vi.mock(import('...'))` for auto-mocking modules in unit tests; avoid manual mock factories when possible
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
📚 Learning: 2026-04-10T09:59:30.223Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1274
File: packages/renderer/src/lib/agent-workspaces/AgentWorkspaceDetails.svelte:35-49
Timestamp: 2026-04-10T09:59:30.223Z
Learning: In `AgentWorkspaceDetails.svelte` (openkaiden/kaiden), the `$effect` that calls `window.getAgentWorkspaceConfiguration(workspaceId)` intentionally does NOT reset `configuration = {}` at the start of the effect. The `current` flag in the cleanup function already prevents stale callbacks from a prior `workspaceId` from overwriting state. Resetting eagerly would cause the summary tab to blank on every navigation, which is considered a worse UX tradeoff given that the IPC call is local and fast. Do not flag the absence of an eager reset as a bug.
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
📚 Learning: 2026-04-18T22:59:01.535Z
Learnt from: bmahabirbu
Repo: openkaiden/kaiden PR: 1362
File: packages/renderer/src/lib/secret-vault/SecretVaultList.svelte:71-73
Timestamp: 2026-04-18T22:59:01.535Z
Learning: In `packages/renderer/src/lib/secret-vault/SecretVaultList.svelte`, the `addSecret()` function is an intentional no-op stub (with a TODO comment). The secret-creation flow and its navigation route do not exist yet (`createNavigationFlowsEntry()` exists but is not registered in `navigation-registry.ts#init()`). The CTA button will be wired up in a follow-up PR. Do not flag `addSecret()` as a dead/missing implementation during reviews of this file.
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
📚 Learning: 2026-04-30T12:44:46.782Z
Learnt from: fbricon
Repo: openkaiden/kaiden PR: 1509
File: packages/main/src/plugin/kdn-cli/kdn-cli.ts:103-112
Timestamp: 2026-04-30T12:44:46.782Z
Learning: In `packages/main/src/plugin/kdn-cli/kdn-cli.ts` (`createWorkspace`), silently skipping `--model` with a `console.warn` when `options.model` does not start with `ollama::` or `ramalama::` is intentional and temporary. This is because only OpenCode currently supports that scheme for local runtimes; broader support is tracked in openkaiden/kdn#354. Do not flag this as a silent-failure bug or suggest throwing an error in future reviews.
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
📚 Learning: 2026-04-22T02:58:56.754Z
Learnt from: bmahabirbu
Repo: openkaiden/kaiden PR: 1379
File: packages/renderer/src/lib/models/ModelsCatalogEmptyScreen.spec.ts:32-38
Timestamp: 2026-04-22T02:58:56.754Z
Learning: In `openkaiden/kaiden`, test files under `packages/renderer/src/lib/models/**/*.spec.ts` (and sibling spec files) intentionally use exact user-facing copy strings in `screen.getByText(...)` assertions rather than regex matchers or `data-testid`. This is a deliberate Testing Library convention: if the wording changes, the test should fail to prompt an intentional update. Do not flag exact-string `getByText` assertions as brittle in these test files.
Applied to files:
extensions/openai-compatible/src/openAI.spec.ts
📚 Learning: 2026-05-12T16:35:51.592Z
Learnt from: CR
Repo: openkaiden/kaiden PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-12T16:35:51.592Z
Learning: Unit tests are co-located with source files using *.spec.ts naming convention; E2E tests are located in tests/playwright/src/; test configuration is in vitest.config.js at root
Applied to files:
extensions/openai-compatible/src/openAI.spec.ts
🔇 Additional comments (9)
packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts (1)
81-82: LGTM!packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts (2)
22-22: LGTM!Also applies to: 33-33, 122-122, 127-127, 135-144, 156-157, 168-168
737-917: LGTM!extensions/cursor/src/manager/cursor-inference-manager.ts (1)
118-119: LGTM!Also applies to: 126-128
extensions/cursor/src/manager/cursor-inference-manager.spec.ts (1)
243-244: LGTM!Also applies to: 273-274, 290-292
extensions/mistral/src/manager/mistral-inference-manager.ts (1)
109-110: LGTM!extensions/mistral/src/manager/mistral-inference-manager.spec.ts (1)
278-279: LGTM!Also applies to: 308-309, 325-327
extensions/openai-compatible/src/openAI.ts (1)
142-144: LGTM!Also applies to: 151-153
extensions/openai-compatible/src/openAI.spec.ts (1)
250-251: LGTM!Also applies to: 282-282, 285-286, 304-306
Fixes openkaiden#2066 Signed-off-by: Jeff MAURY <jmaury@redhat.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts (1)
177-228: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd coverage for the newly added openshell IPC routes.
agent-workspace:listOpenshellSandboxesandagent-workspace:deleteOpenshellSandboxwere added in production code, but this suite doesn’t assert handler registration or success/failure task behavior for those flows yet.🤖 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/agent-workspace/agent-workspace-manager.spec.ts` around lines 177 - 228, Add tests in the 'init' suite to assert ipcHandle was registered for the new routes 'agent-workspace:listOpenshellSandboxes' and 'agent-workspace:deleteOpenshellSandbox' (use ipcHandle toHaveBeenCalledWith for each). Also add tests that retrieve the actual handler functions from ipcHandle.mock.calls (or similar) and invoke them to exercise both success and failure paths, asserting the expected task/service interactions (e.g., that the corresponding workspace manager methods or taskRunner are called, and that errors are handled/rejected correctly). Ensure you reference the existing ipcHandle mock and any task/service mocks used elsewhere in this spec so the new tests mirror the style of existing handler invocation tests.extensions/mistral/src/manager/mistral-inference-manager.spec.ts (1)
273-280: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a delete-path scope assertion to prevent false positives.
This test validates update keys but not the
getConfiguration(...)scope used during clear, so scope/key mismatches can pass unnoticed. Assert the expected scope in this scenario.Suggested test hardening
test('calling delete should remove the connection from storage, clear configuration, and dispose', async () => { await mDelete(); + expect(configuration.getConfiguration).toHaveBeenCalledWith( + undefined, + expect.objectContaining({ id: 'fake-uuid-1' }), + ); + expect(CONFIG_UPDATE_MOCK).toHaveBeenCalledWith('mistral.connection._type', undefined); expect(CONFIG_UPDATE_MOCK).toHaveBeenCalledWith('mistral.connection.token', undefined);🤖 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 `@extensions/mistral/src/manager/mistral-inference-manager.spec.ts` around lines 273 - 280, The test should also assert the configuration scope used during clearing to avoid false positives: after calling mDelete(), add an assertion that the getConfiguration mock was called with the exact provider/delete-path scope used by the code (e.g. verify getConfiguration(...) was invoked with the same scope/key the manager uses) and/or assert CONFIG_UPDATE_MOCK was called with the expected scope argument when clearing 'mistral.connection._type' and 'mistral.connection.token' so mismatched scopes cannot pass; locate this by checking the mocks used in the test (mDelete, SECRET_STORAGE_MOCK.delete, CONFIG_UPDATE_MOCK, getConfiguration) and add the scope equality assertions accordingly.
♻️ Duplicate comments (1)
extensions/mistral/src/manager/mistral-inference-manager.ts (1)
117-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear-path config scope/key mismatch leaves stale connection settings.
Line [117] scopes config to
'mistral.connection', but Lines [118]-[119] update fully-qualified keys (mistral.connection.*). This mismatch targets a different path than the set-path and can fail to clear the actual stored_type/tokenentries.Suggested fix
- const config = configuration.getConfiguration('mistral.connection', connection); + const config = configuration.getConfiguration(undefined, connection); await config.update('mistral.connection._type', undefined); await config.update('mistral.connection.token', undefined);🤖 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 `@extensions/mistral/src/manager/mistral-inference-manager.ts` around lines 117 - 119, The config is being fetched with configuration.getConfiguration('mistral.connection', connection) but then cleared using fully-qualified keys ('mistral.connection._type' / 'mistral.connection.token'), so the updates target a different scope and leave stale values; fix by updating the keys relative to the fetched scope (call config.update('_type', undefined) and config.update('token', undefined)) or alternatively fetch the top-level scope with configuration.getConfiguration('mistral', connection) and keep the fully-qualified keys — adjust the code around configuration.getConfiguration and the two config.update calls accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts`:
- Around line 408-419: The code is using "workspace" wording while deleting an
openshell sandbox; update user-visible text and events to refer to "sandbox"
instead: change the task title in taskManager.createTask from `Deleting
workspace ${name}` to `Deleting sandbox ${name}`, update the task.error string
from `Failed to delete workspace: ${detail}` to `Failed to delete sandbox:
${detail}`, and if the sent event name should be sandbox-specific, change
apiSender.send('agent-workspace-update') to a sandbox equivalent (e.g.,
'agent-sandbox-update') so all messages and events reflect the sandbox operation
(affecting taskManager.createTask, openshellCli.deleteSandbox flow,
apiSender.send, and task.error).
- Around line 326-330: The current flow waits for this.list() and aborts
deletion if listing fails; change it so the deletion proceeds even if list()
throws: create the task via this.taskManager.createTask(...), set task.state =
'running', then attempt to call this.removeWorkspaces([id]) regardless of the
result of this.list(); use the list result only to set workspaceName when
available (fall back to id if list fails or returns no match), and ensure any
errors from list() are caught and do not prevent calling removeWorkspaces(),
while still handling and logging errors from removeWorkspaces() to update the
task state.
---
Outside diff comments:
In `@extensions/mistral/src/manager/mistral-inference-manager.spec.ts`:
- Around line 273-280: The test should also assert the configuration scope used
during clearing to avoid false positives: after calling mDelete(), add an
assertion that the getConfiguration mock was called with the exact
provider/delete-path scope used by the code (e.g. verify getConfiguration(...)
was invoked with the same scope/key the manager uses) and/or assert
CONFIG_UPDATE_MOCK was called with the expected scope argument when clearing
'mistral.connection._type' and 'mistral.connection.token' so mismatched scopes
cannot pass; locate this by checking the mocks used in the test (mDelete,
SECRET_STORAGE_MOCK.delete, CONFIG_UPDATE_MOCK, getConfiguration) and add the
scope equality assertions accordingly.
In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts`:
- Around line 177-228: Add tests in the 'init' suite to assert ipcHandle was
registered for the new routes 'agent-workspace:listOpenshellSandboxes' and
'agent-workspace:deleteOpenshellSandbox' (use ipcHandle toHaveBeenCalledWith for
each). Also add tests that retrieve the actual handler functions from
ipcHandle.mock.calls (or similar) and invoke them to exercise both success and
failure paths, asserting the expected task/service interactions (e.g., that the
corresponding workspace manager methods or taskRunner are called, and that
errors are handled/rejected correctly). Ensure you reference the existing
ipcHandle mock and any task/service mocks used elsewhere in this spec so the new
tests mirror the style of existing handler invocation tests.
---
Duplicate comments:
In `@extensions/mistral/src/manager/mistral-inference-manager.ts`:
- Around line 117-119: The config is being fetched with
configuration.getConfiguration('mistral.connection', connection) but then
cleared using fully-qualified keys ('mistral.connection._type' /
'mistral.connection.token'), so the updates target a different scope and leave
stale values; fix by updating the keys relative to the fetched scope (call
config.update('_type', undefined) and config.update('token', undefined)) or
alternatively fetch the top-level scope with
configuration.getConfiguration('mistral', connection) and keep the
fully-qualified keys — adjust the code around configuration.getConfiguration and
the two config.update calls accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6fbf18a5-6f79-4311-ba7b-2ccaa24406d8
📒 Files selected for processing (9)
extensions/cursor/src/manager/cursor-inference-manager.spec.tsextensions/cursor/src/manager/cursor-inference-manager.tsextensions/mistral/src/manager/mistral-inference-manager.spec.tsextensions/mistral/src/manager/mistral-inference-manager.tsextensions/openai-compatible/src/openAI.spec.tsextensions/openai-compatible/src/openAI.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/provider-registry.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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:
extensions/cursor/src/manager/cursor-inference-manager.spec.tspackages/main/src/plugin/provider-registry.tsextensions/openai-compatible/src/openAI.tsextensions/mistral/src/manager/mistral-inference-manager.spec.tsextensions/cursor/src/manager/cursor-inference-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tsextensions/mistral/src/manager/mistral-inference-manager.tsextensions/openai-compatible/src/openAI.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.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:
extensions/cursor/src/manager/cursor-inference-manager.spec.tsextensions/mistral/src/manager/mistral-inference-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tsextensions/openai-compatible/src/openAI.spec.ts
extensions/*/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Register inference, container, and Kubernetes providers through the
ProviderRegistryvia extension APIs
Files:
extensions/cursor/src/manager/cursor-inference-manager.spec.tsextensions/openai-compatible/src/openAI.tsextensions/mistral/src/manager/mistral-inference-manager.spec.tsextensions/cursor/src/manager/cursor-inference-manager.tsextensions/mistral/src/manager/mistral-inference-manager.tsextensions/openai-compatible/src/openAI.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-registry.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.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-registry.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
🧠 Learnings (6)
📚 Learning: 2026-05-05T17:44:50.991Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1671
File: extensions/vertex-ai/src/vertex-ai.ts:363-387
Timestamp: 2026-05-05T17:44:50.991Z
Learning: In this repo (openkaiden/kaiden), do not raise a code review issue when an extension’s `InferenceProviderConnectionFactory.create` factory method implementation omits (or does not use) the optional `logger` and/or `CancellationToken` parameters in its method signature/implementation. Current extensions (e.g., Vertex AI, Gemini, Claude, Mistral, OpenAI-compatible) follow this pattern, so reviewers should treat it as acceptable for `extensions/*` TypeScript source files.
Applied to files:
extensions/cursor/src/manager/cursor-inference-manager.spec.tsextensions/openai-compatible/src/openAI.tsextensions/mistral/src/manager/mistral-inference-manager.spec.tsextensions/cursor/src/manager/cursor-inference-manager.tsextensions/mistral/src/manager/mistral-inference-manager.tsextensions/openai-compatible/src/openAI.spec.ts
📚 Learning: 2026-05-12T10:01:14.248Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1810
File: extensions/kdn/src/kdn-extension.ts:43-46
Timestamp: 2026-05-12T10:01:14.248Z
Learning: In this repo’s extension code, when logging from binary discovery/resolution logic (e.g., choosing/validating custom paths, extension storage locations, or bundled resource paths), it’s intentional to include full filesystem paths in `console.log`/`console.warn` (such as in `extensions/**/src/*-extension.ts`). During review, do not flag these specific full-path messages as a privacy/security issue as long as they are clearly part of the binary resolution steps. If full-path logging appears outside binary discovery/resolution, review/flag it as usual.
Applied to files:
extensions/cursor/src/manager/cursor-inference-manager.spec.tsextensions/openai-compatible/src/openAI.tsextensions/mistral/src/manager/mistral-inference-manager.spec.tsextensions/cursor/src/manager/cursor-inference-manager.tsextensions/mistral/src/manager/mistral-inference-manager.tsextensions/openai-compatible/src/openAI.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:
extensions/cursor/src/manager/cursor-inference-manager.spec.tspackages/main/src/plugin/provider-registry.tsextensions/openai-compatible/src/openAI.tsextensions/mistral/src/manager/mistral-inference-manager.spec.tsextensions/cursor/src/manager/cursor-inference-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tsextensions/mistral/src/manager/mistral-inference-manager.tsextensions/openai-compatible/src/openAI.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
📚 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-registry.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
📚 Learning: 2026-05-05T17:30:20.418Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1671
File: extensions/vertex-ai/src/vertex-ai.ts:272-302
Timestamp: 2026-05-05T17:30:20.418Z
Learning: In the openkaiden/kaiden repo, for cloud inference provider extension code under `extensions/*/src/*.ts`, treat `ProviderConnectionStatus = 'unknown'` as a valid/expected value when registering provider connections (e.g., Gemini/Claude/Mistral/OpenAI-compatible/Vertex AI). `'unknown'` indicates the connection was set up but is not continuously monitored—so do not flag it as incorrect. Only Ollama is expected to use `'started'` because it actively polls a local server.
Applied to files:
extensions/openai-compatible/src/openAI.tsextensions/openai-compatible/src/openAI.spec.ts
📚 Learning: 2026-06-08T14:22:17.045Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 2093
File: extensions/claude/src/manager/claude-inference-manager.ts:109-111
Timestamp: 2026-06-08T14:22:17.045Z
Learning: In provider inference manager extensions (e.g., Mistral/Claude), the `configuration.getConfiguration(...)` argument is intentionally asymmetric:
- For `setConnectionConfiguration`, call `configuration.getConfiguration(undefined, connection)` and rely on the `connection` parameter to apply namespace scoping.
- For `clearConnectionConfiguration`, call `configuration.getConfiguration('<provider>.connection', connection)` (using the explicit provider namespace string) so the persisted keys are cleared correctly.
In future reviews, do not flag this `undefined` vs explicit namespace difference as a bug; it is required for correct write-vs-clear behavior.
Applied to files:
extensions/cursor/src/manager/cursor-inference-manager.tsextensions/mistral/src/manager/mistral-inference-manager.ts
🔇 Additional comments (8)
packages/main/src/plugin/provider-registry.ts (1)
2263-2282: LGTM!packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts (1)
179-229: LGTM!Also applies to: 530-539
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts (1)
741-922: LGTM!extensions/cursor/src/manager/cursor-inference-manager.ts (1)
118-119: LGTM!Also applies to: 126-128
extensions/cursor/src/manager/cursor-inference-manager.spec.ts (1)
243-244: LGTM!Also applies to: 273-274, 290-292
extensions/mistral/src/manager/mistral-inference-manager.ts (1)
109-110: LGTM!extensions/openai-compatible/src/openAI.ts (1)
142-144: LGTM!Also applies to: 151-153
extensions/openai-compatible/src/openAI.spec.ts (1)
250-251: LGTM!Also applies to: 282-282, 285-286, 304-306
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts (1)
177-228: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd coverage for the newly added openshell IPC routes.
agent-workspace:listOpenshellSandboxesandagent-workspace:deleteOpenshellSandboxwere added in production code, but this suite doesn’t assert handler registration or success/failure task behavior for those flows yet.🤖 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/agent-workspace/agent-workspace-manager.spec.ts` around lines 177 - 228, Add tests in the 'init' suite to assert ipcHandle was registered for the new routes 'agent-workspace:listOpenshellSandboxes' and 'agent-workspace:deleteOpenshellSandbox' (use ipcHandle toHaveBeenCalledWith for each). Also add tests that retrieve the actual handler functions from ipcHandle.mock.calls (or similar) and invoke them to exercise both success and failure paths, asserting the expected task/service interactions (e.g., that the corresponding workspace manager methods or taskRunner are called, and that errors are handled/rejected correctly). Ensure you reference the existing ipcHandle mock and any task/service mocks used elsewhere in this spec so the new tests mirror the style of existing handler invocation tests.extensions/mistral/src/manager/mistral-inference-manager.spec.ts (1)
273-280: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a delete-path scope assertion to prevent false positives.
This test validates update keys but not the
getConfiguration(...)scope used during clear, so scope/key mismatches can pass unnoticed. Assert the expected scope in this scenario.Suggested test hardening
test('calling delete should remove the connection from storage, clear configuration, and dispose', async () => { await mDelete(); + expect(configuration.getConfiguration).toHaveBeenCalledWith( + undefined, + expect.objectContaining({ id: 'fake-uuid-1' }), + ); + expect(CONFIG_UPDATE_MOCK).toHaveBeenCalledWith('mistral.connection._type', undefined); expect(CONFIG_UPDATE_MOCK).toHaveBeenCalledWith('mistral.connection.token', undefined);🤖 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 `@extensions/mistral/src/manager/mistral-inference-manager.spec.ts` around lines 273 - 280, The test should also assert the configuration scope used during clearing to avoid false positives: after calling mDelete(), add an assertion that the getConfiguration mock was called with the exact provider/delete-path scope used by the code (e.g. verify getConfiguration(...) was invoked with the same scope/key the manager uses) and/or assert CONFIG_UPDATE_MOCK was called with the expected scope argument when clearing 'mistral.connection._type' and 'mistral.connection.token' so mismatched scopes cannot pass; locate this by checking the mocks used in the test (mDelete, SECRET_STORAGE_MOCK.delete, CONFIG_UPDATE_MOCK, getConfiguration) and add the scope equality assertions accordingly.
♻️ Duplicate comments (1)
extensions/mistral/src/manager/mistral-inference-manager.ts (1)
117-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear-path config scope/key mismatch leaves stale connection settings.
Line [117] scopes config to
'mistral.connection', but Lines [118]-[119] update fully-qualified keys (mistral.connection.*). This mismatch targets a different path than the set-path and can fail to clear the actual stored_type/tokenentries.Suggested fix
- const config = configuration.getConfiguration('mistral.connection', connection); + const config = configuration.getConfiguration(undefined, connection); await config.update('mistral.connection._type', undefined); await config.update('mistral.connection.token', undefined);🤖 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 `@extensions/mistral/src/manager/mistral-inference-manager.ts` around lines 117 - 119, The config is being fetched with configuration.getConfiguration('mistral.connection', connection) but then cleared using fully-qualified keys ('mistral.connection._type' / 'mistral.connection.token'), so the updates target a different scope and leave stale values; fix by updating the keys relative to the fetched scope (call config.update('_type', undefined) and config.update('token', undefined)) or alternatively fetch the top-level scope with configuration.getConfiguration('mistral', connection) and keep the fully-qualified keys — adjust the code around configuration.getConfiguration and the two config.update calls accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts`:
- Around line 408-419: The code is using "workspace" wording while deleting an
openshell sandbox; update user-visible text and events to refer to "sandbox"
instead: change the task title in taskManager.createTask from `Deleting
workspace ${name}` to `Deleting sandbox ${name}`, update the task.error string
from `Failed to delete workspace: ${detail}` to `Failed to delete sandbox:
${detail}`, and if the sent event name should be sandbox-specific, change
apiSender.send('agent-workspace-update') to a sandbox equivalent (e.g.,
'agent-sandbox-update') so all messages and events reflect the sandbox operation
(affecting taskManager.createTask, openshellCli.deleteSandbox flow,
apiSender.send, and task.error).
- Around line 326-330: The current flow waits for this.list() and aborts
deletion if listing fails; change it so the deletion proceeds even if list()
throws: create the task via this.taskManager.createTask(...), set task.state =
'running', then attempt to call this.removeWorkspaces([id]) regardless of the
result of this.list(); use the list result only to set workspaceName when
available (fall back to id if list fails or returns no match), and ensure any
errors from list() are caught and do not prevent calling removeWorkspaces(),
while still handling and logging errors from removeWorkspaces() to update the
task state.
---
Outside diff comments:
In `@extensions/mistral/src/manager/mistral-inference-manager.spec.ts`:
- Around line 273-280: The test should also assert the configuration scope used
during clearing to avoid false positives: after calling mDelete(), add an
assertion that the getConfiguration mock was called with the exact
provider/delete-path scope used by the code (e.g. verify getConfiguration(...)
was invoked with the same scope/key the manager uses) and/or assert
CONFIG_UPDATE_MOCK was called with the expected scope argument when clearing
'mistral.connection._type' and 'mistral.connection.token' so mismatched scopes
cannot pass; locate this by checking the mocks used in the test (mDelete,
SECRET_STORAGE_MOCK.delete, CONFIG_UPDATE_MOCK, getConfiguration) and add the
scope equality assertions accordingly.
In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts`:
- Around line 177-228: Add tests in the 'init' suite to assert ipcHandle was
registered for the new routes 'agent-workspace:listOpenshellSandboxes' and
'agent-workspace:deleteOpenshellSandbox' (use ipcHandle toHaveBeenCalledWith for
each). Also add tests that retrieve the actual handler functions from
ipcHandle.mock.calls (or similar) and invoke them to exercise both success and
failure paths, asserting the expected task/service interactions (e.g., that the
corresponding workspace manager methods or taskRunner are called, and that
errors are handled/rejected correctly). Ensure you reference the existing
ipcHandle mock and any task/service mocks used elsewhere in this spec so the new
tests mirror the style of existing handler invocation tests.
---
Duplicate comments:
In `@extensions/mistral/src/manager/mistral-inference-manager.ts`:
- Around line 117-119: The config is being fetched with
configuration.getConfiguration('mistral.connection', connection) but then
cleared using fully-qualified keys ('mistral.connection._type' /
'mistral.connection.token'), so the updates target a different scope and leave
stale values; fix by updating the keys relative to the fetched scope (call
config.update('_type', undefined) and config.update('token', undefined)) or
alternatively fetch the top-level scope with
configuration.getConfiguration('mistral', connection) and keep the
fully-qualified keys — adjust the code around configuration.getConfiguration and
the two config.update calls accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6fbf18a5-6f79-4311-ba7b-2ccaa24406d8
📒 Files selected for processing (9)
extensions/cursor/src/manager/cursor-inference-manager.spec.tsextensions/cursor/src/manager/cursor-inference-manager.tsextensions/mistral/src/manager/mistral-inference-manager.spec.tsextensions/mistral/src/manager/mistral-inference-manager.tsextensions/openai-compatible/src/openAI.spec.tsextensions/openai-compatible/src/openAI.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/provider-registry.ts
📜 Review details
🔇 Additional comments (8)
packages/main/src/plugin/provider-registry.ts (1)
2263-2282: LGTM!packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts (1)
179-229: LGTM!Also applies to: 530-539
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts (1)
741-922: LGTM!extensions/cursor/src/manager/cursor-inference-manager.ts (1)
118-119: LGTM!Also applies to: 126-128
extensions/cursor/src/manager/cursor-inference-manager.spec.ts (1)
243-244: LGTM!Also applies to: 273-274, 290-292
extensions/mistral/src/manager/mistral-inference-manager.ts (1)
109-110: LGTM!extensions/openai-compatible/src/openAI.ts (1)
142-144: LGTM!Also applies to: 151-153
extensions/openai-compatible/src/openAI.spec.ts (1)
250-251: LGTM!Also applies to: 282-282, 285-286, 304-306
🛑 Comments failed to post (2)
packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts (2)
326-330:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid blocking workspace deletion on pre-delete listing.
Line 326 introduces a hard dependency on
list()beforeremoveWorkspaces(). If listing fails, deletion is never attempted and no task status is emitted.Suggested fix
async remove(id: string): Promise<AgentWorkspaceId> { - const workspaces = await this.list(); - const workspace = workspaces.find(ws => ws.id === id); - const workspaceName = workspace?.name ?? id; + let workspaceName = id; + try { + const workspace = (await this.list()).find(ws => ws.id === id); + workspaceName = workspace?.name ?? id; + } catch { + // best-effort lookup only; do not block delete + } const task = this.taskManager.createTask({ title: `Deleting workspace "${workspaceName}"` });📝 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.let workspaceName = id; try { const workspace = (await this.list()).find(ws => ws.id === id); workspaceName = workspace?.name ?? id; } catch { // best-effort lookup only; do not block delete } const task = this.taskManager.createTask({ title: `Deleting workspace "${workspaceName}"` }); task.state = 'running';🤖 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/agent-workspace/agent-workspace-manager.ts` around lines 326 - 330, The current flow waits for this.list() and aborts deletion if listing fails; change it so the deletion proceeds even if list() throws: create the task via this.taskManager.createTask(...), set task.state = 'running', then attempt to call this.removeWorkspaces([id]) regardless of the result of this.list(); use the list result only to set workspaceName when available (fall back to id if list fails or returns no match), and ensure any errors from list() are caught and do not prevent calling removeWorkspaces(), while still handling and logging errors from removeWorkspaces() to update the task state.
408-419:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse sandbox-specific task/error text in sandbox deletion flow.
Lines 408 and 418 still refer to “workspace” while this method deletes an openshell sandbox, which is user-visible and misleading.
Suggested fix
- const task = this.taskManager.createTask({ title: `Deleting workspace ${name}` }); + const task = this.taskManager.createTask({ title: `Deleting sandbox "${name}"` }); ... - task.error = `Failed to delete workspace: ${detail}`; + task.error = `Failed to delete sandbox: ${detail}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const task = this.taskManager.createTask({ title: `Deleting sandbox "${name}"` }); task.state = 'running'; task.status = 'in-progress'; try { await this.openshellCli.deleteSandbox(name); this.apiSender.send('agent-workspace-update'); task.status = 'success'; } catch (err: unknown) { const detail = err instanceof Error ? err.message : String(err); task.status = 'failure'; task.error = `Failed to delete sandbox: ${detail}`; throw new Error(detail);🤖 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/agent-workspace/agent-workspace-manager.ts` around lines 408 - 419, The code is using "workspace" wording while deleting an openshell sandbox; update user-visible text and events to refer to "sandbox" instead: change the task title in taskManager.createTask from `Deleting workspace ${name}` to `Deleting sandbox ${name}`, update the task.error string from `Failed to delete workspace: ${detail}` to `Failed to delete sandbox: ${detail}`, and if the sent event name should be sandbox-specific, change apiSender.send('agent-workspace-update') to a sandbox equivalent (e.g., 'agent-sandbox-update') so all messages and events reflect the sandbox operation (affecting taskManager.createTask, openshellCli.deleteSandbox flow, apiSender.send, and task.error).
benoitf
approved these changes
Jun 10, 2026
Contributor
Author
|
@coderabbitai resolve |
Contributor
✅ Action performedComments resolved and changes approved. |
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 #2066