Add pre_remove and post_remove hooks for worktree removal lifecycle #68
Conversation
WalkthroughAdds pre-remove and post-remove hook support: configuration fields and validation, executor refactor to support base-path-aware hook execution, integration of pre-remove hooks before git worktree removal and post-remove hooks after removal, updated init template and docs, and corresponding tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RemoveCmd as Remove Command
participant Config as Config Loader
participant PreHook as Pre-Remove Executor
participant Git as Git Worktree
participant PostHook as Post-Remove Executor
User->>RemoveCmd: wtp remove <name>
RemoveCmd->>Config: Load repo config (.wtp.yml)
Config-->>RemoveCmd: Config (hooks)
alt has pre_remove
RemoveCmd->>PreHook: Execute pre-remove hooks
PreHook->>PreHook: Resolve paths relative to worktree
PreHook-->>RemoveCmd: Pre-remove hooks completed / error
end
RemoveCmd->>Git: git worktree remove <path>
Git-->>RemoveCmd: Worktree removed / error
alt has post_remove
RemoveCmd->>PostHook: Execute post-remove hooks
PostHook->>PostHook: Resolve paths relative to repo root
PostHook-->>RemoveCmd: Post-remove hooks completed / error
end
RemoveCmd-->>User: Report success or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cmd/wtp/remove.go (1)
198-229: Consider moving work_dir resolution into the executor.The current approach creates a modified copy of the config just to adjust
work_dirpaths before calling the executor. This adds complexity at the command level. Consider whether this path resolution logic would be better encapsulated within the executor'sExecutePostRemoveHooksmethod, where it can handle the base-path semantics internally.This would:
- Reduce duplication if other commands need similar behavior
- Keep path resolution logic centralized in the hooks module
- Simplify the command-level code
internal/config/config_test.go (1)
449-481: Consider adding an empty slice test case for consistency.
TestHasPostCreateHooksincludes a test case for an empty hooks slice (lines 429-436), butTestHasPreRemoveHooksandTestHasPostRemoveHooksdon't include this edge case. For completeness and consistency, consider adding similar test cases.🔎 Suggested addition
{ name: "config without pre-remove hooks", config: &Config{Hooks: Hooks{}}, expected: false, }, + { + name: "config with empty pre-remove hooks slice", + config: &Config{ + Hooks: Hooks{ + PreRemove: []Hook{}, + }, + }, + expected: false, + }, }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
README.md(1 hunks)cmd/wtp/add.go(1 hunks)cmd/wtp/init.go(1 hunks)cmd/wtp/init_test.go(2 hunks)cmd/wtp/remove.go(4 hunks)cmd/wtp/remove_test.go(3 hunks)docs/architecture.md(1 hunks)internal/config/config.go(3 hunks)internal/config/config_test.go(6 hunks)internal/hooks/executor.go(7 hunks)internal/hooks/executor_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Follow standard Go style with tabs and gofmt; package names must be short and lowercase
Keep import groups tidy; use goimports for organization with local prefix following module pathgithub.com/satococoa/wtp/v2
Adhere to linting rules in.golangci.yml(vet, staticcheck, gosec, mnd, lll=120)
Wrap errors with context; avoid ignoring errors in Go code
Use snake_case for Go filenames (e.g.,remove.go,shell_integration.go)
Document exported items in Go when non-trivial; follow godoc conventions
Files:
cmd/wtp/remove.gocmd/wtp/init_test.gocmd/wtp/remove_test.gocmd/wtp/init.gointernal/hooks/executor_test.gointernal/config/config.gointernal/hooks/executor.gocmd/wtp/add.gointernal/config/config_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Unit tests must be placed alongside packages as*_test.gofiles in the same directory
Target 70% code coverage with unit tests focusing on fast feedback and mocked git interactions using table-driven test cases
Files:
cmd/wtp/init_test.gocmd/wtp/remove_test.gointernal/hooks/executor_test.gointernal/config/config_test.go
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Applies to .wtp.yml : Project hooks must be defined in `.wtp.yml`; keep commands deterministic and safe, avoiding destructive steps by default
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Shell integration must separate completion and hooks for clarity; `wtp cd` outputs absolute worktree path with no side effects
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Worktree naming convention: main worktree is rendered as `@`; non-main worktrees display path relative to `base_dir` (e.g., `.worktrees/feat/hoge` → `feat/hoge`)
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Run commands from inside a worktree to mimic real usage (e.g., `go run ../cmd/wtp add feature/new-feature`)
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Applies to .wtp.yml : Project hooks must be defined in `.wtp.yml`; keep commands deterministic and safe, avoiding destructive steps by default
Applied to files:
docs/architecture.mdcmd/wtp/remove.gocmd/wtp/init_test.goREADME.mdcmd/wtp/remove_test.gocmd/wtp/init.go
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Run commands from inside a worktree to mimic real usage (e.g., `go run ../cmd/wtp add feature/new-feature`)
Applied to files:
cmd/wtp/remove.gocmd/wtp/init_test.goREADME.mdcmd/wtp/remove_test.gocmd/wtp/init.gointernal/hooks/executor.go
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: CLI entrypoint is located at `cmd/wtp`; new command implementations must integrate with the main CLI structure
Applied to files:
cmd/wtp/remove.gocmd/wtp/remove_test.gocmd/wtp/init.gointernal/hooks/executor.gocmd/wtp/add.go
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Shell integration must separate completion and hooks for clarity; `wtp cd` outputs absolute worktree path with no side effects
Applied to files:
cmd/wtp/remove.gocmd/wtp/remove_test.gocmd/wtp/init.gointernal/hooks/executor.go
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Applies to **/*_test.go : Target 70% code coverage with unit tests focusing on fast feedback and mocked git interactions using table-driven test cases
Applied to files:
cmd/wtp/remove.gocmd/wtp/init_test.gocmd/wtp/remove_test.gointernal/hooks/executor_test.go
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: The root module is `github.com/satococoa/wtp/v2` running on Go 1.24; maintain module path consistency
Applied to files:
cmd/wtp/remove.gocmd/wtp/remove_test.go
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Applies to test/e2e/**/*.go : End-to-end tests must be placed in `test/e2e` and exercise real git workflows using the built binary
Applied to files:
cmd/wtp/init_test.gocmd/wtp/remove_test.go
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Applies to **/*.go : Keep import groups tidy; use goimports for organization with local prefix following module path `github.com/satococoa/wtp/v2`
Applied to files:
cmd/wtp/remove_test.gocmd/wtp/init.go
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Use `go run ./cmd/wtp <args>` for rapid feedback during development instead of building binaries
Applied to files:
cmd/wtp/remove_test.go
🧬 Code graph analysis (5)
cmd/wtp/remove.go (3)
internal/config/config.go (5)
LoadConfig(56-96)ConfigFileName(43-43)Config(13-17)Hook(32-39)Hooks(25-29)internal/errors/errors.go (1)
ConfigLoadFailed(198-224)internal/hooks/executor.go (1)
NewExecutor(29-34)
cmd/wtp/remove_test.go (2)
internal/command/types.go (2)
Result(11-15)Command(4-8)internal/config/config.go (5)
Config(13-17)Defaults(20-22)Hooks(25-29)Hook(32-39)HookTypeCommand(51-51)
internal/hooks/executor_test.go (3)
internal/hooks/executor.go (1)
NewExecutor(29-34)internal/config/config.go (5)
Config(13-17)Hooks(25-29)Hook(32-39)HookTypeCopy(49-49)HookTypeCommand(51-51)internal/command/types.go (1)
Command(4-8)
internal/hooks/executor.go (1)
internal/config/config.go (4)
Hooks(25-29)Hook(32-39)HookTypeCopy(49-49)HookTypeCommand(51-51)
internal/config/config_test.go (2)
internal/config/config.go (4)
Hooks(25-29)Hook(32-39)HookTypeCommand(51-51)Config(13-17)internal/command/types.go (1)
Command(4-8)
🔇 Additional comments (19)
cmd/wtp/init_test.go (1)
144-145: LGTM! Test assertions properly validate new hook sections.The test correctly verifies that both
pre_remove:andpost_remove:sections, along with their descriptive comments, are present in the generated configuration file.Also applies to: 159-160
cmd/wtp/init.go (1)
103-111: LGTM! Hook templates are well-structured and safe.The new hook sections follow the established pattern and use safe example commands (echo statements), aligning with the project's guideline to keep commands deterministic and safe by default. Based on learnings, project hooks must be defined in
.wtp.ymland should avoid destructive steps by default.README.md (1)
226-232: LGTM! Documentation clearly explains hook behavior and path resolution.The documentation effectively describes the path resolution semantics and working directory defaults for both pre_remove and post_remove hooks, with clear reasoning (e.g., "the worktree is gone" for post_remove).
cmd/wtp/remove_test.go (2)
302-347: LGTM! Comprehensive integration test for pre-remove hooks.The test properly creates a realistic scenario with temporary directories, config file, and mock executor. It verifies both the hook execution messaging and the actual hook output.
396-430: Good cross-platform test coverage.The test correctly handles platform differences by selecting
pwdfor Unix-like systems andcdfor Windows. This ensures the test validates working directory resolution across different operating systems.docs/architecture.md (1)
116-122: LGTM! Documentation accurately reflects the expanded hook system.The heading change from "Post-create hooks support" to "Hooks support" appropriately reflects the broader scope now that pre-remove and post-remove hooks are included.
Also applies to: 128-128
cmd/wtp/add.go (1)
336-336: LGTM! API update makes hook type checking more explicit.The change from
cfg.HasHooks()tocfg.HasPostCreateHooks()improves clarity by explicitly indicating which hook type is being checked, aligning with the introduction of multiple hook types (post-create, pre-remove, post-remove).internal/hooks/executor_test.go (2)
77-126: Excellent integration test validating pre-remove path semantics.This test effectively validates the key behavior described in the PR objectives:
- Copy hook's
frompath resolves relative to the worktree (source of "original.file")- Copy hook's
topath resolves relative to the repository root (destination "backup.file")- Command hook's
work_dirdefaults to running inside the worktreeThe test provides strong evidence that the path resolution semantics are working as designed.
27-32: LGTM! Comprehensive edge case coverage for new hook types.The nil config and empty hooks tests ensure that both
ExecutePreRemoveHooksandExecutePostRemoveHookshandle edge cases gracefully without errors.Also applies to: 46-56, 58-75
internal/config/config_test.go (2)
40-74: LGTM!Test cases appropriately extended to cover the new
pre_removeandpost_removehooks. The YAML content and assertions are consistent with the new hook structure.
176-187: LGTM!Valid configuration test case appropriately extended to include
PreRemoveandPostRemovehooks, ensuring comprehensive validation coverage.internal/config/config.go (3)
24-29: LGTM!Clean extension of the
Hooksstruct with newPreRemoveandPostRemovefields. The YAML tags follow the existing convention withomitemptyfor optional fields.
130-150: LGTM!The
validateHookshelper is a clean refactor that eliminates code duplication and provides consistent, informative error messages including the hook name and 1-based index.
176-189: LGTM!The three
Has*Hooksmethods provide a clean, consistent API for checking hook presence. The implementations are simple and correct.internal/hooks/executor.go (5)
36-49: LGTM!Clean refactoring of
ExecutePostCreateHooksto use the generalized base-path-aware helper. The path resolution correctly reflects the PR description:from= repo root,to= worktree.
51-65: LGTM!
ExecutePreRemoveHookscorrectly implements the path resolution described in the PR:frompaths resolve from the worktree (to extract files before removal), andtopaths resolve from the repository root.
82-107: LGTM!Clean generalized hook execution loop with helpful progress logging. The error wrapping includes the hook index for easier debugging.
127-151: LGTM!The copy hook implementation correctly handles separate source and destination base paths with appropriate security checks via
ensureWithinBase. The path traversal prevention aligns with the learning about keeping hooks deterministic and safe.
225-228: Verify environment variable semantics for post-remove hooks.
GIT_WTP_WORKTREE_PATHis set tobasePath, which for post-remove hooks would point to the (now removed) worktree directory. This is potentially useful for scripts needing to know which worktree was removed, but the variable name implies an existing path.Consider documenting this behavior or, if the path resolution fix above is applied, reviewing whether this variable should still reference the original worktree path or the current working directory.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @hayaishi. * #68 (comment) The following files were modified: * `cmd/wtp/add.go` * `cmd/wtp/init.go` * `cmd/wtp/remove.go` * `internal/config/config.go` * `internal/hooks/executor.go`
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/hooks/executor_test.go (1)
90-92: Consider adding cleanup for the created file.The file
srcFileis created with0644permissions. Whilet.TempDir()handles cleanup, explicitly using the octal literal0o644would be more consistent with thedirectoryPermissionsconstant style used elsewhere.🔎 Suggested change
- err = os.WriteFile(srcFile, []byte("test"), 0644) + err = os.WriteFile(srcFile, []byte("test"), 0o644)cmd/wtp/remove_test.go (1)
404-407: Minor: Variable shadows imported package name.The variable
commandshadows the importedgithub.com/satococoa/wtp/v2/internal/commandpackage. While this works since the package isn't used after this point, renaming improves clarity.🔎 Suggested rename
- command := "pwd" + cmdStr := "pwd" if runtime.GOOS == "windows" { - command = "cd" + cmdStr = "cd" } cfg := &config.Config{ Defaults: config.Defaults{ BaseDir: "../worktrees", }, Hooks: config.Hooks{ PostRemove: []config.Hook{ { Type: config.HookTypeCommand, - Command: command, + Command: cmdStr, WorkDir: "scripts", }, }, }, }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/wtp/remove.gocmd/wtp/remove_test.gointernal/hooks/executor.gointernal/hooks/executor_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Follow standard Go style with tabs and gofmt; package names must be short and lowercase
Keep import groups tidy; use goimports for organization with local prefix following module pathgithub.com/satococoa/wtp/v2
Adhere to linting rules in.golangci.yml(vet, staticcheck, gosec, mnd, lll=120)
Wrap errors with context; avoid ignoring errors in Go code
Use snake_case for Go filenames (e.g.,remove.go,shell_integration.go)
Document exported items in Go when non-trivial; follow godoc conventions
Files:
internal/hooks/executor_test.gointernal/hooks/executor.gocmd/wtp/remove_test.gocmd/wtp/remove.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Unit tests must be placed alongside packages as*_test.gofiles in the same directory
Target 70% code coverage with unit tests focusing on fast feedback and mocked git interactions using table-driven test cases
Files:
internal/hooks/executor_test.gocmd/wtp/remove_test.go
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Applies to .wtp.yml : Project hooks must be defined in `.wtp.yml`; keep commands deterministic and safe, avoiding destructive steps by default
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Shell integration must separate completion and hooks for clarity; `wtp cd` outputs absolute worktree path with no side effects
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Applies to **/*_test.go : Target 70% code coverage with unit tests focusing on fast feedback and mocked git interactions using table-driven test cases
Applied to files:
internal/hooks/executor_test.gocmd/wtp/remove_test.gocmd/wtp/remove.go
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Run commands from inside a worktree to mimic real usage (e.g., `go run ../cmd/wtp add feature/new-feature`)
Applied to files:
internal/hooks/executor.gocmd/wtp/remove_test.gocmd/wtp/remove.go
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Shell integration must separate completion and hooks for clarity; `wtp cd` outputs absolute worktree path with no side effects
Applied to files:
internal/hooks/executor.gocmd/wtp/remove_test.gocmd/wtp/remove.go
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: CLI entrypoint is located at `cmd/wtp`; new command implementations must integrate with the main CLI structure
Applied to files:
internal/hooks/executor.gocmd/wtp/remove_test.gocmd/wtp/remove.go
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Applies to test/e2e/**/*.go : End-to-end tests must be placed in `test/e2e` and exercise real git workflows using the built binary
Applied to files:
cmd/wtp/remove_test.go
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Applies to .wtp.yml : Project hooks must be defined in `.wtp.yml`; keep commands deterministic and safe, avoiding destructive steps by default
Applied to files:
cmd/wtp/remove_test.gocmd/wtp/remove.go
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Applies to **/*.go : Keep import groups tidy; use goimports for organization with local prefix following module path `github.com/satococoa/wtp/v2`
Applied to files:
cmd/wtp/remove_test.go
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: The root module is `github.com/satococoa/wtp/v2` running on Go 1.24; maintain module path consistency
Applied to files:
cmd/wtp/remove_test.gocmd/wtp/remove.go
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Use `go run ./cmd/wtp <args>` for rapid feedback during development instead of building binaries
Applied to files:
cmd/wtp/remove_test.go
🧬 Code graph analysis (4)
internal/hooks/executor_test.go (3)
internal/hooks/executor.go (1)
NewExecutor(29-34)internal/config/config.go (5)
Config(13-17)Hooks(25-29)Hook(32-39)HookTypeCopy(49-49)HookTypeCommand(51-51)internal/command/types.go (1)
Command(4-8)
internal/hooks/executor.go (1)
internal/config/config.go (4)
Hooks(25-29)Hook(32-39)HookTypeCopy(49-49)HookTypeCommand(51-51)
cmd/wtp/remove_test.go (2)
internal/command/types.go (2)
Result(11-15)Command(4-8)internal/config/config.go (5)
Config(13-17)Defaults(20-22)Hooks(25-29)Hook(32-39)HookTypeCommand(51-51)
cmd/wtp/remove.go (3)
internal/config/config.go (3)
LoadConfig(56-96)ConfigFileName(43-43)Config(13-17)internal/errors/errors.go (1)
ConfigLoadFailed(198-224)internal/hooks/executor.go (1)
NewExecutor(29-34)
🔇 Additional comments (18)
internal/hooks/executor.go (5)
36-49: LGTM! ExecutePostCreateHooks properly resolves paths.The base-path model correctly resolves:
- Copy source from repo root
- Copy destination to worktree
- Commands execute in worktree context
51-65: LGTM! ExecutePreRemoveHooks aligns with PR objectives.Pre-remove hooks correctly:
- Copy
frompaths resolved relative to worktree (source)- Copy
topaths resolved relative to repo root (destination)- Commands execute in worktree context (before removal)
This matches the documented behavior in the PR description.
67-101: LGTM! Post-remove path resolution issue has been addressed.The implementation now correctly:
- Resolves all paths relative to
e.repoRoot(lines 97-99)- Validates
WorkDirdoesn't escape repository root (lines 85-89)- Defaults empty
WorkDirto repo root (lines 77-79)This aligns with the PR objectives where post_remove runs after the worktree is deleted, so all paths must be repo-relative.
246-249: VerifyGIT_WTP_WORKTREE_PATHsemantics for post-remove hooks.For post-remove hooks,
basePathwill bee.repoRoot(not the removed worktree path). This meansGIT_WTP_WORKTREE_PATHwill equalGIT_WTP_REPO_ROOTfor post-remove hooks.Consider whether this is the intended behavior, or if post-remove hooks should not set
GIT_WTP_WORKTREE_PATHat all (since the worktree no longer exists).
306-319: LGTM! Synchronized writer prevents output interleaving.The
synchronizedWritercorrectly uses a mutex to prevent interleaved stdout/stderr output from concurrent goroutines streaming command output.internal/hooks/executor_test.go (4)
27-32: LGTM! Nil config handling test for PreRemove.Correctly verifies that
ExecutePreRemoveHooksreturns no error when the executor is created with a nil config.
46-56: LGTM! No-hooks scenario test for PreRemove.Verifies no-op behavior when PreRemove hooks slice is empty.
58-75: LGTM! Nil config and no-hooks tests for PostRemove.Tests correctly verify that
ExecutePostRemoveHookshandles nil config and empty hook slices gracefully.
77-126: LGTM! Comprehensive pre-remove path resolution test.This test validates the critical pre-remove behavior:
frompaths resolve relative to worktreetopaths resolve relative to repo rootWorkDirresolves relative to worktreeThe test creates actual files and directories, verifies the backup file lands in the repo root, and confirms command output contains the expected worktree-relative path.
cmd/wtp/remove.go (4)
134-143: LGTM! Config loading and pre-remove hook execution.The implementation correctly:
- Loads config from main repo path
- Returns a descriptive error if config loading fails
- Executes pre-remove hooks before the actual removal
163-165: LGTM! Post-remove hooks executed after successful removal.Post-remove hooks are correctly executed after the worktree is removed and the success message is printed.
177-196: LGTM! executePreRemoveHooks helper function.Clean implementation that:
- Short-circuits if no pre-remove hooks are configured
- Prints user-friendly status messages
- Properly propagates errors from hook execution
198-217: LGTM! executePostRemoveHooks helper function.The function signature correctly omits the worktree path since it no longer exists when post-remove hooks run. The work_dir validation has been moved to
ExecutePostRemoveHooksin executor.go, addressing the previous review comment.cmd/wtp/remove_test.go (5)
302-347: LGTM! Pre-remove hook execution test.This test validates the full integration:
- Creates a real
.wtp.ymlconfig file with a pre-remove command hook- Uses a mock command executor for git operations
- Verifies the expected output messages appear
349-394: LGTM! Post-remove hook execution test.Mirrors the pre-remove test structure and validates that post-remove hooks are executed after the worktree removal.
396-430: LGTM! WorkDir resolution test for post-remove hooks.This test validates that relative
WorkDirpaths are correctly resolved against the repository root. Cross-platform command selection (pwdvscd) ensures the test works on Windows.
432-459: LGTM! Path traversal security test.This test is critical for validating the security fix. It confirms that:
- Relative
work_dirvalues like../..are rejected- The specific error message is returned
- The command is never executed (verified by output not containing the echo text)
555-581: LGTM! Config load failure test.Validates that malformed YAML in
.wtp.ymlis properly detected and a descriptive error is returned.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/wtp/remove_test.go (1)
432-459: Important security test for path traversal prevention.This test validates a critical security boundary—ensuring hooks cannot escape the repository root via
..path components. The error message assertion usesfilepath.Join("..", "..")on both sides, ensuring cross-platform consistency.One minor observation: consider adding an additional test case with absolute paths outside the repo (e.g.,
/etcorC:\Windows) to strengthen the security validation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/wtp/remove_test.gointernal/hooks/executor_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Follow standard Go style with tabs and gofmt; package names must be short and lowercase
Keep import groups tidy; use goimports for organization with local prefix following module pathgithub.com/satococoa/wtp/v2
Adhere to linting rules in.golangci.yml(vet, staticcheck, gosec, mnd, lll=120)
Wrap errors with context; avoid ignoring errors in Go code
Use snake_case for Go filenames (e.g.,remove.go,shell_integration.go)
Document exported items in Go when non-trivial; follow godoc conventions
Files:
internal/hooks/executor_test.gocmd/wtp/remove_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Unit tests must be placed alongside packages as*_test.gofiles in the same directory
Target 70% code coverage with unit tests focusing on fast feedback and mocked git interactions using table-driven test cases
Files:
internal/hooks/executor_test.gocmd/wtp/remove_test.go
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Applies to .wtp.yml : Project hooks must be defined in `.wtp.yml`; keep commands deterministic and safe, avoiding destructive steps by default
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Shell integration must separate completion and hooks for clarity; `wtp cd` outputs absolute worktree path with no side effects
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Worktree naming convention: main worktree is rendered as `@`; non-main worktrees display path relative to `base_dir` (e.g., `.worktrees/feat/hoge` → `feat/hoge`)
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Applies to **/*_test.go : Target 70% code coverage with unit tests focusing on fast feedback and mocked git interactions using table-driven test cases
Applied to files:
internal/hooks/executor_test.gocmd/wtp/remove_test.go
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: CLI entrypoint is located at `cmd/wtp`; new command implementations must integrate with the main CLI structure
Applied to files:
cmd/wtp/remove_test.go
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Run commands from inside a worktree to mimic real usage (e.g., `go run ../cmd/wtp add feature/new-feature`)
Applied to files:
cmd/wtp/remove_test.go
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Applies to test/e2e/**/*.go : End-to-end tests must be placed in `test/e2e` and exercise real git workflows using the built binary
Applied to files:
cmd/wtp/remove_test.go
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: The root module is `github.com/satococoa/wtp/v2` running on Go 1.24; maintain module path consistency
Applied to files:
cmd/wtp/remove_test.go
📚 Learning: 2025-12-02T13:33:48.693Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Use `go run ./cmd/wtp <args>` for rapid feedback during development instead of building binaries
Applied to files:
cmd/wtp/remove_test.go
🧬 Code graph analysis (1)
cmd/wtp/remove_test.go (2)
internal/command/types.go (2)
Result(11-15)Command(4-8)internal/config/config.go (5)
Config(13-17)Defaults(20-22)Hooks(25-29)Hook(32-39)HookTypeCommand(51-51)
🔇 Additional comments (8)
cmd/wtp/remove_test.go (4)
302-347: Well-structured integration test for pre-remove hooks.Good test coverage that validates:
- Config file loading from disk
- Pre-remove hook execution messaging
- Hook output capture in the buffer
The test properly uses
t.TempDir()for automatic cleanup and creates realistic directory structures.
349-394: LGTM for post-remove hook integration test.Mirrors the pre-remove test structure appropriately. The test validates that post-remove hooks execute after worktree removal with correct messaging.
396-430: Good cross-platform handling for work_dir test.The use of
runtime.GOOSto selectpwdvscdensures the test works on both Unix and Windows platforms. The test correctly validates thatwork_diris resolved relative to the repository root for post-remove hooks.
555-581: Good error handling test for malformed config.The test validates that YAML parsing errors are properly propagated and the removal operation fails gracefully when the config file is invalid. The malformed YAML at line 562 (
invalidat the end without proper structure) is a realistic error case.internal/hooks/executor_test.go (4)
27-32: LGTM for nil config safety test.This test ensures
ExecutePreRemoveHookshandles nil config gracefully without panicking, which is important for defensive programming.
46-56: Good edge case coverage for empty hooks array.Tests that an empty hooks slice (as opposed to nil) is handled correctly, returning no error without attempting any operations.
58-75: LGTM for post-remove nil/empty config tests.Note that
ExecutePostRemoveHookscorrectly doesn't require a worktree path parameter since the worktree is already removed at that point.
77-126: Comprehensive integration test correctly validating pre-remove path resolution semantics.This test effectively validates the key behavior:
frompaths resolved relative to the worktree (line 104:"original.file"from worktree)topaths resolved relative to repository root (line 105:"backup.file"to repo root)work_dirresolved relative to the worktree for command execution (lines 110, 124)The cross-platform handling at lines 94-97 is correct for validating the working directory via
pwd/cd. Per the documentation, command hooks inpre_removedefault to running inside the worktree, which this test correctly validates.
1 participant
Thanks for developing and maintaining this tool — it has made my
git worktreeworkflows much smoother compared to usinggit worktreedirectly 🙇♂️This PR introduces
pre_remove/post_removehooks to.wtp.yml, enabling users to run cleanup and lifecycle automation aroundgit worktree remove.✨ Motivation
When removing a worktree, users often want to automate tasks such as:
These hooks allow these workflows to be handled consistently within
wtp.🔧 How to Use
📌 Behavior — Path Resolution
pre_removegit worktree removefrompaths are resolved relative to the worktreetopaths are resolved relative to the repository rootwork_dirdefaults to repository rootpost_removegit worktree removefrom,to, andwork_dir) are resolved relative to the repository rootpre_removepost_create(unchanged)frompaths are resolved relative to the repository roottopaths are resolved relative to the worktree🔍 Base Path Summary
fromBase PathtoBase PathHappy to adjust naming, behavior, or docs if you prefer 🙏
If this does not align with the project's direction, please feel free to close the PR.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.