Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for preserving the 16 MiB preview limit while adding a bounded upload-specific reader. Current main does reproduce the reported issue: remote non-image uploads use the preview reader at apps/desktop/src/app/session/hooks/use-prompt-actions/utils.ts:133, and Electron caps that reader at 16 MiB in apps/desktop/electron/hardening.ts:7.
Problems
- The new
ws_max_sizecapability changes the Desktop/backend compatibility boundary, but the PR does not bump it.tui_gateway/server.py:3582-3588saysDESKTOP_BACKEND_CONTRACTmust be bumped when that contract changes; it remains v3. Desktop also still accepts v3 as current atapps/desktop/src/store/updates.ts:90-95. A new Desktop can therefore use the new 256 MiB reader against an older v3 remote backend, whosestart_server()still lacksws_max_size(hermes_cli/web_server.py:17994-18010), and the upload will fail at the old frame limit.
Suggested changes
- Bump both producer and consumer contract constants to v4 and update their tests, so older remote backends surface the established skew/update warning. If transparent compatibility is required, add a version/capability fallback that keeps the 16 MiB reader for pre-v4 backends.
The member note links this bounded path to #62375 and #62382; versioning this requirement makes that compatibility boundary explicit. This is an automated hermes-sweeper review.
| # reaped via the WebSocketDisconnect → disconnect/reap path. | ||
| ws_ping_interval=None if _is_loopback else 20.0, | ||
| ws_ping_timeout=None if _is_loopback else 20.0, | ||
| ws_max_size=_DESKTOP_ATTACHMENT_WS_MAX_BYTES, |
There was a problem hiding this comment.
This changes the remote Desktop/backend wire capability, but tui_gateway/server.py still advertises contract v3 and apps/desktop/src/store/updates.ts accepts v3. Please bump both to v4 (and test it), or add a pre-v4 fallback; otherwise a new Desktop can send a >16 MiB frame to an old v3 backend that still uses Uvicorn's default cap.
|
Closeout review found this PR is not merge-safe yet. The 16 MiB preview / 256 MiB attachment-reader split and 384 MiB WebSocket cap pass focused local gates, but the required backend-contract bump collides with the independent v5 semantics in #69508. Contract levels are cumulative, so this branch cannot truthfully claim v5 or v6 until #69508 lands and origin/main is merged here. I have therefore not pushed the local integration/contract patch. Required sequence: land #69508; refresh this branch from resulting main; assign the next aggregate contract; add exact upper-bound rejection coverage; rerun Python/Desktop gates and independent review. Current upstream head remains 5ac55ec. |
|
Superseded by #73710. Rebased onto current main after #73221 reshaped the Settings-configurable preview reader, kept the dedicated 256 MiB attach IPC + 384 MiB You're credited via |
…tach fix(desktop): allow large remote attachments (supersedes #66555)
…ge-remote-attach fix(desktop): allow large remote attachments (supersedes NousResearch#66555)
…ge-remote-attach fix(desktop): allow large remote attachments (supersedes NousResearch#66555)
…ge-remote-attach fix(desktop): allow large remote attachments (supersedes NousResearch#66555)
Summary
Test plan
npm exec --workspace apps/desktop vitest run electron/hardening.test.ts src/app/session/hooks/use-prompt-actions/utils.test.ts(32 passed)python3 -m pytest tests/test_web_server.py -q(5 passed)hardening.test.tshelper reads and byte-compares a file above 16 MiB; no GUI/Electron renderer was available in this environmenthermes servesmoke with a temporaryHERMES_HOMEand ephemeral query-token auth: sent exactly one successful realfile.attachJSON-RPC carrying a 16,789,561-byte on-disk payload, then read back the backend-stored file and verified exact byte count plus SHA-256 (94b9e8d79a08535eb5f58c74c19539f96e374d20cca9a82c5455902b12539398)HERMES_HOME, harness, and server logs; verified no listener/process remained on port 44041git diff --checkand clean worktree