fix(voice): stop treating the word "no" as the abbreviation "No." - #927
fix(voice): stop treating the word "no" as the abbreviation "No."#927philluiz2323 wants to merge 2 commits into
Conversation
ABBREVIATIONS is matched case-insensitively, and it carried "no" (for
the number sign "No.") and "fig" (for "Fig."). Both are also ordinary
English words that really do end sentences, so any clause closing on one
never confirmed its boundary.
Both voice paths share ends_with_abbreviation, so the damage was
two-sided:
* format::truncate_for_voice silently over-ran its 3-sentence cap.
"The answer is no. The lights are off. I checked the kitchen. And
the hall." emitted all four sentences, while the identical text with
"yes" correctly stopped at three.
* SentenceStreamer withheld the clause from Piper until the *next*
terminator arrived, so first-audio latency was paid for a false
abbreviation on one of the most common short answers a home
assistant gives.
Deleting the entries outright would regress the real abbreviation
("See No. 5 below." would split mid-reference), so match these
case-sensitively in their canonical capitalized form instead: "No."/
"Fig." stay abbreviations, "no."/"fig."/"NO." are words and close the
sentence. Every other entry keeps its case-insensitive match, so
"Dr."/"dr."/"p.m." are unaffected.
A sentence-initial "No." is still not split out on its own, but that is
MIN_SENTENCE_CHARS doing the work — a 3-char clause is below the floor
in both callers (5 here, 8 in the streamer) and merges into the next
clause regardless, exactly as before.
Closes GeniePod#925
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe voice formatter now treats only capitalized ChangesVoice abbreviation boundaries
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 `@crates/genie-core/src/voice/format.rs`:
- Around line 358-362: Add an assertion in the ends_with_abbreviation test cases
covering the uppercase “FIG.” sentence ending, alongside the existing lowercase
“fig.” and uppercase “NO.” assertions, and verify it is treated as a sentence
terminator.
🪄 Autofix (Beta)
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: 669d2699-cb04-4f7d-b0f5-43ee67e43660
📒 Files selected for processing (2)
crates/genie-core/src/voice/format.rscrates/genie-core/src/voice/streaming.rs
Follow-up to the review on GeniePod#927. The case-sensitive list is matched verbatim, so every non-canonical casing is the ordinary word and must close the sentence. "NO." was asserted; "FIG." and the mixed-case "nO." were not, even though they run through the same compare. Adding them pins the whole rule rather than one instance of it. Tests only — no behavior change.
|
Addressed in the latest commit. Added |
Summary
Closes #925.
ABBREVIATIONSis matched case-insensitively and carried"no"(for the number sign
No.) and"fig"(forFig.). Both are also ordinaryEnglish words that really do end sentences, so any clause closing on one never
confirmed its boundary.
ends_with_abbreviationis shared withvoice::streaming::SentenceStreamer, sothe damage was two-sided.
Truncation.
for_voicecaps voice replies at 3 sentences — unless the firstclause ends in "no":
The answer is no. The lights are off. I checked the kitchen. And the hall.The answer is yes. The lights are off. I checked the kitchen. And the hall.The only difference between those two lines is
novsyes.Streaming. Feeding
"The answer is no. The lights are off."emitted nothingat the
no.boundary — the first sentence was withheld from Piper until thenext terminator arrived. That is first-audio latency paid for a false
abbreviation, on one of the most common short answers a home assistant gives.
A sentence-initial
No.masks this in casual testing: a 3-char clause isbelow
MIN_SENTENCE_CHARSin both callers (5 informat, 8 instreaming) andmerges into the next clause anyway. The bug only surfaces when the word ends a
clause long enough to stand alone.
Changes
ABBREVIATIONSinto a newCAPITALIZED_ABBREVIATIONSmatched case-sensitively in their canonicalform.
No./Fig.stay abbreviations;no./fig./NO.are words andclose the sentence.
See No. 5 below.would then split mid-reference. There is a test guarding exactly that.
Dr./dr./p.m.are unaffected.
voice::format(helper + bothfor_voicebehaviours) andin
voice::streaming(the shared-helper latency path).Real Behavior Proof
Tested profile / hardware (check all that apply):
jetsonraspberry_piportable_sbclaptopmacWhat I ran
x86_64 Linux laptop (Ubuntu 22.04 LTS, kernel 6.8.0-136-generic,
rustc 1.96.0 / cargo 1.96.0). No Jetson and no Piper/audio device available to
me — see the validation gap below.
Plus a temporary probe calling
for_voicedirectly onmainover thepaired "no"/"yes" inputs, so the difference was observed rather than inferred.
The probe was removed before committing; only the permanent tests remain.
What I observed
Probe output on
main— same sentence, one word different, two differenttruncation results:
The "no" line returns all four sentences; the "yes" line correctly stops at
three. On this branch both stop at three. Controls on
mainbehaved correctlyin the same probe run (
Ask Dr. Smith about it.andIt costs 3.50 dollars.both truncated to three), which is what isolates the abbreviation list as the
cause rather than the terminator logic.
All four new tests pass here and fail on
main. Full suite:1005 lib tests + all integration suites green on default features and on
--no-default-features; clippy clean on both;cargo fmt --checkclean.Validation gap
I could not verify on Jetson hardware or with a real Piper voice. The
equivalent verification path: both changed call sites are pure text→text (a
&strpredicate and aStringtransform) with no I/O, no async, no audiodependency and no model involvement. The streaming test drives the real
SentenceStreamerthroughfeed()and asserts which sentences are handed tothe TTS channel, which is precisely the boundary the Jetson path would exercise
— what happens downstream of that channel is unchanged by this diff.
Test plan
mainalone —the_word_no_is_not_an_abbreviationand
splits_at_a_clause_ending_in_the_word_nofail.beginning "The answer is no." and confirm audio starts at the first clause
rather than after the second.
Notes for reviewers
Figis included because it is the same defect, not scope creep:figis alowercase English noun in exactly the way
nois, and it sits in the samelist behind the same case-insensitive compare.
is untouched.
Summary by CodeRabbit
Bug Fixes
Tests