feat(speakers): stop a re-diarization from silently reattributing people, and remember the review - #475
Merged
Optic00 merged 23 commits intoAug 5, 2026
Conversation
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.
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.
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.
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.
… 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.
…totype
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.
…irst 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.
…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.
…ized 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.
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.
"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.
…raceback 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.
"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.
…ecar 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.
…rded 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.
…ckend 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.
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.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
8 issues found across 22 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_backfill_cli.py">
<violation number="1" location="tests/test_backfill_cli.py:223">
P3: The new BackfillReportsLostMarkingsTests class is placed after the `if __name__ == "__main__": unittest.main()` block. Running this file directly (`python tests/test_backfill_cli.py`) executes unittest.main() before the class is defined, so the new tests are silently skipped; they only run when imported via unittest discovery. Move the class (and its helpers) above the main guard so both execution modes collect it.</violation>
</file>
<file name="app/e2e-mock-ipc.js">
<violation number="1" location="app/e2e-mock-ipc.js:888">
P2: A merged row can remain partially "kept generic" in T1 after marking it mixed because only one raw id is cleared. Clearing `review_state` for the selected id and its `merged_from` ids would mirror real behavior and avoid mock drift.</violation>
<violation number="2" location="app/e2e-mock-ipc.js:918">
P2: The mock response can misrepresent which raw clusters were updated when a row is merged, so T1 can pass while production behavior differs. Returning all fragment ids (primary plus `merged_from`) would keep IPC contract parity.</violation>
</file>
<file name="tests/test_suggest_speakers_cli.py">
<violation number="1" location="tests/test_suggest_speakers_cli.py:514">
P3: The new classes duplicate the exact `_run` helper (Config creation + get_config patch + STENOAI_USER_DATA_DIR isolation) already in SuggestSpeakersCliTests, and each also re-defines `_seed`. If the CLI invocation env/patch setup changes, these three copies drift silently and tests begin testing the wrong thing. Consider moving `_run`/`_seed` to a small shared mixin or module-level helper so the isolation setup stays in one place.</violation>
</file>
<file name="tests/test_repair_speaker_profiles_cli.py">
<violation number="1" location="tests/test_repair_speaker_profiles_cli.py:239">
P3: The re-diarization run-scoping change to Pass C (channel backfill) has no positive-control test for the run-stamped production path: currently only the legacy both-None match and the mismatch case are covered. Add a case where an entry's diarization_run_id equals the sidecar's diarization_run.run_id and assert channels_backfilled is 1 / channel is written, so a regression in prototype_run_matches for the equal-non-None case can't slip through.</violation>
</file>
<file name="simple_recorder.py">
<violation number="1" location="simple_recorder.py:5363">
P1: A merge fallback now allows `mark-speaker-cluster` to succeed while silently skipping the confirmation cleanup path, so a cluster can be marked mixed but still keep stale speaker evidence. This happens because the fallback empties merge output and leaves `fragment_ids` empty, which bypasses the `if multiple and fragment_ids` block; seeding fallback `fragment_ids` with the requested raw id keeps cleanup consistent.</violation>
</file>
<file name="src/speaker_suggestions.py">
<violation number="1" location="src/speaker_suggestions.py:862">
P3: `set_cluster_review_state` writes whatever `state` value it is given into the sidecar with no check, even though the module explicitly documents that the key holds "exactly one value" (`REVIEW_STATE_GENERIC`). An accidental non-`None`, non-`generic` value from a future caller or a hand-edited sidecar would be persisted verbatim and flow unchanged through `clusters_from_sidecar_channel` into the renderer, which expects only `"generic"`. Validating against `REVIEW_STATE_GENERIC` (or `None`) here keeps the persisted file honest with the documented invariant the way `_freshest_channel` already defends structural shape.</violation>
</file>
<file name="app/renderer/src/components/SpeakerReviewPanel.tsx">
<violation number="1" location="app/renderer/src/components/SpeakerReviewPanel.tsx:751">
P3: The "Keep generic"/"Reopen" mutation has no error feedback, unlike Approve/Change/New person and the multi-speaker toggle on the same row. When the backend refuses the write (for example the cluster no longer exists in the channel, or the sidecar is missing), the rejection is swallowed silently and the click looks like it did nothing, while every other action on the row surfaces a red "Couldn't ..." message. Consider handling the mutation's onError the same way confirm and setMultiSpeaker do, writing to the feedback map (and teaching its message mapping to describe the failed keep-generic/reopen operation), so a failed marking is never mistaken for a successful one.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| # 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 = {}, {} |
Contributor
There was a problem hiding this comment.
P1: A merge fallback now allows mark-speaker-cluster to succeed while silently skipping the confirmation cleanup path, so a cluster can be marked mixed but still keep stale speaker evidence. This happens because the fallback empties merge output and leaves fragment_ids empty, which bypasses the if multiple and fragment_ids block; seeding fallback fragment_ids with the requested raw id keeps cleanup consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At simple_recorder.py, line 5363:
<comment>A merge fallback now allows `mark-speaker-cluster` to succeed while silently skipping the confirmation cleanup path, so a cluster can be marked mixed but still keep stale speaker evidence. This happens because the fallback empties merge output and leaves `fragment_ids` empty, which bypasses the `if multiple and fragment_ids` block; seeding fallback `fragment_ids` with the requested raw id keeps cleanup consistent.</comment>
<file context>
@@ -5230,6 +5300,82 @@ def speaker_timestamps(meeting_stem, channel, diarization_speaker_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:
</file context>
| return { | ||
| success: true, | ||
| resolved_diarization_speaker_id: diarizationSpeakerId, | ||
| fragment_ids: [diarizationSpeakerId], |
Contributor
There was a problem hiding this comment.
P2: The mock response can misrepresent which raw clusters were updated when a row is merged, so T1 can pass while production behavior differs. Returning all fragment ids (primary plus merged_from) would keep IPC contract parity.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/e2e-mock-ipc.js, line 918:
<comment>The mock response can misrepresent which raw clusters were updated when a row is merged, so T1 can pass while production behavior differs. Returning all fragment ids (primary plus `merged_from`) would keep IPC contract parity.</comment>
<file context>
@@ -896,6 +899,27 @@ function install({ ipcMain }) {
+ return {
+ success: true,
+ resolved_diarization_speaker_id: diarizationSpeakerId,
+ fragment_ids: [diarizationSpeakerId],
+ review_state: cluster.review_state,
+ };
</file context>
Suggested change
| fragment_ids: [diarizationSpeakerId], | |
| fragment_ids: [diarizationSpeakerId, ...(cluster.merged_from || [])], |
| } | ||
| // "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; |
Contributor
There was a problem hiding this comment.
P2: A merged row can remain partially "kept generic" in T1 after marking it mixed because only one raw id is cleared. Clearing review_state for the selected id and its merged_from ids would mirror real behavior and avoid mock drift.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/e2e-mock-ipc.js, line 888:
<comment>A merged row can remain partially "kept generic" in T1 after marking it mixed because only one raw id is cleared. Clearing `review_state` for the selected id and its `merged_from` ids would mirror real behavior and avoid mock drift.</comment>
<file context>
@@ -883,6 +883,9 @@ function install({ ipcMain }) {
}
+ // "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);
</file context>
Suggested change
| cluster.review_state = null; | |
| for (const sid of [diarizationSpeakerId, ...(cluster.merged_from || [])]) { | |
| const target = (speakerState.suggestions[channel] || {})[sid]; | |
| if (target) target.review_state = null; | |
| } |
| unittest.main() | ||
|
|
||
|
|
||
| class BackfillReportsLostMarkingsTests(BackfillSpeakerEmbeddingsCliTests): |
Contributor
There was a problem hiding this comment.
P3: The new BackfillReportsLostMarkingsTests class is placed after the if __name__ == "__main__": unittest.main() block. Running this file directly (python tests/test_backfill_cli.py) executes unittest.main() before the class is defined, so the new tests are silently skipped; they only run when imported via unittest discovery. Move the class (and its helpers) above the main guard so both execution modes collect it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_backfill_cli.py, line 223:
<comment>The new BackfillReportsLostMarkingsTests class is placed after the `if __name__ == "__main__": unittest.main()` block. Running this file directly (`python tests/test_backfill_cli.py`) executes unittest.main() before the class is defined, so the new tests are silently skipped; they only run when imported via unittest discovery. Move the class (and its helpers) above the main guard so both execution modes collect it.</comment>
<file context>
@@ -218,3 +218,52 @@ def test_meeting_option_ignores_limit(self):
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
</file context>
| 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): |
Contributor
There was a problem hiding this comment.
P3: The new classes duplicate the exact _run helper (Config creation + get_config patch + STENOAI_USER_DATA_DIR isolation) already in SuggestSpeakersCliTests, and each also re-defines _seed. If the CLI invocation env/patch setup changes, these three copies drift silently and tests begin testing the wrong thing. Consider moving _run/_seed to a small shared mixin or module-level helper so the isolation setup stays in one place.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_suggest_speakers_cli.py, line 514:
<comment>The new classes duplicate the exact `_run` helper (Config creation + get_config patch + STENOAI_USER_DATA_DIR isolation) already in SuggestSpeakersCliTests, and each also re-defines `_seed`. If the CLI invocation env/patch setup changes, these three copies drift silently and tests begin testing the wrong thing. Consider moving `_run`/`_seed` to a small shared mixin or module-level helper so the isolation setup stays in one place.</comment>
<file context>
@@ -488,5 +506,204 @@ def fake_extract(audio_path, channel, segments, output_path, segment_index=None)
+ 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), \
</file context>
|
|
||
| result = self._run(["--apply"], tmp, cfg) | ||
| report = _report(result.output) | ||
| self.assertEqual(report["channels_backfilled"], 0) |
Contributor
There was a problem hiding this comment.
P3: The re-diarization run-scoping change to Pass C (channel backfill) has no positive-control test for the run-stamped production path: currently only the legacy both-None match and the mismatch case are covered. Add a case where an entry's diarization_run_id equals the sidecar's diarization_run.run_id and assert channels_backfilled is 1 / channel is written, so a regression in prototype_run_matches for the equal-non-None case can't slip through.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_repair_speaker_profiles_cli.py, line 239:
<comment>The re-diarization run-scoping change to Pass C (channel backfill) has no positive-control test for the run-stamped production path: currently only the legacy both-None match and the mismatch case are covered. Add a case where an entry's diarization_run_id equals the sidecar's diarization_run.run_id and assert channels_backfilled is 1 / channel is written, so a regression in prototype_run_matches for the equal-non-None case can't slip through.</comment>
<file context>
@@ -155,6 +171,76 @@ def test_apply_backfills_channel_from_sidecar(self):
+
+ 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"])
</file context>
| if state is None: | ||
| cluster.pop(REVIEW_STATE_KEY, None) | ||
| else: | ||
| cluster[REVIEW_STATE_KEY] = state |
Contributor
There was a problem hiding this comment.
P3: set_cluster_review_state writes whatever state value it is given into the sidecar with no check, even though the module explicitly documents that the key holds "exactly one value" (REVIEW_STATE_GENERIC). An accidental non-None, non-generic value from a future caller or a hand-edited sidecar would be persisted verbatim and flow unchanged through clusters_from_sidecar_channel into the renderer, which expects only "generic". Validating against REVIEW_STATE_GENERIC (or None) here keeps the persisted file honest with the documented invariant the way _freshest_channel already defends structural shape.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/speaker_suggestions.py, line 862:
<comment>`set_cluster_review_state` writes whatever `state` value it is given into the sidecar with no check, even though the module explicitly documents that the key holds "exactly one value" (`REVIEW_STATE_GENERIC`). An accidental non-`None`, non-`generic` value from a future caller or a hand-edited sidecar would be persisted verbatim and flow unchanged through `clusters_from_sidecar_channel` into the renderer, which expects only `"generic"`. Validating against `REVIEW_STATE_GENERIC` (or `None`) here keeps the persisted file honest with the documented invariant the way `_freshest_channel` already defends structural shape.</comment>
<file context>
@@ -678,6 +780,124 @@ def set_cluster_multi_speaker(
+ if state is None:
+ cluster.pop(REVIEW_STATE_KEY, None)
+ else:
+ cluster[REVIEW_STATE_KEY] = state
+
+ write_sidecar_document(output_dir, meeting_stem, sidecar)
</file context>
| aria-label={isKept ? 'Reopen this speaker for naming' : 'Keep generic label'} | ||
| title={isKept ? 'Reopen this speaker for naming' : 'Keep generic label'} | ||
| disabled={anyConfirmPending} | ||
| onClick={() => |
Contributor
There was a problem hiding this comment.
P3: The "Keep generic"/"Reopen" mutation has no error feedback, unlike Approve/Change/New person and the multi-speaker toggle on the same row. When the backend refuses the write (for example the cluster no longer exists in the channel, or the sidecar is missing), the rejection is swallowed silently and the click looks like it did nothing, while every other action on the row surfaces a red "Couldn't ..." message. Consider handling the mutation's onError the same way confirm and setMultiSpeaker do, writing to the feedback map (and teaching its message mapping to describe the failed keep-generic/reopen operation), so a failed marking is never mistaken for a successful one.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/renderer/src/components/SpeakerReviewPanel.tsx, line 751:
<comment>The "Keep generic"/"Reopen" mutation has no error feedback, unlike Approve/Change/New person and the multi-speaker toggle on the same row. When the backend refuses the write (for example the cluster no longer exists in the channel, or the sidecar is missing), the rejection is swallowed silently and the click looks like it did nothing, while every other action on the row surfaces a red "Couldn't ..." message. Consider handling the mutation's onError the same way confirm and setMultiSpeaker do, writing to the feedback map (and teaching its message mapping to describe the failed keep-generic/reopen operation), so a failed marking is never mistaken for a successful one.</comment>
<file context>
@@ -638,17 +734,34 @@ export function SpeakerReviewPanel({ summaryFile, isDiarised }: SpeakerReviewPan
+ aria-label={isKept ? 'Reopen this speaker for naming' : 'Keep generic label'}
+ title={isKept ? 'Reopen this speaker for naming' : 'Keep generic label'}
+ disabled={anyConfirmPending}
+ onClick={() =>
+ setReviewState.mutate({
+ meetingStem,
</file context>
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 stenolabs#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #474. Two defects, both real in the code today, both silent.
1. A re-diarization silently reattributed people
reprocess --retranscribe,backfill-speaker-embeddings --forceandfull-reprocesseach produce a fresh diarization run whoseSPEAKER_Nids arenumbered independently of the previous one — so the same id now means a
different voice. Every reader matched evidence on
(meeting, cluster id, channel)only, so a new run'sSPEAKER_0showed up as "Confirmed as X" from aprototype recorded against somebody else.
reprocess --retranscribeevendocumented the consequence in a code comment ("KNOWN CONSEQUENCE, deliberately
not worked around").
The sidecar now carries a
diarization_run.run_id, evidence records the run itwas confirmed against, and one shared predicate decides "is this still
current" for every reader and writer, so the two cannot drift apart.
Seven readers, not the four the design listed. The two extra ones were the
worst:
backfill-participants --relabel-transcriptswrites a person's name intothe transcript. It skipped a cluster id that had vanished from the new run,
but not one that had been reused — which is the normal case, since the
diarizer numbers from
SPEAKER_00every time.repair-speaker-profilesdeduped on(meeting, id, channel)and kept theoldest entry, so after a re-diarization it deleted the prototype
describing the meeting as it is now and kept the superseded one. That pair
only exists because confirmations became run-scoped — the repair command undid
the guarantee this change was made for.
Nothing is deleted. A superseded prototype is still genuine voice evidence of a
real person and keeps scoring candidates everywhere; what it loses is the right
to claim a cluster.
suggest-speakersreports the orphaned assignments so thepanel can say so instead of just dropping the names.
Accepted trade, stated rather than hidden: with run-scoped removal, a wrong
old-run confirmation can no longer be corrected by re-confirming. The remedies
are deleting the person or
repair-speaker-profiles. Silently destroyinggenuine evidence is the worse failure, and it is the one happening today.
2. "Keep generic" died on unmount
The review panel's "Keep generic" only added the row key to a React state set.
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, a
set-cluster-review-stateCLI mirroring
mark-speaker-cluster, and an echo insuggest-speakers. The rowstays visible and reads as parked rather than disappearing: persisted, a hidden
row would put its own undo somewhere nobody can reach.
A re-diarization drops these markings, correctly — the new run's ids describe
nothing. It no longer drops them silently:
backfill-speaker-embeddingsalready reported the mixed markings and now reports these too, and
reprocess --retranscribe, which reported neither, gained the same accounting.Backward compatibility
Every new key is optional and absent means "behaves exactly as today". No
migration, no backfill. The e2e fixture
writeSpeakersSidecarstayslegacy-shaped on purpose —
speaker-naming.t2andspeaker-multi-marking.t2passing against it unchanged is the compatibility proof.
Verification
1020 Python tests, 171 vitest, T1 88 passed, model-free T2 102 passed, typecheck
clean, lint and ruff at baseline. Every new guard was mutation-checked
individually. Reviewed per task by a cross-family reviewer and once more over
the whole branch diff — the whole-branch pass found a UI contradiction the
per-task passes could not see (a parked row offering three ways to name it),
fixed in this branch.
Summary by cubic
Stamped diarization runs and scoped speaker evidence to the run to stop re-diarization from silently reassigning people; also persisted the “Keep generic” review state so the panel remembers decisions across remounts and restarts. Sidecar writes are now flushed to disk before rename to prevent data loss.
Bug Fixes
suggest-speakers,speaker-naming-status,backfill-participants --relabel-transcripts).confirm-speaker,mark-speaker-cluster,delete-person-profile);repair-speaker-profilestreats runs separately.suggest-speakersreturnsstale_assignmentsto surface confirmations orphaned by re-diarization; UI shows a meeting-level notice.reprocess --retranscribeandbackfill-speaker-embeddingsreport discarded review markings (mixed and kept-generic) when ids reset._speakers.jsonafter a crash.New Features
diarization_run.run_id; confirmations and hard negatives are stamped, and one shared predicate decides if evidence is current.review_state: "generic"viaset-cluster-review-stateinsimple_recorder.py, exposed over IPC and echoed bysuggest-speakers.setClusterReviewState;SpeakerSuggestionincludesreview_state; mutation invalidates only that meeting’s suggestions.Written for commit e803724. Summary will update on new commits.