fix(quick-router): accept a leading please on home_control commands - #922
Open
galuis116 wants to merge 2 commits into
Open
fix(quick-router): accept a leading please on home_control commands#922galuis116 wants to merge 2 commits into
galuis116 wants to merge 2 commits into
Conversation
Every home_control matcher is anchored at token 0 — the curated exact-match device set, the starts_with arms, the `set `/`preheat ` setpoint parser, and simple_turn_request's "turn on "/"turn off " prefixes. A single leading politeness token shifted that anchor, so the most common polite actuation forms fell through to the LLM even though their bare counterparts already route: "please turn off the kitchen lights" -> ABSTAIN "please set the thermostat to 68" -> ABSTAIN "please preheat the oven to 400" -> ABSTAIN "please start the dishwasher" -> ABSTAIN Strip a single leading "please " at the two home-control entry points, mirroring scene_or_routine_activation_request and play_media_request, which already strip it for the same reason. A trailing "please" was already dropped by clean_control_entity. The strip happens before the guards rather than inside each arm, so the "in <duration>" schedule guard, the multi-clause/except/only/and abstention, and the known-device gate all still apply to the stripped text — "please turn on the lights in 5 minutes" keeps abstaining rather than actuating now. priority_home_control_request gets the same strip: its arms are mostly contains tests that survive a leading token, but "run movie night" and "warm up the car" are exact matches that do not. The scene test's negative assertion for "please turn on the lights" is updated: it was asserting the absence of a route, and its point — that a device command is not claimed as a scene activation — now holds by the call landing on home_control instead. Closes GeniePod#919
|
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 quick router removes one leading ChangesHome-control politeness handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/genie-core/src/tools/quick.rs (1)
1405-1411: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the priority helper.
The new tests exercise standard
home_control_requestcases throughroute(...), but do not prove thatpriority_home_control_requesthandles a leadingplease. Add a direct test or a route phrase unique to this helper, such asplease run movie night, so this second entry point is regression-tested.Suggested test
+#[test] +fn priority_home_control_accepts_a_leading_please() { + assert_eq!( + priority_home_control_request("please run movie night"), + Some(("movie night".into(), "activate", None)) + ); +}🤖 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/quick.rs` around lines 1405 - 1411, Add regression coverage for priority_home_control_request by testing a leading “please” input, such as “please run movie night,” and asserting it routes identically to the bare command. Keep the test focused on this helper rather than only covering home_control_request through route(...).
🤖 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/quick.rs`:
- Around line 1405-1411: Add regression coverage for
priority_home_control_request by testing a leading “please” input, such as
“please run movie night,” and asserting it routes identically to the bare
command. Keep the test focused on this helper rather than only covering
home_control_request through route(...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d9be7c03-6e70-42da-bc2f-7d0d7f65c192
📒 Files selected for processing (1)
crates/genie-core/src/tools/quick.rs
Follow-up to the review on GeniePod#922. The existing tests exercise home_control_request through route(), but nothing proved priority_home_control_request handles a leading "please". That is the second entry point and it runs earlier in dispatch, so it needs its own coverage rather than inheriting confidence from the other. "please run movie night" is the case that matters: almost every arm in that helper is a `contains` test that survives a leading token, and this exact-match arm is the reason the strip had to be added there at all. The test also asserts the polite call equals the bare call by name and arguments, not merely that some route exists. "warm up the car" is the other exact-match arm and is deliberately not tested: it returns None on main as well, because its remote_start action does not survive canonicalize_household_action, so a leading "please" changes nothing there. Noted in the test comment so the omission reads as a decision rather than an oversight. Tests only — no behavior change.
Contributor
Author
|
Addressed in the latest commit — added One note on scope: |
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.
Summary
Closes #919. Every
home_controlmatcher in the quick-router is anchored attoken 0, so a single leading
pleaseshifted the anchor and the most commonpolite actuation forms fell through to the LLM — even though their bare
counterparts already route. This was the last unguarded intent family: scene/
routine (#894), play_media, shopping list (#843), news (#908), and time/date
(#906) all strip a leading
pleasealready;home_control, thehighest-frequency actuation intent, did not.
Please turn off the kitchen lightshome_control{kitchen lights, turn_off}Please turn off the lightshome_control{lights, turn_off}Please turn on the bedroom lamphome_control{bedroom lamp, turn_on}Please turn on the fanhome_control{fan, turn_on}Please turn off the TVhome_control{tv, turn_off}Please start the dishwasherhome_control{dishwasher normal cycle, activate}Please set the thermostat to 68home_control{thermostat, set_temperature, 68}Please preheat the oven to 400home_control{oven, set_temperature, 400}Changes
pleaseat the top ofhome_control_request,mirroring
scene_or_routine_activation_requestandplay_media_request,which strip it for exactly this reason. A trailing
pleasewas alreadydropped by
clean_control_entity.priority_home_control_request. Most of its arms arecontainstests that survive a leading token, butrun movie nightandwarm up the carare exact matches that do not — and the two entry pointsshould not disagree about politeness.
every existing guard keeps applying to the stripped text: the
in <duration>schedule guard, the multi-clause /
except/only/andabstention, andthe known-device gate.
home_control_accepts_a_leading_pleasecovering theeight polite forms above plus four abstention negatives.
scene_activation_accepts_a_leading_please. It readroute("please turn on the lights").is_none(). Its stated point is that adevice command must not be claimed as a scene activation — that still
holds, and now holds more strongly, because the call falls past the scene
matcher and lands on
home_control{lights, turn_on}. The assertion nowchecks that outcome instead of the absence of any route.
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 live Home Assistant instance
available to me — see the validation gap below.
Plus a temporary probe binding
route()directly over the affected utterances,to read the emitted
ToolCallrather than trusting the assertions alone. Theprobe was removed before committing; only the permanent regression test
remains.
What I observed
cargo test -p genie-core: 1005 lib tests passed, 0 failed, 6 ignored, andall integration suites green.
home_control_accepts_a_leading_pleasepasseshere; on
mainit fails on the first case.cargo clippy --all-targetsproduced no warnings;
cargo fmt --checkproduced no diff.Probe output on this branch:
Every one of the first five printed
ABSTAINonmain. The last two are theguards: the schedule guard and the known-device gate still fire on the polite
form, so stripping
pleasedoes not trade one wrong answer for another.Validation gap
I could not verify on Jetson hardware or against a live Home Assistant. The
equivalent verification path: this change is confined to
crates/genie-core/src/tools/quick.rs, a pure string→ToolCallfunction withno I/O, no async, no hardware dependency, and no model involvement. Its entire
contract is which
ToolCallcomes out for a given utterance, which the testsabove assert directly. The emitted call is byte-identical to the one the bare
utterance already produces and already dispatches on Jetson today — so the HA
dispatch path downstream sees nothing it has not seen before. Nothing in the
diff touches audio, the dashboard, or the model prompt.
Test plan
git checkout main && cargo test -p genie-core --lib home_control_accepts_a_leading_please— fails (test absent on main; cherry-pick the test alone to see it fail).lights" and confirm the light actuates without an LLM round-trip, then say
"please turn on the lights in 5 minutes" and confirm it still falls through
to the LLM rather than firing now.
Notes for reviewers
is untouched.
please.pleaseis stripped, deliberately —please please turn off the lightsis not a real utterance, and a loop would be scope the bug doesnot justify.
Summary by CodeRabbit