fix: detect Podman container socket with Colima fallback priority#144
Closed
brianwtaylor wants to merge 2 commits intoNVIDIA:mainfrom
Closed
fix: detect Podman container socket with Colima fallback priority#144brianwtaylor wants to merge 2 commits intoNVIDIA:mainfrom
brianwtaylor wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
brianwtaylor
force-pushed
the
fix/podman-socket-detection
branch
from
30b2b09 to
0cc32c1
Compare
This was referenced Mar 17, 2026
Extract detectContainerSocket() with dependency injection and add Podman socket candidates (machine, rootless, QEMU). Colima sockets are checked first to preserve existing behavior. Add Podman fallback to isDockerRunning() with host-gateway caveat warning. Closes #116 Signed-off-by: Brian Taylor <brian@briantaylor.xyz> Signed-off-by: Brian Taylor <brian.taylor818@gmail.com>
brianwtaylor
force-pushed
the
fix/podman-socket-detection
branch
from
0cc32c1 to
83f1196
Compare
Contributor
📝 WalkthroughWalkthroughReplaces Docker-only checks with multi-runtime detection: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/lib/onboard.js (1)
25-37:⚠️ Potential issue | 🟡 MinorReturn runtime identity instead of a boolean to avoid Docker-only downstream behavior.
When Podman is detected, downstream flow still treats it as Docker (status text and Docker-specific remediation), because this helper only returns
true/false. Please return the detected runtime and branch accordingly.💡 Proposed fix
-function isDockerRunning() { +function detectContainerRuntime() { try { runCapture("docker info", { ignoreError: false }); - return true; + return "docker"; } catch { // Podman fallback — rootless Podman can substitute for Docker try { runCapture("podman info", { ignoreError: false }); console.log(" ⓘ Using Podman (note: --add-host=host-gateway may not resolve, see issue `#116`)"); - return true; + return "podman"; } catch { - return false; + return null; } } } @@ - if (!isDockerRunning()) { - console.error(" Docker is not running. Please start Docker and try again."); + const runtime = detectContainerRuntime(); + if (!runtime) { + console.error(" Neither Docker nor Podman is running. Please start a container runtime and try again."); process.exit(1); } - console.log(" ✓ Docker is running"); + console.log(` ✓ ${runtime === "podman" ? "Podman" : "Docker"} is running`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 25 - 37, The helper isDockerRunning() currently returns only a boolean causing Podman to be treated like Docker downstream; change its contract to return the detected runtime identifier (e.g., the string "docker" when runCapture("docker info", ...) succeeds, "podman" when runCapture("podman info", ...) succeeds) and return false (or null) when neither is present; update any callers to branch on the returned value instead of a boolean (use strict equality checks like returned === "podman" or === "docker") and ensure messages/remediation use the runtime string rather than assuming Docker; reference isDockerRunning and runCapture in your edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@bin/lib/onboard.js`:
- Around line 25-37: The helper isDockerRunning() currently returns only a
boolean causing Podman to be treated like Docker downstream; change its contract
to return the detected runtime identifier (e.g., the string "docker" when
runCapture("docker info", ...) succeeds, "podman" when runCapture("podman info",
...) succeeds) and return false (or null) when neither is present; update any
callers to branch on the returned value instead of a boolean (use strict
equality checks like returned === "podman" or === "docker") and ensure
messages/remediation use the runtime string rather than assuming Docker;
reference isDockerRunning and runCapture in your edits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1440d786-4083-44d6-b10d-a23e7a3f2bbe
📒 Files selected for processing (3)
bin/lib/onboard.jsbin/lib/runner.jstest/runner.test.js
- Refactor isDockerRunning() to detectContainerRuntime() returning "docker", "podman", or null instead of boolean - Caller branches on runtime name for accurate status messages - Add JSDoc to all functions in changed files for docstring coverage
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bin/lib/runner.js (1)
43-49: Consider movingDOCKER_HOSTmutation out of module scope.Line 43 introduces import-time side effects (
process.envmutation). An explicit initializer (called by CLI entrypoints) would keep module loading pure and improve test isolation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/runner.js` around lines 43 - 49, The module currently mutates process.env.DOCKER_HOST at import time; move that logic into an explicit initializer function (e.g., export a function like initContainerSocket or initializeDockerHost) so modules remain pure and tests can control initialization. Locate the auto-detect block that calls detectContainerSocket() and wrap its behavior into the new function, exporting it and updating CLI entrypoints to call initializeDockerHost() at startup rather than performing the process.env assignment during module load. Ensure the new function checks if process.env.DOCKER_HOST is already set before assigning and returns the detected socket or assigned value to aid testing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/runner.js`:
- Around line 21-33: The current assignment of home uses "/tmp" fallback and
then unconditionally includes home-based socket paths in the candidates array;
change this so home-based candidates are only added when a real home is
available: set home = (opts && opts.home) || process.env.HOME || null (or
undefined) instead of "/tmp", and build the candidates array by conditionally
concatenating the home-derived paths only if home is truthy (leave the Podman
/run/user/${uid}/... and other non-home paths always present); keep the existing
exists and uid logic intact and reference the home variable and candidates array
in runner.js when making this change.
---
Nitpick comments:
In `@bin/lib/runner.js`:
- Around line 43-49: The module currently mutates process.env.DOCKER_HOST at
import time; move that logic into an explicit initializer function (e.g., export
a function like initContainerSocket or initializeDockerHost) so modules remain
pure and tests can control initialization. Locate the auto-detect block that
calls detectContainerSocket() and wrap its behavior into the new function,
exporting it and updating CLI entrypoints to call initializeDockerHost() at
startup rather than performing the process.env assignment during module load.
Ensure the new function checks if process.env.DOCKER_HOST is already set before
assigning and returns the detected socket or assigned value to aid testing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eb0c88df-0bdd-4cb4-b199-d1b7a323462e
📒 Files selected for processing (2)
bin/lib/onboard.jsbin/lib/runner.js
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/lib/onboard.js
Comment on lines
+21
to
33
| const home = (opts && opts.home) || process.env.HOME || "/tmp"; | ||
| const exists = (opts && opts.existsSync) || fs.existsSync; | ||
| const uid = (opts && opts.uid !== undefined) ? opts.uid : (process.getuid ? process.getuid() : 1000); | ||
|
|
||
| const candidates = [ | ||
| // Colima (preferred — existing behavior) | ||
| path.join(home, ".colima/default/docker.sock"), | ||
| path.join(home, ".config/colima/default/docker.sock"), | ||
| // Podman machine | ||
| path.join(home, ".local/share/containers/podman/machine/podman.sock"), | ||
| `/run/user/${uid}/podman/podman.sock`, | ||
| path.join(home, ".local/share/containers/podman/machine/qemu/podman.sock"), | ||
| ]; |
Contributor
There was a problem hiding this comment.
Avoid "/tmp" fallback for home when building socket candidates.
Line 21 can produce false-positive candidate paths under /tmp when HOME is unset, which may set DOCKER_HOST to a non-user socket path. Prefer skipping home-based candidates unless a real home path is available.
Suggested patch
- const home = (opts && opts.home) || process.env.HOME || "/tmp";
+ const home = (opts && opts.home) || process.env.HOME;
const exists = (opts && opts.existsSync) || fs.existsSync;
const uid = (opts && opts.uid !== undefined) ? opts.uid : (process.getuid ? process.getuid() : 1000);
- const candidates = [
- // Colima (preferred — existing behavior)
- path.join(home, ".colima/default/docker.sock"),
- path.join(home, ".config/colima/default/docker.sock"),
- // Podman machine
- path.join(home, ".local/share/containers/podman/machine/podman.sock"),
- `/run/user/${uid}/podman/podman.sock`,
- path.join(home, ".local/share/containers/podman/machine/qemu/podman.sock"),
- ];
+ const candidates = [
+ ...(home ? [
+ // Colima (preferred — existing behavior)
+ path.join(home, ".colima/default/docker.sock"),
+ path.join(home, ".config/colima/default/docker.sock"),
+ // Podman machine
+ path.join(home, ".local/share/containers/podman/machine/podman.sock"),
+ path.join(home, ".local/share/containers/podman/machine/qemu/podman.sock"),
+ ] : []),
+ `/run/user/${uid}/podman/podman.sock`,
+ ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/runner.js` around lines 21 - 33, The current assignment of home uses
"/tmp" fallback and then unconditionally includes home-based socket paths in the
candidates array; change this so home-based candidates are only added when a
real home is available: set home = (opts && opts.home) || process.env.HOME ||
null (or undefined) instead of "/tmp", and build the candidates array by
conditionally concatenating the home-derived paths only if home is truthy (leave
the Podman /run/user/${uid}/... and other non-home paths always present); keep
the existing exists and uid logic intact and reference the home variable and
candidates array in runner.js when making this change.
Contributor
Author
|
Closing — #286 supersedes this with a centralized platform.js runtime detection module that covers Podman, Colima, and Docker Desktop in a cleaner architecture. |
10 tasks
mafueee
pushed a commit
to mafueee/NemoClaw
that referenced
this pull request
Mar 28, 2026
…ter, and sandbox images (NVIDIA#144)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
detectContainerSocket(opts)with dependency injection for testability~/.local/share/containers/podman/machine/podman.sock,/run/user/$UID/podman/podman.sock, and QEMU variantisDockerRunning()in onboard — triespodman infoifdocker infofails, with a warning about--add-host=host-gatewaylimitationsCloses #116
Test plan
Automated Tests
New
detectContainerSockettest suite (6 tests):null/run/user/$UID/podman/podman.sock)~/.config/colima/default/docker.sock)Hardware Validation
Validated socket detection against real filesystem layouts:
~/.colima/default/docker.sock/var/run/docker.sock/var/run/docker.sockOn kobe,
docker psthrough the detected Colima socket confirmed 4 running containers. Colima XDG path (~/.config/colima/) and Podman paths were not present, matching test expectations for a Colima-only setup.Summary by CodeRabbit
New Features
Tests