fix(tools): closed two open literal-wins gaps flagged by codex
Two Codex bot findings from earlier PR reviews were still open.
1. local:// URL selector shadow (read.ts): the local:// branch resolved
`local://foo:1-2` and rewrote readPath to `${localFile.path}:${sel}`,
then let splitPathAndSelPreferringLiteral run on the synthesized
string. A sibling literal `${localFile.path}:${sel}` file would win
over the intended URL selector semantics. The branch now promotes the
URL selector into the explicit-selector state and sets
readPath = localFile.path, so downstream literal-preferring routing
never re-splits the concatenation.
2. Delimited expansion before literal probe (path-utils.ts): grep called
expandDelimitedPathEntries before parsePathSpecs, and
splitDelimitedPathEntry only checked whether the peeled base of the
entry resolved. A real POSIX file whose name contained a delimiter
plus a selector-shaped tail (a;b:1-2) got split into ["a", "b:1-2"]
and never reached the literal-preferring probe. splitDelimitedPathEntry
now short-circuits on probeLiteralPathExists — "missing" is the only
outcome that lets delimiter expansion run.
Added regressions: `read local://notes.md:1-2` still slices the base file
when a sibling `notes.md:1-2` literal exists, and grep searches a real
`a;b:1-2` file without semicolon-splitting.
This commit is contained in:
@@ -709,7 +709,12 @@ export async function splitDelimitedPathEntry(
|
||||
const normalizedEntry = normalizePathLikeInput(entry);
|
||||
if (!hasTopLevelPathDelimiter(normalizedEntry)) return null;
|
||||
if (isInternalUrlPath(normalizedEntry)) return null;
|
||||
|
||||
// A real POSIX file may contain the delimiter and a selector-shaped tail
|
||||
// (`a;b:1-2`, `a b:1-2`). Preserve the raw entry whenever the full literal
|
||||
// resolves — or is only ambiguous — so downstream literal-preferring
|
||||
// splitters see it before delimiter expansion peels or splits (issue #4618
|
||||
// reviewer feedback: delimited expansion ran before the literal check).
|
||||
if ((await probeLiteralPathExists(normalizedEntry, cwd)) !== "missing") return null;
|
||||
const splitter = options.splitter ?? parseSearchPath;
|
||||
const peeledEntry = splitPathAndSel(normalizedEntry).path;
|
||||
if (!hasGlobPathChars(peeledEntry) && (await delimitedPathPartResolves(normalizedEntry, cwd, splitter))) {
|
||||
|
||||
@@ -2118,8 +2118,8 @@ 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);
|
||||
let explicitSelector = params.selector?.trim();
|
||||
let explicitParsedSelector = explicitSelector === undefined ? undefined : parseSel(explicitSelector);
|
||||
if (
|
||||
params.selector !== undefined &&
|
||||
(explicitSelector === undefined || explicitSelector.length === 0 || explicitParsedSelector?.kind === "none")
|
||||
@@ -2221,10 +2221,16 @@ export class ReadTool implements AgentTool<typeof readSchema, ReadToolDetails> {
|
||||
skills: this.session.skills,
|
||||
});
|
||||
if (localFile) {
|
||||
readPath =
|
||||
explicitSelector !== undefined || internalTarget.sel === undefined
|
||||
? localFile.path
|
||||
: `${localFile.path}:${internalTarget.sel}`;
|
||||
readPath = localFile.path;
|
||||
// Promote the URL-embedded selector into the explicit-selector state so
|
||||
// downstream literal-preferring routing does NOT re-split the synthesized
|
||||
// `${localFile.path}:${sel}` string — a sibling literal file at that name
|
||||
// would otherwise shadow the intended local:// URL selector semantics
|
||||
// (issue #4618 reviewer feedback on c493d12).
|
||||
if (explicitSelector === undefined && internalTarget.sel !== undefined) {
|
||||
explicitSelector = internalTarget.sel;
|
||||
explicitParsedSelector = parsed;
|
||||
}
|
||||
} else {
|
||||
return this.#handleInternalUrl(internalTarget.path, parsed, signal);
|
||||
}
|
||||
|
||||
@@ -450,6 +450,35 @@ describe("GrepTool internal URL resolution", () => {
|
||||
expect(text).toMatch(/^\*\d+:.*needle/m);
|
||||
});
|
||||
|
||||
it("read local://<name>:<sel> honors URL selector even when a sibling literal `<name>:<sel>` file exists (issue #4618)", async () => {
|
||||
const localRoot = path.join(artifactsDir, "local");
|
||||
await fs.mkdir(localRoot, { recursive: true });
|
||||
// Base file targeted by `local://notes.md`; selector should slice this one.
|
||||
await Bun.write(
|
||||
path.join(localRoot, "notes.md"),
|
||||
`${Array.from({ length: 10 }, (_, i) => `url-target line ${i + 1}`).join("\n")}\n`,
|
||||
);
|
||||
// Sibling literal `notes.md:1-2` under the same local root — must NOT
|
||||
// shadow the URL selector semantics of `local://notes.md:1-2`.
|
||||
await Bun.write(path.join(localRoot, "notes.md:1-2"), "sibling literal shadow\n");
|
||||
|
||||
LocalProtocolHandler.setOverride({ getArtifactsDir: () => artifactsDir, getSessionId: () => "session" });
|
||||
|
||||
const session = createSession({ hasEditTool: true });
|
||||
session.settings.set("read.summarize.enabled", false);
|
||||
const result = await new ReadTool(session).execute("test-read-local-url-selector", {
|
||||
path: "local://notes.md:1-2",
|
||||
});
|
||||
|
||||
const text = getResultText(result);
|
||||
// The base file was targeted (URL selector semantics preserved), not the
|
||||
// sibling literal. Content check is enough — the read tool's context
|
||||
// expansion around the requested range is unrelated to the shadow bug.
|
||||
expect(text).toContain("url-target line 1");
|
||||
expect(text).toContain("url-target line 2");
|
||||
expect(text).not.toContain("sibling literal shadow");
|
||||
});
|
||||
|
||||
it("keeps hashlines on mutable files when mixed with immutable artifact:// inputs", async () => {
|
||||
const content = "alpha line\nbeta needle line\ngamma line\n";
|
||||
await Bun.write(path.join(artifactsDir, "11.bash.log"), content);
|
||||
|
||||
@@ -289,6 +289,24 @@ describe("literal colon filename resolution (issue #4618)", () => {
|
||||
expect(output).toContain("escaped literal needle");
|
||||
});
|
||||
|
||||
it("searches a literal file whose name contains a semicolon and selector-shaped tail (`a;b:1-2`)", async () => {
|
||||
// Semicolon is the delimited-path separator; without a raw-literal
|
||||
// probe in `splitDelimitedPathEntry`, expandDelimitedPathEntries would
|
||||
// split `a;b:1-2` into `["a", "b:1-2"]` before grep saw the literal file.
|
||||
const literal = path.join(tmpDir, "a;b:1-2");
|
||||
await Bun.write(literal, "delimited literal needle\n");
|
||||
|
||||
const tool = new GrepTool(createSession());
|
||||
const result = await tool.execute("grep-literal-semicolon-selector", {
|
||||
pattern: "needle",
|
||||
path: literal,
|
||||
});
|
||||
const output = getText(result);
|
||||
|
||||
expect(output).toContain("delimited literal needle");
|
||||
expect(output).not.toMatch(/not found/i);
|
||||
});
|
||||
|
||||
it("searches a literal file that looks like an archive selector (`data.zip:1-2`)", async () => {
|
||||
// The base archive exists too; grep must not rematerialize the raw
|
||||
// literal path as archive `data.zip` plus phantom member `1-2`.
|
||||
|
||||
Reference in New Issue
Block a user