fix(onboard): treat refused gateway status as stale - #7169
Conversation
Supersede fork PR #7143 so protected onboarding E2E can run on a trusted branch. Co-authored-by: HwangJohn <angelic805@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@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:
📝 WalkthroughWalkthroughGateway status capture now includes stderr, enabling connection failures to classify as ChangesGateway reuse classification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Onboarding
participant GatewayReuseHelpers
participant OpenShell
participant GatewayState
Onboarding->>GatewayReuseHelpers: Request gateway reuse snapshot
GatewayReuseHelpers->>OpenShell: Capture status with stderr
OpenShell-->>GatewayReuseHelpers: Return status output
GatewayReuseHelpers->>GatewayState: Classify gateway status
GatewayState-->>Onboarding: Return stale or missing reuse state
Onboarding->>OpenShell: Start stale gateway
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit fbe7af2 in the TypeScript / code-coverage/cliThe overall coverage in commit fbe7af2 in the Show a code coverage summary of the most impacted files.
Updated |
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/state/gateway.ts`:
- Around line 128-145: Update the connection-lifecycle detection logic around
the status classification to match every connection-related phrase, including
“Connection refused” and the existing alternatives, against statusError rather
than clean output, while requiring statusError to be non-empty. Preserve the
current transport/reset/aborted/closed checks and add the regression test
covering a healthy status whose diagnostic mentions “Connection refused” so it
is not marked stale.
🪄 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: 36e62a3a-0e51-4f57-86ef-aabbd1679cf4
📒 Files selected for processing (7)
src/lib/onboard/gateway-reuse.test.tssrc/lib/onboard/gateway-reuse.tssrc/lib/onboard/machine/handlers/gateway.test.tssrc/lib/runner-argv.test.tssrc/lib/runner.tssrc/lib/state/gateway.tstest/gateway-state.test.ts
PR Review Advisor — InformationalAdvisor assessment: Informational / medium confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Prevent healthy diagnostic text from forcing a gateway restart. Co-authored-by: HwangJohn <angelic805@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Exact-head maintainer audit on d8ac50a: opt-in stderr capture is stripped from spawn options, remains stdout-only by default, and is used only by the bounded gateway status probe. Stale classification requires both selected-gateway identity and a concrete connection failure on an actual status-error line; auth, configuration, TLS, CLI, and healthy diagnostic text remain non-destructive. Focused CLI tests pass 53/53, integration tests pass 130/130, CLI type-check and diff check pass, both commits are GitHub Verified with DCO and HwangJohn credit, all CI/advisors/protected E2E are green, and no review thread remains unresolved. No blocking security or correctness issue found. Independent review requested from @cv because the PR is authored by the current maintainer account. |
|
Independent comparator audit confirms this is the canonical #7087 implementation over #7143. Exact head |
Co-authored-by: HwangJohn <angelic805@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Current-main refresh is now on exact head
Fresh exact-head CI and automated review are running. No release label was changed. |
Suppress failed command output by default while retaining explicit combined stream capture. Co-authored-by: HwangJohn <angelic805@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Pushed
Validation passed:
Fresh exact-head CI and advisor checks are now running. |
Record the pinned OpenShell output boundary and its removal condition. Classify refusal and authentication-error fixtures without behavior changes. Co-authored-by: HwangJohn <angelic805@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Addressed the exact-head advisor PRA-1 in Verified commit ba440cd.
Validation: gateway-state integration tests 67/67 passed; CLI typecheck and scoped hooks passed. This follow-up changes no runtime behavior. HwangJohn remains credited as co-author. |
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 `@test/fixtures/openshell-status-errors-v0.0.85.json`:
- Around line 1-7: Add the repository’s standard SPDX text as a top-level
$comment field in the JSON fixture, placing it alongside the existing metadata
fields while preserving valid JSON and all current fixture values.
🪄 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: 0cdb76cc-6e6b-47b9-a1d5-d59e55d4ed6d
📒 Files selected for processing (3)
src/lib/state/gateway.tstest/fixtures/openshell-status-errors-v0.0.85.jsontest/gateway-state.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/state/gateway.ts
Co-authored-by: HwangJohn <angelic805@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: HwangJohn <angelic805@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Addressed the canonical advisor warning in signed commit |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
ericksoa
left a comment
There was a problem hiding this comment.
Approved at exact head 1c09d95 after full diff and nine-category security review. Targeted CLI and integration suites, build/typecheck/check:diff, all current CI checks, selected onboard-repair/onboard-resume E2E, advisor ledger, resolved review threads, DCO, commit verification, and current-base checks are green.
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Summary
Re-publishes the reviewed #7143 change set on a trusted NVIDIA branch so the required protected E2E jobs can execute. It captures stderr from failed OpenShell status probes and distinguishes concrete connection failures from authentication, configuration, TLS, and CLI errors before deciding that a gateway is stale. This supersedes #7143 solely because the fork security boundary prevented its required E2E jobs from running.
Related Issue
Fixes #7087
Changes
runner.runCapturestdout-only by default while allowing the bounded gateway status probe to opt into stderr capture.Co-authored-bytrailers, including the current-main sync commit.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededdocs/paths need changing. The current-main topper leaves the effective eight-file product patch byte-identical to the independently reviewed patch (SHA-2566accec058acd136fc90f773c40473b669e4e03de8734e7ee4d571590fc4ee2b0).Verification
Signed-off-by:line and every published commit appears asVerifiedin GitHubpre-commit,commit-msg, and pre-push hooks passed, or equivalent targeted validation passed when hooks were unavailable280efd274:npx vitest run --project cli src/lib/onboard/gateway-reuse.test.ts src/lib/onboard/machine/handlers/gateway.test.ts src/lib/runner-argv.test.ts(61 tests passed);npx vitest run --project integration test/gateway-state.test.ts(67 tests passed)npm run build:cli;npm run typecheck:clinpm run docsbuilds without warnings (doc changes only)Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit