fix: address code review issues in bd worktree command

- Add CheckReadonly calls to create/remove commands
- Add cleanup on partial failure (remove worktree if beads setup fails)
- Prevent removing main repository as worktree
- Fix gitignore entry to use relative path from repo root
- Remove misleading stash check (stashes are repo-wide, not per-worktree)
- Populate Name field in WorktreeInfo struct
- Add #nosec comments for exec.Command and file reads

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Steve Yegge
2025-12-28 01:07:38 -08:00
parent f631299a87
commit b5ab4f2a3b

View File

@@ -138,6 +138,8 @@ func init() {
} }
func runWorktreeCreate(cmd *cobra.Command, args []string) error { func runWorktreeCreate(cmd *cobra.Command, args []string) error {
CheckReadonly("worktree create")
name := args[0] name := args[0]
// Determine worktree path // Determine worktree path
@@ -170,11 +172,13 @@ func runWorktreeCreate(cmd *cobra.Command, args []string) error {
} }
// Create the worktree // Create the worktree
// #nosec G204 - branch and worktreePath are user input but git validates them
gitCmd := exec.Command("git", "worktree", "add", "-b", branch, worktreePath) gitCmd := exec.Command("git", "worktree", "add", "-b", branch, worktreePath)
gitCmd.Dir = repoRoot gitCmd.Dir = repoRoot
output, err := gitCmd.CombinedOutput() output, err := gitCmd.CombinedOutput()
if err != nil { if err != nil {
// Try without -b if branch already exists // Try without -b if branch already exists
// #nosec G204 - branch and worktreePath are user input but git validates them
gitCmd = exec.Command("git", "worktree", "add", worktreePath, branch) gitCmd = exec.Command("git", "worktree", "add", worktreePath, branch)
gitCmd.Dir = repoRoot gitCmd.Dir = repoRoot
output, err = gitCmd.CombinedOutput() output, err = gitCmd.CombinedOutput()
@@ -183,9 +187,16 @@ func runWorktreeCreate(cmd *cobra.Command, args []string) error {
} }
} }
// Helper to clean up worktree on failure
cleanupWorktree := func() {
// #nosec G204 - worktreePath was created by us above
_ = exec.Command("git", "worktree", "remove", "--force", worktreePath).Run()
}
// Create .beads directory in worktree // Create .beads directory in worktree
worktreeBeadsDir := filepath.Join(worktreePath, ".beads") worktreeBeadsDir := filepath.Join(worktreePath, ".beads")
if err := os.MkdirAll(worktreeBeadsDir, 0755); err != nil { if err := os.MkdirAll(worktreeBeadsDir, 0755); err != nil {
cleanupWorktree()
return fmt.Errorf("failed to create .beads directory: %w", err) return fmt.Errorf("failed to create .beads directory: %w", err)
} }
@@ -196,13 +207,20 @@ func runWorktreeCreate(cmd *cobra.Command, args []string) error {
// Fall back to absolute path // Fall back to absolute path
relPath = mainBeadsDir relPath = mainBeadsDir
} }
// #nosec G306 - redirect file needs to be readable
if err := os.WriteFile(redirectPath, []byte(relPath+"\n"), 0644); err != nil { if err := os.WriteFile(redirectPath, []byte(relPath+"\n"), 0644); err != nil {
cleanupWorktree()
return fmt.Errorf("failed to create redirect file: %w", err) return fmt.Errorf("failed to create redirect file: %w", err)
} }
// Add to .gitignore if worktree is inside repo root // Add to .gitignore if worktree is inside repo root
if strings.HasPrefix(worktreePath, repoRoot+string(os.PathSeparator)) { if strings.HasPrefix(worktreePath, repoRoot+string(os.PathSeparator)) {
if err := addToGitignore(repoRoot, name); err != nil { // Use relative path from repo root for gitignore entry
relWorktreePath, err := filepath.Rel(repoRoot, worktreePath)
if err != nil {
relWorktreePath = filepath.Base(worktreePath)
}
if err := addToGitignore(repoRoot, relWorktreePath); err != nil {
// Non-fatal, just warn // Non-fatal, just warn
fmt.Fprintf(os.Stderr, "Warning: failed to update .gitignore: %v\n", err) fmt.Fprintf(os.Stderr, "Warning: failed to update .gitignore: %v\n", err)
} }
@@ -285,6 +303,8 @@ func runWorktreeList(cmd *cobra.Command, args []string) error {
} }
func runWorktreeRemove(cmd *cobra.Command, args []string) error { func runWorktreeRemove(cmd *cobra.Command, args []string) error {
CheckReadonly("worktree remove")
name := args[0] name := args[0]
// Find the worktree // Find the worktree
@@ -299,6 +319,13 @@ func runWorktreeRemove(cmd *cobra.Command, args []string) error {
return err return err
} }
// Don't allow removing the main repository
absWorktree, _ := filepath.Abs(worktreePath)
absMain, _ := filepath.Abs(repoRoot)
if absWorktree == absMain {
return fmt.Errorf("cannot remove main repository as worktree")
}
// Safety checks unless --force // Safety checks unless --force
if !worktreeForce { if !worktreeForce {
if err := checkWorktreeSafety(worktreePath); err != nil { if err := checkWorktreeSafety(worktreePath); err != nil {
@@ -307,8 +334,10 @@ func runWorktreeRemove(cmd *cobra.Command, args []string) error {
} }
// Remove worktree // Remove worktree
// #nosec G204 - worktreePath is validated above
gitCmd := exec.Command("git", "worktree", "remove", worktreePath) gitCmd := exec.Command("git", "worktree", "remove", worktreePath)
if worktreeForce { if worktreeForce {
// #nosec G204 - worktreePath is validated above
gitCmd = exec.Command("git", "worktree", "remove", "--force", worktreePath) gitCmd = exec.Command("git", "worktree", "remove", "--force", worktreePath)
} }
gitCmd.Dir = repoRoot gitCmd.Dir = repoRoot
@@ -317,8 +346,12 @@ func runWorktreeRemove(cmd *cobra.Command, args []string) error {
return fmt.Errorf("failed to remove worktree: %w\n%s", err, string(output)) return fmt.Errorf("failed to remove worktree: %w\n%s", err, string(output))
} }
// Remove from .gitignore // Remove from .gitignore - use relative path from repo root
if err := removeFromGitignore(repoRoot, filepath.Base(worktreePath)); err != nil { relWorktreePath, err := filepath.Rel(repoRoot, worktreePath)
if err != nil {
relWorktreePath = filepath.Base(worktreePath)
}
if err := removeFromGitignore(repoRoot, relWorktreePath); err != nil {
// Non-fatal, just warn // Non-fatal, just warn
fmt.Fprintf(os.Stderr, "Warning: failed to update .gitignore: %v\n", err) fmt.Fprintf(os.Stderr, "Warning: failed to update .gitignore: %v\n", err)
} }
@@ -408,8 +441,10 @@ func parseWorktreeList(output string) []WorktreeInfo {
if current.Path != "" { if current.Path != "" {
worktrees = append(worktrees, current) worktrees = append(worktrees, current)
} }
path := strings.TrimPrefix(line, "worktree ")
current = WorktreeInfo{ current = WorktreeInfo{
Path: strings.TrimPrefix(line, "worktree "), Path: path,
Name: filepath.Base(path),
} }
} else if strings.HasPrefix(line, "HEAD ") { } else if strings.HasPrefix(line, "HEAD ") {
// Skip HEAD hash // Skip HEAD hash
@@ -453,6 +488,7 @@ func getBeadsState(worktreePath, mainBeadsDir string) string {
func getRedirectTarget(worktreePath string) string { func getRedirectTarget(worktreePath string) string {
redirectFile := filepath.Join(worktreePath, ".beads", beads.RedirectFileName) redirectFile := filepath.Join(worktreePath, ".beads", beads.RedirectFileName)
// #nosec G304 - path is constructed from worktreePath which comes from git worktree list
data, err := os.ReadFile(redirectFile) data, err := os.ReadFile(redirectFile)
if err != nil { if err != nil {
return "" return ""
@@ -510,13 +546,9 @@ func checkWorktreeSafety(worktreePath string) error {
return fmt.Errorf("worktree has unpushed commits") return fmt.Errorf("worktree has unpushed commits")
} }
// Check for stashes // Note: We intentionally don't check stashes here because git stashes
gitCmd = exec.Command("git", "stash", "list") // are stored globally in the main repo, not per-worktree. Checking
gitCmd.Dir = worktreePath // stashes would give misleading results.
output, _ = gitCmd.CombinedOutput()
if len(strings.TrimSpace(string(output))) > 0 {
return fmt.Errorf("worktree has stashed changes")
}
return nil return nil
} }