mirror of
https://github.com/github/spec-kit.git
synced 2026-07-07 14:39:00 +08:00
Add workflow step catalog — community-installable step types (#2394)
* Initial plan * Add workflow step catalog: StepRegistry, StepCatalog, CLI commands, and tests Agent-Logs-Url: https://github.com/github/spec-kit/sessions/2885e646-477d-4df8-b9a3-06d8cb29e748 Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com> * Potential fix for pull request finding 'An assert statement has a side-effect' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> * Address PR review: path traversal, cache robustness, collision check, failed-to-load display - Add resolve()+relative_to() path traversal guards in workflow_step_add and workflow_step_remove to prevent directory escape via step_id - Harden _is_url_cache_valid in both StepCatalog and WorkflowCatalog to coerce fetched_at to float and catch TypeError/ValueError - Check STEP_REGISTRY and StepRegistry before installing to prevent collisions with built-in step types or already-installed steps - Show 'Custom (installed, failed to load)' section in workflow step list for steps in the registry that failed to load into STEP_REGISTRY * Fix StepRegistry shape validation and StepCatalog empty-YAML handling Agent-Logs-Url: https://github.com/github/spec-kit/sessions/0dca6393-f5a9-40de-bb5c-77ba6af033d2 Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com> * Polish: rename _default to default_registry, strengthen unreadable-file test Agent-Logs-Url: https://github.com/github/spec-kit/sessions/0dca6393-f5a9-40de-bb5c-77ba6af033d2 Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com> * Address PR review: atomic install, hostname validation, cache resilience, no dynamic imports in list/info Agent-Logs-Url: https://github.com/github/spec-kit/sessions/3e18fef0-e2e6-4b3e-9e8d-9adb1e5e464e Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com> * Fix shutil.move with existing step_dir: remove before move to avoid subdirectory nesting Agent-Logs-Url: https://github.com/github/spec-kit/sessions/3e18fef0-e2e6-4b3e-9e8d-9adb1e5e464e Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com> * Call load_custom_steps at execution time; enforce hostname in _safe_fetch and _validate_url Agent-Logs-Url: https://github.com/github/spec-kit/sessions/73865880-fb25-4061-a43e-4e4b4d1c4de6 Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com> * Wrap YAML parsing in try/except; atomic step install via os.rename() under same fs Agent-Logs-Url: https://github.com/github/spec-kit/sessions/ff915bc5-ec7e-4e6a-b505-35f5795250df Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com> * Validate YAML root is a dict in _load_catalog_config and workflow_step_add; fix WorkflowCatalog hostname validation Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com> * Fix load_custom_steps() package imports and add reserved step ID validation * Move _re/_sys imports out of loop and _RESERVED_STEP_IDS to module level * Address review: collision-resistant module names, extra_files support, remove orphan dir * Harden extra_files: warn on non-dict, resolve symlinks in path traversal check * Switch _safe_fetch and StepCatalog._fetch_single_catalog to use open_url for auth consistency * Harden step_id validation against path-segment tricks; raise on StepRegistry.save() OSError * Clean up sys.modules on broken step packages; handle StepValidationError in step add/remove * Address review thread: int-coerce priorities, sys.modules cleanup, _require_specify_project, registry-first remove * fix: normalize workflow step catalog metadata fallbacks * fix: address latest workflow step and catalog review findings * Handle non-string extra_files keys in workflow step add * Harden StepRegistry symlink reads and extra_files path/URL validation * Harden custom step loader and step remove against symlinks and OSError * Fix StepCatalog.search() to coerce non-string fields before joining * Fix WorkflowCatalog YAML parsing error handling and isinstance checks * Harden step registry save and custom step/catalog ID handling * Harden cache validation and staging OSError handling * Address review: reorder symlink guard and split mixed test - Move symlink-parent check before is_dir() in load_custom_steps() so we never stat an external target through a symlink - Split test_get_merged_steps_normalizes_list_ids_to_strings into two focused tests: one for list-id normalization, one for get_step_info return values Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review: symlink-before-stat in loader, restore registry on rmtree failure - load_custom_steps(): check is_symlink() before is_dir() on step directories so symlinked entries are skipped without statting external targets - workflow_step_remove: restore the registry entry when shutil.rmtree() fails so filesystem and registry state stay consistent and a future 'step add' isn't blocked Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Harden step_id validation and file-write error handling - _validate_step_id_or_exit: reject whitespace-only/padded IDs, Windows-invalid characters (<>:"|?*), control characters, trailing dots/spaces, and Windows reserved device names (con, nul, etc.) - Wrap step.yml/__init__.py staging writes in OSError handler - Wrap extra_files disk writes (mkdir + write_bytes) in OSError handler that names the failing relative path - Registry rollback on rmtree failure: restore verbatim metadata and emit a warning if the restore itself fails Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review: cache symlink guard, verbatim registry rollback, Windows test fix - StepCatalog: add _is_cache_path_safe() guard that checks for symlinks in .specify/workflows/steps/.cache path; skip cache reads and writes when any component is symlinked to prevent writes outside project root - Registry rollback: write metadata directly to registry.data['steps'] and call save() instead of using add() which overwrites timestamps - temp_dir fixture: use ignore_errors=True on Windows to avoid flaky teardown from locked file handles (WinError 32) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Simplify exec_module call by removing redundant nested try/except Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix empty YAML tolerance in WorkflowCatalog.add_catalog, scope ignore_errors to Windows - WorkflowCatalog.add_catalog(): treat None from yaml.safe_load() (empty file) as an empty mapping instead of raising 'corrupted' - temp_dir fixture: limit ignore_errors to sys.platform == 'win32' so real cleanup issues surface on non-Windows platforms Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Chain exceptions in _load_catalog_config for both catalog classes Add 'from exc' to preserve root cause in tracebacks while keeping clean user-facing messages. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Make default catalog tests hermetic by isolating HOME Monkeypatch Path.home() to project_dir and clear catalog env vars so tests don't break on machines with a real ~/.specify/step-catalogs.yml or ~/.specify/workflow-catalogs.yml. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix falsy ID handling in _get_merged_steps for list-based catalogs Check for None explicitly instead of using 'or' which drops valid falsy IDs like 0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Compare reserved step IDs case-insensitively for filesystem safety On case-insensitive filesystems (Windows, common macOS), variants like STEP-REGISTRY.JSON would collide with the actual registry file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add explanatory comments to intentional empty except blocks Document why cache-read failures are silently ignored in both WorkflowCatalog and StepCatalog _fetch_single_catalog methods. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> Co-authored-by: Manfred Riem <mnriem@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Copilot
@@ -7,10 +7,12 @@ Provides:
|
||||
- ``STEP_REGISTRY`` — maps ``type_key`` to ``StepBase`` subclass instances.
|
||||
- ``WorkflowEngine`` — orchestrator that loads, validates, and executes
|
||||
workflow YAML definitions.
|
||||
- ``load_custom_steps`` — loads community-installed step types into STEP_REGISTRY.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from pathlib import Path
|
||||
from typing import TYPE_CHECKING
|
||||
|
||||
if TYPE_CHECKING:
|
||||
@@ -66,3 +68,134 @@ def _register_builtin_steps() -> None:
|
||||
|
||||
|
||||
_register_builtin_steps()
|
||||
|
||||
|
||||
def load_custom_steps(project_root: Path) -> list[str]:
|
||||
"""Load community-installed custom step types into STEP_REGISTRY.
|
||||
|
||||
Scans ``.specify/workflows/steps/`` for installed step packages.
|
||||
Each valid package must contain ``step.yml`` (with a ``step.type_key``
|
||||
field) and ``__init__.py`` (a ``StepBase`` subclass).
|
||||
|
||||
Returns a list of type_keys that were successfully loaded.
|
||||
Silently skips packages that fail to import or validate.
|
||||
"""
|
||||
import hashlib as _hashlib
|
||||
import importlib.util as _importlib_util
|
||||
import re as _re
|
||||
import sys as _sys
|
||||
|
||||
steps_dir = Path(project_root) / ".specify" / "workflows" / "steps"
|
||||
|
||||
# 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
|
||||
# symlink guard. Check symlinks *before* is_dir() since the latter
|
||||
# follows symlinks and would stat an external target.
|
||||
_current = Path(project_root)
|
||||
for _part in (".specify", "workflows", "steps"):
|
||||
_current = _current / _part
|
||||
if _current.is_symlink():
|
||||
return []
|
||||
|
||||
if not steps_dir.is_dir():
|
||||
return []
|
||||
|
||||
loaded: list[str] = []
|
||||
for step_dir in steps_dir.iterdir():
|
||||
# Check symlinks before is_dir() since the latter follows symlinks
|
||||
# and would stat an external target through a symlinked directory.
|
||||
if step_dir.is_symlink():
|
||||
continue
|
||||
if not step_dir.is_dir():
|
||||
continue
|
||||
step_yml = step_dir / "step.yml"
|
||||
init_py = step_dir / "__init__.py"
|
||||
if step_yml.is_symlink() or init_py.is_symlink():
|
||||
continue
|
||||
if not step_yml.is_file() or not init_py.is_file():
|
||||
continue
|
||||
|
||||
try:
|
||||
import yaml as _yaml
|
||||
|
||||
meta = _yaml.safe_load(step_yml.read_text(encoding="utf-8")) or {}
|
||||
step_meta = meta.get("step", {})
|
||||
type_key = step_meta.get("type_key", "")
|
||||
if not type_key:
|
||||
continue
|
||||
|
||||
# Skip if already registered (e.g. built-in or previously loaded)
|
||||
if type_key in STEP_REGISTRY:
|
||||
continue
|
||||
|
||||
# Sanitize type_key so the synthetic module name is a valid identifier
|
||||
# (e.g. "test-custom" → "_speckit_custom_step_test_custom_<hash>").
|
||||
# The 8-char SHA-256 hash of the original type_key makes the name
|
||||
# collision-resistant when different type_keys produce the same
|
||||
# sanitized form (e.g. "a-b" and "a_b" both sanitize to "a_b" but
|
||||
# have different hashes).
|
||||
safe_key = _re.sub(r"[^A-Za-z0-9_]", "_", type_key)
|
||||
key_hash = _hashlib.sha256(type_key.encode()).hexdigest()[:8]
|
||||
module_name = f"_speckit_custom_step_{safe_key}_{key_hash}"
|
||||
|
||||
# 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(
|
||||
module_name,
|
||||
init_py,
|
||||
submodule_search_locations=[str(step_dir)],
|
||||
)
|
||||
if spec is None or spec.loader is None:
|
||||
continue
|
||||
module = _importlib_util.module_from_spec(spec)
|
||||
module.__package__ = module_name
|
||||
# Register before exec so relative imports resolve correctly.
|
||||
_sys.modules[module_name] = module
|
||||
registered = False
|
||||
try:
|
||||
spec.loader.exec_module(module) # type: ignore[union-attr]
|
||||
|
||||
# Find the StepBase subclass in the module
|
||||
from .base import StepBase as _StepBase
|
||||
|
||||
step_class = None
|
||||
for attr_name in dir(module):
|
||||
attr = getattr(module, attr_name)
|
||||
try:
|
||||
if (
|
||||
isinstance(attr, type)
|
||||
and issubclass(attr, _StepBase)
|
||||
and attr is not _StepBase
|
||||
and getattr(attr, "type_key", "") == type_key
|
||||
):
|
||||
step_class = attr
|
||||
break
|
||||
except TypeError:
|
||||
continue
|
||||
|
||||
if step_class is None:
|
||||
continue
|
||||
|
||||
_register_step(step_class())
|
||||
loaded.append(type_key)
|
||||
registered = True
|
||||
finally:
|
||||
# If the step wasn't successfully registered (failed import,
|
||||
# no matching StepBase subclass, or registration error), remove
|
||||
# the synthetic module — and any submodules loaded via relative
|
||||
# imports (e.g. ``from .helpers import …``) — from sys.modules so
|
||||
# a broken/skipped step package leaves no lingering import state
|
||||
# behind.
|
||||
if not registered:
|
||||
_sys.modules.pop(module_name, None)
|
||||
submodule_prefix = module_name + "."
|
||||
for _mod_key in [
|
||||
k for k in _sys.modules if k.startswith(submodule_prefix)
|
||||
]:
|
||||
_sys.modules.pop(_mod_key, None)
|
||||
except Exception: # noqa: BLE001
|
||||
# Silently skip broken step packages at load time
|
||||
continue
|
||||
|
||||
return loaded
|
||||
|
||||
Reference in New Issue
Block a user