Skip to content

feat(inference): route DNS-backed HTTPS custom endpoints through a pinned adapter#6906

Open
DisturbedSage5840C wants to merge 13 commits into
NVIDIA:mainfrom
DisturbedSage5840C:feat/https-pinning-transport-6141
Open

feat(inference): route DNS-backed HTTPS custom endpoints through a pinned adapter#6906
DisturbedSage5840C wants to merge 13 commits into
NVIDIA:mainfrom
DisturbedSage5840C:feat/https-pinning-transport-6141

Conversation

@DisturbedSage5840C

@DisturbedSage5840C DisturbedSage5840C commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Summary

inference set --endpoint-url previously failed closed for any DNS-backed HTTPS custom endpoint, because NemoClaw could not pin the downstream TCP peer while preserving TLS SNI/Host across the OpenShell runtime boundary. This adds an HTTPS Pin Runtime adapter so inference set on an already-onboarded sandbox can route a DNS-backed HTTPS endpoint through a local, SNI-pinned reverse proxy instead of rejecting it, while the real upstream hostname never reaches the sandbox, OpenShell's provider/network-policy config, or the persisted sandbox registry.

Related Issue

Fixes #6141

Changes

  • Add the HTTPS Pin Runtime adapter (src/lib/inference/https-pin-runtime.ts, https-pin-runtime-adapter.ts, https-pin-runtime-adapter-forward.ts): a local reverse-proxy/control-plane that re-validates the resolved peer IP is still public after SSRF validation, then terminates a pinned, SNI-correct outbound TLS connection to the real upstream hostname. The sandbox only ever sees a local http://host.openshell.internal:<port>/route/<id>/... route. The detached adapter process is spawned directly from its own compiled dist/lib/inference/https-pin-runtime-adapter.js output via a require.main === module guard (matching the existing onboard-session.js dist-spawn precedent), not a hand-maintained scripts/*.js wrapper, to keep the entrypoint TypeScript-only.
  • Wire the adapter into inference set --endpoint-url only (src/lib/actions/inference-set.ts, inference-set-route-containment.ts, inference-set.test-support.ts). This is the current requirement and sole consumer per Design runtime-aware HTTPS pinning transport for DNS-backed inference endpoints #6141; onboarding, Hermes Provider setup, and host-side config set are a direct behavior change away from this path (different validation timing and no already-onboarded sandbox to route through), so they intentionally keep failing closed on DNS-backed HTTPS and now say so explicitly (src/lib/sandbox/config.ts, nemoclaw/src/blueprint/ssrf.ts, src/lib/onboard/inference-providers/hermes.ts) instead of implying broader support. inference-set-route-containment.test.ts, inference-set-compatible-provider.test.ts, inference-set-gateway-route-containment.test.ts, and inference-set-provider-alias.test.ts protect the wiring.
  • Add a dedicated adapter port with reciprocal collision checks (src/lib/core/ports.ts).
  • Allowlist two internal-only child-process env vars used solely to launch the detached adapter (NEMOCLAW_HTTPS_PIN_RUNTIME_ADAPTER_PORT, NEMOCLAW_HTTPS_PIN_RUNTIME_ADAPTER_BOOTSTRAP_ROUTE) in ci/env-var-doc-allowlist.json, matching the existing NEMOCLAW_BEDROCK_RUNTIME_* pattern for the same kind of hidden adapter.
  • Fix a test regression this branch's new import chain exposed: registry-recovery-seeded-paths.test.ts's narrow runner.js mock omitted ROOT, which the adapter's SSRF preflight now needs at module-evaluation time.
  • Add a new live E2E target, TC-INF-11 (test/e2e/live/inference-routing.test.ts), with a fake HTTPS-compatible upstream (test/e2e/live/https-pin-compatible-server.ts) published behind a real trycloudflare.com quick tunnel (reusing startPublicMcpHttpsTunnel in test/e2e/live/mcp-bridge-servers.ts), so the adapter's real TLS trust-chain and SNI-pinned forwarding are exercised end to end, not just mocked.
  • Update docs (docs/inference/custom-endpoint-security.mdx, docs/reference/commands.mdx, docs/reference/troubleshooting.mdx) to describe the adapter and its scope; the OpenClaw/Hermes-only new section is gated with AgentOnly variant="openclaw,hermes" since Deep Agents sandboxes change provider/model through onboard --fresh --recreate-sandbox, not inference set.
  • Fix a PR Review Advisor Blocker finding (PRA-1): the adapter's HTTP server rejects every sandbox-facing request that doesn't present the adapter's own bearer token, but inference set was registering the operator's real upstream secret as the persisted route credentialEnv instead of the adapter's token, so requests through the pinned route always 401'd. finalizeInferenceSetRoute (inference-set-route-containment.ts) now stages the adapter's own credentialEnv/token via process.env and persists that env var name as the route credential; the real secret continues to authenticate only the adapter's own outbound leg to the real provider (unchanged). Covered by a new inference-set-https-pin-runtime.test.ts (asserts both legs of the credential handoff for compatible-endpoint and compatible-anthropic-endpoint) plus an updated assertion in inference-set-compatible-provider.test.ts that had pinned the pre-fix value.
  • Fix a CodeRabbit Critical finding: the adapter's control-plane route-registration endpoint (PUT /control/routes/:routeId) accepted requests from any caller that presented the adapter's bearer token, with no restriction on the connecting address. Restrict it to loopback callers only (https-pin-runtime-adapter.ts), since the control plane is only ever meant to be reached from the host CLI process that spawned the adapter, not from the sandbox network the adapter's own data-plane route serves.
  • Fix a PR Review Advisor Major finding: concurrent inference set invocations could race past the "is a healthy adapter already running" check in ensureAdapterProcessLocked and both decide to kill/respawn the shared adapter process. Serialize that read-check-kill-spawn decision behind the existing withAdapterLock file lock (https-pin-runtime-adapter.ts).
  • Wire the previously-unused validateHttpsPinRuntimeAdapterPort into the adapter's own bind path (https-pin-runtime-adapter.ts) instead of leaving it as dead code exported but never called.
  • Fix a 408-timeout socket-lifecycle issue in the forward path (https-pin-runtime-adapter-forward.ts): the client socket was left open after a 408 response instead of being destroyed promptly, and the in-flight upstream request was not canceled when the client disconnected early, leaking the outbound connection. Both are covered by new regression tests.
  • Scope the HTTPS Pin Runtime adapter hint added to SSRF-rejection messages (sandbox/config.ts) to the inference set --endpoint-url call site only; the generic config validator shared with plain config set has no way to know the field it's validating is an inference endpoint, so it must not mention the adapter. Covered by a new regression test in inference-set-endpoint-security.test.ts.
  • Rewrite E2E TC-INF-11 (test/e2e/live/inference-routing.test.ts) to onboard with a placeholder provider/model and exercise inference set --endpoint-url directly against the already-onboarded sandbox, instead of asserting on the adapter at onboarding time — the adapter only ever applies post-onboard, so the original test shape exercised the wrong entrypoint.
  • Fix a CodeRabbit finding: the read-credential/call-adapter/write-token sequence inside finalizeInferenceSetRoute (inference-set-route-containment.ts) stages the adapter's bearer token into the single shared HTTPS_PIN_RUNTIME_ADAPTER_PROVIDER_CREDENTIAL_ENV slot on process.env. The per-sandbox/per-gateway locks already held around this call do not serialize two concurrent calls targeting different sandboxes/gateways against each other, so two concurrent adapter-route provisions could interleave their read/call/write and clobber each other's staged token. Now serialized behind the existing withMcpLifecycleLock primitive, keyed by a fixed process-wide sentinel. Covered by a new regression test that deliberately gates one call mid-flight and proves a second, concurrent call for a different gateway cannot reach its own adapter call until the first releases the lock (and fails without the fix, verified by a local revert-and-confirm).
  • Address a CodeRabbit finding on inference-set-https-pin-runtime.test.ts: the "never persists the real upstream secret" test now explicitly stubs COMPATIBLE_API_KEY as unset (vi.stubEnv("COMPATIBLE_API_KEY", undefined)) instead of relying on the ambient shell/CI environment not happening to have it set.
  • Fix a PR Review Advisor Warning: the adapter's /route/:id data-plane endpoint accepted a forwarding request from any caller presenting the shared bearer token, with no restriction on the connecting address, even though the control port binds 0.0.0.0 to stay reachable from the sandbox's Docker bridge network. Add isPrivateNetworkRemoteAddress (https-pin-runtime-adapter.ts) and gate /route/:id on it (loopback, RFC1918, and IPv6 unique-local/link-local), so a peer reaching the port from outside the intended sandbox-to-host boundary can no longer replay the token against a real, already-registered route. The 0.0.0.0 bind itself is unchanged. Covered by new regression tests asserting a public address is rejected while the Docker-bridge and loopback addresses still pass through to route lookup.
  • Fix a PR Review Advisor Warning: TC-INF-11 proved the pinned adapter reaches the real upstream once, at inference set registration time, but never proved it resists a DNS record change afterward — the exact SSRF/DNS-rebinding class the pinning mechanism exists to close. Extend TC-INF-11 (test/e2e/live/inference-routing.test.ts) to remap the tunnel hostname to an unreachable RFC 5737 documentation address after inference set succeeds (reusing the dns-rebinding-hosts-fixture.ts helpers already proven in mcp-bridge.test.ts), then repeat the chat completion and assert it still reaches the original target through the adapter's already-pinned addresses.
  • Fix a CodeRabbit finding: the adapter-recovery-lock unit tests (https-pin-runtime-adapter.test.ts) exercised __test.tryAcquireAdapterLock/__test.withAdapterLock against the statically-imported module's LOCK_PATH, which is derived from this machine's real os.homedir() — the same lock file a genuinely-running adapter would use, so the tests could steal or wedge a real adapter's lock (or collide across parallel test workers). Isolate the describe("adapter recovery lock (#6141)", ...) block behind a per-test temp HOME (vi.stubEnv + vi.resetModules() + a fresh dynamic import("./https-pin-runtime-adapter")), so each test gets its own LOCK_PATH under a throwaway .nemoclaw directory.
  • Fix a CodeRabbit finding: the negative-lookahead regex asserting the generic config-validator message never mentions inference set/HTTPS Pin Runtime adapter (inference-set-endpoint-security.test.ts) used a capturing alternation group without the case-insensitive flag, so a differently-capitalized variant of either phrase would slip past the assertion undetected. Add a non-capturing group and the i flag.
  • Fix a PR Review Advisor Blocker finding (PRA-1): when the shared adapter process dies and ensureAdapterProcessLocked recovers it, only the one route belonging to the inference set invocation that triggered recovery gets re-seeded into the fresh process; every other previously-registered route silently 404'd as route_not_found, indistinguishable from a route that was never configured. The adapter never persists plaintext credentials by design, so a route's other, non-triggering owner can't be transparently recovered without an operator re-running inference set --endpoint-url. Preserve each route's already-persisted, secretless metadata (targetBaseUrl, pinnedAddresses, providerType, credentialHash) across respawn instead of wiping it to {} (extractPersistedRoutes/computeRespawnState, https-pin-runtime-adapter.ts), marking it orphanedAt, and have the adapter respond 503 route_needs_recovery with an actionable re-run message for a route it knows was orphaned by the last restart, distinct from the 404 route_not_found it still returns for a route that never existed. The new NEMOCLAW_HTTPS_PIN_RUNTIME_ADAPTER_ORPHANED_ROUTE_IDS child-process env var (no credentials) carries the computed orphan set into the respawned process; allowlisted in ci/env-var-doc-allowlist.json. Covered by a new computeRespawnState pure-function unit test and server-level regression tests asserting the 503/404 split and that re-registering an orphaned route heals it.

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
  • Docs not applicable — justification:
  • 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:
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

  • PR description includes a Signed-off-by: line 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 — command/result or justification: npx vitest run --project cli src/lib/inference/https-pin-runtime.test.ts src/lib/inference/https-pin-runtime-adapter.test.ts src/lib/inference/https-pin-runtime-adapter-forward.test.ts src/lib/actions/inference-set-compatible-provider.test.ts src/lib/actions/inference-set-gateway-route-containment.test.ts src/lib/actions/inference-set-provider-alias.test.ts src/lib/actions/inference-set-https-pin-runtime.test.ts src/lib/inference/gateway-route-compatibility.test.ts src/lib/registry-recovery-seeded-paths.test.ts src/lib/core/ports.test.ts → 123+/123+ passed (new PRA-1 fix commit); npx vitest run --project plugin nemoclaw/src/blueprint/ssrf.test.ts → 101/101 passed; full cli and plugin projects run separately with all remaining failures confirmed pre-existing/environmental via git stash A/B on a clean tree (unrelated to this change: workspace-path-with-space Node subprocess flakiness). npm run typecheck:cli and npm --prefix nemoclaw run typecheck both clean. Sanity-verified the new PRA-1 regression coverage is non-vacuous by reverting the fix locally and confirming the affected assertions fail, then restoring it. TC-INF-11 (new live E2E target) is written, typechecked, and linted; a local run against a real Docker daemon and cloudflared reached the public-tunnel readiness probe and failed there — root-caused to this contributor machine's local DNS resolver returning NXDOMAIN for the freshly-minted trycloudflare.com subdomain while public resolvers (1.1.1.1, 8.8.8.8) resolve it correctly, confirmed by reproducing the identical failure with a minimal standalone tunnel script outside the test harness. Not a code defect; CI's live E2E lane runs with normal DNS and will execute this target for real.

    Follow-up review-response commit (loopback restriction, adapter-recovery lock, dead-code wiring, 408/disconnect socket fixes, config hint scoping, TC-INF-11 entrypoint rewrite, credential-env-write race lock, explicit env stub): npm run typecheck:cli clean (empty tsc output); npx vitest run --project cli src/lib/actions/inference-set-endpoint-security.test.ts src/lib/actions/inference-set-gateway-route-containment.test.ts src/lib/actions/inference-set-https-pin-runtime.test.ts src/lib/inference/https-pin-runtime-adapter-forward.test.ts src/lib/inference/https-pin-runtime-adapter.test.ts → 61/61 passed; npx biome check clean on every file this commit touched. The new credential-env-write race regression test was sanity-checked non-vacuous by temporarily reverting the lock and confirming it fails (expected [ 'a-start', 'b-start' ] to deeply equal [ 'a-start' ]), then restoring the fix. Commit 2ee978311 verified via gh api repos/NVIDIA/NemoClaw/commits/2ee978311 --jq '.commit.verification.verified'true.

    Follow-up review-response commit (private-network route-forwarding restriction, DNS-rebinding coverage for TC-INF-11): npm run typecheck:cli clean; npx biome check test/e2e/live/inference-routing.test.ts src/lib/inference/https-pin-runtime-adapter.ts src/lib/inference/https-pin-runtime-adapter.test.ts clean; npx vitest run --project cli src/lib/inference/https-pin-runtime-adapter.test.ts → 23/23 passed. The new private-network-restriction regression tests target an unregistered route ID and distinguish pass/fail purely by the returned 404 error.code (not_found from the gate vs. route_not_found from route lookup), since the test harness's fake response object does not implement a full writable stream and cannot exercise the piped-forwarding path. The DNS-rebinding extension to TC-INF-11 is a live-E2E-only change (requires a real Docker sandbox and cloudflared tunnel) and could not be executed on this contributor machine for the same DNS-resolver reason noted above for the original TC-INF-11; it is typechecked, linted, and reuses the same dns-rebinding-hosts-fixture.ts helpers already exercised by mcp-bridge.test.ts's DNS-rebinding assertion. Commit 671c8d3c6 verified via gh api repos/NVIDIA/NemoClaw/commits/671c8d3c6 --jq '.commit.verification.verified'true.

    Follow-up review-response commit (adapter-lock test isolation, case-insensitive config-hint regex): npm run typecheck:cli clean; npx biome check src/lib/inference/https-pin-runtime-adapter.test.ts src/lib/actions/inference-set-endpoint-security.test.ts clean; npx vitest run --project cli src/lib/inference/https-pin-runtime-adapter.test.ts src/lib/actions/inference-set-endpoint-security.test.ts → 35/35 passed, confirming the new per-test temp-HOME dynamic-import isolation actually acquires/releases a working lock rather than silently no-op'ing. Commit 981d40cb0 verified via gh api repos/NVIDIA/NemoClaw/commits/981d40cb0 --jq '.commit.verification.verified'true.

    Follow-up review-response commit (PRA-1: recover orphaned adapter routes distinctly after restart): npm run typecheck:cli clean; npx biome check src/lib/inference/https-pin-runtime-adapter.ts src/lib/inference/https-pin-runtime-adapter.test.ts ci/env-var-doc-allowlist.json clean; npx vitest run --project cli src/lib/inference/https-pin-runtime-adapter.test.ts → 27/27 passed (includes the two new computeRespawnState unit tests and the two new server-level 503/404/re-registration tests); npx vitest run --project cli src/lib/actions/inference-set-compatible-provider.test.ts src/lib/actions/inference-set-https-pin-runtime.test.ts src/lib/inference/https-pin-runtime-adapter-forward.test.ts → 37/37 passed, confirming the new orphanedRouteIds server option and persisted-state shape change don't regress the surrounding route-registration/forwarding/credential-handoff behavior. Commit 63f99e24d verified via gh api repos/NVIDIA/NemoClaw/commits/63f99e24d --jq '.commit.verification.verified'true.

    Sync commits (bring the branch up to date with a fast-moving main, twice): a signed-off merge with no conflicts (04d56e0ea), then a second signed-off merge (dcb6c72a7) that resolved one genuine, trivial content conflict in src/lib/registry-recovery-seeded-paths.test.ts (both branches independently added the same two-property runner.js mock return in a different key order — kept the semantically-identical main ordering) and one type error surfaced by an unrelated upstream rename (killLocalAdapterPid's processNeedle option renamed to processMatcher in local-adapter-lifecycle.ts; updated the one call site in https-pin-runtime-adapter.ts). npm run typecheck:cli clean; npx biome check src/lib/inference/https-pin-runtime-adapter.ts src/lib/registry-recovery-seeded-paths.test.ts clean; npx vitest run --project cli src/lib/inference/https-pin-runtime-adapter.test.ts src/lib/registry-recovery-seeded-paths.test.ts src/lib/inference/local-adapter-lifecycle.test.ts → 48/48 passed. Commit dcb6c72a7 verified via gh api repos/NVIDIA/NemoClaw/commits/dcb6c72a7 --jq '.commit.verification.verified'true.

  • 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)


Signed-off-by: DisturbedSage maruteymani31@gmail.com

Summary by CodeRabbit

  • New Features
    • Enabled DNS-backed HTTPS custom inference endpoints after sandbox onboarding by routing through a local HTTPS Pin Runtime adapter that preserves the upstream hostname while protecting credentials.
    • Added configurable HTTPS Pin Runtime adapter port (default 11438) with conflict validation.
  • Bug Fixes
    • Improved fail-closed behavior and clarified when HTTPS restrictions apply (onboarding vs persisted routing).
    • Added stricter protections for adapter request/redirect handling (e.g., size/timeouts and blocked redirects).
  • Documentation
    • Updated custom endpoint security guidance, inference set references, and troubleshooting for post-onboarding switching.
  • Tests
    • Expanded unit, integration, and end-to-end coverage for routing, credentials, and adapter behavior.
  • Chores
    • Updated CI environment-variable allowlist for the adapter.

…nned adapter (NVIDIA#6141)

`inference set --endpoint-url` on an already-onboarded sandbox previously
failed closed for any DNS-backed HTTPS endpoint, because NemoClaw could not
pin the downstream TCP peer while preserving TLS SNI/Host across the
OpenShell runtime boundary. Add an HTTPS Pin Runtime adapter: a local
reverse-proxy/control-plane that re-validates the resolved peer IP is still
public after SSRF validation, then terminates a pinned, SNI-correct outbound
TLS connection to the real upstream hostname. The sandbox, its OpenShell
provider config, network policy, and the persisted sandbox registry only
ever see a local `host.openshell.internal` route; the real hostname never
crosses the OpenShell runtime boundary.

This support is scoped to `inference set` on an already-onboarded sandbox.
Onboarding, Hermes Provider setup, and host-side `config set` still reject
DNS-backed HTTPS URLs, and their fail-closed messages/comments now say so
explicitly instead of implying broader support.

Also fixes a test regression this branch's new import chain exposed:
`registry-recovery-seeded-paths.test.ts`'s narrow `runner.js` mock omitted
`ROOT`, which the adapter's SSRF preflight now needs at module-evaluation
time.

Signed-off-by: DisturbedSage <maruteymani31@gmail.com>
Copilot AI review requested due to automatic review settings July 15, 2026 06:19
@copy-pr-bot

copy-pr-bot Bot commented Jul 15, 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.

@coderabbitai

coderabbitai Bot commented Jul 15, 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 a local HTTPS Pin Runtime adapter for DNS-backed HTTPS inference endpoints, including pinned forwarding, lifecycle management, inference-set integration, port validation, documentation, unit tests, and end-to-end coverage.

Changes

HTTPS Pin Runtime adapter

Layer / File(s) Summary
Endpoint contracts and port validation
src/lib/inference/https-pin-runtime.ts, src/lib/core/ports.ts, src/lib/core/ports.test.ts, ci/env-var-doc-allowlist.json
Adds endpoint eligibility, credential mapping, route URL construction, configurable adapter port 11438, conflict validation, environment allowlist entries, and related tests.
Pinned HTTPS request forwarding
src/lib/inference/https-pin-runtime-adapter-forward.ts, src/lib/inference/https-pin-runtime-adapter-forward.test.ts
Adds bounded request forwarding with credential/header handling, TLS SNI pinning, redirect blocking, timeouts, structured errors, cancellation, and tests.
Adapter server and process lifecycle
src/lib/inference/https-pin-runtime-adapter.ts, src/lib/inference/https-pin-runtime-adapter.test.ts
Adds authenticated route registration, health checks, bootstrap configuration, detached-process reuse, persisted metadata, route registration, locking, and lifecycle tests.
Inference-set adapter integration
src/lib/actions/*, src/lib/sandbox/config.ts, nemoclaw/src/blueprint/ssrf.ts, src/lib/onboard/*
Routes eligible DNS-backed HTTPS endpoints through the adapter, passes provider credentials at invocation time, updates dependency wiring and registry metadata, and preserves onboarding/config rejection behavior.
End-to-end coverage and user guidance
test/e2e/live/*, docs/inference/*, docs/reference/*
Adds a fake HTTPS-compatible upstream, configurable tunnel readiness probes, DNS-backed HTTPS routing coverage, and updated onboarding/runtime documentation.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant InferenceSet
  participant HTTPSPinRuntimeAdapter
  participant PublicHTTPSProvider
  InferenceSet->>HTTPSPinRuntimeAdapter: Register DNS-pinned route and credential
  InferenceSet->>HTTPSPinRuntimeAdapter: Persist adapter route URL
  HTTPSPinRuntimeAdapter->>PublicHTTPSProvider: Forward request using pinned address and TLS SNI
  PublicHTTPSProvider-->>HTTPSPinRuntimeAdapter: Return provider response
  HTTPSPinRuntimeAdapter-->>InferenceSet: Stream filtered response
Loading

Suggested labels: feature, area: cli, area: inference, area: networking, area: security, area: e2e

Suggested reviewers: cv

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 17.07% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: routing DNS-backed HTTPS inference endpoints through a pinned adapter.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@github-actions

github-actions Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Blocking findings reported

Advisor assessment: Blockers require maintainer review
Next action: Review the blockers below.
Findings: 1 blocker · 0 warnings · 0 suggestions

Model lanes

  • GPT-5.6 Terra (primary): Failed after a partial review · low confidence · 1 blocker · 0 warnings · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Failed after a partial review · low confidence · 1 blocker · 5 warnings · 1 suggestion

Nemotron output stays in workflow artifacts and does not change the assessment above.

E2E guidance

Advisory only. E2E / PR Gate selects and runs jobs independently.

Recommended E2E: cloud-onboard, credential-sanitization, full-e2e, hermes-e2e, security-posture, inference-routing, network-policy, onboard-repair, onboard-resume

Blockers

PRA-1 Blocker — Register the adapter bearer with the OpenShell provider before switching routes

  • Location: src/lib/actions/inference-set.ts:783
  • Category: security
  • Problem: For a DNS-backed HTTPS route, finalization replaces the persisted credentialEnv with NEMOCLAW_HTTPS_PIN_RUNTIME_ADAPTER_TOKEN and stages a fresh route token in process.env, but the subsequent OpenShell mutation only runs `inference set` with provider/model arguments. It does not update the compatible provider's existing credential key, which remains COMPATIBLE_API_KEY in the checked-in live E2E expectation. OpenShell therefore sends the upstream credential to an adapter that authenticates data-plane requests only with the distinct route token, and rejects the route request with 401.
  • Impact: The newly supported DNS-backed HTTPS inference route cannot authenticate sandbox requests. Attempting to work around it by sharing a provider-wide adapter token would also violate the intended per-route credential isolation.
  • Fix: Before committing the route change, install or rotate the adapter route token through the credential mechanism actually consumed by the selected OpenShell compatible provider, and ensure the binding remains route-scoped on shared gateways.
  • Verification: Inspect the OpenShell provider credential binding after `inference set --endpoint-url https://...`: it must resolve the adapter token rather than COMPATIBLE_API_KEY's upstream secret; then make one sandbox inference request and confirm the adapter accepts it.
  • Test coverage: Add an integration-level test that asserts the OpenShell provider credential source is changed to the route token and that a request through the registered sandbox route receives 2xx, while the original upstream credential is rejected by the adapter.
  • Evidence: src/lib/actions/inference-set-route-containment.ts finalizes eligible HTTPS routes with adapterCredentialEnv and writes process.env[adapter.credentialEnv] = adapter.token. src/lib/actions/inference-set.ts invokes OpenShell inference set with only gateway, provider, model, and optional no-verify, then persists registry metadata around line 783. test/e2e/live/inference-routing.test.ts TC-INF-11 still asserts `Credential keys: COMPATIBLE_API_KEY` after the HTTPS adapter switch. src/lib/inference/https-pin-runtime-adapter.ts authenticates each /route/:id request against route.routeToken and returns 401 on mismatch.

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

Copilot AI 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.

Pull request overview

This PR adds an “HTTPS Pin Runtime” adapter to support DNS-backed HTTPS custom inference endpoints specifically for nemoclaw inference set --endpoint-url on already-onboarded sandboxes, avoiding DNS-rebinding TOCTOU by routing traffic through a local pinned/SNI-correct forwarding proxy and ensuring the runtime only sees a host.openshell.internal route.

Changes:

  • Introduces a host-local HTTPS pinning adapter (server + forwarder + lifecycle + script entrypoint) and wires eligibility/routing into inference set --endpoint-url.
  • Expands port reservation/collision validation to include a dedicated HTTPS-pin adapter port (11438) and threads it through gateway recovery/runtime validation.
  • Adds/updates unit + integration + live E2E coverage and updates docs/error messaging to clarify scope (onboarding/blueprint/config paths still fail closed for DNS-backed HTTPS).

Reviewed changes

Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/e2e/live/mcp-bridge-servers.ts Makes cloudflared readiness probing configurable for reuse by HTTPS-pin live E2E.
test/e2e/live/inference-routing.test.ts Adds TC-INF-11 live E2E asserting DNS-backed HTTPS endpoint is routed via adapter and hostname not persisted.
test/e2e/live/https-pin-compatible-server.ts Adds a minimal HTTPS OpenAI-compatible upstream fixture (self-signed locally, tunneled publicly).
src/lib/sandbox/config.ts Updates error messaging to clarify DNS-backed HTTPS support is limited to inference set --endpoint-url.
src/lib/registry-recovery-seeded-paths.test.ts Fixes runner mock to include ROOT needed by new import chain.
src/lib/onboard/inference-providers/hermes.ts Clarifies onboarding still fails closed for DNS-backed HTTPS (adapter not wired there).
src/lib/onboard/gateway-recovery.ts Includes HTTPS-pin adapter port in gateway recovery port configuration.
src/lib/inference/openrouter-runtime-adapter-lifecycle.ts Extends OpenRouter adapter port validation options to account for HTTPS-pin port conflicts.
src/lib/inference/https-pin-runtime.ts Adds eligibility logic + route-id/base-url builders and constants for the HTTPS-pin adapter.
src/lib/inference/https-pin-runtime.test.ts Unit tests for eligibility, credential header mapping, route-id/base-url behavior.
src/lib/inference/https-pin-runtime-adapter.ts Implements shared multi-route adapter server, control-plane, lifecycle/ensure logic, and state persistence.
src/lib/inference/https-pin-runtime-adapter.test.ts Tests adapter auth/health, control-plane route registration, and preflight/credential ordering.
src/lib/inference/https-pin-runtime-adapter-forward.ts Implements pinned-address forwarding preserving TLS SNI/Host and failing closed on redirects.
src/lib/inference/https-pin-runtime-adapter-forward.test.ts Tests header handling, body limits, timeout behavior, redirect blocking, and TLS SNI validation.
src/lib/core/ports.ts Adds HTTPS-pin adapter port constant and introduces validateHttpsPinRuntimeAdapterPort.
src/lib/core/ports.test.ts Adds coverage for gateway conflict checks + HTTPS-pin adapter port validation.
src/lib/actions/inference-set.ts Plumbs ensureHttpsPinRuntimeAdapter dependency into inference-set routing path.
src/lib/actions/inference-set.test-support.ts Adds test dep injection/stub for ensureHttpsPinRuntimeAdapter.
src/lib/actions/inference-set-route-containment.ts Routes DNS-backed HTTPS endpoints through the adapter during endpoint normalization/finalization.
src/lib/actions/inference-set-provider-alias.test.ts Updates SSRF guidance tests to use the HTTPS-pin adapter guard path.
src/lib/actions/inference-set-gateway-route-containment.test.ts Adjusts containment tests to use HTTP endpoints where DNS-pinning rewrite is exercised directly.
src/lib/actions/inference-set-compatible-provider.test.ts Updates compatible-provider inference-set expectations for adapter-routed endpoint persistence and SSRF preflight usage.
scripts/https-pin-runtime-adapter.js Adds Node launcher for the compiled adapter entrypoint.
nemoclaw/src/blueprint/ssrf.ts Clarifies onboarding still rejects DNS-backed HTTPS and points users to inference set --endpoint-url.
docs/reference/troubleshooting.mdx Updates troubleshooting guidance to reflect inference set support via adapter.
docs/reference/commands.mdx Documents DNS-backed HTTPS behavior and the adapter routing model for inference set.
docs/inference/custom-endpoint-security.mdx Adds an AgentOnly section describing adapter scope and security posture for DNS-backed HTTPS endpoints.
ci/env-var-doc-allowlist.json Allowlists internal env vars used to spawn/bootstrap the HTTPS-pin adapter process.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/lib/inference/https-pin-runtime-adapter.ts
Comment thread src/lib/core/ports.ts

@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: 9

🧹 Nitpick comments (2)
src/lib/inference/https-pin-runtime.ts (1)

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

Optional: de-duplicate the two route-URL builders.

buildHttpsPinRouteBaseUrl and buildHttpsPinRouteLoopbackBaseUrl are identical except for the origin constant. Could be a single helper taking the origin as a parameter.

♻️ Proposed consolidation
-export function buildHttpsPinRouteBaseUrl(routeId: string, endpointUrl: string): string {
-  const url = parseUrl(endpointUrl);
-  const suffix = url ? url.pathname.replace(/\/+$/, "") : "";
-  return `${HTTPS_PIN_RUNTIME_ADAPTER_BASE_ORIGIN}/route/${routeId}${suffix}`;
-}
-
-/** Host-side (loopback) equivalent of {`@link` buildHttpsPinRouteBaseUrl}, for health checks and the control plane. */
-export function buildHttpsPinRouteLoopbackBaseUrl(routeId: string, endpointUrl: string): string {
-  const url = parseUrl(endpointUrl);
-  const suffix = url ? url.pathname.replace(/\/+$/, "") : "";
-  return `${HTTPS_PIN_RUNTIME_ADAPTER_LOOPBACK_ORIGIN}/route/${routeId}${suffix}`;
-}
+function buildHttpsPinRouteUrl(origin: string, routeId: string, endpointUrl: string): string {
+  const url = parseUrl(endpointUrl);
+  const suffix = url ? url.pathname.replace(/\/+$/, "") : "";
+  return `${origin}/route/${routeId}${suffix}`;
+}
+
+export function buildHttpsPinRouteBaseUrl(routeId: string, endpointUrl: string): string {
+  return buildHttpsPinRouteUrl(HTTPS_PIN_RUNTIME_ADAPTER_BASE_ORIGIN, routeId, endpointUrl);
+}
+
+/** Host-side (loopback) equivalent of {`@link` buildHttpsPinRouteBaseUrl}, for health checks and the control plane. */
+export function buildHttpsPinRouteLoopbackBaseUrl(routeId: string, endpointUrl: string): string {
+  return buildHttpsPinRouteUrl(HTTPS_PIN_RUNTIME_ADAPTER_LOOPBACK_ORIGIN, routeId, endpointUrl);
+}
🤖 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/inference/https-pin-runtime.ts` around lines 95 - 107, De-duplicate
buildHttpsPinRouteBaseUrl and buildHttpsPinRouteLoopbackBaseUrl by extracting
their shared URL construction into one helper that accepts the origin as a
parameter. Keep both public builders’ existing signatures and behavior, passing
the appropriate adapter or loopback origin from each.
src/lib/core/ports.ts (1)

81-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Recommended: extract shared port-conflict-checking logic.

validateGatewayPort, validateOpenRouterRuntimeAdapterPort, and now validateHttpsPinRuntimeAdapterPort are three near-identical ~45-line functions differing only in which entries appear in their reservedDefaults/conflicts arrays. Each new adapter port addition (like this PR's HTTPS Pin adapter) requires updating all three in lockstep, which is exactly what happened here and is easy to miss for a future adapter. A shared helper taking { dashboardRangeStart, dashboardRangeEnd, reservedDefaults, conflicts } would collapse this to one implementation and remove the risk of the lists silently drifting.

Also applies to: 143-237

🤖 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/core/ports.ts` around lines 81 - 131, Extract the duplicated
validation logic from validateGatewayPort, validateOpenRouterRuntimeAdapterPort,
and validateHttpsPinRuntimeAdapterPort into one shared helper accepting the
dashboard range, reservedDefaults, and conflicts. Update each public validator
to supply its applicable entries to that helper, ensuring all adapter validators
use the same overlap, reserved-port, and configured-conflict checks without
duplicated lists.
🤖 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 `@ci/env-var-doc-allowlist.json`:
- Around line 30-37: Add NEMOCLAW_HTTPS_PIN_RUNTIME_ADAPTER_TOKEN to
ci/env-var-doc-allowlist.json, using the same internal-only rationale as the
existing NEMOCLAW_HTTPS_PIN_RUNTIME_ADAPTER_PORT and
NEMOCLAW_HTTPS_PIN_RUNTIME_ADAPTER_BOOTSTRAP_ROUTE entries so
scripts/check-env-var-docs.ts accepts the variable read by https-pin-runtime.ts.

In `@scripts/https-pin-runtime-adapter.js`:
- Around line 1-14: Replace the JavaScript entry point at
scripts/https-pin-runtime-adapter.js with the repository-supported
TypeScript/build entry path. In src/lib/inference/https-pin-runtime-adapter.ts,
update the process needle near line 73 and the executable resolution in the
startHttpsPinRuntimeAdapterFromEnv flow near lines 421-424 to reference the
generated executable path instead of the blocked .js source file.

In `@src/lib/inference/https-pin-runtime-adapter-forward.test.ts`:
- Around line 212-216: Update both CA setup guards in the certificate-validation
and mismatch tests to fail the test when resolveCaSetup returns an unsuccessful
result, rather than returning early. Preserve the existing successful setup path
so both tests always exercise TLS certificate/SNI verification when setup
succeeds.

In `@src/lib/inference/https-pin-runtime-adapter-forward.ts`:
- Around line 86-91: Update the timeout callback in the forwarding request flow
to reject with the 408 ForwardHttpError without immediately calling
req.destroy(); coordinate socket draining and closure from the caller after
sendForwardError() completes so the JSON error response can flush. Add a
regression test for the timeout path that verifies the client receives HTTP 408
with error code request_timeout.
- Around line 186-230: Update forwardHttpsPinnedRequest to observe the
downstream req close/aborted events and destroy upstreamReq when the client
disconnects, using the existing failure/cleanup flow without affecting normal
completion. Add a test covering downstream disconnection and verify the upstream
request is aborted promptly.

In `@src/lib/inference/https-pin-runtime-adapter.ts`:
- Around line 272-297: Separate control-plane authentication from sandbox route
authentication: in src/lib/inference/https-pin-runtime-adapter.ts lines 272-297,
require distinct control credentials for route registration and overwrite
operations instead of options.token; in
src/lib/inference/https-pin-runtime-adapter.ts lines 218-240, revalidate
protocol, hostname, and pinned public addresses at the control boundary; in
src/lib/inference/https-pin-runtime-adapter.test.ts lines 90-130, use separate
credentials and verify the sandbox route credential cannot register or overwrite
routes.
- Around line 612-649: Serialize the entire recovery transaction in
ensureHttpsPinRuntimeAdapter with a host-scoped lifecycle lock covering
persisted PID/token reads, stale-process cleanup, spawning, health waiting, and
persistence, so concurrent calls cannot clobber shared state or terminate a
healthy respawn. Add a test that invokes concurrent cold starts and verifies
they converge on one healthy adapter and consistent PID_PATH/TOKEN_PATH state.

In `@src/lib/sandbox/config.ts`:
- Around line 753-761: Update the validation error in the HTTPS DNS-backed URL
branch of the persisted sandbox config validator to use only generic rejection
guidance. Move the `inference set --endpoint-url` hint to the call site that
handles the inference endpoint field, preserving it there while keeping
arbitrary config validation inference-agnostic.

In `@test/e2e/live/inference-routing.test.ts`:
- Around line 379-393: Update the TC-INF-11 setup around onboardSandbox to
onboard with a supported/default route rather than endpointUrl, then invoke the
public inference set flow with --endpoint-url using the DNS-backed URL. Keep the
existing success assertion and ensure subsequent registry, policy, and chat
checks run only after that inference set operation succeeds.

---

Nitpick comments:
In `@src/lib/core/ports.ts`:
- Around line 81-131: Extract the duplicated validation logic from
validateGatewayPort, validateOpenRouterRuntimeAdapterPort, and
validateHttpsPinRuntimeAdapterPort into one shared helper accepting the
dashboard range, reservedDefaults, and conflicts. Update each public validator
to supply its applicable entries to that helper, ensuring all adapter validators
use the same overlap, reserved-port, and configured-conflict checks without
duplicated lists.

In `@src/lib/inference/https-pin-runtime.ts`:
- Around line 95-107: De-duplicate buildHttpsPinRouteBaseUrl and
buildHttpsPinRouteLoopbackBaseUrl by extracting their shared URL construction
into one helper that accepts the origin as a parameter. Keep both public
builders’ existing signatures and behavior, passing the appropriate adapter or
loopback origin from each.
🪄 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: 5369811c-4d9e-47ec-867f-a90aa6990e94

📥 Commits

Reviewing files that changed from the base of the PR and between 5b67372 and 0ffb34d.

📒 Files selected for processing (28)
  • ci/env-var-doc-allowlist.json
  • docs/inference/custom-endpoint-security.mdx
  • docs/reference/commands.mdx
  • docs/reference/troubleshooting.mdx
  • nemoclaw/src/blueprint/ssrf.ts
  • scripts/https-pin-runtime-adapter.js
  • src/lib/actions/inference-set-compatible-provider.test.ts
  • src/lib/actions/inference-set-gateway-route-containment.test.ts
  • src/lib/actions/inference-set-provider-alias.test.ts
  • src/lib/actions/inference-set-route-containment.ts
  • src/lib/actions/inference-set.test-support.ts
  • src/lib/actions/inference-set.ts
  • src/lib/core/ports.test.ts
  • src/lib/core/ports.ts
  • src/lib/inference/https-pin-runtime-adapter-forward.test.ts
  • src/lib/inference/https-pin-runtime-adapter-forward.ts
  • src/lib/inference/https-pin-runtime-adapter.test.ts
  • src/lib/inference/https-pin-runtime-adapter.ts
  • src/lib/inference/https-pin-runtime.test.ts
  • src/lib/inference/https-pin-runtime.ts
  • src/lib/inference/openrouter-runtime-adapter-lifecycle.ts
  • src/lib/onboard/gateway-recovery.ts
  • src/lib/onboard/inference-providers/hermes.ts
  • src/lib/registry-recovery-seeded-paths.test.ts
  • src/lib/sandbox/config.ts
  • test/e2e/live/https-pin-compatible-server.ts
  • test/e2e/live/inference-routing.test.ts
  • test/e2e/live/mcp-bridge-servers.ts

Comment thread ci/env-var-doc-allowlist.json
Comment thread scripts/https-pin-runtime-adapter.js
Comment thread src/lib/inference/https-pin-runtime-adapter-forward.test.ts Outdated
Comment thread src/lib/inference/https-pin-runtime-adapter-forward.ts Outdated
Comment thread src/lib/inference/https-pin-runtime-adapter-forward.ts
Comment thread src/lib/inference/https-pin-runtime-adapter.ts Outdated
Comment thread src/lib/inference/https-pin-runtime-adapter.ts Outdated
Comment thread src/lib/sandbox/config.ts
Comment thread test/e2e/live/inference-routing.test.ts Outdated
…tput

Replace the hand-maintained scripts/https-pin-runtime-adapter.js wrapper
with a require.main guard in the compiled module itself, matching the
onboard-session.js dist-spawn precedent and satisfying the
codebase-growth-guardrails check that blocks new .js source files.

Signed-off-by: DisturbedSage <maruteymani31@gmail.com>
Move the shared resolveCaSetup call to module scope and gate the whole
describe block with describe.skipIf(!setup.ok), matching the
corporate-ca-tls-e2e.test.ts precedent, instead of an if-return guard
inside each test body.

Signed-off-by: DisturbedSage <maruteymani31@gmail.com>
… credential (PRA-1, NVIDIA#6141)

The HTTPS-pin runtime adapter's HTTP server rejects every sandbox-facing
request that doesn't present its own bearer token, but inference set was
registering the operator's real upstream secret as the route credential
instead of the adapter's token, so requests through the pinned route
always 401'd. Wire the adapter's returned credentialEnv/token through as
the persisted route credential, while the real secret still authenticates
only the adapter's own outbound leg to the real provider.

Signed-off-by: DisturbedSage <maruteymani31@gmail.com>

@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: 2

🤖 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/actions/inference-set-https-pin-runtime.test.ts`:
- Around line 100-129: In the test case around “never persists the real upstream
secret name as the route credentialEnv even when unset,” explicitly clear the
ambient COMPATIBLE_API_KEY environment variable with vi.stubEnv before calling
runInferenceSet. Keep the existing credentialValue expectation and test behavior
unchanged.
- Around line 95-97: Update the adapter token handling in the inference-set
route-containment flow around the adapter invocation so it no longer writes
shared process.env[NEMOCLAW_HTTPS_PIN_RUNTIME_ADAPTER_TOKEN] state across calls.
Pass the token through invocation-local state to the adapter, or temporarily
restore the previous environment value after use, while preserving resolution
through HTTPS_PIN_RUNTIME_ADAPTER_PROVIDER_CREDENTIAL_ENV.
🪄 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: 8f203d14-942c-4d3f-9001-1486666c9a6b

📥 Commits

Reviewing files that changed from the base of the PR and between b60d21f and 7813a16.

📒 Files selected for processing (6)
  • src/lib/actions/inference-set-compatible-provider.test.ts
  • src/lib/actions/inference-set-https-pin-runtime.test.ts
  • src/lib/actions/inference-set-provider-alias.test.ts
  • src/lib/actions/inference-set-route-containment.ts
  • src/lib/actions/inference-set.test-support.ts
  • src/lib/inference/https-pin-runtime-adapter-forward.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
  • src/lib/actions/inference-set.test-support.ts
  • src/lib/inference/https-pin-runtime-adapter-forward.test.ts
  • src/lib/actions/inference-set-route-containment.ts
  • src/lib/actions/inference-set-provider-alias.test.ts

Comment thread src/lib/actions/inference-set-https-pin-runtime.test.ts
Comment thread src/lib/actions/inference-set-https-pin-runtime.test.ts
…ngs (NVIDIA#6141)

Addresses CodeRabbit and PR Review Advisor findings raised on PR NVIDIA#6906:

- Restrict the adapter's control-plane route registration endpoint to
  loopback callers only, closing a remote route-injection window.
- Acquire the existing MCP lifecycle lock around adapter process
  recovery so concurrent `inference set` calls can no longer race to
  kill/respawn the shared adapter process.
- Wire the previously dead `validateHttpsPinRuntimeAdapterPort` into
  the adapter bind path instead of leaving it unused.
- Destroy the client socket promptly on a 408 timeout in the forward
  path instead of leaving it to linger; add a regression test.
- Cancel the in-flight upstream request when the client disconnects
  early, closing a request-handle leak; add a regression test.
- Scope the HTTPS Pin Runtime adapter hint in sandbox/config.ts to the
  `inference set --endpoint-url` call site only, not the generic
  config validator used by plain `config set`; add a regression test.
- Rewrite E2E TC-INF-11 to onboard with a placeholder and exercise
  `inference set --endpoint-url` directly, instead of asserting on
  onboarding-time behavior for a flow that only applies post-onboard.
- Serialize the read-credential/call-adapter/write-token sequence in
  finalizeInferenceSetRoute behind a process-wide lock, since the
  per-sandbox/per-gateway locks already held do not serialize two
  concurrent adapter-route provisions targeting different
  sandboxes/gateways against each other; add a regression test proving
  two concurrent calls no longer interleave their process.env writes.
- Make the "never persists the real upstream secret" test explicitly
  stub COMPATIBLE_API_KEY as unset instead of relying on the ambient
  shell environment not happening to have it set.

Signed-off-by: DisturbedSage <maruteymani31@gmail.com>
)

The codebase-growth-guardrails check flagged two if statements added by
the previous commit's new tests. Rewrite both to stay linear per repo
convention: the control-plane test's fake-request helper now builds an
emit list with a ternary instead of branching, and the credential-env
race regression test dispatches per-endpoint mock behavior through a
lookup table instead of an if/else.

Signed-off-by: DisturbedSage <maruteymani31@gmail.com>

@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: 2

Caution

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

⚠️ Outside diff range comments (1)
src/lib/inference/https-pin-runtime-adapter.ts (1)

731-792: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Wait for the old adapter to exit before respawning on the same port.

killLocalAdapterPid() only sends kill and removes the pid file; spawnDetachedNodeAdapter() then binds HTTPS_PIN_RUNTIME_ADAPTER_PORT immediately. If the old process is still releasing the socket, the new adapter can hit EADDRINUSE and the startup path fails. Poll for process exit first, or add a bounded bind retry.

🤖 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/inference/https-pin-runtime-adapter.ts` around lines 731 - 792,
Update ensureAdapterProcessLocked and the stale-adapter cleanup flow so the
previously running adapter has exited before spawnDetachedNodeAdapter binds
HTTPS_PIN_RUNTIME_ADAPTER_PORT. After killStaleAdapter, poll for the old process
to terminate with a bounded timeout, or implement an equivalent bounded retry
for binding, while preserving the existing startup failure and cleanup behavior.
🤖 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/actions/inference-set-endpoint-security.test.ts`:
- Around line 61-69: Update the rejection assertion in the test for
rewriteConfigUrlsWithDnsPinning to make the forbidden-message negative lookahead
case-insensitive, so capitalization variants of “inference set” and “HTTPS Pin
Runtime adapter” are rejected.

In `@src/lib/inference/https-pin-runtime-adapter.test.ts`:
- Around line 385-423: Update the “adapter recovery lock (`#6141`)” tests to use
an isolated temporary lock path rather than __test.LOCK_PATH, ensuring all
tryAcquireAdapterLock and withAdapterLock calls target that per-test path.
Create and clean up the temporary path per test without deleting the real
~/.nemoclaw lock, while preserving the existing acquisition and serialization
assertions.

---

Outside diff comments:
In `@src/lib/inference/https-pin-runtime-adapter.ts`:
- Around line 731-792: Update ensureAdapterProcessLocked and the stale-adapter
cleanup flow so the previously running adapter has exited before
spawnDetachedNodeAdapter binds HTTPS_PIN_RUNTIME_ADAPTER_PORT. After
killStaleAdapter, poll for the old process to terminate with a bounded timeout,
or implement an equivalent bounded retry for binding, while preserving the
existing startup failure and cleanup behavior.
🪄 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: 27a891a7-1f5b-4291-8149-680fd539f4a4

📥 Commits

Reviewing files that changed from the base of the PR and between 7813a16 and 2ee9783.

📒 Files selected for processing (10)
  • src/lib/actions/inference-set-endpoint-security.test.ts
  • src/lib/actions/inference-set-gateway-route-containment.test.ts
  • src/lib/actions/inference-set-https-pin-runtime.test.ts
  • src/lib/actions/inference-set-route-containment.ts
  • src/lib/inference/https-pin-runtime-adapter-forward.test.ts
  • src/lib/inference/https-pin-runtime-adapter-forward.ts
  • src/lib/inference/https-pin-runtime-adapter.test.ts
  • src/lib/inference/https-pin-runtime-adapter.ts
  • src/lib/sandbox/config.ts
  • test/e2e/live/inference-routing.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
  • test/e2e/live/inference-routing.test.ts
  • src/lib/actions/inference-set-https-pin-runtime.test.ts
  • src/lib/actions/inference-set-route-containment.ts
  • src/lib/inference/https-pin-runtime-adapter-forward.ts
  • src/lib/actions/inference-set-gateway-route-containment.test.ts

Comment thread src/lib/actions/inference-set-endpoint-security.test.ts Outdated
Comment thread src/lib/inference/https-pin-runtime-adapter.test.ts
…time adapter

The detached, credential-bearing HTTPS Pin Runtime adapter process was
spawned with the generic buildSubprocessEnv allowlist, forwarding
TOOLCHAIN (DOCKER_HOST, KUBECONFIG, SSH_AUTH_SOCK) and PROXY variables
it has no use for. Add buildMinimalCredentialAdapterEnv, scoped to
just the Node runtime names and TLS trust variables the adapter's own
outbound HTTPS connections need, and use it for this adapter's spawn.
Satisfies NVIDIA#6141 acceptance criterion 6 (minimal helper environment).

Signed-off-by: DisturbedSage <maruteymani31@gmail.com>
…NS-rebind resistance (NVIDIA#6141)

Two PR Review Advisor findings against the HTTPS Pin Runtime adapter:

- The adapter's control port binds 0.0.0.0 to stay reachable from the
  sandbox's Docker bridge network, but every route shares one bearer
  token. A peer reaching the port from outside that intended boundary
  could replay the token against a real, already-registered route.
  Add isPrivateNetworkRemoteAddress and gate /route/:id forwarding on
  it (loopback, RFC1918, and IPv6 unique-local/link-local), without
  changing the 0.0.0.0 bind itself.
- TC-INF-11 proved the pinned adapter reaches the real upstream once,
  at registration time, but never proved it resists a DNS record
  change afterward -- the exact SSRF/DNS-rebinding class the pinning
  mechanism exists to close. Extend it to remap the tunnel hostname to
  an unreachable RFC 5737 address after inference set succeeds, then
  repeat the chat completion and assert it still reaches the original
  target via the adapter's already-pinned addresses.

Signed-off-by: DisturbedSage <maruteymani31@gmail.com>
…VIDIA#6141)

Run the adapter-lock unit tests against a temp HOME instead of the
real ~/.nemoclaw so they cannot steal or wedge a concurrently-running
adapter's lock file. Also make the negative-lookahead regex in the
config-validator hint test case-insensitive.

Signed-off-by: DisturbedSage <maruteymani31@gmail.com>
…fter restart (NVIDIA#6141)

Adapter respawn only re-seeds the one route that triggered recovery;
every other previously-registered route silently 404s as
route_not_found, indistinguishable from a route that never existed.

Preserve each route's already-persisted, secretless metadata across
respawn (marked orphanedAt instead of discarded) and have the adapter
respond 503 route_needs_recovery for a route it knows was orphaned by
the last restart, so operators get an actionable signal instead of a
silent black hole, without ever persisting plaintext credentials.

Signed-off-by: DisturbedSage <maruteymani31@gmail.com>
@DisturbedSage5840C

Copy link
Copy Markdown
Contributor Author

Addressed the PR Review Advisor Blocker finding (PRA-1: "Restore every advertised pinned route after adapter recovery") in 63f99e2.

Fix: ensureAdapterProcessLocked no longer wipes the persisted route-state file to {} on respawn. It now preserves every previously-registered route's already-persisted, secretless metadata (targetBaseUrl, pinnedAddresses, providerType, credentialHash — never plaintext credentials, per this adapter's existing security tradeoff) across the restart, computed by a new pure computeRespawnState helper. The fresh process is told which route ids it could not recover via a new NEMOCLAW_HTTPS_PIN_RUNTIME_ADAPTER_ORPHANED_ROUTE_IDS env var (ids only, no secrets), and the /route/:id handler now returns a distinct 503 route_needs_recovery (with an actionable "re-run inference set --endpoint-url" message) for a route it knows was orphaned by the last restart, instead of the same 404 route_not_found used for a route that never existed. A route heals back to normal automatically once its owner re-runs inference set for that endpoint.

Test coverage: computeRespawnState pure-function unit tests (orphans every route except the one being bootstrapped; no-op when there's no prior state), plus server-level regression tests proving the 503/404 split and that re-registering an orphaned route id serves it normally again.

Signed-off-by: DisturbedSage <maruteymani31@gmail.com>
@wscurran wscurran added area: inference Inference routing, serving, model selection, or outputs area: networking DNS, proxy, TLS, ports, host aliases, or connectivity area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery feature PR adds or expands user-visible functionality labels Jul 15, 2026
@wscurran

Copy link
Copy Markdown
Contributor

✨ Thanks for the fix. Adding an HTTPS Pin Runtime adapter for DNS-backed custom endpoints expands inference routing capability while preserving TLS SNI and sandbox boundaries. Ready for maintainer review.


Related open issues:

…transport-6141

Signed-off-by: DisturbedSage <maruteymani31@gmail.com>

# Conflicts:
#	src/lib/registry-recovery-seeded-paths.test.ts
@apurvvkumaria apurvvkumaria self-assigned this Jul 18, 2026

@apurvvkumaria apurvvkumaria 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.

[security][blocking] Exact head dcb6c72 uses one sandbox-visible bearer token for every route on the shared HTTPS-pin adapter. The server authenticates before route selection with options.token, and every inference set registers that same NEMOCLAW_HTTPS_PIN_RUNTIME_ADAPTER_TOKEN. A sandbox authorized for route A can therefore send its token to /route/; route IDs are deterministic from gateway/provider/endpoint, and the adapter then injects route B's real upstream credential. The private-source check does not isolate sandboxes from one another.

Please make data-plane credentials route-scoped (while keeping the control-plane credential separate and host-only), bind each credential to exactly one route, and add an adversarial two-route regression proving route A's credential receives 401/404 for route B and never reaches B's upstream. This must also preserve restart/recovery behavior without persisting upstream credentials.

…NVIDIA#6906)

The shared HTTPS Pin Runtime adapter authenticated every route with one
sandbox-visible bearer token, so a sandbox authorized for route A could
replay that token against route B's deterministic route id and reach
route B's real upstream credential. Split the adapter's single token
into two: a host-only control-plane token (new
NEMOCLAW_HTTPS_PIN_RUNTIME_ADAPTER_CONTROL_TOKEN env var) that only
authenticates PUT /control/routes/:id, and a distinct per-route
data-plane token minted fresh on every ensureHttpsPinRuntimeAdapter call
that only authenticates that route's own /route/:id path. Neither token
is persisted to disk; restart/recovery still uses computeRespawnState
to mark prior routes orphaned, now carrying routeToken through the
bootstrap handoff to a freshly spawned process.

Adds an adversarial two-route regression proving route A's token gets
401/404 against route B and never reaches route B's upstream, plus a
control-token-replay-on-data-plane regression.

Signed-off-by: DisturbedSage <maruteymani31@gmail.com>
@DisturbedSage5840C

Copy link
Copy Markdown
Contributor Author

Addressed the blocking security finding (shared bearer token across routes) in 07229a4.

Fix: split the adapter's single bearer token into two distinct credentials. A host-only control-plane token (new NEMOCLAW_HTTPS_PIN_RUNTIME_ADAPTER_CONTROL_TOKEN env var, never given to a sandbox) now authenticates only the loopback-restricted PUT /control/routes/:id call. A distinct per-route data-plane token (routeToken) is minted fresh on every ensureHttpsPinRuntimeAdapter call, stored only in that route's in-memory RouteRuntime entry (never persisted to disk, same treatment as the existing credential-hash-only persistence), and authenticates only that route's own /route/:id path. A sandbox authorized for route A now only ever learns route A's token, and that token cannot authenticate against route B's data-plane path — route ids remain deterministic/guessable, but guessing one no longer helps without also holding that specific route's own credential.

This required reordering dispatch: network-origin checks (loopback for control-plane, private-network for data-plane) now run first, then path-specific auth (control token for /control/routes/:id; that route's own token for /route/:id, looked up only after the route is confirmed to exist). One accepted, narrow side effect: an unauthenticated request against a nonexistent route now returns a bare 404 (no token to compare against yet), so a private-network peer can learn route-existence/orphan-status without a credential — bounded by the pre-existing trusted sandbox-to-host network boundary, and never leaks a credential.

Restart/recovery is unaffected in shape: computeRespawnState still marks every route but the one being bootstrapped as orphaned on a fresh adapter process, and the bootstrap handoff to the freshly spawned child now also carries the actively-registering route's routeToken (still never the control token, still never persisted).

Test coverage: rewrote the token-handling tests for the two-credential model, and added the adversarial regression this finding asked for — two routes registered with distinct tokens, route A's own token succeeds against route A, route A's token replayed against route B returns 401/404 and route B's upstream receives zero requests, and route B's own token still succeeds against route B. Also added a regression proving the control-plane token itself is rejected if replayed against a route's data-plane path.

@DisturbedSage5840C

Copy link
Copy Markdown
Contributor Author

The required E2E / PR Gate check is failing because its companion job, E2E / PR Gate Coordination, has been stuck at pending with zero steps executed since 2026-07-19T14:50:07Z — about 19 hours now. This is the lightweight job that records the credentialed-E2E skip for fork PRs; it never started, which is why the gate controller times out waiting on it.

I don't have admin rights on this repo to re-run it (gh run rerun returns must have admin rights to Repository). Could someone with access re-run that job? Everything else on the PR (CodeRabbit, require-maintainer-edits, codebase-growth-guardrails) is passing, and the security fix from 07229a41c is otherwise unblocked pending re-review.

cc @apurvvkumaria for visibility.

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: networking DNS, proxy, TLS, ports, host aliases, or connectivity area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery feature PR adds or expands user-visible functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Design runtime-aware HTTPS pinning transport for DNS-backed inference endpoints

4 participants