fix(cli): accept OpenShell Server endpoint output - #8758
Conversation
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@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:
📝 WalkthroughWalkthroughThe change strengthens managed gateway endpoint classification, scopes gateway status probes, preserves stale reuse state on connection failures, and expands onboarding coverage for endpoint reporting and healthy gateway reuse. ChangesGateway readiness and onboarding reuse
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 04228c9 in the TypeScript / code-coverage/cliThe overall coverage in commit 04228c9 in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/readiness/gateway-production.ts`:
- Around line 308-311: Update the endpoint parsing loop around
stripAnsi(output).matchAll in src/lib/readiness/gateway-production.ts#L308-L311
to match labeled lines with empty values and pass an empty fallback to new
URL(), so empty or trailing-data values return "mismatch" rather than "unknown".
Add empty-value and trailing-data cases expecting "mismatch" in
src/lib/readiness/gateway-production.test.ts#L293-L301.
In `@test/e2e/live/double-onboard.test.ts`:
- Around line 565-567: Update the gateway endpoint assertion around new URL so
an empty URL.port is normalized to the protocol’s default port before
comparison. Treat HTTP as 80 and HTTPS as 443, while preserving the configured
NEMOCLAW_GATEWAY_PORT or 8080 fallback for non-default ports.
In `@test/onboard-gateway-port-conflict-fast-fail.test.ts`:
- Around line 216-224: In the result-validation loop for the two onboarding
invocations, add an assertion that each child process exits successfully with
status 0, alongside the existing result.error and result.signal checks. Keep the
output assertions unchanged.
🪄 Autofix
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: cb5b326f-1094-4f60-880a-9d30eeebae70
📒 Files selected for processing (4)
src/lib/readiness/gateway-production.test.tssrc/lib/readiness/gateway-production.tstest/e2e/live/double-onboard.test.tstest/onboard-gateway-port-conflict-fast-fail.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
1 terminology difference from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
2 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. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
Holding approval for the three unresolved CodeRabbit findings on the current head:
The installer failure passed on rerun; CLI shard 3 is still running. Once these findings are addressed and required checks are green, this should be ready for approval. |
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/readiness/gateway-production.test.ts`:
- Around line 392-400: Update the subprocess.spawnSync mock implementation in
the readiness test to match the exact production openshell command forms,
including the expected arguments for both status and info invocations. Return
the appropriate connection-refused result only for those exact forms, and make
every other command or argument combination fail closed rather than returning a
successful default result.
- Around line 392-400: Reset the standalone mock implementation after each test
by adding subprocess.spawnSync.mockReset() to the file’s existing afterEach
cleanup. Keep the current vi.restoreAllMocks() behavior and ensure the reset
targets the subprocess.spawnSync mock used by the test setup.
🪄 Autofix
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: 576f8b39-6f49-40db-946e-1386e8f11637
📒 Files selected for processing (3)
src/lib/readiness/gateway-production.test.tssrc/lib/readiness/gateway-production.tstest/onboard-gateway-port-conflict-fast-fail.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/onboard-gateway-port-conflict-fast-fail.test.ts
- src/lib/readiness/gateway-production.ts
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
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/onboard-gateway-port-conflict-fast-fail.test.ts`:
- Around line 199-209: Add or retain an integration test that invokes the public
onboarding flow twice and verifies both calls succeed by reusing the gateway
without a port-conflict failure. Keep the existing observeManagedGateway
readiness assertions as lower-level coverage, but ensure the onboarding test
validates the observable repeated-invocation behavior rather than only readiness
state.
🪄 Autofix
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: 5a75806a-4ed9-412b-8b35-8515043b1053
📒 Files selected for processing (2)
test/e2e/live/double-onboard.test.tstest/onboard-gateway-port-conflict-fast-fail.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/live/double-onboard.test.ts
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
senthilr-nv
left a comment
senthilr-nv
left a comment
There was a problem hiding this comment.
Approved — review findings are addressed with focused regression coverage.
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
3 participants
Summary
OpenShell v0.0.101 reports its active gateway endpoint as
Server:, while NemoClaw readiness accepted onlyGateway endpoint:. This companion to #8757 accepts both producer labels, requires every recognized HTTP(S) loopback endpoint to agree with the expected port, and restores reuse of a healthy managed gateway.The first trusted PR E2E run also exposed a registered-but-stopped gateway state: OpenShell returned a connection error without repeating the gateway name, and public readiness classified the selected gateway as unknown instead of stale. The revised collector scopes
statusto the exact gateway and carries that authority into the existing stale-state classifier.The escaped regression had two detection gaps: the parser unit fixture modeled only the older label, and the live double-onboard target did not explicitly assert the producer label or the reuse outcome.
Changes
Gateway endpoint:andServer:labels, failing closed on empty, malformed, off-host, off-port, non-HTTP(S), trailing, or conflicting evidence.double-onboardtarget to prove the first invocation starts the gateway and the second reuses the same runtime.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededServer:endpoint output, scopes status probes to the resolved gateway, and preserves a registered gateway asstaleafter a connection error. It does not change a NemoClaw command, flag, configuration, default, output, or supported workflow.docs/reference/system-readiness.mdxalready documents canonical gateway ownership, stale reuse state, and loopback/port endpoint binding.docs/reference/commands.mdxalready documents existing-gateway reuse and stale reconciliation.docs/changelog/2026-08-10.mdxalready covers v0.0.106 readiness with OpenShell v0.0.101.DGX Station Hardware Evidence
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 unavailablenpm exec -- vitest run --project cli src/lib/readiness/gateway-production.test.ts— 51/51 passednpm exec -- vitest run --project integration test/onboard-gateway-port-conflict-fast-fail.test.ts— 2/2 passednpm run typecheck:cli— passednpm run checks:repository— passednpm run test:e2e-phases:check— passed across 125 tests in 81 filesnpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: not applicable; focused validation covers the changed readiness and E2E contracts.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit
Bug Fixes
Reliability