fix(quick-router): a scheduled or conditional setpoint actuates immediately - #917
Open
kai392 wants to merge 1 commit into
Open
fix(quick-router): a scheduled or conditional setpoint actuates immediately#917kai392 wants to merge 1 commit into
kai392 wants to merge 1 commit into
Conversation
… now
`home_control_request`'s setpoint path guards only a relative delay
("in an hour", GeniePod#828/GeniePod#829). Every other qualifier on the same clause was
dropped and the setpoint actuated immediately, because
`parse_amount` reads "68" straight out of "68 at 9pm":
set the thermostat to 68 at 9pm -> fires now
set the thermostat to 68 tonight -> fires now
set the thermostat to 68 when i get home -> fires now
set the lights to 40 percent except the bedroom -> dims the bedroom
The turn_on/turn_off path already refuses all of these; only the
setpoint path was missing the check. The exclusion case is the worst of
them: it actuates the device the caller just excluded, which is exactly
what `canonicalize_household_action` refuses to do for the `*_except`
verbs.
Add the absolute/event-anchored schedule guard on the value tail, and
the conditional/exclusion guard `simple_turn_request` already applies.
" and " is left out of the second list — on a setpoint it joins a spoken
compound number, not a second device.
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This was referenced Jul 30, 2026
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
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.
Fixes #914.
Root cause
#828 / #829 fixed the relative delay on the setpoint path. Every other way of
qualifying the same setpoint still actuates immediately.
home_control_request'sset/preheatbranch guards onlyin <duration>.parse_temperature_targetthen splits ontoand hands the whole remainder toparse_amount, which reads68straight out of"68 at 9pm"— the qualifyingclause is never inspected, so the setpoint fires now and the caller's actual
request is silently dropped.
The exclusion case is the worst: the room the caller excluded is dimmed anyway —
the outcome
canonicalize_household_actionexplicitly refuses to produce for the*_exceptverbs.This is a gap rather than a missing feature because the turn_on/turn_off path
already refuses all of it.
simple_turn_requesthas a multi-clause guard and itsknown_devicegate rejects a trailing schedule, so"Turn on the porch light at 9pm"and"Turn on the porch light when I get home"both abstain today. Onlythe setpoint path was missed.
Fix
Two guards on the same branch, both mirroring what already exists elsewhere in
this function:
so the device half is untouched, and only on the
tosplit, becauseparse_temperature_targetalso acceptsatas the value separator(
"set the thermostat at 68"must keep routing).simple_turn_requestapplies, minusand: on a setpoint that joins aspoken compound number (
"one hundred and five"), not a second device.Impact
Scheduled and conditional setpoints abstain so the LLM can arm them, matching the
turn path. Unqualified setpoints are unchanged — the new test pins six of them,
including the
atvalue separator and the trailing-room form the #829 testalready covers.
Risk / tradeoffs
Low, and it is a narrowing only — no utterance starts routing that did not
before.
The honest cost: the marker lists are keyword-based, so a schedule phrased
outside them (
"set the thermostat to 68 once the kids are asleep") still firesnow, and a device whose name contains a marker word would abstain. Neither is
new — both lists are the ones the turn path and #829 already live with, and I
kept them identical rather than inventing a third dialect. A grammar-level fix
(require the value tail to be a bare number plus a unit) would be more complete
but is a much larger change to
parse_temperature_target's contract, and I didnot want to bundle it here.
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 was copiedor 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, andthe
aarch64-unknown-linux-gnurelease build), run on this exact commit beforeopening this PR: kai392#6 — all green.
The second of those is the pre-existing #829 test; it still passes unchanged.
What I observed
Before, on
main@ 02a577d:For contrast, the turn path on the same qualifiers, unchanged by this PR:
After, same harness, same build — the setpoint path now matches:
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
"set the thermostat to 68 at 9pm", actually arms a schedule rather than actuating — and that the exclusionform reaches Home Assistant as a correctly-scoped call. Both need the model and a
live HA on device. With
deploy/homeassistant/up, a maintainer could confirmthe thermostat does not change state at request time for the scheduled form.
Summary by CodeRabbit