lkn
/
ollie
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity
ollie/doc/codebase-review.md

7.1 KiB
Raw Blame History

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.