internal/config/expand_test.go (1)

222-235: TestListLocalBranches will fail outside a git repository (e.g., tarball-based CI builds).

This test calls git branch and asserts NotEmpty. If the test runner checks out as an archive or shallow clone without branches, or runs outside a git repo, it will fail. Consider skipping with t.Skip when git is unavailable or when not inside a repo.

Proposed guard

 func TestListLocalBranches(t *testing.T) {
+	// Skip if not in a git repo (e.g., tarball-based CI builds)
+	if err := exec.Command("git", "rev-parse", "--git-dir").Run(); err != nil {
+		t.Skip("not inside a git repository")
+	}
 	branches, err := ListLocalBranches()

internal/config/expand.go (2)

208-222: getBranch() spawns a git subprocess on every call — consider caching.

Expand and ExpandRemote both call getBranch() which execs git rev-parse. In workflows where expansion is called multiple times (e.g., multi-host clean), this spawns redundant processes. A sync.Once or package-level lazy cache would eliminate the repeated overhead.

---

318-332: sanitizeBranch allocates a new strings.NewReplacer per call.

This is fine functionally, but since it's called per-branch in ListLocalBranches and can be called frequently, promoting the replacer to a package-level var avoids repeated allocation.

Optional refactor

+var branchSanitizer = strings.NewReplacer(
+	"/", "-",
+	"\\", "-",
+	":", "-",
+	"*", "-",
+	"?", "-",
+	"\"", "-",
+	"<", "-",
+	">", "-",
+	"|", "-",
+)
+
 func sanitizeBranch(branch string) string {
-	replacer := strings.NewReplacer(
-		"/", "-",
-		"\\", "-",
-		":", "-",
-		"*", "-",
-		"?", "-",
-		"\"", "-",
-		"<", "-",
-		">", "-",
-		"|", "-",
-	)
-	return replacer.Replace(branch)
+	return branchSanitizer.Replace(branch)
 }

internal/clean/clean.go (1)

71-90: Remove — error message loses stderr when both err and stderr are present.

Line 80-83: if err != nil, errMsg is overwritten with err.Error(), discarding any stderr content. Consider combining both.

internal/clean/clean_test.go (1)

37-44: Mock dispatch uses non-deterministic map iteration — safe here, but fragile if prefixes overlap.

The for prefix, resp := range m.responses iteration order is random in Go. If you later add overlapping prefixes (e.g., "rm" and "rm -rf"), the wrong handler may match. Current test prefixes are distinct, so this is fine.

internal/cli/clean.go (3)

74-91: Use defer conn.Close() to avoid leaking connections if Discover panics.

Currently conn.Close() is called inline at line 86. If clean.Discover panics (however unlikely), the connection leaks. A defer is the idiomatic pattern.

Suggested change

 		conn, err := connectToHost(hostCfg, probeTimeout)
 		if err != nil {
 			spinner.Fail()
 			fmt.Printf("  %s Could not connect: %v\n", ui.SymbolWarning, err)
 			continue
 		}
+		defer conn.Close()

 		staleDirs, err := clean.Discover(conn.Client, hostCfg.Dir, localBranches)
-		conn.Close()
 		if err != nil {

Note: since this is in a loop, you may want to extract the body into a closure or helper to scope the defer correctly per iteration.

---

161-183: Same defer conn.Close() pattern applies to the removal loop.

Line 176 has the same inline conn.Close() concern. Additionally, if clean.Remove panics, the connection leaks.

---

228-242: connectToHost error could list which aliases were tried.

The generic "all SSH aliases unreachable" message gives no debugging context. Including the aliases (e.g., fmt.Errorf("all SSH aliases unreachable: %v", hostCfg.SSH)) would help users diagnose connection issues.

internal/config/expand.go (2)

314-329: Unwrapped error from ListLocalBranches.

The raw error from exec.Command is returned without context. If this surfaces to the user (e.g., via rr clean), the message will be opaque.

Proposed fix

 func ListLocalBranches() ([]string, error) {
 	cmd := exec.Command("git", "branch", "--format=%(refname:short)")
 	out, err := cmd.Output()
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to list local git branches: %w", err)
 	}

(Or use errors.WrapWithCode from internal/errors if you want the structured error.)

As per coding guidelines, "Ensure errors are wrapped with context using the internal/errors package".

---

210-235: sync.Once caching prevents testability and could surprise long-running processes.

The branch is cached for the process lifetime. This is fine for CLI invocations, but be aware:

1. Tests that call getBranch() (or any function using it) will be stuck with the first result for the entire test suite run. If tests need different branch values, they'll silently get stale data.
2. If rr ever becomes a daemon/watcher, cached branch won't update on checkout.

For now this is acceptable given the CLI model, but consider exposing a resetBranchCache() (test-only, unexported) if you run into flaky tests later.

internal/host/selector.go (1)

347-349: Minor: double map lookup in the loop body.

s.hosts[name] is looked up twice on lines 348-349. Assigning to a local variable (like done in HostInfo at line 528) would be cleaner and consistent within this file.

Suggested fix

 	for name := range s.hosts {
-		if hasTag(s.hosts[name].Tags, tag) {
-			matchingHosts[name] = s.hosts[name]
+		h := s.hosts[name]
+		if hasTag(h.Tags, tag) {
+			matchingHosts[name] = h
 		}

internal/cli/host.go (1)

95-97: Non-deterministic selection of default remote dir.

Iterating range cfg.Hosts with break on first match gives a random host's Dir as the default, since Go map iteration order is undefined. This was true before this change too, so not a regression — just worth noting if consistency matters.

internal/clean/clean.go (1)

98-105: isDangerousPath could be more robust.

Currently only blocks "", "/", "~", and "~/". Paths like /home, /tmp, or /var would pass through. Consider also rejecting paths with fewer than ~3 components or checking against a denylist of common system directories. Not critical since inputs come from config-derived templates, but worth hardening for defense-in-depth.

internal/cli/clean.go (2)

174-191: Nil-error slice inflates totalFailed count on reconnection failure.

When connectToHost fails (line 176), make([]error, len(r.stale)) creates a slice of nil errors. totalFailed += len(errs) on line 185 counts all of them, but the loop at lines 186-190 skips nil errors and prints nothing. The count is correct in intent (those dirs weren't removed), but the mechanism is misleading for anyone reading this code.

Clearer approach

 		removed, errs := func() ([]string, []error) {
 			conn, err := connectToHost(r.hostCfg, probeTimeout)
 			if err != nil {
 				fmt.Printf("%s %s: reconnection failed: %v\n", ui.SymbolFail, r.hostName, err)
-				return nil, make([]error, len(r.stale)) // count all dirs as failed
+				return nil, nil
 			}
 			defer conn.Close()
 			return clean.Remove(conn.Client, r.stale)
 		}()

 		totalRemoved += len(removed)
-		totalFailed += len(errs)
+		if removed == nil && errs == nil {
+			// Connection failed — count all dirs as failed
+			totalFailed += len(r.stale)
+		} else {
+			totalFailed += len(errs)
+		}

---

236-250: connectToHost uses fmt.Errorf instead of the internal errors package.

Per coding guidelines, errors should be wrapped with context using the internal/errors package.

As per coding guidelines, **/*.go: "Ensure errors are wrapped with context using the internal/errors package".

internal/config/expand.go (2)

211-236: Process-lifetime cache is correct for the use case but blocks unit testing.

The sync.Once caching is appropriate since the branch won't change mid-execution. Just noting that tests for functions consuming getBranch() can't override the cached value. If that becomes a problem later, injecting a branch-provider function would help.

---

315-330: Use the internal errors package for wrapping.

Line 319 uses fmt.Errorf instead of the project's internal/errors package. Other error sites in this file don't return errors, but this exported function should follow the project convention. As per coding guidelines, "Ensure errors are wrapped with context using the internal/errors package".

Suggested fix

-	return nil, fmt.Errorf("failed to list local git branches: %w", err)
+	return nil, errors.WrapWithCode(err, errors.ErrExec,
+		"Failed to list local git branches",
+		"Make sure you're in a git repository.")

This would require adding the errors import from internal/errors. Note the fmt import may still be needed if used elsewhere.

internal/clean/clean.go (2)

91-101: isDangerousPath could be strengthened with a depth check.

The current blocklist protects against known dangerous paths, but a path like /boot or a single-segment absolute path (/anything) would pass. Consider adding a heuristic: reject any absolute path with fewer than N segments (e.g., 3), since legitimate per-branch dirs are always deeply nested.

Optional: add depth heuristic

 func isDangerousPath(path string) bool {
 	trimmed := strings.TrimSpace(path)
 	switch trimmed {
 	case "", "/", "~", "~/",
 		"/home", "/tmp", "/var", "/etc", "/usr", "/opt", "/root",
 		"/home/", "/tmp/", "/var/", "/etc/", "/usr/", "/opt/", "/root/":
 		return true
 	}
+	// Reject shallow absolute paths (fewer than 3 segments like /foo/bar)
+	if strings.HasPrefix(trimmed, "/") {
+		segments := strings.Split(strings.Trim(trimmed, "/"), "/")
+		if len(segments) < 3 {
+			return true
+		}
+	}
 	return false
 }

---

108-142: N+1 remote calls for disk usage — acceptable for expected scale.

getDiskUsage is called once per directory, resulting in N+1 remote round-trips. For the expected handful of stale directories this is fine, but if someone accumulates dozens of stale branch dirs, it could get slow. A future optimization could batch into a single du -sh dir1 dir2 ....

internal/cli/clean.go (2)

249-254: pluralize is brittle — only works for "directory/directories".

It returns "y" or "ies", relying on the caller to provide "director" as the prefix. This is fragile and non-obvious. A clearer API would take the full word.

Suggested improvement

-func pluralize(n int) string {
-	if n == 1 {
-		return "y"
-	}
-	return "ies"
+func pluralize(n int, singular, plural string) string {
+	if n == 1 {
+		return singular
+	}
+	return plural
 }

Usage: pluralize(allStale, "directory", "directories")

---

229-247: Consider extracting a shared SSH alias retry utility instead of using Selector.

Both connectToHost and Selector.trySSHAliases implement identical alias iteration logic, but the suggested refactoring to reuse Selector adds unnecessary overhead: Selector includes event emission, mutex locking, and caching that the clean command doesn't need. Instead, extract the core retry pattern into a lightweight utility function that both code paths can share.

internal/config/expand.go (3)

331-346: ListLocalBranches uses fmt.Errorf instead of the project's internal/errors package.

Other error sites in the codebase wrap with errors.WrapWithCode. This function is called from cleanCommand which then re-wraps, so it's not critical, but it's inconsistent.

As per coding guidelines, "Ensure errors are wrapped with context using the internal/errors package".

---

296-315: ExtractBranchFromPath tilde resolution relies on strings.Index which finds the first occurrence.

If the relative prefix (e.g., rr/proj-) appears more than once in the absolute path (unlikely but possible with nested dirs), this could extract the wrong branch segment. The current usage context makes this improbable, but worth a comment.

---

211-236: Process-lifetime cache for branch is correct for CLI usage but complicates testing.

sync.Once means unit tests can't reset the cached branch between test cases. If tests ever need to exercise different branch values, they'd need to use a build-tag-guarded reset function or refactor to accept a branch provider.

Current test coverage seems to test sanitizeBranch and ExpandRemoteGlob independently (not through getBranch), so this is fine for now.

internal/clean/clean.go (1)

174-184: N+1 remote commands: getDiskUsage runs a separate du for each directory.

For hosts with many stale directories, this means N individual SSH round-trips for disk usage. You could batch them into a single du -sh dir1 dir2 ... dirN call and parse the multi-line output. Not blocking, but would improve latency noticeably for hosts with 10+ stale dirs.

Sketch of batched approach

-	for _, line := range lines {
-		line = strings.TrimSpace(line)
-		if line == "" {
-			continue
-		}
-		usage := getDiskUsage(executor, line)
-		dirs = append(dirs, remoteDir{path: line, diskUsage: usage})
-	}
+	var paths []string
+	for _, line := range lines {
+		line = strings.TrimSpace(line)
+		if line != "" {
+			paths = append(paths, line)
+		}
+	}
+	usageMap := getBatchDiskUsage(executor, paths)
+	for _, p := range paths {
+		dirs = append(dirs, remoteDir{path: p, diskUsage: usageMap[p]})
+	}

internal/cli/clean.go (1)

234-252: connectToHost helper is clean and reusable.

Tries each SSH alias in order, returns the first successful connection with latency info. Last error propagated with context. Consider whether this should live in a shared location (it's useful beyond just clean), but fine here for now.