mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-25 14:53:34 +00:00
fix: misleading UI message (#1265)
* fix: misleading UI message * review fixes * review fixes * enhance test
This commit is contained in:
@@ -427,6 +427,14 @@ impl SubmissionResult {
|
||||
message: message.into(),
|
||||
}
|
||||
}
|
||||
|
||||
/// Create a non-error status message (e.g., for blocking states like approval waiting).
|
||||
/// Uses Ok variant to avoid "Error:" prefix in rendering.
|
||||
pub fn pending(message: impl Into<String>) -> Self {
|
||||
Self::Ok {
|
||||
message: Some(message.into()),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
||||
+113
-5
@@ -187,13 +187,18 @@ impl Agent {
|
||||
);
|
||||
|
||||
// First check thread state without holding lock during I/O
|
||||
let thread_state = {
|
||||
let (thread_state, approval_context) = {
|
||||
let sess = session.lock().await;
|
||||
let thread = sess
|
||||
.threads
|
||||
.get(&thread_id)
|
||||
.ok_or_else(|| Error::from(crate::error::JobError::NotFound { id: thread_id }))?;
|
||||
thread.state
|
||||
let approval_context = thread.pending_approval.as_ref().map(|a| {
|
||||
let desc_preview =
|
||||
crate::agent::agent_loop::truncate_for_preview(&a.description, 80);
|
||||
(a.tool_name.clone(), desc_preview)
|
||||
});
|
||||
(thread.state, approval_context)
|
||||
};
|
||||
|
||||
tracing::debug!(
|
||||
@@ -221,9 +226,13 @@ impl Agent {
|
||||
thread_id = %thread_id,
|
||||
"Thread awaiting approval, rejecting new input"
|
||||
);
|
||||
return Ok(SubmissionResult::error(
|
||||
"Waiting for approval. Use /interrupt to cancel.",
|
||||
));
|
||||
let msg = match approval_context {
|
||||
Some((tool_name, desc_preview)) => format!(
|
||||
"Waiting for approval: {tool_name} — {desc_preview}. Use /interrupt to cancel."
|
||||
),
|
||||
None => "Waiting for approval. Use /interrupt to cancel.".to_string(),
|
||||
};
|
||||
return Ok(SubmissionResult::pending(msg));
|
||||
}
|
||||
ThreadState::Completed => {
|
||||
tracing::warn!(
|
||||
@@ -1917,4 +1926,103 @@ mod tests {
|
||||
created_at: chrono::Utc::now(),
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_awaiting_approval_rejection_includes_tool_context() {
|
||||
// Test that when a thread is in AwaitingApproval state and receives a new message,
|
||||
// process_user_input rejects it with a non-error status that includes tool context.
|
||||
use crate::agent::session::{PendingApproval, Session, Thread, ThreadState};
|
||||
use uuid::Uuid;
|
||||
|
||||
let session_id = Uuid::new_v4();
|
||||
let thread_id = Uuid::new_v4();
|
||||
let mut thread = Thread::with_id(thread_id, session_id);
|
||||
|
||||
// Set thread to AwaitingApproval with a pending tool approval
|
||||
let pending = PendingApproval {
|
||||
request_id: Uuid::new_v4(),
|
||||
tool_name: "shell".to_string(),
|
||||
parameters: serde_json::json!({"command": "echo hello"}),
|
||||
display_parameters: serde_json::json!({"command": "[REDACTED]"}),
|
||||
description: "Execute: echo hello".to_string(),
|
||||
tool_call_id: "call_0".to_string(),
|
||||
context_messages: vec![],
|
||||
deferred_tool_calls: vec![],
|
||||
user_timezone: None,
|
||||
};
|
||||
thread.await_approval(pending);
|
||||
|
||||
let mut session = Session::new("test-user");
|
||||
session.threads.insert(thread_id, thread);
|
||||
|
||||
// Verify thread is in AwaitingApproval state
|
||||
assert_eq!(
|
||||
session.threads[&thread_id].state,
|
||||
ThreadState::AwaitingApproval
|
||||
);
|
||||
|
||||
let result = extract_approval_message(&session, thread_id);
|
||||
|
||||
// Verify result is an Ok with a message (not an Error)
|
||||
match result {
|
||||
Ok(Some(msg)) => {
|
||||
// Should NOT start with "Error:"
|
||||
assert!(
|
||||
!msg.to_lowercase().starts_with("error:"),
|
||||
"Approval rejection should not have 'Error:' prefix. Got: {}",
|
||||
msg
|
||||
);
|
||||
|
||||
// Should contain "waiting for approval"
|
||||
assert!(
|
||||
msg.to_lowercase().contains("waiting for approval"),
|
||||
"Should contain 'waiting for approval'. Got: {}",
|
||||
msg
|
||||
);
|
||||
|
||||
// Should contain the tool name
|
||||
assert!(
|
||||
msg.contains("shell"),
|
||||
"Should contain tool name 'shell'. Got: {}",
|
||||
msg
|
||||
);
|
||||
|
||||
// Should contain the description (or truncated version)
|
||||
assert!(
|
||||
msg.contains("echo hello"),
|
||||
"Should contain description 'echo hello'. Got: {}",
|
||||
msg
|
||||
);
|
||||
}
|
||||
_ => panic!("Expected approval rejection message"),
|
||||
}
|
||||
}
|
||||
|
||||
// Helper function to extract the approval message without needing a full Agent instance
|
||||
fn extract_approval_message(
|
||||
session: &crate::agent::session::Session,
|
||||
thread_id: Uuid,
|
||||
) -> Result<Option<String>, crate::error::Error> {
|
||||
let thread = session.threads.get(&thread_id).ok_or_else(|| {
|
||||
crate::error::Error::from(crate::error::JobError::NotFound { id: thread_id })
|
||||
})?;
|
||||
|
||||
if thread.state == ThreadState::AwaitingApproval {
|
||||
let approval_context = thread.pending_approval.as_ref().map(|a| {
|
||||
let desc_preview =
|
||||
crate::agent::agent_loop::truncate_for_preview(&a.description, 80);
|
||||
(a.tool_name.clone(), desc_preview)
|
||||
});
|
||||
|
||||
let msg = match approval_context {
|
||||
Some((tool_name, desc_preview)) => format!(
|
||||
"Waiting for approval: {tool_name} — {desc_preview}. Use /interrupt to cancel."
|
||||
),
|
||||
None => "Waiting for approval. Use /interrupt to cancel.".to_string(),
|
||||
};
|
||||
Ok(Some(msg))
|
||||
} else {
|
||||
Ok(None)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -130,3 +130,62 @@ async def test_approval_params_toggle(page):
|
||||
await toggle.click()
|
||||
await page.wait_for_timeout(300)
|
||||
assert await params.is_hidden(), "Parameters should be hidden after second toggle"
|
||||
|
||||
|
||||
async def test_waiting_for_approval_message_no_error_prefix(page):
|
||||
"""Verify that input submitted while awaiting approval shows non-error status with tool context.
|
||||
|
||||
Tests the real flow: show approval card, then attempt to send input while approval is pending.
|
||||
Backend rejects with Pending result (not Error), and message includes tool context.
|
||||
"""
|
||||
# First, inject an approval card to simulate the thread being in AwaitingApproval state
|
||||
await page.evaluate("""
|
||||
showApproval({
|
||||
request_id: 'test-req-waiting-approval',
|
||||
thread_id: currentThreadId,
|
||||
tool_name: 'shell',
|
||||
description: 'Execute: echo hello',
|
||||
parameters: '{"command": "echo hello"}'
|
||||
})
|
||||
""")
|
||||
|
||||
# Wait for approval card to be visible (thread is now in AwaitingApproval state)
|
||||
card = page.locator('.approval-card[data-request-id="test-req-waiting-approval"]')
|
||||
await card.wait_for(state="visible", timeout=5000)
|
||||
|
||||
# Record initial message count
|
||||
initial_count = await page.locator(SEL["message_assistant"]).count()
|
||||
|
||||
# Now attempt to send input while approval is pending
|
||||
# (the backend will reject this and return the "Waiting for approval" status message)
|
||||
chat_input = page.locator(SEL["chat_input"])
|
||||
await chat_input.fill("Test input while awaiting approval")
|
||||
await chat_input.press("Enter")
|
||||
|
||||
# Wait for the status message from the backend rejection
|
||||
await page.wait_for_function(
|
||||
f"() => document.querySelectorAll('{SEL['message_assistant']}').length > {initial_count}",
|
||||
timeout=10000,
|
||||
)
|
||||
|
||||
# Get the new status message
|
||||
last_msg = page.locator(SEL["message_assistant"]).last
|
||||
msg_text = await last_msg.text_content()
|
||||
|
||||
# Verify no "Error:" prefix
|
||||
assert not msg_text.lower().startswith("error:"), (
|
||||
f"Approval rejection must NOT have 'Error:' prefix. Got: {msg_text!r}"
|
||||
)
|
||||
|
||||
# Verify it contains "waiting for approval"
|
||||
assert "waiting for approval" in msg_text.lower(), (
|
||||
f"Expected 'Waiting for approval' text. Got: {msg_text!r}"
|
||||
)
|
||||
|
||||
# Verify it contains the tool name and description
|
||||
assert "shell" in msg_text.lower(), (
|
||||
f"Expected tool name 'shell' in message. Got: {msg_text!r}"
|
||||
)
|
||||
assert "echo hello" in msg_text, (
|
||||
f"Expected tool description in message. Got: {msg_text!r}"
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user