Fix wrong group orders caused by leaked randomized stabilizer chain options in CompatiblePairs - #6491
Merged
Merged
Conversation
…ePairs `EXPermutationActionPairs' rebuilds the first factor of the direct product it is handed from a subset of its generators, using `random := 1' together with the known group order as a limit. That is sound in itself, as the closure is accepted only once its stabilizer chain reaches that order. But `DoClosurePrmGp' records the requested randomness on the resulting group, and from there it is inherited by every stabilizer chain computed inside that group later on -- where no limit is known, so nothing validates the result. Group orders computed in it can then be silently wrong. Via `SmallGeneratingSet' and `AbelianInvariants', `CompatiblePairs' either returned a wrong answer or ran into a `no method found' error for `Factors'. Clear the marker once the chain has reached the known order, as is already done elsewhere in the library after a deliberately randomized computation. AI disclosure: this change was prepared with the help of Claude Code (Claude Opus 5), which reduced the reproducer, diagnosed the cause and drafted the patch and the test; reviewed by me. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6491 +/- ##
==========================================
+ Coverage 79.03% 79.06% +0.02%
==========================================
Files 685 685
Lines 293790 293791 +1
Branches 8664 8664
==========================================
+ Hits 232203 232274 +71
+ Misses 59786 59716 -70
Partials 1801 1801 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
hulpke
approved these changes
Aug 5, 2026
hulpke
left a comment
Contributor
There was a problem hiding this comment.
Very nice. That was a nasty one.
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.
Group orders computed inside the group returned by
CompatiblePairscould come out wrong, which made computations either return wrong results or die with a confusingno method founderror.EXPermutationActionPairsrebuilds the first factor of the direct product it is handed from a subset of its generators, usingrandom := 1together with the known group order as a limit:That is sound in itself:
ClosureRandomPermGroupaccepts a chain without verification only while its size has not reachedoptions.limit(lib/stbcrand.gi:1631), and the loop stops only once the known ordersis reached, so the final chain is correct.But
DoClosurePrmGprecords the requested randomness on the result (SetStabChainOptions(C, rec(random := options.random)),lib/grpperm.gi:805), anduis then handed on as a factor of the direct product used byCompatiblePairs. From thereCopyOptionsDefaults(lib/grpperm.gi:744) passesrandom := 1to every stabilizer chain computed inside that group later on — where no limit is known, so nothing validates the result.Reproducer (plain GAP, no packages):
Depending on the state of the random source the same computation instead runs into
which is the generic
AbelianInvariantsmethod computingr := Size(G)/Size(H)for a subgroupHobtained byClosureSubgroupNCand getting a non-integer because the closure reports an order that is too small.The fix clears the marker once the chain has reached the known order. That is what the library already does elsewhere after a deliberately randomized computation, see
lib/grpperm.gi:1035,lib/grpperm.gi:1059andlib/factgrp.gi:1184.The issue was introduced in 2eadf1a and is present in all releases since GAP 4.9.0. It surfaced through
ConstructAllSolvableNonNilpotentGroups(47916)in GrpConst, which reachesCompatiblePairsviaNonSplitExtensions; with the fix that computation runs to completion.AI disclosure: this change was prepared with the help of Claude Code (Claude Opus 5), which reduced the reproducer, diagnosed the cause and drafted the patch and the test; reviewed by me.