fix(openshell): remove stale gateway entry before re-registering on startup - #2606
Conversation
|
Warning Review limit reached
Next review available in: 55 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 Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesGateway re-registration
Estimated code review effort: 2 (Simple) | ~10 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 |
![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-gateway.spec.ts`:
- Around line 400-415: The test “removes stale gateway entry before
re-registering” only verifies that both gateway methods were called, not their
sequence. Update the assertions around openshellCli.removeGateway and
openshellCli.addGateway to assert that removeGateway is invoked before
addGateway, while preserving the existing argument checks.
🪄 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 Plus
Run ID: bfbd4af8-8862-460e-8760-0ceb9a0ca91d
📒 Files selected for processing (2)
packages/main/src/plugin/openshell-cli/openshell-gateway.spec.tspackages/main/src/plugin/openshell-cli/openshell-gateway.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: typecheck
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: linter, formatters
- GitHub Check: macOS
- 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
🧰 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-gateway.tspackages/main/src/plugin/openshell-cli/openshell-gateway.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-gateway.tspackages/main/src/plugin/openshell-cli/openshell-gateway.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-gateway.tspackages/main/src/plugin/openshell-cli/openshell-gateway.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.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-gateway.tspackages/main/src/plugin/openshell-cli/openshell-gateway.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-gateway.tspackages/main/src/plugin/openshell-cli/openshell-gateway.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-gateway.tspackages/main/src/plugin/openshell-cli/openshell-gateway.spec.ts
🔇 Additional comments (3)
packages/main/src/plugin/openshell-cli/openshell-gateway.ts (1)
357-359: LGTM!packages/main/src/plugin/openshell-cli/openshell-gateway.spec.ts (2)
74-74: LGTM!Also applies to: 96-96
417-432: LGTM!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
fbricon
left a comment
fbricon
left a comment
There was a problem hiding this comment.
No more errors about existing gateways
| await this.openshellCli.removeGateway('kaiden-local').catch(() => {}); | ||
| await this.openshellCli.addGateway({ endpoint, local: true, name: 'kaiden-local' }); | ||
| console.log(`[openshell-gateway] registered with CLI as kaiden-local at ${endpoint}`); |
There was a problem hiding this comment.
Is it not better to, instead of removing and adding, check if the gateway is there? Something like:
const gateways = await this.openshellCli.listGateways();
const existing = gateways.find(gw => gw.name === 'kaiden-local');
if (existing) {
if (!existing.active) {
await this.openshellCli.selectGateway('kaiden-local');
}
console.log(`[openshell-gateway] kaiden-local already registered, selected at ${existing.endpoint}`);
return;
}
await this.openshellCli.addGateway({ endpoint, local: true, name: 'kaiden-local' });
console.log(`[openshell-gateway] registered with CLI as kaiden-local at ${endpoint}`);
There was a problem hiding this comment.
it's like the endpoint might be changed
There was a problem hiding this comment.
I dropped all my env, will see if can reproduce again my issue and try your suggestion
There was a problem hiding this comment.
Adopted this approach : registerWithCli now calls listGateways() first:
- If
kaiden-localalready exists with the same endpoint, skip re-registration entirely - If it exists with a different endpoint, remove then re-add (handles endpoint changes across restarts)
- If it doesn't exist, just add directly (no unnecessary remove)
Also added an invocationCallOrder assertion per CodeRabbit's suggestion to verify remove-before-add ordering.
There was a problem hiding this comment.
Overall I think its good on my side but I came across an edge case where openshell gateway remove isn't always reliable. (mac/linux)
I removed the openshell default gateway and started kaiden. It correctly tried to start the kaiden-local gateway but encounter a conflict error because the default gateway still somehow hanged onto the process on that port and kaiden-local uses the same port 17670.
Maybe we need a follow up so we can scan the port activity and fully kill the gateway at the process level
11:28:49 AM [main] [openshell-gateway] registered with CLI as kaiden-local at http://127.0.0.1:17670
11:28:49 AM [main] openshell failed: /Users/bmahabir/kaiden/extensions/openshell/assets/darwin-arm64/openshell provider list -o json — Command execution failed with exit code 1 (stderr: Error: × code: 'Operation is not implemented or not supported', message: "grpc-
│ status header missing, mapped from HTTP status code 404")
11:28:49 AM [main] openshell failed: /Users/bmahabir/kaiden/extensions/openshell/assets/darwin-arm64/openshell sandbox list -g kaiden-local -o json — Command execution failed with exit code 1 (stderr: Error: × code: 'Operation is not implemented or not supported', message: "grpc-
Edit*
on further inspection the default gateway is created with mtls while local-kaiden uses plaintext. The mismatch from the hanged process caused the grpc error because openshell thinks local-kaiden has mtls
|
@fbricon if you can check again as I implemented the proposal of @MarsKubeX on my end it's working as well |
…tartup When Kaiden restarts, the openshell gateway from the previous session is no longer running but its `kaiden-local` CLI entry persists. The init flow correctly detects the gateway is unreachable and auto-starts a new one, but `registerWithCli()` fails with "Gateway 'kaiden-local' already exists". Fix by removing the stale entry before re-adding. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Florent Benoit <fbenoit@redhat.com>
Instead of always removing and re-adding, list gateways first and skip re-registration when kaiden-local already exists with the same endpoint. Only remove and re-add when the endpoint has changed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Florent Benoit <fbenoit@redhat.com>
4b34692 to
44ccc92
Compare
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-gateway.ts`:
- Around line 357-365: The gateway setup flow must still call addGateway when
listGateways fails. Separate listGateways error handling from the registration
path in the surrounding gateway setup method, treating discovery failure as no
existing gateway while preserving removal and early-return behavior for
successful lookups; add a regression test that rejects listGateways and verifies
registration proceeds.
🪄 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 Plus
Run ID: cdd61722-0888-49fd-b40d-bbd569975cef
📒 Files selected for processing (2)
packages/main/src/plugin/openshell-cli/openshell-gateway.spec.tspackages/main/src/plugin/openshell-cli/openshell-gateway.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: unit tests / windows-2022
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: Windows
- GitHub Check: unit tests / macos-15
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: typecheck
- GitHub Check: Linux
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: macOS
- GitHub Check: linter, formatters
🧰 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-gateway.tspackages/main/src/plugin/openshell-cli/openshell-gateway.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-gateway.tspackages/main/src/plugin/openshell-cli/openshell-gateway.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-gateway.tspackages/main/src/plugin/openshell-cli/openshell-gateway.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.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-gateway.tspackages/main/src/plugin/openshell-cli/openshell-gateway.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-gateway.tspackages/main/src/plugin/openshell-cli/openshell-gateway.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-gateway.tspackages/main/src/plugin/openshell-cli/openshell-gateway.spec.ts
🔇 Additional comments (1)
packages/main/src/plugin/openshell-cli/openshell-gateway.spec.ts (1)
74-74: LGTM!Also applies to: 97-97, 401-415, 417-439, 441-457, 459-477
MarsKubeX
left a comment
MarsKubeX
left a comment
There was a problem hiding this comment.
LGTM codewise. Just check the codeRabbit comment, seems valid to me.
|
FIx works for me. I am not convinced by coderabbit's explanation though. If openshell fails to list gateways, chances are you won't be able to add a new one |
4 participants
Summary
kaiden-localCLI entry before re-adding inregisterWithCli(), fixing "Gateway already exists" errors on restartFixes #2605
Test plan
removes stale gateway entry before re-registering— verifiesremoveGatewayis called beforeaddGatewayregisters successfully even when removeGateway fails— verifies graceful handling when no prior entry exists🤖 Generated with Claude Code