fix(tui): resolve cross-platform root detection and fallback paths in pickers by Piyush0049 · Pull Request #3795 · docker/docker-agent

Piyush0049 · 2026-07-23T08:29:16Z

Summary

This PR addresses two cross-platform path handling issues in the TUI file and directory picker components to ensure reliable behavior on Windows and POSIX systems.

1. Cross-Platform Root Directory Boundary Detection

Previous behavior: The TUI checked d.currentDir != "/" to decide whether to append the .. (parent directory) option.
Issue: On Windows, root directories start with drive letters (such as C:\). Since C:\ does not equal /, the TUI treated C:\ as a non-root folder and appended a .. entry pointing back to C:\, causing an infinite navigation loop.
Fix: Replaced hardcoded POSIX root checks with filepath.Dir(d.currentDir) != d.currentDir. This leverages the standard library to verify if a directory is its own parent, which works reliably on Windows, Linux, and macOS.

2. Cross-Platform Directory Fallback (`/` to `.`)

Previous behavior: When os.Getwd() returned an error or empty string, the TUI fell back to workingDir = "/".
Issue: Hardcoding / as a fallback causes invalid path resolutions on Windows systems when execution context is lost.
Fix: Updated the fallback path from / to .. This aligns with the existing fallback pattern used in file_picker.go and ensures safe resolution relative to the process execution directory across all platforms.

Verification

Verified with golangci-lint run (0 issues).
Verified unit test suite with go test ./pkg/tui/dialog.

… pickers

aheritier · 2026-07-26T08:58:43Z

In relation to #3792 we should maybe have some cross platform integration tests

Piyush0049 · 2026-07-26T11:11:23Z

In relation to #3792 we should maybe have some cross platform integration tests

Adding these tests would be a good move.

aheritier · 2026-08-04T07:05:43Z

@Sayt-0 can you verify if the windows support related fixes are addressing these problems too

Sayt-0 · 2026-08-04T10:06:54Z

Checked against the Windows changes merged in #3866.

#3866 adds a blocking native Windows test job, but it does not include the picker root detection and fallback changes from this PR. main still uses currentDir != "/" and / fallbacks, so #3795 remains relevant.

The current Windows suite does not exercise a drive root such as C:\ or the os.Getwd() fallback, so a green Windows job does not cover this regression yet. Please rebase onto main and add targeted regression tests for these cases @Piyush0049 .

Add regression tests for root directory fallback and empty initial working directory as requested by maintainer. Also fix unrelated failing tests on Windows due to slash normalization, and resolve gosec, errorlint, and forbidigo lint issues.

…r-paths # Conflicts: # pkg/tui/dialog/session_browser_test.go # pkg/tui/dialog/skills_test.go

Piyush0049 · 2026-08-04T11:51:20Z

@Sayt-0 I have fixed the code in this PR too. There were some merge conflicts too. Please do let me know if anything needs to be changed in this pr.

Sayt-0

The core fix is correct and useful: filepath.Dir(dir) != dir is the right cross-platform root check, and the . fallback matches the existing pattern in file_picker.go. Verified locally: build, go test ./pkg/tui/dialog, GOOS=windows build of the touched packages, and golangci-lint run all pass.

Blocking point: the PR mixes unrelated GOOS=windows lint fixes into the picker fix. Details in the inline comments.

area	status
root detection idiom (`file_picker.go`, `working_dir_picker.go`)	good
`.` fallback (`working_dir_picker.go`, `tui.go`)	good
regression tests	good, two minor remarks
`selfupdate` / `cmd_windows.go` lint changes	out of scope, please split

Sayt-0 · 2026-08-05T08:11:55Z

+				return fmt.Errorf("installing new binary: %w (copy fallback failed: %w; rollback also failed: %w)", err, cpErr, rbErr)
 			}
-			return fmt.Errorf("installing new binary: %w (copy fallback failed: %v)", err, cpErr)
+			return fmt.Errorf("installing new binary: %w (copy fallback failed: %w)", err, cpErr)


These changes are unrelated to the picker fix. CI lint only runs with GOOS=linux (see Taskfile.yml), so _windows.go files are never linted in CI and this is not needed for the PR to pass. %v to %w also changes error-wrapping semantics: errors.Is/As now match cpErr and rbErr. Please move to a separate chore(lint) PR, together with the nolint changes below and in backgroundjobs/cmd_windows.go and shell/cmd_windows.go.

Sayt-0 · 2026-08-05T08:11:55Z

 	}

-	cmd := exec.Command(path, childArgs...) //nolint:gosec // path is our own freshly installed binary
+	cmd := exec.Command(path, childArgs...) //nolint:noctx // path is our own freshly installed binary; no context needed for re-exec


Same scope concern. The swap itself is correct (G204 is globally excluded in .golangci.yml, so the gosec directive was unused and nolintlint flags it under GOOS=windows; noctx is the actual finding), but it belongs in the lint cleanup PR.

Sayt-0 · 2026-08-05T08:11:55Z

+		uintptr(unsafe.Pointer(&info)), //nolint:gosec // Windows API requires unsafe pointer
 		uint32(unsafe.Sizeof(info))); err != nil {
 		_ = windows.CloseHandle(job)
 		return nil, err
 	}

-	handle, err := windows.OpenProcess(windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, false, uint32(proc.Pid))
+	handle, err := windows.OpenProcess(windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, false, uint32(proc.Pid)) //nolint:gosec // Pid is safe to convert to uint32 on Windows


Out of scope for this PR, same as the selfupdate changes. The directives are justified (Windows API requires unsafe.Pointer; PID fits in uint32), but they should move to the lint cleanup PR. Same for the mirrored changes in pkg/tools/builtin/shell/cmd_windows.go.

Sayt-0 · 2026-08-05T08:11:55Z

+func TestWorkingDirPickerEmptyFallback(t *testing.T) {
+	t.Parallel()
+
+	// Pass an empty string for the initial directory.
+	// NewWorkingDirPickerDialog should fall back to os.Getwd().
+	d := NewWorkingDirPickerDialog(t.Context(), nil, nil, nil, "").(*workingDirPickerDialog)
+
+	cwd, err := os.Getwd()
+	require.NoError(t, err)
+
+	require.Equal(t, cwd, d.currentDir, "empty initial directory should fall back to current working directory")
+}


The name suggests coverage of the new cwd = "." fallback, but this test exercises the pre-existing empty-string to os.Getwd() path (the . branch is only reachable when Getwd fails, which is hard to simulate). Suggested rename: TestWorkingDirPickerEmptyInitialDirUsesGetwd. The test is still worth keeping as it covers a previously untested path.

Sayt-0 · 2026-08-05T08:11:55Z

+	for _, e := range d.entries {
+		if e.name == ".." {
+			t.Errorf("root directory should not have a parent dir entry, but got '..'")
+		}
+	}


The manual loop can reuse the existing helper:

assert.NotContains(t, entryNames(d.entries), "..")

…r-paths

- Rename TestWorkingDirPickerEmptyFallback to TestWorkingDirPickerEmptyInitialDirUsesGetwd - Simplify file picker root test to use assert.NotContains

Piyush0049 · 2026-08-05T10:11:26Z

@Sayt-0 I have fixed the PR according to your review.

fix(tui): resolve cross-platform root detection and fallback paths in…

13acdb7

… pickers

Piyush0049 requested a review from a team as a code owner July 23, 2026 08:29

aheritier added 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 23, 2026

Piyush0049 added 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 23, 2026

Piyush0049 added 2 commits August 4, 2026 17:17

Merge remote-tracking branch 'origin/main' into fix/tui-windows-picke…

69e08c4

…r-paths # Conflicts: # pkg/tui/dialog/session_browser_test.go # pkg/tui/dialog/skills_test.go

Sayt-0 requested changes Aug 5, 2026

View reviewed changes

Piyush0049 added 2 commits August 5, 2026 15:22

Merge remote-tracking branch 'origin/main' into fix/tui-windows-picke…

8ac9698

…r-paths

test: address maintainer review comments for picker tests

e18bdf3

- Rename TestWorkingDirPickerEmptyFallback to TestWorkingDirPickerEmptyInitialDirUsesGetwd - Simplify file picker root test to use assert.NotContains

Sayt-0 enabled auto-merge August 5, 2026 12:04

Sayt-0 approved these changes Aug 5, 2026

View reviewed changes

Sayt-0 merged commit d1a5e40 into docker:main Aug 5, 2026
9 checks passed

BrewTestBot mentioned this pull request Aug 5, 2026

docker-agent 1.122.0 Homebrew/homebrew-core#297235

Merged

aheritier mentioned this pull request Aug 6, 2026

docs: auto-update for merged PRs (2026-08-06) #3919

Merged

Navigation Menu

Uh oh!

fix(tui): resolve cross-platform root detection and fallback paths in pickers - #3795

fix(tui): resolve cross-platform root detection and fallback paths in pickers#3795
Sayt-0 merged 5 commits into
docker:mainfrom
Piyush0049:fix/tui-windows-picker-paths

Piyush0049 commented Jul 23, 2026

Uh oh!

aheritier commented Jul 26, 2026

Uh oh!

Piyush0049 commented Jul 26, 2026

Uh oh!

aheritier commented Aug 4, 2026

Uh oh!

Sayt-0 commented Aug 4, 2026 •

edited

Loading

Uh oh!

Piyush0049 commented Aug 4, 2026

Uh oh!

Sayt-0 left a comment

Uh oh!

Sayt-0 Aug 5, 2026

Uh oh!

Sayt-0 Aug 5, 2026

Uh oh!

Sayt-0 Aug 5, 2026

Uh oh!

Sayt-0 Aug 5, 2026

Uh oh!

Sayt-0 Aug 5, 2026

Uh oh!

Piyush0049 commented Aug 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Piyush0049 commented Jul 23, 2026

Summary

1. Cross-Platform Root Directory Boundary Detection

2. Cross-Platform Directory Fallback (/ to .)

Verification

Uh oh!

aheritier commented Jul 26, 2026

Uh oh!

Piyush0049 commented Jul 26, 2026

Uh oh!

aheritier commented Aug 4, 2026

Uh oh!

Sayt-0 commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piyush0049 commented Aug 4, 2026

Uh oh!

Sayt-0 left a comment

Choose a reason for hiding this comment

Uh oh!

Sayt-0 Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

Sayt-0 Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

Sayt-0 Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

Sayt-0 Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

Sayt-0 Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

Piyush0049 commented Aug 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

2. Cross-Platform Directory Fallback (`/` to `.`)

Sayt-0 commented Aug 4, 2026 •

edited

Loading