mirror of
https://github.com/badlogic/pi-mono.git
synced 2026-11-05 23:37:25 +00:00
fix(coding-agent): find and ls limit edge cases (#11747)
Some checks failed
CI / build-check (push) Waiting to run
CI / test (coding-agent) (push) Waiting to run
CI / test (durable) (push) Waiting to run
CI / test (other) (push) Waiting to run
CI / mcp-conformance (push) Waiting to run
Environments / daemons (push) Has been cancelled
Environments / tests (macos-latest) (push) Has been cancelled
Environments / tests (ubuntu-24.04-arm) (push) Has been cancelled
Environments / tests (ubuntu-latest) (push) Has been cancelled
Environments / tests (windows-11-arm) (push) Has been cancelled
Environments / tests (windows-latest) (push) Has been cancelled
Environments / check (push) Has been cancelled
Environments / release-daemons (darwin-arm64, macos-latest) (push) Has been cancelled
Environments / release-daemons (darwin-x64, macos-latest) (push) Has been cancelled
Environments / release-daemons (linux-arm64, ubuntu-24.04-arm) (push) Has been cancelled
Environments / release-daemons (linux-x64, ubuntu-latest) (push) Has been cancelled
Environments / release-daemons (windows-arm64, windows-11-arm) (push) Has been cancelled
Environments / release-daemons (windows-x64, windows-latest) (push) Has been cancelled
Some checks failed
CI / build-check (push) Waiting to run
CI / test (coding-agent) (push) Waiting to run
CI / test (durable) (push) Waiting to run
CI / test (other) (push) Waiting to run
CI / mcp-conformance (push) Waiting to run
Environments / daemons (push) Has been cancelled
Environments / tests (macos-latest) (push) Has been cancelled
Environments / tests (ubuntu-24.04-arm) (push) Has been cancelled
Environments / tests (ubuntu-latest) (push) Has been cancelled
Environments / tests (windows-11-arm) (push) Has been cancelled
Environments / tests (windows-latest) (push) Has been cancelled
Environments / check (push) Has been cancelled
Environments / release-daemons (darwin-arm64, macos-latest) (push) Has been cancelled
Environments / release-daemons (darwin-x64, macos-latest) (push) Has been cancelled
Environments / release-daemons (linux-arm64, ubuntu-24.04-arm) (push) Has been cancelled
Environments / release-daemons (linux-x64, ubuntu-latest) (push) Has been cancelled
Environments / release-daemons (windows-arm64, windows-11-arm) (push) Has been cancelled
Environments / release-daemons (windows-x64, windows-latest) (push) Has been cancelled
Three limit problems in the find and ls tools.
**1. `limit: 0` and other values below 1.** find passed `limit` to fd
unchanged. fd treats `--max-results 0` as no limit, so `find({ pattern:
"*", limit: 0 })` returned every match, followed by `[0 results limit
reached. Use limit=0 for more, or refine pattern]`. ls with `limit: 0`
listed nothing and printed `(empty directory)`. A fractional limit such
as `1.5` printed `1.5 results limit reached. Use limit=3`.
Fix: both tools use `Math.max(1, Math.floor(limit ?? DEFAULT_LIMIT))`.
grep already clamps to at least 1. The fd path and the custom `glob`
path share this value.
**2. Notice at exactly N results (#7121).** With 3 matching files and
`limit: 3`, fd returned 3 results. find checked `results >= limit` and
printed `[3 results limit reached. Use limit=6 for more, or refine
pattern]`, even though there were no more files.
Fix: find asks fd for `limit + 1` results, shows the first `limit`, and
prints the notice only when the extra result exists. ls already worked
this way. Custom `FindOperations.glob` now also gets `limit + 1`. The
tool shows at most `limit` of the returned results and prints the notice
only when more were returned. The interface is unchanged. This follows
closed PR #7122 without its unrelated changes.
**3. Results past the 50KB cap.** find kept every fd line in memory.
`truncateHead` then dropped everything past 50KB. find now handles each
line as it arrives. It keeps lines up to the first one past the cap and
only counts the rest, as grep does in #11705. Because that first line
past the cap is kept, `truncateHead` makes the same truncation decision
as before. `details.truncation.totalLines` and `totalBytes` come from
the counts, so the notices and `details` are unchanged.
Behavior differences:
- A custom `glob` that ignores `options.limit` and returns more results
used to have all of them shown. Now the tool shows the first `limit`
results with the limit notice.
- Custom `glob` implementations get `limit + 1` instead of `limit`.
- In the fd path, output made only of blank lines with exit code 0 now
gives `No files found matching pattern` instead of empty text. fd does
not produce such output.
**Testing**
- New tests in `test/tools.test.ts`:
- find: exactly N matches gives no notice, and N+1 matches give the
notice with N lines (#7121). Fails on main.
- find: `limit` 0, -5, 0.5 and 1.9 each show 1 result with the `1
results` notice. Fails on main.
- find with custom `glob`: the operation gets `limit + 1`, and exact and
over-limit results give the right output. Fails on main.
- find past the byte cap: 300 names of 200 bytes each.
`details.truncation` equals `truncateHead` of the full output, except
for `content`. Passes on main too; it guards the counting. It fails if
the `totalLines` override is removed.
- ls: `limit` 0, -5, 0.5 and 1.9 each list 1 entry with the notice.
Fails on main.
- ls: exactly N entries gives no notice. Passes on main too; it guards
the existing behavior.
- `test/tools.test.ts` and the find regression suites 3302, 3303 and
6104 pass (110 tests). `npm run check` passes.
fixes #7121
This PR is AI-generated.
This commit is contained in:
parent
8da141c2ad
commit
676af1d777
4 changed files with 149 additions and 27 deletions
|
|
@ -19,6 +19,7 @@
|
|||
- Fixed changelog notes repeating on every fresh launch when the installed package version has a prerelease suffix, such as `0.1.5-20261020.7`: the saved version was read as `0.1.0`, so the `0.1.1` to `0.1.5` notes counted as new each time. A saved version without `major.minor.patch` now shows no notes and is replaced by the installed version, instead of showing the whole changelog ([#11703](https://github.com/earendil-works/pi/issues/11703))
|
||||
- Fixed RPC mode cutting off shutdown cleanup, such as a pending MCP token refresh, when stdin closed and SIGTERM arrived together, which process managers like Paseo and systemd do. Pi lost the refreshed token and the MCP server revoked the sign-in. Once a shutdown has started, a stdin EOF or a single signal no longer interrupts it; a second signal still exits at once. Print mode likewise no longer dies from a signal that arrives during its final cleanup ([#11599](https://github.com/earendil-works/pi/issues/11599))
|
||||
- Fixed the grep tool reading every matched file into memory when `context` was set, which ran out of memory on large files; context now comes from ripgrep, which also fixes UTF-16 files and lone CRs in matched lines, and the match limit notice no longer appears when exactly `limit` matches exist ([#9276](https://github.com/earendil-works/pi/issues/9276), [#11423](https://github.com/earendil-works/pi/issues/11423))
|
||||
- Fixed the find tool reporting the result limit when exactly `limit` files match, and `limit: 0` in find and ls returning every result or an empty listing; limits below 1 now mean 1 ([#7121](https://github.com/earendil-works/pi/issues/7121))
|
||||
|
||||
|
||||
## [1.5.0] - 2026-11-02
|
||||
|
|
|
|||
|
|
@ -108,7 +108,8 @@ export function createFindToolDefinition(
|
|||
(async () => {
|
||||
try {
|
||||
const searchPath = resolveToCwd(searchDir || ".", ctx?.cwd || cwd);
|
||||
const effectiveLimit = limit ?? DEFAULT_LIMIT;
|
||||
// fd treats --max-results 0 as unlimited, so a limit below 1 means 1.
|
||||
const effectiveLimit = Math.max(1, Math.floor(limit ?? DEFAULT_LIMIT));
|
||||
const ops = customOps ?? defaultFindOperations;
|
||||
|
||||
// If custom operations provide glob(), use that instead of fd.
|
||||
|
|
@ -121,9 +122,10 @@ export function createFindToolDefinition(
|
|||
settle(() => reject(new Error("Operation aborted")));
|
||||
return;
|
||||
}
|
||||
// One result past the limit tells "exactly N matches" apart from "more than N".
|
||||
const results = await ops.glob(pattern, searchPath, {
|
||||
ignore: ["**/node_modules/**", "**/.git/**"],
|
||||
limit: effectiveLimit,
|
||||
limit: effectiveLimit + 1,
|
||||
});
|
||||
if (signal?.aborted) {
|
||||
settle(() => reject(new Error("Operation aborted")));
|
||||
|
|
@ -139,9 +141,11 @@ export function createFindToolDefinition(
|
|||
return;
|
||||
}
|
||||
|
||||
const resultLimitReached = results.length > effectiveLimit;
|
||||
// Relativize paths against the search root for stable output.
|
||||
const relativized = results.map((p) => relativizeFindResultPath(p, searchPath));
|
||||
const resultLimitReached = relativized.length >= effectiveLimit;
|
||||
const relativized = results
|
||||
.slice(0, effectiveLimit)
|
||||
.map((p) => relativizeFindResultPath(p, searchPath));
|
||||
const rawOutput = relativized.join("\n");
|
||||
const truncation = truncateHead(rawOutput, { maxLines: Number.MAX_SAFE_INTEGER });
|
||||
let resultOutput = truncation.content;
|
||||
|
|
@ -195,7 +199,8 @@ export function createFindToolDefinition(
|
|||
current = parent;
|
||||
}
|
||||
if (!insideGitRepo) args.push("--no-require-git");
|
||||
args.push("--max-results", String(effectiveLimit));
|
||||
// One result past the limit tells "exactly N matches" apart from "more than N" (#7121).
|
||||
args.push("--max-results", String(effectiveLimit + 1));
|
||||
|
||||
// fd --glob matches against the basename unless --full-path is set; in --full-path
|
||||
// mode it matches against the absolute candidate path, so a path-containing
|
||||
|
|
@ -215,7 +220,13 @@ export function createFindToolDefinition(
|
|||
const child = spawn(fdPath, args, { stdio: ["ignore", "pipe", "pipe"] });
|
||||
const rl = createInterface({ input: child.stdout });
|
||||
let stderr = "";
|
||||
const lines: string[] = [];
|
||||
// Results within the limit, stored only up to the first one past the byte cap:
|
||||
// truncateHead drops the rest anyway, so they are counted but not kept.
|
||||
const storedResults: string[] = [];
|
||||
let resultCount = 0;
|
||||
// Byte size of all results within the limit, each counted with a trailing newline.
|
||||
let outputBytes = 0;
|
||||
let resultLimitReached = false;
|
||||
|
||||
stopChild = () => {
|
||||
if (!child.killed) {
|
||||
|
|
@ -231,8 +242,17 @@ export function createFindToolDefinition(
|
|||
stderr += chunk.toString();
|
||||
});
|
||||
|
||||
rl.on("line", (line) => {
|
||||
lines.push(line);
|
||||
rl.on("line", (rawLine) => {
|
||||
const line = rawLine.replace(/\r$/, "").trim();
|
||||
if (!line) return;
|
||||
if (resultCount === effectiveLimit) {
|
||||
resultLimitReached = true;
|
||||
return;
|
||||
}
|
||||
const result = relativizeFindResultPath(line, searchPath);
|
||||
resultCount++;
|
||||
if (outputBytes - 1 <= DEFAULT_MAX_BYTES) storedResults.push(result);
|
||||
outputBytes += Buffer.byteLength(result, "utf-8") + 1;
|
||||
});
|
||||
|
||||
child.on("error", (error) => {
|
||||
|
|
@ -246,15 +266,12 @@ export function createFindToolDefinition(
|
|||
settle(() => reject(new Error("Operation aborted")));
|
||||
return;
|
||||
}
|
||||
const output = lines.join("\n");
|
||||
if (code !== 0) {
|
||||
if (code !== 0 && resultCount === 0) {
|
||||
const errorMsg = stderr.trim() || `fd exited with code ${code}`;
|
||||
if (!output) {
|
||||
settle(() => reject(new Error(errorMsg)));
|
||||
return;
|
||||
}
|
||||
settle(() => reject(new Error(errorMsg)));
|
||||
return;
|
||||
}
|
||||
if (!output) {
|
||||
if (resultCount === 0) {
|
||||
settle(() =>
|
||||
resolve({
|
||||
content: [{ type: "text", text: "No files found matching pattern" }],
|
||||
|
|
@ -264,16 +281,13 @@ export function createFindToolDefinition(
|
|||
return;
|
||||
}
|
||||
|
||||
const relativized: string[] = [];
|
||||
for (const rawLine of lines) {
|
||||
const line = rawLine.replace(/\r$/, "").trim();
|
||||
if (!line) continue;
|
||||
relativized.push(relativizeFindResultPath(line, searchPath));
|
||||
}
|
||||
|
||||
const resultLimitReached = relativized.length >= effectiveLimit;
|
||||
const rawOutput = relativized.join("\n");
|
||||
const truncation = truncateHead(rawOutput, { maxLines: Number.MAX_SAFE_INTEGER });
|
||||
// Stored results extend past the byte cap when more exist, so truncateHead decides
|
||||
// truncation as for the full output; only the totals need the unstored rest.
|
||||
const truncation = {
|
||||
...truncateHead(storedResults.join("\n"), { maxLines: Number.MAX_SAFE_INTEGER }),
|
||||
totalLines: resultCount,
|
||||
totalBytes: outputBytes - 1,
|
||||
};
|
||||
let resultOutput = truncation.content;
|
||||
const details: FindToolDetails = {};
|
||||
const notices: string[] = [];
|
||||
|
|
|
|||
|
|
@ -80,7 +80,8 @@ export function createLsToolDefinition(
|
|||
(async () => {
|
||||
try {
|
||||
const dirPath = resolveToCwd(path || ".", ctx?.cwd || cwd);
|
||||
const effectiveLimit = limit ?? DEFAULT_LIMIT;
|
||||
// A limit of 0 would list nothing and report "(empty directory)", so it means 1.
|
||||
const effectiveLimit = Math.max(1, Math.floor(limit ?? DEFAULT_LIMIT));
|
||||
|
||||
// Check if path exists.
|
||||
if (!(await ops.exists(dirPath))) {
|
||||
|
|
|
|||
|
|
@ -13,10 +13,11 @@ import {
|
|||
} from "../src/core/tools/bash.ts";
|
||||
import { createEditToolDefinition } from "../src/core/tools/edit.ts";
|
||||
import { computeEditsDiff } from "../src/core/tools/edit-diff.ts";
|
||||
import { createFindToolDefinition } from "../src/core/tools/find.ts";
|
||||
import { createFindToolDefinition, type FindOperations } from "../src/core/tools/find.ts";
|
||||
import { createGrepToolDefinition } from "../src/core/tools/grep.ts";
|
||||
import { createLsToolDefinition } from "../src/core/tools/ls.ts";
|
||||
import { createReadToolDefinition } from "../src/core/tools/read.ts";
|
||||
import { truncateHead } from "../src/core/tools/truncate.ts";
|
||||
import { createWriteToolDefinition } from "../src/core/tools/write.ts";
|
||||
import {
|
||||
createEditTool,
|
||||
|
|
@ -1163,6 +1164,93 @@ describe("Coding Agent Tools", () => {
|
|||
|
||||
expect(getTextOutput(result)).toContain("No files found matching pattern");
|
||||
});
|
||||
|
||||
// #7121: exactly `limit` matches must not report that the limit was reached.
|
||||
it("should report the result limit only when more results exist", async () => {
|
||||
for (const name of ["a.txt", "b.txt", "c.txt"]) writeFileSync(join(testDir, name), name);
|
||||
|
||||
const exact = await findTool.execute("test-call-find-exact-limit", {
|
||||
pattern: "*.txt",
|
||||
path: testDir,
|
||||
limit: 3,
|
||||
});
|
||||
expect(getTextOutput(exact).split("\n").sort()).toEqual(["a.txt", "b.txt", "c.txt"]);
|
||||
expect(exact.details).toBeUndefined();
|
||||
|
||||
const over = await findTool.execute("test-call-find-over-limit", {
|
||||
pattern: "*.txt",
|
||||
path: testDir,
|
||||
limit: 2,
|
||||
});
|
||||
const [listing, notice] = getTextOutput(over).split("\n\n");
|
||||
expect(listing.split("\n")).toHaveLength(2);
|
||||
expect(notice).toBe("[2 results limit reached. Use limit=4 for more, or refine pattern]");
|
||||
expect(over.details).toEqual({ resultLimitReached: 2 });
|
||||
});
|
||||
|
||||
it("should treat limits below 1 as 1 and floor fractional limits", async () => {
|
||||
for (const name of ["a.txt", "b.txt", "c.txt"]) writeFileSync(join(testDir, name), name);
|
||||
|
||||
for (const limit of [0, -5, 0.5, 1.9]) {
|
||||
const result = await findTool.execute("test-call-find-small-limit", {
|
||||
pattern: "*.txt",
|
||||
path: testDir,
|
||||
limit,
|
||||
});
|
||||
const [listing, notice] = getTextOutput(result).split("\n\n");
|
||||
expect(listing.split("\n")).toHaveLength(1);
|
||||
expect(notice).toBe("[1 results limit reached. Use limit=2 for more, or refine pattern]");
|
||||
expect(result.details).toEqual({ resultLimitReached: 1 });
|
||||
}
|
||||
});
|
||||
|
||||
it("should report full output totals past the byte cap", async () => {
|
||||
// 300 names of 200 bytes join to about 60KB; equal lengths make the kept prefix independent of fd's order.
|
||||
const names = Array.from({ length: 300 }, (_, i) => `${String(i).padStart(3, "0")}${"x".repeat(193)}.txt`);
|
||||
for (const name of names) writeFileSync(join(testDir, name), "");
|
||||
|
||||
const result = await findTool.execute("test-call-find-byte-cap", { pattern: "*.txt", path: testDir });
|
||||
const expected = truncateHead(names.join("\n"), { maxLines: Number.MAX_SAFE_INTEGER });
|
||||
expect(result.details?.truncation).toEqual({ ...expected, content: result.details?.truncation?.content });
|
||||
expect(result.details?.truncation?.content.split("\n")).toHaveLength(expected.outputLines);
|
||||
expect(result.details?.resultLimitReached).toBeUndefined();
|
||||
expect(getTextOutput(result)).toMatch(/\n\n\[50\.0KB limit reached\]$/);
|
||||
});
|
||||
|
||||
it("should ask custom glob operations for one result past the limit", async () => {
|
||||
const requestedLimits: number[] = [];
|
||||
const createTool = (results: string[]) => {
|
||||
const operations: FindOperations = {
|
||||
exists: () => true,
|
||||
glob: (_pattern, _cwd, options) => {
|
||||
requestedLimits.push(options.limit);
|
||||
return results.slice(0, options.limit);
|
||||
},
|
||||
};
|
||||
return createFindToolDefinition(testDir, { operations });
|
||||
};
|
||||
|
||||
const exact = await createTool(["a.txt", "b.txt"]).execute(
|
||||
"test-call-find-custom-exact",
|
||||
{ pattern: "*.txt", limit: 2 },
|
||||
undefined,
|
||||
undefined,
|
||||
fakeCtx(testDir),
|
||||
);
|
||||
expect(getTextOutput(exact)).toBe("a.txt\nb.txt");
|
||||
expect(exact.details).toBeUndefined();
|
||||
|
||||
const over = await createTool(["a.txt", "b.txt", "c.txt"]).execute(
|
||||
"test-call-find-custom-over",
|
||||
{ pattern: "*.txt", limit: 0 },
|
||||
undefined,
|
||||
undefined,
|
||||
fakeCtx(testDir),
|
||||
);
|
||||
expect(getTextOutput(over)).toBe("a.txt\n\n[1 results limit reached]");
|
||||
expect(over.details).toEqual({ resultLimitReached: 1 });
|
||||
expect(requestedLimits).toEqual([3, 2]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("ls tool", () => {
|
||||
|
|
@ -1176,6 +1264,24 @@ describe("Coding Agent Tools", () => {
|
|||
expect(output).toContain(".hidden-file");
|
||||
expect(output).toContain(".hidden-dir/");
|
||||
});
|
||||
|
||||
it("should treat limits below 1 as 1 and floor fractional limits", async () => {
|
||||
for (const name of ["a.txt", "b.txt", "c.txt"]) writeFileSync(join(testDir, name), name);
|
||||
|
||||
for (const limit of [0, -5, 0.5, 1.9]) {
|
||||
const result = await lsTool.execute("test-call-ls-small-limit", { path: testDir, limit });
|
||||
expect(getTextOutput(result)).toBe("a.txt\n\n[1 entries limit reached. Use limit=2 for more]");
|
||||
expect(result.details).toEqual({ entryLimitReached: 1 });
|
||||
}
|
||||
});
|
||||
|
||||
it("should report the entry limit only when more entries exist", async () => {
|
||||
for (const name of ["a.txt", "b.txt", "c.txt"]) writeFileSync(join(testDir, name), name);
|
||||
|
||||
const result = await lsTool.execute("test-call-ls-exact-limit", { path: testDir, limit: 3 });
|
||||
expect(getTextOutput(result)).toBe("a.txt\nb.txt\nc.txt");
|
||||
expect(result.details).toBeUndefined();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue