Skip to content

Add safe attachment upload groundwork - #150

Merged
morgmart merged 7 commits into
mainfrom
morganm/attachment-upload-support
Sep 23, 2026
Merged

morgmart merged 7 commits into
mainfrom
morganm/attachment-upload-support

Conversation

@morgmart

@morgmart morgmart commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

What this changes

First attachment-only split from #122, based on current main. Adds the development upload endpoint without exposing new composer controls or changing the editor/mention chips.

Files are signed for their captured community, with bounded size, concurrent requests, duration and response bytes. Invalid upload locations or mismatched file hashes are rejected. Existing authenticated generic downloads are reused; permission, format and metadata rejection remain relay-owned.

530 added lines total: 155 production and 375 tests. No repository documentation added. No dependencies, native adapters, UI or unrelated styling.

Validation

At 8adb26257071989b2aab42842510ebb7a572101c, clean working tree:

  • Required commit hooks passed with no bypass.
  • Required push hooks passed: TypeScript and 193 tests across 14 related files, including 21 new attachment cases.
  • Real local HTTP through the production broker proves signed binary upload → protected download, community routing, origin rejection, server error handling, invalid descriptors, cancellation, concurrency recovery and byte/deadline budgets. Upstream is controlled, not deployed.
  • No browser tests added or removed: no browser behavior/UI changed.
  • Independent review completed at this exact head with no blocking findings. Hosted DCO passed; broader hosted CI was still running at the last inspection. Draft retained pending hosted validation.

Audit and remaining work

Use one generic file model plus optional media previews, not per-extension controls. Old Buzz's source supports documents and archives separately from stricter media validation; ordinary documents are not universally metadata-cleaned. Berd's folders are local path references, not remotely shared copies. Folder bundling needs a separate product choice and is excluded here.

Public guidance: Drive storage versus previews, unreliable browser MIME hints, directory selection returns files/relative paths, layered upload controls.

Not yet validated: deployed relay acceptance, complete composer send/retry, packaged native uploads. The research CLI rejected a Markdown upload as unsupported; that is a CLI/path observation, not proof that this new endpoint or all relay file classes fail. Verify deployed acceptance before enabling controls.

Next slices retain files locally until Send everywhere: existing-conversation selection/removal/upload/recovery, then first-message conversation creation and destination-correct retry. Paste/drop and automatic metadata cleanup stay separate. No merge requested.

Latest update: removed docs/attachments.md at Morgan’s request in 00b9a724c8d3031b01ee73190faef0ecf4f81ca2. No implementation or test changes. Required push hooks passed TypeScript and 193 tests across 14 files at this head; earlier independent review applies to unchanged code. Hosted checks for this documentation-only follow-up remain separate from the earlier head’s results.

Review follow-up

At e853663d02ad66a383527b237f4cfe32b235dbef, preserved known upload error statuses before response-body parsing and cancelled unused bodies. Added five regression cases covering oversized/absent bodies, cancellation, and admission recovery. These failed before the fix and passed afterward; all 26 attachment tests passed. Required commit and push hooks passed without bypass, including TypeScript and 198 tests across 14 files. Independent focused review found no issues. No browser tests or UI changed. Hosted CI for this follow-up is pending; previous-head CI does not validate it. Existing deployed-relay and native validation gaps remain unchanged.

Browser test repair after syncing main

At c8eb68e19a3448596ddca6e49f894b4001af9daa, updated the two stale Conversation options interactions in the existing live-status browser tests to use Channel settings and its dedicated Close control. The Close control is required at narrow widths, where the panel covers the header. Test-only repair approved by Morgan; no production behavior, assertions, retries, or timeouts changed. Zero browser cases added or removed.

Validation: the previous head failed these scenarios in both hosted engines. A label-only local repair also reproduced the covered-header failure in both engines; the complete repair passed all six cases in tests/browser/live-status.spec.mjs on Chromium and WebKit (18.8s) at 16760ff plus the exact test diff committed here. Commit hooks made no changes. Required push hooks at this commit passed TypeScript and 200 tests across 14 files. Broader hosted CI for this head is pending; no merge performed.

Carl added 2 commits September 22, 2026 18:12
Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
@morgmart
morgmart marked this pull request as ready for review September 23, 2026 03:43
@morgmart
morgmart requested review from a team, comp615 and wesbillman as code owners September 23, 2026 03:43
Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

No blocking findings for the development-only attachment-upload groundwork at 00b9a724c8d3031b01ee73190faef0ecf4f81ca2 against 877ae2a6749221dd6850d5695b5c8de023a239e3. Reviewed community capture and signing, upload/response bounds, cancellation and admission recovery, descriptor validation, existing protected downloads, and test integration. One nonblocking error-classification note is inline.

Source-only review; no tests or live uploads executed by this review. Existing exact-head CI passed; Windows validation was skipped. Tests combine real local HTTP/broker/signing with a controlled upstream, plus direct helper cases for request-size/deadline limits. This does not prove deployed relay acceptance, composer send/retry, or packaged native uploads. Retain the PR’s deployed-acceptance gate before enabling controls; those later surfaces are not blockers for this endpoint-only split.

Comment thread dev/attachment-upload.mjs
Carl added 3 commits September 23, 2026 08:16
Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

No blocking findings for the development-only attachment-upload groundwork at c8eb68e19a3448596ddca6e49f894b4001af9daa against base/merge-base 78566abe9110445b8748e6eddfbe58ccc79f1706. Both independent review lanes are complete. This is a comment review, not approval.

  • The previous nonblocking error-status finding is resolved: known upstream statuses survive oversized/absent bodies; unused bodies are cancelled. Reviewed captured-community routing/signing, upload and response bounds, cancellation/admission recovery, descriptor validation and protected downloads. The browser-only repair uses the existing settings controls without weakening the recovery/composer assertions.
  • Exact-head hosted CI passes, including JavaScript, Rust/tool integration, browser measurements, all four browser shards and CI required; DCO/security checks also pass. Windows validation is skipped. I reviewed source and regression ordering rather than rerunning broad suites locally.
  • Scope and remaining gate: the tests use real local HTTP/broker/signing with a controlled upstream; they do not establish deployed relay acceptance. Keep that acceptance gate before exposing upload controls. Composer send/retry and packaged native uploads remain explicitly outside this split, not hidden claims of completion.

Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
@morgmart
morgmart merged commit 6e6417d into main Sep 23, 2026
12 checks passed
@morgmart
morgmart deleted the morganm/attachment-upload-support branch September 23, 2026 17:35
zrmarley added a commit that referenced this pull request Sep 23, 2026
…search-send

* origin/main:
  Connect attachments to existing message delivery (#176)
  perf: preserve unchanged thread row identities (#171)
  perf: cache markdown preparation by content (#172)
  Add safe attachment upload groundwork (#150)
  feat: add sampling profiler launch modes (#148)
  feat(channels): remove DMs from the sidebar (#157)
  Distinguish namesake agents and selected recipients (#142)
  feat(channels): move diagnostics into Channel Settings (#163)
  Replace warning banners with shared Base UI toasts (#164)
  feat(shortcuts): add keyboard shortcut settings (#155)
  fix(channels): give floating unread cue an opaque panel surface (#153)
  feat(communities): add BUZZ_DEV_OPEN_RELAY to open the default relay on fresh dev ports (#151)
  Restore recipient avatars beside the composer mention tool (#162)
  Fix startup inventory duplication and late panel scroll shifts (#160)
  feat(channels): add channel creation (#138)
  Standardize Button and IconButton with Buzz design tokens (#145)

Signed-off-by: Zach Marley <zmarley@squareup.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants