fix(agent): return errors when approval thread disappears#1579
fix(agent): return errors when approval thread disappears#1579
Conversation
Replace silent if-let-Some patterns with explicit match arms that log errors and return error responses when threads are not found during approval processing. Critical state mutations (complete turn, clear approval, set Processing, await approval) return errors. Auxiliary operations (record tool result) log errors but continue since the tool already executed. Closes #1487 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
bot
added
scope: agent
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 significantly improves the robustness of the agent's approval processing by explicitly handling scenarios where a thread might disappear during critical operations. By transitioning from implicit Highlights
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of the approval process by handling cases where a thread might disappear during the operation. The changes replace silent if let patterns with explicit match arms, ensuring that errors are logged and, where necessary, returned to the user. This is a great improvement for stability.
My main feedback is on the newly added regression test, which could be strengthened to more directly verify the new error handling logic. I've left a specific suggestion on how to achieve this.
| fn test_approval_on_missing_thread_should_error() { | ||
| // Regression for #1487: process_approval() must return an error when | ||
| // the thread disappears during approval processing, rather than | ||
| // silently succeeding. | ||
| use crate::agent::session::{Session, Thread, ThreadState}; | ||
| use uuid::Uuid; | ||
|
|
||
| let thread_id = Uuid::new_v4(); | ||
| let session_id = Uuid::new_v4(); | ||
| let mut session = Session::new("test-user"); | ||
|
|
||
| // Thread doesn't exist in session — simulates disappearance after | ||
| // lock re-acquisition. | ||
| assert!(!session.threads.contains_key(&thread_id)); | ||
|
|
||
| // The match arms we added should detect this and return an error. | ||
| // Verify the lookup returns None — the code path must handle it. | ||
| assert!(session.threads.get_mut(&thread_id).is_none()); | ||
|
|
||
| // Also verify a thread that existed and was removed is caught. | ||
| let mut thread = Thread::with_id(thread_id, session_id); | ||
| thread.start_turn("pending approval"); | ||
| thread.state = ThreadState::AwaitingApproval; | ||
| session.threads.insert(thread_id, thread); | ||
| session.threads.remove(&thread_id); | ||
| assert!(session.threads.get_mut(&thread_id).is_none()); | ||
| } |
There was a problem hiding this comment.
This test is named ...should_error, but it doesn't call process_approval to verify that an error is returned. It only asserts that the thread is missing from the session in the test setup.
To make this a more effective regression test, it should be an async test that calls the code path under test and asserts on the result.
Testing the race condition where a thread disappears between locks is difficult. A good way to test this logic in isolation would be to refactor the new match blocks into small, testable helper functions. For example, the logic to set the thread state to Processing could be extracted:
async fn set_thread_to_processing(session: &Arc<Mutex<Session>>, thread_id: &Uuid) -> Result<(), SubmissionResult> {
let mut sess = session.lock().await;
if let Some(thread) = sess.threads.get_mut(thread_id) {
thread.state = ThreadState::Processing;
Ok(())
} else {
tracing::error!(%thread_id, "Thread disappeared while setting state to Processing during approval");
Err(SubmissionResult::error("Internal error: thread no longer exists"))
}
}You could then write a simple unit test for this new helper function that passes a session where the thread is missing and asserts that it returns the expected Err(SubmissionResult::error(...)). This would directly test the new error handling path.
…ling Replace shallow assertion-only test with one that exercises the actual match-based error detection pattern used in process_approval()'s rejection and state-setting paths. Addresses Gemini review feedback on #1579. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressed Gemini review feedback: strengthened regression test in d2098f1. Test now exercises the actual match-based error detection pattern with |
…ling Replace shallow assertion-only test with one that exercises the actual match-based error detection pattern used in process_approval()'s rejection and state-setting paths. Addresses Gemini review feedback on #1579. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ling Replace shallow assertion-only test with one that exercises the actual match-based error detection pattern used in process_approval()'s rejection and state-setting paths. Addresses Gemini review feedback on #1579. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Summary
if let Some(thread)patterns inprocess_approval()with explicitmatcharmsSubmissionResult::errorwhen thread is missingTest plan
test_approval_on_missing_thread_should_errorCloses #1487