Remove leftover session dirs

[canercidam]

No description provided.

[claude]

Claude finished @canercidam's task in 1m 58s —— View job

---

Review

Reviewed the one-commit change to main.go that makes clean remove session dirs after stopping containers.

Issues flagged inline:

1. Silent error suppression (main.go:218-224) — _ = os.RemoveAll(...) hides exactly the failure mode this PR is meant to address. If removal fails (perms, open file, etc.), the user gets no signal and the leftover persists. Surface with at least slog.Warn.

2. Path-traversal via user-supplied session arg (main.go:222) — session comes straight from CLI args. filepath.Join(sessionsDir, "..") resolves to the parent of sessionsDir, so playground clean .. would RemoveAll the whole playground state dir. Validate that the joined path stays under sessionsDir, or reject names with path separators / ...

3. Early-return skips cleanup on partial failure (main.go:213) — If StopSession errors on any one session in the loop, the function returns before the new dir-removal block runs. For clean all, one failing docker-compose down means none of the remaining session dirs get cleaned — the exact leak this PR addresses. Consider best-effort: collect errors from the stop loop but still attempt dir cleanup.

Other notes (non-blocking):

• The temp bind-mount dir under $TMPDIR/builder-playground/<sessionID> (see utils/temp.go) is not touched by this change. clean all users may reasonably expect those to be cleaned too; today they're only "auto-removed at reboot" per the warning in main.go:739.
• GetLocalSessions() only returns sessions that still have docker containers, so clean all won't enumerate orphaned session dirs. That's fine here because the isAll branch removes the whole sessionsDir wholesale — but it does mean the latest symlink disappears as a side effect, which is probably the intent.
  · Branch: caner/clean-all-remove-dirs

[claude]

Claude finished @canercidam's task in 1m 37s —— View job

---

Review

Re-reviewed after fixup commit c3cb3a1 ("simplify logic and handle errors").

Status of issues from the prior round:

1. Silent error suppression — fixed in c3cb3a1. os.RemoveAll failures are now surfaced via slog.Warn for both the all and per-session paths.
2. Path-traversal via user-supplied session arg — still present (main.go:228). session is unvalidated; playground clean .. resolves filepath.Join(sessionsDir, "..") to the parent of sessionsDir and os.RemoveAll wipes the entire playground state dir. clean ../.. can reach further, up to ~/.local/state. See inline comment for a fix sketch using filepath.Rel.
3. Early-return skips cleanup on partial StopSession failure — still present (main.go:207-212 → early-returns before the new cleanup block at main.go:213). For clean all, one failing docker-compose down means every remaining session dir leaks — the regression this PR is meant to eliminate. See inline comment for a best-effort sketch using errors.Join.

Other notes (non-blocking, unchanged from prior review):

• Temp bind-mount dirs under $TMPDIR/builder-playground/<sessionID> (see utils/temp.go) are still not touched by clean. Users running clean all may reasonably expect those too.
• GetLocalSessions() only enumerates sessions that still have docker containers, so orphaned session dirs won't be cleaned by name — but the isAll branch RemoveAlls the whole sessionsDir wholesale, so this is only an issue for the per-session path.
  · Branch: caner/clean-all-remove-dirs

[claude]

Claude finished @canercidam's task in 1m 59s —— View job

---

Re-review (after commit e466b63 "avoid accidental dir removal")

Status of issues from prior review rounds:

1. Silent error suppression — fixed in c3cb3a1 (first re-review round). os.RemoveAll failures are surfaced via slog.Warn on both paths.

2. Path-traversal via user-supplied session arg — fixed in e466b63. The new sessionNameRegex (^[a-z0-9]+(-[a-z0-9]+)*$) at main.go:184 is validated before cleanup and disallows /, \, ., and .., matching the format produced by utils.GeneratePetName(). Refusing to combine all with named sessions is also a nice tightening.

3. Early-return on partial StopSession failure — still present (main.go:221-226 → return err short-circuits before the new cleanup block at main.go:227). For clean all, one failing docker-compose down still leaks every remaining session dir — the regression this PR is meant to eliminate. See the inline comment on main.go:227 for a sketch using errors.Join on the clean path.

Other notes (non-blocking, unchanged from prior rounds):

• Temp bind-mount dirs under $TMPDIR/builder-playground/<sessionID> (see utils/temp.go) are still not touched by clean. Users running clean all may reasonably expect those too.
• GetLocalSessions() only enumerates sessions with live docker containers, so orphaned session dirs won't be cleaned by name — but the isAll branch wipes sessionsDir wholesale, so this only matters on the per-session path.

---

• Fetch latest diff
• Check status of prior-round issues
• Look for new issues
• Post review

· Branch: caner/clean-all-remove-dirs ·