feat(workflows): align workflow CLI with extension command surface#3419
Open
marcelsafin wants to merge 42 commits into
Open
feat(workflows): align workflow CLI with extension command surface#3419marcelsafin wants to merge 42 commits into
marcelsafin wants to merge 42 commits into
Conversation
Contributor
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.
Comment on lines
+352
to
+356
| 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: |
Contributor
Author
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).
Comment on lines
+1004
to
+1007
| 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 |
Contributor
Author
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.
Comment on lines
898
to
905
| 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, | ||
| }) |
Contributor
Author
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.
Comment on lines
+1088
to
+1094
| 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) |
Contributor
Author
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.
Comment on lines
+1109
to
+1115
| 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) |
Contributor
Author
There was a problem hiding this comment.
Done — same guard added to workflow disable.
Comment on lines
+584
to
+586
| 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}") |
Contributor
Author
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.
Comment on lines
861
to
866
| 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) |
Contributor
Author
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.
Comment on lines
+675
to
+679
| # 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 | ||
| ) |
Contributor
Author
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.
Comment on lines
+637
to
+640
| console.print( | ||
| f"[red]Error:[/red] Workflow ID in YAML ({definition.id!r}) " | ||
| f"does not match the requested workflow ID ({expected_id!r})." | ||
| ) |
Contributor
Author
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.
Comment on lines
896
to
900
| 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." | ||
| ) |
Contributor
Author
There was a problem hiding this comment.
Fixed in 4fcfd1a. Same treatment for the catalog-mismatch message: both repr() values wrapped in _escape_markup.
Comment on lines
585
to
592
| 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}") |
Contributor
Author
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.
Comment on lines
792
to
804
| 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) |
Contributor
Author
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).
Comment on lines
+1075
to
+1087
| 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"]) |
Contributor
Author
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}") | |||
Contributor
Author
There was a problem hiding this comment.
Fixed. The download exception is now wrapped in _escape_markup(str(exc)), matching the catalog install path.
Comment on lines
+1086
to
+1095
| 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"]) |
Contributor
Author
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.
Comment on lines
1178
to
1182
| 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: |
Contributor
Author
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.
Comment on lines
638
to
641
| console.print("[red]Error:[/red] Workflow validation failed:") | ||
| for err in errors: | ||
| console.print(f" \u2022 {err}") | ||
| raise typer.Exit(1) |
Contributor
Author
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.
Comment on lines
896
to
899
| console.print("[red]Error:[/red] Downloaded workflow validation failed:") | ||
| for err in errors: | ||
| console.print(f" \u2022 {err}") | ||
| raise typer.Exit(1) |
Contributor
Author
There was a problem hiding this comment.
Fixed in 3bf431f. _install_workflow_from_catalog now escapes each validation error before printing.
mnriem
requested changes
Jul 10, 2026
mnriem
left a comment
mnriem
left a comment
Collaborator
There was a problem hiding this comment.
Please address Copilot feedback
Comment on lines
626
to
627
| try: | ||
| definition = WorkflowDefinition.from_yaml(yaml_path) |
Comment on lines
723
to
725
| final_url = resp.geturl() | ||
| final_parsed = urlparse(final_url) | ||
| final_host = final_parsed.hostname or "" |
Comment on lines
768
to
769
| 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>
Contributor
Author
|
Fixed the latest review findings in 6a127b3, 12d985b, and ce3e0ab: workflow ownership checks are lexical and reject symlinked owning storage, registry read failures now fail closed before side effects, local-copy reinstalls roll back safely, bundled update messaging is accurate, and bundle removal converts workflow-registry failures into clean errors. Full suite: 3979 passed, 110 skipped. Posted on behalf of @marcelsafin by GitHub Copilot (model: GPT-5.6 Sol). @copilot review |
Comment on lines
+90
to
+93
| # Defense-in-depth: refuse to read through symlinked parents or a | ||
| # symlinked registry file (mirrors StepRegistry._load). | ||
| if self._has_symlinked_parent() or self.registry_path.is_symlink(): | ||
| return default_registry |
Comment on lines
+759
to
+763
| if existed_before: | ||
| if backup_bytes is not None: | ||
| dest_file.write_bytes(backup_bytes) | ||
| else: | ||
| shutil.rmtree(dest_dir, ignore_errors=True) |
Comment on lines
+749
to
+752
| existed_before = dest_dir.is_dir() | ||
| backup_bytes = ( | ||
| dest_file.read_bytes() if existed_before and dest_file.is_file() else None | ||
| ) |
Comment on lines
+998
to
+1001
| existed_before = workflow_dir.is_dir() | ||
| prior_workflow_bytes = ( | ||
| workflow_file.read_bytes() if existed_before and workflow_file.is_file() else None | ||
| ) |
Comment on lines
+204
to
+208
| except Exception as exc: # noqa: BLE001 | ||
| raise BundlerError( | ||
| f"Failed to remove bundle '{bundle_id}': {exc}. " | ||
| "No changes were recorded." | ||
| ) from exc |
Comment on lines
+1211
to
+1214
| console.print( | ||
| f"[yellow]Warning:[/yellow] Failed to restore registry entry " | ||
| f"for '{safe_id}' after directory removal failure: {restore_exc}" | ||
| ) |
Comment on lines
1215
to
1217
| console.print( | ||
| f"[red]Error:[/red] Failed to remove workflow directory {workflow_dir}: {exc}" | ||
| ) |
Comment on lines
+1011
to
+1016
| if existed_before: | ||
| if prior_workflow_bytes is not None: | ||
| workflow_file.write_bytes(prior_workflow_bytes) | ||
| else: | ||
| import shutil | ||
| shutil.rmtree(workflow_dir, ignore_errors=True) |
…ck orphans, backup-read boundaries, and Rich escaping 1. WorkflowRegistry._load(): a symlinked .specify/.specify/workflows parent (or a symlinked registry file) silently returned an empty registry instead of raising, unlike an unreadable-file read failure. A read-only caller (notably the bundler's remove path) querying is_installed() before ever writing could conclude an installed workflow is absent, skip removing it, then delete the bundle record -- leaving the workflow untracked but still on disk. Now raises OSError immediately, matching the existing unreadable-file fail-closed behavior. 2/8. _validate_and_install_local and _install_workflow_from_catalog: when the destination directory already existed but had no prior workflow.yml (e.g. a leftover empty dir), existed_before was True but there were no backup bytes to restore, so the rollback closure did nothing on a later failure -- leaving the newly copied/ downloaded file behind. Both now unlink the newly created file in this case, restoring the pre-existing directory to its prior (empty) state. 3/4. Both install paths read the prior workflow.yml bytes (to seed the reinstall rollback) *before* any try/except boundary: a read failure on the existing file (e.g. a transient permission/FS issue) leaked a raw, unescaped OSError instead of the same clean CLI error used by every other failure branch in these functions. Both reads are now guarded by their own try/except OSError, with no writes attempted before the read succeeds (so there is nothing to roll back on this specific failure). 5. remove_bundle's exception-conversion message unconditionally claimed "No changes were recorded," even though a failure can occur after earlier components in the same bundle have already been removed from disk (save_records never runs on this path, so the record is left claiming the bundle fully installed). The message now reports how many components were already removed when that happened, instead of asserting no changes occurred. 6/7. workflow_remove's new post-registry-removal directory-failure error and its restore-failure warning interpolated workflow_dir and the exception values into Rich markup unescaped. A project path or OS error message containing Rich-markup-like brackets could be parsed as markup and hide/corrupt the displayed text. Both now use the existing _escape_markup helper, consistent with every other error path in this file. Tests (tests/test_workflows.py unless noted): - TestWorkflowRegistry::test_load_symlinked_workflows_dir_fails_closed_not_silently_empty (1) - TestWorkflowCliAlignment::test_add_dev_fresh_install_into_preexisting_empty_dir_cleans_new_file (2) - TestWorkflowCliAlignment::test_add_catalog_fresh_install_into_preexisting_empty_dir_cleans_new_file (8) - TestWorkflowCliAlignment::test_add_dev_reinstall_backup_read_failure_gives_clean_error (3) - TestWorkflowCliAlignment::test_add_catalog_reinstall_backup_read_failure_gives_clean_error (4) - tests/integration/test_bundler_install_flow.py::test_remove_partial_failure_message_reflects_partial_state (5) - TestWorkflowRemoveGuard::test_remove_directory_and_restore_failure_escapes_rich_markup (6/7) All seven were confirmed red beforehand, matching each thread's described failure mode exactly (silent empty registry instead of a raise; orphaned new file left behind; raw unescaped OSError leaking; a misleading "no changes were recorded" claim; Rich markup consuming bracketed path/exception text). Also updated test_registry_save_refuses_symlinked_parent, a pre-existing test that asserted the symlinked-parent raise at add()/save() time -- it now raises at construction instead, per fix github#1, so the test was adjusted to match without weakening its guarantee (still asserts no writes occur under the symlinked target). Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Addressed all 8 review findings in 6b593e3: registry loading now fails closed on unsafe symlinks; install rollback/backup paths preserve prior state and handle read failures cleanly; bundle-removal state is accurate; Rich dynamics are escaped. Full suite: 3986 passed, 110 skipped; changed files pass ruff. Posted on behalf of @marcelsafin by GitHub Copilot (model: GPT-5.6 Sol). @copilot review |
Comment on lines
+202
to
+204
| except BundlerError: | ||
| raise | ||
| except Exception as exc: # noqa: BLE001 |
Comment on lines
889
to
890
| with tempfile.NamedTemporaryFile(suffix=".yml", delete=False) as tmp: | ||
| tmp.write(resp.read()) |
| f"[red]Error:[/red] Workflow '{safe_wf_id}' redirected to non-HTTPS URL: {_escape_markup(final_url)}" | ||
| ) | ||
| raise typer.Exit(1) | ||
| workflow_file.write_bytes(response.read()) |
… removal, bounded workflow downloads 1. bundle remove: BundlerError raised by the primitive installer itself (e.g. from a kind manager) bypassed the partial-removal bookkeeping message added previously via a bare `except BundlerError: raise`. Now routes through the same detail-construction logic as generic exceptions, so a mid-loop BundlerError after an earlier successful removal still reports that the project may be partially uninstalled, while a zero-removal BundlerError still reports "No components were removed." Both preserve the original exception message and chain `from exc`. 2/3. workflow add --from and catalog install/update downloads used unbounded `response.read()`, buffering the entire server-controlled body into memory before any size check, and trusted Content-Length alone where checked at all. Added a single shared `_read_response_within_limit()` helper reused by both call sites: it fails fast on an oversized declared Content-Length, and separately enforces the same cap while streaming in 64KiB chunks so a chunked or Content-Length-less response cannot bypass the limit by lying about or omitting its size. Chose 5 MiB as the cap: workflow YAML definitions are small step/metadata text, not binaries, so this is generous headroom against a malicious/misbehaving server without affecting any legitimate workflow definition. Both call sites already route any raised exception through their existing clean-error and rollback (`_cleanup_failed_install`) paths, so no additional error-handling plumbing was needed. Tests: extended the shared `_FakeResponse` test helper (and 5 duplicate per-test FakeResponse classes) to support `.read(amt)` chunked reads with an internal cursor (backward compatible with existing bare `.read()` callers) plus header simulation. Added red-first tests for: BundlerError after partial removal reporting partial state, BundlerError with zero removals reporting no changes, --from oversized-Content-Length rejection, --from oversized-streamed-body-without-Content-Length rejection, and the same two cases for the catalog install path (asserting no orphan directory/registry mutation on rejection). tests/integration/test_bundler_install_flow.py: 17 passed tests/test_workflows.py: 485 passed tests -q: 3992 passed, 110 skipped ruff check: clean on all touched files Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…est assertions workflow_add's --from download path opened a NamedTemporaryFile(delete=False) -- which creates the file on disk immediately -- then wrote the size-limited response body before assigning `tmp_path`. If `_read_response_within_limit` raised (oversized declared Content-Length, or an over-cap streamed body with no/understated Content-Length), the exception propagated out of the `with` block before `tmp_path` was ever set, so the outer except handler had no path to clean up: a 0-byte `.yml` temp file was left behind permanently on every rejected/failed --from download. Fixed by assigning `tmp_path` immediately after the file is opened (before the size-limited read/write), and unlinking it in the except branch when set. Normal post-download cleanup in the existing `finally: tmp_path.unlink(missing_ok=True)` is unchanged. Verified (not assumed) the catalog install path has no equivalent leak: it writes the response bytes directly to `workflow_file` inside `workflow_dir` (no separate temp file), and any read/size-limit failure is already caught by the existing `except Exception: _cleanup_failed_install()` handler, which correctly restores a reinstalled file or removes a freshly-created directory. While investigating, found the previous round's 4 size-limit tests were false positives: `_read_response_within_limit`'s `max_bytes` parameter had its default bound to `_MAX_WORKFLOW_YAML_BYTES` at function-definition time, so monkeypatching the module attribute in tests had no effect on the function's actual behavior -- the tests were passing because the oversized mock bodies failed downstream YAML/id validation instead of the size check. Fixed by resolving `max_bytes` from the module attribute at call time (default `None`, resolved inside the function body) so tests can actually override the effective limit, and strengthened all 4 tests' assertions to match the specific size-limit error text (whitespace-collapsed to tolerate Rich's line-wrapping), so they now prove the real code path fires. Tests: added 2 red-first regression tests (oversized-streamed-body and oversized-Content-Length --from downloads leave no leftover temp file, verified against a scratch tempfile.tempdir), confirmed red (real 0-byte file found) before the fix and green after. Strengthened the pre-existing 4 --from/catalog size-limit tests to assert on the actual error message instead of generic exit-code/non-empty-output checks. tests/test_workflows.py: 487 passed tests -k bundler: 186 passed tests -q: 3994 passed, 110 skipped ruff check: clean on all touched files Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Addressed the 3 latest findings plus download-cleanup follow-up in db45f6c and b8269c8: BundlerError now reports partial removal accurately, both workflow download paths share a streamed 5 MiB cap, failed --from downloads remove their temp file, and tests assert the actual cap path. Full suite: 3994 passed, 110 skipped; changed files pass ruff. Posted on behalf of @marcelsafin by GitHub Copilot (model: GPT-5.6 Sol). @copilot review |
Comment on lines
+1285
to
+1293
| shutil.rmtree(workflow_dir) | ||
| except OSError as exc: | ||
| # The registry removal already succeeded; restore the original | ||
| # entry verbatim so the registry doesn't claim this workflow is | ||
| # uninstalled while its directory is still sitting on disk. | ||
| try: | ||
| if registry_metadata is not None: | ||
| registry.data["workflows"][workflow_id] = registry_metadata | ||
| registry.save() |
Comment on lines
+822
to
+828
| if existed_before: | ||
| if backup_bytes is not None: | ||
| dest_file.write_bytes(backup_bytes) | ||
| else: | ||
| dest_file.unlink(missing_ok=True) | ||
| else: | ||
| shutil.rmtree(dest_dir, ignore_errors=True) |
Comment on lines
+1093
to
+1100
| if existed_before: | ||
| if prior_workflow_bytes is not None: | ||
| workflow_file.write_bytes(prior_workflow_bytes) | ||
| else: | ||
| workflow_file.unlink(missing_ok=True) | ||
| else: | ||
| import shutil | ||
| shutil.rmtree(workflow_dir, ignore_errors=True) |
Comment on lines
+477
to
+479
| # symlinked-parent handling below (it silently substitutes | ||
| # an empty registry instead of raising, so a query against | ||
| # it can't be trusted as a safety signal here). |
Comment on lines
+141
to
+149
| fd, tmp = tempfile.mkstemp( | ||
| dir=str(self.registry_path.parent), | ||
| prefix=f".{self.registry_path.name}.", | ||
| suffix=".tmp", | ||
| ) | ||
| try: | ||
| with os.fdopen(fd, "w", encoding="utf-8") as f: | ||
| json.dump(self.data, f, indent=2) | ||
| os.replace(tmp, self.registry_path) |
Addresses 5 Copilot review findings on HEAD b8269c8, all centered on transaction integrity around workflow install/remove/registry writes, following the atomic_write_json pattern already used in _utils.py: 1. WorkflowRegistry.save() now preserves the existing registry file's mode (e.g. 0640/0644) across a save instead of silently downgrading it to mkstemp's 0600 default; a brand-new registry still gets the secure 0600 default. 2. workflow_remove now stages the install directory out of the way via an atomic rename *before* the registry write, rather than deleting it directly with shutil.rmtree after the registry already claims it removed. This closes a real data-integrity gap: a partially-failed rmtree could no longer leave a damaged directory re-marked "installed" by the old manual restore-after-rmtree-failure code (now deleted -- it's structurally impossible to need it). A registry-write failure renames the staged directory back (guarded, with an explicit warning if the restore-back rename itself fails); a registry-write success is durable, so a later failure to delete the staged directory is now a warning (exit 0), not a contradictory "Error: Failed to remove" (exit 1) that used to claim failure while the registry already recorded success. 3. Local (--dev/--from/plain path) and catalog install/reinstall now write new content to a same-directory staging file and commit it onto the destination workflow.yml via a single atomic swap, instead of writing/downloading directly into the destination file. A prior file (reinstall) is renamed aside rather than overwritten in place, so it can be restored via rename -- never a content rewrite -- if registry.add() subsequently fails; a rollback failure is now explicitly reported as a warning instead of escaping unguarded and masking the original clean error. This also removes the need to read the prior file's bytes into memory before installing (that read-before-write step and its failure mode are now unreachable), and both local and catalog installs share the same four small helpers (_stage_workflow_file / _commit_workflow_file / _discard_staged_workflow_file / _rollback_committed_workflow_file, plus guarded wrappers) rather than duplicating the logic. 4. Updated a stale comment (workflow_run's ownership-guard rationale) that still described WorkflowRegistry._load() as silently substituting an empty registry; it now fails closed by raising OSError, which the comment now states plainly. Tests: rewrote the two workflow_remove tests whose assertions encoded the old (incoherent) rmtree-then-restore contract to instead prove the new stage-then-commit contract (post-registry-success cleanup failure is a warning+exit 0; pre-registry-success stage-restore failure is guarded and escapes markup correctly). Rewrote the local/catalog "backup read failure" tests, which tested a step the new design no longer performs, into "restore-rename failure" tests proving the new guarded rollback boundary. Added registry file-mode preservation tests. All other existing install/remove/reinstall tests (save-failure cleanup, pre-existing-empty-dir handling, early-failure-during- reinstall parametrized cases, Rich markup escaping) continue to pass unmodified against the new implementation. Verified via GraphQL that all 5 threads are current (not outdated/ resolved) before fixing. Full suite: 3996 passed, 110 skipped. Ruff clean on all touched files. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
_commit_workflow_file() renames a prior workflow.yml aside to workflow.yml.bak so it can be restored if registry.add() subsequently fails. Neither the local install/reinstall path nor the catalog install/reinstall path ever cleaned up that backup after a successful registry.add() -- every successful reinstall permanently left a workflow.yml.bak sibling, which later reinstalls would silently overwrite/re-orphan. Add a shared _discard_committed_backup_file() helper, called from both success paths right after registry.add() durably succeeds (and before the final "installed" message, preserving output ordering). A fresh install (backup_file is None) is a no-op. A cleanup failure is reported as a warning (exit 0), not a failure, since the install itself already succeeded -- consistent with workflow_remove's post-commit cleanup warning semantics. Add red-first regression tests proving: (1) successful local reinstall leaves no workflow.yml.bak sibling, (2) successful catalog reinstall leaves no workflow.yml.bak sibling, (3) a cleanup failure on the backup file after a successful reinstall reports a warning and still exits 0 with the registry correctly reflecting the new install. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
_stage_workflow_file() does dest_dir.mkdir(parents=True, exist_ok=True) then tempfile.mkstemp(dir=dest_dir, ...). For a fresh install (no prior directory), if mkdir succeeds but mkstemp then raises (disk full/EMFILE/quota), the exception previously propagated straight past both the local-install and catalog-install call sites without any cleanup, leaving the newly-created empty workflow directory orphaned on disk with no error indicating why. Fix at the shared _stage_workflow_file() boundary instead of duplicating cleanup at each call site: track whether this call created dest_dir: on a mkstemp failure, remove that directory via a guarded rmdir (never a broad rmtree, so any concurrently written content would be left untouched) before re-raising the original OSError unchanged. A pre-existing (reinstall) dest_dir is never touched by this cleanup, and a cleanup failure is reported as its own warning without masking the original error. Add red-first regression tests proving: a fresh local install (--dev, plain local path, --from) and a fresh catalog install both clean up the orphaned directory on a simulated mkstemp failure, and a reinstall over a pre-existing directory is left untouched by the same failure. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Addressed all 5 latest findings plus transaction-cleanup follow-ups in 00465c3, 7c6bc90, and 915caf8: workflow files/directories now use atomic staging and guarded rollback, registry saves preserve existing mode, successful reinstalls discard backups, and failed staging leaves no workflow directory. Full suite: 4004 passed, 110 skipped; changed files pass ruff. Posted on behalf of @marcelsafin by GitHub Copilot (model: GPT-5.6 Sol). @copilot review |
Comment on lines
+591
to
+594
| for i in range(len(parts) - 2): | ||
| if parts[i] == ".specify" and parts[i + 1] == "workflows": | ||
| registry_root = Path(*parts[:i]) if i else Path(lexical.anchor or ".") | ||
| registered_id = parts[i + 2] |
Comment on lines
1096
to
1097
| finally: | ||
| tmp_path.unlink(missing_ok=True) |
Comment on lines
+1575
to
+1578
| if backup is not None and wf_dir is not None and wf_file is not None: | ||
| try: | ||
| wf_dir.mkdir(parents=True, exist_ok=True) | ||
| wf_file.write_bytes(backup) |
Comment on lines
+1650
to
+1652
| try: | ||
| registry.add(workflow_id, {**metadata, "enabled": False}) | ||
| except OSError as exc: |
| WorkflowRegistry(project_dir) | ||
| assert not (outside / "workflows").exists() | ||
|
|
||
| def test_registry_save_preserves_existing_file_mode(self, project_dir): |
| mode = stat.S_IMODE(registry.registry_path.stat().st_mode) | ||
| assert mode == 0o644, f"expected 0644, got {oct(mode)}" | ||
|
|
||
| def test_registry_save_on_new_registry_uses_secure_default_mode(self, project_dir): |
|
|
||
| assert seen["validator"] is _reject_insecure_download_redirect | ||
|
|
||
| def test_registry_save_failure_preserves_file_on_disk(self, project_dir, monkeypatch): |
Comment on lines
+209
to
+210
| else: | ||
| result.skipped.append(component) | ||
| detail = "No components were removed." |
| # out of .specify/workflows, fail to find an owner, and let | ||
| # engine.load_workflow below run the symlink target unchecked -- | ||
| # silently bypassing a disabled workflow's guard. | ||
| lexical = Path(os.path.normpath(str(source_path.absolute()))) |
Address 3 current Copilot review findings on the disabled-workflow guard in `workflow run`/`workflow resume`: - The lexical `.specify/workflows/<id>` ownership scan stopped at the first match scanning from the start of the path. A nested project living beneath an outer installed workflow's own directory tree (reusing the same segment names) was attributed to the wrong (outer) workflow and ID, gating the run on an unrelated workflow's disabled state. `_scan_for_workflow_owner` now scans from the end so the nearest (innermost) owner always wins. - A path with no `.specify/workflows` segments of its own (e.g. `/tmp/alias.yml`) that is itself a symlink resolving *into* installed storage bypassed the disabled check entirely, since only the raw lexical path was inspected. `_resolve_installed_workflow_ownership` now additionally resolves the real path when the lexical scan finds no owner and re-runs the same scan against it, so an outward-pointing alias into a disabled workflow is caught too. Genuinely standalone external files (no symlink anywhere on the path) are unaffected. - `workflow resume` bypassed the disabled check altogether: engine.resume() replays a persisted run directly from disk with no registry awareness. RunState now optionally persists `installed_workflow_id` and `installed_registry_root` at run start (set by workflow_run when the source resolved to an installed ID); `workflow_resume` pre-loads the run state and re-checks the registry's *current* disabled state before calling engine.resume(), mirroring workflow_run's own guard. Both new fields default to None via RunState.load()'s `.get()`, so runs from a direct/non-installed source, and any run persisted before this schema addition, resume exactly as before. The ownership-mapping logic (previously inlined in workflow_run) is extracted into `_resolve_installed_workflow_ownership` / `_scan_for_workflow_owner` so both the lexical and resolved-path cases share the same scan and the existing inward-symlink-component refusal. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-only tests Two more current Copilot review findings, both in workflow_add/update: - `workflow add --from`'s `finally: tmp_path.unlink(missing_ok=True)` ran unguarded after `_validate_and_install_local` had already committed the file and registry entry (success) or already raised its own clean `typer.Exit` (failure). An OSError from that cleanup unlink would surface as an unhandled failure even though the install itself succeeded. It is now wrapped in try/except OSError, printing a neutral warning that doesn't claim success or failure (the finally runs on both outcomes) instead of propagating. - `workflow_update`'s per-item loop performed its own outer backup (`wf_file.read_bytes()`) and restore (`wf_file.write_bytes(backup)`) around `_install_workflow_from_catalog`, which is itself fully transactional (staged download, atomic rename-based commit, its own rollback on registry failure) and never leaves a raw OSError or a partially-written workflow.yml. The outer restore was therefore dead weight for its stated purpose, and — being an unguarded byte-level write — was itself an unnecessary place a second failure could truncate an already-safely-preserved file. Removed; the loop now only records success/failure. Also marks 3 registry-save file-mode tests (`test_registry_save_preserves_existing_file_mode`, `test_registry_save_on_new_registry_uses_secure_default_mode`, `test_registry_save_failure_preserves_file_on_disk`) as POSIX-only via the repo's existing `skipif(sys.platform == "win32", ...)` pattern, since they assert exact POSIX permission bits that don't hold on Windows. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The final Copilot review finding: `remove_bundle`'s zero-removed-components
error message claimed "No components were removed." even when the failing
installer component may have deleted files before raising -- prior review
rounds already established that DefaultPrimitiveInstaller's removal paths
are not atomic and can leave partial filesystem changes despite raising
before `result.uninstalled` is populated. The zero-count message is now a
conservative caution ("...but the failing component may have made partial
changes before raising, so the project may be partially uninstalled.")
instead of an unconditional claim of no side effects. The >0-removed path
(which already reports the confirmed partial list) is unchanged.
Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
RunState.installed_registry_root previously persisted the creation-time absolute project path unconditionally whenever a run belonged to an installed workflow. After the whole project directory was renamed or moved, workflow_resume would open a WorkflowRegistry at that now nonexistent path, get back an empty/default registry, and silently skip the disabled-workflow check -- a paused run for a disabled workflow could be resumed successfully from the new location. Fix persists installed_registry_root only when the owning root genuinely differs from the current project_root (true cross-project direct-file- source invocations). The common same-project case now persists None and is re-derived from the live project_root at resume time via a new _resolve_run_owner_root() helper, which also falls back to project_root if a stored root no longer exists on disk -- covering both the common case transparently surviving project moves and the cross-project case degrading safely if its owner project vanishes, rather than silently skipping the disabled check. Backward compatible: state files missing the new fields, and states with a still-existing distinct cross-project root, behave unchanged. Added regression tests: - resume blocked after project moved then disabled at new location - resume still works after project moved while workflow stays enabled - cross-project registry root is still correctly honored when it exists - resume falls back to current project's registry when a stored cross-project root no longer exists Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Addressed all 9 latest findings plus the moved-project resume follow-up in 792450e, 800b962, 616f727, and 4f24735: installed ownership now handles nested/symlink paths, disabled state is enforced on resume with move-safe run metadata, post-commit cleanup and update rollback are coherent, POSIX mode tests skip Windows, and bundle failure wording is conservative. Full suite: 4014 passed, 110 skipped; changed files pass ruff. Posted on behalf of @marcelsafin by GitHub Copilot (model: GPT-5.6 Sol). @copilot review |
| staged_file.unlink(missing_ok=True) | ||
| if not existed_before: | ||
| import shutil | ||
| shutil.rmtree(dest_dir, ignore_errors=True) |
| dest_file.unlink(missing_ok=True) | ||
| else: | ||
| import shutil | ||
| shutil.rmtree(dest_dir, ignore_errors=True) |
Comment on lines
+1227
to
+1229
| if tmp_path is not None: | ||
| tmp_path.unlink(missing_ok=True) | ||
| console.print(f"[red]Error:[/red] Failed to download workflow: {_escape_markup(str(exc))}") |
Comment on lines
+575
to
+576
| installed_workflow_id=state_data.get("installed_workflow_id"), | ||
| installed_registry_root=state_data.get("installed_registry_root"), |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.