Attach files to a message - #489
Merged
Merged
Conversation
One file both sides read. The composer refuses a file before uploading it and the server refuses it again on arrival, and those two refusals have to agree: a limit written twice is a limit that drifts, and the drift shows up as a file the composer accepted and the server threw away with no explanation. SVG is deliberately absent from the accepted images. An SVG can carry script, and one served inline from this origin is stored XSS against everybody in the channel.
namesNoFormat and mediaTypeOf decide whether a claim the browser could not make is worth sending to the server to sniff. Both were untested repo-wide while three comments described them as load-bearing.
attachedAt is null until the file reaches a message somebody actually sent, and three readers depend on that meaning: the sweeper deletes a null, the upload route counts a null against the cap, and DELETE refuses to withdraw a non-null.
The backstop counts every unsent row one person holds across all channels and groups, which is the one scope a client cannot rename.
A claim that names no format is an absence rather than a refusal: it is thrown away and the bytes are read. A claim that does name one is handed back for the caller to refuse by name. The answer is never a claim that names nothing. Returning one put an empty parenthetical in the refusal a person reads.
The per-message cap counts the rows one composer session staged, so two tabs get eight each rather than eight between them — the alternative counted a closed tab's leftovers and refused a pick the client had already accepted, naming files nobody could see. A second backstop counts every unsent row per person across all channels and groups, because the group is a client-named form field and bounds nothing on its own. The fetch route revalidates rather than caching, so a withdrawn file stops being served; a 304 answers without reading the blob it is not going to send.
The modality comes from the sniffed type rather than the part's declared one: a PNG whose browser claim was text/plain would otherwise be run through toString(utf8) and captioned as a file. A run inlines a bounded number of bytes. History degrades to a note when it runs out; the asked message refuses loudly, because silently dropping a file from the message somebody just sent is the failure the strict mode exists to prevent.
attachedAt is written here because this is the first place that both knows the message and knows it is the one being asked about. It is written after the whole history resolves, so a turn that failed does not mark its files as sent.
Needs only DATABASE_URL: a job that deletes rows should not demand an encryption key. Deletes in resumable batches, so a run killed by a deadline keeps its progress.
The SDK's accept filter is the wildcard on purpose. Its match is an exact string equality, so a narrow list there refused the very files screenPickedFiles deliberately passes — the ones whose type the browser could not name and the server is supposed to sniff. The narrow list moved to the file dialog, which greys files out rather than refusing them.
A claim that names no format gets the benefit of the doubt and a round trip; a claim that names an unsupported one is refused here, because there the two halves already agree.
Of the rows a run carries, the ones nothing else is holding are released when it fails — the composer's own chips are not, because it restores them.
Measures its own content rather than animating to auto: closing measures an empty box and animates zero to zero, which is a jump wearing an animation's clothes. Not AnimatePresence — an exiting element stays mounted until its animation reports finished, and under happy-dom that report never comes.
Its own markup rather than the SDK's strip, which starts inside the editor's column and hangs off nothing. The modality comes from the server's sniffed type, so a tile does not flip when the message is sent.
The previous instructions left the migrating role a non-owner, so a migration that drops the vector extension failed with must be owner.
bun reads bunfig.toml from the working directory only, so cd server && bun test silently ran without the preload the root config declares and reported a different result from CI.
The trailing slash matched directories only, so a symlink by that name was tracked — and three of them were committed once.
All 68 test files were checked by nothing. That hid a duplicate import and a call passing three arguments to a four-parameter function, whose config would have thrown the moment its onUpload ran. Last in this branch so every commit before it is green under the old configuration.
guidovizoso
requested review from
MikeRyanDev,
davidmckayv,
mxmzb and
tylerslaton
as code owners
September 11, 2026 16:23
CI reported two `# Unhandled error between tests` with zero failing tests, both landing after composer-upload-group.test.tsx: React still had work posted when that file's afterAll ran GlobalRegistrator.unregister(), and the callback fired against a document that no longer existed. cleanup unmounts, but unmounting does not retract a callback the scheduler has already posted. The 17 files this branch added now await settleReactWork() before unregistering. Why act and not a setTimeout, in two parts. The scheduler posts with setImmediate when one exists and only falls back to MessageChannel when it does not, and bun defines setImmediate — so the pending work is an immediate. Immediates run in registration order, so a yield that is itself an immediate is ordered strictly after one already posted, while a setTimeout(0) is a different queue with no such ordering: it usually lands after, which is the kind of usually that only fails on CI. act yields through enqueueTask, which resolves to timers.setImmediate — the same primitive. And one turn is not enough whatever the primitive, because work run during a turn can schedule more: an upload settles on a microtask after the yield was already queued, and the render it triggers is posted behind it. So the drain has to be a fixed point rather than a count, and that is what act computes — while its scope is open React routes newly scheduled callbacks into the act queue instead of the scheduler, and it alternates flushing that queue with yielding a macrotask until a yield finds it empty. It lives in one module because the reason is the whole value: an unexplained await in a teardown is the first thing a reader deletes. transcript-attachments.test.tsx had already hand-rolled a single setTimeout(0) here, against this same hazard; it calls the same function now. The failure does not reproduce on macOS, so the local run proves no regression rather than the fix: bun test app is 738 pass, 0 fail, 0 errors.
davidmckayv
requested changes
Sep 11, 2026
davidmckayv
left a comment
davidmckayv
left a comment
Contributor
There was a problem hiding this comment.
Please fix the four inline findings before merging: repeated-reference budget bypass, attachment loss after a queued-send failure, silent failure to persist sent state, and restoring a retry draft above the file cap. Add focused regression checks and exercise failure/retry through the real channel send path.
Validation: reproduced the budget bypass with the production resolver; ran the existing queued-send failure test, which currently asserts deletion. The persistence and over-cap restoration findings are source-traced; no GUI or database-race execution is claimed. CI is currently green.
Non-blocking scope note: 27,181 of the 35,767 additions are tests or generated migration metadata. Please trim review-history commentary (975 comment-only lines in attachments.ts alone) and consider separating the network-policy and test-tooling changes.
`resolveAttachmentParts` emits a base64 part for every occurrence of an attachment id, but the budget was charged through a `charged` set that billed each id once. Every copy after the first was free, and exempt from the cut, so a repeated id could inline without limit: forty parts naming one stored 1,024 byte image, against `newInlineBudget(1024)`, produced one read, forty inlined parts and 40,960 decoded bytes with `remaining` reporting zero. At the 8 MiB upload ceiling a hundred references come to roughly 1.04 GiB of base64 against a 32 MiB budget, which is the heap exhaustion the budget exists to prevent. The charge now runs once per part encoded and no id is exempt from the cut once the room is gone. `loadOnce` is untouched: deduplicating the read was never the bug, and one id still costs one trip to `bytea`. Deduplicating the output instead was the other way to make the two numbers agree, and is rejected in the comment: it would change what the model is handed and break the rule that whatever runs after this owns the parts it was given.
The regression test drives forty parts naming one stored 1,024 byte image through the real `resolveAttachmentParts` under a 1,024 byte budget and asserts on the decoded bytes behind every `data` source that left the function: 40,960 before the fix, 1,024 after. It asserts there rather than on `budget.remaining` because the counter is what lied — it read zero while forty copies went out. The two tests that pinned the old behaviour are inverted rather than deleted. "one id named twice is charged to the budget once" becomes "read once and charged twice", asserting the memo and the per-part charge as separate claims since collapsing them into one is what hid this. "an id already paid for survives the budget running out" becomes "a repeated id is cut like any other", with the rejected argument kept in the comment: the second mention is a second encoded copy, not a free one.
A queue transition reported the attachments a run was carrying that nothing else held, which is a list whose only possible use is deletion: there are no words around those files, no skills, and no message to put them back into. So the one caller deleted them. It reports the messages now, carrying the rows the cap actually let through, and adds the restore that puts them back at the front of the queue.
The drain's catch deleted the staged rows behind a drained turn whose send failed, on the reasoning that nothing pointed at them any more. Something did: channel-chat adds the user message to the transcript before the run and leaves it there when the run fails, so the deletion emptied the tiles under a message that stayed on screen, with nothing said and no way back to the files. The messages go back in the queue instead — words, skills and rows — visible as parked entries, carried by the next turn, and released only when somebody takes one back by hand. The drain is held until a turn starts so a server that is refusing every request cannot be re-sent in a loop.
CopilotKitCore.runAgent catches everything the agent throws, reports it through emitError, and returns an empty result — so a gateway 503, a dead stream and a refused request all reached say() as a resolved promise. Every caller reads that as "it went": the composer cleared the box and gave up the chips it was riding, the queue emptied into a draft nothing retried, and both failure paths written in conversation-view for exactly this were dead code. The reason a turn ended without an answer is already read off the subscription that draws the notice, so deliver reads the same fact and throws. Stop is unaffected: onStop clears awaitingReply before aborting, so a turn somebody ended is not a failure. The two voided say() call sites catch, since neither has a draft to restore.
Outgoing attachments are hidden from the staged count while a send is out, which is right for a send that lands and wrong for one that fails: a second full batch is accepted behind the first, and the failure hands the first back onto a strip that has filled up. canSendDraft asks about upload status and emptiness, not the cap, so that doubled draft stayed sendable and the retry shipped twice the server's per-message budget. Nothing is dropped to fix it — trimming the strip would delete files somebody picked, on a path they never asked for. Every chip stays, every one is removable by hand, and Send says no with a sentence naming the limit and the number to remove. The Enter key is gated on the same function as the button.
The existing coverage rendered messages={[]} with a rejecting onSubmit, which is
exactly the state in which deleting a failed send's rows looks free: there is no
transcript to contradict. It asserted the DELETE and so pinned the data loss.
The new file runs a real ChannelChat over a real agent with only fetch stubbed,
and covers the reviewer's two reproductions step for step: a screenshot parked
mid-turn whose drained send answers 503, and a batch of eight restored beside
eight more. It also pins the retry — the next turn carries the restored message
and its file, ahead of the newer words — and that nothing re-sends on its own.
The old test is inverted: no DELETE, the queue back, and the row released only
when the restored message is removed by hand.
davidmckayv
approved these changes
Sep 11, 2026
davidmckayv
left a comment
davidmckayv
left a comment
Contributor
There was a problem hiding this comment.
The four original change requests are addressed at 98aa4ec: emitted attachment copies consume the budget; failed queued sends retain retryable files; failed persistence refuses the turn; and oversized restored drafts cannot be sent.
Verification: 96 focused tests passed locally, including the real ChannelChat failure/retry path. Re-running the original repeated-ID reproduction now refuses excess copies in strict mode and emits only 1,024 bytes under a 1,024-byte history budget. The PostgreSQL write-failure, withdrawal-race, concurrent-stamp, and idempotent-retry tests passed in CI. Local PostgreSQL was unavailable; those database results were verified from CI logs.
Correction to my earlier queued-send comment: our rejecting-submit test did not establish that a real HTTP 503 reached the old DELETE catch. CopilotKit emits the failure while resolving runAgent. The new failure bridge and real-channel tests now exercise that behavior and verify recovery. The earlier 503-to-deletion claim was too strong.
Approving the fixes. One non-blocking UUID-normalization edge is noted inline; it does not affect the ordinary composer path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Attach a file to a message: paste it, drop it, or pick it; see it staged, refuse it with a reason if it cannot be taken, send it, and read it back in the transcript. The bytes go to Postgres, and the model is handed the file itself rather than a URL it could not fetch.
What is here
The contract.
shared/attachments.tsis read from both sides. The composer refuses a file before uploading it and the server refuses it again on arrival; a limit written twice is a limit that drifts, and the drift shows up as a file the composer accepted and the server threw away with no explanation. SVG is deliberately not accepted — served inline from this origin, one pasted into a channel is stored XSS.What a file is, decided from its bytes. A claim the browser could not make is an absence rather than a refusal: it is thrown away and the bytes are sniffed. A claim that does name a type is refused by name. The modality the model sees and the modality the transcript draws both come from the sniffed type, so a screenshot the browser mislabelled does not reach the model as mojibake or draw as a grey file card.
Two caps, because one of them is client-named. The per-message cap counts the rows one composer session staged, so two tabs get eight each rather than eight between them — the alternative counted a closed tab's leftovers and refused a pick the client had already accepted, naming files nobody could see. Since that bucket is a form field the client chooses, a second backstop counts every unsent row per person across all channels and groups: 32 rows, which bounds staged storage at 256 MiB without a legitimate person ever meeting it.
A run inlines a bounded number of bytes. History degrades to a note when the budget runs out. The asked message refuses loudly instead, because silently dropping a file from the message somebody just sent is the failure the strict path exists to prevent.
Unsent files are reclaimed. An hourly CronJob deletes staged attachments nobody came back to send, in resumable batches, needing only
DATABASE_URL— a job that deletes rows should not demand an encryption key.Breaking, for operators
networkPolicy.kubernetesApiCidris now required when network policies are on and computers are in sandbox mode. It was allowed to be empty, which rendered an egress rule carrying ports 443 and 6443 and no destination at all — permitting those ports to every address, and negating the private-range exceptions the rule beside it spells out.An existing sandbox install that never set it will fail
helm upgradeat template time. Nothing in the cluster changes until the value is supplied, and the message names the command that finds it:Typically
172.20.0.0/16on EKS and10.96.0.0/12on GKE and kubeadm.Known limitations
These are deliberate, not unknown. The full list with file-level detail is in the branch's review ledger.
markAttachmentsSenthas no row-count check, so a send matching no rows says nothing and the files are swept a day later under a message that still shows them.role="log"reading order are inverted. Confirmed, and fenced with a test — every in-file repair breaks the scroller's anchoring, so it needs a different approach rather than a patch.Not attachment work, and why it is here
app/tsconfig.jsonnow includestests. All 68 app test files were type-checked by nothing, which had already hidden two real bugs in this branch — a duplicate import, and a call passing three arguments to a four-parameter function whose config would have thrown the moment it ran. Enabling it surfaced 29 errors: 3 real fixture defects, 26 scaffolding. The six unrelated test files that had to compile are one labelled commit.app/serve.tsarrived on main and was checked by nothing for the same reason. With DOM inlib, bun-types resolvesWebSocketto the DOM one, which has noterminate(). Six real errors, fixed.Testing
bun test app738 pass ·bun test shared58 pass ·bun test server2191 pass / 8 fail.Those 8 are identical by name to
origin/main's own 8 — five agent-handoff, one people-paging, and two Mastra tests that need migration 0029's enum, which a fresh database gets and a long-lived local one may not. Zero new failures.Both typechecks clean, lint and formatter clean over 561 files, build clean,
helm lintand all five CI chart targets rendering and passingcheck-rendered-chart.