1 files changed, 278 insertions, 0 deletions
diff --git a/docs/IMPROVEMENT_PLAN.md b/docs/IMPROVEMENT_PLAN.md
new file mode 100644
index 0000000..db5d6a9
--- /dev/null
+++ b/docs/IMPROVEMENT_PLAN.md
@@ -0,0 +1,278 @@
+# Improvement Plan
+
+Comprehensive assessment of stability, architectural coherence, simplicity and cleanliness issues, with prioritized fixes.
+
+---
+
+## Critical Bugs (fix immediately)
+
+### 1. `sandboxDir` variable shadowing — git clone failures
+
+**File:** `internal/executor/claude.go`, lines ~119–137
+
+Both `setupSandbox` calls inside `Run()` use short variable declarations (`:=`) inside nested blocks, creating inner variables that shadow the outer `var sandboxDir string`. The outer variable is always `""`.
+
+**Effects:**
+- `teardownSandbox` is never called → agent work discarded, changes never pushed to origin
+- Sandboxes accumulate in `/tmp/claudomator-sandbox-*` until disk is full
+- BLOCKED task resume always clones fresh → loses agent's partial work from first run
+- Error messages omit sandbox path, making debugging harder
+
+**Fix:**
+
+```go
+// In the resume path (inside `if projectDir != ""`):
+var err error
+sandboxDir, err = setupSandbox(t.Agent.ProjectDir, r.Logger)  // = not :=
+
+// In the non-resume path (inside `else if projectDir != ""`):
+var err error
+sandboxDir, err = setupSandbox(t.Agent.ProjectDir, r.Logger)  // = not :=
+```
+
+Both sites already declare `var err error` on the line above. Simply drop the `:` from `:=`.
+
+### 2. Hardcoded `master` branch in sandbox teardown
+
+**File:** `internal/executor/claude.go`, `teardownSandbox()`, line ~377
+
+```go
+git pull --rebase origin master   // fails on repos using "main"
+```
+
+**Fix:** Detect the remote default branch before pushing:
+
+```go
+branch, _ := exec.Command("git", "-C", sandboxDir, "rev-parse", "--abbrev-ref", "HEAD").Output()
+defaultBranch := strings.TrimSpace(string(branch))
+if defaultBranch == "" || defaultBranch == "HEAD" {
+    defaultBranch = "main"
+}
+// then use defaultBranch instead of "master"
+```
+
+---
+
+## High Priority — Stability & Correctness
+
+### 3. GeminiRunner is a non-functional stub in production code
+
+**File:** `internal/executor/gemini.go`
+
+`execOnce()` spawns a goroutine writing hardcoded JSON to a pipe instead of running the `gemini` binary. Any task with `agent.type: "gemini"` silently returns fake output.
+
+**Options (choose one):**
+- **Implement properly:** Mirror ClaudeRunner structure — subprocess + process group + stream parsing + sandbox support. GeminiRunner must also implement `LogPather`.
+- **Remove until ready:** Delete `gemini.go`, remove `GeminiRunner` from `NewPool` in `cli/serve.go`, and gate the `Classifier` on Claude-only operation. Add `// TODO: GeminiRunner not implemented` clearly.
+
+The stub in production is a correctness hazard. Removing it is safer than leaving it in place.
+
+### 4. Atomic execution creation + state update
+
+**Files:** `internal/executor/executor.go` `execute()`, `internal/storage/db.go`
+
+`CreateExecution` and `UpdateTaskState(RUNNING)` are two separate DB writes with no transaction. A crash between them leaves the task PENDING with an orphaned RUNNING execution record. `RecoverStaleRunning` handles the RUNNING-task case but not the PENDING-task-with-running-execution case.
+
+**Fix:** Add `storage.DB.CreateExecutionAndSetRunning(exec, taskID)` that wraps both writes in a single transaction.
+
+### 5. `context.Background()` in resume submission
+
+**File:** `internal/api/server.go`, `handleAnswerQuestion()`
+
+```go
+p.SubmitResume(context.Background(), task, exec)
+```
+
+This detaches the resume from any server lifecycle context. If the server has a root context with a shutdown deadline, resumes submitted this way will not be cancelled.
+
+**Fix:** Pass the server's root context (or a derived context) instead of `Background()`.
+
+### 6. Pool `dispatch()` goroutine leaks on server shutdown
+
+**File:** `internal/executor/executor.go`
+
+`NewPool` starts `go p.dispatch()` but `workCh` is never closed. On server shutdown, `dispatch()` blocks forever on `<-p.workCh`. The pool has no `Shutdown()` method.
+
+**Fix:** Add `Pool.Shutdown(ctx context.Context)` that closes `workCh` and waits for active workers to drain.
+
+---
+
+## Medium Priority — Architectural Coherence
+
+### 7. Priority field is stored but never dispatched
+
+**Files:** `internal/task/task.go`, `internal/executor/executor.go`
+
+Priority is parsed, stored in SQLite, returned via API — but `dispatch()` uses a single FIFO channel, ignoring priority entirely.
+
+**Options:**
+- Use a priority queue (e.g., `container/heap`) keyed by `Priority` for `workCh`
+- Or remove `Priority` from `Task` and documentation until implemented — don't ship half-features
+
+### 8. RetryConfig is stored but retry is manual
+
+**Files:** `internal/task/task.go`, `internal/executor/executor.go`
+
+`RetryConfig.MaxAttempts` and `Backoff` are stored but never read. Every "retry" is a manual user action via `/resume`.
+
+**Options:**
+- Implement automatic retry in `handleRunResult` when `MaxAttempts > 1` and `currentAttempt < MaxAttempts`
+- Or remove `RetryConfig` from the schema/API until implemented
+
+### 9. Changestats extracted in two places
+
+**Files:** `internal/executor/executor.go` `handleRunResult()`, `internal/api/server.go` `processResult()`
+
+Both call `task.ParseChangestatFromFile()` on the same file and write to the same row. The second write is idempotent but the duplication is confusing.
+
+**Fix:** Remove the extraction from `api.Server.processResult()`. The pool already handles it reliably before broadcasting the result.
+
+### 10. `withFailureHistory` shallow copy is unsafe
+
+**File:** `internal/executor/executor.go`
+
+```go
+copy := *t      // copies struct
+copy.Agent = t.Agent  // copies AgentConfig struct, slices share backing array
+```
+
+If any downstream code appends to `copy.Agent.AllowedTools`, it will mutate the original. The current code only writes to `SystemPromptAppend` (a string), so this is safe today but fragile.
+
+**Fix:** Deep-copy slices in AgentConfig:
+```go
+agent := t.Agent
+agent.AllowedTools = append([]string(nil), t.Agent.AllowedTools...)
+agent.DisallowedTools = append([]string(nil), t.Agent.DisallowedTools...)
+agent.ContextFiles = append([]string(nil), t.Agent.ContextFiles...)
+agent.AdditionalArgs = append([]string(nil), t.Agent.AdditionalArgs...)
+copy.Agent = agent
+```
+
+### 11. Additive migration strategy has no version tracking
+
+**File:** `internal/storage/db.go` `migrate()`
+
+Migrations are applied in code order; the only idempotency guard is ignoring "column already exists" errors. There is no way to detect if a migration was partially applied, rolled back incorrectly, or was applied out of order by a different code version.
+
+**Fix (minimal):** Use a `schema_migrations` table with a version counter. Apply only migrations with version > current max. This prevents re-running migrations and makes the migration history explicit.
+
+### 12. Server test-seam fields in production struct
+
+**File:** `internal/api/server.go`
+
+```go
+elaborateCmdPath string   // overrides claudeBinPath in tests
+validateCmdPath  string   // overrides claudeBinPath in tests
+```
+
+These exist solely to inject test paths. They are an internal testing seam leaked into the production struct.
+
+**Fix:** Extract an `Elaborator` interface (e.g., `Elaborate(ctx, request, workDir string) (string, error)`) and inject it. Tests inject a fake; production builds inject the real Claude-backed implementation.
+
+---
+
+## Lower Priority — Simplicity & Cleanliness
+
+### 13. Classifier output parsing is brittle string manipulation
+
+**File:** `internal/executor/classifier.go`
+
+The classifier strips `"Loaded cached credentials."` by line scanning, strips markdown fences by `strings.TrimPrefix/TrimSuffix`, and falls back through multiple parsing paths. This will silently break if Gemini CLI output format changes.
+
+**Fix:** Request `--output-format json` and parse the structured response. Treat any parse failure as a classification error (fall back to default model), and log the raw output for debugging.
+
+### 14. `gitSafe` is inconsistently applied in teardownSandbox
+
+**File:** `internal/executor/claude.go`
+
+`setupSandbox` wraps all git commands with `gitSafe(...)` (adds `-c safe.directory=*`). `teardownSandbox` calls `git` directly without `gitSafe` on most commands. This means teardown can fail with "dubious ownership" errors in mixed-owner environments.
+
+**Fix:** Use `gitSafe` consistently in `teardownSandbox`.
+
+### 15. `tailFile` loads entire file into memory
+
+**File:** `internal/executor/claude.go`
+
+```go
+func tailFile(path string, n int) string {
+    // scans all lines, keeps last n
+}
+```
+
+For large log files this reads everything before discarding early lines. Fine in practice (stderr is typically small) but fragile if anything large is written to stderr.
+
+**Fix:** Use a circular buffer of n lines to avoid O(filesize) memory.
+
+### 16. `execute()` and `executeResume()` duplicate context/cancel/defer setup
+
+**File:** `internal/executor/executor.go`
+
+Both methods contain identical blocks for: context timeout + cancel setup, `activePerAgent` increment/decrement, `doneCh` signal, cancel registration/deregistration. This is ~40 lines duplicated.
+
+**Fix:** Extract `withWorkerSlot(ctx, t, fn func(ctx context.Context) error) error` that handles the common setup and calls `fn`. Both paths become 5–10 lines.
+
+### 17. `QuestionRegistry` and `QuestionHandler` are unused
+
+**Files:** `internal/executor/question.go`, `internal/executor/executor.go`
+
+`QuestionRegistry` is created in `NewPool` and stored on `Pool.Questions` but nothing reads from it. `QuestionHandler` interface exists but is never instantiated or wired to anything. The intended design (pool detects `tool_use` and invokes a handler) is not implemented.
+
+**Fix:** Remove `QuestionRegistry`, `QuestionHandler`, and `Pool.Questions` until the feature is actually needed. The current question flow (agent writes file, pool reads BlockedError) already works without them.
+
+### 18. Script registry endpoint is undocumented and empty
+
+**File:** `internal/api/scripts.go`
+
+The `ScriptRegistry` type and `POST /api/scripts/{name}` endpoint are defined but the registry is never populated from config. There are no example scripts. The endpoint exists in production code but can't be used.
+
+**Fix:** Either populate scripts from config (`[[scripts]]` entries in config.toml) with documentation, or remove the endpoint until it's ready.
+
+### 19. `parseStream` ignores scanner errors
+
+**File:** `internal/executor/claude.go`
+
+`parseStream()` loops over `scanner.Scan()` but never checks `scanner.Err()` after the loop. If the scanner hits a read error mid-stream, it silently stops. The function returns `nil` error, making the task appear to complete successfully when it may have lost output.
+
+**Fix:**
+```go
+if err := scanner.Err(); err != nil {
+    if streamErr == nil {
+        streamErr = fmt.Errorf("reading claude stdout: %w", err)
+    }
+}
+```
+
+### 20. Log files have no size limit or rotation
+
+**Files:** `internal/executor/claude.go`, `internal/storage/db.go`
+
+Execution logs in `~/.claudomator/executions/` have no size limit, rotation, or TTL. Long-running or repeatedly-retried tasks can produce arbitrarily large log files. Old executions are never cleaned up.
+
+**Fix (minimal):** Add a `CleanupOldExecutions(olderThan time.Duration)` method to `storage.DB` and call it on server startup (or on a ticker). Delete execution log directories for executions older than the TTL.
+
+---
+
+## Summary by Effort
+
+| # | Issue | Effort | Impact |
+|---|-------|--------|--------|
+| 1 | `sandboxDir` shadowing | XS (2 chars: `=` not `:=`) | Critical — root cause of git clone failures |
+| 2 | Hardcoded `master` branch | S | High — breaks all `main`-branch repos |
+| 3 | GeminiRunner stub | M–L | High — silent wrong output in production |
+| 4 | Atomic exec creation | S | Medium — crash-recovery edge case |
+| 5 | context.Background() resume | XS | Low — shutdown correctness |
+| 6 | Pool shutdown | S | Medium — goroutine leak on shutdown |
+| 7 | Priority dispatch | M | Medium — misleading API |
+| 8 | RetryConfig unused | S | Low — misleading API |
+| 9 | Changestats duplication | XS | Low — confusing code |
+| 10 | Shallow copy in withFailureHistory | S | Low — latent bug |
+| 11 | Migration versioning | M | Medium — operational risk |
+| 12 | Test seams in Server struct | M | Low — architecture cleanliness |
+| 13 | Brittle classifier parsing | S | Medium — silent classification failures |
+| 14 | gitSafe inconsistency | XS | Medium — owner errors in teardown |
+| 15 | tailFile memory | XS | Low — theoretical |
+| 16 | execute/executeResume duplication | M | Low — maintainability |
+| 17 | QuestionRegistry dead code | XS | Low — confusion |
+| 18 | Script registry empty | S | Low — dead endpoint |
+| 19 | parseStream ignores scanner.Err | XS | Medium — silent data loss |
+| 20 | Log file cleanup | S | Medium — disk usage |