Remove REVIEW.md, standardize close.ts export, and minor cleanup

REVIEW.md

@@ -1,156 +0,0 @@
-# Code Review
-
-## Overall Impression
-
-This is a well-scoped project with a clear mental model. The module boundaries are sensible, the dependency footprint is minimal, and the functional (no classes) approach fits the problem well. The codebase reads quickly, which is valuable.
-
-That said, there are structural patterns that will become friction points as you add commands or change behavior. Most of the feedback here is about **extracting repeated patterns** and **reducing the surface area of cli.ts**.
-
----
-
-## 1. cli.ts is carrying too much weight
-
-At ~820 lines, `cli.ts` is the entry point, the command registry, and the implementation of every command handler. That's three jobs. Each command handler is 20-60 lines of real logic — session lookup, spinner management, error handling, VM orchestration — all inline.
-
-Right now there are 12+ commands. Adding a 13th means appending more logic to a file that's already the longest in the project. There's no natural place to put "command-adjacent" helpers either, so things like `saveChanges`, `branchFromPrompt`, and `fallbackBranchName` just float in the file above the command they serve.
-
-Consider a `commands/` directory where each command (or logical group) is its own file exporting an action function. `cli.ts` becomes just the Commander wiring — argument definitions, descriptions, and `.action(handler)` calls. This makes each command independently readable and testable.
-
----
-
-## 2. Repeated session-lookup boilerplate
-
-This exact pattern appears in **8 commands** (open, review, shell, save, diff, show, log, dir):
-
-```typescript
-const root = await git.repoRoot()
-const session = await state.getSession(root, branch)
-if (!session) {
-  console.error(`✖ No session found for branch "${branch}".`)
-  process.exit(1)
-}
-```
-
-A helper like `requireSession(branch): Promise<{ root: string; session: Session }>` would cut four lines per command and give you a single place to improve the error message later (e.g., suggesting `sandlot list` to see available sessions).
-
----
-
-## 3. Duplicated "pipe through temp file into container" pattern
-
-Both `saveChanges` (line 44-51 of cli.ts) and the merge conflict resolver (line 447-455) follow the same flow:
-
-1. Write content to a temp file under `~/.sandlot/`
-2. `cat` it into `claude -p` inside the container
-3. Capture output
-4. Delete the temp file
-
-This should be a single helper in `vm.ts`, something like:
-
-```typescript
-export async function claudePipe(input: string, prompt: string): Promise<string>
-```
-
-The current approach is fragile — each call site has to remember to clean up the temp file, choose a temp path that won't collide, and handle errors. Centralizing this also makes it easier to switch to stdin piping later.
-
----
-
-## 4. API key parsing is duplicated
-
-`branchFromPrompt()` in cli.ts (lines 87-92) and `create()` in vm.ts (lines 83-88) both independently read `~/.env` and parse `ANTHROPIC_API_KEY` with the same regex. If you ever change the env file location or key format, you need to find both.
-
-Extract this to a shared utility — maybe in a `env.ts` or even just a function in `config.ts`, which is currently underutilized.
-
----
-
-## 5. config.ts is dead code
-
-`config.ts` defines a config schema and loader, but nothing imports it. `vm.ts` hardcodes `-m 4G` and `ubuntu:24.04`. This is confusing for someone reading the codebase — it looks like configuration is supported but it silently isn't.
-
-Either wire it in or remove it. Dead code that looks intentional is worse than no code at all, because it misleads people about how the system works.
-
----
-
-## 6. Pager logic is duplicated
-
-The `diff` and `show` commands both have identical pager-fallback logic (lines 551-560 and 590-599):
-
-```typescript
-const lines = output.split("\n").length
-const termHeight = process.stdout.rows || 24
-if (lines > termHeight) {
-  const pager = Bun.spawn(["less", "-R"], { ... })
-  // ...
-}
-```
-
-This is a natural utility function: `pipeToPagedOutput(content: string)`.
-
----
-
-## 7. ANSI codes are scattered and ad-hoc
-
-The `list` command defines its own color constants inline (lines 255-261). The spinner uses raw escape codes. `markdown.ts` uses them too. The error/success icons (`✖`, `✔`, `◆`) are hardcoded at each call site.
-
-A small `fmt.ts` or extending `spinner.ts` into a more general terminal output module would centralize this. You don't need a full library — just a handful of named helpers like `fmt.dim()`, `fmt.green()`, `fmt.success()`, `fmt.error()`. This also makes it trivial to add `--no-color` support later.
-
----
-
-## 8. `git.branchExists()` has a surprising side effect
-
-This function is named like a pure query, but it calls `git fetch origin` on every invocation (line 29). That means every `sandlot new` makes a network round-trip, which could be slow or fail on spotty connections. The caller (`createWorktree`) has no indication this will happen.
-
-At minimum, document this. Ideally, separate the fetch from the check, or accept a `fetch?: boolean` option.
-
----
-
-## 9. `vm.create()` is doing too many things
-
-This ~90-line function handles: container creation, apt package installation, host symlink setup, Bun installation, Claude Code installation, git config, API key provisioning, activity hook installation, and settings file creation.
-
-Breaking this into named phases (even just private helper functions within `vm.ts`) would make it easier to debug provisioning failures and to modify individual steps. Something like:
-
-```
-createContainer() → installPackages() → installTooling() → configureEnvironment()
-```
-
----
-
-## 10. State file has no concurrency protection
-
-`state.ts` does load → modify → save with no locking. If a user runs `sandlot new foo` and `sandlot close bar` at the same time, one write can clobber the other. For a CLI tool this is unlikely but not impossible — especially since `sandlot new` is long-running (it starts a container, launches Claude, and then saves on exit).
-
-A simple approach: use an atomic write pattern (write to `.state.json.tmp`, then rename). That at least prevents partial writes. Full advisory locking is probably overkill.
-
----
-
-## 11. Inconsistent error presentation
-
-Comparing error output across commands:
-
-- `new`: `✖ Branch name or prompt is required.`
-- `show`: `No session found for branch "${branch}".` (no icon)
-- `log`: `✖ git log failed` (lowercase, terse)
-- `vm start`: `✖ ${(err as Error).message ?? err}` (raw error message)
-
-A consistent `die(message)` or `fatal(message)` function that always formats the same way would clean this up.
-
----
-
-## 12. The fish completions string will rot
-
-The ~55-line hardcoded fish completions block (lines 746-801) needs to be manually updated whenever a command, option, or subcommand changes. It's easy to forget.
-
-You could generate this from Commander's own command tree at runtime — Commander exposes the registered commands and options. This way completions are always in sync.
-
----
-
-## Summary of Suggested Extractions
-
-| New module | What moves there |
-|---|---|
-| `commands/*.ts` | Individual command handlers out of cli.ts |
-| `fmt.ts` | ANSI helpers, `die()`, `success()`, pager |
-| `env.ts` (or extend `config.ts`) | API key reading, env file parsing |
-| `vm.ts` internal helpers | Break `create()` into phases; add `claudePipe()` |
-
-The architecture is sound. These are refinements to make the codebase match the quality of the design. The core module split (`git` / `vm` / `state`) is right — it's the glue layer in `cli.ts` that needs the most attention.

src/cli.ts

@@ -8,7 +8,7 @@ import { action as listAction } from "./commands/list.ts"
 import { action as openAction } from "./commands/open.ts"
 import { action as reviewAction } from "./commands/review.ts"
 import { action as shellAction } from "./commands/shell.ts"
-import { closeAction } from "./commands/close.ts"
+import { action as closeAction } from "./commands/close.ts"
 import { action as mergeAction } from "./commands/merge.ts"
 import { action as saveAction } from "./commands/save.ts"
 import { action as diffAction } from "./commands/diff.ts"

src/commands/close.ts

@@ -7,7 +7,7 @@ import * as vm from "../vm.ts"
 import * as state from "../state.ts"
 import { die } from "../fmt.ts"
 
-export async function closeAction(branch: string, opts: { force?: boolean } = {}) {
+export async function action(branch: string, opts: { force?: boolean } = {}) {
   const root = await git.repoRoot()
   const session = await state.getSession(root, branch)
   const worktreeAbs = session?.worktree ?? join(homedir(), '.sandlot', basename(root), branch)

src/commands/list.ts

@@ -36,8 +36,7 @@ export async function action(opts: { json?: boolean }) {
   }
 
   if (sessions.length === 0) {
-    if (opts.json) console.log("[]")
-    else console.log("◆ No active sessions.")
+    console.log("◆ No active sessions.")
     return
   }
 

src/commands/merge.ts

@@ -4,7 +4,7 @@ import * as vm from "../vm.ts"
 import * as state from "../state.ts"
 import { spinner } from "../spinner.ts"
 import { die } from "../fmt.ts"
-import { closeAction } from "./close.ts"
+import { action as closeAction } from "./close.ts"
 
 export async function action(branch: string) {
   const root = await git.repoRoot()

src/commands/new.ts

@@ -5,6 +5,7 @@ import * as git from "../git.ts"
 import * as vm from "../vm.ts"
 import * as state from "../state.ts"
 import { spinner } from "../spinner.ts"
+import { die } from "../fmt.ts"
 import { getApiKey } from "../env.ts"
 import { renderMarkdown } from "../markdown.ts"
 import { saveChanges } from "./helpers.ts"
@@ -63,8 +64,7 @@ export async function action(
   if (!branch && opts.print) {
     branch = await branchFromPrompt(opts.print)
   } else if (!branch) {
-    console.error("✖ Branch name or prompt is required.")
-    process.exit(1)
+    die("Branch name or prompt is required.")
   } else if (branch.includes(" ")) {
     // If the "branch" contains spaces, it's actually a prompt — derive the branch name
     prompt = branch
@@ -75,8 +75,7 @@ export async function action(
 
   const existing = await state.getSession(root, branch)
   if (existing) {
-    console.error(`✖ Session "${branch}" already exists. Use "sandlot open ${branch}" to re-enter it.`)
-    process.exit(1)
+    die(`Session "${branch}" already exists. Use "sandlot open ${branch}" to re-enter it.`)
   }
 
   const spin = spinner("Creating worktree", branch)

src/fmt.ts

@@ -2,11 +2,9 @@
 
 export const reset = "\x1b[0m"
 export const dim = "\x1b[2m"
-export const bold = "\x1b[1m"
 export const green = "\x1b[32m"
 export const yellow = "\x1b[33m"
 export const cyan = "\x1b[36m"
-export const white = "\x1b[37m"
 
 // ── Formatted output ────────────────────────────────────────────────