-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(cli): Support Windows-style tilde paths #6029
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
Changes from all commits
c7f5081
839df00
16ccc97
fde1509
a4a38d1
28ee32d
060227c
7e0392e
591c8e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,28 +11,15 @@ import type { | |||||||||||
| } from './types.js'; | ||||||||||||
| import { CommandKind } from './types.js'; | ||||||||||||
| import * as fs from 'node:fs'; | ||||||||||||
| import * as os from 'node:os'; | ||||||||||||
| import * as path from 'node:path'; | ||||||||||||
| import { | ||||||||||||
| loadServerHierarchicalMemory, | ||||||||||||
| ConditionalRulesRegistry, | ||||||||||||
| expandHomeDir, | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Since this file was already modified to import
Suggested change
Then in — qwen3.7-max via Qwen Code /review
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the catch. The completion path now uses the public |
||||||||||||
| } from '@qwen-code/qwen-code-core'; | ||||||||||||
| import { t } from '../../i18n/index.js'; | ||||||||||||
| import { SettingScope } from '../../config/settings.js'; | ||||||||||||
|
|
||||||||||||
| export function expandHomeDir(p: string): string { | ||||||||||||
| if (!p) { | ||||||||||||
| return ''; | ||||||||||||
| } | ||||||||||||
| let expandedPath = p; | ||||||||||||
| if (p.toLowerCase().startsWith('%userprofile%')) { | ||||||||||||
| expandedPath = os.homedir() + p.substring('%userprofile%'.length); | ||||||||||||
| } else if (p === '~' || p.startsWith('~/')) { | ||||||||||||
| expandedPath = os.homedir() + p.substring(1); | ||||||||||||
| } | ||||||||||||
| return path.normalize(expandedPath); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| function findExistingWorkspaceDirectory( | ||||||||||||
| directory: string, | ||||||||||||
| existingDirectories: Set<string>, | ||||||||||||
|
|
@@ -87,10 +74,10 @@ function getPathCompletions( | |||||||||||
| const trimmed = partial.trim(); | ||||||||||||
| if (!trimmed) return []; | ||||||||||||
|
|
||||||||||||
| const expanded = trimmed.startsWith('~') | ||||||||||||
| ? trimmed.replace(/^~/, os.homedir()) | ||||||||||||
| : trimmed; | ||||||||||||
| const endsWithSep = expanded.endsWith('/') || expanded.endsWith(path.sep); | ||||||||||||
| const inputEndsWithSep = trimmed.endsWith('/') || trimmed.endsWith('\\'); | ||||||||||||
| const expanded = expandHomeDir(trimmed); | ||||||||||||
| const endsWithSep = | ||||||||||||
| inputEndsWithSep || expanded.endsWith('/') || expanded.endsWith(path.sep); | ||||||||||||
| const searchDir = endsWithSep ? expanded : path.dirname(expanded); | ||||||||||||
| const namePrefix = endsWithSep ? '' : path.basename(expanded); | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -401,6 +401,11 @@ describe('resolvePath', () => { | |
| expect(result).toBe(path.resolve(cwd, 'src/main.ts')); | ||
| }); | ||
|
|
||
| it('resolves empty paths against the provided base directory', () => { | ||
| const result = resolvePath('/base/dir', ''); | ||
| expect(result).toBe(path.resolve('/base/dir', '')); | ||
| }); | ||
|
|
||
| it('returns absolute paths unchanged', () => { | ||
| const absolutePath = '/absolute/path/to/file.ts'; | ||
| const result = resolvePath('/some/base', absolutePath); | ||
|
|
@@ -419,6 +424,12 @@ describe('resolvePath', () => { | |
| expect(result).toBe(path.join(homeDir, 'documents/file.txt')); | ||
| }); | ||
|
|
||
| it('expands Windows-style tilde-prefixed paths to home directory', () => { | ||
| const homeDir = os.homedir(); | ||
| const result = resolvePath('/some/base', '~\\documents\\file.txt'); | ||
| expect(result).toBe(path.join(homeDir, 'documents', 'file.txt')); | ||
| }); | ||
|
|
||
| it('uses baseDir when provided for relative paths', () => { | ||
| const baseDir = '/custom/base'; | ||
| const result = resolvePath(baseDir, './relative/path'); | ||
|
|
@@ -619,6 +630,9 @@ describe('resolveAndValidatePath', () => { | |
| expect(resolveAndValidatePath(configWithHome, '~/project')).toBe( | ||
| homeSubdir, | ||
| ); | ||
| expect(resolveAndValidatePath(configWithHome, '~\\project')).toBe( | ||
| homeSubdir, | ||
| ); | ||
| expect(resolveAndValidatePath(configWithHome, '~')).toBe(fakeHome); | ||
| } finally { | ||
| homedirSpy.mockRestore(); | ||
|
|
@@ -931,10 +945,37 @@ describe('expandHomeDir', () => { | |
| expect(expandHomeDir('~')).toBe(path.normalize(homeDir)); | ||
| }); | ||
|
|
||
| it('should preserve trailing separators for home directory paths', () => { | ||
| expect(expandHomeDir('~/')).toBe(path.normalize(homeDir + path.sep)); | ||
| expect(expandHomeDir('~\\')).toBe(path.normalize(homeDir + path.sep)); | ||
| }); | ||
|
|
||
| it('should expand ~/path to home directory path', () => { | ||
| expect(expandHomeDir('~/documents')).toBe(path.join(homeDir, 'documents')); | ||
| }); | ||
|
|
||
| it('should expand Windows-style ~\\path to home directory path', () => { | ||
| expect(expandHomeDir('~\\documents')).toBe(path.join(homeDir, 'documents')); | ||
| }); | ||
|
|
||
| it('should preserve trailing separators in Windows-style tilde paths', () => { | ||
| expect(expandHomeDir('~\\documents\\')).toBe( | ||
| path.normalize(path.join(homeDir, 'documents') + path.sep), | ||
| ); | ||
| }); | ||
|
|
||
| it('should handle mixed separators in Windows-style tilde paths', () => { | ||
| expect(expandHomeDir('~\\foo/bar\\baz')).toBe( | ||
| path.join(homeDir, 'foo', 'bar', 'baz'), | ||
| ); | ||
| }); | ||
|
|
||
| it('should preserve legacy POSIX tilde path semantics', () => { | ||
| expect(expandHomeDir('~/foo\\bar')).toBe( | ||
| path.normalize(path.join(homeDir, 'foo\\bar')), | ||
| ); | ||
| }); | ||
|
|
||
| it('should not expand ~path (no slash)', () => { | ||
| expect(expandHomeDir('~documents')).toBe('~documents'); | ||
| }); | ||
|
|
@@ -946,7 +987,28 @@ describe('expandHomeDir', () => { | |
|
|
||
| it('should expand %userprofile%\\path to home directory path', () => { | ||
| const result = expandHomeDir('%userprofile%\\documents'); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The it('should expand %USERPROFILE%/path with forward-slash separator', () => {
expect(expandHomeDir('%USERPROFILE%/documents')).toBe(
path.join(homeDir, 'documents'),
);
});— qwen3.7-max via Qwen Code /review
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added this coverage in expect(expandHomeDir('%USERPROFILE%/documents')).toBe(
path.join(homeDir, 'documents'),
);I also added coverage for |
||
| expect(result).toBe(path.normalize(homeDir + '\\documents')); | ||
| expect(result).toBe(path.join(homeDir, 'documents')); | ||
| }); | ||
|
|
||
| it('should expand %USERPROFILE%/path with forward-slash separator', () => { | ||
| expect(expandHomeDir('%USERPROFILE%/documents')).toBe( | ||
| path.join(homeDir, 'documents'), | ||
| ); | ||
| }); | ||
|
|
||
| it('should preserve trailing separators for %USERPROFILE% paths', () => { | ||
| expect(expandHomeDir('%USERPROFILE%/')).toBe( | ||
| path.normalize(homeDir + path.sep), | ||
| ); | ||
| expect(expandHomeDir('%USERPROFILE%\\documents\\')).toBe( | ||
| path.normalize(path.join(homeDir, 'documents') + path.sep), | ||
| ); | ||
| }); | ||
|
|
||
| it('should preserve legacy %USERPROFILE% prefix semantics without a separator', () => { | ||
| expect(expandHomeDir('%USERPROFILE%foo')).toBe( | ||
| path.normalize(`${homeDir}foo`), | ||
| ); | ||
| }); | ||
|
|
||
| it('should return regular absolute path unchanged (but normalized)', () => { | ||
|
|
||
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.
[Suggestion] This test only covers the error path (directory not found). No success test verifies that
cd ~\existingDiractually callsrelocateWorkingDirectory. Consider adding a success-path test mirroring the existing../nexttest pattern:— qwen3.7-max via Qwen Code /review
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.
Thanks for the suggestion. This is covered now by the success-path Windows-style home-relative /cd test, and the targeted
cdCommand.test.ts -t "Windows-style home-relative"run passed: 2 passed, 18 skipped.