Skip to content

fix(tools/skill-evals): require exact boolean for negate in assertions - #966

Open
surajthedev wants to merge 1 commit into
apache:mainfrom
surajthedev:fix/negate-must-be-strict-boolean
Open

fix(tools/skill-evals): require exact boolean for negate in assertions#966
surajthedev wants to merge 1 commit into
apache:mainfrom
surajthedev:fix/negate-must-be-strict-boolean

Conversation

@surajthedev

Copy link
Copy Markdown

Closes #942

tools/skill-evals/src/skill_evals/runner.py read the negate assertion key using Python truthiness, so a value like "false" (a non-empty string, therefore truthy) would silently invert a security assertion the author intended to leave un-negated.

This PR requires negate to be an exact boolean. Any non-boolean value now raises a clear TypeError instead of silently producing the wrong result.

Added three tests covering negate: true, negate: false, and non-boolean values (string, int, float, list, dict).

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
@justinmclean

justinmclean commented Jul 27, 2026

Copy link
Copy Markdown
Member

Good catch on the bug. The bad-value loop is well chosen: [] and {} are falsy, so the old check swallowed them silently.

Two notes, however. The rejection mechanism turns a one-assertion failure into a whole-run abort, which I'd change before this lands. And test_assert_negate_true_inverts_result duplicates an existing test six lines above it.

runner.py, the raise TypeError in evaluate_deterministic_assertion

This breaks the function's own contract. Its first docstring paragraph still says a spec error returns None, and the only caller, in compare_structural, is written against that:

holds, note = evaluate_deterministic_assertion(spec, actual)
if holds is None:
    ok = False
    notes.append(f"{key}: {note}")

compare_structural is called from the per-case loop in main, and neither that call nor the loop around it sits in a try/except — the only ones nearby wrap prompt formatting and the CLI subprocess. So a TypeError unwinds the whole run. _evaluate_deterministic_assertion_raw handles the same class of authoring mistake, a missing field, by returning (None, note) and failing that one assertion. As written, one typo in one assertions.json costs the entire suite rather than one case.

Two ways to reject the bad value without that: validate in load_assertions, which already rejects bad shapes in this same file with ValueError and would fail once at load naming the file and key; or return (None, "negate must be a boolean, got 'false'") and let the existing path report it. I'd take the first, since it catches the typo before any model is invoked.

tests/test_runner.py, test_assert_negate_true_inverts_result

This duplicates test_assert_negate_inverts_regex_result six lines above, which already covers negate: True inverting a predicate. Only the predicate type differs. Worth dropping, or adding a comment saying the point is cross-predicate coverage if that's the intent.

Two things still uncovered: nothing goes through compare_structural, which is where the raise-vs-return question above actually bites, and nothing covers negate being absent — the path every assertion in the tree uses, and the one this refactor touched (spec.get("negate")spec.get("negate", False)).

This review was drafted by an AI-assisted tool and
confirmed by a Magpie maintainer. The findings
below are observations, not blockers; a Magpie
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.

More on how Magpie handles maintainer review:
[CONTRIBUTING.md](https://github.com/apache/magpie/blob/main/CONTRIBUTING.md).

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.

Treat the negate key as a strict boolean in the skill-evals runner

3 participants