chore: harden lint configuration and restore checks #55
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. WalkthroughAdds pervasive I/O error handling across CLI commands and hooks, introduces repository/worktree public APIs, hardens hook file-copying with path and permission checks, expands centralized error helpers, updates tooling/formatting config, and enhances the e2e test framework with lifecycle utilities and assertion helpers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
There was a problem hiding this comment.
Pull request overview
This PR hardens the linting configuration by removing errcheck/gosec exclusions and enforces comprehensive error handling across the codebase. It also adds path validation for hook operations, improves test reliability by replacing platform-specific permission tests with file/directory conflicts, and establishes a consistent formatter workflow.
- Removed linter exclusions for fmt output functions and security warnings, then propagated all fmt.Fprint* errors
- Added path safety checks (ensureWithinBase, ensureDirWritable) and #nosec comments with justifications for validated file operations
- Configured dynamic goimports local-prefixes via Taskfile using
go list -mfor fork compatibility
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
test/e2e/shell_test.go |
Replaced os.Setenv/Unsetenv with t.Setenv for cleaner test isolation |
test/e2e/framework/framework.go |
Added package and exported function documentation; changed receiver to pointer-less for stateless method |
test/e2e/framework/assertions.go |
Added documentation for all exported assertion helpers |
internal/hooks/executor_test.go |
Replaced permission-based tests with file/directory conflict scenarios for cross-platform reliability |
internal/hooks/executor.go |
Added path traversal prevention (ensureWithinBase), permission checks (ensureDirWritable), and propagated fmt errors with #nosec justifications |
internal/git/worktree.go |
Added package and constant documentation |
internal/git/repository.go |
Added documentation for exported types and functions |
internal/errors/errors.go |
Added documentation for all error constructor functions |
internal/config/config.go |
Added path cleaning/validation in LoadConfig and documented constants |
internal/command/shell.go |
Changed receiver to pointer-less for stateless Execute method |
internal/command/builders.go |
Added package documentation |
cmd/wtp/shell_init.go |
Added shell validation map, #nosec comment for validated exec, and propagated fmt errors |
cmd/wtp/remove.go |
Propagated fmt.Fprintf/Fprintln errors throughout removal flow |
cmd/wtp/main.go |
Explicitly ignored error in final stderr write before exit |
cmd/wtp/list.go |
Propagated all fmt output errors in list/display functions |
cmd/wtp/init_test.go |
Replaced chmod-based write failure with mocked writeFile for reliable testing |
cmd/wtp/init.go |
Added permission checks before config write and propagated fmt errors |
cmd/wtp/hook_test.go |
Added require.NoError for hook print functions |
cmd/wtp/hook.go |
Changed hook print functions to return errors instead of void |
cmd/wtp/flag_completion.go |
Propagated fmt.Fprintln errors in completion output |
cmd/wtp/cd_test.go |
Used require.NoError in cleanup to ensure test isolation |
cmd/wtp/cd.go |
Propagated fmt output errors in cd command and helper functions |
cmd/wtp/add_test.go |
Added require.NoError for display message calls |
cmd/wtp/add.go |
Added package comment and propagated fmt errors in success message display |
Taskfile.yml |
Added dynamic GOIMPORTS_LOCAL_PREFIXES derivation using go list -m |
README.md |
Documented formatter workflow and task-based approach for contributors |
.golangci.yml |
Removed errcheck/gosec exclusions, added revive rules for exported/unused, configured environment-based goimports prefix |
internal/io/flusher.go |
Added package documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,3 +1,4 @@ | |||
| // Package framework provides helpers for end-to-end testing. | |||
There was a problem hiding this comment.
The package comment differs between framework.go ("contains helpers for constructing repositories") and assertions.go ("provides helpers for end-to-end testing"). Package comments should be consistent across all files in the same package. Consider using the same comment text or removing the duplicate, keeping only one package comment (typically in the main file of the package).
| // Package framework provides helpers for end-to-end testing. |
| # is bypassed. | ||
| local-prefixes: | ||
| - github.com/satococoa/wtp/v2 | ||
| - ${GOIMPORTS_LOCAL_PREFIXES:-github.com/satococoa/wtp/v2} |
There was a problem hiding this comment.
The environment variable syntax ${GOIMPORTS_LOCAL_PREFIXES:-github.com/satococoa/wtp/v2} uses shell-like parameter expansion, but golangci-lint's YAML configuration may not support this syntax. This could result in the literal string being used instead of the intended environment variable substitution. Consider verifying that golangci-lint actually supports this syntax, or using a different approach to make the configuration dynamic.
| - ${GOIMPORTS_LOCAL_PREFIXES:-github.com/satococoa/wtp/v2} | |
| - github.com/satococoa/wtp/v2 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/errors/errors.go (1)
313-326: Potential panic ifremotesslice is empty.Line 319 accesses
remotes[0]unconditionally. While the function name implies multiple branches were found (suggesting at least one remote), an empty slice would cause a panic.Consider adding a defensive check:
func MultipleBranchesFound(branchName string, remotes []string) error { msg := fmt.Sprintf("branch '%s' exists in multiple remotes: %s", branchName, strings.Join(remotes, ", ")) + if len(remotes) == 0 { + return errors.New(msg) + } msg += fmt.Sprintf(` Solution: Specify the remote explicitly: • wtp add --track %s/%s %s`, remotes[0], branchName, branchName)test/e2e/framework/framework.go (1)
341-353:WithTimeoutfunction is incomplete and has incorrect usage pattern.The function starts and waits for the command internally, then returns a closure that does nothing useful. The caller receives a function that's meant to be applied to a command, but by the time it's called, the command has already been executed. This pattern doesn't allow the caller to use the timeout correctly.
Consider restructuring to either:
- Return a configured command that the caller can execute, or
- Execute the command and return the result directly
-// WithTimeout adds a timeout to an exec command for use in helpers. -func WithTimeout(timeout time.Duration) func(cmd *exec.Cmd) { - return func(cmd *exec.Cmd) { - timer := time.AfterFunc(timeout, func() { - if cmd.Process != nil { - _ = cmd.Process.Kill() - } - }) - _ = cmd.Start() - _ = cmd.Wait() - timer.Stop() - } +// RunWithTimeout executes a command with the specified timeout and returns combined output. +func RunWithTimeout(cmd *exec.Cmd, timeout time.Duration) ([]byte, error) { + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + // Create a new command with context + ctxCmd := exec.CommandContext(ctx, cmd.Path, cmd.Args[1:]...) + ctxCmd.Dir = cmd.Dir + ctxCmd.Env = cmd.Env + + return ctxCmd.CombinedOutput() }Alternatively, use
exec.CommandContextfor built-in timeout support via context cancellation.cmd/wtp/add.go (1)
252-259: EmptygitOutputbeing categorized as an invalid path error may be misleading.When git exits with an error but produces no output, the root cause could be various issues (network problems, corruption, etc.), not necessarily an invalid path. This could lead to confusing error messages for users.
Consider removing the empty output check or handling it as a separate generic error case:
func isInvalidPathError(errorOutput, workTreePath, gitOutput string) bool { return strings.Contains(errorOutput, "could not create directory") || strings.Contains(errorOutput, "unable to create") || strings.Contains(errorOutput, "is not a directory") || strings.Contains(errorOutput, "fatal:") || - strings.Contains(workTreePath, "/dev/") || - gitOutput == "" + strings.Contains(workTreePath, "/dev/") }
🧹 Nitpick comments (11)
cmd/wtp/flag_completion.go (1)
49-56: Completion now fails fast on write errorsChecking
fmt.Fprintlnand returningfalseon error is a reasonable way to satisfy errcheck and avoid silently ignoring I/O failures, given the boolean API. If you ever need to distinguish “no matches” from “writer error”, you’d have to plumb an error out of this path, but that’s optional and would be a broader API change.cmd/wtp/cd_test.go (1)
10-12: Stronger env handling in tests; t.Setenv is an optional simplificationUsing
t.Cleanuptogether withrequire.NoErroraroundos.Setenv/os.Unsetenvmakes the WTP_SHELL_INTEGRATION handling much safer and more explicit. As a minor future improvement, you could considert.Setenvin the subtests to avoid manual cleanup, but the current pattern is perfectly acceptable.Also applies to: 86-94, 106-112
cmd/wtp/hook_test.go (1)
134-139: Correct error handling for hook print functions.The
require.NoErrorwrapping aligns with the updated signatures ofprintBashHookandprintFishHookthat now return errors.Consider adding a test case for
printZshHookedge cases for consistency, though this is a minor suggestion since zsh likely shares similar logic with bash.cmd/wtp/init.go (1)
52-63: Potential duplication: consider consolidating permission checks.The permission check here (lines 57-63) appears to duplicate the logic in ensureWritableDirectory (lines 126-141), which is called later at line 104. Consider removing this early check since ensureWritableDirectory performs the same validation.
If you prefer to keep the early check for fail-fast behavior, add a comment explaining why both checks are needed.
internal/git/worktree.go (1)
81-96: Consider simplifying or documenting the redundant path check.When
IsMainis true, the method returns immediately. The subsequent path comparison (lines 89-91) can only be reached whenIsMainis false, which means it's checking if the path matches despite the flag being unset. This handles legacy data or cases whereIsMainwasn't populated, but the comment on line 93-95 could be more explicit about when this scenario occurs.// IsMainWorktree returns true if this is the main/root worktree func (w *Worktree) IsMainWorktree(mainWorktreePath string) bool { - // If IsMain flag is set, use it (set by GetWorktrees) if w.IsMain { return true } - // If mainWorktreePath is provided, compare paths + // Fallback: compare paths when IsMain flag wasn't populated (e.g., manually created Worktree) if mainWorktreePath != "" { return w.Path == mainWorktreePath } - // This shouldn't happen in normal usage since we always provide mainWorktreePath - // But if it does, we can't determine without more context return false }internal/hooks/executor.go (1)
267-270: TheensureDirWritablecheck may fail for newly created directories.In
executeCopyHookWithWriter(lines 107-110),os.MkdirAllcreates the destination directory. However, here incopyFile,ensureDirWritableis called on the parent directory. If the parent was just created byMkdirAllin the caller, this should work. But ifcopyFileis called directly without priorMkdirAll, this could fail unexpectedly.This appears to be fine given the current call flow, but consider adding a brief comment clarifying that the caller is expected to ensure the parent directory exists.
test/e2e/framework/assertions.go (2)
98-106: Minor redundancy in file existence check.The
assert.Trueon line 101 already fails the test if the file doesn't exist, making theif repo.HasFile(path)guard on line 102 redundant in the failure case. The guard only prevents reading from a non-existent file after the test has already failed.This is fine as-is for safety, but could be simplified:
func AssertFileContains(t *testing.T, repo *TestRepo, path, content string) { t.Helper() - assert.True(t, repo.HasFile(path), "File '%s' does not exist", path) - if repo.HasFile(path) { + if !repo.HasFile(path) { + t.Fatalf("File '%s' does not exist", path) + } - fileContent := repo.ReadFile(path) - assert.Contains(t, fileContent, content, "Expected file '%s' to contain '%s', got: %s", path, content, fileContent) - } + fileContent := repo.ReadFile(path) + assert.Contains(t, fileContent, content, "Expected file '%s' to contain '%s', got: %s", path, content, fileContent) }
149-171: Consider whether thin wrappers add value.
AssertEqual,AssertNotEqual,AssertTrue, andAssertFalseare thin wrappers around testify functions. While they provide consistency within this framework, they don't add significant functionality. This is fine if the goal is a unified assertion API, but tests could also useassert.*directly.test/e2e/framework/framework.go (1)
355-373: Consider adding quotes and backslash to dangerous characters.The validation blocklist doesn't include
",', or\, which could potentially be used for escape sequences or quoting issues in edge cases. While this is test code with controlled inputs, adding these characters would make the validation more robust.// Check for shell metacharacters that could be dangerous // Note: { and } are allowed for branch names like branch@{upstream} - dangerousChars := []string{";", "&", "|", "$", "`", "(", ")", "<", ">", "\n", "\r"} + dangerousChars := []string{";", "&", "|", "$", "`", "(", ")", "<", ">", "\n", "\r", "\"", "'", "\\"}cmd/wtp/add.go (2)
110-121: Hook warning message is swallowed when write fails, but subsequent error is returned.The error handling logic here returns
warnErrfrom line 112 if writing the warning message fails, but this means the original hook error (err) is lost. Consider whether you want to:
- Log both errors, or
- Prioritize the hook error over the write error
Current behavior prioritizes the write error, which may be counterintuitive for debugging hook failures.
If both errors matter, consider wrapping them:
if err := executePostCreateHooks(w, cfg, mainRepoPath, workTreePath); err != nil { if _, warnErr := fmt.Fprintf(w, "Warning: Hook execution failed: %v\n", err); warnErr != nil { - return warnErr + return fmt.Errorf("hook execution failed: %w (also failed to write warning: %v)", err, warnErr) } }
244-246: Overly broad pattern match may cause false categorization.
isPathAlreadyExistsErrorwill match any error containing "already exists", which overlaps with worktree and branch error patterns. This works correctly only because of the check order inanalyzeGitWorktreeError. Consider adding a more specific pattern or documenting this dependency.func isPathAlreadyExistsError(errorOutput string) bool { - return strings.Contains(errorOutput, "already exists") + // Must be checked after isWorktreeAlreadyExistsError and isBranchAlreadyExistsError + // as this pattern is broader and would match those cases too + return strings.Contains(errorOutput, "already exists") && + !strings.Contains(errorOutput, "already checked out") && + !strings.Contains(errorOutput, "already used by worktree") && + !(strings.Contains(errorOutput, "branch") && strings.Contains(errorOutput, "already exists")) }Alternatively, add a comment documenting the ordering requirement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.golangci.yml(2 hunks)README.md(1 hunks)Taskfile.yml(1 hunks)cmd/wtp/add.go(9 hunks)cmd/wtp/add_test.go(6 hunks)cmd/wtp/cd.go(3 hunks)cmd/wtp/cd_test.go(3 hunks)cmd/wtp/flag_completion.go(1 hunks)cmd/wtp/hook.go(4 hunks)cmd/wtp/hook_test.go(2 hunks)cmd/wtp/init.go(5 hunks)cmd/wtp/init_test.go(1 hunks)cmd/wtp/list.go(3 hunks)cmd/wtp/main.go(1 hunks)cmd/wtp/remove.go(4 hunks)cmd/wtp/shell_init.go(4 hunks)internal/command/builders.go(1 hunks)internal/command/shell.go(1 hunks)internal/config/config.go(3 hunks)internal/errors/errors.go(19 hunks)internal/git/repository.go(5 hunks)internal/git/worktree.go(2 hunks)internal/hooks/executor.go(9 hunks)internal/hooks/executor_test.go(2 hunks)internal/io/flusher.go(1 hunks)test/e2e/framework/assertions.go(7 hunks)test/e2e/framework/framework.go(10 hunks)test/e2e/shell_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
cmd/wtp/init.go (1)
internal/errors/errors.go (2)
ConfigAlreadyExists(237-245)DirectoryAccessFailed(248-272)
cmd/wtp/cd.go (2)
internal/git/worktree.go (1)
Worktree(18-23)internal/config/config.go (1)
Config(13-17)
cmd/wtp/list.go (2)
internal/git/worktree.go (1)
Worktree(18-23)internal/config/config.go (1)
Config(13-17)
test/e2e/framework/assertions.go (1)
test/e2e/framework/framework.go (1)
TestRepo(217-220)
internal/git/repository.go (1)
internal/errors/errors.go (1)
NotInGitRepository(11-19)
cmd/wtp/add.go (3)
internal/hooks/executor.go (1)
NewExecutor(29-34)internal/command/executor.go (1)
NewExecutor(9-13)internal/config/config.go (1)
Config(13-17)
⏰ 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). (2)
- GitHub Check: Agent
- GitHub Check: E2E Tests (macos-latest)
🔇 Additional comments (69)
internal/command/builders.go (1)
1-1: Package comment addition looks goodComment accurately describes the package and aligns with the new revive
package-commentsrule. No behavioral impact.internal/io/flusher.go (1)
1-1: Package doc is consistent and clearThe new package comment matches the helpers provided here and should satisfy the new revive
package-commentsrule without changing behavior.internal/command/shell.go (1)
16-25: Unnamed receiver is an appropriate lint-friendly tweakDropping the receiver name avoids revive’s
unused-receivercomplaint while preserving Execute’s behavior. No functional change.cmd/wtp/main.go (1)
28-32: Explicitly discarding Fprintf return values is fine hereAssigning to
_, _satisfies errcheck without altering behavior; ignoring write errors to stderr inmainis reasonable for this CLI.README.md (1)
392-398: Formatting section is clear and aligned with toolingThe new “Formatting” section clearly explains the
go tool task fmtworkflow and how goimports’ local prefix is derived, matching the linter/task changes..golangci.yml (1)
73-80: Stronger revive rules match the rest of the hardening workEnabling
exported,package-comments,unused-parameter, andunused-receiveris consistent with the added package/type comments and small receiver/param tweaks across the codebase. Expect a bit more lint churn initially, but overall this is a solid tightening.Taskfile.yml (1)
72-79: LGTM - Robust module path detection with fallback.The fmt task correctly detects the Go module path dynamically and falls back to a hardcoded value when detection fails. Setting
GOIMPORTS_LOCAL_PREFIXESensures consistent import grouping.Minor note:
|| echo ""is technically redundant since command substitution returns empty on failure, but it doesn't cause any issues.test/e2e/shell_test.go (3)
54-54: Good improvement usingt.Setenvfor test environment management.Using
t.Setenvinstead of manualos.Setenv/defer os.Unsetenvis the idiomatic Go approach. It ensures automatic cleanup when the subtest completes, improving test isolation and reducing boilerplate.
135-135: Consistent use oft.Setenv.
152-152: Consistent use oft.Setenv.cmd/wtp/shell_init.go (3)
13-22: Good security hardening with shell whitelist validation.The
allowedShellswhitelist prevents command injection by restricting the shell parameter to known-safe values before passing it toexec.Command. The error message is clear and actionable.
30-31: Appropriate#nosecannotation with clear justification.The G204 suppression is valid since:
execomes fromos.Executable()(the running binary)shellis validated against theallowedShellswhitelist
81-85: Correct error propagation for I/O operations.The pattern of checking
fmt.Fprintlnerror before calling the hook printer, and returning the hook printer's result directly, ensures write failures are properly surfaced to callers. This is consistently applied across all shell init functions.cmd/wtp/add_test.go (3)
9-9: Good addition ofrequireimport for error assertions.Adding
requirealongside existingassertfollows the testify best practice: userequirefor errors that should halt the test, andassertfor softer checks where the test can continue.
754-754: Correct use ofrequire.NoErrorfor error-returning function.Since
displaySuccessMessagenow returns an error, wrapping calls withrequire.NoErrorensures tests fail fast if the function unexpectedly errors. This is the appropriate choice since subsequent assertions depend on successful execution.
833-833: Consistent error handling fordisplaySuccessMessageWithCommitish.cmd/wtp/hook_test.go (1)
9-9: Appropriate addition ofrequireimport.internal/config/config.go (3)
1-1: LGTM!The expanded documentation improves clarity for package users and constant usage.
Also applies to: 40-50
55-63: LGTM!The path normalization logic properly handles both relative and absolute paths, improving security and consistency. The error handling for path resolution is clear and appropriate.
77-78: LGTM!The nosec comment is well-justified since the path is constructed from a validated repository root and a fixed constant filename, mitigating path traversal risks.
cmd/wtp/init_test.go (1)
186-190: LGTM!The test improvement using dependency injection (writeFile override) is cleaner and more portable than the previous permission-based approach. This provides reliable error simulation across platforms.
cmd/wtp/hook.go (2)
46-68: LGTM!The hook handlers now properly propagate write errors from the print functions instead of unconditionally returning nil, improving error visibility.
70-154: LGTM!All print functions now properly capture and propagate write errors, ensuring that failures in generating shell hooks are visible to the caller rather than silently ignored.
cmd/wtp/remove.go (2)
147-149: LGTM!Status message writes now properly check for errors and propagate them, ensuring that failures in writing success messages are not silently ignored.
Also applies to: 207-208
340-342: LGTM!Completion functions now properly handle write errors, ensuring consistency with error propagation throughout the command.
Also applies to: 383-385
internal/git/repository.go (4)
1-1: LGTM!The expanded documentation clarifies the package purpose and the Repository struct's role in worktree operations.
Also applies to: 14-17
19-30: LGTM!The constructor properly validates that the path is a git repository before creating the Repository instance, and the Path() accessor provides clean access to the repository root.
77-94: LGTM!The GetWorktrees method correctly uses the porcelain format for stable parsing and properly marks the first worktree as the main worktree, following git's behavior.
96-126: LGTM!The CreateWorktree and RemoveWorktree methods provide clean wrappers around git commands with proper parameter handling and consistent error propagation.
cmd/wtp/cd.go (4)
128-131: LGTM!The target path write now properly checks for errors and propagates them, ensuring write failures are visible to the caller.
293-298: LGTM!The refactoring into helper functions improves code organization and maintainability while properly propagating errors from both helpers.
300-344: LGTM!The new helper functions cleanly separate the logic for writing main and managed worktrees, with consistent error checking for all write operations.
392-394: LGTM!The completion function now properly handles write errors, ensuring consistency with error propagation throughout the command.
internal/hooks/executor_test.go (3)
860-870: LGTM!The test now uses a filesystem conflict (file where directory is expected) to trigger the error, making it more portable and reliable than permission-based testing.
877-886: LGTM!Creating a file instead of a directory reliably triggers the ReadDir failure without relying on platform-specific permission handling.
937-951: LGTM!The test uses a path conflict (directory where file is expected) to reliably trigger the copy failure, avoiding platform-specific permission issues.
cmd/wtp/init.go (4)
7-7: LGTM!The filepath import and writeFile abstraction enable better testability and align with the test improvements seen in init_test.go.
Also applies to: 20-20
104-111: LGTM!The ensureWritableDirectory check before writing the config file adds good safety, and the use of the writeFile abstraction enables testability.
119-124: LGTM!The print statements now properly check for errors and propagate them, ensuring write failures are visible rather than silently ignored.
126-141: LGTM!The ensureWritableDirectory helper provides clean validation with clear error messages, checking directory existence, type, and write permissions.
cmd/wtp/list.go (4)
149-154: LGTM! Error propagation for empty worktree case.The error handling for the "No worktrees found" message now correctly propagates I/O errors to the caller.
157-175: LGTM! Consistent error propagation from display helpers.Both
displayWorktreesQuietanddisplayWorktreesRelativenow return errors, and their results are properly checked and propagated. This aligns with the PR's goal of propagating formatter output errors across the CLI.
274-283: LGTM! Clean error propagation in quiet mode.The function signature update to return
errorand the per-line error checking are well-implemented. The early return on write failure prevents unnecessary work.
286-335: LGTM! Comprehensive error handling in table display.All
fmt.Fprintfcalls for headers, dividers, and data rows now check for errors and return early on failure. The pattern is consistent and thorough.internal/git/worktree.go (3)
1-28: LGTM! Good documentation and clean struct methods.The package comment, constant documentation, and the
Name()method are well-implemented. Documentation improvements align with best practices.
46-54: LGTM! Clean completion name formatting for root worktrees.The
CompletionNamemethod correctly handles root worktrees with a distinct format and delegates to the helper for non-root cases.
56-79: LGTM! Well-documented priority logic for completion formatting.The
formatNonRootWorktreeCompletionhelper has clear priority handling:
- Path suffix match → branch only
- Directory name match → branch only
- Otherwise → "directory@branch"
The empty branch fallback to directory name is sensible.
internal/hooks/executor.go (6)
1-1: LGTM! Error-checked hook progress messages.The package comment and error propagation for hook start/completion messages are well-implemented.
Also applies to: 45-56
75-98: LGTM! Solid path traversal prevention.The path safety checks using
ensureWithinBasefor both source (relative to repo root) and destination (relative to worktree path) correctly prevent directory traversal attacks via../sequences in hook configurations.
125-136: LGTM! Correct path containment validation.The
ensureWithinBasehelper properly usesfilepath.Reland checks for..prefix patterns to detect path escapes. The error messages are clear and actionable.
248-256: Permission check may not work correctly on Windows.The bitmask check
srcInfo.Mode().Perm()&0o400 == 0relies on Unix permission semantics. On Windows,os.FileMode.Perm()returns a simplified permission model where this check may not accurately reflect readability.Consider guarding this with a runtime check or documenting the limitation:
+ // Note: Permission checks may not accurately reflect access on Windows if srcInfo.Mode().Perm()&0o400 == 0 { return fmt.Errorf("failed to copy file: source file is not readable") }Alternatively, the existing
os.Opencall on line 259 will fail with a permission error if the file isn't readable, providing a natural fallback.
299-306: Same Windows permission caveat applies here.The read permission check
srcInfo.Mode().Perm()&0o400 == 0and the subsequent write permission check viaensureDirWritablehave the same Windows compatibility considerations as noted forcopyFile.
335-350: LGTM! Clean directory writability helper.The
ensureDirWritablefunction correctly validates that the path exists, is a directory, and has write permission. The error messages are descriptive.test/e2e/framework/assertions.go (2)
1-23: LGTM! Good package comment and worktree creation assertion.The package documentation and the
AssertWorktreeCreatedfunction are well-structured with propert.Helper()usage.
25-72: LGTM! Comprehensive assertion helpers.The error and output assertion helpers are well-designed:
AssertErrorContainsvalidates both error presence and contentAssertHelpfulErrorprovides useful validation for user-facing error qualityAssertMultipleStringsInOutputhandles batch string checks cleanlyinternal/errors/errors.go (3)
1-35: LGTM! Well-structured error helpers with actionable guidance.The package comment and the first two error constructors demonstrate the consistent pattern: clear problem statement, contextual solutions, and helpful tips. The output cleanup in
GitCommandFailedis a nice touch.
88-103: LGTM! Helpful worktree-not-found error with available options.The error message dynamically includes available worktrees when provided, giving users immediate actionable information.
328-361: LGTM! Comprehensive hook execution error handling.The
HookExecutionFailedfunction provides excellent context-specific suggestions based on the error type (permission denied, file not found, command not found). The 1-indexed hook number in the message is user-friendly.test/e2e/framework/framework.go (6)
375-379: LGTM!The function appropriately separates command creation from validation, with clear documentation that validation occurs during initialization.
222-238: LGTM!Good isolation approach by setting
HOMEto the temp directory, ensuring tests don't interact with the actual user's home directory. The argument validation before command execution is a sensible safety measure.
158-171: LGTM!The environment-level
RunWTPcorrectly omits directory/HOME configuration, as it's meant for commands that don't require a specific repository context. The argument validation is consistent withTestRepo.RunWTP.
293-302: LGTM!The
#nosec G304annotation is appropriate here since the file paths are constructed from the controlled test repository path. The helper correctly fatals on read errors, which is suitable for test code.
43-73: LGTM!Good security hygiene: the
#nosec G204annotation is well-justified for building test binaries, and the subsequent path validation ensures the binary path is clean and absolute before use.
198-202: LGTM!Using a blank receiver
(*TestEnvironment)is idiomatic Go when the method doesn't need receiver state but you want to keep it as a method for API consistency.cmd/wtp/add.go (5)
335-351: LGTM!Good error propagation pattern for I/O operations. The hook execution flow is clear with appropriate status messages and proper error handling at each step.
438-457: LGTM!Silent error handling is appropriate for shell completion functions. Using
fmt.Printlnto stdout is correct here since completion output must go to stdout regardless of any configured writer.
469-507: LGTM!Thorough error propagation for all write operations. The success message provides clear, actionable guidance with the generated
wtp cdcommand.
387-436: LGTM!Good branch listing implementation with proper deduplication and filtering of HEAD references. Error handling for write operations is correctly propagated.
319-333: The--trackflag is missing from the command definition, not an error message issue.The review comment correctly identifies that the error message references
--track, but incorrectly concludes the error message should be changed. The tests inadd_test.go(lines 521, 659, 689) confirm the--trackflag is an intended feature. The actual issue is that the flag definition is missing from theFlagsslice inadd.go(lines 38-45).The fix should add the missing flag to the command definition:
Flags: []cli.Flag{ &cli.StringFlag{ Name: "branch", Usage: "Create new branch", Aliases: []string{"b"}, }, &cli.StringFlag{ Name: "track", Usage: "Specify which remote to track", Aliases: []string{"t"}, }, },Likely an incorrect or invalid review comment.
| goimports: | ||
| # Task-based formatting sets GOIMPORTS_LOCAL_PREFIXES via `go list -m`. | ||
| # The default keeps CI and ad-hoc runs deterministic if the task runner | ||
| # is bypassed. | ||
| local-prefixes: | ||
| - github.com/satococoa/wtp/v2 | ||
| - ${GOIMPORTS_LOCAL_PREFIXES:-github.com/satococoa/wtp/v2} | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
golangci-lint ${ENV:-default} syntax configuration environment variable expansion
💡 Result:
Short answer: golangci-lint does not perform shell-style environment expansion like ${ENV:-default}. Its config supports only internal placeholders such as ${base-path} and ${config-path} (and other config-specific variables), not arbitrary ${VAR:-default} shell semantics. [1][2]
Workarounds:
- Preprocess your .golangci.yml with a tool that supports POSIX expansions (envsubst with shell-assigned defaults, or a full replacer like vex/go envsubst libraries) and pass the resulting file to golangci-lint via --config. [2][3]
Sources:
- golangci-lint docs: placeholders ${base-path}/${config-path} in config; config file reference. [1]
- POSIX/envsubst behavior and alternatives (envsubst lacks full ${VAR:-default} semantics; use shell assignments or tools like vex that support ${VAR:-default}). [2][3]
🏁 Script executed:
# Check if there's version info or config hints in the repository
git ls-files | head -20Repository: satococoa/wtp
Length of output: 423
🏁 Script executed:
# Read the .golangci.yml file to see the exact configuration
cat -n .golangci.yml | sed -n '90,110p'Repository: satococoa/wtp
Length of output: 610
🏁 Script executed:
# Check if there's any preprocessing in Taskfile.yml or CI workflows
cat Taskfile.yml | head -50Repository: satococoa/wtp
Length of output: 1252
🏁 Script executed:
# Check CI workflow for any envsubst or preprocessing
cat .github/workflows/ci.yml | grep -A 5 -B 5 "golangci-lint\|goimports\|GOIMPORTS"Repository: satococoa/wtp
Length of output: 294
The configuration uses unsupported syntax that will not expand environment variables
golangci-lint v2.6.2 (used in your CI) does not support shell-style ${ENV:-default} expansion in configuration files. The literal string ${GOIMPORTS_LOCAL_PREFIXES:-github.com/satococoa/wtp/v2} will be passed to goimports as-is, preventing your intended CI override mechanism.
Required fix: Preprocess the .golangci.yml file with envsubst or a Go environment variable expansion tool before passing it to golangci-lint. Update your Taskfile.yml and CI workflow to expand variables before linting. For example:
envsubst < .golangci.yml | golangci-lint run --config=/dev/stdinAlternatively, use a Go-based tool like github.com/a8m/envsubst to handle the ${VAR:-default} semantics correctly.
🤖 Prompt for AI Agents
.golangci.yml around lines 97-103: the YAML contains shell-style
`${GOIMPORTS_LOCAL_PREFIXES:-github.com/satococoa/wtp/v2}` which golangci-lint
v2.6.2 will not expand; remove the shell expansion from the static config and
instead update Taskfile.yml and CI to preprocess the config (for example run
envsubst or a Go env-subst tool to expand `${VAR:-default}`) and then pass the
expanded config to golangci-lint (e.g. via stdin or a temp file); alternatively
replace the dynamic syntax with a concrete value in the repo and implement the
env substitution step in CI/Taskfile so CI overrides work as intended.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/hooks/executor.go (2)
254-256: Permission bit check may not reflect actual access on all platforms.The check
srcInfo.Mode().Perm()&0o400 == 0only examines the owner read bit. On Unix, access also depends on group/other permissions when the effective user isn't the owner. On Windows, mode bits are largely ignored in favor of ACLs. The subsequentos.Openwill fail with an accurate system error if truly unreadable.Consider whether the pre-check adds value over letting the actual I/O operation surface the error, or use a broader mask like
0o444(any read bit).- if srcInfo.Mode().Perm()&0o400 == 0 { + if srcInfo.Mode().Perm()&0o444 == 0 { return fmt.Errorf("failed to copy file: source file is not readable") }
299-301: Directory readability check could be more comprehensive.For directories on Unix, both read (to list entries) and execute (to traverse) permissions are typically needed. The current check only examines the owner read bit. Consider checking for
0o555(read+execute for any user) or relying onos.ReadDirto surface accurate system errors.cmd/wtp/init.go (1)
132-134: Consider using full path in error message for better debugging.Line 133 uses
filepath.Base(path), which strips directory context. Including the full path would help users quickly identify which directory caused the issue.Apply this diff:
if !info.IsDir() { - return fmt.Errorf("%s is not a directory", filepath.Base(path)) + return fmt.Errorf("%s is not a directory", path) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/wtp/init.go(5 hunks)internal/hooks/executor.go(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/hooks/executor.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: E2E Tests (macos-latest)
🔇 Additional comments (6)
internal/hooks/executor.go (5)
45-56: LGTM!Consistent error propagation for write operations aligns with the PR objective to surface I/O errors across the CLI.
81-98: Path containment validation is well-structured.The design correctly validates relative paths against their respective base directories (repo root for source, worktree for destination) while allowing absolute paths to bypass validation. This is appropriate since absolute paths represent explicit developer intent, and the config is developer-controlled.
125-136: LGTM!The path containment check correctly handles both direct parent traversal (
..) and deeper escapes (../...), usingfilepath.Relwhich properly handles platform-specific path separators.
335-350: LGTM as a defensive layer.The writability check using
0o222(any write bit) is reasonable. SinceexecuteCopyHookWithWritercreates the destination directory viaos.MkdirAllbefore callingcopyFile/copyDir, this serves as defense-in-depth against external changes or edge cases.
178-183: LGTM!Error propagation for log writes is consistent with the pattern used elsewhere in this file.
cmd/wtp/init.go (1)
119-124: LGTM!Properly captures and propagates print errors, which aligns with the PR objective to surface formatter output errors.
| repoInfo, repoStatErr := os.Stat(repo.Path()) | ||
| if repoStatErr != nil { | ||
| return errors.DirectoryAccessFailed("access repository", repo.Path(), repoStatErr) | ||
| } | ||
|
|
||
| if repoInfo.Mode().Perm()&0o222 == 0 { | ||
| return errors.DirectoryAccessFailed( | ||
| "create configuration file", | ||
| configPath, | ||
| fmt.Errorf("repository directory is read-only"), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove duplicate repository accessibility checks.
Lines 52–63 verify that repo.Path() is accessible and writable, but Lines 104–107 invoke ensureWritableDirectory on the same path, performing identical checks. This duplication adds unnecessary complexity.
Apply this diff to remove the redundant block:
- repoInfo, repoStatErr := os.Stat(repo.Path())
- if repoStatErr != nil {
- return errors.DirectoryAccessFailed("access repository", repo.Path(), repoStatErr)
- }
-
- if repoInfo.Mode().Perm()&0o222 == 0 {
- return errors.DirectoryAccessFailed(
- "create configuration file",
- configPath,
- fmt.Errorf("repository directory is read-only"),
- )
- }
-
// Create configuration with comments📝 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.
| repoInfo, repoStatErr := os.Stat(repo.Path()) | |
| if repoStatErr != nil { | |
| return errors.DirectoryAccessFailed("access repository", repo.Path(), repoStatErr) | |
| } | |
| if repoInfo.Mode().Perm()&0o222 == 0 { | |
| return errors.DirectoryAccessFailed( | |
| "create configuration file", | |
| configPath, | |
| fmt.Errorf("repository directory is read-only"), | |
| ) | |
| } | |
| // Create configuration with comments |
🤖 Prompt for AI Agents
In cmd/wtp/init.go around lines 52 to 63, remove the redundant manual repository
accessibility/writability check (the os.Stat + Mode().Perm()&0o222 block)
because ensureWritableDirectory is already called later (lines ~104–107) to
perform the same validation; delete that duplicate block so the function relies
on ensureWritableDirectory for repository write checks and avoid double-checking
the same path.
| if repoInfo.Mode().Perm()&0o222 == 0 { | ||
| return errors.DirectoryAccessFailed( | ||
| "create configuration file", | ||
| configPath, | ||
| fmt.Errorf("repository directory is read-only"), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Inconsistent permission check—use owner write bit (0o200) instead of 0o222.
Line 57 checks 0o222 (any write permission), while Line 136 in ensureWritableDirectory checks 0o200 (owner write permission). The 0o222 check passes if group or other has write permission, even when the owner doesn't, which can lead to false positives since the current user (likely the owner) may still lack write access.
Apply this diff to align the permission checks:
- if repoInfo.Mode().Perm()&0o222 == 0 {
+ if repoInfo.Mode().Perm()&0o200 == 0 {
return errors.DirectoryAccessFailed(
"create configuration file",
configPath,
fmt.Errorf("repository directory is read-only"),
)
}📝 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.
| if repoInfo.Mode().Perm()&0o222 == 0 { | |
| return errors.DirectoryAccessFailed( | |
| "create configuration file", | |
| configPath, | |
| fmt.Errorf("repository directory is read-only"), | |
| ) | |
| } | |
| if repoInfo.Mode().Perm()&0o200 == 0 { | |
| return errors.DirectoryAccessFailed( | |
| "create configuration file", | |
| configPath, | |
| fmt.Errorf("repository directory is read-only"), | |
| ) | |
| } |
🤖 Prompt for AI Agents
In cmd/wtp/init.go around lines 57 to 63, the permission check uses 0o222 (any
write bit) but should use the owner write bit 0o200 to match
ensureWritableDirectory and avoid false positives; change the bitmask from 0o222
to 0o200 so the code verifies owner write permission and keep the existing error
return unchanged.
Summary
Testing
Issues
Summary by CodeRabbit
New Features
Documentation
Refactor
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.