fix(quick-router): abstain on absolute setpoint schedules and conditions - #916
fix(quick-router): abstain on absolute setpoint schedules and conditions#916RealDiligent wants to merge 3 commits into
Conversation
|
Warning Review limit reached
Next review available in: 44 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 quick router detects absolute schedules, conditionals, and exclusions in ChangesQualified setpoint routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 2051-2056: Update the scheduled-setpoint detection around
is_absolute_schedule_tail to inspect the final “ at ” tail whether the numeric
value follows “to” or “at”, so inputs like “set the thermostat at 68 at 9pm” are
recognized as scheduled rather than actuated immediately. Add a regression test
covering this “at”-separator 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: 32e85a42-fa01-4157-bacc-c33cea6b844f
📒 Files selected for processing (1)
crates/genie-core/src/tools/quick.rs
| if let Some((_, after_to)) = rest.split_once(" to ") | ||
| && let Some((_, at_tail)) = after_to.rsplit_once(" at ") | ||
| { | ||
| let at_tail = at_tail.trim(); | ||
| if is_absolute_schedule_tail(at_tail) { | ||
| return true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle scheduled setpoints using at as the value separator.
set the thermostat at 68 at 9pm bypasses this to-only branch, then parse_temperature_target reads 68 and actuates immediately. Inspect the final at tail regardless of whether the numeric value was introduced by to or at, and add this regression case.
Proposed fix
- if let Some((_, after_to)) = rest.split_once(" to ")
- && let Some((_, at_tail)) = after_to.rsplit_once(" at ")
- {
- let at_tail = at_tail.trim();
- if is_absolute_schedule_tail(at_tail) {
- return true;
- }
+ if let Some((before_at, at_tail)) = rest.rsplit_once(" at ")
+ && parse_temperature_target(before_at).is_some()
+ && is_absolute_schedule_tail(at_tail.trim())
+ {
+ return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some((_, after_to)) = rest.split_once(" to ") | |
| && let Some((_, at_tail)) = after_to.rsplit_once(" at ") | |
| { | |
| let at_tail = at_tail.trim(); | |
| if is_absolute_schedule_tail(at_tail) { | |
| return true; | |
| if let Some((before_at, at_tail)) = rest.rsplit_once(" at ") | |
| && parse_temperature_target(before_at).is_some() | |
| && is_absolute_schedule_tail(at_tail.trim()) | |
| { | |
| return true; |
🤖 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 2051 - 2056, Update the
scheduled-setpoint detection around is_absolute_schedule_tail to inspect the
final “ at ” tail whether the numeric value follows “to” or “at”, so inputs like
“set the thermostat at 68 at 9pm” are recognized as scheduled rather than
actuated immediately. Add a regression test covering this “at”-separator form.
Closes GeniePod#914 Relative "in <duration>" setpoints already abstained (GeniePod#829), but absolute schedules ("at 9pm", "tonight"), conditionals ("when I get home"), and exclusions ("except the bedroom") still actuated now and dropped the qualifier. Mirror the turn_on/turn_off multi-clause abstain so the LLM can arm them, while keeping bare setpoints and "at"-as-value-separator. Co-authored-by: Cursor <cursoragent@cursor.com>
set_brightness keeps a float value argument; assert against json!(40.0). Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/genie-core/src/tools/quick.rs (1)
2051-2056:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe
atvalue-separator form still bypasses scheduled-setpoint detection.
set the thermostat at 68 at 9pmhas no" to "clause, so this block never inspects the final" at "tail;parse_temperature_targetthen actuates immediately and drops the schedule. Add a separator-independent check and regression cases for thermostat and brightness forms. This is the same unresolved finding from the previous review.Proposed fix
- if let Some((_, after_to)) = rest.split_once(" to ") - && let Some((_, at_tail)) = after_to.rsplit_once(" at ") + if let Some((before_at, at_tail)) = rest.rsplit_once(" at ") + && parse_temperature_target(before_at).is_some() { let at_tail = at_tail.trim(); if is_absolute_schedule_tail(at_tail) { return true; }Also applies to: 6438-6485
🤖 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 2051 - 2056, Update the scheduled-setpoint detection around is_absolute_schedule_tail so it independently inspects the final “ at ” tail, rather than requiring a preceding “ to ” clause; ensure forms like “set the thermostat at 68 at 9pm” are recognized before parse_temperature_target actuates immediately. Add regression coverage for both thermostat and brightness value-separator forms, preserving existing “to … at …” behavior.
🤖 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.
Duplicate comments:
In `@crates/genie-core/src/tools/quick.rs`:
- Around line 2051-2056: Update the scheduled-setpoint detection around
is_absolute_schedule_tail so it independently inspects the final “ at ” tail,
rather than requiring a preceding “ to ” clause; ensure forms like “set the
thermostat at 68 at 9pm” are recognized before parse_temperature_target actuates
immediately. Add regression coverage for both thermostat and brightness
value-separator forms, preserving existing “to … at …” behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f9ca3902-74ba-4195-9e78-9da9a048f3b7
📒 Files selected for processing (1)
crates/genie-core/src/tools/quick.rs
Spoken amounts like "one hundred and five" contain " and "; abstaining on that token broke quick_spoken_setpoint_test. Keep when/unless/if/except/ only/everything without the coordinating "and" guard. Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Closes #914
Summary
Relative
in <duration>setpoints already abstained (#829), but absolute schedules (at 9pm,tonight), conditionals (when I get home), and exclusions (except the bedroom) still actuated immediately and dropped the qualifier — including dimming rooms the caller asked to exclude.Root cause
The
set/preheatbranch only guardedin <time_expression>.parse_temperature_targetstill read the numeric setpoint out of strings like68 at 9pmor40 percent except the bedroom.Fix
setpoint_has_schedule_or_conditionmirroringsimple_turn_requestmulti-clause abstain (when/unless/if/except/only/and/everything).tonight,before bed, …) andto <value> at <clock/time>tails (soset the thermostat at 68still works as the value separator).set the lights to 30 percent at 9pm).Impact
Scheduled/conditional setpoints abstain for LLM grounding; bare setpoints and room-qualified
in the denkeep actuating now.Test plan
setpoint_with_absolute_schedule_or_condition_abstainscovers the issue reproductions and keep-alive cases.cargo test— CI validation required.Real Behavior Proof
CI-only / static review on Windows dev host. Verification is the new
route()unit tests plus full workspace CI on Linux.Summary by CodeRabbit