Skip to content

fix(openshell): use extension assets folder for bundled binaries in dev mode - #2486

Merged
benoitf merged 1 commit into
openkaiden:mainfrom
benoitf:KAIDEN-2485
Jul 20, 2026
Merged

benoitf merged 1 commit into
openkaiden:mainfrom
benoitf:KAIDEN-2485

Conversation

@benoitf

@benoitf benoitf commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

fixes #2485

In development mode, process.resourcesPath is not set, so bundled OpenShell binaries were never found and the system PATH was used instead. Now checks the extension's assets/-/ folder when not in production, matching the layout created by download scripts.

note: tsconfig.json is updated to allow the usage of if (import.meta.env.PROD)

@benoitf
benoitf requested a review from a team as a code owner July 17, 2026 16:24
@benoitf
benoitf requested review from bmahabirbu and fbricon and removed request for a team July 17, 2026 16:24
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

OpenShell bundled-binary discovery now uses extension assets in development and process.resourcesPath in production, including a separate image-builder assets subdirectory. Tests cover path resolution, priority, fallback, version checks, and registration. TypeScript compilation includes shared declarations.

Changes

OpenShell bundled discovery

Layer / File(s) Summary
Environment-specific bundled path resolution
extensions/openshell/src/manager/openshell-cli-manager.ts
Production resolves binaries under process.resourcesPath; development resolves them under assets/<platform>-<arch>, with image-builder binaries under assets/image-builder/<platform>-<arch>.
Discovery test coverage and registration validation
extensions/openshell/src/manager/openshell-cli-manager.spec.ts
Specs cover bundled-path expectations, production and development resolution, priority and fallback behavior, version checks, image-builder metadata, and gateway registration assertions.
Shared declaration compilation
extensions/openshell/tsconfig.json
The TypeScript project includes shared declaration files from ../../types/*.d.ts.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant OpenshellCliManager
  participant RuntimeEnvironment
  participant BundledBinary
  OpenshellCliManager->>RuntimeEnvironment: Read PROD, platform, and architecture
  alt Production mode
    OpenshellCliManager->>BundledBinary: Check process.resourcesPath bundled path
  else Development mode
    OpenshellCliManager->>BundledBinary: Check assets platform-architecture path
    OpenshellCliManager->>BundledBinary: Run --version
  end
Loading

Possibly related PRs

  • openkaiden/kaiden#2020: Refactors OpenShell binary discovery and registration logic used by this path-resolution flow.
  • openkaiden/kaiden#2310: Modifies the bundled-resource discovery and priority flow used by these changes.
  • openkaiden/kaiden#2333: Adds gateway CLI discovery and registration through the shared OpenshellCliManager pipeline.

Suggested reviewers: fbricon, bmahabirbu, gastoner

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes address #2485 by reusing bundled assets in development mode instead of falling back to PATH.
Out of Scope Changes check ✅ Passed No clearly unrelated changes stand out; the tsconfig update supports the new import.meta.env.PROD usage.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title clearly describes the main change: using extension assets for bundled binaries in development mode.
Description check ✅ Passed The description matches the code changes and explains the dev-mode bundled binary lookup update.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Comment thread extensions/openshell/src/manager/openshell-cli-manager.spec.ts Outdated
Comment thread extensions/openshell/src/manager/openshell-cli-manager.ts

@jeffmaury jeffmaury left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typecheck errors to be fixed and image-builder is not correctly processed

@benoitf

benoitf commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

@jeffmaury / @bmahabirbu this is fixed

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.spec.ts`:
- Around line 215-239: Update the test uses process.resourcesPath with original
subdir in production mode so cleanup runs in a finally block regardless of
assertion or initialization failures. Keep the existing test setup and
assertions in the try section, and restore process.resourcesPath and call
vi.unstubAllEnvs() from finally.
🪄 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: 5012bd1c-b8a9-427b-9001-61ca7f7f6ed4

📥 Commits

Reviewing files that changed from the base of the PR and between 925eca9 and a1e36b4.

📒 Files selected for processing (3)
  • extensions/openshell/src/manager/openshell-cli-manager.spec.ts
  • extensions/openshell/src/manager/openshell-cli-manager.ts
  • extensions/openshell/tsconfig.json
📜 Review details
🧰 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:

  • extensions/openshell/src/manager/openshell-cli-manager.ts
  • extensions/openshell/src/manager/openshell-cli-manager.spec.ts
extensions/*/src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Register inference, container, and Kubernetes providers through the ProviderRegistry via extension APIs

Files:

  • extensions/openshell/src/manager/openshell-cli-manager.ts
  • extensions/openshell/src/manager/openshell-cli-manager.spec.ts
**/*.spec.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.spec.{ts,tsx,js,jsx}: Use test() instead of it() for test cases in Vitest unit tests
Use vi.mock(import('...')) for auto-mocking modules in unit tests; avoid manual mock factories when possible
Use vi.resetAllMocks() in beforeEach hooks instead of vi.clearAllMocks() for resetting mocks between tests
When an auto-mocked function or class method needs a real implementation, use vi.mocked(...) with the prototype pattern for class methods: vi.mocked(MyClass.prototype.myMethod).mockImplementation(...)

Files:

  • extensions/openshell/src/manager/openshell-cli-manager.spec.ts
🧠 Learnings (4)
📚 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
  • extensions/openshell/src/manager/openshell-cli-manager.spec.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
  • extensions/openshell/src/manager/openshell-cli-manager.spec.ts
📚 Learning: 2026-05-12T17:14:02.153Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1850
File: packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte:66-70
Timestamp: 2026-05-12T17:14:02.153Z
Learning: When reviewing code that uses `AgentWorkspaceSummaryUI.runtime`, treat it as a required, non-null `string` per the `openkaiden/kdn-api` 0.12.0 schema. Therefore, code like `a.runtime.localeCompare(b.runtime)` is safe and should not trigger warnings about possible `undefined`/`null` values or suggestions to use nullish coalescing/optional chaining for `runtime` (unless the current local types still mark `runtime` as optional, indicating a schema/version mismatch).

Applied to files:

  • extensions/openshell/src/manager/openshell-cli-manager.ts
  • extensions/openshell/src/manager/openshell-cli-manager.spec.ts
📚 Learning: 2026-06-29T13:16:53.102Z
Learnt from: benoitf
Repo: openkaiden/kaiden PR: 2296
File: extensions/container/packages/extension/src/helper/socket-finder/_socket-finder-module.ts:28-29
Timestamp: 2026-06-29T13:16:53.102Z
Learning: When reviewing imports in openkaiden/kaiden TypeScript/JavaScript files, prefer the configured `/@/` path alias instead of relative imports that would require traversing out of the current directory/module group (i.e., paths containing `..` that cross boundaries). 

Do not require alias conversion for descendant-path relative imports within the socket-finder module directory—for example, in `extensions/container/packages/extension/src/helper/socket-finder/**`, imports like `./podman/podman-version-detector` and `./podman/podman-windows-finder` are acceptable and should not be flagged.

Applied to files:

  • extensions/openshell/src/manager/openshell-cli-manager.ts
  • extensions/openshell/src/manager/openshell-cli-manager.spec.ts
🔇 Additional comments (4)
extensions/openshell/tsconfig.json (1)

21-21: LGTM!

extensions/openshell/src/manager/openshell-cli-manager.ts (1)

73-76: LGTM!

Also applies to: 139-143, 171-172, 203-220

extensions/openshell/src/manager/openshell-cli-manager.spec.ts (2)

24-24: LGTM!

Also applies to: 85-89, 141-146, 178-182, 241-261, 263-287


215-239: 🎯 Functional Correctness

No issue: vi.stubEnv('PROD', true) matches Vitest’s boolean typing for PROD.

			> Likely an incorrect or invalid review comment.

Comment thread extensions/openshell/src/manager/openshell-cli-manager.spec.ts
@benoitf
benoitf marked this pull request as draft July 20, 2026 09:29
…ev mode (#2485)

In development mode, process.resourcesPath is not set, so bundled
OpenShell binaries were never found and the system PATH was used
instead. Now checks the extension's assets/<platform>-<arch>/ folder
when not in production, matching the layout created by download scripts.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Florent Benoit <fbenoit@redhat.com>
@benoitf
benoitf marked this pull request as ready for review July 20, 2026 10:49
@benoitf
benoitf merged commit 6dbe33c into openkaiden:main Jul 20, 2026
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OpenShell binaries assets are not registered in development mode

3 participants