fix: resolve POSIX path handling and test failures on Windows - #3792
fix: resolve POSIX path handling and test failures on Windows#3792Piyush0049 wants to merge 6 commits into
Conversation
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🔴 CRITICAL
This PR introduces two bugs in its path-handling fixes:
-
postedit.go — Path-scoped glob patterns (containing ) will never match because the code compares a relative pattern against an absolute . Post-edit commands configured with patterns like will silently never execute.
-
workingdir.go — Removing from the function removes a safety net that guaranteed an absolute path result when is relative.
cc80abb to
9e1b3df
Compare
|
Actually two of my commits were not verified. I fixed them. Please do let me know if any other changes are required. |
|
I have pushed a commit to resolve the merge conflicts. Please do let me know if any other changes are required. |
Sayt-0
left a comment
There was a problem hiding this comment.
Review summary
The postedit.go change fixes a real cross-platform bug and most test fixes are sound. However, two production changes look test-driven and weaken documented path contracts on Windows, and the main behavioral fix ships without tests.
| area | issue | severity |
|---|---|---|
pkg/session/session.go |
accepts drive-relative paths on Windows, contradicts the doc comment above it | high |
pkg/tools/workingdir/workingdir.go |
rooted non-absolute cwd passed through to spawned MCP/LSP processes, non-deterministic resolution | high |
pkg/tools/builtin/filesystem/postedit.go |
new matching logic has no test coverage; silent fallback when filepath.Rel fails |
medium |
go.mod / go.sum |
identical to main; diff noise from manual sync commits | low |
pkg/selfupdate/exec_windows.go |
nolint justification outdated since Go 1.20 | nit |
What works well:
- postedit path-scoped matching is the right direction and stays backward compatible for base-name patterns.
- Replacing
/etc/hostswitht.TempDir()paths makes those tests more robust on every platform. - The sqlitestore Windows skip, editorname default helper, and sessionplan assertion fix follow standard practice.
Requested changes:
- Revert the two production changes and make the test fixtures platform-absolute instead (details inline).
- Add
postedit_test.gocovering the new matching behavior (details inline). - Rebase onto main so
go.mod/go.sumand thechore: accept/reset go.modcommits drop out of the diff.
Note: CI has no Windows runner, so the Windows-only lint fixes and test results are only verifiable locally. A windows-latest test job would lock in these gains; fine as a follow-up.
d97beb4 to
ceae046
Compare
|
@Sayt-0 I read the review. I have pushed a commit to fix the issues you highlighted. Please do let me know if any other changes are required. |
There was a linting issue from my side. I have resolved it. |
|
If it's not too expensive and slow @Sayt-0 we should setup a windows node to validate our tests. Maybe on PRs only, not sure |
Summary
This pull request resolves several path resolution bugs and unit test failures on Windows environments.
Proposed Changes
1.
pkg/tools/builtin/filesystem/postedit.gorunPostEditCommandsto match path-scoped glob patterns (e.g.pkg/*.go) against full relative paths instead of stripping directory names withfilepath.Base().2.
pkg/session/session.goAddAttachedFileto recognize POSIX absolute paths starting with/on Windows. This prevents file attachments from being dropped and fixes out-of-bounds slice panics in tests.3.
pkg/tools/workingdir/workingdir.go&workingdir_test.goworkingdir.Resolve()to treat leading/as absolute paths across OS boundaries. This prevents paths like/tmp/appfrom being joined to local host working directories on Windows.filepath.Join().4.
pkg/tools/builtin/sessionplan/sessionplan_test.goTestPathassertions to usefilepath.Join()for platform path separator compatibility.5.
pkg/tui/internal/editorname/editorname_test.goTestFromEnvto dynamically match platform default editor names (Notepadon Windows,Vielsewhere).6.
pkg/session/sqlitestore/sqlitestore_test.goTestNew_DirectoryNotWritablebecause POSIX permission bits (0o555) do not prevent directory writes on Windows NTFS.Verification
All affected test suites have been verified and pass cleanly:
go test ./pkg/session/...go test ./pkg/session/sqlitestorego test ./pkg/tools/workingdirgo test ./pkg/tools/builtin/sessionplango test ./pkg/tui/internal/editorname