feat(openshell): add openshell-gateway binary discovery and CLI tool registration - #2333
Conversation
|
Warning Review limit reached
Next review available in: 47 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 (2)
📝 WalkthroughWalkthroughAdds openshell-gateway configuration and CLI registration, updates Electron fixture shutdown and temporary settings setup, and increases a Playwright smoke-test visibility timeout. ChangesOpenShell Gateway CLI Registration
Playwright test stability
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
needs to be rebased after #2331 |
f06942e to
a622d5c
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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 `@extensions/openshell/src/manager/openshell-cli-manager.ts`:
- Around line 98-114: The `init()` flow in `openshell-cli-manager.ts` should not
exit early when `discoverBinary('openshell-gateway', ...)` returns nothing,
because that makes later setup in `OpenShellCliManager.init` unreachable. Remove
the `return`, keep the warning, and still proceed with `registerCliTool` using a
valid fallback `gwRegistration` object when `gwResult` is missing. Also simplify
the `gwRegistration` assignment so the `installationSource: 'extension'`
fallback is used directly and not left as dead code.
🪄 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: 597419f0-601c-4404-be85-a43e8fb5644f
📒 Files selected for processing (2)
extensions/openshell/package.jsonextensions/openshell/src/manager/openshell-cli-manager.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
⚠️ CI failures not shown inline (4)
GitHub Actions: fullsend / dispatch _ Route: feat(openshell): add openshell-gateway binary discovery and CLI tool registration
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: feat(openshell): add openshell-gateway binary discovery and CLI tool registration
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 / dispatch _ Route: feat(openshell): add openshell-gateway binary discovery and CLI tool registration
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 / 6_dispatch _ Route.txt: feat(openshell): add openshell-gateway binary discovery and CLI tool registration
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 (3)
extensions/*/package.json
📄 CodeRabbit inference engine (AGENTS.md)
extensions/*/package.json: Extensions must declareengines.kaidenversion compatibility in theirpackage.json
Extensionpackage.jsonmust havemainfield pointing to./dist/extension.js
Configuration properties for API keys, tokens, or secrets must use"format": "password"in the configuration definition to ensure input masking in the UI
Files:
extensions/openshell/package.json
**/*.{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:
extensions/openshell/src/manager/openshell-cli-manager.ts
extensions/*/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Register inference, container, and Kubernetes providers through the
ProviderRegistryvia extension APIs
Files:
extensions/openshell/src/manager/openshell-cli-manager.ts
🧠 Learnings (5)
📚 Learning: 2026-05-06T11:29:33.170Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1671
File: extensions/vertex-ai/package.json:9-11
Timestamp: 2026-05-06T11:29:33.170Z
Learning: In the openkaiden/kaiden repo, all built-in extensions under extensions/ should specify engines with kaiden: "^0.0.1" in package.json. Do not flag each extension individually; enforce a repo-wide alignment in a single PR. During reviews, verify that every extensions/*/package.json has "engines": { "kaiden": "^0.0.1" }. If a file deviates, surface the discrepancy as a single repo-wide task rather than per-file.
Applied to files:
extensions/openshell/package.json
📚 Learning: 2026-05-05T17:44:50.991Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1671
File: extensions/vertex-ai/src/vertex-ai.ts:363-387
Timestamp: 2026-05-05T17:44:50.991Z
Learning: In this repo (openkaiden/kaiden), do not raise a code review issue when an extension’s `InferenceProviderConnectionFactory.create` factory method implementation omits (or does not use) the optional `logger` and/or `CancellationToken` parameters in its method signature/implementation. Current extensions (e.g., Vertex AI, Gemini, Claude, Mistral, OpenAI-compatible) follow this pattern, so reviewers should treat it as acceptable for `extensions/*` TypeScript source files.
Applied to files:
extensions/openshell/src/manager/openshell-cli-manager.ts
📚 Learning: 2026-05-12T10:01:14.248Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1810
File: extensions/kdn/src/kdn-extension.ts:43-46
Timestamp: 2026-05-12T10:01:14.248Z
Learning: In this repo’s extension code, when logging from binary discovery/resolution logic (e.g., choosing/validating custom paths, extension storage locations, or bundled resource paths), it’s intentional to include full filesystem paths in `console.log`/`console.warn` (such as in `extensions/**/src/*-extension.ts`). During review, do not flag these specific full-path messages as a privacy/security issue as long as they are clearly part of the binary resolution steps. If full-path logging appears outside binary discovery/resolution, review/flag it as usual.
Applied to files:
extensions/openshell/src/manager/openshell-cli-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:
extensions/openshell/src/manager/openshell-cli-manager.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:
extensions/openshell/src/manager/openshell-cli-manager.ts
🔇 Additional comments (1)
extensions/openshell/package.json (1)
47-54: LGTM!
|
somehow it's breaking the e2e tests (issues when stopping the application, tests still pass 🙃 ) |
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 `@tests/playwright/src/fixtures/electron-app.ts`:
- Around line 158-168: The shutdown race in the electron app teardown leaves a
dangling timeout and can also surface an unhandled rejection from the async
close path. Update the teardown logic around the Promise.race block to keep a
reference to the timeout and clear it once the race settles, and ensure the
closeAllWindows(app) / app.close() promise is always observed so a late failure
is caught or suppressed after timeout. Use the teardown code in electron-app.ts
and the closeAllWindows/app.close sequence to locate the fix.
🪄 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: 35da3db0-54ff-4afb-9264-d940ef5e350a
📒 Files selected for processing (2)
tests/playwright/src/fixtures/electron-app.tstests/playwright/src/specs/workspaces-smoke.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: typecheck
- GitHub Check: unit tests / macos-15
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: linter, formatters
- GitHub Check: macOS
- GitHub Check: unit tests / windows-2022
⚠️ CI failures not shown inline (4)
GitHub Actions: fullsend / dispatch _ Route: feat(openshell): add openshell-gateway binary discovery and CLI tool registration
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 / dispatch _ Route: feat(openshell): add openshell-gateway binary discovery and CLI tool registration
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 / 6_dispatch _ Route.txt: feat(openshell): add openshell-gateway binary discovery and CLI tool registration
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 / dispatch _ Route: feat(openshell): add openshell-gateway binary discovery and CLI tool registration
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
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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:
tests/playwright/src/specs/workspaces-smoke.spec.tstests/playwright/src/fixtures/electron-app.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:
tests/playwright/src/specs/workspaces-smoke.spec.ts
tests/playwright/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use the Page Object Model pattern for Playwright E2E tests with proper locator conventions
Files:
tests/playwright/src/specs/workspaces-smoke.spec.tstests/playwright/src/fixtures/electron-app.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:
tests/playwright/src/specs/workspaces-smoke.spec.tstests/playwright/src/fixtures/electron-app.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:
tests/playwright/src/specs/workspaces-smoke.spec.tstests/playwright/src/fixtures/electron-app.ts
🔇 Additional comments (2)
tests/playwright/src/fixtures/electron-app.ts (1)
356-364: LGTM!tests/playwright/src/specs/workspaces-smoke.spec.ts (1)
278-278: LGTM!
|
Maybe a rebase might fix e2e? |
bmahabirbu
left a comment
bmahabirbu
left a comment
There was a problem hiding this comment.
LGTM codewise and gave it a test gateway shows up just fine on linux
not related but it would be cool if we could consolidate the openshell cards into one bigger card now that we have 3 binaries
8653136 to
cef63c0
Compare
…registration Add discovery and registration of the `openshell-gateway` binary using the existing binary resolution pipeline (custom config → bundled → storage → system). Introduce `openshell.gateway.binary.path` configuration property for custom binary paths (non-Windows only). Closes #2332 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Florent Benoit <fbenoit@redhat.com>
cef63c0 to
a8d67f5
Compare
4 participants
Summary
openshell-gatewaybinary using the existing binary resolution pipeline (custom config → bundled → storage → system PATH)openshell.gateway.binary.pathconfiguration property for custom binary paths (non-Windows only)Closes #2332
fixes #2147
Test plan
openshell-gatewayappears in CLI tools list when the binary is available on the systemopenshell.gateway.binary.pathconfiguration takes prioritywhen: "!isWindows")openshellandopenshell-image-builderregistration is unaffected