Skip to content

refactor(onboard): extract provider menu builder#5165

Merged
cv merged 3 commits into
mainfrom
codex/onboard-provider-menu-flow
Jun 11, 2026
Merged

refactor(onboard): extract provider menu builder#5165
cv merged 3 commits into
mainfrom
codex/onboard-provider-menu-flow

Conversation

@cv

@cv cv commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

Extracts onboarding inference-provider menu composition from src/lib/onboard.ts into a focused provider-menu helper. This keeps setupNim responsible for live detection and provider dispatch while making provider menu ordering and agent-specific entries unit-testable.

Related Issue

Refs #3802

Changes

  • Adds src/lib/onboard/provider-menu.ts to build the inference provider option list from already-detected host/provider state.
  • Adds src/lib/onboard/provider-menu.test.ts coverage for base ordering, local provider entries, Model Router placement, Hermes Provider availability, and Windows-host Ollama menu behavior.
  • Replaces the inline provider menu assembly block in src/lib/onboard.ts with buildInferenceProviderMenu(...) without changing provider-selection branches.
  • Narrows the test-onboard-inference-smoke.sh root-registry stub so messaging hook registry imports keep their real MessagingHookRegistry class.

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

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

Local verification run:

  • npx @biomejs/biome format --write src/lib/onboard.ts src/lib/onboard/provider-menu.ts src/lib/onboard/provider-menu.test.ts
  • npx @biomejs/biome lint src/lib/onboard.ts src/lib/onboard/provider-menu.ts src/lib/onboard/provider-menu.test.ts
  • npm run build:cli
  • npx vitest run src/lib/onboard/provider-menu.test.ts src/lib/onboard/vllm-menu.test.ts src/lib/onboard/ollama-install-menu.test.ts
  • npm run typecheck:cli
  • npx vitest run test/onboard.test.ts
  • npx prek run shfmt shellcheck --files test/e2e/test-onboard-inference-smoke.sh
  • bash test/e2e/test-onboard-inference-smoke.sh
  • git diff --check

Local caveat: npx prek run --all-files and npm test -- --exclude test/release-latest-tag.test.ts both reached test/release-latest-tag.test.ts and failed there because fixture commits inherit the local missing SSH signing key (No private key found for public key /home/cvillela/.ssh/git-signing-key.pub). No release-tag test files were changed.


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

Summary by CodeRabbit

  • Refactor
    • Improved internal code organization for inference provider menu construction, consolidating logic into a centralized module for better maintainability and testability.

@cv cv added refactor PR restructures code without intended behavior change area: cli Command line interface, flags, terminal UX, or output area: architecture Architecture, design debt, major refactors, or maintainability labels Jun 10, 2026
@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

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 0aaabc74-3360-4cf5-9b14-b6e56af54c69

📥 Commits

Reviewing files that changed from the base of the PR and between 1b99e5d and f052689.

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

📝 Walkthrough

Walkthrough

This PR extracts inference provider menu construction from inline logic in setupNim into a centralized, reusable buildInferenceProviderMenu function. The new module defines type contracts, implements ordered option assembly with conditional provider detection, and includes test coverage for menu composition and Windows Ollama entry handling.

Changes

Inference Provider Menu Centralization

Layer / File(s) Summary
Provider Menu Contract and Core Implementation
src/lib/onboard/provider-menu.ts
Exports ProviderMenuChoice, BuildInferenceProviderMenuInput, and InferenceProviderMenu types; implements buildInferenceProviderMenu to assemble ordered menu options starting from base remote providers, applying label overrides, and conditionally appending running Ollama, experimental NIM, vLLM, Windows Ollama start/install, routed, and agent-provider entries.
Provider Menu Tests
src/lib/onboard/provider-menu.test.ts
Vitest suite verifying base option ordering, conditional inclusion of local/routed/agent-scoped providers, and Windows-host Ollama start vs install behavior under WSL conditions.
Integration with setupNim Workflow
src/lib/onboard.ts
Replaces manual menu construction with buildInferenceProviderMenu call; updates imports to add resolveOllamaInstallMenuEntry and remove resolveRunningOllamaMenuEntry; adopts shared ProviderMenuChoice type from the new provider-menu module.

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A menu once scattered, now tidy and clear,
Options assembled with logic sincere,
Windows and Ollama, with tests standing tall,
The provider choice refactored for all!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% 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
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'refactor(onboard): extract provider menu builder' accurately describes the main change: extracting the provider menu construction logic from src/lib/onboard.ts into a dedicated helper module src/lib/onboard/provider-menu.ts.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/onboard-provider-menu-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

PR Review Advisor

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

Consider writing more tests for
  • **Runtime validation** — Exercise an interactive or dry onboarding provider prompt and verify option keys/order for a representative detected-host matrix match the pre-refactor sequence.. The pure helper has useful unit coverage, but its output feeds setupNim's onboarding host detection, non-interactive provider selection, provider recovery, and security-sensitive provider dispatch. Runtime or integration validation would better catch caller/helper contract drift after the extraction.
  • **Runtime validation** — Exercise non-interactive NEMOCLAW_PROVIDER=ollama and verify the helper-produced running Ollama option reaches the existing Ollama validation branch.. The pure helper has useful unit coverage, but its output feeds setupNim's onboarding host detection, non-interactive provider selection, provider recovery, and security-sensitive provider dispatch. Runtime or integration validation would better catch caller/helper contract drift after the extraction.
  • **Runtime validation** — Exercise non-interactive NEMOCLAW_PROVIDER=install-ollama and NEMOCLAW_PROVIDER=start-windows-ollama and verify availability decisions still match the pre-refactor conditions.. The pure helper has useful unit coverage, but its output feeds setupNim's onboarding host detection, non-interactive provider selection, provider recovery, and security-sensitive provider dispatch. Runtime or integration validation would better catch caller/helper contract drift after the extraction.
  • **Runtime validation** — Exercise Hermes Agent onboarding with hermesProvider present, and normal agent onboarding with NEMOCLAW_PROVIDER=hermesProvider rejected by the Hermes-only diagnostic.. The pure helper has useful unit coverage, but its output feeds setupNim's onboarding host detection, non-interactive provider selection, provider recovery, and security-sensitive provider dispatch. Runtime or integration validation would better catch caller/helper contract drift after the extraction.
  • **Runtime validation** — Exercise routed blueprint enabled versus disabled and verify only the routed menu entry changes, without altering provider dispatch.. The pure helper has useful unit coverage, but its output feeds setupNim's onboarding host detection, non-interactive provider selection, provider recovery, and security-sensitive provider dispatch. Runtime or integration validation would better catch caller/helper contract drift after the extraction.
  • **Acceptance clause:** Refs Umbrella: refactor onboarding into a serializable FSM #3802 — add test evidence or identify existing coverage. The PR body references Umbrella: refactor onboarding into a serializable FSM #3802, but deterministic linkedIssues was empty and no trusted issue body/comments were provided, so no literal issue acceptance clauses were available to map to the diff.

Workflow run details

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

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: cloud-onboard-e2e, inference-routing-e2e, onboard-negative-paths-e2e, model-router-provider-routed-inference-e2e
Optional E2E: gpu-e2e, wsl-e2e

Dispatch hint: cloud-onboard-e2e,inference-routing-e2e,onboard-negative-paths-e2e

Auto-dispatched E2E: cloud-onboard-e2e, inference-routing-e2e, onboard-negative-paths-e2e via nightly-e2e.yaml at f05268931508ad8a7ea6042eba93cb4fff13cbd5nightly run

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • cloud-onboard-e2e (medium): Validates the primary non-interactive cloud onboarding path after the provider menu refactor, including install/onboard, sandbox health, security checks, and inference.local.
  • inference-routing-e2e (medium): Exercises non-interactive provider selection and inference routing for NVIDIA/default, custom OpenAI-compatible, OpenAI, and Anthropic paths, plus credential/transport error classification. This directly covers provider option availability changes.
  • onboard-negative-paths-e2e (medium): Covers onboard edge cases including NEMOCLAW_PROVIDER/NEMOCLAW_MODEL handling and provider-aware credential validation; useful as a merge-blocking guard because the changed code controls provider availability during onboard.
  • model-router-provider-routed-inference-e2e (medium): The routed provider option was moved into the new menu builder. This regression test proves routed onboard still creates a provider that answers through inference.local.

Optional E2E

  • gpu-e2e (high): Provides live local Ollama coverage for the provider menu’s Ollama option and local inference path. Recommended if GPU E2E capacity is available, but high-cost/gated compared with the required cloud/provider tests.
  • wsl-e2e (high): The helper now owns WSL Windows-host Ollama start/install entries. Existing WSL full E2E does not specifically select Windows-host Ollama, but it can catch broad WSL onboard/build regressions from this refactor.

New E2E recommendations

  • interactive provider menu / local provider matrix (high): Existing E2E coverage does not directly assert the integrated interactive provider menu across Ollama running/not running, Windows-host Ollama install/start, vLLM, Local NIM, routed, and agent-scoped providers. The new unit tests cover the pure helper, but not the real onboard prompts and environment detection boundaries.
    • Suggested test: Add a hermetic provider-menu onboard scenario E2E that stubs/probes local provider state and verifies visible menu choices plus successful non-interactive selection for each provider key.
  • WSL Windows-host Ollama onboarding (medium): The changed code specifically controls WSL Windows-host Ollama install/start menu entries, but the existing WSL E2E runs the full cloud path and may skip Docker-dependent coverage; it does not prove Windows-host Ollama selection works end-to-end.
    • Suggested test: Add a WSL-specific E2E for NEMOCLAW_PROVIDER=start-windows-ollama/install-windows-ollama with mocked or controlled Windows Ollama detection and validation.

Dispatch hint

  • Workflow: .github/workflows/nightly-e2e.yaml
  • jobs input: cloud-onboard-e2e,inference-routing-e2e,onboard-negative-paths-e2e

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Recommendation

Required Vitest E2E scenarios: ubuntu-repo-cloud-openclaw
Optional Vitest E2E scenarios: None

Dispatch required Vitest E2E scenarios:

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

Workflow run

Full Vitest E2E advisor summary

Vitest E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required Vitest E2E scenarios

  • ubuntu-repo-cloud-openclaw: Changes refactor inference provider menu construction used during onboarding. The Ubuntu cloud OpenClaw live scenario is the smallest live-supported Vitest scenario that exercises repo-current Docker onboarding with the NVIDIA/cloud provider path and downstream smoke/inference/credential validation.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw

Optional Vitest E2E scenarios

  • None.

Relevant changed files

  • src/lib/onboard.ts
  • src/lib/onboard/provider-menu.test.ts
  • src/lib/onboard/provider-menu.ts

@github-actions

Copy link
Copy Markdown
Contributor

Brev E2E (full): FAILED on branch codex/onboard-provider-menu-flowSee logs

@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 27300503612
Target ref: 1b99e5d49c69bb10ccc2e3020eafb8681ff206e0
Workflow ref: main
Requested jobs: cloud-onboard-e2e
Summary: 1 passed, 0 failed, 0 skipped

Job Result
cloud-onboard-e2e ✅ success

@cv

cv commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator Author

Validation update for head 1b99e5d49c69bb10ccc2e3020eafb8681ff206e0.

This supersedes the stale Brev E2E failure comment from run https://github.com/NVIDIA/NemoClaw/actions/runs/27299932077. That run was from the pre-fix smoke-harness head and is no longer representative of the PR.

Current state:

Local validation also passed for Biome format/lint on touched files, npm run build:cli, targeted provider/vLLM/Ollama menu tests, npm run typecheck:cli, test/onboard.test.ts, git diff --check, shell formatting/checks for the touched E2E script, and the local onboard inference smoke script.

Remaining GitHub blocker is expected branch protection / human review (reviewDecision=REVIEW_REQUIRED), not a failing check.

@cv

cv commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator Author

Stack update: opened draft PR #5171, refactor(onboard): extract provider host state, on top of this PR.

What moved:

  • Added detectInferenceProviderHostState in src/lib/onboard/provider-host-state.ts.
  • Moved local provider host probes out of setupNim: host Ollama presence/reachability, Windows-host Ollama state/reachability, WSL duplicate-daemon warning, vLLM reachability/profile/image-cache state, Ollama install-menu state, and GPU NIM capability.
  • setupNim now consumes the typed host-state object before building the provider menu; provider dispatch and credential/model selection remain in setupNim for later slices.

Coverage added:

  • src/lib/onboard/provider-host-state.test.ts covers Linux local Ollama/vLLM detection, WSL plus Windows-host Ollama reachability with the duplicate-daemon warning, mirrored-network warning suppression, and no extra Windows-host switch probe when running Ollama already resolves to host.docker.internal.

Local validation on head d1e02e22e:

  • Biome format/lint on touched files passed.
  • npm run build:cli passed.
  • npm run typecheck:cli passed.
  • Targeted Vitest passed: provider-host-state, provider-menu, vllm-menu, ollama-install-menu, and test/onboard.test.ts.
  • git diff --check passed.

Known local environment note: full npx prek run --all-files / full npm test remain blocked by the unrelated local release-tag signing fixture issue. This PR does not touch test/release-latest-tag.test.ts.

…nu-merge-update

# Conflicts:
#	test/e2e/test-onboard-inference-smoke.sh
@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 27329386311
Target ref: f05268931508ad8a7ea6042eba93cb4fff13cbd5
Workflow ref: main
Requested jobs: cloud-onboard-e2e,inference-routing-e2e,onboard-negative-paths-e2e
Summary: 3 passed, 0 failed, 0 skipped

Job Result
cloud-onboard-e2e ✅ success
inference-routing-e2e ✅ success
onboard-negative-paths-e2e ✅ success

@cv cv merged commit 9a7bcd5 into main Jun 11, 2026
34 checks passed
@cv cv deleted the codex/onboard-provider-menu-flow branch June 11, 2026 07:40
@cv cv added the area: onboarding Onboarding FSM, provider setup, sandbox launch, or first-run flow label Jun 11, 2026
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: cli Command line interface, flags, terminal UX, or output area: onboarding Onboarding FSM, provider setup, sandbox launch, or first-run flow refactor PR restructures code without intended behavior change v0.0.64 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants