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..f4746273 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,68 @@ 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 +292,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 +365,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 +450,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 && (

{ 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 ?? []; const isExpanded = expanded.has(key); // Expanding is only worth offering when there is more than the @@ -423,7 +511,7 @@ export function SpeakerReviewPanel({ summaryFile, isDiarised }: SpeakerReviewPan

{suggestionLabel(row.suggestion)} @@ -434,6 +522,14 @@ export function SpeakerReviewPanel({ summaryFile, isDiarised }: SpeakerReviewPan Left out of naming and voice recognition. + ) : isKept ? ( + + Kept generic — you decided not to name this speaker. + ) : ( row.suggestion.sample_text && ( {/* Hidden once confirmed_by_user is set -- re-approving an already-confirmed cluster is a no-op that changes @@ -638,17 +734,34 @@ export function SpeakerReviewPanel({ summaryFile, isDiarised }: SpeakerReviewPan {isMarked ? : } {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..942f447e --- /dev/null +++ b/app/renderer/src/components/speakerReviewState.test.ts @@ -0,0 +1,118 @@ +import { describe, expect, it } from 'vitest'; + +import { + isKeptGeneric, + showsKeepGenericButton, + showsNamingActions, + 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('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(); + 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/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..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 @@ -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.** --- @@ -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.** --- @@ -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: 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 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.** --- @@ -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.** --- @@ -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.** --- 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. 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); +}); diff --git a/e2e/specs/speaker-review.t1.spec.ts b/e2e/specs/speaker-review.t1.spec.ts index b2e34dbe..0b7ff2bf 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,70 @@ 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'); + // 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 ({ + 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 }) => { diff --git a/simple_recorder.py b/simple_recorder.py index dd816090..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"), @@ -4918,10 +4938,12 @@ 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, prototype_channel_matches, + prototype_run_matches, read_speakers_sidecar, record_original_labels, relabel_transcript_exact, @@ -4942,6 +4964,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({ @@ -5006,12 +5035,29 @@ 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 more than a stale positive prototype left standing: a + # confirmation made against a superseded run can no longer be corrected + # 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( 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 @@ -5023,9 +5069,17 @@ 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. + # + # 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 [] @@ -5034,7 +5088,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"]: @@ -5042,7 +5096,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( @@ -5052,7 +5106,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 @@ -5089,12 +5143,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 = [] @@ -5107,11 +5161,21 @@ 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. + # + # 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 @@ -5127,7 +5191,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 +5204,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"]) @@ -5171,6 +5235,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. @@ -5230,6 +5300,82 @@ 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): + # 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: + 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') @@ -5263,6 +5409,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, @@ -5289,9 +5436,20 @@ 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") - clusters, id_resolution = merge_same_channel_fragments( - clusters_from_sidecar_channel(meeting_stem, channel_data) - ) + # 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") + 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: @@ -5316,11 +5474,20 @@ 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. 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 @@ -5334,7 +5501,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"]: @@ -5342,7 +5509,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 @@ -5420,6 +5587,7 @@ def speaker_naming_status(meeting_stem): merge_same_channel_fragments, clusters_from_sidecar_channel, prototype_channel_matches, + prototype_run_matches, read_speakers_sidecar, ) @@ -5432,6 +5600,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 @@ -5458,6 +5633,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 @@ -5495,6 +5671,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, @@ -5530,6 +5707,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 @@ -5545,6 +5730,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] @@ -5578,22 +5768,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, @@ -5645,6 +5856,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 @@ -5668,6 +5885,25 @@ 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. + # + # 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() + ], "channels": channels_out, })) @@ -5966,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, ) @@ -6010,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: @@ -6055,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: @@ -6089,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), })) @@ -6115,6 +6366,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, @@ -6157,6 +6409,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(): @@ -6170,6 +6428,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 @@ -6374,7 +6641,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() @@ -6399,11 +6666,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") @@ -6418,6 +6692,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() @@ -6432,7 +6711,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 @@ -6464,6 +6747,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/src/config.py b/src/config.py index 83a21a01..96e834ac 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] @@ -1218,6 +1245,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 +1264,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 +1299,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() @@ -1278,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 @@ -1294,8 +1333,39 @@ 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. 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 + from src.speaker_suggestions import prototype_channel_matches, prototype_run_matches profile = self.get_person_profile(person_id) if profile is None: @@ -1308,6 +1378,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/src/speaker_suggestions.py b/src/speaker_suggestions.py index e43b8592..91207fc3 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 @@ -160,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: @@ -217,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 @@ -546,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, + ), ), ) @@ -582,11 +631,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 @@ -616,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.""" @@ -626,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, @@ -639,34 +755,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: @@ -678,6 +780,124 @@ 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. + + 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 not isinstance(sidecar, dict): + return None, 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], +) -> 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 = _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: + 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 + # 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) + 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. @@ -699,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 @@ -790,6 +1036,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 @@ -1305,6 +1552,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_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_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() diff --git a/tests/test_config.py b/tests/test_config.py index 26caddcc..1fd628b8 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") @@ -1016,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): @@ -1075,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 b68c3b4e..cf7f4014 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,289 @@ 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 _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_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 + # 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 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 + # 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 @@ -285,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_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") diff --git a/tests/test_speaker_multi_marking.py b/tests/test_speaker_multi_marking.py index d29fcbb1..6f1e4761 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, @@ -1048,6 +1051,119 @@ 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 _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) @@ -1074,17 +1190,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 +1305,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 +1336,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, @@ -1405,6 +1525,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) @@ -1423,11 +1546,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. @@ -1579,3 +1728,256 @@ 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")) + + 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"]) + + +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, []) diff --git a/tests/test_speaker_suggestions.py b/tests/test_speaker_suggestions.py index 60fac8fb..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 @@ -6,6 +7,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 +17,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, @@ -22,13 +26,17 @@ longest_segment, merge_same_channel_fragments, prototype_channel_matches, + prototype_run_matches, read_speakers_sidecar, relabel_transcript_exact, relabel_transcript_multi, relabel_transcript_speaker, score_candidates, + set_cluster_multi_speaker, + set_cluster_review_state, suggest_speaker, suggest_speakers_for_meeting, + write_sidecar_document, write_speakers_sidecar, ) from src.voiceprint import cosine_distance @@ -383,6 +391,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 @@ -572,6 +597,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", @@ -1277,5 +1374,204 @@ 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() + + +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}") diff --git a/tests/test_suggest_speakers_cli.py b/tests/test_suggest_speakers_cli.py index eb3c93a8..af5b30f3 100644 --- a/tests/test_suggest_speakers_cli.py +++ b/tests/test_suggest_speakers_cli.py @@ -8,7 +8,13 @@ import simple_recorder from src.config import Config -from src.speaker_suggestions import write_speakers_sidecar +from src.speaker_suggestions import ( + REVIEW_STATE_GENERIC, + read_speakers_sidecar, + set_cluster_review_state, + write_sidecar_document, + write_speakers_sidecar, +) def _last_json(output): @@ -16,6 +22,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 +150,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 +200,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 +232,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 +506,204 @@ 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. + + 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()