From 412fcb5c885e8d131f1fdeeae0634e9435aa2008 Mon Sep 17 00:00:00 2001 From: Loup-Garou911XD <90267658+Loup-Garou911XD@users.noreply.github.com> Date: Sat, 5 Sep 2026 14:35:37 +0530 Subject: [PATCH 1/3] test fixes and improvements --- README.md | 48 ++-- test/auto_apply_plugin_metadata.py | 213 ++++++++++++---- test/test_checks.py | 64 ++++- test/test_plugman_parsing.py | 390 +++++++++++++++++++++++++++++ 4 files changed, 644 insertions(+), 71 deletions(-) create mode 100644 test/test_plugman_parsing.py diff --git a/README.md b/README.md index b78af519..d78bcc45 100644 --- a/README.md +++ b/README.md @@ -87,7 +87,14 @@ There are three different ways the plugin manager can be installed: the category directory you feel is the most relevant to the type of plugin you're submitting, [here](plugins). - You also need a `plugman` dict with the plugin metadata in the plugin (see the [example](https://github.com/bombsquad-community/plugin-manager?tab=readme-ov-file#example) below). - The name of the plugin must be in snake_case and matching the file name. - - Must have the plugin_name, description, external_url, authors and version keys. + - Must have the description, external_url, authors and version keys. + - `plugin_name` is optional and defaults to the file name without `.py`. Spell it out only if you + want it documented in the source; it still has to match the file name. + - Values may be module-level constants defined *above* the dict, so a plugin that already keeps + a `__version__` or `__author__` around does not have to repeat itself: + `version=__version__` and `authors=[{"name": __author__[0], ...}]` both work. The plugin is + parsed, never imported, so only plain literals and indexing into them are understood; + expressions like `__file__.split("/")[-1]` are rejected. - Plugin manager will also show and execute the settings icon if your `ba.Plugin` class has methods `has_settings_ui` and `show_settings_ui`; check out the [colorscheme](https://github.com/bombsquad-community/plugin-manager/blob/eb163cf86014b2a057c4a048dcfa3d5b540b7fe1/plugins/utilities/colorscheme.py#L448-L452) plugin for an example. #### Example: @@ -97,15 +104,18 @@ Let's say you wanna submit this new utility-type plugin named as `sample_plugin. # ba_meta require api 9 import babase +__version__ = "1.0.0" +__author__ = ["Loup", "brostos"] + plugman = dict( - plugin_name="sample_plugin", + plugin_name="sample_plugin", # optional, defaults to the file name description="A test plugin for demonstration purposes blah blah.", external_url="https://www.youtube.com/watch?v=dQw4w9WgXcQ", authors=[ - {"name": "Loup", "email": "loupg450@gmail.com", "discord": "loupgarou_"}, - {"name": "brostos", "email": "", "discord": "brostos"} + {"name": __author__[0], "email": "loupg450@gmail.com", "discord": "loupgarou_"}, + {"name": __author__[1], "email": "", "discord": "brostos"} ], - version="1.0.0", + version=__version__, ) # ba_meta export babase.Plugin @@ -127,7 +137,7 @@ guide) once you open a pull request. ### Updating a Plugin - Make a [pull request](../../compare) with whatever changes you'd like to make to an existing plugin, and add a new - version number in your plugin in the plugman dict. + version number in your plugin in the plugman dict, or in the `__version__` it reads from. #### Example @@ -138,16 +148,26 @@ diff --git a/plugins/utilities/sample_plugin.py b/plugins/utilities/sample_plugi index ebb7dcc..da2b312 100644 --- a/plugins/utilities/sample_plugin.py +++ b/plugins/utilities/sample_plugin.py -@@ -9,7 +9,7 @@ - {"name": "Loup", "email": "loupg450@gmail.com", "discord": "loupgarou_"}, - {"name": "brostos", "email": "", "discord": "brostos"} +@@ -1,16 +1,16 @@ + # ba_meta require api 9 + import babase + +-__version__ = "1.0.0" ++__version__ = "1.1.0" + __author__ = ["Loup", "brostos"] + + plugman = dict( + plugin_name="sample_plugin", # optional, defaults to the file name + description="A test plugin for demonstration purposes blah blah.", + external_url="https://www.youtube.com/watch?v=dQw4w9WgXcQ", + authors=[ + {"name": __author__[0], "email": "loupg450@gmail.com", "discord": "loupgarou_"}, + {"name": __author__[1], "email": "", "discord": "brostos"} ], -- version="1.0.0", -+ version="1.1.0", + version=__version__, ) - - # ba_meta export babase.Plugin -@@ -21,4 +21,4 @@ +@@ -23,5 +23,5 @@ + def has_settings_ui(self): return True def show_settings_ui(self, source_widget): diff --git a/test/auto_apply_plugin_metadata.py b/test/auto_apply_plugin_metadata.py index 088e3cc9..42c7bc72 100644 --- a/test/auto_apply_plugin_metadata.py +++ b/test/auto_apply_plugin_metadata.py @@ -1,23 +1,23 @@ import sys import json import ast +import copy import os import hashlib import subprocess -import get_latest from auto_apply_version_metadata import get_comparable_version_tuple_from_string DEBUG = True +# index.json is deliberately absent: plugin manager releases add their own +# "x.y.z": null entry by hand (see CLAUDE.md), this script only ever touches the +# category manifests. MANIFEST_PATHS = { "minigames": "plugins/minigames.json", "utilities": "plugins/utilities.json", "maps": "plugins/maps.json", - "plugman": "index.json", } -print("DOES THIS RUN AUTO APPLY PLUGIN METADATA?") - def debug_print(*args, **kwargs): if DEBUG: @@ -37,16 +37,6 @@ def md5sum_of(path): return hashlib.md5(fin.read()).hexdigest() -def get_latest_version(plugin_name, category) -> str: - try: - if category != "plugman": - return get_latest.get_latest_plugin_version(plugin_name, MANIFEST_PATHS[category]) - return get_latest.get_latest_plugman_version() - - except Exception as e: - raise e - - def read_manifest_at(path, ref): """Load a manifest as it exists at `ref`, or None if it can't be read there.""" try: @@ -87,19 +77,6 @@ def get_published_versions(plugin_name, category): return manifest["plugins"].get(plugin_name, {}).get("versions", {}) -def update_plugman_json(version): - with open("index.json", "r+") as file: - data = json.load(file) - plugman_version = int(get_latest_version("plugin_manager", "plugman").replace(".", "")) - current_version = int(version["version"].replace(".", "")) - - if current_version > plugman_version: - with open("index.json", "r+") as file: - data = json.load(file) - data[current_version] = None - data["versions"] = dict(sorted(data["versions"].items(), reverse=True)) - - def update_plugin_json(plugin_info, category, plugin_path): name = plugin_info["plugin_name"] version = plugin_info["version"] @@ -158,6 +135,125 @@ def update_plugin_json(plugin_info, category, plugin_path): file.truncate() +def resolve_literal(node, constants, where): + """ast.literal_eval, widened to module-level constants and indexing into them. + + Plugins commonly keep a `__version__` / `__author__` dunder next to the code + that uses them and want the plugman dict to reference those rather than + repeat the values. Nothing here imports or executes the plugin, so only + names this module already resolved to a literal are available, and only + indexing (not attribute access or calls) is applied to them. + """ + if isinstance(node, ast.Name): + if node.id not in constants: + raise ValueError( + f"{where}: {node.id} is not a module-level constant defined above " + "plugman. Only names assigned a literal earlier in the file can be " + "referenced, because the plugin is parsed, never imported." + ) + # Copied so an entry like authors=__author__ cannot be mutated later + # through the constants table. + return copy.deepcopy(constants[node.id]) + + if isinstance(node, ast.Subscript): + container = resolve_literal(node.value, constants, where) + index = resolve_literal(node.slice, constants, where) + try: + return container[index] + except (TypeError, KeyError, IndexError) as err: + raise ValueError( + f"{where}: cannot index {container!r} with {index!r}: {err}" + ) from None + + if isinstance(node, (ast.List, ast.Tuple, ast.Set)): + items = [resolve_literal(item, constants, where) for item in node.elts] + if isinstance(node, ast.Tuple): + return tuple(items) + if isinstance(node, ast.Set): + return set(items) + return items + + if isinstance(node, ast.Dict): + if any(key is None for key in node.keys): + raise ValueError(f"{where}: ** unpacking inside plugman is not supported.") + return { + resolve_literal(key, constants, where): resolve_literal(value, constants, where) + for key, value in zip(node.keys, node.values) + } + + try: + return ast.literal_eval(node) + except (ValueError, TypeError, SyntaxError, MemoryError, RecursionError): + raise ValueError( + f"{where}: unsupported expression `{ast.unparse(node)}`. plugman values must " + "be literals, or module-level constants (optionally indexed, e.g. " + "__author__[0]). Method calls such as __file__.split('/') are not evaluated." + ) from None + + +def collect_module_constants(tree, stop_at): + """Module-level names bound to a literal, in the statements before `stop_at`. + + Order matters: a name assigned *after* plugman would raise NameError when the + game imports the plugin, so it must not resolve here either. + """ + constants = {} + for node in tree.body: + if node is stop_at: + break + if isinstance(node, ast.Assign): + targets = node.targets + elif isinstance(node, ast.AnnAssign) and node.value is not None: + targets = [node.target] + else: + continue + try: + value = ast.literal_eval(node.value) + except (ValueError, TypeError, SyntaxError, MemoryError, RecursionError): + continue + for target in targets: + if isinstance(target, ast.Name): + constants[target.id] = value + return constants + + +def build_plugman_info(node, tree, plugin, file_name_no_extension): + """Turn a `plugman = dict(...)` call node into the dict the manifest needs.""" + where = f"{plugin}: plugman" + constants = collect_module_constants(tree, stop_at=node) + + result = {} + for keyword in node.value.keywords: + if keyword.arg is None: + raise ValueError(f"{where}: ** unpacking inside plugman is not supported.") + result[keyword.arg] = resolve_literal(keyword.value, constants, where) + + # plugin_name is optional: the only value the check below would ever accept + # is the file's own stem, so a plugin that omits it simply gets that. + if "plugin_name" in result: + plugin_name = result["plugin_name"] + # some basic validation specific to plugin manager + if not isinstance(plugin_name, str): + raise ValueError(f"{where}: plugin_name must be a string.") + if plugin_name != plugin_name.lower(): + raise ValueError("Plugin name in plugman must be in snakecase.") + if plugin_name != file_name_no_extension: + raise ValueError("Plugin name in plugman does not match the file name.") + else: + result["plugin_name"] = file_name_no_extension + + missing = [key for key in ("description", "external_url", "authors", "version") + if key not in result] + if missing: + raise ValueError(f"{where}: missing required key(s) {', '.join(missing)}.") + if not isinstance(result["version"], str): + raise ValueError( + f"{where}: version must be a string in x.y.z form, got " + f"{result['version']!r}. Quote it (version=\"1.0.0\")." + ) + return result + + def extract_plugman(plugins): for plugin in plugins: if "plugins" + os.sep in plugin and plugin.endswith(".py"): @@ -175,41 +271,50 @@ def extract_plugman(plugins): with open(plugin, "r") as f: tree = ast.parse(f.read()) - for node in ast.walk(tree): + # A changed file that reaches the end of this loop without stamping + # the manifest ships against the PREVIOUS version's md5sum. That + # used to pass here silently and only surface much later as an + # opaque "checksum changed" failure in test_latest_version, so + # every path below either updates a manifest or raises. + handled = False + + # Module level statements in source order, so collect_module_constants() + # can tell what is defined ABOVE plugman. Every plugin in the catalog + # assigns plugman at column 0; one hidden inside a function or an if + # was never picked up meaningfully anyway and now raises below. + for node in tree.body: if isinstance(node, ast.Assign) and len(node.targets) == 1: target = node.targets[0] if isinstance(target, ast.Name) and target.id == "plugman": - if isinstance(node.value, ast.Dict): - # i dont want to support multiple formats for now - # because its harder to parse and maintain - # ill leave this here for now, though not supported - # Standard dictionary format {key: value} - return ast.literal_eval(node.value) - elif ( + if ( isinstance(node.value, ast.Call) and isinstance(node.value.func, ast.Name) and node.value.func.id == "dict" ): # dict() constructor format - result = {} - for kw in node.value.keywords: - if kw.arg == "plugin_name": - plugin_name = ast.literal_eval(kw.value) - # some basic validation specific to plugin manager - if plugin_name != plugin_name.lower(): - raise ValueError( - "Plugin name in plugman must be in snakecase." - ) - if plugin_name != file_name_no_extension: - raise ValueError( - "Plugin name in plugman does not match the file name." - ) - result[kw.arg] = ast.literal_eval(kw.value) - if category: - update_plugin_json(result, category=category, plugin_path=plugin) - else: - update_plugman_json(result) - # raise ValueError("Variable plugman not found in the file or has unsupported format.") + result = build_plugman_info( + node, tree, plugin, file_name_no_extension + ) + update_plugin_json(result, category=category, plugin_path=plugin) + handled = True + else: + # Only the dict() constructor form is parsed. A literal + # {key: value} was previously returned from here, which + # silently abandoned every remaining changed file too. + raise ValueError( + f"{plugin}: plugman must be assigned with the dict() " + "constructor, e.g. plugman = dict(plugin_name=..., " + "version=...). A literal { } dict is not supported." + ) + + if not handled: + raise ValueError( + f"{plugin}: no plugman dict found. Every plugin under plugins/ " + "needs one so its catalog entry and version can be generated, see\n" + "https://github.com/bombsquad-community/plugin-manager#submitting-a-plugin\n" + f'plugin_name defaults to "{file_name_no_extension}" (the file stem). ' + "Bump version on every edit; the manifest entry is derived from it." + ) if __name__ == "__main__": diff --git a/test/test_checks.py b/test/test_checks.py index 5de80109..125ad248 100644 --- a/test/test_checks.py +++ b/test/test_checks.py @@ -135,20 +135,78 @@ def test_changelog_entries(self): class TestPluginMetadata(unittest.TestCase): def setUp(self): + # os.path.isdir() must be given the joined path. Testing the bare name + # resolves it against the repo root, where no such directory exists, so + # this tuple came out empty and every test below passed vacuously. self.category_directories = tuple( - f'{os.path.join("plugins", path)}' - for path in os.listdir("plugins") if os.path.isdir(path) + os.path.join("plugins", path) + for path in sorted(os.listdir("plugins")) + if os.path.isdir(os.path.join("plugins", path)) ) + # Fail loudly if discovery ever goes empty again, rather than reporting + # a pass for a comparison of nothing against nothing. + self.assertTrue(self.category_directories, + "no category directories discovered under plugins/") + self.api_version_regexp = re.compile(b"(?<=ba_meta require api )(.*)") + self.entry_point_regexp = re.compile(b"ba_meta export ") + + def plugin_files(self, category): + return sorted(name for name in os.listdir(category) if name.endswith(".py")) def test_no_duplicates(self): unique_plugins = set() total_plugin_count = 0 for category in self.category_directories: - plugins = os.listdir(category) + plugins = self.plugin_files(category) total_plugin_count += len(plugins) unique_plugins.update(plugins) self.assertEqual(len(unique_plugins), total_plugin_count) + def test_plugin_files_and_manifest_entries_agree(self): + """Catches a plugin whose metadata step produced nothing at all. + + auto_apply_plugin_metadata.py now raises rather than skipping a file it + cannot read a plugman dict out of, but this is the independent check on + the result: a .py with no entry would be invisible in-game, and an entry + with no .py makes the manager offer a download that cannot exist. + """ + for category in self.category_directories: + manifest_file = f"{category}.json" + with self.subTest(category=category): + self.assertTrue(os.path.isfile(manifest_file), + f"{category} has no matching {manifest_file}") + with open(manifest_file, "rb") as fin: + entries = set(json.load(fin)["plugins"]) + files = {name[:-len(".py")] for name in self.plugin_files(category)} + self.assertEqual( + sorted(files - entries), [], + f"plugin file(s) under {category} with no entry in {manifest_file}; " + "bump the version in the plugman dict so CI can generate one" + ) + self.assertEqual( + sorted(entries - files), [], + f"entries in {manifest_file} with no matching .py under {category}" + ) + + def test_plugins_declare_their_ba_meta_directives(self): + for category in self.category_directories: + for name in self.plugin_files(category): + plugin = os.path.join(category, name) + with open(plugin, "rb") as fin: + content = fin.read() + with self.subTest(plugin=plugin): + self.assertIsNotNone( + self.api_version_regexp.search(content), + f"{plugin} declares no '# ba_meta require api '. The version " + "tests read that straight out of the source, so without it they " + "fail with an unhelpful AttributeError instead." + ) + self.assertIsNotNone( + self.entry_point_regexp.search(content), + f"{plugin} declares no '# ba_meta export', so the game would " + "load nothing from it." + ) + class BaseCategoryMetadataTestCases: class BaseTest(unittest.TestCase): diff --git a/test/test_plugman_parsing.py b/test/test_plugman_parsing.py new file mode 100644 index 00000000..6ca15461 --- /dev/null +++ b/test/test_plugman_parsing.py @@ -0,0 +1,390 @@ +"""Unit tests for test/auto_apply_plugin_metadata.py. + +test_checks.py validates the manifests that the pipeline has already produced. +These tests cover the step before that: turning a plugin's `plugman = dict(...)` +block into the metadata written to plugins/.json, including the cases +that must be rejected rather than silently skipped. + +Run from the repo root with the rest of the suite: + + python -m unittest discover -v + python -m unittest test.test_plugman_parsing -v +""" + +import ast +import contextlib +import io +import json +import os +import pathlib +import sys +import tempfile +import unittest + +# auto_apply_plugin_metadata.py is a CI script rather than a package module: it +# imports its siblings by bare name (`from auto_apply_version_metadata import +# ...`), so its own directory must be importable. +TEST_DIR = pathlib.Path(__file__).resolve().parent +REPO_ROOT = TEST_DIR.parent +if str(TEST_DIR) not in sys.path: + sys.path.insert(0, str(TEST_DIR)) + +import auto_apply_plugin_metadata as apm # noqa: E402 (needs the path above) + + +def plugman_node(source): + """Return (plugman assign node, module tree) for a snippet of plugin source.""" + tree = ast.parse(source) + for node in tree.body: + if ( + isinstance(node, ast.Assign) + and len(node.targets) == 1 + and isinstance(node.targets[0], ast.Name) + and node.targets[0].id == "plugman" + ): + return node, tree + raise AssertionError("test snippet has no module-level plugman assignment") + + +def build(source, stem="sample_plugin"): + """Run build_plugman_info() over a snippet, as extract_plugman() would.""" + node, tree = plugman_node(source) + return apm.build_plugman_info(node, tree, f"plugins/utilities/{stem}.py", stem) + + +def resolve(expression, constants=None): + return apm.resolve_literal(ast.parse(expression, mode="eval").body, + constants or {}, "snippet") + + +# The keys every plugman dict needs, so each test only spells out what it is about. +COMMON_KEYS = ( + ' description="A test plugin.",\n' + ' external_url="https://example.invalid/",\n' + ' authors=[{"name": "n", "email": "", "discord": ""}],\n' +) + + +class TestCollectModuleConstants(unittest.TestCase): + def constants(self, source): + node, tree = plugman_node(source) + return apm.collect_module_constants(tree, stop_at=node) + + def test_literal_above_plugman_is_collected(self): + constants = self.constants('__version__ = "1.0.0"\nplugman = dict()\n') + self.assertEqual(constants["__version__"], "1.0.0") + + def test_assignment_below_plugman_is_ignored(self): + # It would raise NameError when the game imports the plugin, so it must + # not resolve here either. + constants = self.constants('plugman = dict()\n__version__ = "1.0.0"\n') + self.assertNotIn("__version__", constants) + + def test_non_literal_assignment_is_skipped(self): + constants = self.constants( + "import os\nCWD = os.getcwd()\nCOUNT = 2\nplugman = dict()\n" + ) + self.assertNotIn("CWD", constants) + self.assertEqual(constants["COUNT"], 2) + + def test_annotated_assignment_is_collected(self): + constants = self.constants('__version__: str = "2.0.0"\nplugman = dict()\n') + self.assertEqual(constants["__version__"], "2.0.0") + + def test_last_assignment_wins(self): + constants = self.constants('V = "1"\nV = "2"\nplugman = dict()\n') + self.assertEqual(constants["V"], "2") + + def test_assignment_inside_a_function_is_not_module_level(self): + constants = self.constants('def f():\n HIDDEN = "x"\nplugman = dict()\n') + self.assertNotIn("HIDDEN", constants) + + +class TestResolveLiteral(unittest.TestCase): + def test_plain_literals_are_unchanged(self): + self.assertEqual(resolve('"x"'), "x") + self.assertEqual(resolve("[1, 2]"), [1, 2]) + self.assertEqual(resolve("(1, 2)"), (1, 2)) + self.assertEqual(resolve('{"a": 1}'), {"a": 1}) + self.assertEqual(resolve("-1"), -1) + self.assertIsNone(resolve("None")) + + def test_name_resolves_to_its_constant(self): + self.assertEqual(resolve("__version__", {"__version__": "1.0.0"}), "1.0.0") + + def test_subscript_by_index(self): + authors = {"__author__": ["Loup", "brostos"]} + self.assertEqual(resolve("__author__[0]", authors), "Loup") + self.assertEqual(resolve("__author__[-1]", authors), "brostos") + + def test_subscript_by_dict_key(self): + self.assertEqual(resolve('META["v"]', {"META": {"v": 7}}), 7) + + def test_constants_resolve_inside_nested_containers(self): + result = resolve('[{"name": __author__[1]}]', {"__author__": ["a", "b"]}) + self.assertEqual(result, [{"name": "b"}]) + + def test_resolved_value_is_a_copy(self): + constants = {"__author__": [{"name": "a"}]} + result = resolve("__author__", constants) + result[0]["name"] = "mutated" + self.assertEqual(constants["__author__"][0]["name"], "a") + + def test_unknown_name_is_rejected(self): + with self.assertRaisesRegex(ValueError, "not a module-level constant"): + resolve("__nope__") + + def test_method_call_is_rejected(self): + with self.assertRaisesRegex(ValueError, "unsupported expression"): + resolve("__file__.split('/')[-1]", {"__file__": "a/b.py"}) + + def test_dict_unpacking_is_rejected(self): + with self.assertRaisesRegex(ValueError, r"\*\* unpacking"): + resolve("{**EXTRA}", {"EXTRA": {"a": 1}}) + + def test_bad_index_is_reported_clearly(self): + with self.assertRaisesRegex(ValueError, "cannot index"): + resolve("__author__[5]", {"__author__": ["only one"]}) + + +class TestBuildPlugmanInfo(unittest.TestCase): + def test_plugin_name_defaults_to_the_file_stem(self): + info = build(f'plugman = dict(\n{COMMON_KEYS} version="1.0.0",\n)\n') + self.assertEqual(info["plugin_name"], "sample_plugin") + + def test_explicit_matching_plugin_name_is_kept(self): + info = build( + 'plugman = dict(\n plugin_name="sample_plugin",\n' + f'{COMMON_KEYS} version="1.0.0",\n)\n' + ) + self.assertEqual(info["plugin_name"], "sample_plugin") + + def test_plugin_name_must_match_the_file_name(self): + with self.assertRaisesRegex(ValueError, "does not match the file name"): + build( + 'plugman = dict(\n plugin_name="something_else",\n' + f'{COMMON_KEYS} version="1.0.0",\n)\n' + ) + + def test_plugin_name_must_be_snakecase(self): + with self.assertRaisesRegex(ValueError, "snakecase"): + build( + 'plugman = dict(\n plugin_name="Sample_Plugin",\n' + f'{COMMON_KEYS} version="1.0.0",\n)\n', + stem="Sample_Plugin", + ) + + def test_missing_required_keys_are_named(self): + with self.assertRaisesRegex( + ValueError, "missing required key\\(s\\) description, external_url, authors" + ): + build('plugman = dict(\n version="1.0.0",\n)\n') + + def test_version_must_be_a_string(self): + # __version__ = 1.0 is the easy mistake once the value moves to a dunder. + with self.assertRaisesRegex(ValueError, "version must be a string"): + build(f'__version__ = 1.0\nplugman = dict(\n{COMMON_KEYS}' + " version=__version__,\n)\n") + + def test_dunders_resolve(self): + info = build( + '__version__ = "1.2.3"\n' + '__author__ = ["Loup", "brostos"]\n' + "plugman = dict(\n" + ' description="A test plugin.",\n' + ' external_url="https://example.invalid/",\n' + " authors=[\n" + ' {"name": __author__[0], "email": "a@b.c", "discord": "loupgarou_"},\n' + ' {"name": __author__[1], "email": "", "discord": "brostos"},\n' + " ],\n" + " version=__version__,\n" + ")\n" + ) + self.assertEqual(info["version"], "1.2.3") + self.assertEqual([author["name"] for author in info["authors"]], + ["Loup", "brostos"]) + + +class TestExtractPlugman(unittest.TestCase): + """End to end over a throwaway tree, which is what CI actually invokes.""" + + def setUp(self): + cwd = os.getcwd() + tmp = tempfile.TemporaryDirectory() + # addCleanup rather than tearDown so the chdir is undone even if setUp + # or a test raises; test_checks.py depends on cwd being the repo root. + self.addCleanup(tmp.cleanup) + self.addCleanup(os.chdir, cwd) + + root = pathlib.Path(tmp.name) + (root / "plugins" / "utilities").mkdir(parents=True) + os.chdir(root) + self.write_manifest({"plugins": {}}) + + # The script prints per-file progress for the CI log; keep it out of the + # test report. Failures still surface, they are raised not printed. + self.enterContext(contextlib.redirect_stdout(io.StringIO())) + + # get_published_versions() shells out to `git show :`. The + # temp tree is not a repository, so every ref fails and it falls back to + # the working tree manifest, which is what these tests seed. + base_ref = os.environ.pop("PLUGMAN_BASE_REF", None) + if base_ref is not None: + self.addCleanup(os.environ.__setitem__, "PLUGMAN_BASE_REF", base_ref) + + def write_manifest(self, content): + pathlib.Path("plugins/utilities.json").write_text( + json.dumps(content, indent=2), encoding="utf-8") + + def manifest(self): + return json.loads( + pathlib.Path("plugins/utilities.json").read_text(encoding="utf-8")) + + def write_plugin(self, stem, source): + path = f"plugins/utilities/{stem}.py" + pathlib.Path(path).write_text(source, encoding="utf-8") + return path + + def test_writes_a_null_placeholder_for_a_new_plugin(self): + path = self.write_plugin( + "sample_plugin", f'plugman = dict(\n{COMMON_KEYS} version="1.0.0",\n)\n') + apm.extract_plugman([path]) + entry = self.manifest()["plugins"]["sample_plugin"] + self.assertEqual(entry["versions"], {"1.0.0": None}) + self.assertEqual(entry["description"], "A test plugin.") + + def test_dunder_metadata_end_to_end(self): + path = self.write_plugin( + "sample_plugin", + '__version__ = "1.0.0"\n' + '__author__ = ["Loup", "brostos"]\n' + "plugman = dict(\n" + ' description="A test plugin.",\n' + ' external_url="https://example.invalid/",\n' + " authors=[\n" + ' {"name": __author__[0], "email": "", "discord": ""},\n' + ' {"name": __author__[1], "email": "", "discord": ""},\n' + " ],\n" + " version=__version__,\n" + ")\n", + ) + apm.extract_plugman([path]) + entry = self.manifest()["plugins"]["sample_plugin"] + self.assertEqual(entry["versions"], {"1.0.0": None}) + self.assertEqual([author["name"] for author in entry["authors"]], + ["Loup", "brostos"]) + + def test_missing_plugman_dict_raises(self): + path = self.write_plugin("sample_plugin", "# ba_meta require api 9\nx = 1\n") + with self.assertRaisesRegex(ValueError, "no plugman dict found"): + apm.extract_plugman([path]) + self.assertEqual(self.manifest()["plugins"], {}) + + def test_literal_dict_form_raises(self): + path = self.write_plugin( + "sample_plugin", 'plugman = {"plugin_name": "sample_plugin"}\n') + with self.assertRaisesRegex(ValueError, "dict\\(\\) constructor"): + apm.extract_plugman([path]) + + def test_paths_outside_plugins_are_ignored(self): + apm.extract_plugman(["plugin_manager.py", ".github/workflows/ci.yml", + "CHANGELOG.md", "index.json"]) + self.assertEqual(self.manifest()["plugins"], {}) + + def test_every_changed_file_is_processed(self): + # A `return` used to abandon the rest of the list partway through. + paths = [ + self.write_plugin( + stem, f'plugman = dict(\n{COMMON_KEYS} version="1.0.0",\n)\n') + for stem in ("plugin_one", "plugin_two") + ] + apm.extract_plugman(paths) + self.assertEqual(sorted(self.manifest()["plugins"]), + ["plugin_one", "plugin_two"]) + + def test_version_must_be_greater_than_the_published_one(self): + self.write_manifest({ + "plugins": { + "sample_plugin": { + "description": "A test plugin.", + "external_url": "https://example.invalid/", + "authors": [{"name": "n", "email": "", "discord": ""}], + "versions": {"1.1.0": {"md5sum": "whatever"}}, + } + } + }) + path = self.write_plugin( + "sample_plugin", f'plugman = dict(\n{COMMON_KEYS} version="1.0.0",\n)\n') + with self.assertRaisesRegex(Exception, "cant be lower or equal"): + apm.extract_plugman([path]) + + def test_newest_version_is_listed_first(self): + self.write_manifest({ + "plugins": { + "sample_plugin": { + "description": "A test plugin.", + "external_url": "https://example.invalid/", + "authors": [{"name": "n", "email": "", "discord": ""}], + "versions": {"1.0.9": {"md5sum": "whatever"}}, + } + } + }) + path = self.write_plugin( + "sample_plugin", f'plugman = dict(\n{COMMON_KEYS} version="1.0.10",\n)\n') + apm.extract_plugman([path]) + versions = self.manifest()["plugins"]["sample_plugin"]["versions"] + self.assertEqual(list(versions), ["1.0.10", "1.0.9"]) + + +class TestVersionKey(unittest.TestCase): + """version_key orders every `versions` block in every manifest.""" + + def test_orders_numerically_not_lexically(self): + self.assertGreater(apm.version_key("1.0.10"), apm.version_key("1.0.9")) + self.assertGreater(apm.version_key("1.10.0"), apm.version_key("1.9.0")) + self.assertGreater(apm.version_key("2.0.0"), apm.version_key("1.99.99")) + + def test_equal_versions_compare_equal(self): + self.assertEqual(apm.version_key("1.2.3"), apm.version_key("1.2.3")) + + def test_sorts_a_versions_block_newest_first(self): + versions = ["1.0.9", "1.0.10", "1.1.0", "1.0.2"] + self.assertEqual( + sorted(versions, key=apm.version_key, reverse=True), + ["1.1.0", "1.0.10", "1.0.9", "1.0.2"], + ) + + def test_non_numeric_versions_are_rejected(self): + for version in ("1.0.0-beta", "v1.0.0", "1.0.x", ""): + with self.subTest(version=version): + with self.assertRaisesRegex(ValueError, "not in x.y.z form"): + apm.version_key(version) + + +class TestCatalogPlugmanBlocks(unittest.TestCase): + """Every plugman block actually in the catalog must still parse.""" + + def test_every_plugman_block_in_the_catalog_parses(self): + checked = 0 + for path in sorted((REPO_ROOT / "plugins").glob("*/*.py")): + tree = ast.parse(path.read_text(encoding="utf-8")) + for node in tree.body: + if not ( + isinstance(node, ast.Assign) + and len(node.targets) == 1 + and isinstance(node.targets[0], ast.Name) + and node.targets[0].id == "plugman" + ): + continue + with self.subTest(plugin=path.name): + self.assertIsInstance(node.value, ast.Call) + self.assertEqual(node.value.func.id, "dict") + info = apm.build_plugman_info(node, tree, str(path), path.stem) + self.assertEqual(info["plugin_name"], path.stem) + self.assertIsInstance(info["version"], str) + checked += 1 + self.assertGreater(checked, 0, "no plugman blocks found in plugins/") + + +if __name__ == "__main__": + unittest.main() From 883a4ff46e8f2e733ac048977982da2ca79abe8f Mon Sep 17 00:00:00 2001 From: Loup-Garou911XD <90267658+Loup-Garou911XD@users.noreply.github.com> Date: Sat, 5 Sep 2026 19:36:34 +0530 Subject: [PATCH 2/3] ci fix --- .github/workflows/ci.yml | 75 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a5dc8d5d..b1fead78 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -7,6 +7,26 @@ name: CI # here unmodified/strict against real, permanent git history (unlike # ci-check.yml, which can't yet resolve a commit sha for a brand-new plugin # version and runs leniently instead). +# +# It must run the SAME metadata pipeline as the PR path (ci-check.yml -> +# ci-apply.yml): autopep8, then auto_apply_plugin_metadata.py, then +# auto_apply_version_metadata.py. Content can reach main without ever going +# through the PR workflows - a direct maintainer push, or a merge of a PR +# whose checks failed - so a gap here is not covered anywhere else. +# +# Concretely, without the plugin-metadata step a push that bumps a plugin's +# `plugman` version lands the new bytes with no matching manifest entry. The +# manifest goes on advertising the OLD version's md5sum, so the in-game +# manager's checksum check rejects every download of that plugin, and +# test_latest_version fails on this and every later run. That is what 8628c97 +# (finder 1.0 -> 4.1) did to main; it stayed broken until PR #485 reverted the +# whole change, which restored the old bytes rather than recording the new +# ones. +# +# The auto-commit steps below push with the default GITHUB_TOKEN, and GitHub +# does not start a workflow run for those pushes (there is no ci.yml run for +# b1be5d5, the "[ci] apply-formatting" commit). So this job sees each push +# exactly once and does not re-enter itself. on: push: @@ -44,6 +64,61 @@ jobs: with: commit_message: "[ci] apply-formatting" + # Resolved BEFORE any of the auto-commit steps below rewrite HEAD, so it + # stays the tip main had when this push arrived. github.event.before is + # absent/zeroed on a branch's first push and dangling after a force + # push, hence the fallbacks; HEAD^ is the pushed head's first parent, + # which for a merge commit is the previous main tip. + - name: Resolve push base + env: + BEFORE_SHA: ${{ github.event.before }} + run: | + set -euo pipefail + BASE="$BEFORE_SHA" + if [ -z "$BASE" ] || ! git cat-file -e "${BASE}^{commit}" 2>/dev/null; then + # 4b825dc.. is git's empty tree - diffs as "everything is new" and + # simply fails to resolve as a manifest ref, which the metadata + # script handles by falling back to origin/main. + BASE="$(git rev-parse HEAD^ 2>/dev/null || echo 4b825dc642cb6eb9a060e54bf8d69288fbee4904)" + fi + echo "$BASE" > "${RUNNER_TEMP}/base_sha.txt" + echo "Push base: $BASE" + + # The counterpart of ci-check.yml's plugin-metadata step. Diffed against + # HEAD (not the push head) so plugin files that only the autopep8 commit + # above touched are included - their bytes changed, so their checksums + # must be recomputed too. + # + # PLUGMAN_BASE_REF is REQUIRED here, for the same reason ci-check.yml + # sets it. It makes "already published" mean the manifest as of the push + # base, rather than the working tree. On the ordinary path - merging a + # PR - the merge brings the version bump AND the manifest entry that + # ci-apply.yml already stamped onto the PR branch, so the working tree + # here ALREADY lists the version being pushed. Comparing against that + # would make the script raise "Version cant be lower or equal" on every + # single merge. Compared against the base, the version is correctly seen + # as new, the entry is found already stamped with a matching md5sum, and + # the step is a no-op. + # + # This step FAILING is a feature: it means a plugin's bytes changed with + # no version bump, which would leave every existing install unable to + # verify its download. + - name: Apply Plugin Metadata (writes null version placeholders) + run: | + set -euo pipefail + BASE="$(cat "${RUNNER_TEMP}/base_sha.txt")" + # --diff-filter=d drops deletions; the script opens each path it is + # handed, so a removed plugin would crash it. + git diff --name-only --diff-filter=d "$BASE" HEAD > "${RUNNER_TEMP}/changed_files.txt" + cat "${RUNNER_TEMP}/changed_files.txt" + PLUGMAN_BASE_REF="$BASE" \ + python test/auto_apply_plugin_metadata.py "$(cat "${RUNNER_TEMP}/changed_files.txt")" + + - name: Commit Plugin Metadata + uses: stefanzweifel/git-auto-commit-action@v7 + with: + commit_message: "[ci] apply-plugin-metadata" + - name: Apply Version Metadata run: | python test/auto_apply_version_metadata.py $(git log --pretty=format:'%h' -n 1) From 186c808fc23b7b1136c047e4dff5a1abdde73a18 Mon Sep 17 00:00:00 2001 From: Loup-Garou911XD <90267658+Loup-Garou911XD@users.noreply.github.com> Date: Sat, 5 Sep 2026 19:52:32 +0530 Subject: [PATCH 3/3] ci: fail loudly when PR Apply cannot apply fixups PR Apply resolved the target PR with a single "exactly one PR associated with this sha" guard whose failure path called skip(), which exits 0. A sha -> PR association is eventually consistent, so a run starting seconds after a push can see an empty list: that is what happened to PR #479, where PR Apply reported success having pushed nothing. The branch merged without its generated manifest entries and broke main, forcing the revert in #485. Split the guard. Zero associations now polls the API five times before giving up, and both terminal outcomes (still zero, or more than one) fail the job instead of painting it green. skip() stays reserved for states that self-heal or are none of our business. Add a metadata-fixpoint commit status as the actual merge gate. PR Check cannot be one: it generates the metadata into its own workspace and tests that tree, so it goes green on a branch whose committed manifests are still empty. A correctly applied PR is a fixpoint, so the status reports success only when both apply steps found nothing left to push. Every path that leaves fixups unapplied now leaves the status unset rather than green. Make it a required check on main. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Fwri7UzHia9kp3tPH8GgQz --- .github/workflows/ci-apply.yml | 85 ++++++++++++++++++++++++++++++++-- .github/workflows/ci-check.yml | 9 ++++ 2 files changed, 91 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci-apply.yml b/.github/workflows/ci-apply.yml index 046e97ce..546267a1 100644 --- a/.github/workflows/ci-apply.yml +++ b/.github/workflows/ci-apply.yml @@ -80,9 +80,40 @@ jobs: exit 1 } - gh api "repos/${REPO}/commits/${RUN_HEAD_SHA}/pulls" > pulls.json - [ "$(jq 'length' pulls.json)" = "1" ] \ - || skip "Not exactly one PR associated with ${RUN_HEAD_SHA}; skipping." + # The sha -> PR association is eventually consistent: a run that + # starts seconds after the contributor's push can legitimately see + # an empty list. Poll before giving up. + # + # Both terminal outcomes below FAIL rather than `skip`. `skip` exits + # 0, which paints the whole job green - and a green PR Apply that + # pushed nothing is exactly how an unstamped branch merged to main + # and broke it (PR #479, reverted in #485). `skip` is reserved for + # states that self-heal (head moved -> a new run follows) or are + # none of our business (PR closed, not targeting main). "I could not + # identify the PR" is neither: it means the fixups were dropped on + # the floor, and that must be visible. + for attempt in 1 2 3 4 5; do + gh api "repos/${REPO}/commits/${RUN_HEAD_SHA}/pulls" > pulls.json + N_PULLS="$(jq 'length' pulls.json)" + [ "$N_PULLS" = "0" ] || break + echo "No PR associated with ${RUN_HEAD_SHA} yet (attempt ${attempt}/5); retrying in 10s." + sleep 10 + done + + if [ "$N_PULLS" = "0" ]; then + echo "No PR associated with ${RUN_HEAD_SHA} after 5 attempts." >&2 + echo "Formatting/metadata fixups were NOT applied to the branch." >&2 + exit 1 + fi + + # More than one PR sharing a head sha is the ambiguity HARD RULE 3 + # exists for: pushing would risk writing to the wrong branch. Fail + # closed, but fail loudly - silence here is what bit us before. + if [ "$N_PULLS" != "1" ]; then + echo "${N_PULLS} PRs share head ${RUN_HEAD_SHA}; cannot resolve which one this run belongs to." >&2 + echo "Failing closed: nothing was applied. A maintainer must apply the fixups manually." >&2 + exit 1 + fi PR_NUMBER="$(jq -r '.[0].number' pulls.json)" [[ "$PR_NUMBER" =~ ^[0-9]+$ ]] || skip "Bad PR number; skipping." @@ -231,6 +262,7 @@ jobs: git apply --check "$PATCH" - name: Apply fixups patch and commit + id: fixups if: steps.pr.outputs.proceed == '1' working-directory: pr env: @@ -260,9 +292,11 @@ jobs: if ! git diff --cached --quiet; then git commit -m "[ci] apply-plugin-metadata-and-formatting" git push "https://x-access-token:${GH_TOKEN}@github.com/${FORK}.git" "HEAD:${HEAD_REF}" + echo "pushed=1" >> "$GITHUB_OUTPUT" fi - name: Apply Version Metadata using the TRUSTED script only (never the fork's copy) + id: vermeta if: steps.pr.outputs.proceed == '1' working-directory: pr env: @@ -280,7 +314,52 @@ jobs: if ! git diff --cached --quiet; then git commit -m "[ci] apply-version-metadata" git push "https://x-access-token:${GH_TOKEN}@github.com/${FORK}.git" "HEAD:${HEAD_REF}" + echo "pushed=1" >> "$GITHUB_OUTPUT" + fi + + # THE MERGE GATE. PR Check is not one: it GENERATES the metadata into its + # own workspace and then tests that generated tree, so it goes green on a + # branch whose committed manifests are still empty. That is precisely how + # #479 merged and broke main - Check was green, Apply had silently + # skipped, and nothing anywhere asserted the branch was actually stamped. + # + # The invariant that does hold: a correctly-applied PR is a FIXPOINT - + # re-running the pipeline over it produces no changes. So this reports + # success only when both steps above found nothing left to push. When + # they did push, the new head sha retriggers Check -> Apply, and that run + # sets success on the sha a maintainer would actually merge. + # + # Make `metadata-fixpoint` a required status check on main. Then every + # path that leaves fixups unapplied - a skip, a hard failure, a crash - + # leaves the status unset and the PR unmergeable, instead of green. + - name: Report metadata fixpoint status + if: always() && steps.pr.outputs.proceed == '1' + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + HEAD_SHA: ${{ steps.pr.outputs.head_sha }} + FIXUPS_PUSHED: ${{ steps.fixups.outputs.pushed }} + VERMETA_PUSHED: ${{ steps.vermeta.outputs.pushed }} + FIXUPS_RESULT: ${{ steps.fixups.outcome }} + VERMETA_RESULT: ${{ steps.vermeta.outcome }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + run: | + set -euo pipefail + if [ "$FIXUPS_RESULT" != "success" ] || [ "$VERMETA_RESULT" != "success" ]; then + STATE="failure" + DESC="Formatting/metadata could not be applied; see the PR Apply run." + elif [ -n "$FIXUPS_PUSHED" ] || [ -n "$VERMETA_PUSHED" ]; then + STATE="failure" + DESC="Formatting/metadata was just applied; waiting for the re-run on the new commit." + else + STATE="success" + DESC="Formatting and generated metadata are up to date on this commit." fi + gh api --method POST "repos/${REPO}/statuses/${HEAD_SHA}" \ + -f state="$STATE" \ + -f context="metadata-fixpoint" \ + -f description="$DESC" \ + -f target_url="$RUN_URL" - name: On mechanical failure, notify the contributor if: failure() && steps.pr.outputs.pr_number != '' diff --git a/.github/workflows/ci-check.yml b/.github/workflows/ci-check.yml index ea4b8d8c..cfd4cc21 100644 --- a/.github/workflows/ci-check.yml +++ b/.github/workflows/ci-check.yml @@ -17,6 +17,15 @@ name: PR Check # DO NOT switch this to `pull_request_target` and do not add `permissions: # contents: write` here "to save a round trip" - that reintroduces the # pwn-request hole this split exists to close. +# +# THIS JOB IS NOT A MERGE GATE. It GENERATES the formatting and metadata into +# its own workspace and then tests THAT tree, so it goes green on a branch +# whose committed manifests are still empty - which is exactly how #479 merged +# and broke main. Do not add a "patch must be empty" check here either: the +# first run on any real PR legitimately produces a patch, and failing would +# stop ci-apply.yml (`conclusion == 'success'`) from ever applying it. The gate +# is the `metadata-fixpoint` commit status that ci-apply.yml sets; make that +# the required check on main. on: pull_request: