fix(session): bound oversized PDF requests - #1529
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe change separates request-size failures from context overflow, adds budgeted media projections, and retries oversized model requests with degraded and stripped media. PDFs use path-only handling, while compaction and tests adopt the new projection behavior. ChangesRequest-size media recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SessionProcessor
participant nextMediaMessages
participant MessageV2
participant LLM
SessionProcessor->>LLM: submit normal media messages
LLM-->>SessionProcessor: request_too_large
SessionProcessor->>nextMediaMessages: request reduced projection
nextMediaMessages->>MessageV2: nextMediaMessages(current)
MessageV2-->>nextMediaMessages: degraded or stripped projection
nextMediaMessages->>MessageV2: render projected model messages
MessageV2-->>nextMediaMessages: reduced messages
nextMediaMessages-->>SessionProcessor: projection and messages
SessionProcessor->>LLM: replay with reduced media
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Root cause: The repository-wide dependency audit resolved vulnerable tar, brace-expansion, fast-uri, js-yaml, and sharp releases. Bun cannot express the path-scoped js-yaml override needed for the exact vulnerable transitive pin, while gray-matter still called js-yaml APIs removed in v4. Changes: - Pin safe tar, fast-uri, js-yaml, and Sharp releases. - Refresh all vulnerable brace-expansion major lines without forcing incompatible versions. - Patch gray-matter to use the supported load and dump APIs and record its compatible js-yaml range. - Add a frontmatter parse/stringify regression test. Verification: - Frozen reinstall and bun audit at high severity passed with zero findings. - Frontmatter tests: 38 passed; desktop icon tests: 10 passed. - Targeted typechecks, app build, and desktop build passed. - Full PR CI passed across macOS, Windows, CodeQL, dependency review, desktop smoke, E2E artifacts, and performance baseline. Review: Cold review found no P0-P3 issues and no unresolved review threads remained. Related: Follow-up to the audit failure observed while merging #1529. Residual risk: Sharp 0.35 carries its documented minor-version breaking changes, but current call sites use supported APIs and cross-platform install/build checks passed.
Summary
Keep path-backed PDFs out of provider payloads, bound aggregate media projected into each model request, and recover safely from HTTP 413 by retrying only materially smaller media projections.
There is no related GitHub issue; this PR addresses a directly reported desktop bug where attaching many PDFs could make the model request unresponsive.
Why
Path-backed PDFs were automatically read and promoted into inline base64 media whenever the selected model accepted PDF/image input. Multiple PDFs therefore expanded one user turn into a large provider request without an aggregate count or byte budget. HTTP 413 was also treated as token context overflow, which sent a request-body problem into compaction instead of reducing media.
Related Issue
None. This came from a direct user bug report.
Human Review Status
Pending
Review Focus
Please focus on the request-only projection boundary, the 8-item / 12 MiB aggregate media limits, and the finite HTTP 413 recovery path. In particular, verify that explicit context-overflow evidence still wins over a bare 413 and that no retry is sent when the next media projection would be identical.
Risk Notes
The main behavioral tradeoff is that older media can be represented by omission markers once the aggregate provider-request budget is reached; durable conversation history and original file paths remain unchanged. HTTP 413 recovery is limited to projections with a strictly smaller selected-media set and still passes through the existing side-effect safety gate.
The visible-UI conditional checklist item is left unticked because no UI or copy surface changed. The docs/release/dependency conditional item is left unticked because this PR changes no docs, release metadata, dependencies, permissions, credentials, deletions, generated files, or local-only files.
How To Verify
Screenshots or Recordings
Not applicable — no visible UI changes.
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit
New Features
Bug Fixes
Tests