feat(openshell-cli): integrate OpenShell TypeScript SDK with CLI fall#2496
feat(openshell-cli): integrate OpenShell TypeScript SDK with CLI fall#2496MarsKubeX wants to merge 1 commit into
Conversation
…back Add SDK-first strategy for sandbox operations (list, create, delete) with automatic CLI fallback when the SDK is unavailable or fails. Update tests to account for the resolveGatewayEndpoint call path introduced by the SDK integration and mock getSdkClient in timer-driven auto-refresh tests to avoid real gRPC connections under fake timers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Marcel Bertagnini <mbertagn@redhat.com>
📝 WalkthroughWalkthroughChangesOpenShell sandbox lifecycle operations now prefer the local OpenShell SDK, mapping SDK responses into existing models and falling back to CLI commands when SDK calls fail or CLI-only options are required. Endpoint health checks similarly try SDK health checks before CLI status checks. OpenShell SDK integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OpenshellCli
participant OpenShellClient
participant CLI
OpenshellCli->>OpenshellCli: Resolve gateway endpoint
OpenshellCli->>OpenShellClient: Perform sandbox operation
OpenShellClient-->>OpenshellCli: Return SDK result
OpenshellCli->>CLI: Fallback when SDK fails or CLI-only options exist
CLI-->>OpenshellCli: Return CLI result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)package.jsonTraceback (most recent call last): 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/openshell-cli/openshell-cli.ts`:
- Around line 206-213: Update the requiresCli calculation to evaluate each
CLI-only option independently instead of chaining heterogeneous values with ??.
Ensure empty arrays or strings do not suppress later non-empty options, so any
meaningful command, policy, GPU, CPU, memory, or upload requirement selects the
CLI path.
- Around line 129-132: Update the OpenShellClient.connect options in the gateway
connection flow so insecureSkipVerify is controlled by an explicit per-gateway
trust/insecure configuration setting, not by whether gateway starts with
https://. Preserve normal TLS certificate verification unless that setting
explicitly enables insecure mode.
🪄 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: 4f19346a-0cbb-45c5-8acb-eabb5435f194
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
package.jsonpackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: unit tests / macos-15
- 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: Windows
- GitHub Check: macOS
- GitHub Check: unit tests / windows-2022
🧰 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/openshell-cli/openshell-cli.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/openshell-cli/openshell-cli.tspackages/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
engineIdparameter to identify the container engine
Files:
packages/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-cli.spec.ts
🧠 Learnings (3)
📚 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.tspackages/main/src/plugin/openshell-cli/openshell-cli.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/openshell-cli/openshell-cli.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.ts
📚 Learning: 2026-06-29T13:16:53.102Z
Learnt from: benoitf
Repo: openkaiden/kaiden PR: 2296
File: extensions/container/packages/extension/src/helper/socket-finder/_socket-finder-module.ts:28-29
Timestamp: 2026-06-29T13:16:53.102Z
Learning: When reviewing imports in openkaiden/kaiden TypeScript/JavaScript files, prefer the configured `/@/` path alias instead of relative imports that would require traversing out of the current directory/module group (i.e., paths containing `..` that cross boundaries).
Do not require alias conversion for descendant-path relative imports within the socket-finder module directory—for example, in `extensions/container/packages/extension/src/helper/socket-finder/**`, imports like `./podman/podman-version-detector` and `./podman/podman-windows-finder` are acceptable and should not be flagged.
Applied to files:
packages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.ts
🔇 Additional comments (9)
package.json (1)
219-219: 📐 Maintainability & Code Quality | 🏗️ Heavy liftLocal
link:dependency to a sibling repo path will break outside this exact checkout.
"@nvidia/openshell-sdk": "link:../OpenShell/sdk/typescript"requires a siblingOpenShellcheckout at a specific relative path. This will fail to install for CI runners, other contributors, or any packaging pipeline that only clones this repo, sincenpm/yarnlink:resolves to a local filesystem path rather than a registry artifact.Confirm this is an intentional temporary step before publishing
@nvidia/openshell-sdkto a registry, and track replacing it before merge/release.packages/main/src/plugin/openshell-cli/openshell-cli.ts (6)
22-22: LGTM!
56-64: 🎯 Functional Correctness | ⚡ Quick win
resource_versionof0is incorrectly dropped.
Number(ref.resourceVersion) || undefinedtreats0as falsy, so a sandbox withresourceVersion: '0'maps toresource_version: undefinedinstead of0. Use a NaN check instead.🐛 Proposed fix
- resource_version: Number(ref.resourceVersion) || undefined, + resource_version: ref.resourceVersion === undefined || Number.isNaN(Number(ref.resourceVersion)) + ? undefined + : Number(ref.resourceVersion),
108-108: 🩺 Stability & Availability | ⚡ Quick winCached SDK clients are never released, and concurrent cache misses can double-connect.
#sdkClientsgrows for the process lifetime butdispose()(lines 613-620) only clears the deleting-poll timer and disposes the emitter — cachedOpenShellClientinstances are never closed. If the SDK holds a persistent connection per client, this leaks a connection per distinct gateway endpoint string for the app's lifetime. Separately, two concurrent calls togetSdkClientfor the same uncached endpoint will both callOpenShellClient.connect(...), and the last one to finish wins the cache slot, leaking the other connection.Please confirm whether
OpenShellClientrequires an explicit close/disconnect call, and if so, wire it intodispose(); consider caching the in-flightPromise<OpenShellClient>rather than the resolved client to avoid duplicate concurrent connects.Also applies to: 120-136
298-311: 🚀 Performance & Scalability | ⚡ Quick winRedundant gateway-list CLI calls per sandbox listing (N+1 pattern).
listSandboxesalways re-resolves the gateway endpoint viaresolveGatewayEndpoint→listGateways()(a fullopenshell gateway listsubprocess call), even when the caller already has the gateway list.listSandboxesPerGatewayfetches gateways once, then callslistSandboxes(gateway.name)per gateway — each of which re-fetches the full gateway list again, andlistSandboxesForGatewaydoes the same. This doubles CLI subprocess spawns per gateway on every call, and is exercised repeatedly by the 5-second auto-refresh poll.This is corroborated by
packages/main/src/plugin/openshell-cli/openshell-cli.spec.ts(e.g. lines 853-856, 922-926, 964-971), whose mockedexec.execsequences now require two consecutivegatewaysresolutions per gateway per cycle.Consider letting
listSandboxesaccept an already-resolved endpoint (or the pre-fetched gateway list) so callers that already hold the gateway data don't trigger a second CLI round-trip.Also applies to: 320-328
339-353: 🩺 Stability & Availability | ⚡ Quick win
waitDeletedtimeout after a successful delete triggers a redundant CLI delete retry.
client.sandbox.delete(name)andclient.sandbox.waitDeleted(name, 30)share one try/catch. If delete succeeds but the 30s wait throws (timeout), execution falls through torunCli(['sandbox', 'delete', name])on a sandbox that may already be gone — the CLI delete could then fail (surfacing an error to the caller despite the operation having actually succeeded). No test covers this partial-success scenario.Separate the two calls so a
waitDeletedfailure doesn't re-trigger the CLI fallback for an already-deleted sandbox.🛠️ Proposed fix
if (gateway) { try { const client = await this.getSdkClient(gateway); await client.sandbox.delete(name); - await client.sandbox.waitDeleted(name, 30); - return; + try { + await client.sandbox.waitDeleted(name, 30); + } catch (waitErr: unknown) { + console.warn( + `[openshell] waitDeleted timed out after successful delete: ${waitErr instanceof Error ? waitErr.message : String(waitErr)}`, + ); + } + return; } catch (err: unknown) {
464-471: LGTM!packages/main/src/plugin/openshell-cli/openshell-cli.spec.ts (2)
405-468: LGTM!Also applies to: 518-553, 610-641, 1079-1124
816-823: LGTM!Also applies to: 850-858, 906-926, 943-995, 1007-1063
| const client = await OpenShellClient.connect({ | ||
| gateway, | ||
| insecureSkipVerify: gateway.startsWith('https://'), | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
@nvidia/openshell-sdk OpenShellClient connect insecureSkipVerify option
💡 Result:
In the NVIDIA OpenShell ecosystem, the option to skip TLS certificate verification is typically referred to as insecure [1][2] or gateway_insecure [3], rather than insecureSkipVerify. Support for disabling TLS verification is available across the OpenShell SDKs to facilitate testing or deployments using self-signed certificates [1][2][4]. Python SDK In the Python SandboxClient, the insecure boolean parameter is used to disable TLS certificate verification for OIDC discovery and gateway connections [1][2]. It is off by default [1]. - Usage: SandboxClient(..., insecure=True) [1][2]. Rust CLI / SDK The Rust-based openshell-sdk and its associated CLI utilize a TlsOptions structure containing a gateway_insecure boolean field [3]. When set to true, the client skips TLS certificate verification for gateway connections [3]. Community Implementations Community-developed wrappers, such as openshell-node, also provide an insecure option (defaulting to false) to skip TLS for testing purposes [4]. General Guidance Official documentation consistently emphasizes that these insecure options are intended for testing or specific self-signed infrastructure [1][2][4]. Production environments should rely on the standard mTLS or OIDC-based secure channel configurations [2][5]. [6][1][3][2][4]
Citations:
- 1: https://github.com/NVIDIA/OpenShell/blob/abe42fb5/python/openshell/sandbox.py
- 2: feat(python-sdk): support OIDC Bearer auth on SandboxClient NVIDIA/OpenShell#1621
- 3: https://github.com/NVIDIA/OpenShell/blob/abe42fb5/crates/openshell-cli/src/tls.rs
- 4: https://github.com/moonshot-partners/openshell-node
- 5: https://deepwiki.com/NVIDIA/OpenShell/8.5-python-sdk
- 6: feat(sdk): add openshell-sdk crate NVIDIA/OpenShell#1862
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- openshell-cli.ts around the cited lines ---'
sed -n '90,150p' packages/main/src/plugin/openshell-cli/openshell-cli.ts | cat -n
echo
echo '--- search for insecureSkipVerify / gateway_insecure / insecure flags in repo ---'
rg -n "insecureSkipVerify|gateway_insecure|insecure\s*:|gateway.*insecure|skipVerify|skip verification" packages/main/src -g '!**/dist/**' -g '!**/build/**' || true
echo
echo '--- locate gateway config / client connect types ---'
rg -n "interface .*Gateway|type .*Gateway|OpenShellClient\.connect|connect\(" packages/main/src packages -g '!**/dist/**' -g '!**/build/**' || trueRepository: openkaiden/kaiden
Length of output: 12398
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- gateway types ---'
sed -n '1,220p' packages/api/src/openshell-gateway-info.ts | cat -n
echo
echo '--- openshell-gateway implementation ---'
sed -n '1,220p' packages/main/src/plugin/openshell-cli/openshell-gateway.ts | cat -n
echo
echo '--- openshell-cli tests around gateway insecure behavior ---'
sed -n '1080,1145p' packages/main/src/plugin/openshell-cli/openshell-cli.spec.ts | cat -n
echo
echo '--- any config fields related to gateway trust/insecure in renderer/store ---'
rg -n "gateway.*insecure|insecure.*gateway|trust.*gateway|certificate|tls" packages -g '!**/dist/**' -g '!**/build/**' || trueRepository: openkaiden/kaiden
Length of output: 38569
Do not disable TLS verification based on the URL scheme insecureSkipVerify is enabled for every https:// gateway here, so certificate checks are skipped for any HTTPS endpoint. Gate this behind an explicit per-gateway trust/insecure setting instead.
🤖 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/openshell-cli/openshell-cli.ts` around lines 129 -
132, Update the OpenShellClient.connect options in the gateway connection flow
so insecureSkipVerify is controlled by an explicit per-gateway trust/insecure
configuration setting, not by whether gateway starts with https://. Preserve
normal TLS certificate verification unless that setting explicitly enables
insecure mode.
| const requiresCli = !!( | ||
| options.uploads?.length ?? | ||
| options.command?.length ?? | ||
| options.policy ?? | ||
| options.gpuDevice ?? | ||
| options.cpu ?? | ||
| options.memory | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
?? chaining across heterogeneous fields can silently skip real CLI-only options.
?? only short-circuits on null/undefined, not on falsy-but-defined values. If options.uploads is [] (length 0, defined) or options.policy/options.cpu/options.memory is '', the chain stops there and never evaluates the remaining fields — even if e.g. options.command is non-empty. Since the SDK client.sandbox.create call below never forwards command, policy, cpu, memory, or gpuDevice, a request like { uploads: [], command: ['ls'] } would incorrectly take the SDK path and silently drop command.
No existing test exercises this combination (all cases set the earlier fields to undefined, not to an empty-but-defined value).
🐛 Proposed fix
- const requiresCli = !!(
- options.uploads?.length ??
- options.command?.length ??
- options.policy ??
- options.gpuDevice ??
- options.cpu ??
- options.memory
- );
+ const requiresCli = !!(
+ options.uploads?.length ||
+ options.command?.length ||
+ options.policy ||
+ options.gpuDevice ||
+ options.cpu ||
+ options.memory
+ );📝 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 requiresCli = !!( | |
| options.uploads?.length ?? | |
| options.command?.length ?? | |
| options.policy ?? | |
| options.gpuDevice ?? | |
| options.cpu ?? | |
| options.memory | |
| ); | |
| const requiresCli = !!( | |
| options.uploads?.length || | |
| options.command?.length || | |
| options.policy || | |
| options.gpuDevice || | |
| options.cpu || | |
| options.memory | |
| ); |
🤖 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/openshell-cli/openshell-cli.ts` around lines 206 -
213, Update the requiresCli calculation to evaluate each CLI-only option
independently instead of chaining heterogeneous values with ??. Ensure empty
arrays or strings do not suppress later non-empty options, so any meaningful
command, policy, GPU, CPU, memory, or upload requirement selects the CLI path.
1 participant
Summary
Integrate the OpenShell TypeScript SDK (
@nvidia/openshell-sdk) intoOpenshellCliusing an SDK-first strategy with automatic CLI fallback. When a gateway endpoint is resolvable, sandbox operations go through the SDK's gRPC client; if the SDK call fails or the operation requires CLI-only features (uploads, policy files, resource flags), it transparently falls back to spawning theopenshellbinary.What changed
openshell-cli.ts— Added SDK integration for four operations:createSandbox: uses SDK when no CLI-only options are present (uploads,policy,cpu,memory,gpuDevice,command). Falls back to CLI otherwise.listSandboxes: resolves the gateway endpoint, callsclient.sandbox.list(), convertsSandboxRef[]toSandboxInfo[]viasandboxRefToInfo. Falls back to CLI on failure.deleteSandbox: callsclient.sandbox.delete()+client.sandbox.waitDeleted()(30s timeout). Falls back to CLI on failure.checkEndpointStatus: callsclient.health()before attempting the CLIstatuscommand.PHASE_MAP+sandboxRefToInfo— The SDK returns phases in lowercase (ready,provisioning, etc.) while the rest of the codebase (Zod schemas inopenshell-gateway-info.ts, renderer comparisons inworkspace-utils.ts) expects PascalCase (Ready,Provisioning). ThePHASE_MAPadapter handles this conversion.getSdkClient— Creates and cachesOpenShellClientinstances per gateway endpoint, handling URL normalization andinsecureSkipVerifyfor HTTPS endpoints.resolveGatewayEndpoint— Private helper that callslistGateways()to find the endpoint for a gateway name (or the active gateway).package.json— Added@nvidia/openshell-sdkas alink:dependency pointing to the local OpenShell SDK build (see testing instructions below).Test changes
createSandbox,listSandboxes,deleteSandbox, andcheckEndpointStatus.listSandboxesPerGateway auto-refreshtests to account for the additionallistGateways()call made byresolveGatewayEndpointinsidelistSandboxes, and mockedgetSdkClientin the auto-refreshbeforeEachto avoid real gRPC connections under fake timers.How to test
1. Clone and build the OpenShell SDK
4. Manual E2E verification (requires a running OpenShell gateway)
pnpm watch)SDK ... failed, falling back to CLImeans the SDK path succeeded):Executing: openshell sandbox create ...log)Known limitations
This is an incremental integration. The following operations remain CLI-only and are tracked locally:
startSandbox/stopSandbox— not yet exposed in the SDKcreateSandboxwith--upload,--policy,--cpu,--memory,--gpu-device— require CLI-only flagsconnectSandbox— requiresExecSandboxInteractive(bidi streaming), not yet wrapped in the TS SDKgatewayandprovidermanagement commands — gateway-scoped, not sandbox-scopedCreateSshSession/ForwardTcpRPCs planned for future SDK releases