-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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.
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
base: main
Are you sure you want to change the base?
fix: lsTool path management and improve workspace file resolution #8985
Conversation
… of the unresolved one
|
Keep this PR in a mergeable state → Learn moreAll Green is an AI agent that automatically: ✅ Addresses code review comments ✅ Fixes failing CI checks ✅ Resolves merge conflicts |
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
Documentation ReviewI've reviewed this PR for documentation updates. No documentation changes are needed because:
The improvements (removing custom path rewriting, fixing workspace file resolution) are implementation details that don't require user documentation updates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 3 files
RomneyDa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nitpicks, otherwise the core changes lgtm
| 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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 👍
| expect(resolveLsToolDirPath("")).toBe("/"); | ||
| }); | ||
|
|
||
| test("resolveLsToolDirPath handles dot", () => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
|
waiting for this |
RomneyDa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a nitpick and bump on previous review comments
| const resolvedPath = await resolveInputPath(extras.ide, dirPath); | ||
| if (!args.dirPath) { | ||
| throw new ContinueError( | ||
| ContinueErrorReason.Unspecified, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andreapizzato please add a ContinueErrorReason for this or use an existing one
Description
Code changes:
lsToolusing the IDE/Platform-provided path resolutionTargeted issues
Here's a few of the issues I've found which should be cleared with this change:
AI Code Review
@continue-reviewChecklist
Screen recording or screenshot
The best demo of this fix is to compare the current main with the branch running the same task; you can view the demo I just recorded.
Tests
I actually had to remove some tests because they were testing the removed function; that specific functionality is tested elsewhere both in the codebase and in the supporting vscode/node SDKs.
Summary by cubic
Switch lsTool to the IDE path resolver and fix workspace URI checks to prevent false “directory/file not found” errors. Addresses #8877, #8744, #6220.
Written for commit 9a7d15b. Summary will update automatically on new commits.