From 8d31e9ec689f5a789bee244de1cdbcbb3d753bbf Mon Sep 17 00:00:00 2001 From: Chris Wanstrath Date: Fri, 20 Feb 2026 15:55:54 -0800 Subject: [PATCH] Remove REVIEW.md, standardize close.ts export, and minor cleanup --- REVIEW.md | 156 ------------------------------------------ src/cli.ts | 2 +- src/commands/close.ts | 2 +- src/commands/list.ts | 3 +- src/commands/merge.ts | 2 +- src/commands/new.ts | 7 +- src/fmt.ts | 2 - 7 files changed, 7 insertions(+), 167 deletions(-) delete mode 100644 REVIEW.md diff --git a/REVIEW.md b/REVIEW.md deleted file mode 100644 index 7815e7d..0000000 --- a/REVIEW.md +++ /dev/null @@ -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 -``` - -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. diff --git a/src/cli.ts b/src/cli.ts index 9c39638..d23a44a 100755 --- a/src/cli.ts +++ b/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" diff --git a/src/commands/close.ts b/src/commands/close.ts index 8e9c0bf..47f3a94 100644 --- a/src/commands/close.ts +++ b/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) diff --git a/src/commands/list.ts b/src/commands/list.ts index af86c5c..aff0264 100644 --- a/src/commands/list.ts +++ b/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 } diff --git a/src/commands/merge.ts b/src/commands/merge.ts index abc6db2..d7a24c1 100644 --- a/src/commands/merge.ts +++ b/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() diff --git a/src/commands/new.ts b/src/commands/new.ts index a0988f6..06faba1 100644 --- a/src/commands/new.ts +++ b/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) diff --git a/src/fmt.ts b/src/fmt.ts index 3591f44..7a475db 100644 --- a/src/fmt.ts +++ b/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 ────────────────────────────────────────────────