From d9edd1812833bec7308a6f794e69de6cfd4f4ae7 Mon Sep 17 00:00:00 2001 From: Markus Date: Thu, 24 Sep 2026 07:34:06 +0200 Subject: [PATCH 01/21] feat(workflows): compose installed workflows via a scoped workflow step Add a built-in `type: workflow` step that runs an installed workflow as a scoped subtree of the current run. Composition is an engine facility: one RunState, one run directory, and one process. - Add `composition` helpers: reserved output names, path-based cycle and depth checks, strict input binding, and declared-output evaluation. - Add `WorkflowStep`, registered in the built-in step registry, and `WorkflowDefinition.outputs` validated by `validate_workflow`. - Refactor `_execute_steps` to operate on an `ExecutionScope`; the root run is the root scope and nested scopes persist in `state.json` under `workflow_scopes` with backward-compatible load. - Commit a completed child's status and its caller-step result in one locked, atomic state write. - On resume, reuse the bound target and composed definition snapshot; explicit root input updates rebind reached, incomplete calls through their authored mapping. - Turn nested resolution, binding, cycle/depth, and output-evaluation errors into failed workflow-step results so `continue_on_error` applies. - Report nested scopes in `workflow status` and the `--json` payload. - Document the step type, scope isolation, outputs, and composition limits. Closes #4680 Assisted-by: opencode (model: deepseek-v4.1-flash, supervised) --- docs/reference/overview.md | 2 +- docs/reference/workflows.md | 94 +- src/specify_cli/workflows/__init__.py | 2 + src/specify_cli/workflows/_commands.py | 24 + src/specify_cli/workflows/command_status.py | 27 + src/specify_cli/workflows/composition.py | 599 +++++++ src/specify_cli/workflows/engine.py | 571 +++++-- .../workflows/step/workflow/__init__.py | 104 ++ tests/specify_cli/bundles/test_references.py | 2 +- tests/test_workflows.py | 3 +- tests/workflows/test_workflow_composition.py | 1379 +++++++++++++++++ 11 files changed, 2705 insertions(+), 102 deletions(-) create mode 100644 src/specify_cli/workflows/composition.py create mode 100644 src/specify_cli/workflows/step/workflow/__init__.py create mode 100644 tests/workflows/test_workflow_composition.py 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 f1c4391947..f97da4f101 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 @@ -539,9 +539,101 @@ 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. +### 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..b76fe708c9 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,6 +70,7 @@ def _register_builtin_steps() -> None: _register_step(SlotStep()) _register_step(SwitchStep()) _register_step(WhileStep()) + _register_step(WorkflowStep()) _register_builtin_steps() 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/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_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/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" From 2c4add602abf5981dda23f1c435ec6232368ddee Mon Sep 17 00:00:00 2001 From: Markus Date: Thu, 24 Sep 2026 09:15:00 +0200 Subject: [PATCH 02/21] fix(workflows): address review feedback on workflow composition - Escape authored scope IDs before rendering `workflow status` so a valid step ID containing Rich markup (e.g. `[/red]`) no longer raises MarkupError or misformats the scope tree. - Bound-check a nested scope's persisted `current_step_index` against its snapshot's step count, mirroring the root resume check, so a malformed state cannot silently complete a child with an empty step slice. - Replace the composition doc example that selected a workflow from a `prompt` step (whose stdout is always empty) with a declared workflow input, and note why. - Restore the process-global step registry after the custom-step scope test via monkeypatch.setitem instead of leaking `scope-probe`. Assisted-by: opencode (model: deepseek-v4.1-flash, supervised) --- docs/reference/workflows.md | 16 ++++-- src/specify_cli/workflows/command_status.py | 9 ++- src/specify_cli/workflows/composition.py | 17 +++++- tests/workflows/test_workflow_composition.py | 59 ++++++++++++++++++-- 4 files changed, 89 insertions(+), 12 deletions(-) diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index f97da4f101..a2b3327355 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -551,14 +551,15 @@ 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" +inputs: + target: + type: string + required: true +steps: - id: run-selected type: workflow - workflow: "{{ steps.triage.output.stdout }}" + workflow: "{{ inputs.target }}" input: report: "{{ inputs.report }}" slug: "{{ inputs.slug }}" @@ -569,6 +570,11 @@ steps: | `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. | +The `workflow` expression must resolve to a value the engine captures: a +declared workflow input or a preceding step's captured output (for example a +`shell` step's `stdout`) both work. A `prompt` step streams its output to the +agent and returns an empty `stdout`, so it cannot drive `workflow` selection. + `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`. diff --git a/src/specify_cli/workflows/command_status.py b/src/specify_cli/workflows/command_status.py index 8179722227..8036db7886 100644 --- a/src/specify_cli/workflows/command_status.py +++ b/src/specify_cli/workflows/command_status.py @@ -19,9 +19,14 @@ def _render_scopes(scopes: dict, indent: str) -> None: continue s = record.get("status", "unknown") sc = colors.get(s, "white") + # Scope keys are authored step IDs, and validation permits Rich markup + # characters such as ``[`` and ``]``; escape both interpolated values + # (as ``workflow run`` does) so a bracketed ID renders literally + # instead of being parsed as a style tag or raising ``MarkupError``. cli.console.print( - f"{indent}[{sc}]●[/{sc}] {key}: {s} " - f"[dim]({record.get('workflow_id', '?')})[/dim]" + f"{indent}[{sc}]●[/{sc}] {cli._escape_markup(str(key))}: {s} " + f"[dim]({cli._escape_markup(str(record.get('workflow_id', '?')))})" + f"[/dim]" ) nested = record.get("workflow_scopes") if isinstance(nested, dict) and nested: diff --git a/src/specify_cli/workflows/composition.py b/src/specify_cli/workflows/composition.py index 2e809d7975..e20d3de02e 100644 --- a/src/specify_cli/workflows/composition.py +++ b/src/specify_cli/workflows/composition.py @@ -571,7 +571,8 @@ def _validate_scope_record( 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)) + parsed_definition = WorkflowDefinition(definition) + errors = validate_workflow(parsed_definition) if errors: msg = ( f"Invalid run state: '{path}.definition' is invalid: " @@ -579,6 +580,20 @@ def _validate_scope_record( ) raise ValueError(msg) + # A nested scope resumes by slicing its persisted definition at + # ``current_step_index``; an index at or beyond the step count would + # otherwise yield an empty slice and let the scope silently complete + # without running its remaining steps. Mirrors the root-run bound check in + # ``WorkflowEngine.resume``, which ``RunState.load`` cannot apply until the + # definition (and its step count) is known. + if index >= len(parsed_definition.steps): + msg = ( + f"Invalid run state: '{path}.current_step_index' ({index}) is " + f"out of range for workflow {workflow_id!r} with " + f"{len(parsed_definition.steps)} step(s)" + ) + raise ValueError(msg) + children = record.get("workflow_scopes", {}) _validate_scope_tree(children, validate_workflow, path=f"{path}.workflow_scopes") diff --git a/tests/workflows/test_workflow_composition.py b/tests/workflows/test_workflow_composition.py index e7c9f013ba..9f95a2e568 100644 --- a/tests/workflows/test_workflow_composition.py +++ b/tests/workflows/test_workflow_composition.py @@ -792,6 +792,28 @@ def test_backward_compatible_pre_feature_state(self, project_dir): loaded = RunState.load("old", project_dir) assert loaded.workflow_scopes == {} + def test_load_rejects_out_of_range_nested_step_index(self, project_dir): + """A nested scope's index must stay within its persisted step count. + + A malformed index would otherwise slice the child's remaining steps to + an empty list and let the scope silently complete on resume. + """ + _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") + path = state.runs_dir / "state.json" + data = json.loads(path.read_text(encoding="utf-8")) + scope = data["workflow_scopes"]["c"] + scope["current_step_index"] = len(scope["definition"]["steps"]) + 1 + path.write_text(json.dumps(data), encoding="utf-8") + + with pytest.raises(ValueError, match="out of range"): + RunState.load(state.run_id, project_dir) + 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")])) @@ -1226,8 +1248,8 @@ def test_completed_scope_reused_on_reentry(self, project_dir): class TestCustomStepInsideScope: - def test_custom_step_sees_only_child_scope(self, project_dir): - from specify_cli.workflows import STEP_REGISTRY, _register_step + def test_custom_step_sees_only_child_scope(self, project_dir, monkeypatch): + from specify_cli.workflows import STEP_REGISTRY from specify_cli.workflows.base import StepBase, StepResult class _ScopeProbe(StepBase): @@ -1241,8 +1263,10 @@ def execute(self, config, context): } ) - if "scope-probe" not in STEP_REGISTRY: - _register_step(_ScopeProbe()) + # Register through monkeypatch so the process-global registry is + # restored after the test instead of leaking a custom step into later + # tests (mirrors tests/specify_cli/bundles/test_references.py). + monkeypatch.setitem(STEP_REGISTRY, "scope-probe", _ScopeProbe()) _install( project_dir, @@ -1325,6 +1349,33 @@ def test_status_human_renders_scopes(self, project_dir): assert "Workflow scopes" in result.stdout assert "c: completed" in result.stdout + def test_status_escapes_markup_in_scope_ids(self, project_dir): + """An authored step ID with Rich markup must not crash ``status``. + + The nested scope key is a caller step ID, and validation permits + brackets, so ``[`` / ``]`` must be escaped rather than parsed as markup. + """ + _install(project_dir, "leaf", _workflow("leaf", [_shell("x", "echo hi")])) + _install( + project_dir, + "mid", + _workflow( + "mid", + [{"id": "call[/red]", "type": "workflow", "workflow": "leaf"}], + ), + ) + _install( + project_dir, + "parent", + _workflow("parent", [{"id": "c", "type": "workflow", "workflow": "mid"}]), + ) + 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 "call[/red]: completed" in result.stdout + def test_resume_input_forwards_through_parent_mapping(self, project_dir): _install( project_dir, From 862d10ba3b24b38647f8a67e5eb4aa85edae6d77 Mon Sep 17 00:00:00 2001 From: Markus Date: Thu, 24 Sep 2026 09:58:58 +0200 Subject: [PATCH 03/21] fix(workflows): address second review pass on workflow composition - Add a downstream consumption test: a parent step after the workflow call evaluates `{{ steps.call.output.echoed }}`, so a failure to publish the completed child's output into the caller context before continuing is now caught (final-state inspection alone could not). - Update the built-in step inventories from 12 to 13 and add the `workflow` step to workflows/README.md and workflows/ARCHITECTURE.md. - Drop the two dangling references to spec/workflow_composition/ (the design decisions and implementation plan are not part of this change). Assisted-by: opencode (model: deepseek-v4.1-flash, supervised) --- src/specify_cli/workflows/composition.py | 3 +-- tests/workflows/test_workflow_composition.py | 15 ++++++++++-- workflows/ARCHITECTURE.md | 3 ++- workflows/README.md | 24 +++++++++++++++++++- 4 files changed, 39 insertions(+), 6 deletions(-) diff --git a/src/specify_cli/workflows/composition.py b/src/specify_cli/workflows/composition.py index e20d3de02e..ad5740f40c 100644 --- a/src/specify_cli/workflows/composition.py +++ b/src/specify_cli/workflows/composition.py @@ -1,7 +1,6 @@ """Workflow composition — scoped subtree execution helpers. -Implements the decisions recorded in -``spec/workflow_composition/design_decisions.md``: +Implements the workflow composition decisions: - reserved output names and the composition depth limit, - registry-backed, installed-and-enabled target resolution, diff --git a/tests/workflows/test_workflow_composition.py b/tests/workflows/test_workflow_composition.py index 9f95a2e568..daabd4e323 100644 --- a/tests/workflows/test_workflow_composition.py +++ b/tests/workflows/test_workflow_composition.py @@ -1,8 +1,7 @@ """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``. +persistence/resume, and CLI reporting. """ from __future__ import annotations @@ -301,6 +300,10 @@ def _parent_and_child(self, project_dir): "workflow": "child", "input": {"declared": "{{ inputs.shared }}"}, }, + _shell( + "consume", + "echo {{ steps.call.output.echoed | default('MISSING') }}", + ), ], inputs={"shared": {"type": "string", "default": "secret"}}, ), @@ -322,6 +325,14 @@ def test_caller_cannot_see_child_locals(self, project_dir): # Only declared outputs + stable metadata cross the boundary. assert set(call_output) == {"workflow", "status", "echoed"} + def test_caller_can_consume_child_output_downstream(self, project_dir): + """The caller context must publish a completed child's output *before* + the next step runs, so a downstream expression can consume it.""" + self._parent_and_child(project_dir) + state = _run(project_dir, "parent") + assert state.status == RunStatus.COMPLETED + assert state.step_results["consume"]["output"]["stdout"].strip() == "secret" + class TestPublicOutputShapes: def test_completed_shape(self, project_dir): diff --git a/workflows/ARCHITECTURE.md b/workflows/ARCHITECTURE.md index 088baac228..58fd493599 100644 --- a/workflows/ARCHITECTURE.md +++ b/workflows/ARCHITECTURE.md @@ -79,7 +79,7 @@ When a `gate` step pauses execution, the engine persists `current_step_index` an ## Step Types -The engine ships with 12 built-in step types, each in its own subpackage under `src/specify_cli/workflows/step/`: +The engine ships with 13 built-in step types, each in its own subpackage under `src/specify_cli/workflows/step/`: | Type Key | Class | Purpose | Returns `next_steps`? | |----------|-------|---------|-----------------------| @@ -95,6 +95,7 @@ The engine ships with 12 built-in step types, each in its own subpackage under ` | `do-while` | `DoWhileStep` | Loop, always runs body at least once | Yes (always) | | `fan-out` | `FanOutStep` | Dispatch per item over a collection | No (engine expands) | | `fan-in` | `FanInStep` | Aggregate results from fan-out | No | +| `workflow` | `WorkflowStep` | Run an installed workflow as a scoped subtree | No | ## Step Registry diff --git a/workflows/README.md b/workflows/README.md index 7382d6e625..8e7376df9b 100644 --- a/workflows/README.md +++ b/workflows/README.md @@ -85,7 +85,7 @@ The bundled `speckit` workflow only declares `spec` (and optional ## Step Types -Workflows support 12 built-in step types: +Workflows support 13 built-in step types: ### Command Steps (default) @@ -313,6 +313,28 @@ Aggregate results from fan-out steps: output: {} ``` +### Workflow Steps + +Compose an installed workflow as a scoped subtree of the current run — a +function call with one run state and one process. Values cross the boundary +only through the target's declared `inputs` and `outputs`: + +```yaml +- id: run-selected + type: workflow + workflow: "{{ inputs.target }}" # installed, registered, and enabled + input: + report: "{{ inputs.report }}" +``` + +The `workflow` value must resolve to something the engine captures — a declared +workflow input or a preceding step's captured output such as a `shell` step's +`stdout`. A `prompt` step streams its output and returns an empty `stdout`, so +it cannot drive `workflow` selection. The child receives only the bound declared +inputs, and only its declared outputs (merged with `workflow` and `status`) are +published back to the caller. See +[Workflow composition](../docs/reference/workflows.md#workflow-composition-type-workflow). + ## Error Handling By default, any step that returns `StepResult(status=StepStatus.FAILED, ...)` From 5c03032f7242a0139af9bda7002e4dca550b0e19 Mon Sep 17 00:00:00 2001 From: Markus Date: Thu, 24 Sep 2026 10:21:45 +0200 Subject: [PATCH 04/21] fix(workflows): reject malformed workflow step input in the shared helper `evaluate_input_mapping` previously coerced any non-mapping `input` (for example a list) to an empty mapping. Because `WorkflowEngine.execute` accepts unvalidated definitions, a malformed `input:` ran the child with defaults instead of failing, even though `validate_workflow_call_config` rejects the same shape at definition time. Preserve the omitted/null case, raise `ValueError` otherwise. All three call sites (`WorkflowStep.execute`, and the initial and resume binding paths) map the error to a failed workflow-step result. Assisted-by: opencode (model: deepseek-v4.1-flash, supervised) --- src/specify_cli/workflows/composition.py | 19 +++++- tests/workflows/test_workflow_composition.py | 63 +++++++++++++++++++- 2 files changed, 79 insertions(+), 3 deletions(-) diff --git a/src/specify_cli/workflows/composition.py b/src/specify_cli/workflows/composition.py index ad5740f40c..37f11ac73a 100644 --- a/src/specify_cli/workflows/composition.py +++ b/src/specify_cli/workflows/composition.py @@ -184,9 +184,24 @@ def validate_workflow_call_config(config: dict[str, Any]) -> list[str]: 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): + """Evaluate a caller's ``input`` mapping once in the caller scope. + + ``mapping`` is the raw ``input`` value from the step config. Omitted or + explicitly null (``None``) means "no inputs". Any other non-mapping is + malformed: ``WorkflowEngine.execute`` may be handed an unvalidated + definition, so fail closed here rather than silently discarding the + caller's mapping and running the child with defaults. The same shape is + rejected at definition time by ``validate_workflow_call_config``; the + callers turn this ``ValueError`` into a failed workflow-step result. + """ + if mapping is None: return {} + if not isinstance(mapping, dict): + msg = ( + f"'input' must be a mapping or omitted, got " + f"{type(mapping).__name__}." + ) + raise ValueError(msg) return { name: evaluate_expression(value, context) for name, value in mapping.items() diff --git a/tests/workflows/test_workflow_composition.py b/tests/workflows/test_workflow_composition.py index daabd4e323..47226c82da 100644 --- a/tests/workflows/test_workflow_composition.py +++ b/tests/workflows/test_workflow_composition.py @@ -13,12 +13,13 @@ import pytest import yaml -from specify_cli.workflows.base import RunStatus +from specify_cli.workflows.base import RunStatus, StepContext from specify_cli.workflows.composition import ( MAX_COMPOSITION_DEPTH, RESERVED_OUTPUT_NAMES, bind_composed_inputs, check_composition_path, + evaluate_input_mapping, validate_workflow_call_config, ) from specify_cli.workflows.engine import ( @@ -161,6 +162,27 @@ def test_non_mapping_input_rejected(self): assert any("'input' must be a mapping" in e for e in errors) +class TestEvaluateInputMapping: + def test_omitted_returns_empty(self): + assert evaluate_input_mapping({}, StepContext()) == {} + + def test_explicit_null_returns_empty(self): + assert evaluate_input_mapping(None, StepContext()) == {} + + @pytest.mark.parametrize("mapping", [["x"], "who", 5, True]) + def test_non_mapping_rejected(self, mapping): + # An unvalidated definition can reach the engine; a malformed ``input`` + # must fail rather than silently run the child with defaults. + with pytest.raises(ValueError, match="must be a mapping or omitted"): + evaluate_input_mapping(mapping, StepContext()) + + def test_values_evaluated_in_caller_scope(self): + context = StepContext(inputs={"who": "world"}) + assert evaluate_input_mapping( + {"name": "{{ inputs.who }}", "literal": "x"}, context + ) == {"name": "world", "literal": "x"} + + class TestCheckCompositionPath: def test_cycle_reported_before_depth(self): path = [f"w{i}" for i in range(MAX_COMPOSITION_DEPTH + 5)] + ["target"] @@ -704,6 +726,45 @@ def test_non_string_expression_target(self, project_dir): assert state.step_results["c"]["status"] == "failed" assert "expected a string" in state.step_results["c"]["error"] + def test_malformed_input_mapping_fails_step(self, project_dir): + """A non-mapping ``input`` must fail, not run the child on defaults. + + ``_run`` executes an unvalidated definition (as a direct engine caller + may), so the helper itself has to reject the malformed mapping. + """ + _install( + project_dir, + "child", + _workflow( + "child", + [_shell("x", "echo {{ inputs.who }}")], + inputs={"who": {"type": "string", "default": "default-who"}}, + ), + ) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "c", + "type": "workflow", + "workflow": "child", + "input": ["not-a-mapping"], + "continue_on_error": True, + } + ], + ), + ) + state = _run(project_dir, "parent") + assert state.status == RunStatus.COMPLETED + assert state.step_results["c"]["status"] == "failed" + assert "must be a mapping or omitted" in state.step_results["c"]["error"] + # The guard fires before a child scope is created, so the child never + # runs on silently-discarded inputs. + assert "c" not in state.workflow_scopes + class TestRecursionAndDepth: def test_cycle_rejected(self, project_dir): From 415e8e382edbc5ce2d377c81aecc410614382551 Mon Sep 17 00:00:00 2001 From: Markus Date: Thu, 24 Sep 2026 12:26:10 +0200 Subject: [PATCH 05/21] fix(workflows): trim a dynamic workflow target before ID validation `ShellStep` returns captured stdout verbatim, so `echo child` yields `"child\n"` and `_ID_PATTERN.fullmatch` rejected it, forcing authors to use `printf` for dynamic workflow selection. Trim surrounding whitespace on the resolved target before validating it; literal targets are already pattern-safe, so this is a no-op for them. Assisted-by: opencode (model: deepseek-v4.1-flash, supervised) --- docs/reference/workflows.md | 2 ++ .../workflows/step/workflow/__init__.py | 5 ++++ tests/workflows/test_workflow_composition.py | 26 +++++++++++++++++++ 3 files changed, 33 insertions(+) diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index a2b3327355..186a26aee6 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -574,6 +574,8 @@ The `workflow` expression must resolve to a value the engine captures: a declared workflow input or a preceding step's captured output (for example a `shell` step's `stdout`) both work. A `prompt` step streams its output to the agent and returns an empty `stdout`, so it cannot drive `workflow` selection. +A dynamically resolved string is whitespace-trimmed before ID validation, so a +`shell` step using `echo child` selects `child` despite the trailing newline. `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 diff --git a/src/specify_cli/workflows/step/workflow/__init__.py b/src/specify_cli/workflows/step/workflow/__init__.py index e9306b99ee..0cbd0ecfe9 100644 --- a/src/specify_cli/workflows/step/workflow/__init__.py +++ b/src/specify_cli/workflows/step/workflow/__init__.py @@ -54,6 +54,11 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: f"resolved to {type(target).__name__}, expected a string." ), ) + # A dynamic target is usually a step's captured stdout, which keeps + # its trailing newline (``echo child`` -> ``\"child\\n\"``) and would + # otherwise fail ID validation. Trim surrounding whitespace; literal + # targets are already pattern-safe, so this is a no-op for them. + target = target.strip() if ( not _ID_PATTERN.fullmatch(target) or target in _RESERVED_WORKFLOW_IDS diff --git a/tests/workflows/test_workflow_composition.py b/tests/workflows/test_workflow_composition.py index 47226c82da..acb1df28d9 100644 --- a/tests/workflows/test_workflow_composition.py +++ b/tests/workflows/test_workflow_composition.py @@ -289,6 +289,32 @@ def test_runtime_selected_target(self, project_dir): assert state.status == RunStatus.COMPLETED assert state.step_results["call"]["output"]["workflow"] == "child" + def test_runtime_target_from_echo_is_trimmed(self, project_dir): + """``echo`` adds a trailing newline; the resolved target is trimmed. + + Dynamic selection must work with an ordinary ``echo child``, not only + with newline-free commands like ``printf child``. + """ + _install(project_dir, "child", _workflow("child", [_shell("x", "echo hi")])) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + _shell("pick", "echo 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): From be4f2a81fb87f0cb633331d88a3e0a875b32c7cc Mon Sep 17 00:00:00 2001 From: Markus Date: Thu, 24 Sep 2026 12:27:42 +0200 Subject: [PATCH 06/21] fix(workflows): validate persisted scope snapshots structurally `RunState.load` re-ran `validate_workflow` on every persisted nested definition, which rejects any step type absent from the process-global `STEP_REGISTRY`. `workflow run`/`resume` call `load_custom_steps`, but `workflow status` does not, so a run whose composed child used a project custom step could be persisted yet not reloaded in a fresh process. Replace the semantic pass with a structural check covering only the shapes needed to slice and deserialize a scope (mapping snapshot sections, a list of step mappings each with a non-empty id). Step types are no longer restricted to the currently registered implementations. Assisted-by: opencode (model: deepseek-v4.1-flash, supervised) --- src/specify_cli/workflows/composition.py | 72 +++++++++++++----- tests/workflows/test_workflow_composition.py | 79 ++++++++++++++++++++ 2 files changed, 131 insertions(+), 20 deletions(-) diff --git a/src/specify_cli/workflows/composition.py b/src/specify_cli/workflows/composition.py index 37f11ac73a..a01809f0f9 100644 --- a/src/specify_cli/workflows/composition.py +++ b/src/specify_cli/workflows/composition.py @@ -517,13 +517,18 @@ def validate_serialized_scopes(scopes: Any) -> None: 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") + Validation is deliberately *structural*: it checks the snapshot shapes the + engine relies on to slice and deserialize a scope, without consulting the + process-global step registry. ``RunState.load`` is reached by commands that + do not call ``load_custom_steps`` (for example ``workflow status``), so a + full ``validate_workflow`` pass would reject an otherwise valid run whose + composed child uses a project-installed custom step. + """ + _validate_scope_tree(scopes, path="workflow_scopes") -def _validate_scope_tree(scopes: Any, validate_workflow: Any, *, path: str) -> None: +def _validate_scope_tree(scopes: Any, *, path: str) -> None: if not isinstance(scopes, dict): msg = f"Invalid run state: '{path}' must be a JSON object" raise ValueError(msg) @@ -536,14 +541,47 @@ def _validate_scope_tree(scopes: Any, validate_workflow: Any, *, path: str) -> N f"Invalid run state: '{path}.{key}' must be a JSON object" ) raise ValueError(msg) - _validate_scope_record(record, validate_workflow, path=f"{path}.{key}") + _validate_scope_record(record, path=f"{path}.{key}") -def _validate_scope_record( - record: dict[str, Any], validate_workflow: Any, *, path: str -) -> None: - from .engine import WorkflowDefinition +def _validate_definition_shape(definition: dict[str, Any], *, path: str) -> None: + """Structural validation of a persisted definition snapshot. + + Only the shapes the engine needs to safely deserialize and resume a scope + are required: a mapping ``workflow`` header, a list of step mappings each + carrying a non-empty string ``id``, and mapping ``inputs``/``outputs`` when + present. Step types are intentionally **not** restricted to the currently + registered implementations; see ``validate_serialized_scopes``. + """ + header = definition.get("workflow") + if header is not None and not isinstance(header, dict): + msg = f"Invalid run state: '{path}.workflow' must be a JSON object" + raise ValueError(msg) + + for key in ("inputs", "outputs"): + value = definition.get(key) + if value is not None and not isinstance(value, dict): + msg = f"Invalid run state: '{path}.{key}' must be a JSON object" + raise ValueError(msg) + + steps = definition.get("steps") + if not isinstance(steps, list): + msg = f"Invalid run state: '{path}.steps' must be a list" + raise ValueError(msg) + for i, step in enumerate(steps): + if not isinstance(step, dict): + msg = f"Invalid run state: '{path}.steps[{i}]' must be a JSON object" + raise ValueError(msg) + step_id = step.get("id") + if not isinstance(step_id, str) or not step_id: + msg = ( + f"Invalid run state: '{path}.steps[{i}].id' must be a " + "non-empty string" + ) + raise ValueError(msg) + +def _validate_scope_record(record: dict[str, Any], *, path: str) -> None: 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" @@ -585,14 +623,7 @@ def _validate_scope_record( if not isinstance(definition, dict): msg = f"Invalid run state: '{path}.definition' must be a JSON object" raise ValueError(msg) - parsed_definition = WorkflowDefinition(definition) - errors = validate_workflow(parsed_definition) - if errors: - msg = ( - f"Invalid run state: '{path}.definition' is invalid: " - + " ".join(errors) - ) - raise ValueError(msg) + _validate_definition_shape(definition, path=f"{path}.definition") # A nested scope resumes by slicing its persisted definition at # ``current_step_index``; an index at or beyond the step count would @@ -600,16 +631,17 @@ def _validate_scope_record( # without running its remaining steps. Mirrors the root-run bound check in # ``WorkflowEngine.resume``, which ``RunState.load`` cannot apply until the # definition (and its step count) is known. - if index >= len(parsed_definition.steps): + steps = definition["steps"] + if index >= len(steps): msg = ( f"Invalid run state: '{path}.current_step_index' ({index}) is " f"out of range for workflow {workflow_id!r} with " - f"{len(parsed_definition.steps)} step(s)" + f"{len(steps)} step(s)" ) raise ValueError(msg) children = record.get("workflow_scopes", {}) - _validate_scope_tree(children, validate_workflow, path=f"{path}.workflow_scopes") + _validate_scope_tree(children, path=f"{path}.workflow_scopes") __all__ = [ diff --git a/tests/workflows/test_workflow_composition.py b/tests/workflows/test_workflow_composition.py index acb1df28d9..5375cd73f7 100644 --- a/tests/workflows/test_workflow_composition.py +++ b/tests/workflows/test_workflow_composition.py @@ -988,6 +988,85 @@ def coordinated_handoff(self, context, step_id, data, **kwargs): ) +class TestSerializedScopeValidation: + def _save_state_with_scope(self, project_dir, definition): + state = RunState(run_id="r", workflow_id="w", project_root=project_dir) + state.status = RunStatus.PAUSED + state.workflow_scopes = { + "c": { + "workflow_id": "child", + "invocation_id": "c", + "definition": definition, + "inputs": {}, + "status": "paused", + "current_step_index": 0, + "step_results": {}, + "workflow_scopes": {}, + } + } + state.save() + return state + + def test_load_allows_well_formed_unregistered_step_type(self, project_dir): + """Schema validation must not depend on the live step registry. + + A composed child may use a project custom step that is not loaded in + this process (``workflow status`` never calls ``load_custom_steps``). + """ + self._save_state_with_scope( + project_dir, + _workflow("child", [{"id": "s", "type": "not-registered"}]), + ) + loaded = RunState.load("r", project_dir) + assert loaded.workflow_scopes["c"]["workflow_id"] == "child" + + def test_load_rejects_non_list_steps(self, project_dir): + definition = _workflow("child", []) + definition["steps"] = {"id": "s"} + self._save_state_with_scope(project_dir, definition) + with pytest.raises(ValueError, match=r"\.steps' must be a list"): + RunState.load("r", project_dir) + + def test_load_rejects_step_without_id(self, project_dir): + self._save_state_with_scope( + project_dir, _workflow("child", [{"type": "shell", "run": "true"}]) + ) + with pytest.raises(ValueError, match="non-empty string"): + RunState.load("r", project_dir) + + def test_custom_step_scope_loads_without_registration( + self, project_dir, monkeypatch + ): + from specify_cli.workflows import STEP_REGISTRY + from specify_cli.workflows.base import StepBase, StepResult + + class _Custom(StepBase): + type_key = "temp-custom-step" + + def execute(self, config, context): + return StepResult(output={"ok": True}) + + monkeypatch.setitem(STEP_REGISTRY, "temp-custom-step", _Custom()) + _install( + project_dir, + "child", + _workflow("child", [{"id": "s", "type": "temp-custom-step"}]), + ) + _install( + project_dir, + "parent", + _workflow("parent", [{"id": "c", "type": "workflow", "workflow": "child"}]), + ) + state = _run(project_dir, "parent") + assert state.status == RunStatus.COMPLETED + + # Simulate a fresh process that did not load project custom steps: + # `workflow status` must still be able to load the persisted run. + monkeypatch.delitem(STEP_REGISTRY, "temp-custom-step") + loaded = RunState.load(state.run_id, project_dir) + assert loaded.workflow_scopes["c"]["workflow_id"] == "child" + + class TestResume: def _paused_child(self, project_dir, child_steps, *, child_inputs=None, outputs=None): _install( From 2a6d2ae8907e74ca9a0ee232a86a059f947efa47 Mon Sep 17 00:00:00 2001 From: Markus Date: Thu, 24 Sep 2026 12:29:24 +0200 Subject: [PATCH 07/21] fix(workflows): surface a nested gate in the structured run payload A pause inside a composed workflow leaves the root resting on the `workflow` call step, so `_gate_outcome` returned None and `scopes` carried only ids/status. `workflow run/status --json` therefore reported `paused` without the gate message/options/choice, unlike the same workflow run directly. Search the serialized scope tree for the active gate when the root is not one, returning its detail plus the `scope_path` of invocation ids. Also expose each scope's `current_step_id` in the summary. Assisted-by: opencode (model: deepseek-v4.1-flash, supervised) --- src/specify_cli/workflows/_commands.py | 90 ++++++++++++++++---- tests/workflows/test_workflow_composition.py | 90 ++++++++++++++++++++ 2 files changed, 165 insertions(+), 15 deletions(-) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index fb7b5adf23..e7b5d76034 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -925,6 +925,24 @@ def _failed_step_error(state: Any) -> str | None: return getattr(state, "error", None) +def _scope_current_step_id(record: dict[str, Any]) -> str | None: + """Step id a serialized scope rests on, derived from its snapshot index.""" + index = record.get("current_step_index") + definition = record.get("definition") + steps = definition.get("steps") if isinstance(definition, dict) else None + if ( + isinstance(index, int) + and not isinstance(index, bool) + and isinstance(steps, list) + and 0 <= index < len(steps) + and isinstance(steps[index], dict) + ): + step_id = steps[index].get("id") + if isinstance(step_id, str): + return step_id + return None + + def _scope_summary(scopes: Any) -> list[dict[str, Any]]: """Compact nested-scope summary for the machine-readable payload.""" summary: list[dict[str, Any]] = [] @@ -938,6 +956,7 @@ def _scope_summary(scopes: Any) -> list[dict[str, Any]]: "invocation_id": key, "workflow_id": record.get("workflow_id"), "status": record.get("status"), + "current_step_id": _scope_current_step_id(record), "scopes": _scope_summary(record.get("workflow_scopes")), } ) @@ -985,6 +1004,58 @@ def _is_gate_step(step: dict[str, Any]) -> bool: return isinstance(output, dict) and "on_reject" in output +def _gate_details(step_id: str, output: Any) -> dict[str, Any]: + """Normalise a gate step's output into the stable JSON gate schema. + + ``message``, ``options``, and ``choice`` may be non-string YAML literals in + an unvalidated workflow (``GateStep`` coerces none of them for the payload), + so all three are normalised: message → str, options → list[str] | None, + choice → str | None (None means no decision yet). + """ + output = output if isinstance(output, dict) else {} + message = output.get("message") + choice = output.get("choice") + return { + "step_id": step_id, + "message": None if message is None else str(message), + "options": _normalize_gate_options(output.get("options")), + "choice": None if choice is None else str(choice), + } + + +def _scope_gate(scopes: Any, path: list[str]) -> dict[str, Any] | None: + """Find the active gate inside a serialized scope tree. + + A pause/abort inside a composed workflow leaves the *root* resting on the + ``workflow`` call, so the gate itself lives in a nested scope. Return its + detail augmented with the ``scope_path`` (invocation ids root → leaf) so an + orchestrator can drive a nested gate exactly as a top-level one. + """ + if not isinstance(scopes, dict): + return None + for key, record in scopes.items(): + if not isinstance(record, dict): + continue + child_path = [*path, key] + # A deeper paused scope is more specific; check descendants first. + nested = _scope_gate(record.get("workflow_scopes"), child_path) + if nested is not None: + return nested + if str(record.get("status")) not in ("paused", "aborted"): + continue + step_id = _scope_current_step_id(record) + step_results = record.get("step_results") + if step_id is None or not isinstance(step_results, dict): + continue + step = step_results.get(step_id) + if isinstance(step, dict) and _is_gate_step(step): + return { + **_gate_details(step_id, step.get("output")), + "scope_path": child_path, + } + return None + + def _gate_outcome(state: Any) -> dict[str, Any] | None: """Gate detail for the structured outcome, when the run rests at a gate. @@ -1002,21 +1073,10 @@ def _gate_outcome(state: Any) -> dict[str, Any] | None: if getattr(state.status, "value", state.status) not in ("paused", "aborted"): return None step = (getattr(state, "step_results", None) or {}).get(state.current_step_id) - if not isinstance(step, dict) or not _is_gate_step(step): - return None - output = step.get("output") or {} - # `message`, `options`, and `choice` may be non-string YAML literals in an - # unvalidated workflow (GateStep coerces none of them for the payload), so - # normalise all three for a stable JSON schema: message → str, options → - # list[str] | None, choice → str | None (None means no decision yet). - message = output.get("message") - choice = output.get("choice") - return { - "step_id": state.current_step_id, - "message": None if message is None else str(message), - "options": _normalize_gate_options(output.get("options")), - "choice": None if choice is None else str(choice), - } + if isinstance(step, dict) and _is_gate_step(step): + return _gate_details(state.current_step_id, step.get("output")) + # Not a top-level gate: a composed workflow may be paused on a nested gate. + return _scope_gate(getattr(state, "workflow_scopes", None), []) def _normalize_gate_options(options: Any) -> list[str] | None: diff --git a/tests/workflows/test_workflow_composition.py b/tests/workflows/test_workflow_composition.py index 5375cd73f7..3f21f2db14 100644 --- a/tests/workflows/test_workflow_composition.py +++ b/tests/workflows/test_workflow_composition.py @@ -1509,6 +1509,96 @@ def test_run_json_payload_includes_scopes(self, project_dir): assert payload["status"] == "completed" assert payload["scopes"][0]["invocation_id"] == "c" assert payload["scopes"][0]["workflow_id"] == "child" + assert payload["scopes"][0]["current_step_id"] == "x" + + def _install_paused_composed(self, project_dir): + _install( + project_dir, + "child", + _workflow( + "child", + [ + { + "id": "g", + "type": "gate", + "message": "Nested ok?", + "options": ["approve", "reject"], + } + ], + ), + ) + _install( + project_dir, + "parent", + _workflow("parent", [{"id": "c", "type": "workflow", "workflow": "child"}]), + ) + + def test_composed_pause_surfaces_nested_gate(self, project_dir): + """A pause inside a composed workflow must expose the nested gate. + + ``_gate_outcome`` otherwise sees only the root ``workflow`` call step, + leaving orchestrators without the message/options/choice to drive it. + """ + self._install_paused_composed(project_dir) + payload = json.loads( + self._invoke(project_dir, ["workflow", "run", "parent", "--json"]).stdout + ) + assert payload["status"] == "paused" + assert payload["gate"] == { + "step_id": "g", + "message": "Nested ok?", + "options": ["approve", "reject"], + "choice": None, + "scope_path": ["c"], + } + + def test_status_json_surfaces_nested_gate(self, project_dir): + self._install_paused_composed(project_dir) + run = json.loads( + self._invoke(project_dir, ["workflow", "run", "parent", "--json"]).stdout + ) + payload = json.loads( + self._invoke( + project_dir, ["workflow", "status", run["run_id"], "--json"] + ).stdout + ) + assert payload["gate"]["scope_path"] == ["c"] + assert payload["gate"]["step_id"] == "g" + + def test_deeply_nested_gate_reports_scope_path(self, project_dir): + _install( + project_dir, + "leaf", + _workflow( + "leaf", + [ + { + "id": "g", + "type": "gate", + "message": "Leaf?", + "options": ["approve", "reject"], + } + ], + ), + ) + _install( + project_dir, + "mid", + _workflow( + "mid", [{"id": "inner", "type": "workflow", "workflow": "leaf"}] + ), + ) + _install( + project_dir, + "parent", + _workflow("parent", [{"id": "c", "type": "workflow", "workflow": "mid"}]), + ) + payload = json.loads( + self._invoke(project_dir, ["workflow", "run", "parent", "--json"]).stdout + ) + assert payload["status"] == "paused" + assert payload["gate"]["scope_path"] == ["c", "inner"] + assert payload["gate"]["step_id"] == "g" def test_run_json_payload_stable_without_scopes(self, project_dir): _install(project_dir, "plain", _workflow("plain", [_shell("x", "echo hi")])) From e0d63fc5255cb268d067a079c3b3272ca06c4310 Mon Sep 17 00:00:00 2001 From: Markus Date: Thu, 24 Sep 2026 15:13:42 +0200 Subject: [PATCH 08/21] fix(workflows): persist a scope's resting step id for gate reporting A scope paused inside a nested control-flow body (if/switch/loop) keeps the enclosing step's index while `current_step_id` points at the inner gate, so deriving the id from the snapshot index surfaced the enclosing step and hid the gate from JSON clients. Persist `current_step_id` in `ExecutionScope._serialize`, restore and structurally validate it in `deserialize_scope`/`validate_serialized_scopes`, and prefer it in `_scope_current_step_id` with the index derivation kept as a fallback for older states. Assisted-by: opencode (model: deepseek-v4.1-flash, supervised) --- src/specify_cli/workflows/_commands.py | 12 ++++- src/specify_cli/workflows/composition.py | 16 +++++++ tests/workflows/test_workflow_composition.py | 50 ++++++++++++++++++++ 3 files changed, 77 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index e7b5d76034..2e1365960a 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -926,7 +926,17 @@ def _failed_step_error(state: Any) -> str | None: def _scope_current_step_id(record: dict[str, Any]) -> str | None: - """Step id a serialized scope rests on, derived from its snapshot index.""" + """Step id a serialized scope rests on. + + Prefers the persisted ``current_step_id``: it is the only accurate source + when a scope paused inside a nested control-flow body (``if``/``switch``/ + loop), where the snapshot index still points at the enclosing step. Falls + back to deriving the id from that index for states written before the field + was persisted. + """ + persisted = record.get("current_step_id") + if isinstance(persisted, str) and persisted: + return persisted index = record.get("current_step_index") definition = record.get("definition") steps = definition.get("steps") if isinstance(definition, dict) else None diff --git a/src/specify_cli/workflows/composition.py b/src/specify_cli/workflows/composition.py index a01809f0f9..862e5ca2a8 100644 --- a/src/specify_cli/workflows/composition.py +++ b/src/specify_cli/workflows/composition.py @@ -417,6 +417,7 @@ def _serialize(self) -> dict[str, Any]: "inputs": self.inputs, "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": { key: child._serialize() @@ -496,6 +497,7 @@ def deserialize_scope( workflow_dir=record.get("workflow_dir"), step_results=record.get("step_results", {}) or {}, current_step_index=record.get("current_step_index", 0), + current_step_id=record.get("current_step_id"), status=RunStatus(record.get("status", RunStatus.RUNNING.value)), parent=parent, root_state=root_state, @@ -612,6 +614,20 @@ def _validate_scope_record(record: dict[str, Any], *, path: str) -> None: ) raise ValueError(msg) + # The step a scope rests on. It is *not* required to appear in + # ``definition.steps``: a scope paused inside a nested control-flow body + # (``if``/``switch``/loop) rests on a step that only exists in that body, + # while ``current_step_index`` still points at the enclosing step. + current_step_id = record.get("current_step_id") + if current_step_id is not None and ( + not isinstance(current_step_id, str) or not current_step_id + ): + msg = ( + f"Invalid run state: '{path}.current_step_id' must be a " + f"non-empty string or null, got {current_step_id!r}" + ) + raise ValueError(msg) + status = record.get("status", RunStatus.RUNNING.value) try: RunStatus(status) diff --git a/tests/workflows/test_workflow_composition.py b/tests/workflows/test_workflow_composition.py index 3f21f2db14..0cadda9967 100644 --- a/tests/workflows/test_workflow_composition.py +++ b/tests/workflows/test_workflow_composition.py @@ -1600,6 +1600,56 @@ def test_deeply_nested_gate_reports_scope_path(self, project_dir): assert payload["gate"]["scope_path"] == ["c", "inner"] assert payload["gate"]["step_id"] == "g" + def test_gate_inside_nested_control_flow_reports_gate(self, project_dir): + """A gate inside an ``if`` body must be reported, not its enclosing step. + + Nested control-flow bodies run with ``step_offset=-1``, so the scope's + snapshot index still points at the enclosing ``if`` while + ``current_step_id`` points at the gate. Deriving the id from the index + alone would surface the ``if`` step and hide the gate from JSON clients. + """ + _install( + project_dir, + "child", + _workflow( + "child", + [ + { + "id": "branch", + "type": "if", + "condition": "true", + "then": [ + { + "id": "g", + "type": "gate", + "message": "Nested control-flow ok?", + "options": ["approve", "reject"], + } + ], + } + ], + ), + ) + _install( + project_dir, + "parent", + _workflow("parent", [{"id": "c", "type": "workflow", "workflow": "child"}]), + ) + payload = json.loads( + self._invoke(project_dir, ["workflow", "run", "parent", "--json"]).stdout + ) + assert payload["status"] == "paused" + assert payload["gate"] == { + "step_id": "g", + "message": "Nested control-flow ok?", + "options": ["approve", "reject"], + "choice": None, + "scope_path": ["c"], + } + # The scope's resting step id is persisted, not just derived on read. + loaded = RunState.load(payload["run_id"], project_dir) + assert loaded.workflow_scopes["c"]["current_step_id"] == "g" + 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"]) From c40b84b0af08add752380c72ab75157b485818d5 Mon Sep 17 00:00:00 2001 From: Markus Date: Thu, 24 Sep 2026 17:11:54 +0200 Subject: [PATCH 09/21] fix(workflows): persist composed definitions as YAML snapshots A composed child's resolved definition was embedded in state.json, so a valid YAML scalar that JSON cannot encode (an unquoted `2026-01-01` parses to `datetime.date`) made json.dump raise TypeError before the child ran. Only the definition was affected; the single atomic state.json write that ties child completion to the caller result must stay. Write each child's immutable definition to `snapshots/.yml` (YAML round-trips native scalars) and keep only its reference in state.json. The snapshot is written before the reference is saved, so a crash can only leave an unused file, never a dangling reference. Legacy states with an embedded definition still load; validation stays registry-independent and now resolves snapshot references. Also coerce a non-string gate message to text before it reaches the persisted step output, since the same date scalar would otherwise break state.json there. Assisted-by: opencode (model: deepseek-v4.1-flash, supervised) --- docs/reference/workflows.md | 16 +- src/specify_cli/workflows/composition.py | 219 ++++++++++++++++-- src/specify_cli/workflows/engine.py | 11 +- .../workflows/step/gate/__init__.py | 7 + tests/workflows/test_workflow_composition.py | 100 +++++++- 5 files changed, 325 insertions(+), 28 deletions(-) diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index 186a26aee6..6c74fed83b 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -630,12 +630,15 @@ 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. +the validated inputs are persisted with the run. The snapshot is written as an +immutable YAML file under the run's `snapshots/` directory (so YAML-native +values such as unquoted dates round-trip, unlike JSON), while `state.json` +records only its reference. 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 @@ -751,6 +754,7 @@ Each workflow run persists its state at `.specify/workflows/runs//`: - `state.json` — current run state and step progress - `inputs.json` — resolved input values - `log.jsonl` — step-by-step execution log +- `snapshots/*.yml` — immutable composed-child definition snapshots This enables `specify workflow resume` to continue from the exact step where a run was paused (e.g., at a gate) or failed. diff --git a/src/specify_cli/workflows/composition.py b/src/specify_cli/workflows/composition.py index 862e5ca2a8..6685221f19 100644 --- a/src/specify_cli/workflows/composition.py +++ b/src/specify_cli/workflows/composition.py @@ -14,11 +14,16 @@ from __future__ import annotations +import hashlib +import os import re +import tempfile from dataclasses import dataclass, field from pathlib import Path from typing import TYPE_CHECKING, Any +import yaml + from .base import RunStatus, StepContext from .expressions import evaluate_expression @@ -307,6 +312,125 @@ def check_composition_path(active_path: list[str], target: str) -> None: raise ValueError(msg) +# -- Definition snapshots ------------------------------------------------- +# +# A composed child's resolved definition is immutable for the life of its +# invocation. Persisting it as a YAML file (instead of embedding the parsed +# mapping in ``state.json``) keeps YAML-native scalars — dates, datetimes, etc. +# — that ``json.dump`` cannot encode. Execution state (status, progress, step +# results) stays in ``state.json`` so the single atomic write that ties a +# completed child to its caller result is preserved. + +#: Directory under a run directory holding immutable definition snapshots. +SNAPSHOT_DIRNAME = "snapshots" + +#: Snapshot file name: ``-.yml``. +_SNAPSHOT_REF_PATTERN = re.compile(r"^[a-z0-9][a-z0-9._-]*\.yml$") + + +def _invocation_path(scope: ExecutionScope) -> list[str]: + """Invocation ids from the root scope down to *scope*.""" + parts: list[str] = [] + node: ExecutionScope | None = scope + while node is not None: + parts.append(node.scope_id) + node = node.parent + return list(reversed(parts)) + + +def _snapshot_ref_for(scope: ExecutionScope) -> str: + """Deterministic, filesystem-safe snapshot name for *scope*. + + Derived from the invocation path and the target workflow id rather than the + authored step id, so a step id containing ``/`` or ``:`` can never escape + the snapshots directory. + """ + digest = hashlib.sha256( + "\x00".join(_invocation_path(scope)).encode("utf-8") + ).hexdigest()[:12] + return f"{scope.workflow_id}-{digest}.yml" + + +def _definition_snapshot_path(run_dir: Path, ref: str) -> Path: + """Resolve a snapshot reference to a path inside *run_dir*/snapshots.""" + if not isinstance(ref, str) or not _SNAPSHOT_REF_PATTERN.fullmatch(ref): + msg = f"Invalid definition snapshot reference: {ref!r}" + raise ValueError(msg) + snapshots_dir = (run_dir / SNAPSHOT_DIRNAME).resolve() + path = (snapshots_dir / ref).resolve() + if path.parent != snapshots_dir: + msg = f"Invalid definition snapshot reference: {ref!r}" + raise ValueError(msg) + return path + + +def _read_definition_snapshot(run_dir: Path, ref: str) -> dict[str, Any]: + """Load a persisted definition snapshot, failing closed on any problem.""" + path = _definition_snapshot_path(run_dir, ref) + if not path.is_file(): + msg = f"Invalid run state: definition snapshot {ref!r} is missing" + raise ValueError(msg) + with open(path, encoding="utf-8") as f: + try: + data = yaml.safe_load(f) + except yaml.YAMLError as exc: + msg = ( + f"Invalid run state: definition snapshot {ref!r} is not " + f"valid YAML: {exc}" + ) + raise ValueError(msg) from exc + if not isinstance(data, dict): + msg = f"Invalid run state: definition snapshot {ref!r} must be a mapping" + raise ValueError(msg) + return data + + +def _write_definition_snapshot( + run_dir: Path, ref: str, data: dict[str, Any] +) -> None: + """Atomically write a definition snapshot (temp file + ``os.replace``).""" + path = _definition_snapshot_path(run_dir, ref) + path.parent.mkdir(parents=True, exist_ok=True) + fd, tmp = tempfile.mkstemp( + dir=str(path.parent), prefix=f".{ref}.", suffix=".tmp" + ) + try: + with os.fdopen(fd, "w", encoding="utf-8") as f: + yaml.safe_dump(data, f, sort_keys=False) + os.replace(tmp, path) + except BaseException: + try: + os.unlink(tmp) + except OSError: + pass + raise + + +def write_definition_snapshot( + scope: ExecutionScope, data: dict[str, Any] +) -> str | None: + """Write *data* as *scope*'s snapshot; return its reference (or ``None``). + + Returns ``None`` for an unpersisted, in-memory scope (no root ``RunState``), + which keeps ``_serialize``'s embedded-definition fallback in play. + """ + state = scope.root().root_state + if state is None: + return None + ref = _snapshot_ref_for(scope) + _write_definition_snapshot(state.runs_dir, ref, data) + return ref + + +def _scope_definition_data(record: dict[str, Any], *, run_dir: Path) -> dict[str, Any]: + """Return a scope record's definition, from a snapshot or the legacy embed.""" + ref = record.get("definition_snapshot") + if isinstance(ref, str) and ref: + return _read_definition_snapshot(run_dir, ref) + embedded = record.get("definition", {}) + return embedded if isinstance(embedded, dict) else {} + + # -- Execution scope ------------------------------------------------------ @@ -322,6 +446,10 @@ class ExecutionScope: scope_id: str workflow_id: str definition: WorkflowDefinition | None = None + #: Persisted snapshot reference for :attr:`definition` (see the section + #: above). ``None`` means the definition is embedded in ``state.json`` + #: (legacy states, or an in-memory scope with no run directory). + definition_ref: str | 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) @@ -407,13 +535,10 @@ def build_context(self, *, is_resume: bool = False) -> StepContext: def _serialize(self) -> dict[str, Any]: """Serialize this node and its descendants into plain JSON data.""" - return { + record: dict[str, Any] = { "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, @@ -424,6 +549,18 @@ def _serialize(self) -> dict[str, Any]: for key, child in self.workflow_scopes.items() }, } + if self.definition_ref: + # Immutable definition lives in a YAML snapshot; state.json keeps + # only the reference, so YAML-native scalars never hit json.dump. + record["definition_snapshot"] = self.definition_ref + else: + # Legacy/pre-snapshot states, or an in-memory scope with no run + # directory: keep the embedded definition (already JSON-safe, since + # it was loaded from state.json or never persisted). + record["definition"] = ( + self.definition.data if self.definition is not None else {} + ) + return record def _sync_to_state(self, state: RunState) -> None: """Copy the root scope's live fields into *state* (lock held).""" @@ -488,11 +625,14 @@ def deserialize_scope( """Rebuild a runtime ``ExecutionScope`` from a persisted record.""" from .engine import WorkflowDefinition - definition = WorkflowDefinition(record.get("definition", {})) + definition = WorkflowDefinition( + _scope_definition_data(record, run_dir=root_state.runs_dir) + ) scope = ExecutionScope( scope_id=record.get("invocation_id", ""), workflow_id=record.get("workflow_id", ""), definition=definition, + definition_ref=record.get("definition_snapshot"), inputs=record.get("inputs", {}) or {}, workflow_dir=record.get("workflow_dir"), step_results=record.get("step_results", {}) or {}, @@ -514,23 +654,29 @@ def deserialize_scope( # -- Persisted-scope validation ------------------------------------------- -def validate_serialized_scopes(scopes: Any) -> None: +def validate_serialized_scopes( + scopes: Any, *, run_dir: Path | None = None +) -> None: """Validate a persisted ``workflow_scopes`` tree. Raises ``ValueError`` on any malformed node so ``RunState.load`` can fail - closed, mirroring its existing validation style. + closed, mirroring its existing validation style. ``run_dir`` locates the + per-run ``snapshots/`` directory so a scope's definition snapshot can be + loaded and structurally checked. - Validation is deliberately *structural*: it checks the snapshot shapes the - engine relies on to slice and deserialize a scope, without consulting the + Validation is deliberately *structural*: it checks the shapes the engine + relies on to slice and deserialize a scope, without consulting the process-global step registry. ``RunState.load`` is reached by commands that do not call ``load_custom_steps`` (for example ``workflow status``), so a full ``validate_workflow`` pass would reject an otherwise valid run whose composed child uses a project-installed custom step. """ - _validate_scope_tree(scopes, path="workflow_scopes") + _validate_scope_tree(scopes, run_dir=run_dir, path="workflow_scopes") -def _validate_scope_tree(scopes: Any, *, path: str) -> None: +def _validate_scope_tree( + scopes: Any, *, run_dir: Path | None, path: str +) -> None: if not isinstance(scopes, dict): msg = f"Invalid run state: '{path}' must be a JSON object" raise ValueError(msg) @@ -543,7 +689,7 @@ def _validate_scope_tree(scopes: Any, *, path: str) -> None: f"Invalid run state: '{path}.{key}' must be a JSON object" ) raise ValueError(msg) - _validate_scope_record(record, path=f"{path}.{key}") + _validate_scope_record(record, run_dir=run_dir, path=f"{path}.{key}") def _validate_definition_shape(definition: dict[str, Any], *, path: str) -> None: @@ -583,7 +729,45 @@ def _validate_definition_shape(definition: dict[str, Any], *, path: str) -> None raise ValueError(msg) -def _validate_scope_record(record: dict[str, Any], *, path: str) -> None: +def _resolve_scope_definition( + record: dict[str, Any], *, run_dir: Path | None, path: str +) -> dict[str, Any]: + """Load a scope's definition from its snapshot, or the legacy embed. + + Raises ``ValueError`` if neither is present or the snapshot cannot be read. + """ + ref = record.get("definition_snapshot") + if ref is not None: + if not isinstance(ref, str) or not ref: + msg = ( + f"Invalid run state: '{path}.definition_snapshot' must be a " + f"non-empty string, got {ref!r}" + ) + raise ValueError(msg) + if run_dir is None: + msg = ( + f"Invalid run state: '{path}.definition_snapshot' cannot be " + "resolved without a run directory" + ) + raise ValueError(msg) + return _read_definition_snapshot(run_dir, ref) + + definition = record.get("definition") + if definition is None: + msg = ( + f"Invalid run state: '{path}' must carry either " + "'definition_snapshot' or 'definition'" + ) + raise ValueError(msg) + if not isinstance(definition, dict): + msg = f"Invalid run state: '{path}.definition' must be a JSON object" + raise ValueError(msg) + return definition + + +def _validate_scope_record( + record: dict[str, Any], *, run_dir: Path | None, path: str +) -> None: 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" @@ -635,10 +819,7 @@ def _validate_scope_record(record: dict[str, Any], *, path: str) -> None: 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) + definition = _resolve_scope_definition(record, run_dir=run_dir, path=path) _validate_definition_shape(definition, path=f"{path}.definition") # A nested scope resumes by slicing its persisted definition at @@ -657,12 +838,13 @@ def _validate_scope_record(record: dict[str, Any], *, path: str) -> None: raise ValueError(msg) children = record.get("workflow_scopes", {}) - _validate_scope_tree(children, path=f"{path}.workflow_scopes") + _validate_scope_tree(children, run_dir=run_dir, path=f"{path}.workflow_scopes") __all__ = [ "MAX_COMPOSITION_DEPTH", "RESERVED_OUTPUT_NAMES", + "SNAPSHOT_DIRNAME", "ExecutionScope", "bind_composed_inputs", "check_composition_path", @@ -673,4 +855,5 @@ def _validate_scope_record(record: dict[str, Any], *, path: str) -> None: "validate_serialized_scopes", "validate_workflow_call_config", "validate_workflow_outputs", + "write_definition_snapshot", ] diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index b6a2a94043..be017c18dc 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -38,6 +38,7 @@ evaluate_input_mapping, validate_serialized_scopes, validate_workflow_outputs, + write_definition_snapshot, ) # -- Workflow Definition -------------------------------------------------- @@ -897,7 +898,7 @@ def load(cls, run_id: str, project_root: Path) -> RunState: # 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) + validate_serialized_scopes(workflow_scopes, run_dir=runs_dir) state = cls( run_id=state_data["run_id"], @@ -1759,6 +1760,14 @@ def _run_workflow_call( parent=scope, root_state=scope.root().root_state, ) + # Persist the resolved definition as an immutable YAML snapshot before + # referencing it from state.json. YAML round-trips native scalars + # (dates, datetimes) that json.dump cannot encode; writing it first + # means a crash can only leave an unused snapshot, never a scope whose + # definition reference points at nothing. + child_scope.definition_ref = write_definition_snapshot( + child_scope, definition.data + ) scope.add_workflow_scope(effective_id, child_scope) scope.persist() diff --git a/src/specify_cli/workflows/step/gate/__init__.py b/src/specify_cli/workflows/step/gate/__init__.py index 5aac060c0f..a602913b8b 100644 --- a/src/specify_cli/workflows/step/gate/__init__.py +++ b/src/specify_cli/workflows/step/gate/__init__.py @@ -40,6 +40,13 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: message = config.get("message", "Review required.") if isinstance(message, str) and "{{" in message: message = evaluate_expression(message, context) + # A YAML-native scalar message (an unquoted ``2026-01-01`` parses to a + # ``datetime.date``) is legitimate config but not JSON-serializable, and + # the step output is persisted in state.json. Coerce any non-null, + # non-string message to text so a valid gate cannot crash the run at + # save time. + if message is not None and not isinstance(message, str): + message = str(message) options = config.get("options", ["approve", "reject"]) on_reject = config.get("on_reject", "abort") diff --git a/tests/workflows/test_workflow_composition.py b/tests/workflows/test_workflow_composition.py index 0cadda9967..8b889edd05 100644 --- a/tests/workflows/test_workflow_composition.py +++ b/tests/workflows/test_workflow_composition.py @@ -865,8 +865,16 @@ def test_state_json_contains_scope_tree(self, project_dir): 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" + scope = data["workflow_scopes"]["c"] + assert scope["workflow_id"] == "child" + # The resolved definition lives in an immutable YAML snapshot, not in + # the JSON state, so YAML-native scalars never reach json.dump. + assert "definition" not in scope + ref = scope["definition_snapshot"] + snapshot = yaml.safe_load( + (state.runs_dir / "snapshots" / ref).read_text(encoding="utf-8") + ) + assert snapshot["workflow"]["id"] == "child" def test_load_defaults_when_absent(self, project_dir): state = RunState(run_id="r", workflow_id="w", project_root=project_dir) @@ -906,12 +914,50 @@ def test_load_rejects_out_of_range_nested_step_index(self, project_dir): path = state.runs_dir / "state.json" data = json.loads(path.read_text(encoding="utf-8")) scope = data["workflow_scopes"]["c"] - scope["current_step_index"] = len(scope["definition"]["steps"]) + 1 + snapshot = yaml.safe_load( + (state.runs_dir / "snapshots" / scope["definition_snapshot"]).read_text( + encoding="utf-8" + ) + ) + scope["current_step_index"] = len(snapshot["steps"]) + 1 path.write_text(json.dumps(data), encoding="utf-8") with pytest.raises(ValueError, match="out of range"): RunState.load(state.run_id, project_dir) + def test_yaml_native_scalar_in_composed_definition_persists(self, project_dir): + """A YAML-native date must not break composed-scope persistence. + + PyYAML parses an unquoted ``2026-01-01`` to ``datetime.date``, which + ``json.dump`` cannot encode. The resolved definition is kept in a YAML + snapshot; the gate's persisted step output is coerced to text. + """ + from datetime import date + + _install( + project_dir, + "child", + _workflow( + "child", + [{"id": "review", "type": "gate", "message": date(2026, 1, 1)}], + ), + ) + _install( + project_dir, + "parent", + _workflow("parent", [{"id": "c", "type": "workflow", "workflow": "child"}]), + ) + state = _run(project_dir, "parent") + assert state.status == RunStatus.PAUSED + scope = state.workflow_scopes["c"] + snapshot = yaml.safe_load( + (state.runs_dir / "snapshots" / scope["definition_snapshot"]).read_text( + encoding="utf-8" + ) + ) + assert isinstance(snapshot["steps"][0]["message"], date) + assert scope["step_results"]["review"]["output"]["message"] == "2026-01-01" + 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")])) @@ -1034,6 +1080,54 @@ def test_load_rejects_step_without_id(self, project_dir): with pytest.raises(ValueError, match="non-empty string"): RunState.load("r", project_dir) + def _save_state_with_snapshot_ref(self, project_dir, ref): + state = RunState(run_id="r", workflow_id="w", project_root=project_dir) + state.status = RunStatus.PAUSED + state.workflow_scopes = { + "c": { + "workflow_id": "child", + "invocation_id": "c", + "definition_snapshot": ref, + "inputs": {}, + "status": "paused", + "current_step_index": 0, + "step_results": {}, + "workflow_scopes": {}, + } + } + state.save() + return state + + def test_snapshot_definition_loads(self, project_dir): + snap_dir = ( + project_dir / ".specify" / "workflows" / "runs" / "r" / "snapshots" + ) + snap_dir.mkdir(parents=True, exist_ok=True) + (snap_dir / "child-abc123.yml").write_text( + yaml.safe_dump( + _workflow("child", [{"id": "s", "type": "shell", "run": "true"}]) + ), + encoding="utf-8", + ) + self._save_state_with_snapshot_ref(project_dir, "child-abc123.yml") + loaded = RunState.load("r", project_dir) + assert ( + loaded.workflow_scopes["c"]["definition_snapshot"] + == "child-abc123.yml" + ) + + def test_load_rejects_missing_snapshot(self, project_dir): + self._save_state_with_snapshot_ref(project_dir, "child-deadbeef.yml") + with pytest.raises(ValueError, match="missing"): + RunState.load("r", project_dir) + + def test_load_rejects_unsafe_snapshot_ref(self, project_dir): + self._save_state_with_snapshot_ref(project_dir, "../escape.yml") + with pytest.raises( + ValueError, match="Invalid definition snapshot reference" + ): + RunState.load("r", project_dir) + def test_custom_step_scope_loads_without_registration( self, project_dir, monkeypatch ): From 1a96b81f689db54e5df0f0de9a22d48bc8356a79 Mon Sep 17 00:00:00 2001 From: Markus Date: Thu, 24 Sep 2026 17:58:53 +0200 Subject: [PATCH 10/21] fix(workflows): make composed outputs persistence-safe Declared outputs are copied into step results, so YAML-native values such as dates could pass validation and later crash state persistence. Validate output values recursively against strict JSON semantics, preserving valid JSON types and failing unsafe values at the workflow-step boundary before json.dump runs. The runtime guard also covers direct engine callers that skip validation. Dynamic workflow-call failures now retain the resolved target in their output, and the composition documentation declares the inputs used by its example and uses accurate nested-composition terminology. Assisted-by: opencode (model: deepseek-v4.1-flash, supervised) --- docs/reference/workflows.md | 11 ++- src/specify_cli/workflows/composition.py | 42 ++++++++- .../workflows/step/workflow/__init__.py | 11 ++- tests/workflows/test_workflow_composition.py | 86 ++++++++++++++++++- 4 files changed, 144 insertions(+), 6 deletions(-) diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index 6c74fed83b..fe757b95fa 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -555,6 +555,12 @@ inputs: target: type: string required: true + report: + type: string + default: "" + slug: + type: string + default: "" steps: - id: run-selected @@ -640,8 +646,9 @@ 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 +Nested composition is allowed: `A -> B -> D` and `A -> C -> D` are legal +diamonds. A workflow may not appear twice on the active path — `A -> B -> A` is +a cycle and is rejected, so bounded recursion is not supported. Composition is limited to 16 included levels; the root is depth 0 and entering depth 17 is rejected. diff --git a/src/specify_cli/workflows/composition.py b/src/specify_cli/workflows/composition.py index 6685221f19..c596a0deed 100644 --- a/src/specify_cli/workflows/composition.py +++ b/src/specify_cli/workflows/composition.py @@ -15,6 +15,7 @@ from __future__ import annotations import hashlib +import math import os import re import tempfile @@ -138,6 +139,10 @@ def validate_workflow_outputs(definition: WorkflowDefinition) -> list[str]: errors.append( f"Output {name!r} must contain exactly the 'value' field." ) + else: + error = _json_value_error(entry["value"], path="'value'") + if error: + errors.append(f"Output {name!r}: {error}.") return errors @@ -274,6 +279,37 @@ def bind_composed_inputs( # -- Output evaluation ---------------------------------------------------- +def _json_value_error(value: Any, *, path: str) -> str | None: + """Return why *value* cannot be persisted as strict JSON, if any. + + Workflow outputs are copied into ``step_results`` and written with + ``json.dump``. Reject values that would crash persistence or be silently + changed by JSON object-key coercion; valid JSON values retain their exact + type across the composition boundary. + """ + if value is None or isinstance(value, (str, bool, int)): + return None + if isinstance(value, float): + if math.isfinite(value): + return None + return f"{path} must be a finite JSON number" + if isinstance(value, list): + for index, item in enumerate(value): + error = _json_value_error(item, path=f"{path}[{index}]") + if error: + return error + return None + if isinstance(value, dict): + for key, item in value.items(): + if not isinstance(key, str): + return f"{path} has a non-string key of type {type(key).__name__}" + error = _json_value_error(item, path=f"{path}.{key}") + if error: + return error + return None + return f"{path} is not JSON-safe (got {type(value).__name__})" + + def evaluate_composed_outputs( definition: WorkflowDefinition, scope: ExecutionScope ) -> dict[str, Any]: @@ -286,7 +322,11 @@ def evaluate_composed_outputs( for name, entry in outputs.items(): if not isinstance(entry, dict) or "value" not in entry: continue - result[name] = evaluate_expression(entry["value"], context) + value = evaluate_expression(entry["value"], context) + error = _json_value_error(value, path=f"Output {name!r}") + if error: + raise ValueError(f"{error}.") + result[name] = value return result diff --git a/src/specify_cli/workflows/step/workflow/__init__.py b/src/specify_cli/workflows/step/workflow/__init__.py index 0cbd0ecfe9..4836e06db7 100644 --- a/src/specify_cli/workflows/step/workflow/__init__.py +++ b/src/specify_cli/workflows/step/workflow/__init__.py @@ -28,6 +28,11 @@ class WorkflowStep(StepBase): def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: step_id = config.get("id", "?") + # Seed with the authored value so a failure *before* the target resolves + # still reports something; once it resolves, the except below reports + # the resolved id (matching the success and invalid-ID paths) instead of + # the ``{{ ... }}`` expression. + target: Any = config.get("workflow") try: from specify_cli.workflows.engine import _ID_PATTERN from specify_cli.workflows.overlay.schema import ( @@ -93,11 +98,13 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: ) except Exception as exc: # noqa: BLE001 # Runtime resolution failures become a failed step result so the - # caller's normal continue_on_error handling applies. + # caller's normal continue_on_error handling applies. Report the + # resolved ``target`` when it was reached, falling back to the + # authored value only for a failure before resolution. return StepResult( status=StepStatus.FAILED, output={ - "workflow": config.get("workflow"), + "workflow": target, "status": StepStatus.FAILED.value, }, error=f"Workflow step {step_id!r}: {exc}", diff --git a/tests/workflows/test_workflow_composition.py b/tests/workflows/test_workflow_composition.py index 8b889edd05..ffc307c517 100644 --- a/tests/workflows/test_workflow_composition.py +++ b/tests/workflows/test_workflow_composition.py @@ -13,7 +13,7 @@ import pytest import yaml -from specify_cli.workflows.base import RunStatus, StepContext +from specify_cli.workflows.base import RunStatus, StepContext, StepStatus from specify_cli.workflows.composition import ( MAX_COMPOSITION_DEPTH, RESERVED_OUTPUT_NAMES, @@ -128,6 +128,67 @@ def test_non_mapping_entry_rejected(self): errors = self._errors({"result": "x"}) assert any("must be a mapping" in e for e in errors) + def test_non_json_safe_value_rejected(self): + # An unquoted YAML date is a valid scalar but not a string expression; + # accepting it would later crash json.dump on the declared output. + from datetime import date + + errors = self._errors({"when": {"value": date(2026, 1, 1)}}) + assert any("not JSON-safe" in e for e in errors) + + @pytest.mark.parametrize( + "value", + [True, 42, 3.14, None, ["a", 1], {"ok": True, "items": [1, 2]}], + ) + def test_json_safe_literal_values_accepted(self, value): + assert self._errors({"result": {"value": value}}) == [] + + def test_nested_non_json_safe_value_rejected(self): + from datetime import date + + errors = self._errors( + {"result": {"value": {"when": [date(2026, 1, 1)]}}} + ) + assert any("not JSON-safe" in e for e in errors) + + def test_non_string_object_key_rejected(self): + errors = self._errors({"result": {"value": {1: "numeric"}}}) + assert any("non-string key" in e for e in errors) + + +class TestComposedOutputPersistence: + def test_non_json_safe_evaluated_output_fails_workflow_step(self, project_dir): + """Output persistence errors become a failed workflow-step result. + + The aggregate boundary catches output evaluation failures so the caller + can still use normal ``continue_on_error`` handling instead of crashing + later in ``json.dump``. ``WorkflowEngine.execute`` accepts unvalidated + definitions, so retain the runtime guard in addition to validation. + """ + from datetime import date + + from specify_cli.workflows.composition import ExecutionScope + + definition = WorkflowDefinition( + _workflow( + "child", + [_shell("s", "true")], + outputs={"when": {"value": date(2026, 1, 1)}}, + ) + ) + scope = ExecutionScope( + scope_id="call", + workflow_id="child", + definition=definition, + ) + result = WorkflowEngine(project_dir)._aggregate_workflow_result( + scope, definition, "call" + ) + + assert result.status == StepStatus.FAILED + assert scope.status == RunStatus.FAILED + assert "not JSON-safe" in (result.error or "") + class TestWorkflowCallConfigValidation: def test_literal_valid(self): @@ -752,6 +813,29 @@ def test_non_string_expression_target(self, project_dir): assert state.step_results["c"]["status"] == "failed" assert "expected a string" in state.step_results["c"]["error"] + def test_dynamic_target_failure_reports_resolved_id(self, project_dir): + """A post-resolution failure reports the resolved id, not the template.""" + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + _shell("pick", "echo missing-wf"), + { + "id": "c", + "type": "workflow", + "workflow": "{{ steps.pick.output.stdout }}", + "continue_on_error": True, + }, + ], + ), + ) + state = _run(project_dir, "parent") + assert state.step_results["c"]["status"] == "failed" + assert state.step_results["c"]["output"]["workflow"] == "missing-wf" + assert "not installed" in state.step_results["c"]["error"] + def test_malformed_input_mapping_fails_step(self, project_dir): """A non-mapping ``input`` must fail, not run the child on defaults. From 95eed5423ae7e16e3085987a507e2355ac7fc35c Mon Sep 17 00:00:00 2001 From: Markus Date: Thu, 24 Sep 2026 19:47:06 +0200 Subject: [PATCH 11/21] fix(workflows): persist failed composed rebind state Reject non-JSON-safe workflow-call input values at validation and evaluation time, and record a safe empty mapping when an unvalidated caller fails before input binding. This preserves the failed step result instead of letting a YAML native scalar crash json persistence. A failed rebind of a paused child now marks the child failed and persists its error in the same atomic write as the failed caller result, preventing a parent that continues on error from leaving a stale paused subtree behind. Assisted-by: opencode (model: deepseek-v4.1-flash, supervised) --- src/specify_cli/workflows/composition.py | 31 ++++- src/specify_cli/workflows/engine.py | 21 +++- tests/workflows/test_workflow_composition.py | 119 +++++++++++++++++++ 3 files changed, 163 insertions(+), 8 deletions(-) diff --git a/src/specify_cli/workflows/composition.py b/src/specify_cli/workflows/composition.py index c596a0deed..e06e77fffd 100644 --- a/src/specify_cli/workflows/composition.py +++ b/src/specify_cli/workflows/composition.py @@ -180,11 +180,15 @@ def validate_workflow_call_config(config: dict[str, Any]) -> list[str]: f"Workflow step {step_id!r}: 'input' must be a mapping." ) elif isinstance(input_mapping, dict): - for key in input_mapping: + for key, value in input_mapping.items(): if not isinstance(key, str): errors.append( f"Workflow step {step_id!r}: 'input' keys must be strings." ) + continue + error = _json_value_error(value, path=f"'input.{key}'") + if error: + errors.append(f"Workflow step {step_id!r}: {error}.") return errors @@ -212,10 +216,17 @@ def evaluate_input_mapping( f"{type(mapping).__name__}." ) raise ValueError(msg) - return { - name: evaluate_expression(value, context) - for name, value in mapping.items() - } + evaluated: dict[str, Any] = {} + for name, value in mapping.items(): + if not isinstance(name, str): + msg = "'input' keys must be strings." + raise ValueError(msg) + resolved = evaluate_expression(value, context) + error = _json_value_error(resolved, path=f"Input {name!r}") + if error: + raise ValueError(f"{error}.") + evaluated[name] = resolved + return evaluated def bind_composed_inputs( @@ -581,6 +592,7 @@ def _serialize(self) -> dict[str, Any]: "workflow_dir": self.workflow_dir, "inputs": self.inputs, "status": self.status.value, + "error": self.error, "current_step_index": self.current_step_index, "current_step_id": self.current_step_id, "step_results": self.step_results, @@ -679,6 +691,7 @@ def deserialize_scope( current_step_index=record.get("current_step_index", 0), current_step_id=record.get("current_step_id"), status=RunStatus(record.get("status", RunStatus.RUNNING.value)), + error=record.get("error"), parent=parent, root_state=root_state, ) @@ -859,6 +872,14 @@ def _validate_scope_record( msg = f"Invalid run state: '{path}.status' is invalid: {status!r}" raise ValueError(msg) from None + error = record.get("error") + if error is not None and not isinstance(error, str): + msg = ( + f"Invalid run state: '{path}.error' must be a string or null, " + f"got {error!r}" + ) + raise ValueError(msg) + definition = _resolve_scope_definition(record, run_dir=run_dir, path=path) _validate_definition_shape(definition, path=f"{path}.definition") diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index be017c18dc..aa2c49ca72 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -1384,6 +1384,16 @@ def _execute_steps( result = step_impl.execute(step_config, context) # Record step results — prefer resolved values from step output + if step_type == "workflow" and result.status == StepStatus.FAILED: + # A failed workflow call may have rejected its authored input + # mapping before evaluation. Never fall back to that raw YAML + # value here: it can contain a non-JSON scalar (for example a + # date), which would make recording the failure crash. + recorded_input = result.output.get("input", {}) + else: + recorded_input = result.output.get("input") or step_config.get( + "input", {} + ) step_data = { "type": step_type, "integration": result.output.get("integration") @@ -1394,8 +1404,7 @@ def _execute_steps( or context.default_model, "options": result.output.get("options") or step_config.get("options", {}), - "input": result.output.get("input") - or step_config.get("input", {}), + "input": recorded_input, "output": result.output, "status": result.status.value, "error": result.error, @@ -1673,13 +1682,19 @@ def _run_workflow_call( caller_id=effective_id, ) except ValueError as exc: + error = f"Workflow step {effective_id!r}: {exc}" + # The caller will record its failed workflow step through + # record_and_save(), which atomically persists this failed + # child state and the caller result together. + child_scope.status = RunStatus.FAILED + child_scope.error = error return StepResult( status=StepStatus.FAILED, output={ "workflow": definition.id, "status": RunStatus.FAILED.value, }, - error=f"Workflow step {effective_id!r}: {exc}", + error=error, ) child_scope.persist() child_scope.status = RunStatus.RUNNING diff --git a/tests/workflows/test_workflow_composition.py b/tests/workflows/test_workflow_composition.py index ffc307c517..082f61955f 100644 --- a/tests/workflows/test_workflow_composition.py +++ b/tests/workflows/test_workflow_composition.py @@ -222,6 +222,18 @@ def test_non_mapping_input_rejected(self): ) assert any("'input' must be a mapping" in e for e in errors) + def test_non_json_safe_input_value_rejected(self): + from datetime import date + + errors = validate_workflow_call_config( + { + "id": "s", + "workflow": "bugfix", + "input": {"when": date(2026, 1, 1)}, + } + ) + assert any("not JSON-safe" in e for e in errors) + class TestEvaluateInputMapping: def test_omitted_returns_empty(self): @@ -243,6 +255,16 @@ def test_values_evaluated_in_caller_scope(self): {"name": "{{ inputs.who }}", "literal": "x"}, context ) == {"name": "world", "literal": "x"} + def test_non_json_safe_value_rejected(self): + from datetime import date + + with pytest.raises(ValueError, match="not JSON-safe"): + evaluate_input_mapping({"when": date(2026, 1, 1)}, StepContext()) + + def test_non_string_key_rejected(self): + with pytest.raises(ValueError, match="keys must be strings"): + evaluate_input_mapping({1: "x"}, StepContext()) + class TestCheckCompositionPath: def test_cycle_reported_before_depth(self): @@ -875,6 +897,50 @@ def test_malformed_input_mapping_fails_step(self, project_dir): # runs on silently-discarded inputs. assert "c" not in state.workflow_scopes + def test_non_json_safe_input_mapping_persists_clean_failure(self, project_dir): + """Unsafe authored inputs must not crash recording the failure result. + + ``_run`` intentionally bypasses definition validation, covering direct + engine callers that hand a YAML-native scalar to a workflow step. + """ + from datetime import date + + _install( + project_dir, + "child", + _workflow( + "child", + [_shell("x", "echo {{ inputs.who }}")], + inputs={"who": {"type": "string", "default": "world"}}, + ), + ) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "c", + "type": "workflow", + "workflow": "child", + "input": {"who": date(2026, 1, 1)}, + "continue_on_error": True, + } + ], + ), + ) + state = _run(project_dir, "parent") + result = state.step_results["c"] + assert state.status == RunStatus.COMPLETED + assert result["status"] == "failed" + assert "not JSON-safe" in result["error"] + assert result["input"] == {} + assert "c" not in state.workflow_scopes + + loaded = RunState.load(state.run_id, project_dir) + assert loaded.step_results["c"]["input"] == {} + class TestRecursionAndDepth: def test_cycle_rejected(self, project_dir): @@ -1437,6 +1503,59 @@ def test_resume_input_update_forwards_through_mapping(self, project_dir): assert resumed.status == RunStatus.COMPLETED assert resumed.workflow_scopes["c"]["inputs"]["verdict"] == "approve" + def test_rebind_failure_marks_paused_child_failed(self, project_dir): + """A rejected resumed binding cannot leave a bypassed child paused.""" + _install( + project_dir, + "child", + _workflow( + "child", + [{"id": "g", "type": "gate", "message": "ok?", "options": ["approve", "reject"]}], + inputs={ + "mode": { + "type": "string", + "default": "", + "enum": ["approve", ""], + } + }, + ), + ) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "c", + "type": "workflow", + "workflow": "child", + "input": {"mode": "{{ inputs.mode }}"}, + "continue_on_error": True, + }, + _shell("after", "echo continued"), + ], + inputs={"mode": {"type": "string", "default": ""}}, + ), + ) + engine = WorkflowEngine(project_dir) + state = engine.execute(_definition(project_dir, "parent"), {}) + assert state.status == RunStatus.PAUSED + assert state.workflow_scopes["c"]["status"] == "paused" + + resumed = engine.resume(state.run_id, {"mode": "invalid"}) + child = resumed.workflow_scopes["c"] + caller = resumed.step_results["c"] + assert resumed.status == RunStatus.COMPLETED + assert resumed.step_results["after"]["output"]["stdout"].strip() == "continued" + assert caller["status"] == "failed" + assert child["status"] == "failed" + assert child["error"] == caller["error"] + + loaded = RunState.load(resumed.run_id, project_dir) + assert loaded.workflow_scopes["c"]["status"] == "failed" + assert loaded.workflow_scopes["c"]["error"] == caller["error"] + def test_resume_without_input_updates_keeps_binding(self, project_dir): self._paused_child( project_dir, From 344c4e1e3ceea88b751aa1de4ddde7ffe3a96c24 Mon Sep 17 00:00:00 2001 From: Markus Date: Thu, 24 Sep 2026 20:05:47 +0200 Subject: [PATCH 12/21] fix(workflows): harden composed workflow failures Address nested gate reporting, circular JSON validation, abort status, and persistence-safe invalid targets.\n\nAssisted-by: OpenCode (model: gpt-5.6-terra, autonomous) --- src/specify_cli/workflows/_commands.py | 4 +- src/specify_cli/workflows/composition.py | 43 ++++++---- src/specify_cli/workflows/engine.py | 2 +- .../workflows/step/workflow/__init__.py | 20 ++++- tests/workflows/test_workflow_composition.py | 86 ++++++++++++++++++- 5 files changed, 133 insertions(+), 22 deletions(-) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index 2e1365960a..d9fec453a0 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -1047,12 +1047,12 @@ def _scope_gate(scopes: Any, path: list[str]) -> dict[str, Any] | None: if not isinstance(record, dict): continue child_path = [*path, key] + if str(record.get("status")) not in ("paused", "aborted"): + continue # A deeper paused scope is more specific; check descendants first. nested = _scope_gate(record.get("workflow_scopes"), child_path) if nested is not None: return nested - if str(record.get("status")) not in ("paused", "aborted"): - continue step_id = _scope_current_step_id(record) step_results = record.get("step_results") if step_id is None or not isinstance(step_results, dict): diff --git a/src/specify_cli/workflows/composition.py b/src/specify_cli/workflows/composition.py index e06e77fffd..85d2722039 100644 --- a/src/specify_cli/workflows/composition.py +++ b/src/specify_cli/workflows/composition.py @@ -290,7 +290,9 @@ def bind_composed_inputs( # -- Output evaluation ---------------------------------------------------- -def _json_value_error(value: Any, *, path: str) -> str | None: +def _json_value_error( + value: Any, *, path: str, ancestors: set[int] | None = None +) -> str | None: """Return why *value* cannot be persisted as strict JSON, if any. Workflow outputs are copied into ``step_results`` and written with @@ -304,20 +306,31 @@ def _json_value_error(value: Any, *, path: str) -> str | None: if math.isfinite(value): return None return f"{path} must be a finite JSON number" - if isinstance(value, list): - for index, item in enumerate(value): - error = _json_value_error(item, path=f"{path}[{index}]") - if error: - return error - return None - if isinstance(value, dict): - for key, item in value.items(): - if not isinstance(key, str): - return f"{path} has a non-string key of type {type(key).__name__}" - error = _json_value_error(item, path=f"{path}.{key}") - if error: - return error - return None + if isinstance(value, (list, dict)): + ancestors = ancestors if ancestors is not None else set() + if id(value) in ancestors: + return f"{path} contains a circular container" + ancestors.add(id(value)) + try: + if isinstance(value, list): + for index, item in enumerate(value): + error = _json_value_error( + item, path=f"{path}[{index}]", ancestors=ancestors + ) + if error: + return error + return None + for key, item in value.items(): + if not isinstance(key, str): + return f"{path} has a non-string key of type {type(key).__name__}" + error = _json_value_error( + item, path=f"{path}.{key}", ancestors=ancestors + ) + if error: + return error + return None + finally: + ancestors.remove(id(value)) return f"{path} is not JSON-safe (got {type(value).__name__})" diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index aa2c49ca72..ff5039146a 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -1840,7 +1840,7 @@ def _aggregate_workflow_result( status=StepStatus.FAILED, output={ "workflow": definition.id, - "status": RunStatus.FAILED.value, + "status": RunStatus.ABORTED.value, "aborted": True, }, error=child_scope.error, diff --git a/src/specify_cli/workflows/step/workflow/__init__.py b/src/specify_cli/workflows/step/workflow/__init__.py index 4836e06db7..a4ed50488f 100644 --- a/src/specify_cli/workflows/step/workflow/__init__.py +++ b/src/specify_cli/workflows/step/workflow/__init__.py @@ -14,6 +14,7 @@ from specify_cli.workflows.base import StepBase, StepContext, StepResult, StepStatus from specify_cli.workflows.composition import ( + _json_value_error, evaluate_input_mapping, resolve_composed_workflow, validate_workflow_call_config, @@ -26,6 +27,13 @@ class WorkflowStep(StepBase): type_key = "workflow" + @staticmethod + def _safe_target(value: Any) -> Any: + """Keep failure diagnostics safe for the JSON-backed run state.""" + if _json_value_error(value, path="'workflow'") is None: + return value + return f"<{type(value).__name__}>" + def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: step_id = config.get("id", "?") # Seed with the authored value so a failure *before* the target resolves @@ -43,7 +51,10 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: if not isinstance(target_expr, str): return StepResult( status=StepStatus.FAILED, - output={"workflow": target_expr, "status": StepStatus.FAILED.value}, + output={ + "workflow": self._safe_target(target_expr), + "status": StepStatus.FAILED.value, + }, error=( f"Workflow step {step_id!r}: 'workflow' must be a string." ), @@ -53,7 +64,10 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: if not isinstance(target, str): return StepResult( status=StepStatus.FAILED, - output={"workflow": target, "status": StepStatus.FAILED.value}, + output={ + "workflow": self._safe_target(target), + "status": StepStatus.FAILED.value, + }, error=( f"Workflow step {step_id!r}: 'workflow' expression " f"resolved to {type(target).__name__}, expected a string." @@ -104,7 +118,7 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: return StepResult( status=StepStatus.FAILED, output={ - "workflow": target, + "workflow": self._safe_target(target), "status": StepStatus.FAILED.value, }, error=f"Workflow step {step_id!r}: {exc}", diff --git a/tests/workflows/test_workflow_composition.py b/tests/workflows/test_workflow_composition.py index 082f61955f..920de0c4ad 100644 --- a/tests/workflows/test_workflow_composition.py +++ b/tests/workflows/test_workflow_composition.py @@ -155,6 +155,22 @@ def test_non_string_object_key_rejected(self): errors = self._errors({"result": {"value": {1: "numeric"}}}) assert any("non-string key" in e for e in errors) + @pytest.mark.parametrize("factory", [list, dict]) + def test_circular_container_rejected(self, factory): + value = factory() + if isinstance(value, list): + value.append(value) + else: + value["self"] = value + + errors = self._errors({"result": {"value": value}}) + + assert any("circular container" in error for error in errors) + + def test_shared_acyclic_container_accepted(self): + shared = ["value"] + assert self._errors({"result": {"value": {"one": shared, "two": shared}}}) == [] + class TestComposedOutputPersistence: def test_non_json_safe_evaluated_output_fails_workflow_step(self, project_dir): @@ -578,7 +594,10 @@ def test_aborted_shape(self, project_dir): ) state = _run(project_dir, "parent") assert state.status == RunStatus.ABORTED - assert state.step_results["c"]["output"].get("aborted") is True + result = state.step_results["c"] + assert result["status"] == "failed" + assert result["output"]["status"] == "aborted" + assert result["output"].get("aborted") is True class TestContinueOnError: @@ -941,6 +960,41 @@ def test_non_json_safe_input_mapping_persists_clean_failure(self, project_dir): loaded = RunState.load(state.run_id, project_dir) assert loaded.step_results["c"]["input"] == {} + def test_non_json_safe_dynamic_target_persists_clean_failure( + self, project_dir, monkeypatch + ): + """Invalid target diagnostics must not make the failed call unsaveable.""" + from datetime import date + + monkeypatch.setattr( + "specify_cli.workflows.step.workflow.evaluate_expression", + lambda *_: date(2026, 1, 1), + ) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "c", + "type": "workflow", + "workflow": "{{ inputs.target }}", + "continue_on_error": True, + } + ], + ), + ) + + state = _run(project_dir, "parent") + result = state.step_results["c"] + + assert state.status == RunStatus.COMPLETED + assert result["status"] == "failed" + assert result["output"]["workflow"] == "" + assert "expected a string" in result["error"] + assert RunState.load(state.run_id, project_dir).step_results["c"] == result + class TestRecursionAndDepth: def test_cycle_rejected(self, project_dir): @@ -1897,6 +1951,36 @@ def test_deeply_nested_gate_reports_scope_path(self, project_dir): assert payload["gate"]["scope_path"] == ["c", "inner"] assert payload["gate"]["step_id"] == "g" + def test_inactive_scope_cannot_report_a_stale_nested_gate(self): + from specify_cli.workflows._commands import _scope_gate + + stale_gate = { + "status": "paused", + "current_step_id": "stale", + "step_results": { + "stale": {"type": "gate", "output": {"message": "Old gate"}} + }, + } + active_gate = { + "status": "paused", + "current_step_id": "active", + "step_results": { + "active": {"type": "gate", "output": {"message": "Active gate"}} + }, + } + + gate = _scope_gate( + { + "completed": {"status": "completed", "workflow_scopes": {"old": stale_gate}}, + "active": active_gate, + }, + [], + ) + + assert gate is not None + assert gate["step_id"] == "active" + assert gate["scope_path"] == ["active"] + def test_gate_inside_nested_control_flow_reports_gate(self, project_dir): """A gate inside an ``if`` body must be reported, not its enclosing step. From 0dbacc62eea984b3c871f08a77e320479e73a737 Mon Sep 17 00:00:00 2001 From: Markus Date: Thu, 24 Sep 2026 20:43:43 +0200 Subject: [PATCH 13/21] test(workflows): make composed resume test portable Replace the POSIX-only shell command with a test-local fail-once step so the failed child retry is exercised consistently on Windows.\n\nAssisted-by: OpenCode (model: gpt-5.6-terra, autonomous) --- tests/workflows/test_workflow_composition.py | 22 +++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/tests/workflows/test_workflow_composition.py b/tests/workflows/test_workflow_composition.py index 920de0c4ad..6b29d6d205 100644 --- a/tests/workflows/test_workflow_composition.py +++ b/tests/workflows/test_workflow_composition.py @@ -1414,14 +1414,29 @@ def test_pause_resumes_from_scope_index(self, project_dir): 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" + def test_failed_call_retries_on_resume(self, project_dir, monkeypatch): + from specify_cli.workflows import STEP_REGISTRY + from specify_cli.workflows.base import StepBase, StepResult + + calls = 0 + + class _FailOnce(StepBase): + type_key = "fail-once" + + def execute(self, config, context): + nonlocal calls + calls += 1 + if calls == 1: + return StepResult(status=StepStatus.FAILED, error="first attempt") + return StepResult(status=StepStatus.COMPLETED) + + monkeypatch.setitem(STEP_REGISTRY, "fail-once", _FailOnce()) _install( project_dir, "child", _workflow( "child", - [_shell("x", f"test -f {marker} || {{ touch {marker}; exit 1; }}")], + [{"id": "x", "type": "fail-once"}], ), ) _install( @@ -1440,6 +1455,7 @@ def test_failed_call_retries_on_resume(self, project_dir): resumed = engine.resume(state.run_id) assert resumed.status == RunStatus.COMPLETED assert resumed.workflow_scopes["c"]["status"] == "completed" + assert calls == 2 def test_failed_output_evaluation_retries_on_resume(self, project_dir): marker = project_dir / "valid-json" From 2b683c5d829b455058683c06615af524abdc0704 Mon Sep 17 00:00:00 2001 From: Markus Date: Thu, 24 Sep 2026 21:05:23 +0200 Subject: [PATCH 14/21] fix(workflows): atomically persist failed rebinds Stage resumed child binding failures until the caller result is recorded under the run lock, preventing concurrent fan-out saves from persisting an unpaired child failure.\n\nAssisted-by: OpenCode (model: gpt-5.6-terra, autonomous) --- src/specify_cli/workflows/composition.py | 32 ++++++-- src/specify_cli/workflows/engine.py | 11 +-- tests/workflows/test_workflow_composition.py | 80 +++++++++++++++++++- 3 files changed, 110 insertions(+), 13 deletions(-) diff --git a/src/specify_cli/workflows/composition.py b/src/specify_cli/workflows/composition.py index 85d2722039..0c3aa8968e 100644 --- a/src/specify_cli/workflows/composition.py +++ b/src/specify_cli/workflows/composition.py @@ -521,6 +521,10 @@ class ExecutionScope: current_step_id: str | None = None status: RunStatus = RunStatus.RUNNING error: str | None = None + # A workflow caller stages a terminal child transition here so its caller + # result can be recorded in the same locked state write. + pending_terminal_status: RunStatus | None = None + pending_terminal_error: str | None = None workflow_scopes: dict[str, ExecutionScope] = field(default_factory=dict) parent: ExecutionScope | None = None root_state: RunState | None = None @@ -657,23 +661,37 @@ def record_and_save( *, complete_child: bool = False, ) -> None: - """Record a step result and complete its child in one locked write. + """Record a step result and finalize its child in one locked write. - Used for the workflow-call boundary so a persisted ``COMPLETED`` child - can never lack its caller-step result. + Used for the workflow-call boundary so a persisted terminal 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 + child = self.workflow_scopes.get(step_id) + if child is not None: + if complete_child: + child.status = RunStatus.COMPLETED + elif child.pending_terminal_status is not None: + child.status = child.pending_terminal_status + child.error = child.pending_terminal_error + child.pending_terminal_status = None + child.pending_terminal_error = None 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 + child = self.workflow_scopes.get(step_id) + if child is not None: + if complete_child: + child.status = RunStatus.COMPLETED + elif child.pending_terminal_status is not None: + child.status = child.pending_terminal_status + child.error = child.pending_terminal_error + child.pending_terminal_status = None + child.pending_terminal_error = None if context.steps is not self.step_results: context.steps[step_id] = data self.step_results[step_id] = data diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index ff5039146a..ded0cf8f6d 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -1683,11 +1683,12 @@ def _run_workflow_call( ) except ValueError as exc: error = f"Workflow step {effective_id!r}: {exc}" - # The caller will record its failed workflow step through - # record_and_save(), which atomically persists this failed - # child state and the caller result together. - child_scope.status = RunStatus.FAILED - child_scope.error = error + # Stage, rather than mutate, the terminal child state. The + # caller records both sides through record_and_save(), so a + # concurrent fan-out save cannot persist one without the + # other. + child_scope.pending_terminal_status = RunStatus.FAILED + child_scope.pending_terminal_error = error return StepResult( status=StepStatus.FAILED, output={ diff --git a/tests/workflows/test_workflow_composition.py b/tests/workflows/test_workflow_composition.py index 6b29d6d205..e873d94f36 100644 --- a/tests/workflows/test_workflow_composition.py +++ b/tests/workflows/test_workflow_composition.py @@ -8,7 +8,7 @@ import json from pathlib import Path -from threading import Event +from threading import Event, Thread import pytest import yaml @@ -1626,6 +1626,84 @@ def test_rebind_failure_marks_paused_child_failed(self, project_dir): assert loaded.workflow_scopes["c"]["status"] == "failed" assert loaded.workflow_scopes["c"]["error"] == caller["error"] + def test_concurrent_save_cannot_persist_unpaired_rebind_failure( + self, project_dir, monkeypatch + ): + """A failed rebinding is committed with its caller result atomically.""" + from specify_cli.workflows.composition import ExecutionScope + + _install( + project_dir, + "child", + _workflow( + "child", + [ + { + "id": "g", + "type": "gate", + "message": "ok?", + "options": ["approve", "reject"], + } + ], + inputs={ + "mode": { + "type": "string", + "default": "", + "enum": ["approve", ""], + } + }, + ), + ) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "c", + "type": "workflow", + "workflow": "child", + "input": {"mode": "{{ inputs.mode }}"}, + "continue_on_error": True, + } + ], + inputs={"mode": {"type": "string", "default": ""}}, + ), + ) + engine = WorkflowEngine(project_dir) + state = engine.execute(_definition(project_dir, "parent"), {}) + assert state.status == RunStatus.PAUSED + + snapshots: list[dict] = [] + real_record_and_save = ExecutionScope.record_and_save + + def coordinated_handoff(self, context, step_id, data, **kwargs): + if step_id == "c": + worker = Thread(target=self.persist) + worker.start() + worker.join(timeout=5) + assert not worker.is_alive(), "concurrent save did not complete" + snapshots.append( + json.loads( + (self.root().root_state.runs_dir / "state.json").read_text( + encoding="utf-8" + ) + ) + ) + return real_record_and_save(self, context, step_id, data, **kwargs) + + monkeypatch.setattr(ExecutionScope, "record_and_save", coordinated_handoff) + resumed = engine.resume(state.run_id, {"mode": "invalid"}) + + assert resumed.status == RunStatus.COMPLETED + assert len(snapshots) == 1 + snapshot = snapshots[0] + assert snapshot["workflow_scopes"]["c"]["status"] == "paused" + assert snapshot["step_results"]["c"]["status"] == "paused" + assert resumed.workflow_scopes["c"]["status"] == "failed" + assert resumed.step_results["c"]["status"] == "failed" + def test_resume_without_input_updates_keeps_binding(self, project_dir): self._paused_child( project_dir, From 3c14c9d5c12de51668495092eac8569edee07ec0 Mon Sep 17 00:00:00 2001 From: Markus Date: Fri, 25 Sep 2026 08:25:45 +0200 Subject: [PATCH 15/21] fix(workflows): preserve composed scope identity Require registry and definition IDs to match, restore exact target matching, and attribute nested execution logs to their invocation scope.\n\nAssisted-by: OpenCode (model: gpt-5.6-terra, autonomous) --- docs/reference/workflows.md | 6 +- src/specify_cli/workflows/composition.py | 12 ++++ .../workflows/step/workflow/__init__.py | 5 -- tests/workflows/test_workflow_composition.py | 56 ++++++++++++++++--- 4 files changed, 64 insertions(+), 15 deletions(-) diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index fe757b95fa..29295b2ef1 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -580,8 +580,10 @@ The `workflow` expression must resolve to a value the engine captures: a declared workflow input or a preceding step's captured output (for example a `shell` step's `stdout`) both work. A `prompt` step streams its output to the agent and returns an empty `stdout`, so it cannot drive `workflow` selection. -A dynamically resolved string is whitespace-trimmed before ID validation, so a -`shell` step using `echo child` selects `child` despite the trailing newline. +The resolved value must exactly match the installed workflow ID; it is not +trimmed, case-normalized, or otherwise transformed. A shell-driven selection +must therefore emit only the ID, for example `printf child` rather than +`echo child` (which includes a trailing newline). `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 diff --git a/src/specify_cli/workflows/composition.py b/src/specify_cli/workflows/composition.py index 0c3aa8968e..e1eb267470 100644 --- a/src/specify_cli/workflows/composition.py +++ b/src/specify_cli/workflows/composition.py @@ -110,6 +110,12 @@ def resolve_composed_workflow( f"Workflow {workflow_id!r} is invalid: " + " ".join(errors) ) raise ValueError(msg) + if definition.id != workflow_id: + msg = ( + f"Workflow registry entry {workflow_id!r} resolves to a definition " + f"with ID {definition.id!r}." + ) + raise ValueError(msg) return definition @@ -576,6 +582,12 @@ 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: + if self.parent is not None: + entry = { + **entry, + "scope_path": _invocation_path(self)[1:], + "workflow_id": self.workflow_id, + } state.append_log(entry) def build_context(self, *, is_resume: bool = False) -> StepContext: diff --git a/src/specify_cli/workflows/step/workflow/__init__.py b/src/specify_cli/workflows/step/workflow/__init__.py index a4ed50488f..d410553209 100644 --- a/src/specify_cli/workflows/step/workflow/__init__.py +++ b/src/specify_cli/workflows/step/workflow/__init__.py @@ -73,11 +73,6 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: f"resolved to {type(target).__name__}, expected a string." ), ) - # A dynamic target is usually a step's captured stdout, which keeps - # its trailing newline (``echo child`` -> ``\"child\\n\"``) and would - # otherwise fail ID validation. Trim surrounding whitespace; literal - # targets are already pattern-safe, so this is a no-op for them. - target = target.strip() if ( not _ID_PATTERN.fullmatch(target) or target in _RESERVED_WORKFLOW_IDS diff --git a/tests/workflows/test_workflow_composition.py b/tests/workflows/test_workflow_composition.py index e873d94f36..a5ccece202 100644 --- a/tests/workflows/test_workflow_composition.py +++ b/tests/workflows/test_workflow_composition.py @@ -20,6 +20,7 @@ bind_composed_inputs, check_composition_path, evaluate_input_mapping, + resolve_composed_workflow, validate_workflow_call_config, ) from specify_cli.workflows.engine import ( @@ -388,12 +389,8 @@ def test_runtime_selected_target(self, project_dir): assert state.status == RunStatus.COMPLETED assert state.step_results["call"]["output"]["workflow"] == "child" - def test_runtime_target_from_echo_is_trimmed(self, project_dir): - """``echo`` adds a trailing newline; the resolved target is trimmed. - - Dynamic selection must work with an ordinary ``echo child``, not only - with newline-free commands like ``printf child``. - """ + def test_runtime_target_from_echo_requires_exact_id(self, project_dir): + """A dynamic target is not normalized before installed-ID matching.""" _install(project_dir, "child", _workflow("child", [_shell("x", "echo hi")])) _install( project_dir, @@ -406,13 +403,23 @@ def test_runtime_target_from_echo_is_trimmed(self, project_dir): "id": "call", "type": "workflow", "workflow": "{{ steps.pick.output.stdout }}", + "continue_on_error": True, }, ], ), ) state = _run(project_dir, "parent") assert state.status == RunStatus.COMPLETED - assert state.step_results["call"]["output"]["workflow"] == "child" + result = state.step_results["call"] + assert result["status"] == "failed" + assert result["output"]["workflow"] == "child\n" + assert "not a valid workflow ID" in result["error"] + + def test_registry_key_must_match_resolved_definition_id(self, project_dir): + _install(project_dir, "child", _workflow("other", [_shell("x", "echo hi")])) + + with pytest.raises(ValueError, match="registry entry 'child'.*ID 'other'"): + resolve_composed_workflow(project_dir, "child") class TestScopeIsolation: @@ -481,6 +488,39 @@ def test_caller_can_consume_child_output_downstream(self, project_dir): assert state.step_results["consume"]["output"]["stdout"].strip() == "secret" +class TestScopeLogging: + def test_nested_events_include_their_scope_path(self, project_dir): + _install(project_dir, "child", _workflow("child", [_shell("build", "echo child")])) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + _shell("build", "echo parent"), + {"id": "call", "type": "workflow", "workflow": "child"}, + ], + ), + ) + + state = _run(project_dir, "parent") + entries = [ + json.loads(line) + for line in (state.runs_dir / "log.jsonl").read_text(encoding="utf-8").splitlines() + ] + started = [ + entry + for entry in entries + if entry["event"] == "step_started" and entry["step_id"] == "build" + ] + + assert len(started) == 2 + root_entry = next(entry for entry in started if "scope_path" not in entry) + child_entry = next(entry for entry in started if entry.get("scope_path") == ["call"]) + assert root_entry["step_id"] == child_entry["step_id"] == "build" + assert child_entry["workflow_id"] == "child" + + class TestPublicOutputShapes: def test_completed_shape(self, project_dir): _install(project_dir, "child", _workflow("child", [_shell("x", "echo hi")])) @@ -862,7 +902,7 @@ def test_dynamic_target_failure_reports_resolved_id(self, project_dir): _workflow( "parent", [ - _shell("pick", "echo missing-wf"), + _shell("pick", "printf missing-wf"), { "id": "c", "type": "workflow", From d8ce4717ae484568fd178653d3e47c45284914b6 Mon Sep 17 00:00:00 2001 From: Markus Date: Fri, 25 Sep 2026 15:46:16 +0200 Subject: [PATCH 16/21] docs(workflows): clarify gate JSON normalization Assisted-by: OpenCode (model: gpt-5.6-terra, autonomous) --- src/specify_cli/workflows/_commands.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index d9fec453a0..7fb888a65b 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -1017,10 +1017,9 @@ def _is_gate_step(step: dict[str, Any]) -> bool: def _gate_details(step_id: str, output: Any) -> dict[str, Any]: """Normalise a gate step's output into the stable JSON gate schema. - ``message``, ``options``, and ``choice`` may be non-string YAML literals in - an unvalidated workflow (``GateStep`` coerces none of them for the payload), - so all three are normalised: message → str, options → list[str] | None, - choice → str | None (None means no decision yet). + Unvalidated or legacy gate records may contain non-string values, so all + fields are normalised: message → str, options → list[str] | None, choice → + str | None (None means no decision yet). """ output = output if isinstance(output, dict) else {} message = output.get("message") From 3f3fdaa401f49a1f876bceb9c8c8811a8f4d32de Mon Sep 17 00:00:00 2001 From: Markus Date: Fri, 25 Sep 2026 20:52:31 +0200 Subject: [PATCH 17/21] fix(workflows): isolate composed child invocations Assisted-by: OpenCode (model: gpt-5.6-terra, autonomous) --- src/specify_cli/workflows/composition.py | 5 +- src/specify_cli/workflows/engine.py | 114 +++++-- tests/workflows/test_workflow_composition.py | 301 +++++++++++++++++++ 3 files changed, 399 insertions(+), 21 deletions(-) diff --git a/src/specify_cli/workflows/composition.py b/src/specify_cli/workflows/composition.py index e1eb267470..a3c39f8f50 100644 --- a/src/specify_cli/workflows/composition.py +++ b/src/specify_cli/workflows/composition.py @@ -672,6 +672,7 @@ def record_and_save( data: dict[str, Any], *, complete_child: bool = False, + child_scope_id: str | None = None, ) -> None: """Record a step result and finalize its child in one locked write. @@ -681,7 +682,7 @@ def record_and_save( root = self.root() state = root.root_state if state is None: - child = self.workflow_scopes.get(step_id) + child = self.workflow_scopes.get(child_scope_id or step_id) if child is not None: if complete_child: child.status = RunStatus.COMPLETED @@ -695,7 +696,7 @@ def record_and_save( self.step_results[step_id] = data return with state._lock: - child = self.workflow_scopes.get(step_id) + child = self.workflow_scopes.get(child_scope_id or step_id) if child is not None: if complete_child: child.status = RunStatus.COMPLETED diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index ded0cf8f6d..ff6f01bb73 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -1337,6 +1337,7 @@ def _execute_steps( registry: dict[str, Any], *, step_offset: int = 0, + invocation_path: tuple[str, ...] = (), ) -> None: """Execute a list of steps sequentially within *scope*.""" for i, step_config in enumerate(steps): @@ -1378,7 +1379,7 @@ def _execute_steps( # 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 + step_config, context, scope, registry, step_impl, invocation_path ) else: result = step_impl.execute(step_config, context) @@ -1421,6 +1422,7 @@ def _execute_steps( scope.record_and_save( context, step_id, step_data, complete_child=result.status == StepStatus.COMPLETED, + child_scope_id=":".join([*invocation_path, step_id]), ) else: self._record_result(context, scope, step_id, step_data) @@ -1508,9 +1510,13 @@ def _execute_steps( # A step-path stack for exact nested resume is a future # enhancement. if result.next_steps: + nested_path = (*invocation_path, str(step_id)) + if step_type in ("while", "do-while"): + nested_path = (*invocation_path, f"{step_id}:0") self._execute_steps( result.next_steps, context, scope, registry, step_offset=-1, + invocation_path=nested_path, ) if scope.status in ( RunStatus.PAUSED, @@ -1553,6 +1559,10 @@ def _execute_steps( self._execute_steps( [ns_copy], context, scope, registry, step_offset=-1, + invocation_path=( + *invocation_path, + f"{step_id}:{_loop_iter + 1}", + ), ) if scope.status in ( RunStatus.PAUSED, @@ -1577,7 +1587,7 @@ def _execute_steps( if template and items: fan_out_results = self._run_fan_out( items, template, step_id, context, scope, registry, - result.output.get("max_concurrency", 1), + result.output.get("max_concurrency", 1), invocation_path, ) context.item = None # Preserve original output and add collected results @@ -1642,17 +1652,45 @@ def _run_workflow_call( scope: ExecutionScope, registry: dict[str, Any], step_impl: Any, + invocation_path: tuple[str, ...], ) -> StepResult: """Execute (or resume) a ``type: workflow`` call in a nested scope.""" - effective_id = step_config.get("id", "workflow") + step_id = step_config.get("id", "workflow") + effective_id = ":".join([*invocation_path, step_id]) 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.definition is None: # pragma: no cover - defensive + return StepResult( + status=StepStatus.FAILED, + output={ + "workflow": existing.workflow_id, + "status": RunStatus.FAILED.value, + }, + error=( + f"Workflow step {effective_id!r}: persisted scope has " + "no definition snapshot." + ), + ) + return self._aggregate_workflow_result( + existing, existing.definition, effective_id + ) + + if existing is not None and existing.status == RunStatus.ABORTED: + if existing.definition is None: # pragma: no cover - defensive + return StepResult( + status=StepStatus.FAILED, + output={ + "workflow": existing.workflow_id, + "status": RunStatus.FAILED.value, + }, + error=( + f"Workflow step {effective_id!r}: persisted scope has " + "no definition snapshot." + ), + ) + return self._aggregate_workflow_result( + existing, existing.definition, effective_id ) if existing is not None: @@ -1702,13 +1740,29 @@ def _run_workflow_call( 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, - ) + try: + self._execute_steps( + definition.steps[start:], + child_context, + child_scope, + registry, + step_offset=start, + ) + except Exception as exc: # noqa: BLE001 - isolate child runtime failures + error = ( + f"Workflow step {effective_id!r}: workflow " + f"{definition.id!r} failed: {exc}" + ) + child_scope.pending_terminal_status = RunStatus.FAILED + child_scope.pending_terminal_error = error + return StepResult( + status=StepStatus.FAILED, + output={ + "workflow": definition.id, + "status": RunStatus.FAILED.value, + }, + error=error, + ) return self._aggregate_workflow_result( child_scope, definition, effective_id ) @@ -1788,9 +1842,25 @@ def _run_workflow_call( scope.persist() child_context = child_scope.build_context(is_resume=False) - self._execute_steps( - definition.steps, child_context, child_scope, registry, step_offset=0 - ) + try: + self._execute_steps( + definition.steps, child_context, child_scope, registry, step_offset=0 + ) + except Exception as exc: # noqa: BLE001 - isolate child runtime failures + error = ( + f"Workflow step {effective_id!r}: workflow " + f"{definition.id!r} failed: {exc}" + ) + child_scope.pending_terminal_status = RunStatus.FAILED + child_scope.pending_terminal_error = error + return StepResult( + status=StepStatus.FAILED, + output={ + "workflow": definition.id, + "status": RunStatus.FAILED.value, + }, + error=error, + ) return self._aggregate_workflow_result( child_scope, definition, effective_id ) @@ -1864,6 +1934,7 @@ def _run_fan_out( scope: ExecutionScope, registry: dict[str, Any], max_concurrency: Any, + invocation_path: tuple[str, ...] = (), ) -> list[Any]: """Run a fan-out template once per item; return per-item outputs in item order. @@ -1913,7 +1984,12 @@ 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, scope, registry, step_offset=-1, + [item_step], + item_ctx, + scope, + registry, + step_offset=-1, + invocation_path=invocation_path, ) # Read back through the context that was actually executed against, # not the outer closure — clearer and robust if StepContext copying diff --git a/tests/workflows/test_workflow_composition.py b/tests/workflows/test_workflow_composition.py index a5ccece202..2ab787280d 100644 --- a/tests/workflows/test_workflow_composition.py +++ b/tests/workflows/test_workflow_composition.py @@ -641,6 +641,110 @@ def test_aborted_shape(self, project_dir): class TestContinueOnError: + def test_child_expression_failure_uses_call_boundary_on_execute(self, project_dir): + """Expression errors inside a child obey the caller's recovery policy.""" + _install( + project_dir, + "child", + _workflow( + "child", + [ + _shell("source", "printf not-json"), + { + "id": "parse", + "type": "if", + "condition": "{{ steps.source.output.stdout | from_json }}", + "then": [_shell("after-parse", "echo unreachable")], + }, + ], + ), + ) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "c", + "type": "workflow", + "workflow": "child", + "continue_on_error": True, + }, + _shell("after", "echo recovered"), + ], + ), + ) + + engine = WorkflowEngine(project_dir) + state = engine.execute(_definition(project_dir, "parent"), {}) + assert state.status == RunStatus.COMPLETED + assert state.step_results["c"]["status"] == "failed" + assert state.workflow_scopes["c"]["status"] == "failed" + assert "from_json: invalid JSON" in state.step_results["c"]["error"] + assert state.step_results["after"]["output"]["stdout"].strip() == "recovered" + + def test_child_expression_failure_uses_call_boundary_on_resume(self, project_dir): + _install( + project_dir, + "child", + _workflow( + "child", + [ + { + "id": "gate", + "type": "gate", + "message": "continue?", + "options": ["approve", "reject"], + "verdict_input": "verdict", + }, + _shell("source", "printf not-json"), + { + "id": "parse", + "type": "if", + "condition": "{{ steps.source.output.stdout | from_json }}", + "then": [_shell("after-parse", "echo unreachable")], + }, + ], + inputs={ + "verdict": { + "type": "string", + "default": "", + "enum": ["", "approve", "reject"], + } + }, + ), + ) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "c", + "type": "workflow", + "workflow": "child", + "input": {"verdict": "{{ inputs.verdict }}"}, + "continue_on_error": True, + }, + _shell("after", "echo recovered"), + ], + inputs={"verdict": {"type": "string", "default": ""}}, + ), + ) + + 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.step_results["c"]["status"] == "failed" + assert resumed.workflow_scopes["c"]["status"] == "failed" + assert "from_json: invalid JSON" in resumed.step_results["c"]["error"] + assert resumed.step_results["after"]["output"]["stdout"].strip() == "recovered" + @pytest.mark.parametrize("continue_on_error", [False, True]) def test_output_evaluation_failure_uses_call_boundary( self, project_dir, continue_on_error @@ -1829,6 +1933,203 @@ def test_root_input_update_propagates_through_nested_calls(self, project_dir): class TestRepeatedCalls: + def test_nested_if_workflow_calls_have_distinct_loop_invocations(self, project_dir): + _install( + project_dir, + "child", + _workflow( + "child", + [_shell("capture", "echo {{ inputs.value }}")], + inputs={"value": {"type": "string", "required": True}}, + ), + ) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "loop", + "type": "while", + "condition": "{{ true }}", + "max_iterations": 3, + "steps": [ + { + "id": "branch", + "type": "if", + "condition": "{{ true }}", + "then": [ + { + "id": "call", + "type": "workflow", + "workflow": "child", + "input": {"value": "{{ inputs.value }}"}, + } + ], + } + ], + } + ], + inputs={"value": {"type": "string", "default": "loop"}}, + ), + ) + + state = _run(project_dir, "parent") + assert state.status == RunStatus.COMPLETED + children = [ + child + for child in state.workflow_scopes.values() + if child["workflow_id"] == "child" + ] + assert len(children) == 3 + assert all(child["inputs"] == {"value": "loop"} for child in children) + + def test_nested_if_workflow_calls_have_distinct_fan_out_invocations(self, project_dir): + _install( + project_dir, + "child", + _workflow( + "child", + [_shell("capture", "echo {{ inputs.value }}")], + inputs={"value": {"type": "string", "required": True}}, + ), + ) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "spread", + "type": "fan-out", + "items": ["a", "b", "c"], + "step": { + "id": "branch", + "type": "if", + "condition": "{{ true }}", + "then": [ + { + "id": "call", + "type": "workflow", + "workflow": "child", + "input": {"value": "{{ item }}"}, + } + ], + }, + } + ], + ), + ) + + state = _run(project_dir, "parent") + assert state.status == RunStatus.COMPLETED + children = [ + child + for child in state.workflow_scopes.values() + if child["workflow_id"] == "child" + ] + assert {child["inputs"]["value"] for child in children} == {"a", "b", "c"} + + def test_aborted_child_is_not_restarted_when_resuming_fan_out(self, project_dir): + _install( + project_dir, + "child", + _workflow( + "child", + [ + { + "id": "choose", + "type": "if", + "condition": "{{ inputs.kind == 'pause' }}", + "then": [ + { + "id": "pause-gate", + "type": "gate", + "message": "continue?", + "options": ["approve", "reject"], + "on_reject": "abort", + "verdict_input": "resume", + } + ], + "else": [ + { + "id": "abort-gate", + "type": "gate", + "message": "continue?", + "options": ["approve", "reject"], + "on_reject": "abort", + "verdict_input": "verdict", + } + ], + }, + _shell("after-gate", "echo should-not-run"), + ], + inputs={ + "kind": {"type": "string", "required": True}, + "verdict": { + "type": "string", + "default": "reject", + "enum": ["", "approve", "reject"], + }, + "resume": { + "type": "string", + "default": "", + "enum": ["", "approve", "reject"], + }, + }, + ), + ) + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + { + "id": "spread", + "type": "fan-out", + "items": ["pause", "abort"], + "max_concurrency": 2, + "step": { + "id": "call", + "type": "workflow", + "workflow": "child", + "input": { + "kind": "{{ item }}", + "verdict": "{{ inputs.verdict }}", + "resume": "{{ inputs.resume }}", + }, + }, + } + ], + inputs={ + "verdict": {"type": "string", "default": "reject"}, + "resume": {"type": "string", "default": ""}, + }, + ), + ) + + engine = WorkflowEngine(project_dir) + state = engine.execute(_definition(project_dir, "parent"), {}) + assert state.status == RunStatus.PAUSED + aborted = next( + child + for child in state.workflow_scopes.values() + if child["status"] == "aborted" + ) + assert "after-gate" not in aborted["step_results"] + + resumed = engine.resume(state.run_id, {"resume": "approve"}) + assert resumed.status == RunStatus.ABORTED + aborted = next( + child + for child in resumed.workflow_scopes.values() + if child["status"] == "aborted" + ) + assert "after-gate" not in aborted["step_results"] + def test_fan_out_scopes_are_distinct(self, project_dir): _install( project_dir, From da514766557e1a84a8129d01a1b2903ee847df18 Mon Sep 17 00:00:00 2001 From: Markus Date: Fri, 25 Sep 2026 21:45:40 +0200 Subject: [PATCH 18/21] docs(workflows): document workflow step for publishing Assisted-by: OpenCode (model: gpt-5.6-terra, autonomous) --- workflows/PUBLISHING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workflows/PUBLISHING.md b/workflows/PUBLISHING.md index 4e26fdfb25..5b7850cbe0 100644 --- a/workflows/PUBLISHING.md +++ b/workflows/PUBLISHING.md @@ -90,7 +90,7 @@ steps: - ✅ `version` follows semantic versioning (X.Y.Z) - ✅ `description` is concise - ✅ All step IDs are unique -- ✅ Step types are valid: `command`, `prompt`, `shell`, `init`, `slot`, `gate`, `if`, `switch`, `while`, `do-while`, `fan-out`, `fan-in` +- ✅ Step types are valid: `command`, `prompt`, `shell`, `init`, `slot`, `gate`, `if`, `switch`, `while`, `do-while`, `fan-out`, `fan-in`, `workflow` - ✅ Required fields present per step type (e.g., `condition` for `if`, `expression` for `switch`) - ✅ Input types are valid: `string`, `number`, `boolean` - ✅ Step IDs do not contain `:` (reserved for engine-generated nested IDs like `parentId:childId`) From 16e7e14d3a934a65f1baef48b83a201bae21b5e4 Mon Sep 17 00:00:00 2001 From: Markus Date: Sat, 26 Sep 2026 06:51:00 +0200 Subject: [PATCH 19/21] fix(workflows): bound IDs and match nested gates Assisted-by: GitHub Copilot (model: gpt-5.6-terra, autonomous) --- .../workflows/_command_run_ownership.py | 3 +- src/specify_cli/workflows/_commands.py | 21 +++-- src/specify_cli/workflows/command_run.py | 4 +- src/specify_cli/workflows/composition.py | 10 +-- src/specify_cli/workflows/engine.py | 23 ++++-- .../workflows/overlay/layer_sources.py | 5 +- .../workflows/overlay/operations.py | 7 ++ src/specify_cli/workflows/overlay/resolver.py | 6 +- src/specify_cli/workflows/overlay/schema.py | 7 ++ .../workflows/step/workflow/__init__.py | 4 +- .../specify_cli/workflows/test_command_add.py | 1 + tests/test_workflows.py | 24 ++++++ tests/workflows/test_overlay_layer_sources.py | 1 + tests/workflows/test_workflow_composition.py | 79 +++++++++++++++++++ 14 files changed, 165 insertions(+), 30 deletions(-) diff --git a/src/specify_cli/workflows/_command_run_ownership.py b/src/specify_cli/workflows/_command_run_ownership.py index 322a05f2b4..12ccd58a52 100644 --- a/src/specify_cli/workflows/_command_run_ownership.py +++ b/src/specify_cli/workflows/_command_run_ownership.py @@ -6,6 +6,7 @@ from pathlib import Path from . import _commands as cli +from .engine import is_valid_workflow_id def _same_existing_path(left: Path, right: Path) -> bool: @@ -125,7 +126,7 @@ def ownership_for(candidate: Path) -> tuple[Path, str] | None: if ( not isinstance(workflow_id, str) or workflow_id in cli._RESERVED_WORKFLOW_IDS - or not cli._WORKFLOW_ID_PATTERN.fullmatch(workflow_id) + or not is_valid_workflow_id(workflow_id) ): continue try: diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index 7fb888a65b..de1634e6e3 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -30,6 +30,7 @@ ) from .._project import _resolve_init_dir_override as _resolve_init_dir_override from ..shared_infra import verify_archive_sha256 +from .engine import is_valid_workflow_id workflow_app = typer.Typer( name="workflow", @@ -154,7 +155,6 @@ def _resolve_installed_workflow_ownership( return resolver(source_path, err) -_WORKFLOW_ID_PATTERN = re.compile(r"^[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$") _RESERVED_WORKFLOW_IDS: frozenset[str] = frozenset({"overlays", "runs", "steps"}) @@ -263,7 +263,7 @@ def _validate_workflow_id_or_exit(workflow_id: str) -> None: """Validate that ``workflow_id`` is a safe installed-workflow directory name.""" if ( workflow_id in _RESERVED_WORKFLOW_IDS - or not _WORKFLOW_ID_PATTERN.fullmatch(workflow_id) + or not is_valid_workflow_id(workflow_id) ): console.print( f"[red]Error:[/red] Invalid workflow ID: {_escape_markup(repr(workflow_id))}" @@ -1032,7 +1032,9 @@ def _gate_details(step_id: str, output: Any) -> dict[str, Any]: } -def _scope_gate(scopes: Any, path: list[str]) -> dict[str, Any] | None: +def _scope_gate( + scopes: Any, path: list[str], active_status: str +) -> dict[str, Any] | None: """Find the active gate inside a serialized scope tree. A pause/abort inside a composed workflow leaves the *root* resting on the @@ -1046,10 +1048,12 @@ def _scope_gate(scopes: Any, path: list[str]) -> dict[str, Any] | None: if not isinstance(record, dict): continue child_path = [*path, key] - if str(record.get("status")) not in ("paused", "aborted"): + if str(record.get("status")) != active_status: continue - # A deeper paused scope is more specific; check descendants first. - nested = _scope_gate(record.get("workflow_scopes"), child_path) + # A deeper active scope is more specific; check descendants first. + nested = _scope_gate( + record.get("workflow_scopes"), child_path, active_status + ) if nested is not None: return nested step_id = _scope_current_step_id(record) @@ -1079,13 +1083,14 @@ def _gate_outcome(state: Any) -> dict[str, Any] | None: # notably `completed`/`failed` — must be suppressed: current_step_id is # not cleared when a run whose last executed step was a gate moves on, so # without this guard it would surface stale detail (run/resume/status). - if getattr(state.status, "value", state.status) not in ("paused", "aborted"): + status = getattr(state.status, "value", state.status) + if status not in ("paused", "aborted"): return None step = (getattr(state, "step_results", None) or {}).get(state.current_step_id) if isinstance(step, dict) and _is_gate_step(step): return _gate_details(state.current_step_id, step.get("output")) # Not a top-level gate: a composed workflow may be paused on a nested gate. - return _scope_gate(getattr(state, "workflow_scopes", None), []) + return _scope_gate(getattr(state, "workflow_scopes", None), [], status) def _normalize_gate_options(options: Any) -> list[str] | None: diff --git a/src/specify_cli/workflows/command_run.py b/src/specify_cli/workflows/command_run.py index d966fd71e1..e9012bf4e6 100644 --- a/src/specify_cli/workflows/command_run.py +++ b/src/specify_cli/workflows/command_run.py @@ -20,7 +20,7 @@ def workflow_run( ): """Run a workflow from an installed ID or local YAML path.""" from . import load_custom_steps - from .engine import WorkflowEngine + from .engine import WorkflowEngine, is_valid_workflow_id source_path = cli.Path(source).expanduser() is_file_source = ( @@ -63,7 +63,7 @@ def workflow_run( # bypassing the disabled check below. if ( source in cli._RESERVED_WORKFLOW_IDS - or not cli._WORKFLOW_ID_PATTERN.fullmatch(source) + or not is_valid_workflow_id(source) ): err.print( f"[red]Error:[/red] Invalid workflow ID: {cli._escape_markup(repr(source))}" diff --git a/src/specify_cli/workflows/composition.py b/src/specify_cli/workflows/composition.py index a3c39f8f50..012eb459f4 100644 --- a/src/specify_cli/workflows/composition.py +++ b/src/specify_cli/workflows/composition.py @@ -56,11 +56,11 @@ _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 +def _is_valid_workflow_id(value: Any) -> bool: + """Return whether *value* meets the engine's workflow-ID contract.""" + from .engine import is_valid_workflow_id - return _ID_PATTERN + return is_valid_workflow_id(value) def _reserved_workflow_ids() -> frozenset[str]: @@ -169,7 +169,7 @@ def validate_workflow_call_config(config: dict[str, Any]) -> list[str]: ) elif "{{" not in target: # A literal target must be a valid, non-reserved workflow ID. - if not _id_pattern().fullmatch(target): + if not _is_valid_workflow_id(target): errors.append( f"Workflow step {step_id!r}: 'workflow' literal {target!r} " "must be lowercase alphanumeric with hyphens." diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index ff6f01bb73..31c1168867 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -134,9 +134,20 @@ def from_string(cls, content: str) -> WorkflowDefinition: # -- Workflow Validation -------------------------------------------------- -# ID format: lowercase alphanumeric with hyphens +# ID format: lowercase alphanumeric with hyphens. Keep this below the common +# filesystem component limit once composed-workflow snapshot suffixes are added. +MAX_WORKFLOW_ID_LENGTH = 200 _ID_PATTERN = re.compile(r"^[a-z0-9][a-z0-9-]*[a-z0-9]$|^[a-z0-9]$") + +def is_valid_workflow_id(value: Any) -> bool: + """Return whether *value* is a workflow ID safe for all storage paths.""" + return ( + isinstance(value, str) + and len(value) <= MAX_WORKFLOW_ID_LENGTH + and _ID_PATTERN.fullmatch(value) is not None + ) + # Keys accepted under a workflow's ``requires`` block: the advisory # pre-conditions documented for workflows (``speckit_version`` and # ``integrations``). This is the *workflow* schema only — the bundle manifest's @@ -221,10 +232,10 @@ def validate_workflow(definition: WorkflowDefinition) -> list[str]: f"'workflow.id' must be a string, got " f"{type(definition.id).__name__} ({definition.id!r})." ) - elif not _ID_PATTERN.fullmatch(definition.id): + elif not is_valid_workflow_id(definition.id): errors.append( f"Workflow ID {definition.id!r} must be lowercase alphanumeric " - f"with hyphens." + f"with hyphens and at most {MAX_WORKFLOW_ID_LENGTH} characters." ) if definition.name is None or definition.name == "": @@ -652,7 +663,7 @@ def _validate_installed_origin( "Invalid run state: 'installed_workflow_id' must be a " f"string or null, got {type(installed_workflow_id).__name__}" ) - if not _ID_PATTERN.fullmatch(installed_workflow_id): + if not is_valid_workflow_id(installed_workflow_id): raise ValueError( "Invalid run state: 'installed_workflow_id' must be a " "lowercase alphanumeric workflow ID with hyphens" @@ -864,9 +875,7 @@ def load(cls, run_id: str, project_root: Path) -> RunState: ) workflow_id = state_data["workflow_id"] - if not isinstance(workflow_id, str) or not _ID_PATTERN.fullmatch( - workflow_id - ): + if not is_valid_workflow_id(workflow_id): raise ValueError( "Invalid run state: 'workflow_id' must be a lowercase " "alphanumeric workflow ID with hyphens" diff --git a/src/specify_cli/workflows/overlay/layer_sources.py b/src/specify_cli/workflows/overlay/layer_sources.py index a62cef9340..510f0873d7 100644 --- a/src/specify_cli/workflows/overlay/layer_sources.py +++ b/src/specify_cli/workflows/overlay/layer_sources.py @@ -7,7 +7,8 @@ import yaml -from .schema import Overlay, _RESERVED_WORKFLOW_IDS, _SAFE_ID_PATTERN, validate_overlay_yaml +from ..engine import is_valid_workflow_id +from .schema import Overlay, _RESERVED_WORKFLOW_IDS, validate_overlay_yaml @dataclass @@ -38,7 +39,7 @@ def _validate_workflow_id(workflow_id: str, context_path: Path) -> None: """ if ( not isinstance(workflow_id, str) - or not _SAFE_ID_PATTERN.fullmatch(workflow_id) + or not is_valid_workflow_id(workflow_id) or workflow_id in _RESERVED_WORKFLOW_IDS ): raise OverlayLoadError( diff --git a/src/specify_cli/workflows/overlay/operations.py b/src/specify_cli/workflows/overlay/operations.py index ab390774c3..d948484137 100644 --- a/src/specify_cli/workflows/overlay/operations.py +++ b/src/specify_cli/workflows/overlay/operations.py @@ -13,6 +13,7 @@ from ..._console import console, err_console from ...extensions import normalize_priority from .. import _commands as cli +from ..engine import is_valid_workflow_id from . import WorkflowResolver from .schema import _RESERVED_WORKFLOW_IDS, _SAFE_ID_PATTERN, validate_overlay_yaml @@ -33,6 +34,12 @@ def _validate_overlay_id_or_exit(id_value: str, label: str) -> None: def _validate_workflow_id_or_exit(workflow_id: str) -> None: """Validate a workflow id, treating the overlay root as reserved.""" _validate_overlay_id_or_exit(workflow_id, "workflow ID") + if not is_valid_workflow_id(workflow_id): + err_console.print( + f"[red]Error:[/red] Invalid workflow ID {workflow_id!r}: " + "maximum length is 200 characters." + ) + raise typer.Exit(1) if workflow_id in _RESERVED_WORKFLOW_IDS: err_console.print( f"[red]Error:[/red] Invalid workflow ID {workflow_id!r}: " diff --git a/src/specify_cli/workflows/overlay/resolver.py b/src/specify_cli/workflows/overlay/resolver.py index bd50de6da3..e97fa23234 100644 --- a/src/specify_cli/workflows/overlay/resolver.py +++ b/src/specify_cli/workflows/overlay/resolver.py @@ -4,7 +4,7 @@ from pathlib import Path -from ..engine import WorkflowDefinition +from ..engine import WorkflowDefinition, is_valid_workflow_id from .composer import StepListComposer from .layer_sources import ( BaseWorkflowSource, @@ -12,14 +12,14 @@ ProjectOverlaySource, ) from .merge import ComposedStep -from .schema import _RESERVED_WORKFLOW_IDS, _SAFE_ID_PATTERN +from .schema import _RESERVED_WORKFLOW_IDS def _validate_workflow_id(workflow_id: str) -> None: """Reject workflow IDs that are unsafe as installed-storage path segments.""" if ( not isinstance(workflow_id, str) - or not _SAFE_ID_PATTERN.fullmatch(workflow_id) + or not is_valid_workflow_id(workflow_id) or workflow_id in _RESERVED_WORKFLOW_IDS ): raise ValueError(f"Invalid workflow ID: {workflow_id!r}") diff --git a/src/specify_cli/workflows/overlay/schema.py b/src/specify_cli/workflows/overlay/schema.py index 969bbd94a9..a68cfa9fe1 100644 --- a/src/specify_cli/workflows/overlay/schema.py +++ b/src/specify_cli/workflows/overlay/schema.py @@ -7,6 +7,7 @@ from typing import Any, Literal from ...extensions import normalize_priority +from ..engine import MAX_WORKFLOW_ID_LENGTH # Safe single-segment identifiers: no path separators, no traversal, no dots. _SAFE_ID_PATTERN = re.compile(r"^[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$") @@ -146,6 +147,12 @@ def validate_overlay_yaml(data: dict[str, Any]) -> tuple[Overlay | None, list[st ): errors.append(err) extends = "" + elif len(extends) > MAX_WORKFLOW_ID_LENGTH: + errors.append( + f"Overlay 'extends' {extends!r} exceeds the maximum workflow ID " + f"length of {MAX_WORKFLOW_ID_LENGTH} characters." + ) + extends = "" priority = normalize_priority(data.get("priority", 10)) diff --git a/src/specify_cli/workflows/step/workflow/__init__.py b/src/specify_cli/workflows/step/workflow/__init__.py index d410553209..dc0618c5d6 100644 --- a/src/specify_cli/workflows/step/workflow/__init__.py +++ b/src/specify_cli/workflows/step/workflow/__init__.py @@ -42,7 +42,7 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: # the ``{{ ... }}`` expression. target: Any = config.get("workflow") try: - from specify_cli.workflows.engine import _ID_PATTERN + from specify_cli.workflows.engine import is_valid_workflow_id from specify_cli.workflows.overlay.schema import ( _RESERVED_WORKFLOW_IDS, ) @@ -74,7 +74,7 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: ), ) if ( - not _ID_PATTERN.fullmatch(target) + not is_valid_workflow_id(target) or target in _RESERVED_WORKFLOW_IDS ): return StepResult( diff --git a/tests/specify_cli/workflows/test_command_add.py b/tests/specify_cli/workflows/test_command_add.py index 263fe79add..4b4cfb5261 100644 --- a/tests/specify_cli/workflows/test_command_add.py +++ b/tests/specify_cli/workflows/test_command_add.py @@ -772,6 +772,7 @@ def test_add_rejects_reserved_overlay_storage_id(self, temp_dir, monkeypatch): "bad id", " bad-id", "bad-id ", + "a" * 201, ], ) def test_safe_workflow_id_dir_rejects_reserved_or_non_segment_ids( diff --git a/tests/test_workflows.py b/tests/test_workflows.py index bfc1eda3d8..c5f2a8ebeb 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -4842,6 +4842,30 @@ def test_invalid_id_format(self): errors = validate_workflow(definition) assert any("lowercase alphanumeric" in e for e in errors) + @pytest.mark.parametrize( + ("workflow_id", "valid"), + [("a" * 200, True), ("a" * 201, False)], + ) + def test_workflow_id_length_limit(self, workflow_id, valid): + from specify_cli.workflows.engine import WorkflowDefinition, validate_workflow + + definition = WorkflowDefinition( + { + "schema_version": "1.0", + "workflow": { + "id": workflow_id, + "name": "Test", + "version": "1.0.0", + }, + "steps": [{"id": "step-one", "command": "speckit.specify"}], + } + ) + + errors = validate_workflow(definition) + assert (errors == []) is valid + if not valid: + assert any("at most 200 characters" in error for error in errors) + def test_workflow_id_with_trailing_newline_is_invalid(self): from specify_cli.workflows.engine import WorkflowDefinition, validate_workflow diff --git a/tests/workflows/test_overlay_layer_sources.py b/tests/workflows/test_overlay_layer_sources.py index 172115df05..6c08ee0898 100644 --- a/tests/workflows/test_overlay_layer_sources.py +++ b/tests/workflows/test_overlay_layer_sources.py @@ -128,6 +128,7 @@ def test_unicode_error_raises_overlay_load_error(self, project_dir: Path) -> Non "/absolute", "UPPER", "has space", + "a" * 201, ] diff --git a/tests/workflows/test_workflow_composition.py b/tests/workflows/test_workflow_composition.py index 2ab787280d..33d19acec2 100644 --- a/tests/workflows/test_workflow_composition.py +++ b/tests/workflows/test_workflow_composition.py @@ -415,6 +415,32 @@ def test_runtime_target_from_echo_requires_exact_id(self, project_dir): assert result["output"]["workflow"] == "child\n" assert "not a valid workflow ID" in result["error"] + def test_runtime_target_rejects_id_over_200_characters(self, project_dir): + long_id = "a" * 201 + _install( + project_dir, + "parent", + _workflow( + "parent", + [ + _shell("pick", f"printf {long_id}"), + { + "id": "call", + "type": "workflow", + "workflow": "{{ steps.pick.output.stdout }}", + "continue_on_error": True, + }, + ], + ), + ) + + state = _run(project_dir, "parent") + + result = state.step_results["call"] + assert result["status"] == "failed" + assert result["output"]["workflow"] == long_id + assert "not a valid workflow ID" in result["error"] + def test_registry_key_must_match_resolved_definition_id(self, project_dir): _install(project_dir, "child", _workflow("other", [_shell("x", "echo hi")])) @@ -2410,12 +2436,65 @@ def test_inactive_scope_cannot_report_a_stale_nested_gate(self): "active": active_gate, }, [], + "paused", ) assert gate is not None assert gate["step_id"] == "active" assert gate["scope_path"] == ["active"] + @pytest.mark.parametrize( + ("root_status", "expected_scope"), + [("paused", "paused"), ("aborted", "aborted")], + ) + def test_mixed_sibling_statuses_report_gate_matching_root_status( + self, root_status, expected_scope + ): + from types import SimpleNamespace + + from specify_cli.workflows._commands import _gate_outcome + + def gate(status, step_id): + return { + "status": status, + "current_step_id": step_id, + "step_results": { + step_id: { + "type": "gate", + "output": {"message": step_id, "on_reject": "abort"}, + } + }, + } + + # The non-matching sibling is deliberately first, reproducing the + # timing-dependent insertion order from concurrent fan-out execution. + state = SimpleNamespace( + status=SimpleNamespace(value=root_status), + current_step_id="fan-out", + step_results={}, + workflow_scopes={ + "aborted": gate("aborted", "abort-gate"), + "paused": gate("paused", "pause-gate"), + }, + ) + + outcome = _gate_outcome(state) + assert outcome is not None + assert outcome["scope_path"] == [expected_scope] + assert outcome["step_id"] == { + "paused": "pause-gate", + "aborted": "abort-gate", + }[expected_scope] + + def test_maximum_workflow_id_keeps_snapshot_filename_within_limits(self): + from specify_cli.workflows.composition import ExecutionScope, _snapshot_ref_for + + reference = _snapshot_ref_for( + ExecutionScope(scope_id="call", workflow_id="a" * 200) + ) + + assert len(reference.encode("ascii")) <= 255 + def test_gate_inside_nested_control_flow_reports_gate(self, project_dir): """A gate inside an ``if`` body must be reported, not its enclosing step. From 7c5f9f5ce74908bd7c90638d3fec9e9beaa134a3 Mon Sep 17 00:00:00 2001 From: Markus Date: Sat, 26 Sep 2026 08:22:22 +0200 Subject: [PATCH 20/21] fix(workflows): hash composition snapshot names Keep the established workflow ID contract and bound composition snapshot filenames with a full digest instead. Existing readable snapshot references remain loadable. Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) --- src/specify_cli/workflows/composition.py | 19 +++++++----- src/specify_cli/workflows/engine.py | 14 +++------ .../workflows/overlay/operations.py | 7 ----- src/specify_cli/workflows/overlay/schema.py | 9 ------ .../specify_cli/workflows/test_command_add.py | 1 - tests/test_workflows.py | 29 ++++++++++++++----- tests/workflows/test_overlay_layer_sources.py | 1 - tests/workflows/test_workflow_composition.py | 28 +++++++++++------- 8 files changed, 54 insertions(+), 54 deletions(-) diff --git a/src/specify_cli/workflows/composition.py b/src/specify_cli/workflows/composition.py index 012eb459f4..7f73fb5ea5 100644 --- a/src/specify_cli/workflows/composition.py +++ b/src/specify_cli/workflows/composition.py @@ -394,7 +394,8 @@ def check_composition_path(active_path: list[str], target: str) -> None: #: Directory under a run directory holding immutable definition snapshots. SNAPSHOT_DIRNAME = "snapshots" -#: Snapshot file name: ``-.yml``. +#: Snapshot file name: ``.yml``. Older readable +#: ``-.yml`` references remain valid when loading state. _SNAPSHOT_REF_PATTERN = re.compile(r"^[a-z0-9][a-z0-9._-]*\.yml$") @@ -411,14 +412,16 @@ def _invocation_path(scope: ExecutionScope) -> list[str]: def _snapshot_ref_for(scope: ExecutionScope) -> str: """Deterministic, filesystem-safe snapshot name for *scope*. - Derived from the invocation path and the target workflow id rather than the - authored step id, so a step id containing ``/`` or ``:`` can never escape - the snapshots directory. + The complete digest keeps the component bounded independently of authored + invocation and workflow ID lengths. The target ID is included so the name + also changes if the same invocation path is ever rebound before persisting. """ - digest = hashlib.sha256( - "\x00".join(_invocation_path(scope)).encode("utf-8") - ).hexdigest()[:12] - return f"{scope.workflow_id}-{digest}.yml" + digest = hashlib.sha256() + for value in [*_invocation_path(scope), scope.workflow_id]: + encoded = value.encode("utf-8") + digest.update(len(encoded).to_bytes(8, "big")) + digest.update(encoded) + return f"{digest.hexdigest()}.yml" def _definition_snapshot_path(run_dir: Path, ref: str) -> Path: diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index 31c1168867..c2f0c1a656 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -134,19 +134,13 @@ def from_string(cls, content: str) -> WorkflowDefinition: # -- Workflow Validation -------------------------------------------------- -# ID format: lowercase alphanumeric with hyphens. Keep this below the common -# filesystem component limit once composed-workflow snapshot suffixes are added. -MAX_WORKFLOW_ID_LENGTH = 200 +# ID format: lowercase alphanumeric with hyphens. _ID_PATTERN = re.compile(r"^[a-z0-9][a-z0-9-]*[a-z0-9]$|^[a-z0-9]$") def is_valid_workflow_id(value: Any) -> bool: - """Return whether *value* is a workflow ID safe for all storage paths.""" - return ( - isinstance(value, str) - and len(value) <= MAX_WORKFLOW_ID_LENGTH - and _ID_PATTERN.fullmatch(value) is not None - ) + """Return whether *value* follows the workflow-ID format.""" + return isinstance(value, str) and _ID_PATTERN.fullmatch(value) is not None # Keys accepted under a workflow's ``requires`` block: the advisory # pre-conditions documented for workflows (``speckit_version`` and @@ -235,7 +229,7 @@ def validate_workflow(definition: WorkflowDefinition) -> list[str]: elif not is_valid_workflow_id(definition.id): errors.append( f"Workflow ID {definition.id!r} must be lowercase alphanumeric " - f"with hyphens and at most {MAX_WORKFLOW_ID_LENGTH} characters." + "with hyphens." ) if definition.name is None or definition.name == "": diff --git a/src/specify_cli/workflows/overlay/operations.py b/src/specify_cli/workflows/overlay/operations.py index d948484137..ab390774c3 100644 --- a/src/specify_cli/workflows/overlay/operations.py +++ b/src/specify_cli/workflows/overlay/operations.py @@ -13,7 +13,6 @@ from ..._console import console, err_console from ...extensions import normalize_priority from .. import _commands as cli -from ..engine import is_valid_workflow_id from . import WorkflowResolver from .schema import _RESERVED_WORKFLOW_IDS, _SAFE_ID_PATTERN, validate_overlay_yaml @@ -34,12 +33,6 @@ def _validate_overlay_id_or_exit(id_value: str, label: str) -> None: def _validate_workflow_id_or_exit(workflow_id: str) -> None: """Validate a workflow id, treating the overlay root as reserved.""" _validate_overlay_id_or_exit(workflow_id, "workflow ID") - if not is_valid_workflow_id(workflow_id): - err_console.print( - f"[red]Error:[/red] Invalid workflow ID {workflow_id!r}: " - "maximum length is 200 characters." - ) - raise typer.Exit(1) if workflow_id in _RESERVED_WORKFLOW_IDS: err_console.print( f"[red]Error:[/red] Invalid workflow ID {workflow_id!r}: " diff --git a/src/specify_cli/workflows/overlay/schema.py b/src/specify_cli/workflows/overlay/schema.py index a68cfa9fe1..d8bc2de715 100644 --- a/src/specify_cli/workflows/overlay/schema.py +++ b/src/specify_cli/workflows/overlay/schema.py @@ -7,8 +7,6 @@ from typing import Any, Literal from ...extensions import normalize_priority -from ..engine import MAX_WORKFLOW_ID_LENGTH - # Safe single-segment identifiers: no path separators, no traversal, no dots. _SAFE_ID_PATTERN = re.compile(r"^[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$") _RESERVED_OVERLAY_WORKFLOW_IDS: frozenset[str] = frozenset({"overlays"}) @@ -147,13 +145,6 @@ def validate_overlay_yaml(data: dict[str, Any]) -> tuple[Overlay | None, list[st ): errors.append(err) extends = "" - elif len(extends) > MAX_WORKFLOW_ID_LENGTH: - errors.append( - f"Overlay 'extends' {extends!r} exceeds the maximum workflow ID " - f"length of {MAX_WORKFLOW_ID_LENGTH} characters." - ) - extends = "" - priority = normalize_priority(data.get("priority", 10)) edits_raw = data.get("edits") diff --git a/tests/specify_cli/workflows/test_command_add.py b/tests/specify_cli/workflows/test_command_add.py index 4b4cfb5261..263fe79add 100644 --- a/tests/specify_cli/workflows/test_command_add.py +++ b/tests/specify_cli/workflows/test_command_add.py @@ -772,7 +772,6 @@ def test_add_rejects_reserved_overlay_storage_id(self, temp_dir, monkeypatch): "bad id", " bad-id", "bad-id ", - "a" * 201, ], ) def test_safe_workflow_id_dir_rejects_reserved_or_non_segment_ids( diff --git a/tests/test_workflows.py b/tests/test_workflows.py index c5f2a8ebeb..2ef12a0656 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -4842,13 +4842,10 @@ def test_invalid_id_format(self): errors = validate_workflow(definition) assert any("lowercase alphanumeric" in e for e in errors) - @pytest.mark.parametrize( - ("workflow_id", "valid"), - [("a" * 200, True), ("a" * 201, False)], - ) - def test_workflow_id_length_limit(self, workflow_id, valid): + def test_long_workflow_id_remains_valid(self): from specify_cli.workflows.engine import WorkflowDefinition, validate_workflow + workflow_id = "a" * 255 definition = WorkflowDefinition( { "schema_version": "1.0", @@ -4862,9 +4859,7 @@ def test_workflow_id_length_limit(self, workflow_id, valid): ) errors = validate_workflow(definition) - assert (errors == []) is valid - if not valid: - assert any("at most 200 characters" in error for error in errors) + assert errors == [] def test_workflow_id_with_trailing_newline_is_invalid(self): from specify_cli.workflows.engine import WorkflowDefinition, validate_workflow @@ -7731,6 +7726,24 @@ def test_save_and_load(self, project_dir): assert loaded.inputs == {"name": "login"} assert loaded.step_results == state.step_results + def test_load_preserves_preexisting_long_workflow_ids(self, project_dir): + from specify_cli.workflows.engine import RunState + + workflow_id = "a" * 255 + state = RunState( + run_id="long-workflow", + workflow_id=workflow_id, + project_root=project_dir, + installed_workflow_id=workflow_id, + installed_registry_root=str(project_dir.resolve()), + ) + state.save() + + loaded = RunState.load("long-workflow", project_dir) + + assert loaded.workflow_id == workflow_id + assert loaded.installed_workflow_id == workflow_id + @pytest.mark.parametrize("invalid_step_results", [None, [], "invalid", 1, True]) def test_load_rejects_non_object_step_results( self, project_dir, invalid_step_results diff --git a/tests/workflows/test_overlay_layer_sources.py b/tests/workflows/test_overlay_layer_sources.py index 6c08ee0898..172115df05 100644 --- a/tests/workflows/test_overlay_layer_sources.py +++ b/tests/workflows/test_overlay_layer_sources.py @@ -128,7 +128,6 @@ def test_unicode_error_raises_overlay_load_error(self, project_dir: Path) -> Non "/absolute", "UPPER", "has space", - "a" * 201, ] diff --git a/tests/workflows/test_workflow_composition.py b/tests/workflows/test_workflow_composition.py index 33d19acec2..a96df681a5 100644 --- a/tests/workflows/test_workflow_composition.py +++ b/tests/workflows/test_workflow_composition.py @@ -415,20 +415,25 @@ def test_runtime_target_from_echo_requires_exact_id(self, project_dir): assert result["output"]["workflow"] == "child\n" assert "not a valid workflow ID" in result["error"] - def test_runtime_target_rejects_id_over_200_characters(self, project_dir): - long_id = "a" * 201 + def test_long_valid_target_uses_bounded_snapshot_name(self, project_dir): + # This ID fits an installed-workflow directory component, but adding the + # old readable ``-.yml`` suffix exceeded the common 255-byte cap. + long_id = "a" * 239 + _install( + project_dir, + long_id, + _workflow(long_id, [_shell("x", "echo hi")]), + ) _install( project_dir, "parent", _workflow( "parent", [ - _shell("pick", f"printf {long_id}"), { "id": "call", "type": "workflow", - "workflow": "{{ steps.pick.output.stdout }}", - "continue_on_error": True, + "workflow": long_id, }, ], ), @@ -437,9 +442,12 @@ def test_runtime_target_rejects_id_over_200_characters(self, project_dir): state = _run(project_dir, "parent") result = state.step_results["call"] - assert result["status"] == "failed" + assert state.status == RunStatus.COMPLETED + assert result["status"] == "completed" assert result["output"]["workflow"] == long_id - assert "not a valid workflow ID" in result["error"] + reference = state.workflow_scopes["call"]["definition_snapshot"] + assert len(reference.encode("ascii")) == 68 + assert (state.runs_dir / "snapshots" / reference).is_file() def test_registry_key_must_match_resolved_definition_id(self, project_dir): _install(project_dir, "child", _workflow("other", [_shell("x", "echo hi")])) @@ -2486,14 +2494,14 @@ def gate(status, step_id): "aborted": "abort-gate", }[expected_scope] - def test_maximum_workflow_id_keeps_snapshot_filename_within_limits(self): + def test_snapshot_filename_is_bounded_independently_of_workflow_id(self): from specify_cli.workflows.composition import ExecutionScope, _snapshot_ref_for reference = _snapshot_ref_for( - ExecutionScope(scope_id="call", workflow_id="a" * 200) + ExecutionScope(scope_id="call", workflow_id="a" * 255) ) - assert len(reference.encode("ascii")) <= 255 + assert len(reference.encode("ascii")) == 68 def test_gate_inside_nested_control_flow_reports_gate(self, project_dir): """A gate inside an ``if`` body must be reported, not its enclosing step. From 7ece7a168514fc8a6b9e17bba5e68c00f85c96d4 Mon Sep 17 00:00:00 2001 From: Markus Date: Sat, 26 Sep 2026 15:07:37 +0200 Subject: [PATCH 21/21] fix(workflows): persist exact sequential resume cursors Freeze selected expansions, preserve nested invocation identity, and commit cursor progress with step outcomes. Retain output retry boundaries and failure-resolution logging. Assisted-by: OpenCode (model: gpt-6-astra, autonomous) --- src/specify_cli/workflows/_continuation.py | 263 +++++++++++++ src/specify_cli/workflows/composition.py | 154 +++++++- src/specify_cli/workflows/engine.py | 372 +++++++++++++++++-- tests/test_workflows.py | 364 ++++++++++++++++++ tests/workflows/test_workflow_composition.py | 213 ++++++++++- 5 files changed, 1334 insertions(+), 32 deletions(-) create mode 100644 src/specify_cli/workflows/_continuation.py diff --git a/src/specify_cli/workflows/_continuation.py b/src/specify_cli/workflows/_continuation.py new file mode 100644 index 0000000000..de123c63e4 --- /dev/null +++ b/src/specify_cli/workflows/_continuation.py @@ -0,0 +1,263 @@ +"""Private persisted cursors for exact sequential workflow resume. + +The cursor is deliberately independent of the step registry. Commands which +only inspect a run must be able to validate its progress even when a custom +step plugin is not installed in the current process. +""" + +from __future__ import annotations + +import hashlib +import os +from dataclasses import asdict, dataclass +from pathlib import Path +from typing import Any + +import yaml + +CONTINUATION_VERSION = 1 +_PHASES = frozenset({"running", "paused", "failed", "interrupted", "children"}) +_SOURCE_KINDS = frozenset({"workflow", "expansion"}) + + +@dataclass +class SequenceSource: + kind: str + snapshot: str | None = None + + +@dataclass +class ActivationCursor: + activation_path: list[dict[str, Any]] + step_id: str + step_type: str + attempt: int + phase: str + child_sequence: SequenceCursor | None = None + + +@dataclass +class SequenceCursor: + source: SequenceSource + next_index: int = 0 + active: ActivationCursor | None = None + + +@dataclass +class Continuation: + version: int + sequence: SequenceCursor + + def serialize(self) -> dict[str, Any]: + return asdict(self) + + +def new_continuation() -> dict[str, Any]: + """Return the initial root cursor in its JSON-persisted form.""" + return Continuation( + version=CONTINUATION_VERSION, + sequence=SequenceCursor(source=SequenceSource(kind="workflow")), + ).serialize() + + +def _continuations_dir(run_dir: Path) -> Path: + return run_dir / "continuations" + + +def _safe_snapshot_path(run_dir: Path, ref: str) -> Path: + if not isinstance(ref, str) or not ref.startswith("continuations/"): + raise ValueError("Invalid continuation: unsafe snapshot reference") + name = ref.removeprefix("continuations/") + if "/" in name or "\\" in name or not name.endswith(".yml"): + raise ValueError("Invalid continuation: unsafe snapshot reference") + digest = name[:-4] + if len(digest) != 64 or any(char not in "0123456789abcdef" for char in digest): + raise ValueError("Invalid continuation: unsafe snapshot reference") + directory = _continuations_dir(run_dir) + path = directory / name + try: + path.relative_to(directory) + except ValueError as exc: # pragma: no cover - defensive containment check + raise ValueError("Invalid continuation: unsafe snapshot reference") from exc + if path.is_symlink(): + raise ValueError("Invalid continuation: snapshot must not be a symlink") + return path + + +def _normalize_steps(steps: Any) -> list[dict[str, Any]]: + if not isinstance(steps, list): + raise ValueError("Continuation expansion must be a list of step mappings") + normalized: list[dict[str, Any]] = [] + for index, step in enumerate(steps): + if not isinstance(step, dict): + raise ValueError( + "Continuation expansion entries must be step mappings" + ) + copy = dict(step) + step_id = copy.get("id") + if step_id is None: + copy["id"] = f"step-{index}" + elif not isinstance(step_id, str) or not step_id: + raise ValueError("Continuation expansion step IDs must be non-empty strings") + normalized.append(copy) + return normalized + + +def write_expansion_snapshot(run_dir: Path, steps: Any) -> tuple[str, list[dict[str, Any]]]: + """Write an immutable normalized child sequence before state references it.""" + normalized = _normalize_steps(steps) + content = yaml.safe_dump(normalized, sort_keys=False, allow_unicode=False).encode("utf-8") + digest = hashlib.sha256(content).hexdigest() + directory = _continuations_dir(run_dir) + directory.mkdir(parents=True, exist_ok=True) + if directory.is_symlink(): + raise ValueError("Invalid continuation: continuations directory must not be a symlink") + ref = f"continuations/{digest}.yml" + path = _safe_snapshot_path(run_dir, ref) + if path.exists(): + if path.is_symlink() or path.read_bytes() != content: + raise ValueError("Invalid continuation: conflicting expansion snapshot") + return ref, normalized + fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600) + try: + with os.fdopen(fd, "wb") as handle: + handle.write(content) + except BaseException: + try: + path.unlink(missing_ok=True) + except OSError: + pass + raise + return ref, normalized + + +def read_expansion_snapshot(run_dir: Path, ref: str) -> list[dict[str, Any]]: + """Load and verify an immutable expansion snapshot.""" + path = _safe_snapshot_path(run_dir, ref) + try: + content = path.read_bytes() + except FileNotFoundError as exc: + raise ValueError("Invalid continuation: expansion snapshot is missing") from exc + digest = hashlib.sha256(content).hexdigest() + if path.name != f"{digest}.yml": + raise ValueError("Invalid continuation: expansion snapshot hash mismatch") + try: + parsed = yaml.safe_load(content) + except yaml.YAMLError as exc: + raise ValueError("Invalid continuation: malformed expansion snapshot") from exc + normalized = _normalize_steps(parsed) + if normalized != parsed: + raise ValueError("Invalid continuation: expansion snapshot is not normalized") + return normalized + + +def _validate_path(path: Any) -> None: + if not isinstance(path, list) or not path: + raise ValueError("Invalid continuation: activation_path must be a non-empty list") + for segment in path: + if not isinstance(segment, dict) or not isinstance(segment.get("kind"), str): + raise ValueError("Invalid continuation: invalid activation path segment") + + +def validate_continuation( + data: Any, + *, + run_dir: Path, + root_steps: list[dict[str, Any]] | None = None, +) -> None: + """Validate a persisted cursor and every snapshot it references.""" + if not isinstance(data, dict): + raise ValueError("Invalid continuation: expected a JSON object") + version = data.get("version") + if version != CONTINUATION_VERSION: + if isinstance(version, int) and version > CONTINUATION_VERSION: + raise ValueError("Unsupported continuation version: run was created by a newer Specify version") + raise ValueError(f"Unsupported continuation version: {version!r}") + _validate_sequence(data.get("sequence"), run_dir=run_dir, root_steps=root_steps) + + +def _validate_sequence( + data: Any, + *, + run_dir: Path, + root_steps: list[dict[str, Any]] | None, +) -> None: + if not isinstance(data, dict): + raise ValueError("Invalid continuation: sequence must be a JSON object") + source = data.get("source") + if not isinstance(source, dict) or source.get("kind") not in _SOURCE_KINDS: + raise ValueError("Invalid continuation: unknown sequence source") + kind = source["kind"] + ref = source.get("snapshot") + if kind == "expansion": + steps = read_expansion_snapshot(run_dir, ref) + elif ref is not None: + raise ValueError("Invalid continuation: workflow source must not have a snapshot") + else: + steps = root_steps + next_index = data.get("next_index") + if isinstance(next_index, bool) or not isinstance(next_index, int) or next_index < 0: + raise ValueError("Invalid continuation: next_index must be a non-negative integer") + if steps is not None and next_index > len(steps): + raise ValueError("Invalid continuation: next_index is outside its sequence") + active = data.get("active") + if active is None: + return + if not isinstance(active, dict): + raise ValueError("Invalid continuation: active must be a JSON object or null") + _validate_path(active.get("activation_path")) + if not isinstance(active.get("step_id"), str) or not active["step_id"]: + raise ValueError("Invalid continuation: active step_id must be a non-empty string") + if not isinstance(active.get("step_type"), str) or not active["step_type"]: + raise ValueError("Invalid continuation: active step_type must be a non-empty string") + attempt = active.get("attempt") + if isinstance(attempt, bool) or not isinstance(attempt, int) or attempt < 1: + raise ValueError("Invalid continuation: active attempt must be a positive integer") + if active.get("phase") not in _PHASES: + raise ValueError("Invalid continuation: unknown activation phase") + if steps is not None: + if next_index >= len(steps): + raise ValueError("Invalid continuation: active activation is past its sequence") + step = steps[next_index] + if active["step_id"] != step.get("id") or active["step_type"] != step.get("type", "command"): + raise ValueError("Invalid continuation: active activation does not match its sequence") + child = active.get("child_sequence") + if active["phase"] == "children": + if child is None: + raise ValueError("Invalid continuation: children activation requires child_sequence") + _validate_sequence(child, run_dir=run_dir, root_steps=None) + elif child is not None: + raise ValueError("Invalid continuation: only children activations may have child_sequence") + + +def sequence_steps(cursor: dict[str, Any], *, root_steps: list[dict[str, Any]], run_dir: Path) -> list[dict[str, Any]]: + """Resolve a cursor source to its immutable full sequence.""" + source = cursor["source"] + if source["kind"] == "workflow": + return root_steps + return read_expansion_snapshot(run_dir, source["snapshot"]) + + +def allows_expansion_index(continuation: dict[str, Any], index: int) -> bool: + """Accept an expansion-relative compatibility index from early cursor runs.""" + sequence = continuation["sequence"] + active = sequence.get("active") + while active is not None and active.get("phase") == "children": + sequence = active["child_sequence"] + if sequence["next_index"] == index and sequence.get("active") is not None: + return True + active = sequence.get("active") + return False + + +def active_leaf(sequence: dict[str, Any]) -> dict[str, Any] | None: + """Return the deepest active activation in a cursor tree.""" + active = sequence.get("active") + if active is None: + return None + child = active.get("child_sequence") + if child is not None: + leaf = active_leaf(child) + if leaf is not None: + return leaf + return active diff --git a/src/specify_cli/workflows/composition.py b/src/specify_cli/workflows/composition.py index 7f73fb5ea5..2507cb923f 100644 --- a/src/specify_cli/workflows/composition.py +++ b/src/specify_cli/workflows/composition.py @@ -15,10 +15,12 @@ from __future__ import annotations import hashlib +import json import math import os import re import tempfile +from copy import deepcopy from dataclasses import dataclass, field from pathlib import Path from typing import TYPE_CHECKING, Any @@ -528,6 +530,7 @@ class ExecutionScope: step_results: dict[str, dict[str, Any]] = field(default_factory=dict) current_step_index: int = 0 current_step_id: str | None = None + continuation: dict[str, Any] | None = None status: RunStatus = RunStatus.RUNNING error: str | None = None # A workflow caller stages a terminal child transition here so its caller @@ -627,6 +630,7 @@ def _serialize(self) -> dict[str, Any]: "error": self.error, "current_step_index": self.current_step_index, "current_step_id": self.current_step_id, + "continuation": self.continuation, "step_results": self.step_results, "workflow_scopes": { key: child._serialize() @@ -652,6 +656,7 @@ def _sync_to_state(self, state: RunState) -> None: state.error = self.error state.current_step_id = self.current_step_id state.current_step_index = self.current_step_index + state.continuation = self.continuation state.step_results = self.step_results state.workflow_scopes = { key: child._serialize() @@ -714,6 +719,137 @@ def record_and_save( root._sync_to_state(state) state._save_locked() + def commit_cursor_result( + self, + context: StepContext, + step_id: str, + data: dict[str, Any], + *, + sequence: dict[str, Any], + phase: str | None, + child_sequence: dict[str, Any] | None, + root_index: int, + current_step_id: str, + status: RunStatus, + error: str | None = None, + child_scope_id: str | None = None, + complete_child: bool = False, + ) -> None: + """Commit a cursor outcome and its caller/child result in one state write.""" + root = self.root() + state = root.root_state + if state is None: # pragma: no cover - cursor execution has a run state + raise ValueError("Cursor execution requires a run state") + with state._lock: + child = self.workflow_scopes.get(child_scope_id) if child_scope_id else None + missing = object() + previous_result = self.step_results.get(step_id, missing) + previous_context = context.steps.get(step_id, missing) + previous_sequence = deepcopy(sequence) + previous_fields = ( + self.current_step_index, self.current_step_id, self.status, self.error + ) + previous_child = ( + child.status, child.error, + child.pending_terminal_status, child.pending_terminal_error, + ) if child is not None else None + try: + if child is not None: + if complete_child: + child.status = RunStatus.COMPLETED + elif child.pending_terminal_status is not None: + child.status = child.pending_terminal_status + child.error = child.pending_terminal_error + child.pending_terminal_status = None + child.pending_terminal_error = None + if context.steps is not self.step_results: + context.steps[step_id] = data + self.step_results[step_id] = data + if phase is None: + sequence["active"] = None + sequence["next_index"] += 1 + else: + active = sequence["active"] + active["phase"] = phase + if child_sequence is not None: + active["child_sequence"] = child_sequence + self.current_step_index = root_index + self.current_step_id = current_step_id + self.status = status + self.error = error + root._sync_to_state(state) + state._save_locked() + except BaseException: + # A write can fail after os.replace() has already committed + # state.json (for example while writing the inputs mirror). + # Keep that committed transition authoritative in memory so + # outer exception handling cannot persist an old cursor over it. + try: + disk = json.loads((state.runs_dir / "state.json").read_text(encoding="utf-8")) + except (OSError, ValueError): + disk = {} + if ( + disk.get("continuation") == state.continuation + and disk.get("step_results") == state.step_results + and disk.get("workflow_scopes") == state.workflow_scopes + and disk.get("status") == state.status.value + and disk.get("inputs") == state.inputs + ): + raise + sequence.clear() + sequence.update(previous_sequence) + if previous_result is missing: + self.step_results.pop(step_id, None) + else: + self.step_results[step_id] = previous_result + if context.steps is not self.step_results: + if previous_context is missing: + context.steps.pop(step_id, None) + else: + context.steps[step_id] = previous_context + (self.current_step_index, self.current_step_id, self.status, self.error) = previous_fields + if child is not None and previous_child is not None: + (child.status, child.error, + child.pending_terminal_status, child.pending_terminal_error) = previous_child + root._sync_to_state(state) + raise + + def complete_cursor_expansion( + self, sequence: dict[str, Any], *, root_index: int, step_id: str + ) -> None: + """Persist parent advancement after its child sequence is exhausted.""" + root = self.root() + state = root.root_state + if state is None: # pragma: no cover - cursor execution has a run state + raise ValueError("Cursor execution requires a run state") + with state._lock: + old_index = sequence["next_index"] + old_active = sequence["active"] + old_projection = self.current_step_index, self.current_step_id + sequence["active"] = None + sequence["next_index"] += 1 + self.current_step_index = root_index + self.current_step_id = step_id + root._sync_to_state(state) + try: + state._save_locked() + except BaseException: + try: + disk = json.loads((state.runs_dir / "state.json").read_text(encoding="utf-8")) + except (OSError, ValueError): + disk = {} + if ( + disk.get("continuation") == state.continuation + and disk.get("workflow_scopes") == state.workflow_scopes + and disk.get("status") == state.status.value + ): + raise + sequence["next_index"] = old_index + sequence["active"] = old_active + self.current_step_index, self.current_step_id = old_projection + root._sync_to_state(state) + raise + def deserialize_scope( record: dict[str, Any], @@ -737,6 +873,7 @@ def deserialize_scope( step_results=record.get("step_results", {}) or {}, current_step_index=record.get("current_step_index", 0), current_step_id=record.get("current_step_id"), + continuation=record.get("continuation"), status=RunStatus(record.get("status", RunStatus.RUNNING.value)), error=record.get("error"), parent=parent, @@ -930,6 +1067,16 @@ def _validate_scope_record( definition = _resolve_scope_definition(record, run_dir=run_dir, path=path) _validate_definition_shape(definition, path=f"{path}.definition") + continuation = record.get("continuation") + if continuation is not None: + from ._continuation import validate_continuation + + if run_dir is None: # pragma: no cover - caller always provides it + raise ValueError("Invalid run state: continuation requires a run directory") + validate_continuation( + continuation, run_dir=run_dir, root_steps=definition["steps"] + ) + # A nested scope resumes by slicing its persisted definition at # ``current_step_index``; an index at or beyond the step count would # otherwise yield an empty slice and let the scope silently complete @@ -937,7 +1084,12 @@ def _validate_scope_record( # ``WorkflowEngine.resume``, which ``RunState.load`` cannot apply until the # definition (and its step count) is known. steps = definition["steps"] - if index >= len(steps): + if continuation is not None: + from ._continuation import allows_expansion_index + + if index >= len(steps) and ( + continuation is None or not allows_expansion_index(continuation, index) + ): msg = ( f"Invalid run state: '{path}.current_step_index' ({index}) is " f"out of range for workflow {workflow_id!r} with " diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index c2f0c1a656..32991683c8 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -28,6 +28,13 @@ default_integration_key, try_read_integration_json, ) +from ._continuation import ( + allows_expansion_index, + new_continuation, + sequence_steps, + validate_continuation, + write_expansion_snapshot, +) from .base import RunStatus, StepContext, StepResult, StepStatus from .composition import ( ExecutionScope, @@ -719,6 +726,9 @@ def __init__( self.status = RunStatus.CREATED self.current_step_index = 0 self.current_step_id: str | None = None + # Private execution authority for cursor-backed runs. The public + # current_step fields remain compatibility projections. + self.continuation: dict[str, Any] | 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 @@ -798,12 +808,14 @@ def _save_locked(self) -> None: "status": self.status.value, "current_step_index": self.current_step_index, "current_step_id": self.current_step_id, + "continuation": self.continuation, "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, + "inputs": self.inputs, } self._atomic_write_json(runs_dir / "state.json", state_data) self._atomic_write_json(runs_dir / "inputs.json", {"inputs": self.inputs}) @@ -903,6 +915,21 @@ def load(cls, run_id: str, project_root: Path) -> RunState: workflow_scopes = state_data.get("workflow_scopes", {}) validate_serialized_scopes(workflow_scopes, run_dir=runs_dir) + continuation = state_data.get("continuation") + if continuation is not None: + root_steps = None + workflow_copy = runs_dir / "workflow.yml" + if workflow_copy.is_file(): + try: + root_data = yaml.safe_load(workflow_copy.read_text(encoding="utf-8")) + except (OSError, yaml.YAMLError) as exc: + raise ValueError("Invalid continuation: unable to load workflow snapshot") from exc + if isinstance(root_data, dict) and isinstance(root_data.get("steps"), list): + root_steps = root_data["steps"] + validate_continuation( + continuation, run_dir=runs_dir, root_steps=root_steps + ) + state = cls( run_id=state_data["run_id"], workflow_id=workflow_id, @@ -929,6 +956,9 @@ 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") + # States created before continuations deliberately retain their legacy + # coarse resume behavior until their first resumed activation. + state.continuation = continuation state.step_results = step_results state.workflow_scopes = workflow_scopes state.workflow_dir = state_data.get("workflow_dir") @@ -936,8 +966,13 @@ def load(cls, run_id: str, project_root: Path) -> RunState: state.updated_at = state_data.get("updated_at", "") state.error = state_data.get("error") + inputs = state_data.get("inputs") + if inputs is not None: + if not isinstance(inputs, dict): + raise ValueError("Invalid run state: 'inputs' must be a JSON object") + state.inputs = inputs inputs_path = runs_dir / "inputs.json" - if inputs_path.exists(): + if inputs is None and inputs_path.exists(): with open(inputs_path, encoding="utf-8") as f: inputs_data = json.load(f) if not isinstance(inputs_data, dict): @@ -1111,6 +1146,7 @@ def execute( ) state.workflow_dir = workflow_dir state.status = RunStatus.RUNNING + state.continuation = new_continuation() state.save() context = StepContext( @@ -1127,7 +1163,9 @@ def execute( # Execute steps try: - self._execute_steps(definition.steps, context, scope, STEP_REGISTRY) + self._execute_cursor_sequence( + definition.steps, context, scope, STEP_REGISTRY, state.continuation + ) except KeyboardInterrupt: scope.status = RunStatus.PAUSED scope.append_log({"event": "workflow_interrupted"}) @@ -1180,7 +1218,10 @@ def resume( # index (e.g. a hand-edited state.json) would otherwise slice # definition.steps[state.current_step_index:] into an empty list # below, silently completing the run without executing any step. - if state.current_step_index >= len(definition.steps): + if state.current_step_index >= len(definition.steps) and ( + state.continuation is None + or not allows_expansion_index(state.continuation, state.current_step_index) + ): msg = ( "Invalid run state: 'current_step_index' " f"({state.current_step_index}) is out of range for " @@ -1214,23 +1255,28 @@ def resume( from . import STEP_REGISTRY - state.error = None - 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 :] - step_offset = state.current_step_index + state.error = None + state.status = RunStatus.RUNNING + scope.error = None + scope.status = RunStatus.RUNNING try: - self._execute_steps( - remaining_steps, context, scope, STEP_REGISTRY, - step_offset=step_offset, - ) + if scope.continuation is None: + # Legacy runs and the not-yet-cursor-backed loop/fan-out + # state machines retain the prior coarse replay behavior. + self._execute_steps( + definition.steps[state.current_step_index :], + context, + scope, + STEP_REGISTRY, + step_offset=state.current_step_index, + ) + else: + self._execute_cursor_sequence( + definition.steps, context, scope, STEP_REGISTRY, scope.continuation + ) except KeyboardInterrupt: scope.status = RunStatus.PAUSED scope.append_log({"event": "workflow_interrupted"}) @@ -1279,6 +1325,7 @@ def _build_root_scope( step_results=state.step_results, current_step_index=state.current_step_index, current_step_id=state.current_step_id, + continuation=state.continuation, status=state.status, error=state.error, root_state=state, @@ -1308,6 +1355,7 @@ def _scope_or_wrap(target: Any) -> tuple[ExecutionScope, RunState | None]: step_results=state.step_results, current_step_index=state.current_step_index, current_step_id=state.current_step_id, + continuation=state.continuation, status=state.status, error=state.error, root_state=state, @@ -1327,11 +1375,211 @@ def _sync_wrapped(state: RunState | None, scope: ExecutionScope) -> None: state.error = scope.error state.current_step_id = scope.current_step_id state.current_step_index = scope.current_step_index + state.continuation = scope.continuation state.workflow_scopes = { key: child._serialize() for key, child in scope.workflow_scopes.items() } + def _cursor_step_data( + self, + step_config: dict[str, Any], + step_type: str, + result: StepResult, + context: StepContext, + ) -> dict[str, Any]: + """Build the persisted result record shared by cursor executions.""" + if step_type == "workflow" and result.status == StepStatus.FAILED: + recorded_input = result.output.get("input", {}) + else: + recorded_input = result.output.get("input") or step_config.get("input", {}) + data = { + "type": step_type, + "integration": result.output.get("integration") + or step_config.get("integration") + or context.default_integration, + "model": result.output.get("model") + or step_config.get("model") + or context.default_model, + "options": result.output.get("options") or step_config.get("options", {}), + "input": recorded_input, + "output": result.output, + "status": result.status.value, + "error": result.error, + } + if step_type == "command" and "integration_args" in result.output: + data["integration_args"] = result.output["integration_args"] + data["integration_options"] = result.output["integration_options"] + return data + + @staticmethod + def _cursor_path(parent: list[dict[str, Any]], step_id: str) -> list[dict[str, Any]]: + return [*parent, {"kind": "step", "id": step_id}] + + def _execute_cursor_sequence( + self, + root_steps: list[dict[str, Any]], + context: StepContext, + scope: ExecutionScope, + registry: dict[str, Any], + continuation: dict[str, Any] | None, + sequence: dict[str, Any] | None = None, + path: list[dict[str, Any]] | None = None, + invocation_path: tuple[str, ...] = (), + ) -> None: + """Execute one durable sequential cursor tree. + + This handles ordinary steps and finite ``next_steps`` expansions. Loops + and fan-out retain their established executor until their dedicated + cursor frames are introduced; keeping that boundary explicit avoids + silently claiming exact item/iteration resume semantics. + """ + if continuation is None: + raise ValueError("Cursor-backed execution requires a continuation") + sequence = sequence or continuation["sequence"] + path = path or [] + steps = sequence_steps(sequence, root_steps=root_steps, run_dir=scope.root().root_state.runs_dir) + + while sequence["next_index"] < len(steps): + index = sequence["next_index"] + step_config = steps[index] + step_id = step_config.get("id", f"step-{index}") + step_type = step_config.get("type", "command") + active = sequence.get("active") + + if active is not None and active.get("phase") == "children": + child = active["child_sequence"] + nested_path = (*invocation_path, step_id) + self._execute_cursor_sequence( + root_steps, context, scope, registry, continuation, child, + active["activation_path"], nested_path, + ) + if scope.status in (RunStatus.PAUSED, RunStatus.FAILED, RunStatus.ABORTED): + return + root_index = continuation["sequence"]["next_index"] + if sequence is continuation["sequence"]: + root_index += 1 + scope.complete_cursor_expansion( + sequence, root_index=min(root_index, len(root_steps) - 1), step_id=step_id + ) + continue + + # Dedicated loop and fan-out frames are intentionally deferred. Do + # not partially persist a generic cursor then hand execution to a + # state machine that cannot restore it. + if step_type in {"while", "do-while", "fan-out"}: + scope.continuation = None + scope.persist() + self._execute_steps( + steps[index:], context, scope, registry, + step_offset=index if sequence is continuation["sequence"] else -1, + invocation_path=invocation_path, + ) + return + + if active is None: + active = { + "activation_path": self._cursor_path(path, step_id), + "step_id": step_id, + "step_type": step_type, + "attempt": 1, + "phase": "running", + } + sequence["active"] = active + else: + active["attempt"] += 1 + active["phase"] = "running" + scope.current_step_index = min(continuation["sequence"]["next_index"], len(root_steps) - 1) + scope.current_step_id = step_id + scope.status = RunStatus.RUNNING + scope.error = None + scope.persist() + scope.append_log({"event": "step_started", "step_id": step_id, "type": step_type}) + label = step_config.get("command", "") or step_type + if self.on_step_start is not None: + with self._callback_lock: + self.on_step_start(step_id, label) + + step_impl = registry.get(step_type) + if step_impl is None: + result = StepResult( + status=StepStatus.FAILED, + error=f"Unknown step type: {step_type!r}", + ) + else: + try: + result = ( + self._run_workflow_call( + step_config, context, scope, registry, step_impl, invocation_path + ) + if step_type == "workflow" + else step_impl.execute(step_config, context) + ) + except KeyboardInterrupt: + active["phase"] = "interrupted" + scope.status = RunStatus.PAUSED + scope.persist() + raise + except Exception as exc: + active["phase"] = "failed" + scope.status = RunStatus.FAILED + scope.error = str(exc) + scope.persist() + raise + + data = self._cursor_step_data(step_config, step_type, result, context) + blocked = result.status == StepStatus.PAUSED or ( + result.status == StepStatus.FAILED + and (result.output.get("aborted") or step_config.get("continue_on_error") is not True) + ) + status = ( + RunStatus.PAUSED if result.status == StepStatus.PAUSED + else RunStatus.ABORTED if result.output.get("aborted") + else RunStatus.FAILED if blocked else RunStatus.RUNNING + ) + phase = ( + "paused" if result.status == StepStatus.PAUSED else "failed" + ) if blocked else None + child_sequence = None + # Failed/paused outcomes take precedence over any returned children, + # including handled failures, matching the legacy executor. + if result.next_steps and result.status not in (StepStatus.FAILED, StepStatus.PAUSED): + ref, _ = write_expansion_snapshot(scope.root().root_state.runs_dir, result.next_steps) + child_sequence = { + "source": {"kind": "expansion", "snapshot": ref}, + "next_index": 0, + "active": None, + } + phase = "children" + root_index = min(continuation["sequence"]["next_index"], len(root_steps) - 1) + if sequence is continuation["sequence"] and phase is None: + root_index = min(index + 1, len(root_steps) - 1) + scope.commit_cursor_result( + context, step_id, data, sequence=sequence, + phase=phase, child_sequence=child_sequence, + root_index=root_index, + current_step_id=step_id, status=status, + error=result.error if status in (RunStatus.FAILED, RunStatus.ABORTED) else None, + child_scope_id=":".join([*invocation_path, step_id]) if step_type == "workflow" else None, + complete_child=step_type == "workflow" and result.status == StepStatus.COMPLETED, + ) + scope.append_log({"event": "step_completed", "step_id": step_id, "status": result.status.value}) + if result.status == StepStatus.FAILED: + # Outcome events follow the atomic result/cursor transition so + # logging errors cannot make committed work replay on resume. + if result.output.get("aborted"): + scope.append_log({"event": "workflow_aborted", "step_id": step_id}) + else: + scope.append_log({ + "event": "step_failed" if blocked else "step_continue_on_error", + "step_id": step_id, + "error": result.error, + }) + if blocked: + return + if child_sequence is not None: + continue + def _execute_steps( self, steps: list[dict[str, Any]], @@ -1741,16 +1989,50 @@ def _run_workflow_call( 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) try: - self._execute_steps( - definition.steps[start:], - child_context, - child_scope, - registry, - step_offset=start, - ) + if child_scope.continuation is None: + self._execute_steps( + definition.steps[child_scope.current_step_index :], + child_context, + child_scope, + registry, + step_offset=child_scope.current_step_index, + ) + elif context.inside_fan_out: + # Fan-out owns a separate item state machine. Until its + # per-item cursor frames are implemented, retain the + # established coarse retry semantics for composed items. + # A child may have a cursor paused inside a nested + # expansion, whose compatibility index is relative to + # that expansion rather than the child definition. + # Restarting this legacy fan-out item therefore starts at + # its containing top-level operation. + child_scope.continuation = None + child_scope.current_step_index = 0 + self._execute_steps( + definition.steps, + child_context, + child_scope, + registry, + step_offset=0, + ) + elif ( + child_scope.continuation["sequence"]["next_index"] + >= len(definition.steps) + ): + # The child sequence completed but evaluation of declared + # outputs failed. Retrying this caller activation must + # reevaluate outputs, not replay the child's last step. + pass + else: + self._execute_cursor_sequence( + definition.steps, + child_context, + child_scope, + registry, + child_scope.continuation, + ) except Exception as exc: # noqa: BLE001 - isolate child runtime failures error = ( f"Workflow step {effective_id!r}: workflow " @@ -1832,6 +2114,7 @@ def _run_workflow_call( status=RunStatus.RUNNING, parent=scope, root_state=scope.root().root_state, + continuation=None if context.inside_fan_out else new_continuation(), ) # Persist the resolved definition as an immutable YAML snapshot before # referencing it from state.json. YAML round-trips native scalars @@ -1846,9 +2129,18 @@ def _run_workflow_call( child_context = child_scope.build_context(is_resume=False) try: - self._execute_steps( - definition.steps, child_context, child_scope, registry, step_offset=0 - ) + if child_scope.continuation is None: + self._execute_steps( + definition.steps, child_context, child_scope, registry, step_offset=0 + ) + else: + self._execute_cursor_sequence( + definition.steps, + child_context, + child_scope, + registry, + child_scope.continuation, + ) except Exception as exc: # noqa: BLE001 - isolate child runtime failures error = ( f"Workflow step {effective_id!r}: workflow " @@ -1892,6 +2184,14 @@ def _aggregate_workflow_result( ) child_scope.status = RunStatus.FAILED child_scope.error = error + # Preserve the pre-cursor output retry boundary: rerun only the + # last top-level operation, which may produce a usable value on + # retry, and retain the completed prefix. Dedicated + # ``workflow_outputs`` frames will replace this reset. + if child_scope.continuation is not None: + child_scope.continuation["sequence"]["next_index"] = max( + len(definition.steps) - 1, 0 + ) return StepResult( status=StepStatus.FAILED, output={ @@ -2125,6 +2425,24 @@ def item_halt_status(idx: int) -> RunStatus | None: scope.error = halt_rec.get("error") self._sync_wrapped(wrap_state, scope) return slots[: halted_at + 1] + # The legacy concurrent fan-out coordinator attributes a halt from the + # template step's direct record. A template can itself be a completed + # control-flow parent whose nested workflow call aborted, so that record + # does not expose the terminal outcome. Preserve the existing composed + # workflow behavior until fan-out gets branch-local cursors. + if scope.status == RunStatus.PAUSED: + aborted_child = next( + ( + child + for key, child in scope.workflow_scopes.items() + if key.startswith(f"{step_id}:") + and child.status == RunStatus.ABORTED + ), + None, + ) + if aborted_child is not None: + scope.status = RunStatus.ABORTED + scope.error = aborted_child.error self._sync_wrapped(wrap_state, scope) return slots[:collected] diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 2ef12a0656..49ec0fbcf3 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -10248,6 +10248,370 @@ def _engine(self, project_dir): from specify_cli.workflows.engine import WorkflowEngine return WorkflowEngine(project_dir) + @pytest.mark.parametrize("outcome", ["failed", "paused", "handled", "aborted"]) + def test_unsuccessful_expansion_preserves_outcome(self, project_dir, monkeypatch, outcome): + from specify_cli.workflows import STEP_REGISTRY + from specify_cli.workflows.base import StepBase, StepResult, StepStatus + from specify_cli.workflows.engine import RunState, WorkflowDefinition + + calls = [] + + class Expand(StepBase): + def execute(self, config, context): + calls.append(config["id"]) + if config["id"] == "expand": + return StepResult( + status=( + StepStatus.COMPLETED if context.inputs["ready"] + else StepStatus.PAUSED if outcome == "paused" + else StepStatus.FAILED + ), + output={"aborted": True} if outcome == "aborted" else {}, + error="not ready" if not context.inputs["ready"] else None, + next_steps=[{"id": "child", "type": "expand"}], + ) + return StepResult() + + monkeypatch.setitem(STEP_REGISTRY, "expand", Expand()) + definition = WorkflowDefinition({ + "schema_version": "1.0", + "workflow": {"id": "expansion-outcome", "name": "Expansion outcome", "version": "1.0.0"}, + "inputs": {"ready": {"type": "boolean", "default": False}}, + "steps": [ + {"id": "expand", "type": "expand", "continue_on_error": outcome in {"handled", "aborted"}}, + {"id": "after", "type": "expand"}, + ], + }) + engine = self._engine(project_dir) + state = engine.execute(definition) + loaded = RunState.load(state.run_id, project_dir) + assert loaded.status.value == ("completed" if outcome == "handled" else outcome) + assert calls == (["expand", "after"] if outcome == "handled" else ["expand"]) + if outcome == "aborted": + active = loaded.continuation["sequence"]["active"] + assert active["phase"] == "failed" + assert active.get("child_sequence") is None + if outcome in {"failed", "paused"}: + active = loaded.continuation["sequence"]["active"] + assert active["phase"] == outcome + assert active.get("child_sequence") is None + # An unchanged resume retries the blocker rather than its children. + assert engine.resume(state.run_id).status.value == outcome + assert calls == ["expand", "expand"] + assert engine.resume(state.run_id, {"ready": True}).status.value == "completed" + assert calls == ["expand", "expand", "expand", "child", "after"] + + @pytest.mark.parametrize( + ("outcome", "event", "expected_status"), + [ + ("failed", "step_failed", "failed"), + ("handled", "step_continue_on_error", "running"), + ("aborted", "workflow_aborted", "aborted"), + ], + ) + def test_cursor_failure_resolution_logged_after_commit( + self, project_dir, monkeypatch, outcome, event, expected_status + ): + from specify_cli.workflows import STEP_REGISTRY + from specify_cli.workflows.base import StepBase, StepResult, StepStatus + from specify_cli.workflows.composition import ExecutionScope + from specify_cli.workflows.engine import RunState, WorkflowDefinition + + class Fail(StepBase): + def execute(self, config, context): + return StepResult( + status=StepStatus.FAILED, error="expected failure", + output={"aborted": True} if outcome == "aborted" else {}, + ) + + monkeypatch.setitem(STEP_REGISTRY, "fail", Fail()) + original = ExecutionScope.append_log + snapshots = [] + + def inspect_commit(self, entry): + if entry["event"] == event: + snapshots.append(RunState.load(self.root().root_state.run_id, project_dir)) + return original(self, entry) + + monkeypatch.setattr(ExecutionScope, "append_log", inspect_commit) + definition = WorkflowDefinition({ + "schema_version": "1.0", + "workflow": {"id": "failure-log", "name": "Failure log", "version": "1.0.0"}, + "steps": [{"id": "fail", "type": "fail", "continue_on_error": outcome != "failed"}], + }) + state = self._engine(project_dir).execute(definition) + events = [json.loads(line) for line in (state.runs_dir / "log.jsonl").read_text().splitlines()] + resolutions = [entry for entry in events if entry["event"] in { + "step_failed", "step_continue_on_error", "workflow_aborted" + }] + assert len(resolutions) == 1 + assert resolutions[0]["event"] == event + assert resolutions[0]["step_id"] == "fail" + if outcome != "aborted": + assert resolutions[0]["error"] == "expected failure" + assert len(snapshots) == 1 + assert snapshots[0].status.value == expected_status + assert snapshots[0].step_results["fail"]["status"] == "failed" + assert snapshots[0].continuation["sequence"]["next_index"] == (1 if outcome == "handled" else 0) + + def test_nested_expansion_resumes_without_replaying_parent_or_prefix( + self, project_dir, monkeypatch + ): + """A persisted branch expansion is authoritative after its child pauses.""" + from specify_cli.workflows import STEP_REGISTRY + from specify_cli.workflows.base import StepBase, StepResult, StepStatus + from specify_cli.workflows.engine import WorkflowDefinition + + calls: list[str] = [] + + class _Count(StepBase): + type_key = "count" + + def execute(self, config, context): + calls.append(config["id"]) + if config.get("pause") and context.inputs["verdict"] != "approve": + return StepResult(status=StepStatus.PAUSED) + return StepResult(status=StepStatus.COMPLETED) + + monkeypatch.setitem(STEP_REGISTRY, "count", _Count()) + definition = WorkflowDefinition.from_string( + """ +schema_version: "1.0" +workflow: + id: exact-branch-resume + name: Exact branch resume + version: "1.0.0" +inputs: + route: + type: string + default: then + verdict: + type: string + default: "" +steps: + - id: choose + type: if + condition: "{{ inputs.route == 'then' }}" + then: + - id: prepare + type: count + - id: review + type: count + pause: true + - id: publish + type: count + else: + - id: wrong-route + type: count + - id: after + type: count +""" + ) + engine = self._engine(project_dir) + + paused = engine.execute(definition) + assert paused.status.value == "paused" + assert calls == ["prepare", "review"] + + resumed = engine.resume(paused.run_id, {"route": "else", "verdict": "approve"}) + assert resumed.status.value == "completed" + assert calls == ["prepare", "review", "review", "publish", "after"] + assert "wrong-route" not in calls + + def test_long_expansion_resumes_with_root_compatibility_index( + self, project_dir, monkeypatch + ): + from specify_cli.workflows import STEP_REGISTRY + from specify_cli.workflows.base import StepBase, StepResult, StepStatus + from specify_cli.workflows.engine import RunState, WorkflowDefinition + + calls = [] + + class Count(StepBase): + type_key = "count" + + def execute(self, config, context): + calls.append(config["id"]) + return StepResult( + status=StepStatus.PAUSED + if config["id"] == "review" and not context.inputs["approved"] + else StepStatus.COMPLETED + ) + + monkeypatch.setitem(STEP_REGISTRY, "count", Count()) + definition = WorkflowDefinition.from_string( + """ +schema_version: "1.0" +workflow: {id: long-branch, name: Long branch, version: "1.0.0"} +inputs: {approved: {type: boolean, default: false}} +steps: + - id: choose + type: if + condition: true + then: + - {id: one, type: count} + - {id: two, type: count} + - {id: three, type: count} + - {id: review, type: count} + - {id: after, type: count} +""" + ) + engine = self._engine(project_dir) + paused = engine.execute(definition) + assert paused.current_step_index == 0 + assert RunState.load(paused.run_id, project_dir).current_step_id == "review" + finished = engine.resume(paused.run_id, {"approved": True}) + assert finished.status.value == "completed" + assert calls == ["one", "two", "three", "review", "review", "after"] + + def test_committed_leaf_survives_completion_log_failure( + self, project_dir, monkeypatch + ): + from specify_cli.workflows import STEP_REGISTRY + from specify_cli.workflows.base import StepBase, StepResult, StepStatus + from specify_cli.workflows.engine import RunState, WorkflowDefinition + + calls = [] + + class Count(StepBase): + type_key = "count" + + def execute(self, config, context): + calls.append(config["id"]) + return StepResult(status=StepStatus.COMPLETED) + + monkeypatch.setitem(STEP_REGISTRY, "count", Count()) + definition = WorkflowDefinition.from_string( + """ +schema_version: "1.0" +workflow: {id: logging-fault, name: Logging fault, version: "1.0.0"} +steps: + - {id: first, type: count} + - {id: second, type: count} +""" + ) + engine = self._engine(project_dir) + from specify_cli.workflows.composition import ExecutionScope + + original = ExecutionScope.append_log + raised = False + + def fail_once(self, entry): + nonlocal raised + if entry.get("event") == "step_completed" and not raised: + raised = True + raise RuntimeError("log failed") + return original(self, entry) + + monkeypatch.setattr(ExecutionScope, "append_log", fail_once) + with pytest.raises(RuntimeError, match="log failed"): + engine.execute(definition, run_id="logging-fault-run") + state = RunState.load("logging-fault-run", project_dir) + assert state.continuation["sequence"]["next_index"] == 1 + assert state.step_results["first"]["status"] == "completed" + assert engine.resume(state.run_id).status.value == "completed" + assert calls == ["first", "second"] + + def test_failed_cursor_save_does_not_commit_unadvanced_result( + self, project_dir, monkeypatch + ): + from specify_cli.workflows import STEP_REGISTRY + from specify_cli.workflows.base import StepBase, StepResult, StepStatus + from specify_cli.workflows.engine import RunState, WorkflowDefinition + + calls = [] + + class Count(StepBase): + type_key = "count" + + def execute(self, config, context): + calls.append(config["id"]) + return StepResult(status=StepStatus.COMPLETED) + + monkeypatch.setitem(STEP_REGISTRY, "count", Count()) + definition = WorkflowDefinition.from_string( + """ +schema_version: "1.0" +workflow: {id: save-fault, name: Save fault, version: "1.0.0"} +steps: + - {id: first, type: count} + - {id: second, type: count} +""" + ) + original = RunState._save_locked + failed = False + + def fail_before_write(self): + nonlocal failed + if self.step_results.get("first") and not failed: + failed = True + raise OSError("state write failed") + return original(self) + + monkeypatch.setattr(RunState, "_save_locked", fail_before_write) + engine = self._engine(project_dir) + with pytest.raises(OSError, match="state write failed"): + engine.execute(definition, run_id="save-fault-run") + loaded = RunState.load("save-fault-run", project_dir) + assert loaded.continuation["sequence"]["next_index"] == 0 + assert "first" not in loaded.step_results + assert engine.resume(loaded.run_id).status.value == "completed" + assert calls == ["first", "first", "second"] + + def test_inputs_mirror_failure_preserves_committed_cursor( + self, project_dir, monkeypatch + ): + from specify_cli.workflows import STEP_REGISTRY + from specify_cli.workflows.base import StepBase, StepResult, StepStatus + from specify_cli.workflows.engine import RunState, WorkflowDefinition + + calls = [] + + class Count(StepBase): + type_key = "count" + + def execute(self, config, context): + calls.append(config["id"]) + return StepResult(status=StepStatus.COMPLETED) + + monkeypatch.setitem(STEP_REGISTRY, "count", Count()) + definition = WorkflowDefinition.from_string( + """ +schema_version: "1.0" +workflow: {id: mirror-fault, name: Mirror fault, version: "1.0.0"} +steps: + - {id: first, type: count} + - {id: second, type: count} +""" + ) + original = RunState._atomic_write_json + failed = False + + def fail_mirror(path, data): + nonlocal failed + if path.name == "inputs.json" and "first" in data_state["step_results"] and not failed: + failed = True + raise OSError("mirror write failed") + return original(path, data) + + engine = self._engine(project_dir) + data_state = None + # The mirror write receives only inputs; inspect the committed state + # alongside it to identify the first completed-step transition. + def fail_after_state_write(path, data): + nonlocal data_state + if path.name == "state.json": + data_state = data + return fail_mirror(path, data) + + monkeypatch.setattr(RunState, "_atomic_write_json", staticmethod(fail_after_state_write)) + with pytest.raises(OSError, match="mirror write failed"): + engine.execute(definition, run_id="mirror-fault-run") + loaded = RunState.load("mirror-fault-run", project_dir) + assert loaded.continuation["sequence"]["next_index"] == 1 + assert engine.resume(loaded.run_id).status.value == "completed" + assert calls == ["first", "second"] + def test_resume_with_input_reruns_step_with_new_value(self, project_dir): from specify_cli.workflows.engine import WorkflowDefinition from specify_cli.workflows.base import RunStatus diff --git a/tests/workflows/test_workflow_composition.py b/tests/workflows/test_workflow_composition.py index a96df681a5..c7fb16957d 100644 --- a/tests/workflows/test_workflow_composition.py +++ b/tests/workflows/test_workflow_composition.py @@ -1642,7 +1642,10 @@ def test_failed_output_evaluation_retries_on_resume(self, project_dir): "child", _workflow( "child", - [_shell("x", f"test -f {marker} && printf '{{\"ok\": true}}' || printf bad")], + [ + _shell("prepare", "echo prepared"), + _shell("x", f"test -f {marker} && printf '{{\"ok\": true}}' || printf bad"), + ], outputs={"parsed": {"value": "{{ steps.x.output.stdout | from_json }}"}}, ), ) @@ -1651,15 +1654,23 @@ def test_failed_output_evaluation_retries_on_resume(self, project_dir): "parent", _workflow("parent", [{"id": "c", "type": "workflow", "workflow": "child"}]), ) + calls = [] engine = WorkflowEngine(project_dir) + engine.on_step_start = lambda step_id, label: calls.append(step_id) state = engine.execute(_definition(project_dir, "parent"), {}) assert state.status == RunStatus.FAILED assert state.workflow_scopes["c"]["status"] == "failed" + assert calls == ["c", "prepare", "x"] + + # Repeated output failures must keep the completed prefix intact. + assert engine.resume(state.run_id).status == RunStatus.FAILED + assert calls == ["c", "prepare", "x", "c", "x"] marker.touch() resumed = engine.resume(state.run_id) assert resumed.status == RunStatus.COMPLETED assert resumed.step_results["c"]["output"]["parsed"] == {"ok": True} + assert calls == ["c", "prepare", "x", "c", "x", "c", "x"] def test_resume_uses_definition_snapshot(self, project_dir): self._paused_child( @@ -1854,7 +1865,7 @@ def test_concurrent_save_cannot_persist_unpaired_rebind_failure( assert state.status == RunStatus.PAUSED snapshots: list[dict] = [] - real_record_and_save = ExecutionScope.record_and_save + real_commit = ExecutionScope.commit_cursor_result def coordinated_handoff(self, context, step_id, data, **kwargs): if step_id == "c": @@ -1869,9 +1880,9 @@ def coordinated_handoff(self, context, step_id, data, **kwargs): ) ) ) - return real_record_and_save(self, context, step_id, data, **kwargs) + return real_commit(self, context, step_id, data, **kwargs) - monkeypatch.setattr(ExecutionScope, "record_and_save", coordinated_handoff) + monkeypatch.setattr(ExecutionScope, "commit_cursor_result", coordinated_handoff) resumed = engine.resume(state.run_id, {"mode": "invalid"}) assert resumed.status == RunStatus.COMPLETED @@ -1967,6 +1978,200 @@ def test_root_input_update_propagates_through_nested_calls(self, project_dir): class TestRepeatedCalls: + def test_same_named_calls_in_separate_branches_keep_distinct_scopes( + self, project_dir + ): + _install(project_dir, "child", _workflow( + "child", [_shell("capture", "echo {{ inputs.value }}")], + inputs={"value": {"type": "string", "required": True}}, + )) + parent = _workflow("parent", [ + {"id": branch, "type": "if", "condition": True, "then": [ + {"id": "call", "type": "workflow", "workflow": "child", + "input": {"value": branch}}, + ]} + for branch in ("first", "second") + ]) + state = WorkflowEngine(project_dir).execute(WorkflowDefinition(parent)) + assert state.status == RunStatus.COMPLETED + assert set(state.workflow_scopes) == {"first:call", "second:call"} + assert { + key: record["step_results"]["capture"]["output"]["stdout"].strip() + for key, record in state.workflow_scopes.items() + } == {"first:call": "first", "second:call": "second"} + assert set(RunState.load(state.run_id, project_dir).workflow_scopes) == { + "first:call", "second:call" + } + + def test_later_nested_call_reuses_its_own_scope_on_resume(self, project_dir): + from specify_cli.workflows import STEP_REGISTRY + from specify_cli.workflows.base import StepBase, StepResult + + calls = [] + + class Count(StepBase): + type_key = "count" + + def execute(self, config, context): + calls.append(context.inputs["value"]) + if context.inputs["value"] == "second" and not context.inputs["approved"]: + return StepResult(status=StepStatus.PAUSED) + return StepResult(status=StepStatus.COMPLETED) + + old = STEP_REGISTRY.get("count") + STEP_REGISTRY["count"] = Count() + try: + _install(project_dir, "child", _workflow( + "child", [{"id": "count", "type": "count"}], + inputs={ + "value": {"type": "string", "required": True}, + "approved": {"type": "boolean", "default": False}, + }, + )) + parent = _workflow("parent", [ + {"id": branch, "type": "if", "condition": True, "then": [ + {"id": "call", "type": "workflow", "workflow": "child", + "input": {"value": branch, "approved": "{{ inputs.approved }}"}}, + ]} + for branch in ("first", "second") + ], inputs={"approved": {"type": "boolean", "default": False}}) + engine = WorkflowEngine(project_dir) + state = engine.execute(WorkflowDefinition(parent)) + assert state.status == RunStatus.PAUSED + assert set(state.workflow_scopes) == {"first:call", "second:call"} + assert engine.resume(state.run_id, {"approved": True}).status == RunStatus.COMPLETED + assert calls == ["first", "second", "second"] + finally: + if old is None: + STEP_REGISTRY.pop("count", None) + else: + STEP_REGISTRY["count"] = old + + def test_handled_child_failure_commits_caller_progress(self, project_dir, monkeypatch): + from specify_cli.workflows import STEP_REGISTRY + from specify_cli.workflows.base import StepBase, StepResult + from specify_cli.workflows.composition import ExecutionScope + + calls = [] + + class Fail(StepBase): + type_key = "fail-once" + + def execute(self, config, context): + calls.append("child") + return StepResult(status=StepStatus.FAILED, error="expected failure") + + monkeypatch.setitem(STEP_REGISTRY, "fail-once", Fail()) + _install(project_dir, "child", _workflow( + "child", [{"id": "fail", "type": "fail-once"}] + )) + parent = WorkflowDefinition(_workflow("parent", [ + {"id": "call", "type": "workflow", "workflow": "child", + "continue_on_error": True}, + _shell("after", "echo done"), + ])) + original = ExecutionScope.append_log + raised = False + + def fail_completion_log(self, entry): + nonlocal raised + if entry.get("event") == "step_completed" and entry.get("step_id") == "call" and not raised: + raised = True + raise RuntimeError("log failed") + return original(self, entry) + + monkeypatch.setattr(ExecutionScope, "append_log", fail_completion_log) + engine = WorkflowEngine(project_dir) + with pytest.raises(RuntimeError, match="log failed"): + engine.execute(parent, run_id="handled-failure-run") + loaded = RunState.load("handled-failure-run", project_dir) + assert loaded.continuation["sequence"]["next_index"] == 1 + assert loaded.step_results["call"]["status"] == "failed" + assert loaded.workflow_scopes["call"]["status"] == "failed" + assert engine.resume(loaded.run_id).status == RunStatus.COMPLETED + assert calls == ["child"] + + def test_long_child_expansion_keeps_scope_index_loadable(self, project_dir, monkeypatch): + from specify_cli.workflows import STEP_REGISTRY + from specify_cli.workflows.base import StepBase, StepResult + + calls = [] + + class Count(StepBase): + type_key = "count" + + def execute(self, config, context): + calls.append(config["id"]) + return StepResult( + status=StepStatus.PAUSED + if config["id"] == "review" and not context.inputs["approved"] + else StepStatus.COMPLETED + ) + + monkeypatch.setitem(STEP_REGISTRY, "count", Count()) + _install(project_dir, "child", _workflow( + "child", [{"id": "branch", "type": "if", "condition": True, + "then": [{"id": name, "type": "count"} + for name in ("one", "two", "three", "review", "after")]}], + inputs={"approved": {"type": "boolean", "default": False}}, + )) + parent = WorkflowDefinition(_workflow( + "parent", [{"id": "call", "type": "workflow", "workflow": "child", + "input": {"approved": "{{ inputs.approved }}"}}], + inputs={"approved": {"type": "boolean", "default": False}}, + )) + engine = WorkflowEngine(project_dir) + paused = engine.execute(parent) + assert paused.status == RunStatus.PAUSED + child = RunState.load(paused.run_id, project_dir).workflow_scopes["call"] + assert child["current_step_index"] == 0 + assert child["current_step_id"] == "review" + assert engine.resume(paused.run_id, {"approved": True}).status == RunStatus.COMPLETED + assert calls == ["one", "two", "three", "review", "review", "after"] + + def test_child_handoff_write_failure_retries_without_replaying_child( + self, project_dir, monkeypatch + ): + from specify_cli.workflows import STEP_REGISTRY + from specify_cli.workflows.base import StepBase, StepResult + + calls = [] + + class Count(StepBase): + type_key = "count" + + def execute(self, config, context): + calls.append(config["id"]) + return StepResult(status=StepStatus.COMPLETED) + + monkeypatch.setitem(STEP_REGISTRY, "count", Count()) + _install(project_dir, "child", _workflow( + "child", [{"id": "count", "type": "count"}] + )) + parent = WorkflowDefinition(_workflow( + "parent", [{"id": "call", "type": "workflow", "workflow": "child"}] + )) + real_write = RunState._atomic_write_json + failed = False + + def fail_handoff(path, data): + nonlocal failed + if (path.name == "state.json" and not failed + and data["step_results"].get("call", {}).get("status") == "completed"): + failed = True + raise OSError("handoff write failed") + return real_write(path, data) + + monkeypatch.setattr(RunState, "_atomic_write_json", staticmethod(fail_handoff)) + engine = WorkflowEngine(project_dir) + with pytest.raises(OSError, match="handoff write failed"): + engine.execute(parent, run_id="handoff-fault-run") + saved = RunState.load("handoff-fault-run", project_dir) + assert saved.workflow_scopes["call"]["status"] == "running" + assert "call" not in saved.step_results + assert engine.resume(saved.run_id).status == RunStatus.COMPLETED + assert calls == ["count"] + def test_nested_if_workflow_calls_have_distinct_loop_invocations(self, project_dir): _install( project_dir,