Skip to content

Conversation

@Pavanmanikanta98
Copy link

@Pavanmanikanta98 Pavanmanikanta98 commented Nov 15, 2025

Closes #2992

Summary

Adds validation to prevent message history from starting with ModelResponse, which causes provider API errors (e.g., Bedrock: "conversation must start with user message").

Changes

  • Added validation in _agent_graph.py after message cleaning (line 218)
  • Raises clear UserError before invalid history reaches provider
  • Added test to verify the exception is raised for invalid history

Testing

  • ✅ Test verifies UserError is raised when history starts with ModelResponse
  • ✅ All existing tests pass (non-exception behavior already covered)

Related

Add validation in UserPromptNode to raise UserError if message history starts with ModelResponse, ensuring conversations begin with a user message (ModelRequest)

Include comprehensive tests for invalid history, valid history, empty history, multiple messages, and validation after message cleaning to prevent issues with malformed conversation logs
@Pavanmanikanta98 Pavanmanikanta98 force-pushed the fix/issue-2992-disallow-initial-model-response branch from aaee6e4 to 6ef2b8c Compare November 15, 2025 18:07
@Pavanmanikanta98
Copy link
Author

Note on test_outlines.py Changes

During implementation, I discovered that tests/models/test_outlines.py::test_input_format was failing in CI.

Root Cause

The test was using message histories that started with ModelResponse:

# Before:
tool_call_message_history: list[ModelMessage] = [
    ModelResponse(parts=[ToolCallPart(...)]),  # Invalid: starts with ModelResponse
    ModelRequest(parts=[ToolReturnPart(...)]),
]

This worked previously because there was no validation at the framework level. The test's purpose was to verify that OutlinesModel rejects tool calls, not to test message history validation.

However, with the new validation in place, the test now fails early with:

UserError: Message history cannot start with a `ModelResponse`. Conversations must begin with a user message.

This prevented the test from reaching the OutlinesModel code it was meant to test.

Solution

I updated the test to use valid message history structure by adding an initial ModelRequest:

# After:
tool_call_message_history: list[ModelMessage] = [
    ModelRequest(parts=[UserPromptPart(content='some user prompt')]),  # Valid start
    ModelResponse(parts=[ToolCallPart(...)]),
    ModelRequest(parts=[ToolReturnPart(...)]),
]

This maintains the test's original purpose (verifying OutlinesModel rejects tool calls) while complying with the correct message history structure that all LLM providers require.

Rationale

This change makes the test more correct:

  • ✅ Real conversations must start with a user message
  • ✅ All major LLM providers (Bedrock, OpenAI, Anthropic) require this
  • ✅ The framework should validate this before model-specific code runs
  • ✅ Tests should use valid data unless specifically testing validation

The same fix was applied to the file_part_message_history test case for consistency.


Let me know if you'd prefer a different approach for the test data (e.g., more realistic user prompts like "What is my location?" for the tool call test).

@DouweM DouweM self-assigned this Nov 17, 2025
Only keep the test that verifies the exception is raised.
The non-exception behavior is already tested by existing tests.

Addresses review feedback from @DouweM on PR pydantic#3440
@Pavanmanikanta98
Copy link
Author

Thanks for the review @DouweM! I've removed the redundant tests and kept only test_message_history_cannot_start_with_model_response which verifies the exception is raised.

Changes made:

  • ✅ Removed test_message_history_starts_with_model_request
  • ✅ Removed test_empty_message_history_is_valid
  • ✅ Removed test_message_history_with_multiple_messages
  • ✅ Removed test_validation_happens_after_cleaning
  • ✅ Kept only the exception validation test

All tests and pre-commit checks pass locally. Ready for re-review!

@Pavanmanikanta98
Copy link
Author

@DouweM CI failures are from Temporal server errors (temporal.download returning 500), not this PR. All agent tests pass. The Temporal download issue should resolve soon.

@DouweM DouweM enabled auto-merge (squash) November 18, 2025 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Forbid first ModelResponse message

2 participants