Skip to content

refactor(onboard): type provider and sandbox phase updates#5562

Merged
cv merged 2 commits into
mainfrom
refactor/onboard-phase-refined-handlers
Jun 20, 2026
Merged

refactor(onboard): type provider and sandbox phase updates#5562
cv merged 2 commits into
mainfrom
refactor/onboard-phase-refined-handlers

Conversation

@cv

@cv cv commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

Continue the #5518 onboarding nullability series by making the provider/sandbox phase adapter consume typed context updates. Provider inference handlers must now return a provider/model-selected update, and sandbox handlers must return a sandbox-created update, so the generic phase sequence no longer accepts arbitrary partial context bags for these lifecycle boundaries.

Related Issue

Refs #5518

Changes

  • Add assertProviderModelSelectedContext for provider/model-only refinement before sandbox-specific requirements are needed.
  • Update createProviderInferencePhase and createSandboxPhase to use the refined context update types and merge helpers.
  • Adapt buildOnboardFlowPhaseSequence to validate and project handler results into the typed phase updates.
  • Update provider/sandbox phase and flow-sequence tests for the stricter phase contracts.

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

  • Git hooks passed during commit and push, or npx prek run --from-ref main --to-ref HEAD passes
  • Targeted tests pass for changed behavior
  • Full npm test passes (broad runtime changes only)
  • 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

Summary by CodeRabbit

  • Tests

    • Added coverage for onboarding flow context validation, ensuring provider/model selection and sandbox GPU selection prerequisites are enforced, with clear error messages.
    • Expanded phase-sequence expectations to verify required fields are produced or rejected appropriately.
  • Refactor

    • Split onboarding context validation into specialized checks for provider/model selection and sandbox setup.
    • Improved phase wiring to validate and reshape phase contexts before downstream execution.
    • Updated phase handler typing and context merging to use more targeted helpers.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv cv self-assigned this Jun 20, 2026
@coderabbitai

coderabbitai Bot commented Jun 20, 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: 17b94539-b415-428e-a986-81465bd04b32

📥 Commits

Reviewing files that changed from the base of the PR and between 0acdc22 and a8ca150.

📒 Files selected for processing (2)
  • src/lib/onboard/machine/flow-phases/provider-sandbox.test.ts
  • src/lib/onboard/machine/flow-sequence.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/lib/onboard/machine/flow-phases/provider-sandbox.test.ts

📝 Walkthrough

Walkthrough

Extracts a new assertProviderModelSelectedContext helper from assertProviderSelectedContext in flow-context.ts. Updates provider-sandbox.ts phase wiring to use specialized context update types and typed merge helpers. Wraps providerInference and sandbox handlers in flow-sequence.ts with async functions that assert required context fields before returning shaped downstream context objects.

Changes

Onboarding context assertion & phase wiring

Layer / File(s) Summary
New assertProviderModelSelectedContext helper and refactored assertProviderSelectedContext
src/lib/onboard/machine/flow-context.ts, src/lib/onboard/machine/flow-context.test.ts
Adds the exported assertProviderModelSelectedContext to validate context.model and context.provider, and refactors assertProviderSelectedContext to delegate to it before checking sandboxGpuConfig. Tests cover both the passing and throwing cases of the new helper.
Provider-sandbox phase: specialized types, mergers, and prerequisite assertion
src/lib/onboard/machine/flow-phases/provider-sandbox.ts, src/lib/onboard/machine/flow-phases/provider-sandbox.test.ts
Replaces generic Partial<Context> shapes with ProviderModelSelectedContextUpdate and SandboxCreatedContextUpdate, narrows SandboxPhaseHandler input to ProviderModelSelectedOnboardFlowContext, swaps mergeOnboardFlowContext calls for typed mergers, and adds assertProviderSelectedContext guard before runSandbox. Test harness gains a patchable context builder with expanded mock fields and a new test case for sandbox rejection when sandboxGpuConfig is missing.
flow-sequence async wrappers with assertion guards
src/lib/onboard/machine/flow-sequence.ts, src/lib/onboard/machine/flow-sequence.test.ts
Imports assertProviderModelSelectedContext and assertSandboxCreatedContext, and replaces direct handler pass-through with async wrappers that assert required context fields on handler output and return shaped context objects with only downstream-needed fields. Test helpers are updated to support context patching, and new validation tests assert rejection when required fields are omitted.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • NVIDIA/NemoClaw#5559: Overlaps directly with sandbox-phase guard usage and flow-context assertion helpers that this PR refactors.
  • NVIDIA/NemoClaw#5561: Shares the same mergeProviderModelSelectedContext and mergeSandboxCreatedContext context refinement work that this PR builds on.

Suggested labels

refactor, area: onboarding, v0.0.64

Poem

A bunny split the assertion in two,
So model and provider each get their due.
The sandbox now checks before it will run,
Typed mergers replace the generic one.
🐇✨ Clean context, the flow carries on!

🚥 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): type provider and sandbox phase updates' accurately captures the main change: introducing stricter typing for provider and sandbox phase context updates throughout the onboarding flow.
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 refactor/onboard-phase-refined-handlers

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

@github-code-quality

github-code-quality Bot commented Jun 20, 2026

Copy link
Copy Markdown

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the branch is 96%. Coverage data for the branch is not yet available.

Show a code coverage summary of the most covered files.
File a8ca150 +/-
nemoclaw/src/se...cret-scanner.ts 100%
nemoclaw/src/commands/slash.ts 100%
nemoclaw/src/li...bprocess-env.ts 100%
nemoclaw/src/bl...eprint/state.ts 98%
nemoclaw/src/onboard/config.ts 98%
nemoclaw/src/bl...int/snapshot.ts 97%
nemoclaw/src/bl...print/runner.ts 95%
nemoclaw/src/co...ration-state.ts 94%
nemoclaw/src/bl...ate-networks.ts 94%
nemoclaw/src/index.ts 94%

TypeScript / code-coverage/cli

The overall coverage in the branch is 46%. Coverage data for the branch is not yet available.

Show a code coverage summary of the most covered files.
File a8ca150 +/-
src/lib/state/o...oard-session.ts 90%
src/lib/inference/local.ts 76%
src/lib/sandbox/config.ts 72%
src/lib/actions...dbox/rebuild.ts 67%
src/lib/onboard/preflight.ts 64%
src/lib/actions...licy-channel.ts 56%
src/lib/state/sandbox.ts 55%
src/lib/policy/index.ts 49%
src/lib/onboard...er-gpu-patch.ts 44%
src/lib/onboard.ts 18%

Updated June 20, 2026 18:11 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

github-actions Bot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: cloud-onboard-vitest, cloud-inference-vitest, inference-routing-vitest
Optional E2E: onboard-negative-paths-vitest, onboard-resume-vitest

Dispatch hint: cloud-onboard-vitest,cloud-inference-vitest,inference-routing-vitest

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • cloud-onboard-vitest (high): Runs the real cloud onboarding path through provider selection, sandbox creation, OpenShell gateway registration, and post-onboard health/security checks. This directly covers the modified provider_selection-to-sandbox flow.
  • cloud-inference-vitest (high): Validates that a real onboarded sandbox can route requests through inference.local using the selected hosted provider/model and credentials, which is the user-visible contract most likely to regress if provider inference context mapping is wrong.
  • inference-routing-vitest (medium): Exercises PR-safe inference routing failure classification and cleanup paths around provider selection and endpoint/credential handling, complementing the live hosted inference check.

Optional E2E

  • onboard-negative-paths-vitest (low): Useful adjacent coverage for user-facing onboarding failure behavior now that the flow adds stricter incomplete-state assertions, but the changed contract is primarily covered by unit tests and normal onboarding E2E.
  • onboard-resume-vitest (high): Optional confidence for persisted onboard session continuity across phase boundaries; this PR changes context/session propagation but does not directly modify resume logic.

New E2E recommendations

  • None.

Dispatch hint

  • Workflow: .github/workflows/e2e-vitest-scenarios.yaml
  • jobs input: cloud-onboard-vitest,cloud-inference-vitest,inference-routing-vitest

@github-actions

github-actions Bot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Recommendation

Required Vitest E2E scenarios: ubuntu-repo-cloud-openclaw
Optional Vitest E2E scenarios: ubuntu-repo-docker-post-reboot-recovery

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: Core onboarding flow context and provider/sandbox phase sequencing changed. The baseline live-supported Ubuntu cloud OpenClaw scenario exercises provider/model selection, sandbox creation, and post-onboard smoke/inference/credential validation through the Vitest registry driver.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw

Optional Vitest E2E scenarios

  • ubuntu-repo-docker-post-reboot-recovery: Optional adjacent lifecycle coverage on the same live-supported cloud OpenClaw onboarding path, useful if reviewers want extra confidence that the changed context narrowing still preserves post-onboard recovery invariants.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-docker-post-reboot-recovery

Relevant changed files

  • src/lib/onboard/machine/flow-context.test.ts
  • src/lib/onboard/machine/flow-context.ts
  • src/lib/onboard/machine/flow-phases/provider-sandbox.test.ts
  • src/lib/onboard/machine/flow-phases/provider-sandbox.ts
  • src/lib/onboard/machine/flow-sequence.test.ts
  • src/lib/onboard/machine/flow-sequence.ts

@github-actions

github-actions Bot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor

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

Consider writing more tests for
  • **Runtime validation** — runOnboardSequenceWithRunner stops before sandbox when providerInference returns a context missing provider or model. The changed surface is a runtime/sandbox lifecycle boundary. The PR now includes targeted helper, phase, and sequence negative tests that cover the prior review concern; broader runner-level guard tests would add confidence but are not required for the observed diff.
  • **Runtime validation** — runOnboardSequenceWithRunner stops before openclaw and policies when sandbox returns a context missing sandboxName. The changed surface is a runtime/sandbox lifecycle boundary. The PR now includes targeted helper, phase, and sequence negative tests that cover the prior review concern; broader runner-level guard tests would add confidence but are not required for the observed diff.

Workflow run details

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

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv cv added the v0.0.66 Release target label Jun 20, 2026
@cv

This comment was marked as outdated.

@cv

cv commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator Author

Correction to my previous comment: shell interpolation stripped the inline code spans.

Addressed the PR Review Advisor nice idea by adding negative-path coverage for the refined phase guard boundaries:

  • createSandboxPhase.run now has a test proving missing sandboxGpuConfig rejects before runSandbox is called.
  • buildOnboardFlowPhaseSequence now has tests proving incomplete provider inference results and sandbox results fail at the wrapper assertions.

@cv cv merged commit fec9e0a into main Jun 20, 2026
44 checks passed
@cv cv deleted the refactor/onboard-phase-refined-handlers branch June 20, 2026 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v0.0.66 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant