Fix MCP lifecycle trace user scope - #1646
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 primarily addresses a bug in the advanced MCP lifecycle trace test, ensuring that the test uses a consistent user scope for all steps of the extension lifecycle. This resolves an issue where the test failed due to a mismatch in user contexts. Additionally, it introduces a new "single-message REPL mode" feature, which allows the agent to handle a single user input and then terminate, with robust handling for event-triggered routines to ensure they complete before exit. Highlights
Ignored Files
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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a single-message REPL mode, which modifies how event-triggered routines are handled by waiting for their completion before the REPL session exits. This involves changes to message processing, routine engine execution, and REPL channel management. The review comments suggest updating the pull request description to accurately reflect the full scope of these product behavior changes, as it was initially described as a 'test-only fix'. Additionally, an idiomatic improvement in routine_engine.rs is suggested, recommending let _ = ... instead of std::mem::drop(...) for ignoring JoinHandle return values.
I am having trouble creating individual review comments. Click here to see my feedback.
src/agent/agent_loop.rs (88-95)
The PR description states this is a 'test-only fix', but this function is part of a larger set of changes to the single-message REPL mode and routine engine, which are product behavior changes. To maintain clarity for future code maintenance, please update the pull request title and description to accurately reflect the full scope of the changes, including the improvements to the single-message REPL mode.
src/agent/routine_engine.rs (214)
Using std::mem::drop to ignore the JoinHandle is a bit unconventional. The handle is dropped at the end of the statement anyway, so this call is redundant. To explicitly ignore the return value and avoid compiler warnings about an unused result, it's more idiomatic to assign it to _.
let _ = self.spawn_fire(triggered.routine, "event", Some(triggered.detail));
There was a problem hiding this comment.
Pull request overview
This PR primarily fixes the advanced MCP lifecycle trace E2E test by ensuring MCP install, token injection, and activation all occur under the same user scope. It also includes additional changes to REPL single-message mode behavior, routine-engine trigger execution semantics, and CI test timeouts.
Changes:
- Fix
mcp_extension_lifecycleE2E trace test to consistently use"test-user"for secret injection and activation. - Update REPL
-m(single-message) mode to delay quitting until the turn finishes, including waiting for event-triggered routines. - Add explicit timeouts around CI test commands and update a Telegram hot-activation E2E assertion payload shape.
Reviewed changes
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e_advanced_traces.rs | Uses a single TEST_USER_ID for MCP token injection and activation in the lifecycle trace test. |
| tests/e2e/scenarios/test_telegram_hot_activation.py | Updates expected setup payloads to include an empty fields object. |
| src/channels/repl.rs | Changes single-message REPL mode to enqueue /quit only after the turn finishes; adds metadata flagging for single-message mode. |
| src/agent/routine_engine.rs | Refactors event trigger matching to optionally await spawned routine tasks; changes spawn_fire to return a JoinHandle. |
| src/agent/agent_loop.rs | Detects single-message REPL via message metadata and adjusts shutdown/response suppression behavior accordingly. |
| .github/workflows/test.yml | Adds job-level timeouts and wraps cargo test invocations with timeout to bound runtime. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
No issues found. |
* Fix REPL single-message hang and cap CI test duration * Fix Clippy nested-if lint in REPL startup * Fix single-message approval flow * Handle empty single-message REPL exits * Wait for one-shot event routines before exit * Fix MCP lifecycle trace user scope
* Fix REPL single-message hang and cap CI test duration * Fix Clippy nested-if lint in REPL startup * Fix single-message approval flow * Handle empty single-message REPL exits * Wait for one-shot event routines before exit * Fix MCP lifecycle trace user scope
Summary
Fix the advanced MCP lifecycle trace test to use a single user scope throughout install, token injection, and activation.
Root cause
The test rig runs tool_install as test-user, but the test was injecting the OAuth token and calling activate() under default. That split the MCP install and activation steps across different user namespaces, so activation correctly returned NotInstalled.
Change
Verification
Notes
This is a test-only fix; no product behavior changes.