Skip to content

fix(checks): Not must not invert unevaluable failures - #2658

Closed
davidberenstein1957 wants to merge 3 commits into
mainfrom
fix/not-unevaluable-invert
Closed

fix(checks): Not must not invert unevaluable failures#2658
davidberenstein1957 wants to merge 3 commits into
mainfrom
fix/not-unevaluable-invert

Conversation

@davidberenstein1957

Copy link
Copy Markdown
Member

Summary

  • Adds CheckResult.evaluable (default True). Structural “could not evaluate” failures from built-ins set evaluable=False.
  • Not passes unevaluable results through without inverting them, so a missing key or match= type mismatch no longer becomes a green suite (Not() turns a check that could not be evaluated into a pass #2637).
  • Status stays FAIL for those cases — additive field only, no FAIL→ERROR migration.

Test plan

  • uv run pytest libs/giskard-checks/tests/builtin/test_composition.py::TestNot (10 passed)
  • make test-unit PACKAGE=giskard-checks (760 passed, 4 skipped)
  • ruff check / ruff format --check on changed Python sources
  • basedpyright skipped locally (pre-existing reportInvalidTypeForm on comparison fields; CI still runs it)

Closes #2637

Structural miss cases (missing key, type mismatch, unsupported
comparison) now set CheckResult.evaluable=False. Not passes those
through unchanged so Not(Equals(missing_key)) stays fail instead of
silently becoming pass (#2637).
Keep main's updated Checks/Scan quickstart examples; preserve the
evaluable=False / Not passthrough changes from the feature branch.

Co-authored-by: Kevin Messiaen <kevinmessiaen@users.noreply.github.com>
@kevinmessiaen

Copy link
Copy Markdown
Member

I would not add a new evaluable boolean flag to the check results. It'll probably be cleaner to treat those as check failures.

  • missing key: it's should be a failure, the data is not present. A simple check to validate that data is present beforehand would "silent" the error
  • mismatching comparaison type: it is a python error, so propagating an error make sense

@cursor

cursor Bot commented Aug 3, 2026

Copy link
Copy Markdown

Superseded by a different approach: structural unevaluable outcomes now return ERROR (no evaluable field) on a fresh branch from main — see the replacement PR linked from #2637 / branch cursor/not-structural-error-8f54. Please close this PR in favor of that one.

kevinmessiaen added a commit that referenced this pull request Aug 4, 2026
…-green) (#2693)

fix(checks): treat structural unevaluable outcomes as ERROR

Missing keys, type mismatches for configured modes, unsupported
comparisons, and similar preconditions now return CheckResult.error
instead of failure so Not cannot invert them into a silent pass
(#2637). Assertion-false paths stay FAIL. Supersedes the evaluable
field approach on #2658.

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Kevin Messiaen <kevinmessiaen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Not() turns a check that could not be evaluated into a pass

3 participants