Bug-hunt round 33: unbounded total-size JSONL reads, two nitpicks

.abcd/work/DECISIONS.md

@@ -25,17 +25,6 @@ Architecture-shaping decisions graduate to an ADR under
 - 2026-07-17 — WhisperX VAD defaults to silero (`-vad` overrides): pyannote's
   checkpoint trips newer torch's `weights_only` load and aborts every run;
   found in the first live end-to-end session on the target Mac.
-- 2026-07-18 — Sanitise the finding `id` and verdict fields (`value`/`of`/`at`)
-  through `SafeText` when rendered to `report.md` and the review terminal: a
-  shared session's `findings.jsonl` is not revalidated by `analyze.Load`, so
-  those channels could still inject forged report structure / ANSI. Residual of
-  the earlier control-byte hardening, caught by a confirmation hunt.
-- 2026-07-18 — Third hardening pass (confirmation hunt): `review.describe`'s
-  verdict echo now `SafeText`s the id/verdict fields (the sibling of the fix
-  above, on the record path); `SafeText` also strips the Unicode BiDi/isolate
-  and line-separator controls (Trojan-Source, CVE-2021-42574); and `validate`
-  caps a finding's evidence at 64 ids, so a hostile answer cannot write a single
-  findings.jsonl line larger than the JSONL reader's buffer and brick the file.
 - 2026-07-17 — `record` uses ffmpeg avfoundation for both screen and microphone
   capture, not `screencapture -v`: ffmpeg is already a hard dependency (mic +
   transcribe), its SIGINT→finalise-container behaviour is battle-tested and
@@ -73,6 +62,17 @@ Architecture-shaping decisions graduate to an ADR under
   (confirmed, unverified, duplicate, rejected). Flagged divergences from the
   note: task-boundary chunking is deferred behind a seam (timeline carries no
   task markers), and keyframe extraction (AC3) is deferred to a later intent.
+- 2026-07-18 — Sanitise the finding `id` and verdict fields (`value`/`of`/`at`)
+  through `SafeText` when rendered to `report.md` and the review terminal: a
+  shared session's `findings.jsonl` is not revalidated by `analyze.Load`, so
+  those channels could still inject forged report structure / ANSI. Residual of
+  the earlier control-byte hardening, caught by a confirmation hunt.
+- 2026-07-18 — Third hardening pass (confirmation hunt): `review.describe`'s
+  verdict echo now `SafeText`s the id/verdict fields (the sibling of the fix
+  above, on the record path); `SafeText` also strips the Unicode BiDi/isolate
+  and line-separator controls (Trojan-Source, CVE-2021-42574); and `validate`
+  caps a finding's evidence at 64 ids, so a hostile answer cannot write a single
+  findings.jsonl line larger than the JSONL reader's buffer and brick the file.
 - 2026-07-18 — Security hardening (harden branch). Demo capture server: binds
   loopback by default (a bare `:port` normalises to `127.0.0.1:port`, opt into a
   wider bind with an explicit host); the write endpoints now require a loopback
@@ -850,3 +850,39 @@ Architecture-shaping decisions graduate to an ADR under
   new regression test. Both reviewers' verdicts were BLOCK, so per the
   loop's merge gate the PR (#48) stays open for the human rather than
   auto-merging, even with the fix pushed and CI green.
+- 2026-08-07 — Bug-hunt round 33: one confirmed defect (two readers), two
+  nitpicks. `session.ReadJSONL` capped a single line at `MaxJSONLLine` but
+  never the whole file, contradicting the comment on `maxManifestBytes` that
+  listed it as an already-bounded sibling reader; a file built from many
+  small, individually-legal lines defeated the per-line cap and OOM'd
+  `merge`, `report`, and `analyze`. New `session.MaxJSONLBytes` (16 MiB,
+  matching `analyze.Ingest`'s existing cap) bounds the running total in
+  `ReadJSONL`, with a matching `WriteJSONL` pre-flight check for its two
+  actual callers (transcript.jsonl, timeline.jsonl). Post-hoc adversarial
+  review of the round's own PR (correctness; docs accuracy) independently
+  caught that `analyze.ParseRecords` — findings.jsonl's own scanner, never
+  routed through `ReadJSONL` — carried the identical gap and was missed by
+  the round's own comment claiming every sibling reader was already bounded;
+  `ParseRecords` now enforces `MaxJSONLBytes` too, and the comments on
+  `maxManifestBytes`/`WriteJSONL` were corrected to name the readers and
+  writers accurately (`WriteJSONL` never writes findings.jsonl;
+  `analyze.commitFindings`/`review.AppendVerdict` do, through their own
+  locked descriptors). Nitpicks fixed: `AGENTS.md` claimed CI runs both the
+  plain and race-enabled `go test` lines, but CI only runs the race-enabled
+  one (no test differs between them); two 2026-07-18 `DECISIONS.md` entries
+  had been spliced ahead of a run of 2026-07-17 entries they were committed
+  after, breaking the file's own "newest last" rule by both date and commit
+  order, and were moved back. Reverted before merge: `report.eventLine`'s
+  `orDash` wrapper was initially removed as unreachable dead code, but round
+  28 (2026-08-05, above) had already considered and explicitly rejected this
+  exact claim as a deliberate locally-redundant guard against a future
+  caller invariant change — the same rationale `review.go`'s `checkTargets`
+  states for its own SafeText calls. An adversarial PR reviewer caught the
+  reintroduction before merge; the wrapper stays. Refuted: unbounded
+  error-accumulation in `analyze.Ingest`/`errors.Join` "defeating"
+  `maxAnswerBytes` (one refuter showed the same OOM reproduces from
+  `json.Unmarshal` alone before a single error accumulates, and a realistic
+  degenerate answer stays around 255 MB, well within bounds — split verdict,
+  discarded); a dangling-link nitpick in `AGENTS.md`'s abcd-managed fence
+  (split verdict on whether `.abcd/rules.json` gives an indirect fix path;
+  discarded as out of scope).

AGENTS.md

@@ -79,8 +79,9 @@ sh -n install.sh && bash -n install.sh  # installer syntax
 ```
 
 CI (`.github/workflows/ci.yml`) runs every gate above (the single-test example
-line is illustrative, not a gate) on every push and pull request, plus checks
-with no local command above: installer flag-handling
+line is illustrative, not a gate; the race-enabled test line supersedes the
+plain one, so CI runs only that one) on every push and pull request, plus
+checks with no local command above: installer flag-handling
 tests (`--help`/`--dir`/`--version`/`--bogus`), a compile-only cross-check for
 the other release platforms, a version-stamp ldflags check, a full-history
 `gitleaks` secret scan, and a `zizmor` workflow-security audit.

internal/analyze/analyze.go

@@ -124,9 +124,20 @@ func ParseRecords(r io.Reader, name string) ([]Finding, []Verdict, error) {
 	sc := bufio.NewScanner(r)
 	sc.Buffer(make([]byte, 0, 64*1024), session.MaxJSONLLine)
 	line := 0
+	var total int64
 	for sc.Scan() {
 		line++
 		raw := sc.Bytes()
+		// A per-line cap alone leaves the file's total size unbounded: a
+		// hand-edited or exchanged findings.jsonl built from many small,
+		// individually-legal lines would otherwise drive this loop's per-line
+		// allocation (findings and verdicts both accumulate into slices) well
+		// past the bytes on disk, mirroring the amplification session.ReadJSONL
+		// guards against for its own callers.
+		total += int64(len(raw)) + 1
+		if total > session.MaxJSONLBytes {
+			return nil, nil, fmt.Errorf("%s: exceeds %d bytes across %d lines; refusing to read", name, session.MaxJSONLBytes, line)
+		}
 		if len(bytes.TrimSpace(raw)) == 0 {
 			continue
 		}

internal/analyze/analyze_test.go

@@ -1,6 +1,7 @@
 package analyze
 
 import (
+	"bytes"
 	"errors"
 	"fmt"
 	"os"
@@ -372,6 +373,32 @@ func TestLoadRejectsNullLine(t *testing.T) {
 	}
 }
 
+// TestLoadRejectsOversizedTotal covers the file-size hole a per-line cap alone
+// leaves open: ParseRecords scans findings.jsonl (attacker-controllable once a
+// session is exchanged) with a bufio.Scanner capped at session.MaxJSONLLine
+// per line, but nothing previously bounded how many lines the file may hold.
+// A file built from many small, individually-legal lines used to buffer
+// without limit into the findings/verdicts slices, mirroring the
+// amplification session.ReadJSONL guards against for its own callers.
+// ParseRecords now caps the running total at session.MaxJSONLBytes, matching
+// ReadJSONL's stance.
+func TestLoadRejectsOversizedTotal(t *testing.T) {
+	dir := t.TempDir()
+	// {"kind":"verdict"}\n is 19 bytes; comfortably over MaxJSONLBytes in total.
+	line := []byte(`{"kind":"verdict"}` + "\n")
+	var buf bytes.Buffer
+	for buf.Len() <= session.MaxJSONLBytes {
+		buf.Write(line)
+	}
+	if err := os.WriteFile(filepath.Join(dir, session.FindingsFile), buf.Bytes(), 0o644); err != nil {
+		t.Fatalf("write findings: %v", err)
+	}
+	_, _, err := Load(dir)
+	if err == nil || !strings.Contains(err.Error(), "exceeds") {
+		t.Fatalf("expected an oversize-total refusal, got %v", err)
+	}
+}
+
 func TestIngestUnknownRubric(t *testing.T) {
 	dir := writeSession(t, timelineFixture)
 	_, err := Ingest(dir, strings.NewReader(`{"rubric":"testimony-analysis/v99","findings":[]}`))

internal/session/session.go

@@ -131,13 +131,16 @@ func (m Manifest) T0() (int64, error) {
 // maxManifestBytes caps LoadManifest's read of manifest.json. A genuine manifest
 // is a few hundred bytes; 1 MiB is generous for one carrying long notes or a big
 // task list. The cap matters because manifest.json in an exchanged session is
-// attacker-controllable (see Manifest.T0's threat note), and it is the last
-// untrusted read on the session surface that was still unbounded: an attacker
-// ships a multi-gigabyte manifest (a few KB once zipped) and any command that
-// loads it — merge, report, analyze, transcribe — would otherwise buffer the
-// whole file into memory before parsing and drive the process into OOM. Every
-// sibling reader of untrusted session files is already bounded (ReadJSONL caps a
-// line at MaxJSONLLine, analyze.Ingest at maxAnswerBytes, the demo body caps).
+// attacker-controllable (see Manifest.T0's threat note), and an attacker ships
+// a multi-gigabyte manifest (a few KB once zipped) that any command loading it
+// — merge, report, analyze, transcribe — would otherwise buffer into memory
+// before parsing and drive the process into OOM. Every sibling reader of an
+// untrusted session's JSONL files is bounded the same way: ReadJSONL caps both
+// a line (MaxJSONLLine) and the whole file (MaxJSONLBytes), and
+// analyze.ParseRecords — findings.jsonl's own scanner, not routed through
+// ReadJSONL — carries the same pair of caps. analyze.Ingest caps the untrusted
+// answer it validates at maxAnswerBytes, and the demo body caps what it
+// accepts at capture time; neither reads a session's own JSONL files back.
 const maxManifestBytes = 1 << 20 // 1 MiB
 
 // LoadManifest reads manifest.json from dir.
@@ -409,11 +412,26 @@ func SafeTextLines(s string) string {
 
 // MaxJSONLLine is the largest single JSONL record the readers accept. It is the
 // shared read-side invariant every writer must respect: a record persisted above
-// this size is durably unreadable, breaking merge, report, and analyze for the
+// this size is durably unreadable, breaking merge, report, or analyze for the
 // whole session, so the capture endpoints reject anything larger rather than
 // accept a line no reader can take back.
 const MaxJSONLLine = 4 << 20 // 4 MiB
 
+// MaxJSONLBytes caps a JSONL reader's total read across every line in a file,
+// the counterpart to MaxJSONLLine bounding a single one. A per-line cap alone
+// leaves total file size unbounded: a session's JSONL artefacts are
+// attacker-controllable when exchanged (see ReadJSONL's no-follow comment),
+// and a file built from many small, well-formed lines defeats MaxJSONLLine
+// while still driving json.Unmarshal's per-line allocation into hundreds of
+// megabytes for a file only tens of megabytes on disk. Both ReadJSONL and
+// analyze.ParseRecords (findings.jsonl's own scanner) enforce it. 16 MiB
+// matches the cap analyze.Ingest already applies to untrusted input at the
+// same scale; a genuine session's timeline, interactions, transcript, or
+// findings file is a small fraction of that. events.rrweb.jsonl is archival
+// only — no command reads it back through either scanner, so this cap does
+// not bound it.
+const MaxJSONLBytes = 16 << 20 // 16 MiB
+
 // jsonlEncoder returns a json.Encoder configured exactly as WriteJSONL's own
 // encoders are, so a size measured against it predicts what WriteJSONL will
 // later check and write. HTML escaping is disabled: JSONL artefacts are never
@@ -455,12 +473,22 @@ func ReadJSONL[T any](path string) ([]T, error) {
 	defer f.Close()
 
 	var out []T
+	var total int64
 	sc := bufio.NewScanner(f)
 	sc.Buffer(make([]byte, 0, 64*1024), MaxJSONLLine)
 	line := 0
 	for sc.Scan() {
 		line++
 		raw := sc.Bytes()
+		// Counted before the blank-line skip so a file padded with blank lines
+		// past the cap is refused rather than scanned past forever. bufio.ScanLines
+		// strips a trailing \r along with the \n, so a CRLF file is undercounted by
+		// one byte per line against what is actually on disk — benign here, since
+		// the extra byte is never itself decoded or retained.
+		total += int64(len(raw)) + 1
+		if total > MaxJSONLBytes {
+			return nil, fmt.Errorf("%s: exceeds %d bytes across %d lines; refusing to read", path, MaxJSONLBytes, line)
+		}
 		// Skip blank lines, including whitespace-only ones (as may appear in a
 		// hand-edited or exchanged session), matching analyze.Load so the two
 		// JSONL readers agree on what counts as blank.
@@ -484,21 +512,28 @@ func ReadJSONL[T any](path string) ([]T, error) {
 // from an untrusted, downloaded session directory — cannot be redirected to an
 // arbitrary file outside the session.
 //
-// It also holds the writers to MaxJSONLLine, the read-side invariant: without
-// the check merge could persist a timeline.jsonl (or analyze a findings.jsonl)
-// carrying a record longer than ReadJSONL can scan back, report success, and
-// leave the operator with an artefact its own reader — and every later merge,
-// report, and analyze run over that session — refuses whole. The whole set is
-// measured before the file is opened, so a refusal neither truncates an
-// existing artefact nor leaves a prefix of the new one behind, matching the
-// all-or-nothing stance of analyze.Ingest and demo.appendRecords. That costs a
-// second encoding pass over records that are small structs; a durably
-// unreadable session is the worse trade.
+// It also holds its two callers — transcribe (transcript.jsonl) and merge
+// (timeline.jsonl) — to the read-side invariants ReadJSONL enforces: without
+// the checks below, either could persist a record longer than MaxJSONLLine, or
+// a set totalling more than MaxJSONLBytes, that ReadJSONL can never scan back,
+// report success, and leave the operator with an artefact its own reader — and
+// every later merge, report, and analyze run over that session — refuses
+// whole. The whole set is measured before the file is opened, so a refusal
+// neither truncates an existing artefact nor leaves a prefix of the new one
+// behind, matching the all-or-nothing stance of analyze.Ingest and
+// demo.appendRecords. That costs a second encoding pass over records that are
+// small structs; a durably unreadable session is the worse trade.
+//
+// findings.jsonl never passes through here: analyze.commitFindings and
+// review.AppendVerdict write it through their own locked descriptors, so
+// ParseRecords (its read side) enforces MaxJSONLLine/MaxJSONLBytes without a
+// matching write-side pre-flight check.
 func WriteJSONL[T any](path string, values []T) error {
 	// Encode into one reusable buffer so the pre-flight pass holds a single
 	// record, not the whole file, in memory.
 	var buf bytes.Buffer
 	check := jsonlEncoder(&buf)
+	var total int64
 	for i, v := range values {
 		buf.Reset()
 		if err := check.Encode(v); err != nil {
@@ -521,6 +556,14 @@ func WriteJSONL[T any](path string, values []T) error {
 			// no line of any file, and no line of the source transcript either.
 			return fmt.Errorf("%s: line %d of the output encodes to %d bytes, over the %d-byte JSONL line limit", path, i+1, buf.Len(), MaxJSONLLine)
 		}
+		total += int64(buf.Len())
+		if total > MaxJSONLBytes {
+			// Same write-before-read stance as MaxJSONLLine's check above and
+			// SaveManifest's own cap: refuse before opening the file rather than
+			// persist a timeline.jsonl or transcript.jsonl that ReadJSONL's matching
+			// total-size cap would then refuse to read back.
+			return fmt.Errorf("%s: output would be %d bytes across %d lines, over the %d-byte JSONL file limit ReadJSONL enforces; refusing to write a session no command could read back", path, total, i+1, MaxJSONLBytes)
+		}
 	}
 
 	f, err := OpenFileNoFollow(path, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0o644)