From 1dcde13f2691050611afbbafd0c3a41c8da5eb25 Mon Sep 17 00:00:00 2001 From: Ollie Agent Date: Sun, 9 Aug 2026 12:55:02 +0200 Subject: [PATCH] rm doc --- doc/code-review-2026-08-09.md | 196 ---------------------------------- 1 file changed, 196 deletions(-) delete mode 100644 doc/code-review-2026-08-09.md diff --git a/doc/code-review-2026-08-09.md b/doc/code-review-2026-08-09.md deleted file mode 100644 index 267767b..0000000 --- a/doc/code-review-2026-08-09.md +++ /dev/null @@ -1,196 +0,0 @@ -# Codebase Analysis: Redundancies, Dead Code & Improvement Patterns - -_2026-08-09_ - -## 1. Dead Code (safe to remove) - -| Item | Location | Notes | -|------|----------|-------| -| Truncated `geminiKey` stub | `backend/gemini.go:20-21` | Comment-only, no body | -| `kiroMetadataEvent` type | `backend/codewhisperer_internal.go:122` | Defined but never decoded | -| Orphaned doc comments | `agent/history.go:86, :343` | Document deleted functions | -| `fsedsl` dead exports | `fsedsl/tree.go`, `fsedsl/decl.go` | `WithReadOnly`, `WithIndex`, `WithResolver`, `CreateFile`, `Alias`, `ValidateReadMode` — only used in self-tests | -| `fsedsl` filesystem-backed tree logic | `fsedsl/tree.go:230-370` | Vestigial from pre-EDSL era; only `MkdirAll` is used in production | - -## 2. Redundancies - -### Critical: `SetModel` cache bug - -`backend/codewhisperer.go:52` — `SetModel` doesn't reset `ctxLength`. After switching models, stale context length is returned. Other backends correctly invalidate. - -### High-impact duplications - -| What | Where | Fix | -|------|-------|-----| -| UUIDv4 generation (3 identical copies) | `agent/history.go:437`, `session/session.go:52`, `backend/codewhisperer_internal.go:647` | Extract to `paths.NewUUID()` | -| Backend boilerplate (Name/Model/SetModel/DefaultModel) | All 4 backend files | `baseBackend` embedded struct | -| Factory post-processing (`if bc.Model` × 7) | `backend/new.go:34-136` | Single post-construction call | -| RPC wire types (3 definitions, type mismatch) | `toolsrv/conn.go`, `cmd/ollie-remote/main.go`, tests | Export from `toolsrv/rpcwire.go` | -| Tool listing rendering (identical format) | `agent/runtime.go:90`, `toolsrv/server.go:410` | Single shared function | -| LSP cmd boilerplate (7 identical mains) | `tools/lsp/cmd/*/main.go` | `tools/lsp/cmdutil` package | -| XDG_RUNTIME_DIR resolution (4 implementations, 4 different fallbacks) | `toolsrv/transport.go`, `tools/lsp/adapter.go`, `cmd/olliesrv/main.go`, `toolsrv/shell.go` | `paths.RuntimeDir()` | -| Git repo detection (3 copies) | `agent/agent.go:402`, `agent/prompt_resolver.go:20`, `cmd/ollie-remote/main.go:379` | Utility in `paths/` | -| Stream scanner setup (identical 2-liner, 3×) | `ollama.go:175`, `openai.go:237`, `anthropic.go:322` | `newStreamScanner(r io.Reader)` helper | -| `parseRetryAfter` misplaced | `backend/openai.go:365-378` (called from `backend.go:163`) | Move to `backend.go` | -| Tool encoding loops (4 separate implementations) | Each backend file | Generic `mapTools[T]()` helper | -| ContentBlock→image conversion (3 copies) | `anthropic.go`, `openai.go`, `codewhisperer.go` | Shared helper with provider-specific type mapper | - -### Medium-impact - -| What | Where | Fix | -|------|-------|-----| -| `kiroUserDataDir()` reimplements `paths.DataDir()` | `backend/codewhisperer_internal.go` | Use `paths.DataDir()` | -| 9P path helpers duplicated across `cmd/` binaries | Various cmd/ files | Private, small — acceptable | -| `os.MkdirAll` without error check (6 places) | Various | Intentional best-effort — acceptable | - -## 3. Structural Issues - -### God files - -| File | Lines | Problem | -|------|-------|---------| -| `fs/handlers.go` | 888 | Mixes root, session, and agent handlers. Split into 3 files. | -| `agent/loop.go` | 709 | `run()` is 300+ lines combining streaming, retry, tool exec, caching. Extract sub-functions. | -| `agent/agent.go` | 661 | Facade over 5+ subsystems + 5 pass-through proxy methods (~50 lines of pure delegation) | -| `toolsrv/shell.go` | 671 | Validation + sandbox config + execution + output limiting — moderate; less urgent | - -### Inconsistencies - -| Pattern | Inconsistency | -|---------|---------------| -| `Anthropic.BaseURL` exported | Other backends keep `baseURL` unexported | -| Agent ctl dispatch | Uses package-level map; session/root use inline maps | -| Error wrapping | Mix of `%w` and `%v` in `backend/` — breaks `errors.Is/As` | -| Naming | `CodeWhispererBackend` when everything else says "kiro" | -| Context length | Hardcoded (Anthropic) vs dynamic (all others) | -| Model listing | Hardcoded static list (Anthropic) vs live API (others) — will go stale | -| Tool server access | Three paths to the same `*toolsrv.Conn` | -| Event handler wiring | Done in `fs/newroot.go` and `fs/handlers.go` differently; undocumented contract | - -### Unclear boundaries - -- `fsedsl` is a "reusable library" with exactly one consumer (`fs/`). Its dead exports and filesystem-backed tree code add cognitive load without value. -- `agent.Agent` exposes detached-process methods that are pure delegation — the fs layer could call `ag.ToolServer()` directly, removing unnecessary indirection. - -## 4. Recommended Patterns - -### 4.1 — Extract `baseBackend` struct (highest ROI) - -```go -// backend/base.go -type baseBackend struct { - name string - model string - client *http.Client - ctxLength int - ctxModel string -} - -func (b *baseBackend) Name() string { return b.name } -func (b *baseBackend) Model() string { return b.model } -func (b *baseBackend) SetModel(m string) { b.model = m; b.ctxLength = 0 } -``` - -Embed in each backend. Saves ~40 lines. Fixes the CodeWhisperer cache bug automatically. - -### 4.2 — Shared UUID utility - -```go -// paths/uuid.go -func NewUUID() string { - var b [16]byte - rand.Read(b[:]) - b[6] = (b[6] & 0x0f) | 0x40 - b[8] = (b[8] & 0x3f) | 0x80 - return fmt.Sprintf("%08x-%04x-%04x-%04x-%012x", b[0:4], b[4:6], b[6:8], b[8:10], b[10:16]) -} -``` - -Replaces 3 identical copies. - -### 4.3 — Split `fs/handlers.go` - -Mechanical split by domain: -- `fs/handlers_root.go` — root-level ctl + read handlers (backends, models, agents, tools, help) -- `fs/handlers_session.go` — session ctl + env/pause/connect/bypass -- `fs/handlers_agent.go` — agent ctl handlers map + prompt/fifo/chat/log/plan/cfg/stats/proc - -No logic changes needed. - -### 4.4 — `paths.RuntimeDir()` with one canonical fallback - -```go -// paths/paths.go -func RuntimeDir() string { - if d := os.Getenv("XDG_RUNTIME_DIR"); d != "" { - return d - } - return fmt.Sprintf("/run/user/%d", os.Getuid()) -} -``` - -Eliminates 4 divergent implementations with different (incorrect) fallback behaviors. - -### 4.5 — Export RPC wire types from `toolsrv/` - -```go -// toolsrv/rpcwire.go -type RPCRequest struct { - JSONRPC string `json:"jsonrpc"` - ID json.RawMessage `json:"id"` - Method string `json:"method"` - Params json.RawMessage `json:"params,omitempty"` -} - -type RPCResponse struct { - JSONRPC string `json:"jsonrpc"` - ID json.RawMessage `json:"id"` - Result json.RawMessage `json:"result,omitempty"` - Error *RPCError `json:"error,omitempty"` -} -``` - -Fixes the `int64` vs `json.RawMessage` type mismatch between client and server (potential serialization bug). - -### 4.6 — `tools/lsp/cmdutil` for LSP command binaries - -7 binaries with nearly identical main functions. Extract the boilerplate into a shared harness that takes a method name and params builder. - -### 4.7 — Unified factory in `backend/new.go` - -```go -// Before (7 repetitions): -case "ollama": - b, err := NewOllama(bc.Host) - if err != nil { return nil, err } - if bc.Model != "" { b.SetModel(bc.Model) } - return b, nil - -// After: -b, err := construct(which, bc) -if err != nil { return nil, err } -if bc.Model != "" { b.SetModel(bc.Model) } -return b, nil -``` - -## 5. Priority Order - -| # | Change | Effort | Risk | -|---|--------|--------|------| -| 1 | Fix CodeWhisperer `SetModel` cache bug | 1 line | None | -| 2 | Delete dead code (gemini stub, orphaned comments, `kiroMetadataEvent`) | 5 min | None | -| 3 | Extract UUID utility | 15 min | None | -| 4 | Merge tool-listing renderers | 15 min | None | -| 5 | Extract `baseBackend` | 30 min | Low | -| 6 | Unify factory post-processing | 15 min | Low | -| 7 | Split `fs/handlers.go` | 30 min | None | -| 8 | Add `paths.RuntimeDir()` | 20 min | Low | -| 9 | Export shared RPC wire types | 45 min | Low | -| 10 | Factor `tools/lsp/cmdutil` | 1–2 hr | Low | -| 11 | Extract `run()` sub-functions in agent/loop.go | 1–2 hr | Medium | -| 12 | Move `parseRetryAfter` to `backend.go` | 5 min | None | -| 13 | Rename `CodeWhispererBackend` → `KiroBackend` | 30 min | Low | -| 14 | Remove agent detached-process proxy methods | 1 hr | Medium | -| 15 | Audit/trim `fsedsl` dead exports | 1 hr | Medium | - -Items 1–7 are an afternoon's work with near-zero risk. Items 8–10 are a solid second pass. Items 11–15 are worthwhile but require more care.