feat: expand default worktree base dir with repo placeholders #28
Conversation
|
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. WalkthroughReplaces static worktree base paths with a repository-scoped placeholder ( Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Command (cd/list/remove/init)
participant Config as config package
participant FS as Filesystem
CLI->>Config: ResolveWorktreePath(mainRepoPath, worktreeName)
Note over Config: expandBaseDirVariables(base_dir)\nreplace `${WTP_REPO_BASENAME}` -> filepath.Base(mainRepoPath)
Config->>Config: Build absolute resolved base path
Config-->>CLI: resolved worktree path
CLI->>FS: check / access / operate on resolved path
Note right of CLI: init uses `writeFile` (mockable) for config writes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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
|
1 participant
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
New Features
Documentation