fix(core): treat empty 'pages' parameter as unset in ReadFile#3559
Merged
Conversation
params.pages !== undefined let "" fall through to parsePDFPageRange(''),
which returns null and surfaced "Invalid pages parameter: ''" for every
read_file call from models that default optional strings to "".
Switch to a truthy check so "" behaves the same as an omitted field, and
add a regression test.
Fixes #3558
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. |
Collaborator
|
@copilot pls review |
Contributor
The PR looks good. The change is minimal and correct:
All 38 tests pass ( |
wenshao
approved these changes
Apr 23, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅ — gpt-5.4 via Qwen Code /review
TaimoorSiddiquiOfficial
pushed a commit
to TaimoorSiddiquiOfficial/HopCode
that referenced
this pull request
Apr 23, 2026
…#3559) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
chiga0
pushed a commit
that referenced
this pull request
Apr 24, 2026
params.pages !== undefined let "" fall through to parsePDFPageRange(''),
which returns null and surfaced "Invalid pages parameter: ''" for every
read_file call from models that default optional strings to "".
Switch to a truthy check so "" behaves the same as an omitted field, and
add a regression test.
Fixes #3558
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
…#3559) params.pages !== undefined let "" fall through to parsePDFPageRange(''), which returns null and surfaced "Invalid pages parameter: ''" for every read_file call from models that default optional strings to "". Switch to a truthy check so "" behaves the same as an omitted field, and add a regression test. Fixes QwenLM#3558
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
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.
Summary
params.pages !== undefinedlet""fall through toparsePDFPageRange(''), which returnsnulland surfacedInvalid pages parameter: ''for everyread_filecall from models that default optional strings to"".""behaves the same as an omitted field.pages: ''.Fixes #3558
Origin and why this slipped through
The
pagesparameter was introduced in #3160 (feat(core): PDF text extraction fallback and Jupyter notebook parsing, merged 2026-04-20). That PR added, inpackages/core/src/tools/read-file.ts:!== undefinedis the natural guard if you think about the parameter in JS/TS terms — the caller is code and would only pass the field when meaningful. But the caller here is an LLM emitting JSON against a tool schema, and two realities from that side weren't accounted for:Some models habitually populate every declared string field with
""rather than omitting optionals. I reproduced this end-to-end on the released 0.15.0 build across 9 models in~/.qwen/settings.json. With a prompt that nudges the model to fill all parameters:pages: ""?Invalid pages parameter?null(schema blocks first)limit: 0firstoffset: ""firstEven without the explicit "fill all params" nudge, qwen3.6-plus and pai/glm-5 do this under slightly longer conversations — e.g. when
/reviewsubagents read many diff files in sequence.The retry-loop detector amplifies, not suppresses, the symptom. Once the tool returns
Invalid pages parameter: '', the model retries with the same call shape (it doesn't introspect the error to drop the field). After a few retries we hitRETRY LOOP DETECTED, which is what users actually see — the original validation message is buried underneath.So the fix is to make the validation tolerant of the common
""shape rather than ask every consumer (every model, every subagent system prompt) to know that""is forbidden for this specific field.Test plan
npx vitest run packages/core/src/tools/read-file.test.ts— 38 passedshould treat empty pages parameter as unsetpassesqwen -m qwen3.6-plus ...) confirms the error surface before the fix