diff --git a/data/agents/user-preferences.md b/data/agents/user-preferences.md deleted file mode 100644 index 51b68d9..0000000 --- a/data/agents/user-preferences.md +++ /dev/null @@ -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. diff --git a/data/prompts/user-preferences.md b/data/prompts/user-preferences.md index 51b68d9..8d2a4fd 100644 --- a/data/prompts/user-preferences.md +++ b/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.] diff --git a/doc/codebase-review.md b/doc/codebase-review.md new file mode 100644 index 0000000..c3ac6ab --- /dev/null +++ b/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.