feat(runtime): add Podman as supported container runtime for macOS and Linux#1359
Conversation
fdcc6d6 to
75de11a
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved macOS Podman incompatibility guard; added Podman socket discovery and support across runtime detection, onboarding, CoreDNS patching, scripts, tests, and documentation; Changes
sequenceDiagram
participant User
participant Script as Runtime Script
participant Detector as Socket Detector
participant Runtime as Engine (Colima/Podman/Docker)
participant Onboard as Onboard Logic
participant CoreDNS as CoreDNS Patch
User->>Script: run install or fix command
Script->>Detector: probe sockets (Colima, Podman, Docker)
Detector-->>Script: socket path or none
alt socket found
Script->>Runtime: set DOCKER_HOST to unix://socket
Script->>Onboard: inferContainerRuntime()
Onboard->>CoreDNS: shouldPatchCoredns(runtime)
CoreDNS-->>Script: patch result
else no socket
Script-->>User: skip/exit (no socket found)
end
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds first-class Podman support alongside existing container runtimes, updating runtime/socket detection and CoreDNS patching behavior to work on macOS and Linux, with accompanying docs and test updates.
Changes:
- Add Podman socket discovery and runtime classification (Node + shell helpers), and remove the prior macOS Podman “unsupported” guard.
- Extend CoreDNS patching logic to run for Podman (in addition to Colima).
- Update docs and tests to reflect Podman support and new runtime-selection behavior.
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
bin/lib/platform.js |
Adds Podman socket candidates; updates socket candidate ordering and CoreDNS patch condition. |
bin/lib/onboard.js |
Removes macOS Podman rejection; patches CoreDNS for Podman/Colima with updated logging. |
scripts/lib/runtime.sh |
Adds Podman socket detection and runtime classification; adjusts socket test helper behavior. |
scripts/fix-coredns.sh |
Detects Podman sockets in addition to Colima; passes detected runtime into upstream resolver. |
scripts/setup.sh |
Removes Podman macOS block; applies CoreDNS patch for Podman and Colima. |
scripts/smoke-macos-install.sh |
Adds podman runtime option and improves runtime error messaging. |
test/platform.test.js |
Adds tests for Podman socket candidate discovery and CoreDNS patch eligibility. |
test/runtime-shell.test.js |
Adds tests for Podman socket detection and DOCKER_HOST runtime classification. |
test/smoke-macos-install.test.js |
Updates unsupported runtime coverage and adds Podman runtime-option test. |
README.md |
Updates runtime compatibility matrix to include Podman on Linux/macOS (Apple Silicon). |
docs/reference/troubleshooting.md |
Adds Podman-specific troubleshooting guidance. |
.shellcheckrc |
Improves ShellCheck source following configuration (external sources + script-relative source-path). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Fix: forward CoreDNS to the container's default gateway IP, which | ||
| # is reachable from pods and routes DNS through Docker/Podman to the host. | ||
| # |
There was a problem hiding this comment.
The header comment says the patch forwards CoreDNS to the container's default gateway IP, but the implementation currently derives an upstream nameserver from resolv.conf via resolve_coredns_upstream(). Please either update the comment to match the current behavior or change the implementation to actually compute and use the container default gateway IP so docs and behavior stay aligned.
| # Fix: forward CoreDNS to the container's default gateway IP, which | |
| # is reachable from pods and routes DNS through Docker/Podman to the host. | |
| # | |
| # Fix: forward CoreDNS to a non-loopback upstream resolver derived from | |
| # the container and host resolv.conf, which is reachable from pods and | |
| # routes DNS through Docker/Podman to the host. |
| printf '%s\n' "$socket_path" | ||
| return 0 | ||
| fi | ||
| done | ||
| else |
There was a problem hiding this comment.
find_podman_socket() hardcodes /run/user/$uid for rootless Podman. Many setups expose the socket at "$XDG_RUNTIME_DIR/podman/podman.sock" (usually the same, but not always). Consider checking XDG_RUNTIME_DIR first when set, then falling back to /run/user/$uid, to match the intended detection priority and avoid missing sockets in nonstandard environments.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
scripts/fix-coredns.sh (1)
63-66: Error handling is more strict but reasonable.Changing from a fallback to
8.8.8.8to an error exit is a conservative choice. If the DNS upstream cannot be determined, failing explicitly prevents silent misconfiguration. However, this could cause onboarding failures in edge cases where the previous fallback would have worked.Consider if a fallback with a warning would be more user-friendly for environments with unusual DNS configurations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/fix-coredns.sh` around lines 63 - 66, The script currently exits with an error when UPSTREAM_DNS is empty (in the block checking UPSTREAM_DNS and DETECTED_RUNTIME), which may be too strict; change the logic to emit a warning including DETECTED_RUNTIME and the fact no non-loopback upstream was found, then set UPSTREAM_DNS to a safe fallback (e.g., 8.8.8.8) so onboarding doesn’t fail in odd environments, while still logging the issue for operators; update any downstream uses of UPSTREAM_DNS to rely on this fallback and ensure the warning is clear and actionable.test/platform.test.js (1)
40-55: Inconsistent assertion style: usesassert.deepEqualwhile rest of file usesexpect().toEqual().These tests use
assert.deepEqualwhile all other tests in this file useexpect(...).toEqual(). For consistency, consider using the same assertion style throughout.♻️ Suggested fix for consistency
describe("getPodmanSocketCandidates", () => { it("returns macOS Podman socket paths", () => { const home = "/tmp/test-home"; - assert.deepEqual(getPodmanSocketCandidates({ platform: "darwin", home }), [ + expect(getPodmanSocketCandidates({ platform: "darwin", home })).toEqual([ path.join(home, ".local/share/containers/podman/machine/podman.sock"), "/var/run/docker.sock", ]); }); it("returns Linux Podman socket paths with uid", () => { - assert.deepEqual( - getPodmanSocketCandidates({ platform: "linux", home: "/tmp/test-home", uid: 1001 }), - ["/run/user/1001/podman/podman.sock", "/run/podman/podman.sock"], - ); + expect( + getPodmanSocketCandidates({ platform: "linux", home: "/tmp/test-home", uid: 1001 }), + ).toEqual(["/run/user/1001/podman/podman.sock", "/run/podman/podman.sock"]); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/platform.test.js` around lines 40 - 55, Replace the inconsistent assert.deepEqual usage in the getPodmanSocketCandidates tests with the project's standard expect(...).toEqual() style: update both test cases that call getPodmanSocketCandidates (referenced by the function name getPodmanSocketCandidates in the describe block) to use expect(actual).toEqual(expected) instead of assert.deepEqual(...), keeping the same inputs (platform, home, uid) and expected arrays.scripts/lib/runtime.sh (1)
129-141: Consider usingXDG_RUNTIME_DIRfor Linux socket discovery.The PR description mentions XDG_RUNTIME_DIR-based discovery, but the implementation hardcodes
/run/user/$uid. While this is typically correct, respectingXDG_RUNTIME_DIRwhen set would be more robust for non-standard configurations.♻️ Suggested enhancement
else local uid uid="$(id -u 2>/dev/null || echo 1000)" + local xdg_runtime="${XDG_RUNTIME_DIR:-/run/user/$uid}" for socket_path in \ - "/run/user/$uid/podman/podman.sock" \ + "$xdg_runtime/podman/podman.sock" \ "/run/podman/podman.sock" do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lib/runtime.sh` around lines 129 - 141, The socket discovery block currently hardcodes "/run/user/$uid" instead of respecting XDG_RUNTIME_DIR; update the logic in the else branch that defines uid and iterates over socket_path to first check if XDG_RUNTIME_DIR is set and use "$XDG_RUNTIME_DIR/podman/podman.sock" (or "$XDG_RUNTIME_DIR/podman.sock" as appropriate) before falling back to "/run/user/$uid/podman/podman.sock" and "/run/podman/podman.sock"; ensure you still derive uid with uid="$(id -u 2>/dev/null || echo 1000)" and use the same socket_exists function to test each candidate path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/fix-coredns.sh`:
- Around line 63-66: The script currently exits with an error when UPSTREAM_DNS
is empty (in the block checking UPSTREAM_DNS and DETECTED_RUNTIME), which may be
too strict; change the logic to emit a warning including DETECTED_RUNTIME and
the fact no non-loopback upstream was found, then set UPSTREAM_DNS to a safe
fallback (e.g., 8.8.8.8) so onboarding doesn’t fail in odd environments, while
still logging the issue for operators; update any downstream uses of
UPSTREAM_DNS to rely on this fallback and ensure the warning is clear and
actionable.
In `@scripts/lib/runtime.sh`:
- Around line 129-141: The socket discovery block currently hardcodes
"/run/user/$uid" instead of respecting XDG_RUNTIME_DIR; update the logic in the
else branch that defines uid and iterates over socket_path to first check if
XDG_RUNTIME_DIR is set and use "$XDG_RUNTIME_DIR/podman/podman.sock" (or
"$XDG_RUNTIME_DIR/podman.sock" as appropriate) before falling back to
"/run/user/$uid/podman/podman.sock" and "/run/podman/podman.sock"; ensure you
still derive uid with uid="$(id -u 2>/dev/null || echo 1000)" and use the same
socket_exists function to test each candidate path.
In `@test/platform.test.js`:
- Around line 40-55: Replace the inconsistent assert.deepEqual usage in the
getPodmanSocketCandidates tests with the project's standard
expect(...).toEqual() style: update both test cases that call
getPodmanSocketCandidates (referenced by the function name
getPodmanSocketCandidates in the describe block) to use
expect(actual).toEqual(expected) instead of assert.deepEqual(...), keeping the
same inputs (platform, home, uid) and expected arrays.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 18e8a1e0-3cf6-45da-901a-4a14149adfdf
📒 Files selected for processing (11)
.shellcheckrcREADME.mdbin/lib/onboard.jsbin/lib/platform.jsdocs/reference/troubleshooting.mdscripts/fix-coredns.shscripts/lib/runtime.shscripts/smoke-macos-install.shtest/platform.test.jstest/runtime-shell.test.jstest/smoke-macos-install.test.js
- fix-coredns.sh: update comment to accurately describe the upstream resolver strategy (resolv.conf-derived, not gateway IP) - runtime.sh: check XDG_RUNTIME_DIR first in find_podman_socket(), falling back to /run/user/$uid for rootless Podman compatibility Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
|
Addressed both Copilot comments:
|
cv
left a comment
There was a problem hiding this comment.
Clean rebase of #819. Podman socket detection, priority ordering, and CoreDNS patching logic all look correct. Good test coverage. The shouldPatchCoredns narrowing from all-runtimes to colima+podman-only is the right call.
Once this lands, platform.js is unblocked for TS migration.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/dns-proxy.test.js (1)
109-115: Tighten theexit 1assertion to the same error block.
expect(content).toContain("exit 1")is a bit broad. Consider asserting the error message andexit 1as an ordered block so unrelatedexit 1lines do not satisfy this test.Suggested assertion tweak
- expect(content).toContain("exit 1"); + expect(content).toMatch( + /Could not determine a non-loopback DNS upstream[\s\S]*exit 1/ + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/dns-proxy.test.js` around lines 109 - 115, The test currently checks for "exit 1" loosely; tighten it so the "Could not determine a non-loopback DNS upstream" error and the subsequent "exit 1" occur together. Modify the assertions in the "exits with error when no upstream DNS can be resolved" test: after reading FIX_COREDNS into content, locate the index of the error message string and assert that "exit 1" appears at or after that index (or use a single regex that matches the error message followed by an "exit 1" line). Use the existing variables (content and FIX_COREDNS) and replace the broad expect(content).toContain("exit 1") with this ordered-check approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/dns-proxy.test.js`:
- Around line 109-115: The test currently checks for "exit 1" loosely; tighten
it so the "Could not determine a non-loopback DNS upstream" error and the
subsequent "exit 1" occur together. Modify the assertions in the "exits with
error when no upstream DNS can be resolved" test: after reading FIX_COREDNS into
content, locate the index of the error message string and assert that "exit 1"
appears at or after that index (or use a single regex that matches the error
message followed by an "exit 1" line). Use the existing variables (content and
FIX_COREDNS) and replace the broad expect(content).toContain("exit 1") with this
ordered-check approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8c0efb1-f6b5-441b-869b-28dd50656280
📒 Files selected for processing (2)
test/dns-proxy.test.jstest/platform.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/platform.test.js
|
✨ Thanks for submitting this pull request, which proposes a way to support Podman as an alternative runtime on macOS and Linux. This could give users more flexibility in how they run local models. |
…d Linux Rebased onto current main (resolved conflicts in README.md and onboard.js). Removed isUnsupportedRuntime Podman warning since Podman is now supported. Co-authored-by: nv-ddave <nv-ddave@users.noreply.github.com> Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
- fix-coredns.sh: update comment to accurately describe the upstream resolver strategy (resolv.conf-derived, not gateway IP) - runtime.sh: check XDG_RUNTIME_DIR first in find_podman_socket(), falling back to /run/user/$uid for rootless Podman compatibility Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
b044813 to
7207be8
Compare
|
Rebased onto current main — conflicts in |
|
The |
- onboard.js: restore closing brace for WSL if-block — without it, all post-preflight code (openshell install, gateway setup, etc.) only ran under WSL - platform.test.js: replace assert.deepEqual with expect().toEqual() in getPodmanSocketCandidates tests (assert was never imported) - dns-proxy.test.js: update content assertions to match rewritten fix-coredns.sh (now delegates to resolve_coredns_upstream, supports Podman alongside Colima, validates UPSTREAM_DNS) - nemoclaw-cli-recovery.test.js: increase timeout from 5s to 15s (the extra runtime.sh sourcing in the Podman branch adds startup cost) Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The test compared full tmpdir listings before/after, so a leftover nemoclaw-curl-probe-* dir from a prior run caused a false failure. Now checks only that no *new* entries leaked. Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The SC1091 suppression is sufficient — shellcheck doesn't need external-sources to pass on these scripts. Keep the original config to minimize the PR's surface area. Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…salvage (#1585) ## Summary - Adds 8 agent skills and 4 TypeScript scripts that automate the maintainer workflow: triage, gate-check, salvage, security sweep, test gaps, hotspots, sequencing - Skills are designed for `/loop 10m /nemoclaw-maintainer-loop` continuous operation - Scripts make triage scoring, gate checking, and hotspot detection deterministic (call `gh-pr-merge-now` as data engine) - Battle-tested in this session: triaged queue, salvaged 3 PRs (#1359, #898, #1089), merged all 4 including #1139 ## Test plan - [ ] Invoke `/nemoclaw-maintainer-loop` and verify triage output - [ ] Run `node --experimental-strip-types --no-warnings .agents/skills/nemoclaw-maintainer-loop/scripts/triage.ts --approved-only` - [ ] Run `node --experimental-strip-types --no-warnings .agents/skills/nemoclaw-maintainer-loop/scripts/check-gates.ts <pr-number>` - [ ] Verify skills YAML passes `prek` validation (tested in pre-commit) 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added automated triage, prioritization, and merge-gate checks plus version/straggler tooling to streamline daily maintainer flows. * Added end-to-end maintainer skills for morning/day/evening run-books and release handoff guidance. * **Documentation** * Added workflows for PR review priorities, hotspot cooling, salvage PRs, security sweeps, sequencing work, and closing test gaps. * Added persistent state schema and guidance for maintainer loop operations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…d Linux (NVIDIA#1359) Rebase + cleanup of NVIDIA#819 for @nv-ddave (by request). ## What changed from the original PR - Rebased onto current main (resolved conflicts in onboard.js, platform.js, fix-coredns.sh, test/platform.test.js, README.md) - Removed the commitlint-failing revert commit — squashed into one clean `feat(runtime):` commit with Signed-off-by - Removed `isUnsupportedMacosRuntime` entirely (Podman on macOS is now supported) ## Summary of the feature (credit: @nv-ddave) Adds first-class Podman support alongside Docker and Colima: - Socket detection via `getPodmanSocketCandidates()` (machine socket, XDG_RUNTIME_DIR, /run/podman) with platform guards - CoreDNS patching enabled for Colima + Podman (both use custom network bridges) - `fix-coredns.sh` updated to use container default-gateway IP instead of resolv.conf - Runtime detection, README platform matrix, and troubleshooting docs updated - Test coverage: unit tests for platform.js + runtime-shell integration tests Co-authored-by: nv-ddave <nv-ddave@users.noreply.github.com> Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Podman is now recognized as a supported runtime on macOS and Linux; onboarding/install flows accept it. * **Bug Fixes** * Improved runtime detection and CoreDNS handling to prefer Colima/Podman and avoid premature unsupported-runtime exits; CoreDNS patching now fails if no valid upstream is found. * **Documentation** * Updated container runtimes matrix and added a Podman troubleshooting guide. * **Tests** * Expanded/adjusted tests for Podman detection and related flows. * **Chores** * Updated ShellCheck configuration to follow sourced files. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: nv-ddave <nv-ddave@users.noreply.github.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…salvage (NVIDIA#1585) ## Summary - Adds 8 agent skills and 4 TypeScript scripts that automate the maintainer workflow: triage, gate-check, salvage, security sweep, test gaps, hotspots, sequencing - Skills are designed for `/loop 10m /nemoclaw-maintainer-loop` continuous operation - Scripts make triage scoring, gate checking, and hotspot detection deterministic (call `gh-pr-merge-now` as data engine) - Battle-tested in this session: triaged queue, salvaged 3 PRs (NVIDIA#1359, NVIDIA#898, NVIDIA#1089), merged all 4 including NVIDIA#1139 ## Test plan - [ ] Invoke `/nemoclaw-maintainer-loop` and verify triage output - [ ] Run `node --experimental-strip-types --no-warnings .agents/skills/nemoclaw-maintainer-loop/scripts/triage.ts --approved-only` - [ ] Run `node --experimental-strip-types --no-warnings .agents/skills/nemoclaw-maintainer-loop/scripts/check-gates.ts <pr-number>` - [ ] Verify skills YAML passes `prek` validation (tested in pre-commit) 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added automated triage, prioritization, and merge-gate checks plus version/straggler tooling to streamline daily maintainer flows. * Added end-to-end maintainer skills for morning/day/evening run-books and release handoff guidance. * **Documentation** * Added workflows for PR review priorities, hotspot cooling, salvage PRs, security sweeps, sequencing work, and closing test gaps. * Added persistent state schema and guidance for maintainer loop operations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rebase + cleanup of #819 for @nv-ddave (by request).
What changed from the original PR
feat(runtime):commit with Signed-off-byisUnsupportedMacosRuntimeentirely (Podman on macOS is now supported)Summary of the feature (credit: @nv-ddave)
Adds first-class Podman support alongside Docker and Colima:
getPodmanSocketCandidates()(machine socket, XDG_RUNTIME_DIR, /run/podman) with platform guardsfix-coredns.shupdated to use container default-gateway IP instead of resolv.confCo-authored-by: nv-ddave nv-ddave@users.noreply.github.com
Signed-off-by: Benedikt Schackenberg 6381261+BenediktSchackenberg@users.noreply.github.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores