FIX: stop score aggregators from discarding generator input - #2408
Merged
Roman Lutz (romanlutz) merged 2 commits intoAug 21, 2026
Merged
Conversation
Every aggregator is typed Callable[[Iterable[Score]], ...] but consumed the iterable twice: a validation loop ran first, then list(scores) materialized it. A generator was drained by the validation pass, so list(scores) returned empty and the aggregator fell through to its empty-input branch, silently discarding the scores. Concretely, on the same three scores: FloatScale MAX returned 0.0 instead of 0.9, TrueFalse OR returned False instead of True, and the RAISE_ON_EMPTY variants raised "No scores available for aggregation". The true/false case is a silent false negative on a successful attack. Materialize before validating at all three sites. List inputs are unaffected and type validation is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Roman Lutz (romanlutz)
approved these changes
Aug 21, 2026
Roman Lutz (romanlutz)
left a comment
Contributor
There was a problem hiding this comment.
It's not currently an issue we're facing because this is internal only and we can just make sure there are no generators but I suppose there's no harm in accepting the fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Every score aggregator is typed to accept an
Iterable[Score]:but each one consumes that iterable twice — validating first, materializing second:
Pass a generator and the validation loop drains it,
list(scores)returns[], and the aggregator falls through to its empty-input branch. The scores are silently discarded — no error, no warning.Reproduction on
main, same three scores each time:The true/false case is the one that worries me: a composite scorer that should report True (objective achieved) reports False instead. That is a silent false negative on a successful attack, which is the failure direction you least want in a red-teaming tool. The
RAISE_ON_EMPTYvariants are less dangerous but actively misleading — they raise "No scores available for aggregation" while holding a perfectly good set of scores.Affects three call sites:
true_false_score_aggregator.pyand both aggregator factories infloat_scale_score_aggregator.py.Scope, stated honestly: every in-repo caller currently passes a list or a materialized sequence, so nothing in PyRIT is broken today — this is latent. It matters because these are public, exported API (
pyrit.score.__all__listsTrueFalseScoreAggregator,FloatScaleScoreAggregator,FloatScaleScorerByCategory,FloatScaleScorerAllCategories) and are demonstrated indoc/code/scoring/3_combining_scorers. A user writing a custom scorer that yields scores, or an internal refactor that swaps a list comprehension for a generator expression, hits it immediately and gets a wrong number rather than a traceback.Changes
Materialize before validating, at all three sites:
Three lines reordered per site. Behavior for list/sequence inputs is unchanged, and type validation still rejects the same inputs with the same message — the only difference is that a bad generator is now fully consumed before raising, rather than short-circuiting on the first bad element.
Tests and Documentation
Added to
tests/unit/score/test_true_false_score_aggregator.pyandtests/unit/score/test_float_scale_score_aggregator.py:test_aggregators_accept_generators(both files) — asserts a generator aggregates identically to the equivalent list, acrossOR/ANDandMAX/MIN/AVERAGEplus both category-aware factories.test_raise_on_empty_aggregator_accepts_generators— a non-empty generator must not trip the empty-input guard.test_generator_of_wrong_type_still_raises(both files) — pins that materializing first did not weaken type validation.The three behavioral tests fail on
mainand pass with this change (verified by reverting only the two source files and re-running:3 failed, 42 passed).Verification:
pytest tests/unit/score/test_true_false_score_aggregator.py tests/unit/score/test_float_scale_score_aggregator.py-> 45 passedpytest -n 4 --dist=loadfile tests/unit-> full suite greenpre-commit run --files <changed>-> all hooks pass, includingruffandtyNo documentation changes — this restores the behavior the existing type signature already advertises.