Skip to content

fix(inference): route local compatible endpoints through gateway#6331

Closed
chengjiew wants to merge 10 commits into
mainfrom
fix/5744_local-vllm-inference-gateway
Closed

fix(inference): route local compatible endpoints through gateway#6331
chengjiew wants to merge 10 commits into
mainfrom
fix/5744_local-vllm-inference-gateway

Conversation

@chengjiew

@chengjiew chengjiew commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Routes loopback OpenAI-compatible endpoints through the OpenShell host alias when registering gateway inference, while keeping host-side endpoint validation against the user-provided localhost URL. This fixes sandbox inference.local 503s for local vLLM-style endpoints.

Related Issue

Fixes #5744

Changes

  • Rewrite http://localhost:<port>/..., 127.0.0.1, and ::1 compatible endpoint URLs to host.openshell.internal:<port> for gateway registration.
  • Skip OpenShell's gateway verify only for this loopback rewrite because the host-side smoke already validates the original endpoint.
  • Add a regression test for the compatible-endpoint registration args and document the container-reachable binding requirement.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Self-reviewed; scoped to compatible-endpoint gateway URL registration, backed by targeted regression test and live before/after sandbox repro on aits-log-worker-6.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

  • PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run check:diff passed when hooks were skipped or unavailable
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — npm_config_cache=/Users/wangchengjie/Project/NemoClaw-issue-5744/.npm-cache npx vitest run src/lib/onboard/inference-providers/remote.test.ts src/lib/onboard/inference-selection-validation.test.ts src/lib/onboard/local-inference-route.test.ts src/lib/inference/local.test.ts → 4 files, 71 tests passed
  • Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result:
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Additional verification:

  • npm run typecheck:cli passed
  • npm run format:check passed
  • npm run docs passed with 0 errors and 2 existing Fern warnings
  • git diff --check passed
  • Live repro on aits@aits-log-worker-6:
    • Baseline with host http://localhost:8000/v1/models returning 200 produced sandbox https://inference.local/v1/modelsHTTP/1.1 503 Service Unavailable
    • Patched build with the same input produced sandbox https://inference.local/v1/modelsHTTP/1.1 200 OK

Signed-off-by: Chengjie Wang chengjiew@nvidia.com

Summary by CodeRabbit

  • New Features
    • Improved onboarding for local OpenAI-compatible inference endpoints by automatically routing supported loopback HTTP URLs through the gateway for container access.
    • Added verification handling so the system skips verification when the endpoint is rewritten for gateway use, and applies the appropriate CLI options for compatible endpoints.
  • Documentation
    • Clarified loopback endpoint validation, gateway routing expectations, and recommended host binding (--host 0.0.0.0).
    • Updated the raw model server example to bind on all interfaces for container-reachable access.
  • Tests
    • Added coverage for endpoint rewriting, gateway credential reuse behavior, and non-rewrite scenarios.

@github-code-quality

github-code-quality Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage remains at 96%, unchanged from the branch.

TypeScript / code-coverage/cli

The overall coverage in the branch remains at 77%, unchanged from the branch.

Show a code coverage summary of the most impacted files.
File 9dec1fb dc666e5 +/-
src/lib/messagi...ate-resolver.ts 78% 67% -11%
src/lib/runner.ts 83% 80% -3%
src/lib/adapters/http/probe.ts 91% 89% -2%
src/lib/security/redact.ts 98% 96% -2%
src/lib/onboard...iders/remote.ts 96% 94% -2%
src/lib/adapter...tp/curl-args.ts 99% 97% -2%
src/lib/credentials/store.ts 59% 61% +2%
src/lib/messagi.../persistence.ts 92% 95% +3%
src/lib/inference/config.ts 93% 98% +5%
src/lib/state/config-io.ts 88% 95% +7%

Updated July 10, 2026 11:56 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds gateway-reachable rewriting for eligible loopback compatible endpoints, tests rewrite and credential-reuse flows, and updates local inference setup documentation and examples.

Changes

Gateway-reachable endpoint rewrite

Layer / File(s) Summary
Gateway URL rewrite and provider integration
src/lib/onboard/inference-providers/remote.ts
Rewrites eligible loopback HTTP compatible endpoints to host.openshell.internal, persists the rewritten URL, and disables verification when it differs from the resolved endpoint.
Provider setup behavior tests
src/lib/onboard/inference-providers/remote.test.ts
Tests loopback variants, gateway credential reuse, non-rewrite cases, provider arguments, and verification flags.
Local inference routing documentation
docs/inference/local-compatible-inference-setup.mdx, test/inference-options-docs.test.ts
Documents gateway routing and container-reachable binding, updates the llama-server example, and adjusts documentation assertions.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant setupRemoteProviderInference
  participant gatewayReachableCompatibleEndpointUrl
  participant upsertProvider
  participant runOpenshell

  setupRemoteProviderInference->>gatewayReachableCompatibleEndpointUrl: Rewrite resolved endpoint URL
  gatewayReachableCompatibleEndpointUrl-->>setupRemoteProviderInference: Return gateway endpoint URL
  setupRemoteProviderInference->>upsertProvider: Register gateway endpoint
  setupRemoteProviderInference->>runOpenshell: Run inference set with verification flags
Loading

Suggested labels: area: providers, area: docs

Suggested reviewers: ericksoa

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: routing local compatible endpoints through the gateway.
Linked Issues check ✅ Passed The code rewrites loopback compatible endpoints for gateway registration while preserving host-side validation, which addresses #5744's 503 proxying failure.
Out of Scope Changes check ✅ Passed The added tests and documentation directly support the gateway-routing fix and do not introduce unrelated changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/5744_local-vllm-inference-gateway

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: inference-routing, network-policy, onboard-repair, onboard-resume, cloud-onboard
Optional E2E: None

Dispatch hint: inference-routing,network-policy,onboard-repair,onboard-resume

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • inference-routing: Required by the deterministic risk plan for inference-policy changes. It validates that selected providers are reachable through the route advertised to the agent and catches mismatches between host validation and sandbox runtime routing.
  • network-policy: Required by the deterministic risk plan for inference-policy/network-boundary changes. The compatible-endpoint host bridge rewrite must be validated against real allow/deny network policy behavior.
  • onboard-repair: Required by the deterministic risk plan for lifecycle-state changes. It verifies onboarding repair converges persisted metadata, provider/gateway state, and live sandbox runtime state after partial or stale provider setup.
  • onboard-resume: Required by the deterministic risk plan for lifecycle-state changes. It validates that interrupted onboarding and resumed provider configuration do not leave stale routes, credentials, or sandbox metadata.
  • cloud-onboard (high): Changed onboard, trace timing, scorecard, or E2E workflow code can affect cloud onboard wall-clock behavior and should refresh the trusted cloud-onboard trace timing signal.

Optional E2E

  • None.

New E2E recommendations

  • None.

Dispatch hint

  • Workflow: .github/workflows/e2e.yaml
  • jobs input: inference-routing,network-policy,onboard-repair,onboard-resume

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

E2E Target Recommendation

Required E2E targets: inference-routing, network-policy, onboard-repair, onboard-resume
Optional E2E targets: None

Dispatch required E2E targets:

  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=inference-routing
  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=network-policy
  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=onboard-repair
  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=onboard-resume

Workflow run

Full E2E target advisor summary

E2E Target Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E targets

  • inference-routing: Inference selection, reachability, and network policy must agree at the real host-to-sandbox boundary.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=inference-routing
  • network-policy: Inference selection, reachability, and network policy must agree at the real host-to-sandbox boundary.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=network-policy
  • onboard-repair: Onboarding and sandbox state must converge across persisted metadata, reported status, and the live runtime.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=onboard-repair
  • onboard-resume: Onboarding and sandbox state must converge across persisted metadata, reported status, and the live runtime.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=onboard-resume

Optional E2E targets

  • None.

Relevant changed files

  • src/lib/onboard/inference-providers/remote.ts

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Fix PRA-1: Topology-dependent rewrite applied without topology detection; then add or justify PRA-T1.
Open items: 11 required · 4 warnings · 1 suggestion · 8 test follow-ups
Since last review: 0 prior items resolved · 10 still apply · 7 new items found

Action checklist

  • PRA-1 Fix: Topology-dependent rewrite applied without topology detection in src/lib/onboard/inference-providers/remote.ts:11
  • PRA-2 Fix: --no-verify injection without host-side probe success validation in src/lib/onboard/inference-providers/remote.ts:220
  • PRA-4 Fix: PR claims to fix [Linux][Inference] nemoclaw sandbox returns 503 when proxying inference requests to local vLLM custom endpoint via inference.local gateway #5744 but fix is inert on reported topology in src/lib/onboard/inference-providers/remote.ts:11
  • PRA-5 Fix: Silent catch swallows URL parse errors breaking caller contract in src/lib/onboard/inference-providers/remote.ts:33
  • PRA-6 Fix: State transition divergence: registered URL differs from verified URL in src/lib/onboard/inference-providers/remote.ts:220
  • PRA-11 Fix: Workaround lacks regression test proving source cannot regress and vague removal condition in src/lib/onboard/inference-providers/remote.ts:11
  • PRA-12 Fix: Security boundary bypass: --no-verify injected on URL pattern alone without topology detection or probe success in src/lib/onboard/inference-providers/remote.ts:220
  • PRA-13 Fix: No direct unit tests for gatewayReachableCompatibleEndpointUrl function in src/lib/onboard/inference-providers/remote.ts:11
  • PRA-14 Fix: Missing security boundary negative tests for gateway rewrite in test/onboard-inference-gateway-scope.test.ts:1
  • PRA-15 Fix: No test for ghost provider cleanup on inference set failure in src/lib/onboard/inference-providers/remote.ts:235
  • PRA-7 Resolve or justify: Documentation claims topology-dependent rewrite but specifies no validated topologies in docs/inference/local-compatible-inference-setup.mdx:168
  • PRA-8 Resolve or justify: Credential reuse path bypasses rewrite validation in src/lib/onboard/inference-providers/remote.ts:185
  • PRA-9 Resolve or justify: IPv6 zone ID handling produces invalid rewritten hostname in src/lib/onboard/inference-providers/remote.ts:35
  • PRA-10 Resolve or justify: No rollback/cleanup on inference set failure after provider create in src/lib/onboard/inference-providers/remote.ts:235
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: No direct unit tests for gatewayReachableCompatibleEndpointUrl function
  • PRA-T6 Add or justify test follow-up: Missing security boundary negative tests for gateway rewrite
  • PRA-T7 Add or justify test follow-up: No test for ghost provider cleanup on inference set failure
  • PRA-T8 Add or justify test follow-up: Test verifies smoke against original URL not rewritten gateway URL
  • PRA-3 In-scope improvement: Test over-scaffold: full registry mock for single scenario in test/onboard-inference-gateway-scope.test.ts:22

Findings index

ID Severity Category Location Required action
PRA-1 Required architecture src/lib/onboard/inference-providers/remote.ts:11 Add topology detection (OpenShell version/driver check) before applying the rewrite and --no-verify. If topology is unsupported, skip rewrite and do not inject --no-verify. Alternatively, make the rewrite opt-in via config until CI validates target topologies.
PRA-2 Required architecture src/lib/onboard/inference-providers/remote.ts:220 Gate --no-verify injection behind both the URL rewrite condition AND a successful host-side reachability probe. If probe fails, do not inject --no-verify and fail onboarding with actionable error.
PRA-3 Improvement architecture test/onboard-inference-gateway-scope.test.ts:22 Simplify the test harness to only mock the dependencies actually exercised by the test cases. Remove unused registry mock or split into focused test files if multiple scenarios need different mocks.
PRA-4 Required acceptance src/lib/onboard/inference-providers/remote.ts:11 Either: (1) update issue #5744 to clarify which topologies this fix applies to and document them, (2) add topology detection and gate the rewrite behind it, or (3) remove the rewrite if the target topology is not the supported one. Do not merge claiming 'Fixes #5744' without topology qualification.
PRA-5 Required correctness src/lib/onboard/inference-providers/remote.ts:33 Either let the parse error propagate with a clear message, or explicitly validate URL format before the try block and throw a descriptive error. Add logging to distinguish parse failures from intentional non-rewrite cases.
PRA-6 Required correctness src/lib/onboard/inference-providers/remote.ts:220 Either verify against the registered gateway URL, or ensure the host-side verification actually exercises the gateway path. The test should assert that verifyOnboardInferenceSmoke is called with the correct URL for the topology.
PRA-7 Resolve/justify docs docs/inference/local-compatible-inference-setup.mdx:168 Add explicit topology qualification: specify which OpenShell version/driver combinations are CI-validated. Add a version badge or link to the CI job that validates the topology. If only 0.0.72 is known working, state that clearly.
PRA-8 Resolve/justify correctness src/lib/onboard/inference-providers/remote.ts:185 Add validation that the existing provider's configuration is compatible with the rewrite before applying it. Or skip the rewrite in credential reuse mode and use the existing provider URL as-is.
PRA-9 Resolve/justify correctness src/lib/onboard/inference-providers/remote.ts:35 Either reject IPv6 zone IDs explicitly in the loopback check, or strip zone IDs before rewrite. Add test cases for IPv6 with zone IDs.
PRA-10 Resolve/justify correctness src/lib/onboard/inference-providers/remote.ts:235 Add cleanup logic in the catch/retry path: if inference set fails and a provider was created in this attempt, delete it before retrying. Or track created providers and clean up on final failure.
PRA-11 Required security src/lib/onboard/inference-providers/remote.ts:11 Add a regression test that validates the gateway bridge behavior this workaround depends on. Document a concrete removal condition tied to an OpenShell version/feature flag. Consider making the rewrite opt-in via config until the bridge behavior is stable and tested.
PRA-12 Required security src/lib/onboard/inference-providers/remote.ts:220 Gate --no-verify injection behind BOTH: (1) topology detection confirming the rewrite is effective on this OpenShell version/driver, AND (2) a successful host-side reachability probe that exercises the actual gateway path. If either check fails, do not inject --no-verify and fail onboarding with actionable error.
PRA-13 Required tests src/lib/onboard/inference-providers/remote.ts:11 Add focused unit tests for gatewayReachableCompatibleEndpointUrl covering: valid rewrite cases (localhost, 127.0.0.1, ::1 on various ports), negative cases (privileged ports <1024, non-loopback hosts, HTTPS, URLs with auth, malformed URLs), IPv6 zone ID handling, and parse error behavior. Each case should assert the exact return value (rewritten URL, original URL, or error).
PRA-14 Required tests test/onboard-inference-gateway-scope.test.ts:1 Add negative test cases to test/onboard-inference-gateway-scope.test.ts (or a new focused test file) for each security boundary. Each must assert: rewrite is blocked or fails with appropriate error, and --no-verify is NOT injected in the inference set command.
PRA-15 Required tests src/lib/onboard/inference-providers/remote.ts:235 Add test: mock inference set failure after provider create → verify provider is cleaned up (deleted) before retry or on final failure. This may require refactoring the onboarding loop to track created providers for cleanup.
PRA-16 Required tests src/lib/onboard/inference-providers/remote.ts:220 Either: (1) update the test to verify against the gateway URL when rewrite occurs, and add a host-side probe that exercises the gateway path; or (2) add a separate integration test that validates the full gateway route works at runtime. The current test should at minimum assert the correct URL is passed to verifyOnboardInferenceSmoke for the topology.

🚨 Required before merge

Address these before merging unless a maintainer explicitly overrides the advisor with rationale.

PRA-1 Required — Topology-dependent rewrite applied without topology detection

  • Location: src/lib/onboard/inference-providers/remote.ts:11
  • Category: architecture
  • Problem: The gatewayReachableCompatibleEndpointUrl() function rewrites loopback URLs to host.openshell.internal and injects --no-verify, but no OpenShell version/driver detection occurs before applying this rewrite. NV QA confirmed the rewrite is inert on OpenShell 0.0.44 native Docker Engine (the reported topology for [Linux][Inference] nemoclaw sandbox returns 503 when proxying inference requests to local vLLM custom endpoint via inference.local gateway #5744). On unsupported topologies, the rewrite produces a sandbox-internal hostname that may not resolve, and --no-verify bypasses host-side verification.
  • Impact: Users on unsupported topologies get broken onboarding or silent failures with reduced security. The fix claimed for [Linux][Inference] nemoclaw sandbox returns 503 when proxying inference requests to local vLLM custom endpoint via inference.local gateway #5744 does not work on the reported topology.
  • Required action: Add topology detection (OpenShell version/driver check) before applying the rewrite and --no-verify. If topology is unsupported, skip rewrite and do not inject --no-verify. Alternatively, make the rewrite opt-in via config until CI validates target topologies.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read remote.ts lines 11-60 (gatewayReachableCompatibleEndpointUrl) and lines 220-230 (--no-verify injection). Verify no topology detection exists before rewrite.
  • Missing regression test: CI integration test: onboard compatible-endpoint with http://localhost:8000/v1 against vLLM bound to 0.0.0.0 on target topology, verify sandbox inference.local returns 200. Document the target topology in the test.
  • Done when: The required change is committed and verification passes: Read remote.ts lines 11-60 (gatewayReachableCompatibleEndpointUrl) and lines 220-230 (--no-verify injection). Verify no topology detection exists before rewrite.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:11-45 src/lib/onboard/inference-providers/remote.ts:220-230

PRA-2 Required — --no-verify injection without host-side probe success validation

  • Location: src/lib/onboard/inference-providers/remote.ts:220
  • Category: architecture
  • Problem: The --no-verify flag is injected whenever gatewayEndpointUrl !== resolvedEndpointUrl (i.e., whenever the rewrite occurs). There is no host-side reachability probe that must succeed before --no-verify is added. If the host bridge is unavailable, onboarding proceeds with --no-verify but the sandbox route will fail.
  • Impact: Security boundary bypass: --no-verify skips host-side verification of the inference route. If the gateway bridge is broken, users get a false-success onboarding that fails at runtime with no clear diagnostics.
  • Required action: Gate --no-verify injection behind both the URL rewrite condition AND a successful host-side reachability probe. If probe fails, do not inject --no-verify and fail onboarding with actionable error.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read remote.ts lines 220-230. Verify --no-verify condition is only (config.skipVerify || gatewayEndpointUrl !== resolvedEndpointUrl).
  • Missing regression test: Test: mock host-side probe failure → --no-verify not injected, onboarding fails with actionable error. Test: mock probe success → --no-verify injected.
  • Done when: The required change is committed and verification passes: Read remote.ts lines 220-230. Verify --no-verify condition is only (config.skipVerify || gatewayEndpointUrl !== resolvedEndpointUrl).
  • Evidence: src/lib/onboard/inference-providers/remote.ts:220-230

PRA-4 Required — PR claims to fix #5744 but fix is inert on reported topology

PRA-5 Required — Silent catch swallows URL parse errors breaking caller contract

  • Location: src/lib/onboard/inference-providers/remote.ts:33
  • Category: correctness
  • Problem: gatewayReachableCompatibleEndpointUrl() wraps new URL() in try/catch and returns endpointUrl unchanged on any parse error. The caller treats the return value as either a rewritten URL or the original valid URL. A malformed URL (e.g., 'not-a-url', 'http://', 'http://[invalid') silently passes through as 'valid', breaking the contract that the function validates and rewrites URLs.
  • Impact: Invalid endpoint configurations fail silently instead of producing actionable errors. Users may not realize their endpoint URL is malformed. The onboarding flow proceeds with a garbage URL.
  • Required action: Either let the parse error propagate with a clear message, or explicitly validate URL format before the try block and throw a descriptive error. Add logging to distinguish parse failures from intentional non-rewrite cases.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read remote.ts lines 18-25. Verify the catch block returns endpointUrl without distinction.
  • Missing regression test: Test case: malformed URL passed to gatewayReachableCompatibleEndpointUrl → should throw or return structured error, not silently return original string.
  • Done when: The required change is committed and verification passes: Read remote.ts lines 18-25. Verify the catch block returns endpointUrl without distinction.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:18-25

PRA-6 Required — State transition divergence: registered URL differs from verified URL

  • Location: src/lib/onboard/inference-providers/remote.ts:220
  • Category: correctness
  • Problem: The onboarding flow registers the rewritten gateway URL (host.openshell.internal) via 'provider create' but verifies the smoke test against the original user-provided URL (localhost). The test at line 75-80 of onboard-inference-gateway-scope.test.ts confirms this: verifyOnboardInferenceSmoke is called with the original endpointUrl, not gatewayEndpointUrl. This creates a divergence where the registered route and the verified route differ.
  • Impact: Onboarding may pass verification (host-side localhost works) but the registered gateway route points to host.openshell.internal which may not resolve in the sandbox context. Users get false-success onboarding that fails at runtime.
  • Required action: Either verify against the registered gateway URL, or ensure the host-side verification actually exercises the gateway path. The test should assert that verifyOnboardInferenceSmoke is called with the correct URL for the topology.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read remote.ts lines 220-240. Compare gatewayEndpointUrl (rewritten) passed to upsertProvider vs endpointUrl (original) passed to verifyOnboardInferenceSmoke. Check test/onboard-inference-gateway-scope.test.ts lines 75-80.
  • Missing regression test: Test verifying that the URL used for gateway registration matches the URL verified at runtime, or that host-side verification exercises the actual gateway route.
  • Done when: The required change is committed and verification passes: Read remote.ts lines 220-240. Compare gatewayEndpointUrl (rewritten) passed to upsertProvider vs endpointUrl (original) passed to verifyOnboardInferenceSmoke. Check test/onboard-inference-gateway-scope.test.ts lines 75-80.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:220-240 test/onboard-inference-gateway-scope.test.ts:75-80

PRA-11 Required — Workaround lacks regression test proving source cannot regress and vague removal condition

  • Location: src/lib/onboard/inference-providers/remote.ts:11
  • Category: security
  • Problem: The gatewayReachableCompatibleEndpointUrl() workaround (comment line 11: 'Remove this when OpenShell can verify provider routes from the sandbox/gateway network context') fails the source-of-truth review: (1) invalid state = host loopback URL unreachable from sandbox; (2) source boundary = OpenShell gateway bridge does not resolve host.openshell.internal from host-side verifier context; (3) why not fixed here = OpenShell owns gateway bridge (plausible); (4) regression test proving source cannot regress = MISSING; (5) removal condition = vague ('when OpenShell can verify'). No test locks in the gateway bridge behavior that makes this workaround necessary.
  • Impact: The workaround may become permanently embedded without a trigger for removal. If the OpenShell gateway bridge behavior changes (e.g., starts resolving host.openshell.internal from host context), the rewrite becomes incorrect but no test will detect it. Conversely, if the bridge stops working on a topology, the rewrite silently produces broken routes.
  • Required action: Add a regression test that validates the gateway bridge behavior this workaround depends on. Document a concrete removal condition tied to an OpenShell version/feature flag. Consider making the rewrite opt-in via config until the bridge behavior is stable and tested.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read remote.ts lines 11-15 (workaround comment). Search for any test that validates host.openshell.internal resolution behavior from host vs sandbox contexts.
  • Missing regression test: Test that validates: from host-side verifier context, host.openshell.internal resolves to the gateway bridge IP; from sandbox context, it resolves to the same. This test should fail if the bridge behavior changes, triggering workaround review.
  • Done when: The required change is committed and verification passes: Read remote.ts lines 11-15 (workaround comment). Search for any test that validates host.openshell.internal resolution behavior from host vs sandbox contexts.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:11-15

PRA-12 Required — Security boundary bypass: --no-verify injected on URL pattern alone without topology detection or probe success

  • Location: src/lib/onboard/inference-providers/remote.ts:220
  • Category: security
  • Problem: The --no-verify flag (which skips OpenShell's gateway route verification) is injected whenever the URL rewrite occurs (gatewayEndpointUrl !== resolvedEndpointUrl). This decision is based solely on the URL pattern (HTTP loopback on unprivileged port), with NO topology detection (OpenShell version/driver check) and NO requirement that a host-side reachability probe succeeds. On unsupported topologies, this bypasses the security check that the gateway route is actually functional.
  • Impact: Users on unsupported topologies get a false-success onboarding: the provider is registered with a rewritten URL that may not resolve in the sandbox, and --no-verify skips the verification that would catch this. The security boundary (gateway route verification) is bypassed based on a client-side URL pattern match.
  • Required action: Gate --no-verify injection behind BOTH: (1) topology detection confirming the rewrite is effective on this OpenShell version/driver, AND (2) a successful host-side reachability probe that exercises the actual gateway path. If either check fails, do not inject --no-verify and fail onboarding with actionable error.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read remote.ts lines 220-230. Verify --no-verify condition is only (config.skipVerify || gatewayEndpointUrl !== resolvedEndpointUrl) with no topology check or probe success gate.
  • Missing regression test: Test: on unsupported topology (mocked), loopback URL → no rewrite, no --no-verify. Test: on supported topology but host probe fails → rewrite occurs but no --no-verify, onboarding fails. Test: on supported topology with probe success → rewrite + --no-verify.
  • Done when: The required change is committed and verification passes: Read remote.ts lines 220-230. Verify --no-verify condition is only (config.skipVerify || gatewayEndpointUrl !== resolvedEndpointUrl) with no topology check or probe success gate.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:220-230

PRA-13 Required — No direct unit tests for gatewayReachableCompatibleEndpointUrl function

  • Location: src/lib/onboard/inference-providers/remote.ts:11
  • Category: tests
  • Problem: The new gatewayReachableCompatibleEndpointUrl() function (lines 11-45) has zero direct unit tests. The only coverage is indirect via the gateway-scope integration test which mocks runOpenshell and asserts command args. Critical behaviors lack unit test coverage: parse error handling, IPv6 zone IDs, privileged port boundary (1023 vs 1024), non-loopback hosts, non-HTTP protocols, URLs with authentication, and port parsing edge cases.
  • Impact: Behavioral regressions in URL parsing, loopback detection, and rewrite logic will not be caught by unit tests. The function is security-sensitive (controls --no-verify injection) but only tested through a single happy-path integration test.
  • Required action: Add focused unit tests for gatewayReachableCompatibleEndpointUrl covering: valid rewrite cases (localhost, 127.0.0.1, ::1 on various ports), negative cases (privileged ports <1024, non-loopback hosts, HTTPS, URLs with auth, malformed URLs), IPv6 zone ID handling, and parse error behavior. Each case should assert the exact return value (rewritten URL, original URL, or error).
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Search test/ directory for any test file that directly imports and calls gatewayReachableCompatibleEndpointUrl. Verify coverage of the cases listed above.
  • Missing regression test: Unit test suite for gatewayReachableCompatibleEndpointUrl with 15+ cases covering all URL parsing, loopback detection, port validation, and rewrite branches.
  • Done when: The required change is committed and verification passes: Search test/ directory for any test file that directly imports and calls gatewayReachableCompatibleEndpointUrl. Verify coverage of the cases listed above.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:11-45 test/onboard-inference-gateway-scope.test.ts:45-85 (only indirect coverage)

PRA-14 Required — Missing security boundary negative tests for gateway rewrite

  • Location: test/onboard-inference-gateway-scope.test.ts:1
  • Category: tests
  • Problem: The new test 'registers loopback compatible endpoints through the gateway alias but keeps host smoke local' covers only the happy path (http://localhost:8000/v1\). Critical security boundaries lack test coverage: (1) privileged port 1023 must NOT rewrite (IPv4 and IPv6); (2) IPv6 loopback with zone ID (::1%eth0) must be rejected or handled; (3) malformed URLs must fail explicitly, not silently pass; (4) host-side probe failure must NOT inject --no-verify; (5) topology detection guard (when added) must block rewrite on unsupported topologies.
  • Impact: Without these tests, regressions could allow privileged port rewrites (exposing privileged services), produce invalid hostnames from IPv6 zone IDs, silently accept malformed URLs, or inject --no-verify when the gateway bridge is down — all bypassing security checks.
  • Required action: Add negative test cases to test/onboard-inference-gateway-scope.test.ts (or a new focused test file) for each security boundary. Each must assert: rewrite is blocked or fails with appropriate error, and --no-verify is NOT injected in the inference set command.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read test/onboard-inference-gateway-scope.test.ts lines 45-85. Verify no test cases for port 1023, IPv6 ::1, zone IDs, malformed URLs, or host-probe failure.
  • Missing regression test: Five negative test cases: (1) http://localhost:1023/v1 → no rewrite, no --no-verify; (2) http://[::1]:1023/v1 → no rewrite, no --no-verify; (3) http://[::1%eth0]:8000/v1 → reject or strip zone ID; (4) malformed URL → explicit error; (5) host probe failure → no --no-verify, actionable error.
  • Done when: The required change is committed and verification passes: Read test/onboard-inference-gateway-scope.test.ts lines 45-85. Verify no test cases for port 1023, IPv6 ::1, zone IDs, malformed URLs, or host-probe failure.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:30-45 src/lib/onboard/inference-providers/remote.ts:220-230 test/onboard-inference-gateway-scope.test.ts:45-85

PRA-15 Required — No test for ghost provider cleanup on inference set failure

  • Location: src/lib/onboard/inference-providers/remote.ts:235
  • Category: tests
  • Problem: The onboarding loop (lines 235-255) creates/upserts a provider, then calls inference set. If inference set fails, the loop retries but the provider remains registered with the rewritten URL. The risk plan invariant 'partial failure and retry converge without ghost resources' (lifecycle-state family) has no test evidence. No test mocks inference set failure and verifies provider cleanup.
  • Impact: Failed onboarding attempts leave stale provider registrations with rewritten URLs. Subsequent onboarding may conflict with ghost providers or users see unexpected provider entries. The lifecycle-state invariant is unvalidated.
  • Required action: Add test: mock inference set failure after provider create → verify provider is cleaned up (deleted) before retry or on final failure. This may require refactoring the onboarding loop to track created providers for cleanup.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read remote.ts lines 235-255. Check test/onboard-inference-gateway-scope.test.ts and any remote.test.ts for provider cleanup on failure.
  • Missing regression test: Test: mock inference set failure after provider create → verify provider delete is called, no ghost provider remains registered.
  • Done when: The required change is committed and verification passes: Read remote.ts lines 235-255. Check test/onboard-inference-gateway-scope.test.ts and any remote.test.ts for provider cleanup on failure.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:235-255 test/onboard-inference-gateway-scope.test.ts

PRA-16 Required — Test verifies smoke against original URL not rewritten gateway URL

  • Location: src/lib/onboard/inference-providers/remote.ts:220
  • Category: tests
  • Problem: The new test (lines 75-80 of onboard-inference-gateway-scope.test.ts) asserts verifyOnboardInferenceSmoke is called with the original endpointUrl (localhost:8000), not the rewritten gatewayEndpointUrl (host.openshell.internal:8000). This matches the current code behavior but validates the divergence (F-006): the registered route and verified route differ. The risk plan invariant 'the selected provider is reachable through the route advertised to the agent' (inference-policy) requires runtime validation that the gateway route works, not just the host-side localhost.
  • Impact: The test passes but validates incorrect behavior: onboarding verifies the host-side URL while registering a different gateway URL. If the gateway bridge is broken, onboarding succeeds but runtime inference fails. The required E2E jobs (inference-routing, network-policy) would catch this but have no checked-in test coverage.
  • Required action: Either: (1) update the test to verify against the gateway URL when rewrite occurs, and add a host-side probe that exercises the gateway path; or (2) add a separate integration test that validates the full gateway route works at runtime. The current test should at minimum assert the correct URL is passed to verifyOnboardInferenceSmoke for the topology.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read remote.ts lines 220-240. Compare gatewayEndpointUrl passed to upsertProvider vs endpointUrl passed to verifyOnboardInferenceSmoke. Check test/onboard-inference-gateway-scope.test.ts lines 75-80.
  • Missing regression test: Integration test: onboard with loopback URL on validated topology → verify sandbox inference.local returns 200 (proves gateway route works). Unit test: verifyOnboardInferenceSmoke called with gatewayEndpointUrl when rewrite occurs.
  • Done when: The required change is committed and verification passes: Read remote.ts lines 220-240. Compare gatewayEndpointUrl passed to upsertProvider vs endpointUrl passed to verifyOnboardInferenceSmoke. Check test/onboard-inference-gateway-scope.test.ts lines 75-80.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:220-240 test/onboard-inference-gateway-scope.test.ts:75-80
Review findings by urgency: 11 required fixes, 4 items to resolve/justify, 1 in-scope improvement

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-7 Resolve/justify — Documentation claims topology-dependent rewrite but specifies no validated topologies

  • Location: docs/inference/local-compatible-inference-setup.mdx:168
  • Category: docs
  • Problem: The updated docs state 'This rewrite depends on an OpenShell topology that resolves host.openshell.internal inside the sandbox' but do not specify which OpenShell versions/drivers are validated. Evidence shows the rewrite is inert on 0.0.44 (reported topology) and only works on 0.0.72 (unvalidated in CI). No version matrix, CI badge, or topology qualification is documented.
  • Impact: Users cannot determine if their topology supports the rewrite. The docs imply universal applicability with a caveat, but the behavior is topology-specific with only one anecdotal working topology.
  • Recommended action: Add explicit topology qualification: specify which OpenShell version/driver combinations are CI-validated. Add a version badge or link to the CI job that validates the topology. If only 0.0.72 is known working, state that clearly.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read docs/inference/local-compatible-inference-setup.mdx lines 168-180. Check for version matrix, CI badge, or specific topology identification.
  • Missing regression test: N/A - documentation accuracy issue. Add CI job that validates docs claims against actual topology.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read docs/inference/local-compatible-inference-setup.mdx lines 168-180. Check for version matrix, CI badge, or specific topology identification.
  • Evidence: docs/inference/local-compatible-inference-setup.mdx:168-180

PRA-8 Resolve/justify — Credential reuse path bypasses rewrite validation

  • Location: src/lib/onboard/inference-providers/remote.ts:185
  • Category: correctness
  • Problem: When reuseGatewayCredentialWithoutLocalKey is true (lines 185-195), the code compares gatewayEndpointUrl !== resolvedEndpointUrl and calls upsertProvider with the rewritten URL. However, it does not validate that the existing provider's topology matches the rewrite assumption. If a provider was previously registered with a different URL (e.g., manually entered host.openshell.internal), the rewrite may be inappropriate but is applied anyway.
  • Impact: Existing providers may be silently updated with a rewritten URL that doesn't match their actual topology, breaking inference routing for those providers.
  • Recommended action: Add validation that the existing provider's configuration is compatible with the rewrite before applying it. Or skip the rewrite in credential reuse mode and use the existing provider URL as-is.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read remote.ts lines 185-195. Verify no topology compatibility check before upsertProvider with gatewayEndpointUrl.
  • Missing regression test: Test: existing provider registered with host.openshell.internal URL, onboarding with localhost URL and reuseGatewayCredentialWithoutLocalKey → should not rewrite existing provider.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read remote.ts lines 185-195. Verify no topology compatibility check before upsertProvider with gatewayEndpointUrl.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:185-195

PRA-9 Resolve/justify — IPv6 zone ID handling produces invalid rewritten hostname

  • Location: src/lib/onboard/inference-providers/remote.ts:35
  • Category: correctness
  • Problem: The hostname normalization at line 35 (hostname.replace(/^\[|\]$/g, '')) strips brackets but does not handle IPv6 zone IDs (e.g., '::1%eth0'). A URL like 'http://[::1%eth0]:8000/v1' would pass the loopback check (hostname becomes '::1%eth0', which includes '::1') but produce 'host.openshell.internal%eth0' after rewrite — an invalid hostname.
  • Impact: IPv6 addresses with zone IDs (valid in some container networking setups) produce malformed gateway URLs that will fail to resolve.
  • Recommended action: Either reject IPv6 zone IDs explicitly in the loopback check, or strip zone IDs before rewrite. Add test cases for IPv6 with zone IDs.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read remote.ts lines 30-45. Check if hostname.includes('%') is handled. Test with 'http://[::1%eth0]:8000/v1'.
  • Missing regression test: Test case: IPv6 loopback with zone ID (e.g., [::1%eth0]:8000) → should either rewrite cleanly to host.openshell.internal or reject with clear error.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read remote.ts lines 30-45. Check if hostname.includes('%') is handled. Test with 'http://[::1%eth0]:8000/v1'.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:30-45

PRA-10 Resolve/justify — No rollback/cleanup on inference set failure after provider create

  • Location: src/lib/onboard/inference-providers/remote.ts:235
  • Category: correctness
  • Problem: The onboarding loop (lines 235-255) calls provider create/upsert, then inference set. If inference set fails, the loop retries but the provider remains registered with the rewritten URL. There is no cleanup or rollback of the provider registration on failure. This can leave ghost providers in OpenShell.
  • Impact: Failed onboarding attempts leave stale provider registrations with rewritten URLs. Subsequent onboarding attempts may conflict with these ghost providers or users may see unexpected provider entries.
  • Recommended action: Add cleanup logic in the catch/retry path: if inference set fails and a provider was created in this attempt, delete it before retrying. Or track created providers and clean up on final failure.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read remote.ts lines 235-255. Verify no provider deletion on inference set failure.
  • Missing regression test: Test: mock inference set failure after provider create → verify provider is cleaned up, not left registered.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read remote.ts lines 235-255. Verify no provider deletion on inference set failure.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:235-255

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

PRA-3 Improvement — Test over-scaffold: full registry mock for single scenario

  • Location: test/onboard-inference-gateway-scope.test.ts:22
  • Category: architecture
  • Problem: The test harness uses createDirectSetupInferenceHarnessFactory which appears to mock the full registry and command router, but the new test only exercises a single happy-path scenario. The mock surface is larger than needed for the tested behavior.
  • Impact: Test maintenance burden and reduced readability. Over-mocking can mask integration issues that a lighter harness would expose.
  • Suggested action: Simplify the test harness to only mock the dependencies actually exercised by the test cases. Remove unused registry mock or split into focused test files if multiple scenarios need different mocks.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read test/support/setup-inference-test-harness.ts and test/onboard-inference-gateway-scope.test.ts. Identify which mocked dependencies are actually used by the new test.
  • Missing regression test: N/A - simplification improvement
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: test/onboard-inference-gateway-scope.test.ts:22 test/support/setup-inference-test-harness.ts
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — Run the `inference-routing` E2E job for Inference selection, reachability, and network policy must agree at the real host-to-sandbox boundary. Matched files: `src/lib/onboard/inference-providers/remote.ts`.. Deterministic regression risks require live validation: lifecycle-state, inference-policy.
  • PRA-T2 Runtime validation — Run the `network-policy` E2E job for Inference selection, reachability, and network policy must agree at the real host-to-sandbox boundary. Matched files: `src/lib/onboard/inference-providers/remote.ts`.. Deterministic regression risks require live validation: lifecycle-state, inference-policy.
  • PRA-T3 Runtime validation — Run the `onboard-repair` E2E job for Onboarding and sandbox state must converge across persisted metadata, reported status, and the live runtime. Matched files: `src/lib/onboard/inference-providers/remote.ts`.. Deterministic regression risks require live validation: lifecycle-state, inference-policy.
  • PRA-T4 Runtime validation — Run the `onboard-resume` E2E job for Onboarding and sandbox state must converge across persisted metadata, reported status, and the live runtime. Matched files: `src/lib/onboard/inference-providers/remote.ts`.. Deterministic regression risks require live validation: lifecycle-state, inference-policy.
  • PRA-T5 No direct unit tests for gatewayReachableCompatibleEndpointUrl function — Add focused unit tests for gatewayReachableCompatibleEndpointUrl covering: valid rewrite cases (localhost, 127.0.0.1, ::1 on various ports), negative cases (privileged ports <1024, non-loopback hosts, HTTPS, URLs with auth, malformed URLs), IPv6 zone ID handling, and parse error behavior. Each case should assert the exact return value (rewritten URL, original URL, or error).
  • PRA-T6 Missing security boundary negative tests for gateway rewrite — Add negative test cases to test/onboard-inference-gateway-scope.test.ts (or a new focused test file) for each security boundary. Each must assert: rewrite is blocked or fails with appropriate error, and --no-verify is NOT injected in the inference set command.
  • PRA-T7 No test for ghost provider cleanup on inference set failure — Add test: mock inference set failure after provider create → verify provider is cleaned up (deleted) before retry or on final failure. This may require refactoring the onboarding loop to track created providers for cleanup.
  • PRA-T8 Test verifies smoke against original URL not rewritten gateway URL — Either: (1) update the test to verify against the gateway URL when rewrite occurs, and add a host-side probe that exercises the gateway path; or (2) add a separate integration test that validates the full gateway route works at runtime. The current test should at minimum assert the correct URL is passed to verifyOnboardInferenceSmoke for the topology.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Required — Topology-dependent rewrite applied without topology detection

  • Location: src/lib/onboard/inference-providers/remote.ts:11
  • Category: architecture
  • Problem: The gatewayReachableCompatibleEndpointUrl() function rewrites loopback URLs to host.openshell.internal and injects --no-verify, but no OpenShell version/driver detection occurs before applying this rewrite. NV QA confirmed the rewrite is inert on OpenShell 0.0.44 native Docker Engine (the reported topology for [Linux][Inference] nemoclaw sandbox returns 503 when proxying inference requests to local vLLM custom endpoint via inference.local gateway #5744). On unsupported topologies, the rewrite produces a sandbox-internal hostname that may not resolve, and --no-verify bypasses host-side verification.
  • Impact: Users on unsupported topologies get broken onboarding or silent failures with reduced security. The fix claimed for [Linux][Inference] nemoclaw sandbox returns 503 when proxying inference requests to local vLLM custom endpoint via inference.local gateway #5744 does not work on the reported topology.
  • Required action: Add topology detection (OpenShell version/driver check) before applying the rewrite and --no-verify. If topology is unsupported, skip rewrite and do not inject --no-verify. Alternatively, make the rewrite opt-in via config until CI validates target topologies.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read remote.ts lines 11-60 (gatewayReachableCompatibleEndpointUrl) and lines 220-230 (--no-verify injection). Verify no topology detection exists before rewrite.
  • Missing regression test: CI integration test: onboard compatible-endpoint with http://localhost:8000/v1 against vLLM bound to 0.0.0.0 on target topology, verify sandbox inference.local returns 200. Document the target topology in the test.
  • Done when: The required change is committed and verification passes: Read remote.ts lines 11-60 (gatewayReachableCompatibleEndpointUrl) and lines 220-230 (--no-verify injection). Verify no topology detection exists before rewrite.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:11-45 src/lib/onboard/inference-providers/remote.ts:220-230

PRA-2 Required — --no-verify injection without host-side probe success validation

  • Location: src/lib/onboard/inference-providers/remote.ts:220
  • Category: architecture
  • Problem: The --no-verify flag is injected whenever gatewayEndpointUrl !== resolvedEndpointUrl (i.e., whenever the rewrite occurs). There is no host-side reachability probe that must succeed before --no-verify is added. If the host bridge is unavailable, onboarding proceeds with --no-verify but the sandbox route will fail.
  • Impact: Security boundary bypass: --no-verify skips host-side verification of the inference route. If the gateway bridge is broken, users get a false-success onboarding that fails at runtime with no clear diagnostics.
  • Required action: Gate --no-verify injection behind both the URL rewrite condition AND a successful host-side reachability probe. If probe fails, do not inject --no-verify and fail onboarding with actionable error.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read remote.ts lines 220-230. Verify --no-verify condition is only (config.skipVerify || gatewayEndpointUrl !== resolvedEndpointUrl).
  • Missing regression test: Test: mock host-side probe failure → --no-verify not injected, onboarding fails with actionable error. Test: mock probe success → --no-verify injected.
  • Done when: The required change is committed and verification passes: Read remote.ts lines 220-230. Verify --no-verify condition is only (config.skipVerify || gatewayEndpointUrl !== resolvedEndpointUrl).
  • Evidence: src/lib/onboard/inference-providers/remote.ts:220-230

PRA-3 Improvement — Test over-scaffold: full registry mock for single scenario

  • Location: test/onboard-inference-gateway-scope.test.ts:22
  • Category: architecture
  • Problem: The test harness uses createDirectSetupInferenceHarnessFactory which appears to mock the full registry and command router, but the new test only exercises a single happy-path scenario. The mock surface is larger than needed for the tested behavior.
  • Impact: Test maintenance burden and reduced readability. Over-mocking can mask integration issues that a lighter harness would expose.
  • Suggested action: Simplify the test harness to only mock the dependencies actually exercised by the test cases. Remove unused registry mock or split into focused test files if multiple scenarios need different mocks.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read test/support/setup-inference-test-harness.ts and test/onboard-inference-gateway-scope.test.ts. Identify which mocked dependencies are actually used by the new test.
  • Missing regression test: N/A - simplification improvement
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: test/onboard-inference-gateway-scope.test.ts:22 test/support/setup-inference-test-harness.ts

PRA-4 Required — PR claims to fix #5744 but fix is inert on reported topology

PRA-5 Required — Silent catch swallows URL parse errors breaking caller contract

  • Location: src/lib/onboard/inference-providers/remote.ts:33
  • Category: correctness
  • Problem: gatewayReachableCompatibleEndpointUrl() wraps new URL() in try/catch and returns endpointUrl unchanged on any parse error. The caller treats the return value as either a rewritten URL or the original valid URL. A malformed URL (e.g., 'not-a-url', 'http://', 'http://[invalid') silently passes through as 'valid', breaking the contract that the function validates and rewrites URLs.
  • Impact: Invalid endpoint configurations fail silently instead of producing actionable errors. Users may not realize their endpoint URL is malformed. The onboarding flow proceeds with a garbage URL.
  • Required action: Either let the parse error propagate with a clear message, or explicitly validate URL format before the try block and throw a descriptive error. Add logging to distinguish parse failures from intentional non-rewrite cases.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read remote.ts lines 18-25. Verify the catch block returns endpointUrl without distinction.
  • Missing regression test: Test case: malformed URL passed to gatewayReachableCompatibleEndpointUrl → should throw or return structured error, not silently return original string.
  • Done when: The required change is committed and verification passes: Read remote.ts lines 18-25. Verify the catch block returns endpointUrl without distinction.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:18-25

PRA-6 Required — State transition divergence: registered URL differs from verified URL

  • Location: src/lib/onboard/inference-providers/remote.ts:220
  • Category: correctness
  • Problem: The onboarding flow registers the rewritten gateway URL (host.openshell.internal) via 'provider create' but verifies the smoke test against the original user-provided URL (localhost). The test at line 75-80 of onboard-inference-gateway-scope.test.ts confirms this: verifyOnboardInferenceSmoke is called with the original endpointUrl, not gatewayEndpointUrl. This creates a divergence where the registered route and the verified route differ.
  • Impact: Onboarding may pass verification (host-side localhost works) but the registered gateway route points to host.openshell.internal which may not resolve in the sandbox context. Users get false-success onboarding that fails at runtime.
  • Required action: Either verify against the registered gateway URL, or ensure the host-side verification actually exercises the gateway path. The test should assert that verifyOnboardInferenceSmoke is called with the correct URL for the topology.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read remote.ts lines 220-240. Compare gatewayEndpointUrl (rewritten) passed to upsertProvider vs endpointUrl (original) passed to verifyOnboardInferenceSmoke. Check test/onboard-inference-gateway-scope.test.ts lines 75-80.
  • Missing regression test: Test verifying that the URL used for gateway registration matches the URL verified at runtime, or that host-side verification exercises the actual gateway route.
  • Done when: The required change is committed and verification passes: Read remote.ts lines 220-240. Compare gatewayEndpointUrl (rewritten) passed to upsertProvider vs endpointUrl (original) passed to verifyOnboardInferenceSmoke. Check test/onboard-inference-gateway-scope.test.ts lines 75-80.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:220-240 test/onboard-inference-gateway-scope.test.ts:75-80

PRA-7 Resolve/justify — Documentation claims topology-dependent rewrite but specifies no validated topologies

  • Location: docs/inference/local-compatible-inference-setup.mdx:168
  • Category: docs
  • Problem: The updated docs state 'This rewrite depends on an OpenShell topology that resolves host.openshell.internal inside the sandbox' but do not specify which OpenShell versions/drivers are validated. Evidence shows the rewrite is inert on 0.0.44 (reported topology) and only works on 0.0.72 (unvalidated in CI). No version matrix, CI badge, or topology qualification is documented.
  • Impact: Users cannot determine if their topology supports the rewrite. The docs imply universal applicability with a caveat, but the behavior is topology-specific with only one anecdotal working topology.
  • Recommended action: Add explicit topology qualification: specify which OpenShell version/driver combinations are CI-validated. Add a version badge or link to the CI job that validates the topology. If only 0.0.72 is known working, state that clearly.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read docs/inference/local-compatible-inference-setup.mdx lines 168-180. Check for version matrix, CI badge, or specific topology identification.
  • Missing regression test: N/A - documentation accuracy issue. Add CI job that validates docs claims against actual topology.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read docs/inference/local-compatible-inference-setup.mdx lines 168-180. Check for version matrix, CI badge, or specific topology identification.
  • Evidence: docs/inference/local-compatible-inference-setup.mdx:168-180

PRA-8 Resolve/justify — Credential reuse path bypasses rewrite validation

  • Location: src/lib/onboard/inference-providers/remote.ts:185
  • Category: correctness
  • Problem: When reuseGatewayCredentialWithoutLocalKey is true (lines 185-195), the code compares gatewayEndpointUrl !== resolvedEndpointUrl and calls upsertProvider with the rewritten URL. However, it does not validate that the existing provider's topology matches the rewrite assumption. If a provider was previously registered with a different URL (e.g., manually entered host.openshell.internal), the rewrite may be inappropriate but is applied anyway.
  • Impact: Existing providers may be silently updated with a rewritten URL that doesn't match their actual topology, breaking inference routing for those providers.
  • Recommended action: Add validation that the existing provider's configuration is compatible with the rewrite before applying it. Or skip the rewrite in credential reuse mode and use the existing provider URL as-is.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read remote.ts lines 185-195. Verify no topology compatibility check before upsertProvider with gatewayEndpointUrl.
  • Missing regression test: Test: existing provider registered with host.openshell.internal URL, onboarding with localhost URL and reuseGatewayCredentialWithoutLocalKey → should not rewrite existing provider.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read remote.ts lines 185-195. Verify no topology compatibility check before upsertProvider with gatewayEndpointUrl.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:185-195

PRA-9 Resolve/justify — IPv6 zone ID handling produces invalid rewritten hostname

  • Location: src/lib/onboard/inference-providers/remote.ts:35
  • Category: correctness
  • Problem: The hostname normalization at line 35 (hostname.replace(/^\[|\]$/g, '')) strips brackets but does not handle IPv6 zone IDs (e.g., '::1%eth0'). A URL like 'http://[::1%eth0]:8000/v1' would pass the loopback check (hostname becomes '::1%eth0', which includes '::1') but produce 'host.openshell.internal%eth0' after rewrite — an invalid hostname.
  • Impact: IPv6 addresses with zone IDs (valid in some container networking setups) produce malformed gateway URLs that will fail to resolve.
  • Recommended action: Either reject IPv6 zone IDs explicitly in the loopback check, or strip zone IDs before rewrite. Add test cases for IPv6 with zone IDs.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read remote.ts lines 30-45. Check if hostname.includes('%') is handled. Test with 'http://[::1%eth0]:8000/v1'.
  • Missing regression test: Test case: IPv6 loopback with zone ID (e.g., [::1%eth0]:8000) → should either rewrite cleanly to host.openshell.internal or reject with clear error.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read remote.ts lines 30-45. Check if hostname.includes('%') is handled. Test with 'http://[::1%eth0]:8000/v1'.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:30-45

PRA-10 Resolve/justify — No rollback/cleanup on inference set failure after provider create

  • Location: src/lib/onboard/inference-providers/remote.ts:235
  • Category: correctness
  • Problem: The onboarding loop (lines 235-255) calls provider create/upsert, then inference set. If inference set fails, the loop retries but the provider remains registered with the rewritten URL. There is no cleanup or rollback of the provider registration on failure. This can leave ghost providers in OpenShell.
  • Impact: Failed onboarding attempts leave stale provider registrations with rewritten URLs. Subsequent onboarding attempts may conflict with these ghost providers or users may see unexpected provider entries.
  • Recommended action: Add cleanup logic in the catch/retry path: if inference set fails and a provider was created in this attempt, delete it before retrying. Or track created providers and clean up on final failure.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read remote.ts lines 235-255. Verify no provider deletion on inference set failure.
  • Missing regression test: Test: mock inference set failure after provider create → verify provider is cleaned up, not left registered.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read remote.ts lines 235-255. Verify no provider deletion on inference set failure.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:235-255

PRA-11 Required — Workaround lacks regression test proving source cannot regress and vague removal condition

  • Location: src/lib/onboard/inference-providers/remote.ts:11
  • Category: security
  • Problem: The gatewayReachableCompatibleEndpointUrl() workaround (comment line 11: 'Remove this when OpenShell can verify provider routes from the sandbox/gateway network context') fails the source-of-truth review: (1) invalid state = host loopback URL unreachable from sandbox; (2) source boundary = OpenShell gateway bridge does not resolve host.openshell.internal from host-side verifier context; (3) why not fixed here = OpenShell owns gateway bridge (plausible); (4) regression test proving source cannot regress = MISSING; (5) removal condition = vague ('when OpenShell can verify'). No test locks in the gateway bridge behavior that makes this workaround necessary.
  • Impact: The workaround may become permanently embedded without a trigger for removal. If the OpenShell gateway bridge behavior changes (e.g., starts resolving host.openshell.internal from host context), the rewrite becomes incorrect but no test will detect it. Conversely, if the bridge stops working on a topology, the rewrite silently produces broken routes.
  • Required action: Add a regression test that validates the gateway bridge behavior this workaround depends on. Document a concrete removal condition tied to an OpenShell version/feature flag. Consider making the rewrite opt-in via config until the bridge behavior is stable and tested.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read remote.ts lines 11-15 (workaround comment). Search for any test that validates host.openshell.internal resolution behavior from host vs sandbox contexts.
  • Missing regression test: Test that validates: from host-side verifier context, host.openshell.internal resolves to the gateway bridge IP; from sandbox context, it resolves to the same. This test should fail if the bridge behavior changes, triggering workaround review.
  • Done when: The required change is committed and verification passes: Read remote.ts lines 11-15 (workaround comment). Search for any test that validates host.openshell.internal resolution behavior from host vs sandbox contexts.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:11-15

PRA-12 Required — Security boundary bypass: --no-verify injected on URL pattern alone without topology detection or probe success

  • Location: src/lib/onboard/inference-providers/remote.ts:220
  • Category: security
  • Problem: The --no-verify flag (which skips OpenShell's gateway route verification) is injected whenever the URL rewrite occurs (gatewayEndpointUrl !== resolvedEndpointUrl). This decision is based solely on the URL pattern (HTTP loopback on unprivileged port), with NO topology detection (OpenShell version/driver check) and NO requirement that a host-side reachability probe succeeds. On unsupported topologies, this bypasses the security check that the gateway route is actually functional.
  • Impact: Users on unsupported topologies get a false-success onboarding: the provider is registered with a rewritten URL that may not resolve in the sandbox, and --no-verify skips the verification that would catch this. The security boundary (gateway route verification) is bypassed based on a client-side URL pattern match.
  • Required action: Gate --no-verify injection behind BOTH: (1) topology detection confirming the rewrite is effective on this OpenShell version/driver, AND (2) a successful host-side reachability probe that exercises the actual gateway path. If either check fails, do not inject --no-verify and fail onboarding with actionable error.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read remote.ts lines 220-230. Verify --no-verify condition is only (config.skipVerify || gatewayEndpointUrl !== resolvedEndpointUrl) with no topology check or probe success gate.
  • Missing regression test: Test: on unsupported topology (mocked), loopback URL → no rewrite, no --no-verify. Test: on supported topology but host probe fails → rewrite occurs but no --no-verify, onboarding fails. Test: on supported topology with probe success → rewrite + --no-verify.
  • Done when: The required change is committed and verification passes: Read remote.ts lines 220-230. Verify --no-verify condition is only (config.skipVerify || gatewayEndpointUrl !== resolvedEndpointUrl) with no topology check or probe success gate.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:220-230

PRA-13 Required — No direct unit tests for gatewayReachableCompatibleEndpointUrl function

  • Location: src/lib/onboard/inference-providers/remote.ts:11
  • Category: tests
  • Problem: The new gatewayReachableCompatibleEndpointUrl() function (lines 11-45) has zero direct unit tests. The only coverage is indirect via the gateway-scope integration test which mocks runOpenshell and asserts command args. Critical behaviors lack unit test coverage: parse error handling, IPv6 zone IDs, privileged port boundary (1023 vs 1024), non-loopback hosts, non-HTTP protocols, URLs with authentication, and port parsing edge cases.
  • Impact: Behavioral regressions in URL parsing, loopback detection, and rewrite logic will not be caught by unit tests. The function is security-sensitive (controls --no-verify injection) but only tested through a single happy-path integration test.
  • Required action: Add focused unit tests for gatewayReachableCompatibleEndpointUrl covering: valid rewrite cases (localhost, 127.0.0.1, ::1 on various ports), negative cases (privileged ports <1024, non-loopback hosts, HTTPS, URLs with auth, malformed URLs), IPv6 zone ID handling, and parse error behavior. Each case should assert the exact return value (rewritten URL, original URL, or error).
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Search test/ directory for any test file that directly imports and calls gatewayReachableCompatibleEndpointUrl. Verify coverage of the cases listed above.
  • Missing regression test: Unit test suite for gatewayReachableCompatibleEndpointUrl with 15+ cases covering all URL parsing, loopback detection, port validation, and rewrite branches.
  • Done when: The required change is committed and verification passes: Search test/ directory for any test file that directly imports and calls gatewayReachableCompatibleEndpointUrl. Verify coverage of the cases listed above.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:11-45 test/onboard-inference-gateway-scope.test.ts:45-85 (only indirect coverage)

PRA-14 Required — Missing security boundary negative tests for gateway rewrite

  • Location: test/onboard-inference-gateway-scope.test.ts:1
  • Category: tests
  • Problem: The new test 'registers loopback compatible endpoints through the gateway alias but keeps host smoke local' covers only the happy path (http://localhost:8000/v1\). Critical security boundaries lack test coverage: (1) privileged port 1023 must NOT rewrite (IPv4 and IPv6); (2) IPv6 loopback with zone ID (::1%eth0) must be rejected or handled; (3) malformed URLs must fail explicitly, not silently pass; (4) host-side probe failure must NOT inject --no-verify; (5) topology detection guard (when added) must block rewrite on unsupported topologies.
  • Impact: Without these tests, regressions could allow privileged port rewrites (exposing privileged services), produce invalid hostnames from IPv6 zone IDs, silently accept malformed URLs, or inject --no-verify when the gateway bridge is down — all bypassing security checks.
  • Required action: Add negative test cases to test/onboard-inference-gateway-scope.test.ts (or a new focused test file) for each security boundary. Each must assert: rewrite is blocked or fails with appropriate error, and --no-verify is NOT injected in the inference set command.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read test/onboard-inference-gateway-scope.test.ts lines 45-85. Verify no test cases for port 1023, IPv6 ::1, zone IDs, malformed URLs, or host-probe failure.
  • Missing regression test: Five negative test cases: (1) http://localhost:1023/v1 → no rewrite, no --no-verify; (2) http://[::1]:1023/v1 → no rewrite, no --no-verify; (3) http://[::1%eth0]:8000/v1 → reject or strip zone ID; (4) malformed URL → explicit error; (5) host probe failure → no --no-verify, actionable error.
  • Done when: The required change is committed and verification passes: Read test/onboard-inference-gateway-scope.test.ts lines 45-85. Verify no test cases for port 1023, IPv6 ::1, zone IDs, malformed URLs, or host-probe failure.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:30-45 src/lib/onboard/inference-providers/remote.ts:220-230 test/onboard-inference-gateway-scope.test.ts:45-85

PRA-15 Required — No test for ghost provider cleanup on inference set failure

  • Location: src/lib/onboard/inference-providers/remote.ts:235
  • Category: tests
  • Problem: The onboarding loop (lines 235-255) creates/upserts a provider, then calls inference set. If inference set fails, the loop retries but the provider remains registered with the rewritten URL. The risk plan invariant 'partial failure and retry converge without ghost resources' (lifecycle-state family) has no test evidence. No test mocks inference set failure and verifies provider cleanup.
  • Impact: Failed onboarding attempts leave stale provider registrations with rewritten URLs. Subsequent onboarding may conflict with ghost providers or users see unexpected provider entries. The lifecycle-state invariant is unvalidated.
  • Required action: Add test: mock inference set failure after provider create → verify provider is cleaned up (deleted) before retry or on final failure. This may require refactoring the onboarding loop to track created providers for cleanup.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read remote.ts lines 235-255. Check test/onboard-inference-gateway-scope.test.ts and any remote.test.ts for provider cleanup on failure.
  • Missing regression test: Test: mock inference set failure after provider create → verify provider delete is called, no ghost provider remains registered.
  • Done when: The required change is committed and verification passes: Read remote.ts lines 235-255. Check test/onboard-inference-gateway-scope.test.ts and any remote.test.ts for provider cleanup on failure.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:235-255 test/onboard-inference-gateway-scope.test.ts

PRA-16 Required — Test verifies smoke against original URL not rewritten gateway URL

  • Location: src/lib/onboard/inference-providers/remote.ts:220
  • Category: tests
  • Problem: The new test (lines 75-80 of onboard-inference-gateway-scope.test.ts) asserts verifyOnboardInferenceSmoke is called with the original endpointUrl (localhost:8000), not the rewritten gatewayEndpointUrl (host.openshell.internal:8000). This matches the current code behavior but validates the divergence (F-006): the registered route and verified route differ. The risk plan invariant 'the selected provider is reachable through the route advertised to the agent' (inference-policy) requires runtime validation that the gateway route works, not just the host-side localhost.
  • Impact: The test passes but validates incorrect behavior: onboarding verifies the host-side URL while registering a different gateway URL. If the gateway bridge is broken, onboarding succeeds but runtime inference fails. The required E2E jobs (inference-routing, network-policy) would catch this but have no checked-in test coverage.
  • Required action: Either: (1) update the test to verify against the gateway URL when rewrite occurs, and add a host-side probe that exercises the gateway path; or (2) add a separate integration test that validates the full gateway route works at runtime. The current test should at minimum assert the correct URL is passed to verifyOnboardInferenceSmoke for the topology.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read remote.ts lines 220-240. Compare gatewayEndpointUrl passed to upsertProvider vs endpointUrl passed to verifyOnboardInferenceSmoke. Check test/onboard-inference-gateway-scope.test.ts lines 75-80.
  • Missing regression test: Integration test: onboard with loopback URL on validated topology → verify sandbox inference.local returns 200 (proves gateway route works). Unit test: verifyOnboardInferenceSmoke called with gatewayEndpointUrl when rewrite occurs.
  • Done when: The required change is committed and verification passes: Read remote.ts lines 220-240. Compare gatewayEndpointUrl passed to upsertProvider vs endpointUrl passed to verifyOnboardInferenceSmoke. Check test/onboard-inference-gateway-scope.test.ts lines 75-80.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:220-240 test/onboard-inference-gateway-scope.test.ts:75-80

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Changes requested

Merge posture: Do not merge yet
Primary next action: Resolve or justify PRA-1: Warn before recommending all-interface inference server binds.
Open items: 0 required · 2 warnings · 0 suggestions · 8 test follow-ups
Since last review: 1 prior item resolved · 2 still apply · 0 new items found

Action checklist

  • PRA-1 Resolve or justify: Warn before recommending all-interface inference server binds in docs/inference/local-compatible-inference-setup.mdx:61
  • PRA-2 Resolve or justify: Add runtime coverage for the sandbox inference route and egress boundary in src/lib/onboard/inference-providers/remote.ts:356
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Runtime validation
  • PRA-T6 Add or justify test follow-up: Acceptance clause
  • PRA-T7 Add or justify test follow-up: Acceptance clause
  • PRA-T8 Add or justify test follow-up: Acceptance clause

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify security docs/inference/local-compatible-inference-setup.mdx:61 Add a warning adjacent to the `0.0.0.0` guidance and raw-model example explaining that all-interface binds expose the service unless restricted, and recommend host firewall/interface scoping to the OpenShell/Docker bridge or enabling endpoint authentication.
PRA-2 Resolve/justify security src/lib/onboard/inference-providers/remote.ts:356 Add focused runtime/security coverage that starts or stubs an OpenAI-compatible local server, configures `NEMOCLAW_ENDPOINT_URL=http://localhost:8000/v1\`, verifies sandbox access through `https://inference.local/v1/models\`, and includes a paired denial assertion for unintended egress; if the current harness cannot do this, document the limitation and checked-in guard in this PR.
Review findings by urgency: 0 required fixes, 2 items to resolve/justify, 0 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Warn before recommending all-interface inference server binds

  • Location: docs/inference/local-compatible-inference-setup.mdx:61
  • Category: security
  • Problem: The self-hosted inference guide now tells users to bind local model servers on a container-reachable address and changes the llama-server example to `--host 0.0.0.0`, while also documenting dummy API keys for unauthenticated servers, but it does not warn that `0.0.0.0` exposes the model API on all host interfaces unless constrained.
  • Impact: Users following the guide may unintentionally expose an unauthenticated local model API to other host interfaces or networks, increasing the risk of unauthorized inference use and prompt/response disclosure.
  • Recommended action: Add a warning adjacent to the `0.0.0.0` guidance and raw-model example explaining that all-interface binds expose the service unless restricted, and recommend host firewall/interface scoping to the OpenShell/Docker bridge or enabling endpoint authentication.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read docs/inference/local-compatible-inference-setup.mdx around the `0.0.0.0` guidance and llama-server example; confirm a warning and mitigations appear adjacent to both recommendations.
  • Missing regression test: Documentation-only security note; no runtime regression is required. If docs snapshot tests cover this section, add an assertion that the exposure warning remains adjacent to the `0.0.0.0` guidance and `llama-server --host 0.0.0.0` example.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read docs/inference/local-compatible-inference-setup.mdx around the `0.0.0.0` guidance and llama-server example; confirm a warning and mitigations appear adjacent to both recommendations.
  • Evidence: docs/inference/local-compatible-inference-setup.mdx:61 says: Make sure the local server listens on an address reachable from containers, such as `0.0.0.0`; a server bound only to `127.0.0.1` can still be unreachable from the sandbox route. docs/inference/local-compatible-inference-setup.mdx:78 changes the raw model example to `--host 0.0.0.0`. docs/inference/local-compatible-inference-setup.mdx:96 and :147 say unauthenticated endpoints may use any non-empty dummy API key.

PRA-2 Resolve/justify — Add runtime coverage for the sandbox inference route and egress boundary

  • Location: src/lib/onboard/inference-providers/remote.ts:356
  • Category: security
  • Problem: The PR changes the inference trust boundary by registering loopback compatible endpoints through `host.openshell.internal` and skipping OpenShell verification for that rewritten route, but checked-in tests only assert provider registration commands and host-smoke arguments, not an actual sandbox `inference.local` request or paired policy-denial behavior.
  • Impact: A future change could preserve the mocked registration arguments while the real sandbox route still returns 503, or while policy accidentally allows direct host/public egress beyond the intended inference route.
  • Recommended action: Add focused runtime/security coverage that starts or stubs an OpenAI-compatible local server, configures `NEMOCLAW_ENDPOINT_URL=http://localhost:8000/v1\`, verifies sandbox access through `https://inference.local/v1/models\`, and includes a paired denial assertion for unintended egress; if the current harness cannot do this, document the limitation and checked-in guard in this PR.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect test/onboard-inference-gateway-scope.test.ts and any e2e/security tests for a real sandbox request to `https://inference.local/v1/models\` plus a negative egress assertion.
  • Missing regression test: Add a checked-in runtime/E2E regression that verifies the rewritten compatible-endpoint route succeeds through `inference.local` and verifies direct host-alias/public egress remains denied under the configured policy.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect test/onboard-inference-gateway-scope.test.ts and any e2e/security tests for a real sandbox request to `https://inference.local/v1/models\` plus a negative egress assertion.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:356-358 adds `--no-verify` when `gatewayEndpointUrl !== resolvedEndpointUrl`. test/onboard-inference-gateway-scope.test.ts:60-99 asserts `provider create` uses `OPENAI_BASE_URL=http://host.openshell.internal:8000/v1\` and host smoke uses the original endpoint, but it does not spawn a sandbox or request `https://inference.local/v1/models\`. Security risk context lists the inference-policy invariant: network policy permits the intended route and denies unintended egress.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

  • None.
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — Run the `inference-routing` E2E job for Inference selection, reachability, and network policy must agree at the real host-to-sandbox boundary. Matched files: `src/lib/onboard/inference-providers/remote.ts`.. Deterministic regression risks require live validation: lifecycle-state, inference-policy. The changed code affects inference routing and network-policy trust boundaries. Existing tests provide focused unit/docs coverage for command construction, host-smoke arguments, registry metadata, and docs topology text, but risk-plan invariants require runtime validation for selected-provider reachability and unintended-egress denial.
  • PRA-T2 Runtime validation — Add a checked-in runtime/E2E regression that starts or stubs an OpenAI-compatible local server on `0.0.0.0:8000`, onboards/configures `NEMOCLAW_ENDPOINT_URL=http://localhost:8000/v1\`, and verifies a sandbox request to `https://inference.local/v1/models\` succeeds through the rewritten route.. Deterministic regression risks require live validation: lifecycle-state, inference-policy. The changed code affects inference routing and network-policy trust boundaries. Existing tests provide focused unit/docs coverage for command construction, host-smoke arguments, registry metadata, and docs topology text, but risk-plan invariants require runtime validation for selected-provider reachability and unintended-egress denial.
  • PRA-T3 Runtime validation — Run the `network-policy` E2E job for Inference selection, reachability, and network policy must agree at the real host-to-sandbox boundary. Matched files: `src/lib/onboard/inference-providers/remote.ts`.. Deterministic regression risks require live validation: lifecycle-state, inference-policy. The changed code affects inference routing and network-policy trust boundaries. Existing tests provide focused unit/docs coverage for command construction, host-smoke arguments, registry metadata, and docs topology text, but risk-plan invariants require runtime validation for selected-provider reachability and unintended-egress denial.
  • PRA-T4 Runtime validation — Add a paired negative assertion in that runtime/security coverage proving unintended direct host-alias or public egress remains denied under the configured policy.. Deterministic regression risks require live validation: lifecycle-state, inference-policy. The changed code affects inference routing and network-policy trust boundaries. Existing tests provide focused unit/docs coverage for command construction, host-smoke arguments, registry metadata, and docs topology text, but risk-plan invariants require runtime validation for selected-provider reachability and unintended-egress denial.
  • PRA-T5 Runtime validation — Run the `onboard-repair` E2E job for Onboarding and sandbox state must converge across persisted metadata, reported status, and the live runtime. Matched files: `src/lib/onboard/inference-providers/remote.ts`.. Deterministic regression risks require live validation: lifecycle-state, inference-policy. The changed code affects inference routing and network-policy trust boundaries. Existing tests provide focused unit/docs coverage for command construction, host-smoke arguments, registry metadata, and docs topology text, but risk-plan invariants require runtime validation for selected-provider reachability and unintended-egress denial.
  • PRA-T6 Acceptance clause — When a LangChain Deep Agents Code sandbox is onboarded with a local vLLM custom endpoint (e.g. `http://localhost:8000/v1\`\), the host-side vLLM server is reachable directly from the host (`/v1/models` and `/v1/chat/completions` both return 200), but requests from inside the sandbox via `https://inference.local/v1/models\` return 503. — add test evidence or identify existing coverage. The code rewrites compatible-endpoint loopback registration through `host.openshell.internal` while preserving host-side smoke on the original URL, and the new harness test verifies the registration command. Checked-in runtime evidence for an actual sandbox `https://inference.local/v1/models\` request is not present and is covered by F-002.
  • PRA-T7 Acceptance clause — Step 4: The NemoClaw gateway correctly proxies the sandbox inference request to the registered custom endpoint (`http://localhost:8000/v1\`\). dcode returns "VLLM" with exit 0. — add test evidence or identify existing coverage. The diff implements the route transformation needed for the gateway registration and documents `status`/agent verification, but repository tests do not run dcode or a live sandbox inference request; F-002 covers the missing runtime regression.
  • PRA-T8 Acceptance clause — This identifies a current topology where `localhost` -> `host.openshell.internal` for gateway registration changes sandbox reachability. The loopback-only caveat still applies separately: the vLLM server must listen on a container-reachable address such as `0.0.0.0`, not only `127.0.0.1`. — add test evidence or identify existing coverage. Docs now state the container-reachable bind requirement and the test asserts `--host 0.0.0.0`; however, the all-interface exposure warning is missing and tracked by F-001.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Warn before recommending all-interface inference server binds

  • Location: docs/inference/local-compatible-inference-setup.mdx:61
  • Category: security
  • Problem: The self-hosted inference guide now tells users to bind local model servers on a container-reachable address and changes the llama-server example to `--host 0.0.0.0`, while also documenting dummy API keys for unauthenticated servers, but it does not warn that `0.0.0.0` exposes the model API on all host interfaces unless constrained.
  • Impact: Users following the guide may unintentionally expose an unauthenticated local model API to other host interfaces or networks, increasing the risk of unauthorized inference use and prompt/response disclosure.
  • Recommended action: Add a warning adjacent to the `0.0.0.0` guidance and raw-model example explaining that all-interface binds expose the service unless restricted, and recommend host firewall/interface scoping to the OpenShell/Docker bridge or enabling endpoint authentication.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read docs/inference/local-compatible-inference-setup.mdx around the `0.0.0.0` guidance and llama-server example; confirm a warning and mitigations appear adjacent to both recommendations.
  • Missing regression test: Documentation-only security note; no runtime regression is required. If docs snapshot tests cover this section, add an assertion that the exposure warning remains adjacent to the `0.0.0.0` guidance and `llama-server --host 0.0.0.0` example.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read docs/inference/local-compatible-inference-setup.mdx around the `0.0.0.0` guidance and llama-server example; confirm a warning and mitigations appear adjacent to both recommendations.
  • Evidence: docs/inference/local-compatible-inference-setup.mdx:61 says: Make sure the local server listens on an address reachable from containers, such as `0.0.0.0`; a server bound only to `127.0.0.1` can still be unreachable from the sandbox route. docs/inference/local-compatible-inference-setup.mdx:78 changes the raw model example to `--host 0.0.0.0`. docs/inference/local-compatible-inference-setup.mdx:96 and :147 say unauthenticated endpoints may use any non-empty dummy API key.

PRA-2 Resolve/justify — Add runtime coverage for the sandbox inference route and egress boundary

  • Location: src/lib/onboard/inference-providers/remote.ts:356
  • Category: security
  • Problem: The PR changes the inference trust boundary by registering loopback compatible endpoints through `host.openshell.internal` and skipping OpenShell verification for that rewritten route, but checked-in tests only assert provider registration commands and host-smoke arguments, not an actual sandbox `inference.local` request or paired policy-denial behavior.
  • Impact: A future change could preserve the mocked registration arguments while the real sandbox route still returns 503, or while policy accidentally allows direct host/public egress beyond the intended inference route.
  • Recommended action: Add focused runtime/security coverage that starts or stubs an OpenAI-compatible local server, configures `NEMOCLAW_ENDPOINT_URL=http://localhost:8000/v1\`, verifies sandbox access through `https://inference.local/v1/models\`, and includes a paired denial assertion for unintended egress; if the current harness cannot do this, document the limitation and checked-in guard in this PR.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect test/onboard-inference-gateway-scope.test.ts and any e2e/security tests for a real sandbox request to `https://inference.local/v1/models\` plus a negative egress assertion.
  • Missing regression test: Add a checked-in runtime/E2E regression that verifies the rewritten compatible-endpoint route succeeds through `inference.local` and verifies direct host-alias/public egress remains denied under the configured policy.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect test/onboard-inference-gateway-scope.test.ts and any e2e/security tests for a real sandbox request to `https://inference.local/v1/models\` plus a negative egress assertion.
  • Evidence: src/lib/onboard/inference-providers/remote.ts:356-358 adds `--no-verify` when `gatewayEndpointUrl !== resolvedEndpointUrl`. test/onboard-inference-gateway-scope.test.ts:60-99 asserts `provider create` uses `OPENAI_BASE_URL=http://host.openshell.internal:8000/v1\` and host smoke uses the original endpoint, but it does not spawn a sandbox or request `https://inference.local/v1/models\`. Security risk context lists the inference-policy invariant: network policy permits the intended route and denies unintended egress.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/lib/onboard/inference-providers/remote.ts (1)

104-165: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Compute gatewayEndpointUrl before the reuse branch.
The reuse path leaves it equal to resolvedEndpointUrl, so inference set won’t add --no-verify for a provider that was already rewritten to host.openshell.internal. That can bring back the loopback verify failure on resume/reuse; add a reuse-path test in remote.test.ts.

🤖 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/inference-providers/remote.ts` around lines 104 - 165,
Compute gatewayEndpointUrl before entering the
reuseGatewayCredentialWithoutLocalKey branch so it reflects the
gateway-rewritten endpoint in both paths. Right now the reuse path leaves
gatewayEndpointUrl at resolvedEndpointUrl, which prevents inference set from
applying the same no-verify behavior for reused providers; update the remote
flow in remote.ts around gatewayEndpointUrl/providerResult handling and add a
reuse-path test in remote.test.ts to cover the resumed provider case.

Source: Path instructions

🧹 Nitpick comments (2)
src/lib/onboard/inference-providers/remote.ts (1)

29-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Trailing-slash normalization is hard to follow.

parsed.toString().replace(/\/$/, parsed.pathname === "/" ? "/" : "") is a no-op when pathname === "/" (replaces trailing / with /) and only has an effect when the path is non-root and ends with / (strips it). Worth a short comment explaining intent, or simplifying, since this is on the path that computes the URL registered with the gateway.

🤖 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/inference-providers/remote.ts` around lines 29 - 30, The URL
normalization in the `remote.ts` host rewrite is hard to read because
`parsed.toString().replace(...)` hides the intent of stripping only a non-root
trailing slash. Simplify the logic in the `parsed.hostname` / return path, or
add a short comment near the URL-building code to make it clear that root paths
keep their slash while non-root paths have the trailing slash removed before
registering the gateway URL.
src/lib/onboard/inference-providers/remote.test.ts (1)

53-92: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Good happy-path coverage; missing the credential-reuse branch.

This test correctly exercises the fresh-provision path and asserts on the real (non-mocked) gatewayReachableCompatibleEndpointUrl behavior via the injected upsertProvider/runOpenshell mocks — that's an appropriate boundary. Consider adding a case with reuseGatewayCredentialWithoutLocalKey: true for a loopback compatible-endpoint, which would surface the gatewayEndpointUrl recomputation gap flagged in remote.ts (Lines 104-165).

🤖 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/inference-providers/remote.test.ts` around lines 53 - 92, Add
a second test in setupRemoteProviderInference that covers the
reuseGatewayCredentialWithoutLocalKey branch for a loopback compatible-endpoint.
Use the existing makeDeps fixture and verify that when
reuseGatewayCredentialWithoutLocalKey is true, the code recomputes
gatewayEndpointUrl correctly before calling upsertProvider and runOpenshell.
Keep the current happy-path test, but extend coverage with this case so the
credential-reuse behavior is asserted through the same
setupRemoteProviderInference entry point.
🤖 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 `@src/lib/onboard/inference-providers/remote.ts`:
- Around line 11-32: The loopback rewrite logic in
gatewayReachableCompatibleEndpointUrl misses default-port HTTP URLs and has a
dead IPv6 hostname check because URL.hostname preserves brackets for IPv6
literals. Update the loopback detection to rely on the normalized hostname
handling already in this helper, remove the ineffective "::1" comparison, and
allow http://localhost/v1-style URLs to be rewritten even when parsed.port is
empty so the compatible-endpoint path is still routed through
host.openshell.internal.

---

Outside diff comments:
In `@src/lib/onboard/inference-providers/remote.ts`:
- Around line 104-165: Compute gatewayEndpointUrl before entering the
reuseGatewayCredentialWithoutLocalKey branch so it reflects the
gateway-rewritten endpoint in both paths. Right now the reuse path leaves
gatewayEndpointUrl at resolvedEndpointUrl, which prevents inference set from
applying the same no-verify behavior for reused providers; update the remote
flow in remote.ts around gatewayEndpointUrl/providerResult handling and add a
reuse-path test in remote.test.ts to cover the resumed provider case.

---

Nitpick comments:
In `@src/lib/onboard/inference-providers/remote.test.ts`:
- Around line 53-92: Add a second test in setupRemoteProviderInference that
covers the reuseGatewayCredentialWithoutLocalKey branch for a loopback
compatible-endpoint. Use the existing makeDeps fixture and verify that when
reuseGatewayCredentialWithoutLocalKey is true, the code recomputes
gatewayEndpointUrl correctly before calling upsertProvider and runOpenshell.
Keep the current happy-path test, but extend coverage with this case so the
credential-reuse behavior is asserted through the same
setupRemoteProviderInference entry point.

In `@src/lib/onboard/inference-providers/remote.ts`:
- Around line 29-30: The URL normalization in the `remote.ts` host rewrite is
hard to read because `parsed.toString().replace(...)` hides the intent of
stripping only a non-root trailing slash. Simplify the logic in the
`parsed.hostname` / return path, or add a short comment near the URL-building
code to make it clear that root paths keep their slash while non-root paths have
the trailing slash removed before registering the gateway URL.
🪄 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: Enterprise

Run ID: 1220f9a7-0668-44d1-ac0d-d868232051af

📥 Commits

Reviewing files that changed from the base of the PR and between f0d2549 and a74eb68.

📒 Files selected for processing (3)
  • docs/inference/use-local-inference.mdx
  • src/lib/onboard/inference-providers/remote.test.ts
  • src/lib/onboard/inference-providers/remote.ts

Comment thread src/lib/onboard/inference-providers/remote.ts
Signed-off-by: Chengjie Wang <chengjiew@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Jul 6, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@chengjiew chengjiew changed the title Fix local compatible endpoint routing for sandbox inference fix(inference): route local compatible endpoints through gateway Jul 6, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/lib/onboard/inference-providers/remote.test.ts (1)

63-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Solid coverage of the rewrite contract, but IPv6 loopback success path is untested.

Both success-path tests use localhost and 127.0.0.1; the helper's isLoopback check also treats ::1 as loopback (src/lib/onboard/inference-providers/remote.ts:11-39), but no test exercises the bracketed-IPv6 rewrite branch (bracket-stripping regex + ::1 match). Consider adding one more case to lock in that behavior, since it's a distinct code path in the helper.

🤖 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/inference-providers/remote.test.ts` around lines 63 - 146,
Add a dedicated test in setupRemoteProviderInference to cover the IPv6 loopback
branch, since the current cases only exercise localhost and 127.0.0.1. Use the
existing remote.test.ts setup with makeDeps and assert that a bracketed ::1
endpoint is treated as loopback and rewritten to the sandbox/gateway host the
same way as the other success paths. Reference setupRemoteProviderInference and
the isLoopback/endpoint rewrite behavior so the bracket-stripping path stays
covered even if the implementation changes.
🤖 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 `@src/lib/onboard/inference-providers/remote.test.ts`:
- Around line 63-146: Add a dedicated test in setupRemoteProviderInference to
cover the IPv6 loopback branch, since the current cases only exercise localhost
and 127.0.0.1. Use the existing remote.test.ts setup with makeDeps and assert
that a bracketed ::1 endpoint is treated as loopback and rewritten to the
sandbox/gateway host the same way as the other success paths. Reference
setupRemoteProviderInference and the isLoopback/endpoint rewrite behavior so the
bracket-stripping path stays covered even if the implementation changes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: a28aca25-17cd-494f-a35e-498922cda13c

📥 Commits

Reviewing files that changed from the base of the PR and between a74eb68 and 030c98d.

📒 Files selected for processing (2)
  • src/lib/onboard/inference-providers/remote.test.ts
  • src/lib/onboard/inference-providers/remote.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/lib/onboard/inference-providers/remote.ts

chengjiew added 2 commits July 7, 2026 01:06
Signed-off-by: Chengjie Wang <chengjiew@nvidia.com>
Signed-off-by: Chengjie Wang <chengjiew@nvidia.com>
@wscurran wscurran added area: inference Inference routing, serving, model selection, or outputs area: local-models Local model providers, downloads, launch, or connectivity area: routing Request routing, policy routing, model selection, or fallback logic area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression labels Jul 7, 2026
@wscurran

wscurran commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

✨ Thanks for the PR. This fixes the sandbox inference.local 503s for local vLLM-compatible endpoints by routing loopback addresses through the OpenShell host alias. Ready for maintainer review.


Related open issues:


Related open issues:

Signed-off-by: San Dang <sdang@nvidia.com>
@sandl99 sandl99 self-assigned this Jul 10, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
docs/inference/local-compatible-inference-setup.mdx (2)

351-351: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Minor wording nit: "one time" → "once".

Static analysis flags this as more idiomatic phrasing.

✏️ Suggested wording
-The prompt masks the key during input and retries one time on a bad key before failing.
+The prompt masks the key during input and retries once on a bad key before failing.
🤖 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 `@docs/inference/local-compatible-inference-setup.mdx` at line 351, In the
retry behavior sentence, replace the phrase “one time” with the more idiomatic
“once,” preserving the rest of the wording.

Source: Linters/SAST tools


58-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider mentioning 127.0.0.1/::1 alongside localhost.

The rewrite behavior applies to all loopback forms (localhost, 127.0.0.1, ::1) per the PR objectives, but the doc's examples only show localhost. Worth a brief mention so users entering 127.0.0.1 or ::1 know the same alias rewrite applies.

Also applies to: 86-89

🤖 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 `@docs/inference/local-compatible-inference-setup.mdx` around lines 58 - 63,
Update the local inference setup documentation to explicitly mention that
loopback endpoint handling applies to localhost, 127.0.0.1, and ::1. Extend the
examples or surrounding explanation in the relevant onboarding guidance so users
know all three forms receive the same host.openshell.internal alias rewrite.
🤖 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 `@docs/inference/local-compatible-inference-setup.mdx`:
- Line 351: In the retry behavior sentence, replace the phrase “one time” with
the more idiomatic “once,” preserving the rest of the wording.
- Around line 58-63: Update the local inference setup documentation to
explicitly mention that loopback endpoint handling applies to localhost,
127.0.0.1, and ::1. Extend the examples or surrounding explanation in the
relevant onboarding guidance so users know all three forms receive the same
host.openshell.internal alias rewrite.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: d4b10a0c-915f-4f8e-ad2b-49b757f7ea7a

📥 Commits

Reviewing files that changed from the base of the PR and between 14f49ff and 12da08f.

📒 Files selected for processing (1)
  • docs/inference/local-compatible-inference-setup.mdx

sandl99 added 5 commits July 10, 2026 17:58
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: San Dang <sdang@nvidia.com>
This reverts commit 3cbe47e.

Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: San Dang <sdang@nvidia.com>
@sandl99 sandl99 added the v0.0.80 Release target label Jul 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

E2E Target Results — ✅ All requested jobs passed

Run: 29091003888
Workflow ref: fix/5744_local-vllm-inference-gateway
Requested targets: (default — all supported)
Requested jobs: inference-routing,network-policy,onboard-repair,onboard-resume,cloud-onboard
Summary: 5 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
cloud-onboard ✅ success
inference-routing ✅ success
network-policy ✅ success
onboard-repair ✅ success
onboard-resume ✅ success

@sandl99

sandl99 commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Advisor follow-up for #6331 (comment)

I agree the in-scope risk is runtime proof for the bridge route: this PR verifies the user-entered local endpoint on the host, but registers the gateway provider URL through host.openshell.internal so sandbox traffic reaches the host service.

That is addressed with selective E2E on this head:

I am not expanding this PR with topology detection or a large direct URL-helper negative-test matrix. The code path remains narrow: only compatible-endpoint HTTP loopback URLs without auth, without IPv6 zone IDs, and not on explicit privileged ports rewrite. Malformed URLs continue through the existing endpoint preflight path rather than making this helper the URL validator. The reuse path computes gatewayEndpointUrl before credential reuse and re-upserts when a loopback route needs the gateway alias.

The broader items in the Nemotron comment are either covered by the E2E evidence above or are out of scope for this focused #5744 fix:

  • OpenShell topology detection is a gateway capability concern, not something to grow inside this onboarding branch.
  • Provider cleanup after inference set failure is existing general onboarding behavior, not introduced by this PR.
  • Host smoke against the original URL is intentional; the runtime bridge is validated by E2E/status rather than by the host-side probe.

Keeping the checked-in regression simple: one integration-style command construction test plus the live E2E evidence.

@cv cv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact-head maintainer sweep at dc666e5759: ordinary CI and the five-target live run are green, but contributor compliance is a hard blocker. GitHub reports these commits as unsigned and Unverified: 030c98d6250e26954fbba11f0bc711b199305fcb, 14f49ff89c2aec4b8c8aaa9987dd7ee3eb835125, and 9506281950235a7a30628400ee7195f75e20c332.

Every commit retained in a contributor PR must appear as GitHub Verified. Maintainers cannot amend, sign, or replace contributor history on the contributor behalf. Please publish a clean, fully Verified history; if this already-published branch cannot be safely rewritten, open a fresh branch and PR with compliant commits. The replacement head will need fresh CI and trusted advisor evidence.

Also disposition the remaining trusted-advisor security warning about recommending an unauthenticated 0.0.0.0 inference bind by adding an adjacent exposure/mitigation warning or documenting an explicit accepted-risk rationale.

@sandl99

sandl99 commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

@cv closing this PR in favor of the clean replacement with verified history and the adjacent 0.0.0.0 exposure warning: #6649

@sandl99 sandl99 closed this Jul 10, 2026
@sandl99
sandl99 deleted the fix/5744_local-vllm-inference-gateway branch July 10, 2026 17:50
cv added a commit that referenced this pull request Jul 11, 2026
<!-- markdownlint-disable MD041 -->
## Summary
Replaces PR #6331 with a clean, GitHub-verified branch for the local
compatible-endpoint gateway fix.
The change keeps host-side validation on the user-entered loopback URL,
registers eligible sandbox routes through `host.openshell.internal`, and
documents the exposure risk of `0.0.0.0` binds.

## Related Issue
Fixes #5744.
Replaces #6331.

## Changes
- Rewrite only `compatible-endpoint` HTTP URLs with exact loopback hosts
and explicit bundled policy ports (`11434`, `11435`, or `8000`) when
registering the OpenShell provider route.
- Keep host validation on the user-entered URL; on credential reuse,
update an existing provider to the gateway alias without exporting or
replacing its credential, then configure inference with `--no-verify`.
- Extract the focused transformation and recovery logic from
`remote.ts`, and document the exact supported URL shapes, bundled ports,
bridge reachability, and `0.0.0.0` exposure mitigations.
- Add unit, recovery-integration, docs, fast E2E-support, and hermetic
live inference-routing coverage for the gateway registration contract.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [x] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates
<!-- Check exactly one tests line and one docs line. Check other lines
when applicable. Add every requested justification or approval
reference. -->
- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [x] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification: This is the
clean-history replacement for #6331 after the maintainer sweep at
#6331 (review).
The replacement preserves the reviewed gateway-routing change and adds
the requested adjacent `0.0.0.0` exposure warning instead of accepting
the risk silently.
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each applicable item only when supported by the requested
evidence. Run targeted tests once per relevant change set and rerun
after later edits or hook autofixes that can affect the tested behavior.
Do not rerun hook-covered checks. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or
`npm run check:diff` passed when hooks were skipped or unavailable
- [x] Targeted behavior tests pass for the current change set, or tests
are marked not applicable above — command/result or justification:
focused CLI, integration, docs, E2E-support, mock-parity,
project-membership, title, size, and build checks passed; `npm run docs`
reported 0 errors and 2 existing warnings; live run
[29129492764](https://github.com/NVIDIA/NemoClaw/actions/runs/29129492764)
passed `inference-routing`, `network-policy`, `onboard-repair`,
`onboard-resume`, and `cloud-onboard`.
- [x] Applicable broad gate passed — `npm test` for broad
runtime/test-harness changes; `npm run check` for repo-wide
validation/coverage changes — command/result: `env -u SSH_CONNECTION -u
SSH_CLIENT -u SSH_TTY npm test` passed 1,383 test files and 15,864 tests
(39 skipped, 1 todo).
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only)
- [x] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Compatible inference endpoints using supported loopback addresses and
ports can now be automatically routed through the sandbox gateway.
- Rebuilds can restore compatible endpoint routing without requiring
stored host credentials.
- Endpoint paths, query strings, and fragments are preserved during
automatic routing.

- **Documentation**
- Expanded guidance explains supported endpoints, routing behavior,
validation limits, and runtime status checks.
  - Clarified credential handling during gateway recovery.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Co-authored-by: Carlos Villela <cvillela@nvidia.com>
Co-authored-by: Charan Jagwani <cjagwani@nvidia.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
…DIA#6649)

<!-- markdownlint-disable MD041 -->
## Summary
Replaces PR NVIDIA#6331 with a clean, GitHub-verified branch for the local
compatible-endpoint gateway fix.
The change keeps host-side validation on the user-entered loopback URL,
registers eligible sandbox routes through `host.openshell.internal`, and
documents the exposure risk of `0.0.0.0` binds.

## Related Issue
Fixes NVIDIA#5744.
Replaces NVIDIA#6331.

## Changes
- Rewrite only `compatible-endpoint` HTTP URLs with exact loopback hosts
and explicit bundled policy ports (`11434`, `11435`, or `8000`) when
registering the OpenShell provider route.
- Keep host validation on the user-entered URL; on credential reuse,
update an existing provider to the gateway alias without exporting or
replacing its credential, then configure inference with `--no-verify`.
- Extract the focused transformation and recovery logic from
`remote.ts`, and document the exact supported URL shapes, bundled ports,
bridge reachability, and `0.0.0.0` exposure mitigations.
- Add unit, recovery-integration, docs, fast E2E-support, and hermetic
live inference-routing coverage for the gateway registration contract.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [x] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates
<!-- Check exactly one tests line and one docs line. Check other lines
when applicable. Add every requested justification or approval
reference. -->
- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [x] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification: This is the
clean-history replacement for NVIDIA#6331 after the maintainer sweep at
NVIDIA#6331 (review).
The replacement preserves the reviewed gateway-routing change and adds
the requested adjacent `0.0.0.0` exposure warning instead of accepting
the risk silently.
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each applicable item only when supported by the requested
evidence. Run targeted tests once per relevant change set and rerun
after later edits or hook autofixes that can affect the tested behavior.
Do not rerun hook-covered checks. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or
`npm run check:diff` passed when hooks were skipped or unavailable
- [x] Targeted behavior tests pass for the current change set, or tests
are marked not applicable above — command/result or justification:
focused CLI, integration, docs, E2E-support, mock-parity,
project-membership, title, size, and build checks passed; `npm run docs`
reported 0 errors and 2 existing warnings; live run
[29129492764](https://github.com/NVIDIA/NemoClaw/actions/runs/29129492764)
passed `inference-routing`, `network-policy`, `onboard-repair`,
`onboard-resume`, and `cloud-onboard`.
- [x] Applicable broad gate passed — `npm test` for broad
runtime/test-harness changes; `npm run check` for repo-wide
validation/coverage changes — command/result: `env -u SSH_CONNECTION -u
SSH_CLIENT -u SSH_TTY npm test` passed 1,383 test files and 15,864 tests
(39 skipped, 1 todo).
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only)
- [x] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Compatible inference endpoints using supported loopback addresses and
ports can now be automatically routed through the sandbox gateway.
- Rebuilds can restore compatible endpoint routing without requiring
stored host credentials.
- Endpoint paths, query strings, and fragments are preserved during
automatic routing.

- **Documentation**
- Expanded guidance explains supported endpoints, routing behavior,
validation limits, and runtime status checks.
  - Clarified credential handling during gateway recovery.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Co-authored-by: Carlos Villela <cvillela@nvidia.com>
Co-authored-by: Charan Jagwani <cjagwani@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: inference Inference routing, serving, model selection, or outputs area: local-models Local model providers, downloads, launch, or connectivity area: routing Request routing, policy routing, model selection, or fallback logic area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression v0.0.80 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Linux][Inference] nemoclaw sandbox returns 503 when proxying inference requests to local vLLM custom endpoint via inference.local gateway

4 participants