TODO.md: ten findings from a six-expert review panel #4

2026-09-20T13:35:00-07:00

zzg commented

2026-09-20 13:35:00 -07:00

Six independent Opus reviewers read this repo and costslayer at main — empirical rigour, security, storage/durability, applied ML, reliability, and developer experience. TODO.md is the synthesis: ten items, ordered by irreversibility — silent data loss and credential exposure first.

Claims were re-verified against the source rather than taken on a reviewer's word. Three are worth reading before the rest.

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 and unconditionally truncates. The comment assumes only the tail can be bad; nothing enforces it. A probe on a scratch copy flipped one bit in chunk 0 of a 10-chunk file:

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)

store.go:255-259 means a read query on a not-yet-opened shard can trigger it, so browsing an old day in the UI is enough.

There is no auth primitive in the repository. grep -rn 'Bearer|Authorization' over internal/ and cmd/ returns nothing, and config.go:66 binds :3000 on all interfaces. The README instructs users to enable OTEL_LOG_USER_PROMPTS, OTEL_LOG_TOOL_CONTENT and OTEL_LOG_ASSISTANT_RESPONSES, so the data directory holds source, shell output and any credential that passed through a session — unredacted, 0o644, served to anyone who can reach the port. A DNS-rebinding page needs no network adjacency at all.

Silent loss after acknowledgement. store.go:165-168 detaches the buffer before attempting I/O and never re-buffers on failure, after the exporter was already told 200 OK; flushErr is then handed to the next unrelated caller and cleared. The store package imports no logger, so none of it is recorded.

Also covered: no lockfile plus a process-memory blob offset under O_APPEND, which can return another trace's text from a CRC-valid chunk; no directory fsync; retention closing shard handles live readers hold; unbounded query and decode allocation with MaxBytes = 0 by default; and the insights package, where a faithful replication of the sequence miner produced ~300 qualifying findings from traces with no sequential structure at all.

Two positives are recorded so they don't get regressed: the UI has no XSS (app.js routes everything through textContent/createTextNode), and go.mod has zero requires. Also noted — go test -race ./... passes clean on all 10 packages, which is precisely why four of these ten are invisible today.

Nothing here is applied; this is a reading list, not a patch.

🤖 Generated with Claude Code

Six independent Opus reviewers read this repo and `costslayer` at `main` — empirical rigour, security, storage/durability, applied ML, reliability, and developer experience. `TODO.md` is the synthesis: ten items, ordered by irreversibility — silent data loss and credential exposure first. Claims were re-verified against the source rather than taken on a reviewer's word. Three are worth reading before the rest. **`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 and unconditionally truncates. The comment assumes only the tail can be bad; nothing enforces it. A probe on a scratch copy flipped one bit in chunk 0 of a 10-chunk file: ``` 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) ``` `store.go:255-259` means a *read* query on a not-yet-opened shard can trigger it, so browsing an old day in the UI is enough. **There is no auth primitive in the repository.** `grep -rn 'Bearer|Authorization'` over `internal/` and `cmd/` returns nothing, and `config.go:66` binds `:3000` on all interfaces. The README instructs users to enable `OTEL_LOG_USER_PROMPTS`, `OTEL_LOG_TOOL_CONTENT` and `OTEL_LOG_ASSISTANT_RESPONSES`, so the data directory holds source, shell output and any credential that passed through a session — unredacted, `0o644`, served to anyone who can reach the port. A DNS-rebinding page needs no network adjacency at all. **Silent loss after acknowledgement.** `store.go:165-168` detaches the buffer before attempting I/O and never re-buffers on failure, after the exporter was already told 200 OK; `flushErr` is then handed to the next unrelated caller and cleared. The store package imports no logger, so none of it is recorded. Also covered: no lockfile plus a process-memory blob offset under `O_APPEND`, which can return another trace's text from a CRC-valid chunk; no directory fsync; retention closing shard handles live readers hold; unbounded query and decode allocation with `MaxBytes = 0` by default; and the insights package, where a faithful replication of the sequence miner produced ~300 qualifying findings from traces with no sequential structure at all. Two positives are recorded so they don't get regressed: the UI has no XSS (`app.js` routes everything through `textContent`/`createTextNode`), and `go.mod` has zero requires. Also noted — `go test -race ./...` passes clean on all 10 packages, which is precisely why four of these ten are invisible today. Nothing here is applied; this is a reading list, not a patch. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

zzg added 1 commit 2026-09-20 13:35:00 -07:00

Add TODO.md: ten findings from a six-expert review panel cbe6d944f9

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 <noreply@anthropic.com>

You are not authorized to merge this pull request.

This pull request can be merged automatically.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin todo/expert-panel-findings:todo/expert-panel-findings

git checkout todo/expert-panel-findings

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: zzg/fathom#4