Repository navigation
fix(onboard): reserve the route before a resumed sandbox recreate - #12055
Conversation
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
laitingsheng
added
bug-fix
|
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 (8)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe change fixes sandbox resume handling for messaging configuration and inference route ownership. Resume now distinguishes missing stored configuration from actual drift and reserves eligible routes for the current session before sandbox creation. ChangesSandbox resume recovery
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant SandboxResume
participant reserveSandboxInferenceRoute
participant createSandbox
SandboxResume->>reserveSandboxInferenceRoute: reserve route with inference metadata and session ID
reserveSandboxInferenceRoute-->>SandboxResume: reservation result
SandboxResume->>createSandbox: create sandbox after successful reservation
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The resume flow preserves sandbox reuse for environment-only messaging configuration and enforces route ownership before sandbox creation. No remaining merge-blocking risk was identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit 2d1cec7 in the TypeScript / code-coverage/cliThe overall line coverage in commit 2d1cec7 in the Show a line coverage summary of the most impacted files.
Updated |
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
Integrated merged dependency #12056 in signed commit Local review: alternative review completed. Reviewed the full eight-file diff against The local Advisor checkout transfer failed with Validation:
Latest-commit hosted CI, automated review, and deterministic E2E remain required. Selected jobs are Signed-off-by: Julie Yaunches jyaunches@nvidia.com |
|
PR Review Advisor finished for commit Request review only when Require no Advisor blockers is green. |
|
Reviewed all nine specialist summaries, findings ledgers, and E2E receipts from Advisor run 35343446310 for The customer-value and migration P1 findings describe one existing limitation: a published explicit host-local route remains unreserved during resumed recreation. A temporary handler-level diagnostic invoked both the candidate handler and the handler from current main Issue #11973 reproduces hosted inference with a custom image. This PR explicitly preserves the host-local exclusion; the finding identifies an inherited, unaddressed sibling path rather than behavior newly broken by this change. No expansion into that path has been made. The Advisor aggregate still fails; this disposition does not claim clearance or waive the gate. Required CI and managed-image activation now pass. Manual E2E run 35342194508 stopped before live fanout because it was dispatched before the image prerequisite completed; it provides no live qualification. Further publication is awaiting clarification of the newly supplied v1-only/no-image-publication instructions. |
|
The inherited host-local reservation finding is now tracked in dependency #12075, commit A separate checkout of current This PR remains unchanged at The earlier checkout-guidance question is resolved: this work belongs on |
rsliter
left a comment
rsliter
left a comment
There was a problem hiding this comment.
Approved at exact head 2d1cec7.
Reviewed every changed file. The resumed recreate path revalidates the existing route under the mutation locks, reserves it for the current session immediately before creation, preserves host-local provenance, and leaves foreign pending ownership to fail closed at the verified create boundary. The messaging change correctly avoids treating ambient environment values as drift when the sandbox recorded no channel configuration.
Validation: focused sandbox route, messaging, and resume tests passed (64/64); exact-head CI and CodeRabbit are green; no unresolved review threads.
Security review PASS across injection, authentication/authorization, cryptography, secrets, supply chain, configuration, data privacy, concurrency, and error handling.
) ## Outcome Resumed sandbox creation reserves a published host-local inference route for the current onboarding session before create admission. Retries also validate existing pending host-local authority. It preserves the recorded receipt, provenance, gateway port, and runtime driver. Changed or incomplete authority remains rejected. ## Reason Create admission requires a session-owned pending route. A resumed sandbox step can instead retain its published host-local row and fail with “The sandbox create route reservation is not owned by this onboarding session.” The failure reproduced on `main` at `39581824493055c15d13fc1d27a8477395e2e602`. This PR complements the hosted-route repair in merged PR #12055. The hosted path excludes host-local provenance; this PR repairs that host-local path. ## Changes - Use the existing registry reservation API inside the sandbox create flow's locks, after preparing the recreate transaction and before create admission. - Carry explicit host-local authority into reservation, including pending retries. Let the registry reject selection changes, missing runtime authority, and competing ownership. - Exercise the handler with the real registry and the authority actually passed to create admission. Cover published and same-session pending admission, changed-model rejection, missing gateway or driver authority, and a reservation owned by another session. - Name the sandbox and provide a documented `doctor` diagnostic command when host-local reservation fails. Preserve the original error as the cause and leave registry authority unchanged. ## Verification <!-- ci-loop-12075-validation-3d98d8b1d --> - Automated validation passed for `3d98d8b1d`: 55 successful PR checks and 20 skipped checks, with no failures. - [PR Review Advisor 35792599095](https://github.com/NVIDIA/NemoClaw/actions/runs/35792599095): all nine specialists clear; `Require no Advisor blockers` passed. - [Selective E2E 35793745741](https://github.com/NVIDIA/NemoClaw/actions/runs/35793745741), attempt 1: all seven selected test jobs and `Relevant E2E` passed. Coverage: cloud onboarding, OpenClaw repair, OpenClaw/Hermes resume, OpenShell upgrades from v0.0.89 and v0.0.123, and the Deep Agents typed target. The trusted `main` workflow used Docker and `inference_mode=mock`. This is focused evidence, not full-suite release or live llama.cpp/GPU qualification. - The trusted E2E dispatch receipt matches candidate `3d98d8b1dbe809b059d8b409ac0ad1efe1b5ac05`, base `d14ad9b7a79aaab14cf5e987eb24f790ed03e31c`, workflow `d34d7fbebaa0173c84ea525b7da4d4e220738b63`, run, attempt, and selected jobs/target. <!-- ci-loop-12075-repair-3d98d8b1d --> - Current revision `3d98d8b1d`: 122 tests passed across seven CLI suites covering host-local reservation, route publication, recreate journal/resume, route locking, registry reservation, and core flow phases. Command: `node_modules/.bin/vitest run --project cli --maxWorkers 1 --testTimeout 30000` with those seven test files. - Four missing-authority diagnostic cases failed before the repair and passed afterward. Published and pending rows missing a gateway port or driver now identify the sandbox and its `doctor` command, preserve the underlying error, prevent create, and leave the registry unchanged. - Normal pre-commit, commit-msg, publication-validation, and CLI type-check hooks passed for the diagnostic repair. GitHub verifies all four PR commits. - Reproduced the ownership failure on a separate current-main checkout. The positive admission and changed-route rejection tests failed before the repair. - Focused CLI tests for host-local reservation, route publication, recreate transactions, and registry reservations: 126 passed. After adding the fourth host-local denial case, all four host-local tests passed. - Review repair: 130 focused tests passed across four suites. Both pending-row denial tests failed before removing the validation bypass. A temporary wrong-session mutation at the create caller made the positive handoff test fail; the original source bytes were restored. - Core onboarding flow tests: 35 passed. These existing tests required execution outside the filesystem sandbox for their config writes. - Normal pre-commit and commit-msg checks passed, including lint, repository rules, growth limits, and secret scanning. - Normal pre-push publication validation and CLI type-check passed for the repair. Plugin and JavaScript configuration checks passed on the initial commit and were not selected for the repair. Those initial two signed commits are Verified on GitHub. - The diff contains no secrets, API keys, or credentials. ## Review notes CI loop update: merge commit `2db858272` incorporated PR #12055 without changing either reservation path. All 48 reported CI checks passed for that commit, with seven skipped. [Advisor run 35789179863](https://github.com/NVIDIA/NemoClaw/actions/runs/35789179863) completed all nine specialists: eight clear and one operability diagnostic finding. Commit `3d98d8b1d` repairs that finding with failing-test-first evidence. CI and Advisor evaluation for `3d98d8b1d` passed. Required selective E2E also passed; see the linked results above. Alternative review completed for NVIDIA/NemoClaw commit `b1c5da5d3d86d5c8d977e35bceba784cff022e43` against base `39581824493055c15d13fc1d27a8477395e2e602`. Full-diff self-review covered all five changed files under `src/lib/onboard.ts` and `src/lib/onboard/machine/`, registry authority, create admission, lock placement, error propagation, and recreate journal identity. No remaining actionable finding was identified. This is not independent approval. The local Advisor was unavailable during this shepherd session: transferring its disposable review bundle into the Lima VM failed with `ENOSPC`. The partial task-owned transfer was removed; no controller, gateway, or sandbox started. No local Advisor clearance is claimed. Hosted Advisor run 35350904716 completed all nine specialists for the initial commit: eight clear and one test-handoff finding. The repair makes the stub qualify the fifteenth creation argument, and mutation evidence proves the test detects a foreign session. CodeRabbit’s pending-row concern is addressed for explicit host-local authority through the existing registry validator. No interrupted-write producer of an incomplete row was found; the denial tests deliberately restore malformed rows. Routes without explicit provenance retain their existing classification. This test evidence covers the handler and registry boundary. Live llama.cpp recovery is not claimed. Hosted CI, independent automated review, and deterministically selected E2E passed for `3d98d8b1d`. The PR remains draft and still requires an approving maintainer review before merge. --- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved reliability when resuming host-local sandboxes by checking that the inference route reservation still matches the current session and route before creation. * Reuses pending reservations owned by the same session and prevents sandbox creation when a reservation is outdated, belongs to another session, or lacks required connection details. * Provides recovery guidance when a reservation cannot be accepted and keeps registry state unchanged in rejected cases. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
…nt (#12372) <!-- markdownlint-disable MD041 --> ## Outcome `nemoclaw onboard --resume` can now recreate or repair a sandbox with GPU passthrough after an earlier run completed the sandbox step. Before this change, that resumed run always exited with "A previous onboarding attempt recorded sandbox '<name>' with different build or policy inputs than this run requests." A change to the sandbox GPU settings is still rejected. ## Reason The onboarding sandbox step records a `sandbox_create` fingerprint after `createSandbox()` returns. The fingerprint serialized the whole sandbox GPU configuration object. When GPU passthrough is enabled, sandbox creation runs the GPU proof after the sandbox is Ready and writes the proof result to `sandboxGpuProof` on that same object. The recorded fingerprint therefore included the GPU proof, which is a result of creation, not an input. A resumed run rebuilds the GPU configuration without `sandboxGpuProof`. The pre-create check in `assertCheckpointCreateInputsStillMatch` then never matched, so every resume that recreated or repaired the sandbox exited. The output then recommended `nemoclaw onboard --resume` again, which repeated the same failure. ### Related issues - Fixes #12266 - Relates to #9297: the same rejection on a GPU host. #9318 made `--recreate-sandbox` skip the check, but the GPU proof stayed in the fingerprint. - Relates to #10209: the same rejection on a GPU host after a re-onboard. #10236 removed the first failure in that report, but the fingerprint did not change. - Relates to #11973: the same resume sequence on a host without GPU passthrough, where the check passed and the run continued to recreate the sandbox. #12055 stopped channel values in the process environment from forcing that recreate. `v0.0.127`, which #12266 reports, does not contain #12055. ## Changes - `src/lib/onboard/machine/handlers/sandbox.ts`: - `currentSandboxCreateFingerprint` serializes the sandbox GPU configuration without `sandboxGpuProof`. A configuration without that field serializes as before, so fingerprints recorded without a GPU proof stay valid. The Hermes GPU proof and the GPU compatibility fallback write the same field, so this change also covers them. - Receipts recorded before this change still contain `sandboxGpuProof`. Its value is not available on resume, so the check cannot rebuild the old fingerprint. Checkpoint crash recovery and `assertCheckpointCreateInputsStillMatch` now pass the recorded fingerprint through `withoutRecordedSandboxGpuProof` first. It locates the recorded GPU settings after the current fields that precede them and removes only `sandboxGpuProof`. A receipt without that member is unchanged and still needs an exact match. - `src/lib/onboard/machine/handlers/sandbox-recreate-resume.test.ts`: two tests run onboarding twice on one saved session. The first run's `createSandbox` writes a GPU proof onto the configuration that it receives. The resumed run finds the sandbox not ready. Each test runs once with a recorded receipt that omits the GPU proof and once with a receipt that contains it. - The first test proves that the resumed run recreates the sandbox. - The second test proves that a change to the sandbox GPU mode is still rejected. - `src/lib/onboard/machine/handlers/sandbox-checkpoint-crash-recovery.test.ts`: two crash-recovery tests use a recorded receipt with a GPU proof whose text contains `}|`. - The first test proves that the resumed run reuses the surviving sandbox. - The second test proves that a change to the sandbox GPU mode is still rejected. ## Verification - `npx vitest run --project cli src/lib/onboard/machine/handlers/sandbox-recreate-resume.test.ts src/lib/onboard/machine/handlers/sandbox-checkpoint-crash-recovery.test.ts` — without the `sandbox.ts` change of the latest commit, the two new tests that accept a receipt with a GPU proof failed with `exit 1` from `rejectDriftedCheckpointFingerprint`, and 47 tests passed. With the change, 49 tests passed, including after the documentation review corrections. - `NEMOCLAW_CORPORATE_CA_IMPORT=0 npx vitest run --project cli src/lib/onboard/machine src/lib/onboard/abandoned-route-reservation.test.ts` — 76 files and 1075 tests passed. - `NEMOCLAW_CORPORATE_CA_IMPORT=0 npm run test:changed` — growth guardrails passed (7 tests); 24 files and 285 tests passed. - `NODE_OPTIONS=--max-old-space-size=12288 npm run typecheck:cli` — passed. - `npm run checks:repository` — 18 of 18 checks passed. - `npm run test:titles:check` — passed, including after the documentation review corrections. - `npx oxlint` and `npx oxfmt --check` on the changed files, and `git diff --check` — clean. - `git commit` pre-commit and commit-msg hooks — passed, including the codebase growth guardrails and commitlint. - Documentation review of the staged tree on `bd5e395e23` — PASS, no documentation update needed. Its two wording corrections are applied. - Not run: reproduction on GPU hardware, `npm test`, and `npm run validate:pr`. - The diff contains no secrets, API keys, or credentials. ## Review notes - Sensitive path: onboarding resume and sandbox checkpoint state. The fingerprint still compares every create input and excludes only the GPU proof. For a recorded receipt, only a `sandboxGpuProof` member in the GPU settings is removed before the comparison, and every other field still needs an exact match. The tests that change the GPU mode are the negative evidence for both checks. No credential, policy, or network behavior changes. - For NemoClaw-managed images, the first fingerprint field is the NemoClaw build version. After an upgrade, a session recorded by the earlier release still reports different build or policy inputs, with or without a GPU proof. Rerun that session with `nemoclaw onboard --resume --recreate-sandbox` or `nemoclaw onboard --fresh`. The receipt compatibility applies when that field matches, for example for a sandbox built with `--from <Dockerfile>`, whose first field is the sandbox name. --- Signed-off-by: Tinson Lai <tinsonl@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Sandbox recovery now handles older checkpoints containing GPU verification data, allowing compatible ready sandboxes to be reused and compatible unready sandboxes to be recreated during resume. * When recorded and current GPU modes differ, recovery stops before creating a sandbox and prompts for `--recreate-sandbox`. Other GPU configuration differences remain part of the compatibility check. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Tinson Lai <tinsonl@nvidia.com> Co-authored-by: Rebecca Sliter <sliterrm@gmail.com>
Outcome
nemoclaw onboard --resumecan recreate a sandbox after its inference step was skipped. Before this change, any resume drift decision after a completed sandbox step aborted withError: The sandbox create route reservation is not owned by this onboarding session. A sandbox that recorded no messaging channel configuration is no longer recreated because the process environment carries channel values.Reason
Sandbox creation admits only a pending inference route reservation that the current onboarding session owns. A resumed run reserves that route only when the sandbox step is incomplete, when a failed terminal snapshot resumes, or when the inference step reruns. A resume that skips inference after a completed sandbox step therefore holds the published registry row of the sandbox it is about to replace, and the recreate transaction removes that row before admission runs.
The messaging drift signal compared the recorded channel configuration with the configuration hydrated from the process environment. When the sandbox recorded no channel configuration, any channel value in the environment counted as drift and forced the recreate that then failed.
Related issues
Fixes #11973
Changes
src/lib/onboard/machine/handlers/sandbox.ts: beforecreateSandbox, convert a published registry row into the session's pending route reservation with the route fields of the create authority. Pending rows, including rows owned by another session, and rows with host-local inference provenance are left unchanged, so the admission check keeps rejecting them. The new handler dependencyreserveSandboxInferenceRouteis required because the handler decides the recreate and the create orchestration admits only a session-owned reservation.sandbox-route-publication.test.tsprotects the call, its arguments, its order beforecreateSandbox, and the pending-row cases.src/lib/onboard/machine/handlers/sandbox-resume.ts: addhasMessagingChannelConfigDrift, which reports no drift when the sandbox recorded no channel configuration.sandbox.tsuses it for themessagingChannelConfigChangedresume signal.src/lib/onboard.ts: wireregistry.reserveSandboxInferenceRouteinto the sandbox handler dependencies and write three session dependencies as plain properties instead of a spread, which keeps the file net-neutral.sandbox-route-publication.test.ts(reserve before a resumed recreate, no reserve when the session already owns the reservation, no reserve for a reservation owned by another session),sandbox-ready-messaging.test.ts(Ready sandbox without channels is reused when only the environment carries channel values),sandbox-resume.test.ts(drift helper), and the dependency insandbox-test-fixtures.tsandcore-flow-phases.test.ts.Verification
npx vitest run --project cli src/lib/onboard/machine/handlers/sandbox-route-publication.test.ts src/lib/onboard/machine/handlers/sandbox-ready-messaging.test.ts src/lib/onboard/machine/handlers/sandbox-resume.test.ts— 64 tests passed.npm run test:changed— growth guardrails 7 passed; CLI project 2288 passed, 1 skipped, 1 failed:src/lib/onboard/runtime-provider/podman.test.tstimed out at 5000 ms whiletscran in parallel; rerun alone, 21 passed.npx tsc -p tsconfig.cli.json— passed in the pre-push hook after the OpenShell SDK was installed.npx tsx scripts/checks/source-architecture.mts— passed.npx oxlintandnpx oxfmt --checkon the changed files — 0 warnings, 0 errors, formatted.prekpre-commit and pre-push hooks — passed.npm run docs— not run; nodocs/file changed.Review notes
Sensitive path (onboarding, sandbox lifecycle, registry route reservation). The change writes a reservation only for the current session and only over a published row; a pending reservation owned by another session still fails closed at the create admission check, and
sandbox-route-publication.test.tsasserts that no reservation write happens in that case.Documentation Writer Review: Result: no-docs-needed. Reviewed commit: staged tree on
d9c33770772f06264b3a45b52fe9efc6d83aa34f. AGENTS.md blob:144114b944af8b52f23dd53a8855776bf9d0e674. Agent: Devin CLI subagent. The reviewer recommended a post-merge changelog entry for the changed resume behavior.The messaging drift trigger in the report depends on channel configuration variables in the reporter's process environment; the environment was not available for this change. The route ownership failure reproduces from source for every resume drift decision after a completed sandbox step whose inference step is skipped.
Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit