fix(quick-router): "will it rain tomorrow" asks for current weather, not the forecast - #913
Open
kai392 wants to merge 1 commit into
Open
fix(quick-router): "will it rain tomorrow" asks for current weather, not the forecast#913kai392 wants to merge 1 commit into
kai392 wants to merge 1 commit into
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)
📝 WalkthroughWalkthroughThe quick weather router now shares forecast detection between rain and general weather queries. Future-day rain requests set ChangesWeather forecast 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 |
…eather
`weather_request` has two branches and they disagreed. The general
weather/forecast branch derived `forecast` from the utterance
("tomorrow", "week", "forecast", ...), but the "is it rain…"/"will it
rain…" branch hardcoded `forecast: false` on all four of its returns.
`forecast` selects the 7-day forecast over a right-now reading, so "will
it rain tomorrow" and "will it rain next week" asked Home Assistant for
*today's* conditions and answered a question about a future day with the
current sky. Which answer you got depended only on whether you said
"will it rain in Denver tomorrow" or "what's the weather in Denver
tomorrow".
Lift the existing predicate into `asks_for_forecast` and use it in both
branches, so the two phrasings of the same question resolve identically.
No new semantics: the rule is the one the general branch already applied.
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 #912.
Root cause
weather_requesthas two branches and they disagreed about theforecastflag.The general weather/forecast branch derives it from the utterance:
The
"is it rain…"/"will it rain…"branch above it never computes anything —all four of its return paths hardcode
false.forecastis what selects the 7-day forecast over a right-now reading (per thetool schema:
"true for 7-day forecast, false for current weather"), so aquestion about a future day was answered with the current sky. Which answer you
got depended only on which verb you used:
Fix
Lift the existing predicate into
asks_for_forecastand call it from bothbranches.
No new semantics — the rule is exactly the one the general branch already
applied, now shared instead of duplicated-then-forgotten. That also means the
two branches cannot drift apart again.
Impact
Future-day rain questions ask Home Assistant for the forecast instead of current
conditions. Present-tense questions are unchanged. The two phrasings of the same
question now resolve identically — the new test asserts that directly by routing
both and comparing.
Risk / tradeoffs
Low. The only behavior change is
forecast: false -> trueon rain utterancescontaining
forecast/tomorrow/week/7 day/seven day; everything elsekeeps its current value, and neither branch's
locationhandling is touched.Worth naming: the shared rule is still a keyword list, so
"will it rain in a month"staysforecast: false— a real gap, but a pre-existing one that thegeneral branch has too, and widening the keyword set is a separate change from
making the two branches agree. The existing
rain_query_time_expression_is_not_a_locationtest pins that utterance'slocation, and this PR leaves itsforecastvalue as it was.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#5 — all green.
The pre-existing
rain_query_keeps_named_cityandrain_query_time_expression_is_not_a_locationtests still pass; both assertlocationonly, so theforecastflag was previously unpinned in either branch.What I observed
Before, on
main@ 02a577d:After, same harness, same build:
Validation gap
The routing half is fully covered above. What I could not verify is that the
downstream
get_weatherexecution actually returns tomorrow's forecast forforecast: trueagainst a live provider — that needs network and the configuredweather backend on device.
crates/genie-core/src/tools/weather.rsis the path amaintainer would exercise to confirm it end to end.
Summary by CodeRabbit