feat(hermes): expose optional web dashboard#4442
Conversation
Signed-off-by: Ashish Hunnargikar <ahunnargikar@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:
📝 WalkthroughWalkthroughAdds optional Hermes web dashboard: manifest ChangesHermes Web Dashboard
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
|
🌿 Preview your docs: https://nvidia-preview-pr-4442.docs.buildwithfern.com/nemoclaw |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
E2E Scenario Advisor RecommendationRequired scenario E2E: None Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
PR Review AdvisorFindings: 0 needs attention, 13 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
Signed-off-by: Ashish Hunnargikar <ahunnargikar@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/lib/hermes-dashboard.test.ts (1)
38-44: ⚡ Quick winAdd boundary assertions for port-range validation.
This suite only checks non-numeric input. Add explicit assertions for out-of-range numeric values (e.g.,
1023,65536) to lock the port contract and prevent regressions.Suggested test addition
it("rejects invalid port values", () => { expect(() => readHermesDashboardConfig({ NEMOCLAW_HERMES_DASHBOARD_PORT: "abc", }), ).toThrow(/NEMOCLAW_HERMES_DASHBOARD_PORT/); }); + + it("rejects out-of-range dashboard port values", () => { + expect(() => + readHermesDashboardConfig({ + NEMOCLAW_HERMES_DASHBOARD_PORT: "1023", + }), + ).toThrow(/NEMOCLAW_HERMES_DASHBOARD_PORT/); + + expect(() => + readHermesDashboardConfig({ + NEMOCLAW_HERMES_DASHBOARD_PORT: "65536", + }), + ).toThrow(/NEMOCLAW_HERMES_DASHBOARD_PORT/); + }); });🤖 Prompt for 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. In `@src/lib/hermes-dashboard.test.ts` around lines 38 - 44, Extend the "rejects invalid port values" test for readHermesDashboardConfig to also assert numeric out-of-range ports are rejected: within the same it block (or a new it near it) call readHermesDashboardConfig with NEMOCLAW_HERMES_DASHBOARD_PORT: "1023" and with "65536" and assert each invocation throws (toThrow) matching the existing port-related error (e.g., /NEMOCLAW_HERMES_DASHBOARD_PORT/ or the module's port-range message) so the contract enforces the valid port range.src/lib/actions/sandbox/hermes-dashboard-recovery.test.ts (1)
45-77: ⚡ Quick winAdd explicit coverage for the
"occupied"forward-health branch.This test block currently doesn’t assert the non-takeover path when
isPortForwardHealthyreturns"occupied"(Line 56/65/73 only exercisefalse/true). Please add one case asserting returnfalseand thatensurePortForwardis not called.✅ Suggested test addition
it("restarts the dashboard forward only when the recorded forward is unhealthy", () => { const ensurePortForward = vi.fn(() => true); const getRecoveryConfig = () => ({ publicPort: 9119, internalPort: 19119, tuiEnabled: false, }); @@ expect( ensureHermesDashboardPortForwardIfEnabled("alpha", { getRecoveryConfig, isPortForwardHealthy: () => true, ensurePortForward, }), ).toBe(false); + const ensurePortForwardWhenOccupied = vi.fn(() => true); + expect( + ensureHermesDashboardPortForwardIfEnabled("alpha", { + getRecoveryConfig, + isPortForwardHealthy: () => "occupied", + ensurePortForward: ensurePortForwardWhenOccupied, + }), + ).toBe(false); + expect(ensurePortForwardWhenOccupied).not.toHaveBeenCalled(); + expect( ensureHermesDashboardPortForwardIfEnabled("alpha", { getRecoveryConfig: () => null, isPortForwardHealthy: () => false, ensurePortForward, }), ).toBeNull(); });🤖 Prompt for 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. In `@src/lib/actions/sandbox/hermes-dashboard-recovery.test.ts` around lines 45 - 77, Add a test case in the existing "restarts the dashboard forward only when the recorded forward is unhealthy" block to cover the "occupied" branch: call ensureHermesDashboardPortForwardIfEnabled("alpha", { getRecoveryConfig, isPortForwardHealthy: () => "occupied", ensurePortForward }) and assert it returns false and that the mock ensurePortForward was not called; reference the existing test helpers ensureHermesDashboardPortForwardIfEnabled, getRecoveryConfig and ensurePortForward to locate where to insert the assertion.agents/hermes/start.sh (1)
854-917: Run the Hermes E2E matrix for lifecycle/forwarding confidence.Given this startup-path change wires dashboard process + port forwarding + cleanup in both privilege paths, run the Hermes-specific E2E jobs before merge to validate onboarding, health probes, and live routing behavior end-to-end.
As per coding guidelines,
agents/hermes/**changes affect multi-agent onboarding, health probes, and inference routing, and include explicit Hermes E2E recommendations.🤖 Prompt for 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. In `@agents/hermes/start.sh` around lines 854 - 917, Run the Hermes E2E matrix to validate the changes that wire dashboard process, port forwarding and cleanup in both privilege paths: execute the Hermes E2E jobs covering onboarding, health probes, and live routing and confirm behavior for the flows that exercise start_hermes_dashboard_current_user and start_hermes_dashboard_sandbox_user, start_socat_forwarder forwarding (api/PUBLIC_PORT/INTERNAL_PORT), and cleanup_on_signal/SANDBOX_CHILD_PIDS handling; report any failures so we can fix lifecycle/forwarding, PID collection races, or log/port binding regressions before merge.
🤖 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 `@agents/hermes/start.sh`:
- Around line 157-164: Add two additional port-collision checks in start.sh to
catch cross-binding conflicts: verify HERMES_DASHBOARD_PUBLIC_PORT is not equal
to INTERNAL_PORT and HERMES_DASHBOARD_INTERNAL_PORT is not equal to PUBLIC_PORT;
if either equality holds, emit a clear error to stderr (similar format to the
existing checks) and exit 1. Use the same numeric comparison operator (-eq) and
mirror the error message pattern that references the conflicting variable names
(HERMES_DASHBOARD_PUBLIC_PORT vs INTERNAL_PORT and
HERMES_DASHBOARD_INTERNAL_PORT vs PUBLIC_PORT) so the script fails fast on
cross-collisions.
In `@docs/get-started/quickstart-hermes.mdx`:
- Around line 90-93: Replace the non-copyable console fences with copyable
bash/sh fences and remove the leading "$" prompt markers in the two code blocks:
change the block that currently shows "$ export NEMOCLAW_HERMES_DASHBOARD=1" and
"$ nemohermes onboard" to a ```bash (or ```sh) block containing the two lines
without "$", and likewise change the block that shows "$ openshell forward start
--background 9119 my-hermes" to a ```bash (or ```sh) block with that command
without the "$"; ensure only the language tag is used and no prompt markers
remain so users can copy the commands directly.
In `@src/lib/agent/dashboard-ui.ts`:
- Around line 82-85: The env-port parser currently accepts ports 1..65535
(checks raw and sets const port) which conflicts with the shared Hermes parser
that requires 1024..65535; update the validation in the same function (the block
that reads raw, tests /^\d+$/, assigns const port) to enforce port >= 1024 &&
port <= 65535 and return only in that case so the dashboardUiPort behavior
matches the shared Hermes env parser.
In `@src/lib/onboard.ts`:
- Around line 2820-2828: hermesDashboardState is computed using effectivePort
before ensureDashboardForward() may rebind to a different host port, causing
stale config to be used later; move the call to
onboardHermesDashboard.resolveHermesDashboardOnboardState (or recompute
hermesDashboardState) after ensureDashboardForward() completes and returns the
finalized host port, and update all downstream uses (drift checks and registry
writes) to read the newly computed hermesDashboardState so they operate on the
final, authoritative port value.
- Around line 2829-2840: The rollback lambda passed into
onboardHermesDashboard.createHermesDashboardForwardEnsurer only deletes the
sandbox but must also remove any already-created Hermes dashboard forward to
avoid orphaned host forwards; update the rollbackSandbox function to call the
same cleanup used for forwards (e.g., invoke runOpenshell to delete the
dashboard forward) before or after deleting the sandbox, using the targetSandbox
value and preserve { ignoreError: true } so failures don't block rollback;
locate ensureHermesDashboardForwardIfEnabled /
createHermesDashboardForwardEnsurer and modify the rollbackSandbox arrow
function to run both the dashboard-forward delete command and the existing
sandbox delete command.
In `@test/onboard-dashboard.test.ts`:
- Around line 98-116: The test currently maps runOpenshell.mock.calls to
stopArgs and only checks there is no 3-arg global stop; update the assertions to
ensure any "forward stop" invocation is scoped to "hermes-sandbox" (or exactly
matches the expected ["forward","stop","9119","hermes-sandbox"]). Specifically,
after computing stopArgs (from runOpenshell.mock.calls), add an assertion that
for every args where args[0]==="forward" && args[1]==="stop", either args equals
the expected four-item array or args[args.length-1] === "hermes-sandbox" (i.e.,
no stop call targets a different sandbox); reference runOpenshell.mock.calls and
the stopArgs variable to locate where to add this check.
---
Nitpick comments:
In `@agents/hermes/start.sh`:
- Around line 854-917: Run the Hermes E2E matrix to validate the changes that
wire dashboard process, port forwarding and cleanup in both privilege paths:
execute the Hermes E2E jobs covering onboarding, health probes, and live routing
and confirm behavior for the flows that exercise
start_hermes_dashboard_current_user and start_hermes_dashboard_sandbox_user,
start_socat_forwarder forwarding (api/PUBLIC_PORT/INTERNAL_PORT), and
cleanup_on_signal/SANDBOX_CHILD_PIDS handling; report any failures so we can fix
lifecycle/forwarding, PID collection races, or log/port binding regressions
before merge.
In `@src/lib/actions/sandbox/hermes-dashboard-recovery.test.ts`:
- Around line 45-77: Add a test case in the existing "restarts the dashboard
forward only when the recorded forward is unhealthy" block to cover the
"occupied" branch: call ensureHermesDashboardPortForwardIfEnabled("alpha", {
getRecoveryConfig, isPortForwardHealthy: () => "occupied", ensurePortForward })
and assert it returns false and that the mock ensurePortForward was not called;
reference the existing test helpers ensureHermesDashboardPortForwardIfEnabled,
getRecoveryConfig and ensurePortForward to locate where to insert the assertion.
In `@src/lib/hermes-dashboard.test.ts`:
- Around line 38-44: Extend the "rejects invalid port values" test for
readHermesDashboardConfig to also assert numeric out-of-range ports are
rejected: within the same it block (or a new it near it) call
readHermesDashboardConfig with NEMOCLAW_HERMES_DASHBOARD_PORT: "1023" and with
"65536" and assert each invocation throws (toThrow) matching the existing
port-related error (e.g., /NEMOCLAW_HERMES_DASHBOARD_PORT/ or the module's
port-range message) so the contract enforces the valid port range.
🪄 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: 005f83fc-1431-4869-ab1f-0a24474e5f3e
📒 Files selected for processing (27)
agents/hermes/Dockerfile.baseagents/hermes/manifest.yamlagents/hermes/start.shdocs/get-started/quickstart-hermes.mdxdocs/reference/commands.mdxsrc/lib/actions/sandbox/forward-health.tssrc/lib/actions/sandbox/hermes-dashboard-recovery.test.tssrc/lib/actions/sandbox/hermes-dashboard-recovery.tssrc/lib/actions/sandbox/process-recovery.tssrc/lib/agent/dashboard-ui.tssrc/lib/agent/defs.test.tssrc/lib/agent/defs.tssrc/lib/agent/onboard.test.tssrc/lib/agent/onboard.tssrc/lib/agent/runtime.test.tssrc/lib/agent/runtime.tssrc/lib/hermes-dashboard.test.tssrc/lib/hermes-dashboard.tssrc/lib/onboard.tssrc/lib/onboard/agent-fixed-forward.tssrc/lib/onboard/dashboard.tssrc/lib/onboard/hermes-dashboard.test.tssrc/lib/onboard/hermes-dashboard.tssrc/lib/onboard/landlock-warning.tssrc/lib/state/registry.tstest/hermes-start.test.tstest/onboard-dashboard.test.ts
Selective E2E Results — ❌ Some jobs failedRun: 26603871082
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Selective E2E Results — ✅ All requested jobs passedRun: 26604431598
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Selective E2E Results — ✅ All requested jobs passedRun: 26604622802
|
|
Actionable comments posted: 0 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/onboard.ts`:
- Around line 3113-3117: The hermes dashboard ensure path uses a closure
capturing the old chatUiUrl so when reusing a dashboard forward you must update
chatUiUrl (and any related env var) to the new port before calling
hermesDashboardForwarding.ensureForState; specifically, after calling
ensureDashboardForward(sandboxName, chatUiUrl) and computing reusedPort via
hermesDashboardForwarding.resolveStateForPort(reusedPort), assign chatUiUrl to
the new URL (e.g., `http://127.0.0.1:${reusedPort}`) and update
process.env.CHAT_UI_URL prior to invoking
hermesDashboardForwarding.ensureForState(reusedHermesDashboardState,
sandboxName); apply the same change in the interactive reuse branch as well so
ensureForState sees the updated API port.
🪄 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: 9a3e45ad-943c-4be7-a851-0147e48e69ca
📒 Files selected for processing (3)
src/lib/onboard.tssrc/lib/onboard/hermes-dashboard.tstest/process-recovery.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/process-recovery.test.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Selective E2E Results — ❌ Some jobs failedRun: 26604709304
|
Selective E2E Results — ❌ Some jobs failedRun: 26605357981
|
|
Actionable comments posted: 0 |
Selective E2E Results — ❌ Some jobs failedRun: 26605382605
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Selective E2E Results — ✅ All requested jobs passedRun: 26608228607
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Selective E2E Results — ✅ All requested jobs passedRun: 26608758607
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Selective E2E Results — ✅ All requested jobs passedRun: 26609084519
|
ericksoa
left a comment
There was a problem hiding this comment.
Approved. Re-checked current head 3fd5e5f: required PR checks pass, CodeRabbit completed, no unresolved non-outdated review threads, and the branch-ref Hermes dashboard E2E succeeded. The dashboard/parser fixes look scoped and validated end to end.
## Summary Refreshes the NemoClaw documentation for the v0.0.54 release and regenerates user skills from the Fern MDX source. Also keeps the Fern CLI pin current so local docs checks use the upgraded Fern version. ## Related Issue <!-- No single related issue. This is release-prep documentation catch-up. --> ## Changes - #4403 -> `docs/manage-sandboxes/messaging-channels.mdx`, `docs/reference/commands.mdx`, `docs/about/release-notes.mdx`: Document Telegram, Discord, and Slack post-rebuild bridge verification and summarize channel activation fixes. - #4222 -> `docs/about/release-notes.mdx`: Include Slack generated channel enablement in the v0.0.54 messaging summary. - #4346 -> `docs/get-started/windows-preparation.mdx`, `docs/about/release-notes.mdx`: Document safer Windows bootstrap behavior for Ubuntu first-run and Docker Desktop WSL integration. - #4416 -> `docs/inference/use-local-inference.mdx`, `docs/about/release-notes.mdx`: Document the Docker Desktop WSL requirement for Windows-host Ollama. - #4442 -> `docs/about/release-notes.mdx`: Summarize the optional NemoHermes native web dashboard and related environment variables. - #4426 -> `docs/about/release-notes.mdx`: Summarize copy-paste recovery hints for invalid sandbox names and missing NVIDIA API keys. - #4459 -> `docs/about/release-notes.mdx`: Summarize the Linuxbrew prefix fix for sandbox Homebrew usage. - #4450 -> `docs/about/release-notes.mdx`: Summarize `/nemoclaw` slash command startup activation. - #4468 -> `docs/about/release-notes.mdx`: Summarize scope-upgrade approval recovery. - #4325 -> `docs/about/release-notes.mdx`: Summarize the narrowed `web_fetch` host-gateway allowance. - #4474 -> `docs/about/release-notes.mdx`: Summarize Hermes Provider smoke-check behavior for OAuth versus Nous API key setup. - Refresh generated `.agents/skills/nemoclaw-user-*` references from `docs/` and update `fern/fern.config.json` to Fern `5.41.2`. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification <!-- Check each item you ran and confirmed. Leave unchecked items you skipped. Doc-only changes do not require npm test unless you ran it. --> - [ ] `npx prek run --all-files` passes - [ ] `npm test` passes - [ ] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [x] Docs updated for user-facing behavior changes - [ ] `npm run docs` builds without warnings (doc changes only) - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- <!-- DCO sign-off required by CI. Run: git config user.name && git config user.email --> Signed-off-by: Miyoung Choi <miyoungc@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Optional NemoHermes native web dashboard (configurable port and TUI) * GPU memory cleanup now unloads Ollama models when switching providers or stopping services * **Bug Fixes** * Improved sandbox name validation with suggested slug recovery * Windows-host Ollama now requires Docker Desktop WSL integration and exits with remediation guidance when unsupported * **Documentation** * Clarified quickstart/onboard flow, installer TTY/non‑TTY guidance, Hermes Docker prerequisites, sandbox hardening, and channels add rebuild checks <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/4539?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
🥳 Thank you! |
Summary
Adds an opt-in NemoHermes native web dashboard path so Hermes sandboxes can expose the upstream browser dashboard separately from the OpenAI-compatible API. The API remains on port 8642, while the dashboard defaults to port 9119 when enabled.
Related Issue
Fixes #2979
Changes
NEMOCLAW_HERMES_DASHBOARD, dashboard port, internal port, and optional TUI mode.hermes dashboard --no-openinside Hermes sandboxes and bridge it through a separate host forward on port 9119.Type of Change
Verification
Passed:
npm run build:clinpm run typecheck:clinpm run lint(passes with one existing Biome warning insrc/lib/onboard/child-exit-tracker.test.ts)npx vitest run --project cli src/lib/hermes-dashboard.test.ts src/lib/agent/defs.test.ts src/lib/agent/onboard.test.ts test/onboard-dashboard.test.ts test/process-recovery.test.tsnpx vitest run --project cli test/hermes-start.test.ts --reporter verbosenpm run docs(0 errors, 2 existing Fern warnings)Attempted:
npx prek run --all-filesis blocked locally because the global gitignore marks tracked lockfiles as ignored andhadolintis not installed.npm testfull suite was attempted outside the sandbox; one unrelatedtest/sandbox-connect-inference.test.tscase timed out once, and that file passed on isolated rerun.npx prek run --all-filespassesnpm testpassesTests added or updated for new or changed behavior
No secrets, API keys, or credentials committed
Docs updated for user-facing behavior changes
npm run docsbuilds without warnings (doc changes only)Doc pages follow the style guide (doc changes only)
New doc pages include SPDX header and frontmatter (new pages only)
Signed-off-by: Ashish Hunnargikar ahunnargikar@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests