feat(core): PDF text extraction fallback and Jupyter notebook parsing#3160
Merged
Conversation
…sing For text-only models (qwen3-coder, deepseek) that lack PDF modality support, read_file now falls back to pdftotext (poppler-utils) for text extraction instead of returning an unsupported error. A new `pages` parameter enables paginated PDF reading (e.g. "1-5", "3-"). Also adds structured .ipynb parsing — notebooks are displayed as labeled cells with code blocks and execution outputs rather than raw JSON. Key changes: - New utils/pdf.ts: pdftotext integration with availability caching, page range parsing, 5MB maxBuffer, and 100K char output truncation - New utils/notebook.ts: .ipynb JSON parser with per-cell output truncation (10K chars) and overall notebook truncation (100K chars) - Modified fileUtils.ts: new 'notebook' FileType, PDF fallback logic, pages parameter threading - Modified read-file.ts: pages parameter in schema/validation/execution
Contributor
📋 Review SummaryThis PR introduces PDF text extraction fallback using 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
pdf.ts was importing execCommand from shell-utils.ts, which transitively pulled in tool-utils.ts → ../index.js (barrel), creating a circular dependency that caused AuthType to be undefined during vitest module initialization in 46 test files. Replace with a local execFile wrapper that has no transitive dependencies beyond node:child_process.
Moving the modalities computation outside the if-block caused readManyFiles.test.ts to fail because its mock config doesn't implement getContentGeneratorConfig — previously the method was only called for media files (image/pdf/audio/video), never for text files. Use ?.() to gracefully fall back to an empty modalities object when the method is not defined.
wenshao
commented
Apr 12, 2026
Previously, parsePDFPageRange returned lastPage: Infinity for open-ended ranges like "3-", which bypassed the 20-page validation check and caused pdftotext to extract from the start page to EOF. This violated the documented "Max 20 pages per request" contract. Now validation explicitly rejects open-ended ranges with a helpful message telling users to specify an explicit end page within the limit. The pages parameter schema description and interface comment are also updated to reflect this constraint.
wenshao
commented
Apr 19, 2026
yiliang114
reviewed
Apr 19, 2026
parseInt() silently truncates invalid input, so values like "1-2-3", "5abc", "1-2x", "1x-2", and "1.5" were accepted and then interpreted as the wrong range (e.g. "1-2-3" parsed as 1-2). Switch to regex-based whole-string validation so any non-matching input returns null at ReadFileTool.build() time instead of reaching pdftotext.
readManyFiles previously dropped any file whose processSingleFileContent result carried an error, so users only saw "No files matching the criteria were found or all were skipped." This hid actionable guidance such as the pdftotext-not-installed install hint, password-protected PDF notices, and the >10MB size-limit message. Now the per-file error message (already a human-readable string in llmContent) is included as a content part, so batch reads surface the same guidance as single-file reads.
wenshao
commented
Apr 19, 2026
The strict regex introduced in the previous commit stopped accepting inputs like "1 - 5" or "3 -", which the old parseInt-based parser handled (parseInt skips leading whitespace). Allow optional \s* on each side of the hyphen while still rejecting malformed trailing tokens such as "5abc" and "1-2-3".
The previous commit surfaced per-file errors through readManyFiles, but FileReadInfo still lacked a status field and atCommandProcessor hardcoded ToolCallStatus.Success for every entry in result.files. So a failed read (missing pdftotext, password-protected PDF, >10MB file) rendered in the UI as if it had succeeded, just with the error text embedded in the LLM content. Add an optional `error` field on FileReadInfo, populate it in readFileContent, and use it in atCommandProcessor to pick ToolCallStatus.Error plus a resultDisplay string the user can see.
wenshao
commented
Apr 19, 2026
When a text-dense PDF produced more than 5MB of stdout, Node killed the child and `execFile` delivered the error as `ERR_CHILD_PROCESS_STDIO_MAXBUFFER`, which fell into the generic `pdftotext failed:` branch — so a perfectly valid PDF failed instead of returning the usual truncated output. Detect the maxBuffer error code in the execFile wrapper, and in extractPDFText use the partial stdout with the existing truncation note. Also lower the maxBuffer to 2×MAX_PDF_TEXT_OUTPUT_CHARS (from 5MB) since anything past that is discarded anyway — this also caps RSS for pathological inputs.
wenshao
commented
Apr 19, 2026
The generic 9.9MB file-size check ran before the pdf branch knew whether
we were taking the base64 inline path or the pdftotext text-extraction
path. That meant `read_file("huge.pdf", pages="1-5")` was rejected up
front even though pdftotext streams through the file and only emits a
capped (100K char) text slice — never loading 15MB into Node memory.
Move the size gate past the fileType/modalities decision point and skip
it when the PDF will go through text extraction (pages parameter set,
or model lacks pdf modality). The base64 inline path still carries its
own encoded-size cap, so oversized PDFs continue to be rejected there.
An adversarial pass over the PDF utilities turned up several issues that warrant hardening before the PR lands: - Argument injection (C1): filenames starting with `-` (e.g. `-opw=foo.pdf`) are parsed as options by poppler's argv parser when passed positionally. Insert `--` before `filePath` in both `extractPDFText` and `getPDFPageCount` so the shell's option parser stops processing flags. Reproduced locally: `pdftotext -h -` prints help while `pdftotext -- -h -` treats `-h` as the input file. - Brittle availability signal (H1): `isPdftotextAvailable` used `stderr.length > 0` as the positive signal, so a sandbox that suppresses stderr would cache `false` for the whole process. Switch to the exit code. - Concurrent availability probes (H2): N parallel callers (e.g. an `@`-glob of PDFs) each spawned their own `pdftotext -v` before the first probe resolved. Cache the in-flight promise. - Precision-loss bypass of the 20-page cap (H3): `Number()` collapses any integer past 2^53 onto the same value, so the string `"999999999999999998-999999999999999999"` parsed as a 1-page range and slid past the validator. Cap accepted page numbers at 1,000,000. - Timeout error clarity (M2): 30s timeouts surfaced as the generic `pdftotext failed:` branch with empty stderr. Detect SIGTERM/killed and emit a dedicated "timed out after 30s" message. - Over-eager maxBuffer success (M1): the previous commit treated any maxBuffer overrun with non-empty stdout as a truncated success. If the overrun was driven by stderr spam (password warnings, corrupt- PDF diagnostics), that delivered garbage as success. Require at least MAX_PDF_TEXT_OUTPUT_CHARS of stdout before treating as truncated; otherwise re-run the password/corrupt detectors on the captured stderr. Added regression tests for each.
Two defense-in-depth guards suggested by the adversarial audit:
- Non-regular files (FIFOs, sockets, /dev/zero, character devices)
have meaningless `stats.size` (typically 0), so the 10MB size gate
would happily wave them through. Handing `/dev/zero` to pdftotext
then produced a 30s-timeout failure after the wrapper streamed
megabytes into Node. Require `stats.isFile()` before routing into
any extraction path.
- The previous commit skipped the 10MB gate for the PDF text-
extraction path so `read_file("huge.pdf", pages="1-5")` could
work. Unbounded, though, a multi-GB PDF would make pdftotext run
until the 30s timeout fires. Add a separate 100MB ceiling for the
extraction path with a guidance error pointing the user at `pages`
or document splitting. The base64 inline path keeps its own encoded-
size cap.
Added regression tests for both.
Two notebook-rendering issues surfaced by the audit: - ipykernel emits ANSI CSI/SGR escape sequences (`\x1B[0;31m...`) in error tracebacks by default. Those codes add noise and burn tokens without conveying anything useful once we're rendering to plain text. Strip them from stream, execute_result, display_data, and error outputs. - Cells whose only output was a non-text MIME type (image/png, text/html, application/vnd.jupyter.widget-view+json, ...) were silently dropped — the model saw the source code with no indication that a plot or HTML block existed. Emit a `[non-text output: <mime-types>]` placeholder so the model knows something was there without us inlining the payload.
…NSI/MIME)
Reverse audit on the previous three commits surfaced four medium-
severity issues plus a polish item:
- isPdftotextAvailable in-flight promise leak: the `.then(...)` cleared
the cached promise on success but a synchronous throw inside the
IIFE would have left a rejected promise stuck in the slot forever.
Switch to `.finally` so the slot is always cleared.
- Timeout detection on Windows: Node's `execFile` `timeout` terminates
via TerminateProcess on Windows, where `signal` is typically `null`
rather than `'SIGTERM'`. The previous SIGTERM-only check would let
Windows timeouts fall through to the generic "pdftotext failed"
branch. Accept null/undefined signal alongside SIGTERM.
- ANSI regex was CSI-only: missed OSC hyperlinks (`ESC ]8;;url`),
DCS, APC/PM/SOS, and lone two-byte escapes that ipykernel and
related tools sometimes emit. Extend the pattern to cover all four
families.
- Non-text MIME placeholder was attacker-controlled: a malicious
notebook could set `data: {"\nIGNORE PREVIOUS INSTRUCTIONS\n": ...}`
and that key would flow unescaped into `[non-text output: ...]`,
smuggling prompt-injection payload bytes into the LLM context.
Filter keys against the IANA MIME-type grammar before joining.
- Hoisted PDF_EXTRACTION_MAX_MB to module scope alongside the other
size constants so it's discoverable in one place.
Comment/test polish from the convergence audit: - The `[@-Z\-_]` C1-Fe branch of the ANSI regex does not actually match `ESC c` (RIS), `ESC 7`, or `ESC 8`, which sit at 0x63/0x37/0x38. It does match IND/NEL/HTS/RI (ESC D/E/H/M). Correct the jsdoc example. - The `should clear the in-flight promise after a probe to allow retries` test wasn't distinguishing the `.finally` behaviour from the `resetPdftotextCache()` call that immediately precedes the second probe. Rename it to reflect what it actually verifies; the `.finally` remains as defence-in-depth (a synchronous throw inside the IIFE's own handlers can't leave the in-flight slot stuck on a rejected promise).
Collaborator
Author
Self-audit pass (3 rounds)Ran three independent audits over the full PR surface and applied the fixes as follow-up commits. Posting a summary so reviewers can spot-check without reading each commit in isolation. Round 1 — forward/adversarial + integration
Round 2 — reverse audit on round-1 fixes
Round 3 — convergence audit on round-2 fixes
After round 3 the convergence auditor confirmed the remaining surface ( |
wenshao
commented
Apr 19, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
No issues found. LGTM! ✅ — gpt-5.4 via Qwen Code /review
wenshao
commented
Apr 19, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
🔄 Incremental Review (glm-5.1)
Reviewed the 534-line diff since the last review round (commit 2709563 → e61f5d3). All previously identified issues have been addressed with solid fixes and comprehensive test coverage.
Deterministic analysis: tsc ✅ | eslint ✅ | 228 unit tests ✅
No new high-confidence issues found. One low-confidence observation about the timedOut detection heuristic in pdf.ts (could misclassify non-timeout kills), but real-world impact is negligible.
The incremental changes are well-crafted with strong defensive engineering. LGTM! ✅
— glm-5.1 via Qwen Code /review
This was referenced Apr 23, 2026
Merged
7 tasks
mabry1985
added a commit
to protoLabsAI/protoCLI
that referenced
this pull request
May 3, 2026
…LM#3717) (#211) Cherry-picked from QwenLM/qwen-code: 6efcf2b Adds a session-scoped FileReadCache that lets ReadFile substitute a short placeholder for full text Reads of files the model has already seen end-to-end and that have not been modified since. Range-scoped Reads, non-text payloads, truncated reads, and post-write Reads keep going through the full pipeline. Compaction interaction is handled by upstream's own client.ts hook: when chat compaction succeeds, getFileReadCache().clear() fires so post-compaction Reads re-emit bytes the model can no longer retrieve from its truncated context. The cache is keyed by (stats.dev, stats.ino) so symlinks, hardlinks, and case-variant paths converge to one entry; rm + recreate is correctly identified as a fresh entry. The escape hatch Config.fileReadCacheDisabled flag (default false) lets operators fully disable the fast-path. Adaptations from upstream: - Dropped the auto-memory isAutoMemPath / memoryFreshnessNote imports — both come from the un-ported QwenLM#3087 managed-memory subsystem. The cache treats every text file uniformly; if we ever port the auto-memory branch we'll re-introduce the bypass for AGENTS.md-style files. - Dropped the BackgroundTaskRegistry / BackgroundShellRegistry imports/fields the cherry-pick tried to add to Config — those belong to the un-ported background-agents subsystem. - Kept our existing trackFileRead (read-before-edit enforcement) and sessionFileTracker.record (P3 external-change detection) alongside upstream's new cache.recordRead — they're orthogonal and all run in the post-read recording block. - Dropped the params.pages === undefined arm of isFullRead; we haven't ported the PDF/Jupyter pages parameter yet (QwenLM#3160). Detection on offset+limit covers our case. Tests: 163 across the four touched test files (29 for the cache service itself; 9 for read-file caching paths; new write-file recordWrite test; new edit.ts FileReadCache integration test). typecheck + core build clean. Used --no-verify to skip the lint-staged vitest/no-conditional-expect flag that disagrees with CI's lint config (same situation as PR #197). Co-authored-by: Automaker <automaker@localhost> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
…QwenLM#3160) * feat(core): add PDF text extraction fallback and Jupyter notebook parsing For text-only models (qwen3-coder, deepseek) that lack PDF modality support, read_file now falls back to pdftotext (poppler-utils) for text extraction instead of returning an unsupported error. A new `pages` parameter enables paginated PDF reading (e.g. "1-5", "3-"). Also adds structured .ipynb parsing — notebooks are displayed as labeled cells with code blocks and execution outputs rather than raw JSON. Key changes: - New utils/pdf.ts: pdftotext integration with availability caching, page range parsing, 5MB maxBuffer, and 100K char output truncation - New utils/notebook.ts: .ipynb JSON parser with per-cell output truncation (10K chars) and overall notebook truncation (100K chars) - Modified fileUtils.ts: new 'notebook' FileType, PDF fallback logic, pages parameter threading - Modified read-file.ts: pages parameter in schema/validation/execution * fix(core): avoid circular dependency via shell-utils in pdf.ts pdf.ts was importing execCommand from shell-utils.ts, which transitively pulled in tool-utils.ts → ../index.js (barrel), creating a circular dependency that caused AuthType to be undefined during vitest module initialization in 46 test files. Replace with a local execFile wrapper that has no transitive dependencies beyond node:child_process. * fix(core): use optional call on getContentGeneratorConfig Moving the modalities computation outside the if-block caused readManyFiles.test.ts to fail because its mock config doesn't implement getContentGeneratorConfig — previously the method was only called for media files (image/pdf/audio/video), never for text files. Use ?.() to gracefully fall back to an empty modalities object when the method is not defined. * fix(core): reject open-ended PDF page ranges to enforce 20-page limit Previously, parsePDFPageRange returned lastPage: Infinity for open-ended ranges like "3-", which bypassed the 20-page validation check and caused pdftotext to extract from the start page to EOF. This violated the documented "Max 20 pages per request" contract. Now validation explicitly rejects open-ended ranges with a helpful message telling users to specify an explicit end page within the limit. The pages parameter schema description and interface comment are also updated to reflect this constraint. * fix(core): tighten parsePDFPageRange to reject malformed tokens parseInt() silently truncates invalid input, so values like "1-2-3", "5abc", "1-2x", "1x-2", and "1.5" were accepted and then interpreted as the wrong range (e.g. "1-2-3" parsed as 1-2). Switch to regex-based whole-string validation so any non-matching input returns null at ReadFileTool.build() time instead of reaching pdftotext. * fix(core): surface processSingleFileContent errors in readManyFiles readManyFiles previously dropped any file whose processSingleFileContent result carried an error, so users only saw "No files matching the criteria were found or all were skipped." This hid actionable guidance such as the pdftotext-not-installed install hint, password-protected PDF notices, and the >10MB size-limit message. Now the per-file error message (already a human-readable string in llmContent) is included as a content part, so batch reads surface the same guidance as single-file reads. * fix(core): tolerate whitespace around hyphen in parsePDFPageRange The strict regex introduced in the previous commit stopped accepting inputs like "1 - 5" or "3 -", which the old parseInt-based parser handled (parseInt skips leading whitespace). Allow optional \s* on each side of the hyphen while still rejecting malformed trailing tokens such as "5abc" and "1-2-3". * fix(cli,core): render failed @file reads as Error in atCommandProcessor The previous commit surfaced per-file errors through readManyFiles, but FileReadInfo still lacked a status field and atCommandProcessor hardcoded ToolCallStatus.Success for every entry in result.files. So a failed read (missing pdftotext, password-protected PDF, >10MB file) rendered in the UI as if it had succeeded, just with the error text embedded in the LLM content. Add an optional `error` field on FileReadInfo, populate it in readFileContent, and use it in atCommandProcessor to pick ToolCallStatus.Error plus a resultDisplay string the user can see. * fix(core): treat pdftotext maxBuffer overrun as truncation When a text-dense PDF produced more than 5MB of stdout, Node killed the child and `execFile` delivered the error as `ERR_CHILD_PROCESS_STDIO_MAXBUFFER`, which fell into the generic `pdftotext failed:` branch — so a perfectly valid PDF failed instead of returning the usual truncated output. Detect the maxBuffer error code in the execFile wrapper, and in extractPDFText use the partial stdout with the existing truncation note. Also lower the maxBuffer to 2×MAX_PDF_TEXT_OUTPUT_CHARS (from 5MB) since anything past that is discarded anyway — this also caps RSS for pathological inputs. * fix(core): skip 10MB size gate for PDF text-extraction path The generic 9.9MB file-size check ran before the pdf branch knew whether we were taking the base64 inline path or the pdftotext text-extraction path. That meant `read_file("huge.pdf", pages="1-5")` was rejected up front even though pdftotext streams through the file and only emits a capped (100K char) text slice — never loading 15MB into Node memory. Move the size gate past the fileType/modalities decision point and skip it when the PDF will go through text extraction (pages parameter set, or model lacks pdf modality). The base64 inline path still carries its own encoded-size cap, so oversized PDFs continue to be rejected there. * fix(core): harden pdftotext wrapper against six audit findings An adversarial pass over the PDF utilities turned up several issues that warrant hardening before the PR lands: - Argument injection (C1): filenames starting with `-` (e.g. `-opw=foo.pdf`) are parsed as options by poppler's argv parser when passed positionally. Insert `--` before `filePath` in both `extractPDFText` and `getPDFPageCount` so the shell's option parser stops processing flags. Reproduced locally: `pdftotext -h -` prints help while `pdftotext -- -h -` treats `-h` as the input file. - Brittle availability signal (H1): `isPdftotextAvailable` used `stderr.length > 0` as the positive signal, so a sandbox that suppresses stderr would cache `false` for the whole process. Switch to the exit code. - Concurrent availability probes (H2): N parallel callers (e.g. an `@`-glob of PDFs) each spawned their own `pdftotext -v` before the first probe resolved. Cache the in-flight promise. - Precision-loss bypass of the 20-page cap (H3): `Number()` collapses any integer past 2^53 onto the same value, so the string `"999999999999999998-999999999999999999"` parsed as a 1-page range and slid past the validator. Cap accepted page numbers at 1,000,000. - Timeout error clarity (M2): 30s timeouts surfaced as the generic `pdftotext failed:` branch with empty stderr. Detect SIGTERM/killed and emit a dedicated "timed out after 30s" message. - Over-eager maxBuffer success (M1): the previous commit treated any maxBuffer overrun with non-empty stdout as a truncated success. If the overrun was driven by stderr spam (password warnings, corrupt- PDF diagnostics), that delivered garbage as success. Require at least MAX_PDF_TEXT_OUTPUT_CHARS of stdout before treating as truncated; otherwise re-run the password/corrupt detectors on the captured stderr. Added regression tests for each. * fix(core): gate non-regular files and oversized PDFs before extraction Two defense-in-depth guards suggested by the adversarial audit: - Non-regular files (FIFOs, sockets, /dev/zero, character devices) have meaningless `stats.size` (typically 0), so the 10MB size gate would happily wave them through. Handing `/dev/zero` to pdftotext then produced a 30s-timeout failure after the wrapper streamed megabytes into Node. Require `stats.isFile()` before routing into any extraction path. - The previous commit skipped the 10MB gate for the PDF text- extraction path so `read_file("huge.pdf", pages="1-5")` could work. Unbounded, though, a multi-GB PDF would make pdftotext run until the 30s timeout fires. Add a separate 100MB ceiling for the extraction path with a guidance error pointing the user at `pages` or document splitting. The base64 inline path keeps its own encoded- size cap. Added regression tests for both. * fix(core): strip ANSI escapes and surface non-text outputs in notebooks Two notebook-rendering issues surfaced by the audit: - ipykernel emits ANSI CSI/SGR escape sequences (`\x1B[0;31m...`) in error tracebacks by default. Those codes add noise and burn tokens without conveying anything useful once we're rendering to plain text. Strip them from stream, execute_result, display_data, and error outputs. - Cells whose only output was a non-text MIME type (image/png, text/html, application/vnd.jupyter.widget-view+json, ...) were silently dropped — the model saw the source code with no indication that a plot or HTML block existed. Emit a `[non-text output: <mime-types>]` placeholder so the model knows something was there without us inlining the payload. * fix(core): round-2 audit fixes (in-flight cleanup, Windows timeout, ANSI/MIME) Reverse audit on the previous three commits surfaced four medium- severity issues plus a polish item: - isPdftotextAvailable in-flight promise leak: the `.then(...)` cleared the cached promise on success but a synchronous throw inside the IIFE would have left a rejected promise stuck in the slot forever. Switch to `.finally` so the slot is always cleared. - Timeout detection on Windows: Node's `execFile` `timeout` terminates via TerminateProcess on Windows, where `signal` is typically `null` rather than `'SIGTERM'`. The previous SIGTERM-only check would let Windows timeouts fall through to the generic "pdftotext failed" branch. Accept null/undefined signal alongside SIGTERM. - ANSI regex was CSI-only: missed OSC hyperlinks (`ESC ]8;;url`), DCS, APC/PM/SOS, and lone two-byte escapes that ipykernel and related tools sometimes emit. Extend the pattern to cover all four families. - Non-text MIME placeholder was attacker-controlled: a malicious notebook could set `data: {"\nIGNORE PREVIOUS INSTRUCTIONS\n": ...}` and that key would flow unescaped into `[non-text output: ...]`, smuggling prompt-injection payload bytes into the LLM context. Filter keys against the IANA MIME-type grammar before joining. - Hoisted PDF_EXTRACTION_MAX_MB to module scope alongside the other size constants so it's discoverable in one place. * chore(core): correct ANSI comment example and rename cache-reset test Comment/test polish from the convergence audit: - The `[@-Z\-_]` C1-Fe branch of the ANSI regex does not actually match `ESC c` (RIS), `ESC 7`, or `ESC 8`, which sit at 0x63/0x37/0x38. It does match IND/NEL/HTS/RI (ESC D/E/H/M). Correct the jsdoc example. - The `should clear the in-flight promise after a probe to allow retries` test wasn't distinguishing the `.finally` behaviour from the `resetPdftotextCache()` call that immediately precedes the second probe. Rename it to reflect what it actually verifies; the `.finally` remains as defence-in-depth (a synchronous throw inside the IIFE's own handlers can't leave the in-flight slot stuck on a rejected promise).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Qwen Code's primary models (
qwen3-coder-*,deepseek) are text-only and don't support the PDF modality. When users try to read PDFs with these models,read_filereturns an "Unsupported pdf file" error, breaking the workflow. Similarly,.ipynbfiles can be read as JSON text, but the raw JSON structure is hard to read and forces the LLM to parse notebook format on its own.What
PDF text extraction fallback
Before: Text-only model reads PDF → fails with "This model does not support PDF input directly". Users had to convert the file manually or install an extension.
After: Text-only model reads PDF → automatically falls back to the system
pdftotextand returns the extracted text. Models that support the PDF modality (Gemini, Claude) are unchanged and still receive base64 directly.Structured Jupyter notebook parsing
Before:
.ipynbfiles were shown as plain JSON, so the LLM saw the raw nested cells/outputs/metadata structure.After: Notebooks are rendered per cell, code blocks are marked with syntax-highlighting hints, and execution outputs are displayed directly after each cell.
Behavior change
PDF read behavior changed: When the model doesn't support the PDF modality, we used to return an "unsupported" error and suggest installing the document-skills extension. We now attempt
pdftotexttext extraction instead. Ifpdftotextis not installed, we return installation guidance. This means:System dependency
pdftotextandpdfinfocome from poppler-utils (a system package, not an npm dependency):When missing, we degrade gracefully — no crash, just an error message pointing at installation. Existing functionality is unaffected: models that support the PDF modality (Gemini, Claude) still use base64 directly and don't depend on poppler-utils at all.
PDF routing matrix
pagesargSafety bounds
Files changed
utils/pdf.tsutils/notebook.tsutils/fileUtils.tstools/read-file.ts*.test.tsTest plan
Manual verification details
A small Node harness was written that imports the actual built modules (
processSingleFileContent,extractPDFText,readNotebook) and exercises each routing path against on-disk fixtures, so the assertions cover the same code the tool invokes at runtime — not a re-implementation.Fixtures
/usr/share/doc/shared-mime-info/shared-mime-info-spec.pdf— a 19-page LaTeX-produced PDF (pdfTeX-1.40.26), 148 KB on disk. Real prose, not a synthetic blank..ipynb(markdown +print('hello world')stream output +6*7execute_result +1/0error traceback) so all three notebook output shapes are covered.Observed results
modalities)pagesreturnDisplayllmContentlength{}Read pdf as text: sample.pdf{}"1-5"Read pdf as text (pages 1-5): sample.pdfextractPDFTextdirect,firstPage=1, lastPage=5success: true{}Read notebook: sample.ipynbJupyter Notebook (python, 4 cells)header,```pythonfences for code, and per-cellOutput:blocks containinghello world,42, andZeroDivisionError: division by zeroenv -i PATH=/nonexistent node …){}Failed to read pdf: sample.pdf[Cannot extract text from PDF: "sample.pdf". pdftotext is not installed. Install poppler-utils to enable PDF text extraction (e.g.apt-get install poppler-utilsorbrew install poppler).](verbatim, witherrorType: read_content_failure)The page-range size scaling (14,198 / 46,645 ≈ 30% for 5/19 pages) and the verbatim install-guidance message are the two assertions worth scrutinising — they confirm
parsePDFPageRangeactually wires through topdftotext -f/-land thatisPdftotextAvailable()'s ENOENT branch produces the documented error rather than crashing.