fix(quick-router): keep a named qualifier on a freezer status target - #886
fix(quick-router): keep a named qualifier on a freezer status target#886rsnetworkinginc wants to merge 1 commit into
Conversation
"Is the basement freezer on" asks about the basement freezer, but the freezer branch special-cased garage and canonicalized every other target to the bare "freezer", silently swapping in a different appliance — the same unconditional collapse the dryer branch fixed. Preserve a qualifier that precedes the device word, keyed on the word position exactly like the dryer branch, so "basement freezer"/"kitchen freezer" survive while the bare and garage forms are unchanged.
📝 WalkthroughWalkthroughThe freezer status router now preserves explicit qualifiers such as “basement” and “kitchen,” while retaining existing mappings for bare and garage freezer queries. A regression test covers all three cases. ChangesFreezer status routing
Estimated code review effort: 2 (Simple) | ~10 minutes 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.
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/quick.rs`:
- Around line 2776-2794: Update the earlier freezer “too warm” fast path around
the existing qualifier-preserving logic so qualified requests such as “basement
freezer” retain their qualifier instead of returning bare “freezer.” Reuse the
same device-position qualification behavior used by this branch, and add
regression coverage for qualified “too warm” requests, including the plural
“freezers” form.
🪄 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: 9dac62a2-47b1-4b89-a96b-f9d60de4f1d3
📒 Files selected for processing (1)
crates/genie-core/src/tools/quick.rs
| if contains_any(&target, &["freezer", "garage freezer"]) { | ||
| return Some(if target.contains("garage") { | ||
| // Keep a qualifier the caller named, like the sibling branches here | ||
| // (switches, covers, locks, lights, dryer) already do: this branch | ||
| // special-cased "garage" and canonicalized every other target | ||
| // unconditionally, so "is the basement freezer on" reported the bare | ||
| // "freezer" — a different appliance from the one asked about. Key on | ||
| // the device word's position, exactly like the dryer branch above: a | ||
| // qualifier precedes the device ("basement freezer"), while a leftover | ||
| // state word trails it, so only a genuinely qualified target is | ||
| // preserved. The word-order variant "freezer in the garage" still | ||
| // canonicalizes through the garage arm. | ||
| let names_a_qualified_freezer = target.split_whitespace().count() > 1 | ||
| && matches!( | ||
| target.split_whitespace().next_back(), | ||
| Some("freezer" | "freezers") | ||
| ); | ||
| return Some(if names_a_qualified_freezer { | ||
| target | ||
| } else if target.contains("garage") { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Qualified “too warm” requests still lose their qualifier.
The earlier freezer fast path at Line 2479 through Line 2484 returns "freezer" or "garage freezer" before this branch runs. Thus, "Is the basement freezer too warm?" still targets the bare freezer, and the new regression test does not catch it. Reuse this qualifier-preserving logic there and add qualified too warm/freezers coverage.
🤖 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 2776 - 2794, Update the
earlier freezer “too warm” fast path around the existing qualifier-preserving
logic so qualified requests such as “basement freezer” retain their qualifier
instead of returning bare “freezer.” Reuse the same device-position
qualification behavior used by this branch, and add regression coverage for
qualified “too warm” requests, including the plural “freezers” form.
matedev01
left a comment
matedev01
left a comment
There was a problem hiding this comment.
Confirmed CodeRabbit's finding directly: route("is the basement freezer too warm") still returns home_status{entity:"freezer"}, dropping the qualifier. The earlier 'too warm' fast path (crates/genie-core/src/tools/quick.rs ~line 2478, text.contains("freezer") && text.contains("too warm")) runs before this PR's new qualifier-preserving branch and only special-cases 'garage', so any other qualifier (basement, garage, upstairs, ...) is silently lost for that phrasing — the exact bug this PR fixes for the plain 'is the ... freezer on' shape. Worth reusing the same qualifier-preserving logic there, with a regression case for a qualified 'too warm' query.
2 participants
Summary
"Is the basement freezer on?" asks about the basement freezer, but
home_status_target's freezer branch special-cases "garage" and canonicalizes every other target to the barefreezer— the caller's qualifier is silently dropped and a different appliance is reported. This is the same unconditional collapse the dryer branch fixed: preserve a qualifier that precedes the device word, keyed on the word's position exactly like that branch, so genuine qualifiers survive while leftover state words still canonicalize. Closes #885.mainIs the basement freezer on?home_status{entity:"freezer"}home_status{entity:"basement freezer"}check the basement freezerhome_status{entity:"freezer"}home_status{entity:"basement freezer"}Is the kitchen freezer on?home_status{entity:"freezer"}home_status{entity:"kitchen freezer"}Is the freezer on?home_status{entity:"freezer"}Sarah: Is the garage freezer too warm?home_status{entity:"garage freezer"}Changes
freezer/freezers) — a qualifier precedes the device ("basement freezer"), while a leftover state word trails it, so only a genuinely qualified target is preserved. This is the exact shape the dryer branch above uses for the same bug, and the behavior the sibling switches/covers/locks/lights branches already have.garage freezerthrough it, and the priority too-warm path keeps producinggarage freezer.freezer_status_keeps_a_named_qualifier: basement/kitchen phrasings preserve their qualifier, and the bare and garage forms keep their current entities.Real Behavior Proof
Tested profile / hardware (check all that apply):
jetsonraspberry_piportable_sbclaptopmacWhat I ran
x86_64 Linux dev machine (laptop profile),
rustc 1.97.1, branch cut from9e54db4. 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 the freezer phrasings 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 (fix reverted, test kept):
After the fix: basement/kitchen phrasings resolve to their qualified entity; the bare and garage forms are byte-for-byte unchanged.
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.GENIE_RUN_RELEASE_TESTS=1 cargo test --workspace --locked --all-targets— fully green: every workspace target ok (genie-core lib 966 passed / 6 ignored,tools::quick108/108, genie-common 137/137, and the release-gatedtool_dispatch_testtarget 18/18 withbinary_size_budgetpassing). One documented load flake (genie-common tegrastats::mem_available_mb_async_matches_sync_version) failed once while a second workspace build ran concurrently and passes clean in isolation and in the full quiet-machine pass; it is untouched by this diff.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). Predictions byte-identical tomain(cmpclean against a freshmainbaseline).--no-default-featurestest forgenie-core/genie-ctl— green.genie-corebinary: 7,079,992 bytes vs. the 7,079,800-bytemainbaseline built with the same toolchain — +192 bytes for the added position test.Jetson gap: no Jetson hardware and no
aarch64cross toolchain on this box, so the cross build is left to theCross-compile (aarch64 / Jetson)CI job. The change is a pure word-level&strcomparison 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 freezer_status_keeps_a_named_qualifier— passes here, fails onmain(the first assertion reportsfreezerwherebasement freezeris expected).cargo test -p genie-core --lib tools::quick— the other 107 router tests are unchanged and green, including the existing garage-freezer tests (routes_weather_and_home_status_before_memory_recall, the too-warm priority path) and the BFCLhome-status-freezercase.Notes for reviewers
memory_recallonmainby design (the semantic household matcher claims it), and "freezer in the garage" is claimed by the garage/covers word branch earlier in the function — both are deliberately untouched; the new test documents where the garage guardrails actually live.freezer && too warmarm keeps its own garage/bare split; extending qualifier retention to that text-level arm would need status-target cleaning it doesn't have, so it is left alone (same scoping call as the dryer fix made for the package/delivery branch).Summary by CodeRabbit
Bug Fixes
Tests