Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions crates/goose-acp/tests/common_tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ pub async fn run_basic_completion<S: Session>() {
let output = session
.prompt("what is 1+1", PermissionDecision::Cancel)
.await;
assert!(output.text.contains("2"));
assert_eq!(output.text, "2");
expected_session_id.assert_matches(&session.id().0);
}

Expand Down Expand Up @@ -65,7 +65,7 @@ pub async fn run_mcp_http_server<S: Session>() {
PermissionDecision::Cancel,
)
.await;
assert!(output.text.contains(FAKE_CODE));
assert_eq!(output.text, FAKE_CODE);
expected_session_id.assert_matches(&session.id().0);
}

Expand All @@ -85,7 +85,7 @@ pub async fn run_builtin_and_mcp<S: Session>() {
include_str!("../test_data/openai_builtin_execute.txt"),
),
(
r#""writeResult": "Successfully wrote to /tmp/result.txt"#.into(),
r#"\"writeResult\": \"Successfully wrote to /tmp/result.txt"#.into(),
include_str!("../test_data/openai_builtin_final.txt"),
),
],
Expand All @@ -104,10 +104,13 @@ pub async fn run_builtin_and_mcp<S: Session>() {
let mut session = S::new(config, openai).await;
expected_session_id.set(session.id());

let _ = session.prompt(prompt, PermissionDecision::Cancel).await;
let output = session.prompt(prompt, PermissionDecision::Cancel).await;
if matches!(output.tool_status, Some(ToolCallStatus::Failed)) || output.text.contains("error") {
panic!("{}", output.text);
}

let result = fs::read_to_string("/tmp/result.txt").unwrap_or_default();
assert!(result.contains(FAKE_CODE));
assert_eq!(result, format!("{FAKE_CODE}\n"));
expected_session_id.assert_matches(&session.id().0);
}

Expand Down Expand Up @@ -215,6 +218,6 @@ pub async fn run_configured_extension<S: Session>() {
expected_session_id.set(session.id());

let output = session.prompt(prompt, PermissionDecision::Cancel).await;
assert!(output.text.contains(FAKE_CODE));
assert_eq!(output.text, FAKE_CODE);
expected_session_id.assert_matches(&session.id().0);
}
43 changes: 24 additions & 19 deletions crates/goose-acp/tests/fixtures/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
#![recursion_limit = "256"]
#![allow(unused_attributes)]

use assert_json_diff::{assert_json_matches_no_panic, CompareMode, Config};
use async_trait::async_trait;
use fs_err as fs;
use goose::builtin_extension::register_builtin_extensions;
Expand Down Expand Up @@ -153,8 +152,9 @@ impl OpenAiFixture {
let queue = queue.clone();
let expected_session_id = expected_session_id.clone();
move |req: &wiremock::Request| {
let body = String::from_utf8_lossy(&req.body);
let body = std::str::from_utf8(&req.body).unwrap_or("");

// Validate session ID header
let actual = req
.headers
.get(SESSION_ID_HEADER)
Expand All @@ -174,28 +174,30 @@ impl OpenAiFixture {
));
}

let (expected_body, response) = {
let mut q = queue.lock().unwrap();
q.pop_front().unwrap_or_default()
};

if body.contains(&expected_body) && !expected_body.is_empty() {
// See if the actual request matches the expected pattern
let mut q = queue.lock().unwrap();
let (expected_body, response) = q.front().cloned().unwrap_or_default();
if !expected_body.is_empty() && body.contains(&expected_body) {
q.pop_front();
return ResponseTemplate::new(200)
.insert_header("content-type", "text/event-stream")
.set_body_string(response);
}

// Coerce non-json to allow a uniform JSON diff error response.
let exp = serde_json::from_str(&expected_body)
.unwrap_or(serde_json::Value::String(expected_body.clone()));
let act = serde_json::from_str(&body)
.unwrap_or(serde_json::Value::String(body.to_string()));
let diff =
assert_json_matches_no_panic(&exp, &act, Config::new(CompareMode::Strict))
.unwrap_err();
drop(q);

// If there was no body, the request was unexpected. Otherwise, it is a mismatch.
let message = if expected_body.is_empty() {
format!("Unexpected request:\n {}", body)
} else {
format!(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we lose something here with actual diff of the bodies together? Seems like we're only checking against empty.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah the logic was wrong before as it was a substring search. So, what was happening is we were doing a diff of a substring vs an entire json body.

so it was doing a json diff of format!(r#"</info-msg>\n{prompt}""#) vs the actual json body.

This was a relic of an old version which did complete request matching, and is a bug in the current design. that's why json diff isn't useful here.

Make sense?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks for clarifying!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ps #6969

"Expected body to contain:\n {}\n\nActual body:\n {}",
expected_body, body
)
};
// Use OpenAI's error response schema so the provider will pass the error through.
ResponseTemplate::new(417)
.insert_header("content-type", "application/json")
.set_body_json(serde_json::json!({"error": {"message": diff}}))
.set_body_json(serde_json::json!({"error": {"message": message}}))
}
})
.mount(&mock_server)
Expand Down Expand Up @@ -464,7 +466,10 @@ where
runtime.block_on(fut);
})
.unwrap();
handle.join().unwrap();
if let Err(err) = handle.join() {
// Re-raise the original panic so the test shows the real failure message.
std::panic::resume_unwind(err);
}
}

pub mod server;
Loading