From cbe6d944f916c33382762c552a6644a49d212331 Mon Sep 17 00:00:00 2001 From: Zack Zatkin-Gold Date: Sun, 20 Sep 2026 13:34:15 -0700 Subject: [PATCH] Add TODO.md: ten findings from a six-expert review panel Six independent reviewers (empirical rigour, security, storage/durability, applied ML, reliability, DX) read the repo at main. The ten items here are the findings that survived cross-checking against the source. The two that change the most: recoverTail (internal/store/shard.go:107-155) scans frames from offset 0, breaks at the first CRC mismatch and unconditionally truncates, on the assumption that only the tail can be bad. A probe flipping one bit in chunk 0 of a 10-chunk file took it from 2130 bytes to 0. A read query against a not-yet-opened shard can trigger it. There is no auth primitive in the repository, and the default bind is :3000 on all interfaces, while the README instructs users to enable prompt, tool and response content logging. The data directory is a plaintext secrets store served to the network, with no redaction and day-granularity deletion. go test -race ./... passes clean on all 10 packages; the suite never corrupts a byte, fails a write, or races retention against a reader, which is where four of these ten live. Co-Authored-By: Claude Opus 5 --- TODO.md | 300 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 300 insertions(+) create mode 100644 TODO.md diff --git a/TODO.md b/TODO.md new file mode 100644 index 0000000..d94a0aa --- /dev/null +++ b/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= +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. -- 2.56.0