diff --git a/CHANGELOG.md b/CHANGELOG.md index 8438cf2..aaeefd3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,64 @@ called out in a **Breaking** section. ## [Unreleased] +## [0.3.0] - 2026-07-22 + +A robustness release. No new commands: the pipeline gains no surface, but the +existing one no longer accepts malformed or hostile session input in silence. +Every fix below carries a regression test, and the changes were confirmed by a +multi-round review that finished with two consecutive passes finding nothing. + +### Fixed + +Evidence-record integrity — the report is the artefact of record, so a wrong +number in it is the worst kind of bug: + +- A transcript with missing or duplicate utterance `id`s no longer renders every + event under every utterance. Events attach by position, not by an unvalidated + id. +- A finding, utterance, interaction, or ASR segment that omits its required time + is now rejected rather than silently anchored at the session start (`[00:00]`). + Absent and a genuine zero are distinguished throughout. +- A manifest whose `t0_epoch_ms` is absent or negative is refused wherever a time + is placed on the session clock — `merge` and `transcribe` alike — instead of + shifting every event by roughly the whole Unix epoch and reporting success. +- Pre-`t0` times (a recording that predates the manifest anchor) render with a + signed clock rather than being clamped to `[00:00]`. + +Robustness against malformed or exchanged sessions — a session directory is an +exchange unit and may be attacker-authored: + +- A symlink or FIFO planted at any session artefact is refused on both the read + and the write path, rather than redirecting a write or hanging the CLI in + `open(2)` for ever. +- No writer emits a JSONL line larger than the readers can take back; an + over-long record is refused at the point of capture and of write, not + discovered as a permanently unreadable session later. +- `analyze` no longer truncates a findings file on an empty answer, and no longer + loses a human verdict whose value falls outside the known set. +- Untrusted manifest, transcript, and finding text can no longer inject terminal + escape sequences or forge document structure; the sanitiser now covers the + complete Unicode Bidi_Control set. + +Resource and process lifecycle: + +- `record` and the capture server shut down under a deadline, so a stalled + connection can no longer hang session finalisation after Ctrl+C. +- A partial write (a full disk) leaves no truncated, unreadable line behind in + any append path. +- `record` classifies a start-up permission denial correctly instead of + misreporting it as an unexpected mid-session stop. +- An empty capture address no longer binds the unauthenticated endpoints on + every network interface. + +### Changed + +- Validation is stricter at the pipeline's boundaries. Inputs that violate the + documented session schema — a missing required time, an absent or negative + anchor, an over-long record — are now refused with a clear error where earlier + versions accepted them and produced a silently wrong artefact. Well-formed + sessions, including the bundled sample, are unaffected. + ## [0.2.0] - 2026-07-18 ### Added diff --git a/internal/analyze/analyze.go b/internal/analyze/analyze.go index 85429df..0bdf883 100644 --- a/internal/analyze/analyze.go +++ b/internal/analyze/analyze.go @@ -16,7 +16,6 @@ import ( "bytes" "encoding/json" "fmt" - "os" "path/filepath" "regexp" @@ -84,7 +83,11 @@ func IsFindingID(s string) bool { return findingIDRe.MatchString(s) } // callers can render an absence notice. func Load(dir string) ([]Finding, []Verdict, error) { path := filepath.Join(dir, session.FindingsFile) - f, err := os.Open(path) + // Route through the read-side no-follow guard, not plain os.Open: findings.jsonl + // in an exchanged (attacker-authored) session may be a symlink or a FIFO, and a + // FIFO would block this open in open(2) for ever. A missing file still returns + // an fs.ErrNotExist-satisfying error, which callers render as an absence notice. + f, err := session.OpenFileNoFollowRead(path) if err != nil { return nil, nil, err } diff --git a/internal/analyze/analyze_test.go b/internal/analyze/analyze_test.go index c0af352..69a8fff 100644 --- a/internal/analyze/analyze_test.go +++ b/internal/analyze/analyze_test.go @@ -1,6 +1,7 @@ package analyze import ( + "fmt" "os" "path/filepath" "strings" @@ -433,3 +434,114 @@ func TestIngestRejectsOversizedEvidence(t *testing.T) { t.Fatalf("findings.jsonl was written despite the oversized evidence array") } } + +// TestIngestRejectsFindingWithoutT is the missing-anchor regression: a finding +// that omits "t" must be refused, not filed at the start of the session. Pre-fix +// Finding.T decoded as a value type, so an absent "t" became 0 — indistinguishable +// from a finding genuinely anchored at t=0 — and validate's only check on t +// (the range [0, end] for a normal session) waved it through. Ingest returned no +// error and the finding was written and rendered at [00:00], tens of seconds from +// the utterance at t=22 it quotes. +func TestIngestRejectsFindingWithoutT(t *testing.T) { + dir := writeSession(t, timelineFixture) + answer := `{"rubric":"testimony-analysis/v1","findings":[ + {"id":"F-001","type":"bug","severity":3,"quote":"I clicked save and nothing happened", + "evidence":["utt-004","ev-003"]} + ]}` + _, err := Ingest(dir, strings.NewReader(answer)) + if err == nil || !strings.Contains(err.Error(), "missing t") { + t.Fatalf("expected a missing-t refusal, got %v", err) + } + if _, statErr := os.Stat(filepath.Join(dir, session.FindingsFile)); statErr == nil { + t.Fatalf("findings.jsonl was written despite the finding having no t") + } +} + +// TestIngestAcceptsFindingAtZero is the other half of the missing-t fix: t=0 is +// a legitimate anchor (a finding at the very start of the session), which is why +// the check is a nil-pointer test and not a zero test. A zero test would reject +// this honest finding. +func TestIngestAcceptsFindingAtZero(t *testing.T) { + // utt-004 is moved to t=0 so a finding anchored there is genuinely in range. + zeroTimeline := `{"t":0,"src":"speech","id":"utt-004","payload":{"speaker":"P1","t1":6,"text":"Hm. I clicked save and nothing happened. No message."}} +` + dir := writeSession(t, zeroTimeline) + answer := `{"rubric":"testimony-analysis/v1","findings":[ + {"id":"F-001","t":0,"type":"bug","severity":3,"quote":"I clicked save and nothing happened", + "evidence":["utt-004"]} + ]}` + findings, err := Ingest(dir, strings.NewReader(answer)) + if err != nil { + t.Fatalf("Ingest of a finding anchored at t=0: %v", err) + } + if len(findings) != 1 || findings[0].T != 0 { + t.Fatalf("got %+v, want one finding at t=0", findings) + } +} + +// longIDTimeline returns a timeline holding one utterance whose id is ~2 MiB +// long, and that id. Every line stays under session.MaxJSONLLine, so the +// timeline itself is readable; a finding citing the id a few times nonetheless +// serialises past the limit. Long ids are the way to build such a finding +// without an unreadable fixture: a huge quote cannot do it, because the quote +// must be a verbatim substring of the cited utterance and so would push that +// utterance's own line over the limit first. +func longIDTimeline(t float64, id string) string { + return fmt.Sprintf( + `{"t":%g,"src":"speech","id":%q,"payload":{"speaker":"P1","t1":%g,"text":"I clicked save and nothing happened. No message."}}`+"\n", + t, id, t+6) +} + +// TestIngestRejectsOversizedFindingLine is the findings-brick regression seen +// through serialised size rather than cardinality: maxEvidence bounds how many +// ids a finding cites, but nothing bounded how long those ids are, so a finding +// whose fields are each individually valid still serialised to a findings.jsonl +// line larger than session.MaxJSONLLine. Pre-fix Ingest wrote it and reported +// success, leaving the file permanently unreadable to report, review, and the +// re-ingest recovery path. Nothing may be written, and the refusal must name the +// offending finding and its size. +func TestIngestRejectsOversizedFindingLine(t *testing.T) { + longID := "utt-" + strings.Repeat("x", 2<<20) + dir := writeSession(t, longIDTimeline(22, longID)) + + answer := fmt.Sprintf( + `{"findings":[{"id":"F-001","t":22,"type":"bug","severity":3,"quote":"I clicked save and nothing happened","evidence":[%q,%q,%q]}]}`, + longID, longID, longID) + _, err := Ingest(dir, strings.NewReader(answer)) + if err == nil || !strings.Contains(err.Error(), "exceeding the") { + t.Fatalf("expected an over-long line refusal, got %v", err) + } + if !strings.Contains(err.Error(), "F-001") { + t.Fatalf("refusal does not name the offending finding:\n%v", err) + } + if _, statErr := os.Stat(filepath.Join(dir, session.FindingsFile)); statErr == nil { + t.Fatalf("findings.jsonl was written despite an over-long finding line") + } +} + +// TestIngestOversizedFindingLeavesPriorFileIntact proves the size check is +// transactional: it runs before any write, so an answer whose second finding is +// over-long leaves a prior good findings.jsonl exactly as it was, rather than +// truncating it and then failing. +func TestIngestOversizedFindingLeavesPriorFileIntact(t *testing.T) { + longID := "utt-" + strings.Repeat("x", 2<<20) + dir := writeSession(t, timelineFixture+longIDTimeline(23, longID)) + + if _, err := Ingest(dir, strings.NewReader(goodAnswer)); err != nil { + t.Fatalf("first Ingest: %v", err) + } + path := filepath.Join(dir, session.FindingsFile) + before, _ := os.ReadFile(path) + + answer := fmt.Sprintf(`{"findings":[ + {"id":"F-001","t":22,"type":"bug","severity":3,"quote":"I clicked save and nothing happened","evidence":["utt-004"]}, + {"id":"F-002","t":23,"type":"friction","severity":2,"quote":"No message","evidence":[%q,%q,%q]} + ]}`, longID, longID, longID) + if _, err := Ingest(dir, strings.NewReader(answer)); err == nil || !strings.Contains(err.Error(), "F-002") { + t.Fatalf("expected an over-long line refusal naming F-002, got %v", err) + } + after, _ := os.ReadFile(path) + if string(before) != string(after) { + t.Fatalf("a prior findings.jsonl was modified despite the refusal") + } +} diff --git a/internal/analyze/ingest.go b/internal/analyze/ingest.go index 0837617..8221cc9 100644 --- a/internal/analyze/ingest.go +++ b/internal/analyze/ingest.go @@ -88,13 +88,19 @@ func Ingest(dir string, r io.Reader) ([]Finding, error) { decoded = append(decoded, positioned{finding: f, at: i + 1}) } errs = append(errs, validate(decoded, idx)...) - if len(errs) > 0 { - return nil, errors.Join(errs...) - } + // The model is never trusted: every finding lands unverified. Laundering the + // status here, before the size check below, is what makes that check measure + // the line actually written rather than the one the answer proposed. findings := make([]Finding, len(decoded)) for i, p := range decoded { findings[i] = p.finding + findings[i].Status = "unverified" + } + errs = append(errs, oversizedFindings(findings, decoded)...) + + if len(errs) > 0 { + return nil, errors.Join(errs...) } if held, err := holdsVerdicts(dir); err != nil { @@ -103,16 +109,39 @@ func Ingest(dir string, r io.Reader) ([]Finding, error) { return nil, fmt.Errorf("refusing to overwrite %s: it already holds verdict records (the retained precision record)", session.FindingsFile) } - // The model is never trusted: every finding lands unverified. - for i := range findings { - findings[i].Status = "unverified" - } if err := session.WriteJSONL(filepath.Join(dir, session.FindingsFile), findings); err != nil { return nil, fmt.Errorf("write findings: %w", err) } return findings, nil } +// oversizedFindings reports any finding whose findings.jsonl line — its JSON +// encoding plus the newline WriteJSONL appends — would exceed +// session.MaxJSONLLine, the shared invariant every reader scans to. maxEvidence +// bounds how many ids a finding may cite, but nothing bounds the length of a +// verbatim quote, so a finding with a perfectly valid evidence array and an +// enormous quote still serialises to a line no reader can take back: report, +// review, and the re-ingest recovery path would all fail on the file Ingest had +// just reported writing successfully. The check therefore runs before any write +// and joins the transactional error set, so an over-long finding leaves the +// previous findings.jsonl untouched rather than bricking it. Labels come from +// each finding's answer position for the same reason validate's do. +func oversizedFindings(findings []Finding, decoded []positioned) []error { + var errs []error + for i, f := range findings { + label := findingLabel(f, decoded[i].at) + line, err := json.Marshal(f) + if err != nil { + errs = append(errs, fmt.Errorf("%s: cannot encode as JSON: %w", label, err)) + continue + } + if len(line)+1 > session.MaxJSONLLine { + errs = append(errs, fmt.Errorf("%s: encodes to %d bytes, exceeding the %d-byte %s line limit", label, len(line)+1, session.MaxJSONLLine, session.FindingsFile)) + } + } + return errs +} + // parseContainer accepts either a top-level object with a "findings" array (the // preferred container, optionally carrying a "rubric") or a bare array of // findings. It returns the raw finding elements and the rubric string (empty @@ -146,17 +175,56 @@ func parseContainer(data []byte) ([]json.RawMessage, string, error) { } } +// rawFinding is how one element of the untrusted answer is decoded before it is +// trusted. Its T is a pointer so that an absent "t" stays distinguishable from a +// genuine 0: a finding anchored at the very start of the session legitimately +// carries t equal to 0, so a value-typed field cannot tell the two apart. With +// one, an answer omitting "t" decodes to 0 and sails through validate's range +// check — whose floor is 0 for any normal session — and the finding is filed at +// [00:00] by report and review, tens of seconds from the utterance it quotes, +// with nothing on the record to say the model never placed it. Every other +// required field's absence is already caught by its own rule, so t is the lone +// one needing this. Everything else mirrors Finding, which is the shape +// DisallowUnknownFields is closed against. +type rawFinding struct { + ID string `json:"id"` + T *float64 `json:"t"` + Type string `json:"type"` + Severity int `json:"severity"` + Mode string `json:"mode,omitempty"` + Quote string `json:"quote"` + Evidence []string `json:"evidence"` + UI *UI `json:"ui,omitempty"` + Status string `json:"status"` +} + // decodeFinding strictly decodes one finding element. DisallowUnknownFields // closes the shape: a hallucinated or mistyped field is a hard error rather -// than silently dropped, and it applies to the nested ui object too. +// than silently dropped, and it applies to the nested ui object too. A missing +// "t" is rejected here rather than in validate, because by the time a finding +// reaches validate its unset t is indistinguishable from an honest 0 (see +// rawFinding). func decodeFinding(raw json.RawMessage) (Finding, error) { - var f Finding + var rf rawFinding dec := json.NewDecoder(bytes.NewReader(raw)) dec.DisallowUnknownFields() - if err := dec.Decode(&f); err != nil { - return f, err + if err := dec.Decode(&rf); err != nil { + return Finding{}, err + } + if rf.T == nil { + return Finding{}, fmt.Errorf("missing t; a finding must name the moment it is anchored to") } - return f, nil + return Finding{ + ID: rf.ID, + T: *rf.T, + Type: rf.Type, + Severity: rf.Severity, + Mode: rf.Mode, + Quote: rf.Quote, + Evidence: rf.Evidence, + UI: rf.UI, + Status: rf.Status, + }, nil } // holdsVerdicts reports whether an existing findings.jsonl already contains any @@ -168,7 +236,10 @@ func decodeFinding(raw json.RawMessage) (Finding, error) { // a re-ingest — exactly the precision history the guard exists to protect. func holdsVerdicts(dir string) (bool, error) { path := filepath.Join(dir, session.FindingsFile) - f, err := os.Open(path) + // Read-side no-follow guard rather than plain os.Open: a FIFO planted at + // findings.jsonl in an exchanged session would otherwise hang this open for + // ever. A missing file still satisfies os.ErrNotExist and is not an error here. + f, err := session.OpenFileNoFollowRead(path) if err != nil { if errors.Is(err, os.ErrNotExist) { return false, nil diff --git a/internal/analyze/validate.go b/internal/analyze/validate.go index ecdb2b7..380c7e5 100644 --- a/internal/analyze/validate.go +++ b/internal/analyze/validate.go @@ -95,6 +95,18 @@ func atPositions(findings []Finding) []positioned { return out } +// findingLabel names a finding in an error message: its own id when that id is +// well-formed, and otherwise its position in the answer, which is the only +// handle the operator has on a finding whose id is unusable. Both the schema +// rules and the serialised-size check label through here so an operator reading +// a joined error sees one naming scheme throughout. +func findingLabel(f Finding, at int) string { + if IsFindingID(f.ID) { + return f.ID + } + return fmt.Sprintf("finding #%d", at) +} + // validate runs every schema rule against the decoded findings and returns all // errors (transactional and exhaustive), each naming the finding, the field, // and the offending value. Positional labels come from each finding's recorded @@ -107,9 +119,8 @@ func validate(findings []positioned, idx timelineIndex) []error { for _, p := range findings { f := p.finding - label := f.ID - if !IsFindingID(label) { - label = fmt.Sprintf("finding #%d", p.at) + label := findingLabel(f, p.at) + if !IsFindingID(f.ID) { errs = append(errs, fmt.Errorf("%s: id %q must match ^F-\\d{3}$", label, f.ID)) } else if prev, dup := seen[f.ID]; dup { errs = append(errs, fmt.Errorf("%s: duplicate id (first seen at finding #%d)", f.ID, prev)) diff --git a/internal/record/record.go b/internal/record/record.go index f306013..84524c3 100644 --- a/internal/record/record.go +++ b/internal/record/record.go @@ -51,11 +51,13 @@ var stopGrace = 5 * time.Second var startupWindow = 5 * time.Second // Test seams: overridden in tests to drive the lifecycle without installing a -// real signal handler or spawning ffmpeg. In production they are the real -// implementations. +// real signal handler, spawning ffmpeg, or binding a port for the demo server. +// In production they are the real implementations. var ( notifyContext = defaultNotifyContext startRecordersFn = startRecorders + serveDemoFn = demo.Serve + shutdownDemoFn = demo.Shutdown ) // defaultNotifyContext returns a context cancelled on SIGINT/SIGTERM/SIGHUP — @@ -142,7 +144,7 @@ func Run(opts Options) error { var srv *http.Server if opts.Demo { - srv, err = demo.Serve(opts.Addr, dir) + srv, err = serveDemoFn(opts.Addr, dir) if err != nil { stopAll(children) return fmt.Errorf("start demo server: %w", err) @@ -176,16 +178,12 @@ func Run(opts Options) error { // device fault instead of the permission they had never granted. atStartup := time.Since(dead.started) < startupWindow stopAll(children) - if srv != nil { - _ = srv.Shutdown(context.Background()) - } + stopDemo(srv) return errors.New(classifyRecorderExit(dead.stream, dead.err, dead.stderr.tail(), atStartup)) } stopAll(children) - if srv != nil { - _ = srv.Shutdown(context.Background()) - } + stopDemo(srv) // Finalisation validates that each recorder actually left a usable artefact. // A recorder blocked on its TCC prompt for the whole session finalises no @@ -202,6 +200,25 @@ func Run(opts Options) error { return nil } +// stopDemo stops the demo capture server, when one is running, through demo's +// bounded shutdown helper. Both of Run's exit paths — the Ctrl+C branch and the +// recorder-exited branch — go through this one function so neither can drift +// back to stopping the server itself: srv.Shutdown with a context that never +// expires blocks for as long as any connection stays open, so a single stalled +// browser tab left `testimony record -demo` hanging on Ctrl+C instead of +// finalising the session. That is the hang demo.Shutdown exists to prevent, and +// it is only prevented where the helper is actually used. +func stopDemo(srv *http.Server) { + if srv == nil { + return + } + // The shutdown error is deliberately dropped: the two capture streams use + // direct O_APPEND writes, so records already accepted are durable whether the + // drain completed or the deadline forced a Close, and the session must still + // be finalised either way. + _ = shutdownDemoFn(srv) +} + // finaliseOutputs validates each stopped recorder's expected artefact and turns // any that produced nothing into an actionable explanation. It reports whether a // usable audio.wav is present, so the Next block can decide whether to offer diff --git a/internal/record/record_test.go b/internal/record/record_test.go index bc22c23..7513c71 100644 --- a/internal/record/record_test.go +++ b/internal/record/record_test.go @@ -5,6 +5,7 @@ import ( "context" "errors" "io" + "net/http" "os" "os/exec" "path/filepath" @@ -16,6 +17,7 @@ import ( "testing" "time" + "github.com/REPPL/Testimony/internal/demo" "github.com/REPPL/Testimony/internal/session" ) @@ -578,6 +580,107 @@ func TestRunReportsRecorderStoppedWithNoOutput(t *testing.T) { } } +// TestRunStopsDemoServerThroughBoundedShutdown proves that BOTH of Run's exit +// paths — the Ctrl+C branch and the branch a recorder taken on its own drives — +// stop the demo capture server through demo.Shutdown, the helper that carries a +// deadline and a Close fallback. Pre-fix each site called +// srv.Shutdown(context.Background()) directly, so `testimony record -demo` never +// received the bound at all: an unbounded stop waits for as long as any +// connection stays open, and one stalled browser tab left Ctrl+C hanging instead +// of finalising the session. +// +// The deadline itself belongs to demo and is proven there; what regressed, and +// what this test pins, is that record routes both call sites through it rather +// than stopping the server itself — so the assertion is deliberately the narrower +// "the bounded helper was called, once, on each path". Hermetic: the demo serve +// and shutdown seams are stubbed, so no port is bound and no ffmpeg runs. +func TestRunStopsDemoServerThroughBoundedShutdown(t *testing.T) { + // The production seam must be the bounded helper, not some unbounded stop the + // stub below would happily stand in for. + if reflect.ValueOf(shutdownDemoFn).Pointer() != reflect.ValueOf(demo.Shutdown).Pointer() { + t.Fatal("record must stop the demo server through demo.Shutdown, the bounded helper") + } + + // interrupted drives the Ctrl+C branch; recorderDied drives the branch taken + // when a recorder exits on its own. Both must stop the server. + cases := []struct { + name string + interrupt bool + wantErrors bool + }{ + {name: "interrupted", interrupt: true, wantErrors: false}, + {name: "recorder died", interrupt: false, wantErrors: true}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + origNotify, origStart := notifyContext, startRecordersFn + origServe, origShutdown := serveDemoFn, shutdownDemoFn + t.Cleanup(func() { + notifyContext, startRecordersFn = origNotify, origStart + serveDemoFn, shutdownDemoFn = origServe, origShutdown + }) + + // A server value that was never listened on: stopDemo only has to reach + // the seam, and binding a real port would make the test racy. + stub := &http.Server{} + serveDemoFn = func(addr, dir string) (*http.Server, error) { return stub, nil } + + var mu sync.Mutex + var stopped []*http.Server + shutdownDemoFn = func(srv *http.Server) error { + mu.Lock() + stopped = append(stopped, srv) + mu.Unlock() + return nil + } + + var cancel context.CancelFunc + notifyContext = func() (context.Context, context.CancelFunc) { + ctx, cf := context.WithCancel(context.Background()) + cancel = cf + return ctx, cf + } + startRecordersFn = func(dir string, streams []string) ([]*liveChild, error) { + child := newLiveChild(streamMicrophone, newFakeProc(syscall.SIGINT), &lockedBuffer{}) + if c.interrupt { + // A well-behaved recorder that finalised a real audio.wav, so the + // Ctrl+C path completes without any finalise problem. + if err := os.WriteFile(filepath.Join(dir, session.AudioFile), []byte("RIFF...."), 0o644); err != nil { + t.Fatal(err) + } + cancel() + } else { + // The recorder exits on its own, sending Run down the other branch. + _ = child.p.Signal(syscall.SIGINT) + } + return []*liveChild{child}, nil + } + + err := Run(Options{ + Out: t.TempDir(), + Participant: "Bob", + Demo: true, + Addr: "127.0.0.1:8737", + GOOS: "darwin", + Log: io.Discard, + }) + if c.wantErrors && err == nil { + t.Fatal("a recorder exiting on its own must make Run exit non-zero") + } + if !c.wantErrors && err != nil { + t.Fatalf("Run must stop cleanly on interrupt: %v", err) + } + + mu.Lock() + got := append([]*http.Server(nil), stopped...) + mu.Unlock() + if len(got) != 1 || got[0] != stub { + t.Fatalf("the demo server must be stopped exactly once through the bounded helper, got %d call(s)", len(got)) + } + }) + } +} + // --- honest degradation --- // TestRunDegradesHonestly proves that on a platform without capture support and diff --git a/internal/report/report.go b/internal/report/report.go index 2d40ac6..fd66f32 100644 --- a/internal/report/report.go +++ b/internal/report/report.go @@ -166,24 +166,45 @@ func findingAnchor(f analyze.Finding) string { func end(entries []timeline.Entry) float64 { var max float64 - for _, e := range entries { + for i, e := range entries { t := e.T if e.Src == "speech" { t = timeline.SpeechEnd(e) } - if t > max { + // Seed the maximum from the first entry, exactly as analyze.indexTimeline + // seeds idx.end, rather than growing it from the zero value: a session whose + // recording predates the manifest t0 has every entry at a negative + // session-relative time, and a zero-seeded maximum would report that + // session's span as 00:00 instead of where it truly ends. + if i == 0 || t > max { max = t } } return max } +// clock renders a session-relative time as a signed MM:SS stamp. Negative times +// are legitimate and deliberately supported — an external recording whose +// creation_time predates the manifest t0 yields a negative offset, so +// transcript, timeline and findings can all carry pre-t0 anchors — and the +// earlier clamp to zero made every one of them print as [00:00], so report.md +// silently misstated when they happened. The sign is split off before the +// arithmetic because %02d over a negative integer division renders garbage such +// as "-1:-30". func clock(sec float64) string { - if sec < 0 { - sec = 0 + neg := sec < 0 + if neg { + sec = -sec } s := int(sec + 0.5) - return fmt.Sprintf("%02d:%02d", s/60, s%60) + sign := "" + // The sign is taken from the rounded value, not the raw one, so a time a + // fraction of a second before t0 prints as 00:00 rather than the nonsense + // "-00:00". + if neg && s > 0 { + sign = "-" + } + return fmt.Sprintf("%s%02d:%02d", sign, s/60, s%60) } func speaker(u timeline.Entry) string { diff --git a/internal/report/report_test.go b/internal/report/report_test.go index 6f83ce4..2a68011 100644 --- a/internal/report/report_test.go +++ b/internal/report/report_test.go @@ -170,6 +170,73 @@ func TestReportAttachesEventsPerUtteranceWithoutIDs(t *testing.T) { } } +// TestReportRendersNegativeSessionRelativeTimes is the clamped-clock regression. +// A recording whose creation_time predates the manifest t0 yields a negative +// offset, so utterances, events and findings legitimately sit before t0 — +// analyze.indexTimeline admits findings anchored there. Pre-fix report.clock +// clamped every negative time to zero and report.end grew its maximum from the +// zero value, so this fully pre-t0 session rendered every line as [00:00] and +// claimed a duration of 00:00 rather than its true span ending at -35 s. +func TestReportRendersNegativeSessionRelativeTimes(t *testing.T) { + dir := t.TempDir() + if err := session.SaveManifest(dir, session.Manifest{Session: "fixture", App: "app", Participant: "Alice"}); err != nil { + t.Fatalf("SaveManifest: %v", err) + } + // Every entry precedes t0: an utterance spanning -90 s to -35 s, one click + // inside its window, and one standalone click well before it. + tl := `{"t":-125,"src":"event","id":"ev-001","payload":{"kind":"click","selector":"early"}} +{"t":-90,"src":"speech","id":"utt-001","payload":{"speaker":"Alice","t1":-35,"text":"before the clock started"}} +{"t":-88,"src":"event","id":"ev-002","payload":{"kind":"click","selector":"attached"}} +` + if err := os.WriteFile(filepath.Join(dir, session.TimelineFile), []byte(tl), 0o644); err != nil { + t.Fatalf("write timeline: %v", err) + } + findings := "{\"id\":\"F-001\",\"t\":-90,\"type\":\"bug\",\"severity\":3,\"quote\":\"before the clock\",\"evidence\":[\"utt-001\"],\"status\":\"unverified\"}\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) + } + for _, want := range []string{ + "[-02:05]", // the standalone event at -125 s + "[-01:30]", // the utterance at -90 s + "[-01:28]", // the event attached to it + "**Duration:** -00:35", + } { + if !strings.Contains(md, want) { + t.Fatalf("report missing signed clock %q:\n%s", want, md) + } + } + if strings.Contains(md, "[00:00]") { + t.Fatalf("a pre-t0 time was clamped to zero:\n%s", md) + } +} + +// TestReportClockRoundsSymmetrically guards the sign-splitting arithmetic in +// clock: a negative time must round by magnitude the way its positive twin +// does, so the digits never disagree across the sign. +func TestReportClockRoundsSymmetrically(t *testing.T) { + for _, tc := range []struct { + sec float64 + want string + }{ + {0, "00:00"}, + {-0.4, "00:00"}, + {-0.6, "-00:01"}, + {61.5, "01:02"}, + {-61.5, "-01:02"}, + {-90, "-01:30"}, + {-3600, "-60:00"}, + } { + if got := clock(tc.sec); got != tc.want { + t.Fatalf("clock(%g) = %q, want %q", tc.sec, got, tc.want) + } + } +} + 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 8aca0ce..4098e71 100644 --- a/internal/review/review.go +++ b/internal/review/review.go @@ -233,37 +233,74 @@ func AppendVerdict(dir string, v analyze.Verdict) error { if err != nil { return err } - rec := append(b, '\n') - // A findings.jsonl need not end in a newline: it may have been hand edited, - // produced by another tool, or left short by a crash part-way through an - // earlier write. Appending blindly would fuse the verdict onto that - // unterminated final line, producing one physical line holding two JSON - // objects — which makes not just those two records but the entire file - // unparseable to every reader. So probe the last byte and open a fresh line - // when it is not already one. (O_APPEND still puts the write at the end, - // whatever the seek offset; the seek is only to learn the size.) + if err := writeVerdict(f, append(b, '\n')); err != nil { + f.Close() + return err + } + // 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() +} + +// verdictFile is the subset of *os.File that writeVerdict needs; a fake +// satisfies it in tests to exercise the partial-write rollback. +type verdictFile interface { + io.Writer + io.ReaderAt + Seek(offset int64, whence int) (int64, error) + Truncate(size int64) error +} + +// writeVerdict frames rec so it lands as its own physical line and writes it, +// rolling the file back if the write only partly lands. +// +// A findings.jsonl need not end in a newline: it may have been hand edited, +// produced by another tool, or left short by a crash part-way through an +// earlier write. Appending blindly would fuse the verdict onto that +// unterminated final line, producing one physical line holding two JSON +// objects — which makes not just those two records but the entire file +// unparseable to every reader. So probe the last byte and open a fresh line +// when it is not already one. (O_APPEND still puts the write at the end, +// whatever the seek offset; the seek is only to learn the size.) +// +// The write itself needs the same rollback the capture path has in +// demo.appendRecords: os.File.Write gives no atomicity guarantee, so a full +// disk fills the remaining space, returns a short count, and leaves a +// truncated, newline-less fragment (e.g. `{"kind":"verdict","find`) behind. +// That fragment would fuse with the next successful write into one malformed +// physical line and make the whole findings.jsonl — the human verdict record +// this package exists to protect — unparseable to every reader. So on any +// write error the file is truncated back to the length it had immediately +// before the write. The size is re-measured at that point rather than reusing +// the offset the newline probe learned: the descriptor is O_APPEND, so the +// write lands at the true end of file, which a concurrent appender (a second +// `testimony review`, or an analyze ingest) may have moved on since the probe. +// Truncating to the stale offset would delete that other writer's record +// instead of only our own partial bytes. +func writeVerdict(f verdictFile, rec []byte) error { end, err := f.Seek(0, io.SeekEnd) if err != nil { - f.Close() return err } if end > 0 { var last [1]byte if _, err := f.ReadAt(last[:], end-1); err != nil { - f.Close() return err } if last[0] != '\n' { rec = append([]byte{'\n'}, rec...) } } + before, err := f.Seek(0, io.SeekEnd) + if err != nil { + return err + } if _, err := f.Write(rec); err != nil { - f.Close() + // Best-effort roll back any partial bytes; surface the original error. + f.Truncate(before) return err } - // 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() + return nil } // printFinding writes a finding to the analyst's terminal. Every @@ -320,10 +357,25 @@ func readLine(r *bufio.Reader) (string, error) { return line, nil } +// clock renders a session-relative time for the review prompt. Negative times +// are legitimate — an external recording whose creation_time predates the +// manifest t0 yields a negative offset, and analyze.indexTimeline deliberately +// admits findings anchored there — so the sign is rendered rather than clamped +// away. Clamping showed the analyst 00:00 for a pre-t0 finding, the wrong +// moment on the very surface where they record the verdict. This mirrors +// report.clock; see the note in review_test.go about the duplication. func clock(sec float64) string { - if sec < 0 { - sec = 0 + neg := sec < 0 + if neg { + sec = -sec } s := int(sec + 0.5) - return fmt.Sprintf("%02d:%02d", s/60, s%60) + sign := "" + // The sign is taken from the rounded value, not the raw one, so a time a + // fraction of a second before t0 prints as 00:00 rather than the nonsense + // "-00:00". + if neg && s > 0 { + sign = "-" + } + return fmt.Sprintf("%s%02d:%02d", sign, s/60, s%60) } diff --git a/internal/review/review_test.go b/internal/review/review_test.go index 32359b6..0bcfe36 100644 --- a/internal/review/review_test.go +++ b/internal/review/review_test.go @@ -3,6 +3,8 @@ package review import ( "bytes" "encoding/json" + "errors" + "io" "os" "path/filepath" "strings" @@ -320,6 +322,93 @@ func TestAppendVerdictTerminatesAnUnterminatedLastLine(t *testing.T) { } } +// shortWriteFile is a verdictFile whose Write persists a prefix and then errors, +// standing in for a full disk (write(2) fills the remaining space, returns a +// short count, and the next write returns ENOSPC — os.File.Write persists the +// truncated prefix before returning the error). Seek always reports the current +// length, so it also stands in for the O_APPEND descriptor writeVerdict holds. +type shortWriteFile struct { + buf []byte + fail bool // when true, Write keeps only a prefix then returns an error +} + +func (f *shortWriteFile) Seek(offset int64, whence int) (int64, error) { + return int64(len(f.buf)), nil +} + +func (f *shortWriteFile) ReadAt(p []byte, off int64) (int, error) { + if off < 0 || off >= int64(len(f.buf)) { + return 0, io.EOF + } + return copy(p, f.buf[off:]), nil +} + +func (f *shortWriteFile) Truncate(size int64) error { + f.buf = f.buf[:size] + return nil +} + +func (f *shortWriteFile) Write(p []byte) (int, error) { + if f.fail { + half := len(p) / 2 + f.buf = append(f.buf, p[:half]...) + return half, errors.New("no space left on device") + } + f.buf = append(f.buf, p...) + return len(p), nil +} + +// TestWriteVerdictRollsBackPartialWrite is the ENOSPC regression on the verdict +// path: a short write that persists a newline-less prefix must be truncated +// away, so findings.jsonl never retains a partial line that would fuse with the +// next verdict into one malformed physical record and make the whole file — the +// human verdict record the package exists to protect — unparseable to every +// reader. Pre-fix AppendVerdict wrote directly with no rollback, so the prefix +// survived, exactly as demo.appendLines did before its own fix. +func TestWriteVerdictRollsBackPartialWrite(t *testing.T) { + f := &shortWriteFile{} + first, err := json.Marshal(analyze.Verdict{Kind: "verdict", Finding: "F-001", Verdict: "confirmed", At: "2026-07-17"}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if err := writeVerdict(f, append(first, '\n')); err != nil { + t.Fatalf("first verdict: %v", err) + } + good := string(f.buf) + + f.fail = true + second, err := json.Marshal(analyze.Verdict{Kind: "verdict", Finding: "F-002", Verdict: "rejected", At: "2026-07-17"}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if err := writeVerdict(f, append(second, '\n')); err == nil { + t.Fatalf("expected a write error on a full disk") + } + if string(f.buf) != good { + t.Fatalf("partial line survived: file is %q, want the clean prefix %q", f.buf, good) + } + if !strings.HasSuffix(string(f.buf), "\n") { + t.Fatalf("file does not end on a newline: %q", f.buf) + } + + // The rolled-back file still parses one record per line, so a later verdict + // lands cleanly rather than fusing onto a fragment. + f.fail = false + if err := writeVerdict(f, append(second, '\n')); err != nil { + t.Fatalf("verdict after rollback: %v", err) + } + lines := strings.Split(strings.TrimRight(string(f.buf), "\n"), "\n") + if len(lines) != 2 { + t.Fatalf("got %d lines, want 2: %q", len(lines), f.buf) + } + for i, l := range lines { + var v analyze.Verdict + if err := json.Unmarshal([]byte(l), &v); err != nil { + t.Fatalf("line %d is not one JSON record: %v (%q)", i+1, err, l) + } + } +} + // TestAppendVerdictRefusesSymlink is the arbitrary-file-append regression: a // findings.jsonl planted as a symlink must not be followed. func TestAppendVerdictRefusesSymlink(t *testing.T) { @@ -339,3 +428,52 @@ func TestAppendVerdictRefusesSymlink(t *testing.T) { t.Fatalf("victim file appended through symlink: %q", b) } } + +// TestPrintFindingRendersNegativeSessionRelativeTimes is the clamped-clock +// regression, the sibling of the one fixed in internal/report. A recording +// whose creation_time predates the manifest t0 yields a negative offset, and +// analyze.indexTimeline admits findings anchored there. Pre-fix review.clock +// clamped every negative time to zero, so the review prompt stamped a pre-t0 +// finding as [00:00] — the wrong moment, shown on the surface where the analyst +// actually records the verdict. +func TestPrintFindingRendersNegativeSessionRelativeTimes(t *testing.T) { + f := analyze.Finding{ + ID: "F-001", Type: "bug", Severity: 3, T: -90, + Quote: "before the clock started", + UI: &analyze.UI{Selector: "btn", Route: "#x"}, + } + var buf bytes.Buffer + printFinding(&buf, f) + if !strings.Contains(buf.String(), "[-01:30]") { + t.Fatalf("review prompt missing signed clock [-01:30]:\n%s", buf.String()) + } + if strings.Contains(buf.String(), "[00:00]") { + t.Fatalf("a pre-t0 finding time was clamped to zero:\n%s", buf.String()) + } +} + +// TestClockRoundsSymmetrically guards the sign-splitting arithmetic in clock: a +// negative time must round by magnitude the way its positive twin does, so the +// digits never disagree across the sign and a time a fraction of a second +// before t0 never prints the nonsense "-00:00". The expectations match +// report.clock deliberately — the two helpers are byte-identical, and if a +// third caller ever needs one they belong in a shared home (a small formatting +// helper in internal/session, alongside SafeText) rather than a third copy. +func TestClockRoundsSymmetrically(t *testing.T) { + for _, tc := range []struct { + sec float64 + want string + }{ + {0, "00:00"}, + {-0.4, "00:00"}, + {-0.6, "-00:01"}, + {61.5, "01:02"}, + {-61.5, "-01:02"}, + {-90, "-01:30"}, + {-3600, "-60:00"}, + } { + if got := clock(tc.sec); got != tc.want { + t.Fatalf("clock(%g) = %q, want %q", tc.sec, got, tc.want) + } + } +} diff --git a/internal/session/session.go b/internal/session/session.go index 8af248e..6dc20db 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -19,6 +19,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "os" "path/filepath" "strings" @@ -82,10 +83,57 @@ func Create(outRoot string, now time.Time, m Manifest) (dir string, err error) { return dir, nil } +// ErrNoT0 reports a manifest that carries no usable capture anchor. It is +// returned by Manifest.T0 and is worth matching with errors.Is when a caller +// wants to distinguish "this session cannot be placed on a wall clock" from an +// unreadable or malformed manifest. +var ErrNoT0 = errors.New("manifest is missing t0_epoch_ms") + +// T0 returns the session's anchor instant in epoch milliseconds, or ErrNoT0 +// when the manifest carries none. Every caller that converts epoch-ms times to +// session-relative ones — timeline.BuildEntries, transcribe's audio-offset +// derivation — must obtain t0 through here rather than reading the field +// directly. +// +// The check is needed because T0EpochMS is a value-typed int64: a manifest that +// simply omits t0_epoch_ms decodes to 0, which is indistinguishable from a +// recorded zero and is then subtracted from real epoch-ms timestamps. That +// places every event about fifty-seven years into the session and writes a +// silently corrupt timeline — wrong, plausible-looking numbers, which is worse +// than a refusal, because a report built on them reads as evidence. +// +// Treating 0 as absent is safe rather than merely convenient: a genuine +// t0_epoch_ms of 0 is midnight on 1 January 1970, which is not a capture +// instant any recorder can produce. Create derives t0 from the same now that +// names the session directory, so every manifest this tool writes has one. +// Negative values are refused on the same reasoning — they anchor the session +// before the epoch, and no capture starts there. +// +// The check deliberately lives here and not in LoadManifest. Several consumers +// legitimately load a manifest they need no anchor from — report.Render works +// from an already session-relative timeline.jsonl, and analyze.EmitRequest +// reads only the app, participant, and task context — so refusing at load time +// would fail commands that have no use for t0 and no way to be wrong about it. +func (m Manifest) T0() (int64, error) { + if m.T0EpochMS <= 0 { + return 0, fmt.Errorf("%w (session %q); cannot place epoch-millisecond times on the session clock", ErrNoT0, m.Session) + } + return m.T0EpochMS, nil +} + // LoadManifest reads manifest.json from dir. func LoadManifest(dir string) (Manifest, error) { var m Manifest - b, err := os.ReadFile(filepath.Join(dir, ManifestFile)) + // Read through the no-follow guard rather than os.ReadFile: manifest.json in + // an exchanged session is attacker-controllable, and a FIFO planted there + // would block os.ReadFile in open(2) for ever waiting for a writer, hanging + // any command that loads the manifest. + f, err := OpenFileNoFollowRead(filepath.Join(dir, ManifestFile)) + if err != nil { + return m, fmt.Errorf("load manifest: %w", err) + } + defer f.Close() + b, err := io.ReadAll(f) if err != nil { return m, fmt.Errorf("load manifest: %w", err) } @@ -104,31 +152,41 @@ func SaveManifest(dir string, m Manifest) error { return WriteFileNoFollow(filepath.Join(dir, ManifestFile), append(b, '\n'), 0o644) } -// OpenFileNoFollow opens path for writing with O_NOFOLLOW, so a symlink planted -// at the final path component is refused rather than followed, and refuses any -// opened path that is not a regular file. A session directory is an exchange -// unit (a shared or downloaded session may be attacker-authored); without the -// symlink guard a planted symlink — e.g. a timeline.jsonl pointing at -// ~/.ssh/authorized_keys — would redirect a write to an arbitrary file outside +// openNoFollow is the single symlink-and-regular-file guard shared by every +// session-artefact open, read or write. It opens path with O_NOFOLLOW, so a +// symlink planted at the final path component is refused rather than followed, +// and refuses any opened path that is not a regular file. A session directory is +// an exchange unit (a shared or downloaded session may be attacker-authored); +// without the symlink guard a planted symlink — e.g. a timeline.jsonl pointing +// at a private key file — would redirect a write to an arbitrary file outside // the session directory, and without the regular-file guard a FIFO planted at -// the same path would hold the write open for ever, waiting for a reader that -// never arrives, hanging merge or report on a session the operator merely -// received. O_NONBLOCK is set so that the FIFO open itself returns instead of -// blocking before the check can run; it has no effect on the ordinary case, -// because opening a regular file never blocks and the flag does not alter -// subsequent reads or writes on one. flag is OR-ed with O_NOFOLLOW and -// O_NONBLOCK; callers pass the usual O_CREATE/O_TRUNC/O_APPEND/O_WRONLY set. -func OpenFileNoFollow(path string, flag int, perm os.FileMode) (*os.File, error) { +// the same path would hang the CLI in open(2) for ever: on the write side +// waiting for a reader that never arrives, and on the read side waiting for a +// writer that never arrives, so merge, report, or analyze never returns on a +// session the operator merely received. +// +// O_NONBLOCK is what makes the regular-file check reachable at all: opening a +// FIFO — for reading or for writing — normally blocks until the other end is +// present, but with O_NONBLOCK the open returns immediately (a read-only FIFO +// open succeeds at once; a write-only one fails with ENXIO), so fstat can then +// run and refuse it. It has no effect on the ordinary case, because opening a +// regular file never blocks and the flag does not alter subsequent reads or +// writes on one. flag is OR-ed with O_NOFOLLOW and O_NONBLOCK. +// +// verb ("read"/"write") is woven into the refusal messages so they name the +// direction; OpenFileNoFollow keeps verb="write" verbatim so callers and tests +// that assert "refusing to write ..." are undisturbed. +func openNoFollow(path string, flag int, perm os.FileMode, verb string) (*os.File, error) { f, err := os.OpenFile(path, flag|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, perm) if err != nil { if errors.Is(err, syscall.ELOOP) { - return nil, fmt.Errorf("refusing to write %s: it is a symlink", path) + return nil, fmt.Errorf("refusing to %s %s: it is a symlink", verb, path) } return nil, err } // Stat the descriptor rather than the path, so the answer describes the file // that was actually opened and cannot be swapped between the check and the - // write. + // read or write. fi, err := f.Stat() if err != nil { f.Close() @@ -136,11 +194,30 @@ func OpenFileNoFollow(path string, flag int, perm os.FileMode) (*os.File, error) } if !fi.Mode().IsRegular() { f.Close() - return nil, fmt.Errorf("refusing to write %s: it is not a regular file", path) + return nil, fmt.Errorf("refusing to %s %s: it is not a regular file", verb, path) } return f, nil } +// OpenFileNoFollow opens path for writing under the shared openNoFollow guard, +// refusing a planted symlink or non-regular file (see openNoFollow for the full +// threat model). Callers pass the usual O_CREATE/O_TRUNC/O_APPEND/O_WRONLY set; +// O_NOFOLLOW and O_NONBLOCK are added by the guard. +func OpenFileNoFollow(path string, flag int, perm os.FileMode) (*os.File, error) { + return openNoFollow(path, flag, perm, "write") +} + +// OpenFileNoFollowRead opens path read-only under the same guard, so the read +// side of the pipeline is protected too: a FIFO planted at timeline.jsonl, +// interactions.jsonl, transcript.jsonl, findings.jsonl, or manifest.json in an +// exchanged session is refused immediately rather than blocking ReadJSONL or +// LoadManifest in open(2) for ever, and a symlink is refused rather than +// followed out of the session directory. The caller owns the returned file and +// must Close it. +func OpenFileNoFollowRead(path string) (*os.File, error) { + return openNoFollow(path, os.O_RDONLY, 0, "read") +} + // WriteFileNoFollow is os.WriteFile that refuses to follow a symlink at path // (see OpenFileNoFollow). It truncates an existing regular file, as os.WriteFile // does. @@ -201,7 +278,11 @@ const MaxJSONLLine = 4 << 20 // 4 MiB // ReadJSONL decodes a JSON-Lines file into a slice of T. Blank lines are // skipped. A missing file is an error; an empty file yields an empty slice. func ReadJSONL[T any](path string) ([]T, error) { - f, err := os.Open(path) + // Open through the no-follow guard rather than os.Open: a session's JSONL + // artefacts are attacker-controllable when the session was exchanged, and a + // FIFO planted at one would block os.Open in open(2) for ever waiting for a + // writer, hanging merge, report, or analyze on a session merely received. + f, err := OpenFileNoFollowRead(path) if err != nil { return nil, err } @@ -236,7 +317,39 @@ func ReadJSONL[T any](path string) ([]T, error) { // symlink at path (see OpenFileNoFollow) so writing a session artefact — even // 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. 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 := json.NewEncoder(&buf) + for i, v := range values { + buf.Reset() + if err := check.Encode(v); err != nil { + return err + } + // Encode's output already ends in the newline, and that newline counts: + // ReadJSONL's bufio.Scanner buffer caps at MaxJSONLLine bytes and must + // hold the record *and* its terminator to find the line end, so a record + // is readable when its bytes including the newline fit within the limit — + // one byte less payload than the constant's face value. demo's + // tooLongForJSONL draws the boundary on the same side, so the capture and + // artefact writers accept exactly the same set of records. + if buf.Len() > MaxJSONLLine { + return fmt.Errorf("%s: record %d encodes to %d bytes, over the %d-byte JSONL line limit", path, i, buf.Len(), MaxJSONLLine) + } + } + f, err := OpenFileNoFollow(path, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0o644) if err != nil { return err diff --git a/internal/session/session_test.go b/internal/session/session_test.go index eb6b8bd..7703ba2 100644 --- a/internal/session/session_test.go +++ b/internal/session/session_test.go @@ -1,8 +1,10 @@ package session import ( + "errors" "os" "path/filepath" + "strings" "syscall" "testing" "time" @@ -96,6 +98,78 @@ func TestReadJSONLSkipsWhitespaceOnlyLine(t *testing.T) { } } +// TestManifestT0RejectsAbsentAnchor is the epoch-anchored-session regression: a +// manifest that omits t0_epoch_ms decodes to the int64 zero value, which pre-fix +// was read straight out of the field and subtracted from real epoch-millisecond +// event times — anchoring the whole session at 1 January 1970 and placing every +// event about fifty-seven years in, as plausible-looking numbers in a written +// timeline. Manifest.T0 must refuse the absent anchor instead of handing back 0. +func TestManifestT0RejectsAbsentAnchor(t *testing.T) { + dir := t.TempDir() + // Write the manifest by hand, with no t0_epoch_ms at all: this is what an + // exchanged or hand-edited session looks like, and the case SaveManifest of a + // zero-valued struct cannot distinguish from a recorded zero. + body := `{"session":"2026-07-17_153045","app":"settings prototype","participant":"Alice"}` + "\n" + if err := os.WriteFile(filepath.Join(dir, ManifestFile), []byte(body), 0o644); err != nil { + t.Fatalf("write manifest: %v", err) + } + + // Loading still succeeds: report and analyze need the context fields and no + // anchor, so the refusal belongs at the point of use, not at load. + m, err := LoadManifest(dir) + if err != nil { + t.Fatalf("LoadManifest: %v", err) + } + if m.App != "settings prototype" { + t.Fatalf("context fields lost: %+v", m) + } + + t0, err := m.T0() + if err == nil { + t.Fatalf("T0 returned %d for a manifest with no t0_epoch_ms; want refusal", t0) + } + if !errors.Is(err, ErrNoT0) { + t.Fatalf("T0 error does not match ErrNoT0: %v", err) + } + if t0 != 0 { + t.Fatalf("T0 returned %d alongside its error; want 0", t0) + } +} + +// TestManifestT0RejectsPreEpochAnchor covers the same anchor guard for a +// negative t0_epoch_ms, which no recorder can produce and which pre-fix would +// have been used as a real anchor, pushing every event past the end of the +// session instead of before its start. +func TestManifestT0RejectsPreEpochAnchor(t *testing.T) { + m := Manifest{Session: "s", T0EpochMS: -1} + if t0, err := m.T0(); err == nil { + t.Fatalf("T0 accepted a pre-epoch anchor, returning %d; want refusal", t0) + } +} + +// TestManifestT0AcceptsRecordedAnchor is the other half of the guard: a session +// created by Create carries a real t0, and T0 must hand back exactly the value +// Create recorded, so the refusal above cannot be satisfied by rejecting +// everything. +func TestManifestT0AcceptsRecordedAnchor(t *testing.T) { + now := time.Date(2026, 7, 17, 15, 30, 45, 0, time.UTC) + dir, err := Create(t.TempDir(), now, Manifest{App: "settings prototype"}) + if err != nil { + t.Fatalf("Create: %v", err) + } + m, err := LoadManifest(dir) + if err != nil { + t.Fatalf("LoadManifest: %v", err) + } + t0, err := m.T0() + if err != nil { + t.Fatalf("T0 on a recorded session: %v", err) + } + if t0 != now.UnixMilli() { + t.Fatalf("T0: got %d, want %d", t0, now.UnixMilli()) + } +} + // TestWriteJSONLRefusesSymlink is the arbitrary-file-overwrite regression: a // session artefact planted as a symlink (e.g. in a downloaded session) must not // be followed, so the write cannot escape the session directory. Pre-fix @@ -184,6 +258,181 @@ func TestWriteJSONLPlainFileStillWorks(t *testing.T) { } } +// TestWriteJSONLRefusesOverlongRecord is the unreadable-artefact regression: +// MaxJSONLLine is the invariant every writer must respect, but pre-fix +// WriteJSONL enforced no upper bound, so merge could persist a timeline.jsonl +// record larger than ReadJSONL can scan back and still report success. The +// oversized record must be refused, and — because the refusal happens before +// the file is opened — an existing artefact must survive untouched rather than +// be truncated by the failed write. +func TestWriteJSONLRefusesOverlongRecord(t *testing.T) { + path := filepath.Join(t.TempDir(), TimelineFile) + if err := WriteJSONL(path, []map[string]string{{"actor": "Alice"}}); err != nil { + t.Fatalf("seed write: %v", err) + } + + huge := []map[string]string{{"v": strings.Repeat("a", MaxJSONLLine)}} + err := WriteJSONL(path, huge) + if err == nil { + t.Fatal("WriteJSONL persisted a record over MaxJSONLLine; want refusal") + } + if !strings.Contains(err.Error(), "record 0") { + t.Errorf("error does not name the offending index: %v", err) + } + + // The earlier artefact is intact: nothing was written, not even a truncation. + got, err := ReadJSONL[map[string]string](path) + if err != nil { + t.Fatalf("ReadJSONL after refusal: %v", err) + } + if len(got) != 1 || got[0]["actor"] != "Alice" { + t.Fatalf("refused write disturbed the existing artefact: %v", got) + } +} + +// TestWriteJSONLRefusalLeavesNoPartialFile pins the transactional stance on the +// records *before* the offending one: pre-fix there was no check at all, and a +// naive per-record check would still leave the earlier lines on disk followed +// by a line no reader can reach past. A refused set must write nothing. +func TestWriteJSONLRefusalLeavesNoPartialFile(t *testing.T) { + path := filepath.Join(t.TempDir(), FindingsFile) + values := []map[string]string{ + {"actor": "Bob"}, + {"v": strings.Repeat("b", MaxJSONLLine)}, + } + if err := WriteJSONL(path, values); err == nil { + t.Fatal("WriteJSONL accepted a set carrying an over-long record; want refusal") + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Fatalf("refused write created %s (err=%v); want no file at all", FindingsFile, err) + } +} + +// TestWriteJSONLBoundaryLineRoundTrips pins the exact boundary WriteJSONL +// chose: a line is readable when its bytes *including* the trailing newline fit +// within MaxJSONLLine, because ReadJSONL's scanner buffer must hold the +// terminator too. A record encoding to exactly MaxJSONLLine bytes with its +// newline must therefore be accepted and read back; one byte more must not. +func TestWriteJSONLBoundaryLineRoundTrips(t *testing.T) { + // {"v":""}\n is len(payload) + 9 bytes. + const overhead = 9 + path := filepath.Join(t.TempDir(), TimelineFile) + atLimit := []map[string]string{{"v": strings.Repeat("c", MaxJSONLLine-overhead)}} + if err := WriteJSONL(path, atLimit); err != nil { + t.Fatalf("WriteJSONL refused a record at the limit: %v", err) + } + fi, err := os.Stat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + if fi.Size() != MaxJSONLLine { + t.Fatalf("boundary line is %d bytes, want exactly %d — adjust the test, not the guard", fi.Size(), MaxJSONLLine) + } + got, err := ReadJSONL[map[string]string](path) + if err != nil { + t.Fatalf("ReadJSONL could not read back a line WriteJSONL accepted: %v", err) + } + if len(got) != 1 || len(got[0]["v"]) != MaxJSONLLine-overhead { + t.Fatalf("boundary record did not round-trip: %d records", len(got)) + } + + overLimit := []map[string]string{{"v": strings.Repeat("c", MaxJSONLLine-overhead+1)}} + if err := WriteJSONL(path, overLimit); err == nil { + t.Fatal("WriteJSONL accepted a line one byte past the limit; want refusal") + } +} + +// TestReadJSONLRefusesFIFO is the read-side hang regression: pre-fix ReadJSONL +// opened its path with plain os.Open, so a FIFO planted at a JSONL artefact +// (timeline.jsonl and friends, in a session Alice merely received from Bob) +// blocked the read open in open(2) for ever waiting for a writer that never +// came, hanging merge, report, or analyze. The read side now opens through the +// O_NONBLOCK no-follow guard, which returns immediately and refuses the FIFO. +// The read runs in a goroutine and the test fails on a timeout, so a regression +// reports a failure rather than hanging the suite for ever. +func TestReadJSONLRefusesFIFO(t *testing.T) { + path := filepath.Join(t.TempDir(), TimelineFile) + if err := syscall.Mkfifo(path, 0o644); err != nil { + t.Skipf("FIFOs unavailable on this platform: %v", err) + } + + done := make(chan error, 1) + go func() { + _, err := ReadJSONL[map[string]any](path) + done <- err + }() + + select { + case err := <-done: + if err == nil { + t.Fatal("ReadJSONL read from a FIFO; want refusal") + } + case <-time.After(5 * time.Second): + t.Fatal("ReadJSONL blocked on a FIFO instead of refusing it") + } +} + +// TestLoadManifestRefusesFIFO is the same read-side hang regression for the +// manifest: pre-fix LoadManifest used os.ReadFile, whose open(2) blocks for ever +// on a FIFO planted at manifest.json in an exchanged session, hanging every +// command that loads the manifest. LoadManifest now opens through the O_NONBLOCK +// no-follow guard and refuses it immediately. The load runs in a goroutine with +// a timeout so a regression fails rather than hangs. +func TestLoadManifestRefusesFIFO(t *testing.T) { + dir := t.TempDir() + if err := syscall.Mkfifo(filepath.Join(dir, ManifestFile), 0o644); err != nil { + t.Skipf("FIFOs unavailable on this platform: %v", err) + } + + done := make(chan error, 1) + go func() { + _, err := LoadManifest(dir) + done <- err + }() + + select { + case err := <-done: + if err == nil { + t.Fatal("LoadManifest read from a FIFO; want refusal") + } + case <-time.After(5 * time.Second): + t.Fatal("LoadManifest blocked on a FIFO instead of refusing it") + } +} + +// TestReadJSONLPlainFileStillWorks confirms the read-side guard leaves ordinary +// regular-file reads untouched: an honest timeline.jsonl must still decode. +func TestReadJSONLPlainFileStillWorks(t *testing.T) { + path := filepath.Join(t.TempDir(), TimelineFile) + if err := os.WriteFile(path, []byte("{\"actor\":\"Carol\"}\n"), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + got, err := ReadJSONL[map[string]any](path) + if err != nil { + t.Fatalf("ReadJSONL: %v", err) + } + if len(got) != 1 || got[0]["actor"] != "Carol" { + t.Fatalf("plain-file read disturbed by the guard: %v", got) + } +} + +// TestLoadManifestPlainFileStillWorks confirms the manifest read guard leaves an +// ordinary regular-file manifest.json readable. +func TestLoadManifestPlainFileStillWorks(t *testing.T) { + dir := t.TempDir() + body := `{"session":"2026-07-17_153045","app":"settings prototype","participant":"Bob","t0_epoch_ms":1}` + "\n" + if err := os.WriteFile(filepath.Join(dir, ManifestFile), []byte(body), 0o644); err != nil { + t.Fatalf("write manifest: %v", err) + } + m, err := LoadManifest(dir) + if err != nil { + t.Fatalf("LoadManifest: %v", err) + } + if m.App != "settings prototype" || m.Participant != "Bob" { + t.Fatalf("plain manifest read disturbed by the guard: %+v", m) + } +} + // TestSafeText strips control bytes (newline, CR, ESC, DEL, C1) that would // otherwise forge report structure or drive an ANSI terminal, while leaving // ordinary text intact. diff --git a/internal/timeline/timeline.go b/internal/timeline/timeline.go index f5cf651..f046687 100644 --- a/internal/timeline/timeline.go +++ b/internal/timeline/timeline.go @@ -24,6 +24,22 @@ type Utterance struct { Words []Word `json:"words,omitempty"` } +// rawUtterance is how a transcript.jsonl record is decoded before it is +// trusted. Its T0 and T1 are pointers so that an absent "t0" stays +// distinguishable from a genuine 0, exactly as rawInteraction's T is: an +// utterance that begins the moment capture starts legitimately carries t0 equal +// to 0, so a value-typed field cannot tell that record apart from one whose +// "t0" the transcript never named, and a zero test would either reject honest +// testimony or admit malformed lines. Everything else mirrors Utterance. +type rawUtterance struct { + ID string `json:"id"` + T0 *float64 `json:"t0"` + T1 *float64 `json:"t1"` + Speaker string `json:"speaker,omitempty"` + Text string `json:"text"` + Words []Word `json:"words,omitempty"` +} + // Word is one aligned word inside an utterance: its text and start time in // session-relative seconds (docs/reference/session-directory.md). type Word struct { @@ -188,6 +204,50 @@ func checkedInteractions(path string, raw []rawInteraction) ([]Interaction, erro return out, nil } +// checkedUtterances enforces the one transcript field whose absence cannot be +// caught later — t0 — and converts the accepted records for BuildEntries. It is +// the speech-side counterpart of checkedInteractions, and it exists because +// transcript.jsonl is as exchangeable and as hand-editable as interactions.jsonl +// while only the latter was ever guarded. Without the check a line missing "t0" +// decodes leniently to 0, BuildEntries places it at the session's very start, +// and the report prints it at 00:00 above everything that genuinely happened +// there: a malformed transcript would silently plant words at the opening of the +// evidence record, with nothing to say the transcript never timed them. Refusing +// the whole merge names the offending line so the operator repairs the +// transcript instead of reading a fabricated account of when Alice spoke. +// +// A nil t1 is defaulted to t0 rather than refused. A missing end time cannot +// move an utterance to a moment it did not occur — the fabrication hazard t0 +// carries — it can only shrink the span the report joins events against, and +// SpeechEnd already commits the package to a documented answer for a speech +// entry with no end: fall back to its start. Defaulting here reproduces that +// answer at the boundary instead of contradicting it, and it avoids the far +// worse alternative of a zero-valued t1 on an utterance at, say, t0 22, whose +// join window [t0-window, 0+window] is inverted and quietly matches no event at +// all. Discarding a whole session's recoverable testimony over a shrinkable +// window would be the disproportionate answer. +func checkedUtterances(path string, raw []rawUtterance) ([]Utterance, error) { + out := make([]Utterance, 0, len(raw)) + for i, r := range raw { + if r.T0 == nil { + return nil, fmt.Errorf("%s: utterance %d is missing t0; cannot place it on the session clock", path, i+1) + } + t1 := *r.T0 + if r.T1 != nil { + t1 = *r.T1 + } + out = append(out, Utterance{ + ID: r.ID, + T0: *r.T0, + T1: t1, + Speaker: r.Speaker, + Text: r.Text, + Words: r.Words, + }) + } + return out, nil +} + // 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) { @@ -195,24 +255,37 @@ func Merge(dir string) (speech, events int, err error) { if err != nil { return 0, 0, err } - utts, err := readOptionalJSONL[Utterance](filepath.Join(dir, session.TranscriptFile)) + uttsPath := filepath.Join(dir, session.TranscriptFile) + rawUtts, err := readOptionalJSONL[rawUtterance](uttsPath) if err != nil { return 0, 0, fmt.Errorf("read transcript: %w", err) } + utts, err := checkedUtterances(uttsPath, rawUtts) + if err != nil { + return 0, 0, err + } intsPath := filepath.Join(dir, session.InteractionsFile) raw, err := readOptionalJSONL[rawInteraction](intsPath) if err != nil { return 0, 0, fmt.Errorf("read interactions: %w", err) } - // Interaction times are epoch milliseconds; without t0 they cannot be placed - // on the session clock. A manifest lacking t0_epoch_ms leaves T0EpochMS at the - // zero value, which would turn each epoch-ms timestamp into a ~55-year offset - // and write a silently corrupt timeline. Reject it rather than emit nonsense. - // A transcript-only session carries no interactions and is already - // session-relative, so it is unaffected. - if len(raw) > 0 && man.T0EpochMS == 0 { - return 0, 0, fmt.Errorf("manifest is missing t0_epoch_ms; cannot place interactions on the session clock") + // Interaction times are epoch milliseconds; without a usable t0 they cannot be + // placed on the session clock. The anchor is obtained through session.Manifest.T0 + // rather than read from the field directly, so this call site honours the single + // rule that accessor owns: it treats a zero anchor as absent and, crucially, + // also refuses a NEGATIVE t0_epoch_ms — a case an inline `== 0` test missed, so a + // negative anchor slipped through and BuildEntries shifted every interaction by + // +|t0|, writing a silently corrupt timeline while Merge exited 0. The guard + // stays conditional on interactions being present: a transcript-only session + // carries no epoch-ms times, is already session-relative, and legitimately needs + // no anchor, so it must still merge without one. + t0 := man.T0EpochMS + if len(raw) > 0 { + t0, err = man.T0() + if err != nil { + return 0, 0, err + } } ints, err := checkedInteractions(intsPath, raw) @@ -220,7 +293,7 @@ func Merge(dir string) (speech, events int, err error) { return 0, 0, err } - entries := BuildEntries(man.T0EpochMS, utts, ints) + entries := BuildEntries(t0, utts, ints) if err := session.WriteJSONL(filepath.Join(dir, session.TimelineFile), entries); err != nil { return 0, 0, fmt.Errorf("write timeline: %w", err) } diff --git a/internal/timeline/timeline_test.go b/internal/timeline/timeline_test.go index fadb144..fc91b3d 100644 --- a/internal/timeline/timeline_test.go +++ b/internal/timeline/timeline_test.go @@ -1,6 +1,7 @@ package timeline import ( + "errors" "os" "path/filepath" "strconv" @@ -100,6 +101,65 @@ func TestMergeRejectsMissingT0WithInteractions(t *testing.T) { } } +// TestMergeRejectsNegativeT0WithInteractions is the negative-anchor regression: +// Merge used to guard the anchor with an inline `man.T0EpochMS == 0` test, which +// is narrower than the `<= 0` rule session.Manifest.T0 owns. A NEGATIVE +// t0_epoch_ms therefore slipped through, and BuildEntries shifted every +// interaction by +|t0| — placing each event decades into the session — while +// Merge wrote that silently corrupt timeline and exited 0. Routing through +// man.T0 now refuses it with the ErrNoT0-based error, and no timeline is written. +func TestMergeRejectsNegativeT0WithInteractions(t *testing.T) { + dir := t.TempDir() + // A negative anchor: it would place the session before the epoch, which no + // capture can produce. + if err := session.SaveManifest(dir, session.Manifest{Session: "s", T0EpochMS: -1}); err != nil { + t.Fatalf("SaveManifest: %v", err) + } + ints := []Interaction{{T: t0 + 9_500, Kind: "click", Selector: "[data-testid=save-btn]"}} + if err := session.WriteJSONL(filepath.Join(dir, session.InteractionsFile), ints); err != nil { + t.Fatalf("write interactions: %v", err) + } + _, _, err := Merge(dir) + if err == nil || !errors.Is(err, session.ErrNoT0) { + t.Fatalf("expected an ErrNoT0-based error for a negative anchor, got %v", err) + } + // The corrupt, +|t0|-shifted timeline must not have been written. + if _, statErr := os.Stat(filepath.Join(dir, session.TimelineFile)); statErr == nil { + t.Fatalf("timeline.jsonl was written despite the negative t0") + } +} + +// TestMergeTranscriptOnlyWithoutT0 pins the exemption the anchor guard must +// preserve: a transcript-only session (no interactions.jsonl) carries no +// epoch-ms times, is already session-relative, and legitimately needs no anchor, +// so it must still merge even when the manifest omits t0_epoch_ms. Routing the +// guard through man.T0 could have over-tightened Merge into demanding an anchor +// no transcript-only session has any use for; keeping the guard conditional on +// interactions being present is what this test defends. It differs from +// TestMergeAudioOnlySession, which supplies a valid t0: here t0 is absent. +func TestMergeTranscriptOnlyWithoutT0(t *testing.T) { + dir := t.TempDir() + // Manifest without t0_epoch_ms (left at the zero value); no interactions file. + if err := session.SaveManifest(dir, session.Manifest{Session: "s"}); 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 a transcript-only session without t0: %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) + } +} + // TestMergeRejectsInteractionMissingT is the phantom-event regression: an // interactions.jsonl line with no "t" used to decode leniently to the zero value // 0, which BuildEntries turned into a relative time of about minus fifty-six @@ -179,6 +239,102 @@ func TestMergeAcceptsInteractionAtT0(t *testing.T) { } } +// TestMergeRejectsUtteranceMissingT0 is the phantom-utterance regression, the +// speech-side twin of TestMergeRejectsInteractionMissingT. A transcript.jsonl +// line with no "t0" used to decode leniently to the zero value 0, so pre-fix +// Merge counted it and BuildEntries placed it at the session's very start: the +// report printed words at 00:00 that the transcript had never timed, above +// everything that genuinely happened there. Merge must refuse the session and +// name the offending line. +func TestMergeRejectsUtteranceMissingT0(t *testing.T) { + dir := t.TempDir() + if err := session.SaveManifest(dir, session.Manifest{Session: "s", T0EpochMS: t0}); err != nil { + t.Fatalf("SaveManifest: %v", err) + } + // The second record is the malformed one: an utterance with no "t0". + lines := "" + + `{"id":"utt-001","t0":8.0,"t1":12.5,"speaker":"P1","text":"I'll change my display name."}` + "\n" + + `{"id":"utt-002","t1":28.0,"speaker":"P1","text":"I clicked save and nothing happened."}` + "\n" + if err := os.WriteFile(filepath.Join(dir, session.TranscriptFile), []byte(lines), 0o644); err != nil { + t.Fatalf("write transcript: %v", err) + } + + _, _, err := Merge(dir) + if err == nil { + t.Fatalf("expected a missing-t0 error, got nil") + } + if !strings.Contains(err.Error(), "utterance 2") || !strings.Contains(err.Error(), "missing t0") { + t.Fatalf("error should name the offending line and field, got %v", err) + } + // The timeline carrying the phantom utterance must not have been written. + if _, statErr := os.Stat(filepath.Join(dir, session.TimelineFile)); statErr == nil { + t.Fatalf("timeline.jsonl was written despite the malformed utterance") + } +} + +// TestMergeAcceptsUtteranceAtT0 guards the other half of the required-field +// check, mirroring TestMergeAcceptsInteractionAtT0: an utterance beginning the +// moment capture starts carries t0 0, which a value-typed decode cannot +// distinguish from an absent "t0". Alice speaking as recording begins is +// legitimate evidence and must still merge, at t 0. +func TestMergeAcceptsUtteranceAtT0(t *testing.T) { + dir := t.TempDir() + if err := session.SaveManifest(dir, session.Manifest{Session: "s", T0EpochMS: t0}); err != nil { + t.Fatalf("SaveManifest: %v", err) + } + line := `{"id":"utt-001","t0":0,"t1":3.5,"speaker":"P1","text":"Right, I'm starting now."}` + "\n" + if err := os.WriteFile(filepath.Join(dir, session.TranscriptFile), []byte(line), 0o644); err != nil { + t.Fatalf("write transcript: %v", err) + } + + speech, events, err := Merge(dir) + if err != nil { + t.Fatalf("Merge with an utterance at t0: %v", err) + } + if speech != 1 || events != 0 { + t.Fatalf("want speech=1 events=0, got speech=%d events=%d", speech, events) + } + entries, err := session.ReadJSONL[Entry](filepath.Join(dir, session.TimelineFile)) + if err != nil { + t.Fatalf("read timeline: %v", err) + } + if len(entries) != 1 || entries[0].T != 0 || entries[0].Src != "speech" { + t.Fatalf("want a single speech entry at t=0, got %+v", entries) + } +} + +// TestMergeDefaultsUtteranceMissingT1ToT0 pins the deliberate asymmetry between +// the two transcript times: a missing end cannot move an utterance to a moment +// it did not occur, so it is defaulted to t0 rather than refused, matching the +// fallback SpeechEnd already documents. Pre-fix a missing "t1" decoded to 0, so +// an utterance at t0 22 got the join window [22-w, 0+w] — inverted, silently +// matching no event at all. The defaulted entry must instead carry t1 equal to +// t0, giving an honest zero-length span. +func TestMergeDefaultsUtteranceMissingT1ToT0(t *testing.T) { + dir := t.TempDir() + if err := session.SaveManifest(dir, session.Manifest{Session: "s", T0EpochMS: t0}); err != nil { + t.Fatalf("SaveManifest: %v", err) + } + line := `{"id":"utt-002","t0":22.0,"speaker":"P1","text":"I clicked save and nothing happened."}` + "\n" + if err := os.WriteFile(filepath.Join(dir, session.TranscriptFile), []byte(line), 0o644); err != nil { + t.Fatalf("write transcript: %v", err) + } + + if _, _, err := Merge(dir); err != nil { + t.Fatalf("Merge with an utterance missing t1: %v", err) + } + entries, err := session.ReadJSONL[Entry](filepath.Join(dir, session.TimelineFile)) + if err != nil { + t.Fatalf("read timeline: %v", err) + } + if len(entries) != 1 { + t.Fatalf("want a single speech entry, got %+v", entries) + } + if got := SpeechEnd(entries[0]); got != 22.0 { + t.Fatalf("want the end defaulted to t0 (22.0), got %v", got) + } +} + func TestEventsNearWindow(t *testing.T) { entries := sample() var speech []Entry diff --git a/internal/transcribe/ffmpeg.go b/internal/transcribe/ffmpeg.go index 851357b..3644f17 100644 --- a/internal/transcribe/ffmpeg.go +++ b/internal/transcribe/ffmpeg.go @@ -2,6 +2,7 @@ package transcribe import ( "encoding/json" + "errors" "fmt" "os" "os/exec" @@ -23,14 +24,17 @@ func convertAudio(in, out string) error { if !audioExts[ext] { return fmt.Errorf("unsupported audio format %q: expected .m4a, .mov, or .wav", ext) } - if _, err := os.Stat(in); err != nil { + // 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) } - // ffmpeg -y follows a symlink at the output path. In an untrusted (shared or - // downloaded) session directory a pre-planted audio.wav symlink would - // redirect the write outside the session, so refuse to overwrite through one. - if fi, err := os.Lstat(out); err == nil && fi.Mode()&os.ModeSymlink != 0 { - return fmt.Errorf("refusing to write %s: it is a symlink", out) + if err := checkPlainOutput(out); err != nil { + return err } ffmpeg, err := exec.LookPath("ffmpeg") if err != nil { @@ -43,6 +47,38 @@ func convertAudio(in, out string) error { return nil } +// checkPlainOutput refuses an ffmpeg output path that already exists as anything +// other than a regular file. ffmpeg is handed the path as a string and told to +// overwrite it with -y, so this write cannot go through +// session.OpenFileNoFollow, and ffmpeg opens without either O_NOFOLLOW or +// O_NONBLOCK. A session directory is an exchange unit — a shared or downloaded +// session may be attacker-authored — and both non-regular cases matter there. A +// symlink pre-planted at audio.wav would silently redirect the whole conversion +// outside the session, overwriting an arbitrary file the operator never named. A +// FIFO planted at the same path is worse than useless: ffmpeg's open(2) blocks +// for ever waiting for a reader that never arrives, so `testimony transcribe` +// hangs rather than failing, on a session the operator merely received. os.Lstat +// does not resolve the link, so a symlink is reported with ModeSymlink set even +// when its target is missing; it is named separately from the general refusal +// because a redirected write and a stuck one call for different remedies. An +// absent path is fine — that is the ordinary case, and ffmpeg creates it. +func checkPlainOutput(path string) error { + fi, err := os.Lstat(path) + if errors.Is(err, os.ErrNotExist) { + return nil + } + if err != nil { + return err + } + if fi.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("refusing to write %s: it is a symlink", path) + } + if !fi.Mode().IsRegular() { + return fmt.Errorf("refusing to write %s: it is not a regular file", path) + } + return nil +} + // deriveOffset reads the original recording's creation time via ffprobe and // returns creation_epoch_seconds − t0_epoch_seconds. The boolean is false // whenever ffprobe or the creation_time tag is unavailable — derivation is diff --git a/internal/transcribe/transcribe.go b/internal/transcribe/transcribe.go index 2f91e63..29eb440 100644 --- a/internal/transcribe/transcribe.go +++ b/internal/transcribe/transcribe.go @@ -74,12 +74,14 @@ func Run(opts Options) (int, error) { if err := convertAudio(opts.Audio, wav); err != nil { return 0, err } - } else if _, err := os.Stat(wav); err != nil { - return 0, fmt.Errorf("no %s in session %s and no -audio given: run `testimony record` first, or pass -audio FILE", - session.AudioFile, opts.SessionDir) + } else if err := checkSessionAudio(wav, opts.SessionDir); err != nil { + return 0, err } - offset, provenance := resolveOffset(opts, man.T0EpochMS, external) + offset, provenance, err := resolveOffset(opts, man, external) + if err != nil { + return 0, err + } fmt.Fprintf(opts.Log, "offset: %+.2fs (%s)\n", offset, provenance) var segs []segment @@ -101,24 +103,60 @@ func Run(opts Options) (int, error) { return len(utts), nil } +// checkSessionAudio validates the in-place audio.wav before its path is handed +// to the ASR engine. The engine opens the path itself, so this read never passes +// through session.OpenFileNoFollow's regular-file guard, and mere existence is +// not enough to establish that reading it will terminate: in a session that was +// shared or downloaded rather than recorded here, a FIFO planted at audio.wav +// satisfies os.Stat and then blocks the engine's read for ever, hanging +// `testimony transcribe` on a session the operator merely received. A symlink is +// resolved by os.Stat and needs no refusal here — a symlink redirects writes, +// and this path is only ever read. +func checkSessionAudio(wav, sessionDir string) error { + fi, err := os.Stat(wav) + if err != nil { + return fmt.Errorf("no %s in session %s and no -audio given: run `testimony record` first, or pass -audio FILE", + session.AudioFile, sessionDir) + } + if !fi.Mode().IsRegular() { + return fmt.Errorf("refusing to read %s: it is not a regular file", wav) + } + return nil +} + // resolveOffset picks the audio→session offset: the explicit -offset flag // wins; otherwise, for an external recording, the offset is derived from its // creation time vs manifest t0 when ffprobe makes that cheap; otherwise 0. // For an in-place session audio.wav there is no creation_time to derive from // and none is needed — capture starts at t0, so 0 is correct by construction. -// Derivation failure is never fatal. The second return value is the -// provenance, for the mandatory stdout report. -func resolveOffset(opts Options, t0EpochMS int64, external bool) (float64, string) { +// A failed ffprobe derivation is never fatal (default 0), but an unusable t0 on +// the external path is: the manifest t0 is read through Manifest.T0 rather than +// the raw T0EpochMS field, because an absent (0) or negative anchor decodes to a +// value that deriveOffset would subtract from the recording's real epoch-second +// creation time, yielding an offset of roughly the whole Unix epoch (~1.78e9 s) +// that mapSegments then adds to every utterance — writing a transcript.jsonl +// with times about fifty-seven years into the session and returning success, +// silent corruption that reads as evidence. Refusing the run is the only honest +// outcome, so an unusable t0 is surfaced as an error rather than fabricated +// times. t0 is consulted only on this external-derivation path: the -offset flag +// and the in-place audio.wav (captured at t0, offset 0) neither derive from nor +// need an anchor, so a missing t0 must not fail them. The second return value is +// the provenance, for the mandatory stdout report. +func resolveOffset(opts Options, man session.Manifest, external bool) (float64, string, error) { if opts.OffsetSet { - return opts.Offset, "from -offset flag" + return opts.Offset, "from -offset flag", nil } if external { - if off, ok := deriveOffset(opts.Audio, t0EpochMS); ok { - return off, "derived: audio creation_time − manifest t0" + t0, err := man.T0() + if err != nil { + return 0, "", fmt.Errorf("deriving audio offset: %w", err) + } + if off, ok := deriveOffset(opts.Audio, t0); ok { + return off, "derived: audio creation_time − manifest t0", nil } - return 0, "default 0: audio creation time unavailable" + return 0, "default 0: audio creation time unavailable", nil } - return 0, "default 0: session audio.wav captured at t0" + return 0, "default 0: session audio.wav captured at t0", nil } // mapSegments converts engine segments to the Utterance schema of diff --git a/internal/transcribe/transcribe_test.go b/internal/transcribe/transcribe_test.go index 110bb2f..77075c8 100644 --- a/internal/transcribe/transcribe_test.go +++ b/internal/transcribe/transcribe_test.go @@ -1,11 +1,14 @@ package transcribe import ( + "errors" "os" "os/exec" "path/filepath" "strings" + "syscall" "testing" + "time" "github.com/REPPL/Testimony/internal/session" "github.com/REPPL/Testimony/internal/timeline" @@ -109,6 +112,75 @@ func TestWhisperCppFixture(t *testing.T) { } } +// TestWhisperXRejectsUntimedSegment is the speech-at-time-0 regression: the +// segment-level start/end were value-typed float64, so a segment whose start +// whisperx omitted decoded to 0 and mapSegments filed Bob's remark as an +// utterance beginning at session time 0 — speech planted at the head of the +// evidence record, with nothing to say the engine never placed it. The +// word-level fields were already pointers, which is what made the segment-level +// omission an oversight rather than a choice. A missing end is refused too: it +// would otherwise collapse t1 onto t0 and shrink the window EventsNear joins +// interactions over. +func TestWhisperXRejectsUntimedSegment(t *testing.T) { + for _, c := range []struct{ name, raw, want string }{ + {"missing start", `{"segments":[{"end":4.0,"text":"Bob hesitates here."}]}`, "missing start"}, + {"missing end", `{"segments":[{"start":61.5,"text":"Bob hesitates here."}]}`, "missing end"}, + } { + segs, err := parseWhisperX([]byte(c.raw)) + if err == nil { + // Pre-fix this branch ran, and the utterance landed at t0=0. + utts := mapSegments(segs, 0) + t.Fatalf("%s: want refusal, got utterances %+v", c.name, utts) + } + if !strings.Contains(err.Error(), c.want) { + t.Fatalf("%s: want an error naming %q, got %v", c.name, c.want, err) + } + } + + // A fully timed segment must still parse — the guard rejects absence, not a + // legitimate start of 0 at the very beginning of the recording. + segs, err := parseWhisperX([]byte(`{"segments":[{"start":0,"end":2.5,"text":"Alice begins."}]}`)) + if err != nil { + t.Fatalf("a segment starting at a genuine 0 must be accepted, got %v", err) + } + if len(segs) != 1 || segs[0].start != 0 || segs[0].end != 2.5 { + t.Fatalf("timed segment mis-parsed: %+v", segs) + } +} + +// TestWhisperCppRejectsUntimedSegment is the same speech-at-time-0 regression on +// the whisper.cpp adapter: offsets.from/to were value-typed int64, so a segment +// whose "from" whisper-cli omitted decoded to 0 ms and Carol's remark was filed +// at session time 0 rather than where she said it. This engine emits no +// word-level timings, so the offsets are its only clock and there is nothing to +// fall back on; a missing "to" is refused for the same reason as in whisperx. +func TestWhisperCppRejectsUntimedSegment(t *testing.T) { + for _, c := range []struct{ name, raw, want string }{ + {"missing from", `{"transcription":[{"offsets":{"to":9000},"text":"Carol scrolls back."}]}`, "missing offsets.from"}, + {"missing to", `{"transcription":[{"offsets":{"from":75000},"text":"Carol scrolls back."}]}`, "missing offsets.to"}, + } { + segs, err := parseWhisperCpp([]byte(c.raw)) + if err == nil { + // Pre-fix this branch ran, and the utterance landed at t0=0. + utts := mapSegments(segs, 0) + t.Fatalf("%s: want refusal, got utterances %+v", c.name, utts) + } + if !strings.Contains(err.Error(), c.want) { + t.Fatalf("%s: want an error naming %q, got %v", c.name, c.want, err) + } + } + + // A genuine 0 ms offset — speech from the first instant of the recording — + // stays acceptable; only absence is refused. + segs, err := parseWhisperCpp([]byte(`{"transcription":[{"offsets":{"from":0,"to":2500},"text":"Alice begins."}]}`)) + if err != nil { + t.Fatalf("a segment starting at a genuine 0 must be accepted, got %v", err) + } + if len(segs) != 1 || segs[0].start != 0 || segs[0].end != 2.5 { + t.Fatalf("timed segment mis-parsed: %+v", segs) + } +} + func TestMapSegmentsNegativeOffset(t *testing.T) { utts := mapSegments([]segment{ {start: 10.0, end: 12.345, text: " Carol pauses. ", words: []timeline.Word{{W: " Carol ", T: 10.004}}}, @@ -129,7 +201,10 @@ func TestMapSegmentsNegativeOffset(t *testing.T) { } func TestResolveOffsetFlagWins(t *testing.T) { - off, prov := resolveOffset(Options{Offset: 4.25, OffsetSet: true}, 0, true) + off, prov, err := resolveOffset(Options{Offset: 4.25, OffsetSet: true}, session.Manifest{T0EpochMS: 0}, true) + if err != nil { + t.Fatalf("explicit -offset must not consult t0: got error %v", err) + } if off != 4.25 || prov != "from -offset flag" { t.Fatalf("explicit -offset must win: got %v (%s)", off, prov) } @@ -138,7 +213,10 @@ func TestResolveOffsetFlagWins(t *testing.T) { // TestResolveOffsetInPlace covers a record session: no external -audio, so the // offset is 0 by construction (capture starts at t0) with no ffprobe involved. func TestResolveOffsetInPlace(t *testing.T) { - off, prov := resolveOffset(Options{}, 1_700_000_000_000, false) + off, prov, err := resolveOffset(Options{}, session.Manifest{T0EpochMS: 1_700_000_000_000}, false) + if err != nil { + t.Fatalf("in-place path must not fail: got error %v", err) + } if off != 0 { t.Fatalf("in-place audio.wav offset must be 0, got %v", off) } @@ -147,6 +225,64 @@ func TestResolveOffsetInPlace(t *testing.T) { } } +// TestResolveOffsetInPlaceNoT0 proves the crucial constraint's in-place half: the +// record flow transcribes the session's own audio.wav (captured at t0, offset 0) +// and never derives an offset from t0, so a missing t0_epoch_ms must not fail it. +// Pre-fix resolveOffset returned no error at all, so this succeeded incidentally; +// the guard added for the external path must not leak into this branch. +func TestResolveOffsetInPlaceNoT0(t *testing.T) { + off, prov, err := resolveOffset(Options{}, session.Manifest{T0EpochMS: 0}, false) + if err != nil { + t.Fatalf("in-place audio.wav with no t0 must still succeed, got error %v", err) + } + if off != 0 { + t.Fatalf("in-place audio.wav offset must be 0, got %v", off) + } + if prov != "default 0: session audio.wav captured at t0" { + t.Fatalf("unexpected provenance: %q", prov) + } +} + +// TestResolveOffsetExternalNoT0 is the silent-transcript-corruption regression: +// pre-fix resolveOffset took the raw man.T0EpochMS and, on the external +// recording path, handed it to deriveOffset unchecked. A received or hand-edited +// session whose manifest omits t0_epoch_ms decodes that field to 0 (a negative +// value is likewise unusable), so deriveOffset returned the recording's real +// epoch-second creation time — roughly the whole Unix epoch, ~1.78e9 s — as the +// offset, mapSegments added it to every utterance, and transcript.jsonl was +// written with times about fifty-seven years into the session while Run returned +// success. The fix reads t0 through Manifest.T0, so an unusable anchor now surfaces +// as an ErrNoT0-based error and the run refuses rather than fabricating times. +func TestResolveOffsetExternalNoT0(t *testing.T) { + for _, m := range []session.Manifest{ + {Session: "2026-07-22_bob-received", T0EpochMS: 0}, + {Session: "2026-07-22_carol-edited", T0EpochMS: -1}, + } { + _, _, err := resolveOffset(Options{Audio: "bob-interview.m4a"}, m, true) + if err == nil { + t.Fatalf("external recording with unusable t0 (%d) must fail, got nil error", m.T0EpochMS) + } + if !errors.Is(err, session.ErrNoT0) { + t.Fatalf("want an ErrNoT0-based error, got %v", err) + } + } +} + +// TestResolveOffsetExternalOffsetFlagNoT0 proves the crucial constraint that an +// explicit -offset short-circuits before t0 is consulted: an operator who states +// the offset needs no anchor, so a missing t0_epoch_ms must not fail the run even +// on the external path. Pre-fix the raw field was passed through regardless; the +// flag branch now returns before Manifest.T0 is called at all. +func TestResolveOffsetExternalOffsetFlagNoT0(t *testing.T) { + off, prov, err := resolveOffset(Options{Audio: "bob-interview.m4a", Offset: 3.0, OffsetSet: true}, session.Manifest{T0EpochMS: 0}, true) + if err != nil { + t.Fatalf("explicit -offset must succeed without a t0, got error %v", err) + } + if off != 3.0 || prov != "from -offset flag" { + t.Fatalf("explicit -offset must win without consulting t0: got %v (%s)", off, prov) + } +} + // 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) { @@ -283,3 +419,106 @@ func TestConvertAudioRefusesSymlinkOutput(t *testing.T) { t.Fatalf("victim overwritten through symlink: %q", b) } } + +// TestConvertAudioRefusesFIFOOutput is the hang regression: pre-fix the +// output-path guard tested only for ModeSymlink, so a FIFO planted at audio.wav +// in a session Alice merely received from Bob passed the check and ffmpeg's +// open(2) then blocked for ever waiting for a reader, hanging `testimony +// transcribe` with no error. The conversion runs in a goroutine and the test +// fails on a timeout, so a regression reports a failure rather than hanging the +// suite for ever. +func TestConvertAudioRefusesFIFOOutput(t *testing.T) { + dir := t.TempDir() + in := filepath.Join(dir, "voice.wav") + if err := os.WriteFile(in, []byte("not really audio"), 0o644); err != nil { + t.Fatalf("seed input: %v", err) + } + out := filepath.Join(dir, session.AudioFile) + if err := syscall.Mkfifo(out, 0o644); err != nil { + t.Skipf("FIFOs unavailable on this platform: %v", err) + } + + done := make(chan error, 1) + go func() { done <- convertAudio(in, out) }() + + select { + case err := <-done: + if err == nil || !strings.Contains(err.Error(), "not a regular file") { + t.Fatalf("want non-regular-file refusal, got %v", err) + } + case <-time.After(5 * time.Second): + t.Fatal("convertAudio blocked on a FIFO output instead of refusing it") + } +} + +// TestConvertAudioRefusesFIFOInput is the input-side half of the same hang: the +// pre-fix existence check was a bare os.Stat, which a FIFO satisfies, so ffmpeg +// was handed a path whose open(2) never returns. A symlink to a real recording +// must still be accepted — os.Stat resolves it, and only writes are redirected +// by a symlink. +func TestConvertAudioRefusesFIFOInput(t *testing.T) { + dir := t.TempDir() + in := filepath.Join(dir, "voice.wav") + if err := syscall.Mkfifo(in, 0o644); err != nil { + t.Skipf("FIFOs unavailable on this platform: %v", err) + } + + done := make(chan error, 1) + go func() { done <- convertAudio(in, filepath.Join(dir, session.AudioFile)) }() + + select { + case err := <-done: + if err == nil || !strings.Contains(err.Error(), "not a regular file") { + t.Fatalf("want non-regular-file refusal, got %v", err) + } + case <-time.After(5 * time.Second): + t.Fatal("convertAudio blocked on a FIFO input instead of refusing it") + } + + real := filepath.Join(dir, "bob-interview.wav") + if err := os.WriteFile(real, []byte("RIFF"), 0o644); err != nil { + t.Fatal(err) + } + link := filepath.Join(dir, "link.wav") + if err := os.Symlink(real, link); err != nil { + t.Fatalf("symlink: %v", err) + } + // A symlinked recording is legitimate, so it must get past the input guard + // and fail (if at all) only later, on the ffmpeg lookup or the conversion. + if err := convertAudio(link, filepath.Join(dir, session.AudioFile)); err != nil && + strings.Contains(err.Error(), "not a regular file") { + t.Fatalf("symlink to a regular recording must be accepted, got %v", err) + } +} + +// TestCheckSessionAudioRefusesFIFO is the in-place branch of the same hang: with +// no -audio flag the session's own audio.wav is passed straight to the ASR +// engine, and pre-fix a bare os.Stat was the only check, so a FIFO planted there +// blocked the engine's read for ever. An absent file must still produce the +// actionable "run record first" message rather than this refusal. +func TestCheckSessionAudioRefusesFIFO(t *testing.T) { + dir := t.TempDir() + wav := filepath.Join(dir, session.AudioFile) + + err := checkSessionAudio(wav, dir) + if err == nil || !strings.Contains(err.Error(), "run `testimony record` first") { + t.Fatalf("missing audio must stay an actionable error, got %v", err) + } + + if err := syscall.Mkfifo(wav, 0o644); err != nil { + t.Skipf("FIFOs unavailable on this platform: %v", err) + } + if err := checkSessionAudio(wav, dir); err == nil || !strings.Contains(err.Error(), "not a regular file") { + t.Fatalf("want non-regular-file refusal for a FIFO audio.wav, got %v", err) + } + + if err := os.Remove(wav); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(wav, []byte("RIFF"), 0o644); err != nil { + t.Fatal(err) + } + if err := checkSessionAudio(wav, dir); err != nil { + t.Fatalf("a real audio.wav must be accepted, got %v", err) + } +} diff --git a/internal/transcribe/whispercpp.go b/internal/transcribe/whispercpp.go index 4e0fd2f..e0e0f2d 100644 --- a/internal/transcribe/whispercpp.go +++ b/internal/transcribe/whispercpp.go @@ -15,10 +15,19 @@ type whispercppOutput struct { Result struct { Language string `json:"language"` } `json:"result"` + // The offsets are pointers so that an absent time stays distinguishable + // from a genuine 0: a segment of speech at the very start of the recording + // legitimately carries from 0, so a value-typed field cannot tell the two + // apart. With one, a segment whose "from" whisper-cli omitted decodes to 0 + // and mapSegments files the utterance at session time 0 — speech planted at + // the head of the evidence record, minutes from where it was actually said, + // with nothing on the record to say the engine never placed it. This + // mirrors the guard timeline.rawInteraction and analyze.rawFinding apply to + // their own untrusted times. Transcription []struct { Offsets struct { - From int64 `json:"from"` // ms - To int64 `json:"to"` // ms + From *int64 `json:"from"` // ms + To *int64 `json:"to"` // ms } `json:"offsets"` Text string `json:"text"` } `json:"transcription"` @@ -58,16 +67,32 @@ func runWhisperCpp(bin, wav string, opts Options) ([]segment, error) { // parseWhisperCpp converts the whisper.cpp JSON into engine-neutral // segments, using the millisecond offsets. +// +// A segment missing either offset is a hard error rather than a silent +// default, matching parseWhisperX. whisper.cpp emits no word-level timings at +// all, so the segment offsets are the only clock this engine contributes and +// there is nothing left to fall back on: defaulting "from" to 0 relocates the +// speech, and defaulting "to" to "from" yields a zero-length utterance whose +// t1 then shrinks the [t0−window, t1+window] span timeline.EventsNear joins +// interactions over, dropping the very interactions the utterance was about. +// Refusing the run tells the operator their transcript is incomplete, which is +// the one outcome that leaves nothing false on the record. func parseWhisperCpp(raw []byte) ([]segment, error) { var out whispercppOutput if err := json.Unmarshal(raw, &out); err != nil { return nil, fmt.Errorf("parse %s JSON: %w", whisperCppBinary, err) } segs := make([]segment, 0, len(out.Transcription)) - for _, t := range out.Transcription { + for i, t := range out.Transcription { + if t.Offsets.From == nil { + return nil, fmt.Errorf("%s segment %d is missing offsets.from; cannot place it on the audio clock", whisperCppBinary, i+1) + } + if t.Offsets.To == nil { + return nil, fmt.Errorf("%s segment %d is missing offsets.to; cannot say when the speech stopped", whisperCppBinary, i+1) + } segs = append(segs, segment{ - start: float64(t.Offsets.From) / 1000.0, - end: float64(t.Offsets.To) / 1000.0, + start: float64(*t.Offsets.From) / 1000.0, + end: float64(*t.Offsets.To) / 1000.0, text: t.Text, }) } diff --git a/internal/transcribe/whisperx.go b/internal/transcribe/whisperx.go index 8b78f91..f704e10 100644 --- a/internal/transcribe/whisperx.go +++ b/internal/transcribe/whisperx.go @@ -19,9 +19,21 @@ type whisperxOutput struct { Language string `json:"language"` } +// whisperxSegment is how one segment of the engine's output is decoded before +// it is trusted. Start and End are pointers so that an absent time stays +// distinguishable from a genuine 0: a segment of speech at the very start of +// the recording legitimately carries start 0, so a value-typed field cannot +// tell the two apart. With one, a segment whose start whisperx omitted decodes +// to 0 and mapSegments files the utterance at session time 0 — speech planted +// at the head of the evidence record, minutes from where it was actually said, +// with nothing on the record to say the engine never placed it. The word-level +// fields below have carried pointers for exactly this reason since they were +// written; the segment-level ones were the omission. This mirrors the guard +// timeline.rawInteraction and analyze.rawFinding apply to their own untrusted +// times. type whisperxSegment struct { - Start float64 `json:"start"` - End float64 `json:"end"` + Start *float64 `json:"start"` + End *float64 `json:"end"` Text string `json:"text"` Speaker string `json:"speaker"` // diarisation label, when enabled Words []whisperxWord `json:"words"` @@ -114,14 +126,32 @@ func cudaVisible() bool { // parseWhisperX converts the WhisperX JSON into engine-neutral segments. // Words without a start timestamp (alignment miss) are omitted. +// +// A segment missing either of its own times is a hard error rather than a +// silent default. An unaligned word is a routine, expected outcome the engine +// reports for individual tokens, so dropping it costs only word-level detail; +// a segment without times is a malformed engine response, and the two +// alternatives to refusing it both put a fabrication in the evidence record. +// Defaulting start to 0 relocates the speech. Defaulting end to start yields a +// zero-length utterance, and t1 is not decorative: timeline.EventsNear joins +// interactions to speech over [t0−window, t1+window], so a fabricated end +// quietly shrinks the window and drops the very interactions the utterance was +// about. Refusing the run tells the operator their transcript is incomplete, +// which is the one outcome that leaves nothing false on the record. func parseWhisperX(raw []byte) ([]segment, error) { var out whisperxOutput if err := json.Unmarshal(raw, &out); err != nil { return nil, fmt.Errorf("parse whisperx JSON: %w", err) } segs := make([]segment, 0, len(out.Segments)) - for _, s := range out.Segments { - seg := segment{start: s.Start, end: s.End, text: s.Text, speaker: s.Speaker} + for i, s := range out.Segments { + if s.Start == nil { + return nil, fmt.Errorf("whisperx segment %d is missing start; cannot place it on the audio clock", i+1) + } + if s.End == nil { + return nil, fmt.Errorf("whisperx segment %d is missing end; cannot say when the speech stopped", i+1) + } + seg := segment{start: *s.Start, end: *s.End, text: s.Text, speaker: s.Speaker} for _, w := range s.Words { if w.Start == nil { continue