From 392dbe8b56ebcce822dab28cebf418a3d27273e1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 27 Sep 2025 21:07:39 +0000 Subject: [PATCH 1/3] Initial plan From 82ad0bf5b98f874efb2e61df692728c3008efd4b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 27 Sep 2025 21:16:32 +0000 Subject: [PATCH 2/3] Fix OpenAI schema validation for discriminated unions - Add schema wrapping for oneOf schemas to ensure root type is 'object' - Add response unwrapping for both parsed and JSON fallback paths - Include comprehensive tests for schema transformation - Maintain backward compatibility with non-union schemas Co-authored-by: JPrier <24302717+JPrier@users.noreply.github.com> --- src/modelAccessors/openai_accessor.py | 45 +++++++- .../test_openai_accessor_schema_fix.py | 108 ++++++++++++++++++ 2 files changed, 152 insertions(+), 1 deletion(-) create mode 100644 tests/modelAccessors/test_openai_accessor_schema_fix.py diff --git a/src/modelAccessors/openai_accessor.py b/src/modelAccessors/openai_accessor.py index e72c79c..117378c 100644 --- a/src/modelAccessors/openai_accessor.py +++ b/src/modelAccessors/openai_accessor.py @@ -1,3 +1,4 @@ +import json from os import environ from typing import Any, Optional @@ -47,12 +48,15 @@ def call_model( {"role": "user", "content": prompt}, ] + # Ensure the schema is compatible with OpenAI's requirements + openai_schema, is_wrapped = self._ensure_openai_compatible_schema(schema) + kwargs = { "model": model, "messages": messages, "response_format": { "type": "json_schema", - "json_schema": {"name": "response", "schema": schema, "strict": True}, + "json_schema": {"name": "response", "schema": openai_schema, "strict": True}, }, } @@ -65,17 +69,56 @@ def call_model( # Use parsed response when available parsed = getattr(message, "parsed", None) if parsed is not None: + # If we wrapped the schema, unwrap the response + if is_wrapped and isinstance(parsed, dict) and "response" in parsed: + parsed = parsed["response"] return adapter.validate_python(parsed) # Fallback to JSON parsing if parsed is not available raw = message.content if not raw: raise ValueError("No content in response") + + # If we wrapped the schema, parse JSON and unwrap the response + if is_wrapped: + import json + parsed_json = json.loads(raw) + if isinstance(parsed_json, dict) and "response" in parsed_json: + raw = json.dumps(parsed_json["response"]) + return adapter.validate_json(raw) def supports_tools(self, model: str) -> bool: """Check if model supports native tools/function calling""" return model in self.tool_supported_models + + def _ensure_openai_compatible_schema(self, schema: dict) -> tuple[dict, bool]: + """ + Ensure the schema is compatible with OpenAI's structured output requirements. + + OpenAI requires the root schema to have 'type': 'object', but Pydantic's + discriminated unions generate schemas with oneOf at the root level. + + Returns: + tuple: (modified_schema, was_wrapped) + """ + # Check if the schema already has a root type of "object" + if schema.get("type") == "object": + return schema, False + + # If it's a oneOf/anyOf schema (discriminated union), wrap it in an object + if "oneOf" in schema or "anyOf" in schema: + wrapped_schema = { + "type": "object", + "properties": { + "response": schema + }, + "required": ["response"], + "additionalProperties": False + } + return wrapped_schema, True + + return schema, False def _convert_to_openai_tools(self, tools: list[Tool]) -> list[dict[str, Any]]: """Convert our Tool objects to OpenAI's tool format""" diff --git a/tests/modelAccessors/test_openai_accessor_schema_fix.py b/tests/modelAccessors/test_openai_accessor_schema_fix.py new file mode 100644 index 0000000..9f9cbf2 --- /dev/null +++ b/tests/modelAccessors/test_openai_accessor_schema_fix.py @@ -0,0 +1,108 @@ +import json +from typing import cast +from unittest.mock import Mock, patch + +from pydantic import TypeAdapter + +from src.modelAccessors.openai_accessor import OpenAIAccessor +from src.dataModel.model_response import ClarifierResponse, ModelResponse + + +def test_ensure_openai_compatible_schema(): + """Test that discriminated union schemas are properly wrapped for OpenAI compatibility.""" + # Test with discriminated union (should be wrapped) + adapter = cast(TypeAdapter[ModelResponse], TypeAdapter(ClarifierResponse)) + union_schema = adapter.json_schema() + + # Create accessor without initializing the client + with patch('src.modelAccessors.openai_accessor.OpenAI'): + accessor = OpenAIAccessor() + + # Test union schema wrapping + fixed_schema, was_wrapped = accessor._ensure_openai_compatible_schema(union_schema) + assert was_wrapped is True + assert fixed_schema["type"] == "object" + assert "properties" in fixed_schema + assert "response" in fixed_schema["properties"] + assert fixed_schema["required"] == ["response"] + assert fixed_schema["additionalProperties"] is False + assert fixed_schema["properties"]["response"] == union_schema + + # Test object schema (should not be wrapped) + object_schema = { + "type": "object", + "properties": {"test": {"type": "string"}}, + "required": ["test"] + } + unchanged_schema, was_wrapped = accessor._ensure_openai_compatible_schema(object_schema) + assert was_wrapped is False + assert unchanged_schema == object_schema + + +def test_response_unwrapping(): + """Test that wrapped responses are properly unwrapped.""" + # Mock the OpenAI client and response + with patch('src.modelAccessors.openai_accessor.OpenAI') as mock_openai: + accessor = OpenAIAccessor() + mock_client = Mock() + mock_openai.return_value = mock_client + accessor.client = mock_client + + # Create mock response with wrapped data + mock_message = Mock() + mock_message.parsed = {"response": {"type": "implemented", "content": "test"}} + mock_response = Mock() + mock_response.choices = [Mock(message=mock_message)] + mock_client.chat.completions.create.return_value = mock_response + + # Test the call + adapter = cast(TypeAdapter[ModelResponse], TypeAdapter(ClarifierResponse)) + schema = adapter.json_schema() + + result = accessor.call_model( + prompt="test", + adapter=adapter, + schema=schema + ) + + # Verify the response was unwrapped correctly + assert hasattr(result, 'type') + assert result.type == "implemented" + + +def test_json_fallback_unwrapping(): + """Test that JSON fallback also unwraps responses correctly.""" + with patch('src.modelAccessors.openai_accessor.OpenAI') as mock_openai: + accessor = OpenAIAccessor() + mock_client = Mock() + mock_openai.return_value = mock_client + accessor.client = mock_client + + # Create mock response with JSON content (no parsed attribute) + mock_message = Mock() + mock_message.parsed = None + mock_message.content = json.dumps({"response": {"type": "implemented", "content": "test"}}) + mock_response = Mock() + mock_response.choices = [Mock(message=mock_message)] + mock_client.chat.completions.create.return_value = mock_response + + # Test the call + adapter = cast(TypeAdapter[ModelResponse], TypeAdapter(ClarifierResponse)) + schema = adapter.json_schema() + + result = accessor.call_model( + prompt="test", + adapter=adapter, + schema=schema + ) + + # Verify the response was unwrapped correctly + assert hasattr(result, 'type') + assert result.type == "implemented" + + +if __name__ == "__main__": + test_ensure_openai_compatible_schema() + test_response_unwrapping() + test_json_fallback_unwrapping() + print("All tests passed!") \ No newline at end of file From 5b16c9b394bf2652d50405433b6b01ea54099dd9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 27 Sep 2025 22:14:56 +0000 Subject: [PATCH 3/3] Refactor OpenAI schema handling to remove is_wrapped variable - Replace _ensure_openai_compatible_schema with _prepare_schema_for_openai - Add _extract_response_from_openai_format for deterministic unwrapping - Remove is_wrapped variable and inline JSON import - Move schema fix tests to main openai_accessor test file - Maintain same functionality with cleaner separation of concerns Co-authored-by: JPrier <24302717+JPrier@users.noreply.github.com> --- src/modelAccessors/openai_accessor.py | 52 ++++---- tests/modelAccessors/test_openai_accessor.py | 121 +++++++++++++++++- .../test_openai_accessor_schema_fix.py | 108 ---------------- 3 files changed, 149 insertions(+), 132 deletions(-) delete mode 100644 tests/modelAccessors/test_openai_accessor_schema_fix.py diff --git a/src/modelAccessors/openai_accessor.py b/src/modelAccessors/openai_accessor.py index 117378c..8e06a03 100644 --- a/src/modelAccessors/openai_accessor.py +++ b/src/modelAccessors/openai_accessor.py @@ -49,7 +49,7 @@ def call_model( ] # Ensure the schema is compatible with OpenAI's requirements - openai_schema, is_wrapped = self._ensure_openai_compatible_schema(schema) + openai_schema = self._prepare_schema_for_openai(schema) kwargs = { "model": model, @@ -69,46 +69,36 @@ def call_model( # Use parsed response when available parsed = getattr(message, "parsed", None) if parsed is not None: - # If we wrapped the schema, unwrap the response - if is_wrapped and isinstance(parsed, dict) and "response" in parsed: - parsed = parsed["response"] - return adapter.validate_python(parsed) + return adapter.validate_python(self._extract_response_from_openai_format(parsed, schema)) # Fallback to JSON parsing if parsed is not available raw = message.content if not raw: raise ValueError("No content in response") - # If we wrapped the schema, parse JSON and unwrap the response - if is_wrapped: - import json - parsed_json = json.loads(raw) - if isinstance(parsed_json, dict) and "response" in parsed_json: - raw = json.dumps(parsed_json["response"]) - - return adapter.validate_json(raw) + # Parse and extract response + parsed_json = json.loads(raw) + extracted_response = self._extract_response_from_openai_format(parsed_json, schema) + return adapter.validate_json(json.dumps(extracted_response)) def supports_tools(self, model: str) -> bool: """Check if model supports native tools/function calling""" return model in self.tool_supported_models - def _ensure_openai_compatible_schema(self, schema: dict) -> tuple[dict, bool]: + def _prepare_schema_for_openai(self, schema: dict) -> dict: """ - Ensure the schema is compatible with OpenAI's structured output requirements. + Prepare schema for OpenAI's structured output requirements. OpenAI requires the root schema to have 'type': 'object', but Pydantic's discriminated unions generate schemas with oneOf at the root level. - - Returns: - tuple: (modified_schema, was_wrapped) """ # Check if the schema already has a root type of "object" if schema.get("type") == "object": - return schema, False + return schema # If it's a oneOf/anyOf schema (discriminated union), wrap it in an object if "oneOf" in schema or "anyOf" in schema: - wrapped_schema = { + return { "type": "object", "properties": { "response": schema @@ -116,9 +106,27 @@ def _ensure_openai_compatible_schema(self, schema: dict) -> tuple[dict, bool]: "required": ["response"], "additionalProperties": False } - return wrapped_schema, True - return schema, False + return schema + + def _extract_response_from_openai_format(self, response_data: Any, original_schema: dict) -> Any: + """ + Extract the actual response from OpenAI's format, handling wrapped schemas. + + If the original schema was wrapped (oneOf/anyOf), unwrap the response. + Otherwise, return the response as-is. + """ + # Determine if the original schema was wrapped + schema_was_wrapped = ( + original_schema.get("type") != "object" and + ("oneOf" in original_schema or "anyOf" in original_schema) + ) + + # If schema was wrapped and response has the wrapped structure, unwrap it + if schema_was_wrapped and isinstance(response_data, dict) and "response" in response_data: + return response_data["response"] + + return response_data def _convert_to_openai_tools(self, tools: list[Tool]) -> list[dict[str, Any]]: """Convert our Tool objects to OpenAI's tool format""" diff --git a/tests/modelAccessors/test_openai_accessor.py b/tests/modelAccessors/test_openai_accessor.py index c642a13..e7bfa1a 100644 --- a/tests/modelAccessors/test_openai_accessor.py +++ b/tests/modelAccessors/test_openai_accessor.py @@ -1,12 +1,14 @@ """Test OpenAI accessor model validation and structured output support.""" +import json import os +from typing import cast from unittest.mock import Mock, patch from pydantic import TypeAdapter from src.modelAccessors.openai_accessor import OpenAIAccessor -from src.dataModel.model_response import ImplementedResponse +from src.dataModel.model_response import ClarifierResponse, ImplementedResponse, ModelResponse def test_supported_model_validation(): @@ -118,4 +120,119 @@ def test_tool_support(): assert accessor.supports_tools("gpt-5-nano") # Old models shouldn't be in tool support either - assert not accessor.supports_tools("gpt-4") \ No newline at end of file + assert not accessor.supports_tools("gpt-4") + + +def test_prepare_schema_for_openai(): + """Test that discriminated union schemas are properly wrapped for OpenAI compatibility.""" + # Test with discriminated union (should be wrapped) + adapter = cast(TypeAdapter[ModelResponse], TypeAdapter(ClarifierResponse)) + union_schema = adapter.json_schema() + + # Create accessor without initializing the client + with patch('src.modelAccessors.openai_accessor.OpenAI'): + accessor = OpenAIAccessor() + + # Test union schema wrapping + fixed_schema = accessor._prepare_schema_for_openai(union_schema) + assert fixed_schema["type"] == "object" + assert "properties" in fixed_schema + assert "response" in fixed_schema["properties"] + assert fixed_schema["required"] == ["response"] + assert fixed_schema["additionalProperties"] is False + assert fixed_schema["properties"]["response"] == union_schema + + # Test object schema (should not be wrapped) + object_schema = { + "type": "object", + "properties": {"test": {"type": "string"}}, + "required": ["test"] + } + unchanged_schema = accessor._prepare_schema_for_openai(object_schema) + assert unchanged_schema == object_schema + + +def test_extract_response_from_openai_format(): + """Test that responses are properly extracted from OpenAI format.""" + with patch('src.modelAccessors.openai_accessor.OpenAI'): + accessor = OpenAIAccessor() + + # Test with discriminated union schema (should be unwrapped) + adapter = cast(TypeAdapter[ModelResponse], TypeAdapter(ClarifierResponse)) + union_schema = adapter.json_schema() + + wrapped_response = {"response": {"type": "implemented", "content": "test"}} + extracted = accessor._extract_response_from_openai_format(wrapped_response, union_schema) + assert extracted == {"type": "implemented", "content": "test"} + + # Test with object schema (should not be unwrapped) + object_schema = { + "type": "object", + "properties": {"test": {"type": "string"}}, + "required": ["test"] + } + object_response = {"test": "value"} + unchanged = accessor._extract_response_from_openai_format(object_response, object_schema) + assert unchanged == {"test": "value"} + + +def test_response_unwrapping_integration(): + """Test that wrapped responses are properly unwrapped in the full call flow.""" + # Mock the OpenAI client and response + with patch('src.modelAccessors.openai_accessor.OpenAI') as mock_openai: + accessor = OpenAIAccessor() + mock_client = Mock() + mock_openai.return_value = mock_client + accessor.client = mock_client + + # Create mock response with wrapped data + mock_message = Mock() + mock_message.parsed = {"response": {"type": "implemented", "content": "test"}} + mock_response = Mock() + mock_response.choices = [Mock(message=mock_message)] + mock_client.chat.completions.create.return_value = mock_response + + # Test the call + adapter = cast(TypeAdapter[ModelResponse], TypeAdapter(ClarifierResponse)) + schema = adapter.json_schema() + + result = accessor.call_model( + prompt="test", + adapter=adapter, + schema=schema + ) + + # Verify the response was unwrapped correctly + assert hasattr(result, 'type') + assert result.type == "implemented" + + +def test_json_fallback_unwrapping_integration(): + """Test that JSON fallback also unwraps responses correctly.""" + with patch('src.modelAccessors.openai_accessor.OpenAI') as mock_openai: + accessor = OpenAIAccessor() + mock_client = Mock() + mock_openai.return_value = mock_client + accessor.client = mock_client + + # Create mock response with JSON content (no parsed attribute) + mock_message = Mock() + mock_message.parsed = None + mock_message.content = json.dumps({"response": {"type": "implemented", "content": "test"}}) + mock_response = Mock() + mock_response.choices = [Mock(message=mock_message)] + mock_client.chat.completions.create.return_value = mock_response + + # Test the call + adapter = cast(TypeAdapter[ModelResponse], TypeAdapter(ClarifierResponse)) + schema = adapter.json_schema() + + result = accessor.call_model( + prompt="test", + adapter=adapter, + schema=schema + ) + + # Verify the response was unwrapped correctly + assert hasattr(result, 'type') + assert result.type == "implemented" \ No newline at end of file diff --git a/tests/modelAccessors/test_openai_accessor_schema_fix.py b/tests/modelAccessors/test_openai_accessor_schema_fix.py deleted file mode 100644 index 9f9cbf2..0000000 --- a/tests/modelAccessors/test_openai_accessor_schema_fix.py +++ /dev/null @@ -1,108 +0,0 @@ -import json -from typing import cast -from unittest.mock import Mock, patch - -from pydantic import TypeAdapter - -from src.modelAccessors.openai_accessor import OpenAIAccessor -from src.dataModel.model_response import ClarifierResponse, ModelResponse - - -def test_ensure_openai_compatible_schema(): - """Test that discriminated union schemas are properly wrapped for OpenAI compatibility.""" - # Test with discriminated union (should be wrapped) - adapter = cast(TypeAdapter[ModelResponse], TypeAdapter(ClarifierResponse)) - union_schema = adapter.json_schema() - - # Create accessor without initializing the client - with patch('src.modelAccessors.openai_accessor.OpenAI'): - accessor = OpenAIAccessor() - - # Test union schema wrapping - fixed_schema, was_wrapped = accessor._ensure_openai_compatible_schema(union_schema) - assert was_wrapped is True - assert fixed_schema["type"] == "object" - assert "properties" in fixed_schema - assert "response" in fixed_schema["properties"] - assert fixed_schema["required"] == ["response"] - assert fixed_schema["additionalProperties"] is False - assert fixed_schema["properties"]["response"] == union_schema - - # Test object schema (should not be wrapped) - object_schema = { - "type": "object", - "properties": {"test": {"type": "string"}}, - "required": ["test"] - } - unchanged_schema, was_wrapped = accessor._ensure_openai_compatible_schema(object_schema) - assert was_wrapped is False - assert unchanged_schema == object_schema - - -def test_response_unwrapping(): - """Test that wrapped responses are properly unwrapped.""" - # Mock the OpenAI client and response - with patch('src.modelAccessors.openai_accessor.OpenAI') as mock_openai: - accessor = OpenAIAccessor() - mock_client = Mock() - mock_openai.return_value = mock_client - accessor.client = mock_client - - # Create mock response with wrapped data - mock_message = Mock() - mock_message.parsed = {"response": {"type": "implemented", "content": "test"}} - mock_response = Mock() - mock_response.choices = [Mock(message=mock_message)] - mock_client.chat.completions.create.return_value = mock_response - - # Test the call - adapter = cast(TypeAdapter[ModelResponse], TypeAdapter(ClarifierResponse)) - schema = adapter.json_schema() - - result = accessor.call_model( - prompt="test", - adapter=adapter, - schema=schema - ) - - # Verify the response was unwrapped correctly - assert hasattr(result, 'type') - assert result.type == "implemented" - - -def test_json_fallback_unwrapping(): - """Test that JSON fallback also unwraps responses correctly.""" - with patch('src.modelAccessors.openai_accessor.OpenAI') as mock_openai: - accessor = OpenAIAccessor() - mock_client = Mock() - mock_openai.return_value = mock_client - accessor.client = mock_client - - # Create mock response with JSON content (no parsed attribute) - mock_message = Mock() - mock_message.parsed = None - mock_message.content = json.dumps({"response": {"type": "implemented", "content": "test"}}) - mock_response = Mock() - mock_response.choices = [Mock(message=mock_message)] - mock_client.chat.completions.create.return_value = mock_response - - # Test the call - adapter = cast(TypeAdapter[ModelResponse], TypeAdapter(ClarifierResponse)) - schema = adapter.json_schema() - - result = accessor.call_model( - prompt="test", - adapter=adapter, - schema=schema - ) - - # Verify the response was unwrapped correctly - assert hasattr(result, 'type') - assert result.type == "implemented" - - -if __name__ == "__main__": - test_ensure_openai_compatible_schema() - test_response_unwrapping() - test_json_fallback_unwrapping() - print("All tests passed!") \ No newline at end of file