Add Windows support for Git Bash and PowerShell#114
Open
Fronix wants to merge 13 commits intosatococoa:mainfrom
Open
Add Windows support for Git Bash and PowerShell#114Fronix wants to merge 13 commits intosatococoa:mainfrom
Fronix wants to merge 13 commits intosatococoa:mainfrom
Conversation
- Add Windows (amd64/arm64) to CI workflow and GoReleaser config - Fix shell detection and hook scripts for Git Bash and PowerShell - Fix path separator handling in worktree management (git returns forward slashes on Windows, Go uses backslashes) - Skip tests with hardcoded Unix paths on Windows 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Fix variable shadowing in EvalSymlinks calls and line length violation.
PowerShell splits -coverprofile=coverage.out into separate args.
Use filepath.Join, platform-aware commands, and .exe binary extension.
Use cross-platform command in shell executor test and add windows-latest to e2e matrix with bash shell default.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Windows/PowerShell support across CI, release packaging, docs, shell integration, completion patching, path normalization, and many tests; enables Windows builds and normalizes CRLF/LF and OS-specific path/command behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Shell as Shell (pwsh / bash)
participant Profile as Hook function (profile)
participant WTP as wtp executable
participant FS as Filesystem / Git
Shell->>Profile: user invokes wtp or sources hook
Profile->>WTP: resolve executable (wtp.exe → wtp → ./wtp)
Shell->>Profile: user runs "wtp cd <worktree>"
Profile->>WTP: call `wtp cd <worktree>`
WTP->>Profile: returns path / status
Profile->>Shell: on success -> Set-Location (pwsh) / cd (bash)
Shell->>Profile: user runs "wtp add ..."
Profile->>WTP: forward args (honor help/redirected stdout)
WTP->>Profile: returns output
Profile->>Shell: when output includes target dir -> change dir if appropriate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
cmd/wtp/testhelpers_test.go (1)
57-71: Input paths remain Unix-style while expected values use OS-specific separators.The
expectedvalues correctly usefilepath.Joinfor cross-platform compatibility, but the input paths (e.g.,"/path/to/repo/.worktrees/feature/test") remain Unix-style with forward slashes. On Windows, these paths may not be recognized as absolute paths or may behave inconsistently.If the function under test (
fn) is expected to work with real filesystem paths, consider also constructing inputs withfilepath.Joinor documenting that the function handles both path formats.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/wtp/testhelpers_test.go` around lines 57 - 71, The test uses hard-coded Unix-style paths as inputs which can fail on Windows; update the two t.Run cases that call fn (the non-main and outside base_dir cases) to build input paths using filepath.Join or convert with filepath.FromSlash so inputs are OS-native (e.g., replace "/path/to/repo/.worktrees/feature/test" and "/completely/different/path" with filepath.Join or filepath.FromSlash variants) so fn is exercised with platform-correct absolute paths while keeping the expected values using filepath.Join.README.md (1)
125-146: Consider clarifying PATH persistence for PowerShell.Line 137-138 mentions "add this line to your PowerShell profile for persistence" but only shows the session-level
$env:PATHmodification. Users may not know how to make this permanent.📝 Suggested clarification
# Add to PATH (add this line to your PowerShell profile for persistence) $env:PATH += ";$env:LOCALAPPDATA\wtp" + +# To persist the PATH change, add the above line to your PowerShell profile: +# notepad $PROFILE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 125 - 146, Clarify the PowerShell persistence step by explaining how to make the $env:PATH change permanent: instruct the user to open their PowerShell profile (referencing $PROFILE) and append a line that adds the install directory to PATH, or alternatively use the user-level environment setter ([System.Environment]::SetEnvironmentVariable) to persistently add "$env:LOCALAPPDATA\wtp" to the PATH; mention which approach updates only the current session (the existing $env:PATH example) versus persistent changes so readers know to edit their PowerShell profile or use the environment API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/wtp/completion_config.go`:
- Around line 259-271: Add a unit test for patchPowerShellCompletionScript that
verifies the replacement actually occurs (similar to
TestPatchCompletionScriptBashSanitizesDescriptions and
TestPatchCompletionScriptZshInjectsCompletionEnv): create an input PowerShell
completion script containing the original target snippet ("$fn =
$($MyInvocation.MyCommand.Name)..." / Register-ArgumentCompleter -CommandName
$name ...) and assert the output contains "Register-ArgumentCompleter -Native
-CommandName 'wtp' -ScriptBlock {"; optionally add a golden file
pwsh_expected.ps1 and a golden-file test to ensure future changes in urfave/cli
that alter the source format will fail the test. Ensure the test calls
patchPowerShellCompletionScript and checks the replaced string (or compares to
the golden file) so the patching cannot silently pass through unchanged.
In `@cmd/wtp/hook.go`:
- Around line 244-297: The PowerShell hook generated by printPowerShellHook is
missing the `wtp add` auto-cd behavior; update the function printPowerShellHook
to add an elseif branch for when $args[0] -eq "add" that mirrors other shells:
forward help flags to the real executable, detect interactive output with
[Console]::IsOutputRedirected (or its negation) to decide quiet invocation, call
& $__wtpPath `@args` --quiet capturing $targetDir and $LASTEXITCODE, and if exit
code is 0 and $targetDir is set call Set-Location $targetDir; otherwise invoke &
$__wtpPath `@args` normally. Ensure you reference $__wtpPath and preserve existing
completion and cd handling.
- Around line 283-293: The PowerShell handler currently errors when `wtp cd` has
no argument; make it match other shells by treating missing $args[1] as a
request for the main worktree. Change the block around $__wtpPath cd so that
when (-not $args[1]) you call `$targetDir = & $__wtpPath cd 2>$null` (instead of
Write-Error/return), otherwise call `$targetDir = & $__wtpPath cd $args[1]
2>$null`; then preserve the existing $LASTEXITCODE check and
Set-Location/$__wtpPath fallback behavior (referencing $__wtpPath and
$targetDir).
In `@cmd/wtp/list_test.go`:
- Around line 681-683: Replace the unconditional Windows skip around the
runtime.GOOS == osWindows / t.Skip call in cmd/wtp/list_test.go with OS-agnostic
test logic: remove the skip and normalize path handling in the affected tests by
using filepath.FromSlash/filepath.Join (or construct expected outputs using
os.PathSeparator) so the list-output and quiet-mode assertions compare
normalized paths on Windows and Unix; update the assertions in the same test(s)
that reference Unix-specific paths to generate their expected strings via
filepath utilities instead of hardcoded "/" separators (leave a conditional skip
only if a real platform limitation remains, otherwise remove it).
In `@internal/command/executor_test.go`:
- Around line 324-334: The test currently only asserts non-empty output so it
won't detect if workDir is ignored; change it to run a directory-sensitive
command and assert the output matches tmpDir: call shell.Execute with a
platform-specific command (use runtime.GOOS to pick "cmd /C cd" on windows and
"pwd" on unix-like systems) using tmpDir as the working directory, then assert
no error and that the trimmed output equals or contains the tmpDir path
(reference the tmpDir variable and the shell.Execute call).
---
Nitpick comments:
In `@cmd/wtp/testhelpers_test.go`:
- Around line 57-71: The test uses hard-coded Unix-style paths as inputs which
can fail on Windows; update the two t.Run cases that call fn (the non-main and
outside base_dir cases) to build input paths using filepath.Join or convert with
filepath.FromSlash so inputs are OS-native (e.g., replace
"/path/to/repo/.worktrees/feature/test" and "/completely/different/path" with
filepath.Join or filepath.FromSlash variants) so fn is exercised with
platform-correct absolute paths while keeping the expected values using
filepath.Join.
In `@README.md`:
- Around line 125-146: Clarify the PowerShell persistence step by explaining how
to make the $env:PATH change permanent: instruct the user to open their
PowerShell profile (referencing $PROFILE) and append a line that adds the
install directory to PATH, or alternatively use the user-level environment
setter ([System.Environment]::SetEnvironmentVariable) to persistently add
"$env:LOCALAPPDATA\wtp" to the PATH; mention which approach updates only the
current session (the existing $env:PATH example) versus persistent changes so
readers know to edit their PowerShell profile or use the environment API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47afe791-6539-43ab-9f68-f3dd8a0f80e0
📒 Files selected for processing (23)
.github/workflows/ci.yml.github/workflows/e2e-test.yml.goreleaser.ymlREADME.mdcmd/wtp/add.gocmd/wtp/add_test.gocmd/wtp/cd_test.gocmd/wtp/completion_config.gocmd/wtp/completion_config_test.gocmd/wtp/hook.gocmd/wtp/init_test.gocmd/wtp/list_test.gocmd/wtp/shell_init.gocmd/wtp/testhelpers_test.gocmd/wtp/worktree_managed.gointernal/command/executor_test.gointernal/config/config_test.gointernal/git/repository_test.gointernal/git/worktree_test.gotest/e2e/framework/assertions.gotest/e2e/framework/framework.gotest/e2e/hook_streaming_test.gotest/e2e/worktree_test.go
Add PowerShell completion patching test, remove Windows skips in list tests with cross-platform assertions, and assert working directory in executor test.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/wtp/hook.go`:
- Around line 270-279: The wrapper currently emits numeric return values into
the PowerShell pipeline (e.g. in the error branch checking -not $__wtpPath and
in the completion/dispatch paths inside foreach ($arg in $args) where it uses
"return $LASTEXITCODE" or numeric returns); replace any "return 0"/"return
1"/"return $LASTEXITCODE" with a bare "return" after native invocations to avoid
writing numbers to the pipeline, and for the error case when $__wtpPath is
missing set $global:LASTEXITCODE = 1 before doing the bare return; update all
occurrences around $__wtpPath, the foreach ($arg in $args) completion branch,
and any other wrapper exit points accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b18cf40c-dd5f-44a3-ae46-fbde399bfe26
📒 Files selected for processing (4)
cmd/wtp/completion_config_test.gocmd/wtp/hook.gocmd/wtp/list_test.gointernal/command/executor_test.go
✅ Files skipped from review due to trivial changes (2)
- cmd/wtp/list_test.go
- internal/command/executor_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/wtp/completion_config_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Builds on #61 by @thoraxe — adds comprehensive Windows support with Git Bash and PowerShell shell integration, and fixes all CI failures on Windows.
Original changes (from #61)
Additional fixes
"windows"stringshell: bashin CI test steps to prevent PowerShell arg splittingSummary by CodeRabbit
New Features
Documentation
Improvements
Tests