diff --git a/src/specify_cli/bundler/services/references.py b/src/specify_cli/bundler/services/references.py index b5419237d5..71a28daefe 100644 --- a/src/specify_cli/bundler/services/references.py +++ b/src/specify_cli/bundler/services/references.py @@ -48,11 +48,10 @@ def _resolved_locally(root: Path, component: ComponentRef) -> bool: # ``_locate_bundled_step`` to mirror the three lookups above. # ``BUILTIN_STEP_TYPES`` is the bundled-with-Spec-Kit check for this # kind. Deliberately NOT ``STEP_REGISTRY``: ``load_custom_steps`` - # adds project-installed ids to that process-global mapping and - # never removes them, so in a long-lived process a community step - # loaded for one project would be accepted as "bundled" when - # validating another. Without any bundled check at all, every - # built-in step type looked unresolved. + # adds the most recently scanned project's ids to that process-global + # mapping, so a community step could be accepted as "bundled" when + # validating a different root. Without any bundled check at all, + # every built-in step type looked unresolved. if component.id in BUILTIN_STEP_TYPES: return True return StepRegistry(root).is_installed(component.id) diff --git a/src/specify_cli/workflows/__init__.py b/src/specify_cli/workflows/__init__.py index 1e608ca168..fd7baf685b 100644 --- a/src/specify_cli/workflows/__init__.py +++ b/src/specify_cli/workflows/__init__.py @@ -74,11 +74,8 @@ def _register_builtin_steps() -> None: _register_builtin_steps() # The step types Spec Kit ships, snapshotted before any community step can be -# loaded. ``load_custom_steps`` adds project-installed ids to the process-global -# ``STEP_REGISTRY`` and never removes them, so ``STEP_REGISTRY`` cannot answer -# "is this bundled with Spec Kit?" in a long-lived process: a step loaded for one -# project would look built-in for the next. Callers that need the immutable set -# (e.g. the bundler's reference checker) must use this instead. +# loaded. Callers that need the immutable set (e.g. the bundler's reference +# checker) must use this instead of the project-scoped entries in STEP_REGISTRY. BUILTIN_STEP_TYPES: frozenset[str] = frozenset(STEP_REGISTRY) @@ -93,12 +90,24 @@ def load_custom_steps(project_root: Path) -> list[str]: Silently skips packages that fail to import or validate. """ import hashlib as _hashlib + import importlib as _importlib import importlib.util as _importlib_util import re as _re + import shutil as _shutil import sys as _sys steps_dir = Path(project_root) / ".specify" / "workflows" / "steps" + # Custom steps are project-scoped even though the registry and Python module + # cache are process-global. Clear the previous project's classes and package + # modules before every scan so removed or updated code cannot remain active. + for _type_key in tuple(STEP_REGISTRY): + if _type_key not in BUILTIN_STEP_TYPES: + STEP_REGISTRY.pop(_type_key, None) + _module_prefix = "_speckit_custom_step_" + for _mod_key in [k for k in _sys.modules if k.startswith(_module_prefix)]: + _sys.modules.pop(_mod_key, None) + # Defense-in-depth: refuse to execute step code from a symlinked # parent directory under .specify/workflows/steps, which could redirect # the import outside the project root and bypass the install-time @@ -151,6 +160,15 @@ def load_custom_steps(project_root: Path) -> list[str]: key_hash = _hashlib.sha256(type_key.encode()).hexdigest()[:8] module_name = f"_speckit_custom_step_{safe_key}_{key_hash}" + # Removing sys.modules entries alone is insufficient for same-path + # reloads: Python may reuse a same-size, same-mtime .pyc file. + # Custom packages are small and source-controlled by the project, + # so discard only their generated bytecode before importing. + for cache_dir in step_dir.rglob("__pycache__"): + if cache_dir.is_dir() and not cache_dir.is_symlink(): + _shutil.rmtree(cache_dir, ignore_errors=True) + _importlib.invalidate_caches() + # Treat the step directory as a proper package so that relative # imports inside the step (e.g. ``from .helpers import …``) work. spec = _importlib_util.spec_from_file_location( diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 980cc21a6e..d1f0bdbb18 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -9504,6 +9504,113 @@ def test_get_step_info_returns_entry_or_none(self, project_dir, monkeypatch): class TestLoadCustomSteps: """Test dynamic loading of custom step types from the filesystem.""" + def test_loading_another_project_replaces_custom_step_modules(self, tmp_path): + import hashlib + import sys + + from specify_cli.workflows import STEP_REGISTRY, load_custom_steps + + type_key = "project-scoped-step" + key_hash = hashlib.sha256(type_key.encode()).hexdigest()[:8] + module_name = f"_speckit_custom_step_project_scoped_step_{key_hash}" + + def write_step(project_root, marker): + step_dir = ( + project_root + / ".specify" + / "workflows" + / "steps" + / type_key + ) + step_dir.mkdir(parents=True) + (step_dir / "step.yml").write_text( + f"step:\n type_key: {type_key}\n", encoding="utf-8" + ) + (step_dir / "helper.py").write_text( + f"MARKER = {marker!r}\n", encoding="utf-8" + ) + (step_dir / "__init__.py").write_text( + f""" +from specify_cli.workflows.base import StepBase, StepResult +from .helper import MARKER + +class ProjectScopedStep(StepBase): + type_key = {type_key!r} + marker = MARKER + + def execute(self, config, context): + return StepResult() +""", + encoding="utf-8", + ) + + project_a = tmp_path / "project-a" + project_b = tmp_path / "project-b" + write_step(project_a, "project-a") + write_step(project_b, "project-b") + + try: + assert load_custom_steps(project_a) == [type_key] + assert STEP_REGISTRY[type_key].marker == "project-a" + + assert load_custom_steps(project_b) == [type_key] + assert STEP_REGISTRY[type_key].marker == "project-b" + finally: + STEP_REGISTRY.pop(type_key, None) + sys.modules.pop(module_name, None) + sys.modules.pop(f"{module_name}.helper", None) + + def test_reloading_same_project_ignores_stale_bytecode(self, tmp_path): + import hashlib + import os + import sys + + from specify_cli.workflows import STEP_REGISTRY, load_custom_steps + + type_key = "reload-step" + key_hash = hashlib.sha256(type_key.encode()).hexdigest()[:8] + module_name = f"_speckit_custom_step_reload_step_{key_hash}" + step_dir = ( + tmp_path / ".specify" / "workflows" / "steps" / type_key + ) + step_dir.mkdir(parents=True) + (step_dir / "step.yml").write_text( + f"step:\n type_key: {type_key}\n", encoding="utf-8" + ) + helper = step_dir / "helper.py" + helper.write_text("MARKER = 'version-a'\n", encoding="utf-8") + (step_dir / "__init__.py").write_text( + f""" +from specify_cli.workflows.base import StepBase, StepResult +from .helper import MARKER + +class ReloadStep(StepBase): + type_key = {type_key!r} + marker = MARKER + + def execute(self, config, context): + return StepResult() +""", + encoding="utf-8", + ) + + try: + assert load_custom_steps(tmp_path) == [type_key] + assert STEP_REGISTRY[type_key].marker == "version-a" + original_stat = helper.stat() + helper.write_text("MARKER = 'version-b'\n", encoding="utf-8") + os.utime( + helper, + ns=(original_stat.st_atime_ns, original_stat.st_mtime_ns), + ) + + assert load_custom_steps(tmp_path) == [type_key] + assert STEP_REGISTRY[type_key].marker == "version-b" + finally: + STEP_REGISTRY.pop(type_key, None) + sys.modules.pop(module_name, None) + sys.modules.pop(f"{module_name}.helper", None) + def test_empty_steps_dir(self, project_dir): from specify_cli.workflows import load_custom_steps diff --git a/tests/unit/test_bundler_references.py b/tests/unit/test_bundler_references.py index b910a93e99..262b347478 100644 --- a/tests/unit/test_bundler_references.py +++ b/tests/unit/test_bundler_references.py @@ -49,11 +49,11 @@ def test_builtin_step_type_resolves(tmp_path: Path): def test_community_step_is_not_treated_as_bundled(tmp_path: Path): """A community step loaded for one project must not resolve for another. - `load_custom_steps` adds project-installed ids to the process-global - `STEP_REGISTRY` and never removes them, so checking `STEP_REGISTRY` here - would accept project A's community step as "bundled" while validating - project B. `BUILTIN_STEP_TYPES` is snapshotted before any custom step can - load, which is why the check uses it instead. + `load_custom_steps` adds the most recently scanned project's ids to the + process-global `STEP_REGISTRY`, so checking `STEP_REGISTRY` here could + accept another project's community step as "bundled". + `BUILTIN_STEP_TYPES` is snapshotted before any custom step can load, which + is why the check uses it instead. """ from specify_cli.workflows import ( BUILTIN_STEP_TYPES,