fix(cli): recover named gateway before sandbox inventory - #10424
Conversation
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit 9c0a818 in the TypeScript / code-coverage/cliThe overall line coverage in commit 9c0a818 in the Show a line coverage summary of the most impacted files.
Updated |
|
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 (3)
Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughNamed-gateway sandbox listing now recovers the gateway before observation. Failed recovery prevents inventory queries. Diagnostics include normalized error metadata and recovery status. Tests cover ordering, healthy gateways, recovery failures, retry suppression, and non-recoverable observation errors. ChangesNamed Gateway Recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change restores named gateway recovery before mutating sandbox inventory and fails closed on unrecoverable typed errors; targeted tests and checks are reported passing, so no actionable merge-blocking risk remains after normal review. Sequence Diagram(s)sequenceDiagram
participant SandboxRequest
participant captureSandboxListWithGatewayRecovery
participant recoverNamedGatewayRuntime
participant OpenShell
SandboxRequest->>captureSandboxListWithGatewayRecovery: request sandbox inventory
captureSandboxListWithGatewayRecovery->>recoverNamedGatewayRuntime: recover named gateway
recoverNamedGatewayRuntime-->>captureSandboxListWithGatewayRecovery: recovery status
captureSandboxListWithGatewayRecovery->>OpenShell: query sandbox list
OpenShell-->>captureSandboxListWithGatewayRecovery: inventory or observation error
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy the coding objectives in [
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
|
Validated the #10421 gateway-upgrade regression on latest PR commit Focused E2E evidence: manual E2E run, using trusted workflow commit
Each job passed the current gateway upgrade, survivor-state and registry assertions, and all five owned cleanup actions. I checked each raw log and found no occurrence of The current PR dispatch expands to the broader maintained matrix, so this run also contains unrelated failures outside these five jobs. Those failures do not affect this focused validation. |
…utage The round-3 advisor run (workflow run 33018204086) failed all 9 specialist lanes with 'PR review advisor inference configuration did not complete' / 'analysis did not complete', not a code finding — a sibling PR (NVIDIA#10424) passed the same lanes minutes earlier on the same workflow. I don't have rerun rights on NVIDIA/NemoClaw Actions as an outside contributor, so this empty commit re-triggers the pull_request_target 'synchronize' event the advisor workflow listens for. No source change. Signed-off-by: harjoth <harjoth.khara@gmail.com>
|
PR review advisory complete for commit |
|
Revalidated #10421 after updating the branch from Gateway-upgrade results from a broad manual PR run: run 33019075848
All five issue-owned gateway-upgrade jobs passed: Each job passed the upgrade assertion, the sandbox-survivor assertion, and all five cleanup checks. I also checked the raw logs for the retired failure signatures; none appeared. Correction: this was not a focused run. The workflow does not currently support a gateway-only PR selector, so the dispatch expanded to the broad default PR matrix. Cancellation was requested after the five issue-owned jobs completed.
|
<!-- markdownlint-disable MD041 --> ## Summary Named-gateway recovery now rejects non-recoverable lifecycle probe failures before it selects or starts a gateway. The change closes the fail-closed test gap left by #10424. ## Related Issue Related to #10421. Follow-up to #10424. ## Changes - Reuse the typed OpenShell CLI error classifier for gateway lifecycle probes. - Block recovery for authentication, schema, gateway identity, and invalid-request failures. - Stop after a failed post-selection probe without starting the gateway. - Apply the same guard to the direct named-gateway reconciliation path. - Add negative tests that assert `gateway select` and `gateway start` do not run. - Root cause: raw lifecycle failures were classified as recoverable topology states. - Detection gap: #10424 tested these errors only after its mocked recovery call returned success. ## 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: - [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: The implementation review covered authentication, schema, gateway identity, invalid-request, and post-selection failure states. Negative tests assert that forbidden mutations do not run. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable; this PR does not change `scripts/prepare-dgx-station-host.sh`. - Station profile/scenario: Not applicable. - Result: Not applicable. - Supporting evidence: Not applicable. ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` 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 --maxWorkers=1 src/lib/gateway-runtime-action.test.ts src/lib/adapters/openshell/sandbox-observer-cli.test.ts src/lib/actions/sandbox/gateway-state-drift.test.ts`: 3 files and 47 tests passed. `npm run test:titles:check` passed. - [ ] Applicable broad gate passed — Not applicable; this change extends one lifecycle classifier and two current mutation guards. - [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) - [ ] 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) Informational local run: `npm run test:changed` passed 5,485 tests and failed 16 tests across nine unchanged files under host concurrency. Eight files passed sequentially; one unchanged uninstall test retained a five-second timeout. GitHub CI remains the authoritative broad result. --- 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 sandbox recovery handling when gateway lifecycle checks detect authentication, schema, identity, or request-validation errors. * Prevented recovery from selecting or starting a gateway when recovery is blocked. * Preserved the original missing-sandbox result and existing registry data in blocked recovery scenarios. * Improved recognition of missing administrator privileges during gateway command checks. * **Tests** * Added coverage for blocked recovery, lifecycle probe failures, and prevention of unintended gateway startup. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Summary
Mutating sandbox inventory now recovers the exact named OpenShell gateway before it lists sandboxes. This restores installer recovery after legacy gateway retirement while keeping typed observer failures fail-closed.
Failure diagnostics now include the sanitized error kind, reason, and whether gateway recovery ran.
Related Issue
Fixes #10421
Changes
Unknown gateway, not the opaque command failure observed after installer retirement.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededDGX Station Hardware Evidence
scripts/prepare-dgx-station-host.sh.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run --project cli src/lib/openshell-sandbox-list.test.ts src/lib/adapters/openshell/sandbox-observer-cli.test.ts src/lib/gateway-runtime-action.test.ts src/lib/actions/upgrade-sandboxes-preflight.test.ts src/lib/actions/upgrade-sandboxes-recovery.test.ts src/lib/actions/maintenance.test.ts src/lib/actions/sandbox/rebuild-flow-helpers.test.ts src/lib/actions/sandbox/rebuild-gateway-drift.test.ts: 8 files and 194 tests passed.npx vitest run --project e2e-support test/e2e/support/openshell-gateway-upgrade-workflow-boundary.test.ts: 1 file and 14 tests passed.npm run test:titles:checkpassed.npm run typecheck:clipassed.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Rebecca Sliter 571084+rsliter@users.noreply.github.com
Summary by CodeRabbit