Skip to content

lint: add incoherent_exclusive_limits rule#818

Open
AcEKaycgR wants to merge 1 commit into
sourcemeta:mainfrom
AcEKaycgR:feat/linter-incoherent_exclusive_limits
Open

lint: add incoherent_exclusive_limits rule#818
AcEKaycgR wants to merge 1 commit into
sourcemeta:mainfrom
AcEKaycgR:feat/linter-incoherent_exclusive_limits

Conversation

@AcEKaycgR
Copy link
Copy Markdown
Contributor

@AcEKaycgR AcEKaycgR commented May 19, 2026

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
Copy link
Copy Markdown

augmentcode Bot commented May 19, 2026

🤖 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 👎

Copy link
Copy Markdown

@augmentcode augmentcode Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 2 suggestions posted.

Fix All in Augment

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

Comment thread src/alterschema/linter/incoherent_exclusive_limits.h Outdated
Comment thread src/alterschema/common/incoherent_exclusive_limits.h
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 6 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread test/alterschema/alterschema_lint_2019_09_test.cc
Comment thread test/alterschema/alterschema_lint_draft7_test.cc
@AcEKaycgR AcEKaycgR force-pushed the feat/linter-incoherent_exclusive_limits branch from efb534c to bffc321 Compare May 19, 2026 04:19
Copy link
Copy Markdown
Member

@jviotti jviotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 AcEKaycgR force-pushed the feat/linter-incoherent_exclusive_limits branch 2 times, most recently from ed368af to d8625e0 Compare May 22, 2026 14:37
@AcEKaycgR
Copy link
Copy Markdown
Contributor Author

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.

}

auto transform(JSON &schema, const Result &) const -> void override {
schema.into(JSON{false});
Copy link
Copy Markdown
Member

@jviotti jviotti May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 AcEKaycgR force-pushed the feat/linter-incoherent_exclusive_limits branch from d8625e0 to edb10ef Compare May 25, 2026 17:37
Signed-off-by: AcE <kintan0108@gmail.com>
@AcEKaycgR AcEKaycgR force-pushed the feat/linter-incoherent_exclusive_limits branch from edb10ef to 8de48ab Compare May 25, 2026 17:52
@AcEKaycgR
Copy link
Copy Markdown
Contributor Author

AcEKaycgR commented May 25, 2026

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants