From a4c88f8bde8de9ae5457dbfb635a23009bd44a8e Mon Sep 17 00:00:00 2001 From: "B.D. Murphy" <161863146+bdmurphy73@users.noreply.github.com> Date: Sat, 10 Oct 2026 18:46:07 -0700 Subject: [PATCH] Make speaker approval actually clear the badge, without navigating Reviewer report: pressing Approve left the "needs check" badge in place and appeared to navigate to a new page. Root cause was a split brain, not a single bug. Approve writes the voice_status.json ledger, but speaker_confidence read the voice_status key embedded in speakers.json. That inline copy is stale - the web layer overwrites it from the ledger on read (_load_speakers), so the page showed the right value while the assessment computed from the old one. In Last Dwarf, "Advisor" was 'pending' in speakers.json and 'approved' in the ledger, so the reviewer's click had saved and still left it flagged. - assess_speakers() takes the ledger as an explicit argument, authoritative over the inline key; load_speaker_assessments() reads voice_status.json. - New SpeakerAssessment.reviewed flag, surfaced in the serialized view. - Approval clears the badge but keeps the reasons visible: the underlying facts (role-only name, one utterance) stay inspectable on hover rather than being silently discarded. - Corrupt/ non-dict ledgers degrade to {} - this module never raises. - Approve swapped #speakers-list with innerHTML, replacing the whole reviewer list with a single ; now swaps only its own row via outerHTML on #speaker-row-N. - The review panel explained only rename/merge and never mentioned Approve, so the process was unclear; it now lists the three outcomes. 16 new tests. Suite: 681 passed, 8 pre-existing failures unchanged. --- tests/test_speaker_confidence_approval.py | 162 +++++++++++++++++++ umbraaudio/analysis/speaker_confidence.py | 48 +++++- umbraaudio/web/templates/_speakers_list.html | 13 +- umbraaudio/web/templates/speakers.html | 11 +- 4 files changed, 228 insertions(+), 6 deletions(-) create mode 100644 tests/test_speaker_confidence_approval.py diff --git a/tests/test_speaker_confidence_approval.py b/tests/test_speaker_confidence_approval.py new file mode 100644 index 0000000..e0a741c --- /dev/null +++ b/tests/test_speaker_confidence_approval.py @@ -0,0 +1,162 @@ +"""Tests for the reviewer-facing behaviour of speaker identification confidence. + +The specific regression: a reviewer pressed Approve on a speaker, the badge +stayed, and the button looked broken. ``speaker_confidence`` never read +``voice_status``, so no amount of approving could clear it. Its own docstring +claimed an approved voice was a positive signal, so this restores the stated +behaviour rather than adding a new one. +""" +from __future__ import annotations + +import json +from pathlib import Path + +from umbraaudio.analysis.speaker_confidence import ( + assess_speakers, + load_speaker_assessments, +) + +CLEAR = "clear" +NEEDS_REVIEW = "needs_review" + +# A speaker the heuristic will always doubt: a role-only name, no dialogue. +ROLE_ONLY = { + "primary_name": "Advisor", + "demographics": {}, + "vocal_traits": [], + "description": "Created from unresolved name token 'Advisor'", + "aliases": ["Advisor"], +} + + +def _stats(**over) -> dict: + base = { + "Advisor": {"utterances": 1, "needs_review": 0, "mean_confidence": 0.5} + } + base.update(over) + return base + + +def test_role_only_speaker_is_flagged_before_approval() -> None: + out = assess_speakers([dict(ROLE_ONLY)], _stats()) + assert out[0].status == NEEDS_REVIEW + assert out[0].reviewed is False + + +def test_approval_clears_the_badge() -> None: + """The bug: Approve recorded a decision the assessment ignored.""" + approved = dict(ROLE_ONLY, voice_status="approved") + out = assess_speakers([approved], _stats()) + assert out[0].status == CLEAR + assert out[0].reviewed is True + + +def test_approval_keeps_the_reasons_visible() -> None: + """Clearing the badge must not discard the underlying facts.""" + approved = dict(ROLE_ONLY, voice_status="approved") + out = assess_speakers([approved], _stats()) + assert out[0].reasons, "reasons should survive an approval" + assert any("utterance" in r for r in out[0].reasons) + + +def test_approval_is_matched_case_insensitively() -> None: + approved = dict(ROLE_ONLY, voice_status="Approved") + assert assess_speakers([approved], _stats())[0].status == CLEAR + + +def test_pending_voice_status_does_not_clear() -> None: + pending = dict(ROLE_ONLY, voice_status="pending") + assert assess_speakers([pending], _stats())[0].status == NEEDS_REVIEW + + +def test_rejected_voice_status_does_not_clear() -> None: + rejected = dict(ROLE_ONLY, voice_status="rejected") + assert assess_speakers([rejected], _stats())[0].status == NEEDS_REVIEW + + +def test_unreviewed_speaker_is_unaffected() -> None: + """Sanity: speakers without a voice_status behave exactly as before.""" + out = assess_speakers([dict(ROLE_ONLY)], _stats()) + assert out[0].status == NEEDS_REVIEW + + +def test_already_clear_speaker_stays_clear_after_approval() -> None: + solid = { + "primary_name": "Adric", + "demographics": {"gender": "male"}, + "vocal_traits": ["measured"], + "description": "The palace factor", + "aliases": [], + } + stats = {"Adric": {"utterances": 400, "needs_review": 0, "mean_confidence": 0.9}} + out = assess_speakers([dict(solid, voice_status="approved")], stats) + assert out[0].status == CLEAR + assert out[0].reviewed is True + + +# ------------------------------------------------------------- via the loader + + +def test_load_speaker_assessments_exposes_reviewed(tmp_path: Path) -> None: + (tmp_path / "speakers.json").write_text( + json.dumps({"speakers": [dict(ROLE_ONLY, voice_status="approved")]}) + ) + got = load_speaker_assessments(tmp_path) + assert got["Advisor"].reviewed is True + assert got["Advisor"].status == CLEAR + + +def test_missing_speakers_file_returns_empty(tmp_path: Path) -> None: + assert load_speaker_assessments(tmp_path) == {} + + +def test_ledger_overrides_stale_speakers_json(tmp_path: Path) -> None: + """The exact "Advisor" regression: two sources of truth disagreed. + + The Approve button writes ``voice_status.json``. The speaker records in + ``speakers.json`` kept a stale inline ``voice_status`` that the web layer + overwrites on read. Reading the stale copy left an approved speaker flagged + even though the reviewer had accepted it. + """ + (tmp_path / "speakers.json").write_text( + json.dumps({"speakers": [dict(ROLE_ONLY, voice_status="pending")]}) + ) + (tmp_path / "voice_status.json").write_text(json.dumps({"Advisor": "approved"})) + + got = load_speaker_assessments(tmp_path)["Advisor"] + assert got.status == CLEAR, "ledger approval must win over the stale inline copy" + assert got.reviewed is True + + +def test_ledger_pending_does_not_clear(tmp_path: Path) -> None: + (tmp_path / "speakers.json").write_text( + json.dumps({"speakers": [dict(ROLE_ONLY, voice_status="approved")]}) + ) + (tmp_path / "voice_status.json").write_text(json.dumps({"Advisor": "pending"})) + assert load_speaker_assessments(tmp_path)["Advisor"].status == NEEDS_REVIEW + + +def test_corrupt_ledger_does_not_break_the_page(tmp_path: Path) -> None: + (tmp_path / "speakers.json").write_text(json.dumps({"speakers": [ROLE_ONLY]})) + (tmp_path / "voice_status.json").write_text("{not json") + got = load_speaker_assessments(tmp_path) + assert got["Advisor"].status == NEEDS_REVIEW + + +def test_ledger_that_is_a_list_is_ignored(tmp_path: Path) -> None: + (tmp_path / "speakers.json").write_text(json.dumps({"speakers": [ROLE_ONLY]})) + (tmp_path / "voice_status.json").write_text('["approved"]') + assert load_speaker_assessments(tmp_path)["Advisor"].status == NEEDS_REVIEW + + +def test_assess_speakers_accepts_ledger_directly() -> None: + out = assess_speakers([dict(ROLE_ONLY)], _stats(), {"Advisor": "approved"}) + assert out[0].status == CLEAR + + +def test_absent_ledger_key_falls_back_to_inline() -> None: + """No ledger entry for this speaker: the inline value still applies.""" + out = assess_speakers( + [dict(ROLE_ONLY, voice_status="approved")], _stats(), {"Someone Else": "approved"} + ) + assert out[0].status == CLEAR diff --git a/umbraaudio/analysis/speaker_confidence.py b/umbraaudio/analysis/speaker_confidence.py index 9f2fc6d..87aa5ef 100644 --- a/umbraaudio/analysis/speaker_confidence.py +++ b/umbraaudio/analysis/speaker_confidence.py @@ -72,6 +72,9 @@ class SpeakerAssessment: confidence: float = 1.0 status: str = "clear" # "clear" | "needs_review" reasons: list[str] = field(default_factory=list) + # True once a human has accepted this speaker's voice. The heuristic can + # still list reasons; approval is what lifts the badge. + reviewed: bool = False def to_dict(self) -> dict[str, Any]: return { @@ -79,6 +82,7 @@ class SpeakerAssessment: "identification_confidence": round(self.confidence, 3), "review_status": self.status, "review_reasons": self.reasons, + "reviewed": self.reviewed, } @@ -155,6 +159,7 @@ def _near_duplicate(name: str, others: list[str]) -> str | None: def assess_speakers( speakers: list[dict[str, Any]], utterance_stats: dict[str, dict[str, Any]] | None = None, + approvals: dict[str, Any] | None = None, ) -> list[SpeakerAssessment]: """Compute an identification confidence + designation for every speaker. @@ -162,6 +167,12 @@ def assess_speakers( ``{"utterances": int, "needs_review": int, "mean_confidence": float|None}``. When omitted, only name/profile signals are used. + ``approvals`` is the ``voice_status.json`` ledger - the map the Approve + button actually writes. It is authoritative over any ``voice_status`` key + already sitting in ``speakers.json``: the speaker records carry a stale + inline copy that the web layer overwrites on read, so trusting it here made + an approved speaker keep its warning badge forever. + Some defects are *categorical* rather than a matter of degree: a name with a narration verb glued on ("Walking oldest"), an internal "(Scene 2)" label, an uncapitalised name, a probable unmerged duplicate, or a speaker with no @@ -170,6 +181,7 @@ def assess_speakers( decide on (rename / merge / confirm). """ stats = utterance_stats or {} + approvals = approvals or {} names = [s.get("primary_name", "") for s in speakers if s.get("primary_name")] out: list[SpeakerAssessment] = [] @@ -246,6 +258,19 @@ def assess_speakers( status = "clear" if blocking or confidence < CLEAR_THRESHOLD: status = "needs_review" + + # A human decision outranks the heuristic. The docstring already claimed + # an approved voice was a positive signal, but nothing read voice_status - + # so clicking Approve could never clear the badge, which made the button + # look broken. Approval records "a person looked at this speaker and + # accepted it"; the reasons are kept on the assessment so the underlying + # facts (one utterance, no alias evidence) remain visible rather than + # being silently discarded. + raw = approvals.get(name, spk.get("voice_status")) + approved = str(raw or "").strip().lower() == "approved" + if approved and status == "needs_review": + status = "clear" + out.append( SpeakerAssessment( name=name, @@ -253,6 +278,7 @@ def assess_speakers( status=status, # de-dupe, preserve order reasons=list(dict.fromkeys(reasons)), + reviewed=approved, ) ) @@ -283,6 +309,25 @@ def _utterance_stats(db_path: Path) -> dict[str, dict[str, Any]]: } +def _load_voice_ledger(data_dir: Path) -> dict[str, Any]: + """The ``voice_status.json`` approval ledger written by the Approve button. + + Never raises: a missing or corrupt ledger yields ``{}`` so the speakers page + still renders, matching this module's contract everywhere else. + """ + import json + + path = data_dir / "voice_status.json" + if not path.exists(): + return {} + try: + with open(path) as f: + data = json.load(f) + except Exception: + return {} + return data if isinstance(data, dict) else {} + + def load_speaker_assessments(data_dir: Path) -> dict[str, SpeakerAssessment]: """Assess every speaker in a project; returns a name -> assessment map. @@ -300,4 +345,5 @@ def load_speaker_assessments(data_dir: Path) -> dict[str, SpeakerAssessment]: except Exception: return {} stats = _utterance_stats(data_dir / "project.db") - return {a.name: a for a in assess_speakers(speakers, stats)} + approvals = _load_voice_ledger(data_dir) + return {a.name: a for a in assess_speakers(speakers, stats, approvals)} diff --git a/umbraaudio/web/templates/_speakers_list.html b/umbraaudio/web/templates/_speakers_list.html index ae83a5b..5653e15 100644 --- a/umbraaudio/web/templates/_speakers_list.html +++ b/umbraaudio/web/templates/_speakers_list.html @@ -26,8 +26,17 @@ {% if _needs %}
⚠ {{ _needs }} speaker{{ 's' if _needs != 1 }} need{{ '' if _needs != 1 }} checking - — the name or identity looks wrong, so it was not auto-accepted. Hover the - ⚠ needs check badge on a row for the reason. Fix with rename / merge, then re-scan Name Tokens. + — these were not auto-accepted as distinct speakers. For each one: +
    +
  1. Hover the ⚠ needs check badge on the row to see + why — e.g. named only by a role, no dialogue attributed, or a likely duplicate.
    2. +
  2. If the speaker is correct as-is (a real character, name is fine) — press + ✓ Approve on its row. + That records your decision and clears the badge immediately; the page does not reload.
    3. +
  3. If the speaker is wrong — use Edit to correct it, or Merge + it into an existing speaker, then re-scan Name Tokens.
    4. +
+ Hover a row's badge for the full reason list.
Show the list
diff --git a/umbraaudio/web/templates/speakers.html b/umbraaudio/web/templates/speakers.html index 9b2e646..6c9bb2c 100644 --- a/umbraaudio/web/templates/speakers.html +++ b/umbraaudio/web/templates/speakers.html @@ -176,9 +176,14 @@ function approveSpeakerVoice(name, idx) { body: JSON.stringify({speaker_name: name, status: 'approved', distortion_effects: effects}) }).then(r => r.json()).then(data => { if (data.ok) { - htmx.ajax('GET', '/speakers/partial-row/' + window._speakerNames.indexOf(name), { - target: '#speakers-list', - swap: 'innerHTML' + // Swap ONLY this row. The old code targeted #speakers-list with an + // innerHTML swap, which replaced the entire reviewer list with a + // single - that is what read as navigating away to a new page. + var pos = window._speakerNames ? window._speakerNames.indexOf(name) : -1; + var rowIdx = (pos >= 0) ? pos : idx; + htmx.ajax('GET', '/speakers/partial-row/' + rowIdx, { + target: '#speaker-row-' + rowIdx, + swap: 'outerHTML' }); } else { alert('Failed: ' + (data.error || 'unknown'));