perf(workspaces): batch policies into sandbox create --policy YAML - #2354
Conversation
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughSandbox creation now accepts an optional policy path, which is forwarded to the CLI. Network and model endpoint inputs are now combined into a policy object, written to a temporary YAML file, and passed during sandbox creation instead of applying policy updates afterward. ChangesPolicy file passed to sandbox creation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/openshell-cli/openshell-network-policy.ts`:
- Around line 236-265: Remove the stale policy-update helpers by deleting
buildNetworkPolicyOperations, buildModelPolicyOperations, and
openshellCli.policyUpdate from openshell-network-policy.ts since they have no
non-test callers. Keep buildPolicyObject as the single source of truth for
mapping network/model endpoints into OpenshellPolicy, and update any internal
references in openshellCli or related helpers to use it directly so the policy
logic is not duplicated.
🪄 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: 1494a252-a36a-493b-b1c1-ef1588d7d642
📒 Files selected for processing (7)
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.tspackages/main/src/plugin/openshell-cli/openshell-network-policy.spec.tspackages/main/src/plugin/openshell-cli/openshell-network-policy.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: macOS
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: Linux
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: unit tests / windows-2022
- GitHub Check: unit tests / macos-15
- GitHub Check: Windows
⚠️ CI failures not shown inline (4)
GitHub Actions: fullsend / dispatch _ Route: perf(workspaces): batch policies into sandbox create --policy YAML
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mif [[ -f .fullsend/config.yaml ]]; then�[0m
�[36;1m KILL_SWITCH=$(yq '.kill_switch // false' .fullsend/config.yaml)�[0m
�[36;1m if [[ "$KILL_SWITCH" == "true" ]]; then�[0m
�[36;1m echo "::error::Kill switch is active — all agent dispatch halted"�[0m
GitHub Actions: fullsend / dispatch _ Route: perf(workspaces): batch policies into sandbox create --policy YAML
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mEVENT_PAYLOAD=$(jq -c '{�[0m
�[36;1m issue: (.issue // null | if . then {number, html_url} else null end),�[0m
�[36;1m pull_request: (.pull_request // null | if . then {number, html_url,�[0m
�[36;1m head: {ref: .head.ref, sha: .head.sha, repo: {full_name: .head.repo.full_name}},�[0m
�[36;1m base: {ref: .base.ref, repo: {full_name: .base.repo.full_name}}} else null end),�[0m
�[36;1m comment: (.comment // null | if . then {body: .body[:4096]} else null end)�[0m
�[36;1m}' "$GITHUB_EVENT_PATH") || {�[0m
�[36;1m echo "::error::Failed to extract event payload from GITHUB_EVENT_PATH"�[0m
GitHub Actions: fullsend / dispatch _ Route: perf(workspaces): batch policies into sandbox create --policy YAML
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1m�[0m
�[36;1mif [[ ! "$STAGE" =~ ^[a-z][a-z0-9_-]*$ ]]; then�[0m
�[36;1m echo "::error::Invalid stage name: must start with lowercase letter and contain only [a-z0-9_-]"�[0m
GitHub Actions: fullsend / 6_dispatch _ Route.txt: perf(workspaces): batch policies into sandbox create --policy YAML
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1m�[0m
�[36;1mif [[ ! "$STAGE" =~ ^[a-z][a-z0-9_-]*$ ]]; then�[0m
�[36;1m echo "::error::Invalid stage name: must start with lowercase letter and contain only [a-z0-9_-]"�[0m
🧰 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/api/src/openshell-gateway-info.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-network-policy.spec.tspackages/main/src/plugin/openshell-cli/openshell-network-policy.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.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.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-network-policy.spec.tspackages/main/src/plugin/openshell-cli/openshell-network-policy.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.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.tspackages/main/src/plugin/openshell-cli/openshell-network-policy.spec.tspackages/main/src/plugin/openshell-cli/openshell-network-policy.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.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.tspackages/main/src/plugin/openshell-cli/openshell-network-policy.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.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.tspackages/main/src/plugin/openshell-cli/openshell-network-policy.spec.tspackages/main/src/plugin/openshell-cli/openshell-network-policy.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-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:
packages/main/src/plugin/openshell-cli/openshell-cli.tspackages/api/src/openshell-gateway-info.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-network-policy.spec.tspackages/main/src/plugin/openshell-cli/openshell-network-policy.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.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/api/src/openshell-gateway-info.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-network-policy.spec.tspackages/main/src/plugin/openshell-cli/openshell-network-policy.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
🔇 Additional comments (8)
packages/api/src/openshell-gateway-info.ts (1)
80-95: LGTM!packages/main/src/plugin/openshell-cli/openshell-cli.ts (1)
192-194: LGTM!packages/main/src/plugin/openshell-cli/openshell-cli.spec.ts (1)
326-337: LGTM!packages/main/src/plugin/openshell-cli/openshell-network-policy.spec.ts (1)
208-293: LGTM!packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts (3)
195-222: 📐 Maintainability & Code Quality | ⚡ Quick win
createSandboxtiming label includes policy build/write time.
t0is captured beforebuildPolicyObject/writeFile(policyFile, ...)run, but the resulting duration is logged as[workspace-timing] createSandbox. Given this PR's whole purpose is to precisely diagnose where workspace-creation time goes, this mislabels the metric — future profiling of this log will attribute policy-file construction/I/O time to the CLI call itself.🕐 Suggested fix: separate the policy-build timer from the createSandbox timer
const t0 = performance.now(); const policy = buildPolicyObject(workspace.network, endpoint); let policyFile: string | undefined; if (policy) { policyFile = join(tmpdir(), `kaiden-policy-${sandboxName}-${Date.now()}.yaml`); await writeFile(policyFile, dump(policy), 'utf-8'); } + const tPolicy = performance.now(); + console.log(`[workspace-timing] policyBuild: ${(tPolicy - t0).toFixed(0)}ms`); + try { await this.openshellCli.createSandbox({ name: sandboxName, providers: options.secrets, env: env && Object.keys(env).length > 0 ? env : undefined, labels: { ...encodeWorkspaceLabels(options.sourcePath), [AGENT_LABEL]: options.agent }, uploads: uploads.length > 0 ? uploads : undefined, noTty: true, command: ['true'], policy: policyFile, }); } finally { if (policyFile) { await rm(policyFile, { force: true }).catch(() => {}); } } const tSandbox = performance.now(); - console.log(`[workspace-timing] createSandbox: ${(tSandbox - t0).toFixed(0)}ms`); + console.log(`[workspace-timing] createSandbox: ${(tSandbox - tPolicy).toFixed(0)}ms`);
215-219: 📐 Maintainability & Code Quality | 💤 Low valueCleanup failures are silently swallowed.
rm(policyFile, { force: true }).catch(() => {})hides any cleanup failure, making leaked temp policy files (containing network endpoint config) hard to diagnose later. Consider logging a warning on failure instead of a bare no-op catch.
26-26: LGTM!Also applies to: 36-36, 50-50, 223-231
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts (1)
19-19: LGTM!Also applies to: 57-57, 159-159, 443-509, 607-621, 642-647, 657-671, 687-688
Log elapsed time for each step in createOpenshell: createSandbox, enableV2Provider, networkPolicy, modelPolicy, and total wall-clock time. All prefixed with [workspace-timing]. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Fred Bricon <fbricon@gmail.com>
Build a single policy YAML file combining network and model rules, then pass it via --policy to openshell sandbox create instead of running sequential openshell policy update CLI calls. This replaces 5-7 sequential CLI invocations (~49s) with a single call, reducing workspace creation time from ~54s to ~5s. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Fred Bricon <fbricon@gmail.com>
…yObject Remove buildNetworkPolicyEndpoints, buildNetworkPolicyOperations, buildModelPolicyOperations, policyUpdate, and PolicyUpdateOptions which had no non-test callers after batching policies into sandbox create --policy YAML. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Fred Bricon <fbricon@gmail.com>
7c25aa1 to
92c61be
Compare
jeffmaury
left a comment
jeffmaury
left a comment
There was a problem hiding this comment.
LGTM but:
- no related issue
- we should remove logging before merging
3 participants
chore: add timing logs to openshell workspace creation
perf(workspaces): batch policies into sandbox create --policy YAML
Build a single policy YAML file combining network and model rules,
then pass it via --policy to openshell sandbox create instead of
running sequential openshell policy update CLI calls.
This replaces 5-7 sequential CLI invocations (~49s) with a single
call, reducing workspace creation time from ~54s to ~4s.