-
Notifications
You must be signed in to change notification settings - Fork 1.5k
paths: resolve windows short names #2573
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?
Conversation
It uses sync, which is spooky, but I did not really want to update every single usage of the prompt path service for something that is a very edge case and that should be handled be the LRU generally. Closes microsoft/vscode#278527
| const result = execFileSync('powershell.exe', [ | ||
| '-NoProfile', | ||
| '-Command', | ||
| '[System.IO.Path]::GetFullPath($env:VSCODE_SHORT_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.
And yes, unfortunately Node does not seem to have any built-in bindings to do this and realpath does not resolve 8.3 short paths in spite of what language models try to tell you 😛
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.
Pull request overview
This PR adds support for resolving Windows 8.3 short filenames (e.g., PROGRA~1) to their long form (e.g., Program Files) in the prompt path representation service. This addresses an issue where AI model responses containing short Windows paths would not be properly resolved back to their full paths. The implementation uses a node-specific service override with LRU caching to minimize expensive shell calls, and only resolves short paths on Windows platforms.
Key Changes
- Created
PromptPathRepresentationServiceNodethat extends the base service with 8.3 short path resolution logic - Implemented segment-by-segment resolution with LRU caching to reuse directory prefix resolutions across multiple files
- Used PowerShell's
GetFullPathviaexecFileSyncto resolve short paths, with environment variables to prevent shell injection
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/platform/prompts/node/promptPathRepresentationServiceNode.ts |
New node-specific service implementation with 8.3 short path detection regex, segment-wise resolution algorithm, PowerShell-based resolution, and LRU cache for performance |
src/platform/prompts/test/node/promptPathRepresentationServiceNode.spec.ts |
Comprehensive unit tests with test subclass that mocks shell resolution, covering single/multiple short segments, caching behavior, error handling, and integration with URI APIs |
src/extension/extension/vscode-node/services.ts |
Service registration for the new node-specific implementation, overriding the common service with platform-specific behavior |
| * Pattern to detect 8.3 short filename segments (e.g., PROGRA~1, DOCUME~2) | ||
| * Format: 1-6 characters, followed by tilde and 1+ digits | ||
| */ | ||
| const SHORT_NAME_SEGMENT_PATTERN = /^[^~]{1,6}~\d+$/i; |
Copilot
AI
Dec 12, 2025
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 regex pattern for 8.3 short names is incorrect. According to the 8.3 naming convention, the base name can be up to 8 characters (not 1-6), and the tilde-number suffix is part of those 8 characters. For example, "PROGRA1" has 6 chars before the tilde, but valid short names like "ABCDEFG1" would have 7 chars before the tilde. The pattern should be /^[^~]{1,8}~\d+$/i to match the actual 8.3 format, or more precisely /^.{1,6}~\d+$/ since the characters before the tilde (plus the tilde and digit) should total at most 8 characters for the base name.
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.
copilot is wrong here, an 8.3 name can be up to 8 characters and therefore the prefix length can be 6 characters followed by a tilde and a digit
src/platform/prompts/node/promptPathRepresentationServiceNode.ts
Outdated
Show resolved
Hide resolved
src/platform/prompts/node/promptPathRepresentationServiceNode.ts
Outdated
Show resolved
Hide resolved
src/platform/prompts/test/node/promptPathRepresentationServiceNode.spec.ts
Show resolved
Hide resolved
73ee181 to
2835d79
Compare
If you want to do it async, I think it's fine to fork a service like this to a version that's just used for the agent, so you don't spend any time updating chat participant usages. |
|
@connor4312 Can we hold off this change a bit?
|
|
aeschli
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.
Let's first discuss the need of this fix.
- Is it a common scenario?
- How many dups and votes we have for this?
bpasero
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.
It uses sync
👎
|
In VS Code we are not doing any such path normalisation, other than Node.js itself provides a method you can use to expand these paths: |
|
TIL
Makes sense, I think though there will be mismatches though if files are seen by other apps or the terminal. But not sure how frequently this path gets hit. |
|
We need to look at all paths that are emitted by tools. Such path can be the result of from NodeJS fs operations, codebase searches and terminal output. IMO we should only dive into that adventure if this is a problem hit by many users. |
|
Btw there is a popular ask in microsoft/vscode#130082 to always force the file paths (including the workspace) to be in their expanded form to avoid these issues. |
It uses sync, which is spooky, but I did not really want to update every
single usage of the prompt path service for something that is a very
edge case and that should be handled be the LRU generally.
Closes microsoft/vscode#278527