fix(quick-router): "turn the oven on" is answered with a status read instead of actuating - #878
Conversation
… read
The oven branch in `home_status_target` keyed on the bare phrase "oven on"
with no question-shape gate, so a command that merely contained it was
answered with a status report: "turn the oven on" and "remind me to turn
the oven on at six" both returned home_status{entity:"oven"} and the oven
never actuated. Every other appliance already abstains on this word order
— "turn the lights on", "turn the fan on" both fall through to the LLM —
so the oven was the one device where a trailing-particle command silently
failed.
The same predicate also matched "oven" inside "pr[oven]" / "w[oven]",
reporting the oven for "is that theory proven on the test bench".
Gate on the status-question shape the iron block already uses, and match
"oven" as a whole word like the cooktop branch below. Genuine status
questions, the self-cleaning entity, and the set_temperature path are
unchanged.
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe oven quick-router now requires whole-word matching and status-question syntax before returning ChangesOven status routing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 2487-2489: Update the oven-routing condition near
asks_about_the_oven so looks_like_status_query is additionally constrained to
explicit oven-state questions, rather than non-status requests that merely
contain adjacent “oven on” text. Preserve the existing “did ” handling and
self-cleaning exclusion, and add regression coverage for “What happens if I turn
the oven on?” and “Check if I should turn the oven on” to ensure they do not
route to home_status.
- Around line 2481-2486: Update the oven-status detection near
asks_about_the_oven to match “leave the oven” using separate
whitespace-delimited tokens rather than substring containment, preventing
matches within words such as “ovenware”; add a regression case covering that
false positive.
🪄 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: 55e970f9-75bf-45b8-95aa-349e7be513d0
📒 Files selected for processing (1)
crates/genie-core/src/tools/quick.rs
…ject Review follow-up. The first pass gated on looks_like_status_query, which still admitted a command embedded in a question — "what happens if i turn the oven on", "check if i should turn the oven on" — and left "leave the oven" matching inside "leave the ovenware". Admit only the two shapes that ask about the oven's state, every token matched whole: "is/are the oven ... on" with the oven as the subject (so "are you going to turn the oven on" no longer qualifies) and "did ... leave the oven ...". The generic gate is dropped entirely.
matedev01
left a comment
There was a problem hiding this comment.
LGTM — the oven branch had no question-shape gate and no word boundary, so 'turn the oven on' (a command) was answered with a status read and the oven never actuated — every other device already abstains on this word order, which made the oven the odd one out. Verified both CodeRabbit findings are genuinely fixed, not dismissed: asks_oven_state requires 'is'/'are' as the literal first word (rejects embedded/hypothetical forms like 'what happens if i turn the oven on'), and asks_oven_left_on whole-word-matches the 'leave the oven' triple (rejects 'ovenware'). Fixes #877. Verified: clippy -D warnings clean, quick-router tests (108) pass, fmt clean, live BFCL strict_accuracy 96.15% (no regression).
Fixes #877.
Root cause
home_status_target's oven branch runs ahead of thelooks_like_status_querygate and keys on a bare phrase:
So any utterance containing
"oven on"resolves to a status read — including acommand.
"Turn the oven on"returnedhome_status{entity:"oven"}, the turnnever reached the LLM, and the oven did not actuate: the user asked for an
action and got a status report.
Every other appliance already abstains on this word order and lets the LLM
ground the write (
"Turn the lights on","Turn the fan on"both fallthrough), which is what made the oven the one device where a trailing-particle
command silently failed.
The same predicate also matched
oveninsidepr[oven]/w[oven]— thesubstring problem the cooktop branch below was already fixed for in #792.
Fix
Admit only the two shapes that actually ask about the oven's state, every
token matched whole:
is/are the oven … on— the oven is the subject of the question.did … leave the oven …— the past-action check the standard status-queryprefixes do not cover.
I first gated this on
looks_like_status_query(), but review caught that itstill admits a command embedded in a question (
"what happens if I turn the oven on","check if I should turn the oven on"). Anchoring on the oven as thesubject rather than on a bare
is/areprefix is what keeps those out —"are you going to turn the oven on"no longer qualifies either. Matchingleave the ovenas three whole tokens also stops"leave the ovenware"frommatching.
One branch changed, plus a regression test. Nothing outside this predicate is
touched.
Impact
Oven actuation commands stop being swallowed by a status read and reach the LLM,
which grounds them as
home_control. Three substring false positives go away.Genuine oven status questions, the self-cleaning entity, and the
set_temperaturepath are unchanged — all asserted in the new test, androutes_shopping_and_temperature_home_requestsalready pins"Sarah: Did I leave the oven on?"."Is the oven still on?"now resolves too;it did not on
main, because the substring"oven on"is not present.What this does not fix
The interrogative-hypothetical forms —
"what happens if I turn the oven on","check if I should turn the oven on"— no longer reach this branch, but theyare still status-shaped by
looks_like_status_query, so they fall through to thecooktop branch and report
stove. That is a pre-existing gap in a differentpredicate (the same class as #861) and I left it alone rather than widen this
diff; the new test asserts only that they no longer resolve to the oven
entity. The actuation-blocking half — the imperative forms a user actually says
to control the oven — is fully fixed.
Relationship to #862
#862 (issue #861) introduces a
status_shapedgate for the bare-appliancenoun branches — water pressure, sump pump, sous vide, baby monitor, water
heater. It does not touch the
"oven on"phrase branch, which is what thischanges; the two diffs are in different hunks and do not conflict. This PR also
carries a whole-word component that #862 has no equivalent of. Happy to rebase
whichever lands second.
Risk / tradeoffs
Low, but worth naming: the gate is a genuine narrowing, so an oven status
question phrased outside the recognized shapes —
"the oven still on?"with noleading
is/did— now abstains instead of resolving. That is the sametradeoff the iron and bare-appliance branches accepted, and abstaining sends the
turn to the LLM rather than producing a wrong answer.
Real Behavior Proof
Tested profile / hardware:
laptop(Windows host — see the gap note below)What I ran
I do not have a Jetson, and
genie-coredoes not build on my Windows host(
libc::gmtime_r,tokio::signal::unixand friends are Unix-only), so therouting change was exercised two ways.
1. Direct
route()calls. I compiled the four quick-router source filesunmodified —
crates/genie-core/src/tools/{quick,home_action,calc_input,number_words}.rs,included by
#[path], with onlyToolCallandHomeActionKindstubbed — into athrowaway binary and called
tools::quick::route()directly. No logic wascopied or reimplemented.
2.
cargo test -p genie-coreon Linux via GitHub Actions, plus the rest ofthe matrix (
cargo fmt,cargo clippy -D warnings,--no-default-features,and the
aarch64-unknown-linux-gnurelease build), run on this exact commitbefore opening this PR: kai392#3 — all green.
The review follow-up commit was validated the same way:
kai392#4, also green.
What I observed
Before, on
main@ 9e54db4:After, same harness, same build:
Validation gap
The routing half is fully covered above. What I could not verify is the
downstream half: that the LLM, now that it sees
"turn the oven on", emitshome_control{entity:"oven",action:"turn_on"}and the oven actually actuatesagainst a live Home Assistant. That needs the model and an HA instance on
device. A maintainer with the
deploy/homeassistant/config up could confirmthe
off -> onstate change through the HA API.