fix(tools): translate MSYS paths to Windows-native before shell lint (#66494) - #66524
fix(tools): translate MSYS paths to Windows-native before shell lint (#66494)#66524stantheman0128 wants to merge 3 commits into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the lint invocation through the MSYS path boundary. The local Windows premise remains present on current main: tools/file_tools.py:385 resolves MSYS paths to native paths, while tools/file_operations.py:1771 sends them back through Git-Bash escaping for shell linters.
Problems
tools/file_operations.py:1782gates translation only on the host-wide_IS_WINDOWSflag.ShellFileOperationsis shared by local and remote backends (tools/file_operations.py:793-799), so a Windows host running a Docker/SSH/Modal/etc. backend can have valid in-backend POSIX/e/...paths rewritten toE:/.... This conflicts with the existing backend-path boundary documented bytests/tools/test_file_tools.py:475-490.- The added tests mock
_execon an object made with__new__, so they do not exercise the backend distinction.
Suggested changes
- Restrict conversion to
LocalEnvironmentas well as Windows;tools/file_operations.py:1882-1901already has an established local-backend gate pattern. - Add a remote-backend regression test asserting an
/e/...path remains POSIX while retaining the local native-path assertion.
Automated hermes-sweeper review.
| # left untouched so genuine POSIX names (backslashes and all) survive. | ||
| from tools.environments.local import _IS_WINDOWS, _msys_to_windows_path | ||
| if _IS_WINDOWS: | ||
| native = _msys_to_windows_path(path).replace("\\", "/") |
There was a problem hiding this comment.
_IS_WINDOWS describes the host, not the terminal backend. ShellFileOperations also executes in Docker/SSH/Modal/etc.; on a Windows host this rewrites valid remote Linux /e/... paths to E:/.... Please additionally gate this on LocalEnvironment (the existing _lsp_local_only() helper shows that backend-boundary pattern) and add a remote-backend passthrough test.
|
Thanks for the catch on the host-vs-backend boundary. Updated so MSYS lint path translation runs only when the host is Windows and the ops are wired to a LocalEnvironment (same backend check as Also added a remote-backend regression test with a real DockerEnvironment instance (not just a Verification: |
…ousResearch#66494) On Windows/MSYS the post-write shell linters (node --check, tsc, go vet, rustfmt, py_compile) received the file path in Git Bash /e/... form. Git Bash single-quotes the argument, which suppresses MSYS POSIX->Windows conversion, so native node got /e/project/x.js verbatim and resolved it against the current drive root, producing 'Cannot find module E:\e\project\x.js'. Reuse the terminal backend's _msys_to_windows_path translator so the lint hands node the same Windows-native path the terminal already uses for its cwd. No-op off Windows. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ShellFileOperations is shared by local and remote backends. Translating /e/... paths whenever the host is Windows rewrote valid in-backend POSIX paths for Docker/SSH. Restrict the conversion to LocalEnvironment on Windows, matching the _lsp_local_only backend boundary, and cover the remote passthrough with a regression test.
b06d97c to
00245be
Compare
What does this PR do?
On Windows/MSYS the post-write shell linters (
node --check,tsc,go vet,rustfmt,py_compile) received the target file path in Git Bash/e/...form. Git Bash single-quotes the{file}argument, which suppresses MSYS POSIX to Windows argument conversion, so nativenodegot/e/project/x.jsverbatim and resolved it against the current drive root, producingCannot find module 'E:\project\Fina\js\models.js'on every.jsedit.The fix reuses the terminal backend's existing
_msys_to_windows_pathtranslator, so the automatic lint hands the native toolchain the same Windows-native path the terminal already uses for its cwd. This is exactly the "use the same path-resolution engine as the terminal" approach the issue suggests. Off Windows the path is left untouched, so genuine POSIX names (backslashes and all) still round-trip through the existing shell escaper.Related Issue
Fixes #66494
Type of Change
Changes Made
tools/file_operations.py: inShellFileOperations._check_lint, translate the file argument via_msys_to_windows_path(then forward-slash form, single-quoted) when_IS_WINDOWS, before substituting into the linter command. Off Windows keeps the previous_escape_shell_argpath.tests/tools/test_file_operations_edge_cases.py: addTestCheckLintWindowsNativePathcovering both the Windows translation (/e/project/...->'E:/project/...', and the raw MSYS path must NOT appear) and the off-Windows passthrough.How to Test
nodeon PATH, edit a.jsfile throughwrite_file/patchin a project on a non-C: drive (e.g.E:\). Before this change the auto-lint reportsCannot find module 'E:\e\...'; after it,node --checkruns against the correctE:/...path.python -m pytest tests/tools/test_file_operations_edge_cases.py -qVerification output on this branch:
Checklist
Code
fix(tools):)Documentation & Housekeeping
_IS_WINDOWS; POSIX behavior is unchanged.Disclosure: this change was prepared with AI assistance (Claude) and reviewed before submission.