TODO.md

@@ -0,0 +1,300 @@
+# TODO
+
+Ten items, synthesised from a six-expert review panel (empirical rigour,
+security, storage/durability, applied ML, reliability, developer experience).
+Each expert read this repo and `costslayer` at `main` and reported
+independently; the findings below are the ones that survived cross-checking
+against the code.
+
+Ordered by irreversibility first — silent data loss and credential exposure
+above correctness, correctness above polish. Every claim marked **verified** was
+re-checked directly against the source, not taken on an expert's word.
+
+---
+
+### 1. `recoverTail` destroys the data it exists to protect
+
+`internal/store/shard.go:107-155` walks frames from offset 0, breaks at the
+first CRC mismatch, then unconditionally truncates:
+
+```go
+if crc32.ChecksumIEEE(body) != want {
+    break // body landed partially, or the disk lied
+}
+good = end
+}
+if good < size { return f.Truncate(good) }
+```
+
+The comment at `shard.go:13-22` assumes the only bad chunk is the *tail*.
+Nothing enforces that. Demonstrated with a probe on a scratch copy:
+
+```
+10 chunks written, file=2130 bytes, rows readable=10
+after 1-bit flip in chunk 0, rows readable=0
+after writer open (recoverTail): file=0 bytes (was 2130)
+RESULT: recoverTail PERMANENTLY DELETED 2130 of 2130 bytes
+```
+
+Worse, `store.go:255-259` means a *read* query against a not-yet-opened old
+shard goes through `OpenShard` → `recoverTail`, so browsing an old day in the UI
+can trigger the deletion.
+
+Only truncate when the first bad frame is the last frame. For interior damage,
+log the offset, skip the frame and keep scanning so later chunks stay reachable
+— or quarantine the shard to `.corrupt`. Also check the `n` returned by
+`s.f.Write(frame)` at `shard.go:184`; a short write is currently ignored.
+
+### 2. Everything is unauthenticated, on every interface, storing secrets in cleartext
+
+**Verified:** `config.go:66` sets `c.Server.Addr = ":3000"` — empty host, i.e.
+all interfaces. `Handler()` (`server.go:57-82`) wraps the mux in `logRequests`
+and nothing else. A grep for `Bearer|Authorization` across `internal/` and
+`cmd/` returns **nothing**; there is no auth primitive in the repository.
+
+The README instructs users to set `OTEL_LOG_USER_PROMPTS=1`,
+`OTEL_LOG_TOOL_CONTENT=1`, `OTEL_LOG_ASSISTANT_RESPONSES=1`. So the data
+directory holds source code, file contents, shell output and — with high
+probability — any key that passed through a session. Anyone who can reach port
+3000 reads all of it via `/api/traces`, and gets a query engine for free via
+`POST /api/query`.
+
+There is no redaction anywhere (`grep -rni redact|scrub|encrypt` returns zero
+hits outside fixtures), files are `0o644` in a `0o755` directory, and deletion
+granularity is a whole day — so the answer to "I pasted a prod key into a
+prompt" is `rm -rf` that day's traces.
+
+Bind loopback by default; refuse a non-loopback bind without an explicit token;
+require `Authorization` on `/api/*` and `/v1/*`; chmod to `0o600`/`0o700`; add a
+credential-shape redactor on the ingest path; add `fathom purge --trace` /
+`--grep`. The `[privacy]` section currently drops five identity *keys* while
+writing every prompt verbatim, which reads as more protection than it is.
+
+### 3. DNS rebinding drains the store from any web page the operator visits
+
+No `Host` allow-list, no `Origin` check, no CSRF token — `grep` for any of them
+in `internal/web/` returns nothing outside tests.
+
+An attacker page on a 1-second-TTL domain that rebinds to `127.0.0.1` makes
+`fetch('http://evil.tld:3000/api/traces?from=0&to=9e14')` **same-origin** by the
+browser's rules, so the response is readable and the whole corpus is POSTed out.
+Binding to loopback does not help; only Host validation does. This needs no
+network adjacency at all — just a browser tab.
+
+Ingest is separately CSRF-able today: `handleLogs` (`server.go:148-164`) reads
+the body and unmarshals with no `Content-Type` check, so a `text/plain` form
+post is a CORS-simple request that lands without preflight. Same for the
+insights mutation endpoints, which take no token and in one case no body.
+
+Reject requests whose `Host` is not in the configured set, reject foreign
+`Origin`, and require `application/json` on every POST.
+
+### 4. Failed flushes discard spans that were already acknowledged
+
+`store.go:165-168` detaches the buffer before attempting any I/O:
+
+```go
+s.mu.Lock()
+spans, logs := s.spanBuf, s.logBuf
+s.spanBuf, s.logBuf = nil, nil
+s.mu.Unlock()
+```
+
+Every write failure is `fail(err); continue` — nothing is ever put back. The
+exporter was told `{"partialSuccess":{}}` and 200 OK a second earlier, so it
+will never retry. Then `AddSpans` (`store.go:126-130`) hands the error to the
+*next* caller and **clears it**, so one unrelated batch eats a 500 and the store
+reports healthy again. `background()` logs nothing (the package imports no
+logger at all). `Close()` discards the same way on shutdown.
+
+Re-buffer unwritten rows under the mutex with a bounded cap, keep `flushErr`
+sticky until a flush actually succeeds, log every failure with row counts, and
+return an error *before* acknowledging when the buffer is over cap so the
+exporter retries instead of the store silently eating the batch.
+
+This compounds across the pipeline: costslayer's exporter discards the HTTP
+status entirely, so fathom can reject everything and the sender will never know.
+Fixing that side is the cheaper half.
+
+### 5. Corruption, version mismatch and dropped spans are all indistinguishable from "no data"
+
+Six separate error paths in `iterChunks` (`shard.go:208-239`) `return nil`:
+CRC mismatch, decode error, implausible length, read error. `read.go:88-93` then
+`continue`s per shard. In the probe above, `ScanSpans` returned `err == nil` with
+zero rows against a file that had ten good chunks.
+
+Two consequences. Interior bit rot makes every later chunk permanently invisible
+while queries return 200 with a smaller `rows_scanned`. And `chunk.go:254-256`
+rejects any `ver != chunkVersion`, so the day `chunkVersion` goes to 2, every
+existing data directory decodes to nothing, silently — `recoverTail` only checks
+CRC, not version, so it will not even complain.
+
+Ingest has the same shape: `otlp.go:252-254` drops span-ID-less spans with
+`continue` and no counter, while `otlpOK` always returns an empty
+`partialSuccess` — the OTLP field that exists precisely to report rejections.
+A body with the wrong shape decodes to zero spans and returns 200. And
+`nanoToMicro` returns 0 on unparsable input, so `shardDate(0)` files the span
+under `1970-01-01`, invisible to every default query and deleted by the first
+retention sweep.
+
+Thread errors out, count and report `Corrupt`/`Skipped`/`Rejected` in
+`QueryStats` and `partialSuccess`, add an explicit backward-compat branch to
+`DecodeChunk`, and never file a span under 1970.
+
+### 6. Nothing stops two processes opening the same data directory
+
+No lockfile anywhere — `grep -rn "Flock|flock|O_EXCL"` matches nothing.
+`BlobFile` tracks the append offset in *process memory* (`blob.go:16-22`) and
+assigns `off := b.size` at `blob.go:71`, while the file is opened `O_APPEND` so
+the kernel writes at the real end:
+
+```go
+off := b.size
+n, err := b.f.WriteString(s)
+return BlobRef{Off: off, Len: uint32(n)}, nil
+```
+
+Two processes (a stray unit, a manual `fathom serve`, an overlapping container
+restart) and process A records an offset where process B's bytes now live. The
+chunk is valid, the CRC is correct, the ref is in range — and reading it returns
+**another trace's text**. Silent content substitution, undetectable by any check
+in the tree.
+
+Take an exclusive lock on the data dir and fail startup with a clear message.
+Derive the blob offset from the actual write position rather than a
+process-local counter.
+
+### 7. Durability gaps: no directory fsync, and retention closes handles readers hold
+
+**No directory is ever fsynced** — only `blob.go:115` and `shard.go:190` sync
+files. A new day's shard can have its contents synced while the directory entry
+naming them is not, so a power cut leaves the day *absent* rather than corrupt,
+and `ListShards` simply does not enumerate it.
+
+Separately, `dropShard` (`store.go:419-432`) calls `sh.Close()` on a handle the
+shared cache handed out, which `store.go:337` documents as something readers
+must not close. Nothing refcounts. Probe output:
+
+```
+before evict: blob read = "THE-PROMPT-TEXT" err=<nil>
+after evict, SAME held handle: err=... file already closed
+post-evict scan: rows=0 (no error surfaced)
+```
+
+The hourly sweep firing while a trace-detail view is open makes the prompt
+render empty. In the other direction, a query that reopens the file after the
+map entry is deleted leaves a live handle to a deleted file and corrupts the
+size accounting.
+
+Add a `syncDir` helper on every create path; refcount shards with an explicit
+`Release()`, unlink immediately and defer `Close()` to zero readers.
+
+### 8. Bound query and decode memory
+
+`LIMIT` is applied only after the whole result is materialised (`exec.go:616`).
+One goroutine per shard, no cap on `a.rows`, one `*group` per distinct key in an
+unbounded map, and every sample retained for percentiles (`exec.go:563-566`).
+`SELECT p99(duration) FROM spans WHERE time >= 0 GROUP BY trace_id` passes the
+time-range check and can be several GB on a year of traces. `handleQuery` passes
+no context, so the 60s `WriteTimeout` kills the response, not the goroutines.
+
+`chunk.go:298-303` pre-sizes slices from an untrusted count capped at
+`maxRows*maxColumns` = 2^32 — roughly 36 GB of allocation from one frame before
+a single attribute byte is read. `recoverTail` has the same shape at
+`shard.go:141`.
+
+Add a `maxGroups` cap and a truncated flag, cap rows at `q.Limit` where the
+query allows it, use a bounded sketch for percentiles, semaphore the shard
+fan-out, thread a deadline context through, and never pre-size from an untrusted
+length beyond what the remaining buffer can hold.
+
+Related: `config.go:71` sets `MaxBytes = 0` and `store.go:398` only evicts when
+`MaxBytes > 0`, so size-based eviction is **off by default** — an unauthenticated
+writer can fill the disk with a 30-day window to work in.
+
+### 9. The insights findings have no null model and no precision measurement
+
+Not one threshold in `internal/insights/` has been validated against labelled
+data — `grep` for `precision|labeled|ground truth` returns one misleading
+comment. The free parameters (`MinRuns:3`, `MinLen:3`, `PromptSimilarity:0.55`,
+`embedSimilarity:0.82`, `RereadThreshold:3`, `MinCostUSD:0.50`) are tuned
+against nothing, and all 40 tests are fixtures asserting the mechanism fires.
+
+A faithful replication of `crystallize.go`'s miner on traces whose tool calls
+are drawn **i.i.d. with no sequential structure** produced ~300 qualifying
+findings at 150 traces, top one "the same 3 steps in 21 runs". The panel is
+guaranteed to be full whether or not the user has a single real habit.
+
+Specific defects behind that:
+- **Cost is not a saving.** `crystallize.go:102` allocates trace cost uniformly
+  across tool calls (false for agentic runs — cost scales with accumulated
+  context), and four detectors attribute the *entire* trace. The UI renders it as
+  "costing $X", which sends users to spend an hour recovering money that was
+  never on the table.
+- **No denominator.** `context_waste.go:258-272` `continue`s past successes
+  before counting, so a failure *rate* is unknowable and `go test` failing 3 of
+  500 reads identically to 3 of 3.
+- **Severity measures window length.** `insights.go:360-368` thresholds on
+  absolute cost/occurrences, so widening `WindowDays` promotes everything; the
+  bare `occurrences >= 20` disjunct promotes with zero cost input.
+- **`AddressableCost` is computed from the truncated evidence list** (capped at
+  8), so the metric that exists to prevent overclaiming undercounts more as
+  findings get stronger.
+- **Threshold mismatch.** `MinOccurrences` is advertised to the client as the
+  floor, but three detectors fire at n=2 and `Waste` counts calls, so one trace
+  can produce a finding.
+
+Add a within-trace permutation null with BH correction; split
+`cost_observed_usd` from `cost_recoverable_usd`; compute failure rates against a
+base rate; normalise severity to a rate; compute `AddressableCost` from full
+trace IDs.
+
+### 10. Make "working" distinguishable from "loaded but inert"
+
+No `/healthz`, no `/readyz` (grep returns only test hits), and `/api/stats`
+reports shard byte sizes only — no ingest counters, no last-ingest timestamp, no
+error counts. `store.Open` (`store.go:97-101`) only calls `MkdirAll`, which
+succeeds on an existing read-only directory, so fathom starts, logs `listening`,
+accepts every POST with 200, and every flush fails into the silent void of item
+4. Retention deletes days with no log at all. A failed insights analysis keeps
+serving the previous result with `ready: true` and no timestamp rendered.
+
+Add `/healthz` and `/readyz`, a startup write-probe that fails loudly, ingest
+and flush counters plus `last_ingest_at` / `last_flush_error` on `/api/stats`,
+eviction logging, and `stale: true` + `GeneratedAt` on the insights response.
+
+The pattern already exists in this codebase and is worth copying: `LocalStatus`
+(`analyzer.go:96-101`) distinguishes "a model is available" from "a model was
+actually used". That is exactly the loaded-vs-inert discrimination missing
+everywhere else.
+
+---
+
+## Notes
+
+**`go test -race ./...` passes clean on all 10 packages.** That is worth stating
+plainly because it is not evidence of durability: the suite never corrupts a
+byte, never fails a write, and never races retention against a live reader.
+Items 1, 4, 6 and 7 all live in that gap. Tests that inject those faults are
+prerequisites for trusting the fixes.
+
+**Two positives the panel checked and confirmed**, worth protecting: the web UI
+has no XSS — `app.js` contains zero `innerHTML`/`eval`, every node goes through
+`el()`'s `textContent`/`createTextNode`, and trace content is attacker-
+influenceable and rendered safely anyway. And there is no dependency surface at
+all: `go.mod` has zero requires. A future `innerHTML` in `app.js` would turn
+item 3 into full store theft.
+
+**One cross-project contradiction.** `context_waste.go:230` suggests splitting
+long runs in two — which forfeits the prompt-cache prefix that costslayer's L3
+exists to preserve. Acting on that finding can increase spend. Estimate the
+cache-rebuild cost before emitting the suggestion.
+
+**One documented guarantee the code does not keep.** `local.go:1-12` and
+`config.go:46-52` promise insights data "never leaves the machine", but
+`candidates()` puts `OLLAMA_HOST` and `FATHOM_LOCAL_MODEL_URL` *ahead* of the
+localhost defaults with no loopback check — and what travels is up to six user
+prompts, not a hash. Anyone with `OLLAMA_HOST` pointed at a GPU box silently
+ships real prompts over plaintext HTTP. Either enforce loopback or correct the
+comment. `GET /api/insights?from=&to=` also triggers on-demand full-store
+analysis despite the comment above it promising the opposite.