-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix CI approval flows and stale fixtures #1478
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| use std::sync::Arc; | ||
|
|
||
| use futures::StreamExt; | ||
| use uuid::Uuid; | ||
|
|
||
| use crate::agent::context_monitor::ContextMonitor; | ||
| use crate::agent::heartbeat::spawn_heartbeat; | ||
|
|
@@ -1010,15 +1011,59 @@ impl Agent { | |
| } | ||
| } | ||
|
|
||
| // Resolve session and thread | ||
| let (session, thread_id) = self | ||
| .session_manager | ||
| .resolve_thread( | ||
| &message.user_id, | ||
| &message.channel, | ||
| message.conversation_scope(), | ||
| ) | ||
| .await; | ||
| // Resolve session and thread. Approval submissions are allowed to | ||
| // target an already-loaded owned thread by UUID across channels so the | ||
| // web approval UI can approve work that originated from HTTP/other | ||
| // owner-scoped channels. | ||
| let approval_thread_uuid = if matches!( | ||
| submission, | ||
| Submission::ExecApproval { .. } | Submission::ApprovalResponse { .. } | ||
| ) { | ||
| message | ||
| .conversation_scope() | ||
| .and_then(|thread_id| Uuid::parse_str(thread_id).ok()) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let (session, thread_id) = if let Some(target_thread_id) = approval_thread_uuid { | ||
| let session = self | ||
| .session_manager | ||
| .get_or_create_session(&message.user_id) | ||
| .await; | ||
| let mut sess = session.lock().await; | ||
| if sess.threads.contains_key(&target_thread_id) { | ||
| sess.active_thread = Some(target_thread_id); | ||
| sess.last_active_at = chrono::Utc::now(); | ||
| drop(sess); | ||
| self.session_manager | ||
| .register_thread( | ||
| &message.user_id, | ||
| &message.channel, | ||
| target_thread_id, | ||
| Arc::clone(&session), | ||
| ) | ||
| .await; | ||
| (session, target_thread_id) | ||
| } else { | ||
| drop(sess); | ||
| self.session_manager | ||
| .resolve_thread( | ||
| &message.user_id, | ||
| &message.channel, | ||
| message.conversation_scope(), | ||
| ) | ||
| .await | ||
| } | ||
|
Comment on lines
+1029
to
+1057
|
||
| } else { | ||
| self.session_manager | ||
| .resolve_thread( | ||
| &message.user_id, | ||
| &message.channel, | ||
| message.conversation_scope(), | ||
| ) | ||
| .await | ||
| }; | ||
| tracing::debug!( | ||
| message_id = %message.id, | ||
| thread_id = %thread_id, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
self.session_manager.resolve_thread(...)call is duplicated here and in the finalelseblock. While functionally correct, consider extracting this logic into a helper function or a dedicated method withinSessionManagerto reduce repetition and improve readability. For instance,SessionManagercould expose a method likeresolve_or_activate_thread_for_approvalthat encapsulates this specific logic.References
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I left this as-is for now to keep the CI fix narrowly scoped, since the duplicated branch is just the fallback into the existing SessionManager path. If we touch this area again, I agree a small helper such as
resolve_or_activate_thread_for_approvalwould make the control flow easier to read.