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/overview.md b/docs/reference/overview.md index cbfd1c48a8..99a0e8089a 100644 --- a/docs/reference/overview.md +++ b/docs/reference/overview.md @@ -34,7 +34,7 @@ Artifacts are the commands, templates, and scripts a project exposes, whichever ## Workflows -Workflows automate multi-step Spec-Driven Development processes into repeatable sequences. They chain commands, prompts, shell steps, and human checkpoints together, with support for conditional logic, loops, fan-out/fan-in, and the ability to pause and resume from the exact point of interruption. +Workflows automate multi-step Spec-Driven Development processes into repeatable sequences. They chain commands, prompts, shell steps, and human checkpoints together, with support for conditional logic, loops, fan-out/fan-in, workflow composition (running another installed workflow as a scoped subtree via a `type: workflow` step), and the ability to pause and resume from the exact point of interruption. [Workflows reference →](workflows.md) diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index 811ab4ebf4..d29518083e 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -1,6 +1,6 @@ # Workflows -Workflows automate multi-step Spec-Driven Development processes — chaining commands, prompts, shell steps, and human checkpoints into repeatable sequences. They support conditional logic, loops, fan-out/fan-in, and can be paused and resumed from the exact point of interruption. +Workflows automate multi-step Spec-Driven Development processes — chaining commands, prompts, shell steps, and human checkpoints into repeatable sequences. They support conditional logic, loops, fan-out/fan-in, composition (running another installed workflow as a scoped subtree), and can be paused and resumed from the exact point of interruption. ## Run a Workflow @@ -540,9 +540,246 @@ specify workflow run speckit -i spec="Build a kanban board with drag-and-drop ta | `do-while` | Execute at least once, then loop on condition | | `fan-out` | Dispatch a step for each item in a list | | `fan-in` | Aggregate results from a fan-out step | +| `workflow` | Run an installed workflow as a scoped subtree | > **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. +- 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 + +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). + +### Workflow composition (`type: workflow`) + +A `workflow` step runs an installed workflow as a **scoped subtree of the +current run** — there is one run, one run directory, and one process. The +included workflow behaves like a function call: values cross the boundary only +through its declared `inputs` and `outputs`. + +```yaml +steps: + - id: triage + type: prompt + prompt: "Select the workflow to run" + + - id: run-selected + type: workflow + workflow: "{{ steps.triage.output.stdout }}" + input: + report: "{{ inputs.report }}" + slug: "{{ inputs.slug }}" +``` + +| Field | Required | Description | +| ---------- | -------- | ----------- | +| `workflow` | yes | Installed workflow ID, or an expression evaluated in the caller's scope. The resolved value must be a valid ID of a registered, installed, and enabled workflow. Literal IDs are validated at definition time. | +| `input` | no | Mapping of the target's declared input names to values evaluated in the caller's scope. An undeclared name is rejected. Defaults, required, type, and enum rules apply. | + +`type: workflow` is an engine facility (like `fan-out`), not a custom-step API. +The engine owns the nested scope tree; custom steps still receive only a +`StepContext`. + +#### Scope isolation + +The included workflow receives a separate expression scope: + +- `inputs` contains only the resolved, declared, and validated mapped inputs. +- `steps` contains only the included workflow's own step results. +- Caller inputs and caller step results are **not** visible unless explicitly + passed through the `input` mapping. +- Project root, integration defaults, and the run ID remain available as + execution infrastructure. + +#### Declared outputs + +An included workflow exposes values back to its caller only through a top-level +`outputs` block. Each entry requires a `value` expression evaluated in the +included workflow's local scope once it completes: + +```yaml +outputs: + result: + value: "{{ steps.fix.output.stdout }}" + tested: + value: "{{ steps.test.output.exit_code == 0 }}" +``` + +The caller reads them from the workflow step's output: + +```yaml +"{{ steps.run-selected.output.result }}" +``` + +Output names must be safe lowercase identifiers and cannot use the reserved +names `workflow`, `status`, `error`, `aborted`, `integration`, `model`, +`options`, or `input`. A whole expression preserves its resolved type; +interpolation mixed with text produces a string. Paused, failed, and aborted +scopes do not evaluate outputs. + +#### Lifecycle and failure handling + +The workflow step reports the aggregate outcome of its subtree: all required +steps complete → `completed`; an included step pauses → the run pauses; an +included failure (unhandled) → the run fails; an included gate abort → the run +aborts (`output.aborted: true`). `continue_on_error: true` on the workflow step +lets the caller continue past an otherwise unhandled included failure; it never +overrides an abort or bypasses a pause. + +#### Resume and composition limits + +The resolved target, its composed definition snapshot (including overlays), and +the validated inputs are persisted with the run. On resume the engine reuses the +snapshot and resumes at the included scope's local step index; it does not +re-resolve the target. Editing an installed workflow affects new invocations, +not a scope already bound within a persisted run. `workflow resume --input` +updates the **root** workflow's inputs; a composing workflow forwards them by +mapping them into the child's declared inputs. + +Recursive composition is allowed, but cycles are rejected by path (`A -> B -> A` +fails while `A -> B -> D` and `A -> C -> D` is a legal diamond). Composition is +limited to 16 included levels; the root is depth 0 and entering depth 17 is +rejected. + ### Per-Step Integration Configuration Command steps may pass structured runtime configuration to integrations that diff --git a/src/specify_cli/workflows/__init__.py b/src/specify_cli/workflows/__init__.py index 2bb3de56a5..20aeb2024a 100644 --- a/src/specify_cli/workflows/__init__.py +++ b/src/specify_cli/workflows/__init__.py @@ -56,6 +56,7 @@ def _register_builtin_steps() -> None: from .step.slot import SlotStep from .step.switch import SwitchStep from .step.while_loop import WhileStep + from .step.workflow import WorkflowStep _register_step(CommandStep()) _register_step(DoWhileStep()) @@ -69,17 +70,34 @@ def _register_builtin_steps() -> None: _register_step(SlotStep()) _register_step(SwitchStep()) _register_step(WhileStep()) + _register_step(WorkflowStep()) _register_builtin_steps() # 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 +115,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 +211,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 +226,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/_commands.py b/src/specify_cli/workflows/_commands.py index de01789d0c..fb7b5adf23 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -925,6 +925,25 @@ def _failed_step_error(state: Any) -> str | None: return getattr(state, "error", None) +def _scope_summary(scopes: Any) -> list[dict[str, Any]]: + """Compact nested-scope summary for the machine-readable payload.""" + summary: list[dict[str, Any]] = [] + if not isinstance(scopes, dict): + return summary + for key, record in scopes.items(): + if not isinstance(record, dict): + continue + summary.append( + { + "invocation_id": key, + "workflow_id": record.get("workflow_id"), + "status": record.get("status"), + "scopes": _scope_summary(record.get("workflow_scopes")), + } + ) + return summary + + def _workflow_run_payload(state: Any) -> dict[str, Any]: """Machine-readable summary of a run/resume outcome.""" payload = { @@ -940,6 +959,11 @@ def _workflow_run_payload(state: Any) -> dict[str, Any]: error = _failed_step_error(state) if error is not None: payload["error"] = error + # Only present when composition is in play, so existing payloads stay + # byte-for-byte stable for runs without nested scopes. + scopes = _scope_summary(getattr(state, "workflow_scopes", None)) + if scopes: + payload["scopes"] = scopes return payload diff --git a/src/specify_cli/workflows/command_status.py b/src/specify_cli/workflows/command_status.py index d9af4b48d6..8179722227 100644 --- a/src/specify_cli/workflows/command_status.py +++ b/src/specify_cli/workflows/command_status.py @@ -5,6 +5,29 @@ from . import _commands as cli +def _render_scopes(scopes: dict, indent: str) -> None: + """Render nested composition scopes indented in the human status view.""" + colors = { + "completed": "green", + "failed": "red", + "aborted": "red", + "paused": "yellow", + "running": "blue", + } + for key, record in scopes.items(): + if not isinstance(record, dict): + continue + s = record.get("status", "unknown") + sc = colors.get(s, "white") + cli.console.print( + f"{indent}[{sc}]●[/{sc}] {key}: {s} " + f"[dim]({record.get('workflow_id', '?')})[/dim]" + ) + nested = record.get("workflow_scopes") + if isinstance(nested, dict) and nested: + _render_scopes(nested, indent + " ") + + @cli.workflow_app.command("status") def workflow_status( run_id: str | None = cli.typer.Argument( @@ -90,6 +113,10 @@ def workflow_status( s, "white" ) cli.console.print(f" [{sc}]●[/{sc}] {step_id}: {s}") + + if getattr(state, "workflow_scopes", None): + cli.console.print("\n [bold]Workflow scopes:[/bold]") + _render_scopes(state.workflow_scopes, " ") else: runs = engine.list_runs() diff --git a/src/specify_cli/workflows/composition.py b/src/specify_cli/workflows/composition.py new file mode 100644 index 0000000000..2e809d7975 --- /dev/null +++ b/src/specify_cli/workflows/composition.py @@ -0,0 +1,599 @@ +"""Workflow composition — scoped subtree execution helpers. + +Implements the decisions recorded in +``spec/workflow_composition/design_decisions.md``: + +- reserved output names and the composition depth limit, +- registry-backed, installed-and-enabled target resolution, +- strict input binding, +- declared-output evaluation, +- the internal ``ExecutionScope`` data model and persistence helpers. + +The engine special-cases ``type: workflow`` and owns the scope tree; custom +steps never see an ``ExecutionScope``. +""" + +from __future__ import annotations + +import re +from dataclasses import dataclass, field +from pathlib import Path +from typing import TYPE_CHECKING, Any + +from .base import RunStatus, StepContext +from .expressions import evaluate_expression + +if TYPE_CHECKING: + from .engine import RunState, WorkflowDefinition + +#: Engine metadata and run-control keys that cannot be declared as workflow +#: outputs. ``aborted`` controls run-abort behaviour; ``integration``, +#: ``model``, ``options`` and ``input`` are copied into persisted step +#: metadata by the engine; ``workflow``/``status``/``error`` are the stable +#: call metadata. +RESERVED_OUTPUT_NAMES: frozenset[str] = frozenset( + { + "workflow", + "status", + "error", + "aborted", + "integration", + "model", + "options", + "input", + } +) + +#: Maximum number of included-workflow levels. The root workflow is depth 0. +MAX_COMPOSITION_DEPTH = 16 + +#: Safe single-segment identifier: lowercase letters, digits, and hyphens. +_SAFE_NAME_PATTERN = re.compile(r"^[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$") + + +def _id_pattern() -> re.Pattern[str]: + """Return the engine's exact workflow-ID pattern (lazy import).""" + from .engine import _ID_PATTERN + + return _ID_PATTERN + + +def _reserved_workflow_ids() -> frozenset[str]: + """Return the reserved installed-workflow directory names (lazy import).""" + from .overlay.schema import _RESERVED_WORKFLOW_IDS + + return _RESERVED_WORKFLOW_IDS + + +# -- Target resolution ---------------------------------------------------- + + +def resolve_composed_workflow( + project_root: Path, workflow_id: str +) -> WorkflowDefinition: + """Resolve an installed, enabled workflow ID to its composed definition. + + Combines the registry existence/enabled checks that currently live in the + CLI with overlay resolution and validation. Raises ``ValueError`` (never + ``typer.Exit``) so a workflow step can surface the failure as a failed + step result. + """ + from .catalog import WorkflowRegistry + from .engine import validate_workflow + from .overlay import WorkflowResolver + + if not isinstance(workflow_id, str) or not workflow_id: + msg = "Workflow target must be a non-empty string." + raise ValueError(msg) + + registry = WorkflowRegistry(project_root) + metadata = registry.get(workflow_id) + if metadata is None: + msg = f"Workflow {workflow_id!r} is not installed." + raise ValueError(msg) + if not isinstance(metadata, dict): + msg = f"Registry entry for workflow {workflow_id!r} is corrupted." + raise ValueError(msg) + if not metadata.get("enabled", True): + msg = f"Workflow {workflow_id!r} is disabled." + raise ValueError(msg) + + definition = WorkflowResolver(project_root).resolve(workflow_id) + errors = validate_workflow(definition) + if errors: + msg = ( + f"Workflow {workflow_id!r} is invalid: " + " ".join(errors) + ) + raise ValueError(msg) + return definition + + +# -- Definition-time validation ------------------------------------------ + + +def validate_workflow_outputs(definition: WorkflowDefinition) -> list[str]: + """Validate a workflow's top-level ``outputs`` block.""" + errors: list[str] = [] + outputs = definition.outputs + if not isinstance(outputs, dict): + return ["'outputs' must be a mapping (or omitted)."] + for name, entry in outputs.items(): + if not isinstance(name, str) or not _SAFE_NAME_PATTERN.fullmatch(name): + errors.append( + f"Output {name!r} must be a safe identifier (lowercase " + "letters, digits, and hyphens)." + ) + continue + if name in RESERVED_OUTPUT_NAMES: + errors.append(f"Output {name!r} is a reserved name.") + continue + if not isinstance(entry, dict): + errors.append(f"Output {name!r} must be a mapping.") + continue + if set(entry.keys()) != {"value"}: + errors.append( + f"Output {name!r} must contain exactly the 'value' field." + ) + return errors + + +def validate_workflow_call_config(config: dict[str, Any]) -> list[str]: + """Validate a ``type: workflow`` step config (project-independent).""" + errors: list[str] = [] + step_id = config.get("id", "?") + target = config.get("workflow") + + if "workflow" not in config: + errors.append( + f"Workflow step {step_id!r} is missing 'workflow' field." + ) + elif not isinstance(target, str): + errors.append( + f"Workflow step {step_id!r}: 'workflow' must be a string, got " + f"{type(target).__name__}." + ) + elif "{{" not in target: + # A literal target must be a valid, non-reserved workflow ID. + if not _id_pattern().fullmatch(target): + errors.append( + f"Workflow step {step_id!r}: 'workflow' literal {target!r} " + "must be lowercase alphanumeric with hyphens." + ) + elif target in _reserved_workflow_ids(): + errors.append( + f"Workflow step {step_id!r}: 'workflow' literal {target!r} " + "is reserved." + ) + + input_mapping = config.get("input") + if input_mapping is not None and not isinstance(input_mapping, dict): + errors.append( + f"Workflow step {step_id!r}: 'input' must be a mapping." + ) + elif isinstance(input_mapping, dict): + for key in input_mapping: + if not isinstance(key, str): + errors.append( + f"Workflow step {step_id!r}: 'input' keys must be strings." + ) + return errors + + +# -- Input binding -------------------------------------------------------- + + +def evaluate_input_mapping( + mapping: Any, context: StepContext +) -> dict[str, Any]: + """Evaluate a caller's ``input`` mapping once in the caller scope.""" + if not isinstance(mapping, dict): + return {} + return { + name: evaluate_expression(value, context) + for name, value in mapping.items() + } + + +def bind_composed_inputs( + definition: WorkflowDefinition, + provided: dict[str, Any], + *, + caller_id: str, + workflow_id: str, + resolve_default: Any, +) -> dict[str, Any]: + """Strictly bind caller-supplied values to a target workflow's inputs. + + Unlike the low-level ``_resolve_inputs`` path, an undeclared mapped name is + rejected rather than silently discarded. ``resolve_default`` is the + engine's sentinel resolver (``WorkflowEngine._resolve_default``). + """ + from .engine import WorkflowEngine + + input_defs = definition.inputs if isinstance(definition.inputs, dict) else {} + + for name in provided: + if name not in input_defs: + msg = ( + f"Workflow step {caller_id!r} passed undeclared input {name!r} " + f"to workflow {workflow_id!r}." + ) + raise ValueError(msg) + + resolved: dict[str, Any] = {} + for name, input_def in input_defs.items(): + if not isinstance(input_def, dict): + continue + if name in provided: + value = resolve_default(name, provided[name]) + elif "default" in input_def: + value = resolve_default(name, input_def["default"]) + elif input_def.get("required", False): + msg = ( + f"Workflow step {caller_id!r} did not provide required input " + f"{name!r} for workflow {workflow_id!r}." + ) + raise ValueError(msg) + else: + continue + + coerce_input_def = input_def + if ( + name == "integration" + and value == "auto" + and isinstance(input_def.get("enum"), list) + ): + coerce_input_def = { + key: val for key, val in input_def.items() if key != "enum" + } + resolved[name] = WorkflowEngine._coerce_input( + name, value, coerce_input_def + ) + return resolved + + +# -- Output evaluation ---------------------------------------------------- + + +def evaluate_composed_outputs( + definition: WorkflowDefinition, scope: ExecutionScope +) -> dict[str, Any]: + """Evaluate a completed scope's declared outputs in its local context.""" + outputs = definition.outputs + if not isinstance(outputs, dict): + return {} + context = scope.build_context(is_resume=False) + result: dict[str, Any] = {} + for name, entry in outputs.items(): + if not isinstance(entry, dict) or "value" not in entry: + continue + result[name] = evaluate_expression(entry["value"], context) + return result + + +# -- Cycle and depth ------------------------------------------------------ + + +def check_composition_path(active_path: list[str], target: str) -> None: + """Reject a cyclic or too-deep composition entry. + + Cycle detection runs first so a recursive reference reports a cycle even + when the depth limit would also apply. + """ + if target in active_path: + chain = " -> ".join([*active_path, target]) + msg = f"Workflow composition cycle detected: {chain}." + raise ValueError(msg) + if len(active_path) > MAX_COMPOSITION_DEPTH: + chain = " -> ".join([*active_path, target]) + msg = ( + f"Workflow composition exceeds the maximum depth of " + f"{MAX_COMPOSITION_DEPTH}: {chain}." + ) + raise ValueError(msg) + + +# -- Execution scope ------------------------------------------------------ + + +@dataclass +class ExecutionScope: + """Runtime node in the composed execution tree. + + The root scope wraps a :class:`RunState`; nested scopes hang off it. Every + scope owns its local inputs, progress, step results, stable binding, and + nested scopes. Persistence always flows through the root's ``RunState``. + """ + + scope_id: str + workflow_id: str + definition: WorkflowDefinition | None = None + inputs: dict[str, Any] = field(default_factory=dict) + workflow_dir: str | None = None + step_results: dict[str, dict[str, Any]] = field(default_factory=dict) + current_step_index: int = 0 + current_step_id: str | None = None + status: RunStatus = RunStatus.RUNNING + error: str | None = None + workflow_scopes: dict[str, ExecutionScope] = field(default_factory=dict) + parent: ExecutionScope | None = None + root_state: RunState | None = None + # Runtime-only resume intent: root --input updates flow through reached, + # incomplete calls; an ordinary resume retains their persisted bindings. + rebind_inputs_on_resume: bool = False + + def root(self) -> ExecutionScope: + """Return the root scope of this tree.""" + node = self + while node.parent is not None: + node = node.parent + return node + + def _lock(self) -> Any: + state = self.root().root_state + return state._lock if state is not None else None + + def add_workflow_scope(self, key: str, child: ExecutionScope) -> None: + """Attach a nested scope under the run lock (concurrent fan-out safe).""" + lock = self._lock() + if lock is None: + self.workflow_scopes[key] = child + return + with lock: + self.workflow_scopes[key] = child + + def record_step_result(self, step_id: str, data: dict[str, Any]) -> None: + """Record one step result under the run lock.""" + lock = self._lock() + if lock is None: + self.step_results[step_id] = data + return + with lock: + self.step_results[step_id] = data + + def set_step_output(self, step_id: str, output: Any) -> None: + """Replace a recorded step's ``output`` under the run lock.""" + lock = self._lock() + if lock is None: + if step_id in self.step_results: + self.step_results[step_id]["output"] = output + return + with lock: + if step_id in self.step_results: + self.step_results[step_id]["output"] = output + + def append_log(self, entry: dict[str, Any]) -> None: + """Delegate logging to the root run state.""" + state = self.root().root_state + if state is not None: + state.append_log(entry) + + def build_context(self, *, is_resume: bool = False) -> StepContext: + """Build a ``StepContext`` scoped to this node.""" + root = self.root() + state = root.root_state + definition = self.definition + return StepContext( + inputs=self.inputs, + steps=self.step_results, + default_integration=( + definition.default_integration if definition is not None else None + ), + default_model=( + definition.default_model if definition is not None else None + ), + default_options=( + definition.default_options if definition is not None else {} + ), + project_root=str(state.project_root) if state is not None else None, + run_id=state.run_id if state is not None else None, + is_resume=is_resume, + workflow_dir=self.workflow_dir, + ) + + def _serialize(self) -> dict[str, Any]: + """Serialize this node and its descendants into plain JSON data.""" + return { + "workflow_id": self.workflow_id, + "invocation_id": self.scope_id, + "workflow_dir": self.workflow_dir, + "definition": ( + self.definition.data if self.definition is not None else {} + ), + "inputs": self.inputs, + "status": self.status.value, + "current_step_index": self.current_step_index, + "step_results": self.step_results, + "workflow_scopes": { + key: child._serialize() + for key, child in self.workflow_scopes.items() + }, + } + + def _sync_to_state(self, state: RunState) -> None: + """Copy the root scope's live fields into *state* (lock held).""" + state.status = self.status + state.error = self.error + state.current_step_id = self.current_step_id + state.current_step_index = self.current_step_index + state.step_results = self.step_results + state.workflow_scopes = { + key: child._serialize() + for key, child in self.workflow_scopes.items() + } + + def persist(self) -> None: + """Serialize the whole tree into the root state and save once.""" + root = self.root() + state = root.root_state + if state is None: + return + with state._lock: + root._sync_to_state(state) + state._save_locked() + + def record_and_save( + self, + context: StepContext, + step_id: str, + data: dict[str, Any], + *, + complete_child: bool = False, + ) -> None: + """Record a step result and complete its child in one locked write. + + Used for the workflow-call boundary so a persisted ``COMPLETED`` child + can never lack its caller-step result. + """ + root = self.root() + state = root.root_state + if state is None: + if complete_child and step_id in self.workflow_scopes: + self.workflow_scopes[step_id].status = RunStatus.COMPLETED + if context.steps is not self.step_results: + context.steps[step_id] = data + self.step_results[step_id] = data + return + with state._lock: + if complete_child and step_id in self.workflow_scopes: + self.workflow_scopes[step_id].status = RunStatus.COMPLETED + if context.steps is not self.step_results: + context.steps[step_id] = data + self.step_results[step_id] = data + root._sync_to_state(state) + state._save_locked() + + +def deserialize_scope( + record: dict[str, Any], + *, + parent: ExecutionScope | None, + root_state: RunState, +) -> ExecutionScope: + """Rebuild a runtime ``ExecutionScope`` from a persisted record.""" + from .engine import WorkflowDefinition + + definition = WorkflowDefinition(record.get("definition", {})) + scope = ExecutionScope( + scope_id=record.get("invocation_id", ""), + workflow_id=record.get("workflow_id", ""), + definition=definition, + inputs=record.get("inputs", {}) or {}, + workflow_dir=record.get("workflow_dir"), + step_results=record.get("step_results", {}) or {}, + current_step_index=record.get("current_step_index", 0), + status=RunStatus(record.get("status", RunStatus.RUNNING.value)), + parent=parent, + root_state=root_state, + ) + scope.workflow_scopes = { + key: deserialize_scope( + child, parent=scope, root_state=root_state + ) + for key, child in (record.get("workflow_scopes") or {}).items() + } + return scope + + +# -- Persisted-scope validation ------------------------------------------- + + +def validate_serialized_scopes(scopes: Any) -> None: + """Validate a persisted ``workflow_scopes`` tree. + + Raises ``ValueError`` on any malformed node so ``RunState.load`` can fail + closed, mirroring its existing validation style. + """ + from .engine import validate_workflow + + _validate_scope_tree(scopes, validate_workflow, path="workflow_scopes") + + +def _validate_scope_tree(scopes: Any, validate_workflow: Any, *, path: str) -> None: + if not isinstance(scopes, dict): + msg = f"Invalid run state: '{path}' must be a JSON object" + raise ValueError(msg) + for key, record in scopes.items(): + if not isinstance(key, str): + msg = f"Invalid run state: '{path}' keys must be strings" + raise ValueError(msg) + if not isinstance(record, dict): + msg = ( + f"Invalid run state: '{path}.{key}' must be a JSON object" + ) + raise ValueError(msg) + _validate_scope_record(record, validate_workflow, path=f"{path}.{key}") + + +def _validate_scope_record( + record: dict[str, Any], validate_workflow: Any, *, path: str +) -> None: + from .engine import WorkflowDefinition + + workflow_id = record.get("workflow_id") + if not isinstance(workflow_id, str) or not workflow_id: + msg = f"Invalid run state: '{path}.workflow_id' must be a non-empty string" + raise ValueError(msg) + + inputs = record.get("inputs", {}) + if not isinstance(inputs, dict): + msg = f"Invalid run state: '{path}.inputs' must be a JSON object" + raise ValueError(msg) + + step_results = record.get("step_results", {}) + if not isinstance(step_results, dict): + msg = f"Invalid run state: '{path}.step_results' must be a JSON object" + raise ValueError(msg) + for step_id, result in step_results.items(): + if not isinstance(result, dict): + msg = ( + f"Invalid run state: '{path}.step_results.{step_id}' must be " + "a JSON object" + ) + raise ValueError(msg) + + index = record.get("current_step_index", 0) + if isinstance(index, bool) or not isinstance(index, int) or index < 0: + msg = ( + f"Invalid run state: '{path}.current_step_index' must be a " + f"non-negative integer, got {index!r}" + ) + raise ValueError(msg) + + status = record.get("status", RunStatus.RUNNING.value) + try: + RunStatus(status) + except ValueError: + msg = f"Invalid run state: '{path}.status' is invalid: {status!r}" + raise ValueError(msg) from None + + definition = record.get("definition", {}) + if not isinstance(definition, dict): + msg = f"Invalid run state: '{path}.definition' must be a JSON object" + raise ValueError(msg) + errors = validate_workflow(WorkflowDefinition(definition)) + if errors: + msg = ( + f"Invalid run state: '{path}.definition' is invalid: " + + " ".join(errors) + ) + raise ValueError(msg) + + children = record.get("workflow_scopes", {}) + _validate_scope_tree(children, validate_workflow, path=f"{path}.workflow_scopes") + + +__all__ = [ + "MAX_COMPOSITION_DEPTH", + "RESERVED_OUTPUT_NAMES", + "ExecutionScope", + "bind_composed_inputs", + "check_composition_path", + "deserialize_scope", + "evaluate_composed_outputs", + "evaluate_input_mapping", + "resolve_composed_workflow", + "validate_serialized_scopes", + "validate_workflow_call_config", + "validate_workflow_outputs", +] diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index d81aae3212..b6a2a94043 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -29,7 +29,16 @@ try_read_integration_json, ) from .base import RunStatus, StepContext, StepResult, StepStatus - +from .composition import ( + ExecutionScope, + bind_composed_inputs, + check_composition_path, + deserialize_scope, + evaluate_composed_outputs, + evaluate_input_mapping, + validate_serialized_scopes, + validate_workflow_outputs, +) # -- Workflow Definition -------------------------------------------------- @@ -90,6 +99,10 @@ def __init__(self, data: dict[str, Any], source_path: Path | None = None) -> Non # Steps self.steps: list[dict[str, Any]] = data.get("steps", []) + # Declared outputs exposed to a caller when this workflow is composed + # into another via a ``type: workflow`` step. + self.outputs: dict[str, Any] = data.get("outputs", {}) + @classmethod def from_yaml(cls, path: Path) -> WorkflowDefinition: """Load a workflow definition from a YAML file.""" @@ -140,7 +153,7 @@ def _get_valid_step_types() -> set[str]: return set(STEP_REGISTRY.keys()) return { "command", "shell", "prompt", "gate", "if", "init", "slot", - "switch", "while", "do-while", "fan-out", "fan-in", + "switch", "while", "do-while", "fan-out", "fan-in", "workflow", } @@ -362,6 +375,12 @@ def validate_workflow(definition: WorkflowDefinition) -> list[str]: ) _validate_steps(definition.steps, seen_ids, errors, input_defs) + # -- Outputs ---------------------------------------------------------- + # Declared outputs are only meaningful when this workflow is composed into + # another, but the schema is validated unconditionally so an authoring + # mistake surfaces at install/validation time. + errors.extend(validate_workflow_outputs(definition)) + return errors @@ -695,6 +714,10 @@ def __init__( self.current_step_index = 0 self.current_step_id: str | None = None self.step_results: dict[str, dict[str, Any]] = {} + # Nested composition scopes, keyed by effective invocation id. The + # runtime tree lives in ``ExecutionScope`` objects; this is its + # serialized persistence form (see ``composition``). + self.workflow_scopes: dict[str, dict[str, Any]] = {} # Guards step_results mutation and save() so a concurrent fan-out cannot # mutate the dict while save() is serializing it (which would raise # "dictionary changed size during iteration"). @@ -745,29 +768,39 @@ def save(self) -> None: nor leave a reader observing a half-written file. Racing writers only contend to be last; they never corrupt. """ + with self._lock: + self._save_locked() + + def _save_locked(self) -> None: + """Serialize and write state; assumes ``self._lock`` is held. + + Split from :meth:`save` so the composed-execution helpers can update a + child scope and its caller's step result and then write once, without + re-acquiring the non-reentrant run lock. + """ runs_dir = self.runs_dir runs_dir.mkdir(parents=True, exist_ok=True) - with self._lock: - # Stamp updated_at inside the lock so the timestamp matches the - # snapshot this thread serializes (concurrent savers don't race it). - self.updated_at = datetime.now(timezone.utc).isoformat() - state_data = { - "run_id": self.run_id, - "workflow_id": self.workflow_id, - "installed_workflow_id": self.installed_workflow_id, - "installed_registry_root": self.installed_registry_root, - "status": self.status.value, - "current_step_index": self.current_step_index, - "current_step_id": self.current_step_id, - "step_results": self.step_results, - "workflow_dir": self.workflow_dir, - "created_at": self.created_at, - "updated_at": self.updated_at, - "error": self.error, - } - self._atomic_write_json(runs_dir / "state.json", state_data) - self._atomic_write_json(runs_dir / "inputs.json", {"inputs": self.inputs}) + # Stamp updated_at inside the lock so the timestamp matches the + # snapshot this thread serializes (concurrent savers don't race it). + self.updated_at = datetime.now(timezone.utc).isoformat() + state_data = { + "run_id": self.run_id, + "workflow_id": self.workflow_id, + "installed_workflow_id": self.installed_workflow_id, + "installed_registry_root": self.installed_registry_root, + "status": self.status.value, + "current_step_index": self.current_step_index, + "current_step_id": self.current_step_id, + "step_results": self.step_results, + "workflow_scopes": self.workflow_scopes, + "workflow_dir": self.workflow_dir, + "created_at": self.created_at, + "updated_at": self.updated_at, + "error": self.error, + } + self._atomic_write_json(runs_dir / "state.json", state_data) + self._atomic_write_json(runs_dir / "inputs.json", {"inputs": self.inputs}) @staticmethod def _atomic_write_json(path: Path, data: dict[str, Any]) -> None: @@ -861,6 +894,11 @@ def load(cls, run_id: str, project_root: Path) -> RunState: f"{step_id!r} must be a JSON object" ) + # Nested composition scopes. Older state files predate the field, so a + # missing key defaults to ``{}`` and those runs keep loading unchanged. + workflow_scopes = state_data.get("workflow_scopes", {}) + validate_serialized_scopes(workflow_scopes) + state = cls( run_id=state_data["run_id"], workflow_id=workflow_id, @@ -888,6 +926,7 @@ def load(cls, run_id: str, project_root: Path) -> RunState: state.current_step_index = current_step_index state.current_step_id = state_data.get("current_step_id") state.step_results = step_results + state.workflow_scopes = workflow_scopes state.workflow_dir = state_data.get("workflow_dir") state.created_at = state_data.get("created_at", "") state.updated_at = state_data.get("updated_at", "") @@ -1080,25 +1119,27 @@ def execute( workflow_dir=workflow_dir, ) + scope = self._build_root_scope(state, definition) + # Execute steps try: - self._execute_steps(definition.steps, context, state, STEP_REGISTRY) + self._execute_steps(definition.steps, context, scope, STEP_REGISTRY) except KeyboardInterrupt: - state.status = RunStatus.PAUSED - state.append_log({"event": "workflow_interrupted"}) - state.save() + scope.status = RunStatus.PAUSED + scope.append_log({"event": "workflow_interrupted"}) + scope.persist() return state except Exception as exc: - state.status = RunStatus.FAILED - state.error = str(exc) - state.append_log({"event": "workflow_failed", "error": str(exc)}) - state.save() + scope.status = RunStatus.FAILED + scope.error = str(exc) + scope.append_log({"event": "workflow_failed", "error": str(exc)}) + scope.persist() raise - if state.status == RunStatus.RUNNING: - state.status = RunStatus.COMPLETED - state.append_log({"event": "workflow_finished", "status": state.status.value}) - state.save() + if scope.status == RunStatus.RUNNING: + scope.status = RunStatus.COMPLETED + scope.append_log({"event": "workflow_finished", "status": scope.status.value}) + scope.persist() return state def resume( @@ -1173,6 +1214,9 @@ def resume( state.status = RunStatus.RUNNING state.save() + scope = self._build_root_scope(state, definition) + scope.rebind_inputs_on_resume = bool(inputs) + # Resume from the current step — re-execute it so gates # can prompt interactively again. remaining_steps = definition.steps[state.current_step_index :] @@ -1180,63 +1224,130 @@ def resume( try: self._execute_steps( - remaining_steps, context, state, STEP_REGISTRY, + remaining_steps, context, scope, STEP_REGISTRY, step_offset=step_offset, ) except KeyboardInterrupt: - state.status = RunStatus.PAUSED - state.append_log({"event": "workflow_interrupted"}) - state.save() + scope.status = RunStatus.PAUSED + scope.append_log({"event": "workflow_interrupted"}) + scope.persist() return state except Exception as exc: - state.status = RunStatus.FAILED - state.error = str(exc) - state.append_log({"event": "resume_failed", "error": str(exc)}) - state.save() + scope.status = RunStatus.FAILED + scope.error = str(exc) + scope.append_log({"event": "resume_failed", "error": str(exc)}) + scope.persist() raise - if state.status == RunStatus.RUNNING: - state.status = RunStatus.COMPLETED - state.append_log({"event": "workflow_finished", "status": state.status.value}) - state.save() + if scope.status == RunStatus.RUNNING: + scope.status = RunStatus.COMPLETED + scope.append_log({"event": "workflow_finished", "status": scope.status.value}) + scope.persist() return state @staticmethod def _record_result( - context: StepContext, state: RunState, step_id: str, data: dict[str, Any] + context: StepContext, scope: ExecutionScope, step_id: str, data: dict[str, Any] ) -> None: """Record a step result into both the live context and persistent state. - ``record_step_result`` writes ``state.step_results`` under the run lock. - On a resume run ``context.steps`` *is* that same dict, so that locked - write is the only one needed; mirror into ``context.steps`` separately - only when it is a distinct object (a fresh run), to avoid an unlocked - mutation of the shared dict that could race a concurrent ``save()``. + ``scope.record_step_result`` writes ``scope.step_results`` under the run + lock. On a resume run ``context.steps`` *is* that same dict, so that + locked write is the only one needed; mirror into ``context.steps`` + separately only when it is a distinct object (a fresh run), to avoid an + unlocked mutation of the shared dict that could race a concurrent + ``save()``. """ - if context.steps is not state.step_results: + if context.steps is not scope.step_results: context.steps[step_id] = data - state.record_step_result(step_id, data) + scope.record_step_result(step_id, data) + + def _build_root_scope( + self, state: RunState, definition: WorkflowDefinition + ) -> ExecutionScope: + """Build the root ``ExecutionScope`` wrapping *state*.""" + scope = ExecutionScope( + scope_id=definition.id, + workflow_id=definition.id, + definition=definition, + inputs=state.inputs, + workflow_dir=state.workflow_dir, + step_results=state.step_results, + current_step_index=state.current_step_index, + current_step_id=state.current_step_id, + status=state.status, + error=state.error, + root_state=state, + ) + scope.workflow_scopes = { + key: deserialize_scope(record, parent=scope, root_state=state) + for key, record in (state.workflow_scopes or {}).items() + } + return scope + + @staticmethod + def _scope_or_wrap(target: Any) -> tuple[ExecutionScope, RunState | None]: + """Accept an ``ExecutionScope`` or wrap a bare ``RunState``. + + The public entry points always pass an ``ExecutionScope``; this keeps + the historical private-method contract (a ``RunState``) working for + direct callers such as the fan-out concurrency tests. + """ + if isinstance(target, ExecutionScope): + return target, None + state: RunState = target + scope = ExecutionScope( + scope_id=state.workflow_id, + workflow_id=state.workflow_id, + inputs=state.inputs, + workflow_dir=state.workflow_dir, + step_results=state.step_results, + current_step_index=state.current_step_index, + current_step_id=state.current_step_id, + status=state.status, + error=state.error, + root_state=state, + ) + scope.workflow_scopes = { + key: deserialize_scope(record, parent=scope, root_state=state) + for key, record in (state.workflow_scopes or {}).items() + } + return scope, state + + @staticmethod + def _sync_wrapped(state: RunState | None, scope: ExecutionScope) -> None: + """Copy a wrapped scope's scalar fields back into its ``RunState``.""" + if state is None: + return + state.status = scope.status + state.error = scope.error + state.current_step_id = scope.current_step_id + state.current_step_index = scope.current_step_index + state.workflow_scopes = { + key: child._serialize() + for key, child in scope.workflow_scopes.items() + } def _execute_steps( self, steps: list[dict[str, Any]], context: StepContext, - state: RunState, + scope: ExecutionScope, registry: dict[str, Any], *, step_offset: int = 0, ) -> None: - """Execute a list of steps sequentially.""" + """Execute a list of steps sequentially within *scope*.""" for i, step_config in enumerate(steps): step_id = step_config.get("id", f"step-{i}") step_type = step_config.get("type", "command") - state.current_step_id = step_id + scope.current_step_id = step_id if step_offset >= 0: - state.current_step_index = step_offset + i - state.save() + scope.current_step_index = step_offset + i + scope.persist() - state.append_log( + scope.append_log( {"event": "step_started", "step_id": step_id, "type": step_type} ) @@ -1249,19 +1360,27 @@ def _execute_steps( step_impl = registry.get(step_type) if not step_impl: - state.status = RunStatus.FAILED - state.error = f"Unknown step type: {step_type!r}" - state.append_log( + scope.status = RunStatus.FAILED + scope.error = f"Unknown step type: {step_type!r}" + scope.append_log( { "event": "step_failed", "step_id": step_id, "error": f"Unknown step type: {step_type!r}", } ) - state.save() + scope.persist() return - result: StepResult = step_impl.execute(step_config, context) + # Workflow composition is an engine facility: run the included + # subtree now, before recording the caller's step result. A bound + # invocation must bypass caller-side target resolution on reentry. + if step_type == "workflow": + result: StepResult = self._run_workflow_call( + step_config, context, scope, registry, step_impl + ) + else: + result = step_impl.execute(step_config, context) # Record step results — prefer resolved values from step output step_data = { @@ -1285,9 +1404,18 @@ def _execute_steps( step_data["integration_options"] = result.output[ "integration_options" ] - self._record_result(context, state, step_id, step_data) + if step_type == "workflow": + # Commit the child's terminal status and the caller's step + # result in one locked, atomic write so a persisted completed + # child never lacks its caller result. + scope.record_and_save( + context, step_id, step_data, + complete_child=result.status == StepStatus.COMPLETED, + ) + else: + self._record_result(context, scope, step_id, step_data) - state.append_log( + scope.append_log( { "event": "step_completed", "step_id": step_id, @@ -1297,8 +1425,8 @@ def _execute_steps( # Handle gate pauses if result.status == StepStatus.PAUSED: - state.status = RunStatus.PAUSED - state.save() + scope.status = RunStatus.PAUSED + scope.persist() return # Handle failures @@ -1308,15 +1436,15 @@ def _execute_steps( # `continue_on_error` does NOT override them — that flag # is for transient/expected step failures only. if result.output.get("aborted"): - state.status = RunStatus.ABORTED - state.error = result.error - state.append_log( + scope.status = RunStatus.ABORTED + scope.error = result.error + scope.append_log( { "event": "workflow_aborted", "step_id": step_id, } ) - state.save() + scope.persist() return # `continue_on_error: true` lets the pipeline route @@ -1341,26 +1469,26 @@ def _execute_steps( # values like the string `"true"` silently change # run semantics. if step_config.get("continue_on_error") is True: - state.append_log( + scope.append_log( { "event": "step_continue_on_error", "step_id": step_id, "error": result.error, } ) - state.save() + scope.persist() continue - state.status = RunStatus.FAILED - state.error = result.error - state.append_log( + scope.status = RunStatus.FAILED + scope.error = result.error + scope.append_log( { "event": "step_failed", "step_id": step_id, "error": result.error, } ) - state.save() + scope.persist() return # Execute nested steps (from control flow) @@ -1371,10 +1499,10 @@ def _execute_steps( # enhancement. if result.next_steps: self._execute_steps( - result.next_steps, context, state, registry, + result.next_steps, context, scope, registry, step_offset=-1, ) - if state.status in ( + if scope.status in ( RunStatus.PAUSED, RunStatus.FAILED, RunStatus.ABORTED, @@ -1413,10 +1541,10 @@ def _execute_steps( base_id = orig or f"step-{ns_idx}" ns_copy["id"] = f"{step_id}:{base_id}:{_loop_iter + 1}" self._execute_steps( - [ns_copy], context, state, registry, + [ns_copy], context, scope, registry, step_offset=-1, ) - if state.status in ( + if scope.status in ( RunStatus.PAUSED, RunStatus.FAILED, RunStatus.ABORTED, @@ -1424,7 +1552,7 @@ def _execute_steps( return if orig and ns_copy["id"] in context.steps: self._record_result( - context, state, orig, + context, scope, orig, context.steps[ns_copy["id"]], ) @@ -1438,7 +1566,7 @@ def _execute_steps( template = result.output.get("step_template", {}) if template and items: fan_out_results = self._run_fan_out( - items, template, step_id, context, state, registry, + items, template, step_id, context, scope, registry, result.output.get("max_concurrency", 1), ) context.item = None @@ -1448,8 +1576,8 @@ def _execute_steps( # set_step_output updates the recorded dict under the run lock; # context.steps[step_id] is that same object, so it reflects the # change too — no separate (unlocked) context mutation needed. - state.set_step_output(step_id, fan_out_output) - if state.status in ( + scope.set_step_output(step_id, fan_out_output) + if scope.status in ( RunStatus.PAUSED, RunStatus.FAILED, RunStatus.ABORTED, @@ -1458,7 +1586,249 @@ def _execute_steps( else: # Empty items or no template — normalize output result.output["results"] = [] - state.set_step_output(step_id, result.output) + scope.set_step_output(step_id, result.output) + + def _active_workflow_path(self, scope: ExecutionScope) -> list[str]: + """Return the active workflow-ID path from the root to *scope*.""" + path: list[str] = [] + node: ExecutionScope | None = scope + while node is not None: + path.append(node.workflow_id) + node = node.parent + path.reverse() + return path + + def _bind_composed_inputs( + self, + step_config: dict[str, Any], + context: StepContext, + definition: WorkflowDefinition, + *, + caller_id: str, + provided: dict[str, Any] | None = None, + ) -> dict[str, Any]: + """Strictly bind a caller's input mapping to a target definition. + + ``provided`` may carry already-evaluated values (the initial call); when + omitted, the caller's ``input`` mapping is evaluated against the current + caller context (the resume path). + """ + if provided is None: + provided = evaluate_input_mapping( + step_config.get("input", {}), context + ) + return bind_composed_inputs( + definition, + provided, + caller_id=caller_id, + workflow_id=definition.id, + resolve_default=self._resolve_default, + ) + + def _run_workflow_call( + self, + step_config: dict[str, Any], + context: StepContext, + scope: ExecutionScope, + registry: dict[str, Any], + step_impl: Any, + ) -> StepResult: + """Execute (or resume) a ``type: workflow`` call in a nested scope.""" + effective_id = step_config.get("id", "workflow") + existing = scope.workflow_scopes.get(effective_id) + + if existing is not None and existing.status == RunStatus.COMPLETED: + recorded = scope.step_results.get(effective_id, {}) + output = recorded.get("output") + return StepResult( + status=StepStatus.COMPLETED, + output=dict(output) if isinstance(output, dict) else {}, + ) + + if existing is not None: + # Incomplete (PAUSED/FAILED): reuse the bound target and definition + # snapshot. Only explicit root input updates re-evaluate the + # caller's mapping; otherwise retain the persisted child binding. + child_scope = existing + definition = child_scope.definition + if definition is None: # pragma: no cover - defensive + return StepResult( + status=StepStatus.FAILED, + output={ + "workflow": child_scope.workflow_id, + "status": RunStatus.FAILED.value, + }, + error=( + f"Workflow step {effective_id!r}: persisted scope has " + "no definition snapshot." + ), + ) + if scope.root().rebind_inputs_on_resume: + try: + child_scope.inputs = self._bind_composed_inputs( + step_config, + context, + definition, + caller_id=effective_id, + ) + except ValueError as exc: + return StepResult( + status=StepStatus.FAILED, + output={ + "workflow": definition.id, + "status": RunStatus.FAILED.value, + }, + error=f"Workflow step {effective_id!r}: {exc}", + ) + child_scope.persist() + child_scope.status = RunStatus.RUNNING + child_scope.error = None + start = child_scope.current_step_index + child_context = child_scope.build_context(is_resume=True) + self._execute_steps( + definition.steps[start:], + child_context, + child_scope, + registry, + step_offset=start, + ) + return self._aggregate_workflow_result( + child_scope, definition, effective_id + ) + + # Only a new invocation resolves the target and its definition. The + # stored scope is authoritative for completed and incomplete calls. + resolved: StepResult = step_impl.execute(step_config, context) + if resolved.status != StepStatus.COMPLETED: + return resolved + + call = resolved.output + target_id = call.get("workflow") + definition = call.get("definition") + if not isinstance(target_id, str) or definition is None: + return StepResult( + status=StepStatus.FAILED, + output={ + "workflow": target_id, + "status": RunStatus.FAILED.value, + }, + error=( + f"Workflow step {effective_id!r}: unresolved workflow call." + ), + ) + + try: + check_composition_path( + self._active_workflow_path(scope), target_id + ) + except ValueError as exc: + return StepResult( + status=StepStatus.FAILED, + output={ + "workflow": target_id, + "status": RunStatus.FAILED.value, + }, + error=f"Workflow step {effective_id!r}: {exc}", + ) + + try: + bound_inputs = self._bind_composed_inputs( + step_config, + context, + definition, + caller_id=effective_id, + provided=call.get("inputs"), + ) + except ValueError as exc: + return StepResult( + status=StepStatus.FAILED, + output={ + "workflow": target_id, + "status": RunStatus.FAILED.value, + }, + error=f"Workflow step {effective_id!r}: {exc}", + ) + + child_scope = ExecutionScope( + scope_id=effective_id, + workflow_id=target_id, + definition=definition, + inputs=bound_inputs, + workflow_dir=call.get("workflow_dir"), + status=RunStatus.RUNNING, + parent=scope, + root_state=scope.root().root_state, + ) + scope.add_workflow_scope(effective_id, child_scope) + scope.persist() + + child_context = child_scope.build_context(is_resume=False) + self._execute_steps( + definition.steps, child_context, child_scope, registry, step_offset=0 + ) + return self._aggregate_workflow_result( + child_scope, definition, effective_id + ) + + def _aggregate_workflow_result( + self, + child_scope: ExecutionScope, + definition: WorkflowDefinition, + effective_id: str, + ) -> StepResult: + """Map an included scope's terminal status to a caller step result.""" + status = child_scope.status + # A successfully exhausted subtree stays RUNNING until the caller's + # result and its COMPLETED status are committed together under the lock. + if status in (RunStatus.RUNNING, RunStatus.COMPLETED): + output: dict[str, Any] = { + "workflow": definition.id, + "status": RunStatus.COMPLETED.value, + } + try: + output.update(evaluate_composed_outputs(definition, child_scope)) + except Exception as exc: # noqa: BLE001 - expression failures are step failures + error = ( + f"Workflow step {effective_id!r}: failed to evaluate outputs " + f"for workflow {definition.id!r}: {exc}" + ) + child_scope.status = RunStatus.FAILED + child_scope.error = error + return StepResult( + status=StepStatus.FAILED, + output={ + "workflow": definition.id, + "status": RunStatus.FAILED.value, + }, + error=error, + ) + return StepResult(status=StepStatus.COMPLETED, output=output) + if status == RunStatus.PAUSED: + return StepResult( + status=StepStatus.PAUSED, + output={ + "workflow": definition.id, + "status": RunStatus.PAUSED.value, + }, + ) + if status == RunStatus.ABORTED: + return StepResult( + status=StepStatus.FAILED, + output={ + "workflow": definition.id, + "status": RunStatus.FAILED.value, + "aborted": True, + }, + error=child_scope.error, + ) + return StepResult( + status=StepStatus.FAILED, + output={ + "workflow": definition.id, + "status": RunStatus.FAILED.value, + }, + error=child_scope.error, + ) def _run_fan_out( self, @@ -1466,7 +1836,7 @@ def _run_fan_out( template: dict[str, Any], step_id: str, context: StepContext, - state: RunState, + scope: ExecutionScope, registry: dict[str, Any], max_concurrency: Any, ) -> list[Any]: @@ -1492,7 +1862,9 @@ def _run_fan_out( coerces to <= 1 runs sequentially, while a numeric string like ``"4"`` or a float like ``4.0`` is honored. """ + scope, wrap_state = self._scope_or_wrap(scope) if not items: + self._sync_wrapped(wrap_state, scope) return [] halting = (RunStatus.PAUSED, RunStatus.FAILED, RunStatus.ABORTED) @@ -1516,7 +1888,7 @@ def run_item(idx: int, item_ctx: StepContext) -> Any: item_step = dict(template) item_step["id"] = item_id(idx) self._execute_steps( - [item_step], item_ctx, state, registry, step_offset=-1, + [item_step], item_ctx, scope, registry, step_offset=-1, ) # Read back through the context that was actually executed against, # not the outer closure — clearer and robust if StepContext copying @@ -1533,11 +1905,12 @@ def run_item(idx: int, item_ctx: StepContext) -> Any: for item_idx, item_val in enumerate(items): context.item = item_val results.append(run_item(item_idx, context)) - if state.status in halting: + if scope.status in halting: break finally: context.item = previous_item context.inside_fan_out = previous_inside_fan_out + self._sync_wrapped(wrap_state, scope) return results # Concurrent path — bounded sliding window; results assembled in item order. @@ -1570,7 +1943,7 @@ def item_halt_status(idx: int) -> RunStatus | None: # record_step_result (e.g. an unknown step type returns early). # Every item runs the same template, so the shared run status is # this item's own outcome; attribute the halt to it. - return state.status if state.status in halting else None + return scope.status if scope.status in halting else None status = rec.get("status") if status == StepStatus.PAUSED.value: return RunStatus.PAUSED @@ -1596,7 +1969,7 @@ def item_halt_status(idx: int) -> RunStatus | None: while ( next_submit < n and len(futures) < workers - and state.status not in halting + and scope.status not in halting ): futures[next_submit] = pool.submit(run_isolated, next_submit) next_submit += 1 @@ -1632,21 +2005,23 @@ def item_halt_status(idx: int) -> RunStatus | None: if halt is not None: halted_at, halted_status = halt - # A later in-flight item may have overwritten state.status before the + # A later in-flight item may have overwritten scope.status before the # pool joined; restore the halting item's own outcome so the final run # status matches the sequential semantics. - state.status = halted_status + scope.status = halted_status # Restore the halting item's error so it matches the terminal - # status — a concurrent item may have overwritten state.error + # status — a concurrent item may have overwritten scope.error # before the pool joined. Assign unconditionally when a record # exists (even when the halting item's own error is falsy) so a # third-party step returning FAILED with no message never inherits # an unrelated concurrent item's error; this mirrors the sequential - # path, which sets state.error = result.error verbatim. + # path, which sets scope.error = result.error verbatim. halt_rec = context.steps.get(item_id(halted_at)) if isinstance(halt_rec, dict): - state.error = halt_rec.get("error") + scope.error = halt_rec.get("error") + self._sync_wrapped(wrap_state, scope) return slots[: halted_at + 1] + self._sync_wrapped(wrap_state, scope) return slots[:collected] def _resolve_inputs( 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/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 7a3a3b8cad..5738533e1a 100644 --- a/src/specify_cli/workflows/step/command_add.py +++ b/src/specify_cli/workflows/step/command_add.py @@ -1,109 +1,327 @@ -"""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 +_MAX_STEP_CATALOG_RESPONSE_BYTES = 50 * 1024 * 1024 -@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, +def _cleanup_download_tmp_path(tmp_path: cli.Path | None) -> None: + """Best-effort unlink of a partially-downloaded step archive temp file. + + 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( + "[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." ) - 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 + extract_tmp: tempfile.TemporaryDirectory[str] | None = None + committed = False + 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 + ) + 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, + 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) + + 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=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) + entry = installer.install_step_package( + project_root, + step_id, + package_root, + 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: + 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 +344,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 @@ -144,233 +361,152 @@ 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) - - 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) + return cli.read_response_limited( + resp, + max_bytes=_MAX_STEP_CATALOG_RESPONSE_BYTES, + error_type=ValueError, + label="step package response", + ) - # 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) + 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) 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" - ) - 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})" + 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) - # 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: + raise installer.StepInstallError( + f"Failed to write extra file '{rel_path}': {exc}" + ) from exc + + entry = installer.install_step_package( + project_root, + step_id, + package_dir, + source="catalog", + catalog_name=info.get("_catalog_name", ""), + catalog_metadata=info, + force=force, + ) + committed = True + finally: + try: + package_tmp.cleanup() + except OSError as cleanup_exc: + if committed: cli.console.print( - f"[red]Error:[/red] Failed to write extra file '{rel_path}': {exc}" + "[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)})" ) - raise cli.typer.Exit(1) + elif __import__("sys").exc_info()[0] is None: + raise installer.StepInstallError( + "Failed to remove temporary step package directory: " + f"{cleanup_exc}" + ) from cleanup_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) + _print_installed(step_id, entry) - step_name = info.get("name") or step_id - step_version = info.get("version") or step_meta.get("version") or "0.0.0" - # 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/command_remove.py b/src/specify_cli/workflows/step/command_remove.py index 58ec7602f3..76421b2321 100644 --- a/src/specify_cli/workflows/step/command_remove.py +++ b/src/specify_cli/workflows/step/command_remove.py @@ -3,9 +3,8 @@ from __future__ import annotations from .. import _commands as cli -from . import step_app - from . import _helpers as step_helpers +from . import step_app @step_app.command("remove") @@ -13,85 +12,31 @@ 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 + import shutil + + from . import installer project_root = cli._require_specify_project() step_helpers._validate_step_id_or_exit(step_id) - registry = StepRegistry(project_root) - in_registry = registry.is_installed(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: even though step_helpers._validate_step_id_or_exit rejects path - # separators, ensure that the resolved directory is a single child of - # steps_base_dir and is not steps_base_dir itself. 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) - - 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") - raise cli.typer.Exit(1) - - if not in_registry and dir_exists: - # The registry was likely reset due to corruption. Warn the user that the - # directory is being removed even though there is no registry entry, so - # the orphaned package can be cleaned up and a fresh install attempted. + staged_dir, removed_orphan = installer.remove_step_package(project_root, step_id) + except installer.StepInstallError as exc: + cli.console.print(f"[red]Error:[/red] {exc}") + raise cli.typer.Exit(1) from exc + if removed_orphan: cli.console.print( - f"[yellow]Warning:[/yellow] '{step_id}' has no registry entry " - "(registry may have been reset). Removing the orphaned directory." + f"[yellow]Warning:[/yellow] '{cli._escape_markup(step_id)}' had no registry " + "entry. Removing the orphaned directory." ) - - if dir_exists and not in_registry: - # No registry write needed; just delete the orphaned directory. - import shutil - + cli.console.print(f"[green]✓[/green] Step type '{cli._escape_markup(step_id)}' uninstalled") + if staged_dir is not None: try: - shutil.rmtree(step_dir) + shutil.rmtree(staged_dir) except OSError as exc: cli.console.print( - f"[red]Error:[/red] Failed to remove step directory {step_dir}: {exc}" + "[yellow]Warning:[/yellow] Step was uninstalled, but its staged " + f"directory could not be deleted: {cli._escape_markup(str(exc))}. " + f"Remove it manually: {cli._escape_markup(str(staged_dir))}" ) - raise cli.typer.Exit(1) - elif in_registry: - # Remove the registry entry, then the directory. If the directory - # delete fails, restore the registry entry so state stays consistent - # and a future `step add` isn't blocked by an orphaned directory - # with no registry entry. - registry_metadata = registry.get(step_id) - try: - registry.remove(step_id) - except StepValidationError as exc: - 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: - # Restore the original registry entry verbatim (bypass add() - # which would overwrite timestamps). - try: - if registry_metadata is not None: - registry.data["steps"][step_id] = registry_metadata - registry.save() - 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}" - ) - cli.console.print( - f"[red]Error:[/red] Failed to remove step directory {step_dir}: {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 new file mode 100644 index 0000000000..5c1a2efbc1 --- /dev/null +++ b/src/specify_cli/workflows/step/installer.py @@ -0,0 +1,827 @@ +"""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 contextlib +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('<>:"|?*') +_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 +# --------------------------------------------------------------------------- + + +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]: + 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_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_values["author"] + if catalog_values["author"] is not None + else package_values["author"] or "" + ), + "type_key": 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): + _copy_regular_file(entry.path, target, mode) + else: + raise StepInstallError( + f"Step package contains unsupported file: {entry.path}" + ) + + _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, + 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(): + 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: + 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. + 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 + + +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) + + # 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, + validate_step_package(package_dir, step_id), + 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" + 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. + staged_meta = validate_step_package(staged_dir, step_id) + entry = _build_entry( + step_id, + 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: + 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 + + +def remove_step_package(project_root: Path, step_id: str) -> tuple[Path | None, bool]: + """Remove one custom step under the same transaction as installation. + + The returned directory is staged after the registry removal has committed; + the second value identifies a removed orphan. Callers can delete the staged + directory best-effort without changing the successful result. + """ + from .catalog import StepRegistry, StepValidationError + + validate_step_id(step_id) + with _step_install_transaction(project_root): + steps_base_dir = resolve_steps_base_dir(project_root) + step_dir = _resolve_step_dir(steps_base_dir, step_id) + _reject_unsafe_destination(step_dir) + registry = StepRegistry(project_root) + in_registry = registry.is_installed(step_id) + if not in_registry and not step_dir.exists(): + raise StepInstallError(f"Step type '{step_id}' is not installed") + + staged_dir: Path | None = None + if step_dir.exists(): + try: + staged_dir = Path( + tempfile.mkdtemp( + prefix=f".{step_id}.removing-", dir=steps_base_dir + ) + ) + staged_dir.rmdir() + os.replace(step_dir, staged_dir) + except OSError as exc: + raise StepInstallError( + f"Failed to stage step directory '{step_dir}' for removal: {exc}" + ) from exc + + if not in_registry: + return staged_dir, True + try: + registry.remove(step_id) + except (StepValidationError, OSError, TypeError, ValueError) as exc: + if staged_dir is not None: + try: + os.replace(staged_dir, step_dir) + except OSError as restore_exc: + raise StepInstallError( + f"Failed to update the step registry for '{step_id}': {exc}. " + f"The package remains staged at '{staged_dir}' because restore " + f"failed: {restore_exc}." + ) from exc + raise StepInstallError( + f"Failed to update the step registry for '{step_id}': {exc}" + ) from exc + return staged_dir, False diff --git a/src/specify_cli/workflows/step/workflow/__init__.py b/src/specify_cli/workflows/step/workflow/__init__.py new file mode 100644 index 0000000000..e9306b99ee --- /dev/null +++ b/src/specify_cli/workflows/step/workflow/__init__.py @@ -0,0 +1,104 @@ +"""Workflow step — execute an installed workflow as a scoped subtree. + +The step itself performs caller-side resolution only: it evaluates the +``workflow`` target expression, resolves the installed/enabled target +definition, and evaluates the input mapping. The engine special-cases +``type: workflow`` and runs the resulting subtree in a nested +``ExecutionScope``. +""" + +from __future__ import annotations + +from pathlib import Path +from typing import Any + +from specify_cli.workflows.base import StepBase, StepContext, StepResult, StepStatus +from specify_cli.workflows.composition import ( + evaluate_input_mapping, + resolve_composed_workflow, + validate_workflow_call_config, +) +from specify_cli.workflows.expressions import evaluate_expression + + +class WorkflowStep(StepBase): + """Compose an installed workflow into the current run.""" + + type_key = "workflow" + + def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: + step_id = config.get("id", "?") + try: + from specify_cli.workflows.engine import _ID_PATTERN + from specify_cli.workflows.overlay.schema import ( + _RESERVED_WORKFLOW_IDS, + ) + + target_expr = config.get("workflow") + if not isinstance(target_expr, str): + return StepResult( + status=StepStatus.FAILED, + output={"workflow": target_expr, "status": StepStatus.FAILED.value}, + error=( + f"Workflow step {step_id!r}: 'workflow' must be a string." + ), + ) + + target = evaluate_expression(target_expr, context) + if not isinstance(target, str): + return StepResult( + status=StepStatus.FAILED, + output={"workflow": target, "status": StepStatus.FAILED.value}, + error=( + f"Workflow step {step_id!r}: 'workflow' expression " + f"resolved to {type(target).__name__}, expected a string." + ), + ) + if ( + not _ID_PATTERN.fullmatch(target) + or target in _RESERVED_WORKFLOW_IDS + ): + return StepResult( + status=StepStatus.FAILED, + output={"workflow": target, "status": StepStatus.FAILED.value}, + error=( + f"Workflow step {step_id!r}: {target!r} is not a valid " + "workflow ID." + ), + ) + + project_root = ( + Path(context.project_root) if context.project_root else Path(".") + ) + definition = resolve_composed_workflow(project_root, target) + raw_inputs = evaluate_input_mapping(config.get("input", {}), context) + workflow_dir = ( + str(definition.source_path.resolve().parent) + if definition.source_path is not None + else None + ) + return StepResult( + status=StepStatus.COMPLETED, + output={ + "workflow": target, + "definition": definition, + "inputs": raw_inputs, + "workflow_dir": workflow_dir, + }, + ) + except Exception as exc: # noqa: BLE001 + # Runtime resolution failures become a failed step result so the + # caller's normal continue_on_error handling applies. + return StepResult( + status=StepStatus.FAILED, + output={ + "workflow": config.get("workflow"), + "status": StepStatus.FAILED.value, + }, + error=f"Workflow step {step_id!r}: {exc}", + ) + + def validate(self, config: dict[str, Any]) -> list[str]: + errors = super().validate(config) + errors.extend(validate_workflow_call_config(config)) + return errors 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/bundles/test_references.py b/tests/specify_cli/bundles/test_references.py index a020d64a9d..b9351a3449 100644 --- a/tests/specify_cli/bundles/test_references.py +++ b/tests/specify_cli/bundles/test_references.py @@ -27,7 +27,7 @@ def test_bundled_extension_resolves(tmp_path: Path): def test_builtin_step_type_resolves(tmp_path: Path): """A built-in step type must resolve, like a bundled extension. - Spec Kit ships 12 step types as built-ins registered in ``STEP_REGISTRY`` + Spec Kit ships 13 step types as built-ins registered in ``STEP_REGISTRY`` rather than as on-disk asset directories, so there is no ``_locate_bundled_step``. The ``steps`` branch of ``_resolved_locally`` only asked ``StepRegistry(root).is_installed()``, which tracks *community* step 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 e19c53c6b1..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( @@ -325,11 +327,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 +373,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", @@ -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) @@ -604,3 +609,634 @@ 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_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") + 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..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 @@ -61,3 +60,49 @@ 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_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_remove.py b/tests/specify_cli/workflows/step/test_command_remove.py index a6bd5781c0..5e902b733d 100644 --- a/tests/specify_cli/workflows/step/test_command_remove.py +++ b/tests/specify_cli/workflows/step/test_command_remove.py @@ -7,7 +7,6 @@ import pytest - class TestWorkflowStepRemoveCLI: """Test the 'specify workflow step remove' CLI command edge cases.""" @@ -17,6 +16,7 @@ def test_remove_orphaned_directory(self, project_dir, monkeypatch): This covers the case where the registry was reset due to corruption. """ from typer.testing import CliRunner + from specify_cli import app monkeypatch.chdir(project_dir) @@ -40,6 +40,7 @@ def test_remove_orphaned_directory(self, project_dir, monkeypatch): def test_remove_not_installed(self, project_dir, monkeypatch): """step remove fails cleanly when neither directory nor registry entry exist.""" from typer.testing import CliRunner + from specify_cli import app monkeypatch.chdir(project_dir) @@ -53,6 +54,7 @@ def test_remove_not_installed(self, project_dir, monkeypatch): def test_remove_registered_step(self, project_dir, monkeypatch): """step remove works normally when both directory and registry entry exist.""" from typer.testing import CliRunner + from specify_cli import app from specify_cli.workflows.step.catalog import StepRegistry @@ -79,6 +81,7 @@ def test_remove_registered_step(self, project_dir, monkeypatch): @pytest.mark.skipif(not hasattr(os, "symlink"), reason="symlinks are unavailable") def test_remove_rejects_symlinked_steps_base_dir(self, project_dir, monkeypatch): from typer.testing import CliRunner + from specify_cli import app monkeypatch.chdir(project_dir) 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 new file mode 100644 index 0000000000..18fbfd417d --- /dev/null +++ b/tests/specify_cli/workflows/step/test_installer.py @@ -0,0 +1,674 @@ +"""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_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 +): + 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 + ) 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 diff --git a/tests/test_workflows.py b/tests/test_workflows.py index d8abcc0f55..bfc1eda3d8 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -4,7 +4,7 @@ - Step registry & auto-discovery - Base classes (StepBase, StepContext, StepResult) - Expression engine -- All 12 built-in step types +- All 13 built-in step types - Workflow definition loading & validation - Workflow engine execution & state persistence - Workflow catalog & registry @@ -107,6 +107,7 @@ def test_all_step_types_registered(self): expected = { "command", "shell", "prompt", "gate", "if", "switch", "while", "do-while", "fan-out", "fan-in", "init", "slot", + "workflow", } assert expected.issubset(set(STEP_REGISTRY.keys())) diff --git a/tests/workflows/test_workflow_composition.py b/tests/workflows/test_workflow_composition.py new file mode 100644 index 0000000000..e7c9f013ba --- /dev/null +++ b/tests/workflows/test_workflow_composition.py @@ -0,0 +1,1379 @@ +"""Tests for workflow composition (the built-in ``type: workflow`` step). + +Covers the composition helpers, engine scoped execution, strict input binding, +persistence/resume, and CLI reporting. See +``spec/workflow_composition/implementation_plan.md``. +""" + +from __future__ import annotations + +import json +from pathlib import Path +from threading import Event + +import pytest +import yaml + +from specify_cli.workflows.base import RunStatus +from specify_cli.workflows.composition import ( + MAX_COMPOSITION_DEPTH, + RESERVED_OUTPUT_NAMES, + bind_composed_inputs, + check_composition_path, + validate_workflow_call_config, +) +from specify_cli.workflows.engine import ( + RunState, + WorkflowDefinition, + WorkflowEngine, + validate_workflow, +) + +# -- Helpers -------------------------------------------------------------- + + +def _workflow( + workflow_id: str, + steps: list[dict], + *, + inputs: dict | None = None, + outputs: dict | None = None, + name: str | None = None, +) -> dict: + data: dict = { + "schema_version": "1.0", + "workflow": { + "id": workflow_id, + "name": name or workflow_id.title(), + "version": "1.0.0", + }, + "steps": steps, + } + if inputs is not None: + data["inputs"] = inputs + if outputs is not None: + data["outputs"] = outputs + return data + + +def _install(project_root: Path, workflow_id: str, data: dict, *, enabled: bool = True) -> Path: + from specify_cli.workflows.catalog import WorkflowRegistry + + workflow_dir = project_root / ".specify" / "workflows" / workflow_id + workflow_dir.mkdir(parents=True, exist_ok=True) + path = workflow_dir / "workflow.yml" + path.write_text(yaml.safe_dump(data), encoding="utf-8") + WorkflowRegistry(project_root).add( + workflow_id, + { + "name": data["workflow"]["name"], + "version": "1.0.0", + "enabled": enabled, + }, + ) + return path + + +def _definition(project_root: Path, workflow_id: str) -> WorkflowDefinition: + return WorkflowDefinition.from_yaml( + project_root / ".specify" / "workflows" / workflow_id / "workflow.yml" + ) + + +def _run(project_root: Path, workflow_id: str, inputs: dict | None = None) -> RunState: + engine = WorkflowEngine(project_root) + return engine.execute(_definition(project_root, workflow_id), inputs or {}) + + +def _shell(step_id: str, run: str, **extra) -> dict: + return {"id": step_id, "type": "shell", "run": run, **extra} + + +# -- Composition helper unit tests --------------------------------------- + + +class TestWorkflowOutputsValidation: + def _errors(self, outputs) -> list[str]: + definition = WorkflowDefinition( + _workflow("w", [_shell("s", "echo")], outputs=outputs) + ) + return validate_workflow(definition) + + def test_safe_output_names_accepted(self): + errors = self._errors({"result": {"value": "{{ steps.s.output.stdout }}"}}) + assert errors == [] + + @pytest.mark.parametrize("name", sorted(RESERVED_OUTPUT_NAMES)) + def test_reserved_output_names_rejected(self, name): + errors = self._errors({name: {"value": "x"}}) + assert any("reserved" in e for e in errors), errors + + def test_non_mapping_outputs_rejected(self): + errors = self._errors([{"value": "x"}]) + assert any("'outputs' must be a mapping" in e for e in errors) + + def test_entry_missing_value_rejected(self): + errors = self._errors({"result": {"expr": "x"}}) + assert any("exactly the 'value' field" in e for e in errors) + + def test_entry_extra_keys_rejected(self): + errors = self._errors({"result": {"value": "x", "extra": 1}}) + assert any("exactly the 'value' field" in e for e in errors) + + def test_bad_output_name_rejected(self): + errors = self._errors({"Bad Name": {"value": "x"}}) + assert any("safe identifier" in e for e in errors) + + def test_non_mapping_entry_rejected(self): + errors = self._errors({"result": "x"}) + assert any("must be a mapping" in e for e in errors) + + +class TestWorkflowCallConfigValidation: + def test_literal_valid(self): + assert validate_workflow_call_config({"id": "s", "workflow": "bugfix"}) == [] + + def test_missing_workflow(self): + errors = validate_workflow_call_config({"id": "s"}) + assert any("missing 'workflow'" in e for e in errors) + + def test_non_string_workflow(self): + errors = validate_workflow_call_config({"id": "s", "workflow": 5}) + assert any("must be a string" in e for e in errors) + + def test_invalid_literal_id(self): + errors = validate_workflow_call_config({"id": "s", "workflow": "Bad_ID"}) + assert any("lowercase alphanumeric" in e for e in errors) + + def test_reserved_literal_id(self): + errors = validate_workflow_call_config({"id": "s", "workflow": "runs"}) + assert any("reserved" in e for e in errors) + + def test_expression_target_allowed(self): + errors = validate_workflow_call_config( + {"id": "s", "workflow": "{{ steps.pick.output.stdout }}"} + ) + assert errors == [] + + def test_non_mapping_input_rejected(self): + errors = validate_workflow_call_config( + {"id": "s", "workflow": "bugfix", "input": ["x"]} + ) + assert any("'input' must be a mapping" in e for e in errors) + + +class TestCheckCompositionPath: + def test_cycle_reported_before_depth(self): + path = [f"w{i}" for i in range(MAX_COMPOSITION_DEPTH + 5)] + ["target"] + with pytest.raises(ValueError, match="cycle"): + check_composition_path(path, "target") + + def test_depth_16_allowed(self): + path = [f"w{i}" for i in range(MAX_COMPOSITION_DEPTH)] + check_composition_path(path, "new") + + def test_depth_17_rejected(self): + path = [f"w{i}" for i in range(MAX_COMPOSITION_DEPTH + 1)] + with pytest.raises(ValueError, match="maximum depth"): + check_composition_path(path, "new") + + def test_diamond_allowed(self): + # A -> B -> D and A -> C -> D: D is not in the A->B path. + check_composition_path(["a", "b"], "d") + check_composition_path(["a", "c"], "d") + + +class TestStrictInputBinding: + def _bind(self, definition, provided, resolve_default=lambda n, v: v): + return bind_composed_inputs( + definition, + provided, + caller_id="call", + workflow_id=definition.id, + resolve_default=resolve_default, + ) + + def _def(self, inputs): + return WorkflowDefinition(_workflow("child", [_shell("s", "echo")], inputs=inputs)) + + def test_undeclared_input_rejected(self): + definition = self._def({"who": {"type": "string"}}) + with pytest.raises(ValueError, match="undeclared input"): + self._bind(definition, {"typo": "x"}) + + def test_defaults_applied(self): + definition = self._def({"who": {"type": "string", "default": "world"}}) + assert self._bind(definition, {}) == {"who": "world"} + + def test_required_missing_rejected(self): + definition = self._def({"who": {"type": "string", "required": True}}) + with pytest.raises(ValueError, match="required input"): + self._bind(definition, {}) + + def test_enum_enforced(self): + definition = self._def( + {"mode": {"type": "string", "enum": ["a", "b"], "default": "a"}} + ) + assert self._bind(definition, {"mode": "b"}) == {"mode": "b"} + with pytest.raises(ValueError, match="not in allowed values"): + self._bind(definition, {"mode": "c"}) + + def test_integration_auto_sentinel(self): + definition = self._def( + {"integration": {"type": "string", "default": "auto", "enum": ["claude"]}} + ) + + def resolve_default(name, value): + return "claude" if name == "integration" and value == "auto" else value + + assert self._bind(definition, {}, resolve_default) == {"integration": "claude"} + + +# -- Engine composition tests -------------------------------------------- + + +class TestLiteralAndRuntimeTargets: + def test_literal_target(self, project_dir): + _install(project_dir, "child", _workflow("child", [_shell("x", "echo hi")])) + _install( + project_dir, + "parent", + _workflow( + "parent", + [{"id": "call", "type": "workflow", "workflow": "child"}], + ), + ) + state = _run(project_dir, "parent") + assert state.status == RunStatus.COMPLETED + assert state.step_results["call"]["output"]["workflow"] == "child" + + def test_runtime_selected_target(self, project_dir): + _install(project_dir, "child", _workflow("child", [_shell("x", "echo hi")])) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + _shell("pick", "printf child"), + { + "id": "call", + "type": "workflow", + "workflow": "{{ steps.pick.output.stdout }}", + }, + ], + ), + ) + state = _run(project_dir, "parent") + assert state.status == RunStatus.COMPLETED + assert state.step_results["call"]["output"]["workflow"] == "child" + + +class TestScopeIsolation: + def _parent_and_child(self, project_dir): + _install( + project_dir, + "child", + _workflow( + "child", + [ + _shell("child-local", "echo {{ inputs.declared }}"), + _shell("peek-input", "echo {{ inputs.shared | default('MISSING') }}"), + _shell( + "peek-step", + "echo {{ steps.caller-step.output.stdout | default('NOPE') }}", + ), + ], + inputs={"declared": {"type": "string", "default": "d"}}, + outputs={"echoed": {"value": "{{ steps.child-local.output.stdout }}"}}, + ), + ) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + _shell("caller-step", "echo caller-value"), + { + "id": "call", + "type": "workflow", + "workflow": "child", + "input": {"declared": "{{ inputs.shared }}"}, + }, + ], + inputs={"shared": {"type": "string", "default": "secret"}}, + ), + ) + + def test_child_cannot_see_caller_locals(self, project_dir): + self._parent_and_child(project_dir) + state = _run(project_dir, "parent") + child = state.workflow_scopes["call"] + assert child["step_results"]["peek-input"]["output"]["stdout"].strip() == "MISSING" + assert child["step_results"]["peek-step"]["output"]["stdout"].strip() == "NOPE" + + def test_caller_cannot_see_child_locals(self, project_dir): + self._parent_and_child(project_dir) + state = _run(project_dir, "parent") + assert "child-local" not in state.step_results + call_output = state.step_results["call"]["output"] + assert call_output["echoed"].strip() == "secret" + # Only declared outputs + stable metadata cross the boundary. + assert set(call_output) == {"workflow", "status", "echoed"} + + +class TestPublicOutputShapes: + def test_completed_shape(self, project_dir): + _install(project_dir, "child", _workflow("child", [_shell("x", "echo hi")])) + _install( + project_dir, + "parent", + _workflow("parent", [{"id": "c", "type": "workflow", "workflow": "child"}]), + ) + out = _run(project_dir, "parent").step_results["c"]["output"] + assert out["status"] == "completed" + assert out["workflow"] == "child" + + def test_failed_shape(self, project_dir): + _install(project_dir, "child", _workflow("child", [_shell("x", "exit 3")])) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "c", + "type": "workflow", + "workflow": "child", + "continue_on_error": True, + } + ], + ), + ) + result = _run(project_dir, "parent").step_results["c"] + assert result["status"] == "failed" + assert result["output"]["status"] == "failed" + assert result["output"]["workflow"] == "child" + + def test_paused_shape(self, project_dir): + _install( + project_dir, + "child", + _workflow( + "child", + [{"id": "g", "type": "gate", "message": "ok?", "options": ["approve", "reject"]}], + ), + ) + _install( + project_dir, + "parent", + _workflow("parent", [{"id": "c", "type": "workflow", "workflow": "child"}]), + ) + state = _run(project_dir, "parent") + assert state.status == RunStatus.PAUSED + assert state.step_results["c"]["output"]["status"] == "paused" + + def test_aborted_shape(self, project_dir): + _install( + project_dir, + "child", + _workflow( + "child", + [ + { + "id": "g", + "type": "gate", + "message": "ok?", + "options": ["approve", "reject"], + "on_reject": "abort", + } + ], + inputs={"verdict": {"type": "string", "default": ""}}, + ), + ) + # Abort requires a reject choice; route it through a verdict input. + _install( + project_dir, + "child2", + _workflow( + "child2", + [ + { + "id": "g", + "type": "gate", + "message": "ok?", + "options": ["approve", "reject"], + "on_reject": "abort", + "verdict_input": "verdict", + } + ], + inputs={ + "verdict": { + "type": "string", + "default": "", + "enum": ["approve", "reject", ""], + } + }, + ), + ) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "c", + "type": "workflow", + "workflow": "child2", + "input": {"verdict": "{{ inputs.verdict }}"}, + } + ], + inputs={"verdict": {"type": "string", "default": "reject"}}, + ), + ) + state = _run(project_dir, "parent") + assert state.status == RunStatus.ABORTED + assert state.step_results["c"]["output"].get("aborted") is True + + +class TestContinueOnError: + @pytest.mark.parametrize("continue_on_error", [False, True]) + def test_output_evaluation_failure_uses_call_boundary( + self, project_dir, continue_on_error + ): + _install( + project_dir, + "child", + _workflow( + "child", + [_shell("x", "printf not-json")], + outputs={"parsed": {"value": "{{ steps.x.output.stdout | from_json }}"}}, + ), + ) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "c", "type": "workflow", "workflow": "child", + "continue_on_error": continue_on_error, + }, + _shell("after", "echo continued"), + ], + ), + ) + + state = _run(project_dir, "parent") + result = state.step_results["c"] + assert result["status"] == "failed" + assert result["output"] == {"workflow": "child", "status": "failed"} + assert "failed to evaluate outputs" in result["error"] + assert "from_json: invalid JSON" in result["error"] + assert state.workflow_scopes["c"]["status"] == "failed" + assert state.workflow_scopes["c"]["step_results"]["x"]["status"] == "completed" + if continue_on_error: + assert state.status == RunStatus.COMPLETED + assert state.step_results["after"]["output"]["stdout"].strip() == "continued" + else: + assert state.status == RunStatus.FAILED + assert state.error == result["error"] + assert "after" not in state.step_results + + def test_call_boundary_continue(self, project_dir): + _install(project_dir, "child", _workflow("child", [_shell("x", "exit 3")])) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "c", + "type": "workflow", + "workflow": "child", + "continue_on_error": True, + }, + _shell("after", "echo continued"), + ], + ), + ) + state = _run(project_dir, "parent") + assert state.status == RunStatus.COMPLETED + assert state.step_results["c"]["status"] == "failed" + assert state.step_results["after"]["output"]["stdout"].strip() == "continued" + + def test_included_step_continue(self, project_dir): + _install( + project_dir, + "child", + _workflow( + "child", + [ + _shell("bad", "exit 3", continue_on_error=True), + _shell("ok", "echo child-ok"), + ], + ), + ) + _install( + project_dir, + "parent", + _workflow("parent", [{"id": "c", "type": "workflow", "workflow": "child"}]), + ) + state = _run(project_dir, "parent") + assert state.status == RunStatus.COMPLETED + assert state.workflow_scopes["c"]["step_results"]["ok"]["status"] == "completed" + + def test_abort_not_overridden_by_continue_on_error(self, project_dir): + _install( + project_dir, + "child", + _workflow( + "child", + [ + { + "id": "g", + "type": "gate", + "message": "ok?", + "options": ["approve", "reject"], + "on_reject": "abort", + "verdict_input": "verdict", + } + ], + inputs={ + "verdict": { + "type": "string", + "default": "", + "enum": ["approve", "reject", ""], + } + }, + ), + ) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "c", + "type": "workflow", + "workflow": "child", + "continue_on_error": True, + "input": {"verdict": "{{ inputs.verdict }}"}, + } + ], + inputs={"verdict": {"type": "string", "default": "reject"}}, + ), + ) + state = _run(project_dir, "parent") + assert state.status == RunStatus.ABORTED + + def test_pause_not_bypassed_by_continue_on_error(self, project_dir): + _install( + project_dir, + "child", + _workflow( + "child", + [{"id": "g", "type": "gate", "message": "ok?", "options": ["approve", "reject"]}], + ), + ) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "c", + "type": "workflow", + "workflow": "child", + "continue_on_error": True, + } + ], + ), + ) + state = _run(project_dir, "parent") + assert state.status == RunStatus.PAUSED + + +class TestRuntimeResolutionFailures: + def test_unknown_target(self, project_dir): + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "c", + "type": "workflow", + "workflow": "does-not-exist", + "continue_on_error": True, + } + ], + ), + ) + state = _run(project_dir, "parent") + assert state.status == RunStatus.COMPLETED + assert state.step_results["c"]["status"] == "failed" + assert state.step_results["c"]["output"]["status"] == "failed" + assert "not installed" in state.step_results["c"]["error"] + + def test_disabled_target(self, project_dir): + _install(project_dir, "child", _workflow("child", [_shell("x", "echo")]), enabled=False) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "c", + "type": "workflow", + "workflow": "child", + "continue_on_error": True, + } + ], + ), + ) + state = _run(project_dir, "parent") + assert state.step_results["c"]["status"] == "failed" + assert "disabled" in state.step_results["c"]["error"] + + def test_unknown_input(self, project_dir): + _install( + project_dir, + "child", + _workflow("child", [_shell("x", "echo")], inputs={"known": {"type": "string"}}), + ) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "c", + "type": "workflow", + "workflow": "child", + "input": {"typo": "x"}, + "continue_on_error": True, + } + ], + ), + ) + state = _run(project_dir, "parent") + assert state.step_results["c"]["status"] == "failed" + assert "undeclared input" in state.step_results["c"]["error"] + + def test_non_string_expression_target(self, project_dir): + _install(project_dir, "child", _workflow("child", [_shell("x", "echo")])) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + _shell("pick", "echo 5"), + { + "id": "c", + "type": "workflow", + "workflow": "{{ steps.pick.output.exit_code }}", + "continue_on_error": True, + }, + ], + ), + ) + state = _run(project_dir, "parent") + assert state.step_results["c"]["status"] == "failed" + assert "expected a string" in state.step_results["c"]["error"] + + +class TestRecursionAndDepth: + def test_cycle_rejected(self, project_dir): + _install( + project_dir, + "a", + _workflow("a", [{"id": "b", "type": "workflow", "workflow": "b"}]), + ) + _install( + project_dir, + "b", + _workflow("b", [{"id": "a", "type": "workflow", "workflow": "a"}]), + ) + state = _run(project_dir, "a") + assert state.status == RunStatus.FAILED + assert "cycle" in (state.error or "").lower() + + def test_diamond_allowed(self, project_dir): + _install(project_dir, "d", _workflow("d", [_shell("x", "echo d")])) + _install( + project_dir, "b", _workflow("b", [{"id": "d", "type": "workflow", "workflow": "d"}]) + ) + _install( + project_dir, "c", _workflow("c", [{"id": "d", "type": "workflow", "workflow": "d"}]) + ) + _install( + project_dir, + "a", + _workflow( + "a", + [ + {"id": "b", "type": "workflow", "workflow": "b"}, + {"id": "c", "type": "workflow", "workflow": "c"}, + ], + ), + ) + state = _run(project_dir, "a") + assert state.status == RunStatus.COMPLETED + assert "d" in state.workflow_scopes["b"]["workflow_scopes"] + assert "d" in state.workflow_scopes["c"]["workflow_scopes"] + + def _chain(self, project_dir, length: int) -> None: + for i in range(length): + if i == length - 1: + steps = [_shell("x", "echo end")] + else: + steps = [{"id": "next", "type": "workflow", "workflow": f"w{i + 1}"}] + _install(project_dir, f"w{i}", _workflow(f"w{i}", steps)) + + def test_depth_16_allowed(self, project_dir): + # w0 (depth 0) ... w16 (depth 16): 16 included levels. + self._chain(project_dir, MAX_COMPOSITION_DEPTH + 1) + state = _run(project_dir, "w0") + assert state.status == RunStatus.COMPLETED + + def test_depth_17_rejected(self, project_dir): + self._chain(project_dir, MAX_COMPOSITION_DEPTH + 2) + state = _run(project_dir, "w0") + assert state.status == RunStatus.FAILED + assert "maximum depth" in (state.error or "") + + +class TestPersistence: + def test_state_json_contains_scope_tree(self, project_dir): + _install(project_dir, "child", _workflow("child", [_shell("x", "echo hi")])) + _install( + project_dir, + "parent", + _workflow("parent", [{"id": "c", "type": "workflow", "workflow": "child"}]), + ) + state = _run(project_dir, "parent") + state_path = state.runs_dir / "state.json" + data = json.loads(state_path.read_text(encoding="utf-8")) + assert "workflow_scopes" in data + assert data["workflow_scopes"]["c"]["workflow_id"] == "child" + assert data["workflow_scopes"]["c"]["definition"]["workflow"]["id"] == "child" + + def test_load_defaults_when_absent(self, project_dir): + state = RunState(run_id="r", workflow_id="w", project_root=project_dir) + state.status = RunStatus.PAUSED + state.save() + path = state.runs_dir / "state.json" + data = json.loads(path.read_text(encoding="utf-8")) + data.pop("workflow_scopes", None) + path.write_text(json.dumps(data), encoding="utf-8") + loaded = RunState.load("r", project_dir) + assert loaded.workflow_scopes == {} + + def test_backward_compatible_pre_feature_state(self, project_dir): + state = RunState(run_id="old", workflow_id="w", project_root=project_dir) + state.status = RunStatus.PAUSED + state.save() + path = state.runs_dir / "state.json" + data = json.loads(path.read_text(encoding="utf-8")) + data.pop("workflow_scopes", None) + path.write_text(json.dumps(data), encoding="utf-8") + loaded = RunState.load("old", project_dir) + assert loaded.workflow_scopes == {} + + def test_completion_handoff_is_atomic(self, project_dir, monkeypatch): + """Every persisted snapshot with a COMPLETED child also has its caller result.""" + _install(project_dir, "child", _workflow("child", [_shell("x", "echo hi")])) + _install( + project_dir, + "parent", + _workflow("parent", [{"id": "c", "type": "workflow", "workflow": "child"}]), + ) + + snapshots: list[dict] = [] + real = RunState._atomic_write_json + + def spy(path, data): + if str(path).endswith("state.json"): + snapshots.append(json.loads(json.dumps(data))) + return real(path, data) + + monkeypatch.setattr(RunState, "_atomic_write_json", staticmethod(spy)) + state = _run(project_dir, "parent") + assert state.status == RunStatus.COMPLETED + assert snapshots + for snap in snapshots: + for key, scope in (snap.get("workflow_scopes") or {}).items(): + if scope.get("status") == "completed": + assert key in snap["step_results"], snap + + def test_concurrent_fan_out_cannot_save_unpaired_completion( + self, project_dir, monkeypatch + ): + from specify_cli.workflows.composition import ExecutionScope + + _install(project_dir, "child", _workflow("child", [_shell("x", "echo hi")])) + _install( + project_dir, + "parent", + _workflow( + "parent", + [{ + "id": "spread", "type": "fan-out", "items": ["a", "b"], + "max_concurrency": 2, + "step": {"id": "call", "type": "workflow", "workflow": "child"}, + }], + ), + ) + + first_waiting = Event() + sibling_saved = Event() + snapshots = [] + real_record_and_save = ExecutionScope.record_and_save + + def coordinated_handoff(self, context, step_id, data, **kwargs): + if step_id == "spread:call:0": + first_waiting.set() + assert sibling_saved.wait(5), "sibling never saved during handoff" + elif step_id == "spread:call:1": + assert first_waiting.wait(5), "first item never reached handoff" + # Emulate the sibling worker's ordinary progress save while + # item 0 has finished its subtree but has not recorded its result. + self.persist() + snapshots.append(json.loads((self.root().root_state.runs_dir / "state.json").read_text())) + sibling_saved.set() + return real_record_and_save(self, context, step_id, data, **kwargs) + + monkeypatch.setattr(ExecutionScope, "record_and_save", coordinated_handoff) + state = _run(project_dir, "parent") + assert state.status == RunStatus.COMPLETED + assert len(snapshots) == 1 + snapshot = snapshots[0] + assert snapshot["workflow_scopes"]["spread:call:0"]["status"] == "running" + assert "spread:call:0" not in snapshot["step_results"] + assert all( + child["status"] != "completed" or key in snapshot["step_results"] + for key, child in snapshot["workflow_scopes"].items() + ) + + +class TestResume: + def _paused_child(self, project_dir, child_steps, *, child_inputs=None, outputs=None): + _install( + project_dir, + "child", + _workflow("child", child_steps, inputs=child_inputs, outputs=outputs), + ) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "c", + "type": "workflow", + "workflow": "child", + "input": {"verdict": "{{ inputs.verdict }}"}, + } + ], + inputs={"verdict": {"type": "string", "default": ""}}, + ), + ) + + def test_pause_resumes_from_scope_index(self, project_dir): + self._paused_child( + project_dir, + [ + {"id": "g", "type": "gate", "message": "ok?", "options": ["approve", "reject"], + "verdict_input": "verdict"}, + _shell("after", "echo after"), + ], + child_inputs={ + "verdict": { + "type": "string", + "default": "", + "enum": ["approve", ""], + } + }, + ) + engine = WorkflowEngine(project_dir) + state = engine.execute(_definition(project_dir, "parent"), {}) + assert state.status == RunStatus.PAUSED + + resumed = engine.resume(state.run_id, {"verdict": "approve"}) + assert resumed.status == RunStatus.COMPLETED + child = resumed.workflow_scopes["c"] + assert child["step_results"]["after"]["status"] == "completed" + + def test_failed_call_retries_on_resume(self, project_dir): + marker = project_dir / "marker" + _install( + project_dir, + "child", + _workflow( + "child", + [_shell("x", f"test -f {marker} || {{ touch {marker}; exit 1; }}")], + ), + ) + _install( + project_dir, + "parent", + _workflow( + "parent", + [{"id": "c", "type": "workflow", "workflow": "child"}], + ), + ) + engine = WorkflowEngine(project_dir) + state = engine.execute(_definition(project_dir, "parent"), {}) + assert state.status == RunStatus.FAILED + assert state.workflow_scopes["c"]["status"] == "failed" + + resumed = engine.resume(state.run_id) + assert resumed.status == RunStatus.COMPLETED + assert resumed.workflow_scopes["c"]["status"] == "completed" + + def test_failed_output_evaluation_retries_on_resume(self, project_dir): + marker = project_dir / "valid-json" + _install( + project_dir, + "child", + _workflow( + "child", + [_shell("x", f"test -f {marker} && printf '{{\"ok\": true}}' || printf bad")], + outputs={"parsed": {"value": "{{ steps.x.output.stdout | from_json }}"}}, + ), + ) + _install( + project_dir, + "parent", + _workflow("parent", [{"id": "c", "type": "workflow", "workflow": "child"}]), + ) + engine = WorkflowEngine(project_dir) + state = engine.execute(_definition(project_dir, "parent"), {}) + assert state.status == RunStatus.FAILED + assert state.workflow_scopes["c"]["status"] == "failed" + + marker.touch() + resumed = engine.resume(state.run_id) + assert resumed.status == RunStatus.COMPLETED + assert resumed.step_results["c"]["output"]["parsed"] == {"ok": True} + + def test_resume_uses_definition_snapshot(self, project_dir): + self._paused_child( + project_dir, + [ + {"id": "g", "type": "gate", "message": "ok?", "options": ["approve", "reject"], + "verdict_input": "verdict"}, + _shell("x", "echo v1"), + ], + child_inputs={ + "verdict": { + "type": "string", + "default": "", + "enum": ["approve", ""], + } + }, + outputs={"v": {"value": "{{ steps.x.output.stdout }}"}}, + ) + engine = WorkflowEngine(project_dir) + state = engine.execute(_definition(project_dir, "parent"), {}) + assert state.status == RunStatus.PAUSED + + # Edit the installed child: new invocations see v2, the bound scope does not. + edited = _workflow( + "child", + [ + {"id": "g", "type": "gate", "message": "ok?", "options": ["approve", "reject"], + "verdict_input": "verdict"}, + _shell("x", "echo v2"), + ], + inputs={"verdict": {"type": "string", "default": "", "enum": ["approve", ""]}}, + outputs={"v": {"value": "{{ steps.x.output.stdout }}"}}, + ) + _install(project_dir, "child", edited) + + resumed = engine.resume(state.run_id, {"verdict": "approve"}) + assert resumed.status == RunStatus.COMPLETED + assert resumed.step_results["c"]["output"]["v"].strip() == "v1" + + def test_resume_does_not_resolve_bound_target(self, project_dir, monkeypatch): + from specify_cli.workflows.catalog import WorkflowRegistry + from specify_cli.workflows.step.workflow import WorkflowStep + + self._paused_child( + project_dir, + [ + { + "id": "g", "type": "gate", "message": "ok?", + "options": ["approve", "reject"], "verdict_input": "verdict", + } + ], + child_inputs={"verdict": {"type": "string", "default": ""}}, + ) + engine = WorkflowEngine(project_dir) + state = engine.execute(_definition(project_dir, "parent"), {}) + assert state.status == RunStatus.PAUSED + + registry = WorkflowRegistry(project_dir) + registry.add("child", {**registry.get("child"), "enabled": False}) + + def unexpected_resolution(self, config, context): + pytest.fail("a bound workflow target was resolved again") + + monkeypatch.setattr(WorkflowStep, "execute", unexpected_resolution) + resumed = engine.resume(state.run_id, {"verdict": "approve"}) + assert resumed.status == RunStatus.COMPLETED + assert resumed.workflow_scopes["c"]["status"] == "completed" + + def test_resume_input_update_forwards_through_mapping(self, project_dir): + self._paused_child( + project_dir, + [ + {"id": "g", "type": "gate", "message": "ok?", "options": ["approve", "reject"], + "on_reject": "abort", "verdict_input": "verdict"}, + ], + child_inputs={ + "verdict": { + "type": "string", + "default": "", + "enum": ["approve", "reject", ""], + } + }, + ) + engine = WorkflowEngine(project_dir) + state = engine.execute(_definition(project_dir, "parent"), {}) + assert state.status == RunStatus.PAUSED + + resumed = engine.resume(state.run_id, {"verdict": "approve"}) + assert resumed.status == RunStatus.COMPLETED + assert resumed.workflow_scopes["c"]["inputs"]["verdict"] == "approve" + + def test_resume_without_input_updates_keeps_binding(self, project_dir): + self._paused_child( + project_dir, + [ + {"id": "g", "type": "gate", "message": "ok?", "options": ["approve", "reject"], + "verdict_input": "verdict"}, + ], + child_inputs={ + "verdict": { + "type": "string", + "default": "approve", + "enum": ["approve", ""], + } + }, + ) + engine = WorkflowEngine(project_dir) + state = engine.execute(_definition(project_dir, "parent"), {}) + assert state.status == RunStatus.PAUSED + before = state.workflow_scopes["c"]["inputs"]["verdict"] + + # No explicit --input: the gate still sees the persisted default. + resumed = engine.resume(state.run_id) + assert resumed.workflow_scopes["c"]["inputs"]["verdict"] == before + + def test_retry_gate_keeps_reset_child_input_without_root_update(self, project_dir): + self._paused_child( + project_dir, + [{ + "id": "g", "type": "gate", "message": "ok?", + "options": ["approve", "reject"], "on_reject": "retry", + "verdict_input": "verdict", + }], + child_inputs={"verdict": { + "type": "string", "default": "", "enum": ["", "approve", "reject"], + }}, + ) + engine = WorkflowEngine(project_dir) + state = engine.execute(_definition(project_dir, "parent"), {"verdict": "reject"}) + assert state.status == RunStatus.PAUSED + assert state.inputs["verdict"] == "reject" + assert state.workflow_scopes["c"]["inputs"]["verdict"] == "" + assert state.workflow_scopes["c"]["step_results"]["g"]["output"]["choice"] == "reject" + + resumed = engine.resume(state.run_id) + assert resumed.status == RunStatus.PAUSED + assert resumed.workflow_scopes["c"]["inputs"]["verdict"] == "" + assert resumed.workflow_scopes["c"]["step_results"]["g"]["output"]["choice"] is None + + approved = engine.resume(state.run_id, {"verdict": "approve"}) + assert approved.status == RunStatus.COMPLETED + assert approved.workflow_scopes["c"]["inputs"]["verdict"] == "approve" + + def test_root_input_update_propagates_through_nested_calls(self, project_dir): + gate = { + "id": "g", "type": "gate", "message": "ok?", + "options": ["approve", "reject"], "verdict_input": "verdict", + } + inputs = {"verdict": {"type": "string", "default": ""}} + _install(project_dir, "leaf", _workflow("leaf", [gate], inputs=inputs)) + _install(project_dir, "middle", _workflow( + "middle", [{ + "id": "leaf-call", "type": "workflow", "workflow": "leaf", + "input": {"verdict": "{{ inputs.verdict }}"}, + }], inputs=inputs, + )) + _install(project_dir, "parent", _workflow( + "parent", [{ + "id": "middle-call", "type": "workflow", "workflow": "middle", + "input": {"verdict": "{{ inputs.verdict }}"}, + }], inputs=inputs, + )) + engine = WorkflowEngine(project_dir) + state = engine.execute(_definition(project_dir, "parent"), {}) + assert state.status == RunStatus.PAUSED + + resumed = engine.resume(state.run_id, {"verdict": "approve"}) + assert resumed.status == RunStatus.COMPLETED + middle = resumed.workflow_scopes["middle-call"] + assert middle["inputs"]["verdict"] == "approve" + leaf = middle["workflow_scopes"]["leaf-call"] + assert leaf["inputs"]["verdict"] == "approve" + assert leaf["step_results"]["g"]["output"]["choice"] == "approve" + + +class TestRepeatedCalls: + def test_fan_out_scopes_are_distinct(self, project_dir): + _install( + project_dir, + "child", + _workflow( + "child", + [_shell("x", "echo {{ inputs.who | default('?') }}")], + inputs={"who": {"type": "string", "default": "?"}}, + ), + ) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "spread", + "type": "fan-out", + "items": ["a", "b", "c"], + "max_concurrency": 3, + "step": { + "id": "call", + "type": "workflow", + "workflow": "child", + "input": {"who": "{{ item }}"}, + }, + } + ], + ), + ) + state = _run(project_dir, "parent") + assert state.status == RunStatus.COMPLETED + assert set(state.workflow_scopes) == { + "spread:call:0", + "spread:call:1", + "spread:call:2", + } + + def test_completed_scope_reused_on_reentry(self, project_dir): + counter = project_dir / "count.txt" + _install( + project_dir, + "child", + _workflow("child", [_shell("x", f"echo run >> {counter}")]), + ) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "loop", + "type": "while", + "condition": "{{ inputs.loop == 'yes' }}", + "max_iterations": 2, + "steps": [ + {"id": "call", "type": "workflow", "workflow": "child"}, + { + "id": "gate", + "type": "gate", + "message": "ok?", + "options": ["approve", "reject"], + }, + ], + } + ], + inputs={"loop": {"type": "string", "default": "yes"}}, + ), + ) + engine = WorkflowEngine(project_dir) + state = engine.execute(_definition(project_dir, "parent"), {"loop": "yes"}) + assert state.status == RunStatus.PAUSED + # Re-run the same enclosing while step on resume; the completed child is reused. + resumed = engine.resume(state.run_id) + assert resumed.status == RunStatus.PAUSED + assert counter.read_text(encoding="utf-8").count("run") == 1 + + +class TestCustomStepInsideScope: + def test_custom_step_sees_only_child_scope(self, project_dir): + from specify_cli.workflows import STEP_REGISTRY, _register_step + from specify_cli.workflows.base import StepBase, StepResult + + class _ScopeProbe(StepBase): + type_key = "scope-probe" + + def execute(self, config, context): + return StepResult( + output={ + "inputs": dict(context.inputs), + "steps": sorted(context.steps), + } + ) + + if "scope-probe" not in STEP_REGISTRY: + _register_step(_ScopeProbe()) + + _install( + project_dir, + "child", + _workflow( + "child", + [ + {"id": "probe", "type": "scope-probe"}, + ], + inputs={"only": {"type": "string", "default": "v"}}, + ), + ) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + _shell("caller", "echo x"), + { + "id": "c", + "type": "workflow", + "workflow": "child", + "input": {"only": "v"}, + }, + ], + ), + ) + state = _run(project_dir, "parent") + probe = state.workflow_scopes["c"]["step_results"]["probe"]["output"] + assert probe["inputs"] == {"only": "v"} + assert probe["steps"] == [] + + +# -- CLI tests ------------------------------------------------------------ + + +class TestCliReporting: + def _install_composed(self, project_dir): + _install(project_dir, "child", _workflow("child", [_shell("x", "echo hi")])) + _install( + project_dir, + "parent", + _workflow("parent", [{"id": "c", "type": "workflow", "workflow": "child"}]), + ) + + def _invoke(self, project_dir, args): + from unittest.mock import patch + + from typer.testing import CliRunner + + from specify_cli import app + + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir): + return runner.invoke(app, args, catch_exceptions=False) + + def test_run_json_payload_includes_scopes(self, project_dir): + self._install_composed(project_dir) + result = self._invoke(project_dir, ["workflow", "run", "parent", "--json"]) + assert result.exit_code == 0, result.output + payload = json.loads(result.stdout) + assert payload["status"] == "completed" + assert payload["scopes"][0]["invocation_id"] == "c" + assert payload["scopes"][0]["workflow_id"] == "child" + + def test_run_json_payload_stable_without_scopes(self, project_dir): + _install(project_dir, "plain", _workflow("plain", [_shell("x", "echo hi")])) + result = self._invoke(project_dir, ["workflow", "run", "plain", "--json"]) + payload = json.loads(result.stdout) + assert "scopes" not in payload + + def test_status_human_renders_scopes(self, project_dir): + self._install_composed(project_dir) + run = json.loads( + self._invoke(project_dir, ["workflow", "run", "parent", "--json"]).stdout + ) + result = self._invoke(project_dir, ["workflow", "status", run["run_id"]]) + assert result.exit_code == 0, result.output + assert "Workflow scopes" in result.stdout + assert "c: completed" in result.stdout + + def test_resume_input_forwards_through_parent_mapping(self, project_dir): + _install( + project_dir, + "child", + _workflow( + "child", + [ + { + "id": "g", + "type": "gate", + "message": "ok?", + "options": ["approve", "reject"], + "verdict_input": "verdict", + } + ], + inputs={ + "verdict": { + "type": "string", + "default": "", + "enum": ["approve", ""], + } + }, + ), + ) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "c", + "type": "workflow", + "workflow": "child", + "input": {"verdict": "{{ inputs.verdict }}"}, + } + ], + inputs={"verdict": {"type": "string", "default": ""}}, + ), + ) + run = json.loads( + self._invoke(project_dir, ["workflow", "run", "parent", "--json"]).stdout + ) + assert run["status"] == "paused" + resumed = json.loads( + self._invoke( + project_dir, + ["workflow", "resume", run["run_id"], "--input", "verdict=approve", "--json"], + ).stdout + ) + assert resumed["status"] == "completed" + assert resumed["scopes"][0]["status"] == "completed"