feat(openshell): add OpenshellCli wrapper for sandbox and gateway operations - #2020
Conversation
|
Warning Review limit reached
More reviews will be available in 12 minutes and 41 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR adds comprehensive OpenShell CLI and gateway management to the plugin system. It introduces generalized binary discovery in the extension manager, a typed OpenshellCli wrapper for sandbox and gateway operations, an OpenshellGateway class for managing the local gateway server lifecycle with auto-start and health checking, and integrates both into the plugin dependency container. ChangesOpenShell CLI and Gateway Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
benoitf
left a comment
benoitf
left a comment
There was a problem hiding this comment.
I think we're missing the part that manage the lifecycle of the gateway (and how to start the gateway)
I guess it's a separate file as it's not for the CLI but without the gateway part, it won't be possible to start /list etc stuff
gastoner
changed the title
jeffmaury
left a comment
jeffmaury
left a comment
There was a problem hiding this comment.
After discussion with @benoitf, I don't think we need to handle gateways in the extension.
On the backend component, we need to:
- discover existing gateways
- for each gateway list the sandboxes
- if we have found no gateways, we should start one
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/index.ts`:
- Around line 3751-3752: Replace the direct call to openshellGateway.init() with
a TaskManager-managed task: obtain the OpenshellGateway instance
(openshellGateway from container.get<OpenshellGateway>(OpenshellGateway)) and
call TaskManager.createTask() with a descriptive title (e.g., "Initialize
openshell gateway") and an action that invokes openshellGateway.init(); ensure
the created task is started/registered so progress and failures are surfaced
consistently and any errors are handled or logged by the task framework rather
than using console.error directly.
In `@packages/main/src/plugin/openshell-cli/openshell-cli.spec.ts`:
- Around line 525-538: Add a new test case to openshell-cli.spec.ts that calls
openshellCli.listSandboxesPerGateway() with two gateways where one gateway's
exec call rejects and the other resolves: set up vi.mocked(exec.exec) to first
return JSON for two gateways (e.g., gw-1 and gw-2), then mockResolvedValueOnce
for the successful gateway's sandboxes and mockRejectedValueOnce for the failing
gateway (or vice versa) so the function sees a mixed partial failure; assert the
returned array has entries for both gateways and that the failing gateway's
sandboxes is [] while the successful gateway returns its sandboxes, ensuring
listSandboxesPerGateway continues after one gateway error.
- Around line 23-35: Add a new spec that verifies mixed partial-failure behavior
of OpenshellCli.listSandboxesPerGateway: mock Exec.exec (the exec instance used
in the spec) to reject for one gateway command and resolve with valid sandbox
output for the other gateways, then call new OpenshellCli(OPENSHELL_CLI_PATH,
exec).listSandboxesPerGateway(...) and assert the returned map contains
sandboxes for the successful gateways while the failing gateway is either absent
or has an empty list; use the existing Exec mock import and the exec variable
from the test file to set per-call behaviors so the implementation continues
after the per-gateway error.
In `@packages/main/src/plugin/openshell-cli/openshell-cli.ts`:
- Around line 210-221: The addGateway method currently allows both
options.remote and options.local to be set and will pass both flags to runCli;
update addGateway to check for this mutually exclusive state and throw a clear
in-process error if both are present (e.g., validate before constructing args in
addGateway and return/throw). Also tighten the GatewayAddOptions type to a
discriminated union (e.g., one variant with remote: string and no local, and
another with local: true and no remote) so TypeScript callers cannot construct
both simultaneously; keep runCli usage intact but only build args after the
runtime guard and with the narrowed GatewayAddOptions shape.
- Around line 259-280: The current logging in runCli and execCLI exposes raw CLI
arguments (user-controlled flags like labels/command/endpoint/remote); change
both to stop logging args.join(' ') — instead extract and log only the
subcommand (args[0] or fullArgs[0]) alongside cliPath, or build a redactedArgs
by replacing flag values with placeholders (e.g., map flags like --label,
--endpoint, --remote to "--flag=<redacted>") before logging; update
console.log/console.error in runCli and execCLI to use the safe value and ensure
any options object is not printed, while keeping error handling (extractCliError
and thrown Error) unchanged.
In `@packages/main/src/plugin/openshell-cli/openshell-gateway.spec.ts`:
- Around line 269-287: Add a test that exercises the forced-stop escalation path
by starting the gateway (gateway.start) with spawn mocked to return a child
(createMockChildProcess) that never emits 'exit', call gateway.stop(), use fake
timers (vi.useFakeTimers) and advance timers past the gateway's kill timeout
(the escalation delay used inside gateway.stop), then await the stop promise and
assert proc.kill was called with 'SIGKILL' (and optionally that 'SIGTERM' was
called first); reference spawn, createMockChildProcess, gateway.start,
gateway.stop, proc.emit, and proc.kill to locate and implement the test.
- Around line 234-248: The test currently asserts exec.exec was called with the
`status` args but doesn't lock call order; update the assertion to assert the
first call explicitly using toHaveBeenNthCalledWith(1, CLI_BINARY, ['status',
'--gateway-endpoint', 'http://127.0.0.1:17670', '--gateway-insecure']) (and
similarly assert the subsequent `gateway add` call as nth call) so that
gateway.start()'s sequencing (waitForReady() calling exec.exec for status before
registerWithCli() calling exec.exec for add) is protected against regressions;
locate assertions around gateway.start() and replace the plain
toHaveBeenCalledWith checks with toHaveBeenNthCalledWith referencing exec.exec
and CLI_BINARY.
In `@packages/main/src/plugin/openshell-cli/openshell-gateway.ts`:
- Around line 128-153: The start() sequence can leave a stray child if
waitForReady() or registerWithCli() throws; wrap the awaits after spawn (the
calls to waitForReady() and registerWithCli()) in a try/catch and on any error
synchronously tear down the spawned process referenced by this.#gatewayProcess
(call kill()/remove listeners/cleanup, set this.#gatewayProcess = undefined)
before rethrowing or returning the error; ensure the same cleanup also runs if
the process failed to start in the 'error' handler or if exit happens during
startup.
- Around line 182-183: isRunning() incorrectly checks
this.#gatewayProcess.exitCode === undefined; change the running check to test
for exitCode === null (child process exitCode is null while running) or
otherwise use exitCode !== null to detect stopped; additionally, update start()
to wrap the waitForReady() call in a try/catch and call stop() if waitForReady()
throws (then rethrow the error) so a failed startup does not leave the spawned
gateway process running; reference methods: isRunning(), start(), stop(),
waitForReady(), and the private field `#gatewayProcess.exitCode` when making these
fixes.
🪄 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: 41a63342-a7be-4bc2-83d7-01370346761f
📒 Files selected for processing (7)
extensions/openshell/src/manager/openshell-cli-manager.tspackages/api/src/openshell-gateway-info.tspackages/main/src/plugin/index.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/openshell-cli/openshell-gateway.spec.tspackages/main/src/plugin/openshell-cli/openshell-gateway.ts
📜 Review details
🧰 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:
packages/api/src/openshell-gateway-info.tspackages/main/src/plugin/index.tspackages/main/src/plugin/openshell-cli/openshell-gateway.spec.tspackages/main/src/plugin/openshell-cli/openshell-gateway.tspackages/main/src/plugin/openshell-cli/openshell-cli.tsextensions/openshell/src/manager/openshell-cli-manager.tspackages/main/src/plugin/openshell-cli/openshell-cli.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/index.tspackages/main/src/plugin/openshell-cli/openshell-gateway.spec.tspackages/main/src/plugin/openshell-cli/openshell-gateway.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.ts
packages/main/src/plugin/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
All major services must be registered as singletons in the Inversify DI container during initialization in the PluginSystem
Files:
packages/main/src/plugin/index.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/index.tspackages/main/src/plugin/openshell-cli/openshell-gateway.spec.tspackages/main/src/plugin/openshell-cli/openshell-gateway.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/openshell-cli/openshell-cli.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/openshell-cli/openshell-gateway.spec.tspackages/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
ProviderRegistryvia extension APIs
Files:
extensions/openshell/src/manager/openshell-cli-manager.ts
🧠 Learnings (4)
📚 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/api/src/openshell-gateway-info.tspackages/main/src/plugin/index.tspackages/main/src/plugin/openshell-cli/openshell-gateway.spec.tspackages/main/src/plugin/openshell-cli/openshell-gateway.tspackages/main/src/plugin/openshell-cli/openshell-cli.tsextensions/openshell/src/manager/openshell-cli-manager.tspackages/main/src/plugin/openshell-cli/openshell-cli.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/index.tspackages/main/src/plugin/openshell-cli/openshell-gateway.spec.tspackages/main/src/plugin/openshell-cli/openshell-gateway.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/openshell-cli/openshell-cli.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/openshell/src/manager/openshell-cli-manager.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/openshell/src/manager/openshell-cli-manager.ts
🪛 Biome (2.4.16)
packages/main/src/plugin/openshell-cli/openshell-gateway.spec.ts
[error] 27-27: Do not shadow the global "Proxy" property.
(lint/suspicious/noShadowRestrictedNames)
packages/main/src/plugin/openshell-cli/openshell-cli.spec.ts
[error] 23-23: Do not shadow the global "Proxy" property.
(lint/suspicious/noShadowRestrictedNames)
🪛 OpenGrep (1.22.0)
packages/main/src/plugin/openshell-cli/openshell-gateway.ts
[ERROR] 90-90: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 212-212: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 229-229: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
packages/main/src/plugin/openshell-cli/openshell-cli.ts
[ERROR] 90-90: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 248-248: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 263-263: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 275-275: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
extensions/openshell/src/manager/openshell-cli-manager.ts
[ERROR] 158-158: 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 (5)
packages/api/src/openshell-gateway-info.ts (1)
21-54: LGTM!Also applies to: 65-74
extensions/openshell/src/manager/openshell-cli-manager.ts (1)
20-20: LGTM!Also applies to: 28-32, 39-44, 46-174
packages/main/src/plugin/openshell-cli/openshell-cli.ts (1)
62-205: LGTM!Also applies to: 224-255
packages/main/src/plugin/openshell-cli/openshell-gateway.spec.ts (1)
27-27: ⚡ Quick winAlias the imported
Proxytype to avoid shadowing the globalProxy.
packages/main/src/plugin/openshell-cli/openshell-gateway.spec.tsimportsimport type { Proxy } ...and usesnew Exec({} as Proxy)(line 59). This can trip Biome’s restricted-name shadowing forProxy; rename the type locally (e.g.,import type { Proxy as ProxyType } ...) and update the cast.packages/main/src/plugin/openshell-cli/openshell-gateway.ts (1)
225-229: Delegate gateway registration throughOpenshellCli.This lifecycle manager is still reconstructing
openshell gateway add ...directly even thoughOpenshellCliis already injected for gateway operations. Keep command construction in the wrapper to avoid drift.
Introduces a low-level CLI wrapper that maps Kaiden sandbox operations to openshell commands (create, list, start, stop, delete, connect) and registers it as a singleton in the plugin DI container. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
|
Rebased |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
4 participants
Introduces a low-level CLI wrapper that maps Kaiden sandbox/gateway operations to openshell commands (create, list, start, stop, delete, connect) and registers it as a singleton in the plugin DI container.
Closes #1923