diff --git a/.abcd/development/brief/04-surfaces/01-demo.md b/.abcd/development/brief/04-surfaces/01-demo.md index 9149181..7f17002 100644 --- a/.abcd/development/brief/04-surfaces/01-demo.md +++ b/.abcd/development/brief/04-surfaces/01-demo.md @@ -25,7 +25,9 @@ app contains at least one intentional usability flaw, found by talking. array of raw rrweb events, one per line, to `events.rrweb.jsonl`. Request bodies are validated as JSON, re-encoded to a single line so an embedded newline cannot split one record across lines, and size-limited (8 MiB); - writes are serialised under a mutex. + writes are serialised under a mutex. A persisted batch answers `204 No + Content`; if an append fails the endpoint answers `500` rather than a false + `204`, so the client does not treat a dropped event as captured. - The capture surface is loopback-only by default and the write endpoints are guarded against cross-origin forgery: a request must carry a loopback `Host`, a same-origin (or absent) `Origin`, and `Content-Type: application/json`. diff --git a/.abcd/development/brief/04-surfaces/03-merge.md b/.abcd/development/brief/04-surfaces/03-merge.md index 46ab5a4..c83af16 100644 --- a/.abcd/development/brief/04-surfaces/03-merge.md +++ b/.abcd/development/brief/04-surfaces/03-merge.md @@ -13,8 +13,12 @@ alongside the manifest. ## Behaviour -- Reads `manifest.json`, `transcript.jsonl`, and `interactions.jsonl`; a - missing file is an error. +- Reads `manifest.json` (required), `transcript.jsonl`, and + `interactions.jsonl`. A missing `transcript.jsonl` or `interactions.jsonl` is + treated as zero records, not an error, so a default audio-only `record` + session (which never writes `interactions.jsonl`) still merges to a + speech-only timeline, and a session merged before transcription still merges + to an event-only one. A malformed line in either file is still an error. - Utterances enter as `src: "speech"` entries at their `t0` (already session-relative); interactions enter as `src: "event"` entries with epoch milliseconds converted via `t0_epoch_ms`, and are assigned sequential diff --git a/.abcd/development/brief/05-internals/02-schemas.md b/.abcd/development/brief/05-internals/02-schemas.md index 4ed4a73..e743a27 100644 --- a/.abcd/development/brief/05-internals/02-schemas.md +++ b/.abcd/development/brief/05-internals/02-schemas.md @@ -120,5 +120,9 @@ validation boundary; every field below is checked, and `status` is forced to ``` Effective status: every finding starts `unverified`; verdict records apply in -file order and the last one for a finding wins. Design and rationale: +file order and the last one for a finding wins. A verdict whose `verdict` value +is outside the closed enum (a typo, an empty string, or a foreign value in a +shared session) is ignored, so its finding keeps `unverified` and still appears +in the report and review queue rather than vanishing into an unrendered group. +Design and rationale: [`../01-product/04-analysis.md`](../01-product/04-analysis.md). diff --git a/.abcd/work/DECISIONS.md b/.abcd/work/DECISIONS.md index 0f9885e..7e32908 100644 --- a/.abcd/work/DECISIONS.md +++ b/.abcd/work/DECISIONS.md @@ -128,3 +128,22 @@ Architecture-shaping decisions graduate to an ADR under alone and prints that installing `gh` enables provenance verification. The dependency section (ffmpeg pinned-GPG path, whisperx/whisper.cpp, private-mktemp uv install) is unchanged. +- 2026-07-18 — bughunt-1 correctness fixes. `timeline.Merge` now treats a + missing `transcript.jsonl`/`interactions.jsonl` as zero records (via + `readOptionalJSONL`, tolerating `fs.ErrNotExist`), so the documented default + audio-only `record` → `merge` pipeline no longer aborts with "no such file"; + brief 04-surfaces/03-merge.md and docs/reference/cli.md updated. +- 2026-07-18 — Demo capture handler (`appendLines`) now checks the append + write error and answers `500` instead of a false `204`, and writes each + record + newline as one buffer so a partial write cannot leave a truncated, + unparseable JSONL line; brief 04-surfaces/01-demo.md updated. +- 2026-07-18 — `analyze.Load` validates the verdict enum (the previously-unused + `verdictSet`): a verdict value outside `confirmed|rejected|duplicate` is + ignored, so its finding stays `unverified` and no longer vanishes from the + report and review queue into an unrendered status group; schema doc noted. +- 2026-07-18 — `session.WriteJSONL` and `review.AppendVerdict` now return the + file `Close()` error (matching `WriteFileNoFollow`), so a write-back failure + deferred to close is not masked as success on committed artefacts. +- 2026-07-18 — Demo/record banners derive the display URL via `demo.DisplayURL` + instead of concatenating `-addr` after a literal "localhost", fixing the + broken `http://localhost0.0.0.0:8737` shown for an explicit-host bind. diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 45a37c6..054db70 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -72,7 +72,7 @@ testimony merge -session DIR |---|---|---| | `-session` | *(required)* | session directory | -Behaviour: reads `manifest.json`, `transcript.jsonl`, and `interactions.jsonl`; converts interaction epoch-millisecond times to session-relative seconds via `t0_epoch_ms`; writes the time-sorted `timeline.jsonl`; prints `merged N utterances + M events → `. +Behaviour: reads `manifest.json` (required), `transcript.jsonl`, and `interactions.jsonl`; converts interaction epoch-millisecond times to session-relative seconds via `t0_epoch_ms`; writes the time-sorted `timeline.jsonl`; prints `merged N utterances + M events → `. A missing `transcript.jsonl` or `interactions.jsonl` counts as zero records rather than an error, so a default audio-only `record` session (which never writes `interactions.jsonl`) still merges to a speech-only timeline. ## `testimony report` diff --git a/internal/analyze/analyze.go b/internal/analyze/analyze.go index 45e0e5a..85429df 100644 --- a/internal/analyze/analyze.go +++ b/internal/analyze/analyze.go @@ -112,6 +112,16 @@ func Load(dir string) ([]Finding, []Verdict, error) { if err := json.Unmarshal(raw, &v); err != nil { return nil, nil, fmt.Errorf("%s:%d: %w", path, line, err) } + // The verdict enum is closed (confirmed|rejected|duplicate). A verdict + // carrying any other value — a typo, an empty string, or a foreign + // value from a shared/hand-edited session — is not representable, so it + // is ignored rather than applied. The finding then keeps its + // "unverified" status and still appears in the report and the review + // queue, instead of landing in a status group nothing renders and + // silently vanishing from both. + if !verdictSet[v.Verdict] { + continue + } verdicts = append(verdicts, v) continue } diff --git a/internal/demo/demo.go b/internal/demo/demo.go index 02ce647..209f600 100644 --- a/internal/demo/demo.go +++ b/internal/demo/demo.go @@ -60,7 +60,7 @@ func Run(addr, outRoot string) error { fmt.Printf(`testimony demo — capture session started session dir : %s - url : http://localhost%s + url : %s 1. Start your voice/screen recorder NOW (QuickTime → File → New Audio Recording). 2. Say “session start” aloud, open the URL, and think aloud while you explore. @@ -70,7 +70,7 @@ func Run(addr, outRoot string) error { testimony merge -session %s testimony report -session %s -`, dir, addr, dir, dir, dir) +`, dir, DisplayURL(addr), dir, dir, dir) // Block until interrupted, then shut the server down cleanly. ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) @@ -175,8 +175,17 @@ func (s *server) appendLines(w http.ResponseWriter, r *http.Request, f *os.File, s.mu.Lock() defer s.mu.Unlock() for _, l := range lines { - f.Write(l) - f.Write([]byte("\n")) + // Write the record and its terminating newline as a single buffer: a + // partial write (e.g. ENOSPC) can then never land a data line without + // its newline, which would join to the next successful write and produce + // one malformed physical line that later fails merge's JSONL reader. + if _, err := f.Write(append(l, '\n')); err != nil { + // The capture was not persisted; tell the client so it does not treat + // a dropped event as recorded, rather than answering 204 as if it had + // succeeded. + http.Error(w, "capture write failed", http.StatusInternalServerError) + return + } } w.WriteHeader(http.StatusNoContent) } @@ -249,6 +258,24 @@ func isJSONContentType(ct string) bool { // are not published to the LAN even though the banner prints "localhost". An // operator who deliberately wants a wider bind can still pass an explicit host // (e.g. "0.0.0.0:8737"). +// DisplayURL renders the human-facing URL an operator opens for a capture +// server bound to addr. It shows "localhost" only for the host-less default +// (":8737" -> http://localhost:8737); when an operator passes an explicit host +// for a wider bind (e.g. "0.0.0.0:8737") it shows that host, so the printed URL +// is never the broken "http://localhost0.0.0.0:8737" that raw concatenation of +// addr after the "localhost" literal produced. Shared by demo.Run and +// record.printStatus. +func DisplayURL(addr string) string { + host, port, err := net.SplitHostPort(addr) + if err != nil { + return "http://localhost" + addr // malformed addr: preserve the old form + } + if host == "" { + host = "localhost" + } + return "http://" + net.JoinHostPort(host, port) +} + func listenAddr(addr string) string { host, port, err := net.SplitHostPort(addr) if err != nil { diff --git a/internal/demo/demo_test.go b/internal/demo/demo_test.go index 18c9105..7d92e73 100644 --- a/internal/demo/demo_test.go +++ b/internal/demo/demo_test.go @@ -125,6 +125,44 @@ func TestBatchCompactsEmbeddedNewline(t *testing.T) { } } +// TestAppendLinesReportsWriteError is the dropped-write regression: when the +// append to a stream file fails, the handler must not answer 204 (which tells the +// browser the capture was persisted and stops it re-sending). Here the stream +// file is closed underneath the server so its Write fails; the handler must +// surface an error status instead of a silent 204. +func TestAppendLinesReportsWriteError(t *testing.T) { + s, _ := newTestServer(t) + if err := s.interactions.Close(); err != nil { + t.Fatalf("close: %v", err) + } + + w := httptest.NewRecorder() + s.handleInteraction(w, jsonPost("/api/interactions", "{\"t\":1,\"kind\":\"click\"}", nil)) + if w.Code == http.StatusNoContent { + t.Fatalf("a failed capture write returned 204; want an error status") + } + if w.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500", w.Code) + } +} + +// TestDisplayURL pins the human-facing URL: "localhost" only for the host-less +// default, and the real host otherwise, never the broken "localhost0.0.0.0:8737" +// that concatenating a full -addr after the "localhost" literal produced. +func TestDisplayURL(t *testing.T) { + cases := map[string]string{ + ":8737": "http://localhost:8737", + "0.0.0.0:8737": "http://0.0.0.0:8737", + "127.0.0.1:8737": "http://127.0.0.1:8737", + "[::1]:8737": "http://[::1]:8737", + } + for in, want := range cases { + if got := DisplayURL(in); got != want { + t.Errorf("DisplayURL(%q) = %q, want %q", in, got, want) + } + } +} + // TestWriteEndpointGuard covers the CSRF / DNS-rebinding surface: a request must // carry a JSON Content-Type, a loopback Host, and (when present) a same-origin // Origin. A rejected request must write nothing. diff --git a/internal/record/record.go b/internal/record/record.go index 16e26d2..3880870 100644 --- a/internal/record/record.go +++ b/internal/record/record.go @@ -285,7 +285,7 @@ func printStatus(log io.Writer, recorders []string, demoOn bool, addr string) { fmt.Fprintf(log, " recording : %s\n", strings.Join(recorders, ", ")) } if demoOn { - fmt.Fprintf(log, " demo url : http://localhost%s\n", addr) + fmt.Fprintf(log, " demo url : %s\n", demo.DisplayURL(addr)) } fmt.Fprint(log, "\n Say “session start” aloud, then think aloud while you work.\n") fmt.Fprint(log, " Press Ctrl+C to stop.\n") diff --git a/internal/report/report_test.go b/internal/report/report_test.go index 3a438e4..27437a2 100644 --- a/internal/report/report_test.go +++ b/internal/report/report_test.go @@ -88,6 +88,37 @@ func TestReportNoFindings(t *testing.T) { } } +// TestReportKeepsFindingWithUnknownVerdict is the vanishing-finding regression. +// findings.jsonl is a shared/hand-editable artefact; a verdict line carrying a +// value outside the closed enum (here a "confirm" typo) must not push its finding +// into an unrendered status group. The finding must still appear, falling back to +// Unverified, rather than disappearing from the report entirely. +func TestReportKeepsFindingWithUnknownVerdict(t *testing.T) { + dir := t.TempDir() + if err := session.SaveManifest(dir, session.Manifest{Session: "fixture", App: "app", Participant: "P1"}); err != nil { + t.Fatalf("SaveManifest: %v", err) + } + if err := os.WriteFile(filepath.Join(dir, session.TimelineFile), []byte(timelineFixture), 0o644); err != nil { + t.Fatalf("write timeline: %v", err) + } + findings := "{\"id\":\"F-001\",\"t\":22,\"type\":\"bug\",\"severity\":3,\"quote\":\"ok\",\"evidence\":[\"utt-004\"],\"status\":\"unverified\"}\n" + + "{\"kind\":\"verdict\",\"finding\":\"F-001\",\"verdict\":\"confirm\",\"at\":\"2026-07-17\"}\n" + if err := os.WriteFile(filepath.Join(dir, session.FindingsFile), []byte(findings), 0o644); err != nil { + t.Fatalf("write findings: %v", err) + } + + md, err := Render(dir, 2.5) + if err != nil { + t.Fatalf("Render: %v", err) + } + if !strings.Contains(md, "**F-001**") { + t.Fatalf("finding with an unrecognised verdict vanished from the report:\n%s", md) + } + if !strings.Contains(md, "### Unverified (1)") { + t.Fatalf("finding with an unrecognised verdict should fall back to Unverified:\n%s", md) + } +} + func findingLines(t *testing.T, dir string) []string { t.Helper() b, err := os.ReadFile(filepath.Join(dir, session.FindingsFile)) diff --git a/internal/review/review.go b/internal/review/review.go index 0893414..267385c 100644 --- a/internal/review/review.go +++ b/internal/review/review.go @@ -210,11 +210,13 @@ func AppendVerdict(dir string, v analyze.Verdict) error { if err != nil { return err } - defer f.Close() if _, err := f.Write(append(b, '\n')); err != nil { + f.Close() return err } - return nil + // Return the Close error so a verdict is never reported recorded when its + // bytes did not reach disk (write-back deferred to close on NFS/full device). + return f.Close() } // printFinding writes a finding to the analyst's terminal. Every diff --git a/internal/session/session.go b/internal/session/session.go index 8260756..4b5a9ed 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -197,14 +197,22 @@ func WriteJSONL[T any](path string, values []T) error { if err != nil { return err } - defer f.Close() w := bufio.NewWriter(f) enc := json.NewEncoder(w) for _, v := range values { if err := enc.Encode(v); err != nil { + f.Close() return err } } - return w.Flush() + if err := w.Flush(); err != nil { + f.Close() + return err + } + // Return the Close error: on a filesystem that defers write-back errors to + // close (NFS close-to-open, or a full device), the final failure surfaces + // here, not from Flush — mirroring WriteFileNoFollow, so a committed artefact + // is never reported written when its bytes did not reach disk. + return f.Close() } diff --git a/internal/timeline/timeline.go b/internal/timeline/timeline.go index 327967a..2ac43f1 100644 --- a/internal/timeline/timeline.go +++ b/internal/timeline/timeline.go @@ -4,7 +4,9 @@ package timeline import ( + "errors" "fmt" + "io/fs" "path/filepath" "sort" @@ -123,6 +125,21 @@ func EventsNear(entries []Entry, u Entry, window float64) []string { return ids } +// readOptionalJSONL reads a JSONL input that a valid session may legitimately +// lack. A default audio-only `testimony record` run captures no interactions, +// and a session may equally reach merge before transcription: an absent file is +// therefore zero records, not a fatal error, so an audio-only or interaction-only +// session still merges to a partial timeline instead of aborting the documented +// pipeline. Any other read error (a malformed line, a permission failure) is +// still returned unchanged. +func readOptionalJSONL[T any](path string) ([]T, error) { + out, err := session.ReadJSONL[T](path) + if errors.Is(err, fs.ErrNotExist) { + return nil, nil + } + return out, err +} + // Merge reads manifest, transcript and interactions from dir, writes // timeline.jsonl, and returns the number of speech and event entries. func Merge(dir string) (speech, events int, err error) { @@ -130,11 +147,11 @@ func Merge(dir string) (speech, events int, err error) { if err != nil { return 0, 0, err } - utts, err := session.ReadJSONL[Utterance](filepath.Join(dir, session.TranscriptFile)) + utts, err := readOptionalJSONL[Utterance](filepath.Join(dir, session.TranscriptFile)) if err != nil { return 0, 0, fmt.Errorf("read transcript: %w", err) } - ints, err := session.ReadJSONL[Interaction](filepath.Join(dir, session.InteractionsFile)) + ints, err := readOptionalJSONL[Interaction](filepath.Join(dir, session.InteractionsFile)) if err != nil { return 0, 0, fmt.Errorf("read interactions: %w", err) } diff --git a/internal/timeline/timeline_test.go b/internal/timeline/timeline_test.go index 3ea64c6..80e2d65 100644 --- a/internal/timeline/timeline_test.go +++ b/internal/timeline/timeline_test.go @@ -1,6 +1,12 @@ package timeline -import "testing" +import ( + "os" + "path/filepath" + "testing" + + "github.com/REPPL/Testimony/internal/session" +) const t0 = int64(1_784_300_400_000) // arbitrary session anchor, epoch ms @@ -41,6 +47,33 @@ func TestBuildEntriesSortsByTime(t *testing.T) { } } +// TestMergeAudioOnlySession is the audio-only-record regression: a default +// `testimony record` session has manifest + transcript but no interactions.jsonl +// (only the demo server writes that file), and record itself prints `merge` as +// the next step. Merge must treat the absent interactions file as zero events and +// still write a speech-only timeline, not abort with a "no such file" error. +func TestMergeAudioOnlySession(t *testing.T) { + dir := t.TempDir() + if err := session.SaveManifest(dir, session.Manifest{Session: "s", T0EpochMS: t0}); err != nil { + t.Fatalf("SaveManifest: %v", err) + } + utts := []Utterance{{ID: "utt-001", T0: 1, T1: 2, Speaker: "P1", Text: "hello"}} + if err := session.WriteJSONL(filepath.Join(dir, session.TranscriptFile), utts); err != nil { + t.Fatalf("write transcript: %v", err) + } + + speech, events, err := Merge(dir) + if err != nil { + t.Fatalf("Merge on audio-only session: %v", err) + } + if speech != 1 || events != 0 { + t.Fatalf("want speech=1 events=0, got speech=%d events=%d", speech, events) + } + if _, err := os.Stat(filepath.Join(dir, session.TimelineFile)); err != nil { + t.Fatalf("timeline.jsonl not written: %v", err) + } +} + func TestEventsNearWindow(t *testing.T) { entries := sample() var speech []Entry