Skip to content

refactor(onboard): extract docker gateway runtime helpers - #5125

Merged
cv merged 4 commits into
mainfrom
codex/onboard-gateway-helper-flow
Jun 11, 2026
Merged

refactor(onboard): extract docker gateway runtime helpers#5125
cv merged 4 commits into
mainfrom
codex/onboard-gateway-helper-flow

Conversation

@cv

@cv cv commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

Extracts the Docker-driver gateway runtime/process helpers from src/lib/onboard.ts into a focused onboarding module. This keeps the gateway startup orchestration in onboard.ts while moving the state-dir, PID, runtime drift, executable resolution, and port-listener helpers behind an injected helper factory.

Related Issue

Refs #3802

Changes

  • Added src/lib/onboard/docker-driver-gateway-runtime.ts with the Docker-driver gateway runtime helper factory.
  • Replaced the inline runtime/process helper implementations in src/lib/onboard.ts with destructured helpers wired to the existing onboarding dependencies.
  • Preserved existing onboard.ts compatibility exports for gateway environment, runtime drift, PID, and port-listener helpers.
  • Kept launch orchestration, runtime marker writes, and dashboard-forward cleanup in onboard.ts.
  • Added direct tests for the extracted helper factory covering env-configured binary/state resolution, stale PID/marker cleanup, macOS VM-driver child drift, and injected port-listener identity rejection.
  • Documented the OpenShell source boundary and removal condition for the local Docker-driver gateway runtime heuristics.

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)

Verification

Focused local checks passed:

  • npx @biomejs/biome format --write src/lib/onboard.ts src/lib/onboard/docker-driver-gateway-runtime.ts src/lib/onboard/docker-driver-gateway-runtime.test.ts
  • npx @biomejs/biome lint src/lib/onboard.ts src/lib/onboard/docker-driver-gateway-runtime.ts src/lib/onboard/docker-driver-gateway-runtime.test.ts
  • npm run typecheck:cli
  • npm run build:cli
  • git diff --check
  • npx vitest run src/lib/onboard/docker-driver-gateway-runtime.test.ts test/onboard-gateway-runtime.test.ts test/gateway-start-wait.test.ts test/gateway-final-failure-cleanup.test.ts test/gateway-start-failure-integration.test.ts src/lib/onboard/docker-driver-gateway-service.test.ts src/lib/onboard/gateway-binding.test.ts
  • npx vitest run src/lib/onboard/docker-driver-gateway-env.test.ts src/lib/onboard/docker-driver-gateway-launch.test.ts src/lib/onboard/docker-driver-gateway-runtime-marker.test.ts src/lib/onboard/host-gateway-process.test.ts

Remote checks passed:

  • PR CI, CodeQL, macOS E2E, CLI parity, all CLI shards, static checks, build/typecheck, installer integration, plugin tests, commit lint, and DCO.
  • PR Review Advisor: 0 needs attention, 0 worth checking, 0 nice ideas.
  • Regression E2Es: gateway-health-honest-e2e, gateway-drift-preflight-e2e (run).
  • Vitest scenario E2Es: ubuntu-repo-cloud-openclaw, ubuntu-repo-docker-post-reboot-recovery (run).
  • Nightly E2E: concurrent-gateway-ports-e2e passed on rerun (run); the first attempt failed before PR-specific behavior on Docker Hub pull timeouts.

Full local hook/test note: regular commit hooks and regular pre-push hooks currently fail only in test/release-latest-tag.test.ts because this workstation's global git config enables commit signing with /home/cvillela/.ssh/git-signing-key.pub, but the matching private key is unavailable. I did not touch that test or git configuration.

  • npx prek run --all-files passes
  • npm test passes
  • Tests added or updated for new or changed behavior
  • No secrets, API keys, or credentials committed
  • Docs updated for user-facing behavior changes
  • 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: Carlos Villela cvillela@nvidia.com

@cv cv self-assigned this Jun 10, 2026
@copy-pr-bot

copy-pr-bot Bot commented Jun 10, 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 Jun 10, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@cv, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 10 minutes and 7 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: f2203614-a2b4-42c0-9f6a-0ba5807b347c

📥 Commits

Reviewing files that changed from the base of the PR and between 37064e9 and 74e714b.

📒 Files selected for processing (3)
  • src/lib/onboard.ts
  • src/lib/onboard/docker-driver-gateway-runtime.test.ts
  • src/lib/onboard/docker-driver-gateway-runtime.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/onboard-gateway-helper-flow

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

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: ubuntu-repo-cloud-openclaw, ubuntu-repo-docker-post-reboot-recovery
Optional E2E: e2e-branch-validation:full, wsl-e2e

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • ubuntu-repo-cloud-openclaw (medium; ubuntu-latest live Docker scenario with NVIDIA_API_KEY, 45 minute job timeout): Canonical live Ubuntu Docker OpenClaw scenario exercises repo-built CLI onboarding, gateway startup/registration, sandbox creation, credentials, and live inference; this is the primary integration guard for the onboard.ts wiring change.
  • ubuntu-repo-docker-post-reboot-recovery (medium; ubuntu-latest live Docker lifecycle scenario with NVIDIA_API_KEY, 45 minute job timeout): Closest existing live lifecycle scenario for Docker-driver recovery invariants: after onboarding it mutates Docker/container state and verifies status/recovery preserves registry and labeled container state, which is adjacent to stale gateway PID/runtime reuse behavior.

Optional E2E

  • e2e-branch-validation:full (higher; Brev CPU credits, about 10 minutes typical with 90 minute workflow timeout): Useful extra confidence on a clean ephemeral Brev host because the workflow installs from source, runs full onboarding, verifies sandbox behavior, and performs live inference. This overlaps the required canonical scenario but better represents a fresh machine install.
  • wsl-e2e (medium-high; windows-latest workflow, 90 minute timeout, full E2E may skip if Docker is unavailable): Optional platform confidence because WSL executes Linux onboarding/runtime code paths with Docker from a Windows-hosted environment; helpful for changes that inspect process/PID state and Docker gateway lifecycle.

New E2E recommendations

  • docker-driver gateway runtime drift and reuse (high): Existing live scenarios cover normal onboarding and a stopped-container recovery scaffold, but they do not intentionally seed stale gateway PID files, mismatched runtime env/executable drift, stale OpenShell port listeners, or macOS VM-driver child-process drift in a live runtime.
    • Suggested test: Add a targeted live Docker-driver gateway runtime reuse/drift scenario that starts a gateway, writes or mutates the runtime marker/PID state, simulates a stale or foreign port listener where feasible, then verifies NemoClaw fails closed or repairs without deleting the user's sandbox registry/container.

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

E2E Scenario Advisor Recommendation

Required scenario E2E: ubuntu-repo-cloud-openclaw, ubuntu-repo-docker-post-reboot-recovery
Optional scenario E2E: None

Dispatch required scenario E2E:

  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw
  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-docker-post-reboot-recovery

Workflow run

Full scenario advisor summary

E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required scenario E2E

  • ubuntu-repo-cloud-openclaw: Core onboarding now delegates Docker-driver gateway runtime identity, PID, environment, and binary resolution through the extracted runtime helper. The baseline Ubuntu Docker cloud OpenClaw scenario is the smallest live-supported scenario that exercises the normal onboarding gateway/sandbox path affected by this refactor.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw
  • ubuntu-repo-docker-post-reboot-recovery: The changed runtime helper specifically controls Docker-driver gateway reuse, stale PID/port-owner detection, and drift handling, which are central to the live-supported post-reboot recovery scenario’s host-side recovery invariants.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-docker-post-reboot-recovery

Optional scenario E2E

  • None.

Relevant changed files

  • src/lib/onboard.ts
  • src/lib/onboard/docker-driver-gateway-runtime.ts

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor

Findings: 0 needs attention, 1 worth checking, 0 nice ideas
Since last review: 0 prior items resolved, 0 still apply, 1 new item found

Review findings

🛠️ Needs attention

  • None.

🔎 Worth checking

  • Add negative/runtime coverage for Docker-driver gateway process identity (src/lib/onboard/docker-driver-gateway-runtime.ts:330): The refactor adds useful unit coverage around env-configured resolution, stale PID cleanup, macOS VM-driver child drift, and injected listener identity rejection, but the changed surface governs host gateway reuse, PID/process identity, executable/env drift, stale runtime cleanup, and possible process termination. Several fail-closed branches are still only indirectly or partially covered by mocks.
    • Recommendation: Add or identify behavior-specific coverage for Linux `isDockerDriverGatewayProcess` falling back to `ps` while still rejecting missing Docker-driver env, `getDockerDriverGatewayRuntimeDrift` returning `could not verify process executable` when `/proc/<pid>/exe` is unavailable, `resolveOpenShellGatewayBinary` preferring the cached OpenShell sibling before fallback locations, invalid/zero/negative/dead listener PIDs not invoking identity checks, and a runtime/integration validation that a foreign `openshell*` port owner is not adopted or terminated.
    • Evidence: New tests in `src/lib/onboard/docker-driver-gateway-runtime.test.ts` cover several extracted helper paths, and existing `test/onboard-gateway-runtime.test.ts` covers snapshot drift and listener basics. The remaining gaps are in the security-critical host `/proc` and live process-adoption boundaries.

🌱 Nice ideas

  • None.
Consider writing more tests for
  • **Runtime validation** — Add or identify a runtime/integration validation that `isDockerDriverGatewayProcess` falls back from missing or unreadable `/proc/<pid>/cmdline` to captured `ps` args while still requiring Docker-driver env on Linux.. The changed code is mostly a refactor with improved unit tests, but it controls high-risk host-process reuse, PID identity, drift detection, and stale process cleanup. Mocked `/proc` and `ps` behavior cannot fully prove live PID/process ownership and non-termination guarantees.
  • **Runtime validation** — Add or identify a test that `getDockerDriverGatewayRuntimeDrift` reports `could not verify process executable` when Linux `/proc/<pid>/exe` is missing or unreadable.. The changed code is mostly a refactor with improved unit tests, but it controls high-risk host-process reuse, PID identity, drift detection, and stale process cleanup. Mocked `/proc` and `ps` behavior cannot fully prove live PID/process ownership and non-termination guarantees.
  • **Runtime validation** — Add or identify a unit test that `resolveOpenShellGatewayBinary` prefers a sibling `openshell-gateway` next to the cached OpenShell binary before `$HOME/.local/bin` and system fallbacks when no env override is set.. The changed code is mostly a refactor with improved unit tests, but it controls high-risk host-process reuse, PID identity, drift detection, and stale process cleanup. Mocked `/proc` and `ps` behavior cannot fully prove live PID/process ownership and non-termination guarantees.
  • **Runtime validation** — Add or identify a unit test that `getDockerDriverGatewayPortListenerPid` rejects invalid, zero, negative, and dead PIDs without invoking the gateway identity callback.. The changed code is mostly a refactor with improved unit tests, but it controls high-risk host-process reuse, PID identity, drift detection, and stale process cleanup. Mocked `/proc` and `ps` behavior cannot fully prove live PID/process ownership and non-termination guarantees.
  • **Runtime validation** — Add or identify a runtime/integration validation that live Docker-driver gateway reuse/adoption rejects a foreign `openshell*` port owner without terminating unrelated processes.. The changed code is mostly a refactor with improved unit tests, but it controls high-risk host-process reuse, PID identity, drift detection, and stale process cleanup. Mocked `/proc` and `ps` behavior cannot fully prove live PID/process ownership and non-termination guarantees.
  • **Add negative/runtime coverage for Docker-driver gateway process identity** — Add or identify behavior-specific coverage for Linux `isDockerDriverGatewayProcess` falling back to `ps` while still rejecting missing Docker-driver env, `getDockerDriverGatewayRuntimeDrift` returning `could not verify process executable` when `/proc/<pid>/exe` is unavailable, `resolveOpenShellGatewayBinary` preferring the cached OpenShell sibling before fallback locations, invalid/zero/negative/dead listener PIDs not invoking identity checks, and a runtime/integration validation that a foreign `openshell*` port owner is not adopted or terminated.
  • **Acceptance clause:** Refs Umbrella: refactor onboarding into a serializable FSM #3802 — add test evidence or identify existing coverage. The deterministic context reported no linked issue clauses or issue comments for Umbrella: refactor onboarding into a serializable FSM #3802. Local search only found existing unrelated references in onboard machine vocabulary tests, so there is no literal acceptance text available to map to this diff.
Since last review details

Current findings:

  • Add negative/runtime coverage for Docker-driver gateway process identity (src/lib/onboard/docker-driver-gateway-runtime.ts:330): The refactor adds useful unit coverage around env-configured resolution, stale PID cleanup, macOS VM-driver child drift, and injected listener identity rejection, but the changed surface governs host gateway reuse, PID/process identity, executable/env drift, stale runtime cleanup, and possible process termination. Several fail-closed branches are still only indirectly or partially covered by mocks.
    • Recommendation: Add or identify behavior-specific coverage for Linux `isDockerDriverGatewayProcess` falling back to `ps` while still rejecting missing Docker-driver env, `getDockerDriverGatewayRuntimeDrift` returning `could not verify process executable` when `/proc/<pid>/exe` is unavailable, `resolveOpenShellGatewayBinary` preferring the cached OpenShell sibling before fallback locations, invalid/zero/negative/dead listener PIDs not invoking identity checks, and a runtime/integration validation that a foreign `openshell*` port owner is not adopted or terminated.
    • Evidence: New tests in `src/lib/onboard/docker-driver-gateway-runtime.test.ts` cover several extracted helper paths, and existing `test/onboard-gateway-runtime.test.ts` covers snapshot drift and listener basics. The remaining gaps are in the security-critical host `/proc` and live process-adoption boundaries.

Workflow run details

This is an automated advisory review. A human maintainer must make the final merge decision.

@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ❌ Some jobs failed

Run: 27265179682
Target ref: 75f5a52e55a4c4f61c48de1f5003865699d1d1fd
Workflow ref: main
Requested jobs: concurrent-gateway-ports-e2e
Summary: 0 passed, 1 failed, 0 skipped

Job Result
concurrent-gateway-ports-e2e ❌ failure

Failed jobs: concurrent-gateway-ports-e2e. Check run artifacts for logs.

@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 27265179682
Target ref: 75f5a52e55a4c4f61c48de1f5003865699d1d1fd
Workflow ref: main
Requested jobs: concurrent-gateway-ports-e2e
Summary: 1 passed, 0 failed, 0 skipped

Job Result
concurrent-gateway-ports-e2e ✅ success

@wscurran wscurran added area: architecture Architecture, design debt, major refactors, or maintainability area: onboarding Onboarding FSM, provider setup, sandbox launch, or first-run flow refactor PR restructures code without intended behavior change labels Jun 10, 2026
@wscurran

Copy link
Copy Markdown
Contributor


Related open issues:

@cv
cv marked this pull request as ready for review June 10, 2026 19:31
@cv cv added the v0.0.64 label Jun 10, 2026
@cv
cv merged commit 56e5dfd into main Jun 11, 2026
30 checks passed
@cv
cv deleted the codex/onboard-gateway-helper-flow branch June 11, 2026 06:25
@cv cv mentioned this pull request Jun 11, 2026
12 tasks
cv added a commit that referenced this pull request Jun 11, 2026
## Summary
Extracts createSandbox dashboard port and CHAT_UI_URL resolution into
the dashboard-port helper so the precedence chain can be reviewed and
tested outside the full onboarding flow. This draft PR is stacked on
#5125 and keeps Hermes dashboard forwarding orchestration in onboard.ts
for a later slice.

## Related Issue
Refs #3802

## Changes
- Added resolveCreateSandboxDashboardPort to centralize
--control-ui-port, CHAT_UI_URL, registry, agent, and default port
precedence.
- Replaced the inline createSandbox dashboard port/URL block with the
helper while preserving the existing effectivePort and chatUiUrl flow.
- Added focused coverage for precedence, warning emission, schemeless
CHAT_UI_URL normalization, remote host URL rewriting, and malformed
CHAT_UI_URL behavior.

## Type of Change
- [x] 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)

## Verification
Targeted checks run:
- npx @biomejs/biome lint src/lib/onboard.ts
src/lib/onboard/dashboard-port.ts src/lib/onboard/dashboard-port.test.ts
- npm run typecheck:cli
- npm run build:cli
- git diff --check
- npx vitest run src/lib/onboard/dashboard-port.test.ts
test/onboard.test.ts test/onboard-custom-dockerfile.test.ts
test/shellquote-sandbox.test.ts

Required live validation:
- ubuntu-repo-cloud-openclaw passed:
https://github.com/NVIDIA/NemoClaw/actions/runs/27267579584
- concurrent-gateway-ports-e2e passed:
https://github.com/NVIDIA/NemoClaw/actions/runs/27267579798
- double-onboard-e2e passed:
https://github.com/NVIDIA/NemoClaw/actions/runs/27267579798

- [ ] `npx prek run --all-files` passes
- [ ] `npm test` passes
- [x] Tests added or updated for new or changed behavior
- [x] No secrets, API keys, or credentials committed
- [ ] Docs updated for user-facing behavior changes
- [ ] `npm run docs` builds without warnings (doc changes only)
- [ ] 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)

---
Signed-off-by: Carlos Villela <cvillela@nvidia.com>

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

## Summary by CodeRabbit

* **Refactor**
* Refactored internal dashboard port and URL selection logic for
improved maintainability.

* **Tests**
* Added comprehensive tests for dashboard port selection and URL
normalization behavior.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: architecture Architecture, design debt, major refactors, or maintainability area: onboarding Onboarding FSM, provider setup, sandbox launch, or first-run flow refactor PR restructures code without intended behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants