Skip to content

fix: lsTool path management and improve workspace file resolution #8985

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+13 −42
Open

fix: lsTool path management and improve workspace file resolution #8985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
19f0815
fix(lsTool): Remove custom path reinterpreter in favor of the IDE pat…
andreapizzato Dec 3, 2025
de930a7
fix(pathResolver): Check the final computed URI for existence instead…
andreapizzato Dec 3, 2025
931d455
chore(pathResolver): Enhance comments
andreapizzato Dec 3, 2025
9a7d15b
chore(pathResolver): Enhance comments
andreapizzato Dec 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 9 additions & 14 deletions core/tools/implementations/lsTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,20 @@ import ignore from "ignore";

import { ToolImpl } from ".";
import { walkDir } from "../../indexing/walkDir";
import { resolveInputPath } from "../../util/pathResolver";
import { ContinueError, ContinueErrorReason } from "../../util/errors";

export function resolveLsToolDirPath(dirPath: string | undefined) {
if (!dirPath || dirPath === ".") {
return "/";
}
// Don't strip leading slash from absolute paths - let the resolver handle it
if (dirPath.startsWith(".") && !dirPath.startsWith("./")) {
return dirPath.slice(1);
}
return dirPath.replace(/\\/g, "/");
}
import { resolveInputPath } from "../../util/pathResolver";

const MAX_LS_TOOL_LINES = 200;

export const lsToolImpl: ToolImpl = async (args, extras) => {
const dirPath = resolveLsToolDirPath(args?.dirPath);
const resolvedPath = await resolveInputPath(extras.ide, dirPath);
if (!args.dirPath) {
throw new ContinueError(
ContinueErrorReason.Unspecified,
`No paths to explore were provided`,
);
}

const resolvedPath = await resolveInputPath(extras.ide, args?.dirPath);
if (!resolvedPath) {
throw new ContinueError(
ContinueErrorReason.DirectoryNotFound,
Expand Down
26 changes: 1 addition & 25 deletions core/tools/implementations/lsTool.vitest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { expect, test, vi } from "vitest";
import { ToolExtras } from "../..";
import * as walkDirModule from "../../indexing/walkDir";
import { lsToolImpl, resolveLsToolDirPath } from "./lsTool";
import { lsToolImpl } from "./lsTool";

vi.mock("../../indexing/walkDir");

Expand All @@ -12,30 +12,6 @@ const mockExtras = {
},
} as unknown as ToolExtras;

test("resolveLsToolDirPath handles undefined path", () => {
expect(resolveLsToolDirPath(undefined)).toBe("/");
});

test("resolveLsToolDirPath handles empty string", () => {
expect(resolveLsToolDirPath("")).toBe("/");
});

test("resolveLsToolDirPath handles dot", () => {
Copy link
Collaborator

@RomneyDa RomneyDa Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove these tests? Should we only remove the ones checking for empty path?

Copy link
Author

@andreapizzato andreapizzato Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The input path is now only processed through resolveInputPath (core/tools/implementations/lsTool.ts:18 in this PR); therefore I had to remove the tests because the function itself is no longer existing. This does not mean at all those tests were unnecessary and shouldn't be brought back; in fact I discovered this bug trying to use SpecKit (which hosts scripts inside a .speckit folder) and Continue was madly looping unable to read files that were in fact there. Basically I'd love to bring back the old tests + tests for files and folders starting with a . (e,g, .env, .git).

Is it better to test for those cases while testing resolveInputPath alone or do we want to have a separate function evaluating ls input path to be able to bring back these tests?

In the original path parsing function (resolveLsToolDirPath - core/tools/implementations/lsTool.ts:8 in this PR):

  • why was '.' (current folder) replaced with '/' filesystem root?
  • what was the original intent behind removing the initial '.' from a path? Preventing access to parent folders?

Copy link
Collaborator

@RomneyDa RomneyDa Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for explaining. The simple reason is that "." doesn't work with our file reading and listing IDE utils, they require URIs, so we assume "." means relative. So you're right that it's a bug. I think the fix is that "." and "./" paths should resolve relative to the workspace URI root (first workspace if there are multiple)

expect(resolveLsToolDirPath(".")).toBe("/");
});

test("resolveLsToolDirPath handles dot relative", () => {
expect(resolveLsToolDirPath("./hi")).toBe("./hi");
});

test("resolveLsToolDirPath normalizes backslashes to forward slashes", () => {
expect(resolveLsToolDirPath("path\\to\\dir")).toBe("path/to/dir");
});

test("resolveLsToolDirPath preserves forward slashes", () => {
expect(resolveLsToolDirPath("path/to/dir")).toBe("path/to/dir");
});

test("lsToolImpl truncates output when entries exceed MAX_LS_TOOL_LINES", async () => {
// Generate more than MAX_LS_TOOL_LINES entries (which is 200)
const mockEntries = Array.from({ length: 250 }, (_, i) => `file${i}.txt`);
Expand Down
6 changes: 3 additions & 3 deletions core/util/pathResolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ export interface ResolvedPath {
*/
async function isUriWithinWorkspace(ide: IDE, uri: string): Promise<boolean> {
const workspaceDirs = await ide.getWorkspaceDirs();
const { foundInDir } = findUriInDirs(uri, workspaceDirs);
const { foundInDir, uri: foundUri } = findUriInDirs(uri, workspaceDirs);
Copy link
Collaborator

@RomneyDa RomneyDa Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be unnecessary, foundUri and original uri should be the same

Copy link
Author

@andreapizzato andreapizzato Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my bad, I didn't go deeper looking at the implementation of findUriInDirs: it is, in fact, the same. This change does not make sense, will push a new commit removing it 👍


// Check both: within workspace path AND file exists
// Check both: within workspace path AND resolved file exists
if (foundInDir !== null) {
return await ide.fileExists(uri);
return await ide.fileExists(foundUri);
}

return false;
Expand Down
Loading