fix(onboard): gate the legacy gateway destroy verb on installed OpenShell support - #8270
fix(onboard): gate the legacy gateway destroy verb on installed OpenShell support#8270AzeelSajjad wants to merge 8 commits into
Conversation
…8139) Pass an explicit supportsLifecycleCommands: () => false override in the pre-existing production-wired test so it no longer falls through to the real openshell binary probe, which can process.exit(1) the vitest worker when no openshell binary is resolvable. Add a deps-level test in gateway-start-failure.test.ts that constructs the handler with supportsLifecycleCommands in deps and no per-call override, covering the deps ?? (() => false) fallthrough that was previously only reached incidentally.
…A#8139) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lag forward (NVIDIA#8139) Add the missing supportsLifecycleCommands override to the second handleFinalGatewayStartFailure test in test/gateway-final-failure-cleanup.test.ts: without it, the call falls through to the live gatewayCliSupportsLifecycleCommands probe, which calls process.exit(1) inside the vitest worker on a runner without an openshell binary (killing the rest of that worker's tests). Add a test asserting handleGatewayState forwards gatewayCliSupportsLifecycleCommands()'s result unchanged to reconcileGatewayGpuReuseForGpuIntent, guarding against a wrong (inverted or hardcoded) value compiling clean. Document why the abort-with-recovery supportsLifecycleCommands argument in gateway-gpu-passthrough.ts is dead but intentionally kept for uniformity with its sibling call.
📝 WalkthroughWalkthroughGateway cleanup hints now use ChangesGateway lifecycle hint handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLIProbe
participant GatewayHandler
participant Recovery
participant HintHelper
CLIProbe->>GatewayHandler: detect lifecycle-command support
GatewayHandler->>Recovery: pass supportsLifecycleCommands
Recovery->>HintHelper: request gateway removal hints
HintHelper-->>Recovery: return gateway remove and optional gateway destroy commands
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
3 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
4 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 2 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. |
There was a problem hiding this comment.
Reviewed exact head f853cb0.
Security review: PASS with no findings across secrets, input handling, authorization, dependencies, logging, cryptography, configuration, security testing, and lifecycle safety.
I cannot approve the architecture as written. Current main still limits src/lib/onboard to 310 root files and pins the supported OpenShell range to exactly 0.0.85, where gateway destroy is not a valid command. This PR raises the transitional root-file ratchet to 311, adds a helper to preserve pre-0.0.44 guidance, threads that compatibility flag through the onboarding FSM, and compresses an unrelated diagnostics call to stay within another budget. That is additional compatibility and dependency debt without a current supported consumer.
The accepted behavior can be implemented by retaining gateway remove and deleting the obsolete gateway destroy hint at the three affected sites. PR #8188 already provides that smaller independent implementation, so duplicating it here would not be a useful maintainer fix.
Validation on this head: 63 focused CLI tests passed, 3 focused integration tests passed, CLI typecheck passed, source-shape passed, git diff --check passed, and the independent documentation-writer verdict is no-docs-needed. Required GitHub checks are green except E2E / PR Gate, which is pending; the branch also trails current main.
Please either justify an accepted current consumer for the legacy compatibility path without raising the architecture ratchet, or prefer #8188 for the minimal supported-version fix.
|
Addressing the review feedback: I agree with the smaller supported-version fix. The configured OpenShell range has no current consumer for the legacy #8188 is approved and currently has two checks still running. I will close this PR as superseded only after #8188 merges, and the closing note will link the merged replacement. |
|
Closing as conclusively superseded by #8188, which has merged and implements the accepted scope for #8139: remove the obsolete |
3 participants
Summary
Three onboard failure hints printed
openshell gateway destroy -g <name>, a subcommand current OpenShell rejects witherror: unrecognized subcommand 'destroy'. Each printed it unconditionally behind a prose caveat an operator reading a failure hint has no way to evaluate. All three now render their command block through a shared helper gated ongatewayCliSupportsLifecycleCommands, the probe the gateway execution paths already consumed, so the printed commands match the installed OpenShell.Related Issue
Fixes #8139
Changes
src/lib/onboard/gateway-removal-hint.tsexportinggatewayRemovalHintLines(gatewayName, supportsLifecycleCommands). This is consolidation, not a new layer: it has three named current consumers, listed in its docblock, that previously duplicated the same three-line block.src/lib/onboard/gateway-removal-hint.test.tsprotects the contract in both directions.src/lib/onboard/gateway-start-failure.ts(gateway fails to start after all retries),src/lib/onboard/gpu-recovery.ts(GPU passthrough mismatch, no sandboxes registered), andsrc/lib/onboard/gateway-gpu-passthrough.ts(GPU passthrough mismatch, registry unreadable).supportsLifecycleCommandsfrom the probe.src/lib/onboard/machine/handlers/gateway.tsalready computed it for its own use and now forwards it to the reconcile call;src/lib/onboard.tswires it into the start-failure handler. No new probe call is introduced.falseat every layer, so a caller that has not run the probe prints only the verb current OpenShell accepts. The field onGatewayGpuReuseReconcileOptionsis optional for the same reason: the other four construction sites keep compiling and passing untouched.src/lib/onboardinci/source-architecture-budget.jsonfrom 310 to 311 for the one new production file.Known remaining, out of scope:
scripts/install.sh:2428printsopenshell gateway remove X || openshell gateway destroy -g X. The||makes it self-correcting, so it cannot produce theunrecognized subcommandfailure this PR fixes.Type of Change
Quality Gates
docs/and found no page quotes these hint strings. This change alters which commands NemoClaw prints, never which it runs, so the gateway-teardown prose indocs/deployment/gateway-lifecycle-authority.mdxanddocs/reference/commands.mdxremains accurate.Documentation Writer Review
no-docs-neededdocs/forgateway destroy,gateway remove,lifecycle command, andunrecognized subcommand. No page quotes the onboard-hint strings from the changed files. Checked and confirmed still accurate:docs/reference/troubleshooting.mdxGPU-passthrough section (refers generically to "the commands printed by onboarding" without naming verbs);docs/reference/commands.mdxanddocs/deployment/gateway-lifecycle-authority.mdx:190(both describe execution behavior on separate, already-gated paths, not hint strings); two unrelatedopenshell gateway removesnippets indocs/reference/troubleshooting.mdxTLS/CoreDNS recovery that never carried the legacy verb. Review was rerun at headf853cb030after the growth-guardrail commit; that commit only reformats one line insrc/lib/onboard.tswith no behavior, string, or interface change.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/onboard/gateway-removal-hint.test.ts src/lib/onboard/gpu-recovery.test.ts src/lib/onboard/gateway-start-failure.test.ts src/lib/onboard/gateway-gpu-passthrough.test.ts src/lib/onboard/machine/handlers/gateway.test.ts→ 63/63 passing.npx vitest run --project integration test/gateway-final-failure-cleanup.test.ts→ 3/3 passing. Both rerun after rebasing onto currentorigin/main.npm run source-shape:check→ exit 0,source_shape_cases=0.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Azeel Sajjad aasajjad05@gmail.com
Summary by CodeRabbit
Bug Fixes
gateway removecommand.gateway destroyguidance now appears only when supported by the installed CLI.Tests