Conversation
Extract OpenshellCliBase abstract class with shared CLI plumbing and move gateway commands (add/remove/select/list, endpoint status, gateway status) into OpenshellGatewayCli. OpenshellCli now extends the base and delegates gateway queries to OpenshellGatewayCli. OpenshellGateway depends on OpenshellGatewayCli instead of the full OpenshellCli, and gates CLI access behind checkAvailable(). Fixes openkaiden#2371 Signed-off-by: Jeff MAURY <jmaury@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughRefactors OpenShell CLI integration by introducing ChangesOpenShell gateway CLI refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Plugin
participant OpenshellGateway
participant OpenshellGatewayCli
participant Exec
Plugin->>OpenshellGateway: init()
OpenshellGateway->>OpenshellGatewayCli: listGateways()
OpenshellGatewayCli->>Exec: gateway list -o json
Exec-->>OpenshellGatewayCli: stdout
OpenshellGatewayCli-->>OpenshellGateway: gateway list
OpenshellGateway->>OpenshellGatewayCli: checkEndpointStatus(endpoint)
OpenshellGatewayCli-->>OpenshellGateway: healthy/unhealthy
OpenshellGateway-->>OpenshellGateway: resolve `#availablePromise`
Plugin->>OpenshellGateway: checkAvailable()
OpenshellGateway-->>Plugin: readiness resolved
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/main/src/plugin/openshell-cli/openshell-gateway.spec.ts (1)
435-451: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding a regression test for the promise re-settlement bug.
This test correctly captures
gateway.checkAvailable()beforestart()fails, avoiding an unhandled rejection in the test itself. Given the critical finding inopenshell-gateway.ts(an already-settled#availablePromisecan't be updated by a later successfulstart()), consider adding a test that: (1) failsstart()once, (2) retries and succeeds, (3) asserts a new call tocheckAvailable()resolves rather than staying rejected.🤖 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/openshell-cli/openshell-gateway.spec.ts` around lines 435 - 451, Add a regression test around openshellGatewayCli.checkEndpointStatus, gateway.start(), and gateway.checkAvailable() that covers the promise re-settlement bug: first make start() fail, then retry and succeed, and finally call checkAvailable() again to verify it resolves from the new state instead of staying rejected. Keep the existing test pattern in openshell-gateway.spec.ts with mocked process/timers, but assert the second availability check uses a fresh promise after the successful retry.packages/main/src/plugin/openshell-cli/openshell-cli.spec.ts (1)
545-626: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for
checkAvailable()rejection propagation.All tests mock
openshellGateway.checkAvailableto always resolve. Given the readiness-gating logic is new and (per the companionopenshell-gateway.tsreview) has a fragile settlement design, a test asserting thatrunCli/execCLIreject/propagate whencheckAvailable()rejects would guard against regressions in this gating contract.🤖 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/openshell-cli/openshell-cli.spec.ts` around lines 545 - 626, Add a regression test for the readiness gate rejection path in openshell-cli.spec.ts: the current coverage only stubs openshellGateway.checkAvailable as resolving, so verify that runCli/execCLI propagates a rejection when checkAvailable fails. Use the existing openshellCli test setup and mock openshellGateway.checkAvailable to reject, then assert the CLI call rejects with the same error so the new gating contract is protected.
🤖 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/index.ts`:
- Around line 929-931: `workspaceProjectManager.init()` is currently
fire-and-forget in `plugin/index.ts`, which can leave the
`workspace-project-manager:*` IPC handlers unregistered during startup. Update
the startup flow around `workspaceProjectManager.init()` to await its completion
before proceeding, using the `WorkspaceProjectManager` initialization path so
the handlers are available before any early workspace-project calls.
In `@packages/main/src/plugin/openshell-cli/openshell-cli-base.ts`:
- Around line 91-109: The error handling in runCli duplicates the same
exec/catch/extractCliError/log/rethrow flow used elsewhere, so factor that
pattern into a shared protected helper in OpenshellCliBase. Have the helper wrap
this.exec.exec, normalize errors through extractCliError, and either return the
raw result or throw a normalized Error so both runCli, execCLI, and subclasses
like getGatewayStatus can reuse it. Keep runCli focused on invoking the helper
and handling quiet/redacted logging only.
In `@packages/main/src/plugin/openshell-cli/openshell-cli.ts`:
- Around line 19-38: The import block in OpenshellCli mixes alias and
sibling-module imports, and OpenshellGateway should follow the same
relative-import convention as OpenshellCliBase and OpenshellGatewayCli. Update
the OpenshellGateway import in OpenshellCli to use a relative path to the
sibling openshell-gateway module, keeping the rest of the imports unchanged and
consistent with the local-directory import style.
In `@packages/main/src/plugin/openshell-cli/openshell-gateway-cli.ts`:
- Around line 83-94: checkEndpointStatus is swallowing every failure path as an
unhealthy endpoint, which hides CLI/runtime bugs and leaves no trace because
runCli is called with quiet: true. Update checkEndpointStatus in
openshell-gateway-cli.ts to catch the error explicitly, log a meaningful message
with the endpoint and the thrown error before returning false, and keep the
success/failure behavior unchanged. Use the existing checkEndpointStatus and
runCli flow so only the error handling is adjusted.
- Around line 96-106: getGatewayStatus is duplicating the base-class CLI
error-handling path and skipping the shared execution logging. Update
OpenshellGatewayCli.getGatewayStatus to reuse the shared helper from
OpenshellCliBase (the same pattern used by addGateway, removeGateway,
selectGateway, and listGateways) instead of calling this.exec.exec directly, so
the status command gets the standard "Executing: ..." log and centralized
extract/log/throw behavior.
- Around line 78-81: Normalize the gateway list parsing error in
`OpenShellGatewayCLI.listGateways()` so it matches the wrapper’s existing
`Error` shape instead of leaking a raw `ZodError`. Wrap the
`z.array(GatewayInfoSchema).parse(data)` step with error handling, or switch to
`safeParse` and rethrow a descriptive `Error`, keeping the failure path
consistent with `execCLI()` and the rest of `OpenShellGatewayCLI`.
In `@packages/main/src/plugin/openshell-cli/openshell-gateway.spec.ts`:
- Line 29: The openshell-cli spec is using the `/@/` alias for
`OpenshellGatewayCli`, which is inconsistent with the sibling test import style
in this module. Update the import in `openshell-gateway.spec.ts` to use the same
relative path pattern as `openshell-cli.spec.ts`, keeping the type-only import
but switching it to the local `./openshell-gateway-cli.js` reference for
consistency.
In `@packages/main/src/plugin/openshell-cli/openshell-gateway.ts`:
- Around line 56-84: `#availablePromise` in `OpenshellGateway` is a one-shot
latch, so once `init()`/`start()` resolves or rejects it can’t represent later
retries or process crashes. Update the availability flow around `init()`,
`start()`, `checkAvailable()`, and the `onDidCliToolsChange` retry path so each
new startup attempt can reset or replace the promise before settling it, and
make the `exit`/`error` handlers mark the gateway unavailable instead of leaving
a stale resolved state. Also ensure the no-gateway/no-binary path settles
availability consistently so callers never wait forever.
- Around line 231-235: The dispose() method in openshell-gateway is triggering
Biome’s useIterableCallbackReturn warning because the callback passed to
this.#disposables.forEach returns the result of disposable.dispose(). Update the
callback to use a block body in dispose() so it does not implicitly return a
value, while keeping the existing stop() error handling and disposal loop
behavior unchanged.
- Line 30: The import for OpenshellGatewayCli in openshell-gateway.ts uses the
aliased absolute path style even though the target file is a sibling in the same
openshell-cli directory. Update that import to use the same relative import
convention used elsewhere in openshell-cli.ts, keeping the OpenshellGatewayCli
symbol and its local sibling path consistent with the rest of the module.
---
Outside diff comments:
In `@packages/main/src/plugin/openshell-cli/openshell-cli.spec.ts`:
- Around line 545-626: Add a regression test for the readiness gate rejection
path in openshell-cli.spec.ts: the current coverage only stubs
openshellGateway.checkAvailable as resolving, so verify that runCli/execCLI
propagates a rejection when checkAvailable fails. Use the existing openshellCli
test setup and mock openshellGateway.checkAvailable to reject, then assert the
CLI call rejects with the same error so the new gating contract is protected.
In `@packages/main/src/plugin/openshell-cli/openshell-gateway.spec.ts`:
- Around line 435-451: Add a regression test around
openshellGatewayCli.checkEndpointStatus, gateway.start(), and
gateway.checkAvailable() that covers the promise re-settlement bug: first make
start() fail, then retry and succeed, and finally call checkAvailable() again to
verify it resolves from the new state instead of staying rejected. Keep the
existing test pattern in openshell-gateway.spec.ts with mocked process/timers,
but assert the second availability check uses a fresh promise after the
successful retry.
🪄 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: f2566932-9c38-4ff2-8539-4d6ac3cce008
📒 Files selected for processing (11)
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/index.tspackages/main/src/plugin/openshell-cli/openshell-cli-base.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/openshell-cli/openshell-gateway-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-gateway-cli.tspackages/main/src/plugin/openshell-cli/openshell-gateway.spec.tspackages/main/src/plugin/openshell-cli/openshell-gateway.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.spec.tspackages/main/src/plugin/secret-manager/secret-manager.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: Linux
- GitHub Check: unit tests / windows-2022
- GitHub Check: linter, formatters
- GitHub Check: macOS
- GitHub Check: Windows
- GitHub Check: unit tests / macos-15
⚠️ CI failures not shown inline (4)
GitHub Actions: fullsend / dispatch _ Route: fix(openshell): gateway not initialized when extensions are started
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: fix(openshell): gateway not initialized when extensions are started
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: fix(openshell): gateway not initialized when extensions are started
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: fix(openshell): gateway not initialized when extensions are started
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 (5)
**/*.{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.spec.tspackages/main/src/plugin/index.tspackages/main/src/plugin/secret-manager/secret-manager.spec.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli-base.tspackages/main/src/plugin/openshell-cli/openshell-gateway-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-gateway-cli.tspackages/main/src/plugin/openshell-cli/openshell-gateway.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.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/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/secret-manager/secret-manager.spec.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.spec.tspackages/main/src/plugin/openshell-cli/openshell-gateway-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.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/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/index.tspackages/main/src/plugin/secret-manager/secret-manager.spec.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli-base.tspackages/main/src/plugin/openshell-cli/openshell-gateway-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-gateway-cli.tspackages/main/src/plugin/openshell-cli/openshell-gateway.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.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/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/index.tspackages/main/src/plugin/secret-manager/secret-manager.spec.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli-base.tspackages/main/src/plugin/openshell-cli/openshell-gateway-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-gateway-cli.tspackages/main/src/plugin/openshell-cli/openshell-gateway.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-gateway.spec.ts
packages/main/src/plugin/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
All major services must be registered as singletons in the Inversify DI container during initialization in the PluginSystem
Files:
packages/main/src/plugin/index.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/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/index.tspackages/main/src/plugin/secret-manager/secret-manager.spec.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli-base.tspackages/main/src/plugin/openshell-cli/openshell-gateway-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-gateway-cli.tspackages/main/src/plugin/openshell-cli/openshell-gateway.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.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/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/index.tspackages/main/src/plugin/secret-manager/secret-manager.spec.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli-base.tspackages/main/src/plugin/openshell-cli/openshell-gateway-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-gateway-cli.tspackages/main/src/plugin/openshell-cli/openshell-gateway.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.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/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/index.tspackages/main/src/plugin/secret-manager/secret-manager.spec.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli-base.tspackages/main/src/plugin/openshell-cli/openshell-gateway-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-gateway-cli.tspackages/main/src/plugin/openshell-cli/openshell-gateway.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-gateway.spec.ts
🪛 ast-grep (0.44.1)
packages/main/src/plugin/openshell-cli/openshell-cli-base.ts
[warning] 100-100: Avoid command injection
Context: this.exec.exec(cliPath, args, options?.env ? { env: options.env } : undefined)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-typescript)
[warning] 124-124: Avoid command injection
Context: this.exec.exec(cliPath, fullArgs, options)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-typescript)
🪛 Biome (2.5.1)
packages/main/src/plugin/openshell-cli/openshell-gateway-cli.spec.ts
[error] 23-23: Do not shadow the global "Proxy" property.
(lint/suspicious/noShadowRestrictedNames)
packages/main/src/plugin/openshell-cli/openshell-gateway.ts
[error] 234-234: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🪛 OpenGrep (1.23.0)
packages/main/src/plugin/openshell-cli/openshell-cli-base.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)
[ERROR] 125-125: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
packages/main/src/plugin/openshell-cli/openshell-gateway-cli.ts
[ERROR] 99-99: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (15)
packages/main/src/plugin/openshell-cli/openshell-gateway-cli.spec.ts (2)
23-23: Biome shadow-global hint is a false positive here.
import type { Proxy } from '/@/plugin/proxy.js'is a type-only import erased at compile time; it doesn't create a runtime shadow of the globalProxyconstructor. Safe to ignore the linter hint.
20-20: Spec follows Vitest conventions correctly.Uses
test()instead ofit(),vi.mock(import(...))for auto-mocking, andvi.resetAllMocks()inbeforeEachper the repository's testing guidelines.As per coding guidelines: "Use
test()instead ofit()for test cases in Vitest unit tests", "Usevi.mock(import('...'))for auto-mocking modules in unit tests", and "Usevi.resetAllMocks()inbeforeEachhooks instead ofvi.clearAllMocks()".Also applies to: 29-29, 55-61
Source: Coding guidelines
packages/main/src/plugin/openshell-cli/openshell-cli-base.ts (2)
111-119: LGTM!
91-109: 🔒 Security & PrivacyNo command-injection issue here.
packages/main/src/plugin/openshell-cli/openshell-gateway-cli.ts (1)
48-76: LGTM!packages/main/src/plugin/index.ts (1)
603-606: DI registration forOpenshellGatewayClifollows established singleton pattern.Consistent with existing bindings and registered before
OpenshellCli/OpenshellGateway, which now depend on it.As per coding guidelines: "All major services must be registered as singletons in the Inversify DI container during initialization in the PluginSystem."
Source: Coding guidelines
packages/main/src/plugin/secret-manager/openshell-secret-adapter.spec.ts (1)
23-24: 🎯 Functional CorrectnessSame stub-coverage concern as the agent-workspace-manager spec.
{} as OpenshellGatewayCli/{} as OpenshellGatewaystubs will throw ifOpenshellSecretAdapterexercisesOpenshellClimethods that aren't separately mocked (e.g., viavi.spyOn).Also applies to: 33-38
packages/main/src/plugin/secret-manager/secret-manager.spec.ts (1)
26-27: 🎯 Functional CorrectnessSame stub-coverage concern noted in sibling spec files.
{} as OpenshellGatewayCli/{} as OpenshellGatewaystubs will throw if any exercised path invokes their real methods without mocking.Also applies to: 48-54
packages/main/src/plugin/openshell-cli/openshell-cli.ts (3)
74-103: 🎯 Functional CorrectnessGating is correctly wired, but depends on a fragile availability signal in
OpenshellGateway.The constructor injection and
runCli/execCLIoverrides correctly implement the PR's readiness-gating objective. However, this design's correctness hinges entirely onOpenshellGateway.checkAvailable()reliably reflecting current gateway state. See the critical finding inopenshell-gateway.ts(constructor/init/start): the underlying#availablePromiseis a one-shotPromise.withResolvers()that can only settle once, so if the gateway fails once, every subsequentrunCli/execCLIcall here will reject/hang forever even after the gateway later recovers.
209-220: LGTM!
240-294: 🗄️ Data Integrity & IntegrationNo remaining callers on
OpenshellCli
The gateway methods are only used throughOpenshellGatewayCli; no call sites still invoke them onOpenshellCli.packages/main/src/plugin/openshell-cli/openshell-cli.spec.ts (1)
31-53: LGTM!Also applies to: 74-75
packages/main/src/plugin/openshell-cli/openshell-gateway.ts (1)
112-129: LGTM!Also applies to: 311-340
packages/main/src/plugin/openshell-cli/openshell-gateway.spec.ts (1)
63-73: LGTM!Also applies to: 89-246, 268-340, 363-523
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts (1)
42-43: 🎯 Functional CorrectnessNo issue here:
OpenshellCliis auto-mocked.vi.mock(import('/@/plugin/openshell-cli/openshell-cli.js'))replaces the class constructor and instance methods, so the empty constructor arguments in this spec do not fall through to the realcheckAvailable/listGatewaysimplementation.> Likely an incorrect or invalid review comment.
| const workspaceProjectManager = container.get<WorkspaceProjectManager>(WorkspaceProjectManager); | ||
| await workspaceProjectManager.init(); | ||
| workspaceProjectManager.init().catch(console.error); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect WorkspaceProjectManager.init() to check for consumers assuming synchronous completion
ast-grep run --pattern 'class WorkspaceProjectManager {
$$$
}' --lang typescript $(fd -type f 'workspace-project-manager.ts')
rg -nP -C3 '\bworkspaceProjectManager\b' packages/main/src/plugin/index.tsRepository: openkaiden/kaiden
Length of output: 38829
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the surrounding startup sequence in the plugin system
sed -n '880,980p' packages/main/src/plugin/index.ts | cat -n
printf '\n--- workspace project manager references ---\n'
rg -n --hidden -S '\bWorkspaceProjectManager\b|workspace-project-manager:' packages/main/src | cat
printf '\n--- extension loader start references ---\n'
rg -n -C 3 'extensionLoader\.start\(\)|class ExtensionLoader|start\(' packages/main/src/plugin/index.ts packages/main/src | catRepository: openkaiden/kaiden
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the init behavior and any dependencies that could observe it before completion
sed -n '1,220p' packages/main/src/plugin/workspace-project/workspace-project-manager.ts | cat -n
printf '\n--- startup call sites for workspace project APIs ---\n'
rg -n -C 2 'workspace-project-manager:(list|get|create|remove|update|analyze|clone-and-analyze)|workspaceProjectManager\.(list|get|create|remove|update|analyze|cloneAndAnalyze|init)\(' packages/main/src packages/renderer/src packages/preload/src | catRepository: openkaiden/kaiden
Length of output: 18367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact extension startup order near workspace project manager init
sed -n '920,980p' packages/main/src/plugin/index.ts | cat -n
# Read the workspace project manager init and dependent methods in smaller slices
sed -n '1,140p' packages/main/src/plugin/workspace-project/workspace-project-manager.ts | cat -n
sed -n '300,390p' packages/main/src/plugin/workspace-project/workspace-project-manager.ts | cat -nRepository: openkaiden/kaiden
Length of output: 13278
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all call sites that could observe workspace projects during startup
rg -n -C 2 'workspace-project-manager:(list|get|create|remove|update|analyze|clone-and-analyze)' packages/main/src packages/renderer/src packages/preload/src | cat
printf '\n--- plugin startup order ---\n'
sed -n '900,980p' packages/main/src/plugin/index.ts | cat -nRepository: openkaiden/kaiden
Length of output: 12545
Keep workspaceProjectManager.init() awaited
packages/main/src/plugin/index.ts:929-931 — init() also registers the workspace-project-manager:* IPC handlers after loading from disk. Fire-and-forget leaves a window where those handlers are unavailable, so early workspace-project calls can fail or see no data.
🤖 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/index.ts` around lines 929 - 931,
`workspaceProjectManager.init()` is currently fire-and-forget in
`plugin/index.ts`, which can leave the `workspace-project-manager:*` IPC
handlers unregistered during startup. Update the startup flow around
`workspaceProjectManager.init()` to await its completion before proceeding,
using the `WorkspaceProjectManager` initialization path so the handlers are
available before any early workspace-project calls.
| protected async runCli( | ||
| args: string[], | ||
| options?: { redact?: boolean; env?: { [p: string]: string }; quiet?: boolean }, | ||
| ): Promise<void> { | ||
| const cliPath = this.getCliPath(); | ||
| const displayArgs = options?.redact ? this.redactSensitiveArgs(args) : args; | ||
| if (!options?.quiet) { | ||
| console.log(`Executing: ${cliPath} ${displayArgs.join(' ')}`); | ||
| } | ||
| try { | ||
| await this.exec.exec(cliPath, args, options?.env ? { env: options.env } : undefined); | ||
| } catch (err: unknown) { | ||
| const detail = this.extractCliError(err); | ||
| if (!options?.quiet) { | ||
| console.error(`openshell failed: ${cliPath} ${displayArgs.join(' ')} — ${detail}`); | ||
| } | ||
| throw new Error(detail); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate try/catch/error-extraction logic across runCli and execCLI.
Both methods repeat the pattern: call this.exec.exec, catch, call extractCliError, log, and re-throw. A subsequent subclass method (getGatewayStatus in OpenshellGatewayCli) duplicates this pattern a third time outside the base class entirely. Consider extracting a shared protected helper that returns the raw RunResult (or throws a normalized Error), which both execCLI and future stdout-returning helpers like getGatewayStatus could reuse.
♻️ Proposed shared helper
+ protected async runAndCapture(args: string[], options?: RunOptions): Promise<RunResult> {
+ const cliPath = this.getCliPath();
+ try {
+ return await this.exec.exec(cliPath, args, options);
+ } catch (err: unknown) {
+ const detail = this.extractCliError(err);
+ console.error(`openshell failed: ${cliPath} ${args.join(' ')} — ${detail}`);
+ throw new Error(detail);
+ }
+ }
+
protected async execCLI<T>(args: string[], options?: RunOptions): Promise<T> {
- const cliPath = this.getCliPath();
const fullArgs = [...args, '-o', 'json'];
- try {
- const result = await this.exec.exec(cliPath, fullArgs, options);
- return JSON.parse(result.stdout) as T;
- } catch (err: unknown) {
- const detail = this.extractCliError(err);
- console.error(`openshell failed: ${cliPath} ${fullArgs.join(' ')} — ${detail}`);
- throw new Error(detail);
- }
+ const result = await this.runAndCapture(fullArgs, options);
+ return JSON.parse(result.stdout) as T;
}Also applies to: 121-132
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 100-100: Avoid command injection
Context: this.exec.exec(cliPath, args, options?.env ? { env: options.env } : undefined)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-typescript)
🪛 OpenGrep (1.23.0)
[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)
🤖 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/openshell-cli/openshell-cli-base.ts` around lines 91
- 109, The error handling in runCli duplicates the same
exec/catch/extractCliError/log/rethrow flow used elsewhere, so factor that
pattern into a shared protected helper in OpenshellCliBase. Have the helper wrap
this.exec.exec, normalize errors through extractCliError, and either return the
raw result or throw a normalized Error so both runCli, execCLI, and subclasses
like getGatewayStatus can reuse it. Keep runCli focused on invoking the helper
and handling quiet/redacted logging only.
| import { RunOptions } from '@openkaiden/api'; | ||
| import { inject, injectable } from 'inversify'; | ||
| import z from 'zod'; | ||
|
|
||
| import { CliToolRegistry } from '/@/plugin/cli-tool-registry.js'; | ||
| import { OpenshellGateway } from '/@/plugin/openshell-cli/openshell-gateway.js'; | ||
| import { Exec } from '/@/plugin/util/exec.js'; | ||
| import type { | ||
| import { | ||
| CreateProviderOptions, | ||
| CreateSandboxOptions, | ||
| GatewayAddOptions, | ||
| GatewayInfo, | ||
| GatewaySandboxes, | ||
| OpenshellProviderInfo, | ||
| OpenshellProviderInfoSchema, | ||
| SandboxInfo, | ||
| SandboxInfoSchema, | ||
| SetInferenceOptions, | ||
| } from '/@api/openshell-gateway-info.js'; | ||
| import { GatewayInfoSchema, OpenshellProviderInfoSchema, SandboxInfoSchema } from '/@api/openshell-gateway-info.js'; | ||
|
|
||
| import { OpenshellCliBase } from './openshell-cli-base.js'; | ||
| import { OpenshellGatewayCli } from './openshell-gateway-cli.js'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Inconsistent import style: OpenshellGateway should use a relative import.
openshell-gateway.ts is a sibling file within the same openshell-cli/ directory (like OpenshellCliBase and OpenshellGatewayCli, which correctly use relative imports on lines 37-38), yet it's imported via the /@/ alias on line 24. This is inconsistent with the sibling-module convention applied elsewhere in the same import block.
♻️ Proposed fix
-import { OpenshellGateway } from '/@/plugin/openshell-cli/openshell-gateway.js';
+import { OpenshellGateway } from './openshell-gateway.js';As per coding guidelines, **/*.{ts,tsx,js,jsx}: "use relative imports only for sibling modules within the same directory."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { RunOptions } from '@openkaiden/api'; | |
| import { inject, injectable } from 'inversify'; | |
| import z from 'zod'; | |
| import { CliToolRegistry } from '/@/plugin/cli-tool-registry.js'; | |
| import { OpenshellGateway } from '/@/plugin/openshell-cli/openshell-gateway.js'; | |
| import { Exec } from '/@/plugin/util/exec.js'; | |
| import type { | |
| import { | |
| CreateProviderOptions, | |
| CreateSandboxOptions, | |
| GatewayAddOptions, | |
| GatewayInfo, | |
| GatewaySandboxes, | |
| OpenshellProviderInfo, | |
| OpenshellProviderInfoSchema, | |
| SandboxInfo, | |
| SandboxInfoSchema, | |
| SetInferenceOptions, | |
| } from '/@api/openshell-gateway-info.js'; | |
| import { GatewayInfoSchema, OpenshellProviderInfoSchema, SandboxInfoSchema } from '/@api/openshell-gateway-info.js'; | |
| import { OpenshellCliBase } from './openshell-cli-base.js'; | |
| import { OpenshellGatewayCli } from './openshell-gateway-cli.js'; | |
| import { RunOptions } from '`@openkaiden/api`'; | |
| import { inject, injectable } from 'inversify'; | |
| import z from 'zod'; | |
| import { CliToolRegistry } from '/@/plugin/cli-tool-registry.js'; | |
| import { OpenshellGateway } from './openshell-gateway.js'; | |
| import { Exec } from '/@/plugin/util/exec.js'; | |
| import { | |
| CreateProviderOptions, | |
| CreateSandboxOptions, | |
| GatewaySandboxes, | |
| OpenshellProviderInfo, | |
| OpenshellProviderInfoSchema, | |
| SandboxInfo, | |
| SandboxInfoSchema, | |
| SetInferenceOptions, | |
| } from '/@api/openshell-gateway-info.js'; | |
| import { OpenshellCliBase } from './openshell-cli-base.js'; | |
| import { OpenshellGatewayCli } from './openshell-gateway-cli.js'; |
🤖 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/openshell-cli/openshell-cli.ts` around lines 19 -
38, The import block in OpenshellCli mixes alias and sibling-module imports, and
OpenshellGateway should follow the same relative-import convention as
OpenshellCliBase and OpenshellGatewayCli. Update the OpenshellGateway import in
OpenshellCli to use a relative path to the sibling openshell-gateway module,
keeping the rest of the imports unchanged and consistent with the
local-directory import style.
Source: Coding guidelines
| async listGateways(): Promise<GatewayInfo[]> { | ||
| const data = await this.execCLI<unknown>(['gateway', 'list']); | ||
| return z.array(GatewayInfoSchema).parse(data); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file before reading it.
ast-grep outline packages/main/src/plugin/openshell-cli/openshell-gateway-cli.ts --view expanded
# Read the target file with line numbers.
cat -n packages/main/src/plugin/openshell-cli/openshell-gateway-cli.ts
# Find related CLI wrapper patterns and error normalization helpers.
rg -n "extractCliError|safeParse|ZodError|parse\\(" packages/main/src/plugin/openshell-cli -S
# Read any closely related files that define the wrapper pattern.
fd -a "openshell-.*cli.*\\.(ts|js)$" packages/main/src/plugin/openshell-cliRepository: openkaiden/kaiden
Length of output: 6314
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n packages/main/src/plugin/openshell-cli/openshell-cli-base.ts
printf '\n--- openshell-cli.ts ---\n'
cat -n packages/main/src/plugin/openshell-cli/openshell-cli.ts
printf '\n--- spec files mentioning listGateways/parse ---\n'
rg -n "listGateways|GatewayInfoSchema|safeParse|parse\\(" packages/main/src/plugin/openshell-cli -SRepository: openkaiden/kaiden
Length of output: 21264
Normalize schema parse failures here. execCLI() already turns CLI/JSON issues into Error, but z.array(GatewayInfoSchema).parse(data) still throws a raw ZodError if the CLI output shape drifts. Catch that or use safeParse so gateway listing follows the same error shape as the rest of the wrapper.
🤖 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/openshell-cli/openshell-gateway-cli.ts` around lines
78 - 81, Normalize the gateway list parsing error in
`OpenShellGatewayCLI.listGateways()` so it matches the wrapper’s existing
`Error` shape instead of leaking a raw `ZodError`. Wrap the
`z.array(GatewayInfoSchema).parse(data)` step with error handling, or switch to
`safeParse` and rethrow a descriptive `Error`, keeping the failure path
consistent with `execCLI()` and the rest of `OpenShellGatewayCLI`.
| async checkEndpointStatus(endpoint: string): Promise<boolean> { | ||
| const args = ['status', '--gateway-endpoint', endpoint]; | ||
| if (endpoint.startsWith('http://')) { | ||
| args.push('--gateway-insecure'); | ||
| } | ||
| try { | ||
| await this.runCli(args, { quiet: true }); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
checkEndpointStatus swallows all errors silently, including non-CLI bugs.
The bare catch { return false; } treats any thrown error — CLI failure, network issue, or a programming bug in runCli/extractCliError — identically as "endpoint unhealthy," with no logging at all (since quiet: true also suppresses runCli's own error log). This makes it hard to distinguish a genuinely unreachable gateway from an unrelated defect.
♻️ Proposed fix: log before swallowing
try {
await this.runCli(args, { quiet: true });
return true;
- } catch {
+ } catch (err: unknown) {
+ console.debug(`openshell endpoint check failed for ${endpoint}: ${this.extractCliError(err)}`);
return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async checkEndpointStatus(endpoint: string): Promise<boolean> { | |
| const args = ['status', '--gateway-endpoint', endpoint]; | |
| if (endpoint.startsWith('http://')) { | |
| args.push('--gateway-insecure'); | |
| } | |
| try { | |
| await this.runCli(args, { quiet: true }); | |
| return true; | |
| } catch { | |
| return false; | |
| } | |
| } | |
| async checkEndpointStatus(endpoint: string): Promise<boolean> { | |
| const args = ['status', '--gateway-endpoint', endpoint]; | |
| if (endpoint.startsWith('http://')) { | |
| args.push('--gateway-insecure'); | |
| } | |
| try { | |
| await this.runCli(args, { quiet: true }); | |
| return true; | |
| } catch (err: unknown) { | |
| console.debug(`openshell endpoint check failed for ${endpoint}: ${this.extractCliError(err)}`); | |
| return false; | |
| } | |
| } |
🤖 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/openshell-cli/openshell-gateway-cli.ts` around lines
83 - 94, checkEndpointStatus is swallowing every failure path as an unhealthy
endpoint, which hides CLI/runtime bugs and leaves no trace because runCli is
called with quiet: true. Update checkEndpointStatus in openshell-gateway-cli.ts
to catch the error explicitly, log a meaningful message with the endpoint and
the thrown error before returning false, and keep the success/failure behavior
unchanged. Use the existing checkEndpointStatus and runCli flow so only the
error handling is adjusted.
| async getGatewayStatus(): Promise<string> { | ||
| const cliPath = this.getCliPath(); | ||
| try { | ||
| const result = await this.exec.exec(cliPath, ['status']); | ||
| return result.stdout.trim(); | ||
| } catch (err: unknown) { | ||
| const detail = this.extractCliError(err); | ||
| console.error(`openshell failed: ${cliPath} status — ${detail}`); | ||
| throw new Error(detail); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
getGatewayStatus bypasses the shared base-class helpers, duplicating error handling.
Unlike addGateway/removeGateway/selectGateway (which use runCli) and listGateways (which uses execCLI), getGatewayStatus calls this.exec.exec directly and re-implements the extract/log/throw pattern from OpenshellCliBase. This also skips the "Executing: ..." log line that runCli emits, so status invocations are less observable than other gateway commands.
♻️ Proposed fix reusing a shared base helper (see companion suggestion in openshell-cli-base.ts)
async getGatewayStatus(): Promise<string> {
- const cliPath = this.getCliPath();
- try {
- const result = await this.exec.exec(cliPath, ['status']);
- return result.stdout.trim();
- } catch (err: unknown) {
- const detail = this.extractCliError(err);
- console.error(`openshell failed: ${cliPath} status — ${detail}`);
- throw new Error(detail);
- }
+ const result = await this.runAndCapture(['status']);
+ return result.stdout.trim();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async getGatewayStatus(): Promise<string> { | |
| const cliPath = this.getCliPath(); | |
| try { | |
| const result = await this.exec.exec(cliPath, ['status']); | |
| return result.stdout.trim(); | |
| } catch (err: unknown) { | |
| const detail = this.extractCliError(err); | |
| console.error(`openshell failed: ${cliPath} status — ${detail}`); | |
| throw new Error(detail); | |
| } | |
| } | |
| async getGatewayStatus(): Promise<string> { | |
| const result = await this.runAndCapture(['status']); | |
| return result.stdout.trim(); | |
| } |
🧰 Tools
🪛 OpenGrep (1.23.0)
[ERROR] 99-99: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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/openshell-cli/openshell-gateway-cli.ts` around lines
96 - 106, getGatewayStatus is duplicating the base-class CLI error-handling path
and skipping the shared execution logging. Update
OpenshellGatewayCli.getGatewayStatus to reuse the shared helper from
OpenshellCliBase (the same pattern used by addGateway, removeGateway,
selectGateway, and listGateways) instead of calling this.exec.exec directly, so
the status command gets the standard "Executing: ..." log and centralized
extract/log/throw behavior.
| import type { CliToolRegistry } from '/@/plugin/cli-tool-registry.js'; | ||
| import type { Directories } from '/@/plugin/directories.js'; | ||
| import type { OpenshellCli } from '/@/plugin/openshell-cli/openshell-cli.js'; | ||
| import type { OpenshellGatewayCli } from '/@/plugin/openshell-cli/openshell-gateway-cli.js'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Same sibling-import inconsistency as production code.
OpenshellGatewayCli is imported via the /@/ alias here, whereas openshell-cli.spec.ts correctly uses a relative import (./openshell-gateway-cli.js) for the same type. For consistency within the openshell-cli/ module group, prefer the relative path.
♻️ Proposed fix
-import type { OpenshellGatewayCli } from '/@/plugin/openshell-cli/openshell-gateway-cli.js';
+import type { OpenshellGatewayCli } from './openshell-gateway-cli.js';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import type { OpenshellGatewayCli } from '/@/plugin/openshell-cli/openshell-gateway-cli.js'; | |
| import type { OpenshellGatewayCli } from './openshell-gateway-cli.js'; |
🤖 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/openshell-cli/openshell-gateway.spec.ts` at line 29,
The openshell-cli spec is using the `/@/` alias for `OpenshellGatewayCli`, which
is inconsistent with the sibling test import style in this module. Update the
import in `openshell-gateway.spec.ts` to use the same relative path pattern as
`openshell-cli.spec.ts`, keeping the type-only import but switching it to the
local `./openshell-gateway-cli.js` reference for consistency.
Source: Coding guidelines
| import { CliToolRegistry } from '/@/plugin/cli-tool-registry.js'; | ||
| import { Directories } from '/@/plugin/directories.js'; | ||
| import { OpenshellCli } from '/@/plugin/openshell-cli/openshell-cli.js'; | ||
| import { OpenshellGatewayCli } from '/@/plugin/openshell-cli/openshell-gateway-cli.js'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Inconsistent import style: OpenshellGatewayCli should use a relative import.
Same sibling-directory inconsistency as in openshell-cli.ts — openshell-gateway-cli.ts lives in the same openshell-cli/ directory.
♻️ Proposed fix
-import { OpenshellGatewayCli } from '/@/plugin/openshell-cli/openshell-gateway-cli.js';
+import { OpenshellGatewayCli } from './openshell-gateway-cli.js';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { OpenshellGatewayCli } from '/@/plugin/openshell-cli/openshell-gateway-cli.js'; | |
| import { OpenshellGatewayCli } from './openshell-gateway-cli.js'; |
🤖 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/openshell-cli/openshell-gateway.ts` at line 30, The
import for OpenshellGatewayCli in openshell-gateway.ts uses the aliased absolute
path style even though the target file is a sibling in the same openshell-cli
directory. Update that import to use the same relative import convention used
elsewhere in openshell-cli.ts, keeping the OpenshellGatewayCli symbol and its
local sibling path consistent with the rest of the module.
Source: Coding guidelines
| #availablePromise: PromiseWithResolvers<void>; | ||
| #disposables: Disposable[] = []; | ||
|
|
||
| constructor( | ||
| @inject(CliToolRegistry) | ||
| private readonly cliToolRegistry: CliToolRegistry, | ||
| @inject(OpenshellCli) | ||
| private readonly openshellCli: OpenshellCli, | ||
| @inject(OpenshellGatewayCli) | ||
| private readonly openshellGatewayCli: OpenshellGatewayCli, | ||
| @inject(Directories) | ||
| private readonly directories: Directories, | ||
| @inject(Exec) | ||
| private readonly exec: Exec, | ||
| ) {} | ||
| ) { | ||
| this.#availablePromise = Promise.withResolvers<void>(); | ||
| this.#disposables.push(cliToolRegistry.onDidCliToolsChange(() => this.start().catch(console.error))); | ||
| } | ||
|
|
||
| async init(): Promise<void> { | ||
| try { | ||
| const gateways = await this.openshellCli.listGateways(); | ||
| const gateways = await this.openshellGatewayCli.listGateways(); | ||
| const localGateways = gateways.filter(gw => gw.type === 'local' || this.isLocalEndpoint(gw.endpoint)); | ||
| if (localGateways.length > 0) { | ||
| for (const gw of localGateways) { | ||
| if (await this.isEndpointHealthy(gw.endpoint)) { | ||
| if (!gw.active) { | ||
| await this.openshellCli.selectGateway(gw.name); | ||
| await this.openshellGatewayCli.selectGateway(gw.name); | ||
| } | ||
| console.log(`[openshell-gateway] gateway detected (${gw.endpoint}) and is healthy`); | ||
| this.#availablePromise.resolve(undefined); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Critical: #availablePromise is a one-shot latch that can permanently break gateway availability after the first settle.
#availablePromise is created exactly once in the constructor (line 69) via Promise.withResolvers<void>(). Per Promise semantics, once a promise settles, further resolve()/reject() calls are no-ops. This creates two concrete failure modes:
- Permanent unavailability after one failure: if
start()fails once (line 187reject),#availablePromiseis rejected forever. If the gateway later starts successfully (e.g., triggered again viaonDidCliToolsChange→start()once the CLI tool is registered), the subsequentthis.#availablePromise.resolve(undefined)at line 193 is a silent no-op —checkAvailable()(and therefore everyOpenshellCli.runCli/execCLIcall) will reject forever for the rest of the app session, even though the gateway is actually healthy. - Stale "available" after crash: conversely, once resolved successfully (lines 84/104/193), a later gateway process crash (the
exit/errorhandlers only clear#gatewayProcess) does not un-resolve#availablePromise—checkAvailable()will keep reporting the gateway as available even though it is down. - Indefinite pending: in
init(), when no local gateway is healthy andgetGatewayBinaryPath()returnsundefined(lines 95-99), the method returns without ever resolving or rejecting#availablePromise. Any caller ofcheckAvailable()at that point blocks until (if ever) a lateronDidCliToolsChangeevent triggers astart()that settles it.
Since #disposables.push(cliToolRegistry.onDidCliToolsChange(() => this.start()...)) implies start() is expected to be retried multiple times over the app's lifetime, the availability signal needs to reflect the current attempt, not just the first-ever outcome.
🔒 Suggested direction: reset the promise when retrying after a settled state
`#availablePromise`: PromiseWithResolvers<void>;
+ `#availableSettled` = false;
`#disposables`: Disposable[] = [];
constructor(...) {
this.#availablePromise = Promise.withResolvers<void>();
...
}
+
+ private resetAvailabilityIfSettled(): void {
+ if (this.#availableSettled) {
+ this.#availablePromise = Promise.withResolvers<void>();
+ this.#availableSettled = false;
+ }
+ }
+
+ private markAvailable(): void {
+ this.#availablePromise.resolve(undefined);
+ this.#availableSettled = true;
+ }
+
+ private markUnavailable(err: unknown): void {
+ this.#availablePromise.reject(err);
+ this.#availableSettled = true;
+ }Call resetAvailabilityIfSettled() at the top of init()/start(), and replace direct resolve/reject calls with markAvailable()/markUnavailable(). Also consider un-resolving (or exposing a separate live-health check) when the process exits unexpectedly, so checkAvailable() doesn't report stale success after a crash.
Also applies to: 101-110, 187-194, 223-225
🤖 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/openshell-cli/openshell-gateway.ts` around lines 56
- 84, `#availablePromise` in `OpenshellGateway` is a one-shot latch, so once
`init()`/`start()` resolves or rejects it can’t represent later retries or
process crashes. Update the availability flow around `init()`, `start()`,
`checkAvailable()`, and the `onDidCliToolsChange` retry path so each new startup
attempt can reset or replace the promise before settling it, and make the
`exit`/`error` handlers mark the gateway unavailable instead of leaving a stale
resolved state. Also ensure the no-gateway/no-binary path settles availability
consistently so callers never wait forever.
| console.log('[openshell-gateway] already running, skipping start'); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Add this.#availablePromise = Promise.withResolvers<void>(); Here so that it resets the promise so each attempt gets a fresh one or else checks will be stale
|
Closing as invalid |
2 participants
Summary
OpenshellCliBaseabstract class with shared CLI plumbing (getCliPath,runCli,execCLI, error handling)addGateway,removeGateway,selectGateway,listGateways,checkEndpointStatus,getGatewayStatus) into newOpenshellGatewayCliclassOpenshellClinow extends the base and gates CLI access behindOpenshellGateway.checkAvailable(), ensuring the gateway is initialized before extensions invoke CLI operationsOpenshellGatewaydepends onOpenshellGatewayCliinstead of the fullOpenshellCli, breaking the circular initialization dependencyFixes #2371
Test plan
pnpm run typecheck:main)pnpm run lint:check)openshell-gateway-cli.spec.ts(16 tests)openshell-gateway.spec.tsfixed🤖 Generated with Claude Code