fix(sandbox): recover gateway before maintenance sandbox-list retry - #4106
Merged
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
ericksoa
added
bug
NV QA
Contributor
📝 WalkthroughWalkthroughAdds a gateway-recovery wrapper for OpenShell sandbox-list queries, makes backup/rebuild/upgrade flows use it (and await recovery where needed), extends the recover API to accept recoverable states, and updates tests to assert recovery and non-recovery behaviors. ChangesGateway Recovery with Retry Logic
Sequence Diagram(s): sequenceDiagram
participant Caller as maintenance/upgrade/rebuild
participant Capture as captureSandboxListWithGatewayRecovery
participant OpenShell as captureOpenshell
participant Gateway as recoverNamedGatewayRuntime
Caller->>Capture: request sandbox list
Capture->>OpenShell: captureOpenshell(["sandbox","list"])
OpenShell-->>Capture: initial result
alt recoverable failure detected
Capture->>Gateway: recoverNamedGatewayRuntime({recoverableStates})
Gateway-->>Capture: recovered? (true/false)
alt recovered == true
Capture->>OpenShell: captureOpenshell(["sandbox","list"]) (retry)
OpenShell-->>Capture: retried result
end
end
Capture-->>Caller: { result, recoveryAttempted, recoverySucceeded }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
Contributor
PR Review AdvisorFindings: 1 needs attention, 0 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
Contributor
E2E Advisor RecommendationRequired E2E: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Contributor
E2E Scenario Advisor RecommendationRequired scenario E2E: None Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
ericksoa
changed the title
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26318311620
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/openshell-sandbox-list.ts (1)
33-44:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPropagate the actual recovery-attempt status instead of hardcoding it.
recoveryAttemptedis currently forced totrueonce Line 33 is reached, even whenrecoverNamedGatewayRuntime()returnsattempted: false(e.g., filtered byrecoverableStates). That can print misleading recovery diagnostics downstream.🔧 Proposed fix
const recovery = await recoverNamedGatewayRuntime({ recoverableStates: ["missing_named", "named_unhealthy", "named_unreachable"], }); if (!recovery.recovered) { - return { result: initial, recoveryAttempted: true, recoverySucceeded: false }; + return { + result: initial, + recoveryAttempted: recovery.attempted, + recoverySucceeded: false, + }; } return { result: captureOpenshell(["sandbox", "list"]), - recoveryAttempted: true, - recoverySucceeded: true, + recoveryAttempted: recovery.attempted, + recoverySucceeded: recovery.attempted && recovery.recovered, }; }🤖 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/openshell-sandbox-list.ts` around lines 33 - 44, The code hardcodes recoveryAttempted: true after calling recoverNamedGatewayRuntime(); update both return sites to use the actual flag from the recovery object instead of true — i.e., replace the literal true with recovery.attempted (keep recoverySucceeded using recovery.recovered and preserve result values: initial on failure and captureOpenshell(["sandbox","list"]) on success) so the recoverNamedGatewayRuntime(), recovery.attempted, recovery.recovered, captureOpenshell, and initial symbols correctly propagate the real attempt status.
🤖 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.
Outside diff comments:
In `@src/lib/openshell-sandbox-list.ts`:
- Around line 33-44: The code hardcodes recoveryAttempted: true after calling
recoverNamedGatewayRuntime(); update both return sites to use the actual flag
from the recovery object instead of true — i.e., replace the literal true with
recovery.attempted (keep recoverySucceeded using recovery.recovered and preserve
result values: initial on failure and captureOpenshell(["sandbox","list"]) on
success) so the recoverNamedGatewayRuntime(), recovery.attempted,
recovery.recovered, captureOpenshell, and initial symbols correctly propagate
the real attempt status.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 40cb3cc6-9e6e-4389-838e-447d516fd4f2
📒 Files selected for processing (5)
src/lib/actions/gateway-drift-preflight.test.tssrc/lib/actions/sandbox/rebuild-gateway-drift.test.tssrc/lib/gateway-runtime-action.tssrc/lib/inference/local.test.tssrc/lib/openshell-sandbox-list.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26318968280
|
cv
approved these changes
May 23, 2026
This was referenced May 23, 2026
This was referenced May 26, 2026
wscurran
added
area: sandbox
12 tasks
This was referenced Aug 24, 2026
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
openshell sandbox listwrapper that recovers the named NemoClaw OpenShell gateway and retries once when the initial list command fails without a known gateway-drift/schema mismatchbackup-all,upgrade-sandboxes, and rebuild's pre-backup liveness checkFixes #3986.
Validation
npm run build:clinpx vitest run src/lib/actions/gateway-drift-preflight.test.ts src/lib/actions/sandbox/rebuild-gateway-drift.test.ts src/lib/actions/global.test.ts src/commands/global-oclif-command-adapters.test.tsnpm run typecheck:clinpm run checksnpx @biomejs/biome check src/lib/openshell-sandbox-list.ts src/lib/actions/maintenance.ts src/lib/actions/upgrade-sandboxes.ts src/lib/actions/sandbox/rebuild.ts src/lib/actions/gateway-drift-preflight.test.ts src/lib/actions/sandbox/rebuild-gateway-drift.test.ts src/lib/actions/global.ts src/lib/actions/global.test.ts src/commands/backup-all.tsgit diff --checkSummary by CodeRabbit
Bug Fixes
Improvements