feat(api): regenerate a take with corrected text or settings - #1026
Open
Lvigentini wants to merge 2 commits into
Open
feat(api): regenerate a take with corrected text or settings#1026Lvigentini wants to merge 2 commits into
Lvigentini wants to merge 2 commits into
Conversation
jamiepine#870 asks to fix a typo in a story segment and re-run it. Today that means building a whole new generation, which loses the timeline placement, fades, speed and track the old one had -- so a one-character correction costs the arrangement around it. Most of the machinery was already there. `POST /generate/{id}/regenerate` already saved its output as a new *version* under the same generation id, and story items can already be pinned to a version. It simply could not be told to change anything: it re-ran `gen.text` verbatim. It now takes an optional body -- `text`, `language`, `instruct`, `seed` -- and omitted fields reuse the generation's settings. No body at all behaves exactly as before. Versions become self-describing ------------------------------- `generation_versions` held only `audio_path` and `effects_chain`, so a corrected take was unattributable: the generation row would describe the newest text while older takes still pointed at audio of the old text, with nothing recording which was which. Nullable `text`, `language`, `instruct` and `seed` columns fix that. NULL means "same as the generation", so every existing row is already correct and no backfill is needed, and only a field the caller actually overrode is written -- "unchanged" and "explicitly set to the same value" stay distinguishable. The generation row is never rewritten. It keeps the text as first written and each take records what produced it, so the take with the typo stays playable and stays attributable rather than becoming audio nobody can account for. That is what makes A/B between takes meaningful. Seed ---- A regenerate normally drops the seed so the take varies. An explicitly passed seed now survives, since that is the caller asking for a specific result -- usually to reproduce a take they liked. Not included ------------ The timeline side of jamiepine#870 -- rippling the following clips by the duration delta after a correction changes a segment's length -- is not here. It belongs in the story routes, which jamiepine#1007 is currently rewriting; adding it on top of main would conflict for no benefit. Filed as a follow-up on that PR. 12 tests: the corrected text reaching the engine, the generation row keeping the original, the take recording its own settings, unoverridden fields staying NULL, plain regenerates recording nothing, takes accumulating rather than replacing, both seed rules, and the API surface including no-body compatibility. Refs jamiepine#870 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe regeneration API accepts optional text, language, instruction, and seed overrides. The generation pipeline passes resolved values to TTS and stores explicit overrides on new versions. Database migrations, response models, and tests support the new metadata. ChangesRegeneration overrides
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant regenerate_generation
participant TaskManager
participant run_generation
participant TTSEngine
participant create_version
Client->>regenerate_generation: submit RegenerateRequest
regenerate_generation->>TaskManager: start resolved regeneration values
TaskManager->>run_generation: execute with version_overrides
run_generation->>TTSEngine: generate audio with resolved settings
run_generation->>create_version: persist explicit override metadata
create_version-->>Client: return version metadata
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/test_regenerate_overrides.py`:
- Around line 221-226: Update test_regenerate_still_accepts_no_body to require a
200 status from the no-body regeneration request, replacing the permissive
200-or-400 assertion while preserving the existing response-text diagnostic.
🪄 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: 022d1cba-1338-4cb5-9f4d-9917fc1cfa9e
📒 Files selected for processing (7)
backend/database/migrations.pybackend/database/models.pybackend/models.pybackend/routes/generations.pybackend/services/generation.pybackend/services/versions.pybackend/tests/test_regenerate_overrides.py
…ueued job `assert r.status_code in (200, 400)` passed when the thing the test exists to protect was broken. The generation is created `completed`, so a 400 means the no-body compatibility path failed -- the hedge made the assertion unfalsifiable for its own subject. Requiring 200 exposed a second problem: the route enqueues the work and returns, so the background task outlives the test and its monkeypatched backend, and then reaches for the real model. The run went from 5s to 39s. Cancelling the generation after asserting the response keeps it at 6s; the response is what this test covers, and the generation path itself is asserted in the tests above. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Author
|
Correct, and it was the worst shape a test can have. Fixed in
Tightening it to |
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
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #870.
That issue asks to fix a typo in a story segment and re-run it. Today that means building a whole new generation — which loses the timeline placement, fades, speed and track the old one had. A one-character correction costs the arrangement around it.
Most of this already existed
POST /generate/{id}/regeneratealready saved its output as a new version under the same generation id, and story items can already be pinned to a version. It just couldn't be told to change anything — it re-rangen.textverbatim.It now takes an optional body:
curl -X POST localhost:17493/generate/{id}/regenerate \ -d '{"text": "He plays a bandeja, not a smash."}'text,language,instruct,seed— omitted fields reuse the generation's settings, and no body at all behaves exactly as before.Versions become self-describing
generation_versionsheld onlyaudio_pathandeffects_chain, so a corrected take was unattributable: the generation row would describe the newest text while older takes still pointed at audio of the old text, with nothing recording which was which.Four nullable columns fix that. NULL means "same as the generation" — so every existing row is already correct and needs no backfill, and only a field the caller actually overrode is written. That keeps "unchanged" and "explicitly set to the same value" distinguishable rather than collapsing them.
The generation row is never rewritten. It keeps the text as first written; each take records what produced it. The take with the typo stays playable and attributable instead of becoming audio nobody can account for — which is what makes A/B between takes mean anything.
Seed
A regenerate normally drops the seed so the take varies. An explicitly passed seed now survives, since that's the caller asking for a specific result — usually to reproduce a take they liked.
Deliberately not included
The timeline half of #870 — rippling the following clips by the duration delta when a correction changes a segment's length — isn't here. It belongs in the story routes, which #1007 is currently rewriting; adding it on top of
mainwould conflict for no benefit. It's the natural follow-up once #1007 lands, which is also why this saysRefsrather thanCloses.Tests
12 in
backend/tests/test_regenerate_overrides.py: corrected text reaching the engine, the generation row keeping the original, the take recording its own settings, unoverridden fields staying NULL, plain regenerates recording nothing, takes accumulating rather than replacing, both seed rules, and the API surface including no-body compatibility.The model is mocked — what's under test is which strings go where, and loading 3.5 GB of weights wouldn't make those assertions any truer.
Branched off
main, independent of my other open PRs.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes