Skip to content

fix(quick-router): compose a digit with a following magnitude word - #825

Open
jaytbarimbao-collab wants to merge 1 commit into
GeniePod:mainfrom
jaytbarimbao-collab:fix/spoken-number-digit-magnitude
Open

fix(quick-router): compose a digit with a following magnitude word#825
jaytbarimbao-collab wants to merge 1 commit into
GeniePod:mainfrom
jaytbarimbao-collab:fix/spoken-number-digit-magnitude

Conversation

@jaytbarimbao-collab

@jaytbarimbao-collab jaytbarimbao-collab commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Summary

parse_spoken_number's digit fast-path returned on the first integer token and
never consumed a following hundred/thousand, so a mixed digit + magnitude
utterance — how STT commonly renders round numbers ("100 thousand") — was
emitted as two tokens and produced a garbled, un-evaluatable expression:

Utterance Before Expected
what is 100 thousand divided by 4 100 1000 / 4 100000 / 4
what is 20 thousand times 2 20 1000 * 2 20000 * 2
what is 5 hundred plus 10 5 100 + 10 500 + 10

This is the same garble the all-word path already handles
(routes_compound_spoken_cardinals_to_calculate documents "one hundred / five"
once becoming "1 100 / 5"); the digit + magnitude form still slipped through
the fast-path.

Changes

  • parse_spoken_number (crates/genie-core/src/tools/number_words.rs): in the
    digit fast-path, scale the value by an immediately following hundred/
    thousand and consume that word, instead of returning the bare digit. A digit
    with no magnitude word after it still returns as-is.
  • Add routes_digit_with_magnitude_word_to_calculate.

Real Behavior Proof

Verified locally against the real route() (x86 dev host — pure input parsing,
no hardware path):

route("what is 100 thousand divided by 4")  Before: "100 1000 / 4"  After: "100000 / 4"
route("what is 20 thousand times 2")        Before: "20 1000 * 2"   After: "20000 * 2"
route("what is 5 hundred plus 10")          Before: "5 100 + 10"    After: "500 + 10"

# unchanged (regression-guarded):
route("what is 100 divided by 5")        -> "100 / 5"     # bare digit, no magnitude word
route("set a 15 minute timer ...")       -> 900 seconds   # article "a" + digit not summed
route("what is one hundred divided by five") -> "100 / 5"  # all-word path unaffected

A first attempt that parsed digit tokens inside the accumulation loop regressed
"a 15 minute timer" (article a=1 absorbed the 15 → 16 min); the final fix
handles the pair only in the fast-path, leaving that case correct. Full
cargo test -p genie-core — all 34 test binaries pass; cargo fmt --check
clean; cargo clippy --all-targets clean on default and --no-default-features.

  • I have built and run the affected code locally.
  • I have verified the change end-to-end on Jetson hardware.
  • I have NOT verified on Jetson hardware; this is pure input-parsing logic with no device path, fully exercised on the laptop profile.

Tested profile / hardware:

  • jetson
  • raspberry_pi
  • portable_sbc
  • laptop

Summary by CodeRabbit

  • Bug Fixes

    • Improved spoken-number parsing for digit-and-magnitude combinations such as “5 hundred” and “20 thousand.”
    • Calculator requests now correctly interpret mixed numeric phrases while preserving existing behavior for standalone digits.
  • Tests

    • Added coverage for calculations using “hundred” and “thousand” expressions.

`parse_spoken_number`'s digit fast-path returned on the first integer token and
never consumed a following "hundred"/"thousand", so a mixed digit + magnitude
utterance — how STT commonly renders round numbers — was emitted as two tokens:

- "100 thousand divided by 4" -> "100 1000 / 4"  (should be "100000 / 4")
- "20 thousand times 2"       -> "20 1000 * 2"   (should be "20000 * 2")
- "5 hundred plus 10"         -> "5 100 + 10"    (should be "500 + 10")

This is the same garble the all-word path already handles
(`routes_compound_spoken_cardinals_to_calculate`: "one hundred / five" was once
"1 100 / 5"); the digit + magnitude form still slipped through.

- `parse_spoken_number` (`crates/genie-core/src/tools/number_words.rs`): in the
  digit fast-path, scale the value by an immediately following "hundred"/
  "thousand" and consume that word, instead of returning the bare digit. A digit
  with no magnitude word after it still returns as-is, so "100 divided by 5" and
  "a 15 minute timer" (article "a" + digit) are unchanged.
- Add `routes_digit_with_magnitude_word_to_calculate`.
@github-actions github-actions Bot added the bug Something isn't working label Jul 22, 2026
@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

parse_spoken_number now combines digit tokens with immediately following magnitude words, scaling them by 100 or 1,000. Calculator routing tests cover mixed digit-and-magnitude inputs and preserve bare-digit behavior.

Changes

Digit magnitude parsing

Layer / File(s) Summary
Combine digits with magnitude words
crates/genie-core/src/tools/number_words.rs, crates/genie-core/src/tools/quick.rs
parse_spoken_number consumes and scales digit-plus-hundred/thousand tokens, while routing tests verify expanded calculator expressions and unchanged bare-digit handling.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested labels: bug

Suggested reviewers: carlos4s, ultrahighsuper

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% 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 accurately summarizes the main change: combining a digit with a following magnitude word in the quick router.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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/tools/number_words.rs`:
- Around line 16-20: Update the number parsing logic around the match on
tokens.get(start + 1) so hundred and thousand values initialize the existing
parser state with the scaled value, then continue parsing from start + 2 to
consume suffixes such as twenty, 300, or and 5. Retain the immediate return path
only for bare digits.
🪄 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: b1982df1-d4ed-41d2-bbdf-6ffdff30cb88

📥 Commits

Reviewing files that changed from the base of the PR and between ead880e and 6d91053.

📒 Files selected for processing (2)
  • crates/genie-core/src/tools/number_words.rs
  • crates/genie-core/src/tools/quick.rs

Comment on lines +16 to +20
return Some(match tokens.get(start + 1) {
Some(&"hundred") => (value.saturating_mul(100), start + 2),
Some(&"thousand") => (value.saturating_mul(1000), start + 2),
_ => (value, start + 1),
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

The early return truncates mixed cardinal numbers.

Returning after digit + magnitude leaves suffixes such as twenty, 300, or and 5 unconsumed, producing malformed expressions like 500 20 and 2000 300. Initialize the existing parser state from the scaled value and continue parsing from start + 2; retain the immediate return only for bare digits.

🤖 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 16 - 20, Update the
number parsing logic around the match on tokens.get(start + 1) so hundred and
thousand values initialize the existing parser state with the scaled value, then
continue parsing from start + 2 to consume suffixes such as twenty, 300, or and
5. Retain the immediate return path only for bare digits.

@matedev01 matedev01 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Confirmed CodeRabbit's finding directly (added a throwaway test to check the actual return value, not just reading the code):

  • parse_spoken_number(&["100", "thousand", "300"], 0) returns (100000, 2), leaving "300" completely unconsumed.
  • parse_spoken_number(&["5", "hundred", "and", "20"], 0) returns (500, 2), leaving "and 20" dangling.

The early return after composing digit+magnitude stops before the existing while-loop's suffix handling ever runs, so a triple STT rendering like "one hundred thousand three hundred" → "100 thousand 300" produces a malformed/un-evaluatable expression instead of 100300 — which is exactly the class of input this PR sets out to fix. CodeRabbit's suggested fix is right: initialize the existing parser state (total/group/matched) from the scaled value and continue from start + 2 instead of returning immediately, keeping the immediate-return only for a bare digit with no magnitude word.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants