fix(runtime): route local Ollama through auth proxy#6579
Closed
apurvvkumaria wants to merge 1 commit into
Closed
Conversation
Co-authored-by: Chengjie Wang <chengjiew@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
added
provider: ollama
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the branch. TypeScript / code-coverage/cliThe overall coverage in the branch remains at 77%, unchanged from the branch. Show a code coverage summary of the most impacted files.
Updated |
Contributor
📝 WalkthroughWalkthroughAdds WSL detection and container-runtime inference to the shell runtime library to route Ollama access between direct host loopback and a managed auth-proxy path, updates provider URL and health-check logic accordingly, expands shell test coverage for these paths, and revises troubleshooting documentation. ChangesOllama Routing for WSL/Docker Desktop
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Script as get_local_provider_base_url
participant RuntimeDetect as detect_container_runtime_from_docker
participant Loopback as container_can_reach_host_loopback
participant PortLogic as get_ollama_container_port
Script->>RuntimeDetect: docker info
RuntimeDetect-->>Script: ContainerRuntime
Script->>Loopback: is_wsl_runtime + ContainerRuntime
Loopback-->>Script: can reach host loopback?
Script->>PortLogic: NEMOCLAW_OLLAMA_PORT, proxy port
PortLogic-->>Script: raw port or proxy port
Script-->>Script: build ollama-local base URL
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-6579.docs.buildwithfern.com/nemoclaw |
Contributor
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 2 items to resolve/justify, 1 in-scope improvement
|
Contributor
E2E Advisor RecommendationRequired E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Contributor
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/lib/runtime.sh (1)
140-148: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider bounding the
docker infocall with a timeout.The comment states an unhealthy Docker maps to
unknown, but a hung/unresponsive daemon won't return promptly, and this runs on the provider-URL resolution path, so it can stall the caller instead of fail-safing to the proxy route.♻️ Suggested change
- info="$(docker info --format '{{.OperatingSystem}} {{range .Labels}}{{.}} {{end}}' 2>/dev/null || true)" + info="$(timeout 5 docker info --format '{{.OperatingSystem}} {{range .Labels}}{{.}} {{end}}' 2>/dev/null || true)"Please confirm
timeoutis available on all supported runtimes (it may be absent on stock macOS without coreutils).🤖 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 `@scripts/lib/runtime.sh` around lines 140 - 148, Add a timeout around the docker info probe in detect_container_runtime_from_docker so an unresponsive daemon can’t stall provider URL resolution; keep the existing best-effort behavior by falling back to unknown when the command times out or fails. Use the existing detect_container_runtime_from_docker and infer_container_runtime_from_info flow, and verify whether timeout is guaranteed on all supported runtimes; if not, add a portable fallback or guard for environments like stock macOS where timeout may be unavailable.
🤖 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.
Nitpick comments:
In `@scripts/lib/runtime.sh`:
- Around line 140-148: Add a timeout around the docker info probe in
detect_container_runtime_from_docker so an unresponsive daemon can’t stall
provider URL resolution; keep the existing best-effort behavior by falling back
to unknown when the command times out or fails. Use the existing
detect_container_runtime_from_docker and infer_container_runtime_from_info flow,
and verify whether timeout is guaranteed on all supported runtimes; if not, add
a portable fallback or guard for environments like stock macOS where timeout may
be unavailable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2e8c38ac-e29a-4283-b28e-be77a5cdcb88
📒 Files selected for processing (3)
docs/reference/troubleshooting.mdxscripts/lib/runtime.shtest/runtime-shell.test.ts
Contributor
PR Review Advisor — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 1 item to resolve/justify, 0 in-scope improvements
|
cjagwani
requested changes
Jul 9, 2026
cjagwani
left a comment
cjagwani
left a comment
Contributor
There was a problem hiding this comment.
HOLD on exact head 3bf8cf8 for two concrete blockers:
-
The new get_local_provider_base_url and check_local_provider_health behavior has no production caller. At this head, repository search finds only the definitions and tests; smoke-macos-install.sh sources runtime.sh but never invokes either function. Managed onboarding routes Ollama through src/lib/inference/local.ts, which this PR does not change. Please identify and wire the live consumer with acceptance coverage, or narrow/rework the PR so its claimed behavior matches what actually ships.
-
detect_container_runtime_from_docker runs docker info without a bound. An unhealthy daemon can therefore hang this provider-URL path instead of falling back to the token proxy as the comment promises. Please add a portable bounded probe and a hanging-fake-docker regression.
The fail-safe port selection, DCO, verified commit, and static security posture otherwise look sound. Once a live path exists, please provide exact-head acceptance evidence for that path.
Collaborator
Author
|
Closing after exact-head review found that this patch changes only uncoupled shell helpers: repository search shows no production caller for get_local_provider_base_url or check_local_provider_health. Managed onboarding already selects the Ollama sandbox port in src/lib/inference/local.ts through detectContainerRuntimeFromDockerInfo(), whose Docker probe is bounded in src/lib/adapters/docker/runtime.ts. Wiring the dead shell helper would duplicate that live path, while #3136's manually created host.docker.internal provider remains an OpenShell networking limitation. Chengjie Wang's original work and co-author credit remain preserved here; no functional fix should be merged from an unused path. |
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
3 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
Route the managed
ollama-localprovider through NemoClaw's token-gated auth proxy on non-WSL runtimes while preserving the Docker Desktop loopback exception on WSL. This is a clean, verified replacement for #6337; Chengjie Wang's original implementation is preserved and credited as co-author.Related Issue
Refs #3136. Supersedes #6337 because its published commit history cannot satisfy the current GitHub verification requirement.
Changes
/proc/versionsignals.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project integration test/runtime-shell.test.ts(65/65 passed)npm run docsbuilds without warnings (doc changes only) — passed with 0 errors and 2 pre-existing warnings.Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
Bug Fixes
Documentation