Unverified 提交 70a02101 authored 作者: Will Chen's avatar Will Chen 提交者: GitHub

feat: add optional line range params to read_file tool (#2516)

## Summary - Add `start_line_one_indexed` and `end_line_one_indexed_inclusive` optional parameters to the `read_file` agent tool, allowing the agent to read specific line ranges from files - Update consent preview to display line range (e.g., "Read src/App.tsx (lines 10-50)") and XML builder to include `start_line`/`end_line` attributes - Add comprehensive unit tests (36 tests) with minimal mocking, using real temp files on disk ## Test plan - [x] All 36 new unit tests pass covering: schema validation, full file reads, start-only, end-only, both params, edge cases (out-of-bounds clamping, start > end, empty files, single-line files), consent preview, and XML builder - [x] All 747 existing tests pass (no regressions) - [x] Lint, formatting, and type checks pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/dyad-sh/dyad/pull/2516" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a> <!-- devin-review-badge-end --> <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Adds optional line range parameters to the read_file tool so the agent can read specific lines and reflect the range in consent preview and XML. Improves safety with clamping, schema validation, and clear errors, backed by comprehensive tests. - **New Features** - Support start_line_one_indexed and end_line_one_indexed_inclusive to read a specific line range. - Consent preview shows ranges; XML builder adds start_line/end_line attributes. - **Bug Fixes** - Escape start_line/end_line in XML and update parser to accept dyad-read tags with attributes. - Validate start <= end and fix trailing newline handling to match full-file reads. <sup>Written for commit 4c96f70ff20ba37bee46955b739a693acd0afc0c. Summary will update on new commits.</sup> <!-- End of auto-generated description by cubic. --> <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Expands an agent file-access primitive and changes how `<dyad-read>` tags are parsed, which could affect context gathering and partial reads; behavior is well-covered by new unit tests but still touches security-adjacent tooling. > > **Overview** > Adds optional `start_line_one_indexed` / `end_line_one_indexed_inclusive` parameters to the local agent `read_file` tool so it can return a specific line range (with bounds clamping and trailing-newline preservation). > > Updates consent previews and `<dyad-read>` XML generation to surface/encode the requested range, and loosens `parseFilesFromMessage`’s `<dyad-read>` regex to tolerate extra attributes. Includes a comprehensive new `read_file` unit test suite covering schema validation, range behavior, and XML/preview formatting. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 4c96f70ff20ba37bee46955b739a693acd0afc0c. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: 's avatarClaude Opus 4.6 <noreply@anthropic.com>
上级 c12e8f76
......@@ -283,6 +283,10 @@ When rebasing a branch that has drizzle migrations conflicting with upstream (e.
4. Create/update the snapshot file (`drizzle/meta/00XX_snapshot.json`) with the new index, updating `prevId` to reference the previous snapshot's `id`
5. If the PR had subsequent commits that deleted/modified its migration files, those changes become no-ops after renaming — just accept the deletion conflicts by staging the renamed files
### tsgo is stricter than tsc for type checking
The pre-commit hook runs `tsgo` (via `npm run ts`), which is stricter than `tsc --noEmit`. For example, passing a `number` to a function typed `(str: string | null | undefined)` may pass `tsc` but fail `tsgo` with `TS2345: Argument of type 'number' is not assignable to parameter of type 'string'`. Always wrap with `String()` when converting numbers to string parameters.
### OpenAI reasoning model errors with conversation history
When using OpenAI reasoning models (o1, o3, o4-mini) via LiteLLM/Azure, you may see:
......
......@@ -61,6 +61,18 @@
"path": {
"type": "string",
"description": "The file path to read"
},
"start_line_one_indexed": {
"description": "The one-indexed line number to start reading from (inclusive).",
"type": "integer",
"minimum": 1,
"maximum": 9007199254740991
},
"end_line_one_indexed_inclusive": {
"description": "The one-indexed line number to end reading at (inclusive).",
"type": "integer",
"minimum": 1,
"maximum": 9007199254740991
}
},
"required": [
......
......@@ -201,6 +201,18 @@
"path": {
"type": "string",
"description": "The file path to read"
},
"start_line_one_indexed": {
"description": "The one-indexed line number to start reading from (inclusive).",
"type": "integer",
"minimum": 1,
"maximum": 9007199254740991
},
"end_line_one_indexed_inclusive": {
"description": "The one-indexed line number to end reading at (inclusive).",
"type": "integer",
"minimum": 1,
"maximum": 9007199254740991
}
},
"required": [
......@@ -482,6 +494,7 @@
"include": [
"reasoning.encrypted_content"
],
"store": false,
"dyad_options": {
"enable_lazy_edits": true,
"enable_smart_files_context": true
......
......@@ -196,6 +196,18 @@
"path": {
"type": "string",
"description": "The file path to read"
},
"start_line_one_indexed": {
"description": "The one-indexed line number to start reading from (inclusive).",
"type": "integer",
"minimum": 1,
"maximum": 9007199254740991
},
"end_line_one_indexed_inclusive": {
"description": "The one-indexed line number to end reading at (inclusive).",
"type": "integer",
"minimum": 1,
"maximum": 9007199254740991
}
},
"required": [
......
......@@ -40,7 +40,7 @@ export function parseFilesFromMessage(content: string): string[] {
const matches: TagMatch[] = [];
// Parse <dyad-read path="$filePath"></dyad-read>
const dyadReadRegex = /<dyad-read\s+path="([^"]+)"\s*><\/dyad-read>/gs;
const dyadReadRegex = /<dyad-read\s+path="([^"]+)"[^>]*><\/dyad-read>/gs;
let match: RegExpExecArray | null;
while ((match = dyadReadRegex.exec(content)) !== null) {
const filePath = normalizePath(match[1].trim());
......
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
import fs from "node:fs";
import path from "node:path";
import os from "node:os";
import { readFileTool } from "./read_file";
import type { AgentContext } from "./types";
// Mock electron-log
vi.mock("electron-log", () => ({
default: {
scope: () => ({
log: vi.fn(),
warn: vi.fn(),
error: vi.fn(),
debug: vi.fn(),
}),
},
}));
describe("readFileTool", () => {
let testDir: string;
let mockContext: AgentContext;
const testFileContent = `line 1
line 2
line 3
line 4
line 5`;
beforeEach(async () => {
testDir = await fs.promises.mkdtemp(
path.join(os.tmpdir(), "read-file-test-"),
);
await fs.promises.writeFile(
path.join(testDir, "test.txt"),
testFileContent,
);
await fs.promises.writeFile(path.join(testDir, "empty.txt"), "");
await fs.promises.writeFile(
path.join(testDir, "single-line.txt"),
"only one line",
);
await fs.promises.writeFile(
path.join(testDir, "trailing-newline.txt"),
"line 1\nline 2\nline 3\n",
);
mockContext = {
event: {} as any,
appId: 1,
appPath: testDir,
chatId: 1,
supabaseProjectId: null,
supabaseOrganizationSlug: null,
messageId: 1,
isSharedModulesChanged: false,
isDyadPro: false,
todos: [],
dyadRequestId: "test-request",
fileEditTracker: {},
onXmlStream: vi.fn(),
onXmlComplete: vi.fn(),
requireConsent: vi.fn().mockResolvedValue(true),
appendUserMessage: vi.fn(),
onUpdateTodos: vi.fn(),
};
});
afterEach(async () => {
await fs.promises.rm(testDir, { recursive: true, force: true });
vi.clearAllMocks();
});
describe("schema validation", () => {
it("has the correct name", () => {
expect(readFileTool.name).toBe("read_file");
});
it("has defaultConsent set to always", () => {
expect(readFileTool.defaultConsent).toBe("always");
});
it("requires path", () => {
const schema = readFileTool.inputSchema;
expect(() => schema.parse({})).toThrow();
expect(() => schema.parse({ path: "foo.txt" })).not.toThrow();
});
it("accepts optional start_line_one_indexed", () => {
const schema = readFileTool.inputSchema;
expect(() =>
schema.parse({ path: "foo.txt", start_line_one_indexed: 5 }),
).not.toThrow();
});
it("accepts optional end_line_one_indexed_inclusive", () => {
const schema = readFileTool.inputSchema;
expect(() =>
schema.parse({ path: "foo.txt", end_line_one_indexed_inclusive: 10 }),
).not.toThrow();
});
it("accepts both line range params together", () => {
const schema = readFileTool.inputSchema;
expect(() =>
schema.parse({
path: "foo.txt",
start_line_one_indexed: 2,
end_line_one_indexed_inclusive: 4,
}),
).not.toThrow();
});
it("rejects start_line_one_indexed less than 1", () => {
const schema = readFileTool.inputSchema;
expect(() =>
schema.parse({ path: "foo.txt", start_line_one_indexed: 0 }),
).toThrow();
});
it("rejects end_line_one_indexed_inclusive less than 1", () => {
const schema = readFileTool.inputSchema;
expect(() =>
schema.parse({ path: "foo.txt", end_line_one_indexed_inclusive: 0 }),
).toThrow();
});
it("rejects non-integer line numbers", () => {
const schema = readFileTool.inputSchema;
expect(() =>
schema.parse({ path: "foo.txt", start_line_one_indexed: 1.5 }),
).toThrow();
});
it("rejects start_line > end_line", () => {
const schema = readFileTool.inputSchema;
expect(() =>
schema.parse({
path: "foo.txt",
start_line_one_indexed: 4,
end_line_one_indexed_inclusive: 2,
}),
).toThrow(
"start_line_one_indexed must be <= end_line_one_indexed_inclusive",
);
});
});
describe("execute - full file read", () => {
it("reads entire file when no line range is specified", async () => {
const result = await readFileTool.execute(
{ path: "test.txt" },
mockContext,
);
expect(result).toBe(testFileContent);
});
it("returns empty string for empty files", async () => {
const result = await readFileTool.execute(
{ path: "empty.txt" },
mockContext,
);
expect(result).toBe("");
});
it("throws error for non-existent file", async () => {
await expect(
readFileTool.execute({ path: "nope.txt" }, mockContext),
).rejects.toThrow("File does not exist: nope.txt");
});
});
describe("execute - start_line_one_indexed only", () => {
it("reads from start line to end of file", async () => {
const result = await readFileTool.execute(
{ path: "test.txt", start_line_one_indexed: 3 },
mockContext,
);
expect(result).toBe("line 3\nline 4\nline 5");
});
it("reads from line 1 (same as full file)", async () => {
const result = await readFileTool.execute(
{ path: "test.txt", start_line_one_indexed: 1 },
mockContext,
);
expect(result).toBe(testFileContent);
});
it("reads last line when start equals line count", async () => {
const result = await readFileTool.execute(
{ path: "test.txt", start_line_one_indexed: 5 },
mockContext,
);
expect(result).toBe("line 5");
});
it("returns empty string when start exceeds line count", async () => {
const result = await readFileTool.execute(
{ path: "test.txt", start_line_one_indexed: 100 },
mockContext,
);
expect(result).toBe("");
});
});
describe("execute - end_line_one_indexed_inclusive only", () => {
it("reads from beginning to end line", async () => {
const result = await readFileTool.execute(
{ path: "test.txt", end_line_one_indexed_inclusive: 3 },
mockContext,
);
expect(result).toBe("line 1\nline 2\nline 3");
});
it("reads first line only", async () => {
const result = await readFileTool.execute(
{ path: "test.txt", end_line_one_indexed_inclusive: 1 },
mockContext,
);
expect(result).toBe("line 1");
});
it("reads entire file when end equals line count", async () => {
const result = await readFileTool.execute(
{ path: "test.txt", end_line_one_indexed_inclusive: 5 },
mockContext,
);
expect(result).toBe(testFileContent);
});
it("clamps to file length when end exceeds line count", async () => {
const result = await readFileTool.execute(
{ path: "test.txt", end_line_one_indexed_inclusive: 100 },
mockContext,
);
expect(result).toBe(testFileContent);
});
});
describe("execute - both start and end", () => {
it("reads a middle range", async () => {
const result = await readFileTool.execute(
{
path: "test.txt",
start_line_one_indexed: 2,
end_line_one_indexed_inclusive: 4,
},
mockContext,
);
expect(result).toBe("line 2\nline 3\nline 4");
});
it("reads a single line when start equals end", async () => {
const result = await readFileTool.execute(
{
path: "test.txt",
start_line_one_indexed: 3,
end_line_one_indexed_inclusive: 3,
},
mockContext,
);
expect(result).toBe("line 3");
});
it("clamps both to valid range", async () => {
const result = await readFileTool.execute(
{
path: "test.txt",
start_line_one_indexed: 4,
end_line_one_indexed_inclusive: 100,
},
mockContext,
);
expect(result).toBe("line 4\nline 5");
});
});
describe("execute - single-line file", () => {
it("reads full single-line file", async () => {
const result = await readFileTool.execute(
{ path: "single-line.txt" },
mockContext,
);
expect(result).toBe("only one line");
});
it("reads single-line file with line range", async () => {
const result = await readFileTool.execute(
{
path: "single-line.txt",
start_line_one_indexed: 1,
end_line_one_indexed_inclusive: 1,
},
mockContext,
);
expect(result).toBe("only one line");
});
});
describe("execute - trailing newline file", () => {
it("preserves trailing newline on full read via line range", async () => {
const result = await readFileTool.execute(
{
path: "trailing-newline.txt",
start_line_one_indexed: 1,
end_line_one_indexed_inclusive: 3,
},
mockContext,
);
expect(result).toBe("line 1\nline 2\nline 3\n");
});
it("does not add trailing newline for partial range", async () => {
const result = await readFileTool.execute(
{
path: "trailing-newline.txt",
start_line_one_indexed: 1,
end_line_one_indexed_inclusive: 2,
},
mockContext,
);
expect(result).toBe("line 1\nline 2");
});
it("full file read matches line-range full read", async () => {
const fullRead = await readFileTool.execute(
{ path: "trailing-newline.txt" },
mockContext,
);
const rangeRead = await readFileTool.execute(
{
path: "trailing-newline.txt",
start_line_one_indexed: 1,
end_line_one_indexed_inclusive: 3,
},
mockContext,
);
expect(rangeRead).toBe(fullRead);
});
});
describe("getConsentPreview", () => {
it("shows path only when no line range", () => {
const preview = readFileTool.getConsentPreview?.({
path: "src/App.tsx",
});
expect(preview).toBe("Read src/App.tsx");
});
it("shows start and end when both provided", () => {
const preview = readFileTool.getConsentPreview?.({
path: "src/App.tsx",
start_line_one_indexed: 10,
end_line_one_indexed_inclusive: 50,
});
expect(preview).toBe("Read src/App.tsx (lines 10-50)");
});
it("shows start only", () => {
const preview = readFileTool.getConsentPreview?.({
path: "src/App.tsx",
start_line_one_indexed: 10,
});
expect(preview).toBe("Read src/App.tsx (from line 10)");
});
it("shows end only", () => {
const preview = readFileTool.getConsentPreview?.({
path: "src/App.tsx",
end_line_one_indexed_inclusive: 50,
});
expect(preview).toBe("Read src/App.tsx (to line 50)");
});
});
describe("buildXml", () => {
it("returns undefined when path is missing", () => {
const result = readFileTool.buildXml?.({}, false);
expect(result).toBeUndefined();
});
it("builds XML with path only", () => {
const result = readFileTool.buildXml?.({ path: "src/App.tsx" }, false);
expect(result).toBe('<dyad-read path="src/App.tsx"></dyad-read>');
});
it("includes start_line attribute when provided", () => {
const result = readFileTool.buildXml?.(
{ path: "src/App.tsx", start_line_one_indexed: 10 },
false,
);
expect(result).toBe(
'<dyad-read path="src/App.tsx" start_line="10"></dyad-read>',
);
});
it("includes end_line attribute when provided", () => {
const result = readFileTool.buildXml?.(
{ path: "src/App.tsx", end_line_one_indexed_inclusive: 50 },
false,
);
expect(result).toBe(
'<dyad-read path="src/App.tsx" end_line="50"></dyad-read>',
);
});
it("includes both line range attributes", () => {
const result = readFileTool.buildXml?.(
{
path: "src/App.tsx",
start_line_one_indexed: 10,
end_line_one_indexed_inclusive: 50,
},
false,
);
expect(result).toBe(
'<dyad-read path="src/App.tsx" start_line="10" end_line="50"></dyad-read>',
);
});
it("escapes special characters in path", () => {
const result = readFileTool.buildXml?.(
{ path: 'file "with" <special>.ts' },
false,
);
expect(result).toContain("&quot;");
expect(result).toContain("&lt;");
expect(result).toContain("&gt;");
});
});
});
......@@ -5,9 +5,41 @@ import { safeJoin } from "@/ipc/utils/path_utils";
const readFile = fs.promises.readFile;
const readFileSchema = z.object({
path: z.string().describe("The file path to read"),
});
const readFileSchema = z
.object({
path: z.string().describe("The file path to read"),
start_line_one_indexed: z
.number()
.int()
.min(1)
.optional()
.describe(
"The one-indexed line number to start reading from (inclusive).",
),
end_line_one_indexed_inclusive: z
.number()
.int()
.min(1)
.optional()
.describe("The one-indexed line number to end reading at (inclusive)."),
})
.refine(
(data) => {
if (
data.start_line_one_indexed != null &&
data.end_line_one_indexed_inclusive != null
) {
return (
data.start_line_one_indexed <= data.end_line_one_indexed_inclusive
);
}
return true;
},
{
message:
"start_line_one_indexed must be <= end_line_one_indexed_inclusive",
},
);
export const readFileTool: ToolDefinition<z.infer<typeof readFileSchema>> = {
name: "read_file",
......@@ -17,11 +49,33 @@ export const readFileTool: ToolDefinition<z.infer<typeof readFileSchema>> = {
inputSchema: readFileSchema,
defaultConsent: "always",
getConsentPreview: (args) => `Read ${args.path}`,
getConsentPreview: (args) => {
const start = args.start_line_one_indexed;
const end = args.end_line_one_indexed_inclusive;
if (start != null && end != null) {
return `Read ${args.path} (lines ${start}-${end})`;
} else if (start != null) {
return `Read ${args.path} (from line ${start})`;
} else if (end != null) {
return `Read ${args.path} (to line ${end})`;
}
return `Read ${args.path}`;
},
buildXml: (args, _isComplete) => {
if (!args.path) return undefined;
return `<dyad-read path="${escapeXmlAttr(args.path)}"></dyad-read>`;
const attrs = [`path="${escapeXmlAttr(args.path)}"`];
if (args.start_line_one_indexed != null) {
attrs.push(
`start_line="${escapeXmlAttr(String(args.start_line_one_indexed))}"`,
);
}
if (args.end_line_one_indexed_inclusive != null) {
attrs.push(
`end_line="${escapeXmlAttr(String(args.end_line_one_indexed_inclusive))}"`,
);
}
return `<dyad-read ${attrs.join(" ")}></dyad-read>`;
},
execute: async (args, ctx: AgentContext) => {
......@@ -32,6 +86,24 @@ export const readFileTool: ToolDefinition<z.infer<typeof readFileSchema>> = {
}
const content = await readFile(fullFilePath, "utf8");
return content || "";
if (!content) return "";
const start = args.start_line_one_indexed;
const end = args.end_line_one_indexed_inclusive;
if (start == null && end == null) {
return content;
}
const hasTrailingNewline = content.endsWith("\n");
const lines = (hasTrailingNewline ? content.slice(0, -1) : content).split(
"\n",
);
const startIdx = Math.max(0, (start ?? 1) - 1);
const endIdx = Math.min(lines.length, end ?? lines.length);
const result = lines.slice(startIdx, endIdx).join("\n");
return endIdx >= lines.length && hasTrailingNewline
? result + "\n"
: result;
},
};
Markdown 格式
0%
您添加了 0 到此讨论。请谨慎行事。
请先完成此评论的编辑!
注册 或者 后发表评论