fix(onboard): use 127.0.0.1 instead of localhost for local inference detection#1518
Closed
chandler-barlow wants to merge 6 commits into
Closed
fix(onboard): use 127.0.0.1 instead of localhost for local inference detection#1518chandler-barlow wants to merge 6 commits into
chandler-barlow wants to merge 6 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughReplaced hostname-based local endpoints with explicit loopback IP Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 Generate unit tests (beta)
Comment |
3 tasks
…aw into fix/h6-vllm-localhost-bind
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
bin/lib/onboard.js (1)
2685-2690: Use a shared loopback constant to prevent future partial endpoint drift.The same host literal appears in multiple probes/messages. Centralizing it reduces missed updates across onboarding paths.
♻️ Suggested refactor
const OPENCLAW_LAUNCH_AGENT_PLIST = "~/Library/LaunchAgents/ai.openclaw.gateway.plist"; +const LOOPBACK_V4 = "127.0.0.1"; @@ - const ollamaRunning = !!runCapture("curl -sf http://127.0.0.1:11434/api/tags 2>/dev/null", { + const ollamaRunning = !!runCapture(`curl -sf http://${LOOPBACK_V4}:11434/api/tags 2>/dev/null`, { @@ - const vllmRunning = !!runCapture("curl -sf http://127.0.0.1:8000/v1/models 2>/dev/null", { + const vllmRunning = !!runCapture(`curl -sf http://${LOOPBACK_V4}:8000/v1/models 2>/dev/null`, { @@ - `Local Ollama (127.0.0.1:11434)${ollamaRunning ? " — running" : ""}` + + `Local Ollama (${LOOPBACK_V4}:11434)${ollamaRunning ? " — running" : ""}` +Also applies to: 2706-2707, 3087-3087, 3139-3139, 3185-3191, 3208-3208
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 2685 - 2690, Replace repeated loopback string literals with a single shared constant (e.g., const LOOPBACK = "127.0.0.1") and use it wherever probes reference the local host; update the code that creates ollamaRunning and vllmRunning (the runCapture calls used to probe "curl -sf http://127.0.0.1:...") to interpolate that constant, and change any other runCapture or probe calls that use "127.0.0.1" (the other occurrences called out in the review) to reference the same constant so all endpoints stay in sync.
🤖 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/onboard.js`:
- Line 2621: The readiness probe in on-board command (`openshell sandbox exec
... curl -sf http://127.0.0.1:18789/`) no longer matches the test stub in
test/onboard.test.js (which still stubs http://localhost:18789/); update the
test stub in test/onboard.test.js (around the stub block at lines ~1475-1482) to
match the new probe host (use 127.0.0.1:18789 or a host-agnostic regex) so the
test correctly intercepts the readiness check invoked by the code that builds
the probe command.
In `@test/onboard-selection.test.js`:
- Around line 130-132: Update all remaining test stubs/assertions in the same
test file that still reference "localhost" so they use "127.0.0.1" consistently
(the diff already changed command.includes("127.0.0.1:11434/api/tags") and
command.includes("127.0.0.1:8000/v1/models")). Search for any occurrences of
"localhost" in test/onboard-selection.test.js (examples include command.includes
checks and mocked responses around the lines called out in the review) and
replace host portions like "localhost:11434", "localhost:8000", etc. with
"127.0.0.1" so all stubs/assertions (the conditional checks using
command.includes(...) and their returned strings) match the runtime probes.
---
Nitpick comments:
In `@bin/lib/onboard.js`:
- Around line 2685-2690: Replace repeated loopback string literals with a single
shared constant (e.g., const LOOPBACK = "127.0.0.1") and use it wherever probes
reference the local host; update the code that creates ollamaRunning and
vllmRunning (the runCapture calls used to probe "curl -sf http://127.0.0.1:...")
to interpolate that constant, and change any other runCapture or probe calls
that use "127.0.0.1" (the other occurrences called out in the review) to
reference the same constant so all endpoints stay in sync.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1adf543-56d1-4eee-a5ca-23f970dff13a
📒 Files selected for processing (3)
bin/lib/onboard.jssrc/lib/local-inference.tstest/onboard-selection.test.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/local-inference.test.ts (1)
54-57: Test title and assertion intent are slightly misaligned.On Line 54 the test says it covers vllm validation + health checks, but Line 55 validates
ollama-local. Consider renaming or splitting for clarity.✏️ Suggested rename
-it("returns the expected validation and health check commands for vllm-local", () => { +it("returns expected validation URL for ollama-local and health/reachability checks for vllm-local", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/local-inference.test.ts` around lines 54 - 57, The test title says it covers "vllm-local" but the first assertion calls getLocalProviderValidationBaseUrl("ollama-local"), causing a mismatch; update the test to either rename it to reference ollama-local or split into two tests: one that asserts getLocalProviderValidationBaseUrl("ollama-local") returns "http://127.0.0.1:11434/v1" and another that asserts getLocalProviderHealthCheck("vllm-local") returns "curl -sf http://127.0.0.1:8000/v1/models 2>/dev/null", ensuring the test names and assertions align with the functions getLocalProviderValidationBaseUrl and getLocalProviderHealthCheck.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/local-inference.test.ts`:
- Line 83: The regexes in the test assertions using expect(...).toMatch
currently use unescaped dots (e.g., the pattern seen in the
expect(result.message).toMatch(/http:\/\/127.0.0.1:11434/) call), so replace the
dot characters in the IP and any literal dots in the URL/port parts with escaped
dots (i.e., \.) in each affected toMatch call (the occurrences at the patterns
around the expect(...) statements at lines corresponding to 83, 100, 213, and
226) so they match literal dots rather than any character; update each
expect(...).toMatch invocation accordingly while keeping the rest of the
assertion unchanged.
---
Nitpick comments:
In `@src/lib/local-inference.test.ts`:
- Around line 54-57: The test title says it covers "vllm-local" but the first
assertion calls getLocalProviderValidationBaseUrl("ollama-local"), causing a
mismatch; update the test to either rename it to reference ollama-local or split
into two tests: one that asserts
getLocalProviderValidationBaseUrl("ollama-local") returns
"http://127.0.0.1:11434/v1" and another that asserts
getLocalProviderHealthCheck("vllm-local") returns "curl -sf
http://127.0.0.1:8000/v1/models 2>/dev/null", ensuring the test names and
assertions align with the functions getLocalProviderValidationBaseUrl and
getLocalProviderHealthCheck.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9a6cce97-cd19-4120-88e6-ca8e1091a8d4
📒 Files selected for processing (1)
src/lib/local-inference.test.ts
| const result = validateLocalProvider("ollama-local", () => ""); | ||
| expect(result.ok).toBe(false); | ||
| expect(result.message).toMatch(/http:\/\/localhost:11434/); | ||
| expect(result.message).toMatch(/http:\/\/127.0.0.1:11434/); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that current patterns are permissive due to unescaped dots.
python - <<'PY'
import re
patterns = [
r'http:\/\/127.0.0.1:11434',
r'http:\/\/127.0.0.1:8000',
r'^nohup curl -s http:\/\/127.0.0.1:11434\/api\/generate ',
r'^curl -sS --max-time 120 http:\/\/127.0.0.1:11434\/api\/generate ',
]
samples = [
"http://127.0.0.1:11434",
"http://127x0x0x1:11434", # should NOT match exact loopback intent
"nohup curl -s http://127x0x0x1:11434/api/generate ",
"curl -sS --max-time 120 http://127x0x0x1:11434/api/generate ",
]
for p in patterns:
print(f"\nPattern: {p}")
for s in samples:
print(f" matches={bool(re.search(p, s))} :: {s}")
PYRepository: NVIDIA/NemoClaw
Length of output: 1199
🏁 Script executed:
# First, check if the file exists
if [ -f src/lib/local-inference.test.ts ]; then
echo "File exists"
# Get line count
wc -l src/lib/local-inference.test.ts
else
echo "File not found at expected location"
# Search for it
find . -name "local-inference.test.ts" 2>/dev/null
fiRepository: NVIDIA/NemoClaw
Length of output: 106
🏁 Script executed:
# Read the file around the specified lines to verify the patterns
if [ -f src/lib/local-inference.test.ts ]; then
echo "=== Line 83 context ==="
sed -n '80,86p' src/lib/local-inference.test.ts
echo -e "\n=== Line 100 context ==="
sed -n '98,103p' src/lib/local-inference.test.ts
echo -e "\n=== Line 213 context ==="
sed -n '211,216p' src/lib/local-inference.test.ts
echo -e "\n=== Line 226 context ==="
sed -n '224,229p' src/lib/local-inference.test.ts
fiRepository: NVIDIA/NemoClaw
Length of output: 1396
🏁 Script executed:
# Search for all instances of the pattern to be fixed
rg 'toMatch.*127\.0\.0\.1' src/lib/local-inference.test.ts -nRepository: NVIDIA/NemoClaw
Length of output: 386
🏁 Script executed:
# Check if there are any other instances of 127.0.0.1 in the file
rg '127\.0\.0\.1' src/lib/local-inference.test.ts -n | wc -l
echo "---"
rg '127\.0\.0\.1' src/lib/local-inference.test.ts -nRepository: NVIDIA/NemoClaw
Length of output: 719
Escape unescaped dots in regex patterns for accurate IP address matching.
The patterns at lines 83, 100, 213, and 226 use unescaped . in regex, which matches any character instead of literal dots. This weakens test assertions by allowing 127x0x0x1 to match 127.0.0.1.
Apply fixes
- expect(result.message).toMatch(/http:\/\/127.0.0.1:11434/);
+ expect(result.message).toMatch(/http:\/\/127\.0\.0\.1:11434/);
...
- expect(result.message).toMatch(/http:\/\/127.0.0.1:8000/);
+ expect(result.message).toMatch(/http:\/\/127\.0\.0\.1:8000/);
...
- expect(command).toMatch(/^nohup curl -s http:\/\/127.0.0.1:11434\/api\/generate /);
+ expect(command).toMatch(/^nohup curl -s http:\/\/127\.0\.0\.1:11434\/api\/generate /);
...
- expect(command).toMatch(/^curl -sS --max-time 120 http:\/\/127.0.0.1:11434\/api\/generate /);
+ expect(command).toMatch(/^curl -sS --max-time 120 http:\/\/127\.0\.0\.1:11434\/api\/generate /);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expect(result.message).toMatch(/http:\/\/127.0.0.1:11434/); | |
| expect(result.message).toMatch(/http:\/\/127\.0\.0\.1:11434/); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/local-inference.test.ts` at line 83, The regexes in the test
assertions using expect(...).toMatch currently use unescaped dots (e.g., the
pattern seen in the expect(result.message).toMatch(/http:\/\/127.0.0.1:11434/)
call), so replace the dot characters in the IP and any literal dots in the
URL/port parts with escaped dots (i.e., \.) in each affected toMatch call (the
occurrences at the patterns around the expect(...) statements at lines
corresponding to 83, 100, 213, and 226) so they match literal dots rather than
any character; update each expect(...).toMatch invocation accordingly while
keeping the rest of the assertion unchanged.
cv
requested changes
Apr 9, 2026
cv
left a comment
Collaborator
There was a problem hiding this comment.
Security Review — WARNING (incomplete)
The change is a net security positive — bypassing DNS resolution for loopback eliminates ambiguity (dual-stack ::1 resolution, potential DNS hijack in misconfigured environments).
Required fix
Missing test updates: test/onboard.test.js still has 6 stubs matching localhost:18789 that need updating to 127.0.0.1:18789. These stubs no longer intercept the readiness check after this change, which is likely causing the CI failure. CodeRabbit flagged this too.
What's good
- SSRF validator works on resolved IPs, not hostname strings — unaffected
getLocalProviderBaseUrl(container-to-host path) correctly untouched- Pure 1:1 replacement, no logic changes
Collaborator
|
Thanks for the PR. Since this branch does not allow maintainer edits, I can't port it across the recently merged mechanical JS→TS migration stack for you. Please merge or rebase If your branch touches migrated root CLI files or renamed root tests, please run: git fetch origin
git merge origin/main
npm run ts-migration:assist -- --base origin/main --write
npm run build:cli
npm run typecheck:cli
npm run lint
npm testThen push the updated branch. If your branch does not touch migrated root CLI paths, a normal merge/rebase from |
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
4 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.
Rationale
*Addressing review and resolved conflicts to close this pr
Using
localhostcan sometimes resolve to IPv6 addresses or behave inconsistently across different environments and network configurations.Changes
Switched to the explicit IPv4 loopback address
127.0.0.1for local inference service detection to improve reliability.Verification Results
npm test.Leading Standards
This PR follows the project's 'First Principles' approach, prioritizing deterministic behavior and zero-trust security defaults.
Summary by CodeRabbit
Signed-off-by: Chandler Barlow cbarlow@nvidia.com