fix(onboard): ignore stale provider recovery on fresh - #5829
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR threads ChangesFresh provider recovery
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/machine/handlers/provider-inference.ts`:
- Line 180: The handler in provider-inference is not enforcing the `fresh`
contract on resume paths, so completed sessions can still reuse stale
provider/model or inference state. Update `resumeProviderSelection`,
`resumeInference`, and the related `setupNim` flow to use a single effective
resume flag derived from `fresh` (and any recorded-reader/session state) so that
when `fresh` is true, all resume shortcuts are gated off and setup runs instead.
🪄 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: 2a25eb86-f332-41cf-bb38-bc5b9328a9e6
📒 Files selected for processing (6)
src/lib/onboard.tssrc/lib/onboard/machine/core-flow-phases.test.tssrc/lib/onboard/machine/core-flow-phases.tssrc/lib/onboard/machine/handlers/provider-inference.test.tssrc/lib/onboard/machine/handlers/provider-inference.tstest/onboard-resume-provider-recovery.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Selective E2E Results — ✅ All requested jobs passedRun: 28207998174
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Selective E2E Results —
|
| Job | Result |
|---|---|
| onboard-resume-e2e |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/onboard/machine/handlers/provider-inference.ts (1)
210-215: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
freshstill does not gate the resume shortcuts.
freshonly flipsallowRecordedProviderRecoveryinside thesetupNimbranch. When this handler is invoked withfresh: truealongside aresume-eligible completed session,resumeProviderSelection(Line 210) andresumeInference(Line 298) still short-circuit setup and reuse the stale recorded provider/model, which contradicts the--freshcontract. Derive a single effective resume flag (resume && !fresh) and use it in both gates.Proposed fix
+ const canResumeRecordedState = resume && !fresh; while (true) { let forceInferenceSetup = false; const resumeProviderSelection = !forceProviderSelection && - resume && + canResumeRecordedState && session?.steps?.provider_selection?.status === "complete" && typeof provider === "string" && typeof model === "string";const resumeInference = !needsBedrockRuntimeAdapter && !forceProviderSelection && !forceInferenceSetup && - resume && + canResumeRecordedState && deps.isInferenceRouteReady(provider, model);🤖 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/onboard/machine/handlers/provider-inference.ts` around lines 210 - 215, The resume shortcuts in provider-inference still ignore the fresh flag, so a fresh run can reuse recorded provider/model state from a completed session. Introduce a single effective resume condition such as resume && !fresh in provider-inference and apply it consistently to both resumeProviderSelection and resumeInference, alongside the existing forceProviderSelection/session status/type checks, so fresh blocks all resume-based short-circuiting.
🤖 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.ts`:
- Around line 3976-3979: The requested-provider selection in onboard.ts is
forcing `--fresh` interactive runs into the `build` provider path, which
bypasses `promptForInferenceProviderSelection`. Update the logic around
`requestedProvider` and `requestedModel` so the `"build"` fallback only applies
for non-interactive or explicit recovery flows, and let interactive fresh
onboarding fall through to the provider prompt instead. Use the existing
`getNonInteractiveProvider`, `isNonInteractive`, `recoverProvider`, and
`promptForInferenceProviderSelection` flow to keep the decision localized.
---
Duplicate comments:
In `@src/lib/onboard/machine/handlers/provider-inference.ts`:
- Around line 210-215: The resume shortcuts in provider-inference still ignore
the fresh flag, so a fresh run can reuse recorded provider/model state from a
completed session. Introduce a single effective resume condition such as resume
&& !fresh in provider-inference and apply it consistently to both
resumeProviderSelection and resumeInference, alongside the existing
forceProviderSelection/session status/type checks, so fresh blocks all
resume-based short-circuiting.
🪄 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: 41b916d3-5b97-45ea-a470-d28a9b2c95b9
📒 Files selected for processing (5)
src/lib/onboard.tssrc/lib/onboard/machine/core-flow-phases.test.tssrc/lib/onboard/machine/handlers/provider-inference.test.tssrc/lib/onboard/machine/handlers/provider-inference.tstest/onboard-resume-provider-recovery.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/onboard/machine/core-flow-phases.test.ts
- test/onboard-resume-provider-recovery.test.ts
- src/lib/onboard/machine/handlers/provider-inference.test.ts
Selective E2E Results —
|
| Job | Result |
|---|---|
| onboard-resume-e2e |
Selective E2E Results — ✅ All requested jobs passedRun: 28208503726
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Selective E2E Results —
|
| Job | Result |
|---|---|
| onboard-resume-e2e |
Selective E2E Results — ✅ All requested jobs passedRun: 28209162489
|
## Summary Fixes fresh non-interactive onboarding so stale recorded provider/model recovery is ignored before provider selection. This keeps `--fresh` from auto-selecting an old `ollama-local` provider for a `dcode-station` sandbox when the current environment should choose NVIDIA Endpoints or an explicitly requested provider. ## Related Issue Fixes NVIDIA#5719 ## Changes - Thread `fresh` from the core onboarding flow into provider inference setup. - Add a `setupNim` recovery policy option and use inert recovery readers when fresh onboarding disables recorded provider recovery. - Add regression coverage for the handler, core flow boundary, and the stale-session `setupNim` behavior. - Docs pass completed: no docs changes needed because existing docs already describe `--fresh` as discarding saved provider/model selection. ## 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) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: existing docs already state `--fresh` starts over and discards saved provider/model selection; this PR fixes implementation drift. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: self-review focused on preserving non-fresh recovery behavior while disabling stale recovery only for fresh provider selection; targeted tests cover both the flow boundary and setupNim behavior. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `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) Verification run locally: - `npm run build:cli` - `npm test -- --run src/lib/onboard/machine/handlers/provider-inference.test.ts src/lib/onboard/machine/core-flow-phases.test.ts test/onboard-resume-provider-recovery.test.ts` - `npm run typecheck:cli` - `git diff --check` - `npm run source-shape:check` - `SKIP=test-cli npx prek run --files src/lib/onboard.ts src/lib/onboard/machine/core-flow-phases.test.ts src/lib/onboard/machine/core-flow-phases.ts src/lib/onboard/machine/handlers/provider-inference.test.ts src/lib/onboard/machine/handlers/provider-inference.ts test/onboard-resume-provider-recovery.test.ts` - Commit/push hooks passed with `SKIP=test-cli`; the targeted CLI tests above were run directly. --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a “fresh” onboarding option and a configurable provider recovery flag that improves deterministic provider/model selection. * **Bug Fixes** * Updated provider inference/resume behavior so recorded provider recovery is skipped for fresh runs, and recovery-based provider selection is disabled when recovery is turned off. * **Tests** * Added and updated unit/integration coverage to confirm correct `setupNim` recovery-policy arguments, resume-vs-fresh routing, and expected non-interactive vs interactive prompts/logging. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
wscurran
added
area: onboarding
2 participants
Summary
Fixes fresh non-interactive onboarding so stale recorded provider/model recovery is ignored before provider selection. This keeps
--freshfrom auto-selecting an oldollama-localprovider for adcode-stationsandbox when the current environment should choose NVIDIA Endpoints or an explicitly requested provider.Related Issue
Fixes #5719
Changes
freshfrom the core onboarding flow into provider inference setup.setupNimrecovery policy option and use inert recovery readers when fresh onboarding disables recorded provider recovery.setupNimbehavior.--freshas discarding saved provider/model selection.Type of Change
Quality Gates
--freshstarts over and discards saved provider/model selection; this PR fixes implementation drift.Verification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpassesnpm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Verification run locally:
npm run build:clinpm test -- --run src/lib/onboard/machine/handlers/provider-inference.test.ts src/lib/onboard/machine/core-flow-phases.test.ts test/onboard-resume-provider-recovery.test.tsnpm run typecheck:cligit diff --checknpm run source-shape:checkSKIP=test-cli npx prek run --files src/lib/onboard.ts src/lib/onboard/machine/core-flow-phases.test.ts src/lib/onboard/machine/core-flow-phases.ts src/lib/onboard/machine/handlers/provider-inference.test.ts src/lib/onboard/machine/handlers/provider-inference.ts test/onboard-resume-provider-recovery.test.tsSKIP=test-cli; the targeted CLI tests above were run directly.Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests
setupNimrecovery-policy arguments, resume-vs-fresh routing, and expected non-interactive vs interactive prompts/logging.