Fix OpenAI schema validation for discriminated unions#52
Conversation
- 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>
| raise ValueError("No content in response") | ||
|
|
||
| # If we wrapped the schema, parse JSON and unwrap the response | ||
| if is_wrapped: |
There was a problem hiding this comment.
Instead of holding onto the is_wrapped variable can you just have a method that handles the input and output and hides this away from the top level method? Basically is there a way to deterministically determine that the object should be unwrapped and should be wrapped
There was a problem hiding this comment.
Refactored to remove the is_wrapped variable. Created two separate methods: _prepare_schema_for_openai() handles input schema transformation, and _extract_response_from_openai_format() deterministically unwraps responses by checking the original schema structure. This provides cleaner separation of concerns. (5b16c9b)
| @@ -0,0 +1,108 @@ | |||
| import json | |||
There was a problem hiding this comment.
These tests should be in the openai accessor unit test file not in their own
There was a problem hiding this comment.
Moved all schema fix tests from the separate test_openai_accessor_schema_fix.py file into the existing test_openai_accessor.py file. The separate test file has been removed. (5b16c9b)
|
|
||
| # If we wrapped the schema, parse JSON and unwrap the response | ||
| if is_wrapped: | ||
| import json |
There was a problem hiding this comment.
Don't import json at the lower scope, do it at the file level
There was a problem hiding this comment.
Removed the duplicate import json from the lower scope since it was already imported at the file level. (5b16c9b)
|
@copilot resolve the comments left on the review |
- 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>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
🔧 Fixed OpenAI Schema Validation Issue
Problem: OpenAI was rejecting discriminated union schemas with error:
schema must be a JSON Schema of 'type: "object"', got 'type: "None"'Root Cause: Pydantic generates
oneOfschemas for discriminated unions likeClarifierResponse, which lack a root"type": "object"field required by OpenAI's structured output format.Solution:
Key Changes:
_prepare_schema_for_openai()method that wrapsoneOfschemas in an object_extract_response_from_openai_format()for deterministic response unwrappingis_wrappedtracking variable for cleaner codeTesting Results:
✅ Original error scenario now works
✅ CLI no longer shows schema validation error
✅ Mock and Anthropic accessors unaffected
✅ All existing tests pass
✅ Comprehensive test coverage in consolidated file
✅ Cleaner separation of concerns without tracking variables
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.