mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-25 23:16:26 +00:00
Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
47f80ddc22 | ||
|
|
d2098f1030 | ||
|
|
79a2c5d9dd |
+153
-31
@@ -992,8 +992,16 @@ impl Agent {
|
||||
{
|
||||
// Put it back and return error
|
||||
let mut sess = session.lock().await;
|
||||
if let Some(thread) = sess.threads.get_mut(&thread_id) {
|
||||
thread.await_approval(pending);
|
||||
match sess.threads.get_mut(&thread_id) {
|
||||
Some(thread) => {
|
||||
thread.await_approval(pending);
|
||||
}
|
||||
None => {
|
||||
tracing::warn!(
|
||||
%thread_id,
|
||||
"Thread disappeared while restoring pending approval after request ID mismatch"
|
||||
);
|
||||
}
|
||||
}
|
||||
return Ok(SubmissionResult::error(
|
||||
"Request ID mismatch. Use the correct request ID.",
|
||||
@@ -1015,8 +1023,19 @@ impl Agent {
|
||||
// Reset thread state to processing
|
||||
{
|
||||
let mut sess = session.lock().await;
|
||||
if let Some(thread) = sess.threads.get_mut(&thread_id) {
|
||||
thread.state = ThreadState::Processing;
|
||||
match sess.threads.get_mut(&thread_id) {
|
||||
Some(thread) => {
|
||||
thread.state = ThreadState::Processing;
|
||||
}
|
||||
None => {
|
||||
tracing::error!(
|
||||
%thread_id,
|
||||
"Thread disappeared while setting state to Processing during approval"
|
||||
);
|
||||
return Ok(SubmissionResult::error(
|
||||
"Internal error: thread no longer exists",
|
||||
));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1100,13 +1119,21 @@ impl Agent {
|
||||
// Record sanitized result in thread
|
||||
{
|
||||
let mut sess = session.lock().await;
|
||||
if let Some(thread) = sess.threads.get_mut(&thread_id)
|
||||
&& let Some(turn) = thread.last_turn_mut()
|
||||
{
|
||||
if is_tool_error {
|
||||
turn.record_tool_error(result_content.clone());
|
||||
} else {
|
||||
turn.record_tool_result(serde_json::json!(result_content));
|
||||
match sess.threads.get_mut(&thread_id) {
|
||||
Some(thread) => {
|
||||
if let Some(turn) = thread.last_turn_mut() {
|
||||
if is_tool_error {
|
||||
turn.record_tool_error(result_content.clone());
|
||||
} else {
|
||||
turn.record_tool_result(serde_json::json!(result_content));
|
||||
}
|
||||
}
|
||||
}
|
||||
None => {
|
||||
tracing::error!(
|
||||
%thread_id,
|
||||
"Thread disappeared while recording tool result during approval"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1354,13 +1381,22 @@ impl Agent {
|
||||
// Record sanitized result in thread
|
||||
{
|
||||
let mut sess = session.lock().await;
|
||||
if let Some(thread) = sess.threads.get_mut(&thread_id)
|
||||
&& let Some(turn) = thread.last_turn_mut()
|
||||
{
|
||||
if is_deferred_error {
|
||||
turn.record_tool_error(deferred_content.clone());
|
||||
} else {
|
||||
turn.record_tool_result(serde_json::json!(deferred_content));
|
||||
match sess.threads.get_mut(&thread_id) {
|
||||
Some(thread) => {
|
||||
if let Some(turn) = thread.last_turn_mut() {
|
||||
if is_deferred_error {
|
||||
turn.record_tool_error(deferred_content.clone());
|
||||
} else {
|
||||
turn.record_tool_result(serde_json::json!(deferred_content));
|
||||
}
|
||||
}
|
||||
}
|
||||
None => {
|
||||
tracing::error!(
|
||||
%thread_id,
|
||||
tool_name = %tc.name,
|
||||
"Thread disappeared while recording deferred tool result during approval"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1413,8 +1449,19 @@ impl Agent {
|
||||
|
||||
{
|
||||
let mut sess = session.lock().await;
|
||||
if let Some(thread) = sess.threads.get_mut(&thread_id) {
|
||||
thread.await_approval(new_pending);
|
||||
match sess.threads.get_mut(&thread_id) {
|
||||
Some(thread) => {
|
||||
thread.await_approval(new_pending);
|
||||
}
|
||||
None => {
|
||||
tracing::error!(
|
||||
%thread_id,
|
||||
"Thread disappeared while setting up deferred tool approval"
|
||||
);
|
||||
return Ok(SubmissionResult::error(
|
||||
"Internal error: thread no longer exists",
|
||||
));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1546,17 +1593,28 @@ impl Agent {
|
||||
);
|
||||
{
|
||||
let mut sess = session.lock().await;
|
||||
if let Some(thread) = sess.threads.get_mut(&thread_id) {
|
||||
thread.clear_pending_approval();
|
||||
thread.complete_turn(&rejection);
|
||||
// User message already persisted at turn start; save rejection response
|
||||
self.persist_assistant_response(
|
||||
thread_id,
|
||||
&message.channel,
|
||||
&message.user_id,
|
||||
&rejection,
|
||||
)
|
||||
.await;
|
||||
match sess.threads.get_mut(&thread_id) {
|
||||
Some(thread) => {
|
||||
thread.clear_pending_approval();
|
||||
thread.complete_turn(&rejection);
|
||||
// User message already persisted at turn start; save rejection response
|
||||
self.persist_assistant_response(
|
||||
thread_id,
|
||||
&message.channel,
|
||||
&message.user_id,
|
||||
&rejection,
|
||||
)
|
||||
.await;
|
||||
}
|
||||
None => {
|
||||
tracing::error!(
|
||||
%thread_id,
|
||||
"Thread disappeared during approval rejection"
|
||||
);
|
||||
return Ok(SubmissionResult::error(
|
||||
"Internal error: thread no longer exists",
|
||||
));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2098,6 +2156,70 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_approval_on_missing_thread_should_error() {
|
||||
// Regression for #1487: when a thread disappears from the session
|
||||
// during approval processing, the code must return a visible error
|
||||
// rather than silently succeeding.
|
||||
//
|
||||
// We can't call process_approval() directly (requires full Agent),
|
||||
// so we simulate the exact code pattern used in the rejection and
|
||||
// state-setting paths: lock session, match on get_mut, verify the
|
||||
// None arm produces an error.
|
||||
use crate::agent::session::{Session, Thread, ThreadState};
|
||||
use std::sync::Arc;
|
||||
use tokio::sync::Mutex;
|
||||
use uuid::Uuid;
|
||||
|
||||
let thread_id = Uuid::new_v4();
|
||||
let session_id = Uuid::new_v4();
|
||||
let session = Arc::new(Mutex::new(Session::new("test-user")));
|
||||
|
||||
// Scenario 1: Thread never existed
|
||||
{
|
||||
let sess = session.lock().await;
|
||||
let result = match sess.threads.get(&thread_id) {
|
||||
Some(_) => Ok("processed"),
|
||||
None => Err("Internal error: thread no longer exists"),
|
||||
};
|
||||
assert!(result.is_err());
|
||||
assert_eq!(
|
||||
result.unwrap_err(),
|
||||
"Internal error: thread no longer exists"
|
||||
);
|
||||
}
|
||||
|
||||
// Scenario 2: Thread existed then was removed (simulates disappearance
|
||||
// between lock acquisitions -- the TOCTOU window this fix addresses)
|
||||
{
|
||||
let mut sess = session.lock().await;
|
||||
let mut thread = Thread::with_id(thread_id, session_id);
|
||||
thread.start_turn("pending approval");
|
||||
thread.state = ThreadState::AwaitingApproval;
|
||||
sess.threads.insert(thread_id, thread);
|
||||
}
|
||||
{
|
||||
let mut sess = session.lock().await;
|
||||
// Simulate thread disappearing (e.g., pruned by another task)
|
||||
sess.threads.remove(&thread_id);
|
||||
|
||||
// The rejection path must detect this and return an error
|
||||
let result = match sess.threads.get_mut(&thread_id) {
|
||||
Some(thread) => {
|
||||
thread.clear_pending_approval();
|
||||
thread.complete_turn("rejected");
|
||||
Ok("rejection persisted")
|
||||
}
|
||||
None => Err("Internal error: thread no longer exists"),
|
||||
};
|
||||
assert!(result.is_err());
|
||||
assert_eq!(
|
||||
result.unwrap_err(),
|
||||
"Internal error: thread no longer exists"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_queue_cap_rejects_at_capacity() {
|
||||
use crate::agent::session::{MAX_PENDING_MESSAGES, Thread, ThreadState};
|
||||
|
||||
Reference in New Issue
Block a user