[fix] Protect worktree operands with separator#256
Conversation
Pass AddWorktree and MoveWorktree positional operands after -- so dash-leading sources or paths stay data instead of being parsed as git options. Keep MoveWorktree force flags before the separator, and cover the exact argv shape with focused mock runner tests.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR strengthens Git worktree command invocation by inserting -- to terminate option parsing, preventing paths/refs that begin with - from being misinterpreted as flags.
Changes:
- Inserted
--intogit worktree addargument list before positional args. - Refactored
git worktree moveargument construction to place--after options. - Added/updated tests to assert the exact argument sequences passed to the runner.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/git/worktree.go | Inserts -- into worktree add/move commands to safely separate options from positional arguments. |
| internal/git/mock_runner_test.go | Adds tests asserting -- insertion and updates force-move test to compare full expected args. |
loopassembly
changed the title
| if force { | ||
| args = append(args, flagForce, flagForce) | ||
| } | ||
| args = append(args, "--", oldPath, newPath) |
There was a problem hiding this comment.
High — git worktree move may not support -- as end-of-options separator
git worktree add explicitly lists -- in its synopsis; git worktree move does not. The generated invocation is now:
git worktree move --force --force -- /old /new
If move does not recognise -- as end-of-options, git reads -- as <worktree> and /old as <new-path>, silently moving the wrong worktree (or erroring). Verify empirically in a temp repo:
git worktree move -- /some/path /other/pathIf git rejects it, the -- must be omitted for move (or conditioned on the operands actually starting with -). A targeted integration test would catch this definitively.
| } | ||
| } | ||
|
|
||
| func TestMoveWorktreeInsertsDashDash(t *testing.T) { |
There was a problem hiding this comment.
Medium — TestMoveWorktreeNoForce is now fully subsumed by this test
TestMoveWorktreeNoForce (line 335) calls MoveWorktree(r, "/old/path", "/new/path", false) and asserts !slices.Contains(captured, flagForce). This new test makes the same call and asserts slices.Equal(captured, want) — which already guarantees no --force (since want excludes it). The older test adds zero additional coverage.
Suggested: delete TestMoveWorktreeNoForce (or collapse both no-force tests into a table-driven test alongside TestMoveWorktreeForce to keep all three cases in one place).
lugassawan
left a comment
lugassawan
left a comment
There was a problem hiding this comment.
Review Decision: COMMENT
Reviewed by reviewer (swe-workbench). Posted 2 inline comments, deduped 0.
Informational (out-of-diff)
Medium — RemoveWorktree (worktree.go:42) still passes path as a bare positional (not in this diff)
RemoveWorktree was not in the #229 hardening sweep and was not touched in this PR either. A path beginning with - is passed directly to git and parsed as a flag. Suggested: restructure to ["worktree", "remove", "--force"?, "--", path], mirroring the pattern established by this PR — and empirically verify that git worktree remove honours -- (same check recommended for move).
Closes #252.
What changed
AddWorktreepath/source operands after--, matching the existingAddWorktreeFromBranchhardening.MoveWorktreeargs with force flags first, then--, then the old/new path operands.Why
This keeps dash-leading sources or paths from being parsed as git options, while preserving the existing double-
--forcebehavior for locked worktree moves.Validation
go test ./internal/gitgo test ./...