Fix duplicate LLM responses for matched event routines - #1275
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue causing duplicate LLM responses when event-triggered routines are activated by inbound user messages. By introducing an early check for routine triggers and consuming the message if a routine fires, the system now ensures that only the routine-driven response is generated. This change streamlines the interaction flow, prevents redundant replies from the main agent, and improves the overall consistency of the agent's behavior. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where messages triggering event-based routines would also be processed by the main chat pipeline, resulting in duplicate responses. The fix is implemented by checking for and consuming event-triggered user messages before they enter the standard message handling logic. If a routine is fired, the agent loop continues to the next message, preventing a duplicate agent reply. The end-to-end tests and corresponding LLM trace fixtures have been updated to assert that only the routine-driven LLM turn occurs for a matched message, confirming the fix.
There was a problem hiding this comment.
Pull request overview
This PR fixes a duplicate-response behavior where a single inbound message that matches an event-triggered routine could both (1) fire the routine and (2) still flow through the normal agent chat pipeline, producing an extra assistant reply.
Changes:
- Consume inbound event-matching user messages earlier to prevent the main chat/tool pipeline from generating a second assistant response.
- Update E2E trace fixtures to remove the extra (duplicate) assistant response step.
- Strengthen E2E assertions to verify only the routine-driven LLM call occurs and only the routine notification is emitted.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/agent/agent_loop.rs |
Adds early event-trigger checking and skips the normal message handling when an event routine fires, aiming to prevent duplicate responses. |
tests/e2e_advanced_traces.rs |
Updates event-trigger tests to assert exactly one additional LLM call and a single routine notification response after a matching inbound message. |
tests/fixtures/llm_traces/advanced/routine_event_telegram.json |
Removes the previously-recorded extra assistant response from the trace fixture. |
tests/fixtures/llm_traces/advanced/routine_event_any_channel.json |
Removes the previously-recorded extra assistant response from the trace fixture. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR fixes a behavior where an inbound message that matches an event-triggered routine could produce both (1) a routine-driven LLM turn/notification and (2) a separate assistant reply from the normal chat pipeline. It does this by consuming matched event-triggered user messages during handle_message() so they don’t proceed through the standard response path, and updates trace fixtures/tests accordingly.
Changes:
- Move event-trigger checking into
handle_message()and suppress the normal assistant reply by returning an empty response when event routines fire. - Add LLM-call-count and response-count assertions to ensure a matched event message produces only the routine-driven LLM call/notification.
- Update LLM trace fixtures to remove the previously duplicated assistant reply.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/agent/agent_loop.rs |
Consumes matched event-trigger messages inside handle_message() by firing routines and returning an empty response to prevent duplicate assistant replies. |
tests/e2e_advanced_traces.rs |
Strengthens E2E assertions to ensure only one LLM call and one routine notification occur for matched event messages. |
tests/fixtures/llm_traces/advanced/routine_event_telegram.json |
Removes the extra assistant response from the expected trace for Telegram-scoped event routines. |
tests/fixtures/llm_traces/advanced/routine_event_any_channel.json |
Removes the extra assistant response from the expected trace for any-channel event routines. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if matches!(submission, Submission::UserInput { .. }) | ||
| && let Some(engine) = self.routine_engine().await | ||
| { | ||
| let fired = engine.check_event_triggers(message).await; | ||
| if fired > 0 { | ||
| tracing::debug!( | ||
| channel = %message.channel, | ||
| user = %message.user_id, | ||
| fired, | ||
| "Consumed inbound user message with matching event-triggered routine(s)" | ||
| ); | ||
| return Ok(Some(String::new())); | ||
| } |
There was a problem hiding this comment.
Fixed in 89320c0. routine_trigger_message() now extracts the post-hook content from the parsed Submission::UserInput { content } and, when it differs from the original message.content, clones the message with the rewritten content (via Cow::Owned). When content is unchanged, Cow::Borrowed avoids any allocation. The trigger check now consistently matches against what the agent will actually process.
| @@ -191,6 +191,11 @@ impl Agent { | |||
| self.routine_engine_slot = Some(slot); | |||
| } | |||
|
|
|||
| async fn routine_engine(&self) -> Option<Arc<crate::agent::routine_engine::RoutineEngine>> { | |||
| let slot = self.routine_engine_slot.as_ref()?; | |||
| slot.read().await.clone() | |||
| } | |||
zmanian
left a comment
There was a problem hiding this comment.
Review: fix duplicate LLM responses for event-triggered routines
Clean, well-structured fix. The progression through 4 commits shows good iteration:
- Initial fix (consume before handle_message)
- Rustfmt
- Move check inside handle_message to preserve preprocessing
- Match against rewritten input using Cow for efficiency
The final approach is correct -- checking event triggers inside handle_message (after submission parsing) ensures routines fire against preprocessed content and that the message is consumed before the main chat pipeline processes it.
Positives:
routine_trigger_message()helper uses Cow nicely to avoid cloning when content is unchanged- Test coverage is solid: unit tests for the helper (borrowed, owned, internal skip), updated integration tests asserting only 1 response and 1 LLM call, fixture traces updated
- The
routine_engine_slotinitialization simplification is clean - CI fully green
Minor notes:
- Returning
Ok(Some(String::new()))relies on the caller's!response.is_empty()guard to skip empty responses -- this coupling is correct but implicit. A future refactor might considerOk(None)for "consumed, no response needed" but that's outside scope.
LGTM.
18a6fc2 to
89320c0
Compare
There was a problem hiding this comment.
Pull request overview
This PR prevents duplicate assistant turns when an inbound user message matches an event-triggered routine by consuming the message inside handle_message() (after standard preprocessing) and returning an empty response.
Changes:
- Adds a helper to derive the routine-trigger “effective message” from the parsed
Submission(including hook-modified user input). - Moves event-trigger consumption into
handle_message()and suppresses the normal assistant reply when routines fire. - Adds unit tests validating the trigger-message borrowing/rewriting behavior and internal-message exclusion.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…_slot Address Copilot review feedback: - Change check_event_triggers to accept (user_id, channel, content) instead of &IncomingMessage, eliminating the need to clone the full message (including attachments) when hooks rewrite content. - Remove routine_trigger_message and the Cow<IncomingMessage> indirection; the event-trigger check now inlines the is_internal + UserInput guard and passes the post-hook content string directly. - Make routine_engine_slot non-optional since Agent::new() always initializes it. Removes the redundant Option wrapper and simplifies accessor/setter methods. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: consume matched event routine messages * style: run rustfmt for event routine fix * fix: preserve preprocessing for routine-triggered messages * fix: match routines against rewritten input * refactor: narrow check_event_triggers API and simplify routine_engine_slot Address Copilot review feedback: - Change check_event_triggers to accept (user_id, channel, content) instead of &IncomingMessage, eliminating the need to clone the full message (including attachments) when hooks rewrite content. - Remove routine_trigger_message and the Cow<IncomingMessage> indirection; the event-trigger check now inlines the is_internal + UserInput guard and passes the post-hook content string directly. - Make routine_engine_slot non-optional since Agent::new() always initializes it. Removes the redundant Option wrapper and simplifies accessor/setter methods. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: consume matched event routine messages * style: run rustfmt for event routine fix * fix: preserve preprocessing for routine-triggered messages * fix: match routines against rewritten input * refactor: narrow check_event_triggers API and simplify routine_engine_slot Address Copilot review feedback: - Change check_event_triggers to accept (user_id, channel, content) instead of &IncomingMessage, eliminating the need to clone the full message (including attachments) when hooks rewrite content. - Remove routine_trigger_message and the Cow<IncomingMessage> indirection; the event-trigger check now inlines the is_internal + UserInput guard and passes the post-hook content string directly. - Make routine_engine_slot non-optional since Agent::new() always initializes it. Removes the redundant Option wrapper and simplifies accessor/setter methods. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Testing