feat(workflows): align CLI commands with extension/preset pattern#2353
feat(workflows): align CLI commands with extension/preset pattern#2353mnriem wants to merge 9 commits intogithub:mainfrom
Conversation
Add missing workflow CLI commands and flags to match the capabilities already available in `specify extension` and `specify preset`: - `add --dev`: install from local directory for development - `add --from <url>`: install from custom URL - `search --author`: filter search results by author - `update`: update installed workflow(s) to latest version - `enable` / `disable`: toggle workflows without removing them Also adds `WorkflowRegistry.update()` method and shows disabled status in `workflow list` output. Closes github#2342
There was a problem hiding this comment.
Pull request overview
Aligns specify workflow CLI behavior with the established specify extension / specify preset command patterns by adding comparable install/update/toggle features and extending catalog search capabilities.
Changes:
- Adds
workflow update,workflow enable, andworkflow disablecommands, plus disabled-status display inworkflow list. - Extends
workflow addwith--dev(local install) and--from(custom URL install). - Adds
WorkflowRegistry.update()for partial metadata updates and extendsWorkflowCatalog.search()with anauthorfilter (with tests).
Show a summary per file
| File | Description |
|---|---|
tests/test_workflows.py |
Adds tests for registry partial updates and catalog author-filtered search. |
src/specify_cli/workflows/catalog.py |
Introduces WorkflowRegistry.update() and WorkflowCatalog.search(author=...). |
src/specify_cli/__init__.py |
Adds new workflow CLI commands/flags and update/enable/disable behaviors. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 7
- Defensive author field handling in WorkflowCatalog.search(): coerce non-string author values to empty string before comparison - Make 'source' argument optional in workflow add when --from is used - Fix type hint: workflow_id in update command is Optional[str] - Add workflow ID mismatch check after download in workflow update - Use temp dir for atomic update: download+validate in staging dir, then swap into place (no partial state on failure) - WorkflowRegistry.update() now sets updated_at timestamp and preserves installed_at - Add 9 CLI integration tests via CliRunner (enable/disable roundtrip, update error paths, add validation, list disabled status)
There was a problem hiding this comment.
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Add mutual exclusion: --dev cannot be combined with --from, and --dev requires a positional source path - Atomic swap with backup/rollback in workflow update: rename old dir to .bak before moving staged dir in; restore on failure - Exit 1 if any workflow update fails (track had_failures) - Add CLI tests for --dev/--from mutual exclusion
There was a problem hiding this comment.
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 4
- Validate registry entries are dicts before accessing fields (skip malformed entries gracefully) - Add path traversal check in workflow update (resolve + relative_to) - Extract shared URL helpers (_is_loopback_host, _validate_url_scheme, _download_validated) to DRY duplicated HTTPS/redirect validation across workflow add (--from, direct URL, catalog) and workflow update - Add success-path CLI test for workflow update with mocked catalog and download (verifies directory swap + registry metadata update)
There was a problem hiding this comment.
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:5493
workflow_disable()assumesregistry.get(workflow_id)returns a dict and immediately callsmetadata.get(...). If the registry entry is corrupted/non-dict, this will raise an AttributeError. Consider validatingmetadatais a dict and returning a clear error instead of crashing.
metadata = registry.get(workflow_id)
if not metadata.get("enabled", True):
console.print(f"[yellow]Workflow '{workflow_id}' is already disabled[/yellow]")
raise typer.Exit(0)
- Files reviewed: 3/3 changed files
- Comments generated: 6
- Guard workflow_list against malformed (non-dict) registry entries - Validate URL has hostname in _validate_url_scheme (reject hostless URLs) - Fix --dev error message: 'Path not found' instead of 'Directory not found' - Ensure workflows_dir exists before creating temp dir in update - Keep backup until after registry.update() succeeds; restore on failure - Guard Registry.update() against non-dict entries (reset to fresh dict)
There was a problem hiding this comment.
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:5511
- workflow disable assumes
registry.get()returns a dict and callsmetadata.get(...). If the workflow registry entry is corrupted (non-dict) but the key exists, this will raise an AttributeError. Add ametadata is None or not isinstance(metadata, dict)check (matching extension disable) and fail with an explicit corrupted-registry error.
metadata = registry.get(workflow_id)
if not metadata.get("enabled", True):
console.print(f"[yellow]Workflow '{workflow_id}' is already disabled[/yellow]")
raise typer.Exit(0)
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Normalize 'workflows' field to dict in _load() (handles corrupted registry where 'workflows' is a list/string/null) - Validate top-level loaded data is a dict before using it - Guard enable/disable commands against non-dict metadata from get()
There was a problem hiding this comment.
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
Defense-in-depth: _download_validated() now calls _validate_url_scheme() on the source URL before urlopen, not just on the final redirected URL.
There was a problem hiding this comment.
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Reorder --dev/--from/source validation so --dev-without-source produces its own error instead of falling through to generic message - Use constant 'wf-update-' tempdir prefix instead of wf_id (avoids issues with path separators in corrupted IDs) - Fix test_add_dev_requires_source to invoke 'add --dev' without source (previously tested mutual exclusion instead)
There was a problem hiding this comment.
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 3
| backup_dir = None | ||
| if wf_dir.exists(): | ||
| backup_dir = wf_dir.with_name(f"{wf_id}.bak") | ||
| if backup_dir.exists(): | ||
| shutil.rmtree(backup_dir) | ||
| wf_dir.rename(backup_dir) | ||
| try: | ||
| shutil.move(str(staged_dir), str(wf_dir)) | ||
| except Exception: |
There was a problem hiding this comment.
workflow update doesn’t handle KeyboardInterrupt/BaseException during the swap/registry update sequence. If the user hits Ctrl+C after the old workflow is renamed to .bak or after the new directory is moved in, the command can leave .bak directories behind or the workflow in a partially updated state. Consider mirroring the extension update pattern: catch KeyboardInterrupt explicitly, restore from backup_dir when present, and ensure backup cleanup happens in a finally only after the update is fully committed.
| self.data["workflows"][workflow_id] = dict(fields) | ||
| self.data["workflows"][workflow_id]["updated_at"] = datetime.now(timezone.utc).isoformat() |
There was a problem hiding this comment.
In WorkflowRegistry.update(), the corrupted-entry repair path (existing not a dict) overwrites the registry entry with only fields + updated_at, dropping installed_at and any other recoverable metadata. Consider preserving installed_at when present, or at least setting a fresh installed_at so downstream code/tests that assume it exists don’t encounter missing fields after an enable/disable/update operation on a corrupted entry.
| self.data["workflows"][workflow_id] = dict(fields) | |
| self.data["workflows"][workflow_id]["updated_at"] = datetime.now(timezone.utc).isoformat() | |
| now = datetime.now(timezone.utc).isoformat() | |
| repaired = dict(fields) | |
| repaired.setdefault("installed_at", now) | |
| repaired["updated_at"] = now | |
| self.data["workflows"][workflow_id] = repaired |
| if not metadata.get("enabled", True): | ||
| console.print(f"[yellow]Workflow '{workflow_id}' is already disabled[/yellow]") | ||
| raise typer.Exit(0) | ||
|
|
||
| registry.update(workflow_id, {"enabled": False}) |
There was a problem hiding this comment.
workflow disable/workflow enable currently only toggles an enabled flag in the workflow registry. WorkflowEngine.load_workflow() and the workflow run command do not consult this flag, so a “disabled” workflow can still be executed by ID. Consider enforcing the disabled state (e.g., block workflow run <id> when registry entry has enabled=false, and/or have WorkflowEngine.load_workflow() check the registry when resolving installed IDs) or adjust the command messaging to make the current behavior explicit.
…uard - Catch BaseException (not just Exception) in update swap/registry to handle KeyboardInterrupt and restore backup on Ctrl+C - Preserve installed_at (or set fresh one) when repairing corrupted non-dict registry entries in WorkflowRegistry.update() - Block 'workflow run' for disabled workflows with clear error message and re-enable hint
Summary
Aligns
specify workflowCLI commands with the capabilities already available inspecify extensionandspecify preset, closing the gaps identified in #2342.Changes
New commands
workflow update [workflow-id]— Update installed workflow(s) to latest catalog version (with backup/rollback)workflow enable <workflow-id>— Re-enable a disabled workflowworkflow disable <workflow-id>— Disable a workflow without removing itNew flags
workflow add --dev— Install from a local directory for developmentworkflow add --from <url>— Install from a custom URLworkflow search --author <name>— Filter search results by author (case-insensitive)Supporting changes
WorkflowRegistry.update()method for partial metadata updatesworkflow listnow shows(disabled)status for disabled workflowsWorkflowCatalog.search()accepts optionalauthorparameterTests
All 130 workflow tests pass, including new tests for:
WorkflowRegistry.update()— update, no-op on nonexistent, enable/disable round-tripWorkflowCatalog.search(author=...)— filter, case-insensitivity, combined with tag, no-matchNot applicable (per issue)
set-priority/add --priority— workflows run by name, no stackingsearch --verified— no verification process for workflowsCloses #2342