fix: reject fractional PDF page selections#99403
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 3, 2026, 3:01 AM ET / 07:01 UTC. Summary PR surface: Source +3, Tests +28. Total +31 across 3 files. Reproducibility: yes. Current main clearly parses Review metrics: 1 noteworthy metric.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land one parser-boundary integer-validation fix with helper and tool-boundary regression coverage, then close the linked issue and the unmerged sibling PR. Do we have a high-confidence way to reproduce the issue? Yes. Current main clearly parses Is this the best way to solve the issue? Yes. Rejecting fractional tokens in AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against e878efe9508e. Label changesLabel justifications:
Evidence reviewedPR surface: Source +3, Tests +28. Total +31 across 3 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
clawsweeper
Bot
added
proof: sufficient
|
The canonical fix has landed in #99399 as ae4a1e236f7e. That version enforces positive-integer tokens in the shared page-range parser, validates before media/model work, and includes both parser and real tool-boundary regression coverage. Exact-head CI passed; focused sanitized-AWS proof passed 51/51 assertions plus a nonexistent-file smoke proving fractional input fails before I/O. Closing this duplicate. Thank you @ZengWen-DT for independently identifying and testing the same bug. |
2 participants
Closes #99393
What Problem This Solves
Fixes an issue where users could pass fractional PDF page filters such as
pages: "1.5"and have the value accepted by the PDF tool parser. In extraction fallback mode, that non-integer page selection could later be filtered out before extraction, which made the request continue without the page content the user asked for instead of returning a clear input error.Authoring: AI-assisted.
Why This Change Was Made
The PDF page parser now rejects single-page tokens unless they use integer page syntax, matching the documented 1-based page-number contract and the existing integer-only range parser. The fix stays at the shared parser boundary so invalid input is rejected before native-provider checks or extraction fallback receive page numbers.
User Impact
Users now get an actionable
Invalid page number: "1.5"error for fractional PDF page filters instead of a potentially misleading PDF analysis result based on no selected extracted pages.Evidence
Before the change, the parser accepted fractional single-page tokens on current main:
After the change, the same parser inputs fail at the public input boundary:
Real PDF tool execution with
pdfModel.primary = "deepseek/deepseek-v4-pro"and a local PDF fixture now rejects the fractional page selection before model analysis:Focused validation:
Build/runtime smoke:
Live DeepSeek agent lane:
The QA manual tool inventory did not expose
pdf, so the PDF-specific behavior proof above uses the actualcreatePdfToolexecution path rather than the QA manual agent surface.