feat: wire network to OpenShell create workspace#2236
Conversation
|
Warning Review limit reached
More reviews will be available in 20 minutes and 55 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. 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 (4)
📝 WalkthroughWalkthroughAdds a ChangesOpenShell Network Policy Update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
jeffmaury
requested review from
benoitf,
bmahabirbu and
fbricon
and removed request for
a team
Fixes openkaiden#2235 Signed-off-by: Jeff MAURY <jmaury@redhat.com>
Signed-off-by: Jeff MAURY <jmaury@redhat.com>
jeffmaury
force-pushed
the
jeffmaury/GH-2235
branch
from
eab6e77 to
f746685
Compare
![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 196-200: The policyUpdate operation in the policy update flow is
missing explicit engineId parameter for cross-engine routing identification.
Thread the engineId through the container-policy update path by first
identifying where engineId is available in the current scope (likely from
workspace or a parent context), then pass it as a parameter to the
buildNetworkPolicyUpdateOptions function, and ensure that function includes
engineId in the returned policyUpdateOptions object so that when
this.openshellCli.policyUpdate is called, the options contain the explicit
engineId for proper engine routing.
- Around line 200-201: The policyUpdate call in openshellCli.policyUpdate() can
fail and leave the sandbox created in the preceding lines (187-194) in an
orphaned state. Wrap the policyUpdate call in a try-catch block and add a
compensating cleanup action in the catch handler to delete the sandbox that was
created, ensuring state consistency even when policy update fails. Perform the
cleanup as best-effort (catching and logging any errors from the cleanup itself)
before re-throwing the original error.
In `@packages/main/src/plugin/openshell-cli/openshell-network-policy.ts`:
- Around line 139-155: The buildNetworkPolicyUpdateOptions function removes the
old rule via removeRule but does not set ruleName when adding new endpoints. Add
the ruleName property to the returned PolicyUpdateOptions object with the value
NETWORK_RULE_NAME to ensure endpoints are added to the newly created kdn-network
rule instead of creating a rule with an auto-generated name. Include ruleName in
the object alongside removeRule and addEndpoints to match the pattern shown in
the test file.
🪄 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: 3ff84e1f-3e0c-40a1-abc9-f8e6cb02282e
📒 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. (10)
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: unit tests / macos-15
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: macOS
- GitHub Check: typecheck
- GitHub Check: unit tests / windows-2022
- GitHub Check: Windows
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: linter, formatters
- GitHub Check: Linux
🧰 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/api/src/openshell-gateway-info.tspackages/main/src/plugin/openshell-cli/openshell-network-policy.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-network-policy.tspackages/main/src/plugin/openshell-cli/openshell-cli.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-network-policy.spec.tspackages/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-network-policy.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-network-policy.tspackages/main/src/plugin/openshell-cli/openshell-cli.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-network-policy.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-network-policy.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
🧠 Learnings (2)
📚 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/openshell-cli/openshell-network-policy.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-network-policy.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.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-network-policy.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-network-policy.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
🪛 OpenGrep (1.22.0)
packages/main/src/plugin/openshell-cli/openshell-network-policy.ts
[WARNING] 114-114: Sequelize.literal() with dynamic input can lead to SQL injection. Use parameterized queries or model methods instead.
(coderabbit.sql-injection.sequelize-literal)
🔇 Additional comments (5)
packages/api/src/openshell-gateway-info.ts (1)
94-102: LGTM!packages/main/src/plugin/openshell-cli/openshell-network-policy.spec.ts (1)
74-87: ⚡ Quick winTest verifies current behavior but may need update if
ruleNameis added.This test verifies the exact structure of the returned
PolicyUpdateOptionsobject. If the implementation is updated to includeruleName: 'kdn-network'(as suggested in the previous file review), this test expectation will need to be updated accordingly.Potential test update if ruleName is added
expect(options).toEqual({ sandboxName: 'my-sandbox', + ruleName: 'kdn-network', removeRule: 'kdn-network', addEndpoints: ['registry.npmjs.org:443:full', 'registry.npmjs.org:80:full'], binary: '/**', wait: true, });packages/main/src/plugin/openshell-cli/openshell-cli.ts (1)
32-32: LGTM!Also applies to: 49-51, 193-216
packages/main/src/plugin/openshell-cli/openshell-cli.spec.ts (1)
401-524: LGTM!packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts (1)
571-636: LGTM!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Signed-off-by: Jeff MAURY <jmaury@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts (1)
198-203: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftThread
engineIdthrough this policy-update container operation.Line 198 builds operations without engine identity, and Line 202 executes a container policy mutation without
engineId. That violates the cross-layer container contract and can break routing in multi-engine setups.As per coding guidelines,
Container operations must include engineId parameter 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/main/src/plugin/agent-workspace/agent-workspace-manager.ts` around lines 198 - 203, The container policy-update path is missing the engine identity, which breaks the cross-layer container contract in multi-engine setups. Update the operation construction and execution flow in agent-workspace-manager around buildNetworkPolicyOperations and this.openshellCli.policyUpdate so the generated operation carries engineId and the policyUpdate call receives it as part of the container operation payload. Ensure every container-related policy mutation in this loop threads engineId through consistently.Source: Coding guidelines
🤖 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 200-205: The remove-only policy update path in
AgentWorkspaceManager is swallowing every exception, which can hide real
auth/transport/CLI failures. Tighten the catch around this
openshellCli.policyUpdate call so it only ignores the expected “rule not found”
case on fresh sandboxes, and lets all other errors propagate or be reported; use
the existing op.removeRule / op.addEndpoints guard and the policyUpdate method
in agent-workspace-manager.ts to locate the block.
In `@packages/main/src/plugin/openshell-cli/openshell-network-policy.spec.ts`:
- Around line 95-100: The expectation in the openshell network policy spec is
missing the returned ruleName field, so update the assertion for the add
operation to include the builder’s ruleName value. Use the existing ops[1] check
in openshell-network-policy.spec.ts and keep the rest of the expected shape
unchanged so the test validates the full contract.
---
Duplicate comments:
In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts`:
- Around line 198-203: The container policy-update path is missing the engine
identity, which breaks the cross-layer container contract in multi-engine
setups. Update the operation construction and execution flow in
agent-workspace-manager around buildNetworkPolicyOperations and
this.openshellCli.policyUpdate so the generated operation carries engineId and
the policyUpdate call receives it as part of the container operation payload.
Ensure every container-related policy mutation in this loop threads engineId
through consistently.
🪄 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: 43b40172-d1c0-47b8-baaa-d4a30efabf33
📒 Files selected for processing (4)
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-network-policy.spec.tspackages/main/src/plugin/openshell-cli/openshell-network-policy.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: unit tests / macos-15
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: Windows
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: linter, formatters
- GitHub Check: unit tests / windows-2022
- GitHub Check: macOS
- GitHub Check: Linux
🧰 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/agent-workspace/agent-workspace-manager.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.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/agent-workspace/agent-workspace-manager.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.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/agent-workspace/agent-workspace-manager.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.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-network-policy.spec.tspackages/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/agent-workspace/agent-workspace-manager.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.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/agent-workspace/agent-workspace-manager.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.spec.ts
🔇 Additional comments (1)
packages/main/src/plugin/openshell-cli/openshell-network-policy.ts (1)
127-137: LGTM!Also applies to: 139-162
Signed-off-by: Jeff MAURY <jmaury@redhat.com>
jeffmaury
force-pushed
the
jeffmaury/GH-2235
branch
from
503f89c to
3f05d10
Compare
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
3 participants
Fixes #2235