fix(inference): diagnose blocked llama.cpp bridge - #8716
Conversation
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
📝 WalkthroughWalkthroughManaged llama.cpp onboarding now checks host and OpenShell bridge reachability. Connectivity failures report validated network details and a narrow UFW command. Shared Docker fixtures and tests cover success, cleanup, invalid topology, resume behavior, and branded recovery instructions. ChangesManaged llama.cpp bridge reachability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Lifecycle as docker-llama-cpp-managed-lifecycle
participant Bridge as OpenShell Docker network
participant Host as managed llama.cpp host service
participant Firewall as UFW
Lifecycle->>Bridge: Probe gateway host and health endpoint
Bridge->>Host: Forward request to managed port
Host-->>Bridge: Return health response or connectivity failure
Lifecycle->>Firewall: Validate subnet, gateway, and port operands
Lifecycle-->>Lifecycle: Report remediation or standard probe failure
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-8716.docs.buildwithfern.com/nemoclaw |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 9a679bc in the TypeScript / code-coverage/cliThe overall coverage in commit 9a679bc in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.ts (1)
1045-1077: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the bridge failure classification into a helper.
probePrivateBridgenow performs wiring, two probes, failure classification, and message assembly. The added block raises the function's cognitive complexity. Move the classification and message construction into a small local function, for examplebridgeProbeFailure(gateway, port, probe), that returns anErrororundefined. The probe flow then stays linear.The behavior is correct as written. This is a readability change only.
🤖 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 `@src/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.ts` around lines 1045 - 1077, Extract the conditional bridge failure classification and remediation message construction from probePrivateBridge into a local helper such as bridgeProbeFailure(gateway, port, probe) that returns Error or undefined. Replace the inline block with a call to this helper while preserving the existing throw behavior and requireSuccess flow unchanged.Source: Coding guidelines
src/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle-engine.test-support.ts (1)
184-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the bridge gateway configurable with the subnet.
The fixture hardcodes
Gateway: "172.29.0.1"at Line 194 but exposessetOpenShellBridgeSubnet. A test that sets a subnet outside172.29.0.0/16produces a gateway that is outside the subnet.validateUfwRuleOperandsthen rejects the pair for the "gateway outside subnet" reason, not the reason the test intends. Add a matchingsetOpenShellBridgeGatewaysetter so each scenario controls both operands.The current tests still pass, because
0.0.0.0/0fails the prefix check first.Also applies to: 364-364
🤖 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 `@src/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle-engine.test-support.ts` around lines 184 - 199, Make the mocked bridge gateway configurable alongside openShellBridgeSubnet in the test-support fixture. Add an openShellBridgeGateway state value and setOpenShellBridgeGateway setter, then use that value instead of the hardcoded gateway in the openshell-docker response; update scenarios as needed so tests can control both operands.src/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.test.ts (1)
500-504: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant assertions after the exact-match assertion.
Line 500 asserts the full message with
toBe. Lines 503-504 then assert that the same string does not containsudo ufwor0.0.0.0/0. The exact match already proves both. Keep either the exact match or the two negative assertions.🤖 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 `@src/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.test.ts` around lines 500 - 504, In the failure-message assertions for the Docker llama.cpp private sandbox bridge probe test, remove the redundant negative contains checks for “sudo ufw” and “0.0.0.0/0” after the exact toBe assertion, keeping the exact message assertion as the sole validation.
🤖 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.
Inline comments:
In `@docs/inference/set-up-llama-cpp.mdx`:
- Around line 168-178: Update the managed-installation documentation around the
authoritative OpenShell Docker bridge probe to state that the detailed error,
including the exact narrow UFW command and topology values, appears only when
the bridge topology passes validateUfwRuleOperands; otherwise onboarding emits
the generic probe error without network details or a command.
In `@src/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.test.ts`:
- Around line 476-486: Update the test around createLifecycle(...).start(...) to
assert that the thrown error message does not contain the UFW remediation text,
rather than inspecting fixture.capture Docker arguments for "ufw". Keep the
existing probe-failure error assertion and verify the lifecycle’s reported
remediation message directly.
---
Nitpick comments:
In
`@src/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle-engine.test-support.ts`:
- Around line 184-199: Make the mocked bridge gateway configurable alongside
openShellBridgeSubnet in the test-support fixture. Add an openShellBridgeGateway
state value and setOpenShellBridgeGateway setter, then use that value instead of
the hardcoded gateway in the openshell-docker response; update scenarios as
needed so tests can control both operands.
In `@src/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.test.ts`:
- Around line 500-504: In the failure-message assertions for the Docker
llama.cpp private sandbox bridge probe test, remove the redundant negative
contains checks for “sudo ufw” and “0.0.0.0/0” after the exact toBe assertion,
keeping the exact message assertion as the sole validation.
In `@src/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.ts`:
- Around line 1045-1077: Extract the conditional bridge failure classification
and remediation message construction from probePrivateBridge into a local helper
such as bridgeProbeFailure(gateway, port, probe) that returns Error or
undefined. Replace the inline block with a call to this helper while preserving
the existing throw behavior and requireSuccess flow unchanged.
🪄 Autofix
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: Enterprise
Run ID: d80c83aa-d381-490d-913f-5eb67f270a9b
📒 Files selected for processing (7)
docs/inference/set-up-llama-cpp.mdxsrc/lib/onboard/host-service-reachability.test.tssrc/lib/onboard/host-service-reachability.tssrc/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle-engine.test-support.tssrc/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.test.tssrc/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.tssrc/lib/onboard/ufw-auto-apply.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
2 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
2 participants
Summary
Managed llama.cpp onboarding now distinguishes an OpenShell Docker bridge connection failure from a healthy internal runtime and host-loopback path. It stops without changing UFW and reports the validated bridge network, subnet, gateway IP address, fixed port, and narrow operator command.
Related Issue
Fixes #8712
Changes
openshell-dockerbridge probe as the firewall-diagnosis case only after the host-loopback probe passes.Type of Change
Quality Gates
Documentation Writer Review
docs-updatedDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailable — the complete equivalent ran from a clean worktree against refreshedupstream/mainbecause this maintainer checkout usesoriginfor a fork; pre-commit, commitlint, and pre-push passed. The normal pre-push hook also passed during the upstream push.npm run docsbuilds without warnings (doc changes only) — the build passed with 0 errors and the 2 pre-existing Fern warnings.Additional checks:
npm run typecheck:cli: passed.npm run test-size:check: passed.npm run docs: rendered the OpenClaw, Hermes, and Deep Agents variants with 0 errors.Signed-off-by: Senthil Ravichandran senthilr@nvidia.com
Summary by CodeRabbit
Bug Fixes
Documentation