ci(shards): adopt the stable module assignment for both suites (#14111)

[mrveiss]

Thinking Path

Stacked on #14646, which adds the assignment and its tests but wires nothing. This is the half that gives it effect, kept separate because it edits ci.yml next to a note that says "Do not switch it" — that deserves its own review rather than riding along.

The note is not violated. It warns against least_duration, which scatters individual tests across shards and splits modules — the same fixture-sharing hazard that made --dist worksteal worse. A module-level assignment keeps modules whole, which is precisely the property the note exists to protect. The note is rewritten to say what is now true and why the rule still holds.

What Changed

Both shard invocations, backend and slm:

-  --splits 12 --group ${{ matrix.shard }} \
-  --durations-path .test_durations \
+  -p repo_tests.stable_shard \
+  --shard-splits 12 --shard-group ${{ matrix.shard }} \
+  --shard-durations .test_durations \

-n auto --dist loadscope is untouched, deliberately. Sharding composes with it exactly as before: the plugin deselects everything outside the group at collection time, and xdist distributes what remains by scope.

The #13637 exit-5 note is also updated. It said "Fixing the packing is the real repair" — this is that repair, so leaving it would have left a stale claim. The tolerance is kept anyway: it costs nothing, and the condition it guards is a property of the module set rather than the algorithm. A target with fewer modules than shards still empties one, and that must stay announced rather than red.

Verification

Run end to end against real targets, not just reasoned about.

The plugin selects exactly what the offline table predicts. For autobot-backend/a2a, computing the assignment from .test_durations predicts groups 6/7/8/11 with 33/46/53/48 rows. Running the real plugin under real pytest:

group 1 selected: 0      (predicted: not in set)
group 4 selected: 0      (predicted: not in set)
group 7 selected: 46     (predicted: 46)

That is the check that matters, because it proves pytest's node-ID module paths and the durations-file keys are the same shape. Had they diverged, assignment would still have been stable while silently ignoring every weight — balance would degrade with nothing failing.

No empty shards on either real target set:

backend: 1282 modules, 22087 tests, 1.18x spread, empty shards: none
slm:      106 modules,  1501 tests, 1.22x spread, empty shards: none

The slm figure is the direct answer to #13637, which measured group 1 taking 497 of 1644 while groups 8-12 took none.

ci.yml validated as YAML after editing.

Not verified here: wall-clock shard times under the new distribution. Balance is asserted on test count, which is the better proxy than recorded duration but still a proxy — the first real CI run on this PR is the measurement.

Model Used

Opus 5

Closes #14111

[mrveiss]

Review — APPROVE-WITH-NITS (#14646 and #14648 as a pair)

Merge order is #14646 first, then #14648 — the latter is stacked on the former's branch and references repo_tests.stable_shard, which does not exist on base until #14646 lands. Re-verify mergeStateStatus after the first merge retargets the second.

The mechanism is sound and the stability guard is behavioural, not a source-text grep — it builds the table once, computes assignments under two different collected sets, and asserts equality, and asserts the inverse property so it cannot be satisfied by a function returning a constant. The handoff notes record an earlier wrong version of that same test (rebuilt the table from the mutated collection, defeating the point), which is evidence the guard was exercised against a failing case rather than written to pass.

"Stable" is a narrower claim than it reads — please state it precisely

Verified: assignment survives adding a test, adding or removing a module, partial collection, and collection order, because bucket_of hashes the module path string and the table is built from .test_durations rather than the live set.

It does not survive a file rename — inherent to a path hash, arguably fine, but untested and unstated. And it is stable only between durations regenerations: the weekly cron rebuilds .test_durations and the table is deliberately allowed to re-deal. So "shard 7 means the same tests" has roughly a one-week half-life, not forever. Both are fine designs; both should be said out loud, because the value of this change is people reasoning about shard identity across runs.

The empty-shard question: not today, and the tolerance is one-sided

Independently reproduced from the real durations files — backend 1282 modules / 22087 tests, slm 106 / 1501. With ≥12 non-empty buckets the greedy LPT loop necessarily lands the first 12 on distinct shards, so no shard can be empty at these counts.

But the exit-5 tolerance that turns an empty shard into a notice rather than a red run exists only for the slm invocation. The backend invocation has no set +e/exit-5 handling at all. Pre-existing, not introduced here, and safe at 1282 modules — but it means the "announced rather than red" property this change advertises is only actually true for one of the two suites. Worth a tracking issue so a future tree split under 12 modules does not silently reproduce #13637.

The finding I would most want fixed before merge

coverage.yml:10 says the split is "the same twelve-way pytest-split arrangement the PR gate uses" — and coverage.yml:79-80,95-96 still invoke the old flags, untouched by #14648. The moment this merges that comment is false, and "python-suite shard 7" and "coverage shard 7" name different test sets while a comment tells the reader they are the same. That silently reintroduces exactly the cross-workflow confusion #14111 exists to end. Either correct the comment to say the divergence is intentional, or file the coverage migration — but not neither.

Also state the one-time reshuffle

#14648 swaps the algorithm wholesale, so every module's shard membership very likely changes on this merge. One sentence in the body or the ci.yml comment — "shard membership resets here; prior 'shard N is flaky' history no longer applies" — saves a future bisect from chasing a ghost.

Balance, for the record

The 1.18x spread is structural rather than noise: api_endpoint_migrations_test.py alone is 2111 of 22087 tests (9.6%) against an even share of 8.3%, and module-level assignment cannot split it. So the worst shard is bounded below by that one file and grows with it, unboundedly. Nowhere near the 60-minute ceiling today — worth knowing as a durable property, not a defect.

Nits

load_module_weights falls back to {} — equal weighting — on a missing or corrupt durations file with no signal emitted. Given this pair's own philosophy that an empty shard should be announced rather than swallowed, a degraded-balance fallback deserves the same. get_logger is available here.

And the enforcement reality, stated plainly: stable_shard_test.py's assertions are the only place this stability property is ever re-checked, and they run exclusively in python-suite, which is not a required context. A regression in the algorithm would not redden a PR.

[mrveiss]

#14646 is merged (663fcb5c6) and this has been retargeted to Dev_new_gui, so repo_tests/stable_shard.py now exists on base — verified. mergeStateStatus is BLOCKED rather than DIRTY, so no rebase appears needed; that is the review gate plus checks re-running against the new base.

My review is posted above. The one item I would genuinely like closed before this merges, because it goes stale the instant it does:

coverage.yml:10 states the split is "the same twelve-way pytest-split arrangement the PR gate uses", and coverage.yml:79-80,95-96 still invoke the old flags, untouched here. Once this lands, python-suite shard 7 and coverage shard 7 name different test sets while a comment tells the reader they are identical — which is precisely the cross-workflow confusion #14111 was filed to end, reintroduced by the PR that closes it. Correct the comment or file the coverage migration; either is fine, neither is not.

Two smaller ones for the body rather than the code: say that this merge is a one-time full reshuffle of shard membership, so any prior "shard N is flaky" history is void — otherwise a future bisect chases a ghost. And note that the exit-5 empty-shard tolerance exists only for the slm invocation; the backend one has none. Safe at 1282 modules, but the "announced rather than red" property this advertises is currently true for one suite of two.

[mrveiss]

Correction to my previous comment — code spans were dropped by a quoting mistake on my end. The intended references:

• the stale comment is at coverage.yml:10, with the old flags still at coverage.yml:79-80,95-96
• the new module now on base is repo_tests/stable_shard.py
• the checks named are python-suite shard 7 versus coverage shard 7

Nothing else in that comment changed. I built the body with an unquoted shell string and the backticked spans were evaluated rather than passed through; the rest of my reviews use a quoted heredoc, which is why they are intact.

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No hardcoded values detected that have SSOT config equivalents!

[mrveiss]

python-suite shard 9/12 — diagnosis (not yet root-caused)

The shard fails with zero test failures: 2245 passed, 20 skipped, then exit 1.
The failing gate is the sys.modules leak guard:

LEAK: autobot_shared/plugin_sdk/loader_manifest_kind_guard_test.py leaves 8
      sys.modules key(s) installed outside autobot_shared/plugin_sdk/
      — first seen at the test
      autobot-backend/agent_loop/tool_output_spill_test.py::TestNonFatal::test_a_write_failure_keeps_the_full_output
1 file(s) above are NOT on repo_tests/sys_modules_leak_baseline.txt, so this run fails.

Leaked keys: plugin_sdk, .base, .capabilities, .loader, .manifest_contract,
.registry, .hooks, .plugin_manager — all was-genuine -> same-object, i.e. the
real modules aliased under the bare top-level name.

What is established

• tool_output_spill_test.py is the witness, not the leaker. Per _Mutation.escapes_to,
  owner is the file that installed the mutation; "first seen at" is merely the first
  test outside the owner's directory to run while it was still live. This PR's shard
  reshuffle is what put a witness after the owner, which is why it surfaced here.
• The leak is not new. The fixture docstring already records that attribution depends
  on which tests share a shard.
• It is not import-time aliasing. plugin_sdk/__init__.py performs no aliasing, and
  loader.py has no module-level calls — the aliases are only installed from inside
  PluginLoader methods, i.e. during a test body, which the autouse fixture's window
  should cover.
• CI ran the fixture fix. The failing job's head_sha matches the PR head, so this is
  not a stale-merge-ref artifact.

What is not established

Why _fresh_plugin_modules does not remove them. Its root list already includes
plugin_sdk and its before snapshot is per-test, so on the face of it the pop should
fire. I have not identified the gap, and I am explicitly not guessing at a fix.

Not baselining it. Adding the file to sys_modules_leak_baseline.txt would silence a
real leak, and the baseline is documented as something to shrink, not grow.

Holding this PR rather than merging red. python-suite is not a required check, which is
exactly why this needs saying out loud: the PR could be merged green-looking with this
still broken.

[mrveiss]

Root-caused and fixed — closing out my earlier diagnosis

I left a comment saying the leak was localised but not root-caused: the autouse
fixture snapshots before per test and its root list already included plugin_sdk,
so on the face of it the pop should have fired, and I said I would not guess at a
fix. The new docstring supplies the piece I was missing:

whichever test loads a plugin first in the shard installs the aliases, and every
later test — including this one — sees them as pre-existing and leaves them

That is why a before-scoped fix could never work. The aliases are a global,
process-wide mutation made by the loader (#11636), not one this test owns, so
filtering by "what this test added" excludes them by construction — while the guard
still attributes them to this file as the installer. Removing them unconditionally
is the right call, and it is safe precisely because the loader re-creates them on
demand.

The startswith("plugins") → split(".", 1)[0] == "plugins" tightening is a real
fix too: the prefix form would also have swallowed an unrelated top-level module
whose name merely began with those characters.

Verified independently:

• Not baselined — 0 occurrences of loader_manifest_kind_guard in
  sys_modules_leak_baseline.txt on both this branch and base. That was the outcome
  I most wanted to avoid, since baselining would have silenced a real leak.
• 12/12 python-suite shards green, 55/55 checks, 0 failing.

Worth stating because green alone would not have proved it: this leak surfaces only
when a witness test lands in the same shard, so a passing run could have meant
"fixed" or merely "no longer co-located". The fixture diff shows it is the former.