fix: 10 confirmed defects across 7 vendored minqlxtended plugins - #200
Merged
Conversation
added 3 commits
August 27, 2026 08:19
Ports the map-author-credit opt-in from the minqlx branding.py so both runtimes behave consistently: map credit is hidden by default and only shown when qlx_brandingMapCredit is set to 1.
Fixes every finding that survived assessment in the review recorded at
docs/findings/2026-08-27-minqlxtended-upstream-plugin-review.md, where all 32
upstream-vendored plugins were reviewed against the minqlx-plugin-review
checklist and each finding independently re-verified against source.
vpnblock.py (high) - _update_cache runs under @minqlxtended.thread and called
self.msg() directly at four sites, an engine call off the game thread, on every
load, every ~12h refresh, and all three error paths. Routed through a
_msg_next_frame helper that builds the string on the worker and emits in a frame.
balance.py - four fixes:
* roster callbacks re-called add_request from inside a next_frame handler on
any mid-fetch roster change, so a shuffle landing mid-fetch could chase its
own tail. Bounded by MAX_REFETCH; on giving up it answers with cached data,
which self.rating() already degrades safely.
* !elo/!ratings/!teams/!balance each fan out to the ratings API with no gate.
Straight after handle_new_game clears self.ratings, several players asking
at once each spawned a separate thread against the same endpoint. Added a
server-wide COMMAND_COOLDOWN, since the fetch they trigger is shared.
* nine sites read self.game.type_short/state unguarded while handle_new_game
in the same file guards the identical hazard; a command typed during a map
change raised AttributeError. Guarded to match.
* execute_suggestion's @delay(1) stat restore never re-resolved its players,
so a disconnect in that window threw and skipped the other player's restore
too. Each player now restores independently behind its own guard.
ban.py - ban ids came from a zcard read outside the pipeline, so two admins
banning one player concurrently both read the same count and the second write
overwrote the first while both reported success. Now an atomic INCR, seeded
from the existing count via SETNX so ids on existing installs are preserved.
dictionary.py - !define is permission=0 and forked an HTTP worker per call with
no gate. Added a per-player cooldown; the tracking map self-prunes.
infectedmm.py - the delayed warmup callback could fire after g_rrInfected was
toggled off or the plugin unloaded. Re-checks gametype and _loaded first.
leaverban.py - handle_game_start counted bots into players_start, inflating
games_completed in Redis with no matching games_left. Filtered to match the
rest of the file.
aliases.py - cmd_setnoaliases fell through with a bare return on a failed
identifier resolve where cmd_alias returns Return.STOP_ALL.
Also:
* regenerates manifest.json, which additionally corrects branding.py's hash.
It was left stale by 5e614d4 and test_every_manifest_hash_matches_the_file
_on_disk was failing on main as a result.
* syncs the seven files into default-minqlxtended/scripts/, which were
verified identical to the baseline beforehand. No test pins upstream preset
copies to the baseline, only the six QLSM ports.
* records the eight locally-patched upstream files in the plugin README. The
manifest cannot carry this: origin marks lineage, and the baseline test
requires all 33 upstream names stay "upstream".
Tests: test_minqlxtended_plugin_baseline, test_default_minqlxtended_preset,
test_plugin_manifests, test_plugin_compat, test_preset_compat,
test_builtin_presets_migration, test_runtime_default_preset_seeding - 151 passed.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e9bba0e3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+115
to
+117
| next_id_key = f"{base_key}:next_id" | ||
| self.db.setnx(next_id_key, self.db.zcard(base_key)) | ||
| ban_id = self.db.incr(next_id_key) - 1 |
There was a problem hiding this comment.
When player_info is enabled with qlx_pinfo_ban_deactivated=1, or the preset's kickban plugin is enabled, those plugins still append to the same minqlx:players:<id>:bans sorted set using zcard (player_info.py:397-403, kickban.py:157-176) without advancing :next_id. After a manual ban creates the counter, an automatic ban can take the next zcard ID; the following !ban then gets that same ID from INCR and overwrites the automatic ban's zset member and hash. Make every writer of this ban format use the same atomic allocator.
Useful? React with 👍 / 👎.
Comment on lines
+69
to
+70
| if (not self.is_infected_mastermind_gametype) or (not self._loaded): | ||
| return |
There was a problem hiding this comment.
When the plugin is unloaded after this delayed callback is scheduled but before it runs while Infected Mastermind remains active, this guard still passes: _loaded is set by _add_hooks and is only cleared by _remove_hooks, but this plugin has no unload handler that calls _remove_hooks. The timer closure can therefore still call add_infected_mastermind_bot after the plugin is gone, which is exactly the unload case this change intends to prevent; cancel the pending timer or verify that this plugin remains registered before adding the bot.
Useful? React with 👍 / 👎.
added 2 commits
August 27, 2026 23:22
…oad guard Both findings verified against source before accepting; both were correct. 1. ban id allocation was left inconsistent across writers (P2) The INCR allocator in ban.py only covered one of three plugins that write the same minqlx:players:<id>:bans zset. player_info.py:398 and the preset's kickban.py:163 still derived ids from zcard, and kickban.py's own comment already recorded this as a "known limitation shared with ban.py". Mixing the two allocators is worse than the original: before, all three were consistently racy under concurrency; after, an INCR writer and a zcard writer collide deterministically in ordinary mixed use. !ban takes id 0 (counter=1), an automatic ban takes zcard=1, the next !ban takes INCR=1 and overwrites it. All three now allocate through an identical allocate_ban_id() helper, so the fix is whole rather than half-applied. The helper is duplicated per plugin because minqlx plugins are standalone modules with no shared import path; its docstring names the other two copies so they stay in step. voteban.py also derives ids from zcard, but on :votebans -- a different key that none of these writers touch -- so it is deliberately left alone. 2. infectedmm's unload guard could not actually detect an unload (P2) The guard tested self._loaded, which is only cleared by _remove_hooks, and nothing calls that on unload. The gametype half of the check worked, but the unload half the comment promised did not: @minqlxtended.delay never cancels a pending timer and the closure keeps the instance alive. Now checks the engine's own registry -- self.plugins.get(cls) is not self -- which answers the real question and additionally covers a reload, where a new instance has replaced this one. Preset copies of ban.py, player_info.py and infectedmm.py synced; manifest regenerated. 93 tests pass across the baseline, preset, manifest and compat suites.
main was rewritten while this branch was open: the branding.py commit this branch forked from (5e614d4) was replaced by an equivalent one (e3d6bbe), so both sides carried their own copy of the same change and every branding.py conflicted. The two versions differ only in comment wording -- main's adds an `(e.g. "Till Merker")` example to the cvar docs -- so main's copy is taken wholesale for both the baseline and the preset. Regenerating the manifest afterwards also fixes branding.py's hash, which e3d6bbe left stale exactly as 5e614d4 had: test_every_manifest_hash_matches_ the_file_on_disk is failing on main right now, and merging this branch is what corrects it. 139 tests pass across baseline, preset, manifest and compat suites.
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
1 participant
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.
Summary
Fixes every finding that survived assessment in a review of all 32 upstream-vendored minqlxtended plugins. Each plugin was reviewed against the
minqlx-plugin-reviewchecklist, and every finding was then independently re-verified against source before being accepted — two findings were rejected that way (both claimed a mutation-during-iterationRuntimeErrorinplugin_manager.py;self.pluginsis a.copy()snapshot, so neither was real).10 findings accepted across 7 files. None critical, one high.
vpnblock.py:228_update_cacheruns under@minqlxtended.threadand calledself.msg()directly at 4 sites — an engine call off the game thread, on every load, every ~12h refresh, and all three error paths. Routed through a_msg_next_framehelper.balance.py:333add_requestfrom inside anext_framehandler on any mid-fetch roster change, so a shuffle landing mid-fetch could chase its own tail. Bounded byMAX_REFETCH; on giving up it answers with cached data, whichself.rating()already degrades safely.balance.py:377!elo/!ratings/!teams/!balancefan out to the ratings API with no gate. Right afterhandle_new_gameclearsself.ratings, several players asking at once each spawned a separate thread against the same endpoint. Added a server-wideCOMMAND_COOLDOWN— the fetch they trigger is shared, so the flood to guard against is concurrent askers, not one repeat asker.balance.py:472self.game.type_short/stateunguarded, whilehandle_new_gamein the same file guards the identical hazard. A command typed during a map change raisedAttributeError. Guarded to match.balance.py:762execute_suggestion's@delay(1)stat restore never re-resolved its players. A disconnect in that window threw and skipped the other player's restore too. Each player now restores independently behind its own guard.dictionary.py:31!defineispermission=0and forked an HTTP worker per call with no gate. Per-player cooldown; the tracking map self-prunes.leaverban.py:146handle_game_startcounted bots intoplayers_start, inflatinggames_completedin Redis with no matchinggames_left. Filtered to match the rest of the file.infectedmm.py:64g_rrInfectedwas toggled off or the plugin unloaded. Re-checks gametype and_loadedfirst.ban.py:105zcardread outside the pipeline — two admins banning one player concurrently both read the same count and the second write overwrote the first, while both reported success. Now an atomicINCR, seeded from the existing count viaSETNXso ids on existing installs are preserved.aliases.py:101cmd_setnoaliasesfell through with a barereturnon a failed identifier resolve, wherecmd_alias60 lines up returnsReturn.STOP_ALL.Also in this PR
manifest.json. This additionally correctsbranding.py's hash, which was left stale by5e614d4—test_every_manifest_hash_matches_the_file_on_diskwas failing onmainas a result.default-minqlxtended/scripts/. They were verified byte-identical to the baseline beforehand. Note that no test pins upstream preset copies to the baseline —test_preset_scripts_match_the_baseline_portsonly covers the six QLSM ports — so this seam is currently untested.originmarks lineage, andtest_upstream_files_are_marked_upstreamrequires all 33 upstream names stay"upstream".Not in this PR
vpnblock.py:176(cmd_bypassvpn) has the same bare-return-on-failed-resolve bug asaliases.py:101. The review did not flag it, so it is left out rather than expanding scope.dngrtech/minqlxtended-pluginsand repointing the engine clone insetup_host.ymltodngrtech/minqlxtended. That touches test pins and build config and belongs in its own PR.Test plan
151 passed. All 7 patched files compile under
py_compile.Full review, including the 2 rejected findings and the reasoning on each:
docs/findings/2026-08-27-minqlxtended-upstream-plugin-review.md(gitignored process artifact, local only).