Skip to content

fix(tui): avoid stdio fallback for detached websocket sessions - #42136

Closed
joshuadow wants to merge 2 commits into
NousResearch:mainfrom
joshuadow:fix/tui-ws-detached-session-transport
Closed

fix(tui): avoid stdio fallback for detached websocket sessions#42136
joshuadow wants to merge 2 commits into
NousResearch:mainfrom
joshuadow:fix/tui-ws-detached-session-transport

Conversation

@joshuadow

@joshuadow joshuadow commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Refreshes this PR onto current main after the detached-WebSocket implementation landed through overlapping changes.
  • Keeps the current main implementation (_detached_ws_transport plus _close_sessions_for_transport) as the source of truth.
  • Adds a focused regression test proving a running reconnectable session detached from a WebSocket does not write stream events to dashboard stdout/journald.
  • Updates a stale prompt-submit comment to refer to the detached-WS drop transport instead of stdio fallback.

Relationship to adjacent PRs

Conflict-resolution notes

  • origin/main already contains the backend implementation for detached WS sessions.
  • The merge conflicts were resolved in favor of current main for the implementation files.
  • The original duplicate DroppingTransport / _detach_sessions_for_transport approach from this branch was dropped.
  • The remaining PR diff is intentionally small: tests/test_tui_gateway_ws.py plus a comment-only update in tui_gateway/server.py.

Test Plan

  • Focused regression:
    • venv/bin/python -m pytest tests/test_tui_gateway_ws.py::test_ws_disconnect_detached_running_session_events_do_not_write_to_stdout -q
    • 1 passed in 0.25s
  • Syntax / whitespace:
    • python3 -m compileall -q tui_gateway/server.py tui_gateway/ws.py tui_gateway/transport.py
    • compileall ok
    • git diff --check
    • no output
  • Related TUI gateway suite via repo wrapper:
    • scripts/run_tests.sh tests/test_tui_gateway_ws.py tests/test_tui_gateway_server.py tests/tui_gateway/test_protocol.py
    • 309 tests passed, 0 failed in 8.2s

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tui Terminal UI (ui-tui/ + tui_gateway/) labels Jun 8, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

Positive Verification — DroppingTransport for Detached WebSocket Sessions

Reviewed the diff (tui_gateway/server.py, tui_gateway/transport.py, tui_gateway/ws.py, tests/test_tui_gateway_server.py, tests/tui_gateway/test_protocol.py).

Root cause correctly identified: In the dashboard's in-process gateway, stdout is journald (not a TUI reader). Falling back detached WS sessions to _stdio_transport leaked raw JSON-RPC stream frames into service logs and left Desktop out of sync until reconnect.

DroppingTransport is the right pattern: A zero-overhead sentinel that reports success (write()True) while discarding frames. This keeps the running agent safe — no error propagation from a dead transport — until session.resume/prompt.submit rebinds a live client transport.

Orphan detection updated correctly: _ws_session_is_orphaned() now checks against _detached_ws_transport instead of _stdio_transport. The _finalized and running guards prevent false positives on mid-turn or already-cleaned-up sessions.

Test coverage: The integration test test_ws_detach_drops_running_session_events_instead_of_falling_back_to_stdio validates the full flow: detach → assert no stdout leakage → assert orphaned after idle → assert not orphaned while running.

No issues found.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved

Looks Good

  • Fixes detached WS session handling: replaces unsafe fallback to _stdio_transport (which leaks JSON-RPC stream frames into journald in the dashboard service) with a new DroppingTransport that silently discards frames.
  • Dedicated test test_ws_detach_drops_running_session_events_instead_of_falling_back_to_stdio verifies orphaned sessions don't write to stdout.
  • Refactored _detach_sessions_for_transport() helper for cleaner code organization.
  • No security concerns, no debug artifacts.

Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for preserving the regression scenario. This automated hermes-sweeper review found that the detached-WebSocket behavior is already implemented and covered on current main.

  • cef00ae602a8aa1311d55edfad4e322c4ff99993 (#41953, shipped in v2026.6.19) introduced _detached_ws_transport specifically to prevent disconnected WebSocket session frames from falling through to desktop stdout.
  • tui_gateway/server.py:820-823 assigns reconnectable disconnected sessions to that drop transport; tui_gateway/server.py:1119-1137 routes session event frames through it.
  • Existing tests cover the relevant guarantee: tests/test_tui_gateway_server.py:345-357 verifies no stdout output for detached-session frames, and tests/test_tui_gateway_server.py:8383-8401 verifies disconnect cleanup repoints reconnectable sessions to the detached transport.

Closing as already implemented on main.

@teknium1 teknium1 closed this Jul 14, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists sweeper:implemented-on-main Sweeper: behavior already present on current main type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants