lint: add incoherent_exclusive_limits rule

[AcEKaycgR]

Summary

Adds a new lint rule incoherent_exclusive_limits that fires when exclusiveMinimum is greater than or equal to exclusiveMaximum, making the schema unsatisfiable.

Applies to Draft 6, Draft 7, 2019-09, and 2020-12.

Test cases (per dialect)

• Valid: exclusiveMinimum < exclusiveMaximum
• Valid: only exclusiveMinimum present
• Valid: only exclusiveMaximum present
• Invalid: exclusiveMinimum == exclusiveMaximum
• Invalid: exclusiveMinimum > exclusiveMaximum

Changes

• src/alterschema/linter/incoherent_exclusive_limits.h - new rule
• src/alterschema/alterschema.cc - rule registered for all four dialects
• src/alterschema/CMakeLists.txt - header added
• test/alterschema/alterschema_lint_draft6_test.cc - tests added
• test/alterschema/alterschema_lint_draft7_test.cc - tests added
• test/alterschema/alterschema_lint_2019_09_test.cc - tests added
• test/alterschema/alterschema_lint_2020_12_test.cc - tests added

Part of sourcemeta/core#1975.

[augmentcode]

🤖 Augment PR Summary

Summary: This PR introduces a new linter rule, incoherent_exclusive_limits, to detect contradictory numeric-exclusive bounds in JSON Schemas.

Changes:

• Added IncoherentExclusiveLimits rule that triggers when exclusiveMinimum is greater than or equal to exclusiveMaximum
• Registered the rule in the AlterSchema linter bundle so it runs during lint mode
• Updated AlterSchema CMake source list to include the new header
• Added test coverage for Draft 7, 2019-09, and 2020-12 covering coherent, missing-bound, and incoherent-bound cases

Technical Notes: The rule is non-mutating (lint-only) and reports both keywords as implicated when it fires.

_{🤖 Was this summary useful? React with 👍 or 👎}

[augmentcode]

Review completed. 2 suggestions posted.

Comment augment review to trigger a new review at any time.

[cubic-dev-ai]

2 issues found across 6 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[jviotti]

I think this one is a good one to have in the common folder for both the linter and canonicalizer (and testing on both), and having a transform that sets the current schema to false or just adds not: true or whatever to make the unsatisfiability obvious.

I believe we have some rules that do something like this for you to take inspiration from

[AcEKaycgR]

Updated as suggested. The rule is now in src/alterschema/common/ with mutates = std::true_type and a transform that sets the schema to false. It is registered under both the linter and canonicalizer bundles, and canonicalizer tests have been added for all four dialects alongside the existing lint tests.

[jviotti]

Much better, but you will have to guard against $ref's. Consider a schema like this, which is weird, but definitely valid:

{
  "$ref": "#/$defs/test/additionalProperties"
  "$defs": {
    "test": {
      "additionalProperties": {
        "type": "string"
      },
      "exclusiveMaximum": 5,
      "exclusiveMinimum": 8
    }
  }
}

By unconditionally converting $defs/test to false you break the reference. So try to think of potential adversarial tests for how you can break the rule. Also maybe if the schema has a $dynamicAnchor, etc.

If I recall correctly, in other cases, for this reason, what we do is NOT convert the whole thing to false but just add not: {}, or if not is already there, add it into an existing or new allOf

[AcEKaycgR]

Thanks for the detailed feedback. I have updated the implementation to address all your points:

Rule changes (src/alterschema/common/incoherent_exclusive_limits.h):

• Anchor Guards: Added $anchor and $dynamicAnchor checks to the rule's condition() to prevent mutating schemas that define reference targets.
• Non-Destructive Transform: Instead of unconditionally converting the schema to false, the transform now introduces not: {} (which canonicalizes to not: true). If not already exists, it wraps both inside a new allOf. If allOf already exists, it directly appends {not: {}} to the array in-place to avoid any copy-assignment overhead.

Test coverage:

• Draft 2019-09 & Draft 2020-12: Added test cases covering $ref integrity preservation as well as explicit tests for $anchor and $dynamicAnchor guards (validating that the schema remains unmutated when these are present).
• Draft 6 & Draft 7: Anchor-related tests are omitted since $anchor and $dynamicAnchor do not exist in these older dialects. Furthermore, since $ref in Draft 6/7 has exclusive sibling-swallowing behavior under validation anyway, the integration tests successfully verify that reference targets are safely reframed and canonicalized.

All 39 unit tests are passing cleanly and the full alterschema test suite has been verified.

[jviotti]

You need to be careful when referring to not or allOf in the condition. In 2020-12 and 2019-09, those keywords are only valid if the Applicator vocabulary is also in effect!

[jviotti]

Do we really need this check though? Adding a not outside the allOf is still not bad?

[jviotti]

I think you might be overprotecting through. As long as you didn't i.e. turn the entire subschema to false and therefore removed keywords as a byproduct, just letting be while you add not: {} is probably fine and won't break any anchor or reference?

[AcEKaycgR]

Thanks for the follow-up feedback ,You are completely correct, and I have simplified the implementation to address all concerns:

1. Vocabulary Safety & Condition Simplification:
   Removed the not and allOf checks from the condition() method entirely. I had originally added them as an early-termination optimization to check if a schema was already marked as unsatisfiable by a previous transform. However, as you noted, since the transform() method already erases the exclusiveMinimum and exclusiveMaximum keywords, the rule naturally will never apply more than once anyway. Removing these checks completely resolves the vocabulary restriction issue (where these applicator keywords require the Applicator vocabulary to be active in Draft 2019-09 and Draft 2020-12).

2. Anchor Guard Removal:
   Removed the $anchor and $dynamicAnchor guards. These were originally introduced back when we were unconditionally converting the entire schema to false (which would have wiped out the anchors). Now that the transform is non-destructive and only appends the validation constraint structurally (either as not: {} or wrapped under allOf), all $anchor, $dynamicAnchor, $defs, and sibling keywords remain perfectly intact at their original location without breaking any references.

I also cleaned up the test suites by removing the redundant anchor-related tests since the rules are now simpler and rely on standard framework-level anchor preservation. All 36 integration and linter tests are passing successfully.

[jviotti]

Oh, I just realised something. This transformation is not fully semantically correct. The schema is only unsatisfiable if the instance is a number, right? Otherwise the exclusive* ones don't really apply. Can you do another pass on this? Let's try to make sure all transformations keep 100% of the schema semantics in that.

I guess in this case we can turn the not to not: { type: number } or something?

I think we are getting into something here, but as you can see, its very easy to mess up these transformations

[AcEKaycgR]

You're right, Since exclusive limits only apply to numbers, other types should not be affected.
I updated the transform to use not: { type: number } instead of a generic not: {} so the unsatisfiability only targets numbers.
If the schema already has a type that excludes numbers (e.g. type: "object"), the exclusive limits can never apply to any valid instance, so we just erase them without adding a redundant not.
All tests updated and passing cleanly.

[jviotti]

Note that you are unsafely using these keywords for 2019-09 and 2020-12. You have only checked for the validation vocabulary in that case. I think you need to tweak the condition so that this rule only applies on those in BOTH the applicator and validation vocabularies are set

[jviotti]

Can we have a test that covers a type: number + the exclusive impossibility and see how that gets auto fixed? and same with type: integer? I wonder if that will be a funny one. I guess we want to then DELETE the type and move it into the not instead?

[jviotti]

This and other cases means that we put such new property at the end of the object. Which can look weird if there is more schema content than just the exclusive* keywords. I think you need to assign these ones RIGHT AFTER the exclusive ones (we have JSON:: helpers for this, like try_emplace_back I think) when possible. Otherwise while valid, the auto-fixer will give you a weird JSON object with trailing allOf and not's