fix(tools): preserve PDF read-before-write state

src/tools/FileReadTool/FileReadTool.pdfWrite.test.ts

@@ -0,0 +1,181 @@
+import { afterEach, beforeEach, expect, spyOn, test } from 'bun:test'
+import { mkdtemp, readFile, rm, stat, utimes, writeFile } from 'node:fs/promises'
+import { tmpdir } from 'node:os'
+import { join } from 'node:path'
+import { getEmptyToolPermissionContext, type ToolUseContext } from '../../Tool.js'
+import { getChangedFiles } from '../../utils/attachments.js'
+import { createFileStateCacheWithSizeLimit } from '../../utils/fileStateCache.js'
+import * as pdf from '../../utils/pdf.js'
+import { FileWriteTool } from '../FileWriteTool/FileWriteTool.js'
+import { FileReadTool } from './FileReadTool.js'
+import { getImageCreator } from './imageProcessor.js'
+import { asciiPDF } from './fixtures/asciiPDF.js'
+
+const directories: string[] = []
+const originalSimple = process.env.CLAUDE_CODE_SIMPLE
+const originalCheckpoints = process.env.CLAUDE_CODE_DISABLE_FILE_CHECKPOINTING
+let pageCount: ReturnType<typeof spyOn>
+
+beforeEach(() => {
+  process.env.CLAUDE_CODE_SIMPLE = '1'
+  process.env.CLAUDE_CODE_DISABLE_FILE_CHECKPOINTING = '1'
+  // The fixture is a valid one-page PDF. Avoid depending on a system pdfinfo.
+  pageCount = spyOn(pdf, 'getPDFPageCount').mockResolvedValue(1)
+})
+
+afterEach(async () => {
+  pageCount.mockRestore()
+  for (const [name, value] of [
+    ['CLAUDE_CODE_SIMPLE', originalSimple],
+    ['CLAUDE_CODE_DISABLE_FILE_CHECKPOINTING', originalCheckpoints],
+  ] as const) {
+    if (value === undefined) delete process.env[name]
+    else process.env[name] = value
+  }
+  await Promise.all(directories.splice(0).map(path => rm(path, { recursive: true, force: true })))
+})
+
+function context(): ToolUseContext {
+  return {
+    readFileState: createFileStateCacheWithSizeLimit(100),
+    abortController: new AbortController(),
+    updateFileHistoryState: () => {},
+    getAppState: () => ({ toolPermissionContext: getEmptyToolPermissionContext() }),
+  } as unknown as ToolUseContext
+}
+
+async function fixture() {
+  const root = await mkdtemp(join(tmpdir(), 'cc-haha-pdf-write-'))
+  directories.push(root)
+  const filePath = join(root, 'report.pdf')
+  const original = asciiPDF('ORIGINAL')
+  const replacement = asciiPDF('REPLACEMENT')
+  await writeFile(filePath, original)
+  return { root, filePath, original, replacement }
+}
+
+test('full PDF Read authorizes exact ASCII PDF replacement and keeps later Reads as documents', async () => {
+  const { root, filePath, original, replacement } = await fixture()
+  const ctx = context()
+  const templatePath = join(root, 'template.txt')
+  await writeFile(templatePath, replacement)
+  const template = await FileReadTool.call({ file_path: templatePath }, ctx)
+  expect(template.data.type).toBe('text')
+  if (template.data.type !== 'text') throw new Error('Expected text template')
+  expect(template.data.file.content).toBe(replacement)
+  const input = { file_path: filePath, content: template.data.file.content }
+  expect(await FileWriteTool.validateInput(input, ctx)).toMatchObject({ result: false, errorCode: 2 })
+
+  const result = await FileReadTool.call({ file_path: filePath }, ctx)
+  expect(result.data.type).toBe('pdf')
+  expect(result.newMessages?.[0]?.message.content).toMatchObject([
+    { type: 'document', source: { data: Buffer.from(original).toString('base64') } },
+  ])
+  expect(await FileWriteTool.validateInput(input, ctx)).toEqual({ result: true })
+
+  // A cached PDF read must still send the document, not a text-range dedup stub.
+  expect((await FileReadTool.call({ file_path: filePath }, ctx)).data.type).toBe('pdf')
+  await FileWriteTool.call(input, ctx, undefined as never, { uuid: 'pdf-write-test' } as never)
+  expect(await readFile(filePath)).toEqual(Buffer.from(replacement))
+  const afterWrite = await FileReadTool.call({ file_path: filePath }, ctx)
+  expect(afterWrite.data.type).toBe('pdf')
+  if (afterWrite.data.type === 'pdf') {
+    expect(Buffer.from(afterWrite.data.file.base64, 'base64')).toEqual(Buffer.from(replacement))
+  }
+  expect((await FileReadTool.call({ file_path: filePath }, ctx)).data.type).toBe('pdf')
+  expect(await FileWriteTool.validateInput(input, context())).toMatchObject({ result: false, errorCode: 2 })
+})
+
+test('PDF authorization fits the file-state budget without retaining a binary text snapshot', async () => {
+  const { filePath, replacement } = await fixture()
+  const ctx = context()
+  // Much smaller than the PDF payload: authorization must not be evicted just
+  // because a document's base64 expansion exceeds the text-cache budget.
+  ctx.readFileState = createFileStateCacheWithSizeLimit(100, 64)
+  await FileReadTool.call({ file_path: filePath }, ctx)
+  expect(await FileWriteTool.validateInput({ file_path: filePath, content: replacement }, ctx))
+    .toEqual({ result: true })
+})
+
+test('failed PDF Read never authorizes an existing target', async () => {
+  const { filePath, replacement } = await fixture()
+  await writeFile(filePath, 'invalid PDF')
+  const ctx = context()
+  await expect(FileReadTool.call({ file_path: filePath }, ctx)).rejects.toThrow('missing %PDF- header')
+  expect(ctx.readFileState.has(filePath)).toBe(false)
+  expect(await FileWriteTool.validateInput({ file_path: filePath, content: replacement }, ctx))
+    .toMatchObject({ result: false, errorCode: 2 })
+  expect(await readFile(filePath, 'utf8')).toBe('invalid PDF')
+})
+
+test('PDF page extraction neither grants full-file Write access nor dedups against a full Read', async () => {
+  const { root, filePath, original, replacement } = await fixture()
+  const creator = await getImageCreator()
+  const jpeg = await creator({ create: {
+    width: 4, height: 3, channels: 3, background: { r: 20, g: 40, b: 60 },
+  } }).jpeg().toBuffer()
+  await writeFile(join(root, 'page-1.jpg'), jpeg)
+  const extraction = spyOn(pdf, 'extractPDFPages').mockResolvedValue({
+    success: true,
+    data: { type: 'parts', file: { filePath, originalSize: Buffer.byteLength(original), outputDir: root, count: 1 } },
+  })
+  try {
+    const ctx = context()
+    const input = { file_path: filePath, content: replacement }
+    expect((await FileReadTool.call({ file_path: filePath, pages: '1' }, ctx)).data.type).toBe('parts')
+    expect(ctx.readFileState.has(filePath)).toBe(false)
+    expect(await FileWriteTool.validateInput(input, ctx)).toMatchObject({ result: false, errorCode: 2 })
+
+    await FileReadTool.call({ file_path: filePath }, ctx)
+    expect((await FileReadTool.call({ file_path: filePath, pages: '1' }, ctx)).data.type).toBe('parts')
+    expect(extraction).toHaveBeenCalledTimes(2)
+    expect(await FileWriteTool.validateInput(input, ctx)).toEqual({ result: true })
+  } finally {
+    extraction.mockRestore()
+  }
+})
+
+test('external PDF changes after Read are rejected in validation and immediately before Write', async () => {
+  const { filePath, replacement } = await fixture()
+  const ctx = context()
+  await FileReadTool.call({ file_path: filePath }, ctx)
+  const input = { file_path: filePath, content: replacement }
+  expect(await FileWriteTool.validateInput(input, ctx)).toEqual({ result: true })
+
+  const changed = asciiPDF('EXTERNAL_CHANGE')
+  const later = new Date((await stat(filePath)).mtimeMs + 2000)
+  await writeFile(filePath, changed)
+  await utimes(filePath, later, later)
+  // Text-file change attachments must not refresh PDF authorization behind
+  // the model's back when there is no PDF diff to send.
+  expect(await getChangedFiles(ctx)).toEqual([])
+  expect(await FileWriteTool.validateInput(input, ctx)).toMatchObject({ result: false, errorCode: 3 })
+  await expect(FileWriteTool.call(input, ctx, undefined as never, { uuid: 'stale-pdf-write' } as never))
+    .rejects.toThrow('unexpectedly modified')
+  expect(await readFile(filePath, 'utf8')).toBe(changed)
+
+  await FileReadTool.call({ file_path: filePath }, ctx)
+  expect(await FileWriteTool.validateInput(input, ctx)).toEqual({ result: true })
+  await FileWriteTool.call(input, ctx, undefined as never, { uuid: 'reread-pdf-write' } as never)
+  expect(await readFile(filePath)).toEqual(Buffer.from(replacement))
+})
+
+test('a PDF changed during Read is not stamped with the later unread version', async () => {
+  const { filePath, replacement } = await fixture()
+  const ctx = context()
+  const actualRead = pdf.readPDF
+  const later = new Date((await stat(filePath)).mtimeMs + 2000)
+  const reading = spyOn(pdf, 'readPDF').mockImplementation(async path => {
+    const result = await actualRead(path)
+    await writeFile(path, asciiPDF('CHANGED_DURING_READ'))
+    await utimes(path, later, later)
+    return result
+  })
+  try {
+    expect((await FileReadTool.call({ file_path: filePath }, ctx)).data.type).toBe('pdf')
+    expect(await FileWriteTool.validateInput({ file_path: filePath, content: replacement }, ctx))
+      .toMatchObject({ result: false, errorCode: 3 })
+  } finally {
+    reading.mockRestore()
+  }
+})

src/tools/FileReadTool/FileReadTool.ts

@@ -531,8 +531,8 @@ export const FileReadTool = buildTool({
     // The earlier Read tool_result is still in context — two full copies
     // waste cache_creation tokens on every subsequent turn. BQ proxy shows
     // ~18% of Read calls are same-file collisions (up to 2.64% of fleet
-    // cache_creation). Only applies to text/notebook reads — images/PDFs
-    // aren't cached in readFileState so won't match here.
+    // cache_creation). Only applies to text/notebook reads. PDF state tracks
+    // overwrite authorization, not line ranges or model-facing text content.
     //
     // Ant soak: 1,734 dedup hits in 2h, no Read error regression.
     // Killswitch pattern: GB can disable if the stub message confuses
@@ -551,6 +551,7 @@ export const FileReadTool = buildTool({
     // entry reflects post-edit mtime, so deduping against it would wrongly
     // point the model at the pre-edit Read content.
     if (
+      !isPDFExtension(ext) &&
       existingState &&
       !existingState.isPartialView &&
       existingState.offset !== undefined
@@ -1007,6 +1008,18 @@ async function callInner(
       throw new Error(readResult.error.message)
     }
     const pdfData = readResult.data
+    // A successful full document Read satisfies Write's existing-file guard.
+    // Use the pre-read mtime: a change during Read must still require a reread.
+    // Keep this entry out of text diffing/content fallbacks (like other Read
+    // entries, offset is defined). PDFs have no comparable text snapshot; do
+    // not cache base64 payloads that can exceed the file-state cache budget.
+    // Page-range reads above deliberately do not authorize full replacement.
+    readFileState.set(fullFilePath, {
+      content: '',
+      timestamp: Math.floor(stats.mtimeMs),
+      offset: 1,
+      limit: undefined,
+    })
     logFileOperation({
       operation: 'read',
       tool: 'FileReadTool',

src/tools/FileReadTool/fixtures/asciiPDF.ts

@@ -0,0 +1,21 @@
+// Valid ASCII PDF content, with byte offsets and stream length computed so
+// Write can replace it without binary transcoding or repairing the xref.
+export function asciiPDF(label: string): string {
+  const stream = `BT /F1 20 Tf 60 760 Td (${label}) Tj ET\n`
+  const objects = [
+    '<< /Type /Catalog /Pages 2 0 R >>',
+    '<< /Type /Pages /Kids [3 0 R] /Count 1 >>',
+    '<< /Type /Page /Parent 2 0 R /MediaBox [0 0 595 842] /Resources << /Font << /F1 4 0 R >> >> /Contents 5 0 R >>',
+    '<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica >>',
+    `<< /Length ${Buffer.byteLength(stream)} >>\nstream\n${stream}endstream`,
+  ]
+  let content = '%PDF-1.4\n'
+  const offsets: number[] = []
+  for (const [index, object] of objects.entries()) {
+    offsets.push(Buffer.byteLength(content))
+    content += `${index + 1} 0 obj\n${object}\nendobj\n`
+  }
+  const xref = Buffer.byteLength(content)
+  content += `xref\n0 6\n0000000000 65535 f \n${offsets.map(offset => `${String(offset).padStart(10, '0')} 00000 n \n`).join('')}`
+  return `${content}trailer\n<< /Size 6 /Root 1 0 R >>\nstartxref\n${xref}\n%%EOF\n`
+}

src/utils/attachments.pdfWrite.test.ts

@@ -0,0 +1,86 @@
+import { afterEach, beforeEach, expect, spyOn, test } from 'bun:test'
+import { mkdtemp, readFile, rm, stat, utimes, writeFile } from 'node:fs/promises'
+import { tmpdir } from 'node:os'
+import { join } from 'node:path'
+import { getEmptyToolPermissionContext, type ToolUseContext } from '../Tool.js'
+import { FileReadTool } from '../tools/FileReadTool/FileReadTool.js'
+import { asciiPDF } from '../tools/FileReadTool/fixtures/asciiPDF.js'
+import { FileWriteTool } from '../tools/FileWriteTool/FileWriteTool.js'
+import { getChangedFiles } from './attachments.js'
+import { createFileStateCacheWithSizeLimit } from './fileStateCache.js'
+import * as pdf from './pdf.js'
+
+const directories: string[] = []
+const originalSimple = process.env.CLAUDE_CODE_SIMPLE
+const originalCheckpoints = process.env.CLAUDE_CODE_DISABLE_FILE_CHECKPOINTING
+let pageCount: ReturnType<typeof spyOn>
+
+beforeEach(() => {
+  process.env.CLAUDE_CODE_SIMPLE = '1'
+  process.env.CLAUDE_CODE_DISABLE_FILE_CHECKPOINTING = '1'
+  pageCount = spyOn(pdf, 'getPDFPageCount').mockResolvedValue(1)
+})
+
+afterEach(async () => {
+  pageCount.mockRestore()
+  for (const [name, value] of [
+    ['CLAUDE_CODE_SIMPLE', originalSimple],
+    ['CLAUDE_CODE_DISABLE_FILE_CHECKPOINTING', originalCheckpoints],
+  ] as const) {
+    if (value === undefined) delete process.env[name]
+    else process.env[name] = value
+  }
+  await Promise.all(directories.splice(0).map(path => rm(path, { recursive: true, force: true })))
+})
+
+function context(): ToolUseContext {
+  return {
+    readFileState: createFileStateCacheWithSizeLimit(100),
+    abortController: new AbortController(),
+    updateFileHistoryState: () => {},
+    getAppState: () => ({ toolPermissionContext: getEmptyToolPermissionContext() }),
+  } as unknown as ToolUseContext
+}
+
+for (const extension of ['pdf', 'PDF']) {
+  test(`automatic change checks after Write do not authorize an unseen ${extension} version`, async () => {
+    const root = await mkdtemp(join(tmpdir(), 'cc-haha-pdf-attachments-'))
+    directories.push(root)
+    const filePath = join(root, `report.${extension}`)
+    await writeFile(filePath, asciiPDF('ORIGINAL'))
+    const ctx = context()
+    await FileReadTool.call({ file_path: filePath }, ctx)
+    const input = { file_path: filePath, content: asciiPDF('REPLACEMENT') }
+    expect(await FileWriteTool.validateInput(input, ctx)).toEqual({ result: true })
+    await FileWriteTool.call(input, ctx, undefined as never, { uuid: 'pdf-attachment-write' } as never)
+
+    const changed = asciiPDF('EXTERNAL_CHANGE')
+    const later = new Date((await stat(filePath)).mtimeMs + 2000)
+    await writeFile(filePath, changed)
+    await utimes(filePath, later, later)
+    expect(await getChangedFiles(ctx)).toEqual([])
+    expect(await FileWriteTool.validateInput(input, ctx)).toMatchObject({ result: false, errorCode: 3 })
+    await expect(FileWriteTool.call(input, ctx, undefined as never, { uuid: 'unseen-pdf-write' } as never))
+      .rejects.toThrow('unexpectedly modified')
+    expect(await readFile(filePath, 'utf8')).toBe(changed)
+
+    await FileReadTool.call({ file_path: filePath }, ctx)
+    expect(await FileWriteTool.validateInput(input, ctx)).toEqual({ result: true })
+    await FileWriteTool.call(input, ctx, undefined as never, { uuid: 'reread-pdf-attachment-write' } as never)
+    expect(await readFile(filePath, 'utf8')).toBe(input.content)
+  })
+}
+
+test('automatic change checks still deliver external text changes after Write', async () => {
+  const root = await mkdtemp(join(tmpdir(), 'cc-haha-text-attachments-'))
+  directories.push(root)
+  const filePath = join(root, 'report.txt')
+  const ctx = context()
+  await FileWriteTool.call({ file_path: filePath, content: 'ORIGINAL\n' }, ctx, undefined as never, { uuid: 'text-attachment-write' } as never)
+  const later = new Date((await stat(filePath)).mtimeMs + 2000)
+  await writeFile(filePath, 'EXTERNAL_CHANGE\n')
+  await utimes(filePath, later, later)
+  const attachments = await getChangedFiles(ctx)
+  expect(attachments).toMatchObject([{ type: 'edited_text_file', filename: filePath }])
+  expect(JSON.stringify(attachments)).toContain('EXTERNAL_CHANGE')
+})

src/utils/attachments.ts

@@ -2202,6 +2202,11 @@ export async function getChangedFiles(
       const fileState = toolUseContext.readFileState.get(filePath)
       if (!fileState) return null
 
+      // PDF changes have no attachment representation below. An automatic
+      // Read would silently refresh overwrite authorization without showing
+      // the new document to the model, including after a prior Write.
+      if (isPDFExtension(parse(filePath).ext)) return null
+
       // TODO: Implement offset/limit support for changed files
       if (fileState.offset !== undefined || fileState.limit !== undefined) {
         return null