fix(mobile): gate chat attachment size before materializing the file - #4825
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryReviewed the incremental diff (a new emulator/SystemUI ANR learnings doc and an update to the R2-secret learnings doc marking it resolved); no bugs, security issues, or logic errors found — no code files changed in this increment. Files Reviewed (2 files)
Previous Review Summaries (4 snapshots, latest commit e35546e)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit e35546e)Status: No Issues Found | Recommendation: Merge Executive SummaryReviewed the incremental diff (two new learnings documentation files); no bugs, security issues, or logic errors found — no code files changed in this increment. Files Reviewed (2 files)
Previous review (commit 8619b32)Status: No Issues Found | Recommendation: Merge Executive SummaryReviewed the incremental diff (a new learnings documentation file) plus the previously reviewed mobile chat attachment size-gating rework; no bugs, security issues, or logic errors found in changed code. Files Reviewed (9 files)
Previous review (commit bc380d5)Status: No Issues Found | Recommendation: Merge Files Reviewed (8 files)
Previous review (commit 4844288)Status: No Issues Found | Recommendation: Merge Executive SummaryReviewed the mobile chat attachment size-gating rework (picker, state, and queue wiring) with high confidence; no bugs, security issues, or logic errors found in changed code. Files Reviewed (5 files)
Reviewed by claude-sonnet-5 · Input: 32 · Output: 8.4K · Cached: 949.9K Review guidance: REVIEW.md from base branch |
…tor relaunch learning
|
(bot) @kilocode-bot please review |
Review: gate attachment size before materializing — approve, with 3 non-blocking nitsChecked out Logic verified
Findings
Edge cases / races
Test coverageGate and picker helpers are covered well: exact boundary and boundary+1 with the literal toast string, mixed selection, capacity truncation, size-over-capacity precedence, unknown/ Gaps, all in the composer, which has no test file:
Extracting the loop into a small helper (roughly the shape of the deleted RecommendationMerge once the E2E checkbox in the description is filled in — the gate is correct, fail-closed, and strictly reduces peak memory. Nits 1-3 are cheap and worth folding in; the stale- Built for iscekic by Kilo |
|
The review is already posted on PR #4825 (comment link above): approve/merge recommendation, with 4 non-blocking findings and a note that the E2E evidence checkbox should be filled before merging. No further action needed from me — nothing left to trigger. |
|
(bot) Triage of the three non-blocking nits from the 18:00Z review:
No code change on this head; the approving summary stands on the reviewed tree. |
…sioned R2 media secrets
|
(bot) @kilocode-bot please review |
Code Review SummaryStatus: No Blocking Issues | Recommendation: Merge Executive SummaryFresh full review of the current head ( Core fix verified
Findings (non-blocking)SUGGESTION:
The existing test for that path uses SUGGESTION: dead
Previously triaged follow-ups — still open, still non-blocking
Coverage note (unchanged from the prior round): the composer has no test file, so the sequential-materialize claim — the actual memory guarantee — is enforced only by a comment; a future Verification run locally (PR head
|
|
Review already posted and complete — recommendation is Merge, no blocking issues (PR #4825, comment #4825 (comment)). Summary: core fix verified correct (pure pick functions, gate-before-materialize, sequential per-file read, fail-closed size check, shared byte constant). Two new low-severity nits noted (potential No further action needed from me. |
|
(bot) Triage of the two new non-blocking suggestions from the 20:23Z review of head 47015c1:
The four previously triaged items (aggregated rejection toasts, empty-vs-unreadable copy split, |
|
(bot) E2E evidence frames (round r1, Android emulator, KiloClaw chat composer) preserved at |
Summary
What: the mobile kilo-chat attachment composer now checks a picked file's size before reading the
file into memory, at a new mobile-only cap of 10 MiB, reusing the existing selection/toast helpers. Picking
an oversized or unmeasurable file is rejected with the existing rejection toast; only accepted files are
materialized, and multi-select files are materialized one at a time instead of all at once.
Why: picking a chat attachment used to materialize an arbitrarily large local file
(
fetch(file://)+Response.blob()— two full copies: a JSArrayBufferand aByteArrayon the Javaheap in React Native's blob store) and only checked the size afterwards, against the shared 100 MB limit.
On a phone with a ~268 MB heap growth limit that is a real
OutOfMemoryErrorclass, and a 10-filemulti-select multiplied peak memory by 10.
How: the three pick functions now return normalized selections only (no network, no blob). The size gate
lives in
selectAllowedAttachments, which was previously dead code, and now also rejects files whose sizeis unknown or zero (fail-closed:
File.sizesignals an unreadable file with0). Only then does the newmaterializeAttachmentread the file, one file at a time. The composer passes the same 10 MiB constant tothe queue's own post-hoc
maxBytescheck, so the two guards can never disagree.The Sentry crash cluster (KILO-APP-2J / KILO-APP-2R) — mechanism finding
The reported allocation (
NetworkEventUtil→Base64.encodeToString→StringFactory.newStringFromCharson an OkHttp dispatcher thread) was React Native's network-inspector reporting base64-encoding a large
remote HTTP response into a Java
String(NetworkEventUtil.kt:148, reached fromNetworkingModule'sblob
responseHandlersbranch), withenableNetworkEventReporting()defaulting totrue— in releasebuilds too. Under Expo SDK 55 that applied to every
fetch(), because RN'swhatwg-fetchsetsresponseType: 'blob'on every request. The arithmetic matches: 54,298,696 bytes ≈ a 20.4 MB response,220,681,432 bytes ≈ an 82.7 MB response (UTF-16
String, 2 bytes/char × 4/3 base64).The chat attachment picker's
file://fetch was not the crashing call:BlobModule'sUriHandlerserves non-
http(s)URIs before OkHttp (NetworkingModule.kt:309-311), so a picker fetch cannot producethe reported
RealCall$AsyncCall→Callback.onResponseframes.Can
mainstill hit the reported allocation? No. Expo SDK 57 installsexpo/fetchasglobal.fetch(
EXPO_PUBLIC_USE_RN_FETCHappears nowhere in this repo), which runs on its own OkHttp client and neverenters
NetworkingModule. The only remaining first-partyXMLHttpRequest(the signed upload PUT) uses thedefault
textresponse type. The probable crashing surface — labelled unverified — was a very largetRPC/HTTP response on
agent-chat/[session-id], matchingview_names. That allocation was framework code,is already unreachable on
main, and is not fixable here.What this PR fixes is the related, still-live defect on
main: unbounded materialization of pickedattachments (above). This PR is not expected to close KILO-APP-2J or KILO-APP-2R — their allocation site
is framework code
mainno longer reaches, so they should go inert on the next SDK 57 release for anunrelated reason. Recommendation: a human resolves them once an SDK 57 build has field coverage. (No
Fixes …reference in any commit, deliberately.)The number, the trade-offs, and the limit
10 MiB is a product judgement: the binding constraint is retained bytes — up to 10 accepted blobs
(
MESSAGE_ATTACHMENT_MAX_COUNT) stay resident in the blob store until the message is sent, so the worstcase is ~100 MiB against a ~268 MB heap growth limit. At 25 MiB the worst case would be 250 MiB — fatal on
the same device. It is more generous than the existing in-repo mobile precedent (cloud-agent attachments cap
at 5 MiB), and well inside the shared server limit (100 MB), so no server or web behaviour changes.
Accepted trade-offs, deliberately:
quality: 1) can exceed 10 MiB and will now be rejected with thesize toast instead of uploading. Typical phone JPEGs (2–6 MB) are unaffected. No
qualitychange ordownscaling is included — that is a separate product change.
Couldn't read <name>.) instead of being uploadedas an empty attachment — matching the cloud-agent surface's decision to reject
size <= 0.fails; each failed file is named in its own toast.
under-reports the size still falls through to
addFile's post-materialize check. That is a strictlysmaller hole than before (no gate at all), and closing it fully would require reading the file — the thing
the gate exists to avoid.
Feature-state matrix — attachment size behaviour (kilo-chat composer)
selectAllowedAttachmentsreturns it inaccepted;materializeAttachment→queue.addFile→ existing upload/chip/send path, unchanged.<name> exceeds the 10 MB attachment limit.2. Unmeasurable or empty →Couldn't read <name>.3. Too many files → the existingYou can attach up to 10 files.In all three the composer state is untouched and the picker stays available, so retry is immediate. On a multi-select each file gets its own outcome: one bad file is named and skipped, the rest still attach.retryFile) and are out of scope.[]and the composer is unchanged, so there is no empty surface to explain and no next-step CTA beyond the picker the user just dismissed. (A zero-byte file is the "unmeasurable or empty" retryable rejection above, not this state.)Scope
Everything is inside
apps/mobile/src/components/kilo-chat/. No file underpackages/changes — theshared
ATTACHMENT_MAX_BYTES(100 MB) governs the server and the web client and is intentionally leftalone;
maxBytesis a per-caller prop, so mobile can pass a smaller number without affecting web.One environment fact for reading the E2E evidence:
services/kilo-chat/wrangler.jsonc:20declares the mediabucket with
"remote": true, so the happy-path upload below signs and PUTs against the realkilo-chat-mediaR2 bucket, not a local one. That is pre-existing local kilo-chat behaviour, not somethingthis PR changes. Assertions B–D (the ones that verify this fix) are pure client behaviour and touch no
network.
Verification
Unit coverage (vitest, colocated): accepted exactly at the 10 MiB boundary; rejected one byte above it with
the exact literal message
huge.bin exceeds the 10 MB attachment limit.; mixed selection; capacitytruncation; size-rejection precedence over the capacity toast; unknown/
null/zero size rejected asunreadable; both picker paths return selections without calling
fetch;materializeAttachmentdoes readbytes; and
Math.maxsemantics both ways (measured over reported, reported over unreadable stat).chat/[sandbox-id]/[conversation-id], real deviceflow: login → KiloClaw tab → instance → new conversation → paperclip confirmed in the view hierarchy):
VERIFICATION PASSED on the merged head. Two rounds were run; round 1 was environment-blocked (the
R2_*_KILOCHAT_MEDIASecrets-Store pair did not exist anywhere on the machine yet — every size-gateassertion still passed) and round 2 ran the full set once the pair was provisioned:
small.bin(1 MiB) picked → chipsmall.bin, ready→ sent → attachment renders inthe transcript (
Open small.bin). Backend:GET …/attachments/<id>/url 200,POST /v1/messages 201.big.bin(12 MiB) picked → the toast reads verbatimbig.bin exceeds the 10 MB attachment limit.No chip added, composer unchanged, app responsive onthe same screen (screenshot captured at +2.2 s, corroborated by two bracketing frames).
FATAL EXCEPTION/OutOfMemoryError, nored-box/fatal JS in Metro, app PID stable across the whole window.
small.binre-picked → chipready→ sent(
POST /v1/attachments/init 200in 36 ms,POST /v1/messages 201) — the rejection leaves the composerfully usable.
Something went wrong… use /new) — theagent turn on meaningless content, outside every acceptance criterion; the attachment pipeline itself
returned 200/201 throughout.
Visual Changes
None — this change gates file reads; it adds no UI surface. The only visible behaviour is the pre-existing
rejection toast (
<name> exceeds the 10 MB attachment limit.), which now appears before any bytes are readinstead of after; E2E frames of it were captured and are described under Verification.
Reviewer Notes
eslint-disable typescript-eslint/promise-function-asynccomments in the picker were removed withthe async signatures; the one on
pickAttachmentsFromSourceintentionally stays.no-await-in-loopdisable in the composer marks the deliberate sequential materialize loop(bounding peak memory to one file).
addFilesWithinAttachmentCapacitywas deleted: the count limit now lives entirely insideselectAllowedAttachments, which already trims the selection to available capacity before any bytes areread.