Add workflow step catalog — community-installable step types#2394
Add workflow step catalog — community-installable step types#2394Copilot wants to merge 17 commits into
Conversation
…nd 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>
…e-effect' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a catalog/registry system for community-installable workflow step types, plus CLI commands to discover, install, and manage them alongside built-in steps.
Changes:
- Introduces
StepRegistryandStepCatalog(multi-source resolution + SHA256 cache) for step type distribution/management. - Adds dynamic filesystem-based loading of installed custom step packages into
STEP_REGISTRY. - Expands CLI with
specify workflow step …and adds tests and initial (empty) catalog JSON files.
Show a summary per file
| File | Description |
|---|---|
| workflows/step-catalog.json | Adds the built-in “official” step catalog scaffold (currently empty). |
| workflows/step-catalog.community.json | Adds the built-in “community” step catalog scaffold (currently empty). |
| src/specify_cli/workflows/catalog.py | Implements StepRegistry + StepCatalog with config resolution and caching. |
| src/specify_cli/workflows/init.py | Adds load_custom_steps(project_root) dynamic import/registration for installed step packages. |
| src/specify_cli/init.py | Adds Typer subcommands for listing/searching/installing/removing steps and managing step catalogs. |
| tests/test_workflows.py | Adds unit tests covering registry CRUD, catalog resolution/validation, search/info, and dynamic loading behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
src/specify_cli/init.py:5555
workflow_step_remove()buildsstep_dirfrom unvalidatedstep_idand thenshutil.rmtree(step_dir). A malicious value like../...could delete arbitrary directories outside.specify/workflows/steps. Add the same resolved-pathrelative_to()guard used byworkflow_remove/workflow_addbefore performing deletions.
step_dir = project_root / ".specify" / "workflows" / "steps" / step_id
if step_dir.exists():
import shutil
shutil.rmtree(step_dir)
- Files reviewed: 6/6 changed files
- Comments generated: 4
… 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
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/specify_cli/workflows/catalog.py:1021
- StepCatalog.add_catalog()/remove_catalog() read YAML without handling yaml.YAMLError/OSError; those exceptions will bubble past the CLI handlers (which only catch StepValidationError) and produce a stack trace. Wrap YAML reads in try/except and re-raise as StepValidationError so the CLI consistently reports a user-facing error.
def remove_catalog(self, index: int) -> str:
"""Remove a catalog source by index (0-based). Returns the removed name."""
config_path = self.project_root / ".specify" / "step-catalogs.yml"
if not config_path.exists():
raise StepValidationError("No step catalog config file found.")
data = yaml.safe_load(config_path.read_text(encoding="utf-8")) or {}
if not isinstance(data, dict):
raise StepValidationError(
"Catalog config file is corrupted (expected a mapping)."
)
- Files reviewed: 6/6 changed files
- Comments generated: 2
| def _load(self) -> dict[str, Any]: | ||
| """Load registry from disk or create default.""" | ||
| if self.registry_path.exists(): | ||
| try: | ||
| with open(self.registry_path, encoding="utf-8") as f: | ||
| return json.load(f) | ||
| except (json.JSONDecodeError, ValueError): | ||
| return {"schema_version": self.SCHEMA_VERSION, "steps": {}} | ||
| return {"schema_version": self.SCHEMA_VERSION, "steps": {}} | ||
|
|
| data: dict[str, Any] = {"catalogs": []} | ||
| if config_path.exists(): | ||
| raw = yaml.safe_load(config_path.read_text(encoding="utf-8")) | ||
| if not isinstance(raw, dict): | ||
| raise StepValidationError( | ||
| "Catalog config file is corrupted (expected a mapping)." | ||
| ) | ||
| data = raw | ||
|
|
|
@copilot apply changes based on the comments in this thread |
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>
…le 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>
Applied all changes from the review thread in commits
Three new tests cover these cases: malformed |
| spec = _importlib_util.spec_from_file_location( | ||
| f"_speckit_custom_step_{type_key}", init_py | ||
| ) | ||
| if spec is None or spec.loader is None: | ||
| continue |
| except ValueError: | ||
| console.print(f"[red]Error:[/red] Invalid step id '{step_id}'") | ||
| raise typer.Exit(1) | ||
|
|
|
@copilot apply changes based on the comments in this thread |
Applied both changes in commits
|
|
@copilot apply changes based on the comments in this thread |
Applied all three changes from the review thread:
Commits: |
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (3)
src/specify_cli/init.py:6034
- The current path-traversal guard allows
step_idvalues that contain path separators (e.g.foo/bar), which would install into nested directories under.specify/workflows/steps/while still registering the literalstep_idstring. It’s safer to enforce thatstep_idis a single path segment (no/or\\) and reject anything wherePath(step_id).name != step_id(or similar).
steps_base_dir = (project_root / ".specify" / "workflows" / "steps").resolve()
step_dir = (steps_base_dir / step_id).resolve()
try:
step_dir.relative_to(steps_base_dir)
except ValueError:
src/specify_cli/init.py:6215
workflow_step_removewill callshutil.rmtree(step_dir)for any existing path. If the user passes a reserved/internal id likestep-registry.json,step_diris a file (not a directory) andrmtreewill raise, causing an unhandled exception; similarly,.cachecan be deleted accidentally. Consider applying the same reserved-name / dotfile checks asworkflow_step_add, and/or gating the delete withstep_dir.is_dir()(and handling file paths explicitly).
if dir_exists:
import shutil
shutil.rmtree(step_dir)
src/specify_cli/workflows/catalog.py:1026
StepCatalog.add_catalog()computesmax_priorityusing rawcat.get('priority', 0)values. If an existingstep-catalogs.ymlcontains non-int priorities (e.g. quoted strings),max()can raiseTypeErrorwhen comparing mixed types. Consider normalizing priorities withint(...)(ignoring/handling invalid values) before computing the max.
max_priority = max(
(cat.get("priority", 0) for cat in catalogs if isinstance(cat, dict)),
default=0,
)
- Files reviewed: 6/6 changed files
- Comments generated: 2
| from urllib.parse import urlparse | ||
| from urllib.request import urlopen | ||
|
|
| from urllib.parse import urlparse | ||
| from urllib.request import urlopen | ||
|
|
|
@copilot apply changes based on the comments in this thread |
…url for auth consistency
Applied in commit
|
The workflow engine shipped with a dynamic
STEP_REGISTRYbut no distribution mechanism for community-authored step types. This adds a full catalog system for discovering, installing, and managing custom step types, following the same patterns as the workflow/extension catalogs.New classes (
workflows/catalog.py)StepRegistry— persists installed custom steps in.specify/workflows/steps/step-registry.json; validates JSON shape on load and falls back to a clean default on corruption or unreadable filesStepCatalog— multi-source catalog stack with SHA256-based caching; resolvesSPECKIT_STEP_CATALOG_URLenv var →.specify/step-catalogs.yml→~/.specify/step-catalogs.yml→ built-in defaults (step-catalog.json+step-catalog.community.json)StepCatalogError/StepValidationError/StepCatalogEntrysupporting typesDynamic step loading (
workflows/__init__.py)load_custom_steps(project_root)scans.specify/workflows/steps/, dynamically imports each package's__init__.py, finds theStepBasesubclass matching the declaredtype_key, and registers it intoSTEP_REGISTRY. Each step directory is imported as a proper Python package (withsubmodule_search_locationsset and pre-registered insys.modules) so that relative imports within a step package (e.g.from .helpers import ...) work correctly.type_keyvalues are sanitized to valid Python identifiers and suffixed with an 8-char SHA-256 hash of the originaltype_key(e.g.test-custom→_speckit_custom_step_test_custom_<hash>) — the hash suffix makes module names collision-resistant when differenttype_keyvalues produce the same sanitized form (e.g."a-b"and"a_b"). Broken packages are silently skipped. Dynamic imports are intentionally deferred to workflow execution time —load_custom_steps()is called in theworkflow runandworkflow resumeCLI entrypoints before engine validation/execution so installed step types are usable. CLI metadata commands (list,info) read fromStepRegistryandstep.ymlonly and never execute installed step code.CLI surface (
specify workflow step …)addvalidates that the downloadedstep.yml'stype_keymatches the catalog ID and that all fetches use HTTPS before writing anything to disk. Catalog entries may include an optionalextra_filesdict (relative-path → URL) to install additional package files (e.g. helper modules) alongsidestep.ymland__init__.py— each extra file is HTTPS-validated, path-traversal-checked against the resolved staging directory, and downloaded before the atomic rename. Downloads happen in a staging directory understeps_base_dirand are moved into place atomically viaos.rename()only after all validation passes — a transient failure never corrupts or deletes any pre-existing data, and install is refused if the target directory already exists.removeallows cleaning up an orphaned step directory (e.g. after a registry reset due to corruption) even when the registry entry is missing — it prints a warning and removes the directory so a fresh install can be attempted. It still errors if neither the directory nor the registry entry exists.Catalog files
workflows/step-catalog.json— official catalog (empty, ready for entries)workflows/step-catalog.community.json— community catalog (empty, ready for entries)Robustness & security hardening
step_idis validated against the resolved base directory in bothaddandremovebefore any filesystem operation.extra_filespaths are validated by resolving both the destination and the staging base directory beforerelative_to(), correctly handling symlinks.step addrejects IDs that collide with internal names under.specify/workflows/steps/(dotfiles,.cache,step-registry.json) to prevent corrupting cache or registry state.step addrejects IDs that collide with built-in or already-installed step types.step adddownloads and validates in a staging temp dir understeps_base_dir(same filesystem as the destination), then atomically renames into place viaos.rename(); install is refused if the target directory already exists.parsed.hostname(not justparsed.netlocor scheme alone) to correctly reject hostless URLs such ashttps://:80/path— enforced in_validate_catalog_url(),_safe_fetch(), and the fetch-time_validate_url()helpers in bothWorkflowCatalogandStepCatalog._safe_fetch()(used byworkflow step add) andStepCatalog._fetch_single_catalog()route throughspecify_cli.authentication.http.open_url()to support auth.json-based credentials and safe redirect auth stripping, consistent with all other network fetches in the CLI.try/except OSErrorso a read-only filesystem never aborts a successful HTTP fetch.StepRegistry._load()validates the on-disk JSON shape and catchesOSError/UnicodeError, falling back to a clean default on any read failure.StepCatalog.add_catalog()andremove_catalog()wrapyaml.safe_load()intry/except (yaml.YAMLError, OSError, UnicodeDecodeError)so a malformed or unreadable.specify/step-catalogs.ymlraises a cleanStepValidationErrorinstead of an unhandled exception.StepCatalog.add_catalog()treats an empty.specify/step-catalogs.ymlas an empty mapping rather than a corruption error.extra_filestolerance: A warning is printed when a catalog entry'sextra_filesfield is present but not a mapping, and the field is safely ignored.Tests
35+ tests across
TestStepRegistryCustom,TestStepCatalog,TestLoadCustomSteps, andTestWorkflowStepRemoveCLIcovering CRUD, catalog resolution (env var / project / user / default), URL validation, search, dynamic loading edge cases, registry shape validation, unreadable-file fallback, empty YAML initialization, module name sanitization and collision-resistance for hyphenated type keys, package-style relative imports within custom step packages, andstep removeCLI behavior (registered, orphaned, and fully-missing cases). Thechmod-based unreadability test is skipped on Windows.