Skip to content

fix: resolve POSIX path handling and test failures on Windows - #3792

Open
Piyush0049 wants to merge 6 commits into
docker:mainfrom
Piyush0049:fix/windows-compatibility-fixes
Open

fix: resolve POSIX path handling and test failures on Windows#3792
Piyush0049 wants to merge 6 commits into
docker:mainfrom
Piyush0049:fix/windows-compatibility-fixes

Conversation

@Piyush0049

Copy link
Copy Markdown
Contributor

Summary

This pull request resolves several path resolution bugs and unit test failures on Windows environments.

Proposed Changes

1. pkg/tools/builtin/filesystem/postedit.go

  • Updated runPostEditCommands to match path-scoped glob patterns (e.g. pkg/*.go) against full relative paths instead of stripping directory names with filepath.Base().

2. pkg/session/session.go

  • Updated AddAttachedFile to 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.go

  • Updated workingdir.Resolve() to treat leading / as absolute paths across OS boundaries. This prevents paths like /tmp/app from being joined to local host working directories on Windows.
  • Updated unit test assertions to use cross-platform path joining via filepath.Join().

4. pkg/tools/builtin/sessionplan/sessionplan_test.go

  • Updated TestPath assertions to use filepath.Join() for platform path separator compatibility.

5. pkg/tui/internal/editorname/editorname_test.go

  • Updated TestFromEnv to dynamically match platform default editor names (Notepad on Windows, Vi elsewhere).

6. pkg/session/sqlitestore/sqlitestore_test.go

  • Added Windows skip guard for TestNew_DirectoryNotWritable because 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/sqlitestore
  • go test ./pkg/tools/workingdir
  • go test ./pkg/tools/builtin/sessionplan
  • go test ./pkg/tui/internal/editorname

@Piyush0049
Piyush0049 requested a review from a team as a code owner July 22, 2026 18:35
@aheritier aheritier added area/sessions For features/issues/fixes related to session lifecycle (resume, persistence, export) area/tools For features/issues/fixes related to the usage of built-in and MCP tools area/tui For features/issues/fixes related to the TUI kind/fix PR fixes a bug (maps to fix:). Use on PRs only. labels Jul 22, 2026
@aheritier
aheritier requested a review from docker-agent July 22, 2026 20:18

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🔴 CRITICAL

This PR introduces two bugs in its path-handling fixes:

  1. 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.

  2. workingdir.go — Removing from the function removes a safety net that guaranteed an absolute path result when is relative.

Comment thread pkg/tools/builtin/filesystem/postedit.go Outdated
Comment thread pkg/tools/workingdir/workingdir.go
@Piyush0049
Piyush0049 force-pushed the fix/windows-compatibility-fixes branch from cc80abb to 9e1b3df Compare July 23, 2026 06:40
@Piyush0049

Copy link
Copy Markdown
Contributor Author

Actually two of my commits were not verified. I fixed them. Please do let me know if any other changes are required.

@Piyush0049

Copy link
Copy Markdown
Contributor Author

I have pushed a commit to resolve the merge conflicts. Please do let me know if any other changes are required.

@Sayt-0 Sayt-0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/hosts with t.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:

  1. Revert the two production changes and make the test fixtures platform-absolute instead (details inline).
  2. Add postedit_test.go covering the new matching behavior (details inline).
  3. Rebase onto main so go.mod/go.sum and the chore: accept/reset go.mod commits 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.

Comment thread pkg/session/session.go Outdated
Comment thread pkg/tools/workingdir/workingdir.go Outdated
Comment thread pkg/tools/workingdir/workingdir_test.go Outdated
Comment thread pkg/tools/workingdir/workingdir_test.go Outdated
Comment thread pkg/tools/builtin/filesystem/postedit.go Outdated
Comment thread pkg/selfupdate/exec_windows.go Outdated
@Piyush0049
Piyush0049 force-pushed the fix/windows-compatibility-fixes branch from d97beb4 to ceae046 Compare July 24, 2026 14:43
@Piyush0049

Copy link
Copy Markdown
Contributor Author

@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.

@Piyush0049

Piyush0049 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

@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.

@aheritier

Copy link
Copy Markdown
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions For features/issues/fixes related to session lifecycle (resume, persistence, export) area/tools For features/issues/fixes related to the usage of built-in and MCP tools area/tui For features/issues/fixes related to the TUI kind/fix PR fixes a bug (maps to fix:). Use on PRs only.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants