fix(core): reject PDF files to prevent session corruption (fixes #2020)#2024
fix(core): reject PDF files to prevent session corruption (fixes #2020)#2024drewd789 wants to merge 4 commits intoQwenLM:mainfrom
Conversation
|
@drewd789 Thank you for this well-thought-out contribution! The detailed test plan and clear documentation really helped us understand the issue. After comparing with #1982, we're going with that approach because it solves the same problem more comprehensively: instead of blocking PDFs entirely, it assigns appropriate modalities per model. This means we can support it once a model can read PDFs natively. Thanks again for taking the time to dig into this and write such a thorough PR! |
|
@tanzhenxin Thanks for the review! One clarification: PR #1982 prevents API 400 errors, but doesn't prevent session corruption. The base64 inlineData still embeds in the session file, flooding context and requiring manual cleanup. Our fix prevents the corruption at the source. Could PR #1982 be extended to also block session embedding for models without PDF support? That would give both benefits. Happy to discuss! |
|
@drewd789 Well, I think you made a valid point. We can optimize it at where the reading actually happens. But I think we should be consistent with all modalities, check and reject the binary with fallback text content if the model cannot support it. |
|
@tanzhenxin Yes, updated PR to integrate with the Changes:
Test coverage:
Does this look right to you, or would you like me to adjust anything? |
TLDR
Fixes PDF session corruption by rejecting PDF files at the source instead of embedding them as base64 inlineData (which caused API 400 errors). Users should extract PDF text externally before providing it to the tool.
Dive Deeper
PDF files were causing API 400 errors ("Invalid value: file. Supported values: 'text','image_url','video_url' and 'video'") and corrupting session files with base64 data, requiring manual cleanup with a Python script.
This fix rejects PDFs in
processSingleFileContent()with a clear error message directing users to extract text externally.Before vs After
read_fileread_fileread_fileMigration Guide
Users who previously relied on PDF embedding should:
pdftotext,pdfplumber, online converters)Reviewer Test Plan
Primary Test (PDF Rejection)
/read_file path/to/test.pdf(or use the read_file tool via chat)Regression Test (Images Still Work)
/read_file path/to/test.pngEdge Cases
.PDF).docx,.xlsx) - should also be rejected.txt,.md) - should still work normallyTesting Matrix
Files Changed
packages/core/src/utils/fileUtils.ts- Main fix inprocessSingleFileContent()packages/core/src/utils/fileUtils.test.ts- Test updatesdocs/developers/tools/file-system.md- Documentation updatesLinked issues / bugs
Fixes #2020