fix(onboard): treat refused gateway status as stale - #7143
Conversation
Signed-off-by: HwangJohn <angelic805@gmail.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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds optional stderr capture to ChangesGateway reuse failure handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Onboarding
participant GatewayReuse
participant runCapture
participant OpenShell
Onboarding->>GatewayReuse: inspect gateway state
GatewayReuse->>runCapture: run status with includeStderr
runCapture->>OpenShell: execute status probe
OpenShell-->>runCapture: stdout and stderr
runCapture-->>GatewayReuse: combined status output
GatewayReuse-->>Onboarding: stale reuse state
Onboarding->>OpenShell: start gateway
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 105-117: Update hasGatewayConnectionError to remove generic Error
and client error matches, retaining only connection-specific indicators such as
transport error, Connection refused, Connection reset, and No active gateway.
Ensure isGatewayConnected and getGatewayReuseState no longer classify unrelated
authentication, configuration, or CLI failures as stale, and add a regression
case covering a non-connection error.
🪄 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: 8a74aa9e-bd5f-4ce5-b45f-e7014fbb28c4
📒 Files selected for processing (6)
src/lib/onboard/gateway-reuse.test.tssrc/lib/onboard/gateway-reuse.tssrc/lib/runner-argv.test.tssrc/lib/runner.tssrc/lib/state/gateway.tstest/gateway-state.test.ts
PR Review Advisor — InformationalAdvisor assessment: Informational / high 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
1 warning · 0 suggestionsWarningsWarnings do not block.
|
Signed-off-by: HwangJohn <angelic805@gmail.com>
Signed-off-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>
Co-authored-by: HwangJohn <angelic805@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Approved at exact head 16916f4 after independent correctness/security review. The opt-in stderr capture is stripped before argv-only process execution and is scoped to the bounded OpenShell status probe; stale classification is limited to concrete connection failures while auth/config/TLS/CLI errors fail safe without destructive cleanup. Focused coverage includes default/opt-in stderr behavior, false positives, status variants, snapshot propagation, and handler transition. CodeRabbit is resolved, commits are Verified/DCO-compliant, and original-author credit is preserved. Non-blocking follow-up: document the compatibility branch source boundary and retirement condition.
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Blocking correctness issue at exact head 16916f443: hasGatewayConnectionError scans the entire cleaned status transcript for Connection refused and the other connection phrases. A healthy status containing informational history such as Previous diagnostic: Connection refused is therefore classified stale and can trigger destructive gateway cleanup.
Please scope all connection-failure phrases to the parsed status-error suffix and add the paired error-vs-informational regression. The credited replacement #7169 already implements that narrower behavior, includes the missing regression, and is green; it should remain the canonical path for #7087.
|
Closing as superseded by #7169. The replacement preserves HwangJohn's authorship with a Co-authored-by trailer, covers the same seven-file scope, and adds the missing negative regression so historical 'Connection refused' text is not mistaken for a current gateway failure. #7169 is green and now only awaits independent human review. |
<!-- markdownlint-disable MD041 --> ## 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 - Keep `runner.runCapture` stdout-only by default while allowing the bounded gateway status probe to opt into stderr capture. - Mark the selected gateway stale only for concrete connection failures and avoid destructive cleanup for unrelated status errors. - Preserve the reviewed handler transition and add regression coverage for stderr capture, false positives, status variants, snapshot propagation, and gateway restart behavior. - Preserve HwangJohn's original contribution through `Co-authored-by` trailers, including the current-main sync commit. ## 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 <!-- Check one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: no user-facing command, flag, configuration, output format, or procedure changes; this only fixes internal gateway reuse classification. - [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: exact reviewed change set approved on #7143 (review); the replacement preserves that net diff while syncing current main mechanically. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: The change corrects internal gateway-state classification and matches the existing documented onboarding behavior. It adds no command, flag, configuration, output format, or user procedure; no `docs/` paths need changing. The current-main topper leaves the effective eight-file product patch byte-identical to the independently reviewed patch (SHA-256 `6accec058acd136fc90f773c40473b669e4e03de8734e7ee4d571590fc4ee2b0`). - Agent: Codex Desktop <!-- docs-review-head-sha: fbe7af2 --> <!-- docs-review-agents-blob-sha: 9c9b36d --> ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes a `Signed-off-by:` line and every published commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and pre-push hooks passed, or equivalent targeted validation passed when hooks were unavailable - [x] Targeted behavior tests pass for the current change set — exact head `280efd274`: `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) - [x] CLI build and type-check pass — `npm run build:cli`; `npm run typecheck:cli` - [ ] Applicable broad gate passed — not applicable; the focused runner and onboarding behavior is covered by the targeted CLI and integration suites above. - [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) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved gateway reuse detection by capturing and interpreting both stdout and stderr from OpenShell status (including connection refusals and authentication-related failures). * Gateways with connection-related failures are now classified as **stale** and restarted when appropriate. * Gateways with non-connection status errors are now classified as **missing** instead of **stale**. * Refined gateway “connected”/“disconnected” detection using updated status-error patterns (OpenShell v0.0.85). * **Tests** * Expanded snapshot, scenario, and fixture coverage for stderr-inclusive output and revised stale/missing classification behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: HwangJohn <angelic805@gmail.com> Co-authored-by: Aaron Erickson <aerickson@nvidia.com>
2 participants
Summary
Fixes stale NemoClaw gateway reuse after the selected OpenShell gateway process is gone but
openshell statusstill reportsGateway: nemoclawon stderr. Before this change, onboarding could miss that stderr-only refusal and treat the gateway as missing; now it classifies the selected refused gateway as stale and recreates the Docker-driver gateway before sandbox recreation.Related Issue
Fixes #7087
Changes
openshell statusstderr during gateway reuse probing so ignored OpenShell probe failures still provide diagnostic state.runner.runCaptureto honor explicitincludeStderr: truewhile preserving the existing default stdout-only behavior.Gateway: nemoclawplus connection-refused/transport-error status output as a stale gateway even whengateway infois unavailable.Type of Change
Quality Gates
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpm run check:diffpassed on the pushed fork commitnpm run build:clipassednpx vitest run --project cli src/lib/runner-argv.test.ts src/lib/onboard/gateway-reuse.test.tspassed, 35 testsnpx vitest run --project integration test/gateway-state.test.ts test/runner-basic.test.ts test/runner.test.tspassed, 119 testsgatewayReuseState: "stale";onboard --recreate-sandboxrestarted the Docker-driver gateway, recreated sandboxcodex-7087-a, and did not reportPort 8080 is not availablenpx vitest run --project cli src/lib/runner-argv.test.ts -t "stderr"passed, 2 testsnpx vitest run --project cli src/lib/onboard/gateway-reuse.test.tspassed, 2 testsnpx vitest run --project integration test/gateway-state.test.tspassed, 55 testsnpm 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: HwangJohn angelic805@gmail.com
Summary by CodeRabbit
Bug Fixes
Tests
#7087).