Skip to content

Fix wrong group orders caused by leaked randomized stabilizer chain options in CompatiblePairs - #6491

Merged
fingolfin merged 1 commit into
masterfrom
mh/fix-stabchain-random-leak
Aug 5, 2026
Merged

Fix wrong group orders caused by leaked randomized stabilizer chain options in CompatiblePairs#6491
fingolfin merged 1 commit into
masterfrom
mh/fix-stabchain-random-leak

Conversation

@fingolfin

Copy link
Copy Markdown
Member

Group orders computed inside the group returned by CompatiblePairs could come out wrong, which made computations either return wrong results or die with a confusing no method found error.

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:

opt := rec(limit := s, random := 1);
...
u := DoClosurePrmGp(u, [i], opt);
...
until Size(u) = s;
agp := u;

That is sound in itself: ClosureRandomPermGroup accepts a chain without verification only while its size has not reached options.limit (lib/stbcrand.gi:1631), and the loop stops only once the known order s is reached, so the final chain is correct.

But DoClosurePrmGp records the requested randomness on the result (SetStabChainOptions(C, rec(random := options.random)), lib/grpperm.gi:805), and u is then handed on as a factor of the direct product used by CompatiblePairs. From there CopyOptionsDefaults (lib/grpperm.gi:744) passes random := 1 to every stabilizer chain computed inside that group later on — where no limit is known, so nothing validates the result.

Reproducer (plain GAP, no packages):

G := PcGroupCode(379875911272818017220001991, 4356);;
D := DirectProduct(AutomorphismGroup(G), GL(1,11));;

Reset(GlobalMersenneTwister, 4);; Reset(GlobalRandomSource, 4);;
P := DirectProductInfo(EXPermutationActionPairs(D).permgroup).groups[1];;

StabChainOptions(P);                             # rec( random := 1 )
AbelianInvariants(P);                            # [ 2, 2, 4, 5 ]     <- wrong
AbelianInvariants(Group(GeneratorsOfGroup(P)));  # [ 2, 2, 2, 3, 5 ]  <- correct

Depending on the state of the random source the same computation instead runs into

Error, no 1st choice method found for `Factors' on 2 arguments
 [1] Factors( Integers, r ) @ lib/grp.gi:949

which is the generic AbelianInvariants method computing r := Size(G)/Size(H) for a subgroup H obtained by ClosureSubgroupNC and 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:1059 and lib/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 reaches CompatiblePairs via NonSplitExtensions; 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.

…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>
@fingolfin fingolfin added release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them topic: library labels Aug 5, 2026
@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.06%. Comparing base (c0ec89a) to head (b30b76a).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hulpke hulpke left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice. That was a nasty one.

@fingolfin
fingolfin merged commit 6ef2818 into master Aug 5, 2026
33 checks passed
@fingolfin
fingolfin deleted the mh/fix-stabchain-random-leak branch August 5, 2026 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-to-4.16 kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants