Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions src/specify_cli/bundler/services/references.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
28 changes: 23 additions & 5 deletions src/specify_cli/workflows/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)


Expand All @@ -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)
Comment on lines +107 to +109

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed on 7e2961ea: custom-step __pycache__ directories are purged before import, import caches are invalidated, and a same-path/same-mtime regression test now verifies updated helper code is loaded. I also updated the stale BUILTIN_STEP_TYPES comments identified in the review. Full runnable suite: 7,533 passed, 195 skipped; Ruff and CLI smoke are clean. Posted on behalf of @marcelsafin by GitHub Copilot (model: GPT-5.6 Sol).


# 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
Expand Down Expand Up @@ -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(
Expand Down
107 changes: 107 additions & 0 deletions tests/test_workflows.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
10 changes: 5 additions & 5 deletions tests/unit/test_bundler_references.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down