diff --git a/src/tools/FileReadTool/FileReadTool.pdfWrite.test.ts b/src/tools/FileReadTool/FileReadTool.pdfWrite.test.ts new file mode 100644 index 00000000..5c5b91ce --- /dev/null +++ b/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 + +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() + } +}) diff --git a/src/tools/FileReadTool/FileReadTool.ts b/src/tools/FileReadTool/FileReadTool.ts index 9fcf6775..4450ab55 100644 --- a/src/tools/FileReadTool/FileReadTool.ts +++ b/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', diff --git a/src/tools/FileReadTool/fixtures/asciiPDF.ts b/src/tools/FileReadTool/fixtures/asciiPDF.ts new file mode 100644 index 00000000..21fbefca --- /dev/null +++ b/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` +} diff --git a/src/utils/attachments.pdfWrite.test.ts b/src/utils/attachments.pdfWrite.test.ts new file mode 100644 index 00000000..a83e8e48 --- /dev/null +++ b/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 + +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') +}) diff --git a/src/utils/attachments.ts b/src/utils/attachments.ts index f8eb093b..9f9b4397 100644 --- a/src/utils/attachments.ts +++ b/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