Repository navigation
Conversation
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed github/refactor/ui_web_architecture...HEAD at ff6e74ee9242 after refreshing the target ref. I found no issue worth raising.
Coverage included the repository rules in AGENTS.md / CLAUDE.md, CONTEXT-MAP.md, and ui-web/CONTEXT.md; the complete diff; both web composer upload callers; server-side fs.upload enforcement; WebSocket frame sizing; turn attachment resolution; viewer behavior; the corresponding merged change and relevant history; backward compatibility of retaining the 25 MB viewer ceiling; the UI-to-runtime RPC boundary; and whether tests were weakened. The test edit only removes a stale literal from its explanation, while the arithmetic contract still derives from MAX_UPLOAD_BYTES. The refactored UI mirror is aligned at 100 MB and continues to call the runtime only through the gateway.
Independent verification:
uv run pytest tests/test_rpc_transport.py tests/test_rpc_files.py tests/test_rpc_console.py -q: 178 passednpm test --prefix ui-web: 186 files, 2544 tests passednpm run type-check --prefix ui-web: passed- the
check-source-languagecommand from the Makefile, invoked directly becausemakeis unavailable here: passed git diff --check github/refactor/ui_web_architecture...HEAD: passed
At posting time every completed GitHub check passes; only coverage gates is still pending.
The same change as #549, carried onto this branch so the two trunks do not disagree about the ceiling while work continues here. 25 MB was the file viewer's number, adopted when fs.upload landed beside it, and the viewer picked it because no renderer in the page does anything useful past that size. An upload answers a different question: it only has to land a path in the workspace, since a non-image attachment is named to the model and never read into the message. MAX_VIEW_BYTES therefore stays where it is, and a file between the two ceilings can be attached and not previewed; the constant now says so. frame_ceiling_for_upload derives the WebSocket frame ceiling from this constant, so the base64 expansion that once capped attachments at roughly 3 MB cannot reappear from raising the number. Not a cherry-pick: two of the six files in #549 do not exist here. The page's mirrored copy moved from ui-web/src/live/020-rpc.js into ui-web/src/lib/upload.ts, and the two knowledge comments that change swept went with the files this branch removed. raven/rpc/files.py is byte-identical between the branches, so that hunk is the same one. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
ff6e74e to
7eb5129
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
This revision is a rebase onto target 9656dd91fb97; git range-diff and stable patch IDs show that the reviewed upload-limit patch is unchanged. I rechecked the repository rules and context vocabulary, the complete resulting PR diff, affected callers and history, backward compatibility, the UI-to-runtime boundary, and the test edit; the newer target commits do not change the upload callers, handler, frame sizing, turn attachment resolution, or viewer policy. No tests were weakened.
Verification on 7eb5129f4a80:
uv run pytest tests/test_rpc_transport.py tests/test_rpc_files.py tests/test_rpc_console.py -q: 178 passednpm test --prefix ui-web: 186 files, 2558 tests passednpm run type-check --prefix ui-web: passed- source-language gate and
git diff --check: passed
All completed GitHub checks pass; four unit shards are still pending.
|
The frontend rebuild landed on main in #612, as 83 individual commits rather than a squash, so This PR is not lost and was deliberately left open rather than closed -- deleting the branch would have closed it with no way to reopen it. To land the change:
Retargeting before the rebase will show the whole gap between the two branches rather than your change, so do them in that order. Shout if the rebase turns out to be more than it looks and we will sort it out. |
|
Blocking: the landing steps in the preceding maintainer note must be completed. The reviewed code stance is unchanged, but this PR cannot merge while it still targets the retired, read-only branch. No code changed since the existing verification. |
Summary
The same ceiling change as #549, applied to this branch. #549 raises
MAX_UPLOAD_BYTESfrom 25 MB to 100 MB againstmain; this carries it onto the refactor trunk so the two do not disagree while work continues here.Why 25 MB was the wrong number is unchanged: it was the file viewer's ceiling, adopted when
fs.uploadlanded beside it, and the viewer picked it because no renderer in the page does anything useful past that size. An upload answers a different question -- it only has to land a path in the workspace, since a non-image attachment is named to the model and never read into the message.MAX_VIEW_BYTEStherefore stays at 25 MB, and a file between the two ceilings can be attached and not previewed; the constant's comment now says so.frame_ceiling_for_uploadderives the WebSocket frame ceiling from the constant, so the base64 expansion that once capped attachments at roughly 3 MB cannot come back from raising the number.This is not a cherry-pick of #549: two of its six files do not exist here. The page's mirrored copy moved from
ui-web/src/live/020-rpc.jsintoui-web/src/lib/upload.ts, and the two knowledge comments it swept went with the files this branch removed. What remains is four files, andraven/rpc/files.pyis byte-identical between the two branches, so that hunk is the same one.Unrelated to this change, noticed while placing it:
refusalBySizeinui-web/src/lib/upload.tsis exported with no caller on this branch. Onmainthe knowledge upload used it to refuse onFile.sizebefore encoding, and it is the only caller that got that order right -- the composer measures after base64-encoding, so an oversized file is read whole and thrown away while the tab blocks. The knowledge upload path is not on this branch yet, so the export is waiting for it rather than dead by mistake; worth keeping in view when that path is ported.Type
Verification
The server side and the page were verified end to end on #549, at the size that was refused (26.1 MB), against a
raven servebuilt from that branch: the attachment lands in<workspace>/uploads/at exactly 27,400,000 bytes, and a 105 MB file is refused with the new ceiling named in the message. The server hunk here is byte-identical, and the page hunk is the same literal in its new home.Gates run locally on this branch:
No user-facing docs name this ceiling; the one doc mention of 25 MB is the viewer's render cap, which this change does not touch.
Risk
Behaviour change: an attachment up to 100 MB is now accepted where 25 MB was the wall. Per upload the gateway holds the frame, the parsed JSON string and the decoded bytes at once -- roughly 370 MB peak for a maximal one -- and the JSON parse blocks the event loop for a second or more. Single-user desktop use absorbs that; a shared gateway taking concurrent uploads would feel it.
Security: the route is unchanged.
fs.uploadis reachable only over an authorized socket, so the larger allocation is available to the signed-in local reader and to nobody new.Rollback: revert the two constants. Nothing persists and no schema or contract moved.
Related Issues
N/A