Skip to content

fix(mobile): gate chat attachment size before materializing the file - #4825

Merged
iscekic merged 10 commits into
mainfrom
attach-oom-11fd
Jul 29, 2026
Merged

fix(mobile): gate chat attachment size before materializing the file#4825
iscekic merged 10 commits into
mainfrom
attach-oom-11fd

Conversation

@iscekic

@iscekic iscekic commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

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 JS ArrayBuffer and a ByteArray on the Java
heap 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 OutOfMemoryError class, and a 10-file
multi-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 size
is unknown or zero (fail-closed: File.size signals an unreadable file with 0). Only then does the new
materializeAttachment read the file, one file at a time. The composer passes the same 10 MiB constant to
the queue's own post-hoc maxBytes check, so the two guards can never disagree.

The Sentry crash cluster (KILO-APP-2J / KILO-APP-2R) — mechanism finding

The reported allocation (NetworkEventUtilBase64.encodeToStringStringFactory.newStringFromChars
on 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 from NetworkingModule's
blob responseHandlers branch), with enableNetworkEventReporting() defaulting to true — in release
builds too. Under Expo SDK 55 that applied to every fetch(), because RN's whatwg-fetch sets
responseType: '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's UriHandler
serves non-http(s) URIs before OkHttp (NetworkingModule.kt:309-311), so a picker fetch cannot produce
the reported RealCall$AsyncCallCallback.onResponse frames.

Can main still hit the reported allocation? No. Expo SDK 57 installs expo/fetch as global.fetch
(EXPO_PUBLIC_USE_RN_FETCH appears nowhere in this repo), which runs on its own OkHttp client and never
enters NetworkingModule. The only remaining first-party XMLHttpRequest (the signed upload PUT) uses the
default text response type. The probable crashing surface — labelled unverified — was a very large
tRPC/HTTP response on agent-chat/[session-id], matching view_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 picked
attachments (above). This PR is not expected to close KILO-APP-2J or KILO-APP-2R — their allocation site
is framework code main no longer reaches, so they should go inert on the next SDK 57 release for an
unrelated 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 worst
case 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:

  • A maximum-resolution camera capture (quality: 1) can exceed 10 MiB and will now be rejected with the
    size toast
    instead of uploading. Typical phone JPEGs (2–6 MB) are unaffected. No quality change or
    downscaling is included — that is a separate product change.
  • A zero-byte or unmeasurable pick is now rejected (Couldn't read <name>.) instead of being uploaded
    as an empty attachment — matching the cloud-agent surface's decision to reject size <= 0.
  • A multi-select now keeps the files that succeeded instead of discarding the whole pick when one file
    fails; each failed file is named in its own toast.
  • Honest limit of any pre-read gate: an unreadable size stat combined with a picker that actively
    under-reports the size still falls through to addFile's post-materialize check. That is a strictly
    smaller 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)

State Required experience How it is delivered
Happy File within the limit is accepted, the chip shows upload progress, the message sends with the attachment. selectAllowedAttachments returns it in accepted; materializeAttachmentqueue.addFile → existing upload/chip/send path, unchanged.
Unhappy, retryable The rejected file is named, the reason is specific, nothing is added, and the user can immediately pick another file. Three distinct messages, never merged: 1. Too large<name> exceeds the 10 MB attachment limit. 2. Unmeasurable or emptyCouldn't read <name>. 3. Too many files → the existing You 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.
Unhappy, non-retryable Not applicable. Every selection-time rejection is a property of the file the user chose (its byte size, or that it could not be measured), so choosing a different file always recovers. There is no server verdict, no permission verdict, and no irreversible state at this stage. Terminal upload failures are a different, pre-existing chip state (retryFile) and are out of scope.
Empty Not applicable. The gate only runs on a non-empty picker result; a cancelled pick returns [] 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 under packages/ changes — the
shared ATTACHMENT_MAX_BYTES (100 MB) governs the server and the web client and is intentionally left
alone; maxBytes is 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:20 declares the media
bucket with "remote": true, so the happy-path upload below signs and PUTs against the real
kilo-chat-media R2 bucket, not a local one. That is pre-existing local kilo-chat behaviour, not something
this 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; capacity
truncation; size-rejection precedence over the capacity toast; unknown/null/zero size rejected as
unreadable; both picker paths return selections without calling fetch; materializeAttachment does read
bytes; and Math.max semantics both ways (measured over reported, reported over unreadable stat).

  • E2E (Android emulator, KiloClaw chat composer chat/[sandbox-id]/[conversation-id], real device
    flow: 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_MEDIA Secrets-Store pair did not exist anywhere on the machine yet — every size-gate
    assertion still passed) and round 2 ran the full set once the pair was provisioned:
    • A — happy path: small.bin (1 MiB) picked → chip small.bin, ready → sent → attachment renders in
      the transcript (Open small.bin). Backend: GET …/attachments/<id>/url 200, POST /v1/messages 201.
    • B — oversized rejected (the fix): big.bin (12 MiB) picked → the toast reads verbatim
      big.bin exceeds the 10 MB attachment limit. No chip added, composer unchanged, app responsive on
      the same screen (screenshot captured at +2.2 s, corroborated by two bracketing frames).
    • C — no crash, no silent drop: logcat window free of FATAL EXCEPTION/OutOfMemoryError, no
      red-box/fatal JS in Metro, app PID stable across the whole window.
    • D — recoverable: immediately after the rejection, small.bin re-picked → chip ready → sent
      (POST /v1/attachments/init 200 in 36 ms, POST /v1/messages 201) — the rejection leaves the composer
      fully usable.
    • The KiloClaw agent's reply to the random-bytes fixture errored (Something went wrong… use /new) — the
      agent 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 read
instead of after; E2E frames of it were captured and are described under Verification.

Reviewer Notes

  • The two eslint-disable typescript-eslint/promise-function-async comments in the picker were removed with
    the async signatures; the one on pickAttachmentsFromSource intentionally stays.
  • The single no-await-in-loop disable in the composer marks the deliberate sequential materialize loop
    (bounding peak memory to one file).
  • addFilesWithinAttachmentCapacity was deleted: the count limit now lives entirely inside
    selectAllowedAttachments, which already trims the selection to available capacity before any bytes are
    read.

@iscekic iscekic self-assigned this Jul 28, 2026
@kilo-code-bot

kilo-code-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

Reviewed 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)
  • .kilo_workflow/learnings/android-emulator-systemserver-restart-systemui-anr-under-load.md
  • .kilo_workflow/learnings/kiloclaw-chat-attachments-r2-secret-missing.md
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 Summary

Reviewed 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)
  • .kilo_workflow/learnings/android-picker-tap-races-and-toast-capture.md
  • .kilo_workflow/learnings/kiloclaw-chat-attachments-r2-secret-missing.md

Previous review (commit 8619b32)

Status: No Issues Found | Recommendation: Merge

Executive Summary

Reviewed 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)
  • apps/mobile/src/components/kilo-chat/message-attachment-picker.ts
  • apps/mobile/src/components/kilo-chat/message-attachment-picker.test.ts
  • apps/mobile/src/components/kilo-chat/message-attachment-state.ts
  • apps/mobile/src/components/kilo-chat/message-attachment-state.test.ts
  • apps/mobile/src/components/kilo-chat/message-input-attachment-queue.tsx
  • .kilo_workflow/learnings/kilo-interactive-orchestrator-wedges-relaunch.md
  • .kilo_workflow/learnings/kiloclaw-docker-tcp-absent-from-worktree-status.md
  • .kilo_workflow/learnings/worktree-port-offset-airplay-collision.md
  • .kilo_workflow/learnings/dev-start-docker-tcp-refusal-is-transient.md

Previous review (commit bc380d5)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (8 files)
  • apps/mobile/src/components/kilo-chat/message-attachment-picker.ts
  • apps/mobile/src/components/kilo-chat/message-attachment-picker.test.ts
  • apps/mobile/src/components/kilo-chat/message-attachment-state.ts
  • apps/mobile/src/components/kilo-chat/message-attachment-state.test.ts
  • apps/mobile/src/components/kilo-chat/message-input-attachment-queue.tsx
  • .kilo_workflow/learnings/kilo-interactive-orchestrator-wedges-relaunch.md
  • .kilo_workflow/learnings/kiloclaw-docker-tcp-absent-from-worktree-status.md
  • .kilo_workflow/learnings/worktree-port-offset-airplay-collision.md

Previous review (commit 4844288)

Status: No Issues Found | Recommendation: Merge

Executive Summary

Reviewed 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)
  • apps/mobile/src/components/kilo-chat/message-attachment-picker.ts
  • apps/mobile/src/components/kilo-chat/message-attachment-picker.test.ts
  • apps/mobile/src/components/kilo-chat/message-attachment-state.ts
  • apps/mobile/src/components/kilo-chat/message-attachment-state.test.ts
  • apps/mobile/src/components/kilo-chat/message-input-attachment-queue.tsx

Reviewed by claude-sonnet-5 · Input: 32 · Output: 8.4K · Cached: 949.9K

Review guidance: REVIEW.md from base branch main

@iscekic

iscekic commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

(bot) @kilocode-bot please review

@kilo-code-bot

kilo-code-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Review: gate attachment size before materializing — approve, with 3 non-blocking nits

Checked out attach-oom-11fd and read the diff in full. The core mechanic is right and the memory argument holds. Local run: apps/mobile 277 test files / 2344 tests pass; no typecheck or lint findings in the three changed source files (repo-wide pnpm typecheck/pnpm lint noise in my sandbox is pre-existing — CI is green).

Logic verified

  • No materialization in the selection helpers. pickCameraImage/pickLibraryImages/pickFiles now return MessageAttachment[] via the pure assetToSelection (message-attachment-picker.ts:88,101,115); no fetch, no Response.blob(). Only materializeAttachment (:54-63) reads bytes.
  • Gate precedes materialization. selectAllowedAttachments runs at message-input-attachment-queue.tsx:63-66, strictly before the loop at :73-80. Capacity trimming happens inside the gate, so no rejected/truncated file is ever read.
  • Fail-closed on unknown/zero size (message-attachment-state.ts:107-117). More robust than the comment claims: Math.max(file.size, …) also survives File.size being null (Math.max(null,0) === 0) and undefined (NaNsizeFromSelection's Number.isFinite guard → 0). Small correction: the comment at message-attachment-picker.ts:32-33 says File.size "is 0" for a missing file — expo itself treats it as nullable (expo-file-system/src/NetworkTasks.ts:151: file.size ?? 0). Behaviour is fine; the comment is slightly off.
  • One file at a time. Sequential for … await with a scoped no-await-in-loop disable and a per-file try/catch, so a mid-loop materializeAttachment failure names that file and keeps the successful ones (:73-88).
  • maxBytes matches. useAttachmentQueue({ maxBytes: MOBILE_ATTACHMENT_MAX_BYTES }) (:47) is the same constant the gate uses, so the pre-read gate and the queue's post-read blob.size > maxBytes check (packages/kilo-chat-hooks/src/use-attachment-queue.ts:318) cannot disagree; the shared 100 MB ATTACHMENT_MAX_BYTES is untouched. formatFileSize(10 MiB)10 MB, matching the existing chip/preview copy.

Findings

  1. Empty vs unreadable copy diverges from the in-repo precedentmessage-attachment-state.ts:107-117. The cloud-agent surface this PR cites has a separate empty reason with copy File is empty (src/lib/agent-attachments/validate.ts:73-75, :105-110), while here a genuinely 0-byte file gets Couldn't read <name>. — misleading, and the user cannot distinguish an empty file from a stat failure. A third reason: 'empty' with <name> is empty. would match precedent at near-zero cost.
  2. Only the first rejection is surfaced, and the capacity toast is swallowedmessage-attachment-state.ts:139-155 plus message-input-attachment-queue.tsx:63-69. rejected and truncatedCount are computed and then discarded at the call site. A 10-file multi-select with 5 oversized files shows a single toast naming one file, and the truncated remainder is dropped with no signal at all, because size rejections take precedence over the limit toast. The description's "each failed file is named in its own toast" is only true for materialize-time failures. Consider aggregating (3 files were too large.) or at least emitting the limit toast alongside.
  3. materializeAttachment ignores response.okmessage-attachment-picker.ts:57-58. If a file:// fetch resolves non-2xx instead of rejecting, response.blob() yields an error/empty body that is then uploaded as the attachment. if (!response.ok) throw new Error(…) routes it onto the per-file "Couldn't read" path that already exists.

Edge cases / races

  • Stale existingCount (:64): queue.rows.length comes from the render closure captured before the picker await. The 10-file cap is now the only thing bounding retained bytes to ~100 MiB, so it is load-bearing for this fix — two overlapping picks (see your own .kilo_workflow/learnings/android-picker-tap-races-and-toast-capture.md) each read the stale count and can each accept up to 10, i.e. ~200 MiB. A rowsCountRef kept in sync on add/remove, or re-deriving capacity immediately before each addFile, would make the bound actually hold. Pre-existing staleness, so not a merge blocker.
  • Overlapping picks also surface expo's internal rejection text verbatim through the generic toast.error(error.message) at :89-91 ("Different document picking in progress…"). Also pre-existing.
  • Zero-byte legitimate files and max-resolution (quality: 1) camera captures over 10 MiB are now rejected — documented trade-offs, fine subject to nit 1.
  • A throw inside assetToSelection (e.g. new File(uri) on an unexpected scheme) still fails the entire pick via the outer catch — unchanged from before.

Test coverage

Gate 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/null/zero size, Math.max in both directions, expect(fetch).not.toHaveBeenCalled() on the library and document paths, and a positive materializeAttachment test.

Gaps, all in the composer, which has no test file:

  • Sequentiality is untested — "one file at a time" is the memory claim of this PR, and today only a comment enforces it; a future Promise.all refactor would go green.
  • Untested: per-file materialize failure toasting while continuing with the remaining files; tempId → localUri mapping; and that MOBILE_ATTACHMENT_MAX_BYTES (not the shared 100 MB constant) is what reaches useAttachmentQueue.
  • The camera path is not asserted to skip fetch (library and files are).

Extracting the loop into a small helper (roughly the shape of the deleted addFilesWithinAttachmentCapacity: { accepted, materialize, addFile, onUnreadable }) would make all of the above unit-testable without a renderer.

Recommendation

Merge 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-existingCount race is the one I would file as a follow-up, since it partially undermines the ~100 MiB worst case the 10 MiB number is derived from.

Built for iscekic by Kilo

@kilo-code-bot

kilo-code-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

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.

@iscekic

iscekic commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

(bot) Triage of the three non-blocking nits from the 18:00Z review:

  1. Empty vs unreadable copy — valid observation of the divergence from the cloud-agent precedent. The fail-closed rejection of zero-byte/unmeasurable picks is a deliberate, documented behaviour change of this PR; splitting a third empty reason changes user-facing copy and is deferred to a follow-up rather than expanding this fix's scope.
  2. Aggregated rejection toasts — valid UX observation. The single-toast-first-failure shape is deliberate (multi-select keeps the files that succeeded; each materialize-time failure names its file). Aggregating pick-time rejections is a behaviour change beyond this fix, deferred to a follow-up.
  3. response.ok check in materializeAttachment — reasonable defensive hardening; conditional on file:// fetches resolving non-2xx rather than rejecting, which is not observed in E2E. Deferred to the same follow-up.
  4. Stale existingCount race — pre-existing, as noted, and explicitly follow-up material: the 10-file cap bounding retained bytes predates this change.

No code change on this head; the approving summary stands on the reviewed tree.

@iscekic

iscekic commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

(bot) @kilocode-bot please review

@kilo-code-bot

kilo-code-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Blocking Issues | Recommendation: Merge

Executive Summary

Fresh full review of the current head (47015c1) against main — not an incremental diff review. The core fix is correctly implemented: the three pick functions are now pure and allocation-free, the size/capacity gate runs before any bytes are read, and materialization is sequential and per-file recoverable. No bugs, security issues, or logic errors that block merge. Two new low-severity observations below; the four items triaged as follow-ups in the 18:02Z comment are still open and still non-blocking.

Core fix verified

  • No materialization in the selection path. pickCameraImage / pickLibraryImages / pickFiles return MessageAttachment[] built by the synchronous assetToSelection (message-attachment-picker.ts:25-41); the only fetch + Response.blob() left in the module is materializeAttachment (:54-62). Tests assert expect(fetch).not.toHaveBeenCalled() on the library and document paths.
  • Gate strictly precedes the read. selectAllowedAttachments runs at message-input-attachment-queue.tsx:63-66, before the loop at :73-88. Capacity trimming happens inside the gate, so truncated and rejected files are never read.
  • Sequential materialization. for … await with a scoped no-await-in-loop disable bounds peak memory to one file; a per-file try/catch keeps the successful files in a multi-select and names the failed one.
  • Fail-closed on unmeasurable size (message-attachment-state.ts:107-117). sizeFromSelection collapses missing / null / non-finite / negative to 0, and size <= 0 is rejected as unreadable before any read. I checked the picker comment against the installed expo-file-system (SDK 57): NativeFileSystemFile.size is typed number and documented "0 if the file does not exist, or it cannot be read", so the comment at :31-38 is accurate for this version — this supersedes the "slightly off" note in the 18:00Z review (expo own file.size ?? 0 is defensive, and Math.max(null, …) coerces to 0 anyway).
  • The two guards cannot disagree. useAttachmentQueue({ maxBytes: MOBILE_ATTACHMENT_MAX_BYTES }) (:47) feeds the hook post-read input.blob.size > maxBytes check (packages/kilo-chat-hooks/src/use-attachment-queue.ts:318) the same constant the gate uses, and formatFileSize(10 * 1024 * 1024) renders 10 MB, matching the toast copy.
  • Scope claim holds. The shared ATTACHMENT_MAX_BYTES is untouched; the only other useAttachmentQueue caller (apps/web/.../MessageInput.tsx:103-108) still passes 100 MB, and the mobile share-sheet path is separately gated at AGENT_ATTACHMENT_MAX_BYTES (5 MiB) with a pre-read measureSize. No server behaviour changes.
  • No security concerns. No new network surface, no credential or header handling, no logging added. The one Secrets-Store id recorded in the learnings doc already appears in several services/*/wrangler.jsonc files, so it is not new exposure.

Findings (non-blocking)

SUGGESTION: Math.max can propagate NaN into a false "unreadable" rejection — message-attachment-picker.ts:39

Math.max(file.size, asset.size ?? 0, asset.fileSize ?? 0) returns NaN if file.size is ever undefined at runtime (the types say number, so this is unlikely — but expo own code defends with file.size ?? 0). NaN then normalizes to 0 and the file is rejected with Couldn't read <name>. even when the picker reported a perfectly valid size. Filtering the candidates before the max preserves the "reported size over unreadable stat" property in every case:

    size: Math.max(...[file.size, asset.size, asset.fileSize].filter(value => Number.isFinite(value)), 0),

The existing test for that path uses 0, not undefined, so it would not catch it.

SUGGESTION: dead fileSize pass-through — message-attachment-picker.ts:40

size is already a resolved number, so sizeFromSelection selection.size ?? selection.fileSize never reaches the fileSize branch. Harmless, but it reads as if the field still participates.

Previously triaged follow-ups — still open, still non-blocking

  1. Only the first rejection is surfaced, and the capacity toast is swallowed (message-attachment-state.ts:141-157): a multi-select with several oversized files shows one toast, and when a size rejection coexists with truncation the You can attach up to 10 files. signal is dropped entirely (truncatedCount is computed and then discarded at the call site).
  2. unreadable copy also covers genuinely empty files, diverging from the cloud-agent precedent separate empty reason.
  3. materializeAttachment ignores response.ok (:57-58): a non-2xx file:// resolution would upload an error or empty body instead of routing onto the existing per-file "Couldn't read" path.
  4. Stale existingCount (message-input-attachment-queue.tsx:64): queue.rows.length is read from the closure captured before the picker await, so two overlapping picks can each accept up to 10 files. Pre-existing, but the 10-file cap is what bounds the ~100 MiB worst case the 10 MiB number is derived from, so this is the follow-up worth filing.

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 Promise.all refactor would stay green.

Verification run locally (PR head 47015c1)

  • apps/mobile full vitest suite: 277 files / 2344 tests passed; the two changed suites alone: 15 passed.
  • oxlint on the five changed source files: 0 warnings, 0 errors. oxfmt --list-different src/components/kilo-chat: clean. knip (check:unused): clean.
  • tsgo --noEmit: errors present in my sandbox, all traceable to an unbuilt @kilocode/trpc (interrupted install); zero errors in any changed file. CI typecheck, lint, test (kilo-app), check-unused, format-check, CodeQL and trufflehog are all green on this head.
Files Reviewed (12 files)
  • apps/mobile/src/components/kilo-chat/message-attachment-picker.ts
  • apps/mobile/src/components/kilo-chat/message-attachment-picker.test.ts
  • apps/mobile/src/components/kilo-chat/message-attachment-state.ts
  • apps/mobile/src/components/kilo-chat/message-attachment-state.test.ts
  • apps/mobile/src/components/kilo-chat/message-input-attachment-queue.tsx
  • .kilo_workflow/learnings/android-emulator-systemserver-restart-systemui-anr-under-load.md
  • .kilo_workflow/learnings/android-picker-tap-races-and-toast-capture.md
  • .kilo_workflow/learnings/dev-start-docker-tcp-refusal-is-transient.md
  • .kilo_workflow/learnings/kilo-interactive-orchestrator-wedges-relaunch.md
  • .kilo_workflow/learnings/kiloclaw-chat-attachments-r2-secret-missing.md
  • .kilo_workflow/learnings/kiloclaw-docker-tcp-absent-from-worktree-status.md
  • .kilo_workflow/learnings/worktree-port-offset-airplay-collision.md

Recommendation

Merge. The gate is correct, fail-closed, and strictly reduces peak memory versus main; the 10 MiB cap is mobile-local and leaves the shared and web limits alone. Both new suggestions and the four carried-over items are follow-up material — the stale existingCount race is the one worth an issue, since it partially undermines the worst-case retention bound the 10 MiB number was derived from.

Built for iscekic by Kilo

@kilo-code-bot

kilo-code-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

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 NaN propagation in Math.max size calc, and now-dead fileSize pass-through) — non-blocking. Previously deferred follow-ups restated as still open. Full test suite/lint/format green on head 47015c1.

No further action needed from me.

@iscekic

iscekic commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

(bot) Triage of the two new non-blocking suggestions from the 20:23Z review of head 47015c1:

  1. Math.max NaN propagation (message-attachment-picker.ts:39) — valid defensive point; the runtime types declare file.size: number and expo-file-system SDK 57 documents 0 if the file does not exist, or it cannot be read, so the false-rejection path requires an out-of-contract undefined. Filtering candidates before the max is the more robust shape and is added to the same follow-up as the carried-over items; no code change on this approved head.
  2. Dead fileSize pass-through (:40) — correct, harmless readability residue from the gate refactor; folded into the same follow-up.

The four previously triaged items (aggregated rejection toasts, empty-vs-unreadable copy split, response.ok guard, stale existingCount race — the one worth an issue) remain follow-up dispositions as recorded in the 18:05Z triage. No actionable comment is outstanding; the approving summary stands on the current head.

@iscekic iscekic added the human-ready The PR is ready for human review. label Jul 28, 2026
@iscekic

iscekic commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

(bot) E2E evidence frames (round r1, Android emulator, KiloClaw chat composer) preserved at /Users/igor/Projects/e2e-evidence/attach-oom-11fd/: big-pick-t2.png — the rejection toast reading verbatim big.bin exceeds the 10 MB attachment limit.; big-reject-1.png — same rejection, earlier capture; d-chip-failed.png — the round-1 environment-blocked upload state (chip failed on the missing R2 media secret, since provisioned; round r2 then passed the full set, see Verification above).

@iscekic
iscekic requested a review from pandemicsyn July 29, 2026 00:05
@iscekic
iscekic merged commit 02d6836 into main Jul 29, 2026
22 checks passed
@iscekic
iscekic deleted the attach-oom-11fd branch July 29, 2026 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

human-ready The PR is ready for human review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants