# Codebase Review

Date: 2026-08-15

## Scope

Static review of the Go source, tests, `virtfs`, repository metadata, and build/test configuration.

Validation passed:

- `go test ./...`
- `cd virtfs && go test ./...`
- `go vet ./...`

The working tree already contained unrelated changes:

- Deleted `data/agents/user-preferences.md`
- Modified `data/prompts/user-preferences.md`

## High-priority findings

### 1. Recoverable compaction errors panic the process

Location: `cmd/olliesrv/internal/agent/loop.go:212-225`

`autoCompact` converts compaction failures into `panic(...)`. A backend timeout, malformed response, or temporary API failure can terminate the server instead of returning an agent error.

**Recommendation:** propagate the error through the turn loop and emit a normal agent error.

### 2. `ModelCache.Get` exposes mutable internal storage

Location: `cmd/olliesrv/internal/fs/cache.go:22-31`

`Get` returns the cache's backing slice directly. Callers can mutate cached data without synchronization, causing corruption or a data race.

**Recommendation:** return `slices.Clone(c.data)` or copy the data before unlocking.

### 3. Concurrent cache refreshes duplicate backend calls

Location: `cmd/olliesrv/internal/fs/cache.go:22-57`

Multiple callers can observe an unfetched cache and all refresh it concurrently. Each refresh instantiates backends and queries every model list.

**Recommendation:** use singleflight or track refresh state under the mutex.

### 4. Authentication token generation ignores cryptographic errors

Location: `cmd/toolsrv/internal/server/server.go:103-106`

`randomToken` ignores the error returned by `crypto/rand.Read`. It can return a partially initialized or zero-filled token.

**Recommendation:** return `(string, error)` and handle the error at the authentication call site. `GenerateSecret` already follows the safer pattern.

### 5. Persistence errors are silently discarded

Locations: `cmd/olliesrv/internal/session/persist.go:94-95`, `105-109`

Calls such as `os.MkdirAll` and `os.Remove` ignore errors. Persistence can appear successful while losing session state.

**Recommendation:** return and wrap errors. At minimum, log failed cleanup and directory creation.

## Confirmed dead code

### 6. `ToolTag` is defined but never used

Location: `format/format.go:36-37`

`ToolTag = "[[[tool]]]"` has no workspace references. Tool output uses `ToolDelim(name)`.

**Recommendation:** remove it unless it is intentionally part of a public compatibility API.

### 7. Stale root-level build artifacts

The workspace contains ignored top-level binaries:

- `Ollie`
- `olliesrv`
- `ollie-9p`
- `lsp_definition`
- `ollie-remote`

They add noise and consume workspace space.

**Recommendation:** keep build outputs under a dedicated `bin/` or build directory and clean stale root-level artifacts.

## Redundancies and inefficiencies

### 8. Repeated manual byte-slice copying

Locations: `cmd/olliesrv/internal/agent/agent.go:198-203`, `207-212`, `450-452`

The code manually copies byte slices even though the project already uses Go 1.25 and `slices.Clone` elsewhere.

**Recommendation:** use `slices.Clone` consistently.

### 9. Chat reader copies data while holding a read lock

Location: `cmd/olliesrv/internal/agent/agent.go:184-193`

The entire unread chat-log portion is allocated and copied while holding `chatMu.RLock`. Large logs or multiple readers extend lock duration.

**Recommendation:** snapshot boundaries under the lock, then copy after unlocking while preserving backing-array lifetime safely.

### 10. `EnsureTrailingNewline` increments the version unnecessarily

Location: `cmd/olliesrv/internal/agent/agent.go:146-154`

`chatVers` increments even when the log is empty or already ends with a newline.

**Recommendation:** increment only when a newline is appended.

### 11. Tool-call formatting destroys whitespace

Location: `format/event.go:18-20`

`strings.Fields` and `strings.Join` normalize all whitespace in tool-call arguments. This can alter JSON string values and is unnecessary for fenced display.

**Recommendation:** preserve the content verbatim or use a JSON formatter that preserves string values.

## Bad code smells

### 12. Exported access to internal synchronization primitives

Locations: `cmd/olliesrv/internal/agent/agent.go:156-160`, `170-171`

`ChatMu`, `ChatCond`, and `ChatLog` expose internal locking and slice ownership. Callers must correctly pair locks with direct access to mutable internal state.

**Recommendation:** expose snapshot/blocking APIs instead of the mutex.

### 13. `SetEnv` can fail through unavailable tool-server infrastructure

Location: `cmd/olliesrv/internal/agent/agent.go:509-512`

The method calls `ag.runtime.ToolServer.SetEnv` directly, unlike related methods that account for missing infrastructure. Paused or partially restored agents may not have a tool server.

**Recommendation:** make unavailable-server behavior explicit and return an error when the operation cannot be performed.

### 14. `RefreshTools` has the same infrastructure problem

Location: `cmd/olliesrv/internal/agent/agent.go:535-540`

The method directly calls `ag.runtime.ToolServer.ListTools`, which can fail for paused or partially restored agents.

**Recommendation:** return an error or provide an observable no-op status.

### 15. Session lookup by ID is unnecessarily linear

Location: `cmd/olliesrv/internal/session/registry.go:74-86`

`Lookup` performs an O(1) name lookup followed by an O(n) scan for IDs.

**Recommendation:** maintain a second ID index or explicit name/ID indexes.

### 16. Session restoration launches unbounded goroutines

Location: `cmd/olliesrv/internal/session/persist.go:143-161`

One goroutine is created per persisted session file. A large or unexpected session directory can create excessive concurrency and backend initialization load.

**Recommendation:** use bounded worker concurrency.

### 17. `Runtime.Messages` appears to be dead state

Location: `cmd/olliesrv/internal/agent/runtime.go:131-143`

`BuildRuntime` stores tool-list error messages in `Runtime.Messages`, but workspace searches found no meaningful consumers.

**Recommendation:** remove the field if it is not part of an intended diagnostics API, or replace it with explicit diagnostics handling.

### 18. `Preamble` uses linear lookup for named sections

Location: `cmd/olliesrv/internal/agent/runtime.go:18-63`

Named sections are stored in a slice and searched linearly. The current number of sections is small, so this is not a significant performance issue, but the keyed API and list representation add complexity.

**Recommendation:** use an index/map or document the intentionally fixed small cardinality.

## Suggested priority order

1. Replace compaction panic with normal error propagation.
2. Fix token-generation error handling.
3. Fix `ModelCache` ownership and duplicate refreshes.
4. Stop ignoring persistence errors.
5. Remove or confirm `ToolTag` and `Runtime.Messages`.
6. Fix tool-call whitespace normalization.
7. Encapsulate chat-log synchronization.
8. Bound session restoration concurrency.
9. Add an ID index to the session registry.
10. Clean up build artifacts and minor redundancies.