From e12c14c77de5973e5fabbd0376d5cc7277a47955 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Mon, 5 Oct 2026 21:41:33 +0500 Subject: [PATCH 1/2] fix(integrations): refuse scaffold keys that shadow a module or name a keyword `scaffold_integration` validates the key's *shape* (`_KEY_RE`, kebab-case) but never checks what the derived package name would collide with. Two ordinary keys produce broken output: 1. A key matching one of this package's own modules. The scaffold creates a PACKAGE, `integrations//`, and a package shadows a same-named module in the same directory: both 'base.py' and 'base/' present -> import demopkg.base resolves to: scaffolded package base/ Every integration does `from ..base import MarkdownIntegration`, so scaffolding a key named `base` would silently redirect all of them to the empty scaffold. The existing-file guard cannot catch this: it only checks `/__init__.py`, never `.py`. `base`, `catalog` and `manifest` are all currently accepted. 2. A reserved Python keyword. `integrations/class/` cannot be named by any import statement, so the generated package is unreachable. `class`, `import`, `return` and `lambda` are all currently accepted. Both are refused up front now, before anything is written. Soft keywords are deliberately NOT rejected -- they are contextual and `import match` is valid, so `match`, `case` and `_` remain usable keys. This was verified rather than assumed: soft keyword 'match' -> importable? YES iskeyword=False issoftkeyword=True Rebased onto current main (files moved in the workflow/bundler restructure). Co-Authored-By: Claude Opus 5.5 (1M context) --- .../_command_scaffold_generation.py | 24 +++++++++ .../test_command_scaffold_generation.py | 53 +++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/src/specify_cli/integrations/_command_scaffold_generation.py b/src/specify_cli/integrations/_command_scaffold_generation.py index c9f20302fa..2dcb1ce7cc 100644 --- a/src/specify_cli/integrations/_command_scaffold_generation.py +++ b/src/specify_cli/integrations/_command_scaffold_generation.py @@ -2,6 +2,7 @@ from __future__ import annotations +import keyword import re from dataclasses import dataclass from pathlib import Path @@ -220,6 +221,29 @@ def scaffold_integration( raise ValueError("Run this command from the Spec Kit repository root.") package_name = _package_name(clean_key) + # A reserved Python keyword cannot name an importable package: the + # generated ``integrations//`` would be unreachable by any import + # statement. Soft keywords that ``_clean_key`` admits (``match``, + # ``case``) are deliberately NOT rejected here -- they are contextual, and + # ``import match`` is valid. + if keyword.iskeyword(package_name): + raise ValueError( + f"Integration key '{clean_key}' becomes the Python keyword " + f"'{package_name}', which cannot name an importable package. " + "Choose a different key." + ) + # A package shadows a same-named module in the same directory, so + # scaffolding a key that matches one of this package's own modules (base, + # catalog, manifest) would silently take its place -- and every integration + # does ``from ..base import ...``. The existing-file check below cannot + # catch it, because it only looks at ``/__init__.py``. + shadowed = integrations_root / f"{package_name}.py" + if shadowed.exists(): + raise ValueError( + f"Integration key '{clean_key}' collides with the existing module " + f"{shadowed.relative_to(project_root).as_posix()}; the generated " + "package would shadow it. Choose a different key." + ) class_name = _class_name(clean_key) integration_dir = integrations_root / package_name integration_file = integration_dir / "__init__.py" diff --git a/tests/specify_cli/integrations/test_command_scaffold_generation.py b/tests/specify_cli/integrations/test_command_scaffold_generation.py index 1385756f66..96b6cc6c3f 100644 --- a/tests/specify_cli/integrations/test_command_scaffold_generation.py +++ b/tests/specify_cli/integrations/test_command_scaffold_generation.py @@ -131,3 +131,56 @@ def test_scaffold_refuses_symlinked_target_directory(tmp_path): scaffold_integration(root, "my-agent", "markdown") assert not (outside / "my_agent").exists() + + +@pytest.mark.parametrize("key", ["class", "import", "return", "lambda"]) +def test_scaffold_refuses_a_python_keyword_key(tmp_path, key): + """A reserved keyword cannot name an importable package. + + The generated `integrations//` would be unreachable by any import + statement, so the scaffold would emit a package nothing can load. + """ + root = _repo_root(tmp_path) + + with pytest.raises(ValueError, match="Python keyword"): + scaffold_integration(root, key, "markdown") + + assert not (root / "src" / "specify_cli" / "integrations" / key).exists() + + +@pytest.mark.parametrize("key", ["base", "catalog", "manifest"]) +def test_scaffold_refuses_a_key_shadowing_an_existing_module(tmp_path, key): + """A package shadows a same-named module in the same directory. + + `integrations/base.py` and a scaffolded `integrations/base/` can coexist on + disk, and Python resolves the *package* — so `from ..base import ...`, which + every integration does, would silently load the empty scaffold instead. The + existing-file guard cannot catch this: it only checks `/__init__.py`. + """ + root = _repo_root(tmp_path) + (root / "src" / "specify_cli" / "integrations" / f"{key}.py").write_text( + "SENTINEL = 1\n", encoding="utf-8" + ) + + with pytest.raises(ValueError, match="collides with the existing module"): + scaffold_integration(root, key, "markdown") + + # The real module is untouched and no package was created beside it. + assert ( + root / "src" / "specify_cli" / "integrations" / f"{key}.py" + ).read_text(encoding="utf-8") == "SENTINEL = 1\n" + assert not (root / "src" / "specify_cli" / "integrations" / key).exists() + + +@pytest.mark.parametrize("key", ["match", "case", "my-agent"]) +def test_scaffold_still_accepts_soft_keywords_and_ordinary_keys(tmp_path, key): + """Soft keywords are contextual — `import match` is valid, so allow them.""" + root = _repo_root(tmp_path) + + result = scaffold_integration(root, key, "markdown") + + assert result is not None + package = key.replace("-", "_") + assert ( + root / "src" / "specify_cli" / "integrations" / package / "__init__.py" + ).is_file() From 6dd4cb7355cca4db30aed473868e7df953d32d2c Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Thu, 8 Oct 2026 00:32:02 +0500 Subject: [PATCH 2/2] test(integrations): pin the derived-package-name collision; fix comment Every module-collision case had `key == package_name`, so a regression that checked `clean_key` instead of the derived package name still passed. Add `command-info` -> `command_info`, which collides with the real `integrations/command_info.py`; that mutation now fails this case and only this case. Drop `catalog` from the cases and from the source comment: on `main`, `integrations/catalog/` is a package, not a sibling `catalog.py` module, so the existing-file check already rejects that key. The comment now describes the check generically and says it looks for `/__init__.py` rather than `/__init__.py`. Co-Authored-By: Claude Opus 5.5 --- .../_command_scaffold_generation.py | 12 ++++--- .../test_command_scaffold_generation.py | 31 +++++++++++++------ 2 files changed, 28 insertions(+), 15 deletions(-) diff --git a/src/specify_cli/integrations/_command_scaffold_generation.py b/src/specify_cli/integrations/_command_scaffold_generation.py index 2dcb1ce7cc..2dffde0859 100644 --- a/src/specify_cli/integrations/_command_scaffold_generation.py +++ b/src/specify_cli/integrations/_command_scaffold_generation.py @@ -232,11 +232,13 @@ def scaffold_integration( f"'{package_name}', which cannot name an importable package. " "Choose a different key." ) - # A package shadows a same-named module in the same directory, so - # scaffolding a key that matches one of this package's own modules (base, - # catalog, manifest) would silently take its place -- and every integration - # does ``from ..base import ...``. The existing-file check below cannot - # catch it, because it only looks at ``/__init__.py``. + # A package shadows a same-named module in the same directory: when a + # sibling module ``integrations/.py`` already exists, the + # generated ``integrations//`` would silently take its place + # on import -- and every integration does ``from ..base import ...``. The + # check uses the derived package name, so a hyphenated key whose package + # name matches a module is caught as well. The existing-file check below + # cannot catch this case: it only looks for ``/__init__.py``. shadowed = integrations_root / f"{package_name}.py" if shadowed.exists(): raise ValueError( diff --git a/tests/specify_cli/integrations/test_command_scaffold_generation.py b/tests/specify_cli/integrations/test_command_scaffold_generation.py index 96b6cc6c3f..cf3ed8baa9 100644 --- a/tests/specify_cli/integrations/test_command_scaffold_generation.py +++ b/tests/specify_cli/integrations/test_command_scaffold_generation.py @@ -148,28 +148,39 @@ def test_scaffold_refuses_a_python_keyword_key(tmp_path, key): assert not (root / "src" / "specify_cli" / "integrations" / key).exists() -@pytest.mark.parametrize("key", ["base", "catalog", "manifest"]) -def test_scaffold_refuses_a_key_shadowing_an_existing_module(tmp_path, key): +@pytest.mark.parametrize( + "key,module", + [ + ("base", "base"), + ("manifest", "manifest"), + # The derived package name, not the key, is what shadows: the valid + # key `command-info` becomes package `command_info`, which matches the + # real `integrations/command_info.py`. + ("command-info", "command_info"), + ], + ids=["base", "manifest", "hyphenated_key"], +) +def test_scaffold_refuses_a_key_shadowing_an_existing_module(tmp_path, key, module): """A package shadows a same-named module in the same directory. `integrations/base.py` and a scaffolded `integrations/base/` can coexist on disk, and Python resolves the *package* — so `from ..base import ...`, which every integration does, would silently load the empty scaffold instead. The - existing-file guard cannot catch this: it only checks `/__init__.py`. + existing-file guard cannot catch this: it only checks + `/__init__.py`. """ root = _repo_root(tmp_path) - (root / "src" / "specify_cli" / "integrations" / f"{key}.py").write_text( - "SENTINEL = 1\n", encoding="utf-8" - ) + integrations = root / "src" / "specify_cli" / "integrations" + (integrations / f"{module}.py").write_text("SENTINEL = 1\n", encoding="utf-8") with pytest.raises(ValueError, match="collides with the existing module"): scaffold_integration(root, key, "markdown") # The real module is untouched and no package was created beside it. - assert ( - root / "src" / "specify_cli" / "integrations" / f"{key}.py" - ).read_text(encoding="utf-8") == "SENTINEL = 1\n" - assert not (root / "src" / "specify_cli" / "integrations" / key).exists() + assert (integrations / f"{module}.py").read_text(encoding="utf-8") == ( + "SENTINEL = 1\n" + ) + assert not (integrations / module).exists() @pytest.mark.parametrize("key", ["match", "case", "my-agent"])