From 7f2b25ea955c9bbd2240364a9baa00a6c6f0ad40 Mon Sep 17 00:00:00 2001 From: can1357 Date: Sun, 3 May 2026 05:43:18 +0200 Subject: [PATCH] fix(tools): resolved search and find behavior for missing path inputs - Updated search, find, ast-edit, and ast-grep to skip missing paths and run on existing ones. - Added multi-path error handling to throw ToolError only when all provided paths are missing. - Updated search and find outputs to surface skipped `missingPaths` in text and renderer warnings. - Updated CHANGELOG with multi-path tool behavior updates and `search_repos` global-search notes. - Added `PartitionedPaths` and `partitionExistingPaths` to classify existing versus missing path inputs. - Added test helpers and fixtures for multi-path missing-path cases in search/find behavior tests. --- packages/coding-agent/CHANGELOG.md | 2 + packages/coding-agent/src/tools/ast-edit.ts | 15 ++- packages/coding-agent/src/tools/ast-grep.ts | 15 ++- packages/coding-agent/src/tools/find.ts | 54 +++++++-- packages/coding-agent/src/tools/path-utils.ts | 55 +++++++++ packages/coding-agent/src/tools/search.ts | 51 +++++++-- .../test/tools/multi-path-missing.test.ts | 108 ++++++++++++++++++ 7 files changed, 280 insertions(+), 20 deletions(-) create mode 100644 packages/coding-agent/test/tools/multi-path-missing.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 2cdecd940..ce1a9d6e5 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -1,12 +1,14 @@ # Changelog ## [Unreleased] + ### Added - Added `search_code`, `search_commits`, and `search_repos` ops to the `github` tool so the search surface mirrors `gh search`'s subcommands ### Changed +- Updated multi-path `search`, `find`, `ast-edit`, and `ast-grep` calls to skip missing base paths, returning matches from remaining paths and reporting skipped paths in output - Changed `search_repos` to run as a global repository search using query qualifiers without applying the `repo` filter ### Fixed diff --git a/packages/coding-agent/src/tools/ast-edit.ts b/packages/coding-agent/src/tools/ast-edit.ts index d71d4211b..1e0d9166d 100644 --- a/packages/coding-agent/src/tools/ast-edit.ts +++ b/packages/coding-agent/src/tools/ast-edit.ts @@ -20,6 +20,7 @@ import { hasGlobPathChars, normalizePathLikeInput, parseSearchPath, + partitionExistingPaths, resolveExplicitSearchPaths, resolveToCwd, } from "./path-utils"; @@ -226,13 +227,21 @@ export class AstEditTool implements AgentTool 1) { + const partition = await partitionExistingPaths(resolvedPathInputs, this.session.cwd, parseSearchPath); + if (partition.valid.length === 0) { + throw new ToolError(`Path not found: ${partition.missing.join(", ")}`); + } + effectivePathInputs = partition.valid; + } + if (effectivePathInputs.length === 1) { + const parsedPath = parseSearchPath(effectivePathInputs[0] ?? "."); searchPath = resolveToCwd(parsedPath.basePath, this.session.cwd); globFilter = parsedPath.glob; scopePath = formatScopePath(searchPath); } else { - const multiSearchPath = await resolveExplicitSearchPaths(resolvedPathInputs, this.session.cwd, globFilter); + const multiSearchPath = await resolveExplicitSearchPaths(effectivePathInputs, this.session.cwd, globFilter); if (!multiSearchPath) { throw new ToolError("`paths` must contain at least one path or glob"); } diff --git a/packages/coding-agent/src/tools/ast-grep.ts b/packages/coding-agent/src/tools/ast-grep.ts index b0616a5c7..42a767e16 100644 --- a/packages/coding-agent/src/tools/ast-grep.ts +++ b/packages/coding-agent/src/tools/ast-grep.ts @@ -20,6 +20,7 @@ import { hasGlobPathChars, normalizePathLikeInput, parseSearchPath, + partitionExistingPaths, resolveExplicitSearchPaths, resolveToCwd, } from "./path-utils"; @@ -171,13 +172,21 @@ export class AstGrepTool implements AgentTool 1) { + const partition = await partitionExistingPaths(resolvedPathInputs, this.session.cwd, parseSearchPath); + if (partition.valid.length === 0) { + throw new ToolError(`Path not found: ${partition.missing.join(", ")}`); + } + effectivePathInputs = partition.valid; + } + if (effectivePathInputs.length === 1) { + const parsedPath = parseSearchPath(effectivePathInputs[0] ?? "."); searchPath = resolveToCwd(parsedPath.basePath, this.session.cwd); globFilter = parsedPath.glob; scopePath = formatScopePath(searchPath); } else { - const multiSearchPath = await resolveExplicitSearchPaths(resolvedPathInputs, this.session.cwd, globFilter); + const multiSearchPath = await resolveExplicitSearchPaths(effectivePathInputs, this.session.cwd, globFilter); if (!multiSearchPath) { throw new ToolError("`paths` must contain at least one path or glob"); } diff --git a/packages/coding-agent/src/tools/find.ts b/packages/coding-agent/src/tools/find.ts index e31a7dd0e..0266b9340 100644 --- a/packages/coding-agent/src/tools/find.ts +++ b/packages/coding-agent/src/tools/find.ts @@ -27,6 +27,7 @@ import { formatPathRelativeToCwd, normalizePathLikeInput, parseFindPattern, + partitionExistingPaths, resolveExplicitFindPatterns, resolveToCwd, } from "./path-utils"; @@ -59,6 +60,10 @@ export interface FindToolDetails { files?: string[]; truncated?: boolean; error?: string; + /** User-supplied paths whose base directory was missing on disk. The tool + * skipped these and continued with the surviving entries; surfaced as a + * non-fatal warning in the renderer and in the model-facing text. */ + missingPaths?: string[]; } /** @@ -114,8 +119,23 @@ export class FindTool implements AgentTool { throw new ToolError("`paths` must contain non-empty globs or paths"); } - const multiPattern = await resolveExplicitFindPatterns(normalizedPatterns, this.session.cwd); - const parsedPattern = multiPattern ? null : parseFindPattern(normalizedPatterns[0] ?? "."); + // Tolerate missing entries in a multi-path call: skip ones whose base + // directory is gone, and only error if every entry is missing. Single + // missing path keeps the original ENOENT semantics — the user explicitly + // asked about that one path, so silent empty results would be misleading. + let missingPaths: string[] = []; + let effectivePatterns = normalizedPatterns; + if (normalizedPatterns.length > 1 && !this.#customOps) { + const partition = await partitionExistingPaths(normalizedPatterns, this.session.cwd, parseFindPattern); + if (partition.valid.length === 0) { + throw new ToolError(`Path not found: ${partition.missing.join(", ")}`); + } + effectivePatterns = partition.valid; + missingPaths = partition.missing; + } + + const multiPattern = await resolveExplicitFindPatterns(effectivePatterns, this.session.cwd); + const parsedPattern = multiPattern ? null : parseFindPattern(effectivePatterns[0] ?? "."); const hasGlob = multiPattern ? true : (parsedPattern?.hasGlob ?? false); const globPattern = multiPattern?.globPattern ?? parsedPattern?.globPattern ?? "**/*"; const searchPath = resolveToCwd(multiPattern?.basePath ?? parsedPattern?.basePath ?? ".", this.session.cwd); @@ -124,7 +144,6 @@ export class FindTool implements AgentTool { if (searchPath === "/") { throw new ToolError("Searching from root directory '/' is not allowed"); } - const rawLimit = limit ?? DEFAULT_LIMIT; const effectiveLimit = Number.isFinite(rawLimit) ? Math.floor(rawLimit) : Number.NaN; if (!Number.isFinite(effectiveLimit) || effectiveLimit <= 0) { @@ -141,16 +160,29 @@ export class FindTool implements AgentTool { }); }; + const missingPathsNote = + missingPaths.length > 0 ? `Skipped missing paths: ${missingPaths.join(", ")}` : undefined; + const buildResult = (files: string[]): AgentToolResult => { if (files.length === 0) { - const details: FindToolDetails = { scopePath, fileCount: 0, files: [], truncated: false }; - return toolResult(details).text("No files found matching pattern").done(); + const details: FindToolDetails = { + scopePath, + fileCount: 0, + files: [], + truncated: false, + missingPaths: missingPaths.length > 0 ? missingPaths : undefined, + }; + const text = missingPathsNote + ? `No files found matching pattern\n${missingPathsNote}` + : "No files found matching pattern"; + return toolResult(details).text(text).done(); } const listLimit = applyListLimit(files, { limit: effectiveLimit }); const limited = listLimit.items; const limitMeta = listLimit.meta; - const rawOutput = limited.join("\n"); + const baseOutput = limited.join("\n"); + const rawOutput = missingPathsNote ? `${baseOutput}\n\n${missingPathsNote}` : baseOutput; const truncation = truncateHead(rawOutput, { maxLines: Number.MAX_SAFE_INTEGER }); const details: FindToolDetails = { @@ -160,6 +192,7 @@ export class FindTool implements AgentTool { truncated: Boolean(limitMeta.resultLimit || truncation.truncated), resultLimitReached: limitMeta.resultLimit?.reached, truncation: truncation.truncated ? truncation : undefined, + missingPaths: missingPaths.length > 0 ? missingPaths : undefined, }; const resultBuilder = toolResult(details) @@ -380,12 +413,18 @@ export const findToolRenderer = { const truncated = Boolean(details?.truncated || truncation || details?.resultLimitReached || limits?.resultLimit); const files = details?.files ?? []; + const missingPaths = details?.missingPaths ?? []; + const missingNote = + missingPaths.length > 0 ? uiTheme.fg("warning", `skipped missing: ${missingPaths.join(", ")}`) : undefined; + if (fileCount === 0) { const header = renderStatusLine( { icon: "warning", title: "Find", description: args?.paths?.join(", "), meta: ["0 files"] }, uiTheme, ); - return new Text([header, formatEmptyMessage("No files found", uiTheme)].join("\n"), 0, 0); + const lines = [header, formatEmptyMessage("No files found", uiTheme)]; + if (missingNote) lines.push(missingNote); + return new Text(lines.join("\n"), 0, 0); } const meta: string[] = [formatCount("file", fileCount)]; if (details?.scopePath) meta.push(`in ${details.scopePath}`); @@ -406,6 +445,7 @@ export const findToolRenderer = { if (truncationReasons.length > 0) { extraLines.push(uiTheme.fg("warning", `truncated: ${truncationReasons.join(", ")}`)); } + if (missingNote) extraLines.push(missingNote); let cached: RenderCache | undefined; return { diff --git a/packages/coding-agent/src/tools/path-utils.ts b/packages/coding-agent/src/tools/path-utils.ts index 159c2571b..2851699b7 100644 --- a/packages/coding-agent/src/tools/path-utils.ts +++ b/packages/coding-agent/src/tools/path-utils.ts @@ -2,6 +2,7 @@ import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; import * as url from "node:url"; +import { isEnoent } from "@oh-my-pi/pi-utils"; const UNICODE_SPACES = /[\u00A0\u2000-\u200A\u202F\u205F\u3000]/g; const NARROW_NO_BREAK_SPACE = "\u202F"; @@ -442,6 +443,60 @@ export async function resolveExplicitFindPatterns( return resolveFindPatternItems([...new Set(patternItems)], cwd); } +/** + * Result of partitioning a list of user-supplied paths/globs into entries whose + * base directory currently exists on disk versus those that do not. + * + * Used by multi-path tools (search, find, ast_grep, ast_edit) to tolerate one + * or more missing entries in a multi-path call: the surviving entries should + * still be searched, with the missing entries surfaced as a non-fatal warning. + */ +export interface PartitionedPaths { + /** Raw input strings whose resolved base path exists. */ + valid: string[]; + /** Raw input strings whose resolved base path is missing (ENOENT). */ + missing: string[]; +} + +/** + * Stat each input's base path concurrently; return entries split by existence. + * + * `splitter` is expected to be {@link parseFindPattern} or + * {@link parseSearchPath}: both return a `basePath` field that this helper + * resolves against `cwd` and stats. ENOENT is the only swallowed error — every + * other stat failure (permission, IO, etc.) propagates so callers do not silently + * skip paths that exist but are unreadable. + * + * Order of `valid` and `missing` follows the input order, so callers can rely + * on `valid[0]` matching the first surviving user-supplied entry. + */ +export async function partitionExistingPaths( + items: string[], + cwd: string, + splitter: (item: string) => { basePath: string }, +): Promise { + const settled = await Promise.all( + items.map(async item => { + const { basePath } = splitter(item); + const absoluteBasePath = resolveToCwd(basePath, cwd); + try { + await fs.promises.stat(absoluteBasePath); + return { item, exists: true } as const; + } catch (err) { + if (isEnoent(err)) return { item, exists: false } as const; + throw err; + } + }), + ); + const valid: string[] = []; + const missing: string[] = []; + for (const entry of settled) { + if (entry.exists) valid.push(entry.item); + else missing.push(entry.item); + } + return { valid, missing }; +} + export function resolveReadPath(filePath: string, cwd: string): string { const resolved = resolveToCwd(filePath, cwd); const shellEscapedVariant = tryShellEscapedPath(resolved); diff --git a/packages/coding-agent/src/tools/search.ts b/packages/coding-agent/src/tools/search.ts index 0c16506ef..cf711feb4 100644 --- a/packages/coding-agent/src/tools/search.ts +++ b/packages/coding-agent/src/tools/search.ts @@ -22,6 +22,7 @@ import { hasGlobPathChars, normalizePathLikeInput, parseSearchPath, + partitionExistingPaths, resolveExplicitSearchPaths, resolveToCwd, } from "./path-utils"; @@ -68,6 +69,10 @@ export interface SearchToolDetails { * `result.text` lines but uses a `│` gutter and `*` to mark match lines (vs space for * context). The TUI uses this directly so it never parses model-facing hashline anchors. */ displayContent?: string; + /** User-supplied paths whose base directory was missing on disk. The tool + * skipped these and continued with the surviving entries; surfaced as a + * non-fatal warning in the renderer and in the model-facing text. */ + missingPaths?: string[]; } type SearchParams = Static; @@ -140,13 +145,26 @@ export class SearchTool implements AgentTool 1) { + const partition = await partitionExistingPaths(resolvedPathInputs, this.session.cwd, parseSearchPath); + if (partition.valid.length === 0) { + throw new ToolError(`Path not found: ${partition.missing.join(", ")}`); + } + effectivePaths = partition.valid; + missingPaths = partition.missing; + } + if (effectivePaths.length === 1) { + const parsedPath = parseSearchPath(effectivePaths[0] ?? "."); searchPath = resolveToCwd(parsedPath.basePath, this.session.cwd); globFilter = parsedPath.glob; scopePath = formatScopePath(searchPath); } else { - const multiSearchPath = await resolveExplicitSearchPaths(resolvedPathInputs, this.session.cwd, globFilter); + const multiSearchPath = await resolveExplicitSearchPaths(effectivePaths, this.session.cwd, globFilter); if (!multiSearchPath) { throw new ToolError("`paths` must contain at least one path or glob"); } @@ -285,6 +303,8 @@ export class SearchTool implements AgentTool(); + const missingPathsNote = + missingPaths.length > 0 ? `Skipped missing paths: ${missingPaths.join(", ")}` : undefined; if (selectedMatches.length === 0) { const details: SearchToolDetails = { scopePath, @@ -292,8 +312,10 @@ export class SearchTool implements AgentTool 0 ? missingPaths : undefined, }; - return toolResult(details).text("No matches found").done(); + const text = missingPathsNote ? `No matches found\n${missingPathsNote}` : "No matches found"; + return toolResult(details).text(text).done(); } const outputLines: string[] = []; let linesTruncated = false; @@ -365,6 +387,9 @@ export class SearchTool implements AgentTool 0 ? missingPaths : undefined, }; if (truncation.truncated) details.truncation = truncation; if (linesTruncated) details.linesTruncated = true; @@ -487,12 +513,20 @@ export const searchToolRenderer = { details?.truncated || truncation || limits?.matchLimit || limits?.resultLimit || limits?.columnTruncated, ); + const missingPathsList = details?.missingPaths ?? []; + const missingNote = + missingPathsList.length > 0 + ? uiTheme.fg("warning", `skipped missing: ${missingPathsList.join(", ")}`) + : undefined; + if (matchCount === 0) { const header = renderStatusLine( { icon: "warning", title: "Search", description: args?.pattern, meta: ["0 matches"] }, uiTheme, ); - return new Text([header, formatEmptyMessage("No matches found", uiTheme)].join("\n"), 0, 0); + const lines = [header, formatEmptyMessage("No matches found", uiTheme)]; + if (missingNote) lines.push(missingNote); + return new Text(lines.join("\n"), 0, 0); } const summaryParts = [formatCount("match", matchCount), formatCount("file", fileCount)]; @@ -538,8 +572,11 @@ export const searchToolRenderer = { if (limits?.columnTruncated) truncationReasons.push(`line length ${limits.columnTruncated.maxColumn}`); if (truncation?.artifactId) truncationReasons.push(formatFullOutputReference(truncation.artifactId)); - const extraLines = - truncationReasons.length > 0 ? [uiTheme.fg("warning", `truncated: ${truncationReasons.join(", ")}`)] : []; + const extraLines: string[] = []; + if (truncationReasons.length > 0) { + extraLines.push(uiTheme.fg("warning", `truncated: ${truncationReasons.join(", ")}`)); + } + if (missingNote) extraLines.push(missingNote); let cached: RenderCache | undefined; return { diff --git a/packages/coding-agent/test/tools/multi-path-missing.test.ts b/packages/coding-agent/test/tools/multi-path-missing.test.ts new file mode 100644 index 000000000..9d22be15d --- /dev/null +++ b/packages/coding-agent/test/tools/multi-path-missing.test.ts @@ -0,0 +1,108 @@ +import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import { createTools, type ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; + +// Regression for grievances #208 (find) and #209 (search): a multi-path call +// that includes an entry which does not exist on disk must not abort the whole +// lookup. The tool should skip the missing entry and return matches from the +// surviving entries, with a non-fatal "skipped missing paths" notice. + +function createTestSession(cwd: string, overrides: Partial = {}): ToolSession { + return { + cwd, + hasUI: false, + getSessionFile: () => null, + getSessionSpawns: () => "*", + settings: Settings.isolated(), + ...overrides, + }; +} + +function getText(result: { content: Array<{ type: string; text?: string }> }): string { + return result.content + .filter(entry => entry.type === "text") + .map(entry => entry.text ?? "") + .join("\n"); +} + +describe("multi-path tools tolerate missing entries", () => { + let tempDir: string; + + beforeEach(async () => { + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "pi-multi-path-missing-")); + await fs.mkdir(path.join(tempDir, "src"), { recursive: true }); + await Bun.write(path.join(tempDir, "src", "alpha.ts"), "shared-needle alpha\n"); + await Bun.write(path.join(tempDir, "src", "beta.ts"), "shared-needle beta\n"); + }); + + afterEach(async () => { + await fs.rm(tempDir, { recursive: true, force: true }); + }); + + it("search returns matches from existing paths and reports the missing one", async () => { + const tools = await createTools(createTestSession(tempDir)); + const tool = tools.find(entry => entry.name === "search"); + if (!tool) throw new Error("Missing search tool"); + + const result = await tool.execute("search-multi-missing", { + pattern: "shared-needle", + paths: ["src/", "tests/"], + }); + + const text = getText(result); + const details = result.details as { fileCount?: number; missingPaths?: string[] } | undefined; + + expect(text).toContain("shared-needle alpha"); + expect(text).toContain("shared-needle beta"); + expect(text).toContain("Skipped missing paths: tests/"); + expect(details?.fileCount).toBe(2); + expect(details?.missingPaths).toEqual(["tests/"]); + }); + + it("search errors only when every path is missing", async () => { + const tools = await createTools(createTestSession(tempDir)); + const tool = tools.find(entry => entry.name === "search"); + if (!tool) throw new Error("Missing search tool"); + + const promise = tool.execute("search-all-missing", { + pattern: "shared-needle", + paths: ["does-not-exist/", "also-missing/"], + }); + + await expect(promise).rejects.toThrow(/Path not found.*does-not-exist.*also-missing/s); + }); + + it("find returns matches from existing globs and reports the missing one", async () => { + const tools = await createTools(createTestSession(tempDir)); + const tool = tools.find(entry => entry.name === "find"); + if (!tool) throw new Error("Missing find tool"); + + const result = await tool.execute("find-multi-missing", { + paths: ["src/**/*.ts", "tests/**/*.ts"], + }); + + const text = getText(result); + const details = result.details as { fileCount?: number; missingPaths?: string[] } | undefined; + + expect(text).toContain("src/alpha.ts"); + expect(text).toContain("src/beta.ts"); + expect(text).toContain("Skipped missing paths: tests/**/*.ts"); + expect(details?.fileCount).toBe(2); + expect(details?.missingPaths).toEqual(["tests/**/*.ts"]); + }); + + it("find errors only when every glob's base directory is missing", async () => { + const tools = await createTools(createTestSession(tempDir)); + const tool = tools.find(entry => entry.name === "find"); + if (!tool) throw new Error("Missing find tool"); + + const promise = tool.execute("find-all-missing", { + paths: ["nope/**/*.ts", "also-nope/**/*.ts"], + }); + + await expect(promise).rejects.toThrow(/Path not found.*nope.*also-nope/s); + }); +});