fix(quick-router): keep a named qualifier on a dryer status target - #846
Conversation
|
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 ChangesDryer status routing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
This now conflicts with main in crates/genie-core/src/tools/quick.rs — several other quick-router fixes landed just ahead of this one. Please rebase onto main and push; I'll review once it's mergeable. |
The dryer branch of home_status_target canonicalized every match to the
bare laundry "dryer", so "is the hair dryer on" and "is the basement
dryer on" both reported a different appliance in a different room than
the one asked about. Every sibling branch (switches, thermostat, covers,
locks, lights, freezer) already keeps the qualifier the caller named.
Key on the device word position rather than a plain word count: a
qualifier precedes the device ("hair dryer") while a leftover state word
trails it ("dryer done", since " done" is not a STATUS_SUFFIXES entry),
so the bare device, the "drying machine" synonym and the state-word form
all still canonicalize to "dryer".
10ac210 to
3aa3f24
Compare
matedev01
left a comment
There was a problem hiding this comment.
LGTM — the dryer branch canonicalized every match to the bare laundry 'dryer', so 'is the hair dryer on' / 'is the basement dryer on' reported a different appliance in a different room than the one asked about. Keys on word position (qualifier precedes the device word vs. a trailing state word like 'done' follows it) to correctly distinguish a genuine qualifier from the 'dryer done' edge case — nice catch that a plain word-count test would have broken that. Verified: clippy -D warnings clean, quick-router tests (102) pass, fmt clean, live BFCL strict_accuracy 96.15% (no regression).
Summary
The dryer branch of
home_status_targetcanonicalizes every match to the bare laundry"dryer", discarding any qualifier the caller named — so a question about the hair dryer in the bathroom returns a status readout for the laundry dryer, a different appliance in a different room, with no indication the assistant answered about something else. Every sibling device branch in the same function already keeps the qualifier. Closes #845.mainis the hair dryer onhome_status{entity:"dryer"}home_status{entity:"hair dryer"}is the hand dryer onhome_status{entity:"dryer"}home_status{entity:"hand dryer"}is the basement dryer onhome_status{entity:"dryer"}home_status{entity:"basement dryer"}For contrast, the sibling branches on
maintoday:is the kitchen plug on→"kitchen plug",is the side door locked→"side door",is the front gate closed→"front gate",is the garage freezer too warm→"garage freezer".Changes
"dryer"only for an unqualified target, and pass a qualified one through — the same behaviour the switches, thermostat, covers, locks, lights and freezer branches already have.split_whitespace().count() == 1test is not directly reusable here:" done"is not one of theSTATUS_SUFFIXESthatclean_status_targetstrips, sois the dryer donecleans to the two-word target"dryer done"and a plain count check would emit that garbled entity wheremaincorrectly emits"dryer". Key on the device word's position instead — a qualifier precedes the device (hair dryer), a leftover state word trails it (dryer done) — which distinguishes the two cases and, as a bonus, still canonicalizes the"drying machine"synonym since it does not end in the device word.dryer_status_keeps_a_named_qualifier: three qualified devices keep their qualifier; the bare device,check the dryer, thedrying machinesynonym and thedryer donestate-word form all still resolve to"dryer".One branch body changed; every other path in
home_status_targetis untouched.Real Behavior Proof
Tested profile / hardware (check all that apply):
jetsonraspberry_piportable_sbclaptopmacWhat I ran
x86_64 Linux dev machine (laptop profile),
rustc 1.94.0, branch cut from7c67906. The changed path is pure string routing insidequick::route— no audio, Home Assistant, or hardware dependency — soroute()tests exercise it end-to-end. I probedroute()over qualified/unqualified dryer phrasings and the sibling branches against unmodifiedmainfirst, then wrote the regression test and confirmed it fails before applying the fix.What I observed
Probe on unmodified
main:Repro before the fix:
After the fix:
hair dryer/hand dryer/basement dryerkeep their qualifier;is the dryer on,check the dryer,is the drying machine onandis the dryer doneall still return"dryer", byte-identical tomain.Gate results on this branch:
cargo fmt --all -- --check— clean.cargo clippy --workspace --all-targets --locked -- -D warnings— clean;--no-default-featuresclippy forgenie-core/genie-ctl— clean.cargo test --workspace --locked --all-targets—tools::quick98 passed / 0 failed, genie-core lib 938 passed / 0 failed / 6 ignored, every other target green.cargo test --workspace --locked --doc— green.strict_accuracy: 96.15%(25/26) against the--min-strict 96floor, the single holdout being the documented two-callmulti-homework-timercase (quick-router multi-intent prompts emit a single, wrong-priority tool #533). Byte-identical tomain.--no-default-featurestest forgenie-core/genie-ctl— green.binary_size_budgetfails identically on unmodifiedmainwith my localrustc 1.94.0(6.80 MBagainst the6.8 MBbudget), so it is a toolchain artifact on my box rather than a regression here. I measured both sides to be sure:main= 7,131,736 bytes, this branch = 7,131,160 bytes — 576 bytes smaller.Jetson gap: no Jetson hardware and no
aarch64cross toolchain on this box (cargo check --target aarch64-unknown-linux-gnustops atcc-rs: failed to find tool "aarch64-linux-gnu-gcc"), so the cross build is left to theCross-compile (aarch64 / Jetson)CI job. The change is a pure&strtoken comparison with no platform-dependent behaviour, no I/O, and no new dependency, so it is architecture-independent by construction.Test plan
cargo test -p genie-core --lib dryer_status_keeps_a_named_qualifier— passes here, fails onmain.cargo test -p genie-core --lib tools::quick— the other 97 router tests, including the siblingcover_and_gate_status_match_whole_words_not_substringsandstatus_entity_drops_both_state_word_and_time_qualifier, are unchanged.Notes for reviewers
" done"(and friends) toSTATUS_SUFFIXESand then use the plain sibling word-count test everywhere. I deliberately did not do that here:STATUS_SUFFIXESis shared by every device branch, so widening it is a much larger blast radius than this one-branch bug warrants, and it deserves its own PR with its own regression sweep. Happy to follow up with it if you'd like.package/deliverybranch canonicalizes unconditionally in the same way, but its two keywords are not device names a user qualifies by room, so the exposure is different and I left it alone.Summary by CodeRabbit