Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
src/subset.rs
Outdated
| fn integer_multiple_subset(parent: Option<f64>, source: Option<f64>) -> bool { | ||
| let Some(pm) = parent else { | ||
| return true; | ||
| }; | ||
| if let Some(sm) = source { | ||
| let ratio = sm / pm; | ||
| return (ratio.fract()).abs() < f64::EPSILON; | ||
| } | ||
| (pm - 1.0).abs() < f64::EPSILON |
There was a problem hiding this comment.
The integer→number path relies on integer_multiple_subset, but when the integer schema has no multipleOf, this function only returns true if the parent multipleOf is exactly 1.0. Under JSON Schema semantics, any integer is a multiple of 0.5, 0.25, 0.1, etc. (division yields an integer), so {type: integer} should be a subschema of {type: number, multipleOf: 0.5}. As written, is_subschema_of will return false for those cases, causing compatibility checks to flag valid changes as breaking; this is especially visible with fractional multipleOf in the parent.
Useful? React with 👍 / 👎.
| // Handle anchors and fragment identifiers by searching the entire | ||
| // document tree for matching `$id`/`$anchor` pairs. | ||
| if let Some((base, frag)) = path.split_once('#') { | ||
| let anchor = if frag.is_empty() { None } else { Some(frag) }; | ||
| let base_opt = if base.is_empty() { None } else { Some(base) }; |
There was a problem hiding this comment.
resolve_refs_internal only calls find_ref_target when the $ref contains a # fragment, so absolute IDs without fragments (e.g. URN $ref values used in the JSON Schema test suite) skip this lookup and fall through to the BoolSchema(true) fallback below. That means $ref: "urn:uuid:…" is treated as “accept anything” instead of resolving to the schema with that $id, which can silently make breaking changes look compatible. Consider adding a base-only $id lookup across the document tree before falling back.
Useful? React with 👍 / 👎.
1 participant
Start thinking about what it might look like to generate code from the ast we have. Not yet ready for prime time