fix(quick-router): route "i've got a meeting/appointment" to memory_store - #902
Open
Yurii214 wants to merge 2 commits into
Open
fix(quick-router): route "i've got a meeting/appointment" to memory_store#902Yurii214 wants to merge 2 commits into
Yurii214 wants to merge 2 commits 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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesCalendar assertion routing
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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)
6451-6468: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the newly added
anforms.The implementation adds both
i ve got aandi ve got an, but the test exercises onlya. Add an “I've got an appointment …” positive case and an “I've got an idea” negative case to protect both branches.🤖 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 6451 - 6468, Extend routes_ive_got_appointment_to_memory_store to cover the newly added “i ve got an” branch: add a positive “I've got an appointment …” assertion expecting memory_store with the appropriate calendar-event content, and a negative assertion that “I've got an idea” returns None. Preserve the existing “a” and non-appointment coverage.
🤖 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 1367-1374: Update the calendar-event detection in the text parsing
block to match “appointment” and “meeting” as whole tokens rather than using
substring checks on rest. Preserve routing valid calendar phrases to “reminders”
while rejecting phrases such as “appointment book” and “meeting notes” before
returning the memory_store tuple.
---
Nitpick comments:
In `@crates/genie-core/src/tools/quick.rs`:
- Around line 6451-6468: Extend routes_ive_got_appointment_to_memory_store to
cover the newly added “i ve got an” branch: add a positive “I've got an
appointment …” assertion expecting memory_store with the appropriate
calendar-event content, and a negative assertion that “I've got an idea” returns
None. Preserve the existing “a” and non-appointment coverage.
🪄 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: 1a8bb716-d27a-464d-bcba-c7759f504c90
📒 Files selected for processing (1)
crates/genie-core/src/tools/quick.rs
…tore the appointment/meeting assertion in personal_fact_store_request accepted only "i have a"/"i have an"; the equally common contraction "i've got a" (normalized to "i ve got a") fell through to the llm, and the dentist form even misrouted to memory_recall. accept the contraction, same as "i have a".
Yurii214
force-pushed
the
fix/ive-got-appointment
branch
from
July 30, 2026 04:46
b3cf562 to
dc998c1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/genie-core/src/tools/quick.rs (1)
6451-6474: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
anbranch and stated negative examples.The test exercises only
"I've got a …"and"I've got a cold". Add an"I've got an appointment …"case plus"I've got a question for you"and"I've got an idea"to protect both new prefixes and the documented abstention behavior.Suggested test extension
+ let call = route("I've got an appointment on Monday").unwrap(); + assert_eq!(call.name, "memory_store"); + assert_eq!(call.arguments["category"], "reminders"); + // a non-appointment "i've got" is not a calendar event and still abstains. - assert!(route("I've got a cold").is_none()); + for utterance in [ + "I've got a cold", + "I've got a question for you", + "I've got an idea", + ] { + assert!(route(utterance).is_none()); + }🤖 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 6451 - 6474, The test routes_ive_got_appointment_to_memory_store should also cover the “an” appointment prefix and both documented non-calendar abstentions. Add an “I’ve got an appointment …” assertion expecting memory_store with the appropriate reminders category and calendar-event content, plus assertions that route("I've got a question for you") and route("I've got an idea") return None.
🤖 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.
Nitpick comments:
In `@crates/genie-core/src/tools/quick.rs`:
- Around line 6451-6474: The test routes_ive_got_appointment_to_memory_store
should also cover the “an” appointment prefix and both documented non-calendar
abstentions. Add an “I’ve got an appointment …” assertion expecting memory_store
with the appropriate reminders category and calendar-event content, plus
assertions that route("I've got a question for you") and route("I've got an
idea") return None.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e4493078-b986-41cd-a6c0-d14972e9b3ab
📒 Files selected for processing (1)
crates/genie-core/src/tools/quick.rs
adds an "i've got an appointment" case for the new "an" prefix and extends the abstention check to "i've got a question for you" and "i've got an idea", per review. behaviour unchanged; test coverage only.
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
1 participant
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.
Summary
The appointment/meeting assertion in
personal_fact_store_requestonly accepted"i have a"/"i have an", so the equally common present-perfect contraction"i've got a …"(normalized to"i ve got a") missed both prefixes:mainI've got a meeting on Saturday 10AMmemory_store{reminders, "calendar event: meeting on saturday 10am"}I've got a dentist appointment on Fridaymemory_recallmemory_store{reminders, "calendar event: dentist appointment on friday"}I have a meeting …/I have a dentist appointment …already route tomemory_storetoday — the dentist form is worse than a plain fall-through, because it gets grabbed by a latermemory_recallmatcher and reads memory instead of writing the event.Changes
"i ve got a "/"i ve got an "to the accept-set, alongside the existing"i have a "/"i have an ". Therest.contains("appointment") || rest.contains("meeting")gate and thecalendar event: …content are unchanged, so the output is byte-identical to the"i have a"form."I've got a cold","I've got a question for you","I've got an idea"→None(no appointment/meeting).routes_ive_got_appointment_to_memory_store(both positive forms + the"i've got a cold"negative).Real Behavior Proof
Tested profile / hardware (check all that apply):
jetsonraspberry_piportable_sbclaptopmacRan
cargo test -p genie-core --no-default-features --lib tools::quick(119 passed); the newroutes_ive_got_appointment_to_memory_storetest fails onmain(the meeting form routes toNone→ panic) and passes with the change.cargo clippy(0 warnings) andcargo fmt --checkare clean. Pure text-classification logic, no hardware dependency, so the laptop run exercises the full affected path.Summary by CodeRabbit
New Features
Bug Fixes