feat(workflows): align workflow CLI with extension command surface#3419
feat(workflows): align workflow CLI with extension command surface#3419marcelsafin wants to merge 24 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aligns the specify workflow CLI with the established extension/preset command surface, adding missing flags and lifecycle commands so workflows can be installed from dev paths/URLs, searched by author, updated to newer catalog versions, and toggled enabled/disabled without removal.
Changes:
- Added
workflow add --dev <path>andworkflow add <id> --from <url>(with ID mismatch enforcement for--frominstalls). - Added
workflow update [id]to update catalog-installed workflows (with confirmation + backup/restore on failure). - Added
workflow enable/disable <id>and enforced disabled workflows inworkflow runandworkflow listUI.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/specify_cli/workflows/_commands.py |
Implements the aligned CLI surface (add --dev/--from, update, enable/disable) and enforces disabled workflows at run/list time. |
src/specify_cli/workflows/catalog.py |
Extends catalog search to support exact (case-insensitive) --author filtering. |
tests/test_workflows.py |
Adds a dedicated test suite covering the new CLI behaviors and edge cases described in #2342. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not is_file_source: | ||
| from .catalog import WorkflowRegistry | ||
|
|
||
| installed_meta = WorkflowRegistry(project_root).get(source) | ||
| if installed_meta is not None and installed_meta.get("enabled", True) is False: |
There was a problem hiding this comment.
Done — the run guard now checks isinstance(installed_meta, dict), so a corrupted entry no longer crashes (and doesn't block the run, matching the pre-existing behavior of having no check).
| metadata = installed.get(wf_id) or {} | ||
| if metadata.get("source") != "catalog": | ||
| console.print(f"⚠ {safe_id}: Installed from a local path or URL — re-add to update (skipping)") | ||
| continue |
There was a problem hiding this comment.
Done — workflow update now skips non-dict registry entries with a "Registry entry is corrupted (skipping)" warning. Test added.
| registry.add(workflow_id, { | ||
| "name": definition.name or info.get("name", workflow_id), | ||
| "version": definition.version or info.get("version", "0.0.0"), | ||
| "description": definition.description or info.get("description", ""), | ||
| "source": "catalog", | ||
| "catalog_name": info.get("_catalog_name", ""), | ||
| "url": workflow_url, | ||
| }) |
There was a problem hiding this comment.
Good catch — fixed in the shared _install_workflow_from_catalog helper: a prior enabled: False is preserved across updates/reinstalls. Regression test added.
| metadata = registry.get(workflow_id) | ||
| if metadata is None: | ||
| console.print(f"[red]Error:[/red] Workflow '{_escape_markup(workflow_id)}' is not installed") | ||
| raise typer.Exit(1) | ||
| if metadata.get("enabled", True): | ||
| console.print(f"[yellow]Workflow '{_escape_markup(workflow_id)}' is already enabled[/yellow]") | ||
| raise typer.Exit(0) |
There was a problem hiding this comment.
Done — workflow enable exits cleanly with a corrupted-entry error for non-dict values. Test covers both enable and disable.
| metadata = registry.get(workflow_id) | ||
| if metadata is None: | ||
| console.print(f"[red]Error:[/red] Workflow '{_escape_markup(workflow_id)}' is not installed") | ||
| raise typer.Exit(1) | ||
| if not metadata.get("enabled", True): | ||
| console.print(f"[yellow]Workflow '{_escape_markup(workflow_id)}' is already disabled[/yellow]") | ||
| raise typer.Exit(0) |
There was a problem hiding this comment.
Done — same guard added to workflow disable.
| for wf_id, wf_data in installed.items(): | ||
| console.print(f" [bold]{wf_data.get('name', wf_id)}[/bold] ({wf_id}) v{wf_data.get('version', '?')}") | ||
| marker = "" if wf_data.get("enabled", True) else " [red]\\[disabled][/red]" | ||
| console.print(f" [bold]{wf_data.get('name', wf_id)}[/bold] ({wf_id}) v{wf_data.get('version', '?')}{marker}") |
There was a problem hiding this comment.
Fixed in a2ef4a0. workflow list now skips non-dict entries with a warning and keeps listing valid ones. Covered by test_list_skips_corrupted_registry_entry.
| except Exception as exc: | ||
| if workflow_dir.exists(): | ||
| import shutil | ||
| shutil.rmtree(workflow_dir, ignore_errors=True) | ||
| console.print(f"[red]Error:[/red] Failed to install workflow '{source}' from catalog: {exc}") | ||
| console.print(f"[red]Error:[/red] Failed to install workflow '{workflow_id}' from catalog: {exc}") | ||
| raise typer.Exit(1) |
There was a problem hiding this comment.
Fixed in a2ef4a0. Added except typer.Exit: raise before the generic handler, matching the --from download path, so the non-HTTPS redirect error is no longer duplicated.
| # Try as URL (http/https) — either the positional source is a URL, or an | ||
| # explicit --from URL names where to fetch it (mirrors `extension add --from`). | ||
| download_url = from_url or ( | ||
| source if source.startswith(("http://", "https://")) else None | ||
| ) |
There was a problem hiding this comment.
Fixed in 4fcfd1a. workflow add <source> --from <url> now runs _validate_workflow_id_or_exit(source) up front, so a URL/path/uppercase source fails without touching the network. Regression covered by test_add_from_rejects_invalid_source_id_without_fetch.
| console.print( | ||
| f"[red]Error:[/red] Workflow ID in YAML ({definition.id!r}) " | ||
| f"does not match the requested workflow ID ({expected_id!r})." | ||
| ) |
There was a problem hiding this comment.
Fixed in 4fcfd1a. Both repr() values now go through rich.markup.escape so a bracketed typo can't be parsed as markup.
| console.print( | ||
| f"[red]Error:[/red] Workflow ID in YAML ({definition.id!r}) " | ||
| f"does not match catalog key ({source!r}). " | ||
| f"does not match catalog key ({workflow_id!r}). " | ||
| f"The catalog entry may be misconfigured." | ||
| ) |
There was a problem hiding this comment.
Fixed in 4fcfd1a. Same treatment for the catalog-mismatch message: both repr() values wrapped in _escape_markup.
| if not isinstance(wf_data, dict): | ||
| console.print(f" [yellow]Warning:[/yellow] Skipping corrupted registry entry '{wf_id}'.\n") | ||
| continue | ||
| marker = "" if wf_data.get("enabled", True) else " [red]\\[disabled][/red]" | ||
| console.print(f" [bold]{wf_data.get('name', wf_id)}[/bold] ({wf_id}) v{wf_data.get('version', '?')}{marker}") | ||
| desc = wf_data.get("description", "") | ||
| if desc: | ||
| console.print(f" {desc}") |
There was a problem hiding this comment.
Fixed in 3225b4e. workflow list now routes id, name, version and description through _escape_markup before printing. Regression covered by test_list_escapes_rich_markup_in_registry_fields.
| if not info: | ||
| console.print(f"[red]Error:[/red] Workflow '{source}' not found in catalog") | ||
| console.print(f"[red]Error:[/red] Workflow '{workflow_id}' not found in catalog") | ||
| raise typer.Exit(1) | ||
|
|
||
| if not info.get("_install_allowed", True): | ||
| console.print(f"[yellow]Warning:[/yellow] Workflow '{source}' is from a discovery-only catalog") | ||
| console.print(f"[yellow]Warning:[/yellow] Workflow '{workflow_id}' is from a discovery-only catalog") | ||
| console.print("Direct installation is not enabled for this catalog source.") | ||
| raise typer.Exit(1) | ||
|
|
||
| workflow_url = info.get("url") | ||
| if not workflow_url: | ||
| console.print(f"[red]Error:[/red] Workflow '{source}' does not have an install URL in the catalog") | ||
| console.print(f"[red]Error:[/red] Workflow '{workflow_id}' does not have an install URL in the catalog") | ||
| raise typer.Exit(1) |
There was a problem hiding this comment.
Fixed in 3225b4e. _install_workflow_from_catalog now computes safe_wf_id = _escape_markup(workflow_id) once and uses it for every error path (not-found, discovery-only, missing URL, bad scheme, redirect, generic failure).
| for update in updates_available: | ||
| # Installed workflows are a single workflow.yml — back it up so a | ||
| # failed download/validation doesn't destroy the working copy. | ||
| wf_dir = _safe_workflow_id_dir(workflows_dir, update["id"]) | ||
| wf_file = wf_dir / "workflow.yml" | ||
| backup = wf_file.read_bytes() if wf_file.is_file() else None | ||
| try: | ||
| _install_workflow_from_catalog(project_root, registry, workflows_dir, update["id"]) | ||
| except typer.Exit: | ||
| if backup is not None: | ||
| wf_dir.mkdir(parents=True, exist_ok=True) | ||
| wf_file.write_bytes(backup) | ||
| failed.append(update["id"]) |
There was a problem hiding this comment.
Fixed in 3225b4e. _safe_workflow_id_dir and the backup read are now inside the per-workflow try/except typer.Exit, so an unsafe id in a corrupted registry fails that one entry and the loop continues. Regression covered by test_update_reports_unsafe_registry_id_per_workflow.
| @@ -684,7 +742,13 @@ def _validate_and_install_local(yaml_path: Path, source_label: str) -> None: | |||
| console.print(f"[red]Error:[/red] Failed to download workflow: {exc}") | |||
There was a problem hiding this comment.
Fixed. The download exception is now wrapped in _escape_markup(str(exc)), matching the catalog install path.
| try: | ||
| wf_dir = _safe_workflow_id_dir(workflows_dir, update["id"]) | ||
| wf_file = wf_dir / "workflow.yml" | ||
| backup = wf_file.read_bytes() if wf_file.is_file() else None | ||
| _install_workflow_from_catalog(project_root, registry, workflows_dir, update["id"]) | ||
| except typer.Exit: | ||
| if backup is not None and wf_dir is not None and wf_file is not None: | ||
| wf_dir.mkdir(parents=True, exist_ok=True) | ||
| wf_file.write_bytes(backup) | ||
| failed.append(update["id"]) |
There was a problem hiding this comment.
Fixed in d0de7e0. The per-workflow loop now catches (typer.Exit, OSError), the restore write is wrapped in its own try/except so a failed restore only warns, and OSError paths report through the existing 'Failed to update' summary. Regression covered by test_update_survives_oserror_from_backup_read.
| catalog = WorkflowCatalog(project_root) | ||
|
|
||
| try: | ||
| results = catalog.search(query=query, tag=tag) | ||
| results = catalog.search(query=query, tag=tag, author=author) | ||
| except WorkflowCatalogError as exc: |
There was a problem hiding this comment.
Fixed. workflow search now escapes name, id, version, description and tags before printing, matching extension search and the workflow list fix. Regression covered by test_search_escapes_rich_markup_in_catalog_fields.
| console.print("[red]Error:[/red] Workflow validation failed:") | ||
| for err in errors: | ||
| console.print(f" \u2022 {err}") | ||
| raise typer.Exit(1) |
There was a problem hiding this comment.
Fixed in 3bf431f. Validation errors are now escaped before printing in workflow add, and the same fix went into workflow run's validation output since it prints the same strings. Regression test added: a workflow with version: "[bold]bad[/bold]" fails add with the literal value in the output.
| console.print("[red]Error:[/red] Downloaded workflow validation failed:") | ||
| for err in errors: | ||
| console.print(f" \u2022 {err}") | ||
| raise typer.Exit(1) |
There was a problem hiding this comment.
Fixed in 3bf431f. _install_workflow_from_catalog now escapes each validation error before printing.
mnriem
left a comment
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback
| try: | ||
| definition = WorkflowDefinition.from_yaml(yaml_path) |
| final_url = resp.geturl() | ||
| final_parsed = urlparse(final_url) | ||
| final_host = final_parsed.hostname or "" |
| return | ||
|
|
Adds the missing workflow commands and flags so the workflow CLI matches the extension/preset pattern: add --dev and --from, search --author, update, enable and disable. Disabled workflows are blocked from running and marked in list output. Fixes github#2342 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| try: | ||
| parsed_url = urlparse(workflow_url) | ||
| url_host = parsed_url.hostname or "" | ||
| except ValueError: | ||
| console.print( | ||
| f"[red]Error:[/red] Workflow '{safe_wf_id}' has a malformed install URL." | ||
| ) | ||
| raise typer.Exit(1) |
| metadata["enabled"] = True | ||
| registry.add(workflow_id, metadata) |
| metadata["enabled"] = False | ||
| registry.add(workflow_id, metadata) |
…rollback intact - A truthy non-string catalog url (e.g. 123) reached urlparse and raised AttributeError, escaping the clean error path; validate it is a string. - enable/disable mutated the live registry entry before add(), so add's rollback snapshot captured the already-toggled object; pass a fresh mapping instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| existing = self.data["workflows"].get(workflow_id, {}) | ||
| had_entry = workflow_id in self.data["workflows"] | ||
| metadata["installed_at"] = existing.get( | ||
| "installed_at", datetime.now(timezone.utc).isoformat() | ||
| ) | ||
| metadata["updated_at"] = datetime.now(timezone.utc).isoformat() |
| assert registry.get("other-wf") is None | ||
|
|
||
| def test_run_refuses_falsy_non_bool_enabled(self, project_dir, monkeypatch): | ||
| """"enabled": 0 shows as disabled in list — run must agree.""" |
| assert result.exit_code == 0, result.output | ||
|
|
||
| def test_disable_blocks_run_via_path_equivalent_id(self, project_dir, monkeypatch): | ||
| """"align-wf/" must not run a disabled workflow by dodging the registry lookup.""" |
… test docstrings A corrupted-but-parseable registry entry (e.g. a string value) crashed WorkflowRegistry.add with AttributeError on existing.get. Guard the non-dict case while still restoring the original raw value on rollback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| try: | ||
| self.save() | ||
| except OSError: | ||
| # Roll back the in-memory mutation so a later successful save | ||
| # cannot persist metadata for a write that failed. | ||
| if had_entry: | ||
| self.data["workflows"][workflow_id] = raw_existing | ||
| else: | ||
| del self.data["workflows"][workflow_id] |
| if not updates_available: | ||
| if checked: | ||
| console.print("\n[green]All workflows are up to date![/green]") | ||
| else: | ||
| console.print("\n[yellow]No workflows were eligible for update[/yellow]") |
… summary
- save() wrote the registry with open('w'), so a failed dump truncated
the file and the next load reset every entry. Write to a sibling temp
file and os.replace into place.
- workflow update no longer claims all workflows are up to date when
some targets were skipped; it reports checked-only status with a
skipped count.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| with open(self.registry_path, "w", encoding="utf-8") as f: | ||
| json.dump(self.data, f, indent=2) | ||
| # Write-then-replace so a failed dump cannot truncate the registry. | ||
| tmp_path = self.registry_path.with_name(self.registry_path.name + ".tmp") |
| workflows_root = (project_root / ".specify" / "workflows").resolve() | ||
| resolved = source_path.resolve() | ||
| if resolved.is_relative_to(workflows_root): |
…dent disabled guard - save() now uses tempfile.mkstemp in the workflows dir (matching the engine's atomic writer), so a pre-created symlink at a predictable .tmp path cannot redirect the write and concurrent processes cannot collide. - The direct-path disabled guard derives the owning project from the resolved file path instead of the caller's cwd, so running an installed workflow's YAML from outside the project still refuses when disabled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| self.workflows_dir.mkdir(parents=True, exist_ok=True) | ||
| with open(self.registry_path, "w", encoding="utf-8") as f: | ||
| json.dump(self.data, f, indent=2) | ||
| # Unique, exclusive temp then replace: a failed dump cannot truncate | ||
| # the registry, a pre-created symlink cannot redirect the write, and | ||
| # concurrent CLI processes cannot collide on the same temp path. | ||
| fd, tmp = tempfile.mkstemp( |
| raw_existing = self.data["workflows"].get(workflow_id) | ||
| had_entry = workflow_id in self.data["workflows"] | ||
| # Corrupted-but-parseable registries may hold non-dict entries. | ||
| existing = raw_existing if isinstance(raw_existing, dict) else {} |
| dev_wf_file = dev_path / "workflow.yml" | ||
| if not dev_wf_file.exists(): | ||
| console.print(f"[red]Error:[/red] No workflow.yml found in {_escape_markup(source)}") | ||
| raise typer.Exit(1) |
…try, dev-dir file check - WorkflowRegistry now mirrors StepRegistry: _load refuses symlinked parents/registry file and normalizes a non-dict workflows field; save() rejects symlinked paths before writing. - workflow add --dev requires workflow.yml to be a regular file so a directory named workflow.yml gets the documented CLI error instead of an uncaught IsADirectoryError. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| import tempfile | ||
| try: | ||
| with _open_url(source, timeout=30, extra_headers=_wf_url_extra_headers) as resp: | ||
| with _open_url(download_url, timeout=30, extra_headers=_wf_url_extra_headers) as resp: |
All three workflow download sites (add --from, catalog install, step install) passed no redirect_validator to open_url, so an HTTPS URL redirecting to cleartext HTTP issued the insecure request before the post-hoc geturl() check reported it. Shared validator now rejects non-HTTPS redirects (loopback HTTP allowed) pre-follow, matching the preset download path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| self.data = self._load() | ||
|
|
||
| def _has_symlinked_parent(self) -> bool: | ||
| """Return True if any directory under .specify/workflows is a symlink.""" |
| | Option | Description | | ||
| | -------- | ------------------------------------------------------ | | ||
| | `--dev` | Install from a local workflow YAML file or directory | | ||
| | `--from` | Install from a custom URL (`<source>` names the expected workflow ID) | |
… fakes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| if not isinstance(data.get("workflows"), dict): | ||
| data["workflows"] = {} | ||
| return data | ||
| except (json.JSONDecodeError, ValueError, OSError, UnicodeError): |
Fixes #2342
Root cause
The workflow CLI grew after the extension CLI and never picked up the full command surface.
workflow addonly accepted catalog IDs, URLs and plain file paths, and there was no way to update an installed workflow, filter search by author, or disable a workflow without removing it.Change
Mirrors the extension/preset commands, same flag names and behavior:
workflow add --dev <path>installs from a local directory or YAML file for development.workflow add <id> --from <url>installs from an explicit URL and fails if the downloaded workflow ID does not match, so nothing is registered under a wrong name.workflow search --author <name>filters catalog results by author (case-insensitive exact match, same as extensions).workflow update [id]updates catalog-installed workflows when a newer version exists, with a confirm prompt. Local and URL installs are skipped with a hint to re-add. The previousworkflow.ymlis backed up and restored if the download fails.workflow enable/disable <id>toggles anenabledflag in the registry.workflow runrefuses disabled workflows andworkflow listmarks them[disabled].Left out
set-priority(no priority concept for workflows) andsearch --verified(catalog has no verification data), per the issue discussion.The catalog install block in
workflow_addmoved verbatim into a module-level_install_workflow_from_cataloghelper soupdatereuses the exact same download/validate/register path instead of duplicating it.Testing
17 new tests in
TestWorkflowCliAlignmentcover: dev install from directory and file, missing path and missing workflow.yml errors, URL install, ID mismatch rejection, author filtering, update with no workflows, unknown ID, non-catalog skip, newer-version update, up-to-date short-circuit, backup restore on failed download, disable blocking run, enable restoring, list marker, unknown-ID errors and idempotent enable/disable warnings.Full suite: 3851 passed, 107 skipped.
ruff checkclean.