fix: resolve POSIX path handling and test failures on Windows - #3792
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 |
|
👋 This PR has merge conflicts with the base branch. Please rebase or merge the latest base branch and resolve them. I've moved it to draft and added |
91e71e5 to
15af87f
Compare
|
@aheritier I have rebased and resolved the merge conflict in editorname_test.go. |
15af87f to
341f188
Compare
|
The native Windows job now passes, confirming that the branch is compatible with the Windows suite introduced by #3866. Most of the original Windows fixes have since been absorbed or superseded by #3866. The remaining functional change is the path-scoped Since the implementation normalizes paths to |
fe7584a to
5e801c1
Compare
|
@Sayt-0 I have pushed a commit addressing the requests. 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 path-scoped matching logic is correct, backward compatible for base-name patterns, and the test coverage addresses the previous review requests (including the pkg/*.go vs pkg/sub/file.go regression case). One blocking issue remains: the js stub was not updated with the new signature, which breaks the wasm build.
| area | issue | severity |
|---|---|---|
pkg/tools/builtin/filesystem/postedit_js.go |
stub signature not updated, GOOS=js build fails |
blocking |
docs/tools/filesystem/index.md |
doc advertises src/**/*.ts but path.Match has no recursive ** |
non-blocking |
pkg/tools/builtin/filesystem/postedit.go |
filepath.Rel recomputed per pattern |
nit |
Taskfile.yml (check-plan-cross) |
js/wasm cross-compile does not cover the filesystem package, so this class of break is invisible to CI | follow-up suggestion |
On the CI blind spot: check-plan-cross could add the filesystem package to the js/wasm build line:
- GOOS=js GOARCH=wasm go build ./pkg/tools/builtin/plan/ ./pkg/plans/ ./pkg/tools/builtin/filesystem/Fine as a follow-up PR.
|
|
||
| // runPostEditCommands executes configured shell commands after a file edit. | ||
| func runPostEditCommands(ctx context.Context, postEditCommands []PostEditConfig, filePath string) error { | ||
| func runPostEditCommands(ctx context.Context, workingDir string, postEditCommands []PostEditConfig, filePath string) error { |
There was a problem hiding this comment.
The //go:build js stub in postedit_js.go still has the old signature, so the wasm entry point documented in cmd/wasm/README.md no longer compiles:
$ GOOS=js GOARCH=wasm go build ./cmd/wasm
pkg/tools/builtin/filesystem/filesystem.go:592:68: too many arguments in call to runPostEditCommands
have ("context".Context, string, []PostEditConfig, string)
want ("context".Context, []PostEditConfig, string)
CI does not catch this because check-plan-cross only cross-compiles ./pkg/tools/builtin/plan/ and ./pkg/plans/ for js/wasm. Suggested fix in postedit_js.go:
// runPostEditCommands is a no-op under js/wasm (no os/exec available).
func runPostEditCommands(_ context.Context, _ string, _ []PostEditConfig, _ string) error {
return nil
}| target = filepath.ToSlash(filePath) | ||
| } | ||
| } | ||
| matched, err := path.Match(pattern, target) |
There was a problem hiding this comment.
Non-blocking: path.Match treats ** as a single-segment wildcard, while docs/tools/filesystem/index.md advertises src/**/*.ts as an example pattern:
| pattern | target | match |
|---|---|---|
src/**/*.ts |
src/a/foo.ts |
yes |
src/**/*.ts |
src/a/b/foo.ts |
no |
Not a regression (slash patterns never matched at all before this PR), but the doc and the implementation now disagree on **. Two options: adjust the doc example to src/*/*.ts, or adopt a doublestar-capable matcher in a follow-up. Fine to handle outside this PR if preferred.
| target := filepath.Base(filePath) | ||
| if strings.Contains(pattern, "/") { | ||
| if workingDir != "" { | ||
| rel, err := filepath.Rel(workingDir, filePath) |
There was a problem hiding this comment.
Nit: filepath.Rel(workingDir, filePath) does not depend on the pattern, yet it is recomputed for every slash-containing pattern in the loop. The relative target could be computed once in runPostEditCommands and passed to matchPostEdit. Negligible in practice given typical post_edit sizes, so purely optional.
- Fix wasm stub signature in postedit_js.go - Update index.md documentation glob examples - Refactor postedit.go to precompute relative path - Fix upstream tool.Handler signature regression in wasm build - Fix gosec, errorlint, and noctx linting issues
|
@Sayt-0 I have updated the PR. |
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