Conversation
|
All contributors have signed the CLA ✍️ ✅ |
c7c4fa6 to
b01ea63
Compare
| } | ||
| } | ||
|
|
||
| fn json_schema_to_json(schema: &JsonSchema) -> JsonValue { |
There was a problem hiding this comment.
Why do we need custom parse/serialize methods? Can't JSON schema be represented in Rust types for default serializer/deserializer still work?
| panic!("expected function tool"); | ||
| }; | ||
|
|
||
| assert!(description.contains("mcp__sample__fn(args: { open?: Array<{")); |
There was a problem hiding this comment.
can we use the full multiline string in contains assertion?
| } | ||
|
|
||
| #[test] | ||
| fn parse_tool_input_schema_preserves_web_run_shape() { |
There was a problem hiding this comment.
Can this test be reduced to only representative parts?
There was a problem hiding this comment.
As in, specific types or patterns instead of just web_run
| properties: BTreeMap::from([( | ||
| "value".to_string(), | ||
| JsonSchema::String { description: None }, | ||
| JsonSchema::AnyOf { |
There was a problem hiding this comment.
Does this type anyOf definition work with response api?
There was a problem hiding this comment.
yes, anyOf and Enum are supported: https://developers.openai.com/api/docs/guides/structured-outputs#supported-schemas
i will remove allOf, oneOf, and Constant though
pakrym-oai
left a comment
There was a problem hiding this comment.
2 major points:
- Does the newly extended JSON schema format work with Responses API https://developers.openai.com/api/docs/guides/structured-outputs
- Can we drop custom serializer/deserializer.
578a2a5 to
6995d8a
Compare
|
I have read the CLA Document and I hereby sign the CLA |
bc2f102 to
d6a7b8d
Compare
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b837d9ae0b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
9fbcb45 to
fe13088
Compare
|
@codex review |
fe13088 to
1bd6e39
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe13088cc6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| fn normalize_schema_type_name(schema_type: &str) -> Option<&'static str> { | ||
| match schema_type { |
There was a problem hiding this comment.
nit: this can be simpler
| sanitize_json_schema(value); | ||
| } | ||
|
|
||
| if let Some(const_value) = map.remove("const") { |
There was a problem hiding this comment.
is this new logic? do we need a test?
There was a problem hiding this comment.
added parse_tool_input_schema_rewrites_const_to_single_value_enum
| break; | ||
| } | ||
| } | ||
| if matches!(schema_type.as_deref(), Some("enum") | Some("const")) { |
There was a problem hiding this comment.
why would the type be enum or const? I don't think the spec supports it.
| map.remove("type"); | ||
| } | ||
|
|
||
| if let Some(types) = map.get("type").and_then(JsonValue::as_array).cloned() { |
There was a problem hiding this comment.
we are interchangeably using map.get("type") and schema_type around this method.
can we normalize and clean up a bit?
| { | ||
| schema_type = Some("string".to_string()); | ||
| } else if map.contains_key("enum") || map.contains_key("format") { | ||
| schema_type = infer_enum_type(map.get("enum").and_then(JsonValue::as_array)) |
There was a problem hiding this comment.
do we need this extra logic? do we have tests?
| map.insert("type".to_string(), JsonValue::Array(normalized)); | ||
| } | ||
| } | ||
| } else if let Some(schema_type) = map.get("type").and_then(JsonValue::as_str) |
There was a problem hiding this comment.
can we always read type into the array (and turn singular type into a single item vec) for processing
That will avoid special casing multiple/singular cases everywhere.
|
|
||
| pub fn enumeration(values: Vec<JsonValue>, description: Option<String>) -> Self { | ||
| Self { | ||
| schema_type: infer_enum_type(Some(&values)).map(JsonSchemaType::Single), |
There was a problem hiding this comment.
why can't the caller provide the type?
1bd6e39 to
a6e0b76
Compare
Support enum and anyOf in tool JsonSchema Trim json schema parser coverage to representative cases Simplify code mode schema rendering assertion Stabilize JsonSchema follow-up fixes Annotate JsonSchema helper call arguments Union
| BTreeMap::from([("page".to_string(), JsonSchema::number(/*description*/ None),)]), | ||
| BTreeMap::from([( | ||
| "page".to_string(), | ||
| JsonSchema::integer(/*description*/ None), |
There was a problem hiding this comment.
from https://developers.openai.com/api/docs/guides/structured-outputs#supported-schemas, integer and number are both supported
1c312c4 to
1873682
Compare
Restore JsonSchema test comments Unignore passing tool schema tests Update MCP integer schema expectation Preserve defaults for nullable schema unions
1873682 to
f2f1d03
Compare
| } | ||
|
|
||
| if schema_type == "array" && !map.contains_key("items") { | ||
| if matches!(schema_types.as_slice(), [JsonSchemaPrimitiveType::Array]) |
There was a problem hiding this comment.
looks like this is handled in ensure_default_children_for_schema_types
| sanitize_json_schema(&mut input_schema); | ||
| serde_json::from_value::<JsonSchema>(input_schema) | ||
| let schema: JsonSchema = serde_json::from_value(input_schema)?; | ||
| if matches!( |
There was a problem hiding this comment.
nit: this is pretty specific, is it important to have the parser validate? we don't do it for other issues.
There was a problem hiding this comment.
I added this specific case because the structured output supported subset of the json schema only accepts a null type when it's in a list with another valid type (previous JsonSchema didn't have null as a type so would throw a deserialization error on the {"type": "null"} case as well...)
(and as a result this existing test expects an error)
This brings us into better alignment with the JSON schema subset that is supported in [https://developers.openai.com/api/docs/guides/structured-outputs#supported-schemas], and also allows us to render richer function signatures in code mode (e.g., anyOf{null, OtherObjectType})