Skip to content

fix(contracts): read large file attachments from newer clients - #13429

Open
Bil0000 wants to merge 2 commits into
pingdotgg:mainfrom
Bil0000:fix-file-attachment-read-compat
Open

Bil0000 wants to merge 2 commits into
pingdotgg:mainfrom
Bil0000:fix-file-attachment-read-compat

Conversation

@Bil0000

@Bil0000 Bil0000 commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

What Changed

Allow ChatFileAttachment.sizeBytes to decode any positive file size. The upload command still enforces its current 50 MiB limit.

Why

A client with the current reader must be able to open a thread containing a larger file sent by a future client. Today, a 75 MB file attachment makes the whole message fail to decode. This reader change must reach stable desktop and mobile before the 100 MB upload cap in #13261 can be merged, unless maintainers explicitly accept the old-client compatibility risk.

Verification

  • 67 focused contract tests passed, including a 75 MB message decode, a zero-byte rejection, and the existing upload cap test.
  • Contracts typecheck and targeted formatting passed.
  • Targeted lint passed with one warning on an unchanged line.

Model: GPT-6. Harness: Codex.

Summary by CodeRabbit

  • Improvements
    • Attachment records in conversation history and thread streams can now include files larger than the upload size limit. Creating uploads remains subject to that limit.
    • Attachments must still have a size of at least one byte, and images with invalid MIME types remain invalid.
  • Tests
    • Updated validation coverage to check that oversized attachment records are accepted while zero-byte files and invalid image types are rejected.

@github-actions github-actions Bot added the vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. label Sep 24, 2026
@github-actions github-actions Bot added the size:XS 0-9 changed lines (additions + deletions). label Sep 24, 2026
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Sep 24, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at b7bf8c9

Macroscope's review found this PR approvable — This narrowly scoped contract fix lets older clients decode larger file metadata from newer clients while retaining the existing upload cap and image validations. Focused regression coverage confirms the broadened read behavior without introducing a new capability or deployment change.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: c4ae3f16-53a2-45d1-876a-749cdd27afb3

📥 Commits

Reviewing files that changed from the base of the PR and between df3e34c and b7bf8c9.

📒 Files selected for processing (2)
  • packages/contracts/src/orchestration.test.ts
  • packages/contracts/src/orchestration.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/contracts/src/orchestration.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

ChatFileAttachment.sizeBytes no longer enforces the upload size cap. Its minimum remains 1. Tests check decoding above the cap, zero-byte files, and images with invalid MIME types.

Changes

Attachment size validation

Layer / File(s) Summary
Update attachment size validation
packages/contracts/src/orchestration.ts, packages/contracts/src/orchestration.test.ts
ChatFileAttachment.sizeBytes retains its minimum of 1 but no longer has an upper limit. Tests accept a file one byte above the upload cap and reject zero-byte files and images with invalid MIME types.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Suggested reviewers: t3dotgg

Merge Risk: ⚪ Minimal · up to b7bf8

Newer clients can provide larger generic-file attachments for reading, while image and new-upload limits remain in place. No material merge-blocking risk was established.

Security Architecture Review

Security architecture risk: 🔵 Low · up to b7bf8

Current upload controls still limit newly uploaded files to 50 MiB, while the reader can open messages containing larger attachments. No introduced security issue was established, but provider handling and future rollout behavior are not fully verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The immediate contract change affects interpretation of attachment metadata, not the bytes permitted by the current upload input. Its downstream reach includes provider dispatch, so the effective limit depends on upload controls and the selected provider adapter.

Trust Boundaries and Controls

  • observed — The upload input caps declared file size, and server normalization checks claimed attachment metadata against the stored file. These are distinct controls from the widened history-reader schema.

Hardening Proposals

  • proposed — Before increasing the upload cap, verify file-size enforcement at each provider-send boundary independently of the history reader, and sequence the reader rollout ahead of the cap increase.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: allowing newer large file attachments to be read by contracts.
Description check ✅ Passed The description explains what changed, why the change is needed, and how it was verified. It omits the checklist, but the core required information is present and the change is small and focused.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

juliusmarminge added a commit to Bil0000/t3code that referenced this pull request Sep 25, 2026
Once pingdotgg#13429 lets ChatFileAttachment decode any positive size, message
decoding and sendTurn no longer reject a file one byte over the upload
cap. That boundary belongs to AttachmentCreateUploadUrlInput, which
assets.test.ts already covers, so drop the read-side rejection
assertions that would fail once both PRs land.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The reader regression test decoded a fixed 75 MB file. Once the upload cap
rises to 100 MB that value sits under the cap and no longer proves that a
reader tolerates files a newer build accepted. Derive it from the cap, and
note on the schema why sizeBytes has no upper bound.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@macroscopeapp
macroscopeapp Bot dismissed their stale review September 25, 2026 20:40

Dismissing prior approval to re-evaluate b7bf8c9

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS 0-9 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants