Skip to content

feat(tts): add a pronunciation dictionary - #1025

Open
Lvigentini wants to merge 3 commits into
jamiepine:mainfrom
Lvigentini:feat/pronunciation-dictionary
Open

feat(tts): add a pronunciation dictionary#1025
Lvigentini wants to merge 3 commits into
jamiepine:mainfrom
Lvigentini:feat/pronunciation-dictionary

Conversation

@Lvigentini

@Lvigentini Lvigentini commented Aug 9, 2026

Copy link
Copy Markdown

Closes #827.

Names, acronyms, brands and loanwords come out wrong and there is no reusable way to fix them — you edit the text every time, in every script. @aferreira-deo makes the same point on that issue about WCAG and technical terms in coding tutorials.

Approach

A term -> respelling map applied just before TTS:

curl -X POST localhost:17493/pronunciations \
  -d '{"term": "bandeja", "replacement": "ban-DEH-ha", "language": "en"}'

Respelling rather than phonemes, on purpose. Every engine reads plain text, so this works on all eight; a phoneme string only works on the engines that accept one. Cruder, portable. Phoneme support can layer on later per engine without changing the model.

Scoping. Entries are global by default, and can be narrowed 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.

The stored text stays clean

The dictionary runs at generation time, not when the text is saved. generations.text keeps what the author wrote, so History never shows a reader ban-DEH-ha, and editing an entry changes future audio without rewriting the past.

That is also why POST /pronunciations/preview exists — the rewritten string is never stored, so without it there is no way to see what a rule actually does short of listening to the output.

Matching

A single pass over one alternation of every term, longest first. That is what stops replacements cascading: with bandeja -> ban-DEH-ha and ha -> hah, a loop of per-term substitutions yields ban-DEH-hah. Longest-first also lets bandeja alta beat the bandeja inside it.

  • Word boundaries via lookarounds rather than \b, so terms with punctuation still anchor
  • Terms are escaped — a term is text, not a pattern
  • [laugh]-style tags are skipped; they are engine syntax, not speech
  • Capitalisation carries onto the replacement, counting cased characters rather than str.isupper() — that returns True for C++, and shouting the replacement would give C PLUS PLUS

Duplicate scopes are rejected in the service rather than by a unique constraint, since SQL treats NULLs as distinct and a constraint would happily accept two global entries for the same term.

Applied on both /generate and /generate/stream, so a streamed preview matches what the persisted path produces.

Endpoints

Method Path
GET /pronunciations list, optionally filtered to what applies to a language/voice
POST /pronunciations create (409 on a duplicate scope)
PUT /pronunciations/{id} update; omitted fields are left alone
DELETE /pronunciations/{id} delete
POST /pronunciations/preview see the rewrite without generating

Tests

28 in backend/tests/test_pronunciation.py — matching, the no-cascade rule, capitalisation including the C++ case, scope resolution and leakage, degenerate input, CRUD, and the property the design rests on: the engine receives the respelling and the stored row does not. The model is mocked there; loading 3.5 GB of weights would not make the assertion any truer.

Backend only — no UI yet, deliberately, so the model and matching rules can be argued about before anything is built on them.

Branched off main, independent of my other open PRs.

Unrelated observation while testing: on main the backend suite writes into the repo's ./data because VOICEBOX_DATA_DIR is not honoured — every test module that sets it (test_story_mixdown.py, test_data_dir_env.py, and others) is silently using the real data directory, so state persists between runs. #1004 fixes that; flagging since it makes the suite order-dependent today.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added a pronunciation dictionary for custom term respellings.
    • Added tools to create, update, delete, filter, enable, disable, and preview pronunciation entries.
    • Added language- and profile-specific pronunciation settings.
    • Applied pronunciation substitutions during speech generation while preserving the original text in history.
    • Preserved capitalization, avoided chained replacements, and prevented duplicate pronunciation entries.
  • Tests
    • Added comprehensive coverage for pronunciation matching, validation, scoping, previews, and generation behavior.

Names, acronyms, brands and loanwords come out wrong and there is no reusable
way to fix them (jamiepine#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 jamiepine#827

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a scoped pronunciation dictionary with CRUD and preview APIs. Pronunciation substitutions apply during TTS generation, while generation history retains the original text. Tests cover matching, scoping, validation, APIs, preview, migration, and generation integration.

Changes

Pronunciation dictionary

Layer / File(s) Summary
Pronunciation contracts and persistence
backend/database/..., backend/models.py
Adds the PronunciationEntry ORM model, validated request and response models, and an idempotent uniqueness migration.
Pronunciation substitution service
backend/services/pronunciation.py
Retrieves scoped enabled entries, builds longest-first patterns, preserves capitalization, skips bracketed tags, prevents replacement chaining, and detects duplicates.
Pronunciation management API
backend/routes/pronunciation.py, backend/routes/__init__.py
Adds list, create, update, delete, and preview endpoints with profile validation, filtering, duplicate detection, persistence, and router registration.
Generation integration and validation
backend/services/generation.py, backend/routes/generations.py, backend/tests/test_pronunciation.py
Applies substitutions before TTS generation while retaining original history text. Tests cover matching, scoping, APIs, preview, migration, persistence, logging, and generation integration.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Generation
  participant PronunciationService
  participant Database
  participant TTSEngine
  Generation->>PronunciationService: apply_pronunciations(text, language, profile_id)
  PronunciationService->>Database: load enabled scoped entries
  Database-->>PronunciationService: pronunciation entries
  PronunciationService-->>Generation: rewritten text and substitutions
  Generation->>TTSEngine: generate_chunked(rewritten text)
  Generation->>Database: store original authored text
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 39.02% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: adding pronunciation dictionary support for TTS.
Linked Issues check ✅ Passed The changes implement reusable pronunciation corrections with scoped CRUD, preview, validation, and TTS integration requested in issue [#827].
Out of Scope Changes check ✅ Passed The database, service, API, migration, integration, and test changes directly support the pronunciation dictionary objectives.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/database/models.py`:
- Around line 152-166: Add database-enforced uniqueness for the normalized
pronunciation term across each language and profile scope, treating NULL
language and NULL profile_id values consistently on SQLite; define the
constraint or expression index on the pronunciation_entries model. Update the
pronunciation insert and update handlers to catch violations of this constraint
and return HTTP 409, while preserving find_duplicate() as an early check.

In `@backend/models.py`:
- Around line 133-144: Update both request models containing term and
replacement fields to validate the trimmed values rather than only their raw
lengths. Ensure whitespace-only inputs are rejected while preserving the
existing length limits and route behavior for valid values.

In `@backend/routes/pronunciation.py`:
- Around line 140-143: Validate a supplied profile ID in the preview handler by
calling _validate_profile(data.profile_id, db) before
pronunciation.apply_pronunciations(). Only perform this validation when
data.profile_id is provided, preserving the existing global-scope behavior when
it is absent.

In `@backend/services/pronunciation.py`:
- Around line 149-153: Update the INFO log in the pronunciation rewrite flow to
stop including raw terms and replacements from applied; log only the replacement
count, or move non-sensitive identifiers to an appropriate debug-level message.
- Around line 170-180: Update find_duplicate() to normalize the input term by
stripping whitespace and compare it case-insensitively as a literal value,
escaping SQL wildcard and escape characters instead of treating them as
patterns. Ensure duplicate detection matches the normalized term stored by the
route, and add coverage for whitespace plus literal %, _, and escape-related
terms.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 906cfaad-31d8-4c40-b730-543b41de1ed5

📥 Commits

Reviewing files that changed from the base of the PR and between 51f49de and 7e6c52b.

📒 Files selected for processing (9)
  • backend/database/__init__.py
  • backend/database/models.py
  • backend/models.py
  • backend/routes/__init__.py
  • backend/routes/generations.py
  • backend/routes/pronunciation.py
  • backend/services/generation.py
  • backend/services/pronunciation.py
  • backend/tests/test_pronunciation.py

Comment on lines +152 to +166
__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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/sh
set -eu

# Identify the deployed database dialect and the repository migration mechanism.
rg -n -C 3 'create_engine\(|DATABASE_URL|sqlite|postgres|mysql|Base\.metadata\.create_all|alembic' \
  backend pyproject.toml setup.cfg setup.py 2>/dev/null || true
fd -HI -t f | rg '(^|/)(alembic\.ini|env\.py|.*migration.*)$' || true

Repository: jamiepine/voicebox

Length of output: 5689


🏁 Script executed:

#!/bin/sh
set -eu

echo "== models/PronunciationEntry =="
sed -n '130,180p' backend/database/models.py 2>/dev/null || true

echo "== pronunciation routes/service duplicate logic =="
fd -HI -t f 'pronunciation|pronounce' backend | sed -n '1,20p'
for f in $(fd -HI -t f 'pronunciation|pronounce' backend); do
  echo "--- $f"
  rg -n -C 4 'find_duplicate|duplicate|PronunciationEntry|normalized|normalize|insert|create\(|409|Conflict' "$f" || true
done

echo "== migrations checks around pronunciation_entries =="
rg -n -C 6 'pronunciation_entries|find_duplicate|duplicate|created_if_empty|create_table|unique|uniqueConstraint' backend/database/migrations.py backend/database/models.py 2>/dev/null || true

Repository: jamiepine/voicebox

Length of output: 13642


Add a NULL-aware uniqueness constraint for pronunciation scopes.

find_duplicate() is a pre-insert check, so concurrent creates can insert the same normalized term, language, and global/profile scope into pronunciation_entries. Add/patch a unique constraint or expression index that enforces this scope across SQLite before adding the row, and map its integrity error to HTTP 409 on insert/update.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/database/models.py` around lines 152 - 166, Add database-enforced
uniqueness for the normalized pronunciation term across each language and
profile scope, treating NULL language and NULL profile_id values consistently on
SQLite; define the constraint or expression index on the pronunciation_entries
model. Update the pronunciation insert and update handlers to catch violations
of this constraint and return HTTP 409, while preserving find_duplicate() as an
early check.

Comment thread backend/models.py Outdated
Comment thread backend/routes/pronunciation.py
Comment thread backend/services/pronunciation.py Outdated
Comment thread backend/services/pronunciation.py Outdated
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) <noreply@anthropic.com>
@Lvigentini

Copy link
Copy Markdown
Author

Thanks @coderabbitai — four of five were real. Addressed in 425dbc8.

ilike treated terms as patterns. The worst of them. find_duplicate used term.ilike(term), so % and _ were wildcards rather than literals: band_ja collided with bandeja and was rejected as a duplicate. Now compares lower(term), trimmed first — the route stores the trimmed value, so an untrimmed lookup was missing its own duplicate too.

Whitespace-only values. Correct, and there was already a TrimmedName precedent in the folders work I should have reused. Both request models now strip before length-checking.

Preview accepted an unknown profile. Correct — it fell back to global scope and reported a result generation wouldn't produce. Validates first now.

Scope uniqueness. You were right and my code comment was wrong. It claimed a constraint couldn't express this because SQL treats NULLs as distinct — true of a plain UniqueConstraint, but a unique expression index handles it:

CREATE UNIQUE INDEX uq_pronunciation_scope ON pronunciation_entries (
    lower(term), COALESCE(language, ''), COALESCE(profile_id, '')
)

COALESCE maps the wildcard scopes onto comparable values, so two global entries for the same term now collide as they should. Violations map to 409 instead of 500; find_duplicate stays as the early check that can name the existing row.

That needed a migration you didn't ask for but which the change implies: create_all builds the index with the table, but won't add one to a table that already exists, so a database from an earlier build of this branch would never get it. Pre-existing duplicates are collapsed first (oldest row per scope) or CREATE UNIQUE INDEX fails on them.

Worth recording: the first version used plain CREATE UNIQUE INDEX guarded by an inspector check, and the full suite caught it — several test modules boot the app in one process, the inspector reflects a stale snapshot, and the migration raised. A migration that can raise takes startup down with it, so it's IF NOT EXISTS now with the inspector check kept only as the fast path that skips the dedup scan.

Not taken — logging terms at INFO. Fair point that terms can be names, so the values moved to DEBUG and INFO carries only the count. I didn't go further and drop them entirely: when a rewrite produces wrong audio, "which rule fired" is the first question, and DEBUG is the right level for that rather than nowhere.

7 further tests covering each, including the case-differing global pair a plain UNIQUE would have let through.

@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

git show --stat --oneline 425dbc8
git show --format=fuller --find-renames --find-copies -- \
  backend/services/pronunciation.py \
  backend/models.py \
  backend/routes/pronunciation.py \
  backend/database/models.py \
  backend/database \
  backend/tests/test_pronunciation.py \
  425dbc8

printf '\n--- Relevant definitions ---\n'
ast-grep outline backend/services/pronunciation.py --items all --view expanded
ast-grep outline backend/routes/pronunciation.py --items all --view expanded
ast-grep outline backend/models.py --items all --match 'Pronunciation|TrimmedName' --view expanded

printf '\n--- Index and migration references ---\n'
rg -n -C 4 -i 'uq_pronunciation_scope|create unique index|if not exists|pronunciation_entries|inspect\(' backend

Length of output: 30860


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- Dependency declarations ---'
fd -HI '(^|/)(pyproject\.toml|requirements.*\.txt|Pipfile|poetry\.lock|uv\.lock)$' . \
  -x sh -c 'echo "--- $1"; rg -n -i -C 2 "pydantic|fastapi" "$1"' sh {}

printf '%s\n' '--- Existing Pydantic v2 APIs and configuration ---'
rg -n -C 2 'StringConstraints|model_config|ConfigDict|from_attributes|orm_mode' \
  backend README.md pyproject.toml 2>/dev/null || true

Length of output: 1372


@Lvigentini Verified. Commit 425dbc8 addresses the valid findings.

  • find_duplicate now uses a trimmed, case-normalized literal comparison. This avoids % and _ pattern matching.
  • TrimmedTerm and TrimmedReplacement reject whitespace-only values. The declared pydantic>=2.5.0 dependency supports StringConstraints.
  • Preview now validates profile_id before it applies rules.
  • uq_pronunciation_scope enforces case-insensitive scope uniqueness. The migration handles existing databases and uses IF NOT EXISTS.
  • INFO logs contain only the substitution count. DEBUG logs contain the substitution details.

The new tests cover these cases. The DEBUG logging decision is appropriate for rule diagnostics.

🐇 ✓


✏️ Learnings added
Learnt from: Lvigentini
URL: https://github.com/jamiepine/voicebox/pull/1025

Timestamp: 2026-08-09T17:07:05.031Z
Learning: For the Python FastAPI backend pronunciation dictionary, enforce case-insensitive uniqueness per `(term, language, profile_id)` scope with the `uq_pronunciation_scope` unique expression index over `lower(term)`, `COALESCE(language, '')`, and `COALESCE(profile_id, '')`. The startup migration must preserve the oldest entry for any pre-existing duplicate scope before it creates this index.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

You are interacting with an AI system.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
backend/models.py (1)

5-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Import Annotated from the stdlib typing module, since the project requires Python >=3.12.

Fix
-from typing_extensions import Annotated
+from typing import Annotated
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/models.py` around lines 5 - 6, Update the Annotated import in
backend/models.py to use the standard-library typing module instead of
typing_extensions, while leaving the Pydantic imports and model definitions
unchanged.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/database/migrations.py`:
- Around line 75-78: Update the deduplication query in the migration to retain
the pronunciation entry with the earliest created_at for each normalized term,
language, and profile_id group; use id only as the deterministic tie-breaker
when created_at values match, replacing the current MIN(id) selection.

In `@backend/tests/test_pronunciation.py`:
- Around line 446-448: Update the INFO-log assertions in the pronunciation
rewrite test to reject exposure of every fragment of the user-supplied term,
including both “Alicia” and “Fernandez,” while preserving the requirement that
an INFO record is emitted.

---

Nitpick comments:
In `@backend/models.py`:
- Around line 5-6: Update the Annotated import in backend/models.py to use the
standard-library typing module instead of typing_extensions, while leaving the
Pydantic imports and model definitions unchanged.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5e125ccc-d65f-4719-9f83-1963000b22c4

📥 Commits

Reviewing files that changed from the base of the PR and between 7e6c52b and 425dbc8.

📒 Files selected for processing (6)
  • backend/database/migrations.py
  • backend/database/models.py
  • backend/models.py
  • backend/routes/pronunciation.py
  • backend/services/pronunciation.py
  • backend/tests/test_pronunciation.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • backend/database/models.py
  • backend/services/pronunciation.py
  • backend/routes/pronunciation.py

Comment thread backend/database/migrations.py Outdated
Comment thread backend/tests/test_pronunciation.py Outdated
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) <noreply@anthropic.com>
@Lvigentini

Copy link
Copy Markdown
Author

Both correct, fixed in 4c2ad09.

MIN(id) is not "oldest". Good catch — the code didn't do what its own comment claimed. The dedup step said it kept the oldest row per scope and then ordered by MIN(id) over random UUIDs, so it kept an arbitrary row, and a different arbitrary row on a different machine.

Now ordered by COALESCE(created_at, '') || '|' || id — timestamp first, id only to break ties deterministically. ISO-8601 text sorts chronologically, so this avoids a window function and stays portable across SQLite builds rather than assuming 3.25+.

Added a test that inserts the newer row first, so insertion order can't be what makes it pass.

The privacy assertion was weak. Right — it checked "Alicia" only, so a message leaking just the surname would have passed a test whose whole point is that names don't reach INFO. Now asserts on every fragment of the term and on the replacement.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
backend/database/migrations.py (1)

86-97: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Align uq_pronunciation_scope with the stored term normalization.

Create and update requests strip whitespace via TrimmedTerm, but duplicate lookup compares the input to the raw lower(term) row value, and the ORM/migration indexes use only lower(term). Store the already trimmed term (or index lower(trim(term))) so rows for " bandeja " and "bandeja" cannot both exist and break the 409/duplicates tests.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/database/migrations.py` around lines 86 - 97, Align pronunciation
term storage and uniqueness with TrimmedTerm normalization: ensure create/update
requests persist the trimmed term, and update duplicate lookup plus the
migration’s uq_pronunciation_scope index to use lower(trim(term)) consistently.
Preserve the existing language and profile scope so whitespace variants such as
"  bandeja  " and "bandeja" cannot coexist.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@backend/database/migrations.py`:
- Around line 86-97: Align pronunciation term storage and uniqueness with
TrimmedTerm normalization: ensure create/update requests persist the trimmed
term, and update duplicate lookup plus the migration’s uq_pronunciation_scope
index to use lower(trim(term)) consistently. Preserve the existing language and
profile scope so whitespace variants such as "  bandeja  " and "bandeja" cannot
coexist.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 35b6c809-5723-4529-a64e-5ebce9e2e6ea

📥 Commits

Reviewing files that changed from the base of the PR and between 425dbc8 and 4c2ad09.

📒 Files selected for processing (2)
  • backend/database/migrations.py
  • backend/tests/test_pronunciation.py

Lvigentini added a commit to Lvigentini/voicebox that referenced this pull request Aug 10, 2026
Phase 5. `/generate` and `/generate/stream` now resolve dictionary entries and
prosody markup before synthesis.

Unmarked text is untouched
--------------------------
The property under most scrutiny is the one about *not* changing anything. A
script with no markup and no dictionary hits compiles to a single plain run and
takes the same single-shot call it always did, with the same arguments. Prose
that merely looks like markup -- `5 < 6`, `x > y` -- is literal, because the
parser only recognises a closed tag set.

Malformed markup falls back to speaking the text literally rather than failing
the generation. A stray tag must not be able to break generation for someone
who never used the feature; before this existed the text was literal, so that
is what it degrades to.

Auto-detected rather than opt-in, with `prosody: false` as the escape hatch for
a script that genuinely contains something tag-shaped. An opt-in flag would
have meant updating every caller -- MCP, API, story regeneration -- or leaving
the feature invisible.

One entry point, two callers
----------------------------
`generate_with_prosody` serves both generation paths, so "unmarked text behaves
as before" is a property of one function rather than a claim repeated twice.
`generate_chunked` is passed in rather than imported, so prosody composes with
chunking instead of competing: prosody splits by directive, chunking splits by
length, and a directive run that is still long goes through both.

The stored row keeps the markup, not the resolved text -- consistent with the
dictionary (jamiepine#1025) and regenerate (jamiepine#1026): the resolved form is derivable, the
author's markup is not, and editing markup to regenerate needs it intact.

Two fixes found by building this
--------------------------------
`is_trivial` excluded any plan carrying a substitution, so every respelled
sentence would have taken the renderer path for nothing -- contradicting the
property that makes respelling preferred, that it does not cut. `source_text`
is provenance for display; by that point the respelling is already in the text.

The migration guard for `uq_pronunciation_scope` never fired: SQLAlchemy cannot
reflect an expression-based index and skips it with a warning, so the inspector
never reported it and the dedup scan ran on every startup. Reads sqlite_master
directly now. `IF NOT EXISTS` had been quietly carrying it.

Also drops the direct `apply_pronunciations` call from the generation path: the
dictionary now reaches the engine as markup, which is how `language` and
`phoneme` entries work at all.

19 pipeline tests. 295 backend tests pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Lvigentini added a commit to Lvigentini/voicebox that referenced this pull request Aug 10, 2026
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) <noreply@anthropic.com>
Lvigentini added a commit to Lvigentini/voicebox that referenced this pull request Aug 10, 2026
Phase 5. `/generate` and `/generate/stream` now resolve dictionary entries and
prosody markup before synthesis.

Unmarked text is untouched
--------------------------
The property under most scrutiny is the one about *not* changing anything. A
script with no markup and no dictionary hits compiles to a single plain run and
takes the same single-shot call it always did, with the same arguments. Prose
that merely looks like markup -- `5 < 6`, `x > y` -- is literal, because the
parser only recognises a closed tag set.

Malformed markup falls back to speaking the text literally rather than failing
the generation. A stray tag must not be able to break generation for someone
who never used the feature; before this existed the text was literal, so that
is what it degrades to.

Auto-detected rather than opt-in, with `prosody: false` as the escape hatch for
a script that genuinely contains something tag-shaped. An opt-in flag would
have meant updating every caller -- MCP, API, story regeneration -- or leaving
the feature invisible.

One entry point, two callers
----------------------------
`generate_with_prosody` serves both generation paths, so "unmarked text behaves
as before" is a property of one function rather than a claim repeated twice.
`generate_chunked` is passed in rather than imported, so prosody composes with
chunking instead of competing: prosody splits by directive, chunking splits by
length, and a directive run that is still long goes through both.

The stored row keeps the markup, not the resolved text -- consistent with the
dictionary (jamiepine#1025) and regenerate (jamiepine#1026): the resolved form is derivable, the
author's markup is not, and editing markup to regenerate needs it intact.

Two fixes found by building this
--------------------------------
`is_trivial` excluded any plan carrying a substitution, so every respelled
sentence would have taken the renderer path for nothing -- contradicting the
property that makes respelling preferred, that it does not cut. `source_text`
is provenance for display; by that point the respelling is already in the text.

The migration guard for `uq_pronunciation_scope` never fired: SQLAlchemy cannot
reflect an expression-based index and skips it with a warning, so the inspector
never reported it and the dedup scan ran on every startup. Reads sqlite_master
directly now. `IF NOT EXISTS` had been quietly carrying it.

Also drops the direct `apply_pronunciations` call from the generation path: the
dictionary now reaches the engine as markup, which is how `language` and
`phoneme` entries work at all.

19 pipeline tests. 295 backend tests pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature Request: Add pronunciation dictionary support

1 participant