fix(session-ingest): preserve attachment capability in live heartbeats - #4671
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe incremental commits switch Files Reviewed (2 files)
Previous Review Summary (commit 59c9562)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 59c9562)Status: No Issues Found | Recommendation: Merge Executive SummaryReviewed the heartbeat capability-projection change in Files Reviewed (2 files)
Reviewed by claude-sonnet-5 · Input: 62 · Output: 8.2K · Cached: 2M Review guidance: REVIEW.md from base branch |
|
(bot) @kilocode-bot please re-review latest head 4ed258a. Local E2E found and fixed a distinct upload-path/message-UUID R2 key mismatch, with a regression using intentionally different IDs. |
|
(bot) Final local iOS E2E passed on exact head |
There was a problem hiding this comment.
Re-review @ 4ed258a
Recommendation: ✅ Merge
Re-reviewed the full diff at the latest head. The new second commit adds a genuine bug fix + regression test on top of the already-clean heartbeat change. Both areas look correct.
1. R2 key mismatch fix — correct ✅
apps/mobile/src/components/agents/mobile-session-manager-helpers.ts now presigns downloads with submission.wire.path instead of submission.messageUuid.
Tracing the key construction confirms this is the right fix:
- Upload (
upload-task.ts→getAttachmentUploadUrl→generateCloudAgentAttachmentUploadUrl) builds the key as${userId}/cloud-agent/${messageUuid}/${attachmentId}.${suffix}, where themessageUuidsegment is passed aspath(i.e.pathRef.current). - Download (
getAttachmentDownloadUrl→generateCloudAgentAttachmentDownloadUrl) builds${userId}/cloud-agent/${messageUuid}/${filename}.
In use-agent-attachment-upload.ts, pathRef and messageUuidRef are two independent Crypto.randomUUID() values. The upload always used path, but the download helper was passing the unrelated messageUuid, so the middle key segment never matched and the presigned GET pointed at a nonexistent object. Switching the download to submission.wire.path makes the download key segment identical to the upload key segment. The filename (file.remoteName = remoteFilename = <attachmentId>.<ext>, the basename of the upload key) already matched, so the full key now reconciles. Fix is correct and minimal.
2. Regression test — exercises the bug and would have caught it ✅
mobile-session-manager.test.ts now uses intentionally distinct IDs: messageUuid: 'msg-uuid' vs wire.path: 'upload-path', and asserts every mutate call carries messageUuid: 'upload-path'. Against the pre-fix code (which passed submission.messageUuid), the assertion would have received 'msg-uuid' and failed — so it genuinely reproduces the original mismatch. The previous test used path: 'msg-uuid' (equal to messageUuid), which is exactly why the bug slipped through. Good regression coverage.
3. Heartbeat capability-projection change — still correct & untouched ✅
git diff between the two commits shows the second commit touches only the two mobile files. The UserConnectionDO.ts per-row projection and its test are unchanged from the previously-approved commit. The projection still preserves true / false (truthy object → spread) and absent (undefined → omitted) semantics, consistent with the rewritten DO test.
4. Verification
UserConnectionDO.test.ts— 143/143 passmobile-session-manager.test.ts— passes; full mobile suite 1591/1591 pass- The 2 failing
services/session-ingest/src/routes/api.test.tscases are unrelated 5s-timeout flakes on the R2-offload queue path (file untouched by this PR, times out in the constrained sandbox) - Remaining oxlint findings on the changed files are type-info-dependent
no-unsafe-*noise on the standardvi.mocked(...)mock chain (present pre-PR; resolved with typed linting in CI)
Both the R2 key mismatch fix and its regression test are correct and sufficient.
|
Re-review posted: APPROVED on head Summary: the R2 key mismatch fix is correct (download now presigns with |
Summary
sessions.heartbeatsession rowtrue,false, and absent capability semanticsactiveSessions.listWhy
The merged remote-attachment flow initially enables the mobile paperclip from the active-session snapshot, then the next live heartbeat disables it because the SDK reads capability from each session row while session-ingest only sent it at the envelope level.
Companion CLI PR: Kilo-Org/kilocode#12394
Verification
git diff --check