From 74e51ff5badbccb3eb9605823563a93a438b6011 Mon Sep 17 00:00:00 2001 From: Markus Date: Thu, 24 Sep 2026 09:03:08 +0200 Subject: [PATCH 01/16] feat(workflows): install custom step types from local dirs and archives (#4695) `specify workflow step add` gains `--dev ` and `--from ` alongside the existing catalog source. All three converge on a new `step/installer.py` domain module that owns package validation (shape, symlink/special-file rejection, 512-file/50 MiB limits), same-filesystem staging with revalidation, atomic commit, `--force` replacement, and source-kind-only registry provenance. Direct URLs require a default-deny trust prompt before any request. Docs document the local-authoring flow and the deferred bundle-local limitation. Assisted-by: opencode (model: deepseek-v4.1-flash, autonomous) --- docs/reference/bundles.md | 2 + docs/reference/workflows.md | 139 ++++ src/specify_cli/workflows/step/_helpers.py | 120 +--- src/specify_cli/workflows/step/command_add.py | 588 +++++++++------- .../workflows/step/command_info.py | 20 + src/specify_cli/workflows/step/installer.py | 612 +++++++++++++++++ .../workflows/step/test_command_add.py | 565 +++++++++++++++- .../workflows/step/test_command_info.py | 44 ++ .../workflows/step/test_installer.py | 626 ++++++++++++++++++ 9 files changed, 2365 insertions(+), 351 deletions(-) create mode 100644 src/specify_cli/workflows/step/installer.py create mode 100644 tests/specify_cli/workflows/step/test_installer.py diff --git a/docs/reference/bundles.md b/docs/reference/bundles.md index 139028a377..f95cc7fdba 100644 --- a/docs/reference/bundles.md +++ b/docs/reference/bundles.md @@ -81,6 +81,8 @@ The source may also be a bundle directory or `.zip` artifact. Refresh uses the s A local bundle source supplies the manifest, not its component payloads. Components resolved through catalogs still require network access to refresh, even when already installed. Add `--offline` only when the components being installed or refreshed ship with Spec Kit; otherwise the command reports which component needs network access. Re-run without `--offline` to fetch that component through its catalog. +> **Step payloads resolve through the step catalog only.** A bundle's `provides.steps` entries still resolve exclusively through the active step catalogs. Bundle-local `steps//` payloads and relative `provides.steps[].source` overrides are **not** resolved in this release, so a step declared that way cannot be installed offline. To ship a step with a bundle today, publish it to a step catalog the bundle's users can reach. + ## Update Bundles ```bash diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index 811ab4ebf4..c063c5ccea 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -543,6 +543,145 @@ specify workflow run speckit -i spec="Build a kanban board with drag-and-drop ta > **Security note:** a `shell` step runs a local command with **your** privileges. There is no capability sandbox — `requires` is an advisory pre-condition block (spec-kit version, integrations), not a runtime gate, so it does **not** restrict what a step can do. In particular there is no `requires.permissions` capability gate: it is rejected by validation precisely because it would imply a sandbox that does not exist. Review any catalog or downloaded workflow before running it, and use a `gate` step to require explicit approval before sensitive or destructive shell commands. +### Custom step packages + +Custom step types are installed with `specify workflow step`. A step is a +directory package containing metadata and executable Python: + +```text +my-step/ +├── step.yml # required, at the package root +├── __init__.py # required, at the package root +└── helpers.py # optional nested modules and data files +``` + +`step.yml` declares the step's identity. `step.type_key` must exactly match the +`` passed on the command line — the ID is never inferred from package +content: + +```yaml +step: + type_key: my-step + name: My Step + version: 0.1.0 + author: you + description: What this step does +``` + +`__init__.py` must define a `StepBase` subclass whose `type_key` matches: + +```python +from specify_cli.workflows.base import StepBase, StepResult + + +class MyStep(StepBase): + type_key = "my-step" + + def execute(self, config, context): + return StepResult(output={"ok": True}) +``` + +#### Install from a local directory + +```bash +specify workflow step add my-step --dev /path/to/my-step +``` + +`--dev` takes a **directory** (not an archive, not a bare `step.yml`) that is a +complete package. This needs no catalog, server, or network, which makes it the +supported local-authoring loop: + +```bash +specify workflow step add my-step --dev ./my-step +specify workflow step list +specify workflow step info my-step +# edit ./my-step, then replace the installed copy: +specify workflow step add my-step --dev ./my-step --force +specify workflow step remove my-step +``` + +#### Install from an archive URL + +```bash +specify workflow step add my-step --from https://example.com/my-step.zip +``` + +`--from` accepts a `.zip`, `.tar.gz`, or `.tgz` archive (a bare `step.yml` +URL is **not** a package). The archive may place `step.yml` and `__init__.py` +at its root or under exactly one top-level directory; unrelated top-level +siblings are rejected. Because a step package contains executable Python, a +direct URL install shows a default-deny trust confirmation before any network +request; declining cancels with no request and no error. HTTPS is required +(HTTP is permitted only for loopback hosts), redirects must remain secure, and +downloads are size-bounded. + +#### Install from the catalog + +```bash +specify workflow step add my-step +``` + +Catalog installs resolve individual file URLs from the active step catalogs and +then go through the same validation and commit path as `--dev` and `--from`. +Discovery-only catalogs cannot be installed from. + +#### Replacement and force + +```bash +specify workflow step add my-step --dev ./my-step --force +specify workflow step add my-step --from https://example.com/my-step.zip --force +``` + +`--force` first stages and validates the replacement before touching the +existing installation, and can replace both a registered install and a leftover +unregistered directory. Validation and staging failures leave the previous +package untouched. If a replacement commit fails after the previous package is +removed — removing the old directory, publishing the new one, or writing the +registry — the installation is left incomplete: rerun the command with the +original source and `--force` to reinstall. No automatic rollback is attempted. + +#### Package validation + +Every source is validated identically before anything is committed: + +- `step.yml` and `__init__.py` must be regular, non-symlink files at the package + root. +- The package tree is copied recursively (relative imports, nested helper + modules, and data files are supported). A symlinked package root, any + descendant symlink, and any filesystem object that is not a regular file or + directory are rejected — including inside excluded directories. +- `.git`, `__pycache__`, and `.DS_Store` entries are excluded from the copy and + from the limits. +- A package may contain at most **512 files** and **50 MiB** in total. +- `__init__.py` is **not imported** during installation; it is loaded only when + the step runs. + +> **Security note:** Installing a custom step runs its Python with **your** +> privileges. Only install step packages from sources you trust. + +#### Listing, running, and removing + +Installed custom steps appear in `specify workflow step list` and are loaded +automatically by `workflow add`, `workflow run`, and `workflow resume`. Remove +one with: + +```bash +specify workflow step remove my-step +``` + +#### Registry provenance + +Each installed step records only the *kind* of its source — `catalog` +(optionally with the catalog name), `local`, or `url`. Local paths and source +URLs are never persisted. `specify workflow step info ` shows the source. + +#### Bundle-local limitation + +A bundle's `provides.steps` still resolves only through the active step +catalogs. Bundle-local `steps//` payloads and relative +`provides.steps[].source` overrides are **not** resolved in this release, so +such steps are not installable offline. See the [Bundles reference](bundles.md). + ### Per-Step Integration Configuration Command steps may pass structured runtime configuration to integrations that diff --git a/src/specify_cli/workflows/step/_helpers.py b/src/specify_cli/workflows/step/_helpers.py index 45250ceffc..34ada2f306 100644 --- a/src/specify_cli/workflows/step/_helpers.py +++ b/src/specify_cli/workflows/step/_helpers.py @@ -1,107 +1,47 @@ -"""Shared validation helpers for workflow step commands.""" +"""Shared validation helpers for workflow step commands. + +This module preserves the CLI-coupled ``*_or_exit`` entry points used by the +registered step commands. The behavior now lives in +:mod:`specify_cli.workflows.step.installer`; these wrappers print the shared +error prefix and exit, while the domain module stays CLI-independent. +""" from __future__ import annotations from .. import _commands as cli - -# Custom step packages contain executable Python, metadata, and optional helper -# files downloaded one-by-one rather than as an archive. Mirror the archive -# ceilings so a catalog cannot turn individually valid files into an unbounded -# aggregate download. -_MAX_STEP_PACKAGE_FILES = 512 -_MAX_STEP_PACKAGE_BYTES = 50 * 1024 * 1024 # 50 MiB - -_RESERVED_STEP_IDS: frozenset[str] = frozenset({".cache", "step-registry.json"}) - -_WINDOWS_RESERVED_NAMES: frozenset[str] = frozenset( - { - "con", - "prn", - "aux", - "nul", - "com1", - "com2", - "com3", - "com4", - "com5", - "com6", - "com7", - "com8", - "com9", - "lpt1", - "lpt2", - "lpt3", - "lpt4", - "lpt5", - "lpt6", - "lpt7", - "lpt8", - "lpt9", - } +from .installer import ( + _MAX_STEP_PACKAGE_BYTES, + _MAX_STEP_PACKAGE_FILES, + StepInstallError, + resolve_steps_base_dir, + validate_step_id, ) -_WINDOWS_INVALID_CHARS: frozenset[str] = frozenset('<>:"|?*') +__all__ = [ + "_MAX_STEP_PACKAGE_BYTES", + "_MAX_STEP_PACKAGE_FILES", + "StepInstallError", + "resolve_steps_base_dir", + "validate_step_id", +] def _validate_step_id_or_exit(step_id: str) -> None: """Validate that ``step_id`` is a single safe path component. - Rejects empty strings, whitespace-only strings, leading/trailing whitespace, - path separators, ``.``/``..`` components, dotfile prefixes, reserved names, - Windows-invalid filename characters, trailing dots/spaces, and Windows - reserved device names. Exits with code 1 on failure. + Exits with code 1 on failure. """ - # Strip the stem (before first dot) for Windows reserved-name check - stem = step_id.split(".")[0].lower() if step_id else "" - if ( - not step_id - or not step_id.strip() - or step_id != step_id.strip() - or "/" in step_id - or "\\" in step_id - or step_id in (".", "..") - or step_id.startswith(".") - or step_id.endswith(".") - or step_id.endswith(" ") - or step_id.lower() in _RESERVED_STEP_IDS - or stem in _WINDOWS_RESERVED_NAMES - or any(c in _WINDOWS_INVALID_CHARS for c in step_id) - or any(ord(c) < 32 for c in step_id) - ): - cli.console.print( - f"[red]Error:[/red] Invalid step id '{step_id}': must be a single safe " - "path component (no separators, no leading dot, not a reserved name, " - "no invalid filename characters)" - ) - raise cli.typer.Exit(1) + try: + validate_step_id(step_id) + except StepInstallError as exc: + cli.console.print(f"[red]Error:[/red] {exc}") + raise cli.typer.Exit(1) from exc def _resolve_steps_base_dir_or_exit(project_root: cli.Path) -> cli.Path: """Resolve .specify/workflows/steps while refusing symlinked parent directories.""" - project_root_resolved = project_root.resolve() - steps_base_dir_unresolved = project_root / ".specify" / "workflows" / "steps" - - current = project_root - for part in (".specify", "workflows", "steps"): - current = current / part - if current.is_symlink(): - cli.console.print( - f"[red]Error:[/red] Refusing to use symlinked step directory '{current}'" - ) - raise cli.typer.Exit(1) - if current.exists() and not current.is_dir(): - cli.console.print( - f"[red]Error:[/red] Step directory path is not a directory: '{current}'" - ) - raise cli.typer.Exit(1) - - steps_base_dir = steps_base_dir_unresolved.resolve() try: - steps_base_dir.relative_to(project_root_resolved) - except ValueError: - cli.console.print( - f"[red]Error:[/red] Step directory escapes project root: '{steps_base_dir}'" - ) - raise cli.typer.Exit(1) - - return steps_base_dir + return resolve_steps_base_dir(project_root) + except StepInstallError as exc: + cli.console.print(f"[red]Error:[/red] {exc}") + raise cli.typer.Exit(1) from exc diff --git a/src/specify_cli/workflows/step/command_add.py b/src/specify_cli/workflows/step/command_add.py index 7a3a3b8cad..3aba60bce3 100644 --- a/src/specify_cli/workflows/step/command_add.py +++ b/src/specify_cli/workflows/step/command_add.py @@ -1,109 +1,288 @@ -"""Command handler for ``specify workflow step add``.""" +"""Command handler for ``specify workflow step add``. + +The registered handler stays a thin orchestrator: it parses options, validates +them, and dispatches to a per-source private helper (catalog, ``--dev`` local +directory, and ``--from`` archive URL). All three sources converge on +``step/installer.py``'s single validation + staged-commit path. +""" from __future__ import annotations +from typing import Annotated + from .. import _commands as cli +from . import _helpers as step_helpers from . import step_app -from . import _helpers as step_helpers +def _cleanup_download_tmp_path(tmp_path: cli.Path | None) -> None: + """Best-effort unlink of a partially-downloaded step archive temp file. -@step_app.command("add") -def workflow_step_add( - step_id: str = cli.typer.Argument(..., help="Step type ID from catalog"), -): - """Install a custom step type from the step catalog.""" - from .catalog import ( - StepCatalog, - StepCatalogError, - StepRegistry, - StepValidationError, + A cleanup ``OSError`` must never replace/mask whatever error or interrupt is + already propagating -- warn about it and keep going. + """ + if tmp_path is None: + return + try: + tmp_path.unlink(missing_ok=True) + except OSError as cleanup_exc: + cli.console.print( + "[yellow]Warning:[/yellow] Could not remove temporary " + f"step download file: {cli._escape_markup(str(cleanup_exc))} " + f"(path: {cli._escape_markup(str(tmp_path))})" + ) + + +def _print_installed(step_id: str, entry: dict) -> None: + step_name = entry.get("name") or step_id + cli.console.print(f"[green]✓[/green] Step type '{step_name}' ({step_id}) installed") + cli.console.print( + " Use [cyan]specify workflow step list[/cyan] to verify the installation." ) - project_root = cli._require_specify_project() + +def _install_from_dev( + project_root: cli.Path, step_id: str, dev: str, *, force: bool +) -> None: + """Install a complete step package from a local directory.""" + from . import installer + + dev_path = cli.Path(dev).expanduser() + if dev_path.is_symlink(): + raise installer.StepInstallError( + f"Refusing to install from a symlinked source directory: '{dev_path}'" + ) + if not dev_path.is_dir(): + raise installer.StepInstallError( + "--dev source must be a directory containing step.yml and " + f"__init__.py: '{dev_path}'" + ) + + entry = installer.install_step_package( + project_root, step_id, dev_path, source="local", force=force + ) + _print_installed(step_id, entry) + + +def _install_from_url( + project_root: cli.Path, step_id: str, from_url: str, *, force: bool +) -> None: + """Install a step package archive from a direct URL.""" + import tempfile + from urllib.parse import urlparse + + from rich.panel import Panel + + from specify_cli.authentication.github_http import ( + resolve_github_release_asset_api_url as _resolve_gh_asset, + ) + from specify_cli.authentication.http import ( + github_provider_hosts as _github_provider_hosts, + ) + from specify_cli.authentication.http import open_url as _open_url + + from . import installer + + try: + parsed = urlparse(from_url) + hostname = parsed.hostname + _ = parsed.port + except ValueError: + raise installer.StepInstallError( + f"Invalid URL: {cli._escape_markup(from_url)}" + ) from None + if not hostname: + raise installer.StepInstallError( + f"Invalid URL: {cli._escape_markup(from_url)}" + ) + if not cli.is_https_or_localhost_http(from_url): + raise installer.StepInstallError( + "URL must use HTTPS for security. HTTP is only allowed for " + "loopback URLs." + ) + + # Reject before the trust prompt and before any network request. + installer.check_installable(project_root, step_id, force=force) + + # Prompt BEFORE any request (and before any spinner) so the user can see + # and answer it; a declined prompt issues no request and exits 0. + cli.console.print() + cli.console.print( + Panel( + "[bold]You are installing a workflow step type directly from an " + "external URL.\nA step package contains executable Python.[/bold]\n\n" + f"URL: {cli._escape_markup(from_url)}\n\n" + "Only install step packages from sources you trust.", + title="[bold yellow]⚠ Untrusted Source[/bold yellow]", + border_style="yellow", + padding=(1, 2), + ) + ) + cli.console.print() + if not cli.typer.confirm("Continue with installation?", default=False): + cli.console.print("Cancelled") + raise cli.typer.Exit(0) + + download_url = from_url + extra_headers = None + tmp_path: cli.Path | None = None + try: + resolved_url = _resolve_gh_asset( + from_url, + _open_url, + timeout=30, + github_hosts=_github_provider_hosts(), + redirect_validator=cli._reject_insecure_download_redirect, + ) + if resolved_url: + download_url = resolved_url + extra_headers = {"Accept": "application/octet-stream"} + + with _open_url( + download_url, + timeout=30, + extra_headers=extra_headers, + redirect_validator=cli._reject_insecure_download_redirect, + ) as resp: + final_url = resp.geturl() + if not cli.is_https_or_localhost_http(final_url): + raise installer.StepInstallError( + f"URL redirected to non-HTTPS: {cli._escape_markup(final_url)}" + ) + content_type = ( + resp.getheader("Content-Type") + if hasattr(resp, "getheader") + else None + ) + archive_format = ( + cli.archive_format_from_name(final_url) + or cli.archive_format_from_name(from_url) + or cli.archive_format_from_content_type(content_type) + ) + if archive_format is None: + raise installer.StepInstallError( + "URL does not reference a supported archive " + "(.zip, .tar.gz, or .tgz)" + ) + downloaded = cli.read_response_limited( + resp, + error_type=ValueError, + label="step archive download", + ) + + with tempfile.NamedTemporaryFile( + suffix=cli.archive_suffix(archive_format), delete=False + ) as tmp: + tmp_path = cli.Path(tmp.name) + tmp.write(downloaded) + + with tempfile.TemporaryDirectory( + prefix="speckit-step-archive-" + ) as extract_dir: + extracted_root = cli.Path(extract_dir) + # safe_extract_archive re-detects and confirms the archive bytes. + cli.safe_extract_archive( + tmp_path, + extracted_root, + source_name=final_url, + content_type=content_type, + ) + package_root = installer.resolve_package_root(extracted_root) + entry = installer.install_step_package( + project_root, + step_id, + package_root, + source="url", + force=force, + ) + except cli.typer.Exit: + raise + except installer.StepInstallError: + raise + except Exception as exc: + raise installer.StepInstallError( + f"Failed to install step from URL: {cli._escape_markup(str(exc))}" + ) from exc + finally: + _cleanup_download_tmp_path(tmp_path) + + _print_installed(step_id, entry) + + +def _install_from_catalog(project_root: cli.Path, step_id: str, *, force: bool) -> None: + """Install a step package from the step catalog. + + The catalog fetch (URL/derivation/count preflight) stays a catalog concern; + the materialized files are then handed to the shared installer. + """ + import tempfile + + from . import installer + from .catalog import StepCatalog, StepCatalogError catalog = StepCatalog(project_root) try: info = catalog.get_step_info(step_id) except StepCatalogError as exc: - cli.console.print(f"[red]Error:[/red] {exc}") - raise cli.typer.Exit(1) + raise installer.StepInstallError(str(exc)) from exc if not info: - cli.console.print( - f"[red]Error:[/red] Step type '{step_id}' not found in catalog" + raise installer.StepInstallError( + f"Step type '{step_id}' not found in catalog" ) - raise cli.typer.Exit(1) if not info.get("_install_allowed", True): cli.console.print( - f"[yellow]Warning:[/yellow] Step type '{step_id}' is from a discovery-only catalog" + f"[yellow]Warning:[/yellow] Step type '{step_id}' is from a " + "discovery-only catalog" ) cli.console.print("Direct installation is not enabled for this catalog source.") raise cli.typer.Exit(1) - # Reject step IDs that collide with built-in step types - from .. import STEP_REGISTRY as _step_reg - - if step_id in _step_reg: - cli.console.print( - f"[red]Error:[/red] Step type '{step_id}' conflicts with a built-in step type" - ) - raise cli.typer.Exit(1) - - # Reject if already installed - registry = StepRegistry(project_root) - if registry.is_installed(step_id): - cli.console.print( - f"[red]Error:[/red] Step type '{step_id}' is already installed. " - "Remove it first with: [cyan]specify workflow step remove " - f"{step_id}[/cyan]" - ) - raise cli.typer.Exit(1) + # Reject built-in collisions and duplicates before any download. + installer.check_installable(project_root, step_id, force=force) declared_step_yml_url = info.get("step_yml_url") if declared_step_yml_url is not None and not isinstance(declared_step_yml_url, str): - cli.console.print( - f"[red]Error:[/red] Catalog entry for '{step_id}' has a malformed " - "step.yml URL; expected a non-empty string" + raise installer.StepInstallError( + f"Catalog entry for '{step_id}' has a malformed step.yml URL; " + "expected a non-empty string" ) - raise cli.typer.Exit(1) step_yml_url = declared_step_yml_url or info.get("url") if step_yml_url is None or ( isinstance(step_yml_url, str) and not step_yml_url.strip() ): - cli.console.print(f"[red]Error:[/red] Catalog entry for '{step_id}' has no URL") - raise cli.typer.Exit(1) + raise installer.StepInstallError( + f"Catalog entry for '{step_id}' has no URL" + ) if not isinstance(step_yml_url, str): - cli.console.print( - f"[red]Error:[/red] Catalog entry for '{step_id}' has a malformed " - "step.yml URL; expected a non-empty string" + raise installer.StepInstallError( + f"Catalog entry for '{step_id}' has a malformed step.yml URL; " + "expected a non-empty string" ) - raise cli.typer.Exit(1) - # Derive __init__.py URL: replace trailing step.yml with __init__.py - # or use explicit init_url if provided. + # Derive __init__.py URL: replace trailing step.yml with __init__.py or use + # explicit init_url if provided. init_url = info.get("init_url") if init_url is not None and (not isinstance(init_url, str) or not init_url.strip()): - cli.console.print( - f"[red]Error:[/red] Catalog entry for '{step_id}' has a malformed " - "__init__.py URL; expected a non-empty string" + raise installer.StepInstallError( + f"Catalog entry for '{step_id}' has a malformed __init__.py URL; " + "expected a non-empty string" ) - raise cli.typer.Exit(1) if not init_url: if step_yml_url.endswith("step.yml"): init_url = step_yml_url[: -len("step.yml")] + "__init__.py" else: - cli.console.print( - f"[red]Error:[/red] Cannot derive __init__.py URL from '{step_yml_url}'. " - "Catalog entry should provide 'init_url' or a 'url' ending in 'step.yml'." + raise installer.StepInstallError( + f"Cannot derive __init__.py URL from '{step_yml_url}'. " + "Catalog entry should provide 'init_url' or a 'url' ending in " + "'step.yml'." ) - raise cli.typer.Exit(1) # Preflight the declared file count before creating a staging directory or - # issuing any request. The two required files are always part of the package; - # duplicate declarations for them in extra_files are ignored below and do - # not count twice. + # issuing any request. The two required files are always part of the + # package; duplicate declarations for them in extra_files are ignored below + # and do not count twice. extra_files = info.get("extra_files") if extra_files is not None and not isinstance(extra_files, dict): cli.console.print( @@ -126,12 +305,11 @@ def _is_required_package_file(rel_path: object) -> bool: 1 for rel_path in (extra_files or {}) if not _is_required_package_file(rel_path) ) package_file_count = 2 + declared_extra_count - if package_file_count > step_helpers._MAX_STEP_PACKAGE_FILES: - cli.console.print( - f"[red]Error:[/red] Step package declares {package_file_count} files, " - f"exceeding the {step_helpers._MAX_STEP_PACKAGE_FILES}-file limit" + if package_file_count > installer._MAX_STEP_PACKAGE_FILES: + raise installer.StepInstallError( + f"Step package declares {package_file_count} files, exceeding the " + f"{installer._MAX_STEP_PACKAGE_FILES}-file limit" ) - raise cli.typer.Exit(1) from specify_cli.authentication.http import open_url as _open_url @@ -146,231 +324,127 @@ def _safe_fetch(url: str) -> bytes: raise ValueError(f"Redirect to non-HTTPS URL: {final_url}") return cli._read_response_within_limit(resp) - step_helpers._validate_step_id_or_exit(step_id) - - steps_base_dir = step_helpers._resolve_steps_base_dir_or_exit(project_root) - step_dir = (steps_base_dir / step_id).resolve() - # Defense-in-depth: ensure the resolved directory is a direct child of - # steps_base_dir even after symlink resolution. - try: - rel_parts = step_dir.relative_to(steps_base_dir).parts - except ValueError: - cli.console.print(f"[red]Error:[/red] Invalid step id '{step_id}'") - raise cli.typer.Exit(1) - if rel_parts != (step_id,): - cli.console.print(f"[red]Error:[/red] Invalid step id '{step_id}'") - raise cli.typer.Exit(1) - - import shutil - import tempfile - - # Refuse if step_dir already exists (e.g. leftover from a previous failed/manual - # install that wasn't registered). The user should remove it before retrying. - if step_dir.exists(): - cli.console.print( - f"[red]Error:[/red] Step directory already exists at '{step_dir}'. " - f"Remove it manually or use: [cyan]specify workflow step remove {step_id}[/cyan]" - ) - raise cli.typer.Exit(1) - - # Create steps_base_dir now so the staging temp dir is on the same filesystem, - # enabling a truly atomic os.rename() below. - try: - steps_base_dir.mkdir(parents=True, exist_ok=True) - tmp_path = cli.Path( - tempfile.mkdtemp(prefix="speckit_step_tmp_", dir=steps_base_dir) - ) - except OSError as exc: - cli.console.print( - f"[red]Error:[/red] Failed to create staging directory: {exc}" - ) - raise cli.typer.Exit(1) - try: + with tempfile.TemporaryDirectory(prefix="speckit-step-package-") as package_tmp: + package_dir = cli.Path(package_tmp) try: step_yml_content = _safe_fetch(step_yml_url) init_py_content = _safe_fetch(init_url) except Exception as exc: - cli.console.print(f"[red]Error:[/red] Failed to download step files: {exc}") - raise cli.typer.Exit(1) + raise installer.StepInstallError( + f"Failed to download step files: {exc}" + ) from exc package_bytes = len(step_yml_content) + len(init_py_content) - if package_bytes > step_helpers._MAX_STEP_PACKAGE_BYTES: - cli.console.print( - f"[red]Error:[/red] Step package exceeds the " - f"{step_helpers._MAX_STEP_PACKAGE_BYTES}-byte total size limit" - ) - raise cli.typer.Exit(1) - - # Validate step.yml - try: - import yaml as _yaml - - step_yml_text = step_yml_content.decode("utf-8") - # ``safe_load`` returns None for BOTH an empty document and an - # explicit null scalar (``null``, ``~``, ``NULL``), so it cannot - # tell them apart on its own. ``compose`` yields no node only for - # a genuinely empty document. - node = _yaml.compose(step_yml_text) - meta = _yaml.safe_load(step_yml_text) - is_empty_document = node is None or ( - meta is None - and isinstance(node, _yaml.nodes.ScalarNode) - and node.value == "" - and node.start_mark.index == node.end_mark.index - ) - except Exception as exc: - cli.console.print(f"[red]Error:[/red] Invalid step.yml: {exc}") - raise cli.typer.Exit(1) - - # Do NOT coerce with ``or {}`` here: that also turns a FALSY non-mapping - # (top-level ``[]``, ``false``, ``0``, ``''``, or an explicit ``null``) - # into ``{}`` and silently bypasses this shape check, surfacing the - # unrelated "missing 'step.type_key'" error below instead of the real - # problem. Only a genuinely empty document defaults to ``{}``. - if meta is None and is_empty_document: - meta = {} - elif not isinstance(meta, dict): - cli.console.print("[red]Error:[/red] step.yml must be a YAML mapping") - raise cli.typer.Exit(1) - - step_meta = meta.get("step", {}) - if not isinstance(step_meta, dict): - cli.console.print( - "[red]Error:[/red] step.yml 'step' field must be a mapping" + if package_bytes > installer._MAX_STEP_PACKAGE_BYTES: + raise installer.StepInstallError( + f"Step package exceeds the " + f"{installer._MAX_STEP_PACKAGE_BYTES}-byte total size limit" ) - raise cli.typer.Exit(1) - type_key = step_meta.get("type_key", "") - if not type_key: - cli.console.print( - "[red]Error:[/red] step.yml missing 'step.type_key' field" - ) - raise cli.typer.Exit(1) - - if type_key != step_id: - cli.console.print( - f"[red]Error:[/red] step.yml type_key ({type_key!r}) does not match " - f"catalog ID ({step_id!r})" - ) - raise cli.typer.Exit(1) - # Write the two required files. try: - (tmp_path / "step.yml").write_bytes(step_yml_content) - (tmp_path / "__init__.py").write_bytes(init_py_content) + (package_dir / "step.yml").write_bytes(step_yml_content) + (package_dir / "__init__.py").write_bytes(init_py_content) except OSError as exc: - cli.console.print( - f"[red]Error:[/red] Failed to write step files to staging directory: {exc}" - ) - raise cli.typer.Exit(1) - - # Optionally download additional package files declared in the catalog entry - # (e.g. helper modules). Each entry in ``extra_files`` is a mapping of - # relative-path → URL. step.yml and __init__.py are ignored here (already - # written). Paths are validated to stay within the step package directory to - # prevent path-traversal attacks. + raise installer.StepInstallError( + f"Failed to write step files to staging directory: {exc}" + ) from exc + + # Optionally download additional package files declared in the catalog + # entry (e.g. helper modules). Each entry in ``extra_files`` is a mapping + # of relative-path → URL. Paths are validated to stay within the step + # package directory to prevent path-traversal attacks. for rel_path, file_url in (extra_files or {}).items(): if not isinstance(rel_path, str) or not rel_path.strip(): - cli.console.print( - "[red]Error:[/red] Catalog entry 'extra_files' contains an " - "empty or non-string path key" + raise installer.StepInstallError( + "Catalog entry 'extra_files' contains an empty or non-string " + "path key" ) - raise cli.typer.Exit(1) if _is_required_package_file(rel_path): continue # already written above - # Reject dot-path segments ('', '.', '..') that would refer to the - # package directory itself (IsADirectoryError) or escape it. - rel_parts = cli.Path(rel_path).parts - if not rel_parts or any(seg in ("", ".", "..") for seg in rel_parts): - cli.console.print( - f"[red]Error:[/red] extra_files path '{rel_path}' is not a " - "valid relative file path" + path_parts = cli.Path(rel_path).parts + if not path_parts or any(seg in ("", ".", "..") for seg in path_parts): + raise installer.StepInstallError( + f"extra_files path '{rel_path}' is not a valid relative file path" ) - raise cli.typer.Exit(1) if not isinstance(file_url, str) or not file_url.strip(): - cli.console.print( - f"[red]Error:[/red] extra_files entry '{rel_path}' has an " - "empty or non-string URL" + raise installer.StepInstallError( + f"extra_files entry '{rel_path}' has an empty or non-string URL" ) - raise cli.typer.Exit(1) - # Resolve both destination and base to handle any symlinks in tmp_path itself, - # ensuring the traversal check is robust even on non-canonical paths. - resolved_base = tmp_path.resolve() - dest = (tmp_path / rel_path).resolve() + resolved_base = package_dir.resolve() + dest = (package_dir / rel_path).resolve() try: dest.relative_to(resolved_base) except ValueError: - cli.console.print( - f"[red]Error:[/red] extra_files path '{rel_path}' is outside " - "the step package directory" - ) - raise cli.typer.Exit(1) + raise installer.StepInstallError( + f"extra_files path '{rel_path}' is outside the step package " + "directory" + ) from None try: file_content = _safe_fetch(file_url) except Exception as exc: - cli.console.print( - f"[red]Error:[/red] Failed to download extra file '{rel_path}': {exc}" - ) - raise cli.typer.Exit(1) + raise installer.StepInstallError( + f"Failed to download extra file '{rel_path}': {exc}" + ) from exc package_bytes += len(file_content) - if package_bytes > step_helpers._MAX_STEP_PACKAGE_BYTES: - cli.console.print( - f"[red]Error:[/red] Step package exceeds the " - f"{step_helpers._MAX_STEP_PACKAGE_BYTES}-byte total size limit" + if package_bytes > installer._MAX_STEP_PACKAGE_BYTES: + raise installer.StepInstallError( + f"Step package exceeds the " + f"{installer._MAX_STEP_PACKAGE_BYTES}-byte total size limit" ) - raise cli.typer.Exit(1) try: dest.parent.mkdir(parents=True, exist_ok=True) dest.write_bytes(file_content) except OSError as exc: - cli.console.print( - f"[red]Error:[/red] Failed to write extra file '{rel_path}': {exc}" - ) - raise cli.typer.Exit(1) + raise installer.StepInstallError( + f"Failed to write extra file '{rel_path}': {exc}" + ) from exc - # Atomically rename the staging directory to the final location. - # Both paths are under steps_base_dir (same filesystem), so os.rename() - # is atomic on POSIX and won't leave a partially-written directory at - # step_dir on failure. - try: - cli.os.rename(tmp_path, step_dir) - except OSError as exc: - cli.console.print( - f"[red]Error:[/red] Failed to install step '{step_id}': {exc}" - ) - raise cli.typer.Exit(1) - finally: - # Clean up if the rename hasn't moved tmp_path yet (i.e. on any failure). - shutil.rmtree(tmp_path, ignore_errors=True) + entry = installer.install_step_package( + project_root, + step_id, + package_dir, + source="catalog", + catalog_name=info.get("_catalog_name", ""), + catalog_metadata=info, + force=force, + ) - step_name = info.get("name") or step_id - step_version = info.get("version") or step_meta.get("version") or "0.0.0" + _print_installed(step_id, entry) - # Register in step registry - registry = StepRegistry(project_root) - try: - registry.add( - step_id, - { - "name": step_name, - "version": step_version, - "description": info.get( - "description", step_meta.get("description", "") - ), - "author": info.get("author", step_meta.get("author", "")), - "source": "catalog", - "catalog_name": info.get("_catalog_name", ""), - "type_key": type_key, - }, + +@step_app.command("add") +def workflow_step_add( + step_id: str = cli.typer.Argument(..., help="Step type ID"), + dev: Annotated[str | None, cli.typer.Option("--dev", help="Install from a local step package directory")] = None, + from_url: Annotated[str | None, cli.typer.Option("--from", help="Install from a .zip/.tar.gz/.tgz archive URL")] = None, + force: Annotated[bool, cli.typer.Option("--force", help="Replace an existing installation")] = False, +): + """Install a custom step type from the catalog, a local directory, or a URL.""" + from . import installer + + project_root = cli._require_specify_project() + + if dev is not None and from_url is not None: + cli.console.print( + "[red]Error:[/red] --dev and --from are mutually exclusive" ) - except StepValidationError as exc: - # Roll back the just-installed directory so the system isn't left with - # an unregistered step package on disk after a registry write failure - # (e.g. read-only filesystem, permission denied). - shutil.rmtree(step_dir, ignore_errors=True) - cli.console.print(f"[red]Error:[/red] {exc}") + raise cli.typer.Exit(1) + if dev is not None and not dev.strip(): + cli.console.print("[red]Error:[/red] --dev value must not be empty") + raise cli.typer.Exit(1) + if from_url is not None and not from_url.strip(): + cli.console.print("[red]Error:[/red] --from value must not be empty") raise cli.typer.Exit(1) - cli.console.print(f"[green]✓[/green] Step type '{step_name}' ({step_id}) installed") - cli.console.print( - " Use [cyan]specify workflow step list[/cyan] to verify the installation." - ) + step_helpers._validate_step_id_or_exit(step_id) + + try: + if dev is not None: + _install_from_dev(project_root, step_id, dev, force=force) + elif from_url is not None: + _install_from_url(project_root, step_id, from_url, force=force) + else: + _install_from_catalog(project_root, step_id, force=force) + except installer.StepInstallError as exc: + cli.console.print(f"[red]Error:[/red] {exc}") + raise cli.typer.Exit(1) from exc diff --git a/src/specify_cli/workflows/step/command_info.py b/src/specify_cli/workflows/step/command_info.py index c98072d8c0..6f70a0403b 100644 --- a/src/specify_cli/workflows/step/command_info.py +++ b/src/specify_cli/workflows/step/command_info.py @@ -6,6 +6,23 @@ from . import step_app +def _format_source(installed_meta: dict) -> str: + """Render a registry entry's provenance as a human-facing source label. + + Local and URL installs deliberately store no path/URL, so only the source + kind is shown. + """ + source = installed_meta.get("source") + if source == "catalog": + catalog_name = installed_meta.get("catalog_name") + if catalog_name: + return f"catalog ({cli._escape_markup(str(catalog_name))})" + return "catalog" + if source in ("local", "url"): + return str(source) + return "" + + @step_app.command("info") def workflow_step_info( step_id: str = cli.typer.Argument(..., help="Step type ID"), @@ -46,6 +63,9 @@ def workflow_step_info( f" Description: " f"{cli._escape_markup(str(installed_meta['description']))}" ) + source_label = _format_source(installed_meta) + if source_label: + cli.console.print(f" Source: {source_label}") cli.console.print(" [green]Installed[/green]") return diff --git a/src/specify_cli/workflows/step/installer.py b/src/specify_cli/workflows/step/installer.py new file mode 100644 index 0000000000..1e37e2b57c --- /dev/null +++ b/src/specify_cli/workflows/step/installer.py @@ -0,0 +1,612 @@ +"""Domain install/validation for custom workflow step packages. + +This module owns the source-independent behavior shared by every +``specify workflow step add`` source mode (catalog, ``--dev`` local directory, +and ``--from`` archive URL): step-id and base-directory validation, package +shape/symlink/limit validation, staging, atomic commit, and registry +provenance. It is deliberately CLI-independent -- it never prints and never +raises ``typer.Exit``. Callers receive :class:`StepInstallError` and decide how +to surface it. +""" + +from __future__ import annotations + +import os +import shutil +import stat +import tempfile +from collections.abc import Mapping +from pathlib import Path +from typing import Any + +import yaml + +# Custom step packages contain executable Python, metadata, and optional helper +# files. These ceilings apply uniformly to catalog, local, and archive sources. +_MAX_STEP_PACKAGE_FILES = 512 +_MAX_STEP_PACKAGE_BYTES = 50 * 1024 * 1024 # 50 MiB + +# Files/dirs never copied into (or counted as part of) an installed step +# package. Mirrors ``bundles/packager.py`` ``EXCLUDE_NAMES``. +EXCLUDE_NAMES: frozenset[str] = frozenset({".git", "__pycache__", ".DS_Store"}) + +# Prefix for the private same-filesystem working directory created beneath the +# steps base directory. The leading dot keeps it out of the way, and because it +# contains only a ``staged/`` child (never ``step.yml``/``__init__.py`` at its +# root) the runtime loader never mistakes it for an installable package. +_WORK_DIR_PREFIX = ".speckit-step-install-" + +_RESERVED_STEP_IDS: frozenset[str] = frozenset({".cache", "step-registry.json"}) + +_WINDOWS_RESERVED_NAMES: frozenset[str] = frozenset( + { + "con", + "prn", + "aux", + "nul", + "com1", + "com2", + "com3", + "com4", + "com5", + "com6", + "com7", + "com8", + "com9", + "lpt1", + "lpt2", + "lpt3", + "lpt4", + "lpt5", + "lpt6", + "lpt7", + "lpt8", + "lpt9", + } +) + +_WINDOWS_INVALID_CHARS: frozenset[str] = frozenset('<>:"|?*') + + +class StepInstallError(Exception): + """User-facing step package install/validation failure.""" + + +# --------------------------------------------------------------------------- +# Step id + base directory validation +# --------------------------------------------------------------------------- + + +def validate_step_id(step_id: str) -> None: + """Validate that ``step_id`` is a single safe path component. + + Rejects empty strings, whitespace-only strings, leading/trailing + whitespace, path separators, ``.``/``..`` components, dotfile prefixes, + reserved names, Windows-invalid filename characters, trailing dots/spaces, + and Windows reserved device names. + """ + stem = step_id.split(".")[0].lower() if step_id else "" + if ( + not step_id + or not step_id.strip() + or step_id != step_id.strip() + or "/" in step_id + or "\\" in step_id + or step_id in (".", "..") + or step_id.startswith(".") + or step_id.endswith((".", " ")) + or step_id.lower() in _RESERVED_STEP_IDS + or stem in _WINDOWS_RESERVED_NAMES + or any(c in _WINDOWS_INVALID_CHARS for c in step_id) + or any(ord(c) < 32 for c in step_id) + ): + raise StepInstallError( + f"Invalid step id '{step_id}': must be a single safe " + "path component (no separators, no leading dot, not a reserved name, " + "no invalid filename characters)" + ) + + +def resolve_steps_base_dir(project_root: Path) -> Path: + """Resolve ``.specify/workflows/steps`` refusing symlinked parent dirs.""" + project_root = Path(project_root) + project_root_resolved = project_root.resolve() + steps_base_dir_unresolved = project_root / ".specify" / "workflows" / "steps" + + current = project_root + for part in (".specify", "workflows", "steps"): + current = current / part + if current.is_symlink(): + raise StepInstallError( + f"Refusing to use symlinked step directory '{current}'" + ) + if current.exists() and not current.is_dir(): + raise StepInstallError( + f"Step directory path is not a directory: '{current}'" + ) + + steps_base_dir = steps_base_dir_unresolved.resolve() + try: + steps_base_dir.relative_to(project_root_resolved) + except ValueError: + raise StepInstallError( + f"Step directory escapes project root: '{steps_base_dir}'" + ) from None + + return steps_base_dir + + +def _resolve_step_dir(steps_base_dir: Path, step_id: str) -> Path: + """Return the canonical destination directory for ``step_id``.""" + step_dir = steps_base_dir / step_id + try: + rel_parts = step_dir.relative_to(steps_base_dir).parts + except ValueError: + raise StepInstallError(f"Invalid step id '{step_id}'") from None + if rel_parts != (step_id,): + raise StepInstallError(f"Invalid step id '{step_id}'") + return step_dir + + +def _reject_unsafe_destination(step_dir: Path) -> None: + """Refuse a symlink (including dangling) or non-directory destination.""" + if step_dir.is_symlink(): + raise StepInstallError( + f"Refusing to install step through a symlinked path: '{step_dir}'" + ) + if step_dir.exists() and not step_dir.is_dir(): + raise StepInstallError( + f"Step install path exists but is not a directory: '{step_dir}'" + ) + + +# --------------------------------------------------------------------------- +# Package shape + safety validation +# --------------------------------------------------------------------------- + + +def _walk_package_tree(package_dir: Path): + """Yield ``(path, is_dir, excluded)`` for every descendant of *package_dir*. + + Descends into excluded directories so a symlink or special file hiding + inside ``.git``/``__pycache__`` is still rejected, but never follows a + symlink. Raises :class:`StepInstallError` on any symlink or object that is + neither a regular file nor a directory. + """ + + def _walk(current: Path, excluded_prefix: bool): + try: + entries = sorted(os.scandir(current), key=lambda entry: entry.name) + except OSError as exc: + raise StepInstallError( + f"Failed to read step package directory '{current}': {exc}" + ) from exc + for entry in entries: + try: + mode = entry.stat(follow_symlinks=False).st_mode + except OSError as exc: + raise StepInstallError( + f"Failed to inspect step package entry '{entry.path}': {exc}" + ) from exc + path = Path(entry.path) + if stat.S_ISLNK(mode): + raise StepInstallError(f"Step package contains symlink: {path}") + excluded = excluded_prefix or entry.name in EXCLUDE_NAMES + if stat.S_ISDIR(mode): + yield path, True, excluded + yield from _walk(path, excluded) + elif stat.S_ISREG(mode): + yield path, False, excluded + else: + raise StepInstallError( + f"Step package contains unsupported file: {path}" + ) + + yield from _walk(package_dir, False) + + +def _parse_step_metadata(step_yml_text: str, step_id: str) -> dict[str, Any]: + """Parse and validate ``step.yml``, returning the ``step`` mapping.""" + try: + # ``safe_load`` returns None for BOTH an empty document and an explicit + # null scalar (``null``, ``~``, ``NULL``), so it cannot tell them apart + # on its own. ``compose`` yields no node only for a genuinely empty + # document. + node = yaml.compose(step_yml_text) + meta = yaml.safe_load(step_yml_text) + is_empty_document = node is None or ( + meta is None + and isinstance(node, yaml.nodes.ScalarNode) + and node.value == "" + and node.start_mark.index == node.end_mark.index + ) + except Exception as exc: + raise StepInstallError(f"Invalid step.yml: {exc}") from exc + + # Do NOT coerce with ``or {}`` here: that also turns a FALSY non-mapping + # (top-level ``[]``, ``false``, ``0``, ``''``, or an explicit ``null``) + # into ``{}`` and silently bypasses this shape check. Only a genuinely + # empty document defaults to ``{}``. + if meta is None and is_empty_document: + meta = {} + elif not isinstance(meta, dict): + raise StepInstallError("step.yml must be a YAML mapping") + + step_meta = meta.get("step", {}) + if not isinstance(step_meta, dict): + raise StepInstallError("step.yml 'step' field must be a mapping") + type_key = step_meta.get("type_key", "") + if not type_key: + raise StepInstallError("step.yml missing 'step.type_key' field") + if type_key != step_id: + raise StepInstallError( + f"step.yml type_key ({type_key!r}) does not match step ID ({step_id!r})" + ) + return step_meta + + +def validate_step_package(package_dir: Path, step_id: str) -> dict[str, Any]: + """Validate a materialized step package directory. + + Returns the validated ``step.yml`` ``step`` mapping. Raises + :class:`StepInstallError` on any shape, symlink, limit, path, or identity + violation. Never imports or executes ``__init__.py``. + """ + package_dir = Path(package_dir) + + if package_dir.is_symlink(): + raise StepInstallError( + f"Refusing to install from a symlinked package directory: '{package_dir}'" + ) + if not package_dir.is_dir(): + raise StepInstallError(f"Step package directory not found: '{package_dir}'") + + for required in ("step.yml", "__init__.py"): + required_path = package_dir / required + if required_path.is_symlink(): + raise StepInstallError( + f"Step package '{required}' must be a regular file, not a symlink" + ) + if not required_path.is_file(): + raise StepInstallError( + f"Step package is missing required file '{required}' at its root" + ) + + retained_files = 0 + retained_bytes = 0 + for path, is_dir, excluded in _walk_package_tree(package_dir): + if is_dir or excluded: + continue + retained_files += 1 + try: + retained_bytes += path.lstat().st_size + except OSError as exc: + raise StepInstallError( + f"Failed to inspect step package file '{path}': {exc}" + ) from exc + + if retained_files > _MAX_STEP_PACKAGE_FILES: + raise StepInstallError( + f"Step package contains {retained_files} files, exceeding the " + f"{_MAX_STEP_PACKAGE_FILES}-file limit" + ) + if retained_bytes > _MAX_STEP_PACKAGE_BYTES: + raise StepInstallError( + f"Step package exceeds the {_MAX_STEP_PACKAGE_BYTES}-byte total " + "size limit" + ) + + try: + step_yml_text = (package_dir / "step.yml").read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError) as exc: + raise StepInstallError(f"Invalid step.yml: {exc}") from exc + + return _parse_step_metadata(step_yml_text, step_id) + + +def resolve_package_root(extracted_root: Path) -> Path: + """Resolve a root-level or single-nested step package directory.""" + extracted_root = Path(extracted_root) + root_manifest = extracted_root / "step.yml" + if root_manifest.is_file() and not root_manifest.is_symlink(): + return extracted_root + try: + entries = list(extracted_root.iterdir()) + except OSError as exc: + raise StepInstallError( + f"Failed to inspect archive contents: {exc}" + ) from exc + if len(entries) == 1: + candidate = entries[0] + candidate_manifest = candidate / "step.yml" + if ( + candidate.is_dir() + and not candidate.is_symlink() + and candidate_manifest.is_file() + and not candidate_manifest.is_symlink() + ): + return candidate + raise StepInstallError( + "archive must contain step.yml at its root or in exactly one top-level " + "directory" + ) + + +# --------------------------------------------------------------------------- +# Collision / duplicate preflight +# --------------------------------------------------------------------------- + + +def _reject_builtin_collision(step_id: str) -> None: + from .. import BUILTIN_STEP_TYPES + + if step_id in BUILTIN_STEP_TYPES: + raise StepInstallError( + f"Step type '{step_id}' conflicts with a built-in step type" + ) + + +def _check_duplicate( + registry: Any, step_id: str, step_dir: Path, *, force: bool +) -> None: + if force: + return + if registry.is_installed(step_id): + raise StepInstallError( + f"Step type '{step_id}' is already installed. Remove it first with: " + f"[cyan]specify workflow step remove {step_id}[/cyan]" + ) + if step_dir.exists(): + raise StepInstallError( + f"Step directory already exists at '{step_dir}'. Remove it manually " + f"or use: [cyan]specify workflow step remove {step_id}[/cyan]" + ) + + +def check_installable(project_root: Path, step_id: str, *, force: bool = False) -> Path: + """Advisory preflight shared by all sources. + + Validates the id, base directory, built-in collision, and + duplicate/orphan-destination state without touching the package. The CLI + uses this to reject before a download; :func:`install_step_package` + re-runs the same checks as defense-in-depth. + """ + from .catalog import StepRegistry + + validate_step_id(step_id) + steps_base_dir = resolve_steps_base_dir(project_root) + step_dir = _resolve_step_dir(steps_base_dir, step_id) + _reject_unsafe_destination(step_dir) + _reject_builtin_collision(step_id) + registry = StepRegistry(project_root) + _check_duplicate(registry, step_id, step_dir, force=force) + return step_dir + + +# --------------------------------------------------------------------------- +# Staging + commit +# --------------------------------------------------------------------------- + + +def _build_entry( + step_id: str, + step_meta: Mapping[str, Any], + *, + source: str, + catalog_name: str, + catalog_metadata: Mapping[str, Any] | None, +) -> dict[str, Any]: + catalog_metadata = catalog_metadata or {} + entry: dict[str, Any] = { + "name": catalog_metadata.get("name") + or step_meta.get("name") + or step_id, + "version": catalog_metadata.get("version") + or step_meta.get("version") + or "0.0.0", + "description": catalog_metadata.get( + "description", step_meta.get("description", "") + ), + "author": catalog_metadata.get("author", step_meta.get("author", "")), + "type_key": step_meta["type_key"], + "source": source, + } + if source == "catalog": + entry["catalog_name"] = catalog_name + return entry + + +def _copy_package_tree(source_dir: Path, target_dir: Path) -> None: + """Recursively copy *source_dir* into *target_dir*, skipping excludes. + + Refuses to follow a symlink encountered mid-copy so a source swapped after + validation cannot smuggle external content into the staged package. + """ + + def _copy(current: Path, destination: Path) -> None: + try: + destination.mkdir(parents=True, exist_ok=True) + except OSError as exc: + raise StepInstallError( + f"Failed to stage step package: {exc}" + ) from exc + try: + entries = sorted(os.scandir(current), key=lambda entry: entry.name) + except OSError as exc: + raise StepInstallError( + f"Failed to stage step package: {exc}" + ) from exc + for entry in entries: + if entry.name in EXCLUDE_NAMES: + continue + try: + mode = entry.stat(follow_symlinks=False).st_mode + except OSError as exc: + raise StepInstallError(f"Failed to stage step package: {exc}") from exc + target = destination / entry.name + if stat.S_ISLNK(mode): + raise StepInstallError( + f"Step package contains symlink: {entry.path}" + ) + if stat.S_ISDIR(mode): + _copy(Path(entry.path), target) + elif stat.S_ISREG(mode): + try: + shutil.copyfile(entry.path, target) + except OSError as exc: + raise StepInstallError( + f"Failed to stage step package: {exc}" + ) from exc + else: + raise StepInstallError( + f"Step package contains unsupported file: {entry.path}" + ) + + _copy(source_dir, target_dir) + + +def _replace_install( + step_dir: Path, + staged_dir: Path, + registry: Any, + step_id: str, + entry: dict[str, Any], + *, + force: bool, +) -> None: + """Publish the staged package and record its registry entry.""" + from .catalog import StepValidationError + + if step_dir.exists(): + # --force replacement: the replacement is fully staged and validated, + # so it is safe to remove the previous installation now. + try: + shutil.rmtree(step_dir) + except OSError as exc: + raise StepInstallError( + f"Failed to remove the existing step installation at " + f"'{step_dir}': {exc}. Reinstall from the original source with " + "--force." + ) from exc + try: + os.replace(staged_dir, step_dir) + except OSError as exc: + raise StepInstallError( + f"Failed to publish the replacement for step type '{step_id}': " + f"{exc}. The previous installation was removed; reinstall from " + "the original source with --force." + ) from exc + try: + registry.add(step_id, entry) + except (StepValidationError, OSError, TypeError, ValueError) as exc: + raise StepInstallError( + f"Failed to update the step registry for '{step_id}': {exc}. The " + "step directory was replaced but is not registered; reinstall " + "from the original source with --force." + ) from exc + return + + try: + os.replace(staged_dir, step_dir) + except OSError as exc: + raise StepInstallError( + f"Failed to install step '{step_id}': {exc}" + ) from exc + + try: + registry.add(step_id, entry) + except (StepValidationError, OSError, TypeError, ValueError) as exc: + # Fresh install: roll back the just-published directory so the system + # is not left with an unregistered step package on disk. + shutil.rmtree(step_dir, ignore_errors=True) + raise StepInstallError(str(exc)) from exc + + +def install_step_package( + project_root: Path, + step_id: str, + package_dir: Path, + *, + source: str, + catalog_name: str = "", + catalog_metadata: Mapping[str, Any] | None = None, + force: bool = False, +) -> dict[str, Any]: + """Validate, stage, and commit a step package from any source. + + ``source`` is exactly ``"catalog"``, ``"local"``, or ``"url"``. Returns the + registry entry that was persisted. + """ + from .catalog import StepRegistry + + package_dir = Path(package_dir) + if package_dir.is_symlink(): + raise StepInstallError( + f"Refusing to install from a symlinked package directory: '{package_dir}'" + ) + if not package_dir.is_dir(): + raise StepInstallError(f"Step package directory not found: '{package_dir}'") + + validate_step_id(step_id) + steps_base_dir = resolve_steps_base_dir(project_root) + step_dir = _resolve_step_dir(steps_base_dir, step_id) + _reject_unsafe_destination(step_dir) + + # Reject a source that resolves to (or contains) the install destination: + # a --force replacement would otherwise delete the source before it can be + # copied. + try: + source_resolved = package_dir.resolve() + dest_resolved = step_dir.resolve() + except OSError as exc: + raise StepInstallError(f"Failed to resolve step package path: {exc}") from exc + if source_resolved == dest_resolved or dest_resolved.is_relative_to( + source_resolved + ): + raise StepInstallError( + f"Step package source resolves to the install destination: " + f"'{package_dir}'" + ) + + _reject_builtin_collision(step_id) + registry = StepRegistry(project_root) + _check_duplicate(registry, step_id, step_dir, force=force) + + step_meta = validate_step_package(package_dir, step_id) + entry = _build_entry( + step_id, + step_meta, + source=source, + catalog_name=catalog_name, + catalog_metadata=catalog_metadata, + ) + + try: + steps_base_dir.mkdir(parents=True, exist_ok=True) + work_dir = Path( + tempfile.mkdtemp(prefix=_WORK_DIR_PREFIX, dir=steps_base_dir) + ) + except OSError as exc: + raise StepInstallError(f"Failed to create staging directory: {exc}") from exc + + staged_dir = work_dir / "staged" + try: + _copy_package_tree(package_dir, staged_dir) + # Re-validate the complete staged copy: the source may have changed + # while it was copied. + validate_step_package(staged_dir, step_id) + # Recheck the destination immediately before commit (TOCTOU). + _reject_unsafe_destination(step_dir) + _check_duplicate(registry, step_id, step_dir, force=force) + _replace_install( + step_dir, + staged_dir, + registry, + step_id, + entry, + force=force, + ) + finally: + shutil.rmtree(work_dir, ignore_errors=True) + + return entry diff --git a/tests/specify_cli/workflows/step/test_command_add.py b/tests/specify_cli/workflows/step/test_command_add.py index e19c53c6b1..3250057e74 100644 --- a/tests/specify_cli/workflows/step/test_command_add.py +++ b/tests/specify_cli/workflows/step/test_command_add.py @@ -325,11 +325,11 @@ def test_add_rejects_too_many_package_files_before_network( from specify_cli import app from specify_cli.authentication import http as auth_http - from specify_cli.workflows.step import _helpers as step_helpers + from specify_cli.workflows.step import installer from specify_cli.workflows.step.catalog import StepCatalog monkeypatch.chdir(project_dir) - monkeypatch.setattr(step_helpers, "_MAX_STEP_PACKAGE_FILES", 3) + monkeypatch.setattr(installer, "_MAX_STEP_PACKAGE_FILES", 3) monkeypatch.setattr( StepCatalog, "get_step_info", @@ -371,11 +371,11 @@ def test_add_rejects_package_over_cumulative_size_and_cleans_staging( from specify_cli import app from specify_cli.authentication import http as auth_http - from specify_cli.workflows.step import _helpers as step_helpers + from specify_cli.workflows.step import installer from specify_cli.workflows.step.catalog import StepCatalog monkeypatch.chdir(project_dir) - monkeypatch.setattr(step_helpers, "_MAX_STEP_PACKAGE_BYTES", 40) + monkeypatch.setattr(installer, "_MAX_STEP_PACKAGE_BYTES", 40) monkeypatch.setattr( StepCatalog, "get_step_info", @@ -604,3 +604,560 @@ def _fake_open_url(url, timeout=30, redirect_validator=None): assert result.exit_code != 0 assert "empty or non-string URL" in result.output + + +def _write_package(base, type_key="my-step", *, init_body="# init\n"): + package_dir = base / f"{type_key}-pkg" + package_dir.mkdir(parents=True, exist_ok=True) + (package_dir / "step.yml").write_text( + f"step:\n type_key: {type_key}\n name: My Step\n version: 0.1.0\n", + encoding="utf-8", + ) + (package_dir / "__init__.py").write_text(init_body, encoding="utf-8") + return package_dir + + +def _valid_init_body(type_key: str) -> str: + return ( + "from specify_cli.workflows.base import StepBase, StepResult\n\n\n" + "class CustomStep(StepBase):\n" + f" type_key = {type_key!r}\n\n" + " def execute(self, config, context):\n" + " return StepResult(output={'ok': True})\n" + ) + + +def _make_zip(files): + import io + import zipfile + + buffer = io.BytesIO() + with zipfile.ZipFile(buffer, "w") as archive: + for rel, body in files.items(): + data = body.encode("utf-8") if isinstance(body, str) else body + archive.writestr(rel, data) + return buffer.getvalue() + + +def _make_tar_gz(files): + import io + import tarfile + + buffer = io.BytesIO() + with tarfile.open(fileobj=buffer, mode="w:gz") as archive: + for rel, body in files.items(): + data = body.encode("utf-8") if isinstance(body, str) else body + info = tarfile.TarInfo(rel) + info.size = len(data) + archive.addfile(info, io.BytesIO(data)) + return buffer.getvalue() + + +class _ArchiveResponse: + def __init__(self, url, body=b"", content_type=None): + self.url = url + self.body = body + self.content_type = content_type + self.offset = 0 + + def __enter__(self): + return self + + def __exit__(self, exc_type, exc, tb): + return False + + def getheader(self, name): + if name.lower() == "content-type": + return self.content_type + return None + + def geturl(self): + return self.url + + def read(self, size=-1): + if size < 0: + size = len(self.body) - self.offset + chunk = self.body[self.offset : self.offset + size] + self.offset += len(chunk) + return chunk + + +def _valid_archive_files(type_key="my-step"): + return { + "step.yml": f"step:\n type_key: {type_key}\n name: My Step\n", + "__init__.py": "# init\n", + } + + +class TestWorkflowStepAddSources: + def test_dev_installs_and_loads(self, project_dir, tmp_path, monkeypatch): + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.workflows import STEP_REGISTRY, load_custom_steps + + package = _write_package( + tmp_path, type_key="dev-load-step", init_body=_valid_init_body("dev-load-step") + ) + monkeypatch.chdir(project_dir) + runner = CliRunner() + result = runner.invoke( + app, ["workflow", "step", "add", "dev-load-step", "--dev", str(package)] + ) + + assert result.exit_code == 0, result.output + assert "installed" in result.output + installed = project_dir / ".specify" / "workflows" / "steps" / "dev-load-step" + assert (installed / "step.yml").is_file() + + loaded = load_custom_steps(project_dir) + assert "dev-load-step" in loaded + assert "dev-load-step" in STEP_REGISTRY + + def test_dev_install_list_and_remove(self, project_dir, tmp_path, monkeypatch): + from typer.testing import CliRunner + from specify_cli import app + + package = _write_package(tmp_path, type_key="dev-step") + monkeypatch.chdir(project_dir) + runner = CliRunner() + assert ( + runner.invoke( + app, ["workflow", "step", "add", "dev-step", "--dev", str(package)] + ).exit_code + == 0 + ) + + listed = runner.invoke(app, ["workflow", "step", "list"]) + assert listed.exit_code == 0 + assert "dev-step" in listed.output + + removed = runner.invoke(app, ["workflow", "step", "remove", "dev-step"]) + assert removed.exit_code == 0 + assert not ( + project_dir / ".specify" / "workflows" / "steps" / "dev-step" + ).exists() + + def test_dev_rejects_missing_init(self, project_dir, tmp_path, monkeypatch): + from typer.testing import CliRunner + from specify_cli import app + + package = _write_package(tmp_path, type_key="dev-step") + (package / "__init__.py").unlink() + monkeypatch.chdir(project_dir) + + result = CliRunner().invoke( + app, ["workflow", "step", "add", "dev-step", "--dev", str(package)] + ) + assert result.exit_code != 0 + assert "__init__.py" in result.output + + def test_dev_rejects_symlinked_source_root(self, project_dir, tmp_path, monkeypatch): + if not hasattr(os, "symlink"): + pytest.skip("symlinks are unavailable") + from typer.testing import CliRunner + from specify_cli import app + + package = _write_package(tmp_path, type_key="dev-step") + link = tmp_path / "linked" + link.symlink_to(package, target_is_directory=True) + monkeypatch.chdir(project_dir) + + result = CliRunner().invoke( + app, ["workflow", "step", "add", "dev-step", "--dev", str(link)] + ) + assert result.exit_code != 0 + assert "symlink" in result.output.lower() + + def test_dev_and_from_are_mutually_exclusive(self, project_dir, monkeypatch): + from typer.testing import CliRunner + from specify_cli import app + + monkeypatch.chdir(project_dir) + result = CliRunner().invoke( + app, + [ + "workflow", + "step", + "add", + "dev-step", + "--dev", + "somewhere", + "--from", + "https://example.com/pkg.zip", + ], + ) + assert result.exit_code != 0 + assert "mutually exclusive" in result.output + + @pytest.mark.parametrize("option", ["--dev", "--from"]) + def test_empty_source_value_rejected(self, project_dir, monkeypatch, option): + from typer.testing import CliRunner + from specify_cli import app + + monkeypatch.chdir(project_dir) + result = CliRunner().invoke( + app, ["workflow", "step", "add", "dev-step", option, " "] + ) + assert result.exit_code != 0 + + def test_force_replaces_installed_package(self, project_dir, tmp_path, monkeypatch): + from typer.testing import CliRunner + from specify_cli import app + + package = _write_package(tmp_path, type_key="dev-step", init_body="# old\n") + monkeypatch.chdir(project_dir) + runner = CliRunner() + assert ( + runner.invoke( + app, ["workflow", "step", "add", "dev-step", "--dev", str(package)] + ).exit_code + == 0 + ) + + # A second install without --force is rejected. + duplicate = runner.invoke( + app, ["workflow", "step", "add", "dev-step", "--dev", str(package)] + ) + assert duplicate.exit_code != 0 + assert "already installed" in duplicate.output + + (package / "__init__.py").write_text("# new\n", encoding="utf-8") + forced = runner.invoke( + app, + ["workflow", "step", "add", "dev-step", "--dev", str(package), "--force"], + ) + assert forced.exit_code == 0, forced.output + installed = ( + project_dir / ".specify" / "workflows" / "steps" / "dev-step" / "__init__.py" + ) + assert installed.read_text(encoding="utf-8") == "# new\n" + + def test_force_replaces_orphaned_directory(self, project_dir, tmp_path, monkeypatch): + from typer.testing import CliRunner + from specify_cli import app + + orphan = ( + project_dir / ".specify" / "workflows" / "steps" / "dev-step" + ) + orphan.mkdir(parents=True) + (orphan / "step.yml").write_text("step:\n type_key: dev-step\n", encoding="utf-8") + (orphan / "__init__.py").write_text("# old\n", encoding="utf-8") + + package = _write_package(tmp_path, type_key="dev-step", init_body="# new\n") + monkeypatch.chdir(project_dir) + + result = CliRunner().invoke( + app, + ["workflow", "step", "add", "dev-step", "--dev", str(package), "--force"], + ) + assert result.exit_code == 0, result.output + assert (orphan / "__init__.py").read_text(encoding="utf-8") == "# new\n" + + def test_from_denied_confirmation_issues_no_request( + self, project_dir, monkeypatch + ): + import typer + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.authentication import http as auth_http + + monkeypatch.chdir(project_dir) + monkeypatch.setattr(typer, "confirm", lambda *a, **k: False) + monkeypatch.setattr( + auth_http, + "open_url", + lambda *a, **k: (_ for _ in ()).throw( + AssertionError("network request must not be issued") + ), + ) + + result = CliRunner().invoke( + app, + [ + "workflow", + "step", + "add", + "dev-step", + "--from", + "https://example.com/pkg.zip", + ], + ) + assert result.exit_code == 0 + assert "Cancelled" in result.output + assert not ( + project_dir / ".specify" / "workflows" / "steps" / "dev-step" + ).exists() + + @pytest.mark.parametrize( + ("url", "body_factory", "content_type"), + [ + ("https://example.com/pkg.zip", _make_zip, "application/zip"), + ("https://example.com/pkg.tar.gz", _make_tar_gz, "application/gzip"), + ], + ) + def test_from_archive_installs( + self, project_dir, monkeypatch, url, body_factory, content_type + ): + import typer + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.authentication import http as auth_http + + monkeypatch.chdir(project_dir) + monkeypatch.setattr(typer, "confirm", lambda *a, **k: True) + body = body_factory(_valid_archive_files()) + monkeypatch.setattr( + auth_http, + "open_url", + lambda url, timeout=30, redirect_validator=None, extra_headers=None: ( + _ArchiveResponse(url, body, content_type) + ), + ) + + result = CliRunner().invoke( + app, ["workflow", "step", "add", "my-step", "--from", url] + ) + assert result.exit_code == 0, result.output + assert ( + project_dir / ".specify" / "workflows" / "steps" / "my-step" / "step.yml" + ).is_file() + + def test_from_rejects_non_https(self, project_dir, monkeypatch): + from typer.testing import CliRunner + from specify_cli import app + + monkeypatch.chdir(project_dir) + result = CliRunner().invoke( + app, + [ + "workflow", + "step", + "add", + "my-step", + "--from", + "http://example.com/pkg.zip", + ], + ) + assert result.exit_code != 0 + assert "HTTPS" in result.output + + def test_from_rejects_malformed_url(self, project_dir, monkeypatch): + from typer.testing import CliRunner + from specify_cli import app + + monkeypatch.chdir(project_dir) + result = CliRunner().invoke( + app, + [ + "workflow", + "step", + "add", + "my-step", + "--from", + "https://[not-an-ip]/pkg.zip", + ], + ) + assert result.exit_code != 0 + assert "Invalid URL" in result.output + + def test_from_rejects_redirect_to_non_https(self, project_dir, monkeypatch): + import typer + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.authentication import http as auth_http + + monkeypatch.chdir(project_dir) + monkeypatch.setattr(typer, "confirm", lambda *a, **k: True) + monkeypatch.setattr( + auth_http, + "open_url", + lambda url, timeout=30, redirect_validator=None, extra_headers=None: ( + _ArchiveResponse("http://evil.example.com/pkg.zip", b"", "application/zip") + ), + ) + + result = CliRunner().invoke( + app, + [ + "workflow", + "step", + "add", + "my-step", + "--from", + "https://example.com/pkg.zip", + ], + ) + assert result.exit_code != 0 + assert "non-HTTPS" in result.output + + def test_from_rejects_non_archive_body(self, project_dir, monkeypatch): + import typer + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.authentication import http as auth_http + + monkeypatch.chdir(project_dir) + monkeypatch.setattr(typer, "confirm", lambda *a, **k: True) + monkeypatch.setattr( + auth_http, + "open_url", + lambda url, timeout=30, redirect_validator=None, extra_headers=None: ( + _ArchiveResponse(url, b"step:\n type_key: my-step\n", "text/yaml") + ), + ) + + result = CliRunner().invoke( + app, + [ + "workflow", + "step", + "add", + "my-step", + "--from", + "https://example.com/step.yml", + ], + ) + assert result.exit_code != 0 + assert "supported archive" in result.output + + def test_from_rejects_archive_with_unrelated_siblings( + self, project_dir, monkeypatch + ): + import typer + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.authentication import http as auth_http + + files = { + "inner/step.yml": "step:\n type_key: my-step\n", + "inner/__init__.py": "# init\n", + "README.md": "readme\n", + } + body = _make_zip(files) + monkeypatch.chdir(project_dir) + monkeypatch.setattr(typer, "confirm", lambda *a, **k: True) + monkeypatch.setattr( + auth_http, + "open_url", + lambda url, timeout=30, redirect_validator=None, extra_headers=None: ( + _ArchiveResponse(url, body, "application/zip") + ), + ) + + result = CliRunner().invoke( + app, + [ + "workflow", + "step", + "add", + "my-step", + "--from", + "https://example.com/pkg.zip", + ], + ) + assert result.exit_code != 0 + assert "exactly one top-level" in result.output + + def test_from_denied_when_already_installed_errors_before_prompt( + self, project_dir, tmp_path, monkeypatch + ): + import typer + from typer.testing import CliRunner + from specify_cli import app + + package = _write_package(tmp_path, type_key="my-step") + monkeypatch.chdir(project_dir) + runner = CliRunner() + assert ( + runner.invoke( + app, ["workflow", "step", "add", "my-step", "--dev", str(package)] + ).exit_code + == 0 + ) + + prompts = [] + monkeypatch.setattr( + typer, "confirm", lambda *a, **k: prompts.append(True) or True + ) + result = runner.invoke( + app, + [ + "workflow", + "step", + "add", + "my-step", + "--from", + "https://example.com/pkg.zip", + ], + ) + assert result.exit_code != 0 + assert "already installed" in result.output + assert prompts == [] + + def test_direct_python_call_uses_plain_defaults(self, project_dir, monkeypatch): + """The bundle delegate calls ``workflow_step_add(component.id)``.""" + import typer + + from specify_cli import workflow_step_add + from specify_cli.workflows.step.catalog import StepCatalog + + monkeypatch.chdir(project_dir) + monkeypatch.setattr( + StepCatalog, "get_step_info", lambda self, step_id: None + ) + + # A bare positional call must not raise a TypeError from leaking + # typer.Option metadata; it enters catalog mode and exits cleanly. + with pytest.raises(typer.Exit): + workflow_step_add("my-step") + + +class TestWorkflowStepAddEndToEnd: + _WORKFLOW_YAML = """ +schema_version: "1.0" +workflow: + id: "custom-step-wf" + name: "Custom Step Workflow" + version: "1.0.0" +steps: + - id: run-custom + type: dev-step +""" + + _INIT_BODY = """ +from specify_cli.workflows.base import StepBase, StepResult + + +class DevStep(StepBase): + type_key = "dev-step" + + def execute(self, config, context): + return StepResult(output={"ok": True}) +""" + + def test_dev_install_loads_runs_and_removes( + self, project_dir, tmp_path, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.workflows import load_custom_steps + + package = _write_package(tmp_path, type_key="dev-step", init_body=self._INIT_BODY) + monkeypatch.chdir(project_dir) + runner = CliRunner() + + installed = runner.invoke( + app, ["workflow", "step", "add", "dev-step", "--dev", str(package)] + ) + assert installed.exit_code == 0, installed.output + + assert "dev-step" in load_custom_steps(project_dir) + + workflow_file = tmp_path / "custom-step-wf.yml" + workflow_file.write_text(self._WORKFLOW_YAML, encoding="utf-8") + run = runner.invoke(app, ["workflow", "run", str(workflow_file), "--json"]) + assert run.exit_code == 0, run.output + assert "completed" in run.output + + removed = runner.invoke(app, ["workflow", "step", "remove", "dev-step"]) + assert removed.exit_code == 0 diff --git a/tests/specify_cli/workflows/step/test_command_info.py b/tests/specify_cli/workflows/step/test_command_info.py index 663b12be47..b63d5f8f78 100644 --- a/tests/specify_cli/workflows/step/test_command_info.py +++ b/tests/specify_cli/workflows/step/test_command_info.py @@ -61,3 +61,47 @@ def test_info_escapes_missing_step_id(self, project_dir, monkeypatch): assert result.exit_code == 1, result.output assert step_id in result.output + + def test_info_prints_local_source(self, project_dir, monkeypatch): + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.workflows.step.catalog import StepRegistry + + monkeypatch.chdir(project_dir) + monkeypatch.setattr( + StepRegistry, + "get", + lambda _registry, step_id: { + "name": "Local Step", + "version": "1.0.0", + "source": "local", + }, + ) + + result = CliRunner().invoke(app, ["workflow", "step", "info", "local-step"]) + + assert result.exit_code == 0, result.output + assert "Source:" in result.output + assert "local" in result.output + + def test_info_prints_catalog_source_with_name(self, project_dir, monkeypatch): + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.workflows.step.catalog import StepRegistry + + monkeypatch.chdir(project_dir) + monkeypatch.setattr( + StepRegistry, + "get", + lambda _registry, step_id: { + "name": "Catalog Step", + "version": "1.0.0", + "source": "catalog", + "catalog_name": "default", + }, + ) + + result = CliRunner().invoke(app, ["workflow", "step", "info", "catalog-step"]) + + assert result.exit_code == 0, result.output + assert "catalog (default)" in result.output diff --git a/tests/specify_cli/workflows/step/test_installer.py b/tests/specify_cli/workflows/step/test_installer.py new file mode 100644 index 0000000000..ecbacc4422 --- /dev/null +++ b/tests/specify_cli/workflows/step/test_installer.py @@ -0,0 +1,626 @@ +"""Domain-focused tests for the workflow step package installer.""" + +from __future__ import annotations + +import json +import os +from pathlib import Path + +import pytest + +from specify_cli.workflows.step import installer + + +def _write_package( + package_dir: Path, type_key: str = "my-step", *, init_body: str = "# init\n" +) -> Path: + package_dir.mkdir(parents=True, exist_ok=True) + (package_dir / "step.yml").write_text( + f"step:\n type_key: {type_key}\n name: My Step\n version: 0.1.0\n", + encoding="utf-8", + ) + (package_dir / "__init__.py").write_text(init_body, encoding="utf-8") + return package_dir + + +def _steps_dir(project_dir: Path) -> Path: + return project_dir / ".specify" / "workflows" / "steps" + + +def _register(project_dir: Path, step_id: str, **overrides) -> None: + from specify_cli.workflows.step.catalog import StepRegistry + + entry = { + "name": "My Step", + "version": "0.1.0", + "type_key": step_id, + "source": "catalog", + "catalog_name": "default", + } + entry.update(overrides) + StepRegistry(project_dir).add(step_id, entry) + + +def _registry_entry(project_dir: Path, step_id: str) -> dict: + path = _steps_dir(project_dir) / "step-registry.json" + return json.loads(path.read_text(encoding="utf-8"))["steps"][step_id] + + +# --------------------------------------------------------------------------- +# Step id validation +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("step_id", ["my-step", "my_step", "step2", "a.b", "Step"]) +def test_validate_step_id_accepts_normal(step_id): + installer.validate_step_id(step_id) + + +@pytest.mark.parametrize( + "step_id", + [ + "", + " ", + " padded", + "padded ", + "a/b", + "a\\b", + ".", + "..", + ".hidden", + ".cache", + "step-registry.json", + "con", + "nul", + "com1", + "a:b", + "a*b", + "a= 2: + raise installer.StepInstallError("staged copy invalid") + return real_validate(package_dir, step_id) + + monkeypatch.setattr(installer, "validate_step_package", _validate) + + with pytest.raises(installer.StepInstallError): + installer.install_step_package( + project_dir, "my-step", new_pkg, source="local", force=True + ) + + assert (_steps_dir(project_dir) / "my-step" / "__init__.py").read_text( + encoding="utf-8" + ) == "# old\n" + + +def test_force_registry_failure_warns_reinstall( + tmp_path, project_dir, monkeypatch +): + from specify_cli.workflows.step.catalog import StepRegistry, StepValidationError + + _write_package(_steps_dir(project_dir) / "my-step", init_body="# old\n") + _register(project_dir, "my-step") + new_pkg = _write_package(tmp_path / "pkg", init_body="# new\n") + + def _boom(self, step_id, metadata): + raise StepValidationError("disk full") + + monkeypatch.setattr(StepRegistry, "add", _boom) + + with pytest.raises(installer.StepInstallError) as exc: + installer.install_step_package( + project_dir, "my-step", new_pkg, source="local", force=True + ) + assert "reinstall" in str(exc.value).lower() + assert (_steps_dir(project_dir) / "my-step" / "__init__.py").read_text( + encoding="utf-8" + ) == "# new\n" + + +def test_force_removal_failure_warns_reinstall(tmp_path, project_dir, monkeypatch): + target = _steps_dir(project_dir) / "my-step" + _write_package(target, init_body="# old\n") + _register(project_dir, "my-step") + new_pkg = _write_package(tmp_path / "pkg", init_body="# new\n") + + real_rmtree = installer.shutil.rmtree + + def _rmtree(path, *args, **kwargs): + if Path(path) == target: + raise OSError("cannot remove") + return real_rmtree(path, *args, **kwargs) + + monkeypatch.setattr(installer.shutil, "rmtree", _rmtree) + + with pytest.raises(installer.StepInstallError) as exc: + installer.install_step_package( + project_dir, "my-step", new_pkg, source="local", force=True + ) + assert "reinstall" in str(exc.value).lower() + assert (target / "__init__.py").read_text(encoding="utf-8") == "# old\n" + + +def test_force_publication_failure_warns_reinstall(tmp_path, project_dir, monkeypatch): + target = _steps_dir(project_dir) / "my-step" + _write_package(target, init_body="# old\n") + _register(project_dir, "my-step") + new_pkg = _write_package(tmp_path / "pkg", init_body="# new\n") + + real_replace = installer.os.replace + + def _replace(src, dst, *args, **kwargs): + if Path(dst) == target: + raise OSError("rename failed") + return real_replace(src, dst, *args, **kwargs) + + monkeypatch.setattr(installer.os, "replace", _replace) + + with pytest.raises(installer.StepInstallError) as exc: + installer.install_step_package( + project_dir, "my-step", new_pkg, source="local", force=True + ) + assert "reinstall" in str(exc.value).lower() + + +def test_force_replaces_orphaned_directory(tmp_path, project_dir): + orphan = _write_package(_steps_dir(project_dir) / "my-step", init_body="# old\n") + assert orphan.is_dir() + + new_pkg = _write_package(tmp_path / "pkg", init_body="# new\n") + installer.install_step_package( + project_dir, "my-step", new_pkg, source="local", force=True + ) + + assert (_steps_dir(project_dir) / "my-step" / "__init__.py").read_text( + encoding="utf-8" + ) == "# new\n" + + +def test_no_backup_artifacts_after_force(tmp_path, project_dir): + _write_package(_steps_dir(project_dir) / "my-step", init_body="# old\n") + _register(project_dir, "my-step") + new_pkg = _write_package(tmp_path / "pkg", init_body="# new\n") + + installer.install_step_package( + project_dir, "my-step", new_pkg, source="local", force=True + ) + + names = sorted(path.name for path in _steps_dir(project_dir).iterdir()) + assert names == ["my-step", "step-registry.json"] + + +def test_loader_does_not_discover_staging_package(project_dir): + from specify_cli.workflows import load_custom_steps + + staging = _steps_dir(project_dir) / ".speckit-step-install-abc" / "staged" + _write_package(staging, type_key="staged-only-step") + + loaded = load_custom_steps(project_dir) + assert "staged-only-step" not in loaded + + +def test_builtin_collision_uses_immutable_snapshot(tmp_path, project_dir, monkeypatch): + from specify_cli.workflows import BUILTIN_STEP_TYPES, STEP_REGISTRY + + monkeypatch.delitem(STEP_REGISTRY, "shell", raising=False) + assert "shell" in BUILTIN_STEP_TYPES + + pkg = _write_package(tmp_path / "pkg", type_key="shell") + with pytest.raises(installer.StepInstallError, match="built-in"): + installer.install_step_package(project_dir, "shell", pkg, source="local") + + +def test_check_installable_exposes_duplicate_before_install(tmp_path, project_dir): + pkg = _write_package(tmp_path / "pkg") + installer.check_installable(project_dir, "my-step") + installer.install_step_package(project_dir, "my-step", pkg, source="local") + + with pytest.raises(installer.StepInstallError, match="already installed"): + installer.check_installable(project_dir, "my-step") + # force permits the preflight. + installer.check_installable(project_dir, "my-step", force=True) + + +def _tree(root: Path) -> dict[str, bytes]: + return { + str(path.relative_to(root)): path.read_bytes() + for path in sorted(root.rglob("*")) + if path.is_file() + } + + +def test_all_sources_share_tree_and_metadata(tmp_path): + pkg = _write_package(tmp_path / "pkg", type_key="parity-step") + expected_tree = _tree(pkg) + + entries: dict[str, dict] = {} + trees: dict[str, dict] = {} + for source in ("catalog", "local", "url"): + project = tmp_path / f"proj-{source}" + project.mkdir() + entries[source] = installer.install_step_package( + project, + "parity-step", + pkg, + source=source, + catalog_name="default" if source == "catalog" else "", + catalog_metadata={"name": "Parity"} if source == "catalog" else None, + ) + trees[source] = _tree(_steps_dir(project) / "parity-step") + + assert trees["catalog"] == trees["local"] == trees["url"] == expected_tree + + for source, entry in entries.items(): + assert entry["source"] == source + assert entry["type_key"] == "parity-step" + if source == "catalog": + assert entry["catalog_name"] == "default" + else: + assert "catalog_name" not in entry + + +def test_all_sources_share_identity_rejection(tmp_path): + pkg = _write_package(tmp_path / "pkg", type_key="wrong-step") + for index, source in enumerate(("catalog", "local", "url")): + project = tmp_path / f"proj-{index}" + project.mkdir() + with pytest.raises(installer.StepInstallError, match="does not match"): + installer.install_step_package( + project, "parity-step", pkg, source=source + ) From 4560d2b515ab571aa96b0f58bea070f76867619f Mon Sep 17 00:00:00 2001 From: Markus Date: Fri, 25 Sep 2026 19:06:08 +0200 Subject: [PATCH 02/16] fix(workflows): harden local step installation Assisted-by: OpenCode (model: gpt-5.6-terra, autonomous) Assisted-by: OpenCode (model: deepseek-v4.1-flash, autonomous) --- docs/reference/workflows.md | 18 +- src/specify_cli/workflows/__init__.py | 22 +- .../workflows/step/catalog/_domain.py | 87 +++++-- src/specify_cli/workflows/step/command_add.py | 90 +++++-- src/specify_cli/workflows/step/installer.py | 227 +++++++++++++++--- tests/specify_cli/bundles/test_primitives.py | 26 +- .../step/catalog/test_command_list.py | 4 +- .../workflows/step/catalog/test_registry.py | 33 +++ .../workflows/step/test_command_add.py | 97 +++++++- .../workflows/step/test_command_info.py | 11 +- .../workflows/step/test_command_list.py | 8 +- .../workflows/step/test_command_search.py | 8 +- .../workflows/step/test_installer.py | 180 ++++++++++++++ .../workflows/test_custom_steps.py | 49 ++++ 14 files changed, 761 insertions(+), 99 deletions(-) create mode 100644 tests/specify_cli/workflows/step/catalog/test_registry.py create mode 100644 tests/specify_cli/workflows/test_custom_steps.py diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index c063c5ccea..4cf68316c1 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -652,12 +652,18 @@ Every source is validated identically before anything is committed: directory are rejected — including inside excluded directories. - `.git`, `__pycache__`, and `.DS_Store` entries are excluded from the copy and from the limits. -- A package may contain at most **512 files** and **50 MiB** in total. -- `__init__.py` is **not imported** during installation; it is loaded only when - the step runs. - -> **Security note:** Installing a custom step runs its Python with **your** -> privileges. Only install step packages from sources you trust. +- The installed-package policy permits at most **512 retained files** and + **50 MiB** of retained content. Excluded entries do not consume this budget. +- Archive URLs also pass transport/extraction safety limits before package + validation: at most 512 archive entries, 50 MiB downloaded or extracted, and + 10 MiB per archive member. Catalog files have a 50 MiB per-response bound. +- Installation validates and copies the package but does **not** import or + execute `__init__.py`. Installed custom step modules are loaded during startup + of `workflow add`, `workflow run`, and `workflow resume`, before any particular + custom step necessarily executes. + +> **Security note:** Loading a custom step runs its Python with **your** +> privileges. Only install and retain step packages from sources you trust. #### Listing, running, and removing diff --git a/src/specify_cli/workflows/__init__.py b/src/specify_cli/workflows/__init__.py index 2bb3de56a5..baa3b8fa09 100644 --- a/src/specify_cli/workflows/__init__.py +++ b/src/specify_cli/workflows/__init__.py @@ -75,11 +75,27 @@ def _register_builtin_steps() -> None: # The step types Spec Kit ships, snapshotted before any community step can be # loaded. ``load_custom_steps`` adds project-installed ids to the process-global -# ``STEP_REGISTRY`` and never removes them, so ``STEP_REGISTRY`` cannot answer +# ``STEP_REGISTRY`` and refreshes them for each project, so it cannot answer # "is this bundled with Spec Kit?" in a long-lived process: a step loaded for one # project would look built-in for the next. Callers that need the immutable set # (e.g. the bundler's reference checker) must use this instead. BUILTIN_STEP_TYPES: frozenset[str] = frozenset(STEP_REGISTRY) +_CUSTOM_STEP_MODULES: set[str] = set() + + +def _unload_custom_steps() -> None: + """Clear custom registrations and synthetic imports from a prior project.""" + import sys + + for type_key in tuple(STEP_REGISTRY): + if type_key not in BUILTIN_STEP_TYPES: + del STEP_REGISTRY[type_key] + for module_name in _CUSTOM_STEP_MODULES: + sys.modules.pop(module_name, None) + prefix = module_name + "." + for loaded_name in [name for name in sys.modules if name.startswith(prefix)]: + sys.modules.pop(loaded_name, None) + _CUSTOM_STEP_MODULES.clear() def load_custom_steps(project_root: Path) -> list[str]: @@ -97,6 +113,7 @@ def load_custom_steps(project_root: Path) -> list[str]: import re as _re import sys as _sys + _unload_custom_steps() steps_dir = Path(project_root) / ".specify" / "workflows" / "steps" # Defense-in-depth: refuse to execute step code from a symlinked @@ -192,6 +209,7 @@ def load_custom_steps(project_root: Path) -> list[str]: _register_step(step_class()) loaded.append(type_key) registered = True + _CUSTOM_STEP_MODULES.add(module_name) finally: # If the step wasn't successfully registered (failed import, # no matching StepBase subclass, or registration error), remove @@ -206,7 +224,7 @@ def load_custom_steps(project_root: Path) -> list[str]: k for k in _sys.modules if k.startswith(submodule_prefix) ]: _sys.modules.pop(_mod_key, None) - except Exception: # noqa: BLE001 + except Exception: # noqa: BLE001, S112 # Silently skip broken step packages at load time continue diff --git a/src/specify_cli/workflows/step/catalog/_domain.py b/src/specify_cli/workflows/step/catalog/_domain.py index 08a1b22f57..bc166562bc 100644 --- a/src/specify_cli/workflows/step/catalog/_domain.py +++ b/src/specify_cli/workflows/step/catalog/_domain.py @@ -5,15 +5,17 @@ import hashlib import json import os +import stat +import tempfile import time from dataclasses import dataclass +from datetime import UTC from pathlib import Path from typing import Any import yaml from ...._download_security import ( - MAX_JSON_CATALOG_BYTES as MAX_JSON_CATALOG_BYTES, read_response_limited, ) @@ -111,48 +113,104 @@ def _load(self) -> dict[str, Any]: return default_registry def save(self) -> None: - """Persist registry to disk. - - Raises ``StepValidationError`` with a clear message on filesystem - errors (read-only fs, permission denied, ...) so callers can surface - a clean error to the user rather than an unhandled ``OSError``. - """ + """Persist registry atomically without truncating an existing file.""" if self._has_symlinked_parent() or self.registry_path.is_symlink(): raise StepValidationError( "Refusing to write step registry through a symlinked path." ) + fd = -1 + tmp: str | None = None try: self.steps_dir.mkdir(parents=True, exist_ok=True) - with open(self.registry_path, "w", encoding="utf-8") as f: + fd, tmp = tempfile.mkstemp( + dir=str(self.registry_path.parent), + prefix=f".{self.registry_path.name}.", + suffix=".tmp", + ) + # Keep the exclusive descriptor open while writing and checking the + # path so a replaced temporary file can never be committed. + with os.fdopen(os.dup(fd), "w", encoding="utf-8") as f: json.dump(self.data, f, indent=2) - except OSError as exc: + f.flush() + os.fsync(f.fileno()) + try: + if self.registry_path.exists(): + existing = self.registry_path.stat(follow_symlinks=False) + if stat.S_ISREG(existing.st_mode) and hasattr(os, "fchmod"): + os.fchmod(fd, stat.S_IMODE(existing.st_mode)) + if stat.S_ISREG(existing.st_mode) and hasattr(os, "fchown"): + try: + os.fchown(fd, existing.st_uid, existing.st_gid) + except PermissionError: + pass + except OSError: + # Persisting valid data is more important than preserving mode + # or ownership metadata when that best-effort operation fails. + pass + staged = os.stat(tmp, follow_symlinks=False) + opened = os.fstat(fd) + if ( + not stat.S_ISREG(staged.st_mode) + or staged.st_dev != opened.st_dev + or staged.st_ino != opened.st_ino + ): + raise OSError("Staged step registry changed before commit") + os.close(fd) + fd = -1 + os.replace(tmp, self.registry_path) + tmp = None + except (OSError, TypeError, ValueError) as exc: raise StepValidationError( f"Failed to write step registry at {self.registry_path}: {exc}" ) from exc + finally: + if fd >= 0: + try: + os.close(fd) + except OSError: + pass + if tmp is not None: + try: + os.unlink(tmp) + except OSError: + pass def add(self, step_id: str, metadata: dict[str, Any]) -> None: """Add or update an installed step entry.""" import copy - from datetime import datetime, timezone + from datetime import datetime raw_existing = self.data["steps"].get(step_id) + had_entry = step_id in self.data["steps"] # Corrupted-but-parseable registries may hold non-dict entries; treat # them as absent rather than crashing on existing.get() (mirrors # WorkflowRegistry.add). existing = raw_existing if isinstance(raw_existing, dict) else {} metadata_to_store = copy.deepcopy(metadata) metadata_to_store["installed_at"] = existing.get( - "installed_at", datetime.now(timezone.utc).isoformat() + "installed_at", datetime.now(UTC).isoformat() ) - metadata_to_store["updated_at"] = datetime.now(timezone.utc).isoformat() + metadata_to_store["updated_at"] = datetime.now(UTC).isoformat() self.data["steps"][step_id] = metadata_to_store - self.save() + try: + self.save() + except (StepValidationError, TypeError, ValueError): + if had_entry: + self.data["steps"][step_id] = raw_existing + else: + del self.data["steps"][step_id] + raise def remove(self, step_id: str) -> bool: """Remove an installed step entry. Returns True if found.""" if step_id in self.data["steps"]: + removed_entry = self.data["steps"][step_id] del self.data["steps"][step_id] - self.save() + try: + self.save() + except (StepValidationError, TypeError, ValueError): + self.data["steps"][step_id] = removed_entry + raise return True return False @@ -420,6 +478,7 @@ def _fetch_single_catalog( pass from urllib.parse import urlparse + from specify_cli.authentication.http import open_url as _open_url def _validate_url(url: str) -> None: diff --git a/src/specify_cli/workflows/step/command_add.py b/src/specify_cli/workflows/step/command_add.py index 3aba60bce3..5738533e1a 100644 --- a/src/specify_cli/workflows/step/command_add.py +++ b/src/specify_cli/workflows/step/command_add.py @@ -14,6 +14,8 @@ from . import _helpers as step_helpers from . import step_app +_MAX_STEP_CATALOG_RESPONSE_BYTES = 50 * 1024 * 1024 + def _cleanup_download_tmp_path(tmp_path: cli.Path | None) -> None: """Best-effort unlink of a partially-downloaded step archive temp file. @@ -35,7 +37,11 @@ def _cleanup_download_tmp_path(tmp_path: cli.Path | None) -> None: def _print_installed(step_id: str, entry: dict) -> None: step_name = entry.get("name") or step_id - cli.console.print(f"[green]✓[/green] Step type '{step_name}' ({step_id}) installed") + cli.console.print( + "[green]✓[/green] Step type " + f"'{cli._escape_markup(str(step_name))}' " + f"({cli._escape_markup(str(step_id))}) installed" + ) cli.console.print( " Use [cyan]specify workflow step list[/cyan] to verify the installation." ) @@ -126,6 +132,8 @@ def _install_from_url( download_url = from_url extra_headers = None tmp_path: cli.Path | None = None + extract_tmp: tempfile.TemporaryDirectory[str] | None = None + committed = False try: resolved_url = _resolve_gh_asset( from_url, @@ -154,16 +162,30 @@ def _install_from_url( if hasattr(resp, "getheader") else None ) - archive_format = ( - cli.archive_format_from_name(final_url) - or cli.archive_format_from_name(from_url) - or cli.archive_format_from_content_type(content_type) - ) + declarations = [ + ("requested URL", from_url, cli.archive_format_from_name(from_url)), + ("final URL", final_url, cli.archive_format_from_name(final_url)), + ( + "Content-Type", + content_type or "", + cli.archive_format_from_content_type(content_type), + ), + ] + recognized = [item for item in declarations if item[2] is not None] + archive_format = recognized[0][2] if recognized else None if archive_format is None: raise installer.StepInstallError( "URL does not reference a supported archive " "(.zip, .tar.gz, or .tgz)" ) + if any(item[2] != archive_format for item in recognized): + details = ", ".join( + f"{label} declares {declared}" + for label, _value, declared in recognized + ) + raise installer.StepInstallError( + f"Archive format mismatch: {cli._escape_markup(details)}" + ) downloaded = cli.read_response_limited( resp, error_type=ValueError, @@ -176,15 +198,16 @@ def _install_from_url( tmp_path = cli.Path(tmp.name) tmp.write(downloaded) - with tempfile.TemporaryDirectory( - prefix="speckit-step-archive-" - ) as extract_dir: - extracted_root = cli.Path(extract_dir) + extract_tmp = tempfile.TemporaryDirectory(prefix="speckit-step-archive-") + extracted_root = cli.Path(extract_tmp.name) + try: # safe_extract_archive re-detects and confirms the archive bytes. cli.safe_extract_archive( tmp_path, extracted_root, - source_name=final_url, + source_name=next( + value for _label, value, declared in recognized if declared is not None + ), content_type=content_type, ) package_root = installer.resolve_package_root(extracted_root) @@ -195,6 +218,22 @@ def _install_from_url( source="url", force=force, ) + committed = True + finally: + try: + extract_tmp.cleanup() + except OSError as cleanup_exc: + if committed: + cli.console.print( + "[yellow]Warning:[/yellow] Could not remove temporary step " + f"archive directory: {cli._escape_markup(str(cleanup_exc))} " + f"(path: {cli._escape_markup(extract_tmp.name)})" + ) + elif __import__("sys").exc_info()[0] is None: + raise installer.StepInstallError( + "Failed to remove temporary step archive directory: " + f"{cleanup_exc}" + ) from cleanup_exc except cli.typer.Exit: raise except installer.StepInstallError: @@ -322,10 +361,17 @@ def _safe_fetch(url: str) -> bytes: final_url = resp.geturl() if not cli.is_https_or_localhost_http(final_url): raise ValueError(f"Redirect to non-HTTPS URL: {final_url}") - return cli._read_response_within_limit(resp) + return cli.read_response_limited( + resp, + max_bytes=_MAX_STEP_CATALOG_RESPONSE_BYTES, + error_type=ValueError, + label="step package response", + ) - with tempfile.TemporaryDirectory(prefix="speckit-step-package-") as package_tmp: - package_dir = cli.Path(package_tmp) + package_tmp = tempfile.TemporaryDirectory(prefix="speckit-step-package-") + package_dir = cli.Path(package_tmp.name) + committed = False + try: try: step_yml_content = _safe_fetch(step_yml_url) init_py_content = _safe_fetch(init_url) @@ -408,6 +454,22 @@ def _safe_fetch(url: str) -> bytes: catalog_metadata=info, force=force, ) + committed = True + finally: + try: + package_tmp.cleanup() + except OSError as cleanup_exc: + if committed: + cli.console.print( + "[yellow]Warning:[/yellow] Could not remove temporary step " + f"package directory: {cli._escape_markup(str(cleanup_exc))} " + f"(path: {cli._escape_markup(package_tmp.name)})" + ) + elif __import__("sys").exc_info()[0] is None: + raise installer.StepInstallError( + "Failed to remove temporary step package directory: " + f"{cleanup_exc}" + ) from cleanup_exc _print_installed(step_id, entry) diff --git a/src/specify_cli/workflows/step/installer.py b/src/specify_cli/workflows/step/installer.py index 1e37e2b57c..5c53b17d20 100644 --- a/src/specify_cli/workflows/step/installer.py +++ b/src/specify_cli/workflows/step/installer.py @@ -11,6 +11,7 @@ from __future__ import annotations +import contextlib import os import shutil import stat @@ -66,12 +67,73 @@ ) _WINDOWS_INVALID_CHARS: frozenset[str] = frozenset('<>:"|?*') +_SOURCES: frozenset[str] = frozenset({"catalog", "local", "url"}) class StepInstallError(Exception): """User-facing step package install/validation failure.""" +@contextlib.contextmanager +def _step_install_transaction(project_root: Path): + """Serialize step directory swaps with their registry updates.""" + from ...shared_infra import _ensure_safe_shared_directory + + lock_dir = Path(project_root) / ".specify" + try: + _ensure_safe_shared_directory( + Path(project_root), lock_dir, context="step install lock directory" + ) + except ValueError as exc: + raise StepInstallError(str(exc)) from exc + lock_file = lock_dir / ".step-install.lock" + if lock_file.is_symlink(): + raise StepInstallError(f"Refusing to use symlinked step install lock: {lock_file}") + + flags = os.O_RDWR | os.O_CREAT + flags |= getattr(os, "O_NOFOLLOW", 0) + flags |= getattr(os, "O_CLOEXEC", 0) + try: + fd = os.open(lock_file, flags, 0o600) + except OSError as exc: + raise StepInstallError(f"Failed to open step install lock: {exc}") from exc + try: + if lock_file.is_symlink(): + raise StepInstallError( + f"Refusing to use symlinked step install lock: {lock_file}" + ) + if os.name == "nt": + import errno + import msvcrt + import time + + if os.fstat(fd).st_size == 0: + os.write(fd, b"\0") + while True: + os.lseek(fd, 0, os.SEEK_SET) + try: + msvcrt.locking(fd, msvcrt.LK_NBLCK, 1) + break + except OSError as exc: + if exc.errno not in (errno.EACCES, errno.EDEADLK): + raise + time.sleep(0.05) + else: + import fcntl + + fcntl.flock(fd, fcntl.LOCK_EX) + yield + except StepInstallError: + raise + except OSError as exc: + raise StepInstallError(f"Failed to lock step installation: {exc}") from exc + finally: + try: + os.close(fd) + except OSError: + pass + + # --------------------------------------------------------------------------- # Step id + base directory validation # --------------------------------------------------------------------------- @@ -396,19 +458,51 @@ def _build_entry( catalog_name: str, catalog_metadata: Mapping[str, Any] | None, ) -> dict[str, Any]: + if source not in _SOURCES: + raise StepInstallError( + "Step install source must be one of: catalog, local, url" + ) + if source == "catalog" and not isinstance(catalog_name, str): + raise StepInstallError("Catalog step install requires a string catalog name") + if catalog_metadata is not None and not isinstance(catalog_metadata, Mapping): + raise StepInstallError("Catalog step metadata must be a mapping") catalog_metadata = catalog_metadata or {} + + def _string_value(metadata: Mapping[str, Any], field: str) -> str | None: + value = metadata.get(field) + if value is None: + return None + if not isinstance(value, str): + raise StepInstallError( + f"step metadata '{field}' must be a string when present" + ) + return value + + type_key = _string_value(step_meta, "type_key") + if not type_key: + raise StepInstallError("step.yml missing 'step.type_key' field") + package_values = { + field: _string_value(step_meta, field) + for field in ("name", "version", "description", "author") + } + catalog_values = { + field: _string_value(catalog_metadata, field) + for field in ("name", "version", "description", "author") + } entry: dict[str, Any] = { - "name": catalog_metadata.get("name") - or step_meta.get("name") - or step_id, - "version": catalog_metadata.get("version") - or step_meta.get("version") - or "0.0.0", - "description": catalog_metadata.get( - "description", step_meta.get("description", "") + "name": catalog_values["name"] or package_values["name"] or step_id, + "version": catalog_values["version"] or package_values["version"] or "0.0.0", + "description": ( + catalog_values["description"] + if catalog_values["description"] is not None + else package_values["description"] or "" ), - "author": catalog_metadata.get("author", step_meta.get("author", "")), - "type_key": step_meta["type_key"], + "author": ( + catalog_values["author"] + if catalog_values["author"] is not None + else package_values["author"] or "" + ), + "type_key": type_key, "source": source, } if source == "catalog": @@ -451,12 +545,7 @@ def _copy(current: Path, destination: Path) -> None: if stat.S_ISDIR(mode): _copy(Path(entry.path), target) elif stat.S_ISREG(mode): - try: - shutil.copyfile(entry.path, target) - except OSError as exc: - raise StepInstallError( - f"Failed to stage step package: {exc}" - ) from exc + _copy_regular_file(entry.path, target, mode) else: raise StepInstallError( f"Step package contains unsupported file: {entry.path}" @@ -465,6 +554,37 @@ def _copy(current: Path, destination: Path) -> None: _copy(source_dir, target_dir) +def _copy_regular_file(source: str, target: Path, expected_mode: int) -> None: + """Copy an inspected regular file without following a late symlink swap.""" + flags = os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0) + try: + fd = os.open(source, flags) + except OSError as exc: + raise StepInstallError(f"Failed to stage step package: {exc}") from exc + try: + opened = os.fstat(fd) + source_state = os.stat(source, follow_symlinks=False) + if ( + not stat.S_ISREG(opened.st_mode) + or not stat.S_ISREG(source_state.st_mode) + or opened.st_dev != source_state.st_dev + or opened.st_ino != source_state.st_ino + or stat.S_IFMT(source_state.st_mode) != stat.S_IFMT(expected_mode) + ): + raise StepInstallError( + f"Step package file changed while staging: {source}" + ) + with os.fdopen(fd, "rb", closefd=False) as source_file, target.open("xb") as target_file: + shutil.copyfileobj(source_file, target_file) + except OSError as exc: + raise StepInstallError(f"Failed to stage step package: {exc}") from exc + finally: + try: + os.close(fd) + except OSError: + pass + + def _replace_install( step_dir: Path, staged_dir: Path, @@ -478,6 +598,11 @@ def _replace_install( from .catalog import StepValidationError if step_dir.exists(): + if not force: + raise StepInstallError( + f"Step directory already exists at '{step_dir}'. Remove it manually " + f"or use: [cyan]specify workflow step remove {step_id}[/cyan]" + ) # --force replacement: the replacement is fully staged and validated, # so it is safe to remove the previous installation now. try: @@ -501,8 +626,8 @@ def _replace_install( except (StepValidationError, OSError, TypeError, ValueError) as exc: raise StepInstallError( f"Failed to update the step registry for '{step_id}': {exc}. The " - "step directory was replaced but is not registered; reinstall " - "from the original source with --force." + "package was replaced but the registry metadata was not updated; " + "reinstall from the original source with --force." ) from exc return @@ -518,7 +643,14 @@ def _replace_install( except (StepValidationError, OSError, TypeError, ValueError) as exc: # Fresh install: roll back the just-published directory so the system # is not left with an unregistered step package on disk. - shutil.rmtree(step_dir, ignore_errors=True) + try: + shutil.rmtree(step_dir) + except OSError as cleanup_exc: + raise StepInstallError( + f"Failed to update the step registry for '{step_id}': {exc}. " + f"The unregistered package remains at '{step_dir}' because rollback " + f"failed: {cleanup_exc}. Remove it manually before reinstalling." + ) from exc raise StepInstallError(str(exc)) from exc @@ -572,10 +704,11 @@ def install_step_package( registry = StepRegistry(project_root) _check_duplicate(registry, step_id, step_dir, force=force) - step_meta = validate_step_package(package_dir, step_id) - entry = _build_entry( + # Validate source and all caller-controlled metadata before creating any + # project directories. The staged metadata is used for the final entry. + _build_entry( step_id, - step_meta, + validate_step_package(package_dir, step_id), source=source, catalog_name=catalog_name, catalog_metadata=catalog_metadata, @@ -590,23 +723,51 @@ def install_step_package( raise StepInstallError(f"Failed to create staging directory: {exc}") from exc staged_dir = work_dir / "staged" + committed = False try: _copy_package_tree(package_dir, staged_dir) # Re-validate the complete staged copy: the source may have changed # while it was copied. - validate_step_package(staged_dir, step_id) - # Recheck the destination immediately before commit (TOCTOU). - _reject_unsafe_destination(step_dir) - _check_duplicate(registry, step_id, step_dir, force=force) - _replace_install( - step_dir, - staged_dir, - registry, + staged_meta = validate_step_package(staged_dir, step_id) + entry = _build_entry( step_id, - entry, - force=force, + staged_meta, + source=source, + catalog_name=catalog_name, + catalog_metadata=catalog_metadata, ) + # Serialize destination and registry changes. Source downloads/copying + # stay outside the lock, but all state that can conflict is reloaded and + # checked again immediately before publication. + with _step_install_transaction(project_root): + locked_base_dir = resolve_steps_base_dir(project_root) + if locked_base_dir != steps_base_dir: + raise StepInstallError( + "Step directory changed while staging; reinstall from the original source" + ) + step_dir = _resolve_step_dir(locked_base_dir, step_id) + _reject_unsafe_destination(step_dir) + _reject_builtin_collision(step_id) + registry = StepRegistry(project_root) + _check_duplicate(registry, step_id, step_dir, force=force) + _replace_install( + step_dir, + staged_dir, + registry, + step_id, + entry, + force=force, + ) + committed = True finally: - shutil.rmtree(work_dir, ignore_errors=True) + try: + shutil.rmtree(work_dir) + except OSError as cleanup_exc: + # The staged directory is private and cannot be loaded as a step, + # but callers still need an actionable residual-path diagnostic. + if work_dir.exists() and not committed and os.sys.exc_info()[0] is None: + raise StepInstallError( + f"Failed to remove staging directory '{work_dir}': {cleanup_exc}" + ) from cleanup_exc return entry diff --git a/tests/specify_cli/bundles/test_primitives.py b/tests/specify_cli/bundles/test_primitives.py index a3b4d83f45..8cbcbf85a2 100644 --- a/tests/specify_cli/bundles/test_primitives.py +++ b/tests/specify_cli/bundles/test_primitives.py @@ -12,8 +12,8 @@ import pytest from specify_cli.bundler import BundlerError -from specify_cli.bundles.manifest import ComponentRef from specify_cli.bundles.adapters import DefaultPrimitiveInstaller +from specify_cli.bundles.manifest import ComponentRef from specify_cli.bundles.primitives import ( _ExtensionKindManager, _PresetKindManager, @@ -64,6 +64,22 @@ def test_offline_step_refuses_without_network(tmp_path: Path): manager.install(_component("steps")) +def test_step_manager_delegates_catalog_install_from_bundle_root(tmp_path, monkeypatch): + import specify_cli + + calls: list[tuple[str, Path]] = [] + + def _add(step_id: str) -> None: + calls.append((step_id, Path.cwd())) + + monkeypatch.setattr(specify_cli, "workflow_step_add", _add) + manager = _StepKindManager(tmp_path, allow_network=True) + + manager.install(_component("steps", "catalog-step")) + + assert calls == [("catalog-step", tmp_path)] + + def test_default_installer_threads_allow_network(tmp_path: Path): installer = DefaultPrimitiveInstaller(allow_network=False) with pytest.raises(BundlerError, match="network access is disabled"): @@ -96,10 +112,14 @@ def test_offline_workflow_allows_bundled(tmp_path: Path, monkeypatch): assets, "_locate_bundled_workflow", lambda wid: bundled ) calls: list[tuple] = [] + + def _workflow_add(wid, dev=None, from_url=None): + calls.append((wid, dev, from_url)) + monkeypatch.setattr( specify_cli, "workflow_add", - lambda wid, dev=object(), from_url=object(): calls.append((wid, dev, from_url)), + _workflow_add, ) manager = primitive_manager("workflows", tmp_path, allow_network=False) @@ -483,9 +503,9 @@ def _fake_install(self, *a, **k): def test_refresh_succeeds_and_passes_force_true(tmp_path: Path, monkeypatch): """Regression: bundle update (refresh=True) of an already-installed extension must succeed and pass force=True to install_from_directory.""" + import specify_cli._assets as assets from specify_cli.bundles.installer import install_bundle from specify_cli.bundles.manifest import BundleManifest - import specify_cli._assets as assets from specify_cli.extensions import ExtensionManager bundled = _write_manifest(tmp_path / "ext", "extension", "1.0.0") diff --git a/tests/specify_cli/workflows/step/catalog/test_command_list.py b/tests/specify_cli/workflows/step/catalog/test_command_list.py index 6aff5d3569..34f7076030 100644 --- a/tests/specify_cli/workflows/step/catalog/test_command_list.py +++ b/tests/specify_cli/workflows/step/catalog/test_command_list.py @@ -3,15 +3,13 @@ from __future__ import annotations - - - class TestWorkflowCliAlignment: """CLI alignment with extension/preset commands (#2342).""" def test_step_catalog_list_escapes_rich_markup(self, project_dir, monkeypatch): """User-editable step-catalog name/url/description must not be parsed as Rich markup.""" from typer.testing import CliRunner + from specify_cli import app from specify_cli.workflows.step.catalog import StepCatalog diff --git a/tests/specify_cli/workflows/step/catalog/test_registry.py b/tests/specify_cli/workflows/step/catalog/test_registry.py new file mode 100644 index 0000000000..6cce0676e7 --- /dev/null +++ b/tests/specify_cli/workflows/step/catalog/test_registry.py @@ -0,0 +1,33 @@ +"""Persistence tests for the custom step registry.""" + +from __future__ import annotations + +import json + +import pytest + +from specify_cli.workflows.step.catalog import StepRegistry, StepValidationError + + +def _entry(step_id: str) -> dict[str, str]: + return { + "name": "Example", + "version": "1.0.0", + "description": "", + "author": "", + "type_key": step_id, + "source": "local", + } + + +def test_save_keeps_existing_registry_when_json_serialization_fails(project_dir): + registry = StepRegistry(project_dir) + registry.add("first", _entry("first")) + before = registry.registry_path.read_bytes() + registry.data["steps"]["second"] = {"not_json": {1, 2}} + + with pytest.raises(StepValidationError): + registry.save() + + assert registry.registry_path.read_bytes() == before + assert json.loads(before)["steps"]["first"]["type_key"] == "first" diff --git a/tests/specify_cli/workflows/step/test_command_add.py b/tests/specify_cli/workflows/step/test_command_add.py index 3250057e74..b2f03b879f 100644 --- a/tests/specify_cli/workflows/step/test_command_add.py +++ b/tests/specify_cli/workflows/step/test_command_add.py @@ -7,11 +7,11 @@ import pytest - class TestWorkflowStepAddCLI: @pytest.mark.skipif(not hasattr(os, "symlink"), reason="symlinks are unavailable") def test_add_rejects_symlinked_steps_base_dir(self, project_dir, monkeypatch): from typer.testing import CliRunner + from specify_cli import app from specify_cli.workflows.step.catalog import StepCatalog @@ -40,13 +40,14 @@ def _fake_get_step_info(self, step_id): def test_add_rejects_oversized_step_response(self, project_dir, monkeypatch): from typer.testing import CliRunner + from specify_cli import app - from specify_cli.workflows import _commands as wf_commands - from specify_cli.workflows.step.catalog import StepCatalog from specify_cli.authentication import http as auth_http + from specify_cli.workflows.step import command_add + from specify_cli.workflows.step.catalog import StepCatalog monkeypatch.chdir(project_dir) - monkeypatch.setattr(wf_commands, "_MAX_WORKFLOW_YAML_BYTES", 100) + monkeypatch.setattr(command_add, "_MAX_STEP_CATALOG_RESPONSE_BYTES", 100) monkeypatch.setattr( StepCatalog, "get_step_info", @@ -96,7 +97,7 @@ def read(self, size=-1): assert result.exit_code != 0 assert ( - "responseexceedsthe100-byteworkflowsizelimit" + "steppackageresponse'exceedsmaximumsizeof100bytes" in "".join(result.output.split()) ) assert not ( @@ -118,9 +119,10 @@ def test_add_rejects_falsy_non_mapping_step_yml( genuinely empty document, so it must be distinguished (via ``yaml.compose``) and rejected too, rather than defaulting to {}.""" from typer.testing import CliRunner + from specify_cli import app - from specify_cli.workflows.step.catalog import StepCatalog from specify_cli.authentication import http as auth_http + from specify_cli.workflows.step.catalog import StepCatalog monkeypatch.chdir(project_dir) monkeypatch.setattr( @@ -441,9 +443,10 @@ def read(self, size=-1): def test_add_rejects_non_string_extra_files_key(self, project_dir, monkeypatch): from typer.testing import CliRunner + from specify_cli import app - from specify_cli.workflows.step.catalog import StepCatalog from specify_cli.authentication import http as auth_http + from specify_cli.workflows.step.catalog import StepCatalog monkeypatch.chdir(project_dir) @@ -505,9 +508,10 @@ def test_add_rejects_invalid_extra_files_path( self, project_dir, monkeypatch, rel_path, expected ): from typer.testing import CliRunner + from specify_cli import app - from specify_cli.workflows.step.catalog import StepCatalog from specify_cli.authentication import http as auth_http + from specify_cli.workflows.step.catalog import StepCatalog monkeypatch.chdir(project_dir) @@ -556,9 +560,10 @@ def _fake_open_url(url, timeout=30, redirect_validator=None): def test_add_rejects_non_string_extra_files_url(self, project_dir, monkeypatch): from typer.testing import CliRunner + from specify_cli import app - from specify_cli.workflows.step.catalog import StepCatalog from specify_cli.authentication import http as auth_http + from specify_cli.workflows.step.catalog import StepCatalog monkeypatch.chdir(project_dir) @@ -692,6 +697,7 @@ def _valid_archive_files(type_key="my-step"): class TestWorkflowStepAddSources: def test_dev_installs_and_loads(self, project_dir, tmp_path, monkeypatch): from typer.testing import CliRunner + from specify_cli import app from specify_cli.workflows import STEP_REGISTRY, load_custom_steps @@ -715,6 +721,7 @@ def test_dev_installs_and_loads(self, project_dir, tmp_path, monkeypatch): def test_dev_install_list_and_remove(self, project_dir, tmp_path, monkeypatch): from typer.testing import CliRunner + from specify_cli import app package = _write_package(tmp_path, type_key="dev-step") @@ -739,6 +746,7 @@ def test_dev_install_list_and_remove(self, project_dir, tmp_path, monkeypatch): def test_dev_rejects_missing_init(self, project_dir, tmp_path, monkeypatch): from typer.testing import CliRunner + from specify_cli import app package = _write_package(tmp_path, type_key="dev-step") @@ -755,6 +763,7 @@ def test_dev_rejects_symlinked_source_root(self, project_dir, tmp_path, monkeypa if not hasattr(os, "symlink"): pytest.skip("symlinks are unavailable") from typer.testing import CliRunner + from specify_cli import app package = _write_package(tmp_path, type_key="dev-step") @@ -770,6 +779,7 @@ def test_dev_rejects_symlinked_source_root(self, project_dir, tmp_path, monkeypa def test_dev_and_from_are_mutually_exclusive(self, project_dir, monkeypatch): from typer.testing import CliRunner + from specify_cli import app monkeypatch.chdir(project_dir) @@ -792,6 +802,7 @@ def test_dev_and_from_are_mutually_exclusive(self, project_dir, monkeypatch): @pytest.mark.parametrize("option", ["--dev", "--from"]) def test_empty_source_value_rejected(self, project_dir, monkeypatch, option): from typer.testing import CliRunner + from specify_cli import app monkeypatch.chdir(project_dir) @@ -802,6 +813,7 @@ def test_empty_source_value_rejected(self, project_dir, monkeypatch, option): def test_force_replaces_installed_package(self, project_dir, tmp_path, monkeypatch): from typer.testing import CliRunner + from specify_cli import app package = _write_package(tmp_path, type_key="dev-step", init_body="# old\n") @@ -834,6 +846,7 @@ def test_force_replaces_installed_package(self, project_dir, tmp_path, monkeypat def test_force_replaces_orphaned_directory(self, project_dir, tmp_path, monkeypatch): from typer.testing import CliRunner + from specify_cli import app orphan = ( @@ -858,6 +871,7 @@ def test_from_denied_confirmation_issues_no_request( ): import typer from typer.testing import CliRunner + from specify_cli import app from specify_cli.authentication import http as auth_http @@ -900,6 +914,7 @@ def test_from_archive_installs( ): import typer from typer.testing import CliRunner + from specify_cli import app from specify_cli.authentication import http as auth_http @@ -924,6 +939,7 @@ def test_from_archive_installs( def test_from_rejects_non_https(self, project_dir, monkeypatch): from typer.testing import CliRunner + from specify_cli import app monkeypatch.chdir(project_dir) @@ -943,6 +959,7 @@ def test_from_rejects_non_https(self, project_dir, monkeypatch): def test_from_rejects_malformed_url(self, project_dir, monkeypatch): from typer.testing import CliRunner + from specify_cli import app monkeypatch.chdir(project_dir) @@ -963,6 +980,7 @@ def test_from_rejects_malformed_url(self, project_dir, monkeypatch): def test_from_rejects_redirect_to_non_https(self, project_dir, monkeypatch): import typer from typer.testing import CliRunner + from specify_cli import app from specify_cli.authentication import http as auth_http @@ -993,6 +1011,7 @@ def test_from_rejects_redirect_to_non_https(self, project_dir, monkeypatch): def test_from_rejects_non_archive_body(self, project_dir, monkeypatch): import typer from typer.testing import CliRunner + from specify_cli import app from specify_cli.authentication import http as auth_http @@ -1025,6 +1044,7 @@ def test_from_rejects_archive_with_unrelated_siblings( ): import typer from typer.testing import CliRunner + from specify_cli import app from specify_cli.authentication import http as auth_http @@ -1058,11 +1078,69 @@ def test_from_rejects_archive_with_unrelated_siblings( assert result.exit_code != 0 assert "exactly one top-level" in result.output + def test_from_rejects_original_url_format_mismatch_after_redirect( + self, project_dir, monkeypatch + ): + import typer + from typer.testing import CliRunner + + from specify_cli import app + from specify_cli.authentication import http as auth_http + + monkeypatch.chdir(project_dir) + monkeypatch.setattr(typer, "confirm", lambda *a, **k: True) + body = _make_tar_gz(_valid_archive_files()) + monkeypatch.setattr( + auth_http, + "open_url", + lambda url, timeout=30, redirect_validator=None, extra_headers=None: ( + _ArchiveResponse("https://example.com/download", body, None) + ), + ) + + result = CliRunner().invoke( + app, ["workflow", "step", "add", "my-step", "--from", "https://example.com/pkg.zip"] + ) + + assert result.exit_code != 0 + assert "Archive format mismatch" in result.output + + def test_from_escapes_installed_name(self, project_dir, monkeypatch): + import typer + from typer.testing import CliRunner + + from specify_cli import app + from specify_cli.authentication import http as auth_http + + monkeypatch.chdir(project_dir) + monkeypatch.setattr(typer, "confirm", lambda *a, **k: True) + body = _make_zip( + { + "step.yml": "step:\n type_key: my-step\n name: '[/]'\n", + "__init__.py": "# init\n", + } + ) + monkeypatch.setattr( + auth_http, + "open_url", + lambda url, timeout=30, redirect_validator=None, extra_headers=None: ( + _ArchiveResponse(url, body, "application/zip") + ), + ) + + result = CliRunner().invoke( + app, ["workflow", "step", "add", "my-step", "--from", "https://example.com/pkg.zip"] + ) + + assert result.exit_code == 0, result.output + assert "[/]" in result.output + def test_from_denied_when_already_installed_errors_before_prompt( self, project_dir, tmp_path, monkeypatch ): import typer from typer.testing import CliRunner + from specify_cli import app package = _write_package(tmp_path, type_key="my-step") @@ -1139,6 +1217,7 @@ def test_dev_install_loads_runs_and_removes( self, project_dir, tmp_path, monkeypatch ): from typer.testing import CliRunner + from specify_cli import app from specify_cli.workflows import load_custom_steps diff --git a/tests/specify_cli/workflows/step/test_command_info.py b/tests/specify_cli/workflows/step/test_command_info.py index b63d5f8f78..62391f5f03 100644 --- a/tests/specify_cli/workflows/step/test_command_info.py +++ b/tests/specify_cli/workflows/step/test_command_info.py @@ -1,15 +1,12 @@ """Command-focused workflow tests.""" -from __future__ import annotations - - - +from typing import ClassVar class TestWorkflowStepRichMarkup: """Step discovery commands render metadata as literal text.""" - METADATA = { + METADATA: ClassVar[dict[str, str]] = { "id": "[magenta]step-id[/magenta]", "name": "[red]Step Name[/red]", "version": "[green]1.0.0[/green]", @@ -21,6 +18,7 @@ def test_info_escapes_catalog_metadata( self, project_dir, monkeypatch ): from typer.testing import CliRunner + from specify_cli import app from specify_cli.workflows.step.catalog import StepCatalog, StepRegistry @@ -43,6 +41,7 @@ def test_info_escapes_catalog_metadata( def test_info_escapes_missing_step_id(self, project_dir, monkeypatch): from typer.testing import CliRunner + from specify_cli import app from specify_cli.workflows.step.catalog import StepCatalog, StepRegistry @@ -64,6 +63,7 @@ def test_info_escapes_missing_step_id(self, project_dir, monkeypatch): def test_info_prints_local_source(self, project_dir, monkeypatch): from typer.testing import CliRunner + from specify_cli import app from specify_cli.workflows.step.catalog import StepRegistry @@ -86,6 +86,7 @@ def test_info_prints_local_source(self, project_dir, monkeypatch): def test_info_prints_catalog_source_with_name(self, project_dir, monkeypatch): from typer.testing import CliRunner + from specify_cli import app from specify_cli.workflows.step.catalog import StepRegistry diff --git a/tests/specify_cli/workflows/step/test_command_list.py b/tests/specify_cli/workflows/step/test_command_list.py index e225eea93d..ff9ba007a7 100644 --- a/tests/specify_cli/workflows/step/test_command_list.py +++ b/tests/specify_cli/workflows/step/test_command_list.py @@ -1,15 +1,12 @@ """Command-focused workflow tests.""" -from __future__ import annotations - - - +from typing import ClassVar class TestWorkflowStepRichMarkup: """Step discovery commands render metadata as literal text.""" - METADATA = { + METADATA: ClassVar[dict[str, str]] = { "id": "[magenta]step-id[/magenta]", "name": "[red]Step Name[/red]", "version": "[green]1.0.0[/green]", @@ -21,6 +18,7 @@ def test_list_escapes_installed_metadata( self, project_dir, monkeypatch ): from typer.testing import CliRunner + from specify_cli import app from specify_cli.workflows.step.catalog import StepRegistry diff --git a/tests/specify_cli/workflows/step/test_command_search.py b/tests/specify_cli/workflows/step/test_command_search.py index bd9982da0c..60f7852c7c 100644 --- a/tests/specify_cli/workflows/step/test_command_search.py +++ b/tests/specify_cli/workflows/step/test_command_search.py @@ -1,15 +1,12 @@ """Command-focused workflow tests.""" -from __future__ import annotations - - - +from typing import ClassVar class TestWorkflowStepRichMarkup: """Step discovery commands render metadata as literal text.""" - METADATA = { + METADATA: ClassVar[dict[str, str]] = { "id": "[magenta]step-id[/magenta]", "name": "[red]Step Name[/red]", "version": "[green]1.0.0[/green]", @@ -21,6 +18,7 @@ def test_search_escapes_catalog_metadata( self, project_dir, monkeypatch ): from typer.testing import CliRunner + from specify_cli import app from specify_cli.workflows.step.catalog import StepCatalog diff --git a/tests/specify_cli/workflows/step/test_installer.py b/tests/specify_cli/workflows/step/test_installer.py index ecbacc4422..1e788fd32c 100644 --- a/tests/specify_cli/workflows/step/test_installer.py +++ b/tests/specify_cli/workflows/step/test_installer.py @@ -388,6 +388,38 @@ def test_catalog_provenance_shape(tmp_path, project_dir): assert entry["author"] == "author" +@pytest.mark.parametrize( + "field,value", + [ + ("name", "2026-09-24"), + ("version", "2026-09-24"), + ("description", "2026-09-24"), + ("author", "2026-09-24"), + ], +) +def test_rejects_non_string_persisted_metadata_before_publication( + tmp_path, project_dir, field, value +): + pkg = _write_package(tmp_path / "pkg") + (pkg / "step.yml").write_text( + f"step:\n type_key: my-step\n {field}: {value}\n", encoding="utf-8" + ) + + with pytest.raises(installer.StepInstallError, match="must be a string"): + installer.install_step_package(project_dir, "my-step", pkg, source="local") + + assert not (_steps_dir(project_dir) / "my-step").exists() + + +def test_rejects_unknown_source_before_creating_steps_dir(tmp_path, project_dir): + pkg = _write_package(tmp_path / "pkg") + + with pytest.raises(installer.StepInstallError, match="source"): + installer.install_step_package(project_dir, "my-step", pkg, source="unknown") + + assert not _steps_dir(project_dir).exists() + + # --------------------------------------------------------------------------- # Failure handling # --------------------------------------------------------------------------- @@ -449,6 +481,22 @@ def _validate(package_dir, step_id): ) == "# old\n" +def test_uses_metadata_from_staged_copy(tmp_path, project_dir, monkeypatch): + pkg = _write_package(tmp_path / "pkg") + original_copy = installer._copy_package_tree + + def _copy_then_change(source, target): + original_copy(source, target) + (target / "step.yml").write_text( + "step:\n type_key: my-step\n name: Staged Name\n", encoding="utf-8" + ) + + monkeypatch.setattr(installer, "_copy_package_tree", _copy_then_change) + entry = installer.install_step_package(project_dir, "my-step", pkg, source="local") + + assert entry["name"] == "Staged Name" + + def test_force_registry_failure_warns_reinstall( tmp_path, project_dir, monkeypatch ): @@ -468,6 +516,10 @@ def _boom(self, step_id, metadata): project_dir, "my-step", new_pkg, source="local", force=True ) assert "reinstall" in str(exc.value).lower() + # The previous install stays registered even though its metadata was not + # updated, so the message must not claim it is unregistered. + assert "not registered" not in str(exc.value).lower() + assert StepRegistry(project_dir).is_installed("my-step") assert (_steps_dir(project_dir) / "my-step" / "__init__.py").read_text( encoding="utf-8" ) == "# new\n" @@ -545,6 +597,134 @@ def test_no_backup_artifacts_after_force(tmp_path, project_dir): assert names == ["my-step", "step-registry.json"] +def _install_race_setup(tmp_path, project_dir, monkeypatch, *, force_b): + """Drive two concurrent installs against the install lock. + + Installer A is paused inside the locked critical section while installer B + is guaranteed to be blocked on the lock (its ``fcntl.flock`` attempt has + been observed). The caller resumes A via the returned ``release_a`` event, + then joins both threads and inspects ``outcomes``. + """ + import fcntl + import threading + + pkg_a = _write_package(tmp_path / "pkg-a", init_body="# a\n") + pkg_b = _write_package(tmp_path / "pkg-b", init_body="# b\n") + + class Race: + a_inside = threading.Event() + release_a = threading.Event() + b_attempted_lock = threading.Event() + b_inside_replace = threading.Event() + outcomes: dict[str, Exception | None] = {} + thread_a = None + thread_b = None + + race = Race() + real_replace = installer._replace_install + real_flock = fcntl.flock + + def _replace(step_dir, staged_dir, registry, step_id, entry, *, force): + if threading.current_thread().name == "installer-a": + race.a_inside.set() + if not race.release_a.wait(10): + raise AssertionError("installer A was never released") + else: + race.b_inside_replace.set() + return real_replace( + step_dir, staged_dir, registry, step_id, entry, force=force + ) + + def _flock(fd, operation): + if ( + threading.current_thread().name == "installer-b" + and operation == fcntl.LOCK_EX + ): + race.b_attempted_lock.set() + return real_flock(fd, operation) + + monkeypatch.setattr(installer, "_replace_install", _replace) + monkeypatch.setattr(fcntl, "flock", _flock) + + def _install(label, pkg, force): + try: + installer.install_step_package( + project_dir, "my-step", pkg, source="local", force=force + ) + except Exception as exc: # noqa: BLE001 - recorded for assertions + race.outcomes[label] = exc + else: + race.outcomes[label] = None + + race.thread_a = threading.Thread( + target=_install, args=("a", pkg_a, False), name="installer-a", daemon=True + ) + race.thread_b = threading.Thread( + target=_install, args=("b", pkg_b, force_b), name="installer-b", daemon=True + ) + + race.thread_a.start() + assert race.a_inside.wait(10), "installer A never reached the critical section" + race.thread_b.start() + assert race.b_attempted_lock.wait(10), "installer B never attempted the lock" + return race + + +def _finish_race(race): + race.release_a.set() + race.thread_a.join(timeout=10) + race.thread_b.join(timeout=10) + assert not race.thread_a.is_alive() + assert not race.thread_b.is_alive() + + +def test_install_lock_blocks_concurrent_duplicate(tmp_path, project_dir, monkeypatch): + """A second install of the same id waits for the lock and sees A's commit. + + B can never enter the swap while A holds it; once A commits, B reloads the + registry inside the lock and fails as a duplicate instead of overwriting. + """ + if os.name == "nt": + pytest.skip("fcntl.flock is POSIX-only") + + race = _install_race_setup(tmp_path, project_dir, monkeypatch, force_b=False) + # A holds the lock, so B cannot have reached the directory swap. + assert not race.b_inside_replace.is_set() + + _finish_race(race) + + assert race.outcomes["a"] is None + assert isinstance(race.outcomes["b"], installer.StepInstallError) + assert "already installed" in str(race.outcomes["b"]) + # B never swapped, so A's install is intact. + assert not race.b_inside_replace.is_set() + assert (_steps_dir(project_dir) / "my-step" / "__init__.py").read_text( + encoding="utf-8" + ) == "# a\n" + assert _registry_entry(project_dir, "my-step")["source"] == "local" + + +def test_install_lock_serializes_force_replace(tmp_path, project_dir, monkeypatch): + """A forced install swaps only after the lock is released by the first.""" + if os.name == "nt": + pytest.skip("fcntl.flock is POSIX-only") + + race = _install_race_setup(tmp_path, project_dir, monkeypatch, force_b=True) + # B is blocked on the lock and has not swapped A's package yet. + assert not race.b_inside_replace.is_set() + + _finish_race(race) + + assert race.outcomes["a"] is None + assert race.outcomes["b"] is None + # B reached the swap only after A committed, and its package won. + assert race.b_inside_replace.is_set() + assert (_steps_dir(project_dir) / "my-step" / "__init__.py").read_text( + encoding="utf-8" + ) == "# b\n" + assert _registry_entry(project_dir, "my-step")["type_key"] == "my-step" + + def test_loader_does_not_discover_staging_package(project_dir): from specify_cli.workflows import load_custom_steps diff --git a/tests/specify_cli/workflows/test_custom_steps.py b/tests/specify_cli/workflows/test_custom_steps.py new file mode 100644 index 0000000000..ed0d7e0650 --- /dev/null +++ b/tests/specify_cli/workflows/test_custom_steps.py @@ -0,0 +1,49 @@ +"""Runtime freshness tests for project-local custom workflow steps.""" + +from __future__ import annotations + +import shutil +from pathlib import Path + +from specify_cli.workflows import STEP_REGISTRY, load_custom_steps + + +def _write_step(project_root: Path, marker: str) -> None: + step_dir = project_root / ".specify" / "workflows" / "steps" / "custom" + step_dir.mkdir(parents=True) + (step_dir / "step.yml").write_text( + "step:\n type_key: custom\n", encoding="utf-8" + ) + (step_dir / "__init__.py").write_text( + "from specify_cli.workflows.base import StepBase, StepResult\n\n" + "class Custom(StepBase):\n" + " type_key = 'custom'\n" + " def execute(self, config, context):\n" + f" return StepResult(output={{'marker': {marker!r}}})\n", + encoding="utf-8", + ) + + +def test_custom_steps_refresh_for_active_project(tmp_path): + project_a = tmp_path / "a" + project_b = tmp_path / "b" + _write_step(project_a, "a") + _write_step(project_b, "b") + + assert load_custom_steps(project_a) == ["custom"] + assert STEP_REGISTRY["custom"].execute({}, None).output == {"marker": "a"} + + assert load_custom_steps(project_b) == ["custom"] + assert STEP_REGISTRY["custom"].execute({}, None).output == {"marker": "b"} + + +def test_removed_custom_step_is_not_retained(tmp_path): + project = tmp_path / "project" + _write_step(project, "old") + assert load_custom_steps(project) == ["custom"] + + step_dir = project / ".specify" / "workflows" / "steps" / "custom" + shutil.rmtree(step_dir) + + assert load_custom_steps(project) == [] + assert "custom" not in STEP_REGISTRY From 4b354d775c18cb6baf5d9e21bb4cc9d3c06b305e Mon Sep 17 00:00:00 2001 From: Markus Date: Mon, 28 Sep 2026 14:10:24 +0200 Subject: [PATCH 03/16] fix(workflows): address step install review findings Assisted-by: OpenCode (model: deepseek-v4.1-flash, autonomous) --- docs/reference/workflows.md | 9 +- src/specify_cli/workflows/step/command_add.py | 55 +++- src/specify_cli/workflows/step/installer.py | 90 ++++- .../workflows/step/test_command_add.py | 311 ++++++++++++++++++ .../workflows/step/test_installer.py | 188 +++++++++++ 5 files changed, 626 insertions(+), 27 deletions(-) diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index 4cf68316c1..9caa1b5a3b 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -635,10 +635,11 @@ specify workflow step add my-step --from https://example.com/my-step.zip --force `--force` first stages and validates the replacement before touching the existing installation, and can replace both a registered install and a leftover unregistered directory. Validation and staging failures leave the previous -package untouched. If a replacement commit fails after the previous package is -removed — removing the old directory, publishing the new one, or writing the -registry — the installation is left incomplete: rerun the command with the -original source and `--force` to reinstall. No automatic rollback is attempted. +package untouched. If removing the old directory fails, the replacement is not +published. If publishing the replacement or updating the registry fails after +the old directory has been removed, the installation may be left incomplete: +rerun the command with the original source and `--force` to reinstall. No +automatic rollback is attempted. #### Package validation diff --git a/src/specify_cli/workflows/step/command_add.py b/src/specify_cli/workflows/step/command_add.py index 5738533e1a..75f7b9315e 100644 --- a/src/specify_cli/workflows/step/command_add.py +++ b/src/specify_cli/workflows/step/command_add.py @@ -8,6 +8,7 @@ from __future__ import annotations +import sys from typing import Annotated from .. import _commands as cli @@ -198,7 +199,14 @@ def _install_from_url( tmp_path = cli.Path(tmp.name) tmp.write(downloaded) - extract_tmp = tempfile.TemporaryDirectory(prefix="speckit-step-archive-") + try: + extract_tmp = tempfile.TemporaryDirectory( + prefix="speckit-step-archive-" + ) + except OSError as exc: + raise installer.StepInstallError( + f"Failed to create temporary step archive directory: {exc}" + ) from exc extracted_root = cli.Path(extract_tmp.name) try: # safe_extract_archive re-detects and confirms the archive bytes. @@ -220,20 +228,27 @@ def _install_from_url( ) committed = True finally: + primary_error = sys.exc_info()[1] try: extract_tmp.cleanup() except OSError as cleanup_exc: - if committed: - cli.console.print( - "[yellow]Warning:[/yellow] Could not remove temporary step " - f"archive directory: {cli._escape_markup(str(cleanup_exc))} " + if extract_tmp.name and cli.Path(extract_tmp.name).exists(): + detail = ( + f"{cli._escape_markup(str(cleanup_exc))} " f"(path: {cli._escape_markup(extract_tmp.name)})" ) - elif __import__("sys").exc_info()[0] is None: + cli.console.print( + "[yellow]Warning:[/yellow] Could not remove temporary " + f"step archive directory: {detail}" + ) + if primary_error is None and not committed: raise installer.StepInstallError( "Failed to remove temporary step archive directory: " f"{cleanup_exc}" ) from cleanup_exc + # Do not raise from cleanup: the primary installation error + # (if any) is already propagating, and after commit the install + # has succeeded. The warning above reports the residual path. except cli.typer.Exit: raise except installer.StepInstallError: @@ -368,7 +383,12 @@ def _safe_fetch(url: str) -> bytes: label="step package response", ) - package_tmp = tempfile.TemporaryDirectory(prefix="speckit-step-package-") + try: + package_tmp = tempfile.TemporaryDirectory(prefix="speckit-step-package-") + except OSError as exc: + raise installer.StepInstallError( + f"Failed to create temporary step package directory: {exc}" + ) from exc package_dir = cli.Path(package_tmp.name) committed = False try: @@ -456,20 +476,26 @@ def _safe_fetch(url: str) -> bytes: ) committed = True finally: + primary_error = sys.exc_info()[1] try: package_tmp.cleanup() except OSError as cleanup_exc: - if committed: - cli.console.print( - "[yellow]Warning:[/yellow] Could not remove temporary step " - f"package directory: {cli._escape_markup(str(cleanup_exc))} " + if package_tmp.name and cli.Path(package_tmp.name).exists(): + detail = ( + f"{cli._escape_markup(str(cleanup_exc))} " f"(path: {cli._escape_markup(package_tmp.name)})" ) - elif __import__("sys").exc_info()[0] is None: + cli.console.print( + "[yellow]Warning:[/yellow] Could not remove temporary " + f"step package directory: {detail}" + ) + if primary_error is None and not committed: raise installer.StepInstallError( "Failed to remove temporary step package directory: " f"{cleanup_exc}" ) from cleanup_exc + # Do not raise from cleanup: preserve a primary download/install + # error, or report a successful install with a warning only. _print_installed(step_id, entry) @@ -508,5 +534,10 @@ def workflow_step_add( else: _install_from_catalog(project_root, step_id, force=force) except installer.StepInstallError as exc: + notes = getattr(exc, "__notes__", ()) + for note in notes: + cli.console.print( + f"[yellow]Warning:[/yellow] {cli._escape_markup(note)}" + ) cli.console.print(f"[red]Error:[/red] {exc}") raise cli.typer.Exit(1) from exc diff --git a/src/specify_cli/workflows/step/installer.py b/src/specify_cli/workflows/step/installer.py index 5c53b17d20..4e990a1cd3 100644 --- a/src/specify_cli/workflows/step/installer.py +++ b/src/specify_cli/workflows/step/installer.py @@ -15,7 +15,9 @@ import os import shutil import stat +import sys import tempfile +import warnings from collections.abc import Mapping from pathlib import Path from typing import Any @@ -26,6 +28,7 @@ # files. These ceilings apply uniformly to catalog, local, and archive sources. _MAX_STEP_PACKAGE_FILES = 512 _MAX_STEP_PACKAGE_BYTES = 50 * 1024 * 1024 # 50 MiB +_COPY_CHUNK_BYTES = 64 * 1024 # Files/dirs never copied into (or counted as part of) an installed step # package. Mirrors ``bundles/packager.py`` ``EXCLUDE_NAMES``. @@ -411,6 +414,44 @@ def _reject_builtin_collision(step_id: str) -> None: def _check_duplicate( registry: Any, step_id: str, step_dir: Path, *, force: bool ) -> None: + folded_id = step_id.casefold() + try: + registered_ids = registry.list() + except (AttributeError, TypeError): + registered_ids = () + for registered_id in registered_ids: + if ( + isinstance(registered_id, str) + and registered_id != step_id + and registered_id.casefold() == folded_id + ): + raise StepInstallError( + f"Step ID '{step_id}' collides case-insensitively with registered " + f"step ID '{registered_id}'; use the exact registered ID with --force" + ) + + try: + existing_names = (item.name for item in step_dir.parent.iterdir()) + for existing_name in existing_names: + if existing_name == step_id or existing_name.casefold() != folded_id: + continue + existing_dir = step_dir.parent / existing_name + try: + same_destination = step_dir.exists() and existing_dir.samefile(step_dir) + except OSError: + same_destination = False + if not same_destination: + raise StepInstallError( + f"Step ID '{step_id}' collides case-insensitively with " + f"existing step directory '{existing_name}'" + ) + except FileNotFoundError: + pass + except OSError as exc: + raise StepInstallError( + f"Failed to inspect existing step directories: {exc}" + ) from exc + if force: return if registry.is_installed(step_id): @@ -517,6 +558,8 @@ def _copy_package_tree(source_dir: Path, target_dir: Path) -> None: validation cannot smuggle external content into the staged package. """ + remaining_bytes = [_MAX_STEP_PACKAGE_BYTES] + def _copy(current: Path, destination: Path) -> None: try: destination.mkdir(parents=True, exist_ok=True) @@ -545,7 +588,8 @@ def _copy(current: Path, destination: Path) -> None: if stat.S_ISDIR(mode): _copy(Path(entry.path), target) elif stat.S_ISREG(mode): - _copy_regular_file(entry.path, target, mode) + copied = _copy_regular_file(entry.path, target, mode, remaining_bytes[0]) + remaining_bytes[0] -= copied else: raise StepInstallError( f"Step package contains unsupported file: {entry.path}" @@ -554,8 +598,10 @@ def _copy(current: Path, destination: Path) -> None: _copy(source_dir, target_dir) -def _copy_regular_file(source: str, target: Path, expected_mode: int) -> None: - """Copy an inspected regular file without following a late symlink swap.""" +def _copy_regular_file( + source: str, target: Path, expected_mode: int, remaining_bytes: int +) -> int: + """Copy an inspected regular file within the remaining package byte budget.""" flags = os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0) try: fd = os.open(source, flags) @@ -574,8 +620,24 @@ def _copy_regular_file(source: str, target: Path, expected_mode: int) -> None: raise StepInstallError( f"Step package file changed while staging: {source}" ) - with os.fdopen(fd, "rb", closefd=False) as source_file, target.open("xb") as target_file: - shutil.copyfileobj(source_file, target_file) + copied_bytes = 0 + with os.fdopen(fd, "rb", closefd=False) as source_file, target.open( + "xb" + ) as target_file: + while True: + chunk = source_file.read( + min(_COPY_CHUNK_BYTES, remaining_bytes - copied_bytes + 1) + ) + if not chunk: + break + if copied_bytes + len(chunk) > remaining_bytes: + raise StepInstallError( + f"Step package exceeds the {_MAX_STEP_PACKAGE_BYTES}-byte " + "total size limit while staging" + ) + target_file.write(chunk) + copied_bytes += len(chunk) + return copied_bytes except OSError as exc: raise StepInstallError(f"Failed to stage step package: {exc}") from exc finally: @@ -760,14 +822,20 @@ def install_step_package( ) committed = True finally: + primary_error = sys.exc_info()[1] try: shutil.rmtree(work_dir) except OSError as cleanup_exc: - # The staged directory is private and cannot be loaded as a step, - # but callers still need an actionable residual-path diagnostic. - if work_dir.exists() and not committed and os.sys.exc_info()[0] is None: - raise StepInstallError( - f"Failed to remove staging directory '{work_dir}': {cleanup_exc}" - ) from cleanup_exc + if work_dir.exists(): + message = ( + f"Could not remove step staging directory '{work_dir}': " + f"{cleanup_exc}" + ) + if primary_error is not None: + primary_error.add_note(message) + elif committed: + warnings.warn(message, UserWarning, stacklevel=2) + else: + raise StepInstallError(message) from cleanup_exc return entry diff --git a/tests/specify_cli/workflows/step/test_command_add.py b/tests/specify_cli/workflows/step/test_command_add.py index b2f03b879f..ad45e7fc9b 100644 --- a/tests/specify_cli/workflows/step/test_command_add.py +++ b/tests/specify_cli/workflows/step/test_command_add.py @@ -104,6 +104,254 @@ def read(self, size=-1): project_dir / ".specify" / "workflows" / "steps" / "my-step" ).exists() + def test_catalog_temp_directory_creation_error_is_user_facing( + self, project_dir, monkeypatch + ): + import tempfile + from typer.testing import CliRunner + + from specify_cli import app + from specify_cli.workflows.step.catalog import StepCatalog + + monkeypatch.chdir(project_dir) + monkeypatch.setattr( + StepCatalog, + "get_step_info", + lambda self, step_id: { + "id": step_id, + "name": "Test Step", + "url": "https://example.com/step.yml", + "init_url": "https://example.com/__init__.py", + "_install_allowed": True, + }, + ) + + def _fail_tempdir(*args, **kwargs): + raise OSError("temporary storage unavailable") + + monkeypatch.setattr(tempfile, "TemporaryDirectory", _fail_tempdir) + result = CliRunner().invoke( + app, ["workflow", "step", "add", "my-step"] + ) + + assert result.exit_code != 0 + assert result.exception is None or isinstance(result.exception, SystemExit) + assert "Failed to create temporary step package directory" in result.output + assert "temporarystorageunavailable" in "".join(result.output.split()) + + def test_archive_temp_directory_creation_error_is_user_facing( + self, project_dir, monkeypatch + ): + import tempfile + import zipfile + from io import BytesIO + + import typer + from typer.testing import CliRunner + + from specify_cli import app + from specify_cli.authentication import http as auth_http + + monkeypatch.chdir(project_dir) + monkeypatch.setattr(typer, "confirm", lambda *args, **kwargs: True) + real_tempdir = tempfile.TemporaryDirectory + allocations = 0 + + def _allocate_tempdir(*args, **kwargs): + nonlocal allocations + allocations += 1 + if allocations == 1: + raise OSError("archive temp unavailable") + return real_tempdir(*args, **kwargs) + + archive = BytesIO() + with zipfile.ZipFile(archive, "w") as zf: + for name, content in _valid_archive_files().items(): + zf.writestr(name, content) + + monkeypatch.setattr(tempfile, "TemporaryDirectory", _allocate_tempdir) + monkeypatch.setattr( + auth_http, + "open_url", + lambda url, timeout=30, extra_headers=None, redirect_validator=None: _ArchiveResponse( + url, archive.getvalue(), "application/zip" + ), + ) + + result = CliRunner().invoke( + app, + [ + "workflow", + "step", + "add", + "my-step", + "--from", + "https://example.com/pkg.zip", + ], + ) + + assert result.exit_code != 0 + assert result.exception is None or isinstance(result.exception, SystemExit) + assert "Failed to create temporary step archive directory" in result.output + assert "archivetempunavailable" in "".join(result.output.split()) + + def test_archive_cleanup_failure_preserves_primary_install_error( + self, project_dir, monkeypatch + ): + import tempfile + import zipfile + from io import BytesIO + + import typer + from typer.testing import CliRunner + + from specify_cli import app + from specify_cli.authentication import http as auth_http + from specify_cli.workflows.step import installer + + monkeypatch.chdir(project_dir) + monkeypatch.setattr(typer, "confirm", lambda *args, **kwargs: True) + archive = BytesIO() + with zipfile.ZipFile(archive, "w") as zf: + for name, content in _valid_archive_files().items(): + zf.writestr(name, content) + + real_tempdir = tempfile.TemporaryDirectory + extract_paths = [] + + class _FailCleanupTempDir: + def __init__(self, *args, **kwargs): + self._inner = real_tempdir(*args, **kwargs) + self.name = self._inner.name + extract_paths.append(self.name) + + def cleanup(self): + raise OSError("archive cleanup blocked") + + monkeypatch.setattr(tempfile, "TemporaryDirectory", _FailCleanupTempDir) + monkeypatch.setattr( + auth_http, + "open_url", + lambda url, timeout=30, redirect_validator=None, extra_headers=None: _ArchiveResponse( + url, archive.getvalue(), "application/zip" + ), + ) + + real_install = installer.install_step_package + + def _install_then_raise(*args, **kwargs): + real_install(*args, **kwargs) + raise installer.StepInstallError("primary archive install error") + + monkeypatch.setattr(installer, "install_step_package", _install_then_raise) + result = CliRunner().invoke( + app, + [ + "workflow", + "step", + "add", + "my-step", + "--from", + "https://example.com/pkg.zip", + ], + ) + + assert result.exit_code != 0 + assert "primary archive install error" in result.output + assert "Could not remove temporary step archive directory" in result.output + assert extract_paths[0] in result.output + import shutil + + for extract_path in extract_paths: + shutil.rmtree(extract_path, ignore_errors=True) + + def test_catalog_cleanup_failure_adds_note_to_primary_error( + self, project_dir, monkeypatch + ): + import tempfile + from typer.testing import CliRunner + + from specify_cli import app + from specify_cli.authentication import http as auth_http + from specify_cli.workflows.step.catalog import StepCatalog + from specify_cli.workflows.step import installer + + monkeypatch.chdir(project_dir) + monkeypatch.setattr( + StepCatalog, + "get_step_info", + lambda self, step_id: { + "id": step_id, + "name": "Test Step", + "url": "https://example.com/step.yml", + "init_url": "https://example.com/__init__.py", + "_install_allowed": True, + }, + ) + bodies = { + "https://example.com/step.yml": b"step:\n type_key: my-step\n", + "https://example.com/__init__.py": b"# init\n", + } + + class _Response: + def __init__(self, url): + self.url = url + self.body = bodies[url] + self.offset = 0 + + def __enter__(self): + return self + + def __exit__(self, *args): + return False + + def geturl(self): + return self.url + + def read(self, size=-1): + if size < 0: + size = len(self.body) - self.offset + chunk = self.body[self.offset : self.offset + size] + self.offset += len(chunk) + return chunk + + monkeypatch.setattr( + auth_http, + "open_url", + lambda url, timeout=30, redirect_validator=None: _Response(url), + ) + real_tempdir = tempfile.TemporaryDirectory + allocated = [] + + class _FailCleanupTempDir: + def __init__(self, *args, **kwargs): + self._inner = real_tempdir(*args, **kwargs) + self.name = self._inner.name + allocated.append(self) + + def cleanup(self): + raise OSError("catalog cleanup blocked") + + monkeypatch.setattr(tempfile, "TemporaryDirectory", _FailCleanupTempDir) + + real_install = installer.install_step_package + + def _install_then_raise(*args, **kwargs): + real_install(*args, **kwargs) + raise installer.StepInstallError("primary install error") + + monkeypatch.setattr(installer, "install_step_package", _install_then_raise) + result = CliRunner().invoke(app, ["workflow", "step", "add", "my-step"]) + + assert result.exit_code != 0 + assert "primary install error" in result.output + assert "Could not remove temporary step package directory" in result.output + assert allocated[0].name in result.output + for item in allocated: + import shutil + + shutil.rmtree(item.name, ignore_errors=True) + @pytest.mark.parametrize( "step_yml_body", [b"[]", b"false", b"0", b"''", b"null", b"~", b"NULL"] ) @@ -937,6 +1185,69 @@ def test_from_archive_installs( project_dir / ".specify" / "workflows" / "steps" / "my-step" / "step.yml" ).is_file() + def test_archive_temp_directory_cleanup_failure_warns_after_commit( + self, project_dir, monkeypatch + ): + import tempfile + + import typer + from typer.testing import CliRunner + + from specify_cli import app + from specify_cli.authentication import http as auth_http + + monkeypatch.chdir(project_dir) + monkeypatch.setattr(typer, "confirm", lambda *args, **kwargs: True) + + real_tempdir = tempfile.TemporaryDirectory + extract_paths: list[str] = [] + + class _FailCleanupTempDir: + def __init__(self, *args, **kwargs): + self._inner = real_tempdir(*args, **kwargs) + self.name = self._inner.name + extract_paths.append(self.name) + + def cleanup(self): + raise OSError("archive cleanup blocked") + + monkeypatch.setattr(tempfile, "TemporaryDirectory", _FailCleanupTempDir) + monkeypatch.setattr( + auth_http, + "open_url", + lambda url, timeout=30, redirect_validator=None, extra_headers=None: ( + _ArchiveResponse( + url, + _make_zip(_valid_archive_files()), + "application/zip", + ) + ), + ) + + result = CliRunner().invoke( + app, + [ + "workflow", + "step", + "add", + "my-step", + "--from", + "https://example.com/pkg.zip", + ], + ) + + assert result.exit_code == 0, result.output + assert "installed" in result.output + assert "Could not remove temporary step archive directory" in result.output + assert extract_paths[0] in result.output + assert ( + project_dir / ".specify" / "workflows" / "steps" / "my-step" / "step.yml" + ).is_file() + for path in extract_paths: + import shutil + + shutil.rmtree(path, ignore_errors=True) + def test_from_rejects_non_https(self, project_dir, monkeypatch): from typer.testing import CliRunner diff --git a/tests/specify_cli/workflows/step/test_installer.py b/tests/specify_cli/workflows/step/test_installer.py index 1e788fd32c..189d5e2e4b 100644 --- a/tests/specify_cli/workflows/step/test_installer.py +++ b/tests/specify_cli/workflows/step/test_installer.py @@ -254,6 +254,46 @@ def test_byte_limit_boundary(tmp_path, monkeypatch): assert "total size limit" in str(exc.value) +def test_copy_enforces_byte_limit_if_source_grows_after_validation( + tmp_path, project_dir, monkeypatch +): + pkg = _write_package(tmp_path / "pkg", init_body="# init\n") + original_bytes = sum(path.stat().st_size for path in pkg.iterdir()) + monkeypatch.setattr(installer, "_MAX_STEP_PACKAGE_BYTES", original_bytes) + real_copy = installer._copy_regular_file + grew_source = False + copied_bytes: list[int] = [] + + def _grow_then_copy(source, target, expected_mode, remaining_bytes): + nonlocal grew_source + if not grew_source: + with open(source, "ab") as source_file: + source_file.write(b"x") + grew_source = True + copied = real_copy(source, target, expected_mode, remaining_bytes) + copied_bytes.append(copied) + return copied + + monkeypatch.setattr(installer, "_copy_regular_file", _grow_then_copy) + + with pytest.raises(installer.StepInstallError, match="while staging"): + installer.install_step_package(project_dir, "my-step", pkg, source="local") + + assert sum(copied_bytes) <= original_bytes + assert not (_steps_dir(project_dir) / "my-step").exists() + + +def test_copy_accepts_exact_byte_limit(tmp_path, project_dir, monkeypatch): + pkg = _write_package(tmp_path / "pkg", init_body="# init\n") + total_bytes = sum(path.stat().st_size for path in pkg.iterdir()) + monkeypatch.setattr(installer, "_MAX_STEP_PACKAGE_BYTES", total_bytes) + + installer.install_step_package(project_dir, "my-step", pkg, source="local") + + installed = _steps_dir(project_dir) / "my-step" + assert sum(path.stat().st_size for path in installed.iterdir()) == total_bytes + + # --------------------------------------------------------------------------- # Archive root resolution # --------------------------------------------------------------------------- @@ -341,6 +381,86 @@ def test_duplicate_install_rejected_without_force(tmp_path, project_dir): installer.install_step_package(project_dir, "my-step", pkg, source="local") +def test_force_rejects_casefolded_registered_id_collision(tmp_path, project_dir): + from specify_cli.workflows.step.catalog import StepRegistry + + old_dir = _write_package( + _steps_dir(project_dir) / "Foo", type_key="Foo", init_body="# old\n" + ) + _register(project_dir, "Foo") + replacement = _write_package(tmp_path / "replacement", type_key="foo") + + with pytest.raises(installer.StepInstallError, match="case-insensitively"): + installer.install_step_package( + project_dir, "foo", replacement, source="local", force=True + ) + + assert (old_dir / "__init__.py").read_text(encoding="utf-8") == "# old\n" + assert set(StepRegistry(project_dir).list()) == {"Foo"} + + +def test_force_rejects_casefolded_orphan_directory_collision(tmp_path, project_dir): + old_dir = _write_package( + _steps_dir(project_dir) / "Foo", type_key="Foo", init_body="# old\n" + ) + replacement = _write_package(tmp_path / "replacement", type_key="foo") + + with pytest.raises(installer.StepInstallError, match="case-insensitively"): + installer.install_step_package( + project_dir, "foo", replacement, source="local", force=True + ) + + assert (old_dir / "__init__.py").read_text(encoding="utf-8") == "# old\n" + + +def test_force_allows_exact_id_with_registered_package(tmp_path, project_dir): + from specify_cli.workflows.step.catalog import StepRegistry + + pkg = _write_package( + _steps_dir(project_dir) / "Foo", type_key="Foo", init_body="# old\n" + ) + _register(project_dir, "Foo") + replacement = _write_package( + tmp_path / "replacement", type_key="Foo", init_body="# replacement\n" + ) + + installer.install_step_package( + project_dir, "Foo", replacement, source="local", force=True + ) + + assert (pkg / "__init__.py").read_text(encoding="utf-8") == "# replacement\n" + assert set(StepRegistry(project_dir).list()) == {"Foo"} + + +@pytest.mark.parametrize("force", [False, True]) +def test_casefold_collision_guard_runs_for_force_and_regular_install( + tmp_path, project_dir, monkeypatch, force +): + class _Registry: + def list(self): + return {"Foo": {}} + + def is_installed(self, step_id): + return step_id == "foo" + + steps_dir = _steps_dir(project_dir) + steps_dir.mkdir(parents=True, exist_ok=True) + monkeypatch.setattr(installer, "resolve_steps_base_dir", lambda _root: steps_dir) + monkeypatch.setattr(installer, "_resolve_step_dir", lambda _base, _id: steps_dir / "foo") + monkeypatch.setattr(installer, "_reject_unsafe_destination", lambda _path: None) + monkeypatch.setattr(installer, "_reject_builtin_collision", lambda _step_id: None) + monkeypatch.setattr( + "specify_cli.workflows.step.catalog.StepRegistry", + lambda _root: _Registry(), + ) + pkg = _write_package(tmp_path / f"pkg-{force}", type_key="foo") + + with pytest.raises(installer.StepInstallError, match="case-insensitively"): + installer.install_step_package( + project_dir, "foo", pkg, source="local", force=force + ) + + # --------------------------------------------------------------------------- # Provenance # --------------------------------------------------------------------------- @@ -525,6 +645,74 @@ def _boom(self, step_id, metadata): ) == "# new\n" +def test_staging_cleanup_failure_warns_after_success( + tmp_path, project_dir, monkeypatch +): + pkg = _write_package(tmp_path / "pkg") + real_rmtree = installer.shutil.rmtree + residual_dirs: list[Path] = [] + + def _rmtree(path, *args, **kwargs): + path = Path(path) + if path.name.startswith(installer._WORK_DIR_PREFIX): + residual_dirs.append(path) + raise OSError("cleanup blocked") + return real_rmtree(path, *args, **kwargs) + + monkeypatch.setattr(installer.shutil, "rmtree", _rmtree) + try: + with pytest.warns( + UserWarning, match="Could not remove step staging directory" + ): + installer.install_step_package(project_dir, "my-step", pkg, source="local") + assert (_steps_dir(project_dir) / "my-step").is_dir() + assert len(residual_dirs) == 1 and residual_dirs[0].is_dir() + finally: + for residual_dir in residual_dirs: + if residual_dir.exists(): + real_rmtree(residual_dir) + + +def test_staging_cleanup_failure_preserves_primary_error( + tmp_path, project_dir, monkeypatch +): + pkg = _write_package(tmp_path / "pkg") + real_validate = installer.validate_step_package + real_rmtree = installer.shutil.rmtree + residual_dirs: list[Path] = [] + calls = 0 + + def _validate(package_dir, step_id): + nonlocal calls + calls += 1 + if calls == 2: + raise installer.StepInstallError("primary staged validation failure") + return real_validate(package_dir, step_id) + + def _rmtree(path, *args, **kwargs): + path = Path(path) + if path.name.startswith(installer._WORK_DIR_PREFIX): + residual_dirs.append(path) + raise OSError("cleanup blocked") + return real_rmtree(path, *args, **kwargs) + + monkeypatch.setattr(installer, "validate_step_package", _validate) + monkeypatch.setattr(installer.shutil, "rmtree", _rmtree) + try: + with pytest.raises( + installer.StepInstallError, match="primary staged validation failure" + ) as exc: + installer.install_step_package( + project_dir, "my-step", pkg, source="local" + ) + assert len(residual_dirs) == 1 and residual_dirs[0].is_dir() + assert str(residual_dirs[0]) in exc.value.__notes__[0] + finally: + for residual_dir in residual_dirs: + if residual_dir.exists(): + real_rmtree(residual_dir) + + def test_force_removal_failure_warns_reinstall(tmp_path, project_dir, monkeypatch): target = _steps_dir(project_dir) / "my-step" _write_package(target, init_body="# old\n") From a78b752eac3a3078070fda9ca7ff79a38c78b3ae Mon Sep 17 00:00:00 2001 From: Markus Date: Mon, 28 Sep 2026 15:24:36 +0200 Subject: [PATCH 04/16] fix(workflows): address remaining step install findings Allow archive byte-format detection without transport hints, escape user-controlled installer errors, reject case-folded orphan directories, and exercise install locking on Windows. Assisted-by: OpenCode (model: github-copilot/gpt-6-luna, autonomous) --- src/specify_cli/workflows/step/_helpers.py | 4 +- src/specify_cli/workflows/step/command_add.py | 24 ++---- src/specify_cli/workflows/step/installer.py | 20 ++--- .../workflows/step/test_command_add.py | 55 ++++++++++++ .../workflows/step/test_installer.py | 85 ++++++++++++++----- 5 files changed, 139 insertions(+), 49 deletions(-) diff --git a/src/specify_cli/workflows/step/_helpers.py b/src/specify_cli/workflows/step/_helpers.py index 34ada2f306..41ba275815 100644 --- a/src/specify_cli/workflows/step/_helpers.py +++ b/src/specify_cli/workflows/step/_helpers.py @@ -34,7 +34,7 @@ def _validate_step_id_or_exit(step_id: str) -> None: try: validate_step_id(step_id) except StepInstallError as exc: - cli.console.print(f"[red]Error:[/red] {exc}") + cli.console.print(f"[red]Error:[/red] {cli._escape_markup(str(exc))}") raise cli.typer.Exit(1) from exc @@ -43,5 +43,5 @@ def _resolve_steps_base_dir_or_exit(project_root: cli.Path) -> cli.Path: try: return resolve_steps_base_dir(project_root) except StepInstallError as exc: - cli.console.print(f"[red]Error:[/red] {exc}") + cli.console.print(f"[red]Error:[/red] {cli._escape_markup(str(exc))}") raise cli.typer.Exit(1) from exc diff --git a/src/specify_cli/workflows/step/command_add.py b/src/specify_cli/workflows/step/command_add.py index 75f7b9315e..7919d3aad1 100644 --- a/src/specify_cli/workflows/step/command_add.py +++ b/src/specify_cli/workflows/step/command_add.py @@ -96,11 +96,11 @@ def _install_from_url( _ = parsed.port except ValueError: raise installer.StepInstallError( - f"Invalid URL: {cli._escape_markup(from_url)}" + f"Invalid URL: {from_url}" ) from None if not hostname: raise installer.StepInstallError( - f"Invalid URL: {cli._escape_markup(from_url)}" + f"Invalid URL: {from_url}" ) if not cli.is_https_or_localhost_http(from_url): raise installer.StepInstallError( @@ -156,7 +156,7 @@ def _install_from_url( final_url = resp.geturl() if not cli.is_https_or_localhost_http(final_url): raise installer.StepInstallError( - f"URL redirected to non-HTTPS: {cli._escape_markup(final_url)}" + f"URL redirected to non-HTTPS: {final_url}" ) content_type = ( resp.getheader("Content-Type") @@ -174,18 +174,13 @@ def _install_from_url( ] recognized = [item for item in declarations if item[2] is not None] archive_format = recognized[0][2] if recognized else None - if archive_format is None: - raise installer.StepInstallError( - "URL does not reference a supported archive " - "(.zip, .tar.gz, or .tgz)" - ) if any(item[2] != archive_format for item in recognized): details = ", ".join( f"{label} declares {declared}" for label, _value, declared in recognized ) raise installer.StepInstallError( - f"Archive format mismatch: {cli._escape_markup(details)}" + f"Archive format mismatch: {details}" ) downloaded = cli.read_response_limited( resp, @@ -194,7 +189,8 @@ def _install_from_url( ) with tempfile.NamedTemporaryFile( - suffix=cli.archive_suffix(archive_format), delete=False + suffix=cli.archive_suffix(archive_format) if archive_format else ".archive", + delete=False, ) as tmp: tmp_path = cli.Path(tmp.name) tmp.write(downloaded) @@ -213,9 +209,7 @@ def _install_from_url( cli.safe_extract_archive( tmp_path, extracted_root, - source_name=next( - value for _label, value, declared in recognized if declared is not None - ), + source_name=recognized[0][1] if recognized else None, content_type=content_type, ) package_root = installer.resolve_package_root(extracted_root) @@ -255,7 +249,7 @@ def _install_from_url( raise except Exception as exc: raise installer.StepInstallError( - f"Failed to install step from URL: {cli._escape_markup(str(exc))}" + f"Failed to install step from URL: {exc}" ) from exc finally: _cleanup_download_tmp_path(tmp_path) @@ -539,5 +533,5 @@ def workflow_step_add( cli.console.print( f"[yellow]Warning:[/yellow] {cli._escape_markup(note)}" ) - cli.console.print(f"[red]Error:[/red] {exc}") + cli.console.print(f"[red]Error:[/red] {cli._escape_markup(str(exc))}") raise cli.typer.Exit(1) from exc diff --git a/src/specify_cli/workflows/step/installer.py b/src/specify_cli/workflows/step/installer.py index 4e990a1cd3..c45f348114 100644 --- a/src/specify_cli/workflows/step/installer.py +++ b/src/specify_cli/workflows/step/installer.py @@ -435,16 +435,10 @@ def _check_duplicate( for existing_name in existing_names: if existing_name == step_id or existing_name.casefold() != folded_id: continue - existing_dir = step_dir.parent / existing_name - try: - same_destination = step_dir.exists() and existing_dir.samefile(step_dir) - except OSError: - same_destination = False - if not same_destination: - raise StepInstallError( - f"Step ID '{step_id}' collides case-insensitively with " - f"existing step directory '{existing_name}'" - ) + raise StepInstallError( + f"Step ID '{step_id}' collides case-insensitively with " + f"existing step directory '{existing_name}'" + ) except FileNotFoundError: pass except OSError as exc: @@ -457,12 +451,12 @@ def _check_duplicate( if registry.is_installed(step_id): raise StepInstallError( f"Step type '{step_id}' is already installed. Remove it first with: " - f"[cyan]specify workflow step remove {step_id}[/cyan]" + f"specify workflow step remove {step_id}" ) if step_dir.exists(): raise StepInstallError( f"Step directory already exists at '{step_dir}'. Remove it manually " - f"or use: [cyan]specify workflow step remove {step_id}[/cyan]" + f"or use: specify workflow step remove {step_id}" ) @@ -663,7 +657,7 @@ def _replace_install( if not force: raise StepInstallError( f"Step directory already exists at '{step_dir}'. Remove it manually " - f"or use: [cyan]specify workflow step remove {step_id}[/cyan]" + f"or use: specify workflow step remove {step_id}" ) # --force replacement: the replacement is fully staged and validated, # so it is safe to remove the previous installation now. diff --git a/tests/specify_cli/workflows/step/test_command_add.py b/tests/specify_cli/workflows/step/test_command_add.py index ad45e7fc9b..143774324d 100644 --- a/tests/specify_cli/workflows/step/test_command_add.py +++ b/tests/specify_cli/workflows/step/test_command_add.py @@ -1185,6 +1185,61 @@ def test_from_archive_installs( project_dir / ".specify" / "workflows" / "steps" / "my-step" / "step.yml" ).is_file() + def test_from_github_api_asset_detects_archive_from_bytes( + self, project_dir, monkeypatch + ): + import typer + from typer.testing import CliRunner + + from specify_cli import app + from specify_cli.authentication import http as auth_http + + monkeypatch.chdir(project_dir) + monkeypatch.setattr(typer, "confirm", lambda *a, **k: True) + api_asset_url = ( + "https://api.github.com/repos/example/project/releases/assets/123" + ) + body = _make_zip(_valid_archive_files()) + monkeypatch.setattr( + auth_http, + "open_url", + lambda url, timeout=30, redirect_validator=None, extra_headers=None: ( + _ArchiveResponse(url, body, "application/octet-stream") + ), + ) + + result = CliRunner().invoke( + app, ["workflow", "step", "add", "my-step", "--from", api_asset_url] + ) + + assert result.exit_code == 0, result.output + assert ( + project_dir / ".specify" / "workflows" / "steps" / "my-step" / "step.yml" + ).is_file() + + def test_install_error_escapes_rich_markup_from_step_metadata( + self, project_dir, tmp_path, monkeypatch + ): + from typer.testing import CliRunner + + from specify_cli import app + + package = _write_package(tmp_path, type_key="my-step") + (package / "step.yml").write_text( + "step:\n type_key: '[/]'\n", encoding="utf-8" + ) + monkeypatch.chdir(project_dir) + + result = CliRunner().invoke( + app, + ["workflow", "step", "add", "my-step", "--dev", str(package)], + ) + + assert result.exit_code != 0 + assert "does not match" in result.output + assert "MarkupError" not in result.output + assert "Traceback" not in result.output + def test_archive_temp_directory_cleanup_failure_warns_after_commit( self, project_dir, monkeypatch ): diff --git a/tests/specify_cli/workflows/step/test_installer.py b/tests/specify_cli/workflows/step/test_installer.py index 189d5e2e4b..d0df8364bf 100644 --- a/tests/specify_cli/workflows/step/test_installer.py +++ b/tests/specify_cli/workflows/step/test_installer.py @@ -399,11 +399,41 @@ def test_force_rejects_casefolded_registered_id_collision(tmp_path, project_dir) assert set(StepRegistry(project_dir).list()) == {"Foo"} -def test_force_rejects_casefolded_orphan_directory_collision(tmp_path, project_dir): +def test_force_rejects_casefolded_orphan_directory_collision( + tmp_path, project_dir, monkeypatch +): + from pathlib import Path + old_dir = _write_package( _steps_dir(project_dir) / "Foo", type_key="Foo", init_body="# old\n" ) replacement = _write_package(tmp_path / "replacement", type_key="foo") + destination = _steps_dir(project_dir) / "foo" + real_exists = Path.exists + real_samefile = Path.samefile + + # Simulate a case-insensitive filesystem while running on Linux: both + # spellings resolve to the same destination even though they differ. + monkeypatch.setattr( + Path, + "exists", + lambda self: True if self == destination else real_exists(self), + ) + real_is_dir = Path.is_dir + monkeypatch.setattr( + Path, + "is_dir", + lambda self: True if self == destination else real_is_dir(self), + ) + monkeypatch.setattr( + Path, + "samefile", + lambda self, other: ( + True + if self == old_dir and other == destination + else real_samefile(self, other) + ), + ) with pytest.raises(installer.StepInstallError, match="case-insensitively"): installer.install_step_package( @@ -789,11 +819,10 @@ def _install_race_setup(tmp_path, project_dir, monkeypatch, *, force_b): """Drive two concurrent installs against the install lock. Installer A is paused inside the locked critical section while installer B - is guaranteed to be blocked on the lock (its ``fcntl.flock`` attempt has + is guaranteed to be blocked on the platform lock (its lock attempt has been observed). The caller resumes A via the returned ``release_a`` event, then joins both threads and inspects ``outcomes``. """ - import fcntl import threading pkg_a = _write_package(tmp_path / "pkg-a", init_body="# a\n") @@ -810,7 +839,6 @@ class Race: race = Race() real_replace = installer._replace_install - real_flock = fcntl.flock def _replace(step_dir, staged_dir, registry, step_id, entry, *, force): if threading.current_thread().name == "installer-a": @@ -823,16 +851,41 @@ def _replace(step_dir, staged_dir, registry, step_id, entry, *, force): step_dir, staged_dir, registry, step_id, entry, force=force ) - def _flock(fd, operation): - if ( - threading.current_thread().name == "installer-b" - and operation == fcntl.LOCK_EX - ): - race.b_attempted_lock.set() - return real_flock(fd, operation) - monkeypatch.setattr(installer, "_replace_install", _replace) - monkeypatch.setattr(fcntl, "flock", _flock) + if os.name == "nt": + import msvcrt + + real_locking = msvcrt.locking + + def _locking(fd, operation, nbytes): + is_installer_b = threading.current_thread().name == "installer-b" + try: + result = real_locking(fd, operation, nbytes) + except OSError: + if is_installer_b and operation == msvcrt.LK_NBLCK: + # The non-blocking Windows lock attempt has now confirmed + # contention with installer A. + race.b_attempted_lock.set() + raise + if is_installer_b and operation == msvcrt.LK_NBLCK: + race.b_attempted_lock.set() + return result + + monkeypatch.setattr(msvcrt, "locking", _locking) + else: + import fcntl + + real_flock = fcntl.flock + + def _flock(fd, operation): + if ( + threading.current_thread().name == "installer-b" + and operation == fcntl.LOCK_EX + ): + race.b_attempted_lock.set() + return real_flock(fd, operation) + + monkeypatch.setattr(fcntl, "flock", _flock) def _install(label, pkg, force): try: @@ -872,9 +925,6 @@ def test_install_lock_blocks_concurrent_duplicate(tmp_path, project_dir, monkeyp B can never enter the swap while A holds it; once A commits, B reloads the registry inside the lock and fails as a duplicate instead of overwriting. """ - if os.name == "nt": - pytest.skip("fcntl.flock is POSIX-only") - race = _install_race_setup(tmp_path, project_dir, monkeypatch, force_b=False) # A holds the lock, so B cannot have reached the directory swap. assert not race.b_inside_replace.is_set() @@ -894,9 +944,6 @@ def test_install_lock_blocks_concurrent_duplicate(tmp_path, project_dir, monkeyp def test_install_lock_serializes_force_replace(tmp_path, project_dir, monkeypatch): """A forced install swaps only after the lock is released by the first.""" - if os.name == "nt": - pytest.skip("fcntl.flock is POSIX-only") - race = _install_race_setup(tmp_path, project_dir, monkeypatch, force_b=True) # B is blocked on the lock and has not swapped A's package yet. assert not race.b_inside_replace.is_set() From cf42dcc1dfeb867b65f49bc62c2d600199104cc3 Mon Sep 17 00:00:00 2001 From: Markus Wondrak Date: Tue, 29 Sep 2026 10:22:23 +0200 Subject: [PATCH 05/16] fix(workflows): bound step package traversal depth and entries Replace recursive package validation and staging walks with iterative traversal so deeply nested --dev trees fail with StepInstallError instead of RecursionError. Enforce a 32-level directory depth limit and count directories toward the 512-entry package budget in both validation and copy, covering trees that change after validation. Assisted-by: GitHub Copilot CLI (model: unknown, Auto mode, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/reference/workflows.md | 3 +- src/specify_cli/workflows/step/_helpers.py | 2 + src/specify_cli/workflows/step/installer.py | 123 +++++++++++------- .../workflows/step/test_installer.py | 70 ++++++++++ 4 files changed, 151 insertions(+), 47 deletions(-) diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index 9caa1b5a3b..71634c162c 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -653,7 +653,8 @@ Every source is validated identically before anything is committed: directory are rejected — including inside excluded directories. - `.git`, `__pycache__`, and `.DS_Store` entries are excluded from the copy and from the limits. -- The installed-package policy permits at most **512 retained files** and +- The installed-package policy permits at most **512 retained entries** (files + and directories combined), at most **32 levels** of directory nesting, and **50 MiB** of retained content. Excluded entries do not consume this budget. - Archive URLs also pass transport/extraction safety limits before package validation: at most 512 archive entries, 50 MiB downloaded or extracted, and diff --git a/src/specify_cli/workflows/step/_helpers.py b/src/specify_cli/workflows/step/_helpers.py index 41ba275815..6b8fdd8576 100644 --- a/src/specify_cli/workflows/step/_helpers.py +++ b/src/specify_cli/workflows/step/_helpers.py @@ -11,6 +11,7 @@ from .. import _commands as cli from .installer import ( _MAX_STEP_PACKAGE_BYTES, + _MAX_STEP_PACKAGE_DEPTH, _MAX_STEP_PACKAGE_FILES, StepInstallError, resolve_steps_base_dir, @@ -19,6 +20,7 @@ __all__ = [ "_MAX_STEP_PACKAGE_BYTES", + "_MAX_STEP_PACKAGE_DEPTH", "_MAX_STEP_PACKAGE_FILES", "StepInstallError", "resolve_steps_base_dir", diff --git a/src/specify_cli/workflows/step/installer.py b/src/specify_cli/workflows/step/installer.py index c45f348114..a5c49a6f14 100644 --- a/src/specify_cli/workflows/step/installer.py +++ b/src/specify_cli/workflows/step/installer.py @@ -28,6 +28,7 @@ # files. These ceilings apply uniformly to catalog, local, and archive sources. _MAX_STEP_PACKAGE_FILES = 512 _MAX_STEP_PACKAGE_BYTES = 50 * 1024 * 1024 # 50 MiB +_MAX_STEP_PACKAGE_DEPTH = 32 _COPY_CHUNK_BYTES = 64 * 1024 # Files/dirs never copied into (or counted as part of) an installed step @@ -235,39 +236,48 @@ def _walk_package_tree(package_dir: Path): Descends into excluded directories so a symlink or special file hiding inside ``.git``/``__pycache__`` is still rejected, but never follows a - symlink. Raises :class:`StepInstallError` on any symlink or object that is - neither a regular file nor a directory. + symlink. Raises :class:`StepInstallError` on any symlink, object that is + neither a regular file nor a directory, or nesting deeper than + ``_MAX_STEP_PACKAGE_DEPTH``. Iterative so deep trees cannot exhaust the + Python recursion limit. """ - def _walk(current: Path, excluded_prefix: bool): + def _children(current: Path, depth: int, excluded_prefix: bool): try: entries = sorted(os.scandir(current), key=lambda entry: entry.name) except OSError as exc: raise StepInstallError( f"Failed to read step package directory '{current}': {exc}" ) from exc - for entry in entries: - try: - mode = entry.stat(follow_symlinks=False).st_mode - except OSError as exc: - raise StepInstallError( - f"Failed to inspect step package entry '{entry.path}': {exc}" - ) from exc - path = Path(entry.path) - if stat.S_ISLNK(mode): - raise StepInstallError(f"Step package contains symlink: {path}") - excluded = excluded_prefix or entry.name in EXCLUDE_NAMES - if stat.S_ISDIR(mode): - yield path, True, excluded - yield from _walk(path, excluded) - elif stat.S_ISREG(mode): - yield path, False, excluded - else: + return [(entry, depth, excluded_prefix) for entry in reversed(entries)] + + stack = _children(package_dir, 1, False) + while stack: + entry, depth, excluded_prefix = stack.pop() + try: + mode = entry.stat(follow_symlinks=False).st_mode + except OSError as exc: + raise StepInstallError( + f"Failed to inspect step package entry '{entry.path}': {exc}" + ) from exc + path = Path(entry.path) + if stat.S_ISLNK(mode): + raise StepInstallError(f"Step package contains symlink: {path}") + excluded = excluded_prefix or entry.name in EXCLUDE_NAMES + if stat.S_ISDIR(mode): + if depth > _MAX_STEP_PACKAGE_DEPTH: raise StepInstallError( - f"Step package contains unsupported file: {path}" + f"Step package exceeds the {_MAX_STEP_PACKAGE_DEPTH}-level " + "directory depth limit" ) - - yield from _walk(package_dir, False) + yield path, True, excluded + stack.extend(_children(path, depth + 1, excluded)) + elif stat.S_ISREG(mode): + yield path, False, excluded + else: + raise StepInstallError( + f"Step package contains unsupported file: {path}" + ) def _parse_step_metadata(step_yml_text: str, step_id: str) -> dict[str, Any]: @@ -338,21 +348,28 @@ def validate_step_package(package_dir: Path, step_id: str) -> dict[str, Any]: ) retained_files = 0 + retained_dirs = 0 retained_bytes = 0 for path, is_dir, excluded in _walk_package_tree(package_dir): - if is_dir or excluded: + if excluded: continue - retained_files += 1 - try: - retained_bytes += path.lstat().st_size - except OSError as exc: - raise StepInstallError( - f"Failed to inspect step package file '{path}': {exc}" - ) from exc + if is_dir: + retained_dirs += 1 + else: + retained_files += 1 + try: + retained_bytes += path.lstat().st_size + except OSError as exc: + raise StepInstallError( + f"Failed to inspect step package file '{path}': {exc}" + ) from exc + if retained_files + retained_dirs > _MAX_STEP_PACKAGE_FILES: + break - if retained_files > _MAX_STEP_PACKAGE_FILES: + if retained_files + retained_dirs > _MAX_STEP_PACKAGE_FILES: raise StepInstallError( - f"Step package contains {retained_files} files, exceeding the " + f"Step package contains too many entries ({retained_files} files, " + f"{retained_dirs} directories), exceeding the " f"{_MAX_STEP_PACKAGE_FILES}-file limit" ) if retained_bytes > _MAX_STEP_PACKAGE_BYTES: @@ -546,15 +563,24 @@ def _string_value(metadata: Mapping[str, Any], field: str) -> str | None: def _copy_package_tree(source_dir: Path, target_dir: Path) -> None: - """Recursively copy *source_dir* into *target_dir*, skipping excludes. + """Copy *source_dir* into *target_dir*, skipping excludes. Refuses to follow a symlink encountered mid-copy so a source swapped after - validation cannot smuggle external content into the staged package. + validation cannot smuggle external content into the staged package, and + re-enforces the depth, entry, and byte budgets. Iterative so a deep tree + cannot exhaust the Python recursion limit. """ - remaining_bytes = [_MAX_STEP_PACKAGE_BYTES] - - def _copy(current: Path, destination: Path) -> None: + remaining_bytes = _MAX_STEP_PACKAGE_BYTES + copied_entries = 0 + pending = [(source_dir, target_dir, 0)] + while pending: + current, destination, depth = pending.pop() + if depth > _MAX_STEP_PACKAGE_DEPTH: + raise StepInstallError( + f"Step package exceeds the {_MAX_STEP_PACKAGE_DEPTH}-level " + "directory depth limit" + ) try: destination.mkdir(parents=True, exist_ok=True) except OSError as exc: @@ -579,17 +605,22 @@ def _copy(current: Path, destination: Path) -> None: raise StepInstallError( f"Step package contains symlink: {entry.path}" ) - if stat.S_ISDIR(mode): - _copy(Path(entry.path), target) - elif stat.S_ISREG(mode): - copied = _copy_regular_file(entry.path, target, mode, remaining_bytes[0]) - remaining_bytes[0] -= copied - else: + if not (stat.S_ISDIR(mode) or stat.S_ISREG(mode)): raise StepInstallError( f"Step package contains unsupported file: {entry.path}" ) - - _copy(source_dir, target_dir) + copied_entries += 1 + if copied_entries > _MAX_STEP_PACKAGE_FILES: + raise StepInstallError( + "Step package contains too many entries, exceeding the " + f"{_MAX_STEP_PACKAGE_FILES}-file limit" + ) + if stat.S_ISDIR(mode): + pending.append((Path(entry.path), target, depth + 1)) + else: + remaining_bytes -= _copy_regular_file( + entry.path, target, mode, remaining_bytes + ) def _copy_regular_file( diff --git a/tests/specify_cli/workflows/step/test_installer.py b/tests/specify_cli/workflows/step/test_installer.py index d0df8364bf..94a871b33a 100644 --- a/tests/specify_cli/workflows/step/test_installer.py +++ b/tests/specify_cli/workflows/step/test_installer.py @@ -242,6 +242,76 @@ def test_file_limit_boundary(tmp_path, monkeypatch): assert "2-file limit" in str(exc.value) +def _nest_dirs(root: Path, depth: int) -> Path: + current = root + for _ in range(depth): + current = current / "d" + current.mkdir(parents=True) + (current / "leaf.py").write_text("x", encoding="utf-8") + return current + + +def test_directories_count_toward_file_limit(tmp_path, monkeypatch): + pkg = _write_package(tmp_path / "pkg") + monkeypatch.setattr(installer, "_MAX_STEP_PACKAGE_FILES", 3) + (pkg / "empty-a").mkdir() + installer.validate_step_package(pkg, "my-step") + + (pkg / "empty-b").mkdir() + with pytest.raises(installer.StepInstallError) as exc: + installer.validate_step_package(pkg, "my-step") + assert "3-file limit" in str(exc.value) + + +def test_depth_limit_boundary(tmp_path, monkeypatch): + pkg = _write_package(tmp_path / "pkg") + monkeypatch.setattr(installer, "_MAX_STEP_PACKAGE_DEPTH", 4) + deepest = _nest_dirs(pkg, 4) + installer.validate_step_package(pkg, "my-step") + + (deepest / "d").mkdir() + with pytest.raises(installer.StepInstallError, match="4-level directory depth"): + installer.validate_step_package(pkg, "my-step") + + +def test_default_depth_limit_rejects_deep_tree_without_recursion_error(tmp_path): + pkg = _write_package(tmp_path / "pkg") + _nest_dirs(pkg, installer._MAX_STEP_PACKAGE_DEPTH) + installer.validate_step_package(pkg, "my-step") + + _nest_dirs(tmp_path / "deep", installer._MAX_STEP_PACKAGE_DEPTH + 1) + deep = _write_package(tmp_path / "deep") + with pytest.raises(installer.StepInstallError, match="directory depth limit"): + installer.validate_step_package(deep, "my-step") + + +def test_copy_enforces_depth_limit_if_source_deepens_after_validation( + tmp_path, monkeypatch +): + pkg = _write_package(tmp_path / "pkg") + monkeypatch.setattr(installer, "_MAX_STEP_PACKAGE_DEPTH", 4) + deepest = _nest_dirs(pkg, 4) + installer._copy_package_tree(pkg, tmp_path / "ok") + assert (tmp_path / "ok" / "d" / "d" / "d" / "d" / "leaf.py").is_file() + + (deepest / "d").mkdir() + with pytest.raises(installer.StepInstallError, match="4-level directory depth"): + installer._copy_package_tree(pkg, tmp_path / "too-deep") + + +def test_copy_enforces_entry_limit_if_source_grows_after_validation( + tmp_path, monkeypatch +): + pkg = _write_package(tmp_path / "pkg") + monkeypatch.setattr(installer, "_MAX_STEP_PACKAGE_FILES", 3) + (pkg / "empty-a").mkdir() + installer._copy_package_tree(pkg, tmp_path / "ok") + + (pkg / "empty-b").mkdir() + with pytest.raises(installer.StepInstallError, match="3-file limit"): + installer._copy_package_tree(pkg, tmp_path / "too-many") + + def test_byte_limit_boundary(tmp_path, monkeypatch): pkg = _write_package(tmp_path / "pkg") total = (pkg / "step.yml").stat().st_size + (pkg / "__init__.py").stat().st_size From 2c612fee7ae302a92b56c1c1b5150f24e9c2faf2 Mon Sep 17 00:00:00 2001 From: Markus Wondrak Date: Tue, 29 Sep 2026 10:26:11 +0200 Subject: [PATCH 06/16] test(workflows): compare resolved paths in force-reinstall failure mocks The installer operates on resolved step paths, so on macOS (where the temp directory lives under the /var -> /private/var symlink) the rmtree and os.replace mocks never matched the unresolved target and the expected StepInstallError was not raised. Compare resolved paths instead. Assisted-by: GitHub Copilot CLI (model: unknown, Auto mode, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/specify_cli/workflows/step/test_installer.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/specify_cli/workflows/step/test_installer.py b/tests/specify_cli/workflows/step/test_installer.py index 94a871b33a..6545a74f83 100644 --- a/tests/specify_cli/workflows/step/test_installer.py +++ b/tests/specify_cli/workflows/step/test_installer.py @@ -820,9 +820,11 @@ def test_force_removal_failure_warns_reinstall(tmp_path, project_dir, monkeypatc new_pkg = _write_package(tmp_path / "pkg", init_body="# new\n") real_rmtree = installer.shutil.rmtree + # The installer operates on resolved paths (e.g. /private/var on macOS). + resolved_target = target.resolve() def _rmtree(path, *args, **kwargs): - if Path(path) == target: + if Path(path).resolve() == resolved_target: raise OSError("cannot remove") return real_rmtree(path, *args, **kwargs) @@ -843,9 +845,10 @@ def test_force_publication_failure_warns_reinstall(tmp_path, project_dir, monkey new_pkg = _write_package(tmp_path / "pkg", init_body="# new\n") real_replace = installer.os.replace + resolved_target = target.resolve() def _replace(src, dst, *args, **kwargs): - if Path(dst) == target: + if Path(dst).resolve() == resolved_target: raise OSError("rename failed") return real_replace(src, dst, *args, **kwargs) From 9b821280b056891aa17cf6ab47ad97544abebbbd Mon Sep 17 00:00:00 2001 From: Markus Wondrak Date: Tue, 29 Sep 2026 10:35:59 +0200 Subject: [PATCH 07/16] Update documentation on installed-package policy limits Clarify the impact of excluded entries on budget limits. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- docs/reference/workflows.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index 71634c162c..28d0c3492b 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -655,7 +655,8 @@ Every source is validated identically before anything is committed: from the limits. - The installed-package policy permits at most **512 retained entries** (files and directories combined), at most **32 levels** of directory nesting, and - **50 MiB** of retained content. Excluded entries do not consume this budget. + **50 MiB** of retained content. Excluded entries do not consume the + entry-count or byte budgets, but their directory depth is still validated. - Archive URLs also pass transport/extraction safety limits before package validation: at most 512 archive entries, 50 MiB downloaded or extracted, and 10 MiB per archive member. Catalog files have a 50 MiB per-response bound. From f4cd69870072b33cfbee9b05eae2fb93cdc69408 Mon Sep 17 00:00:00 2001 From: Markus Wondrak Date: Tue, 29 Sep 2026 14:08:54 +0200 Subject: [PATCH 08/16] Refactor step ID collision handling in installer.py Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/step/installer.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/specify_cli/workflows/step/installer.py b/src/specify_cli/workflows/step/installer.py index a5c49a6f14..da804c35de 100644 --- a/src/specify_cli/workflows/step/installer.py +++ b/src/specify_cli/workflows/step/installer.py @@ -448,13 +448,20 @@ def _check_duplicate( ) try: - existing_names = (item.name for item in step_dir.parent.iterdir()) - for existing_name in existing_names: - if existing_name == step_id or existing_name.casefold() != folded_id: + destination_exists = step_dir.exists() + for existing_path in step_dir.parent.iterdir(): + existing_name = existing_path.name + if existing_name == step_id: + continue + aliases_destination = ( + destination_exists and existing_path.samefile(step_dir) + ) + if existing_name.casefold() != folded_id and not aliases_destination: continue raise StepInstallError( - f"Step ID '{step_id}' collides case-insensitively with " - f"existing step directory '{existing_name}'" + f"Step ID '{step_id}' collides case-insensitively or resolves to " + f"the same filesystem path as existing step directory " + f"'{existing_name}'" ) except FileNotFoundError: pass From 6ab3230b1cc9e7f1aa2cf1663c68b169703acfbc Mon Sep 17 00:00:00 2001 From: Markus Wondrak Date: Tue, 29 Sep 2026 16:22:18 +0200 Subject: [PATCH 09/16] fix(workflows): serialize step remove with installs via shared project lock `workflow step remove` mutated the step directory and registry without the install lock, so a concurrent install could resurrect a removed entry or leave a newly installed package unregistered through a stale registry snapshot. - Extract the duplicated workflow/step lock logic into `shared_infra._exclusive_project_lock`; both install transactions now use it. - Run step removal under the step lock and resolve the registry and step directory only after the lock is acquired. - Report only lock-acquisition failures as lock errors; exceptions raised inside the critical section now propagate unchanged. - Add helper tests and deterministic remove-vs-install race tests (both orderings), plus lock-failure and rollback coverage. Assisted-by: GitHub Copilot CLI (model: Claude Opus 5.5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/shared_infra.py | 55 +++++ src/specify_cli/workflows/_commands.py | 47 +--- .../workflows/step/command_remove.py | 45 ++-- src/specify_cli/workflows/step/installer.py | 65 ++---- tests/lock_helpers.py | 67 ++++++ .../workflows/step/test_command_remove.py | 204 ++++++++++++++++++ .../workflows/step/test_installer.py | 38 +--- tests/test_shared_infra_lock.py | 118 ++++++++++ 8 files changed, 496 insertions(+), 143 deletions(-) create mode 100644 tests/lock_helpers.py create mode 100644 tests/test_shared_infra_lock.py diff --git a/src/specify_cli/shared_infra.py b/src/specify_cli/shared_infra.py index 3aff73ae49..9649fe697a 100644 --- a/src/specify_cli/shared_infra.py +++ b/src/specify_cli/shared_infra.py @@ -2,6 +2,7 @@ from __future__ import annotations +import contextlib import hashlib import hmac import logging @@ -233,6 +234,60 @@ def _validate_safe_shared_directory(project_path: Path, directory: Path) -> None raise ValueError(f"Shared infrastructure directory escapes project root: {label}") from None +@contextlib.contextmanager +def _exclusive_project_lock(project_root: Path, lock_name: str, *, context: str): + """Hold an exclusive inter-process lock on ``.specify/``. + + Callers use one lock file per mutable project resource so that its + directory and registry changes are serialized across processes. Every + failure to acquire the lock is raised as ``OSError``. + """ + project_root = Path(project_root) + lock_dir = project_root / ".specify" + try: + _ensure_safe_shared_directory( + project_root, lock_dir, context=f"{context} lock directory" + ) + except ValueError as exc: + raise OSError(str(exc)) from exc + lock_file = lock_dir / lock_name + if lock_file.is_symlink(): + raise OSError(f"Refusing to use symlinked {context} lock: {lock_file}") + + flags = os.O_RDWR | os.O_CREAT + flags |= getattr(os, "O_NOFOLLOW", 0) + flags |= getattr(os, "O_CLOEXEC", 0) + fd = os.open(lock_file, flags, 0o600) + try: + if lock_file.is_symlink(): + raise OSError(f"Refusing to use symlinked {context} lock: {lock_file}") + # Call the lock primitives through their modules so tests can observe + # contention by patching ``msvcrt.locking`` / ``fcntl.flock``. + if os.name == "nt": + import errno + import msvcrt + import time + + if os.fstat(fd).st_size == 0: + os.write(fd, b"\0") + while True: + os.lseek(fd, 0, os.SEEK_SET) + try: + msvcrt.locking(fd, msvcrt.LK_NBLCK, 1) + break + except OSError as exc: + if exc.errno not in (errno.EACCES, errno.EDEADLK): + raise + time.sleep(0.05) + else: + import fcntl + + fcntl.flock(fd, fcntl.LOCK_EX) + yield + finally: + os.close(fd) + + def _ensure_safe_shared_destination( project_path: Path, dest: Path, diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index de01789d0c..423fdb3e09 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -434,51 +434,12 @@ def _stage_workflow_file( @contextlib.contextmanager def _workflow_install_transaction(project_root: Path): """Serialize workflow file swaps with their registry updates.""" - from ..shared_infra import _ensure_safe_shared_directory + from ..shared_infra import _exclusive_project_lock - lock_dir = project_root / ".specify" - try: - _ensure_safe_shared_directory( - project_root, lock_dir, context="workflow install lock directory" - ) - except ValueError as exc: - raise OSError(str(exc)) from exc - lock_file = lock_dir / ".workflow-install.lock" - if lock_file.is_symlink(): - raise OSError(f"Refusing to use symlinked workflow install lock: {lock_file}") - - flags = os.O_RDWR | os.O_CREAT - flags |= getattr(os, "O_NOFOLLOW", 0) - flags |= getattr(os, "O_CLOEXEC", 0) - fd = os.open(lock_file, flags, 0o600) - try: - if lock_file.is_symlink(): - raise OSError( - f"Refusing to use symlinked workflow install lock: {lock_file}" - ) - if os.name == "nt": - import errno - import msvcrt - import time - - if os.fstat(fd).st_size == 0: - os.write(fd, b"\0") - while True: - os.lseek(fd, 0, os.SEEK_SET) - try: - msvcrt.locking(fd, msvcrt.LK_NBLCK, 1) - break - except OSError as exc: - if exc.errno not in (errno.EACCES, errno.EDEADLK): - raise - time.sleep(0.05) - else: - import fcntl - - fcntl.flock(fd, fcntl.LOCK_EX) + with _exclusive_project_lock( + project_root, ".workflow-install.lock", context="workflow install" + ): yield - finally: - os.close(fd) def _commit_workflow_file( diff --git a/src/specify_cli/workflows/step/command_remove.py b/src/specify_cli/workflows/step/command_remove.py index 58ec7602f3..78bb9a983d 100644 --- a/src/specify_cli/workflows/step/command_remove.py +++ b/src/specify_cli/workflows/step/command_remove.py @@ -8,16 +8,16 @@ from . import _helpers as step_helpers -@step_app.command("remove") -def workflow_step_remove( - step_id: str = cli.typer.Argument(..., help="Step type ID to uninstall"), -): - """Uninstall a custom step type.""" - from .catalog import StepRegistry, StepValidationError +def _remove_step_locked(project_root: cli.Path, step_id: str) -> None: + """Remove a step's registry entry and directory while the lock is held. - project_root = cli._require_specify_project() + The registry and step directory are resolved here, after the lock is + acquired, so a concurrent install cannot be lost or resurrected by a stale + registry snapshot. + """ + import shutil - step_helpers._validate_step_id_or_exit(step_id) + from .catalog import StepRegistry, StepValidationError registry = StepRegistry(project_root) in_registry = registry.is_installed(step_id) @@ -53,8 +53,6 @@ def workflow_step_remove( if dir_exists and not in_registry: # No registry write needed; just delete the orphaned directory. - import shutil - try: shutil.rmtree(step_dir) except OSError as exc: @@ -74,8 +72,6 @@ def workflow_step_remove( cli.console.print(f"[red]Error:[/red] {exc}") raise cli.typer.Exit(1) if dir_exists: - import shutil - try: shutil.rmtree(step_dir) except OSError as exc: @@ -94,4 +90,29 @@ def workflow_step_remove( f"[red]Error:[/red] Failed to remove step directory {step_dir}: {exc}" ) raise cli.typer.Exit(1) + + +@step_app.command("remove") +def workflow_step_remove( + step_id: str = cli.typer.Argument(..., help="Step type ID to uninstall"), +): + """Uninstall a custom step type.""" + from .installer import StepInstallError, _step_install_transaction + + project_root = cli._require_specify_project() + + step_helpers._validate_step_id_or_exit(step_id) + + # Removal mutates the same directory and registry as `step add`, so it + # shares the install lock to prevent lost or resurrected registry entries. + try: + with _step_install_transaction(project_root): + _remove_step_locked(project_root, step_id) + except StepInstallError as exc: + cli.console.print( + f"[red]Error:[/red] Failed to lock step removal '{step_id}': " + f"{cli._escape_markup(str(exc))}" + ) + raise cli.typer.Exit(1) + cli.console.print(f"[green]✓[/green] Step type '{step_id}' uninstalled") diff --git a/src/specify_cli/workflows/step/installer.py b/src/specify_cli/workflows/step/installer.py index a5c49a6f14..0ae077cd4f 100644 --- a/src/specify_cli/workflows/step/installer.py +++ b/src/specify_cli/workflows/step/installer.py @@ -80,62 +80,21 @@ class StepInstallError(Exception): @contextlib.contextmanager def _step_install_transaction(project_root: Path): - """Serialize step directory swaps with their registry updates.""" - from ...shared_infra import _ensure_safe_shared_directory + """Serialize step directory and registry mutations (install and remove).""" + from ...shared_infra import _exclusive_project_lock - lock_dir = Path(project_root) / ".specify" - try: - _ensure_safe_shared_directory( - Path(project_root), lock_dir, context="step install lock directory" - ) - except ValueError as exc: - raise StepInstallError(str(exc)) from exc - lock_file = lock_dir / ".step-install.lock" - if lock_file.is_symlink(): - raise StepInstallError(f"Refusing to use symlinked step install lock: {lock_file}") - - flags = os.O_RDWR | os.O_CREAT - flags |= getattr(os, "O_NOFOLLOW", 0) - flags |= getattr(os, "O_CLOEXEC", 0) - try: - fd = os.open(lock_file, flags, 0o600) - except OSError as exc: - raise StepInstallError(f"Failed to open step install lock: {exc}") from exc - try: - if lock_file.is_symlink(): - raise StepInstallError( - f"Refusing to use symlinked step install lock: {lock_file}" + # Only acquisition failures are reported as lock errors; exceptions raised + # by the caller's critical section propagate unchanged. + with contextlib.ExitStack() as stack: + try: + stack.enter_context( + _exclusive_project_lock( + Path(project_root), ".step-install.lock", context="step install" + ) ) - if os.name == "nt": - import errno - import msvcrt - import time - - if os.fstat(fd).st_size == 0: - os.write(fd, b"\0") - while True: - os.lseek(fd, 0, os.SEEK_SET) - try: - msvcrt.locking(fd, msvcrt.LK_NBLCK, 1) - break - except OSError as exc: - if exc.errno not in (errno.EACCES, errno.EDEADLK): - raise - time.sleep(0.05) - else: - import fcntl - - fcntl.flock(fd, fcntl.LOCK_EX) + except OSError as exc: + raise StepInstallError(f"Failed to lock step installation: {exc}") from exc yield - except StepInstallError: - raise - except OSError as exc: - raise StepInstallError(f"Failed to lock step installation: {exc}") from exc - finally: - try: - os.close(fd) - except OSError: - pass # --------------------------------------------------------------------------- diff --git a/tests/lock_helpers.py b/tests/lock_helpers.py new file mode 100644 index 0000000000..e93ab887d6 --- /dev/null +++ b/tests/lock_helpers.py @@ -0,0 +1,67 @@ +"""Test helpers for observing inter-process lock contention.""" + +from __future__ import annotations + +import os +import threading +import time + + +def watch_lock_attempt(monkeypatch, thread_name: str) -> threading.Event: + """Return an event set once ``thread_name`` tries to take a file lock. + + Patches the platform lock primitive (``fcntl.flock`` on POSIX, + ``msvcrt.locking`` on Windows) so a test can deterministically wait until + a thread is blocked on a lock held by another thread. + """ + attempted = threading.Event() + if os.name == "nt": + import msvcrt + + real_locking = msvcrt.locking + + def _locking(fd, operation, nbytes): + observed = ( + threading.current_thread().name == thread_name + and operation == msvcrt.LK_NBLCK + ) + try: + return real_locking(fd, operation, nbytes) + finally: + # Windows polls with non-blocking attempts; the first attempt + # (failed or not) confirms the thread reached the lock. + if observed: + attempted.set() + + monkeypatch.setattr(msvcrt, "locking", _locking) + else: + import fcntl + + real_flock = fcntl.flock + + def _flock(fd, operation): + if ( + threading.current_thread().name == thread_name + and operation == fcntl.LOCK_EX + ): + attempted.set() + return real_flock(fd, operation) + + monkeypatch.setattr(fcntl, "flock", _flock) + return attempted + + +def wait_until_blocked_or_done( + attempted: threading.Event, done: threading.Event, timeout: float = 10 +) -> None: + """Wait until a thread has reached the lock or has already finished. + + Returning on ``done`` lets a test assert on final state (and fail with a + meaningful message) when the code under test does not take the lock. + """ + deadline = time.monotonic() + timeout + while time.monotonic() < deadline: + if attempted.is_set() or done.is_set(): + return + time.sleep(0.01) + raise AssertionError("thread neither attempted the lock nor finished") diff --git a/tests/specify_cli/workflows/step/test_command_remove.py b/tests/specify_cli/workflows/step/test_command_remove.py index a6bd5781c0..4dbd63b300 100644 --- a/tests/specify_cli/workflows/step/test_command_remove.py +++ b/tests/specify_cli/workflows/step/test_command_remove.py @@ -2,10 +2,214 @@ from __future__ import annotations +import json import os +import shutil +import threading +from pathlib import Path import pytest +from tests.lock_helpers import wait_until_blocked_or_done, watch_lock_attempt + + +def _write_package(package_dir: Path, type_key: str) -> Path: + package_dir.mkdir(parents=True, exist_ok=True) + (package_dir / "step.yml").write_text( + f"step:\n type_key: {type_key}\n name: {type_key}\n version: 0.1.0\n", + encoding="utf-8", + ) + (package_dir / "__init__.py").write_text("# init\n", encoding="utf-8") + return package_dir + + +def _steps_dir(project_dir: Path) -> Path: + return project_dir / ".specify" / "workflows" / "steps" + + +def _registered_ids(project_dir: Path) -> set[str]: + path = _steps_dir(project_dir) / "step-registry.json" + return set(json.loads(path.read_text(encoding="utf-8"))["steps"]) + + +def _install(project_dir: Path, tmp_path: Path, step_id: str) -> None: + from specify_cli.workflows.step import installer + + pkg = _write_package(tmp_path / f"pkg-{step_id}", step_id) + installer.install_step_package(project_dir, step_id, pkg, source="local") + + +class _Worker(threading.Thread): + """Run ``target`` in a named thread and record its exception, if any.""" + + def __init__(self, name, target): + super().__init__(name=name, daemon=True) + self._target_fn = target + self.done = threading.Event() + self.error: BaseException | None = None + + def run(self): + try: + self._target_fn() + except BaseException as exc: # noqa: BLE001 - asserted by the test + self.error = exc + finally: + self.done.set() + + +class TestWorkflowStepRemoveLocking: + """`step remove` must share the install lock with `step add`.""" + + def test_remove_waiting_on_install_does_not_resurrect_entry( + self, project_dir, tmp_path, monkeypatch + ): + """Install of `new` holds the lock while `old` is removed. + + Without the lock, the installer's stale `{old}` snapshot is saved as + `{old, new}` after removal persisted `{}`, resurrecting `old` with no + directory. + """ + from specify_cli.workflows.step import installer + from specify_cli.workflows.step.command_remove import workflow_step_remove + + monkeypatch.chdir(project_dir) + _install(project_dir, tmp_path, "old-step") + new_pkg = _write_package(tmp_path / "pkg-new", "new-step") + + installer_inside = threading.Event() + release_installer = threading.Event() + real_replace = installer._replace_install + + def _paused_replace(*args, **kwargs): + if threading.current_thread().name == "installer": + installer_inside.set() + if not release_installer.wait(10): + raise AssertionError("installer was never released") + return real_replace(*args, **kwargs) + + monkeypatch.setattr(installer, "_replace_install", _paused_replace) + remover_attempted = watch_lock_attempt(monkeypatch, "remover") + + install_thread = _Worker( + "installer", + lambda: installer.install_step_package( + project_dir, "new-step", new_pkg, source="local" + ), + ) + remove_thread = _Worker("remover", lambda: workflow_step_remove("old-step")) + + install_thread.start() + assert installer_inside.wait(10), "installer never reached the lock" + remove_thread.start() + wait_until_blocked_or_done(remover_attempted, remove_thread.done) + release_installer.set() + install_thread.join(10) + remove_thread.join(10) + + assert install_thread.error is None + assert remove_thread.error is None + assert _registered_ids(project_dir) == {"new-step"} + assert not (_steps_dir(project_dir) / "old-step").exists() + assert (_steps_dir(project_dir) / "new-step").is_dir() + + def test_install_waiting_on_remove_is_not_unregistered( + self, project_dir, tmp_path, monkeypatch + ): + """Removal of `old` holds the lock while `new` is installed. + + Without the lock, removal's stale `{old}` snapshot is saved as `{}` + after the installer persisted `{old, new}`, leaving `new` on disk but + unregistered. + """ + from specify_cli.workflows.step import installer + from specify_cli.workflows.step.catalog import StepRegistry + from specify_cli.workflows.step.command_remove import workflow_step_remove + + monkeypatch.chdir(project_dir) + _install(project_dir, tmp_path, "old-step") + new_pkg = _write_package(tmp_path / "pkg-new", "new-step") + + remover_inside = threading.Event() + release_remover = threading.Event() + real_remove = StepRegistry.remove + + def _paused_remove(self, step_id): + if threading.current_thread().name == "remover": + remover_inside.set() + if not release_remover.wait(10): + raise AssertionError("remover was never released") + return real_remove(self, step_id) + + monkeypatch.setattr(StepRegistry, "remove", _paused_remove) + installer_attempted = watch_lock_attempt(monkeypatch, "installer") + + remove_thread = _Worker("remover", lambda: workflow_step_remove("old-step")) + install_thread = _Worker( + "installer", + lambda: installer.install_step_package( + project_dir, "new-step", new_pkg, source="local" + ), + ) + + remove_thread.start() + assert remover_inside.wait(10), "remover never reached the registry update" + install_thread.start() + wait_until_blocked_or_done(installer_attempted, install_thread.done) + release_remover.set() + remove_thread.join(10) + install_thread.join(10) + + assert remove_thread.error is None + assert install_thread.error is None + assert _registered_ids(project_dir) == {"new-step"} + assert not (_steps_dir(project_dir) / "old-step").exists() + assert (_steps_dir(project_dir) / "new-step").is_dir() + + def test_remove_fails_cleanly_when_lock_cannot_be_acquired( + self, project_dir, tmp_path, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + monkeypatch.chdir(project_dir) + _install(project_dir, tmp_path, "my-step") + # A directory at the lock path cannot be opened as the lock file. + lock_path = project_dir / ".specify" / ".step-install.lock" + if lock_path.exists(): + lock_path.unlink() + lock_path.mkdir() + + result = CliRunner().invoke(app, ["workflow", "step", "remove", "my-step"]) + + assert result.exit_code == 1, result.output + assert "Failed to lock step removal" in result.output + assert _registered_ids(project_dir) == {"my-step"} + assert (_steps_dir(project_dir) / "my-step").is_dir() + + def test_remove_restores_registry_entry_when_directory_delete_fails( + self, project_dir, tmp_path, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + monkeypatch.chdir(project_dir) + _install(project_dir, tmp_path, "my-step") + step_dir = (_steps_dir(project_dir) / "my-step").resolve() + real_rmtree = shutil.rmtree + + def _failing_rmtree(path, *args, **kwargs): + if Path(path).resolve() == step_dir: + raise OSError("simulated delete failure") + return real_rmtree(path, *args, **kwargs) + + monkeypatch.setattr(shutil, "rmtree", _failing_rmtree) + + result = CliRunner().invoke(app, ["workflow", "step", "remove", "my-step"]) + + assert result.exit_code == 1, result.output + assert "Failed to remove step directory" in result.output + assert _registered_ids(project_dir) == {"my-step"} + assert step_dir.is_dir() class TestWorkflowStepRemoveCLI: diff --git a/tests/specify_cli/workflows/step/test_installer.py b/tests/specify_cli/workflows/step/test_installer.py index 6545a74f83..9344052d71 100644 --- a/tests/specify_cli/workflows/step/test_installer.py +++ b/tests/specify_cli/workflows/step/test_installer.py @@ -9,6 +9,7 @@ import pytest from specify_cli.workflows.step import installer +from tests.lock_helpers import watch_lock_attempt def _write_package( @@ -904,7 +905,7 @@ def _install_race_setup(tmp_path, project_dir, monkeypatch, *, force_b): class Race: a_inside = threading.Event() release_a = threading.Event() - b_attempted_lock = threading.Event() + b_attempted_lock: threading.Event b_inside_replace = threading.Event() outcomes: dict[str, Exception | None] = {} thread_a = None @@ -925,40 +926,7 @@ def _replace(step_dir, staged_dir, registry, step_id, entry, *, force): ) monkeypatch.setattr(installer, "_replace_install", _replace) - if os.name == "nt": - import msvcrt - - real_locking = msvcrt.locking - - def _locking(fd, operation, nbytes): - is_installer_b = threading.current_thread().name == "installer-b" - try: - result = real_locking(fd, operation, nbytes) - except OSError: - if is_installer_b and operation == msvcrt.LK_NBLCK: - # The non-blocking Windows lock attempt has now confirmed - # contention with installer A. - race.b_attempted_lock.set() - raise - if is_installer_b and operation == msvcrt.LK_NBLCK: - race.b_attempted_lock.set() - return result - - monkeypatch.setattr(msvcrt, "locking", _locking) - else: - import fcntl - - real_flock = fcntl.flock - - def _flock(fd, operation): - if ( - threading.current_thread().name == "installer-b" - and operation == fcntl.LOCK_EX - ): - race.b_attempted_lock.set() - return real_flock(fd, operation) - - monkeypatch.setattr(fcntl, "flock", _flock) + race.b_attempted_lock = watch_lock_attempt(monkeypatch, "installer-b") def _install(label, pkg, force): try: diff --git a/tests/test_shared_infra_lock.py b/tests/test_shared_infra_lock.py new file mode 100644 index 0000000000..9e31f8d055 --- /dev/null +++ b/tests/test_shared_infra_lock.py @@ -0,0 +1,118 @@ +"""Tests for the shared inter-process project lock.""" + +from __future__ import annotations + +import os +import threading + +import pytest + +from specify_cli.shared_infra import _exclusive_project_lock +from tests.lock_helpers import watch_lock_attempt + +LOCK_NAME = ".test-resource.lock" + + +def _symlink_or_skip(link, target, *, target_is_directory=False): + try: + link.symlink_to(target, target_is_directory=target_is_directory) + except (OSError, NotImplementedError) as exc: + pytest.skip(f"symlinks are unavailable: {exc}") + + +def test_lock_creates_lock_file_under_specify(tmp_path): + with _exclusive_project_lock(tmp_path, LOCK_NAME, context="test"): + assert (tmp_path / ".specify" / LOCK_NAME).is_file() + + +def test_lock_blocks_second_holder_until_released(tmp_path, monkeypatch): + first_inside = threading.Event() + release_first = threading.Event() + second_inside = threading.Event() + second_attempted = watch_lock_attempt(monkeypatch, "lock-second") + errors: list[BaseException] = [] + + def _hold(inside, release=None): + try: + with _exclusive_project_lock(tmp_path, LOCK_NAME, context="test"): + inside.set() + if release is not None and not release.wait(10): + raise AssertionError("first holder was never released") + except BaseException as exc: # noqa: BLE001 - surfaced below + errors.append(exc) + + first = threading.Thread( + target=_hold, args=(first_inside, release_first), name="lock-first", daemon=True + ) + second = threading.Thread( + target=_hold, args=(second_inside,), name="lock-second", daemon=True + ) + first.start() + assert first_inside.wait(10) + second.start() + assert second_attempted.wait(10), "second holder never attempted the lock" + assert not second_inside.is_set() + + release_first.set() + first.join(10) + second.join(10) + + assert not errors + assert second_inside.is_set() + + +def test_lock_is_released_when_body_raises(tmp_path): + with pytest.raises(RuntimeError): + with _exclusive_project_lock(tmp_path, LOCK_NAME, context="test"): + raise RuntimeError("boom") + + reacquired = threading.Event() + + def _reacquire(): + with _exclusive_project_lock(tmp_path, LOCK_NAME, context="test"): + reacquired.set() + + thread = threading.Thread(target=_reacquire, daemon=True) + thread.start() + thread.join(10) + assert reacquired.is_set() + + +def test_lock_rejects_symlinked_lock_file(tmp_path): + (tmp_path / ".specify").mkdir() + target = tmp_path / "outside.lock" + target.write_text("", encoding="utf-8") + _symlink_or_skip(tmp_path / ".specify" / LOCK_NAME, target) + + with pytest.raises(OSError, match="Refusing to use symlinked test lock"): + with _exclusive_project_lock(tmp_path, LOCK_NAME, context="test"): + pass + + +def test_lock_rejects_symlinked_specify_directory(tmp_path): + project = tmp_path / "project" + project.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + _symlink_or_skip(project / ".specify", outside, target_is_directory=True) + + with pytest.raises(OSError, match="symlinked test lock directory"): + with _exclusive_project_lock(project, LOCK_NAME, context="test"): + pass + assert not (outside / LOCK_NAME).exists() + + +def test_lock_reports_unopenable_lock_file_as_oserror(tmp_path): + (tmp_path / ".specify" / LOCK_NAME).mkdir(parents=True) + + with pytest.raises(OSError): + with _exclusive_project_lock(tmp_path, LOCK_NAME, context="test"): + pass + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX file mode semantics") +def test_lock_file_is_private(tmp_path): + with _exclusive_project_lock(tmp_path, LOCK_NAME, context="test"): + pass + mode = (tmp_path / ".specify" / LOCK_NAME).stat().st_mode & 0o777 + assert mode & 0o077 == 0 From 796f8e2f40f7985e4075f2043514392e1b238ac7 Mon Sep 17 00:00:00 2001 From: Markus Date: Wed, 30 Sep 2026 07:19:16 +0200 Subject: [PATCH 10/16] fix(workflows): bound step package traversal by the entry ceiling The 512-entry ceiling did not bound validation work: each directory listing was fully materialized and sorted before any entry was counted, and excluded subtrees (`.git`, `__pycache__`) were walked in full without consuming the budget, so a `--dev` checkout with a large `.git` cost unbounded time and memory. - Stream `os.scandir` through a shared helper that drops excluded names unread and raises the entry-limit error once the remaining budget is exceeded, before sorting. Validation and staging copy both use it. - Prune excluded directories instead of descending into them, matching `bundles/packager.py`. Their contents were already never staged, so a symlink or deep nesting inside `.git` no longer fails validation. - Update the workflow reference to describe the exclusion behavior. - Replace the symlink-inside-`.git` rejection test with pruning coverage and add read-count regressions for validation, a cross-directory budget, and the staging copy. Assisted-by: OpenCode (model: Claude Opus 5.5, autonomous) --- docs/reference/workflows.md | 10 +- src/specify_cli/workflows/step/installer.py | 136 ++++++++++-------- .../workflows/step/test_installer.py | 118 ++++++++++++++- 3 files changed, 198 insertions(+), 66 deletions(-) diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index 28d0c3492b..50c022cc2a 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -650,13 +650,13 @@ Every source is validated identically before anything is committed: - The package tree is copied recursively (relative imports, nested helper modules, and data files are supported). A symlinked package root, any descendant symlink, and any filesystem object that is not a regular file or - directory are rejected — including inside excluded directories. -- `.git`, `__pycache__`, and `.DS_Store` entries are excluded from the copy and - from the limits. + directory are rejected. +- `.git`, `__pycache__`, and `.DS_Store` entries are skipped without being + inspected: they are not copied, excluded directories are not entered, and + they do not count toward any limit. - The installed-package policy permits at most **512 retained entries** (files and directories combined), at most **32 levels** of directory nesting, and - **50 MiB** of retained content. Excluded entries do not consume the - entry-count or byte budgets, but their directory depth is still validated. + **50 MiB** of retained content. - Archive URLs also pass transport/extraction safety limits before package validation: at most 512 archive entries, 50 MiB downloaded or extracted, and 10 MiB per archive member. Catalog files have a 50 MiB per-response bound. diff --git a/src/specify_cli/workflows/step/installer.py b/src/specify_cli/workflows/step/installer.py index 4c14189f5b..0a0bf18fa3 100644 --- a/src/specify_cli/workflows/step/installer.py +++ b/src/specify_cli/workflows/step/installer.py @@ -31,8 +31,9 @@ _MAX_STEP_PACKAGE_DEPTH = 32 _COPY_CHUNK_BYTES = 64 * 1024 -# Files/dirs never copied into (or counted as part of) an installed step -# package. Mirrors ``bundles/packager.py`` ``EXCLUDE_NAMES``. +# Files/dirs never inspected, copied into, or counted as part of an installed +# step package; excluded directories are pruned without being entered. Mirrors +# ``bundles/packager.py`` ``EXCLUDE_NAMES``. EXCLUDE_NAMES: frozenset[str] = frozenset({".git", "__pycache__", ".DS_Store"}) # Prefix for the private same-filesystem working directory created beneath the @@ -190,29 +191,64 @@ def _reject_unsafe_destination(step_dir: Path) -> None: # --------------------------------------------------------------------------- +def _entry_limit_error() -> StepInstallError: + return StepInstallError( + "Step package contains too many entries, exceeding the " + f"{_MAX_STEP_PACKAGE_FILES}-file limit" + ) + + +def _scan_retained_entries(directory: Path, budget: int) -> list[os.DirEntry]: + """Return *directory*'s retained children, sorted by name. + + Streams the listing and drops ``EXCLUDE_NAMES`` without inspecting them. + Raises the entry-limit error as soon as more than *budget* retained + entries are seen, so an oversized directory is never fully materialized + or sorted. ``OSError`` propagates to the caller. + """ + entries: list[os.DirEntry] = [] + with os.scandir(directory) as iterator: + for entry in iterator: + if entry.name in EXCLUDE_NAMES: + continue + if len(entries) >= budget: + raise _entry_limit_error() + entries.append(entry) + entries.sort(key=lambda entry: entry.name) + return entries + + def _walk_package_tree(package_dir: Path): - """Yield ``(path, is_dir, excluded)`` for every descendant of *package_dir*. - - Descends into excluded directories so a symlink or special file hiding - inside ``.git``/``__pycache__`` is still rejected, but never follows a - symlink. Raises :class:`StepInstallError` on any symlink, object that is - neither a regular file nor a directory, or nesting deeper than - ``_MAX_STEP_PACKAGE_DEPTH``. Iterative so deep trees cannot exhaust the - Python recursion limit. + """Yield ``(path, is_dir)`` for every retained descendant of *package_dir*. + + Excluded names are pruned without being entered or inspected, matching + ``bundles/packager.py``; their contents are never staged, so they are not + part of the package. Every retained entry discovered counts against + ``_MAX_STEP_PACKAGE_FILES`` before its directory listing is sorted, which + bounds traversal work by the entry ceiling. Never follows a symlink. + Raises :class:`StepInstallError` on any symlink, object that is neither a + regular file nor a directory, nesting deeper than + ``_MAX_STEP_PACKAGE_DEPTH``, or too many entries. Iterative so deep trees + cannot exhaust the Python recursion limit. """ + discovered = 0 - def _children(current: Path, depth: int, excluded_prefix: bool): + def _children(current: Path, depth: int): + nonlocal discovered try: - entries = sorted(os.scandir(current), key=lambda entry: entry.name) + entries = _scan_retained_entries( + current, _MAX_STEP_PACKAGE_FILES - discovered + ) except OSError as exc: raise StepInstallError( f"Failed to read step package directory '{current}': {exc}" ) from exc - return [(entry, depth, excluded_prefix) for entry in reversed(entries)] + discovered += len(entries) + return [(entry, depth) for entry in reversed(entries)] - stack = _children(package_dir, 1, False) + stack = _children(package_dir, 1) while stack: - entry, depth, excluded_prefix = stack.pop() + entry, depth = stack.pop() try: mode = entry.stat(follow_symlinks=False).st_mode except OSError as exc: @@ -222,17 +258,16 @@ def _children(current: Path, depth: int, excluded_prefix: bool): path = Path(entry.path) if stat.S_ISLNK(mode): raise StepInstallError(f"Step package contains symlink: {path}") - excluded = excluded_prefix or entry.name in EXCLUDE_NAMES if stat.S_ISDIR(mode): if depth > _MAX_STEP_PACKAGE_DEPTH: raise StepInstallError( f"Step package exceeds the {_MAX_STEP_PACKAGE_DEPTH}-level " "directory depth limit" ) - yield path, True, excluded - stack.extend(_children(path, depth + 1, excluded)) + yield path, True + stack.extend(_children(path, depth + 1)) elif stat.S_ISREG(mode): - yield path, False, excluded + yield path, False else: raise StepInstallError( f"Step package contains unsupported file: {path}" @@ -306,36 +341,22 @@ def validate_step_package(package_dir: Path, step_id: str) -> dict[str, Any]: f"Step package is missing required file '{required}' at its root" ) - retained_files = 0 - retained_dirs = 0 + # The walk enforces the entry ceiling itself; only bytes are summed here. retained_bytes = 0 - for path, is_dir, excluded in _walk_package_tree(package_dir): - if excluded: - continue + for path, is_dir in _walk_package_tree(package_dir): if is_dir: - retained_dirs += 1 - else: - retained_files += 1 - try: - retained_bytes += path.lstat().st_size - except OSError as exc: - raise StepInstallError( - f"Failed to inspect step package file '{path}': {exc}" - ) from exc - if retained_files + retained_dirs > _MAX_STEP_PACKAGE_FILES: - break - - if retained_files + retained_dirs > _MAX_STEP_PACKAGE_FILES: - raise StepInstallError( - f"Step package contains too many entries ({retained_files} files, " - f"{retained_dirs} directories), exceeding the " - f"{_MAX_STEP_PACKAGE_FILES}-file limit" - ) - if retained_bytes > _MAX_STEP_PACKAGE_BYTES: - raise StepInstallError( - f"Step package exceeds the {_MAX_STEP_PACKAGE_BYTES}-byte total " - "size limit" - ) + continue + try: + retained_bytes += path.lstat().st_size + except OSError as exc: + raise StepInstallError( + f"Failed to inspect step package file '{path}': {exc}" + ) from exc + if retained_bytes > _MAX_STEP_PACKAGE_BYTES: + raise StepInstallError( + f"Step package exceeds the {_MAX_STEP_PACKAGE_BYTES}-byte total " + "size limit" + ) try: step_yml_text = (package_dir / "step.yml").read_text(encoding="utf-8") @@ -533,12 +554,14 @@ def _copy_package_tree(source_dir: Path, target_dir: Path) -> None: Refuses to follow a symlink encountered mid-copy so a source swapped after validation cannot smuggle external content into the staged package, and - re-enforces the depth, entry, and byte budgets. Iterative so a deep tree - cannot exhaust the Python recursion limit. + re-enforces the depth, entry, and byte budgets. Entries count against the + entry budget as each listing is streamed, before it is sorted, so an + oversized source directory is rejected without being fully read. Iterative + so a deep tree cannot exhaust the Python recursion limit. """ remaining_bytes = _MAX_STEP_PACKAGE_BYTES - copied_entries = 0 + discovered_entries = 0 pending = [(source_dir, target_dir, 0)] while pending: current, destination, depth = pending.pop() @@ -554,14 +577,15 @@ def _copy_package_tree(source_dir: Path, target_dir: Path) -> None: f"Failed to stage step package: {exc}" ) from exc try: - entries = sorted(os.scandir(current), key=lambda entry: entry.name) + entries = _scan_retained_entries( + current, _MAX_STEP_PACKAGE_FILES - discovered_entries + ) except OSError as exc: raise StepInstallError( f"Failed to stage step package: {exc}" ) from exc + discovered_entries += len(entries) for entry in entries: - if entry.name in EXCLUDE_NAMES: - continue try: mode = entry.stat(follow_symlinks=False).st_mode except OSError as exc: @@ -575,12 +599,6 @@ def _copy_package_tree(source_dir: Path, target_dir: Path) -> None: raise StepInstallError( f"Step package contains unsupported file: {entry.path}" ) - copied_entries += 1 - if copied_entries > _MAX_STEP_PACKAGE_FILES: - raise StepInstallError( - "Step package contains too many entries, exceeding the " - f"{_MAX_STEP_PACKAGE_FILES}-file limit" - ) if stat.S_ISDIR(mode): pending.append((Path(entry.path), target, depth + 1)) else: diff --git a/tests/specify_cli/workflows/step/test_installer.py b/tests/specify_cli/workflows/step/test_installer.py index 9344052d71..a4885520cf 100644 --- a/tests/specify_cli/workflows/step/test_installer.py +++ b/tests/specify_cli/workflows/step/test_installer.py @@ -196,15 +196,129 @@ def test_validate_package_rejects_descendant_symlink(tmp_path): installer.validate_step_package(pkg, "my-step") +class _ScandirSpy: + """Delegating ``os.scandir`` wrapper that records reads beneath *root*.""" + + def __init__(self, root: Path, real=os.scandir): + self.root = root + self.real = real + self.scanned: list[Path] = [] + self.entries_read: dict[Path, int] = {} + + def __call__(self, path=".", *args, **kwargs): + iterator = self.real(path, *args, **kwargs) + if not isinstance(path, (str, os.PathLike)): + return iterator + directory = Path(path) + if directory != self.root and self.root not in directory.parents: + return iterator + self.scanned.append(directory) + self.entries_read[directory] = 0 + spy = self + + class _Counting: + def __iter__(self): + for entry in iterator: + spy.entries_read[directory] += 1 + yield entry + + def __enter__(self): + return self + + def __exit__(self, *exc_info): + iterator.close() + + def close(self): + iterator.close() + + return _Counting() + + @pytest.mark.skipif(not hasattr(os, "symlink"), reason="symlinks are unavailable") -def test_validate_package_rejects_symlink_in_excluded_dir(tmp_path): +def test_excluded_dirs_are_pruned_not_inspected(tmp_path, project_dir, monkeypatch): + """Excluded subtrees are never entered, so their contents cannot fail + validation or reach the installed package.""" pkg = _write_package(tmp_path / "pkg") git_dir = pkg / ".git" git_dir.mkdir() (git_dir / "hook").symlink_to(pkg / "step.yml") - with pytest.raises(installer.StepInstallError, match="symlink"): + deep = git_dir + for _ in range(installer._MAX_STEP_PACKAGE_DEPTH + 2): + deep = deep / "objects" + deep.mkdir(parents=True) + outside = tmp_path / "outside" + outside.mkdir() + (pkg / "__pycache__").symlink_to(outside, target_is_directory=True) + + spy = _ScandirSpy(pkg) + monkeypatch.setattr(os, "scandir", spy) + installer.install_step_package(project_dir, "my-step", pkg, source="local") + + assert not any( + path == git_dir or git_dir in path.parents for path in spy.scanned + ) + step_dir = _steps_dir(project_dir) / "my-step" + assert (step_dir / "step.yml").is_file() + assert not (step_dir / ".git").exists() + assert not (step_dir / "__pycache__").exists() + + +@pytest.mark.skipif(not hasattr(os, "symlink"), reason="symlinks are unavailable") +def test_symlink_named_like_excluded_file_is_still_not_copied(tmp_path, project_dir): + pkg = _write_package(tmp_path / "pkg") + (pkg / ".DS_Store").symlink_to(pkg / "step.yml") + installer.install_step_package(project_dir, "my-step", pkg, source="local") + assert not (_steps_dir(project_dir) / "my-step" / ".DS_Store").exists() + + +def test_validation_stops_reading_oversized_directory(tmp_path, monkeypatch): + """The entry ceiling bounds how many directory entries are read.""" + pkg = _write_package(tmp_path / "pkg") + for index in range(50): + (pkg / f"extra-{index:02}.py").write_text("x", encoding="utf-8") + monkeypatch.setattr(installer, "_MAX_STEP_PACKAGE_FILES", 3) + + spy = _ScandirSpy(pkg) + monkeypatch.setattr(os, "scandir", spy) + with pytest.raises(installer.StepInstallError, match="3-file limit"): installer.validate_step_package(pkg, "my-step") + assert spy.entries_read[pkg] <= 4 + + +def test_validation_budget_spans_directories(tmp_path, monkeypatch): + """Entries already discovered elsewhere shrink the budget for later + directories, so many moderately sized directories cannot evade it.""" + pkg = _write_package(tmp_path / "pkg") + for name in ("a", "b", "c"): + sub = pkg / name + sub.mkdir() + for index in range(20): + (sub / f"f{index:02}.py").write_text("x", encoding="utf-8") + monkeypatch.setattr(installer, "_MAX_STEP_PACKAGE_FILES", 10) + + spy = _ScandirSpy(pkg) + monkeypatch.setattr(os, "scandir", spy) + with pytest.raises(installer.StepInstallError, match="10-file limit"): + installer.validate_step_package(pkg, "my-step") + + assert sum(spy.entries_read.values()) <= 11 + + +def test_copy_stops_reading_oversized_directory(tmp_path, monkeypatch): + pkg = _write_package(tmp_path / "pkg") + for index in range(50): + (pkg / f"extra-{index:02}.py").write_text("x", encoding="utf-8") + monkeypatch.setattr(installer, "_MAX_STEP_PACKAGE_FILES", 3) + + spy = _ScandirSpy(pkg) + monkeypatch.setattr(os, "scandir", spy) + with pytest.raises(installer.StepInstallError, match="3-file limit"): + installer._copy_package_tree(pkg, tmp_path / "staged") + + assert spy.entries_read[pkg] <= 4 + assert not any((tmp_path / "staged").iterdir()) + @pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="mkfifo is unavailable") def test_validate_package_rejects_special_file(tmp_path): From 54d8ea3f6bd1b2e7af43bd0e733820a2b7015ab7 Mon Sep 17 00:00:00 2001 From: Markus Date: Wed, 30 Sep 2026 07:34:08 +0200 Subject: [PATCH 11/16] fix(workflows): escape step IDs in step remove and add messages Step IDs may legally contain `[`/`]`, so interpolating them unescaped into Rich markup rendered an ID such as `[red]step` as `step`. `step remove` printed the ID, step directory, and exception text raw on every path, and the discovery-only warning in `step add` did the same. - Escape the step ID, directory path, and exception text in all `step remove` output, matching `step info`. - Escape the step ID in the `step add` discovery-only catalog warning. - Add CLI regressions for the not-installed, lock-failure, orphan-removal, directory-delete-failure, and discovery-only paths. Assisted-by: OpenCode (model: Claude Opus 5.5, autonomous) --- src/specify_cli/workflows/step/command_add.py | 3 +- .../workflows/step/command_remove.py | 28 ++++-- .../workflows/step/test_command_add.py | 21 +++++ .../workflows/step/test_command_remove.py | 92 +++++++++++++++++++ 4 files changed, 133 insertions(+), 11 deletions(-) diff --git a/src/specify_cli/workflows/step/command_add.py b/src/specify_cli/workflows/step/command_add.py index 7919d3aad1..fec112b5cd 100644 --- a/src/specify_cli/workflows/step/command_add.py +++ b/src/specify_cli/workflows/step/command_add.py @@ -281,7 +281,8 @@ def _install_from_catalog(project_root: cli.Path, step_id: str, *, force: bool) if not info.get("_install_allowed", True): cli.console.print( - f"[yellow]Warning:[/yellow] Step type '{step_id}' is from a " + "[yellow]Warning:[/yellow] Step type " + f"'{cli._escape_markup(step_id)}' is from a " "discovery-only catalog" ) cli.console.print("Direct installation is not enabled for this catalog source.") diff --git a/src/specify_cli/workflows/step/command_remove.py b/src/specify_cli/workflows/step/command_remove.py index 78bb9a983d..f8dcd1b024 100644 --- a/src/specify_cli/workflows/step/command_remove.py +++ b/src/specify_cli/workflows/step/command_remove.py @@ -19,6 +19,7 @@ def _remove_step_locked(project_root: cli.Path, step_id: str) -> None: from .catalog import StepRegistry, StepValidationError + safe_step_id = cli._escape_markup(step_id) registry = StepRegistry(project_root) in_registry = registry.is_installed(step_id) @@ -30,16 +31,16 @@ def _remove_step_locked(project_root: cli.Path, step_id: str) -> None: try: rel_parts = step_dir.relative_to(steps_base_dir).parts except ValueError: - cli.console.print(f"[red]Error:[/red] Invalid step id '{step_id}'") + cli.console.print(f"[red]Error:[/red] Invalid step id '{safe_step_id}'") raise cli.typer.Exit(1) if rel_parts != (step_id,): - cli.console.print(f"[red]Error:[/red] Invalid step id '{step_id}'") + cli.console.print(f"[red]Error:[/red] Invalid step id '{safe_step_id}'") raise cli.typer.Exit(1) dir_exists = step_dir.exists() if not in_registry and not dir_exists: - cli.console.print(f"[red]Error:[/red] Step type '{step_id}' is not installed") + cli.console.print(f"[red]Error:[/red] Step type '{safe_step_id}' is not installed") raise cli.typer.Exit(1) if not in_registry and dir_exists: @@ -47,7 +48,7 @@ def _remove_step_locked(project_root: cli.Path, step_id: str) -> None: # directory is being removed even though there is no registry entry, so # the orphaned package can be cleaned up and a fresh install attempted. cli.console.print( - f"[yellow]Warning:[/yellow] '{step_id}' has no registry entry " + f"[yellow]Warning:[/yellow] '{safe_step_id}' has no registry entry " "(registry may have been reset). Removing the orphaned directory." ) @@ -57,7 +58,8 @@ def _remove_step_locked(project_root: cli.Path, step_id: str) -> None: shutil.rmtree(step_dir) except OSError as exc: cli.console.print( - f"[red]Error:[/red] Failed to remove step directory {step_dir}: {exc}" + "[red]Error:[/red] Failed to remove step directory " + f"{cli._escape_markup(str(step_dir))}: {cli._escape_markup(str(exc))}" ) raise cli.typer.Exit(1) elif in_registry: @@ -69,7 +71,7 @@ def _remove_step_locked(project_root: cli.Path, step_id: str) -> None: try: registry.remove(step_id) except StepValidationError as exc: - cli.console.print(f"[red]Error:[/red] {exc}") + cli.console.print(f"[red]Error:[/red] {cli._escape_markup(str(exc))}") raise cli.typer.Exit(1) if dir_exists: try: @@ -84,10 +86,13 @@ def _remove_step_locked(project_root: cli.Path, step_id: str) -> None: except Exception as restore_exc: # noqa: BLE001 cli.console.print( f"[yellow]Warning:[/yellow] Failed to restore registry entry " - f"for '{step_id}' after directory removal failure: {restore_exc}" + f"for '{safe_step_id}' after directory removal failure: " + f"{cli._escape_markup(str(restore_exc))}" ) cli.console.print( - f"[red]Error:[/red] Failed to remove step directory {step_dir}: {exc}" + "[red]Error:[/red] Failed to remove step directory " + f"{cli._escape_markup(str(step_dir))}: " + f"{cli._escape_markup(str(exc))}" ) raise cli.typer.Exit(1) @@ -110,9 +115,12 @@ def workflow_step_remove( _remove_step_locked(project_root, step_id) except StepInstallError as exc: cli.console.print( - f"[red]Error:[/red] Failed to lock step removal '{step_id}': " + "[red]Error:[/red] Failed to lock step removal " + f"'{cli._escape_markup(step_id)}': " f"{cli._escape_markup(str(exc))}" ) raise cli.typer.Exit(1) - cli.console.print(f"[green]✓[/green] Step type '{step_id}' uninstalled") + cli.console.print( + f"[green]✓[/green] Step type '{cli._escape_markup(step_id)}' uninstalled" + ) diff --git a/tests/specify_cli/workflows/step/test_command_add.py b/tests/specify_cli/workflows/step/test_command_add.py index 143774324d..0a20da6b22 100644 --- a/tests/specify_cli/workflows/step/test_command_add.py +++ b/tests/specify_cli/workflows/step/test_command_add.py @@ -38,6 +38,27 @@ def _fake_get_step_info(self, step_id): assert result.exit_code != 0 assert "Refusing to use symlinked step directory" in result.output + def test_add_discovery_only_warning_prints_step_id_literally( + self, project_dir, monkeypatch + ): + """A valid step ID containing Rich markup brackets is shown verbatim.""" + from typer.testing import CliRunner + + from specify_cli import app + from specify_cli.workflows.step.catalog import StepCatalog + + monkeypatch.chdir(project_dir) + monkeypatch.setattr( + StepCatalog, + "get_step_info", + lambda self, step_id: {"id": step_id, "_install_allowed": False}, + ) + + result = CliRunner().invoke(app, ["workflow", "step", "add", "[red]step"]) + + assert result.exit_code == 1, result.output + assert "Step type '[red]step' is from a" in result.output + def test_add_rejects_oversized_step_response(self, project_dir, monkeypatch): from typer.testing import CliRunner diff --git a/tests/specify_cli/workflows/step/test_command_remove.py b/tests/specify_cli/workflows/step/test_command_remove.py index 4dbd63b300..0183470584 100644 --- a/tests/specify_cli/workflows/step/test_command_remove.py +++ b/tests/specify_cli/workflows/step/test_command_remove.py @@ -296,3 +296,95 @@ def test_remove_rejects_symlinked_steps_base_dir(self, project_dir, monkeypatch) assert result.exit_code != 0 assert "Refusing to use symlinked step directory" in result.output + + +# A valid step ID (brackets are allowed) that Rich would otherwise parse as a +# style tag and drop from the output. +_MARKUP_STEP_ID = "[red]step" + + +def _flat(output: str) -> str: + """Undo Rich line wrapping so long messages can be matched.""" + return output.replace("\n", "") + + +class TestWorkflowStepRemoveMarkupEscaping: + """User-controlled values are printed literally, not parsed as markup.""" + + def test_not_installed_error(self, project_dir, monkeypatch): + from typer.testing import CliRunner + from specify_cli import app + + monkeypatch.chdir(project_dir) + result = CliRunner().invoke( + app, ["workflow", "step", "remove", _MARKUP_STEP_ID] + ) + + assert result.exit_code == 1, result.output + assert f"Step type '{_MARKUP_STEP_ID}' is not installed" in result.output + + def test_lock_failure_error(self, project_dir, monkeypatch): + from typer.testing import CliRunner + from specify_cli import app + + monkeypatch.chdir(project_dir) + lock_path = project_dir / ".specify" / ".step-install.lock" + if lock_path.exists(): + lock_path.unlink() + lock_path.mkdir() + + result = CliRunner().invoke( + app, ["workflow", "step", "remove", _MARKUP_STEP_ID] + ) + + assert result.exit_code == 1, result.output + assert f"Failed to lock step removal '{_MARKUP_STEP_ID}'" in _flat( + result.output + ) + + def test_orphan_warning_and_success_message(self, project_dir, monkeypatch): + from typer.testing import CliRunner + from specify_cli import app + + monkeypatch.chdir(project_dir) + step_dir = _steps_dir(project_dir) / _MARKUP_STEP_ID + step_dir.mkdir(parents=True) + + result = CliRunner().invoke( + app, ["workflow", "step", "remove", _MARKUP_STEP_ID] + ) + + assert result.exit_code == 0, result.output + output = _flat(result.output) + assert f"'{_MARKUP_STEP_ID}' has no registry entry" in output + assert f"Step type '{_MARKUP_STEP_ID}' uninstalled" in output + assert not step_dir.exists() + + def test_directory_delete_failure_error(self, project_dir, monkeypatch): + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.workflows.step.catalog import StepRegistry + + monkeypatch.chdir(project_dir) + StepRegistry(project_dir).add( + _MARKUP_STEP_ID, {"name": "x", "type_key": _MARKUP_STEP_ID} + ) + step_dir = _steps_dir(project_dir) / _MARKUP_STEP_ID + step_dir.mkdir(parents=True) + real_rmtree = shutil.rmtree + + def _failing_rmtree(path, *args, **kwargs): + if Path(path).name == _MARKUP_STEP_ID: + raise OSError("simulated [bold]delete[/bold] failure") + return real_rmtree(path, *args, **kwargs) + + monkeypatch.setattr(shutil, "rmtree", _failing_rmtree) + + result = CliRunner().invoke( + app, ["workflow", "step", "remove", _MARKUP_STEP_ID] + ) + + assert result.exit_code == 1, result.output + output = _flat(result.output) + assert f"{_MARKUP_STEP_ID}: simulated [bold]delete[/bold] failure" in output + assert step_dir.is_dir() From f2a02847006c24009501011d20195d03222f0759 Mon Sep 17 00:00:00 2001 From: Markus Date: Wed, 30 Sep 2026 08:30:57 +0200 Subject: [PATCH 12/16] fix(workflows): call the step package budget an entry limit The package budget counts files and directories together, but the validation, staging, and catalog preflight errors reported it as a "file limit". Report it as an "N-entry limit (files and directories combined)" and update the matching test assertions. Assisted-by: OpenCode (model: Claude Opus 5.5, autonomous) --- src/specify_cli/workflows/step/command_add.py | 3 ++- src/specify_cli/workflows/step/installer.py | 2 +- .../specify_cli/workflows/step/test_command_add.py | 2 +- tests/specify_cli/workflows/step/test_installer.py | 14 +++++++------- 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/specify_cli/workflows/step/command_add.py b/src/specify_cli/workflows/step/command_add.py index fec112b5cd..363f88fede 100644 --- a/src/specify_cli/workflows/step/command_add.py +++ b/src/specify_cli/workflows/step/command_add.py @@ -357,7 +357,8 @@ def _is_required_package_file(rel_path: object) -> bool: if package_file_count > installer._MAX_STEP_PACKAGE_FILES: raise installer.StepInstallError( f"Step package declares {package_file_count} files, exceeding the " - f"{installer._MAX_STEP_PACKAGE_FILES}-file limit" + f"{installer._MAX_STEP_PACKAGE_FILES}-entry limit (files and " + "directories combined)" ) from specify_cli.authentication.http import open_url as _open_url diff --git a/src/specify_cli/workflows/step/installer.py b/src/specify_cli/workflows/step/installer.py index 0a0bf18fa3..7d79589f64 100644 --- a/src/specify_cli/workflows/step/installer.py +++ b/src/specify_cli/workflows/step/installer.py @@ -194,7 +194,7 @@ def _reject_unsafe_destination(step_dir: Path) -> None: def _entry_limit_error() -> StepInstallError: return StepInstallError( "Step package contains too many entries, exceeding the " - f"{_MAX_STEP_PACKAGE_FILES}-file limit" + f"{_MAX_STEP_PACKAGE_FILES}-entry limit (files and directories combined)" ) diff --git a/tests/specify_cli/workflows/step/test_command_add.py b/tests/specify_cli/workflows/step/test_command_add.py index 0a20da6b22..2d3f968e8a 100644 --- a/tests/specify_cli/workflows/step/test_command_add.py +++ b/tests/specify_cli/workflows/step/test_command_add.py @@ -630,7 +630,7 @@ def test_add_rejects_too_many_package_files_before_network( assert result.exit_code != 0 assert result.exception is None or isinstance(result.exception, SystemExit) - assert "exceeding the 3-file limit" in result.output + assert "exceeding the 3-entry limit" in result.output steps_dir = project_dir / ".specify" / "workflows" / "steps" assert not (steps_dir / "my-step").exists() assert list(steps_dir.glob("speckit_step_tmp_*")) == [] diff --git a/tests/specify_cli/workflows/step/test_installer.py b/tests/specify_cli/workflows/step/test_installer.py index a4885520cf..a1ea095e5a 100644 --- a/tests/specify_cli/workflows/step/test_installer.py +++ b/tests/specify_cli/workflows/step/test_installer.py @@ -280,7 +280,7 @@ def test_validation_stops_reading_oversized_directory(tmp_path, monkeypatch): spy = _ScandirSpy(pkg) monkeypatch.setattr(os, "scandir", spy) - with pytest.raises(installer.StepInstallError, match="3-file limit"): + with pytest.raises(installer.StepInstallError, match="3-entry limit"): installer.validate_step_package(pkg, "my-step") assert spy.entries_read[pkg] <= 4 @@ -299,7 +299,7 @@ def test_validation_budget_spans_directories(tmp_path, monkeypatch): spy = _ScandirSpy(pkg) monkeypatch.setattr(os, "scandir", spy) - with pytest.raises(installer.StepInstallError, match="10-file limit"): + with pytest.raises(installer.StepInstallError, match="10-entry limit"): installer.validate_step_package(pkg, "my-step") assert sum(spy.entries_read.values()) <= 11 @@ -313,7 +313,7 @@ def test_copy_stops_reading_oversized_directory(tmp_path, monkeypatch): spy = _ScandirSpy(pkg) monkeypatch.setattr(os, "scandir", spy) - with pytest.raises(installer.StepInstallError, match="3-file limit"): + with pytest.raises(installer.StepInstallError, match="3-entry limit"): installer._copy_package_tree(pkg, tmp_path / "staged") assert spy.entries_read[pkg] <= 4 @@ -336,7 +336,7 @@ def test_excludes_are_not_copied(tmp_path, project_dir, monkeypatch): (pkg / "__pycache__" / "helper.pyc").write_text("x", encoding="utf-8") (pkg / ".DS_Store").write_text("x", encoding="utf-8") - # The excluded entries must not count against the file limit. + # The excluded entries must not count against the entry limit. monkeypatch.setattr(installer, "_MAX_STEP_PACKAGE_FILES", 2) installer.install_step_package(project_dir, "my-step", pkg, source="local") @@ -354,7 +354,7 @@ def test_file_limit_boundary(tmp_path, monkeypatch): (pkg / "extra.py").write_text("x", encoding="utf-8") with pytest.raises(installer.StepInstallError) as exc: installer.validate_step_package(pkg, "my-step") - assert "2-file limit" in str(exc.value) + assert "2-entry limit" in str(exc.value) def _nest_dirs(root: Path, depth: int) -> Path: @@ -375,7 +375,7 @@ def test_directories_count_toward_file_limit(tmp_path, monkeypatch): (pkg / "empty-b").mkdir() with pytest.raises(installer.StepInstallError) as exc: installer.validate_step_package(pkg, "my-step") - assert "3-file limit" in str(exc.value) + assert "3-entry limit" in str(exc.value) def test_depth_limit_boundary(tmp_path, monkeypatch): @@ -423,7 +423,7 @@ def test_copy_enforces_entry_limit_if_source_grows_after_validation( installer._copy_package_tree(pkg, tmp_path / "ok") (pkg / "empty-b").mkdir() - with pytest.raises(installer.StepInstallError, match="3-file limit"): + with pytest.raises(installer.StepInstallError, match="3-entry limit"): installer._copy_package_tree(pkg, tmp_path / "too-many") From a0f3c1cf3d664d09650f7408d09b30be9470fa17 Mon Sep 17 00:00:00 2001 From: Markus Date: Wed, 30 Sep 2026 18:28:39 +0200 Subject: [PATCH 13/16] fix(workflows): report step lock failures once `step remove` shares the step lock with `step add`, but the shared helper labelled every acquisition failure as "Failed to lock step installation". `step remove` then added its own prefix, printing "Failed to lock step removal '': Failed to lock step installation: ". - Make the helper's message operation-neutral ("Failed to acquire the step lock"), since install and remove both use it. - In `step remove`, report the underlying acquisition error after the removal prefix instead of the helper's message. - Assert a single lock message on the remove path, and add a `step add --dev` lock-failure regression. Both fail against the previous wording. Assisted-by: OpenCode (model: Claude Opus 5.5, autonomous) --- .../workflows/step/command_remove.py | 5 +++- src/specify_cli/workflows/step/installer.py | 6 +++-- .../workflows/step/test_command_add.py | 27 +++++++++++++++++++ .../workflows/step/test_command_remove.py | 6 ++++- 4 files changed, 40 insertions(+), 4 deletions(-) diff --git a/src/specify_cli/workflows/step/command_remove.py b/src/specify_cli/workflows/step/command_remove.py index f8dcd1b024..52a9a08a6b 100644 --- a/src/specify_cli/workflows/step/command_remove.py +++ b/src/specify_cli/workflows/step/command_remove.py @@ -114,10 +114,13 @@ def workflow_step_remove( with _step_install_transaction(project_root): _remove_step_locked(project_root, step_id) except StepInstallError as exc: + # Report the underlying acquisition error so the message is not + # prefixed twice by the helper's own lock-failure text. + cause = exc.__cause__ or exc cli.console.print( "[red]Error:[/red] Failed to lock step removal " f"'{cli._escape_markup(step_id)}': " - f"{cli._escape_markup(str(exc))}" + f"{cli._escape_markup(str(cause))}" ) raise cli.typer.Exit(1) diff --git a/src/specify_cli/workflows/step/installer.py b/src/specify_cli/workflows/step/installer.py index 7d79589f64..91e52c1155 100644 --- a/src/specify_cli/workflows/step/installer.py +++ b/src/specify_cli/workflows/step/installer.py @@ -85,7 +85,9 @@ def _step_install_transaction(project_root: Path): from ...shared_infra import _exclusive_project_lock # Only acquisition failures are reported as lock errors; exceptions raised - # by the caller's critical section propagate unchanged. + # by the caller's critical section propagate unchanged. The message is + # operation-neutral because both install and remove share this lock; callers + # that add their own context should report ``exc.__cause__`` instead. with contextlib.ExitStack() as stack: try: stack.enter_context( @@ -94,7 +96,7 @@ def _step_install_transaction(project_root: Path): ) ) except OSError as exc: - raise StepInstallError(f"Failed to lock step installation: {exc}") from exc + raise StepInstallError(f"Failed to acquire the step lock: {exc}") from exc yield diff --git a/tests/specify_cli/workflows/step/test_command_add.py b/tests/specify_cli/workflows/step/test_command_add.py index 2d3f968e8a..f996628cec 100644 --- a/tests/specify_cli/workflows/step/test_command_add.py +++ b/tests/specify_cli/workflows/step/test_command_add.py @@ -1028,6 +1028,33 @@ def test_dev_rejects_missing_init(self, project_dir, tmp_path, monkeypatch): assert result.exit_code != 0 assert "__init__.py" in result.output + def test_dev_lock_failure_is_reported_once_without_installing( + self, project_dir, tmp_path, monkeypatch + ): + from typer.testing import CliRunner + + from specify_cli import app + + package = _write_package(tmp_path, type_key="dev-step") + monkeypatch.chdir(project_dir) + # A directory at the lock path cannot be opened as the lock file. + lock_path = project_dir / ".specify" / ".step-install.lock" + if lock_path.exists(): + lock_path.unlink() + lock_path.mkdir() + + result = CliRunner().invoke( + app, ["workflow", "step", "add", "dev-step", "--dev", str(package)] + ) + + assert result.exit_code == 1, result.output + output = result.output.replace("\n", "") + assert "Failed to acquire the step lock" in output + assert output.count("Failed to") == 1, output + assert not ( + project_dir / ".specify" / "workflows" / "steps" / "dev-step" + ).exists() + def test_dev_rejects_symlinked_source_root(self, project_dir, tmp_path, monkeypatch): if not hasattr(os, "symlink"): pytest.skip("symlinks are unavailable") diff --git a/tests/specify_cli/workflows/step/test_command_remove.py b/tests/specify_cli/workflows/step/test_command_remove.py index 0183470584..790d8810c6 100644 --- a/tests/specify_cli/workflows/step/test_command_remove.py +++ b/tests/specify_cli/workflows/step/test_command_remove.py @@ -182,7 +182,11 @@ def test_remove_fails_cleanly_when_lock_cannot_be_acquired( result = CliRunner().invoke(app, ["workflow", "step", "remove", "my-step"]) assert result.exit_code == 1, result.output - assert "Failed to lock step removal" in result.output + output = _flat(result.output) + assert "Failed to lock step removal 'my-step'" in output + # One removal-specific message, not wrapped in the shared helper's text. + assert output.count("Failed to") == 1, output + assert "installation" not in output assert _registered_ids(project_dir) == {"my-step"} assert (_steps_dir(project_dir) / "my-step").is_dir() From f79dcc29eb58c7ca5c54408bd02dc61ce4eafb3a Mon Sep 17 00:00:00 2001 From: Markus Wondrak Date: Wed, 30 Sep 2026 18:39:30 +0200 Subject: [PATCH 14/16] Update context string in project lock Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/step/installer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/step/installer.py b/src/specify_cli/workflows/step/installer.py index 91e52c1155..de55d1f39d 100644 --- a/src/specify_cli/workflows/step/installer.py +++ b/src/specify_cli/workflows/step/installer.py @@ -92,7 +92,7 @@ def _step_install_transaction(project_root: Path): try: stack.enter_context( _exclusive_project_lock( - Path(project_root), ".step-install.lock", context="step install" + Path(project_root), ".step-install.lock", context="step" ) ) except OSError as exc: From c7f8a513441149c3a6aead33ed571ad9c3e423ad Mon Sep 17 00:00:00 2001 From: Markus Date: Wed, 30 Sep 2026 19:06:54 +0200 Subject: [PATCH 15/16] fix(bundles): restore failed step refreshes under the step lock When a bundle step refresh removes a step and the reinstall fails, the rollback reloaded `step-registry.json`, put the old entry back and saved, all without the step lock that `step add` and `step remove` hold. A concurrent step operation could commit between that reload and save, or save a stale snapshot over the restored entry, and lose a registry entry. - Run the rollback's directory and registry restore inside `_step_install_transaction`, reloading the registry after the lock is taken. - Skip the restore if the step was registered again after the removal, so a newer package and entry are not overwritten by the stale backup. - If the lock can't be acquired, leave the project untouched and add a note to the reinstall error instead of restoring unlocked. - Add regressions for a concurrent writer holding the lock during the rollback, a concurrent reinstall of the same step, and lock failure; all three fail against the previous rollback. Also cover `step remove` with a symlinked lock file, which fails against the old `step install` lock context. Assisted-by: OpenCode (model: Claude Opus 5.5, autonomous) --- src/specify_cli/bundles/primitives.py | 89 +++++++--- tests/specify_cli/bundles/test_primitives.py | 166 ++++++++++++++++++ .../workflows/step/test_command_remove.py | 33 ++++ 3 files changed, 261 insertions(+), 27 deletions(-) diff --git a/src/specify_cli/bundles/primitives.py b/src/specify_cli/bundles/primitives.py index b32342e68d..5e5e073475 100644 --- a/src/specify_cli/bundles/primitives.py +++ b/src/specify_cli/bundles/primitives.py @@ -459,37 +459,72 @@ def refresh(self, component: ComponentRef) -> None: self.remove(component) try: self.install(component) - except BundlerError: - if backup_dir.exists(): - shutil.copytree(backup_dir, step_dir, dirs_exist_ok=True) - # Re-read the registry: ``StepRegistry`` snapshots the file once - # in ``__init__`` (``self.data = self._load()``) and - # ``is_installed`` only consults that snapshot. ``self.remove()`` - # above has already deleted the entry from disk, but - # ``self._registry``'s snapshot still contains it -- so the - # guard was always False here and the restore never ran, in - # exactly the failure case it was written for. The step package - # came back but stayed unregistered: ``workflow step list`` - # stopped showing it and ``workflow step add`` then refused with - # "Step directory already exists". - from ..workflows.catalog import StepRegistry - - current = StepRegistry(self._root) - if metadata is not None and not current.is_installed(component.id): - # Restore the saved entry verbatim rather than via ``add()``, - # which would rewrite the metadata it is meant to roll back: - # this registry is freshly constructed *after* - # ``self.remove()`` deleted the entry, so ``add()`` sees no - # existing record and stamps ``installed_at`` with - # ``datetime.now()`` (it also overwrites ``updated_at`` - # unconditionally). ``workflow_step_remove`` bypasses - # ``add()`` for exactly this reason. - current.data["steps"][component.id] = metadata - current.save() + except BundlerError as exc: + from ..workflows.step.installer import ( + StepInstallError, + _step_install_transaction, + ) + + # Restore under the step lock that `step add` / `step remove` + # hold, so a concurrent step operation can't commit between the + # registry reload and save below (or save a stale snapshot over + # the restored entry). If the lock can't be taken, leave the + # project as it is rather than restoring unlocked. + try: + with _step_install_transaction(self._root): + self._restore_refresh_backup( + component.id, step_dir, backup_dir, metadata + ) + except StepInstallError as lock_exc: + exc.add_note( + f"Step '{component.id}' was not restored after the failed " + f"refresh: {lock_exc}" + ) raise finally: shutil.rmtree(backup_dir.parent, ignore_errors=True) + def _restore_refresh_backup( + self, + step_id: str, + step_dir: Path, + backup_dir: Path, + metadata: dict | None, + ) -> None: + """Put back a step removed by a failed refresh. Caller holds the step lock.""" + import shutil + + # Re-read the registry: ``StepRegistry`` snapshots the file once in + # ``__init__`` (``self.data = self._load()``) and ``is_installed`` only + # consults that snapshot. ``self.remove()`` has already deleted the entry + # from disk, but ``self._registry``'s snapshot still contains it -- so a + # guard on it was always False and the restore never ran, in exactly the + # failure case it was written for. The step package came back but stayed + # unregistered: ``workflow step list`` stopped showing it and + # ``workflow step add`` then refused with "Step directory already + # exists". Reading it under the lock also picks up entries committed by + # concurrent step operations, so saving it cannot drop them. + from ..workflows.catalog import StepRegistry + + current = StepRegistry(self._root) + if current.is_installed(step_id): + # A concurrent operation registered this step after the removal; + # don't overwrite its package or entry with the backup. + return + if backup_dir.exists(): + shutil.copytree(backup_dir, step_dir, dirs_exist_ok=True) + if metadata is not None: + # Restore the saved entry verbatim rather than via ``add()``, which + # would rewrite the metadata it is meant to roll back: this registry + # is freshly constructed *after* ``self.remove()`` deleted the entry, + # so ``add()`` sees no existing record and stamps ``installed_at`` + # with ``datetime.now()`` (it also overwrites ``updated_at`` + # unconditionally). ``workflow_step_remove`` bypasses ``add()`` for + # exactly this reason. + current.data["steps"][step_id] = metadata + current.save() + shutil.rmtree(backup_dir.parent, ignore_errors=True) + def remove(self, component: ComponentRef) -> None: from .. import workflow_step_remove diff --git a/tests/specify_cli/bundles/test_primitives.py b/tests/specify_cli/bundles/test_primitives.py index 8cbcbf85a2..ba39e40381 100644 --- a/tests/specify_cli/bundles/test_primitives.py +++ b/tests/specify_cli/bundles/test_primitives.py @@ -632,3 +632,169 @@ def _boom(step_id, *args, **kwargs): # A rollback must be a rollback: the entry comes back byte-for-byte, not # re-registered with fresh ``installed_at`` / ``updated_at`` stamps. assert restored.get("my-step") == seeded + + +def _seed_installed_step(project_root: Path, step_id: str = "my-step") -> None: + import json + + from specify_cli.workflows.catalog import StepRegistry + + steps_dir = project_root / ".specify" / "workflows" / "steps" + (steps_dir / step_id).mkdir(parents=True) + (steps_dir / step_id / "step.yml").write_text( + f"step:\n type_key: {step_id}\n", encoding="utf-8" + ) + (steps_dir / step_id / "__init__.py").write_text("", encoding="utf-8") + (steps_dir / StepRegistry.REGISTRY_FILE).write_text( + json.dumps( + { + "schema_version": "1.0", + "steps": { + step_id: { + "name": "My Step", + "version": "1.0.0", + "type_key": step_id, + "installed_at": "2020-01-01T00:00:00+00:00", + "updated_at": "2020-02-02T00:00:00+00:00", + } + }, + } + ), + encoding="utf-8", + ) + + +def test_step_refresh_rollback_waits_for_concurrent_registry_writer( + tmp_path: Path, monkeypatch +): + """The refresh rollback must restore the registry under the step lock. + + A concurrent step operation holds the lock with a registry snapshot taken + after the refresh removed the entry. If the rollback restores the entry + without the lock, that writer's stale save drops the restored entry. With + the lock, the rollback waits and restores against the committed registry, + so both entries survive. + """ + import threading + + import specify_cli + from specify_cli.workflows.catalog import StepRegistry + from specify_cli.workflows.step.installer import _step_install_transaction + from tests.lock_helpers import watch_lock_attempt, wait_until_blocked_or_done + + _seed_installed_step(tmp_path) + install_failed = threading.Event() + writer_loaded = threading.Event() + refresh_done = threading.Event() + errors: list[BaseException] = [] + + def _failing_install(step_id, *args, **kwargs): + install_failed.set() + assert writer_loaded.wait(10), "writer never loaded the registry" + raise BundlerError(f"Failed to install step '{step_id}'.") + + monkeypatch.setattr(specify_cli, "workflow_step_add", _failing_install) + manager = primitive_manager("steps", tmp_path, allow_network=True) + + def _refresh(): + try: + manager.refresh(_component("steps", "my-step")) + except BundlerError: + pass + except BaseException as exc: # noqa: BLE001 - surfaced by the test + errors.append(exc) + finally: + refresh_done.set() + + refresher = threading.Thread(target=_refresh, name="refresher") + refresher.start() + try: + assert install_failed.wait(10), "refresh never reached the reinstall" + # Watch only the rollback's lock attempt: removal has already taken + # and released the lock in this thread. + attempted = watch_lock_attempt(monkeypatch, "refresher") + with _step_install_transaction(tmp_path): + writer = StepRegistry(tmp_path) + assert not writer.is_installed("my-step") + writer_loaded.set() + wait_until_blocked_or_done(attempted, refresh_done) + writer.add("other-step", {"name": "Other", "version": "1.0.0"}) + finally: + writer_loaded.set() + refresher.join(10) + + assert not refresher.is_alive() + assert errors == [] + registry = StepRegistry(tmp_path) + assert registry.is_installed("other-step") + assert registry.is_installed("my-step"), ( + tmp_path / ".specify" / "workflows" / "steps" / StepRegistry.REGISTRY_FILE + ).read_text(encoding="utf-8") + + +def test_step_refresh_rollback_lock_failure_keeps_install_error( + tmp_path: Path, monkeypatch +): + """If the rollback cannot take the step lock, it leaves state untouched. + + The reinstall error is still raised, with a note explaining that the + previous step was not restored, instead of restoring without the lock. + """ + import specify_cli + from specify_cli.workflows.catalog import StepRegistry + + _seed_installed_step(tmp_path) + lock_path = tmp_path / ".specify" / ".step-install.lock" + + def _failing_install(step_id, *args, **kwargs): + # Make the lock unobtainable for the rollback only; removal has + # already released it. + if lock_path.exists(): + lock_path.unlink() + lock_path.mkdir() + raise BundlerError(f"Failed to install step '{step_id}'.") + + monkeypatch.setattr(specify_cli, "workflow_step_add", _failing_install) + manager = primitive_manager("steps", tmp_path, allow_network=True) + + with pytest.raises(BundlerError, match="Failed to install step 'my-step'") as info: + manager.refresh(_component("steps", "my-step")) + + notes = "\n".join(getattr(info.value, "__notes__", ())) + assert "was not restored" in notes + assert "Failed to acquire the step lock" in notes + assert not StepRegistry(tmp_path).is_installed("my-step") + assert not (tmp_path / ".specify" / "workflows" / "steps" / "my-step").exists() + + +def test_step_refresh_rollback_keeps_concurrently_installed_step( + tmp_path: Path, monkeypatch +): + """The rollback must not overwrite a step installed after the removal. + + If another operation registers the same step before the rollback takes + the lock, the backup is stale: restoring it would overwrite the new + package files and registry entry. + """ + import specify_cli + from specify_cli.workflows.catalog import StepRegistry + + _seed_installed_step(tmp_path) + step_dir = tmp_path / ".specify" / "workflows" / "steps" / "my-step" + new_entry = {"name": "My Step", "version": "2.0.0", "type_key": "my-step"} + + def _concurrent_install_then_fail(step_id, *args, **kwargs): + step_dir.mkdir(parents=True) + (step_dir / "step.yml").write_text("new package\n", encoding="utf-8") + StepRegistry(tmp_path).add(step_id, new_entry) + raise BundlerError(f"Failed to install step '{step_id}'.") + + monkeypatch.setattr(specify_cli, "workflow_step_add", _concurrent_install_then_fail) + manager = primitive_manager("steps", tmp_path, allow_network=True) + + with pytest.raises(BundlerError, match="Failed to install step 'my-step'"): + manager.refresh(_component("steps", "my-step")) + + assert StepRegistry(tmp_path).get("my-step")["version"] == "2.0.0" + assert (step_dir / "step.yml").read_text(encoding="utf-8") == "new package\n" + assert not (step_dir / "__init__.py").exists() diff --git a/tests/specify_cli/workflows/step/test_command_remove.py b/tests/specify_cli/workflows/step/test_command_remove.py index 790d8810c6..ed7237a52a 100644 --- a/tests/specify_cli/workflows/step/test_command_remove.py +++ b/tests/specify_cli/workflows/step/test_command_remove.py @@ -190,6 +190,39 @@ def test_remove_fails_cleanly_when_lock_cannot_be_acquired( assert _registered_ids(project_dir) == {"my-step"} assert (_steps_dir(project_dir) / "my-step").is_dir() + @pytest.mark.skipif(not hasattr(os, "symlink"), reason="symlinks are unavailable") + def test_remove_symlinked_lock_error_uses_neutral_lock_wording( + self, project_dir, tmp_path, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + monkeypatch.chdir(project_dir) + _install(project_dir, tmp_path, "my-step") + lock_path = project_dir / ".specify" / ".step-install.lock" + if lock_path.exists(): + lock_path.unlink() + target = tmp_path / "elsewhere.lock" + target.write_text("", encoding="utf-8") + try: + lock_path.symlink_to(target) + except OSError as exc: + pytest.skip(f"cannot create symlink: {exc}") + + result = CliRunner().invoke(app, ["workflow", "step", "remove", "my-step"]) + + assert result.exit_code == 1, result.output + # Rich wraps at spaces; collapse whitespace so wrapping can't split words. + output = " ".join(result.output.split()) + assert ( + "Failed to lock step removal 'my-step': " + "Refusing to use symlinked step lock" + ) in output + # The shared lock must not describe a removal as an install. + assert "install lock" not in output + assert _registered_ids(project_dir) == {"my-step"} + assert (_steps_dir(project_dir) / "my-step").is_dir() + def test_remove_restores_registry_entry_when_directory_delete_fails( self, project_dir, tmp_path, monkeypatch ): From 996981d4d10a0881008d0a1a2aee0066c8f0fea4 Mon Sep 17 00:00:00 2001 From: Markus Date: Wed, 30 Sep 2026 19:28:53 +0200 Subject: [PATCH 16/16] revert(bundles): keep the step refresh rollback out of this PR Reverts the `bundles/primitives.py` part of c7f8a513 and its three rollback tests. The step refresh rollback is pre-existing code that this PR did not otherwise touch; locking it pulled the bundle refresh flow into review scope, and further findings there (snapshot and removal before the lock, restore failures masking the reinstall error) are also pre-existing on `main`. It belongs in a separate change for bundle refresh atomicity. The `step remove` symlinked-lock test from c7f8a513 is kept: it covers the step lock and removal locking that this PR adds. Assisted-by: OpenCode (model: Claude Opus 5.5, autonomous) --- src/specify_cli/bundles/primitives.py | 89 +++------- tests/specify_cli/bundles/test_primitives.py | 166 ------------------- 2 files changed, 27 insertions(+), 228 deletions(-) diff --git a/src/specify_cli/bundles/primitives.py b/src/specify_cli/bundles/primitives.py index 5e5e073475..b32342e68d 100644 --- a/src/specify_cli/bundles/primitives.py +++ b/src/specify_cli/bundles/primitives.py @@ -459,72 +459,37 @@ def refresh(self, component: ComponentRef) -> None: self.remove(component) try: self.install(component) - except BundlerError as exc: - from ..workflows.step.installer import ( - StepInstallError, - _step_install_transaction, - ) - - # Restore under the step lock that `step add` / `step remove` - # hold, so a concurrent step operation can't commit between the - # registry reload and save below (or save a stale snapshot over - # the restored entry). If the lock can't be taken, leave the - # project as it is rather than restoring unlocked. - try: - with _step_install_transaction(self._root): - self._restore_refresh_backup( - component.id, step_dir, backup_dir, metadata - ) - except StepInstallError as lock_exc: - exc.add_note( - f"Step '{component.id}' was not restored after the failed " - f"refresh: {lock_exc}" - ) + except BundlerError: + if backup_dir.exists(): + shutil.copytree(backup_dir, step_dir, dirs_exist_ok=True) + # Re-read the registry: ``StepRegistry`` snapshots the file once + # in ``__init__`` (``self.data = self._load()``) and + # ``is_installed`` only consults that snapshot. ``self.remove()`` + # above has already deleted the entry from disk, but + # ``self._registry``'s snapshot still contains it -- so the + # guard was always False here and the restore never ran, in + # exactly the failure case it was written for. The step package + # came back but stayed unregistered: ``workflow step list`` + # stopped showing it and ``workflow step add`` then refused with + # "Step directory already exists". + from ..workflows.catalog import StepRegistry + + current = StepRegistry(self._root) + if metadata is not None and not current.is_installed(component.id): + # Restore the saved entry verbatim rather than via ``add()``, + # which would rewrite the metadata it is meant to roll back: + # this registry is freshly constructed *after* + # ``self.remove()`` deleted the entry, so ``add()`` sees no + # existing record and stamps ``installed_at`` with + # ``datetime.now()`` (it also overwrites ``updated_at`` + # unconditionally). ``workflow_step_remove`` bypasses + # ``add()`` for exactly this reason. + current.data["steps"][component.id] = metadata + current.save() raise finally: shutil.rmtree(backup_dir.parent, ignore_errors=True) - def _restore_refresh_backup( - self, - step_id: str, - step_dir: Path, - backup_dir: Path, - metadata: dict | None, - ) -> None: - """Put back a step removed by a failed refresh. Caller holds the step lock.""" - import shutil - - # Re-read the registry: ``StepRegistry`` snapshots the file once in - # ``__init__`` (``self.data = self._load()``) and ``is_installed`` only - # consults that snapshot. ``self.remove()`` has already deleted the entry - # from disk, but ``self._registry``'s snapshot still contains it -- so a - # guard on it was always False and the restore never ran, in exactly the - # failure case it was written for. The step package came back but stayed - # unregistered: ``workflow step list`` stopped showing it and - # ``workflow step add`` then refused with "Step directory already - # exists". Reading it under the lock also picks up entries committed by - # concurrent step operations, so saving it cannot drop them. - from ..workflows.catalog import StepRegistry - - current = StepRegistry(self._root) - if current.is_installed(step_id): - # A concurrent operation registered this step after the removal; - # don't overwrite its package or entry with the backup. - return - if backup_dir.exists(): - shutil.copytree(backup_dir, step_dir, dirs_exist_ok=True) - if metadata is not None: - # Restore the saved entry verbatim rather than via ``add()``, which - # would rewrite the metadata it is meant to roll back: this registry - # is freshly constructed *after* ``self.remove()`` deleted the entry, - # so ``add()`` sees no existing record and stamps ``installed_at`` - # with ``datetime.now()`` (it also overwrites ``updated_at`` - # unconditionally). ``workflow_step_remove`` bypasses ``add()`` for - # exactly this reason. - current.data["steps"][step_id] = metadata - current.save() - shutil.rmtree(backup_dir.parent, ignore_errors=True) - def remove(self, component: ComponentRef) -> None: from .. import workflow_step_remove diff --git a/tests/specify_cli/bundles/test_primitives.py b/tests/specify_cli/bundles/test_primitives.py index ba39e40381..8cbcbf85a2 100644 --- a/tests/specify_cli/bundles/test_primitives.py +++ b/tests/specify_cli/bundles/test_primitives.py @@ -632,169 +632,3 @@ def _boom(step_id, *args, **kwargs): # A rollback must be a rollback: the entry comes back byte-for-byte, not # re-registered with fresh ``installed_at`` / ``updated_at`` stamps. assert restored.get("my-step") == seeded - - -def _seed_installed_step(project_root: Path, step_id: str = "my-step") -> None: - import json - - from specify_cli.workflows.catalog import StepRegistry - - steps_dir = project_root / ".specify" / "workflows" / "steps" - (steps_dir / step_id).mkdir(parents=True) - (steps_dir / step_id / "step.yml").write_text( - f"step:\n type_key: {step_id}\n", encoding="utf-8" - ) - (steps_dir / step_id / "__init__.py").write_text("", encoding="utf-8") - (steps_dir / StepRegistry.REGISTRY_FILE).write_text( - json.dumps( - { - "schema_version": "1.0", - "steps": { - step_id: { - "name": "My Step", - "version": "1.0.0", - "type_key": step_id, - "installed_at": "2020-01-01T00:00:00+00:00", - "updated_at": "2020-02-02T00:00:00+00:00", - } - }, - } - ), - encoding="utf-8", - ) - - -def test_step_refresh_rollback_waits_for_concurrent_registry_writer( - tmp_path: Path, monkeypatch -): - """The refresh rollback must restore the registry under the step lock. - - A concurrent step operation holds the lock with a registry snapshot taken - after the refresh removed the entry. If the rollback restores the entry - without the lock, that writer's stale save drops the restored entry. With - the lock, the rollback waits and restores against the committed registry, - so both entries survive. - """ - import threading - - import specify_cli - from specify_cli.workflows.catalog import StepRegistry - from specify_cli.workflows.step.installer import _step_install_transaction - from tests.lock_helpers import watch_lock_attempt, wait_until_blocked_or_done - - _seed_installed_step(tmp_path) - install_failed = threading.Event() - writer_loaded = threading.Event() - refresh_done = threading.Event() - errors: list[BaseException] = [] - - def _failing_install(step_id, *args, **kwargs): - install_failed.set() - assert writer_loaded.wait(10), "writer never loaded the registry" - raise BundlerError(f"Failed to install step '{step_id}'.") - - monkeypatch.setattr(specify_cli, "workflow_step_add", _failing_install) - manager = primitive_manager("steps", tmp_path, allow_network=True) - - def _refresh(): - try: - manager.refresh(_component("steps", "my-step")) - except BundlerError: - pass - except BaseException as exc: # noqa: BLE001 - surfaced by the test - errors.append(exc) - finally: - refresh_done.set() - - refresher = threading.Thread(target=_refresh, name="refresher") - refresher.start() - try: - assert install_failed.wait(10), "refresh never reached the reinstall" - # Watch only the rollback's lock attempt: removal has already taken - # and released the lock in this thread. - attempted = watch_lock_attempt(monkeypatch, "refresher") - with _step_install_transaction(tmp_path): - writer = StepRegistry(tmp_path) - assert not writer.is_installed("my-step") - writer_loaded.set() - wait_until_blocked_or_done(attempted, refresh_done) - writer.add("other-step", {"name": "Other", "version": "1.0.0"}) - finally: - writer_loaded.set() - refresher.join(10) - - assert not refresher.is_alive() - assert errors == [] - registry = StepRegistry(tmp_path) - assert registry.is_installed("other-step") - assert registry.is_installed("my-step"), ( - tmp_path / ".specify" / "workflows" / "steps" / StepRegistry.REGISTRY_FILE - ).read_text(encoding="utf-8") - - -def test_step_refresh_rollback_lock_failure_keeps_install_error( - tmp_path: Path, monkeypatch -): - """If the rollback cannot take the step lock, it leaves state untouched. - - The reinstall error is still raised, with a note explaining that the - previous step was not restored, instead of restoring without the lock. - """ - import specify_cli - from specify_cli.workflows.catalog import StepRegistry - - _seed_installed_step(tmp_path) - lock_path = tmp_path / ".specify" / ".step-install.lock" - - def _failing_install(step_id, *args, **kwargs): - # Make the lock unobtainable for the rollback only; removal has - # already released it. - if lock_path.exists(): - lock_path.unlink() - lock_path.mkdir() - raise BundlerError(f"Failed to install step '{step_id}'.") - - monkeypatch.setattr(specify_cli, "workflow_step_add", _failing_install) - manager = primitive_manager("steps", tmp_path, allow_network=True) - - with pytest.raises(BundlerError, match="Failed to install step 'my-step'") as info: - manager.refresh(_component("steps", "my-step")) - - notes = "\n".join(getattr(info.value, "__notes__", ())) - assert "was not restored" in notes - assert "Failed to acquire the step lock" in notes - assert not StepRegistry(tmp_path).is_installed("my-step") - assert not (tmp_path / ".specify" / "workflows" / "steps" / "my-step").exists() - - -def test_step_refresh_rollback_keeps_concurrently_installed_step( - tmp_path: Path, monkeypatch -): - """The rollback must not overwrite a step installed after the removal. - - If another operation registers the same step before the rollback takes - the lock, the backup is stale: restoring it would overwrite the new - package files and registry entry. - """ - import specify_cli - from specify_cli.workflows.catalog import StepRegistry - - _seed_installed_step(tmp_path) - step_dir = tmp_path / ".specify" / "workflows" / "steps" / "my-step" - new_entry = {"name": "My Step", "version": "2.0.0", "type_key": "my-step"} - - def _concurrent_install_then_fail(step_id, *args, **kwargs): - step_dir.mkdir(parents=True) - (step_dir / "step.yml").write_text("new package\n", encoding="utf-8") - StepRegistry(tmp_path).add(step_id, new_entry) - raise BundlerError(f"Failed to install step '{step_id}'.") - - monkeypatch.setattr(specify_cli, "workflow_step_add", _concurrent_install_then_fail) - manager = primitive_manager("steps", tmp_path, allow_network=True) - - with pytest.raises(BundlerError, match="Failed to install step 'my-step'"): - manager.refresh(_component("steps", "my-step")) - - assert StepRegistry(tmp_path).get("my-step")["version"] == "2.0.0" - assert (step_dir / "step.yml").read_text(encoding="utf-8") == "new package\n" - assert not (step_dir / "__init__.py").exists()