fix(tools): added explicit selector fields for read and grep
The literal-path stat fallback made selector-shaped filenames accessible, but it did not give callers a deterministic way to read or grep a range from a literal filename such as test:1-2. Encoding that as test:1-2:1-2 remained recursively ambiguous if a longer literal file later appeared.
Read now accepts an optional selector field that is parsed independently from path. When selector is present, path is treated as the exact path first, so { path: "test:1-2", selector: "1-2" } always means lines 1-2 from the literal file test:1-2. Inline :<sel> remains supported for compatibility.
Grep now accepts an optional line-range selector field with the same literal-path behavior. Explicit selectors bypass path suffix peeling, while archive/internal/URL routing still handles non-literal structured paths.
Updated read/grep tool prompts and added deterministic regressions proving that a longer literal file like test:1-2:5-6 or test:1-2:2-2 does not change the meaning of { path: "test:1-2", selector: ... }.
This commit is contained in:
@@ -2,7 +2,7 @@ Greps files using regex.
|
||||
|
||||
<instruction>
|
||||
- Rust regex (RE2-style): alternation is `foo|bar`, not GNU BRE-style `foo\|bar`; Rust word boundaries like `\bword\b` are supported. Use line anchors or post-filters instead of lookaround/backreferences.
|
||||
- `path`: SHOULD scope to a known path (e.g. `src`); pass several as a delimited list (`src; tests`).
|
||||
- `path`: SHOULD scope to a known path (e.g. `src`); pass several as a delimited list (`src; tests`). Literal colon filename + line range? Use `selector` (e.g. `{"path":"test:1-2","selector":"1-2"}`), not recursive `path:"test:1-2:1-2"`.
|
||||
- Cross-line patterns detected from literal `\n` or `\\n` in `pattern`.
|
||||
</instruction>
|
||||
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
Read files, directories, archives, SQLite, images, documents, internal resources, and web URLs via one `path`.
|
||||
Read files, directories, archives, SQLite, images, documents, internal resources, and web URLs via `path` plus optional `selector`.
|
||||
|
||||
<instruction>
|
||||
- SHOULD parallelize independent reads.
|
||||
@@ -7,7 +7,8 @@ Read files, directories, archives, SQLite, images, documents, internal resources
|
||||
|
||||
## Parameters
|
||||
|
||||
- `path` — required. Local path, internal URI (`skill://`, `agent://`, `artifact://`, `memory://`, `rule://`, `local://`, `vault://`, `mcp://`, `omp://`, `issue://`, `pr://`, `ssh://`), or URL. Append `:<sel>` for ranges/modes (e.g. `src/foo.ts:50-200`, `src/foo.ts:raw`, `db.sqlite:users:42`).
|
||||
- `path` — required. Local path, internal URI (`skill://`, `agent://`, `artifact://`, `memory://`, `rule://`, `local://`, `vault://`, `mcp://`, `omp://`, `issue://`, `pr://`, `ssh://`), or URL. Inline `:<sel>` still works for ranges/modes (e.g. `src/foo.ts:50-200`, `src/foo.ts:raw`, `db.sqlite:users:42`).
|
||||
- `selector` — optional selector without leading `:` (e.g. `"50-200"`, `"raw"`, `"raw:50-100"`, `"conflicts"`). Use when `path` contains literal colons: `{"path":"test:1-2","selector":"1-2"}`.
|
||||
|
||||
## Selectors
|
||||
|
||||
@@ -72,6 +73,6 @@ All URI schemes take the same line selectors. `artifact://<id>` recovers spilled
|
||||
`ssh://host/<absolute-path>` reads a remote text file (UTF-8, ≤1 MiB) or lists a directory one level deep, on a pre-configured SSH host or `~/.ssh/config` alias; `ssh://host/` lists the remote root and bare `ssh://` lists the configured hosts. Files are also writable via `write` and searchable via `search`; a directory only lists (`search` refuses a directory, `write` refuses to overwrite one). A literal `:`, `?`, or `#` in the remote path must be percent-encoded (`%3A`/`%3F`/`%23`) — a trailing `:sel` is read as a line selector, and `?`/`#` start a URL query/fragment. Requires a POSIX login shell (`sh`/`bash`/`zsh`); a Windows host or a non-POSIX shell (fish, csh/tcsh) is rejected — use the `ssh` tool there.
|
||||
|
||||
<critical>
|
||||
- Line ranges go in the selector: `path="src/foo.ts:50-200"`.
|
||||
- Literal colon filename + selector? Use `selector`, not recursive `path:"file:sel:sel"`.
|
||||
- Summary footer names elided ranges? Re-issue ONLY those ranges. NEVER guess `..`/`…` content.
|
||||
</critical>
|
||||
|
||||
@@ -49,6 +49,7 @@ import {
|
||||
parseLineRanges,
|
||||
pathTargetsSsh,
|
||||
type ResolvedSearchTarget,
|
||||
resolveExistingReadPath,
|
||||
resolveReadPath,
|
||||
resolveToolSearchScope,
|
||||
selectorLineRanges,
|
||||
@@ -78,6 +79,9 @@ const searchSchema = type({
|
||||
"path?": searchPathEntry.describe(
|
||||
'file, directory, glob, internal URL, or "<file>:<lines>" selector to search; pass several as a semicolon-delimited list ("src; tests"). Omitted -> searches the workspace root (".")',
|
||||
),
|
||||
"selector?": type("string").describe(
|
||||
'line selector without a leading colon (e.g. "50-100", "50+10", "50-100,200-300"); keeps `path` literal when filenames contain colons',
|
||||
),
|
||||
"case?": type("boolean").describe("case-sensitive search"),
|
||||
"gitignore?": type("boolean").describe("respect gitignore"),
|
||||
"skip?": type("number")
|
||||
@@ -149,9 +153,35 @@ function isReadSelectorGrammar(sel: string): boolean {
|
||||
return lower === "raw" || lower === "conflicts" || parseLineRanges(sel) !== null;
|
||||
}
|
||||
|
||||
async function parsePathSpecs(rawEntries: readonly string[], cwd: string): Promise<GrepPathSpec[]> {
|
||||
async function parsePathSpecs(
|
||||
rawEntries: readonly string[],
|
||||
cwd: string,
|
||||
explicitSelector?: string,
|
||||
): Promise<GrepPathSpec[]> {
|
||||
const explicitRanges =
|
||||
explicitSelector === undefined || explicitSelector.length === 0 ? undefined : parseLineRanges(explicitSelector);
|
||||
if (explicitSelector !== undefined && !explicitRanges) {
|
||||
throw new ToolError(
|
||||
`selector "${explicitSelector}" is invalid — use line ranges like "50-100", "50+10", or "50-100,200-300" without a leading colon`,
|
||||
);
|
||||
}
|
||||
const specs: GrepPathSpec[] = [];
|
||||
for (const entry of rawEntries) {
|
||||
if (explicitRanges) {
|
||||
// Separate selector parameter makes `path` deterministic: first try the
|
||||
// exact local filesystem path (with read-path normalization), then let
|
||||
// archive/internal/URL resolution handle non-literal structured paths.
|
||||
const localPath = /^[a-z][a-z0-9+.-]*:\/\//i.test(entry)
|
||||
? undefined
|
||||
: await resolveExistingReadPath(entry, cwd);
|
||||
specs.push({
|
||||
original: entry,
|
||||
clean: localPath ?? entry,
|
||||
literalFilesystemMatch: localPath !== undefined,
|
||||
ranges: explicitRanges,
|
||||
});
|
||||
continue;
|
||||
}
|
||||
// Internal URLs (`artifact://`, `skill://`, …) use the URL-aware splitter,
|
||||
// which peels selector-shaped tails only for selector-capable schemes and
|
||||
// leaves opaque ones (`mcp://`) intact. Unlike filesystem paths, their
|
||||
@@ -886,7 +916,7 @@ export class GrepTool implements AgentTool<typeof searchSchema, GrepToolDetails>
|
||||
_onUpdate?: AgentToolUpdateCallback<GrepToolDetails>,
|
||||
_toolContext?: AgentToolContext,
|
||||
): Promise<AgentToolResult<GrepToolDetails>> {
|
||||
const { pattern, path: rawPath, case: caseSensitive, gitignore, skip } = params;
|
||||
const { pattern, path: rawPath, selector, case: caseSensitive, gitignore, skip } = params;
|
||||
|
||||
return untilAborted(signal, async () => {
|
||||
// Preserve the pattern verbatim — leading/trailing whitespace is
|
||||
@@ -904,7 +934,7 @@ export class GrepTool implements AgentTool<typeof searchSchema, GrepToolDetails>
|
||||
const scopedPaths = toPathList(rawPath);
|
||||
const effectivePaths = scopedPaths.length > 0 ? scopedPaths : ["."];
|
||||
const rawEntries = await expandDelimitedPathEntries(effectivePaths, this.session.cwd);
|
||||
const pathSpecs = await parsePathSpecs(rawEntries, this.session.cwd);
|
||||
const pathSpecs = await parsePathSpecs(rawEntries, this.session.cwd, selector);
|
||||
const materializedExternalPaths = new Map<string, string>();
|
||||
const materializeExternalUrlForSearch = async (rawPath: string) => {
|
||||
const target = parseReadUrlTarget(rawPath);
|
||||
|
||||
@@ -315,6 +315,18 @@ export function splitPathAndSel(rawPath: string): { path: string; sel?: string }
|
||||
return { path: basePath, sel };
|
||||
}
|
||||
|
||||
/** Resolve a read-tool path variant and return it only when it already exists on disk. */
|
||||
export async function resolveExistingReadPath(filePath: string, cwd: string): Promise<string | undefined> {
|
||||
const resolved = resolveReadPath(filePath, cwd);
|
||||
try {
|
||||
await fs.promises.stat(resolved);
|
||||
return resolved;
|
||||
} catch (err) {
|
||||
if (isEnoent(err) || isEnotdir(err)) return undefined;
|
||||
return resolved;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Async sibling of {@link splitPathAndSel} that prefers a literal filesystem
|
||||
* path over selector interpretation when the raw input exists on disk.
|
||||
@@ -329,12 +341,9 @@ export async function splitPathAndSelPreferringLiteral(
|
||||
): Promise<{ path: string; sel?: string }> {
|
||||
const strict = splitPathAndSel(rawPath);
|
||||
if (strict.sel === undefined) return strict;
|
||||
try {
|
||||
await fs.promises.stat(resolveReadPath(rawPath, cwd));
|
||||
return { path: rawPath };
|
||||
} catch {
|
||||
return strict;
|
||||
}
|
||||
const resolved = await resolveExistingReadPath(rawPath, cwd);
|
||||
if (resolved !== undefined) return { path: rawPath };
|
||||
return strict;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -99,6 +99,7 @@ import {
|
||||
type LineRange,
|
||||
parseLineRanges,
|
||||
pathTargetsSsh,
|
||||
resolveExistingReadPath,
|
||||
resolveReadPath,
|
||||
splitDelimitedPathEntry,
|
||||
splitInternalUrlSel,
|
||||
@@ -747,7 +748,10 @@ function splitPdfImageMemberReadPath(readPath: string): { pdfPath: string; membe
|
||||
|
||||
const readSchema = type({
|
||||
path: type("string").describe(
|
||||
'Local path, internal URI (e.g. "omp://", "issue://123", "pr://123"), or URL; append :<sel> for line ranges or raw mode (e.g. "src/foo.ts:50-100")',
|
||||
'Local path, internal URI (e.g. "omp://", "issue://123", "pr://123"), or URL. Inline :<sel> is still accepted for compatibility.',
|
||||
),
|
||||
"selector?": type("string").describe(
|
||||
'selector without a leading colon (e.g. "50-100", "raw", "raw:50-100", "conflicts"); keeps `path` literal when filenames contain colons',
|
||||
),
|
||||
});
|
||||
|
||||
@@ -2114,6 +2118,14 @@ export class ReadTool implements AgentTool<typeof readSchema, ReadToolDetails> {
|
||||
_toolContext?: AgentToolContext,
|
||||
): Promise<AgentToolResult<ReadToolDetails>> {
|
||||
let { path: readPath } = params;
|
||||
const explicitSelector = params.selector?.trim();
|
||||
const explicitParsedSelector = explicitSelector === undefined ? undefined : parseSel(explicitSelector);
|
||||
if (
|
||||
params.selector !== undefined &&
|
||||
(explicitSelector === undefined || explicitSelector.length === 0 || explicitParsedSelector?.kind === "none")
|
||||
) {
|
||||
throw invalidSelector(params.selector);
|
||||
}
|
||||
if (readPath.startsWith("file://")) {
|
||||
readPath = expandPath(readPath);
|
||||
}
|
||||
@@ -2134,40 +2146,55 @@ export class ReadTool implements AgentTool<typeof readSchema, ReadToolDetails> {
|
||||
if (!this.session.settings.get("fetch.enabled")) {
|
||||
throw new ToolError("URL reads are disabled by settings.");
|
||||
}
|
||||
if (parsedUrlTarget.ranges !== undefined) {
|
||||
if (explicitParsedSelector?.kind === "conflicts") {
|
||||
throw new ToolError("The explicit read selector `conflicts` is only supported for local files.");
|
||||
}
|
||||
const urlRaw =
|
||||
explicitParsedSelector === undefined ? parsedUrlTarget.raw : isRawSelector(explicitParsedSelector);
|
||||
const urlRanges =
|
||||
explicitParsedSelector?.kind === "lines" ? explicitParsedSelector.ranges : parsedUrlTarget.ranges;
|
||||
if (urlRanges !== undefined && urlRanges.length > 1) {
|
||||
const cached = await loadReadUrlCacheEntry(
|
||||
this.session,
|
||||
{ path: parsedUrlTarget.path, raw: parsedUrlTarget.raw },
|
||||
{ path: parsedUrlTarget.path, raw: urlRaw },
|
||||
signal,
|
||||
{ ensureArtifact: true, preferCached: true },
|
||||
);
|
||||
return this.#buildInMemoryMultiRangeResult(cached.output, parsedUrlTarget.ranges, {
|
||||
return this.#buildInMemoryMultiRangeResult(cached.output, urlRanges, {
|
||||
details: { ...cached.details },
|
||||
sourceUrl: cached.details.finalUrl,
|
||||
entityLabel: "URL output",
|
||||
raw: parsedUrlTarget.raw,
|
||||
raw: urlRaw,
|
||||
immutable: true,
|
||||
});
|
||||
}
|
||||
if (parsedUrlTarget.offset !== undefined || parsedUrlTarget.limit !== undefined) {
|
||||
const urlRange = urlRanges?.[0];
|
||||
const urlOffset = explicitParsedSelector?.kind === "lines" ? urlRange?.startLine : parsedUrlTarget.offset;
|
||||
const urlLimit =
|
||||
explicitParsedSelector?.kind === "lines" && urlRange
|
||||
? urlRange.endLine !== undefined
|
||||
? urlRange.endLine - urlRange.startLine + 1
|
||||
: undefined
|
||||
: parsedUrlTarget.limit;
|
||||
if (urlOffset !== undefined || urlLimit !== undefined) {
|
||||
const cached = await loadReadUrlCacheEntry(
|
||||
this.session,
|
||||
{ path: parsedUrlTarget.path, raw: parsedUrlTarget.raw },
|
||||
{ path: parsedUrlTarget.path, raw: urlRaw },
|
||||
signal,
|
||||
{
|
||||
ensureArtifact: true,
|
||||
preferCached: true,
|
||||
},
|
||||
);
|
||||
return this.#buildInMemoryTextResult(cached.output, parsedUrlTarget.offset, parsedUrlTarget.limit, {
|
||||
return this.#buildInMemoryTextResult(cached.output, urlOffset, urlLimit, {
|
||||
details: { ...cached.details },
|
||||
sourceUrl: cached.details.finalUrl,
|
||||
entityLabel: "URL output",
|
||||
raw: parsedUrlTarget.raw,
|
||||
raw: urlRaw,
|
||||
immutable: true,
|
||||
});
|
||||
}
|
||||
return executeReadUrl(this.session, { path: parsedUrlTarget.path, raw: parsedUrlTarget.raw }, signal);
|
||||
return executeReadUrl(this.session, { path: parsedUrlTarget.path, raw: urlRaw }, signal);
|
||||
}
|
||||
|
||||
// Handle internal URLs (agent://, artifact://, memory://, skill://, rule://, local://, mcp://, omp://, issue://, pr://).
|
||||
@@ -2175,8 +2202,9 @@ export class ReadTool implements AgentTool<typeof readSchema, ReadToolDetails> {
|
||||
// off the URL and surfaced via parseSel rather than confusing handlers.
|
||||
const internalRouter = InternalUrlRouter.instance();
|
||||
if (internalRouter.canHandle(readPath)) {
|
||||
const internalTarget = splitInternalUrlSel(readPath);
|
||||
const parsed = parseSel(internalTarget.sel);
|
||||
const internalTarget =
|
||||
explicitSelector === undefined ? splitInternalUrlSel(readPath) : { path: readPath, sel: explicitSelector };
|
||||
const parsed = explicitParsedSelector ?? parseSel(internalTarget.sel);
|
||||
if (internalTarget.sel !== undefined && parsed.kind === "none") {
|
||||
throw new ToolError(
|
||||
`Invalid selector ':${internalTarget.sel}' on '${internalTarget.path}'. Use :N, :N-M, :N+K, :N- (open-ended), a comma-separated list of ranges, :raw, or a range combined with raw (e.g. :raw:50-100).`,
|
||||
@@ -2193,7 +2221,10 @@ export class ReadTool implements AgentTool<typeof readSchema, ReadToolDetails> {
|
||||
skills: this.session.skills,
|
||||
});
|
||||
if (localFile) {
|
||||
readPath = internalTarget.sel === undefined ? localFile.path : `${localFile.path}:${internalTarget.sel}`;
|
||||
readPath =
|
||||
explicitSelector !== undefined || internalTarget.sel === undefined
|
||||
? localFile.path
|
||||
: `${localFile.path}:${internalTarget.sel}`;
|
||||
} else {
|
||||
return this.#handleInternalUrl(internalTarget.path, parsed, signal);
|
||||
}
|
||||
@@ -2206,22 +2237,28 @@ export class ReadTool implements AgentTool<typeof readSchema, ReadToolDetails> {
|
||||
// resolution share misses instead of re-globbing the workspace.
|
||||
const suffixCache: SuffixMatchCache = new Map();
|
||||
|
||||
// Prefer a literal filesystem match over the strict `:<sel>` peel so real
|
||||
// POSIX filenames whose tail matches the selector grammar (e.g. `test:1-2`,
|
||||
// `data.zip:1-2`, `notes.db:raw`) win over the structured-path resolvers
|
||||
// below. When the raw path resolves literally on disk AND the strict
|
||||
// splitter would have peeled a selector, the archive / sqlite / pdf-image
|
||||
// dispatchers must decline — otherwise `data.zip:1-2` still opens
|
||||
// `data.zip` and errors on the phantom member (issue #4618).
|
||||
const literalSplit = await splitPathAndSelPreferringLiteral(readPath, this.session.cwd);
|
||||
// Literal wins whenever the strict grammar would have peeled a suffix but
|
||||
// the async splitter decided to keep the raw path (fs.stat succeeded).
|
||||
const rawPathIsLiteral = literalSplit.sel === undefined && splitPathAndSel(readPath).sel !== undefined;
|
||||
// Prefer a literal filesystem match over selector interpretation so real
|
||||
// POSIX filenames containing selector-looking suffixes win over structured
|
||||
// archive / sqlite / pdf-image dispatch. With explicit `selector`, `path`
|
||||
// is exact: `path: "test:1-2", selector: "1-2"` means "lines 1-2 from
|
||||
// the literal file test:1-2", without recursively depending on whether a
|
||||
// longer `test:1-2:1-2` filename also exists (issue #4618).
|
||||
const literalSplit =
|
||||
explicitSelector === undefined
|
||||
? await splitPathAndSelPreferringLiteral(readPath, this.session.cwd)
|
||||
: { path: readPath, sel: explicitSelector };
|
||||
const rawPathIsLiteral =
|
||||
explicitSelector !== undefined
|
||||
? readPath.includes(":") && (await resolveExistingReadPath(readPath, this.session.cwd)) !== undefined
|
||||
: literalSplit.sel === undefined && splitPathAndSel(readPath).sel !== undefined;
|
||||
|
||||
if (!rawPathIsLiteral) {
|
||||
const archivePath = await this.#resolveArchiveReadPath(readPath, suffixCache, signal);
|
||||
if (archivePath) {
|
||||
const archiveSubPath = splitPathAndSel(archivePath.archiveSubPath);
|
||||
const archiveSubPath =
|
||||
explicitSelector === undefined
|
||||
? splitPathAndSel(archivePath.archiveSubPath)
|
||||
: { path: archivePath.archiveSubPath, sel: explicitSelector };
|
||||
const archiveParsed = parseSel(archiveSubPath.sel);
|
||||
return this.#readArchive(
|
||||
readPath,
|
||||
|
||||
@@ -149,6 +149,28 @@ describe("literal colon filename resolution (issue #4618)", () => {
|
||||
expect(output).not.toContain("line 40");
|
||||
});
|
||||
|
||||
it("uses explicit `selector` to read lines from a literal selector-shaped filename deterministically", async () => {
|
||||
const literal = path.join(tmpDir, "test:1-2");
|
||||
const longerLiteral = path.join(tmpDir, "test:1-2:5-6");
|
||||
const lines = Array.from({ length: 40 }, (_, i) => `literal line ${i + 1}`).join("\n");
|
||||
await Bun.write(literal, `${lines}\n`);
|
||||
await Bun.write(longerLiteral, "wrong longer literal\n");
|
||||
|
||||
const session = createSession();
|
||||
session.settings.set("read.summarize.enabled", false);
|
||||
const tool = new ReadTool(session);
|
||||
const result = await tool.execute("read-explicit-selector-literal", {
|
||||
path: literal,
|
||||
selector: "5-6",
|
||||
});
|
||||
const output = getText(result);
|
||||
|
||||
expect(output).toContain("literal line 5");
|
||||
expect(output).toContain("literal line 6");
|
||||
expect(output).not.toContain("literal line 30");
|
||||
expect(output).not.toContain("wrong longer literal");
|
||||
});
|
||||
|
||||
it("reads a literal file that looks like an archive selector (`data.zip:1-2`)", async () => {
|
||||
// A real POSIX file whose name ends in a selector-shaped tail after an
|
||||
// archive extension. The archive resolver would otherwise open `data.zip`
|
||||
@@ -204,6 +226,25 @@ describe("literal colon filename resolution (issue #4618)", () => {
|
||||
expect(output).not.toMatch(/not found/i);
|
||||
});
|
||||
|
||||
it("uses explicit `selector` to grep a literal selector-shaped filename deterministically", async () => {
|
||||
const literal = path.join(tmpDir, "test:1-2");
|
||||
const longerLiteral = path.join(tmpDir, "test:1-2:2-2");
|
||||
await Bun.write(literal, "needle outside\nneedle inside\nneedle outside again\n");
|
||||
await Bun.write(longerLiteral, "wrong longer literal needle\n");
|
||||
|
||||
const tool = new GrepTool(createSession());
|
||||
const result = await tool.execute("grep-explicit-selector-literal", {
|
||||
pattern: "needle",
|
||||
path: literal,
|
||||
selector: "2-2",
|
||||
});
|
||||
const output = getText(result);
|
||||
|
||||
expect(output).toContain("needle inside");
|
||||
expect(output).not.toContain("needle outside again");
|
||||
expect(output).not.toContain("wrong longer literal");
|
||||
});
|
||||
|
||||
it("searches a shell-escaped literal file whose name ends in a selector-shaped suffix", async () => {
|
||||
await fs.mkdir(path.join(tmpDir, "dir"), { recursive: true });
|
||||
await Bun.write(path.join(tmpDir, "dir", "a b:1-2"), "escaped literal needle\n");
|
||||
|
||||
Reference in New Issue
Block a user