fix(cli): fail closed on OpenShell gateway drift - #3423
Conversation
Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds OpenShell gateway-image-drift and protobuf/schema-mismatch detection, formatted issue printing, integrates preflight/result validation into maintenance and sandbox flows, adds adapter and tests, sets a Vitest sentinel, updates installer/onboard/e2e/test helpers, and threads a runtime timeout option. ChangesGateway Drift Detection Infrastructure
Maintenance Actions Gateway Drift Integration
Sandbox Operations Gateway Drift Integration
Error Classification and Recovery
Supporting Infrastructure and Cleanup
Sequence DiagramsequenceDiagram
participant CLI as NemoClaw CLI
participant OpenShell as OpenShell runtime
participant Docker as Docker engine
CLI->>OpenShell: capture RPC (e.g. sandbox list / inference get)
CLI->>Docker: inspect gateway cluster container (image, ports)
Docker-->>CLI: container inspect JSON (image, published ports)
OpenShell-->>CLI: RPC result { status, output }
CLI->>CLI: detectOpenShellStateRpcPreflightIssue (uses Docker/OpenShell info)
CLI->>CLI: detectOpenShellStateRpcResultIssue (analyze result output/status)
CLI->>CLI: formatOpenShellStateRpcIssue -> printOpenShellStateRpcIssue (console.error)
CLI-->>CLI: exit(1) on detected issue
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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 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 |
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/adapters/openshell/gateway-drift.ts`:
- Around line 258-292: The user-visible guidance always says "preflight failed
before ${action}" and hardcodes the default gateway volume name
(DEFAULT_GATEWAY_NAME); update the message to be phase-aware and gateway-name
aware: change the opening line built around action to choose wording based on
whether the issue was detected during preflight versus at runtime (e.g. consult
issue.phase or issue.detectedAt / issue.kind === "protobuf_mismatch" to emit
"preflight failed before ${action}" vs "detected while ${action} was running"),
and replace the hardcoded DEFAULT_GATEWAY_NAME reference with the actual gateway
name variable used elsewhere in this module (use the gateway name variable
instead of DEFAULT_GATEWAY_NAME) so the recovery text and volume name reflect
custom gateways; keep all other context lines (drift, OpenShell output via
compactOutput, CLI_DISPLAY_NAME, CLI_NAME, command) unchanged.
🪄 Autofix (Beta)
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: 280a86e2-4450-4928-a7fe-c385c68c5fcf
📒 Files selected for processing (17)
ci/env-var-doc-allowlist.jsonscripts/install.shsrc/lib/actions/gateway-drift-preflight.test.tssrc/lib/actions/maintenance.tssrc/lib/actions/sandbox/gateway-state-drift.test.tssrc/lib/actions/sandbox/gateway-state.tssrc/lib/actions/sandbox/rebuild-gateway-drift.test.tssrc/lib/actions/sandbox/rebuild.tssrc/lib/actions/sandbox/status.tssrc/lib/actions/upgrade-sandboxes.tssrc/lib/adapters/openshell/gateway-drift.test.tssrc/lib/adapters/openshell/gateway-drift.tssrc/lib/adapters/openshell/runtime.tssrc/lib/onboard.tssrc/lib/runtime-recovery.test.tssrc/lib/runtime-recovery.tsvitest.config.ts
Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
## Summary Adds a failing-test-first regression guard for #3423 / #3399 in the dedicated `regression-e2e.yaml` holding pen. The guard exercises the CLI boundary with fake `openshell` and `docker` shims so it catches the real failure mode that unit-level mocks can miss: 1. `openshell sandbox list` exits non-zero with the #3399 protobuf `invalid wire type` decode error. 2. A stale patched gateway image reports as `nemoclaw-cluster:0.0.36-fuse-overlayfs-aa8b8487` while installed OpenShell is `0.0.37`. ## Expected red/green behavior - 🔴 On current main / unfixed code, `gateway-drift-preflight-e2e` fails because NemoClaw either treats the protobuf failure as a stopped sandbox or misses the patched-image drift. - 🟢 On #3423 after the fix, the same job should pass by failing closed with schema-drift recovery guidance and by detecting patched stale gateway images before trusting sandbox state. ## Regression workflow This is **not** added to scheduled nightly. It lives in `.github/workflows/regression-e2e.yaml` and can be dispatched explicitly: ```bash gh workflow run regression-e2e.yaml -f jobs=gateway-drift-preflight-e2e --ref <branch> ``` ## Verification - `bash -n test/e2e/test-gateway-drift-preflight.sh` - `bash test/e2e/test-gateway-drift-preflight.sh` currently fails on the expected missing fail-closed guidance (red guard) before #3423 lands. Related: #3423 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added an end-to-end regression that validates gateway-drift preflight behavior, ensuring schema mismatches and stale gateway images fail safely and report correct diagnostics. * **Chores** * Updated CI workflow to include and dispatch the new gateway-drift preflight regression job. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/3463) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Coverage guard landed; PR needs rebase/fixCoverage guard PR #3463 has merged to I merged current bash test/e2e/test-gateway-drift-preflight.shResult: the protobuf mismatch half now passes, but the patched-image drift half still fails: Please rebase/merge as OpenShell cluster version gh workflow run regression-e2e.yaml -f jobs=gateway-drift-preflight-e2e --ref fix/3399-gateway-drift-preflightExpected final state: |
Recognize locally patched nemoclaw-cluster image tags when comparing the running OpenShell gateway image with the installed CLI version. Also hardens the regression guard shims so the fake Docker command survives NemoClaw's subprocess environment filtering. Related: NVIDIA#3423 NVIDIA#3463
✅ Coverage guard verified
Note: GitHub Actions can only dispatch This PR now has a regression test proving the fix. The regression guard stays in |
|
✨ Thanks for submitting this detailed PR to address the OpenShell gateway drift issue. This change aims to improve the robustness of the sandbox environment by failing closed when OpenShell sandbox-state RPCs may be schema-broken. Related open PRs: Related open issues: |
## Summary Migrate `test/e2e/test-gateway-drift-preflight.sh` with the simplest equivalent Vitest coverage. ## Related Issues Refs #5098 Refs #3399 Refs #3423 ## Contract mapping - Legacy assertion: protobuf invalid-wire sandbox-list failures fail closed and do not classify a registered sandbox as stopped. - Replacement: `test/gateway-drift-preflight.test.ts` drives the real repo CLI through fake `openshell`/`docker` binaries and asserts fail-closed diagnostics/no backup success. - Boundary preserved: real `node bin/nemoclaw.js backup-all`, PATH-resolved `openshell sandbox list`, fake process exit/output boundary, and filesystem registry state. - Legacy assertion: stale patched cluster gateway image drift fails before `openshell sandbox list`. - Replacement: `test/gateway-drift-preflight.test.ts` fakes active Docker cluster inspect output and asserts image drift output plus no sandbox-list call. - Boundary preserved: real CLI preflight, PATH-resolved Docker inspect shim, OpenShell version shim, and process exit behavior. - Legacy assertion: host-process gateway binary drift blocks both `backup-all` and `upgrade-sandboxes --check` before sandbox-list. - Replacement: `test/gateway-drift-preflight.test.ts` seeds runtime markers/fallback binaries and invokes both real CLI commands. - Boundary preserved: real host-process runtime marker JSON, live fake gateway PID for marker trust, `/proc`/process identity probe, and fallback binary resolution. - Legacy assertion: a stale dead-PID runtime marker does not fabricate host-process drift when the live fallback gateway binary matches the installed CLI. - Replacement: `test/gateway-drift-preflight.test.ts` seeds a stale marker pointing at an old binary and asserts the CLI proceeds to `openshell sandbox list`. - Boundary preserved: real marker file semantics, fallback PATH sibling resolution, and subprocess call logging. ## Simplicity check - Test shape: focused process Vitest test in `test/gateway-drift-preflight.test.ts`. - Original runner/lane: `regression-e2e.yaml` job `gateway-drift-preflight-e2e`, `runs-on: ubuntu-latest`, Node 22, fake Docker/OpenShell shims, no secrets. - Replacement runner: same runner class via `e2e-vitest-scenarios.yaml` job `gateway-drift-preflight-vitest`, `runs-on: ubuntu-latest`, Node 22, no secrets. - New shared helpers: none; one-off fake CLI/Docker/OpenShell helpers stay local to the test. - New framework/registry/ledger: **none** - Workflow changes: add selective free-standing Vitest job and selector inventory mapping; legacy shell script and regression lane deletion deferred to #5098 Phase 11. - Selective dispatch: `e2e-vitest-scenarios.yaml` with `jobs=gateway-drift-preflight-vitest` on this PR branch. ## Verification - `npm ci --ignore-scripts` - `npm run build:cli` - `npx vitest run --project cli test/gateway-drift-preflight.test.ts --silent=false --reporter=default` - `npx vitest run --project e2e-vitest-support test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts` - `npm run typecheck:cli` - `git diff --check` - PR: #5350 - Same-runner selective run: https://github.com/NVIDIA/NemoClaw/actions/runs/27438428046 — queued Note: local full pre-push CLI suite was attempted by hooks but is not part of the focused migration validation and failed on unrelated local environment/pre-existing issues (`nemoclaw/node_modules/json5` missing, `nemoclaw/dist/blueprint/private-networks.js` missing, macOS timeout/platform expectations). Focused migration validations above passed. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added gateway drift preflight checks to prevent unsafe backup and upgrade operations by detecting schema mismatches, gateway image drift, and host-process incompatibilities. * **Tests** * Expanded end-to-end Vitest scenarios covering gateway drift detection, fail-closed behavior, and various drift permutations. * **Chores** * CI updated with a new automated job for gateway-drift-preflight and integrated so its results appear in PR reports. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Carlos Villela <cvillela@nvidia.com>
Summary
Fail closed when OpenShell sandbox-state RPCs may be schema-broken by gateway image drift or protobuf invalid-wire-type output. This prevents backup, status, rebuild, recover, and sandbox upgrade flows from trusting stale OpenShell state and misclassifying registered running sandboxes as stopped.
Related Issue
Fixes #3399
Changes
src/lib/adapters/openshell/gateway-drift.ts.backup-all,upgrade-sandboxes, sandboxstatus,rebuild, and recover/connect liveness checks before trusting OpenShell state RPCs.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)npm test -- --reporter=dotwas run and did not complete cleanly in this worktree because of existing/unrelated host-sensitive failures observed intest/install-preflight.test.tsandtest/fetch-guard-patch-regression.test.ts. Targeted gateway-drift tests, CLI typecheck, CLI build, shell formatting/linting forscripts/install.sh, and Codex review passed.Signed-off-by: Yimo Jiang yimoj@nvidia.com
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores