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

[surajthedev]

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).

[justinmclean]

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).

[surajthedev]

Hi @justinmclean, Thank you for your detailed review and valuable feedback. I have addressed all the issues you pointed out: The validation has been updated so it no longer causes the entire test run to abort. The duplicate test has been removed/updated as appropriate. The missing test coverage has also been added, including the compare_structural path and the default behavior when negate is absent. Could you please take another look and let me know if everything is working as expected now? I am extremely sorry for the problems my changes caused. I sincerely apologize for the inconvenience, and I really appreciate you taking the time to review my PR and provide helpful feedback. Thank you!

…

On 28 Jul 2026, at 4:36 AM, Justin Mclean ***@***.***> wrote: justinmclean left a comment (apache/magpie#966) <#966 (comment)> 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>](https://github.com/apache/magpie/blob/main/CONTRIBUTING.md). — Reply to this email directly, view it on GitHub <#966?email_source=notifications&email_token=B56R6U7HZROD4MMX5MGJR2D5G7N7BA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMBZG44DCMRSGAY2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-5097812201>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/B56R6U3GLOZMRBAPMLFJSXL5G7N7BAVCNFSNUABGKJSXA33TNF2G64TZHMYTEMRTHA4TIOJWG45US43TOVSTWNBZHA3DSOJZGEZDDILWAI>. Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS <https://github.com/notifications/mobile/ios/B56R6UYYOJQCVLWQM2RZ2ZT5G7N7BA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMBZG44DCMRSGAY2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG> and Android <https://github.com/notifications/mobile/android/B56R6UZCKGYJA46P4PD62VT5G7N7BA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMBZG44DCMRSGAY2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>. Download it today! You are receiving this because you authored the thread.