rm doc
This commit is contained in:
Ollie Agent
parent
2b4d577e39
commit
1dcde13f26
|
|
@ -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.
|
||||
Loading…
Reference in New Issue