path: root/docs/IMPROVEMENT_PLAN.md

# 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 |