Closed
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds platform and runtime detection (Docker/Colima/Podman/WSL), centralizes Docker-host detection, introduces local-inference utilities for vLLM/Ollama (health checks & base URLs), makes CoreDNS patching runtime-aware (Colima), switches install guidance to GitHub, and adds macOS smoke-test script plus multiple unit and shell tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant InstallScript as install.sh
participant RuntimeLib as Platform (JS/Shell)
participant DockerHost as Docker Host / Socket
participant Setup as setup.sh / onboard
participant LocalInf as local-inference (vLLM/Ollama)
User->>InstallScript: run installer
InstallScript->>RuntimeLib: detect_docker_host / infer runtime
RuntimeLib->>DockerHost: probe DOCKER_HOST or socket paths
DockerHost-->>RuntimeLib: dockerHost + runtime
RuntimeLib-->>InstallScript: report runtime
InstallScript->>Setup: start setup
Setup->>RuntimeLib: is_unsupported_macos_runtime?
alt Podman on macOS
RuntimeLib-->>Setup: unsupported -> exit
Setup-->>User: error (recommend Colima/Docker Desktop)
else Supported runtime
RuntimeLib-->>Setup: ok
Setup->>LocalInf: validateLocalProvider(vllm-local / ollama-local)
LocalInf->>LocalInf: run health-check (curl to localhost)
alt provider reachable
LocalInf-->>Setup: {ok:true} + base URL
Setup->>Setup: set OPENAI_BASE_URL
else unreachable
LocalInf-->>Setup: {ok:false, error}
Setup-->>User: error and exit
end
alt runtime == colima
Setup->>RuntimeLib: patch CoreDNS (fix-coredns.sh)
RuntimeLib->>DockerHost: apply CoreDNS patch using UPSTREAM_DNS
end
Setup-->>User: setup complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 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)
📝 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.
Actionable comments posted: 2
🧹 Nitpick comments (6)
test/credentials.test.js (1)
24-28: Increase the test timeout to reduce CI flakiness.Line 27 sets
timeout: 5000, which is tight for a shell-driven FIFO test with an intentional delay. Bumping this will improve reliability on slower macOS runners.Suggested diff
const result = spawnSync("bash", ["-lc", script], { cwd: path.join(__dirname, ".."), encoding: "utf-8", - timeout: 5000, + timeout: 15000, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/credentials.test.js` around lines 24 - 28, The test uses spawnSync in test/credentials.test.js to run the shell script and sets timeout: 5000 which is too low; update the timeout option passed to spawnSync (where spawnSync is called with args ["-lc", script]) to a higher value (e.g., 20000 or 30000) so the FIFO-based shell test with intentional delays is less likely to time out on slow CI/macOS runners.test/local-inference.test.js (1)
28-33: Consider adding test coverage for edge cases.The test suite covers the main flows well, but consider adding tests for:
getLocalProviderHealthCheck("vllm-local")to ensure parity- Unknown provider handling for both
getLocalProviderBaseUrlandgetLocalProviderHealthCheck♻️ Optional: Additional test cases
it("returns the expected health check command for vllm-local", () => { assert.equal( getLocalProviderHealthCheck("vllm-local"), "curl -sf http://localhost:8000/v1/models 2>/dev/null", ); }); it("returns null for unknown provider base URL", () => { assert.equal(getLocalProviderBaseUrl("unknown-provider"), null); }); it("returns null for unknown provider health check", () => { assert.equal(getLocalProviderHealthCheck("unknown-provider"), null); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/local-inference.test.js` around lines 28 - 33, Add tests to cover the edge cases mentioned: assert that getLocalProviderHealthCheck("vllm-local") returns "curl -sf http://localhost:8000/v1/models 2>/dev/null", and add two assertions that getLocalProviderBaseUrl("unknown-provider") and getLocalProviderHealthCheck("unknown-provider") both return null; locate the test file around the existing getLocalProviderHealthCheck("ollama-local") case and add these three new it() cases referencing the same helper functions.scripts/lib/runtime.sh (1)
114-117: Consider extending loopback detection to include IPv6.The current check
[[ "$ip" == 127.* ]]only matches IPv4 loopback addresses. If/etc/resolv.confcontains::1(IPv6 loopback), it won't be filtered.This may be acceptable if the environments you support only use IPv4 nameservers in resolv.conf, but worth noting for future-proofing.
♻️ Optional: Add IPv6 loopback detection
is_loopback_ip() { local ip="${1:-}" - [[ "$ip" == 127.* ]] + [[ "$ip" == 127.* ]] || [[ "$ip" == "::1" ]] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lib/runtime.sh` around lines 114 - 117, The is_loopback_ip function currently only matches IPv4 loopback addresses; update its check to also detect IPv6 loopback by returning true for strings like "::1" (and optionally "::1/128" if you expect CIDR suffixes). In other words, enhance the conditional inside is_loopback_ip to test for either a 127.* IPv4 pattern or an IPv6 "::1" form (handle possible CIDR suffix or surrounding brackets if needed) so resolv.conf entries containing ::1 are filtered as loopback.bin/lib/platform.js (1)
55-67: Consider adding Linux/WSL socket candidates for completeness.Currently
getDockerSocketCandidatesreturns an empty array for non-Darwin platforms, relying on the system default/var/run/docker.sock. This works because Docker CLI falls back to the default socket, but explicit handling could improve consistency with the macOS path.This is fine as-is since Linux/WSL typically use the system socket and don't need user-scoped resolution, but worth noting if future runtimes require user-scoped sockets on Linux.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/platform.js` around lines 55 - 67, getDockerSocketCandidates currently only returns socket candidates for darwin via getColimaDockerSocketCandidates and a user-scoped path; for other platforms it returns an empty array which hides potential Linux/WSL socket locations. Update getDockerSocketCandidates to include explicit Linux/WSL candidates (e.g., /var/run/docker.sock and common user/WSL-specific sockets) when platform !== "darwin", while keeping the existing darwin behavior and using the same opts.home and opts.platform handling; reference the function name getDockerSocketCandidates, the helper getColimaDockerSocketCandidates, and the platform/home variables to locate where to add the additional socket paths.test/runtime-shell.test.js (1)
13-13: Consider removing or documenting the emptyafterEachhook.The empty
afterEachhook serves no purpose. Either remove it or add a comment explaining it's a placeholder for future cleanup logic.♻️ Proposed cleanup
-afterEach(() => {});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/runtime-shell.test.js` at line 13, Remove or document the no-op test hook: either delete the empty afterEach() call in test/runtime-shell.test.js (the afterEach hook shown) or replace it with a short comment inside the file explaining it is intentionally left empty as a placeholder for future teardown logic so readers know it’s deliberate.scripts/smoke-macos-install.sh (1)
161-177: Consider adding a timeout to the log polling loop.The
feed_install_answersfunction polls the install log waiting for the "OpenClaw gateway launched" message. If the install stalls or the message format changes, this loop will run indefinitely. Consider adding a maximum iteration count or timeout.♻️ Proposed timeout addition
feed_install_answers() { local answers_pipe="$1" local install_log="$2" + local max_wait=300 # 5 minutes + local waited=0 ( printf '%s\n' "$SANDBOX_NAME" while :; do if [ -f "$install_log" ] && grep -q "OpenClaw gateway launched inside sandbox" "$install_log"; then break fi + waited=$((waited + 1)) + if [ "$waited" -ge "$max_wait" ]; then + echo "[smoke] Timed out waiting for gateway launch" >&2 + break + fi sleep 1 done printf 'n\n' ) > "$answers_pipe" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/smoke-macos-install.sh` around lines 161 - 177, The feed_install_answers function currently polls install_log forever waiting for the exact "OpenClaw gateway launched inside sandbox" string; add a timeout mechanism (e.g., max wait seconds or max iterations plus sleep) inside the while loop that checks install_log so the loop exits with an error if the message never appears, and ensure the function logs or writes a failure response to answers_pipe before returning; update references in this function (feed_install_answers, answers_pipe, install_log, the grep for "OpenClaw gateway launched inside sandbox") to implement the timeout and a clear failure path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/install.sh`:
- Around line 21-23: The script fails when run via curl|bash because
BASH_SOURCE[0] becomes /dev/stdin so SCRIPT_DIR points at /dev and sourcing
"$SCRIPT_DIR/lib/runtime.sh" fails; fix by ensuring runtime.sh functions
(socket_exists, find_colima_docker_socket, find_docker_desktop_socket,
detect_docker_host) are available at runtime: either inline those functions
directly into install.sh before the source line (so sourcing is no longer
required) or replace the sourcing logic that uses BASH_SOURCE and SCRIPT_DIR
with a robust fallback that downloads the runtime.sh to a temp file and sources
it (or uses bash <(curl ...) execution) when BASH_SOURCE points to /dev/stdin,
keeping set -euo pipefail behavior intact.
In `@test/install-preflight.test.js`:
- Around line 223-256: The fake docker stub created via writeExecutable
(path.join(fakeBin, "docker")) currently exits 1 for the "docker info" path
which causes the installer to treat Docker as not running; change the stub so
that when invoked with the "info" argument it exits 0 (success) so the test can
exercise the dual-runtime detection path and produce "Both Colima and Docker
Desktop are available". Also confirm the installer checks
NEMOCLAW_TEST_SOCKET_PATHS in scripts/install.sh (or create the expected socket
files/paths in the test using mkfifo/touch) so the runtime socket detection sees
both colimaSocket and dockerDesktopSocket.
---
Nitpick comments:
In `@bin/lib/platform.js`:
- Around line 55-67: getDockerSocketCandidates currently only returns socket
candidates for darwin via getColimaDockerSocketCandidates and a user-scoped
path; for other platforms it returns an empty array which hides potential
Linux/WSL socket locations. Update getDockerSocketCandidates to include explicit
Linux/WSL candidates (e.g., /var/run/docker.sock and common user/WSL-specific
sockets) when platform !== "darwin", while keeping the existing darwin behavior
and using the same opts.home and opts.platform handling; reference the function
name getDockerSocketCandidates, the helper getColimaDockerSocketCandidates, and
the platform/home variables to locate where to add the additional socket paths.
In `@scripts/lib/runtime.sh`:
- Around line 114-117: The is_loopback_ip function currently only matches IPv4
loopback addresses; update its check to also detect IPv6 loopback by returning
true for strings like "::1" (and optionally "::1/128" if you expect CIDR
suffixes). In other words, enhance the conditional inside is_loopback_ip to test
for either a 127.* IPv4 pattern or an IPv6 "::1" form (handle possible CIDR
suffix or surrounding brackets if needed) so resolv.conf entries containing ::1
are filtered as loopback.
In `@scripts/smoke-macos-install.sh`:
- Around line 161-177: The feed_install_answers function currently polls
install_log forever waiting for the exact "OpenClaw gateway launched inside
sandbox" string; add a timeout mechanism (e.g., max wait seconds or max
iterations plus sleep) inside the while loop that checks install_log so the loop
exits with an error if the message never appears, and ensure the function logs
or writes a failure response to answers_pipe before returning; update references
in this function (feed_install_answers, answers_pipe, install_log, the grep for
"OpenClaw gateway launched inside sandbox") to implement the timeout and a clear
failure path.
In `@test/credentials.test.js`:
- Around line 24-28: The test uses spawnSync in test/credentials.test.js to run
the shell script and sets timeout: 5000 which is too low; update the timeout
option passed to spawnSync (where spawnSync is called with args ["-lc", script])
to a higher value (e.g., 20000 or 30000) so the FIFO-based shell test with
intentional delays is less likely to time out on slow CI/macOS runners.
In `@test/local-inference.test.js`:
- Around line 28-33: Add tests to cover the edge cases mentioned: assert that
getLocalProviderHealthCheck("vllm-local") returns "curl -sf
http://localhost:8000/v1/models 2>/dev/null", and add two assertions that
getLocalProviderBaseUrl("unknown-provider") and
getLocalProviderHealthCheck("unknown-provider") both return null; locate the
test file around the existing getLocalProviderHealthCheck("ollama-local") case
and add these three new it() cases referencing the same helper functions.
In `@test/runtime-shell.test.js`:
- Line 13: Remove or document the no-op test hook: either delete the empty
afterEach() call in test/runtime-shell.test.js (the afterEach hook shown) or
replace it with a short comment inside the file explaining it is intentionally
left empty as a placeholder for future teardown logic so readers know it’s
deliberate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11814782-31fb-45c1-ade8-f86d57a37565
📒 Files selected for processing (19)
README.mdbin/lib/credentials.jsbin/lib/local-inference.jsbin/lib/onboard.jsbin/lib/platform.jsbin/lib/runner.jsinstall.shscripts/fix-coredns.shscripts/install.shscripts/lib/runtime.shscripts/setup.shscripts/smoke-macos-install.shtest/credentials.test.jstest/install-preflight.test.jstest/local-inference.test.jstest/platform.test.jstest/runner.test.jstest/runtime-shell.test.jstest/smoke-macos-install.test.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
README.md (1)
54-62: Clarify runtime prerequisites to avoid conflicting guidance.Good addition overall, but Line 51 (“Docker installed and running”) can read as mandatory for all platforms, while this new table introduces runtime-specific options. Consider replacing the global Docker prerequisite with a “container runtime + Docker CLI/context as required” note to keep prerequisites unambiguous across Linux/macOS/WSL.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 54 - 62, The global prerequisite "Docker installed and running" conflicts with the per-platform runtime table under the "Container Runtime Support" heading; replace that global line with a clarified prerequisite that says something like "A supported container runtime for your platform (see 'Container Runtime Support') and the Docker CLI/context where required" so it reflects platform-specific options and notes (refer to the "Container Runtime Support" table and the existing "Docker installed and running" phrase to locate and update the text).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@README.md`:
- Around line 54-62: The global prerequisite "Docker installed and running"
conflicts with the per-platform runtime table under the "Container Runtime
Support" heading; replace that global line with a clarified prerequisite that
says something like "A supported container runtime for your platform (see
'Container Runtime Support') and the Docker CLI/context where required" so it
reflects platform-specific options and notes (refer to the "Container Runtime
Support" table and the existing "Docker installed and running" phrase to locate
and update the text).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/install-preflight.test.js`:
- Around line 193-256: The macOS runtime-choice test ("does not silently prefer
Colima when both macOS runtimes are available") can misrun on Linux CI because
the real uname is used when invoking the installer; add a fake uname executable
into the same fakeBin (used by PATH) that returns "Darwin" for the kernel and
"x86_64" for machine so the installer follows the macOS branch; modify the test
setup where writeExecutable creates tools in fakeBin (alongside node, npm,
docker, colima) to add a uname stub that prints "Darwin" for uname -s and
"x86_64" for uname -m so the spawnSync(CURL_PIPE_INSTALLER, ...) invocation
exercises the macOS dual-runtime check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e6a4959d-806c-476f-a2b3-67ebabc44719
📒 Files selected for processing (2)
scripts/install.shtest/install-preflight.test.js
This was referenced Mar 18, 2026
kjw3
force-pushed
the
feat/macos-support
branch
from
9b897b0 to
83cc3f4
Compare
wscurran
added
Platform: macOS
Contributor
|
Thanks for the submission! |
Contributor
Author
|
Merged in with #295 |
10 tasks
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
2 participants
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
This PR moves the macOS support work forward across the open tracking issues:
#260#137#120#116#157What changed
Validation
Automated:
npm testManual on real macOS:
8.8.8.8=== Installation complete ===Summary by CodeRabbit