From f283b36a11071facf87069a14271c5a28534e8ae Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 4 Aug 2026 20:07:01 +0200 Subject: [PATCH 01/23] docs(speakers): name the T1 test the review-state slice must replace Pre-flight scan of the plan found it mandating a behaviour change that an existing T1 test pins the opposite of: 'Keep generic dismisses the row locally' asserts the row reaches toHaveCount(0), while task 7 keeps it visible and quietly marked. The plan said nothing about it, which left an implementer to improvise, and the obvious improvisation is quietly adjusting the assertion. Task 7 now requires rewriting AND renaming that test, because the old name becomes false and a silent adjustment would disguise a product decision as test maintenance. --- .../plans/2026-08-04-speaker-review-run-provenance.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md b/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md index af96a706..9d8736e1 100644 --- a/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md +++ b/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md @@ -164,7 +164,7 @@ - Produces: `speakers.setClusterReviewState({ meetingStem, channel, diarizationSpeakerId, generic })` on the preload bridge, and `useSetClusterReviewState` in the hooks module. - [ ] **Step 1: Write the failing renderer tests.** Beside `speakerReviewOrdering.test.ts`, test the pure derivations rather than the component internals: a row whose suggestion carries `review_state: "generic"` reads as kept-generic; the notice text is produced when `stale_assignments` is non-empty and not when it is empty. Extract the derivations as exported helpers so they are testable without mounting, the same way `orderProfilesForRow` already is. -- [ ] **Step 2: Add the failing T1 assertions.** In `speaker-review.t1.spec.ts`: after clicking "Keep generic" the row stays visible and reads as kept generic; the button is absent on a confirmed row and on a mixed row. Extend `app/e2e-mock-ipc.js` with the new handler and the `review_state` echo so the mock matches the real contract. +- [ ] **Step 2: Replace the T1 test that pins the old behaviour, deliberately.** `speaker-review.t1.spec.ts` currently holds `'Keep generic dismisses the row locally, no confirm call needed'`, which asserts the row reaches `toHaveCount(0)`. This slice inverts that on purpose, so the test must be **rewritten and renamed** - its present name becomes false. The replacement asserts the new contract: after clicking, the row stays visible, reads as kept generic, and the undo is one click away. Renaming it is the point: a silently adjusted assertion would disguise a product decision as test maintenance. Then add the two new assertions - the button is absent on a confirmed row and on a mixed row - and extend `app/e2e-mock-ipc.js` with the new handler and the `review_state` echo so the mock matches the real contract. Leave the in-flight-disabled assertion around line 253 intact; it covers a different property of the same button. - [ ] **Step 3: Run both and watch them fail.** `npx vitest run` and `npm run test:e2e -- --project=t1 --grep speaker`. - [ ] **Step 4: Bridge and wire.** The `ipcMain.handle` mirrors the `mark-speaker-cluster` handler including `parsePythonFailureJson` on error; the preload entry joins the existing `speakers` group; the hook invalidates `speakersKeys.suggestions(meetingStem)`. In the panel, the button calls the mutation, and the `dismissed` state and its `notDismissed` filtering are **removed** - the marker now comes from query data, which is what makes it survive a remount by construction. Gate the button off on confirmed and mixed rows; today it renders on both because it sits outside the `!isMarked` conditional. - [ ] **Step 5: Run typecheck, lint, vitest, T1.** Compare lint against the 37/0 baseline. From ff8bcc83a22392a731ae3c2beec8d43d4adecee1 Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 4 Aug 2026 20:09:51 +0200 Subject: [PATCH 02/23] feat(speakers): stamp each diarization run with an id write_speakers_sidecar mints a diarization_run block (run_id, created_at) on every call, since every new-run producer funnels through it. The read-modify-write helpers rewrite the whole document and carry the run id forward unchanged - a rewrite of the same diarization output is not a new run. Optional key: absent sidecars and legacy documents round-trip unchanged. --- src/speaker_suggestions.py | 17 ++++++- tests/test_speaker_suggestions.py | 73 +++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 1 deletion(-) diff --git a/src/speaker_suggestions.py b/src/speaker_suggestions.py index e43b8592..60a3cf07 100644 --- a/src/speaker_suggestions.py +++ b/src/speaker_suggestions.py @@ -46,6 +46,7 @@ import subprocess import tempfile import time +import uuid from collections import Counter from dataclasses import dataclass, field from pathlib import Path @@ -582,11 +583,25 @@ def write_speakers_sidecar( "which line belongs to this cluster" via fuzzy timestamp matching after the fact -- see the plan doc's Phase 8. Omitted (not written as an empty list) when not given, so older sidecars and this key's - absence both read the same way via `.get("transcript_lines")`.""" + absence both read the same way via `.get("transcript_lines")`. + + Mints a fresh `diarization_run` (`run_id` + `created_at`) every call, + minted HERE rather than by each caller: every producer of a new run -- + the live pipeline, the Phase 3 backfill command, and a future + re-diarize -- funnels through this one function, so stamping it here + needs no new plumbing at any call site and cannot be forgotten by one. + The read-modify-write helpers (`write_sidecar_document`, + `set_cluster_multi_speaker`) rewrite the whole document instead of + calling back in here, so they carry the existing run id forward + unchanged -- a rewrite of the same diarization output is not a new + run, and re-minting on every write would make a stale review-state + key (added in a later slice) look current when the clusters it was + reviewed against never changed.""" payload = { "meeting_id": meeting_stem, "created_at": time.time(), "channels": channels, + "diarization_run": {"run_id": str(uuid.uuid4()), "created_at": time.time()}, } if turn_manifest: payload["transcript_lines"] = turn_manifest diff --git a/tests/test_speaker_suggestions.py b/tests/test_speaker_suggestions.py index 60fac8fb..6d2f5837 100644 --- a/tests/test_speaker_suggestions.py +++ b/tests/test_speaker_suggestions.py @@ -27,6 +27,7 @@ relabel_transcript_multi, relabel_transcript_speaker, score_candidates, + set_cluster_multi_speaker, suggest_speaker, suggest_speakers_for_meeting, write_speakers_sidecar, @@ -572,6 +573,78 @@ def test_read_corrupt_sidecar_returns_none(self): (output_dir / "mtg001_speakers.json").write_text("{not json") self.assertIsNone(read_speakers_sidecar(output_dir, "mtg001")) + def test_write_stamps_a_diarization_run(self): + with tempfile.TemporaryDirectory() as tmp_dir: + output_dir = Path(tmp_dir) + channels = { + "mic": { + "recording_type": "in_person", + "clusters": { + "SPEAKER_0": {"embedding": [1.0, 0.0], "speech_duration_seconds": 30.0, "segment_count": 5}, + }, + }, + } + write_speakers_sidecar(output_dir, "mtg001", channels) + loaded = read_speakers_sidecar(output_dir, "mtg001") + run = loaded["diarization_run"] + self.assertIsInstance(run["run_id"], str) + self.assertTrue(run["run_id"]) + self.assertIsInstance(run["created_at"], float) + + def test_successive_writes_mint_different_run_ids(self): + with tempfile.TemporaryDirectory() as tmp_dir: + output_dir = Path(tmp_dir) + channels = { + "mic": { + "recording_type": "in_person", + "clusters": { + "SPEAKER_0": {"embedding": [1.0, 0.0], "speech_duration_seconds": 30.0, "segment_count": 5}, + }, + }, + } + write_speakers_sidecar(output_dir, "mtg001", channels) + first_run_id = read_speakers_sidecar(output_dir, "mtg001")["diarization_run"]["run_id"] + write_speakers_sidecar(output_dir, "mtg001", channels) + second_run_id = read_speakers_sidecar(output_dir, "mtg001")["diarization_run"]["run_id"] + self.assertNotEqual(first_run_id, second_run_id) + + def test_legacy_sidecar_without_diarization_run_round_trips_unchanged(self): + with tempfile.TemporaryDirectory() as tmp_dir: + output_dir = Path(tmp_dir) + legacy = { + "meeting_id": "mtg001", + "created_at": 100.0, + "channels": { + "mic": { + "recording_type": "in_person", + "clusters": { + "SPEAKER_0": {"embedding": [1.0, 0.0], "speech_duration_seconds": 30.0, "segment_count": 5}, + }, + }, + }, + } + (output_dir / "mtg001_speakers.json").write_text(json.dumps(legacy)) + loaded = read_speakers_sidecar(output_dir, "mtg001") + self.assertEqual(loaded, legacy) + self.assertIsNone(loaded.get("diarization_run")) + + def test_rewrite_via_set_cluster_multi_speaker_preserves_run_id(self): + with tempfile.TemporaryDirectory() as tmp_dir: + output_dir = Path(tmp_dir) + channels = { + "system": { + "recording_type": "remote", + "clusters": { + "SPEAKER_0": {"embedding": [1.0, 0.0], "speech_duration_seconds": 30.0, "segment_count": 5}, + }, + }, + } + write_speakers_sidecar(output_dir, "mtg001", channels) + run_id_before = read_speakers_sidecar(output_dir, "mtg001")["diarization_run"]["run_id"] + set_cluster_multi_speaker(output_dir, "mtg001", "system", "SPEAKER_0", True) + run_id_after = read_speakers_sidecar(output_dir, "mtg001")["diarization_run"]["run_id"] + self.assertEqual(run_id_before, run_id_after) + def test_clusters_from_sidecar_channel_builds_expected_shape(self): channel = { "recording_type": "remote", From 737ffce914336d518c9a1d1bb9dd71a4a70f869c Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 4 Aug 2026 20:19:58 +0200 Subject: [PATCH 03/23] feat(speakers): add the shared run-staleness predicate prototype_run_matches decides whether a stored prototype or hard negative is still evidence about the sidecar's CURRENT diarization run's clusters. It lives once, beside prototype_channel_matches, so the read path (suggest-speakers) and the write path (run-scoped remove_speaker_evidence, both coming next) share one comparison instead of drifting apart on it. --- src/speaker_suggestions.py | 30 ++++++++++++++++++++++++++++++ tests/test_speaker_suggestions.py | 18 ++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/src/speaker_suggestions.py b/src/speaker_suggestions.py index 60a3cf07..32ed08f5 100644 --- a/src/speaker_suggestions.py +++ b/src/speaker_suggestions.py @@ -161,6 +161,36 @@ def prototype_channel_matches(prototype: dict, channel_name: str, channel_record return prototype.get("recording_type") == channel_recording_type +def prototype_run_matches(entry: dict, sidecar_run_id) -> bool: + """Is a stored prototype/hard-negative still evidence about the sidecar's + CURRENT diarization run's clusters? + + Both the read path (which prototypes may populate `confirmed_by_user`) + and the write path (`remove_speaker_evidence`'s run-scoped removal) call + this one predicate so they can never drift apart on what "still current" + means (see docs/superpowers/specs/2026-08-04-speaker-review-run-provenance-design.md + section 4/5). + + The two mixed absent/present cases are deliberately asymmetric, because + they arise from different histories rather than a coin flip: + - entry absent, sidecar present: only reachable if the meeting was + re-diarized (stamping the sidecar with a run id) AFTER the entry was + confirmed on a build that predates run stamping. The entry's clusters + are provably not this run's clusters, so it is stale -- this is the + exact hazard this slice exists to catch. + - entry present, sidecar absent: only reachable if a build WITHOUT run + stamping re-diarized a meeting whose entry was confirmed by a build + WITH it -- the reverse order from the case above, and not the one this + slice targets. Nothing proves the now-unstamped sidecar's clusters are + the confirmed run's clusters, so this is pinned stale defensively too. + Both absent stays current: pure legacy, nothing here was ever run-stamped. + """ + entry_run_id = entry.get("diarization_run_id") + if entry_run_id is None and sidecar_run_id is None: + return True + return entry_run_id == sidecar_run_id + + def build_clusters_from_diarization(segments: list, embeddings: dict) -> dict: """Group one channel's raw diarizer segments + per-speaker embeddings into the `clusters` shape `write_speakers_sidecar` expects: diff --git a/tests/test_speaker_suggestions.py b/tests/test_speaker_suggestions.py index 6d2f5837..4dcf1863 100644 --- a/tests/test_speaker_suggestions.py +++ b/tests/test_speaker_suggestions.py @@ -22,6 +22,7 @@ longest_segment, merge_same_channel_fragments, prototype_channel_matches, + prototype_run_matches, read_speakers_sidecar, relabel_transcript_exact, relabel_transcript_multi, @@ -384,6 +385,23 @@ def test_legacy_prototype_falls_back_to_recording_type(self): self.assertFalse(prototype_channel_matches(legacy, "system", "remote")) +class PrototypeRunMatchesTests(unittest.TestCase): + def test_both_absent_is_current(self): + self.assertTrue(prototype_run_matches({}, None)) + + def test_both_present_and_equal_is_current(self): + self.assertTrue(prototype_run_matches({"diarization_run_id": "run-a"}, "run-a")) + + def test_both_present_and_different_is_stale(self): + self.assertFalse(prototype_run_matches({"diarization_run_id": "run-a"}, "run-b")) + + def test_entry_absent_sidecar_present_is_stale(self): + self.assertFalse(prototype_run_matches({}, "run-a")) + + def test_entry_present_sidecar_absent_is_stale(self): + self.assertFalse(prototype_run_matches({"diarization_run_id": "run-a"}, None)) + + class SuggestSpeakersForMeetingTests(unittest.TestCase): def test_same_person_not_suggested_for_two_clusters(self): # Two clusters both plausibly Max; only the closer one should claim From 6e0b8629f4dbda0d24b046bc429838a05ea9ba46 Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 4 Aug 2026 20:23:52 +0200 Subject: [PATCH 04/23] docs(speakers): correct why the asymmetric staleness rows are reachable Review of the predicate found the reasoning understated: an unstamped prototype against a stamped sidecar is not the pre-stamping-build case, it is the ordinary upgrade path - confirming against a still-legacy sidecar stores no id even on a current build, and the meeting's first re-diarization then stamps it. The verdict was right, the explanation would have sent a later reader looking for an exotic history. Corrected in the spec rather than only in the ledger, because the later tasks copy this reasoning into their own call sites. --- .../specs/2026-08-04-speaker-review-run-provenance-design.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/superpowers/specs/2026-08-04-speaker-review-run-provenance-design.md b/docs/superpowers/specs/2026-08-04-speaker-review-run-provenance-design.md index 7ceb9a49..ca34b61f 100644 --- a/docs/superpowers/specs/2026-08-04-speaker-review-run-provenance-design.md +++ b/docs/superpowers/specs/2026-08-04-speaker-review-run-provenance-design.md @@ -87,8 +87,8 @@ The rule is deliberately ASYMMETRIC because of how the two ids can come to disag | absent | absent | current | pure legacy, nothing was ever re-diarized with run stamping | | present | present, equal | current | confirmed against exactly this run | | present | present, different | stale | confirmed against a different run's clusters | -| absent | present | stale | can only arise if the recording was re-diarized after the confirmation - the confirm predates run stamping, the sidecar postdates it | -| present | absent | stale | defensive: only reachable if a build without run stamping re-diarized after a stamped confirm; the sidecar's clusters are then not provably the confirm-time run | +| absent | present | stale | a fresh run happened after the confirmation. Note this is the ordinary upgrade path, not an exotic one: confirming against a still-legacy sidecar stores no id even on a stamped build, and the meeting's first re-diarization then stamps the sidecar | +| present | absent | stale | defensive: reachable through a build without run stamping, or through a restored `.bak` sidecar. Either way the sidecar's clusters are not provably the confirm-time run | The rule lives as one shared predicate in `src/speaker_suggestions.py` (beside `prototype_channel_matches`, which `src/config.py` already imports the same way), so the read path and the write path below cannot drift apart. From 8035d659fcefdcb24fe0936075c12c7c4f74eb88 Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 4 Aug 2026 20:30:20 +0200 Subject: [PATCH 05/23] feat(speakers): stamp confirm-speaker's evidence with its diarization run id add_speaker_prototype gains an optional diarization_run_id, written only when not None (same absent-means-legacy convention as channel). confirm- speaker reads the sidecar's diarization_run.run_id once and threads it through every add_speaker_prototype call it makes: the positive prototype and both directions of the mutual hard negatives. Gives Task 2's prototype_run_matches predicate something to compare against. --- simple_recorder.py | 13 +++++-- src/config.py | 13 ++++++- tests/test_config.py | 30 ++++++++++++++ tests/test_confirm_speaker_cli.py | 65 ++++++++++++++++++++++++++++++- 4 files changed, 116 insertions(+), 5 deletions(-) diff --git a/simple_recorder.py b/simple_recorder.py index dd816090..588ca48d 100644 --- a/simple_recorder.py +++ b/simple_recorder.py @@ -4942,6 +4942,13 @@ def confirm_speaker(meeting_stem, channel, diarization_speaker_id, person_id, ne print(json.dumps({"success": False, "error": f"No {channel!r} channel in sidecar for {meeting_stem!r}"})) sys.exit(1) + # Read once from the loaded sidecar rather than re-reading per call -- + # every prototype and hard negative this command writes below describes + # a cluster from THIS sidecar, so they all carry the same run id. `None` + # on a legacy sidecar with no diarization_run block, which every + # add_speaker_prototype call already treats as "no run to record". + run_id = (sidecar.get("diarization_run") or {}).get("run_id") + raw_clusters = clusters_from_sidecar_channel(meeting_stem, channel_data) if diarization_speaker_id not in raw_clusters: print(json.dumps({ @@ -5052,7 +5059,7 @@ def confirm_speaker(meeting_stem, channel, diarization_speaker_id, person_id, ne speech_duration_seconds=context.speech_duration_seconds, segment_count=context.segment_count, created_from="user_corrected" if reassigned_from else "user_confirmed", - channel=channel, + channel=channel, diarization_run_id=run_id, ) # Mutual hard negatives against any OTHER speaker already confirmed in @@ -5127,7 +5134,7 @@ def confirm_speaker(meeting_stem, channel, diarization_speaker_id, person_id, ne speech_duration_seconds=other_context.speech_duration_seconds, segment_count=other_context.segment_count, created_from="user_confirmed", negative=True, - channel=channel, + channel=channel, diarization_run_id=run_id, ) # And exactly ONE the other way: THIS cluster is a single piece of # evidence about them, however many clusters they own. Adding it per @@ -5140,7 +5147,7 @@ def confirm_speaker(meeting_stem, channel, diarization_speaker_id, person_id, ne speech_duration_seconds=context.speech_duration_seconds, segment_count=context.segment_count, created_from="user_confirmed", negative=True, - channel=channel, + channel=channel, diarization_run_id=run_id, ) hard_negatives_added.append(other_person["display_name"]) diff --git a/src/config.py b/src/config.py index 83a21a01..cb22ea50 100644 --- a/src/config.py +++ b/src/config.py @@ -1218,6 +1218,7 @@ def add_speaker_prototype( created_from: str, channel: Optional[str] = None, negative: bool = False, + diarization_run_id: Optional[str] = None, ) -> Optional[dict]: """Append a `SpeakerPrototype` to a person's positive `prototypes` (default) or `hard_negatives` (`negative=True`) list. Returns None @@ -1236,7 +1237,15 @@ def add_speaker_prototype( from the wrong channel's clusters. `None` is allowed only for legacy/enrollment paths with no channel to record — matchers fall back to `recording_type` as a channel proxy for those (see - src.speaker_suggestions.prototype_channel_matches).""" + src.speaker_suggestions.prototype_channel_matches). + + `diarization_run_id` is the sidecar's `diarization_run.run_id` the + embedding was confirmed from. Same absent-means-legacy convention as + `channel`: it must be stored so a later run can tell "this evidence + is from the diarization output currently on disk" from "the + clusters have since been re-diarized and this entry's ids may not + mean what they used to" (src.speaker_suggestions.prototype_run_matches). + `None` for callers with no run to report, e.g. enrollment.""" profile = self.get_person_profile(person_id) if profile is None: return None @@ -1263,6 +1272,8 @@ def add_speaker_prototype( } if channel is not None: prototype["channel"] = channel + if diarization_run_id is not None: + prototype["diarization_run_id"] = diarization_run_id key = "hard_negatives" if negative else "prototypes" profile.setdefault(key, []).append(prototype) profile["updated_at"] = time.time() diff --git a/tests/test_config.py b/tests/test_config.py index 26caddcc..1b4356ab 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1002,6 +1002,36 @@ def test_add_speaker_prototype_omits_channel_when_none(self): ) self.assertNotIn("channel", prototype) + def test_add_speaker_prototype_stores_diarization_run_id_when_given(self): + with tempfile.TemporaryDirectory() as tmp_dir: + config = Config(config_path=Path(tmp_dir) / "config.json") + person = config.create_person_profile("Max") + prototype = config.add_speaker_prototype( + person["person_id"], [0.1, 0.2], + recording_type="in_person", meeting_id="mtg001", + diarization_speaker_id="SPEAKER_00", + speech_duration_seconds=25.0, segment_count=4, + created_from="user_confirmed", diarization_run_id="r1", + ) + self.assertEqual(prototype["diarization_run_id"], "r1") + + def test_add_speaker_prototype_omits_diarization_run_id_when_none(self): + # Same absent-means-legacy convention as `channel`: a prototype + # written before this field existed, or from a caller that has no + # run to report, must read exactly like one written today with no + # run id -- not like one that carries an explicit `None`. + with tempfile.TemporaryDirectory() as tmp_dir: + config = Config(config_path=Path(tmp_dir) / "config.json") + person = config.create_person_profile("Max") + prototype = config.add_speaker_prototype( + person["person_id"], [0.1, 0.2], + recording_type="in_person", meeting_id="mtg001", + diarization_speaker_id="SPEAKER_00", + speech_duration_seconds=25.0, segment_count=4, + created_from="user_confirmed", + ) + self.assertNotIn("diarization_run_id", prototype) + def test_add_speaker_prototype_rejects_invalid_channel(self): with tempfile.TemporaryDirectory() as tmp_dir: config = Config(config_path=Path(tmp_dir) / "config.json") diff --git a/tests/test_confirm_speaker_cli.py b/tests/test_confirm_speaker_cli.py index b68c3b4e..b8fbc546 100644 --- a/tests/test_confirm_speaker_cli.py +++ b/tests/test_confirm_speaker_cli.py @@ -1,5 +1,6 @@ import json import tempfile +import time import unittest from pathlib import Path from unittest import mock @@ -8,7 +9,7 @@ import simple_recorder from src.config import Config -from src.speaker_suggestions import write_speakers_sidecar +from src.speaker_suggestions import read_speakers_sidecar, write_speakers_sidecar def _last_json(output): @@ -147,6 +148,68 @@ def test_second_confirmation_in_same_meeting_creates_mutual_hard_negatives(self) self.assertEqual(len(sarah_profile["hard_negatives"]), 1) self.assertEqual(sarah_profile["hard_negatives"][0]["embedding_mean"], [1.0, 0.0]) + def test_confirm_stamps_prototypes_and_hard_negatives_with_the_sidecars_run_id(self): + with tempfile.TemporaryDirectory() as tmp: + self._seed_sidecar(tmp) + output_dir = Path(tmp) / "output" + run_id = read_speakers_sidecar(output_dir, "mtg001")["diarization_run"]["run_id"] + cfg = Config(config_path=Path(tmp) / "config.json") + + result1, cfg = self._run(["mtg001", "mic", "SPEAKER_00", "--new-person", "Max"], tmp, cfg=cfg) + max_id = _last_json(result1.output)["person_id"] + result2, cfg = self._run(["mtg001", "mic", "SPEAKER_01", "--new-person", "Sarah"], tmp, cfg=cfg) + sarah_id = _last_json(result2.output)["person_id"] + + max_profile = cfg.get_person_profile(max_id) + sarah_profile = cfg.get_person_profile(sarah_id) + # Positive prototype and mutual hard negative both carry it, on + # both sides -- every add_speaker_prototype call this command + # makes is expected to thread the same id. + self.assertEqual(max_profile["prototypes"][0]["diarization_run_id"], run_id) + self.assertEqual(max_profile["hard_negatives"][0]["diarization_run_id"], run_id) + self.assertEqual(sarah_profile["prototypes"][0]["diarization_run_id"], run_id) + self.assertEqual(sarah_profile["hard_negatives"][0]["diarization_run_id"], run_id) + + def test_confirm_against_legacy_sidecar_produces_prototypes_without_run_id(self): + with tempfile.TemporaryDirectory() as tmp: + self._seed_legacy_sidecar(tmp) + cfg = Config(config_path=Path(tmp) / "config.json") + + result1, cfg = self._run(["mtg001", "mic", "SPEAKER_00", "--new-person", "Max"], tmp, cfg=cfg) + max_id = _last_json(result1.output)["person_id"] + result2, cfg = self._run(["mtg001", "mic", "SPEAKER_01", "--new-person", "Sarah"], tmp, cfg=cfg) + sarah_id = _last_json(result2.output)["person_id"] + + max_profile = cfg.get_person_profile(max_id) + sarah_profile = cfg.get_person_profile(sarah_id) + self.assertNotIn("diarization_run_id", max_profile["prototypes"][0]) + self.assertNotIn("diarization_run_id", max_profile["hard_negatives"][0]) + self.assertNotIn("diarization_run_id", sarah_profile["prototypes"][0]) + self.assertNotIn("diarization_run_id", sarah_profile["hard_negatives"][0]) + + def _seed_legacy_sidecar(self, tmp, meeting_stem="mtg001"): + # A sidecar written before diarization_run existed -- no top-level + # "diarization_run" key at all, not one holding None. Written by + # hand rather than through write_speakers_sidecar, which always + # stamps a run now. + output_dir = Path(tmp) / "output" + output_dir.mkdir(parents=True, exist_ok=True) + payload = { + "meeting_id": meeting_stem, + "created_at": time.time(), + "channels": { + "mic": { + "recording_type": "in_person", + "clusters": { + "SPEAKER_00": {"embedding": [1.0, 0.0], "speech_duration_seconds": 30.0, "segment_count": 5}, + "SPEAKER_01": {"embedding": [0.0, 1.0], "speech_duration_seconds": 25.0, "segment_count": 4}, + }, + }, + }, + } + (output_dir / f"{meeting_stem}_speakers.json").write_text(json.dumps(payload)) + return output_dir + def _seed_three_cluster_sidecar(self, tmp, meeting_stem="mtg001"): """One channel, three clusters -- the shape that appears as soon as the diarizer splits one person across two clusters, which is the normal From c3db7ae5aaef30c6db260837bc3cf1ab196070be Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 4 Aug 2026 20:43:42 +0200 Subject: [PATCH 06/23] fix(speakers): stop a re-diarization from deleting the last run's prototype remove_speaker_evidence matched on (meeting_id, channel, sids) alone. A re-diarization numbers its clusters from SPEAKER_0 again with no memory of who held that id before, so confirming the new run's first cluster as someone deleted the prototype the previous run's confirmation had recorded against a genuinely different voice - silently, and only visible months later as a worse suggestion. The removal now takes an optional run scope, and every caller that works from a sidecar (confirm-speaker's reassignment, negative-cleanup and idempotency-rebuild removals, mark-speaker-cluster's withdrawal loop) passes that sidecar's run id. delete_person_profile passes each source prototype's own run id, since the negatives it cleans up were written by the same confirm. The default is a distinct sentinel, not None: None is itself a scope ("the sidecar reports no run") and must match only equally run-less evidence, while callers with no sidecar in hand keep today's unscoped behaviour exactly. The trade this accepts: a confirmation made against a superseded run can no longer be corrected by re-confirming the same cluster id, because the two are no longer the same cluster - such entries have to go through the repair CLI's by-id removal. Destroying genuine evidence is the worse failure of the two, and it is the one happening today. Three mark-speaker-cluster tests hand-built a prototype against a run-stamped sidecar without stamping it, describing a state a real confirm cannot produce; they now stamp the seeded run id. --- simple_recorder.py | 31 +++++++++--- src/config.py | 54 +++++++++++++++++++- tests/test_config.py | 77 ++++++++++++++++++++++++++++- tests/test_confirm_speaker_cli.py | 70 ++++++++++++++++++++++++++ tests/test_speaker_multi_marking.py | 14 +++++- 5 files changed, 235 insertions(+), 11 deletions(-) diff --git a/simple_recorder.py b/simple_recorder.py index 588ca48d..0424f0a3 100644 --- a/simple_recorder.py +++ b/simple_recorder.py @@ -5013,12 +5013,20 @@ def confirm_speaker(meeting_stem, channel, diarization_speaker_id, person_id, ne # rebuilds correct negatives against the people still confirmed in this # channel. A same-person re-confirm just replaces the prototype instead # of appending a duplicate. + # + # Every removal here is scoped to THIS sidecar's run, because the cluster + # ids only identify a voice within one diarization run -- a re-diarization + # renumbers from SPEAKER_0 with no memory of who held that id, so + # unscoped these removals would treat a stranger's confirmation as this + # cluster's previous owner and delete it. The cost is that a + # confirmation made against a superseded run can no longer be corrected + # by re-confirming; the repair CLI removes such entries by id. reassigned_from = [] for existing_person in config.get_person_profiles(): removed = config.remove_speaker_evidence( existing_person["person_id"], meeting_id=meeting_stem, channel=channel, channel_recording_type=channel_recording_type, - sids=fragment_ids, + sids=fragment_ids, diarization_run_id=run_id, ) if not removed or existing_person["person_id"] == person["person_id"]: continue @@ -5041,7 +5049,7 @@ def confirm_speaker(meeting_stem, channel, diarization_speaker_id, person_id, ne config.remove_speaker_evidence( existing_person["person_id"], meeting_id=meeting_stem, channel=channel, channel_recording_type=channel_recording_type, - negative=True, + negative=True, diarization_run_id=run_id, ) for other in config.get_person_profiles(): if other["person_id"] == existing_person["person_id"]: @@ -5049,7 +5057,7 @@ def confirm_speaker(meeting_stem, channel, diarization_speaker_id, person_id, ne config.remove_speaker_evidence( other["person_id"], meeting_id=meeting_stem, channel=channel, channel_recording_type=channel_recording_type, - sids=fragment_ids, negative=True, + sids=fragment_ids, negative=True, diarization_run_id=run_id, ) prototype = config.add_speaker_prototype( @@ -5096,12 +5104,12 @@ def confirm_speaker(meeting_stem, channel, diarization_speaker_id, person_id, ne config.remove_speaker_evidence( existing_person["person_id"], meeting_id=meeting_stem, channel=channel, channel_recording_type=channel_recording_type, - sids=fragment_ids, negative=True, + sids=fragment_ids, negative=True, diarization_run_id=run_id, ) config.remove_speaker_evidence( person["person_id"], meeting_id=meeting_stem, channel=channel, channel_recording_type=channel_recording_type, - negative=True, + negative=True, diarization_run_id=run_id, ) hard_negatives_added = [] @@ -5296,6 +5304,10 @@ def mark_speaker_cluster(meeting_stem, channel, diarization_speaker_id, multiple # the CLI honest about what just happened. channel_data = (sidecar.get("channels") or {}).get(channel) or {} channel_recording_type = channel_data.get("recording_type") + # From the rewritten document set_cluster_multi_speaker returned, which + # carries the existing run forward -- marking a cluster is an annotation + # of this diarization output, not a new one. `None` on a legacy sidecar. + run_id = (sidecar.get("diarization_run") or {}).get("run_id") clusters, id_resolution = merge_same_channel_fragments( clusters_from_sidecar_channel(meeting_stem, channel_data) ) @@ -5323,11 +5335,14 @@ def mark_speaker_cluster(meeting_stem, channel, diarization_speaker_id, multiple cleared_from = [] restored_lines = 0 if multiple and fragment_ids: + # Run-scoped for the same reason the confirm path is: the cluster + # this marking describes exists only within this run, so an entry + # from a superseded run shares nothing with it but a reused id. for person in config.get_person_profiles(): removed = config.remove_speaker_evidence( person["person_id"], meeting_id=meeting_stem, channel=channel, channel_recording_type=channel_recording_type, - sids=fragment_ids, + sids=fragment_ids, diarization_run_id=run_id, ) if not removed: continue @@ -5341,7 +5356,7 @@ def mark_speaker_cluster(meeting_stem, channel, diarization_speaker_id, multiple config.remove_speaker_evidence( person["person_id"], meeting_id=meeting_stem, channel=channel, channel_recording_type=channel_recording_type, - sids=fragment_ids, negative=True, + sids=fragment_ids, negative=True, diarization_run_id=run_id, ) for other in config.get_person_profiles(): if other["person_id"] == person["person_id"]: @@ -5349,7 +5364,7 @@ def mark_speaker_cluster(meeting_stem, channel, diarization_speaker_id, multiple config.remove_speaker_evidence( other["person_id"], meeting_id=meeting_stem, channel=channel, channel_recording_type=channel_recording_type, - sids=fragment_ids, negative=True, + sids=fragment_ids, negative=True, diarization_run_id=run_id, ) if cleared_from: # The transcript is the artefact a human reads, and the one the diff --git a/src/config.py b/src/config.py index cb22ea50..44f19ab1 100644 --- a/src/config.py +++ b/src/config.py @@ -167,6 +167,25 @@ def is_apple_silicon() -> bool: return sys.platform == "darwin" and platform.machine() in ("arm64", "aarch64") +class _AnyDiarizationRun: + """The "no run scope at all" default of `remove_speaker_evidence`. + + A sentinel rather than `None`, because `None` is already a meaningful + scope on this axis: it is what a legacy sidecar with no `diarization_run` + block reports, and scoping to it must match only equally run-less + evidence. Sharing one value for "don't filter by run" and "filter by the + absence of a run" would either make every run-unaware caller start + filtering or make a legacy-sidecar caller delete run-stamped evidence it + cannot have produced. + """ + + def __repr__(self) -> str: + return "ANY_DIARIZATION_RUN" + + +ANY_DIARIZATION_RUN = _AnyDiarizationRun() + + class Config: """Manages application configuration with file persistence.""" @@ -1171,6 +1190,13 @@ def delete_person_profile(self, person_id: str) -> bool: and reuses `remove_speaker_evidence` (the same removal primitive the correction path already relies on) to strip any hard-negative entry in another profile derived from that specific confirmation. + + Each prototype's OWN `diarization_run_id` is the run scope for its + cleanup, not the meeting's current one: the negatives it produced + were written by the same confirm and therefore carry the same run + id, while a later re-diarization's negatives about the same cluster + id describe a different voice and belong to whoever is still + confirmed there. """ profiles = self._config.get("person_profiles", []) target = next((p for p in profiles if p.get("person_id") == person_id), None) @@ -1190,6 +1216,7 @@ def delete_person_profile(self, person_id: str) -> bool: channel=proto.get("channel"), channel_recording_type=proto.get("recording_type"), sids={sid}, negative=True, + diarization_run_id=proto.get("diarization_run_id"), ) remaining = [p for p in profiles if p.get("person_id") != person_id] @@ -1289,6 +1316,7 @@ def remove_speaker_evidence( channel_recording_type: Optional[str], sids: Optional[set] = None, negative: bool = False, + diarization_run_id=ANY_DIARIZATION_RUN, ) -> int: """Remove a person's positive prototypes (or hard negatives, with `negative=True`) belonging to one meeting+channel, optionally @@ -1305,8 +1333,28 @@ def remove_speaker_evidence( fallback rule as everything else (src.speaker_suggestions.prototype_channel_matches), so legacy entries without a channel field are covered too. + + `diarization_run_id` narrows that correction to evidence from ONE + diarization run (src.speaker_suggestions.prototype_run_matches). + Callers working from a sidecar must pass its run id, because + `(meeting_id, channel, sid)` is not stable across runs: a + re-diarization numbers its clusters from SPEAKER_0 again with no + memory of who held that id before, so without this scope confirming + the new run's first cluster deletes the prototype an earlier run's + confirmation recorded against a genuinely different voice. + `ANY_DIARIZATION_RUN` (the default) removes regardless of run, which + is what every caller that has no sidecar in hand -- the repair CLI's + analysis, enrollment cleanup -- still needs; passing `None` is the + distinct "the sidecar reports no run" scope, not the absence of one. + + The trade this scope accepts: a confirmation made against a + superseded run can no longer be corrected by re-confirming the same + cluster id, since the two are no longer recognised as the same + cluster. That evidence has to be dropped through the repair CLI's + by-id removal instead. Silently destroying a genuine prototype is + the worse failure of the two, and it is the one happening today. """ - from src.speaker_suggestions import prototype_channel_matches + from src.speaker_suggestions import prototype_channel_matches, prototype_run_matches profile = self.get_person_profile(person_id) if profile is None: @@ -1319,6 +1367,10 @@ def remove_speaker_evidence( entry.get("meeting_id") == meeting_id and prototype_channel_matches(entry, channel, channel_recording_type) and (sids is None or entry.get("diarization_speaker_id") in sids) + and ( + diarization_run_id is ANY_DIARIZATION_RUN + or prototype_run_matches(entry, diarization_run_id) + ) ) ] removed = len(entries) - len(kept) diff --git a/tests/test_config.py b/tests/test_config.py index 1b4356ab..1fd628b8 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1046,13 +1046,14 @@ def test_add_speaker_prototype_rejects_invalid_channel(self): ) def _add(self, config, person_id, meeting_id, sid, channel=None, - recording_type="in_person", negative=False): + recording_type="in_person", negative=False, diarization_run_id=None): return config.add_speaker_prototype( person_id, [0.1, 0.2], recording_type=recording_type, meeting_id=meeting_id, diarization_speaker_id=sid, speech_duration_seconds=25.0, segment_count=4, created_from="user_confirmed", channel=channel, negative=negative, + diarization_run_id=diarization_run_id, ) def test_remove_speaker_evidence_scopes_to_meeting_and_channel(self): @@ -1105,6 +1106,80 @@ def test_remove_speaker_evidence_matches_legacy_entries_via_recording_type(self) ) self.assertEqual(removed, 1) + def test_remove_speaker_evidence_without_a_run_scope_ignores_run_ids(self): + # The sentinel default is what every pre-existing caller relies on: + # omitting the parameter must not start filtering, or the repair and + # correction paths that never learned about runs would quietly stop + # removing anything. + with tempfile.TemporaryDirectory() as tmp_dir: + config = Config(config_path=Path(tmp_dir) / "config.json") + pid = config.create_person_profile("Max")["person_id"] + self._add(config, pid, "mtg001", "SPEAKER_00", channel="mic", diarization_run_id="r1") + self._add(config, pid, "mtg001", "SPEAKER_01", channel="mic", diarization_run_id="r2") + self._add(config, pid, "mtg001", "SPEAKER_02", channel="mic") + removed = config.remove_speaker_evidence( + pid, meeting_id="mtg001", channel="mic", + channel_recording_type="in_person", + ) + self.assertEqual(removed, 3) + self.assertEqual(config.get_person_profile(pid)["prototypes"], []) + + def test_remove_speaker_evidence_scoped_to_a_run_spares_other_runs(self): + with tempfile.TemporaryDirectory() as tmp_dir: + config = Config(config_path=Path(tmp_dir) / "config.json") + pid = config.create_person_profile("Max")["person_id"] + self._add(config, pid, "mtg001", "SPEAKER_00", channel="mic", diarization_run_id="r1") + self._add(config, pid, "mtg001", "SPEAKER_00", channel="mic", diarization_run_id="r2") + self._add(config, pid, "mtg001", "SPEAKER_00", channel="mic") + removed = config.remove_speaker_evidence( + pid, meeting_id="mtg001", channel="mic", + channel_recording_type="in_person", + sids={"SPEAKER_00"}, diarization_run_id="r2", + ) + self.assertEqual(removed, 1) + remaining = config.get_person_profile(pid)["prototypes"] + self.assertEqual( + [p.get("diarization_run_id") for p in remaining], ["r1", None], + ) + + def test_remove_speaker_evidence_scoped_to_none_only_removes_run_less_entries(self): + # An explicit None is a real scope ("the sidecar reports no run"), not + # the absence of one. Collapsing the two would make a caller working + # against a legacy sidecar delete run-stamped evidence it cannot have + # produced. + with tempfile.TemporaryDirectory() as tmp_dir: + config = Config(config_path=Path(tmp_dir) / "config.json") + pid = config.create_person_profile("Max")["person_id"] + self._add(config, pid, "mtg001", "SPEAKER_00", channel="mic", diarization_run_id="r1") + self._add(config, pid, "mtg001", "SPEAKER_00", channel="mic") + removed = config.remove_speaker_evidence( + pid, meeting_id="mtg001", channel="mic", + channel_recording_type="in_person", + sids={"SPEAKER_00"}, diarization_run_id=None, + ) + self.assertEqual(removed, 1) + remaining = config.get_person_profile(pid)["prototypes"] + self.assertEqual([p.get("diarization_run_id") for p in remaining], ["r1"]) + + def test_delete_person_profile_scopes_negative_cleanup_to_each_prototypes_run(self): + # The negatives a confirm creates carry that confirm's run id, so the + # cleanup that follows the deleted person's prototypes must not reach + # past them into a later run's evidence about a still-existing person. + with tempfile.TemporaryDirectory() as tmp_dir: + config = Config(config_path=Path(tmp_dir) / "config.json") + max_id = config.create_person_profile("Max")["person_id"] + sarah_id = config.create_person_profile("Sarah")["person_id"] + self._add(config, max_id, "mtg001", "SPEAKER_00", channel="mic", diarization_run_id="r1") + # Sarah's negative derived from Max's r1 confirm, plus one from a + # later re-diarization that has nothing to do with him. + self._add(config, sarah_id, "mtg001", "SPEAKER_00", channel="mic", + negative=True, diarization_run_id="r1") + self._add(config, sarah_id, "mtg001", "SPEAKER_00", channel="mic", + negative=True, diarization_run_id="r2") + self.assertTrue(config.delete_person_profile(max_id)) + remaining = config.get_person_profile(sarah_id)["hard_negatives"] + self.assertEqual([n["diarization_run_id"] for n in remaining], ["r2"]) + def test_remove_speaker_evidence_missing_person_returns_zero(self): with tempfile.TemporaryDirectory() as tmp_dir: config = Config(config_path=Path(tmp_dir) / "config.json") diff --git a/tests/test_confirm_speaker_cli.py b/tests/test_confirm_speaker_cli.py index b8fbc546..ef5cbdd7 100644 --- a/tests/test_confirm_speaker_cli.py +++ b/tests/test_confirm_speaker_cli.py @@ -187,6 +187,76 @@ def test_confirm_against_legacy_sidecar_produces_prototypes_without_run_id(self) self.assertNotIn("diarization_run_id", sarah_profile["prototypes"][0]) self.assertNotIn("diarization_run_id", sarah_profile["hard_negatives"][0]) + def _rediarize(self, tmp, meeting_stem="mtg001"): + """Overwrite the sidecar with a run whose cluster ids are the same but + whose voices are not -- exactly what a re-diarization produces, since + the diarizer numbers from SPEAKER_00 every time with no memory of who + held that id before. Returns the new run id.""" + output_dir = Path(tmp) / "output" + write_speakers_sidecar(output_dir, meeting_stem, { + "mic": { + "recording_type": "in_person", + "clusters": { + "SPEAKER_00": {"embedding": [0.0, 1.0], "speech_duration_seconds": 28.0, "segment_count": 5}, + "SPEAKER_01": {"embedding": [1.0, 0.0], "speech_duration_seconds": 22.0, "segment_count": 4}, + }, + }, + }) + return read_speakers_sidecar(output_dir, meeting_stem)["diarization_run"]["run_id"] + + def test_confirming_a_reused_cluster_id_from_a_newer_run_spares_the_older_runs_person(self): + with tempfile.TemporaryDirectory() as tmp: + self._seed_sidecar(tmp) + output_dir = Path(tmp) / "output" + run1 = read_speakers_sidecar(output_dir, "mtg001")["diarization_run"]["run_id"] + cfg = Config(config_path=Path(tmp) / "config.json") + + result1, cfg = self._run(["mtg001", "mic", "SPEAKER_00", "--new-person", "Max"], tmp, cfg=cfg) + max_id = _last_json(result1.output)["person_id"] + + run2 = self._rediarize(tmp) + self.assertNotEqual(run1, run2) + + # Same id, genuinely different voice. Nothing here supersedes + # Max's confirmation -- it was made about a cluster that no longer + # exists, not about this one. + result2, cfg = self._run(["mtg001", "mic", "SPEAKER_00", "--new-person", "Sarah"], tmp, cfg=cfg) + data2 = _last_json(result2.output) + self.assertTrue(data2["success"]) + self.assertEqual(data2["reassigned_from"], []) + + max_profile = cfg.get_person_profile(max_id) + self.assertEqual(len(max_profile["prototypes"]), 1) + self.assertEqual(max_profile["prototypes"][0]["diarization_run_id"], run1) + self.assertEqual(max_profile["prototypes"][0]["embedding_mean"], [1.0, 0.0]) + + sarah_profile = cfg.get_person_profile(data2["person_id"]) + self.assertEqual(len(sarah_profile["prototypes"]), 1) + self.assertEqual(sarah_profile["prototypes"][0]["diarization_run_id"], run2) + self.assertEqual(sarah_profile["prototypes"][0]["embedding_mean"], [0.0, 1.0]) + + def test_reconfirming_a_cluster_on_a_legacy_sidecar_still_supersedes(self): + # The correction path on a library that predates run stamping: with no + # run id anywhere, re-confirming is still the "Change" flow and must + # take the prototype off the person who no longer owns the cluster. + with tempfile.TemporaryDirectory() as tmp: + self._seed_legacy_sidecar(tmp) + cfg = Config(config_path=Path(tmp) / "config.json") + + result1, cfg = self._run(["mtg001", "mic", "SPEAKER_00", "--new-person", "Max"], tmp, cfg=cfg) + max_id = _last_json(result1.output)["person_id"] + + self._seed_legacy_sidecar(tmp) # rewritten, still no run block + result2, cfg = self._run(["mtg001", "mic", "SPEAKER_00", "--new-person", "Sarah"], tmp, cfg=cfg) + data2 = _last_json(result2.output) + self.assertTrue(data2["success"]) + self.assertEqual(data2["reassigned_from"], ["Max"]) + + self.assertEqual(cfg.get_person_profile(max_id)["prototypes"], []) + sarah_profile = cfg.get_person_profile(data2["person_id"]) + self.assertEqual(len(sarah_profile["prototypes"]), 1) + self.assertNotIn("diarization_run_id", sarah_profile["prototypes"][0]) + def _seed_legacy_sidecar(self, tmp, meeting_stem="mtg001"): # A sidecar written before diarization_run existed -- no top-level # "diarization_run" key at all, not one holding None. Written by diff --git a/tests/test_speaker_multi_marking.py b/tests/test_speaker_multi_marking.py index d29fcbb1..d565025a 100644 --- a/tests/test_speaker_multi_marking.py +++ b/tests/test_speaker_multi_marking.py @@ -1048,6 +1048,14 @@ def _run(self, command, args, tmp, cfg=None): mock.patch.dict("os.environ", {"STENOAI_USER_DATA_DIR": tmp}): return CliRunner().invoke(command, args) + def _seeded_run_id(self, tmp): + """The seeded sidecar's run id. Tests that hand-build a prototype + instead of going through `confirm-speaker` have to stamp it, or they + describe a state the app cannot reach: evidence about a run that is + not the one on disk, which the withdrawal path deliberately leaves + alone because its cluster ids mean something else now.""" + return read_speakers_sidecar(Path(tmp) / "output", "mtg001")["diarization_run"]["run_id"] + def test_marks_and_reports_the_new_minimum_speaker_count(self): with tempfile.TemporaryDirectory() as tmp: self._seed(tmp) @@ -1074,17 +1082,19 @@ def test_marking_one_cluster_keeps_the_negatives_earned_by_the_others(self): person = cfg.create_person_profile("Julian") pid = person["person_id"] # Confirmed on SPEAKER_0 (the cluster about to be marked)... + run_id = self._seeded_run_id(tmp) cfg.add_speaker_prototype( pid, [1.0, 0.0], recording_type="remote", meeting_id="mtg001", diarization_speaker_id="SPEAKER_0", speech_duration_seconds=60.0, segment_count=10, created_from="user_confirmed", channel="system", + diarization_run_id=run_id, ) # ...and ruled OUT for SPEAKER_1 by a different confirmation. cfg.add_speaker_prototype( pid, [0.0, 1.0], recording_type="remote", meeting_id="mtg001", diarization_speaker_id="SPEAKER_1", speech_duration_seconds=40.0, segment_count=8, created_from="user_confirmed", channel="system", - negative=True, + negative=True, diarization_run_id=run_id, ) result = self._run( @@ -1187,6 +1197,7 @@ def test_without_a_recorded_original_the_line_says_multiple_speakers(self): meeting_id="mtg001", diarization_speaker_id="SPEAKER_00", speech_duration_seconds=30.0, segment_count=5, created_from="user_confirmed", channel="mic", + diarization_run_id=self._seeded_run_id(tmp), ) result = self._run( simple_recorder.mark_speaker_cluster, @@ -1217,6 +1228,7 @@ def test_a_manifest_that_no_longer_fits_leaves_the_transcript_alone(self): meeting_id="mtg001", diarization_speaker_id="SPEAKER_00", speech_duration_seconds=30.0, segment_count=5, created_from="user_confirmed", channel="mic", + diarization_run_id=self._seeded_run_id(tmp), ) result = self._run( simple_recorder.mark_speaker_cluster, From 95ac55053967667cf9bd6d01ace0de7d5d914f00 Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 4 Aug 2026 21:00:33 +0200 Subject: [PATCH 07/23] test(speakers): assert the run scope at every removal, not just the first The mark-speaker-cluster withdrawal loop and four of confirm-speaker's five removals passed the run id with nothing asserting it: deleting the argument left the whole suite green, because the fixtures stamped in the previous commit carry the matching id and no test built a stale-run entry for those paths. Mutating each of the eight call sites individually now fails a test. New: mark-speaker-cluster leaves an older run's confirmation alone when the reused cluster id is marked mixed, and withdraws only this run's negatives; confirm-speaker keeps a previous run's negatives through both the idempotency rebuild and a reassignment. Also corrects what the trade costs. It is not one stale positive prototype: freezing the correction path freezes the hard negatives a wrong confirm minted, and the mutual-negative loop can hand somebody their own voice as a reason to refuse a match. That entry used to be rebuilt away by the next confirm and now survives indefinitely; repair-speaker-profiles removes it by id. Records at the mutual-negative source selection that its missing run filter went from a bounded window to an unbounded one, for the read path to close, and that the still_present guard's disagreement is that task's call. --- simple_recorder.py | 32 ++++++++- src/config.py | 33 ++++++--- tests/test_confirm_speaker_cli.py | 79 +++++++++++++++++++++ tests/test_speaker_multi_marking.py | 105 ++++++++++++++++++++++++++++ 4 files changed, 236 insertions(+), 13 deletions(-) diff --git a/simple_recorder.py b/simple_recorder.py index 0424f0a3..5edbd3fa 100644 --- a/simple_recorder.py +++ b/simple_recorder.py @@ -5018,9 +5018,18 @@ def confirm_speaker(meeting_stem, channel, diarization_speaker_id, person_id, ne # ids only identify a voice within one diarization run -- a re-diarization # renumbers from SPEAKER_0 with no memory of who held that id, so # unscoped these removals would treat a stranger's confirmation as this - # cluster's previous owner and delete it. The cost is that a + # cluster's previous owner and delete it. + # + # The cost is more than a stale positive prototype left standing: a # confirmation made against a superseded run can no longer be corrected - # by re-confirming; the repair CLI removes such entries by id. + # by re-confirming that id, which freezes the hard negatives it minted + # too. The mutual-negative loop further down records each confirmed + # cluster as negative evidence against the other people confirmed in this + # channel, so a confirm that got the owner wrong leaves somebody holding + # their OWN voice as a reason to refuse a future match -- and that entry + # now outlives every later confirm instead of being rebuilt away by the + # idempotency removals below. Clearing it takes `repair-speaker-profiles`, + # which drops entries by prototype_id and is not run-scoped. reassigned_from = [] for existing_person in config.get_person_profiles(): removed = config.remove_speaker_evidence( @@ -5038,6 +5047,13 @@ def confirm_speaker(meeting_stem, channel, diarization_speaker_id, person_id, ne # used to strip the negatives the clusters they KEEP still justify, # and the rebuild below only restores negatives for the person being # confirmed now, so that evidence was simply lost. + # + # This read stays unscoped while the removal it guards is scoped, so + # a leftover prototype from a superseded run reads as "still owns a + # cluster here" and suppresses the cleanup. Left as is deliberately: + # the failure direction is keeping evidence rather than destroying + # it. Whether the two should agree is the read path's decision (the + # next task), not something to settle from one side here. still_present = any( p.get("meeting_id") == meeting_stem and prototype_channel_matches(p, channel, channel_recording_type) @@ -5122,6 +5138,18 @@ def confirm_speaker(meeting_stem, channel, diarization_speaker_id, person_id, ne # them. Matching only the first prototype left the second cluster # with no negative evidence at all, so a later meeting could still # match this speaker to it. + # + # NOT run-scoped, and the run scoping added to the removals above + # made that worse rather than neutral. This selects a prototype by + # meeting+sid+channel and then mints a negative from the CURRENT + # run's embedding for that id -- so a prototype confirmed against a + # superseded run produces a negative about a voice that person was + # never confirmed next to. Before the scoping the damage window was + # bounded: the next confirm of that id deleted the stale prototype, + # because it matched unscoped. Now stale prototypes persist + # indefinitely, so this read can fire arbitrarily far in the future. + # Closing it is the read path's job (the next task) -- this selection + # has to run the same prototype_run_matches predicate. matches = [ p for p in (other_person.get("prototypes") or []) if p.get("meeting_id") == meeting_stem diff --git a/src/config.py b/src/config.py index 44f19ab1..96e834ac 100644 --- a/src/config.py +++ b/src/config.py @@ -1342,17 +1342,28 @@ def remove_speaker_evidence( memory of who held that id before, so without this scope confirming the new run's first cluster deletes the prototype an earlier run's confirmation recorded against a genuinely different voice. - `ANY_DIARIZATION_RUN` (the default) removes regardless of run, which - is what every caller that has no sidecar in hand -- the repair CLI's - analysis, enrollment cleanup -- still needs; passing `None` is the - distinct "the sidecar reports no run" scope, not the absence of one. - - The trade this scope accepts: a confirmation made against a - superseded run can no longer be corrected by re-confirming the same - cluster id, since the two are no longer recognised as the same - cluster. That evidence has to be dropped through the repair CLI's - by-id removal instead. Silently destroying a genuine prototype is - the worse failure of the two, and it is the one happening today. + `ANY_DIARIZATION_RUN` (the default) removes regardless of run. Every + in-repo caller works from a sidecar and passes a scope today, so the + default carries no traffic; it exists so that a future caller with no + sidecar in hand gets today's semantics by not knowing about runs, + rather than silently filtering. Passing `None` is the distinct "the + sidecar reports no run" scope, not the absence of one. + + The trade this scope accepts, and it is heavier than one stale + positive prototype: a confirmation made against a superseded run can + no longer be corrected by re-confirming the same cluster id, since + the two are no longer recognised as the same cluster. That freezes + the hard negatives the wrong confirmation minted as well, and those + can include one built from a person's OWN voice -- confirm-speaker + records each confirmed cluster as negative evidence against the other + people confirmed in that channel, so a confirm that got the owner + wrong hands somebody their own embedding as a reason to refuse a + match. Re-confirming used to clear it; now it survives every later + confirm, and self-suppression does not expire on its own. The escape + hatch is `repair-speaker-profiles`, which drops entries by + `prototype_id` via `remove_speaker_evidence_by_ids` and is unaffected + by run scope. Silently destroying genuine evidence is still the worse + failure of the two, and it is the one happening today. """ from src.speaker_suggestions import prototype_channel_matches, prototype_run_matches diff --git a/tests/test_confirm_speaker_cli.py b/tests/test_confirm_speaker_cli.py index ef5cbdd7..badeb5ce 100644 --- a/tests/test_confirm_speaker_cli.py +++ b/tests/test_confirm_speaker_cli.py @@ -235,6 +235,85 @@ def test_confirming_a_reused_cluster_id_from_a_newer_run_spares_the_older_runs_p self.assertEqual(sarah_profile["prototypes"][0]["diarization_run_id"], run2) self.assertEqual(sarah_profile["prototypes"][0]["embedding_mean"], [0.0, 1.0]) + def test_confirming_a_reused_cluster_id_keeps_an_older_runs_negatives(self): + # The idempotency-rebuild removals, which the test above never + # reaches: it stops at a positive removal that matches nothing. Those + # two clear the negatives THIS confirm is about to rewrite, so + # unscoped they take a previous run's negatives with them -- evidence + # about a different voice that nothing in this confirm questioned. + with tempfile.TemporaryDirectory() as tmp: + self._seed_sidecar(tmp) + output_dir = Path(tmp) / "output" + run1 = read_speakers_sidecar(output_dir, "mtg001")["diarization_run"]["run_id"] + cfg = Config(config_path=Path(tmp) / "config.json") + + r1, cfg = self._run(["mtg001", "mic", "SPEAKER_00", "--new-person", "Max"], tmp, cfg=cfg) + max_id = _last_json(r1.output)["person_id"] + r2, cfg = self._run(["mtg001", "mic", "SPEAKER_01", "--new-person", "Sarah"], tmp, cfg=cfg) + sarah_id = _last_json(r2.output)["person_id"] + + run2 = self._rediarize(tmp) + # Sarah is confirmed on the new run's SPEAKER_00 -- the id her own + # run-1 hard negative is recorded against. + _, cfg = self._run(["mtg001", "mic", "SPEAKER_00", "--person-id", sarah_id], tmp, cfg=cfg) + + sarah_profile = cfg.get_person_profile(sarah_id) + self.assertEqual( + [n["diarization_run_id"] for n in sarah_profile["hard_negatives"]], [run1], + "her run-1 negative is about the voice that held SPEAKER_00 back then", + ) + self.assertEqual( + sorted(p["diarization_run_id"] for p in sarah_profile["prototypes"]), + sorted([run1, run2]), + ) + max_profile = cfg.get_person_profile(max_id) + self.assertEqual([p["diarization_run_id"] for p in max_profile["prototypes"]], [run1]) + self.assertEqual([n["diarization_run_id"] for n in max_profile["hard_negatives"]], [run1]) + + def test_reassigning_this_runs_cluster_leaves_the_previous_runs_negatives_standing(self): + # The reassignment loop's two negative cleanups, reached only when a + # confirm actually supersedes somebody. Both are about the cluster + # being taken away, so neither may reach into a previous run's + # evidence about a reused id. + with tempfile.TemporaryDirectory() as tmp: + self._seed_sidecar(tmp) + output_dir = Path(tmp) / "output" + run1 = read_speakers_sidecar(output_dir, "mtg001")["diarization_run"]["run_id"] + cfg = Config(config_path=Path(tmp) / "config.json") + + self._run(["mtg001", "mic", "SPEAKER_00", "--new-person", "Max"], tmp, cfg=cfg) + r2, cfg = self._run(["mtg001", "mic", "SPEAKER_01", "--new-person", "Sarah"], tmp, cfg=cfg) + sarah_id = _last_json(r2.output)["person_id"] + sarah_run1_negative = cfg.get_person_profile(sarah_id)["hard_negatives"][0]["prototype_id"] + + self._rediarize(tmp) + r3, cfg = self._run(["mtg001", "mic", "SPEAKER_00", "--new-person", "Ida"], tmp, cfg=cfg) + ida_id = _last_json(r3.output)["person_id"] + # Hand-built rather than earned, because a run-1 confirmation + # would also leave Ida a run-1 prototype, and the cleanup under + # test only runs for someone who owns no cluster here any more. + ida_run1_negative = cfg.add_speaker_prototype( + ida_id, [0.5, 0.5], recording_type="in_person", meeting_id="mtg001", + diarization_speaker_id="SPEAKER_01", speech_duration_seconds=20.0, + segment_count=4, created_from="user_confirmed", channel="mic", + negative=True, diarization_run_id=run1, + )["prototype_id"] + + # Ida loses the cluster to Jon, so both cleanups fire. + r4, cfg = self._run(["mtg001", "mic", "SPEAKER_00", "--new-person", "Jon"], tmp, cfg=cfg) + self.assertEqual(_last_json(r4.output)["reassigned_from"], ["Ida"]) + + self.assertEqual( + [n["prototype_id"] for n in cfg.get_person_profile(ida_id)["hard_negatives"]], + [ida_run1_negative], + "her run-2 negative went with the cluster; the run-1 one is not this confirm's", + ) + self.assertIn( + sarah_run1_negative, + [n["prototype_id"] for n in cfg.get_person_profile(sarah_id)["hard_negatives"]], + "a bystander's run-1 negative survives a reassignment in run 2", + ) + def test_reconfirming_a_cluster_on_a_legacy_sidecar_still_supersedes(self): # The correction path on a library that predates run stamping: with no # run id anywhere, re-confirming is still the "Change" flow and must diff --git a/tests/test_speaker_multi_marking.py b/tests/test_speaker_multi_marking.py index d565025a..b9a6e4e4 100644 --- a/tests/test_speaker_multi_marking.py +++ b/tests/test_speaker_multi_marking.py @@ -1056,6 +1056,111 @@ def _seeded_run_id(self, tmp): alone because its cluster ids mean something else now.""" return read_speakers_sidecar(Path(tmp) / "output", "mtg001")["diarization_run"]["run_id"] + def _rediarize(self, tmp): + """Rewrite the seeded sidecar with a fresh run whose cluster ids are + reused but whose voices are not, which is what a re-diarization + produces. Returns the new run id.""" + self._seed(tmp, clusters={ + "SPEAKER_0": { + "embedding": [0.0, 1.0], "speech_duration_seconds": 55.0, + "segment_count": 9, "segments": [{"start": 2.0, "end": 6.0}], + }, + "SPEAKER_1": { + "embedding": [1.0, 0.0], "speech_duration_seconds": 35.0, + "segment_count": 7, "segments": [{"start": 21.0, "end": 25.0}], + }, + }) + return self._seeded_run_id(tmp) + + def test_marking_a_reused_cluster_id_leaves_an_older_runs_confirmation_alone(self): + # The withdrawal loop's half of the run-scope defect. Marking THIS + # run's SPEAKER_0 as mixed says nothing about the person confirmed on + # a previous run's SPEAKER_0: the diarizer reuses the id for an + # unrelated voice, so unscoped this would withdraw a confirmation + # nobody questioned and delete the prototype behind it. + with tempfile.TemporaryDirectory() as tmp: + self._seed(tmp) + cfg = Config(config_path=Path(tmp) / "config.json") + pid = cfg.create_person_profile("Julian")["person_id"] + run1 = self._seeded_run_id(tmp) + cfg.add_speaker_prototype( + pid, [1.0, 0.0], recording_type="remote", meeting_id="mtg001", + diarization_speaker_id="SPEAKER_0", speech_duration_seconds=60.0, + segment_count=10, created_from="user_confirmed", channel="system", + diarization_run_id=run1, + ) + + run2 = self._rediarize(tmp) + self.assertNotEqual(run1, run2) + + result = self._run( + simple_recorder.mark_speaker_cluster, + ["mtg001", "system", "SPEAKER_0"], tmp, cfg=cfg, + ) + data = _last_json(result.output) + self.assertTrue(data["success"]) + self.assertEqual( + data["cleared_confirmation_from"], [], + "the marking describes this run's cluster, not the one Julian was confirmed on", + ) + profile = cfg.get_person_profile(pid) + self.assertEqual( + [p["diarization_run_id"] for p in profile["prototypes"]], [run1], + ) + + def test_marking_withdraws_this_runs_negatives_and_keeps_an_older_runs(self): + # The other two removals in the same loop, which the test above never + # reaches (it stops at a positive removal that matches nothing). A + # negative recorded against a previous run's SPEAKER_0 is evidence + # about a different voice, so the marking must leave it standing on + # both the withdrawn person's profile and everyone else's. + with tempfile.TemporaryDirectory() as tmp: + self._seed(tmp) + cfg = Config(config_path=Path(tmp) / "config.json") + julian = cfg.create_person_profile("Julian")["person_id"] + max_id = cfg.create_person_profile("Max")["person_id"] + run1 = self._seeded_run_id(tmp) + run2 = self._rediarize(tmp) + + # Julian is the one confirmed on the CURRENT run's SPEAKER_0... + cfg.add_speaker_prototype( + julian, [0.0, 1.0], recording_type="remote", meeting_id="mtg001", + diarization_speaker_id="SPEAKER_0", speech_duration_seconds=55.0, + segment_count=9, created_from="user_confirmed", channel="system", + diarization_run_id=run2, + ) + # ...and Max was ruled out for it, in this run and in the last. + for run_id in (run1, run2): + cfg.add_speaker_prototype( + max_id, [0.0, 1.0], recording_type="remote", meeting_id="mtg001", + diarization_speaker_id="SPEAKER_0", speech_duration_seconds=55.0, + segment_count=9, created_from="user_confirmed", channel="system", + negative=True, diarization_run_id=run_id, + ) + # Julian carries a stale one of his own from before the re-run. + cfg.add_speaker_prototype( + julian, [1.0, 0.0], recording_type="remote", meeting_id="mtg001", + diarization_speaker_id="SPEAKER_0", speech_duration_seconds=60.0, + segment_count=10, created_from="user_confirmed", channel="system", + negative=True, diarization_run_id=run1, + ) + + result = self._run( + simple_recorder.mark_speaker_cluster, + ["mtg001", "system", "SPEAKER_0"], tmp, cfg=cfg, + ) + data = _last_json(result.output) + self.assertEqual(data["cleared_confirmation_from"], ["Julian"]) + self.assertEqual(cfg.get_person_profile(julian)["prototypes"], []) + self.assertEqual( + [n["diarization_run_id"] for n in cfg.get_person_profile(julian)["hard_negatives"]], + [run1], + ) + self.assertEqual( + [n["diarization_run_id"] for n in cfg.get_person_profile(max_id)["hard_negatives"]], + [run1], + ) + def test_marks_and_reports_the_new_minimum_speaker_count(self): with tempfile.TemporaryDirectory() as tmp: self._seed(tmp) From 8f24072e6abd1be80a648d54840a5053fea021ae Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 4 Aug 2026 21:23:11 +0200 Subject: [PATCH 08/23] fix(speakers): stop a superseded run's confirmation from naming this run's clusters Every reader that asks "who is this cluster" now runs the same prototype_run_matches predicate the write path already does: the panel's confirmed_by_user derivation, confirm-speaker's still_present guard and its mutual-negative source selection, and speaker-naming-status' named count. A re-diarization numbers from SPEAKER_0 again with no memory of who held that id, so unscoped each of these reads a stranger's confirmation as this cluster's - it puts a name the user never chose on a row, mints permanent hard negatives about a voice nobody was confirmed next to, and counts an unnamed cluster as taken care of in the warning shown before a delete. suggest-speakers gains stale_assignments so the loss is reported rather than merely handled. It is collected per cluster and only while the cluster is still unclaimed: nothing ever deletes a superseded prototype, so a notice derived from the prototypes alone could never go away. confirmed_participant_names stays meeting-scoped, now with the reason in the docstring. Attendance is a property of the meeting, not of a run. Three existing fixtures paired an unstamped prototype with a stamped sidecar - a state no build can produce. They now stamp the prototype, or use the legacy sidecar their legacy prototype belongs with. --- simple_recorder.py | 111 ++++++++++++++----- src/speaker_suggestions.py | 11 ++ tests/test_confirm_speaker_cli.py | 79 +++++++++++++- tests/test_speaker_multi_marking.py | 29 +++++ tests/test_suggest_speakers_cli.py | 162 +++++++++++++++++++++++++++- 5 files changed, 362 insertions(+), 30 deletions(-) diff --git a/simple_recorder.py b/simple_recorder.py index 5edbd3fa..97973a26 100644 --- a/simple_recorder.py +++ b/simple_recorder.py @@ -4922,6 +4922,7 @@ def confirm_speaker(meeting_stem, channel, diarization_speaker_id, person_id, ne confirmed_participant_names, merge_same_channel_fragments, prototype_channel_matches, + prototype_run_matches, read_speakers_sidecar, record_original_labels, relabel_transcript_exact, @@ -5048,15 +5049,16 @@ def confirm_speaker(meeting_stem, channel, diarization_speaker_id, person_id, ne # and the rebuild below only restores negatives for the person being # confirmed now, so that evidence was simply lost. # - # This read stays unscoped while the removal it guards is scoped, so - # a leftover prototype from a superseded run reads as "still owns a - # cluster here" and suppresses the cleanup. Left as is deliberately: - # the failure direction is keeping evidence rather than destroying - # it. Whether the two should agree is the read path's decision (the - # next task), not something to settle from one side here. + # Run-scoped like the removal it guards, and it has to be: a + # leftover prototype from a superseded run would otherwise read as + # "still owns a cluster here" and suppress the cleanup for good, + # since nothing ever deletes that prototype. "Present" here means + # present in the meeting as it is diarized NOW, which is the only + # sense in which the negatives below are still justified. still_present = any( p.get("meeting_id") == meeting_stem and prototype_channel_matches(p, channel, channel_recording_type) + and prototype_run_matches(p, run_id) for p in (config.get_person_profile(existing_person["person_id"]) or {}).get( "prototypes", ) or [] @@ -5139,22 +5141,20 @@ def confirm_speaker(meeting_stem, channel, diarization_speaker_id, person_id, ne # with no negative evidence at all, so a later meeting could still # match this speaker to it. # - # NOT run-scoped, and the run scoping added to the removals above - # made that worse rather than neutral. This selects a prototype by - # meeting+sid+channel and then mints a negative from the CURRENT - # run's embedding for that id -- so a prototype confirmed against a - # superseded run produces a negative about a voice that person was - # never confirmed next to. Before the scoping the damage window was - # bounded: the next confirm of that id deleted the stale prototype, - # because it matched unscoped. Now stale prototypes persist - # indefinitely, so this read can fire arbitrarily far in the future. - # Closing it is the read path's job (the next task) -- this selection - # has to run the same prototype_run_matches predicate. + # Run-scoped, because this selects a prototype by meeting+sid+channel + # and then mints a negative from the CURRENT run's embedding for that + # id. Unscoped, a prototype confirmed against a superseded run would + # produce a negative about a voice that person was never confirmed + # next to -- permanent suppression evidence built from a coincidence + # of cluster numbering, in both directions, and it would keep firing + # for as long as the meeting exists since the superseded prototype is + # deliberately never deleted. matches = [ p for p in (other_person.get("prototypes") or []) if p.get("meeting_id") == meeting_stem and p.get("diarization_speaker_id") in other_sids and prototype_channel_matches(p, channel, channel_recording_type) + and prototype_run_matches(p, run_id) ] if not matches: continue @@ -5470,6 +5470,7 @@ def speaker_naming_status(meeting_stem): merge_same_channel_fragments, clusters_from_sidecar_channel, prototype_channel_matches, + prototype_run_matches, read_speakers_sidecar, ) @@ -5482,6 +5483,13 @@ def speaker_naming_status(meeting_stem): })) return + # A name from a superseded diarization run does not name anything here: + # the run this sidecar describes renumbered its clusters, so that + # prototype's id now belongs to whichever voice inherited it. Counting + # it as named is the one error direction that costs data -- it hides an + # unnamed cluster from the delete warning, and an unnamed cluster cannot + # be named again once the audio is gone. + run_id = (sidecar.get("diarization_run") or {}).get("run_id") profiles = get_config().get_person_profiles() total = 0 named = 0 @@ -5508,6 +5516,7 @@ def speaker_naming_status(meeting_stem): p.get("meeting_id") == meeting_stem and p.get("diarization_speaker_id") in fragment_ids and prototype_channel_matches(p, channel_name, recording_type) + and prototype_run_matches(p, run_id) for p in (person.get("prototypes") or []) ) for person in profiles @@ -5545,6 +5554,7 @@ def suggest_speakers(meeting_stem): merge_same_channel_fragments, minimum_speaker_count, prototype_channel_matches, + prototype_run_matches, read_speakers_sidecar, sample_text_from_samples, suggest_speakers_for_meeting, @@ -5580,6 +5590,14 @@ def suggest_speakers(meeting_stem): # unaffected; they cut audio at this run's own segments. turn_manifest = sidecar.get("transcript_lines") + # Which diarization run the clusters below belong to. A prototype + # confirmed against a DIFFERENT run describes a voice this run may have + # given to somebody else -- the diarizer numbers from SPEAKER_0 every + # time with no memory of who held that id -- so it may not speak for + # any row here. `None` on a legacy sidecar, where the predicate's + # both-absent rule keeps every existing confirmation current. + run_id = (sidecar.get("diarization_run") or {}).get("run_id") + profiles = get_config().get_person_profiles() # Merge fragments per channel first, then suggest for ALL channels in # one call -- used-person exclusivity is meeting-wide (a person can't @@ -5595,6 +5613,11 @@ def suggest_speakers(meeting_stem): results_by_channel = suggest_speakers_for_meeting(merged_by_channel, profiles) channels_out = {} + # People whose confirmation in this meeting was made against a run that + # no longer describes anything on screen, keyed by person id so someone + # who lost several clusters is reported once. Insertion-ordered, so the + # notice reads in the order the clusters appear. + stale_assignments = {} for channel_name, channel in (sidecar.get("channels") or {}).items(): clusters = merged_by_channel[channel_name] results = results_by_channel[channel_name] @@ -5628,22 +5651,43 @@ def suggest_speakers(meeting_stem): # not yet met), and any client-side "just confirmed" feedback # is gone the moment the panel unmounts (e.g. navigating away # and back). This survives both. + # + # Only evidence from THIS run counts. A prototype confirmed + # against a superseded run would otherwise show up as a + # confirmation the user appears to have made themselves, on a + # row that may be a different person entirely -- and unlike a + # wrong suggestion, nothing about it invites a second look. confirmed_by_user = None confirmed_person_id = None + superseded_owners = [] for person in profiles: - if any( - p.get("meeting_id") == meeting_stem + owned = [ + p for p in (person.get("prototypes") or []) + if p.get("meeting_id") == meeting_stem and p.get("diarization_speaker_id") in fragment_ids and prototype_channel_matches(p, channel_name, recording_type) - for p in (person.get("prototypes") or []) - ): - confirmed_by_user = person["display_name"] - # The id as well as the name: display names are not a - # stable identity (a rename can make two profiles read - # alike), and the panel uses this to tell which people - # already hold a cluster of THIS meeting. - confirmed_person_id = person["person_id"] - break + ] + if not owned: + continue + if any(prototype_run_matches(p, run_id) for p in owned): + if confirmed_by_user is None: + confirmed_by_user = person["display_name"] + # The id as well as the name: display names are not a + # stable identity (a rename can make two profiles read + # alike), and the panel uses this to tell which people + # already hold a cluster of THIS meeting. + confirmed_person_id = person["person_id"] + else: + superseded_owners.append(person) + # Reported per cluster and only while the cluster is still + # unclaimed, so the notice this feeds can actually go away. + # Nothing ever deletes a superseded prototype -- that is the + # point of the run scoping -- so a notice derived from the + # prototypes alone would outlive every action the user could + # take to answer it. + if confirmed_by_user is None: + for person in superseded_owners: + stale_assignments.setdefault(person["person_id"], person["display_name"]) cluster_out[sid] = { "status": r.status, "suggested_person_id": r.suggested_person_id, @@ -5718,6 +5762,17 @@ def suggest_speakers(meeting_stem): # as four clusters with nothing indicating anything was dropped. # No caller acts on this number today -- see minimum_speaker_count. "minimum_speaker_count": minimum_speaker_count(sidecar.get("channels") or {}), + # Confirmations this meeting's re-diarization orphaned. The panel + # renders one meeting-level notice from this: the clusters were + # renumbered, these people's assignments no longer point at anything + # on screen, and re-confirming them is the only thing that restores + # the link. Empty is the normal case, including on every legacy + # library, and their voice evidence is untouched either way -- it + # keeps scoring candidates in every meeting. + "stale_assignments": [ + {"person_id": pid, "display_name": name} + for pid, name in stale_assignments.items() + ], "channels": channels_out, })) diff --git a/src/speaker_suggestions.py b/src/speaker_suggestions.py index 32ed08f5..465b0a9a 100644 --- a/src/speaker_suggestions.py +++ b/src/speaker_suggestions.py @@ -1350,6 +1350,17 @@ def confirmed_participant_names(meeting_stem: str, profiles: list) -> list: already carry the `meeting_id` they were confirmed from. A person counts once even with multiple prototypes from this meeting (e.g. merged same-channel fragments, or confirmed on both channels). + + Meeting-scoped and NOT run-scoped, deliberately -- do not "fix" this + into running prototype_run_matches the way the cluster-level readers + do. Attendance is a property of the meeting, not of a diarization run: + a prototype from a superseded run no longer says WHICH cluster this + person is, but it still says they were confirmed as present here, and + that stays true however often the audio is re-diarized. Run-filtering + it would empty the Participants section on every reprocess (the + `full-reprocess` restore reads exactly this), deleting correct + information to enforce a scope that answers a question nobody asked + here. """ names = [] for person in profiles: diff --git a/tests/test_confirm_speaker_cli.py b/tests/test_confirm_speaker_cli.py index badeb5ce..cf7f4014 100644 --- a/tests/test_confirm_speaker_cli.py +++ b/tests/test_confirm_speaker_cli.py @@ -336,6 +336,78 @@ def test_reconfirming_a_cluster_on_a_legacy_sidecar_still_supersedes(self): self.assertEqual(len(sarah_profile["prototypes"]), 1) self.assertNotIn("diarization_run_id", sarah_profile["prototypes"][0]) + def test_a_superseded_prototype_does_not_keep_someone_present_in_this_channel(self): + # The `still_present` read that guards the negative cleanup. When a + # person loses their only cluster of THIS run, the negatives they + # earned by being here go with it -- but a leftover prototype from a + # superseded run reads as "they still own a cluster here" and + # suppresses that cleanup, leaving evidence behind that nothing in + # this meeting justifies any more. + with tempfile.TemporaryDirectory() as tmp: + self._seed_sidecar(tmp) + output_dir = Path(tmp) / "output" + run1 = read_speakers_sidecar(output_dir, "mtg001")["diarization_run"]["run_id"] + cfg = Config(config_path=Path(tmp) / "config.json") + max_id = cfg.create_person_profile("Max")["person_id"] + # His run-1 cluster, on the id he does NOT hold in run 2. + cfg.add_speaker_prototype( + max_id, [0.0, 1.0], recording_type="in_person", meeting_id="mtg001", + diarization_speaker_id="SPEAKER_01", speech_duration_seconds=25.0, + segment_count=4, created_from="user_confirmed", channel="mic", + diarization_run_id=run1, + ) + + self._rediarize(tmp) + # Earned, not hand-built: confirming him and then Sarah is what + # mints his run-2 negative in the first place. + _, cfg = self._run(["mtg001", "mic", "SPEAKER_00", "--person-id", max_id], tmp, cfg=cfg) + _, cfg = self._run(["mtg001", "mic", "SPEAKER_01", "--new-person", "Sarah"], tmp, cfg=cfg) + self.assertEqual(len(cfg.get_person_profile(max_id)["hard_negatives"]), 1) + + # He loses his one run-2 cluster to Ida, so he is no longer + # present in this channel in this run at all. + r, cfg = self._run(["mtg001", "mic", "SPEAKER_00", "--new-person", "Ida"], tmp, cfg=cfg) + self.assertEqual(_last_json(r.output)["reassigned_from"], ["Max"]) + + max_profile = cfg.get_person_profile(max_id) + self.assertEqual( + max_profile["hard_negatives"], [], + "his run-2 negatives rest on a presence he no longer has", + ) + self.assertEqual( + [p["diarization_run_id"] for p in max_profile["prototypes"]], [run1], + "and his run-1 prototype is still not this confirm's to touch", + ) + + def test_a_superseded_prototype_does_not_seed_negatives_from_this_runs_voices(self): + # The mutual-negative source selection. It picks a person by + # meeting+cluster id and then mints a negative from the CURRENT run's + # embedding for that id -- so an unscoped match records "Sarah is not + # this voice" about a voice Max was never confirmed next to, and + # hands Max the same about Sarah. Hard negatives are permanent + # suppression, so a wrong one is not noise: it refuses a real match + # for either of them in meetings that have nothing to do with this. + with tempfile.TemporaryDirectory() as tmp: + self._seed_sidecar(tmp) + output_dir = Path(tmp) / "output" + run1 = read_speakers_sidecar(output_dir, "mtg001")["diarization_run"]["run_id"] + cfg = Config(config_path=Path(tmp) / "config.json") + max_id = cfg.create_person_profile("Max")["person_id"] + cfg.add_speaker_prototype( + max_id, [0.0, 1.0], recording_type="in_person", meeting_id="mtg001", + diarization_speaker_id="SPEAKER_01", speech_duration_seconds=25.0, + segment_count=4, created_from="user_confirmed", channel="mic", + diarization_run_id=run1, + ) + + self._rediarize(tmp) + result, cfg = self._run(["mtg001", "mic", "SPEAKER_00", "--new-person", "Sarah"], tmp, cfg=cfg) + data = _last_json(result.output) + self.assertTrue(data["success"]) + self.assertEqual(data["hard_negatives_added_against"], []) + self.assertEqual(cfg.get_person_profile(data["person_id"])["hard_negatives"], []) + self.assertEqual(cfg.get_person_profile(max_id)["hard_negatives"], []) + def _seed_legacy_sidecar(self, tmp, meeting_stem="mtg001"): # A sidecar written before diarization_run existed -- no top-level # "diarization_run" key at all, not one holding None. Written by @@ -497,8 +569,13 @@ def test_cross_channel_id_collision_does_not_create_hard_negatives(self): def test_legacy_prototype_without_channel_still_matches_via_recording_type(self): # A prototype confirmed before the channel field existed must still # count as a same-channel confirmation via the recording_type proxy. + # On a legacy sidecar, because that is where such a prototype + # actually lives: a build old enough to write no `channel` wrote no + # run block either, and pairing it with a freshly stamped sidecar + # would describe a meeting re-diarized since that confirm -- which + # the run scope correctly refuses, testing something else entirely. with tempfile.TemporaryDirectory() as tmp: - self._seed_sidecar(tmp) + self._seed_legacy_sidecar(tmp) cfg = Config(config_path=Path(tmp) / "config.json") alice = cfg.create_person_profile("Alice") cfg.add_speaker_prototype( diff --git a/tests/test_speaker_multi_marking.py b/tests/test_speaker_multi_marking.py index b9a6e4e4..4f62507a 100644 --- a/tests/test_speaker_multi_marking.py +++ b/tests/test_speaker_multi_marking.py @@ -1522,6 +1522,9 @@ def _seed(self, tmp): }) return output_dir + def _run_id(self, tmp): + return read_speakers_sidecar(Path(tmp) / "output", "mtg001")["diarization_run"]["run_id"] + def test_counts_unnamed_clusters(self): with tempfile.TemporaryDirectory() as tmp: self._seed(tmp) @@ -1540,11 +1543,37 @@ def test_a_confirmed_cluster_no_longer_counts_as_unnamed(self): meeting_id="mtg001", diarization_speaker_id="SPEAKER_0", speech_duration_seconds=60.0, segment_count=10, created_from="user_confirmed", channel="system", + # Stamped with the sidecar's own run, the way a real confirm + # against it does -- an unstamped prototype here would + # describe a state the app cannot reach. + diarization_run_id=self._run_id(tmp), ) data = _last_json(self._run(["mtg001"], tmp, cfg=cfg).output) self.assertEqual(data["named_clusters"], 1) self.assertEqual(data["unnamed_clusters"], 1) + def test_a_name_from_a_superseded_run_does_not_count_as_named(self): + # This feeds the sentence shown before a delete, and the cost of + # getting it wrong is one-directional: an unnamed cluster is gone + # for good once the audio is deleted, and a stale prototype claiming + # its id is exactly how a cluster nobody has ever heard gets counted + # as already taken care of. + with tempfile.TemporaryDirectory() as tmp: + self._seed(tmp) + cfg = Config(config_path=Path(tmp) / "config.json") + person = cfg.create_person_profile("Julian") + cfg.add_speaker_prototype( + person["person_id"], [1.0, 0.0], recording_type="remote", + meeting_id="mtg001", diarization_speaker_id="SPEAKER_0", + speech_duration_seconds=60.0, segment_count=10, + created_from="user_confirmed", channel="system", + diarization_run_id=self._run_id(tmp), + ) + self._seed(tmp) # re-diarized: same ids, new run, other voices + data = _last_json(self._run(["mtg001"], tmp, cfg=cfg).output) + self.assertEqual(data["named_clusters"], 0) + self.assertEqual(data["unnamed_clusters"], 2) + def test_a_marked_cluster_is_not_counted_as_waiting_to_be_named(self): # It has already been reviewed and ruled out. Counting it would nag # about the one row that can never be resolved. diff --git a/tests/test_suggest_speakers_cli.py b/tests/test_suggest_speakers_cli.py index eb3c93a8..5dede7c3 100644 --- a/tests/test_suggest_speakers_cli.py +++ b/tests/test_suggest_speakers_cli.py @@ -8,7 +8,11 @@ import simple_recorder from src.config import Config -from src.speaker_suggestions import write_speakers_sidecar +from src.speaker_suggestions import ( + read_speakers_sidecar, + write_sidecar_document, + write_speakers_sidecar, +) def _last_json(output): @@ -16,6 +20,11 @@ def _last_json(output): return json.loads(line) +def _seeded_run_id(tmp, meeting_stem="mtg001"): + """The run id of the sidecar just written into `tmp`.""" + return read_speakers_sidecar(Path(tmp) / "output", meeting_stem)["diarization_run"]["run_id"] + + class SuggestSpeakersCliTests(unittest.TestCase): """Covers the identification anchors (channel/duration/segment_count/ first_timestamp) added to each cluster's output -- without these, a @@ -139,6 +148,11 @@ def test_confirmed_by_user_persists_across_requests_unlike_transient_ui_state(se person["person_id"], [1.0, 0.0], recording_type="remote", meeting_id="mtg001", diarization_speaker_id="SPEAKER_0", speech_duration_seconds=10.0, segment_count=2, created_from="user_confirmed", + # Stamped with the run on disk, the way a real confirm + # against this sidecar stamps it. Unstamped it would describe + # a confirmation made before the meeting was re-diarized, + # which is a different test (see SuggestSpeakersRunScopeTests). + diarization_run_id=_seeded_run_id(tmp), ) result = self._run(["mtg001"], tmp, cfg=cfg) data = _last_json(result.output) @@ -184,6 +198,7 @@ def test_confirmed_by_user_scoped_to_recording_type_not_just_diarization_id(self person["person_id"], [1.0, 0.0], recording_type="in_person", meeting_id="mtg001", diarization_speaker_id="SPEAKER_0", speech_duration_seconds=10.0, segment_count=2, created_from="user_confirmed", + diarization_run_id=_seeded_run_id(tmp), ) result = self._run(["mtg001"], tmp, cfg=cfg) data = _last_json(result.output) @@ -215,6 +230,7 @@ def test_confirmed_by_user_resolves_through_merged_fragments(self): person["person_id"], [0.995, 0.0999], recording_type="remote", meeting_id="mtg001", diarization_speaker_id="SPEAKER_2", speech_duration_seconds=1538.0, segment_count=552, created_from="user_confirmed", + diarization_run_id=_seeded_run_id(tmp), ) result = self._run(["mtg001"], tmp, cfg=cfg) data = _last_json(result.output) @@ -488,5 +504,149 @@ def fake_extract(audio_path, channel, segments, output_path, segment_index=None) self.assertEqual(len(captured["segments"]), 2) +class SuggestSpeakersRunScopeTests(unittest.TestCase): + """What the panel may still call "confirmed" after the meeting was + diarized a second time. + + A re-diarization numbers its clusters from SPEAKER_0 again with no + memory of who held that id, so an older run's prototype describes a + voice this run may have given to somebody else. Reporting it as + `confirmed_by_user` puts a name the user never chose on a stranger's + row -- and it looks exactly like a confirmation they made themselves, + so nothing invites them to check it. + """ + + def _run(self, args, tmp, cfg=None): + cfg = cfg or Config(config_path=Path(tmp) / "config.json") + with mock.patch("src.config.get_config", return_value=cfg), \ + mock.patch.dict("os.environ", {"STENOAI_USER_DATA_DIR": tmp}): + return CliRunner().invoke(simple_recorder.suggest_speakers, args) + + def _seed(self, tmp, clusters=None): + output_dir = Path(tmp) / "output" + output_dir.mkdir(parents=True, exist_ok=True) + write_speakers_sidecar(output_dir, "mtg001", { + "system": { + "recording_type": "remote", + "clusters": clusters or { + "SPEAKER_0": {"embedding": [1.0, 0.0], "speech_duration_seconds": 60.0, + "segment_count": 10, "segments": [{"start": 1.0, "end": 5.0}]}, + "SPEAKER_1": {"embedding": [0.0, 1.0], "speech_duration_seconds": 40.0, + "segment_count": 8, "segments": [{"start": 20.0, "end": 24.0}]}, + }, + }, + }) + return output_dir + + def _run_id(self, tmp): + return read_speakers_sidecar(Path(tmp) / "output", "mtg001")["diarization_run"]["run_id"] + + def _rediarize(self, tmp): + """A second diarization run over the same meeting: same cluster ids, + swapped voices. Returns the new run id.""" + self._seed(tmp, clusters={ + "SPEAKER_0": {"embedding": [0.0, 1.0], "speech_duration_seconds": 55.0, + "segment_count": 9, "segments": [{"start": 2.0, "end": 6.0}]}, + "SPEAKER_1": {"embedding": [1.0, 0.0], "speech_duration_seconds": 35.0, + "segment_count": 7, "segments": [{"start": 21.0, "end": 25.0}]}, + }) + return self._run_id(tmp) + + def _make_legacy(self, tmp): + """Strip the run block, leaving the pre-run-stamping sidecar shape + every already-processed meeting on disk still has.""" + output_dir = Path(tmp) / "output" + sidecar = read_speakers_sidecar(output_dir, "mtg001") + sidecar.pop("diarization_run", None) + write_sidecar_document(output_dir, "mtg001", sidecar) + + def _confirm_by_hand(self, cfg, name, sid, run_id, embedding=(1.0, 0.0)): + person = cfg.create_person_profile(name) + cfg.add_speaker_prototype( + person["person_id"], list(embedding), recording_type="remote", + meeting_id="mtg001", diarization_speaker_id=sid, + speech_duration_seconds=60.0, segment_count=10, + created_from="user_confirmed", channel="system", + diarization_run_id=run_id, + ) + return person + + def test_a_confirmation_from_a_superseded_run_is_not_reported_as_confirmed(self): + with tempfile.TemporaryDirectory() as tmp: + self._seed(tmp) + cfg = Config(config_path=Path(tmp) / "config.json") + person = self._confirm_by_hand(cfg, "Julian", "SPEAKER_0", self._run_id(tmp)) + self._rediarize(tmp) + data = _last_json(self._run(["mtg001"], tmp, cfg=cfg).output) + cluster = data["channels"]["system"]["SPEAKER_0"] + self.assertIsNone(cluster["confirmed_by_user"]) + self.assertIsNone(cluster["confirmed_person_id"]) + self.assertEqual( + data["stale_assignments"], + [{"person_id": person["person_id"], "display_name": "Julian"}], + ) + + def test_a_confirmation_from_this_run_is_reported_and_is_not_stale(self): + with tempfile.TemporaryDirectory() as tmp: + self._seed(tmp) + cfg = Config(config_path=Path(tmp) / "config.json") + person = self._confirm_by_hand(cfg, "Julian", "SPEAKER_0", self._run_id(tmp)) + data = _last_json(self._run(["mtg001"], tmp, cfg=cfg).output) + cluster = data["channels"]["system"]["SPEAKER_0"] + self.assertEqual(cluster["confirmed_by_user"], "Julian") + self.assertEqual(cluster["confirmed_person_id"], person["person_id"]) + self.assertEqual(data["stale_assignments"], []) + + def test_a_legacy_pair_with_no_run_ids_anywhere_still_reports_the_confirmation(self): + # The whole installed base: sidecars written before run stamping, + # prototypes confirmed against them. Nothing here was ever + # re-diarized, so nothing may be reported as superseded. + with tempfile.TemporaryDirectory() as tmp: + self._seed(tmp) + self._make_legacy(tmp) + cfg = Config(config_path=Path(tmp) / "config.json") + self._confirm_by_hand(cfg, "Julian", "SPEAKER_0", None) + data = _last_json(self._run(["mtg001"], tmp, cfg=cfg).output) + self.assertEqual( + data["channels"]["system"]["SPEAKER_0"]["confirmed_by_user"], "Julian", + ) + self.assertEqual(data["stale_assignments"], []) + + def test_a_person_who_lost_two_clusters_is_reported_once(self): + with tempfile.TemporaryDirectory() as tmp: + self._seed(tmp) + cfg = Config(config_path=Path(tmp) / "config.json") + person = self._confirm_by_hand(cfg, "Julian", "SPEAKER_0", self._run_id(tmp)) + cfg.add_speaker_prototype( + person["person_id"], [0.0, 1.0], recording_type="remote", + meeting_id="mtg001", diarization_speaker_id="SPEAKER_1", + speech_duration_seconds=40.0, segment_count=8, + created_from="user_confirmed", channel="system", + diarization_run_id=self._run_id(tmp), + ) + self._rediarize(tmp) + data = _last_json(self._run(["mtg001"], tmp, cfg=cfg).output) + self.assertEqual(len(data["stale_assignments"]), 1) + self.assertEqual(data["stale_assignments"][0]["display_name"], "Julian") + + def test_a_cluster_someone_has_since_confirmed_reports_no_stale_owner(self): + # The notice has to be able to go away. Nothing deletes a + # superseded prototype -- that is the point of the run scoping -- so + # if a re-confirmed cluster kept reporting its previous owner, the + # panel would carry the notice for the rest of the meeting's life + # with no action left that could clear it. + with tempfile.TemporaryDirectory() as tmp: + self._seed(tmp) + cfg = Config(config_path=Path(tmp) / "config.json") + self._confirm_by_hand(cfg, "Julian", "SPEAKER_0", self._run_id(tmp)) + new_run = self._rediarize(tmp) + self._confirm_by_hand(cfg, "Sarah", "SPEAKER_0", new_run, embedding=(0.0, 1.0)) + data = _last_json(self._run(["mtg001"], tmp, cfg=cfg).output) + self.assertEqual( + data["channels"]["system"]["SPEAKER_0"]["confirmed_by_user"], "Sarah", + ) + self.assertEqual(data["stale_assignments"], []) + + if __name__ == "__main__": unittest.main() From 43217fe1d561501cdc201eced8292b0b51f2de6b Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 4 Aug 2026 21:27:03 +0200 Subject: [PATCH 09/23] fix(speakers): stop the participants backfill from renaming a re-diarized transcript A fifth reader of "this prototype IS this cluster", and the one that writes the result into the file the user reads as the record of the meeting. `backfill-participants --relabel-transcripts` selected prototypes by meeting+channel+cluster id only, so after a re-diarization it wrote one participant's name onto whichever voice inherited that id. It already skipped an id that vanished from the new run; the reused id - the more likely case, since the diarizer numbers from SPEAKER_00 every time - went through. The participants line itself stays meeting-scoped and unchanged: being confirmed as present in a meeting stays true however often the audio is re-diarized. Only the relabel claim is run-scoped. Not listed in the spec's section 5, found by grepping the remaining prototype readers after wiring the four it does list. Five fixtures paired an unstamped prototype with a stamped sidecar. Two of them assert that NOTHING gets relabeled, so they had started passing for the wrong reason - the run scope skipped the prototype before the property under test could fail. --- simple_recorder.py | 16 +++++ tests/test_backfill_participants_cli.py | 78 +++++++++++++++++++++---- 2 files changed, 84 insertions(+), 10 deletions(-) diff --git a/simple_recorder.py b/simple_recorder.py index 97973a26..32242203 100644 --- a/simple_recorder.py +++ b/simple_recorder.py @@ -6220,6 +6220,7 @@ def backfill_participants(relabel_transcripts): confirmed_participant_names, merge_same_channel_fragments, prototype_channel_matches, + prototype_run_matches, read_speakers_sidecar, relabel_transcript_exact, relabel_transcript_multi, @@ -6262,6 +6263,12 @@ def backfill_participants(relabel_transcripts): # needs pooled_segments. Building both unconditionally is cheap # and keeps the branch below simple. turn_manifest = sidecar.get("transcript_lines") + # Which run these cluster ids belong to. Relabeling reads a + # prototype as "this person IS this cluster" and then writes their + # name into the transcript, so it is scoped like every other reader + # of a current assignment -- and unlike the participants line + # above, which is deliberately not (see confirmed_participant_names). + sidecar_run_id = (sidecar.get("diarization_run") or {}).get("run_id") assignments = [] target_ids_by_name: dict = {} for channel_name, channel_data in (sidecar.get("channels") or {}).items(): @@ -6275,6 +6282,15 @@ def backfill_participants(relabel_transcripts): prototype, channel_name, recording_type, ): continue + if not prototype_run_matches(prototype, sidecar_run_id): + # Confirmed against a run this sidecar no longer + # describes. The id survived the re-diarization, but + # the voice behind it did not, so writing this name + # onto its lines would put one participant's name on + # another's words -- the failure this whole slice + # exists to stop, and here it lands in the file the + # user reads as the record of the meeting. + continue sid = prototype.get("diarization_speaker_id") if sid not in id_resolution: continue # sidecar regenerated since this prototype was confirmed diff --git a/tests/test_backfill_participants_cli.py b/tests/test_backfill_participants_cli.py index a01bf872..8d83f702 100644 --- a/tests/test_backfill_participants_cli.py +++ b/tests/test_backfill_participants_cli.py @@ -8,7 +8,7 @@ import simple_recorder from src.config import Config -from src.speaker_suggestions import write_speakers_sidecar +from src.speaker_suggestions import read_speakers_sidecar, write_speakers_sidecar def _last_json(output): @@ -29,17 +29,26 @@ def _run(self, args, tmp, cfg): result = CliRunner().invoke(simple_recorder.backfill_participants, args) return result - def _confirm(self, cfg, person_name, meeting_id, sid="SPEAKER_00", recording_type="in_person"): + def _confirm(self, cfg, person_name, meeting_id, sid="SPEAKER_00", recording_type="in_person", + run_id=None): person = cfg.create_person_profile(person_name) cfg.add_speaker_prototype( person["person_id"], [1.0, 0.0], recording_type=recording_type, meeting_id=meeting_id, diarization_speaker_id=sid, speech_duration_seconds=30.0, segment_count=5, - created_from="user_confirmed", + created_from="user_confirmed", diarization_run_id=run_id, ) return person + def _run_id(self, tmp, meeting_stem="mtg001"): + """The seeded sidecar's run id, for tests that hand-build a prototype + against it. Unstamped it would describe a confirmation made before + the meeting was re-diarized, which the relabel path deliberately + refuses -- and several of these tests assert that nothing gets + relabeled, so they would pass for the wrong reason.""" + return read_speakers_sidecar(Path(tmp) / "output", meeting_stem)["diarization_run"]["run_id"] + def test_writes_participants_for_meeting_with_no_prior_section(self): with tempfile.TemporaryDirectory() as tmp: output_dir = Path(tmp) / "output" @@ -109,7 +118,7 @@ def test_relabel_transcripts_flag_off_by_default(self): original = "Session: mtg001\n\n" + "=" * 60 + "\n\n[00:05] [Speaker 2] hello there" transcript_path.write_text(original, encoding="utf-8") cfg = Config(config_path=Path(tmp) / "config.json") - self._confirm(cfg, "Max", "mtg001") + self._confirm(cfg, "Max", "mtg001", run_id=self._run_id(tmp)) result = self._run([], tmp, cfg) data = _last_json(result.output) @@ -141,7 +150,7 @@ def test_relabel_transcripts_flag_relabels_confirmed_clusters(self): encoding="utf-8", ) cfg = Config(config_path=Path(tmp) / "config.json") - self._confirm(cfg, "Max", "mtg001") + self._confirm(cfg, "Max", "mtg001", run_id=self._run_id(tmp)) result = self._run(["--relabel-transcripts"], tmp, cfg) data = _last_json(result.output) @@ -182,7 +191,7 @@ def test_relabel_transcripts_uses_exact_matching_when_sidecar_has_manifest(self) encoding="utf-8", ) cfg = Config(config_path=Path(tmp) / "config.json") - self._confirm(cfg, "Max", "mtg001") + self._confirm(cfg, "Max", "mtg001", run_id=self._run_id(tmp)) result = self._run(["--relabel-transcripts"], tmp, cfg) data = _last_json(result.output) @@ -219,7 +228,7 @@ def test_relabel_transcripts_exact_match_relabels_the_right_line(self): encoding="utf-8", ) cfg = Config(config_path=Path(tmp) / "config.json") - self._confirm(cfg, "Max", "mtg001") + self._confirm(cfg, "Max", "mtg001", run_id=self._run_id(tmp)) result = self._run(["--relabel-transcripts"], tmp, cfg) data = _last_json(result.output) @@ -252,7 +261,7 @@ def test_relabel_transcripts_is_idempotent_on_already_relabeled_meeting(self): encoding="utf-8", ) cfg = Config(config_path=Path(tmp) / "config.json") - self._confirm(cfg, "Max", "mtg001") + self._confirm(cfg, "Max", "mtg001", run_id=self._run_id(tmp)) result = self._run(["--relabel-transcripts"], tmp, cfg) data = _last_json(result.output) @@ -298,8 +307,8 @@ def test_cross_channel_collision_is_skipped_not_guessed(self): encoding="utf-8", ) cfg = Config(config_path=Path(tmp) / "config.json") - self._confirm(cfg, "Valentin Weyer", "mtg001", sid="SPEAKER_00", recording_type="in_person") - self._confirm(cfg, "Inga Hahn", "mtg001", sid="SPEAKER_00", recording_type="remote") + self._confirm(cfg, "Valentin Weyer", "mtg001", sid="SPEAKER_00", recording_type="in_person", run_id=self._run_id(tmp)) + self._confirm(cfg, "Inga Hahn", "mtg001", sid="SPEAKER_00", recording_type="remote", run_id=self._run_id(tmp)) result = self._run(["--relabel-transcripts"], tmp, cfg) data = _last_json(result.output) @@ -308,6 +317,55 @@ def test_cross_channel_collision_is_skipped_not_guessed(self): self.assertEqual(data["transcripts_skipped_ambiguous"], {"mtg001": 1}) self.assertIn("[00:05] [Speaker 3] contested line", transcript_path.read_text()) + def test_relabel_transcripts_leaves_a_re_diarized_meeting_alone(self): + # This command writes a person's NAME onto transcript lines, chosen + # by cluster id. A re-diarization gives that id to whichever voice + # the diarizer numbered first this time, so an unscoped run of this + # backfill puts one participant's name on another participant's + # words -- silently, in the file the user reads as the record of the + # meeting. Not relabeling is the only honest answer: after a + # re-diarization nothing here knows which cluster was theirs. + # + # Participants are a different question and stay meeting-scoped: + # they were confirmed as present in this meeting, and that stays + # true however often the audio is re-diarized. + with tempfile.TemporaryDirectory() as tmp: + output_dir = Path(tmp) / "output" + output_dir.mkdir(parents=True, exist_ok=True) + channels = { + "mic": { + "recording_type": "in_person", + "clusters": { + "SPEAKER_00": { + "embedding": [1.0, 0.0], "speech_duration_seconds": 30.0, "segment_count": 5, + "segments": [{"start": 4.0, "end": 6.0}], + }, + }, + }, + } + write_speakers_sidecar(output_dir, "mtg001", channels) + run1 = read_speakers_sidecar(output_dir, "mtg001")["diarization_run"]["run_id"] + transcripts_dir = Path(tmp) / "transcripts" + transcripts_dir.mkdir(parents=True, exist_ok=True) + transcript_path = transcripts_dir / "mtg001_transcript.txt" + transcript_path.write_text( + "Session: mtg001\n\n" + "=" * 60 + "\n\n[00:05] [Speaker 2] hello there", + encoding="utf-8", + ) + cfg = Config(config_path=Path(tmp) / "config.json") + self._confirm(cfg, "Max", "mtg001", run_id=run1) + + write_speakers_sidecar(output_dir, "mtg001", channels) # re-diarized + + result = self._run(["--relabel-transcripts"], tmp, cfg) + data = _last_json(result.output) + self.assertTrue(data["success"]) + self.assertEqual(data["transcripts_relabeled"], {}) + self.assertIn("[00:05] [Speaker 2] hello there", transcript_path.read_text()) + self.assertEqual( + data["meetings_updated"], [{"meeting_id": "mtg001", "participants": ["Max"]}], + ) + if __name__ == "__main__": unittest.main() From 29c5c42864ed2365a66a9dfa0c0e4b4da00f78d2 Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 4 Aug 2026 21:36:00 +0200 Subject: [PATCH 10/23] fix(speakers): teach repair-speaker-profiles that two runs are not one All three of its passes compared evidence across diarization runs as if the cluster ids meant the same thing in each, which since the run scoping they do not: Pass B deduped on (meeting, sid, channel) and kept the OLDEST entry. One person legitimately holds that key twice now - once from the run they were confirmed in, once from after a re-diarization - so the repair tool deleted the entry describing the meeting as it is now and kept the superseded one. This pair only exists because confirmations became run-scoped, so the tool undid the guarantee that change was made for. Pass A read "its owner holds this id on the other channel" as a cross-channel collision even when the two entries came from different runs, where it means nothing, and dropped a hard negative that was right for its own run. Pass C resolved a channel-less entry's id against whatever sidecar is on disk. From a later run that is a guess, and it would be written down as recorded fact for every prototype_channel_matches afterwards. Found by asking Codex which prototype readers were left after wiring the four the spec lists; verified against the code rather than taken on trust. Its fourth finding - a person whose superseded cluster id is gone from the new run never reaching stale_assignments - is real, unactionable by the notice it feeds, and now written down where that notice is built. The channel-backfill fixture paired a channel-less prototype with a stamped sidecar; a build old enough to write no channel wrote no run block either, so the sidecars there are now legacy-shaped too. --- simple_recorder.py | 39 +++++++++- tests/test_repair_speaker_profiles_cli.py | 90 ++++++++++++++++++++++- 2 files changed, 125 insertions(+), 4 deletions(-) diff --git a/simple_recorder.py b/simple_recorder.py index 32242203..daad5374 100644 --- a/simple_recorder.py +++ b/simple_recorder.py @@ -5769,6 +5769,14 @@ def suggest_speakers(meeting_stem): # the link. Empty is the normal case, including on every legacy # library, and their voice evidence is untouched either way -- it # keeps scoring candidates in every meeting. + # + # Known gap, accepted: someone whose only superseded prototype names + # a cluster id the new run does not produce at all is never listed, + # because the collection walks this run's clusters. Their assignment + # really is orphaned, but no row here can carry them and the notice + # says "re-confirm them", which they cannot. It only goes unnoticed + # once every surviving cluster is confirmed -- until then the notice + # is up anyway for the others. "stale_assignments": [ {"person_id": pid, "display_name": name} for pid, name in stale_assignments.items() @@ -6495,7 +6503,7 @@ def repair_speaker_profiles(apply_changes): prototype_channel_matches shrinks to entries whose sidecar is gone. """ from src.config import get_config, get_data_dirs - from src.speaker_suggestions import read_speakers_sidecar + from src.speaker_suggestions import prototype_run_matches, read_speakers_sidecar config = get_config() profiles = config.get_person_profiles() @@ -6520,11 +6528,18 @@ def _stats(person): n_rt = negative.get("recording_type") if not negative.get("prototype_id") or not n_meeting or not n_sid or n_rt in (None, "unknown"): continue + # Same run only. "This negative cites a cluster its owner holds + # on the other channel" is evidence of a collision only if both + # entries describe the same diarization run; across runs the id + # was simply handed to a different voice, and reading that as a + # collision would delete a negative that is exactly right for + # the run it came from. owner_rts = { p.get("recording_type") for other in profiles if other["person_id"] != person["person_id"] for p in (other.get("prototypes") or []) if p.get("meeting_id") == n_meeting and p.get("diarization_speaker_id") == n_sid + and prototype_run_matches(p, negative.get("diarization_run_id")) } owner_rts.discard(None) owner_rts.discard("unknown") @@ -6539,6 +6554,11 @@ def _stats(person): # Pass B -- duplicates within one person's list (oldest kept). The key # includes channel (recording_type for legacy entries): the same # SPEAKER_N on mic and system are different clusters, not duplicates. + # It includes the diarization run for the same reason one step further + # out: since confirmations are run-scoped, one person legitimately holds + # the same meeting+channel+id twice, once per run. Without the run in + # the key this pass drops the NEWER of the two -- keeping the superseded + # entry and deleting the one that describes the meeting as it is now. for person in profiles: for negative_flag, key_name in ((False, "prototypes"), (True, "hard_negatives")): seen = set() @@ -6553,7 +6573,11 @@ def _stats(person): sid = entry.get("diarization_speaker_id") if not meeting_id or not sid: continue - dedupe_key = (meeting_id, sid, entry.get("channel") or entry.get("recording_type")) + dedupe_key = ( + meeting_id, sid, + entry.get("channel") or entry.get("recording_type"), + entry.get("diarization_run_id"), + ) if dedupe_key in seen: drops.setdefault((person["person_id"], negative_flag), set()).add(entry.get("prototype_id")) _stats(person)["duplicates_removed"] += 1 @@ -6585,6 +6609,17 @@ def _stats(person): sidecar = sidecar_cache[meeting_id] if sidecar is None: continue + if not prototype_run_matches( + entry, (sidecar.get("diarization_run") or {}).get("run_id"), + ): + # The sidecar describes a different run, so the cluster + # this id resolves to is whatever the diarizer numbered + # that way this time. Writing its channel onto the entry + # would turn a guess into recorded fact, and every later + # prototype_channel_matches would trust it. Left legacy, + # it keeps the recording_type proxy, which at least + # admits to being one. + continue owners = [ name for name, ch in (sidecar.get("channels") or {}).items() if sid in (ch.get("clusters") or {}) diff --git a/tests/test_repair_speaker_profiles_cli.py b/tests/test_repair_speaker_profiles_cli.py index eeead9c6..c1f6354f 100644 --- a/tests/test_repair_speaker_profiles_cli.py +++ b/tests/test_repair_speaker_profiles_cli.py @@ -8,7 +8,11 @@ import simple_recorder from src.config import Config -from src.speaker_suggestions import write_speakers_sidecar +from src.speaker_suggestions import ( + read_speakers_sidecar, + write_sidecar_document, + write_speakers_sidecar, +) def _report(output): @@ -25,13 +29,14 @@ def _run(self, args, tmp, cfg): return result def _add(self, cfg, person_id, meeting_id, sid, recording_type, - channel=None, negative=False): + channel=None, negative=False, run_id=None): return cfg.add_speaker_prototype( person_id, [0.1, 0.2], recording_type=recording_type, meeting_id=meeting_id, diarization_speaker_id=sid, speech_duration_seconds=25.0, segment_count=4, created_from="user_confirmed", channel=channel, negative=negative, + diarization_run_id=run_id, ) def _seed_collision_library(self, cfg): @@ -118,6 +123,15 @@ def test_same_sid_on_different_channels_is_not_a_duplicate(self): self.assertEqual(report["duplicates_removed"], 0) self.assertEqual(len(cfg.get_person_profile(alice["person_id"])["prototypes"]), 2) + def _strip_run_block(self, output_dir, meeting_stem): + """Make a sidecar legacy-shaped. A prototype with no `channel` comes + from a build that wrote no run block either, so pairing one with a + freshly stamped sidecar would describe a meeting re-diarized since + that confirm -- which the backfill deliberately refuses.""" + sidecar = read_speakers_sidecar(output_dir, meeting_stem) + sidecar.pop("diarization_run", None) + write_sidecar_document(output_dir, meeting_stem, sidecar) + def test_apply_backfills_channel_from_sidecar(self): with tempfile.TemporaryDirectory() as tmp: output_dir = Path(tmp) / "output" @@ -135,6 +149,8 @@ def test_apply_backfills_channel_from_sidecar(self): "system": {"recording_type": "remote", "clusters": {"SPEAKER_00": {"embedding": [0.0, 1.0], "speech_duration_seconds": 30.0, "segment_count": 5}}}, }) + self._strip_run_block(output_dir, "mtg003") + self._strip_run_block(output_dir, "mtg004") cfg = Config(config_path=Path(tmp) / "config.json") alice = cfg.create_person_profile("Alice") unambiguous = self._add(cfg, alice["person_id"], "mtg003", "SPEAKER_00", "in_person") @@ -155,6 +171,76 @@ def test_apply_backfills_channel_from_sidecar(self): # recording_type fallback path. self.assertNotIn("channel", by_id[orphan["prototype_id"]]) + def test_two_runs_of_the_same_cluster_id_are_not_duplicates(self): + # Since confirmations are run-scoped, one person can legitimately + # hold the same meeting+channel+cluster id twice: once from the run + # they were confirmed in, once from the run after a re-diarization. + # Deduping them by id alone deletes the NEWER entry (oldest is + # kept), so the repair tool would leave the superseded prototype + # standing and remove the only one that describes the meeting as it + # exists now -- the exact inversion of what it is for. + with tempfile.TemporaryDirectory() as tmp: + cfg = Config(config_path=Path(tmp) / "config.json") + alice = cfg.create_person_profile("Alice") + self._add(cfg, alice["person_id"], "mtg002", "SPEAKER_00", "in_person", + channel="mic", run_id="run-1") + self._add(cfg, alice["person_id"], "mtg002", "SPEAKER_00", "in_person", + channel="mic", run_id="run-2") + result = self._run(["--apply"], tmp, cfg) + report = _report(result.output) + self.assertEqual(report["duplicates_removed"], 0) + self.assertEqual(len(cfg.get_person_profile(alice["person_id"])["prototypes"]), 2) + + def test_a_negative_is_not_called_a_collision_against_another_runs_owner(self): + # Pass A calls a negative a cross-channel collision when the person + # who owns that cluster id owns it on the OTHER channel. Across two + # runs that comparison means nothing -- the id was reassigned, not + # mis-channeled -- and acting on it deletes a hard negative that is + # still exactly right for its own run. + with tempfile.TemporaryDirectory() as tmp: + cfg = Config(config_path=Path(tmp) / "config.json") + bob = cfg.create_person_profile("Bob") + alice = cfg.create_person_profile("Alice") + # Alice owns SPEAKER_00 on system, but only in the LATER run. + self._add(cfg, alice["person_id"], "mtg001", "SPEAKER_00", "remote", + channel="system", run_id="run-2") + negative = self._add(cfg, bob["person_id"], "mtg001", "SPEAKER_00", "in_person", + channel="mic", negative=True, run_id="run-1") + result = self._run(["--apply"], tmp, cfg) + report = _report(result.output) + self.assertEqual(report["collision_negatives_dropped"], 0) + self.assertEqual( + [n["prototype_id"] for n in cfg.get_person_profile(bob["person_id"])["hard_negatives"]], + [negative["prototype_id"]], + ) + + def test_channel_is_not_backfilled_from_a_run_the_entry_never_belonged_to(self): + # Pass C resolves a channel-less entry's cluster id against the + # sidecar on disk. If that sidecar is from a later run, the id it + # resolves belongs to whatever the diarizer numbered that way this + # time, so the channel written onto the entry is a guess -- and it + # then feeds prototype_channel_matches everywhere as if it were + # recorded fact. Leaving it legacy keeps the recording_type + # fallback, which is honest about being a proxy. + with tempfile.TemporaryDirectory() as tmp: + output_dir = Path(tmp) / "output" + output_dir.mkdir(parents=True, exist_ok=True) + write_speakers_sidecar(output_dir, "mtg003", { + "mic": {"recording_type": "in_person", + "clusters": {"SPEAKER_00": {"embedding": [1.0, 0.0], "speech_duration_seconds": 30.0, "segment_count": 5}}}, + }) + cfg = Config(config_path=Path(tmp) / "config.json") + alice = cfg.create_person_profile("Alice") + entry = self._add(cfg, alice["person_id"], "mtg003", "SPEAKER_00", "in_person", + run_id="a-run-that-is-not-on-disk") + + result = self._run(["--apply"], tmp, cfg) + report = _report(result.output) + self.assertEqual(report["channels_backfilled"], 0) + stored = cfg.get_person_profile(alice["person_id"])["prototypes"][0] + self.assertEqual(stored["prototype_id"], entry["prototype_id"]) + self.assertNotIn("channel", stored) + def test_clean_library_reports_all_zeroes(self): with tempfile.TemporaryDirectory() as tmp: cfg = Config(config_path=Path(tmp) / "config.json") From cbe5c25c4445143d2213b9f256e71cd3a17181ef Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 4 Aug 2026 21:36:39 +0200 Subject: [PATCH 11/23] docs(speakers): tick off task 5 in the plan --- .../2026-08-04-speaker-review-run-provenance.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md b/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md index 9d8736e1..c3fc4665 100644 --- a/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md +++ b/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md @@ -124,12 +124,12 @@ - Consumes: Task 2's predicate. - Produces: `suggest-speakers` gains a top-level `stale_assignments` array of `{person_id, display_name}`. The renderer (Task 7) renders one meeting-level notice from it. Absent or empty means nothing to report. -- [ ] **Step 1: Write the failing tests.** A prototype from run `r1` against a sidecar stamped `r2` must **not** produce `confirmed_by_user` or `confirmed_person_id` for that cluster, and must appear once in `stale_assignments`. A prototype from `r2` against `r2` behaves exactly as today. A legacy pair (no ids at all) behaves exactly as today. Add one test that `speaker-naming-status` does not count a stale prototype's cluster as named. -- [ ] **Step 2: Run and watch them fail.** -- [ ] **Step 3: Apply the predicate at every reader.** The `confirmed_by_user` / `confirmed_person_id` derivation; `still_present`; the mutual-negative `matches` selection - without scoping there, a stale old-run prototype whose sid collides with a new-run sid would be treated as owning the new cluster and would seed negatives built from the new run's embeddings. Then assemble `stale_assignments`, deduped per person. -- [ ] **Step 4: Leave participants alone, and say why in the code.** `confirmed_participant_names` stays meeting-scoped. Add the comment: attendance is a property of the meeting, not of a run, and run-filtering the `full-reprocess` restore would empty the section on every reprocess. Without that comment a later reader will "fix" it. -- [ ] **Step 5: Run green, then the whole suite.** -- [ ] **Step 6: Commit.** +- [x] **Step 1: Write the failing tests.** A prototype from run `r1` against a sidecar stamped `r2` must **not** produce `confirmed_by_user` or `confirmed_person_id` for that cluster, and must appear once in `stale_assignments`. A prototype from `r2` against `r2` behaves exactly as today. A legacy pair (no ids at all) behaves exactly as today. Add one test that `speaker-naming-status` does not count a stale prototype's cluster as named. +- [x] **Step 2: Run and watch them fail.** +- [x] **Step 3: Apply the predicate at every reader.** The `confirmed_by_user` / `confirmed_person_id` derivation; `still_present`; the mutual-negative `matches` selection - without scoping there, a stale old-run prototype whose sid collides with a new-run sid would be treated as owning the new cluster and would seed negatives built from the new run's embeddings. Then assemble `stale_assignments`, deduped per person. +- [x] **Step 4: Leave participants alone, and say why in the code.** `confirmed_participant_names` stays meeting-scoped. Add the comment: attendance is a property of the meeting, not of a run, and run-filtering the `full-reprocess` restore would empty the section on every reprocess. Without that comment a later reader will "fix" it. +- [x] **Step 5: Run green, then the whole suite.** +- [x] **Step 6: Commit.** --- From aad6ad5fd2477ca0fea6ab298596f0ee762f19e3 Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 4 Aug 2026 21:49:38 +0200 Subject: [PATCH 12/23] feat(speakers): persist "keep generic" instead of losing it on unmount "Keep generic" was the only review outcome that changed nothing on disk: it added the row key to a React state set and filtered the row out client-side. Navigating away and back re-presented every row the reviewer had already dealt with - the exact work the button exists to save. It is now a per-cluster sidecar key with one value, written by a new `set-cluster-review-state` CLI that mirrors mark-speaker-cluster's shape and never-raises contract, and echoed back per cluster by suggest-speakers so the panel derives it from persisted state rather than component state. One value on purpose: "assigned" is already derivable from a prototype and "mixed" already has its own key, so recording either here would be a second copy with a consistency obligation and no gain. It changes no score and no suggestion status. A reviewer parking a row is a statement about their progress, not about the voice - as evidence it would let a shrug quietly suppress a real match. Merged rows follow contains_multiple_speakers exactly: the mark is written on the raw id it was handed, and the merged row reads generic when any member carries it. Both transitions that supersede it - a confirm and a mixed marking - sweep the whole fragment set, so a mark cannot survive on a fragment nobody can see or click. --- simple_recorder.py | 92 +++++++++++++++++ src/speaker_suggestions.py | 117 +++++++++++++++++++++ tests/test_speaker_multi_marking.py | 152 ++++++++++++++++++++++++++++ tests/test_speaker_suggestions.py | 133 ++++++++++++++++++++++++ tests/test_suggest_speakers_cli.py | 57 +++++++++++ 5 files changed, 551 insertions(+) diff --git a/simple_recorder.py b/simple_recorder.py index daad5374..f2c95c76 100644 --- a/simple_recorder.py +++ b/simple_recorder.py @@ -4918,6 +4918,7 @@ def confirm_speaker(meeting_stem, channel, diarization_speaker_id, person_id, ne """ from src.config import get_config, get_data_dirs from src.speaker_suggestions import ( + clear_cluster_review_state, clusters_from_sidecar_channel, confirmed_participant_names, merge_same_channel_fragments, @@ -5214,6 +5215,12 @@ def confirm_speaker(meeting_stem, channel, diarization_speaker_id, person_id, ne pooled_segments.extend(raw_clusters_by_id.get(fragment_id, {}).get("segments") or []) relabeled_lines = relabel_transcript_speaker(transcript_path, pooled_segments, person["display_name"]) + # Naming the cluster supersedes "a human kept this generic": the row is + # now decided, and leaving the marking would have the panel report a + # confirmed row as still parked. Swept across every fragment, because + # the merged row reads generic when ANY member carries the key. + clear_cluster_review_state(output_dir, meeting_stem, channel, fragment_ids) + # Cheap and always-safe (unlike transcript relabeling, no reason to # gate this behind a flag) -- keeps the meeting's Participants chip in # sync with every confirm, including plain CLI/backfill-validation use. @@ -5273,6 +5280,78 @@ def speaker_timestamps(meeting_stem, channel, diarization_speaker_id): print(f" [{_format_timestamp(seg['start'])} - {_format_timestamp(seg['end'])}]") +@cli.command(name='set-cluster-review-state') +@click.argument('meeting_stem') +@click.argument('channel') +@click.argument('diarization_speaker_id') +@click.option( + '--generic/--clear', 'generic', default=True, + help="--generic (default) records that a human reviewed this cluster and " + "chose to leave it unnamed; --clear removes that marking.", +) +def set_cluster_review_state_command(meeting_stem, channel, diarization_speaker_id, generic): + """Record how far the review got on one diarized cluster. + + "Keep generic" is the only review outcome that leaves no other trace. A + confirm writes a prototype, a mixed marking writes its own key, but + deciding to leave a row alone used to change nothing on disk -- so a + restart, or merely navigating away and back, re-presented every row the + reviewer had already dealt with. That is the work the button exists to + save, undone by the panel unmounting. + + It changes no score and no suggestion: it says the reviewer stopped + here, not that the voice is unidentifiable. Naming the cluster or + marking it as holding several people supersedes it, and both clear it. + """ + from src.config import get_data_dirs + from src.speaker_suggestions import ( + REVIEW_STATE_GENERIC, + clusters_from_sidecar_channel, + merge_same_channel_fragments, + set_cluster_review_state, + ) + + output_dir = get_data_dirs()["output"] + state = REVIEW_STATE_GENERIC if generic else None + sidecar = set_cluster_review_state( + output_dir, meeting_stem, channel, diarization_speaker_id, state, + ) + if sidecar is None: + print(json.dumps({ + "success": False, + "error": f"No cluster {diarization_speaker_id!r} in {channel!r} channel of {meeting_stem!r}", + })) + sys.exit(1) + + # The reach, not just the id: the panel shows merged rows, so a caller + # needs to know which raw clusters the row it just marked covers -- + # same reporting mark-speaker-cluster does, and the same reason. + channel_data = (sidecar.get("channels") or {}).get(channel) or {} + resolved_id = diarization_speaker_id + fragment_ids = [diarization_speaker_id] + try: + clusters, id_resolution = merge_same_channel_fragments( + clusters_from_sidecar_channel(meeting_stem, channel_data) + ) + except (KeyError, TypeError, ValueError): + # A sidecar whose embeddings cannot be merged still took the mark; + # report the raw id rather than failing an action that succeeded. + clusters, id_resolution = {}, {} + resolved_id = id_resolution.get(diarization_speaker_id, diarization_speaker_id) + if resolved_id in clusters: + fragment_ids = [resolved_id, *clusters[resolved_id][1].merged_from] + + print(json.dumps({ + "success": True, + "meeting_id": meeting_stem, + "channel": channel, + "diarization_speaker_id": diarization_speaker_id, + "resolved_diarization_speaker_id": resolved_id, + "fragment_ids": fragment_ids, + "review_state": state, + })) + + @cli.command(name='mark-speaker-cluster') @click.argument('meeting_stem') @click.argument('channel') @@ -5306,6 +5385,7 @@ def mark_speaker_cluster(meeting_stem, channel, diarization_speaker_id, multiple """ from src.config import get_config, get_data_dirs from src.speaker_suggestions import ( + clear_cluster_review_state, confirmed_participant_names, merge_same_channel_fragments, clusters_from_sidecar_channel, @@ -5363,6 +5443,12 @@ def mark_speaker_cluster(meeting_stem, channel, diarization_speaker_id, multiple cleared_from = [] restored_lines = 0 if multiple and fragment_ids: + # "A human kept this generic" is superseded by "a human says it is + # several people" -- the second is a stronger statement about the + # same cluster. Swept across every fragment, because the merged row + # reads generic when ANY member carries the key, so a leftover on a + # fragment would keep marking a row nobody can click. + clear_cluster_review_state(output_dir, meeting_stem, channel, fragment_ids) # Run-scoped for the same reason the confirm path is: the cluster # this marking describes exists only within this run, so an entry # from a superseded run shares nothing with it but a reused id. @@ -5739,6 +5825,12 @@ def suggest_speakers(meeting_stem): # real case this was built for). True means a human said so, # and this cluster is out of naming for good. "contains_multiple_speakers": context.contains_multiple_speakers, + # Set by `set-cluster-review-state`. Echoed so the panel can + # read a reviewer's "leave this one generic" back out of + # persisted state instead of component state -- which is + # what makes it survive a remount and a restart. Changes no + # score and no status: it is progress, not evidence. + "review_state": context.review_state, # Same signal already used to gate suggestion status (real- # data-validated this session against the echo/crosstalk # artifact pattern) -- reused here to flag likely-artifact diff --git a/src/speaker_suggestions.py b/src/speaker_suggestions.py index 465b0a9a..341d2465 100644 --- a/src/speaker_suggestions.py +++ b/src/speaker_suggestions.py @@ -248,6 +248,11 @@ class ClusterContext: # enrolling it as anyone's voice evidence would poison the profile it # was filed under and every future suggestion scored against it. contains_multiple_speakers: bool = False + # How far a human got reviewing this cluster (see REVIEW_STATE_KEY), or + # None where they have not said. Carried here only so the panel can show + # it back: unlike contains_multiple_speakers it feeds no score and no + # gate, because it describes the reviewer's progress and not the voice. + review_state: Optional[str] = None @dataclass @@ -577,6 +582,19 @@ def union(a, b): contains_multiple_speakers=any( clusters[sid][1].contains_multiple_speakers for sid in members ), + # Same any() as above, for a different reason: the reviewer + # marked the ROW they saw, and which raw fragment carried + # the mark is an implementation detail they never saw. One + # marked member therefore marks the merged row -- and the + # transitions clear the whole member set, so a mark cannot + # survive on a fragment nobody can reach. + review_state=next( + ( + clusters[sid][1].review_state for sid in members + if clusters[sid][1].review_state is not None + ), + None, + ), ), ) @@ -661,6 +679,26 @@ def write_speakers_sidecar( MULTI_SPEAKER_KEY = "contains_multiple_speakers" +# How far a human got reviewing one cluster, for the decisions that produce +# no other trace. Same placement rationale as MULTI_SPEAKER_KEY: written +# into the cluster entry itself so it travels with exactly the cluster it +# describes, only written when set, absent means "not marked". +# +# Exactly one value, deliberately. "Assigned" is already derivable from a +# matching prototype and "mixed" is already MULTI_SPEAKER_KEY; recording +# either here as a second copy would create a consistency obligation with +# no information gain, and the copies would disagree the first time one +# path updated without the other. "Unreviewed" is the absence of +# everything. +# +# It changes no score and no suggestion status. A reviewer saying "leave +# this one generic" is a statement about their own progress, not about the +# voice -- treating it as evidence would let a shrug quietly suppress a +# real match. +REVIEW_STATE_KEY = "review_state" +REVIEW_STATE_GENERIC = "generic" + + def cluster_ids_marked_multi_speaker(channel_data: dict) -> set: """Every raw diarization_speaker_id in one channel marked as holding more than one person.""" @@ -723,6 +761,84 @@ def set_cluster_multi_speaker( return sidecar +def _freshest_channel(output_dir: Path, meeting_stem: str, channel: str) -> tuple: + """The sidecar as it is on disk RIGHT NOW plus its `channel` entry, for a + read-modify-write that must not overwrite a concurrent one. + + Same reasoning as in set_cluster_multi_speaker, and the same limit: it + narrows the race to re-read-until-rename rather than eliminating it. + Returns `(None, None)` when the sidecar or the channel is gone. + """ + sidecar = read_speakers_sidecar(output_dir, meeting_stem) + if sidecar is None: + return None, None + channel_data = (sidecar.get("channels") or {}).get(channel) + if channel_data is None: + return None, None + return sidecar, channel_data + + +def set_cluster_review_state( + output_dir: Path, meeting_stem: str, channel: str, + diarization_speaker_id: str, state: Optional[str], +) -> Optional[dict]: + """Set/clear how far the review got on ONE raw cluster. + + Writes to exactly the id it was handed, like set_cluster_multi_speaker: + the panel shows merged rows, but the sidecar records raw clusters, and + inventing a write to the merge primary would put the mark on an id the + caller never named. The merged view resolves it on the way out (see + merge_same_channel_fragments). + + `state=None` clears. Returns the written sidecar, or None when the + sidecar, channel or cluster does not exist -- callers report that rather + than reporting a mark that never happened. + """ + sidecar, channel_data = _freshest_channel(output_dir, meeting_stem, channel) + if sidecar is None: + return None + cluster = (channel_data.get("clusters") or {}).get(diarization_speaker_id) + if cluster is None: + return None + + if state is None: + cluster.pop(REVIEW_STATE_KEY, None) + else: + cluster[REVIEW_STATE_KEY] = state + + write_sidecar_document(output_dir, meeting_stem, sidecar) + return sidecar + + +def clear_cluster_review_state( + output_dir: Path, meeting_stem: str, channel: str, diarization_speaker_ids, +) -> int: + """Drop the review marking from a whole set of raw cluster ids in one + write. Returns how many actually carried it. + + Takes a set because the transitions that call it (a confirm, a mixed + marking) are about a MERGED row, and the merged view reads generic when + any member carries the key -- clearing only the primary would leave the + row marked by a fragment nobody can see or click. + + Never raises and never reports a failure: it runs after the action it + follows has already succeeded, and a missing sidecar or channel there + means there is nothing to clear, not that the confirm went wrong. + """ + sidecar, channel_data = _freshest_channel(output_dir, meeting_stem, channel) + if sidecar is None: + return 0 + clusters = channel_data.get("clusters") or {} + cleared = 0 + for sid in diarization_speaker_ids: + cluster = clusters.get(sid) + if isinstance(cluster, dict) and cluster.pop(REVIEW_STATE_KEY, None) is not None: + cleared += 1 + if cleared: + write_sidecar_document(output_dir, meeting_stem, sidecar) + return cleared + + def write_sidecar_document(output_dir: Path, meeting_stem: str, sidecar: dict) -> None: """Replace a meeting's whole sidecar document atomically. @@ -835,6 +951,7 @@ def clusters_from_sidecar_channel(meeting_id: str, channel: dict) -> dict: speech_duration_seconds=cluster.get("speech_duration_seconds", 0.0), segment_count=cluster.get("segment_count", 0), contains_multiple_speakers=bool(cluster.get(MULTI_SPEAKER_KEY)), + review_state=cluster.get(REVIEW_STATE_KEY), ), ) return out diff --git a/tests/test_speaker_multi_marking.py b/tests/test_speaker_multi_marking.py index 4f62507a..721ef66f 100644 --- a/tests/test_speaker_multi_marking.py +++ b/tests/test_speaker_multi_marking.py @@ -28,6 +28,8 @@ from src.config import Config from src.speaker_suggestions import ( MULTI_SPEAKER_KEY, + REVIEW_STATE_GENERIC, + REVIEW_STATE_KEY, extract_sample_text, ClusterContext, clusters_from_sidecar_channel, @@ -37,6 +39,7 @@ read_speakers_sidecar, sample_segments, set_cluster_multi_speaker, + set_cluster_review_state, suggest_speaker, suggest_speakers_for_meeting, write_speakers_sidecar, @@ -1725,3 +1728,152 @@ def test_a_clip_exactly_at_the_audio_cap_counts_as_fitting(self): target_ids={("system", "SPEAKER_0")}, ) self.assertAlmostEqual(samples[0]["start"], 10.0, places=1) + + +class SetClusterReviewStateCliTests(unittest.TestCase): + """The CLI behind "Keep generic". It records that a human looked at a + row and chose to leave it unnamed -- the one review outcome that + produces no other trace, and therefore the one that a restart silently + undoes if it is not written down.""" + + def _run(self, command, args, tmp, cfg=None): + cfg = cfg or Config(config_path=Path(tmp) / "config.json") + with mock.patch("src.config.get_config", return_value=cfg), \ + mock.patch.dict("os.environ", {"STENOAI_USER_DATA_DIR": tmp}): + return CliRunner().invoke(command, args) + + def _seed(self, tmp, clusters=None): + output_dir = Path(tmp) / "output" + output_dir.mkdir(parents=True, exist_ok=True) + write_speakers_sidecar(output_dir, "mtg001", { + "system": { + "recording_type": "remote", + "clusters": clusters or { + "SPEAKER_0": {"embedding": [1.0, 0.0], "speech_duration_seconds": 60.0, + "segment_count": 10, "segments": [{"start": 1.0, "end": 5.0}]}, + "SPEAKER_1": {"embedding": [0.0, 1.0], "speech_duration_seconds": 40.0, + "segment_count": 8, "segments": [{"start": 20.0, "end": 24.0}]}, + }, + }, + }) + return output_dir + + def _stored(self, output_dir, sid): + return read_speakers_sidecar(output_dir, "mtg001")["channels"]["system"]["clusters"][sid] + + def test_marking_generic_round_trips_and_clears(self): + with tempfile.TemporaryDirectory() as tmp: + output_dir = self._seed(tmp) + result = self._run(simple_recorder.set_cluster_review_state_command, + ["mtg001", "system", "SPEAKER_0"], tmp) + data = _last_json(result.output) + self.assertTrue(data["success"]) + self.assertEqual(data["review_state"], REVIEW_STATE_GENERIC) + self.assertEqual(self._stored(output_dir, "SPEAKER_0")[REVIEW_STATE_KEY], + REVIEW_STATE_GENERIC) + + result = self._run(simple_recorder.set_cluster_review_state_command, + ["mtg001", "system", "SPEAKER_0", "--clear"], tmp) + data = _last_json(result.output) + self.assertTrue(data["success"]) + self.assertIsNone(data["review_state"]) + self.assertNotIn(REVIEW_STATE_KEY, self._stored(output_dir, "SPEAKER_0")) + + def test_reports_the_merged_reach_not_just_the_id_it_was_handed(self): + # The reviewer clicked one row; that row may be several raw + # clusters. Saying which ones it covers keeps the CLI honest about + # what just happened, the same way mark-speaker-cluster does. + with tempfile.TemporaryDirectory() as tmp: + self._seed(tmp, clusters={ + "SPEAKER_0": {"embedding": [1.0, 0.0], "speech_duration_seconds": 1600.0, + "segment_count": 580}, + "SPEAKER_2": {"embedding": [0.995, 0.0999], "speech_duration_seconds": 1538.0, + "segment_count": 552}, + }) + result = self._run(simple_recorder.set_cluster_review_state_command, + ["mtg001", "system", "SPEAKER_2"], tmp) + data = _last_json(result.output) + self.assertEqual(data["diarization_speaker_id"], "SPEAKER_2") + self.assertEqual(data["resolved_diarization_speaker_id"], "SPEAKER_0") + self.assertEqual(sorted(data["fragment_ids"]), ["SPEAKER_0", "SPEAKER_2"]) + + def test_a_missing_sidecar_fails_as_json_and_never_as_a_traceback(self): + with tempfile.TemporaryDirectory() as tmp: + (Path(tmp) / "output").mkdir(parents=True, exist_ok=True) + result = self._run(simple_recorder.set_cluster_review_state_command, + ["never-diarised", "system", "SPEAKER_0"], tmp) + self.assertEqual(result.exit_code, 1) + self.assertFalse(_last_json(result.output)["success"]) + self.assertNotIn("Traceback", result.output) + + def test_a_missing_channel_fails_as_json(self): + with tempfile.TemporaryDirectory() as tmp: + self._seed(tmp) + result = self._run(simple_recorder.set_cluster_review_state_command, + ["mtg001", "mic", "SPEAKER_0"], tmp) + self.assertEqual(result.exit_code, 1) + self.assertFalse(_last_json(result.output)["success"]) + self.assertNotIn("Traceback", result.output) + + def test_a_missing_cluster_fails_as_json(self): + with tempfile.TemporaryDirectory() as tmp: + self._seed(tmp) + result = self._run(simple_recorder.set_cluster_review_state_command, + ["mtg001", "system", "SPEAKER_99"], tmp) + self.assertEqual(result.exit_code, 1) + self.assertFalse(_last_json(result.output)["success"]) + self.assertNotIn("Traceback", result.output) + + def test_confirming_a_cluster_clears_the_marking_from_every_fragment(self): + # "Generic" means a human chose to stop there. Naming the row is a + # stronger statement about the same cluster and supersedes it -- and + # a key left on a fragment would keep the merged row reading generic + # after the confirm, because the merged view is an any(). + with tempfile.TemporaryDirectory() as tmp: + output_dir = self._seed(tmp, clusters={ + "SPEAKER_0": {"embedding": [1.0, 0.0], "speech_duration_seconds": 1600.0, + "segment_count": 580}, + "SPEAKER_2": {"embedding": [0.995, 0.0999], "speech_duration_seconds": 1538.0, + "segment_count": 552}, + }) + cfg = Config(config_path=Path(tmp) / "config.json") + for sid in ("SPEAKER_0", "SPEAKER_2"): + set_cluster_review_state(output_dir, "mtg001", "system", sid, REVIEW_STATE_GENERIC) + + result = self._run(simple_recorder.confirm_speaker, + ["mtg001", "system", "SPEAKER_2", "--new-person", "Julian"], + tmp, cfg=cfg) + self.assertTrue(_last_json(result.output)["success"]) + for sid in ("SPEAKER_0", "SPEAKER_2"): + self.assertNotIn(REVIEW_STATE_KEY, self._stored(output_dir, sid)) + + def test_marking_a_cluster_as_mixed_clears_it_from_every_fragment(self): + with tempfile.TemporaryDirectory() as tmp: + output_dir = self._seed(tmp, clusters={ + "SPEAKER_0": {"embedding": [1.0, 0.0], "speech_duration_seconds": 1600.0, + "segment_count": 580}, + "SPEAKER_2": {"embedding": [0.995, 0.0999], "speech_duration_seconds": 1538.0, + "segment_count": 552}, + }) + for sid in ("SPEAKER_0", "SPEAKER_2"): + set_cluster_review_state(output_dir, "mtg001", "system", sid, REVIEW_STATE_GENERIC) + + result = self._run(simple_recorder.mark_speaker_cluster, + ["mtg001", "system", "SPEAKER_0"], tmp) + self.assertTrue(_last_json(result.output)["success"]) + for sid in ("SPEAKER_0", "SPEAKER_2"): + self.assertNotIn(REVIEW_STATE_KEY, self._stored(output_dir, sid)) + + def test_clearing_a_mixed_marking_does_not_resurrect_a_review_marking(self): + # --single withdraws the "two people" statement; it says nothing + # about the reviewer having once kept the row generic, and inventing + # that back would put a mark on the row nobody set. + with tempfile.TemporaryDirectory() as tmp: + output_dir = self._seed(tmp) + set_cluster_review_state(output_dir, "mtg001", "system", "SPEAKER_0", + REVIEW_STATE_GENERIC) + self._run(simple_recorder.mark_speaker_cluster, + ["mtg001", "system", "SPEAKER_0"], tmp) + self._run(simple_recorder.mark_speaker_cluster, + ["mtg001", "system", "SPEAKER_0", "--single"], tmp) + self.assertNotIn(REVIEW_STATE_KEY, self._stored(output_dir, "SPEAKER_0")) diff --git a/tests/test_speaker_suggestions.py b/tests/test_speaker_suggestions.py index 4dcf1863..62d1be4d 100644 --- a/tests/test_speaker_suggestions.py +++ b/tests/test_speaker_suggestions.py @@ -6,6 +6,8 @@ from src.speaker_suggestions import ( ClusterContext, + REVIEW_STATE_GENERIC, + REVIEW_STATE_KEY, SAME_MEETING_MERGE_DISTANCE_THRESHOLD, SUGGESTION_CONFIDENCE_MARGIN, SUGGESTION_DISTANCE_THRESHOLD, @@ -14,6 +16,7 @@ SUGGESTION_MIN_DURATION_SECONDS, SUGGESTION_MIN_SEGMENT_COUNT, build_clusters_from_diarization, + clear_cluster_review_state, clusters_from_sidecar_channel, confirmed_participant_names, determine_recording_type, @@ -29,6 +32,7 @@ relabel_transcript_speaker, score_candidates, set_cluster_multi_speaker, + set_cluster_review_state, suggest_speaker, suggest_speakers_for_meeting, write_speakers_sidecar, @@ -1368,5 +1372,134 @@ def test_returns_false_on_timeout(self): self.assertFalse(ok) +class ReviewStateTests(unittest.TestCase): + """"Keep generic" as a persisted fact rather than a React state set. + + The button exists for the row a reviewer looked at and decided to leave + alone. Held only in the component, that decision dies on a remount -- + navigating away and back re-presents every row they already dealt with, + which is exactly the work the button was meant to save. Persisting it + also makes it survivable in the other direction: a half-finished review + can be picked up tomorrow. + """ + + def _seed(self, tmp, clusters=None): + output_dir = Path(tmp) / "output" + output_dir.mkdir(parents=True, exist_ok=True) + write_speakers_sidecar(output_dir, "mtg001", { + "system": { + "recording_type": "remote", + "clusters": clusters or { + "SPEAKER_0": {"embedding": [1.0, 0.0], "speech_duration_seconds": 60.0, + "segment_count": 10}, + "SPEAKER_1": {"embedding": [0.0, 1.0], "speech_duration_seconds": 40.0, + "segment_count": 8}, + }, + }, + }) + return output_dir + + def _stored(self, output_dir, sid): + sidecar = read_speakers_sidecar(output_dir, "mtg001") + return sidecar["channels"]["system"]["clusters"][sid] + + def test_marking_writes_the_key_on_the_exact_cluster_it_was_handed(self): + with tempfile.TemporaryDirectory() as tmp: + output_dir = self._seed(tmp) + result = set_cluster_review_state( + output_dir, "mtg001", "system", "SPEAKER_0", REVIEW_STATE_GENERIC, + ) + self.assertIsNotNone(result) + self.assertEqual(self._stored(output_dir, "SPEAKER_0")[REVIEW_STATE_KEY], + REVIEW_STATE_GENERIC) + self.assertNotIn(REVIEW_STATE_KEY, self._stored(output_dir, "SPEAKER_1")) + + def test_clearing_removes_the_key_rather_than_storing_a_null(self): + # Absent means "not marked" everywhere in this sidecar, so a stored + # null would be a third state no reader knows about. + with tempfile.TemporaryDirectory() as tmp: + output_dir = self._seed(tmp) + set_cluster_review_state(output_dir, "mtg001", "system", "SPEAKER_0", + REVIEW_STATE_GENERIC) + set_cluster_review_state(output_dir, "mtg001", "system", "SPEAKER_0", None) + self.assertNotIn(REVIEW_STATE_KEY, self._stored(output_dir, "SPEAKER_0")) + + def test_a_missing_sidecar_channel_or_cluster_returns_none(self): + with tempfile.TemporaryDirectory() as tmp: + output_dir = self._seed(tmp) + self.assertIsNone(set_cluster_review_state( + output_dir, "never-diarised", "system", "SPEAKER_0", REVIEW_STATE_GENERIC)) + self.assertIsNone(set_cluster_review_state( + output_dir, "mtg001", "mic", "SPEAKER_0", REVIEW_STATE_GENERIC)) + self.assertIsNone(set_cluster_review_state( + output_dir, "mtg001", "system", "SPEAKER_99", REVIEW_STATE_GENERIC)) + + def test_a_rewrite_preserves_the_marking(self): + # set_cluster_multi_speaker rewrites the whole document; the two + # markings are independent facts about the same cluster and neither + # may drop the other. + with tempfile.TemporaryDirectory() as tmp: + output_dir = self._seed(tmp) + set_cluster_review_state(output_dir, "mtg001", "system", "SPEAKER_0", + REVIEW_STATE_GENERIC) + set_cluster_multi_speaker(output_dir, "mtg001", "system", "SPEAKER_1", True) + self.assertEqual(self._stored(output_dir, "SPEAKER_0")[REVIEW_STATE_KEY], + REVIEW_STATE_GENERIC) + + def test_a_merged_row_reads_generic_when_any_fragment_carries_it(self): + # Mirrors how contains_multiple_speakers merges: the marking is + # written on a raw id, the panel shows the merged row, and the + # reviewer's decision was about the row they saw. + with tempfile.TemporaryDirectory() as tmp: + output_dir = self._seed(tmp, clusters={ + "SPEAKER_0": {"embedding": [1.0, 0.0], "speech_duration_seconds": 1600.0, + "segment_count": 580}, + "SPEAKER_2": {"embedding": [0.995, 0.0999], "speech_duration_seconds": 1538.0, + "segment_count": 552}, + }) + # The NON-primary fragment: SPEAKER_0 wins the merge on duration. + set_cluster_review_state(output_dir, "mtg001", "system", "SPEAKER_2", + REVIEW_STATE_GENERIC) + sidecar = read_speakers_sidecar(output_dir, "mtg001") + merged, _ = merge_same_channel_fragments( + clusters_from_sidecar_channel("mtg001", sidecar["channels"]["system"]) + ) + self.assertEqual(merged["SPEAKER_0"][1].merged_from, ["SPEAKER_2"]) + self.assertEqual(merged["SPEAKER_0"][1].review_state, REVIEW_STATE_GENERIC) + + def test_a_legacy_cluster_without_the_key_reads_as_unmarked(self): + with tempfile.TemporaryDirectory() as tmp: + output_dir = self._seed(tmp) + sidecar = read_speakers_sidecar(output_dir, "mtg001") + clusters = clusters_from_sidecar_channel("mtg001", sidecar["channels"]["system"]) + self.assertIsNone(clusters["SPEAKER_0"][1].review_state) + + def test_clearing_sweeps_every_fragment_of_a_merged_row(self): + # A key left on a non-primary fragment would keep the merged row + # reading generic after a confirm, because the merged view is an + # any() over the members. + with tempfile.TemporaryDirectory() as tmp: + output_dir = self._seed(tmp) + for sid in ("SPEAKER_0", "SPEAKER_1"): + set_cluster_review_state(output_dir, "mtg001", "system", sid, + REVIEW_STATE_GENERIC) + cleared = clear_cluster_review_state( + output_dir, "mtg001", "system", {"SPEAKER_0", "SPEAKER_1"}, + ) + self.assertEqual(cleared, 2) + for sid in ("SPEAKER_0", "SPEAKER_1"): + self.assertNotIn(REVIEW_STATE_KEY, self._stored(output_dir, sid)) + + def test_clearing_what_was_never_marked_reports_nothing_and_raises_nothing(self): + with tempfile.TemporaryDirectory() as tmp: + output_dir = self._seed(tmp) + self.assertEqual( + clear_cluster_review_state(output_dir, "mtg001", "system", {"SPEAKER_0"}), 0) + self.assertEqual( + clear_cluster_review_state(output_dir, "gone", "system", {"SPEAKER_0"}), 0) + self.assertEqual( + clear_cluster_review_state(output_dir, "mtg001", "mic", {"SPEAKER_0"}), 0) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_suggest_speakers_cli.py b/tests/test_suggest_speakers_cli.py index 5dede7c3..af5b30f3 100644 --- a/tests/test_suggest_speakers_cli.py +++ b/tests/test_suggest_speakers_cli.py @@ -9,7 +9,9 @@ import simple_recorder from src.config import Config from src.speaker_suggestions import ( + REVIEW_STATE_GENERIC, read_speakers_sidecar, + set_cluster_review_state, write_sidecar_document, write_speakers_sidecar, ) @@ -504,6 +506,61 @@ def fake_extract(audio_path, channel, segments, output_path, segment_index=None) self.assertEqual(len(captured["segments"]), 2) +class SuggestSpeakersReviewStateTests(unittest.TestCase): + """The panel reads the "kept generic" marking from here rather than + holding it in component state, which is what makes it survive a remount + and a restart by construction.""" + + def _run(self, args, tmp, cfg=None): + cfg = cfg or Config(config_path=Path(tmp) / "config.json") + with mock.patch("src.config.get_config", return_value=cfg), \ + mock.patch.dict("os.environ", {"STENOAI_USER_DATA_DIR": tmp}): + return CliRunner().invoke(simple_recorder.suggest_speakers, args) + + def _seed(self, tmp, clusters=None): + output_dir = Path(tmp) / "output" + output_dir.mkdir(parents=True, exist_ok=True) + write_speakers_sidecar(output_dir, "mtg001", { + "system": { + "recording_type": "remote", + "clusters": clusters or { + "SPEAKER_0": {"embedding": [1.0, 0.0], "speech_duration_seconds": 60.0, + "segment_count": 10}, + "SPEAKER_1": {"embedding": [0.0, 1.0], "speech_duration_seconds": 40.0, + "segment_count": 8}, + }, + }, + }) + return output_dir + + def test_review_state_is_echoed_per_cluster(self): + with tempfile.TemporaryDirectory() as tmp: + output_dir = self._seed(tmp) + set_cluster_review_state(output_dir, "mtg001", "system", "SPEAKER_0", + REVIEW_STATE_GENERIC) + data = _last_json(self._run(["mtg001"], tmp).output) + self.assertEqual( + data["channels"]["system"]["SPEAKER_0"]["review_state"], REVIEW_STATE_GENERIC) + self.assertIsNone(data["channels"]["system"]["SPEAKER_1"]["review_state"]) + + def test_a_marking_on_a_fragment_marks_the_row_it_was_made_on(self): + # The reviewer clicked one row; the sidecar records raw clusters. + # Reading the marking back on the row they saw is the whole point. + with tempfile.TemporaryDirectory() as tmp: + output_dir = self._seed(tmp, clusters={ + "SPEAKER_0": {"embedding": [1.0, 0.0], "speech_duration_seconds": 1600.0, + "segment_count": 580}, + "SPEAKER_2": {"embedding": [0.995, 0.0999], "speech_duration_seconds": 1538.0, + "segment_count": 552}, + }) + set_cluster_review_state(output_dir, "mtg001", "system", "SPEAKER_2", + REVIEW_STATE_GENERIC) + data = _last_json(self._run(["mtg001"], tmp).output) + row = data["channels"]["system"]["SPEAKER_0"] + self.assertEqual(row["merged_from"], ["SPEAKER_2"]) + self.assertEqual(row["review_state"], REVIEW_STATE_GENERIC) + + class SuggestSpeakersRunScopeTests(unittest.TestCase): """What the panel may still call "confirmed" after the meeting was diarized a second time. From 702a8dcaaf307bd0aab20c5a202c446cc2681917 Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 4 Aug 2026 21:58:17 +0200 Subject: [PATCH 13/23] fix(speakers): answer a structurally wrong sidecar with JSON, not a traceback The never-raises contract covered missing things but not wrong-typed ones. This file is JSON on a user's disk, so a half-written copy, a restored backup or a hand-edit can leave `channels`, `clusters` or a cluster entry holding the wrong type - and `.get` on a list raises. The CLI is invoked by Electron, which parses the last JSON line of stdout, so a traceback surfaces as "something went wrong" with the actual state unreported. The checks live in the shared read helper, so mark-speaker-cluster gets them too. Its exposure was the same and predates this slice; found while hardening the new command, not by testing it separately. Both commands also guard the merge that computes a marking's reach. It runs after the write has already landed and fails on any OTHER cluster in the channel with no usable embedding - raising there would report a failure for an action that succeeded. The two new write helpers now re-read immediately before writing, like set_cluster_multi_speaker: the review-state key rides in the same document as the voice embeddings, so writing back a copy that went stale would take a concurrent marking with it. Removing that second read from set_cluster_multi_speaker as "redundant" is what showed it is not - the test that pins it caught it. AttributeError is deliberately not in either catch tuple: with the type checks upstream it is unreachable, and a caught exception no test can provoke is noise that reads as protection. --- simple_recorder.py | 21 +++++-- src/speaker_suggestions.py | 90 +++++++++++++++++++---------- tests/test_speaker_multi_marking.py | 32 ++++++++++ 3 files changed, 106 insertions(+), 37 deletions(-) diff --git a/simple_recorder.py b/simple_recorder.py index f2c95c76..9872cb12 100644 --- a/simple_recorder.py +++ b/simple_recorder.py @@ -5334,8 +5334,12 @@ def set_cluster_review_state_command(meeting_stem, channel, diarization_speaker_ clusters_from_sidecar_channel(meeting_stem, channel_data) ) except (KeyError, TypeError, ValueError): - # A sidecar whose embeddings cannot be merged still took the mark; - # report the raw id rather than failing an action that succeeded. + # The mark is already on disk by now; only the merge that computes + # its reach can still fail, and it does whenever any OTHER cluster + # in this channel lacks a usable embedding. Report the raw id + # rather than failing an action that already succeeded. + # (A structurally wrong channels/clusters map cannot get this far -- + # the write refuses it first; see _freshest_channel.) clusters, id_resolution = {}, {} resolved_id = id_resolution.get(diarization_speaker_id, diarization_speaker_id) if resolved_id in clusters: @@ -5416,9 +5420,16 @@ def mark_speaker_cluster(meeting_stem, channel, diarization_speaker_id, multiple # carries the existing run forward -- marking a cluster is an annotation # of this diarization output, not a new one. `None` on a legacy sidecar. run_id = (sidecar.get("diarization_run") or {}).get("run_id") - clusters, id_resolution = merge_same_channel_fragments( - clusters_from_sidecar_channel(meeting_stem, channel_data) - ) + try: + clusters, id_resolution = merge_same_channel_fragments( + clusters_from_sidecar_channel(meeting_stem, channel_data) + ) + except (KeyError, TypeError, ValueError): + # Same as set-cluster-review-state: the marking already landed, and + # only the reach computation can still fail -- on any OTHER cluster + # in this channel with no usable embedding. A traceback here would + # claim a marking failed that did not. + clusters, id_resolution = {}, {} resolved_id = id_resolution.get(diarization_speaker_id, diarization_speaker_id) fragment_ids = set() if resolved_id in clusters: diff --git a/src/speaker_suggestions.py b/src/speaker_suggestions.py index 341d2465..10430990 100644 --- a/src/speaker_suggestions.py +++ b/src/speaker_suggestions.py @@ -722,34 +722,20 @@ def set_cluster_multi_speaker( doesn't exist -- callers report that as an error rather than silently marking nothing. """ - sidecar = read_speakers_sidecar(output_dir, meeting_stem) + sidecar, channel_data = _freshest_channel(output_dir, meeting_stem, channel) if sidecar is None: return None - channel_data = (sidecar.get("channels") or {}).get(channel) - if channel_data is None: - return None - cluster = (channel_data.get("clusters") or {}).get(diarization_speaker_id) - if cluster is None: + cluster = _cluster_entries(channel_data).get(diarization_speaker_id) + if not isinstance(cluster, dict): return None - # Re-read immediately before writing and apply this ONE change to the - # freshest copy, rather than writing back the whole document as it - # looked on entry. The write is atomic, the read-modify-write was not: - # two overlapping marks both started from the same sidecar, and whoever - # replaced the file second discarded the other's marking silently -- - # along with any confirmation cleanup its caller had already performed - # against it. This narrows the window to re-read-until-rename instead of - # entry-until-rename; it does not make the sequence a transaction, and a - # mark landing inside that remaining window would still be lost. Closing - # it fully needs a lock that works on macOS and Windows alike, which is - # a bigger change than this defect warrants. - freshest = read_speakers_sidecar(output_dir, meeting_stem) - if freshest is not None: - fresh_cluster = ( - ((freshest.get("channels") or {}).get(channel) or {}).get("clusters") or {} - ).get(diarization_speaker_id) - if fresh_cluster is not None: - sidecar = freshest + # The second read, as late as possible before the write -- see + # _freshest_channel on the window this closes and the one it leaves. + fresh_sidecar, fresh_channel = _freshest_channel(output_dir, meeting_stem, channel) + if fresh_sidecar is not None: + fresh_cluster = _cluster_entries(fresh_channel).get(diarization_speaker_id) + if isinstance(fresh_cluster, dict): + sidecar = fresh_sidecar cluster = fresh_cluster if marked: @@ -765,19 +751,45 @@ def _freshest_channel(output_dir: Path, meeting_stem: str, channel: str) -> tupl """The sidecar as it is on disk RIGHT NOW plus its `channel` entry, for a read-modify-write that must not overwrite a concurrent one. - Same reasoning as in set_cluster_multi_speaker, and the same limit: it - narrows the race to re-read-until-rename rather than eliminating it. + Callers use it twice: once to validate, and once again immediately + before writing, applying their change to whatever that second read + returned. The write is atomic; the read-modify-write around it is not. + Two overlapping marks that both started from the same in-memory sidecar + ended with the second writer discarding the first's marking silently, + along with any confirmation cleanup its caller had already performed + against it. The second read narrows that window to read-until-rename + instead of entry-until-rename. It does not make the sequence a + transaction, and a write landing inside the remaining window is still + lost; closing it fully needs a lock that works on macOS and Windows + alike, which is a bigger change than this defect warrants. + Returns `(None, None)` when the sidecar or the channel is gone. + + Types are checked rather than assumed. This file is JSON on a user's + disk: a half-written copy, a restored backup or a hand-edit can leave + any of these keys holding the wrong type, and `.get` on a list raises. + Every caller owes its own caller a JSON error rather than a traceback, + so a structurally wrong document has to read the same as a missing one. """ sidecar = read_speakers_sidecar(output_dir, meeting_stem) - if sidecar is None: + if not isinstance(sidecar, dict): return None, None - channel_data = (sidecar.get("channels") or {}).get(channel) - if channel_data is None: + channels = sidecar.get("channels") + if not isinstance(channels, dict): + return None, None + channel_data = channels.get(channel) + if not isinstance(channel_data, dict): return None, None return sidecar, channel_data +def _cluster_entries(channel_data: dict) -> dict: + """One channel's `clusters` map, or an empty one when it is missing or + the wrong type (see _freshest_channel on why that is not assumed).""" + clusters = channel_data.get("clusters") + return clusters if isinstance(clusters, dict) else {} + + def set_cluster_review_state( output_dir: Path, meeting_stem: str, channel: str, diarization_speaker_id: str, state: Optional[str], @@ -797,10 +809,20 @@ def set_cluster_review_state( sidecar, channel_data = _freshest_channel(output_dir, meeting_stem, channel) if sidecar is None: return None - cluster = (channel_data.get("clusters") or {}).get(diarization_speaker_id) - if cluster is None: + cluster = _cluster_entries(channel_data).get(diarization_speaker_id) + if not isinstance(cluster, dict): return None + # The second read, same as set_cluster_multi_speaker: this key rides in + # the same document as the voice embeddings, so writing back a copy + # that went stale would take a concurrent marking with it. + fresh_sidecar, fresh_channel = _freshest_channel(output_dir, meeting_stem, channel) + if fresh_sidecar is not None: + fresh_cluster = _cluster_entries(fresh_channel).get(diarization_speaker_id) + if isinstance(fresh_cluster, dict): + sidecar = fresh_sidecar + cluster = fresh_cluster + if state is None: cluster.pop(REVIEW_STATE_KEY, None) else: @@ -828,7 +850,11 @@ def clear_cluster_review_state( sidecar, channel_data = _freshest_channel(output_dir, meeting_stem, channel) if sidecar is None: return 0 - clusters = channel_data.get("clusters") or {} + # The second read, same as the two setters above. + fresh_sidecar, fresh_channel = _freshest_channel(output_dir, meeting_stem, channel) + if fresh_sidecar is not None: + sidecar, channel_data = fresh_sidecar, fresh_channel + clusters = _cluster_entries(channel_data) cleared = 0 for sid in diarization_speaker_ids: cluster = clusters.get(sid) diff --git a/tests/test_speaker_multi_marking.py b/tests/test_speaker_multi_marking.py index 721ef66f..17cdb0b4 100644 --- a/tests/test_speaker_multi_marking.py +++ b/tests/test_speaker_multi_marking.py @@ -1877,3 +1877,35 @@ def test_clearing_a_mixed_marking_does_not_resurrect_a_review_marking(self): self._run(simple_recorder.mark_speaker_cluster, ["mtg001", "system", "SPEAKER_0", "--single"], tmp) self.assertNotIn(REVIEW_STATE_KEY, self._stored(output_dir, "SPEAKER_0")) + + def _write_raw(self, tmp, payload): + output_dir = Path(tmp) / "output" + output_dir.mkdir(parents=True, exist_ok=True) + (output_dir / "mtg001_speakers.json").write_text(json.dumps(payload)) + return output_dir + + def test_a_structurally_wrong_sidecar_fails_as_json_not_as_a_traceback(self): + # The never-raises contract is not only about missing things. This + # file is JSON on a user's disk: a half-written copy, a botched + # restore or a hand-edit can leave any of these keys holding the + # wrong type, and Electron parses the last JSON line of stdout -- + # a traceback reaches the UI as "something went wrong", with the + # actual state unreported. + broken = [ + {"channels": ["not-a-dict"]}, + {"channels": {"system": ["not-a-dict"]}}, + {"channels": {"system": {"clusters": ["not-a-dict"]}}}, + {"channels": {"system": {"clusters": {"SPEAKER_0": "not-a-dict"}}}}, + # Reaches the merge, which needs an embedding per cluster. + {"channels": {"system": {"clusters": {"SPEAKER_0": {}}}}}, + ] + for payload in broken: + with self.subTest(payload=payload), tempfile.TemporaryDirectory() as tmp: + self._write_raw(tmp, payload) + for command in (simple_recorder.set_cluster_review_state_command, + simple_recorder.mark_speaker_cluster): + result = self._run(command, ["mtg001", "system", "SPEAKER_0"], tmp) + self.assertNotIn("Traceback", result.output) + self.assertIn(result.exit_code, (0, 1)) + if result.exit_code == 1: + self.assertFalse(_last_json(result.output)["success"]) From 66b6c59f64958b1ac49ea3ca154069aaf88e4e60 Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 4 Aug 2026 21:58:37 +0200 Subject: [PATCH 14/23] docs(speakers): tick off task 6 in the plan --- .../2026-08-04-speaker-review-run-provenance.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md b/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md index c3fc4665..96e4e87d 100644 --- a/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md +++ b/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md @@ -143,12 +143,12 @@ - Consumes: the sidecar document helpers. - Produces: per-cluster optional key `review_state` with the single value `"generic"`; CLI `set-cluster-review-state --generic|--clear`; `suggest-speakers` echoes `review_state` per cluster so the renderer can read it. -- [ ] **Step 1: Write the failing tests.** The helper sets and clears the key on the exact raw id it was handed. A merged row reads as generic when **any** raw member carries it, mirroring how `merge_same_channel_fragments` already computes `contains_multiple_speakers` with `any()`. `confirm-speaker` clears it from **every fragment id** of the confirmed cluster, and `mark-speaker-cluster --multiple` does the same - so no orphaned key on a non-primary fragment can keep a row generic after a confirm. The CLI's never-raises contract: missing sidecar, missing channel, and missing cluster each produce `success: false` JSON and exit 1, with no traceback. -- [ ] **Step 2: Run and watch them fail.** -- [ ] **Step 3: Implement the helper and the CLI.** The write helper mirrors `set_cluster_multi_speaker`: re-read the freshest sidecar immediately before writing, apply the one change, replace atomically via `write_sidecar_document`. The CLI mirrors `mark-speaker-cluster`'s argument shape and reports the merged reach (resolved id plus fragment set) in its JSON. -- [ ] **Step 4: Wire the transitions and the echo.** Clear on confirm and on mark; echo `review_state` per cluster from `suggest-speakers`. -- [ ] **Step 5: Run green, then the whole suite.** -- [ ] **Step 6: Commit.** +- [x] **Step 1: Write the failing tests.** The helper sets and clears the key on the exact raw id it was handed. A merged row reads as generic when **any** raw member carries it, mirroring how `merge_same_channel_fragments` already computes `contains_multiple_speakers` with `any()`. `confirm-speaker` clears it from **every fragment id** of the confirmed cluster, and `mark-speaker-cluster --multiple` does the same - so no orphaned key on a non-primary fragment can keep a row generic after a confirm. The CLI's never-raises contract: missing sidecar, missing channel, and missing cluster each produce `success: false` JSON and exit 1, with no traceback. +- [x] **Step 2: Run and watch them fail.** +- [x] **Step 3: Implement the helper and the CLI.** The write helper mirrors `set_cluster_multi_speaker`: re-read the freshest sidecar immediately before writing, apply the one change, replace atomically via `write_sidecar_document`. The CLI mirrors `mark-speaker-cluster`'s argument shape and reports the merged reach (resolved id plus fragment set) in its JSON. +- [x] **Step 4: Wire the transitions and the echo.** Clear on confirm and on mark; echo `review_state` per cluster from `suggest-speakers`. +- [x] **Step 5: Run green, then the whole suite.** +- [x] **Step 6: Commit.** --- From 6aff5b6541fdc7aa590e9d84966410c5ff6bebb9 Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 4 Aug 2026 22:14:32 +0200 Subject: [PATCH 15/23] feat(speakers): show the review's own progress in the panel "Keep generic" now calls the backend instead of adding a key to a React state set. The marker comes back with the suggestions query, so it survives a remount and a restart by construction rather than by remembering to keep a second copy in sync. The row stays visible and reads as kept generic, where it used to disappear. Hiding it was fine while the decision lived for one session; persisted, a hidden row puts its own undo somewhere nobody can reach, and a reviewer returning tomorrow could not tell "I decided to leave this" from "this never appeared". The button also stops rendering on confirmed and mixed rows - it sat outside the conditional that hides the naming actions, so it invited a click that would say two contradictory things about one cluster. Adds the meeting-level notice for confirmations a re-diarization orphaned, from the stale_assignments the read path already reports. It says the assignments are gone, not the people: their voice evidence is untouched and still scores candidates everywhere. The T1 test that pinned the old behaviour is rewritten and renamed, not edited: 'Keep generic dismisses the row locally' asserted the row reached toHaveCount(0), which this slice deliberately makes false. Quietly flipping that assertion would have disguised a product decision as test maintenance. The derivations are exported and tested without mounting, and the mock IPC carries the same transitions as the real CLI (a confirm and a mixed marking both clear the marker) so the two cannot drift. --- app/e2e-mock-ipc.js | 27 ++++ app/main.js | 15 ++ app/preload.js | 1 + .../src/components/SpeakerReviewPanel.tsx | 134 +++++++++++++++--- .../src/components/speakerReviewState.test.ts | 91 ++++++++++++ .../src/hooks/useSpeakerSuggestions.ts | 22 +++ app/renderer/src/lib/ipc.ts | 39 +++++ e2e/specs/speaker-review.t1.spec.ts | 66 ++++++++- 8 files changed, 373 insertions(+), 22 deletions(-) create mode 100644 app/renderer/src/components/speakerReviewState.test.ts diff --git a/app/e2e-mock-ipc.js b/app/e2e-mock-ipc.js index f87ded87..d7ffb82f 100644 --- a/app/e2e-mock-ipc.js +++ b/app/e2e-mock-ipc.js @@ -883,6 +883,9 @@ function install({ ipcMain }) { cluster.confirmed_by_user = null; cluster.confirmed_person_id = null; } + // "A human kept this generic" is superseded by "a human says it is + // several people" -- same transition the real CLI performs. + cluster.review_state = null; } else if (cluster.prevSuggestion) { Object.assign(cluster, cluster.prevSuggestion); delete cluster.prevSuggestion; @@ -896,6 +899,27 @@ function install({ ipcMain }) { }; }, + // Mutates the same speakerState the suggestions echo is built from, so + // a spec sees the marking the way the real backend delivers it: written + // to the sidecar, read back on the next suggest-speakers. The real CLI + // also clears it on a confirm and on a mixed marking (both are stronger + // statements about the same cluster); those transitions live in the two + // handlers above so the mock cannot drift from that contract. + 'set-cluster-review-state': async (_event, params) => { + const { channel, diarizationSpeakerId, generic } = params || {}; + const cluster = (speakerState.suggestions[channel] || {})[diarizationSpeakerId]; + if (!cluster) { + return { success: false, error: `No cluster ${diarizationSpeakerId} in ${channel}` }; + } + cluster.review_state = generic ? 'generic' : null; + return { + success: true, + resolved_diarization_speaker_id: diarizationSpeakerId, + fragment_ids: [diarizationSpeakerId], + review_state: cluster.review_state, + }; + }, + 'speaker-naming-status': async (_event, meetingStem) => { const clusters = Object.values(speakerState.suggestions).flatMap((c) => Object.values(c)); const countable = clusters.filter((c) => !c.contains_multiple_speakers); @@ -987,6 +1011,9 @@ function install({ ipcMain }) { // already hold a cluster of this meeting by id, never by display // name (a rename can leave two profiles reading alike). confirmed_person_id: person.person_id, + // Naming the row supersedes "a human kept this generic" -- the real + // CLI clears it across the whole fragment set on every confirm. + review_state: null, }; return { diff --git a/app/main.js b/app/main.js index fb465550..758ad0b7 100644 --- a/app/main.js +++ b/app/main.js @@ -8055,6 +8055,21 @@ ipcMain.handle('mark-speaker-cluster', async (_e, params) => { } }); +ipcMain.handle('set-cluster-review-state', async (_e, params) => { + try { + const out = await runPythonScript('simple_recorder.py', [ + 'set-cluster-review-state', + params.meetingStem, + params.channel, + params.diarizationSpeakerId, + params.generic ? '--generic' : '--clear', + ]); + return JSON.parse(out); + } catch (error) { + return parsePythonFailureJson(error); + } +}); + ipcMain.handle('speaker-naming-status', async (_e, meetingStem) => { try { const out = await runPythonScript('simple_recorder.py', ['speaker-naming-status', meetingStem]); diff --git a/app/preload.js b/app/preload.js index 8810ca45..385206a5 100644 --- a/app/preload.js +++ b/app/preload.js @@ -209,6 +209,7 @@ const stenoai = { getSampleAudio: (meetingStem, channel, diarizationSpeakerId, segmentIndex) => invoke('get-speaker-sample-audio', meetingStem, channel, diarizationSpeakerId, segmentIndex), markCluster: (params) => invoke('mark-speaker-cluster', params), + setClusterReviewState: (params) => invoke('set-cluster-review-state', params), namingStatus: (meetingStem) => invoke('speaker-naming-status', meetingStem), }, diff --git a/app/renderer/src/components/SpeakerReviewPanel.tsx b/app/renderer/src/components/SpeakerReviewPanel.tsx index 00a51c55..de0a0215 100644 --- a/app/renderer/src/components/SpeakerReviewPanel.tsx +++ b/app/renderer/src/components/SpeakerReviewPanel.tsx @@ -14,9 +14,10 @@ import { useGetSpeakerSampleAudio, useDeletePersonProfile, useMarkSpeakerCluster, + useSetClusterReviewState, meetingStemFromSummaryFile, } from '@/hooks/useSpeakerSuggestions'; -import type { PersonProfile, SpeakerSuggestion } from '@/lib/ipc'; +import type { PersonProfile, SpeakerSuggestion, StaleAssignment } from '@/lib/ipc'; interface SpeakerReviewPanelProps { summaryFile: string; @@ -84,6 +85,55 @@ export function orderProfilesForRow s.display_name); + const listed = + names.length === 1 + ? names[0] + : `${names.slice(0, -1).join(', ')} and ${names[names.length - 1]}`; + return ( + `This meeting was analysed again, so the speakers were renumbered. ` + + `${listed} ${names.length === 1 ? 'is' : 'are'} no longer linked to any speaker below. ` + + `Nothing was deleted -- assign them again to restore the link.` + ); +} + // "mic" is always the device owner's own recording side (in-person audio); // "system" is loopback capture of the other call participant(s) -- see // determine_recording_type (src/speaker_suggestions.py) for the same @@ -229,8 +279,8 @@ export function SpeakerReviewPanel({ summaryFile, isDiarised }: SpeakerReviewPan const confirmSpeaker = useConfirmSpeaker(); const deleteProfile = useDeletePersonProfile(); const markCluster = useMarkSpeakerCluster(); + const setReviewState = useSetClusterReviewState(); - const [dismissed, setDismissed] = React.useState>(new Set()); const [expanded, setExpanded] = React.useState>(new Set()); const [changeOpenFor, setChangeOpenFor] = React.useState(null); const [newPersonRow, setNewPersonRow] = React.useState(null); @@ -302,19 +352,23 @@ export function SpeakerReviewPanel({ summaryFile, isDiarised }: SpeakerReviewPan const alreadyInMeeting = new Set( rows.map((row) => row.suggestion.confirmed_person_id).filter((id): id is string => !!id), ); - const notDismissed = rows.filter((row) => !dismissed.has(rowKey(row))); // A row a human has explicitly marked stays in the main list even if its // turn shape also matches the artifact heuristic -- hiding it behind // "Show N filtered rows" would bury the undo for a deliberate action - // behind a toggle the user has no reason to open. + // behind a toggle the user has no reason to open. Same for a row kept + // generic: the marking IS a deliberate action, and its undo lives on the + // row itself. const isFiltered = (row: Row) => - row.suggestion.is_likely_artifact && !row.suggestion.contains_multiple_speakers; - const artifactRows = notDismissed.filter(isFiltered); - const primaryRows = notDismissed.filter((row) => !isFiltered(row)); - const visibleRows = showFiltered ? notDismissed : primaryRows; + row.suggestion.is_likely_artifact + && !row.suggestion.contains_multiple_speakers + && !isKeptGeneric(row.suggestion); + const artifactRows = rows.filter(isFiltered); + const primaryRows = rows.filter((row) => !isFiltered(row)); + const visibleRows = showFiltered ? rows : primaryRows; const recordingAvailable = suggestionsQuery.data?.recording_available ?? false; + const staleNotice = staleAssignmentNotice(suggestionsQuery.data?.stale_assignments); - if (!suggestionsQuery.data || notDismissed.length === 0) return null; + if (!suggestionsQuery.data || rows.length === 0) return null; const duplicateProfile = newPersonName.trim() ? (profilesQuery.data ?? []).find((p) => namesCollide(p.display_name, newPersonName)) @@ -383,6 +437,20 @@ export function SpeakerReviewPanel({ summaryFile, isDiarised }: SpeakerReviewPan > Speakers + {/* Above the rows and above the speaker-count note, because it + explains why the rows below look unfamiliar: without it, a + reviewer returning to a re-analysed meeting just finds the names + they entered gone, with nothing saying why or that re-entering + them is all it takes. */} + {staleNotice && ( +

+ {staleNotice} +

+ )} {minimumSpeakers > totalClusters && (

Left out of naming and voice recognition. + ) : isKept ? ( + + Kept generic — you decided not to name this speaker. + ) : ( row.suggestion.sample_text && ( : } {isMarked ? 'One person' : null} - + {/* The row stays put and reads as parked, rather than + vanishing. Hiding it was fine while the decision lived + for one session; now that it is written down, a hidden + row would put its undo somewhere nobody can reach -- + and a reviewer coming back tomorrow would have no way to + tell "I decided to leave this" from "this never + appeared". */} + {showsKeepGenericButton(row.suggestion) && ( + + )} diff --git a/app/renderer/src/components/speakerReviewState.test.ts b/app/renderer/src/components/speakerReviewState.test.ts new file mode 100644 index 00000000..5d640b32 --- /dev/null +++ b/app/renderer/src/components/speakerReviewState.test.ts @@ -0,0 +1,91 @@ +import { describe, expect, it } from 'vitest'; + +import { isKeptGeneric, showsKeepGenericButton, staleAssignmentNotice } from './SpeakerReviewPanel'; + +const suggestion = (over: Record = {}) => + ({ + status: 'none', + suggested_person_id: null, + suggested_name: null, + merged_from: [], + candidates: [], + reasons: [], + speech_duration_seconds: 60, + segment_count: 10, + first_timestamp: '00:12', + sample_text: null, + samples: [], + contains_multiple_speakers: false, + is_likely_artifact: false, + confirmed_by_user: null, + ...over, + }) as never; + +describe('isKeptGeneric', () => { + it('reads the marking out of the query payload, not component state', () => { + // The whole point of persisting it: this derivation has no memory of + // its own, so a remount cannot lose the decision. + expect(isKeptGeneric(suggestion({ review_state: 'generic' }))).toBe(true); + }); + + it('treats an absent marking as not reviewed', () => { + expect(isKeptGeneric(suggestion())).toBe(false); + }); + + it('ignores a value it does not know', () => { + // Forward compatibility in the safe direction: a newer build writing a + // state this one has never heard of must not read as "kept generic", + // because that hides the row's remaining actions behind an undo for a + // decision nobody made here. + expect(isKeptGeneric(suggestion({ review_state: 'something-newer' }))).toBe(false); + }); +}); + +describe('showsKeepGenericButton', () => { + it('offers the button on an ordinary unreviewed row', () => { + expect(showsKeepGenericButton(suggestion())).toBe(true); + }); + + it('hides it once the row is confirmed', () => { + // A named row is decided. Offering "keep generic" there invites a click + // that would say two contradictory things about the same cluster. + expect(showsKeepGenericButton(suggestion({ confirmed_by_user: 'Max' }))).toBe(false); + }); + + it('hides it on a row marked as holding several people', () => { + expect(showsKeepGenericButton(suggestion({ contains_multiple_speakers: true }))).toBe(false); + }); + + it('keeps offering it on a row that is already kept generic', () => { + // That is the undo, and it is the only way back. + expect(showsKeepGenericButton(suggestion({ review_state: 'generic' }))).toBe(true); + }); +}); + +describe('staleAssignmentNotice', () => { + it('says nothing when nothing was orphaned', () => { + expect(staleAssignmentNotice([])).toBeNull(); + expect(staleAssignmentNotice(undefined)).toBeNull(); + }); + + it('names the people whose assignment no longer points anywhere', () => { + const notice = staleAssignmentNotice([ + { person_id: 'p1', display_name: 'Max' }, + { person_id: 'p2', display_name: 'Sarah' }, + ]); + expect(notice).toContain('Max'); + expect(notice).toContain('Sarah'); + }); + + it('does not join a single name with "and", and agrees with the verb', () => { + const one = staleAssignmentNotice([{ person_id: 'p1', display_name: 'Max' }]); + expect(one).toContain('Max is no longer'); + expect(one).not.toContain(' and '); + + const two = staleAssignmentNotice([ + { person_id: 'p1', display_name: 'Max' }, + { person_id: 'p2', display_name: 'Sarah' }, + ]); + expect(two).toContain('Max and Sarah are no longer'); + }); +}); diff --git a/app/renderer/src/hooks/useSpeakerSuggestions.ts b/app/renderer/src/hooks/useSpeakerSuggestions.ts index d64f2295..02a09c45 100644 --- a/app/renderer/src/hooks/useSpeakerSuggestions.ts +++ b/app/renderer/src/hooks/useSpeakerSuggestions.ts @@ -173,6 +173,28 @@ export function useMarkSpeakerCluster() { }); } +/** Recording that a human reviewed a cluster and left it unnamed. + * + * Invalidates only THIS meeting's suggestions, unlike the marking mutation + * next door: keeping a row generic changes nothing about the person + * profiles and nothing about any other meeting -- it is a note about this + * review, so widening the invalidation would refetch every cached meeting + * for no change. */ +export function useSetClusterReviewState() { + const qc = useQueryClient(); + return useMutation({ + mutationFn: async (args: { + meetingStem: string; + channel: string; + diarizationSpeakerId: string; + generic: boolean; + }) => unwrap(await ipc().speakers.setClusterReviewState(args)), + onSuccess: async (_data, args) => { + await qc.invalidateQueries({ queryKey: speakersKeys.suggestions(args.meetingStem) }); + }, + }); +} + /** How many speaker clusters of a meeting are still unnamed -- read once, * right before a delete confirmation, to decide whether to add a sentence * saying the unnamed ones are about to become unnameable forever. diff --git a/app/renderer/src/lib/ipc.ts b/app/renderer/src/lib/ipc.ts index 1c415193..08d96a78 100644 --- a/app/renderer/src/lib/ipc.ts +++ b/app/renderer/src/lib/ipc.ts @@ -501,6 +501,13 @@ export interface SpeakerSuggestion { * WHICH person a cluster went to compares this, not the name. Optional: * a payload predating this field simply carries no id. */ confirmed_person_id?: string | null; + /** How far a human got reviewing this cluster. `"generic"` means they + * looked at it and chose to leave it unnamed -- the one review outcome + * that leaves no other trace, so it is read from here rather than from + * component state and survives a remount by construction. Absent on a + * payload predating the field, and on every unreviewed row. Never + * affects the suggestion: it records progress, not evidence. */ + review_state?: string | null; } export type SuggestSpeakersResponse = Result<{ meeting_id: string; @@ -515,15 +522,43 @@ export type SuggestSpeakersResponse = Result<{ * Nothing consumes this number today -- Sortformer takes no speaker-count * hint, so there is no re-diarization call to feed it into. */ minimum_speaker_count: number; + /** People whose confirmation in this meeting was made against a + * diarization run that no longer describes the clusters below, because + * the meeting was re-diarized since. Their voice evidence is untouched + * and keeps scoring candidates everywhere; what is lost is the link to a + * cluster, and only re-confirming restores it. Absent or empty is the + * normal case, including on every library that was never re-diarized. */ + stale_assignments?: StaleAssignment[]; channels: Record>; }>; +export interface StaleAssignment { + person_id: string; + display_name: string; +} + export interface MarkSpeakerClusterParams { meetingStem: string; channel: string; diarizationSpeakerId: string; containsMultipleSpeakers: boolean; } + +export interface SetClusterReviewStateParams { + meetingStem: string; + channel: string; + diarizationSpeakerId: string; + /** True records "a human reviewed this and left it unnamed"; false + * removes that marking, which is the undo. */ + generic: boolean; +} +export type SetClusterReviewStateResponse = Result<{ + resolved_diarization_speaker_id: string; + /** Every raw cluster id the marked row covers. The panel shows merged + * rows, so one click can reach several ids. */ + fragment_ids: string[]; + review_state: string | null; +}>; export type MarkSpeakerClusterResponse = Result<{ resolved_diarization_speaker_id: string; contains_multiple_speakers: boolean; @@ -1120,6 +1155,10 @@ export interface StenoaiBridge { GetSpeakerSampleAudioResponse >; markCluster: RequestFn<[params: MarkSpeakerClusterParams], MarkSpeakerClusterResponse>; + setClusterReviewState: RequestFn< + [params: SetClusterReviewStateParams], + SetClusterReviewStateResponse + >; namingStatus: RequestFn<[meetingStem: string], SpeakerNamingStatusResponse>; }; diff --git a/e2e/specs/speaker-review.t1.spec.ts b/e2e/specs/speaker-review.t1.spec.ts index b2e34dbe..b364ed79 100644 --- a/e2e/specs/speaker-review.t1.spec.ts +++ b/e2e/specs/speaker-review.t1.spec.ts @@ -140,7 +140,13 @@ test('a person profile can be deleted from the Change popover, unwinding any row await expect(page.getByRole('button', { name: 'Julian', exact: true })).toHaveCount(0); }); -test('Keep generic dismisses the row locally, no confirm call needed', async ({ launchApp }) => { +// Replaces 'Keep generic dismisses the row locally, no confirm call needed', +// which asserted the row reached toHaveCount(0). That was true and is now +// deliberately false: the decision is persisted, and a persisted-but-hidden +// row would put its own undo somewhere nobody can reach. Renamed rather than +// edited in place -- quietly flipping the assertion would disguise a product +// decision as test maintenance. +test('Keep generic marks the row and leaves the undo one click away', async ({ launchApp }) => { const { page } = await launchApp({ mockIpc: true, env: { STENOAI_E2E_SEED_SPEAKER_SUGGESTIONS: '1' }, @@ -150,7 +156,63 @@ test('Keep generic dismisses the row locally, no confirm call needed', async ({ const row = page.getByTestId('speaker-row-mic:SPEAKER_2'); await expect(row).toBeVisible(); await row.getByRole('button', { name: 'Keep generic label' }).click(); - await expect(row).toHaveCount(0); + + await expect(row).toBeVisible(); + await expect(row).toContainText('Kept generic'); + + const reopen = row.getByRole('button', { name: 'Reopen this speaker for naming' }); + await expect(reopen).toBeVisible(); + await reopen.click(); + await expect(page.getByTestId('speaker-kept-generic-mic:SPEAKER_2')).toHaveCount(0); + await expect(row.getByRole('button', { name: 'Keep generic label' })).toBeVisible(); +}); + +test('the kept-generic marking survives leaving the meeting and coming back', async ({ + launchApp, +}) => { + // The defect this whole slice exists for. The decision used to live in a + // React state set, so it died with the panel, and every row the reviewer + // had already dealt with came back on the next visit -- the exact work + // the button was meant to save. + const { page } = await launchApp({ + mockIpc: true, + env: { STENOAI_E2E_SEED_SPEAKER_SUGGESTIONS: '1' }, + }); + await openDetail(page); + + const row = page.getByTestId('speaker-row-mic:SPEAKER_2'); + await row.getByRole('button', { name: 'Keep generic label' }).click(); + await expect(row).toContainText('Kept generic'); + + await page.evaluate(() => { + window.location.hash = '#/'; + }); + await openDetail(page); + + const rowAfter = page.getByTestId('speaker-row-mic:SPEAKER_2'); + await expect(rowAfter).toContainText('Kept generic'); + await expect(rowAfter.getByRole('button', { name: 'Reopen this speaker for naming' })).toBeVisible(); +}); + +test('Keep generic is not offered on a row that is already decided', async ({ launchApp }) => { + const { page } = await launchApp({ + mockIpc: true, + env: { STENOAI_E2E_SEED_SPEAKER_SUGGESTIONS: '1' }, + }); + await openDetail(page); + + // Confirmed: naming the row settles it, so parking it would say two + // contradictory things about the same cluster. + const confirmed = page.getByTestId('speaker-row-mic:SPEAKER_0'); + await confirmed.getByRole('button', { name: 'Approve' }).click(); + await expect(confirmed).toContainText('✓ Confirmed as Julian'); + await expect(confirmed.getByRole('button', { name: 'Keep generic label' })).toHaveCount(0); + + // Marked as several people: same reasoning from the other direction. + const marked = page.getByTestId('speaker-row-mic:SPEAKER_2'); + await marked.getByRole('button', { name: 'This is more than one person' }).click(); + await expect(marked).toContainText('More than one person'); + await expect(marked.getByRole('button', { name: 'Keep generic label' })).toHaveCount(0); }); test('a cluster with no suggestion and no candidates never renders', async ({ launchApp }) => { From b5f1c58031de150e73aec4fd34df79946c5d0db7 Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 4 Aug 2026 22:16:02 +0200 Subject: [PATCH 16/23] fix(speakers): serialise the review-state write against the other sidecar writes It is a read-modify-write of the same document a confirm rewrites, and that overlap is the one the backend can narrow but not close. Gating the clicks is the half the UI can guarantee. --- app/renderer/src/components/SpeakerReviewPanel.tsx | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/renderer/src/components/SpeakerReviewPanel.tsx b/app/renderer/src/components/SpeakerReviewPanel.tsx index de0a0215..5e7fd672 100644 --- a/app/renderer/src/components/SpeakerReviewPanel.tsx +++ b/app/renderer/src/components/SpeakerReviewPanel.tsx @@ -473,7 +473,13 @@ export function SpeakerReviewPanel({ summaryFile, isDiarised }: SpeakerReviewPan clickable while a confirm was still in progress. */} {visibleRows.map((row) => { const key = rowKey(row); - const anyConfirmPending = confirmSpeaker.isPending || markCluster.isPending; + // setReviewState belongs in this gate even though it writes only a + // marker: it is a read-modify-write of the same sidecar a confirm + // rewrites, and that pair is exactly the overlap the backend can + // narrow but not close (see _freshest_channel). Serialising the + // clicks is the half of it the UI can actually guarantee. + const anyConfirmPending = + confirmSpeaker.isPending || markCluster.isPending || setReviewState.isPending; const isMarked = row.suggestion.contains_multiple_speakers; const isKept = isKeptGeneric(row.suggestion); const samples = row.suggestion.samples ?? []; From eafb592f271e5245d791f9f42842a80db514b8e1 Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 4 Aug 2026 22:16:03 +0200 Subject: [PATCH 17/23] docs(speakers): tick off task 7 in the plan --- .../2026-08-04-speaker-review-run-provenance.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md b/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md index 96e4e87d..2309b784 100644 --- a/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md +++ b/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md @@ -163,12 +163,12 @@ - Consumes: Task 6's CLI and echo, Task 5's `stale_assignments`. - Produces: `speakers.setClusterReviewState({ meetingStem, channel, diarizationSpeakerId, generic })` on the preload bridge, and `useSetClusterReviewState` in the hooks module. -- [ ] **Step 1: Write the failing renderer tests.** Beside `speakerReviewOrdering.test.ts`, test the pure derivations rather than the component internals: a row whose suggestion carries `review_state: "generic"` reads as kept-generic; the notice text is produced when `stale_assignments` is non-empty and not when it is empty. Extract the derivations as exported helpers so they are testable without mounting, the same way `orderProfilesForRow` already is. -- [ ] **Step 2: Replace the T1 test that pins the old behaviour, deliberately.** `speaker-review.t1.spec.ts` currently holds `'Keep generic dismisses the row locally, no confirm call needed'`, which asserts the row reaches `toHaveCount(0)`. This slice inverts that on purpose, so the test must be **rewritten and renamed** - its present name becomes false. The replacement asserts the new contract: after clicking, the row stays visible, reads as kept generic, and the undo is one click away. Renaming it is the point: a silently adjusted assertion would disguise a product decision as test maintenance. Then add the two new assertions - the button is absent on a confirmed row and on a mixed row - and extend `app/e2e-mock-ipc.js` with the new handler and the `review_state` echo so the mock matches the real contract. Leave the in-flight-disabled assertion around line 253 intact; it covers a different property of the same button. -- [ ] **Step 3: Run both and watch them fail.** `npx vitest run` and `npm run test:e2e -- --project=t1 --grep speaker`. -- [ ] **Step 4: Bridge and wire.** The `ipcMain.handle` mirrors the `mark-speaker-cluster` handler including `parsePythonFailureJson` on error; the preload entry joins the existing `speakers` group; the hook invalidates `speakersKeys.suggestions(meetingStem)`. In the panel, the button calls the mutation, and the `dismissed` state and its `notDismissed` filtering are **removed** - the marker now comes from query data, which is what makes it survive a remount by construction. Gate the button off on confirmed and mixed rows; today it renders on both because it sits outside the `!isMarked` conditional. -- [ ] **Step 5: Run typecheck, lint, vitest, T1.** Compare lint against the 37/0 baseline. -- [ ] **Step 6: Commit.** +- [x] **Step 1: Write the failing renderer tests.** Beside `speakerReviewOrdering.test.ts`, test the pure derivations rather than the component internals: a row whose suggestion carries `review_state: "generic"` reads as kept-generic; the notice text is produced when `stale_assignments` is non-empty and not when it is empty. Extract the derivations as exported helpers so they are testable without mounting, the same way `orderProfilesForRow` already is. +- [x] **Step 2: Replace the T1 test that pins the old behaviour, deliberately.** `speaker-review.t1.spec.ts` currently holds `'Keep generic dismisses the row locally, no confirm call needed'`, which asserts the row reaches `toHaveCount(0)`. This slice inverts that on purpose, so the test must be **rewritten and renamed** - its present name becomes false. The replacement asserts the new contract: after clicking, the row stays visible, reads as kept generic, and the undo is one click away. Renaming it is the point: a silently adjusted assertion would disguise a product decision as test maintenance. Then add the two new assertions - the button is absent on a confirmed row and on a mixed row - and extend `app/e2e-mock-ipc.js` with the new handler and the `review_state` echo so the mock matches the real contract. Leave the in-flight-disabled assertion around line 253 intact; it covers a different property of the same button. +- [x] **Step 3: Run both and watch them fail.** `npx vitest run` and `npm run test:e2e -- --project=t1 --grep speaker`. +- [x] **Step 4: Bridge and wire.** The `ipcMain.handle` mirrors the `mark-speaker-cluster` handler including `parsePythonFailureJson` on error; the preload entry joins the existing `speakers` group; the hook invalidates `speakersKeys.suggestions(meetingStem)`. In the panel, the button calls the mutation, and the `dismissed` state and its `notDismissed` filtering are **removed** - the marker now comes from query data, which is what makes it survive a remount by construction. Gate the button off on confirmed and mixed rows; today it renders on both because it sits outside the `!isMarked` conditional. +- [x] **Step 5: Run typecheck, lint, vitest, T1.** Compare lint against the 37/0 baseline. +- [x] **Step 6: Commit.** --- From 978b4f4396d554f342981b6bc1295cb51fc5f391 Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 4 Aug 2026 22:19:33 +0200 Subject: [PATCH 18/23] feat(speakers): say which review markings a re-diarization just discarded A fresh diarization numbers its clusters from SPEAKER_0 again, so every marking a human made against the old ids stops describing anything. Dropping them is right - carrying them forward would attach a person's statement to whichever voice inherited an id. Dropping them silently is not: they are the one thing in that file no re-run can reproduce. The backfill already reported the mixed markings and now reports the kept- generic ones the same way. `reprocess --retranscribe` reported neither, which is a pre-existing silent loss on the path this slice already touches; it gains a warning plus one greppable stdout line, because reprocess streams lines rather than one JSON document. Both counts come from one shared helper. They are the only two places a marking is ever lost, and a second copy of the counting is exactly how one of them would quietly stop counting the newer kind. The reprocess test asserts on today's silence first, so it fails before the fix rather than after. --- simple_recorder.py | 53 +++++++++++++++++---- src/speaker_suggestions.py | 33 +++++++++++++ tests/test_backfill_cli.py | 49 ++++++++++++++++++++ tests/test_speaker_multi_marking.py | 72 +++++++++++++++++++++++++++++ 4 files changed, 198 insertions(+), 9 deletions(-) diff --git a/simple_recorder.py b/simple_recorder.py index 9872cb12..bff108f2 100644 --- a/simple_recorder.py +++ b/simple_recorder.py @@ -353,7 +353,27 @@ def _persist_speaker_sidecar(output_dir, meeting_stem: str, transcript_data: dic speaker_clusters = transcript_data.get("speaker_clusters") or {} if not speaker_clusters: return False - from src.speaker_suggestions import write_speakers_sidecar + from src.speaker_suggestions import ( + count_review_markings, read_speakers_sidecar, write_speakers_sidecar, + ) + # Read before overwriting, purely to report what this run discards. A + # fresh diarization numbers its clusters from SPEAKER_0 again, so every + # marking a human made against the old ids stops describing anything -- + # carrying them forward would attach a person's statement to whichever + # voice inherited the id. Losing them is right; losing them silently is + # not, and they are the one thing in this file no re-run reproduces. + # `reprocess` streams lines rather than one JSON document, so the report + # is a warning plus one greppable stdout line, mirroring the backfill's + # wording. + lost = count_review_markings(read_speakers_sidecar(output_dir, meeting_stem)) + if lost["multi_speaker"] or lost["review_state"]: + message = ( + f"{meeting_stem}: re-diarization discards " + f"{lost['multi_speaker']} cluster(s) marked as containing multiple speakers " + f"and {lost['review_state']} kept generic." + ) + logger.warning("Speaker markings lost: %s", message) + print(f"LOST_SPEAKER_MARKINGS: {message}", flush=True) write_speakers_sidecar( output_dir, meeting_stem, speaker_clusters, turn_manifest=transcript_data.get("turn_manifest"), @@ -6182,7 +6202,7 @@ def backfill_speaker_embeddings(limit, extension, force, meeting_stem): from src.config import get_config, get_data_dirs from src.transcriber import WhisperTranscriber, STENO_DIARIZE_TIMEOUT_FLOOR_S, _run_steno_diarize from src.speaker_suggestions import ( - build_clusters_from_diarization, cluster_ids_marked_multi_speaker, + build_clusters_from_diarization, count_review_markings, determine_recording_type, read_speakers_sidecar, speakers_sidecar_path, write_speakers_sidecar, ) @@ -6226,6 +6246,7 @@ def backfill_speaker_embeddings(limit, extension, force, meeting_stem): skipped_no_audio = [] skipped_no_clusters = [] lost_multi_speaker_markings = [] + lost_review_state_markings = [] errors = [] for stem in stems: @@ -6271,17 +6292,27 @@ def backfill_speaker_embeddings(limit, extension, force, meeting_stem): # marking is genuinely gone rather than transferable -- but # it is the one thing in that file no re-run can reproduce, # so losing it is reported instead of silent. - dropped = sum( - len(cluster_ids_marked_multi_speaker(ch)) - for ch in ((previous_sidecar or {}).get("channels") or {}).values() - ) - if dropped: + # Counted by the shared helper, so this report and + # _persist_speaker_sidecar's cannot drift apart on what + # counts as a marking (they are the only two places one is + # ever lost). + dropped = count_review_markings(previous_sidecar) + if dropped["multi_speaker"]: logger.warning( "backfill-speaker-embeddings: %s had %d cluster(s) marked as " "containing multiple speakers; re-diarization discards those markings.", - stem, dropped, + stem, dropped["multi_speaker"], + ) + lost_multi_speaker_markings.append( + {"stem": stem, "clusters": dropped["multi_speaker"]}) + if dropped["review_state"]: + logger.warning( + "backfill-speaker-embeddings: %s had %d cluster(s) kept generic; " + "re-diarization discards those markings.", + stem, dropped["review_state"], ) - lost_multi_speaker_markings.append({"stem": stem, "clusters": dropped}) + lost_review_state_markings.append( + {"stem": stem, "clusters": dropped["review_state"]}) write_speakers_sidecar(output_dir, stem, channels_out) processed.append(stem) else: @@ -6305,6 +6336,10 @@ def backfill_speaker_embeddings(limit, extension, force, meeting_stem): "skipped_no_clusters": skipped_no_clusters, "skipped_already_processed": skipped_already_processed, "lost_multi_speaker_markings": lost_multi_speaker_markings, + # Same shape and the same reason: a marking is a human statement the + # re-diarization cannot carry over, and the only one in this file no + # re-run can reproduce. + "lost_review_state_markings": lost_review_state_markings, "errors": errors, "total_meetings": len(all_stems), })) diff --git a/src/speaker_suggestions.py b/src/speaker_suggestions.py index 10430990..b86d4893 100644 --- a/src/speaker_suggestions.py +++ b/src/speaker_suggestions.py @@ -709,6 +709,39 @@ def cluster_ids_marked_multi_speaker(channel_data: dict) -> set: } +def count_review_markings(sidecar: Optional[dict]) -> dict: + """How many clusters in a whole sidecar carry each human marking: + `{"multi_speaker": n, "review_state": n}`. + + Exists once so the two paths that overwrite a sidecar with a fresh + diarization -- `backfill-speaker-embeddings` and + `_persist_speaker_sidecar` (reprocess --retranscribe) -- report the same + thing. Those are the only places a marking is ever lost, and a second + copy of this counting is how one of them would quietly stop counting the + newer kind. + + Tolerates any shape, including None: it is called only to build a + warning, and a warning must never be what fails a re-diarization. + """ + counts = {"multi_speaker": 0, "review_state": 0} + if not isinstance(sidecar, dict): + return counts + channels = sidecar.get("channels") + if not isinstance(channels, dict): + return counts + for channel_data in channels.values(): + if not isinstance(channel_data, dict): + continue + for cluster in _cluster_entries(channel_data).values(): + if not isinstance(cluster, dict): + continue + if cluster.get(MULTI_SPEAKER_KEY): + counts["multi_speaker"] += 1 + if cluster.get(REVIEW_STATE_KEY): + counts["review_state"] += 1 + return counts + + def set_cluster_multi_speaker( output_dir: Path, meeting_stem: str, channel: str, diarization_speaker_id: str, marked: bool, diff --git a/tests/test_backfill_cli.py b/tests/test_backfill_cli.py index 532b8513..71582ee6 100644 --- a/tests/test_backfill_cli.py +++ b/tests/test_backfill_cli.py @@ -218,3 +218,52 @@ def test_meeting_option_ignores_limit(self): if __name__ == "__main__": unittest.main() + + +class BackfillReportsLostMarkingsTests(BackfillSpeakerEmbeddingsCliTests): + """A re-diarization drops every human marking on the old clusters, and + that is correct: the new run numbers its clusters independently, so + carrying the markings over would attach a person's statement to whichever + voice happened to inherit an id. Losing them SILENTLY is the defect -- + they are the one thing in that file no re-run can reproduce.""" + + def _seed_marked_sidecar(self, tmp, stem="mtg001", multi=False, generic=False): + from src.speaker_suggestions import ( + REVIEW_STATE_GENERIC, set_cluster_multi_speaker, set_cluster_review_state, + write_speakers_sidecar, + ) + output_dir = Path(tmp) / "output" + write_speakers_sidecar(output_dir, stem, { + "mic": { + "recording_type": "in_person", + "clusters": { + "SPEAKER_0": {"embedding": [1.0, 0.0], "speech_duration_seconds": 30.0, + "segment_count": 5}, + "SPEAKER_1": {"embedding": [0.0, 1.0], "speech_duration_seconds": 20.0, + "segment_count": 4}, + }, + }, + }) + if multi: + set_cluster_multi_speaker(output_dir, stem, "mic", "SPEAKER_0", True) + if generic: + set_cluster_review_state(output_dir, stem, "mic", "SPEAKER_1", REVIEW_STATE_GENERIC) + + def test_reports_review_markings_it_is_about_to_discard(self): + with tempfile.TemporaryDirectory() as tmp: + self._seed_meeting(tmp) + self._seed_marked_sidecar(tmp, multi=True, generic=True) + data = _last_json(self._run(["--force"], tmp).output) + self.assertEqual(data["processed"], ["mtg001"]) + self.assertEqual( + data["lost_multi_speaker_markings"], [{"stem": "mtg001", "clusters": 1}]) + self.assertEqual( + data["lost_review_state_markings"], [{"stem": "mtg001", "clusters": 1}]) + + def test_says_nothing_when_there_was_nothing_to_lose(self): + with tempfile.TemporaryDirectory() as tmp: + self._seed_meeting(tmp) + self._seed_marked_sidecar(tmp) + data = _last_json(self._run(["--force"], tmp).output) + self.assertEqual(data["lost_multi_speaker_markings"], []) + self.assertEqual(data["lost_review_state_markings"], []) diff --git a/tests/test_speaker_multi_marking.py b/tests/test_speaker_multi_marking.py index 17cdb0b4..6f1e4761 100644 --- a/tests/test_speaker_multi_marking.py +++ b/tests/test_speaker_multi_marking.py @@ -1909,3 +1909,75 @@ def test_a_structurally_wrong_sidecar_fails_as_json_not_as_a_traceback(self): self.assertIn(result.exit_code, (0, 1)) if result.exit_code == 1: self.assertFalse(_last_json(result.output)["success"]) + + +class PersistSidecarReportsLostMarkingsTests(unittest.TestCase): + """`reprocess --retranscribe` re-runs the whole transcription, including + diarization, and overwrites the sidecar through _persist_speaker_sidecar. + Every human marking on the old clusters goes with it -- correctly, since + the new run's ids describe different voices -- but this path said nothing + at all, unlike the backfill next door. A marking is the one thing in that + file no re-run can reproduce, so its loss has to be greppable afterwards. + """ + + def _seed(self, tmp, multi=False, generic=False): + from src.speaker_suggestions import ( + REVIEW_STATE_GENERIC, set_cluster_multi_speaker, set_cluster_review_state, + ) + output_dir = Path(tmp) / "output" + output_dir.mkdir(parents=True, exist_ok=True) + write_speakers_sidecar(output_dir, "mtg001", { + "mic": { + "recording_type": "in_person", + "clusters": { + "SPEAKER_0": {"embedding": [1.0, 0.0], "speech_duration_seconds": 30.0, + "segment_count": 5}, + "SPEAKER_1": {"embedding": [0.0, 1.0], "speech_duration_seconds": 20.0, + "segment_count": 4}, + }, + }, + }) + if multi: + set_cluster_multi_speaker(output_dir, "mtg001", "mic", "SPEAKER_0", True) + if generic: + set_cluster_review_state(output_dir, "mtg001", "mic", "SPEAKER_1", REVIEW_STATE_GENERIC) + return output_dir + + _FRESH_RUN = { + "speaker_clusters": { + "mic": { + "recording_type": "in_person", + "clusters": { + "SPEAKER_0": {"embedding": [0.0, 1.0], "speech_duration_seconds": 28.0, + "segment_count": 5}, + }, + }, + }, + } + + def test_overwriting_a_marked_sidecar_reports_both_kinds_of_loss(self): + with tempfile.TemporaryDirectory() as tmp: + output_dir = self._seed(tmp, multi=True, generic=True) + with mock.patch("simple_recorder.logger") as log: + self.assertTrue( + simple_recorder._persist_speaker_sidecar(output_dir, "mtg001", self._FRESH_RUN)) + warned = " ".join(str(c) for c in log.warning.call_args_list) + self.assertIn("mtg001", warned) + self.assertIn("multiple speakers", warned) + self.assertIn("kept generic", warned) + + def test_says_nothing_when_the_previous_sidecar_carried_no_markings(self): + with tempfile.TemporaryDirectory() as tmp: + output_dir = self._seed(tmp) + with mock.patch("simple_recorder.logger") as log: + simple_recorder._persist_speaker_sidecar(output_dir, "mtg001", self._FRESH_RUN) + self.assertEqual(log.warning.call_args_list, []) + + def test_a_first_run_with_no_previous_sidecar_reports_nothing(self): + with tempfile.TemporaryDirectory() as tmp: + output_dir = Path(tmp) / "output" + output_dir.mkdir(parents=True, exist_ok=True) + with mock.patch("simple_recorder.logger") as log: + self.assertTrue( + simple_recorder._persist_speaker_sidecar(output_dir, "mtg001", self._FRESH_RUN)) + self.assertEqual(log.warning.call_args_list, []) From 5930f9dddf80ba8cafcdd61ad6796c7479c6b992 Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 4 Aug 2026 22:19:33 +0200 Subject: [PATCH 19/23] docs(speakers): tick off task 8 in the plan --- .../plans/2026-08-04-speaker-review-run-provenance.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md b/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md index 2309b784..b16274a5 100644 --- a/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md +++ b/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md @@ -182,11 +182,11 @@ - Consumes: Task 6's key. - Produces: no new public surface; a counted, logged report on both paths. -- [ ] **Step 1: Write the failing tests.** `backfill-speaker-embeddings --force` over a sidecar carrying `review_state` markings reports their count alongside the existing `lost_multi_speaker_markings`. The `reprocess --retranscribe` path emits a warning naming both counts where today it emits nothing - this is the pre-existing silent loss, so assert on the current silence first to prove the test bites. -- [ ] **Step 2: Run and watch them fail.** -- [ ] **Step 3: Implement.** The backfill already reads the previous sidecar before overwriting; extend its accounting. Give `_persist_speaker_sidecar` the same read-before-overwrite accounting, reported as a `logger.warning` plus one greppable stdout line mirroring the backfill's wording, because `reprocess` streams lines rather than one JSON document. Do not surface it in the renderer; that is out of scope. -- [ ] **Step 4: Run green.** -- [ ] **Step 5: Commit.** +- [x] **Step 1: Write the failing tests.** `backfill-speaker-embeddings --force` over a sidecar carrying `review_state` markings reports their count alongside the existing `lost_multi_speaker_markings`. The `reprocess --retranscribe` path emits a warning naming both counts where today it emits nothing - this is the pre-existing silent loss, so assert on the current silence first to prove the test bites. +- [x] **Step 2: Run and watch them fail.** +- [x] **Step 3: Implement.** The backfill already reads the previous sidecar before overwriting; extend its accounting. Give `_persist_speaker_sidecar` the same read-before-overwrite accounting, reported as a `logger.warning` plus one greppable stdout line mirroring the backfill's wording, because `reprocess` streams lines rather than one JSON document. Do not surface it in the renderer; that is out of scope. +- [x] **Step 4: Run green.** +- [x] **Step 5: Commit.** --- From b70bbcccce5b99084434f76d0bca7d3b4156f781 Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 4 Aug 2026 22:26:53 +0200 Subject: [PATCH 20/23] test(speakers): drive the review state through the real bridge and backend The standing e2e rule for a user-facing change, and the only proof that matters for this one: "Keep generic" used to change nothing outside React, so a test that stops at the renderer proves nothing about it. This drives window.stenoai.speakers.setClusterReviewState through the preload bridge against the real CLI, reads the meeting's _speakers.json off disk, and asserts the key landed on exactly the cluster it was handed - with the voice embeddings still intact after the rewrite. Also covers the two transitions the panel depends on: a confirm clears the marking, and the explicit undo removes the key rather than storing a null. The writeSpeakersSidecar fixture stays legacy-shaped, untouched. speaker-naming.t2 and speaker-multi-marking.t2 staying green against it is the backward-compatibility proof for the run block and this key alike. --- e2e/specs/speaker-multi-marking.t2.spec.ts | 85 ++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/e2e/specs/speaker-multi-marking.t2.spec.ts b/e2e/specs/speaker-multi-marking.t2.spec.ts index 718a7a94..1fa7f532 100644 --- a/e2e/specs/speaker-multi-marking.t2.spec.ts +++ b/e2e/specs/speaker-multi-marking.t2.spec.ts @@ -42,6 +42,18 @@ type StenoWindow = Window & { newPersonName?: string; personId?: string; }) => Promise<{ success: boolean; error?: string }>; + setClusterReviewState: (params: { + meetingStem: string; + channel: string; + diarizationSpeakerId: string; + generic: boolean; + }) => Promise<{ + success: boolean; + error?: string; + resolved_diarization_speaker_id?: string; + fragment_ids?: string[]; + review_state?: string | null; + }>; suggestForMeeting: (meetingStem: string) => Promise<{ success: boolean; minimum_speaker_count?: number; @@ -54,6 +66,7 @@ type StenoWindow = Window & { suggested_name: string | null; candidates: unknown[]; contains_multiple_speakers?: boolean; + review_state?: string | null; sample_text?: string | null; samples?: Array<{ start: number; end: number; text: string | null }>; } @@ -426,3 +439,75 @@ test('deleting a meeting reports its unnamed speakers and removes its voice-embe expect(fileSig(realUserDataDir())).toEqual(realDirBefore); }); + +test('keeping a speaker generic round-trips through the sidecar, and a confirm clears it', async ({ + launchApp, + userDataDir, +}) => { + // The standing-rule coverage for the persisted review state: the panel's + // "Keep generic" used to change nothing outside React, so the only proof + // that matters is that the marking is on disk and comes back out of a + // real suggest-speakers. The fixture stays legacy-shaped (no run block) -- + // its specs staying green IS the backward-compatibility proof. + const realDirBefore = fileSig(realUserDataDir()); + const stem = 'e2e-review-state'; + seedMeeting(userDataDir, stem); + + const { page } = await launchApp(); + const sidecarPath = path.join(userDataDir, 'output', `${stem}_speakers.json`); + + const before = await page.evaluate( + (s) => (window as StenoWindow).stenoai.speakers.suggestForMeeting(s), stem, + ); + expect(before.channels.system.SPEAKER_0.review_state ?? null).toBeNull(); + + const marked = await page.evaluate( + (params) => (window as StenoWindow).stenoai.speakers.setClusterReviewState(params), + { meetingStem: stem, channel: 'system', diarizationSpeakerId: 'SPEAKER_0', generic: true }, + ); + expect(marked.success).toBe(true); + expect(marked.review_state).toBe('generic'); + + // On disk, in the cluster entry itself -- and the embeddings the sidecar + // exists to carry survived the rewrite. + await expect + .poll(() => readJson(sidecarPath).channels.system.clusters.SPEAKER_0.review_state) + .toBe('generic'); + expect(readJson(sidecarPath).channels.system.clusters.SPEAKER_0.embedding).toEqual([1.0, 0.0]); + // Written to exactly the cluster it was handed, not smeared across the channel. + expect(readJson(sidecarPath).channels.system.clusters.SPEAKER_1.review_state).toBeUndefined(); + + const after = await page.evaluate( + (s) => (window as StenoWindow).stenoai.speakers.suggestForMeeting(s), stem, + ); + expect(after.channels.system.SPEAKER_0.review_state).toBe('generic'); + + // Naming the cluster is a stronger statement about it and supersedes the + // marking -- otherwise the panel would report a confirmed row as parked. + const confirmed = await page.evaluate( + (params) => (window as StenoWindow).stenoai.speakers.confirm(params), + { meetingStem: stem, channel: 'system', diarizationSpeakerId: 'SPEAKER_0', newPersonName: 'Ida' }, + ); + expect(confirmed.success).toBe(true); + await expect + .poll(() => readJson(sidecarPath).channels.system.clusters.SPEAKER_0.review_state) + .toBeUndefined(); + + // And the explicit undo removes the key rather than storing a null. + const reMarked = await page.evaluate( + (params) => (window as StenoWindow).stenoai.speakers.setClusterReviewState(params), + { meetingStem: stem, channel: 'system', diarizationSpeakerId: 'SPEAKER_1', generic: true }, + ); + expect(reMarked.success).toBe(true); + const cleared = await page.evaluate( + (params) => (window as StenoWindow).stenoai.speakers.setClusterReviewState(params), + { meetingStem: stem, channel: 'system', diarizationSpeakerId: 'SPEAKER_1', generic: false }, + ); + expect(cleared.success).toBe(true); + expect(cleared.review_state).toBeNull(); + await expect + .poll(() => readJson(sidecarPath).channels.system.clusters.SPEAKER_1.review_state) + .toBeUndefined(); + + expect(fileSig(realUserDataDir())).toEqual(realDirBefore); +}); From 522fd12cde70c8a36cee857886309039877c92e1 Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 4 Aug 2026 22:26:53 +0200 Subject: [PATCH 21/23] docs(speakers): tick off task 9 in the plan --- .../plans/2026-08-04-speaker-review-run-provenance.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md b/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md index b16274a5..979ee942 100644 --- a/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md +++ b/docs/superpowers/plans/2026-08-04-speaker-review-run-provenance.md @@ -200,11 +200,11 @@ - Consumes: everything above. - Produces: the standing-rule coverage for a user-facing change. -- [ ] **Step 1: Write the failing spec.** Drive `window.stenoai.speakers.setClusterReviewState` through the preload bridge against the real backend, then read the meeting's `_speakers.json` from disk and assert the `review_state` key on the right cluster; clear it and assert it is gone. Model-free, following the existing T2 speaker specs. -- [ ] **Step 2: Run and watch it fail.** `npm run test:e2e -- --project=t2 --grep-invert @pipeline`. -- [ ] **Step 3: Confirm the compatibility proof still holds.** `speaker-naming.t2` and `speaker-multi-marking.t2` must pass against the **unchanged**, legacy-shaped `writeSpeakersSidecar` fixture. If a change to that fixture was needed to make anything pass, the backward compatibility is broken and the cause is in Tasks 1-6, not in the fixture. -- [ ] **Step 4: Full verification.** `ruff check .`, `python -m unittest discover tests`, `npm run typecheck:renderer`, `npm run lint:renderer`, `npx vitest run`, T1 and model-free T2. Compare every number against the baseline in Global Constraints and classify any difference. -- [ ] **Step 5: Commit.** +- [x] **Step 1: Write the failing spec.** Drive `window.stenoai.speakers.setClusterReviewState` through the preload bridge against the real backend, then read the meeting's `_speakers.json` from disk and assert the `review_state` key on the right cluster; clear it and assert it is gone. Model-free, following the existing T2 speaker specs. +- [x] **Step 2: Run and watch it fail.** `npm run test:e2e -- --project=t2 --grep-invert @pipeline`. +- [x] **Step 3: Confirm the compatibility proof still holds.** `speaker-naming.t2` and `speaker-multi-marking.t2` must pass against the **unchanged**, legacy-shaped `writeSpeakersSidecar` fixture. If a change to that fixture was needed to make anything pass, the backward compatibility is broken and the cause is in Tasks 1-6, not in the fixture. +- [x] **Step 4: Full verification.** `ruff check .`, `python -m unittest discover tests`, `npm run typecheck:renderer`, `npm run lint:renderer`, `npx vitest run`, T1 and model-free T2. Compare every number against the baseline in Global Constraints and classify any difference. +- [x] **Step 5: Commit.** --- From e87236fb38d112b90f394a0e05e63fbc26047a5a Mon Sep 17 00:00:00 2001 From: Optic00 Date: Tue, 4 Aug 2026 22:39:39 +0200 Subject: [PATCH 22/23] fix(speakers): a parked row stops offering three ways to name it The kept-generic row rendered "Kept generic - you decided not to name this speaker" with Approve, Change and New person beside the sentence, and a button offering to "Reopen" something that was never closed. Parking a row now means what marking one already means in this panel: the naming actions go with the decision and come back on the one click that reverses it. The row's label recedes to the secondary colour too, so parked rows read as done at a glance. Found by the whole-branch review, which is where it could be found: each of the three fields is right on its own, and only the combination renders the contradiction. --- .../src/components/SpeakerReviewPanel.tsx | 17 +++++++++-- .../src/components/speakerReviewState.test.ts | 29 ++++++++++++++++++- e2e/specs/speaker-review.t1.spec.ts | 7 +++++ 3 files changed, 50 insertions(+), 3 deletions(-) diff --git a/app/renderer/src/components/SpeakerReviewPanel.tsx b/app/renderer/src/components/SpeakerReviewPanel.tsx index 5e7fd672..f4746273 100644 --- a/app/renderer/src/components/SpeakerReviewPanel.tsx +++ b/app/renderer/src/components/SpeakerReviewPanel.tsx @@ -112,6 +112,19 @@ export function showsKeepGenericButton(suggestion: SpeakerSuggestion): boolean { return !suggestion.confirmed_by_user && !suggestion.contains_multiple_speakers; } +/** Whether this row still offers Approve / Change / New person. + * + * Withheld on a mixed row because the backend refuses to enroll it, and on + * a row kept generic because parking a row has to mean the same thing here + * as marking one does: the reviewer said they are done with it. Leaving the + * naming actions there produced a row that said "you decided not to name + * this speaker" with three ways to name it beside the sentence, and offered + * to "Reopen" something that was never closed. Reopening is one click, and + * it brings them all back. */ +export function showsNamingActions(suggestion: SpeakerSuggestion): boolean { + return !suggestion.contains_multiple_speakers && !isKeptGeneric(suggestion); +} + /** The one meeting-level sentence for confirmations a re-diarization * orphaned, or null when there are none. * @@ -498,7 +511,7 @@ export function SpeakerReviewPanel({ summaryFile, isDiarised }: SpeakerReviewPan

{suggestionLabel(row.suggestion)} @@ -583,7 +596,7 @@ export function SpeakerReviewPanel({ summaryFile, isDiarised }: SpeakerReviewPan become available, and a "Change" picker would be an invitation to do the one thing this marking exists to prevent. The undo below is what stays reachable. */} - {!isMarked && ( + {showsNamingActions(row.suggestion) && ( <> {/* Hidden once confirmed_by_user is set -- re-approving an already-confirmed cluster is a no-op that changes diff --git a/app/renderer/src/components/speakerReviewState.test.ts b/app/renderer/src/components/speakerReviewState.test.ts index 5d640b32..942f447e 100644 --- a/app/renderer/src/components/speakerReviewState.test.ts +++ b/app/renderer/src/components/speakerReviewState.test.ts @@ -1,6 +1,11 @@ import { describe, expect, it } from 'vitest'; -import { isKeptGeneric, showsKeepGenericButton, staleAssignmentNotice } from './SpeakerReviewPanel'; +import { + isKeptGeneric, + showsKeepGenericButton, + showsNamingActions, + staleAssignmentNotice, +} from './SpeakerReviewPanel'; const suggestion = (over: Record = {}) => ({ @@ -62,6 +67,28 @@ describe('showsKeepGenericButton', () => { }); }); +describe('showsNamingActions', () => { + it('offers naming on an ordinary row', () => { + expect(showsNamingActions(suggestion())).toBe(true); + }); + + it('withholds it on a row marked as several people', () => { + expect(showsNamingActions(suggestion({ contains_multiple_speakers: true }))).toBe(false); + }); + + it('withholds it on a row kept generic', () => { + // Otherwise the row says "Kept generic - you decided not to name this + // speaker" while Approve, Change and New person sit beside it, and the + // button offers to "Reopen" something that was never closed. Parking a + // row has to mean the same thing here as marking one does. + expect(showsNamingActions(suggestion({ review_state: 'generic' }))).toBe(false); + }); + + it('comes back the moment the row is reopened', () => { + expect(showsNamingActions(suggestion({ review_state: null }))).toBe(true); + }); +}); + describe('staleAssignmentNotice', () => { it('says nothing when nothing was orphaned', () => { expect(staleAssignmentNotice([])).toBeNull(); diff --git a/e2e/specs/speaker-review.t1.spec.ts b/e2e/specs/speaker-review.t1.spec.ts index b364ed79..0b7ff2bf 100644 --- a/e2e/specs/speaker-review.t1.spec.ts +++ b/e2e/specs/speaker-review.t1.spec.ts @@ -159,12 +159,19 @@ test('Keep generic marks the row and leaves the undo one click away', async ({ l await expect(row).toBeVisible(); await expect(row).toContainText('Kept generic'); + // Parked means parked: the naming actions go with the decision, the same + // way they do on a row marked as several people. Otherwise the row says + // "you decided not to name this speaker" with three ways to name it + // beside the sentence, and offers to reopen what was never closed. + await expect(row.getByRole('button', { name: 'Change' })).toHaveCount(0); + await expect(row.getByRole('button', { name: 'New person' })).toHaveCount(0); const reopen = row.getByRole('button', { name: 'Reopen this speaker for naming' }); await expect(reopen).toBeVisible(); await reopen.click(); await expect(page.getByTestId('speaker-kept-generic-mic:SPEAKER_2')).toHaveCount(0); await expect(row.getByRole('button', { name: 'Keep generic label' })).toBeVisible(); + await expect(row.getByRole('button', { name: 'Change' })).toBeVisible(); }); test('the kept-generic marking survives leaving the meeting and coming back', async ({ From e803724222e691ded62ab6d782770231be1ec3fa Mon Sep 17 00:00:00 2001 From: Optic00 Date: Wed, 5 Aug 2026 18:58:17 +0200 Subject: [PATCH 23/23] fix(speakers): flush the sidecar to disk before renaming it into place Atomic and durable are not the same guarantee. The rename made sure a reader never sees half a document; it said nothing about the bytes having left the page cache. A power cut or kernel panic in that window renames an EMPTY file over the real sidecar - and unlike a transcript this one cannot be regenerated, because it holds the only copy of the meeting's voice embeddings and the source audio is deleted by default. The rename itself is flushed too, best-effort and silent: by then the data is already on disk, opening a directory is not portable, and failing there would turn a completed write into a reported error. A failed flush now takes the same path a failed write already did - temp file removed, error raised - because a write that could not be made durable must not report success. Found in the review of #473 and carried open since; verified still open before fixing. The test pins the ORDER, not just the call: flushing after the rename protects nothing. --- src/speaker_suggestions.py | 28 +++++++++++- tests/test_speaker_suggestions.py | 72 +++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/src/speaker_suggestions.py b/src/speaker_suggestions.py index b86d4893..91207fc3 100644 --- a/src/speaker_suggestions.py +++ b/src/speaker_suggestions.py @@ -919,13 +919,39 @@ def write_sidecar_document(output_dir: Path, meeting_stem: str, sidecar: dict) - try: with os.fdopen(fd, "w") as fh: json.dump(sidecar, fh, indent=2) + # Flushed to stable storage BEFORE the rename, because atomic + # and durable are not the same guarantee. The rename makes sure + # a reader never sees half a document; it says nothing about the + # bytes having left the page cache. A power cut or kernel panic + # in that window renames an EMPTY file over the real sidecar -- + # and unlike a transcript, this one cannot be regenerated, since + # the source audio is deleted by default. Flushing afterwards + # would protect nothing. + fh.flush() + os.fsync(fh.fileno()) tmp_path.replace(path) except OSError: # Never leave a half-written temp file behind for someone to find - # later and mistake for a real sidecar. + # later and mistake for a real sidecar. Covers a failed flush too: + # a write that could not be made durable must not report success. tmp_path.unlink(missing_ok=True) raise + # And the directory entry, so a crash cannot leave it pointing at the + # old file after the caller was told the sidecar was replaced. + # Best-effort and deliberately silent: the data itself is already on + # disk, opening a directory is not portable (Windows refuses it), and + # failing here would turn a completed write into a reported error. + if hasattr(os, "O_DIRECTORY"): + try: + dir_fd = os.open(str(path.parent), os.O_DIRECTORY) + try: + os.fsync(dir_fd) + finally: + os.close(dir_fd) + except OSError: + pass + def minimum_speaker_count(channels: dict) -> int: """The smallest number of real people this meeting's diarization is diff --git a/tests/test_speaker_suggestions.py b/tests/test_speaker_suggestions.py index 62d1be4d..688d00cb 100644 --- a/tests/test_speaker_suggestions.py +++ b/tests/test_speaker_suggestions.py @@ -1,4 +1,5 @@ import json +import os import tempfile import unittest from pathlib import Path @@ -35,6 +36,7 @@ set_cluster_review_state, suggest_speaker, suggest_speakers_for_meeting, + write_sidecar_document, write_speakers_sidecar, ) from src.voiceprint import cosine_distance @@ -1503,3 +1505,73 @@ def test_clearing_what_was_never_marked_reports_nothing_and_raises_nothing(self) if __name__ == "__main__": unittest.main() + + +class SidecarDurabilityTests(unittest.TestCase): + """The rename is atomic; that is not the same as durable. + + This file holds the ONLY copy of a meeting's voice embeddings, and the + source audio is deleted by default -- so unlike a transcript, it cannot + be regenerated. An atomic rename guarantees a reader never sees a half + document; it guarantees nothing about the bytes having reached stable + storage. Without a flush, a power cut or kernel panic in the window + between the write and the disk can leave the renamed file empty, which + is exactly the unrecoverable outcome the atomic rename was chosen to + prevent. + """ + + def _seed(self, tmp): + output_dir = Path(tmp) / "output" + output_dir.mkdir(parents=True, exist_ok=True) + return output_dir, { + "meeting_id": "mtg001", + "channels": {"system": {"recording_type": "remote", "clusters": {}}}, + } + + def test_the_bytes_are_flushed_to_disk_before_the_rename(self): + with tempfile.TemporaryDirectory() as tmp: + output_dir, doc = self._seed(tmp) + calls = [] + real_fsync = os.fsync + real_replace = Path.replace + + def spy_fsync(fd): + calls.append("fsync") + return real_fsync(fd) + + def spy_replace(self, target): + calls.append("replace") + return real_replace(self, target) + + with mock.patch("src.speaker_suggestions.os.fsync", side_effect=spy_fsync), \ + mock.patch.object(Path, "replace", spy_replace): + write_sidecar_document(output_dir, "mtg001", doc) + + self.assertIn("fsync", calls, "the temp file was renamed into place unflushed") + self.assertLess( + calls.index("fsync"), calls.index("replace"), + "flushing after the rename protects nothing", + ) + self.assertEqual(read_speakers_sidecar(output_dir, "mtg001"), doc) + + if hasattr(os, "O_DIRECTORY"): + # The directory entry too, and necessarily AFTER the rename: + # otherwise a crash can leave it pointing at the old file + # even though the caller was told the sidecar was replaced. + self.assertGreater( + calls.count("fsync"), 1, "the rename itself was never flushed", + ) + last_fsync = len(calls) - 1 - calls[::-1].index("fsync") + self.assertGreater(last_fsync, calls.index("replace")) + + def test_a_failed_flush_leaves_no_temp_file_and_does_not_claim_success(self): + # Same contract the write already had: a failure must not leave a + # half-written temp file behind for someone to mistake for a real + # sidecar, and must not return as if the sidecar had been replaced. + with tempfile.TemporaryDirectory() as tmp: + output_dir, doc = self._seed(tmp) + with mock.patch("src.speaker_suggestions.os.fsync", side_effect=OSError("disk gone")): + with self.assertRaises(OSError): + write_sidecar_document(output_dir, "mtg001", doc) + leftovers = [p.name for p in output_dir.iterdir()] + self.assertEqual(leftovers, [], f"temp file left behind: {leftovers}")