fix(quick-router): abstain on a cancel-verb timer command instead of setting a new timer - #882
Conversation
…setting a new timer "Cancel the 10 minute timer" / "stop the 5 minute timer" name an existing timer by its duration, but timer_request read the duration as a request and did the exact opposite of what was asked: the cancellation itself started a fresh 10-minute timer. Gate the parse on a leading cancel/stop/pause/delete verb (and the "turn off"/"shut off" forms) and abstain, so the LLM grounds the cancellation. "remind me to stop the dryer in 10 minutes" and every genuine set request are untouched.
📝 WalkthroughWalkthroughThe quick router now abstains when timer utterances begin with cancellation or termination verbs. Tests cover cancellation phrasing and confirm that valid timer-setting requests remain routed to ChangesTimer cancellation 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 |
![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
🧹 Nitpick comments (1)
crates/genie-core/src/tools/quick.rs (1)
6684-6691: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover every supported cancellation prefix.
The implementation adds eleven cancellation forms, but this regression test omits
end,resume,remove,clear,dismiss, andshut off. Add those cases so future edits cannot silently break individual branches.Suggested test additions
for utterance in [ "Cancel the 10 minute timer.", "Stop the 5 minute timer.", + "End the 10 minute timer.", + "Resume the 10 minute timer.", "Leo: cancel my 10 minute timer", "delete the 20 minute timer", + "remove the 10 minute timer", + "clear the 10 minute timer", + "dismiss the 10 minute timer", "pause the 10 minute timer", "turn off the 10 minute timer", + "shut off the 10 minute timer", ] {🤖 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 6684 - 6691, Expand the utterance list in the cancellation regression test to include timer requests using the missing prefixes “end,” “resume,” “remove,” “clear,” “dismiss,” and “shut off,” while preserving the existing cases and test structure.
🤖 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 2801-2816: Update the cancellation detection around
is_cancel_command to remove a leading politeness prefix such as “please” before
matching cancellation verbs and existing “turn off”/“shut off” forms, while
preserving normal command handling. Add a regression case covering “please
cancel the 10 minute timer” and verify it does not fall through to set_timer.
---
Nitpick comments:
In `@crates/genie-core/src/tools/quick.rs`:
- Around line 6684-6691: Expand the utterance list in the cancellation
regression test to include timer requests using the missing prefixes “end,”
“resume,” “remove,” “clear,” “dismiss,” and “shut off,” while preserving the
existing cases and test structure.
🪄 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: 698b5853-220e-4b6a-9673-77ad84e37c99
📒 Files selected for processing (1)
crates/genie-core/src/tools/quick.rs
| let is_cancel_command = matches!( | ||
| text.split_whitespace().next(), | ||
| Some( | ||
| "cancel" | ||
| | "stop" | ||
| | "end" | ||
| | "pause" | ||
| | "resume" | ||
| | "delete" | ||
| | "remove" | ||
| | "clear" | ||
| | "dismiss" | ||
| ) | ||
| ) || text.starts_with("turn off ") | ||
| || text.starts_with("shut off "); | ||
| if is_cancel_command { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle leading politeness before matching cancellation verbs.
please cancel the 10 minute timer does not match this guard because the first token is please; the duration parser can then still emit set_timer, recreating the cancellation bug. Strip a leading politeness prefix for this check and add a regression case.
Proposed fix
+ let command = text.strip_prefix("please ").unwrap_or(text);
let is_cancel_command = matches!(
- text.split_whitespace().next(),
+ command.split_whitespace().next(),
Some(
"cancel"
// ...
)
- ) || text.starts_with("turn off ")
- || text.starts_with("shut off ");
+ ) || command.starts_with("turn off ")
+ || command.starts_with("shut off ");📝 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.
| let is_cancel_command = matches!( | |
| text.split_whitespace().next(), | |
| Some( | |
| "cancel" | |
| | "stop" | |
| | "end" | |
| | "pause" | |
| | "resume" | |
| | "delete" | |
| | "remove" | |
| | "clear" | |
| | "dismiss" | |
| ) | |
| ) || text.starts_with("turn off ") | |
| || text.starts_with("shut off "); | |
| if is_cancel_command { | |
| let command = text.strip_prefix("please ").unwrap_or(text); | |
| let is_cancel_command = matches!( | |
| command.split_whitespace().next(), | |
| Some( | |
| "cancel" | |
| | "stop" | |
| | "end" | |
| | "pause" | |
| | "resume" | |
| | "delete" | |
| | "remove" | |
| | "clear" | |
| | "dismiss" | |
| ) | |
| ) || command.starts_with("turn off ") | |
| || command.starts_with("shut off "); | |
| if is_cancel_command { |
🤖 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 2801 - 2816, Update the
cancellation detection around is_cancel_command to remove a leading politeness
prefix such as “please” before matching cancellation verbs and existing “turn
off”/“shut off” forms, while preserving normal command handling. Add a
regression case covering “please cancel the 10 minute timer” and verify it does
not fall through to set_timer.
matedev01
left a comment
matedev01
left a comment
There was a problem hiding this comment.
Confirmed CodeRabbit's finding directly: route("please cancel the 10 minute timer") still returns set_timer{seconds:600} — a throwaway test shows the leading-verb guard only checks the first token, so a leading 'please' bypasses it entirely and reproduces the exact inversion bug (cancel -> sets a new timer) this PR exists to fix. Worth stripping the politeness prefix before the guard check, as CodeRabbit suggested, with a regression case for the polite phrasing alongside the existing ones.
2 participants
Summary
timer_requestkeys on the word "timer" plus any parseable duration, so in "cancel the 10 minute timer" the duration that only names which timer is read as a request — the cancellation itself starts a fresh 10-minute timer, the exact opposite of what was asked. Gate the parse on a leading cancel/stop/pause/delete verb (and the "turn off"/"shut off" forms) and abstain, since there is no deterministic cancel tool for the router to emit. Closes #881.maincancel the 10 minute timerset_timer{seconds:600}Stop the 5 minute timer.set_timer{seconds:300}delete the 20 minute timerset_timer{seconds:1200}pause the 10 minute timerset_timer{seconds:600}turn off the 10 minute timerset_timer{seconds:600}set a 10 minute timerset_timer{seconds:600}remind me to stop the dryer in 10 minutesset_timer{label:"stop the dryer"}Changes
timer_request, after the existing timer/remind gate, abstain when the utterance leads with a cancel verb (cancel/stop/end/pause/resume/delete/remove/clear/dismiss, or theturn off/shut offprefixes). Only the leading verb is gated, so "remind me to stop the dryer in 10 minutes" — where the verb sits inside the task clause — still schedules, and every genuine set request is untouched.cancel_verb_timer_commands_abstain_instead_of_setting_a_timer: five cancellation phrasings (plus a speaker-prefixed one) abstain, and the two positive controls above keep routing toset_timer.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 cancellation 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 (test added, fix absent):
After the fix: all six cancellation phrasings abstain; "set a 10 minute timer" and "remind me to stop the dryer in 10 minutes" route to
set_timerunchanged.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, and the release-gatedtool_dispatch_testtarget withbinary_size_budgetpassing). One documented load flake (llm::provider::tests::loopback_127_range_allowed_without_remote_flag) 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,080,248 bytes vs. the 7,079,800-bytemainbaseline built with the same toolchain — +448 bytes for the added verb gate;binary_size_budgetpasses.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 cancel_verb_timer_commands_abstain_instead_of_setting_a_timer— passes here, fails onmain(the first phrasing sets a 600-second timer where an abstain is expected).cargo test -p genie-core --lib tools::quick— the other 107 router tests are unchanged and green, including every existing timer/reminder-label test.Notes for reviewers
multi-homework-timercase starts with "Start homework mode …";startis deliberately not in the cancel-verb list (it is a set/activate verb here), and the BFCL prediction file is byte-identical tomain.resumeis included because "resume the 10 minute timer" also names an existing timer; emitting a fresh one is wrong for it in exactly the same way.Summary by CodeRabbit