FIX: stop seeded converters from reseeding the global RNG - #2397
FIX: stop seeded converters from reseeding the global RNG#2397Vishnu Rajeev (VishnuR23) wants to merge 2 commits into
Conversation
ZalgoConverter, ProportionSelectionStrategy and WordProportionSelectionStrategy called random.seed() on the process-wide RNG. Passing seed= to any one of them reset global random state on every conversion, so every other component drawing from the `random` module (~17 modules, including CharSwapConverter, RandomCapitalLettersConverter, InsertPunctuationConverter and seed sampling) silently stopped varying. Each of the three now owns a random.Random instance instead. Seeded output is byte-identical to before; only the global side effect is removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| # converter yields the same output on every call. | ||
| if self._seed is not None: | ||
| random.seed(self._seed) | ||
| self._rng.seed(self._seed) |
There was a problem hiding this comment.
This no longer guarantees the converter's documented reproducibility when Zalgo is composed with an unseeded random word selector. WordLevelConverter.convert_async() performs word selection after this reset, but WordProportionSelectionStrategy now owns an independent RNG, so repeated calls to ZalgoConverter(seed=42, word_selection_strategy=WordProportionSelectionStrategy(proportion=0.5)) can select different words and produce different outputs. Please either use one operation-local RNG across selection and mark generation, or explicitly define seeds as component-scoped and update the public contract accordingly. Add a regression test for this composed case.
|
|
||
| async def test_zalgo_seed_does_not_disturb_global_rng(): | ||
| """A seeded converter must not reseed the process-wide RNG.""" | ||
| random.seed(0) |
There was a problem hiding this comment.
This regression test mutates the process-wide RNG and leaves it seeded at 0, which can make later tests order-dependent. The same pattern appears in both new selection-strategy tests. No setup seed is needed here: capture the existing random.getstate(), exercise the component, and compare against that state. Alternatively, restore the original state in a finally block.
…ests Per review on microsoft#2397: - Seeding ZalgoConverter no longer implicitly seeded a randomized word selection strategy, which previously rode on the global seed. Define seeds as component-scoped and document that contract on all three seed params; seed the strategy too for end-to-end reproducibility. Adds a regression test covering both the unseeded (varies) and seeded (repeats) cases. - The new regression tests left the global RNG seeded at 0, making later tests order-dependent. Restore the original state in a finally block. Keep a setup seed distinct from the component's own seed: without it the assertion passes vacuously, since a leaking component that reseeds to the value a previous test used lands back on the captured state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@microsoft-github-policy-service agree |
|
Thanks — both were right, and the second turned out to be more interesting than it looked. Pushed fixes for both. 1. Composed reproducibility ( Confirmed. I took your second option, seeds are component-scoped, rather than threading one operation-local RNG through selection and mark generation. Reasoning: the operation-local route means ZalgoConverter(
seed=42,
word_selection_strategy=WordProportionSelectionStrategy(proportion=0.5, seed=7),
) # -> identical output across callsContract updated on all three Happy to switch to the operation-local RNG if you would rather one seed control the whole pipeline — bigger diff, but I do not mind doing it. 2. Tests mutating global RNG ( Right, and thanks — leaving the global RNG seeded at One wrinkle worth flagging: your first suggestion (drop the setup seed, capture the existing state and compare) makes two of the three tests pass vacuously. Without a distinct setup seed, So I used your Verified both directions:
|
Description
Three components seed Python's process-wide RNG when given a
seed:ZalgoConverter.validate_input—random.seed(self._seed)ProportionSelectionStrategy.select_range(anchor="random") —random.seed(self._seed)WordProportionSelectionStrategy.select_words—random.seed(self._seed)Each then draws from the
randommodule itself. Becausevalidate_input/select_*run on every conversion, a single seeded instance resets global random state repeatedly, and roughly 17 modules underpyrit/draw from that same global RNG —CharSwapConverter,RandomCapitalLettersConverter,InsertPunctuationConverter,EmojiConverter,LeetspeakConverter,UnicodeConfusableConverter,SeedDatasetsampling, and others.The result: seeding one converter for reproducibility silently de-randomizes unrelated converters in the same process. For a red-teaming framework this quietly costs attack diversity — a campaign keeps re-testing the same variations while appearing randomized.
Reproduction on
main— an unrelated converter, alongside a seededZalgoConverter:Fix
Each of the three now owns a
random.Randominstance and reseeds that rather than the global module.random.Random(seed)yields the same sequence asrandom.seed(seed)plus the module-level functions, so seeded output is byte-identical to before — I verified this by capturing outputs for seeds 1/42/123 on both sides of the change and diffing them. Only the global side effect is removed.grep -rn "random\.seed(" pyrit/is now empty.Note this does change one edge case: an unseeded instance no longer inherits a user's global
random.seed(...). That path is what the per-componentseedargument is for, and relying on it is what caused the bug.Tests and Documentation
Three regression tests assert
random.getstate()is unchanged across a seeded call — precise and non-flaky, no reliance on sampling luck:test_zalgo_seed_does_not_disturb_global_rngTestProportionSelectionStrategy::test_select_range_seed_does_not_disturb_global_rngTestWordProportionSelectionStrategy::test_select_words_seed_does_not_disturb_global_rngAll three fail on
mainand pass with the fix (confirmed by reverting only the source changes and re-running:3 failed, 103 passed). Also addedtest_zalgo_seed_is_repeatable_on_same_instanceandtest_zalgo_unseeded_converters_stay_independentto pin both directions of the contract.One existing test needed updating:
test_char_swap_converter_proportion_unchanged_with_iterationspatchedrandom.sampleto control word selection, which worked only because the strategy called the global module. It now patches the strategy's own RNG; the assertion it exists for (selection happens once, not per iteration) is unchanged.Verification:
pytest -n 4 --dist=loadfile tests/unit-> 15130 passed, 121 skippedpytest tests/unit/converter-> 1125 passed, 34 skippedpre-commit run --files <changed>-> all hooks pass, includingruff format,ruff check, andtyNo documentation changes — internal RNG ownership only, no public API or notebook surface affected.