fix(onboard): omit TLS servername for IP-literal gateway health probe hosts

[cdiddy77]

Summary

On Node.js 25 and newer, the Docker-driver gateway health probe throws at http2.connect because it sets the TLS servername option to the probe URL's IP-literal hostname (127.0.0.1), which Node 25 rejects (ERR_INVALID_ARG_VALUE, formerly deprecation DEP0123); the probe's catch reports each poll as not ready, so nemoclaw onboard fails the gateway health wait against a healthy gateway. This change sends servername only for DNS hostnames — for IP endpoints, certificate verification matches the connection IP against the certificate's IP SANs without SNI, so onboarding passes the health wait on every supported Node version.

Related Issue

Fixes #7527

Changes

• src/lib/onboard/gateway-http-readiness.ts: dockerDriverGatewayHttp2ConnectOptions sets servername only when the probe hostname is not an IP literal (net.isIP on the bracket-stripped hostname, covering bracketed IPv6).
• src/lib/onboard/gateway-http-readiness.test.ts: three regression tests — IP-literal host omits servername, bracketed IPv6 host omits servername, DNS hostname keeps servername.

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: the fix restores documented onboarding behavior (the health wait succeeding against a healthy gateway); no page under docs/ describes the probe's TLS internals or the failing behavior, so no page is stale.
• 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:

Documentation Writer Review

• Documentation writer subagent reviewed the completed changes
• Result: no-docs-needed
• Evidence: no documentation paths changed; the reviewer checked docs/ for pages describing the gateway health probe, TLS servername, or Node 25 behavior and found none stale; the dated changelog entry is pre-tag release prep, not a per-PR obligation. Re-run for maintainer commits bd434d8 and 153e375 (test-only teardown change and its revert; head tree is identical to the originally reviewed 464fb0e): pass, no docs impact.
• Agent: Claude Code

DGX Station Hardware Evidence

• Tested on DGX Station
• Tested commit:
• Station profile/scenario:
• Result:
• Supporting evidence:

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: npx vitest run --project cli src/lib/onboard/gateway-http-readiness.test.ts → 5 passed (2 pre-existing, 3 new); npm run typecheck:cli → clean
• 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: Charles Parker charles@parkerfolk.net

[copy-pr-bot]

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]

📝 Walkthrough

Walkthrough

The gateway readiness probe now omits TLS servername for IPv4 and IPv6 literals while preserving it for DNS hostnames. Tests mock HTTP/2 connections and verify all three host formats.

Changes

Gateway TLS servername handling

Layer / File(s)	Summary
IP-aware TLS connection options src/lib/onboard/gateway-http-readiness.ts	HTTP/2 TLS options strip IPv6 brackets, detect IP literals, and set servername only for DNS hostnames.
TLS servername behavior tests src/lib/onboard/gateway-http-readiness.test.ts	Mocks HTTP/2 connections and verifies servername omission for IPv4 and bracketed IPv6 hosts, while retaining it for DNS names.

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

Possibly related issues

• NVIDIA/NemoClaw 7527 — Addresses TLS servername handling for IP-literal gateway hosts.

Suggested labels: area: onboarding

Suggested reviewers: copilot

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 matches the main change: omitting TLS servername for IP-literal gateway health probe hosts.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[Copilot]

Pull request overview

Fixes nemoclaw onboard gateway readiness polling on Node.js 25+ by avoiding an invalid TLS servername (SNI) value when the probe target is an IP-literal host (IPv4/IPv6). This restores expected onboarding behavior against healthy OpenShell gateways across supported Node versions.

Changes:

• Update the Docker-driver HTTP/2 readiness probe to set TLS servername only for DNS hostnames (using net.isIP, including bracketed IPv6 inputs).
• Add regression tests to ensure servername is omitted for IPv4/IPv6 literals and retained for DNS hostnames.

Security review (nemoclaw-maintainer-security-code-review)

• PASS Secrets/Credentials (no secrets introduced; test fixtures write non-sensitive placeholder files)
• PASS Input Validation/Sanitization (tightens handling of URL hostname → TLS option mapping)
• PASS AuthN/AuthZ (no changes)
• PASS Dependencies (no changes)
• PASS Error Handling/Logging (no new logging; same readiness behavior, but avoids deterministic throw path on Node 25)
• PASS Cryptography/Data Protection (continues mTLS; change is SNI behavior only)
• PASS Config/Security Headers (no changes)
• PASS Security Testing (tests added for the sensitive TLS option behavior)
• PASS System Security (does not weaken controls; avoids a Node runtime incompatibility)

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/lib/onboard/gateway-http-readiness.ts	Only sets http2.connect TLS servername for non-IP hostnames to avoid Node 25+ throw on IP-literal SNI.
src/lib/onboard/gateway-http-readiness.test.ts	Adds regression tests validating servername omission/retention behavior across IP-literal vs DNS hosts.

---

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

[github-actions]

PR Review Advisor — Informational

Advisor assessment: Informational / high confidence
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions
Status: No actionable findings remain in the canonical review ledger.

Model lanes

• GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
• Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
• Model comparison: normalized findings match; normalized E2E selections differ; severity counts match.

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

Since last review: 0 prior items resolved · 0 still apply · 0 new items found

E2E guidance

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

Recommended E2E: onboard-repair, onboard-resume, cloud-onboard

Workflow run details

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

[coderabbitai]

Actionable comments posted: 1

🤖 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/gateway-http-readiness.test.ts`:
- Around line 93-146: Update the afterEach cleanup in the
“isDockerDriverGatewayHttpReady TLS servername” suite to call vi.unstubAllEnvs()
and restore the http2.connect spy after each test, while preserving the existing
tlsDirs cleanup. Ensure subsequent tests start with no stubbed environment
values and a restored connect implementation or cleared spy state.

🪄 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: d493312c-7575-4b7c-a0aa-a80f608bb826

📥 Commits

Reviewing files that changed from the base of the PR and between dbac677 and 464fb0e.

📒 Files selected for processing (2)

• src/lib/onboard/gateway-http-readiness.test.ts
• src/lib/onboard/gateway-http-readiness.ts

[Copilot]

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

[apurvvkumaria]

Maintainer review at exact head 153e375df568090d6c60b25a07388eabade26994: the implementation strips IPv6 brackets only for IP classification, omits TLS servername for IPv4/IPv6 literals, and preserves DNS SNI. Focused IPv4, bracketed-IPv6, and DNS tests cover the behavior; repository-wide mock isolation makes explicit per-test restoration redundant. Automated review has no remaining findings. Approved on code quality; merge still requires the full exact-head CI and E2E gate to finish successfully.

[apurvvkumaria]

Exact-head maintainer status for 153e375df568090d6c60b25a07388eabade26994: the security-reviewed fork workflows were approved, the complete CI matrix passed (all eight CLI shards, aggregate coverage, build/typecheck, static checks, installer integration, reviewed npm audit, plugin tests, CodeQL, macOS, CLI/docs parity, DCO, commit lint, and docs receipt), and the code review is approved. The deterministic E2E plan selected onboard-repair and onboard-resume, but no selected E2E job ran and no repository credential was exposed. Controller run 30171622637 failed closed because approve-credentialed-e2e-for-fork-pr has no required-reviewer protection, so GitHub recorded no deployment approval. This is repository environment configuration, not a PR-code failure. After an administrator adds the documented required reviewer, this PR needs one fresh signed SHA and fresh exact-head CI/E2E; I will not retry or waive the same-head gate.