diff --git a/src/specify_cli/integration_scaffold.py b/src/specify_cli/integration_scaffold.py index f0ed210332..cea1116bc1 100644 --- a/src/specify_cli/integration_scaffold.py +++ b/src/specify_cli/integration_scaffold.py @@ -2,6 +2,7 @@ from __future__ import annotations +import keyword import re from dataclasses import dataclass from pathlib import Path @@ -220,6 +221,28 @@ 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 (``match``, ``case``, ``_``) are deliberately + # NOT rejected -- 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/integrations/test_integration_scaffold.py b/tests/integrations/test_integration_scaffold.py index f38ea824d5..d0c12f4cdf 100644 --- a/tests/integrations/test_integration_scaffold.py +++ b/tests/integrations/test_integration_scaffold.py @@ -236,3 +236,56 @@ def test_integration_scaffold_accepts_uppercase_type(tmp_path, monkeypatch): root / "src" / "specify_cli" / "integrations" / "my_agent" / "__init__.py" ).read_text(encoding="utf-8") assert "class MyAgentIntegration(YamlIntegration):" in content + + +@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()