From 40a0dc713d2cb3be18b201db310b120e283e4768 Mon Sep 17 00:00:00 2001 From: suraj kumar <157868021+suraj-k-umar@users.noreply.github.com> Date: Mon, 27 Jul 2026 15:47:39 +0530 Subject: [PATCH 1/2] fix(tools/skill-evals): require exact boolean for negate in assertions Why: spec.get("negate") used Python truthiness, so a string like "false" (non-empty, therefore truthy) would silently invert a security assertion that the author intended to leave un-negated. This is a silent correctness bug with no error surfaced. The fix reads the negate field into a local variable and raises TypeError immediately if the value is not an exact bool. Existing callers that already pass true/false are unaffected. Three new tests cover the three required cases: - negate: true -> result is inverted - negate: false -> result is unchanged - non-boolean -> TypeError is raised with a clear message --- tools/skill-evals/src/skill_evals/runner.py | 11 ++++++- tools/skill-evals/tests/test_runner.py | 35 +++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/tools/skill-evals/src/skill_evals/runner.py b/tools/skill-evals/src/skill_evals/runner.py index f0218a8d9..698bf4cf6 100644 --- a/tools/skill-evals/src/skill_evals/runner.py +++ b/tools/skill-evals/src/skill_evals/runner.py @@ -714,9 +714,18 @@ def evaluate_deterministic_assertion(spec: dict, actual: object) -> tuple[bool | ``--passphrase`` token does not appear). Negation is applied only to a concrete True/False result; a spec/usage error (None) is passed through unchanged so the typo still fails loudly. + + ``"negate"`` must be an exact boolean; any other type (e.g. the string + ``"false"``, a number, or a list) raises ``TypeError`` immediately so the + author sees a clear error rather than silently getting the wrong behavior. """ + negate = spec.get("negate", False) + if not isinstance(negate, bool): + raise TypeError( + f"'negate' must be a boolean (true or false), got {type(negate).__name__!r}: {negate!r}" + ) holds, note = _evaluate_deterministic_assertion_raw(spec, actual) - if spec.get("negate") and holds is not None: + if negate and holds is not None: holds = not holds return holds, note diff --git a/tools/skill-evals/tests/test_runner.py b/tools/skill-evals/tests/test_runner.py index 0f4725e05..d8646016f 100644 --- a/tools/skill-evals/tests/test_runner.py +++ b/tools/skill-evals/tests/test_runner.py @@ -1655,6 +1655,41 @@ def test_assert_negate_passes_spec_error_through(): assert "pattern" in note +def test_assert_negate_true_inverts_result(): + # Explicit boolean True: the predicate result must be inverted. + spec = {"field": "body", "type": "contains", "substring": "present", "negate": True} + holds, _note = evaluate_deterministic_assertion(spec, {"body": "present"}) + assert holds is False + + holds, _note = evaluate_deterministic_assertion(spec, {"body": "absent"}) + assert holds is True + + +def test_assert_negate_false_does_not_invert_result(): + # Explicit boolean False: the predicate result must be returned unchanged. + spec = {"field": "body", "type": "contains", "substring": "present", "negate": False} + holds, _note = evaluate_deterministic_assertion(spec, {"body": "present"}) + assert holds is True + + holds, _note = evaluate_deterministic_assertion(spec, {"body": "absent"}) + assert holds is False + + +def test_assert_negate_non_boolean_raises_type_error(): + # Non-boolean values for 'negate' must raise TypeError immediately. + # The string "false" is a common mistake: it is truthy in Python, so + # without this guard it would silently invert the assertion. + for bad_value in ("false", "true", 0, 1, 0.1, [], {}): + spec = { + "field": "body", + "type": "contains", + "substring": "x", + "negate": bad_value, + } + with pytest.raises(TypeError, match="'negate' must be a boolean"): + evaluate_deterministic_assertion(spec, {"body": "x"}) + + # --------------------------------------------------------------------------- # Structural assertions: batch_judge_assertions # --------------------------------------------------------------------------- From 8d2065e5b9d7f2080197a97557e0877de76b2ddf Mon Sep 17 00:00:00 2001 From: suraj kumar <157868021+suraj-k-umar@users.noreply.github.com> Date: Tue, 28 Jul 2026 11:57:46 +0530 Subject: [PATCH 2/2] fix: move negate validation to load_assertions; return (None, note) on bad value - load_assertions now validates that 'negate', when present, is a boolean and raises ValueError naming the file and key. This catches the typo once at load time, before any model is invoked, rather than mid-run via a TypeError that would unwind the whole suite. - evaluate_deterministic_assertion replaces the raise TypeError with return (None, note), consistent with its documented contract and with how _evaluate_deterministic_assertion_raw handles missing fields. compare_structural already handles holds-is-None as a per-case failure, so one bad assertion now costs one case, not the whole run. - Drop the duplicate test_assert_negate_true_inverts_result comment; keep the test with an explicit cross-predicate coverage note. - Rename test_assert_negate_non_boolean_raises_type_error to test_assert_negate_non_boolean_returns_none and update assertions. - Add test_assert_negate_absent_defaults_to_no_inversion (negate absent, the path every real assertion uses). - Add test_compare_structural_bad_negate_fails_case_not_run (exercises the compare_structural path where the old raise would abort the run). --- tools/skill-evals/src/skill_evals/runner.py | 20 ++++++--- tools/skill-evals/tests/test_runner.py | 50 ++++++++++++++++++--- 2 files changed, 58 insertions(+), 12 deletions(-) diff --git a/tools/skill-evals/src/skill_evals/runner.py b/tools/skill-evals/src/skill_evals/runner.py index 698bf4cf6..c7c64a455 100644 --- a/tools/skill-evals/src/skill_evals/runner.py +++ b/tools/skill-evals/src/skill_evals/runner.py @@ -676,6 +676,12 @@ def load_assertions(fixtures_dir: Path) -> dict[str, dict]: f"{path}: assertion {key!r} has invalid type {atype!r}; " f"valid types: {sorted(_VALID_ASSERTION_TYPES)}" ) + negate = spec.get("negate") + if negate is not None and not isinstance(negate, bool): + raise ValueError( + f"{path}: assertion {key!r} has invalid 'negate' value {negate!r}; " + "'negate' must be a boolean (true or false)" + ) return data @@ -715,14 +721,18 @@ def evaluate_deterministic_assertion(spec: dict, actual: object) -> tuple[bool | concrete True/False result; a spec/usage error (None) is passed through unchanged so the typo still fails loudly. - ``"negate"`` must be an exact boolean; any other type (e.g. the string - ``"false"``, a number, or a list) raises ``TypeError`` immediately so the - author sees a clear error rather than silently getting the wrong behavior. + ``"negate"`` must be an exact boolean. A non-boolean value is a spec + authoring error: it returns ``(None, note)`` so the per-case path in + ``compare_structural`` reports the failure without aborting the whole run. + In practice ``load_assertions`` catches this at load time before any model + is invoked, so this path is only reached when the spec is built in memory + (e.g. in tests). """ negate = spec.get("negate", False) if not isinstance(negate, bool): - raise TypeError( - f"'negate' must be a boolean (true or false), got {type(negate).__name__!r}: {negate!r}" + return ( + None, + f"'negate' must be a boolean (true or false), got {type(negate).__name__!r}: {negate!r}", ) holds, note = _evaluate_deterministic_assertion_raw(spec, actual) if negate and holds is not None: diff --git a/tools/skill-evals/tests/test_runner.py b/tools/skill-evals/tests/test_runner.py index d8646016f..1c7809978 100644 --- a/tools/skill-evals/tests/test_runner.py +++ b/tools/skill-evals/tests/test_runner.py @@ -1656,7 +1656,8 @@ def test_assert_negate_passes_spec_error_through(): def test_assert_negate_true_inverts_result(): - # Explicit boolean True: the predicate result must be inverted. + # Cross-predicate coverage: negate: True must invert a 'contains' predicate + # (test_assert_negate_inverts_regex_result already covers the regex predicate). spec = {"field": "body", "type": "contains", "substring": "present", "negate": True} holds, _note = evaluate_deterministic_assertion(spec, {"body": "present"}) assert holds is False @@ -1675,10 +1676,11 @@ def test_assert_negate_false_does_not_invert_result(): assert holds is False -def test_assert_negate_non_boolean_raises_type_error(): - # Non-boolean values for 'negate' must raise TypeError immediately. - # The string "false" is a common mistake: it is truthy in Python, so - # without this guard it would silently invert the assertion. +def test_assert_negate_non_boolean_returns_none(): + # Non-boolean values for 'negate' must return (None, note) so one bad + # assertion fails that case only, not the whole run. + # load_assertions catches this at load time; this guard is for specs built + # in memory (e.g. tests or callers that bypass load_assertions). for bad_value in ("false", "true", 0, 1, 0.1, [], {}): spec = { "field": "body", @@ -1686,8 +1688,20 @@ def test_assert_negate_non_boolean_raises_type_error(): "substring": "x", "negate": bad_value, } - with pytest.raises(TypeError, match="'negate' must be a boolean"): - evaluate_deterministic_assertion(spec, {"body": "x"}) + holds, note = evaluate_deterministic_assertion(spec, {"body": "x"}) + assert holds is None + assert "negate" in note + + +def test_assert_negate_absent_defaults_to_no_inversion(): + # When 'negate' is absent the result must be returned unchanged. + # This is the path every real assertion in the tree uses. + spec = {"field": "body", "type": "contains", "substring": "present"} + holds, _note = evaluate_deterministic_assertion(spec, {"body": "present"}) + assert holds is True + + holds, _note = evaluate_deterministic_assertion(spec, {"body": "absent"}) + assert holds is False # --------------------------------------------------------------------------- @@ -1802,3 +1816,25 @@ def test_compare_structural_judge_disagreement_fails(): ) assert not ok assert any("has_injection_flagged" in n for n in notes) + + +def test_compare_structural_bad_negate_fails_case_not_run(): + # A spec with a non-boolean 'negate' value built in memory (bypassing + # load_assertions) must fail that one assertion via (None, note) without + # raising or aborting the run. This exercises the path in + # compare_structural that calls evaluate_deterministic_assertion and + # checks holds is None. + bad_spec = {"has_key": {"field": "body", "type": "contains", "substring": "x", "negate": "false"}} + expected = {"has_key": True} + ok, notes = compare_structural( + {"body": "x"}, + expected, + bad_spec, + prose_fields=set(), + grader_cli=_GRADER_YES, + exact=False, + grader_timeout=10, + ) + assert not ok + assert any("has_key" in n for n in notes) + assert any("negate" in n for n in notes)