Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as "wtp add"
participant Exec as "command.Executor"
participant Hooks as "PostCreateHooks"
participant FS as "Filesystem"
User->>CLI: run `wtp add -b <branch> [--quiet]`
CLI->>CLI: resolveAddWriters(stdout, status) based on --quiet
CLI->>Exec: create worktree (git worktree add / custom hook)
Exec->>FS: create worktree on disk
FS-->>Exec: created absolute path
Exec-->>CLI: return created path / status
CLI->>Hooks: run PostCreateHooks (write to statusWriter)
Hooks-->>CLI: hook results (warnings/errors to statusWriter)
CLI-->>User: stdoutWriter: absolute path (if --quiet) or full success message
CLI-->>User: statusWriter/stderr: human messages, warnings, hook output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3600d4d30e
ℹ️ 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 "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/e2e/worktree_creation_test.go (1)
95-95: Use the raw output path forAssertWorktreeExiststo reduce symlink flakiness.Line 95 currently passes
resolvedActual; the helper performs string containment on listed worktree paths, so canonical-vs-noncanonical path differences can cause false negatives.Proposed fix
- framework.AssertWorktreeExists(t, repo, resolvedActual) + framework.AssertWorktreeExists(t, repo, filepath.Clean(actualPath))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/worktree_creation_test.go` at line 95, The test currently passes the canonicalized path variable resolvedActual into AssertWorktreeExists which does string containment checks and can fail on symlink vs canonical path differences; change the call to pass the raw output path variable (the original path produced by the command — i.e., the non-canonical path used before resolving to resolvedActual) into AssertWorktreeExists so the helper compares against the same form as the worktree listing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/wtp/add.go`:
- Line 31: The UsageText string assigned to UsageText in cmd/wtp/add.go is too
long for the linter; break the single long literal into shorter concatenated
string literals (or use a raw backtick multi-line string) so no line exceeds the
max length. Locate the UsageText assignment (the UsageText field) and split the
content like "wtp add <existing-branch>\n" + " wtp add -b <new-branch>
[<commit>]\n" + " wtp add -b <new-branch> --quiet" or replace with a
backtick-wrapped multi-line string to keep the same value while satisfying the
linter.
- Around line 155-170: The gocritic "unnamedResult" is triggered by treating
Root().Writer/ErrWriter as values instead of calling them; in resolveAddWriters
fix this by invoking the writer providers: call cmd.Root().Writer() to obtain
stdoutWriter (fall back to os.Stdout if nil) and call cmd.Root().ErrWriter()
when setting statusWriter for the quiet case (fall back to os.Stderr if nil);
keep using cmd.Bool("quiet") and the local variables stdoutWriter and
statusWriter as before.
- Around line 137-138: The post-create exec handling leaks command output in
quiet mode because executePostCreateCommand is creating
command.Command{Interactive: true}, which causes internal/command/shell.go to
wire stdout/stderr to the terminal and bypass the provided statusWriter; update
executePostCreateCommand (the callsite using cmdExec and cmd.String("exec") with
workTreePath) to not set Interactive when a statusWriter (quiet mode) is
provided — either remove/disable Interactive or ensure command.Command uses the
passed writer for stdout/stderr so output is routed via statusWriter instead of
directly to os.Stdout/os.Stderr.
---
Nitpick comments:
In `@test/e2e/worktree_creation_test.go`:
- Line 95: The test currently passes the canonicalized path variable
resolvedActual into AssertWorktreeExists which does string containment checks
and can fail on symlink vs canonical path differences; change the call to pass
the raw output path variable (the original path produced by the command — i.e.,
the non-canonical path used before resolving to resolvedActual) into
AssertWorktreeExists so the helper compares against the same form as the
worktree listing.
There was a problem hiding this comment.
Pull request overview
This PR adds a --quiet flag to the wtp add command to enable script-friendly output, addressing issue #91. The feature is motivated by integration needs with external tools like Claude Code that require machine-readable output (only the created worktree path) on stdout, while keeping status messages, warnings, and progress information on stderr.
Changes:
- Added
--quietflag towtp addcommand with-qalias - Implemented writer routing to separate stdout (path) from stderr (status/warnings) in quiet mode
- Added comprehensive unit test coverage for quiet mode scenarios (success, hook failures, exec output, worktree creation failures)
- Added e2e test for quiet mode integration
- Updated README with quiet flag example
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/wtp/add.go | Added --quiet flag definition, writer resolution logic (resolveAddWriters), and conditional output routing for quiet vs normal modes |
| cmd/wtp/add_test.go | Added TestAddCommand_QuietModeOutput with 4 test cases covering success, hook failures, exec output, and creation failures; updated flag list test |
| test/e2e/worktree_creation_test.go | Added e2e test TestUserCreatesWorktree_WithQuietFlag_ShouldOutputPathOnly to verify end-to-end quiet mode behavior |
| README.md | Added example demonstrating quiet flag usage in Quick Start section |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -66,6 +68,35 @@ func TestUserCreatesWorktree_WithNewBranchFlag_ShouldCreateBranchAndWorktree(t * | |||
| framework.AssertWorktreeExists(t, repo, "feature/payment") | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
E2E tests in this codebase follow a specific documentation pattern with User Story and Business Value comments. This test should include a full header comment similar to other tests in this file, explaining:
- What the test validates
- User Story: describing the user's need
- Business Value: explaining the benefit
For example:
// TestUserCreatesWorktree_WithQuietFlag_ShouldOutputPathOnly tests
// script integration with machine-readable output.
//
// User Story: As a developer integrating wtp with automation tools, I want
// machine-readable output so scripts can reliably parse the worktree path.
//
// Business Value: Enables integration with external tools like Claude Code hooks,
// CI/CD pipelines, and other automation that needs to process the worktree path.
| // TestUserCreatesWorktree_WithQuietFlag_ShouldOutputPathOnly tests | |
| // script integration with machine-readable output from quiet mode. | |
| // | |
| // User Story: As a developer integrating wtp with automation tools, I want | |
| // quiet mode to output only the worktree path so scripts can reliably parse | |
| // and consume it without additional text processing. | |
| // | |
| // Business Value: Enables robust integration with external tools like CI/CD | |
| // pipelines, editor extensions, and other automation that depend on stable, | |
| // machine-readable output from wtp commands. |
| env := framework.NewTestEnvironment(t) | ||
| defer env.Cleanup() | ||
|
|
||
| repo := env.CreateTestRepo("user-creates-quiet-worktree") | ||
|
|
||
| output, err := repo.RunWTP("add", "--branch", "feature/quiet", "--quiet") |
There was a problem hiding this comment.
E2E tests in this codebase use Given-When-Then comments to structure the test narrative. The test should include:
- Given: Initial conditions (User wants machine-readable output, User is in a git repository)
- When: User action (User runs "wtp add --branch feature/quiet --quiet")
- Then: Expected outcome assertions (Output should contain only the absolute path)
This pattern is consistently followed in all other E2E tests in this file and is documented as a requirement.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/wtp/add.go (1)
144-145:⚠️ Potential issue | 🟠 Major
--execcan still violate quiet-mode stdout in interactive execution paths.Routing this call through
statusWriteris good, butexecutePostCreateCommandstill constructscommand.Command{Interactive: true}. If the executor binds interactive output directly to terminal streams, child stdout can bypassstatusWriter, breaking the “path-only stdout” contract in quiet mode.Proposed fix
- if err := executePostCreateCommand(statusWriter, cmdExec, cmd.String("exec"), workTreePath); err != nil { + if err := executePostCreateCommand(statusWriter, cmdExec, cmd.String("exec"), workTreePath, cmd.Bool("quiet")); err != nil { return fmt.Errorf("worktree was created at '%s', but --exec command failed: %w", workTreePath, err) }-func executePostCreateCommand(w io.Writer, cmdExec command.Executor, execCommand, workTreePath string) error { +func executePostCreateCommand( + w io.Writer, + cmdExec command.Executor, + execCommand, workTreePath string, + quiet bool, +) error { @@ commandToRun := command.Command{ WorkDir: workTreePath, - Interactive: true, + Interactive: !quiet, }Verification script (read-only): confirm executor behavior and whether quiet+exec coverage exists.
#!/bin/bash set -euo pipefail echo "== add.go: callsite and interactive setting ==" rg -n -C3 'executePostCreateCommand|Interactive:' cmd/wtp/add.go echo "== executor internals: interactive stdout/stderr wiring ==" fd -t f shell.go internal cmd | xargs -r rg -n -C4 'Interactive|os.Stdout|os.Stderr|Stdout|Stderr|CombinedOutput|Output\(' echo "== tests: quiet + --exec stdout contract coverage ==" rg -n -C3 --type=go 'quiet|--quiet|exec|stdout|stderr|path only|path-only' cmd/wtp/add_test.go test/e2e/worktree_creation_test.goExpected confirmation: if interactive mode writes directly to
os.Stdout/os.Stderr, this concern is valid unless quiet+exec explicitly guards against stdout leakage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/wtp/add.go` around lines 144 - 145, The post-create executor currently builds command.Command with Interactive: true (see executePostCreateCommand usage and its internal construction of command.Command{Interactive: true}), which can cause child stdout/stderr to bypass statusWriter in quiet mode; change the executor invocation so the created command is non-interactive (Interactive: false) or explicitly wire its stdout/stderr to statusWriter (or a writer derived from statusWriter) when calling executePostCreateCommand (the callsite that passes cmdExec and workTreePath), ensuring child process output never writes directly to os.Stdout/os.Stderr in quiet runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/wtp/add.go`:
- Around line 144-145: The post-create executor currently builds command.Command
with Interactive: true (see executePostCreateCommand usage and its internal
construction of command.Command{Interactive: true}), which can cause child
stdout/stderr to bypass statusWriter in quiet mode; change the executor
invocation so the created command is non-interactive (Interactive: false) or
explicitly wire its stdout/stderr to statusWriter (or a writer derived from
statusWriter) when calling executePostCreateCommand (the callsite that passes
cmdExec and workTreePath), ensuring child process output never writes directly
to os.Stdout/os.Stderr in quiet runs.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/wtp/add.gocmd/wtp/add_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/wtp/add_test.go
How to verify (
|
Summary
Closes #91
Summary by CodeRabbit
New Features
Documentation
Tests