fix(onboard): restore not-ready check after compatibility decisions - #8373
fix(onboard): restore not-ready check after compatibility decisions#8373jyaunches wants to merge 2 commits into
Conversation
PR NVIDIA#8027 moved the not_ready → repair-and-recreate decision to the top of decideSandboxResume, ahead of compatibility, configuration, and tool- disclosure checks. This caused gateway upgrade/recreation and named- rebuild resume flows to fail with 'Cannot replace same-name sandbox: no recreate transaction proves ownership' because: 1. The sandbox appears not_ready on an upgraded gateway (expected) 2. repair-and-recreate fires before compatibility checks can route the resume through their proven journal-capable paths 3. selectedGatewayForSandboxRecreate returns null for old-version checkpoints without gateway authority → journal cannot begin 4. replacesSameNameSandbox is true for repair-and-recreate → throws Restoring the original ordering (not_ready checked AFTER compatibility, reuse, configuration, and tool-disclosure) ensures: - Gateway upgrades route through compatibility-driven recreate (no journal ownership required since removeRegistryEntry is false) - Named rebuilds route through recreateSandboxRequested (config check) - not_ready only triggers repair-and-recreate as a last resort for genuinely damaged sandboxes where no other trigger explains the state Fixes the gateway-version E2E regression (4/5 jobs failing) and the Deep Agents named-rebuild regression introduced by 3312899. Signed-off-by: Jessica Yaunches <jyaunches@nvidia.com>
📝 WalkthroughWalkthrough
ChangesSandbox resume handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/sandbox-resume.test.ts`:
- Around line 136-138: Rename the test case around the DCode auto-approval
scenario to behavior-oriented wording that states it falls through to
repair-and-recreate when the sandbox is not_ready. Keep the test implementation
and assertions unchanged.
- Around line 163-178: Strengthen the compatibility-recreate test for
decideSandboxResume by using the table’s _noteFragment parameter to assert that
note contains the expected fragment, and require removeRegistryEntry to equal
false. Keep the existing recreate-kind assertion and verify these concrete
metadata values for every drift case.
🪄 Autofix
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: fbe2284b-915a-4443-9a61-cf75552e76a5
📒 Files selected for processing (2)
src/lib/onboard/machine/handlers/sandbox-resume.test.tssrc/lib/onboard/machine/handlers/sandbox-resume.ts
| ["live DCode inference selection", { inferenceSelectionChanged: true }, "DCode model/provider"], | ||
| ["agent selection", { resumeAgentChanged: true }, "Agent selection changed"], | ||
| ["Hermes inference route", { inferenceRouteConfigChanged: true }, "inference route"], | ||
| ] as const)("uses compatibility recreate for %s drift even when not-ready", (_label, drift, _noteFragment) => { | ||
| expect( | ||
| decideSandboxResume( | ||
| resumeSignals({ | ||
| sandboxReuseState: "not_ready", | ||
| ...drift, | ||
| }), | ||
| ), | ||
| ).toEqual({ kind: "repair-and-recreate" }); | ||
| ).toEqual({ | ||
| kind: "recreate", | ||
| note: expect.any(String), | ||
| removeRegistryEntry: expect.any(Boolean), | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the compatibility decision metadata.
The table defines _noteFragment, but the assertion does not use it. expect.any(String) accepts an unrelated note, and expect.any(Boolean) accepts removeRegistryEntry: true. The test can pass after a registry-entry handling regression. Assert the note fragment and removeRegistryEntry: false for each case.
As per path instructions, tests must verify observable outcomes through the public boundary instead of accepting arbitrary values for contract fields.
Proposed fix
- note: expect.any(String),
- removeRegistryEntry: expect.any(Boolean),
+ note: expect.stringContaining(_noteFragment),
+ removeRegistryEntry: false,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ["live DCode inference selection", { inferenceSelectionChanged: true }, "DCode model/provider"], | |
| ["agent selection", { resumeAgentChanged: true }, "Agent selection changed"], | |
| ["Hermes inference route", { inferenceRouteConfigChanged: true }, "inference route"], | |
| ] as const)("uses compatibility recreate for %s drift even when not-ready", (_label, drift, _noteFragment) => { | |
| expect( | |
| decideSandboxResume( | |
| resumeSignals({ | |
| sandboxReuseState: "not_ready", | |
| ...drift, | |
| }), | |
| ), | |
| ).toEqual({ kind: "repair-and-recreate" }); | |
| ).toEqual({ | |
| kind: "recreate", | |
| note: expect.any(String), | |
| removeRegistryEntry: expect.any(Boolean), | |
| }); | |
| ["live DCode inference selection", { inferenceSelectionChanged: true }, "DCode model/provider"], | |
| ["agent selection", { resumeAgentChanged: true }, "Agent selection changed"], | |
| ["Hermes inference route", { inferenceRouteConfigChanged: true }, "inference route"], | |
| ] as const)("uses compatibility recreate for %s drift even when not-ready", (_label, drift, _noteFragment) => { | |
| expect( | |
| decideSandboxResume( | |
| resumeSignals({ | |
| sandboxReuseState: "not_ready", | |
| ...drift, | |
| }), | |
| ), | |
| ).toEqual({ | |
| kind: "recreate", | |
| note: expect.stringContaining(_noteFragment), | |
| removeRegistryEntry: false, | |
| }); |
🤖 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.test.ts` around lines 163 -
178, Strengthen the compatibility-recreate test for decideSandboxResume by using
the table’s _noteFragment parameter to assert that note contains the expected
fragment, and require removeRegistryEntry to equal false. Keep the existing
recreate-kind assertion and verify these concrete metadata values for every
drift case.
Source: Path instructions
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Address PR review advisor PRA-2 warning: add test coverage ensuring runtime-configuration decisions (recreateSandboxRequested, web search, GPU, messaging, Hermes gateway, observability) and tool-disclosure decisions (migration, change) take precedence over the not-ready repair fallback. Also rename the DCode auto-approval test to accurately reflect the repair-and-recreate outcome (PRA-1 suggestion). Signed-off-by: Jessica Yaunches <jyaunches@nvidia.com>
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/sandbox-resume.test.ts`:
- Around line 182-202: Update the parameterized cases in the decideSandboxResume
tests to include an expected diagnostic-note fragment for each drift scenario,
including the cases around the second assertion block. Replace each
expect.any(String) note matcher with
expect.stringContaining(expectedNoteFragment), while preserving the existing
kind and exact removeRegistryEntry assertions.
🪄 Autofix
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: 200fc238-e218-4a25-89ac-73b498592c8a
📒 Files selected for processing (1)
src/lib/onboard/machine/handlers/sandbox-resume.test.ts
| it.each([ | ||
| ["explicit recreate request", { recreateSandboxRequested: true }, false], | ||
| ["web search config change", { webSearchConfigChanged: true }, true], | ||
| ["sandbox GPU config change", { sandboxGpuConfigChanged: true }, true], | ||
| ["messaging channel config change", { messagingChannelConfigChanged: true }, true], | ||
| ["Hermes tool gateway config change", { hermesToolGatewayConfigChanged: true }, true], | ||
| ["observability change", { observabilityChanged: true }, false], | ||
| ] as const)("uses runtime-configuration recreate for %s even when not-ready", (_label, drift, expectedRemoveRegistry) => { | ||
| expect( | ||
| decideSandboxResume( | ||
| resumeSignals({ | ||
| sandboxReuseState: "not_ready", | ||
| ...drift, | ||
| }), | ||
| ), | ||
| ).toEqual({ | ||
| kind: "recreate", | ||
| note: expect.any(String), | ||
| removeRegistryEntry: expectedRemoveRegistry, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the scenario-specific diagnostic note.
The expect.any(String) matchers at Line 199 and Line 217 only prove that the result contains a string. They do not prove that decideSandboxResume reports the drift that triggered recreation. A wrong branch can pass when its kind and removeRegistryEntry values match.
Add an expected note fragment to each table row. Assert it with expect.stringContaining(expectedNoteFragment). Keep the exact removeRegistryEntry assertions.
As per path instructions, tests must verify observable outcomes through the public boundary instead of accepting arbitrary values for contract fields.
Also applies to: 204-219
🤖 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.test.ts` around lines 182 -
202, Update the parameterized cases in the decideSandboxResume tests to include
an expected diagnostic-note fragment for each drift scenario, including the
cases around the second assertion block. Replace each expect.any(String) note
matcher with expect.stringContaining(expectedNoteFragment), while preserving the
existing kind and exact removeRegistryEntry assertions.
Source: Path instructions
2 participants
Summary
Restores the original ordering of the
not_readysandbox check indecideSandboxResume, placing it after compatibility, configuration,and tool-disclosure checks instead of before them.
Related Issue
Regression introduced by #8027 (commit 3312899).
Problem
PR #8027 moved the
not_ready → repair-and-recreatedecision to thetop of
decideSandboxResume. This caused gateway upgrade/recreationand named-rebuild resume flows to fail because:
not_readyon an upgraded gateway (expectedtransient state during upgrade)
repair-and-recreatefires before compatibility checks can routethe resume through their proven journal-capable paths
selectedGatewayForSandboxRecreatereturns null for old-versioncheckpoints without gateway authority → journal cannot begin
replacesSameNameSandboxis true forrepair-and-recreate→ throwsCannot replace same-name sandbox: no recreate transaction proves ownership of the source sandbox and its registry row.Fix
Restore the original ordering so that:
journal ownership required since
removeRegistryEntryis false)recreateSandboxRequested(config check)not_readyonly triggersrepair-and-recreateas a last resort forgenuinely damaged sandboxes where no other trigger explains the state
Regressions Fixed
with
Recreate failed after sandbox was destroyedleft nemoclaw status failing (same recreate/resume surface)
Type of Change
Quality Gates
in a pure decision function; no new code paths introduced
Verification
npx vitest run src/lib/onboard/machine/handlers/sandbox-resume.test.ts— 30 passednpx vitest run src/lib/onboard/machine/— 602 passednpm run typecheck:cli— cleannpm run build:cli— cleanSigned-off-by: Jessica Yaunches jyaunches@nvidia.com
Summary by CodeRabbit