fix(read_file): address PR review feedback

- Use consistent camelCase in error messages (anchorLine, maxLines instead of snake_case)
- Accept both camelCase and snake_case for indentation config in NativeToolCallParser
- Continue counting total lines after hitting limit in readSlice() for accurate metadata
- Return empty content instead of throwing when offset exceeds file length
- Disable strict mode in tool schema when partial reads enabled to allow truly optional params
- Update tests to match new behavior
This commit is contained in:
Hannes Rudolph 2025-12-21 17:06:26 -07:00
parent 00e62bf8b1
commit 823bd2afac
4 changed files with 71 additions and 54 deletions

View file

@ -328,35 +328,45 @@ export class NativeToolCallParser {
entry.mode = file.mode
}
// Map indentation configuration
// Map indentation configuration (accepts both camelCase and snake_case)
if (file.indentation && typeof file.indentation === "object") {
const indent = file.indentation
const indentConfig: FileEntry["indentation"] = {}
if (indent.anchor_line !== undefined) {
const anchorLine = Number(indent.anchor_line)
// anchorLine (camelCase) or anchor_line (snake_case)
const anchorLineValue = indent.anchorLine ?? indent.anchor_line
if (anchorLineValue !== undefined) {
const anchorLine = Number(anchorLineValue)
if (!isNaN(anchorLine) && anchorLine > 0) {
indentConfig.anchorLine = anchorLine
}
}
if (indent.max_levels !== undefined) {
const maxLevels = Number(indent.max_levels)
// maxLevels (camelCase) or max_levels (snake_case)
const maxLevelsValue = indent.maxLevels ?? indent.max_levels
if (maxLevelsValue !== undefined) {
const maxLevels = Number(maxLevelsValue)
if (!isNaN(maxLevels) && maxLevels >= 0) {
indentConfig.maxLevels = maxLevels
}
}
if (indent.include_siblings !== undefined) {
indentConfig.includeSiblings = Boolean(indent.include_siblings)
// includeSiblings (camelCase) or include_siblings (snake_case)
const includeSiblingsValue = indent.includeSiblings ?? indent.include_siblings
if (includeSiblingsValue !== undefined) {
indentConfig.includeSiblings = Boolean(includeSiblingsValue)
}
if (indent.include_header !== undefined) {
indentConfig.includeHeader = Boolean(indent.include_header)
// includeHeader (camelCase) or include_header (snake_case)
const includeHeaderValue = indent.includeHeader ?? indent.include_header
if (includeHeaderValue !== undefined) {
indentConfig.includeHeader = Boolean(includeHeaderValue)
}
if (indent.max_lines !== undefined) {
const maxLines = Number(indent.max_lines)
// maxLines (camelCase) or max_lines (snake_case)
const maxLinesValue = indent.maxLines ?? indent.max_lines
if (maxLinesValue !== undefined) {
const maxLines = Number(maxLinesValue)
if (!isNaN(maxLines) && maxLines > 0) {
indentConfig.maxLines = maxLines
}

View file

@ -140,16 +140,18 @@ export function createReadFileTool(options: CreateReadFileToolOptions = {}): Ope
}
}
// When using strict mode, ALL properties must be in the required array
// Optional properties are handled by having type: ["...", "null"]
const fileRequiredProperties = partialReadsEnabled ? ["path", "offset", "mode", "indentation"] : ["path"]
// Only 'path' is truly required. Other properties are optional.
// When partialReadsEnabled is true, we disable strict mode to allow optional properties
// without requiring the model to explicitly pass null for each one.
const fileRequiredProperties = ["path"]
const useStrictMode = !partialReadsEnabled
return {
type: "function",
function: {
name: "read_file",
description,
strict: true,
strict: useStrictMode,
parameters: {
type: "object",
properties: {

View file

@ -81,10 +81,14 @@ describe("read-file-content", () => {
})
})
it("should throw error when offset exceeds file length", async () => {
it("should return empty content when offset exceeds file length", async () => {
const content = "Line 1\nLine 2"
await withTempFile("slice-past-end-test.txt", content, async (filepath) => {
await expect(readSlice(filepath, 100, 3)).rejects.toThrow("offset exceeds file length")
const result = await readSlice(filepath, 100, 3)
expect(result.content).toBe("")
expect(result.lineCount).toBe(0)
expect(result.totalLines).toBe(2)
expect(result.metadata.totalLinesInFile).toBe(2)
})
})
@ -154,7 +158,7 @@ def another():
it("should throw error for anchorLine=0", async () => {
await withTempFile("indent-zero-anchor-test.py", pythonCode, async (filepath) => {
await expect(readIndentationBlock(filepath, 1, 100, { anchorLine: 0 })).rejects.toThrow(
"anchor_line must be a 1-indexed line number",
"anchorLine must be a 1-indexed line number",
)
})
})
@ -162,7 +166,7 @@ def another():
it("should throw error when anchorLine exceeds file length", async () => {
await withTempFile("indent-past-end-test.py", pythonCode, async (filepath) => {
await expect(readIndentationBlock(filepath, 1, 100, { anchorLine: 100 })).rejects.toThrow(
"anchor_line exceeds file length",
"anchorLine exceeds file length",
)
})
})
@ -262,7 +266,10 @@ def my_function():
it("should handle empty files gracefully", async () => {
await withTempFile("mode-empty-test.txt", "", async (filepath) => {
await expect(readFileContent({ filePath: filepath })).rejects.toThrow()
const result = await readFileContent({ filePath: filepath })
expect(result.content).toBe("")
expect(result.lineCount).toBe(0)
expect(result.totalLines).toBe(0)
})
})

View file

@ -247,7 +247,8 @@ export async function readSlice(
lineNumber++
if (lineNumber >= offset && collected.length < limit) {
// Only collect content if we haven't hit the limit yet
if (!truncatedByLimit && lineNumber >= offset && collected.length < limit) {
// Track first line collected
if (startLine === 0) {
startLine = lineNumber
@ -261,33 +262,12 @@ export async function readSlice(
lineLengthTruncations.push(lineNumber)
}
collected.push(`${lineNumber} | ${display}`)
}
if (collected.length >= limit) {
truncatedByLimit = true
input.destroy()
// We don't know the total lines yet, so estimate
const totalLines = lineNumber
const linesReturned = collected.length
resolve({
content: collected.join("\n"),
lineCount: linesReturned,
totalLines,
metadata: {
filePath,
totalLinesInFile: totalLines, // Approximate - file was truncated early
linesReturned,
startLine,
endLine,
hasMoreBefore: startLine > 1,
hasMoreAfter: true, // We hit the limit, so there's likely more
linesBeforeStart: startLine - 1,
linesAfterEnd: 0, // Unknown when truncated
truncatedByLimit: true,
lineLengthTruncations,
},
})
return
// Check if we've hit the limit
if (collected.length >= limit) {
truncatedByLimit = true
// Continue counting lines instead of destroying stream
}
}
pos = nextNewline + 1
@ -301,7 +281,7 @@ export async function readSlice(
// Process any remaining data (last line without newline)
if (buffer.length > 0) {
lineNumber++
if (lineNumber >= offset && collected.length < limit) {
if (!truncatedByLimit && lineNumber >= offset && collected.length < limit) {
if (startLine === 0) {
startLine = lineNumber
}
@ -316,12 +296,30 @@ export async function readSlice(
}
}
if (lineNumber < offset) {
reject(new RangeError("offset exceeds file length"))
// Handle offset beyond EOF gracefully - return empty content instead of throwing
const totalLines = lineNumber
if (totalLines === 0 || offset > totalLines) {
resolve({
content: "",
lineCount: 0,
totalLines,
metadata: {
filePath,
totalLinesInFile: totalLines,
linesReturned: 0,
startLine: offset,
endLine: offset,
hasMoreBefore: offset > 1 && totalLines > 0,
hasMoreAfter: false,
linesBeforeStart: Math.min(offset - 1, totalLines),
linesAfterEnd: 0,
truncatedByLimit: false,
lineLengthTruncations: [],
},
})
return
}
const totalLines = lineNumber
const linesReturned = collected.length
const linesAfterEnd = endLine > 0 ? totalLines - endLine : 0
@ -365,17 +363,17 @@ export async function readIndentationBlock(
} = config
if (anchorLine === 0) {
throw new RangeError("anchor_line must be a 1-indexed line number")
throw new RangeError("anchorLine must be a 1-indexed line number")
}
if (maxLines === 0) {
throw new RangeError("max_lines must be greater than zero")
throw new RangeError("maxLines must be greater than zero")
}
// Load all lines
const records = await collectFileLines(filePath)
if (records.length === 0 || anchorLine > records.length) {
throw new RangeError("anchor_line exceeds file length")
throw new RangeError("anchorLine exceeds file length")
}
const anchorIndex = anchorLine - 1