fix: arm64 gateway health detection fails on non-TTY environments - #1723
Merged
Conversation
NVIDIA#1711) Three changes that combine to fix the ARM64 onboard loop: 1. isGatewayConnected(): also match 'Server Status' (OpenShell 0.0.25+) 2. isGatewayHealthy(): accept gateway info as proof of health when openshell status returns empty (ARM64/non-TTY fallback path) 3. onboard health poll: add gateway select before each probe, increase defaults on ARM64 (30×10s vs 5×2s) to account for slow k3s init All values remain overridable via NEMOCLAW_HEALTH_POLL_COUNT and NEMOCLAW_HEALTH_POLL_INTERVAL environment variables.
Contributor
|
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:
📝 WalkthroughWalkthroughUpdated gateway health detection and polling: status parsing now recognizes "Server Status", health evaluation accepts gateway-info as a fallback when status is empty, ARM64 polling defaults increased, and a gateway select is executed before each health probe. Changes
Sequence Diagram(s)sequenceDiagram
participant Onboard as Onboard Process
participant CLI as OpenShell CLI
participant GW as Gateway Info Provider
Onboard->>CLI: run "gateway select <GATEWAY_NAME>" (ignore errors)
Onboard->>CLI: run "status"
CLI-->>Onboard: statusOutput (may be empty / contain "Connected" or "Server Status")
Onboard->>CLI: run "gateway info" and "gateway info -g"
CLI-->>GW: gwInfoOutput, activeGatewayInfoOutput
Onboard->>Onboard: isGatewayConnected(statusOutput)
Onboard->>Onboard: getReportedGatewayName(...), hasStaleGateway(...), hasActiveGatewayInfo(...)
Onboard-->>Onboard: determine healthy if (status indicates connected and name matches) OR (status empty but gw-info confirms name+endpoint)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/gateway-state.test.ts (1)
77-93: Add a regression case for non-empty, non-connected status.Line 77-93 should also assert that fallback does not mark healthy when status is present but explicitly disconnected. This will lock in the intended “empty-status-only” fallback behavior.
✅ Suggested test addition
describe("isGatewayHealthy", () => { + it("returns false when status is present but disconnected", () => { + const statusDisconnected = ` +Server Status + +Gateway: nemoclaw +Status: Disconnected +`; + expect(isGatewayHealthy(statusDisconnected, GW_INFO_NAMED, GW_INFO_ACTIVE)).toBe(false); + }); + it("returns true when status shows Connected and gateway name matches", () => { expect(isGatewayHealthy(STATUS_CONNECTED, GW_INFO_NAMED, GW_INFO_ACTIVE)).toBe(true); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gateway-state.test.ts` around lines 77 - 93, Add a regression test ensuring that a non-empty status string that indicates "disconnected" does not trigger the fallback healthy result: update the test suite around the existing cases for isGatewayHealthy by adding a case that passes a non-empty disconnected status (instead of ""), with GW_INFO_NAMED and GW_INFO_ACTIVE (or similar GW_INFO_* fixtures), and assert isGatewayHealthy(disconnectedStatus, GW_INFO_NAMED, GW_INFO_ACTIVE) returns false so only an empty status uses the fallback logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/gateway-state.ts`:
- Around line 94-97: The fallback health check currently returns true when
namedGatewayKnown && activeInfo && activeGatewayName === GATEWAY_NAME even if
the status output is non-empty; change the conditional to only apply the
fallback when the status output is empty/blank (e.g., add a check like !status
|| status.trim() === ''), so update the if that contains namedGatewayKnown,
activeInfo and activeGatewayName === GATEWAY_NAME to include the emptiness check
for status before returning true.
---
Nitpick comments:
In `@test/gateway-state.test.ts`:
- Around line 77-93: Add a regression test ensuring that a non-empty status
string that indicates "disconnected" does not trigger the fallback healthy
result: update the test suite around the existing cases for isGatewayHealthy by
adding a case that passes a non-empty disconnected status (instead of ""), with
GW_INFO_NAMED and GW_INFO_ACTIVE (or similar GW_INFO_* fixtures), and assert
isGatewayHealthy(disconnectedStatus, GW_INFO_NAMED, GW_INFO_ACTIVE) returns
false so only an empty status uses the fallback logic.
🪄 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: Pro
Run ID: adcd16f8-ccbd-4aca-bdf5-2899f0b208d8
📒 Files selected for processing (3)
src/lib/gateway-state.tssrc/lib/onboard.tstest/gateway-state.test.ts
Contributor
Author
|
@coderabbitai Good catch — the fallback was indeed too broad. Fixed in 3b0dada: added a |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1923
Everything looks good. ✅ |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/gateway-state.test.ts (2)
13-15: Unused imports:hasStaleGateway,hasActiveGatewayInfo,getReportedGatewayName.These helper functions are imported but not directly tested. They're exercised indirectly through
isGatewayHealthyandgetGatewayReuseState. Consider either removing the unused imports or adding dedicated test cases for these utility functions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gateway-state.test.ts` around lines 13 - 15, The imports for hasStaleGateway, hasActiveGatewayInfo, and getReportedGatewayName are unused in this test file; either remove these three imports from the import list in test/gateway-state.test.ts or add focused unit tests that call hasStaleGateway, hasActiveGatewayInfo, and getReportedGatewayName directly (asserting expected outputs for representative inputs) so they are covered; locate the import statement that includes these symbols and either delete the three names or add new test cases that reference those exact function names.
102-114: Consider adding tests for remainingGatewayReuseStatevalues.The current tests cover
"healthy"and"missing"states well, including the ARM64 fallback path. For more complete coverage, consider adding tests for the"active-unnamed","foreign-active", and"stale"states in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gateway-state.test.ts` around lines 102 - 114, Add unit tests covering the remaining GatewayReuseState values by adding new it() cases that call getGatewayReuseState with inputs that produce "active-unnamed", "foreign-active", and "stale"; for example, exercise the code paths using combinations of GW_INFO_NAMED, GW_INFO_ACTIVE, STATUS_CONNECTED and empty or mismatched gateway info strings to assert getGatewayReuseState returns "active-unnamed", "foreign-active", and "stale" respectively (keep test names descriptive and follow the existing describe("getGatewayReuseState") pattern).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/gateway-state.test.ts`:
- Around line 13-15: The imports for hasStaleGateway, hasActiveGatewayInfo, and
getReportedGatewayName are unused in this test file; either remove these three
imports from the import list in test/gateway-state.test.ts or add focused unit
tests that call hasStaleGateway, hasActiveGatewayInfo, and
getReportedGatewayName directly (asserting expected outputs for representative
inputs) so they are covered; locate the import statement that includes these
symbols and either delete the three names or add new test cases that reference
those exact function names.
- Around line 102-114: Add unit tests covering the remaining GatewayReuseState
values by adding new it() cases that call getGatewayReuseState with inputs that
produce "active-unnamed", "foreign-active", and "stale"; for example, exercise
the code paths using combinations of GW_INFO_NAMED, GW_INFO_ACTIVE,
STATUS_CONNECTED and empty or mismatched gateway info strings to assert
getGatewayReuseState returns "active-unnamed", "foreign-active", and "stale"
respectively (keep test names descriptive and follow the existing
describe("getGatewayReuseState") pattern).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a2d2d22a-a3e9-40a3-ae14-8847d1552978
📒 Files selected for processing (2)
src/lib/gateway-state.tstest/gateway-state.test.ts
Contributor
|
✨ Thanks for submitting this PR, which proposes a fix for a bug with gateway health detection on ARM64 and may improve the overall reliability of the onboard process. Possibly related open issues: |
- Add regression test for Disconnected status (non-empty status must not trigger ARM64 fallback healthy path) - Add dedicated unit tests for hasStaleGateway, hasActiveGatewayInfo, and getReportedGatewayName (previously imported but only used indirectly) - Add tests for remaining GatewayReuseState values: foreign-active, stale, active-unnamed Signed-off-by: kagura-agent <kagura@openclaw.ai>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/gateway-state.test.ts (2)
34-46: Deduplicate identical gateway-info fixtures to reduce drift.
GW_INFO_NAMEDandGW_INFO_ACTIVEare identical today; sharing one base fixture makes future fixture edits safer.Diff proposal
-const GW_INFO_NAMED = ` +const GW_INFO_NAMED_AND_ACTIVE = ` Gateway Info Gateway: nemoclaw Gateway endpoint: https://127.0.0.1:8080/ `; -const GW_INFO_ACTIVE = ` -Gateway Info - -Gateway: nemoclaw -Gateway endpoint: https://127.0.0.1:8080/ -`; +const GW_INFO_NAMED = GW_INFO_NAMED_AND_ACTIVE; +const GW_INFO_ACTIVE = GW_INFO_NAMED_AND_ACTIVE;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gateway-state.test.ts` around lines 34 - 46, GW_INFO_NAMED and GW_INFO_ACTIVE are identical fixtures which can diverge accidentally; replace the duplicate strings with a single shared fixture (e.g., GW_INFO or BASE_GW_INFO) and have both GW_INFO_NAMED and GW_INFO_ACTIVE reference that single value (or remove one and reuse the other where referenced) so future edits are applied in one place; update any tests importing/using GW_INFO_NAMED/GW_INFO_ACTIVE to use the shared symbol.
152-179: Add one ANSI-only status regression test for fallback eligibility.Given the status-empty guard strips ANSI before trim, a focused ANSI-only input test would better pin the ARM64/non-TTY edge case.
Diff proposal
describe("isGatewayHealthy", () => { @@ it("returns true via fallback when status is empty but gateway info confirms health (`#1711`)", () => { // ARM64 / non-TTY: openshell status returns "" expect(isGatewayHealthy("", GW_INFO_NAMED, GW_INFO_ACTIVE)).toBe(true); }); + + it("returns true via fallback when status is ANSI-only", () => { + const ansiOnlyStatus = "\u001b[2K\u001b[0m"; + expect(isGatewayHealthy(ansiOnlyStatus, GW_INFO_NAMED, GW_INFO_ACTIVE)).toBe(true); + }); @@ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gateway-state.test.ts` around lines 152 - 179, Add a test that verifies ANSI-only status triggers the fallback: call isGatewayHealthy with a status string containing only ANSI escape sequences (e.g., green/reset codes) and GW_INFO_NAMED and GW_INFO_ACTIVE as the other args, and assert it returns true; place it near the other cases and name it like "returns true via fallback when status is ANSI-only" to ensure the ANSI stripping + empty check allows fallback, referencing isGatewayHealthy, GW_INFO_NAMED and GW_INFO_ACTIVE to locate the test area.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/gateway-state.test.ts`:
- Around line 34-46: GW_INFO_NAMED and GW_INFO_ACTIVE are identical fixtures
which can diverge accidentally; replace the duplicate strings with a single
shared fixture (e.g., GW_INFO or BASE_GW_INFO) and have both GW_INFO_NAMED and
GW_INFO_ACTIVE reference that single value (or remove one and reuse the other
where referenced) so future edits are applied in one place; update any tests
importing/using GW_INFO_NAMED/GW_INFO_ACTIVE to use the shared symbol.
- Around line 152-179: Add a test that verifies ANSI-only status triggers the
fallback: call isGatewayHealthy with a status string containing only ANSI escape
sequences (e.g., green/reset codes) and GW_INFO_NAMED and GW_INFO_ACTIVE as the
other args, and assert it returns true; place it near the other cases and name
it like "returns true via fallback when status is ANSI-only" to ensure the ANSI
stripping + empty check allows fallback, referencing isGatewayHealthy,
GW_INFO_NAMED and GW_INFO_ACTIVE to locate the test area.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ea57d7d-bbc5-4f6a-9e20-13f44c0abe75
📒 Files selected for processing (1)
test/gateway-state.test.ts
Address CodeRabbit review suggestions: - Share one base fixture for GW_INFO_NAMED/GW_INFO_ACTIVE - Add ANSI-escape-only status test (fallback triggers after stripping)
Contributor
Author
|
Addressed the review suggestions in 12270e2:
All 32 gateway-state tests pass. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/gateway-state.test.ts (1)
19-213: Consider centralizing the"nemoclaw"literal into one test constant.The gateway name is repeated across multiple fixtures/assertions. A single constant would reduce maintenance drift if the default name changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gateway-state.test.ts` around lines 19 - 213, Introduce a single test constant for the gateway name (e.g., GATEWAY_NAME = "nemoclaw") and replace all hard-coded "nemoclaw" literals in the fixtures and expectations (STATUS_CONNECTED, STATUS_SERVER_STATUS_ONLY, GW_INFO_BASE, GW_INFO_NAMED, GW_INFO_ACTIVE, GW_INFO_UNNAMED_ENDPOINT, STATUS_FOREIGN, and any uses of replace that change "nemoclaw") to reference that constant; ensure tests that build altered names (like other-gw) still work by replacing GATEWAY_NAME when composing those variants and update references in tests that call hasStaleGateway, getReportedGatewayName, isGatewayHealthy, and getGatewayReuseState so they use the new GATEWAY_NAME constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/gateway-state.test.ts`:
- Around line 19-213: Introduce a single test constant for the gateway name
(e.g., GATEWAY_NAME = "nemoclaw") and replace all hard-coded "nemoclaw" literals
in the fixtures and expectations (STATUS_CONNECTED, STATUS_SERVER_STATUS_ONLY,
GW_INFO_BASE, GW_INFO_NAMED, GW_INFO_ACTIVE, GW_INFO_UNNAMED_ENDPOINT,
STATUS_FOREIGN, and any uses of replace that change "nemoclaw") to reference
that constant; ensure tests that build altered names (like other-gw) still work
by replacing GATEWAY_NAME when composing those variants and update references in
tests that call hasStaleGateway, getReportedGatewayName, isGatewayHealthy, and
getGatewayReuseState so they use the new GATEWAY_NAME constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: da709aa8-930e-4736-b739-782f419771d0
📒 Files selected for processing (1)
test/gateway-state.test.ts
kagura-agent
changed the title
When status reports Connected with Gateway: nemoclaw, isGatewayHealthy returns true via the primary path, so getGatewayReuseState correctly returns 'healthy' not 'active-unnamed'. Signed-off-by: kagura-agent <kagura-agent@users.noreply.github.com>
prekshivyas
approved these changes
Apr 16, 2026
prekshivyas
left a comment
prekshivyas
left a comment
Collaborator
There was a problem hiding this comment.
Solid fix for ARM64 onboarding — fallback is properly guarded on empty status, assertion change is correct (Connected + matching name = healthy), tests are comprehensive. @kagura-agent can you rebase onto main to trigger CI?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1711.
nemoclaw onboardnever completes on ARM64 (Raspberry Pi 5) because the gateway health check always returns false, even though the gateway is fully functional.Root Cause
Three issues combine to break health detection on ARM64:
openshell statusreturns empty string in non-TTY contexts (Node.js child process on ARM64)isGatewayHealthy()hard-gates onisGatewayConnected(), which requires"Connected"in status output — no fallback when status is emptyisGatewayConnected()doesn't match"Server Status"(OpenShell 0.0.25+ changed output format)Changes
src/lib/gateway-state.tsisGatewayConnected(): also match"Server Status"(OpenShell 0.0.25+)isGatewayHealthy(): accept gateway info as proof of health when status is empty — if the named gateway exists (hasStaleGateway) and has an active endpoint (hasActiveGatewayInfo) and the name matches, treat as healthysrc/lib/onboard.tsgateway selectbefore each health probe (ensures correct gateway is active in non-TTY environments)NEMOCLAW_HEALTH_POLL_COUNTandNEMOCLAW_HEALTH_POLL_INTERVALtest/gateway-state.test.ts(new)isGatewayConnectedwithConnected,Server Status, empty, and undefined inputsisGatewayHealthyprimary path, ARM64 fallback path, and failure casesgetGatewayReuseStatehealthy detection via both pathsTesting
npx vitest run test/gateway-state.test.ts— 13/13 passnpx tsc -p tsconfig.src.json --noEmit— zero errorsSummary by CodeRabbit
Bug Fixes
Tests
Signed-off-by: kagura-agent kagura-agent@users.noreply.github.com