feat(openshell-gateway): pin supervisor container image to gateway binary version#2346
Conversation
📝 WalkthroughWalkthroughAdds an optional gateway start option and generates a local TOML config during startup to pin the supervisor image to the gateway version, with fallback behavior when version detection or config writing fails. ChangesSupervisor image pinning
Estimated code review effort: 4 (Complex) | ~45 minutes 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 |
gastoner
force-pushed
the
gateway_image_version_pin
branch
from
7a6fc97 to
1cba3f3
Compare
gastoner
requested review from
benoitf and
fbricon
and removed request for
a team
gastoner
force-pushed
the
gateway_image_version_pin
branch
from
1cba3f3 to
ee6e9b9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/main/src/plugin/openshell-cli/openshell-gateway.ts (1)
135-156: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winNew TOCTOU race on the
#gatewayProcessguard.Before this change
start()ran synchronously from the top-level guard check through tothis.#gatewayProcess = spawn(...), so no context switch could occur in between. Insertingawait this.writeSupervisorConfig(...)(Line 155) now yields to the event loop before#gatewayProcessis assigned. A second concurrentstart()call (e.g. triggered again frominit()/an IPC restart action) would pass the sameif (this.#gatewayProcess) returnguard at Lines 135-138 since the field is stillundefined, resulting in duplicate spawned processes, port conflicts, and a leaked/unreferenced child process.🔒 Proposed fix: add a re-entrancy guard around the async startup window
export class OpenshellGateway implements Disposable { `#gatewayProcess`: ChildProcess | undefined; `#port`: number = DEFAULT_PORT; `#bindAddress`: string = DEFAULT_BIND_ADDRESS; + `#starting` = false; async start(options?: OpenshellGatewayStartOptions): Promise<void> { if (this.#gatewayProcess) { console.log('[openshell-gateway] already running, skipping start'); return; } + if (this.#starting) { + console.log('[openshell-gateway] start already in progress, skipping'); + return; + } + this.#starting = true; + try { ... const configPath = await this.writeSupervisorConfig(binaryPath, options?.supervisorImage); const args = this.buildArgs(options?.disableTls ?? true, configPath); ... + } finally { + this.#starting = 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.ts` around lines 135 - 156, `openshell-gateway` has a TOCTOU race in `start()` because `await this.writeSupervisorConfig(...)` yields before `this.#gatewayProcess` is assigned, so concurrent calls can both pass the initial guard and spawn duplicate processes. Add a re-entrancy/startup-in-progress guard in `start()` around the async window, using a dedicated flag or promise that is set before any await and cleared on success/failure. Keep the existing `#gatewayProcess` check, but make it protect both the pre-await setup and the eventual `spawn` path to prevent overlapping starts.
🤖 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 548-580: The current tests in openshell-gateway.spec only cover
version-detection failures, but not the writeFile rejection branch in
writeSupervisorConfig. Add a test that makes writeFile reject after version
detection succeeds, then verify gateway.start still proceeds without --config
and spawns GATEWAY_BINARY with the non-config args. Use the existing
gateway.start, writeSupervisorConfig, writeFile, and spawn setup in this spec to
locate the right spot.
- Around line 481-581: The new supervisor image pinning tests repeat the same
console and child-process setup in each case, so extract that shared boilerplate
into a local beforeEach inside the supervisor image pinning describe block.
Initialize the console log spy, createMockChildProcess, and
vi.mocked(spawn).mockReturnValue there, then keep each test focused on its
unique exec.exec mock and assertions. Use the describe block for supervisor
image pinning and the createMockChildProcess/spawn mocks as the key symbols to
relocate.
In `@packages/main/src/plugin/openshell-cli/openshell-gateway.ts`:
- Around line 259-267: The TOML generated in openshell-gateway’s config writer
interpolates the supervisor image directly, so a caller-supplied override can
break the string or inject extra keys. Update the config-building logic around
the `image`/`content` creation in `OpenShellGateway` to escape TOML string
characters before writing `supervisor_image`, or use a proper TOML serializer
instead of manual interpolation. Keep the fix localized to the `writeFile`
config generation path so `supervisorImage` and the derived default version both
produce safe output.
- Around line 257-275: The fixed temp-file path in writeSupervisorConfig is
vulnerable because it uses a guessable shared location for kaiden-gateway.toml.
Update openshell-gateway.ts to create the supervisor config inside a unique
per-process temp directory instead of join(tmpdir(), 'kaiden-gateway.toml'), and
use that unique path for both writeFile and the later spawn/config handling in
OpenShellGateway. Also add cleanup when the gateway is stopped/disposed so the
temp directory and config file are removed, and adjust openshell-gateway.spec.ts
expectations away from the hardcoded /tmp/kaiden-gateway.toml path.
---
Outside diff comments:
In `@packages/main/src/plugin/openshell-cli/openshell-gateway.ts`:
- Around line 135-156: `openshell-gateway` has a TOCTOU race in `start()`
because `await this.writeSupervisorConfig(...)` yields before
`this.#gatewayProcess` is assigned, so concurrent calls can both pass the
initial guard and spawn duplicate processes. Add a
re-entrancy/startup-in-progress guard in `start()` around the async window,
using a dedicated flag or promise that is set before any await and cleared on
success/failure. Keep the existing `#gatewayProcess` check, but make it protect
both the pre-await setup and the eventual `spawn` path to prevent overlapping
starts.
🪄 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: 32a5c63c-c2a3-4e0f-9ac6-33e8a8b29e02
📒 Files selected for processing (3)
packages/api/src/openshell-gateway-info.tspackages/main/src/plugin/openshell-cli/openshell-gateway.spec.tspackages/main/src/plugin/openshell-cli/openshell-gateway.ts
📜 Review details
⚠️ CI failures not shown inline (4)
GitHub Actions: fullsend / dispatch _ Route: feat(openshell-gateway): pin supervisor container image to gateway binary version
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-gateway): pin supervisor container image to gateway binary version
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-gateway): pin supervisor container image to gateway binary version
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-gateway): pin supervisor container image to gateway binary version
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 (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
/@/path aliases instead of relative paths for imports outside the current directory's module group; use relative imports only for sibling modules within the same directory
Files:
packages/api/src/openshell-gateway-info.tspackages/main/src/plugin/openshell-cli/openshell-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-05-12T17:14:02.153Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1850
File: packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte:66-70
Timestamp: 2026-05-12T17:14:02.153Z
Learning: When reviewing code that uses `AgentWorkspaceSummaryUI.runtime`, treat it as a required, non-null `string` per the `openkaiden/kdn-api` 0.12.0 schema. Therefore, code like `a.runtime.localeCompare(b.runtime)` is safe and should not trigger warnings about possible `undefined`/`null` values or suggestions to use nullish coalescing/optional chaining for `runtime` (unless the current local types still mark `runtime` as optional, indicating a schema/version mismatch).
Applied to files:
packages/api/src/openshell-gateway-info.tspackages/main/src/plugin/openshell-cli/openshell-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/api/src/openshell-gateway-info.tspackages/main/src/plugin/openshell-cli/openshell-gateway.tspackages/main/src/plugin/openshell-cli/openshell-gateway.spec.ts
📚 Learning: 2026-03-09T08:47:09.657Z
Learnt from: benoitf
Repo: kortex-hub/kortex PR: 1077
File: packages/main/src/plugin/skill/skill-manager.ts:80-109
Timestamp: 2026-03-09T08:47:09.657Z
Learning: In the kortex-hub/kortex repository, IPC handlers (via ipcHandle()) may be registered directly inside feature manager/service classes (e.g., SkillManager in packages/main/src/plugin/skill/skill-manager.ts) rather than exclusively in packages/main/src/plugin/index.ts. Treat this as an accepted design pattern for files under the plugin directory. Reviewers should not require centralization in index.ts; allow IPC registration proximity to the feature that owns the handler. When reviewing code, accept direct ipcHandle() registrations inside feature managers and ensure the pattern is consistently applied across similar feature-manager modules.
Applied to files:
packages/main/src/plugin/openshell-cli/openshell-gateway.tspackages/main/src/plugin/openshell-cli/openshell-gateway.spec.ts
🪛 OpenGrep (1.23.0)
packages/main/src/plugin/openshell-cli/openshell-gateway.ts
[ERROR] 247-247: 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 (4)
packages/api/src/openshell-gateway-info.ts (1)
114-122: LGTM!packages/main/src/plugin/openshell-cli/openshell-gateway.ts (2)
21-23: LGTM!Also applies to: 38-38
233-255: LGTM!packages/main/src/plugin/openshell-cli/openshell-gateway.spec.ts (1)
35-41: LGTM!Also applies to: 78-78, 117-120, 168-171, 208-208, 240-243, 273-281, 290-298, 307-315, 324-326, 346-348, 364-366, 384-386, 427-429, 454-456, 469-471
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
might overlap with #2227 in this related PR, it's using a mustache template file so it looks more extensible approach to add new entries to the toml file I also filed an upstream PR in OpenShell so I don't know |
|
@benoit if this pr goes in I was taking a look at the config creation and for 2227 which has the mustache template architecture inline it could be worth getting that in since bind moutning will be a no op as we are using --upload for 0.3? then have this fix ontop @jeffmaury WDYT? |
no but it's still unapproved :-/ |
|
@benoitf and @bmahabirbu this one is targeted for 0.3 and bind mounts are for 0.4 |
|
yes I'm just saying it should use mustache approach there (as the one for bind mount) |
|
This pr could just copy the mustache template implementation from 2227 It wouldn't make sense to merge this as it is if we would have to rewrite it later on (Also 2227 doesn't wire the bind mounts up to the workspace creation it just enables it via a config for the gateway so its a no op since my other 0.3 pr will use --upload for mounting) |
…nary version Write a TOML config file that pins the supervisor_image to the gateway binary's own semver, preventing :latest tag drift. When version detection fails, the gateway starts without --config so behavior is unchanged. Also adds a supervisorImage option to allow explicit image overrides. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
- Update start tests to mock version detection and assert --config flag - Add comprehensive supervisor image pinning test suite (6 tests) - Remove JSDoc comment, simplify writeSupervisorConfig formatting Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
…eration Replace hardcoded TOML string with a .toml.template file rendered via Mustache. Config is now written to the Kaiden data directory instead of /tmp, and includes bind mount settings alongside supervisor image pinning. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
gastoner
force-pushed
the
gateway_image_version_pin
branch
from
ee6e9b9 to
9b87043
Compare
|
@jeffmaury @benoitf @bmahabirbu moved to mustache template |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/main/src/plugin/openshell-cli/openshell-gateway.ts (1)
247-280: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSame TOML-injection root cause as flagged on the template file.
image(which can be the caller-suppliedsupervisorImageoverride) is passed unescaped intoMustache.render, and the template renders it via triple-mustache ({{{supervisorImage}}}), which bypasses only HTML-escaping — not TOML string escaping. See the companion comment onopenshell-gateway.toml.template(Lines 6-8) for the injection scenario and proposed fix (escape"/\inimagebefore rendering, or validate the value against an image-reference pattern).🤖 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 247 - 280, The createGatewayConfig flow in openshell-gateway.ts passes supervisorImage into Mustache.render without TOML-safe escaping, so mirror the template-side fix here by sanitizing or validating the image value before rendering. Update createGatewayConfig to ensure any caller-supplied supervisorImage (and the version-derived fallback) cannot inject TOML syntax, either by escaping characters like backslash and double quote before passing image to gatewayConfigTemplate or by rejecting values that do not match a valid image-reference pattern.packages/main/src/plugin/openshell-cli/openshell-gateway.toml.template (1)
6-8: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUnescaped
supervisorImagecan break out of the TOML string (injection).
{{{supervisorImage}}}(triple-mustache) only disables Mustache's HTML-escaping — it does not escape TOML special characters. A caller-suppliedsupervisorImage(viaOpenshellGatewayStartOptions.supervisorImage, seeopenshell-gateway.tscreateGatewayConfig) containing a"or backslash/newline would break out of the quoted string and inject arbitrary keys/sections into the generatedgateway.toml. This is the same class of issue flagged on a prior version of this code (manual TOML interpolation), now re-introduced via the Mustache template path since escaping was never added to theimagevalue before rendering.Auto-derived images (from
getGatewayVersion()) are safe since that path is numerically validated, but the explicit override is not.🛡️ Proposed fix: escape TOML special characters before rendering (in openshell-gateway.ts)
- const config = Mustache.render(gatewayConfigTemplate, { - enableBindMounts: true, - supervisorImage: image, - }); + const escapedImage = image?.replace(/\\/g, '\\\\').replace(/"/g, '\\"'); + const config = Mustache.render(gatewayConfigTemplate, { + enableBindMounts: true, + supervisorImage: escapedImage, + });🤖 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.toml.template` around lines 6 - 8, The supervisorImage value is rendered into the gateway TOML with triple-mustache, so it is not escaped for TOML and can inject new keys or sections. Update createGatewayConfig in openshell-gateway.ts to sanitize or TOML-escape OpenshellGatewayStartOptions.supervisorImage before it reaches the openshell-gateway.toml.template, and keep the template as a plain quoted string reference to the already-escaped value. Make sure the fix covers quotes, backslashes, and newlines so the generated supervisor_image field cannot break out of the string.
🤖 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 451-574: The gateway config generation tests cover the main
version and fallback paths, but they still miss the injection-escaping case for
`supervisorImage`. Add a focused test in `gateway.start` coverage that passes a
value containing a double quote and asserts the rendered config from `writeFile`
still produces one valid `supervisor_image` TOML entry. Use the existing
`gateway.start`, `writeFile`, and `GATEWAY_CONFIG_PATH` symbols to locate the
right suite and keep the assertion aligned with the template/render fix.
---
Duplicate comments:
In `@packages/main/src/plugin/openshell-cli/openshell-gateway.toml.template`:
- Around line 6-8: The supervisorImage value is rendered into the gateway TOML
with triple-mustache, so it is not escaped for TOML and can inject new keys or
sections. Update createGatewayConfig in openshell-gateway.ts to sanitize or
TOML-escape OpenshellGatewayStartOptions.supervisorImage before it reaches the
openshell-gateway.toml.template, and keep the template as a plain quoted string
reference to the already-escaped value. Make sure the fix covers quotes,
backslashes, and newlines so the generated supervisor_image field cannot break
out of the string.
In `@packages/main/src/plugin/openshell-cli/openshell-gateway.ts`:
- Around line 247-280: The createGatewayConfig flow in openshell-gateway.ts
passes supervisorImage into Mustache.render without TOML-safe escaping, so
mirror the template-side fix here by sanitizing or validating the image value
before rendering. Update createGatewayConfig to ensure any caller-supplied
supervisorImage (and the version-derived fallback) cannot inject TOML syntax,
either by escaping characters like backslash and double quote before passing
image to gatewayConfigTemplate or by rejecting values that do not match a valid
image-reference pattern.
🪄 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: dc06583b-9dd2-4e0c-957f-4e1d005e23a2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
package.jsonpackages/api/src/openshell-gateway-info.tspackages/main/src/plugin/openshell-cli/openshell-gateway.spec.tspackages/main/src/plugin/openshell-cli/openshell-gateway.toml.templatepackages/main/src/plugin/openshell-cli/openshell-gateway.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: unit tests / windows-2022
- GitHub Check: linter, formatters
- GitHub Check: unit tests / macos-15
- GitHub Check: Linux
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: macOS
- GitHub Check: typecheck
- GitHub Check: Windows
⚠️ CI failures not shown inline (4)
GitHub Actions: fullsend / dispatch _ Route: feat(openshell-gateway): pin supervisor container image to gateway binary version
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-gateway): pin supervisor container image to gateway binary version
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 / dispatch _ Route: feat(openshell-gateway): pin supervisor container image to gateway binary version
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-gateway): pin supervisor container image to gateway binary version
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 (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
/@/path aliases instead of relative paths for imports outside the current directory's module group; use relative imports only for sibling modules within the same directory
Files:
packages/api/src/openshell-gateway-info.tspackages/main/src/plugin/openshell-cli/openshell-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-05-12T17:14:02.153Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1850
File: packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte:66-70
Timestamp: 2026-05-12T17:14:02.153Z
Learning: When reviewing code that uses `AgentWorkspaceSummaryUI.runtime`, treat it as a required, non-null `string` per the `openkaiden/kdn-api` 0.12.0 schema. Therefore, code like `a.runtime.localeCompare(b.runtime)` is safe and should not trigger warnings about possible `undefined`/`null` values or suggestions to use nullish coalescing/optional chaining for `runtime` (unless the current local types still mark `runtime` as optional, indicating a schema/version mismatch).
Applied to files:
packages/api/src/openshell-gateway-info.tspackages/main/src/plugin/openshell-cli/openshell-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/api/src/openshell-gateway-info.tspackages/main/src/plugin/openshell-cli/openshell-gateway.tspackages/main/src/plugin/openshell-cli/openshell-gateway.spec.ts
📚 Learning: 2026-03-09T08:47:09.657Z
Learnt from: benoitf
Repo: kortex-hub/kortex PR: 1077
File: packages/main/src/plugin/skill/skill-manager.ts:80-109
Timestamp: 2026-03-09T08:47:09.657Z
Learning: In the kortex-hub/kortex repository, IPC handlers (via ipcHandle()) may be registered directly inside feature manager/service classes (e.g., SkillManager in packages/main/src/plugin/skill/skill-manager.ts) rather than exclusively in packages/main/src/plugin/index.ts. Treat this as an accepted design pattern for files under the plugin directory. Reviewers should not require centralization in index.ts; allow IPC registration proximity to the feature that owns the handler. When reviewing code, accept direct ipcHandle() registrations inside feature managers and ensure the pattern is consistently applied across similar feature-manager modules.
Applied to files:
packages/main/src/plugin/openshell-cli/openshell-gateway.tspackages/main/src/plugin/openshell-cli/openshell-gateway.spec.ts
🪛 OpenGrep (1.23.0)
packages/main/src/plugin/openshell-cli/openshell-gateway.ts
[ERROR] 237-237: 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 (9)
packages/api/src/openshell-gateway-info.ts (2)
106-114: LGTM!
80-95: Unrelatedpolicyfield addition not covered by PR scope/summary.The
policy?: stringfield added toCreateSandboxOptions(Line 94) isn't mentioned in the PR'sai_summaryor objectives (which only describe thesupervisorImageoption). It's only referenced separately in the providedlibrary_context. Confirm this is an intentional, in-scope change and not a stray merge artifact.packages/main/src/plugin/openshell-cli/openshell-gateway.ts (5)
21-41: LGTM!
58-66: LGTM!
145-146: LGTM!Config path is generated under the app data directory (via
Directories.getDataDirectory()), which resolves the previously flagged symlink-attack concern of writing to a shared/tmppath.Also applies to: 223-234
236-245: Version parsing looks correct; static analysis hint is a false positive.
getGatewayVersion()validates the parsed semver parts before use, correctly falling back to no pinning on malformed output. Note: OpenGrep flagged Line 237 for "dynamic command passed to exec," butthis.exec.exec(binaryPath, [...])ultimately delegates to Node'sspawn(command, args)(seeexec.ts), not a shell-invoking exec — this is a false positive.
281-299: LGTM!packages/main/src/plugin/openshell-cli/openshell-gateway.spec.ts (1)
21-46: LGTM!Prior review feedback (extracting shared setup, adding a
writeFilerejection test) appears to have been addressed in thegateway config generationsuite below.Also applies to: 76-86
package.json (1)
165-165: 🗄️ Data Integrity & IntegrationNo issue:
mustacheis already declared as a dependency.> Likely an incorrect or invalid review comment.
benoitf
left a comment
benoitf
left a comment
There was a problem hiding this comment.
LGTM but it contains stuff for bind mount in the mustache template which seems unsalted to this PR
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
|
Removed bind mounts |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/main/src/plugin/openshell-cli/openshell-gateway.spec.ts (1)
520-538: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInjection-escaping coverage for
supervisorImagestill outstanding.A prior review round suggested adding a test that passes a
supervisorImagecontaining a"to verify the rendered TOML still produces a single validsupervisor_imageentry, once the template/render escaping fix lands. That suggestion isn't marked as addressed and no such test is visible in this segment.🤖 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 520 - 538, Add a test in openshell-gateway.spec.ts around gateway.start that covers a supervisorImage value containing a double quote, since the current coverage around version detection in gateway.start does not verify template escaping. Use the existing writeFile assertion pattern to confirm the rendered TOML still contains exactly one supervisor_image entry and that the injected quote is escaped rather than breaking the config. Reference the gateway.start flow and the GATEWAY_CONFIG_PATH/writeFile checks to keep the new test aligned with the existing config-generation suite.
🤖 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.
Duplicate comments:
In `@packages/main/src/plugin/openshell-cli/openshell-gateway.spec.ts`:
- Around line 520-538: Add a test in openshell-gateway.spec.ts around
gateway.start that covers a supervisorImage value containing a double quote,
since the current coverage around version detection in gateway.start does not
verify template escaping. Use the existing writeFile assertion pattern to
confirm the rendered TOML still contains exactly one supervisor_image entry and
that the injected quote is escaped rather than breaking the config. Reference
the gateway.start flow and the GATEWAY_CONFIG_PATH/writeFile checks to keep the
new test aligned with the existing config-generation suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0d368a90-c5bd-491e-98b4-b9f515fb6bbc
📒 Files selected for processing (3)
packages/main/src/plugin/openshell-cli/openshell-gateway.spec.tspackages/main/src/plugin/openshell-cli/openshell-gateway.toml.templatepackages/main/src/plugin/openshell-cli/openshell-gateway.ts
💤 Files with no reviewable changes (2)
- packages/main/src/plugin/openshell-cli/openshell-gateway.toml.template
- packages/main/src/plugin/openshell-cli/openshell-gateway.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: unit tests / windows-2022
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: linter, formatters
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: macOS
- GitHub Check: unit tests / macos-15
- GitHub Check: typecheck
⚠️ CI failures not shown inline (4)
GitHub Actions: fullsend / dispatch _ Route: feat(openshell-gateway): pin supervisor container image to gateway binary version
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-gateway): pin supervisor container image to gateway binary version
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-gateway): pin supervisor container image to gateway binary version
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-gateway): pin supervisor container image to gateway binary version
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 (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.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
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.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.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.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.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.spec.ts
🔇 Additional comments (1)
packages/main/src/plugin/openshell-cli/openshell-gateway.spec.ts (1)
461-469: LGTM!
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
4 participants
Write a TOML config file that pins the supervisor_image to the gateway binary's own semver, preventing :latest tag drift. When version detection fails, the gateway starts without --config so behavior is unchanged.
Also adds a supervisorImage option to allow explicit image overrides.
Closes #2315