cmd/wtp/add.go (2)

346-394: Generalize remote handling and HEAD filtering in getBranches.

Right now we:

• Trim only origin/ but not other remotes.
• Skip only origin/HEAD (misses upstream/HEAD, etc.).
• Don’t guard against bare remote names (e.g., just origin).

This can yield noisy/duplicated suggestions and misses common setups with multiple remotes. Recommend:

• Query remotes via git remote (best-effort).
• Skip HEAD for any remote and bare remote names.
• Strip the <remote>/ prefix for any known remote to dedupe local vs remote.

Proposed patch (localized to this function):

 func getBranches(w io.Writer) error {
   // Get current directory
   cwd, err := os.Getwd()
   if err != nil {
     return err
   }

+  // Best-effort: gather declared remotes to normalize names
+  remCmd := exec.Command("git", "remote")
+  remCmd.Dir = cwd
+  remOut, _ := remCmd.Output() // ignore error; completion should be resilient
+  remotes := strings.Fields(strings.TrimSpace(string(remOut)))
+
   // Get all branches using git for-each-ref for better control
   gitCmd := exec.Command("git", "for-each-ref", "--format=%(refname:short)", "refs/heads", "refs/remotes")
   gitCmd.Dir = cwd
   output, err := gitCmd.Output()
   if err != nil {
     return err
   }

   branches := strings.Split(strings.TrimSpace(string(output)), "\n")

   // Use a map to avoid duplicates
   seen := make(map[string]bool)

   for _, branch := range branches {
     if branch == "" {
       continue
     }

-    // Skip HEAD references and bare origin
-    if branch == "origin/HEAD" || branch == "origin" {
-      continue
-    }
+    // Skip HEAD references (local or any remote) and bare remote names
+    if branch == "HEAD" || strings.HasSuffix(branch, "/HEAD") {
+      continue
+    }
+    isBareRemote := false
+    for _, r := range remotes {
+      if branch == r {
+        isBareRemote = true
+        break
+      }
+    }
+    if isBareRemote {
+      continue
+    }

     // Remove remote prefix for display, but keep track of what we've seen
     displayName := branch
-    if strings.HasPrefix(branch, "origin/") {
-      // For remote branches, show without the origin/ prefix
-      displayName = strings.TrimPrefix(branch, "origin/")
-    }
+    for _, r := range remotes {
+      prefix := r + "/"
+      if strings.HasPrefix(branch, prefix) {
+        // For remote branches, show without the <remote>/ prefix
+        displayName = strings.TrimPrefix(branch, prefix)
+        break
+      }
+    }

     // Skip if already seen (handles case where local and remote have same name)
     if seen[displayName] {
       continue
     }

     seen[displayName] = true
     fmt.Fprintln(w, displayName)
   }

   return nil
 }

---

346-394: Optional: add timeouts to git calls to avoid hung completions on large repos.

Use exec.CommandContext with a short context deadline (e.g., 200–500ms) so completion never blocks the user. If the deadline hits, just emit nothing.

cmd/wtp/add_test.go (1)

968-982: Broaden assertions to catch regressions (optional).

Consider asserting that the function writes zero or more lines and never includes origin/HEAD or bare remote names once the getBranches change is applied. This makes the completion contract explicit.

CLAUDE.md (1)

251-314: Minor copyedits for clarity (optional).

A few bullets use stacked bolds and hyphens that read oddly (“No side effects: …”). Consider consistent bullet structure and parallel phrasing. Not blocking.

cmd/wtp/cd.go (2)

271-294: Avoid duplicate naming logic across commands (optional).

getWorktreeNameFromPathCd mirrors the naming logic used by add success messaging (getWorktreeNameFromPath). Consider centralizing in a small shared helper (e.g., internal/naming) so all commands and errors use the same single source of truth.

---

113-167: Nit: compute mainRepoPath once.

findMainWorktreePath(worktrees) is called twice. Cache it to avoid recomputation and clarify intent.

test/e2e/error_test.go (1)

152-165: Strengthen assertion: verify absolute path semantics

Since wtp cd now guarantees an absolute path, assert that the output is absolute. This prevents regressions to relative paths.

@@
-import (
-	"strings"
-	"testing"
-
-	"github.com/satococoa/wtp/test/e2e/framework"
-)
+import (
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/satococoa/wtp/test/e2e/framework"
+)
@@
-		// Output should be a valid path
-		outputPath := strings.TrimSpace(output)
-		framework.AssertTrue(t, strings.Contains(outputPath, "test-branch"), "Should contain worktree name")
+		// Output should be an absolute path and contain the worktree name
+		outputPath := strings.TrimSpace(output)
+		framework.AssertTrue(t, filepath.IsAbs(outputPath), "Path should be absolute: %s", outputPath)
+		framework.AssertTrue(t, strings.Contains(outputPath, "test-branch"), "Should contain worktree name")

cmd/wtp/remove_test.go (4)

853-864: Rename misleading subtest title; it doesn’t test a fallback-on-error path.

filepath.Rel rarely errors on POSIX; this case asserts a long relative path, not a “fallback to directory name”. Rename for clarity.

- t.Run("should fallback to directory name on error", func(t *testing.T) {
+ t.Run("should return relative path when outside base_dir", func(t *testing.T) {
@@
-   // Use invalid path to trigger error
-   name := getWorktreeNameFromPath("/completely/different/path", cfg, "/path/to/repo", false)
-   assert.Equal(t, "../../../../completely/different/path", name) // actual filepath.Rel result
+   // Path outside base_dir should yield a relative path from base_dir
+   name := getWorktreeNameFromPath("/completely/different/path", cfg, "/path/to/repo", false)
+   assert.Equal(t, "../../../../completely/different/path", name)

---

830-830: Format the file (gofmt).

CI reports “File is not properly formatted (gofmt)”. Please run gofmt/goimports.

---

901-915: Prefer writer-based completion capture to avoid stdout redirection in tests.

The test silences os.Stdout to avoid noise. If completeWorktrees writes to cmd.Root().Writer (as other commands do), this redirection is unnecessary. See suggested change in remove.go comment for completeWorktrees.

---

830-864: Extract and Centralize getWorktreeNameFromPath Tests

Ripgrep confirms that the same three scenarios for getWorktreeNameFromPath are tested in two places:

• cmd/wtp/remove_test.go (lines 830–864)
• cmd/wtp/cd_test.go (lines 221–255)

To reduce duplication and maintenance overhead:

• Create a new test file cmd/wtp/get_worktree_name_test.go containing a helper function, e.g.:

package wtp

import (
    "path/filepath"
    "testing"

    "github.com/stretchr/testify/assert"
    "yourmodule/config"
)

func testGetWorktreeNameFromPath(t *testing.T) {
    cfg := &config.Config{ Defaults: config.Defaults{ BaseDir: ".worktrees" } }
    mainRepo := "/path/to/repo"

    t.Run("main worktree → @", func(t *testing.T) {
        name := getWorktreeNameFromPath(mainRepo, cfg, mainRepo, true)
        assert.Equal(t, "@", name)
    })
    t.Run("non-main worktree → relative path", func(t *testing.T) {
        p := filepath.Join(mainRepo, ".worktrees/feature/test")
        name := getWorktreeNameFromPath(p, cfg, mainRepo, false)
        assert.Equal(t, "feature/test", name)
    })
    t.Run("fallback on error → raw filepath.Rel", func(t *testing.T) {
        p := "/completely/different/path"
        rel, _ := filepath.Rel(mainRepo, p)
        name := getWorktreeNameFromPath(p, cfg, mainRepo, false)
        assert.Equal(t, rel, name)
    })
}

• In both remove_test.go and cd_test.go, replace their inline blocks with:

func TestGetWorktreeNameFromPath(t *testing.T) {
    testGetWorktreeNameFromPath(t)
}

• Delete the duplicated T.Run cases from lines 830–864 in remove_test.go and 221–255 in cd_test.go.

This centralization ensures any future change to the naming logic only needs to be updated in one place.

cmd/wtp/remove.go (4)

281-304: Unify base_dir resolution and normalize paths.

Use the same resolver used elsewhere (cfg.ResolveWorktreePath) and normalize inputs. This avoids subtle separator issues and keeps behavior consistent across commands.

 func getWorktreeNameFromPath(worktreePath string, cfg *config.Config, mainRepoPath string, isMain bool) string {
   if isMain {
     return "@"
   }

-  // Get base_dir path
-  baseDir := cfg.Defaults.BaseDir
-  if !filepath.IsAbs(baseDir) {
-    baseDir = filepath.Join(mainRepoPath, baseDir)
-  }
+  // Resolve and normalize base_dir consistently
+  baseDir := cfg.ResolveWorktreePath(mainRepoPath, "")
+  baseDir = filepath.Clean(baseDir)
+  worktreePath = filepath.Clean(worktreePath)

   // Calculate relative path from base_dir
   relPath, err := filepath.Rel(baseDir, worktreePath)
   if err != nil {
     // Fallback to directory name
     return filepath.Base(worktreePath)
   }

   return relPath
 }

---

41-44: Normalize with filepath.Clean instead of TrimSuffix.

TrimSuffix("/") won’t handle Windows separators and can leave redundant components. filepath.Clean is portable and reliable.

- // Remove trailing slash if it exists
- baseDir = strings.TrimSuffix(baseDir, "/")
+ // Normalize path
+ baseDir = filepath.Clean(baseDir)

---

350-362: Write completions to the CLI writer for consistency and easier testing.

urfave/cli’s ShellComplete prints to stdout, but using Root().Writer with a fallback preserves that behavior and keeps tests simpler.

-func completeWorktrees(_ context.Context, _ *cli.Command) {
+func completeWorktrees(_ context.Context, cmd *cli.Command) {
   var buf bytes.Buffer
   if err := getWorktreesForRemove(&buf); err != nil {
     return
   }

-  // Output each line using fmt.Println for urfave/cli compatibility
-  scanner := bufio.NewScanner(&buf)
+  w := cmd.Root().Writer
+  if w == nil {
+    w = os.Stdout
+  }
+  // Output each line using fmt.Fprintln for urfave/cli compatibility
+  scanner := bufio.NewScanner(&buf)
   for scanner.Scan() {
-    fmt.Println(scanner.Text())
+    fmt.Fprintln(w, scanner.Text())
   }
 }

---

214-279: Matching priority and error surfaces make sense; consider stable ordering in “available worktrees.”

The matching order (branch, basename, relative name) is reasonable. If error messages include availableWorktrees, consider sorting them for deterministic output (low priority).

cmd/wtp/hook_test.go (1)

102-146: Add zsh edge-case coverage to match bash/fish parity.

Edge-case handling is verified for bash and fish; include zsh to keep parity.

   tests := []struct {
     name          string
     shell         string
     requiredLogic []string
   }{
@@
     {
       name:  "fish hook handles empty argument",
       shell: "fish",
       requiredLogic: []string{
         "if test -z \"$argv[2]\"",
         "echo \"Usage:",
         "return 1",
       },
     },
+    {
+      name:  "zsh hook handles empty argument",
+      shell: "zsh",
+      requiredLogic: []string{
+        "[[ -z \"$2\" ]]", // zsh supports [[ ... ]]
+        "echo \"Usage:",
+        "return 1",
+      },
+    },
   }
@@
   switch tt.shell {
   case "bash":
     printBashHook(&buf)
   case "fish":
     printFishHook(&buf)
+  case "zsh":
+    printZshHook(&buf)
   }

cmd/wtp/shell_init.go (3)

101-119: Consider making completion execution overridable for tests.

exec.Command spawns the binary; allowing an overridable runner simplifies unit tests for shell-init without forking a process.

+// test seam
+var runCompletion = outputCompletion
@@
- if err := outputCompletion(w, "bash"); err != nil {
+ if err := runCompletion(w, "bash"); err != nil {
@@
- if err := outputCompletion(w, "zsh"); err != nil {
+ if err := runCompletion(w, "zsh"); err != nil {
@@
- if err := outputCompletion(w, "fish"); err != nil {
+ if err := runCompletion(w, "fish"); err != nil {

---

119-119: Format the file (gofmt).

CI reports formatting issues. Please run gofmt/goimports.

---

47-99: Add unit tests for “shell-init ” to assert combined output.

Recommend tests asserting that:

• completion content is present (e.g., bash “complete -F …”)
• hook content is present (e.g., “wtp()” and “cd "$target_dir"”)
• a separating newline exists between the two blocks

I can draft cmd/wtp/shell_init_test.go table tests mirroring hook_test.go if helpful.

test/e2e/shell_test.go (4)

15-28: CD now outputs a path — assert it’s absolute.

Strengthen the assertion to ensure the new contract is upheld.

   // Output should be a valid path
   outputPath := strings.TrimSpace(output)
-  framework.AssertTrue(t, strings.Contains(outputPath, "test-branch"), "Should contain worktree name")
+  framework.AssertTrue(t, strings.Contains(outputPath, "test-branch"), "Should contain worktree name")
+  framework.AssertTrue(t, strings.HasPrefix(outputPath, "/") || strings.HasPrefix(outputPath, "\\"), "Should be an absolute path")

---

54-57: Remove legacy WTP_SHELL_INTEGRATION usage.

Env-based integration was removed; these lines are vestigial and can be dropped.

- // Simulate shell integration
- os.Setenv("WTP_SHELL_INTEGRATION", "1")
- defer os.Unsetenv("WTP_SHELL_INTEGRATION")
+ // No env-based integration needed; cd always returns a path

---

136-139: Same here: drop WTP_SHELL_INTEGRATION from completion behavior tests.

It no longer affects list/cd; removing reduces confusion.

- os.Setenv("WTP_SHELL_INTEGRATION", "1")
- defer os.Unsetenv("WTP_SHELL_INTEGRATION")
+ // No env-based integration needed

Also applies to: 155-157

---

68-120: Consider adding E2E for “wtp shell-init ”.

Verifies the one-liner UX promised in the PR: output contains both completion and hook parts. This complements the existing completion tests.

cmd/wtp/cd_test.go (7)

17-79: Make absolute-path tests platform-agnostic or skip on Windows

These cases hardcode POSIX absolute paths and use filepath.IsAbs on them. On Windows runners, these will fail. Either synthesize OS-specific absolute paths or skip on Windows.

Option A (recommended): guard on Windows.

 func TestCdCommand_AlwaysOutputsAbsolutePath(t *testing.T) {
+  if runtime.GOOS == "windows" {
+    t.Skip("POSIX paths in test data; skip on Windows.")
+  }
   // Setup a realistic worktree scenario

Add import:

+ "runtime"

Option B: construct paths via filepath.Join with a temp root and build worktreeList using filepath.ToSlash when needed. I can draft this if you want.

---

81-125: Simplify env handling with t.Setenv; avoid manual restore

Manual save/restore is verbose and error-prone. t.Setenv restores automatically and makes intent clearer.

- originalEnv := os.Getenv("WTP_SHELL_INTEGRATION")
- defer func() {
-   if originalEnv != "" {
-     os.Setenv("WTP_SHELL_INTEGRATION", originalEnv)
-   } else {
-     os.Unsetenv("WTP_SHELL_INTEGRATION")
-   }
- }()
...
   for _, env := range envStates {
     t.Run(env.name, func(t *testing.T) {
-      if env.value == "" {
-        os.Unsetenv("WTP_SHELL_INTEGRATION")
-      } else {
-        os.Setenv("WTP_SHELL_INTEGRATION", env.value)
-      }
+      if env.value == "" {
+        _ = os.Unsetenv("WTP_SHELL_INTEGRATION") // want truly unset
+      } else {
+        t.Setenv("WTP_SHELL_INTEGRATION", env.value)
+      }

---

126-160: Consider asserting CLI usage/args metadata for UX consistency

The error-path test looks good. To enforce the CLI UX contract, add asserts for ArgsUsage and a helpful Description snippet via TestCdCommand_CoreBehavior (see dedicated comment), or include a case for “unknown worktree” to ensure the user gets a clear, actionable error.

---

162-208: Edge-case tests are good; watch for POSIX path assumptions

These cover special characters and completion asterisk. Like the main test, they hardcode POSIX paths. If Windows support in CI is planned, use filepath utilities or skip there.

---

210-218: Strengthen command surface checks (ArgsUsage and Description)

To keep CLI UX aligned with README examples (per learnings), assert ArgsUsage and a key Description phrase so regressions are caught.

 func TestCdCommand_CoreBehavior(t *testing.T) {
   cmd := NewCdCommand()
   assert.Equal(t, "cd", cmd.Name)
   assert.Equal(t, "Output absolute path to worktree", cmd.Usage)
   assert.NotNil(t, cmd.ShellComplete)
+  assert.Equal(t, "<worktree-name>", cmd.ArgsUsage)
+  assert.Contains(t, cmd.Description, "Output the absolute path")
+  assert.Contains(t, cmd.Description, "eval \"$(wtp hook")
   // The rest is implementation detail - what matters is that it works
 }

---

221-255: Test name vs expectation mismatch; clarify fallback semantics and make cross-platform

• The subtest title says “fallback to directory name on error,” but it asserts a long relative path. That’s confusing for future readers.
• Per retrieved learnings for worktree naming, the intended fallback is directory name. If that’s still the product decision, both the implementation and this test should expect filepath.Base. If not, rename the test to reflect “relative path” fallback.
• Also, the hardcoded "../../../../..." won’t be correct on Windows.

Option A (keep current behavior; fix name and make OS-agnostic):

- t.Run("should fallback to directory name on error", func(t *testing.T) {
+ t.Run("should fallback to relative path on error", func(t *testing.T) {
...
- // Use invalid path to trigger error
- name := getWorktreeNameFromPathCd("/completely/different/path", cfg, "/path/to/repo", false)
- assert.Equal(t, "../../../../completely/different/path", name) // actual filepath.Rel result
+ // Use invalid path to trigger error; compare using filepath.Rel for portability
+ name := getWorktreeNameFromPathCd("/completely/different/path", cfg, "/path/to/repo", false)
+ rel, _ := filepath.Rel("/path/to/repo", "/completely/different/path")
+ assert.Equal(t, rel, name)

Option B (align to learnings: fallback to directory name):

// In cmd/wtp/cd.go (implementation change)
func getWorktreeNameFromPathCd(path string, cfg *config.Config, repoRoot string, isMain bool) string {
    if isMain {
        return "@"
    }
    baseDir := cfg.Defaults.BaseDir
    rel := strings.TrimPrefix(strings.TrimPrefix(path, filepath.Join(repoRoot, baseDir)+string(os.PathSeparator)), string(os.PathSeparator))
    if rel != "" && rel != path {
        return filepath.ToSlash(rel)
    }
    // fallback to directory name
    return filepath.Base(path)
}

If you choose Option B, update this test to expect "path". I can send a follow-up patch.

---

257-289: Use require for setup errors and de-duplicate with shared test helpers

• For mkdir/chdir setup, use require.NoError to fail fast.
• Static analysis flagged duplication with add/remove tests. Extract helpers (e.g., withTempGitDir(t), captureStdout(t), newTestConfig(baseDir)) into a small internal/testutil package used by all tests.

 err := os.MkdirAll(gitDir, 0755)
-assert.NoError(t, err)
+require.NoError(t, err)

 ...
 err = os.Chdir(tempDir)
-assert.NoError(t, err)
+require.NoError(t, err)

Example helper (new file suggestion):

// internal/testutil/fs.go
package testutil

import (
  "os"
  "path/filepath"
  "testing"
)

func WithTempGitDir(t *testing.T) string {
  t.Helper()
  dir := t.TempDir()
  require.NoError(t, os.MkdirAll(filepath.Join(dir, ".git"), 0o755))
  return dir
}

I can factor these across test files to address the dupl finding.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: CR
PR: satococoa/wtp#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-11T13:56:29.054Z
Learning: Applies to cmd/wtp/** : Keep CLI UX consistent with README examples and autocompletion behavior

Learnt from: CR
PR: satococoa/wtp#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-24T14:15:20.744Z
Learning: Applies to cmd/wtp/*.go : Update command help text when adding new features.

Learnt from: CR
PR: satococoa/wtp#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-11T13:56:29.054Z
Learning: Applies to cmd/wtp/** : Keep CLI UX consistent with README examples and autocompletion behavior

Learnt from: CR
PR: satococoa/wtp#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-24T14:15:20.744Z
Learning: Applies to cmd/wtp/completion.go : The getWorktreeNameFromPath() function in cmd/wtp/completion.go must implement worktree naming logic: return '@' for main worktree, relative path from base_dir for non-main worktrees, and fall back to directory name if path calculation fails.

Learnt from: CR
PR: satococoa/wtp#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-11T13:56:29.054Z
Learning: Reference AGENTS.md for contributor workflow; cite CLAUDE.md for deep design questions

Learnt from: CR
PR: satococoa/wtp#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-11T13:56:29.054Z
Learning: Applies to .wtp.yml : .wtp.yml defines project hooks executed on worktree creation; keep defaults safe

cmd/wtp/add.go (1)

35-41: Nice: dynamic branch completion is wired into wtp add.

Hooking ShellComplete: completeBranches aligns with the new architecture and improves UX. Keep README examples consistent with this behavior.

cmd/wtp/add_test.go (1)

934-966: Fix formatting and remove duplication flagged by the linter.

• gofmt failure at Line 937 indicates formatting drift. Run gofmt -s -w cmd/wtp/add_test.go (or go tool task dev) to satisfy the pipeline.
• Lines 934–966 duplicate logic also present in cmd/wtp/cd_test.go:257-289. Extract a tiny helper (e.g., in cmd/wtp/testutil) and reuse it in both tests to silence the “dupl” check.

Minimal change within this file (use a helper for silencing stdout):

 func TestCompleteBranches(t *testing.T) {
   t.Run("should not panic when called", func(t *testing.T) {
     cmd := &cli.Command{}
-
-    // Should not panic even without proper git setup
-    assert.NotPanics(t, func() {
-      // Capture stdout to avoid noise in tests
-      oldStdout := os.Stdout
-      os.Stdout = os.NewFile(0, os.DevNull)
-      defer func() { os.Stdout = oldStdout }()
-
-      completeBranches(context.Background(), cmd)
-    })
+    // Should not panic even without proper git setup
+    assert.NotPanics(t, func() {
+      withSilencedStdout(t, func() {
+        completeBranches(context.Background(), cmd)
+      })
+    })
   })
 }

Add this helper once and reuse across tests (new file suggestion):

// cmd/wtp/testutil/helpers_test.go
package testutil

import (
  "os"
  "testing"
)

func WithSilencedStdout(t *testing.T, fn func()) {
  t.Helper()
  oldStdout := os.Stdout
  os.Stdout = os.NewFile(0, os.DevNull)
  defer func() { os.Stdout = oldStdout }()
  fn()
}

Then import and call testutil.WithSilencedStdout from both test files. This addresses both the formatting violation (after gofmt) and the duplication warning.

⛔ Skipped due to learnings

Learnt from: CR
PR: satococoa/wtp#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-24T14:15:20.744Z
Learning: Applies to cmd/wtp/*.go : Update command help text when adding new features.

Learnt from: CR
PR: satococoa/wtp#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-24T14:15:20.744Z
Learning: Applies to cmd/wtp/completion.go : The getWorktreeNameFromPath() function in cmd/wtp/completion.go must implement worktree naming logic: return '@' for main worktree, relative path from base_dir for non-main worktrees, and fall back to directory name if path calculation fails.

cmd/wtp/hook.go (1)

12-44: Good UX: clear help text and per-shell subcommands.

The help/description text is explicit and aligns with the PR goals (“Less is More” separation). Nice.

cmd/wtp/cd.go (1)

67-79: Help and examples for cd look good.

Clear about printing an absolute path and how to enable hooks. Matches the new architecture.

README.md (3)

253-256: Confirm Homebrew auto-completions actually ship in the tap formula

“Tab completion is automatically installed! No manual setup required.” is great if the tap installs bash/zsh/fish completion files into the proper locations. Please verify the Homebrew formula generates and installs completions (including fish) on install/upgrade; otherwise this line can mislead users.

Would you like me to open a follow-up to update the tap formula and add CI to assert the completion files are present after brew install?

---

274-286: Docs for wtp cd behavior are clear and align with refactor

Explaining that wtp cd always prints an absolute path and showing the command-substitution pattern is exactly what users need without the hook. Nice.

---

315-318: “wtp once” auto-configuration claim needs validation or clarification

Auto-configuring both completion and hooks on first tab-press is a strong claim and non-standard for Homebrew-installed CLIs. If this relies on shell-specific lazy-loading or post-install hooks, please link to the mechanism or adjust wording to avoid overpromising.

If this behavior isn’t guaranteed across shells, propose changing to: “Homebrew users: tab completion is installed automatically. For wtp cd integration, run: eval '$(wtp hook )' or use the one-liner: eval '$(wtp shell-init )'.”

cmd/wtp/main.go (1)

38-40: Good wiring: adds ‘hook’ and ‘shell-init’; rely on built-in completion

Replacing the bespoke completion command with urfave/cli’s built-in completion and introducing focused hook/shell-init commands makes the surface area smaller and clearer. LGTM.

cmd/wtp/shell_init_test.go (1)

1-76: It looks like gofmt (and possibly revive) isn’t installed in this environment, so I can’t automatically verify that the file is properly formatted or lint-clean. Please manually run:

gofmt -s -l cmd/wtp/shell_init_test.go
revive -config revive.toml ./cmd/wtp/shell_init_test.go

and confirm there’s no output (i.e., no formatting or lint issues). Once that’s done, we’ll be sure the fixes have been applied correctly.

cmd/wtp/remove.go (1)

66-95: Help text and flags look solid.

Usage, examples, and new flags (with-branch, force-branch) align with the PR’s UX goals and retrieved learnings. Good job.

cmd/wtp/hook_test.go (1)

35-100: Good coverage of hook scripts’ essential behavior.

Validates per-shell fragments and ensures we’ve removed legacy WTP_SHELL_INTEGRATION. Nice.

cmd/wtp/shell_init.go (1)

13-45: Solid UX and docs in Description.

The one-liner instructions per shell align with the PR objectives and README UX guidance.

test/e2e/shell_test.go (1)

30-49: Branch resolution path checks look good.

The relaxed path checks cover both “worktrees/feature/test” and “feature/test”, matching the new behavior. Nice.

cmd/wtp/cd_test.go (1)

3-13: Import require and normalize imports/formatting

• In cmd/wtp/cd_test.go, add the "github.com/stretchr/testify/require" import (right after assert).
• Run the Go formatting tools locally to ensure imports are grouped and ordered correctly:

  gofmt -s -w cmd/wtp/cd_test.go
  goimports -w cmd/wtp/cd_test.go

Updated import block:

 import (
   "bytes"
   "context"
   "os"
   "path/filepath"
   "testing"

   "github.com/satococoa/wtp/internal/config"
   "github.com/stretchr/testify/assert"
+  "github.com/stretchr/testify/require"
   "github.com/urfave/cli/v3"
 )

cmd/wtp/cd_test.go (1)

228-241: Bug: stdout redirection uses invalid FD; open /dev/null for writing.

This is a repeat of a prior note; fix to avoid flakiness and resource leaks.

Apply:

@@
 	assert.NotPanics(t, func() {
 		// Capture stdout to avoid noise in tests
-		oldStdout := os.Stdout
-		os.Stdout = os.NewFile(0, os.DevNull)
-		defer func() { os.Stdout = oldStdout }()
+		devNull, err := os.OpenFile(os.DevNull, os.O_WRONLY, 0)
+		require.NoError(t, err)
+		defer devNull.Close()
+		oldStdout := os.Stdout
+		os.Stdout = devNull
+		defer func() { os.Stdout = oldStdout }()
@@
 		completeWorktreesForCd(context.Background(), cmd)
 	})
 })

And add the import:

@@
-	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"

.golangci.yml (1)

90-90: Unlimited same-issue reporting is OK; confirm CI noise tolerance.

max-same-issues: 0 can create noisy reports on mass violations. If this was intentional for migration, ignore; otherwise consider restoring a small cap (e.g., 50).

cmd/wtp/remove.go (7)

34-39: Use config.DefaultBaseDir for consistency.

Hardcoding "../worktrees" diverges from config.DefaultBaseDir used elsewhere. Prefer the constant for one source of truth.

-        defaultCfg := &config.Config{
-            Defaults: config.Defaults{
-                BaseDir: "../worktrees",
-            },
-        }
+        defaultCfg := &config.Config{
+            Defaults: config.Defaults{
+                BaseDir: config.DefaultBaseDir,
+            },
+        }

---

42-45: Normalize baseDir with filepath.Clean instead of trimming "/" (Windows-safe).

strings.TrimSuffix(baseDir, "/") misses "" on Windows. Clean once and drop manual trimming.

- baseDir := cfg.ResolveWorktreePath(mainRepoPath, "")
- // Remove trailing slash if it exists
- baseDir = strings.TrimSuffix(baseDir, "/")
+ baseDir := filepath.Clean(cfg.ResolveWorktreePath(mainRepoPath, ""))

---

58-65: Tighten “inside base dir” check to avoid false positives.

Prefix test should consider path separator and the exact ".." case.

- // If relative path starts with "..", it's outside base directory
- return !strings.HasPrefix(relPath, "..")
+ // Outside if ".." or starts with "..{sep}"
+ sep := string(os.PathSeparator)
+ return relPath != ".." && !strings.HasPrefix(relPath, ".."+sep)

---

141-149: Guard against empty ExecutionResult to avoid index panic.

Executor contracts can change; add a sanity check before indexing Results[0].

 result, err := executor.Execute([]command.Command{listCmd})
 if err != nil {
     return errors.GitCommandFailed("git worktree list", err.Error())
 }
 
-// Parse worktrees from command output
-worktrees := parseWorktreesFromOutput(result.Results[0].Output)
+if len(result.Results) == 0 {
+    return errors.GitCommandFailed("git worktree list", "empty result")
+}
+// Parse worktrees from command output
+worktrees := parseWorktreesFromOutput(result.Results[0].Output)

---

290-304: Unify name derivation via ResolveWorktreePath and normalize slashes.

Keeps behavior consistent with isWorktreeManaged and produces portable display names (forward slashes) across platforms.

- // Get base_dir path
- baseDir := cfg.Defaults.BaseDir
- if !filepath.IsAbs(baseDir) {
-     baseDir = filepath.Join(mainRepoPath, baseDir)
- }
+ // Resolve base_dir path consistently with config
+ baseDir := cfg.ResolveWorktreePath(mainRepoPath, "")
 
 // Calculate relative path from base_dir
 relPath, err := filepath.Rel(baseDir, worktreePath)
 if err != nil {
     // Fallback to directory name
     return filepath.Base(worktreePath)
 }
 
-return relPath
+return filepath.ToSlash(relPath)

---

338-346: Optional: sort candidates for deterministic completion UX.

Sorting names before printing yields stable completion ordering.

- for i := range worktrees {
+ var names []string
+ for i := range worktrees {
     wt := &worktrees[i]
     if !wt.IsMain && isWorktreeManaged(wt.Path, cfg, mainRepoPath, wt.IsMain) {
         // Calculate worktree name as relative path from base_dir
         name := getWorktreeNameFromPath(wt.Path, cfg, mainRepoPath, wt.IsMain)
-        fmt.Fprintln(w, name)
+        names = append(names, name)
     }
 }
+ sort.Strings(names)
+ for _, n := range names {
+     fmt.Fprintln(w, n)
+ }

---

352-363: Surface scanner errors (non-fatal) for easier debugging.

Print a debug line on scanner error; keeps ShellComplete signature unchanged.

 scanner := bufio.NewScanner(&buf)
 for scanner.Scan() {
     fmt.Println(scanner.Text())
 }
+_ = scanner.Err() // intentionally ignored; consider logging if you add a logger

cmd/wtp/shell_init.go (2)

13-22: Capture stderr from completion generation (better diagnostics).

Use CombinedOutput so failures include stderr text from the completion command.

 var runCompletionCommand = func(shell string) ([]byte, error) {
     exe, err := os.Executable()
     if err != nil {
         // Fallback to "wtp" if we can't find the executable
         exe = "wtp"
     }
 
-    cmd := exec.Command(exe, "completion", shell)
-    return cmd.Output()
+    cmd := exec.Command(exe, "completion", shell)
+    return cmd.CombinedOutput()
 }

---

114-122: Include command output on error to ease troubleshooting.

Propagate the captured output when completion generation fails.

 func outputCompletion(w io.Writer, shell string) error {
     output, err := runCompletionCommand(shell)
     if err != nil {
-        return fmt.Errorf("failed to generate %s completion: %w", shell, err)
+        return fmt.Errorf("failed to generate %s completion: %w\n%s", shell, err, string(output))
     }
 
     _, err = w.Write(output)
     return err
 }

cmd/wtp/add_test.go (1)

943-947: Fix stdout redirection to /dev/null (fd must be writable; don’t use fd=0).

os.NewFile(0, ...) targets stdin. Open /dev/null for writing instead to avoid EBADF on writes.

-            oldStdout := os.Stdout
-            os.Stdout = os.NewFile(0, os.DevNull)
-            defer func() { os.Stdout = oldStdout }()
+            oldStdout := os.Stdout
+            devnull, _ := os.OpenFile(os.DevNull, os.O_WRONLY, 0)
+            defer func() {
+                _ = devnull.Close()
+                os.Stdout = oldStdout
+            }()
+            os.Stdout = devnull

cmd/wtp/testhelpers_test.go (2)

31-34: Don’t ignore errors from os.Getwd; assert and restore safely.

Capture and assert the error from os.Getwd to avoid masking failures.

Apply:

- oldDir, _ := os.Getwd()
- t.Cleanup(func() { _ = os.Chdir(oldDir) })
- assert.NoError(t, os.Chdir(tempDir))
+ oldDir, err := os.Getwd()
+ assert.NoError(t, err)
+ t.Cleanup(func() { _ = os.Chdir(oldDir) })
+ assert.NoError(t, os.Chdir(tempDir))

---

61-67: Make expected relative path robust across platforms.

Compute expected via filepath.Rel instead of hard‑coding "../../../../...".

Apply:

- name := fn("/completely/different/path", cfg, "/path/to/repo", false)
- assert.Equal(t, "../../../../completely/different/path", name)
+ baseDir := filepath.Join("/path/to/repo", ".worktrees")
+ name := fn("/completely/different/path", cfg, "/path/to/repo", false)
+ expected, err := filepath.Rel(baseDir, "/completely/different/path")
+ assert.NoError(t, err)
+ assert.Equal(t, expected, name)

cmd/wtp/cd.go (3)

69-80: Expose the new one‑liner in help: reference ‘shell-init’.

Align CLI help with the PR objective “eval "$(wtp shell-init bash)"”.

Apply:

 		Description: "Output the absolute path to the specified worktree.\n\n" +
 			"Usage:\n" +
 			"  Direct:     cd \"$(wtp cd feature)\"\n" +
-			"  With hook:  wtp cd feature\n\n" +
-			"To enable the hook for easier navigation:\n" +
-			"  Bash: eval \"$(wtp hook bash)\"\n" +
-			"  Zsh:  eval \"$(wtp hook zsh)\"\n" +
-			"  Fish: wtp hook fish | source",
+			"  With hook:  wtp cd feature\n\n" +
+			"Setup:\n" +
+			"  One‑liner:  eval \"$(wtp shell-init bash)\"\n" +
+			"  Hook only:  eval \"$(wtp hook bash)\"",

---

121-129: Guard against empty executor results to avoid panic.

Check Results length before indexing.

Apply:

 	result, err := executor.Execute([]command.Command{listCmd})
 	if err != nil {
 		return fmt.Errorf("failed to get worktrees: %w", err)
 	}
+	if len(result.Results) == 0 {
+		return fmt.Errorf("failed to get worktrees: no results returned")
+	}
 
 	// Parse worktrees from command output
 	worktrees := parseWorktreesFromOutput(result.Results[0].Output)

---

281-294: Handle relPath == "." gracefully.

When a non‑main worktree equals base_dir (edge case), avoid returning "." as a name; fall back to basename.

Apply:

 	relPath, err := filepath.Rel(baseDir, worktreePath)
 	if err != nil {
 		// Fallback to directory name
 		return filepath.Base(worktreePath)
 	}
 
-	return relPath
+	if relPath == "." {
+		return filepath.Base(worktreePath)
+	}
+	return relPath

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: CR
PR: satococoa/wtp#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-11T13:56:29.054Z
Learning: Applies to cmd/wtp/** : Keep CLI UX consistent with README examples and autocompletion behavior

Learnt from: CR
PR: satococoa/wtp#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-24T14:15:20.744Z
Learning: Applies to cmd/wtp/*.go : Update command help text when adding new features.

Learnt from: CR
PR: satococoa/wtp#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-24T14:15:20.744Z
Learning: Before committing any changes, always run 'go tool task dev' to ensure code is formatted, linted, and tested.

Learnt from: CR
PR: satococoa/wtp#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-11T13:56:29.054Z
Learning: Applies to **/*.go : Follow lint rules (vet, staticcheck, gosec, lll=120, avoid ignored errors)

Learnt from: CR
PR: satococoa/wtp#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-11T13:56:29.054Z
Learning: Applies to **/*.go : Keep functions focused and small; use Go idioms

Learnt from: CR
PR: satococoa/wtp#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-11T13:56:29.054Z
Learning: Applies to **/*.go : Use gofmt and goimports for all Go code formatting

Learnt from: CR
PR: satococoa/wtp#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-11T13:56:29.054Z
Learning: Applies to **/*.go : Use short, lowercase package names

Learnt from: CR
PR: satococoa/wtp#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-11T13:56:29.054Z
Learning: Applies to **/*_test.go : Write tests as table-driven tests and name test files with *_test.go

Learnt from: CR
PR: satococoa/wtp#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-24T14:15:20.744Z
Learning: Applies to cmd/wtp/completion.go : The getWorktreeNameFromPath() function in cmd/wtp/completion.go must implement worktree naming logic: return '@' for main worktree, relative path from base_dir for non-main worktrees, and fall back to directory name if path calculation fails.

internal/errors/errors_test.go (2)

357-365: Update expectations to shell-init.

Error copy should reference wtp shell-init, not wtp completion.

Apply:

 func TestShellIntegrationRequired(t *testing.T) {
 	err := ShellIntegrationRequired()

 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "shell integration")
 	assert.Contains(t, err.Error(), "eval")
-	assert.Contains(t, err.Error(), "wtp completion")
+	assert.Contains(t, err.Error(), "wtp shell-init")
 	assert.Contains(t, err.Error(), "Setup:")
 }

---

433-436: Keep generic “helpful content” test aligned with new command name.

Apply:

 		{
 			name:     "ShellIntegrationRequired contains setup",
 			errorFn:  ShellIntegrationRequired,
-			keywords: []string{"Setup:", "eval", "completion"},
+			keywords: []string{"Setup:", "eval", "shell-init"},
 		},

cmd/wtp/remove_test.go (3)

845-849: Fix stdout redirection: using fd 0 (stdin) is wrong; open /dev/null for write.

os.NewFile(0, os.DevNull) targets fd 0 and doesn’t open the path; writes may fail subtly. Open os.DevNull with O_WRONLY and assign that to os.Stdout.

Apply:

-			// Capture stdout to avoid noise in tests
-			oldStdout := os.Stdout
-			os.Stdout = os.NewFile(0, os.DevNull)
-			defer func() { os.Stdout = oldStdout }()
+			// Capture stdout to avoid noise in tests
+			oldStdout := os.Stdout
+			devNull, _ := os.OpenFile(os.DevNull, os.O_WRONLY, 0)
+			defer devNull.Close()
+			os.Stdout = devNull
+			defer func() { os.Stdout = oldStdout }()

---

790-818: Improve mock result sequencing across multiple commands in a single Execute call.

Current logic returns the same m.results[m.callCount] for every command in the batch. If production ever batches >1 command per call, results won’t map 1:1. Safer to advance per-command.

Apply:

 func (m *mockRemoveCommandExecutor) Execute(commands []command.Command) (*command.ExecutionResult, error) {
 	// Accumulate all commands instead of overwriting
 	m.executedCommands = append(m.executedCommands, commands...)

 	if m.shouldFail && m.callCount > 0 {
 		errorMsg := m.errorMsg
 		if errorMsg == "" {
 			errorMsg = "mock error"
 		}
 		return nil, &mockRemoveError{message: errorMsg}
 	}

-	results := make([]command.Result, len(commands))
-	for i, cmd := range commands {
-		if m.callCount < len(m.results) {
-			results[i] = m.results[m.callCount]
-			results[i].Command = cmd
-		} else {
-			results[i] = command.Result{
-				Command: cmd,
-				Output:  "",
-				Error:   nil,
-			}
-		}
-	}
+	results := make([]command.Result, len(commands))
+	for i, cmd := range commands {
+		idx := m.callCount + i
+		if idx < len(m.results) {
+			results[i] = m.results[idx]
+			results[i].Command = cmd
+		} else {
+			results[i] = command.Result{Command: cmd}
+		}
+	}

-	m.callCount++
+	m.callCount += len(commands)
 	return &command.ExecutionResult{Results: results}, nil
 }

---

121-124: This subtest doesn’t validate anything. Either assert real behavior or drop it.

It only asserts true. Consider validating that --force-branch without --branch is either rejected or no-ops with a user-facing hint, depending on your intended UX.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: CR
PR: satococoa/wtp#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-11T13:56:29.054Z
Learning: Applies to cmd/wtp/** : Keep CLI UX consistent with README examples and autocompletion behavior

Learnt from: CR
PR: satococoa/wtp#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-24T14:15:20.744Z
Learning: Applies to cmd/wtp/completion.go : The getWorktreeNameFromPath() function in cmd/wtp/completion.go must implement worktree naming logic: return '@' for main worktree, relative path from base_dir for non-main worktrees, and fall back to directory name if path calculation fails.

Learnt from: CR
PR: satococoa/wtp#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-24T14:15:20.744Z
Learning: Applies to cmd/wtp/*.go : Update command help text when adding new features.