fix(onboard): recreate on fresh model drift#6317
Closed
chengjiew wants to merge 4 commits into
Closed
Conversation
Signed-off-by: Chengjie Wang <chengjiew@nvidia.com>
Contributor
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds provider/model drift detection to sandbox resume handling and recreates sandboxes when the stored provider/model differs from the requested configuration. Recreate flows now preserve state while skipping restoration of ChangesProvider/model drift recreate flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe 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.
TypeScript / code-coverage/cliThe overall coverage in the branch is 71%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered 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 — BlockedMerge posture: Do not merge until addressed Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
![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/sandbox-resume.ts (1)
63-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
canReuseSandbox's provider/model check is now unreachable.
decideSandboxResume(lines 151-152) returns early viaproviderModelDriftDecisionwheneverproviderModelConfigChangedis true, beforecanReuseSandboxis ever invoked. The!signals.providerModelConfigChangedclause added at line 66 can therefore never evaluate tofalsein practice — it's dead defensive code given the current call order.Low priority since it's harmless and could be read as an intentional invariant guard against future reordering.
🤖 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/sandbox-resume.ts` around lines 63 - 75, The provider/model guard in canReuseSandbox is unreachable because decideSandboxResume already exits via providerModelDriftDecision when providerModelConfigChanged is true. Remove the redundant !signals.providerModelConfigChanged check from canReuseSandbox, or if you want to keep it as an invariant guard, make that intent explicit in canReuseSandbox and ensure it matches the current call order in decideSandboxResume.
🤖 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/sandbox-resume.ts`:
- Around line 63-75: The provider/model guard in canReuseSandbox is unreachable
because decideSandboxResume already exits via providerModelDriftDecision when
providerModelConfigChanged is true. Remove the redundant
!signals.providerModelConfigChanged check from canReuseSandbox, or if you want
to keep it as an invariant guard, make that intent explicit in canReuseSandbox
and ensure it matches the current call order in decideSandboxResume.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a3ce2d02-d835-46f1-a10c-8de456c08acc
📒 Files selected for processing (4)
src/lib/onboard/machine/handlers/sandbox-resume.test.tssrc/lib/onboard/machine/handlers/sandbox-resume.tssrc/lib/onboard/machine/handlers/sandbox.test.tssrc/lib/onboard/machine/handlers/sandbox.ts
Signed-off-by: Chengjie Wang <chengjiew@nvidia.com>
Signed-off-by: Chengjie Wang <chengjiew@nvidia.com>
21 tasks
Collaborator
|
Thanks @chengjiew for the original investigation and implementation direction. #6332 supersedes this PR with live DCode identity validation, ownership-aware managed re-onboard restore, post-restore registry gating, and missing-metadata/custom-image coverage. Chengjie is credited as a co-author on the replacement commit. |
Signed-off-by: Chengjie Wang <chengjiew@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/snapshot.test.ts (1)
1396-1396: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse literal
:for PATH, notpath.delimiter.Based on learnings, this test suite prefers the established POSIX separator
:when buildingprocess.env.PATH, since these tests only run on Linux CI runners (Windows is limited to WSL e2e tests elsewhere).♻️ Suggested fix
- process.env.PATH = `${binDir}${path.delimiter}${oldPath || ""}`; + process.env.PATH = `${binDir}:${oldPath || ""}`;🤖 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 `@test/snapshot.test.ts` at line 1396, The PATH setup in this snapshot test is using a platform-dependent separator via path.delimiter, but this suite expects the POSIX literal ":" because it runs on Linux CI. Update the PATH assignment in the test setup to build the string with ":" directly, keeping the existing binDir and oldPath handling intact.Source: Learnings
🤖 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 `@test/snapshot.test.ts`:
- Line 1396: The PATH setup in this snapshot test is using a platform-dependent
separator via path.delimiter, but this suite expects the POSIX literal ":"
because it runs on Linux CI. Update the PATH assignment in the test setup to
build the string with ":" directly, keeping the existing binDir and oldPath
handling intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7e71b935-c758-4c8f-8927-50e40128b9eb
📒 Files selected for processing (15)
docs/get-started/quickstart-langchain-deepagents-code.mdxdocs/reference/commands-nemohermes.mdxdocs/reference/commands.mdxsrc/lib/onboard.tssrc/lib/onboard/machine/handlers/sandbox-drift.test.tssrc/lib/onboard/machine/handlers/sandbox-drift.tssrc/lib/onboard/machine/handlers/sandbox-resume.test.tssrc/lib/onboard/machine/handlers/sandbox-resume.tssrc/lib/onboard/machine/handlers/sandbox-test-fixtures.tssrc/lib/onboard/machine/handlers/sandbox.test.tssrc/lib/onboard/machine/handlers/sandbox.tssrc/lib/onboard/types.tssrc/lib/state/sandbox.tstest/package-contract/cli/config-set-cli-dispatch.test.tstest/snapshot.test.ts
✅ Files skipped from review due to trivial changes (2)
- docs/get-started/quickstart-langchain-deepagents-code.mdx
- docs/reference/commands-nemohermes.mdx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/onboard/machine/handlers/sandbox-resume.test.ts
- src/lib/onboard/machine/handlers/sandbox.test.ts
apurvvkumaria
added a commit
that referenced
this pull request
Jul 6, 2026
## Summary Keep managed Deep Agents Code runtime routing aligned with the provider/model selected during same-name re-onboarding. The fix treats the live `dcode identity` result as authoritative, reconciles mixed-ownership `config.toml` state without restoring stale routing, and publishes registry metadata only after the restored runtime is verified. ## Related Issue Closes #6311 Supersedes #6317. This incorporates and extends @chengjiew's original drift/recreate investigation; Chengjie is credited as a co-author on the commit. ## Changes - Recreate stock managed DCode sandboxes when live route, provider, model, or endpoint identity is stale or unreadable; refuse unverifiable reuse when the registry row is absent. - Restore only four bounded display preferences from backed-up DCode config: three booleans and the enumerated thread sort order. Keep fresh `models`, `update`, and generated provider metadata authoritative, and drop free-form or behavior-bearing backup settings. - Perform the merge with bounded TOML parsing, regular-file checks, same-directory staging, inode revalidation, `fsync`, and atomic replacement. - Validate the live restored selection before writing registry/status metadata, and leave custom-image plus ordinary snapshot/rebuild restore behavior unchanged. - Keep generic hotspots bounded by extracting gateway failure handling, DCode resume policy, and caller-authorized state-file restore policy into focused modules. - Add unit, handler, restore-boundary, and orchestration regression coverage plus user-facing documentation. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [x] 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: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [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: independent reviews checked live identity parsing, restore ordering, custom-image provenance, file safety, same-user atomic-replace assumptions, and registry publication; all confirmed blockers were resolved. - [ ] 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 — command/result or justification: focused CLI suites 67/67; snapshot, OpenClaw restore, and spawned gateway integrations 53/53; final restore/finalization follow-ups 22/22 and prepared-context integration 2/2. - [x] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: not applicable; this is a scoped DCode onboarding/restore fix, and the targeted suites plus normal repository hooks cover the changed boundaries. - [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) — passed with 0 errors and 2 existing unspecified Fern warnings. - [x] 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: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Managed Deep Agents Code onboarding and resume now verify live `dcode identity` to detect selection drift and recreate sandboxes when selections are unreadable or mismatched. * Managed restores can merge backed-up `config.toml` while keeping freshly generated model routing and provider metadata authoritative. * **Bug Fixes** * Prevents unverified managed DCode reuse when the expected registry entry is missing. * Improves gateway-start failure output by redacting sensitive diagnostics and printing clearer remediation commands. * **Documentation** * Updated quickstart and command reference for revised restore behavior and non-interactive recreation rules. * **Tests** * Added coverage for managed config merge ownership, selection drift, and sandbox finalization/resume flows. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Chengjie Wang <chengjiew@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Chengjie Wang <chengjiew@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Author
|
Thanks @apurvvkumaria, makes sense to me. I’m good with superseding this PR in favor of #6332, and I appreciate you preserving the co-author credit. |
wscurran
added
area: onboarding
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
## Summary Keep managed Deep Agents Code runtime routing aligned with the provider/model selected during same-name re-onboarding. The fix treats the live `dcode identity` result as authoritative, reconciles mixed-ownership `config.toml` state without restoring stale routing, and publishes registry metadata only after the restored runtime is verified. ## Related Issue Closes NVIDIA#6311 Supersedes NVIDIA#6317. This incorporates and extends @chengjiew's original drift/recreate investigation; Chengjie is credited as a co-author on the commit. ## Changes - Recreate stock managed DCode sandboxes when live route, provider, model, or endpoint identity is stale or unreadable; refuse unverifiable reuse when the registry row is absent. - Restore only four bounded display preferences from backed-up DCode config: three booleans and the enumerated thread sort order. Keep fresh `models`, `update`, and generated provider metadata authoritative, and drop free-form or behavior-bearing backup settings. - Perform the merge with bounded TOML parsing, regular-file checks, same-directory staging, inode revalidation, `fsync`, and atomic replacement. - Validate the live restored selection before writing registry/status metadata, and leave custom-image plus ordinary snapshot/rebuild restore behavior unchanged. - Keep generic hotspots bounded by extracting gateway failure handling, DCode resume policy, and caller-authorized state-file restore policy into focused modules. - Add unit, handler, restore-boundary, and orchestration regression coverage plus user-facing documentation. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [x] 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: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [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: independent reviews checked live identity parsing, restore ordering, custom-image provenance, file safety, same-user atomic-replace assumptions, and registry publication; all confirmed blockers were resolved. - [ ] 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 — command/result or justification: focused CLI suites 67/67; snapshot, OpenClaw restore, and spawned gateway integrations 53/53; final restore/finalization follow-ups 22/22 and prepared-context integration 2/2. - [x] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: not applicable; this is a scoped DCode onboarding/restore fix, and the targeted suites plus normal repository hooks cover the changed boundaries. - [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) — passed with 0 errors and 2 existing unspecified Fern warnings. - [x] 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: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Managed Deep Agents Code onboarding and resume now verify live `dcode identity` to detect selection drift and recreate sandboxes when selections are unreadable or mismatched. * Managed restores can merge backed-up `config.toml` while keeping freshly generated model routing and provider metadata authoritative. * **Bug Fixes** * Prevents unverified managed DCode reuse when the expected registry entry is missing. * Improves gateway-start failure output by redacting sensitive diagnostics and printing clearer remediation commands. * **Documentation** * Updated quickstart and command reference for revised restore behavior and non-interactive recreation rules. * **Tests** * Added coverage for managed config merge ownership, selection drift, and sandbox finalization/resume flows. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Chengjie Wang <chengjiew@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Chengjie Wang <chengjiew@nvidia.com> Co-authored-by: Carlos Villela <cvillela@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
3 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
Recreate an existing ready sandbox during fresh onboarding when the selected provider/model differs from the registry metadata. This prevents
nemoclaw statusfrom showing the new model while the live in-sandbox agent keeps running the previous startup configuration.Related Issue
Fixes #6311
Changes
Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailableNPM_CONFIG_CACHE=/tmp/nemoclaw-6311-npm-cache npx vitest run src/lib/onboard/machine/handlers/sandbox.test.ts src/lib/onboard/machine/handlers/sandbox-resume.test.ts --project clipassed, 40 tests.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:NPM_CONFIG_CACHE=/tmp/nemoclaw-6311-npm-cache npm run typecheck:clipassed.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Chengjie Wang chengjiew@nvidia.com
Summary by CodeRabbit
config.tomlto ensure the newly selected model is used.config.tomlrestore behavior.