feat(openshell): build and create sandbox via openshell-image-builder - #2135
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAgentWorkspaceManager gains an OpenShell-gated create path (KAIDEN_OPENSHELL=1) that builds a sanitized workspace image via OpenshellImageBuilder (cwd-aware) and calls OpenshellCli.createSandbox (supports noTty); tests cover the OpenShell flow and CLI flag placement. ChangesOpenShell Workspace Creation
Sequence Diagram(s)sequenceDiagram
participant Test
participant AgentWorkspaceManager
participant KdnCli
participant OpenshellImageBuilder
participant OpenshellCli
Test->>AgentWorkspaceManager: create(options) with KAIDEN_OPENSHELL=1
AgentWorkspaceManager->>KdnCli: writeWorkspaceConfig(options)
AgentWorkspaceManager->>AgentWorkspaceManager: sanitizeImageTag(name or sourcePath)
AgentWorkspaceManager->>OpenshellImageBuilder: buildImage(tag, { agent/model/inference/endpoint, cwd })
OpenshellImageBuilder-->>AgentWorkspaceManager: builtImageRef
AgentWorkspaceManager->>OpenshellCli: createSandbox({ from: builtImageRef, providers, labels (base64url sourcePath), noTty:true, command:['true'] })
OpenshellCli-->>AgentWorkspaceManager: sandboxId
AgentWorkspaceManager-->>Test: { id: sandboxId }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 155-160: The createSandbox call in agent-workspace-manager.ts is
missing the required engineId routing info; update the
AgentWorkspaceCreateOptions type to include an engineId field (e.g., engineId:
string) and plumb the selected engine through the workspace creation flow, then
pass that engineId into this.openshellCli.createSandbox({...}) as the engineId
property alongside name/from/providers/labels so container operations include
the engine identifier for correct multi-engine routing.
- Around line 155-160: When calling openshellCli.createSandbox in
agent-workspace-manager (the block using sandboxName, imageTag and
options.secrets), merge options.workspaceConfiguration?.secrets into the
providers list so onboarding-owned secrets are included; specifically build
providers from both options.secrets and options.workspaceConfiguration.secrets
(deduplicating if needed) before passing to createSandbox (this complements
ensureModelSecret which stores onboarding secrets separately). Update the call
site that currently passes only options.secrets to pass the merged providers
array, and add a regression test that exercises the OpenShell workspace creation
path with workspaceConfiguration.secrets to assert the created sandbox receives
those provider bindings.
- Around line 124-131: sanitizeImageTag currently can return an empty string for
names consisting only of disallowed characters; update sanitizeImageTag to
guarantee a non-empty component by appending or replacing with a stable fallback
when the sanitized result is empty (e.g., compute a short deterministic hash or
base36 suffix from the original name and return a fallback like
`fallback-<shortHash>` or prefix with `img-<shortHash>`); ensure the function
still lowercases and strips invalid chars, but if
.split(...).filter(Boolean).join('-') yields an empty string, return the
deterministic fallback so callers (e.g., image name construction that uses
sanitizeImageTag) never produce an invalid image tag.
🪄 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: e592f1d7-0fb5-406b-b67c-6e41516483ca
📒 Files selected for processing (3)
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/openshell-cli/openshell-image-builder.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: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: Windows
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: macOS
- GitHub Check: linter, formatters
- GitHub Check: Linux
- GitHub Check: unit tests / windows-2025
- GitHub Check: unit tests / macos-15
- GitHub Check: typecheck
🧰 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-image-builder.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.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-image-builder.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/openshell-cli/openshell-image-builder.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.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:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.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/openshell-cli/openshell-image-builder.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.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-image-builder.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
🪛 OpenGrep (1.22.0)
packages/main/src/plugin/openshell-cli/openshell-image-builder.ts
[ERROR] 101-101: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/api/src/openshell-gateway-info.ts (1)
43-56:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd
engineIdto the sandbox-create contract.
CreateSandboxOptionsstill has noengineId, so downstream main-process container operations cannot thread engine routing information. This violates the multi-engine contract and leavescreateSandbox()calls ambiguous.Suggested diff
export interface CreateSandboxOptions { + engineId: string; name?: string; gateway?: string; from?: string; gpu?: boolean; gpuDevice?: string; cpu?: string; memory?: string; providers?: string[]; labels?: Record<string, string>; uploads?: Array<{ local: string; remote: string }>; command?: string[]; noTty?: boolean; }As per coding guidelines, container operations must include
engineIdparameter to identify the container engine.🤖 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/api/src/openshell-gateway-info.ts` around lines 43 - 56, CreateSandboxOptions is missing engineId so engine routing can't be propagated; add an optional engineId?: string property to the CreateSandboxOptions interface (the object passed to createSandbox) and update any call sites or handlers that construct or forward CreateSandboxOptions to accept and forward engineId so container operations can route by engine. Ensure the property name is exactly engineId and is treated as optional where not yet provided.Source: Coding guidelines
packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts (2)
124-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuarantee non-empty sanitized image tags.
For names that sanitize to nothing,
sanitizeImageTag()returns'', and Line 145 buildskaiden-workspace-:latest, which is invalid and breaks workspace creation.Suggested diff
private sanitizeImageTag(name: string): string { - return name + const sanitized = name .toLowerCase() .replace(/[^a-z0-9-]+/g, '-') .split('-') .filter(Boolean) .join('-'); + if (sanitized) return sanitized; + const fallback = Buffer.from(name).toString('base64url').slice(0, 12).toLowerCase(); + return `ws-${fallback || 'default'}`; }Also applies to: 145-145
🤖 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 124 - 131, sanitizeImageTag can return an empty string for inputs that strip to nothing, causing invalid image names like "kaiden-workspace-:latest"; update sanitizeImageTag (and its usage) to guarantee a non-empty, valid token by: after the current normalize/filter/join steps, fallback to a safe default (e.g., "workspace" or "default") if the result is empty, and ensure no leading/trailing dashes remain; reference sanitizeImageTag and the code that composes the image name (the kaiden-workspace-... builder) to apply this change so image tags are always valid.
155-159:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMerge onboarding secrets into
providersfor OpenShell sandbox creation.Line 158 forwards only
options.secrets, butensureModelSecret()intentionally skips mutation whenworkspaceConfiguration.secretsis present (Line 190). That drops onboarding-owned provider bindings in OpenShell mode.Suggested diff
+ const providers = [...new Set([...(options.secrets ?? []), ...(options.workspaceConfiguration?.secrets ?? [])])]; + await this.openshellCli.createSandbox({ name: sandboxName, from: imageTag, - providers: options.secrets, + providers: providers.length > 0 ? providers : undefined, labels: { 'ai.openkaiden.kaiden.workspace': Buffer.from(options.sourcePath).toString('base64url') }, noTty: true, command: ['true'], });🤖 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 155 - 159, The createSandbox call currently forwards only options.secrets, which drops onboarding-owned provider bindings because ensureModelSecret() avoids mutating when workspaceConfiguration.secrets exists; fix by merging onboarding provider bindings into the providers passed to openshellCli.createSandbox: compute a mergedProviders object/structure (e.g., combine options.secrets with the onboarding-owned providers that ensureModelSecret or the onboarding config provides) and pass mergedProviders as the providers argument to openshellCli.createSandbox (referencing openshellCli.createSandbox, sandboxName, imageTag, options.secrets, ensureModelSecret, workspaceConfiguration.secrets).
🤖 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.
Duplicate comments:
In `@packages/api/src/openshell-gateway-info.ts`:
- Around line 43-56: CreateSandboxOptions is missing engineId so engine routing
can't be propagated; add an optional engineId?: string property to the
CreateSandboxOptions interface (the object passed to createSandbox) and update
any call sites or handlers that construct or forward CreateSandboxOptions to
accept and forward engineId so container operations can route by engine. Ensure
the property name is exactly engineId and is treated as optional where not yet
provided.
In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts`:
- Around line 124-131: sanitizeImageTag can return an empty string for inputs
that strip to nothing, causing invalid image names like
"kaiden-workspace-:latest"; update sanitizeImageTag (and its usage) to guarantee
a non-empty, valid token by: after the current normalize/filter/join steps,
fallback to a safe default (e.g., "workspace" or "default") if the result is
empty, and ensure no leading/trailing dashes remain; reference sanitizeImageTag
and the code that composes the image name (the kaiden-workspace-... builder) to
apply this change so image tags are always valid.
- Around line 155-159: The createSandbox call currently forwards only
options.secrets, which drops onboarding-owned provider bindings because
ensureModelSecret() avoids mutating when workspaceConfiguration.secrets exists;
fix by merging onboarding provider bindings into the providers passed to
openshellCli.createSandbox: compute a mergedProviders object/structure (e.g.,
combine options.secrets with the onboarding-owned providers that
ensureModelSecret or the onboarding config provides) and pass mergedProviders as
the providers argument to openshellCli.createSandbox (referencing
openshellCli.createSandbox, sandboxName, imageTag, options.secrets,
ensureModelSecret, workspaceConfiguration.secrets).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 53903256-2303-4f04-8ccc-66a5efeb3a15
📒 Files selected for processing (5)
packages/api/src/openshell-gateway-info.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/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 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 / ubuntu-24.04
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: Windows
- GitHub Check: macOS
- GitHub Check: Linux
- GitHub Check: linter, formatters
- GitHub Check: unit tests / windows-2025
- GitHub Check: typecheck
- GitHub Check: unit tests / macos-15
🧰 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.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/api/src/openshell-gateway-info.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.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:
packages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.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.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.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/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.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/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.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.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/api/src/openshell-gateway-info.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
🔇 Additional comments (3)
packages/main/src/plugin/openshell-cli/openshell-cli.ts (1)
147-149: LGTM!packages/main/src/plugin/openshell-cli/openshell-cli.spec.ts (1)
255-261: LGTM!packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts (1)
397-404: LGTM!
benoitf
left a comment
benoitf
left a comment
There was a problem hiding this comment.
I don't think we should build image automatically, it should be optional
as you may not be able to build the image locally and you would need to fetch the image so it would require a registry
What would be the fallback when we don't build the image? |
Adds OpenShell workspace creation path: when KAIDEN_OPENSHELL is set, create() builds a container image with openshell-image-builder (passing agent, model, inference, and endpoint) then provisions the sandbox via openshell CLI instead of kdn. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
… immediately Without these flags the sandbox create command connects interactively and never returns, leaving the task in progress indefinitely. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
…mpty Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Jeff MAURY <jmaury@redhat.com>
Signed-off-by: Jeff MAURY <jmaury@redhat.com>
|
I'm not sure to get why it has been merged without having it optional. |
3 participants
Adds OpenShell workspace creation path: when KAIDEN_OPENSHELL is set, create() builds a container image with openshell-image-builder (passing agent, model, inference, and endpoint) then provisions the sandbox via openshell CLI instead of kdn.
Fixes #2108
See demo with opencode + local llm (ollama) + skill
kaiden-openshell.mp4
Limitations:
needs fix: update OpenShell provider creation #2137 to work with credentials (anthropic inference, etc)