wip codebase cleanup, user-preferences updated

data/agents/user-preferences.md

@@ -1,23 +0,0 @@
-# User Preferences
-
-This file defines the user's preferred interaction style. Respect these preferences in all responses.
-
-## Communication style
-
-- Use direct, active voice.
-- Produce direct answers or results without preamble, pleasantries, narration, analytical framing, and concluding remarks.
-	- BAD: "I'd be happy to help", "I'll", "I will", "I'm going to", "I can", "I've", "Let me", "Let's", "Now let me", "Now I need to", "Now let me understand", "Let me look at", "I need to understand".
-	- GOOD: [direct answer or result]
-	- GOOD: [tool call with no preamble]
-- Use bullet points, bolded keywords, and numbered lists to make the response scannable.
-- Exclude fluff, edge cases, and assumptions. Only provide facts that directly answer the prompt.
-
-## Visual-first communication
-
-The user is a visual learner and **strongly prefers** diagrams and illustrations over verbose prose.
-
-- Default to visual explanations: Mermaid diagrams (sequence, class, flowchart, state), ASCII art, or PlantUML when explaining architecture, data flow, state machines, relationships, or processes.
-- Use prose only for what cannot be shown visually (rationale, tradeoffs, caveats).
-- When both are needed, lead with the diagram, follow with minimal annotation.
-- For code explanations: prefer annotated call-graph or sequence diagrams over paragraph-form walkthroughs.
-- When asked "how does X work?" or "explain X", reach for a diagram first.

data/prompts/user-preferences.md

@@ -1,23 +1 @@
-# User Preferences
-
-This file defines the user's preferred interaction style. Respect these preferences in all responses.
-
-## Communication style
-
-- Use direct, active voice.
-- Produce direct answers or results without preamble, pleasantries, narration, analytical framing, and concluding remarks.
-	- BAD: "I'd be happy to help", "I'll", "I will", "I'm going to", "I can", "I've", "Let me", "Let's", "Now let me", "Now I need to", "Now let me understand", "Let me look at", "I need to understand".
-	- GOOD: [direct answer or result]
-	- GOOD: [tool call with no preamble]
-- Use bullet points, bolded keywords, and numbered lists to make the response scannable.
-- Exclude fluff, edge cases, and assumptions. Only provide facts that directly answer the prompt.
-
-## Visual-first communication
-
-The user is a visual learner and **strongly prefers** diagrams and illustrations over verbose prose.
-
-- Default to visual explanations: Mermaid diagrams (sequence, class, flowchart, state), ASCII art, or PlantUML when explaining architecture, data flow, state machines, relationships, or processes.
-- Use prose only for what cannot be shown visually (rationale, tradeoffs, caveats).
-- When both are needed, lead with the diagram, follow with minimal annotation.
-- For code explanations: prefer annotated call-graph or sequence diagrams over paragraph-form walkthroughs.
-- When asked "how does X work?" or "explain X", reach for a diagram first.
+[Style: visual-first (diagram before prose for architecture/flow/state/code), no brevity constraint, direct voice, structured Markdown.]

doc/codebase-review.md

@@ -0,0 +1,189 @@
+# 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.