feat: add symlink hook #71
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. 📝 WalkthroughWalkthroughAdds explicit symlink hook support across docs, config, execution, and tests to enable creating symlinks (e.g., sharing Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*_test.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (3)📚 Learning: 2025-12-02T13:33:48.693ZApplied to files:
📚 Learning: 2025-12-02T13:33:48.693ZApplied to files:
📚 Learning: 2025-12-02T13:33:48.693ZApplied to files:
🧬 Code graph analysis (1)internal/hooks/executor_test.go (2)
🔇 Additional comments (5)
✏️ Tip: You can disable this entire section by setting 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 a new symlink hook type to wtp, enabling users to create symbolic links from the main worktree to new worktrees. This is useful for sharing large or mutable directories (e.g., .bin, .cache, node_modules) between worktrees without duplicating content.
Key changes:
- Added
HookTypeSymlinkconstant and validation logic in the config package - Implemented
executeSymlinkHookWithWriterin the hooks executor with path validation and safety checks - Added comprehensive test coverage including platform-specific symlink support detection
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
internal/config/config.go |
Added symlink hook type constant and validation rules requiring from and to fields |
internal/config/config_test.go |
Added test cases for valid symlink hooks and validation error cases (missing fields, invalid field combinations) |
internal/hooks/executor.go |
Implemented symlink hook execution with path resolution, security checks, and relative symlink creation |
internal/hooks/executor_test.go |
Added requireSymlinkSupport helper and basic symlink hook test verifying link creation and target resolution |
docs/architecture.md |
Added symlink hook example to configuration documentation |
README.md |
Added symlink hook documentation with examples and use cases |
cmd/wtp/init.go |
Added commented symlink hook example to the init template |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/hooks/executor.go
Outdated
| linkTarget := srcPath | ||
| if relTarget, err := filepath.Rel(dstDir, srcPath); err == nil { | ||
| linkTarget = relTarget | ||
| } | ||
|
|
||
| if err := os.Symlink(linkTarget, dstPath); err != nil { |
There was a problem hiding this comment.
The symlink implementation creates relative symlinks when possible, which differs from the copy hook behavior that always uses absolute paths. This inconsistency could lead to unexpected behavior. Consider whether symlinks should always be absolute (like copy operations) for consistency, or document this difference clearly. Relative symlinks can break if the symlink is moved, while absolute symlinks remain valid but are less portable.
| linkTarget := srcPath | |
| if relTarget, err := filepath.Rel(dstDir, srcPath); err == nil { | |
| linkTarget = relTarget | |
| } | |
| if err := os.Symlink(linkTarget, dstPath); err != nil { | |
| // Use an absolute path for the symlink target for consistency with copy hooks. | |
| if err := os.Symlink(srcPath, dstPath); err != nil { |
| func TestExecutePostCreateHooks_Symlink(t *testing.T) { | ||
| requireSymlinkSupport(t) | ||
|
|
||
| tempDir := t.TempDir() | ||
| repoRoot := filepath.Join(tempDir, "repo") | ||
| worktreeDir := filepath.Join(tempDir, "worktree") | ||
|
|
||
| err := os.MkdirAll(repoRoot, directoryPermissions) | ||
| require.NoError(t, err) | ||
| err = os.MkdirAll(worktreeDir, directoryPermissions) | ||
| require.NoError(t, err) | ||
|
|
||
| srcDir := filepath.Join(repoRoot, ".bin") | ||
| require.NoError(t, os.MkdirAll(srcDir, directoryPermissions)) | ||
| require.NoError(t, os.WriteFile(filepath.Join(srcDir, "tool"), []byte("bin"), 0644)) | ||
|
|
||
| cfg := &config.Config{ | ||
| Hooks: config.Hooks{ | ||
| PostCreate: []config.Hook{ | ||
| { | ||
| Type: config.HookTypeSymlink, | ||
| From: ".bin", | ||
| To: ".bin", | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| executor := NewExecutor(cfg, repoRoot) | ||
| var buf bytes.Buffer | ||
| err = executor.ExecutePostCreateHooks(&buf, worktreeDir) | ||
| assert.NoError(t, err) | ||
|
|
||
| dstPath := filepath.Join(worktreeDir, ".bin") | ||
| info, err := os.Lstat(dstPath) | ||
| require.NoError(t, err) | ||
| assert.NotZero(t, info.Mode()&os.ModeSymlink) | ||
|
|
||
| linkTarget, err := os.Readlink(dstPath) | ||
| require.NoError(t, err) | ||
|
|
||
| resolvedTarget := linkTarget | ||
| if !filepath.IsAbs(resolvedTarget) { | ||
| resolvedTarget = filepath.Join(filepath.Dir(dstPath), resolvedTarget) | ||
| } | ||
| assert.Equal(t, filepath.Clean(srcDir), filepath.Clean(resolvedTarget)) | ||
|
|
||
| output := buf.String() | ||
| assert.Contains(t, output, "Symlinking: .bin → .bin") | ||
| assert.Contains(t, output, "✓ Hook 1 completed") | ||
| } |
There was a problem hiding this comment.
Missing test coverage for the symlink hook's error handling cases. The test suite should include tests for: source path doesn't exist, destination path already exists, and path traversal attempts. The copy hook has similar test coverage (e.g., TestExecutePostCreateHooks_CopyNonExistentFile), and symlink hooks should have equivalent coverage.
Summary
Testing
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.