refactor(cli): extract dashboard delivery chain into contract/health/recover modules#2398
Merged
jyaunches merged 1 commit intoNVIDIA:mainfrom Apr 24, 2026
Conversation
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:
📝 WalkthroughWalkthroughIntroduces a Dashboard Delivery Contract plus health verification and recovery modules, centralizing dashboard port/URL/CORS/forwarding logic, adding end-to-end chain checks and idempotent recovery, and migrating consumers (onboard, nemoclaw) to use the new contract while removing the old dashboard helper module. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI / Client
participant Contract as DashboardContract
participant Health as DashboardHealth
participant Recover as DashboardRecover
participant Sandbox as Sandbox (openshell)
participant Config as SandboxConfig
CLI->>Contract: buildChain(hints)
Contract-->>CLI: DashboardDeliveryChain { accessUrl, port, forwardTarget, bindAddress, corsOrigins, healthEndpoint }
CLI->>Health: verifyDashboardChain(name, chain, deps)
rect rgba(100, 150, 200, 0.5)
Health->>Sandbox: executeSandboxCommand("curl http://127.0.0.1:port/health")
Sandbox-->>Health: HTTP status (200/401/000/other)
Health->>Health: gateway link status
end
rect rgba(150, 100, 200, 0.5)
Health->>Sandbox: captureForwardList()
Sandbox-->>Health: forward list text
Health->>Health: forward link status
end
rect rgba(200, 150, 100, 0.5)
Health->>Config: downloadSandboxConfig(name)
Config-->>Health: gateway.controlUi.allowedOrigins
Health->>Health: cors link status
end
Health-->>CLI: ChainStatus { healthy, links, diagnosis }
alt not healthy
CLI->>Recover: recoverDashboardChain(name, chain, deps)
opt gateway failure
Recover->>Recover: getSessionAgent(name)
Recover->>Sandbox: restartGateway(name, port, agent)
Sandbox-->>Recover: restart result
end
opt forward failure
Recover->>Sandbox: stopForward(port)
Recover->>Sandbox: startForward(target, name)
end
opt cors mismatch
Recover-->>CLI: append CORS diagnostic (no auto-fix)
end
Recover->>Health: verifyDashboardChain(name, chain, deps)
Health-->>Recover: ChainStatus (after)
Recover-->>CLI: RecoverResult { attempted, before, after, actions }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/onboard.ts (1)
6149-6170:⚠️ Potential issue | 🟠 MajorWSL dashboard URLs regress to localhost-only here.
On Line 6150,
buildChain({ chatUiUrl, isWsl: isWsl() })does not provide thewslHostAddresshint thatsrc/lib/dashboard-contract.ts:89-106needs to derive a Windows-reachable URL whenCHAT_UI_URLis still loopback. On Line 6151,contractGetDashboardAccessInfo()is also called without that hint, so theVS Code/WSLentry fromsrc/lib/dashboard-contract.ts:187-201is never added. The same issue leaks into the agent path on Line 6169 because it still builds URLs from rawchatUiUrlinstead of the resolvedchain.accessUrl. Please thread the resolved WSL host address through this path so WSL users still get a usable dashboard URL after the refactor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 6149 - 6170, The WSL host hint isn't being passed through after the refactor so dashboard URLs fall back to localhost; update the call sequence around buildChain/contractGetDashboardAccessInfo/contractGetDashboardGuidanceLines and the agent path to use the resolved chain access URL (or wslHostAddress) instead of raw chatUiUrl. Concretely: ensure buildChain(...) returns (or is called with) the resolved WSL host hint (wslHostAddress or accessUrl), then pass that resolved value into contractGetDashboardAccessInfo(chain, token) and contractGetDashboardGuidanceLines(chain, dashboardAccess, ...), and when calling agentOnboard.printDashboardUi(...) and its buildControlUiUrls callback, use chain.accessUrl (or the resolved wslHostAddress) to construct Windows-reachable control/dashboard URLs rather than the original chatUiUrl.
🧹 Nitpick comments (2)
specs/2026-04-23_dashboard-delivery-contract/validation.md (1)
239-248: SPDX validation step is incomplete for repository policy.The check currently validates only one SPDX line. It should validate both copyright and license lines.
Proposed fix
-**Then**: Each starts with `// SPDX-FileCopyrightText:` +**Then**: Each file includes both SPDX lines (`SPDX-FileCopyrightText` and `SPDX-License-Identifier`) -1. **Execute**: Bash: `for f in src/lib/dashboard-contract.ts src/lib/dashboard-health.ts src/lib/dashboard-recover.ts src/lib/dashboard-contract.test.ts src/lib/dashboard-health.test.ts src/lib/dashboard-recover.test.ts; do head -1 "$f" | grep -q "SPDX" && echo "$f: OK" || echo "$f: MISSING SPDX"; done` +1. **Execute**: Bash: `for f in src/lib/dashboard-contract.ts src/lib/dashboard-health.ts src/lib/dashboard-recover.ts src/lib/dashboard-contract.test.ts src/lib/dashboard-health.test.ts src/lib/dashboard-recover.test.ts specs/2026-04-23_dashboard-delivery-contract/validation.md; do grep -q "SPDX-FileCopyrightText" "$f" && grep -q "SPDX-License-Identifier" "$f" && echo "$f: OK" || echo "$f: MISSING SPDX"; done`As per coding guidelines
**/*.{js,mjs,ts,tsx,sh,md}: Every source file must include both SPDX header lines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/2026-04-23_dashboard-delivery-contract/validation.md` around lines 239 - 248, Scenario 6.2's SPDX validation only checks for a single "SPDX" occurrence; update the validation to assert both required header lines are present by changing the check that currently runs the bash loop to verify the first two lines of each file contain "SPDX-FileCopyrightText" and "SPDX-License-Identifier" (i.e., for the files listed in the scenario: src/lib/dashboard-contract.ts, src/lib/dashboard-health.ts, src/lib/dashboard-recover.ts and their .test.ts counterparts) so the test echoes OK only when both headers exist; locate the existing bash command in the Scenario 6.2 block and replace it with a two-line check per file that validates line 1 contains SPDX-FileCopyrightText and line 2 contains SPDX-License-Identifier.src/lib/dashboard-recover.test.ts (1)
111-117: This idempotency test never exercises a recovered chain.Both calls start from an already healthy chain, so this only proves the no-op path twice. If the goal is “recover once, then no-op on the second call”, make the first invocation start unhealthy and flip the mocks to healthy before the second one.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/dashboard-recover.test.ts` around lines 111 - 117, The test currently calls recoverDashboardChain twice on an already-healthy chain, so it never verifies a real recovery; change the test to make the first call start with an unhealthy chain (use buildMockDeps configured for failure/unhealthy state or mutate the mock to indicate missing/invalid blocks) and assert that the first result indicates attempted recovery, then flip the mocks to a healthy state (update the same mock dependencies or replace with buildMockDeps healthy) and call recoverDashboardChain a second time asserting it is a no-op (attempted === false); reference recoverDashboardChain and buildMockDeps to locate and modify the mock setup and the two calls in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@specs/2026-04-23_dashboard-delivery-contract/spec.md`:
- Around line 15-20: The document has markdownlint failures: unnamed fenced code
blocks, missing blank lines around fences, and restarted ordered-list numbering;
fix by adding a language label or inline comment to every fenced block (e.g.,
change triple-backtick blocks that show "Link 1: ... Link 4:" to use ```text or
```ini), ensure there is a blank line before and after each fenced code block,
and convert any restarted ordered lists into a single continuous ordered list
(or use explicit numbering tokens like "1." for each item if intentional); apply
these same fixes to all reported regions (the fenced block with "Link 1: Gateway
Process → running inside sandbox on :18789" and the other ranges referenced) so
the file is clean for markdownlint-cli2.
- Around line 1-10: The spec file starting with the "Dashboard Delivery
Contract" header is missing the repository-mandated SPDX license header; add the
required SPDX HTML comment at the very top of the Markdown (above the "#
Dashboard Delivery Contract" header) using the repository's standard SPDX string
(e.g., <!-- SPDX-License-Identifier: ... -->) so the file complies with the rule
for all .md files.
In `@specs/2026-04-23_dashboard-delivery-contract/tests.md`:
- Around line 92-174: Fix markdownlint errors by normalizing ordered-list
numbering to be sequential for the test cases (ensure the numbered list entries
for the tests under getDashboardGuidanceLines(), buildControlUiUrls(), and
buildAuthenticatedDashboardUrl() are consecutively numbered) and add a blank
line before and after any fenced code blocks or lists to satisfy the
"blank-lines-around-fenced-blocks" rule; update the sections around lines
referenced (including the blocks covering tests 16–32 and the other affected
ranges 232–238, 355–363) so the list numbering is restarted only where intended
and every fenced block has an empty line above and below.
- Around line 1-3: This Markdown test spec is missing the repository-mandated
SPDX license header; add the SPDX header as an HTML comment at the top of the
file (above the "# Dashboard Delivery Contract — Test Specification" title)
using the repo's required format (e.g., <!-- SPDX-License-Identifier: ... -->)
so the file conforms to the rule that all .md files include the SPDX header.
In `@specs/2026-04-23_dashboard-delivery-contract/validation.md`:
- Around line 6-333: Many headings (e.g., "Overview", "Phase 1: Dashboard
Contract Module — Validation Scenarios", "Scenario 1.1: buildChain produces
correct output for all platform combinations", and other "Phase X" / "Scenario
Y" headings) are missing a blank line after the heading which triggers MD022
warnings; fix by ensuring every Markdown heading line (lines starting with #,
##, ###, etc.) is followed by a single blank line before the next content or
subheading throughout the file (insert a newline after each of the headings
referenced above and any other headings present) so the document conforms to
markdownlint rule MD022.
- Around line 1-5: This file is missing the required SPDX header lines; add two
HTML comment SPDX lines at the very top of the Markdown (before the "#
Validation Plan: Dashboard Delivery Contract" heading): one using the
SPDX-FileCopyrightText token and one using SPDX-License-Identifier, ensuring
they appear as HTML comments so the file satisfies the
`**/*.{js,mjs,ts,tsx,sh,md}` header policy.
- Around line 78-87: The validation incorrectly expects ensureDashboardForward
to be deleted; update the test in specs/validation to reflect that
ensureDashboardForward now exists and delegates to buildChain: replace the
deletion check for ensureDashboardForward with a check that
ensureDashboardForward is present in src/lib/onboard.ts and that its
implementation calls or references buildChain (e.g., grep for "function
ensureDashboardForward" and "buildChain" within onboard.ts or validate the
delegation pattern), and remove ensureDashboardForward from the list of eight
deleted functions in the grep/count assertion so the test no longer fails
spuriously.
In `@src/lib/dashboard-contract.ts`:
- Around line 100-103: The code assigns raw chatUiUrl to accessUrl when
hasNonLoopbackUrl is true (and later similarly at the alternate URL branch),
which can leave values like "host:port" without a scheme; update the assignment
logic in the block that sets accessUrl (and the alternate URL branch that
mirrors this behavior) to canonicalize chatUiUrl into an absolute URL by adding
a default scheme (e.g., "http://") when missing and normalizing trailing
slashes, so accessUrl is always a proper absolute URL before use; locate the
branches that reference hasNonLoopbackUrl, chatUiUrl, accessUrl (and the
alternate URL logic) and replace the direct assignment with a small utility call
or inline check that ensures chatUiUrl starts with "http://" or "https://" and
prefixes "http://" if not present.
- Around line 156-163: The code builds dashboard URL fragments using the raw
token (hash = token ? `#token=${token}` : "") which can break URLs and
comparisons; replace all raw fragment constructions with a consistently encoded
fragment (e.g., create a single tokenFragment = token ?
`#token=${encodeURIComponent(token)}` : "") and use that variable wherever you
currently use hash or manually append token (references: hash, token, baseUrl,
CONTROL_UI_PATH, chatUiUrl). Update the other occurrences noted (around the
blocks that construct URLs at lines referenced in the comment) to use this
encoded tokenFragment and ensure any baseUrl/chatUi comparisons use normalized
forms (no fragment) so equality checks remain correct.
- Around line 46-56: The resolvePort function can return invalid ports (NaN, 0,
>65535) which then flow into accessUrl/forwardTarget; update resolvePort to
normalize and validate the parsed port: coerce to an integer, ensure it's
between 1 and 65535, and if it fails validation return defaultPort; also guard
any usage sites that call resolvePort (references to h.port / h.port usage) to
use the validated result rather than raw parsed values.
In `@src/lib/dashboard-recover.ts`:
- Around line 64-83: The current flow ignores the boolean result of
deps.restartGateway(sandboxName, chain.port, agent) and immediately calls
verifyDashboardChain, which can report false negatives while the gateway is
still warming up; change the logic in the recovery block so you capture the
return value of deps.restartGateway(...) and only push "restarted gateway" if it
returned true, and do not immediately call verifyDashboardChain
thereafter—instead either wait a short backoff or poll the gateway readiness
(e.g., loop with a retry/backoff checking the gateway health) until
restartGateway reports success or the gateway becomes healthy before running
verifyDashboardChain; use restartGateway, getSessionAgent and
verifyDashboardChain names to locate and modify the code.
In `@src/nemoclaw.ts`:
- Around line 361-365: The recovery chain is being built with only the port so
buildChain() falls back to a loopback contract; instead extract the platform
forwarding hints from the session agent returned by
agentRuntime.getSessionAgent(sandboxName) and pass them into buildChain (or pass
an agent-derived platformHints object) so real values like forward
host/target/contract (e.g., agent?.forwardHost, agent?.forwardTarget,
agent?.forwardContract or agent?.forward) are supplied rather than only
agent?.forwardPort ?? DASHBOARD_PORT before calling recoverDashboardChain;
update the buildChain call to use those agent hints (or a single agentHints
param) and keep makeDeps()/recoverDashboardChain usage the same.
- Around line 280-283: The captureForwardList implementation uses a nonexistent
runCaptureOpenshell and returns a raw object instead of the string|null that
verifyForward expects; replace the call with the existing captureOpenshell
helper (captureOpenshell(["forward","list"], { ignoreError: true })) and return
its result directly (ensuring you preserve async/await semantics if
captureOpenshell is async) so captureForwardList returns a string|null suitable
for verifyForward to parse.
---
Outside diff comments:
In `@src/lib/onboard.ts`:
- Around line 6149-6170: The WSL host hint isn't being passed through after the
refactor so dashboard URLs fall back to localhost; update the call sequence
around
buildChain/contractGetDashboardAccessInfo/contractGetDashboardGuidanceLines and
the agent path to use the resolved chain access URL (or wslHostAddress) instead
of raw chatUiUrl. Concretely: ensure buildChain(...) returns (or is called with)
the resolved WSL host hint (wslHostAddress or accessUrl), then pass that
resolved value into contractGetDashboardAccessInfo(chain, token) and
contractGetDashboardGuidanceLines(chain, dashboardAccess, ...), and when calling
agentOnboard.printDashboardUi(...) and its buildControlUiUrls callback, use
chain.accessUrl (or the resolved wslHostAddress) to construct Windows-reachable
control/dashboard URLs rather than the original chatUiUrl.
---
Nitpick comments:
In `@specs/2026-04-23_dashboard-delivery-contract/validation.md`:
- Around line 239-248: Scenario 6.2's SPDX validation only checks for a single
"SPDX" occurrence; update the validation to assert both required header lines
are present by changing the check that currently runs the bash loop to verify
the first two lines of each file contain "SPDX-FileCopyrightText" and
"SPDX-License-Identifier" (i.e., for the files listed in the scenario:
src/lib/dashboard-contract.ts, src/lib/dashboard-health.ts,
src/lib/dashboard-recover.ts and their .test.ts counterparts) so the test echoes
OK only when both headers exist; locate the existing bash command in the
Scenario 6.2 block and replace it with a two-line check per file that validates
line 1 contains SPDX-FileCopyrightText and line 2 contains
SPDX-License-Identifier.
In `@src/lib/dashboard-recover.test.ts`:
- Around line 111-117: The test currently calls recoverDashboardChain twice on
an already-healthy chain, so it never verifies a real recovery; change the test
to make the first call start with an unhealthy chain (use buildMockDeps
configured for failure/unhealthy state or mutate the mock to indicate
missing/invalid blocks) and assert that the first result indicates attempted
recovery, then flip the mocks to a healthy state (update the same mock
dependencies or replace with buildMockDeps healthy) and call
recoverDashboardChain a second time asserting it is a no-op (attempted ===
false); reference recoverDashboardChain and buildMockDeps to locate and modify
the mock setup and the two calls in the test.
🪄 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: 29d7ed79-9758-410b-959b-3961e152e8ca
📒 Files selected for processing (14)
specs/2026-04-23_dashboard-delivery-contract/spec.mdspecs/2026-04-23_dashboard-delivery-contract/tests.mdspecs/2026-04-23_dashboard-delivery-contract/validation.mdsrc/lib/dashboard-contract.test.tssrc/lib/dashboard-contract.tssrc/lib/dashboard-health.test.tssrc/lib/dashboard-health.tssrc/lib/dashboard-recover.test.tssrc/lib/dashboard-recover.tssrc/lib/dashboard.test.tssrc/lib/dashboard.tssrc/lib/onboard.tssrc/nemoclaw.tstest/onboard.test.ts
💤 Files with no reviewable changes (2)
- src/lib/dashboard.test.ts
- src/lib/dashboard.ts
Comment on lines
+1
to
+10
| # Dashboard Delivery Contract | ||
|
|
||
| **Issue:** #2390 | ||
| **Branch:** `issue-2390-dashboard-delivery-contract` | ||
| **Worktree:** `/Users/jyaunches/Development/NemoClaw-working/issue-2390` | ||
|
|
||
| ## Overview | ||
|
|
||
| Extract the dashboard delivery chain logic — URL derivation, CORS origins, port forwarding target, health probing, and recovery — from the monolith files (`onboard.ts`, `nemoclaw.ts`, `nemoclaw-start.sh`) into three focused modules that serve as the single source of truth for dashboard reachability. | ||
|
|
Contributor
There was a problem hiding this comment.
Add the required SPDX header to this spec file.
This new Markdown file is missing the repository-mandated license header.
Suggested fix
+<!-- SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -->
+<!-- SPDX-License-Identifier: Apache-2.0 -->
+
# Dashboard Delivery ContractAs per coding guidelines, every **/*.{js,mjs,ts,tsx,sh,md} file must include the SPDX header, and Markdown must use HTML comments for it.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Dashboard Delivery Contract | |
| **Issue:** #2390 | |
| **Branch:** `issue-2390-dashboard-delivery-contract` | |
| **Worktree:** `/Users/jyaunches/Development/NemoClaw-working/issue-2390` | |
| ## Overview | |
| Extract the dashboard delivery chain logic — URL derivation, CORS origins, port forwarding target, health probing, and recovery — from the monolith files (`onboard.ts`, `nemoclaw.ts`, `nemoclaw-start.sh`) into three focused modules that serve as the single source of truth for dashboard reachability. | |
| <!-- SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. --> | |
| <!-- SPDX-License-Identifier: Apache-2.0 --> | |
| # Dashboard Delivery Contract | |
| **Issue:** `#2390` | |
| **Branch:** `issue-2390-dashboard-delivery-contract` | |
| **Worktree:** `/Users/jyaunches/Development/NemoClaw-working/issue-2390` | |
| ## Overview | |
| Extract the dashboard delivery chain logic — URL derivation, CORS origins, port forwarding target, health probing, and recovery — from the monolith files (`onboard.ts`, `nemoclaw.ts`, `nemoclaw-start.sh`) into three focused modules that serve as the single source of truth for dashboard reachability. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/2026-04-23_dashboard-delivery-contract/spec.md` around lines 1 - 10,
The spec file starting with the "Dashboard Delivery Contract" header is missing
the repository-mandated SPDX license header; add the required SPDX HTML comment
at the very top of the Markdown (above the "# Dashboard Delivery Contract"
header) using the repository's standard SPDX string (e.g., <!--
SPDX-License-Identifier: ... -->) so the file complies with the rule for all .md
files.
Comment on lines
+15
to
+20
| ``` | ||
| Link 1: Gateway Process → running inside sandbox on :18789 | ||
| Link 2: Port Forward → SSH tunnel: host:18789 → sandbox:18789 | ||
| Link 3: CORS / Auth Config → allowedOrigins includes the browser's origin | ||
| Link 4: External Routing → [Brev] nginx + cloudflared → host:18789 | ||
| ``` |
Contributor
There was a problem hiding this comment.
Clean up the markdownlint failures in this document.
These sections currently trip markdownlint-cli2 for unnamed fenced code blocks, missing blank lines around fences, and restarted ordered-list numbering. Since this spec is meant to be the source of truth for the refactor, it should land lint-clean as well.
Also applies to: 142-154, 172-199, 215-235, 247-268, 414-430, 469-487, 559-564, 591-599, 632-649
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/2026-04-23_dashboard-delivery-contract/spec.md` around lines 15 - 20,
The document has markdownlint failures: unnamed fenced code blocks, missing
blank lines around fences, and restarted ordered-list numbering; fix by adding a
language label or inline comment to every fenced block (e.g., change
triple-backtick blocks that show "Link 1: ... Link 4:" to use ```text or
```ini), ensure there is a blank line before and after each fenced code block,
and convert any restarted ordered lists into a single continuous ordered list
(or use explicit numbering tokens like "1." for each item if intentional); apply
these same fixes to all reported regions (the fenced block with "Link 1: Gateway
Process → running inside sandbox on :18789" and the other ranges referenced) so
the file is clean for markdownlint-cli2.
Comment on lines
+1
to
+3
| # Dashboard Delivery Contract — Test Specification | ||
|
|
||
| Test guide for each implementation phase, following TDD (red → green → refactor). |
Contributor
There was a problem hiding this comment.
Add the required SPDX header to this test spec.
This new Markdown file is missing the repository-mandated license header.
Suggested fix
+<!-- SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -->
+<!-- SPDX-License-Identifier: Apache-2.0 -->
+
# Dashboard Delivery Contract — Test SpecificationAs per coding guidelines, every **/*.{js,mjs,ts,tsx,sh,md} file must include the SPDX header, and Markdown must use HTML comments for it.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Dashboard Delivery Contract — Test Specification | |
| Test guide for each implementation phase, following TDD (red → green → refactor). | |
| <!-- SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. --> | |
| <!-- SPDX-License-Identifier: Apache-2.0 --> | |
| # Dashboard Delivery Contract — Test Specification | |
| Test guide for each implementation phase, following TDD (red → green → refactor). |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/2026-04-23_dashboard-delivery-contract/tests.md` around lines 1 - 3,
This Markdown test spec is missing the repository-mandated SPDX license header;
add the SPDX header as an HTML comment at the top of the file (above the "#
Dashboard Delivery Contract — Test Specification" title) using the repo's
required format (e.g., <!-- SPDX-License-Identifier: ... -->) so the file
conforms to the rule that all .md files include the SPDX header.
Comment on lines
+92
to
+174
| 16. `should return single URL with token hash for loopback chain` | ||
| - **Input**: Loopback chain + token `"my-token"` | ||
| - **Expected**: Single entry `{ label: "Dashboard", url: "http://127.0.0.1:18789/#token=my-token" }` | ||
| - **Covers**: Token URL construction | ||
|
|
||
| 17. `should include both loopback and access URLs for non-loopback chain` | ||
| - **Input**: Chain with non-loopback accessUrl + token | ||
| - **Expected**: Two entries (Dashboard + Alt 1) | ||
| - **Covers**: Multi-URL output | ||
|
|
||
| 18. `should include VS Code/WSL URL when wslHostAddress provided` | ||
| - **Input**: WSL chain + `{ wslHostAddress: "172.24.240.1" }` | ||
| - **Expected**: Entry with label "VS Code/WSL" and WSL IP URL | ||
| - **Covers**: WSL URL generation | ||
|
|
||
| 19. `should return URLs without hash when token is null` | ||
| - **Input**: Chain + null token | ||
| - **Expected**: URLs without `#token=` | ||
| - **Covers**: Null token path | ||
|
|
||
| 20. `should deduplicate when access URL matches loopback` | ||
| - **Input**: Loopback chain | ||
| - **Expected**: Single entry, no duplicates | ||
| - **Covers**: Deduplication | ||
|
|
||
| ### `getDashboardGuidanceLines()` tests | ||
|
|
||
| 21. `should return port forwarding hint with correct port` | ||
| - **Input**: Chain with port 18789 | ||
| - **Expected**: Guidance includes "Port 18789" | ||
| - **Covers**: Port in guidance text | ||
|
|
||
| 22. `should include WSL-specific hint when WSL detected` | ||
| - **Input**: Options with WSL indicators | ||
| - **Expected**: Guidance includes "WSL" hint | ||
| - **Covers**: WSL guidance | ||
|
|
||
| 23. `should include note when no URLs generated` | ||
| - **Input**: Empty access list | ||
| - **Expected**: "No dashboard URLs were generated" | ||
| - **Covers**: Empty access edge case | ||
|
|
||
| ### `buildControlUiUrls()` tests (folded from deleted `dashboard.test.ts`) | ||
|
|
||
| 24. `should build URL with token hash` | ||
| - **Input**: `buildControlUiUrls("my-token")` | ||
| - **Expected**: `["http://127.0.0.1:18789/#token=my-token"]` | ||
| - **Covers**: Backward compat with old dashboard.ts | ||
|
|
||
| 25. `should build URL without token` | ||
| - **Input**: `buildControlUiUrls(null)` | ||
| - **Expected**: `["http://127.0.0.1:18789/"]` | ||
|
|
||
| 26. `should include chatUiUrl when passed as non-loopback http URL` | ||
| - **Input**: `buildControlUiUrls("tok", 18789, "https://my-dashboard.example.com")` | ||
| - **Expected**: 2 URLs | ||
| - **Covers**: Explicit chatUiUrl param (no env read) | ||
|
|
||
| 27. `should deduplicate when chatUiUrl matches local` | ||
| - **Input**: `buildControlUiUrls(null, 18789, "http://127.0.0.1:18789")` | ||
| - **Expected**: 1 URL | ||
|
|
||
| 28. `should ignore non-http chatUiUrl` | ||
| - **Input**: `buildControlUiUrls("tok", 18789, "ftp://example.com")` | ||
| - **Expected**: 1 URL | ||
|
|
||
| 29. `should ignore empty chatUiUrl` | ||
| - **Input**: `buildControlUiUrls("tok", 18789, " ")` | ||
| - **Expected**: 1 URL | ||
|
|
||
| 30. `should use configured port in displayed URL` | ||
| - **Input**: `buildControlUiUrls("my-token", 19000)` | ||
| - **Expected**: `["http://127.0.0.1:19000/#token=my-token"]` | ||
|
|
||
| ### `buildAuthenticatedDashboardUrl()` tests | ||
|
|
||
| 31. `should append token hash to URL` | ||
| - **Input**: `buildAuthenticatedDashboardUrl("http://127.0.0.1:18789/", "tok")` | ||
| - **Expected**: `"http://127.0.0.1:18789/#token=tok"` | ||
|
|
||
| 32. `should return base URL when token is null` | ||
| - **Input**: `buildAuthenticatedDashboardUrl("http://127.0.0.1:18789/", null)` | ||
| - **Expected**: `"http://127.0.0.1:18789/"` |
Contributor
There was a problem hiding this comment.
Fix the markdownlint issues in the numbered examples.
This file currently fails lint for restarted ordered-list numbering and missing blank lines around fenced blocks. Since the document is meant to drive implementation and verification, it should be kept lint-clean alongside the code changes.
Also applies to: 232-238, 355-363
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 92-92: Ordered list item prefix
Expected: 1; Actual: 16; Style: 1/2/3
(MD029, ol-prefix)
[warning] 97-97: Ordered list item prefix
Expected: 2; Actual: 17; Style: 1/2/3
(MD029, ol-prefix)
[warning] 102-102: Ordered list item prefix
Expected: 3; Actual: 18; Style: 1/2/3
(MD029, ol-prefix)
[warning] 107-107: Ordered list item prefix
Expected: 4; Actual: 19; Style: 1/2/3
(MD029, ol-prefix)
[warning] 112-112: Ordered list item prefix
Expected: 5; Actual: 20; Style: 1/2/3
(MD029, ol-prefix)
[warning] 119-119: Ordered list item prefix
Expected: 1; Actual: 21; Style: 1/2/3
(MD029, ol-prefix)
[warning] 124-124: Ordered list item prefix
Expected: 2; Actual: 22; Style: 1/2/3
(MD029, ol-prefix)
[warning] 129-129: Ordered list item prefix
Expected: 3; Actual: 23; Style: 1/2/3
(MD029, ol-prefix)
[warning] 136-136: Ordered list item prefix
Expected: 1; Actual: 24; Style: 1/2/3
(MD029, ol-prefix)
[warning] 141-141: Ordered list item prefix
Expected: 2; Actual: 25; Style: 1/2/3
(MD029, ol-prefix)
[warning] 145-145: Ordered list item prefix
Expected: 3; Actual: 26; Style: 1/2/3
(MD029, ol-prefix)
[warning] 150-150: Ordered list item prefix
Expected: 4; Actual: 27; Style: 1/2/3
(MD029, ol-prefix)
[warning] 154-154: Ordered list item prefix
Expected: 5; Actual: 28; Style: 1/2/3
(MD029, ol-prefix)
[warning] 158-158: Ordered list item prefix
Expected: 6; Actual: 29; Style: 1/2/3
(MD029, ol-prefix)
[warning] 162-162: Ordered list item prefix
Expected: 7; Actual: 30; Style: 1/2/3
(MD029, ol-prefix)
[warning] 168-168: Ordered list item prefix
Expected: 1; Actual: 31; Style: 1/2/3
(MD029, ol-prefix)
[warning] 172-172: Ordered list item prefix
Expected: 2; Actual: 32; Style: 1/2/3
(MD029, ol-prefix)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/2026-04-23_dashboard-delivery-contract/tests.md` around lines 92 - 174,
Fix markdownlint errors by normalizing ordered-list numbering to be sequential
for the test cases (ensure the numbered list entries for the tests under
getDashboardGuidanceLines(), buildControlUiUrls(), and
buildAuthenticatedDashboardUrl() are consecutively numbered) and add a blank
line before and after any fenced code blocks or lists to satisfy the
"blank-lines-around-fenced-blocks" rule; update the sections around lines
referenced (including the blocks covering tests 16–32 and the other affected
ranges 232–238, 355–363) so the list numbering is restarted only where intended
and every fenced block has an empty line above and below.
Comment on lines
+1
to
+5
| # Validation Plan: Dashboard Delivery Contract | ||
|
|
||
| Generated from: specs/2026-04-23_dashboard-delivery-contract/spec.md | ||
| Test Spec: specs/2026-04-23_dashboard-delivery-contract/tests.md | ||
|
|
Contributor
There was a problem hiding this comment.
Missing required SPDX header in this new Markdown file.
Please add the two SPDX lines at the top using HTML comments for Markdown.
Proposed fix
+<!-- SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -->
+<!-- SPDX-License-Identifier: Apache-2.0 -->
+
# Validation Plan: Dashboard Delivery ContractAs per coding guidelines **/*.{js,mjs,ts,tsx,sh,md}: Every source file must include an SPDX license header and “Use HTML comments for Markdown.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Validation Plan: Dashboard Delivery Contract | |
| Generated from: specs/2026-04-23_dashboard-delivery-contract/spec.md | |
| Test Spec: specs/2026-04-23_dashboard-delivery-contract/tests.md | |
| <!-- SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. --> | |
| <!-- SPDX-License-Identifier: Apache-2.0 --> | |
| # Validation Plan: Dashboard Delivery Contract | |
| Generated from: specs/2026-04-23_dashboard-delivery-contract/spec.md | |
| Test Spec: specs/2026-04-23_dashboard-delivery-contract/tests.md | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/2026-04-23_dashboard-delivery-contract/validation.md` around lines 1 -
5, This file is missing the required SPDX header lines; add two HTML comment
SPDX lines at the very top of the Markdown (before the "# Validation Plan:
Dashboard Delivery Contract" heading): one using the SPDX-FileCopyrightText
token and one using SPDX-License-Identifier, ensuring they appear as HTML
comments so the file satisfies the `**/*.{js,mjs,ts,tsx,sh,md}` header policy.
Comment on lines
+64
to
+83
| if (!before.links.gateway.ok) { | ||
| const agent = deps.getSessionAgent(sandboxName); | ||
| deps.restartGateway(sandboxName, chain.port, agent); | ||
| actions.push("restarted gateway"); | ||
| } | ||
|
|
||
| // Link 2: Forward | ||
| if (!before.links.forward.ok) { | ||
| deps.stopForward(chain.port); | ||
| deps.startForward(chain.forwardTarget, sandboxName); | ||
| actions.push("re-established forward"); | ||
| } | ||
|
|
||
| // Link 3: CORS — diagnose only | ||
| if (!before.links.cors.ok) { | ||
| actions.push(`CORS mismatch — rebuild required (${before.links.cors.detail})`); | ||
| } | ||
|
|
||
| // Re-verify after recovery | ||
| const after = verifyDashboardChain(sandboxName, chain, deps); |
Contributor
There was a problem hiding this comment.
Honor restartGateway() and avoid re-verifying immediately.
The boolean returned by restartGateway() is ignored here, so actions can claim success even when the restart failed. Then the follow-up health check runs immediately, which is prone to false negatives while the gateway is still warming up. That makes the recovery result both misleading and flaky.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/dashboard-recover.ts` around lines 64 - 83, The current flow ignores
the boolean result of deps.restartGateway(sandboxName, chain.port, agent) and
immediately calls verifyDashboardChain, which can report false negatives while
the gateway is still warming up; change the logic in the recovery block so you
capture the return value of deps.restartGateway(...) and only push "restarted
gateway" if it returned true, and do not immediately call verifyDashboardChain
thereafter—instead either wait a short backoff or poll the gateway readiness
(e.g., loop with a retry/backoff checking the gateway health) until
restartGateway reports success or the gateway becomes healthy before running
verifyDashboardChain; use restartGateway, getSessionAgent and
verifyDashboardChain names to locate and modify the code.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/nemoclaw.ts (2)
280-283:⚠️ Potential issue | 🟡 Minor
captureForwardListstill doesn't check command exit status.Per the
DashboardHealthDepsinterface,captureForwardListshould returnnullon failure. SincecaptureOpenshellalways returns an object, the checkfwdResult ?is always truthy and will returnfwdResult.outputeven when the command fails (status !== 0).🔧 Proposed fix (as suggested in prior review)
captureForwardList: () => { const fwdResult = captureOpenshell(["forward", "list"], { ignoreError: true }); - return fwdResult ? fwdResult.output : null; + return fwdResult.status === 0 ? fwdResult.output : null; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 280 - 283, captureForwardList currently returns fwdResult.output unconditionally because captureOpenshell always returns an object; update captureForwardList to inspect the result.status (or equivalent exit code) from captureOpenshell(["forward", "list"], { ignoreError: true}) and return null when status !== 0 (only return fwdResult.output when status === 0), referencing the captureForwardList function and the fwdResult.status/output fields to locate the change.
361-365:⚠️ Potential issue | 🟠 MajorRecovery chain still built with only
port, missing platform hints.As noted in the prior review, passing only
porttobuildChain()causes fallback to the loopback contract. On Brev/WSL, this means recovery may re-create forwards with a loopback target, leaving the dashboard unreachable.Consider extracting platform forwarding hints from the session agent (e.g.,
agent?.forwardHost,agent?.forwardTarget) and passing them tobuildChain().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 361 - 365, The recovery chain is being built with only a port which can cause fallback to a loopback contract; update the call that constructs the chain by extracting platform forwarding hints from the session agent (e.g., agent?.forwardHost and agent?.forwardTarget) and pass them into buildChain so it receives { port: agent?.forwardPort ?? DASHBOARD_PORT, host: agent?.forwardHost, target: agent?.forwardTarget } (or the equivalent parameter names expected by buildChain). Locate the buildChain invocation created via buildDashboardRecoverDeps() and modify the chain construction to include these agent-derived hint properties, ensuring you use optional chaining for agent to avoid runtime errors before calling recoverDashboardChain.
🧹 Nitpick comments (1)
src/nemoclaw.ts (1)
286-286: Remove unused import insidedownloadSandboxConfig.
startGatewayForRecoveryis imported but never used within this function. This appears to be leftover from copy-paste.🧹 Proposed fix
downloadSandboxConfig: (name) => { try { - const { startGatewayForRecovery } = require("./lib/onboard"); // Use the same download-and-parse pattern as fetchGatewayAuthTokenFromSandbox🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` at line 286, The require of startGatewayForRecovery inside downloadSandboxConfig is unused and should be removed; edit the downloadSandboxConfig function to delete the line "const { startGatewayForRecovery } = require('./lib/onboard');" (or otherwise stop importing that symbol) so there are no leftover unused imports referencing startGatewayForRecovery, leaving only the imports actually used by downloadSandboxConfig.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/nemoclaw.ts`:
- Around line 280-283: captureForwardList currently returns fwdResult.output
unconditionally because captureOpenshell always returns an object; update
captureForwardList to inspect the result.status (or equivalent exit code) from
captureOpenshell(["forward", "list"], { ignoreError: true}) and return null when
status !== 0 (only return fwdResult.output when status === 0), referencing the
captureForwardList function and the fwdResult.status/output fields to locate the
change.
- Around line 361-365: The recovery chain is being built with only a port which
can cause fallback to a loopback contract; update the call that constructs the
chain by extracting platform forwarding hints from the session agent (e.g.,
agent?.forwardHost and agent?.forwardTarget) and pass them into buildChain so it
receives { port: agent?.forwardPort ?? DASHBOARD_PORT, host: agent?.forwardHost,
target: agent?.forwardTarget } (or the equivalent parameter names expected by
buildChain). Locate the buildChain invocation created via
buildDashboardRecoverDeps() and modify the chain construction to include these
agent-derived hint properties, ensuring you use optional chaining for agent to
avoid runtime errors before calling recoverDashboardChain.
---
Nitpick comments:
In `@src/nemoclaw.ts`:
- Line 286: The require of startGatewayForRecovery inside downloadSandboxConfig
is unused and should be removed; edit the downloadSandboxConfig function to
delete the line "const { startGatewayForRecovery } = require('./lib/onboard');"
(or otherwise stop importing that symbol) so there are no leftover unused
imports referencing startGatewayForRecovery, leaving only the imports actually
used by downloadSandboxConfig.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 057f82d2-004c-47ac-ad13-926582b204e6
📒 Files selected for processing (1)
src/nemoclaw.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/nemoclaw.ts (2)
282-285:⚠️ Potential issue | 🟡 MinorReturn
nullwhenopenshell forward listfails.This still forwards
.outputon non-zero exits, soverifyDashboardChain()can end up parsing CLI diagnostics as if they were a real forward list. Please gate the return onfwdResult.status === 0.Suggested fix
captureForwardList: () => { const fwdResult = captureOpenshell(["forward", "list"], { ignoreError: true }); - return fwdResult ? fwdResult.output : null; + return fwdResult && fwdResult.status === 0 ? fwdResult.output : null; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 282 - 285, The captureForwardList helper returns fwdResult.output even when the openshell command failed; update captureForwardList (which calls captureOpenshell) to only return fwdResult.output when fwdResult exists AND fwdResult.status === 0, otherwise return null so verifyDashboardChain() doesn't parse CLI error diagnostics as a valid forward list.
363-365:⚠️ Potential issue | 🟠 MajorBuild the recovery chain from real platform hints, not just the port.
With only
port,buildChain()falls back to the loopback contract (127.0.0.1, loopback-only CORS, port-only forward target). On Brev/WSL that can "recover" the wrong target and still leave the dashboard unreachable after a sandbox restart. Please pass the same platform hints used to derive the live dashboard URL/forward contract instead of reconstructing a loopback default here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 363 - 365, The current buildChain call only passes the port (agent?.forwardPort), which causes buildChain to fall back to a loopback-only contract; instead retrieve and pass the full platform hint set used to derive the live dashboard URL/forward contract from the same agent session and feed those into buildChain/recoverDashboardChain. Concretely: get the session agent via agentRuntime.getSessionAgent(sandboxName), extract the platform hint fields used for URL/forwarding (e.g. host/forwardHost, target/forwardTarget, cors/forwardCors and port/forwardPort or a single platformHints/platformForward object if present) and call buildChain({ ...platformHints }) or use recoverDashboardChain(platformHints) so the recovery chain is constructed from the real platform hints rather than only the port.
🧹 Nitpick comments (1)
src/nemoclaw.ts (1)
288-290: Drop the unused onboard import here.
startGatewayForRecoveryis never referenced in this closure, so this should be removed rather than left as an unprefixed unused binding.Suggested fix
- const { startGatewayForRecovery } = require("./lib/onboard"); // Use the same download-and-parse pattern as fetchGatewayAuthTokenFromSandboxAs per coding guidelines, "Unused variables must be prefixed with
_in JavaScript and TypeScript files".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 288 - 290, The require for startGatewayForRecovery is unused in this closure—remove the destructuring import `const { startGatewayForRecovery } = require("./lib/onboard");` (or if you intentionally want to keep it for future use, rename it to `_startGatewayForRecovery`) so the unused binding is not left unprefixed; ensure other nearby symbols like `fetchGatewayAuthTokenFromSandbox` and `tmpDir` remain unchanged.
🤖 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/nemoclaw.ts`:
- Around line 242-249: The pre-check uses agentRuntime.getHealthProbeUrl(agent)
but the recovery logic uses the dashboard contract's health endpoint
(buildChain().healthEndpoint), causing false negatives; modify
isSandboxGatewayRunning() so that when constructing probeUrl it always uses the
dashboard contract health path (buildChain().healthEndpoint) for both OpenClaw
and non-OpenClaw flows (keeping host/port logic the same, e.g., use
DASHBOARD_PORT for localhost OpenClaw case and the agent host for remote
agents), replacing references to agentRuntime.getHealthProbeUrl(agent) and
ensuring recoverDashboardChain() and isSandboxGatewayRunning() probe the same
health endpoint.
---
Duplicate comments:
In `@src/nemoclaw.ts`:
- Around line 282-285: The captureForwardList helper returns fwdResult.output
even when the openshell command failed; update captureForwardList (which calls
captureOpenshell) to only return fwdResult.output when fwdResult exists AND
fwdResult.status === 0, otherwise return null so verifyDashboardChain() doesn't
parse CLI error diagnostics as a valid forward list.
- Around line 363-365: The current buildChain call only passes the port
(agent?.forwardPort), which causes buildChain to fall back to a loopback-only
contract; instead retrieve and pass the full platform hint set used to derive
the live dashboard URL/forward contract from the same agent session and feed
those into buildChain/recoverDashboardChain. Concretely: get the session agent
via agentRuntime.getSessionAgent(sandboxName), extract the platform hint fields
used for URL/forwarding (e.g. host/forwardHost, target/forwardTarget,
cors/forwardCors and port/forwardPort or a single platformHints/platformForward
object if present) and call buildChain({ ...platformHints }) or use
recoverDashboardChain(platformHints) so the recovery chain is constructed from
the real platform hints rather than only the port.
---
Nitpick comments:
In `@src/nemoclaw.ts`:
- Around line 288-290: The require for startGatewayForRecovery is unused in this
closure—remove the destructuring import `const { startGatewayForRecovery } =
require("./lib/onboard");` (or if you intentionally want to keep it for future
use, rename it to `_startGatewayForRecovery`) so the unused binding is not left
unprefixed; ensure other nearby symbols like `fetchGatewayAuthTokenFromSandbox`
and `tmpDir` remain unchanged.
🪄 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: 31cf0abd-4be1-4787-9132-20f3cac16deb
📒 Files selected for processing (1)
src/nemoclaw.ts
jyaunches
force-pushed
the
issue-2390-dashboard-delivery-contract
branch
from
4d6ec68 to
6f57657
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/lib/dashboard-contract.ts (3)
151-163:⚠️ Potential issue | 🟠 MajorEncode
#token=fragments consistently across all dashboard URLs.Line 156 still appends the raw token, while Lines 189-192 build the WSL URL with
encodeURIComponent(). Tokens containing reserved characters break the local URLs and can also make the dedupe at Line 195 miss the matching WSL entry.Proposed fix
export function buildControlUiUrls( token: string | null = null, port: number = DASHBOARD_PORT, chatUiUrl?: string, ): string[] { - const hash = token ? `#token=${token}` : ""; + const hash = token ? `#token=${encodeURIComponent(token)}` : ""; const baseUrl = `http://127.0.0.1:${port}`; const urls = [`${baseUrl}${CONTROL_UI_PATH}${hash}`]; @@ export function getDashboardAccessInfo( @@ const dashboardAccess = buildControlUiUrls(token, chain.port, chain.accessUrl).map( (url, index) => ({ label: index === 0 ? "Dashboard" : `Alt ${index}`, - url: buildAuthenticatedDashboardUrl(url, null), + url, }), );Also applies to: 175-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/dashboard-contract.ts` around lines 151 - 163, The token fragment is appended raw in buildControlUiUrls (using `#token=${token}`) which breaks URLs and deduping; change construction of the hash to encode the token with encodeURIComponent (e.g., use `#token=${encodeURIComponent(token)}`) so every URL built in buildControlUiUrls and the related WSL/alternate-URL logic (the code that builds the chat/WSL URL and any other places that append CONTROL_UI_PATH+hash) uses the same encoded token format to preserve valid URLs and allow the new Set() dedupe to match correctly.
46-56:⚠️ Potential issue | 🟠 MajorValidate
portbefore it enters the contract.Lines 51-54 and Line 93 still allow
NaN,0, or out-of-range ports from eitherchatUiUrlparsing orh.port. Once that happens, every derived field in the chain becomes malformed.Proposed fix
+function normalizePort(port: number | undefined, fallback: number): number { + if (!Number.isInteger(port)) return fallback; + if (port < 1 || port > 65535) return fallback; + return port; +} + function resolvePort(chatUiUrl: string, defaultPort: number): number { const raw = String(chatUiUrl || "").trim(); if (!raw) return defaultPort; try { const parsed = new URL(/^[a-z]+:\/\//i.test(raw) ? raw : `http://${raw}`); - return parsed.port ? Number(parsed.port) : defaultPort; + return normalizePort(parsed.port ? Number(parsed.port) : undefined, defaultPort); } catch { const portMatch = raw.match(/:(\d{2,5})(?:[/?#]|$)/); - return portMatch ? Number(portMatch[1]) : defaultPort; + return normalizePort(portMatch ? Number(portMatch[1]) : undefined, defaultPort); } } @@ - const port = h.port ?? resolvePort(chatUiUrl, defaultPort); + const port = normalizePort(h.port, resolvePort(chatUiUrl, defaultPort));Also applies to: 89-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/dashboard-contract.ts` around lines 46 - 56, The resolvePort function currently returns Number(parsed.port) or Number(portMatch[1]) without validating the result, allowing NaN, 0 or out-of-range values into the contract; update resolvePort to coerce and validate the parsed port (ensure it's an integer between 1 and 65535) and fall back to defaultPort for any invalid value, and apply the same validation where h.port is used elsewhere (the other port-parsing code around the h.port handling referenced in the comment) so every place that parses a port uses this validated result before building the derived fields.
99-103:⚠️ Potential issue | 🟠 MajorCanonicalize scheme-less
chatUiUrlvalues before storing or reusing them.Line 101 returns raw
chatUiUrl, but Lines 159-162 only reuse it when it already starts withhttp://orhttps://. A value likechat.example.com:18789therefore produces a non-absoluteaccessUrland then disappears frombuildControlUiUrls()/ dashboard output.Proposed fix
+function toAbsoluteHttpUrl(rawUrl: string): string { + return /^[a-z]+:\/\//i.test(rawUrl) ? rawUrl : `http://${rawUrl}`; +} + export function buildChain(hints?: PlatformHints): DashboardDeliveryChain { @@ let accessUrl: string; if (hasNonLoopbackUrl) { - accessUrl = chatUiUrl; + accessUrl = toAbsoluteHttpUrl(chatUiUrl).replace(/\/$/, ""); } else if (h.isWsl && h.wslHostAddress) { accessUrl = `http://${h.wslHostAddress}:${port}`; } else { @@ export function buildControlUiUrls( @@ ): string[] { const hash = token ? `#token=${token}` : ""; const baseUrl = `http://127.0.0.1:${port}`; const urls = [`${baseUrl}${CONTROL_UI_PATH}${hash}`]; - const chatUi = (chatUiUrl || "").trim().replace(/\/$/, ""); - if (chatUi && /^https?:\/\//i.test(chatUi) && chatUi !== baseUrl) { - urls.push(`${chatUi}${CONTROL_UI_PATH}${hash}`); + const chatUi = (chatUiUrl || "").trim().replace(/\/$/, ""); + const absoluteChatUi = chatUi ? toAbsoluteHttpUrl(chatUi).replace(/\/$/, "") : ""; + if (absoluteChatUi && absoluteChatUi !== baseUrl) { + urls.push(`${absoluteChatUi}${CONTROL_UI_PATH}${hash}`); } return [...new Set(urls)]; }Also applies to: 159-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/dashboard-contract.ts` around lines 99 - 103, The code returns raw chatUiUrl into accessUrl when hasNonLoopbackUrl is true, but later logic (and buildControlUiUrls()) only recognizes absolute URLs with http/https; update the canonicalization so any scheme-less chatUiUrl (e.g., "chat.example.com:18789") is normalized to an absolute URL by prepending "http://" (or otherwise ensuring it starts with "http://" or "https://") before storing/reusing it; modify the logic around accessUrl assignment (where chatUiUrl is used) and the code paths in buildControlUiUrls() to reuse the canonicalized URL so scheme-less values are preserved.src/lib/dashboard-recover.ts (1)
64-68:⚠️ Potential issue | 🟠 MajorRecord gateway restart as success only when it actually succeeds.
restartGateway()returns a boolean, but the current flow always appends"restarted gateway". This can report a successful action even when restart failed.💡 Proposed fix
if (!before.links.gateway.ok) { const agent = deps.getSessionAgent(sandboxName); - deps.restartGateway(sandboxName, chain.port, agent); - actions.push("restarted gateway"); + const restarted = deps.restartGateway(sandboxName, chain.port, agent); + actions.push(restarted ? "restarted gateway" : "gateway restart failed"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/dashboard-recover.ts` around lines 64 - 68, The code unconditionally pushes "restarted gateway" even though deps.restartGateway(sandboxName, chain.port, agent) returns a boolean; update the block that runs when !before.links.gateway.ok to capture the boolean result from restartGateway, and only push "restarted gateway" into actions when that result is true (otherwise push a failure entry or handle/log the false case); reference the existing deps.restartGateway call, deps.getSessionAgent(sandboxName) for the agent, the actions array, and the surrounding condition on before.links.gateway.ok to locate where to add the check.
🤖 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/dashboard-health.ts`:
- Around line 95-105: In verifyForward, don't declare a forward healthy based
solely on sandboxName and portStr match; also require the row's STATUS be
"running" before returning ok: true. Locate the parsing loop in verifyForward
(parts = line.trim().split(/\s+/)) and add a check for the STATUS field (the
STATUS token in parts — e.g., parts[4] or the column position used by the
command producing output) and only return { ok: true, detail: ... } when
parts[2] === portStr, parts[0] === sandboxName, and the STATUS token
(case-insensitive) === "running"; otherwise treat it as a conflict/not running
and return ok: false with an appropriate detail message. Ensure detail still
uses PID/host tokens (parts[3], parts[1]) as before.
In `@src/lib/onboard.ts`:
- Around line 6175-6178: Compute the WSL host IP (e.g., via your existing helper
like getWslHostIp()) and include it in the options object passed to buildChain,
contractGetDashboardAccessInfo and contractGetDashboardGuidanceLines so the
dashboard can generate the VS Code/WSL URL; specifically, replace the current
options that only contain isWsl/isWsl() with an options object containing both
isWsl and the wslHostIp (or DashboardAccessOptions shape expected by
contractGetDashboardAccessInfo/contractGetDashboardGuidanceLines) and pass that
same options into those calls (buildChain, contractGetDashboardAccessInfo,
contractGetDashboardGuidanceLines).
---
Duplicate comments:
In `@src/lib/dashboard-contract.ts`:
- Around line 151-163: The token fragment is appended raw in buildControlUiUrls
(using `#token=${token}`) which breaks URLs and deduping; change construction of
the hash to encode the token with encodeURIComponent (e.g., use
`#token=${encodeURIComponent(token)}`) so every URL built in buildControlUiUrls
and the related WSL/alternate-URL logic (the code that builds the chat/WSL URL
and any other places that append CONTROL_UI_PATH+hash) uses the same encoded
token format to preserve valid URLs and allow the new Set() dedupe to match
correctly.
- Around line 46-56: The resolvePort function currently returns
Number(parsed.port) or Number(portMatch[1]) without validating the result,
allowing NaN, 0 or out-of-range values into the contract; update resolvePort to
coerce and validate the parsed port (ensure it's an integer between 1 and 65535)
and fall back to defaultPort for any invalid value, and apply the same
validation where h.port is used elsewhere (the other port-parsing code around
the h.port handling referenced in the comment) so every place that parses a port
uses this validated result before building the derived fields.
- Around line 99-103: The code returns raw chatUiUrl into accessUrl when
hasNonLoopbackUrl is true, but later logic (and buildControlUiUrls()) only
recognizes absolute URLs with http/https; update the canonicalization so any
scheme-less chatUiUrl (e.g., "chat.example.com:18789") is normalized to an
absolute URL by prepending "http://" (or otherwise ensuring it starts with
"http://" or "https://") before storing/reusing it; modify the logic around
accessUrl assignment (where chatUiUrl is used) and the code paths in
buildControlUiUrls() to reuse the canonicalized URL so scheme-less values are
preserved.
In `@src/lib/dashboard-recover.ts`:
- Around line 64-68: The code unconditionally pushes "restarted gateway" even
though deps.restartGateway(sandboxName, chain.port, agent) returns a boolean;
update the block that runs when !before.links.gateway.ok to capture the boolean
result from restartGateway, and only push "restarted gateway" into actions when
that result is true (otherwise push a failure entry or handle/log the false
case); reference the existing deps.restartGateway call,
deps.getSessionAgent(sandboxName) for the agent, the actions array, and the
surrounding condition on before.links.gateway.ok to locate where to add the
check.
🪄 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: 1f0aa5ec-bc9c-412f-8711-4f20ad63d646
📒 Files selected for processing (11)
src/lib/dashboard-contract.test.tssrc/lib/dashboard-contract.tssrc/lib/dashboard-health.test.tssrc/lib/dashboard-health.tssrc/lib/dashboard-recover.test.tssrc/lib/dashboard-recover.tssrc/lib/dashboard.test.tssrc/lib/dashboard.tssrc/lib/onboard.tssrc/nemoclaw.tstest/onboard.test.ts
💤 Files with no reviewable changes (2)
- src/lib/dashboard.test.ts
- src/lib/dashboard.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/dashboard-contract.test.ts
- src/nemoclaw.ts
jyaunches
force-pushed
the
issue-2390-dashboard-delivery-contract
branch
from
6f57657 to
e40b673
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
src/lib/dashboard-contract.ts (1)
32-58:⚠️ Potential issue | 🟠 MajorNormalize
portbefore putting it into the contract.
resolvePort()andh.portstill accept invalid values likeNaN,0, or>65535. Those flow straight intoaccessUrl,corsOrigins, andforwardTarget, so one bad hint makes the whole delivery chain invalid.Suggested fix
+function normalizePort(port: number | undefined, fallback: number): number { + if (!Number.isInteger(port)) return fallback; + if (port < 1 || port > 65535) return fallback; + return port; +} + function resolvePort(chatUiUrl: string, defaultPort: number): number { const raw = String(chatUiUrl || "").trim(); if (!raw) return defaultPort; try { const parsed = new URL(ensureScheme(raw)); - return parsed.port ? Number(parsed.port) : defaultPort; + return normalizePort(parsed.port ? Number(parsed.port) : undefined, defaultPort); } catch { const m = raw.match(/:(\d{2,5})(?:[/?#]|$)/); - return m ? Number(m[1]) : defaultPort; + return normalizePort(m ? Number(m[1]) : undefined, defaultPort); } } @@ - const port = h.port ?? resolvePort(chatUiUrl, DASHBOARD_PORT); + const port = normalizePort(h.port, resolvePort(chatUiUrl, DASHBOARD_PORT));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/dashboard-contract.ts` around lines 32 - 58, The port value coming from resolvePort() or hints (PlatformHints.port) can be NaN, 0 or out-of-range and leaks into accessUrl, corsOrigins and forwardTarget; update the validation so the effective port is a positive integer 1–65535 and otherwise fall back to DASHBOARD_PORT: ensure resolvePort returns a finite integer within that range (or default), and in buildChain coerce/sanitize h.port before using it (e.g. parseInt/Number check, isFinite, clamp to 1..65535 or use DASHBOARD_PORT) so accessUrl, corsOrigins and forwardTarget always receive a valid port.src/nemoclaw.ts (2)
364-367:⚠️ Potential issue | 🟠 MajorBuild the recovery chain from agent/platform hints, not just env + port.
buildChain({ chatUiUrl, port })drops the non-loopback/WSL forwarding context that the contract uses to deriveaccessUrl,corsOrigins, andforwardTarget. On Brev/WSL that can recreate the wrong forward target and diagnose CORS against loopback instead of the real public URL.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 364 - 367, The recovery chain is being built only from chatUiUrl and port which loses agent/platform forwarding context; update the buildChain call to pass the agent-derived hints (use values from agentRuntime.getSessionAgent(sandboxName) such as agent?.forwardHost, agent?.forwardTarget, agent?.platform or equivalent properties) so that buildChain (and recoverDashboardChain/makeDeps if they accept options) can derive accessUrl, corsOrigins, and forwardTarget correctly; modify the invocation of buildChain({ chatUiUrl, port }) to include these agent/platform fields (and update recoverDashboardChain/makeDeps parameter shapes if needed) so non-loopback/WSL forwarding is preserved.
243-250:⚠️ Potential issue | 🟠 MajorUse the dashboard contract’s
/healthendpoint for every agent here.Non-OpenClaw agents still probe
agentRuntime.getHealthProbeUrl(agent), but the new chain verifier probesbuildChain().healthEndpoint, which is always/health. If those differ, this pre-check can report “down” and trigger recovery even thoughrecoverDashboardChain()later sees the gateway as healthy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 243 - 250, The pre-check uses agentRuntime.getHealthProbeUrl(agent) for non-OpenClaw agents which can differ from the chain verifier's buildChain().healthEndpoint (always "/health") and cause false "down" results; change the probe selection so probeUrl always points to the dashboard contract health endpoint (http://127.0.0.1:${DASHBOARD_PORT}/health) regardless of agent, replacing the conditional that references agentRuntime.getHealthProbeUrl(agent) in the block that obtains agent via agentRuntime.getSessionAgent(sandboxName) so the pre-check aligns with buildChain().healthEndpoint and avoids spurious recoverDashboardChain() triggers.src/lib/dashboard-recover.ts (1)
32-45:⚠️ Potential issue | 🟠 MajorAvoid the immediate final verify after restart/forward recovery.
This still re-runs
verifyDashboardChain()right after the repair attempts. A successfulrestartGateway()only means the process launched; it does not guarantee/healthis already serving, so normal warm-up can be reported as a failed recovery.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/dashboard-recover.ts` around lines 32 - 45, The final verifyDashboardChain(...) call runs immediately after attempting repairs and can false-fail because restartGateway(name, ...) only starts the process; instead, after calling deps.restartGateway, deps.stopForward and deps.startForward, do not call verifyDashboardChain synchronously; either poll/wait for the gateway to become healthy with a short retry loop and timeout (using an explicit health check method or reusing verifyDashboardChain in a retriable loop), or return a “pending” recovery state and let the caller re-run verification later; update the logic around deps.restartGateway, deps.stopForward, deps.startForward, actions and verifyDashboardChain to use that delayed/retry approach.src/lib/dashboard-health.ts (1)
39-44:⚠️ Potential issue | 🟠 MajorRequire a running forward row before marking Link 2 healthy.
This currently treats any matching sandbox+port row as healthy. A stale or non-running
openshell forward listentry will suppress recovery even though the dashboard is still unreachable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/dashboard-health.ts` around lines 39 - 44, When scanning the openshell forward list (loop over output, variables output, portStr, name), only treat a matching sandbox+port row as healthy if that row indicates an active/running forward; update the condition inside the for-loop to check the row's running indicator (e.g., require a non-empty/non-"?" PID in p[3] or an explicit "running" status token in the parsed columns) before returning { ok: true, ... }, otherwise treat it as not healthy so recovery can proceed.
🤖 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/dashboard-health.test.ts`:
- Around line 5-6: Tests import compiled artifacts from dist; change imports to
point at the source modules so tests run against current source. Replace imports
of "../../dist/lib/dashboard-health" and "../../dist/lib/dashboard-contract"
with their source equivalents (importing verifyDashboardChain and buildChain
from the src modules), and apply the same change in the companion test file that
references dashboard-recover to ensure both tests import source modules rather
than dist artifacts.
In `@src/lib/dashboard-health.ts`:
- Around line 50-58: verifyCors currently only checks new
URL(chain.accessUrl).origin but buildChain publishes chain.corsOrigins and the
verifier must ensure all control UI origins are allowed; update verifyCors to
read config.gateway.controlUi.allowedOrigins and verify that every origin in
chain.corsOrigins (falling back to [new URL(chain.accessUrl).origin] if
corsOrigins is absent) is present in allowedOrigins, returning ok: true only
when all are included and ok: false with a detail listing the missing origins
when any are absent; reference verifyCors, chain.corsOrigins, buildChain and
buildControlUiUrls to locate the relevant logic.
In `@src/lib/onboard.ts`:
- Around line 6169-6174: The fallback chat UI URL hardcodes CONTROL_UI_PORT;
change the chatUiUrl assignment to prefer an agent-forwarded port when available
(use agent.forwardPort from createSandbox()) so the dashboard chain and
buildControlUiUrls(token, chain.port, chain.accessUrl) point at the correct
port; specifically, update the chatUiUrl expression (and any place that builds
chain.port) to use agent.forwardPort || CONTROL_UI_PORT (or equivalent) when
CHAT_UI_URL is unset, ensuring buildChain(...) and subsequent
buildControlUiUrls(...) receive the proper port for agent sandboxes.
---
Duplicate comments:
In `@src/lib/dashboard-contract.ts`:
- Around line 32-58: The port value coming from resolvePort() or hints
(PlatformHints.port) can be NaN, 0 or out-of-range and leaks into accessUrl,
corsOrigins and forwardTarget; update the validation so the effective port is a
positive integer 1–65535 and otherwise fall back to DASHBOARD_PORT: ensure
resolvePort returns a finite integer within that range (or default), and in
buildChain coerce/sanitize h.port before using it (e.g. parseInt/Number check,
isFinite, clamp to 1..65535 or use DASHBOARD_PORT) so accessUrl, corsOrigins and
forwardTarget always receive a valid port.
In `@src/lib/dashboard-health.ts`:
- Around line 39-44: When scanning the openshell forward list (loop over output,
variables output, portStr, name), only treat a matching sandbox+port row as
healthy if that row indicates an active/running forward; update the condition
inside the for-loop to check the row's running indicator (e.g., require a
non-empty/non-"?" PID in p[3] or an explicit "running" status token in the
parsed columns) before returning { ok: true, ... }, otherwise treat it as not
healthy so recovery can proceed.
In `@src/lib/dashboard-recover.ts`:
- Around line 32-45: The final verifyDashboardChain(...) call runs immediately
after attempting repairs and can false-fail because restartGateway(name, ...)
only starts the process; instead, after calling deps.restartGateway,
deps.stopForward and deps.startForward, do not call verifyDashboardChain
synchronously; either poll/wait for the gateway to become healthy with a short
retry loop and timeout (using an explicit health check method or reusing
verifyDashboardChain in a retriable loop), or return a “pending” recovery state
and let the caller re-run verification later; update the logic around
deps.restartGateway, deps.stopForward, deps.startForward, actions and
verifyDashboardChain to use that delayed/retry approach.
In `@src/nemoclaw.ts`:
- Around line 364-367: The recovery chain is being built only from chatUiUrl and
port which loses agent/platform forwarding context; update the buildChain call
to pass the agent-derived hints (use values from
agentRuntime.getSessionAgent(sandboxName) such as agent?.forwardHost,
agent?.forwardTarget, agent?.platform or equivalent properties) so that
buildChain (and recoverDashboardChain/makeDeps if they accept options) can
derive accessUrl, corsOrigins, and forwardTarget correctly; modify the
invocation of buildChain({ chatUiUrl, port }) to include these agent/platform
fields (and update recoverDashboardChain/makeDeps parameter shapes if needed) so
non-loopback/WSL forwarding is preserved.
- Around line 243-250: The pre-check uses agentRuntime.getHealthProbeUrl(agent)
for non-OpenClaw agents which can differ from the chain verifier's
buildChain().healthEndpoint (always "/health") and cause false "down" results;
change the probe selection so probeUrl always points to the dashboard contract
health endpoint (http://127.0.0.1:${DASHBOARD_PORT}/health) regardless of agent,
replacing the conditional that references agentRuntime.getHealthProbeUrl(agent)
in the block that obtains agent via agentRuntime.getSessionAgent(sandboxName) so
the pre-check aligns with buildChain().healthEndpoint and avoids spurious
recoverDashboardChain() triggers.
🪄 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: 3de538b5-3163-4119-b9d6-72049d77e237
📒 Files selected for processing (11)
src/lib/dashboard-contract.test.tssrc/lib/dashboard-contract.tssrc/lib/dashboard-health.test.tssrc/lib/dashboard-health.tssrc/lib/dashboard-recover.test.tssrc/lib/dashboard-recover.tssrc/lib/dashboard.test.tssrc/lib/dashboard.tssrc/lib/onboard.tssrc/nemoclaw.tstest/onboard.test.ts
💤 Files with no reviewable changes (2)
- src/lib/dashboard.test.ts
- src/lib/dashboard.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/dashboard-contract.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/onboard.test.ts
Comment on lines
+5
to
+6
| import { verifyDashboardChain } from "../../dist/lib/dashboard-health"; | ||
| import { buildChain } from "../../dist/lib/dashboard-contract"; |
Contributor
There was a problem hiding this comment.
Import the source modules instead of dist in these tests.
These assertions are tied to whatever was last built into dist, so they can miss regressions in the source under review or fail on a clean checkout where dist/ has not been generated. The same fix should be applied to src/lib/dashboard-recover.test.ts as well.
🔧 Proposed fix
-import { verifyDashboardChain } from "../../dist/lib/dashboard-health";
-import { buildChain } from "../../dist/lib/dashboard-contract";
+import { verifyDashboardChain } from "./dashboard-health";
+import { buildChain } from "./dashboard-contract";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { verifyDashboardChain } from "../../dist/lib/dashboard-health"; | |
| import { buildChain } from "../../dist/lib/dashboard-contract"; | |
| import { verifyDashboardChain } from "./dashboard-health"; | |
| import { buildChain } from "./dashboard-contract"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/dashboard-health.test.ts` around lines 5 - 6, Tests import compiled
artifacts from dist; change imports to point at the source modules so tests run
against current source. Replace imports of "../../dist/lib/dashboard-health" and
"../../dist/lib/dashboard-contract" with their source equivalents (importing
verifyDashboardChain and buildChain from the src modules), and apply the same
change in the companion test file that references dashboard-recover to ensure
both tests import source modules rather than dist artifacts.
Comment on lines
+50
to
+58
| function verifyCors(name: string, chain: DashboardDeliveryChain, deps: DashboardHealthDeps): LinkStatus { | ||
| const config = deps.downloadSandboxConfig(name); | ||
| if (!config) return { ok: false, detail: "could not download openclaw.json" }; | ||
| const origins = config.gateway?.controlUi?.allowedOrigins ?? []; | ||
| let accessOrigin: string | null; | ||
| try { accessOrigin = new URL(chain.accessUrl).origin; } catch { return { ok: false, detail: "bad accessUrl" }; } | ||
| return origins.includes(accessOrigin) | ||
| ? { ok: true, detail: `allowedOrigins includes ${accessOrigin}` } | ||
| : { ok: false, detail: `missing ${accessOrigin} in allowedOrigins` }; |
Contributor
There was a problem hiding this comment.
Validate the full CORS contract, not just accessUrl.
buildChain() publishes chain.corsOrigins, but this check only looks for new URL(chain.accessUrl).origin. If allowedOrigins is missing http://127.0.0.1:${chain.port}, the verifier can still report healthy even though the local control UI URL generated by buildControlUiUrls() will fail CORS. ``
Suggested fix
function verifyCors(name: string, chain: DashboardDeliveryChain, deps: DashboardHealthDeps): LinkStatus {
const config = deps.downloadSandboxConfig(name);
if (!config) return { ok: false, detail: "could not download openclaw.json" };
const origins = config.gateway?.controlUi?.allowedOrigins ?? [];
- let accessOrigin: string | null;
- try { accessOrigin = new URL(chain.accessUrl).origin; } catch { return { ok: false, detail: "bad accessUrl" }; }
- return origins.includes(accessOrigin)
- ? { ok: true, detail: `allowedOrigins includes ${accessOrigin}` }
- : { ok: false, detail: `missing ${accessOrigin} in allowedOrigins` };
+ const missing = chain.corsOrigins.filter((origin) => !origins.includes(origin));
+ return missing.length === 0
+ ? { ok: true, detail: `allowedOrigins includes ${chain.corsOrigins.join(", ")}` }
+ : { ok: false, detail: `missing ${missing.join(", ")} in allowedOrigins` };
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/dashboard-health.ts` around lines 50 - 58, verifyCors currently only
checks new URL(chain.accessUrl).origin but buildChain publishes
chain.corsOrigins and the verifier must ensure all control UI origins are
allowed; update verifyCors to read config.gateway.controlUi.allowedOrigins and
verify that every origin in chain.corsOrigins (falling back to [new
URL(chain.accessUrl).origin] if corsOrigins is absent) is present in
allowedOrigins, returning ok: true only when all are included and ok: false with
a detail listing the missing origins when any are absent; reference verifyCors,
chain.corsOrigins, buildChain and buildControlUiUrls to locate the relevant
logic.
e40b673 to
fb555de
Compare
ericksoa
previously requested changes
Apr 24, 2026
Contributor
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Good refactor direction — the contract/health/recover split with injected deps is clean architecture, and the #2342 fix (accept 401 as alive on /health) is correct. 52 new tests.
Blocking: NEMOCLAW_AGENT_TIMEOUT reverted
The onboard.ts diff deletes the NEMOCLAW_AGENT_TIMEOUT block from patchStagedDockerfile that was just merged in #2326. This is likely a stale branch — the PR was probably branched before #2326 landed. The deleted lines (around the old diff line 1395-1404) are:
const agentTimeout = process.env.NEMOCLAW_AGENT_TIMEOUT;
if (agentTimeout && POSITIVE_INT_RE.test(agentTimeout)) {
dockerfile = dockerfile.replace(
/^ARG NEMOCLAW_AGENT_TIMEOUT=.*$/m,
`ARG NEMOCLAW_AGENT_TIMEOUT=${agentTimeout}`,
);
}Please rebase onto current main so this block is preserved.
Looks good otherwise
buildChain()is pure — noprocess.envreads, all config viaPlatformHints. Good.isSandboxGatewayRunningfix — probing/healthinstead of/and usingcurl -w '%{http_code}'instead ofcurl -sfcorrectly handles the 401 auth-gated case.- Recovery deps are properly injected —
DashboardRecoverDepsextendsDashboardHealthDeps, CORS is diagnose-only (no auto-fix), gateway→forward ordering is correct. - Token encoding improvement —
encodeURIComponent(token)inbuildControlUiUrlsis more correct than the old bare interpolation. - Test coverage — 32 contract + 13 health + 7 recovery tests with good edge cases (empty URLs, IPv6, schemeless, WSL).
Minor (non-blocking)
-
buildDashboardRecoverDeps()is heavy — thedownloadSandboxConfigclosure does tmpdir creation, openshell download, JSON parse, and cleanup inline. Consider extracting this into a named helper for readability. -
buildControlUiUrlssignature change — now takeschatUiUrlas 3rd param instead of readingprocess.env.CHAT_UI_URL. This is the right move for testability, but verify no external callers depend on the old env-reading behavior.
jyaunches
force-pushed
the
issue-2390-dashboard-delivery-contract
branch
2 times, most recently
from
7849a25 to
fca502a
Compare
…recover modules Extract the scattered dashboard delivery chain logic from onboard.ts and nemoclaw.ts into three focused modules: - dashboard-contract.ts: Pure buildChain() — single source of truth for access URL, CORS origins, forward target, health endpoint, port, bind address. No process.env reads. - dashboard-health.ts: verifyDashboardChain() checks gateway, forward, and CORS links with injected deps. Accepts HTTP 200/401 as alive. - dashboard-recover.ts: recoverDashboardChain() — link-aware, idempotent recovery (gateway → forward order, CORS diagnose-only). Key changes: - Delete src/lib/dashboard.ts (replaced by dashboard-contract.ts) - Delete 7 dashboard helpers from onboard.ts (~90 lines) - Rewrite ensureDashboardForward to use buildChain() - Fix isSandboxGatewayRunning to probe /health and accept 401 (NVIDIA#2342) - Delete recoverSandboxProcesses/ensureSandboxPortForward from nemoclaw.ts - Wire checkAndRecoverSandboxProcesses to recoverDashboardChain() - 52 new tests (32 contract + 13 health + 7 recovery) Fixes NVIDIA#2390 Fixes NVIDIA#2342 Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
jyaunches
force-pushed
the
issue-2390-dashboard-delivery-contract
branch
from
fca502a to
56729ef
Compare
Contributor
|
LGTM 🚢 Pulled locally:
Contract split is clean, #2342 regression test in place. No security regressions. |
brandonpelfrey
approved these changes
Apr 24, 2026
6 tasks
3 tasks
6 tasks
2 tasks
This was referenced Apr 25, 2026
This was referenced Apr 25, 2026
ericksoa
added a commit
that referenced
this pull request
Apr 25, 2026
## Summary - Reverts the `nemoclaw.ts` changes from #2398 that caused `nemoclaw status` to hang indefinitely - Restores the original `isSandboxGatewayRunning`, `recoverSandboxProcesses`, `ensureSandboxPortForward`, and `checkAndRecoverSandboxProcesses` implementations - Leaves the dashboard-contract/health/recover modules in place (onboard.ts depends on them) ## Root cause PR #2398 replaced the simple gateway recovery path with `recoverDashboardChain()` which introduced unbounded calls that hang in CI. The bisect confirmed #2398 as the sole culprit: | Run | Commit | Result | |-----|--------|--------| | Last good | `de97a00d` (Apr 24 16:06) | pass | | Bisect 4 | `9fbfbaca` (#2398 only) | **hang** | | First bad | `79c8e2a9` (Apr 25 00:10) | hang | sandbox-survival, skip-permissions, sandbox-operations, and cloud-e2e all hang at `nemoclaw <name> status` after #2398. ## What this reopens - #2342 (false "Offline" with device auth) — the old `curl -sf /` probe returns failure on 401. This needs a targeted fix, not a refactor. - #2390 — dashboard delivery chain consolidation goal. Can be re-attempted with proper E2E validation. ## Test plan - [ ] `npx tsc -p tsconfig.src.json --noEmit` passes - [ ] Dashboard unit tests pass - [ ] Nightly E2E: sandbox-survival, skip-permissions, sandbox-operations, cloud-e2e pass <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved gateway health check reliability by using explicit status outputs instead of HTTP code interpretation. * Enhanced automatic recovery mechanism for sandbox gateway outages with better restart and port forwarding restoration. * Updated diagnostic logging to better reflect gateway restart and connectivity restoration activities. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This was referenced Apr 25, 2026
Contributor
This PR was reverted in #2471@jyaunches — the Root causeThe new Additionally, What was revertedOnly the To re-land this
|
This was referenced Apr 26, 2026
jyaunches
added a commit
that referenced
this pull request
Apr 27, 2026
…d) (#2495) ## ⚡ Interim PR — Unblocks #2306 This is an **interim extraction PR** that lands the foundational module decomposition needed by #2306 (extract rebuild/recreate path + canonicalize credential resolution). It is intentionally scoped to pure code movement with no behavior changes. Rebased onto current main. Supersedes #2087 and #2489. --- ## Summary Steps 1–3 of the original extraction plan (authored by @ericksoa in #2087): - Extract `onboard-providers.ts` — provider metadata, CRUD, labels, inference config - Extract `onboard-ollama-proxy.ts` — proxy lifecycle, model pull/validate - Extract `onboard-inference-probes.ts` — endpoint validation probes (OpenAI, Anthropic, Gemini) Reduces `onboard.ts` by ~1,000 lines. All 3 new modules are under 400 lines. No behavior changes — all existing exports remain accessible via re-exports and thin wrappers. **Step 4 (dashboard extraction) was dropped** — main already landed `dashboard-contract.ts`, `dashboard-health.ts`, and `dashboard-recover.ts` (#2398) which supersede that extraction. Steps 5–10 (gateway, preflight, messaging, policies, inference-setup, sandbox) remain deferred per the original plan. ## Downstream: #2306 `#2306` (extract rebuild/recreate path + canonicalize credential resolution) builds directly on top of this PR. It imports `REMOTE_PROVIDER_CONFIG`, `upsertProvider`, and `ensureOllamaAuthProxy` from the new extracted modules via onboard.ts re-exports. Merging this PR first gives #2306 a clean module surface to build on. ## CodeRabbit comments All 6 comments addressed: - 1 resolved (deleted planning doc) - 4 are valid improvements on **pre-existing code** that was moved as-is — intentionally out of scope for a pure extraction PR, flagged as follow-up material - 1 is incorrect (`upsertMessagingProviders` was never exported on main) ## Rebase notes - Ported all main-side refinements into the extracted modules (e.g., `startOllamaAuthProxy` now returns `boolean`, `getProbeAuthMode` always returns `undefined`) - Added type annotations for the CJS `require()` imports to satisfy strict TypeScript - Re-imported endpoint URL constants that `onboard.ts` still references after extraction ## Test plan - [x] `npx tsc -p tsconfig.src.json --noEmit` compiles cleanly - [x] `npx tsc -p tsconfig.cli.json --noEmit` compiles cleanly (pre-existing failures only, same as main) - [x] Affected tests pass: gemini-probe-auth, credential-exposure, wsl2-probe-timeout - [x] CI: all 15 checks green - [ ] Manual smoke test: `nemoclaw onboard` completes successfully Co-authored-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Reorganized onboarding logic into modular utilities for improved code maintainability and separation of concerns. * Separated provider management, inference endpoint validation, and Ollama proxy lifecycle into dedicated modules. * **Tests** * Updated tests to reflect new code organization structure. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Aaron Erickson <aerickson@nvidia.com>
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
4 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
Extract the scattered dashboard delivery chain logic — URL derivation, CORS origins, port forwarding target, health probing, and recovery — from the monolith files (onboard.ts, nemoclaw.ts) into three focused modules that serve as the single source of truth for dashboard reachability. Fixes #2342 false "Offline" reading when device auth is enabled.
Related Issue
Fixes #2390
Fixes #2342
Changes
src/lib/dashboard-contract.ts: PurebuildChain()function — single source of truth for all dashboard config (access URL, CORS origins, forward target, health endpoint, port, bind address). Noprocess.envreads.src/lib/dashboard-health.ts:verifyDashboardChain()checks all 3 delivery chain links (gateway, forward, CORS) with injected deps. Accepts HTTP 200 and 401 as "alive" (fixes [NemoClaw][Brev Launchable] OpenClaw Gateway Dashboard shows "Version n/a" and "Health Offline" after Brev Launchable deployment succeeds #2342).src/lib/dashboard-recover.ts:recoverDashboardChain()— link-aware, idempotent recovery that only fixes broken links (gateway → forward, CORS diagnose-only).src/lib/dashboard.ts— replaced by dashboard-contract.ts.src/lib/onboard.ts(~90 lines removed):getDashboardForwardPort,getDashboardForwardTarget,getDashboardForwardStartCommand,buildAuthenticatedDashboardUrl,getWslHostAddress,getDashboardAccessInfo,getDashboardGuidanceLines.ensureDashboardForwardin onboard.ts to usebuildChain()instead of re-deriving config.isSandboxGatewayRunning()in nemoclaw.ts to probe/healthand accept 401 as alive.recoverSandboxProcesses()andensureSandboxPortForward()from nemoclaw.ts —checkAndRecoverSandboxProcesses()delegates torecoverDashboardChain().test/onboard.test.ts— imports from new modules, assertions rewritten for contract API.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)AI Disclosure
Signed-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor