Skip to content

fix(core): Gate large PDF text extraction - #6409

Merged
doudouOUC merged 12 commits into
QwenLM:mainfrom
doudouOUC:fix/pdf-read-budget-reference
Jul 7, 2026
Merged

fix(core): Gate large PDF text extraction#6409
doudouOUC merged 12 commits into
QwenLM:mainfrom
doudouOUC:fix/pdf-read-budget-reference

Conversation

@doudouOUC

Copy link
Copy Markdown
Collaborator

What this PR does

This PR adds a PDF read budget policy so text-only PDF handling no longer injects full large-document extraction results into the prompt. Large full-PDF text fallback now returns short guidance to use the pages parameter, @-attached large PDFs become lightweight references instead of failed reads, and explicit page-range extraction remains supported with an additional token guard for dense pages.

Why it's needed

A user reported that reading a 100-page PDF with a text-only model produced a context overflow after automatic compression. The root cause was that the PDF fallback extracted roughly 100k characters with pdftotext, which could push the next request past the hard prompt safety limit before the model had a chance to recover. Keeping large PDFs as references and requiring explicit page ranges prevents that oversized tool result from entering conversation context.

Reviewer Test Plan

How to verify

Run the focused PDF/read tests and confirm they pass. Attach a large PDF on a text-only model and confirm the attachment contributes a short reference that tells the model to call read_file with pages; call read_file on the same PDF without pages and confirm it returns a short file_too_large error; call read_file with a narrow pages range and confirm text extraction still works unless the extracted page range exceeds the new output budget.

Evidence (Before & After)

Before: the reported 100-page PDF produced about 100k characters of extracted text and the next request failed with Context is too large to send safely after automatic compression. After: local validation against the same PDF returns a 174-character reference for @ attachment behavior, a 174-character file_too_large guidance for no-pages read_file, and still allows pages: "1" extraction.

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested
🐧 Linux ⚠️ not tested

Environment (optional)

Node.js v22.22.3 on macOS. Validation used npx vitest run src/utils/pdf.test.ts src/utils/fileUtils.test.ts src/utils/readManyFiles.test.ts src/tools/read-file.test.ts, npm run lint, npm run typecheck, and npm run build && npm run typecheck.

Risk & Scope

  • Main risk or tradeoff: text-only models now require explicit pages ranges for PDFs above the full-text page limit, so some previously accepted full-document reads become short guidance responses.
  • Not validated / out of scope: OCR, rendering PDF pages as images, changing auto-compaction thresholds, and changing edit authorization semantics for PDF reads are out of scope.
  • Breaking changes / migration notes: no API migration is required; native PDF-capable model behavior is preserved, and explicit pages reads remain supported with the existing 20-page request limit.

Linked Issues

Closes #6408

中文说明

What this PR does

这个 PR 增加了 PDF 读取预算策略,避免纯文本 PDF 处理把大型文档的完整提取结果注入 prompt。大型 PDF 的全文文本回退现在会返回简短 guidance,引导使用 pages 参数;通过 @ 附加的大型 PDF 会变成轻量 reference,而不是失败读取;显式 page-range 提取仍然支持,并额外增加了针对高密度页面文本的 token guard。

Why it's needed

用户反馈在纯文本模型下读取一个 100 页 PDF 后,自动压缩之后仍然出现上下文溢出。根因是 PDF fallback 通过 pdftotext 提取了约 100k 字符,导致下一次请求在模型有机会恢复前就超过 hard prompt safety limit。把大型 PDF 保持为 reference,并要求显式页码范围,可以避免这种过大的 tool result 进入对话上下文。

Reviewer Test Plan

How to verify

运行聚焦的 PDF/read 测试并确认通过。在纯文本模型下附加大型 PDF,确认附件只贡献一段简短 reference,并提示模型使用带 pagesread_file;对同一 PDF 调用不带 pagesread_file,确认返回简短的 file_too_large 错误;再用较窄的 pages 范围调用 read_file,确认文本提取仍然可用,除非该页码范围的提取文本超过新的输出预算。

Evidence (Before & After)

Before:反馈中的 100 页 PDF 会产生约 100k 字符的提取文本,下一次请求失败并报 Context is too large to send safely after automatic compression。After:对同一个 PDF 的本地验证中,@ 附件行为返回 174 字符 reference,不带 pagesread_file 返回 174 字符 file_too_large guidance,同时 pages: "1" 仍然可以进行文本提取。

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested
🐧 Linux ⚠️ not tested

Environment (optional)

macOS 上的 Node.js v22.22.3。验证使用了 npx vitest run src/utils/pdf.test.ts src/utils/fileUtils.test.ts src/utils/readManyFiles.test.ts src/tools/read-file.test.tsnpm run lintnpm run typecheck,以及 npm run build && npm run typecheck

Risk & Scope

  • Main risk or tradeoff:纯文本模型现在会要求超过全文页数限制的 PDF 使用显式 pages 范围,因此部分过去可接受的全文读取会变成简短 guidance 响应。
  • Not validated / out of scope:OCR、将 PDF 页面渲染为图片、修改自动压缩阈值、修改 PDF 读取的编辑授权语义都不在本次范围内。
  • Breaking changes / migration notes:不需要 API 迁移;支持原生 PDF 的模型行为保持不变,显式 pages 读取仍然支持,并保留现有的每次请求 20 页限制。

Linked Issues

Closes #6408

Prevent text-only PDF fallback from injecting full large-document extraction results into the prompt. Large attachment reads now become short references, direct no-pages reads return a short file-too-large error, and page-range extraction is token guarded.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@doudouOUC

Copy link
Copy Markdown
Collaborator Author

Verification against the latest PR branch commit 7118fc39f using the real reported PDF (spark_of_gpt4.pdf, 100 pages, 6,363,772 bytes):

Large PDF read verification

Key results:

{
  "branch": "fix/pdf-read-budget-reference",
  "commit": "7118fc39f",
  "pdf": {
    "sizeBytes": 6363772,
    "pageCount": 100
  },
  "rawPdftotextLowLevel": {
    "chars": 100100,
    "estimatedTokens": 25041,
    "truncated": true
  },
  "attachmentPath": {
    "error": null,
    "contentChars": 174,
    "cacheState": "fresh",
    "cacheable": false
  },
  "readFileNoPages": {
    "errorType": "file_too_large",
    "chars": 174
  },
  "readFilePage1": {
    "errorType": null,
    "chars": 23182
  },
  "nativePdfPath": {
    "isInlineData": true,
    "errorType": null
  }
}

This confirms the original oversized context path is removed for the user-facing entry points: raw pdftotext can still produce a 100,100-character result, but @ attachment behavior now contributes only a 174-character reference, and read_file without pages returns a 174-character file_too_large guidance response. Narrow explicit page reads still work, and native PDF-capable model behavior remains preserved.

@doudouOUC doudouOUC self-assigned this Jul 6, 2026
@doudouOUC
doudouOUC marked this pull request as ready for review July 6, 2026 16:40
Copilot AI review requested due to automatic review settings July 6, 2026 16:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a PDF read budget policy in packages/core to prevent large text-only PDF extractions from overflowing prompt context, while keeping explicit page-range extraction available (with an additional output-size guard) and making @-attached large PDFs resolve to lightweight guidance references.

Changes:

  • Gate full-document PDF text fallback (text-only models) behind a page-count/size heuristic, returning short guidance to use pages instead of inlining large extractions.
  • Treat large @-attached PDFs as references (guidance without a failed read) via largePdfBehavior: 'reference' in readManyFiles.
  • Add a token-budget guard for page-range extraction results and update tool/docs/tests to use shared PDF constants.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/core/src/utils/readManyFiles.ts Passes largePdfBehavior: 'reference' so @-attached large PDFs don’t fail reads but instead provide guidance.
packages/core/src/utils/readManyFiles.test.ts Adds coverage asserting large PDF attachments produce short guidance and are recorded as non-cacheable reads.
packages/core/src/utils/pdf.ts Adds budget-policy helpers/constants, page-count heuristic, guidance builders, and truncated metadata on extraction results.
packages/core/src/utils/pdf.test.ts Adds unit tests for the new PDF budget helpers and truncated behavior.
packages/core/src/utils/fileUtils.ts Enforces large-PDF gating for text extraction without pages and adds a token guard for dense page-range output.
packages/core/src/utils/fileUtils.test.ts Adds focused tests for the new gating/reference behavior and dense-output rejection.
packages/core/src/tools/read-file.ts Centralizes the max-pages-per-read constant in tool schema/docs and validation messaging.
packages/core/src/tools/read-file.test.ts Adds coverage ensuring large PDF reads without pages return a short file_too_large guidance error for text-only models.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/core/src/utils/readManyFiles.test.ts
@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR, @doudouOUC!

Template looks good ✓

Problem: observed bug with clear reproduction. Issue #6408 documents that reading a 100-page PDF on a text-only model produces ~100k characters of extracted text, causing context overflow after automatic compression. The PR author provided verification evidence against the real reported PDF (spark_of_gpt4.pdf, 100 pages, 6.3MB).

Direction: well-aligned with the project's context management goals. Preventing unbounded tool results from entering conversation context is a clear safety improvement. The layered approach (size cap → page-count gate → post-extraction token guard) mirrors proven patterns.

Size: 9 files changed, 971 additions, 66 deletions. Production logic: 253 lines (fileUtils.ts: 154+27, pdf.ts: 70+5, readManyFiles.ts: 1+0, textTokenizer.ts: 22+18, read-file.ts: 6+7). Tests: 719 lines. All changes are within packages/core/src/ — core paths are touched. Under 500 production lines, no hard block. This is a fix-type PR, not a refactor.

Approach: the scope feels right. Each layer addresses a specific failure mode — size cap handles absurdly large files, page-count gate prevents full-document extraction on multi-page PDFs, token guard catches dense single-page extractions. The largePdfBehavior: 'reference' option for @-attached PDFs is a clean separation. The estimateTextTokens extraction from TextTokenizer is a legitimate reuse improvement. No unrelated changes or scope creep detected.

Moving on to code review. 🔍

中文说明

感谢贡献,@doudouOUC

模板完整 ✓

问题:已观测到的 bug,有明确复现。Issue #6408 记录了在纯文本模型下读取 100 页 PDF 产生约 100k 字符提取文本,自动压缩后上下文溢出。PR 作者提供了针对实际报告 PDF(spark_of_gpt4.pdf,100 页,6.3MB)的验证证据。

方向:与项目的上下文管理目标高度一致。防止无界 tool result 进入对话上下文是明确的安全改进。分层方案(大小上限 → 页数门控 → 提取后 token 守卫)与已验证的模式一致。

规模:9 个文件变更,971 行新增,66 行删除。生产逻辑:253 行。测试:719 行。全部变更在 packages/core/src/ 内——触及核心路径。低于 500 生产行,无硬阻断。这是 fix 类型 PR,非 refactor。

方案:范围合理。每层解决特定故障模式——大小上限处理极大文件,页数门控防止多页 PDF 全文提取,token 守卫捕获高密度单页提取。@ 附加 PDF 的 largePdfBehavior: 'reference' 选项是干净的分离。estimateTextTokensTextTokenizer 提取是合理的复用改进。未检测到无关改动或范围蔓延。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal (before reading the diff): I would have added a pdfinfo-based page-count check before running pdftotext, returned a short error/guidance when the page count exceeds a threshold, added a largePdfBehavior option for @-attachment callers, and guarded post-extraction output with a token budget. The PR's implementation matches or exceeds this proposal — the layered guards (size cap → page-count gate → post-extraction token guard) are well-structured and cover edge cases I didn't initially consider (size heuristic when pdfinfo is unavailable, separate caps for full-text vs. paged extraction, UTF-8 byte-length check for maxBuffer recovery).

Reuse check: estimateTextTokens is correctly extracted from the existing TextTokenizer.calculateTokensSync — same algorithm, now available as a standalone function for the PDF token guard. No duplication.

Correctness: the page-count gate uses pdfinfo first, falls back to sizeBytes / 100KB heuristic when pdfinfo is unavailable. The largePdfBehavior: 'error' | 'reference' split is clean — @-attached PDFs get soft references, explicit read_file calls get hard errors. The post-extraction token guard at 12K tokens catches dense single-page PDFs that the page-count gate wouldn't catch. The maxBuffer recovery in extractPDFText now uses Buffer.byteLength instead of stdout.length — correct for non-ASCII text.

One suggestion-level note (non-blocking): the 100MB full-text extraction size cap (PDF_FULL_TEXT_EXTRACTION_MAX_MB) doesn't check largePdfBehavior, so an @-attached >100MB PDF gets a hard error instead of a soft reference. Minor edge case — wenshao flagged this in the review suggestion summary.

No critical blockers found. Code follows project conventions, no over-abstraction, changes are in the right package.

Verification

Unit tests: all 254 tests pass across 4 files (pdf.test.ts, fileUtils.test.ts, readManyFiles.test.ts, read-file.test.ts).

Build + typecheck + lint: all pass clean.

Real-Scenario Testing (tmux)

Tested on a 28-page synthetic PDF with npm run dev (PR branch, qwen3.7-max model).

Test 1: PDF read with pages: '1-5' — ✅ PASS

> Read the PDF at tmp/triage-pdf-test/large.pdf using pages 1-5 and summarize the content.

∴ Thought for 2s
 ✓ Read 1 file

∴ Thought for 1s

◆ Summary of pages 1–5:

  This is a synthetic test PDF. Every page (1 through 5, and beyond)
  contains the same repeated filler text: "This is test content for page." —
  duplicated many times per page with no meaningful variation or structure.

Explicit page-range extraction works correctly — model reads and summarizes the content.

Test 2: PDF read without pages — ✅ PASS (gate triggers correctly)

> Now read the same PDF without specifying any pages parameter.

∴ Thought for 2s
 x ReadFile {"file_path":".../tmp/triage-pdf-test/large.pdf"}
   PDF "large.pdf" has 28 pages, which is too many to read at once. Use the
   'pages' parameter to read a specific page range such as '1-5'. Maximum
   20 pages per request.

∴ Thought for 1s

◆ The tool refused to read the full PDF. It detected 28 pages and enforces a
  20-page maximum per request, returning this guidance:

  │ Use the pages parameter to read a specific page range such as 1-5.

  So the PDF has 28 pages total, and you'd need to read it in chunks
  (e.g., 1-20, then 21-28) to get full coverage.

The page-count gate fires correctly: 28 pages > 10-page full-text limit → short guidance returned instead of full extraction. The model correctly interprets the guidance and reports it to the user.

Note: this test used qwen3.7-max which has native PDF support. The gate triggered because the model's read_file tool call went through the text-extraction fallback path. On a purely text-only model, the same gate would fire earlier via the @-attachment largePdfBehavior: 'reference' path.

中文说明

代码审查

独立方案(读 diff 前):在 pdftotext 前加 pdfinfo 页数检查,超阈值时返回短 guidance,为 @ 附件添加 largePdfBehavior 选项,用 token 预算守卫提取后输出。PR 实现匹配或超越此方案——分层守卫(大小上限 → 页数门控 → 提取后 token 守卫)结构清晰,覆盖了初始未考虑的边界情况。

复用检查: estimateTextTokensTextTokenizer.calculateTokensSync 正确提取——相同算法,现可独立使用。无重复。

正确性: 页数门控优先用 pdfinfo,不可用时回退 sizeBytes / 100KB 启发式。largePdfBehavior: 'error' | 'reference' 分离干净。12K token 提取后守卫捕获高密度单页 PDF。maxBuffer 恢复改用 Buffer.byteLength,对非 ASCII 文本正确。

一个建议级备注(非阻断):100MB 全文提取大小上限不检查 largePdfBehavior@ 附加 >100MB PDF 会得到硬错误而非软 reference。

未发现关键阻断问题。

验证

单元测试: 4 个文件共 254 个测试全部通过。

构建 + 类型检查 + lint: 全部通过。

真实场景测试(tmux)

在 28 页合成 PDF 上用 npm run dev(PR 分支,qwen3.7-max 模型)测试。

测试 1:带 pages: '1-5' 读取 — ✅ 通过。显式页码范围提取正常工作。

测试 2:不带 pages 读取 — ✅ 通过(门控正确触发)。28 页 > 10 页全文限制 → 返回短 guidance 而非全文提取。模型正确解释 guidance 并报告给用户。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

This PR solves a real context overflow bug with a focused, well-tested fix. The before/after evidence is clear: the 28-page test PDF goes from extracting full text (which would overflow context on a text-only model) to returning a 170-character guidance message that tells the model to use pages.

The layered guard design is sound — size cap, page-count gate, and post-extraction token guard each catch a distinct failure mode without overlapping. The largePdfBehavior: 'reference' option cleanly separates @-attachment behavior from explicit read_file calls. The estimateTextTokens extraction is a legitimate reuse improvement that avoids duplicating the token estimation logic.

All 254 unit tests pass, build/typecheck/lint are clean, and tmux testing confirms both the happy path (explicit pages) and the gate path (no pages) work as designed.

My independent proposal matched the PR's approach — the implementation covers edge cases I would have missed (size heuristic when pdfinfo is unavailable, UTF-8 byte-length for maxBuffer recovery). One suggestion-level note about the 100MB size cap not honoring largePdfBehavior remains — minor edge case, not blocking.

The PR has iterated through 5 commits addressing maintainer feedback (wenshao has approved the latest revision). Every change in the diff serves the stated goal — no scope creep, no drive-by refactors.

Approving. ✅

中文说明

这个 PR 用聚焦且测试充分的修复解决了一个真实的上下文溢出 bug。before/after 证据清晰:28 页测试 PDF 从提取全文(在纯文本模型上会溢出上下文)变为返回 170 字符的 guidance 消息,引导模型使用 pages 参数。

分层守卫设计合理——大小上限、页数门控和提取后 token 守卫各自捕获不同故障模式,互不重叠。largePdfBehavior: 'reference' 选项干净地分离了 @ 附件行为和显式 read_file 调用。estimateTextTokens 提取是合理的复用改进。

254 个单元测试全部通过,构建/类型检查/lint 全部干净,tmux 测试确认了正常路径(显式 pages)和门控路径(无 pages)均按设计工作。

独立方案与 PR 一致——实现覆盖了我可能遗漏的边界情况。一个建议级备注:100MB 大小上限不支持 largePdfBehavior——小概率边缘情况,不阻断。

PR 经过 5 次提交迭代,处理了 maintainer 反馈(wenshao 已 approve 最新版本)。diff 中每项变更都服务于目标——无范围蔓延,无顺手重构。

批准 ✅

Qwen Code · qwen3.7-max

@qwen-code-ci-bot qwen-code-ci-bot 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.

LGTM, looks ready to ship. ✅

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Comment thread packages/core/src/utils/fileUtils.ts
@wenshao

wenshao commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Suggestions — commit a6af06a7

File Issue Suggested fix
fileUtils.ts:1049 100MB full-text size cap ignores largePdfBehavior: 'reference' — @-attached >100MB PDF gets hard error instead of soft reference When largePdfBehavior === 'reference', return guidance without error/errorType, matching the page-count gate's reference-mode shape
fileUtils.ts:1281 No test verifies that pages forces pdftotext extraction when model has native PDF modality Add a test with modalities: { pdf: true } + { pages: '1-3' } asserting pdftotext path is taken
read-file.ts:393 Tool description implies native-PDF models have no size limitation for full reads (they still hit 10MB base64 cap) Broaden to "Large PDFs cannot be read all at once; retry with narrower page ranges"
readManyFiles.ts:196 Cache records PDF reference result as full read (full: true) even though content was never delivered Set isTruncated: true on reference results or skip recordAttachedFileRead for guidance-only returns
pdf.ts:66-70 buildPDFTextTooLargeGuidance suggests '1-2' as "narrower" example when user already used '1-2' Compute an actually-narrower example from the user's range, or switch to single-page message for ranges ≤2 pages
readManyFiles.test.ts:218 No integration test for post-extraction token-guard reference path through readManyFiles Add test with small PDF producing dense text >12K tokens, verify reference behavior and cache recording

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/utils/pdf.ts
Comment thread packages/core/src/utils/fileUtils.ts Outdated
Comment thread packages/core/src/utils/fileUtils.ts
doudouOUC and others added 2 commits July 7, 2026 01:43
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Comment thread packages/core/src/utils/fileUtils.ts Outdated
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>

@qwen-code-ci-bot qwen-code-ci-bot 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.

⚠️ Downgraded from Approve to Comment: CI still running.

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/utils/fileUtils.ts Outdated
Comment thread packages/core/src/utils/pdf.ts Outdated
@wenshao

wenshao commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>

@wenshao wenshao 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.

All previously identified Critical findings have been addressed in the 4 new commits. The PDF budget policy is well-structured with layered guards (size cap → page-count gate → post-extraction token gate), thorough test coverage (244 tests), and clean code. LGTM! ✅

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/tools/read-file.test.ts
Comment thread packages/core/src/utils/pdf.ts Outdated
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Comment thread packages/core/src/utils/pdf.ts
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Comment thread packages/core/src/utils/fileUtils.ts Outdated

@qwen-code-ci-bot qwen-code-ci-bot 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.

Review Summary — PR #6409

The layered PDF read-budget policy (size cap → page-count gate → post-extraction token guard) is a sound approach to preventing context overflow. The largePdfBehavior: 'reference' path for @-attached PDFs is a good degradation strategy. Test coverage is thorough.

Below are a few suggestion-level observations for consideration. None are blockers.

Comment thread packages/core/src/utils/fileUtils.ts
Comment thread packages/core/src/utils/fileUtils.ts
Comment thread packages/core/src/utils/fileUtils.ts
Comment thread packages/core/src/utils/fileUtils.test.ts
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Comment thread packages/core/src/utils/fileUtils.ts Outdated
Comment thread packages/core/src/utils/fileUtils.ts
Comment thread packages/core/src/utils/pdf.ts
Comment thread packages/core/src/utils/pdf.ts Outdated
Comment thread packages/core/src/utils/fileUtils.ts
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@doudouOUC
doudouOUC enabled auto-merge July 7, 2026 02:41

@wenshao wenshao 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.

Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.

@qwen-code-ci-bot qwen-code-ci-bot 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.

No review findings. Downgraded from Approve to Comment: CI still running.

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/utils/pdf.ts
@wenshao

wenshao commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>

@qwen-code-ci-bot qwen-code-ci-bot 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.

LGTM, looks ready to ship. ✅

@qwen-code-ci-bot qwen-code-ci-bot added category/core Core engine and logic category/tools Tool integration and execution scope/file-operations File system operations scope/token-management Token handling and limits type/bug Something isn't working as expected labels Jul 7, 2026
@wenshao

wenshao commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

✅ Local verification — works as intended, fixes #6408

I verified this PR locally on Linux (the PR's Tested on table marks Linux ⚠️ not tested) by driving the real processSingleFileContent code path against hand-authored multi-page PDFs, with real pdftotext / pdfinfo (poppler 25.03.0) — nothing mocked. Identical scenarios were run against the merge-base (pre-PR) and the PR-head worktrees for a clean before/after.

TL;DR — the #6408 root cause reproduces on base: a 100-page PDF injects 100,100 characters into the tool result. With this PR the same read returns a 167-character guidance message. The oversized tool result that overflowed context after auto-compaction is gone.

Before / After — real PDFs, text-only model, real pdftotext

# Input → call BASE (pre-PR) MERGED (#6409)
A big100.pdf · read_file (no pages) 100,100 chars, no error 167 chars + file_too_large
B big100.pdf · @-attach 100,100 chars 167 chars reference, no error
C big100.pdf · pages="1-2" 2,288 chars 2,288 chars — preserved ✅
D dense1.pdf · pages="1" (1 dense page ≈14.5k tok) 58,093 chars 264 chars + file_too_large
E pages15.pdf · read_file (no pages) 2,877 chars, no gate 167 chars + file_too_large
F small3.pdf · read_file (no pages) 573 chars 573 chars — preserved ✅

Fixtures: big100.pdf = 100 pages / ~115k extractable chars · dense1.pdf = 1 tall page / ~58k chars · pages15.pdf = 15 pages of tiny text · small3.pdf = 3 pages (control).

What each row confirms

  • A / BLarge PDF reads can overflow prompt context #6408 is real and fixed; direct read becomes a file_too_large error, @-attach becomes a lightweight reference (returnDisplay: "Referenced large PDF", no error).
  • C / F — explicit page-range reads and small PDFs are byte-for-byte unchanged.
  • D — the token-budget guard rejects a single dense page (14,540 est. tokens > 12,000 limit) with short guidance.
  • E — the gate keys on page count (15 > 10), so a 15-page doc is gated even though its text is tiny. Worth being aware of, but consistent with the design.

Edge cases (also real, unmocked)

  • Native-PDF model (modalities.pdf = true): big100.pdf → base64 inlineData, gate fully bypassed. Native PDF behavior is untouched.
  • poppler split (pdfinfo present, pdftotext absent):
    • error mode → READ_CONTENT_FAILURE ("pdftotext is not installed")
    • reference mode → 167-char guidance anyway ✅ — confirms the final commit "Keep large PDF references independent of pdftotext".

Tests & typecheck

  • vitest run on the 4 touched suites → 254 / 254 passed.
  • tsc --noEmit: the 5 PR source files are clean. The only diagnostics in my sandbox are pre-existing cross-worktree artifacts in providers/__tests__ + gitWorktreeService.tspresent identically on the base commit, not from this PR.
  • ESLint wasn't runnable in my sandbox (missing eslint-plugin-check-file dep); CI + the author's macOS run cover it.

Minor, non-blocking observations

  1. PDFTextResult.truncated is now unused by the consumer. It's set in pdf.ts and asserted in pdf.test.ts, but processSingleFileContent no longer reads it (the old isTruncated propagation was dropped in "Clarify PDF text truncation contract"). In practice the 100k-char truncation branch is unreachable from the read path — the 12k-token guard (~48k chars) rejects first, and the truncation notice is already inline in the returned text. Consider wiring isTruncated back or dropping the field.
  2. Reference guidance is optimistic when poppler is absent. Reference mode returns "use the pages parameter" even with no pdftotext; a follow-up paged read then fails with "pdftotext is not installed". Harmless, but the reference can't actually be acted on in that environment.
  3. Token guard runs after extractionpdftotext work is done and then discarded for oversized reads. Bounded to ≤20 pages, so acceptable.

Env: Node v22.22.2 · Linux · poppler-utils 25.03.0 · base c7fa13d6f · PR head 5a976779b. Method: tsx imports each worktree's own fileUtils.ts by absolute path (relative imports resolve to that worktree's src), spawning real pdfinfo/pdftotext.

🇨🇳 中文验证说明(点击展开)

结论:功能符合预期,可修复 #6408

我在 Linux 上做了本地真机验证(PR 的 Tested on 表格把 Linux 标为 ⚠️ 未测试)。方式是用手工构造的多页 PDF直接驱动真实的 processSingleFileContent 代码路径,并调用真实的 pdftotext / pdfinfo(poppler 25.03.0),全程无 mock;对 merge-base(改动前)PR HEAD 两个 worktree 跑完全相同的场景做 A/B 对比。

一句话#6408 的根因在 base 上可复现——一个 100 页 PDF 会把 100,100 个字符注入到工具结果里;本 PR 之后同样的读取只返回 167 个字符的引导信息。自动压缩后仍然撑爆上下文的那个超大 tool result 被消除了。

改动前 / 改动后(真实 PDF,纯文本模型,真实 pdftotext)

# 输入 → 调用 BASE(改动前) MERGED(#6409
A big100.pdf · read_file(无 pages) 100,100 字符,无错误 167 字符 + file_too_large
B big100.pdf · @ 附加 100,100 字符 167 字符 reference,无错误
C big100.pdf · pages="1-2" 2,288 字符 2,288 字符 — 保持不变 ✅
D dense1.pdf · pages="1"(单页 ≈1.45 万 token) 58,093 字符 264 字符 + file_too_large
E pages15.pdf · read_file(无 pages) 2,877 字符,无 gate 167 字符 + file_too_large
F small3.pdf · read_file(无 pages) 573 字符 573 字符 — 保持不变 ✅

各场景说明

  • A / BLarge PDF reads can overflow prompt context #6408 真实存在且已修复;直接读取变为 file_too_large 错误,@ 附加变为轻量 reference(returnDisplay: "Referenced large PDF",不报错)。
  • C / F:显式页码范围读取、小 PDF 完全不受影响。
  • D:token 预算护栏拦住了单个高密度页面(估算 14,540 token > 12,000 上限),返回简短引导。
  • E:gate 是按页数判定的(15 > 10),因此哪怕文本很少,15 页文档也会被拦。属于设计取舍,值得知悉。

边界场景(同样真实、无 mock)

  • 原生 PDF 模型modalities.pdf = true):big100.pdf → base64 inlineData,完全跳过 gate,原生行为不受影响。
  • poppler 拆分(有 pdfinfo、无 pdftotext):error 模式 → READ_CONTENT_FAILURE;reference 模式 → 仍返回 167 字符引导 ✅,印证最后一个 commit "Keep large PDF references independent of pdftotext"

测试与类型检查

  • vitest run 4 个受影响用例文件 → 254 / 254 通过
  • tsc --noEmit:本 PR 的 5 个源码文件干净。沙箱里仅有的报错来自 providers/__tests__gitWorktreeService.ts 的跨 worktree 既有问题,在 base commit 上一模一样存在,与本 PR 无关。
  • ESLint 在我的沙箱缺少 eslint-plugin-check-file 依赖无法运行;由 CI 与作者的 macOS 验证覆盖。

次要、非阻塞的观察

  1. PDFTextResult.truncated 目前不再被消费方使用(在 pdf.ts 里设置、pdf.test.ts 里断言,但 processSingleFileContent 已不再读取它)。实际上 100k 截断分支从读取路径上已不可达(12k-token 护栏 ≈48k 字符会先拦),且截断提示本就内联在返回文本中。建议要么重新接回 isTruncated,要么删掉该字段。
  2. 缺少 poppler 时 reference 引导略显“乐观”——它提示用 pages,但该环境下后续分页读取会以 "pdftotext is not installed" 失败。无害,但该引导实际无法执行。
  3. token 护栏在 pdftotext 抽取之后才判定,即超限读取会先抽取再丢弃(受 ≤20 页限制,可接受)。
📜 Raw terminal transcript (A/B + tests)
$ tsx ab_summary.mts     # imports BOTH worktrees, identical scenarios, real pdftotext/pdfinfo

PR #6409  "Gate large PDF text extraction"  —  REAL pdftotext/pdfinfo, text-only model
============================================================================================
Scenario                              │ BASE (pre-PR)           │ MERGED (PR #6409)
                                      │ llmContent chars / err  │ llmContent chars / err
────────────────────────────────────────────────────────────────────────────────────────
A  big100.pdf  read_file (no pages)   │ 100100  —               │    167  file_too_large
B  big100.pdf  @-attach (reference)   │ 100100  —               │    167  —
C  big100.pdf  read_file pages=1-2    │   2288  —               │   2288  —
D  dense1.pdf  read_file pages=1      │  58093  —               │    264  file_too_large
E  pages15.pdf read_file (no pages)   │   2877  —               │    167  file_too_large
F  small3.pdf  read_file (no pages)   │    573  —               │    573  —
============================================================================================

$ vitest run  (4 PR-touched suites, PR-head worktree)
 Test Files  4 passed (4)
      Tests  254 passed (254)

# poppler split (pdfinfo present, pdftotext absent):
#   I. error mode,     big100 read_file  -> read_content_failure ("pdftotext is not installed")
#   J. reference mode, big100 @-attach   -> 167-char guidance, no error  (independent of pdftotext)
#   K. small3 (<=10p)  read_file         -> read_content_failure (still needs pdftotext to extract)

@doudouOUC
doudouOUC added this pull request to the merge queue Jul 7, 2026
Merged via the queue into QwenLM:main with commit 1ee9780 Jul 7, 2026
27 of 28 checks passed
@doudouOUC
doudouOUC deleted the fix/pdf-read-budget-reference branch July 7, 2026 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category/core Core engine and logic category/tools Tool integration and execution scope/file-operations File system operations scope/token-management Token handling and limits type/bug Something isn't working as expected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Large PDF reads can overflow prompt context

4 participants