fix(openshell): reuse existing provider with the same name#2157
Open
feloy wants to merge 1 commit into
Open
Conversation
When creating a workspace, if an OpenShell provider with the same name already exists, reuse it instead of failing or creating a duplicate. Type comparison against the list output is intentionally omitted: the CLI normalises the type string on storage (e.g. "claude" becomes "claude-code"), so the value in options.type and the value returned by "openshell provider list" can differ even for the same provider. Fixes openkaiden#2154 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
feloy
requested review from
fbricon and
jeffmaury
and removed request for
a team
Contributor
📝 WalkthroughWalkthroughOpenshellSecretAdapter.createSecret now checks for existing OpenShell providers by name before creating a new one. If a matching provider is found, the method returns early without calling createProvider. Unit tests verify provider reuse behavior, and integration tests are updated to mock the listProviders call appropriately. ChangesOpenShell Provider Reuse
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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.
Actionable comments posted: 1
🤖 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/secret-manager/openshell-secret-adapter.ts`:
- Around line 53-59: The current provider reuse logic in
openshell-secret-adapter (the block calling this.openshellCli.listProviders()
and returning { name: options.name }) reuses by name only; change it to compare
the existing provider's configuration/credentials/settings with the incoming
options (e.g., compare provider.settings or provider.config against
options.settings/options.config) and only reuse if they match exactly; if a
provider with the same name has different settings, do not return that
name—generate a new unique provider name (append a suffix or increment) and
proceed to create a new provider instead; update the create/ensure flow in the
adapter method that calls listProviders() and any tests that assumed name-only
reuse to assert the new behavior.
🪄 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: c606553a-6494-48fc-a003-cf4bf1b8dc16
📒 Files selected for processing (3)
packages/main/src/plugin/secret-manager/openshell-secret-adapter.spec.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.tspackages/main/src/plugin/secret-manager/secret-manager.spec.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: typecheck
- GitHub Check: linter, formatters
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: unit tests / windows-2022
- GitHub Check: macOS
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: unit tests / macos-15
- GitHub Check: Windows
- GitHub Check: Linux
🧰 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/secret-manager/openshell-secret-adapter.tspackages/main/src/plugin/secret-manager/secret-manager.spec.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.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/secret-manager/openshell-secret-adapter.tspackages/main/src/plugin/secret-manager/secret-manager.spec.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.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/secret-manager/openshell-secret-adapter.tspackages/main/src/plugin/secret-manager/secret-manager.spec.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.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/secret-manager/secret-manager.spec.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.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/secret-manager/openshell-secret-adapter.tspackages/main/src/plugin/secret-manager/secret-manager.spec.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.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/secret-manager/openshell-secret-adapter.tspackages/main/src/plugin/secret-manager/secret-manager.spec.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.spec.ts
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
Author
|
@coderabbitai help |
Contributor
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
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
1 participant
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.
When creating a workspace, if an OpenShell provider with the same name already exists, reuse it instead of failing or creating a duplicate.
Type comparison against the list output is intentionally omitted: the CLI normalises the type string on storage (e.g. "claude" becomes "claude-code"), so the value in options.type and the value returned by "openshell provider list" can differ even for the same provider.
Fixes #2154