feat: expand default worktree base dir with repo placeholders #28
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughDefault worktree base_dir now supports a Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (invokes wtp)
participant CLI as wtp CLI
participant Cmd as Command (cd/list/remove/...)
participant Config as internal/config
participant FS as Filesystem / Git
Note over Config: New: expandBaseDirVariables\n(replace ${WTP_REPO_BASENAME})
User->>CLI: run command
CLI->>Cmd: dispatch to command handler (cmd *cli.Command)
Cmd->>Config: LoadConfig() / ResolveWorktreePath(mainRepoPath, "")
Config-->>Cmd: resolved baseDir/worktreePath
Cmd->>Cmd: maybeWarnLegacyWorktreeLayout(errWriter, mainRepoPath, cfg, worktrees)
alt legacy layout detected
Cmd->>User: emit migration warning + example git worktree move command
end
Cmd->>FS: perform git/fs operations (create/remove/list)
FS-->>Cmd: result / error
Cmd-->>User: final output / status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for expanding the ${WTP_REPO_BASENAME} placeholder in worktree base directories. The default base directory changes from ../worktrees to ../worktrees/${WTP_REPO_BASENAME}, allowing automatic isolation of worktrees by repository name without manual configuration.
Key Changes:
- Introduced
${WTP_REPO_BASENAME}placeholder expansion in config path resolution - Updated default base directory to include repository name isolation
- Refactored test setup to use more reliable error simulation methods
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/config/config.go | Added expandBaseDirVariables function and updated default base directory constant |
| internal/config/config_test.go | Added tests for placeholder expansion and updated existing tests to use new default |
| cmd/wtp/init.go | Updated template config file with placeholder documentation |
| cmd/wtp/init_test.go | Added test setup for file write mocking |
| cmd/wtp/cd.go, remove.go, list.go | Updated to use config.DefaultBaseDir constant and simplified path resolution |
| cmd/wtp/cd_test.go, remove_test.go, list_test.go | Updated mock paths to include repository name segment |
| test/e2e/hook_streaming_test.go | Updated worktree path assertions to include repository name |
| internal/hooks/executor_test.go | Replaced permission-based error tests with more reliable file-blocking approaches |
| docs/architecture.md | Updated documentation examples to show new path structure |
| README.md | Added placeholder documentation and updated examples |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| return &Config{ | ||
| Version: CurrentVersion, | ||
| Defaults: Defaults{ | ||
| BaseDir: "../worktrees", | ||
| BaseDir: DefaultBaseDir, | ||
| }, |
There was a problem hiding this comment.
Changing DefaultBaseDir to ../worktrees/${WTP_REPO_BASENAME} means that repositories without an explicit .wtp.yml now assume worktrees live under ../worktrees/<repo>/…. Existing users who previously relied on the old default (../worktrees/<branch>) and never generated a config file will still have their worktrees directly under ../worktrees, so isWorktreeManaged/ResolveWorktreePath will treat those directories as unmanaged and commands like wtp list, wtp cd, or wtp remove will no longer find them after upgrading. Consider detecting the legacy layout or retaining the old default when no config file exists so current worktrees remain usable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex
We don’t need automatic detection.
Update the README.md to mention this breaking change for users who don’t use .wtp.yml.
There was a problem hiding this comment.
Summary
- Documented the breaking change for users relying on the legacy
../worktreesdefault and outlined how to preserve that layout when upgrading. README.mdL217-L226
Testing
⚠️ Not run (not requested)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/config/config_test.go (1)
26-72: Excellent test coverage for placeholder expansion.The new test comprehensively validates
expandBaseDirVariablesbehavior across multiple scenarios: single placeholder, multiple placeholders, no placeholder, and trailing slash handling.However, there's a minor cleanup opportunity:
for _, tt := range tests { - tt := tt t.Run(tt.name, func(t *testing.T) {The
tt := ttcopy is no longer needed in Go 1.22+, as loop variables are now properly scoped per iteration by default.Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
README.md(6 hunks)cmd/wtp/cd.go(2 hunks)cmd/wtp/cd_test.go(2 hunks)cmd/wtp/init.go(3 hunks)cmd/wtp/init_test.go(2 hunks)cmd/wtp/list.go(1 hunks)cmd/wtp/list_test.go(11 hunks)cmd/wtp/remove.go(2 hunks)cmd/wtp/remove_test.go(13 hunks)docs/architecture.md(3 hunks)internal/config/config.go(4 hunks)internal/config/config_test.go(5 hunks)internal/hooks/executor_test.go(3 hunks)test/e2e/hook_streaming_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Run gofmt and goimports; keep imports grouped and organized; local import prefix follows module path github.com/satococoa/wtp
Adhere to golangci-lint rules configured for the project (vet, staticcheck, gosec, mnd, lll=120, etc.)
Errors should be wrapped with context and must not be ignored
Use snake_case for Go filenames; document exported identifiers when non-trivial
Files:
cmd/wtp/cd_test.gocmd/wtp/list.gocmd/wtp/remove_test.gocmd/wtp/init_test.gocmd/wtp/list_test.gocmd/wtp/cd.gointernal/hooks/executor_test.gocmd/wtp/init.gocmd/wtp/remove.gotest/e2e/hook_streaming_test.gointernal/config/config.gointernal/config/config_test.go
cmd/wtp/**
📄 CodeRabbit inference engine (AGENTS.md)
cmd/wtp/**: CLI entrypoint and commands are implemented under cmd/wtp
Update CLI help text to reflect user-facing changes
Command behavior: wtp cd prints only the absolute worktree path with no side effects
Command behavior: wtp completion generates pure completion scripts via urfave/cli
Command behavior: wtp hook emits shell functions that intercept wtp cd; wtp shell-init combines completion and hook output
Files:
cmd/wtp/cd_test.gocmd/wtp/list.gocmd/wtp/remove_test.gocmd/wtp/init_test.gocmd/wtp/list_test.gocmd/wtp/cd.gocmd/wtp/init.gocmd/wtp/remove.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Unit tests live alongside packages and use *_test.go naming
Prefer table-driven tests and mock git operations in unit tests
Files:
cmd/wtp/cd_test.gocmd/wtp/remove_test.gocmd/wtp/init_test.gocmd/wtp/list_test.gointernal/hooks/executor_test.gotest/e2e/hook_streaming_test.gointernal/config/config_test.go
docs/**
📄 CodeRabbit inference engine (AGENTS.md)
Maintain docs such as docs/architecture.md and docs/testing-guidelines.md alongside changes
Files:
docs/architecture.md
internal/{git,config,hooks,command,errors,io}/**
📄 CodeRabbit inference engine (AGENTS.md)
Place internal packages within internal/{git,config,hooks,command,errors,io}
Files:
internal/hooks/executor_test.gointernal/config/config.gointernal/config/config_test.go
README.md
📄 CodeRabbit inference engine (AGENTS.md)
Update README when user-facing behavior changes
Files:
README.md
test/e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
test/e2e/**: End-to-end tests reside under test/e2e
Document new workflows with realistic scenarios in test/e2e
Rely on real git workflows in E2E tests
Files:
test/e2e/hook_streaming_test.go
🧠 Learnings (5)
📚 Learning: 2025-10-07T15:56:11.510Z
Learnt from: CR
PR: satococoa/wtp#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T15:56:11.510Z
Learning: Applies to cmd/wtp/completion.go : Use getWorktreeNameFromPath() to resolve worktree display names consistently across completion, errors, and parsing
Applied to files:
cmd/wtp/list.gocmd/wtp/remove_test.gocmd/wtp/cd.gocmd/wtp/remove.gointernal/config/config.go
📚 Learning: 2025-10-07T15:56:11.510Z
Learnt from: CR
PR: satococoa/wtp#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T15:56:11.510Z
Learning: Applies to .wtp.yml : Project hooks are declared in .wtp.yml; keep commands deterministic and safe, avoiding destructive defaults
Applied to files:
docs/architecture.mdREADME.md
📚 Learning: 2025-10-07T15:56:11.510Z
Learnt from: CR
PR: satococoa/wtp#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T15:56:11.510Z
Learning: Applies to cmd/wtp/** : Command behavior: wtp cd prints only the absolute worktree path with no side effects
Applied to files:
docs/architecture.mdcmd/wtp/cd.go
📚 Learning: 2025-10-07T15:56:11.510Z
Learnt from: CR
PR: satococoa/wtp#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T15:56:11.510Z
Learning: Applies to cmd/wtp/** : Command behavior: wtp hook <shell> emits shell functions that intercept wtp cd; wtp shell-init <shell> combines completion and hook output
Applied to files:
docs/architecture.md
📚 Learning: 2025-10-07T15:56:11.510Z
Learnt from: CR
PR: satococoa/wtp#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T15:56:11.510Z
Learning: Applies to cmd/wtp/** : CLI entrypoint and commands are implemented under cmd/wtp
Applied to files:
docs/architecture.md
🧬 Code graph analysis (6)
cmd/wtp/list.go (1)
internal/config/config.go (1)
DefaultBaseDir(42-42)
cmd/wtp/remove_test.go (1)
internal/command/types.go (1)
Command(4-8)
cmd/wtp/list_test.go (1)
internal/config/config.go (2)
Defaults(20-22)DefaultBaseDir(42-42)
cmd/wtp/cd.go (1)
internal/config/config.go (1)
DefaultBaseDir(42-42)
cmd/wtp/remove.go (1)
internal/config/config.go (1)
DefaultBaseDir(42-42)
internal/config/config_test.go (1)
internal/config/config.go (3)
Defaults(20-22)DefaultBaseDir(42-42)Config(13-17)
🪛 GitHub Check: Lint
cmd/wtp/cd_test.go
[failure] 44-44:
File is not properly formatted (gofmt)
internal/config/config_test.go
[failure] 62-62:
The copy of the 'for' variable "tt" can be deleted (Go 1.22+) (copyloopvar)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests (macos-latest)
🔇 Additional comments (23)
docs/architecture.md (1)
105-105: LGTM! Documentation accurately reflects the new placeholder-based path resolution.The documentation updates consistently show the new
${WTP_REPO_BASENAME}placeholder in configuration examples and use the<repo>notation in path examples. This clearly communicates the dynamic, repository-scoped path handling to users.Also applies to: 171-171, 174-174, 215-215
cmd/wtp/list.go (1)
175-175: LGTM! Centralized default base directory reference.This change correctly replaces the hardcoded
"../worktrees"withconfig.DefaultBaseDir, ensuring consistency with the centralized configuration management introduced in this PR.cmd/wtp/init_test.go (2)
141-141: LGTM! Test expectation updated correctly.The test now correctly expects the
${WTP_REPO_BASENAME}placeholder in the generated configuration file, matching the changes incmd/wtp/init.go.
201-205: LGTM! Proper mocking enables error path testing.The addition of
writeFilemocking with proper save/restore pattern enables testing of the file write error path without relying on filesystem permissions.cmd/wtp/list_test.go (1)
112-112: LGTM! Consistent use of centralized default base directory.All test cases now correctly use
config.DefaultBaseDirinstead of hardcoded path strings. The special case at line 696 appropriately preserves the.worktreesoverride for subdirectory context testing while using the constant elsewhere.Also applies to: 181-181, 226-226, 248-248, 309-309, 377-377, 427-427, 458-458, 561-561, 696-696, 787-787
cmd/wtp/init.go (3)
19-19: LGTM! Mockable function enables comprehensive testing.The
writeFilevariable allows mocking in tests, enabling validation of error handling paths as demonstrated incmd/wtp/init_test.go.
58-59: LGTM! Configuration template updated with placeholder.The generated configuration now includes the
${WTP_REPO_BASENAME}placeholder with clear documentation, aligning with the PR's core feature.
92-92: LGTM! Uses mockable writeFile function.Correctly uses the mockable
writeFilefunction instead of directly callingos.WriteFile, enabling comprehensive test coverage.test/e2e/hook_streaming_test.go (1)
49-50: LGTM! E2E test updated for dynamic repository-based paths.The test now correctly constructs the worktree path using
filepath.Base(repo.Path())to dynamically extract the repository name, matching the behavior of the${WTP_REPO_BASENAME}placeholder expansion.cmd/wtp/remove.go (2)
36-36: LGTM! Centralized default base directory reference.This change correctly replaces the hardcoded
"../worktrees"withconfig.DefaultBaseDir, consistent with changes across other command files.
291-291: LGTM! Simplified path resolution using centralized resolver.The change delegates base directory resolution to
cfg.ResolveWorktreePath(), centralizing path resolution logic and ensuring consistent handling of the${WTP_REPO_BASENAME}placeholder.cmd/wtp/cd_test.go (1)
23-23: Fix formatting issue on line 44.The test data updates correctly reflect the new repository-scoped path structure with the
mainprefix. However, line 44 has a formatting issue that needs to be corrected.Please run
gofmtto fix the indentation:#!/bin/bash # Fix formatting issues in the file gofmt -w cmd/wtp/cd_test.goBased on static analysis hints.
Also applies to: 44-44, 50-50
⛔ Skipped due to learnings
Learnt from: CR PR: satococoa/wtp#0 File: AGENTS.md:0-0 Timestamp: 2025-10-07T15:56:11.510Z Learning: Applies to **/*.go : Run gofmt and goimports; keep imports grouped and organized; local import prefix follows module path github.com/satococoa/wtpcmd/wtp/cd.go (2)
29-39: LGTM! Centralized path resolution is well-integrated.The changes correctly delegate base directory resolution to
cfg.ResolveWorktreePath(mainRepoPath, ""), which handles the new${WTP_REPO_BASENAME}placeholder expansion. This ensures consistent path resolution across the codebase.
281-281: LGTM! Consistent path resolution.Using
cfg.ResolveWorktreePath(mainRepoPath, "")here maintains consistency with the centralized path resolution approach used throughout the file.internal/config/config_test.go (1)
375-427: LGTM! Test cases properly reflect placeholder expansion.The updated test cases correctly validate that:
- Default base_dir expands
${WTP_REPO_BASENAME}to the repository basename- Relative paths without placeholders work as before
- Absolute paths are handled correctly
- Empty worktree names return just the base directory
README.md (2)
189-220: LGTM! Clear documentation of placeholder expansion.The documentation effectively explains:
- How to use
${WTP_REPO_BASENAME}in configuration- That it expands to the repository directory name
- The benefit of zero-config isolation between repositories
This will help users understand and leverage the new feature.
332-354: LGTM! Directory structure clearly illustrates placeholder expansion.The updated diagram with
<repo-name>/effectively visualizes how the placeholder creates repository-specific worktree directories, maintaining the same hierarchical structure within each repository's namespace.cmd/wtp/remove_test.go (1)
43-83: LGTM! Test data properly updated for new path structure.All test cases correctly reflect the new worktree path structure where worktrees are organized under
worktrees/main/...(withmainbeing the repository basename in test fixtures). The expectations are consistent across different test scenarios.internal/config/config.go (2)
160-169: LGTM! Clean and correct placeholder expansion implementation.The
expandBaseDirVariablesfunction:
- Correctly extracts the repository basename using
filepath.Base- Handles empty
baseDirgracefully by returning early- Uses
strings.ReplaceAllfor straightforward substitution- Properly handles trailing slashes (filepath.Base strips them automatically)
This implementation is solid and maintainable.
152-158: LGTM! Proper integration of placeholder expansion.Calling
expandBaseDirVariablesbefore converting to an absolute path ensures that placeholders are resolved using the repository root context. This correctly sequences the operations: expand variables → resolve relative paths → join with worktree name.internal/hooks/executor_test.go (3)
817-837: LGTM! More reliable error injection approach.Creating a blocking file at the destination path and attempting to create a nested path under it is a more reliable way to trigger destination creation errors compared to permission-based approaches. This should work consistently across different filesystems and platforms.
839-863: LGTM! Platform-independent error injection.Replacing the directory with a regular file to force
ReadDirfailures is more robust than permission-based testing. This approach should work reliably across different platforms and doesn't require special permission handling.
904-934: LGTM! Effective use of broken symlink for error testing.Using a broken symlink (pointing to a non-existent target) to simulate a file that cannot be copied is a clever and portable way to test error propagation from nested
copyFilecalls. This avoids platform-specific permission manipulation.
|
@codex fix lint |
|
Summary
Testing
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (2)
15-15: Clarify placeholder notation in feature overview.Line 15 uses
<repo-name>as a visual placeholder, but the actual configuration template uses${WTP_REPO_BASENAME}. For consistency with the Configuration section (line 192), consider either:
- Using the full syntax:
../worktrees/${WTP_REPO_BASENAME}/feature/auth- Or add a note clarifying that
<repo-name>expands via the${WTP_REPO_BASENAME}placeholder-`feature/auth` branch goes to `../worktrees/<repo-name>/feature/auth` - no redundant typing, +`feature/auth` branch goes to `../worktrees/${WTP_REPO_BASENAME}/feature/auth` - no redundant typing,
130-144: Add clarification that<repo-name>is a placeholder in Quick Start examples.The examples show
../worktrees/<repo-name>/...paths, but it's not immediately clear whether this is a literal directory name or a placeholder. Add a brief note to help users understand that<repo-name>will expand to their repository's basename (e.g.,myproject).# Create worktree from existing branch (local or remote) +# → Creates worktree at ../worktrees/<repo-name>/feature/auth (where <repo-name> is your repository's basename) # Automatically tracks remote branch if not found locally wtp add feature/auth
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
README.md
📄 CodeRabbit inference engine (AGENTS.md)
Update README when user-facing behavior changes
Files:
README.md
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: satococoa/wtp#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T15:56:11.510Z
Learning: Applies to cmd/wtp/completion.go : Use getWorktreeNameFromPath() to resolve worktree display names consistently across completion, errors, and parsing
📚 Learning: 2025-10-07T15:56:11.510Z
Learnt from: CR
PR: satococoa/wtp#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T15:56:11.510Z
Learning: Applies to .wtp.yml : Project hooks are declared in .wtp.yml; keep commands deterministic and safe, avoiding destructive defaults
Applied to files:
README.md
🔇 Additional comments (2)
README.md (2)
191-226: Documentation of placeholder expansion and breaking changes is comprehensive.The configuration section clearly explains the
${WTP_REPO_BASENAME}placeholder, its expansion behavior, and includes an explicit breaking change notice for users upgrading from the previous default. The migration path is well-documented. The breaking change notice is appropriately placed and formatted.
341-356: Worktree structure diagram correctly reflects the new repository-scoped layout.The updated diagram accurately shows the new default structure with
<repo-name>as a top-level directory under../worktrees/, replacing the previous flat layout. The diagram is clear and the comments linking towtp addcommands help users understand the mapping.
|
fix these lint errors. |
|
Summary
Testing
|
satococoa
force-pushed
the
codex/update-defaultbasedir-for-worktrees
branch
from
13781a6 to
a950867
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/wtp/worktree_managed.go (1)
29-46: Consider logging when path resolution fails.The function now returns
falsewhenfilepath.Absfails, treating the error as "worktree not managed." While this is a reasonable fallback given the boolean return type, silently swallowing path resolution errors could make debugging difficult if users have malformed paths or permission issues.Consider adding a debug log or comment explaining that path resolution failures are treated as "not managed":
absWorktreePath := worktreePath if !filepath.IsAbs(worktreePath) { var err error absWorktreePath, err = filepath.Abs(worktreePath) if err != nil { + // Treat path resolution failure as "not managed" return false } }internal/config/config.go (1)
160-169: Consider handling empty repoRoot edge case.If
repoRootis empty,filepath.Base("")returns".", which would expand${WTP_REPO_BASENAME}to".". While this is unlikely in normal usage (called from repository context), it could produce unexpected paths like../worktrees/./<branch>.Consider adding a guard or comment:
func expandBaseDirVariables(baseDir, repoRoot string) string { if baseDir == "" { return baseDir } + if repoRoot == "" { + repoRoot = "." // or return baseDir unchanged + } repoBaseName := filepath.Base(repoRoot) expanded := strings.ReplaceAll(baseDir, "${WTP_REPO_BASENAME}", repoBaseName) return expanded }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
README.md(6 hunks)cmd/wtp/cd.go(1 hunks)cmd/wtp/cd_test.go(2 hunks)cmd/wtp/init.go(3 hunks)cmd/wtp/init_test.go(2 hunks)cmd/wtp/list_test.go(11 hunks)cmd/wtp/remove.go(1 hunks)cmd/wtp/remove_test.go(15 hunks)cmd/wtp/worktree_managed.go(1 hunks)docs/architecture.md(3 hunks)internal/config/config.go(4 hunks)internal/config/config_test.go(5 hunks)internal/hooks/executor_test.go(3 hunks)test/e2e/hook_streaming_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/wtp/init.go
- internal/config/config_test.go
- cmd/wtp/init_test.go
- cmd/wtp/cd.go
- docs/architecture.md
- cmd/wtp/list_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/wtp/remove_test.go (1)
internal/command/types.go (1)
Command(4-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (10)
test/e2e/hook_streaming_test.go (1)
49-50: LGTM! Test correctly updated for per-repository worktree isolation.The updated path construction properly implements the new directory structure by inserting the repository basename between "worktrees" and the branch name. The comment accurately documents the new default location pattern.
README.md (2)
15-15: LGTM! Examples consistently updated.The documentation examples clearly show the new repository-scoped worktree paths with
<repo-name>placeholder, helping users understand the new default behavior.Also applies to: 130-130, 135-135, 139-139, 143-143
217-226: Clear migration guidance for breaking change.The breaking change note properly warns users about the impact and provides a clear migration path (adding
.wtp.ymlwithbase_dir: "../worktrees"to preserve legacy layout).cmd/wtp/cd_test.go (1)
23-23: Test fixtures correctly updated for new path structure.The test data properly reflects the new default base directory structure with repository name isolation (
/worktrees/main/feature/auth).Also applies to: 44-44, 50-50
cmd/wtp/remove.go (1)
285-285: Correct use of centralized path resolution.Using
cfg.ResolveWorktreePath(mainRepoPath, "")ensures placeholder expansion is applied consistently. The empty string argument effectively retrieves the resolved base directory.cmd/wtp/remove_test.go (1)
57-58: Test data systematically updated for new structure.All test fixtures consistently reflect the new repository-scoped path structure (
/worktrees/main/...), ensuring tests verify the correct behavior.Also applies to: 65-65, 145-146, 153-153, 162-163, 170-170, 179-180, 187-187
internal/config/config.go (1)
42-42: Core placeholder expansion properly integrated.The changes correctly introduce repository-based path isolation by:
- Defining the templated default
- Using it consistently in default config
- Applying expansion before path resolution
Also applies to: 57-57, 153-153
internal/hooks/executor_test.go (3)
860-870: Improved test reliability with explicit blocking file.Creating a regular file to block directory creation is more reliable than depending on non-existent parent directories, making the test behavior more predictable across different systems.
881-895: Better failure injection approach.Replacing the directory with a regular file to force
ReadDirto fail is clearer and more portable than relying on permission-based failures.
951-960: Broken symlink appropriately simulates copy failure.Using a broken symlink to trigger nested file copy errors is a clean approach that doesn't rely on file system permissions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cmd/wtp/worktree_managed_test.go (2)
71-73: Simplify redundant path operations.
filepath.Cleanalready normalizes paths and removes trailing separators, making the explicitTrimSuffixredundant.Apply this diff:
- baseDir := cfg.ResolveWorktreePath(canonicalMainRepoPath, "") - baseDir = strings.TrimSuffix(baseDir, string(filepath.Separator)) - baseDir = filepath.Clean(baseDir) + baseDir := filepath.Clean(cfg.ResolveWorktreePath(canonicalMainRepoPath, ""))
78-82: Consider adding explanatory comments.These assertions validate test setup preconditions (ensuring the relative path resolves inside the base directory). While the logic is sound, a brief comment would clarify the intent for future maintainers.
Example:
// Verify test setup: relPath should be inside baseDir (not escape with "..") relPath, err := filepath.Rel(baseDir, absWorktreePath) require.NoError(t, err) require.NotEqual(t, "..", relPath) require.False(t, strings.HasPrefix(relPath, ".."+string(filepath.Separator)))test/e2e/worktree_test.go (1)
33-45: Consider resolving symlinks for worktree paths to ensure consistent comparison.The comparison resolves symlinks for
expectedPathbut only cleans the worktree paths from the list. Ifrepo.ListWorktrees()returns paths containing symlinks, the comparisonfilepath.Clean(wt) == resolvedExpectedPathcould fail even when pointing to the same location.Apply this diff to resolve symlinks for both paths:
found := false for _, wt := range worktrees { - if filepath.Clean(wt) == resolvedExpectedPath { + resolvedWt, err := filepath.EvalSymlinks(filepath.Clean(wt)) + if err == nil && resolvedWt == resolvedExpectedPath { found = true break } }Alternatively, verify that
repo.ListWorktrees()already returns fully resolved paths, making this change unnecessary:#!/bin/bash # Verify whether ListWorktrees returns resolved or unresolved symlink paths # Find the implementation of ListWorktrees ast-grep --pattern 'func ($$$) ListWorktrees() $$$ { $$$ }' # Search for any symlink handling in the test framework rg -n -A5 -B5 'ListWorktrees' test/e2e/framework/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/wtp/worktree_managed_test.go(1 hunks)test/e2e/worktree_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/wtp/worktree_managed_test.go (1)
internal/config/config.go (3)
Config(13-17)Defaults(20-22)DefaultBaseDir(42-42)
test/e2e/worktree_test.go (1)
test/e2e/framework/assertions.go (2)
AssertNoError(68-71)AssertTrue(144-147)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (5)
cmd/wtp/worktree_managed_test.go (3)
1-13: LGTM!Package declaration and imports are properly organized and all appear necessary for the test implementations.
31-45: LGTM!The negative test case correctly validates that paths outside the configured base directory return false.
15-29: Based on my analysis of the code, the original review comment is incorrect.The Go
filepath.Abs()function returns an absolute representation of a path by joining it with the current working directory if it's not already absolute. Crucially, it does not require the paths to exist on the filesystem. The function callsCleanon the result, but this is purely string-based manipulation.Examining the
isWorktreeManagedCommonimplementation shows:
- It calls
filepath.Abs()on paths only when they are not already absolute- Since
worktreePathis constructed asfilepath.Join(tempDir, "worktrees", "repo", "feature", "foo"), it is an absolute path andfilepath.Abs()will not be invoked on it- The function uses only
filepath.Rel()to compute relative paths—another purely lexical operation that doesn't access the filesystem- No existence checks (like
os.Stat()) or file operations are performedThe test is valid: creating only
mainRepoPathon disk while leavingworktreePathuncreated does not cause any issues since the function operates entirely on path strings.
Likely an incorrect or invalid review comment.
test/e2e/worktree_test.go (2)
5-5: LGTM: Import added for path manipulation.The
path/filepathimport is correctly added to support the new worktree path construction and symlink resolution logic.
28-32: No changes required—code is correct.The symlink resolution is appropriate defensive programming. EvalSymlinks returns the path name after the evaluation of any symbolic links. On non-symlinked paths (like those created by the test), it simply returns the canonical path without error. Since the test framework does not use symlinks and the worktree path is freshly created, this code will succeed reliably.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/wtp/cd.go(4 hunks)cmd/wtp/legacy_warning.go(1 hunks)cmd/wtp/legacy_warning_test.go(1 hunks)cmd/wtp/list.go(3 hunks)cmd/wtp/list_test.go(22 hunks)cmd/wtp/remove.go(5 hunks)cmd/wtp/remove_test.go(17 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/wtp/remove_test.go
🧰 Additional context used
🧬 Code graph analysis (6)
cmd/wtp/legacy_warning.go (2)
internal/config/config.go (2)
Config(13-17)ConfigFileName(40-40)internal/git/worktree.go (1)
Worktree(14-19)
cmd/wtp/list.go (1)
internal/config/config.go (1)
Config(13-17)
cmd/wtp/remove.go (4)
internal/git/worktree.go (1)
Worktree(14-19)internal/config/config.go (4)
LoadConfig(49-79)Config(13-17)Defaults(20-22)DefaultBaseDir(42-42)internal/errors/errors.go (2)
WorktreeNotFound(86-100)WorktreeRemovalFailed(133-166)internal/command/builders.go (1)
GitWorktreeRemove(66-79)
cmd/wtp/list_test.go (1)
internal/config/config.go (3)
Config(13-17)Defaults(20-22)DefaultBaseDir(42-42)
cmd/wtp/legacy_warning_test.go (2)
internal/config/config.go (4)
Config(13-17)Defaults(20-22)DefaultBaseDir(42-42)ConfigFileName(40-40)internal/git/worktree.go (1)
Worktree(14-19)
cmd/wtp/cd.go (1)
internal/config/config.go (4)
LoadConfig(49-79)Config(13-17)Defaults(20-22)DefaultBaseDir(42-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests (macos-latest)
🔇 Additional comments (6)
cmd/wtp/remove.go (6)
62-65: LGTM: Standardized writer access pattern.The refactoring to access the writer through
rootCmd.Writerfollows a consistent pattern and includes appropriate nil fallback.
117-139: LGTM: Well-structured refactoring into helper functions.The extraction of
locateTargetWorktree,guardAgainstRemovingCurrentWorktree, andexecuteWorktreeRemovalinto separate helpers improves readability and maintainability. The error handling flow is clear and correct.
165-191: LGTM: Helper functions are well-implemented.Both
locateTargetWorktreeandguardAgainstRemovingCurrentWorktreeproperly handle error cases, use appropriate error messages, and implement the correct logic for their responsibilities.
193-208: LGTM: Robust error handling for worktree removal.The function properly handles both command execution errors and Git-specific errors, and includes Git output in error messages for better debugging.
336-336: LGTM: Correct implementation of placeholder-based base_dir resolution.The change to use
cfg.ResolveWorktreePath(mainRepoPath, "")properly leverages the new centralized path resolution logic that handles${WTP_REPO_BASENAME}placeholder expansion, aligning with the PR's core objective.
141-163: ****The
notifyLegacyLayoutfunction is not duplicating centralized utilities—it's already calling them. The wrapper is defined and used only inremove.go(called at line 115), and it delegates the actual warning logic to the centralizedmaybeWarnLegacyWorktreeLayoutfromlegacy_warning.go. The pattern does not appear duplicated across other command files.Likely an incorrect or invalid review comment.
| moveCommand := strings.Join([]string{ | ||
| "git worktree move", | ||
| filepath.Join("..", "worktrees", "feature", "foo"), | ||
| filepath.Join("..", "worktrees", filepath.Base(mainRepoPath), "feature", "foo"), | ||
| }, " ") | ||
| assert.Contains(t, output, moveCommand) |
There was a problem hiding this comment.
Update test expectation for quoted move command.
Once the warning prints the migration command with safe quoting, this assertion should expect the %q output (e.g. "git worktree move \"../worktrees/...\" \"../worktrees/...\""). Please update the test string accordingly so it matches the corrected warning format.
🤖 Prompt for AI Agents
In cmd/wtp/legacy_warning_test.go around lines 72 to 77, the test currently
builds an unquoted moveCommand and asserts it appears in output, but the warning
now emits the migration command with shell-safe quoting (%q); update the
expectation to match that by constructing the expected string with quoted paths
(e.g. use fmt.Sprintf("%q", path) for each path or fmt.Sprintf("git worktree
move %q %q", src, dst)) and then assert.Contains(t, output,
expectedQuotedMoveCommand).
| fmt.Fprintf(w, " git worktree move %s %s\n", migration.currentRel, migration.suggestedRel) | ||
| } |
There was a problem hiding this comment.
Quote the suggested git worktree move command.
If the repo name or worktree path contains spaces (totally valid on macOS/Windows), this suggestion becomes git worktree move ../worktrees/My Repo/foo ..., which breaks the one-liner the warning asks the user to run. Please emit shell-safe quoting so the guidance works everywhere.
- fmt.Fprintf(w, " git worktree move %s %s\n", migration.currentRel, migration.suggestedRel)
+ fmt.Fprintf(w, " git worktree move %q %q\n", migration.currentRel, migration.suggestedRel)This uses Go’s %q formatter to escape spaces and special characters consistently.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fmt.Fprintf(w, " git worktree move %s %s\n", migration.currentRel, migration.suggestedRel) | |
| } | |
| fmt.Fprintf(w, " git worktree move %q %q\n", migration.currentRel, migration.suggestedRel) | |
| } |
🤖 Prompt for AI Agents
In cmd/wtp/legacy_warning.go around lines 55-56, the formatted suggestion for
the git worktree move command doesn't quote paths, so names with spaces or
special characters will break the one-line suggestion; change the fmt.Fprintf
call to use Go's %q formatter for the path arguments (e.g., replace %s with %q
for migration.currentRel and migration.suggestedRel) so the output is shell-safe
and properly escaped.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/wtp/list.go (2)
107-110: Consider reusing therootCmdalias for consistency.The
rootCmdalias is introduced here butcmd.Root()is called again at line 158. For consistency and to fully benefit from the alias, consider reusingrootCmdin the errWriter logic as well.
156-164: Simplify the errWriter selection logic for better readability.The current nested conditionals work correctly but are harder to follow. Consider this clearer structure:
- errWriter := io.Discard - if cmd != nil { - if root := cmd.Root(); root != nil && root.ErrWriter != nil { - errWriter = root.ErrWriter - } else if root != nil && root.ErrWriter == nil { - errWriter = os.Stderr - } - } + errWriter := io.Discard + if cmd != nil { + root := cmd.Root() + if root != nil { + if root.ErrWriter != nil { + errWriter = root.ErrWriter + } else { + errWriter = os.Stderr + } + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/wtp/list.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/wtp/list.go (1)
internal/config/config.go (1)
Config(13-17)
🔇 Additional comments (3)
cmd/wtp/list.go (3)
35-35: LGTM! Good addition of the spacing slots constant.The new
columnSpacingSlotsconstant improves maintainability and is correctly used in line 408 for spacing calculations.
130-130: LGTM! Correct parameter name change.The parameter name change from
_tocmdis appropriate since the function now uses this parameter for errWriter selection and legacy layout warnings.
408-409: LGTM! Good refactor of spacing calculation.Using the
columnSpacingSlotsconstant makes the spacing calculation clearer and more maintainable.
satococoa
force-pushed
the
codex/update-defaultbasedir-for-worktrees
branch
from
0576c96 to
df8cc63
Compare
Summary
${WTP_REPO_BASENAME}expansion when resolving worktree base directories and update the default to../worktrees/${WTP_REPO_BASENAME}Testing
https://chatgpt.com/codex/tasks/task_e_68f26696ce10832785e2fc425a19846a
Summary by CodeRabbit
Breaking Changes
New Features
Documentation
Tests