Skip to content

fix(acp): fixtures now raise content mismatch errors#6912

Merged
codefromthecrypt merged 1 commit intomainfrom
acp-test-polish
Feb 5, 2026
Merged

fix(acp): fixtures now raise content mismatch errors#6912
codefromthecrypt merged 1 commit intomainfrom
acp-test-polish

Conversation

@codefromthecrypt
Copy link
Collaborator

@codefromthecrypt codefromthecrypt commented Feb 3, 2026

Summary

Fix ACP test fixture to show clear error messages on content mismatch. Before, it returned empty string which defeated the logic involved.

Type of Change

  • Bug fix
  • Tests

AI Assistance

  • This PR was created or reviewed with AI assistance

Testing

Introduced a typo and fixed the before and after issue

Related Issues

Relates to #6765

Diff

I introduced a typo (space) in the request match expression to trigger a failure.

Before

  json atoms at path "(root)" are not equal:                                                                                                                                           
      lhs:                                                                                                                                                                             
          ""                                                                                                                                                                           
      rhs:                                                                                                                                                                             
          {                                                                                                                                                                            
            "messages": [                                                                                                                                                              
              ...                                                                                                                                                                      

After:

  Expected body to contain:                                                                                                                                                            
    lookup[\"get_ code\"]({}): string - Get the code                                                                                                                                   
                                                                                                                                                                                       
  Actual body:                                                                                                                                                                         
    {"messages":[{"content":"You are a general-purpose AI agent called goose...     

@codefromthecrypt codefromthecrypt marked this pull request as ready for review February 3, 2026 07:53
Copilot AI review requested due to automatic review settings February 3, 2026 07:53
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a bug in the ACP test fixture error reporting. Previously, when request body matching failed, the fixture would pop the expected value from the queue before checking if it matched, causing error messages to show the wrong expected value (or empty string if queue was exhausted). This made debugging test failures very difficult.

Changes:

  • Fixed queue handling to peek before popping, ensuring error messages show the correct expected value
  • Replaced JSON diff error messages with simple, readable expected/actual format
  • Improved panic propagation in test runner to show actual failure messages
  • Made test assertions more precise with assert_eq! instead of contains checks

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
crates/goose-acp/tests/fixtures/mod.rs Fixed queue peek-before-pop logic, removed assert_json_diff dependency, improved error message formatting, and fixed panic propagation in test runner
crates/goose-acp/tests/common_tests/mod.rs Changed test assertions from contains to assert_eq! for more precise validation, added error checking for run_builtin_and_mcp test

let message = if expected_body.is_empty() {
format!("Unexpected request:\n {}", body)
} else {
format!(
Copy link
Collaborator

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
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
Collaborator

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
Collaborator Author

Choose a reason for hiding this comment

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

ps #6969

@katzdave katzdave added this pull request to the merge queue Feb 4, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 4, 2026
@codefromthecrypt codefromthecrypt added this pull request to the merge queue Feb 5, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 5, 2026
Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt codefromthecrypt added this pull request to the merge queue Feb 5, 2026
Merged via the queue into main with commit aa4f320 Feb 5, 2026
15 of 16 checks passed
@codefromthecrypt codefromthecrypt deleted the acp-test-polish branch February 5, 2026 06:42
tlongwell-block added a commit that referenced this pull request Feb 5, 2026
* origin/main:
  fix: detect context length errors in GCP Vertex AI provider (#6976)
  Added the ability to escape Jinja variables in recipes to include them in prompts (#6975)
  Bug Fix: bump pctx (#6967)
  fix(acp): fixtures now raise content mismatch errors (#6912)
  custom provider form minor improvements (#6966)
  Fix 'Edit In Place' feature appending instead of editing messages (#6955)
  docs: change RPI slash commands (#6963)
kuccello pushed a commit to kuccello/goose that referenced this pull request Feb 7, 2026
Signed-off-by: Adrian Cole <adrian@tetrate.io>
Tyler-Hardin pushed a commit to Tyler-Hardin/goose that referenced this pull request Feb 11, 2026
Signed-off-by: Adrian Cole <adrian@tetrate.io>
Tyler-Hardin pushed a commit to Tyler-Hardin/goose that referenced this pull request Feb 11, 2026
Signed-off-by: Adrian Cole <adrian@tetrate.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants