fix(rebuild): preserve compatible provider recovery#6665
Merged
Conversation
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Contributor
|
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)
📝 WalkthroughWalkthroughAuthoritative rebuild validation now requires a preflighted locked resume context. Provider recovery accepts that configuration for incomplete sessions, with inference wiring and tests covering sandbox matching, reservations, messaging revalidation, and live-slice options. ChangesProvider recovery gating
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant rebuildProviderFlowOptions
participant handleProviderInferenceState
participant createRecovery
participant setupNim
rebuildProviderFlowOptions->>handleProviderInferenceState: provide validated authoritative resume configuration
handleProviderInferenceState->>createRecovery: pass authoritativeResumeConfig
createRecovery->>createRecovery: evaluate session sandbox and recovery eligibility
handleProviderInferenceState->>setupNim: configure provider with sessionId
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the branch. TypeScript / code-coverage/cliThe overall coverage in the branch remains at 77%, unchanged from the branch. Show a code coverage summary of the most impacted files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Contributor
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Contributor
PR Review Advisor — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 1 item to resolve/justify, 0 in-scope improvements
|
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 29133396068
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard/machine/handlers/provider-inference-recovery-gating.test.ts (1)
62-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider also asserting the observable return value, not only the
setupNimmock call.The test only verifies
calls.setupNimwas invoked with the recovery flag set totrue; it does not assert anything abouthandleProviderInferenceState's return value (e.g., recovered provider/model/sandboxName). Per path instructions for test files, prefer observable outcomes through the public boundary over mock-call assertions where feasible — a follow-upexpect(result).toMatchObject(...)would make the test more resilient to internal refactors of thesetupNimcall signature while still proving the gate fired.As per path instructions, "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."
🤖 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-recovery-gating.test.ts` around lines 62 - 91, Update the test for handleProviderInferenceState to capture its return value and assert the observable recovered provider, model, and sandboxName with expect(result).toMatchObject(...). Retain the setupNim assertion only if needed to verify the recovery gate, but prioritize the public return value over the mock-call signature.Source: Path instructions
🤖 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.
Nitpick comments:
In `@src/lib/onboard/machine/handlers/provider-inference-recovery-gating.test.ts`:
- Around line 62-91: Update the test for handleProviderInferenceState to capture
its return value and assert the observable recovered provider, model, and
sandboxName with expect(result).toMatchObject(...). Retain the setupNim
assertion only if needed to verify the recovery gate, but prioritize the public
return value over the mock-call signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 27235fa2-0c05-4745-855c-a11e4cdf89f9
📒 Files selected for processing (4)
src/lib/onboard/machine/handlers/provider-inference-recovery-gating.test.tssrc/lib/onboard/machine/handlers/provider-inference-recovery.tssrc/lib/onboard/machine/handlers/provider-inference.test.tssrc/lib/onboard/machine/handlers/provider-inference.ts
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 29133737100
|
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
cv
approved these changes
Jul 11, 2026
cv
enabled auto-merge (squash)
July 11, 2026 01:16
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 29134144534
|
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard/authoritative-rebuild-target.ts (1)
115-131: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGateway binding is resolved before the higher-level preconditions are checked.
resolveAuthoritativeOnboardGatewayBinding(opts)runs as soon asauthoritativeResumeConfigis true, ahead of theresume/recreateSandbox/onboardLockAlreadyHeldchecks in the sameif. If a caller ever passes a partially-populated gateway (name without port, or vice versa) alongsideonboardLockAlreadyHeld: false, the surfaced error becomes the gateway-mismatch message instead of the unified "requires a preflighted locked rebuild resume" message that this whole block is meant to guarantee. Current tests only exercise fully-populated or fully-absent gateway data, so this inconsistency isn't caught.Consider checking the higher-level preconditions first, then resolving the gateway only once those pass, so the error surface stays uniform for any authoritative gate failure.
🤖 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/authoritative-rebuild-target.ts` around lines 115 - 131, Check the authoritative resume preconditions before calling resolveAuthoritativeOnboardGatewayBinding in the authoritativeResumeConfig block. Keep the resume, recreateSandbox, lock, gateway, and target validations grouped so any failed higher-level gate throws the unified preflighted locked rebuild resume error; resolve the gateway only after those prerequisite checks pass, while preserving the existing gateway validation behavior.
🤖 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.
Nitpick comments:
In `@src/lib/onboard/authoritative-rebuild-target.ts`:
- Around line 115-131: Check the authoritative resume preconditions before
calling resolveAuthoritativeOnboardGatewayBinding in the
authoritativeResumeConfig block. Keep the resume, recreateSandbox, lock,
gateway, and target validations grouped so any failed higher-level gate throws
the unified preflighted locked rebuild resume error; resolve the gateway only
after those prerequisite checks pass, while preserving the existing gateway
validation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2c404df4-8e54-411c-a7d3-bcb479605396
📒 Files selected for processing (4)
src/lib/onboard/authoritative-rebuild-target.test.tssrc/lib/onboard/authoritative-rebuild-target.tssrc/lib/onboard/machine/handlers/provider-inference-recovery-gating.test.tssrc/lib/onboard/machine/handlers/provider-inference-recovery.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/onboard/machine/handlers/provider-inference-recovery.ts
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary Preserve the preflighted compatible-endpoint recovery authority while an authoritative rebuild revalidates messaging inference after deleting the old sandbox. This prevents the recreate flow from falling back to NVIDIA Endpoints and demanding an unrelated host key. ## Related Issue Regression follow-up to NVIDIA#6634. ## Changes - Thread authoritative rebuild context into recorded-provider recovery. - Keep ordinary incomplete sessions, fresh sandboxes, mismatched identities, and foreign reservations fail-closed. - Cover both the recovery gate and the two-pass messaging revalidation flow. ## 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: internal rebuild recovery correction; commands and documented behavior are unchanged - [x] 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: pending maintainer review; the change retains NVIDIA#6634's fresh, mismatched, and unauthorized recovery denials - [ ] 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] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run --project cli src/lib/onboard/machine/handlers/provider-inference-recovery-gating.test.ts src/lib/onboard/machine/handlers/provider-inference.test.ts` (44 passed); adjacent recovery/rebuild selection (126 passed) - [ ] Applicable broad gate passed — full CLI project: 8,101 passed; six unrelated local prerequisite/time-budget failures, with timeout cases passing under a realistic budget; CI remains authoritative - [ ] 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) --- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved provider inference recovery for authoritative resume flows, including incomplete onboarding sessions. * Recovery behavior now correctly accounts for sandbox/session context and foreign reservation flags. * Added stronger validation for preflighted locked rebuild resumes, with clearer blocking when required authoritative conditions are missing. * **Tests** * Added new test coverage for authoritative incomplete-session recovery across matching/mismatched sandboxes and reservation scenarios. * Expanded assertions to verify inference setup inputs and recovery gating behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Preserve the preflighted compatible-endpoint recovery authority while an authoritative rebuild revalidates messaging inference after deleting the old sandbox. This prevents the recreate flow from falling back to NVIDIA Endpoints and demanding an unrelated host key.
Related Issue
Regression follow-up to #6634.
Changes
Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project cli src/lib/onboard/machine/handlers/provider-inference-recovery-gating.test.ts src/lib/onboard/machine/handlers/provider-inference.test.ts(44 passed); adjacent recovery/rebuild selection (126 passed)npm run docsbuilds without warnings (doc changes only)Signed-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit