From c3ada2e7f8728c0c243b3a7b78c0d2f34b84b55f Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Wed, 29 Jul 2026 07:32:14 +0100 Subject: [PATCH 01/20] fix: resolve the audio offset before the conversion mutates the session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `transcribe -audio ext.m4a` converted the external recording over the session's audio.wav before resolving the audio->session offset, so every way resolution can fail — a manifest with no usable t0_epoch_ms, a derived offset past the plausibility bound — exited 1 with the record-origin capture already destroyed and no audio.offset.json written. The session then matched the documented record-origin shape (no sidecar => audio.wav captured at t0), so a later bare `transcribe` took the in-place branch, printed the false provenance "captured at t0", and wrote a silently time-shifted transcript at exit 0. Resolution reads only the manifest and the external recording, so it now runs first and a refused run leaves the session byte-for-byte as it found it. Resolution probes the recording with ffprobe, though, so the refusals that make -audio safe to open cannot stay inside convertAudio: ffprobe opens without O_NONBLOCK, and a FIFO at -audio would block its open(2) for ever and hang the command that used to refuse it in milliseconds. The accepted- container and regular-file checks move into checkExternalAudio, called before the offset is resolved and again by convertAudio, so the guard keeps one home and nothing opens the recording before it is proven a regular file of a container the pipeline accepts. An explicit -offset also failed to rewrite an EXISTING sidecar, so an operator's correction applied to one transcript and the next bare re-run resurrected the stale value. It now rewrites the sidecar when one is already there; a record-origin session with no sidecar stays sidecar-free, since inventing one would make every later bare run inherit a one-off flag. Assisted-by: Claude:claude-opus-5 --- internal/transcribe/ffmpeg.go | 30 ++- internal/transcribe/transcribe.go | 54 +++++- internal/transcribe/transcribe_test.go | 245 +++++++++++++++++++++++++ 3 files changed, 320 insertions(+), 9 deletions(-) diff --git a/internal/transcribe/ffmpeg.go b/internal/transcribe/ffmpeg.go index 1ad3064..8760baa 100644 --- a/internal/transcribe/ffmpeg.go +++ b/internal/transcribe/ffmpeg.go @@ -18,22 +18,38 @@ import ( // and plain WAV. Everything is normalised through ffmpeg regardless. var audioExts = map[string]bool{".m4a": true, ".mov": true, ".wav": true} -// convertAudio produces the canonical ASR input — 16 kHz mono PCM WAV — from -// the original recording via an ffmpeg subprocess. -func convertAudio(in, out string) error { +// checkExternalAudio validates the operator-named recording before anything +// opens it. It lives apart from convertAudio because the conversion is no longer +// the first thing to touch the path: transcribe.Run resolves the audio→session +// offset first (so a refused run cannot destroy the session's audio.wav), and +// that derivation hands the same path to an ffprobe subprocess. Both subprocesses +// open without O_NONBLOCK, so a FIFO at -audio blocks their open(2) for ever and +// hangs the command; and ffprobe is a media parser, so pointing it at a file +// whose container the pipeline does not even accept widens its exposure for +// nothing. Run calls this on the external branch and convertAudio calls it again, +// so the guard keeps one home and neither entry point is unguarded. +// +// os.Stat resolves a symlink, so an operator pointing -audio at a symlinked +// recording is fine; what must be refused is a non-regular target. +func checkExternalAudio(in string) error { ext := strings.ToLower(filepath.Ext(in)) if !audioExts[ext] { return fmt.Errorf("unsupported audio format %q: expected .m4a, .mov, or .wav", ext) } - // os.Stat resolves a symlink, so an operator pointing -audio at a symlinked - // recording is fine; what must be refused is a non-regular target, because a - // FIFO handed to ffmpeg as its input blocks the subprocess's open(2) for ever, - // waiting for a writer that never arrives. if fi, err := os.Stat(in); err != nil { return fmt.Errorf("audio file: %w", err) } else if !fi.Mode().IsRegular() { return fmt.Errorf("refusing to read %s: it is not a regular file", in) } + return nil +} + +// convertAudio produces the canonical ASR input — 16 kHz mono PCM WAV — from +// the original recording via an ffmpeg subprocess. +func convertAudio(in, out string) error { + if err := checkExternalAudio(in); err != nil { + return err + } if err := checkPlainOutput(out); err != nil { return err } diff --git a/internal/transcribe/transcribe.go b/internal/transcribe/transcribe.go index b0735f3..5447f0c 100644 --- a/internal/transcribe/transcribe.go +++ b/internal/transcribe/transcribe.go @@ -73,24 +73,56 @@ func Run(opts Options) (int, error) { wav := filepath.Join(opts.SessionDir, session.AudioFile) external := opts.Audio != "" && !sameFile(opts.Audio, wav) if external { - if err := convertAudio(opts.Audio, wav); err != nil { + // Refuse an unusable -audio before anything opens it. The offset derivation + // below probes this path with ffprobe, whose open(2) on a FIFO never returns, + // so this guard has to fire ahead of it rather than waiting for convertAudio. + if err := checkExternalAudio(opts.Audio); err != nil { return 0, err } } else if err := checkSessionAudio(wav, opts.SessionDir); err != nil { return 0, err } + // Resolve the offset BEFORE the conversion replaces audio.wav. Resolution + // reads only the manifest and the external recording, so it can run first — + // and it must, because every way it fails (an unusable manifest t0, an + // implausible derived offset) refuses the run. Converting first destroyed the + // session's record-origin capture and left no sidecar behind, so the session + // became indistinguishable from one recorded here (no sidecar ⇒ captured at + // t0, docs/reference/session-directory.md): the next bare `transcribe` took + // the in-place branch, reported "captured at t0", and wrote a silently + // time-shifted transcript at exit 0. A refused run now leaves the session + // byte-for-byte as it found it. offset, provenance, err := resolveOffset(opts, man, external) if err != nil { return 0, err } + if external { + if err := convertAudio(opts.Audio, wav); err != nil { + return 0, err + } + } + // An external recording's audio.wav is NOT captured at t0, and disk bytes // cannot distinguish it from a record-origin audio.wav. Persist the offset // beside audio.wav so a later bare `transcribe` (a re-run with a different // model, reusing audio.wav) reads it back instead of silently assuming 0 and // shifting every utterance by the forgotten offset. record-origin audio.wav // writes no sidecar and correctly defaults to 0. - if external { + // + // An explicit -offset on the in-place path rewrites an EXISTING sidecar: that + // is the operator correcting an offset a previous external run got wrong, and + // leaving the stale value in place applied the correction to one transcript + // only, for the next bare re-run to silently resurrect. A session with no + // sidecar is record-origin and stays that way — inventing one would make every + // later bare run inherit a one-off flag. + persist := external + if !persist && opts.OffsetSet { + if persist, err = offsetSidecarExists(opts.SessionDir); err != nil { + return 0, err + } + } + if persist { if err := writeOffsetSidecar(opts.SessionDir, offset, provenance); err != nil { return 0, fmt.Errorf("persist audio offset: %w", err) } @@ -242,6 +274,24 @@ func writeOffsetSidecar(dir string, offset float64, provenance string) error { return session.WriteFileNoFollow(filepath.Join(dir, session.AudioOffsetFile), append(b, '\n'), 0o644) } +// offsetSidecarExists reports whether a sidecar is already present beside +// audio.wav, without parsing it. The caller only needs to know whether this +// session already carries an external-audio offset, and an unusable sidecar +// (malformed, oversized) is precisely the one an explicit -offset repairs — the +// guidance readOffsetSidecar prints says so — so parsing here would refuse the +// repair. os.Lstat, not os.Stat: a symlink planted at the sidecar name must +// count as present, so the rewrite meets session.WriteFileNoFollow's refusal +// rather than being mistaken for absence and skipped. +func offsetSidecarExists(dir string) (bool, error) { + if _, err := os.Lstat(filepath.Join(dir, session.AudioOffsetFile)); err != nil { + if errors.Is(err, os.ErrNotExist) { + return false, nil + } + return false, err + } + return true, nil +} + // readOffsetSidecar reads the offset sidecar if present. ok is false with no // error when the file is absent (the record-origin case). A present-but-unusable // sidecar — unreadable, oversized, malformed, or carrying a non-finite offset — diff --git a/internal/transcribe/transcribe_test.go b/internal/transcribe/transcribe_test.go index b2c4bab..0f27781 100644 --- a/internal/transcribe/transcribe_test.go +++ b/internal/transcribe/transcribe_test.go @@ -2,6 +2,7 @@ package transcribe import ( "errors" + "io" "os" "os/exec" "path/filepath" @@ -392,6 +393,250 @@ func TestResolveOffsetExternalOffsetFlagNoT0(t *testing.T) { } } +// recordOriginAudio stands in for the bytes `testimony record` captured into +// audio.wav — the irreplaceable original a failed transcribe run must not +// destroy. +const recordOriginAudio = "RIFF record-origin capture" + +// fakeTools puts a stub whisperx, ffmpeg, and ffprobe on PATH so Run's +// detectEngine, convertAudio, and deriveOffset all resolve without any of the +// three installed. The stub whisperx writes a minimal valid engine output into +// the --output_dir it is handed (always its last argument). The stub ffprobe +// READS the recording it is pointed at, exactly as the real one does — that is +// what makes a FIFO at -audio block its open(2) for ever. +func fakeTools(t *testing.T) { + t.Helper() + bin := t.TempDir() + whisperx := "#!/bin/sh\nfor last; do :; done\n" + + `printf '%s' '{"segments":[{"start":1,"end":2,"text":"Alice speaks."}]}' > "$last/audio.json"` + "\n" + // `: < FILE` opens the recording with a shell builtin — PATH here holds only + // these stubs, so an external reader such as cat would not be found and the + // open would never happen. + ffprobe := "#!/bin/sh\nfor last; do :; done\n: < \"$last\"\nprintf '%s' '{}'\n" + for name, script := range map[string]string{ + "whisperx": whisperx, + "ffmpeg": "#!/bin/sh\nexit 0\n", + "ffprobe": ffprobe, + } { + if err := os.WriteFile(filepath.Join(bin, name), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + } + t.Setenv("PATH", bin) +} + +// seedSession writes a manifest and a record-origin audio.wav, returning the +// session directory and the audio path. +func seedSession(t *testing.T, m session.Manifest) (dir, wav string) { + t.Helper() + dir = t.TempDir() + if err := session.SaveManifest(dir, m); err != nil { + t.Fatalf("SaveManifest: %v", err) + } + wav = filepath.Join(dir, session.AudioFile) + if err := os.WriteFile(wav, []byte(recordOriginAudio), 0o644); err != nil { + t.Fatal(err) + } + return dir, wav +} + +// stubConvert stands in for ffmpeg and reports, through the returned pointer, +// whether the conversion ran at all. It writes different bytes from +// recordOriginAudio, so a conversion that reached audio.wav is visible in the +// file's content. +func stubConvert(t *testing.T) *bool { + t.Helper() + old := convertRunner + t.Cleanup(func() { convertRunner = old }) + ran := false + convertRunner = func(ffmpeg, in, tmpPath string) error { + ran = true + return os.WriteFile(tmpPath, []byte("RIFF converted external recording"), 0o644) + } + return &ran +} + +// externalRecording writes a stand-in recording outside the session directory. +func externalRecording(t *testing.T) string { + t.Helper() + path := filepath.Join(t.TempDir(), "bob-interview.m4a") + if err := os.WriteFile(path, []byte("external recording"), 0o644); err != nil { + t.Fatal(err) + } + return path +} + +// TestRunResolvesOffsetBeforeConverting is the destroyed-capture regression. +// Pre-fix Run converted the external recording over the session's audio.wav +// BEFORE resolving the offset, so every way resolution can fail — an unusable +// manifest t0, an implausible derived offset — exited 1 with the record-origin +// capture already overwritten and no audio.offset.json written. The session was +// then indistinguishable from a record-origin one (no sidecar ⇒ captured at t0, +// docs/reference/session-directory.md), so a later bare `transcribe` took the +// in-place branch, reported the false provenance "captured at t0", and wrote a +// silently time-shifted transcript at exit 0. Resolution now runs first, so a +// refused run leaves the session exactly as it found it. +func TestRunResolvesOffsetBeforeConverting(t *testing.T) { + t.Run("unusable t0", func(t *testing.T) { + fakeTools(t) + // No t0_epoch_ms: a received or hand-edited manifest. + dir, wav := seedSession(t, session.Manifest{Session: "2026-07-22_bob-received"}) + converted := stubConvert(t) + + _, err := Run(Options{SessionDir: dir, Audio: externalRecording(t), Engine: EngineWhisperX, Log: io.Discard}) + if err == nil { + t.Fatal("an external recording with no usable t0 must fail the run") + } + if !errors.Is(err, session.ErrNoT0) { + t.Fatalf("want an ErrNoT0-based error, got %v", err) + } + assertSessionUntouched(t, dir, wav, converted) + }) + + t.Run("implausible derived offset", func(t *testing.T) { + fakeTools(t) + dir, wav := seedSession(t, session.Manifest{Session: "s", T0EpochMS: 1_700_000_000_000}) + converted := stubConvert(t) + old := deriveOffsetFn + t.Cleanup(func() { deriveOffsetFn = old }) + deriveOffsetFn = func(audio string, t0EpochMS int64) (float64, bool) { return 5e9, true } + + _, err := Run(Options{SessionDir: dir, Audio: externalRecording(t), Engine: EngineWhisperX, Log: io.Discard}) + if err == nil || !strings.Contains(err.Error(), "exceeds") { + t.Fatalf("an implausible derived offset must fail the run, got %v", err) + } + assertSessionUntouched(t, dir, wav, converted) + }) +} + +// assertSessionUntouched checks that a refused run left neither a converted +// audio.wav nor a sidecar behind. +func assertSessionUntouched(t *testing.T, dir, wav string, converted *bool) { + t.Helper() + if *converted { + t.Error("the conversion ran before the offset was resolved; a refused run must not touch the session's audio") + } + if b, err := os.ReadFile(wav); err != nil || string(b) != recordOriginAudio { + t.Errorf("the record-origin %s was replaced by a refused run: %q (err=%v)", session.AudioFile, b, err) + } + if _, err := os.Stat(filepath.Join(dir, session.AudioOffsetFile)); !os.IsNotExist(err) { + t.Errorf("a refused run wrote %s (err=%v); the session now claims an offset it never resolved", session.AudioOffsetFile, err) + } +} + +// TestRunRefusesNonRegularExternalAudio is the hang regression on the offset +// derivation. The refusals that make -audio safe to open — the accepted-container +// check and the regular-file check — lived inside convertAudio, so hoisting the +// offset resolution above the conversion put an ffprobe subprocess on the +// operator-named path AHEAD of them: ffprobe opens without O_NONBLOCK, so a FIFO +// at -audio (a session bundle is an exchange unit, and tar preserves FIFOs) +// blocked its open(2) for ever and `testimony transcribe` hung with no error +// instead of refusing in milliseconds. The guard now runs before anything opens +// the recording. The run happens in a goroutine and the test fails on a timeout, +// so a regression reports a failure rather than hanging the suite for ever. +func TestRunRefusesNonRegularExternalAudio(t *testing.T) { + fakeTools(t) + dir, _ := seedSession(t, session.Manifest{Session: "s", T0EpochMS: 1_700_000_000_000}) + fifo := filepath.Join(t.TempDir(), "bob-interview.m4a") + if err := syscall.Mkfifo(fifo, 0o644); err != nil { + t.Skipf("FIFOs unavailable on this platform: %v", err) + } + + done := make(chan error, 1) + go func() { + _, err := Run(Options{SessionDir: dir, Audio: fifo, Engine: EngineWhisperX, Log: io.Discard}) + done <- err + }() + select { + case err := <-done: + if err == nil || !strings.Contains(err.Error(), "not a regular file") { + t.Fatalf("want a non-regular-file refusal, got %v", err) + } + case <-time.After(5 * time.Second): + t.Fatal("transcribe blocked on a FIFO -audio instead of refusing it") + } +} + +// TestRunRefusesUnsupportedExternalAudioBeforeProbing is the same ordering +// invariant on the container check: a file whose extension the pipeline does not +// accept must be refused by name, before ffprobe is asked to parse it — the +// probe is a media parser, and pointing it at arbitrary operator-named files +// widens its attack surface for nothing. +func TestRunRefusesUnsupportedExternalAudioBeforeProbing(t *testing.T) { + fakeTools(t) + dir, _ := seedSession(t, session.Manifest{Session: "s", T0EpochMS: 1_700_000_000_000}) + notes := filepath.Join(t.TempDir(), "notes.txt") + if err := os.WriteFile(notes, []byte("not a recording"), 0o644); err != nil { + t.Fatal(err) + } + old := deriveOffsetFn + t.Cleanup(func() { deriveOffsetFn = old }) + probed := false + deriveOffsetFn = func(audio string, t0EpochMS int64) (float64, bool) { + probed = true + return 0, false + } + + _, err := Run(Options{SessionDir: dir, Audio: notes, Engine: EngineWhisperX, Log: io.Discard}) + if err == nil || !strings.Contains(err.Error(), "unsupported audio format") { + t.Fatalf("want an unsupported-format refusal, got %v", err) + } + if probed { + t.Error("the recording was probed before its container was accepted") + } +} + +// TestRunExplicitOffsetUpdatesExistingSidecar is the discarded-correction +// regression: an operator who spots a wrong offset re-runs with -offset N, but +// pre-fix the in-place path never rewrote the sidecar, so the correction applied +// to that one transcript and the next bare re-run silently resurrected the stale +// value. An existing sidecar is the session's record of an external audio origin, +// so an explicit -offset now rewrites it. +func TestRunExplicitOffsetUpdatesExistingSidecar(t *testing.T) { + fakeTools(t) + man := session.Manifest{Session: "s", T0EpochMS: 1_700_000_000_000} + dir, _ := seedSession(t, man) + if err := writeOffsetSidecar(dir, 12.5, "derived: audio creation_time − manifest t0"); err != nil { + t.Fatalf("writeOffsetSidecar: %v", err) + } + + if _, err := Run(Options{SessionDir: dir, Engine: EngineWhisperX, Offset: 30, OffsetSet: true, Log: io.Discard}); err != nil { + t.Fatalf("Run: %v", err) + } + + off, _, ok, err := readOffsetSidecar(dir) + if err != nil || !ok { + t.Fatalf("sidecar after the correction: ok=%v err=%v", ok, err) + } + if off != 30 { + t.Fatalf("sidecar offset: got %v, want 30 (the operator's correction, not the stale 12.5)", off) + } + // The next bare run must resolve to the corrected value. + got, _, err := resolveOffset(Options{SessionDir: dir}, man, false) + if err != nil { + t.Fatalf("bare re-run: %v", err) + } + if got != 30 { + t.Fatalf("bare re-run offset: got %v, want 30", got) + } +} + +// TestRunExplicitOffsetWritesNoSidecarForRecordOrigin guards the other half of +// the sidecar model: a session with no sidecar is record-origin (audio.wav +// captured at t0), and an explicit -offset for one transcript must not invent a +// sidecar that would make every later bare run inherit it. +func TestRunExplicitOffsetWritesNoSidecarForRecordOrigin(t *testing.T) { + fakeTools(t) + dir, _ := seedSession(t, session.Manifest{Session: "s", T0EpochMS: 1_700_000_000_000}) + + if _, err := Run(Options{SessionDir: dir, Engine: EngineWhisperX, Offset: 30, OffsetSet: true, Log: io.Discard}); err != nil { + t.Fatalf("Run: %v", err) + } + if _, err := os.Stat(filepath.Join(dir, session.AudioOffsetFile)); !os.IsNotExist(err) { + t.Fatalf("a record-origin session must stay sidecar-free, got err=%v", err) + } +} + // TestSameFileTreatedInPlace proves -audio pointing at the session's own // audio.wav is recognised as the in-place case (no self-conversion). func TestSameFileTreatedInPlace(t *testing.T) { From 921bc7b708b1f18f08eb4226ccf0c0c12c00c3cb Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Wed, 29 Jul 2026 07:32:14 +0100 Subject: [PATCH 02/20] fix: reject a non-finite report -window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit -window accepted any float64 strconv parses, so NaN and +/-Inf reached report.Render unchecked and both produced a report.md that misstates the record, at exit 0. Every comparison against NaN is false, so a NaN window detached every interface event from the speech it accompanied; +Inf put every event inside the first utterance's window and filed them all under it. report.md is the human evidence artefact, so either outcome fabricates what the participant was doing while they spoke. Only finiteness is required: a negative window is legitimate — it narrows the join — and stays accepted. Assisted-by: Claude:claude-opus-5 --- internal/cli/cli.go | 11 +++++++ internal/cli/cli_test.go | 62 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+) diff --git a/internal/cli/cli.go b/internal/cli/cli.go index ecf9f0c..493c009 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -4,6 +4,7 @@ package cli import ( "flag" "fmt" + "math" "os" "path/filepath" "strings" @@ -84,6 +85,16 @@ func Run(args []string) int { if *dir == "" { return fail(fmt.Errorf("report: -session is required")) } + // A non-finite window is not a join window at all, and Render has no way to + // notice: every comparison against NaN is false, so a NaN window silently + // detaches every event from the speech it accompanied, while +Inf puts every + // event inside the first utterance's window and files them all under it. + // Either way report.md — the human evidence artefact — misstates what the + // participant was doing while they spoke, and the command exits 0. A negative + // window is legitimate (it narrows the join), so only finiteness is required. + if math.IsNaN(*window) || math.IsInf(*window, 0) { + return fail(fmt.Errorf("report: -window must be a finite number of seconds, got %v", *window)) + } md, err := report.Render(*dir, *window) if err != nil { return fail(err) diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 37fc853..ce1a148 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -1,8 +1,10 @@ package cli import ( + "io" "os" "path/filepath" + "strings" "syscall" "testing" "time" @@ -10,6 +12,30 @@ import ( "github.com/REPPL/Testimony/internal/session" ) +// captureStderr runs fn with os.Stderr redirected to a pipe and returns +// everything it wrote there, so a test can assert on the operator-facing +// message and not merely on the exit code. +func captureStderr(t *testing.T, fn func()) string { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + old := os.Stderr + os.Stderr = w + read := make(chan string, 1) + go func() { + b, _ := io.ReadAll(r) + read <- string(b) + }() + fn() + os.Stderr = old + w.Close() + got := <-read + r.Close() + return got +} + // miniSession writes a minimal but valid session (manifest + one timeline entry) // so `analyze` reaches its -out write / -ingest read without failing earlier. func miniSession(t *testing.T) string { @@ -69,3 +95,39 @@ func TestAnalyzeIngestRefusesFIFO(t *testing.T) { t.Fatal("analyze -ingest blocked on a FIFO instead of refusing it") } } + +// TestReportRejectsNonFiniteWindow is the fabricated-join regression: -window +// took any float64 strconv would parse, so NaN and ±Inf reached report.Render. +// Every comparison against NaN is false, so a NaN window detached every event +// from the speech it accompanied; +Inf made every event fall inside the first +// utterance's window and be filed under it. Both wrote a report.md that misstates +// what the participant was doing while they spoke, and both exited 0. A negative +// window is legitimate (it narrows the join) and must stay accepted. +func TestReportRejectsNonFiniteWindow(t *testing.T) { + for _, w := range []string{"NaN", "Inf", "-Inf"} { + dir := miniSession(t) + var code int + stderr := captureStderr(t, func() { + code = Run([]string{"report", "-session", dir, "-window", w}) + }) + if code == 0 { + t.Fatalf("-window %s returned success; want a refusal", w) + } + if !strings.Contains(stderr, "testimony: report: -window must be a finite number") { + t.Fatalf("-window %s: want the finite-window refusal on stderr, got %q", w, stderr) + } + if _, err := os.Stat(filepath.Join(dir, session.ReportFile)); !os.IsNotExist(err) { + t.Fatalf("-window %s rendered a report anyway (err=%v)", w, err) + } + } + + for _, w := range []string{"2.5", "-1"} { + dir := miniSession(t) + if code := Run([]string{"report", "-session", dir, "-window", w}); code != 0 { + t.Fatalf("-window %s must still render, got exit %d", w, code) + } + if _, err := os.Stat(filepath.Join(dir, session.ReportFile)); err != nil { + t.Fatalf("-window %s wrote no report: %v", w, err) + } + } +} From 89131e6d68409dc52a17843deaa4fb734b27dfe5 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Wed, 29 Jul 2026 07:32:14 +0100 Subject: [PATCH 03/20] fix: order the report by time, not by timeline file order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit report.Render consumed timeline.jsonl in line order and never sorted, so a hand-edited or exchanged timeline rendered utterances and standalone events in whatever order the lines sat in — [00:50] printed above [00:10]. Render already refuses to assume that timeline came from merge (its join step keys on position, not on ids merge would have made unique), and report.md is read as the chronological record of the session, so the file order silently misstated when things happened. The standalone-event flush likewise walks events forward assuming they ascend. The same stable sort by t that timeline.Merge applies now runs on load, so a merge-produced timeline renders byte-for-byte as before. Assisted-by: Claude:claude-opus-5 --- internal/report/report.go | 10 +++++++++ internal/report/report_test.go | 37 ++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/internal/report/report.go b/internal/report/report.go index efb9539..c9b2f73 100644 --- a/internal/report/report.go +++ b/internal/report/report.go @@ -36,6 +36,16 @@ func Render(dir string, window float64) (string, error) { if err != nil { return "", fmt.Errorf("read timeline (run `testimony merge` first?): %w", err) } + // Order the record by time rather than trusting the file's line order. The + // timeline this reads is not necessarily the one merge wrote: a hand-edited or + // exchanged timeline.jsonl reaches Render directly — the same assumption the + // join step below already refuses to make about ids — and an out-of-order one + // rendered [00:50] above [00:10], so report.md, which is read as the + // chronological record of the session, silently misstated when things happened. + // The standalone-event flush below also walks events forward on the assumption + // they ascend. sort.SliceStable with the same less function timeline.Merge uses + // keeps the two orderings identical, so a merge-produced timeline is unchanged. + sort.SliceStable(entries, func(i, j int) bool { return entries[i].T < entries[j].T }) var speech, events []timeline.Entry for _, e := range entries { diff --git a/internal/report/report_test.go b/internal/report/report_test.go index 2ebcd4f..3a22548 100644 --- a/internal/report/report_test.go +++ b/internal/report/report_test.go @@ -533,3 +533,40 @@ func TestReportFlushesEventPastLegacySentinel(t *testing.T) { t.Fatalf("standalone event at the legacy 1e18 sentinel was dropped from the report:\n%s", md) } } + +// TestRenderSortsByTime is the out-of-order-report regression. Render consumed +// timeline.jsonl in file order, so a hand-edited or exchanged timeline — the case +// Render's own join step already defends against — rendered utterances and +// standalone events in whatever order the lines happened to sit in, printing +// [00:50] before [00:10]. report.md is read as the chronological record of the +// session, so the file order silently rewrote when things happened. Render now +// applies the same stable sort by t that timeline.Merge does. +func TestRenderSortsByTime(t *testing.T) { + const unsorted = `{"t":50,"src":"speech","id":"utt-002","payload":{"speaker":"P1","t1":52,"text":"Second remark."}} +{"t":10,"src":"speech","id":"utt-001","payload":{"speaker":"P1","t1":12,"text":"First remark."}} +{"t":40,"src":"event","id":"ev-002","payload":{"kind":"scroll"}} +{"t":5,"src":"event","id":"ev-001","payload":{"kind":"click"}} +` + dir := setupSession(t) + if err := os.WriteFile(filepath.Join(dir, session.TimelineFile), []byte(unsorted), 0o644); err != nil { + t.Fatalf("write timeline: %v", err) + } + + md, err := Render(dir, 2.5) + if err != nil { + t.Fatalf("Render: %v", err) + } + // Every entry is outside every join window here, so all four appear at the top + // level and must read in chronological order. + prev := -1 + for _, want := range []string{"[00:05] click", "[00:10]", "First remark", "[00:40] scroll", "[00:50]", "Second remark"} { + at := strings.Index(md, want) + if at < 0 { + t.Fatalf("report is missing %q:\n%s", want, md) + } + if at < prev { + t.Fatalf("report is out of chronological order at %q:\n%s", want, md) + } + prev = at + } +} From baeff176103b7193aaf1f62d5e840f1fc32cc1d9 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Wed, 29 Jul 2026 07:32:14 +0100 Subject: [PATCH 04/20] fix: exit 2 on a missing required -session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A missing required flag exited 1 through fail(), the status reserved for a runtime failure of a well-formed command, while every sibling usage error — no command, an unknown command, a flag-parse failure — exited 2. To a caller that branches on the status, a script or CI, "you forgot -session" was therefore indistinguishable from "this session could not be read", and the two need different remedies. The five -session checks now report through usageErr, which keeps the shared "testimony: " stderr shape and returns 2; the exit-status table in docs/reference/cli.md states the contract. Assisted-by: Claude:claude-opus-5 --- docs/reference/cli.md | 2 +- internal/cli/cli.go | 29 +++++++++++++++++++++++------ internal/cli/cli_test.go | 28 ++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 7 deletions(-) diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 07e977e..5780260 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -12,7 +12,7 @@ Running `testimony` with no command, or with an unknown command, prints the usag |---|---| | 0 | success | | 1 | runtime error — the message is printed to stderr as `testimony: ` | -| 2 | usage error or unknown command | +| 2 | usage error — no command, an unknown command, an unparseable flag, or a missing required flag | ## `testimony demo` diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 493c009..c09715d 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -67,7 +67,7 @@ func Run(args []string) int { dir := fs.String("session", "", "session directory") fs.Parse(rest) if *dir == "" { - return fail(fmt.Errorf("merge: -session is required")) + return usageErr(fmt.Errorf("merge: -session is required")) } speech, events, err := timeline.Merge(*dir) if err != nil { @@ -83,7 +83,7 @@ func Run(args []string) int { window := fs.Float64("window", 2.5, "utterance↔event join window, seconds") fs.Parse(rest) if *dir == "" { - return fail(fmt.Errorf("report: -session is required")) + return usageErr(fmt.Errorf("report: -session is required")) } // A non-finite window is not a join window at all, and Render has no way to // notice: every comparison against NaN is false, so a NaN window silently @@ -147,7 +147,7 @@ func Run(args []string) int { offset := fs.Float64("offset", 0, "audio→session clock offset in seconds (default: derived from the recording's creation time)") fs.Parse(rest) if *dir == "" { - return fail(fmt.Errorf("transcribe: -session is required")) + return usageErr(fmt.Errorf("transcribe: -session is required")) } offsetSet := false fs.Visit(func(f *flag.Flag) { @@ -181,7 +181,7 @@ func Run(args []string) int { ingest := fs.String("ingest", "", "validate answer JSON at FILE (or \"-\" for stdin) into findings.jsonl") fs.Parse(rest) if *dir == "" { - return fail(fmt.Errorf("analyze: -session is required")) + return usageErr(fmt.Errorf("analyze: -session is required")) } if *ingest != "" { if *out != "" { @@ -235,7 +235,7 @@ func Run(args []string) int { verdict := fs.String("verdict", "", "non-interactive: confirmed | rejected | duplicate-of-F-NNN") fs.Parse(rest) if *dir == "" { - return fail(fmt.Errorf("review: -session is required")) + return usageErr(fmt.Errorf("review: -session is required")) } if err := review.Run(review.Options{ Dir: *dir, @@ -264,11 +264,28 @@ func Run(args []string) int { } } -func fail(err error) int { +// printErr writes an operator-facing error in the one shape every command uses. +func printErr(err error) { fmt.Fprintln(os.Stderr, "testimony:", err) +} + +// fail reports a runtime failure of a well-formed command — the invocation was +// right and the work could not be done (exit 1). +func fail(err error) int { + printErr(err) return 1 } +// usageErr reports a wrong invocation (exit 2), the status the no-command, +// unknown-command, and flag-parse paths already use. A missing required flag +// belongs with them: reported as a runtime error it was indistinguishable to a +// caller — a script, CI — from a session that genuinely could not be read. +// docs/reference/cli.md states the contract. +func usageErr(err error) int { + printErr(err) + return 2 +} + // isCharDevice reports whether f is an interactive terminal, gating review's // interactive walk so CI (where stdin is a pipe) never blocks. func isCharDevice(f *os.File) bool { diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index ce1a148..03dc0b8 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -131,3 +131,31 @@ func TestReportRejectsNonFiniteWindow(t *testing.T) { } } } + +// TestMissingSessionIsAUsageError pins the exit-status contract of +// docs/reference/cli.md: a wrong invocation exits 2 and a runtime failure of a +// well-formed command exits 1. A missing required -session was reported as a +// runtime error (1), so it was indistinguishable to a caller from a session that +// genuinely could not be read, while the sibling usage errors — no command, an +// unknown command, a flag-parse failure — all exited 2. +func TestMissingSessionIsAUsageError(t *testing.T) { + for _, cmd := range []string{"merge", "report", "transcribe", "analyze", "review"} { + var code int + stderr := captureStderr(t, func() { code = Run([]string{cmd}) }) + if code != 2 { + t.Errorf("%s without -session: exit %d, want 2 (usage error)", cmd, code) + } + if want := "testimony: " + cmd + ": -session is required"; !strings.Contains(stderr, want) { + t.Errorf("%s without -session: want %q on stderr, got %q", cmd, want, stderr) + } + } + + // A well-formed command that fails at runtime keeps exit 1. + var code int + captureStderr(t, func() { + code = Run([]string{"merge", "-session", filepath.Join(t.TempDir(), "absent")}) + }) + if code != 1 { + t.Errorf("merge on an unreadable session: exit %d, want 1 (runtime error)", code) + } +} From 0951d658ce301ddb1edfddb4ab1c2fe0f12170c7 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Wed, 29 Jul 2026 07:32:35 +0100 Subject: [PATCH 05/20] chore: align the pinned commit identity with the GitHub account The pinned identity name now matches the global git configuration and the author of the repository's entire history (REPPL), so sessions that read .abcd/config/identity.json no longer try to switch the author name. Assisted-by: Claude:claude-fable-5 --- .abcd/config/identity.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.abcd/config/identity.json b/.abcd/config/identity.json index 9070b87..e546977 100644 --- a/.abcd/config/identity.json +++ b/.abcd/config/identity.json @@ -1,4 +1,4 @@ { - "name": "Alex Reppel", + "name": "REPPL", "email": "77722411+REPPL@users.noreply.github.com" } From 667d595335e364d3377e6dfb210367b9b19fdf83 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Wed, 29 Jul 2026 07:37:45 +0100 Subject: [PATCH 06/20] fix: install v0.4.0 and gate the pin on every release MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit install.sh pinned VERSION="v0.1.0" while v0.4.0 was current, so the one-line installer handed every new user a binary three releases old. With gh present the install failed outright: v0.1.0 predates the release workflow's build attestations, so `gh attestation verify` finds no provenance and the script refuses to install. Without gh it silently installed a binary that predates the v0.2.0 security hardening and has no record/analyze/review commands, so the tutorial dies at its first step, `testimony record -demo`. The number is the symptom; the cause is that nothing noticed the pin had gone stale. Add a check to the release workflow's verify job comparing install.sh's pinned VERSION against the tag being released, failing the release when they differ. It only compares and fails — the release workflow still pushes to no branch — so the bump must land in the tagged commit itself, which is where it has to be anyway for the tarball the script fetches to exist. Verification semantics (SHA256SUMS, attestation) are untouched. Assisted-by: Claude:claude-opus-5 --- .github/workflows/release.yml | 28 +++++++++++++++++++++++++++- install.sh | 2 +- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index fea3f0c..ee551f1 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -39,7 +39,8 @@ permissions: jobs: # Gate the release on the pushed commit, mirroring ci.yml's check job (gofmt, - # build, vet, the race leg, and the pipeline smoke). A red step here aborts + # build, vet, the race leg, and the pipeline smoke) plus one release-only check: + # that install.sh's pinned version matches the tag. A red step here aborts # before anything is built or published. Checked out at github.sha — the commit # the pushed tag pointed at, never the default-branch tip or the re-resolvable # tag name — so the gate exercises exactly the commit whose tarballs will ship. @@ -58,6 +59,31 @@ jobs: ref: ${{ github.sha }} persist-credentials: false + - name: Assert install.sh pins the released version + # install.sh carries a default VERSION= pin naming the release a one-line + # `curl … | sh` install fetches. Nothing else notices when a release moves + # on and the pin does not, and a stale pin hands every new user an old + # binary — or, once that old release predates build attestations, an + # install that dies in the gh verification step. This workflow pushes to no + # branch, so the pin cannot be corrected from here: the check only compares + # and fails, which forces the bump to land in the tagged commit itself, + # where install.sh must already name the version it installs. Plain string + # equality against the checked-out file. TAG arrives via env and is read as + # a shell variable, never interpolated into this script (injection-safe). + run: | + set -euo pipefail + pinned="$(sed -n 's/^VERSION="\(.*\)"$/\1/p' install.sh)" + if [ -z "$pinned" ]; then + echo "could not read the VERSION pin from install.sh (has the assignment moved?)" >&2 + exit 1 + fi + if [ "$pinned" != "$TAG" ]; then + echo "install.sh pins VERSION=\"$pinned\", but this release is $TAG." >&2 + echo "Bump the pin in install.sh and re-tag: the tagged commit must name the release it installs." >&2 + exit 1 + fi + echo "install.sh pins $pinned, matching the released tag." + - name: Set up Go uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: diff --git a/install.sh b/install.sh index 3876ffb..8c34da5 100644 --- a/install.sh +++ b/install.sh @@ -33,7 +33,7 @@ set -eu REPO="REPPL/Testimony" -VERSION="v0.1.0" +VERSION="v0.4.0" # Pinned OpenPGP fingerprint of the evermeet.cx ffmpeg publisher key # (Helmut K. C. Tessarek, key id 0x476C4B611A660874). The local-macOS ffmpeg From 3e70a3cccda3b6e0ef9762102eb47909792bade7 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Wed, 29 Jul 2026 07:37:51 +0100 Subject: [PATCH 07/20] chore: correct the pipeline-smoke comment in ci.yml MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment claimed the smoke test "runs on both OSes ... on each host", but the check job is ubuntu-only — the same file documents dropping the macOS leg forty lines above. A comment that contradicts its own workflow misleads the next reader into assuming macOS coverage that does not exist. Say what the step actually does. Assisted-by: Claude:claude-opus-5 --- .github/workflows/ci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5c663da..9d4a5cd 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -65,8 +65,8 @@ jobs: # The full pipeline against the bundled sample session: merge must produce # a timeline, report must render it, and both outputs must be non-empty. - # Runs on both OSes — it also proves the freshly built binary actually runs - # the pipeline on each host. + # Ubuntu-only, like the rest of this job — it also proves the freshly built + # binary actually runs the pipeline, not merely that it compiles. - name: Pipeline smoke test run: | set -euo pipefail From b2cb6dfd27455151962988c744f734375ecc07c8 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Wed, 29 Jul 2026 07:37:56 +0100 Subject: [PATCH 08/20] fix: stop choose() offering the same option twice Both no-Homebrew fallbacks call choose() with one option repeated ("local" "local", "whisperx" "whisperx"), because there is genuinely only one choice when brew is absent. That rendered the prompts as [local/local/skip] and [whisperx/whisperx/skip], which reads like a bug to the user at the exact moment they are being asked to trust the installer. Collapse a duplicated option so it is offered, and matched, once. Distinct options, --yes returning the first option, and an unrecognized reply meaning skip are all unchanged. Assisted-by: Claude:claude-opus-5 --- install.sh | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/install.sh b/install.sh index 8c34da5..2c8cf18 100644 --- a/install.sh +++ b/install.sh @@ -64,12 +64,15 @@ ask() { return 1 } -# choose "Question" "option-a" "option-b" → prints the chosen word. +# choose "Question" "option-a" "option-b" → prints the chosen word. When both +# options are the same word there is only one choice to offer, so it is rendered +# (and matched) once: [local/skip], never [local/local/skip]. choose() { q="$1"; a="$2"; b="$3" if [ "$ASSUME_YES" = 1 ]; then printf '%s' "$a"; return; fi + if [ "$a" = "$b" ]; then opts="$a"; else opts="$a/$b"; fi if [ -r /dev/tty ] && [ -w /dev/tty ]; then - printf '%s [%s/%s/skip] ' "$q" "$a" "$b" > /dev/tty + printf '%s [%s/skip] ' "$q" "$opts" > /dev/tty IFS= read -r reply < /dev/tty || reply="" case "$reply" in "$a") printf '%s' "$a" ;; From 918fcb58591445139dba90c97a42d992151f0a38 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Wed, 29 Jul 2026 07:38:02 +0100 Subject: [PATCH 09/20] fix: name the cause when the release tarball cannot be downloaded A --version naming a release that does not exist (or a tag written without its leading v) reached the user only as a raw curl/wget failure in the middle of "Downloading ...", which names neither the cause nor the fix. Report the release and platform asset that were not found, the vX.Y.Z tag form, and where the releases are listed. Still fail-closed: die exits non-zero, nothing is retried, and no dependency is added. Assisted-by: Claude:claude-opus-5 --- install.sh | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/install.sh b/install.sh index 2c8cf18..98cc7cc 100644 --- a/install.sh +++ b/install.sh @@ -119,7 +119,14 @@ install_binary() { trap 'rm -rf "$tmp"' EXIT INT TERM say "Downloading $tarball ..." - fetch "$base/$tarball" "$tmp/$tarball" + # A bad --version (or a platform the release never published) surfaces from + # curl/wget as a bare 404 in the middle of "Downloading". Name the actual + # cause instead. Still fail-closed: die exits non-zero, nothing is retried. + fetch "$base/$tarball" "$tmp/$tarball" || die "could not download $tarball + Release \"$VERSION\" — or its $plat asset — was not found at + $base/$tarball + Release tags are of the form vX.Y.Z (note the leading 'v'). + Releases: https://github.com/$REPO/releases" # Integrity: verify the tarball against the release's published SHA256SUMS. # No hash is pinned in this script — it is fetched from the release itself. From 4fbb39abf78c8889126986eeaff23a9508150b79 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Wed, 29 Jul 2026 07:38:08 +0100 Subject: [PATCH 10/20] fix: declare a successor for every docs-lint banned token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `abcd docs lint` refused to load .abcd/docs-lint.json at all, failing with "banned_tokens entry present_tense/previously has no successor", so the repo's documentation-currency lint was a silent no-op: every invocation, including the release gate, ended in a config parse error rather than a lint result. The banned_tokens schema requires each entry to carry a machine-readable successor — the replacement the finding message cites — and none of the nine entries did. Add the successor each ban already described in prose. Assisted-by: Claude:claude-opus-5 --- .abcd/docs-lint.json | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/.abcd/docs-lint.json b/.abcd/docs-lint.json index 77becfe..5581741 100644 --- a/.abcd/docs-lint.json +++ b/.abcd/docs-lint.json @@ -1,15 +1,15 @@ { "roots": ["docs", "README.md"], "banned_tokens": [ - {"id":"present_tense/previously","pattern":"(?i)\\bpreviously\\b","severity":"blocker","allow_context":["(?i) if this genuinely describes something external."}, - {"id":"present_tense/formerly","pattern":"(?i)\\bformerly\\b","severity":"blocker","allow_context":["(?i) if this genuinely describes something external."}, - {"id":"present_tense/to-be-implemented","pattern":"(?i)\\bto be implemented\\b","severity":"blocker","allow_context":["(?i) if this genuinely describes something external."}, - {"id":"present_tense/will-be-added","pattern":"(?i)\\bwill be (added|implemented|introduced)\\b","severity":"blocker","allow_context":["(?i) if this genuinely describes something external."}, - {"id":"present_tense/has-been-replaced","pattern":"(?i)\\b(has|have|had)\\s+been\\s+replaced\\b","severity":"blocker","allow_context":["(?i) if this genuinely describes something external."}, - {"id":"present_tense/renamed-from","pattern":"(?i)\\brenamed from\\b","severity":"blocker","allow_context":["(?i) if this genuinely describes something external."}, - {"id":"present_tense/no-longer","pattern":"(?i)\\bno longer\\b","severity":"warn","allow_context":["(?i) if this genuinely describes something external."}, + {"id":"present_tense/formerly","pattern":"(?i)\\bformerly\\b","severity":"blocker","successor":"present-tense phrasing","allow_context":["(?i) if this genuinely describes something external."}, + {"id":"present_tense/to-be-implemented","pattern":"(?i)\\bto be implemented\\b","severity":"blocker","successor":"present-tense phrasing","allow_context":["(?i) if this genuinely describes something external."}, + {"id":"present_tense/will-be-added","pattern":"(?i)\\bwill be (added|implemented|introduced)\\b","severity":"blocker","successor":"present-tense phrasing","allow_context":["(?i) if this genuinely describes something external."}, + {"id":"present_tense/has-been-replaced","pattern":"(?i)\\b(has|have|had)\\s+been\\s+replaced\\b","severity":"blocker","successor":"present-tense phrasing","allow_context":["(?i) if this genuinely describes something external."}, + {"id":"present_tense/renamed-from","pattern":"(?i)\\brenamed from\\b","severity":"blocker","successor":"present-tense phrasing","allow_context":["(?i) if this genuinely describes something external."}, + {"id":"present_tense/no-longer","pattern":"(?i)\\bno longer\\b","severity":"warn","successor":"present-tense phrasing","allow_context":["(?i)