From 7e6c52bf4a9bc015bb4f4e43e4527ea3a11e939b Mon Sep 17 00:00:00 2001 From: Lvigentini Date: Mon, 10 Aug 2026 02:18:04 +1000 Subject: [PATCH 1/3] feat(tts): add a pronunciation dictionary Names, acronyms, brands and loanwords come out wrong and there is no reusable way to fix them (#827) -- you edit the text every time, in every script. Adds a term -> respelling map applied just before TTS. Respelling rather than phonemes on purpose: every engine reads plain text, so `bandeja -> ban-DEH-ha` works on all of them, where a phoneme string only works on the engines that accept one. Cruder, portable. Entries are global by default and can be scoped to a language, a voice, or both. Language scope is the one that earns its keep for mixed-language work: a Spanish term needs respelling while the engine is reading English and must be left alone when it is already reading Spanish. A profile-scoped entry beats a global one; a language-specific entry beats a wildcard. Applied at generation time, not when the text is saved. `generations.text` keeps what the author wrote, so History stays readable and editing an entry changes future audio without rewriting the past. `POST /pronunciations/preview` exists because of that -- the rewritten string is never stored, so without it there is no way to see what a rule does short of listening. Matching is a single pass over one alternation of all terms, longest first. That is what stops replacements cascading: with `bandeja -> ban-DEH-ha` and `ha -> hah`, a loop of per-term substitutions produces `ban-DEH-hah`. It also lets a multi-word entry beat the single-word entry inside it. Word boundaries use lookarounds so terms with punctuation still anchor, terms are escaped so a term is text and not a pattern, and `[laugh]`-style tags are skipped because they are engine syntax rather than speech. Capitalisation carries onto the replacement, counting cased characters rather than `str.isupper()` -- that returns True for `C++`, and shouting the replacement would turn `C plus plus` into `C PLUS PLUS`. Duplicate scopes are rejected in the service rather than by a unique constraint, since SQL treats NULLs as distinct and would accept two global entries for the same term. Applies on both `/generate` and `/generate/stream` so a streamed preview matches what the persisted path produces. 28 tests covering matching, the no-cascade rule, capitalisation, scope resolution, degenerate input, CRUD, and the property the design rests on: the engine receives the respelling and the stored row does not. Closes #827 Co-Authored-By: Claude Opus 5 (1M context) --- backend/database/__init__.py | 2 + backend/database/models.py | 25 ++ backend/models.py | 71 ++++++ backend/routes/__init__.py | 2 + backend/routes/generations.py | 10 +- backend/routes/pronunciation.py | 143 +++++++++++ backend/services/generation.py | 10 +- backend/services/pronunciation.py | 183 ++++++++++++++ backend/tests/test_pronunciation.py | 365 ++++++++++++++++++++++++++++ 9 files changed, 808 insertions(+), 3 deletions(-) create mode 100644 backend/routes/pronunciation.py create mode 100644 backend/services/pronunciation.py create mode 100644 backend/tests/test_pronunciation.py diff --git a/backend/database/__init__.py b/backend/database/__init__.py index fd1252bf4..5df700754 100644 --- a/backend/database/__init__.py +++ b/backend/database/__init__.py @@ -20,6 +20,7 @@ ProfileChannelMapping, ProfileSample, Project, + PronunciationEntry, Story, StoryItem, VoiceProfile, @@ -41,6 +42,7 @@ "MCPClientBinding", "ProfileChannelMapping", "ProfileSample", + "PronunciationEntry", "Project", "Story", "StoryItem", diff --git a/backend/database/models.py b/backend/database/models.py index b85a55b17..5fc2650e6 100644 --- a/backend/database/models.py +++ b/backend/database/models.py @@ -141,6 +141,31 @@ class GenerationVersion(Base): created_at = Column(DateTime, default=datetime.utcnow) +class PronunciationEntry(Base): + """A term the engine says wrong, and how to spell it so it says it right. + + Respelling rather than phonemes: every engine reads plain text, so + ``bandeja -> ban-DEH-ha`` works everywhere, where a phoneme string only + works on the engines that accept one. + """ + + __tablename__ = "pronunciation_entries" + + id = Column(String, primary_key=True, default=lambda: str(uuid.uuid4())) + term = Column(String, nullable=False) + replacement = Column(String, nullable=False) + # NULL applies in every generation language. A code restricts the entry to + # that language -- a Spanish word needs respelling when the engine is + # reading English, but not when it is already reading Spanish. + language = Column(String, nullable=True) + # NULL is a global entry; set to scope it to one voice. + profile_id = Column(String, ForeignKey("profiles.id"), nullable=True) + enabled = Column(Boolean, default=True, nullable=False) + notes = Column(Text, nullable=True) + created_at = Column(DateTime, default=datetime.utcnow) + updated_at = Column(DateTime, default=datetime.utcnow, onupdate=datetime.utcnow) + + class EffectPreset(Base): """Saved effect chain preset.""" diff --git a/backend/models.py b/backend/models.py index 7970ce41e..1b620aaf2 100644 --- a/backend/models.py +++ b/backend/models.py @@ -127,6 +127,77 @@ class Config: from_attributes = True +class PronunciationEntryCreate(BaseModel): + """Request model for creating a pronunciation entry.""" + + term: str = Field(..., min_length=1, max_length=200) + replacement: str = Field(..., min_length=1, max_length=500) + language: Optional[str] = Field( + None, + pattern="^(zh|en|ja|ko|de|fr|ru|pt|es|it|he|ar|da|el|fi|hi|ms|nl|no|pl|sv|sw|tr)$", + description="Apply only when generating in this language. Omit for all languages.", + ) + profile_id: Optional[str] = Field( + None, description="Scope to one voice. Omit for a global entry." + ) + enabled: bool = True + notes: Optional[str] = Field(None, max_length=1000) + + +class PronunciationEntryUpdate(BaseModel): + """Request model for updating a pronunciation entry. Omitted fields are left alone.""" + + term: Optional[str] = Field(None, min_length=1, max_length=200) + replacement: Optional[str] = Field(None, min_length=1, max_length=500) + language: Optional[str] = Field( + None, pattern="^(zh|en|ja|ko|de|fr|ru|pt|es|it|he|ar|da|el|fi|hi|ms|nl|no|pl|sv|sw|tr)$" + ) + profile_id: Optional[str] = None + enabled: Optional[bool] = None + notes: Optional[str] = Field(None, max_length=1000) + + +class PronunciationEntryResponse(BaseModel): + """Response model for a pronunciation entry.""" + + id: str + term: str + replacement: str + language: Optional[str] = None + profile_id: Optional[str] = None + enabled: bool = True + notes: Optional[str] = None + created_at: datetime + updated_at: datetime + + class Config: + from_attributes = True + + +class PronunciationPreviewRequest(BaseModel): + """Request to see what the dictionary would do to a piece of text.""" + + text: str = Field(..., min_length=1, max_length=50000) + language: Optional[str] = None + profile_id: Optional[str] = None + + +class PronunciationSubstitution(BaseModel): + """One replacement the dictionary made.""" + + term: str + replacement: str + entry_id: str + + +class PronunciationPreviewResponse(BaseModel): + """What the engine would actually be given, and why it differs.""" + + original: str + result: str + applied: List[PronunciationSubstitution] + + class HistoryQuery(BaseModel): """Query model for generation history.""" diff --git a/backend/routes/__init__.py b/backend/routes/__init__.py index 42999d2d1..1df33f527 100644 --- a/backend/routes/__init__.py +++ b/backend/routes/__init__.py @@ -15,6 +15,7 @@ def register_routers(app: FastAPI) -> None: from .captures import router as captures_router from .stories import router as stories_router from .effects import router as effects_router + from .pronunciation import router as pronunciation_router from .audio import router as audio_router from .models import router as models_router from .settings import router as settings_router @@ -36,6 +37,7 @@ def register_routers(app: FastAPI) -> None: app.include_router(captures_router) app.include_router(stories_router) app.include_router(effects_router) + app.include_router(pronunciation_router) app.include_router(audio_router) app.include_router(models_router) app.include_router(settings_router) diff --git a/backend/routes/generations.py b/backend/routes/generations.py index fbbeece67..1491b99a4 100644 --- a/backend/routes/generations.py +++ b/backend/routes/generations.py @@ -10,7 +10,7 @@ from sqlalchemy.orm import Session from .. import config, models -from ..services import history, personality, profiles, tts +from ..services import history, personality, profiles, pronunciation, tts from ..database import Generation as DBGeneration, VoiceProfile as DBVoiceProfile, get_db from ..services.generation import run_generation from ..services.task_queue import cancel_generation as cancel_generation_job, enqueue_generation @@ -363,9 +363,15 @@ async def stream_speech( runaway_detector = has_tts_runaway + # Same respelling the persisted path does, so a streamed preview matches + # what /generate would produce. + stream_text, _applied = pronunciation.apply_pronunciations( + data.text, data.language, db, profile_id=data.profile_id + ) + audio, sample_rate = await generate_chunked( tts_model, - data.text, + stream_text, voice_prompt, language=data.language, seed=data.seed, diff --git a/backend/routes/pronunciation.py b/backend/routes/pronunciation.py new file mode 100644 index 000000000..d29da95b3 --- /dev/null +++ b/backend/routes/pronunciation.py @@ -0,0 +1,143 @@ +"""Pronunciation dictionary endpoints.""" + +import logging + +from fastapi import APIRouter, Depends, HTTPException, Query +from sqlalchemy.orm import Session + +from .. import models +from ..database import PronunciationEntry, VoiceProfile as DBVoiceProfile, get_db +from ..services import pronunciation + +logger = logging.getLogger(__name__) + +router = APIRouter() + + +def _validate_profile(profile_id: str | None, db: Session) -> None: + if profile_id is None: + return + if db.query(DBVoiceProfile).filter_by(id=profile_id).first() is None: + raise HTTPException(status_code=404, detail=f"Profile '{profile_id}' not found") + + +@router.get("/pronunciations", response_model=list[models.PronunciationEntryResponse]) +async def list_pronunciations( + language: str | None = Query(None, description="Filter to entries that apply to this language"), + profile_id: str | None = Query(None, description="Filter to entries that apply to this voice"), + include_disabled: bool = Query(True), + db: Session = Depends(get_db), +): + """List dictionary entries. + + With no filters this returns everything, which is what a management screen + wants. Passing ``language`` or ``profile_id`` narrows it to what would + actually apply to a generation with those settings. + """ + if language is None and profile_id is None: + q = db.query(PronunciationEntry) + if not include_disabled: + q = q.filter(PronunciationEntry.enabled.is_(True)) + return q.order_by(PronunciationEntry.term).all() + + return pronunciation.get_entries( + db, language=language, profile_id=profile_id, include_disabled=include_disabled + ) + + +@router.post("/pronunciations", response_model=models.PronunciationEntryResponse) +async def create_pronunciation( + data: models.PronunciationEntryCreate, + db: Session = Depends(get_db), +): + """Add a term and how to say it.""" + _validate_profile(data.profile_id, db) + + existing = pronunciation.find_duplicate(db, data.term, data.language, data.profile_id) + if existing is not None: + raise HTTPException( + status_code=409, + detail=( + f"An entry for '{data.term}' already exists in this scope " + f"(id {existing.id}). Update it instead." + ), + ) + + entry = PronunciationEntry( + term=data.term.strip(), + replacement=data.replacement.strip(), + language=data.language, + profile_id=data.profile_id, + enabled=data.enabled, + notes=data.notes, + ) + db.add(entry) + db.commit() + db.refresh(entry) + return entry + + +@router.put("/pronunciations/{entry_id}", response_model=models.PronunciationEntryResponse) +async def update_pronunciation( + entry_id: str, + data: models.PronunciationEntryUpdate, + db: Session = Depends(get_db), +): + """Update an entry. Omitted fields are left as they are.""" + entry = db.query(PronunciationEntry).filter_by(id=entry_id).first() + if entry is None: + raise HTTPException(status_code=404, detail="Pronunciation entry not found") + + fields = data.model_dump(exclude_unset=True) + if "profile_id" in fields: + _validate_profile(fields["profile_id"], db) + + # Re-check the scope only when something that defines it moved. + if {"term", "language", "profile_id"} & fields.keys(): + clash = pronunciation.find_duplicate( + db, + fields.get("term", entry.term), + fields.get("language", entry.language), + fields.get("profile_id", entry.profile_id), + exclude_id=entry_id, + ) + if clash is not None: + raise HTTPException( + status_code=409, + detail=f"That scope already has an entry for this term (id {clash.id}).", + ) + + for key, value in fields.items(): + setattr(entry, key, value.strip() if key in {"term", "replacement"} and value else value) + + db.commit() + db.refresh(entry) + return entry + + +@router.delete("/pronunciations/{entry_id}") +async def delete_pronunciation(entry_id: str, db: Session = Depends(get_db)): + """Delete an entry.""" + entry = db.query(PronunciationEntry).filter_by(id=entry_id).first() + if entry is None: + raise HTTPException(status_code=404, detail="Pronunciation entry not found") + db.delete(entry) + db.commit() + return {"message": "Pronunciation entry deleted"} + + +@router.post("/pronunciations/preview", response_model=models.PronunciationPreviewResponse) +async def preview_pronunciations( + data: models.PronunciationPreviewRequest, + db: Session = Depends(get_db), +): + """Show what the engine would be given for this text. + + The dictionary runs at generation time and the rewritten text is never + stored, so without this there is no way to see what a rule actually does + short of listening to the output. + """ + result, applied = pronunciation.apply_pronunciations( + data.text, data.language, db, profile_id=data.profile_id + ) + return {"original": data.text, "result": result, "applied": applied} diff --git a/backend/services/generation.py b/backend/services/generation.py index a4b2e8a3f..24d52468f 100644 --- a/backend/services/generation.py +++ b/backend/services/generation.py @@ -21,7 +21,7 @@ from typing import Literal, Optional from .. import config -from . import history, profiles +from . import history, profiles, pronunciation from ..database import get_db from ..utils.tasks import get_task_manager @@ -76,6 +76,14 @@ async def run_generation( ) await history.update_generation_status(generation_id, "generating", bg_db) + + # Respell dictionary terms on the way into the engine only. The row in + # `generations` keeps what the author wrote, so History stays readable + # and editing an entry changes future audio without rewriting the past. + text, _applied = pronunciation.apply_pronunciations( + text, language, bg_db, profile_id=profile_id + ) + trim_fn = trim_tts_output if engine_needs_trim(engine) else None runaway_detector = has_tts_runaway if engine_retries_runaway(engine) else None diff --git a/backend/services/pronunciation.py b/backend/services/pronunciation.py new file mode 100644 index 000000000..8a4e27a74 --- /dev/null +++ b/backend/services/pronunciation.py @@ -0,0 +1,183 @@ +"""Pronunciation dictionary — reusable fixes for terms the engine says wrong. + +Names, acronyms, brands and loanwords come out wrong and there is no way to +correct them short of editing the text every time (#827). This maps a term to a +respelling applied just before TTS. + +Respelling rather than phonemes on purpose: every engine reads plain text, so +``bandeja -> ban-DEH-ha`` works on all of them, where a phoneme string only +works on the engines that accept one. It is cruder and it is portable. + +Applied at generation time, not when the text is saved, so ``generations.text`` +keeps what the author wrote. Editing an entry then changes future audio without +rewriting history, and the History tab never shows a reader ``ban-DEH-ha``. +""" + +import logging +import re + +from sqlalchemy import or_ +from sqlalchemy.orm import Session + +from ..database import PronunciationEntry + +logger = logging.getLogger(__name__) + +# Paralinguistic tags like [laugh] are engine syntax, not speech. A term that +# happens to appear inside one must not be rewritten. +_TAG_RE = re.compile(r"\[[^\]]*\]") + + +def get_entries( + db: Session, + language: str | None = None, + profile_id: str | None = None, + include_disabled: bool = False, +) -> list[PronunciationEntry]: + """Entries that apply to a given language and profile. + + An entry with a NULL language or profile is a wildcard, so the filters ask + for "matches, or unset" rather than equality. + """ + q = db.query(PronunciationEntry) + if not include_disabled: + q = q.filter(PronunciationEntry.enabled.is_(True)) + if language is not None: + q = q.filter( + or_(PronunciationEntry.language.is_(None), PronunciationEntry.language == language) + ) + if profile_id is not None: + q = q.filter( + or_(PronunciationEntry.profile_id.is_(None), PronunciationEntry.profile_id == profile_id) + ) + else: + q = q.filter(PronunciationEntry.profile_id.is_(None)) + return q.all() + + +def _match_case(source: str, replacement: str) -> str: + """Carry the matched text's capitalisation onto the replacement. + + A term at the start of a sentence is capitalised there and nowhere else, so + storing one lowercase entry has to cover both. + + "All caps" counts *cased* characters, not ``str.isupper()``: that returns + True for ``C++``, which has one cased letter, and shouting the replacement + would turn ``C plus plus`` into ``C PLUS PLUS``. An acronym like ``WCAG`` + has four and is genuinely all caps. + """ + cased = [c for c in source if c.isalpha()] + if len(cased) > 1 and all(c.isupper() for c in cased): + return replacement.upper() + if source[:1].isupper(): + return replacement[:1].upper() + replacement[1:] + return replacement + + +def _tag_spans(text: str) -> list[tuple[int, int]]: + return [(m.start(), m.end()) for m in _TAG_RE.finditer(text)] + + +def build_pattern(terms: list[str]) -> re.Pattern | None: + """One alternation over every term, longest first. + + Longest-first matters twice. It lets a multi-word entry beat the + single-word entry inside it, and because this is a single pass, a + replacement can never be re-matched by another rule — so + ``bandeja -> ban-DEH-ha`` and ``ha -> hah`` cannot compound into + ``ban-DEH-hah``, which is what a loop of per-term substitutions would do. + + Lookarounds rather than ``\\b`` so terms that start or end with punctuation + still anchor on a word boundary. + """ + usable = [t for t in terms if t and t.strip()] + if not usable: + return None + ordered = sorted(set(usable), key=len, reverse=True) + alternation = "|".join(re.escape(t) for t in ordered) + return re.compile(rf"(? tuple[str, list[dict]]: + """Rewrite *text* using the dictionary. + + Returns the rewritten text and a record of what was replaced, so a caller + can log or surface it — a silent rewrite of someone's script is worse than + no rewrite at all. + """ + if not text or not text.strip(): + return text, [] + + entries = get_entries(db, language=language, profile_id=profile_id) + if not entries: + return text, [] + + # A profile-scoped entry beats a global one for the same term; a + # language-specific entry beats a wildcard. Sorting the losers first lets + # the later assignment win. + by_term: dict = {} + for e in sorted( + entries, + key=lambda e: ((e.profile_id is not None), (e.language is not None)), + ): + by_term[e.term.lower()] = e + + pattern = build_pattern([e.term for e in by_term.values()]) + if pattern is None: + return text, [] + + skip = _tag_spans(text) + applied: list[dict] = [] + + def substitute(m: re.Match) -> str: + if any(start <= m.start() < end for start, end in skip): + return m.group(0) + entry = by_term.get(m.group(0).lower()) + if entry is None: + return m.group(0) + out = _match_case(m.group(0), entry.replacement) + applied.append({"term": m.group(0), "replacement": out, "entry_id": entry.id}) + return out + + result = pattern.sub(substitute, text) + if applied: + logger.info( + "Pronunciation dictionary rewrote %d term(s): %s", + len(applied), + ", ".join(f"{a['term']}->{a['replacement']}" for a in applied[:5]), + ) + return result, applied + + +def find_duplicate( + db: Session, + term: str, + language: str | None, + profile_id: str | None, + exclude_id: str | None = None, +) -> PronunciationEntry | None: + """An existing entry for the same term in the same scope. + + Enforced here rather than as a unique constraint because SQL treats NULLs + as distinct, so a constraint would happily accept two global entries for + the same term — exactly the case worth catching. + """ + q = db.query(PronunciationEntry).filter(PronunciationEntry.term.ilike(term)) + q = ( + q.filter(PronunciationEntry.language.is_(None)) + if language is None + else q.filter(PronunciationEntry.language == language) + ) + q = ( + q.filter(PronunciationEntry.profile_id.is_(None)) + if profile_id is None + else q.filter(PronunciationEntry.profile_id == profile_id) + ) + if exclude_id: + q = q.filter(PronunciationEntry.id != exclude_id) + return q.first() diff --git a/backend/tests/test_pronunciation.py b/backend/tests/test_pronunciation.py new file mode 100644 index 000000000..43ea21a74 --- /dev/null +++ b/backend/tests/test_pronunciation.py @@ -0,0 +1,365 @@ +""" +Tests for the pronunciation dictionary (#827). + +The rules that matter are the ones that are easy to get wrong: replacements +must not cascade into each other, capitalisation has to survive, scope has to +resolve in a defined order, and the rewrite must never reach the stored text. + +Usage: + python -m pytest backend/tests/test_pronunciation.py -v +""" + +import os +import sys +import tempfile +import uuid +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).parent.parent.parent)) + +_DATA_DIR = tempfile.mkdtemp(prefix="voicebox-pronunciation-test-") +os.environ["VOICEBOX_DATA_DIR"] = _DATA_DIR + +from starlette.testclient import TestClient # noqa: E402 + +from backend.app import app # noqa: E402 +from backend.database import PronunciationEntry, get_db # noqa: E402 +from backend.services.pronunciation import apply_pronunciations, build_pattern # noqa: E402 + + +@pytest.fixture(scope="module") +def client(): + with TestClient(app) as c: + yield c + + +@pytest.fixture +def db(client): + # Depends on `client` so the app lifespan has run: SessionLocal is None + # until init_db(), and a test that only asked for `db` would get it unset. + session = next(get_db()) + try: + yield session + finally: + session.query(PronunciationEntry).delete() + session.commit() + session.close() + + +@pytest.fixture +def profile(client): + """A throwaway voice profile. + + Unique name and explicit cleanup because ``VOICEBOX_DATA_DIR`` is not + honoured on this branch (see #981/#1004) — the suite writes to the repo's + ./data, so a fixed name collides with the previous run's leftovers. + """ + name = f"Pronunciation Test Voice {uuid.uuid4().hex[:8]}" + r = client.post("/profiles", json={"name": name, "language": "en"}) + assert r.status_code == 200, r.text + created = r.json() + yield created + client.delete(f"/profiles/{created['id']}") + + +def add(db, term, replacement, language=None, profile_id=None, enabled=True): + entry = PronunciationEntry( + term=term, + replacement=replacement, + language=language, + profile_id=profile_id, + enabled=enabled, + ) + db.add(entry) + db.commit() + db.refresh(entry) + return entry + + +# ── Matching ───────────────────────────────────────────────────────── + + +def test_replaces_a_term(db): + add(db, "bandeja", "ban-DEH-ha") + out, applied = apply_pronunciations("He plays a bandeja here.", "en", db) + assert out == "He plays a ban-DEH-ha here." + assert len(applied) == 1 + + +def test_matching_is_case_insensitive_but_capitalisation_survives(db): + """One lowercase entry has to cover the term at the start of a sentence.""" + add(db, "bandeja", "ban-DEH-ha") + out, _ = apply_pronunciations("Bandeja is the shot.", "en", db) + assert out == "Ban-DEH-ha is the shot." + + +def test_all_caps_stays_all_caps(db): + add(db, "wcag", "W C A G") + out, _ = apply_pronunciations("Follow WCAG rules.", "en", db) + assert out == "Follow W C A G rules." + + +def test_only_whole_words_match(db): + """Substring matching would turn 'brandeja' into nonsense.""" + add(db, "bandeja", "ban-DEH-ha") + out, applied = apply_pronunciations("A brandejapalooza appeared.", "en", db) + assert out == "A brandejapalooza appeared." + assert applied == [] + + +def test_replacements_do_not_cascade(db): + """The single-pass alternation exists for this: a loop of per-term + substitutions would rewrite the output of an earlier rule.""" + add(db, "bandeja", "ban-DEH-ha") + add(db, "ha", "HAH") + out, applied = apply_pronunciations("The bandeja.", "en", db) + assert out == "The ban-DEH-ha." + assert len(applied) == 1 + + +def test_longer_terms_win(db): + add(db, "bandeja", "ban-DEH-ha") + add(db, "bandeja alta", "ban-DEH-ha AL-ta") + out, _ = apply_pronunciations("A bandeja alta lands deep.", "en", db) + assert out == "A ban-DEH-ha AL-ta lands deep." + + +def test_paralinguistic_tags_are_left_alone(db): + """[laugh] is engine syntax, not speech.""" + add(db, "laugh", "LAFF") + out, _ = apply_pronunciations("[laugh] I did laugh.", "en", db) + assert out == "[laugh] I did LAFF." + + +def test_accented_terms_match(db): + add(db, "víbora", "VEE-bo-ra") + out, applied = apply_pronunciations("Then the víbora.", "en", db) + assert out == "Then the VEE-bo-ra." + assert len(applied) == 1 + + +def test_reports_what_it_changed(db): + """A silent rewrite of someone's script is worse than no rewrite.""" + entry = add(db, "bandeja", "ban-DEH-ha") + _, applied = apply_pronunciations("bandeja and bandeja", "en", db) + assert len(applied) == 2 + assert all(a["entry_id"] == entry.id for a in applied) + + +# ── Scope ──────────────────────────────────────────────────────────── + + +def test_language_scoped_entry_only_applies_to_that_language(db): + """The Spanish word needs respelling when the engine reads English, and + must be left alone when it is already reading Spanish.""" + add(db, "bandeja", "ban-DEH-ha", language="en") + assert apply_pronunciations("una bandeja", "es", db)[0] == "una bandeja" + assert apply_pronunciations("a bandeja", "en", db)[0] == "a ban-DEH-ha" + + +def test_wildcard_language_applies_everywhere(db): + add(db, "bandeja", "ban-DEH-ha") + assert "ban-DEH-ha" in apply_pronunciations("una bandeja", "es", db)[0] + + +def test_disabled_entries_are_skipped(db): + add(db, "bandeja", "ban-DEH-ha", enabled=False) + assert apply_pronunciations("a bandeja", "en", db)[0] == "a bandeja" + + +def test_profile_entry_beats_global(db, profile): + add(db, "bandeja", "GLOBAL") + add(db, "bandeja", "PER-VOICE", profile_id=profile["id"]) + + out, _ = apply_pronunciations("a bandeja", "en", db, profile_id=profile["id"]) + assert out == "a PER-VOICE" + + # A different voice still gets the global entry. + assert apply_pronunciations("a bandeja", "en", db)[0] == "a GLOBAL" + + +def test_other_profiles_entries_do_not_leak(db, profile): + add(db, "bandeja", "PER-VOICE", profile_id=profile["id"]) + assert apply_pronunciations("a bandeja", "en", db)[0] == "a bandeja" + + +# ── Degenerate input ───────────────────────────────────────────────── + + +def test_no_entries_is_a_no_op(db): + out, applied = apply_pronunciations("nothing to do", "en", db) + assert out == "nothing to do" + assert applied == [] + + +@pytest.mark.parametrize("text", ["", " "]) +def test_blank_text_is_returned_unchanged(db, text): + add(db, "bandeja", "ban-DEH-ha") + assert apply_pronunciations(text, "en", db) == (text, []) + + +def test_build_pattern_handles_no_usable_terms(): + assert build_pattern([]) is None + assert build_pattern(["", " "]) is None + + +def test_regex_metacharacters_in_a_term_are_literal(db): + """A term is text, not a pattern -- an unescaped '.' would match anything.""" + add(db, "C++", "C plus plus") + out, _ = apply_pronunciations("I write C++ daily.", "en", db) + assert out == "I write C plus plus daily." + + +# ── API ────────────────────────────────────────────────────────────── + + +def test_crud_roundtrip(client, db): + created = client.post( + "/pronunciations", json={"term": "bandeja", "replacement": "ban-DEH-ha", "language": "en"} + ) + assert created.status_code == 200, created.text + entry_id = created.json()["id"] + + listed = client.get("/pronunciations").json() + assert any(e["id"] == entry_id for e in listed) + + updated = client.put(f"/pronunciations/{entry_id}", json={"replacement": "ban-DAY-ha"}) + assert updated.status_code == 200 + assert updated.json()["replacement"] == "ban-DAY-ha" + assert updated.json()["term"] == "bandeja", "omitted fields must be left alone" + + assert client.delete(f"/pronunciations/{entry_id}").status_code == 200 + assert client.get("/pronunciations").json() == [] + + +def test_duplicate_in_the_same_scope_is_rejected(client, db): + body = {"term": "bandeja", "replacement": "ban-DEH-ha", "language": "en"} + assert client.post("/pronunciations", json=body).status_code == 200 + clash = client.post("/pronunciations", json=body) + assert clash.status_code == 409 + assert "already exists" in clash.json()["detail"] + + +def test_same_term_in_a_different_language_is_allowed(client, db): + assert client.post( + "/pronunciations", json={"term": "bandeja", "replacement": "A", "language": "en"} + ).status_code == 200 + assert client.post( + "/pronunciations", json={"term": "bandeja", "replacement": "B", "language": "it"} + ).status_code == 200 + + +def test_unknown_profile_is_rejected(client, db): + r = client.post( + "/pronunciations", + json={"term": "x", "replacement": "y", "profile_id": "no-such-profile"}, + ) + assert r.status_code == 404 + + +def test_preview_shows_the_rewrite(client, db): + client.post("/pronunciations", json={"term": "bandeja", "replacement": "ban-DEH-ha"}) + r = client.post( + "/pronunciations/preview", json={"text": "a bandeja", "language": "en"} + ) + assert r.status_code == 200, r.text + body = r.json() + assert body["original"] == "a bandeja" + assert body["result"] == "a ban-DEH-ha" + assert body["applied"][0]["term"] == "bandeja" + + +def test_missing_entry_404s(client, db): + assert client.put("/pronunciations/nope", json={"replacement": "x"}).status_code == 404 + assert client.delete("/pronunciations/nope").status_code == 404 + + +# ── The property the design rests on ───────────────────────────────── + + +@pytest.mark.asyncio +async def test_engine_gets_the_respelling_but_the_row_keeps_the_original( + client, db, profile, monkeypatch +): + """The property the whole design rests on. + + If the respelling were stored, History would show a reader ``ban-DEH-ha`` + and editing an entry could never change anything already generated. So the + engine must see the respelling and the database must not. + + The model itself is mocked -- this is about which string goes where, and + loading 3.5 GB of weights would not make the assertion any truer. + """ + import numpy as np + + from backend.services import history as history_service + from backend.services.generation import run_generation + + client.post("/pronunciations", json={"term": "bandeja", "replacement": "ban-DEH-ha"}) + + original = "He plays a bandeja." + generation = await history_service.create_generation( + profile_id=profile["id"], + text=original, + language="en", + audio_path="", + duration=0, + seed=None, + db=db, + status="generating", + engine="qwen", + ) + + seen = {} + + class FakeBackend: + def is_loaded(self): + return True + + async def fake_generate_chunked(_model, text, _voice_prompt, **_kwargs): + seen["text"] = text + return np.zeros(2400, dtype=np.float32), 24000 + + async def fake_load(*_a, **_k): + return None + + async def fake_voice_prompt(*_a, **_k): + return {} + + monkeypatch.setattr("backend.backends.get_tts_backend_for_engine", lambda _e: FakeBackend()) + monkeypatch.setattr("backend.backends.load_engine_model", fake_load) + monkeypatch.setattr("backend.utils.chunked_tts.generate_chunked", fake_generate_chunked) + monkeypatch.setattr( + "backend.services.profiles.create_voice_prompt_for_profile", fake_voice_prompt + ) + + await run_generation( + generation_id=generation.id, + profile_id=profile["id"], + text=original, + language="en", + engine="qwen", + model_size="1.7B", + seed=None, + mode="generate", + ) + + assert seen["text"] == "He plays a ban-DEH-ha.", "engine should receive the respelling" + + db.expire_all() + stored = client.get(f"/history/{generation.id}").json() + assert stored["text"] == original, "the stored row must keep the author's text" + assert "ban-DEH-ha" not in stored["text"] + + +def test_preview_is_the_only_way_to_see_the_rewrite(client, db): + """Since the rewrite is never stored, preview is what makes it inspectable + rather than a black box between the text and the audio.""" + client.post("/pronunciations", json={"term": "bandeja", "replacement": "ban-DEH-ha"}) + body = client.post( + "/pronunciations/preview", json={"text": "a bandeja", "language": "en"} + ).json() + assert body["result"] != body["original"] + assert body["applied"] From 425dbc8a54179dc11ab61c2f53e7fa19665bab0b Mon Sep 17 00:00:00 2001 From: Lvigentini Date: Mon, 10 Aug 2026 03:06:10 +1000 Subject: [PATCH 2/3] fix(pronunciation): address review findings on #1025 Four of the five CodeRabbit findings were real. SQL wildcards in a term ----------------------- `find_duplicate` used `term.ilike(term)`, so `%` and `_` in a term were read as patterns rather than literals -- `band_ja` collided with `bandeja` and was rejected as a duplicate. Compares `lower(term)` instead, trimmed first, since the route stores the trimmed value and an untrimmed lookup missed its own duplicate. Whitespace-only values ---------------------- `min_length=1` accepted `" "`, which the route then stripped and stored as empty -- a no-op entry, or a replacement that deletes the matched speech. Both request models now strip before length-checking, matching the `TrimmedName` approach already used for folders. Preview accepted an unknown profile ----------------------------------- It silently fell back to global scope and reported a result the real generation would not produce, which 404s on an unknown profile. Validates first. Scope uniqueness now enforced by the database --------------------------------------------- `find_duplicate` is check-then-act; two concurrent creates both pass it. My comment claimed a constraint could not express this because SQL treats NULLs as distinct -- that was wrong. A unique expression index over `lower(term), COALESCE(language, ''), COALESCE(profile_id, '')` maps the wildcard scopes onto comparable values and holds. Violations map to 409 rather than 500. `find_duplicate` stays as the early check that can name the existing row in the message. Added a migration for it: `create_all` builds the index with the table, but will not add one to a table that already exists, so a database from an earlier build of this feature would never get it. Pre-existing duplicates are collapsed first, keeping the oldest row per scope, or CREATE UNIQUE INDEX would fail. `IF NOT EXISTS` because the inspector reflects a snapshot and a migration that raises takes startup down with it -- which the full suite reproduced, several modules booting the app in one process. Not taken: logging terms at INFO. Terms are user-supplied and often names, so the count stays at INFO and the values moved to DEBUG. 7 further tests: literal wildcards, untrimmed duplicate lookup, whitespace-only rejection on both fields, trimmed storage, preview validation, the database constraint including the case-differing global pair a plain UNIQUE would let through, and that terms stay out of INFO logs. Co-Authored-By: Claude Opus 5 (1M context) --- backend/database/migrations.py | 50 +++++++++++++++++ backend/database/models.py | 35 +++++++++++- backend/models.py | 21 ++++++-- backend/routes/pronunciation.py | 34 ++++++++++-- backend/services/pronunciation.py | 26 +++++---- backend/tests/test_pronunciation.py | 83 +++++++++++++++++++++++++++++ 6 files changed, 228 insertions(+), 21 deletions(-) diff --git a/backend/database/migrations.py b/backend/database/migrations.py index d353b58c8..44af01627 100644 --- a/backend/database/migrations.py +++ b/backend/database/migrations.py @@ -43,9 +43,59 @@ def run_migrations(engine) -> None: _migrate_generation_versions(engine, inspector, tables) _migrate_capture_settings(engine, inspector, tables) _migrate_mcp_bindings(engine, inspector, tables) + _migrate_pronunciation_entries(engine, inspector, tables) _normalize_storage_paths(engine, tables) +def _migrate_pronunciation_entries(engine, inspector, tables: set[str]) -> None: + """Add the scope-uniqueness index to an existing pronunciation_entries table. + + ``create_all`` builds the index with the table, so a fresh install needs + nothing here. A database created by an earlier build of this feature has the + table but not the index, and ``create_all`` will not add one to a table that + already exists. + + Duplicates that predate the index would make CREATE UNIQUE INDEX fail, so + they are collapsed first, keeping the oldest row in each scope. + + ``IF NOT EXISTS`` rather than relying on the inspector check alone: the + inspector reflects a snapshot, and a migration that raises takes startup + down with it. The check stays as the fast path that skips the dedup scan. + """ + if "pronunciation_entries" not in tables: + return + existing = {ix["name"] for ix in inspector.get_indexes("pronunciation_entries")} + if "uq_pronunciation_scope" in existing: + return + + with engine.connect() as conn: + removed = conn.execute( + text( + """ + DELETE FROM pronunciation_entries WHERE id NOT IN ( + SELECT MIN(id) FROM pronunciation_entries + GROUP BY lower(term), COALESCE(language, ''), COALESCE(profile_id, '') + ) + """ + ) + ).rowcount + conn.execute( + text( + """ + CREATE UNIQUE INDEX IF NOT EXISTS uq_pronunciation_scope + ON pronunciation_entries ( + lower(term), COALESCE(language, ''), COALESCE(profile_id, '') + ) + """ + ) + ) + conn.commit() + + if removed: + logger.info("Collapsed %d duplicate pronunciation entries before indexing", removed) + logger.info("Added uq_pronunciation_scope index to pronunciation_entries") + + # -- helpers --------------------------------------------------------------- def _get_columns(inspector, table: str) -> set[str]: diff --git a/backend/database/models.py b/backend/database/models.py index 5fc2650e6..d15c2e0c5 100644 --- a/backend/database/models.py +++ b/backend/database/models.py @@ -1,9 +1,21 @@ """ORM model definitions for the voicebox SQLite database.""" -from datetime import datetime import uuid +from datetime import datetime -from sqlalchemy import Column, String, Integer, Float, DateTime, Text, ForeignKey, Boolean, JSON +from sqlalchemy import ( + JSON, + Boolean, + Column, + DateTime, + Float, + ForeignKey, + Index, + Integer, + String, + Text, + func, +) from sqlalchemy.ext.declarative import declarative_base from ..utils.capture_chords import ( @@ -165,6 +177,25 @@ class PronunciationEntry(Base): created_at = Column(DateTime, default=datetime.utcnow) updated_at = Column(DateTime, default=datetime.utcnow, onupdate=datetime.utcnow) + # One entry per (term, language, profile) scope, enforced by the database + # rather than only by a pre-insert check, which two concurrent creates can + # both pass. + # + # A plain UniqueConstraint would not do it: SQL treats NULLs as distinct, + # so two global entries for the same term would both be accepted -- exactly + # the case worth catching. COALESCE maps the wildcard scopes onto a real + # value so they compare equal, and lower() makes the term case-insensitive + # to match how it is looked up. + __table_args__ = ( + Index( + "uq_pronunciation_scope", + func.lower(term), + func.coalesce(language, ""), + func.coalesce(profile_id, ""), + unique=True, + ), + ) + class EffectPreset(Base): """Saved effect chain preset.""" diff --git a/backend/models.py b/backend/models.py index 1b620aaf2..20a1acdde 100644 --- a/backend/models.py +++ b/backend/models.py @@ -2,7 +2,8 @@ Pydantic models for request/response validation. """ -from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, StringConstraints +from typing_extensions import Annotated from typing import Optional, List from datetime import datetime @@ -127,11 +128,21 @@ class Config: from_attributes = True +# A value of " " passes a raw min_length check and then stores as empty once +# the route strips it. Strip first, then length-check the result. +TrimmedTerm = Annotated[ + str, StringConstraints(strip_whitespace=True, min_length=1, max_length=200) +] +TrimmedReplacement = Annotated[ + str, StringConstraints(strip_whitespace=True, min_length=1, max_length=500) +] + + class PronunciationEntryCreate(BaseModel): """Request model for creating a pronunciation entry.""" - term: str = Field(..., min_length=1, max_length=200) - replacement: str = Field(..., min_length=1, max_length=500) + term: TrimmedTerm + replacement: TrimmedReplacement language: Optional[str] = Field( None, pattern="^(zh|en|ja|ko|de|fr|ru|pt|es|it|he|ar|da|el|fi|hi|ms|nl|no|pl|sv|sw|tr)$", @@ -147,8 +158,8 @@ class PronunciationEntryCreate(BaseModel): class PronunciationEntryUpdate(BaseModel): """Request model for updating a pronunciation entry. Omitted fields are left alone.""" - term: Optional[str] = Field(None, min_length=1, max_length=200) - replacement: Optional[str] = Field(None, min_length=1, max_length=500) + term: Optional[TrimmedTerm] = None + replacement: Optional[TrimmedReplacement] = None language: Optional[str] = Field( None, pattern="^(zh|en|ja|ko|de|fr|ru|pt|es|it|he|ar|da|el|fi|hi|ms|nl|no|pl|sv|sw|tr)$" ) diff --git a/backend/routes/pronunciation.py b/backend/routes/pronunciation.py index d29da95b3..e8f396cdd 100644 --- a/backend/routes/pronunciation.py +++ b/backend/routes/pronunciation.py @@ -3,6 +3,7 @@ import logging from fastapi import APIRouter, Depends, HTTPException, Query +from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session from .. import models @@ -14,6 +15,28 @@ router = APIRouter() +_SCOPE_TAKEN = ( + "That scope already has an entry for this term. Update the existing entry instead." +) + + +def _commit_or_conflict(db: Session) -> None: + """Commit, turning a scope collision into a 409. + + ``find_duplicate`` runs first and gives a friendlier message naming the + existing row, but it is a check-then-act pair: two concurrent creates can + both pass it. ``uq_pronunciation_scope`` is what actually holds, so its + violation has to surface as a conflict rather than a 500. + """ + try: + db.commit() + except IntegrityError as exc: + db.rollback() + if "uq_pronunciation_scope" in str(exc.orig): + raise HTTPException(status_code=409, detail=_SCOPE_TAKEN) from exc + raise + + def _validate_profile(profile_id: str | None, db: Session) -> None: if profile_id is None: return @@ -64,15 +87,15 @@ async def create_pronunciation( ) entry = PronunciationEntry( - term=data.term.strip(), - replacement=data.replacement.strip(), + term=data.term, + replacement=data.replacement, language=data.language, profile_id=data.profile_id, enabled=data.enabled, notes=data.notes, ) db.add(entry) - db.commit() + _commit_or_conflict(db) db.refresh(entry) return entry @@ -108,9 +131,9 @@ async def update_pronunciation( ) for key, value in fields.items(): - setattr(entry, key, value.strip() if key in {"term", "replacement"} and value else value) + setattr(entry, key, value) - db.commit() + _commit_or_conflict(db) db.refresh(entry) return entry @@ -137,6 +160,7 @@ async def preview_pronunciations( stored, so without this there is no way to see what a rule actually does short of listening to the output. """ + _validate_profile(data.profile_id, db) result, applied = pronunciation.apply_pronunciations( data.text, data.language, db, profile_id=data.profile_id ) diff --git a/backend/services/pronunciation.py b/backend/services/pronunciation.py index 8a4e27a74..8def983ee 100644 --- a/backend/services/pronunciation.py +++ b/backend/services/pronunciation.py @@ -16,7 +16,7 @@ import logging import re -from sqlalchemy import or_ +from sqlalchemy import func, or_ from sqlalchemy.orm import Session from ..database import PronunciationEntry @@ -146,10 +146,12 @@ def substitute(m: re.Match) -> str: result = pattern.sub(substitute, text) if applied: - logger.info( - "Pronunciation dictionary rewrote %d term(s): %s", - len(applied), - ", ".join(f"{a['term']}->{a['replacement']}" for a in applied[:5]), + # Terms are user-supplied and are often names, so the values stay at + # DEBUG; INFO carries only how many were rewritten. + logger.info("Pronunciation dictionary rewrote %d term(s)", len(applied)) + logger.debug( + "Pronunciation substitutions: %s", + ", ".join(f"{a['term']}->{a['replacement']}" for a in applied), ) return result, applied @@ -163,11 +165,17 @@ def find_duplicate( ) -> PronunciationEntry | None: """An existing entry for the same term in the same scope. - Enforced here rather than as a unique constraint because SQL treats NULLs - as distinct, so a constraint would happily accept two global entries for - the same term — exactly the case worth catching. + An early check so the caller can return a useful 409 naming the existing + entry. The database enforces the same rule via ``uq_pronunciation_scope``, + which is what actually holds under concurrent creates. + + Compares ``lower(term)`` rather than ``ilike``: a term is a literal, and + ``ilike`` would read ``%`` and ``_`` in it as wildcards, so ``band_ja`` + would collide with ``bandeja``. Trimmed first, because the route stores the + trimmed value and an untrimmed lookup would miss its own duplicate. """ - q = db.query(PronunciationEntry).filter(PronunciationEntry.term.ilike(term)) + normalized = term.strip().lower() + q = db.query(PronunciationEntry).filter(func.lower(PronunciationEntry.term) == normalized) q = ( q.filter(PronunciationEntry.language.is_(None)) if language is None diff --git a/backend/tests/test_pronunciation.py b/backend/tests/test_pronunciation.py index 43ea21a74..1b5e782f8 100644 --- a/backend/tests/test_pronunciation.py +++ b/backend/tests/test_pronunciation.py @@ -363,3 +363,86 @@ def test_preview_is_the_only_way_to_see_the_rewrite(client, db): ).json() assert body["result"] != body["original"] assert body["applied"] + + +# ── Review findings (CodeRabbit on #1025) ──────────────────────────── + + +def test_sql_wildcards_in_a_term_are_literal(client, db): + """`ilike` read `%` and `_` in a term as wildcards, so `band_ja` collided + with `bandeja` and blocked a legitimate entry as a duplicate.""" + assert client.post( + "/pronunciations", json={"term": "bandeja", "replacement": "ban-DEH-ha"} + ).status_code == 200 + # Distinct term that ilike would have matched against the one above. + assert client.post( + "/pronunciations", json={"term": "band_ja", "replacement": "BAND-ja"} + ).status_code == 200 + assert client.post( + "/pronunciations", json={"term": "band%", "replacement": "BAND"} + ).status_code == 200 + + +def test_untrimmed_term_still_finds_its_duplicate(client, db): + """The stored value is trimmed, so an untrimmed lookup must normalise too or + it misses the duplicate it just created.""" + assert client.post( + "/pronunciations", json={"term": "bandeja", "replacement": "A"} + ).status_code == 200 + clash = client.post("/pronunciations", json={"term": " bandeja ", "replacement": "B"}) + assert clash.status_code == 409 + + +@pytest.mark.parametrize("field", ["term", "replacement"]) +def test_whitespace_only_values_are_rejected(client, db, field): + """`min_length=1` accepted " ", which then stored as empty — a no-op entry, + or a replacement that deletes the matched speech.""" + body = {"term": "bandeja", "replacement": "ban-DEH-ha"} + body[field] = " " + assert client.post("/pronunciations", json=body).status_code == 422 + + +def test_values_are_stored_trimmed(client, db): + created = client.post( + "/pronunciations", json={"term": " bandeja ", "replacement": " ban-DEH-ha "} + ).json() + assert created["term"] == "bandeja" + assert created["replacement"] == "ban-DEH-ha" + + +def test_preview_rejects_an_unknown_profile(client, db): + """Generation 404s on an unknown profile; preview silently fell back to + global scope, so it reported a different result than it would produce.""" + r = client.post( + "/pronunciations/preview", + json={"text": "a bandeja", "profile_id": "no-such-profile"}, + ) + assert r.status_code == 404 + + +def test_the_database_enforces_scope_uniqueness(client, db): + """find_duplicate is check-then-act; two concurrent creates can both pass + it. The constraint is what actually holds — including for the NULL scopes, + which a plain UNIQUE would treat as distinct.""" + from sqlalchemy.exc import IntegrityError + + add(db, "bandeja", "A") + # Differs only by case, and both are global scope — the pair a plain + # UNIQUE(term, language, profile_id) would have let through. + db.add(PronunciationEntry(term="Bandeja", replacement="B")) + with pytest.raises(IntegrityError): + db.commit() + db.rollback() + + +def test_terms_are_not_logged_at_info(client, db, caplog): + """Terms are user-supplied and often names, so the values belong at DEBUG.""" + import logging + + add(db, "Alicia Fernandez", "ah-LEE-see-ah") + with caplog.at_level(logging.INFO, logger="backend.services.pronunciation"): + apply_pronunciations("Ask Alicia Fernandez.", "en", db) + + info = [r for r in caplog.records if r.levelno == logging.INFO] + assert info, "should still report that a rewrite happened" + assert all("Alicia" not in r.getMessage() for r in info) From 4c2ad09436fa50f8c793aacfa02beeee14f836df Mon Sep 17 00:00:00 2001 From: Lvigentini Date: Mon, 10 Aug 2026 03:16:04 +1000 Subject: [PATCH 3/3] fix(pronunciation): dedupe by created_at, not by random UUID Two more review findings, both correct. `MIN(id)` is not "oldest" ------------------------ The dedup step before CREATE UNIQUE INDEX claimed to keep the oldest row in each scope but ordered by `MIN(id)`, and ids are random UUIDs -- so it kept an arbitrary row, and a different one on a different machine. The code did not do what its own comment said. Now orders by `COALESCE(created_at, '') || '|' || id`: timestamp first, id only to break ties deterministically. ISO-8601 text sorts chronologically, so this needs no window function and stays portable across SQLite builds. Weak privacy assertion ---------------------- The test that terms stay out of INFO logs checked only the first name, so a message leaking just the surname would have passed. Checks every fragment of the term and the replacement. Added a test that the dedup keeps the oldest row, inserting the newer entry first so insertion order cannot be what makes it pass. Co-Authored-By: Claude Opus 5 (1M context) --- backend/database/migrations.py | 13 +++++++-- backend/tests/test_pronunciation.py | 45 ++++++++++++++++++++++++++++- 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/backend/database/migrations.py b/backend/database/migrations.py index 44af01627..34a6d7b94 100644 --- a/backend/database/migrations.py +++ b/backend/database/migrations.py @@ -58,6 +58,13 @@ def _migrate_pronunciation_entries(engine, inspector, tables: set[str]) -> None: Duplicates that predate the index would make CREATE UNIQUE INDEX fail, so they are collapsed first, keeping the oldest row in each scope. + "Oldest" has to mean ``created_at``, not ``MIN(id)``: ids are random UUIDs, + so ordering by id would keep an arbitrary row and pick a different one on a + different machine. The concatenated key sorts by timestamp first and falls + back to id, which keeps the choice deterministic when two rows share a + timestamp -- ISO-8601 text sorts chronologically, so this needs no window + function and stays portable across SQLite builds. + ``IF NOT EXISTS`` rather than relying on the inspector check alone: the inspector reflects a snapshot, and a migration that raises takes startup down with it. The check stays as the fast path that skips the dedup scan. @@ -72,8 +79,10 @@ def _migrate_pronunciation_entries(engine, inspector, tables: set[str]) -> None: removed = conn.execute( text( """ - DELETE FROM pronunciation_entries WHERE id NOT IN ( - SELECT MIN(id) FROM pronunciation_entries + DELETE FROM pronunciation_entries + WHERE COALESCE(created_at, '') || '|' || id NOT IN ( + SELECT MIN(COALESCE(created_at, '') || '|' || id) + FROM pronunciation_entries GROUP BY lower(term), COALESCE(language, ''), COALESCE(profile_id, '') ) """ diff --git a/backend/tests/test_pronunciation.py b/backend/tests/test_pronunciation.py index 1b5e782f8..c6d8f2792 100644 --- a/backend/tests/test_pronunciation.py +++ b/backend/tests/test_pronunciation.py @@ -445,4 +445,47 @@ def test_terms_are_not_logged_at_info(client, db, caplog): info = [r for r in caplog.records if r.levelno == logging.INFO] assert info, "should still report that a rewrite happened" - assert all("Alicia" not in r.getMessage() for r in info) + # Every fragment, not just the first: a message leaking only the surname + # would still be leaking a name. + for fragment in ("Alicia", "Fernandez", "ah-LEE-see-ah"): + assert all(fragment not in r.getMessage() for r in info), fragment + + +def test_dedup_migration_keeps_the_oldest_row(client, db): + """The migration collapses pre-index duplicates, and "oldest" has to mean + created_at -- ids are random UUIDs, so ordering by id keeps an arbitrary + row and would pick a different one on a different machine.""" + from datetime import datetime + + from sqlalchemy import text as sql_text + + # Via the module, not `from ... import engine`: init_db() rebinds the + # global, so a name imported earlier still points at the pre-init None. + from backend.database import session as db_session + + engine = db_session.engine + + with engine.connect() as conn: + conn.execute(sql_text("DROP INDEX IF EXISTS uq_pronunciation_scope")) + conn.commit() + + older = PronunciationEntry( + term="bandeja", replacement="KEEP", created_at=datetime(2020, 1, 1) + ) + newer = PronunciationEntry( + term="Bandeja", replacement="DROP", created_at=datetime(2030, 1, 1) + ) + # Insert newest first so insertion order can't be what makes it pass. + db.add(newer) + db.commit() + db.add(older) + db.commit() + + from backend.database.migrations import run_migrations + + run_migrations(engine) + + db.expire_all() + rows = db.query(PronunciationEntry).all() + assert len(rows) == 1 + assert rows[0].replacement == "KEEP"