Skip to content

fix: update OpenShell provider creation - #2137

Merged
jeffmaury merged 1 commit into
openkaiden:mainfrom
jeffmaury:GH-2132
Jun 11, 2026
Merged

jeffmaury merged 1 commit into
openkaiden:mainfrom
jeffmaury:GH-2132

Conversation

@jeffmaury

Copy link
Copy Markdown
Contributor

Fixes #2132

@jeffmaury
jeffmaury requested a review from a team as a code owner June 11, 2026 12:25
@jeffmaury
jeffmaury requested review from benoitf and fbricon and removed request for a team June 11, 2026 12:25
@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6163a059-9947-463a-85dc-2ad169a96f0b

📥 Commits

Reviewing files that changed from the base of the PR and between 3a9e28c and 34ac0c9.

📒 Files selected for processing (8)
  • extensions/claude/package.json
  • extensions/claude/src/manager/claude-inference-manager.spec.ts
  • extensions/claude/src/manager/claude-inference-manager.ts
  • extensions/openai-compatible/package.json
  • extensions/openai-compatible/src/openAI.spec.ts
  • extensions/openai-compatible/src/openAI.ts
  • packages/main/src/plugin/openshell-cli/openshell-cli.spec.ts
  • packages/main/src/plugin/openshell-cli/openshell-cli.ts
📜 Recent 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). (5)
  • GitHub Check: macOS
  • GitHub Check: linter, formatters
  • GitHub Check: unit tests / macos-15
  • GitHub Check: Windows
  • GitHub Check: unit tests / ubuntu-24.04
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{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/claude/src/manager/claude-inference-manager.ts
  • extensions/openai-compatible/src/openAI.ts
  • packages/main/src/plugin/openshell-cli/openshell-cli.ts
  • extensions/openai-compatible/src/openAI.spec.ts
  • extensions/claude/src/manager/claude-inference-manager.spec.ts
  • packages/main/src/plugin/openshell-cli/openshell-cli.spec.ts
extensions/*/src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Register inference, container, and Kubernetes providers through the ProviderRegistry via extension APIs

Files:

  • extensions/claude/src/manager/claude-inference-manager.ts
  • extensions/openai-compatible/src/openAI.ts
  • extensions/openai-compatible/src/openAI.spec.ts
  • extensions/claude/src/manager/claude-inference-manager.spec.ts
extensions/*/package.json

📄 CodeRabbit inference engine (AGENTS.md)

extensions/*/package.json: Extensions must declare engines.kaiden version compatibility in their package.json
Extension package.json must have main field pointing to ./dist/extension.js
Configuration properties for API keys, tokens, or secrets must use "format": "password" in the configuration definition to ensure input masking in the UI

Files:

  • extensions/openai-compatible/package.json
  • extensions/claude/package.json
packages/main/src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

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)
Use apiSender.send() to send events from main process to renderer for real-time updates
Long-running operations should use TaskManager.createTask() with title and action configuration

Files:

  • packages/main/src/plugin/openshell-cli/openshell-cli.ts
  • packages/main/src/plugin/openshell-cli/openshell-cli.spec.ts
packages/{main,renderer,preload}/src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Container operations must include engineId parameter to identify the container engine

Files:

  • packages/main/src/plugin/openshell-cli/openshell-cli.ts
  • packages/main/src/plugin/openshell-cli/openshell-cli.spec.ts
**/*.spec.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.spec.{ts,tsx,js,jsx}: Use test() instead of it() for test cases in Vitest unit tests
Use vi.mock(import('...')) for auto-mocking modules in unit tests; avoid manual mock factories when possible
Use vi.resetAllMocks() in beforeEach hooks instead of vi.clearAllMocks() for resetting mocks between tests
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(...)

Files:

  • extensions/openai-compatible/src/openAI.spec.ts
  • extensions/claude/src/manager/claude-inference-manager.spec.ts
  • packages/main/src/plugin/openshell-cli/openshell-cli.spec.ts
🧠 Learnings (8)
📚 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/claude/src/manager/claude-inference-manager.ts
  • extensions/openai-compatible/src/openAI.ts
  • extensions/openai-compatible/src/openAI.spec.ts
  • extensions/claude/src/manager/claude-inference-manager.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/claude/src/manager/claude-inference-manager.ts
  • extensions/openai-compatible/src/openAI.ts
  • extensions/openai-compatible/src/openAI.spec.ts
  • extensions/claude/src/manager/claude-inference-manager.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/claude/src/manager/claude-inference-manager.ts
  • extensions/openai-compatible/src/openAI.ts
  • packages/main/src/plugin/openshell-cli/openshell-cli.ts
  • extensions/openai-compatible/src/openAI.spec.ts
  • extensions/claude/src/manager/claude-inference-manager.spec.ts
  • packages/main/src/plugin/openshell-cli/openshell-cli.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/claude/src/manager/claude-inference-manager.ts
📚 Learning: 2026-06-10T09:07:26.260Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 2093
File: extensions/claude/src/manager/claude-inference-manager.ts:174-179
Timestamp: 2026-06-10T09:07:26.260Z
Learning: When reviewing inference provider extensions in this repo, do not flag the `lifecycle.delete` handler registered inside `registerInferenceProviderConnection` for missing `try/catch/finally` around sequential cleanup calls (e.g., `clearConnectionConfiguration`, dispose, map-delete, `removeConnection`). This lack of wrapping is an intentional, established cross-extension convention; adding it to only one extension would create inconsistency.

Applied to files:

  • extensions/claude/src/manager/claude-inference-manager.ts
  • extensions/claude/src/manager/claude-inference-manager.spec.ts
📚 Learning: 2026-05-06T11:29:33.170Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1671
File: extensions/vertex-ai/package.json:9-11
Timestamp: 2026-05-06T11:29:33.170Z
Learning: In the openkaiden/kaiden repo, all built-in extensions under extensions/ should specify engines with kaiden: "^0.0.1" in package.json. Do not flag each extension individually; enforce a repo-wide alignment in a single PR. During reviews, verify that every extensions/*/package.json has "engines": { "kaiden": "^0.0.1" }. If a file deviates, surface the discrepancy as a single repo-wide task rather than per-file.

Applied to files:

  • extensions/openai-compatible/package.json
  • extensions/claude/package.json
📚 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.ts
  • extensions/openai-compatible/src/openAI.spec.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/openshell-cli/openshell-cli.ts
  • packages/main/src/plugin/openshell-cli/openshell-cli.spec.ts
🪛 ast-grep (0.43.0)
packages/main/src/plugin/openshell-cli/openshell-cli.ts

[warning] 303-303: Avoid command injection
Context: this.exec.exec(cliPath, args, options?.env ? { env: options.env } : undefined)
Note: Security best practice.

(command-injection-typescript)

🪛 OpenGrep (1.22.0)
packages/main/src/plugin/openshell-cli/openshell-cli.ts

[ERROR] 304-304: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.

(coderabbit.command-injection.exec-js)

🔇 Additional comments (10)
extensions/claude/package.json (1)

40-46: LGTM!

extensions/claude/src/manager/claude-inference-manager.ts (2)

119-119: LGTM!


128-128: LGTM!

extensions/claude/src/manager/claude-inference-manager.spec.ts (1)

362-362: LGTM!

Also applies to: 392-395, 412-413

extensions/openai-compatible/package.json (1)

34-40: LGTM!

extensions/openai-compatible/src/openAI.ts (1)

145-145: LGTM!

Also applies to: 154-154

extensions/openai-compatible/src/openAI.spec.ts (1)

252-252: LGTM!

Also applies to: 287-290, 309-310

packages/main/src/plugin/openshell-cli/openshell-cli.spec.ts (1)

675-683: ⚡ Quick win

Security improvement: Credentials via environment variables.

The test updates correctly reflect the new pattern where credentials are passed to the OpenShell CLI via environment variables rather than command-line arguments. This prevents credential exposure in process listings and logs.

Also applies to: 696-716, 730-751

packages/main/src/plugin/openshell-cli/openshell-cli.ts (2)

284-294: ⚡ Quick win

Security improvement: Credentials passed via environment variables.

The refactoring correctly moves credential values from CLI arguments to environment variables. This prevents credential exposure in process listings, shell history, and command-line logs while maintaining the same functional behavior.


299-304: Static analysis false positives: Command injection warnings are incorrect.

The static analysis tools flagged potential command injection, but the implementation is already using the recommended secure pattern:

  1. Array-based arguments: args is passed as an array (line 304), not a shell string
  2. Structured environment: env is passed as an object in options, not concatenated into command strings
  3. Controlled command path: cliPath comes from the CLI tool registry

The code follows security best practices for process execution. No changes needed.

Source: Linters/SAST tools


📝 Walkthrough

Walkthrough

Per-connection secret keys for Claude and OpenAI were renamed to claude.connection.ANTHROPIC_API_KEY and openai.connection.OPENAI_API_KEY; OpenShell CLI now passes provider secrets via a process env instead of embedding values in CLI args, with corresponding test updates.

Changes

Configuration and Credential Handling Updates

Layer / File(s) Summary
Claude extension configuration key rename
extensions/claude/package.json, extensions/claude/src/manager/claude-inference-manager.ts, extensions/claude/src/manager/claude-inference-manager.spec.ts
Configuration schema and per-connection credential storage/clearing updated from claude.connection.token to claude.connection.ANTHROPIC_API_KEY. Tests updated to assert the new key is set during registration/restoration and cleared on deletion.
OpenAI-compatible extension configuration key rename
extensions/openai-compatible/package.json, extensions/openai-compatible/src/openAI.ts, extensions/openai-compatible/src/openAI.spec.ts
Configuration property renamed from openai.connection.token to openai.connection.OPENAI_API_KEY; implementation writes/clears the new key and tests updated to expect it during registration, restoration, and deletion.
OpenShell CLI credential environment variable support
packages/main/src/plugin/openshell-cli/openshell-cli.ts, packages/main/src/plugin/openshell-cli/openshell-cli.spec.ts
createProvider now builds an env map from options.credentials and passes it to runCli (with redact: true); runCli forwards env to exec.exec. Tests updated to assert the new exec.exec call shape and that credential values are supplied via the env argument.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • openkaiden/kaiden#2093: Both PRs modify Claude’s per-connection workspace configuration key handling.
  • openkaiden/kaiden#2049: Related changes to OpenAI-compatible connection credential/config handling and tests.
  • openkaiden/kaiden#2073: Related provider connection configuration and secret-field changes affecting secret creation/reading.

Suggested reviewers

  • fbricon
  • benoitf
🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The PR title partially covers the changes but is overly broad; it mentions updating OpenShell provider creation without specifying the main change of renaming configuration properties for Claude and OpenAI inference providers. Consider a more specific title such as: 'fix: rename inference provider connection property keys to match OpenShell provider properties' to better reflect the primary changes.
Description check ❓ Inconclusive The PR description consists only of 'Fixes #2132' with no substantive details about the implementation changes, making it vague despite being technically related to the changeset. Expand the description to explain that configuration keys were renamed from generic names (token) to provider-specific names (ANTHROPIC_API_KEY, OPENAI_API_KEY) and that CLI credential passing was refactored.
Out of Scope Changes check ❓ Inconclusive The openshell-cli.ts credential handling refactoring (using env instead of CLI flags) goes beyond the stated issue objective of renaming properties but appears necessary for proper provider creation implementation. Clarify whether the credential handling refactoring in openshell-cli.ts is required by issue #2132 or if it should be separated into a distinct PR for better changeability tracking.
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR successfully addresses the primary objective of issue #2132 by renaming inference connection properties (claude.connection.token→ANTHROPIC_API_KEY, openai.connection.token→OPENAI_API_KEY) to align with OpenShell provider requirements, with corresponding test updates.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@jeffmaury
jeffmaury requested review from feloy and removed request for fbricon June 11, 2026 12:25
@codecov

codecov Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@jeffmaury
jeffmaury requested a review from fbricon June 11, 2026 13:33
Fixes openkaiden#2132

Signed-off-by: Jeff MAURY <jmaury@redhat.com>
@jeffmaury
jeffmaury enabled auto-merge (squash) June 11, 2026 14:28
@jeffmaury
jeffmaury merged commit 0fde461 into openkaiden:main Jun 11, 2026
14 checks passed
@jeffmaury
jeffmaury deleted the GH-2132 branch June 11, 2026 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adjust inference connection properties to OpenShell properties

3 participants