fix(dashboard): keep resumed chat alive on network binds - #26265
hehehe0803 wants to merge 7 commits into
Conversation
b408a82 to
6d47206
Compare
6d47206 to
9fca80e
Compare
|
Rebased/retested this fix against current What changed in the update:
Verification run locally on current I also checked the diff for private/local addresses and did not include any real LAN/VPN IPs, hostnames, local user paths, or credentials. One unrelated note from the broader retest: |
|
Please use PULL_REQUEST_TEMPLATE.md |
There was a problem hiding this comment.
Pull request overview
This PR fixes the dashboard’s “Sessions → Resume in Chat” behavior when the dashboard is intentionally bound to a specific non-loopback host (LAN/VPN), ensuring the embedded PTY WebSocket can stay connected and that repeat resume attempts can reliably restart a stale terminal.
Changes:
- Allow
/api/ptyWebSocket clients when the dashboard is bound to an explicit non-loopback host (still guarded by the session token). - Add a
resumeNoncequery param so repeated “Resume in Chat” clicks for the same session force a fresh PTY identity in the still-mounted ChatPage. - Validate that the Node binary used for dashboard-spawned TUI is new enough, so old system Node versions don’t get treated as “good enough”.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/pages/SessionsPage.tsx | Adds resumeNonce to chat navigation query to force a fresh resume attempt. |
| web/src/pages/ChatPage.tsx | Incorporates resumeNonce into the PTY “identity” so the WS/terminal is rebuilt without unmounting the page. |
| tests/hermes_cli/test_web_server.py | Adds regression coverage for explicit network-host binds and the WS client allow-guard behavior. |
| hermes_cli/web_server.py | Broadens the “public bind” determination to include any non-loopback bind (not only 0.0.0.0/::). |
| hermes_cli/main.py | Adds Node version probing so unsupported system Node is treated as unusable and triggers bootstrap. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const qs = new URLSearchParams({ | ||
| resume: session.id, | ||
| resumeNonce: String(Date.now()), |
| // SessionsPage may intentionally navigate to the *same* resume target again | ||
| // after a previous dashboard PTY has ended. React keeps ChatPage mounted | ||
| // persistently so ordinary tab switches preserve the live PTY, which also | ||
| // means a same-URL resume click would otherwise leave the stale terminal on | ||
| // screen forever. `resumeNonce` is a route-level "start a fresh PTY for this | ||
| // resume click" signal; it is not forwarded to the backend. | ||
| const resumeNonce = searchParams.get("resumeNonce"); | ||
| const channel = useMemo( | ||
| () => | ||
| ["chat", resumeParam ?? "new", resumeNonce ?? "0", generateChannelId()].join( | ||
| "_", | ||
| ), |
| if _node_is_usable(shutil.which("node")) and shutil.which("npm"): | ||
| return | ||
| if os.environ.get("HERMES_SKIP_NODE_BOOTSTRAP"): | ||
| return |
9fca80e to
6c1415e
Compare
|
Updated this PR against current main and addressed the review feedback:
Verification run locally: I also ran |
|
Follow-up pushed to address the failing CI test job on commit Root causes from the failed test log:
Local verification:
CI has restarted; most checks are already green, with the long |
2b9d2b2 to
856135b
Compare
|
Follow-up after the latest CI failure:
All checks are green; PR merge state is |
079be7c to
b0e4d8a
Compare
b0e4d8a to
77c45a2
Compare
|
Thanks for documenting the explicit private-interface reproduction and adding targeted regression coverage. This is already implemented on current
|
What does this PR do?
Fixes dashboard embedded chat resume when the dashboard is intentionally served on an explicit non-loopback network host, such as a private LAN/VPN address.
The existing merged WebSocket fix covers all-interface binds (
0.0.0.0/::), but not the common explicit private-interface bind shape:In that mode the HTTP dashboard can load, while
/api/ptycloses before accept because the WebSocket client-IP gate still behaves like the dashboard is loopback-only. This can present as Sessions → Resume in Chat opening a chat page that immediately shows[session ended], reconnect/disconnect UI, or a spawned TUI runtime error even though the underlying session is still valid.This PR:
--insecureoperator opt-in onapp.state.allow_publicand uses it for dashboard PTY WebSocket admission;resumeNonceso repeated Sessions → Resume in Chat clicks for the same session can restart a stale PTY identity;resumeNonceis only folded into the opaque dashboard event channel id and is not interpreted as backend resume state;HERMES_SKIP_NODE_BOOTSTRAPis set, surfacing a clear error instead of letting the embedded TUI crash later;Related context: #18633, #25072, #27801.
Related Issue
Fixes #26264
Type of Change
Changes Made
hermes_cli/web_server.pyallow_publicfromstart_server()ontoapp.state._ws_client_is_allowed()accept non-loopback clients only when the dashboard was intentionally exposed with--insecureor bound to all interfaces.tests/hermes_cli/test_web_server.py--insecureopt-in.bound_host/allow_publicin the PTY WebSocket test fixture so tests cannot pass because of leaked global app state.web/src/pages/SessionsPage.tsxresumeNoncewithcrypto.randomUUID()when available, with a timestamp/random fallback.web/src/pages/ChatPage.tsxhermes_cli/main.pyHERMES_SKIP_NODE_BOOTSTRAPis set but the available Node/npm runtime is unusable for the dashboard-spawned TUI.tests/hermes_cli/test_tui_resume_flow.pyHow to Test
Automated checks run on this branch:
Python targeted tests:
Result:
40 passed.Python syntax check:
Result: passed.
Web production build:
Result: passed.
Manual/browser verification shape:
[session ended], reconnect-only UI, or an unsupported Node runtime crash.Additional check:
cd web && npm run lintwas also run. It still fails on the current repository lint baseline in unrelated files/rules (for exampleOAuthProvidersCard.tsx,Toast.tsx, i18n files, and existingSessionsPage.tsxeffect warnings). No new lint-specific issue was introduced by this change.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs