diff --git a/.abcd/work/DECISIONS.md b/.abcd/work/DECISIONS.md index 3e6f8ed..4d2807e 100644 --- a/.abcd/work/DECISIONS.md +++ b/.abcd/work/DECISIONS.md @@ -283,3 +283,13 @@ Architecture-shaping decisions graduate to an ADR under not running in CI (tooling), doubled CI runs on PR branches (trigger set is tied to the merge queue), and the /dev/null TTY gate (per round 8). +- 2026-07-30 — Bug-hunt round 11: hunts and refutations ran sequentially in + one session (the subagent model was persistently overloaded server-side; + the routine's sequential fallback). One substantive fix: an explicit + `transcribe -offset` is validated at the CLI (finite, within ±1e9 s) — + the flag was the one unbounded offset entry point, running the engine + before a bare JSON error, or writing a transcript merge refuses and a + sidecar transcribe's own reader refuses. Nitpicks: a demo bind failure + (port taken) no longer creates a stray session directory; the CHANGELOG's + Alice-persona claim corrected to Bob. Refuted: documenting the capture + endpoints' 500 (the enumerated codes are the request-refusal contract). diff --git a/CHANGELOG.md b/CHANGELOG.md index 86bfb91..f0441b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,7 +40,8 @@ Capture and diagnostics: unopenable `http://localhost:0`; the same applies to `record -demo`. - The demo page falls back to `fetch` when `sendBeacon` refuses to queue a capture batch, which used to drop the batch silently; its seeded display - name now uses the Alice persona. + name now uses the Bob persona, so the tutorial's rename to Alice is a real + change. - Diagnostic stderr tails cut on rune boundaries rather than mid-character; a record refused by the JSONL writer is named as a 1-based line of the output file; `transcribe` defaults its log sink instead of panicking when @@ -99,6 +100,16 @@ Invocation contract: `-finding`/`-verdict` pairing and verdict syntax, an unknown `transcribe -engine`, and a malformed capture `-addr` (which also no longer creates a session directory before refusing). +- An explicit `transcribe -offset` is held to the same bound as a derived or + persisted one: a non-finite value used to fail only after the conversion or + engine work was already spent, with a bare JSON encoding error at the + runtime status, and a finite but absurd one wrote a transcript at exit 0 + that `merge` refuses one command later — naming `transcript.jsonl` rather + than the flag — while an external run persisted a sidecar `transcribe`'s + own reader refuses. +- A `demo` whose well-formed address fails to bind (the port is already taken) + no longer creates a session directory first, which left a stray session — + manifest plus two empty stream files — behind at every refused bind. Capture integrity: diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 221d2fd..d602a77 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -58,7 +58,7 @@ testimony transcribe -session DIR [-audio FILE] | `-device` | `auto` | (whisperx) inference device: `auto`, `cpu`, or `cuda`. `auto` picks `cuda` only when an NVIDIA GPU is present, and never on macOS | | `-compute_type` | `auto` | (whisperx) compute type: `auto`, `int8`, `float16`, … . `auto` follows the device: `float16` on CUDA, `int8` on CPU | | `-vad` | `auto` | (whisperx) VAD method: `auto`, `silero`, or `pyannote`. `auto` picks `silero`; `pyannote` fails under newer torch versions | -| `-offset` | derived | audio-to-session clock offset in seconds. When not given: with `-audio`, derived from the recording's creation time minus the manifest's `t0_epoch_ms`, or 0 when derivation is impossible; without it, read back from `audio.offset.json` when the session has one, else 0 | +| `-offset` | derived | audio-to-session clock offset in seconds. When not given: with `-audio`, derived from the recording's creation time minus the manifest's `t0_epoch_ms`, or 0 when derivation is impossible; without it, read back from `audio.offset.json` when the session has one, else 0. A non-finite value, or one beyond ±10⁹ seconds (the bound every derived or persisted offset already meets), is a usage error | Behaviour: with `-audio`, requires ffmpeg on PATH and converts the recording to 16 kHz mono `audio.wav` in the session directory; without it (or when `-audio` points at the session's own `audio.wav`), it uses the existing `audio.wav` in place and skips the conversion. It then runs the engine, applies the offset, and writes `transcript.jsonl`. Always prints the offset it used and its provenance — one of: diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 3563057..c8f4976 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -188,6 +188,17 @@ func Run(args []string) int { offsetSet = true } }) + // An unusable explicit offset is a wrong invocation (exit 2), refused + // before any conversion or engine work starts — the -window precedent + // above. Unchecked, a non-finite -offset failed only after that work + // was already spent, with a bare JSON encoding error at exit 1, and a + // finite but absurd one wrote a transcript at exit 0 that merge + // refuses one command later, naming transcript.jsonl rather than the flag. + if offsetSet { + if err := transcribe.CheckOffset(*offset); err != nil { + return usageErr(fmt.Errorf("transcribe: %w", err)) + } + } n, err := transcribe.Run(transcribe.Options{ SessionDir: *dir, Audio: *audio, diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index f5a9b78..35eb7ce 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -132,6 +132,31 @@ func TestReportRejectsNonFiniteWindow(t *testing.T) { } } +// TestTranscribeRejectsUnusableOffset pins -offset to the same invocation +// contract as -window: a non-finite or over-magnitude value is a wrong +// invocation (exit 2), refused before any conversion or engine work. Unchecked, +// `-offset NaN` failed only after that work was already spent, with a bare +// JSON encoding error at exit 1, and `-offset 1e300` wrote a transcript at +// exit 0 that merge refuses one command later, naming transcript.jsonl +// rather than the flag. The refusal must precede engine detection, so this test +// needs no ASR engine on PATH — on the pre-fix path these invocations instead +// failed with the engine-missing (or JSON encoding) runtime error at exit 1. +func TestTranscribeRejectsUnusableOffset(t *testing.T) { + dir := miniSession(t) + for _, v := range []string{"NaN", "Inf", "-Inf", "1e300"} { + var code int + stderr := captureStderr(t, func() { + code = Run([]string{"transcribe", "-session", dir, "-offset", v}) + }) + if code != 2 { + t.Errorf("-offset %s: exit %d, want 2 (usage error, like -window)", v, code) + } + if !strings.Contains(stderr, "testimony: transcribe: -offset") { + t.Errorf("-offset %s: want the -offset refusal on stderr, got %q", v, stderr) + } + } +} + // TestStrayPositionalIsAUsageError pins the other half of the invocation // contract: no command takes positional arguments (docs/reference/cli.md), and // flag parsing stops at the first non-flag argument, so a stray positional diff --git a/internal/demo/demo.go b/internal/demo/demo.go index baf4dae..4b244e4 100644 --- a/internal/demo/demo.go +++ b/internal/demo/demo.go @@ -71,16 +71,33 @@ const maxBatchBody = 8 << 20 // Run starts the demo capture server on addr, creating a new session // directory under outRoot. It blocks until the process is interrupted. func Run(addr, outRoot string) error { + // Bind before the session directory exists. The CLI's CheckAddr already + // refuses a malformed -addr up front, but a well-formed address can still + // fail to bind (the port is taken, or the host cannot be listened on), and + // creating the directory first left a stray session behind — a manifest + // plus two empty stream files — for a server that never served. Binding is + // the only way to learn the answer, so it goes first; a directory-creation + // failure after it just closes the listener, leaving nothing behind either + // way. + bind, err := listenAddr(addr) + if err != nil { + return err + } + ln, err := net.Listen("tcp", bind) + if err != nil { + return err + } dir, err := session.Create(outRoot, time.Now(), session.Manifest{ App: DefaultApp, Participant: "P1", Tasks: []string{DefaultTask}, }) if err != nil { + ln.Close() return err } - srv, err := Serve(addr, dir) + srv, err := serveOn(ln, addr, dir) if err != nil { return err } @@ -127,24 +144,41 @@ func Shutdown(srv *http.Server) error { // streams into the existing session directory dir. It binds synchronously (so // a bind failure is returned) and then serves in the background, returning the // running *http.Server for the caller to Shutdown. record reuses this to run -// the demo app into the same directory as the recorders. +// the demo app into the same directory as the recorders; demo.Run binds first +// itself (so a refused bind never creates a session directory) and enters at +// serveOn. func Serve(addr, dir string) (*http.Server, error) { - // Resolve the bind address before touching the session directory, so an addr - // that will be refused never leaves empty stream files behind. + // Resolve the bind address and bind it before touching the session + // directory, so an addr that will be refused never leaves empty stream + // files behind. bind, err := listenAddr(addr) if err != nil { return nil, err } + ln, err := net.Listen("tcp", bind) + if err != nil { + return nil, err + } + return serveOn(ln, addr, dir) +} + +// serveOn serves the demo capture app on an already-bound listener, taking +// ownership of it: on any error the listener is closed. addr is the address +// the operator requested, kept only to surface the requested host alongside +// the bound port on the returned server. +func serveOn(ln net.Listener, addr, dir string) (*http.Server, error) { // The interaction shape check needs the manifest's t0 anchor, and every // caller creates the session (and so the manifest) before serving into it. // Loading it here also refuses a session whose anchor merge could never // use, before any capture is accepted against it. man, err := session.LoadManifest(dir) if err != nil { + ln.Close() return nil, err } t0, err := man.T0() if err != nil { + ln.Close() return nil, err } open := func(name string) (*os.File, error) { @@ -152,11 +186,13 @@ func Serve(addr, dir string) (*http.Server, error) { } inter, err := open(session.InteractionsFile) if err != nil { + ln.Close() return nil, err } raw, err := open(session.RawEventsFile) if err != nil { inter.Close() + ln.Close() return nil, err } s := &server{interactions: inter, rawEvents: raw, t0: t0} @@ -170,12 +206,6 @@ func Serve(addr, dir string) (*http.Server, error) { mux.HandleFunc("/api/interactions", s.handleInteraction) mux.HandleFunc("/api/events", s.handleRawEvents) - ln, err := net.Listen("tcp", bind) - if err != nil { - inter.Close() - raw.Close() - return nil, err - } // A deliberately wider bind serves the page to other devices, but allowWrite // still pins capture posts to loopback clients — lifting that pin would // reopen the CSRF/DNS-rebinding surface the guard exists for. The operator @@ -183,8 +213,8 @@ func Serve(addr, dir string) (*http.Server, error) { // which surfaces no status to the page, so without this line the first // signal that a remote participant's session recorded nothing was merge // counting 0 events. - if !loopbackHost(bind) { - fmt.Fprintf(os.Stderr, "testimony demo: warning: bound to %s, but capture posts are accepted from loopback clients only — a page opened from another device is served yet records nothing\n", bind) + if !loopbackHost(ln.Addr().String()) { + fmt.Fprintf(os.Stderr, "testimony demo: warning: bound to %s, but capture posts are accepted from loopback clients only — a page opened from another device is served yet records nothing\n", ln.Addr().String()) } // The two stream files use direct O_APPEND writes (no buffering), so their // data is durable without an explicit Close; the OS reclaims them on exit, diff --git a/internal/demo/demo_test.go b/internal/demo/demo_test.go index 25a3bab..7ada25b 100644 --- a/internal/demo/demo_test.go +++ b/internal/demo/demo_test.go @@ -364,6 +364,32 @@ func TestWriteEndpointGuard(t *testing.T) { } } +// TestRunBindFailureCreatesNoSessionDir pins Run's ordering: the bind comes +// before the session directory is created. A well-formed -addr can still fail +// to bind (most plainly, the port is already taken — a second `testimony demo` +// beside a first), and creating the directory first left a stray session +// behind — manifest plus two empty stream files — for a server that never +// served, the same class the malformed-addr refusal already forecloses. Run +// only blocks after a successful bind, so with the port held it returns. +func TestRunBindFailureCreatesNoSessionDir(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("hold a port: %v", err) + } + defer ln.Close() + out := t.TempDir() + if err := Run(ln.Addr().String(), out); err == nil { + t.Fatal("Run on a taken port: want a bind error, got nil") + } + entries, err := os.ReadDir(out) + if err != nil { + t.Fatalf("read out root: %v", err) + } + if len(entries) != 0 { + t.Errorf("a refused bind created a session directory anyway: %v", entries) + } +} + // TestWiderBindWarnsCaptureStaysLoopback pins the operator signal for the // advertised wider bind: an explicit non-loopback host serves the page to // other devices, but allowWrite still pins capture posts to loopback clients, diff --git a/internal/transcribe/transcribe.go b/internal/transcribe/transcribe.go index b644f13..0a7f2d5 100644 --- a/internal/transcribe/transcribe.go +++ b/internal/transcribe/transcribe.go @@ -319,6 +319,30 @@ const maxOffsetSidecarBytes = 64 << 10 // so the operator learns which input is wrong. const maxOffsetSeconds = 1e9 +// CheckOffset validates an explicit -offset value without touching the +// session, so the CLI can refuse an unusable offset as a usage error before +// any work starts — the CheckEngine/CheckAddr pattern. The derived and +// sidecar paths already refuse an offset beyond maxOffsetSeconds where the +// bad value enters (resolveOffset, readOffsetSidecar); the explicit flag was +// the one unbounded entry point. A non-finite flag failed only after the +// conversion (external, at the sidecar persist) or the whole engine run +// (in place, at the transcript write) was already spent, with a bare JSON +// encoding error at the runtime status; a finite but absurd one wrote a +// transcript at exit 0 that merge refuses one command later — naming +// transcript.jsonl rather than the flag — while the external path persisted +// a sidecar readOffsetSidecar itself refuses on the next bare run. No +// genuine offset is refused: a value past ±1e9 seconds already fails every +// downstream reader. +func CheckOffset(v float64) error { + if math.IsNaN(v) || math.IsInf(v, 0) { + return fmt.Errorf("-offset must be a finite number of seconds, got %v", v) + } + if math.Abs(v) > maxOffsetSeconds { + return fmt.Errorf("-offset %g exceeds %g seconds in magnitude; no audio→session offset is that large", v, maxOffsetSeconds) + } + return nil +} + // writeOffsetSidecar persists offset (with its provenance, for the operator) // beside audio.wav. The write is atomic (temp + rename) as well as // no-follow: a plain truncating write destroyed the prior sidecar's bytes diff --git a/internal/transcribe/transcribe_test.go b/internal/transcribe/transcribe_test.go index 0d222da..2a63365 100644 --- a/internal/transcribe/transcribe_test.go +++ b/internal/transcribe/transcribe_test.go @@ -3,6 +3,7 @@ package transcribe import ( "errors" "io" + "math" "os" "os/exec" "path/filepath" @@ -202,6 +203,23 @@ func TestMapSegmentsNegativeOffset(t *testing.T) { } } +// TestCheckOffset pins the explicit-flag bound: the derived and sidecar paths +// refuse a non-finite or over-magnitude offset where the bad value enters, and +// the flag path must apply the same rule. Every genuine offset (including the +// exact ±1e9 boundary and negative values) stays accepted. +func TestCheckOffset(t *testing.T) { + for _, v := range []float64{math.NaN(), math.Inf(1), math.Inf(-1), 1e300, -1e300, 1e9 + 1} { + if err := CheckOffset(v); err == nil { + t.Errorf("CheckOffset(%v): want a refusal, got nil", v) + } + } + for _, v := range []float64{0, 4.25, -12.4, 1e9, -1e9} { + if err := CheckOffset(v); err != nil { + t.Errorf("CheckOffset(%v): want acceptance, got %v", v, err) + } + } +} + func TestResolveOffsetFlagWins(t *testing.T) { off, prov, err := resolveOffset(Options{Offset: 4.25, OffsetSet: true}, session.Manifest{T0EpochMS: 0}, true) if err != nil {