From c15c465b0be215e4fa6e2603828c95db78976f38 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E7=A8=8B=E5=BA=8F=E5=91=98=E9=98=BF=E6=B1=9F=28Relakkes?=
 =?UTF-8?q?=29?= <relakkes@gmail.com>
Date: Thu, 2 Jul 2026 21:00:07 +0800
Subject: [PATCH] fix: ignore Read pages for non-PDF files (#928)

Treat pages as PDF-only during validation and execution so model-supplied pages on images or text files do not loop on invalid parameter errors.

Tested:
- bun test src/tools/FileReadTool/FileReadTool.test.ts
- bun run check:server

Confidence: high
Scope-risk: narrow
---
 src/tools/FileReadTool/FileReadTool.test.ts | 67 +++++++++++++++++++++
 src/tools/FileReadTool/FileReadTool.ts      | 20 +++---
 2 files changed, 79 insertions(+), 8 deletions(-)
 create mode 100644 src/tools/FileReadTool/FileReadTool.test.ts

diff --git a/src/tools/FileReadTool/FileReadTool.test.ts b/src/tools/FileReadTool/FileReadTool.test.ts
new file mode 100644
index 00000000..6d62e8ee
--- /dev/null
+++ b/src/tools/FileReadTool/FileReadTool.test.ts
@@ -0,0 +1,67 @@
+import { describe, expect, test } from 'bun:test'
+import { PDF_MAX_PAGES_PER_READ } from '../../constants/apiLimits.js'
+import type { ToolUseContext } from '../../Tool.js'
+import { getEmptyToolPermissionContext } from '../../Tool.js'
+import { FileReadTool } from './FileReadTool.js'
+
+function makeToolUseContext(): ToolUseContext {
+  return {
+    getAppState: () => ({
+      toolPermissionContext: getEmptyToolPermissionContext(),
+    }),
+  } as unknown as ToolUseContext
+}
+
+describe('FileReadTool pages validation', () => {
+  test('ignores invalid PDF pages values for non-PDF files', async () => {
+    const context = makeToolUseContext()
+
+    await expect(
+      FileReadTool.validateInput(
+        { file_path: '/tmp/screenshot.png', pages: '0' },
+        context,
+      ),
+    ).resolves.toEqual({ result: true })
+
+    await expect(
+      FileReadTool.validateInput(
+        { file_path: '/tmp/example.ts', pages: '' },
+        context,
+      ),
+    ).resolves.toEqual({ result: true })
+
+    await expect(
+      FileReadTool.validateInput(
+        { file_path: 'C:\\tmp\\SCREENSHOT.PNG', pages: '0' },
+        context,
+      ),
+    ).resolves.toEqual({ result: true })
+  })
+
+  test('keeps PDF pages validation strict', async () => {
+    const context = makeToolUseContext()
+
+    await expect(
+      FileReadTool.validateInput(
+        { file_path: '/tmp/document.pdf', pages: '0' },
+        context,
+      ),
+    ).resolves.toMatchObject({
+      result: false,
+      errorCode: 7,
+    })
+
+    await expect(
+      FileReadTool.validateInput(
+        {
+          file_path: '/tmp/document.pdf',
+          pages: `1-${PDF_MAX_PAGES_PER_READ + 1}`,
+        },
+        context,
+      ),
+    ).resolves.toMatchObject({
+      result: false,
+      errorCode: 8,
+    })
+  })
+})
diff --git a/src/tools/FileReadTool/FileReadTool.ts b/src/tools/FileReadTool/FileReadTool.ts
index 37b31f7d..9befb67d 100644
--- a/src/tools/FileReadTool/FileReadTool.ts
+++ b/src/tools/FileReadTool/FileReadTool.ts
@@ -417,8 +417,15 @@ export const FileReadTool = buildTool({
   },
   renderToolUseErrorMessage,
   async validateInput({ file_path, pages }, toolUseContext: ToolUseContext) {
-    // Validate pages parameter (pure string parsing, no I/O)
-    if (pages !== undefined) {
+    // Path expansion + extension checks are string-only and avoid I/O before
+    // permission evaluation.
+    const fullFilePath = expandPath(file_path)
+    const ext = path.extname(fullFilePath).toLowerCase()
+
+    // Validate pages parameter only for PDF files. Models sometimes send
+    // PDF-only pages values when reading images or text files; those should
+    // not block the read path.
+    if (pages !== undefined && isPDFExtension(ext)) {
       const parsed = parsePDFPageRange(pages)
       if (!parsed) {
         return {
@@ -440,9 +447,6 @@ export const FileReadTool = buildTool({
       }
     }
 
-    // Path expansion + deny rule check (no I/O)
-    const fullFilePath = expandPath(file_path)
-
     const appState = toolUseContext.getAppState()
     const denyRule = matchingRuleForInput(
       fullFilePath,
@@ -469,7 +473,6 @@ export const FileReadTool = buildTool({
 
     // Binary extension check (string check on extension only, no I/O).
     // PDF, images, and SVG are excluded - this tool renders them natively.
-    const ext = path.extname(fullFilePath).toLowerCase()
     if (
       hasBinaryExtension(fullFilePath) &&
       !isPDFExtension(ext) &&
@@ -517,6 +520,7 @@ export const FileReadTool = buildTool({
     }
 
     const ext = path.extname(file_path).toLowerCase().slice(1)
+    const effectivePages = isPDFExtension(ext) ? pages : undefined
     // Use expandPath for consistent path normalization with FileEditTool/FileWriteTool
     // (especially handles whitespace trimming and Windows path separators)
     const fullFilePath = expandPath(file_path)
@@ -599,7 +603,7 @@ export const FileReadTool = buildTool({
         ext,
         offset,
         limit,
-        pages,
+        effectivePages,
         maxSizeBytes,
         maxTokens,
         readFileState,
@@ -622,7 +626,7 @@ export const FileReadTool = buildTool({
               ext,
               offset,
               limit,
-              pages,
+              effectivePages,
               maxSizeBytes,
               maxTokens,
               readFileState,