Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughConsolidates add command usage text, replaces MultipleBranchesError with AmbiguousReferenceError, updates ambiguity detection and messaging, changes README guidance to require creating local tracking branches before retrying Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 removes user-facing references to hidden/unsupported wtp add flags (notably --track/--force messaging), aligns CLI usage/error text and README examples with the public CLI surface, and refactors duplicated add/remove CLI test helpers into a shared helper.
Changes:
- Update error messages and tests to remove hidden-flag guidance and provide updated solutions/usage text.
- Replace add command inline
UsageTextwith a sharedaddUsageTextconstant and assert it in tests. - Introduce
createTestSubcommandtest helper and refactor add/remove tests to use it.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/errors/errors_test.go | Updates assertions to match revised error text (usage/examples/solutions). |
| internal/errors/errors.go | Removes hidden-flag references in errors; updates multi-remote guidance and config/worktree creation messaging. |
| cmd/wtp/testhelpers_test.go | Adds shared createTestSubcommand helper for CLI parsing in tests. |
| cmd/wtp/remove_test.go | Refactors remove CLI helper to use shared subcommand helper. |
| cmd/wtp/add_test.go | Aligns add tests with public flags/usage text; removes old multi-remote wrapper expectations. |
| cmd/wtp/add.go | Introduces addUsageText constant; removes stale multi-remote wrapper logic and --force messaging from add errors. |
| README.md | Updates documentation examples to reflect manual remote disambiguation (no hidden flags). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/errors/errors.go
Outdated
| // MultipleBranchesFound reports that a branch name matches multiple remotes and needs manual disambiguation. | ||
| func MultipleBranchesFound(branchName string, remotes []string) error { | ||
| msg := fmt.Sprintf("branch '%s' exists in multiple remotes: %s", branchName, strings.Join(remotes, ", ")) | ||
| msg += fmt.Sprintf(` | ||
|
|
||
| Solution: Specify the remote explicitly: | ||
| • wtp add --track %s/%s %s`, remotes[0], branchName, branchName) | ||
|
|
||
| if len(remotes) > 1 { | ||
| msg += fmt.Sprintf("\n • wtp add --track %s/%s %s", remotes[1], branchName, branchName) | ||
| } | ||
| Solution: Create a local branch that tracks the remote you want, then run wtp add again: | ||
| • git switch --track %s/%s | ||
| • wtp add %s`, remotes[0], branchName, branchName) |
There was a problem hiding this comment.
MultipleBranchesFound builds the suggestion using remotes[0]. In practice remotes is constructed from a map iteration in internal/git.Repository.ResolveBranch, so the “example” remote will be nondeterministic and may not match the remote the user actually wants. Consider sorting remotes and either (a) emitting one git switch --track <remote>/<branch> suggestion per remote, or (b) wording the message explicitly as an example and not implying it’s the chosen remote. Also consider guarding against an empty remotes slice to avoid a panic if this helper is ever called with no remotes.
There was a problem hiding this comment.
Addressed on the branch. MultipleBranchesFound now sorts the remote list, guards the empty-slice case, and emits one git branch --track <local> <remote>/<branch> example per remote so the guidance is deterministic and does not imply a chosen remote.
cmd/wtp/testhelpers_test.go
Outdated
| } | ||
| cmdArgs = append(cmdArgs, args...) | ||
|
|
||
| _ = app.Run(context.Background(), cmdArgs) |
There was a problem hiding this comment.
createTestSubcommand ignores the error returned by app.Run. If flag parsing fails (e.g., a caller passes an unknown flag key), the helper will still return a command and tests may pass while exercising an uninitialized CLI context. Consider returning the error (or taking t *testing.T and asserting require.NoError) so test failures are surfaced reliably.
| _ = app.Run(context.Background(), cmdArgs) | |
| if err := app.Run(context.Background(), cmdArgs); err != nil { | |
| panic(err) | |
| } |
cmd/wtp/remove_test.go
Outdated
| return app.Commands[0] | ||
| return createTestSubcommand("remove", []cli.Flag{ | ||
| &cli.BoolFlag{Name: "force"}, | ||
| &cli.BoolFlag{Name: "branch"}, |
There was a problem hiding this comment.
createRemoveTestCLICommand defines a branch flag, but the actual remove CLI flag is named with-branch (see NewRemoveCommand in cmd/wtp/remove.go). Using a non-existent flag name makes the test helper diverge from the real CLI surface; consider renaming this flag to with-branch and updating any test inputs that set flags["branch"] accordingly.
| &cli.BoolFlag{Name: "branch"}, | |
| &cli.BoolFlag{Name: "with-branch"}, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 777f102ca1
ℹ️ 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".
internal/errors/errors.go
Outdated
| • git switch --track %s/%s | ||
| • wtp add %s`, remotes[0], branchName, branchName) |
There was a problem hiding this comment.
Replace
git switch --track disambiguation guidance
The new multi-remote error guidance tells users to run git switch --track <remote>/<branch> and then wtp add <branch>, but git switch switches the current worktree to that branch (see git switch -h: "create and switch to a new branch" / "set branch tracking configuration"), so the subsequent wtp add will fail because that branch is already checked out in another worktree. In the exact ambiguous-remote scenario this message targets, following these steps leaves users stuck on an "already used by worktree" error instead of resolving the ambiguity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/wtp/add_test.go (1)
1029-1035:⚠️ Potential issue | 🟡 MinorThis assertion now masks a bad fallback.
Because
analyzeGitWorktreeError()currently treats any unmatchedfatal:output as an invalid-path case, this test can pass even when branch ambiguity is reported with the wrong guidance. Please keep a stronger expectation here for the user-facing ambiguity message so thewtp add -b <new> <ambiguous>path stays covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/wtp/add_test.go` around lines 1029 - 1035, The test case "multiple branches error" is too lax and masks a bad fallback in analyzeGitWorktreeError; update this case so expectedError matches the specific user-facing ambiguity guidance produced for branch-ambiguity (not just "matched multiple branches") and ensure expectedType reflects the appropriate non-nil error type returned by analyzeGitWorktreeError; locate the "multiple branches error" case in cmd/wtp/add_test.go and tighten expectedError to the exact ambiguity message string emitted by analyzeGitWorktreeError and set expectedType to the error type used for ambiguous-branch errors.
🤖 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/remove_test.go`:
- Around line 795-800: The test helper createRemoveTestCLICommand is using the
test-only flag name "branch" instead of the public flag name "--with-branch",
which masks regressions in NewRemoveCommand; update createRemoveTestCLICommand
to register the BoolFlag with Name "with-branch" (replacing "branch") and update
all occurrences of the flags map keys in this test file from "branch" to
"with-branch" so tests exercise the actual public flag wiring exposed by
NewRemoveCommand.
In `@cmd/wtp/testhelpers_test.go`:
- Around line 72-106: The helper createTestSubcommand currently swallows errors
from app.Run; change its signature to accept t *testing.T and check the return
value of app.Run(context.Background(), cmdArgs), calling t.Fatalf("app.Run
failed: %v", err) on non-nil error so tests fail fast; then update the wrapper
functions createTestCLICommand and createRemoveTestCLICommand to accept t
*testing.T and forward it into createTestSubcommand, and update their call sites
in tests accordingly.
In `@internal/errors/errors.go`:
- Around line 304-311: The message returned by MultipleBranchesFound instructs
users to run "git switch --track", which checks out the branch and causes a
subsequent "wtp add" to fail; update the Solution text inside
MultipleBranchesFound to use "git branch --track <remote>/<branch>" (i.e.,
replace "git switch --track %s/%s" with "git branch --track %s/%s") so the
tracking branch is created without checking it out, leaving the repository state
suitable for rerunning wtp add; ensure the formatted placeholders still use
remotes[0] and branchName and keep the rest of the message unchanged.
In `@README.md`:
- Around line 167-174: Update the README example for disambiguating multi-remote
branches to use a non-checkout command: replace the suggested "git switch
--track upstream/feature/shared" with "git branch --track feature/shared
upstream/feature/shared" so the local tracking branch is created without
checking it out before running wtp add; also update the Error Handling section
wording (the message referenced around the current "Please specify remote
explicitly") to match the tool's actual error text about branches existing in
multiple remotes (e.g., "Error: branch 'X' exists in multiple remotes: ...") so
the docs reflect the real output.
---
Outside diff comments:
In `@cmd/wtp/add_test.go`:
- Around line 1029-1035: The test case "multiple branches error" is too lax and
masks a bad fallback in analyzeGitWorktreeError; update this case so
expectedError matches the specific user-facing ambiguity guidance produced for
branch-ambiguity (not just "matched multiple branches") and ensure expectedType
reflects the appropriate non-nil error type returned by analyzeGitWorktreeError;
locate the "multiple branches error" case in cmd/wtp/add_test.go and tighten
expectedError to the exact ambiguity message string emitted by
analyzeGitWorktreeError and set expectedType to the error type used for
ambiguous-branch errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c53fe4c-0b55-40ab-834f-155d65af1560
📒 Files selected for processing (7)
README.mdcmd/wtp/add.gocmd/wtp/add_test.gocmd/wtp/remove_test.gocmd/wtp/testhelpers_test.gointernal/errors/errors.gointernal/errors/errors_test.go
Summary
Testing
Summary by CodeRabbit
Bug Fixes
Documentation