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'));