fix(tools): scale a digit followed by a spelled magnitude in parse_spoken_number - #838
fix(tools): scale a digit followed by a spelled magnitude in parse_spoken_number#838jeffrey701 wants to merge 1 commit into
Conversation
…oken_number
The digit fast-path returned a leading digit token immediately, silently
dropping a spelled magnitude after it: parse_spoken_number(["5","hundred"])
returned (5, 1) not (500, 2). So "what is 20 percent of 5 hundred" computed on 5
("5 * 20 / 100" = 1) instead of 500 (= 100), and "5 hundred seconds" made a 100s
timer. Only the mixed digit+magnitude form was wrong; fully-spelled and
fully-digit already worked.
Seed the accumulator from the leading digit when a hundred/thousand follows so
the existing magnitude loop scales it; a bare digit still returns immediately.
Adds tests. (parse_amount has a separate twin shortcut; focused follow-up.)
📝 WalkthroughWalkthrough
ChangesSpoken number parsing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/genie-core/src/tools/number_words.rs (1)
125-150: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd calculator and duration regression tests.
These tests validate
parse_spoken_numberdirectly, but not its downstream consumers incrates/genie-core/src/tools/calc_input.rsandcrates/genie-core/src/tools/quick.rs. Add one regression case through each path to ensure"5 hundred"remains scaled after integration.🤖 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 `@crates/genie-core/src/tools/number_words.rs` around lines 125 - 150, Add integration regression tests for the calculator path in calc_input.rs and the duration path in quick.rs, each exercising the input “5 hundred” and asserting it produces the scaled value 500. Keep the existing parse_spoken_number unit tests unchanged and follow the established test helpers and assertions in each consumer.
🤖 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.
Nitpick comments:
In `@crates/genie-core/src/tools/number_words.rs`:
- Around line 125-150: Add integration regression tests for the calculator path
in calc_input.rs and the duration path in quick.rs, each exercising the input “5
hundred” and asserting it produces the scaled value 500. Keep the existing
parse_spoken_number unit tests unchanged and follow the established test helpers
and assertions in each consumer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 47a636d0-19c9-4354-8e69-c62f23345970
📒 Files selected for processing (1)
crates/genie-core/src/tools/number_words.rs
|
Closing in favor of #825, which was opened a day earlier and covers the same |
Summary
parse_spoken_numberhas a digit fast-path that returns a leading digit token immediately, silently dropping a spelled magnitude that follows it:So
parse_spoken_number(&["5", "hundred"], 0)returns(5, 1)instead of(500, 2). This produces a confidently-wrong calculator answer:route("what is 20 percent of 5 hundred")buildscalc_number_starting_atfromparse_spoken_number, gets5, and emits"5 * 20 / 100"→ 1, instead of"500 * 20 / 100"→ 100. The duration path is affected the same way ("5 hundred seconds"→ a 100s timer, not 500).Only the mixed digit + spelled-magnitude form was broken — the fully-spelled (
"five hundred"→ 500) and fully-digit ("500") forms already work, and"five hundred"is exactly what "5 hundred" means. This seeds the accumulator from the leading digit when a magnitude follows, so the existinghundred/thousandloop scales it; a bare digit with no magnitude still returns immediately, unchanged.(Note:
parse_amounthas a separate first-digit shortcut with the same blind spot on the temperature-setpoint path; that is a distinct function and left for a follow-up so this change stays focused onparse_spoken_number.)Closes #837
Real Behavior Proof
Tested profile / hardware (check all that apply):
jetsonraspberry_piportable_sbclaptopmacgenie-core is a Linux-only crate, so it doesn't compile on my Windows dev box — CI runs the real
cargo teston Linux, exercising the four added tests. Locally I reproducedparse_spoken_number(old vs new) pluscardinal_wordin arustcharness.What I ran
What I observed
The harness asserts OLD
["5","hundred"]→(5,1)(the dropped magnitude), NEW →(500,2),["3","thousand"]→(3000,2),["2","thousand","five","hundred"]→(2500,4),["5","hundred","and","twenty"]→(520,4), and no change for the bare-digit (["5","minutes"]→(5,1),["42"]→(42,1)) and fully-spelled (["five","hundred"]→(500,2)) forms.Summary by CodeRabbit
Bug Fixes
Tests