diff --git a/.abcd/development/brief/04-surfaces/06-analyze.md b/.abcd/development/brief/04-surfaces/06-analyze.md index 98f8384..73085b6 100644 --- a/.abcd/development/brief/04-surfaces/06-analyze.md +++ b/.abcd/development/brief/04-surfaces/06-analyze.md @@ -63,7 +63,12 @@ page ([`../05-internals/02-schemas.md`](../05-internals/02-schemas.md)). `findings.jsonl`. Ingest never trusts the model — nothing lands `confirmed`. - To protect the retained precision record, ingest refuses to overwrite a `findings.jsonl` that already holds verdict records; a fresh or verdict-free - file is written cleanly. + file is written cleanly. The guard counts any `kind:"verdict"` line, including + one whose value is outside the closed enum (a hand-edited or shared file), so a + foreign-valued human decision is never silently truncated by a re-ingest. +- An answer with no findings (a bare `[]`, `{"findings":[]}`, or a truncated + file) is refused rather than written: the write truncates, so an empty answer + would otherwise erase a prior good `findings.jsonl` and report success. ## Deferred diff --git a/.abcd/development/brief/05-internals/02-schemas.md b/.abcd/development/brief/05-internals/02-schemas.md index e743a27..f60bc0b 100644 --- a/.abcd/development/brief/05-internals/02-schemas.md +++ b/.abcd/development/brief/05-internals/02-schemas.md @@ -89,7 +89,7 @@ validation boundary; every field below is checked, and `status` is forced to | Field | Type | Required | Notes | |---|---|---|---| | `id` | string | yes | `^F-\d{3}$` (F-NNN, zero-padded); unique within the file | -| `t` | float64 | yes | finding time, session-relative seconds; `0 ≤ t ≤ sessionEnd` | +| `t` | float64 | yes | finding time, session-relative seconds; `sessionStart ≤ t ≤ sessionEnd`, where the bounds are the earliest and latest entry times in `timeline.jsonl` and `sessionStart` is `0` unless the timeline holds negative-time utterances (a recording predating `t0`) | | `type` | string | yes | one of `bug \| friction \| inconsistency \| preference \| idea` | | `severity` | int | yes | Nielsen-style `1..4` (cosmetic, minor, major, blocker) | | `mode` | string | no | `A \| B`, default `A`; only Mode A is produced in this slice | diff --git a/.abcd/work/DECISIONS.md b/.abcd/work/DECISIONS.md index e87d6a6..7792d47 100644 --- a/.abcd/work/DECISIONS.md +++ b/.abcd/work/DECISIONS.md @@ -155,3 +155,22 @@ Architecture-shaping decisions graduate to an ADR under (`bytes.TrimSpace`), matching `analyze.Load`, so an exchanged/hand-edited session's blank line is skipped as documented rather than crashing merge/report with "unexpected end of JSON input". +- 2026-07-18 — `analyze.validate` derives the finding-`t` lower bound from the + timeline (earliest entry time, floored at 0) instead of hard-coding 0, so a + finding faithfully anchored to a legitimately negative-time utterance — an + external recording whose `creation_time` predates `t0`, giving a negative + `deriveOffset` — no longer fails the whole transactional ingest. +- 2026-07-18 — `analyze.Ingest` refuses an answer with no findings (bare `[]`, + `{"findings":[]}`, or a truncated file) rather than writing an empty slice with + O_TRUNC, which previously erased a prior good `findings.jsonl` and reported + success. +- 2026-07-18 — `analyze.holdsVerdicts` scans `findings.jsonl` for any raw + `kind:"verdict"` line instead of consulting the enum-filtered `analyze.Load` + slice, so the overwrite guard still fires for a hand-edited/shared file whose + only verdict carries an out-of-enum value, protecting the retained precision + record from a truncating re-ingest. +- 2026-07-18 — `demo.appendRecords` truncates a stream file back to its + pre-write length when a write fails, so a short write (ENOSPC persists a + newline-less prefix) can no longer leave a partial JSONL line that corrupts one + physical record and breaks merge's reader; corrected the false comment claiming + `os.File.Write` gives newline atomicity. diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 054db70..5494a64 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -135,7 +135,7 @@ testimony analyze -session DIR -ingest FILE # validate the answer → find Emit behaviour: writes a single self-contained prompt — the rubric version header (`testimony-analysis/v1`), the second-coder stance, two-pass instructions (segment coding, then session synthesis), the rubric body (five `type` definitions, the `1..4` severity scale, the evidence hard-constraints), the session context (app, participant, tasks), the timeline lines inline, and the required output shape with a worked example. Nothing in the session directory is mutated. The timeline is emitted whole (v1 does not chunk by task boundary; the manifest carries no task timestamps). With `-out FILE` the prompt goes to a file and the command prints `wrote `; otherwise it prints to stdout. -Ingest behaviour: reads the answer from `FILE` (or stdin when `-`), accepting a top-level object with a `findings` array (optionally a `rubric`, which must be a known version) or a bare array. Ingest is the sole validation boundary and never trusts the model. Each finding is decoded with unknown fields disallowed, then checked against every schema rule (see [session directory reference](session-directory.md#findingsjsonl)): id format and uniqueness, `t` within the session, the `type` and `severity` enums, non-empty `evidence` with every id real and at least one spoken `utt-*` anchor, a `quote` that is a verbatim substring of one *cited* evidence utterance, and any `ui` selector/route matching a real event. Validation is transactional — all errors are reported at once and nothing is written on any failure. On success every finding is forced to `status: unverified`, `findings.jsonl` is written, and the command prints `validated N findings → (all unverified)`. Ingest refuses to overwrite a `findings.jsonl` that already holds verdict records. +Ingest behaviour: reads the answer from `FILE` (or stdin when `-`), accepting a top-level object with a `findings` array (optionally a `rubric`, which must be a known version) or a bare array. Ingest is the sole validation boundary and never trusts the model. Each finding is decoded with unknown fields disallowed, then checked against every schema rule (see [session directory reference](session-directory.md#findingsjsonl)): id format and uniqueness, `t` within the session, the `type` and `severity` enums, non-empty `evidence` with every id real and at least one spoken `utt-*` anchor, a `quote` that is a verbatim substring of one *cited* evidence utterance, and any `ui` selector/route matching a real event. Validation is transactional — all errors are reported at once and nothing is written on any failure. On success every finding is forced to `status: unverified`, `findings.jsonl` is written, and the command prints `validated N findings → (all unverified)`. An answer with no findings (a bare `[]`, `{"findings":[]}`, or a truncated file) is refused rather than written, so it cannot erase a prior `findings.jsonl`. Ingest refuses to overwrite a `findings.jsonl` that already holds verdict records — counting any `kind:"verdict"` line, even one whose value is outside the closed enum. ## `testimony review` diff --git a/docs/reference/session-directory.md b/docs/reference/session-directory.md index 651aa68..5047dfc 100644 --- a/docs/reference/session-directory.md +++ b/docs/reference/session-directory.md @@ -109,7 +109,7 @@ Ingest validates every finding against the merged timeline and is the sole valid | Field | Type | Required | Meaning | |---|---|---|---| | `id` | string | yes | `F-NNN`, zero-padded (`^F-\d{3}$`); unique within the file | -| `t` | number | yes | finding time, session-relative seconds; within `[0, sessionEnd]` | +| `t` | number | yes | finding time, session-relative seconds; within `[sessionStart, sessionEnd]` (the earliest and latest timeline entry times; `sessionStart` is `0` unless the timeline holds negative-time utterances from a recording predating `t0`) | | `type` | string | yes | one of `bug`, `friction`, `inconsistency`, `preference`, `idea` | | `severity` | integer | yes | usability-severity scale `1..4`: cosmetic, minor, major, blocker | | `mode` | string | no | `A` or `B`, default `A`; only Mode A is produced today | diff --git a/internal/analyze/analyze_test.go b/internal/analyze/analyze_test.go index c52ec9e..064adab 100644 --- a/internal/analyze/analyze_test.go +++ b/internal/analyze/analyze_test.go @@ -159,6 +159,83 @@ func TestIngestRefusesOverwriteWithVerdicts(t *testing.T) { } } +// negativeTimeline is a merged timeline whose utterance sits at a negative +// session-relative time, as an external recording predating manifest t0 +// legitimately produces (deriveOffset returns creation_time − t0 < 0). +const negativeTimeline = `{"t":-3,"src":"speech","id":"utt-004","payload":{"speaker":"P1","t1":-1,"text":"Hm. I clicked save and nothing happened. No message."}} +{"t":-2.5,"src":"event","id":"ev-003","payload":{"kind":"click","route":"#general","selector":"[data-testid=save-btn]","text":"Save"}} +` + +// TestIngestAcceptsNegativeAnchoredFinding is the negative-time regression: a +// finding anchored to a legitimately negative-time utterance must ingest. Pre-fix +// validate hard-coded the lower bound at 0, so it rejected `t` outside [0, end] +// and failed the whole (transactional) ingest for such a session. +func TestIngestAcceptsNegativeAnchoredFinding(t *testing.T) { + dir := writeSession(t, negativeTimeline) + answer := `{"rubric":"testimony-analysis/v1","findings":[ + {"id":"F-001","t":-3.0,"type":"bug","severity":3,"quote":"I clicked save and nothing happened", + "evidence":["utt-004","ev-003"],"status":"unverified"} + ]}` + findings, err := Ingest(dir, strings.NewReader(answer)) + if err != nil { + t.Fatalf("Ingest of a negative-anchored finding: %v", err) + } + if len(findings) != 1 || findings[0].T != -3.0 { + t.Fatalf("got %+v, want one finding at t=-3", findings) + } +} + +// TestIngestRefusesEmptyFindings is the data-loss regression: an empty findings +// array must not truncate a prior good findings.jsonl. Pre-fix Ingest wrote an +// empty slice with O_TRUNC and reported success. +func TestIngestRefusesEmptyFindings(t *testing.T) { + dir := writeSession(t, timelineFixture) + 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) + + for _, empty := range []string{`{"findings":[]}`, `[]`} { + if _, err := Ingest(dir, strings.NewReader(empty)); err == nil || !strings.Contains(err.Error(), "no findings") { + t.Fatalf("empty answer %q: expected a no-findings refusal, got %v", empty, err) + } + after, _ := os.ReadFile(path) + if string(before) != string(after) { + t.Fatalf("empty answer %q truncated a prior findings.jsonl", empty) + } + } +} + +// TestIngestRefusesOverwriteWithForeignVerdict is the guard-precision +// regression: findings.jsonl whose only verdict line carries an out-of-enum +// value (a hand-edited/shared file) must still block a truncating re-ingest. +// Pre-fix holdsVerdicts consulted analyze.Load, which drops out-of-enum verdicts, +// so the guard saw none and the human-decision record was overwritten. +func TestIngestRefusesOverwriteWithForeignVerdict(t *testing.T) { + dir := writeSession(t, timelineFixture) + if _, err := Ingest(dir, strings.NewReader(goodAnswer)); err != nil { + t.Fatalf("first Ingest: %v", err) + } + path := filepath.Join(dir, session.FindingsFile) + f, err := os.OpenFile(path, os.O_APPEND|os.O_WRONLY, 0o644) + if err != nil { + t.Fatalf("open: %v", err) + } + // "confirm" is not in the closed enum (confirmed|rejected|duplicate). + f.WriteString(`{"kind":"verdict","finding":"F-001","verdict":"confirm","at":"2026-07-18"}` + "\n") + f.Close() + + before, _ := os.ReadFile(path) + if _, err := Ingest(dir, strings.NewReader(goodAnswer)); err == nil || !strings.Contains(err.Error(), "refusing to overwrite") { + t.Fatalf("expected overwrite refusal for a foreign-verdict file, got %v", err) + } + after, _ := os.ReadFile(path) + if string(before) != string(after) { + t.Fatalf("findings.jsonl was modified despite the refusal") + } +} + func TestEffectiveStatusLastWins(t *testing.T) { findings := []Finding{{ID: "F-001"}, {ID: "F-002"}} verdicts := []Verdict{ diff --git a/internal/analyze/ingest.go b/internal/analyze/ingest.go index 54f879d..9913abd 100644 --- a/internal/analyze/ingest.go +++ b/internal/analyze/ingest.go @@ -1,6 +1,7 @@ package analyze import ( + "bufio" "bytes" "encoding/json" "errors" @@ -60,6 +61,13 @@ func Ingest(dir string, r io.Reader) ([]Finding, error) { if rubric != "" && !knownRubrics[rubric] { return nil, fmt.Errorf("unknown rubric %q (expected %s)", rubric, RubricVersion) } + // An empty findings array (a bare `[]`, `{"findings":[]}`, or a truncated + // answer file) is a no-op, not a truncating write: the write below opens with + // O_TRUNC, so proceeding would erase a prior good findings.jsonl and report + // success. Refuse it, mirroring the verdict-overwrite guard. + if len(raws) == 0 { + return nil, fmt.Errorf("answer contains no findings; refusing to overwrite %s", session.FindingsFile) + } var ( findings []Finding @@ -141,14 +149,42 @@ func decodeFinding(raw json.RawMessage) (Finding, error) { } // holdsVerdicts reports whether an existing findings.jsonl already contains any -// verdict record. A missing file is not an error (false, nil). +// verdict record. A missing file is not an error (false, nil). It scans for raw +// kind:"verdict" lines rather than reusing analyze.Load, whose verdict slice is +// filtered to the closed enum (confirmed|rejected|duplicate): a hand-edited or +// shared file whose only verdict lines carry a foreign or typo'd value would +// otherwise slip past the guard and have its human-decision records truncated by +// a re-ingest — exactly the precision history the guard exists to protect. func holdsVerdicts(dir string) (bool, error) { - _, verdicts, err := Load(dir) + path := filepath.Join(dir, session.FindingsFile) + f, err := os.Open(path) if err != nil { if errors.Is(err, os.ErrNotExist) { return false, nil } return false, err } - return len(verdicts) > 0, nil + defer f.Close() + + sc := bufio.NewScanner(f) + sc.Buffer(make([]byte, 0, 64*1024), 4*1024*1024) + for sc.Scan() { + raw := sc.Bytes() + if len(bytes.TrimSpace(raw)) == 0 { + continue + } + var probe struct { + Kind string `json:"kind"` + } + if err := json.Unmarshal(raw, &probe); err != nil { + return false, fmt.Errorf("%s: %w", path, err) + } + if probe.Kind == "verdict" { + return true, nil + } + } + if err := sc.Err(); err != nil { + return false, fmt.Errorf("%s: %w", path, err) + } + return false, nil } diff --git a/internal/analyze/validate.go b/internal/analyze/validate.go index 0f51b53..7ef0270 100644 --- a/internal/analyze/validate.go +++ b/internal/analyze/validate.go @@ -24,6 +24,7 @@ type timelineIndex struct { uttText map[string]string selectors map[string]bool routes map[string]bool + start float64 end float64 } @@ -35,7 +36,7 @@ func indexTimeline(entries []timeline.Entry) timelineIndex { selectors: map[string]bool{}, routes: map[string]bool{}, } - for _, e := range entries { + for i, e := range entries { idx.ids[e.ID] = true end := e.T switch e.Src { @@ -55,6 +56,15 @@ func indexTimeline(entries []timeline.Entry) timelineIndex { if end > idx.end { idx.end = end } + // Track the earliest entry start so the finding-time lower bound matches + // what the timeline can actually hold: an external recording whose + // creation_time predates the manifest t0 yields a negative offset + // (deriveOffset), so transcript.jsonl and the merged timeline legitimately + // carry negative session-relative times. Hard-coding 0 as the floor would + // reject a faithful finding anchored to such an utterance. + if i == 0 || e.T < idx.start { + idx.start = e.T + } } return idx } @@ -77,8 +87,15 @@ func validate(findings []Finding, idx timelineIndex) []error { seen[f.ID] = i + 1 } - if f.T < 0 || f.T > idx.end { - errs = append(errs, fmt.Errorf("%s: t %g is outside the session [0, %g]", label, f.T, idx.end)) + // The floor is the earlier of 0 and the earliest entry start: a normal + // session keeps 0 as the lower bound, while a session with negative-time + // utterances (a recording predating t0) admits a finding anchored there. + lo := 0.0 + if idx.start < lo { + lo = idx.start + } + if f.T < lo || f.T > idx.end { + errs = append(errs, fmt.Errorf("%s: t %g is outside the session [%g, %g]", label, f.T, lo, idx.end)) } if !typeSet[f.Type] { errs = append(errs, fmt.Errorf("%s: type %q not in bug|friction|inconsistency|preference|idea", label, f.Type)) diff --git a/internal/demo/demo.go b/internal/demo/demo.go index 209f600..c415726 100644 --- a/internal/demo/demo.go +++ b/internal/demo/demo.go @@ -174,20 +174,45 @@ func (s *server) appendLines(w http.ResponseWriter, r *http.Request, f *os.File, s.mu.Lock() defer s.mu.Unlock() + if err := appendRecords(f, lines); err != nil { + // The capture was not persisted; tell the client so it does not treat a + // dropped event as recorded, rather than answering 204 as if it had + // succeeded. + http.Error(w, "capture write failed", http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusNoContent) +} + +// appendFile is the subset of *os.File that appendRecords needs; a fake +// satisfies it in tests to exercise the partial-write rollback. +type appendFile interface { + io.Writer + Seek(offset int64, whence int) (int64, error) + Truncate(size int64) error +} + +// appendRecords writes each line and its terminating newline to f. os.File.Write +// gives no atomicity guarantee: a full disk fills the remaining space and returns +// a short count, so a bare Write can persist a truncated, newline-less prefix +// (e.g. `{"t":123,"kind":"cl`) before ENOSPC surfaces. That partial line would +// join the next successful write into one malformed physical record and break +// the JSONL reader for the whole file. So on any write error appendRecords +// truncates f back to the length it had before the failing write, so no partial +// line survives — the caller only reports the drop, the file stays clean. +func appendRecords(f appendFile, lines [][]byte) error { for _, l := range lines { - // Write the record and its terminating newline as a single buffer: a - // partial write (e.g. ENOSPC) can then never land a data line without - // its newline, which would join to the next successful write and produce - // one malformed physical line that later fails merge's JSONL reader. + before, err := f.Seek(0, io.SeekEnd) + if err != nil { + return err + } if _, err := f.Write(append(l, '\n')); err != nil { - // The capture was not persisted; tell the client so it does not treat - // a dropped event as recorded, rather than answering 204 as if it had - // succeeded. - http.Error(w, "capture write failed", http.StatusInternalServerError) - return + // Best-effort roll back any partial bytes; surface the original error. + f.Truncate(before) + return err } } - w.WriteHeader(http.StatusNoContent) + return nil } // compactLine canonicalises one accepted JSON value into a single newline-free diff --git a/internal/demo/demo_test.go b/internal/demo/demo_test.go index 7d92e73..38a7124 100644 --- a/internal/demo/demo_test.go +++ b/internal/demo/demo_test.go @@ -1,6 +1,7 @@ package demo import ( + "errors" "net/http" "net/http/httptest" "os" @@ -146,6 +147,58 @@ func TestAppendLinesReportsWriteError(t *testing.T) { } } +// shortWriteFile is an appendFile 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). +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) 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 +} + +// TestAppendRecordsRollsBackPartialWrite is the ENOSPC regression: a short write +// that persists a newline-less prefix must be truncated away, so the stream file +// never retains a partial line that would corrupt one physical JSONL record and +// break merge's reader for the whole file. Pre-fix appendLines wrote directly +// with no rollback, so the prefix survived. +func TestAppendRecordsRollsBackPartialWrite(t *testing.T) { + f := &shortWriteFile{} + if err := appendRecords(f, [][]byte{[]byte(`{"t":1,"kind":"click"}`)}); err != nil { + t.Fatalf("first append: %v", err) + } + good := string(f.buf) + + f.fail = true + if err := appendRecords(f, [][]byte{[]byte(`{"t":2,"kind":"click"}`)}); 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) + } +} + // TestDisplayURL pins the human-facing URL: "localhost" only for the host-less // default, and the real host otherwise, never the broken "localhost0.0.0.0:8737" // that concatenating a full -addr after the "localhost" literal produced.