Connect attachments to existing message delivery

[morgmart]

What this does

Connects completed file uploads to the existing message-delivery system, including messages and replies containing only attachments. This builds on #150; it is groundwork, not an attachment interface.

No composer UI, installed-app upload support, photo/video preparation, or documentation changes are included.

Why it matters

The future attachment interface can use the same permissions, cancellation, and delivery behavior as ordinary messages. Retrying a failed message reuses its signed event and completed uploads rather than uploading the files again.

How it works

The broker advertises upload support, the connection performs the upload, and the shared session owns permission checks and cancellation. Caller cancellation, session disposal, cache clearing, and authoritative access loss invalidate in-flight uploads and prevent late results from escaping.

Only completed uploads enter the existing outbox. Before sending, their URLs and metadata are checked against the intended community; filenames are normalized for safe links and relay-compatible metadata. Uploads retain the existing 20 MiB limit, with bounded responses and a deadline. Unsupported connections do not expose upload capability.

Verification

• Pre-push checks passed at dc59e3ae52dc20ee803954eb4b5bb5e721604b66: TypeScript, 1,218 related tests across 85 files, and design-system guards. Pre-commit formatting, lint, and security checks also passed.
• The 39 directly affected tests cover upload responses and failures, cancellation and late results, access loss, attachment-only messages/replies, and same-event delivery retry. The integration test exercises a real local broker through binary upload, signed message publication, and receipt.
• Independent pre-push review found a filename bidi-control issue. A failing regression reproduced it; the fix reuses the receiver's safety policy and covers both the link label and attachment metadata. Re-review found no remaining blockers.
• No browser cases added or removed: this slice has no UI. Browser/native builds, packaged-app connectivity, and live-service upload/download acceptance remain unverified; local-broker integration does not establish those results. Hosted checks are separate from the local results above.

Originating conversation: buzz://message?channel=72d6edc1-3d68-43d1-a359-004c37902b25&id=f50a47adff85862c7911ee8398095b8a28295d8af8e0d75457bede2501fb5e0e

[morgmart]

Carl, an automated reviewer, commenting via Morgan’s GitHub account.

Royal Court review: changes needed

Reviewed 1f84835bbb7bbb83af8a4b45094ca3414ffa7a27..dc59e3ae52dc20ee803954eb4b5bb5e721604b66. All three independent lanes have returned: Princess Donut (architecture), Mongo (runtime boundaries), and Mordecai (test honesty). Carl verified the proposed findings and traced the completed-upload → message → receiving projection contract. No source changes were made during this review.

P2 [Must Fix]: escape character references in attachment labels

src/features/relay/attachments.ts:79–86 escapes Markdown punctuation but leaves & unescaped. Through the production attachmentMessage → foldMessages path at this exact head, a generic file named report ©.pdf becomes report ©.pdf; report A.pdf becomes report A.pdf.

This is not merely how the raw Markdown looks. The receiver parses character references, then prefers the parsed link label over the correct imeta filename (message-content.ts:180–193, fold.ts:122–127). FileAttachment.tsx:54,100 uses that changed name for both the displayed file and suggested download filename.

Smallest correction: escape ampersands when building the Markdown label, without changing the normalized metadata filename or receiver precedence. A read-only probe applying that escape preserved both named and numeric examples through the real fold. Add regression assertions through foldMessages for the original normalized filename, one attachment, and intended visible message content.

Mordecai independently identified the coverage gap behind this failure: attachments.test.ts:136–163 calls parseAttachments directly, skipping Markdown projection, while broker.integration.test.ts:127–139 checks the outgoing event but not the receiving fold. Consolidated here rather than reported as a second blocker.

Sound boundaries and scope

The existing session remains the permission/lifetime owner, transport owns upload I/O, and the existing outbox owns signing and exact-event retries. Community-bound descriptors, response bounds, known HTTP errors, and cancellation/late-result fencing remain sound in the reviewed scope. Mongo’s direct label/metadata assessment does not cover the downstream Markdown decoding demonstrated above.

This is headless groundwork, not UI enablement. No visual/design-system pass was applicable. Installed-app connectivity, media preparation, new-conversation support, and live-service acceptance remain explicitly outside this slice. No browser/native/live-service validation is inferred from local broker tests.

Prior-work check: builds on merged #150; #122 still contains the older composer stack and earlier upload-timing prose, which is not authority over Morgan’s newer direction. #165 separately changes attachment preservation on edits and overlaps messages.ts; retain both contracts when integrating, without expanding this PR into that work.

Verification used the existing hook/CI evidence plus focused, read-only production-module probes at the reviewed head; broad suites were not rerun by Carl. Exit criterion: the minimal escaping correction and receiver-level regressions pass, with existing ownership and scope unchanged. No approval or merge is given.

GitHub does not allow this account to request changes on its own PR. This COMMENT records the same changes-needed verdict; it is not an approval.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

No remaining blocking findings at c01a4b626517316558d61dcedb01e578c1180f24. The architecture fits this headless attachment-delivery foundation. This is a review verdict, not a GitHub approval or merge authorization.

• Ownership is sound: the connection owns upload I/O; the shared session owns participation/cancellation and rejects late results after caller cancellation, disposal, cache clearing or authoritative access loss. Completed descriptors enter the existing community/viewer-partitioned outbox, which persists before publishing and reuses the signed event on retry. No competing upload queue, delivery owner or cache was introduced.
• The concrete defect was fixed during review: at dc59e3a, literal character references in filenames were decoded by the receiving Markdown fold and overrode correct filename metadata. I independently reproduced the author-side finding. c01a4b6 adds the minimal ampersand escape and three full-fold regressions. I reviewed the entire intervening diff and ran 28 focused production-module probes covering attachment-only/captioned roots and replies, named/decimal/hex references, slash/bidi references and punctuation; all preserved the intended filename and content. No source edits were made by this review.
• Validation remains bounded: all three delegated lanes returned, covering upload/metadata trust, session lifetime/access and message/outbox recovery. I traced broker upload → descriptor → message/root/reply → persistence/sign/publish/retry → receiving fold/protected media. Previous-head hosted checks passed with Windows skipped; the fix-head CI is still running at this check. Broad suites were not rerun. Browser/native/live-service upload/download acceptance remains unverified; missing composer UI, installed-app upload support and media preparation are explicit non-goals, not blockers.

Non-blocking follow-up for UI integration: attachment-only replies have a blank unread-activity preview because unread.ts:405–422 keeps only folded text. This consumer behavior predates the PR; use an attachment-name/type fallback when integrating that surface. It does not justify expanding this foundation’s blocking scope.

Remaining merge gates: successful checks on the current head and required human approval. No architectural redesign is needed for this slice; it is not certification of the complete attachment feature.