From 30fdc7d67344e31955fccf4f4c6e29ca0fd41943 Mon Sep 17 00:00:00 2001 From: Ollie Agent Date: Sun, 16 Aug 2026 19:21:58 +0200 Subject: [PATCH] Remove obsolete codebase review --- doc/codebase-review.md | 189 ----------------------------------------- 1 file changed, 189 deletions(-) delete mode 100644 doc/codebase-review.md diff --git a/doc/codebase-review.md b/doc/codebase-review.md deleted file mode 100644 index c3ac6ab..0000000 --- a/doc/codebase-review.md +++ /dev/null @@ -1,189 +0,0 @@ -# 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.